[PATCH RFC v3 0/2] Q40 IDE fixes

[PATCH RFC v3 0/2] Q40 IDE fixes

[Michael Schmitz]

v4 of the Q40 IDE patch, split into two parts. First patch just fixes the
bug introduced when converting Q40 to use pata_falcon, second patch adds the
byte-swapping option requested by William.

Please review this again, just to make sure I haven't messed up anything
when applying the review comments to the split patches.

Cheers,

   Michael

[PATCH RFC v3 1/2] 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 (and the later obsoleted
falconide.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 v3:

- split off byte swap option into separate patch

Geert Uytterhoeven:
- review comments

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

Changes from 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 RFC v3 2/2] 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 classic
little endian byte order, whereas legacy disks use 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 3 and 4.

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

diff --git a/drivers/ata/pata_falcon.c b/drivers/ata/pata_falcon.c
index 346259e3bbc8..cec3c3a6eed9 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,10 +178,20 @@ 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;
 
 	if (base_res) {		/* only Q40 has IO resources */
+		if (pdev->id)
+			pata_falcon_swap_mask >>= 2;
+		priv->swap_mask = pata_falcon_swap_mask;
 		io_offset = 0x10000;
 		reg_scale = 1;
 		base = (void __iomem *)base_res->start;
@@ -178,6 +201,7 @@ static int __init pata_falcon_init_one(struct platform_device *pdev)
 			      &base_res->start,
 			      &ctl_res->start);
 	} else {
+		priv->swap_mask = pata_falcon_swap_mask;
 		base = (void __iomem *)base_mem_res->start;
 		ctl_base = (void __iomem *)ctl_mem_res->start;
 
@@ -199,6 +223,9 @@ 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;
 
+	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 RFC v3 2/2] m68k/q40: add data_swab option for pata_falcon to byte-swap disk data

[Finn Thain]

On Thu, 17 Aug 2023, Michael Schmitz wrote:

> Some users of pata_falcon on Q40 have IDE disks in classic
> little endian byte order, whereas legacy disks use 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 3 and 4.

Many would say that's off by one, as it's popular to number the LSB as bit 
zero.

