[PATCH] arc: use little endian accesses

[PATCH] arc: use little endian accesses

[Lada Trimasova]

Memory access primitives should use cpu_to_le16, cpu_to_le32, le16_to_cpu
and le32_to_cpu because it is not really guaranteed that drivers handles
any ordering themselves.
For example, serial port driver doesn't work when kernel is build for
arc big endian architecture.

Signed-off-by: Lada Trimasova <ltrimas@synopsys.com>
Cc: Alexey Brodkin <abrodkin@synopsys.com>
Cc: Vineet Gupta <vgupta@synopsys.com>
---
 arch/arc/include/asm/io.h | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/arch/arc/include/asm/io.h b/arch/arc/include/asm/io.h
index 694ece8..0b3d5ea 100644
--- a/arch/arc/include/asm/io.h
+++ b/arch/arc/include/asm/io.h
@@ -129,15 +129,17 @@ static inline void __raw_writel(u32 w, volatile void __iomem *addr)
 #define writel(v,c)		({ __iowmb(); writel_relaxed(v,c); })
 
 /*
- * Relaxed API for drivers which can handle any ordering themselves
+ * This are defined to perform little endian accesses
  */
 #define readb_relaxed(c)	__raw_readb(c)
-#define readw_relaxed(c)	__raw_readw(c)
-#define readl_relaxed(c)	__raw_readl(c)
+#define readw_relaxed(c) ({ u16 __r = le16_to_cpu((__force __le16) \
+					__raw_readw(c)); __r; })
+#define readl_relaxed(c) ({ u32 __r = le32_to_cpu((__force __le32) \
+					__raw_readl(c)); __r; })
 
 #define writeb_relaxed(v,c)	__raw_writeb(v,c)
-#define writew_relaxed(v,c)	__raw_writew(v,c)
-#define writel_relaxed(v,c)	__raw_writel(v,c)
+#define writew_relaxed(v,c)	__raw_writew((__force u16) cpu_to_le16(v),c)
+#define writel_relaxed(v,c)	__raw_writel((__force u32) cpu_to_le32(v),c)
 
 #include <asm-generic/io.h>
 
-- 
2.5.0

Re: [PATCH] arc: use little endian accesses

[Vineet Gupta]

+CC Noam

On Wednesday 09 March 2016 10:51 PM, Lada Trimasova wrote:
> Memory access primitives should use cpu_to_le16, cpu_to_le32, le16_to_cpu
> and le32_to_cpu because it is not really guaranteed that drivers handles
> any ordering themselves.

That is the driver issue. readxx as API simply returns data in native endianness.
We've had EZChip running big endian and so far and they didn't need this change.

> For example, serial port driver doesn't work when kernel is build for
> arc big endian architecture.

Last I tested Big Endian on SDP with 8250 part + 8250 driver it was working fine.
I presume this is the systemC model for device and standard 8250 driver and very
likely the model is not fixed endian or something.

Alexey knows about this stuff - this was discussed on lkml back in 2013 when he
was fighting the Xilinx systemAce driver endian issues

> Signed-off-by: Lada Trimasova <ltrimas@synopsys.com>

Sorry NACK on this ! If you still think we need it I need more data / details on
what exactly is failing in 8250 and how !

-Vineet

> Cc: Alexey Brodkin <abrodkin@synopsys.com>
> Cc: Vineet Gupta <vgupta@synopsys.com>
> ---
>  arch/arc/include/asm/io.h | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arc/include/asm/io.h b/arch/arc/include/asm/io.h
> index 694ece8..0b3d5ea 100644
> --- a/arch/arc/include/asm/io.h
> +++ b/arch/arc/include/asm/io.h
> @@ -129,15 +129,17 @@ static inline void __raw_writel(u32 w, volatile void __iomem *addr)
>  #define writel(v,c)		({ __iowmb(); writel_relaxed(v,c); })
>  
>  /*
> - * Relaxed API for drivers which can handle any ordering themselves
> + * This are defined to perform little endian accesses
>   */
>  #define readb_relaxed(c)	__raw_readb(c)
> -#define readw_relaxed(c)	__raw_readw(c)
> -#define readl_relaxed(c)	__raw_readl(c)
> +#define readw_relaxed(c) ({ u16 __r = le16_to_cpu((__force __le16) \
> +					__raw_readw(c)); __r; })
> +#define readl_relaxed(c) ({ u32 __r = le32_to_cpu((__force __le32) \
> +					__raw_readl(c)); __r; })
>  
>  #define writeb_relaxed(v,c)	__raw_writeb(v,c)
> -#define writew_relaxed(v,c)	__raw_writew(v,c)
> -#define writel_relaxed(v,c)	__raw_writel(v,c)
> +#define writew_relaxed(v,c)	__raw_writew((__force u16) cpu_to_le16(v),c)
> +#define writel_relaxed(v,c)	__raw_writel((__force u32) cpu_to_le32(v),c)
>  
>  #include <asm-generic/io.h>
>  

