diff mbox series

[V2,2/2] PCI: Add support for "preserve-boot-config" property

Message ID 20240110030725.710547-3-vidyas@nvidia.com (mailing list archive)
State New, archived
Headers show
Series Add support to preserve boot config in the DT flow | expand

Commit Message

Vidya Sagar Jan. 10, 2024, 3:07 a.m. UTC
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(-)

Comments

Bjorn Helgaas Jan. 12, 2024, 4:58 p.m. UTC | #1
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
Bjorn Helgaas Jan. 12, 2024, 4:59 p.m. UTC | #2
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.
Vidya Sagar Jan. 15, 2024, 2:32 p.m. UTC | #3
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
Rob Herring (Arm) Jan. 16, 2024, 4:55 p.m. UTC | #4
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
Bjorn Helgaas Jan. 19, 2024, 5:31 p.m. UTC | #5
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 mbox series

Patch

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)