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

[Chanwoo Choi <cw00.choi@samsung.com>]

On 19. 8. 21. 오후 10:03, Leonard Crestez wrote:
> 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.

This patch doesn't check the range of input value
with the supported frequencies of devfreq device.

For example,
The devfreq0 device has the following available frequencies
100000000 200000000 300000000

and then user enters the 500000000 as following:
echo 500000000 > /sys/class/devfreq/devfreq0/min_freq

In result, get_effective_min_freq() will return the 500000000.
It is wrong value. it show the unsupported frequency through
min_freq sysfs path.

- devfreq->scaling_min_freq is 100000000,                                   
- 1000 * (unsigned long)dev_pm_qos_read_value(devfreq->dev.parent,
  DEV_PM_QOS_MIN_FREQUENCY)); is 500000000


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

Initial version of devfreq has not forced to use the OPP interface 
as the mandatory. So, it just provides the external function
devfreq_register_opp_notifier.

But,
We can call 'devfreq_register_opp_notifier' during devfreq_add_device()
because the OPP interface is mandatory for devfreq.

> Because I'd call dev_pm_opp_register_notifier inside devfreq_add_device 
> and remove all the devfreq_register/unregister_notifier APIs.

OK.

> 
> --
> Regards,
> Leonard
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics