endianness swapped

Quoted 6 lines · Angelo Dureghello

as you may know, i am working on mcf5441x.
Sorry for not following carefully all the threads, but from a certain kernel version (likely 4.19 or near there), seems ioread32be reads the bytes swapped in endianness (mcf-edma dma driver not working anymore).

Quoted 2 lines

Has there been a change about this in the architecture I/O access ? How should i proceed now ? Fixing the DMA driver read/write, or what ?

looks like the reason of my ioread32be now swapped is:

https://patchwork.kernel.org/patch/10766673/

Trying to figure out what to do now.

Quoted 7 lines · Angelo Dureghello

Quoted 6 lines · Angelo Dureghello

as you may know, i am working on mcf5441x.
Sorry for not following carefully all the threads, but from a certain kernel version (likely 4.19 or near there), seems ioread32be reads the bytes swapped in endianness (mcf-edma dma driver not working anymore).

Quoted 8 lines

Quoted 2 lines

Has there been a change about this in the architecture I/O access ? How should i proceed now ? Fixing the DMA driver read/write, or what ?

looks like the reason of my ioread32be now swapped is:

https://patchwork.kernel.org/patch/10766673/

Trying to figure out what to do now.

This is commit aecc787c06f4300f ("iomap: Use non-raw io functions for io{read|write}XXbe"):

lib/iomap.c +2 -2

@@ -65,8 +65,8 @@ static void bad_io_access(unsigned long port, const
 char *access)
 #endif
 
 #ifndef mmio_read16be
-#define mmio_read16be(addr) be16_to_cpu(__raw_readw(addr))
-#define mmio_read32be(addr) be32_to_cpu(__raw_readl(addr))
+#define mmio_read16be(addr) swab16(readw(addr))
+#define mmio_read32be(addr) swab32(readl(addr))
 #endif
 

unsigned int ioread8(void __iomem *addr)

lib/iomap.c +2 -2

@@ -106,8 +106,8 @@ EXPORT_SYMBOL(ioread32be);
 #endif
 
 #ifndef mmio_write16be
-#define mmio_write16be(val,port) __raw_writew(be16_to_cpu(val),port)
-#define mmio_write32be(val,port) __raw_writel(be32_to_cpu(val),port)
+#define mmio_write16be(val,port) writew(swab16(val),port)
+#define mmio_write32be(val,port) writel(swab32(val),port)
 
 On big endian, the raw accessors are assumed to be non-swapping,

while non-raw accessors are assumed to be swapping. The latter is not true for Coldfire internal registers, cfr. arch/m68k/include/asm/io_no.h:

static inline u16 readw(const volatile void __iomem *addr)

{
        if (cf_internalio(addr))
                return __raw_readw(addr);
        return __le16_to_cpu(__raw_readw(addr));
}

Orthogonal to how Coldfire's read[wl]() should be fixed, I find it a bit questionable to swap data twice on big endian architectures.

Fortunately we can avoid that by defining our own
mmio_{read,write}{16,32}be()...

Quoted 44 lines · Geert Uytterhoeven

Quoted 7 lines · Angelo Dureghello

Quoted 6 lines · Angelo Dureghello

as you may know, i am working on mcf5441x.
Sorry for not following carefully all the threads, but from a certain kernel version (likely 4.19 or near there), seems ioread32be reads the bytes swapped in endianness (mcf-edma dma driver not working anymore).

Quoted 8 lines

Quoted 2 lines

Has there been a change about this in the architecture I/O access ? How should i proceed now ? Fixing the DMA driver read/write, or what ?

looks like the reason of my ioread32be now swapped is:

https://patchwork.kernel.org/patch/10766673/

Trying to figure out what to do now.

This is commit aecc787c06f4300f ("iomap: Use non-raw io functions for io{read|write}XXbe"):

lib/iomap.c +2 -2

@@ -65,8 +65,8 @@ static void bad_io_access(unsigned long port, const
 char *access)
  #endif
 
  #ifndef mmio_read16be
-#define mmio_read16be(addr) be16_to_cpu(__raw_readw(addr))
-#define mmio_read32be(addr) be32_to_cpu(__raw_readl(addr))
+#define mmio_read16be(addr) swab16(readw(addr))
+#define mmio_read32be(addr) swab32(readl(addr))
  #endif
 

unsigned int ioread8(void __iomem *addr)

lib/iomap.c +2 -2

@@ -106,8 +106,8 @@ EXPORT_SYMBOL(ioread32be);
  #endif
 
  #ifndef mmio_write16be
-#define mmio_write16be(val,port) __raw_writew(be16_to_cpu(val),port)
-#define mmio_write32be(val,port) __raw_writel(be32_to_cpu(val),port)
+#define mmio_write16be(val,port) writew(swab16(val),port)
+#define mmio_write32be(val,port) writel(swab32(val),port)
 
 On big endian, the raw accessors are assumed to be non-swapping,

Quoted 3 lines

while non-raw accessors are assumed to be swapping. The latter is not true for Coldfire internal registers, cfr. arch/m68k/include/asm/io_no.h:

The raw accessors are always assumed to be non-swapping in the asm-generic code, while the non-raw ones are assumed to be little-endian in order for them to work with portable drivers.

We have some other cases of big-endian machines that use a hardware byteswap on their MMIO buses (iirc some mips and superh parts), but they then need to swap the __raw_* accessor data words in software to get back to the normal behavior, as well as swizzle the address for accesses that are less than 32 bit wide.

Coldfire makes the behavior of readw()/readl() depend on the MMIO address, presumably since that was the easiest way to get drivers working originally, but it breaks the assumption in the asm-generic code.

Quoted 9 lines

static inline u16 readw(const volatile void __iomem *addr)

{
         if (cf_internalio(addr))
                 return __raw_readw(addr);
         return __le16_to_cpu(__raw_readw(addr));
}

Orthogonal to how Coldfire's read[wl]() should be fixed, I find it a bit questionable to swap data twice on big endian architectures.

I would expect that the compiler is capable of detecting a double swap and optimize it out. Even if it can't, there are not that many instances of io{read,write}{16,32}be in the kernel, so the increase in kernel image size from a double swap should be limited to a few extra instructions, and the runtime overhead should be negligible compared to the bus access.

Quoted 2 lines

Fortunately we can avoid that by defining our own
mmio_{read,write}{16,32}be()...

Quoted 9 lines · Arnd Bergmann

Quoted 2 lines · Geert Uytterhoeven

Quoted 1 line · Angelo Dureghello

On Sat, Apr 27, 2019 at 05:32:22PM +0200, Angelo Dureghello wrote:

Coldfire makes the behavior of readw()/readl() depend on the MMIO address, presumably since that was the easiest way to get drivers working originally, but it breaks the assumption in the asm-generic code.

Yes, that is right.

There is a number of common hardware modules that Freescale have used in the ColdFire SoC parts and in their ARM based parts (iMX families). The ARM parts are pretty much always little endian, and the ColdFire is always big endian. The hardware registers in those hardware blocks are always accessed in native endian of the processor.

So the address range checks are to deal with those internal hardware blocks (i2c, spi, dma, etc), since we know those are at fixed addresses. That leaves the usual endian swapping in place for other general (ie external) devices (PCI devices, network chips, etc).

Quoted 11 lines · Greg Ungerer

Quoted 9 lines · Arnd Bergmann

Quoted 2 lines · Geert Uytterhoeven

Quoted 1 line · Angelo Dureghello

On Sat, Apr 27, 2019 at 05:32:22PM +0200, Angelo Dureghello wrote:

Coldfire makes the behavior of readw()/readl() depend on the MMIO address, presumably since that was the easiest way to get drivers working originally, but it breaks the assumption in the asm-generic code.

Quoted 7 lines

Yes, that is right.

There is a number of common hardware modules that Freescale have used in the ColdFire SoC parts and in their ARM based parts (iMX families). The ARM parts are pretty much always little endian, and the ColdFire is always big endian. The hardware registers in those hardware blocks are always accessed in native endian of the processor.

