[PATCH v4 0/2] Q40 IDE fixes

[PATCH v4 0/2] Q40 IDE fixes

[Michael Schmitz]

Version 4 of the pata_falcon bugfix patch for Q40 support.

Changes to patch 1 only - now uses register shift instead of
register step (or gap, or scale ...). Most drivers appear to
use shift these days where platform integration differences
have to be reconciled. ata_port_desc() will now report the
MMIO address used by the data transfer function (which differs
from command and control base address in the Q40 case).

Cheers,

   Michael

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

[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 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>

---

Changes from v3:

Sergey Shtylyov:
- change use of reg_scale to reg_shift

Geert Uytterhoeven:
- factor out ata_port_desc() from platform specific code

Changes from v2:

Finn Thain:
- add back stable Cc:

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 | 50 +++++++++++++++++++++++----------------
 1 file changed, 29 insertions(+), 21 deletions(-)

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
@@ -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_shift = 2; /* Falcon defaults */
 
 	dev_info(&pdev->dev, "Atari Falcon and Q40/Q60 PATA controller\n");
 
@@ -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);
 
 	irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
 	if (irq_res && irq_res->start > 0) {
-- 
2.17.1

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

[Geert Uytterhoeven]

Hi Michael,

On Wed, Aug 23, 2023 at 12:14 AM Michael Schmitz <schmitzmic@gmail.com> 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>
>
> ---
>
> Changes from v3:
>
> Sergey Shtylyov:
> - change use of reg_scale to reg_shift
>
> Geert Uytterhoeven:
> - factor out ata_port_desc() from platform specific code

Thanks for the update!

> --- a/drivers/ata/pata_falcon.c
> +++ b/drivers/ata/pata_falcon.c

> +       ata_port_desc(ap, "cmd %px ctl %px data %pa",
> +                     base, ctl_base, &ap->ioaddr.data_addr);

%px and ap->ioaddr.data_addr

The rest LGTM, so with the above fixed
Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

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

[Michael Schmitz]

Hi Geert,

On 23/08/23 21:05, Geert Uytterhoeven wrote:
>
> Thanks for the update!
>
>> --- a/drivers/ata/pata_falcon.c
>> +++ b/drivers/ata/pata_falcon.c
>> +       ata_port_desc(ap, "cmd %px ctl %px data %pa",
>> +                     base, ctl_base, &ap->ioaddr.data_addr);
> %px and ap->ioaddr.data_addr
>
> The rest LGTM, so with the above fixed
> Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>

Will fix this in v4.

Thanks,

     Michael

>
> Gr{oetje,eeting}s,
>
>                          Geert
>

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

[Sergey Shtylyov]

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

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

[Michael Schmitz]

Hi Sergey,

On 24/08/23 04:05, Sergey Shtylyov wrote:
> Hello!
>
>     I prefer CCing my OMP account when you send the PATA patches,
> as is returned by scripts/get_maintainer.pl...
Sorry, I was left with the impression OMP was rejecting list messages 
from linux-ide ...
>
> 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...

Will do.

Cheers,

     Michael

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

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

[Sergey Shtylyov]

On 8/24/23 4:56 AM, Michael Schmitz wrote:
[...]

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

> Sorry, I was left with the impression OMP was rejecting list messages from linux-ide ...

   No, it rejected my reply to you for some reason.
   However, the msgs from linux-ide seem to be still stuck somewhere
as well...

MBR, Sergey

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

[Michael Schmitz]

Hi Sergey,

thanks for clarifying, I'll correct that for v5.

Cheers,

     Michael

On 24/08/23 22:00, Sergey Shtylyov wrote:
> On 8/24/23 4:56 AM, Michael Schmitz wrote:
> [...]
>
>>>      I prefer CCing my OMP account when you send the PATA patches,
>>> as is returned by scripts/get_maintainer.pl...
>> Sorry, I was left with the impression OMP was rejecting list messages from linux-ide ...
>     No, it rejected my reply to you for some reason.
>     However, the msgs from linux-ide seem to be still stuck somewhere
> as well...
>
> MBR, Sergey

[PATCH v4 2/2] ata: pata_falcon: add data_swab option 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>
Tested-by: William R Sowerbutts <will@sowerbutts.com>
Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>

---

Changes since v2:

Geert Uytterhoeven:
- only shift swap bitmask if pdev->id > 0

Finn Thain:
- use pdev->devno directly for byte swap check

Changes since v1:

Damien Le Moal:
- change patch title
- drop swap_data flag

Finn Thain:
- drop allocation of ap->private struct, use field as bitmask

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 | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/ata/pata_falcon.c b/drivers/ata/pata_falcon.c
index 3841ea200bcb..7cf15bd9764a 100644
--- a/drivers/ata/pata_falcon.c
+++ b/drivers/ata/pata_falcon.c
@@ -33,6 +33,11 @@
 #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)");
+
 static const struct scsi_host_template pata_falcon_sht = {
 	ATA_PIO_SHT(DRV_NAME),
 };
@@ -50,7 +55,7 @@ static unsigned int pata_falcon_data_xfer(struct ata_queued_cmd *qc,
 
 	if (dev->class == ATA_DEV_ATA && cmd &&
 	    !blk_rq_is_passthrough(scsi_cmd_to_rq(cmd)))
-		swap = 0;
+		swap = (uintptr_t)ap->private_data & BIT(dev->devno);
 
 	/* Transfer multiple of 2 bytes */
 	if (rw == READ) {
@@ -194,6 +199,9 @@ static int __init pata_falcon_init_one(struct platform_device *pdev)
 	ata_port_desc(ap, "cmd %px ctl %px data %pa",
 		      base, ctl_base, &ap->ioaddr.data_addr);
 
+	ap->private_data = (void *)(uintptr_t)(pdev->id > 0 ?
+		pata_falcon_swap_mask >> 2 : pata_falcon_swap_mask);
+
 	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 v4 2/2] ata: pata_falcon: add data_swab option to byte-swap disk data

[Geert Uytterhoeven]

On Wed, Aug 23, 2023 at 12:14 AM Michael Schmitz <schmitzmic@gmail.com> 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>
> Tested-by: William R Sowerbutts <will@sowerbutts.com>
> Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>
>
> ---
>
> Changes since v2:
>
> Geert Uytterhoeven:
> - only shift swap bitmask if pdev->id > 0

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: [PATCH v4 2/2] ata: pata_falcon: add data_swab option to byte-swap disk data

[Geert Uytterhoeven]

On Wed, Aug 23, 2023 at 11:10 AM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Wed, Aug 23, 2023 at 12:14 AM Michael Schmitz <schmitzmic@gmail.com> 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>
> > Tested-by: William R Sowerbutts <will@sowerbutts.com>
> > Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>
> >
> > ---
> >
> > Changes since v2:
> >
> > Geert Uytterhoeven:
> > - only shift swap bitmask if pdev->id > 0
>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Oops, I meant
Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>
But it never hurts to have more review tags ;-)

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: [PATCH v4 2/2] ata: pata_falcon: add data_swab option to byte-swap disk data

[Michael Schmitz]

Hi Geert,

On 23/08/23 21:10, Geert Uytterhoeven wrote:
> On Wed, Aug 23, 2023 at 11:10 AM Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
>> On Wed, Aug 23, 2023 at 12:14 AM Michael Schmitz <schmitzmic@gmail.com> 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>
>>> Tested-by: William R Sowerbutts <will@sowerbutts.com>
>>> Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>
>>>
>>> ---
>>>
>>> Changes since v2:
>>>
>>> Geert Uytterhoeven:
>>> - only shift swap bitmask if pdev->id > 0
>> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Oops, I meant
> Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>
> But it never hurts to have more review tags ;-)

