* [PATCH v2] platform/x86:intel/pmc: Ignore all LTRs during suspend
@ 2024-09-06 18:40 Xi Pardee
2024-09-09 8:07 ` Ilpo Järvinen
2024-09-11 12:16 ` Hans de Goede
0 siblings, 2 replies; 6+ messages in thread
From: Xi Pardee @ 2024-09-06 18:40 UTC (permalink / raw)
To: xi.pardee, irenic.rajneesh, david.e.box, hdegoede, ilpo.jarvinen,
platform-driver-x86, linux-kernel, linux-pm
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;
};
/**
--
2.43.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] platform/x86:intel/pmc: Ignore all LTRs during suspend
2024-09-06 18:40 [PATCH v2] platform/x86:intel/pmc: Ignore all LTRs during suspend Xi Pardee
@ 2024-09-09 8:07 ` Ilpo Järvinen
2024-09-10 0:43 ` Xi Pardee
2024-09-11 12:16 ` Hans de Goede
1 sibling, 1 reply; 6+ messages in thread
From: Ilpo Järvinen @ 2024-09-09 8:07 UTC (permalink / raw)
To: Xi Pardee
Cc: irenic.rajneesh, david.e.box, Hans de Goede, platform-driver-x86,
LKML, linux-pm
[-- Attachment #1: Type: text/plain, Size: 4631 bytes --]
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>
--
i.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] platform/x86:intel/pmc: Ignore all LTRs during suspend
2024-09-09 8:07 ` Ilpo Järvinen
@ 2024-09-10 0:43 ` Xi Pardee
2024-09-10 8:58 ` Hans de Goede
0 siblings, 1 reply; 6+ messages in thread
From: Xi Pardee @ 2024-09-10 0:43 UTC (permalink / raw)
To: Ilpo Järvinen
Cc: irenic.rajneesh, david.e.box, Hans de Goede, platform-driver-x86,
LKML, linux-pm
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.
Thanks!
Xi
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] platform/x86:intel/pmc: Ignore all LTRs during suspend
2024-09-10 0:43 ` Xi Pardee
@ 2024-09-10 8:58 ` Hans de Goede
0 siblings, 0 replies; 6+ messages in thread
From: Hans de Goede @ 2024-09-10 8:58 UTC (permalink / raw)
To: Xi Pardee, Ilpo Järvinen
Cc: irenic.rajneesh, david.e.box, platform-driver-x86, LKML, linux-pm
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] platform/x86:intel/pmc: Ignore all LTRs during suspend
2024-09-06 18:40 [PATCH v2] platform/x86:intel/pmc: Ignore all LTRs during suspend Xi Pardee
2024-09-09 8:07 ` Ilpo Järvinen
@ 2024-09-11 12:16 ` Hans de Goede
2024-09-11 12:17 ` Hans de Goede
1 sibling, 1 reply; 6+ messages in thread
From: Hans de Goede @ 2024-09-11 12:16 UTC (permalink / raw)
To: Xi Pardee, irenic.rajneesh, david.e.box, ilpo.jarvinen,
platform-driver-x86, linux-kernel, linux-pm
Hi,
On 9/6/24 8:40 PM, 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>
Thanks, patch looks good to me:
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Regards,
Hans
>
> 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;
> };
>
> /**
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] platform/x86:intel/pmc: Ignore all LTRs during suspend
2024-09-11 12:16 ` Hans de Goede
@ 2024-09-11 12:17 ` Hans de Goede
0 siblings, 0 replies; 6+ messages in thread
From: Hans de Goede @ 2024-09-11 12:17 UTC (permalink / raw)
To: Xi Pardee, irenic.rajneesh, david.e.box, ilpo.jarvinen,
platform-driver-x86, linux-kernel, linux-pm
Hi,
On 9/11/24 2:16 PM, Hans de Goede wrote:
> Hi,
>
> On 9/6/24 8:40 PM, 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>
>
> Thanks, patch looks good to me:
>
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Oops, wrong auto-reply, what I meant is:
Thank you for your patch, I've applied this patch to my review-hans
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans
Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.
Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.
Regards,
Hans
>> 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;
>> };
>>
>> /**
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-09-11 12:17 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-09-06 18:40 [PATCH v2] platform/x86:intel/pmc: Ignore all LTRs during suspend Xi Pardee
2024-09-09 8:07 ` Ilpo Järvinen
2024-09-10 0:43 ` Xi Pardee
2024-09-10 8:58 ` Hans de Goede
2024-09-11 12:16 ` Hans de Goede
2024-09-11 12:17 ` Hans de Goede
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).