Re: [RFC PATCH 09/11] devfreq: exynos-bus: Add interconnect functionality to exynos-bus

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

Hi Artur and Leonard,

On 19. 8. 9. 오전 12:00, Leonard Crestez wrote:
> On 29.07.2019 04:49, Chanwoo Choi wrote:
>> On 19. 7. 23. 오후 9:20, Artur Świgoń wrote:
>>> This patch adds interconnect functionality to the exynos-bus devfreq
>>> driver.
>>>
>>> The devfreq target() callback provided by exynos-bus now selects either the
>>> frequency calculated by the devfreq governor or the frequency requested via
>>> the interconnect API for the given node, whichever is higher.
>>
>> Basically, I agree to support the QoS requirement between devices.
>> But, I think that need to consider the multiple cases.
>>
>> 1. When changing the devfreq governor by user,
>> For example of the connection between bus_dmc/leftbus/display on patch8,
>> there are possible multiple cases with various devfreq governor
>> which is changed on the runtime by user through sysfs interface.
>>
>> If users changes the devfreq governor as following:
>> Before,
>> - bus_dmc (simple_ondemand, available frequency 100/200/300/400 MHz)
>> --> bus_leftbus(simple_ondemand, available frequency 100/200/300/400 MHz)
>> ----> bus_display(passive)
>>
>> After changed governor of bus_dmc,
>> if the min_freq by interconnect requirement is 400Mhz,
>> - bus_dmc (powersave) : min_freq and max_freq and cur_freq is 100MHz
>> --> bus_leftbus(simple_ondemand) : cur_freq is 400Mhz
>> ----> bus_display(passive)
>>
>> The final frequency is 400MHz of bus_dmc
>> even if the min_freq/max_freq/cur_freq is 100MHz.
>> It cannot show the correct min_freq/max_freq through
>> devfreq sysfs interface.
>>
>>
>> 2. When disabling the some frequency by devfreq-thermal throttling,
>> This patch checks the min_freq of interconnect requirement
>> in the exynos_bus_target() and exynos_bus_passive_target().
>> Also, it cannot show the correct min_freq/max_freq through
>> devfreq sysfs interface.
>>
>> For example of bus_dmc bus,
>> - The available frequencies are 100MHz, 200MHz, 300MHz, 400MHz
>> - Disable 400MHz by devfreq-thermal throttling
>> - min_freq is 100MHz
>> - max_freq is 300MHz
>> - min_freq of interconnect is 400MHz
>>
>> In result, the final frequency is 400MHz by exynos_bus_target()
>> There are no problem for working. But, the user cannot know
>> reason why cur_freq is 400MHz even if max_freq is 300MHz.
>>
>> Basically, update_devfreq() considers the all constraints
>> of min_freq/max_freq to decide the proper target frequency.
> 
> I think that applying the interconnect min_freq via dev_pm_qos can help 
> with many of these concerns: update_devfreq controls all the min/max 
> handling, sysfs is accurate and better decisions can be made regarding 
> thermal. Enforcing constraints in the core is definitely better.
> 
> This wouldn't even be a very big change, you don't need to actually move 
> the interconnect code outside of devfreq. Just make every devfreq/icc 
> node register a dev_pm_qos_request for itself during registration and 
> call dev_pm_qos_update_request inside exynos_bus_icc_set.
> 
> See: https://patchwork.kernel.org/patch/11084279/

I agree this approach of Leonard. If we use the dev_pm_qos[1] feature,
it resolve the issue of my comment1/2.

In summary, when receive the minimum frequency requirement
from interconnect path, the each bus uses the dev_pm_qos interface
in order to inform the minimum frequency from interconnect to devfreq.
And then the devfreq core will execute the update_devfreq()
with all frequency requirements as following:
- the user input (min/max frequency though devfreq sysfs
- the target frequency provided by devfreq governor
- the minimum frequency from interconnect path

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics