From: Will Deacon <hidden> Date: 2012-10-17 15:46:00
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
From: Will Deacon <hidden> Date: 2012-10-17 15:45:14
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 <redacted>
---
drivers/mmc/host/mmci.c | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
From: Will Deacon <hidden> Date: 2012-10-17 15:45:25
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 <redacted>
Cc: Ben Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Will Deacon <redacted>
---
include/asm-generic/io.h | 30 ------------------------------
1 files changed, 0 insertions(+), 30 deletions(-)
From: Will Deacon <hidden> Date: 2012-10-17 15:45:28
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 <redacted>
Cc: Ben Herrenschmidt <benh@kernel.crashing.org>
Acked-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Will Deacon <redacted>
---
include/asm-generic/io.h | 12 ++++++------
1 files changed, 6 insertions(+), 6 deletions(-)
From: Will Deacon <hidden> Date: 2012-10-17 15:46:19
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 <redacted>
---
drivers/net/ethernet/smsc/smc91x.h | 20 ++++++++++----------
1 files changed, 10 insertions(+), 10 deletions(-)
On Wed, Oct 17, 2012 at 5:45 PM, Will Deacon [off-list ref] 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.
quoted hunk
Cc: Mike Frysinger <redacted>
Cc: Ben Herrenschmidt <benh@kernel.crashing.org>
Acked-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Will Deacon <redacted>
---
include/asm-generic/io.h | 12 ++++++------
1 files changed, 6 insertions(+), 6 deletions(-)
@@ -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
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2012-10-18 00:02:05
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 <redacted>
Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2012-10-18 00:04:30
On Wed, 2012-10-17 at 21:16 +0200, Geert Uytterhoeven wrote:
On Wed, Oct 17, 2012 at 5:45 PM, Will Deacon [off-list ref] wrote:
quoted
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.
quoted
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.
quoted
Cc: Mike Frysinger <redacted>
Cc: Ben Herrenschmidt <benh@kernel.crashing.org>
Acked-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Will Deacon <redacted>
---
include/asm-generic/io.h | 12 ++++++------
1 files changed, 6 insertions(+), 6 deletions(-)
@@ -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
Hi Ben,
On Thu, Oct 18, 2012 at 2:04 AM, Benjamin Herrenschmidt
[off-list ref] wrote:
On Wed, 2012-10-17 at 21:16 +0200, Geert Uytterhoeven wrote:
quoted
On Wed, Oct 17, 2012 at 5:45 PM, Will Deacon [off-list ref] wrote:
quoted
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
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
From: James Hogan <hidden> Date: 2012-10-19 08:25:53
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
From: Will Deacon <hidden> Date: 2012-10-19 09:27:25
On Fri, Oct 19, 2012 at 09:25:48AM +0100, James Hogan wrote:
On 17/10/12 16:45, Will Deacon wrote:
quoted
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
From: Will Deacon <hidden> Date: 2012-10-19 12:53:22
On Thu, Oct 18, 2012 at 06:48:16AM +0100, Geert Uytterhoeven wrote:
On Thu, Oct 18, 2012 at 2:04 AM, Benjamin Herrenschmidt
[off-list ref] wrote:
quoted
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
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2012-10-23 02:17:06
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
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 <redacted>
Cc: Ben Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Will Deacon <redacted>
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
From: Will Deacon <hidden> Date: 2012-10-26 13:43:55
On Fri, Oct 26, 2012 at 02:29:03PM +0100, Arnd Bergmann wrote:
On Wednesday 17 October 2012, Will Deacon wrote:
quoted
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 <redacted>
Cc: Ben Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Will Deacon <redacted>
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
Hi Ben,
On Tue, Oct 23, 2012 at 3:25 AM, Benjamin Herrenschmidt
[off-list ref] wrote:
On Thu, 2012-10-18 at 07:48 +0200, Geert Uytterhoeven wrote:
quoted
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.
quoted
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
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.
quoted
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
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2012-10-28 20:38:47
On Sun, 2012-10-28 at 10:28 +0100, Geert Uytterhoeven wrote:
quoted
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.
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.
quoted
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.