Re: [PATCH v3 4/4] cpufreq: CPPC: Preserve OSPM-set registers across suspend/resume

On 7/24/26 22:59, Sumit Gupta wrote:
> The previous patch preserves the OSPM-set registers across CPU hotplug,
> but system suspend/resume is a separate path. On platforms that reset
> those registers or the performance controls across suspend, the values
> are lost.
> 
> Reuse the same save/restore mechanism for suspend/resume:
> 
>   - suspend() only saves the current OSPM-set values. It does not revert
>     them to firmware, since a later offline() already does that when
>     system suspend offlines the secondary CPUs.
>   - resume() needs the same steps as online(), so .resume points at
>     online() directly.
> 
> Suggested-by: Christian Loehle <[email protected]>
> Signed-off-by: Sumit Gupta <[email protected]>
> ---
>  drivers/cpufreq/cppc_cpufreq.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> index 8a13ec49eb9d..be2d873ccd84 100644
> --- a/drivers/cpufreq/cppc_cpufreq.c
> +++ b/drivers/cpufreq/cppc_cpufreq.c
> @@ -921,6 +921,22 @@ static int cppc_cpufreq_cpu_online(struct cpufreq_policy *policy)
>  	return 0;
>  }
>  
> +/*
> + * suspend() only saves the OSPM-set values. It does not revert them to the
> + * firmware values.
> + *
> + * This is because system suspend may later offline the secondary CPUs.
> + * That runs offline(), which saves the values and then reverts to firmware.
> + * If suspend() had already reverted, offline() would save the firmware value
> + * instead of the user's request, and the request would be lost.
> + */
> +static int cppc_cpufreq_cpu_suspend(struct cpufreq_policy *policy)
> +{
> +	cppc_cpufreq_save_requested_regs(policy);
> +
> +	return 0;
> +}
> +

Wouldn't it be safer to commit everything here and not rely on the later
offline() to still need a fully usable CPPC platform (in particular for
PCC), as at this stage all but 2 CPUs might already be offline()d?
Also either the later offline() saves registers successfully, and overwrites
this snapshot, or it fails and overwrites them with U64, right?


>  static void cppc_cpufreq_cpu_exit(struct cpufreq_policy *policy)
>  {
>  	struct cppc_cpudata *cpu_data = policy->driver_data;
> @@ -1235,6 +1251,8 @@ static struct cpufreq_driver cppc_cpufreq_driver = {
>  	.exit = cppc_cpufreq_cpu_exit,
>  	.online = cppc_cpufreq_cpu_online,
>  	.offline = cppc_cpufreq_cpu_offline,
> +	.suspend = cppc_cpufreq_cpu_suspend,
> +	.resume = cppc_cpufreq_cpu_online,
>  	.set_boost = cppc_cpufreq_set_boost,
>  	.attr = cppc_cpufreq_attr,
>  	.name = "cppc_cpufreq",