Re: [PATCH 08/22] hw/pci-host/gpex-acpi: Use build_pci_host_bridge_osc_method

[Gustavo Romero <gustavo.romero@linaro.org>]

Hi Eric,

On 5/14/25 14:00, Eric Auger wrote:
> gpex build_host_bridge_osc() and x86 originated
> build_pci_host_bridge_osc_method() are mostly identical.
> 
> In GPEX, SUPP is set to CDW2 but is not further used. CTRL
> is same as Local0.
> 
> So let gpex code reuse build_pci_host_bridge_osc_method()
> and remove build_host_bridge_osc().
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> 
> ---
> 
> The DSDT diff  is given below:
> diff --git a/dsdt.dsl_before b/dsdt.dsl_after_osc_change
> index 3224a56..fa7558e 100644
> --- a/dsdt.dsl_before
> +++ b/dsdt.dsl_after_osc_change
> @@ -5,13 +5,13 @@
>    *
>    * Disassembling to symbolic ASL+ operators
>    *
> - * Disassembly of dsdt.dat, Mon Apr  7 05:33:06 2025
> + * Disassembly of dsdt.dat, Mon Apr  7 05:37:20 2025
>    *
>    * Original Table Header:
>    *     Signature        "DSDT"
> - *     Length           0x00001A4F (6735)
> + *     Length           0x00001A35 (6709)
>    *     Revision         0x02
> - *     Checksum         0xBF
> + *     Checksum         0xDD
>    *     OEM ID           "BOCHS "
>    *     OEM Table ID     "BXPC    "
>    *     OEM Revision     0x00000001 (1)
> @@ -1849,27 +1849,26 @@ DefinitionBlock ("", "DSDT", 2, "BOCHS ", "BXPC    ", 0x00000001)
>                   {
>                       CreateDWordField (Arg3, 0x04, CDW2)
>                       CreateDWordField (Arg3, 0x08, CDW3)
> -                    SUPP = CDW2 /* \_SB_.PCI0._OSC.CDW2 */
> -                    CTRL = CDW3 /* \_SB_.PCI0._OSC.CDW3 */
> -                    CTRL &= 0x1F
> +                    Local0 = CDW3 /* \_SB_.PCI0._OSC.CDW3 */
> +                    Local0 &= 0x1F
>                       If ((Arg1 != One))
>                       {
>                           CDW1 |= 0x08
>                       }
> 
> -                    If ((CDW3 != CTRL))
> +                    If ((CDW3 != Local0))
>                       {
>                           CDW1 |= 0x10
>                       }
> 
> -                    CDW3 = CTRL /* \_SB_.PCI0.CTRL */
> -                    Return (Arg3)
> +                    CDW3 = Local0
>                   }
>                   Else
>                   {
>                       CDW1 |= 0x04
> -                    Return (Arg3)
>                   }
> +
> +                Return (Arg3)
>               }
> 
>               Method (_DSM, 4, NotSerialized)  // _DSM: Device-Specific Method

The problem I face with diffs in the commit body is that tools like b4, which are
based on git am, get very confused on how to handle it. I'm surprised nobody ever
complained about it. I'm wondering if there is any catch on it, because I have to
edit commits like this manually, removing the diff, to make it finally apply to
the series. Anyways, do you mind at least removing the valid diff header, like:

> diff --git a/dsdt.dsl_before b/dsdt.dsl_after_osc_change
> index 3224a56..fa7558e 100644
> --- a/dsdt.dsl_before
> +++ b/dsdt.dsl_after_osc_change

from the commit message so it doesn't confuse b4?


