Re: [PATCHv2] PM / devfreq: Add dev_pm_qos support

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

On 19. 8. 21. 오전 12:26, Leonard Crestez wrote:
> On 8/14/2019 4:14 AM, Chanwoo Choi wrote:
>> On 19. 8. 14. 오전 10:06, Chanwoo Choi wrote:
>>> On 19. 8. 13. 오후 8:27, Leonard Crestez wrote:
>>>> On 13.08.2019 09:10, Chanwoo Choi wrote:
>>>>> In case of cpufreq, cpufreq.c replace the body of store_min_freq()
>>>>> and store_max_freq() by using struct dev_pm_qos_request instancce
>>>>> with dev_pm_qos_update_request().
>>>>>
>>>>> If you use the new way with dev_pm_qos_update_request() for
>>>>> min_freq_store() and max_freq_store(), it doesn't need to
>>>>> get the final frequency from three candidate frequencies.
>>>>
>>>> Yes, I saw that but didn't implement the equivalent for devfreq because
>>>> it's not clear what there is to gain.
>>>
>>> I think that it is clear. Just use the dev_pm_qos_request interface
>>> for both user input through sysfs and device input with qos request.
>>> Already PM_QOS has the feature to get the final freuency among
>>> the multiple request. When use the dev_pm_qos request, the devfreq
>>> doesn't need to compare between user input and device input with qos.
>>> It make devfreq core more clear and simple
> 
>>>> Since dev_pm_qos is measured in khz it means that min_freq/max_req on
>>>> sysfq would lose 3 significant digits, however those digits are probably
>>>> useless anyway.
>>>
>>> I think that it doesn't matter. This patch already considers the this issue
>>> by using '* 1000'. We can get either KHz or MHz with additional operation.
>>> I think that it is not problem.
> 
> It introduces the following issue:
> 
> # echo 333333333 > /sys/class/devfreq/devfreq0/min_freq
> # cat /sys/class/devfreq/devfreq0/min_freq
> 333333000
> 
> Changing rounding rules could confuse userspace tools. This is not 
> entirely a new issue because freq values which are not an integer number 
> of khz are likely not an integer number of hz either.

As I knew that, user don't need to enter the perfect supported clock
of devfreq0 because the final frequency is decided by device driver
with some limitations like the range of h/w clock.

The user can input the wanted frequency like 333333333,
but, the device driver try to find the similar frequency with policy
and the can decide the final supported frequency because the h/w clock
for devfreq0 might not support the perfect same clock frequency of user input.

Also, if it is the problem to lose the 3 significant digits, 
it should be fixed on dev_pm_qos.

> 
>>> Actually, I think that I want to use the only dev_pm_qos_request
>>> for all external request like devfreq cooling of thermal,
>>> user input through sysfs and device request with dev_pm_qos_request.
>>>
>>> Already, dev_pm_qos_request is designed to consider the multiple requirements.
>>> We don't need to use the various method (OPP interface, sysfs input, dev_pm_qos)
>>> because make it more simple and easy.
>>>
>>> I think that after finished the review of this patch, I will do refactor the devfreq_cooling.c
>>> by using the dev_pm_qos_request. Or, if there are some volunteeer,
>>
>> Sorry, I would withdraw the this opinion about replacing
>> the OPP enable/disable interface with the dev_pm_qos_request
>> because even if devfreq-cooling.c needs the 'dev' instance
>> to use the dev_pm_qos_request method, it is not clear until now.
>> It needs how to get the device instance of devfreq on device-tree.
> 
> I looked a bit at the devfreq-cooling implementation and it seems like 
> there aren't any users in upstream?

Right. there are no upstream patch. But, ARM developer contributed
the devfreq-cooling.c in order to control ARM Mali frequency
according to temperature. If you want, you can check
the reference code from ARM Mali kernel driver.

> 
> As far as I can tell a devfreq implementation needs to call 
> of_devfreq_cooling_register and then the devfreq cooling code could 
> register a dev_pm_qos request on devfreq->dev.parent. I'm not sure I 
> understand what problem you see.

Ah. you're right. It is my misunderstand. I though that there are no
any way to get the 'devfreq' instance from device tree. But, I checked
the devfreq-cooling.c again, as you commented, the devfreq-cooling.c
provides the of_devfreq_cooling_reigster().



-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel