| Message ID | 20190723092711.11786-4-jonnyc@amazon.com |
|---|---|
| State | Superseded |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | Amazon's Annapurna Labs DT-based PCIe host controller driver | expand |
On Tue, Jul 23, 2019 at 12:27:11PM +0300, Jonathan Chocron wrote: > This basically aligns the usage of PCI_PROBE_ONLY and > PCI_REASSIGN_ALL_BUS in dw_pcie_host_init() with the logic in > pci_host_common_probe(). > > Now it will be possible to control via the devicetree whether to just > probe the PCI bus (in cases where FW already configured it) or to fully > configure it. > > Signed-off-by: Jonathan Chocron <jonnyc@amazon.com> > --- > .../pci/controller/dwc/pcie-designware-host.c | 23 +++++++++++++++---- > 1 file changed, 19 insertions(+), 4 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c > index d2ca748e4c85..0a294d8aa21a 100644 > --- a/drivers/pci/controller/dwc/pcie-designware-host.c > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c > @@ -342,6 +342,8 @@ int dw_pcie_host_init(struct pcie_port *pp) > if (!bridge) > return -ENOMEM; > > + of_pci_check_probe_only(); > + > ret = devm_of_pci_get_host_bridge_resources(dev, 0, 0xff, > &bridge->windows, &pp->io_base); > if (ret) > @@ -474,6 +476,10 @@ int dw_pcie_host_init(struct pcie_port *pp) > > pp->root_bus_nr = pp->busn->start; > > + /* Do not reassign bus nums if probe only */ > + if (!pci_has_flag(PCI_PROBE_ONLY)) > + pci_add_flags(PCI_REASSIGN_ALL_BUS); This changes the default for bus reassignment on all DWC host (that are !PCI_PROBE_ONLY), we should drop this line, it can trigger regressions. If we still want to merge it as a separate change we must test it on all DWC host bridges to make sure it does not trigger any issues with current set-ups, that's not going to be easy though. Lorenzo > + > bridge->dev.parent = dev; > bridge->sysdata = pp; > bridge->busnr = pp->root_bus_nr; > @@ -490,11 +496,20 @@ int dw_pcie_host_init(struct pcie_port *pp) > if (pp->ops->scan_bus) > pp->ops->scan_bus(pp); > > - pci_bus_size_bridges(pp->root_bus); > - pci_bus_assign_resources(pp->root_bus); > + /* > + * We insert PCI resources into the iomem_resource and > + * ioport_resource trees in either pci_bus_claim_resources() > + * or pci_bus_assign_resources(). > + */ > + if (pci_has_flag(PCI_PROBE_ONLY)) { > + pci_bus_claim_resources(pp->root_bus); > + } else { > + pci_bus_size_bridges(pp->root_bus); > + pci_bus_assign_resources(pp->root_bus); > > - list_for_each_entry(child, &pp->root_bus->children, node) > - pcie_bus_configure_settings(child); > + list_for_each_entry(child, &pp->root_bus->children, node) > + pcie_bus_configure_settings(child); > + } > > pci_bus_add_devices(pp->root_bus); > return 0; > -- > 2.17.1 >
On Wed, 2019-08-07 at 17:36 +0100, Lorenzo Pieralisi wrote: > On Tue, Jul 23, 2019 at 12:27:11PM +0300, Jonathan Chocron wrote: > > This basically aligns the usage of PCI_PROBE_ONLY and > > PCI_REASSIGN_ALL_BUS in dw_pcie_host_init() with the logic in > > pci_host_common_probe(). > > > > Now it will be possible to control via the devicetree whether to > > just > > probe the PCI bus (in cases where FW already configured it) or to > > fully > > configure it. > > > > Signed-off-by: Jonathan Chocron <jonnyc@amazon.com> > > --- > > .../pci/controller/dwc/pcie-designware-host.c | 23 > > +++++++++++++++---- > > 1 file changed, 19 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c > > b/drivers/pci/controller/dwc/pcie-designware-host.c > > index d2ca748e4c85..0a294d8aa21a 100644 > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c > > @@ -342,6 +342,8 @@ int dw_pcie_host_init(struct pcie_port *pp) > > if (!bridge) > > return -ENOMEM; > > > > + of_pci_check_probe_only(); > > + > > ret = devm_of_pci_get_host_bridge_resources(dev, 0, 0xff, > > &bridge->windows, &pp- > > >io_base); > > if (ret) > > @@ -474,6 +476,10 @@ int dw_pcie_host_init(struct pcie_port *pp) > > > > pp->root_bus_nr = pp->busn->start; > > > > + /* Do not reassign bus nums if probe only */ > > + if (!pci_has_flag(PCI_PROBE_ONLY)) > > + pci_add_flags(PCI_REASSIGN_ALL_BUS); > > This changes the default for bus reassignment on all DWC host (that > are > !PCI_PROBE_ONLY), we should drop this line, it can trigger > regressions. > Will be dropped as part of v4. There might also be a behavioral difference below where I added the if (pci_has_flag(PCI_PROBE_ONLY)). Is that still ok? As I pointed out in the cover letter, since PCI_PROBE_ONLY is a system wide flag, does it make sense to call of_pci_check_probe_only() here, in the context of a specific driver (including the existing invocation in pci_host_common_probe()), as opposed to a platform/arch context? > If we still want to merge it as a separate change we must test it on > all > DWC host bridges to make sure it does not trigger any issues with > current set-ups, that's not going to be easy though. > Just out of curiosity, how are such exhaustive tests achieved when a patch requires them? > Lorenzo > > > + > > bridge->dev.parent = dev; > > bridge->sysdata = pp; > > bridge->busnr = pp->root_bus_nr; > > @@ -490,11 +496,20 @@ int dw_pcie_host_init(struct pcie_port *pp) > > if (pp->ops->scan_bus) > > pp->ops->scan_bus(pp); > > > > - pci_bus_size_bridges(pp->root_bus); > > - pci_bus_assign_resources(pp->root_bus); > > + /* > > + * We insert PCI resources into the iomem_resource and > > + * ioport_resource trees in either pci_bus_claim_resources() > > + * or pci_bus_assign_resources(). > > + */ > > + if (pci_has_flag(PCI_PROBE_ONLY)) { > > + pci_bus_claim_resources(pp->root_bus); > > + } else { > > + pci_bus_size_bridges(pp->root_bus); > > + pci_bus_assign_resources(pp->root_bus); > > > > - list_for_each_entry(child, &pp->root_bus->children, node) > > - pcie_bus_configure_settings(child); > > + list_for_each_entry(child, &pp->root_bus->children, > > node) > > + pcie_bus_configure_settings(child); > > + } > > > > pci_bus_add_devices(pp->root_bus); > > return 0; > > -- > > 2.17.1 > >
Side note, run: git log --oneline on drivers/pci/controller/dwc existing files and make sure commit subjects are in line with those. Eg PCI: dw: should be PCI: dwc: On Thu, Aug 08, 2019 at 09:30:05AM +0000, Chocron, Jonathan wrote: > On Wed, 2019-08-07 at 17:36 +0100, Lorenzo Pieralisi wrote: > > On Tue, Jul 23, 2019 at 12:27:11PM +0300, Jonathan Chocron wrote: > > > This basically aligns the usage of PCI_PROBE_ONLY and > > > PCI_REASSIGN_ALL_BUS in dw_pcie_host_init() with the logic in > > > pci_host_common_probe(). > > > > > > Now it will be possible to control via the devicetree whether to > > > just > > > probe the PCI bus (in cases where FW already configured it) or to > > > fully > > > configure it. > > > > > > Signed-off-by: Jonathan Chocron <jonnyc@amazon.com> > > > --- > > > .../pci/controller/dwc/pcie-designware-host.c | 23 > > > +++++++++++++++---- > > > 1 file changed, 19 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c > > > b/drivers/pci/controller/dwc/pcie-designware-host.c > > > index d2ca748e4c85..0a294d8aa21a 100644 > > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c > > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c > > > @@ -342,6 +342,8 @@ int dw_pcie_host_init(struct pcie_port *pp) > > > if (!bridge) > > > return -ENOMEM; > > > > > > + of_pci_check_probe_only(); > > > + > > > ret = devm_of_pci_get_host_bridge_resources(dev, 0, 0xff, > > > &bridge->windows, &pp- > > > >io_base); > > > if (ret) > > > @@ -474,6 +476,10 @@ int dw_pcie_host_init(struct pcie_port *pp) > > > > > > pp->root_bus_nr = pp->busn->start; > > > > > > + /* Do not reassign bus nums if probe only */ > > > + if (!pci_has_flag(PCI_PROBE_ONLY)) > > > + pci_add_flags(PCI_REASSIGN_ALL_BUS); > > > > This changes the default for bus reassignment on all DWC host (that > > are > > !PCI_PROBE_ONLY), we should drop this line, it can trigger > > regressions. > > > Will be dropped as part of v4. There might also be a behavioral > difference below where I added the if (pci_has_flag(PCI_PROBE_ONLY)). > Is that still ok? That's true but I doubt any DWC host has a DT firmware with "linux,pci-probe-only" in it. It is trial and error I am afraid, please make sure all DWC maintainers are copied in. > As I pointed out in the cover letter, since PCI_PROBE_ONLY is a system > wide flag, does it make sense to call of_pci_check_probe_only() here, > in the context of a specific driver (including the existing invocation > in pci_host_common_probe()), as opposed to a platform/arch context? It is an ongoing discussion to define how we should handle PCI_PROBE_ONLY. Adding this code into DWC I do not think it would hurt but if we can postpone it for the next (v5.5) merge window after we debate this at LPC within the PCI microconference it would be great. Please sync with Benjamin as a first step, I trust he would ask you to do the right thing. > > If we still want to merge it as a separate change we must test it on > > all > > DWC host bridges to make sure it does not trigger any issues with > > current set-ups, that's not going to be easy though. > > > Just out of curiosity, how are such exhaustive tests achieved when a > patch requires them? CC DWC host bridge maintainers and ask them to test it. I do not have the HW (and FW) required, I am sorry, that's the only option I can give you. -next coverage would help too but to a minor extent. Thanks, Lorenzo > > > Lorenzo > > > > > + > > > bridge->dev.parent = dev; > > > bridge->sysdata = pp; > > > bridge->busnr = pp->root_bus_nr; > > > @@ -490,11 +496,20 @@ int dw_pcie_host_init(struct pcie_port *pp) > > > if (pp->ops->scan_bus) > > > pp->ops->scan_bus(pp); > > > > > > - pci_bus_size_bridges(pp->root_bus); > > > - pci_bus_assign_resources(pp->root_bus); > > > + /* > > > + * We insert PCI resources into the iomem_resource and > > > + * ioport_resource trees in either pci_bus_claim_resources() > > > + * or pci_bus_assign_resources(). > > > + */ > > > + if (pci_has_flag(PCI_PROBE_ONLY)) { > > > + pci_bus_claim_resources(pp->root_bus); > > > + } else { > > > + pci_bus_size_bridges(pp->root_bus); > > > + pci_bus_assign_resources(pp->root_bus); > > > > > > - list_for_each_entry(child, &pp->root_bus->children, node) > > > - pcie_bus_configure_settings(child); > > > + list_for_each_entry(child, &pp->root_bus->children, > > > node) > > > + pcie_bus_configure_settings(child); > > > + } > > > > > > pci_bus_add_devices(pp->root_bus); > > > return 0; > > > -- > > > 2.17.1 > > >
On Thu, 2019-08-08 at 11:58 +0100, Lorenzo Pieralisi wrote: > Side note, run: > > git log --oneline on drivers/pci/controller/dwc existing files and > make sure commit subjects are in line with those. > > Eg PCI: dw: should be PCI: dwc: > Done. > On Thu, Aug 08, 2019 at 09:30:05AM +0000, Chocron, Jonathan wrote: > > On Wed, 2019-08-07 at 17:36 +0100, Lorenzo Pieralisi wrote: > > > On Tue, Jul 23, 2019 at 12:27:11PM +0300, Jonathan Chocron wrote: > > > > This basically aligns the usage of PCI_PROBE_ONLY and > > > > PCI_REASSIGN_ALL_BUS in dw_pcie_host_init() with the logic in > > > > pci_host_common_probe(). > > > > > > > > Now it will be possible to control via the devicetree whether > > > > to > > > > just > > > > probe the PCI bus (in cases where FW already configured it) or > > > > to > > > > fully > > > > configure it. > > > > > > > > Signed-off-by: Jonathan Chocron <jonnyc@amazon.com> > > > > --- > > > > .../pci/controller/dwc/pcie-designware-host.c | 23 > > > > +++++++++++++++---- > > > > 1 file changed, 19 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c > > > > b/drivers/pci/controller/dwc/pcie-designware-host.c > > > > index d2ca748e4c85..0a294d8aa21a 100644 > > > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c > > > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c > > > > @@ -342,6 +342,8 @@ int dw_pcie_host_init(struct pcie_port *pp) > > > > if (!bridge) > > > > return -ENOMEM; > > > > > > > > + of_pci_check_probe_only(); > > > > + > > > > ret = devm_of_pci_get_host_bridge_resources(dev, 0, > > > > 0xff, > > > > &bridge->windows, &pp- > > > > > io_base); > > > > > > > > if (ret) > > > > @@ -474,6 +476,10 @@ int dw_pcie_host_init(struct pcie_port > > > > *pp) > > > > > > > > pp->root_bus_nr = pp->busn->start; > > > > > > > > + /* Do not reassign bus nums if probe only */ > > > > + if (!pci_has_flag(PCI_PROBE_ONLY)) > > > > + pci_add_flags(PCI_REASSIGN_ALL_BUS); > > > > > > This changes the default for bus reassignment on all DWC host > > > (that > > > are > > > !PCI_PROBE_ONLY), we should drop this line, it can trigger > > > regressions. > > > > > > > Will be dropped as part of v4. There might also be a behavioral > > difference below where I added the if > > (pci_has_flag(PCI_PROBE_ONLY)). > > Is that still ok? > > That's true but I doubt any DWC host has a DT firmware with > "linux,pci-probe-only" in it. > > It is trial and error I am afraid, please make sure all DWC > maintainers are copied in. > > > As I pointed out in the cover letter, since PCI_PROBE_ONLY is a > > system > > wide flag, does it make sense to call of_pci_check_probe_only() > > here, > > in the context of a specific driver (including the existing > > invocation > > in pci_host_common_probe()), as opposed to a platform/arch context? > > It is an ongoing discussion to define how we should handle > PCI_PROBE_ONLY. Adding this code into DWC I do not think it > would hurt but if we can postpone it for the next (v5.5) merge > window after we debate this at LPC within the PCI microconference > it would be great. > The preceding patches are more urgent, so I'm fine with postponing this patch to the next merge window (I'll drop it from v4). > Please sync with Benjamin as a first step, I trust he would ask > you to do the right thing. > Of course, I'll sync with him. But again, let's not have this patch delay the others please. > > > If we still want to merge it as a separate change we must test it > > > on > > > all > > > DWC host bridges to make sure it does not trigger any issues with > > > current set-ups, that's not going to be easy though. > > > > > > > Just out of curiosity, how are such exhaustive tests achieved when > > a > > patch requires them? > > CC DWC host bridge maintainers and ask them to test it. I do not have > the HW (and FW) required, I am sorry, that's the only option I can > give > you. -next coverage would help too but to a minor extent. > Thanks for the info! > Thanks, > Lorenzo > > > > > > Lorenzo > > > > > > > + > > > > bridge->dev.parent = dev; > > > > bridge->sysdata = pp; > > > > bridge->busnr = pp->root_bus_nr; > > > > @@ -490,11 +496,20 @@ int dw_pcie_host_init(struct pcie_port > > > > *pp) > > > > if (pp->ops->scan_bus) > > > > pp->ops->scan_bus(pp); > > > > > > > > - pci_bus_size_bridges(pp->root_bus); > > > > - pci_bus_assign_resources(pp->root_bus); > > > > + /* > > > > + * We insert PCI resources into the iomem_resource and > > > > + * ioport_resource trees in either > > > > pci_bus_claim_resources() > > > > + * or pci_bus_assign_resources(). > > > > + */ > > > > + if (pci_has_flag(PCI_PROBE_ONLY)) { > > > > + pci_bus_claim_resources(pp->root_bus); > > > > + } else { > > > > + pci_bus_size_bridges(pp->root_bus); > > > > + pci_bus_assign_resources(pp->root_bus); > > > > > > > > - list_for_each_entry(child, &pp->root_bus->children, > > > > node) > > > > - pcie_bus_configure_settings(child); > > > > + list_for_each_entry(child, &pp->root_bus- > > > > >children, > > > > node) > > > > + pcie_bus_configure_settings(child); > > > > + } > > > > > > > > pci_bus_add_devices(pp->root_bus); > > > > return 0; > > > > -- > > > > 2.17.1 > > > >
diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c index d2ca748e4c85..0a294d8aa21a 100644 --- a/drivers/pci/controller/dwc/pcie-designware-host.c +++ b/drivers/pci/controller/dwc/pcie-designware-host.c @@ -342,6 +342,8 @@ int dw_pcie_host_init(struct pcie_port *pp) if (!bridge) return -ENOMEM; + of_pci_check_probe_only(); + ret = devm_of_pci_get_host_bridge_resources(dev, 0, 0xff, &bridge->windows, &pp->io_base); if (ret) @@ -474,6 +476,10 @@ int dw_pcie_host_init(struct pcie_port *pp) pp->root_bus_nr = pp->busn->start; + /* Do not reassign bus nums if probe only */ + if (!pci_has_flag(PCI_PROBE_ONLY)) + pci_add_flags(PCI_REASSIGN_ALL_BUS); + bridge->dev.parent = dev; bridge->sysdata = pp; bridge->busnr = pp->root_bus_nr; @@ -490,11 +496,20 @@ int dw_pcie_host_init(struct pcie_port *pp) if (pp->ops->scan_bus) pp->ops->scan_bus(pp); - pci_bus_size_bridges(pp->root_bus); - pci_bus_assign_resources(pp->root_bus); + /* + * We insert PCI resources into the iomem_resource and + * ioport_resource trees in either pci_bus_claim_resources() + * or pci_bus_assign_resources(). + */ + if (pci_has_flag(PCI_PROBE_ONLY)) { + pci_bus_claim_resources(pp->root_bus); + } else { + pci_bus_size_bridges(pp->root_bus); + pci_bus_assign_resources(pp->root_bus); - list_for_each_entry(child, &pp->root_bus->children, node) - pcie_bus_configure_settings(child); + list_for_each_entry(child, &pp->root_bus->children, node) + pcie_bus_configure_settings(child); + } pci_bus_add_devices(pp->root_bus); return 0;
This basically aligns the usage of PCI_PROBE_ONLY and PCI_REASSIGN_ALL_BUS in dw_pcie_host_init() with the logic in pci_host_common_probe(). Now it will be possible to control via the devicetree whether to just probe the PCI bus (in cases where FW already configured it) or to fully configure it. Signed-off-by: Jonathan Chocron <jonnyc@amazon.com> --- .../pci/controller/dwc/pcie-designware-host.c | 23 +++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-)