Message-ID:
 <DM4PR12MB6109367F36487B582ED8EA968C1FA@DM4PR12MB6109.namprd12.prod.outlook.com>
Date: Thu, 25 Sep 2025 07:37:12 +0000
From: "Guntupalli, Manikanta" <manikanta.guntupalli@....com>
To: Jorge Marques <gastmaier@...il.com>, Arnd Bergmann <arnd@...nel.org>
CC: Alexandre Belloni <alexandre.belloni@...tlin.com>, Jorge Marques
	<jorge.marques@...log.com>, Wolfram Sang <wsa+renesas@...g-engineering.com>,
	Frank Li <Frank.Li@....com>, Arnd Bergmann <arnd@...db.de>,
	"linux-i3c@...ts.infradead.org" <linux-i3c@...ts.infradead.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "git
 (AMD-Xilinx)" <git@....com>, "Simek, Michal" <michal.simek@....com>
Subject: RE: [PATCH] [v2] i3c: fix big-endian FIFO transfers

[Public]

Hi,

> -----Original Message-----
> From: Jorge Marques <gastmaier@...il.com>
> Sent: Thursday, September 25, 2025 12:47 PM
> To: Arnd Bergmann <arnd@...nel.org>
> Cc: Alexandre Belloni <alexandre.belloni@...tlin.com>; Jorge Marques
> <jorge.marques@...log.com>; Wolfram Sang <wsa+renesas@...g-
> engineering.com>; Frank Li <Frank.Li@....com>; Arnd Bergmann
> <arnd@...db.de>; Guntupalli, Manikanta <manikanta.guntupalli@....com>; linux-
> i3c@...ts.infradead.org; linux-kernel@...r.kernel.org
> Subject: Re: [PATCH] [v2] i3c: fix big-endian FIFO transfers
>
> On Wed, Sep 24, 2025 at 10:18:33PM +0200, Arnd Bergmann wrote:
> > From: Arnd Bergmann <arnd@...db.de>
> >
> > Short MMIO transfers that are not a multiple of four bytes in size
> > need a special case for the final bytes, however the existing
> > implementation is not endian-safe and introduces an incorrect byteswap
> > on big-endian kernels.
> >
> > This usually does not cause problems because most systems are
> > little-endian and most transfers are multiple of four bytes long, but
> > still needs to be fixed to avoid the extra byteswap.
> >
> > Change the special case for both i3c_writel_fifo() and
> > i3c_readl_fifo() to use non-byteswapping writesl() and readsl() with a
> > single element instead of the byteswapping writel()/readl() that are
> > meant for individual MMIO registers. As data is copied between a FIFO
> > and a memory buffer, the writesl()/readsl() loops are typically based
> > on __raw_readl()/ __raw_writel(), resulting in the order of bytes in
> > the FIFO to match the order in the buffer, regardless of the CPU endianess.
> >
> > The earlier versions in the dw-i3c and i3c-master-cdns had a correct
> > implementation, but the generic version that was recently added broke it.
> >
> > Fixes: 733b439375b4 ("i3c: master: Add inline i3c_readl_fifo() and
> > i3c_writel_fifo()")
> > Cc: Manikanta Guntupalli <manikanta.guntupalli@....com>
> > Signed-off-by: Arnd Bergmann <arnd@...db.de>
> > ---
> > This was a recent regression, the version in 6.16 still works, but
> > 6.17-rc is broken.
> >
> > v2 changes:
> >  - add code comments
> >  - write correct data buffer
> > ---
> >  drivers/i3c/internals.h | 12 ++++++++++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/i3c/internals.h b/drivers/i3c/internals.h index
> > 0d857cc68cc5..79ceaa5f5afd 100644
> > --- a/drivers/i3c/internals.h
> > +++ b/drivers/i3c/internals.h
> > @@ -38,7 +38,11 @@ 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() instead of writel() to keep FIFO
> > +            * byteorder on big-endian targets
> > +            */
> > +           writesl(addr, &tmp, 1);
> >     }
> >  }
> >
> > @@ -55,7 +59,11 @@ static inline void i3c_readl_fifo(const void __iomem *addr,
> void *buf,
> >     if (nbytes & 3) {
> >             u32 tmp;
> >
> > -           tmp = readl(addr);
> > +           /*
> > +            * readsl() instead of readl() to keep FIFO
> > +            * byteorder on big-endian targets
> > +            */
> > +           readsl(addr, &tmp, 1);
> >             memcpy(buf + (nbytes & ~3), &tmp, nbytes & 3);
> >     }
> >  }
> Reviewed-by: Jorge Marques <jorge.marques@...log.com>
> > --
> > 2.39.5
> >

This patch fixes the sub-word transfer case on big-endian kernels, but it still does not address the scenario of little-endian kernels accessing big-endian FIFOs.

With the current version, i3c_writel_fifo() and i3c_readl_fifo() only work when the FIFO has the same endianness as the CPU. On platforms such as the ZCU102 (Zynq UltraScale+ MPSoC, Cortex-A53, little-endian), the I3C FIFOs are big-endian, and this patch alone is not sufficient - transfers fail in that configuration.

We have validated this on ZCU102, and the mismatch between LE kernel and BE FIFO is still an issue.

On top of this fix, explicit FIFO endianness support is required, as proposed in [PATCH v7 3/4] "i3c: master: Add endianness support for i3c_readl_fifo() and i3c_writel_fifo()". That approach adds an endian argument and uses writesl_be()/readsl_be() where necessary, e.g.:

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);
        }
}


Thanks,
Manikanta.

Powered by blists - more mailing lists