* [PATCH v1] hw/i2c/aspeed_i2c: Latch first received byte for SMBus block reads @ 2026-08-13 9:02 Jamin Lin 2026-08-13 9:08 ` Jamin Lin 0 siblings, 1 reply; 4+ messages in thread From: Jamin Lin @ 2026-08-13 9:02 UTC (permalink / raw) To: Cédric Le Goater, Peter Maydell, Steven Lee, Troy Lee, Kane Chen, Andrew Jeffery, Joel Stanley, open list:ASPEED BMCs, open list:All patches CC here Cc: Jamin Lin, Troy Lee, Mikail.Sadic@ibm.com 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 <jamin_lin@aspeedtech.com> --- 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* RE: [PATCH v1] hw/i2c/aspeed_i2c: Latch first received byte for SMBus block reads 2026-08-13 9:02 [PATCH v1] hw/i2c/aspeed_i2c: Latch first received byte for SMBus block reads Jamin Lin @ 2026-08-13 9:08 ` Jamin Lin 2026-08-13 15:54 ` Mikail Sadic 0 siblings, 1 reply; 4+ messages in thread From: Jamin Lin @ 2026-08-13 9:08 UTC (permalink / raw) To: Cédric Le Goater, Peter Maydell, Steven Lee, Troy Lee, Kane Chen, Andrew Jeffery, Joel Stanley, open list:ASPEED BMCs, open list:All patches CC here Cc: Troy Lee, Mikail.Sadic@ibm.com Hi Mikail, Could you please review this patch and verify that it works with the UCD9000 driver? Thanks, Jamin > -----Original Message----- > From: Jamin Lin <jamin_lin@aspeedtech.com> > Sent: Thursday, August 13, 2026 5:02 PM > To: Cédric Le Goater <clg@kaod.org>; Peter Maydell > <peter.maydell@linaro.org>; Steven Lee <steven_lee@aspeedtech.com>; Troy > Lee <leetroy@gmail.com>; Kane Chen <kane_chen@aspeedtech.com>; > Andrew Jeffery <andrew@codeconstruct.com.au>; Joel Stanley > <joel@jms.id.au>; open list:ASPEED BMCs <qemu-arm@nongnu.org>; open > list:All patches CC here <qemu-devel@nongnu.org> > Cc: Jamin Lin <jamin_lin@aspeedtech.com>; Troy Lee > <troy_lee@aspeedtech.com>; Mikail.Sadic@ibm.com > 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 <jamin_lin@aspeedtech.com> > --- > 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* RE: [PATCH v1] hw/i2c/aspeed_i2c: Latch first received byte for SMBus block reads 2026-08-13 9:08 ` Jamin Lin @ 2026-08-13 15:54 ` Mikail Sadic 2026-08-14 2:19 ` Jamin Lin 0 siblings, 1 reply; 4+ messages in thread From: Mikail Sadic @ 2026-08-13 15:54 UTC (permalink / raw) To: Jamin Lin, Cédric Le Goater, Peter Maydell, Steven Lee, Troy Lee, Kane Chen, Andrew Jeffery, Joel Stanley, open list:ASPEED BMCs, open list:All patches CC here Cc: Troy Lee Hi Jamin, Tested-by: Mikail Sadic <mikail.sadic@ibm.com> 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. Thanks again for picking this up and catching the original problem. - Mikail Sadic -----Original Message----- From: Jamin Lin <jamin_lin@aspeedtech.com> Sent: Thursday, August 13, 2026 4:08 AM To: Cédric Le Goater <clg@kaod.org>; Peter Maydell <peter.maydell@linaro.org>; Steven Lee <steven_lee@aspeedtech.com>; Troy Lee <leetroy@gmail.com>; Kane Chen <kane_chen@aspeedtech.com>; Andrew Jeffery <andrew@codeconstruct.com.au>; Joel Stanley <joel@jms.id.au>; open list:ASPEED BMCs <qemu-arm@nongnu.org>; open list:All patches CC here <qemu-devel@nongnu.org> Cc: Troy Lee <troy_lee@aspeedtech.com>; Mikail Sadic <Mikail.Sadic@ibm.com> 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 <jamin_lin@aspeedtech.com> > Sent: Thursday, August 13, 2026 5:02 PM > To: Cédric Le Goater <clg@kaod.org>; Peter Maydell > <peter.maydell@linaro.org>; Steven Lee <steven_lee@aspeedtech.com>; > Troy Lee <leetroy@gmail.com>; Kane Chen <kane_chen@aspeedtech.com>; > Andrew Jeffery <andrew@codeconstruct.com.au>; Joel Stanley > <joel@jms.id.au>; open list:ASPEED BMCs <qemu-arm@nongnu.org>; open > list:All patches CC here <qemu-devel@nongnu.org> > Cc: Jamin Lin <jamin_lin@aspeedtech.com>; Troy Lee > <troy_lee@aspeedtech.com>; Mikail.Sadic@ibm.com > 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 <jamin_lin@aspeedtech.com> > --- > 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* RE: [PATCH v1] hw/i2c/aspeed_i2c: Latch first received byte for SMBus block reads 2026-08-13 15:54 ` Mikail Sadic @ 2026-08-14 2:19 ` Jamin Lin 0 siblings, 0 replies; 4+ messages in thread From: Jamin Lin @ 2026-08-14 2:19 UTC (permalink / raw) To: Mikail Sadic, Cédric Le Goater, Peter Maydell, Steven Lee, Troy Lee, Kane Chen, Andrew Jeffery, Joel Stanley, open list:ASPEED BMCs, open list:All patches CC here Cc: Troy Lee > Subject: RE: [PATCH v1] hw/i2c/aspeed_i2c: Latch first received byte for SMBus > block reads > > Hi Jamin, > > Tested-by: Mikail Sadic <mikail.sadic@ibm.com> > > 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://patchwork.kernel.org/project/qemu-devel/patch/20260814020836.3119613-1-jamin_lin@aspeedtech.com/ Thanks, Jamin > > Thanks again for picking this up and catching the original problem. > > - Mikail Sadic > > > -----Original Message----- > From: Jamin Lin <jamin_lin@aspeedtech.com> > Sent: Thursday, August 13, 2026 4:08 AM > To: Cédric Le Goater <clg@kaod.org>; Peter Maydell > <peter.maydell@linaro.org>; Steven Lee <steven_lee@aspeedtech.com>; Troy > Lee <leetroy@gmail.com>; Kane Chen <kane_chen@aspeedtech.com>; > Andrew Jeffery <andrew@codeconstruct.com.au>; Joel Stanley > <joel@jms.id.au>; open list:ASPEED BMCs <qemu-arm@nongnu.org>; open > list:All patches CC here <qemu-devel@nongnu.org> > Cc: Troy Lee <troy_lee@aspeedtech.com>; Mikail Sadic > <Mikail.Sadic@ibm.com> > 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 <jamin_lin@aspeedtech.com> > > Sent: Thursday, August 13, 2026 5:02 PM > > To: Cédric Le Goater <clg@kaod.org>; Peter Maydell > > <peter.maydell@linaro.org>; Steven Lee <steven_lee@aspeedtech.com>; > > Troy Lee <leetroy@gmail.com>; Kane Chen <kane_chen@aspeedtech.com>; > > Andrew Jeffery <andrew@codeconstruct.com.au>; Joel Stanley > > <joel@jms.id.au>; open list:ASPEED BMCs <qemu-arm@nongnu.org>; open > > list:All patches CC here <qemu-devel@nongnu.org> > > Cc: Jamin Lin <jamin_lin@aspeedtech.com>; Troy Lee > > <troy_lee@aspeedtech.com>; Mikail.Sadic@ibm.com > > 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 <jamin_lin@aspeedtech.com> > > --- > > 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2026-08-14 2:20 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-08-13 9:02 [PATCH v1] hw/i2c/aspeed_i2c: Latch first received byte for SMBus block reads Jamin Lin 2026-08-13 9:08 ` Jamin Lin 2026-08-13 15:54 ` Mikail Sadic 2026-08-14 2:19 ` Jamin Lin
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.