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

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

>>> 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".
> 
> Intel based system.
> 
> So the final D3 state is decided by the presence of power resources?
> 

Right.  This is why I've mentioned in some of my series that 
pci_d3cold_enable() / pci_d3cold_disable() are misnamed and 
dev->no_d3cold can be misleading.

FYI - I have (slowly) been modifying this series to use 
pci_d3_cold_enable()/pci_d3_cold_disable() instead of the extra overhead 
of the policy decision but I have some problems.

I anticipate future version will also modify pci_bridge_d3_update().

>>
>>>
>>>> +       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.
> 
> So sounds like it only applies to PCI devices?

Correct.

> 
> Kai-Heng
> 
>>
>>>
>>> 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
>>>>
>>