Re: [PATCH] arc: use little endian accesses

[Vineet Gupta]

On Thursday 10 March 2016 10:35 AM, Vineet Gupta wrote:
> +CC Noam
>
> On Wednesday 09 March 2016 10:51 PM, Lada Trimasova wrote:
>> > Memory access primitives should use cpu_to_le16, cpu_to_le32, le16_to_cpu
>> > and le32_to_cpu because it is not really guaranteed that drivers handles
>> > any ordering themselves.
> That is the driver issue. readxx as API simply returns data in native endianness.
> We've had EZChip running big endian and so far and they didn't need this change.
>
>> > For example, serial port driver doesn't work when kernel is build for
>> > arc big endian architecture.
> Last I tested Big Endian on SDP with 8250 part + 8250 driver it was working fine.
> I presume this is the systemC model for device and standard 8250 driver and very
> likely the model is not fixed endian or something.
>
> Alexey knows about this stuff - this was discussed on lkml back in 2013 when he
> was fighting the Xilinx systemAce driver endian issues
>

Do you need "native-endian" DT entry in nsimosci DT bindings for uart ?

Re: [PATCH] arc: use little endian accesses

[Alexey Brodkin]

Hi Vineet,

On Thu, 2016-03-10 at 05:05 +0000, Vineet Gupta wrote:
> +CC Noam
> 
> On Wednesday 09 March 2016 10:51 PM, Lada Trimasova wrote:
> > 
> > Memory access primitives should use cpu_to_le16, cpu_to_le32, le16_to_cpu
> > and le32_to_cpu because it is not really guaranteed that drivers handles
> > any ordering themselves.
> That is the driver issue. readxx as API simply returns data in native endianness.
> We've had EZChip running big endian and so far and they didn't need this change.

Let me disagree with you here.
See what is said in "include/asm-generic/io.h":
---------------------->8---------------------
/*
 * __raw_{read,write}{b,w,l,q}() access memory in native endianness.
 *
 * On some architectures memory mapped IO needs to be accessed differently.
 * On the simple architectures, we just read/write the memory location
 * directly.
 */

...

/*
 * {read,write}{b,w,l,q}() access little endian memory and return result in
 * native endianness.
 */
---------------------->8---------------------

And that's an implementation we have for ARC:
---------------------->8---------------------
#define readb(c)		({ u8  __v = readb_relaxed(c); __iormb(); __v; })
#define readw(c)		({ u16 __v = readw_relaxed(c); __iormb(); __v; })
#define readl(c)		({ u32 __v = readl_relaxed(c); __iormb(); __v; })

#define writeb(v,c)		({ __iowmb(); writeb_relaxed(v,c); })
#define writew(v,c)		({ __iowmb(); writew_relaxed(v,c); })
#define writel(v,c)		({ __iowmb(); writel_relaxed(v,c); })

/*
 * Relaxed API for drivers which can handle any ordering themselves
 */
#define readb_relaxed(c)	__raw_readb(c)
#define readw_relaxed(c)	__raw_readw(c)
#define readl_relaxed(c)	__raw_readl(c)

#define writeb_relaxed(v,c)	__raw_writeb(v,c)
#define writew_relaxed(v,c)	__raw_writew(v,c)
#define writel_relaxed(v,c)	__raw_writel(v,c)
---------------------->8---------------------

Which is effectively (related to endianess discussion):
---------------------->8---------------------
#define readX(c)	__raw_readX(c)
#define writeX(v,c)	__raw_writeX(v,c)
---------------------->8---------------------

That looks IMHO incorrect if we read API description in "include/asm-generic/io.h".
BTW description of {read,write}{b,w,l,q}() is a bit misleading in part saying
"... and return result in __native_endianness__".

But real implementation of {read,write}{b,w,l,q}() in "include/asm-generic/io.h"
really shows what was meant - note __leXX_to_cpu() and cpu_to_leXX are used.

> > 
> > For example, serial port driver doesn't work when kernel is build for
> > arc big endian architecture.
> Last I tested Big Endian on SDP with 8250 part + 8250 driver it was working fine.
> I presume this is the systemC model for device and standard 8250 driver and very
> likely the model is not fixed endian or something.

