[RESEND PATCH 0/4] Fix endianness of generic I/O accessors

[RESEND PATCH 0/4] Fix endianness of generic I/O accessors

[Will Deacon]

Hello,

This is a resend of the patches I originally sent a while back:

  http://marc.info/?l=linux-kernel&m=133552356617938&w=2

There was some loose conclusion then that documentation needed to be
updated, which I didn't get round to doing. However, after discussion
with Ben at LPC (following his `Big and Little Endian inside/out'
presentation), he encouraged me to resend the patches anyway.

So here they are. I also included two extra patches to fix the MMC and
ethernet drivers used on the ARM64 model, which is where I came across
this issue in the first place.

Comments welcome,

Will

Big endian is not dead -- it just smells funny.


Will Deacon (4):
  asm-generic: io: remove {read,write} string functions
  asm-generic: io: don't perform swab during {in,out} string functions
  mmc: mmci: use io{read,write}*_rep accessors instead of string
    functions
  net: smc91x: use io{read,write}*_rep accessors instead of string
    functions

 drivers/mmc/host/mmci.c            |    8 +++---
 drivers/net/ethernet/smsc/smc91x.h |   20 ++++++++--------
 include/asm-generic/io.h           |   42 +++++------------------------------
 3 files changed, 20 insertions(+), 50 deletions(-)

-- 
1.7.4.1

[RESEND PATCH 1/4] asm-generic: io: remove {read,write} string functions

[Will Deacon]

The {read,write}s{b,w,l} functions are not defined across all
architectures and therefore shouldn't be used by portable drivers. We
should encourage driver writers to use the io{read,write}{8,16,32}_rep
functions instead.

This patch removes the {read,write} string functions for the generic IO
header as they have no place in a new architecture port.

Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Mike Frysinger <vapier@gentoo.org>
Cc: Ben Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 include/asm-generic/io.h |   30 ------------------------------
 1 files changed, 0 insertions(+), 30 deletions(-)

diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
index 448303b..3607921 100644
--- a/include/asm-generic/io.h
+++ b/include/asm-generic/io.h
@@ -217,36 +217,6 @@ static inline void outsl(unsigned long addr, const void *buffer, int count)
 }
 #endif
 
-static inline void readsl(const void __iomem *addr, void *buf, int len)
-{
-	insl(addr - PCI_IOBASE, buf, len);
-}
-
-static inline void readsw(const void __iomem *addr, void *buf, int len)
-{
-	insw(addr - PCI_IOBASE, buf, len);
-}
-
-static inline void readsb(const void __iomem *addr, void *buf, int len)
-{
-	insb(addr - PCI_IOBASE, buf, len);
-}
-
-static inline void writesl(const void __iomem *addr, const void *buf, int len)
-{
-	outsl(addr - PCI_IOBASE, buf, len);
-}
-
-static inline void writesw(const void __iomem *addr, const void *buf, int len)
-{
-	outsw(addr - PCI_IOBASE, buf, len);
-}
-
-static inline void writesb(const void __iomem *addr, const void *buf, int len)
-{
-	outsb(addr - PCI_IOBASE, buf, len);
-}
-
 #ifndef CONFIG_GENERIC_IOMAP
 #define ioread8(addr)		readb(addr)
 #define ioread16(addr)		readw(addr)
-- 
1.7.4.1

[RESEND PATCH 2/4] asm-generic: io: don't perform swab during {in,out} string functions

[Will Deacon]

The {in,out}s{b,w,l} functions are designed to operate on a stream of
bytes and therefore should not perform any byte-swapping, regardless of
the CPU byte order.

This patch fixes the generic IO header so that {in,out}s{b,w,l} call
the __raw_{read,write} functions directly rather than going via the
endian-correcting accessors.

Cc: Mike Frysinger <vapier@gentoo.org>
Cc: Ben Herrenschmidt <benh@kernel.crashing.org>
Acked-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 include/asm-generic/io.h |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
index 3607921..403b861 100644
--- a/include/asm-generic/io.h
+++ b/include/asm-generic/io.h
@@ -148,7 +148,7 @@ static inline void insb(unsigned long addr, void *buffer, int count)
 	if (count) {
 		u8 *buf = buffer;
 		do {
-			u8 x = inb(addr);
+			u8 x = __raw_readb(addr + PCI_IOBASE);
 			*buf++ = x;
 		} while (--count);
 	}
@@ -161,7 +161,7 @@ static inline void insw(unsigned long addr, void *buffer, int count)
 	if (count) {
 		u16 *buf = buffer;
 		do {
-			u16 x = inw(addr);
+			u16 x = __raw_readw(addr + PCI_IOBASE);
 			*buf++ = x;
 		} while (--count);
 	}
@@ -174,7 +174,7 @@ static inline void insl(unsigned long addr, void *buffer, int count)
 	if (count) {
 		u32 *buf = buffer;
 		do {
-			u32 x = inl(addr);
+			u32 x = __raw_readl(addr + PCI_IOBASE);
 			*buf++ = x;
 		} while (--count);
 	}
@@ -187,7 +187,7 @@ static inline void outsb(unsigned long addr, const void *buffer, int count)
 	if (count) {
 		const u8 *buf = buffer;
 		do {
-			outb(*buf++, addr);
+			__raw_writeb(*buf++, addr + PCI_IOBASE);
 		} while (--count);
 	}
 }
@@ -199,7 +199,7 @@ static inline void outsw(unsigned long addr, const void *buffer, int count)
 	if (count) {
 		const u16 *buf = buffer;
 		do {
-			outw(*buf++, addr);
+			__raw_writew(*buf++, addr + PCI_IOBASE);
 		} while (--count);
 	}
 }
@@ -211,7 +211,7 @@ static inline void outsl(unsigned long addr, const void *buffer, int count)
 	if (count) {
 		const u32 *buf = buffer;
 		do {
-			outl(*buf++, addr);
+			__raw_writel(*buf++, addr + PCI_IOBASE);
 		} while (--count);
 	}
 }
-- 
1.7.4.1

[RESEND PATCH 3/4] mmc: mmci: use io{read,write}*_rep accessors instead of string functions

[Will Deacon]

The {read,write}s{b,w,l} operations are not defined by all architectures
and are being removed from the asm-generic/io.h interface.

This patch replaces the usage of these string functions with
io{read,write}{8,16,32}_rep calls instead, which are defined for all
architectures.

Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Ben Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 drivers/mmc/host/mmci.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index edc3e9b..4f125b4 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -840,14 +840,14 @@ static int mmci_pio_read(struct mmci_host *host, char *buffer, unsigned int rema
 		if (unlikely(count & 0x3)) {
 			if (count < 4) {
 				unsigned char buf[4];
-				readsl(base + MMCIFIFO, buf, 1);
+				ioread32_rep(base + MMCIFIFO, buf, 1);
 				memcpy(ptr, buf, count);
 			} else {
-				readsl(base + MMCIFIFO, ptr, count >> 2);
+				ioread32_rep(base + MMCIFIFO, ptr, count >> 2);
 				count &= ~0x3;
 			}
 		} else {
-			readsl(base + MMCIFIFO, ptr, count >> 2);
+			ioread32_rep(base + MMCIFIFO, ptr, count >> 2);
 		}
 
 		ptr += count;
@@ -900,7 +900,7 @@ static int mmci_pio_write(struct mmci_host *host, char *buffer, unsigned int rem
 		 * byte become a 32bit write, 7 bytes will be two
 		 * 32bit writes etc.
 		 */
-		writesl(base + MMCIFIFO, ptr, (count + 3) >> 2);
+		iowrite32_rep(base + MMCIFIFO, ptr, (count + 3) >> 2);
 
 		ptr += count;
 		remain -= count;
-- 
1.7.4.1

[RESEND PATCH 4/4] net: smc91x: use io{read,write}*_rep accessors instead of string functions

[Will Deacon]

The {read,write}s{b,w,l} operations are not defined by all architectures
and are being removed from the asm-generic/io.h interface.

This patch replaces the usage of these string functions in the default
SMC accessors with io{read,write}{8,16,32}_rep calls instead, which are
defined for all architectures.

Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Ben Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 drivers/net/ethernet/smsc/smc91x.h |   20 ++++++++++----------
 1 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/smsc/smc91x.h b/drivers/net/ethernet/smsc/smc91x.h
index 5f53fbb..370e13d 100644
--- a/drivers/net/ethernet/smsc/smc91x.h
+++ b/drivers/net/ethernet/smsc/smc91x.h
@@ -286,16 +286,16 @@ static inline void mcf_outsw(void *a, unsigned char *p, int l)
 
 #define SMC_IO_SHIFT		(lp->io_shift)
 
-#define SMC_inb(a, r)		readb((a) + (r))
-#define SMC_inw(a, r)		readw((a) + (r))
-#define SMC_inl(a, r)		readl((a) + (r))
-#define SMC_outb(v, a, r)	writeb(v, (a) + (r))
-#define SMC_outw(v, a, r)	writew(v, (a) + (r))
-#define SMC_outl(v, a, r)	writel(v, (a) + (r))
-#define SMC_insw(a, r, p, l)	readsw((a) + (r), p, l)
-#define SMC_outsw(a, r, p, l)	writesw((a) + (r), p, l)
-#define SMC_insl(a, r, p, l)	readsl((a) + (r), p, l)
-#define SMC_outsl(a, r, p, l)	writesl((a) + (r), p, l)
+#define SMC_inb(a, r)		ioread8((a) + (r))
+#define SMC_inw(a, r)		ioread16((a) + (r))
+#define SMC_inl(a, r)		ioread32((a) + (r))
+#define SMC_outb(v, a, r)	iowrite8(v, (a) + (r))
+#define SMC_outw(v, a, r)	iowrite16(v, (a) + (r))
+#define SMC_outl(v, a, r)	iowrite32(v, (a) + (r))
+#define SMC_insw(a, r, p, l)	ioread16_rep((a) + (r), p, l)
+#define SMC_outsw(a, r, p, l)	iowrite16_rep((a) + (r), p, l)
+#define SMC_insl(a, r, p, l)	ioread32_rep((a) + (r), p, l)
+#define SMC_outsl(a, r, p, l)	iowrite32_rep((a) + (r), p, l)
 
 #define RPC_LSA_DEFAULT		RPC_LED_100_10
 #define RPC_LSB_DEFAULT		RPC_LED_TX_RX
-- 
1.7.4.1

Re: [RESEND PATCH 2/4] asm-generic: io: don't perform swab during {in,out} string functions

[Geert Uytterhoeven]

On Wed, Oct 17, 2012 at 5:45 PM, Will Deacon <will.deacon@arm.com> wrote:
> The {in,out}s{b,w,l} functions are designed to operate on a stream of
> bytes and therefore should not perform any byte-swapping, regardless of
> the CPU byte order.

Euh?

They transfer a stream of bytes between memory and PCI/ISA I/O space by
reading from/writing to a single I/O port of width 8, 16, or 32 bits.
On big endian machines, I/O port accesses may need to be swapped.

> This patch fixes the generic IO header so that {in,out}s{b,w,l} call
> the __raw_{read,write} functions directly rather than going via the
> endian-correcting accessors.

Furthermore __raw_*() has different semantics besides endianness, like
ordering barriers. So you can't just change one for the other.

> Cc: Mike Frysinger <vapier@gentoo.org>
> Cc: Ben Herrenschmidt <benh@kernel.crashing.org>
> Acked-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
>  include/asm-generic/io.h |   12 ++++++------
>  1 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
> index 3607921..403b861 100644
> --- a/include/asm-generic/io.h
> +++ b/include/asm-generic/io.h
> @@ -148,7 +148,7 @@ static inline void insb(unsigned long addr, void *buffer, int count)

The "addr" parameter is actually a misnomer. Probably it should be "port".
Oops, inb() and friends also use "addr".

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [RESEND PATCH 2/4] asm-generic: io: don't perform swab during {in,out} string functions

[Benjamin Herrenschmidt]

On Wed, 2012-10-17 at 16:45 +0100, Will Deacon wrote:
> The {in,out}s{b,w,l} functions are designed to operate on a stream of
> bytes and therefore should not perform any byte-swapping, regardless of
> the CPU byte order.
> 
> This patch fixes the generic IO header so that {in,out}s{b,w,l} call
> the __raw_{read,write} functions directly rather than going via the
> endian-correcting accessors.
> 
> Cc: Mike Frysinger <vapier@gentoo.org>

Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

> Acked-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
>  include/asm-generic/io.h |   12 ++++++------
>  1 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
> index 3607921..403b861 100644
> --- a/include/asm-generic/io.h
> +++ b/include/asm-generic/io.h
> @@ -148,7 +148,7 @@ static inline void insb(unsigned long addr, void *buffer, int count)
>  	if (count) {
>  		u8 *buf = buffer;
>  		do {
> -			u8 x = inb(addr);
> +			u8 x = __raw_readb(addr + PCI_IOBASE);
>  			*buf++ = x;
>  		} while (--count);
>  	}
> @@ -161,7 +161,7 @@ static inline void insw(unsigned long addr, void *buffer, int count)
>  	if (count) {
>  		u16 *buf = buffer;
>  		do {
> -			u16 x = inw(addr);
> +			u16 x = __raw_readw(addr + PCI_IOBASE);
>  			*buf++ = x;
>  		} while (--count);
>  	}
> @@ -174,7 +174,7 @@ static inline void insl(unsigned long addr, void *buffer, int count)
>  	if (count) {
>  		u32 *buf = buffer;
>  		do {
> -			u32 x = inl(addr);
> +			u32 x = __raw_readl(addr + PCI_IOBASE);
>  			*buf++ = x;
>  		} while (--count);
>  	}
> @@ -187,7 +187,7 @@ static inline void outsb(unsigned long addr, const void *buffer, int count)
>  	if (count) {
>  		const u8 *buf = buffer;
>  		do {
> -			outb(*buf++, addr);
> +			__raw_writeb(*buf++, addr + PCI_IOBASE);
>  		} while (--count);
>  	}
>  }
> @@ -199,7 +199,7 @@ static inline void outsw(unsigned long addr, const void *buffer, int count)
>  	if (count) {
>  		const u16 *buf = buffer;
>  		do {
> -			outw(*buf++, addr);
> +			__raw_writew(*buf++, addr + PCI_IOBASE);
>  		} while (--count);
>  	}
>  }
> @@ -211,7 +211,7 @@ static inline void outsl(unsigned long addr, const void *buffer, int count)
>  	if (count) {
>  		const u32 *buf = buffer;
>  		do {
> -			outl(*buf++, addr);
> +			__raw_writel(*buf++, addr + PCI_IOBASE);
>  		} while (--count);
>  	}
>  }

Re: [RESEND PATCH 2/4] asm-generic: io: don't perform swab during {in,out} string functions

[Benjamin Herrenschmidt]

On Wed, 2012-10-17 at 21:16 +0200, Geert Uytterhoeven wrote:
> On Wed, Oct 17, 2012 at 5:45 PM, Will Deacon <will.deacon@arm.com> wrote:
> > The {in,out}s{b,w,l} functions are designed to operate on a stream of
> > bytes and therefore should not perform any byte-swapping, regardless of
> > the CPU byte order.
> 
> Euh?

Will is perfectly right :-)

> They transfer a stream of bytes between memory and PCI/ISA I/O space by
> reading from/writing to a single I/O port of width 8, 16, or 32 bits.
> On big endian machines, I/O port accesses may need to be swapped.

No. Not for streams. I suggest you watch the recording of my Plumbers
talk :-)

The sort story is that endianness is not a property of the IO port but
of the information that transit through it. If you're just going to copy
it into memory, you want to preserve it's format and so do not byteswap.

The byteswap we do on standard accessors is a "helper" because we assume
that underneath those IO ports are registers that are Little Endian. But
when using one as a window to a byte stream, we must not arbitrarily
swap the byte stream. We copy it as-is to memory, and then one can work
at interpreting the various fields that might or might not be present in
that stream with the appropriate accessors for memory accesses. 

You will notice that all our stream IO functions are similarly
non-swapping on powerpc.

> > This patch fixes the generic IO header so that {in,out}s{b,w,l} call
> > the __raw_{read,write} functions directly rather than going via the
> > endian-correcting accessors.
> 
> Furthermore __raw_*() has different semantics besides endianness, like
> ordering barriers. So you can't just change one for the other.

This is asm-generic. If you need barriers, implement your own. As far as
I can tell, Will is perfectly correct.

Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

Cheers,
Ben.

> > Cc: Mike Frysinger <vapier@gentoo.org>
> > Cc: Ben Herrenschmidt <benh@kernel.crashing.org>
> > Acked-by: Arnd Bergmann <arnd@arndb.de>
> > Signed-off-by: Will Deacon <will.deacon@arm.com>
> > ---
> >  include/asm-generic/io.h |   12 ++++++------
> >  1 files changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
> > index 3607921..403b861 100644
> > --- a/include/asm-generic/io.h
> > +++ b/include/asm-generic/io.h
> > @@ -148,7 +148,7 @@ static inline void insb(unsigned long addr, void *buffer, int count)
> 
> The "addr" parameter is actually a misnomer. Probably it should be "port".
> Oops, inb() and friends also use "addr".
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

Re: [RESEND PATCH 2/4] asm-generic: io: don't perform swab during {in,out} string functions

[Geert Uytterhoeven]

Hi Ben,

On Thu, Oct 18, 2012 at 2:04 AM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Wed, 2012-10-17 at 21:16 +0200, Geert Uytterhoeven wrote:
>> On Wed, Oct 17, 2012 at 5:45 PM, Will Deacon <will.deacon@arm.com> wrote:
>> > The {in,out}s{b,w,l} functions are designed to operate on a stream of
>> > bytes and therefore should not perform any byte-swapping, regardless of
>> > the CPU byte order.
>>
>> Euh?
>
> Will is perfectly right :-)
>
>> They transfer a stream of bytes between memory and PCI/ISA I/O space by
>> reading from/writing to a single I/O port of width 8, 16, or 32 bits.
>> On big endian machines, I/O port accesses may need to be swapped.
>
> No. Not for streams. I suggest you watch the recording of my Plumbers
> talk :-)

I will. But as email text on lkml is easier to refer to than videos, I
will continue below,
so yo can refute me in plain text ;-)

> The sort story is that endianness is not a property of the IO port but
> of the information that transit through it. If you're just going to copy
> it into memory, you want to preserve it's format and so do not byteswap.
>
> The byteswap we do on standard accessors is a "helper" because we assume
> that underneath those IO ports are registers that are Little Endian. But
> when using one as a window to a byte stream, we must not arbitrarily
> swap the byte stream. We copy it as-is to memory, and then one can work
> at interpreting the various fields that might or might not be present in
> that stream with the appropriate accessors for memory accesses.

So assume you have the bytestream "Hello, world!\n" in memory on the
PCI device.I.e.

00000000  48 65 6c 6c 6f 2c 20 77  6f 72 6c 64 21 0a        |Hello, world!.|

You want to copy it to system RAM using readsl(), which does:

               u32 *buf = buffer;
               do {
                       u32 x = __raw_readl(addr + PCI_IOBASE);
                       *buf++ = x;
                } while (--count);

On little endian, the first __raw_readl() should return "0x6c6c6548", so
it is stored correctly by "*buf = x ".
On big endian, the first __raw_readl() should return "0x48656c6c" instead,
else it's stored incorrectly by "*buf = x ".
But the PCI bus is little endian, so I expect __raw_readl() would return
"0x6c6c6548", and thus needs swapping?

On m68k (classic with MMU), the situation is a bit more complicated, as some ISA
(or PCMCIA) busses are physically wired swapped. But inw() and insw() always do
either both swapping, or both no swapping. The state of support for ISA drivers
is a bit vague, though.

Now, on m68k without MMU, inw() doesn't do swapping, but insw() does.
This agrees more or less with you and Will, except that the bus seems to be
physically swapped?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [RESEND PATCH 4/4] net: smc91x: use io{read,write}*_rep accessors instead of string functions

[James Hogan]

On 17/10/12 16:45, Will Deacon wrote:
> The {read,write}s{b,w,l} operations are not defined by all architectures
> and are being removed from the asm-generic/io.h interface.
> 
> This patch replaces the usage of these string functions in the default
> SMC accessors with io{read,write}{8,16,32}_rep calls instead, which are
> defined for all architectures.

Hi,

Should this patch (and the mmc one) precede patch 1?

Also there appear to be some other places that it's used (e.g.
smsc911x.c and smc911x.h). I guess these should be fixed up first too?

Cheers
James

Re: [RESEND PATCH 4/4] net: smc91x: use io{read,write}*_rep accessors instead of string functions

[James Hogan]

On 17/10/12 16:45, Will Deacon wrote:
> The {read,write}s{b,w,l} operations are not defined by all architectures
> and are being removed from the asm-generic/io.h interface.
> 
> This patch replaces the usage of these string functions in the default
> SMC accessors with io{read,write}{8,16,32}_rep calls instead, which are
> defined for all architectures.

Hi,

Should this patch (and the mmc one) precede patch 1?

Also there appear to be some other places that it's used (e.g.
smsc911x.c and smc911x.h). I guess these should be fixed up first too?

Cheers
James

Re: [RESEND PATCH 4/4] net: smc91x: use io{read,write}*_rep accessors instead of string functions

[Will Deacon]

On Fri, Oct 19, 2012 at 09:25:48AM +0100, James Hogan wrote:
> On 17/10/12 16:45, Will Deacon wrote:
> > The {read,write}s{b,w,l} operations are not defined by all architectures
> > and are being removed from the asm-generic/io.h interface.
> > 
> > This patch replaces the usage of these string functions in the default
> > SMC accessors with io{read,write}{8,16,32}_rep calls instead, which are
> > defined for all architectures.
> 
> Hi,
> 
> Should this patch (and the mmc one) precede patch 1?

Yup, I can reorder that easily enough.

> Also there appear to be some other places that it's used (e.g.
> smsc911x.c and smc911x.h). I guess these should be fixed up first too?

I could have a look if you like, but I'm wary of changing the I/O accessors
in anything but a mechanical fashion given that I can't test any of my
changes.

Will

Re: [RESEND PATCH 2/4] asm-generic: io: don't perform swab during {in,out} string functions

[Will Deacon]

On Thu, Oct 18, 2012 at 06:48:16AM +0100, Geert Uytterhoeven wrote:
> On Thu, Oct 18, 2012 at 2:04 AM, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
> > The sort story is that endianness is not a property of the IO port but
> > of the information that transit through it. If you're just going to copy
> > it into memory, you want to preserve it's format and so do not byteswap.
> >
> > The byteswap we do on standard accessors is a "helper" because we assume
> > that underneath those IO ports are registers that are Little Endian. But
> > when using one as a window to a byte stream, we must not arbitrarily
> > swap the byte stream. We copy it as-is to memory, and then one can work
> > at interpreting the various fields that might or might not be present in
> > that stream with the appropriate accessors for memory accesses.
> 
> So assume you have the bytestream "Hello, world!\n" in memory on the
> PCI device.I.e.
> 
> 00000000  48 65 6c 6c 6f 2c 20 77  6f 72 6c 64 21 0a        |Hello, world!.|
> 
> You want to copy it to system RAM using readsl(), which does:
> 
>                u32 *buf = buffer;
>                do {
>                        u32 x = __raw_readl(addr + PCI_IOBASE);
>                        *buf++ = x;
>                 } while (--count);
> 
> On little endian, the first __raw_readl() should return "0x6c6c6548", so
> it is stored correctly by "*buf = x ".
> On big endian, the first __raw_readl() should return "0x48656c6c" instead,
> else it's stored incorrectly by "*buf = x ".

So far so good...

> But the PCI bus is little endian, so I expect __raw_readl() would return
> "0x6c6c6548", and thus needs swapping?

I think this would only happen if your busses are wired swapped, in which
case you'll have to handle this in your arch code because reading from a
device and then writing to memory will end up with the data in the wrong
order (the data stream won't be affected by passing through the CPU).

Will

Re: [RESEND PATCH 2/4] asm-generic: io: don't perform swab during {in,out} string functions

[Benjamin Herrenschmidt]

On Thu, 2012-10-18 at 07:48 +0200, Geert Uytterhoeven wrote:

> So assume you have the bytestream "Hello, world!\n" in memory on the
> PCI device.I.e.
> 
> 00000000  48 65 6c 6c 6f 2c 20 77  6f 72 6c 64 21 0a        |Hello, world!.|
> 
> You want to copy it to system RAM using readsl(), which does:
> 
>                u32 *buf = buffer;
>                do {
>                        u32 x = __raw_readl(addr + PCI_IOBASE);
>                        *buf++ = x;
>                 } while (--count);
> 
> On little endian, the first __raw_readl() should return "0x6c6c6548", so
> it is stored correctly by "*buf = x ".

Right.

> On big endian, the first __raw_readl() should return "0x48656c6c" instead,
> else it's stored incorrectly by "*buf = x ".
> But the PCI bus is little endian, so I expect __raw_readl() would return
> "0x6c6c6548", and thus needs swapping?

No. The PCI bus will return 0x48656c6c.

This is due to how the PCI bus is wired to the CPU bus, which is called
"byte address invariant". When doing a read of your byte 0, the CPU will
effectively read 0 with byte enables picking 48. Since the CPU wants
the first byte in the MSB, the bus must be wired up to the CPU such that
the MSB is the first byte in address order.

This is how a BE CPU is normally wired to a LE bus. The fact that a
register needs to be swapped comes from the fact that the device will
put the MSB in the higher address byte, which then corresponds to the
LSB on the BE CPU -> needs swapping.

But for a byte stream, as you can see, no swapping is required.

To some extent, it looks as if the wiring is byte swapped, in order to
provide the byte address invariance (and thus causes registers to look
byteswapped) but it's really not, it's just a convention.

> On m68k (classic with MMU), the situation is a bit more complicated, as some ISA
> (or PCMCIA) busses are physically wired swapped. But inw() and insw() always do
> either both swapping, or both no swapping. The state of support for ISA drivers
> is a bit vague, though.

Then something is horribly wrong in those m68k setups :-) Either in the
way the busses are wired or in your implementation of either inw or
insw.

> Now, on m68k without MMU, inw() doesn't do swapping, but insw() does.
> This agrees more or less with you and Will, except that the bus seems to be
> physically swapped?

Yeah that's possible. In the early days people incorrectly thought it
would be smart to do that, thus avoiding a byteswap for register
accesses .... and causing all data (DMA or FIFO) to be all backward.

That's for example why the disks in the Tivo are byteswapped 16-bit by
16-bit on the platter (ie, all the data is).

I wouldn't be surprised if some 68k suffered from similar HW designer
stupidity.

But the "proper" generic rule is byte address invariance, and that
implies swap on register access and no swap on byte streams.

Cheers,
Ben.


> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arch" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RESEND PATCH 1/4] asm-generic: io: remove {read,write} string functions

[Arnd Bergmann]

On Wednesday 17 October 2012, Will Deacon wrote:
> The {read,write}s{b,w,l} functions are not defined across all
> architectures and therefore shouldn't be used by portable drivers. We
> should encourage driver writers to use the io{read,write}{8,16,32}_rep
> functions instead.
> 
> This patch removes the {read,write} string functions for the generic IO
> header as they have no place in a new architecture port.
> 
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Mike Frysinger <vapier@gentoo.org>
> Cc: Ben Herrenschmidt <benh@kernel.crashing.org>
> Signed-off-by: Will Deacon <will.deacon@arm.com>

Acked-by: Arnd Bergmann <arnd@arndb.de>

What should we do with the series? I can take them through the asm-generic
tree if you like, or we can let Catalin take them through the arm64 tree.

	Arnd

Re: [RESEND PATCH 1/4] asm-generic: io: remove {read,write} string functions

[Will Deacon]

On Fri, Oct 26, 2012 at 02:29:03PM +0100, Arnd Bergmann wrote:
> On Wednesday 17 October 2012, Will Deacon wrote:
> > The {read,write}s{b,w,l} functions are not defined across all
> > architectures and therefore shouldn't be used by portable drivers. We
> > should encourage driver writers to use the io{read,write}{8,16,32}_rep
> > functions instead.
> > 
> > This patch removes the {read,write} string functions for the generic IO
> > header as they have no place in a new architecture port.
> > 
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: Mike Frysinger <vapier@gentoo.org>
> > Cc: Ben Herrenschmidt <benh@kernel.crashing.org>
> > Signed-off-by: Will Deacon <will.deacon@arm.com>
> 
> Acked-by: Arnd Bergmann <arnd@arndb.de>

Cheers Arnd!

> What should we do with the series? I can take them through the asm-generic
> tree if you like, or we can let Catalin take them through the arm64 tree.

Can you take patch 2/4 via asm-generic please? I'll respin the rest of the
series and try and update some more drivers in the process so that the
removal of the generic stream functions doesn't cause breakage for drivers
that aren't arch-specific already.

Thanks,

Will

Re: [RESEND PATCH 2/4] asm-generic: io: don't perform swab during {in,out} string functions

[Geert Uytterhoeven]

Hi Ben,

On Tue, Oct 23, 2012 at 3:25 AM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Thu, 2012-10-18 at 07:48 +0200, Geert Uytterhoeven wrote:
>> So assume you have the bytestream "Hello, world!\n" in memory on the
>> PCI device.I.e.
>>
>> 00000000  48 65 6c 6c 6f 2c 20 77  6f 72 6c 64 21 0a        |Hello, world!.|
>>
>> You want to copy it to system RAM using readsl(), which does:
>>
>>                u32 *buf = buffer;
>>                do {
>>                        u32 x = __raw_readl(addr + PCI_IOBASE);
>>                        *buf++ = x;
>>                 } while (--count);
>>
>> On little endian, the first __raw_readl() should return "0x6c6c6548", so
>> it is stored correctly by "*buf = x ".
>
> Right.
>
>> On big endian, the first __raw_readl() should return "0x48656c6c" instead,
>> else it's stored incorrectly by "*buf = x ".
>> But the PCI bus is little endian, so I expect __raw_readl() would return
>> "0x6c6c6548", and thus needs swapping?
>
> No. The PCI bus will return 0x48656c6c.
>
> This is due to how the PCI bus is wired to the CPU bus, which is called
> "byte address invariant". When doing a read of your byte 0, the CPU will
> effectively read 0 with byte enables picking 48. Since the CPU wants
> the first byte in the MSB, the bus must be wired up to the CPU such that
> the MSB is the first byte in address order.

According to
https://developer.apple.com/library/mac/#documentation/DeviceDrivers/Conceptual/WritingPCIDrivers/endianness/endianness.html

"Byte-invariant addressing is a property of the bus bridge itself."

and

"From the software designer’s perspective, this means that the
hardware does not byte swap the data. However, from the hardware
designer’s perspective, the hardware must byte swap all data."

So this depends on a correct hardware implementation in the PCI
host bridge?

> This is how a BE CPU is normally wired to a LE bus. The fact that a
> register needs to be swapped comes from the fact that the device will
> put the MSB in the higher address byte, which then corresponds to the
> LSB on the BE CPU -> needs swapping.
>
> But for a byte stream, as you can see, no swapping is required.
>
> To some extent, it looks as if the wiring is byte swapped, in order to
> provide the byte address invariance (and thus causes registers to look
> byteswapped) but it's really not, it's just a convention.
>
>> On m68k (classic with MMU), the situation is a bit more complicated, as some ISA
>> (or PCMCIA) busses are physically wired swapped. But inw() and insw() always do
>> either both swapping, or both no swapping. The state of support for ISA drivers
>> is a bit vague, though.
>
> Then something is horribly wrong in those m68k setups :-) Either in the
> way the busses are wired or in your implementation of either inw or
> insw.

On (classic) m68k all of this is not about PCI (Atari Hades PCI is no
more), but about ISA and PCMCIA. I.e. no PCI host bridge with a modern
understanding of how it should be wired correctly on a big endian platform.

>> Now, on m68k without MMU, inw() doesn't do swapping, but insw() does.
>> This agrees more or less with you and Will, except that the bus seems to be
>> physically swapped?
>
> Yeah that's possible. In the early days people incorrectly thought it
> would be smart to do that, thus avoiding a byteswap for register
> accesses .... and causing all data (DMA or FIFO) to be all backward.
>
> That's for example why the disks in the Tivo are byteswapped 16-bit by
> 16-bit on the platter (ie, all the data is).
>
> I wouldn't be surprised if some 68k suffered from similar HW designer
> stupidity.

Yep, Atari IDE suffers from this, too.

> But the "proper" generic rule is byte address invariance, and that
> implies swap on register access and no swap on byte streams.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [RESEND PATCH 2/4] asm-generic: io: don't perform swab during {in,out} string functions

[Benjamin Herrenschmidt]

On Sun, 2012-10-28 at 10:28 +0100, Geert Uytterhoeven wrote:

> > This is due to how the PCI bus is wired to the CPU bus, which is called
> > "byte address invariant". When doing a read of your byte 0, the CPU will
> > effectively read 0 with byte enables picking 48. Since the CPU wants
> > the first byte in the MSB, the bus must be wired up to the CPU such that
> > the MSB is the first byte in address order.
> 
> According to
> https://developer.apple.com/library/mac/#documentation/DeviceDrivers/Conceptual/WritingPCIDrivers/endianness/endianness.html
> 
> "Byte-invariant addressing is a property of the bus bridge itself."

Right.

> and
> 
> "From the software designer’s perspective, this means that the
> hardware does not byte swap the data. However, from the hardware
> designer’s perspective, the hardware must byte swap all data."
> 
> So this depends on a correct hardware implementation in the PCI
> host bridge?

Correct. It is also how AMBA works on BE for example, it's generally
accepted that this is the "right" way to wire a bridge.

> > Then something is horribly wrong in those m68k setups :-) Either in the
> > way the busses are wired or in your implementation of either inw or
> > insw.
> 
> On (classic) m68k all of this is not about PCI (Atari Hades PCI is no
> more), but about ISA and PCMCIA. I.e. no PCI host bridge with a modern
> understanding of how it should be wired correctly on a big endian platform.

There's nothing modern about byte address invariance. The same rule
applies to ISA and PCMCIA just the same. It's possible that your
specific m68k platforms were designed by monkeys on crack, which seems
to be a common breed among HW designers, but that doesn't make it
right :-)

Cheers,
Ben.