RE: [PATCH v3 4/8] i2c/aspeed: Fix DMA receive first-byte handling for block reads

Hi Mikail,

> -----Original Message-----
> From: Cédric Le Goater <[email protected]>
> Sent: Tuesday, August 11, 2026 12:31 PM
> To: Mikail Sadic <[email protected]>; [email protected]
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; Steven Lee
> <[email protected]>; [email protected]; Jamin Lin
> <[email protected]>; Kane Chen <[email protected]>;
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]
> Subject: Re: [PATCH v3 4/8] i2c/aspeed: Fix DMA receive first-byte handling for
> block reads
> 
> Jamin,
> 
> I would appreciate a feedback from you on this change.
> 
Hi Mikail,

Before going further -- are you sure this value is correct in DMA mode?
AST2600/AST2700 datasheet, I2CC08 (Transmit/Receive Byte Buffer):
    15:8  RO  Receive Byte Buffer
              "This register is valid when DMA Buffer is not enabled."

That reads to me as: the receive byte buffer only holds a defined value
in pool buffer mode, not when DMA is enabled. If that is right, then
modelling it in the DMA path makes QEMU report a block length the
silicon would not provide, and a driver relying on it would pass under
emulation but fail on a real board.

Have you confirmed this works on real hardware, or only in QEMU?
If the datasheet reading is correct, I think the mirror belongs in the
pool buffer path instead -- ast2600_i2c_setup_buff_rx() does not enable
the DMA buffer, yet aspeed_i2c_bus_recv()'s RX_BUFF_EN branch never
updates reg_byte_buf either. That is also the default mode:
i2c-ast2600.c sets mode = BUFF_MODE and no AST2600 board DT overrides
aspeed,transfer-mode.

1. On AST2600, is your setup using DMA mode or buffer mode?  ---> AS far As I know, customers should use the buffer mode because I2C DMA mode issue in AST2600.

2. Are you testing AST2700 as well? There both modes set RX_DMA_EN (DMA and Buffer mode) --
   ast2700_i2c_setup_buff_rx() does too -- so I2CC08[15:8] would never
   be valid, and the driver reads the length from "BYTE_DATA_LOG (0x84)",
   which QEMU does not model at all. That would need a separate patch.
   https://github.com/AspeedTech-BMC/linux/commit/f9938ec5799e83f5b09d3a7acac7287c0cde2c26
   https://patchwork.kernel.org/project/qemu-devel/patch/[email protected]/ 

   
Thanks,
Jamin


> On 8/10/26 20:57, Mikail Sadic wrote:
> > An SMBus block read (I2C_M_RECV_LEN) reads the block length from the
> > first received byte. The Linux/U-Boot aspeed I2C driver obtains that
> > first byte from the I2CC_STS_AND_BUFF register (modelled here as
> > reg_byte_buf), even when the transfer uses DMA. The DMA receive path,
> > however, only wrote received data to DRAM and never updated
> > reg_byte_buf, so block reads read a stale/zero length.
> >
> > Mirror the first DMA-received byte into reg_byte_buf so that
> > I2C_M_RECV_LEN transfers using DMA report the correct block length.
> > This is required for the ucd9000 driver, which uses
> > i2c_smbus_read_block_data().
> >
> > Signed-off-by: Mikail Sadic <[email protected]>
> 
> Does this change deserve a Fixes: tag ?
> 
> Thanks,
> 
> C.
> 
> > ---
> >   hw/i2c/aspeed_i2c.c | 7 +++++++
> >   1 file changed, 7 insertions(+)
> >
> > diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c index
> > 27afcaecee..facb54d27e 100644
> > --- a/hw/i2c/aspeed_i2c.c
> > +++ b/hw/i2c/aspeed_i2c.c
> > @@ -365,6 +365,7 @@ static void aspeed_i2c_bus_recv(AspeedI2CBus *bus)
> >       uint32_t reg_pool_ctrl = aspeed_i2c_bus_pool_ctrl_offset(bus);
> >       uint32_t reg_byte_buf = aspeed_i2c_bus_byte_buf_offset(bus);
> >       uint32_t reg_dma_len = aspeed_i2c_bus_dma_len_offset(bus);
> > +    bool first_dma_byte;
> >       int pool_rx_count = SHARED_ARRAY_FIELD_EX32(bus->regs,
> reg_pool_ctrl,
> >                                                   RX_SIZE) + 1;
> >
> > @@ -391,6 +392,7 @@ static void aspeed_i2c_bus_recv(AspeedI2CBus *bus)
> >           }
> >
> >           aspeed_i2c_set_rx_dma_dram_offset(bus);
> > +        first_dma_byte = true;
> >           while (bus->regs[reg_dma_len]) {
> >               MemTxResult result;
> >
> > @@ -407,6 +409,11 @@ static void aspeed_i2c_bus_recv(AspeedI2CBus
> *bus)
> >                   return;
> >               }
> >
> > +            /* Mirror first byte to reg_byte_buf for I2C_M_RECV_LEN. */
> > +            if (first_dma_byte) {
> > +                SHARED_ARRAY_FIELD_DP32(bus->regs, reg_byte_buf,
> RX_BUF, data);
> > +                first_dma_byte = false;
> > +            }
> >               bus->dma_dram_offset++;
> >               bus->regs[reg_dma_len]--;
> >               /* In new mode, keep track of how many bytes we RXed */