Hi Michael,
On Wed, Aug 16, 2023 at 12:32 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 (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@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: Finn Thain <fthain@xxxxxxxxxxxxxx>
Cc: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
Signed-off-by: Michael Schmitz <schmitzmic@xxxxxxxxx>
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@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