> ---
>   hw/pci-host/gpex-acpi.c | 60 +++--------------------------------------
>   1 file changed, 4 insertions(+), 56 deletions(-)
> 
> diff --git a/hw/pci-host/gpex-acpi.c b/hw/pci-host/gpex-acpi.c
> index f1ab30f3d5..98c9868c3f 100644
> --- a/hw/pci-host/gpex-acpi.c
> +++ b/hw/pci-host/gpex-acpi.c
> @@ -50,60 +50,7 @@ static void acpi_dsdt_add_pci_route_table(Aml *dev, uint32_t irq,
>       }
>   }
>   
> -static Aml *build_host_bridge_osc(bool enable_native_pcie_hotplug)
> -{
> -    Aml *method, *UUID, *ifctx, *ifctx1, *elsectx;
> -
> -    method = aml_method("_OSC", 4, AML_NOTSERIALIZED);
> -    aml_append(method,
> -        aml_create_dword_field(aml_arg(3), aml_int(0), "CDW1"));
> -
> -    /* PCI Firmware Specification 3.0
> -     * 4.5.1. _OSC Interface for PCI Host Bridge Devices
> -     * The _OSC interface for a PCI/PCI-X/PCI Express hierarchy is
> -     * identified by the Universal Unique IDentifier (UUID)
> -     * 33DB4D5B-1FF7-401C-9657-7441C03DD766
> -     */
> -    UUID = aml_touuid("33DB4D5B-1FF7-401C-9657-7441C03DD766");
> -    ifctx = aml_if(aml_equal(aml_arg(0), UUID));
> -    aml_append(ifctx,
> -        aml_create_dword_field(aml_arg(3), aml_int(4), "CDW2"));
> -    aml_append(ifctx,
> -        aml_create_dword_field(aml_arg(3), aml_int(8), "CDW3"));
> -    aml_append(ifctx, aml_store(aml_name("CDW2"), aml_name("SUPP")));
> -    aml_append(ifctx, aml_store(aml_name("CDW3"), aml_name("CTRL")));
> -
> -    /*
> -     * Allow OS control for SHPCHotplug, PME, AER, PCIeCapability,
> -     * and PCIeHotplug depending on enable_native_pcie_hotplug
> -     */
> -    aml_append(ifctx, aml_and(aml_name("CTRL"),
> -               aml_int(0x1E | (enable_native_pcie_hotplug ? 0x1 : 0x0)),
> -               aml_name("CTRL")));
> -
> -    ifctx1 = aml_if(aml_lnot(aml_equal(aml_arg(1), aml_int(0x1))));
> -    aml_append(ifctx1, aml_or(aml_name("CDW1"), aml_int(0x08),
> -                              aml_name("CDW1")));
> -    aml_append(ifctx, ifctx1);
> -
> -    ifctx1 = aml_if(aml_lnot(aml_equal(aml_name("CDW3"), aml_name("CTRL"))));
> -    aml_append(ifctx1, aml_or(aml_name("CDW1"), aml_int(0x10),
> -                              aml_name("CDW1")));
> -    aml_append(ifctx, ifctx1);
> -
> -    aml_append(ifctx, aml_store(aml_name("CTRL"), aml_name("CDW3")));
> -    aml_append(ifctx, aml_return(aml_arg(3)));
> -    aml_append(method, ifctx);
> -
> -    elsectx = aml_else();
> -    aml_append(elsectx, aml_or(aml_name("CDW1"), aml_int(4),
> -                               aml_name("CDW1")));
> -    aml_append(elsectx, aml_return(aml_arg(3)));
> -    aml_append(method, elsectx);
> -    return method;
> -}
> -
> -static Aml *build_host_bridge_dsm(void)
> +static Aml *build_pci_host_bridge_dsm_method(void)
>   {
>       Aml *method = aml_method("_DSM", 4, AML_NOTSERIALIZED);
>       Aml *UUID, *ifctx, *ifctx1, *buf;
> @@ -134,8 +81,9 @@ static void acpi_dsdt_add_host_bridge_methods(Aml *dev,
>       aml_append(dev, aml_name_decl("SUPP", aml_int(0)));
>       aml_append(dev, aml_name_decl("CTRL", aml_int(0)));
>       /* 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());
> +    aml_append(dev,
> +               build_pci_host_bridge_osc_method(enable_native_pcie_hotplug));
> +    aml_append(dev, build_pci_host_bridge_dsm_method());
>   }
>   
>   void acpi_dsdt_add_gpex(Aml *scope, struct GPEXConfig *cfg)

Otherwise:

Reviewed-by: Gustavo Romero <gustavo.romero@linaro.org>


Cheers,
Gustavo