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

[Sumit Gupta]

On 27/07/26 19:15, Christian Loehle wrote:
> External email: Use caution opening links or attachments
>
>
> 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 <christian.loehle@xxxxxxx>
> > Signed-off-by: Sumit Gupta <sumitg@xxxxxxxxxx>
> > ---
> >   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?

Agreed.
I will save the requested values and restore the firmware values in
suspend(), while the CPPC is still fully usable.
offline() will skip these steps when suspend() has already performed them.

Thanks,
Sumit

> >   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",