[PATCH 0/3] Q40 IDE fixes

[PATCH 0/3] Q40 IDE fixes

[Michael Schmitz]

The pata_falcon IDE driver for m68k Q40 has been broken since Q40
was converted to use pata_falcon in preparation for removal of the
legacy IDE core in v5.14. A bug has only recently been reported.

This patch series to fix the bug has seen some discussion and review
on linux-m68k in the past week. It's also been tested on both Q40
and Atari.

The bug reporter has since found that he can use pata_legacy for
his Q40, but that is solely due to his disk written in the default
IDE byte order. Other users of Q40 will have data on disk in
big-endian order, and will have to use the pata_falcon driver
which does not attempt byte swapping to host byte order in its
data_xfer() function.

Patch 1 fixes the bug introduced in v5.14. That one ought not
to be controversial (I hope).

Patch 2 adds a module parameter to select drives to have byte
swapping to host byte order applied (useful for connecting drives
in IDE default byte order to Q40 and Atari for interoperability).

Patch 3 changes the Atari Falcon IDE platform device to use a
device ID compatible with the byte swapping scheme. This patch
should go through the m68k tree, not IDE.

Feedback welcome!

Cheers,

   Michael

[PATCH 1/3] m68k/q40: fix IO base selection for Q40 in pata_falcon.c

[Michael Schmitz]

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: <stable@vger.kernel.org> # 5.14
Cc: Finn Thain <fthain@linux-m68k.org>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>

