[PATCH 06/15] PCI/ACPI: Link host bridge to its ACPI fw node

[PATCH 06/15] PCI/ACPI: Link host bridge to its ACPI fw node

[Robert Richter]

A lookup of a host bridge's corresponding acpi device (struct
acpi_device) is not possible, for example:

	adev = ACPI_COMPANION(&host_bridge->dev);

This could be useful to find a host bridge's fwnode handle and to
determine and call additional host bridge ACPI parameters and methods
such as HID/CID or _UID.

Make this work by linking the host bridge to its ACPI fw node.

Signed-off-by: Robert Richter <rrichter@amd.com>
---
 drivers/acpi/pci_root.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index d57cf8454b93..846c979e4c29 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -1083,6 +1083,7 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
 		goto out_release_info;
 
 	host_bridge = to_pci_host_bridge(bus->bridge);
+	host_bridge->dev.fwnode = acpi_fwnode_handle(device);
 	if (!(root->osc_control_set & OSC_PCI_EXPRESS_NATIVE_HP_CONTROL))
 		host_bridge->native_pcie_hotplug = 0;
 	if (!(root->osc_control_set & OSC_PCI_SHPC_NATIVE_HP_CONTROL))
-- 
2.30.2

Re: [PATCH 06/15] PCI/ACPI: Link host bridge to its ACPI fw node

[Jonathan Cameron]

On Wed, 31 Aug 2022 10:15:54 +0200
Robert Richter <rrichter@amd.com> wrote:

> A lookup of a host bridge's corresponding acpi device (struct
> acpi_device) is not possible, for example:
> 
> 	adev = ACPI_COMPANION(&host_bridge->dev);
> 
> This could be useful to find a host bridge's fwnode handle and to
> determine and call additional host bridge ACPI parameters and methods
> such as HID/CID or _UID.
> 
> Make this work by linking the host bridge to its ACPI fw node.
> 
> Signed-off-by: Robert Richter <rrichter@amd.com>

Seems sensible to me, though I'm not an expert in this are of the code.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>


> ---
>  drivers/acpi/pci_root.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index d57cf8454b93..846c979e4c29 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -1083,6 +1083,7 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
>  		goto out_release_info;
>  
>  	host_bridge = to_pci_host_bridge(bus->bridge);
> +	host_bridge->dev.fwnode = acpi_fwnode_handle(device);
>  	if (!(root->osc_control_set & OSC_PCI_EXPRESS_NATIVE_HP_CONTROL))
>  		host_bridge->native_pcie_hotplug = 0;
>  	if (!(root->osc_control_set & OSC_PCI_SHPC_NATIVE_HP_CONTROL))

Re: [PATCH 06/15] PCI/ACPI: Link host bridge to its ACPI fw node

[Bjorn Helgaas]

On Wed, Aug 31, 2022 at 10:15:54AM +0200, Robert Richter wrote:
> A lookup of a host bridge's corresponding acpi device (struct
> acpi_device) is not possible, for example:
> 
> 	adev = ACPI_COMPANION(&host_bridge->dev);
> 
> This could be useful to find a host bridge's fwnode handle and to
> determine and call additional host bridge ACPI parameters and methods
> such as HID/CID or _UID.
> 
> Make this work by linking the host bridge to its ACPI fw node.

s/acpi device/ACPI device/ to match other "ACPI" usage
s/fw node/fwnode/ (if it should match "fwnode handle" above)

I guess this patch makes ACPI_COMPANION() work where it didn't before,
right?  E.g., the two ACPI_COMPANION() uses added by this series
(cxl_find_next_rch() and cxl_restricted_host_probe()).

I'm not really clear on the strategy of when we should use acpi_device
vs acpi_handle, but does this mean there's code in places like
pci_root.c that should be reworked to take advantage of this?  That
code evaluates _DSM and _OSC, but I think it currently uses
acpi_handle for that.

Bjorn

Re: [PATCH 06/15] PCI/ACPI: Link host bridge to its ACPI fw node

[Rafael J. Wysocki]

