Re: [PATCH] aspeed: i2c: Fix DMA len write-enable bit handling

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

On 6/24/22 22:34, Peter Delevoryas wrote:
> 
> 
>> On Jun 24, 2022, at 1:31 PM, Peter Delevoryas <pdel@fb.com> 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
> 
> Arg, forgot this:
> 
> Fixes: ba2cccd64e90f34 ("aspeed: i2c: Add new mode support”)
> 
> Should I resend as v2?


No. patchwork did it :

http://patchwork.ozlabs.org/project/qemu-devel/patch/20220624203151.2026355-1-pdel@fb.com/

Thanks,

C.


> Thanks,
> Peter
> 
>>
>> 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 37ae1f2e04..c4fce7474a 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
>>
>