---

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) {
-- 
2.17.1

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

[Damien Le Moal]

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@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> # 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@linux-m68k.org>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>
> 
> ---
> 
> 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

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

[Michael Schmitz]

Hi Damien,

thanks for your review!

Am 18.08.2023 um 12:42 schrieb Damien Le Moal:
> 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

Will do.

>
>>
>> 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: <stable@vger.kernel.org> # 5.14
>
> 5.14+ ? But I do not think you need to specify anything anyway since you have
> the Fixes tag.

5.14+ perhaps. I'll check the docs again to see whether Fixes: obsoletes 
the stable backport tag. I've so far used both together...

Cheers,

	Michael


>
>> Cc: Finn Thain <fthain@linux-m68k.org>
>> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
>> Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>
>>
>> ---
>>
>> 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) {
>

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

[Finn Thain]

On Fri, 18 Aug 2023, Michael Schmitz wrote:

> >> Cc: <stable@vger.kernel.org> # 5.14
> >
> > 5.14+ ? But I do not think you need to specify anything anyway since 
> > you have the Fixes tag.
> 
> 5.14+ perhaps. I'll check the docs again to see whether Fixes: obsoletes 
> the stable backport tag. I've so far used both together...
> 

You'd specify a "# x.y+" limit along with a "Fixes" tag if you don't want 
to backport as far back as the buggy commit (because some other 
pre-requisite isn't present on the older branches). But that does not 
apply in this case.

Writing "# 5.14" is surprising because (according to www.kernel.org 
landing page) that branch was abandoned, and no live branch was named. 
But in "git log" you can see that people write this anyway.

Writing "# 5.14+" or "# 5.15+" is clear enough but is normally omitted 
when it can be inferred from the Fixes tag. That's been my experience, at 
least.

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

[Sergey Shtylyov]

Hello!

On 8/18/23 1:12 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 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: <stable@vger.kernel.org> # 5.14
> Cc: Finn Thain <fthain@linux-m68k.org>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> 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..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;

   Maybe reg_step?

[...]

MBR, Sergey

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

[Michael Schmitz]

Hi Sergey,

thanks for your review!

On 20/08/23 08:29, Sergey Shtylyov wrote:
> Hello!
>
> On 8/18/23 1:12 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 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: <stable@vger.kernel.org> # 5.14
>> Cc: Finn Thain <fthain@linux-m68k.org>
>> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
>> 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..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;
>     Maybe reg_step?

Could name it that, too. I can't recall where I picked up the term 
'register scaling'...

I'll see what's the consensus (if any) in drivers/.

Cheers,

     Michael


>
> [...]
>
> MBR, Sergey

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

[Michael Schmitz]

Hi Sergey,

Am 21.08.2023 um 07:19 schrieb Michael Schmitz:
>>> 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;
>>     Maybe reg_step?
>
> Could name it that, too. I can't recall where I picked up the term
> 'register scaling'...
>
> I'll see what's the consensus (if any) in drivers/.

I've seen some use of 'step' but mostly use of 'shift'.

Rewriting pata_falcon_init_one() to use register shift instead of 
register step is trivial, so unless anyone objects, I'll send that 
version as v4.

Cheers,

	Michael

>
> Cheers,
>
>     Michael
>
>
>>
>> [...]
>>
>> MBR, Sergey

[PATCH 2/3] m68k/q40: add data_swab option for pata_falcon to byte-swap disk data

[Michael Schmitz]

Some users of pata_falcon on Q40 have IDE disks in default
IDE little endian byte order, whereas legacy disks use
host-native big-endian byte order as on the Atari Falcon.

Add module parameter 'data_swab' to allow connecting drives
with non-native data byte order. Drives selected by the
data_swap bit mask will have their user data byte-swapped to
host byte order, i.e. 'pata_falcon.data_swab=2' will byte-swap
all user data on drive B, leaving data on drive A in native
byte order. On Q40, drives on a second IDE interface may be
added to the bit mask as bits 2 and 3.

Default setting is no byte swapping, i.e. compatibility with
the native Falcon or Q40 operating system disk format.

Cc: William R Sowerbutts <will@sowerbutts.com>
Cc: Finn Thain <fthain@linux-m68k.org>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>

---

Changes since RFC v4:

Geert Uytterhoeven:
- don't shift static module parameter for drive 3/4 bitmask
- simplify bit mask calculation to always use pdev->id

Finn Thain:
- correct bit numbers for drive 3/4

Changes since RFC v3:

- split off this byte swap handling into separate patch

- add hint regarding third and fourth drive on Q40

Finn Thain:
- rename module parameter to 'data_swab' to better reflect its use

William Sowerbutts:
- correct IDE drive number used in data swap conditional
---
 drivers/ata/pata_falcon.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/ata/pata_falcon.c b/drivers/ata/pata_falcon.c
index 346259e3bbc8..90488f565d6f 100644
--- a/drivers/ata/pata_falcon.c
+++ b/drivers/ata/pata_falcon.c
@@ -33,6 +33,16 @@
 #define DRV_NAME "pata_falcon"
 #define DRV_VERSION "0.1.0"
 
+static int pata_falcon_swap_mask;
+
+module_param_named(data_swab, pata_falcon_swap_mask, int, 0444);
+MODULE_PARM_DESC(data_swab, "Data byte swap enable/disable bitmap (0x1==drive1, 0x2==drive2, 0x4==drive3, 0x8==drive4, default==0)");
+
+struct pata_falcon_priv {
+	unsigned int swap_mask;
+	bool swap_data;
+};
+
 static const struct scsi_host_template pata_falcon_sht = {
 	ATA_PIO_SHT(DRV_NAME),
 };
@@ -44,13 +54,15 @@ static unsigned int pata_falcon_data_xfer(struct ata_queued_cmd *qc,
 	struct ata_device *dev = qc->dev;
 	struct ata_port *ap = dev->link->ap;
 	void __iomem *data_addr = ap->ioaddr.data_addr;
+	struct pata_falcon_priv *priv = ap->private_data;
 	unsigned int words = buflen >> 1;
 	struct scsi_cmnd *cmd = qc->scsicmd;
+	int dev_id = dev->devno;
 	bool swap = 1;
 
 	if (dev->class == ATA_DEV_ATA && cmd &&
 	    !blk_rq_is_passthrough(scsi_cmd_to_rq(cmd)))
-		swap = 0;
+		swap = priv->swap_data && (priv->swap_mask & BIT(dev_id));
 
 	/* Transfer multiple of 2 bytes */
 	if (rw == READ) {
@@ -123,6 +135,7 @@ 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;
+	struct pata_falcon_priv *priv;
 	void __iomem *base, *ctl_base;
 	int irq = 0, io_offset = 1, reg_scale = 4;
 
@@ -165,6 +178,13 @@ 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;
 
+	priv = devm_kzalloc(&pdev->dev,
+		sizeof(struct pata_falcon_priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	ap->private_data = priv;
+
 	/* N.B. this assumes data_addr will be used for word-sized I/O only */
 	ap->ioaddr.data_addr = (void __iomem *)base_mem_res->start;
 
@@ -199,6 +219,10 @@ static int __init pata_falcon_init_one(struct platform_device *pdev)
 	ap->ioaddr.altstatus_addr	= ctl_base + io_offset;
 	ap->ioaddr.ctl_addr		= ctl_base + io_offset;
 
+	priv->swap_mask = pata_falcon_swap_mask >> (2 * pdev->id);
+	if (priv->swap_mask)
+		priv->swap_data = 1;
+
 	irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
 	if (irq_res && irq_res->start > 0) {
 		irq = irq_res->start;
-- 
2.17.1

Re: [PATCH 2/3] m68k/q40: add data_swab option for pata_falcon to byte-swap disk data

[Damien Le Moal]

On 2023/08/18 7:12, Michael Schmitz wrote:
> Some users of pata_falcon on Q40 have IDE disks in default
> IDE little endian byte order, whereas legacy disks use
> host-native big-endian byte order as on the Atari Falcon.
> 
> Add module parameter 'data_swab' to allow connecting drives
> with non-native data byte order. Drives selected by the
> data_swap bit mask will have their user data byte-swapped to
> host byte order, i.e. 'pata_falcon.data_swab=2' will byte-swap
> all user data on drive B, leaving data on drive A in native
> byte order. On Q40, drives on a second IDE interface may be
> added to the bit mask as bits 2 and 3.
> 
> Default setting is no byte swapping, i.e. compatibility with
> the native Falcon or Q40 operating system disk format.
> 
> Cc: William R Sowerbutts <will@sowerbutts.com>
> Cc: Finn Thain <fthain@linux-m68k.org>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>
> 
> ---
> 
> Changes since RFC v4:
> 
> Geert Uytterhoeven:
> - don't shift static module parameter for drive 3/4 bitmask
> - simplify bit mask calculation to always use pdev->id
> 
> Finn Thain:
> - correct bit numbers for drive 3/4
> 
> Changes since RFC v3:
> 
> - split off this byte swap handling into separate patch
> 
> - add hint regarding third and fourth drive on Q40
> 
> Finn Thain:
> - rename module parameter to 'data_swab' to better reflect its use
> 
> William Sowerbutts:
> - correct IDE drive number used in data swap conditional
> ---
>  drivers/ata/pata_falcon.c | 26 +++++++++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/ata/pata_falcon.c b/drivers/ata/pata_falcon.c
> index 346259e3bbc8..90488f565d6f 100644
> --- a/drivers/ata/pata_falcon.c
> +++ b/drivers/ata/pata_falcon.c
> @@ -33,6 +33,16 @@
>  #define DRV_NAME "pata_falcon"
>  #define DRV_VERSION "0.1.0"
>  
> +static int pata_falcon_swap_mask;
> +
> +module_param_named(data_swab, pata_falcon_swap_mask, int, 0444);
> +MODULE_PARM_DESC(data_swab, "Data byte swap enable/disable bitmap (0x1==drive1, 0x2==drive2, 0x4==drive3, 0x8==drive4, default==0)");
> +
> +struct pata_falcon_priv {
> +	unsigned int swap_mask;
> +	bool swap_data;
> +};
> +
>  static const struct scsi_host_template pata_falcon_sht = {
>  	ATA_PIO_SHT(DRV_NAME),
>  };
> @@ -44,13 +54,15 @@ static unsigned int pata_falcon_data_xfer(struct ata_queued_cmd *qc,
>  	struct ata_device *dev = qc->dev;
>  	struct ata_port *ap = dev->link->ap;
>  	void __iomem *data_addr = ap->ioaddr.data_addr;
> +	struct pata_falcon_priv *priv = ap->private_data;
>  	unsigned int words = buflen >> 1;
>  	struct scsi_cmnd *cmd = qc->scsicmd;
> +	int dev_id = dev->devno;
>  	bool swap = 1;
>  
>  	if (dev->class == ATA_DEV_ATA && cmd &&
>  	    !blk_rq_is_passthrough(scsi_cmd_to_rq(cmd)))
> -		swap = 0;
> +		swap = priv->swap_data && (priv->swap_mask & BIT(dev_id));
>  
>  	/* Transfer multiple of 2 bytes */
>  	if (rw == READ) {
> @@ -123,6 +135,7 @@ 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;
> +	struct pata_falcon_priv *priv;
>  	void __iomem *base, *ctl_base;
>  	int irq = 0, io_offset = 1, reg_scale = 4;
>  
> @@ -165,6 +178,13 @@ 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;
>  
> +	priv = devm_kzalloc(&pdev->dev,
> +		sizeof(struct pata_falcon_priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	ap->private_data = priv;
> +
>  	/* N.B. this assumes data_addr will be used for word-sized I/O only */
>  	ap->ioaddr.data_addr = (void __iomem *)base_mem_res->start;
>  
> @@ -199,6 +219,10 @@ static int __init pata_falcon_init_one(struct platform_device *pdev)
>  	ap->ioaddr.altstatus_addr	= ctl_base + io_offset;
>  	ap->ioaddr.ctl_addr		= ctl_base + io_offset;
>  
> +	priv->swap_mask = pata_falcon_swap_mask >> (2 * pdev->id);

I do not understand the shift here. It seems that it will lead to
priv->swap_mask always being 0...

> +	if (priv->swap_mask)
> +		priv->swap_data = 1;

I do not understand why priv->swap_data is needed, given that it is set to 1 if
priv->swap_mask != 0, the above:

	swap = priv->swap_data && (priv->swap_mask & BIT(dev_id));

should be equivalent to the simpler:

	swap = priv->swap_mask & BIT(dev_id);

No ? Am I missing something ?

> +
>  	irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>  	if (irq_res && irq_res->start > 0) {
>  		irq = irq_res->start;

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 2/3] m68k/q40: add data_swab option for pata_falcon to byte-swap disk data

[Michael Schmitz]

Hi Damien,

thanks for the review ...

Am 18.08.2023 um 12:51 schrieb Damien Le Moal:
> On 2023/08/18 7:12, Michael Schmitz wrote:
>> Some users of pata_falcon on Q40 have IDE disks in default
>> IDE little endian byte order, whereas legacy disks use
>> host-native big-endian byte order as on the Atari Falcon.
>>
>> Add module parameter 'data_swab' to allow connecting drives
>> with non-native data byte order. Drives selected by the
>> data_swap bit mask will have their user data byte-swapped to
>> host byte order, i.e. 'pata_falcon.data_swab=2' will byte-swap
>> all user data on drive B, leaving data on drive A in native
>> byte order. On Q40, drives on a second IDE interface may be
>> added to the bit mask as bits 2 and 3.
>>
>> Default setting is no byte swapping, i.e. compatibility with
>> the native Falcon or Q40 operating system disk format.
>>
>> Cc: William R Sowerbutts <will@sowerbutts.com>
>> Cc: Finn Thain <fthain@linux-m68k.org>
>> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
>> Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>
>>
>> ---
>>
>> Changes since RFC v4:
>>
>> Geert Uytterhoeven:
>> - don't shift static module parameter for drive 3/4 bitmask
>> - simplify bit mask calculation to always use pdev->id
>>
>> Finn Thain:
>> - correct bit numbers for drive 3/4
>>
>> Changes since RFC v3:
>>
>> - split off this byte swap handling into separate patch
>>
>> - add hint regarding third and fourth drive on Q40
>>
>> Finn Thain:
>> - rename module parameter to 'data_swab' to better reflect its use
>>
>> William Sowerbutts:
>> - correct IDE drive number used in data swap conditional
>> ---
>>  drivers/ata/pata_falcon.c | 26 +++++++++++++++++++++++++-
>>  1 file changed, 25 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/ata/pata_falcon.c b/drivers/ata/pata_falcon.c
>> index 346259e3bbc8..90488f565d6f 100644
>> --- a/drivers/ata/pata_falcon.c
>> +++ b/drivers/ata/pata_falcon.c
>> @@ -33,6 +33,16 @@
>>  #define DRV_NAME "pata_falcon"
>>  #define DRV_VERSION "0.1.0"
>>
>> +static int pata_falcon_swap_mask;
>> +
>> +module_param_named(data_swab, pata_falcon_swap_mask, int, 0444);
>> +MODULE_PARM_DESC(data_swab, "Data byte swap enable/disable bitmap (0x1==drive1, 0x2==drive2, 0x4==drive3, 0x8==drive4, default==0)");
>> +
>> +struct pata_falcon_priv {
>> +	unsigned int swap_mask;
>> +	bool swap_data;
>> +};
>> +
>>  static const struct scsi_host_template pata_falcon_sht = {
>>  	ATA_PIO_SHT(DRV_NAME),
>>  };
>> @@ -44,13 +54,15 @@ static unsigned int pata_falcon_data_xfer(struct ata_queued_cmd *qc,
>>  	struct ata_device *dev = qc->dev;
>>  	struct ata_port *ap = dev->link->ap;
>>  	void __iomem *data_addr = ap->ioaddr.data_addr;
>> +	struct pata_falcon_priv *priv = ap->private_data;
>>  	unsigned int words = buflen >> 1;
>>  	struct scsi_cmnd *cmd = qc->scsicmd;
>> +	int dev_id = dev->devno;
>>  	bool swap = 1;
>>
>>  	if (dev->class == ATA_DEV_ATA && cmd &&
>>  	    !blk_rq_is_passthrough(scsi_cmd_to_rq(cmd)))
>> -		swap = 0;
>> +		swap = priv->swap_data && (priv->swap_mask & BIT(dev_id));
>>
>>  	/* Transfer multiple of 2 bytes */
>>  	if (rw == READ) {
>> @@ -123,6 +135,7 @@ 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;
>> +	struct pata_falcon_priv *priv;
>>  	void __iomem *base, *ctl_base;
>>  	int irq = 0, io_offset = 1, reg_scale = 4;
>>
>> @@ -165,6 +178,13 @@ 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;
>>
>> +	priv = devm_kzalloc(&pdev->dev,
>> +		sizeof(struct pata_falcon_priv), GFP_KERNEL);
>> +	if (!priv)
>> +		return -ENOMEM;
>> +
>> +	ap->private_data = priv;
>> +
>>  	/* N.B. this assumes data_addr will be used for word-sized I/O only */
>>  	ap->ioaddr.data_addr = (void __iomem *)base_mem_res->start;
>>
>> @@ -199,6 +219,10 @@ static int __init pata_falcon_init_one(struct platform_device *pdev)
>>  	ap->ioaddr.altstatus_addr	= ctl_base + io_offset;
>>  	ap->ioaddr.ctl_addr		= ctl_base + io_offset;
>>
>> +	priv->swap_mask = pata_falcon_swap_mask >> (2 * pdev->id);
>
> I do not understand the shift here. It seems that it will lead to
> priv->swap_mask always being 0...

On Q40, it is possible to have two ISA IDE adapters, and two platform 
devices are defined (in arch/m68k/q40/config.c) One will have 
pdev->id==0, the other will have pdev->id==1.

In the pdev->id==0 case, there's no shift of the bit mask passed in as 
module parameter, so the data transfer function will examine bits 0 and 
1. That case we've verified in testing.
In the other case, we shift down by two bits, so the data transfer 
function will now examine bits 2 and 3 from the module parameter.

>
>> +	if (priv->swap_mask)
>> +		priv->swap_data = 1;
>
> I do not understand why priv->swap_data is needed, given that it is set to 1 if
> priv->swap_mask != 0, the above:
>
> 	swap = priv->swap_data && (priv->swap_mask & BIT(dev_id));
>
> should be equivalent to the simpler:
>
> 	swap = priv->swap_mask & BIT(dev_id);
>
> No ? Am I missing something ?

I had hoped to avoid the pointer dereference and bit shift in the 
default (and by far most common) case where none of the bits are set.

Compared to the simpler version, I actually just save the bit shift, so 
it's probably a pointless optimization.

Finn had suggested to simplify this even further, and use ap->private as 
bit mask directly (saving the kzalloc()). If you're OK with that, I'll 
change the code accordingly.

Cheers,

	Michael


>> +
>>  	irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>>  	if (irq_res && irq_res->start > 0) {
>>  		irq = irq_res->start;
>

Re: [PATCH 2/3] m68k/q40: add data_swab option for pata_falcon to byte-swap disk data

[Damien Le Moal]

On 2023/08/18 12:08, Michael Schmitz wrote:
> Hi Damien,
> 
> thanks for the review ...
> 
> Am 18.08.2023 um 12:51 schrieb Damien Le Moal:
>> On 2023/08/18 7:12, Michael Schmitz wrote:
>>> Some users of pata_falcon on Q40 have IDE disks in default
>>> IDE little endian byte order, whereas legacy disks use
>>> host-native big-endian byte order as on the Atari Falcon.
>>>
>>> Add module parameter 'data_swab' to allow connecting drives
>>> with non-native data byte order. Drives selected by the
>>> data_swap bit mask will have their user data byte-swapped to
>>> host byte order, i.e. 'pata_falcon.data_swab=2' will byte-swap
>>> all user data on drive B, leaving data on drive A in native
>>> byte order. On Q40, drives on a second IDE interface may be
>>> added to the bit mask as bits 2 and 3.
>>>
>>> Default setting is no byte swapping, i.e. compatibility with
>>> the native Falcon or Q40 operating system disk format.
>>>
>>> Cc: William R Sowerbutts <will@sowerbutts.com>
>>> Cc: Finn Thain <fthain@linux-m68k.org>
>>> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
>>> Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>
>>>
>>> ---
>>>
>>> Changes since RFC v4:
>>>
>>> Geert Uytterhoeven:
>>> - don't shift static module parameter for drive 3/4 bitmask
>>> - simplify bit mask calculation to always use pdev->id
>>>
>>> Finn Thain:
>>> - correct bit numbers for drive 3/4
>>>
>>> Changes since RFC v3:
>>>
>>> - split off this byte swap handling into separate patch
>>>
>>> - add hint regarding third and fourth drive on Q40
>>>
>>> Finn Thain:
>>> - rename module parameter to 'data_swab' to better reflect its use
>>>
>>> William Sowerbutts:
>>> - correct IDE drive number used in data swap conditional
>>> ---
>>>  drivers/ata/pata_falcon.c | 26 +++++++++++++++++++++++++-
>>>  1 file changed, 25 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/ata/pata_falcon.c b/drivers/ata/pata_falcon.c
>>> index 346259e3bbc8..90488f565d6f 100644
>>> --- a/drivers/ata/pata_falcon.c
>>> +++ b/drivers/ata/pata_falcon.c
>>> @@ -33,6 +33,16 @@
>>>  #define DRV_NAME "pata_falcon"
>>>  #define DRV_VERSION "0.1.0"
>>>
>>> +static int pata_falcon_swap_mask;
>>> +
>>> +module_param_named(data_swab, pata_falcon_swap_mask, int, 0444);
>>> +MODULE_PARM_DESC(data_swab, "Data byte swap enable/disable bitmap (0x1==drive1, 0x2==drive2, 0x4==drive3, 0x8==drive4, default==0)");
>>> +
>>> +struct pata_falcon_priv {
>>> +	unsigned int swap_mask;
>>> +	bool swap_data;
>>> +};
>>> +
>>>  static const struct scsi_host_template pata_falcon_sht = {
>>>  	ATA_PIO_SHT(DRV_NAME),
>>>  };
>>> @@ -44,13 +54,15 @@ static unsigned int pata_falcon_data_xfer(struct ata_queued_cmd *qc,
>>>  	struct ata_device *dev = qc->dev;
>>>  	struct ata_port *ap = dev->link->ap;
>>>  	void __iomem *data_addr = ap->ioaddr.data_addr;
>>> +	struct pata_falcon_priv *priv = ap->private_data;
>>>  	unsigned int words = buflen >> 1;
>>>  	struct scsi_cmnd *cmd = qc->scsicmd;
>>> +	int dev_id = dev->devno;
>>>  	bool swap = 1;
>>>
>>>  	if (dev->class == ATA_DEV_ATA && cmd &&
>>>  	    !blk_rq_is_passthrough(scsi_cmd_to_rq(cmd)))
>>> -		swap = 0;
>>> +		swap = priv->swap_data && (priv->swap_mask & BIT(dev_id));
>>>
>>>  	/* Transfer multiple of 2 bytes */
>>>  	if (rw == READ) {
>>> @@ -123,6 +135,7 @@ 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;
>>> +	struct pata_falcon_priv *priv;
>>>  	void __iomem *base, *ctl_base;
>>>  	int irq = 0, io_offset = 1, reg_scale = 4;
>>>
>>> @@ -165,6 +178,13 @@ 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;
>>>
>>> +	priv = devm_kzalloc(&pdev->dev,
>>> +		sizeof(struct pata_falcon_priv), GFP_KERNEL);
>>> +	if (!priv)
>>> +		return -ENOMEM;
>>> +
>>> +	ap->private_data = priv;
>>> +
>>>  	/* N.B. this assumes data_addr will be used for word-sized I/O only */
>>>  	ap->ioaddr.data_addr = (void __iomem *)base_mem_res->start;
>>>
>>> @@ -199,6 +219,10 @@ static int __init pata_falcon_init_one(struct platform_device *pdev)
>>>  	ap->ioaddr.altstatus_addr	= ctl_base + io_offset;
>>>  	ap->ioaddr.ctl_addr		= ctl_base + io_offset;
>>>
>>> +	priv->swap_mask = pata_falcon_swap_mask >> (2 * pdev->id);
>>
>> I do not understand the shift here. It seems that it will lead to
>> priv->swap_mask always being 0...
> 
> On Q40, it is possible to have two ISA IDE adapters, and two platform 
> devices are defined (in arch/m68k/q40/config.c) One will have 
> pdev->id==0, the other will have pdev->id==1.
> 
> In the pdev->id==0 case, there's no shift of the bit mask passed in as 
> module parameter, so the data transfer function will examine bits 0 and 
> 1. That case we've verified in testing.
> In the other case, we shift down by two bits, so the data transfer 
> function will now examine bits 2 and 3 from the module parameter.
> 
>>
>>> +	if (priv->swap_mask)
>>> +		priv->swap_data = 1;
>>
>> I do not understand why priv->swap_data is needed, given that it is set to 1 if
>> priv->swap_mask != 0, the above:
>>
>> 	swap = priv->swap_data && (priv->swap_mask & BIT(dev_id));
>>
>> should be equivalent to the simpler:
>>
>> 	swap = priv->swap_mask & BIT(dev_id);
>>
>> No ? Am I missing something ?
> 
> I had hoped to avoid the pointer dereference and bit shift in the 
> default (and by far most common) case where none of the bits are set.
> 
> Compared to the simpler version, I actually just save the bit shift, so 
> it's probably a pointless optimization.
> 
> Finn had suggested to simplify this even further, and use ap->private as 
> bit mask directly (saving the kzalloc()). If you're OK with that, I'll 
> change the code accordingly.

Sounds good. And given that we are talking about old IDE devices, which are
*really* slow, I would not worry too much about optimizing for pointer
dereference :)

> 
> Cheers,
> 
> 	Michael
> 
> 
>>> +
>>>  	irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>>>  	if (irq_res && irq_res->start > 0) {
>>>  		irq = irq_res->start;
>>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 2/3] m68k/q40: add data_swab option for pata_falcon to byte-swap disk data

[Michael Schmitz]

Hi Damien,

Am 18.08.2023 um 15:15 schrieb Damien Le Moal:
> On 2023/08/18 12:08, Michael Schmitz wrote:
>> Hi Damien,
>>
>> thanks for the review ...
>>
>> Am 18.08.2023 um 12:51 schrieb Damien Le Moal:
>>> On 2023/08/18 7:12, Michael Schmitz wrote:
>>>> Some users of pata_falcon on Q40 have IDE disks in default
>>>> IDE little endian byte order, whereas legacy disks use
>>>> host-native big-endian byte order as on the Atari Falcon.
>>>>
>>>> Add module parameter 'data_swab' to allow connecting drives
>>>> with non-native data byte order. Drives selected by the
>>>> data_swap bit mask will have their user data byte-swapped to
>>>> host byte order, i.e. 'pata_falcon.data_swab=2' will byte-swap
>>>> all user data on drive B, leaving data on drive A in native
>>>> byte order. On Q40, drives on a second IDE interface may be
>>>> added to the bit mask as bits 2 and 3.
>>>>
>>>> Default setting is no byte swapping, i.e. compatibility with
>>>> the native Falcon or Q40 operating system disk format.
>>>>
>>>> Cc: William R Sowerbutts <will@sowerbutts.com>
>>>> Cc: Finn Thain <fthain@linux-m68k.org>
>>>> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
>>>> Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>
>>>>
>>>> ---
>>>>
>>>> Changes since RFC v4:
>>>>
>>>> Geert Uytterhoeven:
>>>> - don't shift static module parameter for drive 3/4 bitmask
>>>> - simplify bit mask calculation to always use pdev->id
>>>>
>>>> Finn Thain:
>>>> - correct bit numbers for drive 3/4
>>>>
>>>> Changes since RFC v3:
>>>>
>>>> - split off this byte swap handling into separate patch
>>>>
>>>> - add hint regarding third and fourth drive on Q40
>>>>
>>>> Finn Thain:
>>>> - rename module parameter to 'data_swab' to better reflect its use
>>>>
>>>> William Sowerbutts:
>>>> - correct IDE drive number used in data swap conditional
>>>> ---
>>>>  drivers/ata/pata_falcon.c | 26 +++++++++++++++++++++++++-
>>>>  1 file changed, 25 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/ata/pata_falcon.c b/drivers/ata/pata_falcon.c
>>>> index 346259e3bbc8..90488f565d6f 100644
>>>> --- a/drivers/ata/pata_falcon.c
>>>> +++ b/drivers/ata/pata_falcon.c
>>>> @@ -33,6 +33,16 @@
>>>>  #define DRV_NAME "pata_falcon"
>>>>  #define DRV_VERSION "0.1.0"
>>>>
>>>> +static int pata_falcon_swap_mask;
>>>> +
>>>> +module_param_named(data_swab, pata_falcon_swap_mask, int, 0444);
>>>> +MODULE_PARM_DESC(data_swab, "Data byte swap enable/disable bitmap (0x1==drive1, 0x2==drive2, 0x4==drive3, 0x8==drive4, default==0)");
>>>> +
>>>> +struct pata_falcon_priv {
>>>> +	unsigned int swap_mask;
>>>> +	bool swap_data;
>>>> +};
>>>> +
>>>>  static const struct scsi_host_template pata_falcon_sht = {
>>>>  	ATA_PIO_SHT(DRV_NAME),
>>>>  };
>>>> @@ -44,13 +54,15 @@ static unsigned int pata_falcon_data_xfer(struct ata_queued_cmd *qc,
>>>>  	struct ata_device *dev = qc->dev;
>>>>  	struct ata_port *ap = dev->link->ap;
>>>>  	void __iomem *data_addr = ap->ioaddr.data_addr;
>>>> +	struct pata_falcon_priv *priv = ap->private_data;
>>>>  	unsigned int words = buflen >> 1;
>>>>  	struct scsi_cmnd *cmd = qc->scsicmd;
>>>> +	int dev_id = dev->devno;
>>>>  	bool swap = 1;
>>>>
>>>>  	if (dev->class == ATA_DEV_ATA && cmd &&
>>>>  	    !blk_rq_is_passthrough(scsi_cmd_to_rq(cmd)))
>>>> -		swap = 0;
>>>> +		swap = priv->swap_data && (priv->swap_mask & BIT(dev_id));
>>>>
>>>>  	/* Transfer multiple of 2 bytes */
>>>>  	if (rw == READ) {
>>>> @@ -123,6 +135,7 @@ 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;
>>>> +	struct pata_falcon_priv *priv;
>>>>  	void __iomem *base, *ctl_base;
>>>>  	int irq = 0, io_offset = 1, reg_scale = 4;
>>>>
>>>> @@ -165,6 +178,13 @@ 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;
>>>>
>>>> +	priv = devm_kzalloc(&pdev->dev,
>>>> +		sizeof(struct pata_falcon_priv), GFP_KERNEL);
>>>> +	if (!priv)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	ap->private_data = priv;
>>>> +
>>>>  	/* N.B. this assumes data_addr will be used for word-sized I/O only */
>>>>  	ap->ioaddr.data_addr = (void __iomem *)base_mem_res->start;
>>>>
>>>> @@ -199,6 +219,10 @@ static int __init pata_falcon_init_one(struct platform_device *pdev)
>>>>  	ap->ioaddr.altstatus_addr	= ctl_base + io_offset;
>>>>  	ap->ioaddr.ctl_addr		= ctl_base + io_offset;
>>>>
>>>> +	priv->swap_mask = pata_falcon_swap_mask >> (2 * pdev->id);
>>>
>>> I do not understand the shift here. It seems that it will lead to
>>> priv->swap_mask always being 0...
>>
>> On Q40, it is possible to have two ISA IDE adapters, and two platform
>> devices are defined (in arch/m68k/q40/config.c) One will have
>> pdev->id==0, the other will have pdev->id==1.
>>
>> In the pdev->id==0 case, there's no shift of the bit mask passed in as
>> module parameter, so the data transfer function will examine bits 0 and
>> 1. That case we've verified in testing.
>> In the other case, we shift down by two bits, so the data transfer
>> function will now examine bits 2 and 3 from the module parameter.
>>
>>>
>>>> +	if (priv->swap_mask)
>>>> +		priv->swap_data = 1;
>>>
>>> I do not understand why priv->swap_data is needed, given that it is set to 1 if
>>> priv->swap_mask != 0, the above:
>>>
>>> 	swap = priv->swap_data && (priv->swap_mask & BIT(dev_id));
>>>
>>> should be equivalent to the simpler:
>>>
>>> 	swap = priv->swap_mask & BIT(dev_id);
>>>
>>> No ? Am I missing something ?
>>
>> I had hoped to avoid the pointer dereference and bit shift in the
>> default (and by far most common) case where none of the bits are set.
>>
>> Compared to the simpler version, I actually just save the bit shift, so
>> it's probably a pointless optimization.
>>
>> Finn had suggested to simplify this even further, and use ap->private as
>> bit mask directly (saving the kzalloc()). If you're OK with that, I'll
>> change the code accordingly.
>
> Sounds good. And given that we are talking about old IDE devices, which are
> *really* slow, I would not worry too much about optimizing for pointer
> dereference :)

The drive's probably faster than the CPU, at least the one I'm currently 
using :)

Regardless, I'll change this, test and respin.

Cheers,

	Michael

>
>>
>> Cheers,
>>
>> 	Michael
>>
>>
>>>> +
>>>>  	irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>>>>  	if (irq_res && irq_res->start > 0) {
>>>>  		irq = irq_res->start;
>>>
>

Re: [PATCH 2/3] m68k/q40: add data_swab option for pata_falcon to byte-swap disk data

[Sergey Shtylyov]

On 8/18/23 1:12 AM, Michael Schmitz wrote:

> Some users of pata_falcon on Q40 have IDE disks in default
> IDE little endian byte order, whereas legacy disks use
> host-native big-endian byte order as on the Atari Falcon.
> 
> Add module parameter 'data_swab' to allow connecting drives
> with non-native data byte order. Drives selected by the
> data_swap bit mask will have their user data byte-swapped to
> host byte order, i.e. 'pata_falcon.data_swab=2' will byte-swap
> all user data on drive B, leaving data on drive A in native
> byte order. On Q40, drives on a second IDE interface may be
> added to the bit mask as bits 2 and 3.
> 
> Default setting is no byte swapping, i.e. compatibility with
> the native Falcon or Q40 operating system disk format.
> 
> Cc: William R Sowerbutts <will@sowerbutts.com>
> Cc: Finn Thain <fthain@linux-m68k.org>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>
> 
> ---
> 
> Changes since RFC v4:
> 
> Geert Uytterhoeven:
> - don't shift static module parameter for drive 3/4 bitmask
> - simplify bit mask calculation to always use pdev->id
> 
> Finn Thain:
> - correct bit numbers for drive 3/4
> 
> Changes since RFC v3:
> 
> - split off this byte swap handling into separate patch
> 
> - add hint regarding third and fourth drive on Q40
> 
> Finn Thain:
> - rename module parameter to 'data_swab' to better reflect its use
> 
> William Sowerbutts:
> - correct IDE drive number used in data swap conditional
> ---
>  drivers/ata/pata_falcon.c | 26 +++++++++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/ata/pata_falcon.c b/drivers/ata/pata_falcon.c
> index 346259e3bbc8..90488f565d6f 100644
> --- a/drivers/ata/pata_falcon.c
> +++ b/drivers/ata/pata_falcon.c
> @@ -33,6 +33,16 @@
>  #define DRV_NAME "pata_falcon"
>  #define DRV_VERSION "0.1.0"
>  
> +static int pata_falcon_swap_mask;
> +
> +module_param_named(data_swab, pata_falcon_swap_mask, int, 0444);
> +MODULE_PARM_DESC(data_swab, "Data byte swap enable/disable bitmap (0x1==drive1, 0x2==drive2, 0x4==drive3, 0x8==drive4, default==0)");

   Hm, Greg KH keeps saying us that the module parameters belong to '90s. :-)

[...]
> @@ -44,13 +54,15 @@ static unsigned int pata_falcon_data_xfer(struct ata_queued_cmd *qc,
>  	struct ata_device *dev = qc->dev;
>  	struct ata_port *ap = dev->link->ap;
>  	void __iomem *data_addr = ap->ioaddr.data_addr;
> +	struct pata_falcon_priv *priv = ap->private_data;
>  	unsigned int words = buflen >> 1;
>  	struct scsi_cmnd *cmd = qc->scsicmd;
> +	int dev_id = dev->devno;

   You hardly need this variable...

>  	bool swap = 1;
>  
>  	if (dev->class == ATA_DEV_ATA && cmd &&
>  	    !blk_rq_is_passthrough(scsi_cmd_to_rq(cmd)))
> -		swap = 0;
> +		swap = priv->swap_data && (priv->swap_mask & BIT(dev_id));

   This looks convoluted -- only the 2nd subexpression should be enough...

[...]
> @@ -165,6 +178,13 @@ 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;
>  
> +	priv = devm_kzalloc(&pdev->dev,
> +		sizeof(struct pata_falcon_priv), GFP_KERNEL);

   sizeof(*priv) is preferred IIRC...

> +	if (!priv)
> +		return -ENOMEM;
> +
> +	ap->private_data = priv;

   Also you hardly need a heap allocation -- encoding your couple flags
could use the ap->private_data itself...

[...]

MBR, Sergey

Re: [PATCH 2/3] m68k/q40: add data_swab option for pata_falcon to byte-swap disk data

[Michael Schmitz]

Hi Sergey,

thanks for reviewing - this has mostly been addressed in v2 or v3 (which 
I forgot to send to you, sorry). Damien asked for the patch title to be 
changed (now 'ata: pata_falcon: add data_swab option to byte-swap disk 
data) so you might have missed it on the list.

On 21/08/23 06:07, Sergey Shtylyov wrote:
> On 8/18/23 1:12 AM, Michael Schmitz wrote:
>
>> Some users of pata_falcon on Q40 have IDE disks in default
>> IDE little endian byte order, whereas legacy disks use
>> host-native big-endian byte order as on the Atari Falcon.
>>
>> Add module parameter 'data_swab' to allow connecting drives
>> with non-native data byte order. Drives selected by the
>> data_swap bit mask will have their user data byte-swapped to
>> host byte order, i.e. 'pata_falcon.data_swab=2' will byte-swap
>> all user data on drive B, leaving data on drive A in native
>> byte order. On Q40, drives on a second IDE interface may be
>> added to the bit mask as bits 2 and 3.
>>
>> Default setting is no byte swapping, i.e. compatibility with
>> the native Falcon or Q40 operating system disk format.
>>
>> Cc: William R Sowerbutts <will@sowerbutts.com>
>> Cc: Finn Thain <fthain@linux-m68k.org>
>> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
>> Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>
>>
>> ---
>>
>> Changes since RFC v4:
>>
>> Geert Uytterhoeven:
>> - don't shift static module parameter for drive 3/4 bitmask
>> - simplify bit mask calculation to always use pdev->id
>>
>> Finn Thain:
>> - correct bit numbers for drive 3/4
>>
>> Changes since RFC v3:
>>
>> - split off this byte swap handling into separate patch
>>
>> - add hint regarding third and fourth drive on Q40
>>
>> Finn Thain:
>> - rename module parameter to 'data_swab' to better reflect its use
>>
>> William Sowerbutts:
>> - correct IDE drive number used in data swap conditional
>> ---
>>   drivers/ata/pata_falcon.c | 26 +++++++++++++++++++++++++-
>>   1 file changed, 25 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/ata/pata_falcon.c b/drivers/ata/pata_falcon.c
>> index 346259e3bbc8..90488f565d6f 100644
>> --- a/drivers/ata/pata_falcon.c
>> +++ b/drivers/ata/pata_falcon.c
>> @@ -33,6 +33,16 @@
>>   #define DRV_NAME "pata_falcon"
>>   #define DRV_VERSION "0.1.0"
>>   
>> +static int pata_falcon_swap_mask;
>> +
>> +module_param_named(data_swab, pata_falcon_swap_mask, int, 0444);
>> +MODULE_PARM_DESC(data_swab, "Data byte swap enable/disable bitmap (0x1==drive1, 0x2==drive2, 0x4==drive3, 0x8==drive4, default==0)");
>     Hm, Greg KH keeps saying us that the module parameters belong to '90s. :-)
What else can I use that would allow setting a driver parameter at boot 
time? This driver will be built-in pretty much all the time.
>
> [...]
>> @@ -44,13 +54,15 @@ static unsigned int pata_falcon_data_xfer(struct ata_queued_cmd *qc,
>>   	struct ata_device *dev = qc->dev;
>>   	struct ata_port *ap = dev->link->ap;
>>   	void __iomem *data_addr = ap->ioaddr.data_addr;
>> +	struct pata_falcon_priv *priv = ap->private_data;
>>   	unsigned int words = buflen >> 1;
>>   	struct scsi_cmnd *cmd = qc->scsicmd;
>> +	int dev_id = dev->devno;
>     You hardly need this variable...
Fixed in v3.
>
>>   	bool swap = 1;
>>   
>>   	if (dev->class == ATA_DEV_ATA && cmd &&
>>   	    !blk_rq_is_passthrough(scsi_cmd_to_rq(cmd)))
>> -		swap = 0;
>> +		swap = priv->swap_data && (priv->swap_mask & BIT(dev_id));
>     This looks convoluted -- only the 2nd subexpression should be enough...
Pointless attempt at optimizing this for the default case. Gone now.
>
> [...]
>> @@ -165,6 +178,13 @@ 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;
>>   
>> +	priv = devm_kzalloc(&pdev->dev,
>> +		sizeof(struct pata_falcon_priv), GFP_KERNEL);
>     sizeof(*priv) is preferred IIRC...
>
>> +	if (!priv)
>> +		return -ENOMEM;
>> +
>> +	ap->private_data = priv;
>     Also you hardly need a heap allocation -- encoding your couple flags
> could use the ap->private_data itself...

That's what Finn suggested as well - changed in v2.

Cheers,

     Michael

>
> [...]
>
> MBR, Sergey

Re: [PATCH 2/3] m68k/q40: add data_swab option for pata_falcon to byte-swap disk data

[Sergei Shtylyov]

Hello!

On 8/20/23 10:27 PM, Michael Schmitz wrote:
[...]

> thanks for reviewing - this has mostly been addressed in v2 or v3 (which I forgot to send to you, sorry). Damien asked for the patch title to be changed (now 'ata: pata_falcon: add data_swab option to byte-swap disk data) so you might have missed it on the list.

   I didn't want to repeat such request after him. :-)
   I'm subscribed to linux-ide thru my Gmail account, and I'm still not seeing
your further patch versions on the list... :-/

[...]

>>> Some users of pata_falcon on Q40 have IDE disks in default
>>> IDE little endian byte order, whereas legacy disks use
>>> host-native big-endian byte order as on the Atari Falcon.
>>>
>>> Add module parameter 'data_swab' to allow connecting drives
>>> with non-native data byte order. Drives selected by the
>>> data_swap bit mask will have their user data byte-swapped to
>>> host byte order, i.e. 'pata_falcon.data_swab=2' will byte-swap
>>> all user data on drive B, leaving data on drive A in native
>>> byte order. On Q40, drives on a second IDE interface may be
>>> added to the bit mask as bits 2 and 3.
>>>
>>> Default setting is no byte swapping, i.e. compatibility with
>>> the native Falcon or Q40 operating system disk format.
>>>
>>> Cc: William R Sowerbutts <will@sowerbutts.com>
>>> Cc: Finn Thain <fthain@linux-m68k.org>
>>> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
>>> Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>

[...]

>>> diff --git a/drivers/ata/pata_falcon.c b/drivers/ata/pata_falcon.c
>>> index 346259e3bbc8..90488f565d6f 100644
>>> --- a/drivers/ata/pata_falcon.c
>>> +++ b/drivers/ata/pata_falcon.c
>>> @@ -33,6 +33,16 @@
>>>   #define DRV_NAME "pata_falcon"
>>>   #define DRV_VERSION "0.1.0"
>>>   +static int pata_falcon_swap_mask;
>>> +
>>> +module_param_named(data_swab, pata_falcon_swap_mask, int, 0444);
>>> +MODULE_PARM_DESC(data_swab, "Data byte swap enable/disable bitmap (0x1==drive1, 0x2==drive2, 0x4==drive3, 0x8==drive4, default==0)");
>>     Hm, Greg KH keeps saying us that the module parameters belong to '90s. :-)

> What else can I use that would allow setting a driver parameter at boot time? This driver will be built-in pretty much all the time.

   I guess he means sysfs...

[...]

MBR, Sergey

Re: [PATCH 2/3] m68k/q40: add data_swab option for pata_falcon to byte-swap disk data

[Sergei Shtylyov]

On 8/22/23 10:10 PM, Sergei Shtylyov wrote:
[...]

>> thanks for reviewing - this has mostly been addressed in v2 or v3 (which I forgot to send to you, sorry). Damien asked for the patch title to be changed (now 'ata: pata_falcon: add data_swab option to byte-swap disk data) so you might have missed it on the list.
> 
>    I didn't want to repeat such request after him. :-)
>    I'm subscribed to linux-ide thru my Gmail account, and I'm still not seeing
> your further patch versions on the list... :-/

   Had to reply from Gmail account as the OMP server rejected my msg.
Please be sure to always CC: me on the PATA patches!

> [...]
> 
>>>> Some users of pata_falcon on Q40 have IDE disks in default
>>>> IDE little endian byte order, whereas legacy disks use
>>>> host-native big-endian byte order as on the Atari Falcon.
>>>>
>>>> Add module parameter 'data_swab' to allow connecting drives
>>>> with non-native data byte order. Drives selected by the
>>>> data_swap bit mask will have their user data byte-swapped to
>>>> host byte order, i.e. 'pata_falcon.data_swab=2' will byte-swap
>>>> all user data on drive B, leaving data on drive A in native
>>>> byte order. On Q40, drives on a second IDE interface may be
>>>> added to the bit mask as bits 2 and 3.
>>>>
>>>> Default setting is no byte swapping, i.e. compatibility with
>>>> the native Falcon or Q40 operating system disk format.
>>>>
>>>> Cc: William R Sowerbutts <will@sowerbutts.com>
>>>> Cc: Finn Thain <fthain@linux-m68k.org>
>>>> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
>>>> Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>

[...]

MBR, Sergey

Re: [PATCH 2/3] m68k/q40: add data_swab option for pata_falcon to byte-swap disk data

[Michael Schmitz]

Hi Sergey,

Am 23.08.2023 um 07:44 schrieb Sergei Shtylyov:
> On 8/22/23 10:10 PM, Sergei Shtylyov wrote:
> [...]
>
>>> thanks for reviewing - this has mostly been addressed in v2 or v3 (which I forgot to send to you, sorry). Damien asked for the patch title to be changed (now 'ata: pata_falcon: add data_swab option to byte-swap disk data) so you might have missed it on the list.
>>
>>    I didn't want to repeat such request after him. :-)
>>    I'm subscribed to linux-ide thru my Gmail account, and I'm still not seeing
>> your further patch versions on the list... :-/
>
>    Had to reply from Gmail account as the OMP server rejected my msg.
> Please be sure to always CC: me on the PATA patches!

Will do - v4 to go out shortly (afer a final boot test).

Cheers,

	Michael

[PATCH 3/3] m68k/atari: change Falcon IDE platform device to id 0

[Michael Schmitz]

The pata_falcon data byte swapping patch relies on pdev->id
being 0 or 1. Q40 uses these IDs, but Atari used -1. Change
pdev->id to 0 for Atari Falcon IDE platform device so
selection of drive to byte-swap through pata_falcon.data_swab
can be used on Falcon as well.
Tested on ARAnyM so far.

Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>
---
 arch/m68k/atari/config.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/m68k/atari/config.c b/arch/m68k/atari/config.c
index 38a7c0578105..e3e437cd0f84 100644
--- a/arch/m68k/atari/config.c
+++ b/arch/m68k/atari/config.c
@@ -925,7 +925,7 @@ int __init atari_platform_init(void)
 #endif
 
 	if (ATARIHW_PRESENT(IDE)) {
-		pdev = platform_device_register_simple("atari-falcon-ide", -1,
+		pdev = platform_device_register_simple("atari-falcon-ide", 0,
 			atari_falconide_rsrc, ARRAY_SIZE(atari_falconide_rsrc));
 		if (IS_ERR(pdev))
 			rv = PTR_ERR(pdev);
-- 
2.17.1