Message-ID:
 <DM4PR12MB6109D513FD8E43B76D501ACC8C1EA@DM4PR12MB6109.namprd12.prod.outlook.com>
Date: Fri, 26 Sep 2025 10:38:08 +0000
From: "Guntupalli, Manikanta" <manikanta.guntupalli@....com>
To: Frank Li <Frank.li@....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>,
	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: Frank Li <Frank.li@....com>
> Sent: Thursday, September 25, 2025 8:39 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>; 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
> > +            */
>
> endian look like how CPU decode byte order to MSB to LSB.
> targets FIFO define look like
>
> BIT 31..24   23..16     15..8     7..0
>     B3        B2         B1       B0
>
> regardless CPU is big endian or little endian system, data in memory should be
>
> 0x000 B0
> 0x004 B1
> 0x008 B2
> 0x00c B3
>
> I think your sentence in commit message is better
>
> /* writesl() instead of writel() to keep FIFO byte orderer to match the order in the
> buffer regardless of the CPU endianess.
> */
>
> Frank
> > +           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);
> >     }
> >  }
> > --
> > 2.39.5
> >
> >
> > --
> > linux-i3c mailing list
> > linux-i3c@...ts.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-i3c

We will include this patch as part of "[PATCH V8 0/5] Add AMD I3C master controller driver and bindings", since the patch series depends on it.

Thanks,
Manikanta.

Powered by blists - more mailing lists