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

[Xi Pardee]

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@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > Signed-off-by: Xi Pardee <xi.pardee@xxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxx>

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