[PATCH v2 0/3] Q40 IDE fixes

[PATCH v2 0/3] Q40 IDE fixes

[Michael Schmitz]

Version 2 of the Q40 bugfix series for pata_falcon.

Comments by Damien and Finn have been addressed.

Cheers,

   Michael

[PATCH v2 1/3] 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 scale and byte vs. word
offset in the address.

This was correct for the Falcon case, which does not apply
any address translation to the register addresses. In the
Q40 case, all of device base address, byte access offset
and register scaling is included in the platform specific
ISA access translation (in asm/mm_io.h).

As a consequence, such address translation gets applied
twice, and register addresses are mangled.

Use the device base address from the platform IO resource,
and use standard register offsets from that base in order
to calculate register addresses (the IO address translation
will then apply the correct ISA window base and scaling).

Encode PIO_OFFSET into IO port addresses for all registers
except the data transfer register. Encode the MMIO offset
there (pata_falcon_data_xfer() directly uses raw IO with
no address translation).

Reported-by: William R Sowerbutts <will@sowerbutts.com>
Closes: https://lore.kernel.org/r/CAMuHMdUU62jjunJh9cqSqHT87B0H0A4udOOPs=WN7WZKpcagVA@mail.gmail.com
Link: https://lore.kernel.org/r/CAMuHMdUU62jjunJh9cqSqHT87B0H0A4udOOPs=WN7WZKpcagVA@mail.gmail.com
Fixes: 44b1fbc0f5f3 ("m68k/q40: Replace q40ide driver with pata_falcon and falconide")
Cc: Finn Thain <fthain@linux-m68k.org>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>

---

Changes from v1:

Damien Le Moal:
- change patch title
- drop stable backport tag

Changes from RFC v3:

- split off byte swap option into separate patch

Geert Uytterhoeven:
- review comments

Changes from RFC v2:
- add driver parameter 'data_swap' as bit mask for drives to swap

Changes from RFC v1:

Finn Thain:
- take care to supply IO address suitable for ioread8/iowrite8
- use MMIO address for data transfer
---
 drivers/ata/pata_falcon.c | 55 ++++++++++++++++++++++++---------------
 1 file changed, 34 insertions(+), 21 deletions(-)

diff --git a/drivers/ata/pata_falcon.c b/drivers/ata/pata_falcon.c
index 996516e64f13..346259e3bbc8 100644
--- a/drivers/ata/pata_falcon.c
+++ b/drivers/ata/pata_falcon.c
@@ -123,8 +123,8 @@ static int __init pata_falcon_init_one(struct platform_device *pdev)
 	struct resource *base_res, *ctl_res, *irq_res;
 	struct ata_host *host;
 	struct ata_port *ap;
-	void __iomem *base;
-	int irq = 0;
+	void __iomem *base, *ctl_base;
+	int irq = 0, io_offset = 1, reg_scale = 4;
 
 	dev_info(&pdev->dev, "Atari Falcon and Q40/Q60 PATA controller\n");
 
@@ -165,26 +165,39 @@ static int __init pata_falcon_init_one(struct platform_device *pdev)
 	ap->pio_mask = ATA_PIO4;
 	ap->flags |= ATA_FLAG_SLAVE_POSS | ATA_FLAG_NO_IORDY;
 
-	base = (void __iomem *)base_mem_res->start;
 	/* N.B. this assumes data_addr will be used for word-sized I/O only */