Model is indeed little-endian. We build it only once and than changing
only "nsim_isa_big_endian" property (which changes only CPU core endianess) may use
it equally well with little- and big-endian builds of Linux kernel.

-Alexey

Re: [PATCH] arc: use little endian accesses

[Vineet Gupta]

On Thursday 10 March 2016 01:14 PM, Alexey Brodkin wrote:
>>> For example, serial port driver doesn't work when kernel is build for
>>> arc big endian architecture.
>> Last I tested Big Endian on SDP with 8250 part + 8250 driver it was working fine.
>> I presume this is the systemC model for device and standard 8250 driver and very
>> likely the model is not fixed endian or something.
> Model is indeed little-endian. We build it only once and than changing
> only "nsim_isa_big_endian" property (which changes only CPU core endianess) may use
> it equally well with little- and big-endian builds of Linux kernel.

Can you or Lada provide more details as to exactly what is not working - what
driver to be precise 8250 or dw-8250.
And where exactly the failure shows up. I want to understand this more
Noam told me off list that he has no issues with both big endian ARC + 8250 in
systemc model or silicon.

-Vineet

Re: [PATCH] arc: use little endian accesses

[Lada Trimasova]

Hi Vineet, Alexey, Arnd,
On Thu, 2016-03-10 at 09:55 +0000, Vineet Gupta wrote:

Can you or Lada provide more details as to exactly what is not working - what
driver to be precise 8250 or dw-8250.
And where exactly the failure shows up. I want to understand this more
Noam told me off list that he has no issues with both big endian ARC + 8250 in
systemc model or silicon.

-Vineet



Driver is 8250, kernel is built for BE arc, nsim option in model "nsim_isa_big_endian = 1".

With current "readl" and "writel" implementation for ARC we read word from memory without any endianess manipulation. So in case of little endian architecture we get what we want: the first memory byte is the low byte in the word. But in case of big endian architecture the word endianess is swapped: the first memory byte is the high word byte.

And for example, let's see what happens when we use "readl" in  function "serial8250_early_in" in driver/tty/serial/8250.

Take a look to one line from memory dump:
0xf0000010: 0x0b    0x00    0x00    0x00    0x60    0x00    0x00    0x00

When kernel is built for little endian architecture, we read this data in status register in function "serial_putc" using "readl" function in driver/tty/serial/8250 as:
r0: 0x0000006
The low byte is 0x0b, so the condition "if ((status & BOTH_EMPTY) == BOTH_EMPTY)"  is true, as BOTH_EMPTY is some mask with low bytes set.

When kernel is built with "CPU_BIG_ENDIAN" and model nsim option is "nsim_isa_big_endian=1", we read this data as:
r0: 0x6000000
So as you can see the low byte is 0x00 and above mentioned condition never becomes true, we can't continue initialization.


Regards,
Lada

Re: [PATCH] arc: use little endian accesses

[Arnd Bergmann]

On Thursday 10 March 2016, Lada Trimasova wrote:
> Driver is 8250, kernel is built for BE arc, nsim option in model "nsim_isa_big_endian = 1".
> 
> With current "readl" and "writel" implementation for ARC we read word from memory without any endianess manipulation. So in case of little endian architecture we get what we want: the first memory byte is the low byte in the word. But in case of big endian architecture the word endianess is swapped: the first memory byte is the high word byte.
> 
> And for example, let's see what happens when we use "readl" in  function "serial8250_early_in" in driver/tty/serial/8250.
> 
> Take a look to one line from memory dump:
> 0xf0000010: 0x0b    0x00    0x00    0x00    0x60    0x00    0x00    0x00
> 
> When kernel is built for little endian architecture, we read this data in status register in function "serial_putc" using "readl" function in driver/tty/serial/8250 as:
> r0: 0x0000006
> The low byte is 0x0b, so the condition "if ((status & BOTH_EMPTY) == BOTH_EMPTY)"  is true, as BOTH_EMPTY is some mask with low bytes set.
> 
> When kernel is built with "CPU_BIG_ENDIAN" and model nsim option is "nsim_isa_big_endian=1", we read this data as:
> r0: 0x6000000
> So as you can see the low byte is 0x00 and above mentioned condition never becomes true, we can't continue initialization.
> 

Ok, this sounds like a completely normal architecture implementation then,
and your patch looks correct.

If some other driver breaks because of this change, you should investigate
what went wrong there, and treat it as a driver specific problem.

	Arnd

Re: [PATCH] arc: use little endian accesses

[Vineet Gupta]