On Wed, Sep 7, 2022 at 8:37 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Wed, Aug 31, 2022 at 10:15:54AM +0200, Robert Richter wrote:
> > A lookup of a host bridge's corresponding acpi device (struct
> > acpi_device) is not possible, for example:
> >
> >       adev = ACPI_COMPANION(&host_bridge->dev);
> >
> > This could be useful to find a host bridge's fwnode handle and to
> > determine and call additional host bridge ACPI parameters and methods
> > such as HID/CID or _UID.
> >
> > Make this work by linking the host bridge to its ACPI fw node.
>
> s/acpi device/ACPI device/ to match other "ACPI" usage
> s/fw node/fwnode/ (if it should match "fwnode handle" above)
>
> I guess this patch makes ACPI_COMPANION() work where it didn't before,
> right?  E.g., the two ACPI_COMPANION() uses added by this series
> (cxl_find_next_rch() and cxl_restricted_host_probe()).
>
> I'm not really clear on the strategy of when we should use acpi_device
> vs acpi_handle,

acpi_handle should be used for interactions with the ACPICA code, like
when AML is evaluated, and acpi_device for pretty much everything
else.

> but does this mean there's code in places like
> pci_root.c that should be reworked to take advantage of this?  That
> code evaluates _DSM and _OSC, but I think it currently uses
> acpi_handle for that.

That's fine.

RE: [PATCH 06/15] PCI/ACPI: Link host bridge to its ACPI fw node

[Dan Williams]

Robert Richter wrote:
> A lookup of a host bridge's corresponding acpi device (struct
> acpi_device) is not possible, for example:
> 
> 	adev = ACPI_COMPANION(&host_bridge->dev);
> 
> This could be useful to find a host bridge's fwnode handle and to
> determine and call additional host bridge ACPI parameters and methods
> such as HID/CID or _UID.

When is this explicitly needed. "Could be useful" is interesting, but it
needs to have a practical need.

> 
> Make this work by linking the host bridge to its ACPI fw node.
> 
> Signed-off-by: Robert Richter <rrichter@amd.com>
> ---
>  drivers/acpi/pci_root.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index d57cf8454b93..846c979e4c29 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -1083,6 +1083,7 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
>  		goto out_release_info;
>  
>  	host_bridge = to_pci_host_bridge(bus->bridge);
> +	host_bridge->dev.fwnode = acpi_fwnode_handle(device);
>  	if (!(root->osc_control_set & OSC_PCI_EXPRESS_NATIVE_HP_CONTROL))
>  		host_bridge->native_pcie_hotplug = 0;
>  	if (!(root->osc_control_set & OSC_PCI_SHPC_NATIVE_HP_CONTROL))
> -- 
> 2.30.2
> 

Re: [PATCH 06/15] PCI/ACPI: Link host bridge to its ACPI fw node

[Rafael J. Wysocki]

On Thu, Sep 8, 2022 at 8:05 AM Dan Williams <dan.j.williams@intel.com> wrote:
>
> Robert Richter wrote:
> > A lookup of a host bridge's corresponding acpi device (struct
> > acpi_device) is not possible, for example:
> >
> >       adev = ACPI_COMPANION(&host_bridge->dev);
> >
> > This could be useful to find a host bridge's fwnode handle and to
> > determine and call additional host bridge ACPI parameters and methods
> > such as HID/CID or _UID.
>
> When is this explicitly needed. "Could be useful" is interesting, but it
> needs to have a practical need.

It is needed and it is present on x86 AFAICS (see my last reply in this thread).

This seems to be addressing an ARM64-specific issue.

> >
> > Make this work by linking the host bridge to its ACPI fw node.
> >
> > Signed-off-by: Robert Richter <rrichter@amd.com>
> > ---
> >  drivers/acpi/pci_root.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> > index d57cf8454b93..846c979e4c29 100644
> > --- a/drivers/acpi/pci_root.c
> > +++ b/drivers/acpi/pci_root.c
> > @@ -1083,6 +1083,7 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
> >               goto out_release_info;
> >
> >       host_bridge = to_pci_host_bridge(bus->bridge);
> > +     host_bridge->dev.fwnode = acpi_fwnode_handle(device);
> >       if (!(root->osc_control_set & OSC_PCI_EXPRESS_NATIVE_HP_CONTROL))
> >               host_bridge->native_pcie_hotplug = 0;
> >       if (!(root->osc_control_set & OSC_PCI_SHPC_NATIVE_HP_CONTROL))
> > --
> > 2.30.2
> >
>
>

