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

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

[Johannes Berg]

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@sipsolutions.net>
---
 arch/mips/boot/dts/brcm/bcm6328.dtsi |  1 +
 arch/mips/boot/dts/brcm/bcm7125.dtsi |  1 +
 arch/mips/boot/dts/brcm/bcm7346.dtsi |  1 +
 arch/mips/boot/dts/brcm/bcm7358.dtsi |  1 +
 arch/mips/boot/dts/brcm/bcm7360.dtsi |  1 +
 arch/mips/boot/dts/brcm/bcm7362.dtsi |  1 +
 arch/mips/boot/dts/brcm/bcm7420.dtsi |  1 +
 arch/mips/boot/dts/brcm/bcm7425.dtsi |  1 +
 arch/mips/boot/dts/brcm/bcm7435.dtsi |  1 +
 drivers/base/regmap/regmap-mmio.c    | 16 ++++++++--------
 10 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/arch/mips/boot/dts/brcm/bcm6328.dtsi b/arch/mips/boot/dts/brcm/bcm6328.dtsi
index 459b9b252c3b..d61b1616b604 100644
--- a/arch/mips/boot/dts/brcm/bcm6328.dtsi
+++ b/arch/mips/boot/dts/brcm/bcm6328.dtsi
@@ -74,6 +74,7 @@
 		timer: timer@10000040 {
 			compatible = "syscon";
 			reg = <0x10000040 0x2c>;
+			little-endian;
 		};
 
 		reboot {
diff --git a/arch/mips/boot/dts/brcm/bcm7125.dtsi b/arch/mips/boot/dts/brcm/bcm7125.dtsi
index 4fc7ecee273c..1a7efa883c5e 100644
--- a/arch/mips/boot/dts/brcm/bcm7125.dtsi
+++ b/arch/mips/boot/dts/brcm/bcm7125.dtsi
@@ -98,6 +98,7 @@
 		sun_top_ctrl: syscon@404000 {
 			compatible = "brcm,bcm7125-sun-top-ctrl", "syscon";
 			reg = <0x404000 0x60c>;
+			little-endian;
 		};
 
 		reboot {
diff --git a/arch/mips/boot/dts/brcm/bcm7346.dtsi b/arch/mips/boot/dts/brcm/bcm7346.dtsi
index a3039bb53477..d4bf52cfcf17 100644
--- a/arch/mips/boot/dts/brcm/bcm7346.dtsi
+++ b/arch/mips/boot/dts/brcm/bcm7346.dtsi
@@ -118,6 +118,7 @@
 		sun_top_ctrl: syscon@404000 {
 			compatible = "brcm,bcm7346-sun-top-ctrl", "syscon";
 			reg = <0x404000 0x51c>;
+			little-endian;
 		};
 
 		reboot {
diff --git a/arch/mips/boot/dts/brcm/bcm7358.dtsi b/arch/mips/boot/dts/brcm/bcm7358.dtsi
index 4274ff41ec21..8e2501694d03 100644
--- a/arch/mips/boot/dts/brcm/bcm7358.dtsi
+++ b/arch/mips/boot/dts/brcm/bcm7358.dtsi
@@ -112,6 +112,7 @@
 		sun_top_ctrl: syscon@404000 {
 			compatible = "brcm,bcm7358-sun-top-ctrl", "syscon";
 			reg = <0x404000 0x51c>;
+			little-endian;
 		};
 
 		reboot {
diff --git a/arch/mips/boot/dts/brcm/bcm7360.dtsi b/arch/mips/boot/dts/brcm/bcm7360.dtsi
index 0dcc9163c27b..7e5f76040fb8 100644
--- a/arch/mips/boot/dts/brcm/bcm7360.dtsi
+++ b/arch/mips/boot/dts/brcm/bcm7360.dtsi
@@ -112,6 +112,7 @@
 		sun_top_ctrl: syscon@404000 {
 			compatible = "brcm,bcm7360-sun-top-ctrl", "syscon";
 			reg = <0x404000 0x51c>;
+			little-endian;
 		};
 
 		reboot {
diff --git a/arch/mips/boot/dts/brcm/bcm7362.dtsi b/arch/mips/boot/dts/brcm/bcm7362.dtsi
index 2f3f9fc2c478..c739ea77acb0 100644
--- a/arch/mips/boot/dts/brcm/bcm7362.dtsi
+++ b/arch/mips/boot/dts/brcm/bcm7362.dtsi
@@ -118,6 +118,7 @@
 		sun_top_ctrl: syscon@404000 {
 			compatible = "brcm,bcm7362-sun-top-ctrl", "syscon";
 			reg = <0x404000 0x51c>;
+			little-endian;
 		};
 
 		reboot {
diff --git a/arch/mips/boot/dts/brcm/bcm7420.dtsi b/arch/mips/boot/dts/brcm/bcm7420.dtsi
index bee221b3b568..5f55d0a50a28 100644
--- a/arch/mips/boot/dts/brcm/bcm7420.dtsi
+++ b/arch/mips/boot/dts/brcm/bcm7420.dtsi
@@ -99,6 +99,7 @@
 		sun_top_ctrl: syscon@404000 {
 			compatible = "brcm,bcm7420-sun-top-ctrl", "syscon";
 			reg = <0x404000 0x60c>;
+			little-endian;
 		};
 
 		reboot {
diff --git a/arch/mips/boot/dts/brcm/bcm7425.dtsi b/arch/mips/boot/dts/brcm/bcm7425.dtsi
index 571f30f52e3f..e24d41ab4e30 100644
--- a/arch/mips/boot/dts/brcm/bcm7425.dtsi
+++ b/arch/mips/boot/dts/brcm/bcm7425.dtsi
@@ -100,6 +100,7 @@
 		sun_top_ctrl: syscon@404000 {
 			compatible = "brcm,bcm7425-sun-top-ctrl", "syscon";
 			reg = <0x404000 0x51c>;
+			little-endian;
 		};
 
 		reboot {
diff --git a/arch/mips/boot/dts/brcm/bcm7435.dtsi b/arch/mips/boot/dts/brcm/bcm7435.dtsi
index 614ee211f71a..8b9432cc062b 100644
--- a/arch/mips/boot/dts/brcm/bcm7435.dtsi
+++ b/arch/mips/boot/dts/brcm/bcm7435.dtsi
@@ -114,6 +114,7 @@
 		sun_top_ctrl: syscon@404000 {
 			compatible = "brcm,bcm7425-sun-top-ctrl", "syscon";
 			reg = <0x404000 0x51c>;
+			little-endian;
 		};
 
 		reboot {
diff --git a/drivers/base/regmap/regmap-mmio.c b/drivers/base/regmap/regmap-mmio.c
index 8812bfb9e3b8..eea51569f0eb 100644
--- a/drivers/base/regmap/regmap-mmio.c
+++ b/drivers/base/regmap/regmap-mmio.c
@@ -133,17 +133,17 @@ static int regmap_mmio_gather_write(void *context,
 	while (val_size) {
 		switch (ctx->val_bytes) {
 		case 1:
-			__raw_writeb(*(u8 *)val, ctx->regs + offset);
+			writeb(*(u8 *)val, ctx->regs + offset);
 			break;
 		case 2:
-			__raw_writew(*(u16 *)val, ctx->regs + offset);
+			writew(*(u16 *)val, ctx->regs + offset);
 			break;
 		case 4:
-			__raw_writel(*(u32 *)val, ctx->regs + offset);
+			writel(*(u32 *)val, ctx->regs + offset);
 			break;
 #ifdef CONFIG_64BIT
 		case 8:
-			__raw_writeq(*(u64 *)val, ctx->regs + offset);
+			writeq(*(u64 *)val, ctx->regs + offset);
 			break;
 #endif
 		default:
@@ -193,17 +193,17 @@ static int regmap_mmio_read(void *context,
 	while (val_size) {
 		switch (ctx->val_bytes) {
 		case 1:
-			*(u8 *)val = __raw_readb(ctx->regs + offset);
+			*(u8 *)val = readb(ctx->regs + offset);
 			break;
 		case 2:
-			*(u16 *)val = __raw_readw(ctx->regs + offset);
+			*(u16 *)val = readw(ctx->regs + offset);
 			break;
 		case 4:
-			*(u32 *)val = __raw_readl(ctx->regs + offset);
+			*(u32 *)val = readl(ctx->regs + offset);
 			break;
 #ifdef CONFIG_64BIT
 		case 8:
-			*(u64 *)val = __raw_readq(ctx->regs + offset);
+			*(u64 *)val = readq(ctx->regs + offset);
 			break;
 #endif
 		default:
-- 
2.6.2

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

[Arnd Bergmann]

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@sipsolutions.net>

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.	

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.

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.

	Arnd

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@sipsolutions.net>
> 
> 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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2676 bytes --]

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

[Johannes Berg]

On Mon, 2016-01-25 at 23:52 +0000, Mark Brown wrote:

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

This makes sense, and I agree that the latter is 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.

I don't think there's ioread64be/iowrite64be, and I'm also not entirely
sure how that works since it uses __raw_* internally in lib/iomap.c?

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

Yes, that makes sense.

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

To my knowledge before I did a couple of days ago nobody ever ran i.MX6
in big endian mode :)

johannes

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

[Arnd Bergmann]

On Tuesday 26 January 2016 09:25:36 Johannes Berg wrote:
> On Mon, 2016-01-25 at 23:52 +0000, Mark Brown wrote:
> 
> >  - 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).
> 
> This makes sense, and I agree that the latter is better.

+1

> >  - 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.
> 
> I don't think there's ioread64be/iowrite64be, and I'm also not entirely
> sure how that works since it uses __raw_* internally in lib/iomap.c?

Again, it's complicated: 

* We should probably add ioread64be()/iowrite64be() on *64bit* architectures
  for consistency with readq/writeq

* On 32-bit architectures, you generally cannot do 64-bit atomic I/O
  operations, and we have two implementations that do it nonatomically,
  depending on how a device is wired to the bus, see
  include/linux/io-64-nonatomic-{hi-lo,lo-hi}.h.
  I think we should just not go there for regmap unless we absolutely
  have to.

> > 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.
> 
> Yes, that makes sense.

Sounds good.

> > 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.
> 
> To my knowledge before I did a couple of days ago nobody ever ran i.MX6
> in big endian mode :)

At least nobody ever talked about it in public if they did, because they
would have to do the same patches you wrote.

There is still one open question about the defaults: I think we all agree
that there is no way we can change the default for compatible="syscon"
devices on ARM to from little-endian to cpu-endian, as that would break
everything. Annotating the MIPS dts files as "cpu-endian" and leaving the
rest to default to "little" is probably best here.

However, we have some freedom at the regmap-mmio level, which we can
sanitize in 4.6 if we want to make it more consistent with the rest
of regmap. We have around 50 callers of {devm_,}regmap_init_mmio()
and almost all of them do not specify endianess but expect little-endian
behavior. We can change all existing instances to set REGMAP_ENDIAN_NATIVE
explicitly and change regmap_init_mmio() to return an error if the
caller does not specify a particular endianess (big, little, native).
This is a tradeoff between interface simplicity (defaulting to LE is
convenient for most people) and consistency (all other regmap interfaces
default to native if I understood Mark right).

	Arnd

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

[Johannes Berg]

On Tue, 2016-01-26 at 10:09 +0100, Arnd Bergmann wrote:

> Again, it's complicated: 
> 
> * We should probably add ioread64be()/iowrite64be() on *64bit*
> architectures
>   for consistency with readq/writeq

Right.

> * On 32-bit architectures, you generally cannot do 64-bit atomic I/O
>   operations, and we have two implementations that do it
> nonatomically,
>   depending on how a device is wired to the bus, see
>   include/linux/io-64-nonatomic-{hi-lo,lo-hi}.h.
>   I think we should just not go there for regmap unless we absolutely
>   have to.

regmap-mmio doesn't define the 8-bit accesses for 32-bit platforms, so
this is a non-issue, I think.

> There is still one open question about the defaults: I think we all
> agree that there is no way we can change the default for
> compatible="syscon" devices on ARM to from little-endian to cpu-
> endian, as that would break everything. Annotating the MIPS dts files
> as "cpu-endian" and leaving the rest to default to "little" is
> probably best here.

Since regmap-mmio in practice was always little endian, we should
definitely make that consistent and explicit. Annotating those that
need special CPU-endian handling (MIPS with the byteswap engine) would
be best, I agree.

Note that I made a mistake here yesterday - the *reg* for MMIO is still
NATIVE, while the *value* is LITTLE_ENDIAN. Looks like regmap-core can
byteswap both, which makes sense for I2C and similar busses.

> However, we have some freedom at the regmap-mmio level, which we can
> sanitize in 4.6 if we want to make it more consistent with the rest
> of regmap. We have around 50 callers of {devm_,}regmap_init_mmio()
> and almost all of them do not specify endianess but expect little-
> endian behavior. We can change all existing instances to set
> REGMAP_ENDIAN_NATIVE explicitly and change regmap_init_mmio() to
> return an error if the caller does not specify a particular endianess
> (big, little, native). 

I'm not sure that we can, since regmap also takes the value from the DT
directly, and it seems that a driver passing it would mean the DT value
is no longer honoured?


johannes

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 915 bytes --]

On Tue, Jan 26, 2016 at 10:24:22AM +0100, Johannes Berg wrote:
> On Tue, 2016-01-26 at 10:09 +0100, Arnd Bergmann wrote:

> > However, we have some freedom at the regmap-mmio level, which we can
> > sanitize in 4.6 if we want to make it more consistent with the rest
> > of regmap. We have around 50 callers of {devm_,}regmap_init_mmio()
> > and almost all of them do not specify endianess but expect little-
> > endian behavior. We can change all existing instances to set
> > REGMAP_ENDIAN_NATIVE explicitly and change regmap_init_mmio() to
> > return an error if the caller does not specify a particular endianess
> > (big, little, native). 

> I'm not sure that we can, since regmap also takes the value from the DT
> directly, and it seems that a driver passing it would mean the DT value
> is no longer honoured?

DT should override the value in the driver rather than the other way
around.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

[Arnd Bergmann]

On Tuesday 26 January 2016 11:36:52 Mark Brown wrote:
> On Tue, Jan 26, 2016 at 10:24:22AM +0100, Johannes Berg wrote:
> > On Tue, 2016-01-26 at 10:09 +0100, Arnd Bergmann wrote:
> 
> > > However, we have some freedom at the regmap-mmio level, which we can
> > > sanitize in 4.6 if we want to make it more consistent with the rest
> > > of regmap. We have around 50 callers of {devm_,}regmap_init_mmio()
> > > and almost all of them do not specify endianess but expect little-
> > > endian behavior. We can change all existing instances to set
> > > REGMAP_ENDIAN_NATIVE explicitly and change regmap_init_mmio() to
> > > return an error if the caller does not specify a particular endianess
> > > (big, little, native). 
> 
> > I'm not sure that we can, since regmap also takes the value from the DT
> > directly, and it seems that a driver passing it would mean the DT value
> > is no longer honoured?
> 
> DT should override the value in the driver rather than the other way
> around.

So if we default to little-endian for all regmap-mmio mappings, and let
the driver or DT override that, and the regmap core already has the
logic to read the ordering from DT, should we remove the parsing of 
the byteorder attributes from syscon and just let regmap do its thing?

I was thinking about adding the patch below, but it sounds like we can
just remove the parsing completely.

	Arnd


diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
index b7aabeefab07..69d9bb84c94a 100644
--- a/drivers/mfd/syscon.c
+++ b/drivers/mfd/syscon.c
@@ -64,10 +64,13 @@ static struct syscon *of_syscon_register(struct device_node *np)
 		goto err_map;
 	}
 
-	/* Parse the device's DT node for an endianness specification */
+	/* Parse the device's DT node for an endianness specification,
+	   default to little-endian on all architectures */
 	if (of_property_read_bool(np, "big-endian"))
 		syscon_config.val_format_endian = REGMAP_ENDIAN_BIG;
-	 else if (of_property_read_bool(np, "little-endian"))
+	else if (of_property_read_bool(np, "native-endian"))
+		syscon_config.val_format_endian = REGMAP_ENDIAN_NATIVE;
+	else
 		syscon_config.val_format_endian = REGMAP_ENDIAN_LITTLE;
 
 	/*

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

[Johannes Berg]

> I was thinking about adding the patch below, but it sounds like we
> can just remove the parsing completely.

Yeah, that code looks almost identical in regmap.c.

Let me know if you want anything tested btw - it's pretty simple to
build and tftpboot a kernel now that I have the setup :)

johannes

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 242 bytes --]

On Tue, Jan 26, 2016 at 02:20:14PM +0100, Johannes Berg wrote:

> Let me know if you want anything tested btw - it's pretty simple to
> build and tftpboot a kernel now that I have the setup :)

I'm going to have some patches out later today.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 713 bytes --]

On Tue, Jan 26, 2016 at 03:23:12PM +0000, Mark Brown wrote:
> On Tue, Jan 26, 2016 at 02:20:14PM +0100, Johannes Berg wrote:
> 
> > Let me know if you want anything tested btw - it's pretty simple to
> > build and tftpboot a kernel now that I have the setup :)
> 
> I'm going to have some patches out later today.

No I won't, it'll be tomorrow before I get the actual fix done.  Trying
to work around this without just going and doing the conversion to
reg_read() and reg_write() is far too messy whichever way I try to do
it, I'd rather put that conversion in an -rc than try to dance around
the issue.  It may be safer to just go with the revert and live with
the broken DTs for another release.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 717 bytes --]

On Tue, Jan 26, 2016 at 02:16:16PM +0100, Arnd Bergmann wrote:

> So if we default to little-endian for all regmap-mmio mappings, and let
> the driver or DT override that, and the regmap core already has the
> logic to read the ordering from DT, should we remove the parsing of 
> the byteorder attributes from syscon and just let regmap do its thing?

> I was thinking about adding the patch below, but it sounds like we can
> just remove the parsing completely.

Probably, yes.  I've got a patch for that now, just trying to work out
what to do to resolve the readl/writel thing without breaking MIPS which
is harder than it sounds without doing a proper fix (which I obviously
want to avoid in -rc).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

[Arnd Bergmann]

On Tuesday 26 January 2016 10:24:22 Johannes Berg wrote:
> On Tue, 2016-01-26 at 10:09 +0100, Arnd Bergmann wrote:
> > * On 32-bit architectures, you generally cannot do 64-bit atomic I/O
> >   operations, and we have two implementations that do it
> > nonatomically,
> >   depending on how a device is wired to the bus, see
> >   include/linux/io-64-nonatomic-{hi-lo,lo-hi}.h.
> >   I think we should just not go there for regmap unless we absolutely
> >   have to.
> 
> regmap-mmio doesn't define the 8-bit accesses for 32-bit platforms, so
> this is a non-issue, I think.

Ok, good.

> > There is still one open question about the defaults: I think we all
> > agree that there is no way we can change the default for
> > compatible="syscon" devices on ARM to from little-endian to cpu-
> > endian, as that would break everything. Annotating the MIPS dts files
> > as "cpu-endian" and leaving the rest to default to "little" is
> > probably best here.
> 
> Since regmap-mmio in practice was always little endian, we should
> definitely make that consistent and explicit. Annotating those that
> need special CPU-endian handling (MIPS with the byteswap engine) would
> be best, I agree.
> 
> Note that I made a mistake here yesterday - the *reg* for MMIO is still
> NATIVE, while the *value* is LITTLE_ENDIAN. Looks like regmap-core can
> byteswap both, which makes sense for I2C and similar busses.

I see, that's something I completely missed, and it's logical that
the reg number must be native endian for MMIO but not for everything.

	Arnd

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1893 bytes --]

On Tue, Jan 26, 2016 at 10:09:35AM +0100, Arnd Bergmann wrote:

> * On 32-bit architectures, you generally cannot do 64-bit atomic I/O
>   operations, and we have two implementations that do it nonatomically,
>   depending on how a device is wired to the bus, see
>   include/linux/io-64-nonatomic-{hi-lo,lo-hi}.h.
>   I think we should just not go there for regmap unless we absolutely
>   have to.

That's fine, we don't support 64 bit MMIO regmaps on 32 bit systems
anyway (they're ifdefed out and will generate an error if you try to use
them).  If someone wants that functionality they can sort it out.

> However, we have some freedom at the regmap-mmio level, which we can
> sanitize in 4.6 if we want to make it more consistent with the rest
> of regmap. We have around 50 callers of {devm_,}regmap_init_mmio()
> and almost all of them do not specify endianess but expect little-endian
> behavior. We can change all existing instances to set REGMAP_ENDIAN_NATIVE
> explicitly and change regmap_init_mmio() to return an error if the
> caller does not specify a particular endianess (big, little, native).

As far as I can tell almost all the users actually want little endian
(everyone except these MIPS users) so if we're going to go through and
change all the users we should be going for that.  Given that things
just won't work if they have the wrong endianness I'm reasonably
comfortable with keeping a default in the core - so long as the core is
explicit about what it's doing so unlikely to get confused I think we
are reasonably safe.

> This is a tradeoff between interface simplicity (defaulting to LE is
> convenient for most people) and consistency (all other regmap interfaces
> default to native if I understood Mark right).

All other regmaps default to either native or big endian (native for
things with explicit read and write operations, big for byte stream
buses).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2543 bytes --]

On Mon, Jan 25, 2016 at 11:24:44PM +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

Please send patches to maintainers and please use subject lines
reflecting the style for the subsystem.

> > 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 I'd expect to be happening here is that either the driver or the
DT should be specifying the endianness of the hardware.  If the device
is always a given endianness then I'd expect that to turn up in the
driver rather than the DT.

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

The problem here is that regmap already has that functionality (it needs
it for non-MMIO buses anyway) and so it knows what's going on really
wants the I/O accessors to get out of the way.

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

I can't identify *anything* which says we shouldn't use the __raw
accessors in architecture neutral code with the possible exception of
the __.  Seriously, how is anyone supposed to use this stuff if we have
hidden assumptions like this?

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

Yeah, the native endian formatting is causing a lot of trouble.  The
MIPS folks really should also have come and talked to me rather than
writing such obvious nonsense in the DT in the first place.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

[Arnd Bergmann]

On Monday 25 January 2016 22:47:10 Mark Brown wrote:
> On Mon, Jan 25, 2016 at 11:24:44PM +0100, Arnd Bergmann wrote:
> > On Monday 25 January 2016 23:07:55 Johannes Berg wrote:
> > > 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 I'd expect to be happening here is that either the driver or the
> DT should be specifying the endianness of the hardware.  If the device
> is always a given endianness then I'd expect that to turn up in the
> driver rather than the DT.
> 
> > > 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.
> 
> The problem here is that regmap already has that functionality (it needs
> it for non-MMIO buses anyway) and so it knows what's going on really
> wants the I/O accessors to get out of the way.

I think we need to at least document the default behavior for
syscon (without changing behavior on ARM), and allow the DT to
specify cpu-endian as an override (or make MIPS default to that,
but that would be rather inconsistent and a pain to document
in the syscon binding).

> > 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.
> 
> I can't identify *anything* which says we shouldn't use the __raw
> accessors in architecture neutral code with the possible exception of
> the __.  Seriously, how is anyone supposed to use this stuff if we have
> hidden assumptions like this?

I thought everyone knew by now.

> > 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.
> 
> Yeah, the native endian formatting is causing a lot of trouble.  The
> MIPS folks really should also have come and talked to me rather than
> writing such obvious nonsense in the DT in the first place.

They should also have talked to me as the syscon maintainer...

	Arnd

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 474 bytes --]

On Tue, Jan 26, 2016 at 12:01:25AM +0100, Arnd Bergmann wrote:
> On Monday 25 January 2016 22:47:10 Mark Brown wrote:

> > I can't identify *anything* which says we shouldn't use the __raw
> > accessors in architecture neutral code with the possible exception of
> > the __.  Seriously, how is anyone supposed to use this stuff if we have
> > hidden assumptions like this?

> I thought everyone knew by now.

There's always the danger that we might get some new developers!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 207 bytes --]

On Mon, Jan 25, 2016 at 11:07:55PM +0100, Johannes Berg wrote:
> This reverts commit 29bb45f25ff3051354ed330c0d0f10418a2b8c7c.

Sorry, this just turned up now - must've got delayed somewhere along the
line.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]