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
