On 2023/08/18 7:12, Michael Schmitz wrote:
> With commit 44b1fbc0f5f3 ("m68k/q40: Replace q40ide driver
> with pata_falcon and falconide"), the Q40 IDE driver was
> replaced by pata_falcon.c.
Please change the patch title to:
ata: pata_falcon: fix IO base selection for Q40
>
> 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> # 5.14
5.14+ ? But I do not think you need to specify anything anyway since you have
the Fixes tag.
> Cc: Finn Thain <fthain@xxxxxxxxxxxxxx>
> Cc: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> Signed-off-by: Michael Schmitz <schmitzmic@xxxxxxxxx>
>
> ---
>
> Changes from RFC v3:
>
> - split off byte swap option into separate patch
>
> Geert Uytterhoeven:
> - review comments
>
> Changes from RFC v2:
> - add driver parameter 'data_swap' as bit mask for drives to swap
>
> Changes from RFC v1:
>
> Finn Thain:
> - take care to supply IO address suitable for ioread8/iowrite8
> - use MMIO address for data transfer
> ---
> drivers/ata/pata_falcon.c | 55 ++++++++++++++++++++++++---------------
> 1 file changed, 34 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/ata/pata_falcon.c b/drivers/ata/pata_falcon.c
> index 996516e64f13..346259e3bbc8 100644
> --- a/drivers/ata/pata_falcon.c
> +++ b/drivers/ata/pata_falcon.c
> @@ -123,8 +123,8 @@ static int __init pata_falcon_init_one(struct platform_device *pdev)
> struct resource *base_res, *ctl_res, *irq_res;
> struct ata_host *host;
> struct ata_port *ap;
> - void __iomem *base;
> - int irq = 0;
> + void __iomem *base, *ctl_base;
> + int irq = 0, io_offset = 1, reg_scale = 4;
>
> dev_info(&pdev->dev, "Atari Falcon and Q40/Q60 PATA controller\n");
>
> @@ -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);
> + } else {
> + base = (void __iomem *)base_mem_res->start;
> + ctl_base = (void __iomem *)ctl_mem_res->start;
> +
> + ata_port_desc(ap, "cmd %pa ctl %pa",
> + &base_mem_res->start,
> + &ctl_mem_res->start);
> + }
> +
> + ap->ioaddr.error_addr = base + io_offset + 1 * reg_scale;
> + ap->ioaddr.feature_addr = base + io_offset + 1 * reg_scale;
> + ap->ioaddr.nsect_addr = base + io_offset + 2 * reg_scale;
> + ap->ioaddr.lbal_addr = base + io_offset + 3 * reg_scale;
> + ap->ioaddr.lbam_addr = base + io_offset + 4 * reg_scale;
> + ap->ioaddr.lbah_addr = base + io_offset + 5 * reg_scale;
> + ap->ioaddr.device_addr = base + io_offset + 6 * reg_scale;
> + ap->ioaddr.status_addr = base + io_offset + 7 * reg_scale;
> + ap->ioaddr.command_addr = base + io_offset + 7 * reg_scale;
> +
> + ap->ioaddr.altstatus_addr = ctl_base + io_offset;
> + ap->ioaddr.ctl_addr = ctl_base + io_offset;
>
> irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> if (irq_res && irq_res->start > 0) {
--
Damien Le Moal
Western Digital Research
