[PATCH] drivers: pci: convert generic host controller to DT host bridge creation API
Bjorn Helgaas
bhelgaas at google.com
Fri Aug 22 08:27:42 PDT 2014
On Fri, Aug 22, 2014 at 7:32 AM, Liviu Dudau <liviu at dudau.co.uk> 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 at dudau.co.uk> 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 at 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.
>> >
>> > 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@arm.com
>>
>> > 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 at vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>
> --
> -------------------
> .oooO
> ( )
> \ ( Oooo.
> \_) ( )
> ) /
> (_/
>
> One small step
> for me ...
>
More information about the linux-arm-kernel
mailing list