Re: [PATCH v2 4/6] hw/i2c/aspeed: fix buffer transfer in non-packet mode

[Timur Maksimov <gessingate@gmail.com>]

Hi, Jamin.

Understood. I don’t planning to write a driver, but I might verify them
if I have some time. Otherwise, let’s just skip them,  as I am not sure
that the current implementation is incorrect.

On Thu, 2026-10-29 at 07:44 +0000, Jamin Lin wrote:
> Hi Timur, 
> 
> I'm not saying they don't work.
> 
> My main concern is that we currently don't have firmware to test this
> mode on real hardware.
> Although we can test it in QEMU using QTest, this does not guarantee
> that QEMU behaves the same way as real hardware.
> Without testing on real hardware, we cannot confirm that the behavior
> implemented by this patch matches the actual hardware behavior.
> 
> That said, I think the QTest in this series is very useful, as the
> functional tests don't cover all the I2C modes.
> 
> Thanks,
> Jamin
> 
> > Subject: Re: [PATCH v2 4/6] hw/i2c/aspeed: fix buffer transfer in
> > non-packet
> > mode
> > 
> > Hi Jamin, you are right;  no driver uses those modes, and I can't
> > verify their
> > work at the moment.
> > Do you suppose, that they don't work? Are there issues with them?
> > We can exclude them from tests, but  buffer mode is important for
> > old chips
> > like AST2500.
> > 
> > On Thu, 2026-10-29 at 06:33 +0000, Jamin Lin wrote:
> > > Hi Timur,
> > > 
> > > As far as I can see, no ASPEED Linux driver uses non-packet
> > > buffer or
> > > DMA mode for master transfers:
> > > - i2c-aspeed.c (old register mode) only uses byte mode; the
> > > address
> > >   is always written to the byte buffer register before START.
> > > - i2c-ast2600.c in the ASPEED SDK kernel (new register mode) sets
> > >   PKT_EN for every master transfer, and the address comes from
> > >   PKT_ADDR.
> > > 
> > > Have you verified this behavior on real hardware? Which firmware
> > > are
> > > you running that uses non-packet buffer/DMA mode?
> > > 
> > > Thanks,
> > > Jamin
> > > 
> > > > Subject: [PATCH v2 4/6] hw/i2c/aspeed: fix buffer transfer in
> > > > non-
> > > > packet mode
> > > > 
> > > > In non-packet mode, the first byte of the tx buffer pool
> > > > contains a
> > > > device address, so avoid sending it as a data byte.
> > > > 
> > > > Fixes: 961faf3ddbd8 ("hw/i2c/aspeed: Fix TXBUF transmission
> > > > start
> > > > position
> > > > error")
> > > > Signed-off-by: Timur Maksimov <gessingate@gmail.com>
> > > > ---
> > > >  hw/i2c/aspeed_i2c.c | 11 +++++++----
> > > >  1 file changed, 7 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c index
> > > > ff6ef6d23a..510758fb7a 100644
> > > > --- a/hw/i2c/aspeed_i2c.c
> > > > +++ b/hw/i2c/aspeed_i2c.c
> > > > @@ -382,7 +382,7 @@ static void
> > > > aspeed_i2c_bus_recv_dma_pool(AspeedI2CBus *bus)
> > > >      SHARED_ARRAY_FIELD_DP32(bus->regs, reg_cmd, RX_DMA_EN,
> > 0);  }
> > > > 
> > > > -static int aspeed_i2c_bus_send(AspeedI2CBus *bus)
> > > > +static int aspeed_i2c_bus_send(AspeedI2CBus *bus, bool
> > > > send_addr)
> > > >  {
> > > >      AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(bus-
> > > > >controller);
> > > >      int ret = -1;
> > > > @@ -391,11 +391,12 @@ static int
> > > > aspeed_i2c_bus_send(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);
> > > > +    uint8_t pool_offset = send_addr ? 1 : 0;
> > > >      int pool_tx_count = SHARED_ARRAY_FIELD_EX32(bus->regs,
> > > > reg_pool_ctrl,
> > > 
> > >                                                  T
> > X_COUNT) + 1;
> > > > 
> > > >      if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd,
> > > > TX_BUFF_EN))
> > {
> > > > -        for (i = 0; i < pool_tx_count; i++) {
> > > > +        for (i = pool_offset; i < pool_tx_count; i++) {
> > > >              uint8_t *pool_base = aic->bus_pool_base(bus);
> > > > 
> > > >              trace_aspeed_i2c_bus_send("BUF", i + 1,
> > pool_tx_count,
> > > > @@
> > > > -633,6 +634,7 @@ static void
> > > > aspeed_i2c_bus_handle_cmd(AspeedI2CBus
> > > > *bus, uint64_t value)
> > > >      uint32_t reg_intr_sts =
> > > > aspeed_i2c_bus_intr_sts_offset(bus);
> > > >      uint32_t reg_cmd = aspeed_i2c_bus_cmd_offset(bus);
> > > >      uint32_t reg_dma_len = aspeed_i2c_bus_dma_len_offset(bus);
> > > > +    bool start_cmd = SHARED_ARRAY_FIELD_EX32(bus->regs,
> > > > reg_cmd,
> > > > +M_START_CMD);
> > > > 
> > > >      if (!aspeed_i2c_check_sram(bus)) {
> > > >          return;
> > > > @@ -642,7 +644,7 @@ static void
> > > > aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
> > > >          aspeed_i2c_bus_cmd_dump(bus);
> > > >      }
> > > > 
> > > > -    if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd,
> > M_START_CMD)) {
> > > > +    if (start_cmd) {
> > > >          uint8_t state = aspeed_i2c_get_state(bus) &
> > I2CD_MACTIVE ?
> > > >              I2CD_MSTARTR : I2CD_MSTART;
> > > >          uint8_t addr;
> > > > @@ -686,7 +688,8 @@ static void
> > > > aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
> > > > 
> > > >      if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, M_TX_CMD))
> > {
> > > >          aspeed_i2c_set_state(bus, I2CD_MTXD);
> > > > -        if (aspeed_i2c_bus_send(bus)) {
> > > > +        if (aspeed_i2c_bus_send(bus, start_cmd &&
> > > > 
> > +                                     !aspeed_i2c_bus_pkt
> > _mode_en
> > > > (bus)))
> > > > + {
> > > >              SHARED_ARRAY_FIELD_DP32(bus->regs, reg_intr_sts,
> > > > TX_NAK, 1);
> > > >              i2c_end_transfer(bus->bus);
> > > >          } else {
> > > > 
> > > > --
> > > > 2.53.0