Re: [PATCH] drivers: pci: convert generic host controller to DT host bridge creation API

[Bjorn Helgaas <bhelgaas@google.com>]

[+cc Lorenzo]

On Wed, Aug 20, 2014 at 05:35:59PM -0500, Bjorn Helgaas wrote:
> On Wed, Aug 20, 2014 at 7:31 AM, Liviu Dudau <liviu@dudau.co.uk> wrote:
> > On Wed, Aug 20, 2014 at 01:27:57PM +0200, Arnd Bergmann wrote:
> >> On Tuesday 12 August 2014, Liviu Dudau wrote:
> >> > +       return of_create_pci_host_bridge(dev, 0, 0xff, &gen_pci_ops,
> >> > +                                       gen_pci_setup, pci);
> >>
> >> I had not noticed it earlier, but the setup callback is actually a feature
> >> of the arm32 PCI code that I had hoped to avoid when moving to the
> >> generic API. Can we do this as a more regular sequence of
> >>
> >>
> >>       ret = of_create_pci_host_bridge(dev, 0, 0xff, &gen_pci_ops, pci);
> >>       if (ret)
> >>               return ret;
> >>
> >>       ret = gen_pci_setup(pci);
> >>       if (ret)
> >>               pci_destroy_host_bridge(dev, pci);
> >>       return ret;
> >>
> >> ?
> >>
> >>       Arnd
> >
> > Hi Arnd,
> >
> > That has been the general approach of my patchset up to v9. But, as Bjorn has
> > mentioned in his v8 review and I have put in my cover letter, the regular
> > aproach means that architectures that use pci_scan_root_bus() will have to
> > drop their one liner and replace it with the more verbose of_create_pci_host_bridge()
> > followed by pci_scan_child_bus() and pci_bus_add_devices() (basically, the content
> > of pci_scan_root_bus()). For those architectures it will lead to a net increase of
> > lines of code.
> >
> > The patch for pci-host-generic.c is the first to use the callback setup function, but
> > not the only one. My PCI host bridge driver for Juno has the same need, and I'm betting
> > all other host bridge controllers will use it as it will be the only opportunity to
> > finish the controller setup before we start scanning the child busses. I'm trying to
> > balance ease of read vs ease of use here and it is the best version I've come up with
> > so far.
> 
> My guess is that you're referring to
> http://lkml.kernel.org/r/20140708011136.GE22939@google.com
> 
> I'm trying to get to the point where arch code can discover the host
> bridge, configure it, learn its properties (apertures, etc.), then
> pass it off completely to the PCI core for PCI device enumeration.
> pci_scan_root_bus() is the closest thing we have to that right now, so
> that's why I point to that.  Here's the current pci_scan_root_bus():
> 
>   pci_scan_root_bus()
>   {
>     pci_create_root_bus();
>     /* 1 */
>     pci_scan_child_bus()
>     /* 2 */
>     pci_bus_add_devices()
>   }
> 
> This is obviously incomplete as it is -- for example, it does nothing
> about assigning resources to PCI devices, so it only works if we rely
> completely on the firmware to do that.  Some arches (x86, ia64, etc.)
> don't want to rely on firmware, so they basically open-code
> pci_scan_root_bus() and insert resource assignment at (2) above.  That
> resource assignment really *should* be done in pci_scan_root_bus()
> itself, but it's quite a bit of work to make that happen.
> 
> In your case, of_create_pci_host_bridge() open-codes
> pci_scan_root_bus() and calls the "setup" callback at (1) in the
> outline above.  I don't have any problem with that, and I don't care
> whether you do it by passing in a callback function pointer or via
> some other means.
> 
> However, I would ask whether this is really a requirement.  Most
> (maybe all) other arches require nothing special at (1), i.e., between
> pci_create_root_bus() and pci_scan_child_bus().  If you can do it
> *before* pci_create_root_bus(), I think that would be nicer, but maybe
> you can't.

I talked to Lorenzo here at LinuxCon and he explained this so it makes a
lot more sense to me now.  Would something like the following work?

  gen_pci_probe()
  {
    LIST_HEAD(res);
    resource_size_t io_base = 0;

    of_parse_pci_host_bridge_resources(dev, &res, 0, 0xff, &io_base);
    gen_pci_setup(&res, io_base);

    pci_create_root_bus(..., &res);
    pci_scan_child_bus();
    ... pci_assign_unassigned_bus_resources
    pci_bus_add_resources();
  }

Then we at least have all the PCI-related code consolidated, without
the arch-specific stuff mixed in.  We could almost use pci_scan_root_bus(),
but not quite, because of the pci_assign_unassigned_bus_resources() call
that pci_scan_root_bus() doesn't do.

Bjorn