* [PATCH V2 0/2] Add support to preserve boot config in the DT flow @ 2024-01-10 3:07 ` Vidya Sagar 0 siblings, 0 replies; 58+ messages in thread From: Vidya Sagar @ 2024-01-10 3:07 UTC (permalink / raw) To: lpieralisi, kw, robh, bhelgaas, krzysztof.kozlowski+dt, conor+dt, will, frowand.list Cc: linux-pci, devicetree, linux-kernel, linux-arm-kernel, treding, jonathanh, kthota, mmaddireddy, vidyas, sagar.tv Add support to preserve the boot configuration of the PCIe bridges per host bridge basis based on the presence of the DT flag "preserve-boot-config" in the respective host bridge node. The existing "linux,pci-probe-only" works at a system level and can't be used at a single host bridge granularity. Also, the support for preserving the boot configuration per host bridge basis is already present for the ACPI based boot flow and this patch series extends that support for the DT based boot flow. V2: * Addressed issues reported by kernel test robot <lkp@intel.com> Vidya Sagar (2): dt-bindings: Add PCIe "preserve-boot-config" property PCI: Add support for "preserve-boot-config" property Documentation/devicetree/bindings/pci/pci.txt | 4 ++++ drivers/pci/controller/pci-host-common.c | 5 ++++- drivers/pci/of.c | 18 ++++++++++++++++++ drivers/pci/probe.c | 2 +- include/linux/of_pci.h | 6 ++++++ 5 files changed, 33 insertions(+), 2 deletions(-) -- 2.25.1 ^ permalink raw reply [flat|nested] 58+ messages in thread
* [PATCH V2 0/2] Add support to preserve boot config in the DT flow @ 2024-01-10 3:07 ` Vidya Sagar 0 siblings, 0 replies; 58+ messages in thread From: Vidya Sagar @ 2024-01-10 3:07 UTC (permalink / raw) To: lpieralisi, kw, robh, bhelgaas, krzysztof.kozlowski+dt, conor+dt, will, frowand.list Cc: linux-pci, devicetree, linux-kernel, linux-arm-kernel, treding, jonathanh, kthota, mmaddireddy, vidyas, sagar.tv Add support to preserve the boot configuration of the PCIe bridges per host bridge basis based on the presence of the DT flag "preserve-boot-config" in the respective host bridge node. The existing "linux,pci-probe-only" works at a system level and can't be used at a single host bridge granularity. Also, the support for preserving the boot configuration per host bridge basis is already present for the ACPI based boot flow and this patch series extends that support for the DT based boot flow. V2: * Addressed issues reported by kernel test robot <lkp@intel.com> Vidya Sagar (2): dt-bindings: Add PCIe "preserve-boot-config" property PCI: Add support for "preserve-boot-config" property Documentation/devicetree/bindings/pci/pci.txt | 4 ++++ drivers/pci/controller/pci-host-common.c | 5 ++++- drivers/pci/of.c | 18 ++++++++++++++++++ drivers/pci/probe.c | 2 +- include/linux/of_pci.h | 6 ++++++ 5 files changed, 33 insertions(+), 2 deletions(-) -- 2.25.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 58+ messages in thread
* [PATCH V2 1/2] dt-bindings: Add PCIe "preserve-boot-config" property 2024-01-10 3:07 ` Vidya Sagar @ 2024-01-10 3:07 ` Vidya Sagar -1 siblings, 0 replies; 58+ messages in thread From: Vidya Sagar @ 2024-01-10 3:07 UTC (permalink / raw) To: lpieralisi, kw, robh, bhelgaas, krzysztof.kozlowski+dt, conor+dt, will, frowand.list Cc: linux-pci, devicetree, linux-kernel, linux-arm-kernel, treding, jonathanh, kthota, mmaddireddy, vidyas, sagar.tv The existing "linux,pci-probe-only" property applies at a system level and it is not possible to selectively convey individual host bridge's requirement w.r.t preserving the boot configuration done by the platform firmware to the kernel. "preserve-boot-config" addresses that concern and can be used to preserve the boot configuration for host bridges selectivey. Signed-off-by: Vidya Sagar <vidyas@nvidia.com> --- V2: * None Documentation/devicetree/bindings/pci/pci.txt | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Documentation/devicetree/bindings/pci/pci.txt b/Documentation/devicetree/bindings/pci/pci.txt index 6a8f2874a24d..0a5ff998cbe8 100644 --- a/Documentation/devicetree/bindings/pci/pci.txt +++ b/Documentation/devicetree/bindings/pci/pci.txt @@ -32,6 +32,10 @@ driver implementation may support the following properties: root port to downstream device and host bridge drivers can do programming which depends on CLKREQ signal existence. For example, programming root port not to advertise ASPM L1 Sub-States support if there is no CLKREQ signal. +- preserve-boot-config: + If present this property specifies that this host bridge is already + configured by the platform firmware and the OS doesn't need to reconfigure + it again. PCI-PCI Bridge properties ------------------------- -- 2.25.1 ^ permalink raw reply related [flat|nested] 58+ messages in thread
* [PATCH V2 1/2] dt-bindings: Add PCIe "preserve-boot-config" property @ 2024-01-10 3:07 ` Vidya Sagar 0 siblings, 0 replies; 58+ messages in thread From: Vidya Sagar @ 2024-01-10 3:07 UTC (permalink / raw) To: lpieralisi, kw, robh, bhelgaas, krzysztof.kozlowski+dt, conor+dt, will, frowand.list Cc: linux-pci, devicetree, linux-kernel, linux-arm-kernel, treding, jonathanh, kthota, mmaddireddy, vidyas, sagar.tv The existing "linux,pci-probe-only" property applies at a system level and it is not possible to selectively convey individual host bridge's requirement w.r.t preserving the boot configuration done by the platform firmware to the kernel. "preserve-boot-config" addresses that concern and can be used to preserve the boot configuration for host bridges selectivey. Signed-off-by: Vidya Sagar <vidyas@nvidia.com> --- V2: * None Documentation/devicetree/bindings/pci/pci.txt | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Documentation/devicetree/bindings/pci/pci.txt b/Documentation/devicetree/bindings/pci/pci.txt index 6a8f2874a24d..0a5ff998cbe8 100644 --- a/Documentation/devicetree/bindings/pci/pci.txt +++ b/Documentation/devicetree/bindings/pci/pci.txt @@ -32,6 +32,10 @@ driver implementation may support the following properties: root port to downstream device and host bridge drivers can do programming which depends on CLKREQ signal existence. For example, programming root port not to advertise ASPM L1 Sub-States support if there is no CLKREQ signal. +- preserve-boot-config: + If present this property specifies that this host bridge is already + configured by the platform firmware and the OS doesn't need to reconfigure + it again. PCI-PCI Bridge properties ------------------------- -- 2.25.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 58+ messages in thread
* Re: [PATCH V2 1/2] dt-bindings: Add PCIe "preserve-boot-config" property 2024-01-10 3:07 ` Vidya Sagar @ 2024-01-12 14:33 ` Rob Herring -1 siblings, 0 replies; 58+ messages in thread From: Rob Herring @ 2024-01-12 14:33 UTC (permalink / raw) To: Vidya Sagar Cc: lpieralisi, kw, bhelgaas, krzysztof.kozlowski+dt, conor+dt, will, frowand.list, linux-pci, devicetree, linux-kernel, linux-arm-kernel, treding, jonathanh, kthota, mmaddireddy, sagar.tv On Wed, Jan 10, 2024 at 08:37:24AM +0530, Vidya Sagar wrote: > The existing "linux,pci-probe-only" property applies at a system level > and it is not possible to selectively convey individual host bridge's > requirement w.r.t preserving the boot configuration done by the platform > firmware to the kernel. "preserve-boot-config" addresses that concern > and can be used to preserve the boot configuration for host bridges > selectivey. > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > --- > V2: > * None > > Documentation/devicetree/bindings/pci/pci.txt | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/Documentation/devicetree/bindings/pci/pci.txt b/Documentation/devicetree/bindings/pci/pci.txt > index 6a8f2874a24d..0a5ff998cbe8 100644 > --- a/Documentation/devicetree/bindings/pci/pci.txt > +++ b/Documentation/devicetree/bindings/pci/pci.txt > @@ -32,6 +32,10 @@ driver implementation may support the following properties: > root port to downstream device and host bridge drivers can do programming > which depends on CLKREQ signal existence. For example, programming root port > not to advertise ASPM L1 Sub-States support if there is no CLKREQ signal. > +- preserve-boot-config: > + If present this property specifies that this host bridge is already > + configured by the platform firmware and the OS doesn't need to reconfigure > + it again. Anything new must be a schema. Don't create something new to workaround the problem. Extend the existing support. For this, make "linux,pci-probe-only" allowed in host bridge nodes. Yeah, there's the issue that this property sets a global flag, but I'd imagine the PCI maintainers would agree that some flags should/could be per bus. Rob _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH V2 1/2] dt-bindings: Add PCIe "preserve-boot-config" property @ 2024-01-12 14:33 ` Rob Herring 0 siblings, 0 replies; 58+ messages in thread From: Rob Herring @ 2024-01-12 14:33 UTC (permalink / raw) To: Vidya Sagar Cc: lpieralisi, kw, bhelgaas, krzysztof.kozlowski+dt, conor+dt, will, frowand.list, linux-pci, devicetree, linux-kernel, linux-arm-kernel, treding, jonathanh, kthota, mmaddireddy, sagar.tv On Wed, Jan 10, 2024 at 08:37:24AM +0530, Vidya Sagar wrote: > The existing "linux,pci-probe-only" property applies at a system level > and it is not possible to selectively convey individual host bridge's > requirement w.r.t preserving the boot configuration done by the platform > firmware to the kernel. "preserve-boot-config" addresses that concern > and can be used to preserve the boot configuration for host bridges > selectivey. > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > --- > V2: > * None > > Documentation/devicetree/bindings/pci/pci.txt | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/Documentation/devicetree/bindings/pci/pci.txt b/Documentation/devicetree/bindings/pci/pci.txt > index 6a8f2874a24d..0a5ff998cbe8 100644 > --- a/Documentation/devicetree/bindings/pci/pci.txt > +++ b/Documentation/devicetree/bindings/pci/pci.txt > @@ -32,6 +32,10 @@ driver implementation may support the following properties: > root port to downstream device and host bridge drivers can do programming > which depends on CLKREQ signal existence. For example, programming root port > not to advertise ASPM L1 Sub-States support if there is no CLKREQ signal. > +- preserve-boot-config: > + If present this property specifies that this host bridge is already > + configured by the platform firmware and the OS doesn't need to reconfigure > + it again. Anything new must be a schema. Don't create something new to workaround the problem. Extend the existing support. For this, make "linux,pci-probe-only" allowed in host bridge nodes. Yeah, there's the issue that this property sets a global flag, but I'd imagine the PCI maintainers would agree that some flags should/could be per bus. Rob ^ permalink raw reply [flat|nested] 58+ messages in thread
* [PATCH V2 2/2] PCI: Add support for "preserve-boot-config" property 2024-01-10 3:07 ` Vidya Sagar @ 2024-01-10 3:07 ` Vidya Sagar -1 siblings, 0 replies; 58+ messages in thread From: Vidya Sagar @ 2024-01-10 3:07 UTC (permalink / raw) To: lpieralisi, kw, robh, bhelgaas, krzysztof.kozlowski+dt, conor+dt, will, frowand.list Cc: linux-pci, devicetree, linux-kernel, linux-arm-kernel, treding, jonathanh, kthota, mmaddireddy, vidyas, sagar.tv, kernel test robot Add support for "preserve-boot-config" property that can be used to selectively (i.e. per host bridge) instruct the kernel to preserve the boot time configuration done by the platform firmware. Reported-by: kernel test robot <lkp@intel.com> Signed-off-by: Vidya Sagar <vidyas@nvidia.com> --- V2: * Addressed issues reported by kernel test robot <lkp@intel.com> drivers/pci/controller/pci-host-common.c | 5 ++++- drivers/pci/of.c | 18 ++++++++++++++++++ drivers/pci/probe.c | 2 +- include/linux/of_pci.h | 6 ++++++ 4 files changed, 29 insertions(+), 2 deletions(-) diff --git a/drivers/pci/controller/pci-host-common.c b/drivers/pci/controller/pci-host-common.c index 6be3266cd7b5..d3475dc9ec44 100644 --- a/drivers/pci/controller/pci-host-common.c +++ b/drivers/pci/controller/pci-host-common.c @@ -68,13 +68,16 @@ int pci_host_common_probe(struct platform_device *pdev) of_pci_check_probe_only(); + bridge->preserve_config = + of_pci_check_preserve_boot_config(dev->of_node); + /* Parse and map our Configuration Space windows */ cfg = gen_pci_init(dev, bridge, ops); if (IS_ERR(cfg)) return PTR_ERR(cfg); /* Do not reassign resources if probe only */ - if (!pci_has_flag(PCI_PROBE_ONLY)) + if (!(pci_has_flag(PCI_PROBE_ONLY) || bridge->preserve_config)) pci_add_flags(PCI_REASSIGN_ALL_BUS); bridge->sysdata = cfg; diff --git a/drivers/pci/of.c b/drivers/pci/of.c index 51e3dd0ea5ab..ed3c0dd9804e 100644 --- a/drivers/pci/of.c +++ b/drivers/pci/of.c @@ -258,6 +258,24 @@ void of_pci_check_probe_only(void) } EXPORT_SYMBOL_GPL(of_pci_check_probe_only); +/** + * of_pci_check_preserve_boot_config - Return true if the boot configuration + * needs to be preserved + * @node: Device tree node with the domain information. + * + * This function looks for a property called "preserve-boot-config" for a given + * PCIe controller's node and returns true if found. Having this property + * for a PCIe controller ensures that the kernel doesn't re-enumerate and + * reconfigure the BAR resources that are already done by the platform firmware. + * + * Return: true if the property exists false otherwise. + */ +bool of_pci_check_preserve_boot_config(struct device_node *node) +{ + return of_property_read_bool(node, "preserve-boot-config"); +} +EXPORT_SYMBOL_GPL(of_pci_check_preserve_boot_config); + /** * devm_of_pci_get_host_bridge_resources() - Resource-managed parsing of PCI * host bridge resources from DT diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 795534589b98..79d0ac34f567 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -3085,7 +3085,7 @@ int pci_host_probe(struct pci_host_bridge *bridge) * ioport_resource trees in either pci_bus_claim_resources() * or pci_bus_assign_resources(). */ - if (pci_has_flag(PCI_PROBE_ONLY)) { + if (pci_has_flag(PCI_PROBE_ONLY) || bridge->preserve_config) { pci_bus_claim_resources(bus); } else { pci_bus_size_bridges(bus); diff --git a/include/linux/of_pci.h b/include/linux/of_pci.h index 29658c0ee71f..ba5532005125 100644 --- a/include/linux/of_pci.h +++ b/include/linux/of_pci.h @@ -13,6 +13,7 @@ struct device_node *of_pci_find_child_device(struct device_node *parent, unsigned int devfn); int of_pci_get_devfn(struct device_node *np); void of_pci_check_probe_only(void); +bool of_pci_check_preserve_boot_config(struct device_node *node); #else static inline struct device_node *of_pci_find_child_device(struct device_node *parent, unsigned int devfn) @@ -26,6 +27,11 @@ static inline int of_pci_get_devfn(struct device_node *np) } static inline void of_pci_check_probe_only(void) { } + +static inline bool of_pci_check_preserve_boot_config(struct device_node *node) +{ + return false; +} #endif #if IS_ENABLED(CONFIG_OF_IRQ) -- 2.25.1 ^ permalink raw reply related [flat|nested] 58+ messages in thread
* [PATCH V2 2/2] PCI: Add support for "preserve-boot-config" property @ 2024-01-10 3:07 ` Vidya Sagar 0 siblings, 0 replies; 58+ messages in thread From: Vidya Sagar @ 2024-01-10 3:07 UTC (permalink / raw) To: lpieralisi, kw, robh, bhelgaas, krzysztof.kozlowski+dt, conor+dt, will, frowand.list Cc: linux-pci, devicetree, linux-kernel, linux-arm-kernel, treding, jonathanh, kthota, mmaddireddy, vidyas, sagar.tv, kernel test robot Add support for "preserve-boot-config" property that can be used to selectively (i.e. per host bridge) instruct the kernel to preserve the boot time configuration done by the platform firmware. Reported-by: kernel test robot <lkp@intel.com> Signed-off-by: Vidya Sagar <vidyas@nvidia.com> --- V2: * Addressed issues reported by kernel test robot <lkp@intel.com> drivers/pci/controller/pci-host-common.c | 5 ++++- drivers/pci/of.c | 18 ++++++++++++++++++ drivers/pci/probe.c | 2 +- include/linux/of_pci.h | 6 ++++++ 4 files changed, 29 insertions(+), 2 deletions(-) diff --git a/drivers/pci/controller/pci-host-common.c b/drivers/pci/controller/pci-host-common.c index 6be3266cd7b5..d3475dc9ec44 100644 --- a/drivers/pci/controller/pci-host-common.c +++ b/drivers/pci/controller/pci-host-common.c @@ -68,13 +68,16 @@ int pci_host_common_probe(struct platform_device *pdev) of_pci_check_probe_only(); + bridge->preserve_config = + of_pci_check_preserve_boot_config(dev->of_node); + /* Parse and map our Configuration Space windows */ cfg = gen_pci_init(dev, bridge, ops); if (IS_ERR(cfg)) return PTR_ERR(cfg); /* Do not reassign resources if probe only */ - if (!pci_has_flag(PCI_PROBE_ONLY)) + if (!(pci_has_flag(PCI_PROBE_ONLY) || bridge->preserve_config)) pci_add_flags(PCI_REASSIGN_ALL_BUS); bridge->sysdata = cfg; diff --git a/drivers/pci/of.c b/drivers/pci/of.c index 51e3dd0ea5ab..ed3c0dd9804e 100644 --- a/drivers/pci/of.c +++ b/drivers/pci/of.c @@ -258,6 +258,24 @@ void of_pci_check_probe_only(void) } EXPORT_SYMBOL_GPL(of_pci_check_probe_only); +/** + * of_pci_check_preserve_boot_config - Return true if the boot configuration + * needs to be preserved + * @node: Device tree node with the domain information. + * + * This function looks for a property called "preserve-boot-config" for a given + * PCIe controller's node and returns true if found. Having this property + * for a PCIe controller ensures that the kernel doesn't re-enumerate and + * reconfigure the BAR resources that are already done by the platform firmware. + * + * Return: true if the property exists false otherwise. + */ +bool of_pci_check_preserve_boot_config(struct device_node *node) +{ + return of_property_read_bool(node, "preserve-boot-config"); +} +EXPORT_SYMBOL_GPL(of_pci_check_preserve_boot_config); + /** * devm_of_pci_get_host_bridge_resources() - Resource-managed parsing of PCI * host bridge resources from DT diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 795534589b98..79d0ac34f567 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -3085,7 +3085,7 @@ int pci_host_probe(struct pci_host_bridge *bridge) * ioport_resource trees in either pci_bus_claim_resources() * or pci_bus_assign_resources(). */ - if (pci_has_flag(PCI_PROBE_ONLY)) { + if (pci_has_flag(PCI_PROBE_ONLY) || bridge->preserve_config) { pci_bus_claim_resources(bus); } else { pci_bus_size_bridges(bus); diff --git a/include/linux/of_pci.h b/include/linux/of_pci.h index 29658c0ee71f..ba5532005125 100644 --- a/include/linux/of_pci.h +++ b/include/linux/of_pci.h @@ -13,6 +13,7 @@ struct device_node *of_pci_find_child_device(struct device_node *parent, unsigned int devfn); int of_pci_get_devfn(struct device_node *np); void of_pci_check_probe_only(void); +bool of_pci_check_preserve_boot_config(struct device_node *node); #else static inline struct device_node *of_pci_find_child_device(struct device_node *parent, unsigned int devfn) @@ -26,6 +27,11 @@ static inline int of_pci_get_devfn(struct device_node *np) } static inline void of_pci_check_probe_only(void) { } + +static inline bool of_pci_check_preserve_boot_config(struct device_node *node) +{ + return false; +} #endif #if IS_ENABLED(CONFIG_OF_IRQ) -- 2.25.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 58+ messages in thread
* Re: [PATCH V2 2/2] PCI: Add support for "preserve-boot-config" property 2024-01-10 3:07 ` Vidya Sagar @ 2024-01-12 16:58 ` Bjorn Helgaas -1 siblings, 0 replies; 58+ messages in thread From: Bjorn Helgaas @ 2024-01-12 16:58 UTC (permalink / raw) To: Vidya Sagar Cc: lpieralisi, kw, robh, bhelgaas, krzysztof.kozlowski+dt, conor+dt, will, frowand.list, linux-pci, devicetree, linux-kernel, linux-arm-kernel, treding, jonathanh, kthota, mmaddireddy, sagar.tv, kernel test robot On Wed, Jan 10, 2024 at 08:37:25AM +0530, Vidya Sagar wrote: > Add support for "preserve-boot-config" property that can be used to > selectively (i.e. per host bridge) instruct the kernel to preserve the > boot time configuration done by the platform firmware. > > Reported-by: kernel test robot <lkp@intel.com> > Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > --- > V2: > * Addressed issues reported by kernel test robot <lkp@intel.com> > > drivers/pci/controller/pci-host-common.c | 5 ++++- > drivers/pci/of.c | 18 ++++++++++++++++++ > drivers/pci/probe.c | 2 +- > include/linux/of_pci.h | 6 ++++++ > 4 files changed, 29 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/controller/pci-host-common.c b/drivers/pci/controller/pci-host-common.c > index 6be3266cd7b5..d3475dc9ec44 100644 > --- a/drivers/pci/controller/pci-host-common.c > +++ b/drivers/pci/controller/pci-host-common.c > @@ -68,13 +68,16 @@ int pci_host_common_probe(struct platform_device *pdev) > > of_pci_check_probe_only(); > > + bridge->preserve_config = > + of_pci_check_preserve_boot_config(dev->of_node); Thanks for leveraging the existing "preserve_config" support for the ACPI _DSM. Is pci_host_common_probe() the best place for this? I think there are many DT platform drivers that do not use pci_host_common_probe(), so I wonder if there's a more generic place to put this. I see Rob's concern about adding "preserve-boot-config" vs extending "linux,pci-probe-only" and I don't really have an opinion on that, although I do think the "pci-probe-only" name is not as descriptive as it could be. I guess somebody will argue that "preserve_config" could be more descriptive, too :) Bjorn ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH V2 2/2] PCI: Add support for "preserve-boot-config" property @ 2024-01-12 16:58 ` Bjorn Helgaas 0 siblings, 0 replies; 58+ messages in thread From: Bjorn Helgaas @ 2024-01-12 16:58 UTC (permalink / raw) To: Vidya Sagar Cc: lpieralisi, kw, robh, bhelgaas, krzysztof.kozlowski+dt, conor+dt, will, frowand.list, linux-pci, devicetree, linux-kernel, linux-arm-kernel, treding, jonathanh, kthota, mmaddireddy, sagar.tv, kernel test robot On Wed, Jan 10, 2024 at 08:37:25AM +0530, Vidya Sagar wrote: > Add support for "preserve-boot-config" property that can be used to > selectively (i.e. per host bridge) instruct the kernel to preserve the > boot time configuration done by the platform firmware. > > Reported-by: kernel test robot <lkp@intel.com> > Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > --- > V2: > * Addressed issues reported by kernel test robot <lkp@intel.com> > > drivers/pci/controller/pci-host-common.c | 5 ++++- > drivers/pci/of.c | 18 ++++++++++++++++++ > drivers/pci/probe.c | 2 +- > include/linux/of_pci.h | 6 ++++++ > 4 files changed, 29 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/controller/pci-host-common.c b/drivers/pci/controller/pci-host-common.c > index 6be3266cd7b5..d3475dc9ec44 100644 > --- a/drivers/pci/controller/pci-host-common.c > +++ b/drivers/pci/controller/pci-host-common.c > @@ -68,13 +68,16 @@ int pci_host_common_probe(struct platform_device *pdev) > > of_pci_check_probe_only(); > > + bridge->preserve_config = > + of_pci_check_preserve_boot_config(dev->of_node); Thanks for leveraging the existing "preserve_config" support for the ACPI _DSM. Is pci_host_common_probe() the best place for this? I think there are many DT platform drivers that do not use pci_host_common_probe(), so I wonder if there's a more generic place to put this. I see Rob's concern about adding "preserve-boot-config" vs extending "linux,pci-probe-only" and I don't really have an opinion on that, although I do think the "pci-probe-only" name is not as descriptive as it could be. I guess somebody will argue that "preserve_config" could be more descriptive, too :) Bjorn _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH V2 2/2] PCI: Add support for "preserve-boot-config" property 2024-01-12 16:58 ` Bjorn Helgaas @ 2024-01-15 14:32 ` Vidya Sagar -1 siblings, 0 replies; 58+ messages in thread From: Vidya Sagar @ 2024-01-15 14:32 UTC (permalink / raw) To: Bjorn Helgaas, robh Cc: lpieralisi, kw, bhelgaas, krzysztof.kozlowski+dt, conor+dt, will, frowand.list, linux-pci, devicetree, linux-kernel, linux-arm-kernel, treding, jonathanh, kthota, mmaddireddy, sagar.tv, kernel test robot On 1/12/2024 10:28 PM, Bjorn Helgaas wrote: > External email: Use caution opening links or attachments > > > On Wed, Jan 10, 2024 at 08:37:25AM +0530, Vidya Sagar wrote: >> Add support for "preserve-boot-config" property that can be used to >> selectively (i.e. per host bridge) instruct the kernel to preserve the >> boot time configuration done by the platform firmware. >> >> Reported-by: kernel test robot <lkp@intel.com> >> Signed-off-by: Vidya Sagar <vidyas@nvidia.com> >> --- >> V2: >> * Addressed issues reported by kernel test robot <lkp@intel.com> >> >> drivers/pci/controller/pci-host-common.c | 5 ++++- >> drivers/pci/of.c | 18 ++++++++++++++++++ >> drivers/pci/probe.c | 2 +- >> include/linux/of_pci.h | 6 ++++++ >> 4 files changed, 29 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/pci/controller/pci-host-common.c b/drivers/pci/controller/pci-host-common.c >> index 6be3266cd7b5..d3475dc9ec44 100644 >> --- a/drivers/pci/controller/pci-host-common.c >> +++ b/drivers/pci/controller/pci-host-common.c >> @@ -68,13 +68,16 @@ int pci_host_common_probe(struct platform_device *pdev) >> >> of_pci_check_probe_only(); >> >> + bridge->preserve_config = >> + of_pci_check_preserve_boot_config(dev->of_node); > > Thanks for leveraging the existing "preserve_config" support for the > ACPI _DSM. Is pci_host_common_probe() the best place for this? I > think there are many DT platform drivers that do not use > pci_host_common_probe(), so I wonder if there's a more generic place > to put this. My understanding is that pci_host_common_probe() is mainly used in systems where the firmware would have taken care of all the platform specific initialization and giving the ECAM and 'ranges' info through DT for the Linux kernel to go ahead and perform the enumeration. This is similar to ACPI way of handing over the system from firmware to the OS. If PCIe controllers are getting initialized in the kernel itself, then pci_host_probe() is called directly from the respective host controller drivers which is the case with all the DesignWare based implementations including Tegra194 and Tegra234. In those systems, since the controllers themselves have come up and gotten initialized in the kernel, resource assignment has to happen anyway. > > I see Rob's concern about adding "preserve-boot-config" vs extending > "linux,pci-probe-only" and I don't really have an opinion on that, > although I do think the "pci-probe-only" name is not as descriptive as > it could be. I guess somebody will argue that "preserve_config" could > be more descriptive, too :) Honestly I would have liked to repurpose of_pci_check_probe_only() API to look for "preserve-boot-config" in the respective PCIe controller's DT node and not "linux,pci-probe-only" in the chosen entry, had it not for the single usage of of_pci_check_probe_only() in arch/powerpc /platforms/pseries/setup.c file. Also FWIW, "linux,pci-probe-only" is not documented anywhere. Since there is at least one user for of_pci_check_probe_only(), and combining with the fact that the scope where "linux,pci-probe-only" and "preserve-boot-config" are used (i.e. chosen entry Vs individual PCIe controller node), I prefer to have it as a separate option. Rob, please let me know if you have any strong objections to that? > > Bjorn ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH V2 2/2] PCI: Add support for "preserve-boot-config" property @ 2024-01-15 14:32 ` Vidya Sagar 0 siblings, 0 replies; 58+ messages in thread From: Vidya Sagar @ 2024-01-15 14:32 UTC (permalink / raw) To: Bjorn Helgaas, robh Cc: lpieralisi, kw, bhelgaas, krzysztof.kozlowski+dt, conor+dt, will, frowand.list, linux-pci, devicetree, linux-kernel, linux-arm-kernel, treding, jonathanh, kthota, mmaddireddy, sagar.tv, kernel test robot On 1/12/2024 10:28 PM, Bjorn Helgaas wrote: > External email: Use caution opening links or attachments > > > On Wed, Jan 10, 2024 at 08:37:25AM +0530, Vidya Sagar wrote: >> Add support for "preserve-boot-config" property that can be used to >> selectively (i.e. per host bridge) instruct the kernel to preserve the >> boot time configuration done by the platform firmware. >> >> Reported-by: kernel test robot <lkp@intel.com> >> Signed-off-by: Vidya Sagar <vidyas@nvidia.com> >> --- >> V2: >> * Addressed issues reported by kernel test robot <lkp@intel.com> >> >> drivers/pci/controller/pci-host-common.c | 5 ++++- >> drivers/pci/of.c | 18 ++++++++++++++++++ >> drivers/pci/probe.c | 2 +- >> include/linux/of_pci.h | 6 ++++++ >> 4 files changed, 29 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/pci/controller/pci-host-common.c b/drivers/pci/controller/pci-host-common.c >> index 6be3266cd7b5..d3475dc9ec44 100644 >> --- a/drivers/pci/controller/pci-host-common.c >> +++ b/drivers/pci/controller/pci-host-common.c >> @@ -68,13 +68,16 @@ int pci_host_common_probe(struct platform_device *pdev) >> >> of_pci_check_probe_only(); >> >> + bridge->preserve_config = >> + of_pci_check_preserve_boot_config(dev->of_node); > > Thanks for leveraging the existing "preserve_config" support for the > ACPI _DSM. Is pci_host_common_probe() the best place for this? I > think there are many DT platform drivers that do not use > pci_host_common_probe(), so I wonder if there's a more generic place > to put this. My understanding is that pci_host_common_probe() is mainly used in systems where the firmware would have taken care of all the platform specific initialization and giving the ECAM and 'ranges' info through DT for the Linux kernel to go ahead and perform the enumeration. This is similar to ACPI way of handing over the system from firmware to the OS. If PCIe controllers are getting initialized in the kernel itself, then pci_host_probe() is called directly from the respective host controller drivers which is the case with all the DesignWare based implementations including Tegra194 and Tegra234. In those systems, since the controllers themselves have come up and gotten initialized in the kernel, resource assignment has to happen anyway. > > I see Rob's concern about adding "preserve-boot-config" vs extending > "linux,pci-probe-only" and I don't really have an opinion on that, > although I do think the "pci-probe-only" name is not as descriptive as > it could be. I guess somebody will argue that "preserve_config" could > be more descriptive, too :) Honestly I would have liked to repurpose of_pci_check_probe_only() API to look for "preserve-boot-config" in the respective PCIe controller's DT node and not "linux,pci-probe-only" in the chosen entry, had it not for the single usage of of_pci_check_probe_only() in arch/powerpc /platforms/pseries/setup.c file. Also FWIW, "linux,pci-probe-only" is not documented anywhere. Since there is at least one user for of_pci_check_probe_only(), and combining with the fact that the scope where "linux,pci-probe-only" and "preserve-boot-config" are used (i.e. chosen entry Vs individual PCIe controller node), I prefer to have it as a separate option. Rob, please let me know if you have any strong objections to that? > > Bjorn _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH V2 2/2] PCI: Add support for "preserve-boot-config" property 2024-01-15 14:32 ` Vidya Sagar @ 2024-01-16 16:55 ` Rob Herring -1 siblings, 0 replies; 58+ messages in thread From: Rob Herring @ 2024-01-16 16:55 UTC (permalink / raw) To: Vidya Sagar Cc: Bjorn Helgaas, lpieralisi, kw, bhelgaas, krzysztof.kozlowski+dt, conor+dt, will, frowand.list, linux-pci, devicetree, linux-kernel, linux-arm-kernel, treding, jonathanh, kthota, mmaddireddy, sagar.tv, kernel test robot On Mon, Jan 15, 2024 at 08:02:56PM +0530, Vidya Sagar wrote: > > > On 1/12/2024 10:28 PM, Bjorn Helgaas wrote: > > External email: Use caution opening links or attachments > > > > > > On Wed, Jan 10, 2024 at 08:37:25AM +0530, Vidya Sagar wrote: > > > Add support for "preserve-boot-config" property that can be used to > > > selectively (i.e. per host bridge) instruct the kernel to preserve the > > > boot time configuration done by the platform firmware. > > > > > > Reported-by: kernel test robot <lkp@intel.com> > > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > > > --- > > > V2: > > > * Addressed issues reported by kernel test robot <lkp@intel.com> > > > > > > drivers/pci/controller/pci-host-common.c | 5 ++++- > > > drivers/pci/of.c | 18 ++++++++++++++++++ > > > drivers/pci/probe.c | 2 +- > > > include/linux/of_pci.h | 6 ++++++ > > > 4 files changed, 29 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/pci/controller/pci-host-common.c b/drivers/pci/controller/pci-host-common.c > > > index 6be3266cd7b5..d3475dc9ec44 100644 > > > --- a/drivers/pci/controller/pci-host-common.c > > > +++ b/drivers/pci/controller/pci-host-common.c > > > @@ -68,13 +68,16 @@ int pci_host_common_probe(struct platform_device *pdev) > > > > > > of_pci_check_probe_only(); > > > > > > + bridge->preserve_config = > > > + of_pci_check_preserve_boot_config(dev->of_node); > > > > Thanks for leveraging the existing "preserve_config" support for the > > ACPI _DSM. Is pci_host_common_probe() the best place for this? I > > think there are many DT platform drivers that do not use > > pci_host_common_probe(), so I wonder if there's a more generic place > > to put this. > My understanding is that pci_host_common_probe() is mainly used in > systems where the firmware would have taken care of all the platform > specific initialization and giving the ECAM and 'ranges' info through DT > for the Linux kernel to go ahead and perform the enumeration. This is > similar to ACPI way of handing over the system from firmware to the OS. > > If PCIe controllers are getting initialized in the kernel itself, then > pci_host_probe() is called directly from the respective host controller > drivers which is the case with all the DesignWare based implementations > including Tegra194 and Tegra234. In those systems, since the controllers > themselves have come up and gotten initialized in the kernel, resource > assignment has to happen anyway. > > > > > I see Rob's concern about adding "preserve-boot-config" vs extending > > "linux,pci-probe-only" and I don't really have an opinion on that, > > although I do think the "pci-probe-only" name is not as descriptive as > > it could be. I guess somebody will argue that "preserve_config" could > > be more descriptive, too :) > Honestly I would have liked to repurpose of_pci_check_probe_only() API > to look for "preserve-boot-config" in the respective PCIe controller's > DT node and not "linux,pci-probe-only" in the chosen entry, had it not > for the single usage of of_pci_check_probe_only() in arch/powerpc > /platforms/pseries/setup.c file. > Also FWIW, "linux,pci-probe-only" is not documented anywhere. Yes, it is[1]. > > Since there is at least one user for of_pci_check_probe_only(), and > combining with the fact that the scope where "linux,pci-probe-only" and > "preserve-boot-config" are used (i.e. chosen entry Vs individual PCIe > controller node), I prefer to have it as a separate option. > Rob, please let me know if you have any strong objections to that? Didn't I already object? What's the concern with existing users? There shouldn't be any. If "linux,pci-probe-only" appeared in a bridge node, it would have been ignored and now would be honored. Rob [1] https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/chosen.yaml#L140 ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH V2 2/2] PCI: Add support for "preserve-boot-config" property @ 2024-01-16 16:55 ` Rob Herring 0 siblings, 0 replies; 58+ messages in thread From: Rob Herring @ 2024-01-16 16:55 UTC (permalink / raw) To: Vidya Sagar Cc: Bjorn Helgaas, lpieralisi, kw, bhelgaas, krzysztof.kozlowski+dt, conor+dt, will, frowand.list, linux-pci, devicetree, linux-kernel, linux-arm-kernel, treding, jonathanh, kthota, mmaddireddy, sagar.tv, kernel test robot On Mon, Jan 15, 2024 at 08:02:56PM +0530, Vidya Sagar wrote: > > > On 1/12/2024 10:28 PM, Bjorn Helgaas wrote: > > External email: Use caution opening links or attachments > > > > > > On Wed, Jan 10, 2024 at 08:37:25AM +0530, Vidya Sagar wrote: > > > Add support for "preserve-boot-config" property that can be used to > > > selectively (i.e. per host bridge) instruct the kernel to preserve the > > > boot time configuration done by the platform firmware. > > > > > > Reported-by: kernel test robot <lkp@intel.com> > > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > > > --- > > > V2: > > > * Addressed issues reported by kernel test robot <lkp@intel.com> > > > > > > drivers/pci/controller/pci-host-common.c | 5 ++++- > > > drivers/pci/of.c | 18 ++++++++++++++++++ > > > drivers/pci/probe.c | 2 +- > > > include/linux/of_pci.h | 6 ++++++ > > > 4 files changed, 29 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/pci/controller/pci-host-common.c b/drivers/pci/controller/pci-host-common.c > > > index 6be3266cd7b5..d3475dc9ec44 100644 > > > --- a/drivers/pci/controller/pci-host-common.c > > > +++ b/drivers/pci/controller/pci-host-common.c > > > @@ -68,13 +68,16 @@ int pci_host_common_probe(struct platform_device *pdev) > > > > > > of_pci_check_probe_only(); > > > > > > + bridge->preserve_config = > > > + of_pci_check_preserve_boot_config(dev->of_node); > > > > Thanks for leveraging the existing "preserve_config" support for the > > ACPI _DSM. Is pci_host_common_probe() the best place for this? I > > think there are many DT platform drivers that do not use > > pci_host_common_probe(), so I wonder if there's a more generic place > > to put this. > My understanding is that pci_host_common_probe() is mainly used in > systems where the firmware would have taken care of all the platform > specific initialization and giving the ECAM and 'ranges' info through DT > for the Linux kernel to go ahead and perform the enumeration. This is > similar to ACPI way of handing over the system from firmware to the OS. > > If PCIe controllers are getting initialized in the kernel itself, then > pci_host_probe() is called directly from the respective host controller > drivers which is the case with all the DesignWare based implementations > including Tegra194 and Tegra234. In those systems, since the controllers > themselves have come up and gotten initialized in the kernel, resource > assignment has to happen anyway. > > > > > I see Rob's concern about adding "preserve-boot-config" vs extending > > "linux,pci-probe-only" and I don't really have an opinion on that, > > although I do think the "pci-probe-only" name is not as descriptive as > > it could be. I guess somebody will argue that "preserve_config" could > > be more descriptive, too :) > Honestly I would have liked to repurpose of_pci_check_probe_only() API > to look for "preserve-boot-config" in the respective PCIe controller's > DT node and not "linux,pci-probe-only" in the chosen entry, had it not > for the single usage of of_pci_check_probe_only() in arch/powerpc > /platforms/pseries/setup.c file. > Also FWIW, "linux,pci-probe-only" is not documented anywhere. Yes, it is[1]. > > Since there is at least one user for of_pci_check_probe_only(), and > combining with the fact that the scope where "linux,pci-probe-only" and > "preserve-boot-config" are used (i.e. chosen entry Vs individual PCIe > controller node), I prefer to have it as a separate option. > Rob, please let me know if you have any strong objections to that? Didn't I already object? What's the concern with existing users? There shouldn't be any. If "linux,pci-probe-only" appeared in a bridge node, it would have been ignored and now would be honored. Rob [1] https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/chosen.yaml#L140 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH V2 2/2] PCI: Add support for "preserve-boot-config" property 2024-01-15 14:32 ` Vidya Sagar @ 2024-01-19 17:31 ` Bjorn Helgaas -1 siblings, 0 replies; 58+ messages in thread From: Bjorn Helgaas @ 2024-01-19 17:31 UTC (permalink / raw) To: Vidya Sagar Cc: robh, lpieralisi, kw, bhelgaas, krzysztof.kozlowski+dt, conor+dt, will, frowand.list, linux-pci, devicetree, linux-kernel, linux-arm-kernel, treding, jonathanh, kthota, mmaddireddy, sagar.tv, kernel test robot On Mon, Jan 15, 2024 at 08:02:56PM +0530, Vidya Sagar wrote: > On 1/12/2024 10:28 PM, Bjorn Helgaas wrote: > > On Wed, Jan 10, 2024 at 08:37:25AM +0530, Vidya Sagar wrote: > > > Add support for "preserve-boot-config" property that can be used to > > > selectively (i.e. per host bridge) instruct the kernel to preserve the > > > boot time configuration done by the platform firmware. > > > > > > Reported-by: kernel test robot <lkp@intel.com> > > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > > > --- > > > V2: > > > * Addressed issues reported by kernel test robot <lkp@intel.com> > > > > > > drivers/pci/controller/pci-host-common.c | 5 ++++- > > > drivers/pci/of.c | 18 ++++++++++++++++++ > > > drivers/pci/probe.c | 2 +- > > > include/linux/of_pci.h | 6 ++++++ > > > 4 files changed, 29 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/pci/controller/pci-host-common.c b/drivers/pci/controller/pci-host-common.c > > > index 6be3266cd7b5..d3475dc9ec44 100644 > > > --- a/drivers/pci/controller/pci-host-common.c > > > +++ b/drivers/pci/controller/pci-host-common.c > > > @@ -68,13 +68,16 @@ int pci_host_common_probe(struct platform_device *pdev) > > > > > > of_pci_check_probe_only(); > > > > > > + bridge->preserve_config = > > > + of_pci_check_preserve_boot_config(dev->of_node); > > > > Thanks for leveraging the existing "preserve_config" support for the > > ACPI _DSM. Is pci_host_common_probe() the best place for this? I > > think there are many DT platform drivers that do not use > > pci_host_common_probe(), so I wonder if there's a more generic place > > to put this. > > My understanding is that pci_host_common_probe() is mainly used in > systems where the firmware would have taken care of all the platform > specific initialization and giving the ECAM and 'ranges' info through DT > for the Linux kernel to go ahead and perform the enumeration. This is > similar to ACPI way of handing over the system from firmware to the OS. > > If PCIe controllers are getting initialized in the kernel itself, then > pci_host_probe() is called directly from the respective host controller > drivers which is the case with all the DesignWare based implementations > including Tegra194 and Tegra234. In those systems, since the controllers > themselves have come up and gotten initialized in the kernel, resource > assignment has to happen anyway. acpi_pci_root_create() sets "preserve_config" based on the DSM_PCI_PRESERVE_BOOT_CONFIG _DSM for all ACPI host bridges. Similarly, I think we should set "preserve_config" based on the DT "preserve-boot-config" property for *all* DT-based host bridges, regardless of where the controller init happens. acpi_pci_root_create pci_create_root_bus pci_alloc_host_bridge pci_register_host_bridge acpi_evaluate_dsm_typed(DSM_PCI_PRESERVE_BOOT_CONFIG) <-- pci_scan_child_bus pci_host_common_probe + of_pci_check_preserve_boot_config <-- proposed pci_host_probe pci_scan_root_bus_bridge pci_register_host_bridge pci_set_bus_of_node pci_scan_child_bus Maybe we should do both in pci_register_host_bridge()? E.g., make a function that sets "preserve_config" based on either the ACPI _DSM or the DT property, whichever is appropriate, and call it from pci_register_host_bridge()? Bjorn ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH V2 2/2] PCI: Add support for "preserve-boot-config" property @ 2024-01-19 17:31 ` Bjorn Helgaas 0 siblings, 0 replies; 58+ messages in thread From: Bjorn Helgaas @ 2024-01-19 17:31 UTC (permalink / raw) To: Vidya Sagar Cc: robh, lpieralisi, kw, bhelgaas, krzysztof.kozlowski+dt, conor+dt, will, frowand.list, linux-pci, devicetree, linux-kernel, linux-arm-kernel, treding, jonathanh, kthota, mmaddireddy, sagar.tv, kernel test robot On Mon, Jan 15, 2024 at 08:02:56PM +0530, Vidya Sagar wrote: > On 1/12/2024 10:28 PM, Bjorn Helgaas wrote: > > On Wed, Jan 10, 2024 at 08:37:25AM +0530, Vidya Sagar wrote: > > > Add support for "preserve-boot-config" property that can be used to > > > selectively (i.e. per host bridge) instruct the kernel to preserve the > > > boot time configuration done by the platform firmware. > > > > > > Reported-by: kernel test robot <lkp@intel.com> > > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > > > --- > > > V2: > > > * Addressed issues reported by kernel test robot <lkp@intel.com> > > > > > > drivers/pci/controller/pci-host-common.c | 5 ++++- > > > drivers/pci/of.c | 18 ++++++++++++++++++ > > > drivers/pci/probe.c | 2 +- > > > include/linux/of_pci.h | 6 ++++++ > > > 4 files changed, 29 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/pci/controller/pci-host-common.c b/drivers/pci/controller/pci-host-common.c > > > index 6be3266cd7b5..d3475dc9ec44 100644 > > > --- a/drivers/pci/controller/pci-host-common.c > > > +++ b/drivers/pci/controller/pci-host-common.c > > > @@ -68,13 +68,16 @@ int pci_host_common_probe(struct platform_device *pdev) > > > > > > of_pci_check_probe_only(); > > > > > > + bridge->preserve_config = > > > + of_pci_check_preserve_boot_config(dev->of_node); > > > > Thanks for leveraging the existing "preserve_config" support for the > > ACPI _DSM. Is pci_host_common_probe() the best place for this? I > > think there are many DT platform drivers that do not use > > pci_host_common_probe(), so I wonder if there's a more generic place > > to put this. > > My understanding is that pci_host_common_probe() is mainly used in > systems where the firmware would have taken care of all the platform > specific initialization and giving the ECAM and 'ranges' info through DT > for the Linux kernel to go ahead and perform the enumeration. This is > similar to ACPI way of handing over the system from firmware to the OS. > > If PCIe controllers are getting initialized in the kernel itself, then > pci_host_probe() is called directly from the respective host controller > drivers which is the case with all the DesignWare based implementations > including Tegra194 and Tegra234. In those systems, since the controllers > themselves have come up and gotten initialized in the kernel, resource > assignment has to happen anyway. acpi_pci_root_create() sets "preserve_config" based on the DSM_PCI_PRESERVE_BOOT_CONFIG _DSM for all ACPI host bridges. Similarly, I think we should set "preserve_config" based on the DT "preserve-boot-config" property for *all* DT-based host bridges, regardless of where the controller init happens. acpi_pci_root_create pci_create_root_bus pci_alloc_host_bridge pci_register_host_bridge acpi_evaluate_dsm_typed(DSM_PCI_PRESERVE_BOOT_CONFIG) <-- pci_scan_child_bus pci_host_common_probe + of_pci_check_preserve_boot_config <-- proposed pci_host_probe pci_scan_root_bus_bridge pci_register_host_bridge pci_set_bus_of_node pci_scan_child_bus Maybe we should do both in pci_register_host_bridge()? E.g., make a function that sets "preserve_config" based on either the ACPI _DSM or the DT property, whichever is appropriate, and call it from pci_register_host_bridge()? Bjorn _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH V2 2/2] PCI: Add support for "preserve-boot-config" property 2024-01-10 3:07 ` Vidya Sagar @ 2024-01-12 16:59 ` Bjorn Helgaas -1 siblings, 0 replies; 58+ messages in thread From: Bjorn Helgaas @ 2024-01-12 16:59 UTC (permalink / raw) To: Vidya Sagar Cc: lpieralisi, kw, robh, bhelgaas, krzysztof.kozlowski+dt, conor+dt, will, frowand.list, linux-pci, devicetree, linux-kernel, linux-arm-kernel, treding, jonathanh, kthota, mmaddireddy, sagar.tv, kernel test robot On Wed, Jan 10, 2024 at 08:37:25AM +0530, Vidya Sagar wrote: > Add support for "preserve-boot-config" property that can be used to > selectively (i.e. per host bridge) instruct the kernel to preserve the > boot time configuration done by the platform firmware. > > Reported-by: kernel test robot <lkp@intel.com> > Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > --- > V2: > * Addressed issues reported by kernel test robot <lkp@intel.com> I don't think the lkp Reported-by adds useful information in this case. I have no idea what lkp actually reported, but I don't think it reported a bug that is fixed by this patch. ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH V2 2/2] PCI: Add support for "preserve-boot-config" property @ 2024-01-12 16:59 ` Bjorn Helgaas 0 siblings, 0 replies; 58+ messages in thread From: Bjorn Helgaas @ 2024-01-12 16:59 UTC (permalink / raw) To: Vidya Sagar Cc: lpieralisi, kw, robh, bhelgaas, krzysztof.kozlowski+dt, conor+dt, will, frowand.list, linux-pci, devicetree, linux-kernel, linux-arm-kernel, treding, jonathanh, kthota, mmaddireddy, sagar.tv, kernel test robot On Wed, Jan 10, 2024 at 08:37:25AM +0530, Vidya Sagar wrote: > Add support for "preserve-boot-config" property that can be used to > selectively (i.e. per host bridge) instruct the kernel to preserve the > boot time configuration done by the platform firmware. > > Reported-by: kernel test robot <lkp@intel.com> > Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > --- > V2: > * Addressed issues reported by kernel test robot <lkp@intel.com> I don't think the lkp Reported-by adds useful information in this case. I have no idea what lkp actually reported, but I don't think it reported a bug that is fixed by this patch. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 58+ messages in thread
* [PATCH V3] PCI: Add support for preserving boot configuration 2024-01-10 3:07 ` Vidya Sagar @ 2024-02-22 12:41 ` Vidya Sagar -1 siblings, 0 replies; 58+ messages in thread From: Vidya Sagar @ 2024-02-22 12:41 UTC (permalink / raw) To: bhelgaas, rafael, lenb, will, lpieralisi, kw, robh, frowand.list Cc: linux-pci, linux-acpi, linux-kernel, linux-arm-kernel, devicetree, treding, jonathanh, kthota, mmaddireddy, vidyas, sagar.tv Add support for preserving the boot configuration done by the platform firmware per host bridge basis, based on the presence of 'linux,pci-probe-only' property in the respective PCIe host bridge device-tree node. It also unifies the ACPI and DT based boot flows in this regard. Signed-off-by: Vidya Sagar <vidyas@nvidia.com> --- V3: * Unified ACPI and DT flows as part of addressing Bjorn's review comments V2: * Addressed issues reported by kernel test robot <lkp@intel.com> drivers/acpi/pci_root.c | 12 ------- drivers/pci/controller/pci-host-common.c | 4 --- drivers/pci/of.c | 22 ++++++++++++ drivers/pci/probe.c | 46 ++++++++++++++++++------ include/linux/of_pci.h | 6 ++++ 5 files changed, 63 insertions(+), 27 deletions(-) diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c index 84030804a763..ddc2b3e89111 100644 --- a/drivers/acpi/pci_root.c +++ b/drivers/acpi/pci_root.c @@ -1008,7 +1008,6 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root, int node = acpi_get_node(device->handle); struct pci_bus *bus; struct pci_host_bridge *host_bridge; - union acpi_object *obj; info->root = root; info->bridge = device; @@ -1050,17 +1049,6 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root, if (!(root->osc_ext_control_set & OSC_CXL_ERROR_REPORTING_CONTROL)) host_bridge->native_cxl_error = 0; - /* - * Evaluate the "PCI Boot Configuration" _DSM Function. If it - * exists and returns 0, we must preserve any PCI resource - * assignments made by firmware for this host bridge. - */ - obj = acpi_evaluate_dsm(ACPI_HANDLE(bus->bridge), &pci_acpi_dsm_guid, 1, - DSM_PCI_PRESERVE_BOOT_CONFIG, NULL); - if (obj && obj->type == ACPI_TYPE_INTEGER && obj->integer.value == 0) - host_bridge->preserve_config = 1; - ACPI_FREE(obj); - acpi_dev_power_up_children_with_adr(device); pci_scan_child_bus(bus); diff --git a/drivers/pci/controller/pci-host-common.c b/drivers/pci/controller/pci-host-common.c index 6be3266cd7b5..e2602e38ae45 100644 --- a/drivers/pci/controller/pci-host-common.c +++ b/drivers/pci/controller/pci-host-common.c @@ -73,10 +73,6 @@ int pci_host_common_probe(struct platform_device *pdev) if (IS_ERR(cfg)) return PTR_ERR(cfg); - /* Do not reassign resources if probe only */ - if (!pci_has_flag(PCI_PROBE_ONLY)) - pci_add_flags(PCI_REASSIGN_ALL_BUS); - bridge->sysdata = cfg; bridge->ops = (struct pci_ops *)&ops->pci_ops; bridge->msi_domain = true; diff --git a/drivers/pci/of.c b/drivers/pci/of.c index 51e3dd0ea5ab..7b553dd83587 100644 --- a/drivers/pci/of.c +++ b/drivers/pci/of.c @@ -258,6 +258,28 @@ void of_pci_check_probe_only(void) } EXPORT_SYMBOL_GPL(of_pci_check_probe_only); +/** + * of_pci_bridge_check_probe_only - Return true if the boot configuration + * needs to be preserved + * @node: Device tree node with the domain information. + * + * This function looks for "linux,pci-probe-only" property for a given + * PCIe controller's node and returns true if found. Having this property + * for a PCIe controller ensures that the kernel doesn't re-enumerate and + * reconfigure the BAR resources that are already done by the platform firmware. + * NOTE: The scope of "linux,pci-probe-only" defined within a PCIe bridge device + * is limited to the hierarchy under that particular bridge device. whereas + * the scope of "linux,pci-probe-only" defined within chosen node is + * system wide. + * + * Return: true if the property exists false otherwise. + */ +bool of_pci_bridge_check_probe_only(struct device_node *node) +{ + return of_property_read_bool(node, "linux,pci-probe-only"); +} +EXPORT_SYMBOL_GPL(of_pci_bridge_check_probe_only); + /** * devm_of_pci_get_host_bridge_resources() - Resource-managed parsing of PCI * host bridge resources from DT diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 795534589b98..d62d1f151ba9 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -15,6 +15,7 @@ #include <linux/cpumask.h> #include <linux/aer.h> #include <linux/acpi.h> +#include <linux/pci-acpi.h> #include <linux/hypervisor.h> #include <linux/irqdomain.h> #include <linux/pm_runtime.h> @@ -877,6 +878,28 @@ static void pci_set_bus_msi_domain(struct pci_bus *bus) dev_set_msi_domain(&bus->dev, d); } +static void pci_check_config_preserve(struct pci_host_bridge *host_bridge) +{ + if (&host_bridge->dev) { + union acpi_object *obj; + + /* + * Evaluate the "PCI Boot Configuration" _DSM Function. If it + * exists and returns 0, we must preserve any PCI resource + * assignments made by firmware for this host bridge. + */ + obj = acpi_evaluate_dsm(ACPI_HANDLE(&host_bridge->dev), &pci_acpi_dsm_guid, 1, + DSM_PCI_PRESERVE_BOOT_CONFIG, NULL); + if (obj && obj->type == ACPI_TYPE_INTEGER && obj->integer.value == 0) + host_bridge->preserve_config = 1; + ACPI_FREE(obj); + } + + if (host_bridge->dev.parent && host_bridge->dev.parent->of_node) + host_bridge->preserve_config = + of_pci_bridge_check_probe_only(host_bridge->dev.parent->of_node); +} + static int pci_register_host_bridge(struct pci_host_bridge *bridge) { struct device *parent = bridge->dev.parent; @@ -971,6 +994,9 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge) if (nr_node_ids > 1 && pcibus_to_node(bus) == NUMA_NO_NODE) dev_warn(&bus->dev, "Unknown NUMA node; performance will be reduced\n"); + /* Check if the boot configuration by FW needs to be preserved */ + pci_check_config_preserve(bridge); + /* Coalesce contiguous windows */ resource_list_for_each_entry_safe(window, n, &resources) { if (list_is_last(&window->node, &resources)) @@ -3080,20 +3106,18 @@ int pci_host_probe(struct pci_host_bridge *bridge) bus = bridge->bus; + /* If we must preserve the resource configuration, claim now */ + if (pci_has_flag(PCI_PROBE_ONLY) || bridge->preserve_config) + pci_bus_claim_resources(bus); + /* - * We insert PCI resources into the iomem_resource and - * ioport_resource trees in either pci_bus_claim_resources() - * or pci_bus_assign_resources(). + * Assign whatever was left unassigned. If we didn't claim above, + * this will reassign everything. */ - if (pci_has_flag(PCI_PROBE_ONLY)) { - pci_bus_claim_resources(bus); - } else { - pci_bus_size_bridges(bus); - pci_bus_assign_resources(bus); + pci_assign_unassigned_root_bus_resources(bus); - list_for_each_entry(child, &bus->children, node) - pcie_bus_configure_settings(child); - } + list_for_each_entry(child, &bus->children, node) + pcie_bus_configure_settings(child); pci_bus_add_devices(bus); return 0; diff --git a/include/linux/of_pci.h b/include/linux/of_pci.h index 29658c0ee71f..9e045de3be44 100644 --- a/include/linux/of_pci.h +++ b/include/linux/of_pci.h @@ -13,6 +13,7 @@ struct device_node *of_pci_find_child_device(struct device_node *parent, unsigned int devfn); int of_pci_get_devfn(struct device_node *np); void of_pci_check_probe_only(void); +bool of_pci_bridge_check_probe_only(struct device_node *node); #else static inline struct device_node *of_pci_find_child_device(struct device_node *parent, unsigned int devfn) @@ -26,6 +27,11 @@ static inline int of_pci_get_devfn(struct device_node *np) } static inline void of_pci_check_probe_only(void) { } + +static inline bool of_pci_bridge_check_probe_only(struct device_node *node) +{ + return false; +} #endif #if IS_ENABLED(CONFIG_OF_IRQ) -- 2.25.1 ^ permalink raw reply related [flat|nested] 58+ messages in thread
* [PATCH V3] PCI: Add support for preserving boot configuration @ 2024-02-22 12:41 ` Vidya Sagar 0 siblings, 0 replies; 58+ messages in thread From: Vidya Sagar @ 2024-02-22 12:41 UTC (permalink / raw) To: bhelgaas, rafael, lenb, will, lpieralisi, kw, robh, frowand.list Cc: linux-pci, linux-acpi, linux-kernel, linux-arm-kernel, devicetree, treding, jonathanh, kthota, mmaddireddy, vidyas, sagar.tv Add support for preserving the boot configuration done by the platform firmware per host bridge basis, based on the presence of 'linux,pci-probe-only' property in the respective PCIe host bridge device-tree node. It also unifies the ACPI and DT based boot flows in this regard. Signed-off-by: Vidya Sagar <vidyas@nvidia.com> --- V3: * Unified ACPI and DT flows as part of addressing Bjorn's review comments V2: * Addressed issues reported by kernel test robot <lkp@intel.com> drivers/acpi/pci_root.c | 12 ------- drivers/pci/controller/pci-host-common.c | 4 --- drivers/pci/of.c | 22 ++++++++++++ drivers/pci/probe.c | 46 ++++++++++++++++++------ include/linux/of_pci.h | 6 ++++ 5 files changed, 63 insertions(+), 27 deletions(-) diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c index 84030804a763..ddc2b3e89111 100644 --- a/drivers/acpi/pci_root.c +++ b/drivers/acpi/pci_root.c @@ -1008,7 +1008,6 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root, int node = acpi_get_node(device->handle); struct pci_bus *bus; struct pci_host_bridge *host_bridge; - union acpi_object *obj; info->root = root; info->bridge = device; @@ -1050,17 +1049,6 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root, if (!(root->osc_ext_control_set & OSC_CXL_ERROR_REPORTING_CONTROL)) host_bridge->native_cxl_error = 0; - /* - * Evaluate the "PCI Boot Configuration" _DSM Function. If it - * exists and returns 0, we must preserve any PCI resource - * assignments made by firmware for this host bridge. - */ - obj = acpi_evaluate_dsm(ACPI_HANDLE(bus->bridge), &pci_acpi_dsm_guid, 1, - DSM_PCI_PRESERVE_BOOT_CONFIG, NULL); - if (obj && obj->type == ACPI_TYPE_INTEGER && obj->integer.value == 0) - host_bridge->preserve_config = 1; - ACPI_FREE(obj); - acpi_dev_power_up_children_with_adr(device); pci_scan_child_bus(bus); diff --git a/drivers/pci/controller/pci-host-common.c b/drivers/pci/controller/pci-host-common.c index 6be3266cd7b5..e2602e38ae45 100644 --- a/drivers/pci/controller/pci-host-common.c +++ b/drivers/pci/controller/pci-host-common.c @@ -73,10 +73,6 @@ int pci_host_common_probe(struct platform_device *pdev) if (IS_ERR(cfg)) return PTR_ERR(cfg); - /* Do not reassign resources if probe only */ - if (!pci_has_flag(PCI_PROBE_ONLY)) - pci_add_flags(PCI_REASSIGN_ALL_BUS); - bridge->sysdata = cfg; bridge->ops = (struct pci_ops *)&ops->pci_ops; bridge->msi_domain = true; diff --git a/drivers/pci/of.c b/drivers/pci/of.c index 51e3dd0ea5ab..7b553dd83587 100644 --- a/drivers/pci/of.c +++ b/drivers/pci/of.c @@ -258,6 +258,28 @@ void of_pci_check_probe_only(void) } EXPORT_SYMBOL_GPL(of_pci_check_probe_only); +/** + * of_pci_bridge_check_probe_only - Return true if the boot configuration + * needs to be preserved + * @node: Device tree node with the domain information. + * + * This function looks for "linux,pci-probe-only" property for a given + * PCIe controller's node and returns true if found. Having this property + * for a PCIe controller ensures that the kernel doesn't re-enumerate and + * reconfigure the BAR resources that are already done by the platform firmware. + * NOTE: The scope of "linux,pci-probe-only" defined within a PCIe bridge device + * is limited to the hierarchy under that particular bridge device. whereas + * the scope of "linux,pci-probe-only" defined within chosen node is + * system wide. + * + * Return: true if the property exists false otherwise. + */ +bool of_pci_bridge_check_probe_only(struct device_node *node) +{ + return of_property_read_bool(node, "linux,pci-probe-only"); +} +EXPORT_SYMBOL_GPL(of_pci_bridge_check_probe_only); + /** * devm_of_pci_get_host_bridge_resources() - Resource-managed parsing of PCI * host bridge resources from DT diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 795534589b98..d62d1f151ba9 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -15,6 +15,7 @@ #include <linux/cpumask.h> #include <linux/aer.h> #include <linux/acpi.h> +#include <linux/pci-acpi.h> #include <linux/hypervisor.h> #include <linux/irqdomain.h> #include <linux/pm_runtime.h> @@ -877,6 +878,28 @@ static void pci_set_bus_msi_domain(struct pci_bus *bus) dev_set_msi_domain(&bus->dev, d); } +static void pci_check_config_preserve(struct pci_host_bridge *host_bridge) +{ + if (&host_bridge->dev) { + union acpi_object *obj; + + /* + * Evaluate the "PCI Boot Configuration" _DSM Function. If it + * exists and returns 0, we must preserve any PCI resource + * assignments made by firmware for this host bridge. + */ + obj = acpi_evaluate_dsm(ACPI_HANDLE(&host_bridge->dev), &pci_acpi_dsm_guid, 1, + DSM_PCI_PRESERVE_BOOT_CONFIG, NULL); + if (obj && obj->type == ACPI_TYPE_INTEGER && obj->integer.value == 0) + host_bridge->preserve_config = 1; + ACPI_FREE(obj); + } + + if (host_bridge->dev.parent && host_bridge->dev.parent->of_node) + host_bridge->preserve_config = + of_pci_bridge_check_probe_only(host_bridge->dev.parent->of_node); +} + static int pci_register_host_bridge(struct pci_host_bridge *bridge) { struct device *parent = bridge->dev.parent; @@ -971,6 +994,9 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge) if (nr_node_ids > 1 && pcibus_to_node(bus) == NUMA_NO_NODE) dev_warn(&bus->dev, "Unknown NUMA node; performance will be reduced\n"); + /* Check if the boot configuration by FW needs to be preserved */ + pci_check_config_preserve(bridge); + /* Coalesce contiguous windows */ resource_list_for_each_entry_safe(window, n, &resources) { if (list_is_last(&window->node, &resources)) @@ -3080,20 +3106,18 @@ int pci_host_probe(struct pci_host_bridge *bridge) bus = bridge->bus; + /* If we must preserve the resource configuration, claim now */ + if (pci_has_flag(PCI_PROBE_ONLY) || bridge->preserve_config) + pci_bus_claim_resources(bus); + /* - * We insert PCI resources into the iomem_resource and - * ioport_resource trees in either pci_bus_claim_resources() - * or pci_bus_assign_resources(). + * Assign whatever was left unassigned. If we didn't claim above, + * this will reassign everything. */ - if (pci_has_flag(PCI_PROBE_ONLY)) { - pci_bus_claim_resources(bus); - } else { - pci_bus_size_bridges(bus); - pci_bus_assign_resources(bus); + pci_assign_unassigned_root_bus_resources(bus); - list_for_each_entry(child, &bus->children, node) - pcie_bus_configure_settings(child); - } + list_for_each_entry(child, &bus->children, node) + pcie_bus_configure_settings(child); pci_bus_add_devices(bus); return 0; diff --git a/include/linux/of_pci.h b/include/linux/of_pci.h index 29658c0ee71f..9e045de3be44 100644 --- a/include/linux/of_pci.h +++ b/include/linux/of_pci.h @@ -13,6 +13,7 @@ struct device_node *of_pci_find_child_device(struct device_node *parent, unsigned int devfn); int of_pci_get_devfn(struct device_node *np); void of_pci_check_probe_only(void); +bool of_pci_bridge_check_probe_only(struct device_node *node); #else static inline struct device_node *of_pci_find_child_device(struct device_node *parent, unsigned int devfn) @@ -26,6 +27,11 @@ static inline int of_pci_get_devfn(struct device_node *np) } static inline void of_pci_check_probe_only(void) { } + +static inline bool of_pci_bridge_check_probe_only(struct device_node *node) +{ + return false; +} #endif #if IS_ENABLED(CONFIG_OF_IRQ) -- 2.25.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 58+ messages in thread
* Re: [PATCH V3] PCI: Add support for preserving boot configuration 2024-02-22 12:41 ` Vidya Sagar @ 2024-02-22 17:06 ` Bjorn Helgaas -1 siblings, 0 replies; 58+ messages in thread From: Bjorn Helgaas @ 2024-02-22 17:06 UTC (permalink / raw) To: Vidya Sagar Cc: bhelgaas, rafael, lenb, will, lpieralisi, kw, robh, frowand.list, linux-pci, linux-acpi, linux-kernel, linux-arm-kernel, devicetree, treding, jonathanh, kthota, mmaddireddy, sagar.tv On Thu, Feb 22, 2024 at 06:11:10PM +0530, Vidya Sagar wrote: > Add support for preserving the boot configuration done by the > platform firmware per host bridge basis, based on the presence of > 'linux,pci-probe-only' property in the respective PCIe host bridge > device-tree node. It also unifies the ACPI and DT based boot flows > in this regard. > +/** > + * of_pci_bridge_check_probe_only - Return true if the boot configuration > + * needs to be preserved I don't like the "check_probe_only" name because it's a boolean function but the name doesn't tell me what a true/false return value means. Something like "preserve_resources" would be better. If you want "probe_only", even removing the "check" would help. > + * @node: Device tree node with the domain information. > + * > + * This function looks for "linux,pci-probe-only" property for a given > + * PCIe controller's node and returns true if found. Having this property > + * for a PCIe controller ensures that the kernel doesn't re-enumerate and > + * reconfigure the BAR resources that are already done by the platform firmware. This is generic PCI, not PCIe-specific (also in commit log and comment below). I think "enumeration" specifically refers to discovering what devices are present, and the kernel always does that, so drop that part. Reconfiguring BARs and bridge windows is what we want to prevent. > + * NOTE: The scope of "linux,pci-probe-only" defined within a PCIe bridge device > + * is limited to the hierarchy under that particular bridge device. whereas > + * the scope of "linux,pci-probe-only" defined within chosen node is > + * system wide. > + * > + * Return: true if the property exists false otherwise. > + */ > +bool of_pci_bridge_check_probe_only(struct device_node *node) > +{ > + return of_property_read_bool(node, "linux,pci-probe-only"); > +} > +EXPORT_SYMBOL_GPL(of_pci_bridge_check_probe_only); Why does this need to be exported for modules and exposed via include/linux/pci.h? > +static void pci_check_config_preserve(struct pci_host_bridge *host_bridge) > +{ > + if (&host_bridge->dev) { Checking &host_bridge->dev doesn't seem like the right way to determine whether this is an ACPI host bridge. > + union acpi_object *obj; > + > + /* > + * Evaluate the "PCI Boot Configuration" _DSM Function. If it > + * exists and returns 0, we must preserve any PCI resource > + * assignments made by firmware for this host bridge. > + */ > + obj = acpi_evaluate_dsm(ACPI_HANDLE(&host_bridge->dev), &pci_acpi_dsm_guid, 1, > + DSM_PCI_PRESERVE_BOOT_CONFIG, NULL); > + if (obj && obj->type == ACPI_TYPE_INTEGER && obj->integer.value == 0) > + host_bridge->preserve_config = 1; > + ACPI_FREE(obj); > + } ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH V3] PCI: Add support for preserving boot configuration @ 2024-02-22 17:06 ` Bjorn Helgaas 0 siblings, 0 replies; 58+ messages in thread From: Bjorn Helgaas @ 2024-02-22 17:06 UTC (permalink / raw) To: Vidya Sagar Cc: bhelgaas, rafael, lenb, will, lpieralisi, kw, robh, frowand.list, linux-pci, linux-acpi, linux-kernel, linux-arm-kernel, devicetree, treding, jonathanh, kthota, mmaddireddy, sagar.tv On Thu, Feb 22, 2024 at 06:11:10PM +0530, Vidya Sagar wrote: > Add support for preserving the boot configuration done by the > platform firmware per host bridge basis, based on the presence of > 'linux,pci-probe-only' property in the respective PCIe host bridge > device-tree node. It also unifies the ACPI and DT based boot flows > in this regard. > +/** > + * of_pci_bridge_check_probe_only - Return true if the boot configuration > + * needs to be preserved I don't like the "check_probe_only" name because it's a boolean function but the name doesn't tell me what a true/false return value means. Something like "preserve_resources" would be better. If you want "probe_only", even removing the "check" would help. > + * @node: Device tree node with the domain information. > + * > + * This function looks for "linux,pci-probe-only" property for a given > + * PCIe controller's node and returns true if found. Having this property > + * for a PCIe controller ensures that the kernel doesn't re-enumerate and > + * reconfigure the BAR resources that are already done by the platform firmware. This is generic PCI, not PCIe-specific (also in commit log and comment below). I think "enumeration" specifically refers to discovering what devices are present, and the kernel always does that, so drop that part. Reconfiguring BARs and bridge windows is what we want to prevent. > + * NOTE: The scope of "linux,pci-probe-only" defined within a PCIe bridge device > + * is limited to the hierarchy under that particular bridge device. whereas > + * the scope of "linux,pci-probe-only" defined within chosen node is > + * system wide. > + * > + * Return: true if the property exists false otherwise. > + */ > +bool of_pci_bridge_check_probe_only(struct device_node *node) > +{ > + return of_property_read_bool(node, "linux,pci-probe-only"); > +} > +EXPORT_SYMBOL_GPL(of_pci_bridge_check_probe_only); Why does this need to be exported for modules and exposed via include/linux/pci.h? > +static void pci_check_config_preserve(struct pci_host_bridge *host_bridge) > +{ > + if (&host_bridge->dev) { Checking &host_bridge->dev doesn't seem like the right way to determine whether this is an ACPI host bridge. > + union acpi_object *obj; > + > + /* > + * Evaluate the "PCI Boot Configuration" _DSM Function. If it > + * exists and returns 0, we must preserve any PCI resource > + * assignments made by firmware for this host bridge. > + */ > + obj = acpi_evaluate_dsm(ACPI_HANDLE(&host_bridge->dev), &pci_acpi_dsm_guid, 1, > + DSM_PCI_PRESERVE_BOOT_CONFIG, NULL); > + if (obj && obj->type == ACPI_TYPE_INTEGER && obj->integer.value == 0) > + host_bridge->preserve_config = 1; > + ACPI_FREE(obj); > + } _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH V3] PCI: Add support for preserving boot configuration 2024-02-22 17:06 ` Bjorn Helgaas @ 2024-02-22 21:18 ` Vidya Sagar -1 siblings, 0 replies; 58+ messages in thread From: Vidya Sagar @ 2024-02-22 21:18 UTC (permalink / raw) To: Bjorn Helgaas Cc: bhelgaas, rafael, lenb, will, lpieralisi, kw, robh, frowand.list, linux-pci, linux-acpi, linux-kernel, linux-arm-kernel, devicetree, treding, jonathanh, kthota, mmaddireddy, sagar.tv On 22-02-2024 22:36, Bjorn Helgaas wrote: > External email: Use caution opening links or attachments > > > On Thu, Feb 22, 2024 at 06:11:10PM +0530, Vidya Sagar wrote: >> Add support for preserving the boot configuration done by the >> platform firmware per host bridge basis, based on the presence of >> 'linux,pci-probe-only' property in the respective PCIe host bridge >> device-tree node. It also unifies the ACPI and DT based boot flows >> in this regard. >> +/** >> + * of_pci_bridge_check_probe_only - Return true if the boot configuration >> + * needs to be preserved > I don't like the "check_probe_only" name because it's a boolean > function but the name doesn't tell me what a true/false return value > means. Something like "preserve_resources" would be better. If you > want "probe_only", even removing the "check" would help. I'll change it in the next patch. >> + * @node: Device tree node with the domain information. >> + * >> + * This function looks for "linux,pci-probe-only" property for a given >> + * PCIe controller's node and returns true if found. Having this property >> + * for a PCIe controller ensures that the kernel doesn't re-enumerate and >> + * reconfigure the BAR resources that are already done by the platform firmware. > This is generic PCI, not PCIe-specific (also in commit log and comment > below). > > I think "enumeration" specifically refers to discovering what devices > are present, and the kernel always does that, so drop that part. > Reconfiguring BARs and bridge windows is what we want to prevent. Agree and I'll address it in the next patch. >> + * NOTE: The scope of "linux,pci-probe-only" defined within a PCIe bridge device >> + * is limited to the hierarchy under that particular bridge device. whereas >> + * the scope of "linux,pci-probe-only" defined within chosen node is >> + * system wide. >> + * >> + * Return: true if the property exists false otherwise. >> + */ >> +bool of_pci_bridge_check_probe_only(struct device_node *node) >> +{ >> + return of_property_read_bool(node, "linux,pci-probe-only"); >> +} >> +EXPORT_SYMBOL_GPL(of_pci_bridge_check_probe_only); > Why does this need to be exported for modules and exposed via > include/linux/pci.h? On a second through, I think it is not required to be exported. I'll address this also in the next patch. >> +static void pci_check_config_preserve(struct pci_host_bridge *host_bridge) >> +{ >> + if (&host_bridge->dev) { > Checking &host_bridge->dev doesn't seem like the right way to > determine whether this is an ACPI host bridge. Honestly, I couldn't find a clear way to differentiate between an ACPI based host bridge and a DT based host bridge. Hence, the current code tries to get the information using both ways and since a system can only be either ACPI or DT based, but one at a time, preserve_config will be set only once (assuming the system wants it to be set). Let me know if there is a better approach for this? I was looking at the way 'external_facing' gets set in both the boot flows and I see that there is no common place for it and the respective flows have their functions separately i.e. pci_acpi_set_external_facing() for ACPI and pci_set_bus_of_node() for DT. Thanks, Vidya Sagar >> + union acpi_object *obj; >> + >> + /* >> + * Evaluate the "PCI Boot Configuration" _DSM Function. If it >> + * exists and returns 0, we must preserve any PCI resource >> + * assignments made by firmware for this host bridge. >> + */ >> + obj = acpi_evaluate_dsm(ACPI_HANDLE(&host_bridge->dev), &pci_acpi_dsm_guid, 1, >> + DSM_PCI_PRESERVE_BOOT_CONFIG, NULL); >> + if (obj && obj->type == ACPI_TYPE_INTEGER && obj->integer.value == 0) >> + host_bridge->preserve_config = 1; >> + ACPI_FREE(obj); >> + } ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH V3] PCI: Add support for preserving boot configuration @ 2024-02-22 21:18 ` Vidya Sagar 0 siblings, 0 replies; 58+ messages in thread From: Vidya Sagar @ 2024-02-22 21:18 UTC (permalink / raw) To: Bjorn Helgaas Cc: bhelgaas, rafael, lenb, will, lpieralisi, kw, robh, frowand.list, linux-pci, linux-acpi, linux-kernel, linux-arm-kernel, devicetree, treding, jonathanh, kthota, mmaddireddy, sagar.tv On 22-02-2024 22:36, Bjorn Helgaas wrote: > External email: Use caution opening links or attachments > > > On Thu, Feb 22, 2024 at 06:11:10PM +0530, Vidya Sagar wrote: >> Add support for preserving the boot configuration done by the >> platform firmware per host bridge basis, based on the presence of >> 'linux,pci-probe-only' property in the respective PCIe host bridge >> device-tree node. It also unifies the ACPI and DT based boot flows >> in this regard. >> +/** >> + * of_pci_bridge_check_probe_only - Return true if the boot configuration >> + * needs to be preserved > I don't like the "check_probe_only" name because it's a boolean > function but the name doesn't tell me what a true/false return value > means. Something like "preserve_resources" would be better. If you > want "probe_only", even removing the "check" would help. I'll change it in the next patch. >> + * @node: Device tree node with the domain information. >> + * >> + * This function looks for "linux,pci-probe-only" property for a given >> + * PCIe controller's node and returns true if found. Having this property >> + * for a PCIe controller ensures that the kernel doesn't re-enumerate and >> + * reconfigure the BAR resources that are already done by the platform firmware. > This is generic PCI, not PCIe-specific (also in commit log and comment > below). > > I think "enumeration" specifically refers to discovering what devices > are present, and the kernel always does that, so drop that part. > Reconfiguring BARs and bridge windows is what we want to prevent. Agree and I'll address it in the next patch. >> + * NOTE: The scope of "linux,pci-probe-only" defined within a PCIe bridge device >> + * is limited to the hierarchy under that particular bridge device. whereas >> + * the scope of "linux,pci-probe-only" defined within chosen node is >> + * system wide. >> + * >> + * Return: true if the property exists false otherwise. >> + */ >> +bool of_pci_bridge_check_probe_only(struct device_node *node) >> +{ >> + return of_property_read_bool(node, "linux,pci-probe-only"); >> +} >> +EXPORT_SYMBOL_GPL(of_pci_bridge_check_probe_only); > Why does this need to be exported for modules and exposed via > include/linux/pci.h? On a second through, I think it is not required to be exported. I'll address this also in the next patch. >> +static void pci_check_config_preserve(struct pci_host_bridge *host_bridge) >> +{ >> + if (&host_bridge->dev) { > Checking &host_bridge->dev doesn't seem like the right way to > determine whether this is an ACPI host bridge. Honestly, I couldn't find a clear way to differentiate between an ACPI based host bridge and a DT based host bridge. Hence, the current code tries to get the information using both ways and since a system can only be either ACPI or DT based, but one at a time, preserve_config will be set only once (assuming the system wants it to be set). Let me know if there is a better approach for this? I was looking at the way 'external_facing' gets set in both the boot flows and I see that there is no common place for it and the respective flows have their functions separately i.e. pci_acpi_set_external_facing() for ACPI and pci_set_bus_of_node() for DT. Thanks, Vidya Sagar >> + union acpi_object *obj; >> + >> + /* >> + * Evaluate the "PCI Boot Configuration" _DSM Function. If it >> + * exists and returns 0, we must preserve any PCI resource >> + * assignments made by firmware for this host bridge. >> + */ >> + obj = acpi_evaluate_dsm(ACPI_HANDLE(&host_bridge->dev), &pci_acpi_dsm_guid, 1, >> + DSM_PCI_PRESERVE_BOOT_CONFIG, NULL); >> + if (obj && obj->type == ACPI_TYPE_INTEGER && obj->integer.value == 0) >> + host_bridge->preserve_config = 1; >> + ACPI_FREE(obj); >> + } _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH V3] PCI: Add support for preserving boot configuration 2024-02-22 21:18 ` Vidya Sagar @ 2024-02-22 22:08 ` Bjorn Helgaas -1 siblings, 0 replies; 58+ messages in thread From: Bjorn Helgaas @ 2024-02-22 22:08 UTC (permalink / raw) To: Vidya Sagar Cc: bhelgaas, rafael, lenb, will, lpieralisi, kw, robh, frowand.list, linux-pci, linux-acpi, linux-kernel, linux-arm-kernel, devicetree, treding, jonathanh, kthota, mmaddireddy, sagar.tv On Fri, Feb 23, 2024 at 02:48:24AM +0530, Vidya Sagar wrote: > On 22-02-2024 22:36, Bjorn Helgaas wrote: > > On Thu, Feb 22, 2024 at 06:11:10PM +0530, Vidya Sagar wrote: > > > + if (&host_bridge->dev) { > > Checking &host_bridge->dev doesn't seem like the right way to > > determine whether this is an ACPI host bridge. BTW, I think this condition is *always* true, since it's testing the address of a member of a struct. > Honestly, I couldn't find a clear way to differentiate between an > ACPI based host bridge and a DT based host bridge. Hence, the > current code tries to get the information using both ways and since > a system can only be either ACPI or DT based, but one at a time, > preserve_config will be set only once (assuming the system wants it > to be set). Let me know if there is a better approach for this? I'm not sure ACPI and DT will always be mutually exclusive; I think we're headed toward some combinations, e.g., https://lore.kernel.org/linux-pci/1692120000-46900-1-git-send-email-lizhi.hou@amd.com/ But I think "if (ACPI_HANDLE(&host_bridge->dev))" would work. Bjorn ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH V3] PCI: Add support for preserving boot configuration @ 2024-02-22 22:08 ` Bjorn Helgaas 0 siblings, 0 replies; 58+ messages in thread From: Bjorn Helgaas @ 2024-02-22 22:08 UTC (permalink / raw) To: Vidya Sagar Cc: bhelgaas, rafael, lenb, will, lpieralisi, kw, robh, frowand.list, linux-pci, linux-acpi, linux-kernel, linux-arm-kernel, devicetree, treding, jonathanh, kthota, mmaddireddy, sagar.tv On Fri, Feb 23, 2024 at 02:48:24AM +0530, Vidya Sagar wrote: > On 22-02-2024 22:36, Bjorn Helgaas wrote: > > On Thu, Feb 22, 2024 at 06:11:10PM +0530, Vidya Sagar wrote: > > > + if (&host_bridge->dev) { > > Checking &host_bridge->dev doesn't seem like the right way to > > determine whether this is an ACPI host bridge. BTW, I think this condition is *always* true, since it's testing the address of a member of a struct. > Honestly, I couldn't find a clear way to differentiate between an > ACPI based host bridge and a DT based host bridge. Hence, the > current code tries to get the information using both ways and since > a system can only be either ACPI or DT based, but one at a time, > preserve_config will be set only once (assuming the system wants it > to be set). Let me know if there is a better approach for this? I'm not sure ACPI and DT will always be mutually exclusive; I think we're headed toward some combinations, e.g., https://lore.kernel.org/linux-pci/1692120000-46900-1-git-send-email-lizhi.hou@amd.com/ But I think "if (ACPI_HANDLE(&host_bridge->dev))" would work. Bjorn _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 58+ messages in thread
* [PATCH V4] PCI: Add support for preserving boot configuration 2024-02-22 12:41 ` Vidya Sagar @ 2024-02-23 8:00 ` Vidya Sagar -1 siblings, 0 replies; 58+ messages in thread From: Vidya Sagar @ 2024-02-23 8:00 UTC (permalink / raw) To: bhelgaas, rafael, lenb, will, lpieralisi, kw, robh, frowand.list Cc: linux-pci, linux-acpi, linux-kernel, linux-arm-kernel, devicetree, treding, jonathanh, kthota, mmaddireddy, vidyas, sagar.tv Add support for preserving the boot configuration done by the platform firmware per host bridge basis, based on the presence of 'linux,pci-probe-only' property in the respective PCI host bridge device-tree node. It also unifies the ACPI and DT based boot flows in this regard. Signed-off-by: Vidya Sagar <vidyas@nvidia.com> --- V4: * Addressed Bjorn's review comments V3: * Unified ACPI and DT flows as part of addressing Bjorn's review comments V2: * Addressed issues reported by kernel test robot <lkp@intel.com> drivers/acpi/pci_root.c | 12 ------- drivers/pci/controller/pci-host-common.c | 4 --- drivers/pci/of.c | 21 +++++++++++ drivers/pci/probe.c | 46 ++++++++++++++++++------ include/linux/of_pci.h | 6 ++++ 5 files changed, 62 insertions(+), 27 deletions(-) diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c index 84030804a763..ddc2b3e89111 100644 --- a/drivers/acpi/pci_root.c +++ b/drivers/acpi/pci_root.c @@ -1008,7 +1008,6 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root, int node = acpi_get_node(device->handle); struct pci_bus *bus; struct pci_host_bridge *host_bridge; - union acpi_object *obj; info->root = root; info->bridge = device; @@ -1050,17 +1049,6 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root, if (!(root->osc_ext_control_set & OSC_CXL_ERROR_REPORTING_CONTROL)) host_bridge->native_cxl_error = 0; - /* - * Evaluate the "PCI Boot Configuration" _DSM Function. If it - * exists and returns 0, we must preserve any PCI resource - * assignments made by firmware for this host bridge. - */ - obj = acpi_evaluate_dsm(ACPI_HANDLE(bus->bridge), &pci_acpi_dsm_guid, 1, - DSM_PCI_PRESERVE_BOOT_CONFIG, NULL); - if (obj && obj->type == ACPI_TYPE_INTEGER && obj->integer.value == 0) - host_bridge->preserve_config = 1; - ACPI_FREE(obj); - acpi_dev_power_up_children_with_adr(device); pci_scan_child_bus(bus); diff --git a/drivers/pci/controller/pci-host-common.c b/drivers/pci/controller/pci-host-common.c index 6be3266cd7b5..e2602e38ae45 100644 --- a/drivers/pci/controller/pci-host-common.c +++ b/drivers/pci/controller/pci-host-common.c @@ -73,10 +73,6 @@ int pci_host_common_probe(struct platform_device *pdev) if (IS_ERR(cfg)) return PTR_ERR(cfg); - /* Do not reassign resources if probe only */ - if (!pci_has_flag(PCI_PROBE_ONLY)) - pci_add_flags(PCI_REASSIGN_ALL_BUS); - bridge->sysdata = cfg; bridge->ops = (struct pci_ops *)&ops->pci_ops; bridge->msi_domain = true; diff --git a/drivers/pci/of.c b/drivers/pci/of.c index 51e3dd0ea5ab..f0f1156040a5 100644 --- a/drivers/pci/of.c +++ b/drivers/pci/of.c @@ -258,6 +258,27 @@ void of_pci_check_probe_only(void) } EXPORT_SYMBOL_GPL(of_pci_check_probe_only); +/** + * of_pci_bridge_preserve_resources - Return true if the boot configuration + * needs to be preserved + * @node: Device tree node with the domain information. + * + * This function looks for "linux,pci-probe-only" property for a given + * PCI controller's node and returns true if found. Having this property + * for a PCI controller ensures that the kernel doesn't reconfigure the + * BARs and bridge windows that are already done by the platform firmware. + * NOTE: The scope of "linux,pci-probe-only" defined within a PCI bridge device + * is limited to the hierarchy under that particular bridge device. whereas + * the scope of "linux,pci-probe-only" defined within chosen node is + * system wide. + * + * Return: true if the property exists false otherwise. + */ +bool of_pci_bridge_preserve_resources(struct device_node *node) +{ + return of_property_read_bool(node, "linux,pci-probe-only"); +} + /** * devm_of_pci_get_host_bridge_resources() - Resource-managed parsing of PCI * host bridge resources from DT diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 795534589b98..c57648084503 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -15,6 +15,7 @@ #include <linux/cpumask.h> #include <linux/aer.h> #include <linux/acpi.h> +#include <linux/pci-acpi.h> #include <linux/hypervisor.h> #include <linux/irqdomain.h> #include <linux/pm_runtime.h> @@ -877,6 +878,28 @@ static void pci_set_bus_msi_domain(struct pci_bus *bus) dev_set_msi_domain(&bus->dev, d); } +static void pci_check_config_preserve(struct pci_host_bridge *host_bridge) +{ + if (ACPI_HANDLE(&host_bridge->dev)) { + union acpi_object *obj; + + /* + * Evaluate the "PCI Boot Configuration" _DSM Function. If it + * exists and returns 0, we must preserve any PCI resource + * assignments made by firmware for this host bridge. + */ + obj = acpi_evaluate_dsm(ACPI_HANDLE(&host_bridge->dev), &pci_acpi_dsm_guid, 1, + DSM_PCI_PRESERVE_BOOT_CONFIG, NULL); + if (obj && obj->type == ACPI_TYPE_INTEGER && obj->integer.value == 0) + host_bridge->preserve_config = 1; + ACPI_FREE(obj); + } + + if (host_bridge->dev.parent && host_bridge->dev.parent->of_node) + host_bridge->preserve_config = + of_pci_bridge_preserve_resources(host_bridge->dev.parent->of_node); +} + static int pci_register_host_bridge(struct pci_host_bridge *bridge) { struct device *parent = bridge->dev.parent; @@ -971,6 +994,9 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge) if (nr_node_ids > 1 && pcibus_to_node(bus) == NUMA_NO_NODE) dev_warn(&bus->dev, "Unknown NUMA node; performance will be reduced\n"); + /* Check if the boot configuration by FW needs to be preserved */ + pci_check_config_preserve(bridge); + /* Coalesce contiguous windows */ resource_list_for_each_entry_safe(window, n, &resources) { if (list_is_last(&window->node, &resources)) @@ -3080,20 +3106,18 @@ int pci_host_probe(struct pci_host_bridge *bridge) bus = bridge->bus; + /* If we must preserve the resource configuration, claim now */ + if (pci_has_flag(PCI_PROBE_ONLY) || bridge->preserve_config) + pci_bus_claim_resources(bus); + /* - * We insert PCI resources into the iomem_resource and - * ioport_resource trees in either pci_bus_claim_resources() - * or pci_bus_assign_resources(). + * Assign whatever was left unassigned. If we didn't claim above, + * this will reassign everything. */ - if (pci_has_flag(PCI_PROBE_ONLY)) { - pci_bus_claim_resources(bus); - } else { - pci_bus_size_bridges(bus); - pci_bus_assign_resources(bus); + pci_assign_unassigned_root_bus_resources(bus); - list_for_each_entry(child, &bus->children, node) - pcie_bus_configure_settings(child); - } + list_for_each_entry(child, &bus->children, node) + pcie_bus_configure_settings(child); pci_bus_add_devices(bus); return 0; diff --git a/include/linux/of_pci.h b/include/linux/of_pci.h index 29658c0ee71f..3f3909a5d55d 100644 --- a/include/linux/of_pci.h +++ b/include/linux/of_pci.h @@ -13,6 +13,7 @@ struct device_node *of_pci_find_child_device(struct device_node *parent, unsigned int devfn); int of_pci_get_devfn(struct device_node *np); void of_pci_check_probe_only(void); +bool of_pci_bridge_preserve_resources(struct device_node *node); #else static inline struct device_node *of_pci_find_child_device(struct device_node *parent, unsigned int devfn) @@ -26,6 +27,11 @@ static inline int of_pci_get_devfn(struct device_node *np) } static inline void of_pci_check_probe_only(void) { } + +static inline bool of_pci_bridge_preserve_resources(struct device_node *node) +{ + return false; +} #endif #if IS_ENABLED(CONFIG_OF_IRQ) -- 2.25.1 ^ permalink raw reply related [flat|nested] 58+ messages in thread
* [PATCH V4] PCI: Add support for preserving boot configuration @ 2024-02-23 8:00 ` Vidya Sagar 0 siblings, 0 replies; 58+ messages in thread From: Vidya Sagar @ 2024-02-23 8:00 UTC (permalink / raw) To: bhelgaas, rafael, lenb, will, lpieralisi, kw, robh, frowand.list Cc: linux-pci, linux-acpi, linux-kernel, linux-arm-kernel, devicetree, treding, jonathanh, kthota, mmaddireddy, vidyas, sagar.tv Add support for preserving the boot configuration done by the platform firmware per host bridge basis, based on the presence of 'linux,pci-probe-only' property in the respective PCI host bridge device-tree node. It also unifies the ACPI and DT based boot flows in this regard. Signed-off-by: Vidya Sagar <vidyas@nvidia.com> --- V4: * Addressed Bjorn's review comments V3: * Unified ACPI and DT flows as part of addressing Bjorn's review comments V2: * Addressed issues reported by kernel test robot <lkp@intel.com> drivers/acpi/pci_root.c | 12 ------- drivers/pci/controller/pci-host-common.c | 4 --- drivers/pci/of.c | 21 +++++++++++ drivers/pci/probe.c | 46 ++++++++++++++++++------ include/linux/of_pci.h | 6 ++++ 5 files changed, 62 insertions(+), 27 deletions(-) diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c index 84030804a763..ddc2b3e89111 100644 --- a/drivers/acpi/pci_root.c +++ b/drivers/acpi/pci_root.c @@ -1008,7 +1008,6 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root, int node = acpi_get_node(device->handle); struct pci_bus *bus; struct pci_host_bridge *host_bridge; - union acpi_object *obj; info->root = root; info->bridge = device; @@ -1050,17 +1049,6 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root, if (!(root->osc_ext_control_set & OSC_CXL_ERROR_REPORTING_CONTROL)) host_bridge->native_cxl_error = 0; - /* - * Evaluate the "PCI Boot Configuration" _DSM Function. If it - * exists and returns 0, we must preserve any PCI resource - * assignments made by firmware for this host bridge. - */ - obj = acpi_evaluate_dsm(ACPI_HANDLE(bus->bridge), &pci_acpi_dsm_guid, 1, - DSM_PCI_PRESERVE_BOOT_CONFIG, NULL); - if (obj && obj->type == ACPI_TYPE_INTEGER && obj->integer.value == 0) - host_bridge->preserve_config = 1; - ACPI_FREE(obj); - acpi_dev_power_up_children_with_adr(device); pci_scan_child_bus(bus); diff --git a/drivers/pci/controller/pci-host-common.c b/drivers/pci/controller/pci-host-common.c index 6be3266cd7b5..e2602e38ae45 100644 --- a/drivers/pci/controller/pci-host-common.c +++ b/drivers/pci/controller/pci-host-common.c @@ -73,10 +73,6 @@ int pci_host_common_probe(struct platform_device *pdev) if (IS_ERR(cfg)) return PTR_ERR(cfg); - /* Do not reassign resources if probe only */ - if (!pci_has_flag(PCI_PROBE_ONLY)) - pci_add_flags(PCI_REASSIGN_ALL_BUS); - bridge->sysdata = cfg; bridge->ops = (struct pci_ops *)&ops->pci_ops; bridge->msi_domain = true; diff --git a/drivers/pci/of.c b/drivers/pci/of.c index 51e3dd0ea5ab..f0f1156040a5 100644 --- a/drivers/pci/of.c +++ b/drivers/pci/of.c @@ -258,6 +258,27 @@ void of_pci_check_probe_only(void) } EXPORT_SYMBOL_GPL(of_pci_check_probe_only); +/** + * of_pci_bridge_preserve_resources - Return true if the boot configuration + * needs to be preserved + * @node: Device tree node with the domain information. + * + * This function looks for "linux,pci-probe-only" property for a given + * PCI controller's node and returns true if found. Having this property + * for a PCI controller ensures that the kernel doesn't reconfigure the + * BARs and bridge windows that are already done by the platform firmware. + * NOTE: The scope of "linux,pci-probe-only" defined within a PCI bridge device + * is limited to the hierarchy under that particular bridge device. whereas + * the scope of "linux,pci-probe-only" defined within chosen node is + * system wide. + * + * Return: true if the property exists false otherwise. + */ +bool of_pci_bridge_preserve_resources(struct device_node *node) +{ + return of_property_read_bool(node, "linux,pci-probe-only"); +} + /** * devm_of_pci_get_host_bridge_resources() - Resource-managed parsing of PCI * host bridge resources from DT diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 795534589b98..c57648084503 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -15,6 +15,7 @@ #include <linux/cpumask.h> #include <linux/aer.h> #include <linux/acpi.h> +#include <linux/pci-acpi.h> #include <linux/hypervisor.h> #include <linux/irqdomain.h> #include <linux/pm_runtime.h> @@ -877,6 +878,28 @@ static void pci_set_bus_msi_domain(struct pci_bus *bus) dev_set_msi_domain(&bus->dev, d); } +static void pci_check_config_preserve(struct pci_host_bridge *host_bridge) +{ + if (ACPI_HANDLE(&host_bridge->dev)) { + union acpi_object *obj; + + /* + * Evaluate the "PCI Boot Configuration" _DSM Function. If it + * exists and returns 0, we must preserve any PCI resource + * assignments made by firmware for this host bridge. + */ + obj = acpi_evaluate_dsm(ACPI_HANDLE(&host_bridge->dev), &pci_acpi_dsm_guid, 1, + DSM_PCI_PRESERVE_BOOT_CONFIG, NULL); + if (obj && obj->type == ACPI_TYPE_INTEGER && obj->integer.value == 0) + host_bridge->preserve_config = 1; + ACPI_FREE(obj); + } + + if (host_bridge->dev.parent && host_bridge->dev.parent->of_node) + host_bridge->preserve_config = + of_pci_bridge_preserve_resources(host_bridge->dev.parent->of_node); +} + static int pci_register_host_bridge(struct pci_host_bridge *bridge) { struct device *parent = bridge->dev.parent; @@ -971,6 +994,9 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge) if (nr_node_ids > 1 && pcibus_to_node(bus) == NUMA_NO_NODE) dev_warn(&bus->dev, "Unknown NUMA node; performance will be reduced\n"); + /* Check if the boot configuration by FW needs to be preserved */ + pci_check_config_preserve(bridge); + /* Coalesce contiguous windows */ resource_list_for_each_entry_safe(window, n, &resources) { if (list_is_last(&window->node, &resources)) @@ -3080,20 +3106,18 @@ int pci_host_probe(struct pci_host_bridge *bridge) bus = bridge->bus; + /* If we must preserve the resource configuration, claim now */ + if (pci_has_flag(PCI_PROBE_ONLY) || bridge->preserve_config) + pci_bus_claim_resources(bus); + /* - * We insert PCI resources into the iomem_resource and - * ioport_resource trees in either pci_bus_claim_resources() - * or pci_bus_assign_resources(). + * Assign whatever was left unassigned. If we didn't claim above, + * this will reassign everything. */ - if (pci_has_flag(PCI_PROBE_ONLY)) { - pci_bus_claim_resources(bus); - } else { - pci_bus_size_bridges(bus); - pci_bus_assign_resources(bus); + pci_assign_unassigned_root_bus_resources(bus); - list_for_each_entry(child, &bus->children, node) - pcie_bus_configure_settings(child); - } + list_for_each_entry(child, &bus->children, node) + pcie_bus_configure_settings(child); pci_bus_add_devices(bus); return 0; diff --git a/include/linux/of_pci.h b/include/linux/of_pci.h index 29658c0ee71f..3f3909a5d55d 100644 --- a/include/linux/of_pci.h +++ b/include/linux/of_pci.h @@ -13,6 +13,7 @@ struct device_node *of_pci_find_child_device(struct device_node *parent, unsigned int devfn); int of_pci_get_devfn(struct device_node *np); void of_pci_check_probe_only(void); +bool of_pci_bridge_preserve_resources(struct device_node *node); #else static inline struct device_node *of_pci_find_child_device(struct device_node *parent, unsigned int devfn) @@ -26,6 +27,11 @@ static inline int of_pci_get_devfn(struct device_node *np) } static inline void of_pci_check_probe_only(void) { } + +static inline bool of_pci_bridge_preserve_resources(struct device_node *node) +{ + return false; +} #endif #if IS_ENABLED(CONFIG_OF_IRQ) -- 2.25.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 58+ messages in thread
* Re: [PATCH V4] PCI: Add support for preserving boot configuration 2024-02-23 8:00 ` Vidya Sagar @ 2024-03-05 14:10 ` Rob Herring -1 siblings, 0 replies; 58+ messages in thread From: Rob Herring @ 2024-03-05 14:10 UTC (permalink / raw) To: Vidya Sagar Cc: bhelgaas, rafael, lenb, will, lpieralisi, kw, frowand.list, linux-pci, linux-acpi, linux-kernel, linux-arm-kernel, devicetree, treding, jonathanh, kthota, mmaddireddy, sagar.tv On Fri, Feb 23, 2024 at 01:30:21PM +0530, Vidya Sagar wrote: > Add support for preserving the boot configuration done by the > platform firmware per host bridge basis, based on the presence of > 'linux,pci-probe-only' property in the respective PCI host bridge > device-tree node. It also unifies the ACPI and DT based boot flows > in this regard. > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > --- > V4: > * Addressed Bjorn's review comments > > V3: > * Unified ACPI and DT flows as part of addressing Bjorn's review comments > > V2: > * Addressed issues reported by kernel test robot <lkp@intel.com> > > drivers/acpi/pci_root.c | 12 ------- > drivers/pci/controller/pci-host-common.c | 4 --- > drivers/pci/of.c | 21 +++++++++++ > drivers/pci/probe.c | 46 ++++++++++++++++++------ > include/linux/of_pci.h | 6 ++++ > 5 files changed, 62 insertions(+), 27 deletions(-) > > diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c > index 84030804a763..ddc2b3e89111 100644 > --- a/drivers/acpi/pci_root.c > +++ b/drivers/acpi/pci_root.c > @@ -1008,7 +1008,6 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root, > int node = acpi_get_node(device->handle); > struct pci_bus *bus; > struct pci_host_bridge *host_bridge; > - union acpi_object *obj; > > info->root = root; > info->bridge = device; > @@ -1050,17 +1049,6 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root, > if (!(root->osc_ext_control_set & OSC_CXL_ERROR_REPORTING_CONTROL)) > host_bridge->native_cxl_error = 0; > > - /* > - * Evaluate the "PCI Boot Configuration" _DSM Function. If it > - * exists and returns 0, we must preserve any PCI resource > - * assignments made by firmware for this host bridge. > - */ > - obj = acpi_evaluate_dsm(ACPI_HANDLE(bus->bridge), &pci_acpi_dsm_guid, 1, > - DSM_PCI_PRESERVE_BOOT_CONFIG, NULL); > - if (obj && obj->type == ACPI_TYPE_INTEGER && obj->integer.value == 0) > - host_bridge->preserve_config = 1; > - ACPI_FREE(obj); > - > acpi_dev_power_up_children_with_adr(device); > > pci_scan_child_bus(bus); > diff --git a/drivers/pci/controller/pci-host-common.c b/drivers/pci/controller/pci-host-common.c > index 6be3266cd7b5..e2602e38ae45 100644 > --- a/drivers/pci/controller/pci-host-common.c > +++ b/drivers/pci/controller/pci-host-common.c > @@ -73,10 +73,6 @@ int pci_host_common_probe(struct platform_device *pdev) > if (IS_ERR(cfg)) > return PTR_ERR(cfg); > > - /* Do not reassign resources if probe only */ > - if (!pci_has_flag(PCI_PROBE_ONLY)) > - pci_add_flags(PCI_REASSIGN_ALL_BUS); > - > bridge->sysdata = cfg; > bridge->ops = (struct pci_ops *)&ops->pci_ops; > bridge->msi_domain = true; > diff --git a/drivers/pci/of.c b/drivers/pci/of.c > index 51e3dd0ea5ab..f0f1156040a5 100644 > --- a/drivers/pci/of.c > +++ b/drivers/pci/of.c > @@ -258,6 +258,27 @@ void of_pci_check_probe_only(void) > } > EXPORT_SYMBOL_GPL(of_pci_check_probe_only); > > +/** > + * of_pci_bridge_preserve_resources - Return true if the boot configuration > + * needs to be preserved > + * @node: Device tree node with the domain information. > + * > + * This function looks for "linux,pci-probe-only" property for a given > + * PCI controller's node and returns true if found. Having this property > + * for a PCI controller ensures that the kernel doesn't reconfigure the > + * BARs and bridge windows that are already done by the platform firmware. > + * NOTE: The scope of "linux,pci-probe-only" defined within a PCI bridge device > + * is limited to the hierarchy under that particular bridge device. whereas > + * the scope of "linux,pci-probe-only" defined within chosen node is > + * system wide. > + * > + * Return: true if the property exists false otherwise. > + */ > +bool of_pci_bridge_preserve_resources(struct device_node *node) > +{ > + return of_property_read_bool(node, "linux,pci-probe-only"); This is the wrong type. The existing "linux,pci-probe-only" is a u32 and non-zero value means probe-only. This would return true for 'linux,pci-probe-only = <0>'. Also, this should also check chosen. If you make this work accepting NULL for node, then of_pci_check_probe_only() can be re-implemented to use it. Rob ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH V4] PCI: Add support for preserving boot configuration @ 2024-03-05 14:10 ` Rob Herring 0 siblings, 0 replies; 58+ messages in thread From: Rob Herring @ 2024-03-05 14:10 UTC (permalink / raw) To: Vidya Sagar Cc: bhelgaas, rafael, lenb, will, lpieralisi, kw, frowand.list, linux-pci, linux-acpi, linux-kernel, linux-arm-kernel, devicetree, treding, jonathanh, kthota, mmaddireddy, sagar.tv On Fri, Feb 23, 2024 at 01:30:21PM +0530, Vidya Sagar wrote: > Add support for preserving the boot configuration done by the > platform firmware per host bridge basis, based on the presence of > 'linux,pci-probe-only' property in the respective PCI host bridge > device-tree node. It also unifies the ACPI and DT based boot flows > in this regard. > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > --- > V4: > * Addressed Bjorn's review comments > > V3: > * Unified ACPI and DT flows as part of addressing Bjorn's review comments > > V2: > * Addressed issues reported by kernel test robot <lkp@intel.com> > > drivers/acpi/pci_root.c | 12 ------- > drivers/pci/controller/pci-host-common.c | 4 --- > drivers/pci/of.c | 21 +++++++++++ > drivers/pci/probe.c | 46 ++++++++++++++++++------ > include/linux/of_pci.h | 6 ++++ > 5 files changed, 62 insertions(+), 27 deletions(-) > > diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c > index 84030804a763..ddc2b3e89111 100644 > --- a/drivers/acpi/pci_root.c > +++ b/drivers/acpi/pci_root.c > @@ -1008,7 +1008,6 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root, > int node = acpi_get_node(device->handle); > struct pci_bus *bus; > struct pci_host_bridge *host_bridge; > - union acpi_object *obj; > > info->root = root; > info->bridge = device; > @@ -1050,17 +1049,6 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root, > if (!(root->osc_ext_control_set & OSC_CXL_ERROR_REPORTING_CONTROL)) > host_bridge->native_cxl_error = 0; > > - /* > - * Evaluate the "PCI Boot Configuration" _DSM Function. If it > - * exists and returns 0, we must preserve any PCI resource > - * assignments made by firmware for this host bridge. > - */ > - obj = acpi_evaluate_dsm(ACPI_HANDLE(bus->bridge), &pci_acpi_dsm_guid, 1, > - DSM_PCI_PRESERVE_BOOT_CONFIG, NULL); > - if (obj && obj->type == ACPI_TYPE_INTEGER && obj->integer.value == 0) > - host_bridge->preserve_config = 1; > - ACPI_FREE(obj); > - > acpi_dev_power_up_children_with_adr(device); > > pci_scan_child_bus(bus); > diff --git a/drivers/pci/controller/pci-host-common.c b/drivers/pci/controller/pci-host-common.c > index 6be3266cd7b5..e2602e38ae45 100644 > --- a/drivers/pci/controller/pci-host-common.c > +++ b/drivers/pci/controller/pci-host-common.c > @@ -73,10 +73,6 @@ int pci_host_common_probe(struct platform_device *pdev) > if (IS_ERR(cfg)) > return PTR_ERR(cfg); > > - /* Do not reassign resources if probe only */ > - if (!pci_has_flag(PCI_PROBE_ONLY)) > - pci_add_flags(PCI_REASSIGN_ALL_BUS); > - > bridge->sysdata = cfg; > bridge->ops = (struct pci_ops *)&ops->pci_ops; > bridge->msi_domain = true; > diff --git a/drivers/pci/of.c b/drivers/pci/of.c > index 51e3dd0ea5ab..f0f1156040a5 100644 > --- a/drivers/pci/of.c > +++ b/drivers/pci/of.c > @@ -258,6 +258,27 @@ void of_pci_check_probe_only(void) > } > EXPORT_SYMBOL_GPL(of_pci_check_probe_only); > > +/** > + * of_pci_bridge_preserve_resources - Return true if the boot configuration > + * needs to be preserved > + * @node: Device tree node with the domain information. > + * > + * This function looks for "linux,pci-probe-only" property for a given > + * PCI controller's node and returns true if found. Having this property > + * for a PCI controller ensures that the kernel doesn't reconfigure the > + * BARs and bridge windows that are already done by the platform firmware. > + * NOTE: The scope of "linux,pci-probe-only" defined within a PCI bridge device > + * is limited to the hierarchy under that particular bridge device. whereas > + * the scope of "linux,pci-probe-only" defined within chosen node is > + * system wide. > + * > + * Return: true if the property exists false otherwise. > + */ > +bool of_pci_bridge_preserve_resources(struct device_node *node) > +{ > + return of_property_read_bool(node, "linux,pci-probe-only"); This is the wrong type. The existing "linux,pci-probe-only" is a u32 and non-zero value means probe-only. This would return true for 'linux,pci-probe-only = <0>'. Also, this should also check chosen. If you make this work accepting NULL for node, then of_pci_check_probe_only() can be re-implemented to use it. Rob _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH V4] PCI: Add support for preserving boot configuration 2024-02-23 8:00 ` Vidya Sagar @ 2024-03-05 14:24 ` Rob Herring -1 siblings, 0 replies; 58+ messages in thread From: Rob Herring @ 2024-03-05 14:24 UTC (permalink / raw) To: Vidya Sagar Cc: bhelgaas, rafael, lenb, will, lpieralisi, kw, frowand.list, linux-pci, linux-acpi, linux-kernel, linux-arm-kernel, devicetree, treding, jonathanh, kthota, mmaddireddy, sagar.tv On Fri, Feb 23, 2024 at 01:30:21PM +0530, Vidya Sagar wrote: > Add support for preserving the boot configuration done by the > platform firmware per host bridge basis, based on the presence of > 'linux,pci-probe-only' property in the respective PCI host bridge > device-tree node. It also unifies the ACPI and DT based boot flows > in this regard. > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > --- > V4: > * Addressed Bjorn's review comments > > V3: > * Unified ACPI and DT flows as part of addressing Bjorn's review comments > > V2: > * Addressed issues reported by kernel test robot <lkp@intel.com> > > drivers/acpi/pci_root.c | 12 ------- > drivers/pci/controller/pci-host-common.c | 4 --- > drivers/pci/of.c | 21 +++++++++++ > drivers/pci/probe.c | 46 ++++++++++++++++++------ > include/linux/of_pci.h | 6 ++++ > 5 files changed, 62 insertions(+), 27 deletions(-) One more thing. > @@ -3080,20 +3106,18 @@ int pci_host_probe(struct pci_host_bridge *bridge) > > bus = bridge->bus; > > + /* If we must preserve the resource configuration, claim now */ > + if (pci_has_flag(PCI_PROBE_ONLY) || bridge->preserve_config) > + pci_bus_claim_resources(bus); No reason to check PCI_PROBE_ONLY if you set preserve_config based on /chosen as well. IOW, we should deprecate PCI_PROBE_ONLY flag in favor of the per host bridge setting. Rob ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH V4] PCI: Add support for preserving boot configuration @ 2024-03-05 14:24 ` Rob Herring 0 siblings, 0 replies; 58+ messages in thread From: Rob Herring @ 2024-03-05 14:24 UTC (permalink / raw) To: Vidya Sagar Cc: bhelgaas, rafael, lenb, will, lpieralisi, kw, frowand.list, linux-pci, linux-acpi, linux-kernel, linux-arm-kernel, devicetree, treding, jonathanh, kthota, mmaddireddy, sagar.tv On Fri, Feb 23, 2024 at 01:30:21PM +0530, Vidya Sagar wrote: > Add support for preserving the boot configuration done by the > platform firmware per host bridge basis, based on the presence of > 'linux,pci-probe-only' property in the respective PCI host bridge > device-tree node. It also unifies the ACPI and DT based boot flows > in this regard. > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > --- > V4: > * Addressed Bjorn's review comments > > V3: > * Unified ACPI and DT flows as part of addressing Bjorn's review comments > > V2: > * Addressed issues reported by kernel test robot <lkp@intel.com> > > drivers/acpi/pci_root.c | 12 ------- > drivers/pci/controller/pci-host-common.c | 4 --- > drivers/pci/of.c | 21 +++++++++++ > drivers/pci/probe.c | 46 ++++++++++++++++++------ > include/linux/of_pci.h | 6 ++++ > 5 files changed, 62 insertions(+), 27 deletions(-) One more thing. > @@ -3080,20 +3106,18 @@ int pci_host_probe(struct pci_host_bridge *bridge) > > bus = bridge->bus; > > + /* If we must preserve the resource configuration, claim now */ > + if (pci_has_flag(PCI_PROBE_ONLY) || bridge->preserve_config) > + pci_bus_claim_resources(bus); No reason to check PCI_PROBE_ONLY if you set preserve_config based on /chosen as well. IOW, we should deprecate PCI_PROBE_ONLY flag in favor of the per host bridge setting. Rob _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 58+ messages in thread
* [PATCH V5] PCI: Add support for preserving boot configuration 2024-02-23 8:00 ` Vidya Sagar @ 2024-04-01 7:50 ` Vidya Sagar -1 siblings, 0 replies; 58+ messages in thread From: Vidya Sagar @ 2024-04-01 7:50 UTC (permalink / raw) To: bhelgaas, rafael, lenb, will, lpieralisi, kw, robh, frowand.list Cc: linux-pci, linux-acpi, linux-kernel, linux-arm-kernel, devicetree, treding, jonathanh, kthota, mmaddireddy, vidyas, sagar.tv Add support for preserving the boot configuration done by the platform firmware per host bridge basis, based on the presence of 'linux,pci-probe-only' property in the respective PCI host bridge device-tree node. It also unifies the ACPI and DT based boot flows in this regard. Signed-off-by: Vidya Sagar <vidyas@nvidia.com> --- V5: * Addressed Rob's review comments V4: * Addressed Bjorn's review comments V3: * Unified ACPI and DT flows as part of addressing Bjorn's review comments V2: * Addressed issues reported by kernel test robot <lkp@intel.com> drivers/acpi/pci_root.c | 12 ----- drivers/pci/controller/pci-host-common.c | 4 -- drivers/pci/of.c | 57 +++++++++++++++++++----- drivers/pci/probe.c | 46 ++++++++++++++----- include/linux/of_pci.h | 6 +++ 5 files changed, 88 insertions(+), 37 deletions(-) diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c index 84030804a763..ddc2b3e89111 100644 --- a/drivers/acpi/pci_root.c +++ b/drivers/acpi/pci_root.c @@ -1008,7 +1008,6 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root, int node = acpi_get_node(device->handle); struct pci_bus *bus; struct pci_host_bridge *host_bridge; - union acpi_object *obj; info->root = root; info->bridge = device; @@ -1050,17 +1049,6 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root, if (!(root->osc_ext_control_set & OSC_CXL_ERROR_REPORTING_CONTROL)) host_bridge->native_cxl_error = 0; - /* - * Evaluate the "PCI Boot Configuration" _DSM Function. If it - * exists and returns 0, we must preserve any PCI resource - * assignments made by firmware for this host bridge. - */ - obj = acpi_evaluate_dsm(ACPI_HANDLE(bus->bridge), &pci_acpi_dsm_guid, 1, - DSM_PCI_PRESERVE_BOOT_CONFIG, NULL); - if (obj && obj->type == ACPI_TYPE_INTEGER && obj->integer.value == 0) - host_bridge->preserve_config = 1; - ACPI_FREE(obj); - acpi_dev_power_up_children_with_adr(device); pci_scan_child_bus(bus); diff --git a/drivers/pci/controller/pci-host-common.c b/drivers/pci/controller/pci-host-common.c index 6be3266cd7b5..e2602e38ae45 100644 --- a/drivers/pci/controller/pci-host-common.c +++ b/drivers/pci/controller/pci-host-common.c @@ -73,10 +73,6 @@ int pci_host_common_probe(struct platform_device *pdev) if (IS_ERR(cfg)) return PTR_ERR(cfg); - /* Do not reassign resources if probe only */ - if (!pci_has_flag(PCI_PROBE_ONLY)) - pci_add_flags(PCI_REASSIGN_ALL_BUS); - bridge->sysdata = cfg; bridge->ops = (struct pci_ops *)&ops->pci_ops; bridge->msi_domain = true; diff --git a/drivers/pci/of.c b/drivers/pci/of.c index 51e3dd0ea5ab..e6da3654f9ac 100644 --- a/drivers/pci/of.c +++ b/drivers/pci/of.c @@ -239,24 +239,61 @@ EXPORT_SYMBOL_GPL(of_get_pci_domain_nr); */ void of_pci_check_probe_only(void) { - u32 val; + bool is_preserve_config = of_pci_bridge_preserve_resources(of_chosen); + + if (is_preserve_config) + pci_add_flags(PCI_PROBE_ONLY); + else + pci_clear_flags(PCI_PROBE_ONLY); + + pr_info("PROBE_ONLY %s\n", is_preserve_config ? "enabled" : "disabled"); +} +EXPORT_SYMBOL_GPL(of_pci_check_probe_only); + +/** + * of_pci_bridge_preserve_resources - Return true if the boot configuration + * needs to be preserved + * @node: Device tree node. + * + * This function looks for "linux,pci-probe-only" property for a given + * PCI controller's node and returns true if found. It will also look in the + * chosen node if the property is not found in the given controller's node. + * Having this property ensures that the kernel doesn't reconfigure the + * BARs and bridge windows that are already done by the platform firmware. + * + * Return: true if the property exists false otherwise. + */ +bool of_pci_bridge_preserve_resources(struct device_node *node) +{ + u32 val = 0; int ret; - ret = of_property_read_u32(of_chosen, "linux,pci-probe-only", &val); + if (!node) { + pr_warn("device node is NULL, trying with of_chosen\n"); + node = of_chosen; + } + +retry: + ret = of_property_read_u32(node, "linux,pci-probe-only", &val); if (ret) { - if (ret == -ENODATA || ret == -EOVERFLOW) - pr_warn("linux,pci-probe-only without valid value, ignoring\n"); - return; + if (ret == -ENODATA || ret == -EOVERFLOW) { + pr_warn("Incorrect value for linux,pci-probe-only in %pOF, ignoring\n", node); + return false; + } + if (ret == -EINVAL) { + if (node == of_chosen) + return false; + + node = of_chosen; + goto retry; + } } if (val) - pci_add_flags(PCI_PROBE_ONLY); + return true; else - pci_clear_flags(PCI_PROBE_ONLY); - - pr_info("PROBE_ONLY %s\n", val ? "enabled" : "disabled"); + return false; } -EXPORT_SYMBOL_GPL(of_pci_check_probe_only); /** * devm_of_pci_get_host_bridge_resources() - Resource-managed parsing of PCI diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 795534589b98..b0e0226a8da8 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -15,6 +15,7 @@ #include <linux/cpumask.h> #include <linux/aer.h> #include <linux/acpi.h> +#include <linux/pci-acpi.h> #include <linux/hypervisor.h> #include <linux/irqdomain.h> #include <linux/pm_runtime.h> @@ -877,6 +878,28 @@ static void pci_set_bus_msi_domain(struct pci_bus *bus) dev_set_msi_domain(&bus->dev, d); } +static void pci_check_config_preserve(struct pci_host_bridge *host_bridge) +{ + if (ACPI_HANDLE(&host_bridge->dev)) { + union acpi_object *obj; + + /* + * Evaluate the "PCI Boot Configuration" _DSM Function. If it + * exists and returns 0, we must preserve any PCI resource + * assignments made by firmware for this host bridge. + */ + obj = acpi_evaluate_dsm(ACPI_HANDLE(&host_bridge->dev), &pci_acpi_dsm_guid, 1, + DSM_PCI_PRESERVE_BOOT_CONFIG, NULL); + if (obj && obj->type == ACPI_TYPE_INTEGER && obj->integer.value == 0) + host_bridge->preserve_config = 1; + ACPI_FREE(obj); + } + + if (host_bridge->dev.parent && host_bridge->dev.parent->of_node) + host_bridge->preserve_config = + of_pci_bridge_preserve_resources(host_bridge->dev.parent->of_node); +} + static int pci_register_host_bridge(struct pci_host_bridge *bridge) { struct device *parent = bridge->dev.parent; @@ -971,6 +994,9 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge) if (nr_node_ids > 1 && pcibus_to_node(bus) == NUMA_NO_NODE) dev_warn(&bus->dev, "Unknown NUMA node; performance will be reduced\n"); + /* Check if the boot configuration by FW needs to be preserved */ + pci_check_config_preserve(bridge); + /* Coalesce contiguous windows */ resource_list_for_each_entry_safe(window, n, &resources) { if (list_is_last(&window->node, &resources)) @@ -3080,20 +3106,18 @@ int pci_host_probe(struct pci_host_bridge *bridge) bus = bridge->bus; + /* If we must preserve the resource configuration, claim now */ + if (bridge->preserve_config) + pci_bus_claim_resources(bus); + /* - * We insert PCI resources into the iomem_resource and - * ioport_resource trees in either pci_bus_claim_resources() - * or pci_bus_assign_resources(). + * Assign whatever was left unassigned. If we didn't claim above, + * this will reassign everything. */ - if (pci_has_flag(PCI_PROBE_ONLY)) { - pci_bus_claim_resources(bus); - } else { - pci_bus_size_bridges(bus); - pci_bus_assign_resources(bus); + pci_assign_unassigned_root_bus_resources(bus); - list_for_each_entry(child, &bus->children, node) - pcie_bus_configure_settings(child); - } + list_for_each_entry(child, &bus->children, node) + pcie_bus_configure_settings(child); pci_bus_add_devices(bus); return 0; diff --git a/include/linux/of_pci.h b/include/linux/of_pci.h index 29658c0ee71f..3f3909a5d55d 100644 --- a/include/linux/of_pci.h +++ b/include/linux/of_pci.h @@ -13,6 +13,7 @@ struct device_node *of_pci_find_child_device(struct device_node *parent, unsigned int devfn); int of_pci_get_devfn(struct device_node *np); void of_pci_check_probe_only(void); +bool of_pci_bridge_preserve_resources(struct device_node *node); #else static inline struct device_node *of_pci_find_child_device(struct device_node *parent, unsigned int devfn) @@ -26,6 +27,11 @@ static inline int of_pci_get_devfn(struct device_node *np) } static inline void of_pci_check_probe_only(void) { } + +static inline bool of_pci_bridge_preserve_resources(struct device_node *node) +{ + return false; +} #endif #if IS_ENABLED(CONFIG_OF_IRQ) -- 2.25.1 ^ permalink raw reply related [flat|nested] 58+ messages in thread
* [PATCH V5] PCI: Add support for preserving boot configuration @ 2024-04-01 7:50 ` Vidya Sagar 0 siblings, 0 replies; 58+ messages in thread From: Vidya Sagar @ 2024-04-01 7:50 UTC (permalink / raw) To: bhelgaas, rafael, lenb, will, lpieralisi, kw, robh, frowand.list Cc: linux-pci, linux-acpi, linux-kernel, linux-arm-kernel, devicetree, treding, jonathanh, kthota, mmaddireddy, vidyas, sagar.tv Add support for preserving the boot configuration done by the platform firmware per host bridge basis, based on the presence of 'linux,pci-probe-only' property in the respective PCI host bridge device-tree node. It also unifies the ACPI and DT based boot flows in this regard. Signed-off-by: Vidya Sagar <vidyas@nvidia.com> --- V5: * Addressed Rob's review comments V4: * Addressed Bjorn's review comments V3: * Unified ACPI and DT flows as part of addressing Bjorn's review comments V2: * Addressed issues reported by kernel test robot <lkp@intel.com> drivers/acpi/pci_root.c | 12 ----- drivers/pci/controller/pci-host-common.c | 4 -- drivers/pci/of.c | 57 +++++++++++++++++++----- drivers/pci/probe.c | 46 ++++++++++++++----- include/linux/of_pci.h | 6 +++ 5 files changed, 88 insertions(+), 37 deletions(-) diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c index 84030804a763..ddc2b3e89111 100644 --- a/drivers/acpi/pci_root.c +++ b/drivers/acpi/pci_root.c @@ -1008,7 +1008,6 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root, int node = acpi_get_node(device->handle); struct pci_bus *bus; struct pci_host_bridge *host_bridge; - union acpi_object *obj; info->root = root; info->bridge = device; @@ -1050,17 +1049,6 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root, if (!(root->osc_ext_control_set & OSC_CXL_ERROR_REPORTING_CONTROL)) host_bridge->native_cxl_error = 0; - /* - * Evaluate the "PCI Boot Configuration" _DSM Function. If it - * exists and returns 0, we must preserve any PCI resource - * assignments made by firmware for this host bridge. - */ - obj = acpi_evaluate_dsm(ACPI_HANDLE(bus->bridge), &pci_acpi_dsm_guid, 1, - DSM_PCI_PRESERVE_BOOT_CONFIG, NULL); - if (obj && obj->type == ACPI_TYPE_INTEGER && obj->integer.value == 0) - host_bridge->preserve_config = 1; - ACPI_FREE(obj); - acpi_dev_power_up_children_with_adr(device); pci_scan_child_bus(bus); diff --git a/drivers/pci/controller/pci-host-common.c b/drivers/pci/controller/pci-host-common.c index 6be3266cd7b5..e2602e38ae45 100644 --- a/drivers/pci/controller/pci-host-common.c +++ b/drivers/pci/controller/pci-host-common.c @@ -73,10 +73,6 @@ int pci_host_common_probe(struct platform_device *pdev) if (IS_ERR(cfg)) return PTR_ERR(cfg); - /* Do not reassign resources if probe only */ - if (!pci_has_flag(PCI_PROBE_ONLY)) - pci_add_flags(PCI_REASSIGN_ALL_BUS); - bridge->sysdata = cfg; bridge->ops = (struct pci_ops *)&ops->pci_ops; bridge->msi_domain = true; diff --git a/drivers/pci/of.c b/drivers/pci/of.c index 51e3dd0ea5ab..e6da3654f9ac 100644 --- a/drivers/pci/of.c +++ b/drivers/pci/of.c @@ -239,24 +239,61 @@ EXPORT_SYMBOL_GPL(of_get_pci_domain_nr); */ void of_pci_check_probe_only(void) { - u32 val; + bool is_preserve_config = of_pci_bridge_preserve_resources(of_chosen); + + if (is_preserve_config) + pci_add_flags(PCI_PROBE_ONLY); + else + pci_clear_flags(PCI_PROBE_ONLY); + + pr_info("PROBE_ONLY %s\n", is_preserve_config ? "enabled" : "disabled"); +} +EXPORT_SYMBOL_GPL(of_pci_check_probe_only); + +/** + * of_pci_bridge_preserve_resources - Return true if the boot configuration + * needs to be preserved + * @node: Device tree node. + * + * This function looks for "linux,pci-probe-only" property for a given + * PCI controller's node and returns true if found. It will also look in the + * chosen node if the property is not found in the given controller's node. + * Having this property ensures that the kernel doesn't reconfigure the + * BARs and bridge windows that are already done by the platform firmware. + * + * Return: true if the property exists false otherwise. + */ +bool of_pci_bridge_preserve_resources(struct device_node *node) +{ + u32 val = 0; int ret; - ret = of_property_read_u32(of_chosen, "linux,pci-probe-only", &val); + if (!node) { + pr_warn("device node is NULL, trying with of_chosen\n"); + node = of_chosen; + } + +retry: + ret = of_property_read_u32(node, "linux,pci-probe-only", &val); if (ret) { - if (ret == -ENODATA || ret == -EOVERFLOW) - pr_warn("linux,pci-probe-only without valid value, ignoring\n"); - return; + if (ret == -ENODATA || ret == -EOVERFLOW) { + pr_warn("Incorrect value for linux,pci-probe-only in %pOF, ignoring\n", node); + return false; + } + if (ret == -EINVAL) { + if (node == of_chosen) + return false; + + node = of_chosen; + goto retry; + } } if (val) - pci_add_flags(PCI_PROBE_ONLY); + return true; else - pci_clear_flags(PCI_PROBE_ONLY); - - pr_info("PROBE_ONLY %s\n", val ? "enabled" : "disabled"); + return false; } -EXPORT_SYMBOL_GPL(of_pci_check_probe_only); /** * devm_of_pci_get_host_bridge_resources() - Resource-managed parsing of PCI diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 795534589b98..b0e0226a8da8 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -15,6 +15,7 @@ #include <linux/cpumask.h> #include <linux/aer.h> #include <linux/acpi.h> +#include <linux/pci-acpi.h> #include <linux/hypervisor.h> #include <linux/irqdomain.h> #include <linux/pm_runtime.h> @@ -877,6 +878,28 @@ static void pci_set_bus_msi_domain(struct pci_bus *bus) dev_set_msi_domain(&bus->dev, d); } +static void pci_check_config_preserve(struct pci_host_bridge *host_bridge) +{ + if (ACPI_HANDLE(&host_bridge->dev)) { + union acpi_object *obj; + + /* + * Evaluate the "PCI Boot Configuration" _DSM Function. If it + * exists and returns 0, we must preserve any PCI resource + * assignments made by firmware for this host bridge. + */ + obj = acpi_evaluate_dsm(ACPI_HANDLE(&host_bridge->dev), &pci_acpi_dsm_guid, 1, + DSM_PCI_PRESERVE_BOOT_CONFIG, NULL); + if (obj && obj->type == ACPI_TYPE_INTEGER && obj->integer.value == 0) + host_bridge->preserve_config = 1; + ACPI_FREE(obj); + } + + if (host_bridge->dev.parent && host_bridge->dev.parent->of_node) + host_bridge->preserve_config = + of_pci_bridge_preserve_resources(host_bridge->dev.parent->of_node); +} + static int pci_register_host_bridge(struct pci_host_bridge *bridge) { struct device *parent = bridge->dev.parent; @@ -971,6 +994,9 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge) if (nr_node_ids > 1 && pcibus_to_node(bus) == NUMA_NO_NODE) dev_warn(&bus->dev, "Unknown NUMA node; performance will be reduced\n"); + /* Check if the boot configuration by FW needs to be preserved */ + pci_check_config_preserve(bridge); + /* Coalesce contiguous windows */ resource_list_for_each_entry_safe(window, n, &resources) { if (list_is_last(&window->node, &resources)) @@ -3080,20 +3106,18 @@ int pci_host_probe(struct pci_host_bridge *bridge) bus = bridge->bus; + /* If we must preserve the resource configuration, claim now */ + if (bridge->preserve_config) + pci_bus_claim_resources(bus); + /* - * We insert PCI resources into the iomem_resource and - * ioport_resource trees in either pci_bus_claim_resources() - * or pci_bus_assign_resources(). + * Assign whatever was left unassigned. If we didn't claim above, + * this will reassign everything. */ - if (pci_has_flag(PCI_PROBE_ONLY)) { - pci_bus_claim_resources(bus); - } else { - pci_bus_size_bridges(bus); - pci_bus_assign_resources(bus); + pci_assign_unassigned_root_bus_resources(bus); - list_for_each_entry(child, &bus->children, node) - pcie_bus_configure_settings(child); - } + list_for_each_entry(child, &bus->children, node) + pcie_bus_configure_settings(child); pci_bus_add_devices(bus); return 0; diff --git a/include/linux/of_pci.h b/include/linux/of_pci.h index 29658c0ee71f..3f3909a5d55d 100644 --- a/include/linux/of_pci.h +++ b/include/linux/of_pci.h @@ -13,6 +13,7 @@ struct device_node *of_pci_find_child_device(struct device_node *parent, unsigned int devfn); int of_pci_get_devfn(struct device_node *np); void of_pci_check_probe_only(void); +bool of_pci_bridge_preserve_resources(struct device_node *node); #else static inline struct device_node *of_pci_find_child_device(struct device_node *parent, unsigned int devfn) @@ -26,6 +27,11 @@ static inline int of_pci_get_devfn(struct device_node *np) } static inline void of_pci_check_probe_only(void) { } + +static inline bool of_pci_bridge_preserve_resources(struct device_node *node) +{ + return false; +} #endif #if IS_ENABLED(CONFIG_OF_IRQ) -- 2.25.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 58+ messages in thread
* Re: [PATCH V5] PCI: Add support for preserving boot configuration 2024-04-01 7:50 ` Vidya Sagar @ 2024-04-02 16:01 ` Rob Herring -1 siblings, 0 replies; 58+ messages in thread From: Rob Herring @ 2024-04-02 16:01 UTC (permalink / raw) To: Vidya Sagar Cc: lpieralisi, mmaddireddy, linux-kernel, will, jonathanh, kthota, frowand.list, kw, linux-arm-kernel, lenb, devicetree, sagar.tv, rafael, bhelgaas, linux-pci, treding, linux-acpi On Mon, 01 Apr 2024 13:20:31 +0530, Vidya Sagar wrote: > Add support for preserving the boot configuration done by the > platform firmware per host bridge basis, based on the presence of > 'linux,pci-probe-only' property in the respective PCI host bridge > device-tree node. It also unifies the ACPI and DT based boot flows > in this regard. > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > --- > V5: > * Addressed Rob's review comments > > V4: > * Addressed Bjorn's review comments > > V3: > * Unified ACPI and DT flows as part of addressing Bjorn's review comments > > V2: > * Addressed issues reported by kernel test robot <lkp@intel.com> > > drivers/acpi/pci_root.c | 12 ----- > drivers/pci/controller/pci-host-common.c | 4 -- > drivers/pci/of.c | 57 +++++++++++++++++++----- > drivers/pci/probe.c | 46 ++++++++++++++----- > include/linux/of_pci.h | 6 +++ > 5 files changed, 88 insertions(+), 37 deletions(-) > Reviewed-by: Rob Herring <robh@kernel.org> ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH V5] PCI: Add support for preserving boot configuration @ 2024-04-02 16:01 ` Rob Herring 0 siblings, 0 replies; 58+ messages in thread From: Rob Herring @ 2024-04-02 16:01 UTC (permalink / raw) To: Vidya Sagar Cc: lpieralisi, mmaddireddy, linux-kernel, will, jonathanh, kthota, frowand.list, kw, linux-arm-kernel, lenb, devicetree, sagar.tv, rafael, bhelgaas, linux-pci, treding, linux-acpi On Mon, 01 Apr 2024 13:20:31 +0530, Vidya Sagar wrote: > Add support for preserving the boot configuration done by the > platform firmware per host bridge basis, based on the presence of > 'linux,pci-probe-only' property in the respective PCI host bridge > device-tree node. It also unifies the ACPI and DT based boot flows > in this regard. > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > --- > V5: > * Addressed Rob's review comments > > V4: > * Addressed Bjorn's review comments > > V3: > * Unified ACPI and DT flows as part of addressing Bjorn's review comments > > V2: > * Addressed issues reported by kernel test robot <lkp@intel.com> > > drivers/acpi/pci_root.c | 12 ----- > drivers/pci/controller/pci-host-common.c | 4 -- > drivers/pci/of.c | 57 +++++++++++++++++++----- > drivers/pci/probe.c | 46 ++++++++++++++----- > include/linux/of_pci.h | 6 +++ > 5 files changed, 88 insertions(+), 37 deletions(-) > Reviewed-by: Rob Herring <robh@kernel.org> _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH V5] PCI: Add support for preserving boot configuration 2024-04-02 16:01 ` Rob Herring @ 2024-04-10 7:44 ` Vidya Sagar -1 siblings, 0 replies; 58+ messages in thread From: Vidya Sagar @ 2024-04-10 7:44 UTC (permalink / raw) To: bhelgaas Cc: lpieralisi, mmaddireddy, linux-kernel, will, jonathanh, kthota, frowand.list, kw, linux-arm-kernel, lenb, devicetree, sagar.tv, rafael, linux-pci, treding, linux-acpi, Rob Herring Thanks Rob for the review. Bjorn, do you have any further comments for this patch? On 02-04-2024 21:31, Rob Herring wrote: > External email: Use caution opening links or attachments > > > On Mon, 01 Apr 2024 13:20:31 +0530, Vidya Sagar wrote: >> Add support for preserving the boot configuration done by the >> platform firmware per host bridge basis, based on the presence of >> 'linux,pci-probe-only' property in the respective PCI host bridge >> device-tree node. It also unifies the ACPI and DT based boot flows >> in this regard. >> >> Signed-off-by: Vidya Sagar <vidyas@nvidia.com> >> --- >> V5: >> * Addressed Rob's review comments >> >> V4: >> * Addressed Bjorn's review comments >> >> V3: >> * Unified ACPI and DT flows as part of addressing Bjorn's review comments >> >> V2: >> * Addressed issues reported by kernel test robot <lkp@intel.com> >> >> drivers/acpi/pci_root.c | 12 ----- >> drivers/pci/controller/pci-host-common.c | 4 -- >> drivers/pci/of.c | 57 +++++++++++++++++++----- >> drivers/pci/probe.c | 46 ++++++++++++++----- >> include/linux/of_pci.h | 6 +++ >> 5 files changed, 88 insertions(+), 37 deletions(-) >> > Reviewed-by: Rob Herring <robh@kernel.org> > ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH V5] PCI: Add support for preserving boot configuration @ 2024-04-10 7:44 ` Vidya Sagar 0 siblings, 0 replies; 58+ messages in thread From: Vidya Sagar @ 2024-04-10 7:44 UTC (permalink / raw) To: bhelgaas Cc: lpieralisi, mmaddireddy, linux-kernel, will, jonathanh, kthota, frowand.list, kw, linux-arm-kernel, lenb, devicetree, sagar.tv, rafael, linux-pci, treding, linux-acpi, Rob Herring Thanks Rob for the review. Bjorn, do you have any further comments for this patch? On 02-04-2024 21:31, Rob Herring wrote: > External email: Use caution opening links or attachments > > > On Mon, 01 Apr 2024 13:20:31 +0530, Vidya Sagar wrote: >> Add support for preserving the boot configuration done by the >> platform firmware per host bridge basis, based on the presence of >> 'linux,pci-probe-only' property in the respective PCI host bridge >> device-tree node. It also unifies the ACPI and DT based boot flows >> in this regard. >> >> Signed-off-by: Vidya Sagar <vidyas@nvidia.com> >> --- >> V5: >> * Addressed Rob's review comments >> >> V4: >> * Addressed Bjorn's review comments >> >> V3: >> * Unified ACPI and DT flows as part of addressing Bjorn's review comments >> >> V2: >> * Addressed issues reported by kernel test robot <lkp@intel.com> >> >> drivers/acpi/pci_root.c | 12 ----- >> drivers/pci/controller/pci-host-common.c | 4 -- >> drivers/pci/of.c | 57 +++++++++++++++++++----- >> drivers/pci/probe.c | 46 ++++++++++++++----- >> include/linux/of_pci.h | 6 +++ >> 5 files changed, 88 insertions(+), 37 deletions(-) >> > Reviewed-by: Rob Herring <robh@kernel.org> > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH V5] PCI: Add support for preserving boot configuration 2024-04-01 7:50 ` Vidya Sagar @ 2024-04-10 20:50 ` Bjorn Helgaas -1 siblings, 0 replies; 58+ messages in thread From: Bjorn Helgaas @ 2024-04-10 20:50 UTC (permalink / raw) To: Vidya Sagar Cc: bhelgaas, rafael, lenb, will, lpieralisi, kw, robh, frowand.list, linux-pci, linux-acpi, linux-kernel, linux-arm-kernel, devicetree, treding, jonathanh, kthota, mmaddireddy, sagar.tv On Mon, Apr 01, 2024 at 01:20:31PM +0530, Vidya Sagar wrote: > Add support for preserving the boot configuration done by the > platform firmware per host bridge basis, based on the presence of > 'linux,pci-probe-only' property in the respective PCI host bridge > device-tree node. It also unifies the ACPI and DT based boot flows > in this regard. > drivers/acpi/pci_root.c | 12 ----- > drivers/pci/controller/pci-host-common.c | 4 -- > drivers/pci/of.c | 57 +++++++++++++++++++----- > drivers/pci/probe.c | 46 ++++++++++++++----- > include/linux/of_pci.h | 6 +++ > 5 files changed, 88 insertions(+), 37 deletions(-) What does this apply to? I tried v6.9-rc1: $ git checkout -b wip/2404-vidya-preserve-boot-v5 v6.9-rc1 Switched to a new branch 'wip/2404-vidya-preserve-boot-v5' $ git am m/v5_20240401_vidyas_pci_add_support_for_preserving_boot_configuration.mbx Applying: PCI: Add support for preserving boot configuration error: patch failed: drivers/acpi/pci_root.c:1050 error: drivers/acpi/pci_root.c: patch does not apply Patch failed at 0001 PCI: Add support for preserving boot configuration > diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c > index 84030804a763..ddc2b3e89111 100644 > --- a/drivers/acpi/pci_root.c > +++ b/drivers/acpi/pci_root.c > @@ -1008,7 +1008,6 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root, > int node = acpi_get_node(device->handle); > struct pci_bus *bus; > struct pci_host_bridge *host_bridge; > - union acpi_object *obj; > > info->root = root; > info->bridge = device; > @@ -1050,17 +1049,6 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root, > if (!(root->osc_ext_control_set & OSC_CXL_ERROR_REPORTING_CONTROL)) > host_bridge->native_cxl_error = 0; > > - /* > - * Evaluate the "PCI Boot Configuration" _DSM Function. If it > - * exists and returns 0, we must preserve any PCI resource > - * assignments made by firmware for this host bridge. > - */ > - obj = acpi_evaluate_dsm(ACPI_HANDLE(bus->bridge), &pci_acpi_dsm_guid, 1, > - DSM_PCI_PRESERVE_BOOT_CONFIG, NULL); > - if (obj && obj->type == ACPI_TYPE_INTEGER && obj->integer.value == 0) > - host_bridge->preserve_config = 1; > - ACPI_FREE(obj); > - > acpi_dev_power_up_children_with_adr(device); > > pci_scan_child_bus(bus); > diff --git a/drivers/pci/controller/pci-host-common.c b/drivers/pci/controller/pci-host-common.c > index 6be3266cd7b5..e2602e38ae45 100644 > --- a/drivers/pci/controller/pci-host-common.c > +++ b/drivers/pci/controller/pci-host-common.c > @@ -73,10 +73,6 @@ int pci_host_common_probe(struct platform_device *pdev) > if (IS_ERR(cfg)) > return PTR_ERR(cfg); > > - /* Do not reassign resources if probe only */ > - if (!pci_has_flag(PCI_PROBE_ONLY)) > - pci_add_flags(PCI_REASSIGN_ALL_BUS); > - > bridge->sysdata = cfg; > bridge->ops = (struct pci_ops *)&ops->pci_ops; > bridge->msi_domain = true; > diff --git a/drivers/pci/of.c b/drivers/pci/of.c > index 51e3dd0ea5ab..e6da3654f9ac 100644 > --- a/drivers/pci/of.c > +++ b/drivers/pci/of.c > @@ -239,24 +239,61 @@ EXPORT_SYMBOL_GPL(of_get_pci_domain_nr); > */ > void of_pci_check_probe_only(void) > { > - u32 val; > + bool is_preserve_config = of_pci_bridge_preserve_resources(of_chosen); No need for a bool here: if (of_pci_bridge_preserve_resources(of_chosen)) pci_add_flags(PCI_PROBE_ONLY); I think it would make more sense to add of_pci_bridge_preserve_resources() *above* of_pci_check_probe_only() since the usual order is to define functions earlier in the file than the calls. > + > + if (is_preserve_config) > + pci_add_flags(PCI_PROBE_ONLY); > + else > + pci_clear_flags(PCI_PROBE_ONLY); Not related to *this* patch, but I see that of_pci_check_probe_only() already clears PCI_PROBE_ONLY (added by f81c11af617c ("of/pci: Add of_pci_check_probe_only to parse "linux,pci-probe-only""). I'm concerned about clearing PCI_PROBE_ONLY because some platforms set this unconditionally, and I don't think they necessarily have "linux,pci-probe-only" in DT. Apparently none of them currently calls of_pci_check_probe_only() so PCI_PROBE_ONLY remains set, but clearing it here feels like a landmine waiting for somebody to move this into a unified call path. I guess if we were to drop pci_clear_flags(), that should be a separate patch in case it breaks something. > + pr_info("PROBE_ONLY %s\n", is_preserve_config ? "enabled" : "disabled"); > +} > +EXPORT_SYMBOL_GPL(of_pci_check_probe_only); > + > +/** > + * of_pci_bridge_preserve_resources - Return true if the boot configuration > + * needs to be preserved > + * @node: Device tree node. > + * > + * This function looks for "linux,pci-probe-only" property for a given > + * PCI controller's node and returns true if found. It will also look in the > + * chosen node if the property is not found in the given controller's node. > + * Having this property ensures that the kernel doesn't reconfigure the > + * BARs and bridge windows that are already done by the platform firmware. > + * > + * Return: true if the property exists false otherwise. > + */ > +bool of_pci_bridge_preserve_resources(struct device_node *node) > +{ > + u32 val = 0; > int ret; > > - ret = of_property_read_u32(of_chosen, "linux,pci-probe-only", &val); > + if (!node) { > + pr_warn("device node is NULL, trying with of_chosen\n"); > + node = of_chosen; > + } > + > +retry: > + ret = of_property_read_u32(node, "linux,pci-probe-only", &val); > if (ret) { > - if (ret == -ENODATA || ret == -EOVERFLOW) > - pr_warn("linux,pci-probe-only without valid value, ignoring\n"); > - return; > + if (ret == -ENODATA || ret == -EOVERFLOW) { > + pr_warn("Incorrect value for linux,pci-probe-only in %pOF, ignoring\n", node); > + return false; > + } > + if (ret == -EINVAL) { > + if (node == of_chosen) > + return false; > + > + node = of_chosen; > + goto retry; > + } > } > > if (val) > - pci_add_flags(PCI_PROBE_ONLY); > + return true; > else > - pci_clear_flags(PCI_PROBE_ONLY); > - > - pr_info("PROBE_ONLY %s\n", val ? "enabled" : "disabled"); > + return false; > } > -EXPORT_SYMBOL_GPL(of_pci_check_probe_only); > > /** > * devm_of_pci_get_host_bridge_resources() - Resource-managed parsing of PCI > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index 795534589b98..b0e0226a8da8 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -15,6 +15,7 @@ > #include <linux/cpumask.h> > #include <linux/aer.h> > #include <linux/acpi.h> > +#include <linux/pci-acpi.h> > #include <linux/hypervisor.h> > #include <linux/irqdomain.h> > #include <linux/pm_runtime.h> > @@ -877,6 +878,28 @@ static void pci_set_bus_msi_domain(struct pci_bus *bus) > dev_set_msi_domain(&bus->dev, d); > } > > +static void pci_check_config_preserve(struct pci_host_bridge *host_bridge) > +{ > + if (ACPI_HANDLE(&host_bridge->dev)) { > + union acpi_object *obj; > + > + /* > + * Evaluate the "PCI Boot Configuration" _DSM Function. If it > + * exists and returns 0, we must preserve any PCI resource > + * assignments made by firmware for this host bridge. > + */ > + obj = acpi_evaluate_dsm(ACPI_HANDLE(&host_bridge->dev), &pci_acpi_dsm_guid, 1, > + DSM_PCI_PRESERVE_BOOT_CONFIG, NULL); > + if (obj && obj->type == ACPI_TYPE_INTEGER && obj->integer.value == 0) > + host_bridge->preserve_config = 1; > + ACPI_FREE(obj); > + } > + > + if (host_bridge->dev.parent && host_bridge->dev.parent->of_node) > + host_bridge->preserve_config = > + of_pci_bridge_preserve_resources(host_bridge->dev.parent->of_node); > +} > + > static int pci_register_host_bridge(struct pci_host_bridge *bridge) > { > struct device *parent = bridge->dev.parent; > @@ -971,6 +994,9 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge) > if (nr_node_ids > 1 && pcibus_to_node(bus) == NUMA_NO_NODE) > dev_warn(&bus->dev, "Unknown NUMA node; performance will be reduced\n"); > > + /* Check if the boot configuration by FW needs to be preserved */ > + pci_check_config_preserve(bridge); I really have an allergic reaction to functions named "..._check_..." We can tell that the function has something to do with preserving configuration, and it's void so obviously the side effects are the important thing, but there's no clue in the caller about what the side effects are. I'd prefer something like: bridge->preserve_config = pci_must_preserve_config(bridge); where pci_must_preserve_config() has no side effects, returns bool, and the action is at the caller. > /* Coalesce contiguous windows */ > resource_list_for_each_entry_safe(window, n, &resources) { > if (list_is_last(&window->node, &resources)) > @@ -3080,20 +3106,18 @@ int pci_host_probe(struct pci_host_bridge *bridge) > > bus = bridge->bus; > > + /* If we must preserve the resource configuration, claim now */ > + if (bridge->preserve_config) > + pci_bus_claim_resources(bus); > + > /* > - * We insert PCI resources into the iomem_resource and > - * ioport_resource trees in either pci_bus_claim_resources() > - * or pci_bus_assign_resources(). > + * Assign whatever was left unassigned. If we didn't claim above, > + * this will reassign everything. > */ > - if (pci_has_flag(PCI_PROBE_ONLY)) { > - pci_bus_claim_resources(bus); > - } else { > - pci_bus_size_bridges(bus); > - pci_bus_assign_resources(bus); > + pci_assign_unassigned_root_bus_resources(bus); > > - list_for_each_entry(child, &bus->children, node) > - pcie_bus_configure_settings(child); > - } > + list_for_each_entry(child, &bus->children, node) > + pcie_bus_configure_settings(child); > > pci_bus_add_devices(bus); > return 0; > diff --git a/include/linux/of_pci.h b/include/linux/of_pci.h > index 29658c0ee71f..3f3909a5d55d 100644 > --- a/include/linux/of_pci.h > +++ b/include/linux/of_pci.h > @@ -13,6 +13,7 @@ struct device_node *of_pci_find_child_device(struct device_node *parent, > unsigned int devfn); > int of_pci_get_devfn(struct device_node *np); > void of_pci_check_probe_only(void); > +bool of_pci_bridge_preserve_resources(struct device_node *node); This looks like it should be in drivers/pci/pci.h since it's not used outside drivers/pci/. > #else > static inline struct device_node *of_pci_find_child_device(struct device_node *parent, > unsigned int devfn) > @@ -26,6 +27,11 @@ static inline int of_pci_get_devfn(struct device_node *np) > } > > static inline void of_pci_check_probe_only(void) { } > + > +static inline bool of_pci_bridge_preserve_resources(struct device_node *node) > +{ > + return false; > +} > #endif > > #if IS_ENABLED(CONFIG_OF_IRQ) > -- > 2.25.1 > ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH V5] PCI: Add support for preserving boot configuration @ 2024-04-10 20:50 ` Bjorn Helgaas 0 siblings, 0 replies; 58+ messages in thread From: Bjorn Helgaas @ 2024-04-10 20:50 UTC (permalink / raw) To: Vidya Sagar Cc: bhelgaas, rafael, lenb, will, lpieralisi, kw, robh, frowand.list, linux-pci, linux-acpi, linux-kernel, linux-arm-kernel, devicetree, treding, jonathanh, kthota, mmaddireddy, sagar.tv On Mon, Apr 01, 2024 at 01:20:31PM +0530, Vidya Sagar wrote: > Add support for preserving the boot configuration done by the > platform firmware per host bridge basis, based on the presence of > 'linux,pci-probe-only' property in the respective PCI host bridge > device-tree node. It also unifies the ACPI and DT based boot flows > in this regard. > drivers/acpi/pci_root.c | 12 ----- > drivers/pci/controller/pci-host-common.c | 4 -- > drivers/pci/of.c | 57 +++++++++++++++++++----- > drivers/pci/probe.c | 46 ++++++++++++++----- > include/linux/of_pci.h | 6 +++ > 5 files changed, 88 insertions(+), 37 deletions(-) What does this apply to? I tried v6.9-rc1: $ git checkout -b wip/2404-vidya-preserve-boot-v5 v6.9-rc1 Switched to a new branch 'wip/2404-vidya-preserve-boot-v5' $ git am m/v5_20240401_vidyas_pci_add_support_for_preserving_boot_configuration.mbx Applying: PCI: Add support for preserving boot configuration error: patch failed: drivers/acpi/pci_root.c:1050 error: drivers/acpi/pci_root.c: patch does not apply Patch failed at 0001 PCI: Add support for preserving boot configuration > diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c > index 84030804a763..ddc2b3e89111 100644 > --- a/drivers/acpi/pci_root.c > +++ b/drivers/acpi/pci_root.c > @@ -1008,7 +1008,6 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root, > int node = acpi_get_node(device->handle); > struct pci_bus *bus; > struct pci_host_bridge *host_bridge; > - union acpi_object *obj; > > info->root = root; > info->bridge = device; > @@ -1050,17 +1049,6 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root, > if (!(root->osc_ext_control_set & OSC_CXL_ERROR_REPORTING_CONTROL)) > host_bridge->native_cxl_error = 0; > > - /* > - * Evaluate the "PCI Boot Configuration" _DSM Function. If it > - * exists and returns 0, we must preserve any PCI resource > - * assignments made by firmware for this host bridge. > - */ > - obj = acpi_evaluate_dsm(ACPI_HANDLE(bus->bridge), &pci_acpi_dsm_guid, 1, > - DSM_PCI_PRESERVE_BOOT_CONFIG, NULL); > - if (obj && obj->type == ACPI_TYPE_INTEGER && obj->integer.value == 0) > - host_bridge->preserve_config = 1; > - ACPI_FREE(obj); > - > acpi_dev_power_up_children_with_adr(device); > > pci_scan_child_bus(bus); > diff --git a/drivers/pci/controller/pci-host-common.c b/drivers/pci/controller/pci-host-common.c > index 6be3266cd7b5..e2602e38ae45 100644 > --- a/drivers/pci/controller/pci-host-common.c > +++ b/drivers/pci/controller/pci-host-common.c > @@ -73,10 +73,6 @@ int pci_host_common_probe(struct platform_device *pdev) > if (IS_ERR(cfg)) > return PTR_ERR(cfg); > > - /* Do not reassign resources if probe only */ > - if (!pci_has_flag(PCI_PROBE_ONLY)) > - pci_add_flags(PCI_REASSIGN_ALL_BUS); > - > bridge->sysdata = cfg; > bridge->ops = (struct pci_ops *)&ops->pci_ops; > bridge->msi_domain = true; > diff --git a/drivers/pci/of.c b/drivers/pci/of.c > index 51e3dd0ea5ab..e6da3654f9ac 100644 > --- a/drivers/pci/of.c > +++ b/drivers/pci/of.c > @@ -239,24 +239,61 @@ EXPORT_SYMBOL_GPL(of_get_pci_domain_nr); > */ > void of_pci_check_probe_only(void) > { > - u32 val; > + bool is_preserve_config = of_pci_bridge_preserve_resources(of_chosen); No need for a bool here: if (of_pci_bridge_preserve_resources(of_chosen)) pci_add_flags(PCI_PROBE_ONLY); I think it would make more sense to add of_pci_bridge_preserve_resources() *above* of_pci_check_probe_only() since the usual order is to define functions earlier in the file than the calls. > + > + if (is_preserve_config) > + pci_add_flags(PCI_PROBE_ONLY); > + else > + pci_clear_flags(PCI_PROBE_ONLY); Not related to *this* patch, but I see that of_pci_check_probe_only() already clears PCI_PROBE_ONLY (added by f81c11af617c ("of/pci: Add of_pci_check_probe_only to parse "linux,pci-probe-only""). I'm concerned about clearing PCI_PROBE_ONLY because some platforms set this unconditionally, and I don't think they necessarily have "linux,pci-probe-only" in DT. Apparently none of them currently calls of_pci_check_probe_only() so PCI_PROBE_ONLY remains set, but clearing it here feels like a landmine waiting for somebody to move this into a unified call path. I guess if we were to drop pci_clear_flags(), that should be a separate patch in case it breaks something. > + pr_info("PROBE_ONLY %s\n", is_preserve_config ? "enabled" : "disabled"); > +} > +EXPORT_SYMBOL_GPL(of_pci_check_probe_only); > + > +/** > + * of_pci_bridge_preserve_resources - Return true if the boot configuration > + * needs to be preserved > + * @node: Device tree node. > + * > + * This function looks for "linux,pci-probe-only" property for a given > + * PCI controller's node and returns true if found. It will also look in the > + * chosen node if the property is not found in the given controller's node. > + * Having this property ensures that the kernel doesn't reconfigure the > + * BARs and bridge windows that are already done by the platform firmware. > + * > + * Return: true if the property exists false otherwise. > + */ > +bool of_pci_bridge_preserve_resources(struct device_node *node) > +{ > + u32 val = 0; > int ret; > > - ret = of_property_read_u32(of_chosen, "linux,pci-probe-only", &val); > + if (!node) { > + pr_warn("device node is NULL, trying with of_chosen\n"); > + node = of_chosen; > + } > + > +retry: > + ret = of_property_read_u32(node, "linux,pci-probe-only", &val); > if (ret) { > - if (ret == -ENODATA || ret == -EOVERFLOW) > - pr_warn("linux,pci-probe-only without valid value, ignoring\n"); > - return; > + if (ret == -ENODATA || ret == -EOVERFLOW) { > + pr_warn("Incorrect value for linux,pci-probe-only in %pOF, ignoring\n", node); > + return false; > + } > + if (ret == -EINVAL) { > + if (node == of_chosen) > + return false; > + > + node = of_chosen; > + goto retry; > + } > } > > if (val) > - pci_add_flags(PCI_PROBE_ONLY); > + return true; > else > - pci_clear_flags(PCI_PROBE_ONLY); > - > - pr_info("PROBE_ONLY %s\n", val ? "enabled" : "disabled"); > + return false; > } > -EXPORT_SYMBOL_GPL(of_pci_check_probe_only); > > /** > * devm_of_pci_get_host_bridge_resources() - Resource-managed parsing of PCI > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index 795534589b98..b0e0226a8da8 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -15,6 +15,7 @@ > #include <linux/cpumask.h> > #include <linux/aer.h> > #include <linux/acpi.h> > +#include <linux/pci-acpi.h> > #include <linux/hypervisor.h> > #include <linux/irqdomain.h> > #include <linux/pm_runtime.h> > @@ -877,6 +878,28 @@ static void pci_set_bus_msi_domain(struct pci_bus *bus) > dev_set_msi_domain(&bus->dev, d); > } > > +static void pci_check_config_preserve(struct pci_host_bridge *host_bridge) > +{ > + if (ACPI_HANDLE(&host_bridge->dev)) { > + union acpi_object *obj; > + > + /* > + * Evaluate the "PCI Boot Configuration" _DSM Function. If it > + * exists and returns 0, we must preserve any PCI resource > + * assignments made by firmware for this host bridge. > + */ > + obj = acpi_evaluate_dsm(ACPI_HANDLE(&host_bridge->dev), &pci_acpi_dsm_guid, 1, > + DSM_PCI_PRESERVE_BOOT_CONFIG, NULL); > + if (obj && obj->type == ACPI_TYPE_INTEGER && obj->integer.value == 0) > + host_bridge->preserve_config = 1; > + ACPI_FREE(obj); > + } > + > + if (host_bridge->dev.parent && host_bridge->dev.parent->of_node) > + host_bridge->preserve_config = > + of_pci_bridge_preserve_resources(host_bridge->dev.parent->of_node); > +} > + > static int pci_register_host_bridge(struct pci_host_bridge *bridge) > { > struct device *parent = bridge->dev.parent; > @@ -971,6 +994,9 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge) > if (nr_node_ids > 1 && pcibus_to_node(bus) == NUMA_NO_NODE) > dev_warn(&bus->dev, "Unknown NUMA node; performance will be reduced\n"); > > + /* Check if the boot configuration by FW needs to be preserved */ > + pci_check_config_preserve(bridge); I really have an allergic reaction to functions named "..._check_..." We can tell that the function has something to do with preserving configuration, and it's void so obviously the side effects are the important thing, but there's no clue in the caller about what the side effects are. I'd prefer something like: bridge->preserve_config = pci_must_preserve_config(bridge); where pci_must_preserve_config() has no side effects, returns bool, and the action is at the caller. > /* Coalesce contiguous windows */ > resource_list_for_each_entry_safe(window, n, &resources) { > if (list_is_last(&window->node, &resources)) > @@ -3080,20 +3106,18 @@ int pci_host_probe(struct pci_host_bridge *bridge) > > bus = bridge->bus; > > + /* If we must preserve the resource configuration, claim now */ > + if (bridge->preserve_config) > + pci_bus_claim_resources(bus); > + > /* > - * We insert PCI resources into the iomem_resource and > - * ioport_resource trees in either pci_bus_claim_resources() > - * or pci_bus_assign_resources(). > + * Assign whatever was left unassigned. If we didn't claim above, > + * this will reassign everything. > */ > - if (pci_has_flag(PCI_PROBE_ONLY)) { > - pci_bus_claim_resources(bus); > - } else { > - pci_bus_size_bridges(bus); > - pci_bus_assign_resources(bus); > + pci_assign_unassigned_root_bus_resources(bus); > > - list_for_each_entry(child, &bus->children, node) > - pcie_bus_configure_settings(child); > - } > + list_for_each_entry(child, &bus->children, node) > + pcie_bus_configure_settings(child); > > pci_bus_add_devices(bus); > return 0; > diff --git a/include/linux/of_pci.h b/include/linux/of_pci.h > index 29658c0ee71f..3f3909a5d55d 100644 > --- a/include/linux/of_pci.h > +++ b/include/linux/of_pci.h > @@ -13,6 +13,7 @@ struct device_node *of_pci_find_child_device(struct device_node *parent, > unsigned int devfn); > int of_pci_get_devfn(struct device_node *np); > void of_pci_check_probe_only(void); > +bool of_pci_bridge_preserve_resources(struct device_node *node); This looks like it should be in drivers/pci/pci.h since it's not used outside drivers/pci/. > #else > static inline struct device_node *of_pci_find_child_device(struct device_node *parent, > unsigned int devfn) > @@ -26,6 +27,11 @@ static inline int of_pci_get_devfn(struct device_node *np) > } > > static inline void of_pci_check_probe_only(void) { } > + > +static inline bool of_pci_bridge_preserve_resources(struct device_node *node) > +{ > + return false; > +} > #endif > > #if IS_ENABLED(CONFIG_OF_IRQ) > -- > 2.25.1 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH V5] PCI: Add support for preserving boot configuration 2024-04-10 20:50 ` Bjorn Helgaas @ 2024-04-18 17:31 ` Vidya Sagar -1 siblings, 0 replies; 58+ messages in thread From: Vidya Sagar @ 2024-04-18 17:31 UTC (permalink / raw) To: Bjorn Helgaas Cc: bhelgaas, rafael, lenb, will, lpieralisi, kw, robh, frowand.list, linux-pci, linux-acpi, linux-kernel, linux-arm-kernel, devicetree, treding, jonathanh, kthota, mmaddireddy, sagar.tv Replied inline... On 11-04-2024 02:20, Bjorn Helgaas wrote: > External email: Use caution opening links or attachments > > > On Mon, Apr 01, 2024 at 01:20:31PM +0530, Vidya Sagar wrote: >> Add support for preserving the boot configuration done by the >> platform firmware per host bridge basis, based on the presence of >> 'linux,pci-probe-only' property in the respective PCI host bridge >> device-tree node. It also unifies the ACPI and DT based boot flows >> in this regard. >> drivers/acpi/pci_root.c | 12 ----- >> drivers/pci/controller/pci-host-common.c | 4 -- >> drivers/pci/of.c | 57 +++++++++++++++++++----- >> drivers/pci/probe.c | 46 ++++++++++++++----- >> include/linux/of_pci.h | 6 +++ >> 5 files changed, 88 insertions(+), 37 deletions(-) > What does this apply to? I tried v6.9-rc1: > > $ git checkout -b wip/2404-vidya-preserve-boot-v5 v6.9-rc1 > Switched to a new branch 'wip/2404-vidya-preserve-boot-v5' > > $ git am m/v5_20240401_vidyas_pci_add_support_for_preserving_boot_configuration.mbx > Applying: PCI: Add support for preserving boot configuration > error: patch failed: drivers/acpi/pci_root.c:1050 > error: drivers/acpi/pci_root.c: patch does not apply > Patch failed at 0001 PCI: Add support for preserving boot configuration I'm going to rebase this patch on top of linux-next and send. >> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c >> index 84030804a763..ddc2b3e89111 100644 >> --- a/drivers/acpi/pci_root.c >> +++ b/drivers/acpi/pci_root.c >> @@ -1008,7 +1008,6 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root, >> int node = acpi_get_node(device->handle); >> struct pci_bus *bus; >> struct pci_host_bridge *host_bridge; >> - union acpi_object *obj; >> >> info->root = root; >> info->bridge = device; >> @@ -1050,17 +1049,6 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root, >> if (!(root->osc_ext_control_set & OSC_CXL_ERROR_REPORTING_CONTROL)) >> host_bridge->native_cxl_error = 0; >> >> - /* >> - * Evaluate the "PCI Boot Configuration" _DSM Function. If it >> - * exists and returns 0, we must preserve any PCI resource >> - * assignments made by firmware for this host bridge. >> - */ >> - obj = acpi_evaluate_dsm(ACPI_HANDLE(bus->bridge), &pci_acpi_dsm_guid, 1, >> - DSM_PCI_PRESERVE_BOOT_CONFIG, NULL); >> - if (obj && obj->type == ACPI_TYPE_INTEGER && obj->integer.value == 0) >> - host_bridge->preserve_config = 1; >> - ACPI_FREE(obj); >> - >> acpi_dev_power_up_children_with_adr(device); >> >> pci_scan_child_bus(bus); >> diff --git a/drivers/pci/controller/pci-host-common.c b/drivers/pci/controller/pci-host-common.c >> index 6be3266cd7b5..e2602e38ae45 100644 >> --- a/drivers/pci/controller/pci-host-common.c >> +++ b/drivers/pci/controller/pci-host-common.c >> @@ -73,10 +73,6 @@ int pci_host_common_probe(struct platform_device *pdev) >> if (IS_ERR(cfg)) >> return PTR_ERR(cfg); >> >> - /* Do not reassign resources if probe only */ >> - if (!pci_has_flag(PCI_PROBE_ONLY)) >> - pci_add_flags(PCI_REASSIGN_ALL_BUS); >> - >> bridge->sysdata = cfg; >> bridge->ops = (struct pci_ops *)&ops->pci_ops; >> bridge->msi_domain = true; >> diff --git a/drivers/pci/of.c b/drivers/pci/of.c >> index 51e3dd0ea5ab..e6da3654f9ac 100644 >> --- a/drivers/pci/of.c >> +++ b/drivers/pci/of.c >> @@ -239,24 +239,61 @@ EXPORT_SYMBOL_GPL(of_get_pci_domain_nr); >> */ >> void of_pci_check_probe_only(void) >> { >> - u32 val; >> + bool is_preserve_config = of_pci_bridge_preserve_resources(of_chosen); > No need for a bool here: > > if (of_pci_bridge_preserve_resources(of_chosen)) > pci_add_flags(PCI_PROBE_ONLY); > > I think it would make more sense to add > of_pci_bridge_preserve_resources() *above* of_pci_check_probe_only() > since the usual order is to define functions earlier in the file than > the calls. I'll take care of it in the next patch. >> + >> + if (is_preserve_config) >> + pci_add_flags(PCI_PROBE_ONLY); >> + else >> + pci_clear_flags(PCI_PROBE_ONLY); > Not related to *this* patch, but I see that of_pci_check_probe_only() > already clears PCI_PROBE_ONLY (added by f81c11af617c ("of/pci: Add > of_pci_check_probe_only to parse "linux,pci-probe-only""). > > I'm concerned about clearing PCI_PROBE_ONLY because some platforms set > this unconditionally, and I don't think they necessarily have > "linux,pci-probe-only" in DT. > > Apparently none of them currently calls of_pci_check_probe_only() so > PCI_PROBE_ONLY remains set, but clearing it here feels like a landmine > waiting for somebody to move this into a unified call path. > > I guess if we were to drop pci_clear_flags(), that should be a > separate patch in case it breaks something. As far as the current patch is concerned, since it is not going to do anything different compared to what already exists, I'll leave it at it for now and ponder over moving pci_clear_flags() to a different patch later. > >> + pr_info("PROBE_ONLY %s\n", is_preserve_config ? "enabled" : "disabled"); >> +} >> +EXPORT_SYMBOL_GPL(of_pci_check_probe_only); >> + >> +/** >> + * of_pci_bridge_preserve_resources - Return true if the boot configuration >> + * needs to be preserved >> + * @node: Device tree node. >> + * >> + * This function looks for "linux,pci-probe-only" property for a given >> + * PCI controller's node and returns true if found. It will also look in the >> + * chosen node if the property is not found in the given controller's node. >> + * Having this property ensures that the kernel doesn't reconfigure the >> + * BARs and bridge windows that are already done by the platform firmware. >> + * >> + * Return: true if the property exists false otherwise. >> + */ >> +bool of_pci_bridge_preserve_resources(struct device_node *node) >> +{ >> + u32 val = 0; >> int ret; >> >> - ret = of_property_read_u32(of_chosen, "linux,pci-probe-only", &val); >> + if (!node) { >> + pr_warn("device node is NULL, trying with of_chosen\n"); >> + node = of_chosen; >> + } >> + >> +retry: >> + ret = of_property_read_u32(node, "linux,pci-probe-only", &val); >> if (ret) { >> - if (ret == -ENODATA || ret == -EOVERFLOW) >> - pr_warn("linux,pci-probe-only without valid value, ignoring\n"); >> - return; >> + if (ret == -ENODATA || ret == -EOVERFLOW) { >> + pr_warn("Incorrect value for linux,pci-probe-only in %pOF, ignoring\n", node); >> + return false; >> + } >> + if (ret == -EINVAL) { >> + if (node == of_chosen) >> + return false; >> + >> + node = of_chosen; >> + goto retry; >> + } >> } >> >> if (val) >> - pci_add_flags(PCI_PROBE_ONLY); >> + return true; >> else >> - pci_clear_flags(PCI_PROBE_ONLY); >> - >> - pr_info("PROBE_ONLY %s\n", val ? "enabled" : "disabled"); >> + return false; >> } >> -EXPORT_SYMBOL_GPL(of_pci_check_probe_only); >> >> /** >> * devm_of_pci_get_host_bridge_resources() - Resource-managed parsing of PCI >> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c >> index 795534589b98..b0e0226a8da8 100644 >> --- a/drivers/pci/probe.c >> +++ b/drivers/pci/probe.c >> @@ -15,6 +15,7 @@ >> #include <linux/cpumask.h> >> #include <linux/aer.h> >> #include <linux/acpi.h> >> +#include <linux/pci-acpi.h> >> #include <linux/hypervisor.h> >> #include <linux/irqdomain.h> >> #include <linux/pm_runtime.h> >> @@ -877,6 +878,28 @@ static void pci_set_bus_msi_domain(struct pci_bus *bus) >> dev_set_msi_domain(&bus->dev, d); >> } >> >> +static void pci_check_config_preserve(struct pci_host_bridge *host_bridge) >> +{ >> + if (ACPI_HANDLE(&host_bridge->dev)) { >> + union acpi_object *obj; >> + >> + /* >> + * Evaluate the "PCI Boot Configuration" _DSM Function. If it >> + * exists and returns 0, we must preserve any PCI resource >> + * assignments made by firmware for this host bridge. >> + */ >> + obj = acpi_evaluate_dsm(ACPI_HANDLE(&host_bridge->dev), &pci_acpi_dsm_guid, 1, >> + DSM_PCI_PRESERVE_BOOT_CONFIG, NULL); >> + if (obj && obj->type == ACPI_TYPE_INTEGER && obj->integer.value == 0) >> + host_bridge->preserve_config = 1; >> + ACPI_FREE(obj); >> + } >> + >> + if (host_bridge->dev.parent && host_bridge->dev.parent->of_node) >> + host_bridge->preserve_config = >> + of_pci_bridge_preserve_resources(host_bridge->dev.parent->of_node); >> +} >> + >> static int pci_register_host_bridge(struct pci_host_bridge *bridge) >> { >> struct device *parent = bridge->dev.parent; >> @@ -971,6 +994,9 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge) >> if (nr_node_ids > 1 && pcibus_to_node(bus) == NUMA_NO_NODE) >> dev_warn(&bus->dev, "Unknown NUMA node; performance will be reduced\n"); >> >> + /* Check if the boot configuration by FW needs to be preserved */ >> + pci_check_config_preserve(bridge); > I really have an allergic reaction to functions named "..._check_..." > We can tell that the function has something to do with preserving > configuration, and it's void so obviously the side effects are the > important thing, but there's no clue in the caller about what the side > effects are. > > I'd prefer something like: > > bridge->preserve_config = pci_must_preserve_config(bridge); > > where pci_must_preserve_config() has no side effects, returns bool, > and the action is at the caller. I'll take care of it in the next patch. >> /* Coalesce contiguous windows */ >> resource_list_for_each_entry_safe(window, n, &resources) { >> if (list_is_last(&window->node, &resources)) >> @@ -3080,20 +3106,18 @@ int pci_host_probe(struct pci_host_bridge *bridge) >> >> bus = bridge->bus; >> >> + /* If we must preserve the resource configuration, claim now */ >> + if (bridge->preserve_config) >> + pci_bus_claim_resources(bus); >> + >> /* >> - * We insert PCI resources into the iomem_resource and >> - * ioport_resource trees in either pci_bus_claim_resources() >> - * or pci_bus_assign_resources(). >> + * Assign whatever was left unassigned. If we didn't claim above, >> + * this will reassign everything. >> */ >> - if (pci_has_flag(PCI_PROBE_ONLY)) { >> - pci_bus_claim_resources(bus); >> - } else { >> - pci_bus_size_bridges(bus); >> - pci_bus_assign_resources(bus); >> + pci_assign_unassigned_root_bus_resources(bus); >> >> - list_for_each_entry(child, &bus->children, node) >> - pcie_bus_configure_settings(child); >> - } >> + list_for_each_entry(child, &bus->children, node) >> + pcie_bus_configure_settings(child); >> >> pci_bus_add_devices(bus); >> return 0; >> diff --git a/include/linux/of_pci.h b/include/linux/of_pci.h >> index 29658c0ee71f..3f3909a5d55d 100644 >> --- a/include/linux/of_pci.h >> +++ b/include/linux/of_pci.h >> @@ -13,6 +13,7 @@ struct device_node *of_pci_find_child_device(struct device_node *parent, >> unsigned int devfn); >> int of_pci_get_devfn(struct device_node *np); >> void of_pci_check_probe_only(void); >> +bool of_pci_bridge_preserve_resources(struct device_node *node); > This looks like it should be in drivers/pci/pci.h since it's not used > outside drivers/pci/. Agree. I'll take care of it in the next patch. >> #else >> static inline struct device_node *of_pci_find_child_device(struct device_node *parent, >> unsigned int devfn) >> @@ -26,6 +27,11 @@ static inline int of_pci_get_devfn(struct device_node *np) >> } >> >> static inline void of_pci_check_probe_only(void) { } >> + >> +static inline bool of_pci_bridge_preserve_resources(struct device_node *node) >> +{ >> + return false; >> +} >> #endif >> >> #if IS_ENABLED(CONFIG_OF_IRQ) >> -- >> 2.25.1 >> ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH V5] PCI: Add support for preserving boot configuration @ 2024-04-18 17:31 ` Vidya Sagar 0 siblings, 0 replies; 58+ messages in thread From: Vidya Sagar @ 2024-04-18 17:31 UTC (permalink / raw) To: Bjorn Helgaas Cc: bhelgaas, rafael, lenb, will, lpieralisi, kw, robh, frowand.list, linux-pci, linux-acpi, linux-kernel, linux-arm-kernel, devicetree, treding, jonathanh, kthota, mmaddireddy, sagar.tv Replied inline... On 11-04-2024 02:20, Bjorn Helgaas wrote: > External email: Use caution opening links or attachments > > > On Mon, Apr 01, 2024 at 01:20:31PM +0530, Vidya Sagar wrote: >> Add support for preserving the boot configuration done by the >> platform firmware per host bridge basis, based on the presence of >> 'linux,pci-probe-only' property in the respective PCI host bridge >> device-tree node. It also unifies the ACPI and DT based boot flows >> in this regard. >> drivers/acpi/pci_root.c | 12 ----- >> drivers/pci/controller/pci-host-common.c | 4 -- >> drivers/pci/of.c | 57 +++++++++++++++++++----- >> drivers/pci/probe.c | 46 ++++++++++++++----- >> include/linux/of_pci.h | 6 +++ >> 5 files changed, 88 insertions(+), 37 deletions(-) > What does this apply to? I tried v6.9-rc1: > > $ git checkout -b wip/2404-vidya-preserve-boot-v5 v6.9-rc1 > Switched to a new branch 'wip/2404-vidya-preserve-boot-v5' > > $ git am m/v5_20240401_vidyas_pci_add_support_for_preserving_boot_configuration.mbx > Applying: PCI: Add support for preserving boot configuration > error: patch failed: drivers/acpi/pci_root.c:1050 > error: drivers/acpi/pci_root.c: patch does not apply > Patch failed at 0001 PCI: Add support for preserving boot configuration I'm going to rebase this patch on top of linux-next and send. >> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c >> index 84030804a763..ddc2b3e89111 100644 >> --- a/drivers/acpi/pci_root.c >> +++ b/drivers/acpi/pci_root.c >> @@ -1008,7 +1008,6 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root, >> int node = acpi_get_node(device->handle); >> struct pci_bus *bus; >> struct pci_host_bridge *host_bridge; >> - union acpi_object *obj; >> >> info->root = root; >> info->bridge = device; >> @@ -1050,17 +1049,6 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root, >> if (!(root->osc_ext_control_set & OSC_CXL_ERROR_REPORTING_CONTROL)) >> host_bridge->native_cxl_error = 0; >> >> - /* >> - * Evaluate the "PCI Boot Configuration" _DSM Function. If it >> - * exists and returns 0, we must preserve any PCI resource >> - * assignments made by firmware for this host bridge. >> - */ >> - obj = acpi_evaluate_dsm(ACPI_HANDLE(bus->bridge), &pci_acpi_dsm_guid, 1, >> - DSM_PCI_PRESERVE_BOOT_CONFIG, NULL); >> - if (obj && obj->type == ACPI_TYPE_INTEGER && obj->integer.value == 0) >> - host_bridge->preserve_config = 1; >> - ACPI_FREE(obj); >> - >> acpi_dev_power_up_children_with_adr(device); >> >> pci_scan_child_bus(bus); >> diff --git a/drivers/pci/controller/pci-host-common.c b/drivers/pci/controller/pci-host-common.c >> index 6be3266cd7b5..e2602e38ae45 100644 >> --- a/drivers/pci/controller/pci-host-common.c >> +++ b/drivers/pci/controller/pci-host-common.c >> @@ -73,10 +73,6 @@ int pci_host_common_probe(struct platform_device *pdev) >> if (IS_ERR(cfg)) >> return PTR_ERR(cfg); >> >> - /* Do not reassign resources if probe only */ >> - if (!pci_has_flag(PCI_PROBE_ONLY)) >> - pci_add_flags(PCI_REASSIGN_ALL_BUS); >> - >> bridge->sysdata = cfg; >> bridge->ops = (struct pci_ops *)&ops->pci_ops; >> bridge->msi_domain = true; >> diff --git a/drivers/pci/of.c b/drivers/pci/of.c >> index 51e3dd0ea5ab..e6da3654f9ac 100644 >> --- a/drivers/pci/of.c >> +++ b/drivers/pci/of.c >> @@ -239,24 +239,61 @@ EXPORT_SYMBOL_GPL(of_get_pci_domain_nr); >> */ >> void of_pci_check_probe_only(void) >> { >> - u32 val; >> + bool is_preserve_config = of_pci_bridge_preserve_resources(of_chosen); > No need for a bool here: > > if (of_pci_bridge_preserve_resources(of_chosen)) > pci_add_flags(PCI_PROBE_ONLY); > > I think it would make more sense to add > of_pci_bridge_preserve_resources() *above* of_pci_check_probe_only() > since the usual order is to define functions earlier in the file than > the calls. I'll take care of it in the next patch. >> + >> + if (is_preserve_config) >> + pci_add_flags(PCI_PROBE_ONLY); >> + else >> + pci_clear_flags(PCI_PROBE_ONLY); > Not related to *this* patch, but I see that of_pci_check_probe_only() > already clears PCI_PROBE_ONLY (added by f81c11af617c ("of/pci: Add > of_pci_check_probe_only to parse "linux,pci-probe-only""). > > I'm concerned about clearing PCI_PROBE_ONLY because some platforms set > this unconditionally, and I don't think they necessarily have > "linux,pci-probe-only" in DT. > > Apparently none of them currently calls of_pci_check_probe_only() so > PCI_PROBE_ONLY remains set, but clearing it here feels like a landmine > waiting for somebody to move this into a unified call path. > > I guess if we were to drop pci_clear_flags(), that should be a > separate patch in case it breaks something. As far as the current patch is concerned, since it is not going to do anything different compared to what already exists, I'll leave it at it for now and ponder over moving pci_clear_flags() to a different patch later. > >> + pr_info("PROBE_ONLY %s\n", is_preserve_config ? "enabled" : "disabled"); >> +} >> +EXPORT_SYMBOL_GPL(of_pci_check_probe_only); >> + >> +/** >> + * of_pci_bridge_preserve_resources - Return true if the boot configuration >> + * needs to be preserved >> + * @node: Device tree node. >> + * >> + * This function looks for "linux,pci-probe-only" property for a given >> + * PCI controller's node and returns true if found. It will also look in the >> + * chosen node if the property is not found in the given controller's node. >> + * Having this property ensures that the kernel doesn't reconfigure the >> + * BARs and bridge windows that are already done by the platform firmware. >> + * >> + * Return: true if the property exists false otherwise. >> + */ >> +bool of_pci_bridge_preserve_resources(struct device_node *node) >> +{ >> + u32 val = 0; >> int ret; >> >> - ret = of_property_read_u32(of_chosen, "linux,pci-probe-only", &val); >> + if (!node) { >> + pr_warn("device node is NULL, trying with of_chosen\n"); >> + node = of_chosen; >> + } >> + >> +retry: >> + ret = of_property_read_u32(node, "linux,pci-probe-only", &val); >> if (ret) { >> - if (ret == -ENODATA || ret == -EOVERFLOW) >> - pr_warn("linux,pci-probe-only without valid value, ignoring\n"); >> - return; >> + if (ret == -ENODATA || ret == -EOVERFLOW) { >> + pr_warn("Incorrect value for linux,pci-probe-only in %pOF, ignoring\n", node); >> + return false; >> + } >> + if (ret == -EINVAL) { >> + if (node == of_chosen) >> + return false; >> + >> + node = of_chosen; >> + goto retry; >> + } >> } >> >> if (val) >> - pci_add_flags(PCI_PROBE_ONLY); >> + return true; >> else >> - pci_clear_flags(PCI_PROBE_ONLY); >> - >> - pr_info("PROBE_ONLY %s\n", val ? "enabled" : "disabled"); >> + return false; >> } >> -EXPORT_SYMBOL_GPL(of_pci_check_probe_only); >> >> /** >> * devm_of_pci_get_host_bridge_resources() - Resource-managed parsing of PCI >> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c >> index 795534589b98..b0e0226a8da8 100644 >> --- a/drivers/pci/probe.c >> +++ b/drivers/pci/probe.c >> @@ -15,6 +15,7 @@ >> #include <linux/cpumask.h> >> #include <linux/aer.h> >> #include <linux/acpi.h> >> +#include <linux/pci-acpi.h> >> #include <linux/hypervisor.h> >> #include <linux/irqdomain.h> >> #include <linux/pm_runtime.h> >> @@ -877,6 +878,28 @@ static void pci_set_bus_msi_domain(struct pci_bus *bus) >> dev_set_msi_domain(&bus->dev, d); >> } >> >> +static void pci_check_config_preserve(struct pci_host_bridge *host_bridge) >> +{ >> + if (ACPI_HANDLE(&host_bridge->dev)) { >> + union acpi_object *obj; >> + >> + /* >> + * Evaluate the "PCI Boot Configuration" _DSM Function. If it >> + * exists and returns 0, we must preserve any PCI resource >> + * assignments made by firmware for this host bridge. >> + */ >> + obj = acpi_evaluate_dsm(ACPI_HANDLE(&host_bridge->dev), &pci_acpi_dsm_guid, 1, >> + DSM_PCI_PRESERVE_BOOT_CONFIG, NULL); >> + if (obj && obj->type == ACPI_TYPE_INTEGER && obj->integer.value == 0) >> + host_bridge->preserve_config = 1; >> + ACPI_FREE(obj); >> + } >> + >> + if (host_bridge->dev.parent && host_bridge->dev.parent->of_node) >> + host_bridge->preserve_config = >> + of_pci_bridge_preserve_resources(host_bridge->dev.parent->of_node); >> +} >> + >> static int pci_register_host_bridge(struct pci_host_bridge *bridge) >> { >> struct device *parent = bridge->dev.parent; >> @@ -971,6 +994,9 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge) >> if (nr_node_ids > 1 && pcibus_to_node(bus) == NUMA_NO_NODE) >> dev_warn(&bus->dev, "Unknown NUMA node; performance will be reduced\n"); >> >> + /* Check if the boot configuration by FW needs to be preserved */ >> + pci_check_config_preserve(bridge); > I really have an allergic reaction to functions named "..._check_..." > We can tell that the function has something to do with preserving > configuration, and it's void so obviously the side effects are the > important thing, but there's no clue in the caller about what the side > effects are. > > I'd prefer something like: > > bridge->preserve_config = pci_must_preserve_config(bridge); > > where pci_must_preserve_config() has no side effects, returns bool, > and the action is at the caller. I'll take care of it in the next patch. >> /* Coalesce contiguous windows */ >> resource_list_for_each_entry_safe(window, n, &resources) { >> if (list_is_last(&window->node, &resources)) >> @@ -3080,20 +3106,18 @@ int pci_host_probe(struct pci_host_bridge *bridge) >> >> bus = bridge->bus; >> >> + /* If we must preserve the resource configuration, claim now */ >> + if (bridge->preserve_config) >> + pci_bus_claim_resources(bus); >> + >> /* >> - * We insert PCI resources into the iomem_resource and >> - * ioport_resource trees in either pci_bus_claim_resources() >> - * or pci_bus_assign_resources(). >> + * Assign whatever was left unassigned. If we didn't claim above, >> + * this will reassign everything. >> */ >> - if (pci_has_flag(PCI_PROBE_ONLY)) { >> - pci_bus_claim_resources(bus); >> - } else { >> - pci_bus_size_bridges(bus); >> - pci_bus_assign_resources(bus); >> + pci_assign_unassigned_root_bus_resources(bus); >> >> - list_for_each_entry(child, &bus->children, node) >> - pcie_bus_configure_settings(child); >> - } >> + list_for_each_entry(child, &bus->children, node) >> + pcie_bus_configure_settings(child); >> >> pci_bus_add_devices(bus); >> return 0; >> diff --git a/include/linux/of_pci.h b/include/linux/of_pci.h >> index 29658c0ee71f..3f3909a5d55d 100644 >> --- a/include/linux/of_pci.h >> +++ b/include/linux/of_pci.h >> @@ -13,6 +13,7 @@ struct device_node *of_pci_find_child_device(struct device_node *parent, >> unsigned int devfn); >> int of_pci_get_devfn(struct device_node *np); >> void of_pci_check_probe_only(void); >> +bool of_pci_bridge_preserve_resources(struct device_node *node); > This looks like it should be in drivers/pci/pci.h since it's not used > outside drivers/pci/. Agree. I'll take care of it in the next patch. >> #else >> static inline struct device_node *of_pci_find_child_device(struct device_node *parent, >> unsigned int devfn) >> @@ -26,6 +27,11 @@ static inline int of_pci_get_devfn(struct device_node *np) >> } >> >> static inline void of_pci_check_probe_only(void) { } >> + >> +static inline bool of_pci_bridge_preserve_resources(struct device_node *node) >> +{ >> + return false; >> +} >> #endif >> >> #if IS_ENABLED(CONFIG_OF_IRQ) >> -- >> 2.25.1 >> _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 58+ messages in thread
* [PATCH V6] PCI: Add support for preserving boot configuration 2024-04-01 7:50 ` Vidya Sagar @ 2024-04-18 17:40 ` Vidya Sagar -1 siblings, 0 replies; 58+ messages in thread From: Vidya Sagar @ 2024-04-18 17:40 UTC (permalink / raw) To: bhelgaas, rafael, lenb, will, lpieralisi, kw, robh, frowand.list Cc: linux-pci, linux-acpi, linux-kernel, linux-arm-kernel, devicetree, treding, jonathanh, kthota, mmaddireddy, vidyas, sagar.tv Add support for preserving the boot configuration done by the platform firmware per host bridge basis, based on the presence of 'linux,pci-probe-only' property in the respective PCI host bridge device-tree node. It also unifies the ACPI and DT based boot flows in this regard. Signed-off-by: Vidya Sagar <vidyas@nvidia.com> --- V5: * Addressed Rob's review comments V4: * Addressed Bjorn's review comments V3: * Unified ACPI and DT flows as part of addressing Bjorn's review comments V2: * Addressed issues reported by kernel test robot <lkp@intel.com> drivers/acpi/pci_root.c | 12 ------ drivers/pci/controller/pci-host-common.c | 4 -- drivers/pci/of.c | 54 +++++++++++++++++++----- drivers/pci/pci.h | 7 +++ drivers/pci/probe.c | 48 ++++++++++++++++----- 5 files changed, 88 insertions(+), 37 deletions(-) diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c index 58b89b8d950e..ddc2b3e89111 100644 --- a/drivers/acpi/pci_root.c +++ b/drivers/acpi/pci_root.c @@ -1008,7 +1008,6 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root, int node = acpi_get_node(device->handle); struct pci_bus *bus; struct pci_host_bridge *host_bridge; - union acpi_object *obj; info->root = root; info->bridge = device; @@ -1050,17 +1049,6 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root, if (!(root->osc_ext_control_set & OSC_CXL_ERROR_REPORTING_CONTROL)) host_bridge->native_cxl_error = 0; - /* - * Evaluate the "PCI Boot Configuration" _DSM Function. If it - * exists and returns 0, we must preserve any PCI resource - * assignments made by firmware for this host bridge. - */ - obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(bus->bridge), &pci_acpi_dsm_guid, 1, - DSM_PCI_PRESERVE_BOOT_CONFIG, NULL, ACPI_TYPE_INTEGER); - if (obj && obj->integer.value == 0) - host_bridge->preserve_config = 1; - ACPI_FREE(obj); - acpi_dev_power_up_children_with_adr(device); pci_scan_child_bus(bus); diff --git a/drivers/pci/controller/pci-host-common.c b/drivers/pci/controller/pci-host-common.c index 45b71806182d..c96b2de163b5 100644 --- a/drivers/pci/controller/pci-host-common.c +++ b/drivers/pci/controller/pci-host-common.c @@ -73,10 +73,6 @@ int pci_host_common_probe(struct platform_device *pdev) if (IS_ERR(cfg)) return PTR_ERR(cfg); - /* Do not reassign resources if probe only */ - if (!pci_has_flag(PCI_PROBE_ONLY)) - pci_add_flags(PCI_REASSIGN_ALL_BUS); - bridge->sysdata = cfg; bridge->ops = (struct pci_ops *)&ops->pci_ops; bridge->msi_domain = true; diff --git a/drivers/pci/of.c b/drivers/pci/of.c index 51e3dd0ea5ab..aeacff742706 100644 --- a/drivers/pci/of.c +++ b/drivers/pci/of.c @@ -234,27 +234,61 @@ int of_get_pci_domain_nr(struct device_node *node) EXPORT_SYMBOL_GPL(of_get_pci_domain_nr); /** - * of_pci_check_probe_only - Setup probe only mode if linux,pci-probe-only - * is present and valid + * of_pci_bridge_preserve_resources - Return true if the boot configuration + * needs to be preserved + * @node: Device tree node. + * + * This function looks for "linux,pci-probe-only" property for a given + * PCI controller's node and returns true if found. It will also look in the + * chosen node if the property is not found in the given controller's node. + * Having this property ensures that the kernel doesn't reconfigure the + * BARs and bridge windows that are already done by the platform firmware. + * + * Return: true if the property exists false otherwise. */ -void of_pci_check_probe_only(void) +bool of_pci_bridge_preserve_resources(struct device_node *node) { - u32 val; + u32 val = 0; int ret; - ret = of_property_read_u32(of_chosen, "linux,pci-probe-only", &val); + if (!node) { + pr_warn("device node is NULL, trying with of_chosen\n"); + node = of_chosen; + } + +retry: + ret = of_property_read_u32(node, "linux,pci-probe-only", &val); if (ret) { - if (ret == -ENODATA || ret == -EOVERFLOW) - pr_warn("linux,pci-probe-only without valid value, ignoring\n"); - return; + if (ret == -ENODATA || ret == -EOVERFLOW) { + pr_warn("Incorrect value for linux,pci-probe-only in %pOF, ignoring\n", + node); + return false; + } + if (ret == -EINVAL) { + if (node == of_chosen) + return false; + + node = of_chosen; + goto retry; + } } if (val) + return true; + else + return false; +} + +/** + * of_pci_check_probe_only - Setup probe only mode if linux,pci-probe-only + * is present and valid + */ +void of_pci_check_probe_only(void) +{ + if (of_pci_bridge_preserve_resources(of_chosen)) pci_add_flags(PCI_PROBE_ONLY); else pci_clear_flags(PCI_PROBE_ONLY); - - pr_info("PROBE_ONLY %s\n", val ? "enabled" : "disabled"); } EXPORT_SYMBOL_GPL(of_pci_check_probe_only); diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h index 17fed1846847..1da54ab18534 100644 --- a/drivers/pci/pci.h +++ b/drivers/pci/pci.h @@ -650,6 +650,7 @@ int of_pci_get_max_link_speed(struct device_node *node); u32 of_pci_get_slot_power_limit(struct device_node *node, u8 *slot_power_limit_value, u8 *slot_power_limit_scale); +bool of_pci_bridge_preserve_resources(struct device_node *node); int pci_set_of_node(struct pci_dev *dev); void pci_release_of_node(struct pci_dev *dev); void pci_set_bus_of_node(struct pci_bus *bus); @@ -688,6 +689,12 @@ of_pci_get_slot_power_limit(struct device_node *node, return 0; } +static inline bool +of_pci_bridge_preserve_resources(struct device_node *node) +{ + return false; +} + static inline int pci_set_of_node(struct pci_dev *dev) { return 0; } static inline void pci_release_of_node(struct pci_dev *dev) { } static inline void pci_set_bus_of_node(struct pci_bus *bus) { } diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 1325fbae2f28..ceb9129823ee 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -15,6 +15,7 @@ #include <linux/cpumask.h> #include <linux/aer.h> #include <linux/acpi.h> +#include <linux/pci-acpi.h> #include <linux/hypervisor.h> #include <linux/irqdomain.h> #include <linux/pm_runtime.h> @@ -889,6 +890,30 @@ static void pci_set_bus_msi_domain(struct pci_bus *bus) dev_set_msi_domain(&bus->dev, d); } +static bool pci_must_preserve_config(struct pci_host_bridge *host_bridge) +{ + if (ACPI_HANDLE(&host_bridge->dev)) { + union acpi_object *obj; + + /* + * Evaluate the "PCI Boot Configuration" _DSM Function. If it + * exists and returns 0, we must preserve any PCI resource + * assignments made by firmware for this host bridge. + */ + obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(&host_bridge->dev), &pci_acpi_dsm_guid, + 1, DSM_PCI_PRESERVE_BOOT_CONFIG, NULL, + ACPI_TYPE_INTEGER); + if (obj && obj->integer.value == 0) + return true; + ACPI_FREE(obj); + } + + if (host_bridge->dev.parent && host_bridge->dev.parent->of_node) + return of_pci_bridge_preserve_resources(host_bridge->dev.parent->of_node); + + return false; +} + static int pci_register_host_bridge(struct pci_host_bridge *bridge) { struct device *parent = bridge->dev.parent; @@ -983,6 +1008,9 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge) if (nr_node_ids > 1 && pcibus_to_node(bus) == NUMA_NO_NODE) dev_warn(&bus->dev, "Unknown NUMA node; performance will be reduced\n"); + /* Check if the boot configuration by FW needs to be preserved */ + bridge->preserve_config = pci_must_preserve_config(bridge); + /* Coalesce contiguous windows */ resource_list_for_each_entry_safe(window, n, &resources) { if (list_is_last(&window->node, &resources)) @@ -3074,20 +3102,18 @@ int pci_host_probe(struct pci_host_bridge *bridge) bus = bridge->bus; + /* If we must preserve the resource configuration, claim now */ + if (bridge->preserve_config) + pci_bus_claim_resources(bus); + /* - * We insert PCI resources into the iomem_resource and - * ioport_resource trees in either pci_bus_claim_resources() - * or pci_bus_assign_resources(). + * Assign whatever was left unassigned. If we didn't claim above, + * this will reassign everything. */ - if (pci_has_flag(PCI_PROBE_ONLY)) { - pci_bus_claim_resources(bus); - } else { - pci_bus_size_bridges(bus); - pci_bus_assign_resources(bus); + pci_assign_unassigned_root_bus_resources(bus); - list_for_each_entry(child, &bus->children, node) - pcie_bus_configure_settings(child); - } + list_for_each_entry(child, &bus->children, node) + pcie_bus_configure_settings(child); pci_bus_add_devices(bus); return 0; -- 2.25.1 ^ permalink raw reply related [flat|nested] 58+ messages in thread
* [PATCH V6] PCI: Add support for preserving boot configuration @ 2024-04-18 17:40 ` Vidya Sagar 0 siblings, 0 replies; 58+ messages in thread From: Vidya Sagar @ 2024-04-18 17:40 UTC (permalink / raw) To: bhelgaas, rafael, lenb, will, lpieralisi, kw, robh, frowand.list Cc: linux-pci, linux-acpi, linux-kernel, linux-arm-kernel, devicetree, treding, jonathanh, kthota, mmaddireddy, vidyas, sagar.tv Add support for preserving the boot configuration done by the platform firmware per host bridge basis, based on the presence of 'linux,pci-probe-only' property in the respective PCI host bridge device-tree node. It also unifies the ACPI and DT based boot flows in this regard. Signed-off-by: Vidya Sagar <vidyas@nvidia.com> --- V5: * Addressed Rob's review comments V4: * Addressed Bjorn's review comments V3: * Unified ACPI and DT flows as part of addressing Bjorn's review comments V2: * Addressed issues reported by kernel test robot <lkp@intel.com> drivers/acpi/pci_root.c | 12 ------ drivers/pci/controller/pci-host-common.c | 4 -- drivers/pci/of.c | 54 +++++++++++++++++++----- drivers/pci/pci.h | 7 +++ drivers/pci/probe.c | 48 ++++++++++++++++----- 5 files changed, 88 insertions(+), 37 deletions(-) diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c index 58b89b8d950e..ddc2b3e89111 100644 --- a/drivers/acpi/pci_root.c +++ b/drivers/acpi/pci_root.c @@ -1008,7 +1008,6 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root, int node = acpi_get_node(device->handle); struct pci_bus *bus; struct pci_host_bridge *host_bridge; - union acpi_object *obj; info->root = root; info->bridge = device; @@ -1050,17 +1049,6 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root, if (!(root->osc_ext_control_set & OSC_CXL_ERROR_REPORTING_CONTROL)) host_bridge->native_cxl_error = 0; - /* - * Evaluate the "PCI Boot Configuration" _DSM Function. If it - * exists and returns 0, we must preserve any PCI resource - * assignments made by firmware for this host bridge. - */ - obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(bus->bridge), &pci_acpi_dsm_guid, 1, - DSM_PCI_PRESERVE_BOOT_CONFIG, NULL, ACPI_TYPE_INTEGER); - if (obj && obj->integer.value == 0) - host_bridge->preserve_config = 1; - ACPI_FREE(obj); - acpi_dev_power_up_children_with_adr(device); pci_scan_child_bus(bus); diff --git a/drivers/pci/controller/pci-host-common.c b/drivers/pci/controller/pci-host-common.c index 45b71806182d..c96b2de163b5 100644 --- a/drivers/pci/controller/pci-host-common.c +++ b/drivers/pci/controller/pci-host-common.c @@ -73,10 +73,6 @@ int pci_host_common_probe(struct platform_device *pdev) if (IS_ERR(cfg)) return PTR_ERR(cfg); - /* Do not reassign resources if probe only */ - if (!pci_has_flag(PCI_PROBE_ONLY)) - pci_add_flags(PCI_REASSIGN_ALL_BUS); - bridge->sysdata = cfg; bridge->ops = (struct pci_ops *)&ops->pci_ops; bridge->msi_domain = true; diff --git a/drivers/pci/of.c b/drivers/pci/of.c index 51e3dd0ea5ab..aeacff742706 100644 --- a/drivers/pci/of.c +++ b/drivers/pci/of.c @@ -234,27 +234,61 @@ int of_get_pci_domain_nr(struct device_node *node) EXPORT_SYMBOL_GPL(of_get_pci_domain_nr); /** - * of_pci_check_probe_only - Setup probe only mode if linux,pci-probe-only - * is present and valid + * of_pci_bridge_preserve_resources - Return true if the boot configuration + * needs to be preserved + * @node: Device tree node. + * + * This function looks for "linux,pci-probe-only" property for a given + * PCI controller's node and returns true if found. It will also look in the + * chosen node if the property is not found in the given controller's node. + * Having this property ensures that the kernel doesn't reconfigure the + * BARs and bridge windows that are already done by the platform firmware. + * + * Return: true if the property exists false otherwise. */ -void of_pci_check_probe_only(void) +bool of_pci_bridge_preserve_resources(struct device_node *node) { - u32 val; + u32 val = 0; int ret; - ret = of_property_read_u32(of_chosen, "linux,pci-probe-only", &val); + if (!node) { + pr_warn("device node is NULL, trying with of_chosen\n"); + node = of_chosen; + } + +retry: + ret = of_property_read_u32(node, "linux,pci-probe-only", &val); if (ret) { - if (ret == -ENODATA || ret == -EOVERFLOW) - pr_warn("linux,pci-probe-only without valid value, ignoring\n"); - return; + if (ret == -ENODATA || ret == -EOVERFLOW) { + pr_warn("Incorrect value for linux,pci-probe-only in %pOF, ignoring\n", + node); + return false; + } + if (ret == -EINVAL) { + if (node == of_chosen) + return false; + + node = of_chosen; + goto retry; + } } if (val) + return true; + else + return false; +} + +/** + * of_pci_check_probe_only - Setup probe only mode if linux,pci-probe-only + * is present and valid + */ +void of_pci_check_probe_only(void) +{ + if (of_pci_bridge_preserve_resources(of_chosen)) pci_add_flags(PCI_PROBE_ONLY); else pci_clear_flags(PCI_PROBE_ONLY); - - pr_info("PROBE_ONLY %s\n", val ? "enabled" : "disabled"); } EXPORT_SYMBOL_GPL(of_pci_check_probe_only); diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h index 17fed1846847..1da54ab18534 100644 --- a/drivers/pci/pci.h +++ b/drivers/pci/pci.h @@ -650,6 +650,7 @@ int of_pci_get_max_link_speed(struct device_node *node); u32 of_pci_get_slot_power_limit(struct device_node *node, u8 *slot_power_limit_value, u8 *slot_power_limit_scale); +bool of_pci_bridge_preserve_resources(struct device_node *node); int pci_set_of_node(struct pci_dev *dev); void pci_release_of_node(struct pci_dev *dev); void pci_set_bus_of_node(struct pci_bus *bus); @@ -688,6 +689,12 @@ of_pci_get_slot_power_limit(struct device_node *node, return 0; } +static inline bool +of_pci_bridge_preserve_resources(struct device_node *node) +{ + return false; +} + static inline int pci_set_of_node(struct pci_dev *dev) { return 0; } static inline void pci_release_of_node(struct pci_dev *dev) { } static inline void pci_set_bus_of_node(struct pci_bus *bus) { } diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 1325fbae2f28..ceb9129823ee 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -15,6 +15,7 @@ #include <linux/cpumask.h> #include <linux/aer.h> #include <linux/acpi.h> +#include <linux/pci-acpi.h> #include <linux/hypervisor.h> #include <linux/irqdomain.h> #include <linux/pm_runtime.h> @@ -889,6 +890,30 @@ static void pci_set_bus_msi_domain(struct pci_bus *bus) dev_set_msi_domain(&bus->dev, d); } +static bool pci_must_preserve_config(struct pci_host_bridge *host_bridge) +{ + if (ACPI_HANDLE(&host_bridge->dev)) { + union acpi_object *obj; + + /* + * Evaluate the "PCI Boot Configuration" _DSM Function. If it + * exists and returns 0, we must preserve any PCI resource + * assignments made by firmware for this host bridge. + */ + obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(&host_bridge->dev), &pci_acpi_dsm_guid, + 1, DSM_PCI_PRESERVE_BOOT_CONFIG, NULL, + ACPI_TYPE_INTEGER); + if (obj && obj->integer.value == 0) + return true; + ACPI_FREE(obj); + } + + if (host_bridge->dev.parent && host_bridge->dev.parent->of_node) + return of_pci_bridge_preserve_resources(host_bridge->dev.parent->of_node); + + return false; +} + static int pci_register_host_bridge(struct pci_host_bridge *bridge) { struct device *parent = bridge->dev.parent; @@ -983,6 +1008,9 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge) if (nr_node_ids > 1 && pcibus_to_node(bus) == NUMA_NO_NODE) dev_warn(&bus->dev, "Unknown NUMA node; performance will be reduced\n"); + /* Check if the boot configuration by FW needs to be preserved */ + bridge->preserve_config = pci_must_preserve_config(bridge); + /* Coalesce contiguous windows */ resource_list_for_each_entry_safe(window, n, &resources) { if (list_is_last(&window->node, &resources)) @@ -3074,20 +3102,18 @@ int pci_host_probe(struct pci_host_bridge *bridge) bus = bridge->bus; + /* If we must preserve the resource configuration, claim now */ + if (bridge->preserve_config) + pci_bus_claim_resources(bus); + /* - * We insert PCI resources into the iomem_resource and - * ioport_resource trees in either pci_bus_claim_resources() - * or pci_bus_assign_resources(). + * Assign whatever was left unassigned. If we didn't claim above, + * this will reassign everything. */ - if (pci_has_flag(PCI_PROBE_ONLY)) { - pci_bus_claim_resources(bus); - } else { - pci_bus_size_bridges(bus); - pci_bus_assign_resources(bus); + pci_assign_unassigned_root_bus_resources(bus); - list_for_each_entry(child, &bus->children, node) - pcie_bus_configure_settings(child); - } + list_for_each_entry(child, &bus->children, node) + pcie_bus_configure_settings(child); pci_bus_add_devices(bus); return 0; -- 2.25.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 58+ messages in thread
* Re: [PATCH V6] PCI: Add support for preserving boot configuration 2024-04-18 17:40 ` Vidya Sagar @ 2024-04-21 19:03 ` Bjorn Helgaas -1 siblings, 0 replies; 58+ messages in thread From: Bjorn Helgaas @ 2024-04-21 19:03 UTC (permalink / raw) To: Vidya Sagar Cc: bhelgaas, rafael, lenb, will, lpieralisi, kw, robh, frowand.list, linux-pci, linux-acpi, linux-kernel, linux-arm-kernel, devicetree, treding, jonathanh, kthota, mmaddireddy, sagar.tv On Thu, Apr 18, 2024 at 11:10:43PM +0530, Vidya Sagar wrote: > Add support for preserving the boot configuration done by the > platform firmware per host bridge basis, based on the presence of > 'linux,pci-probe-only' property in the respective PCI host bridge > device-tree node. It also unifies the ACPI and DT based boot flows > in this regard. This looks great; my only comments are: - It might make sense to move the _DSM evaluation to pci-acpi.c instead of probe.c. - There are a few different things going on (move _DSM, look at DT host bridge in addition to of_chosen, integrate ACPI and DT flows, tweak usage of PCI_PROBE_ONLY vs bridge->preserve_config, use pci_assign_unassigned_root_bus_resources() when we didn't before), and I think they could be split into multiple patches to make it easier to read. I took a stab at splitting this, but didn't quite complete it. I'll post that as v7-incomplete, and if you compare with v6, you'll see what's missing (the PCI_PROBE_ONLY and assign resources stuff -- there's some subtlety there that wasn't obvious to me). Bjorn ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH V6] PCI: Add support for preserving boot configuration @ 2024-04-21 19:03 ` Bjorn Helgaas 0 siblings, 0 replies; 58+ messages in thread From: Bjorn Helgaas @ 2024-04-21 19:03 UTC (permalink / raw) To: Vidya Sagar Cc: bhelgaas, rafael, lenb, will, lpieralisi, kw, robh, frowand.list, linux-pci, linux-acpi, linux-kernel, linux-arm-kernel, devicetree, treding, jonathanh, kthota, mmaddireddy, sagar.tv On Thu, Apr 18, 2024 at 11:10:43PM +0530, Vidya Sagar wrote: > Add support for preserving the boot configuration done by the > platform firmware per host bridge basis, based on the presence of > 'linux,pci-probe-only' property in the respective PCI host bridge > device-tree node. It also unifies the ACPI and DT based boot flows > in this regard. This looks great; my only comments are: - It might make sense to move the _DSM evaluation to pci-acpi.c instead of probe.c. - There are a few different things going on (move _DSM, look at DT host bridge in addition to of_chosen, integrate ACPI and DT flows, tweak usage of PCI_PROBE_ONLY vs bridge->preserve_config, use pci_assign_unassigned_root_bus_resources() when we didn't before), and I think they could be split into multiple patches to make it easier to read. I took a stab at splitting this, but didn't quite complete it. I'll post that as v7-incomplete, and if you compare with v6, you'll see what's missing (the PCI_PROBE_ONLY and assign resources stuff -- there's some subtlety there that wasn't obvious to me). Bjorn _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 58+ messages in thread
* [PATCH v7-incomplete 0/3] PCI: Add support for preserving boot configuration 2024-04-18 17:40 ` Vidya Sagar @ 2024-04-21 19:09 ` Bjorn Helgaas -1 siblings, 0 replies; 58+ messages in thread From: Bjorn Helgaas @ 2024-04-21 19:09 UTC (permalink / raw) To: Vidya Sagar Cc: Rafael J . Wysocki, Len Brown, Will Deacon, Lorenzo Pieralisi, Krzysztof Wilczyński, Rob Herring, Frank Rowand, Thierry Reding, Jonathan Hunter, Krishna Thota, Manikanta Maddireddy, Vidya Sagar, linux-pci, linux-acpi, linux-kernel, linux-arm-kernel, devicetree, Bjorn Helgaas From: Bjorn Helgaas <bhelgaas@google.com> This is Vidya's work; I started splitting it into separate patches. This is based on v6 (https://lore.kernel.org/r/20240418174043.3750240-1-vidyas@nvidia.com) but is missing a few pieces, so it's not complete yet. I just wanted to show the idea of what I had in mind. The missing pieces are: - Set PCI_REASSIGN_ALL_BUS if !PCI_PROBE_ONLY - Use bridge->preserve_config instead of PCI_PROBE_ONLY - Change to use pci_assign_unassigned_root_bus_resources() instead of pci_bus_size_bridge() and pci_bus_assign_resources() Bjorn Helgaas (3): PCI: Move PRESERVE_BOOT_CONFIG _DSM evaluation to pci_register_host_bridge() PCI: of: Add of_pci_preserve_config() for per-host bridge support PCI: Unify ACPI and DT 'preserve config' support drivers/acpi/pci_root.c | 12 --------- drivers/pci/of.c | 60 ++++++++++++++++++++++++++++++++--------- drivers/pci/pci-acpi.c | 22 +++++++++++++++ drivers/pci/pci.h | 12 +++++++++ drivers/pci/probe.c | 14 ++++++++++ 5 files changed, 95 insertions(+), 25 deletions(-) -- 2.34.1 ^ permalink raw reply [flat|nested] 58+ messages in thread
* [PATCH v7-incomplete 0/3] PCI: Add support for preserving boot configuration @ 2024-04-21 19:09 ` Bjorn Helgaas 0 siblings, 0 replies; 58+ messages in thread From: Bjorn Helgaas @ 2024-04-21 19:09 UTC (permalink / raw) To: Vidya Sagar Cc: Rafael J . Wysocki, Len Brown, Will Deacon, Lorenzo Pieralisi, Krzysztof Wilczyński, Rob Herring, Frank Rowand, Thierry Reding, Jonathan Hunter, Krishna Thota, Manikanta Maddireddy, Vidya Sagar, linux-pci, linux-acpi, linux-kernel, linux-arm-kernel, devicetree, Bjorn Helgaas From: Bjorn Helgaas <bhelgaas@google.com> This is Vidya's work; I started splitting it into separate patches. This is based on v6 (https://lore.kernel.org/r/20240418174043.3750240-1-vidyas@nvidia.com) but is missing a few pieces, so it's not complete yet. I just wanted to show the idea of what I had in mind. The missing pieces are: - Set PCI_REASSIGN_ALL_BUS if !PCI_PROBE_ONLY - Use bridge->preserve_config instead of PCI_PROBE_ONLY - Change to use pci_assign_unassigned_root_bus_resources() instead of pci_bus_size_bridge() and pci_bus_assign_resources() Bjorn Helgaas (3): PCI: Move PRESERVE_BOOT_CONFIG _DSM evaluation to pci_register_host_bridge() PCI: of: Add of_pci_preserve_config() for per-host bridge support PCI: Unify ACPI and DT 'preserve config' support drivers/acpi/pci_root.c | 12 --------- drivers/pci/of.c | 60 ++++++++++++++++++++++++++++++++--------- drivers/pci/pci-acpi.c | 22 +++++++++++++++ drivers/pci/pci.h | 12 +++++++++ drivers/pci/probe.c | 14 ++++++++++ 5 files changed, 95 insertions(+), 25 deletions(-) -- 2.34.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 58+ messages in thread
* [PATCH v7-incomplete 1/3] PCI: Move PRESERVE_BOOT_CONFIG _DSM evaluation to pci_register_host_bridge() 2024-04-21 19:09 ` Bjorn Helgaas @ 2024-04-21 19:09 ` Bjorn Helgaas -1 siblings, 0 replies; 58+ messages in thread From: Bjorn Helgaas @ 2024-04-21 19:09 UTC (permalink / raw) To: Vidya Sagar Cc: Rafael J . Wysocki, Len Brown, Will Deacon, Lorenzo Pieralisi, Krzysztof Wilczyński, Rob Herring, Frank Rowand, Thierry Reding, Jonathan Hunter, Krishna Thota, Manikanta Maddireddy, Vidya Sagar, linux-pci, linux-acpi, linux-kernel, linux-arm-kernel, devicetree, Bjorn Helgaas From: Bjorn Helgaas <bhelgaas@google.com> Move the PRESERVE_BOOT_CONFIG _DSM evaluation from acpi_pci_root_create() to pci_register_host_bridge(). This will help unify the ACPI _DSM path and the DT-based "linux,pci-probe-only" paths. This should be safe because it happens earlier than it used to: acpi_pci_root_create pci_create_root_bus pci_register_host_bridge + bridge->preserve_config = pci_preserve_config(bridge) pci_acpi_preserve_config + acpi_evaluate_dsm_typed(DSM_PCI_PRESERVE_BOOT_CONFIG) - acpi_evaluate_dsm_typed(DSM_PCI_PRESERVE_BOOT_CONFIG) No functional change intended. --- drivers/acpi/pci_root.c | 12 ------------ drivers/pci/pci-acpi.c | 22 ++++++++++++++++++++++ drivers/pci/pci.h | 5 +++++ drivers/pci/probe.c | 11 +++++++++++ 4 files changed, 38 insertions(+), 12 deletions(-) diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c index 58b89b8d950e..ddc2b3e89111 100644 --- a/drivers/acpi/pci_root.c +++ b/drivers/acpi/pci_root.c @@ -1008,7 +1008,6 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root, int node = acpi_get_node(device->handle); struct pci_bus *bus; struct pci_host_bridge *host_bridge; - union acpi_object *obj; info->root = root; info->bridge = device; @@ -1050,17 +1049,6 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root, if (!(root->osc_ext_control_set & OSC_CXL_ERROR_REPORTING_CONTROL)) host_bridge->native_cxl_error = 0; - /* - * Evaluate the "PCI Boot Configuration" _DSM Function. If it - * exists and returns 0, we must preserve any PCI resource - * assignments made by firmware for this host bridge. - */ - obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(bus->bridge), &pci_acpi_dsm_guid, 1, - DSM_PCI_PRESERVE_BOOT_CONFIG, NULL, ACPI_TYPE_INTEGER); - if (obj && obj->integer.value == 0) - host_bridge->preserve_config = 1; - ACPI_FREE(obj); - acpi_dev_power_up_children_with_adr(device); pci_scan_child_bus(bus); diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c index 004575091596..9cc447da9475 100644 --- a/drivers/pci/pci-acpi.c +++ b/drivers/pci/pci-acpi.c @@ -119,6 +119,28 @@ phys_addr_t acpi_pci_root_get_mcfg_addr(acpi_handle handle) return (phys_addr_t)mcfg_addr; } +bool pci_acpi_preserve_config(struct pci_host_bridge *host_bridge) +{ + if (ACPI_HANDLE(&host_bridge->dev)) { + union acpi_object *obj; + + /* + * Evaluate the "PCI Boot Configuration" _DSM Function. If it + * exists and returns 0, we must preserve any PCI resource + * assignments made by firmware for this host bridge. + */ + obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(&host_bridge->dev), + &pci_acpi_dsm_guid, + 1, DSM_PCI_PRESERVE_BOOT_CONFIG, + NULL, ACPI_TYPE_INTEGER); + if (obj && obj->integer.value == 0) + return true; + ACPI_FREE(obj); + } + + return false; +} + /* _HPX PCI Setting Record (Type 0); same as _HPP */ struct hpx_type0 { u32 revision; /* Not present in _HPP */ diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h index 17fed1846847..180d3907b543 100644 --- a/drivers/pci/pci.h +++ b/drivers/pci/pci.h @@ -734,6 +734,7 @@ static inline void pci_restore_aer_state(struct pci_dev *dev) { } #endif #ifdef CONFIG_ACPI +bool pci_acpi_preserve_config(struct pci_host_bridge *bridge); int pci_acpi_program_hp_params(struct pci_dev *dev); extern const struct attribute_group pci_dev_acpi_attr_group; void pci_set_acpi_fwnode(struct pci_dev *dev); @@ -747,6 +748,10 @@ int acpi_pci_wakeup(struct pci_dev *dev, bool enable); bool acpi_pci_need_resume(struct pci_dev *dev); pci_power_t acpi_pci_choose_state(struct pci_dev *pdev); #else +static inline bool pci_acpi_preserve_config(struct pci_host_bridge *bridge) +{ + return false; +} static inline int pci_dev_acpi_reset(struct pci_dev *dev, bool probe) { return -ENOTTY; diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 1325fbae2f28..ee086d029450 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -889,6 +889,14 @@ static void pci_set_bus_msi_domain(struct pci_bus *bus) dev_set_msi_domain(&bus->dev, d); } +static bool pci_preserve_config(struct pci_host_bridge *host_bridge) +{ + if (pci_acpi_preserve_config(host_bridge)) + return true; + + return false; +} + static int pci_register_host_bridge(struct pci_host_bridge *bridge) { struct device *parent = bridge->dev.parent; @@ -983,6 +991,9 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge) if (nr_node_ids > 1 && pcibus_to_node(bus) == NUMA_NO_NODE) dev_warn(&bus->dev, "Unknown NUMA node; performance will be reduced\n"); + /* Check if the boot configuration by FW needs to be preserved */ + bridge->preserve_config = pci_preserve_config(bridge); + /* Coalesce contiguous windows */ resource_list_for_each_entry_safe(window, n, &resources) { if (list_is_last(&window->node, &resources)) -- 2.34.1 ^ permalink raw reply related [flat|nested] 58+ messages in thread
* [PATCH v7-incomplete 1/3] PCI: Move PRESERVE_BOOT_CONFIG _DSM evaluation to pci_register_host_bridge() @ 2024-04-21 19:09 ` Bjorn Helgaas 0 siblings, 0 replies; 58+ messages in thread From: Bjorn Helgaas @ 2024-04-21 19:09 UTC (permalink / raw) To: Vidya Sagar Cc: Rafael J . Wysocki, Len Brown, Will Deacon, Lorenzo Pieralisi, Krzysztof Wilczyński, Rob Herring, Frank Rowand, Thierry Reding, Jonathan Hunter, Krishna Thota, Manikanta Maddireddy, Vidya Sagar, linux-pci, linux-acpi, linux-kernel, linux-arm-kernel, devicetree, Bjorn Helgaas From: Bjorn Helgaas <bhelgaas@google.com> Move the PRESERVE_BOOT_CONFIG _DSM evaluation from acpi_pci_root_create() to pci_register_host_bridge(). This will help unify the ACPI _DSM path and the DT-based "linux,pci-probe-only" paths. This should be safe because it happens earlier than it used to: acpi_pci_root_create pci_create_root_bus pci_register_host_bridge + bridge->preserve_config = pci_preserve_config(bridge) pci_acpi_preserve_config + acpi_evaluate_dsm_typed(DSM_PCI_PRESERVE_BOOT_CONFIG) - acpi_evaluate_dsm_typed(DSM_PCI_PRESERVE_BOOT_CONFIG) No functional change intended. --- drivers/acpi/pci_root.c | 12 ------------ drivers/pci/pci-acpi.c | 22 ++++++++++++++++++++++ drivers/pci/pci.h | 5 +++++ drivers/pci/probe.c | 11 +++++++++++ 4 files changed, 38 insertions(+), 12 deletions(-) diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c index 58b89b8d950e..ddc2b3e89111 100644 --- a/drivers/acpi/pci_root.c +++ b/drivers/acpi/pci_root.c @@ -1008,7 +1008,6 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root, int node = acpi_get_node(device->handle); struct pci_bus *bus; struct pci_host_bridge *host_bridge; - union acpi_object *obj; info->root = root; info->bridge = device; @@ -1050,17 +1049,6 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root, if (!(root->osc_ext_control_set & OSC_CXL_ERROR_REPORTING_CONTROL)) host_bridge->native_cxl_error = 0; - /* - * Evaluate the "PCI Boot Configuration" _DSM Function. If it - * exists and returns 0, we must preserve any PCI resource - * assignments made by firmware for this host bridge. - */ - obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(bus->bridge), &pci_acpi_dsm_guid, 1, - DSM_PCI_PRESERVE_BOOT_CONFIG, NULL, ACPI_TYPE_INTEGER); - if (obj && obj->integer.value == 0) - host_bridge->preserve_config = 1; - ACPI_FREE(obj); - acpi_dev_power_up_children_with_adr(device); pci_scan_child_bus(bus); diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c index 004575091596..9cc447da9475 100644 --- a/drivers/pci/pci-acpi.c +++ b/drivers/pci/pci-acpi.c @@ -119,6 +119,28 @@ phys_addr_t acpi_pci_root_get_mcfg_addr(acpi_handle handle) return (phys_addr_t)mcfg_addr; } +bool pci_acpi_preserve_config(struct pci_host_bridge *host_bridge) +{ + if (ACPI_HANDLE(&host_bridge->dev)) { + union acpi_object *obj; + + /* + * Evaluate the "PCI Boot Configuration" _DSM Function. If it + * exists and returns 0, we must preserve any PCI resource + * assignments made by firmware for this host bridge. + */ + obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(&host_bridge->dev), + &pci_acpi_dsm_guid, + 1, DSM_PCI_PRESERVE_BOOT_CONFIG, + NULL, ACPI_TYPE_INTEGER); + if (obj && obj->integer.value == 0) + return true; + ACPI_FREE(obj); + } + + return false; +} + /* _HPX PCI Setting Record (Type 0); same as _HPP */ struct hpx_type0 { u32 revision; /* Not present in _HPP */ diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h index 17fed1846847..180d3907b543 100644 --- a/drivers/pci/pci.h +++ b/drivers/pci/pci.h @@ -734,6 +734,7 @@ static inline void pci_restore_aer_state(struct pci_dev *dev) { } #endif #ifdef CONFIG_ACPI +bool pci_acpi_preserve_config(struct pci_host_bridge *bridge); int pci_acpi_program_hp_params(struct pci_dev *dev); extern const struct attribute_group pci_dev_acpi_attr_group; void pci_set_acpi_fwnode(struct pci_dev *dev); @@ -747,6 +748,10 @@ int acpi_pci_wakeup(struct pci_dev *dev, bool enable); bool acpi_pci_need_resume(struct pci_dev *dev); pci_power_t acpi_pci_choose_state(struct pci_dev *pdev); #else +static inline bool pci_acpi_preserve_config(struct pci_host_bridge *bridge) +{ + return false; +} static inline int pci_dev_acpi_reset(struct pci_dev *dev, bool probe) { return -ENOTTY; diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 1325fbae2f28..ee086d029450 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -889,6 +889,14 @@ static void pci_set_bus_msi_domain(struct pci_bus *bus) dev_set_msi_domain(&bus->dev, d); } +static bool pci_preserve_config(struct pci_host_bridge *host_bridge) +{ + if (pci_acpi_preserve_config(host_bridge)) + return true; + + return false; +} + static int pci_register_host_bridge(struct pci_host_bridge *bridge) { struct device *parent = bridge->dev.parent; @@ -983,6 +991,9 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge) if (nr_node_ids > 1 && pcibus_to_node(bus) == NUMA_NO_NODE) dev_warn(&bus->dev, "Unknown NUMA node; performance will be reduced\n"); + /* Check if the boot configuration by FW needs to be preserved */ + bridge->preserve_config = pci_preserve_config(bridge); + /* Coalesce contiguous windows */ resource_list_for_each_entry_safe(window, n, &resources) { if (list_is_last(&window->node, &resources)) -- 2.34.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 58+ messages in thread
* Re: [PATCH v7-incomplete 1/3] PCI: Move PRESERVE_BOOT_CONFIG _DSM evaluation to pci_register_host_bridge() 2024-04-21 19:09 ` Bjorn Helgaas @ 2024-04-22 7:45 ` Andy Shevchenko -1 siblings, 0 replies; 58+ messages in thread From: Andy Shevchenko @ 2024-04-22 7:45 UTC (permalink / raw) To: Bjorn Helgaas Cc: Vidya Sagar, Rafael J . Wysocki, Len Brown, Will Deacon, Lorenzo Pieralisi, Krzysztof Wilczyński, Rob Herring, Frank Rowand, Thierry Reding, Jonathan Hunter, Krishna Thota, Manikanta Maddireddy, Vidya Sagar, linux-pci, linux-acpi, linux-kernel, linux-arm-kernel, devicetree, Bjorn Helgaas Sun, Apr 21, 2024 at 02:09:12PM -0500, Bjorn Helgaas kirjoitti: > From: Bjorn Helgaas <bhelgaas@google.com> > > Move the PRESERVE_BOOT_CONFIG _DSM evaluation from acpi_pci_root_create() > to pci_register_host_bridge(). > > This will help unify the ACPI _DSM path and the DT-based > "linux,pci-probe-only" paths. ... > +bool pci_acpi_preserve_config(struct pci_host_bridge *host_bridge) > +{ > + if (ACPI_HANDLE(&host_bridge->dev)) { Wouldn't the below looks nicer if you invert the conditional? handle = ACPI_HANDLE(...); if (!handle) return false; ... > + union acpi_object *obj; > + > + /* > + * Evaluate the "PCI Boot Configuration" _DSM Function. If it > + * exists and returns 0, we must preserve any PCI resource > + * assignments made by firmware for this host bridge. > + */ > + obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(&host_bridge->dev), > + &pci_acpi_dsm_guid, > + 1, DSM_PCI_PRESERVE_BOOT_CONFIG, > + NULL, ACPI_TYPE_INTEGER); > + if (obj && obj->integer.value == 0) > + return true; > + ACPI_FREE(obj); > + } > + > + return false; > +} -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v7-incomplete 1/3] PCI: Move PRESERVE_BOOT_CONFIG _DSM evaluation to pci_register_host_bridge() @ 2024-04-22 7:45 ` Andy Shevchenko 0 siblings, 0 replies; 58+ messages in thread From: Andy Shevchenko @ 2024-04-22 7:45 UTC (permalink / raw) To: Bjorn Helgaas Cc: Vidya Sagar, Rafael J . Wysocki, Len Brown, Will Deacon, Lorenzo Pieralisi, Krzysztof Wilczyński, Rob Herring, Frank Rowand, Thierry Reding, Jonathan Hunter, Krishna Thota, Manikanta Maddireddy, Vidya Sagar, linux-pci, linux-acpi, linux-kernel, linux-arm-kernel, devicetree, Bjorn Helgaas Sun, Apr 21, 2024 at 02:09:12PM -0500, Bjorn Helgaas kirjoitti: > From: Bjorn Helgaas <bhelgaas@google.com> > > Move the PRESERVE_BOOT_CONFIG _DSM evaluation from acpi_pci_root_create() > to pci_register_host_bridge(). > > This will help unify the ACPI _DSM path and the DT-based > "linux,pci-probe-only" paths. ... > +bool pci_acpi_preserve_config(struct pci_host_bridge *host_bridge) > +{ > + if (ACPI_HANDLE(&host_bridge->dev)) { Wouldn't the below looks nicer if you invert the conditional? handle = ACPI_HANDLE(...); if (!handle) return false; ... > + union acpi_object *obj; > + > + /* > + * Evaluate the "PCI Boot Configuration" _DSM Function. If it > + * exists and returns 0, we must preserve any PCI resource > + * assignments made by firmware for this host bridge. > + */ > + obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(&host_bridge->dev), > + &pci_acpi_dsm_guid, > + 1, DSM_PCI_PRESERVE_BOOT_CONFIG, > + NULL, ACPI_TYPE_INTEGER); > + if (obj && obj->integer.value == 0) > + return true; > + ACPI_FREE(obj); > + } > + > + return false; > +} -- With Best Regards, Andy Shevchenko _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 58+ messages in thread
* [PATCH v7-incomplete 2/3] PCI: of: Add of_pci_preserve_config() for per-host bridge support 2024-04-21 19:09 ` Bjorn Helgaas @ 2024-04-21 19:09 ` Bjorn Helgaas -1 siblings, 0 replies; 58+ messages in thread From: Bjorn Helgaas @ 2024-04-21 19:09 UTC (permalink / raw) To: Vidya Sagar Cc: Rafael J . Wysocki, Len Brown, Will Deacon, Lorenzo Pieralisi, Krzysztof Wilczyński, Rob Herring, Frank Rowand, Thierry Reding, Jonathan Hunter, Krishna Thota, Manikanta Maddireddy, Vidya Sagar, linux-pci, linux-acpi, linux-kernel, linux-arm-kernel, devicetree, Bjorn Helgaas From: Bjorn Helgaas <bhelgaas@google.com> Add of_pci_preserve_config() to look for the "linux,pci-probe-only" property under a specified node. If it's not found there, look under "of_chosen" in addition. If the caller didn't specify a node, look under "of_chosen". With a future patch, this will support "linux,pci-probe-only" on a per host bridge basis based on the presence of the property in the respective PCI host bridge DT node. Implement of_pci_check_probe_only() using of_pci_preserve_config(). --- drivers/pci/of.c | 60 +++++++++++++++++++++++++++++++++++++---------- drivers/pci/pci.h | 7 ++++++ 2 files changed, 54 insertions(+), 13 deletions(-) diff --git a/drivers/pci/of.c b/drivers/pci/of.c index 51e3dd0ea5ab..d21c0bed36f3 100644 --- a/drivers/pci/of.c +++ b/drivers/pci/of.c @@ -233,28 +233,62 @@ int of_get_pci_domain_nr(struct device_node *node) } EXPORT_SYMBOL_GPL(of_get_pci_domain_nr); +/** + * of_pci_preserve_config - Return true if the boot configuration needs to + * be preserved + * @node: Device tree node. + * + * This function looks for "linux,pci-probe-only" property for a given + * PCI controller's node and returns true if found. It will also look in the + * chosen node if the property is not found in the given controller's node. + * Having this property ensures that the kernel doesn't reconfigure the + * BARs and bridge windows that are already done by the platform firmware. + * + * Return: true if the property exists false otherwise. + */ +bool of_pci_preserve_config(struct device_node *node) +{ + u32 val = 0; + int ret; + + if (!node) { + pr_warn("device node is NULL, trying with of_chosen\n"); + node = of_chosen; + } + +retry: + ret = of_property_read_u32(node, "linux,pci-probe-only", &val); + if (ret) { + if (ret == -ENODATA || ret == -EOVERFLOW) { + pr_warn("Incorrect value for linux,pci-probe-only in %pOF, ignoring\n", + node); + return false; + } + if (ret == -EINVAL) { + if (node == of_chosen) + return false; + + node = of_chosen; + goto retry; + } + } + + if (val) + return true; + else + return false; +} + /** * of_pci_check_probe_only - Setup probe only mode if linux,pci-probe-only * is present and valid */ void of_pci_check_probe_only(void) { - u32 val; - int ret; - - ret = of_property_read_u32(of_chosen, "linux,pci-probe-only", &val); - if (ret) { - if (ret == -ENODATA || ret == -EOVERFLOW) - pr_warn("linux,pci-probe-only without valid value, ignoring\n"); - return; - } - - if (val) + if (of_pci_preserve_config(of_chosen)) pci_add_flags(PCI_PROBE_ONLY); else pci_clear_flags(PCI_PROBE_ONLY); - - pr_info("PROBE_ONLY %s\n", val ? "enabled" : "disabled"); } EXPORT_SYMBOL_GPL(of_pci_check_probe_only); diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h index 180d3907b543..feedbedd65df 100644 --- a/drivers/pci/pci.h +++ b/drivers/pci/pci.h @@ -650,6 +650,7 @@ int of_pci_get_max_link_speed(struct device_node *node); u32 of_pci_get_slot_power_limit(struct device_node *node, u8 *slot_power_limit_value, u8 *slot_power_limit_scale); +bool of_pci_preserve_config(struct device_node *node); int pci_set_of_node(struct pci_dev *dev); void pci_release_of_node(struct pci_dev *dev); void pci_set_bus_of_node(struct pci_bus *bus); @@ -688,6 +689,12 @@ of_pci_get_slot_power_limit(struct device_node *node, return 0; } +static inline bool +of_pci_preserve_config(struct device_node *node) +{ + return false; +} + static inline int pci_set_of_node(struct pci_dev *dev) { return 0; } static inline void pci_release_of_node(struct pci_dev *dev) { } static inline void pci_set_bus_of_node(struct pci_bus *bus) { } -- 2.34.1 ^ permalink raw reply related [flat|nested] 58+ messages in thread
* [PATCH v7-incomplete 2/3] PCI: of: Add of_pci_preserve_config() for per-host bridge support @ 2024-04-21 19:09 ` Bjorn Helgaas 0 siblings, 0 replies; 58+ messages in thread From: Bjorn Helgaas @ 2024-04-21 19:09 UTC (permalink / raw) To: Vidya Sagar Cc: Rafael J . Wysocki, Len Brown, Will Deacon, Lorenzo Pieralisi, Krzysztof Wilczyński, Rob Herring, Frank Rowand, Thierry Reding, Jonathan Hunter, Krishna Thota, Manikanta Maddireddy, Vidya Sagar, linux-pci, linux-acpi, linux-kernel, linux-arm-kernel, devicetree, Bjorn Helgaas From: Bjorn Helgaas <bhelgaas@google.com> Add of_pci_preserve_config() to look for the "linux,pci-probe-only" property under a specified node. If it's not found there, look under "of_chosen" in addition. If the caller didn't specify a node, look under "of_chosen". With a future patch, this will support "linux,pci-probe-only" on a per host bridge basis based on the presence of the property in the respective PCI host bridge DT node. Implement of_pci_check_probe_only() using of_pci_preserve_config(). --- drivers/pci/of.c | 60 +++++++++++++++++++++++++++++++++++++---------- drivers/pci/pci.h | 7 ++++++ 2 files changed, 54 insertions(+), 13 deletions(-) diff --git a/drivers/pci/of.c b/drivers/pci/of.c index 51e3dd0ea5ab..d21c0bed36f3 100644 --- a/drivers/pci/of.c +++ b/drivers/pci/of.c @@ -233,28 +233,62 @@ int of_get_pci_domain_nr(struct device_node *node) } EXPORT_SYMBOL_GPL(of_get_pci_domain_nr); +/** + * of_pci_preserve_config - Return true if the boot configuration needs to + * be preserved + * @node: Device tree node. + * + * This function looks for "linux,pci-probe-only" property for a given + * PCI controller's node and returns true if found. It will also look in the + * chosen node if the property is not found in the given controller's node. + * Having this property ensures that the kernel doesn't reconfigure the + * BARs and bridge windows that are already done by the platform firmware. + * + * Return: true if the property exists false otherwise. + */ +bool of_pci_preserve_config(struct device_node *node) +{ + u32 val = 0; + int ret; + + if (!node) { + pr_warn("device node is NULL, trying with of_chosen\n"); + node = of_chosen; + } + +retry: + ret = of_property_read_u32(node, "linux,pci-probe-only", &val); + if (ret) { + if (ret == -ENODATA || ret == -EOVERFLOW) { + pr_warn("Incorrect value for linux,pci-probe-only in %pOF, ignoring\n", + node); + return false; + } + if (ret == -EINVAL) { + if (node == of_chosen) + return false; + + node = of_chosen; + goto retry; + } + } + + if (val) + return true; + else + return false; +} + /** * of_pci_check_probe_only - Setup probe only mode if linux,pci-probe-only * is present and valid */ void of_pci_check_probe_only(void) { - u32 val; - int ret; - - ret = of_property_read_u32(of_chosen, "linux,pci-probe-only", &val); - if (ret) { - if (ret == -ENODATA || ret == -EOVERFLOW) - pr_warn("linux,pci-probe-only without valid value, ignoring\n"); - return; - } - - if (val) + if (of_pci_preserve_config(of_chosen)) pci_add_flags(PCI_PROBE_ONLY); else pci_clear_flags(PCI_PROBE_ONLY); - - pr_info("PROBE_ONLY %s\n", val ? "enabled" : "disabled"); } EXPORT_SYMBOL_GPL(of_pci_check_probe_only); diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h index 180d3907b543..feedbedd65df 100644 --- a/drivers/pci/pci.h +++ b/drivers/pci/pci.h @@ -650,6 +650,7 @@ int of_pci_get_max_link_speed(struct device_node *node); u32 of_pci_get_slot_power_limit(struct device_node *node, u8 *slot_power_limit_value, u8 *slot_power_limit_scale); +bool of_pci_preserve_config(struct device_node *node); int pci_set_of_node(struct pci_dev *dev); void pci_release_of_node(struct pci_dev *dev); void pci_set_bus_of_node(struct pci_bus *bus); @@ -688,6 +689,12 @@ of_pci_get_slot_power_limit(struct device_node *node, return 0; } +static inline bool +of_pci_preserve_config(struct device_node *node) +{ + return false; +} + static inline int pci_set_of_node(struct pci_dev *dev) { return 0; } static inline void pci_release_of_node(struct pci_dev *dev) { } static inline void pci_set_bus_of_node(struct pci_bus *bus) { } -- 2.34.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 58+ messages in thread
* [PATCH v7-incomplete 3/3] PCI: Unify ACPI and DT 'preserve config' support 2024-04-21 19:09 ` Bjorn Helgaas @ 2024-04-21 19:09 ` Bjorn Helgaas -1 siblings, 0 replies; 58+ messages in thread From: Bjorn Helgaas @ 2024-04-21 19:09 UTC (permalink / raw) To: Vidya Sagar Cc: Rafael J . Wysocki, Len Brown, Will Deacon, Lorenzo Pieralisi, Krzysztof Wilczyński, Rob Herring, Frank Rowand, Thierry Reding, Jonathan Hunter, Krishna Thota, Manikanta Maddireddy, Vidya Sagar, linux-pci, linux-acpi, linux-kernel, linux-arm-kernel, devicetree, Bjorn Helgaas From: Bjorn Helgaas <bhelgaas@google.com> --- drivers/pci/probe.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index ee086d029450..2c232c22d6af 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -894,6 +894,9 @@ static bool pci_preserve_config(struct pci_host_bridge *host_bridge) if (pci_acpi_preserve_config(host_bridge)) return true; + if (host_bridge->dev.parent && host_bridge->dev.parent->of_node) + return of_pci_preserve_config(host_bridge->dev.parent->of_node); + return false; } -- 2.34.1 ^ permalink raw reply related [flat|nested] 58+ messages in thread
* [PATCH v7-incomplete 3/3] PCI: Unify ACPI and DT 'preserve config' support @ 2024-04-21 19:09 ` Bjorn Helgaas 0 siblings, 0 replies; 58+ messages in thread From: Bjorn Helgaas @ 2024-04-21 19:09 UTC (permalink / raw) To: Vidya Sagar Cc: Rafael J . Wysocki, Len Brown, Will Deacon, Lorenzo Pieralisi, Krzysztof Wilczyński, Rob Herring, Frank Rowand, Thierry Reding, Jonathan Hunter, Krishna Thota, Manikanta Maddireddy, Vidya Sagar, linux-pci, linux-acpi, linux-kernel, linux-arm-kernel, devicetree, Bjorn Helgaas From: Bjorn Helgaas <bhelgaas@google.com> --- drivers/pci/probe.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index ee086d029450..2c232c22d6af 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -894,6 +894,9 @@ static bool pci_preserve_config(struct pci_host_bridge *host_bridge) if (pci_acpi_preserve_config(host_bridge)) return true; + if (host_bridge->dev.parent && host_bridge->dev.parent->of_node) + return of_pci_preserve_config(host_bridge->dev.parent->of_node); + return false; } -- 2.34.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 58+ messages in thread
* Re: [PATCH v7-incomplete 3/3] PCI: Unify ACPI and DT 'preserve config' support 2024-04-21 19:09 ` Bjorn Helgaas @ 2024-04-22 7:43 ` Andy Shevchenko -1 siblings, 0 replies; 58+ messages in thread From: Andy Shevchenko @ 2024-04-22 7:43 UTC (permalink / raw) To: Bjorn Helgaas Cc: Vidya Sagar, Rafael J . Wysocki, Len Brown, Will Deacon, Lorenzo Pieralisi, Krzysztof Wilczyński, Rob Herring, Frank Rowand, Thierry Reding, Jonathan Hunter, Krishna Thota, Manikanta Maddireddy, Vidya Sagar, linux-pci, linux-acpi, linux-kernel, linux-arm-kernel, devicetree, Bjorn Helgaas Sun, Apr 21, 2024 at 02:09:14PM -0500, Bjorn Helgaas kirjoitti: ... > + if (host_bridge->dev.parent && host_bridge->dev.parent->of_node) > + return of_pci_preserve_config(host_bridge->dev.parent->of_node); If you need fwnode or of_node from struct device, please avoid direct dereference, we have dev_fwnode() of_fwnode_handle() dev_of_node() -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v7-incomplete 3/3] PCI: Unify ACPI and DT 'preserve config' support @ 2024-04-22 7:43 ` Andy Shevchenko 0 siblings, 0 replies; 58+ messages in thread From: Andy Shevchenko @ 2024-04-22 7:43 UTC (permalink / raw) To: Bjorn Helgaas Cc: Vidya Sagar, Rafael J . Wysocki, Len Brown, Will Deacon, Lorenzo Pieralisi, Krzysztof Wilczyński, Rob Herring, Frank Rowand, Thierry Reding, Jonathan Hunter, Krishna Thota, Manikanta Maddireddy, Vidya Sagar, linux-pci, linux-acpi, linux-kernel, linux-arm-kernel, devicetree, Bjorn Helgaas Sun, Apr 21, 2024 at 02:09:14PM -0500, Bjorn Helgaas kirjoitti: ... > + if (host_bridge->dev.parent && host_bridge->dev.parent->of_node) > + return of_pci_preserve_config(host_bridge->dev.parent->of_node); If you need fwnode or of_node from struct device, please avoid direct dereference, we have dev_fwnode() of_fwnode_handle() dev_of_node() -- With Best Regards, Andy Shevchenko _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 58+ messages in thread
end of thread, other threads:[~2024-04-22 7:46 UTC | newest] Thread overview: 58+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-01-10 3:07 [PATCH V2 0/2] Add support to preserve boot config in the DT flow Vidya Sagar 2024-01-10 3:07 ` Vidya Sagar 2024-01-10 3:07 ` [PATCH V2 1/2] dt-bindings: Add PCIe "preserve-boot-config" property Vidya Sagar 2024-01-10 3:07 ` Vidya Sagar 2024-01-12 14:33 ` Rob Herring 2024-01-12 14:33 ` Rob Herring 2024-01-10 3:07 ` [PATCH V2 2/2] PCI: Add support for " Vidya Sagar 2024-01-10 3:07 ` Vidya Sagar 2024-01-12 16:58 ` Bjorn Helgaas 2024-01-12 16:58 ` Bjorn Helgaas 2024-01-15 14:32 ` Vidya Sagar 2024-01-15 14:32 ` Vidya Sagar 2024-01-16 16:55 ` Rob Herring 2024-01-16 16:55 ` Rob Herring 2024-01-19 17:31 ` Bjorn Helgaas 2024-01-19 17:31 ` Bjorn Helgaas 2024-01-12 16:59 ` Bjorn Helgaas 2024-01-12 16:59 ` Bjorn Helgaas 2024-02-22 12:41 ` [PATCH V3] PCI: Add support for preserving boot configuration Vidya Sagar 2024-02-22 12:41 ` Vidya Sagar 2024-02-22 17:06 ` Bjorn Helgaas 2024-02-22 17:06 ` Bjorn Helgaas 2024-02-22 21:18 ` Vidya Sagar 2024-02-22 21:18 ` Vidya Sagar 2024-02-22 22:08 ` Bjorn Helgaas 2024-02-22 22:08 ` Bjorn Helgaas 2024-02-23 8:00 ` [PATCH V4] " Vidya Sagar 2024-02-23 8:00 ` Vidya Sagar 2024-03-05 14:10 ` Rob Herring 2024-03-05 14:10 ` Rob Herring 2024-03-05 14:24 ` Rob Herring 2024-03-05 14:24 ` Rob Herring 2024-04-01 7:50 ` [PATCH V5] " Vidya Sagar 2024-04-01 7:50 ` Vidya Sagar 2024-04-02 16:01 ` Rob Herring 2024-04-02 16:01 ` Rob Herring 2024-04-10 7:44 ` Vidya Sagar 2024-04-10 7:44 ` Vidya Sagar 2024-04-10 20:50 ` Bjorn Helgaas 2024-04-10 20:50 ` Bjorn Helgaas 2024-04-18 17:31 ` Vidya Sagar 2024-04-18 17:31 ` Vidya Sagar 2024-04-18 17:40 ` [PATCH V6] " Vidya Sagar 2024-04-18 17:40 ` Vidya Sagar 2024-04-21 19:03 ` Bjorn Helgaas 2024-04-21 19:03 ` Bjorn Helgaas 2024-04-21 19:09 ` [PATCH v7-incomplete 0/3] " Bjorn Helgaas 2024-04-21 19:09 ` Bjorn Helgaas 2024-04-21 19:09 ` [PATCH v7-incomplete 1/3] PCI: Move PRESERVE_BOOT_CONFIG _DSM evaluation to pci_register_host_bridge() Bjorn Helgaas 2024-04-21 19:09 ` Bjorn Helgaas 2024-04-22 7:45 ` Andy Shevchenko 2024-04-22 7:45 ` Andy Shevchenko 2024-04-21 19:09 ` [PATCH v7-incomplete 2/3] PCI: of: Add of_pci_preserve_config() for per-host bridge support Bjorn Helgaas 2024-04-21 19:09 ` Bjorn Helgaas 2024-04-21 19:09 ` [PATCH v7-incomplete 3/3] PCI: Unify ACPI and DT 'preserve config' support Bjorn Helgaas 2024-04-21 19:09 ` Bjorn Helgaas 2024-04-22 7:43 ` Andy Shevchenko 2024-04-22 7:43 ` Andy Shevchenko
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.