Hi Michael,
On Tue, Aug 22, 2023 at 1:57 AM Michael Schmitz <schmitzmic@xxxxxxxxx> wrote:
> On 21/08/23 19:50, Geert Uytterhoeven wrote:
> > On Sat, Aug 19, 2023 at 1:49 AM Michael Schmitz <schmitzmic@xxxxxxxxx> wrote:
> >> With commit 44b1fbc0f5f3 ("m68k/q40: Replace q40ide driver
> >> with pata_falcon and falconide"), the Q40 IDE driver was
> >> replaced by pata_falcon.c.
> >>
> >> Both IO and memory resources were defined for the Q40 IDE
> >> platform device, but definition of the IDE register addresses
> >> was modeled after the Falcon case, both in use of the memory
> >> resources and in including register scale and byte vs. word
> >> offset in the address.
> >>
> >> This was correct for the Falcon case, which does not apply
> >> any address translation to the register addresses. In the
> >> Q40 case, all of device base address, byte access offset
> >> and register scaling is included in the platform specific
> >> ISA access translation (in asm/mm_io.h).
> >>
> >> As a consequence, such address translation gets applied
> >> twice, and register addresses are mangled.
> >>
> >> Use the device base address from the platform IO resource,
> >> and use standard register offsets from that base in order
> >> to calculate register addresses (the IO address translation
> >> will then apply the correct ISA window base and scaling).
> >>
> >> Encode PIO_OFFSET into IO port addresses for all registers
> >> except the data transfer register. Encode the MMIO offset
> >> there (pata_falcon_data_xfer() directly uses raw IO with
> >> no address translation).
> >>
> >> Reported-by: William R Sowerbutts <will@xxxxxxxxxxxxxx>
> >> Closes: https://lore.kernel.org/r/CAMuHMdUU62jjunJh9cqSqHT87B0H0A4udOOPs=WN7WZKpcagVA@xxxxxxxxxxxxxx
> >> Link: https://lore.kernel.org/r/CAMuHMdUU62jjunJh9cqSqHT87B0H0A4udOOPs=WN7WZKpcagVA@xxxxxxxxxxxxxx
> >> Fixes: 44b1fbc0f5f3 ("m68k/q40: Replace q40ide driver with pata_falcon and falconide")
> >> Cc: stable@xxxxxxxxxxxxxxx
> >> Cc: Finn Thain <fthain@xxxxxxxxxxxxxx>
> >> Cc: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> >> Tested-by: William R Sowerbutts <will@xxxxxxxxxxxxxx>
> >> Signed-off-by: Michael Schmitz <schmitzmic@xxxxxxxxx>
> > Thanks for the update!
> >
> >> --- a/drivers/ata/pata_falcon.c
> >> +++ b/drivers/ata/pata_falcon.c
> >> @@ -165,26 +165,39 @@ static int __init pata_falcon_init_one(struct platform_device *pdev)
> >> ap->pio_mask = ATA_PIO4;
> >> ap->flags |= ATA_FLAG_SLAVE_POSS | ATA_FLAG_NO_IORDY;
> >>
> >> - base = (void __iomem *)base_mem_res->start;
> >> /* N.B. this assumes data_addr will be used for word-sized I/O only */
> >> - ap->ioaddr.data_addr = base + 0 + 0 * 4;
> >> - ap->ioaddr.error_addr = base + 1 + 1 * 4;
> >> - ap->ioaddr.feature_addr = base + 1 + 1 * 4;
> >> - ap->ioaddr.nsect_addr = base + 1 + 2 * 4;
> >> - ap->ioaddr.lbal_addr = base + 1 + 3 * 4;
> >> - ap->ioaddr.lbam_addr = base + 1 + 4 * 4;
> >> - ap->ioaddr.lbah_addr = base + 1 + 5 * 4;
> >> - ap->ioaddr.device_addr = base + 1 + 6 * 4;
> >> - ap->ioaddr.status_addr = base + 1 + 7 * 4;
> >> - ap->ioaddr.command_addr = base + 1 + 7 * 4;
> >> -
> >> - base = (void __iomem *)ctl_mem_res->start;
> >> - ap->ioaddr.altstatus_addr = base + 1;
> >> - ap->ioaddr.ctl_addr = base + 1;
> >> -
> >> - ata_port_desc(ap, "cmd 0x%lx ctl 0x%lx",
> >> - (unsigned long)base_mem_res->start,
> >> - (unsigned long)ctl_mem_res->start);
> >> + ap->ioaddr.data_addr = (void __iomem *)base_mem_res->start;
> >> +
> >> + if (base_res) { /* only Q40 has IO resources */
> >> + io_offset = 0x10000;
> >> + reg_scale = 1;
> >> + base = (void __iomem *)base_res->start;
> >> + ctl_base = (void __iomem *)ctl_res->start;
> >> +
> >> + ata_port_desc(ap, "cmd %pa ctl %pa",
> >> + &base_res->start,
> >> + &ctl_res->start);
> > This can be moved outside the else, using %px to format base and
> > ctl_base.
>
> Right - do we need some additional message spelling out what address Q40
> uses for data transfers? (Redundant for Falcon, of course ...)
>
> Though that could be handled outside the else, too:
>
> ata_port_desc(ap, "cmd %px ctl %px data %pa",
> base, ctl_base, &ap->ioaddr.data_addr);
I guess that wouldn't hurt.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
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
