linux-arch.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] arc: use little endian accesses
@ 2016-03-09 17:21 Lada Trimasova
  2016-03-10  5:05 ` Vineet Gupta
  0 siblings, 1 reply; 17+ messages in thread
From: Lada Trimasova @ 2016-03-09 17:21 UTC (permalink / raw)
  To: linux-snps-arc
  Cc: linux-kernel, linux-arch, Lada Trimasova, Alexey Brodkin, Vineet Gupta

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

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH] arc: use little endian accesses
  2016-03-09 17:21 [PATCH] arc: use little endian accesses Lada Trimasova
@ 2016-03-10  5:05 ` Vineet Gupta
  2016-03-10  5:19   ` Vineet Gupta
                     ` (2 more replies)
  0 siblings, 3 replies; 17+ messages in thread
From: Vineet Gupta @ 2016-03-10  5:05 UTC (permalink / raw)
  To: Lada Trimasova, linux-snps-arc
  Cc: linux-kernel, linux-arch, Alexey Brodkin, Noam Camus

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] arc: use little endian accesses
  2016-03-10  5:05 ` Vineet Gupta
@ 2016-03-10  5:19   ` Vineet Gupta
  2016-03-10  5:19     ` Vineet Gupta
  2016-03-10  7:44   ` Alexey Brodkin
  2016-03-10  7:45   ` Arnd Bergmann
  2 siblings, 1 reply; 17+ messages in thread
From: Vineet Gupta @ 2016-03-10  5:19 UTC (permalink / raw)
  To: Lada Trimasova, linux-snps-arc
  Cc: linux-kernel, linux-arch, Alexey Brodkin, Noam Camus

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 ?

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] arc: use little endian accesses
  2016-03-10  5:19   ` Vineet Gupta
@ 2016-03-10  5:19     ` Vineet Gupta
  0 siblings, 0 replies; 17+ messages in thread
From: Vineet Gupta @ 2016-03-10  5:19 UTC (permalink / raw)
  To: Lada Trimasova, linux-snps-arc
  Cc: linux-kernel, linux-arch, Alexey Brodkin, Noam Camus

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 ?

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] arc: use little endian accesses
  2016-03-10  5:05 ` Vineet Gupta
  2016-03-10  5:19   ` Vineet Gupta
@ 2016-03-10  7:44   ` Alexey Brodkin
  2016-03-10  9:55     ` Vineet Gupta
  2016-03-10  7:45   ` Arnd Bergmann
  2 siblings, 1 reply; 17+ messages in thread
From: Alexey Brodkin @ 2016-03-10  7:44 UTC (permalink / raw)
  To: Vineet Gupta
  Cc: linux-kernel, linux-arch, noamc, Lada Trimasova, linux-snps-arc

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] arc: use little endian accesses
  2016-03-10  5:05 ` Vineet Gupta
  2016-03-10  5:19   ` Vineet Gupta
  2016-03-10  7:44   ` Alexey Brodkin
@ 2016-03-10  7:45   ` Arnd Bergmann
  2016-03-10  7:45     ` Arnd Bergmann
  2016-03-11  5:18     ` Vineet Gupta
  2 siblings, 2 replies; 17+ messages in thread
From: Arnd Bergmann @ 2016-03-10  7:45 UTC (permalink / raw)
  To: Vineet Gupta
  Cc: linux-arch, Lada Trimasova, Alexey Brodkin, linux-kernel,
	Noam Camus, linux-snps-arc

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] arc: use little endian accesses
  2016-03-10  7:45   ` Arnd Bergmann
@ 2016-03-10  7:45     ` Arnd Bergmann
  2016-03-11  5:18     ` Vineet Gupta
  1 sibling, 0 replies; 17+ messages in thread
From: Arnd Bergmann @ 2016-03-10  7:45 UTC (permalink / raw)
  To: Vineet Gupta
  Cc: Lada Trimasova, linux-snps-arc, linux-kernel, linux-arch,
	Alexey Brodkin, Noam Camus

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] arc: use little endian accesses
  2016-03-10  7:44   ` Alexey Brodkin
@ 2016-03-10  9:55     ` Vineet Gupta
  2016-03-10 18:57       ` Lada Trimasova
  0 siblings, 1 reply; 17+ messages in thread
From: Vineet Gupta @ 2016-03-10  9:55 UTC (permalink / raw)
  To: Alexey Brodkin
  Cc: linux-kernel, linux-arch, noamc, Lada Trimasova, linux-snps-arc

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] arc: use little endian accesses
  2016-03-10  9:55     ` Vineet Gupta
@ 2016-03-10 18:57       ` Lada Trimasova
  2016-03-10 18:57         ` Lada Trimasova
                           ` (2 more replies)
  0 siblings, 3 replies; 17+ messages in thread
From: Lada Trimasova @ 2016-03-10 18:57 UTC (permalink / raw)
  To: Vineet Gupta
  Cc: linux-kernel, linux-arch, noamc, Alexey Brodkin, linux-snps-arc

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] arc: use little endian accesses
  2016-03-10 18:57       ` Lada Trimasova
@ 2016-03-10 18:57         ` Lada Trimasova
  2016-03-10 19:23         ` Arnd Bergmann
  2016-03-11  5:07         ` Noam Camus
  2 siblings, 0 replies; 17+ messages in thread
