Re: [PATCH RFC v3] m68k/q40: fix IO base selection for Q40 in pata_falcon.c

Geert Uytterhoeven
2023-08-16T08:08:51Z

Hi Michael,

On Wed, Aug 16, 2023 at 12:32 AM Michael Schmitz <schmitzmic@gmail.com> wrote:
> With commit 44b1fbc0f5f3 ("m68k/q40: Replace q40ide driver
> with pata_falcon and falconide"), the Q40 IDE driver was
> replaced by pata_falcon.c (and the later obsoleted
> falconide.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).
>
> Add module parameter 'data_swap' to allow connecting drives
> with non-native data byte order. Drives selected by the
> data_swap bit mask will have their user data swapped to host
> byte order, i.e. 'pata_falcon.data_swap=2' will swap all user
> data on drive B, leaving data on drive A in native order.
>
> Reported-by: William R Sowerbutts <will@sowerbutts.com>
> Closes: https://lore.kernel.org/r/CAMuHMdUU62jjunJh9cqSqHT87B0H0A4udOOPs=WN7WZKpcagVA@mail.gmail.com
> Link: https://lore.kernel.org/r/CAMuHMdUU62jjunJh9cqSqHT87B0H0A4udOOPs=WN7WZKpcagVA@mail.gmail.com
> Fixes: 44b1fbc0f5f3 ("m68k/q40: Replace q40ide driver with pata_falcon and falconide")
> Cc: Finn Thain <fthain@linux-m68k.org>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>

Thanks for your patch!

> --- a/drivers/ata/pata_falcon.c
> +++ b/drivers/ata/pata_falcon.c

> @@ -165,26 +178,61 @@ 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);
> + priv = devm_kzalloc(&pdev->dev,
> + sizeof(struct pata_falcon_priv), GFP_KERNEL);
> +

Please drop this blank line.

> + if (!priv)
> + return -ENOMEM;
> +
> + ap->private_data = priv;
> +
> + priv->swap_mask = pata_falcon_swap_mask;
> + if (priv->swap_mask)
> + priv->swap_data = 1;
> +
> + if (MACH_IS_Q40) {

Please do not use MACH_IS_xx() checks in a modern platform driver.
The proper way is to either pass parameters through platform_data, or
to use a different platform driver name to match against (i.e. populate
platform_driver.id_table with an array containing name/driver_data
pairs).

However, here you can just use the presence or absence
of base_res and ctl_res (which were unused before) to distinguish.

> + base = (void __iomem *)base_mem_res->start;

Any specific reason this is still using base_mem_res and not
base_res?

> + ap->ioaddr.data_addr = base + 0;

This is the same on Q40 and Falcon, so it can be factored out.

> + base = (void __iomem *)base_res->start;
> + ap->ioaddr.error_addr = base + 0x10000 + 1;
> + ap->ioaddr.feature_addr = base + 0x10000 + 1;
> + ap->ioaddr.nsect_addr = base + 0x10000 + 2;
> + ap->ioaddr.lbal_addr = base + 0x10000 + 3;
> + ap->ioaddr.lbam_addr = base + 0x10000 + 4;
> + ap->ioaddr.lbah_addr = base + 0x10000 + 5;
> + ap->ioaddr.device_addr = base + 0x10000 + 6;
> + ap->ioaddr.status_addr = base + 0x10000 + 7;
> + ap->ioaddr.command_addr = base + 0x10000 + 7;

Compared to Falcon, different base, banksize, and regsize, so e.g.

ap->ioaddr.error_addr = base + 1 * banksize + 1 * regsize;

> +
> + base = (void __iomem *)ctl_res->start;
> + ap->ioaddr.altstatus_addr = base + 0x10000;
> + ap->ioaddr.ctl_addr = base + 0x10000;
> +
> + ata_port_desc(ap, "cmd 0x%lx ctl 0x%lx",
> + (unsigned long)base_res->start,
> + (unsigned long)ctl_res->start);

%pa and e.g. &base_res->start to avoid the cast.

> + } else {
> + 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);
> + }
>
> irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> if (irq_res && irq_res->start > 0) {

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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