Re: [PATCH 06/15] PCI/ACPI: Link host bridge to its ACPI fw node

[Dan Williams]

Rafael J. Wysocki wrote:
> On Thu, Sep 8, 2022 at 8:05 AM Dan Williams <dan.j.williams@intel.com> wrote:
> >
> > Robert Richter wrote:
> > > A lookup of a host bridge's corresponding acpi device (struct
> > > acpi_device) is not possible, for example:
> > >
> > >       adev = ACPI_COMPANION(&host_bridge->dev);
> > >
> > > This could be useful to find a host bridge's fwnode handle and to
> > > determine and call additional host bridge ACPI parameters and methods
> > > such as HID/CID or _UID.
> >
> > When is this explicitly needed. "Could be useful" is interesting, but it
> > needs to have a practical need.
> 
> It is needed and it is present on x86 AFAICS (see my last reply in this thread).
> 
> This seems to be addressing an ARM64-specific issue.
> 

Ah, ok, thanks for pointing that out.

Re: [PATCH 06/15] PCI/ACPI: Link host bridge to its ACPI fw node

[Robert Richter]

On 08.09.22 12:45:16, Dan Williams wrote:
> Rafael J. Wysocki wrote:
> > On Thu, Sep 8, 2022 at 8:05 AM Dan Williams <dan.j.williams@intel.com> wrote:
> > >
> > > Robert Richter wrote:
> > > > A lookup of a host bridge's corresponding acpi device (struct
> > > > acpi_device) is not possible, for example:
> > > >
> > > >       adev = ACPI_COMPANION(&host_bridge->dev);
> > > >
> > > > This could be useful to find a host bridge's fwnode handle and to
> > > > determine and call additional host bridge ACPI parameters and methods
> > > > such as HID/CID or _UID.
> > >
> > > When is this explicitly needed. "Could be useful" is interesting, but it
> > > needs to have a practical need.
> > 
> > It is needed and it is present on x86 AFAICS (see my last reply in this thread).
> > 
> > This seems to be addressing an ARM64-specific issue.
> > 
> 
> Ah, ok, thanks for pointing that out.

No, it is x86. And true, it is set. So this series is actually working
without this patch. It can be dropped.

Now, I just checked my logs. The reason I was adding this is that
during code development I modified the code to have bridge->dev.parent
set. Then, the fwnode is not linked. I later dropped that change but
kept this patch.

Thanks for catching this.

-Robert

Re: [PATCH 06/15] PCI/ACPI: Link host bridge to its ACPI fw node

[Bjorn Helgaas]

On Fri, Sep 09, 2022 at 12:20:45PM +0200, Robert Richter wrote:
> On 08.09.22 12:45:16, Dan Williams wrote:
> > Rafael J. Wysocki wrote:
> > > On Thu, Sep 8, 2022 at 8:05 AM Dan Williams <dan.j.williams@intel.com> wrote:
> > > >
> > > > Robert Richter wrote:
> > > > > A lookup of a host bridge's corresponding acpi device (struct
> > > > > acpi_device) is not possible, for example:
> > > > >
> > > > >       adev = ACPI_COMPANION(&host_bridge->dev);
> > > > >
> > > > > This could be useful to find a host bridge's fwnode handle and to
> > > > > determine and call additional host bridge ACPI parameters and methods
> > > > > such as HID/CID or _UID.

> ...
> No, it is x86. And true, it is set. So this series is actually working
> without this patch. It can be dropped.
> 
> Now, I just checked my logs. The reason I was adding this is that
> during code development I modified the code to have bridge->dev.parent
> set. Then, the fwnode is not linked. I later dropped that change but
> kept this patch.

If this patch does the same thing as the ACPI_COMPANION_SET() in
several pcibios_root_bridge_prepare() implementations, I would love to
keep this patch, which does it in a generic place, and drop the
corresponding code from those arch-specific functions.

But I don't understand the fwnode stuff well enough to know if this is
feasible.

