RE: [PATCH v1] hw/i2c/aspeed_i2c: Latch first received byte for SMBus block reads

> Subject: RE: [PATCH v1] hw/i2c/aspeed_i2c: Latch first received byte for SMBus
> block reads
> 
> Hi Jamin,
> 
> Tested-by: Mikail Sadic <[email protected]>
> 
> I verified that this (along with the AST2700 buffer mode patch and the kernel
> fix you referenced) works with the UCD9000 driver, thank you for the help!
> I am dropping my patch and depending on yours.
> 
> 
> I just have two things I'd like to ask about (not blocking for my purposes, may
> be worth to check):
> 
> 1. Byte mode is not latched. I think the else branch of aspeed_i2c_bus_recv()
> still only writes the receive byte buffer, so 0x84 goes stale there. This may
> cause problems down the line, specifically for AST1040?
> 
> 2. I believe dma_buf_en reads like the FUNC_CFG_DMA_EN bit, but the
> DMA-to-pool call passes true while that bit is clear, clearing it is what selects
> the pool. Getting it from RX_DMA_EN inside the helper may stop someone
> from accidentally "fixing" it to match the name and breaking the AST2600
> case.
> 
Hi Mikail,

Thanks for the suggestion.

Both points are addressed in v2:

1. Byte mode now latches as well.
2. dma_buf_en is gone; the helper reads RX_DMA_EN directly, as you suggested.

I resend v2 here,
https://patchwork.kernel.org/project/qemu-devel/patch/[email protected]/

Thanks,
Jamin
> 
> Thanks again for picking this up and catching the original problem.
> 
> - Mikail Sadic
> 
> 
> -----Original Message-----
> From: Jamin Lin <[email protected]>
> Sent: Thursday, August 13, 2026 4:08 AM
> To: Cédric Le Goater <[email protected]>; Peter Maydell
> <[email protected]>; Steven Lee <[email protected]>; Troy
> Lee <[email protected]>; Kane Chen <[email protected]>;
> Andrew Jeffery <[email protected]>; Joel Stanley
> <[email protected]>; open list:ASPEED BMCs <[email protected]>; open
> list:All patches CC here <[email protected]>
> Cc: Troy Lee <[email protected]>; Mikail Sadic
> <[email protected]>
> Subject: [EXTERNAL] RE: [PATCH v1] hw/i2c/aspeed_i2c: Latch first received
> byte for SMBus block reads
> 
> Hi Mikail,
> 
> Could you please review this patch and verify that it works with the UCD9000
> driver?
> 
> Thanks,
> Jamin
> 
> > -----Original Message-----
> > From: Jamin Lin <[email protected]>
> > Sent: Thursday, August 13, 2026 5:02 PM
> > To: Cédric Le Goater <[email protected]>; Peter Maydell
> > <[email protected]>; Steven Lee <[email protected]>;
> > Troy Lee <[email protected]>; Kane Chen <[email protected]>;
> > Andrew Jeffery <[email protected]>; Joel Stanley
> > <[email protected]>; open list:ASPEED BMCs <[email protected]>; open
> > list:All patches CC here <[email protected]>
> > Cc: Jamin Lin <[email protected]>; Troy Lee
> > <[email protected]>; [email protected]
> > Subject: [PATCH v1] hw/i2c/aspeed_i2c: Latch first received byte for
> > SMBus block reads
> >
> > An SMBus block read takes the block length from the first byte of the
> > transfer, and firmware reads that byte back from a register rather
> > than from the transfer buffer. The receive paths never updated those
> > registers, so block reads reported a bogus length.
> >
> > On AST2600 the driver reads the length from the receive byte buffer,
> > I2CC_MS_TXRX_BYTE_BUF[15:8]. The datasheet documents that field as
> > valid while the DMA buffer is not enabled. The byte mode receive path
> > already updated it, but the pool buffer path did not, and the driver
> > selects buffer mode by default.
> >
> > On AST2700 the driver reads the length from offset 0x84 instead.
> >
> > Add I2CC_BYTE_DATA_LOG at 0x84. Latch the first received byte from the
> > three receive paths: the pool buffer path, the DMA-to-pool path and
> > the DMA-to-DRAM path. Each latch updates the byte data log, and
> > updates the receive byte buffer as well while the DMA buffer is not
> > enabled. The byte data log is outside the register window of the
> > earlier SoCs, so it is only visible on AST2700/AST1040.
> >
> > Signed-off-by: Jamin Lin <[email protected]>
> > ---
> >  hw/i2c/aspeed_i2c.c         | 31 ++++++++++++++++++++++++++++++-
> >  include/hw/i2c/aspeed_i2c.h |  2 ++
> >  2 files changed, 32 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c index
> > 68bdcd0e25..e6d0eee816 100644
> > --- a/hw/i2c/aspeed_i2c.c
> > +++ b/hw/i2c/aspeed_i2c.c
> > @@ -159,6 +159,7 @@ static uint64_t
> > aspeed_i2c_bus_new_read(AspeedI2CBus *bus, hwaddr offset,
> >      case A_I2CS_INTR_CTRL:
> >      case A_I2CS_DMA_LEN_STS:
> >      case A_I2CS_INTR_STS:
> > +    case A_I2CC_BYTE_DATA_LOG:
> >      case A_I2CC_VERSION_CTRL:
> >          value = bus->regs[offset / sizeof(*bus->regs)];
> >          break;
> > @@ -334,6 +335,24 @@ static int
> > aspeed_i2c_bus_send_dma_pool(AspeedI2CBus *bus)
> >      return ret;
> >  }
> >
> > +/*
> > + * Latch the first received byte, which firmware reads back as the
> > +SMBus block
> > + * length. AST2600 reads it from the receive byte buffer, only valid
> > +while the
> > + * DMA buffer is disabled; AST2700 reads it from the byte data log, a
> > +register
> > + * the earlier SoCs do not expose.
> > + */
> > +static void aspeed_i2c_bus_latch_rx_len(AspeedI2CBus *bus, bool
> > dma_buf_en,
> > +                                        uint8_t data) {
> > +    uint32_t reg_byte_buf = aspeed_i2c_bus_byte_buf_offset(bus);
> > +
> > +    ARRAY_FIELD_DP32(bus->regs, I2CC_BYTE_DATA_LOG, RX_BUF, data);
> > +
> > +    if (!dma_buf_en) {
> > +        SHARED_ARRAY_FIELD_DP32(bus->regs, reg_byte_buf, RX_BUF,
> > data);
> > +    }
> > +}
> > +
> >  static void aspeed_i2c_bus_recv_dma_pool(AspeedI2CBus *bus)  {
> >      AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(bus->controller);
> > @@ -349,6 +368,9 @@ static void
> > aspeed_i2c_bus_recv_dma_pool(AspeedI2CBus *bus)
> >          pool_base[offset + i] = i2c_recv(bus->bus);
> >          trace_aspeed_i2c_bus_recv("BUFF", i + 1,
> bus->regs[reg_dma_len],
> >                                    pool_base[offset + i]);
> > +        if (i == 0) {
> > +            aspeed_i2c_bus_latch_rx_len(bus, true, pool_base[offset]);
> > +        }
> >          bus->regs[reg_dma_len]--;
> >          ARRAY_FIELD_DP32(bus->regs, I2CM_DMA_LEN_STS, RX_LEN, i +
> 1);
> >      }
> > @@ -443,6 +465,9 @@ static void aspeed_i2c_bus_recv(AspeedI2CBus *bus)
> >              pool_base[i] = i2c_recv(bus->bus);
> >              trace_aspeed_i2c_bus_recv("BUF", i + 1, pool_rx_count,
> >                                        pool_base[i]);
> > +            if (i == 0) {
> > +                aspeed_i2c_bus_latch_rx_len(bus, false, pool_base[0]);
> > +            }
> >          }
> >
> >          /* Update RX count */
> > @@ -460,7 +485,7 @@ static void aspeed_i2c_bus_recv(AspeedI2CBus *bus)
> >          }
> >
> >          aspeed_i2c_set_rx_dma_dram_offset(bus);
> > -        while (bus->regs[reg_dma_len]) {
> > +        for (i = 0; bus->regs[reg_dma_len]; i++) {
> >              MemTxResult result;
> >
> >              data = i2c_recv(bus->bus); @@ -476,6 +501,10 @@ static
> > void aspeed_i2c_bus_recv(AspeedI2CBus *bus)
> >                  return;
> >              }
> >
> > +            if (i == 0) {
> > +                aspeed_i2c_bus_latch_rx_len(bus, true, data);
> > +            }
> > +
> >              bus->dma_dram_offset++;
> >              bus->regs[reg_dma_len]--;
> >              /* In new mode, keep track of how many bytes we RXed */
> > diff --git a/include/hw/i2c/aspeed_i2c.h b/include/hw/i2c/aspeed_i2c.h
> > index 05937a7a0b..c8e6ea54ad 100644
> > --- a/include/hw/i2c/aspeed_i2c.h
> > +++ b/include/hw/i2c/aspeed_i2c.h
> > @@ -231,6 +231,8 @@ REG32(I2CS_DMA_TX_ADDR_HI, 0x68)
> >      FIELD(I2CS_DMA_TX_ADDR_HI, ADDR_HI, 0, 7)
> > REG32(I2CS_DMA_RX_ADDR_HI, 0x6c)
> >      FIELD(I2CS_DMA_RX_ADDR_HI, ADDR_HI, 0, 7)
> > +REG32(I2CC_BYTE_DATA_LOG, 0x84)
> > +    FIELD(I2CC_BYTE_DATA_LOG, RX_BUF, 0, 8)
> >  REG32(I2CC_VERSION_CTRL, 0x94)
> >      FIELD(I2CC_VERSION_CTRL, FUNC_CFG_DMA_EN, 2, 1)
> >
> > --
> > 2.53.0