| Message ID | 20250327074419.563306-1-jamin_lin@aspeedtech.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v1,1/1] hw/i2c/aspeed: Fix wrong I2CC_DMA_LEN when I2CM_DMA_TX/RX_ADDR set first | expand |
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:
Hi Cedric, > Cc: Troy Lee <troy_lee@aspeedtech.com>; longzl2@lenovo.com > Subject: Re: [PATCH v1 1/1] hw/i2c/aspeed: Fix wrong I2CC_DMA_LEN when > I2CM_DMA_TX/RX_ADDR set first > > 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 for review. Will check functional test issue Jamin > > 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:
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:
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 --- hw/i2c/aspeed_i2c.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)