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 */
