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

[Bjorn Helgaas]

On Fri, Aug 22, 2014 at 7:32 AM, Liviu Dudau <liviu@xxxxxxxxxxx> wrote:
> On Fri, Aug 22, 2014 at 12:13:33AM -0500, Bjorn Helgaas wrote:
>> On Thu, Aug 21, 2014 at 6:01 PM, Liviu Dudau <liviu@xxxxxxxxxxx> wrote:
>> > On Thu, Aug 21, 2014 at 12:02:16PM -0600, Bjorn Helgaas wrote:
>> >> [+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@xxxxxxxxxxx> 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@xxxxxxxxxx
>> >> >
>> >> > 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.
>> >
>> > Hmm, after having a little bit more time to get my brain back into the problem
>> > I'm now not sure this will be good enough.
>> >
>> > Let me explain what I was trying to solve with the callback that Arnd doesn't
>> > like and maybe you (both) can validate if my concerns are real or not:
>> >
>> > I was trying to come up with a function that can easily replace pci_scan_child_bus().
>> > The problem I'm facing is that the ranges that we parse from the device tree
>> > need to be converted to resources and passed on to the pci_host_bridge structure
>> > to be stored as windows. In order for the host bridge driver to be initialised, it
>> > also needs to parse the device tree information *and* use the information calculated
>> > for the io_base in order to program the address translation correctly. The only way
>> > I found to avoid duplicating the parsing step and sequence the initialisation correctly
>> > was to introduce the callback.
>>
>> The current v9 patch has this:
>>
>>   int of_create_pci_host_bridge(...)
>>   {
>>     of_pci_parse_bus_range();
>>     pci_host_bridge_of_get_ranges();
>>     pci_create_root_bus();
>>     setup();
>>     pci_scan_child_bus();
>>     pci_assign_unassigned_bus_resources();
>>     pci_bus_add_devices();
>>   }
>>
>> I don't think there's anything that requires setup() to be done after
>> pci_create_root_bus().  You do pass the "struct pci_host_bridge *" to
>> setup(), but I think that's only a convenient way to pass in the
>> resource information that's also passed to pci_create_root_bus().
>> setup() doesn't actually require the pci_bus or pci_host_bridge
>> structures themselves.  So I think setup() *could* be done before
>> pci_create_root_bus().  I think that would be better because then all
>> the PCI core stuff is together and can be more easily factored out.
>>
>
> OK. In my mind the setup part was not worth doing if we failed to create the root
> bus, hence the order I've put them in. Also, up to v8 and in line with the
> side discussions we had over the last year, the domain information and future
> data was/could be stored in the pci_host_bridge structure, which again introduces
> order. But I agree that in the current shape the setup can be done before
> pci_create_root_bus().

My opinion is that the benefit of separating the arch from the core
code outweighs the benefit of skipping setup when
pci_create_root_bus() fails.  The separation will enable future PCI
core refactoring.  My hope is that one day we can replace the four
existing calls (pci_create_root_bus(), pci_scan_child_bus(),
pci_assign_unassigned_bus_resources(), pci_bus_add_devices()) with a
single new PCI interface.  That will be easier if there's no
arch-specific setup in the middle.

>> If setup() is before pci_create_root_bus(), my objective is met, and I
>> don't care how you structure the rest.  Arnd prefers to avoid the
>> callback, and I do agree that we should avoid callbacks when possible.
>>
>
> I am slightly cagey that I have started with a grand plan of trying to create
> a generic framework that can be future proof and now I'm trying to make the code
> fit the existing API without too much insight on how it can be used in the future.
> If you think this will be alright in your future plans of making pci_host_bridge
> structure more central to the PCI framework, then I'm happy with it.

Using the existing API in the same way as other arches doesn't
necessarily get us *closer* to a generic framework, but doing things
*differently* definitely would make it harder.  I think the approach
we're converging on is a good compromise.  I hope I haven't dampened
your enthusiasm for making a more generic framework.  I want the same
thing, and I hope you continue working on it!

Bjorn

>> > If I follow your suggestion with gen_pci_probe() the question I have is about
>> > gen_pci_setup(). Is that something that is implemented at architectural level?
>> > If so, then we are droping into the arm implementation where each host bridge
>> > driver has to register another hook to be called from arch code. If not, then
>> > we risk having a platform with two host bridges, each implementing
>> > gen_pci_setup().
>>
>> The gen_pci_setup() in Lorenzo's patch [1] is already in
>> pci-host-generic.c and doesn't look arch-specific to me.
>>
>> [1] http://lkml.kernel.org/r/1407861695-25549-1-git-send-email-Liviu.Dudau@xxxxxxx
>>
>> > Arnd, one thing I'm trying to figure out is if you are not actually seeing the
>> > callback I've introduced as a repeat of the arm implementation. That is not
>> > my intent and it is designed to be used only by the host bridge drivers in order
>> > to finish their initialisation once all the generic and architectural code has
>> > run, but before any actual scanning of the root bus happens.
>> >
>> > Best regards,
>> > Liviu
>> >
>> >>
>> >> Bjorn
>> >>
>> >
>> > --
>> > -------------------
>> >    .oooO
>> >    (   )
>> >     \ (  Oooo.
>> >      \_) (   )
>> >           ) /
>> >          (_/
>> >
>> >  One small step
>> >    for me ...
>> >
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>
> --
> -------------------
>    .oooO
>    (   )
>     \ (  Oooo.
>      \_) (   )
>           ) /
>          (_/
>
>  One small step
>    for me ...
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/