Thanks - I can pretty much type that one blind ...

Cheers,

     Michael

>
> Gr{oetje,eeting}s,
>
>                          Geert
>

Re: [PATCH v4 2/2] ata: pata_falcon: add data_swab option to byte-swap disk data

[Sergey Shtylyov]

On 8/23/23 1:13 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>
> 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 3841ea200bcb..7cf15bd9764a 100644
> --- a/drivers/ata/pata_falcon.c
> +++ b/drivers/ata/pata_falcon.c
[...]
> @@ -194,6 +199,9 @@ static int __init pata_falcon_init_one(struct platform_device *pdev)
>  	ata_port_desc(ap, "cmd %px ctl %px data %pa",
>  		      base, ctl_base, &ap->ioaddr.data_addr);
>  
> +	ap->private_data = (void *)(uintptr_t)(pdev->id > 0 ?
> +		pata_falcon_swap_mask >> 2 : pata_falcon_swap_mask);

   How about:

	ap->private_data = (void *)(uintptr_t)(pata_falcon_swap_mask >>
			   (pdev->id > 0 ? 2 : 0));

[...]

MBR, Sergey

Re: [PATCH v4 2/2] ata: pata_falcon: add data_swab option to byte-swap disk data

[Damien Le Moal]

On 8/24/23 01:35, Sergey Shtylyov wrote:
> On 8/23/23 1:13 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>
>> 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 3841ea200bcb..7cf15bd9764a 100644
>> --- a/drivers/ata/pata_falcon.c
>> +++ b/drivers/ata/pata_falcon.c
> [...]
>> @@ -194,6 +199,9 @@ static int __init pata_falcon_init_one(struct platform_device *pdev)
>>  	ata_port_desc(ap, "cmd %px ctl %px data %pa",
>>  		      base, ctl_base, &ap->ioaddr.data_addr);
>>  
>> +	ap->private_data = (void *)(uintptr_t)(pdev->id > 0 ?
>> +		pata_falcon_swap_mask >> 2 : pata_falcon_swap_mask);
> 
>    How about:
> 
> 	ap->private_data = (void *)(uintptr_t)(pata_falcon_swap_mask >>
> 			   (pdev->id > 0 ? 2 : 0));

