Re: [PATCH v3 1/3] hw/i2c/aspeed: Fix Tx count and Rx size error in buffer pool mode

[Cédric Le Goater]

On 8/16/23 11:44, Hang Yu wrote:
> Hello! Thank you for your review!Sorry I forgot to cc other maintainers so I 
> resend this mail.
>
> From: "Cédric Le Goater" <clg@kaod.org>
> Date: 2023-08-16 16:32:58
> To:  Hang Yu <francis_yuu@stu.pku.edu.cn>,qemu-devel@nongnu.org
> Cc:  komlodi@google.com,peter@pjd.dev,Peter Maydell <peter.maydell@linaro.org>,Andrew Jeffery 
> <andrew@aj.id.au>,Joel Stanley <joel@jms.id.au>,"open list:ASPEED BMCs" 
> <qemu-arm@nongnu.org>,qemu-stable@nongnu.org
> Subject: Re: [PATCH v3 1/3] hw/i2c/aspeed: Fix Tx count and Rx size error in 
> buffer pool mode>On 8/12/23 08:52, Hang Yu wrote:
> > > Fixed inconsistency between the regisiter bit field definition header file
> > > and the ast2600 datasheet. The reg name is I2CD1C:Pool Buffer Control
> > > Register in old register mode and  I2CC0C: Master/Slave Pool Buffer Control
> > > Register in new register mode. They share bit field
> > > [12:8]:Transmit Data Byte Count and bit field
> > > [29:24]:Actual Received Pool Buffer Size according to the datasheet.
> > > According to the ast2600 datasheet,the actual Tx count is
> > > Transmit Data Byte Count plus 1, and the max Rx size is
> > > Receive Pool Buffer Size plus 1, both in Pool Buffer Control Register.
> > > The version before forgot to plus 1, and mistake Rx count for Rx size.
> > >
> > > Signed-off-by: Hang Yu <francis_yuu@stu.pku.edu.cn>
> > > Fixes: 3be3d6ccf2ad ("aspeed: i2c: Migrate to registerfields API")
> >
> > This is -stable material with the following patch. It fixes support for
>  >the v08.06 SDK.
> Should I add this line into the commit in next version?
> >
>  >Reviewed-by: Cédric Le Goater <clg@kaod.org>
> Should I add your Reviewed-by tag and send v4 now?Or just wait for
> other maintainers to reply?

No need for a v4. The tags are pulled automatically with the tooling
we use, b4, pwclient, etc.

I will prepare an aspeed PR for QEMU 8.1 with these patches. Then,
the first two could be backported to -stable.

Thanks,

C.

> Thanks,
> Hang.
> > Thanks,
> >
> > C.
> >
> >
> > > ---
> > > v2-->v3:
> > > 1. Merged patch1 and patch2 in v2
> > > 2. added fixes tag
> > > 3. Fixed typos
> > >
> > >   hw/i2c/aspeed_i2c.c         | 8 ++++----
> > >   include/hw/i2c/aspeed_i2c.h | 4 ++--
> > >   2 files changed, 6 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
> > > index 1f071a3811..e485d8bfb8 100644
> > > --- a/hw/i2c/aspeed_i2c.c
> > > +++ b/hw/i2c/aspeed_i2c.c
> > > @@ -236,7 +236,7 @@ static int aspeed_i2c_bus_send(AspeedI2CBus *bus, uint8_t 
> > > pool_start)
> > >       uint32_t reg_byte_buf = aspeed_i2c_bus_byte_buf_offset(bus);
> > >       uint32_t reg_dma_len = aspeed_i2c_bus_dma_len_offset(bus);
> > >       int pool_tx_count = SHARED_ARRAY_FIELD_EX32(bus->regs, reg_pool_ctrl,
> > > -                                                TX_COUNT);
> > > +                                                TX_COUNT) + 1;
> > >   
> > >       if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, TX_BUFF_EN)) {
> > >           for (i = pool_start; i < pool_tx_count; i++) {
> > > @@ -293,7 +293,7 @@ static void aspeed_i2c_bus_recv(AspeedI2CBus *bus)
> > >       uint32_t reg_dma_len = aspeed_i2c_bus_dma_len_offset(bus);
> > >       uint32_t reg_dma_addr = aspeed_i2c_bus_dma_addr_offset(bus);
> > >       int pool_rx_count = SHARED_ARRAY_FIELD_EX32(bus->regs, reg_pool_ctrl,
> > > -                                                RX_COUNT);
> > > +                                                RX_SIZE) + 1;
> > >   
> > >       if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, RX_BUFF_EN)) {
> > >           uint8_t *pool_base = aic->bus_pool_base(bus);
> > > @@ -418,7 +418,7 @@ static void aspeed_i2c_bus_cmd_dump(AspeedI2CBus *bus)
> > >       uint32_t reg_intr_sts = aspeed_i2c_bus_intr_sts_offset(bus);
> > >       uint32_t reg_dma_len = aspeed_i2c_bus_dma_len_offset(bus);
> > >       if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, RX_BUFF_EN)) {
> > > -        count = SHARED_ARRAY_FIELD_EX32(bus->regs, reg_pool_ctrl, TX_COUNT);
> > > +        count = SHARED_ARRAY_FIELD_EX32(bus->regs, reg_pool_ctrl, TX_COUNT) + 
> > > 1;
> > >       } else if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, RX_DMA_EN)) {
> > >           count = bus->regs[reg_dma_len];
> > >       } else { /* BYTE mode */
> > > @@ -490,7 +490,7 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, 
> > > uint64_t value)
> > >            */
> > >           if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, TX_BUFF_EN)) {
> > >               if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_pool_ctrl, TX_COUNT)
> > > -                == 1) {
> > > +                == 0) {
> > >                   SHARED_ARRAY_FIELD_DP32(bus->regs, reg_cmd, M_TX_CMD, 0);
> > >               } else {
> > >                   /*
> > > diff --git a/include/hw/i2c/aspeed_i2c.h b/include/hw/i2c/aspeed_i2c.h
> > > index 51c944efea..2e1e15aaf0 100644
> > > --- a/include/hw/i2c/aspeed_i2c.h
> > > +++ b/include/hw/i2c/aspeed_i2c.h
> > > @@ -139,9 +139,9 @@ REG32(I2CD_CMD, 0x14) /* I2CD Command/Status */
> > >   REG32(I2CD_DEV_ADDR, 0x18) /* Slave Device Address */
> > >       SHARED_FIELD(SLAVE_DEV_ADDR1, 0, 7)
> > >   REG32(I2CD_POOL_CTRL, 0x1C) /* Pool Buffer Control */
> > > -    SHARED_FIELD(RX_COUNT, 24, 5)
> > > +    SHARED_FIELD(RX_COUNT, 24, 6)
> > >       SHARED_FIELD(RX_SIZE, 16, 5)
> > > -    SHARED_FIELD(TX_COUNT, 9, 5)
> > > +    SHARED_FIELD(TX_COUNT, 8, 5)
> > >       FIELD(I2CD_POOL_CTRL, OFFSET, 2, 6) /* AST2400 */
> > >   REG32(I2CD_BYTE_BUF, 0x20) /* Transmit/Receive Byte Buffer */
> > >       SHARED_FIELD(RX_BUF, 8, 8)