Hi Finn, William,
On 23/07/23 21:59, Finn Thain wrote:
On Sun, 23 Jul 2023, Geert Uytterhoeven wrote:
First the correct ISA port address (0x3f6) is translated to the
correct MMIO address (0xff400000 + 4 * 0x3f6 = 0xff400fd8). This is
done when the platform device is declared in arch/m68k/q40/config.c
around line 288.
Then this address is passed to pata_falcon which computes the correct
MMIO addresses for the ATA task file registers in
drivers/ata/pata_falcon.c around line 168 (ap->ioaddr.altstatus_addr =
0xff400fd8 + 1 = 0xff400fd9)
The access to the hardware registers is performed in
drivers/ata/libata-sff.c which uses ioread8/iowrite8. These functions
are defined in lib/iomap.c. These functions look at the address passed
it, determine that it is an MMIO address, and pass it to readb/writeb.
This is the first error, we actually want to do an ISA I/O cycle, not
Thanks for testing the function of Q40 IDE after rebuilding a Q40!
I was left with the impression that the Q40 does not actually have the
IDE controller on the ISA bus. The address translation in io_mm.h
assumes IDE on Q40 works through MMIO on the 0xff400000 IO window. This
also assumes that addresses passed to the m68k IO primitives must be IO
port addresses (< 0x1000 or so).
If that assumption is incorrect, the address translation in io_mm.h must
be changed.
memory cycle, but being passed a pre-translated address confuses these
two functions.
Doesn't seem to do that for my Falcon - the addresses passed are all
from the 0xffxxxxxx range and get passed straight through to the IDE
controller. But there's no translation in use for IDE there.
arch/m68k/include/asm/io_mm.h defines inb/outb/readb/writeb etc. They
translate the provided address into the MMIO address in the Q40s
physical address space and then perform the MMIO access. This is
where the second, unnecessary, translation takes place, and the
resulting address is wrong: (0xff800000 + 1 + 4 * 0xff400fd9) &
0xffffffff = 0xfc803f65 -- and this is the address accessed when we
get the oops.
Could be related to the bug that Michael tackled here?
https://lore.kernel.org/linux-m68k/1623290683-17859-1-git-send-email-schmitzmic@xxxxxxxxx/
That patch just tried to make sure the address translation and IO
primitives are selected based on the machine (and ISA bus) type. It
didn't change the Q40 address translations.
The problem appears to be that pata_falcon.c used addresses from the
MMIO range for both IO and memory cycles (there's no difference on
Falcon - the address translation in use for IDE is an identity mapping).
On Q40, the addresses set up in pata_falcon_init_one() ought to be IO
port addresses instead, as long as Q40 address translations then map
these into the correct IO or memory access windows. But pata_falcon
takes the addresses to use from resources defined at platform init time,
so that won't work anymore. I rather think the second patch in this
context is to blame:
Looks like something was missed in commit 44b1fbc0f5f30e66 ("m68k/q40:
Replace q40ide driver with pata_falcon and falconide") in v5.14. Before,
Q40 used its own IDE driver (q40ide, CONFIG_BLK_DEV_Q40IDE).
Could be that too.
Yep - what's been missed is that the Q40 address translation functions
Q40_ISA_IO_B() and Q40_ISA_IO_W() were only used to set up the register
addresses in the old Q40 driver. My intention with that patch was to
only use the new IO resources for the purpose of request_region(), but
at least (!) since converting to the pata_falcon driver, they are now
also used for the unwanted second translation step.
Can you try changing Q40_ISA_IO_B() and Q40_ISA_IO_W() to identity
mappings, so only one set of translations takes place?
It might be a good idea to verify that IDE works in v5.13
Yes, please do.
And please also verify it's broken after 44b1fbc0f5f30e66 was applied.
Cheers,
Michael