This is so hard to decode... Let's please spell this out.
Something like:

	int shift;

	if (pdev->id)
		shift = 2;
	else
		shift = 0;
	ap->private_data = (uintptr_t)(pata_falcon_swap_mask >> shift);

This is initialization, so no need to try to optimize and rather privilege clear
code.

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

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v4 2/2] ata: pata_falcon: add data_swab option to byte-swap disk data

[Michael Schmitz]

Hi Damien,

On 24/08/23 11:03, Damien Le Moal wrote:
> On 8/24/23 01:35, Sergey Shtylyov wrote:
>> On 8/23/23 1:13 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>
>>> 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 3841ea200bcb..7cf15bd9764a 100644
>>> --- a/drivers/ata/pata_falcon.c
>>> +++ b/drivers/ata/pata_falcon.c
>> [...]
>>> @@ -194,6 +199,9 @@ static int __init pata_falcon_init_one(struct platform_device *pdev)
>>>   	ata_port_desc(ap, "cmd %px ctl %px data %pa",
>>>   		      base, ctl_base, &ap->ioaddr.data_addr);
>>>   
>>> +	ap->private_data = (void *)(uintptr_t)(pdev->id > 0 ?
>>> +		pata_falcon_swap_mask >> 2 : pata_falcon_swap_mask);
>>     How about:
>>
>> 	ap->private_data = (void *)(uintptr_t)(pata_falcon_swap_mask >>
>> 			   (pdev->id > 0 ? 2 : 0));
> This is so hard to decode... Let's please spell this out.
> Something like:
>
> 	int shift;
>
> 	if (pdev->id)

Atari Falcon has pdev->id==-1, so this must be 'if (pdev->id > 0)' here.

(Testing for pdev->id nonzero did work in earlier versions of my patch 
because patch 3 changed the platform device ID on Atari. That patch had 
the potential to confuse user space so I dropped it from v3 on.)

> 		shift = 2;
> 	else
> 		shift = 0;

No need for the 'else' if we initialize shift above (as is done for irq, 
io_offset and reg_shift).

> 	ap->private_data = (uintptr_t)(pata_falcon_swap_mask >> shift);
>
> This is initialization, so no need to try to optimize and rather privilege clear
> code.

Hmm - or maybe I'll leave that spelled out as you suggest.

Cheers,

     Michael

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