In later Freescale/NXP ARM SoCs (i.MX and Layerscape), we also get a lot of devices pulled over from PowerPC, with random endianess. In some cases, the same device that had big-endian registers originally ends up in two different ARM products and one of them uses big-endian while the other one uses little-endian registers.

Quoted 4 lines

So the address range checks are to deal with those internal hardware blocks (i2c, spi, dma, etc), since we know those are at fixed addresses. That leaves the usual endian swapping in place for other general (ie external) devices (PCI devices, network chips, etc).

Is there a complete list of coldfire on-chip device drivers?

Looking at some of the drivers:

- drivers/i2c/busses/i2c-imx.c uses only 8-bit accesses and works either way, same for drivers/tty/serial/mcf.c
- drivers/spi/spi-coldfire-qspi.c is apparently coldfire-only and could use ioread32be for a portable to do big-endian register access.
- edma-common has a wrapper to support both big-endian and little-endian configurations in the same kernel image, but the mcf interrupt handler is hardcoded to the (normally) little-endian ioread32 function.
- drivers/net/ethernet/freescale/fec_main.c is shared between coldfire and i.MX (but not mpc52xx), and is hardcoded to readl/writel, and would need the same trick as edma to make it portable.

Arnd

Quoted 11 lines · Greg Ungerer

Quoted 9 lines · Arnd Bergmann

Quoted 2 lines · Geert Uytterhoeven

Quoted 1 line · Angelo Dureghello

On Sat, Apr 27, 2019 at 05:32:22PM +0200, Angelo Dureghello wrote:

Coldfire makes the behavior of readw()/readl() depend on the MMIO address, presumably since that was the easiest way to get drivers working originally, but it breaks the assumption in the asm-generic code.

Quoted 7 lines

Yes, that is right.

There is a number of common hardware modules that Freescale have used in the ColdFire SoC parts and in their ARM based parts (iMX families). The ARM parts are pretty much always little endian, and the ColdFire is always big endian. The hardware registers in those hardware blocks are always accessed in native endian of the processor.

In later Freescale/NXP ARM SoCs (i.MX and Layerscape), we also get a lot of devices pulled over from PowerPC, with random endianess. In some cases, the same device that had big-endian registers originally ends up in two different ARM products and one of them uses big-endian while the other one uses little-endian registers.

Quoted 4 lines

So the address range checks are to deal with those internal hardware blocks (i2c, spi, dma, etc), since we know those are at fixed addresses. That leaves the usual endian swapping in place for other general (ie external) devices (PCI devices, network chips, etc).

Is there a complete list of coldfire on-chip device drivers?

Looking at some of the drivers:

- drivers/i2c/busses/i2c-imx.c uses only 8-bit accesses and works either way, same for drivers/tty/serial/mcf.c
- drivers/spi/spi-coldfire-qspi.c is apparently coldfire-only and could use ioread32be for a portable to do big-endian register access.
- edma-common has a wrapper to support both big-endian and little-endian configurations in the same kernel image, but the mcf interrupt handler is hardcoded to the (normally) little-endian ioread32 function.
- drivers/net/ethernet/freescale/fec_main.c is shared between coldfire and i.MX (but not mpc52xx), and is hardcoded to readl/writel, and would need the same trick as edma to make it portable.

Quoted 4 lines · Arnd Bergmann

Is there a complete list of coldfire on-chip device drivers?

Looking at some of the drivers:

Quoted 10 lines

- drivers/i2c/busses/i2c-imx.c uses only 8-bit accesses and works either way, same for drivers/tty/serial/mcf.c
- drivers/spi/spi-coldfire-qspi.c is apparently coldfire-only and could use ioread32be for a portable to do big-endian register access.
- edma-common has a wrapper to support both big-endian and little-endian configurations in the same kernel image, but the mcf interrupt handler is hardcoded to the (normally) little-endian ioread32 function.
- drivers/net/ethernet/freescale/fec_main.c is shared between coldfire and i.MX (but not mpc52xx), and is hardcoded to readl/writel, and would need the same trick as edma to make it portable.

