Re: [PATCH v2 1/3] ata: pata_falcon: fix IO base selection for Q40

[Michael Schmitz <schmitzmic@gmail.com>]

Hi Finn,

Am 18.08.2023 um 19:52 schrieb Finn Thain:
>
> On Fri, 18 Aug 2023, 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 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@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>
>
> Cc: stable@vger.kernel.org
>
> From Documentation/process/submitting-patches.rst --
> "Note: Attaching a Fixes: tag does not subvert the stable kernel rules
> process nor the requirement to Cc: stable@vger.kernel.org on all stable
> patch candidates... "

OK, I'll add the Cc: back then.

Thanks,

	Michael

>
>> Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>
>>
>> ---
>>
>> Changes from v1:
>>
>> Damien Le Moal:
>> - change patch title
>> - drop stable backport tag
>>
>> 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) {
>>