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

Arnd Bergmann
2025-09-24T14:06:00Z

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