Messages in this thread Patch in this message |  | | Date | Fri, 9 Sep 2022 14:00:53 -0500 | Subject | Re: [PATCH 7/7] cpufreq: amd-pstate: implement suspend and resume callbacks | From | "Limonciello, Mario" <> |
| |
On 9/9/2022 11:45, Perry Yuan wrote: > 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. > > Signed-off-by: Perry Yuan <Perry.Yuan@amd.com> > --- > drivers/cpufreq/amd-pstate.c | 39 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 39 insertions(+) > > diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c > index e63fed39f90c..749083d28b05 100644 > --- a/drivers/cpufreq/amd-pstate.c > +++ b/drivers/cpufreq/amd-pstate.c > @@ -1476,6 +1476,43 @@ static int amd_pstate_epp_cpu_offline(struct cpufreq_policy *policy) > return amd_pstate_cpu_offline(policy); > } > > +static int amd_pstate_epp_suspend(struct cpufreq_policy *policy) > +{ > + struct amd_cpudata *cpudata = all_cpu_data[policy->cpu]; > + int ret;
I don't see an explicit guard in here to only run this code for epp mode. If you want it to be running both for EPP and non-EPP then you should update the commit message. If you only want it running for EPP, I would think you need a:
if (!epp_enabled) return 0;
> + > + pr_debug("AMD CPU Core %d suspending\n", cpudata->cpu);
This debug statement seems needlessly noisy to me (even for dyndbg). Unless they're for debugging synchronization problems, I would think that you can get a similar result using ftrace for function names.
> + > + cpudata->suspended = true; > + > + /* disable CPPC in lowlevel firmware */ > + ret = amd_pstate_enable(false); > + if (ret) > + pr_err("failed to disable amd pstate during suspend, return %d\n", ret);
amd-pstate uses pr_fmt. You don't need to mention so much in your error message. Something like this would suffice:
pr_err("failed to suspend: %d\n, ret);
> + > + return 0;
Shouldn't you be returning ret here?
> +} > + > +static int amd_pstate_epp_resume(struct cpufreq_policy *policy) > +{ > + struct amd_cpudata *cpudata = all_cpu_data[policy->cpu]; > + > + pr_debug("AMD CPU Core %d resuming\n", cpudata->cpu);
Ditto on above comments.
> + > + if (cpudata->suspended && epp_enabled) {
If you end up adopting the suggestion above for checking epp_enabled in suspend, I don't belivee you also need to check in resume. Your check for cpudata->suspended will make sure this only runs if you did something for suspend.
> + 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;
You can move this into the if statement.
> + > + return 0; > +} > + > static void amd_pstate_verify_cpu_policy(struct amd_cpudata *cpudata, > struct cpufreq_policy_data *policy) > { > @@ -1512,6 +1549,8 @@ static struct cpufreq_driver amd_pstate_epp_driver = { > .update_limits = amd_pstate_epp_update_limits, > .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, > };
|  |