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