[Public] Hi,
> -----Original Message----- > From: Arnd Bergmann <[email protected]> > Sent: Wednesday, September 24, 2025 12:21 AM > 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 Tue, Sep 23, 2025, at 17:45, Manikanta Guntupalli wrote: > > /** > > * i3c_writel_fifo - Write data buffer to 32bit FIFO > > * @addr: FIFO Address to write to > > * @buf: Pointer to the data bytes to write > > * @nbytes: Number of bytes to write > > + * @endian: Endianness of FIFO write > > */ > > static inline void i3c_writel_fifo(void __iomem *addr, const void *buf, > > - int nbytes) > > + int nbytes, enum i3c_fifo_endian endian) > > { > > - writesl(addr, buf, nbytes / 4); > > + if (endian) > > + writesl_be(addr, buf, nbytes / 4); > > + else > > + writesl(addr, buf, nbytes / 4); > > + > > This seems counter-intuitive: a FIFO doesn't really have an endianness, it is > instead > used to transfer a stream of bytes, so if the device has a fixed endianess, > the FIFO > still needs to be read using a plain writesl(). > > I see that your writesl_be() has an incorrect definition, which would lead to > the > i3c_writel_fifo() function accidentally still working if both the device and > CPU use > big-endian registers: > > static inline void writesl_be(volatile void __iomem *addr, > const void *buffer, > unsigned int count) > { > if (count) { > const u32 *buf = buffer; > do { > __raw_writel((u32 __force)__cpu_to_be32(*buf), addr); > buf++; > } while (--count); > } > } > > The __cpu_to_be32() call that you add here means that the FIFO data is swapped > on little-endian CPUs but not swapped on big-endian ones. Compare this to the > normal writesl() function that never swaps because it writes a byte stream. The use of __cpu_to_be32() in writesl_be() is intentional. The goal here is to ensure that the FIFO is always accessed in big-endian format, regardless of whether the CPU is little-endian or big-endian. On little-endian CPUs, this introduces the required byte swap before writing; on big-endian CPUs, the data is already in big-endian order, so no additional swap occurs. This guarantees that the FIFO sees consistent big-endian data, matching the hardware's expectation. > > > 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. Thanks, Manikanta.
