| Message ID | 20230817221232.22035-3-schmitzmic@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Q40 IDE fixes | expand |
On 2023/08/18 7:12, 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> > Signed-off-by: Michael Schmitz <schmitzmic@gmail.com> > > --- > > Changes since RFC v4: > > Geert Uytterhoeven: > - don't shift static module parameter for drive 3/4 bitmask > - simplify bit mask calculation to always use pdev->id > > Finn Thain: > - correct bit numbers for drive 3/4 > > Changes since RFC 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 | 26 +++++++++++++++++++++++++- > 1 file changed, 25 insertions(+), 1 deletion(-) > > diff --git a/drivers/ata/pata_falcon.c b/drivers/ata/pata_falcon.c > index 346259e3bbc8..90488f565d6f 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,6 +178,13 @@ 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; > > @@ -199,6 +219,10 @@ 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; > > + priv->swap_mask = pata_falcon_swap_mask >> (2 * pdev->id); I do not understand the shift here. It seems that it will lead to priv->swap_mask always being 0... > + if (priv->swap_mask) > + priv->swap_data = 1; I do not understand why priv->swap_data is needed, given that it is set to 1 if priv->swap_mask != 0, the above: swap = priv->swap_data && (priv->swap_mask & BIT(dev_id)); should be equivalent to the simpler: swap = priv->swap_mask & BIT(dev_id); No ? Am I missing something ? > + > irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0); > if (irq_res && irq_res->start > 0) { > irq = irq_res->start;
Hi Damien, thanks for the review ... Am 18.08.2023 um 12:51 schrieb Damien Le Moal: > On 2023/08/18 7:12, 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> >> Signed-off-by: Michael Schmitz <schmitzmic@gmail.com> >> >> --- >> >> Changes since RFC v4: >> >> Geert Uytterhoeven: >> - don't shift static module parameter for drive 3/4 bitmask >> - simplify bit mask calculation to always use pdev->id >> >> Finn Thain: >> - correct bit numbers for drive 3/4 >> >> Changes since RFC 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 | 26 +++++++++++++++++++++++++- >> 1 file changed, 25 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/ata/pata_falcon.c b/drivers/ata/pata_falcon.c >> index 346259e3bbc8..90488f565d6f 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,6 +178,13 @@ 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; >> >> @@ -199,6 +219,10 @@ 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; >> >> + priv->swap_mask = pata_falcon_swap_mask >> (2 * pdev->id); > > I do not understand the shift here. It seems that it will lead to > priv->swap_mask always being 0... On Q40, it is possible to have two ISA IDE adapters, and two platform devices are defined (in arch/m68k/q40/config.c) One will have pdev->id==0, the other will have pdev->id==1. In the pdev->id==0 case, there's no shift of the bit mask passed in as module parameter, so the data transfer function will examine bits 0 and 1. That case we've verified in testing. In the other case, we shift down by two bits, so the data transfer function will now examine bits 2 and 3 from the module parameter. > >> + if (priv->swap_mask) >> + priv->swap_data = 1; > > I do not understand why priv->swap_data is needed, given that it is set to 1 if > priv->swap_mask != 0, the above: > > swap = priv->swap_data && (priv->swap_mask & BIT(dev_id)); > > should be equivalent to the simpler: > > swap = priv->swap_mask & BIT(dev_id); > > No ? Am I missing something ? I had hoped to avoid the pointer dereference and bit shift in the default (and by far most common) case where none of the bits are set. Compared to the simpler version, I actually just save the bit shift, so it's probably a pointless optimization. Finn had suggested to simplify this even further, and use ap->private as bit mask directly (saving the kzalloc()). If you're OK with that, I'll change the code accordingly. Cheers, Michael >> + >> irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0); >> if (irq_res && irq_res->start > 0) { >> irq = irq_res->start; >
On 2023/08/18 12:08, Michael Schmitz wrote: > Hi Damien, > > thanks for the review ... > > Am 18.08.2023 um 12:51 schrieb Damien Le Moal: >> On 2023/08/18 7:12, 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> >>> Signed-off-by: Michael Schmitz <schmitzmic@gmail.com> >>> >>> --- >>> >>> Changes since RFC v4: >>> >>> Geert Uytterhoeven: >>> - don't shift static module parameter for drive 3/4 bitmask >>> - simplify bit mask calculation to always use pdev->id >>> >>> Finn Thain: >>> - correct bit numbers for drive 3/4 >>> >>> Changes since RFC 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 | 26 +++++++++++++++++++++++++- >>> 1 file changed, 25 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/ata/pata_falcon.c b/drivers/ata/pata_falcon.c >>> index 346259e3bbc8..90488f565d6f 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,6 +178,13 @@ 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; >>> >>> @@ -199,6 +219,10 @@ 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; >>> >>> + priv->swap_mask = pata_falcon_swap_mask >> (2 * pdev->id); >> >> I do not understand the shift here. It seems that it will lead to >> priv->swap_mask always being 0... > > On Q40, it is possible to have two ISA IDE adapters, and two platform > devices are defined (in arch/m68k/q40/config.c) One will have > pdev->id==0, the other will have pdev->id==1. > > In the pdev->id==0 case, there's no shift of the bit mask passed in as > module parameter, so the data transfer function will examine bits 0 and > 1. That case we've verified in testing. > In the other case, we shift down by two bits, so the data transfer > function will now examine bits 2 and 3 from the module parameter. > >> >>> + if (priv->swap_mask) >>> + priv->swap_data = 1; >> >> I do not understand why priv->swap_data is needed, given that it is set to 1 if >> priv->swap_mask != 0, the above: >> >> swap = priv->swap_data && (priv->swap_mask & BIT(dev_id)); >> >> should be equivalent to the simpler: >> >> swap = priv->swap_mask & BIT(dev_id); >> >> No ? Am I missing something ? > > I had hoped to avoid the pointer dereference and bit shift in the > default (and by far most common) case where none of the bits are set. > > Compared to the simpler version, I actually just save the bit shift, so > it's probably a pointless optimization. > > Finn had suggested to simplify this even further, and use ap->private as > bit mask directly (saving the kzalloc()). If you're OK with that, I'll > change the code accordingly. Sounds good. And given that we are talking about old IDE devices, which are *really* slow, I would not worry too much about optimizing for pointer dereference :) > > Cheers, > > Michael > > >>> + >>> irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0); >>> if (irq_res && irq_res->start > 0) { >>> irq = irq_res->start; >>
Hi Damien, Am 18.08.2023 um 15:15 schrieb Damien Le Moal: > On 2023/08/18 12:08, Michael Schmitz wrote: >> Hi Damien, >> >> thanks for the review ... >> >> Am 18.08.2023 um 12:51 schrieb Damien Le Moal: >>> On 2023/08/18 7:12, 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> >>>> Signed-off-by: Michael Schmitz <schmitzmic@gmail.com> >>>> >>>> --- >>>> >>>> Changes since RFC v4: >>>> >>>> Geert Uytterhoeven: >>>> - don't shift static module parameter for drive 3/4 bitmask >>>> - simplify bit mask calculation to always use pdev->id >>>> >>>> Finn Thain: >>>> - correct bit numbers for drive 3/4 >>>> >>>> Changes since RFC 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 | 26 +++++++++++++++++++++++++- >>>> 1 file changed, 25 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/ata/pata_falcon.c b/drivers/ata/pata_falcon.c >>>> index 346259e3bbc8..90488f565d6f 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,6 +178,13 @@ 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; >>>> >>>> @@ -199,6 +219,10 @@ 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; >>>> >>>> + priv->swap_mask = pata_falcon_swap_mask >> (2 * pdev->id); >>> >>> I do not understand the shift here. It seems that it will lead to >>> priv->swap_mask always being 0... >> >> On Q40, it is possible to have two ISA IDE adapters, and two platform >> devices are defined (in arch/m68k/q40/config.c) One will have >> pdev->id==0, the other will have pdev->id==1. >> >> In the pdev->id==0 case, there's no shift of the bit mask passed in as >> module parameter, so the data transfer function will examine bits 0 and >> 1. That case we've verified in testing. >> In the other case, we shift down by two bits, so the data transfer >> function will now examine bits 2 and 3 from the module parameter. >> >>> >>>> + if (priv->swap_mask) >>>> + priv->swap_data = 1; >>> >>> I do not understand why priv->swap_data is needed, given that it is set to 1 if >>> priv->swap_mask != 0, the above: >>> >>> swap = priv->swap_data && (priv->swap_mask & BIT(dev_id)); >>> >>> should be equivalent to the simpler: >>> >>> swap = priv->swap_mask & BIT(dev_id); >>> >>> No ? Am I missing something ? >> >> I had hoped to avoid the pointer dereference and bit shift in the >> default (and by far most common) case where none of the bits are set. >> >> Compared to the simpler version, I actually just save the bit shift, so >> it's probably a pointless optimization. >> >> Finn had suggested to simplify this even further, and use ap->private as >> bit mask directly (saving the kzalloc()). If you're OK with that, I'll >> change the code accordingly. > > Sounds good. And given that we are talking about old IDE devices, which are > *really* slow, I would not worry too much about optimizing for pointer > dereference :) The drive's probably faster than the CPU, at least the one I'm currently using :) Regardless, I'll change this, test and respin. Cheers, Michael > >> >> Cheers, >> >> Michael >> >> >>>> + >>>> irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0); >>>> if (irq_res && irq_res->start > 0) { >>>> irq = irq_res->start; >>> >
On 8/18/23 1:12 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> > Signed-off-by: Michael Schmitz <schmitzmic@gmail.com> > > --- > > Changes since RFC v4: > > Geert Uytterhoeven: > - don't shift static module parameter for drive 3/4 bitmask > - simplify bit mask calculation to always use pdev->id > > Finn Thain: > - correct bit numbers for drive 3/4 > > Changes since RFC 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 | 26 +++++++++++++++++++++++++- > 1 file changed, 25 insertions(+), 1 deletion(-) > > diff --git a/drivers/ata/pata_falcon.c b/drivers/ata/pata_falcon.c > index 346259e3bbc8..90488f565d6f 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)"); Hm, Greg KH keeps saying us that the module parameters belong to '90s. :-) [...] > @@ -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; You hardly need this variable... > 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)); This looks convoluted -- only the 2nd subexpression should be enough... [...] > @@ -165,6 +178,13 @@ 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); sizeof(*priv) is preferred IIRC... > + if (!priv) > + return -ENOMEM; > + > + ap->private_data = priv; Also you hardly need a heap allocation -- encoding your couple flags could use the ap->private_data itself... [...] MBR, Sergey
Hi Sergey, thanks for reviewing - this has mostly been addressed in v2 or v3 (which I forgot to send to you, sorry). Damien asked for the patch title to be changed (now 'ata: pata_falcon: add data_swab option to byte-swap disk data) so you might have missed it on the list. On 21/08/23 06:07, Sergey Shtylyov wrote: > On 8/18/23 1:12 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> >> Signed-off-by: Michael Schmitz <schmitzmic@gmail.com> >> >> --- >> >> Changes since RFC v4: >> >> Geert Uytterhoeven: >> - don't shift static module parameter for drive 3/4 bitmask >> - simplify bit mask calculation to always use pdev->id >> >> Finn Thain: >> - correct bit numbers for drive 3/4 >> >> Changes since RFC 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 | 26 +++++++++++++++++++++++++- >> 1 file changed, 25 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/ata/pata_falcon.c b/drivers/ata/pata_falcon.c >> index 346259e3bbc8..90488f565d6f 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)"); > Hm, Greg KH keeps saying us that the module parameters belong to '90s. :-) What else can I use that would allow setting a driver parameter at boot time? This driver will be built-in pretty much all the time. > > [...] >> @@ -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; > You hardly need this variable... Fixed in v3. > >> 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)); > This looks convoluted -- only the 2nd subexpression should be enough... Pointless attempt at optimizing this for the default case. Gone now. > > [...] >> @@ -165,6 +178,13 @@ 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); > sizeof(*priv) is preferred IIRC... > >> + if (!priv) >> + return -ENOMEM; >> + >> + ap->private_data = priv; > Also you hardly need a heap allocation -- encoding your couple flags > could use the ap->private_data itself... That's what Finn suggested as well - changed in v2. Cheers, Michael > > [...] > > MBR, Sergey
Hello! On 8/20/23 10:27 PM, Michael Schmitz wrote: [...] > thanks for reviewing - this has mostly been addressed in v2 or v3 (which I forgot to send to you, sorry). Damien asked for the patch title to be changed (now 'ata: pata_falcon: add data_swab option to byte-swap disk data) so you might have missed it on the list. I didn't want to repeat such request after him. :-) I'm subscribed to linux-ide thru my Gmail account, and I'm still not seeing your further patch versions on the list... :-/ [...] >>> 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> >>> Signed-off-by: Michael Schmitz <schmitzmic@gmail.com> [...] >>> diff --git a/drivers/ata/pata_falcon.c b/drivers/ata/pata_falcon.c >>> index 346259e3bbc8..90488f565d6f 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)"); >> Hm, Greg KH keeps saying us that the module parameters belong to '90s. :-) > What else can I use that would allow setting a driver parameter at boot time? This driver will be built-in pretty much all the time. I guess he means sysfs... [...] MBR, Sergey
On 8/22/23 10:10 PM, Sergei Shtylyov wrote: [...] >> thanks for reviewing - this has mostly been addressed in v2 or v3 (which I forgot to send to you, sorry). Damien asked for the patch title to be changed (now 'ata: pata_falcon: add data_swab option to byte-swap disk data) so you might have missed it on the list. > > I didn't want to repeat such request after him. :-) > I'm subscribed to linux-ide thru my Gmail account, and I'm still not seeing > your further patch versions on the list... :-/ Had to reply from Gmail account as the OMP server rejected my msg. Please be sure to always CC: me on the PATA patches! > [...] > >>>> 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> >>>> Signed-off-by: Michael Schmitz <schmitzmic@gmail.com> [...] MBR, Sergey
Hi Sergey, Am 23.08.2023 um 07:44 schrieb Sergei Shtylyov: > On 8/22/23 10:10 PM, Sergei Shtylyov wrote: > [...] > >>> thanks for reviewing - this has mostly been addressed in v2 or v3 (which I forgot to send to you, sorry). Damien asked for the patch title to be changed (now 'ata: pata_falcon: add data_swab option to byte-swap disk data) so you might have missed it on the list. >> >> I didn't want to repeat such request after him. :-) >> I'm subscribed to linux-ide thru my Gmail account, and I'm still not seeing >> your further patch versions on the list... :-/ > > Had to reply from Gmail account as the OMP server rejected my msg. > Please be sure to always CC: me on the PATA patches! Will do - v4 to go out shortly (afer a final boot test). Cheers, Michael
diff --git a/drivers/ata/pata_falcon.c b/drivers/ata/pata_falcon.c index 346259e3bbc8..90488f565d6f 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,6 +178,13 @@ 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; @@ -199,6 +219,10 @@ 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; + priv->swap_mask = pata_falcon_swap_mask >> (2 * pdev->id); + 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;
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> Signed-off-by: Michael Schmitz <schmitzmic@gmail.com> --- Changes since RFC v4: Geert Uytterhoeven: - don't shift static module parameter for drive 3/4 bitmask - simplify bit mask calculation to always use pdev->id Finn Thain: - correct bit numbers for drive 3/4 Changes since RFC 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 | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-)