Re: [PATCH V7 3/4] i3c: master: Add endianness support for i3c_readl_fifo() and i3c_writel_fifo()

["Arnd Bergmann" <arnd@arndb.de>]

On Wed, Sep 24, 2025, at 14:22, Guntupalli, Manikanta wrote:

>> @@ -55,7 +55,7 @@ static inline void i3c_readl_fifo(const void __iomem *addr, void
>> *buf,
>>       if (nbytes & 3) {
>>               u32 tmp;
>>
>> -             tmp = readl(addr);
>> +             readsl(addr, &tmp, 1);
>>               memcpy(buf + (nbytes & ~3), &tmp, nbytes & 3);
>>       }
>>  }
>
> We have not observed any issue on little-endian systems in our testing 
> so far (as I mentioned earlier in asm-generic/io.h: Add big-endian MMIO 
> accessors).

Did you test the little-endian system with the 'endian' flag set
to I3C_FIFO_BIG_ENDIAN though? Your v7 code will still work on
little-endian kernels if that flag is set to I3C_FIFO_LITTLE_ENDIAN,
and it will also work on big-endian kernels if the flag is
set to I3C_FIFO_BIG_ENDIAN. But is broken for the other two:

- on little-endian kernels with I3C_FIFO_BIG_ENDIAN, the entire
  data buffer is byteswapped in 32-bit chunks

- on big-endian kernels with I3C_FIFO_LITTLE_ENDIAN, you run into
  the existing bug of the swapped tail word.

> That said, I understand your point about FIFO semantics being different 
> from fixed-endian registers. To cover both cases, we considered using 
> writesl() for little-endian and introducing a writesl_be() helper for 
> big-endian, as shown below:
>
> static inline void i3c_writel_fifo(void __iomem *addr, const void *buf,
>                                    int nbytes, enum i3c_fifo_endian endian)
> {
>         if (endian)
>                 writesl_be(addr, buf, nbytes / 4);
>         else
>                 writesl(addr, buf, nbytes / 4);
>
>         if (nbytes & 3) {
>                 u32 tmp = 0;
>
>                 memcpy(&tmp, buf + (nbytes & ~3), nbytes & 3);
>
>                 if (endian)
>                         writesl_be(addr, &tmp, 1);
>                 else
>                         writesl(addr, &tmp, 1);
>         }
> }
>
> With this approach, both little-endian and big-endian cases works as expected.

This version should fix the cases where you have a big-endian
kernel with either I3C_FIFO_BIG_ENDIAN or I3C_FIFO_LITTLE_ENDIAN,
as neither combination does any byte swaps.

However I'm fairly sure it's still broken for little-endian
kernels when a driver asks for a I3C_FIFO_BIG_ENDIAN conversion,
same as v7.

     Arnd