* [PATCH v1 1/1] hw/i2c/aspeed: Fix wrong I2CC_DMA_LEN when I2CM_DMA_TX/RX_ADDR set first
@ 2025-03-27 7:44 Jamin Lin via
2025-03-27 13:16 ` Cédric Le Goater
0 siblings, 1 reply; 3+ messages in thread
From: Jamin Lin via @ 2025-03-27 7:44 UTC (permalink / raw)
To: Cédric Le Goater, Peter Maydell, Steven Lee, Troy Lee,
Andrew Jeffery, Joel Stanley, open list:ASPEED BMCs,
open list:All patches CC here
Cc: jamin_lin, troy_lee, longzl2
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(-)
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:
--
2.43.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v1 1/1] hw/i2c/aspeed: Fix wrong I2CC_DMA_LEN when I2CM_DMA_TX/RX_ADDR set first
2025-03-27 7:44 [PATCH v1 1/1] hw/i2c/aspeed: Fix wrong I2CC_DMA_LEN when I2CM_DMA_TX/RX_ADDR set first Jamin Lin via
@ 2025-03-27 13:16 ` Cédric Le Goater
2025-03-28 0:52 ` Jamin Lin
0 siblings, 1 reply; 3+ messages in thread
From: Cédric Le Goater @ 2025-03-27 13:16 UTC (permalink / raw)
To: Jamin Lin, Peter Maydell, Steven Lee, Troy Lee, Andrew Jeffery,
Joel Stanley, open list:ASPEED BMCs,
open list:All patches CC here
Cc: troy_lee, longzl2
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:
^ permalink raw reply [flat|nested] 3+ messages in thread
* RE: [PATCH v1 1/1] hw/i2c/aspeed: Fix wrong I2CC_DMA_LEN when I2CM_DMA_TX/RX_ADDR set first
2025-03-27 13:16 ` Cédric Le Goater
@ 2025-03-28 0:52 ` Jamin Lin
0 siblings, 0 replies; 3+ messages in thread
From: Jamin Lin @ 2025-03-28 0:52 UTC (permalink / raw)
To: Cédric Le Goater, Peter Maydell, Steven Lee, Troy Lee,
Andrew Jeffery, Joel Stanley, open list:ASPEED BMCs,
open list:All patches CC here
Cc: Troy Lee, longzl2@lenovo.com
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:
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-03-28 0:53 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-03-27 7:44 [PATCH v1 1/1] hw/i2c/aspeed: Fix wrong I2CC_DMA_LEN when I2CM_DMA_TX/RX_ADDR set first Jamin Lin via
2025-03-27 13:16 ` Cédric Le Goater
2025-03-28 0:52 ` Jamin Lin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).