Re: [PATCH v2] platform/x86:intel/pmc: Ignore all LTRs during suspend

[Hans de Goede <hdegoede@redhat.com>]

Hi Xi,

On 9/10/24 2:43 AM, Xi Pardee wrote:
> Hi,
> 
> On 9/9/2024 1:07 AM, Ilpo Järvinen wrote:
>> On Fri, 6 Sep 2024, Xi Pardee wrote:
>>
>>> From: Xi Pardee <xi.pardee@intel.com>
>>>
>>> Add support to ignore all LTRs before suspend and restore the previous
>>> LTR values after suspend. This feature could be turned off with module
>>> parameter ltr_ignore_all_suspend.
>>>
>>> LTR value is a mechanism for a device to indicate tolerance to access
>>> the corresponding resource. When system suspends, the resource is not
>>> available and therefore the LTR value could be ignored. Ignoring all
>>> LTR values prevents problematic device from blocking the system to get
>>> to the deepest package state during suspend.
>>>
>>> Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>> Signed-off-by: Xi Pardee <xi.pardee@intel.com>
>>>
>>> v2:
>>> - Add more details to commit message
>>> - Fix format: ltr->LTR, S0IX->S0ix, space between name and email
>>>
>>> ---
>>>   drivers/platform/x86/intel/pmc/core.c | 53 +++++++++++++++++++++++++++
>>>   drivers/platform/x86/intel/pmc/core.h |  2 +
>>>   2 files changed, 55 insertions(+)
>>>
>>> diff --git a/drivers/platform/x86/intel/pmc/core.c b/drivers/platform/x86/intel/pmc/core.c
>>> index 01ae71c6df59..0ec703af16a4 100644
>>> --- a/drivers/platform/x86/intel/pmc/core.c
>>> +++ b/drivers/platform/x86/intel/pmc/core.c
>>> @@ -714,6 +714,49 @@ static int pmc_core_s0ix_blocker_show(struct seq_file *s, void *unused)
>>>   }
>>>   DEFINE_SHOW_ATTRIBUTE(pmc_core_s0ix_blocker);
>>>   +static void pmc_core_ltr_ignore_all(struct pmc_dev *pmcdev)
>>> +{
>>> +    unsigned int i;
>>> +
>>> +    for (i = 0; i < ARRAY_SIZE(pmcdev->pmcs); i++) {
>>> +        struct pmc *pmc;
>>> +        u32 ltr_ign;
>>> +
>>> +        pmc = pmcdev->pmcs[i];
>>> +        if (!pmc)
>>> +            continue;
>>> +
>>> +        guard(mutex)(&pmcdev->lock);
>>> +        pmc->ltr_ign = pmc_core_reg_read(pmc, pmc->map->ltr_ignore_offset);
>>> +
>>> +        /* ltr_ignore_max is the max index value for LTR ignore register */
>>> +        ltr_ign = pmc->ltr_ign | GENMASK(pmc->map->ltr_ignore_max, 0);
>>> +        pmc_core_reg_write(pmc, pmc->map->ltr_ignore_offset, ltr_ign);
>>> +    }
>>> +
>>> +    /*
>>> +     * Ignoring ME during suspend is blocking platforms with ADL PCH to get to
>>> +     * deeper S0ix substate.
>>> +     */
>>> +    pmc_core_send_ltr_ignore(pmcdev, 6, 0);
>>> +}
>>> +
>>> +static void pmc_core_ltr_restore_all(struct pmc_dev *pmcdev)
>>> +{
>>> +    unsigned int i;
>>> +
>>> +    for (i = 0; i < ARRAY_SIZE(pmcdev->pmcs); i++) {
>>> +        struct pmc *pmc;
>>> +
>>> +        pmc = pmcdev->pmcs[i];
>>> +        if (!pmc)
>>> +            continue;
>>> +
>>> +        guard(mutex)(&pmcdev->lock);
>>> +        pmc_core_reg_write(pmc, pmc->map->ltr_ignore_offset, pmc->ltr_ign);
>>> +    }
>>> +}
>>> +
>>>   static inline u64 adjust_lpm_residency(struct pmc *pmc, u32 offset,
>>>                          const int lpm_adj_x2)
>>>   {
>>> @@ -1479,6 +1522,10 @@ static bool warn_on_s0ix_failures;
>>>   module_param(warn_on_s0ix_failures, bool, 0644);
>>>   MODULE_PARM_DESC(warn_on_s0ix_failures, "Check and warn for S0ix failures");
>>>   +static bool ltr_ignore_all_suspend = true;
>>> +module_param(ltr_ignore_all_suspend, bool, 0644);
>>> +MODULE_PARM_DESC(ltr_ignore_all_suspend, "Ignore all LTRs during suspend");
>>> +
>>>   static __maybe_unused int pmc_core_suspend(struct device *dev)
>>>   {
>>>       struct pmc_dev *pmcdev = dev_get_drvdata(dev);
>>> @@ -1488,6 +1535,9 @@ static __maybe_unused int pmc_core_suspend(struct device *dev)
>>>       if (pmcdev->suspend)
>>>           pmcdev->suspend(pmcdev);
>>>   +    if (ltr_ignore_all_suspend)
>>> +        pmc_core_ltr_ignore_all(pmcdev);
>>> +
>>>       /* Check if the syspend will actually use S0ix */
>>>       if (pm_suspend_via_firmware())
>>>           return 0;
>>> @@ -1594,6 +1644,9 @@ static __maybe_unused int pmc_core_resume(struct device *dev)
>>>   {
>>>       struct pmc_dev *pmcdev = dev_get_drvdata(dev);
>>>   +    if (ltr_ignore_all_suspend)
>>> +        pmc_core_ltr_restore_all(pmcdev);
>>> +
>>>       if (pmcdev->resume)
>>>           return pmcdev->resume(pmcdev);
>>>   diff --git a/drivers/platform/x86/intel/pmc/core.h b/drivers/platform/x86/intel/pmc/core.h
>>> index ea04de7eb9e8..e862ea88b816 100644
>>> --- a/drivers/platform/x86/intel/pmc/core.h
>>> +++ b/drivers/platform/x86/intel/pmc/core.h
>>> @@ -372,6 +372,7 @@ struct pmc_info {
>>>    * @map:        pointer to pmc_reg_map struct that contains platform
>>>    *            specific attributes
>>>    * @lpm_req_regs:    List of substate requirements
>>> + * @ltr_ign:        Holds LTR ignore data while suspended
>>>    *
>>>    * pmc contains info about one power management controller device.
>>>    */
>>> @@ -380,6 +381,7 @@ struct pmc {
>>>       void __iomem *regbase;
>>>       const struct pmc_reg_map *map;
>>>       u32 *lpm_req_regs;
>>> +    u32 ltr_ign;
>>>   };
>>>     /**
>>>
>> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> 
> Thanks for the Reviewed-by tag! I wonder if I need to send another version with the Reviewed-by tag for this patch to be accepted.

There is no need for a v3. I'll merge this patch into
my review-hans branch (and from there on it will move
to for-next) soon.

Regards,

Hans