| Message ID | 1461970899-4150603-3-git-send-email-arnd@arndb.de (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Sat, Apr 30, 2016 at 01:01:38AM +0200, Arnd Bergmann wrote: > This changes the pci-host-common implementation to call the > new pci_register_host() interface instead of pci_scan_root_bus(). > > As a result, we get to share the pci_host_bridge structure > as it was originally intended anyway: We ended up having two > copies of pci_host_bridge here because we never got it done > until now. We can also remove the now unused "resources" > list_head, as we add the resources to the pci_host_bridge > directly. > > On top of this, further generalizations are possible to shrink > the pci-host-common.c file, in particular requesting the > resources and scanning for child devices can be moved > into pci_register_host(). > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > drivers/pci/host/pci-host-common.c | 38 ++++++++++++++++++++++++-------------- > drivers/pci/host/pci-host-common.h | 1 - > 2 files changed, 24 insertions(+), 15 deletions(-) > > diff --git a/drivers/pci/host/pci-host-common.c b/drivers/pci/host/pci-host-common.c > index e9f850f07968..3fc529f0297e 100644 > --- a/drivers/pci/host/pci-host-common.c > +++ b/drivers/pci/host/pci-host-common.c > @@ -26,7 +26,7 @@ > > static void gen_pci_release_of_pci_ranges(struct gen_pci *pci) > { > - pci_free_resource_list(&pci->resources); > + pci_free_resource_list(&pci->host.windows); > } > > static int gen_pci_parse_request_of_pci_ranges(struct gen_pci *pci) > @@ -37,12 +37,12 @@ static int gen_pci_parse_request_of_pci_ranges(struct gen_pci *pci) > resource_size_t iobase; > struct resource_entry *win; > > - err = of_pci_get_host_bridge_resources(np, 0, 0xff, &pci->resources, > + err = of_pci_get_host_bridge_resources(np, 0, 0xff, &pci->host.windows, > &iobase); > if (err) > return err; > > - resource_list_for_each_entry(win, &pci->resources) { > + resource_list_for_each_entry(win, &pci->host.windows) { > struct resource *parent, *res = win->res; > > switch (resource_type(res)) { > @@ -130,6 +130,14 @@ static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci) > return 0; > } > > +static void gen_pci_release(struct device *dev) > +{ > + struct gen_pci *pci = container_of(dev, struct gen_pci, host.dev); > + > + gen_pci_release_of_pci_ranges(pci); > + kfree(pci); > +} I don't really like the fact that the alloc of struct gen_pci is so far away from the free. I haven't looked hard enough to figure out if it's reasonable to put them closer. > int pci_host_common_probe(struct platform_device *pdev, > struct gen_pci *pci) > { > @@ -137,7 +145,7 @@ int pci_host_common_probe(struct platform_device *pdev, > const char *type; > struct device *dev = &pdev->dev; > struct device_node *np = dev->of_node; > - struct pci_bus *bus, *child; > + struct pci_bus *child; > > type = of_get_property(np, "device_type", NULL); > if (!type || strcmp(type, "pci")) { > @@ -148,8 +156,8 @@ int pci_host_common_probe(struct platform_device *pdev, > of_pci_check_probe_only(); > > pci->host.dev.parent = dev; > + pci->host.dev.release = gen_pci_release; > 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); > @@ -168,24 +176,26 @@ int pci_host_common_probe(struct platform_device *pdev, > pci_add_flags(PCI_REASSIGN_ALL_RSRC | PCI_REASSIGN_ALL_BUS); > > > - bus = pci_scan_root_bus(dev, pci->cfg.bus_range->start, > - &pci->cfg.ops->ops, pci, &pci->resources); > - if (!bus) { > - dev_err(dev, "Scanning rootbus failed"); > - return -ENODEV; > + pci->host.ops = &pci->cfg.ops->ops; > + pci->host.sysdata = pci; > + pci->host.busnr = pci->cfg.bus_range->start; > + err = pci_register_host(&pci->host); > + if (!err) { > + dev_err(dev, "registering host failed"); > + return err; > } Where do we actually scan the bus here? I don't see it in pci_register_host(). > pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci); > > if (!pci_has_flag(PCI_PROBE_ONLY)) { > - pci_bus_size_bridges(bus); > - pci_bus_assign_resources(bus); > + pci_bus_size_bridges(pci->host.bus); > + pci_bus_assign_resources(pci->host.bus); > > - list_for_each_entry(child, &bus->children, node) > + list_for_each_entry(child, &pci->host.bus->children, node) > pcie_bus_configure_settings(child); > } > > - pci_bus_add_devices(bus); > + pci_bus_add_devices(pci->host.bus); > return 0; > } > > diff --git a/drivers/pci/host/pci-host-common.h b/drivers/pci/host/pci-host-common.h > index 09f3fa0a55d7..c89a756420ee 100644 > --- a/drivers/pci/host/pci-host-common.h > +++ b/drivers/pci/host/pci-host-common.h > @@ -38,7 +38,6 @@ struct gen_pci_cfg_windows { > struct gen_pci { > struct pci_host_bridge host; > struct gen_pci_cfg_windows cfg; > - struct list_head resources; > }; > > int pci_host_common_probe(struct platform_device *pdev, > -- > 2.7.0 > > -- > 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 -- 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 04 May 2016 18:14:18 Bjorn Helgaas wrote: > On Sat, Apr 30, 2016 at 01:01:38AM +0200, Arnd Bergmann wrote: > > > > +static void gen_pci_release(struct device *dev) > > +{ > > + struct gen_pci *pci = container_of(dev, struct gen_pci, host.dev); > > + > > + gen_pci_release_of_pci_ranges(pci); > > + kfree(pci); > > +} > > I don't really like the fact that the alloc of struct gen_pci is so > far away from the free. I haven't looked hard enough to figure out if > it's reasonable to put them closer. It should be easy enough to move the release function next to the one that does the allocation. If we go the other route of having a generic pci_host_alloc() function that every host driver has to call instead of kzalloc(), we can probably drop the need to specify a release function in each driver. > > + err = pci_register_host(&pci->host); > > + if (!err) { > > + dev_err(dev, "registering host failed"); > > + return err; > > } > > Where do we actually scan the bus here? I don't see it in > pci_register_host(). Ah, you are right, that was a mistake. As I said, I have not tried running the code. I left this out of pci_register_host() for compatibility with pci_create_root_bus(), which also doesn't scan the bus, but then I didn't notice that I effectively remove the scan during the conversion of this driver. > > pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci); > > > > if (!pci_has_flag(PCI_PROBE_ONLY)) { > > - pci_bus_size_bridges(bus); > > - pci_bus_assign_resources(bus); > > + pci_bus_size_bridges(pci->host.bus); > > + pci_bus_assign_resources(pci->host.bus); > > > > - list_for_each_entry(child, &bus->children, node) > > + list_for_each_entry(child, &pci->host.bus->children, node) > > pcie_bus_configure_settings(child); > > } > > > > - pci_bus_add_devices(bus); > > + pci_bus_add_devices(pci->host.bus); > > return 0; I was actually thinking we could move both the scan and all the code above into pci_register_host(), based on some flags or other struct members we assign in the pci_host_bridge structure, with the most common combination being the default. I'm still unsure why we need to do the pci_fixup_irqs() instead of having the normal irq setting do the right thing, but if necessary, the host driver can set a flag to ask the core to do it, or we could add an optional function pointer to the of_irq_parse_and_map_pci function (or a host specific one if needed) to struct pci_ops and the call pci_common_swizzle with that. For all the other stuff (size_bridges, assign_resources, configure_settings, add_devices), I'd say a host driver should not really have to worry about this unless it needs to do something special inbetween. Of course we can't do it for the existing pci_scan_root_bus() etc, because the callers expect it not to be done. Arnd -- 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
diff --git a/drivers/pci/host/pci-host-common.c b/drivers/pci/host/pci-host-common.c index e9f850f07968..3fc529f0297e 100644 --- a/drivers/pci/host/pci-host-common.c +++ b/drivers/pci/host/pci-host-common.c @@ -26,7 +26,7 @@ static void gen_pci_release_of_pci_ranges(struct gen_pci *pci) { - pci_free_resource_list(&pci->resources); + pci_free_resource_list(&pci->host.windows); } static int gen_pci_parse_request_of_pci_ranges(struct gen_pci *pci) @@ -37,12 +37,12 @@ static int gen_pci_parse_request_of_pci_ranges(struct gen_pci *pci) resource_size_t iobase; struct resource_entry *win; - err = of_pci_get_host_bridge_resources(np, 0, 0xff, &pci->resources, + err = of_pci_get_host_bridge_resources(np, 0, 0xff, &pci->host.windows, &iobase); if (err) return err; - resource_list_for_each_entry(win, &pci->resources) { + resource_list_for_each_entry(win, &pci->host.windows) { struct resource *parent, *res = win->res; switch (resource_type(res)) { @@ -130,6 +130,14 @@ static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci) return 0; } +static void gen_pci_release(struct device *dev) +{ + struct gen_pci *pci = container_of(dev, struct gen_pci, host.dev); + + gen_pci_release_of_pci_ranges(pci); + kfree(pci); +} + int pci_host_common_probe(struct platform_device *pdev, struct gen_pci *pci) { @@ -137,7 +145,7 @@ int pci_host_common_probe(struct platform_device *pdev, const char *type; struct device *dev = &pdev->dev; struct device_node *np = dev->of_node; - struct pci_bus *bus, *child; + struct pci_bus *child; type = of_get_property(np, "device_type", NULL); if (!type || strcmp(type, "pci")) { @@ -148,8 +156,8 @@ int pci_host_common_probe(struct platform_device *pdev, of_pci_check_probe_only(); pci->host.dev.parent = dev; + pci->host.dev.release = gen_pci_release; 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); @@ -168,24 +176,26 @@ int pci_host_common_probe(struct platform_device *pdev, pci_add_flags(PCI_REASSIGN_ALL_RSRC | PCI_REASSIGN_ALL_BUS); - bus = pci_scan_root_bus(dev, pci->cfg.bus_range->start, - &pci->cfg.ops->ops, pci, &pci->resources); - if (!bus) { - dev_err(dev, "Scanning rootbus failed"); - return -ENODEV; + pci->host.ops = &pci->cfg.ops->ops; + pci->host.sysdata = pci; + pci->host.busnr = pci->cfg.bus_range->start; + err = pci_register_host(&pci->host); + if (!err) { + dev_err(dev, "registering host failed"); + return err; } pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci); if (!pci_has_flag(PCI_PROBE_ONLY)) { - pci_bus_size_bridges(bus); - pci_bus_assign_resources(bus); + pci_bus_size_bridges(pci->host.bus); + pci_bus_assign_resources(pci->host.bus); - list_for_each_entry(child, &bus->children, node) + list_for_each_entry(child, &pci->host.bus->children, node) pcie_bus_configure_settings(child); } - pci_bus_add_devices(bus); + pci_bus_add_devices(pci->host.bus); return 0; } diff --git a/drivers/pci/host/pci-host-common.h b/drivers/pci/host/pci-host-common.h index 09f3fa0a55d7..c89a756420ee 100644 --- a/drivers/pci/host/pci-host-common.h +++ b/drivers/pci/host/pci-host-common.h @@ -38,7 +38,6 @@ struct gen_pci_cfg_windows { struct gen_pci { struct pci_host_bridge host; struct gen_pci_cfg_windows cfg; - struct list_head resources; }; int pci_host_common_probe(struct platform_device *pdev,
This changes the pci-host-common implementation to call the new pci_register_host() interface instead of pci_scan_root_bus(). As a result, we get to share the pci_host_bridge structure as it was originally intended anyway: We ended up having two copies of pci_host_bridge here because we never got it done until now. We can also remove the now unused "resources" list_head, as we add the resources to the pci_host_bridge directly. On top of this, further generalizations are possible to shrink the pci-host-common.c file, in particular requesting the resources and scanning for child devices can be moved into pci_register_host(). Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- drivers/pci/host/pci-host-common.c | 38 ++++++++++++++++++++++++-------------- drivers/pci/host/pci-host-common.h | 1 - 2 files changed, 24 insertions(+), 15 deletions(-)