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

[Pierre Gondois]

> > > 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 ?


> 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 ?)


> Does this approach make sense?

Yes right thanks !


> Thank you,
> Sumit Gupta
>
> ....