Re: [PATCH v4 1/2] ata: pata_falcon: fix IO base selection for Q40

[Sergey Shtylyov <s.shtylyov@omp.ru>]

Hello!

   I prefer CCing my OMP account when you send the PATA patches,
as is returned by scripts/get_maintainer.pl...

On 8/23/23 1:13 AM, 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.
> 
> 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 shift 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 shift 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
> for Q40 (the IO address translation will then add the correct
> ISA window base address and byte access offset), with register
> shift 1. Use MMIO base address and register shift 2 as before
> for Falcon.
> 
> Encode PIO_OFFSET into IO port addresses for all registers
> for Q40 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@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: stable@vger.kernel.org
> Cc: Finn Thain <fthain@linux-m68k.org>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Tested-by: William R Sowerbutts <will@sowerbutts.com>
> Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>
> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>

[...]

> diff --git a/drivers/ata/pata_falcon.c b/drivers/ata/pata_falcon.c
> index 996516e64f13..3841ea200bcb 100644
> --- a/drivers/ata/pata_falcon.c
> +++ b/drivers/ata/pata_falcon.c
[...]
> @@ -165,26 +165,34 @@ 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_shift = 0;
> +		base = (void __iomem *)base_res->start;
> +		ctl_base = (void __iomem *)ctl_res->start;
> +	} else {
> +		base = (void __iomem *)base_mem_res->start;
> +		ctl_base = (void __iomem *)ctl_mem_res->start;
> +	}
> +
> +	ap->ioaddr.error_addr	= base + io_offset + (1 << reg_shift);
> +	ap->ioaddr.feature_addr	= base + io_offset + (1 << reg_shift);
> +	ap->ioaddr.nsect_addr	= base + io_offset + (2 << reg_shift);
> +	ap->ioaddr.lbal_addr	= base + io_offset + (3 << reg_shift);
> +	ap->ioaddr.lbam_addr	= base + io_offset + (4 << reg_shift);
> +	ap->ioaddr.lbah_addr	= base + io_offset + (5 << reg_shift);
> +	ap->ioaddr.device_addr	= base + io_offset + (6 << reg_shift);
> +	ap->ioaddr.status_addr	= base + io_offset + (7 << reg_shift);
> +	ap->ioaddr.command_addr	= base + io_offset + (7 << reg_shift);
> +
> +	ap->ioaddr.altstatus_addr	= ctl_base + io_offset;
> +	ap->ioaddr.ctl_addr		= ctl_base + io_offset;
> +
> +	ata_port_desc(ap, "cmd %px ctl %px data %pa",
> +		      base, ctl_base, &ap->ioaddr.data_addr);

   Like Geert said, use "%px" and ap->ioaddr.data_addr here...

[...]

MBR, Sergey