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

[Michael Schmitz <schmitzmic@gmail.com>]

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
>