[Public] Hi,
> -----Original Message----- > From: Arnd Bergmann <[email protected]> > Sent: Wednesday, September 24, 2025 3:30 PM > To: Guntupalli, Manikanta <[email protected]>; git (AMD-Xilinx) > <[email protected]>; Simek, Michal <[email protected]>; Alexandre Belloni > <[email protected]>; Frank Li <[email protected]>; Rob Herring > <[email protected]>; [email protected]; Conor Dooley <[email protected]>; > Przemysław Gaj <[email protected]>; Wolfram Sang <wsa+renesas@sang- > engineering.com>; [email protected]; > [email protected]; S-k, Shyam-sundar <[email protected]>; > Sakari Ailus <[email protected]>; '[email protected]' > <[email protected]>; Kees Cook <[email protected]>; Gustavo A. R. Silva > <[email protected]>; Jarkko Nikula <[email protected]>; Jorge > Marques <[email protected]>; [email protected]; > [email protected]; [email protected]; Linux-Arch <linux- > [email protected]>; [email protected] > Cc: Pandey, Radhey Shyam <[email protected]>; Goud, Srinivas > <[email protected]>; Datta, Shubhrajyoti <[email protected]>; > [email protected] > Subject: Re: [PATCH V7 3/4] i3c: master: Add endianness support for > i3c_readl_fifo() > and i3c_writel_fifo() > > 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); > } > } 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). 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. Thanks, Manikanta.
