Message-ID: <aNVy22ayfAsw+Zgj@lizhi-Precision-Tower-5810>
Date: Thu, 25 Sep 2025 12:50:35 -0400
From: Frank Li <Frank.li@....com>
To: "Guntupalli, Manikanta" <manikanta.guntupalli@....com>
Cc: Arnd Bergmann <arnd@...db.de>, "git (AMD-Xilinx)" <git@....com>,
	"Simek, Michal" <michal.simek@....com>,
	Alexandre Belloni <alexandre.belloni@...tlin.com>,
	Rob Herring <robh@...nel.org>,
	"krzk+dt@...nel.org" <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Przemysław Gaj <pgaj@...ence.com>,
	Wolfram Sang <wsa+renesas@...g-engineering.com>,
	"tommaso.merciai.xr@...renesas.com" <tommaso.merciai.xr@...renesas.com>,
	"quic_msavaliy@...cinc.com" <quic_msavaliy@...cinc.com>,
	"S-k, Shyam-sundar" <Shyam-sundar.S-k@....com>,
	Sakari Ailus <sakari.ailus@...ux.intel.com>,
	"'billy_tsai@...eedtech.com'" <billy_tsai@...eedtech.com>,
	Kees Cook <kees@...nel.org>,
	"Gustavo A. R. Silva" <gustavoars@...nel.org>,
	Jarkko Nikula <jarkko.nikula@...ux.intel.com>,
	Jorge Marques <jorge.marques@...log.com>,
	"linux-i3c@...ts.infradead.org" <linux-i3c@...ts.infradead.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Linux-Arch <linux-arch@...r.kernel.org>,
	"linux-hardening@...r.kernel.org" <linux-hardening@...r.kernel.org>,
	"Pandey, Radhey Shyam" <radhey.shyam.pandey@....com>,
	"Goud, Srinivas" <srinivas.goud@....com>,
	"Datta, Shubhrajyoti" <shubhrajyoti.datta@....com>,
	"manion05gk@...il.com" <manion05gk@...il.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 04:37:54PM +0000, Guntupalli, Manikanta wrote:
> [Public]
>
> Hi,
>
> > -----Original Message-----
> > From: Arnd Bergmann <arnd@...db.de>
> > Sent: Thursday, September 25, 2025 5:45 PM
> > To: Guntupalli, Manikanta <manikanta.guntupalli@....com>; git (AMD-Xilinx)
> > <git@....com>; Simek, Michal <michal.simek@....com>; Alexandre Belloni
> > <alexandre.belloni@...tlin.com>; Frank Li <Frank.Li@....com>; Rob Herring
> > <robh@...nel.org>; krzk+dt@...nel.org; Conor Dooley <conor+dt@...nel.org>;
> > Przemysław Gaj <pgaj@...ence.com>; Wolfram Sang <wsa+renesas@...g-
> > engineering.com>; tommaso.merciai.xr@...renesas.com;
> > quic_msavaliy@...cinc.com; S-k, Shyam-sundar <Shyam-sundar.S-k@....com>;
> > Sakari Ailus <sakari.ailus@...ux.intel.com>; 'billy_tsai@...eedtech.com'
> > <billy_tsai@...eedtech.com>; Kees Cook <kees@...nel.org>; Gustavo A. R. Silva
> > <gustavoars@...nel.org>; Jarkko Nikula <jarkko.nikula@...ux.intel.com>; Jorge
> > Marques <jorge.marques@...log.com>; linux-i3c@...ts.infradead.org;
> > devicetree@...r.kernel.org; linux-kernel@...r.kernel.org; Linux-Arch <linux-
> > arch@...r.kernel.org>; linux-hardening@...r.kernel.org
> > Cc: Pandey, Radhey Shyam <radhey.shyam.pandey@....com>; Goud, Srinivas
> > <srinivas.goud@....com>; Datta, Shubhrajyoti <shubhrajyoti.datta@....com>;
> > manion05gk@...il.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.

Yes, 32 bit FIFO generally two orders

BIT 31..24, 23..16, 15..8, 7..0
    B3      B2      B1     B0
    A0      A1      A2     A3

Need write/read to memory as

0x00     B0       A0
0x01     B1       A1
0x02     B2       A2
0x03     B3       A3

We don't expect every driver need duplicate to handle these two kind order
and trail data. Just need clear API defination to avoid confusiton at
difference cpu endian system.

Frank

>
> Thanks,
> Manikanta.

Powered by blists - more mailing lists