> 
> 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 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 | 29 ++++++++++++++++++++++++++++-
>  1 file changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/ata/pata_falcon.c b/drivers/ata/pata_falcon.c
> index 346259e3bbc8..cec3c3a6eed9 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,10 +178,20 @@ 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;
>  
>  	if (base_res) {		/* only Q40 has IO resources */
> +		if (pdev->id)
> +			pata_falcon_swap_mask >>= 2;
> +		priv->swap_mask = pata_falcon_swap_mask;
>  		io_offset = 0x10000;
>  		reg_scale = 1;
>  		base = (void __iomem *)base_res->start;
> @@ -178,6 +201,7 @@ static int __init pata_falcon_init_one(struct platform_device *pdev)
>  			      &base_res->start,
>  			      &ctl_res->start);
>  	} else {
> +		priv->swap_mask = pata_falcon_swap_mask;
>  		base = (void __iomem *)base_mem_res->start;
>  		ctl_base = (void __iomem *)ctl_mem_res->start;
>  
> @@ -199,6 +223,9 @@ 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;
>  
> +	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;
> 

I think the patch could be simplified by dropping the dynamic memory 
allocation...

diff --git a/drivers/ata/pata_falcon.c b/drivers/ata/pata_falcon.c
index 346259e3bbc8..d057f11ec02d 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_data_swab;
+
+module_param_named(data_swab, pata_falcon_data_swab, 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 = (int)ap->private_data & BIT(dev->devno);
 
 	/* Transfer multiple of 2 bytes */
 	if (rw == READ) {
@@ -199,6 +204,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 *)((pata_falcon_data_swab >> (2 * pdev->id)) & 3);
+
 	irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
 	if (irq_res && irq_res->start > 0) {
 		irq = irq_res->start;

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

[Geert Uytterhoeven]

On Thu, Aug 17, 2023 at 12:15 PM Finn Thain <fthain@linux-m68k.org> wrote:
> On Thu, 17 Aug 2023, Michael Schmitz wrote:
> > Some users of pata_falcon on Q40 have IDE disks in classic
> > little endian byte order, whereas legacy disks use 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 3 and 4.
>
> Many would say that's off by one, as it's popular to number the LSB as bit
> zero.
>
> >
> > 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

> > @@ -165,10 +178,20 @@ 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;
> >
> >       if (base_res) {         /* only Q40 has IO resources */
> > +             if (pdev->id)
> > +                     pata_falcon_swap_mask >>= 2;

Although this driver uses module_platform_driver_probe(), and thus
does not support unbind/rebind, shifting a global variable is still
fragile, and depends on probe order (what if the second interface is
probed first?).

> > +             priv->swap_mask = pata_falcon_swap_mask;

priv->swap_mask = pata_falcon_swap_mask >> (2 * pdev->id);

> --- 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_data_swab;
> +
> +module_param_named(data_swab, pata_falcon_data_swab, 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 = (int)ap->private_data & BIT(dev->devno);

"(uintptr_t)", to make it 64-bit clean.

Yeah, we don't support COMPILE_TEST=y yet for PATA_FALCON.
BTW, looks like we don't need any of the following anymore:

    #include <asm/setup.h>
    #include <asm/atarihw.h>
    #include <asm/atariints.h>
    #include <asm/atari_stdma.h>

That leaves us with <asm/ide.h>, (which does not exist on most
architectures, and seems to be mostly obsolete), for the (indirectly
included) definitions of raw_{in,out}sw_{,swapw}()....

>
>         /* Transfer multiple of 2 bytes */
>         if (rw == READ) {
> @@ -199,6 +204,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 *)((pata_falcon_data_swab >> (2 * pdev->id)) & 3);

"(void *)(uintptr_t)", to make it 64-bit clean.

> +
>         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 RFC v3 2/2] m68k/q40: add data_swab option for pata_falcon to byte-swap disk data

[Michael Schmitz]

Hi Geert,

On 17/08/23 23:47, Geert Uytterhoeven wrote:
> On Thu, Aug 17, 2023 at 12:15 PM Finn Thain <fthain@linux-m68k.org> wrote:
>> On Thu, 17 Aug 2023, Michael Schmitz wrote:
>>> Some users of pata_falcon on Q40 have IDE disks in classic
>>> little endian byte order, whereas legacy disks use 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 3 and 4.
>> Many would say that's off by one, as it's popular to number the LSB as bit
>> zero.
>>
>>> 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
>>> @@ -165,10 +178,20 @@ 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;
>>>
>>>        if (base_res) {         /* only Q40 has IO resources */
>>> +             if (pdev->id)
>>> +                     pata_falcon_swap_mask >>= 2;
> Although this driver uses module_platform_driver_probe(), and thus
> does not support unbind/rebind, shifting a global variable is still
> fragile, and depends on probe order (what if the second interface is
> probed first?).
True - I had worried about that but forgot to change it.
>
>>> +             priv->swap_mask = pata_falcon_swap_mask;
> priv->swap_mask = pata_falcon_swap_mask >> (2 * pdev->id);

The Atari platform driver data is registered with pdev->id=-1. That'll 
break here.

(Not sure why that choice of pdev->id, and whether it can be changed to 
0 easily)

Cheers,

     Michael


>
>> --- 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_data_swab;
>> +
>> +module_param_named(data_swab, pata_falcon_data_swab, 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 = (int)ap->private_data & BIT(dev->devno);
> "(uintptr_t)", to make it 64-bit clean.
>
> Yeah, we don't support COMPILE_TEST=y yet for PATA_FALCON.
> BTW, looks like we don't need any of the following anymore:
>
>      #include <asm/setup.h>
>      #include <asm/atarihw.h>
>      #include <asm/atariints.h>
>      #include <asm/atari_stdma.h>
>
> That leaves us with <asm/ide.h>, (which does not exist on most
> architectures, and seems to be mostly obsolete), for the (indirectly
> included) definitions of raw_{in,out}sw_{,swapw}()....
>
>>          /* Transfer multiple of 2 bytes */
>>          if (rw == READ) {
>> @@ -199,6 +204,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 *)((pata_falcon_data_swab >> (2 * pdev->id)) & 3);
> "(void *)(uintptr_t)", to make it 64-bit clean.
>
>> +
>>          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
>

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

[Michael Schmitz]

Hi Geert,

On 18/08/23 07:21, Michael Schmitz wrote:
>
>>>> +             priv->swap_mask = pata_falcon_swap_mask;
>> priv->swap_mask = pata_falcon_swap_mask >> (2 * pdev->id);
>
> The Atari platform driver data is registered with pdev->id=-1. That'll 
> break here.
Confirmed.
>
> (Not sure why that choice of pdev->id, and whether it can be changed 
> to 0 easily)
Seems to work OK with ID set to 0, so I'll simplify that as you suggest.

Cheers,

     Michael


> Cheers,
>
>     Michael
>
>
>>
>>> --- 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_data_swab;
>>> +
>>> +module_param_named(data_swab, pata_falcon_data_swab, 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 = (int)ap->private_data & BIT(dev->devno);
>> "(uintptr_t)", to make it 64-bit clean.
>>
>> Yeah, we don't support COMPILE_TEST=y yet for PATA_FALCON.
>> BTW, looks like we don't need any of the following anymore:
>>
>>      #include <asm/setup.h>
>>      #include <asm/atarihw.h>
>>      #include <asm/atariints.h>
>>      #include <asm/atari_stdma.h>
>>
>> That leaves us with <asm/ide.h>, (which does not exist on most
>> architectures, and seems to be mostly obsolete), for the (indirectly
>> included) definitions of raw_{in,out}sw_{,swapw}()....
>>
>>>          /* Transfer multiple of 2 bytes */
>>>          if (rw == READ) {
>>> @@ -199,6 +204,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 *)((pata_falcon_data_swab >> (2 * 
>>> pdev->id)) & 3);
>> "(void *)(uintptr_t)", to make it 64-bit clean.
>>
>>> +
>>>          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
>>

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

[Geert Uytterhoeven]

Hi Michael,

On Thu, Aug 17, 2023 at 11:28 PM Michael Schmitz <schmitzmic@gmail.com> wrote:
> On 18/08/23 07:21, Michael Schmitz wrote:
> >>>> +             priv->swap_mask = pata_falcon_swap_mask;
> >> priv->swap_mask = pata_falcon_swap_mask >> (2 * pdev->id);
> >
> > The Atari platform driver data is registered with pdev->id=-1. That'll
> > break here.
>
> Confirmed.

Oops.

> > (Not sure why that choice of pdev->id, and whether it can be changed
> > to 0 easily)
> Seems to work OK with ID set to 0, so I'll simplify that as you suggest.

-1 is used to indicate there can only ever be a single instance.
Of course 0 also works, but causes ".0" to be appended to the platform
device's name, visible in e.g. /sys/bus/platform/devices/.

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 RFC v3 2/2] m68k/q40: add data_swab option for pata_falcon to byte-swap disk data

[Finn Thain]

On Fri, 18 Aug 2023, Geert Uytterhoeven wrote:

> Of course 0 also works, but causes ".0" to be appended to the platform 
> device's name, visible in e.g. /sys/bus/platform/devices/.
> 

That seems desirable to me.

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

[Geert Uytterhoeven]

Hi Finn,

On Sat, Aug 19, 2023 at 1:53 AM Finn Thain <fthain@linux-m68k.org> wrote:
> On Fri, 18 Aug 2023, Geert Uytterhoeven wrote:
> > Of course 0 also works, but causes ".0" to be appended to the platform
> > device's name, visible in e.g. /sys/bus/platform/devices/.
>
> That seems desirable to me.

Why would it be desirable?
IMHO this is an unneeded change which is visible in userspace.

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 RFC v3 2/2] m68k/q40: add data_swab option for pata_falcon to byte-swap disk data

[Finn Thain]

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

On Mon, 21 Aug 2023, Geert Uytterhoeven wrote:

> On Sat, Aug 19, 2023 at 1:53 AM Finn Thain <fthain@linux-m68k.org> wrote:
> > On Fri, 18 Aug 2023, Geert Uytterhoeven wrote:
> > > Of course 0 also works, but causes ".0" to be appended to the platform
> > > device's name, visible in e.g. /sys/bus/platform/devices/.
> >
> > That seems desirable to me.
> 
> Why would it be desirable?
> IMHO this is an unneeded change which is visible in userspace.
> 

Only so what's visible in userspace would be consistent across atari and 
q40 (and anything else that happens to instantiate the device in future). 

Basically, I don't really understand why id -1 is better than id 0. I've 
searched Documentation/device-tree and I still don't understand why a 
singleton would need a special bus id.

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

[Michael Schmitz]

CC Adrian for comment...

Does anything in the Debian installer rely on a specific device name 
suffix in /sys/bus/platform/devices/ ?

On Atari, atari-falcon-ide does not have a suffix, but on Q40, it would 
get suffix .0 or .1 depending on what IO base the ISA IDE adapter uses.

Seeing as Q40 support in the atari-falcon-ide has been broken since it 
was introduced, this is rather about future installer support than about 
inadvertently breaking current code.

Cheers,

     Michael

On 21/08/23 23:08, Finn Thain wrote:
> On Mon, 21 Aug 2023, Geert Uytterhoeven wrote:
>
>> On Sat, Aug 19, 2023 at 1:53 AM Finn Thain <fthain@linux-m68k.org> wrote:
>>> On Fri, 18 Aug 2023, Geert Uytterhoeven wrote:
>>>> Of course 0 also works, but causes ".0" to be appended to the platform
>>>> device's name, visible in e.g. /sys/bus/platform/devices/.
>>> That seems desirable to me.
>> Why would it be desirable?
>> IMHO this is an unneeded change which is visible in userspace.
>>
> Only so what's visible in userspace would be consistent across atari and
> q40 (and anything else that happens to instantiate the device in future).
>
> Basically, I don't really understand why id -1 is better than id 0. I've
> searched Documentation/device-tree and I still don't understand why a
> singleton would need a special bus id.

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

[Finn Thain]

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


On Tue, 22 Aug 2023, Michael Schmitz wrote:

> CC Adrian for comment...
> 

My comments in the messages quoted below should not be interpreted as an 
objection to the patches Michael submitted.

I've already done too much bikeshedding there and while it's always 
tempting to do more I will desist.

This was just a look askance at the design of the driver model. So it's 
really only of academic interest.

> Does anything in the Debian installer rely on a specific device name suffix in
> /sys/bus/platform/devices/ ?
> 
> On Atari, atari-falcon-ide does not have a suffix, but on Q40, it would get
> suffix .0 or .1 depending on what IO base the ISA IDE adapter uses.
> 
> Seeing as Q40 support in the atari-falcon-ide has been broken since it was
> introduced, this is rather about future installer support than about
> inadvertently breaking current code.
> 
> Cheers,
> 
>     Michael
> 
> On 21/08/23 23:08, Finn Thain wrote:
> > On Mon, 21 Aug 2023, Geert Uytterhoeven wrote:
> >
> >> On Sat, Aug 19, 2023 at 1:53 AM Finn Thain <fthain@linux-m68k.org> wrote:
> >>> On Fri, 18 Aug 2023, Geert Uytterhoeven wrote:
> >>>> Of course 0 also works, but causes ".0" to be appended to the platform
> >>>> device's name, visible in e.g. /sys/bus/platform/devices/.
> >>> That seems desirable to me.
> >> Why would it be desirable?
> >> IMHO this is an unneeded change which is visible in userspace.
> >>
> > Only so what's visible in userspace would be consistent across atari and
> > q40 (and anything else that happens to instantiate the device in future).
> >
> > Basically, I don't really understand why id -1 is better than id 0. I've
> > searched Documentation/device-tree and I still don't understand why a
> > singleton would need a special bus id.
> 

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

[Michael Schmitz]

Hi Richard,

On 17/08/23 22:27, Richard Z wrote:
> Nitpicking now, there is nothing classic about little endian as all 
> older computers were either big or mid endian.

Right, but IDE was introduced on little endian hardware.

Maybe 'IDE default byte order' would be more exact here.

> It is rather some rare ide host adapters like the Q40 one reversed the 
> byte order of 16bit words which was cheap and convenient in the 
> 1980-9ies but not interoperable with the large majority of other host 
> adapters resulting in a reversed byte/16bit order on disk.

True - installing an IDE disk from e.g. Atari on a Mac, Amiga or PC 
wasn't something you'd have antcipated at that time. Floppy disks were 
for data interchange.

Cheers,

     Michael

>
> Richard

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

[Michael Schmitz]

Hi Finn.

On 17/08/23 22:15, Finn Thain wrote:
> On Thu, 17 Aug 2023, Michael Schmitz wrote:
>
>> Some users of pata_falcon on Q40 have IDE disks in classic
>> little endian byte order, whereas legacy disks use 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 3 and 4.
> Many would say that's off by one, as it's popular to number the LSB as bit
> zero.

Old FORTRAN programmers never die. They just cause havoc in C.

I'll fix that.

>
>> 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 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 | 29 ++++++++++++++++++++++++++++-
>>   1 file changed, 28 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/ata/pata_falcon.c b/drivers/ata/pata_falcon.c
>> index 346259e3bbc8..cec3c3a6eed9 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,10 +178,20 @@ 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;
>>   
>>   	if (base_res) {		/* only Q40 has IO resources */
>> +		if (pdev->id)
>> +			pata_falcon_swap_mask >>= 2;
>> +		priv->swap_mask = pata_falcon_swap_mask;
>>   		io_offset = 0x10000;
>>   		reg_scale = 1;
>>   		base = (void __iomem *)base_res->start;
>> @@ -178,6 +201,7 @@ static int __init pata_falcon_init_one(struct platform_device *pdev)
>>   			      &base_res->start,
>>   			      &ctl_res->start);
>>   	} else {
>> +		priv->swap_mask = pata_falcon_swap_mask;
>>   		base = (void __iomem *)base_mem_res->start;
>>   		ctl_base = (void __iomem *)ctl_mem_res->start;
>>   
>> @@ -199,6 +223,9 @@ 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;
>>   
>> +	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;
>>
> I think the patch could be simplified by dropping the dynamic memory
> allocation...

If using a private data pointer field for an int bitmask is acceptable 
use ...

I'll see if I can find any performance impact from the additional 
swap_data flag. If there's none, your solution may be cleaner.

Cheers,

     Michael


>
> diff --git a/drivers/ata/pata_falcon.c b/drivers/ata/pata_falcon.c
> index 346259e3bbc8..d057f11ec02d 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_data_swab;
> +
> +module_param_named(data_swab, pata_falcon_data_swab, 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 = (int)ap->private_data & BIT(dev->devno);
>   
>   	/* Transfer multiple of 2 bytes */
>   	if (rw == READ) {
> @@ -199,6 +204,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 *)((pata_falcon_data_swab >> (2 * pdev->id)) & 3);
> +
>   	irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>   	if (irq_res && irq_res->start > 0) {
>   		irq = irq_res->start;

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

[Finn Thain]

On Fri, 18 Aug 2023, Michael Schmitz wrote:

> > I think the patch could be simplified by dropping the dynamic memory 
> > allocation...
> 
> If using a private data pointer field for an int bitmask is acceptable 
> use ...
> 

I thought that was implicit in Geert's response.

I used a similar technique in drivers/nubus/proc.c and it didn't bother 
anyone.

> I'll see if I can find any performance impact from the additional 
> swap_data flag. If there's none, your solution may be cleaner.
> 

It was the maintenance impact of the extra code that motivated my 
suggestion. However, by avoiding kalloc we also avoid having to 
dereference the pointer in the fast path.

Re: [PATCH RFC v3 0/2] Q40 IDE fixes

[Michael Schmitz]

$subject meant to say v4, of course ...

Cheers,

	Michael

Am 17.08.2023 um 15:49 schrieb Michael Schmitz:
> v4 of the Q40 IDE patch, split into two parts. First patch just fixes the
> bug introduced when converting Q40 to use pata_falcon, second patch adds the
> byte-swapping option requested by William.
>
> Please review this again, just to make sure I haven't messed up anything
> when applying the review comments to the split patches.
>
> Cheers,
>
>    Michael
>

Re: [PATCH RFC v3 0/2] Q40 IDE fixes

[William R Sowerbutts]

I'll test this evening and report back.

With a working pata_legacy possible, I would be happy to live without 
byte-swapping in pata_falcon.

Thanks

Will

On Thu, Aug 17, 2023 at 03:49:59PM +1200, Michael Schmitz wrote:
>v4 of the Q40 IDE patch, split into two parts. First patch just fixes the
>bug introduced when converting Q40 to use pata_falcon, second patch adds the
>byte-swapping option requested by William.
>
>Please review this again, just to make sure I haven't messed up anything
>when applying the review comments to the split patches.
>
>Cheers,
>
>   Michael
>

_________________________________________________________________________
William R Sowerbutts                                  will@sowerbutts.com
"Carpe post meridiem"                               http://sowerbutts.com
         main(){char*s=">#=0> ^#X@#@^7=",c=0,m;for(;c<15;c++)for
         (m=-1;m<7;putchar(m++/6&c%3/2?10:s[c]-31&1<<m?42:32));}  

Re: [PATCH RFC v3 0/2] Q40 IDE fixes

[William R Sowerbutts]

On Thu, Aug 17, 2023 at 10:20:15AM +0100, William R Sowerbutts wrote:
>I'll test this evening and report back.

The patches work.

Thanks

Will

_________________________________________________________________________
William R Sowerbutts                                  will@sowerbutts.com
"Carpe post meridiem"                               http://sowerbutts.com
         main(){char*s=">#=0> ^#X@#@^7=",c=0,m;for(;c<15;c++)for
         (m=-1;m<7;putchar(m++/6&c%3/2?10:s[c]-31&1<<m?42:32));}  

Re: [PATCH RFC v3 0/2] Q40 IDE fixes

[Michael Schmitz]

Hi William,

On 18/08/23 08:25, William R Sowerbutts wrote:
> On Thu, Aug 17, 2023 at 10:20:15AM +0100, William R Sowerbutts wrote:
>> I'll test this evening and report back.
> The patches work.

Thanks for testing - I'll get this series off to linux-ide now.

Cheers,

     Michael

>
> Thanks
>
> Will
>
> _________________________________________________________________________
> William R Sowerbutts                                  will@sowerbutts.com
> "Carpe post meridiem"                               http://sowerbutts.com
>           main(){char*s=">#=0> ^#X@#@^7=",c=0,m;for(;c<15;c++)for
>           (m=-1;m<7;putchar(m++/6&c%3/2?10:s[c]-31&1<<m?42:32));}
>