* Re: [PATCH RFC v3] m68k/q40: fix IO base selection for Q40 in pata_falcon.c
2023-08-15 22:32 [PATCH RFC v3] m68k/q40: fix IO base selection for Q40 in pata_falcon.c Michael Schmitz
@ 2023-08-16 0:23 ` Finn Thain
2023-08-16 4:59 ` Michael Schmitz
2023-08-16 8:07 ` Geert Uytterhoeven
2023-08-16 22:36 ` William R Sowerbutts
2 siblings, 1 reply; 13+ messages in thread
From: Finn Thain @ 2023-08-16 0:23 UTC (permalink / raw)
To: Michael Schmitz; +Cc: will, linux-m68k, rz, geert
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...)
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH RFC v3] m68k/q40: fix IO base selection for Q40 in pata_falcon.c
2023-08-16 0:23 ` Finn Thain
@ 2023-08-16 4:59 ` Michael Schmitz
2023-08-16 5:29 ` Finn Thain
0 siblings, 1 reply; 13+ messages in thread
From: Michael Schmitz @ 2023-08-16 4:59 UTC (permalink / raw)
To: Finn Thain; +Cc: will, linux-m68k, rz, geert
Hi Finn,
thanks for your review!
Am 16.08.2023 um 12:23 schrieb Finn Thain:
> 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".
I actually meant 'later' - the patch changed both falconide.c and
pata_falcon.c, and falconide.c was later obsoleted.
That's now history, may as well drop mention of falconide.c!
>>
>> 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.
Good point. I'll change that.
>> 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).
Forgot to drop that after tests before I got the parameter to work.
>
>> +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,
Right -I'll add 'bit mask' there.
> 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...)
Right - I also wonder whether using priv->swap_data to skip the bit test
in the most likely case (no byte swapping) is actually worth it.
I'll split this into bugfix and byte swap parts
Where should this go - linux-ide for sure, but is Bartlomiej still the
correct maintainer?
Cheers,
Michael
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH RFC v3] m68k/q40: fix IO base selection for Q40 in pata_falcon.c
2023-08-16 4:59 ` Michael Schmitz
@ 2023-08-16 5:29 ` Finn Thain
2023-08-16 7:27 ` Michael Schmitz
2023-08-16 7:37 ` Finn Thain
0 siblings, 2 replies; 13+ messages in thread
From: Finn Thain @ 2023-08-16 5:29 UTC (permalink / raw)
To: Michael Schmitz; +Cc: will, linux-m68k, rz, geert
On Wed, 16 Aug 2023, Michael Schmitz wrote:
>
> Right - I also wonder whether using priv->swap_data to skip the bit test in
> the most likely case (no byte swapping) is actually worth it.
>
Given it's a platform device that's only ever instantiated once, you could
just use the module-scope variable rather than device-scope.
BTW, the patch description could state some of the implications for the
default setting i.e. interoperability with the vendor operating system
(and IDE disks from Atari Falcon I guess) as opposed to interoperability
with everyone else.
> I'll split this into bugfix and byte swap parts
>
> Where should this go - linux-ide for sure, but is Bartlomiej still the
> correct maintainer?
>
The latest MAINTAINERS file says,
$ scripts/get_maintainer.pl -f drivers/ata/pata_falcon.c
Sergey Shtylyov <s.shtylyov@omp.ru> (reviewer:LIBATA PATA DRIVERS)
Damien Le Moal <dlemoal@kernel.org> (maintainer:LIBATA SUBSYSTEM (Serial and Parallel ATA drivers))
linux-ide@vger.kernel.org (open list:LIBATA PATA DRIVERS)
linux-kernel@vger.kernel.org (open list)
See also c4f9c8bbcc24 and fd86194aca1f.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH RFC v3] m68k/q40: fix IO base selection for Q40 in pata_falcon.c
2023-08-16 5:29 ` Finn Thain
@ 2023-08-16 7:27 ` Michael Schmitz
2023-08-16 7:37 ` Finn Thain
1 sibling, 0 replies; 13+ messages in thread
From: Michael Schmitz @ 2023-08-16 7:27 UTC (permalink / raw)
To: Finn Thain; +Cc: will, linux-m68k, rz, geert
Hi Finn,
Am 16.08.2023 um 17:29 schrieb Finn Thain:
>
> On Wed, 16 Aug 2023, Michael Schmitz wrote:
>
>>
>> Right - I also wonder whether using priv->swap_data to skip the bit test in
>> the most likely case (no byte swapping) is actually worth it.
>>
>
> Given it's a platform device that's only ever instantiated once, you could
> just use the module-scope variable rather than device-scope.
The Q40 platform device data specify two possible sets of IO and memory
resources, that might mean you can have up to two IDE interfaces on Q40.
Is that correct, Richard?
I'd have to change the probe logic to assign bits 1 and 2 to the first
interface, and 3 and 4 to the second to make full use of the
device-scope bitmasks.
>
> BTW, the patch description could state some of the implications for the
> default setting i.e. interoperability with the vendor operating system
> (and IDE disks from Atari Falcon I guess) as opposed to interoperability
> with everyone else.
I'll add that to the commit message.
>
>> I'll split this into bugfix and byte swap parts
>>
>> Where should this go - linux-ide for sure, but is Bartlomiej still the
>> correct maintainer?
>>
>
> The latest MAINTAINERS file says,
>
> $ scripts/get_maintainer.pl -f drivers/ata/pata_falcon.c
> Sergey Shtylyov <s.shtylyov@omp.ru> (reviewer:LIBATA PATA DRIVERS)
> Damien Le Moal <dlemoal@kernel.org> (maintainer:LIBATA SUBSYSTEM (Serial and Parallel ATA drivers))
> linux-ide@vger.kernel.org (open list:LIBATA PATA DRIVERS)
> linux-kernel@vger.kernel.org (open list)
>
> See also c4f9c8bbcc24 and fd86194aca1f.
Thanks.
Cheers,
Michael
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH RFC v3] m68k/q40: fix IO base selection for Q40 in pata_falcon.c
2023-08-16 5:29 ` Finn Thain
2023-08-16 7:27 ` Michael Schmitz
@ 2023-08-16 7:37 ` Finn Thain
2023-08-16 8:06 ` Michael Schmitz
1 sibling, 1 reply; 13+ messages in thread
From: Finn Thain @ 2023-08-16 7:37 UTC (permalink / raw)
To: Michael Schmitz; +Cc: will, linux-m68k, rz, geert
On Wed, 16 Aug 2023, Finn Thain wrote:
> Given it's a platform device that's only ever instantiated once, you
> could just use the module-scope variable rather than device-scope.
>
I see that atari instantiates one platform device but q40 actually
instantiates two ata ports, and each port has two drives. So it appears
the module parameter would need to have 4 bits.
You could set ap->private_data to pdev. Then pata_falcon_data_xfer() can
use pdev->id and sdev_target->id to find the relevant bit in the module
parameter.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH RFC v3] m68k/q40: fix IO base selection for Q40 in pata_falcon.c
2023-08-16 7:37 ` Finn Thain
@ 2023-08-16 8:06 ` Michael Schmitz
2023-08-16 9:04 ` Finn Thain
0 siblings, 1 reply; 13+ messages in thread
From: Michael Schmitz @ 2023-08-16 8:06 UTC (permalink / raw)
To: Finn Thain; +Cc: will, linux-m68k, rz, geert
Hi Finn,
Am 16.08.2023 um 19:37 schrieb Finn Thain:
> On Wed, 16 Aug 2023, Finn Thain wrote:
>
>> Given it's a platform device that's only ever instantiated once, you
>> could just use the module-scope variable rather than device-scope.
>>
>
> I see that atari instantiates one platform device but q40 actually
> instantiates two ata ports, and each port has two drives. So it appears
> the module parameter would need to have 4 bits.
That's what I thought.
>
> You could set ap->private_data to pdev. Then pata_falcon_data_xfer() can
> use pdev->id and sdev_target->id to find the relevant bit in the module
> parameter.
Instead of doing that for each call to pata_falcon_data_xfer(), I'd
rather find the relevant bits from the module parameter at device probe
time, and set the device-scope bit mask accordingly.
Cheers,
Michael
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH RFC v3] m68k/q40: fix IO base selection for Q40 in pata_falcon.c
2023-08-16 8:06 ` Michael Schmitz
@ 2023-08-16 9:04 ` Finn Thain
2023-08-16 10:40 ` Finn Thain
0 siblings, 1 reply; 13+ messages in thread
From: Finn Thain @ 2023-08-16 9:04 UTC (permalink / raw)
To: Michael Schmitz; +Cc: will, linux-m68k, rz, geert
On Wed, 16 Aug 2023, Michael Schmitz wrote:
> Instead of doing that for each call to pata_falcon_data_xfer(), I'd
> rather find the relevant bits from the module parameter at device probe
> time, and set the device-scope bit mask accordingly.
>
Good idea -- I see why you'd want to optimize pata_falcon_data_xfer(). But
you don't need a device-scope bit mask, you need a device-scope bit, which
could be obtained just by comparing private_data with NULL.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH RFC v3] m68k/q40: fix IO base selection for Q40 in pata_falcon.c
2023-08-16 9:04 ` Finn Thain
@ 2023-08-16 10:40 ` Finn Thain
0 siblings, 0 replies; 13+ messages in thread
From: Finn Thain @ 2023-08-16 10:40 UTC (permalink / raw)
To: Michael Schmitz; +Cc: will, linux-m68k, rz, geert
On Wed, 16 Aug 2023, Finn Thain wrote:
> On Wed, 16 Aug 2023, Michael Schmitz wrote:
>
> > Instead of doing that for each call to pata_falcon_data_xfer(), I'd
> > rather find the relevant bits from the module parameter at device probe
> > time, and set the device-scope bit mask accordingly.
> >
>
> Good idea -- I see why you'd want to optimize pata_falcon_data_xfer(). But
> you don't need a device-scope bit mask, you need a device-scope bit, which
> could be obtained just by comparing private_data with NULL.
>
You're right, of course -- you need two bits per port. Sorry for the
noise.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH RFC v3] m68k/q40: fix IO base selection for Q40 in pata_falcon.c
2023-08-15 22:32 [PATCH RFC v3] m68k/q40: fix IO base selection for Q40 in pata_falcon.c Michael Schmitz
2023-08-16 0:23 ` Finn Thain
@ 2023-08-16 8:07 ` Geert Uytterhoeven
2023-08-16 19:40 ` Michael Schmitz
2023-08-16 22:36 ` William R Sowerbutts
2 siblings, 1 reply; 13+ messages in thread
From: Geert Uytterhoeven @ 2023-08-16 8:07 UTC (permalink / raw)
To: Michael Schmitz; +Cc: will, linux-m68k, rz, Finn Thain
Hi Michael,
On Wed, Aug 16, 2023 at 12:32 AM Michael Schmitz <schmitzmic@gmail.com> 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).
>
> 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
> 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>
Thanks for your patch!
> --- a/drivers/ata/pata_falcon.c
> +++ b/drivers/ata/pata_falcon.c
> @@ -165,26 +178,61 @@ 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;
>
> - base = (void __iomem *)base_mem_res->start;
> - /* N.B. this assumes data_addr will be used for word-sized I/O only */
> - ap->ioaddr.data_addr = base + 0 + 0 * 4;
> - ap->ioaddr.error_addr = base + 1 + 1 * 4;
> - ap->ioaddr.feature_addr = base + 1 + 1 * 4;
> - ap->ioaddr.nsect_addr = base + 1 + 2 * 4;
> - ap->ioaddr.lbal_addr = base + 1 + 3 * 4;
> - ap->ioaddr.lbam_addr = base + 1 + 4 * 4;
> - ap->ioaddr.lbah_addr = base + 1 + 5 * 4;
> - ap->ioaddr.device_addr = base + 1 + 6 * 4;
> - ap->ioaddr.status_addr = base + 1 + 7 * 4;
> - ap->ioaddr.command_addr = base + 1 + 7 * 4;
> -
> - base = (void __iomem *)ctl_mem_res->start;
> - ap->ioaddr.altstatus_addr = base + 1;
> - ap->ioaddr.ctl_addr = base + 1;
> -
> - ata_port_desc(ap, "cmd 0x%lx ctl 0x%lx",
> - (unsigned long)base_mem_res->start,
> - (unsigned long)ctl_mem_res->start);
> + priv = devm_kzalloc(&pdev->dev,
> + sizeof(struct pata_falcon_priv), GFP_KERNEL);
> +
Please drop this blank line.
> + if (!priv)
> + return -ENOMEM;
> +
> + ap->private_data = priv;
> +
> + priv->swap_mask = pata_falcon_swap_mask;
> + if (priv->swap_mask)
> + priv->swap_data = 1;
> +
> + if (MACH_IS_Q40) {
Please do not use MACH_IS_xx() checks in a modern platform driver.
The proper way is to either pass parameters through platform_data, or
to use a different platform driver name to match against (i.e. populate
platform_driver.id_table with an array containing name/driver_data
pairs).
However, here you can just use the presence or absence
of base_res and ctl_res (which were unused before) to distinguish.
> + base = (void __iomem *)base_mem_res->start;
Any specific reason this is still using base_mem_res and not
base_res?
> + ap->ioaddr.data_addr = base + 0;
This is the same on Q40 and Falcon, so it can be factored out.
> + base = (void __iomem *)base_res->start;
> + ap->ioaddr.error_addr = base + 0x10000 + 1;
> + ap->ioaddr.feature_addr = base + 0x10000 + 1;
> + ap->ioaddr.nsect_addr = base + 0x10000 + 2;
> + ap->ioaddr.lbal_addr = base + 0x10000 + 3;
> + ap->ioaddr.lbam_addr = base + 0x10000 + 4;
> + ap->ioaddr.lbah_addr = base + 0x10000 + 5;
> + ap->ioaddr.device_addr = base + 0x10000 + 6;
> + ap->ioaddr.status_addr = base + 0x10000 + 7;
> + ap->ioaddr.command_addr = base + 0x10000 + 7;
Compared to Falcon, different base, banksize, and regsize, so e.g.
ap->ioaddr.error_addr = base + 1 * banksize + 1 * regsize;
> +
> + base = (void __iomem *)ctl_res->start;
> + ap->ioaddr.altstatus_addr = base + 0x10000;
> + ap->ioaddr.ctl_addr = base + 0x10000;
> +
> + ata_port_desc(ap, "cmd 0x%lx ctl 0x%lx",
> + (unsigned long)base_res->start,
> + (unsigned long)ctl_res->start);
%pa and e.g. &base_res->start to avoid the cast.
> + } else {
> + base = (void __iomem *)base_mem_res->start;
> + /* N.B. this assumes data_addr will be used for word-sized I/O only */
> + ap->ioaddr.data_addr = base + 0 + 0 * 4;
> + ap->ioaddr.error_addr = base + 1 + 1 * 4;
> + ap->ioaddr.feature_addr = base + 1 + 1 * 4;
> + ap->ioaddr.nsect_addr = base + 1 + 2 * 4;
> + ap->ioaddr.lbal_addr = base + 1 + 3 * 4;
> + ap->ioaddr.lbam_addr = base + 1 + 4 * 4;
> + ap->ioaddr.lbah_addr = base + 1 + 5 * 4;
> + ap->ioaddr.device_addr = base + 1 + 6 * 4;
> + ap->ioaddr.status_addr = base + 1 + 7 * 4;
> + ap->ioaddr.command_addr = base + 1 + 7 * 4;
> +
> + base = (void __iomem *)ctl_mem_res->start;
> + ap->ioaddr.altstatus_addr = base + 1;
> + ap->ioaddr.ctl_addr = base + 1;
> +
> + ata_port_desc(ap, "cmd 0x%lx ctl 0x%lx",
> + (unsigned long)base_mem_res->start,
> + (unsigned long)ctl_mem_res->start);
> + }
>
> irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> if (irq_res && irq_res->start > 0) {
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH RFC v3] m68k/q40: fix IO base selection for Q40 in pata_falcon.c
2023-08-16 8:07 ` Geert Uytterhoeven
@ 2023-08-16 19:40 ` Michael Schmitz
0 siblings, 0 replies; 13+ messages in thread
From: Michael Schmitz @ 2023-08-16 19:40 UTC (permalink / raw)
To: Geert Uytterhoeven; +Cc: will, linux-m68k, rz, Finn Thain
Hi Geert,
thanks for reviewing this!
On 16/08/23 20:07, Geert Uytterhoeven wrote:
>
>> - ata_port_desc(ap, "cmd 0x%lx ctl 0x%lx",
>> - (unsigned long)base_mem_res->start,
>> - (unsigned long)ctl_mem_res->start);
>> + priv = devm_kzalloc(&pdev->dev,
>> + sizeof(struct pata_falcon_priv), GFP_KERNEL);
>> +
> Please drop this blank line.
OK/
>
>> + if (!priv)
>> + return -ENOMEM;
>> +
>> + ap->private_data = priv;
>> +
>> + priv->swap_mask = pata_falcon_swap_mask;
>> + if (priv->swap_mask)
>> + priv->swap_data = 1;
>> +
>> + if (MACH_IS_Q40) {
> Please do not use MACH_IS_xx() checks in a modern platform driver.
> The proper way is to either pass parameters through platform_data, or
> to use a different platform driver name to match against (i.e. populate
> platform_driver.id_table with an array containing name/driver_data
> pairs).
>
> However, here you can just use the presence or absence
> of base_res and ctl_res (which were unused before) to distinguish.
Will do.
>
>> + base = (void __iomem *)base_mem_res->start;
> Any specific reason this is still using base_mem_res and not
> base_res?
Yes - data transfers don't use ioread8()/iowrite8() and need neither IO
port token added nor ISA address translation applied. We can use the
final address (as would be calculated by the ISA address translation)
directly.
>
>> + ap->ioaddr.data_addr = base + 0;
> This is the same on Q40 and Falcon, so it can be factored out.
Right - needs a slight rewrite because I overwrite 'base' in the next
line but since most of the other register definitions can be
generalized, it'll look much cleaner.
>
>> + base = (void __iomem *)base_res->start;
>> + ap->ioaddr.error_addr = base + 0x10000 + 1;
>> + ap->ioaddr.feature_addr = base + 0x10000 + 1;
>> + ap->ioaddr.nsect_addr = base + 0x10000 + 2;
>> + ap->ioaddr.lbal_addr = base + 0x10000 + 3;
>> + ap->ioaddr.lbam_addr = base + 0x10000 + 4;
>> + ap->ioaddr.lbah_addr = base + 0x10000 + 5;
>> + ap->ioaddr.device_addr = base + 0x10000 + 6;
>> + ap->ioaddr.status_addr = base + 0x10000 + 7;
>> + ap->ioaddr.command_addr = base + 0x10000 + 7;
> Compared to Falcon, different base, banksize, and regsize, so e.g.
>
> ap->ioaddr.error_addr = base + 1 * banksize + 1 * regsize;
Yes, we could phrase it that way.
The 0x10000 isn't a bank size but the IO port token that's stripped in
ioread8()/iowrite8() before the remainder is passed to inb()/outb()
where address translation happens.
The alternative would be to hide this in our ioport_map() (for the Q40
case only). Or use CONFIG_HAS_IOPORT_MAP and the generic port mapping
code. Meant to try that, but William now reports this works OK (for Q40).
>> +
>> + base = (void __iomem *)ctl_res->start;
>> + ap->ioaddr.altstatus_addr = base + 0x10000;
>> + ap->ioaddr.ctl_addr = base + 0x10000;
>> +
>> + ata_port_desc(ap, "cmd 0x%lx ctl 0x%lx",
>> + (unsigned long)base_res->start,
>> + (unsigned long)ctl_res->start);
> %pa and e.g. &base_res->start to avoid the cast.
OK.
>
>> + } else {
>> + base = (void __iomem *)base_mem_res->start;
>> + /* N.B. this assumes data_addr will be used for word-sized I/O only */
>> + ap->ioaddr.data_addr = base + 0 + 0 * 4;
>> + ap->ioaddr.error_addr = base + 1 + 1 * 4;
>> + ap->ioaddr.feature_addr = base + 1 + 1 * 4;
>> + ap->ioaddr.nsect_addr = base + 1 + 2 * 4;
>> + ap->ioaddr.lbal_addr = base + 1 + 3 * 4;
>> + ap->ioaddr.lbam_addr = base + 1 + 4 * 4;
>> + ap->ioaddr.lbah_addr = base + 1 + 5 * 4;
>> + ap->ioaddr.device_addr = base + 1 + 6 * 4;
>> + ap->ioaddr.status_addr = base + 1 + 7 * 4;
>> + ap->ioaddr.command_addr = base + 1 + 7 * 4;
>> +
>> + base = (void __iomem *)ctl_mem_res->start;
>> + ap->ioaddr.altstatus_addr = base + 1;
>> + ap->ioaddr.ctl_addr = base + 1;
>> +
>> + ata_port_desc(ap, "cmd 0x%lx ctl 0x%lx",
>> + (unsigned long)base_mem_res->start,
>> + (unsigned long)ctl_mem_res->start);
>> + }
>>
>> irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>> if (irq_res && irq_res->start > 0) {
We still need to verify that CONFIG_HAS_IOPORT_MAP does not interfere
with multiplatform kernels or Atari configuration, but for Q40 users
with non-native disk byte order, pata_legacy might be the easier option,
Cheers,
Michael
> Gr{oetje,eeting}s,
>
> Geert
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH RFC v3] m68k/q40: fix IO base selection for Q40 in pata_falcon.c
2023-08-15 22:32 [PATCH RFC v3] m68k/q40: fix IO base selection for Q40 in pata_falcon.c Michael Schmitz
2023-08-16 0:23 ` Finn Thain
2023-08-16 8:07 ` Geert Uytterhoeven
@ 2023-08-16 22:36 ` William R Sowerbutts
2023-08-17 1:35 ` Michael Schmitz
2 siblings, 1 reply; 13+ messages in thread
From: William R Sowerbutts @ 2023-08-16 22:36 UTC (permalink / raw)
To: Michael Schmitz; +Cc: linux-m68k, rz, geert, Finn Thain
I tested this patch
[ 1.540000] atari-falcon-ide atari-falcon-ide.0: Atari Falcon and Q40/Q60 PATA controller
[ 1.570000] scsi host0: pata_falcon
[ 1.580000] ata1: PATA max PIO4 cmd 0x1f0 ctl 0x3f6 irq 14
[ 1.590000] atari-falcon-ide atari-falcon-ide.1: Atari Falcon and Q40/Q60 PATA controller
[ 1.630000] scsi host1: pata_falcon
[ 1.640000] ata2: PATA max PIO4 cmd 0x170 ctl 0x376 irq 15
[ 1.840000] Unable to handle kernel NULL pointer dereference at virtual address 00000000
[ 1.840000] Oops: 00000000
[ 1.840000] Modules linked in:
[ 1.840000] PC: [<00211a82>] pata_falcon_data_xfer+0x2e/0x220
[ 1.840000] SR: 2700 SP: (ptrval) a2: 00853070
[ 1.840000] d0: 00915ea0 d1: 000010a0 d2: 000000b0 d3: 00000ea0
[ 1.840000] d4: 00000160 d5: 00000000 a0: 0091549a a1: 00915d60
[ 1.840000] Process kworker/0:1 (pid: 14, task=(ptrval))
[ 1.840000] Frame format=7 eff addr=00859e64 ssw=0505 faddr=00000000
[ 1.840000] wb 1 stat/addr/data: 0005 00306b70 004666b4
[ 1.840000] wb 2 stat/addr/data: 0005 00032934 00466b22
[ 1.840000] wb 3 stat/addr/data: 0005 00859e44 00000000
[ 1.840000] push data: 004666b4 00032a56 00464982 00000003
[ 1.840000] Stack from 00859e44:
[ 1.840000] 00000000 00000ea0 00915658 00000001 00000000 004caff4 00914000 002109b0
[ 1.840000] 0020ff74 00000000 002109ec 0091549a 00915ea0 00000160 00000000 004caff4
[ 1.840000] 0091549a 00210ae8 0091549a 004caff4 00000ea0 00000160 00915458 00000008
[ 1.840000] 0091549a 00914000 00915550 00211072 0091549a 00915458 00000008 0091549a
[ 1.840000] 00914000 00211564 0091549a 0091549a 00211084 0031bcfa 00203eb4 00000000
[ 1.840000] 007ca5e0 0091405e 00914000 00915550 0020ff74 0000a7c6 0091549a 0091405e
[ 1.840000] Call Trace: [<002109b0>] ata_pio_xfer+0x0/0x8c
[ 1.840000] [<0020ff74>] ata_sff_busy_wait+0x0/0x42
[ 1.840000] [<002109ec>] ata_pio_xfer+0x3c/0x8c
[ 1.840000] [<00210ae8>] ata_pio_sector+0xac/0x112
[ 1.840000] [<00211072>] ata_pio_sectors+0x94/0xa6
[ 1.840000] [<00211564>] ata_sff_hsm_move+0x4e0/0x56e
[ 1.840000] [<00211084>] ata_sff_hsm_move+0x0/0x56e
[ 1.840000] [<0031bcfa>] warn_slowpath_fmt+0x0/0x62
[ 1.840000] [<00203eb4>] ata_msleep+0x0/0xa2
[ 1.840000] [<0020ff74>] ata_sff_busy_wait+0x0/0x42
[ 1.840000] [<0000a7c6>] ATANSM+0x62/0x70
[ 1.840000] [<002116ea>] ata_sff_pio_task+0xf8/0x104
[ 1.840000] [<00015cbc>] kernel_thread+0x0/0x68
[ 1.840000] [<00024f36>] process_one_work+0x12a/0x1b2
[ 1.840000] [<00022fc2>] worker_clr_flags+0x0/0x72
[ 1.840000] [<00025406>] worker_thread+0x230/0x292
[ 1.840000] [<00015cbc>] kernel_thread+0x0/0x68
[ 1.840000] [<000295a4>] kthread_exit+0x0/0x14
[ 1.840000] [<000251d6>] worker_thread+0x0/0x292
[ 1.840000] [<00029668>] kthread+0x96/0xa0
[ 1.840000] [<000295d2>] kthread+0x0/0xa0
[ 1.840000] [<00002898>] ret_from_kernel_thread+0xc/0x14
[ 1.840000]
[ 1.840000] Code: 286d 0024 2c6d 24e8 2404 e28a 2a68 0008 <2055> 2068 00ac 2228 00fc 7601 c684 7001 b0a9 0108 6742 7007 c082 4a85 6700 0090
[ 1.840000] Disabling lock debugging due to kernel taint
[ 1.840000] note: kworker/0:1[14] exited with irqs disabled
This line is the problem:
>+ int dev_id = cmd->device->sdev_target->id;
I changed this to 'dev_id = dev->devno' instead, which fixed it.
I confirmed that the byte swapping on/off feature works.
Will
On Wed, Aug 16, 2023 at 10:32:12AM +1200, 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).
>
>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
>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;
>+
>+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)");
>+
>+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);
>
> /* 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;
> int irq = 0;
>
>@@ -165,26 +178,61 @@ 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;
>
>- base = (void __iomem *)base_mem_res->start;
>- /* N.B. this assumes data_addr will be used for word-sized I/O only */
>- ap->ioaddr.data_addr = base + 0 + 0 * 4;
>- ap->ioaddr.error_addr = base + 1 + 1 * 4;
>- ap->ioaddr.feature_addr = base + 1 + 1 * 4;
>- ap->ioaddr.nsect_addr = base + 1 + 2 * 4;
>- ap->ioaddr.lbal_addr = base + 1 + 3 * 4;
>- ap->ioaddr.lbam_addr = base + 1 + 4 * 4;
>- ap->ioaddr.lbah_addr = base + 1 + 5 * 4;
>- ap->ioaddr.device_addr = base + 1 + 6 * 4;
>- ap->ioaddr.status_addr = base + 1 + 7 * 4;
>- ap->ioaddr.command_addr = base + 1 + 7 * 4;
>-
>- base = (void __iomem *)ctl_mem_res->start;
>- ap->ioaddr.altstatus_addr = base + 1;
>- ap->ioaddr.ctl_addr = base + 1;
>-
>- ata_port_desc(ap, "cmd 0x%lx ctl 0x%lx",
>- (unsigned long)base_mem_res->start,
>- (unsigned long)ctl_mem_res->start);
>+ priv = devm_kzalloc(&pdev->dev,
>+ sizeof(struct pata_falcon_priv), GFP_KERNEL);
>+
>+ if (!priv)
>+ return -ENOMEM;
>+
>+ ap->private_data = priv;
>+
>+ priv->swap_mask = pata_falcon_swap_mask;
>+ if (priv->swap_mask)
>+ priv->swap_data = 1;
>+
>+ if (MACH_IS_Q40) {
>+ base = (void __iomem *)base_mem_res->start;
>+ ap->ioaddr.data_addr = base + 0;
>+ base = (void __iomem *)base_res->start;
>+ ap->ioaddr.error_addr = base + 0x10000 + 1;
>+ ap->ioaddr.feature_addr = base + 0x10000 + 1;
>+ ap->ioaddr.nsect_addr = base + 0x10000 + 2;
>+ ap->ioaddr.lbal_addr = base + 0x10000 + 3;
>+ ap->ioaddr.lbam_addr = base + 0x10000 + 4;
>+ ap->ioaddr.lbah_addr = base + 0x10000 + 5;
>+ ap->ioaddr.device_addr = base + 0x10000 + 6;
>+ ap->ioaddr.status_addr = base + 0x10000 + 7;
>+ ap->ioaddr.command_addr = base + 0x10000 + 7;
>+
>+ base = (void __iomem *)ctl_res->start;
>+ ap->ioaddr.altstatus_addr = base + 0x10000;
>+ ap->ioaddr.ctl_addr = base + 0x10000;
>+
>+ ata_port_desc(ap, "cmd 0x%lx ctl 0x%lx",
>+ (unsigned long)base_res->start,
>+ (unsigned long)ctl_res->start);
>+ } else {
>+ base = (void __iomem *)base_mem_res->start;
>+ /* N.B. this assumes data_addr will be used for word-sized I/O only */
>+ ap->ioaddr.data_addr = base + 0 + 0 * 4;
>+ ap->ioaddr.error_addr = base + 1 + 1 * 4;
>+ ap->ioaddr.feature_addr = base + 1 + 1 * 4;
>+ ap->ioaddr.nsect_addr = base + 1 + 2 * 4;
>+ ap->ioaddr.lbal_addr = base + 1 + 3 * 4;
>+ ap->ioaddr.lbam_addr = base + 1 + 4 * 4;
>+ ap->ioaddr.lbah_addr = base + 1 + 5 * 4;
>+ ap->ioaddr.device_addr = base + 1 + 6 * 4;
>+ ap->ioaddr.status_addr = base + 1 + 7 * 4;
>+ ap->ioaddr.command_addr = base + 1 + 7 * 4;
>+
>+ base = (void __iomem *)ctl_mem_res->start;
>+ ap->ioaddr.altstatus_addr = base + 1;
>+ ap->ioaddr.ctl_addr = base + 1;
>+
>+ ata_port_desc(ap, "cmd 0x%lx ctl 0x%lx",
>+ (unsigned long)base_mem_res->start,
>+ (unsigned long)ctl_mem_res->start);
>+ }
>
> irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> if (irq_res && irq_res->start > 0) {
>--
>2.17.1
>
_________________________________________________________________________
William R Sowerbutts will@sowerbutts.com
"Carpe post meridiem" http://sowerbutts.com
main(){char*s=">#=0> ^#X@#@^7=",c=0,m;for(;c<15;c++)for
(m=-1;m<7;putchar(m++/6&c%3/2?10:s[c]-31&1<<m?42:32));}
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH RFC v3] m68k/q40: fix IO base selection for Q40 in pata_falcon.c
2023-08-16 22:36 ` William R Sowerbutts
@ 2023-08-17 1:35 ` Michael Schmitz
0 siblings, 0 replies; 13+ messages in thread
From: Michael Schmitz @ 2023-08-17 1:35 UTC (permalink / raw)
To: William R Sowerbutts; +Cc: linux-m68k, rz, geert, Finn Thain
Hi William,
On 17/08/23 10:36, William R Sowerbutts wrote:
> I tested this patch
>
> [ 1.540000] atari-falcon-ide atari-falcon-ide.0: Atari Falcon and Q40/Q60 PATA controller
> [ 1.570000] scsi host0: pata_falcon
> [ 1.580000] ata1: PATA max PIO4 cmd 0x1f0 ctl 0x3f6 irq 14
> [ 1.590000] atari-falcon-ide atari-falcon-ide.1: Atari Falcon and Q40/Q60 PATA controller
> [ 1.630000] scsi host1: pata_falcon
> [ 1.640000] ata2: PATA max PIO4 cmd 0x170 ctl 0x376 irq 15
>
> [ 1.840000] Unable to handle kernel NULL pointer dereference at virtual address 00000000
> [ 1.840000] Oops: 00000000
> [ 1.840000] Modules linked in:
> [ 1.840000] PC: [<00211a82>] pata_falcon_data_xfer+0x2e/0x220
> [ 1.840000] SR: 2700 SP: (ptrval) a2: 00853070
> [ 1.840000] d0: 00915ea0 d1: 000010a0 d2: 000000b0 d3: 00000ea0
> [ 1.840000] d4: 00000160 d5: 00000000 a0: 0091549a a1: 00915d60
> [ 1.840000] Process kworker/0:1 (pid: 14, task=(ptrval))
> [ 1.840000] Frame format=7 eff addr=00859e64 ssw=0505 faddr=00000000
> [ 1.840000] wb 1 stat/addr/data: 0005 00306b70 004666b4
> [ 1.840000] wb 2 stat/addr/data: 0005 00032934 00466b22
> [ 1.840000] wb 3 stat/addr/data: 0005 00859e44 00000000
> [ 1.840000] push data: 004666b4 00032a56 00464982 00000003
> [ 1.840000] Stack from 00859e44:
> [ 1.840000] 00000000 00000ea0 00915658 00000001 00000000 004caff4 00914000 002109b0
> [ 1.840000] 0020ff74 00000000 002109ec 0091549a 00915ea0 00000160 00000000 004caff4
> [ 1.840000] 0091549a 00210ae8 0091549a 004caff4 00000ea0 00000160 00915458 00000008
> [ 1.840000] 0091549a 00914000 00915550 00211072 0091549a 00915458 00000008 0091549a
> [ 1.840000] 00914000 00211564 0091549a 0091549a 00211084 0031bcfa 00203eb4 00000000
> [ 1.840000] 007ca5e0 0091405e 00914000 00915550 0020ff74 0000a7c6 0091549a 0091405e
> [ 1.840000] Call Trace: [<002109b0>] ata_pio_xfer+0x0/0x8c
> [ 1.840000] [<0020ff74>] ata_sff_busy_wait+0x0/0x42
> [ 1.840000] [<002109ec>] ata_pio_xfer+0x3c/0x8c
> [ 1.840000] [<00210ae8>] ata_pio_sector+0xac/0x112
> [ 1.840000] [<00211072>] ata_pio_sectors+0x94/0xa6
> [ 1.840000] [<00211564>] ata_sff_hsm_move+0x4e0/0x56e
> [ 1.840000] [<00211084>] ata_sff_hsm_move+0x0/0x56e
> [ 1.840000] [<0031bcfa>] warn_slowpath_fmt+0x0/0x62
> [ 1.840000] [<00203eb4>] ata_msleep+0x0/0xa2
> [ 1.840000] [<0020ff74>] ata_sff_busy_wait+0x0/0x42
> [ 1.840000] [<0000a7c6>] ATANSM+0x62/0x70
> [ 1.840000] [<002116ea>] ata_sff_pio_task+0xf8/0x104
> [ 1.840000] [<00015cbc>] kernel_thread+0x0/0x68
> [ 1.840000] [<00024f36>] process_one_work+0x12a/0x1b2
> [ 1.840000] [<00022fc2>] worker_clr_flags+0x0/0x72
> [ 1.840000] [<00025406>] worker_thread+0x230/0x292
> [ 1.840000] [<00015cbc>] kernel_thread+0x0/0x68
> [ 1.840000] [<000295a4>] kthread_exit+0x0/0x14
> [ 1.840000] [<000251d6>] worker_thread+0x0/0x292
> [ 1.840000] [<00029668>] kthread+0x96/0xa0
> [ 1.840000] [<000295d2>] kthread+0x0/0xa0
> [ 1.840000] [<00002898>] ret_from_kernel_thread+0xc/0x14
> [ 1.840000]
> [ 1.840000] Code: 286d 0024 2c6d 24e8 2404 e28a 2a68 0008 <2055> 2068 00ac 2228 00fc 7601 c684 7001 b0a9 0108 6742 7007 c082 4a85 6700 0090
> [ 1.840000] Disabling lock debugging due to kernel taint
> [ 1.840000] note: kworker/0:1[14] exited with irqs disabled
>
> This line is the problem:
>
>> + int dev_id = cmd->device->sdev_target->id;
> I changed this to 'dev_id = dev->devno' instead, which fixed it.
Odd - but dev->devno appears to be the correct one.
>
> I confirmed that the byte swapping on/off feature works.
Thanks - I'll work in Geert's and Finn's comments and will send a
two-patch series.
Cheers,
Michael
>
> Will
>
>
> On Wed, Aug 16, 2023 at 10:32:12AM +1200, 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).
>>
>> 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
>> 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;
>> +
>> +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)");
>> +
>> +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);
>>
>> /* 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;
>> int irq = 0;
>>
>> @@ -165,26 +178,61 @@ 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;
>>
>> - base = (void __iomem *)base_mem_res->start;
>> - /* N.B. this assumes data_addr will be used for word-sized I/O only */
>> - ap->ioaddr.data_addr = base + 0 + 0 * 4;
>> - ap->ioaddr.error_addr = base + 1 + 1 * 4;
>> - ap->ioaddr.feature_addr = base + 1 + 1 * 4;
>> - ap->ioaddr.nsect_addr = base + 1 + 2 * 4;
>> - ap->ioaddr.lbal_addr = base + 1 + 3 * 4;
>> - ap->ioaddr.lbam_addr = base + 1 + 4 * 4;
>> - ap->ioaddr.lbah_addr = base + 1 + 5 * 4;
>> - ap->ioaddr.device_addr = base + 1 + 6 * 4;
>> - ap->ioaddr.status_addr = base + 1 + 7 * 4;
>> - ap->ioaddr.command_addr = base + 1 + 7 * 4;
>> -
>> - base = (void __iomem *)ctl_mem_res->start;
>> - ap->ioaddr.altstatus_addr = base + 1;
>> - ap->ioaddr.ctl_addr = base + 1;
>> -
>> - ata_port_desc(ap, "cmd 0x%lx ctl 0x%lx",
>> - (unsigned long)base_mem_res->start,
>> - (unsigned long)ctl_mem_res->start);
>> + priv = devm_kzalloc(&pdev->dev,
>> + sizeof(struct pata_falcon_priv), GFP_KERNEL);
>> +
>> + if (!priv)
>> + return -ENOMEM;
>> +
>> + ap->private_data = priv;
>> +
>> + priv->swap_mask = pata_falcon_swap_mask;
>> + if (priv->swap_mask)
>> + priv->swap_data = 1;
>> +
>> + if (MACH_IS_Q40) {
>> + base = (void __iomem *)base_mem_res->start;
>> + ap->ioaddr.data_addr = base + 0;
>> + base = (void __iomem *)base_res->start;
>> + ap->ioaddr.error_addr = base + 0x10000 + 1;
>> + ap->ioaddr.feature_addr = base + 0x10000 + 1;
>> + ap->ioaddr.nsect_addr = base + 0x10000 + 2;
>> + ap->ioaddr.lbal_addr = base + 0x10000 + 3;
>> + ap->ioaddr.lbam_addr = base + 0x10000 + 4;
>> + ap->ioaddr.lbah_addr = base + 0x10000 + 5;
>> + ap->ioaddr.device_addr = base + 0x10000 + 6;
>> + ap->ioaddr.status_addr = base + 0x10000 + 7;
>> + ap->ioaddr.command_addr = base + 0x10000 + 7;
>> +
>> + base = (void __iomem *)ctl_res->start;
>> + ap->ioaddr.altstatus_addr = base + 0x10000;
>> + ap->ioaddr.ctl_addr = base + 0x10000;
>> +
>> + ata_port_desc(ap, "cmd 0x%lx ctl 0x%lx",
>> + (unsigned long)base_res->start,
>> + (unsigned long)ctl_res->start);
>> + } else {
>> + base = (void __iomem *)base_mem_res->start;
>> + /* N.B. this assumes data_addr will be used for word-sized I/O only */
>> + ap->ioaddr.data_addr = base + 0 + 0 * 4;
>> + ap->ioaddr.error_addr = base + 1 + 1 * 4;
>> + ap->ioaddr.feature_addr = base + 1 + 1 * 4;
>> + ap->ioaddr.nsect_addr = base + 1 + 2 * 4;
>> + ap->ioaddr.lbal_addr = base + 1 + 3 * 4;
>> + ap->ioaddr.lbam_addr = base + 1 + 4 * 4;
>> + ap->ioaddr.lbah_addr = base + 1 + 5 * 4;
>> + ap->ioaddr.device_addr = base + 1 + 6 * 4;
>> + ap->ioaddr.status_addr = base + 1 + 7 * 4;
>> + ap->ioaddr.command_addr = base + 1 + 7 * 4;
>> +
>> + base = (void __iomem *)ctl_mem_res->start;
>> + ap->ioaddr.altstatus_addr = base + 1;
>> + ap->ioaddr.ctl_addr = base + 1;
>> +
>> + ata_port_desc(ap, "cmd 0x%lx ctl 0x%lx",
>> + (unsigned long)base_mem_res->start,
>> + (unsigned long)ctl_mem_res->start);
>> + }
>>
>> irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>> if (irq_res && irq_res->start > 0) {
>> --
>> 2.17.1
>>
> _________________________________________________________________________
> William R Sowerbutts will@sowerbutts.com
> "Carpe post meridiem" http://sowerbutts.com
> main(){char*s=">#=0> ^#X@#@^7=",c=0,m;for(;c<15;c++)for
> (m=-1;m<7;putchar(m++/6&c%3/2?10:s[c]-31&1<<m?42:32));}
>
^ permalink raw reply [flat|nested] 13+ messages in thread