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
