Re: [RFC v1 4/4] platform/x86/amd: pmc: Add support for using constraints to decide D3 policy

[Mario Limonciello <mario.limonciello@amd.com>]

On 10/15/2023 21:11, Kai-Heng Feng wrote:
> Hi Mario,
> 
> On Tue, Oct 10, 2023 at 6:57 AM Mario Limonciello
> <mario.limonciello@amd.com> wrote:
>>
>> The default kernel policy will allow modern machines to effectively put
>> all PCIe bridges into PCI D3. This policy doesn't match what Windows uses.
>>
>> In Windows the driver stack includes a "Power Engine Plugin" (uPEP driver)
>> to decide the policy for integrated devices using PEP device constraints.
>>
>> Device constraints are expressed as a number in the _DSM of the PNP0D80
>> device and exported by the kernel in acpi_get_lps0_constraint().
>>
>> Add support for SoCs to use constraints on Linux as well for deciding
>> target state for integrated PCI bridges.
>>
>> No SoCs are introduced by default with this change, they will be added
>> later on a case by case basis.
>>
>> Link: https://learn.microsoft.com/en-us/windows-hardware/design/device-experiences/platform-design-for-modern-standby#low-power-core-silicon-cpu-soc-dram
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>>   drivers/platform/x86/amd/pmc/pmc.c | 59 ++++++++++++++++++++++++++++++
>>   1 file changed, 59 insertions(+)
>>
>> diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
>> index c1e788b67a74..34e76c6b3fb2 100644
>> --- a/drivers/platform/x86/amd/pmc/pmc.c
>> +++ b/drivers/platform/x86/amd/pmc/pmc.c
>> @@ -570,6 +570,14 @@ static void amd_pmc_dbgfs_unregister(struct amd_pmc_dev *dev)
>>          debugfs_remove_recursive(dev->dbgfs_dir);
>>   }
>>
>> +static bool amd_pmc_use_acpi_constraints(struct amd_pmc_dev *dev)
>> +{
>> +       switch (dev->cpu_id) {
>> +       default:
>> +               return false;
>> +       }
>> +}
>> +
>>   static bool amd_pmc_is_stb_supported(struct amd_pmc_dev *dev)
>>   {
>>          switch (dev->cpu_id) {
>> @@ -741,6 +749,41 @@ static int amd_pmc_czn_wa_irq1(struct amd_pmc_dev *pdev)
>>          return 0;
>>   }
>>
>> +/*
>> + * Constraints are specified in the ACPI LPS0 device and specify what the
>> + * platform intended for the device.
>> + *
>> + * If a constraint is present and >= to ACPI_STATE_S3, then enable D3 for the
>> + * device.
>> + * If a constraint is not present or < ACPI_STATE_S3, then disable D3 for the
>> + * device.
>> + */
>> +static enum bridge_d3_pref amd_pmc_d3_check(struct pci_dev *pci_dev)
>> +{
>> +       struct acpi_device *adev = ACPI_COMPANION(&pci_dev->dev);
>> +       int constraint;
>> +
>> +       if (!adev)
>> +               return BRIDGE_PREF_UNSET;
>> +
>> +       constraint = acpi_get_lps0_constraint(adev);
>> +       dev_dbg(&pci_dev->dev, "constraint is %d\n", constraint);
>> +
>> +       switch (constraint) {
>> +       case ACPI_STATE_UNKNOWN:
>> +       case ACPI_STATE_S0:
>> +       case ACPI_STATE_S1:
>> +       case ACPI_STATE_S2:
> 
> I believe it's a typo?
> I think ACPI_STATE_Dx should be used for device state.

Yes; typo thanks.

> 
>> +               return BRIDGE_PREF_DRIVER_D0;
>> +       case ACPI_STATE_S3:
>> +               return BRIDGE_PREF_DRIVER_D3;
> 
> I've seen both 3 (i.e. ACPI_STATE_D3_HOT) and 4 (i.e.
> ACPI_STATE_D3_COLD) defined in LPI constraint table.
> Should those two be treated differently?

Was this AMD system or Intel system?  AFAIK AMD doesn't use D3cold in 
constraints, they are collectively "D3".

> 
>> +       default:
>> +               break;
>> +       }
>> +
>> +       return BRIDGE_PREF_UNSET;
>> +}
>> +
>>   static int amd_pmc_verify_czn_rtc(struct amd_pmc_dev *pdev, u32 *arg)
>>   {
>>          struct rtc_device *rtc_device;
>> @@ -881,6 +924,11 @@ static struct acpi_s2idle_dev_ops amd_pmc_s2idle_dev_ops = {
>>          .restore = amd_pmc_s2idle_restore,
>>   };
>>
>> +static struct pci_d3_driver_ops amd_pmc_d3_ops = {
>> +       .check = amd_pmc_d3_check,
>> +       .priority = 50,
>> +};
>> +
>>   static int amd_pmc_suspend_handler(struct device *dev)
>>   {
>>          struct amd_pmc_dev *pdev = dev_get_drvdata(dev);
>> @@ -1074,10 +1122,19 @@ static int amd_pmc_probe(struct platform_device *pdev)
>>                          amd_pmc_quirks_init(dev);
>>          }
>>
>> +       if (amd_pmc_use_acpi_constraints(dev)) {
>> +               err = pci_register_driver_d3_policy_cb(&amd_pmc_d3_ops);
>> +               if (err)
>> +                       goto err_register_lps0;
>> +       }
> 
> Does this only apply to PCI? USB can have ACPI companion too.

It only applies to things in the constraints, which is only "SOC 
internal" devices.  No internal USB devices.

> 
> Kai-Heng
> 
>> +
>>          amd_pmc_dbgfs_register(dev);
>>          pm_report_max_hw_sleep(U64_MAX);
>>          return 0;
>>
>> +err_register_lps0:
>> +       if (IS_ENABLED(CONFIG_SUSPEND))
>> +               acpi_unregister_lps0_dev(&amd_pmc_s2idle_dev_ops);
>>   err_pci_dev_put:
>>          pci_dev_put(rdev);
>>          return err;
>> @@ -1089,6 +1146,8 @@ static void amd_pmc_remove(struct platform_device *pdev)
>>
>>          if (IS_ENABLED(CONFIG_SUSPEND))
>>                  acpi_unregister_lps0_dev(&amd_pmc_s2idle_dev_ops);
>> +       if (amd_pmc_use_acpi_constraints(dev))
>> +               pci_unregister_driver_d3_policy_cb(&amd_pmc_d3_ops);
>>          amd_pmc_dbgfs_unregister(dev);
>>          pci_dev_put(dev->rdev);
>>          mutex_destroy(&dev->lock);
>> --
>> 2.34.1
>>