1. Patchew
  2. QEMU
  3. [PATCH 00/14] aspeed: Add I2C new register DMA slave mode support
  4. View patch

[PATCH 04/14] aspeed: i2c: Fix DMA len write-enable bit handling

Peter Delevoryas posted 14 patches 3 years, 7 months ago
Only 11 patches received!
[PATCH 04/14] aspeed: i2c: Fix DMA len write-enable bit handling
Posted by Peter Delevoryas 3 years, 7 months ago 
I noticed i2c rx transfers were getting shortened to "1" on Zephyr. It
seems to be because the Zephyr i2c driver sets the RX DMA len with the
RX field write-enable bit set (bit 31) to avoid a read-modify-write. [1]

/* 0x1C : I2CM Master DMA Transfer Length Register   */

I think we should be checking the write-enable bits on the incoming
value, not checking the register array. I'm not sure we're even writing
the write-enable bits to the register array, actually.

[1] https://github.com/AspeedTech-BMC/zephyr/blob/db3dbcc9c52e67a47180890ac938ed380b33f91c/drivers/i2c/i2c_aspeed.c#L145-L148

Fixes: ba2cccd64e90f34 ("aspeed: i2c: Add new mode support")
Signed-off-by: Peter Delevoryas <pdel@fb.com>
---
 hw/i2c/aspeed_i2c.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
index 2cfd05cb6c..6c8222717f 100644
--- a/hw/i2c/aspeed_i2c.c
+++ b/hw/i2c/aspeed_i2c.c
@@ -644,18 +644,18 @@ static void aspeed_i2c_bus_new_write(AspeedI2CBus *bus, hwaddr offset,
                                                      RX_BUF_LEN) + 1;
         break;
     case A_I2CM_DMA_LEN:
-        w1t = ARRAY_FIELD_EX32(bus->regs, I2CM_DMA_LEN, RX_BUF_LEN_W1T) ||
-                   ARRAY_FIELD_EX32(bus->regs, I2CM_DMA_LEN, TX_BUF_LEN_W1T);
+        w1t = FIELD_EX32(value, I2CM_DMA_LEN, RX_BUF_LEN_W1T) ||
+              FIELD_EX32(value, I2CM_DMA_LEN, TX_BUF_LEN_W1T);
         /* If none of the w1t bits are set, just write to the reg as normal. */
         if (!w1t) {
             bus->regs[R_I2CM_DMA_LEN] = value;
             break;
         }
-        if (ARRAY_FIELD_EX32(bus->regs, I2CM_DMA_LEN, RX_BUF_LEN_W1T)) {
+        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));
         }
-        if (ARRAY_FIELD_EX32(bus->regs, I2CM_DMA_LEN, TX_BUF_LEN_W1T)) {
+        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));
         }
-- 
2.30.2
Re: [PATCH 04/14] aspeed: i2c: Fix DMA len write-enable bit handling
Posted by Cédric Le Goater 3 years, 7 months ago 
On 6/27/22 21:54, Peter Delevoryas wrote:
> I noticed i2c rx transfers were getting shortened to "1" on Zephyr. It
> seems to be because the Zephyr i2c driver sets the RX DMA len with the
> RX field write-enable bit set (bit 31) to avoid a read-modify-write. [1]
> 
> /* 0x1C : I2CM Master DMA Transfer Length Register   */
> 
> I think we should be checking the write-enable bits on the incoming
> value, not checking the register array. I'm not sure we're even writing
> the write-enable bits to the register array, actually.
> 
> [1] https://github.com/AspeedTech-BMC/zephyr/blob/db3dbcc9c52e67a47180890ac938ed380b33f91c/drivers/i2c/i2c_aspeed.c#L145-L148
> 
> Fixes: ba2cccd64e90f34 ("aspeed: i2c: Add new mode support")
> Signed-off-by: Peter Delevoryas <pdel@fb.com>
> ---

Nice. Can you move this patch at beginning of the series ?

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.


