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

[Pierre Gondois <pierre.gondois@arm.com>]

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