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

Michael Schmitz
2023-08-16T19:40:45Z

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
>