| Message ID | 20250408082504.3060742-1-jamin_lin@aspeedtech.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] hw/i2c/aspeed: Fix wrong I2CC_DMA_LEN when I2CM_DMA_TX/RX_ADDR set first | expand |
Hi Cedric, After discussing with the I2C hardware designers, we confirmed that the I2c design in AST2600 and AST2700 A1 is the same. The datasheet will be updated accordingly for AST2700. However, please note that bit 15 and bit 31 are not available on AST2700 A0 and FW do not set either bit 15 and bit 31. AST2700 A0 was an engineering sample version. Given this, I plan to resend the v3 patch with AST2700 A0 explicitly marked as unsupported. I prefer not to introduce workarounds specifically for AST2700 A0, as it is not a production-grade silicon. I will resend the v3 patch with the same content as v1, since the only issue in v1 was a functional test failure on AST2700 A0. Apologies for the inconvenience, and thank you for your understanding. Thanks-Jamin ************* Email Confidentiality Notice ******************** 免責聲明: 本信件(或其附件)可能包含機密資訊，並受法律保護。如 台端非指定之收件者，請以電子郵件通知本電子郵件之發送者, 並請立即刪除本電子郵件及其附件和銷毀所有複印件。謝謝您的合作! DISCLAIMER: This message (and any attachments) may contain legally privileged and/or other confidential information. If you have received it in error, please notify the sender by reply e-mail and immediately delete the e-mail and any attachments without copying or disclosing the contents. Thank you. > -----Original Message----- > From: Jamin Lin <jamin_lin@aspeedtech.com> > Sent: Tuesday, April 8, 2025 4:25 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>; 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> > Subject: [PATCH v2] hw/i2c/aspeed: Fix wrong I2CC_DMA_LEN when > I2CM_DMA_TX/RX_ADDR set first > > 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. > > Ideally, this issue should be resolved by updating the model to set > I2CC_DMA_LEN (0x54) when the firmware writes to the I2CM_DMA_LEN > (0x1C) register, instead of when it writes to I2CM_DMA_TX_ADDR (0x30) or > I2CM_DMA_RX_ADDR (0x34). > > Originally, the design of I2CM_DMA_LEN (0x1C) included buffer length > write-enable bits for the current command: > Bit 31 enabled the RX buffer length update Bit 15 enabled the TX buffer length > update > > In other words, when the firmware set either bit 31 or bit 15, the I2C model > could safely update I2CC_DMA_LEN (0x54) with the value in I2CM_DMA_LEN > (0x1C). > > However, starting with the AST2700, the design of the I2CM_DMA_LEN (0x1C) > register was changed. The write-enable bits (bit 31 and bit 15) were removed, > meaning there is no longer an explicit indication of whether the firmware > intends to update the TX or RX length. > > As a result, on AST2700 and newer SoCs, the model cannot reliably determine > whether a write to I2CM_DMA_LEN was meant for TX or RX. This ambiguity is > especially problematic when the value written is 0, which actually corresponds > to a DMA length of 1. > > To ensure consistent behavior across all SoCs, the model now updates > I2CC_DMA_LEN when I2CM_CMD (0x18) is written, as this is the final > command that initiates a TX or RX transfer and reflects the firmware’s intent > more clearly. > > Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com> > Fixes: ba2cccd (aspeed: i2c: Add new mode support) > --- > hw/i2c/aspeed_i2c.c | 18 ++++++++++++++---- > 1 file changed, 14 insertions(+), 4 deletions(-) > > diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c index > a8fbb9f44a..c659099e9a 100644 > --- a/hw/i2c/aspeed_i2c.c > +++ b/hw/i2c/aspeed_i2c.c > @@ -634,6 +634,20 @@ static void aspeed_i2c_bus_new_write(AspeedI2CBus > *bus, hwaddr offset, > break; > } > > + /* Handle DMA length */ > + if (SHARED_FIELD_EX32(value, TX_DMA_EN) && > + SHARED_FIELD_EX32(value, M_TX_CMD)) { > + bus->regs[R_I2CC_DMA_LEN] = ARRAY_FIELD_EX32(bus->regs, > + > I2CM_DMA_LEN, > + > TX_BUF_LEN) + 1; > + } > + if (SHARED_FIELD_EX32(value, RX_DMA_EN) && > + SHARED_FIELD_EX32(value, M_RX_CMD)) { > + bus->regs[R_I2CC_DMA_LEN] = ARRAY_FIELD_EX32(bus->regs, > + > I2CM_DMA_LEN, > + > RX_BUF_LEN) + 1; > + } > + > if (bus->regs[R_I2CM_INTR_STS] & 0xffff0000) { > qemu_log_mask(LOG_UNIMP, "%s: Packet mode is not > implemented\n", > __func__); > @@ -656,8 +670,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 +677,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) || > -- > 2.43.0
Hello, On 4/9/25 11:10, Jamin Lin wrote: > Hi Cedric, > > After discussing with the I2C hardware designers, we confirmed that the I2c design in AST2600 and AST2700 A1 is the same. > The datasheet will be updated accordingly for AST2700. > > However, please note that bit 15 and bit 31 are not available on AST2700 A0 and FW do not set either bit 15 and bit 31. > AST2700 A0 was an engineering sample version. Given this, I plan to resend the v3 patch with AST2700 A0 explicitly marked as unsupported. Are you going to introduce an I2C property to distinguish the A0 implementation for the A1 ? > I prefer not to introduce workarounds specifically for AST2700 A0, as it is not a production-grade silicon. We can deprecate the AST2700 A0 machine too, See 56a37eda93ed, and change the ast2700-evb machine alias. > I will resend the v3 patch with the same content as v1, since the only issue in v1 was a functional test failure on AST2700 A0. > Apologies for the inconvenience, and thank you for your understanding. That's ok. Nothing is merged. Let's get it right first. Thanks, C. > > Thanks-Jamin > > ************* Email Confidentiality Notice ******************** > 免責聲明: > 本信件(或其附件)可能包含機密資訊，並受法律保護。如 台端非指定之收件者，請以電子郵件通知本電子郵件之發送者, 並請立即刪除本電子郵件及其附件和銷毀所有複印件。謝謝您的合作! > > DISCLAIMER: > This message (and any attachments) may contain legally privileged and/or other confidential information. If you have received it in error, please notify the sender by reply e-mail and immediately delete the e-mail and any attachments without copying or disclosing the contents. Thank you. > >> -----Original Message----- >> From: Jamin Lin <jamin_lin@aspeedtech.com> >> Sent: Tuesday, April 8, 2025 4:25 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>; 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> >> Subject: [PATCH v2] hw/i2c/aspeed: Fix wrong I2CC_DMA_LEN when >> I2CM_DMA_TX/RX_ADDR set first >> >> 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. >> >> Ideally, this issue should be resolved by updating the model to set >> I2CC_DMA_LEN (0x54) when the firmware writes to the I2CM_DMA_LEN >> (0x1C) register, instead of when it writes to I2CM_DMA_TX_ADDR (0x30) or >> I2CM_DMA_RX_ADDR (0x34). >> >> Originally, the design of I2CM_DMA_LEN (0x1C) included buffer length >> write-enable bits for the current command: >> Bit 31 enabled the RX buffer length update Bit 15 enabled the TX buffer length >> update >> >> In other words, when the firmware set either bit 31 or bit 15, the I2C model >> could safely update I2CC_DMA_LEN (0x54) with the value in I2CM_DMA_LEN >> (0x1C). >> >> However, starting with the AST2700, the design of the I2CM_DMA_LEN (0x1C) >> register was changed. The write-enable bits (bit 31 and bit 15) were removed, >> meaning there is no longer an explicit indication of whether the firmware >> intends to update the TX or RX length. >> >> As a result, on AST2700 and newer SoCs, the model cannot reliably determine >> whether a write to I2CM_DMA_LEN was meant for TX or RX. This ambiguity is >> especially problematic when the value written is 0, which actually corresponds >> to a DMA length of 1. >> >> To ensure consistent behavior across all SoCs, the model now updates >> I2CC_DMA_LEN when I2CM_CMD (0x18) is written, as this is the final >> command that initiates a TX or RX transfer and reflects the firmware’s intent >> more clearly. >> >> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com> >> Fixes: ba2cccd (aspeed: i2c: Add new mode support) >> --- >> hw/i2c/aspeed_i2c.c | 18 ++++++++++++++---- >> 1 file changed, 14 insertions(+), 4 deletions(-) >> >> diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c index >> a8fbb9f44a..c659099e9a 100644 >> --- a/hw/i2c/aspeed_i2c.c >> +++ b/hw/i2c/aspeed_i2c.c >> @@ -634,6 +634,20 @@ static void aspeed_i2c_bus_new_write(AspeedI2CBus >> *bus, hwaddr offset, >> break; >> } >> >> + /* Handle DMA length */ >> + if (SHARED_FIELD_EX32(value, TX_DMA_EN) && >> + SHARED_FIELD_EX32(value, M_TX_CMD)) { >> + bus->regs[R_I2CC_DMA_LEN] = ARRAY_FIELD_EX32(bus->regs, >> + >> I2CM_DMA_LEN, >> + >> TX_BUF_LEN) + 1; >> + } >> + if (SHARED_FIELD_EX32(value, RX_DMA_EN) && >> + SHARED_FIELD_EX32(value, M_RX_CMD)) { >> + bus->regs[R_I2CC_DMA_LEN] = ARRAY_FIELD_EX32(bus->regs, >> + >> I2CM_DMA_LEN, >> + >> RX_BUF_LEN) + 1; >> + } >> + >> if (bus->regs[R_I2CM_INTR_STS] & 0xffff0000) { >> qemu_log_mask(LOG_UNIMP, "%s: Packet mode is not >> implemented\n", >> __func__); >> @@ -656,8 +670,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 +677,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) || >> -- >> 2.43.0 >
Hi Cedric, > Subject: Re: [PATCH v2] hw/i2c/aspeed: Fix wrong I2CC_DMA_LEN when > I2CM_DMA_TX/RX_ADDR set first > > Hello, > > On 4/9/25 11:10, Jamin Lin wrote: > > Hi Cedric, > > > > After discussing with the I2C hardware designers, we confirmed that the I2c > design in AST2600 and AST2700 A1 is the same. > > The datasheet will be updated accordingly for AST2700. > > > > However, please note that bit 15 and bit 31 are not available on AST2700 A0 > and FW do not set either bit 15 and bit 31. > > AST2700 A0 was an engineering sample version. Given this, I plan to resend > the v3 patch with AST2700 A0 explicitly marked as unsupported. > > Are you going to introduce an I2C property to distinguish the > A0 implementation for the A1 ? > No. I want the I2C model to be as simple as possible, without adding any ""AST2700 A0"" specific changes. V1 patch work for AST2700 A1, AST2600, AST2500, AST1030, AST2500, AST2400 but not AST2700 A0. > > I prefer not to introduce workarounds specifically for AST2700 A0, as it is not > a production-grade silicon. > > We can deprecate the AST2700 A0 machine too, See 56a37eda93ed, and > change the ast2700-evb machine alias. Will do. > > > I will resend the v3 patch with the same content as v1, since the only issue in > v1 was a functional test failure on AST2700 A0. > > Apologies for the inconvenience, and thank you for your understanding. > > That's ok. Nothing is merged. Let's get it right first. > Thanks-Jamin > Thanks, > > C. > > > > > > > Thanks-Jamin > > > > ************* Email Confidentiality Notice ******************** > > 免責聲明: > > 本信件(或其附件)可能包含機密資訊，並受法律保護。如 台端非指定之 > 收件者，請以電子郵件通知本電子郵件之發送者, 並請立即刪除本電子郵 > 件及其附件和銷毀所有複印件。謝謝您的合作! > > > > DISCLAIMER: > > This message (and any attachments) may contain legally privileged and/or > other confidential information. If you have received it in error, please notify > the sender by reply e-mail and immediately delete the e-mail and any > attachments without copying or disclosing the contents. Thank you. > > > >> -----Original Message----- > >> From: Jamin Lin <jamin_lin@aspeedtech.com> > >> Sent: Tuesday, April 8, 2025 4:25 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>; 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> > >> Subject: [PATCH v2] hw/i2c/aspeed: Fix wrong I2CC_DMA_LEN when > >> I2CM_DMA_TX/RX_ADDR set first > >> > >> 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. > >> > >> Ideally, this issue should be resolved by updating the model to set > >> I2CC_DMA_LEN (0x54) when the firmware writes to the I2CM_DMA_LEN > >> (0x1C) register, instead of when it writes to I2CM_DMA_TX_ADDR (0x30) > >> or I2CM_DMA_RX_ADDR (0x34). > >> > >> Originally, the design of I2CM_DMA_LEN (0x1C) included buffer length > >> write-enable bits for the current command: > >> Bit 31 enabled the RX buffer length update Bit 15 enabled the TX > >> buffer length update > >> > >> In other words, when the firmware set either bit 31 or bit 15, the > >> I2C model could safely update I2CC_DMA_LEN (0x54) with the value in > >> I2CM_DMA_LEN (0x1C). > >> > >> However, starting with the AST2700, the design of the I2CM_DMA_LEN > >> (0x1C) register was changed. The write-enable bits (bit 31 and bit > >> 15) were removed, meaning there is no longer an explicit indication > >> of whether the firmware intends to update the TX or RX length. > >> > >> As a result, on AST2700 and newer SoCs, the model cannot reliably > >> determine whether a write to I2CM_DMA_LEN was meant for TX or RX. > >> This ambiguity is especially problematic when the value written is 0, > >> which actually corresponds to a DMA length of 1. > >> > >> To ensure consistent behavior across all SoCs, the model now updates > >> I2CC_DMA_LEN when I2CM_CMD (0x18) is written, as this is the final > >> command that initiates a TX or RX transfer and reflects the > >> firmware’s intent more clearly. > >> > >> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com> > >> Fixes: ba2cccd (aspeed: i2c: Add new mode support) > >> --- > >> hw/i2c/aspeed_i2c.c | 18 ++++++++++++++---- > >> 1 file changed, 14 insertions(+), 4 deletions(-) > >> > >> diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c index > >> a8fbb9f44a..c659099e9a 100644 > >> --- a/hw/i2c/aspeed_i2c.c > >> +++ b/hw/i2c/aspeed_i2c.c > >> @@ -634,6 +634,20 @@ static void > >> aspeed_i2c_bus_new_write(AspeedI2CBus > >> *bus, hwaddr offset, > >> break; > >> } > >> > >> + /* Handle DMA length */ > >> + if (SHARED_FIELD_EX32(value, TX_DMA_EN) && > >> + SHARED_FIELD_EX32(value, M_TX_CMD)) { > >> + bus->regs[R_I2CC_DMA_LEN] = > ARRAY_FIELD_EX32(bus->regs, > >> + > >> I2CM_DMA_LEN, > >> + > >> TX_BUF_LEN) + 1; > >> + } > >> + if (SHARED_FIELD_EX32(value, RX_DMA_EN) && > >> + SHARED_FIELD_EX32(value, M_RX_CMD)) { > >> + bus->regs[R_I2CC_DMA_LEN] = > ARRAY_FIELD_EX32(bus->regs, > >> + > >> I2CM_DMA_LEN, > >> + > >> RX_BUF_LEN) + 1; > >> + } > >> + > >> if (bus->regs[R_I2CM_INTR_STS] & 0xffff0000) { > >> qemu_log_mask(LOG_UNIMP, "%s: Packet mode is not > >> implemented\n", > >> __func__); @@ -656,8 +670,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 +677,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) > || > >> -- > >> 2.43.0 > >
>>> After discussing with the I2C hardware designers, we confirmed that the I2c >>> design in AST2600 and AST2700 A1 is the same. >>> The datasheet will be updated accordingly for AST2700. >>> >>> However, please note that bit 15 and bit 31 are not available on AST2700 A0 >>> and FW do not set either bit 15 and bit 31. >>> AST2700 A0 was an engineering sample version. Given this, I plan to resend >>> the v3 patch with AST2700 A0 explicitly marked as unsupported. >> >> Are you going to introduce an I2C property to distinguish the >> A0 implementation for the A1 ? >> > > No. I want the I2C model to be as simple as possible, without adding any ""AST2700 A0"" specific changes. > V1 patch work for AST2700 A1, AST2600, AST2500, AST1030, AST2500, AST2400 but not AST2700 A0. ok. Please first adjust the ast2700 functional tests for the ast2700a0-evb machine (no I2C test) and deprecate ast2700a0-evb at the same time. Thanks, C.
diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c index a8fbb9f44a..c659099e9a 100644 --- a/hw/i2c/aspeed_i2c.c +++ b/hw/i2c/aspeed_i2c.c @@ -634,6 +634,20 @@ static void aspeed_i2c_bus_new_write(AspeedI2CBus *bus, hwaddr offset, break; } + /* Handle DMA length */ + if (SHARED_FIELD_EX32(value, TX_DMA_EN) && + SHARED_FIELD_EX32(value, M_TX_CMD)) { + bus->regs[R_I2CC_DMA_LEN] = ARRAY_FIELD_EX32(bus->regs, + I2CM_DMA_LEN, + TX_BUF_LEN) + 1; + } + if (SHARED_FIELD_EX32(value, RX_DMA_EN) && + SHARED_FIELD_EX32(value, M_RX_CMD)) { + bus->regs[R_I2CC_DMA_LEN] = ARRAY_FIELD_EX32(bus->regs, + I2CM_DMA_LEN, + RX_BUF_LEN) + 1; + } + if (bus->regs[R_I2CM_INTR_STS] & 0xffff0000) { qemu_log_mask(LOG_UNIMP, "%s: Packet mode is not implemented\n", __func__); @@ -656,8 +670,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 +677,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) ||
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. Ideally, this issue should be resolved by updating the model to set I2CC_DMA_LEN (0x54) when the firmware writes to the I2CM_DMA_LEN (0x1C) register, instead of when it writes to I2CM_DMA_TX_ADDR (0x30) or I2CM_DMA_RX_ADDR (0x34). Originally, the design of I2CM_DMA_LEN (0x1C) included buffer length write-enable bits for the current command: Bit 31 enabled the RX buffer length update Bit 15 enabled the TX buffer length update In other words, when the firmware set either bit 31 or bit 15, the I2C model could safely update I2CC_DMA_LEN (0x54) with the value in I2CM_DMA_LEN (0x1C). However, starting with the AST2700, the design of the I2CM_DMA_LEN (0x1C) register was changed. The write-enable bits (bit 31 and bit 15) were removed, meaning there is no longer an explicit indication of whether the firmware intends to update the TX or RX length. As a result, on AST2700 and newer SoCs, the model cannot reliably determine whether a write to I2CM_DMA_LEN was meant for TX or RX. This ambiguity is especially problematic when the value written is 0, which actually corresponds to a DMA length of 1. To ensure consistent behavior across all SoCs, the model now updates I2CC_DMA_LEN when I2CM_CMD (0x18) is written, as this is the final command that initiates a TX or RX transfer and reflects the firmware’s intent more clearly. Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com> Fixes: ba2cccd (aspeed: i2c: Add new mode support) --- hw/i2c/aspeed_i2c.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-)