| Message ID | 20230822221359.31024-2-schmitzmic@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v4,1/2] ata: pata_falcon: fix IO base selection for Q40 | expand |
Hi Michael, On Wed, Aug 23, 2023 at 12:14 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. > > 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 shift 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 shift 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 > for Q40 (the IO address translation will then add the correct > ISA window base address and byte access offset), with register > shift 1. Use MMIO base address and register shift 2 as before > for Falcon. > > Encode PIO_OFFSET into IO port addresses for all registers > for Q40 except the data transfer register. Encode the MMIO > offset there (pata_falcon_data_xfer() directly uses raw IO > with no address translation). > > 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: stable@vger.kernel.org > 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> > > --- > > Changes from v3: > > Sergey Shtylyov: > - change use of reg_scale to reg_shift > > Geert Uytterhoeven: > - factor out ata_port_desc() from platform specific code Thanks for the update! > --- a/drivers/ata/pata_falcon.c > +++ b/drivers/ata/pata_falcon.c > + ata_port_desc(ap, "cmd %px ctl %px data %pa", > + base, ctl_base, &ap->ioaddr.data_addr); %px and ap->ioaddr.data_addr The rest LGTM, so with the above fixed Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org> Gr{oetje,eeting}s, Geert
Hello! I prefer CCing my OMP account when you send the PATA patches, as is returned by scripts/get_maintainer.pl... On 8/23/23 1:13 AM, 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. > > 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 shift 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 shift 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 > for Q40 (the IO address translation will then add the correct > ISA window base address and byte access offset), with register > shift 1. Use MMIO base address and register shift 2 as before > for Falcon. > > Encode PIO_OFFSET into IO port addresses for all registers > for Q40 except the data transfer register. Encode the MMIO > offset there (pata_falcon_data_xfer() directly uses raw IO > with no address translation). > > 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: stable@vger.kernel.org > 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 996516e64f13..3841ea200bcb 100644 > --- a/drivers/ata/pata_falcon.c > +++ b/drivers/ata/pata_falcon.c [...] > @@ -165,26 +165,34 @@ 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); > + ap->ioaddr.data_addr = (void __iomem *)base_mem_res->start; > + > + if (base_res) { /* only Q40 has IO resources */ > + io_offset = 0x10000; > + reg_shift = 0; > + base = (void __iomem *)base_res->start; > + ctl_base = (void __iomem *)ctl_res->start; > + } else { > + base = (void __iomem *)base_mem_res->start; > + ctl_base = (void __iomem *)ctl_mem_res->start; > + } > + > + ap->ioaddr.error_addr = base + io_offset + (1 << reg_shift); > + ap->ioaddr.feature_addr = base + io_offset + (1 << reg_shift); > + ap->ioaddr.nsect_addr = base + io_offset + (2 << reg_shift); > + ap->ioaddr.lbal_addr = base + io_offset + (3 << reg_shift); > + ap->ioaddr.lbam_addr = base + io_offset + (4 << reg_shift); > + ap->ioaddr.lbah_addr = base + io_offset + (5 << reg_shift); > + ap->ioaddr.device_addr = base + io_offset + (6 << reg_shift); > + ap->ioaddr.status_addr = base + io_offset + (7 << reg_shift); > + ap->ioaddr.command_addr = base + io_offset + (7 << reg_shift); > + > + ap->ioaddr.altstatus_addr = ctl_base + io_offset; > + ap->ioaddr.ctl_addr = ctl_base + io_offset; > + > + ata_port_desc(ap, "cmd %px ctl %px data %pa", > + base, ctl_base, &ap->ioaddr.data_addr); Like Geert said, use "%px" and ap->ioaddr.data_addr here... [...] MBR, Sergey
Hi Geert, On 23/08/23 21:05, Geert Uytterhoeven wrote: > > Thanks for the update! > >> --- a/drivers/ata/pata_falcon.c >> +++ b/drivers/ata/pata_falcon.c >> + ata_port_desc(ap, "cmd %px ctl %px data %pa", >> + base, ctl_base, &ap->ioaddr.data_addr); > %px and ap->ioaddr.data_addr > > The rest LGTM, so with the above fixed > Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org> Will fix this in v4. Thanks, Michael > > Gr{oetje,eeting}s, > > Geert >
Hi Sergey, On 24/08/23 04:05, Sergey Shtylyov wrote: > Hello! > > I prefer CCing my OMP account when you send the PATA patches, > as is returned by scripts/get_maintainer.pl... Sorry, I was left with the impression OMP was rejecting list messages from linux-ide ... > > On 8/23/23 1:13 AM, 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. >> >> 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 shift 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 shift 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 >> for Q40 (the IO address translation will then add the correct >> ISA window base address and byte access offset), with register >> shift 1. Use MMIO base address and register shift 2 as before >> for Falcon. >> >> Encode PIO_OFFSET into IO port addresses for all registers >> for Q40 except the data transfer register. Encode the MMIO >> offset there (pata_falcon_data_xfer() directly uses raw IO >> with no address translation). >> >> 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: stable@vger.kernel.org >> 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 996516e64f13..3841ea200bcb 100644 >> --- a/drivers/ata/pata_falcon.c >> +++ b/drivers/ata/pata_falcon.c > [...] >> @@ -165,26 +165,34 @@ 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); >> + ap->ioaddr.data_addr = (void __iomem *)base_mem_res->start; >> + >> + if (base_res) { /* only Q40 has IO resources */ >> + io_offset = 0x10000; >> + reg_shift = 0; >> + base = (void __iomem *)base_res->start; >> + ctl_base = (void __iomem *)ctl_res->start; >> + } else { >> + base = (void __iomem *)base_mem_res->start; >> + ctl_base = (void __iomem *)ctl_mem_res->start; >> + } >> + >> + ap->ioaddr.error_addr = base + io_offset + (1 << reg_shift); >> + ap->ioaddr.feature_addr = base + io_offset + (1 << reg_shift); >> + ap->ioaddr.nsect_addr = base + io_offset + (2 << reg_shift); >> + ap->ioaddr.lbal_addr = base + io_offset + (3 << reg_shift); >> + ap->ioaddr.lbam_addr = base + io_offset + (4 << reg_shift); >> + ap->ioaddr.lbah_addr = base + io_offset + (5 << reg_shift); >> + ap->ioaddr.device_addr = base + io_offset + (6 << reg_shift); >> + ap->ioaddr.status_addr = base + io_offset + (7 << reg_shift); >> + ap->ioaddr.command_addr = base + io_offset + (7 << reg_shift); >> + >> + ap->ioaddr.altstatus_addr = ctl_base + io_offset; >> + ap->ioaddr.ctl_addr = ctl_base + io_offset; >> + >> + ata_port_desc(ap, "cmd %px ctl %px data %pa", >> + base, ctl_base, &ap->ioaddr.data_addr); > Like Geert said, use "%px" and ap->ioaddr.data_addr here... Will do. Cheers, Michael > > [...] > > MBR, Sergey
On 8/24/23 4:56 AM, Michael Schmitz wrote: [...] >> I prefer CCing my OMP account when you send the PATA patches, >> as is returned by scripts/get_maintainer.pl... > Sorry, I was left with the impression OMP was rejecting list messages from linux-ide ... No, it rejected my reply to you for some reason. However, the msgs from linux-ide seem to be still stuck somewhere as well... MBR, Sergey
Hi Sergey, thanks for clarifying, I'll correct that for v5. Cheers, Michael On 24/08/23 22:00, Sergey Shtylyov wrote: > On 8/24/23 4:56 AM, Michael Schmitz wrote: > [...] > >>> I prefer CCing my OMP account when you send the PATA patches, >>> as is returned by scripts/get_maintainer.pl... >> Sorry, I was left with the impression OMP was rejecting list messages from linux-ide ... > No, it rejected my reply to you for some reason. > However, the msgs from linux-ide seem to be still stuck somewhere > as well... > > MBR, Sergey
diff --git a/drivers/ata/pata_falcon.c b/drivers/ata/pata_falcon.c index 996516e64f13..3841ea200bcb 100644 --- a/drivers/ata/pata_falcon.c +++ b/drivers/ata/pata_falcon.c @@ -123,8 +123,8 @@ 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; - void __iomem *base; - int irq = 0; + void __iomem *base, *ctl_base; + int irq = 0, io_offset = 1, reg_shift = 2; /* Falcon defaults */ dev_info(&pdev->dev, "Atari Falcon and Q40/Q60 PATA controller\n"); @@ -165,26 +165,34 @@ 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); + ap->ioaddr.data_addr = (void __iomem *)base_mem_res->start; + + if (base_res) { /* only Q40 has IO resources */ + io_offset = 0x10000; + reg_shift = 0; + base = (void __iomem *)base_res->start; + ctl_base = (void __iomem *)ctl_res->start; + } else { + base = (void __iomem *)base_mem_res->start; + ctl_base = (void __iomem *)ctl_mem_res->start; + } + + ap->ioaddr.error_addr = base + io_offset + (1 << reg_shift); + ap->ioaddr.feature_addr = base + io_offset + (1 << reg_shift); + ap->ioaddr.nsect_addr = base + io_offset + (2 << reg_shift); + ap->ioaddr.lbal_addr = base + io_offset + (3 << reg_shift); + ap->ioaddr.lbam_addr = base + io_offset + (4 << reg_shift); + ap->ioaddr.lbah_addr = base + io_offset + (5 << reg_shift); + ap->ioaddr.device_addr = base + io_offset + (6 << reg_shift); + ap->ioaddr.status_addr = base + io_offset + (7 << reg_shift); + ap->ioaddr.command_addr = base + io_offset + (7 << reg_shift); + + ap->ioaddr.altstatus_addr = ctl_base + io_offset; + ap->ioaddr.ctl_addr = ctl_base + io_offset; + + ata_port_desc(ap, "cmd %px ctl %px data %pa", + base, ctl_base, &ap->ioaddr.data_addr); irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0); if (irq_res && irq_res->start > 0) {
With commit 44b1fbc0f5f3 ("m68k/q40: Replace q40ide driver with pata_falcon and falconide"), the Q40 IDE driver was replaced by pata_falcon.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 shift 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 shift 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 for Q40 (the IO address translation will then add the correct ISA window base address and byte access offset), with register shift 1. Use MMIO base address and register shift 2 as before for Falcon. Encode PIO_OFFSET into IO port addresses for all registers for Q40 except the data transfer register. Encode the MMIO offset there (pata_falcon_data_xfer() directly uses raw IO with no address translation). 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: stable@vger.kernel.org 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> --- Changes from v3: Sergey Shtylyov: - change use of reg_scale to reg_shift Geert Uytterhoeven: - factor out ata_port_desc() from platform specific code Changes from v2: Finn Thain: - add back stable Cc: Changes from v1: Damien Le Moal: - change patch title - drop stable backport tag Changes from RFC v3: - split off byte swap option into separate patch Geert Uytterhoeven: - review comments Changes from RFC v2: - add driver parameter 'data_swap' as bit mask for drives to swap Changes from RFC v1: Finn Thain: - take care to supply IO address suitable for ioread8/iowrite8 - use MMIO address for data transfer --- drivers/ata/pata_falcon.c | 50 +++++++++++++++++++++++---------------- 1 file changed, 29 insertions(+), 21 deletions(-)