>   hw/i2c/aspeed_i2c.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
> index 2cfd05cb6c..6c8222717f 100644
> --- a/hw/i2c/aspeed_i2c.c
> +++ b/hw/i2c/aspeed_i2c.c
> @@ -644,18 +644,18 @@ static void aspeed_i2c_bus_new_write(AspeedI2CBus *bus, hwaddr offset,
>                                                        RX_BUF_LEN) + 1;
>           break;
>       case A_I2CM_DMA_LEN:
> -        w1t = ARRAY_FIELD_EX32(bus->regs, I2CM_DMA_LEN, RX_BUF_LEN_W1T) ||
> -                   ARRAY_FIELD_EX32(bus->regs, I2CM_DMA_LEN, TX_BUF_LEN_W1T);
> +        w1t = FIELD_EX32(value, I2CM_DMA_LEN, RX_BUF_LEN_W1T) ||
> +              FIELD_EX32(value, I2CM_DMA_LEN, TX_BUF_LEN_W1T);
>           /* If none of the w1t bits are set, just write to the reg as normal. */
>           if (!w1t) {
>               bus->regs[R_I2CM_DMA_LEN] = value;
>               break;
>           }
> -        if (ARRAY_FIELD_EX32(bus->regs, I2CM_DMA_LEN, RX_BUF_LEN_W1T)) {
> +        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));
>           }
> -        if (ARRAY_FIELD_EX32(bus->regs, I2CM_DMA_LEN, TX_BUF_LEN_W1T)) {
> +        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));
>           }
Re: [PATCH 04/14] aspeed: i2c: Fix DMA len write-enable bit handling
Posted by Peter Delevoryas 3 years, 7 months ago 

> On Jun 28, 2022, at 12:01 AM, Cédric Le Goater <clg@kaod.org> wrote:
> 
> On 6/27/22 21:54, Peter Delevoryas wrote:
>> I noticed i2c rx transfers were getting shortened to "1" on Zephyr. It
>> seems to be because the Zephyr i2c driver sets the RX DMA len with the
>> RX field write-enable bit set (bit 31) to avoid a read-modify-write. [1]
>> /* 0x1C : I2CM Master DMA Transfer Length Register   */
>> I think we should be checking the write-enable bits on the incoming
>> value, not checking the register array. I'm not sure we're even writing
>> the write-enable bits to the register array, actually.
>> [1] https://github.com/AspeedTech-BMC/zephyr/blob/db3dbcc9c52e67a47180890ac938ed380b33f91c/drivers/i2c/i2c_aspeed.c#L145-L148
>> Fixes: ba2cccd64e90f34 ("aspeed: i2c: Add new mode support")
>> Signed-off-by: Peter Delevoryas <pdel@fb.com>
>> ---
> 
> Nice. Can you move this patch at beginning of the series ?

Yeah sure! I’ll do that on v2.

> 
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> 
> Thanks,
> 
> C.
> 
> 
>>  hw/i2c/aspeed_i2c.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>> diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
>> index 2cfd05cb6c..6c8222717f 100644
>> --- a/hw/i2c/aspeed_i2c.c
>> +++ b/hw/i2c/aspeed_i2c.c
>> @@ -644,18 +644,18 @@ static void aspeed_i2c_bus_new_write(AspeedI2CBus *bus, hwaddr offset,
>>                                                       RX_BUF_LEN) + 1;
>>          break;
>>      case A_I2CM_DMA_LEN:
>> -        w1t = ARRAY_FIELD_EX32(bus->regs, I2CM_DMA_LEN, RX_BUF_LEN_W1T) ||
>> -                   ARRAY_FIELD_EX32(bus->regs, I2CM_DMA_LEN, TX_BUF_LEN_W1T);
>> +        w1t = FIELD_EX32(value, I2CM_DMA_LEN, RX_BUF_LEN_W1T) ||
>> +              FIELD_EX32(value, I2CM_DMA_LEN, TX_BUF_LEN_W1T);
>>          /* If none of the w1t bits are set, just write to the reg as normal. */
>>          if (!w1t) {
>>              bus->regs[R_I2CM_DMA_LEN] = value;
>>              break;
>>          }
>> -        if (ARRAY_FIELD_EX32(bus->regs, I2CM_DMA_LEN, RX_BUF_LEN_W1T)) {
>> +        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));
>>          }
>> -        if (ARRAY_FIELD_EX32(bus->regs, I2CM_DMA_LEN, TX_BUF_LEN_W1T)) {
>> +        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));
>>          }
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.