-	ap->ioaddr.data_addr		= base + 0 + 0 * 4;
-	ap->ioaddr.error_addr		= base + 1 + 1 * 4;
-	ap->ioaddr.feature_addr		= base + 1 + 1 * 4;
-	ap->ioaddr.nsect_addr		= base + 1 + 2 * 4;
-	ap->ioaddr.lbal_addr		= base + 1 + 3 * 4;
-	ap->ioaddr.lbam_addr		= base + 1 + 4 * 4;
-	ap->ioaddr.lbah_addr		= base + 1 + 5 * 4;
-	ap->ioaddr.device_addr		= base + 1 + 6 * 4;
-	ap->ioaddr.status_addr		= base + 1 + 7 * 4;
-	ap->ioaddr.command_addr		= base + 1 + 7 * 4;
-
-	base = (void __iomem *)ctl_mem_res->start;
-	ap->ioaddr.altstatus_addr	= base + 1;
-	ap->ioaddr.ctl_addr		= base + 1;
-
-	ata_port_desc(ap, "cmd 0x%lx ctl 0x%lx",
-		      (unsigned long)base_mem_res->start,
-		      (unsigned long)ctl_mem_res->start);
+	ap->ioaddr.data_addr = (void __iomem *)base_mem_res->start;
+
+	if (base_res) {		/* only Q40 has IO resources */
+		io_offset = 0x10000;
+		reg_scale = 1;
+		base = (void __iomem *)base_res->start;
+		ctl_base = (void __iomem *)ctl_res->start;
+
+		ata_port_desc(ap, "cmd %pa ctl %pa",
+			      &base_res->start,
+			      &ctl_res->start);
+	} else {
+		base = (void __iomem *)base_mem_res->start;
+		ctl_base = (void __iomem *)ctl_mem_res->start;
+
+		ata_port_desc(ap, "cmd %pa ctl %pa",
+			      &base_mem_res->start,
+			      &ctl_mem_res->start);
+	}
+
+	ap->ioaddr.error_addr	= base + io_offset + 1 * reg_scale;
+	ap->ioaddr.feature_addr	= base + io_offset + 1 * reg_scale;
+	ap->ioaddr.nsect_addr	= base + io_offset + 2 * reg_scale;
+	ap->ioaddr.lbal_addr	= base + io_offset + 3 * reg_scale;
+	ap->ioaddr.lbam_addr	= base + io_offset + 4 * reg_scale;
+	ap->ioaddr.lbah_addr	= base + io_offset + 5 * reg_scale;
+	ap->ioaddr.device_addr	= base + io_offset + 6 * reg_scale;
+	ap->ioaddr.status_addr	= base + io_offset + 7 * reg_scale;
+	ap->ioaddr.command_addr	= base + io_offset + 7 * reg_scale;
+
+	ap->ioaddr.altstatus_addr	= ctl_base + io_offset;
+	ap->ioaddr.ctl_addr		= ctl_base + io_offset;
 
 	irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
 	if (irq_res && irq_res->start > 0) {
-- 
2.17.1

[PATCH v2 2/3] 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>
Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>

---

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 346259e3bbc8..27443cb757de 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),
 };