On Friday 11 March 2016 12:54 AM, Arnd Bergmann wrote:
> On Thursday 10 March 2016, Lada Trimasova wrote:
>> Driver is 8250, kernel is built for BE arc, nsim option in model "nsim_isa_big_endian = 1".
>>
>> With current "readl" and "writel" implementation for ARC we read word from memory without any endianess manipulation. So in case of little endian architecture we get what we want: the first memory byte is the low byte in the word. But in case of big endian architecture the word endianess is swapped: the first memory byte is the high word byte.
>>
>> And for example, let's see what happens when we use "readl" in  function "serial8250_early_in" in driver/tty/serial/8250.
>>
>> Take a look to one line from memory dump:
>> 0xf0000010: 0x0b    0x00    0x00    0x00    0x60    0x00    0x00    0x00
>>
>> When kernel is built for little endian architecture, we read this data in status register in function "serial_putc" using "readl" function in driver/tty/serial/8250 as:
>> r0: 0x0000006
>> The low byte is 0x0b, so the condition "if ((status & BOTH_EMPTY) == BOTH_EMPTY)"  is true, as BOTH_EMPTY is some mask with low bytes set.
>>
>> When kernel is built with "CPU_BIG_ENDIAN" and model nsim option is "nsim_isa_big_endian=1", we read this data as:
>> r0: 0x6000000
>> So as you can see the low byte is 0x00 and above mentioned condition never becomes true, we can't continue initialization.
>>
> Ok, this sounds like a completely normal architecture implementation then,
> and your patch looks correct.
>
> If some other driver breaks because of this change, you should investigate
> what went wrong there, and treat it as a driver specific problem.

Although I believe Arnd the most, I was not convinced abt this (see below why) so
did a bit of investigation and looks like this patch is indeed correct and Arnd as
always is right :-)

The current upstream kernel doesn't boot on BE configured AXS103 is stuck
similarly in uart. With this patch it comes up fine. My trouble was I'd seen BE
work (w/o this patch) in our internal 3.18 branch. Turns out that in 3.18, ARC
io.h used the asm-generic version of readX(). My patch b8a033023994 ("ARCv2:
barriers") to upstream introduced the io barriers for ARCHS and in the process
introduced our own readX() accessors, which were not the same as correct
asm-generic ver. Thus I inadvertently introduced this bug.

@Lada I will fix up the changelog to add some of the background behind this
change, and mark this for stable backport as well.

Thx,
-Vineet

>
> 	Arnd
>

Re: [PATCH] arc: use little endian accesses

[Vineet Gupta]

On Friday 11 March 2016 06:14 PM, Vineet Gupta wrote:
> @Lada I will fix up the changelog to add some of the background behind this
> change, and mark this for stable backport as well.

This is what I'm planning to add.
Noam please give this a spin - you might have to revert those native-endian DT
bindings from UART DT.

----->
>From f778cc65717687a3d3f26dd21bef62cd059f1b8b Mon Sep 17 00:00:00 2001
From: Lada Trimasova <ltrimas@synopsys.com>
Date: Wed, 9 Mar 2016 20:21:04 +0300
Subject: [PATCH] ARC: [BE] readl()/writel() to work in Big Endian CPU
 configuration

read{l,w}() write{l,w}() primitives should use le{16,32}_to_cpu() and
cpu_to_le{16,32}() respectively to ensure device registers are read
correctly in Big Endian CPU configuration.

Per Arnd Bergmann
| Most drivers using readl() or readl_relaxed() expect those to perform byte
| swaps on big-endian architectures, as the registers tend to be fixed endian

This was needed for getting UART to work correctly on a Big Endian ARC.

The ARC accessors originally were fine, and the bug got introduced
inadventently by commit b8a033023994 ("ARCv2: barriers")

Fixes: b8a033023994 ("ARCv2: barriers")
Link: http://lkml.kernel.org/r/201603100845.30602.arnd@arndb.de
Cc: Alexey Brodkin <abrodkin@synopsys.com>
Cc: stable@vger.kernel.org  [4.2+]
Cc: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Lada Trimasova <ltrimas@synopsys.com>
[vgupta: beefed up changelog, added Fixes/stable tags]
Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
---
 arch/arc/include/asm/io.h | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/arch/arc/include/asm/io.h b/arch/arc/include/asm/io.h
index 694ece8a0243..27b17adea50d 100644
--- a/arch/arc/include/asm/io.h
+++ b/arch/arc/include/asm/io.h
@@ -129,15 +129,23 @@ static inline void __raw_writel(u32 w, volatile void __iomem
*addr)
 #define writel(v,c)		({ __iowmb(); writel_relaxed(v,c); })

 /*
- * Relaxed API for drivers which can handle any ordering themselves
+ * Relaxed API for drivers which can handle barrier ordering themselves
+ *
+ * Also these are defined to perform little endian accesses.
+ * To provide the typical device register semantics of fixed endian,
+ * swap the byte order for Big Endian
+ *
+ * http://lkml.kernel.org/r/201603100845.30602.arnd@arndb.de
  */
 #define readb_relaxed(c)	__raw_readb(c)
-#define readw_relaxed(c)	__raw_readw(c)
-#define readl_relaxed(c)	__raw_readl(c)
+#define readw_relaxed(c) ({ u16 __r = le16_to_cpu((__force __le16) \
+					__raw_readw(c)); __r; })
+#define readl_relaxed(c) ({ u32 __r = le32_to_cpu((__force __le32) \
+					__raw_readl(c)); __r; })

 #define writeb_relaxed(v,c)	__raw_writeb(v,c)
