Re: [PATCH v3 2/2] PM / devfreq: Use dev_pm_qos for sysfs min/max_freq

[Leonard Crestez <leonard.crestez@nxp.com>]

On 21.08.2019 05:02, Chanwoo Choi wrote:
> On 19. 8. 21. 오전 12:24, Leonard Crestez wrote:
>> Now that devfreq supports dev_pm_qos requests we can use them to handle
>> the min/max_freq values set by userspace in sysfs, similar to cpufreq.
>>
>> Since dev_pm_qos handles frequencies as kHz this change reduces the
>> precision of min_freq and max_freq. This shouldn't introduce problems
>> because frequencies which are not an integer number of kHz are likely
>> not an integer number of Hz either.
>>
>> Try to ensure compatibilitity by rounding min values down and rounding
>> max values up.
>>
>> Simplify the {min,max}_freq_store code by setting "null" values of 0 and
>> MAX_S32 respectively instead of clamping to what freq tables are
>> actually supported. Values are already automatically clamped on
>> readback.
>>
>> Also simplify by droping the limitation that userspace min_freq must be
>> lower than userspace max_freq, it is already documented that max_freq
>> takes precedence.
>>
>> @@ -1358,33 +1371,20 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr,
>>   
>>   	ret = sscanf(buf, "%lu", &value);
>>   	if (ret != 1)
>>   		return -EINVAL;
>>   
>> -	mutex_lock(&df->lock);
>> -
>> -	if (value) {
>> -		if (value > df->max_freq) {
>> -			ret = -EINVAL;
>> -			goto unlock;
>> -		}
> 
> Actually, the user can input the value they want.
> So, the above code is not necessary because the devfreq->scaling_max_freq
> is never overflow from supported maximum frequency. The devfreq->scaling_max_freq
> is based on OPP entries from DT.
> 
> But, if we replace the existing request way of devfreq-cooling.c
> with dev_pm_qos, devfreq->scaling_max_freq doesn't guarantee
> the supported maximum frequency. >
> We need to keep the supported min_freq/max_freq value without dev_pm_qos
> requirement because the dev_pm_qos requirement might have the overflow value.
> the dev_pm_qos doesn't know the supported minimum and maximum frequency
> of devfreq device.

I'm not sure I understand what you mean. My patch allows user to set 
entirely arbitrary min/max rates and this is good because we already 
have a well-defined way to handle this: max overrides min.

The scaling_min_freq and scaling_max_freq variables can just be kept 
around indefinitely no matter what happens to thermal. They're just a 
cache for dev_pm_opp_find_freq_ceil and dev_pm_opp_find_freq_floor.

BTW: I noticed that scaling_min_freq and scaling_max_freq are updated in 
devfreq_notifier_call but devfreq->nb is not registered by default, only 
when a driver requests it explicitly. Is there a reason for this?
Because I'd call dev_pm_opp_register_notifier inside devfreq_add_device 
and remove all the devfreq_register/unregister_notifier APIs.

--
Regards,
Leonard