@@ -46,11 +51,12 @@ static unsigned int pata_falcon_data_xfer(struct ata_queued_cmd *qc,
 	void __iomem *data_addr = ap->ioaddr.data_addr;
 	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 = (uintptr_t)ap->private_data & BIT(dev_id);
 
 	/* Transfer multiple of 2 bytes */
 	if (rw == READ) {
@@ -199,6 +205,8 @@ 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;
 
+	ap->private_data = (void *)(uintptr_t)(pata_falcon_swap_mask >> (2 * pdev->id));
+
 	irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
 	if (irq_res && irq_res->start > 0) {
 		irq = irq_res->start;
-- 
2.17.1

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

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

[Finn Thain]

On Fri, 18 Aug 2023, 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 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 346259e3bbc8..27443cb757de 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),
>  };
> @@ -46,11 +51,12 @@ static unsigned int pata_falcon_data_xfer(struct ata_queued_cmd *qc,
>  	void __iomem *data_addr = ap->ioaddr.data_addr;
>  	unsigned int words = buflen >> 1;
>  	struct scsi_cmnd *cmd = qc->scsicmd;
> +	int dev_id = dev->devno;

Is that variable really needed?

>  	bool swap = 1;
>  
>  	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_id);
>  
>  	/* Transfer multiple of 2 bytes */
>  	if (rw == READ) {
> @@ -199,6 +205,8 @@ 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;
>  
> +	ap->private_data = (void *)(uintptr_t)(pata_falcon_swap_mask >> (2 * pdev->id));
> +

My compiler doesn't need that extra type cast in there...

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

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

[Finn Thain]

On Fri, 18 Aug 2023, Michael Schmitz wrote:

> With commit 44b1fbc0f5f3 ("m68k/q40: Replace q40ide driver
> with pata_falcon and falconide"), the Q40 IDE driver was
> replaced by pata_falcon.c.
> 
> Both IO and memory resources were defined for the Q40 IDE
> platform device, but definition of the IDE register addresses
> was modeled after the Falcon case, both in use of the memory
> resources and in including register scale and byte vs. word
> offset in the address.
> 
> This was correct for the Falcon case, which does not apply
> any address translation to the register addresses. In the
> Q40 case, all of device base address, byte access offset
> and register scaling is included in the platform specific
> ISA access translation (in asm/mm_io.h).
> 
> As a consequence, such address translation gets applied
> twice, and register addresses are mangled.
> 
> Use the device base address from the platform IO resource,
> and use standard register offsets from that base in order
> to calculate register addresses (the IO address translation
> will then apply the correct ISA window base and scaling).
> 
> Encode PIO_OFFSET into IO port addresses for all registers
> except the data transfer register. Encode the MMIO offset
> there (pata_falcon_data_xfer() directly uses raw IO with
> no address translation).
> 
> Reported-by: William R Sowerbutts <will@sowerbutts.com>
> Closes: https://lore.kernel.org/r/CAMuHMdUU62jjunJh9cqSqHT87B0H0A4udOOPs=WN7WZKpcagVA@mail.gmail.com
> Link: https://lore.kernel.org/r/CAMuHMdUU62jjunJh9cqSqHT87B0H0A4udOOPs=WN7WZKpcagVA@mail.gmail.com
> Fixes: 44b1fbc0f5f3 ("m68k/q40: Replace q40ide driver with pata_falcon and falconide")
> Cc: Finn Thain <fthain@linux-m68k.org>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>

Cc: stable@vger.kernel.org

From Documentation/process/submitting-patches.rst --
"Note: Attaching a Fixes: tag does not subvert the stable kernel rules 
process nor the requirement to Cc: stable@vger.kernel.org on all stable 
patch candidates... "

> Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>
> 
> ---
> 
> Changes from v1:
> 
> Damien Le Moal:
> - change patch title
> - drop stable backport tag
> 
> Changes from RFC v3:
> 
> - split off byte swap option into separate patch
> 
> Geert Uytterhoeven:
> - review comments
> 
> Changes from RFC v2:
> - add driver parameter 'data_swap' as bit mask for drives to swap
> 
> Changes from RFC v1:
> 
> Finn Thain:
> - take care to supply IO address suitable for ioread8/iowrite8
> - use MMIO address for data transfer
> ---
>  drivers/ata/pata_falcon.c | 55 ++++++++++++++++++++++++---------------
>  1 file changed, 34 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/ata/pata_falcon.c b/drivers/ata/pata_falcon.c
> index 996516e64f13..346259e3bbc8 100644
> --- a/drivers/ata/pata_falcon.c
> +++ b/drivers/ata/pata_falcon.c
> @@ -123,8 +123,8 @@ static int __init pata_falcon_init_one(struct platform_device *pdev)
>  	struct resource *base_res, *ctl_res, *irq_res;
>  	struct ata_host *host;
>  	struct ata_port *ap;
> -	void __iomem *base;
> -	int irq = 0;
> +	void __iomem *base, *ctl_base;
> +	int irq = 0, io_offset = 1, reg_scale = 4;
>  
>  	dev_info(&pdev->dev, "Atari Falcon and Q40/Q60 PATA controller\n");
>  
> @@ -165,26 +165,39 @@ static int __init pata_falcon_init_one(struct platform_device *pdev)
>  	ap->pio_mask = ATA_PIO4;
>  	ap->flags |= ATA_FLAG_SLAVE_POSS | ATA_FLAG_NO_IORDY;
>  
> -	base = (void __iomem *)base_mem_res->start;
>  	/* N.B. this assumes data_addr will be used for word-sized I/O only */
> -	ap->ioaddr.data_addr		= base + 0 + 0 * 4;
> -	ap->ioaddr.error_addr		= base + 1 + 1 * 4;
> -	ap->ioaddr.feature_addr		= base + 1 + 1 * 4;
> -	ap->ioaddr.nsect_addr		= base + 1 + 2 * 4;
> -	ap->ioaddr.lbal_addr		= base + 1 + 3 * 4;
> -	ap->ioaddr.lbam_addr		= base + 1 + 4 * 4;
> -	ap->ioaddr.lbah_addr		= base + 1 + 5 * 4;
> -	ap->ioaddr.device_addr		= base + 1 + 6 * 4;
> -	ap->ioaddr.status_addr		= base + 1 + 7 * 4;
> -	ap->ioaddr.command_addr		= base + 1 + 7 * 4;
> -
> -	base = (void __iomem *)ctl_mem_res->start;
> -	ap->ioaddr.altstatus_addr	= base + 1;
> -	ap->ioaddr.ctl_addr		= base + 1;
> -
> -	ata_port_desc(ap, "cmd 0x%lx ctl 0x%lx",
> -		      (unsigned long)base_mem_res->start,
> -		      (unsigned long)ctl_mem_res->start);
> +	ap->ioaddr.data_addr = (void __iomem *)base_mem_res->start;
> +
> +	if (base_res) {		/* only Q40 has IO resources */
> +		io_offset = 0x10000;
> +		reg_scale = 1;
> +		base = (void __iomem *)base_res->start;
> +		ctl_base = (void __iomem *)ctl_res->start;
> +
> +		ata_port_desc(ap, "cmd %pa ctl %pa",
> +			      &base_res->start,
> +			      &ctl_res->start);
> +	} else {
> +		base = (void __iomem *)base_mem_res->start;
> +		ctl_base = (void __iomem *)ctl_mem_res->start;
> +
> +		ata_port_desc(ap, "cmd %pa ctl %pa",
> +			      &base_mem_res->start,
> +			      &ctl_mem_res->start);
> +	}
> +
> +	ap->ioaddr.error_addr	= base + io_offset + 1 * reg_scale;
> +	ap->ioaddr.feature_addr	= base + io_offset + 1 * reg_scale;
> +	ap->ioaddr.nsect_addr	= base + io_offset + 2 * reg_scale;
> +	ap->ioaddr.lbal_addr	= base + io_offset + 3 * reg_scale;
> +	ap->ioaddr.lbam_addr	= base + io_offset + 4 * reg_scale;
> +	ap->ioaddr.lbah_addr	= base + io_offset + 5 * reg_scale;
> +	ap->ioaddr.device_addr	= base + io_offset + 6 * reg_scale;
> +	ap->ioaddr.status_addr	= base + io_offset + 7 * reg_scale;
> +	ap->ioaddr.command_addr	= base + io_offset + 7 * reg_scale;
> +
> +	ap->ioaddr.altstatus_addr	= ctl_base + io_offset;
> +	ap->ioaddr.ctl_addr		= ctl_base + io_offset;
>  
>  	irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>  	if (irq_res && irq_res->start > 0) {
> 

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

[Geert Uytterhoeven]

Hi Finn,

On Fri, Aug 18, 2023 at 9:43 AM Finn Thain <fthain@linux-m68k.org> wrote:
> On Fri, 18 Aug 2023, 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>

> > --- a/drivers/ata/pata_falcon.c
> > +++ b/drivers/ata/pata_falcon.c
> > @@ -199,6 +205,8 @@ 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;
> >
> > +     ap->private_data = (void *)(uintptr_t)(pata_falcon_swap_mask >> (2 * pdev->id));
> > +
>
> My compiler doesn't need that extra type cast in there...

Because it's a 32-bit compiler ;-)
With a 64-bit compiler, you would get

    warning: cast to pointer from integer of different size
[-Wint-to-pointer-cast]

Alternatively, you can change pata_falcon_swap_mask from int to long.

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


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 v2 3/3] m68k/atari: change Falcon IDE platform device to id 0

[Geert Uytterhoeven]

Hi Michael,

On Fri, Aug 18, 2023 at 9:14 AM Michael Schmitz <schmitzmic@gmail.com> wrote:
> 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>

Iff this patch is accepted, it must go in before [PATCH v2 2/3], else
the latter would cause a regression. And backporting to stable must
take that into account, too.

Alternative, only consider pdev->id if it's positive?

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 v2 3/3] m68k/atari: change Falcon IDE platform device to id 0

[Michael Schmitz]

Hi Geert,

Am 19.08.2023 um 02:44 schrieb Geert Uytterhoeven:
> Hi Michael,
>
> On Fri, Aug 18, 2023 at 9:14 AM Michael Schmitz <schmitzmic@gmail.com> wrote:
>> 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>
>
> Iff this patch is accepted, it must go in before [PATCH v2 2/3], else
> the latter would cause a regression. And backporting to stable must
> take that into account, too.

I don't see it as a regression - the driver still works OK, it's only 
the byte swap option that's broken on Atari, and that's newly introduced.

But this patch changes the user space exposed platform device name, as 
you point out elsewhere. That's reason enough to drop it.

> Alternative, only consider pdev->id if it's positive?

Only shift by two bits if pdev->id is > 0? (some consider 0 positive...)

I'll rewrite that bit and respin.

Cheers,

	Michael


>
> 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 v2 1/3] ata: pata_falcon: fix IO base selection for Q40

[Michael Schmitz]

Hi Finn,

Am 18.08.2023 um 19:52 schrieb Finn Thain:
>
> On Fri, 18 Aug 2023, Michael Schmitz wrote:
>
>> With commit 44b1fbc0f5f3 ("m68k/q40: Replace q40ide driver
>> with pata_falcon and falconide"), the Q40 IDE driver was
>> replaced by pata_falcon.c.
>>
>> Both IO and memory resources were defined for the Q40 IDE
>> platform device, but definition of the IDE register addresses
>> was modeled after the Falcon case, both in use of the memory
>> resources and in including register scale and byte vs. word
>> offset in the address.
>>
>> This was correct for the Falcon case, which does not apply
>> any address translation to the register addresses. In the
>> Q40 case, all of device base address, byte access offset
>> and register scaling is included in the platform specific
>> ISA access translation (in asm/mm_io.h).
>>
>> As a consequence, such address translation gets applied
>> twice, and register addresses are mangled.
>>
>> Use the device base address from the platform IO resource,
>> and use standard register offsets from that base in order
>> to calculate register addresses (the IO address translation
>> will then apply the correct ISA window base and scaling).
>>
>> Encode PIO_OFFSET into IO port addresses for all registers
>> except the data transfer register. Encode the MMIO offset
>> there (pata_falcon_data_xfer() directly uses raw IO with
>> no address translation).
>>
>> Reported-by: William R Sowerbutts <will@sowerbutts.com>
>> Closes: https://lore.kernel.org/r/CAMuHMdUU62jjunJh9cqSqHT87B0H0A4udOOPs=WN7WZKpcagVA@mail.gmail.com
>> Link: https://lore.kernel.org/r/CAMuHMdUU62jjunJh9cqSqHT87B0H0A4udOOPs=WN7WZKpcagVA@mail.gmail.com
>> Fixes: 44b1fbc0f5f3 ("m68k/q40: Replace q40ide driver with pata_falcon and falconide")
>> Cc: Finn Thain <fthain@linux-m68k.org>
>> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
>
> Cc: stable@vger.kernel.org
>
> From Documentation/process/submitting-patches.rst --
> "Note: Attaching a Fixes: tag does not subvert the stable kernel rules
> process nor the requirement to Cc: stable@vger.kernel.org on all stable
> patch candidates... "

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

Thanks,

	Michael

>
>> Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>
>>
>> ---
>>
>> Changes from v1:
>>
>> Damien Le Moal:
>> - change patch title
>> - drop stable backport tag
>>
>> Changes from RFC v3:
>>
>> - split off byte swap option into separate patch
>>
>> Geert Uytterhoeven:
>> - review comments
>>
>> Changes from RFC v2:
>> - add driver parameter 'data_swap' as bit mask for drives to swap
>>
>> Changes from RFC v1:
>>
>> Finn Thain:
>> - take care to supply IO address suitable for ioread8/iowrite8
>> - use MMIO address for data transfer
>> ---
>>  drivers/ata/pata_falcon.c | 55 ++++++++++++++++++++++++---------------
>>  1 file changed, 34 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/ata/pata_falcon.c b/drivers/ata/pata_falcon.c
>> index 996516e64f13..346259e3bbc8 100644
>> --- a/drivers/ata/pata_falcon.c
>> +++ b/drivers/ata/pata_falcon.c
>> @@ -123,8 +123,8 @@ static int __init pata_falcon_init_one(struct platform_device *pdev)
>>  	struct resource *base_res, *ctl_res, *irq_res;
>>  	struct ata_host *host;
>>  	struct ata_port *ap;
>> -	void __iomem *base;
>> -	int irq = 0;
>> +	void __iomem *base, *ctl_base;
>> +	int irq = 0, io_offset = 1, reg_scale = 4;
>>
>>  	dev_info(&pdev->dev, "Atari Falcon and Q40/Q60 PATA controller\n");
>>
>> @@ -165,26 +165,39 @@ static int __init pata_falcon_init_one(struct platform_device *pdev)
>>  	ap->pio_mask = ATA_PIO4;
>>  	ap->flags |= ATA_FLAG_SLAVE_POSS | ATA_FLAG_NO_IORDY;
>>
>> -	base = (void __iomem *)base_mem_res->start;
>>  	/* N.B. this assumes data_addr will be used for word-sized I/O only */
>> -	ap->ioaddr.data_addr		= base + 0 + 0 * 4;
>> -	ap->ioaddr.error_addr		= base + 1 + 1 * 4;
>> -	ap->ioaddr.feature_addr		= base + 1 + 1 * 4;
>> -	ap->ioaddr.nsect_addr		= base + 1 + 2 * 4;
>> -	ap->ioaddr.lbal_addr		= base + 1 + 3 * 4;
>> -	ap->ioaddr.lbam_addr		= base + 1 + 4 * 4;
>> -	ap->ioaddr.lbah_addr		= base + 1 + 5 * 4;
>> -	ap->ioaddr.device_addr		= base + 1 + 6 * 4;
>> -	ap->ioaddr.status_addr		= base + 1 + 7 * 4;
>> -	ap->ioaddr.command_addr		= base + 1 + 7 * 4;
>> -
>> -	base = (void __iomem *)ctl_mem_res->start;
>> -	ap->ioaddr.altstatus_addr	= base + 1;
>> -	ap->ioaddr.ctl_addr		= base + 1;
>> -
>> -	ata_port_desc(ap, "cmd 0x%lx ctl 0x%lx",
>> -		      (unsigned long)base_mem_res->start,
>> -		      (unsigned long)ctl_mem_res->start);
>> +	ap->ioaddr.data_addr = (void __iomem *)base_mem_res->start;
>> +
>> +	if (base_res) {		/* only Q40 has IO resources */
>> +		io_offset = 0x10000;
>> +		reg_scale = 1;
>> +		base = (void __iomem *)base_res->start;
>> +		ctl_base = (void __iomem *)ctl_res->start;
>> +
>> +		ata_port_desc(ap, "cmd %pa ctl %pa",
>> +			      &base_res->start,
>> +			      &ctl_res->start);
>> +	} else {
>> +		base = (void __iomem *)base_mem_res->start;
>> +		ctl_base = (void __iomem *)ctl_mem_res->start;
>> +
>> +		ata_port_desc(ap, "cmd %pa ctl %pa",
>> +			      &base_mem_res->start,
>> +			      &ctl_mem_res->start);
>> +	}
>> +
>> +	ap->ioaddr.error_addr	= base + io_offset + 1 * reg_scale;
>> +	ap->ioaddr.feature_addr	= base + io_offset + 1 * reg_scale;
>> +	ap->ioaddr.nsect_addr	= base + io_offset + 2 * reg_scale;
>> +	ap->ioaddr.lbal_addr	= base + io_offset + 3 * reg_scale;
>> +	ap->ioaddr.lbam_addr	= base + io_offset + 4 * reg_scale;
>> +	ap->ioaddr.lbah_addr	= base + io_offset + 5 * reg_scale;
>> +	ap->ioaddr.device_addr	= base + io_offset + 6 * reg_scale;
>> +	ap->ioaddr.status_addr	= base + io_offset + 7 * reg_scale;
>> +	ap->ioaddr.command_addr	= base + io_offset + 7 * reg_scale;
>> +
>> +	ap->ioaddr.altstatus_addr	= ctl_base + io_offset;
>> +	ap->ioaddr.ctl_addr		= ctl_base + io_offset;
>>
>>  	irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>>  	if (irq_res && irq_res->start > 0) {
>>

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

[Michael Schmitz]

Hi Finn,

Am 18.08.2023 um 19:43 schrieb Finn Thain:
>
> On Fri, 18 Aug 2023, 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 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 346259e3bbc8..27443cb757de 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),
>>  };
>> @@ -46,11 +51,12 @@ static unsigned int pata_falcon_data_xfer(struct ata_queued_cmd *qc,
>>  	void __iomem *data_addr = ap->ioaddr.data_addr;
>>  	unsigned int words = buflen >> 1;
>>  	struct scsi_cmnd *cmd = qc->scsicmd;
>> +	int dev_id = dev->devno;
>
> Is that variable really needed?

Not really, no.

>
>>  	bool swap = 1;
>>
>>  	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_id);
>>
>>  	/* Transfer multiple of 2 bytes */
>>  	if (rw == READ) {
>> @@ -199,6 +205,8 @@ 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;
>>
>> +	ap->private_data = (void *)(uintptr_t)(pata_falcon_swap_mask >> (2 * pdev->id));
>> +
>
> My compiler doesn't need that extra type cast in there...

Geert's suggestion for 64 bit safety...

Cheers,

	Michael

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

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

[Finn Thain]

[-- Attachment #1: Type: text/plain, Size: 973 bytes --]


On Fri, 18 Aug 2023, Geert Uytterhoeven wrote:

> On Fri, Aug 18, 2023 at 9:43 AM Finn Thain <fthain@linux-m68k.org> wrote:
> > On Fri, 18 Aug 2023, Michael Schmitz wrote:

> > > --- a/drivers/ata/pata_falcon.c
> > > +++ b/drivers/ata/pata_falcon.c
> > > @@ -199,6 +205,8 @@ 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;
> > >
> > > +     ap->private_data = (void *)(uintptr_t)(pata_falcon_swap_mask >> (2 * pdev->id));
> > > +
> >
> > My compiler doesn't need that extra type cast in there...
> 
> Because it's a 32-bit compiler ;-)
> With a 64-bit compiler, you would get
> 
>     warning: cast to pointer from integer of different size
> [-Wint-to-pointer-cast]
> 
> Alternatively, you can change pata_falcon_swap_mask from int to long.
> 

I see. Thanks for clarifying that for me.

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

[Finn Thain]

On Sat, 19 Aug 2023, Michael Schmitz wrote:

> >
> > Iff this patch is accepted, it must go in before [PATCH v2 2/3], else 
> > the latter would cause a regression. And backporting to stable must 
> > take that into account, too.
> 
> I don't see it as a regression - the driver still works OK, it's only 
> the byte swap option that's broken on Atari, and that's newly 
> introduced.
> 
> But this patch changes the user space exposed platform device name, as 
> you point out elsewhere. That's reason enough to drop it.
> 

Such a script would be broken on q40 already.

When a fix has a dependency on a separate patch you can backport both by 
specifying a cherry-picking sequence. We may need Geert to add those 
commit hashes though. This is discussed in 
Documentation/process/stable-kernel-rules.rst

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

[Michael Schmitz]

Hi Finn,

Am 19.08.2023 um 12:07 schrieb Finn Thain:
>
> On Sat, 19 Aug 2023, Michael Schmitz wrote:
>
>>>
>>> Iff this patch is accepted, it must go in before [PATCH v2 2/3], else
>>> the latter would cause a regression. And backporting to stable must
>>> take that into account, too.
>>
>> I don't see it as a regression - the driver still works OK, it's only
>> the byte swap option that's broken on Atari, and that's newly
>> introduced.
>>
>> But this patch changes the user space exposed platform device name, as
>> you point out elsewhere. That's reason enough to drop it.
>>
>
> Such a script would be broken on q40 already.

Not necessarily - what I thought of was something like the Debian 
installer.

I don't know for a fact that it relies on the device naming as it is at 
present (or even that it supports Q40), but I'd rather not needlessly 
change something that might break user space code.

> When a fix has a dependency on a separate patch you can backport both by
> specifying a cherry-picking sequence. We may need Geert to add those
> commit hashes though. This is discussed in
> Documentation/process/stable-kernel-rules.rst

If Geert prefers to change the Atari IDE platform device ID, he's always 
free to do that :-)

The current patch will work either way.

Cheers,

	Michael