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

[Vidya Sagar]

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@xxxxxxxxx>
> > Signed-off-by: Vidya Sagar <vidyas@xxxxxxxxxx>
> > ---
> > V2:
> > * Addressed issues reported by kernel test robot <lkp@xxxxxxxxx>
> >
> >   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