Re: [PATCH] Revert "regmap-mmio: Use native endianness for read/write"

[Johannes Berg]

On Mon, 2016-01-25 at 23:24 +0100, Arnd Bergmann wrote:
> On Monday 25 January 2016 23:07:55 Johannes Berg wrote:
> > This reverts commit 29bb45f25ff3051354ed330c0d0f10418a2b8c7c.
> > 
> > Clearly, using "native" endianness is a terrible idea when
> > devices are involved, since those devices are different hw
> > from the CPU, won't change, and the CPU might be able to
> > run in both big and little endian, like ARM and PowerPC can.
> > Therefore, "native" endian doesn't really exist.
> > 
> > Consequently, this commit broke my HummingBoard i.MX6 in big
> > endian mode since it would now try to talk to the little
> > endian hardware with a big endian CPU without conversion.
> > 
> > What the patch really would have to do is introduce some kind
> > of "device-endian" readl/writel, that takes the endianness of
> > the device as an argument. That seems a bit overkill though,
> > and would likely not generate any better code than the double
> > byte-swaps that MIPS is getting now.
> > 
> > Therefore, simply revert the commit to fix the breakage.
> > 
> > Fixes: 29bb45f25ff3 ("regmap-mmio: Use native endianness for
> > read/write")
> > Signed-off-by: Johannes Berg <johannes@xxxxxxxxxxxxxxxx>
> 
> I think it's more complicated than this unfortunately:
> 
> Most architectures behave the same way you explain: all I/O registers
> are fixed-endian, and the CPU may also be fixed-endian or may
> support both using a runtime switch, which we abstract using the
> readl/writel etc helpers in Linux.
> 
> On MIPS, the CPU endianess is set at through an input signal
> on the CPU core, and this cannot change during runtime but can
> change between SoCs or can be configurable with a hardware jumper.
> Some SoC vendors (notably Broadcom) decided to use the same signal
> to control whether there is a byteswap on the device bus or not,
> so the on-chip MMIO registers are always the same endianess as
> the CPU itself.	

It's even *more* complicated :)

> This means that the devices are in fact CPU-endian, and we need
> some way for Linux to represent this. The patch to
> drivers/base/regmap/regmap-mmio.c is clearly wrong, as we
> must never use __raw_*() accessors in an architecture independent
> driver (for a number of reasons), but we still need a fix for
> MIPS so it can specify a way to do the double-swap without
> faking the endianess of the registers.

Mark points out that the regmap core (regmap.c) *does* in fact do the
byteswapping correctly, so it's fairly likely that - as odd as this
seems - using the __raw_*() accessors is actually correct.

> Also, defaulting syscon to "native-endian" when nothing else is
> specified sounds like a bad idea, but we may already be stuck there
> with the precedent in existing bindings after 6a55244e897d
> ("regmap: mmio: request native endian formatting"), we'll have
> to think about that some more.
> 

I can also get it back to working by adding "little-endian;" to the
iomuxc-gpr@020e0000Âsyscon DT node in the dtb that I load, but perhaps
making the default architecture dependent and only using native for
MIPS might be better?

Or, perhaps, the code should just WARN() when endianness is unspecified
(assuming you can actually specify "native-endian;") and we need to fix
all of the instances...

johannes