Bjorn

Re: [PATCH 06/15] PCI/ACPI: Link host bridge to its ACPI fw node

[Dan Williams]

Bjorn Helgaas wrote:
> On Fri, Sep 09, 2022 at 12:20:45PM +0200, Robert Richter wrote:
> > On 08.09.22 12:45:16, Dan Williams wrote:
> > > Rafael J. Wysocki wrote:
> > > > On Thu, Sep 8, 2022 at 8:05 AM Dan Williams <dan.j.williams@intel.com> wrote:
> > > > >
> > > > > Robert Richter wrote:
> > > > > > A lookup of a host bridge's corresponding acpi device (struct
> > > > > > acpi_device) is not possible, for example:
> > > > > >
> > > > > >       adev = ACPI_COMPANION(&host_bridge->dev);
> > > > > >
> > > > > > This could be useful to find a host bridge's fwnode handle and to
> > > > > > determine and call additional host bridge ACPI parameters and methods
> > > > > > such as HID/CID or _UID.
> 
> > ...
> > No, it is x86. And true, it is set. So this series is actually working
> > without this patch. It can be dropped.
> > 
> > Now, I just checked my logs. The reason I was adding this is that
> > during code development I modified the code to have bridge->dev.parent
> > set. Then, the fwnode is not linked. I later dropped that change but
> > kept this patch.
> 
> If this patch does the same thing as the ACPI_COMPANION_SET() in
> several pcibios_root_bridge_prepare() implementations, I would love to
> keep this patch, which does it in a generic place, and drop the
> corresponding code from those arch-specific functions.
> 
> But I don't understand the fwnode stuff well enough to know if this is
> feasible.

I took a brief look, but I could not convince myself that these lookups:

arch/ia64/pci/pci.c ((struct pci_controller *) bridge->bus->sysdata)->companion
arch/loongarch/pci/acpi.c to_acpi_device(((struct pci_config_window *) bridge->bus->sysdata)->parent)
arch/arm64/kernel/pci.c to_acpi_device(((struct pci_config_window *) bridge->bus->sysdata)->parent)
arch/x86/pci/acpi.c ((struct pci_sysdata *) bridge->bus->sysdata)->companion

...are identical to what acpi_pci_root_add() used to create the
acpi_pci_root.

Re: [PATCH 06/15] PCI/ACPI: Link host bridge to its ACPI fw node

[Rafael J. Wysocki]

On Wed, Aug 31, 2022 at 10:17 AM Robert Richter <rrichter@amd.com> wrote:
>
> A lookup of a host bridge's corresponding acpi device (struct
> acpi_device) is not possible, for example:
>
>         adev = ACPI_COMPANION(&host_bridge->dev);

Hmm.

x86 has this code in pcibios_root_bridge_prepare():

    if (!bridge->dev.parent) {
        struct pci_sysdata *sd = bridge->bus->sysdata;
        ACPI_COMPANION_SET(&bridge->dev, sd->companion);
    }

which should set the ACPI companion for the host bridge.

Moreover, on my x86 desktop /sys/devices/pci0000\:00/ (which is the
host bridge AFAICS) has

firmware_node -> ../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00

so clearly the ACPI companion is there.

Are we talking about ARM64 here?

> This could be useful to find a host bridge's fwnode handle and to
> determine and call additional host bridge ACPI parameters and methods
> such as HID/CID or _UID.
>
> Make this work by linking the host bridge to its ACPI fw node.
>
> Signed-off-by: Robert Richter <rrichter@amd.com>
> ---
>  drivers/acpi/pci_root.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index d57cf8454b93..846c979e4c29 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -1083,6 +1083,7 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
>                 goto out_release_info;
>
>         host_bridge = to_pci_host_bridge(bus->bridge);
> +       host_bridge->dev.fwnode = acpi_fwnode_handle(device);

So this would be replacing the existing mechanism on x86, right?

>         if (!(root->osc_control_set & OSC_PCI_EXPRESS_NATIVE_HP_CONTROL))
>                 host_bridge->native_pcie_hotplug = 0;
>         if (!(root->osc_control_set & OSC_PCI_SHPC_NATIVE_HP_CONTROL))
> --