| Message ID | 20170426111809.19922-4-lorenzo.pieralisi@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Apr 26, 2017 at 1:17 PM, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > Current pci_scan_root_bus() interface is made up of two main > code paths: > > - pci_create_root_bus() > - pci_scan_child_bus() > > pci_create_root_bus() is a wrapper function that allows to create > a struct pci_host_bridge structure, initialize it with the passed > parameters and register it with the kernel. > > As the struct pci_host_bridge require additional struct members, > pci_create_root_bus() parameters list has grown in time, making > it unwieldy to add further parameters to it in case the struct > pci_host_bridge gains more members fields to augment its functionality. > > Since PCI core code provides functions to allocate struct > pci_host_bridge, instead of forcing the pci_create_root_bus() interface > to add new parameters to cater for new struct pci_host_bridge > functionality, it is more suitable to add an interface in PCI > core code to scan a PCI bus straight from a struct pci_host_bridge > created and customized by each specific PCI host controller driver. > > Add a pci_scan_root_bus_bridge() function to allow PCI host controller > drivers to create and initialize struct pci_host_bridge and scan > the resulting bus. > > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: Bjorn Helgaas <bhelgaas@google.com> Good idea, yes. To avoid growing the number of interfaces too much, should we change the existing users of pci_register_host_bridge in host drivers over to this entry point, and make the other one local to probe.c then? > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index 7e4ffc4..c7a7f72 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -2412,6 +2412,44 @@ void pci_bus_release_busn_res(struct pci_bus *b) > res, ret ? "can not be" : "is"); > } > > +int pci_scan_root_bus_bridge(struct pci_host_bridge *bridge) > +{ > + struct resource_entry *window; > + bool found = false; > + struct pci_bus *b; > + int max, bus, ret; > + > + if (!bridge) > + return -EINVAL; > + > + resource_list_for_each_entry(window, &bridge->windows) > + if (window->res->flags & IORESOURCE_BUS) { > + found = true; > + break; > + } > + > + ret = pci_register_host_bridge(bridge); > + if (ret < 0) > + return ret; > + > + b = bridge->bus; > + bus = bridge->busnr; > + > + if (!found) { > + dev_info(&b->dev, > + "No busn resource found for root bus, will use [bus %02x-ff]\n", > + bus); > + pci_bus_insert_busn_res(b, bus, 255); > + } > + > + max = pci_scan_child_bus(b); > + > + if (!found) > + pci_bus_update_busn_res_end(b, max); > + > + return 0; > +} > + We probably want an EXPORT_SYMBOL() here as well. Arnd
On Fri, Apr 28, 2017 at 02:28:38PM +0200, Arnd Bergmann wrote: > On Wed, Apr 26, 2017 at 1:17 PM, Lorenzo Pieralisi > <lorenzo.pieralisi@arm.com> wrote: > > Current pci_scan_root_bus() interface is made up of two main > > code paths: > > > > - pci_create_root_bus() > > - pci_scan_child_bus() > > > > pci_create_root_bus() is a wrapper function that allows to create > > a struct pci_host_bridge structure, initialize it with the passed > > parameters and register it with the kernel. > > > > As the struct pci_host_bridge require additional struct members, > > pci_create_root_bus() parameters list has grown in time, making > > it unwieldy to add further parameters to it in case the struct > > pci_host_bridge gains more members fields to augment its functionality. > > > > Since PCI core code provides functions to allocate struct > > pci_host_bridge, instead of forcing the pci_create_root_bus() interface > > to add new parameters to cater for new struct pci_host_bridge > > functionality, it is more suitable to add an interface in PCI > > core code to scan a PCI bus straight from a struct pci_host_bridge > > created and customized by each specific PCI host controller driver. > > > > Add a pci_scan_root_bus_bridge() function to allow PCI host controller > > drivers to create and initialize struct pci_host_bridge and scan > > the resulting bus. > > > > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > > Cc: Arnd Bergmann <arnd@arndb.de> > > Cc: Bjorn Helgaas <bhelgaas@google.com> > > Good idea, yes. To avoid growing the number of interfaces too > much, should we change the existing users of pci_register_host_bridge > in host drivers over to this entry point, and make the other one > local to probe.c then? Yes, the problem is that there are drivers (ie pcie-iproc.c) that require the struct pci_bus (created by pci_register_host_bridge()) to fiddle with it to check link status and THEN scan the bus (so the pci_register_host_bridge() call can't be embedded in the scan interface - the driver requires struct pci_bus for pci_ops to work before scanning the bus itself). I will see how I can accommodate this change because you definitely have a point. > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > > index 7e4ffc4..c7a7f72 100644 > > --- a/drivers/pci/probe.c > > +++ b/drivers/pci/probe.c > > @@ -2412,6 +2412,44 @@ void pci_bus_release_busn_res(struct pci_bus *b) > > res, ret ? "can not be" : "is"); > > } > > > > +int pci_scan_root_bus_bridge(struct pci_host_bridge *bridge) > > +{ > > + struct resource_entry *window; > > + bool found = false; > > + struct pci_bus *b; > > + int max, bus, ret; > > + > > + if (!bridge) > > + return -EINVAL; > > + > > + resource_list_for_each_entry(window, &bridge->windows) > > + if (window->res->flags & IORESOURCE_BUS) { > > + found = true; > > + break; > > + } > > + > > + ret = pci_register_host_bridge(bridge); > > + if (ret < 0) > > + return ret; > > + > > + b = bridge->bus; > > + bus = bridge->busnr; > > + > > + if (!found) { > > + dev_info(&b->dev, > > + "No busn resource found for root bus, will use [bus %02x-ff]\n", > > + bus); > > + pci_bus_insert_busn_res(b, bus, 255); > > + } > > + > > + max = pci_scan_child_bus(b); > > + > > + if (!found) > > + pci_bus_update_busn_res_end(b, max); > > + > > + return 0; > > +} > > + > > We probably want an EXPORT_SYMBOL() here as well. Yep, sure. Thanks for having a look ! Lorenzo
On Tue, May 2, 2017 at 7:15 PM, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > On Fri, Apr 28, 2017 at 02:28:38PM +0200, Arnd Bergmann wrote: >> On Wed, Apr 26, 2017 at 1:17 PM, Lorenzo Pieralisi >> <lorenzo.pieralisi@arm.com> wrote: >> >> Good idea, yes. To avoid growing the number of interfaces too >> much, should we change the existing users of pci_register_host_bridge >> in host drivers over to this entry point, and make the other one >> local to probe.c then? > > Yes, the problem is that there are drivers (ie pcie-iproc.c) that > require the struct pci_bus (created by pci_register_host_bridge()) > to fiddle with it to check link status and THEN scan the bus (so > the pci_register_host_bridge() call can't be embedded in the scan > interface - the driver requires struct pci_bus for pci_ops to work > before scanning the bus itself). > > I will see how I can accommodate this change because you definitely > have a point. The obvious answer for that particular problem would be a link_check() callback in the bridge operations. I think that would also fit in well with the dw_pcie driver that has some private infrastructure for it. I don't know if that callback is sufficient to solve all related problems though. Arnd
[+cc Ray, Scott, Jon] On Tue, May 02, 2017 at 06:15:01PM +0100, Lorenzo Pieralisi wrote: > On Fri, Apr 28, 2017 at 02:28:38PM +0200, Arnd Bergmann wrote: > > On Wed, Apr 26, 2017 at 1:17 PM, Lorenzo Pieralisi > > <lorenzo.pieralisi@arm.com> wrote: > > > Current pci_scan_root_bus() interface is made up of two main > > > code paths: > > > > > > - pci_create_root_bus() > > > - pci_scan_child_bus() > > > > > > pci_create_root_bus() is a wrapper function that allows to create > > > a struct pci_host_bridge structure, initialize it with the passed > > > parameters and register it with the kernel. > > > > > > As the struct pci_host_bridge require additional struct members, > > > pci_create_root_bus() parameters list has grown in time, making > > > it unwieldy to add further parameters to it in case the struct > > > pci_host_bridge gains more members fields to augment its functionality. > > > > > > Since PCI core code provides functions to allocate struct > > > pci_host_bridge, instead of forcing the pci_create_root_bus() interface > > > to add new parameters to cater for new struct pci_host_bridge > > > functionality, it is more suitable to add an interface in PCI > > > core code to scan a PCI bus straight from a struct pci_host_bridge > > > created and customized by each specific PCI host controller driver. > > > > > > Add a pci_scan_root_bus_bridge() function to allow PCI host controller > > > drivers to create and initialize struct pci_host_bridge and scan > > > the resulting bus. > > > > > > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > > > Cc: Arnd Bergmann <arnd@arndb.de> > > > Cc: Bjorn Helgaas <bhelgaas@google.com> > > > > Good idea, yes. To avoid growing the number of interfaces too > > much, should we change the existing users of pci_register_host_bridge > > in host drivers over to this entry point, and make the other one > > local to probe.c then? > > Yes, the problem is that there are drivers (ie pcie-iproc.c) that > require the struct pci_bus (created by pci_register_host_bridge()) > to fiddle with it to check link status and THEN scan the bus (so > the pci_register_host_bridge() call can't be embedded in the scan > interface - the driver requires struct pci_bus for pci_ops to work > before scanning the bus itself). I think code like iproc_pcie_check_link() that requires a struct pci_bus before we even scan the bus is lame. I think the driver should be able to bring up the link before telling the PCI core about the bridge. Aardvark uses a typical pattern: advk_pcie_probe advk_pcie_setup_hw advk_pcie_wait_for_link pci_scan_root_bus I would rather see iproc restructured along that line than add a callback. That would require replacing the pci_bus_read_config uses in iproc_pcie_check_link() with something different, maybe iproc-internal accessors. Slightly messy, but I think doable. > I will see how I can accommodate this change because you definitely > have a point. > > > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > > > index 7e4ffc4..c7a7f72 100644 > > > --- a/drivers/pci/probe.c > > > +++ b/drivers/pci/probe.c > > > @@ -2412,6 +2412,44 @@ void pci_bus_release_busn_res(struct pci_bus *b) > > > res, ret ? "can not be" : "is"); > > > } > > > > > > +int pci_scan_root_bus_bridge(struct pci_host_bridge *bridge) > > > +{ > > > + struct resource_entry *window; > > > + bool found = false; > > > + struct pci_bus *b; > > > + int max, bus, ret; > > > + > > > + if (!bridge) > > > + return -EINVAL; > > > + > > > + resource_list_for_each_entry(window, &bridge->windows) > > > + if (window->res->flags & IORESOURCE_BUS) { > > > + found = true; > > > + break; > > > + } > > > + > > > + ret = pci_register_host_bridge(bridge); > > > + if (ret < 0) > > > + return ret; > > > + > > > + b = bridge->bus; > > > + bus = bridge->busnr; > > > + > > > + if (!found) { > > > + dev_info(&b->dev, > > > + "No busn resource found for root bus, will use [bus %02x-ff]\n", > > > + bus); > > > + pci_bus_insert_busn_res(b, bus, 255); > > > + } > > > + > > > + max = pci_scan_child_bus(b); > > > + > > > + if (!found) > > > + pci_bus_update_busn_res_end(b, max); > > > + > > > + return 0; > > > +} > > > + > > > > We probably want an EXPORT_SYMBOL() here as well. > > Yep, sure. > > Thanks for having a look ! > > Lorenzo > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Thu, May 25, 2017 at 03:56:43PM -0500, Bjorn Helgaas wrote: > [+cc Ray, Scott, Jon] > > On Tue, May 02, 2017 at 06:15:01PM +0100, Lorenzo Pieralisi wrote: > > On Fri, Apr 28, 2017 at 02:28:38PM +0200, Arnd Bergmann wrote: > > > On Wed, Apr 26, 2017 at 1:17 PM, Lorenzo Pieralisi > > > <lorenzo.pieralisi@arm.com> wrote: > > > > Current pci_scan_root_bus() interface is made up of two main > > > > code paths: > > > > > > > > - pci_create_root_bus() > > > > - pci_scan_child_bus() > > > > > > > > pci_create_root_bus() is a wrapper function that allows to create > > > > a struct pci_host_bridge structure, initialize it with the passed > > > > parameters and register it with the kernel. > > > > > > > > As the struct pci_host_bridge require additional struct members, > > > > pci_create_root_bus() parameters list has grown in time, making > > > > it unwieldy to add further parameters to it in case the struct > > > > pci_host_bridge gains more members fields to augment its functionality. > > > > > > > > Since PCI core code provides functions to allocate struct > > > > pci_host_bridge, instead of forcing the pci_create_root_bus() interface > > > > to add new parameters to cater for new struct pci_host_bridge > > > > functionality, it is more suitable to add an interface in PCI > > > > core code to scan a PCI bus straight from a struct pci_host_bridge > > > > created and customized by each specific PCI host controller driver. > > > > > > > > Add a pci_scan_root_bus_bridge() function to allow PCI host controller > > > > drivers to create and initialize struct pci_host_bridge and scan > > > > the resulting bus. > > > > > > > > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > > > > Cc: Arnd Bergmann <arnd@arndb.de> > > > > Cc: Bjorn Helgaas <bhelgaas@google.com> > > > > > > Good idea, yes. To avoid growing the number of interfaces too > > > much, should we change the existing users of pci_register_host_bridge > > > in host drivers over to this entry point, and make the other one > > > local to probe.c then? > > > > Yes, the problem is that there are drivers (ie pcie-iproc.c) that > > require the struct pci_bus (created by pci_register_host_bridge()) > > to fiddle with it to check link status and THEN scan the bus (so > > the pci_register_host_bridge() call can't be embedded in the scan > > interface - the driver requires struct pci_bus for pci_ops to work > > before scanning the bus itself). > > I think code like iproc_pcie_check_link() that requires a struct > pci_bus before we even scan the bus is lame. I think the driver > should be able to bring up the link before telling the PCI core about > the bridge. Aardvark uses a typical pattern: > > advk_pcie_probe > advk_pcie_setup_hw > advk_pcie_wait_for_link > pci_scan_root_bus > > I would rather see iproc restructured along that line than add a > callback. > > That would require replacing the pci_bus_read_config uses in > iproc_pcie_check_link() with something different, maybe iproc-internal > accessors. Slightly messy, but I think doable. I agree with you, it probably requires some cfg space accessors copy and paste though but that's doable. I can write the patch myself but I can't test it so help is appreciated here. Thanks, Lorenzo > > I will see how I can accommodate this change because you definitely > > have a point. > > > > > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > > > > index 7e4ffc4..c7a7f72 100644 > > > > --- a/drivers/pci/probe.c > > > > +++ b/drivers/pci/probe.c > > > > @@ -2412,6 +2412,44 @@ void pci_bus_release_busn_res(struct pci_bus *b) > > > > res, ret ? "can not be" : "is"); > > > > } > > > > > > > > +int pci_scan_root_bus_bridge(struct pci_host_bridge *bridge) > > > > +{ > > > > + struct resource_entry *window; > > > > + bool found = false; > > > > + struct pci_bus *b; > > > > + int max, bus, ret; > > > > + > > > > + if (!bridge) > > > > + return -EINVAL; > > > > + > > > > + resource_list_for_each_entry(window, &bridge->windows) > > > > + if (window->res->flags & IORESOURCE_BUS) { > > > > + found = true; > > > > + break; > > > > + } > > > > + > > > > + ret = pci_register_host_bridge(bridge); > > > > + if (ret < 0) > > > > + return ret; > > > > + > > > > + b = bridge->bus; > > > > + bus = bridge->busnr; > > > > + > > > > + if (!found) { > > > > + dev_info(&b->dev, > > > > + "No busn resource found for root bus, will use [bus %02x-ff]\n", > > > > + bus); > > > > + pci_bus_insert_busn_res(b, bus, 255); > > > > + } > > > > + > > > > + max = pci_scan_child_bus(b); > > > > + > > > > + if (!found) > > > > + pci_bus_update_busn_res_end(b, max); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > > > We probably want an EXPORT_SYMBOL() here as well. > > > > Yep, sure. > > > > Thanks for having a look ! > > > > Lorenzo > > > > _______________________________________________ > > linux-arm-kernel mailing list > > linux-arm-kernel@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 5/26/17 6:07 AM, Lorenzo Pieralisi wrote: > On Thu, May 25, 2017 at 03:56:43PM -0500, Bjorn Helgaas wrote: >> [+cc Ray, Scott, Jon] >> >> On Tue, May 02, 2017 at 06:15:01PM +0100, Lorenzo Pieralisi wrote: >>> On Fri, Apr 28, 2017 at 02:28:38PM +0200, Arnd Bergmann wrote: >>>> On Wed, Apr 26, 2017 at 1:17 PM, Lorenzo Pieralisi >>>> <lorenzo.pieralisi@arm.com> wrote: >>>>> Current pci_scan_root_bus() interface is made up of two main >>>>> code paths: >>>>> >>>>> - pci_create_root_bus() >>>>> - pci_scan_child_bus() >>>>> >>>>> pci_create_root_bus() is a wrapper function that allows to create >>>>> a struct pci_host_bridge structure, initialize it with the passed >>>>> parameters and register it with the kernel. >>>>> >>>>> As the struct pci_host_bridge require additional struct members, >>>>> pci_create_root_bus() parameters list has grown in time, making >>>>> it unwieldy to add further parameters to it in case the struct >>>>> pci_host_bridge gains more members fields to augment its functionality. >>>>> >>>>> Since PCI core code provides functions to allocate struct >>>>> pci_host_bridge, instead of forcing the pci_create_root_bus() interface >>>>> to add new parameters to cater for new struct pci_host_bridge >>>>> functionality, it is more suitable to add an interface in PCI >>>>> core code to scan a PCI bus straight from a struct pci_host_bridge >>>>> created and customized by each specific PCI host controller driver. >>>>> >>>>> Add a pci_scan_root_bus_bridge() function to allow PCI host controller >>>>> drivers to create and initialize struct pci_host_bridge and scan >>>>> the resulting bus. >>>>> >>>>> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> >>>>> Cc: Arnd Bergmann <arnd@arndb.de> >>>>> Cc: Bjorn Helgaas <bhelgaas@google.com> >>>> >>>> Good idea, yes. To avoid growing the number of interfaces too >>>> much, should we change the existing users of pci_register_host_bridge >>>> in host drivers over to this entry point, and make the other one >>>> local to probe.c then? >>> >>> Yes, the problem is that there are drivers (ie pcie-iproc.c) that >>> require the struct pci_bus (created by pci_register_host_bridge()) >>> to fiddle with it to check link status and THEN scan the bus (so >>> the pci_register_host_bridge() call can't be embedded in the scan >>> interface - the driver requires struct pci_bus for pci_ops to work >>> before scanning the bus itself). >> >> I think code like iproc_pcie_check_link() that requires a struct >> pci_bus before we even scan the bus is lame. I think the driver >> should be able to bring up the link before telling the PCI core about >> the bridge. Aardvark uses a typical pattern: >> >> advk_pcie_probe >> advk_pcie_setup_hw >> advk_pcie_wait_for_link >> pci_scan_root_bus >> >> I would rather see iproc restructured along that line than add a >> callback. >> >> That would require replacing the pci_bus_read_config uses in >> iproc_pcie_check_link() with something different, maybe iproc-internal >> accessors. Slightly messy, but I think doable. > > I agree with you, it probably requires some cfg space accessors copy > and paste though but that's doable. I can write the patch myself but > I can't test it so help is appreciated here. > > Thanks, > Lorenzo > I agree with Bjorn on the new proposed sequence that link check should be done before the standard root bus scan starts. Lorenzo, I'm getting quite busy and won't have time to re-write this in the near term. I appreciate that you offer to help to re-organize the code. Once you have a patch, I can help to test it on our platforms. Thanks, Ray >>> I will see how I can accommodate this change because you definitely >>> have a point. >>> >>>>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c >>>>> index 7e4ffc4..c7a7f72 100644 >>>>> --- a/drivers/pci/probe.c >>>>> +++ b/drivers/pci/probe.c >>>>> @@ -2412,6 +2412,44 @@ void pci_bus_release_busn_res(struct pci_bus *b) >>>>> res, ret ? "can not be" : "is"); >>>>> } >>>>> >>>>> +int pci_scan_root_bus_bridge(struct pci_host_bridge *bridge) >>>>> +{ >>>>> + struct resource_entry *window; >>>>> + bool found = false; >>>>> + struct pci_bus *b; >>>>> + int max, bus, ret; >>>>> + >>>>> + if (!bridge) >>>>> + return -EINVAL; >>>>> + >>>>> + resource_list_for_each_entry(window, &bridge->windows) >>>>> + if (window->res->flags & IORESOURCE_BUS) { >>>>> + found = true; >>>>> + break; >>>>> + } >>>>> + >>>>> + ret = pci_register_host_bridge(bridge); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> + b = bridge->bus; >>>>> + bus = bridge->busnr; >>>>> + >>>>> + if (!found) { >>>>> + dev_info(&b->dev, >>>>> + "No busn resource found for root bus, will use [bus %02x-ff]\n", >>>>> + bus); >>>>> + pci_bus_insert_busn_res(b, bus, 255); >>>>> + } >>>>> + >>>>> + max = pci_scan_child_bus(b); >>>>> + >>>>> + if (!found) >>>>> + pci_bus_update_busn_res_end(b, max); >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>> >>>> We probably want an EXPORT_SYMBOL() here as well. >>> >>> Yep, sure. >>> >>> Thanks for having a look ! >>> >>> Lorenzo >>> >>> _______________________________________________ >>> linux-arm-kernel mailing list >>> linux-arm-kernel@lists.infradead.org >>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Fri, May 26, 2017 at 6:37 PM, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > On Thu, May 25, 2017 at 03:56:43PM -0500, Bjorn Helgaas wrote: >> [+cc Ray, Scott, Jon] >> >> On Tue, May 02, 2017 at 06:15:01PM +0100, Lorenzo Pieralisi wrote: >> > On Fri, Apr 28, 2017 at 02:28:38PM +0200, Arnd Bergmann wrote: >> > > On Wed, Apr 26, 2017 at 1:17 PM, Lorenzo Pieralisi >> > > <lorenzo.pieralisi@arm.com> wrote: >> > > > Current pci_scan_root_bus() interface is made up of two main >> > > > code paths: >> > > > >> > > > - pci_create_root_bus() >> > > > - pci_scan_child_bus() >> > > > >> > > > pci_create_root_bus() is a wrapper function that allows to create >> > > > a struct pci_host_bridge structure, initialize it with the passed >> > > > parameters and register it with the kernel. >> > > > >> > > > As the struct pci_host_bridge require additional struct members, >> > > > pci_create_root_bus() parameters list has grown in time, making >> > > > it unwieldy to add further parameters to it in case the struct >> > > > pci_host_bridge gains more members fields to augment its functionality. >> > > > >> > > > Since PCI core code provides functions to allocate struct >> > > > pci_host_bridge, instead of forcing the pci_create_root_bus() interface >> > > > to add new parameters to cater for new struct pci_host_bridge >> > > > functionality, it is more suitable to add an interface in PCI >> > > > core code to scan a PCI bus straight from a struct pci_host_bridge >> > > > created and customized by each specific PCI host controller driver. >> > > > >> > > > Add a pci_scan_root_bus_bridge() function to allow PCI host controller >> > > > drivers to create and initialize struct pci_host_bridge and scan >> > > > the resulting bus. >> > > > >> > > > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> >> > > > Cc: Arnd Bergmann <arnd@arndb.de> >> > > > Cc: Bjorn Helgaas <bhelgaas@google.com> >> > > >> > > Good idea, yes. To avoid growing the number of interfaces too >> > > much, should we change the existing users of pci_register_host_bridge >> > > in host drivers over to this entry point, and make the other one >> > > local to probe.c then? >> > >> > Yes, the problem is that there are drivers (ie pcie-iproc.c) that >> > require the struct pci_bus (created by pci_register_host_bridge()) >> > to fiddle with it to check link status and THEN scan the bus (so >> > the pci_register_host_bridge() call can't be embedded in the scan >> > interface - the driver requires struct pci_bus for pci_ops to work >> > before scanning the bus itself). >> >> I think code like iproc_pcie_check_link() that requires a struct >> pci_bus before we even scan the bus is lame. I think the driver >> should be able to bring up the link before telling the PCI core about >> the bridge. Aardvark uses a typical pattern: >> >> advk_pcie_probe >> advk_pcie_setup_hw >> advk_pcie_wait_for_link >> pci_scan_root_bus >> >> I would rather see iproc restructured along that line than add a >> callback. >> >> That would require replacing the pci_bus_read_config uses in >> iproc_pcie_check_link() with something different, maybe iproc-internal >> accessors. Slightly messy, but I think doable. > > I agree with you, it probably requires some cfg space accessors copy > and paste though but that's doable. I can write the patch myself but > I can't test it so help is appreciated here. > > Thanks, > Lorenzo > >> > I will see how I can accommodate this change because you definitely >> > have a point. >> > >> > > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c >> > > > index 7e4ffc4..c7a7f72 100644 >> > > > --- a/drivers/pci/probe.c >> > > > +++ b/drivers/pci/probe.c >> > > > @@ -2412,6 +2412,44 @@ void pci_bus_release_busn_res(struct pci_bus *b) >> > > > res, ret ? "can not be" : "is"); >> > > > } >> > > > >> > > > +int pci_scan_root_bus_bridge(struct pci_host_bridge *bridge) >> > > > +{ >> > > > + struct resource_entry *window; >> > > > + bool found = false; >> > > > + struct pci_bus *b; >> > > > + int max, bus, ret; >> > > > + >> > > > + if (!bridge) >> > > > + return -EINVAL; >> > > > + >> > > > + resource_list_for_each_entry(window, &bridge->windows) >> > > > + if (window->res->flags & IORESOURCE_BUS) { >> > > > + found = true; >> > > > + break; >> > > > + } >> > > > + >> > > > + ret = pci_register_host_bridge(bridge); >> > > > + if (ret < 0) >> > > > + return ret; >> > > > + >> > > > + b = bridge->bus; >> > > > + bus = bridge->busnr; >> > > > + >> > > > + if (!found) { >> > > > + dev_info(&b->dev, >> > > > + "No busn resource found for root bus, will use [bus %02x-ff]\n", >> > > > + bus); >> > > > + pci_bus_insert_busn_res(b, bus, 255); >> > > > + } >> > > > + >> > > > + max = pci_scan_child_bus(b); >> > > > + >> > > > + if (!found) >> > > > + pci_bus_update_busn_res_end(b, max); >> > > > + >> > > > + return 0; >> > > > +} >> > > > + >> > > >> > > We probably want an EXPORT_SYMBOL() here as well. >> > >> > Yep, sure. >> > >> > Thanks for having a look ! >> > >> > Lorenzo >> > >> > _______________________________________________ >> > linux-arm-kernel mailing list >> > linux-arm-kernel@lists.infradead.org >> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel Hi, This is good idea, because the following patch of mine attempts to add inbound windows, and I had to create another API pci_create_root_bus2 [PATCH v7 2/3] PCI: Add support for PCI inbound windows resources Lorenzo : Once this API is ready, please send it across. I can test it with respect to inbound resources and IOVA reservation on our platform. Regards, Oza.
On Thu, May 25, 2017 at 03:56:43PM -0500, Bjorn Helgaas wrote: > [+cc Ray, Scott, Jon] > > On Tue, May 02, 2017 at 06:15:01PM +0100, Lorenzo Pieralisi wrote: > > On Fri, Apr 28, 2017 at 02:28:38PM +0200, Arnd Bergmann wrote: > > > On Wed, Apr 26, 2017 at 1:17 PM, Lorenzo Pieralisi > > > <lorenzo.pieralisi@arm.com> wrote: > > > > Current pci_scan_root_bus() interface is made up of two main > > > > code paths: > > > > > > > > - pci_create_root_bus() > > > > - pci_scan_child_bus() > > > > > > > > pci_create_root_bus() is a wrapper function that allows to create > > > > a struct pci_host_bridge structure, initialize it with the passed > > > > parameters and register it with the kernel. > > > > > > > > As the struct pci_host_bridge require additional struct members, > > > > pci_create_root_bus() parameters list has grown in time, making > > > > it unwieldy to add further parameters to it in case the struct > > > > pci_host_bridge gains more members fields to augment its functionality. > > > > > > > > Since PCI core code provides functions to allocate struct > > > > pci_host_bridge, instead of forcing the pci_create_root_bus() interface > > > > to add new parameters to cater for new struct pci_host_bridge > > > > functionality, it is more suitable to add an interface in PCI > > > > core code to scan a PCI bus straight from a struct pci_host_bridge > > > > created and customized by each specific PCI host controller driver. > > > > > > > > Add a pci_scan_root_bus_bridge() function to allow PCI host controller > > > > drivers to create and initialize struct pci_host_bridge and scan > > > > the resulting bus. > > > > > > > > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > > > > Cc: Arnd Bergmann <arnd@arndb.de> > > > > Cc: Bjorn Helgaas <bhelgaas@google.com> > > > > > > Good idea, yes. To avoid growing the number of interfaces too > > > much, should we change the existing users of pci_register_host_bridge > > > in host drivers over to this entry point, and make the other one > > > local to probe.c then? > > > > Yes, the problem is that there are drivers (ie pcie-iproc.c) that > > require the struct pci_bus (created by pci_register_host_bridge()) > > to fiddle with it to check link status and THEN scan the bus (so > > the pci_register_host_bridge() call can't be embedded in the scan > > interface - the driver requires struct pci_bus for pci_ops to work > > before scanning the bus itself). > > I think code like iproc_pcie_check_link() that requires a struct > pci_bus before we even scan the bus is lame. I think the driver > should be able to bring up the link before telling the PCI core about > the bridge. Aardvark uses a typical pattern: > > advk_pcie_probe > advk_pcie_setup_hw > advk_pcie_wait_for_link > pci_scan_root_bus > > I would rather see iproc restructured along that line than add a > callback. > > That would require replacing the pci_bus_read_config uses in > iproc_pcie_check_link() with something different, maybe iproc-internal > accessors. Slightly messy, but I think doable. And now I have noticed pci-ftpci100.c requires the same patching for the same reason. I hope I will get this series merged before other DT PCI host controller drivers are because it is honestly becoming unmanageable for me to patch them all - it is time to consolidate them, copy and paste is doing damage here and will soon become impossible to fix. Thanks, Lorenzo > > I will see how I can accommodate this change because you definitely > > have a point. > > > > > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > > > > index 7e4ffc4..c7a7f72 100644 > > > > --- a/drivers/pci/probe.c > > > > +++ b/drivers/pci/probe.c > > > > @@ -2412,6 +2412,44 @@ void pci_bus_release_busn_res(struct pci_bus *b) > > > > res, ret ? "can not be" : "is"); > > > > } > > > > > > > > +int pci_scan_root_bus_bridge(struct pci_host_bridge *bridge) > > > > +{ > > > > + struct resource_entry *window; > > > > + bool found = false; > > > > + struct pci_bus *b; > > > > + int max, bus, ret; > > > > + > > > > + if (!bridge) > > > > + return -EINVAL; > > > > + > > > > + resource_list_for_each_entry(window, &bridge->windows) > > > > + if (window->res->flags & IORESOURCE_BUS) { > > > > + found = true; > > > > + break; > > > > + } > > > > + > > > > + ret = pci_register_host_bridge(bridge); > > > > + if (ret < 0) > > > > + return ret; > > > > + > > > > + b = bridge->bus; > > > > + bus = bridge->busnr; > > > > + > > > > + if (!found) { > > > > + dev_info(&b->dev, > > > > + "No busn resource found for root bus, will use [bus %02x-ff]\n", > > > > + bus); > > > > + pci_bus_insert_busn_res(b, bus, 255); > > > > + } > > > > + > > > > + max = pci_scan_child_bus(b); > > > > + > > > > + if (!found) > > > > + pci_bus_update_busn_res_end(b, max); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > > > We probably want an EXPORT_SYMBOL() here as well. > > > > Yep, sure. > > > > Thanks for having a look ! > > > > Lorenzo > > > > _______________________________________________ > > linux-arm-kernel mailing list > > linux-arm-kernel@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 7e4ffc4..c7a7f72 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -2412,6 +2412,44 @@ void pci_bus_release_busn_res(struct pci_bus *b) res, ret ? "can not be" : "is"); } +int pci_scan_root_bus_bridge(struct pci_host_bridge *bridge) +{ + struct resource_entry *window; + bool found = false; + struct pci_bus *b; + int max, bus, ret; + + if (!bridge) + return -EINVAL; + + resource_list_for_each_entry(window, &bridge->windows) + if (window->res->flags & IORESOURCE_BUS) { + found = true; + break; + } + + ret = pci_register_host_bridge(bridge); + if (ret < 0) + return ret; + + b = bridge->bus; + bus = bridge->busnr; + + if (!found) { + dev_info(&b->dev, + "No busn resource found for root bus, will use [bus %02x-ff]\n", + bus); + pci_bus_insert_busn_res(b, bus, 255); + } + + max = pci_scan_child_bus(b); + + if (!found) + pci_bus_update_busn_res_end(b, max); + + return 0; +} + struct pci_bus *pci_scan_root_bus_msi(struct device *parent, int bus, struct pci_ops *ops, void *sysdata, struct list_head *resources, struct msi_controller *msi) diff --git a/include/linux/pci.h b/include/linux/pci.h index 99878a9..757779a 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -847,6 +847,7 @@ struct pci_bus *pci_scan_root_bus_msi(struct device *parent, int bus, struct pci_bus *pci_scan_root_bus(struct device *parent, int bus, struct pci_ops *ops, void *sysdata, struct list_head *resources); +int pci_scan_root_bus_bridge(struct pci_host_bridge *bridge); struct pci_bus *pci_add_new_bus(struct pci_bus *parent, struct pci_dev *dev, int busnr); void pcie_update_link_speed(struct pci_bus *bus, u16 link_status);
Current pci_scan_root_bus() interface is made up of two main code paths: - pci_create_root_bus() - pci_scan_child_bus() pci_create_root_bus() is a wrapper function that allows to create a struct pci_host_bridge structure, initialize it with the passed parameters and register it with the kernel. As the struct pci_host_bridge require additional struct members, pci_create_root_bus() parameters list has grown in time, making it unwieldy to add further parameters to it in case the struct pci_host_bridge gains more members fields to augment its functionality. Since PCI core code provides functions to allocate struct pci_host_bridge, instead of forcing the pci_create_root_bus() interface to add new parameters to cater for new struct pci_host_bridge functionality, it is more suitable to add an interface in PCI core code to scan a PCI bus straight from a struct pci_host_bridge created and customized by each specific PCI host controller driver. Add a pci_scan_root_bus_bridge() function to allow PCI host controller drivers to create and initialize struct pci_host_bridge and scan the resulting bus. Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> Cc: Arnd Bergmann <arnd@arndb.de> Cc: Bjorn Helgaas <bhelgaas@google.com> --- drivers/pci/probe.c | 38 ++++++++++++++++++++++++++++++++++++++ include/linux/pci.h | 1 + 2 files changed, 39 insertions(+)