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

[Guntupalli, Manikanta]

[Public]

Hi,

> -----Original Message-----
> From: Arnd Bergmann <arnd at arndb.de>
> Sent: Thursday, September 25, 2025 5:45 PM
> To: Guntupalli, Manikanta <manikanta.guntupalli at amd.com>; git (AMD-Xilinx)
> <git at amd.com>; Simek, Michal <michal.simek at amd.com>; Alexandre Belloni
> <alexandre.belloni at bootlin.com>; Frank Li <Frank.Li at nxp.com>; Rob Herring
> <robh at kernel.org>; krzk+dt at kernel.org; Conor Dooley <conor+dt at kernel.org>;
> Przemysław Gaj <pgaj at cadence.com>; Wolfram Sang <wsa+renesas at sang-
> engineering.com>; tommaso.merciai.xr at bp.renesas.com;
> quic_msavaliy at quicinc.com; S-k, Shyam-sundar <Shyam-sundar.S-k at amd.com>;
> Sakari Ailus <sakari.ailus at linux.intel.com>; 'billy_tsai at aspeedtech.com'
> <billy_tsai at aspeedtech.com>; Kees Cook <kees at kernel.org>; Gustavo A. R. Silva
> <gustavoars at kernel.org>; Jarkko Nikula <jarkko.nikula at linux.intel.com>; Jorge
> Marques <jorge.marques at analog.com>; linux-i3c at lists.infradead.org;
> devicetree at vger.kernel.org; linux-kernel at vger.kernel.org; Linux-Arch <linux-
> arch at vger.kernel.org>; linux-hardening at vger.kernel.org
> Cc: Pandey, Radhey Shyam <radhey.shyam.pandey at amd.com>; Goud, Srinivas
> <srinivas.goud at amd.com>; Datta, Shubhrajyoti <shubhrajyoti.datta at amd.com>;
> manion05gk at gmail.com
> Subject: Re: [PATCH V7 3/4] i3c: master: Add endianness support for i3c_readl_fifo()
> and i3c_writel_fifo()
>
> On Thu, Sep 25, 2025, at 11:26, Guntupalli, Manikanta wrote:
>
> >> Can you explain how that works? What I see is that your
> >> readsl_be()/writesl_be() functions do a byteswap on every four bytes,
> >> so the bytestream that gets copied to/from the FIFO gets garbled, in
> >> particular the final
> >> (unaligned) bytes of the kernel buffer end up in the higher bytes of
> >> the FIFO register rather than the first bytes as they do on a big-endian kernel.
> >>
> >> Are both the big-endian and little-endian kernels in your tests on
> >> microblaze, using the upstream version of asm/io.h? Is there a
> >> hardware byteswap between the CPU local bus and the i3c controller?
> >> If there is one, is it set the same way for both kernels?
> >>
> > To clarify, my testing was performed on the latest upstream kernel on
> > a
> > ZCU102 (Zynq UltraScale+ MPSoC, Cortex-A53, little-endian) with
> > big-endian FIFOs and no bus-level byteswap. For more details, please
> > refer to my reply in Re: [PATCH] [v2] i3c: fix big-endian FIFO
> > transfers.
>
> Ok, thanks fro the clarification. I got confused by your description mentioning big-
> endian in [PATCH V7 3/4] and assumed this would be on a big-endian microblaze
> CPU, after I saw that the original i3c_readl_fifo() had a bug in that configuration that
> your patch addressed and this being a driver for a xilinx design. That fix just turned
> out unrelated to what you were actually trying to do ;-)
>
> > Please don't take this as negative or aggressive-my intention is
> > purely to learn and ensure it works correctly in all cases.
>
> No worries, I should not have jumped to conclusions myself based on what I saw in
> your implementation and assumed that fixing the one bug would address your
> problem as well.
>
> I do understand that your driver clearly needs a special case, we just need to come
> to a conclusion what exactly the hardware does and how to best deal with it. This is
> partly about whether you may be able to use an external DMA engine such as
> xlnx,zynqmp-dma-1.0 or xlnx,zynqmp-dpdma, and whether that would need the
> same byteswap.
>
> If you already plan to add that support, you likely need to allocate a bounce buffer
> and byteswap the data in place to have it copied in and out of the FIFO, and when
> you have that, you can use the regular i3c_readl_fifo() unchanged.
> If you are sure that the i3c controller is not going to be used with DMA, or if the FIFO
> register as seen by the DMA master does not require a byteswap, having a local
> helper for the transfer is likely easier.
>
Thanks for understanding.

The I3C controller is not going to be used with DMA in our case.

We had initially implemented local helpers, but based on Frank Li's suggestion, we added the support in i3c_readl_fifo() and i3c_writel_fifo() to make it more generic and beneficial for others as well. That's why we included it here.

Thanks,
Manikanta.