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
