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

Hi Jamin,

Tested-by: Mikail Sadic <[email protected]>

I checked it over and verified it all works, I'm glad my suggestions were helpful.

Thank you,
Mikail Sadic

-----Original Message-----
From: Jamin Lin <[email protected]> 
Sent: Thursday, August 13, 2026 9:20 PM
To: Mikail Sadic <[email protected]>; 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]>
Subject: [EXTERNAL] 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://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.kernel.org_project_qemu-2Ddevel_patch_20260814020836.3119613-2D1-2Djamin-5Flin-40aspeedtech.com_&d=DwIFAw&c=BSDicqBQBDjDI9RkVyTcHQ&r=MwxPIV78QTTKBsiQ-TUBApx4-_ZEUlleOhDjygCqcOU&m=35Of371QTEER-fuhBTEDrGMw9Sq8K3JtKRL1wWPYs-2MBuvWhn-KM-eI6qd9O20W&s=1-a288_BxQVvouXeqT7tnvlD1j9C0HLzGbuFEIl2DxI&e= 

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