Re: [PATCH v1 1/1] hw/i2c/aspeed: Fix wrong I2CC_DMA_LEN when I2CM_DMA_TX/RX_ADDR set first

["Cédric Le Goater" <clg@kaod.org>]

Hello Jamin

On 3/27/25 08:44, Jamin Lin wrote:
> In the previous design, the I2C model would update I2CC_DMA_LEN (0x54) based on
> the value of I2CM_DMA_LEN (0x1C) when the firmware set either I2CM_DMA_TX_ADDR
> (0x30) or I2CM_DMA_RX_ADDR (0x34). However, this only worked correctly if the
> firmware set I2CM_DMA_LEN before setting I2CM_DMA_TX_ADDR or I2CM_DMA_RX_ADDR.
> 
> If the firmware instead set I2CM_DMA_TX_ADDR or I2CM_DMA_RX_ADDR before setting
> I2CM_DMA_LEN, the value written to I2CC_DMA_LEN would be incorrect.
> 
> To fix this issue, the model should be updated to set I2CC_DMA_LEN when the
> firmware writes to the I2CM_DMA_LEN register, rather than when it writes to the
> I2CM_DMA_RX_ADDR and I2CM_DMA_TX_ADDR registers.
> 
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> Fixes: ba2cccd (aspeed: i2c: Add new mode support)
> Change-Id: Ibb4039773a88ba4f2ebda7f1ab5b5f9e99d22456

It looks like this is breaking the functional test. Could you check please ?


Thanks,

C.



> ---
>   hw/i2c/aspeed_i2c.c | 10 ++++++----
>   1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
> index a8fbb9f44a..c68b8a5ec0 100644
> --- a/hw/i2c/aspeed_i2c.c
> +++ b/hw/i2c/aspeed_i2c.c
> @@ -656,8 +656,6 @@ static void aspeed_i2c_bus_new_write(AspeedI2CBus *bus, hwaddr offset,
>           bus->dma_dram_offset =
>               deposit64(bus->dma_dram_offset, 0, 32,
>                         FIELD_EX32(value, I2CM_DMA_TX_ADDR, ADDR));
> -        bus->regs[R_I2CC_DMA_LEN] = ARRAY_FIELD_EX32(bus->regs, I2CM_DMA_LEN,
> -                                                     TX_BUF_LEN) + 1;
>           break;
>       case A_I2CM_DMA_RX_ADDR:
>           bus->regs[R_I2CM_DMA_RX_ADDR] = FIELD_EX32(value, I2CM_DMA_RX_ADDR,
> @@ -665,8 +663,6 @@ static void aspeed_i2c_bus_new_write(AspeedI2CBus *bus, hwaddr offset,
>           bus->dma_dram_offset =
>               deposit64(bus->dma_dram_offset, 0, 32,
>                         FIELD_EX32(value, I2CM_DMA_RX_ADDR, ADDR));
> -        bus->regs[R_I2CC_DMA_LEN] = ARRAY_FIELD_EX32(bus->regs, I2CM_DMA_LEN,
> -                                                     RX_BUF_LEN) + 1;
>           break;
>       case A_I2CM_DMA_LEN:
>           w1t = FIELD_EX32(value, I2CM_DMA_LEN, RX_BUF_LEN_W1T) ||
> @@ -679,10 +675,16 @@ static void aspeed_i2c_bus_new_write(AspeedI2CBus *bus, hwaddr offset,
>           if (FIELD_EX32(value, I2CM_DMA_LEN, RX_BUF_LEN_W1T)) {
>               ARRAY_FIELD_DP32(bus->regs, I2CM_DMA_LEN, RX_BUF_LEN,
>                                FIELD_EX32(value, I2CM_DMA_LEN, RX_BUF_LEN));
> +            bus->regs[R_I2CC_DMA_LEN] = ARRAY_FIELD_EX32(bus->regs,
> +                                                         I2CM_DMA_LEN,
> +                                                         RX_BUF_LEN) + 1;
>           }
>           if (FIELD_EX32(value, I2CM_DMA_LEN, TX_BUF_LEN_W1T)) {
>               ARRAY_FIELD_DP32(bus->regs, I2CM_DMA_LEN, TX_BUF_LEN,
>                                FIELD_EX32(value, I2CM_DMA_LEN, TX_BUF_LEN));
> +            bus->regs[R_I2CC_DMA_LEN] = ARRAY_FIELD_EX32(bus->regs,
> +                                                         I2CM_DMA_LEN,
> +                                                         TX_BUF_LEN) + 1;
>           }
>           break;
>       case A_I2CM_DMA_LEN_STS: