Re: [PATCH 4/9] pci: add DT based ARM Versatile PCI host driver

Bjorn Helgaas
2015-01-24T01:01:27Z

On Tue, Dec 30, 2014 at 10:58:00PM +0100, Arnd Bergmann wrote:
> On Tuesday 30 December 2014 13:28:33 Rob Herring wrote:
> > + list_for_each_entry(win, res, list) {
> > + struct resource *parent, *res = win->res;
> > +
> > + switch (resource_type(res)) {
> > + case IORESOURCE_IO:
> > + parent = &ioport_resource;
> > + err = pci_remap_iospace(res, iobase);
> > + if (err) {
> > + dev_warn(dev, "error %d: failed to map resource %pR\n",
> > + err, res);
> > + continue;
> > + }
> > + break;
> > + case IORESOURCE_MEM:
> > + parent = &iomem_resource;
> > + res_valid |= !(res->flags & IORESOURCE_PREFETCH);
> > +
> > + writel(res->start >> 28, PCI_IMAP(mem));
> > + writel(PHYS_OFFSET >> 28, PCI_SMAP(mem));
> > + mem++;
> > +
> > + break;
> > + case IORESOURCE_BUS:
> > + default:
> > + continue;
> > + }
> > +
> > + err = devm_request_resource(dev, parent, res);
> > + if (err)
> > + goto out_release_res;
> > + }
>
> I wonder if we should also request the physical resource for the I/O space
> window to have it show up in /proc/iomem. We are rather inconsistent in this
> regard between drivers.

I'd like that. We are inconsistent, but I think it's useful to have this
information in /proc/iomem. After all, it is physical address space that
we can't use for anything else, so I guess you could argue that it's
actually a bug to omit it.

> > + pci_add_flags(PCI_ENABLE_PROC_DOMAINS);
> > + pci_add_flags(PCI_REASSIGN_ALL_BUS | PCI_REASSIGN_ALL_RSRC);
> > +
> > + bus = pci_scan_root_bus(&pdev->dev, 0, &pci_versatile_ops, &sys, &pci_res);
> > + if (!bus)
> > + return -ENOMEM;
> > +
> > + pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci);
> > + pci_assign_unassigned_bus_resources(bus);
> > +
> > + return 0;
>
> One general question, mainly for Bjorn: pci_scan_root_bus adds all the devices
> at the Linux driver level by calling pci_bus_add_devices(), but then we call
> pci_fixup_irqs and pci_assign_unassigned_bus_resources after that, which will
> change the devices again. Is this how it's meant to work? How do we ensure
> that we have the correct irq number and resources by the time we enter the
> probe() function of the PCI device driver that gets bound to a device here?

Nope, that isn't how it's meant to work. After pci_bus_add_devices()
completes, drivers can be already bound to the device, and the PCI core
should keep its mitts off things the driver could be using. But I think
we've had this problem for a long time, and I haven't looked at it recently
to see how hard it would be to fix.

Bjorn