Hi Jonathan,
On 5/30/25 12:02 PM, Jonathan Cameron wrote:
> On Tue, 27 May 2025 09:40:07 +0200
> Eric Auger <eric.au...@redhat.com> wrote:
>
>> acpi_dsdt_add_pci_osc() name is confusing as it gives the impression
>> it appends the _OSC method but in fact it also appends the _DSM method
>> for the host bridge. Let's split the function into two separate ones
>> and let them return the method Aml pointer instead. This matches the
>> way it is done on x86 (build_q35_osc_method). In a subsequent patch
>> we will replace the gpex method by the q35 implementation that will
>> become shared between ARM and x86.
>>
>> acpi_dsdt_add_host_bridge_methods is a new top helper that generates
>> both the _OSC and _DSM methods.
>>
>> Signed-off-by: Eric Auger <eric.au...@redhat.com>
>> Reviewed-by: Gustavo Romero <gustavo.rom...@linaro.org>
> Makes complete sense. I've had local equivalent of this on the CXL
> tree for a while as without it we don't register the _DSM for the
> CXL path (and we should). However, can you modify it a little to
> make that easier for me? Basically make sure the _DSM is registered
> for the CXL path as well.
>
> One other comment inline.
>
>
>> ---
>> hw/pci-host/gpex-acpi.c | 31 +++++++++++++++++++++----------
>> 1 file changed, 21 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/pci-host/gpex-acpi.c b/hw/pci-host/gpex-acpi.c
>> index f34b7cf25e..1aa2d12026 100644
>> --- a/hw/pci-host/gpex-acpi.c
>> +++ b/hw/pci-host/gpex-acpi.c
>> @@ -50,13 +50,10 @@ static void acpi_dsdt_add_pci_route_table(Aml *dev,
>> uint32_t irq,
>> }
>> }
>>
>> -static void acpi_dsdt_add_pci_osc(Aml *dev, bool enable_native_pcie_hotplug)
>> +static Aml *build_host_bridge_osc(bool enable_native_pcie_hotplug)
>> {
>> - Aml *method, *UUID, *ifctx, *ifctx1, *elsectx, *buf;
>> + Aml *method, *UUID, *ifctx, *ifctx1, *elsectx;
>>
>> - /* Declare an _OSC (OS Control Handoff) method */
>> - aml_append(dev, aml_name_decl("SUPP", aml_int(0)));
>> - aml_append(dev, aml_name_decl("CTRL", aml_int(0)));
>> method = aml_method("_OSC", 4, AML_NOTSERIALIZED);
>> aml_append(method,
>> aml_create_dword_field(aml_arg(3), aml_int(0), "CDW1"));
>> @@ -103,9 +100,13 @@ static void acpi_dsdt_add_pci_osc(Aml *dev, bool
>> enable_native_pcie_hotplug)
>> aml_name("CDW1")));
>> aml_append(elsectx, aml_return(aml_arg(3)));
>> aml_append(method, elsectx);
>> - aml_append(dev, method);
>> + return method;
>> +}
>>
>> - method = aml_method("_DSM", 4, AML_NOTSERIALIZED);
>> +static Aml *build_host_bridge_dsm(void)
>> +{
>> + Aml *method = aml_method("_DSM", 4, AML_NOTSERIALIZED);
>> + Aml *UUID, *ifctx, *ifctx1, *buf;
>>
>> /* PCI Firmware Specification 3.0
>> * 4.6.1. _DSM for PCI Express Slot Information
>> @@ -124,7 +125,17 @@ static void acpi_dsdt_add_pci_osc(Aml *dev, bool
>> enable_native_pcie_hotplug)
>> byte_list[0] = 0;
>> buf = aml_buffer(1, byte_list);
>> aml_append(method, aml_return(buf));
>> - aml_append(dev, method);
>> + return method;
>> +}
>> +
>> +static void acpi_dsdt_add_host_bridge_methods(Aml *dev,
>> + bool
>> enable_native_pcie_hotplug)
>> +{
>> + aml_append(dev, aml_name_decl("SUPP", aml_int(0)));
>> + aml_append(dev, aml_name_decl("CTRL", aml_int(0)));
> These two declarations seem to be very much part of the _OSC build though not
> within the the method. I 'think' you get left with them later with no users.
> So move them into the osc build here and they will naturally go away when
> you move to the generic code.
>
> They end up unused in the DSDT at the end of the series.
Done
Thanks
Eric
>
> I ran a quick GPEX + pxb-pcie test and we do get the odd mix that the OSC for
> the GPEX say no native hotplug but the OSC for the PXB allows it.
>
>> + /* Declare an _OSC (OS Control Handoff) method */
>> + aml_append(dev, build_host_bridge_osc(enable_native_pcie_hotplug));
>> + aml_append(dev, build_host_bridge_dsm());
>> }
>>
>> void acpi_dsdt_add_gpex(Aml *scope, struct GPEXConfig *cfg)
>> @@ -193,7 +204,7 @@ void acpi_dsdt_add_gpex(Aml *scope, struct GPEXConfig
>> *cfg)
>> if (is_cxl) {
>> build_cxl_osc_method(dev);
>> } else {
>> - acpi_dsdt_add_pci_osc(dev, true);
>> + acpi_dsdt_add_host_bridge_methods(dev, true);
> Can you either drop the use of the wrapper for the DSM part here and call
> it unconditionally (for cxl and PCIe cases) or add an extra call to
> aml_append(dev, build_host_bridge_dsm()) for the is_cxl path?
>
>> }
>>
>> aml_append(scope, dev);
>> @@ -268,7 +279,7 @@ void acpi_dsdt_add_gpex(Aml *scope, struct GPEXConfig
>> *cfg)
>> }
>> aml_append(dev, aml_name_decl("_CRS", rbuf));
>>
>> - acpi_dsdt_add_pci_osc(dev, true);
>> + acpi_dsdt_add_host_bridge_methods(dev, true);
>>
>> Aml *dev_res0 = aml_device("%s", "RES0");
>> aml_append(dev_res0, aml_name_decl("_HID", aml_string("PNP0C02")));