-#define writew_relaxed(v,c)	__raw_writew(v,c)
-#define writel_relaxed(v,c)	__raw_writel(v,c)
+#define writew_relaxed(v,c)	__raw_writew((__force u16) cpu_to_le16(v),c)
+#define writel_relaxed(v,c)	__raw_writel((__force u32) cpu_to_le32(v),c)

 #include <asm-generic/io.h>

-- 
2.5.0

Re: [PATCH] arc: use little endian accesses

[Noam Camus]

> From: Lada Trimasova <Lada.Trimasova@synopsys.com>
> Sent: Thursday, March 10, 2016 8:57 PM

> And for example, let's see what happens when we use "readl" in  function "serial8250_early_in" in driver/tty/serial/8250.

Is your DTS entry includes for serial node following entries:
reg-io-width = <4>
native-endian;

if answer is yes you should expect (in case of big endian compilation) that:
port->iotype would be equal to UPIO_MEM32BE, hence no readl() use for this case.

This is how it works for me here, it sounds like your case is similar.

Regards,
Noam

Re: [PATCH] arc: use little endian accesses

[Arnd Bergmann]

On Thursday 10 March 2016, Vineet Gupta wrote:
> On Wednesday 09 March 2016 10:51 PM, Lada Trimasova wrote:
> > Memory access primitives should use cpu_to_le16, cpu_to_le32, le16_to_cpu
> > and le32_to_cpu because it is not really guaranteed that drivers handles
> > any ordering themselves.
> 
> That is the driver issue. readxx as API simply returns data in native endianness.
> We've had EZChip running big endian and so far and they didn't need this change.

Most drivers using readl() or readl_relaxed() expect those to perform byte
swaps on big-endian architectures, as the registers tend to be fixed endian,
so the change seems reasonable.

It depends a little bit on how endian mode is implemented in the CPU: do you
follow the normal model of swapping byte order in the load/store instructions
of the CPU when running big-endian, or is the CPU always running little-endian
but the bus addresses get mangled on byte accesses to give the illusion
of a big-endian system?

	Arnd

Re: [PATCH] arc: use little endian accesses

[Vineet Gupta]

On Thursday 10 March 2016 01:15 PM, Arnd Bergmann wrote:
> On Thursday 10 March 2016, Vineet Gupta wrote:
>> On Wednesday 09 March 2016 10:51 PM, Lada Trimasova wrote:
>>> Memory access primitives should use cpu_to_le16, cpu_to_le32, le16_to_cpu
>>> and le32_to_cpu because it is not really guaranteed that drivers handles
>>> any ordering themselves.
>> That is the driver issue. readxx as API simply returns data in native endianness.
>> We've had EZChip running big endian and so far and they didn't need this change.
> Most drivers using readl() or readl_relaxed() expect those to perform byte
> swaps on big-endian architectures, as the registers tend to be fixed endian,
> so the change seems reasonable.
>
> It depends a little bit on how endian mode is implemented in the CPU: do you
> follow the normal model of swapping byte order in the load/store instructions
> of the CPU when running big-endian, or is the CPU always running little-endian
> but the bus addresses get mangled on byte accesses to give the illusion
> of a big-endian system?

OK I got the response from hardware guys that we follow the normal mode of
swapping byte order for big-endian mode. Arnd can u please explain how that might
impact the io accessors, it at all.

And what exactly are semantics of readX() and ioreadX() - even if arch specific
and I'd be glad to change that for ARC.

I can also help with documenting them properly some where as went digging into
mailing list first thing Lada posted this patch :-)

Thx,
-Vineet