Hi Cédric
> Subject: Re: [PATCH v1 4/8] hw/i2c/aspeed_i2c: Fix DMA moving data into
> incorrect address
>
> On 2/2/26 07:19, Jamin Lin wrote:
> > From: Jamin Lin <[email protected]>
> >
> > In the previous design, the I2C model updated dma_dram_offset only
> > when firmware programmed the RX/TX DMA buffer address registers. The
> > firmware used to rewrite these registers before issuing each DMA command.
> >
> > The firmware driver behavior has changed to program the DMA address
> > registers only once during I2C initialization. As a result, the I2C
> > model no longer refreshes dma_dram_offset, causing DMA to move data
> > into an incorrect DRAM address.
> >
> > Fix this by introducing helper functions to update dma_dram_offset
> > from the DMA address registers, and invoke them right before handling
> > TX/RX DMA operations. This guarantees DMA always uses the correct
> > buffer address even if the registers are programmed only once.
> >
> > Signed-off-by: Jamin Lin <[email protected]>
>
> Should we backport to stable branches too ?
>
It would be great if this could also be applied to the stable branches.
I will add a Fixes tag.
Thanks-Jamin
> Thanks,
>
> C.
>
>
> > ---
> > hw/i2c/aspeed_i2c.c | 87
> +++++++++++++++++++++++++++++++--------------
> > 1 file changed, 61 insertions(+), 26 deletions(-)
> >
> > diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c index
> > 1ee73a8f5e..fb3d6a5600 100644
> > --- a/hw/i2c/aspeed_i2c.c
> > +++ b/hw/i2c/aspeed_i2c.c
> > @@ -116,8 +116,6 @@ static uint64_t
> aspeed_i2c_bus_old_read(AspeedI2CBus *bus, hwaddr offset,
> > value = -1;
> > break;
> > }
> > -
> > - value = extract64(bus->dma_dram_offset, 0, 32);
> > break;
> > case A_I2CD_DMA_LEN:
> > if (!aic->has_dma) {
> > @@ -221,6 +219,64 @@ static uint8_t aspeed_i2c_get_state(AspeedI2CBus
> *bus)
> > return SHARED_ARRAY_FIELD_EX32(bus->regs, R_I2CD_CMD,
> TX_STATE);
> > }
> >
> > +/*
> > + * The AST2700 support the maximum DRAM size is 8 GB.
> > + * The DRAM offset range is from 0x0_0000_0000 to
> > + * 0x1_FFFF_FFFF and it is enough to use bits [33:0]
> > + * saving the dram offset.
> > + * Therefore, save the high part physical address bit[1:0]
> > + * of Tx/Rx buffer address as dma_dram_offset bit[33:32].
> > + */
> > +static void aspeed_i2c_set_tx_dma_dram_offset(AspeedI2CBus *bus) {
> > + AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(bus->controller);
> > + uint32_t value;
> > +
> > + assert(aic->has_dma);
> > +
> > + if (aspeed_i2c_is_new_mode(bus->controller)) {
> > + value = bus->regs[R_I2CM_DMA_TX_ADDR];
> > + bus->dma_dram_offset =
> > + deposit64(bus->dma_dram_offset, 0, 32,
> > + FIELD_EX32(value, I2CM_DMA_TX_ADDR,
> ADDR));
> > + if (!aic->has_dma64) {
> > + value = bus->regs[R_I2CM_DMA_TX_ADDR_HI];
> > + bus->dma_dram_offset =
> > + deposit64(bus->dma_dram_offset, 32, 32,
> > + extract32(value, 0, 2));
> > + }
> > + } else {
> > + value = bus->regs[R_I2CD_DMA_ADDR];
> > + bus->dma_dram_offset = deposit64(bus->dma_dram_offset, 0,
> 32,
> > + value & 0x3ffffffc);
> > + }
> > +}
> > +
> > +static void aspeed_i2c_set_rx_dma_dram_offset(AspeedI2CBus *bus) {
> > + AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(bus->controller);
> > + uint32_t value;
> > +
> > + assert(aic->has_dma);
> > +
> > + if (aspeed_i2c_is_new_mode(bus->controller)) {
> > + value = bus->regs[R_I2CM_DMA_RX_ADDR];
> > + bus->dma_dram_offset =
> > + deposit64(bus->dma_dram_offset, 0, 32,
> > + FIELD_EX32(value, I2CM_DMA_RX_ADDR,
> ADDR));
> > + if (!aic->has_dma64) {
> > + value = bus->regs[R_I2CM_DMA_RX_ADDR_HI];
> > + bus->dma_dram_offset =
> > + deposit64(bus->dma_dram_offset, 32, 32,
> > + extract32(value, 0, 2));
> > + }
> > + } else {
> > + value = bus->regs[R_I2CD_DMA_ADDR];
> > + bus->dma_dram_offset = deposit64(bus->dma_dram_offset, 0,
> 32,
> > + value & 0x3ffffffc);
> > + }
> > +}
> > +
> > static int aspeed_i2c_dma_read(AspeedI2CBus *bus, uint8_t *data)
> > {
> > MemTxResult result;
> > @@ -270,6 +326,7 @@ static int aspeed_i2c_bus_send(AspeedI2CBus *bus)
> > if (aspeed_i2c_is_new_mode(bus->controller)) {
> > ARRAY_FIELD_DP32(bus->regs, I2CM_DMA_LEN_STS,
> TX_LEN, 0);
> > }
> > + aspeed_i2c_set_tx_dma_dram_offset(bus);
> > while (bus->regs[reg_dma_len]) {
> > uint8_t data;
> > ret = aspeed_i2c_dma_read(bus, &data); @@ -335,6
> +392,7
> > @@ static void aspeed_i2c_bus_recv(AspeedI2CBus *bus)
> > ARRAY_FIELD_DP32(bus->regs, I2CM_DMA_LEN_STS,
> RX_LEN, 0);
> > }
> >
> > + aspeed_i2c_set_rx_dma_dram_offset(bus);
> > while (bus->regs[reg_dma_len]) {
> > MemTxResult result;
> >
> > @@ -401,6 +459,7 @@ static uint8_t aspeed_i2c_get_addr(AspeedI2CBus
> *bus)
> > } else if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd,
> TX_DMA_EN)) {
> > uint8_t data;
> >
> > + aspeed_i2c_set_tx_dma_dram_offset(bus);
> > aspeed_i2c_dma_read(bus, &data);
> > return data;
> > } else {
> > @@ -657,16 +716,10 @@ static void
> aspeed_i2c_bus_new_write(AspeedI2CBus *bus, hwaddr offset,
> > case A_I2CM_DMA_TX_ADDR:
> > bus->regs[R_I2CM_DMA_TX_ADDR] = FIELD_EX32(value,
> I2CM_DMA_TX_ADDR,
> > ADDR);
> > - bus->dma_dram_offset =
> > - deposit64(bus->dma_dram_offset, 0, 32,
> > - FIELD_EX32(value, I2CM_DMA_TX_ADDR,
> ADDR));
> > break;
> > case A_I2CM_DMA_RX_ADDR:
> > bus->regs[R_I2CM_DMA_RX_ADDR] = FIELD_EX32(value,
> I2CM_DMA_RX_ADDR,
> > ADDR);
> > - bus->dma_dram_offset =
> > - deposit64(bus->dma_dram_offset, 0, 32,
> > - FIELD_EX32(value, I2CM_DMA_RX_ADDR,
> ADDR));
> > break;
> > case A_I2CM_DMA_LEN:
> > w1t = FIELD_EX32(value, I2CM_DMA_LEN, RX_BUF_LEN_W1T)
> || @@
> > -748,15 +801,6 @@ static void aspeed_i2c_bus_new_write(AspeedI2CBus
> *bus, hwaddr offset,
> > qemu_log_mask(LOG_UNIMP, "%s: Slave mode DMA TX is not
> implemented\n",
> > __func__);
> > break;
> > -
> > - /*
> > - * The AST2700 support the maximum DRAM size is 8 GB.
> > - * The DRAM offset range is from 0x0_0000_0000 to
> > - * 0x1_FFFF_FFFF and it is enough to use bits [33:0]
> > - * saving the dram offset.
> > - * Therefore, save the high part physical address bit[1:0]
> > - * of Tx/Rx buffer address as dma_dram_offset bit[33:32].
> > - */
> > case A_I2CM_DMA_TX_ADDR_HI:
> > if (!aic->has_dma64) {
> > qemu_log_mask(LOG_GUEST_ERROR, "%s: No DMA 64 bits
> > support\n", @@ -766,8 +810,6 @@ static void
> aspeed_i2c_bus_new_write(AspeedI2CBus *bus, hwaddr offset,
> > bus->regs[R_I2CM_DMA_TX_ADDR_HI] = FIELD_EX32(value,
> >
> I2CM_DMA_TX_ADDR_HI,
> >
> ADDR_HI);
> > - bus->dma_dram_offset = deposit64(bus->dma_dram_offset, 32,
> 32,
> > - extract32(value, 0, 2));
> > break;
> > case A_I2CM_DMA_RX_ADDR_HI:
> > if (!aic->has_dma64) {
> > @@ -778,8 +820,6 @@ static void
> aspeed_i2c_bus_new_write(AspeedI2CBus *bus, hwaddr offset,
> > bus->regs[R_I2CM_DMA_RX_ADDR_HI] = FIELD_EX32(value,
> >
> I2CM_DMA_RX_ADDR_HI,
> >
> ADDR_HI);
> > - bus->dma_dram_offset = deposit64(bus->dma_dram_offset, 32,
> 32,
> > - extract32(value, 0, 2));
> > break;
> > case A_I2CS_DMA_TX_ADDR_HI:
> > qemu_log_mask(LOG_UNIMP,
> > @@ -795,8 +835,6 @@ static void
> aspeed_i2c_bus_new_write(AspeedI2CBus *bus, hwaddr offset,
> > bus->regs[R_I2CS_DMA_RX_ADDR_HI] = FIELD_EX32(value,
> >
> I2CS_DMA_RX_ADDR_HI,
> >
> ADDR_HI);
> > - bus->dma_dram_offset = deposit64(bus->dma_dram_offset, 32,
> 32,
> > - extract32(value, 0, 2));
> > break;
> > default:
> > qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset 0x%"
> > HWADDR_PRIx "\n", @@ -887,9 +925,6 @@ static void
> aspeed_i2c_bus_old_write(AspeedI2CBus *bus, hwaddr offset,
> > qemu_log_mask(LOG_GUEST_ERROR, "%s: No DMA
> support\n", __func__);
> > break;
> > }
> > -
> > - bus->dma_dram_offset = deposit64(bus->dma_dram_offset, 0, 32,
> > - value & 0x3ffffffc);
> > break;
> >
> > case A_I2CD_DMA_LEN:
