Re: [PATCH v3 2/2] m68k: mmu: fix IO access endianness for ColdFire family

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Angelo,

On 06/03/18 22:56, Greg Ungerer wrote:
On 06/03/18 09:13, Angelo Dureghello wrote:
On Mon, Mar 05, 2018 at 11:30:32PM +1000, Greg Ungerer wrote:
On 02/03/18 22:24, Angelo Dureghello wrote:
On Fri, Mar 02, 2018 at 09:28:57AM +1000, Greg Ungerer wrote:
On 01/03/18 22:32, Angelo Dureghello wrote:
This patch fixes IO access endianness, that should be big endian.
If any little endian peripheral may be connected, bytes should
be swapped in hardware.

I am not sure I follow your meaning here. In many cases peripheral
devices will be hooked up and they are inherently little endian
but there is no hardware in between that will translate the endian.
It is just something that has to be dealt with at the software driver
level.

So do you need to make this change for specific devices you are
working with?


Ok, well, i add some story to this patch. I was stuck from months
since  kernel was hanging silently when SPI driver was enabled in
conjunction with mmu enbabled (no issues in spi + nommu).
After hard debugging, i finally found the issue to be related to
a wrong ioremap, and, together (so a double issue), peripheral
registers was wrongly accessed as "little" endian.

This was resulting in never getting any reply from SPI device, and
kernel hanging silently in a wait loop. So, in mmu mode (io_mm.h),
the mcf5441x IO peripheral address area (0xe0000000 to 0xffffffff)
needs to be accessed as "big endian".

That kind of makes sense, the m68k/ColdFire is big-endian. And looking
at the IO access for the non-MMU case that is what it does - just direct
access. (So in other words they are native endianess access).


So to have mmu working on mcf5441x both patches are needed.

As suggester by Geert, i isolated ioread/write to be big endian
for mcf5441x *only*.

That for me is a big red flag. The 5441x is not special, so to need
to have something specific for it doesn't really make sense.



With MMU we have 5441x, 5445x, 547x and 548x. If i am not wrong,
5441x is the only family without PCI, so this was the reason why
i isolated it, since looks like PCI access must be "litte endian".

The presence of a PCI bus in this case is a total red herring though.
All the builtin peripherals are native endian - and that is the same
across all ColdFire.


[snip]
But that is probably a moot point here, because you are talking about
the SPI module that is part of the 5441x SoC right?

Yes.

Its bus interface can't be changed. And of course it works when
configured non-MMU.

I would think that perhaps making then readl/writel native endian for
all ColdFire builds (like non-MMU) makes more sense - its internal
peripherals will certainly always be that.

A consolidation of the all io access functions/macros so that ColdFire
used the same definitions (irrespective of MMU/non-MMU) would make
sense. That could lead to fixing PCI access in all modes as well.


Ok. So if i understand i could create an include io_cf.h (mmu + nommu)
or similar name, with raw io access always as big endian for all. But
how to manage the PCI accesses, where the access shoud be little endian ?

We already have raw access routines that are native endian (so big-endian
for us) in arch/m68k/include/asm/raw_io.h. And those are used by the
timer support code (in arch/m68k/coldfire/) and this has sort of
hidden the problem that you have now found. So that is not really
the answer here - you probably won't be able to change the readl/write/etc
used by the SPI driver.

What your are seeing also exposes a problem with the current PCI
access code for ColdFire. It just treats all readl/writel/etc as
little endian - which you can see is not true for internal peripherals.

The readl/writel/etc functions could check the physical access address
and do the right thing (so if in the PCI mapped range then byte swap,
otherwise don't. That feels like it might be the best solution here.

Let me think this over some more...

I haven't forgotten about this. I have been trying a few different
approaches to get everything to work together (MMU, non-MMU and PCI).
I am close, I should have something in the next week or so for review.

Regards
Greg



Then btw, i can olny test the change on mcf5441x.



I can modify that comment if it results wrong or too hard.

I have my system back working now with new drivers and mmu enabled.

(P.S.; i remember you was using an elf toolchain for mmu, do you have
any link in case ? :)

Oh, yeah, this is what I use:

   https://sourceforge.net/projects/uclinux/files/Tools/m68k-linux-20160822/


Many thanks, really.
 
Regards
Greg


Regards,
Angelo



I don't see that the M5441x parts are really special or different
from any of the other ColdFire (or even traditional m68k) devices
we currently support.

Regards
Greg



Regards,
Angelo

---
Changes from v2:
- patch reduced form 3/3 to 2/2
- isolated big endian IO read/write[w,l] to mcf5441x

Signed-off-by: Angelo Dureghello <angelo@xxxxxxxx>
---
   arch/m68k/include/asm/io_mm.h | 16 ++++++++++++++++
   1 file changed, 16 insertions(+)

diff --git a/arch/m68k/include/asm/io_mm.h b/arch/m68k/include/asm/io_mm.h
index ed5333e87879..a1a0f870b61b 100644
--- a/arch/m68k/include/asm/io_mm.h
+++ b/arch/m68k/include/asm/io_mm.h
@@ -444,13 +444,29 @@ static inline void isa_delay(void)
    */
   #define readb(addr)      in_8(addr)
   #define writeb(val,addr) out_8((addr),(val))
+#ifdef CONFIG_M5441x
+/*
+ * mcf5441x only accesses IO/peripheral internal memory.
+ */
+#define readw(addr)        in_be16(addr)
+#define writew(val, addr)    out_be16((addr), (val))
+#else
   #define readw(addr)      in_le16(addr)
   #define writew(val,addr) out_le16((addr),(val))
+#endif
   #endif /* !CONFIG_ISA && !CONFIG_ATARI_ROM_ISA */
+#ifdef CONFIG_M5441x
+/*
+ * mcf5441x only accesses IO/peripheral internal memory.
+ */
+#define readl(addr)        in_be32(addr)
+#define writel(val, addr)    out_be32((addr), (val))
+#else
   #define readl(addr)      in_le32(addr)
   #define writel(val,addr) out_le32((addr),(val))
+#endif
   #define readsb(port, buf, nr)     raw_insb((port), (u8 *)(buf), (nr))
   #define readsw(port, buf, nr)     raw_insw((port), (u16 *)(buf), (nr))


-- 
To unsubscribe from this list: send the line "unsubscribe linux-m68k" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html





--
To unsubscribe from this list: send the line "unsubscribe linux-m68k" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Video for Linux]     [Yosemite News]     [Linux S/390]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux