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

Michael Schmitz <schmitzmic@xxxxxxxxx> · Fri, 18 Aug 2023 07:14:56 +1200

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@xxxxxxxxxxxxxx>
Cc: Finn Thain <fthain@xxxxxxxxxxxxxx>
Cc: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
Signed-off-by: Michael Schmitz <schmitzmic@xxxxxxxxx>

---

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;






