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

[Mark Brown]

On Mon, Jan 25, 2016 at 11:34:41PM +0100, Johannes Berg wrote:

> 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...

That is probably going to be too much work even if just for MMIO
regmaps, there's too many of them out there.

So, recapping our discussion on IRC a bit here there's two things going
on.  We've got how the regmaps specify their configuration to regmap
which is currently less obvious than it should be for MMIO buses but
boiled down to native/default being little endian in the past since we
were using readl() and writel().  There's also the issue of how we write
out to the device which is currently wrong since __raw accessors are not
protected against things like spinlocks.

This is currently a mess since this code was originally written on
little endian systems and it never registered that readl() and writel()
did any byte swapping so the code has in fact always been writing out
little endian values.  I also note that currently regmap-mmio is using
the byte stream APIs originally intended for I2C and SPI rather than
reg_read() and reg_write() which are a much better fit but were added
later.

I think where we want to end up is with little endian being the default
for MMIO regmaps since that's what we were doing up until this change
and with the MIPS DT changes applied since the DT that was there was
obvious nonsense.  Unfortunately MIPS complicates this since it really
does want native endianness.  

I need to think this through properly but I think what we want to do
here is:

 - Make the default for MMIO regmaps be explicitly little endian with
   either an ifdef for MIPS to keep it working or an explict native
   endianness tag in the DT instead of the straight revert to LE (the
   latter seems better).
 - Convert the MMIO regmap to use reg_read() and reg_write() with
   implementations using either readX() or ioread_*be() and equivalents
   for write.  This means the core does no endianness swapping and it's
   all in the bus.

Unfortunately that all sounds a bit too big for v4.5...  perhaps a
combination of a revert of the implementation and the addition of the
native tag to the DT for v4.5 followed by the reworking of the bus for
v4.6, I really would rather keep the DT change in v4.5 since specifying
LE is just bad and we don't want that to propagate any more than it has
to.

From this I also conclude that we need to improve our testing of big
endian ARM systems since nobody managed to notice this all the time this
was cooking in -next.

Attachment: signature.asc
Description: PGP signature