On Tue, Oct 17, 2023 at 5:34 AM Mario Limonciello <mario.limonciello@xxxxxxx> wrote: > > 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@xxxxxxx> 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@xxxxxxx> > >> --- > >> 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". Intel based system. So the final D3 state is decided by the presence of power resources? > > > > >> + 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? 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 > >> >
