| Message ID | 20240110030725.710547-3-vidyas@nvidia.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add support to preserve boot config in the DT flow | expand |
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
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.
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
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
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
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)
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(-)