That matches up with what we list out in arch/m68k/coldfire/devices.c. I can't think of any other drivers.

There is a lot of use readl/writel and friends in the architecture specific code too, in arch/m68k/coldfire. At first I used __raw_readl/ __raw_writel to always get native endianess. But quote a few uses of readl/writel have crept in over the years.

Quoted 60 lines · Arnd Bergmann

Quoted 57 lines · Geert Uytterhoeven

Quoted 7 lines · Angelo Dureghello

Quoted 6 lines · Angelo Dureghello

as you may know, i am working on mcf5441x.
Sorry for not following carefully all the threads, but from a certain kernel version (likely 4.19 or near there), seems ioread32be reads the bytes swapped in endianness (mcf-edma dma driver not working anymore).

Quoted 8 lines

Quoted 2 lines

Has there been a change about this in the architecture I/O access ? How should i proceed now ? Fixing the DMA driver read/write, or what ?

looks like the reason of my ioread32be now swapped is:

https://patchwork.kernel.org/patch/10766673/

Trying to figure out what to do now.

This is commit aecc787c06f4300f ("iomap: Use non-raw io functions for io{read|write}XXbe"):

lib/iomap.c +2 -2

@@ -65,8 +65,8 @@ static void bad_io_access(unsigned long port, const
 char *access)
  #endif
 
  #ifndef mmio_read16be
-#define mmio_read16be(addr) be16_to_cpu(__raw_readw(addr))
-#define mmio_read32be(addr) be32_to_cpu(__raw_readl(addr))
+#define mmio_read16be(addr) swab16(readw(addr))
+#define mmio_read32be(addr) swab32(readl(addr))
  #endif
 

unsigned int ioread8(void __iomem *addr)

lib/iomap.c +2 -2

@@ -106,8 +106,8 @@ EXPORT_SYMBOL(ioread32be);
  #endif
 
  #ifndef mmio_write16be
-#define mmio_write16be(val,port) __raw_writew(be16_to_cpu(val),port)
-#define mmio_write32be(val,port) __raw_writel(be32_to_cpu(val),port)
+#define mmio_write16be(val,port) writew(swab16(val),port)
+#define mmio_write32be(val,port) writel(swab32(val),port)
 
 On big endian, the raw accessors are assumed to be non-swapping,

while non-raw accessors are assumed to be swapping. The latter is not true for Coldfire internal registers, cfr. arch/m68k/include/asm/io_no.h:

static inline u16 readw(const volatile void __iomem *addr)

{
         if (cf_internalio(addr))
                 return __raw_readw(addr);
         return __le16_to_cpu(__raw_readw(addr));
}

Orthogonal to how Coldfire's read[wl]() should be fixed, I find it a bit questionable to swap data twice on big endian architectures.

Quoted 6 lines

I would expect that the compiler is capable of detecting a double swap and optimize it out. Even if it can't, there are not that many instances of io{read,write}{16,32}be in the kernel, so the increase in kernel image size from a double swap should be limited to a few extra instructions, and the runtime overhead should be negligible compared to the bus access.

Quoted 2 lines

Quoted 2 lines

Fortunately we can avoid that by defining our own
mmio_{read,write}{16,32}be()...

Makes sense.

Quoted 7 lines · Greg Ungerer

Quoted 6 lines · Arnd Bergmann

Quoted 3 lines · Geert Uytterhoeven

Orthogonal to how Coldfire's read[wl]() should be fixed, I find it a bit questionable to swap data twice on big endian architectures.

Quoted 6 lines

Quoted 6 lines

I would expect that the compiler is capable of detecting a double swap and optimize it out. Even if it can't, there are not that many instances of io{read,write}{16,32}be in the kernel, so the increase in kernel image size from a double swap should be limited to a few extra instructions, and the runtime overhead should be negligible compared to the bus access.

Probably the overhead is not negligible on old m68k...