| Message ID | 1407861695-25549-1-git-send-email-Liviu.Dudau@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi guys, On Tue, Aug 12, 2014 at 05:41:35PM +0100, Liviu Dudau wrote: > From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > > In order to consolidate DT configuration for PCI host controllers in the > kernel, a new API was introduced that allows creating a host bridge > and its PCI bus from DT, removing duplicated code present in the > majority of pci host driver implementations. > > This patch converts the existing PCI generic host controller driver to > the new API. Most of the code parsing ranges and creating resources is > now delegated to of_create_pci_host_bridge() API which also triggers > a scan of the root bus. > > The setup hook passed by the host controller code to the generic DT > layer completes the initialization by performing resource filtering > (ie it checks that at least one non-prefetchable memory resource is > present), remapping IO and configuration regions and initializing > the required PCI flags. [...] > -static void gen_pci_release_of_pci_ranges(struct gen_pci *pci) > -{ > - struct pci_host_bridge_window *win; > - > - list_for_each_entry(win, &pci->resources, list) > - release_resource(win->res); > - > - pci_free_resource_list(&pci->resources); > -} I take it Liviu's core patches take care of this clean-up now if we fail mid way through requesting the resources? > -static int gen_pci_setup(int nr, struct pci_sys_data *sys) > +static int gen_pci_setup(struct pci_host_bridge *host, > + resource_size_t io_cpuaddr) > { > - struct gen_pci *pci = sys->private_data; > - list_splice_init(&pci->resources, &sys->resources); > - return 1; > + int err; > + struct pci_host_bridge_window *window; > + u32 restype, prefetchable; > + struct device *dev = host->dev.parent; > + struct resource *iores = NULL; > + bool res_valid = false; > + > + list_for_each_entry(window, &host->windows, list) { > + restype = window->res->flags & IORESOURCE_TYPE_BITS; > + prefetchable = window->res->flags & IORESOURCE_PREFETCH; > + > + /* > + * Require at least one non-prefetchable MEM resource > + */ > + if ((restype == IORESOURCE_MEM) && !prefetchable) > + res_valid = true; > + > + if (restype == IORESOURCE_IO) > + iores = window->res; > + } > + > + if (!res_valid) { > + dev_err(dev, "non-prefetchable memory resource required\n"); > + return -EINVAL; > + } > + > + if (iores) { > + if (!PAGE_ALIGNED(io_cpuaddr)) > + return -EINVAL; Why is this alignment check not in the core code? Probably a question for somebody like Arnd, but do we need to deal with multiple IO resources? Currently we'll just silently take the last one that we found, which doesn't sound ideal. > + if (err) > + return err; > + } > + > + /* Parse and map our Configuration Space windows */ > + err = gen_pci_parse_map_cfg_windows(host); > + if (err) > + return err; > + > + pci_add_flags(PCI_ENABLE_PROC_DOMAINS); > + pci_add_flags(PCI_REASSIGN_ALL_BUS | PCI_REASSIGN_ALL_RSRC); Why does this belong in the host controller driver and how does it interact with the probe-only property? Will
On Tuesday 19 August 2014, Will Deacon wrote: > On Tue, Aug 12, 2014 at 05:41:35PM +0100, Liviu Dudau wrote: > > + if (!res_valid) { > > + dev_err(dev, "non-prefetchable memory resource required\n"); > > + return -EINVAL; > > + } I don't see why this part should be in the host controller driver. It's really a sanity check that could apply anywhere, so could we just move it into the common code, or alternatively drop it? > > + if (iores) { > > + if (!PAGE_ALIGNED(io_cpuaddr)) > > + return -EINVAL; > > Why is this alignment check not in the core code? This confused me too. I have to look back at the core code, but I assume it's either aligned already based on the way this number gets created, or it can have an offset within the page that is the same as the offset within the physical address of iores and that hould be handled internally by pci_remap_iospace(). > Probably a question for > somebody like Arnd, but do we need to deal with multiple IO resources? > Currently we'll just silently take the last one that we found, which doesn't > sound ideal. I can't think of a case where you'd actually have multiple IO resources, but I agree we should either treat that as an error or handle it right. My guess is that handling it is actually easier. Arnd
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
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. Best regards, Liviu > -- > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
On Wednesday 20 August 2014, Liviu Dudau wrote: > 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. I'll try to get hold of Bjorn here and discuss it with him in person. I'd rather see a few extra lines in each driver than the complexity of callback funtions. > 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 main objection to the new approach is that it's different from most other subsystems doing the same thing. For a person reading the pci host driver implementation, when they are familiar with other device drivers, I think it's much clearer what is going on when smaller functions are called in sequence than to see one function passed into some other interface that you now have to read as well in order to understand when it gets called. Arnd
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. Bjorn
[+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
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@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. Yes, that makes sense and should address both yours and Arnd's concerns (I hope). I'm about to head for the second leg of my holiday where I'm going to loose my internet connection (funny enough, going east to west in Europe can result in worse coverage and higher fees), but when I'm going to get back on the 4th of September I will send v10 with this suggestion included. If it is not too much to ask, I would really appreciate if you and Arnd find the time to review the patchset and ACK the non controversial parts as I would like to ask for inclusion in -next as soon as I can. Best regards, Liviu > > Bjorn >
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@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. 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(). 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 >
On Wed, Aug 20, 2014 at 09:39:27PM +0200, Arnd Bergmann wrote: > On Wednesday 20 August 2014, Liviu Dudau wrote: > > 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. > > I'll try to get hold of Bjorn here and discuss it with him in person. I'd > rather see a few extra lines in each driver than the complexity of callback > funtions. > > > 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 main objection to the new approach is that it's different from most other > subsystems doing the same thing. For a person reading the pci host driver > implementation, when they are familiar with other device drivers, I think it's > much clearer what is going on when smaller functions are called in sequence > than to see one function passed into some other interface that you now have > to read as well in order to understand when it gets called. Would it be more clear if (when) the currently opaque sysdata becomes a structure to be filled by the host bridge driver with a .setup member pointing to the callback? That would be my preferred way of expressing the API, tbh, but it means duplicating the existing pci_{create,scan}_root_bus() functions. Best regards, Liviu > > Arnd >
On Thu, Aug 21, 2014 at 6:01 PM, Liviu Dudau <liviu@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@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. 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. > 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 ... >
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@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@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(). > 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. Best regards, Liviu > > 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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
On Fri, Aug 22, 2014 at 7:32 AM, Liviu Dudau <liviu@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@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@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@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> > > -- > ------------------- > .oooO > ( ) > \ ( Oooo. > \_) ( ) > ) / > (_/ > > One small step > for me ... >
Hi Will, sorry for the delay in replying (I was not copied in). On Tue, Aug 19, 2014 at 01:05:54PM +0100, Will Deacon wrote: > Hi guys, > > On Tue, Aug 12, 2014 at 05:41:35PM +0100, Liviu Dudau wrote: > > From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > > > > In order to consolidate DT configuration for PCI host controllers in the > > kernel, a new API was introduced that allows creating a host bridge > > and its PCI bus from DT, removing duplicated code present in the > > majority of pci host driver implementations. > > > > This patch converts the existing PCI generic host controller driver to > > the new API. Most of the code parsing ranges and creating resources is > > now delegated to of_create_pci_host_bridge() API which also triggers > > a scan of the root bus. > > > > The setup hook passed by the host controller code to the generic DT > > layer completes the initialization by performing resource filtering > > (ie it checks that at least one non-prefetchable memory resource is > > present), remapping IO and configuration regions and initializing > > the required PCI flags. > > [...] > > > -static void gen_pci_release_of_pci_ranges(struct gen_pci *pci) > > -{ > > - struct pci_host_bridge_window *win; > > - > > - list_for_each_entry(win, &pci->resources, list) > > - release_resource(win->res); > > - > > - pci_free_resource_list(&pci->resources); > > -} > > I take it Liviu's core patches take care of this clean-up now if we fail mid > way through requesting the resources? Liviu's core patches free the resource list, but do _not_ request the resources (so they won't release them). This is still a moot point that needs clarification. > > -static int gen_pci_setup(int nr, struct pci_sys_data *sys) > > +static int gen_pci_setup(struct pci_host_bridge *host, > > + resource_size_t io_cpuaddr) > > { > > - struct gen_pci *pci = sys->private_data; > > - list_splice_init(&pci->resources, &sys->resources); > > - return 1; > > + int err; > > + struct pci_host_bridge_window *window; > > + u32 restype, prefetchable; > > + struct device *dev = host->dev.parent; > > + struct resource *iores = NULL; > > + bool res_valid = false; > > + > > + list_for_each_entry(window, &host->windows, list) { > > + restype = window->res->flags & IORESOURCE_TYPE_BITS; > > + prefetchable = window->res->flags & IORESOURCE_PREFETCH; > > + > > + /* > > + * Require at least one non-prefetchable MEM resource > > + */ > > + if ((restype == IORESOURCE_MEM) && !prefetchable) > > + res_valid = true; > > + > > + if (restype == IORESOURCE_IO) > > + iores = window->res; > > + } > > + > > + if (!res_valid) { > > + dev_err(dev, "non-prefetchable memory resource required\n"); > > + return -EINVAL; > > + } > > + > > + if (iores) { > > + if (!PAGE_ALIGNED(io_cpuaddr)) > > + return -EINVAL; > > Why is this alignment check not in the core code? Probably a question for > somebody like Arnd, but do we need to deal with multiple IO resources? > Currently we'll just silently take the last one that we found, which doesn't > sound ideal. (1) Yes, the alignment check should be made in core code (2) I could take the first IO resource and warn on multiple IO resources if they are detected. Thoughts ? > > + if (err) > > + return err; > > + } > > + > > + /* Parse and map our Configuration Space windows */ > > + err = gen_pci_parse_map_cfg_windows(host); > > + if (err) > > + return err; > > + > > + pci_add_flags(PCI_ENABLE_PROC_DOMAINS); > > + pci_add_flags(PCI_REASSIGN_ALL_BUS | PCI_REASSIGN_ALL_RSRC); > > Why does this belong in the host controller driver and how does it interact > with the probe-only property? That's a very good question and it is one that confuses me too. Current code in pci_common_init_dev() sets PCI_REASSIGN_ALL_RSRC behind our backs silently. That flag has a side effect only if the probing code detects PCI bridges in the list of devices, since PCI core probing code will try to reassign the bus numbers upon PCI bridge detection IIUC. I do not think that PCI_REASSIGN_ALL_BUS has any side effect on ARM (by grepping around I noticed that PCI_REASSIGN_ALL_RSRC is used in pcibios_assign_all_busses() which in turn is triggered only if PCI bridges are detected, still grokking the code though). Apart from the PCI_ENABLE_PROC_DOMAINS flag which I think it is safe to set by default (but let me check that too), I *think* that setting of PCI_REASSIGN_ALL_BUS and PCI_REASSIGN_ALL_RSRC should be set by DT, as the probe-only flag is. At the moment this is not a problem (well...) since: (a) PCI_REASSIGN_ALL_RSRC is forced by pcibios on ARM (b) it has no implications on the generic host controller since it is run on kvmtools with no PCI bridge devices If Bjorn or Arnd can help me understand the complete reasoning behind those flags I would be very grateful and update code according to Liviu's v10. Thanks, Lorenzo
On Thursday 04 September 2014 14:39:56 Lorenzo Pieralisi wrote: > > > + if (!res_valid) { > > > + dev_err(dev, "non-prefetchable memory resource required\n"); > > > + return -EINVAL; > > > + } > > > + > > > + if (iores) { > > > + if (!PAGE_ALIGNED(io_cpuaddr)) > > > + return -EINVAL; > > > > Why is this alignment check not in the core code? Probably a question for > > somebody like Arnd, but do we need to deal with multiple IO resources? > > Currently we'll just silently take the last one that we found, which doesn't > > sound ideal. > > (1) Yes, the alignment check should be made in core code Makes sense. In theory we could support unaligned addresses (as long as the offset in the page is the same for virtual and physical), but I don't see why we should implement that unless some implementation absolutely requires it. > (2) I could take the first IO resource and warn on multiple IO resources if > they are detected. Thoughts ? I think we should either warn, or be reasonably sure that it will work. Again, in theory this should work, but no sane hardware implementation would do it like that. > > > + if (err) > > > + return err; > > > + } > > > + > > > + /* Parse and map our Configuration Space windows */ > > > + err = gen_pci_parse_map_cfg_windows(host); > > > + if (err) > > > + return err; > > > + > > > + pci_add_flags(PCI_ENABLE_PROC_DOMAINS); > > > + pci_add_flags(PCI_REASSIGN_ALL_BUS | PCI_REASSIGN_ALL_RSRC); > > > > Why does this belong in the host controller driver and how does it interact > > with the probe-only property? > > That's a very good question and it is one that confuses me too. > > Current code in pci_common_init_dev() sets PCI_REASSIGN_ALL_RSRC behind > our backs silently. That flag has a side effect only if the probing code > detects PCI bridges in the list of devices, since PCI core probing code > will try to reassign the bus numbers upon PCI bridge detection IIUC. I do > not think that PCI_REASSIGN_ALL_BUS has any side effect on ARM (by grepping > around I noticed that PCI_REASSIGN_ALL_RSRC is used in > > pcibios_assign_all_busses() > > which in turn is triggered only if PCI bridges are detected, still grokking > the code though). Interesting point: the generic implementation should probably not default to reassigning all buses at all. We could have a (host controller specific, but with standardized name) DT property for it, but it would be best if firmware already probes it to not have to do it again. > Apart from the PCI_ENABLE_PROC_DOMAINS flag which I think it is safe to > set by default (but let me check that too), I think it should be enabled here, as no legacy machine will use this driver. > I *think* that setting of > > PCI_REASSIGN_ALL_BUS and PCI_REASSIGN_ALL_RSRC > > should be set by DT, as the probe-only flag is. At the moment this is not a > problem (well...) since: > > (a) PCI_REASSIGN_ALL_RSRC is forced by pcibios on ARM > (b) it has no implications on the generic host controller since it is > run on kvmtools with no PCI bridge devices > > If Bjorn or Arnd can help me understand the complete reasoning behind > those flags I would be very grateful and update code according to > Liviu's v10. I suspect they were just copied over to preserve the existing behavior. Arnd
Hi Arnd, thanks for having a look. On Thu, Sep 04, 2014 at 03:05:53PM +0100, Arnd Bergmann wrote: > On Thursday 04 September 2014 14:39:56 Lorenzo Pieralisi wrote: > > > > + if (!res_valid) { > > > > + dev_err(dev, "non-prefetchable memory resource required\n"); > > > > + return -EINVAL; > > > > + } > > > > + > > > > + if (iores) { > > > > + if (!PAGE_ALIGNED(io_cpuaddr)) > > > > + return -EINVAL; > > > > > > Why is this alignment check not in the core code? Probably a question for > > > somebody like Arnd, but do we need to deal with multiple IO resources? > > > Currently we'll just silently take the last one that we found, which doesn't > > > sound ideal. > > > > (1) Yes, the alignment check should be made in core code > > Makes sense. In theory we could support unaligned addresses (as long as > the offset in the page is the same for virtual and physical), but I don't > see why we should implement that unless some implementation absolutely > requires it. > > > (2) I could take the first IO resource and warn on multiple IO resources if > > they are detected. Thoughts ? > > I think we should either warn, or be reasonably sure that it will work. > Again, in theory this should work, but no sane hardware implementation > would do it like that. Ok, I will refactor the IO resources mapping code on top of Livius's v10. > > > > + if (err) > > > > + return err; > > > > + } > > > > + > > > > + /* Parse and map our Configuration Space windows */ > > > > + err = gen_pci_parse_map_cfg_windows(host); > > > > + if (err) > > > > + return err; > > > > + > > > > + pci_add_flags(PCI_ENABLE_PROC_DOMAINS); > > > > + pci_add_flags(PCI_REASSIGN_ALL_BUS | PCI_REASSIGN_ALL_RSRC); > > > > > > Why does this belong in the host controller driver and how does it interact > > > with the probe-only property? > > > > That's a very good question and it is one that confuses me too. > > > > Current code in pci_common_init_dev() sets PCI_REASSIGN_ALL_RSRC behind > > our backs silently. That flag has a side effect only if the probing code > > detects PCI bridges in the list of devices, since PCI core probing code > > will try to reassign the bus numbers upon PCI bridge detection IIUC. I do > > not think that PCI_REASSIGN_ALL_BUS has any side effect on ARM (by grepping > > around I noticed that PCI_REASSIGN_ALL_RSRC is used in > > > > pcibios_assign_all_busses() > > > > which in turn is triggered only if PCI bridges are detected, still grokking > > the code though). > > Interesting point: the generic implementation should probably not default > to reassigning all buses at all. We could have a (host controller specific, > but with standardized name) DT property for it, but it would be best if > firmware already probes it to not have to do it again. I think that makes sense, let me point out though that this is *not* how the code works today, since the pcibios code sets: PCI_REASSIGN_ALL_RSRC (pci_common_init_dev() in arch/arm/kernel/bios32.c) by default. I won't set the PCI_REASSIGN_ALL_RSRC and PCI_REASSIGN_ALL_BUS flags. > > Apart from the PCI_ENABLE_PROC_DOMAINS flag which I think it is safe to > > set by default (but let me check that too), > > I think it should be enabled here, as no legacy machine will use this > driver. +1 I will wait for Liviu's v10 and repost accordingly. Lorenzo
On Thursday 04 September 2014 17:02:17 Lorenzo Pieralisi wrote: > On Thu, Sep 04, 2014 at 03:05:53PM +0100, Arnd Bergmann wrote: > > Interesting point: the generic implementation should probably not default > > to reassigning all buses at all. We could have a (host controller specific, > > but with standardized name) DT property for it, but it would be best if > > firmware already probes it to not have to do it again. > > I think that makes sense, let me point out though that this is > *not* how the code works today, since the pcibios code sets: > > PCI_REASSIGN_ALL_RSRC (pci_common_init_dev() in arch/arm/kernel/bios32.c) > > by default. I won't set the PCI_REASSIGN_ALL_RSRC and PCI_REASSIGN_ALL_BUS > flags. Yes, I know it's not what the ARM32 code does at the moment. The main reason for that is that there is normally no firmware at all. Since the new code should be shared with ARM64 and we typically have firmware there, we should reconsider the defaults. Arnd
diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c index 44fe6aa..24b519f 100644 --- a/drivers/pci/host/pci-host-generic.c +++ b/drivers/pci/host/pci-host-generic.c @@ -32,25 +32,21 @@ struct gen_pci_cfg_bus_ops { struct gen_pci_cfg_windows { struct resource res; - struct resource bus_range; void __iomem **win; const struct gen_pci_cfg_bus_ops *ops; }; struct gen_pci { - struct pci_host_bridge host; struct gen_pci_cfg_windows cfg; - struct list_head resources; }; static void __iomem *gen_pci_map_cfg_bus_cam(struct pci_bus *bus, unsigned int devfn, int where) { - struct pci_sys_data *sys = bus->sysdata; - struct gen_pci *pci = sys->private_data; - resource_size_t idx = bus->number - pci->cfg.bus_range.start; + struct gen_pci *pci = bus->sysdata; + resource_size_t idx = bus->number - bus->busn_res.start; return pci->cfg.win[idx] + ((devfn << 8) | where); } @@ -64,9 +60,8 @@ static void __iomem *gen_pci_map_cfg_bus_ecam(struct pci_bus *bus, unsigned int devfn, int where) { - struct pci_sys_data *sys = bus->sysdata; - struct gen_pci *pci = sys->private_data; - resource_size_t idx = bus->number - pci->cfg.bus_range.start; + struct gen_pci *pci = bus->sysdata; + resource_size_t idx = bus->number - bus->busn_res.start; return pci->cfg.win[idx] + ((devfn << 12) | where); } @@ -80,8 +75,7 @@ static int gen_pci_config_read(struct pci_bus *bus, unsigned int devfn, int where, int size, u32 *val) { void __iomem *addr; - struct pci_sys_data *sys = bus->sysdata; - struct gen_pci *pci = sys->private_data; + struct gen_pci *pci = bus->sysdata; addr = pci->cfg.ops->map_bus(bus, devfn, where); @@ -103,8 +97,7 @@ static int gen_pci_config_write(struct pci_bus *bus, unsigned int devfn, int where, int size, u32 val) { void __iomem *addr; - struct pci_sys_data *sys = bus->sysdata; - struct gen_pci *pci = sys->private_data; + struct gen_pci *pci = bus->sysdata; addr = pci->cfg.ops->map_bus(bus, devfn, where); @@ -138,162 +131,40 @@ static const struct of_device_id gen_pci_of_match[] = { }; MODULE_DEVICE_TABLE(of, gen_pci_of_match); -static int gen_pci_calc_io_offset(struct device *dev, - struct of_pci_range *range, - struct resource *res, - resource_size_t *offset) -{ - static atomic_t wins = ATOMIC_INIT(0); - int err, idx, max_win; - unsigned int window; - - if (!PAGE_ALIGNED(range->cpu_addr)) - return -EINVAL; - - max_win = (IO_SPACE_LIMIT + 1) / SZ_64K; - idx = atomic_inc_return(&wins); - if (idx > max_win) - return -ENOSPC; - - window = (idx - 1) * SZ_64K; - err = pci_ioremap_io(window, range->cpu_addr); - if (err) - return err; - - of_pci_range_to_resource(range, dev->of_node, res); - res->start = window; - res->end = res->start + range->size - 1; - *offset = window - range->pci_addr; - return 0; -} - -static int gen_pci_calc_mem_offset(struct device *dev, - struct of_pci_range *range, - struct resource *res, - resource_size_t *offset) -{ - of_pci_range_to_resource(range, dev->of_node, res); - *offset = range->cpu_addr - range->pci_addr; - return 0; -} - -static void gen_pci_release_of_pci_ranges(struct gen_pci *pci) -{ - struct pci_host_bridge_window *win; - - list_for_each_entry(win, &pci->resources, list) - release_resource(win->res); - - pci_free_resource_list(&pci->resources); -} - -static int gen_pci_parse_request_of_pci_ranges(struct gen_pci *pci) -{ - struct of_pci_range range; - struct of_pci_range_parser parser; - int err, res_valid = 0; - struct device *dev = pci->host.dev.parent; - struct device_node *np = dev->of_node; - - if (of_pci_range_parser_init(&parser, np)) { - dev_err(dev, "missing \"ranges\" property\n"); - return -EINVAL; - } - - for_each_of_pci_range(&parser, &range) { - struct resource *parent, *res; - resource_size_t offset; - u32 restype = range.flags & IORESOURCE_TYPE_BITS; - - res = devm_kmalloc(dev, sizeof(*res), GFP_KERNEL); - if (!res) { - err = -ENOMEM; - goto out_release_res; - } - - switch (restype) { - case IORESOURCE_IO: - parent = &ioport_resource; - err = gen_pci_calc_io_offset(dev, &range, res, &offset); - break; - case IORESOURCE_MEM: - parent = &iomem_resource; - err = gen_pci_calc_mem_offset(dev, &range, res, &offset); - res_valid |= !(res->flags & IORESOURCE_PREFETCH || err); - break; - default: - err = -EINVAL; - continue; - } - - if (err) { - dev_warn(dev, - "error %d: failed to add resource [type 0x%x, %lld bytes]\n", - err, restype, range.size); - continue; - } - - err = request_resource(parent, res); - if (err) - goto out_release_res; - - pci_add_resource_offset(&pci->resources, res, offset); - } - - if (!res_valid) { - dev_err(dev, "non-prefetchable memory resource required\n"); - err = -EINVAL; - goto out_release_res; - } - - return 0; - -out_release_res: - gen_pci_release_of_pci_ranges(pci); - return err; -} - -static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci) +static int gen_pci_parse_map_cfg_windows(struct pci_host_bridge *host) { int err; u8 bus_max; resource_size_t busn; - struct resource *bus_range; - struct device *dev = pci->host.dev.parent; + struct pci_bus *bus = host->bus; + struct gen_pci *pci = bus->sysdata; + struct resource *bus_range = &bus->busn_res; + struct device *dev = host->dev.parent; struct device_node *np = dev->of_node; - if (of_pci_parse_bus_range(np, &pci->cfg.bus_range)) - pci->cfg.bus_range = (struct resource) { - .name = np->name, - .start = 0, - .end = 0xff, - .flags = IORESOURCE_BUS, - }; - err = of_address_to_resource(np, 0, &pci->cfg.res); if (err) { dev_err(dev, "missing \"reg\" property\n"); return err; } - pci->cfg.win = devm_kcalloc(dev, resource_size(&pci->cfg.bus_range), + /* Limit the bus-range to fit within reg */ + bus_max = bus_range->start + + (resource_size(&pci->cfg.res) >> pci->cfg.ops->bus_shift) - 1; + pci_bus_update_busn_res_end(bus, min_t(resource_size_t, + bus_range->end, bus_max)); + + pci->cfg.win = devm_kcalloc(dev, resource_size(bus_range), sizeof(*pci->cfg.win), GFP_KERNEL); if (!pci->cfg.win) return -ENOMEM; - /* Limit the bus-range to fit within reg */ - bus_max = pci->cfg.bus_range.start + - (resource_size(&pci->cfg.res) >> pci->cfg.ops->bus_shift) - 1; - pci->cfg.bus_range.end = min_t(resource_size_t, pci->cfg.bus_range.end, - bus_max); - /* Map our Configuration Space windows */ if (!devm_request_mem_region(dev, pci->cfg.res.start, resource_size(&pci->cfg.res), "Configuration Space")) return -ENOMEM; - bus_range = &pci->cfg.bus_range; for (busn = bus_range->start; busn <= bus_range->end; ++busn) { u32 idx = busn - bus_range->start; u32 sz = 1 << pci->cfg.ops->bus_shift; @@ -305,34 +176,66 @@ static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci) return -ENOMEM; } - /* Register bus resource */ - pci_add_resource(&pci->resources, bus_range); return 0; } -static int gen_pci_setup(int nr, struct pci_sys_data *sys) +static int gen_pci_setup(struct pci_host_bridge *host, + resource_size_t io_cpuaddr) { - struct gen_pci *pci = sys->private_data; - list_splice_init(&pci->resources, &sys->resources); - return 1; + int err; + struct pci_host_bridge_window *window; + u32 restype, prefetchable; + struct device *dev = host->dev.parent; + struct resource *iores = NULL; + bool res_valid = false; + + list_for_each_entry(window, &host->windows, list) { + restype = window->res->flags & IORESOURCE_TYPE_BITS; + prefetchable = window->res->flags & IORESOURCE_PREFETCH; + + /* + * Require at least one non-prefetchable MEM resource + */ + if ((restype == IORESOURCE_MEM) && !prefetchable) + res_valid = true; + + if (restype == IORESOURCE_IO) + iores = window->res; + } + + if (!res_valid) { + dev_err(dev, "non-prefetchable memory resource required\n"); + return -EINVAL; + } + + if (iores) { + if (!PAGE_ALIGNED(io_cpuaddr)) + return -EINVAL; + + err = pci_remap_iospace(iores, io_cpuaddr); + if (err) + return err; + } + + /* Parse and map our Configuration Space windows */ + err = gen_pci_parse_map_cfg_windows(host); + if (err) + return err; + + pci_add_flags(PCI_ENABLE_PROC_DOMAINS); + pci_add_flags(PCI_REASSIGN_ALL_BUS | PCI_REASSIGN_ALL_RSRC); + + return 0; } static int gen_pci_probe(struct platform_device *pdev) { - int err; const char *type; const struct of_device_id *of_id; const int *prop; struct device *dev = &pdev->dev; struct device_node *np = dev->of_node; struct gen_pci *pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL); - struct hw_pci hw = { - .nr_controllers = 1, - .private_data = (void **)&pci, - .setup = gen_pci_setup, - .map_irq = of_irq_parse_and_map_pci, - .ops = &gen_pci_ops, - }; if (!pci) return -ENOMEM; @@ -353,24 +256,9 @@ static int gen_pci_probe(struct platform_device *pdev) of_id = of_match_node(gen_pci_of_match, np); pci->cfg.ops = of_id->data; - pci->host.dev.parent = dev; - INIT_LIST_HEAD(&pci->host.windows); - INIT_LIST_HEAD(&pci->resources); - /* Parse our PCI ranges and request their resources */ - err = gen_pci_parse_request_of_pci_ranges(pci); - if (err) - return err; - - /* Parse and map our Configuration Space windows */ - err = gen_pci_parse_map_cfg_windows(pci); - if (err) { - gen_pci_release_of_pci_ranges(pci); - return err; - } - - pci_common_init_dev(dev, &hw); - return 0; + return of_create_pci_host_bridge(dev, 0, 0xff, &gen_pci_ops, + gen_pci_setup, pci); } static struct platform_driver gen_pci_driver = {