Re: [PATCH v3 1/4] cpufreq: CPPC: Keep the policy across CPU hotplug

[Sumit Gupta]

On 27/07/26 18:57, Christian Loehle wrote:
> External email: Use caution opening links or attachments
>
>
> On 7/24/26 22:59, Sumit Gupta wrote:
> > Without online()/offline() callbacks, the cpufreq core fully tears
> > down a policy during exit() when its last online CPU is offlined, and
> > rebuilds it during init() when it comes back.
> >
> > Add lightweight online()/offline() callbacks so the core instead keeps
> > the policy live and reuses the driver's cpu_data across CPU hotplug.
> > This avoids re-reading the CPPC capabilities on every offline/online,
> > making CPU hotplug faster.
> >
> >  From online(), re-enable CPPC, as the platform may have disabled it
> > while the CPU was offline. Since the policy is no longer rebuilt via
> > init(), online() also reprograms the CPPC performance controls
> > (desired/min/max).
> >
> > Signed-off-by: Sumit Gupta <sumitg@xxxxxxxxxx>
> > ---
> >   drivers/cpufreq/cppc_cpufreq.c | 44 ++++++++++++++++++++++++++++++++++
> >   1 file changed, 44 insertions(+)
> >
> > diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> > index f6cea0c54dd9..6dc59f99d880 100644
> > --- a/drivers/cpufreq/cppc_cpufreq.c
> > +++ b/drivers/cpufreq/cppc_cpufreq.c
> > @@ -722,6 +722,48 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
> >        return ret;
> >   }
> >
> > +/*
> > + * With offline() defined, the cpufreq core keeps the policy alive when
> > + * a CPU is hotplugged out.
> > + */
> > +static int cppc_cpufreq_cpu_offline(struct cpufreq_policy *policy)
> > +{
> > +     return 0;
> > +}
> > +
> > +/*
> > + * Re-enable CPPC when the policy's CPU comes back online, since the platform
> > + * may have disabled it while the CPU was offline.
> > + */
> > +static int cppc_cpufreq_cpu_online(struct cpufreq_policy *policy)
> > +{
> > +     struct cppc_cpudata *cpu_data = policy->driver_data;
> > +     unsigned int cpu = policy->cpu;
> > +     int ret;
> > +
> > +     ret = cppc_set_enable(cpu, true);
> > +     if (ret && ret != -EOPNOTSUPP)
> > +             pr_warn("Failed to re-enable CPPC for CPU%d (%d)\n", cpu, ret);
> What's the intention of continuing restoring CPPC register values here?

Yes, there is no benefit in continuing the restore when
cppc_set_enable() fails with anything other than -EOPNOTSUPP.
I will stop the restore at that point and return 0 with a warning,
since propagating the error would cause the core to tear down
the policy.

> > +
> > +     /*
> > +      * The platform may reset the controls while the CPU is offline, so
> > +      * recompute min/max, clamp desired_perf into range, and reprogram them.
> > +      */
> > +     cppc_cpufreq_update_perf_limits(cpu_data, policy);
> > +
> > +     cpu_data->perf_ctrls.desired_perf =
> > +             clamp_t(u32, cpu_data->perf_ctrls.desired_perf,
> > +                     cpu_data->perf_ctrls.min_perf,
> > +                     cpu_data->perf_ctrls.max_perf);
> > +
> > +     ret = cppc_set_perf(cpu, &cpu_data->perf_ctrls);
> > +     if (ret)
> > +             pr_debug("Failed to reapply perf request on CPU%d (%d)\n",
> > +                      cpu, ret);
> I think cppc_set_perf() needs some prep first before using it on reset values.
> We assume that reset value may be Autonomous Mode on, right? So we must never
> write MIN>MAX and vice versa. I think we may just have to read and write
> the 'otherwise-offending' value first on reset.

You are right. cppc_set_perf() writes MIN before MAX, so restoring
a window whose MIN is above the reset MAX can briefly produce MIN > MAX
on registers not accessed through PCC. I will add a preparatory write
that raises MAX in that case, followed by the full restore:
----
  /* min/max recomputed and desired_perf clamped, as posted */

  cppc_get_perf(cpu, &cur);

  if (cpu_data->perf_ctrls.min_perf > cur.max_perf) {
    prep = cur;              /* Rewrites DESIRED with its current value. */
    prep.min_perf = 0;  /* Zero leaves MIN unchanged. */
    prep.max_perf = cpu_data->perf_ctrls.max_perf;

    /* Raise MAX first. */
    cppc_set_perf(cpu, &prep);
  }

  /* Then restore the full desired/min/max request. */
  cppc_set_perf(cpu, &cpu_data->perf_ctrls);
----

The reverse transition does not need this extra write because the
existing MIN-before-MAX order writes the lower MIN first, so the window
only widens before MAX comes down.

Please let me know if you see any issue with this.

Thanks,
Sumit
....