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

[Finn Thain <fthain@linux-m68k.org>]

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...)