lkml.org 
[lkml]   [2022]   [Sep]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
SubjectRe: [PATCH 7/7] cpufreq: amd-pstate: implement suspend and resume callbacks
From
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,
> };

\
 
 \ /
  Last update: 2022-09-09 21:01    [W:0.159 / U:0.040 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site