Fix endianness of generic I/O accessors

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 <[email protected]>

Cc Mike Frysinger <[email protected]>

Cc Ben Herrenschmidt <[email protected]>

Signed-off-by Will Deacon <[email protected]>

Quoted 8 lines · 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.

Quoted 4 lines

Cc Arnd Bergmann <[email protected]>

Cc Mike Frysinger <[email protected]>

Cc Ben Herrenschmidt <[email protected]>

Signed-off-by Will Deacon <[email protected]>

Acked-by Arnd Bergmann <[email protected]>

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.

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.

Quoted 3 lines · 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.

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.

Quoted 3 lines

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.

include/asm-generic/io.h static inline void insb(unsigned long addr, void *buffer, int count)

Quoted 13 lines

Cc Mike Frysinger <[email protected]>

Cc Ben Herrenschmidt <[email protected]>

Acked-by Arnd Bergmann <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---
 include/asm-generic/io.h |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

include/asm-generic/io.h +0 -0

index 3607921..403b861 100644
@@ -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 -- [email protected]

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

Quoted 6 lines · Geert Uytterhoeven

Quoted 3 lines · 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.

Euh?

Will is perfectly right :-)

Quoted 3 lines

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.

Quoted 10 lines · Benjamin Herrenschmidt

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?

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 -- [email protected]

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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 7 lines · Geert Uytterhoeven

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:

Quoted 8 lines

               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.

Quoted 4 lines

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.

Quoted 4 lines

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.

Quoted 3 lines

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.

Quoted 5 lines

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?

Quoted 3 lines

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.

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 <[email protected]>

Acked-by Arnd Bergmann <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---
 include/asm-generic/io.h |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

include/asm-generic/io.h +6 -6

index 3607921..403b861 100644
@@ -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);
 	}
 }

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.

Quoted 6 lines · 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.

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?