RE: [PATCH v9 08/13] cpufreq: amd-pstate: implement suspend and resume callbacks

[Yuan, Perry]

[AMD Official Use Only - General]

Hi Viresh. 

> -----Original Message-----
> From: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
> Sent: Tuesday, December 27, 2022 10:52 AM
> To: Yuan, Perry <Perry.Yuan@xxxxxxx>
> Cc: rafael.j.wysocki@xxxxxxxxx; Limonciello, Mario
> <Mario.Limonciello@xxxxxxx>; Huang, Ray <Ray.Huang@xxxxxxx>;
> Sharma, Deepak <Deepak.Sharma@xxxxxxx>; Fontenot, Nathan
> <Nathan.Fontenot@xxxxxxx>; Deucher, Alexander
> <Alexander.Deucher@xxxxxxx>; Huang, Shimmer
> <Shimmer.Huang@xxxxxxx>; Du, Xiaojian <Xiaojian.Du@xxxxxxx>; Meng,
> Li (Jassmine) <Li.Meng@xxxxxxx>; Karny, Wyes <Wyes.Karny@xxxxxxx>;
> linux-pm@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v9 08/13] cpufreq: amd-pstate: implement suspend and
> resume callbacks
> 
> On 26-12-22, 00:34, Perry Yuan wrote:
> > From: Perry Yuan <Perry.Yuan@xxxxxxx>
> >
> > add suspend and resume support for the AMD processors by
> > amd_pstate_epp driver instance.
> >
> > When the CPPC is suspended, EPP driver will set EPP profile to 'power'
> > profile and set max/min perf to lowest perf value.
> > When resume happens, it will restore the MSR registers with previous
> > cached value.
> >
> > Acked-by: Huang Rui <ray.huang@xxxxxxx>
> > Reviewed-by: Mario Limonciello <Mario.Limonciello@xxxxxxx>
> > Signed-off-by: Perry Yuan <Perry.Yuan@xxxxxxx>
> > ---
> >  drivers/cpufreq/amd-pstate.c | 40
> > ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 40 insertions(+)
> >
> > diff --git a/drivers/cpufreq/amd-pstate.c
> > b/drivers/cpufreq/amd-pstate.c index c671f4955766..e3676d1a85c7 100644
> > --- a/drivers/cpufreq/amd-pstate.c
> > +++ b/drivers/cpufreq/amd-pstate.c
> > @@ -1041,6 +1041,44 @@ static int amd_pstate_epp_verify_policy(struct
> cpufreq_policy_data *policy)
> >  	return 0;
> >  }
> >
> > +static int amd_pstate_epp_suspend(struct cpufreq_policy *policy) {
> > +	struct amd_cpudata *cpudata = policy->driver_data;
> > +	int ret;
> > +
> > +	/* avoid suspending when EPP is not enabled */
> > +	if (cppc_state != AMD_PSTATE_ACTIVE)
> > +		return 0;
> > +
> > +	/* set this flag to avoid setting core offline*/
> > +	cpudata->suspended = true;
> > +
> > +	/* disable CPPC in lowlevel firmware */
> > +	ret = amd_pstate_enable(false);
> > +	if (ret)
> > +		pr_err("failed to suspend, return %d\n", ret);
> > +
> > +	return 0;
> > +}
> > +
> > +static int amd_pstate_epp_resume(struct cpufreq_policy *policy) {
> > +	struct amd_cpudata *cpudata = policy->driver_data;
> > +
> > +	if (cpudata->suspended) {
> 
> When will resume get called without being suspended first ?

Sorry for the late reply.
Theoretically the resume() will get called when system suspended firstly.
Checking cpudata->suspended flag to make sure eachtime resume() is called to resume the previous MSR values safely in my view.
Maybe we can drop the checking code, but it will take more time to run testing~  
So to be safe , we can keep this, I will try to do some optimization in future. 


Perry. 

> 
> > +		mutex_lock(&amd_pstate_limits_lock);
> > +
> > +		/* enable amd pstate from suspend state*/
> > +		amd_pstate_epp_reenable(cpudata);
> > +
> > +		mutex_unlock(&amd_pstate_limits_lock);
> > +
> > +		cpudata->suspended = false;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static struct cpufreq_driver amd_pstate_driver = {
> >  	.flags		= CPUFREQ_CONST_LOOPS |
> CPUFREQ_NEED_UPDATE_LIMITS,
> >  	.verify		= amd_pstate_verify,
> > @@ -1062,6 +1100,8 @@ static struct cpufreq_driver
> amd_pstate_epp_driver = {
> >  	.exit		= amd_pstate_epp_cpu_exit,
> >  	.offline	= amd_pstate_epp_cpu_offline,
> >  	.online		= amd_pstate_epp_cpu_online,
> > +	.suspend	= amd_pstate_epp_suspend,
> > +	.resume		= amd_pstate_epp_resume,
> >  	.name		= "amd_pstate_epp",
> >  	.attr		= amd_pstate_epp_attr,
> >  };
> > --
> > 2.34.1
> 
> --
> viresh