Message-Id: <f2171867-f0d4-49d3-b5b1-8467d9721c67@app.fastmail.com>
Date: Thu, 25 Sep 2025 10:55:27 +0200
From: "Arnd Bergmann" <arnd@...db.de>
To: "Manikanta Guntupalli" <manikanta.guntupalli@....com>,
 "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>,
 "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>, "Michal Simek" <michal.simek@....com>
Subject: Re: [PATCH] [v2] i3c: fix big-endian FIFO transfers

On Thu, Sep 25, 2025, at 09:37, Guntupalli, Manikanta wrote:
>
> 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.

Hi Manikanta,

Thanks a lot for testing my patch and the description of
how you tested it. I think there is still a communication
problem because the term "big-endian FIFO" makes no sense
to me.

If you need an extra byteswap on a little-endian arm64 kernel,
what you have is what I would describe as a "byte-reversed FIFO",
presumably as the result of a peripheral that was initially
designed for big-endian MMIO access, but then adapted for use with
little-endian readl() helpers by adding a data swizzle in front
of each MMIO access including both the MMIO registers and the FIFO.

The result of that is that FIFO data comes out reversed in readsl(),
but does so on both little-endian and big-endian arm64 kernels,
because the hardware byte-reverse remains in place regardless of
the CPUs internal state.

If I'm interpreting this correctly, the function you'd
actually need to make the driver work on both big-endian
and little-endian (arm64) kernels would look roughly
like

static inline void i3c_writel_fifo_bytereversed(void __iomem *addr,
                         const void *buf, int nbytes)
{
        /*
         * byteswap each 32-bit word to work around FIFO quirk.
         * Note: this is different from iowrite32be(), which
         * would only swap on little-endian kernels.
         */
        while (nbytes >= 4) {
                __raw_writel(swab32p(buf), addr);
                buf += 4;
                nbytes -= 4;
        }

        if (nbytes > 0) {
                u32 tmp = 0;

                memcpy(&tmp, buf, nbytes);
                swab32s(&tmp);
                __raw_writel(addr, &tmp, 1);
        }
}

The idea here is to have a function that works the same
way as i3c_writel_fifo() but instead of never swapping the
FIFO data, it would always swap it regardless of the CPU
state, making it portable to (most of) the architectures we
support in Linux. In your version of writesl_be(), the
swap would happen on little-endian kernels but not happen
on big-endian kernels, which is inconsistent with the
documented writesl() behavior, and still makes no sense to
me conceptually. 

I think the i3c_writel_fifo_bytereversed() function would
be obscure enough that we don't need it (or the
underlying writesl_bytereversed() helper) and could
be part of your own driver as you had in the original
versions, but that is something for the i3c maintainers
to decide.

Are you able to test big-endian arm64 kernels to verify this?
Linux-next now has a patch to disallow that configuration,
but you should be able to still use v6.17 and earlier if you
have access to big-endian userspace.

     Arnd

Powered by blists - more mailing lists