From: Lada Trimasova @ 2016-03-10 18:57 UTC (permalink / raw)
  To: Vineet Gupta
  Cc: linux-kernel, linux-arch, noamc, Alexey Brodkin, linux-snps-arc

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] arc: use little endian accesses
  2016-03-10 18:57       ` Lada Trimasova
  2016-03-10 18:57         ` Lada Trimasova
@ 2016-03-10 19:23         ` Arnd Bergmann
  2016-03-11 12:44           ` Vineet Gupta
  2016-03-11  5:07         ` Noam Camus
  2 siblings, 1 reply; 17+ messages in thread
From: Arnd Bergmann @ 2016-03-10 19:23 UTC (permalink / raw)
  To: Lada Trimasova
  Cc: Vineet Gupta, linux-kernel, linux-arch, noamc, Alexey Brodkin,
	linux-snps-arc

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] arc: use little endian accesses
  2016-03-10 18:57       ` Lada Trimasova
  2016-03-10 18:57         ` Lada Trimasova
  2016-03-10 19:23         ` Arnd Bergmann
@ 2016-03-11  5:07         ` Noam Camus
  2016-03-11  5:07           ` Noam Camus
  2 siblings, 1 reply; 17+ messages in thread
From: Noam Camus @ 2016-03-11  5:07 UTC (permalink / raw)
  To: Vineet Gupta, Lada Trimasova
  Cc: linux-arch, linux-snps-arc, Alexey Brodkin, linux-kernel

> 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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] arc: use little endian accesses
  2016-03-11  5:07         ` Noam Camus
@ 2016-03-11  5:07           ` Noam Camus
  0 siblings, 0 replies; 17+ messages in thread
From: Noam Camus @ 2016-03-11  5:07 UTC (permalink / raw)
  To: Vineet Gupta, Lada Trimasova
  Cc: linux-kernel, linux-arch, Alexey Brodkin, linux-snps-arc

> 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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] arc: use little endian accesses
  2016-03-10  7:45   ` Arnd Bergmann
  2016-03-10  7:45     ` Arnd Bergmann
@ 2016-03-11  5:18     ` Vineet Gupta
  1 sibling, 0 replies; 17+ messages in thread
From: Vineet Gupta @ 2016-03-11  5:18 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Lada Trimasova, linux-snps-arc, linux-kernel, linux-arch,
	Alexey Brodkin, Noam Camus

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] arc: use little endian accesses
  2016-03-10 19:23         ` Arnd Bergmann
@ 2016-03-11 12:44           ` Vineet Gupta
  2016-03-12  4:20             ` Vineet Gupta
  0 siblings, 1 reply; 17+ messages in thread
From: Vineet Gupta @ 2016-03-11 12:44 UTC (permalink / raw)
  To: Arnd Bergmann, Lada Trimasova
  Cc: linux-kernel, linux-arch, noamc, Alexey Brodkin, linux-snps-arc

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
>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] arc: use little endian accesses
  2016-03-11 12:44           ` Vineet Gupta
@ 2016-03-12  4:20             ` Vineet Gupta
  2016-03-12  4:20               ` Vineet Gupta
  0 siblings, 1 reply; 17+ messages in thread
From: Vineet Gupta @ 2016-03-12  4:20 UTC (permalink / raw)
  To: Arnd Bergmann, Lada Trimasova, noamc
  Cc: linux-kernel, linux-arch, Alexey Brodkin, linux-snps-arc

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

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH] arc: use little endian accesses
  2016-03-12  4:20             ` Vineet Gupta
@ 2016-03-12  4:20               ` Vineet Gupta
  0 siblings, 0 replies; 17+ messages in thread
From: Vineet Gupta @ 2016-03-12  4:20 UTC (permalink / raw)
  To: Arnd Bergmann, Lada Trimasova, noamc
  Cc: linux-kernel, linux-arch, Alexey Brodkin, linux-snps-arc

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.

----->

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2016-03-12  4:20 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-09 17:21 [PATCH] arc: use little endian accesses Lada Trimasova
2016-03-10  5:05 ` Vineet Gupta
2016-03-10  5:19   ` Vineet Gupta
2016-03-10  5:19     ` Vineet Gupta
2016-03-10  7:44   ` Alexey Brodkin
2016-03-10  9:55     ` Vineet Gupta
2016-03-10 18:57       ` Lada Trimasova
2016-03-10 18:57         ` Lada Trimasova
2016-03-10 19:23         ` Arnd Bergmann
2016-03-11 12:44           ` Vineet Gupta
2016-03-12  4:20             ` Vineet Gupta
2016-03-12  4:20               ` Vineet Gupta
2016-03-11  5:07         ` Noam Camus
2016-03-11  5:07           ` Noam Camus
2016-03-10  7:45   ` Arnd Bergmann
2016-03-10  7:45     ` Arnd Bergmann
2016-03-11  5:18     ` Vineet Gupta

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).