Re: Linux 6.4.4 on m68k - Q40 - pata_falcon causes oops at boot time

[Michael Schmitz <schmitzmic@gmail.com>]

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@gmail.com/

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