Message ID | 20230822221359.31024-3-schmitzmic@gmail.com |
---|---|
State | New |
Headers | show |
Series | [v4,1/2] ata: pata_falcon: fix IO base selection for Q40 | expand |
On Wed, Aug 23, 2023 at 12:14 AM Michael Schmitz <schmitzmic@gmail.com> 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> > Tested-by: William R Sowerbutts <will@sowerbutts.com> > Signed-off-by: Michael Schmitz <schmitzmic@gmail.com> > > --- > > Changes since v2: > > Geert Uytterhoeven: > - only shift swap bitmask if pdev->id > 0 Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Gr{oetje,eeting}s, Geert
On Wed, Aug 23, 2023 at 11:10 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Wed, Aug 23, 2023 at 12:14 AM Michael Schmitz <schmitzmic@gmail.com> 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> > > Tested-by: William R Sowerbutts <will@sowerbutts.com> > > Signed-off-by: Michael Schmitz <schmitzmic@gmail.com> > > > > --- > > > > Changes since v2: > > > > Geert Uytterhoeven: > > - only shift swap bitmask if pdev->id > 0 > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Oops, I meant Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org> But it never hurts to have more review tags ;-) Gr{oetje,eeting}s, Geert
On 8/23/23 1:13 AM, 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> > Tested-by: William R Sowerbutts <will@sowerbutts.com> > Signed-off-by: Michael Schmitz <schmitzmic@gmail.com> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru> [...] > diff --git a/drivers/ata/pata_falcon.c b/drivers/ata/pata_falcon.c > index 3841ea200bcb..7cf15bd9764a 100644 > --- a/drivers/ata/pata_falcon.c > +++ b/drivers/ata/pata_falcon.c [...] > @@ -194,6 +199,9 @@ static int __init pata_falcon_init_one(struct platform_device *pdev) > ata_port_desc(ap, "cmd %px ctl %px data %pa", > base, ctl_base, &ap->ioaddr.data_addr); > > + ap->private_data = (void *)(uintptr_t)(pdev->id > 0 ? > + pata_falcon_swap_mask >> 2 : pata_falcon_swap_mask); How about: ap->private_data = (void *)(uintptr_t)(pata_falcon_swap_mask >> (pdev->id > 0 ? 2 : 0)); [...] MBR, Sergey
On 8/24/23 01:35, Sergey Shtylyov wrote: > On 8/23/23 1:13 AM, 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> >> Tested-by: William R Sowerbutts <will@sowerbutts.com> >> Signed-off-by: Michael Schmitz <schmitzmic@gmail.com> > > Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru> > > [...] > >> diff --git a/drivers/ata/pata_falcon.c b/drivers/ata/pata_falcon.c >> index 3841ea200bcb..7cf15bd9764a 100644 >> --- a/drivers/ata/pata_falcon.c >> +++ b/drivers/ata/pata_falcon.c > [...] >> @@ -194,6 +199,9 @@ static int __init pata_falcon_init_one(struct platform_device *pdev) >> ata_port_desc(ap, "cmd %px ctl %px data %pa", >> base, ctl_base, &ap->ioaddr.data_addr); >> >> + ap->private_data = (void *)(uintptr_t)(pdev->id > 0 ? >> + pata_falcon_swap_mask >> 2 : pata_falcon_swap_mask); > > How about: > > ap->private_data = (void *)(uintptr_t)(pata_falcon_swap_mask >> > (pdev->id > 0 ? 2 : 0)); This is so hard to decode... Let's please spell this out. Something like: int shift; if (pdev->id) shift = 2; else shift = 0; ap->private_data = (uintptr_t)(pata_falcon_swap_mask >> shift); This is initialization, so no need to try to optimize and rather privilege clear code. > > [...] > > MBR, Sergey
Hi Damien, On 24/08/23 11:03, Damien Le Moal wrote: > On 8/24/23 01:35, Sergey Shtylyov wrote: >> On 8/23/23 1:13 AM, 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> >>> Tested-by: William R Sowerbutts <will@sowerbutts.com> >>> Signed-off-by: Michael Schmitz <schmitzmic@gmail.com> >> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru> >> >> [...] >> >>> diff --git a/drivers/ata/pata_falcon.c b/drivers/ata/pata_falcon.c >>> index 3841ea200bcb..7cf15bd9764a 100644 >>> --- a/drivers/ata/pata_falcon.c >>> +++ b/drivers/ata/pata_falcon.c >> [...] >>> @@ -194,6 +199,9 @@ static int __init pata_falcon_init_one(struct platform_device *pdev) >>> ata_port_desc(ap, "cmd %px ctl %px data %pa", >>> base, ctl_base, &ap->ioaddr.data_addr); >>> >>> + ap->private_data = (void *)(uintptr_t)(pdev->id > 0 ? >>> + pata_falcon_swap_mask >> 2 : pata_falcon_swap_mask); >> How about: >> >> ap->private_data = (void *)(uintptr_t)(pata_falcon_swap_mask >> >> (pdev->id > 0 ? 2 : 0)); > This is so hard to decode... Let's please spell this out. > Something like: > > int shift; > > if (pdev->id) Atari Falcon has pdev->id==-1, so this must be 'if (pdev->id > 0)' here. (Testing for pdev->id nonzero did work in earlier versions of my patch because patch 3 changed the platform device ID on Atari. That patch had the potential to confuse user space so I dropped it from v3 on.) > shift = 2; > else > shift = 0; No need for the 'else' if we initialize shift above (as is done for irq, io_offset and reg_shift). > ap->private_data = (uintptr_t)(pata_falcon_swap_mask >> shift); > > This is initialization, so no need to try to optimize and rather privilege clear > code. Hmm - or maybe I'll leave that spelled out as you suggest. Cheers, Michael > >> [...] >> >> MBR, Sergey
Hi Geert, On 23/08/23 21:10, Geert Uytterhoeven wrote: > On Wed, Aug 23, 2023 at 11:10 AM Geert Uytterhoeven > <geert@linux-m68k.org> wrote: >> On Wed, Aug 23, 2023 at 12:14 AM Michael Schmitz <schmitzmic@gmail.com> 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> >>> Tested-by: William R Sowerbutts <will@sowerbutts.com> >>> Signed-off-by: Michael Schmitz <schmitzmic@gmail.com> >>> >>> --- >>> >>> Changes since v2: >>> >>> Geert Uytterhoeven: >>> - only shift swap bitmask if pdev->id > 0 >> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > Oops, I meant > Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org> > But it never hurts to have more review tags ;-) Thanks - I can pretty much type that one blind ... Cheers, Michael > > Gr{oetje,eeting}s, > > Geert >
diff --git a/drivers/ata/pata_falcon.c b/drivers/ata/pata_falcon.c index 3841ea200bcb..7cf15bd9764a 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), }; @@ -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 = (uintptr_t)ap->private_data & BIT(dev->devno); /* Transfer multiple of 2 bytes */ if (rw == READ) { @@ -194,6 +199,9 @@ static int __init pata_falcon_init_one(struct platform_device *pdev) ata_port_desc(ap, "cmd %px ctl %px data %pa", base, ctl_base, &ap->ioaddr.data_addr); + ap->private_data = (void *)(uintptr_t)(pdev->id > 0 ? + pata_falcon_swap_mask >> 2 : pata_falcon_swap_mask); + irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0); if (irq_res && irq_res->start > 0) { irq = irq_res->start;