Re: [PATCH v2 2/2] ACPI: CPPC: Add ospm_nominal_perf support

[Sumit Gupta]

On 13/05/26 21:13, Pierre Gondois wrote:
> External email: Use caution opening links or attachments
>
>
> > > > diff --git a/drivers/cpufreq/cppc_cpufreq.c
> > > > b/drivers/cpufreq/cppc_cpufreq.c
> > > > index 7e7f9dfb7a24..d06cba963550 100644
> > > > --- a/drivers/cpufreq/cppc_cpufreq.c
> > > > +++ b/drivers/cpufreq/cppc_cpufreq.c
> > > > @@ -715,6 +715,16 @@ static int cppc_cpufreq_cpu_init(struct
> > > > cpufreq_policy *policy)
> > > >               goto out;
> > > >       }
> > > >
> > > > +     /*
> > > > +      * Initialize OSPM Nominal Performance to inform firmware of
> > > > +      * OSPM's nominal level. Performance above this value = boost;
> > > > +      * below = throttle. Uses platform nominal by default.
> > > > +      */
> > > > +     ret = cppc_set_ospm_nominal_perf(cpu, caps->nominal_perf);
> > > > +     if (ret && ret != -EOPNOTSUPP)
> > > > +             pr_debug("Failed to set ospm_nominal_perf for CPU%d:
> > > > %d\n",
> > > > +                      cpu, ret);
> > > > +
> > >
> > > IIUC, if (ospm_nominal_perf == nominal_perf), the firmware should
> > > not behave differently. Is this really useful ?
> >
> > Right, it's a no-op from the firmware's side. The init was only so that
> > sysfs would show a value (platform nominal) before any userspace write.
> > Will drop it in v3 and return 0 from sysfs until userspace writes a
> > value.
> >
> >
> > > ------------
> > >
> > > Also this seems like there will need some synchronization
> > > mechanism to keep-up with the boost state.
> > >
> > > If the ospm_nominal_perf is lowered and boost is disabled,
> > > a freq. update should happen. IMO it looks like this could
> > > be handled with (another) freq_qos_request.
> > >
> > > This new freq_qos_request, if we name it ospm_nominal_freq_req,
> > > should only be taken into account if boost is disabled.
> > > Otherwise, if boost is enabled, ospm_nominal_freq_req
> > > should be ignored.
> >
> > Agreed, will add the new freq_qos_request in a follow-up patch.
> >
> > > ------------
> > >
> > > Also, the function seems to set the ospm_nominal_freq for
> > > a single CPU when the policy might be common for multiple
> > > CPUs right ?
> >
> > In v3, after dropping the change from cppc_cpufreq_cpu_init,
> > the problem won't come in this specific instance.
> >
> >
> > > The issues this field raises seems similar to the auto_sel
> > > ones. I.e. :
> > >
> > > - concurrency accesses + need for a scratch value
> > >
> > > - what should happen when unloading the driver
> > >
> > > - the value can be set for single CPUs but we might
> > > want to have the same value for the whole policy
> > >
> > > Maybe a common solution should be found.
> > > (I m not suggesting anything right now unfortunately).
> >
> > One way to address this is to move the sysfs from per-CPU acpi_cppc to
> > a per-policy node under cpufreq (ospm_nominal_perf_freq, kHz).
>
> Yes right, would it make sense to also move the "ospm_nominal_perf"
> to the cpu_data ?

Yes, moved to struct cppc_cpudata in v3.

> > In the sysfs callback, we can convert kHz to perf and write the register
> > on every CPU in policy->cpus.
> > Concurrency is already covered by policy->rwsem at the cpufreq layer.
> > This is similar to how we were handling min/max_perf in earlier version.
>
> Yes right. Something I don't really understand is that similarly to
> auto_sel,
> the sysfs rwsem protects the state of the cpu_data->auto_sel field,
> but we don't really have any guarantee that another driver won't modify
> the firmware state right ? (maybe it's just not worth ?)

Yes, the cached value is updated only by the cpufreq sysfs store path.
A direct caller of cppc_set_ospm_nominal_perf() would bypass it.
cpufreq is the only caller today, so it doesn't surface in practice.
Same applies to auto_sel, and as you noted, it's probably not worth
chasing until we have more callers.

Thank you,
Sumit Gupta


> > Does this approach make sense?
>
> Yes right thanks !
>
>
> > Thank you,
> > Sumit Gupta
> >
> > ....