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

Finn Thain
2023-08-16T00:23:47Z

Hi Michael,

Thanks for fixing this.

On Wed, 16 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 (and the later obsoleted
> falconide.c).

"the latter also made falconide obsolete".

>
> 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).
>
> Add module parameter 'data_swap' to allow connecting drives

How about "data_swab"? A "data swap" could be anything, and a byte swap
could be a number of things depending on the size of a word. Here we are
swapping every pair of bytes, which is called "swab" according to dd's and
libc's terminology.

> with non-native data byte order. Drives selected by the
> data_swap bit mask will have their user data swapped to host
> byte order, i.e. 'pata_falcon.data_swap=2' will swap all user
> data on drive B, leaving data on drive A in native order.
>
> 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 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 | 90 ++++++++++++++++++++++++++++++---------
> 1 file changed, 69 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/ata/pata_falcon.c b/drivers/ata/pata_falcon.c
> index 996516e64f13..e6038eca39d6 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 = 0;
> +

Looks like you need to run checkpatch (static variable initialized to 0).

> +module_param_named(data_swap, pata_falcon_swap_mask, int, 0444);
> +MODULE_PARM_DESC(data_swap, "Data byte swap enable/disable (0x1==drive1, 0x2==drive2, default==0)");

The help string does not mention that this is a bit mask (bit 0 = drive 1,
etc.)

> +
> +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 = cmd->device->sdev_target->id;
> 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 & 1<<dev_id);

I suggest writing that as priv->swap_mask & BIT(dev_id). (I wonder why
checkpatch does not ask for whitespace around the << operator...)