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 11:00, Guntupalli, Manikanta wrote:
>> >     if (nbytes & 3) {
>> >             u32 tmp = 0;
>> >
>> >             memcpy(&tmp, buf + (nbytes & ~3), nbytes & 3);
>> > -           writel(tmp, addr);
>> > +
>> > +           if (endian)
>> > +                   writel_be(tmp, addr);
>> > +           else
>> > +                   writel(tmp, addr);
>>
>> This bit however seems to fix a bug, but does so in a confusing way. The way the
>> FIFO registers usually deal with excess bytes is to put them into the first bytes of the
>> FIFO register, so this should just be a
>>
>>        writesl(addr, &tmp, 1);
>>
>> to write one set of four bytes into the FIFO without endian-swapping.
>>
>> Could it be that you are just trying to use a normal i3c adapter with little-endian
>> registers on a normal big-endian machine but ran into this bug?
> The intention here is to enforce big-endian ordering for the trailing 
> bytes as well. By using __cpu_to_be32() for full words and writel_be() 
> for the leftover word, the FIFO is always accessed in big-endian 
> format, regardless of the CPU's native endianness. This ensures 
> consistent and correct data ordering from the device's perspective.

No, this makes no sense: The 'else' portion is incorrect in the
function, and prevents it from working on all big-endian CPUs because
'writel()' only works for little-endian 32-bit registers. If you just
fix the bug as I described above, this will work correctly on any
driver calling the current function. At that point, your hack becomes
a nop for big-endian systems, while calling the function with
'endian = true' on little-endian kernels is still wrong.

Please start by fixing the existing bug and test that again.

I know that endianess with FIFO registers is hard to understand,
and everyone has to spend the time once to actually wrap their
head around this. Even if you don't believe me, please try the
bugfix below (without your added argument) and think about how
FIFO registers that transfer byte streams are different of
fixed-endian 32-bit registers. Note that your driver uses
little-endian readl() for normal registers, and this is portable
to both LE and BE kernels.

See also the explanation in 41739ee355ab ("asm-generic: io:
don't perform swab during {in,out} string functions").

     Arnd

diff --git a/drivers/i3c/internals.h b/drivers/i3c/internals.h
index 0d857cc68cc5..0f8a25cb71e7 100644
--- a/drivers/i3c/internals.h
+++ b/drivers/i3c/internals.h
@@ -38,7 +38,7 @@ static inline void i3c_writel_fifo(void __iomem *addr, const void *buf,
 		u32 tmp = 0;
 
 		memcpy(&tmp, buf + (nbytes & ~3), nbytes & 3);
-		writel(tmp, addr);
+		writesl(addr, &buf, 1);
 	}
 }
 
@@ -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);
 	}
 }

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c