| Message ID | 20220909164534.71864-8-Perry.Yuan@amd.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Implement AMD Pstate EPP Driver | expand |
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, > };
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; + + pr_debug("AMD CPU Core %d suspending\n", cpudata->cpu); + + 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); + + return 0; +} + +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); + + if (cpudata->suspended && epp_enabled) { + 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 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, };
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(+)