| Message ID | 20220831081603.3415-7-rrichter@amd.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | None | expand |
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))
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
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.
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 >
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)) > --
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 > > > >
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.
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
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
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.
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))
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(+)