| Message ID | cover.1677063656.git.viresh.kumar@linaro.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | OPP: Simplify set_required_opp handling | expand |
Viresh Kumar <viresh.kumar@linaro.org> 于2023年2月22日周三 19:06写道： > > Hello, > > The required-opps configuration is closely tied to genpd and performance > states at the moment and it is not very obvious that required-opps can > live without genpds. Though we don't support configuring required-opps > for non-genpd cases currently. > > This patchset aims at cleaning up this a bit, just like what's done for clk and > regulators. This also makes it possible for platforms to provide their own > version of set_required_opps() helper, which can be used to configure the > devfreq device propertly. > > Jun, > > I haven't found time to test this through yet, though there isn't much anyway I > guess. Can you see if these can solve your problem properly ? Hi Viresh, It looks promising. The function get_target_freq_with_cpufreq() can be wrapped to act as set_required_opps() callback. But my case is a bit complicated. CPU opp depends on both genpd opp and devfreq opp. So the genpd_virt_devs array need to be modified or add another array for devfreq case. While genpd_virt_devs is bounded with genpd directly and coupled with "power-domains" list in device tree. Current required-opp nodes are designed to be aligned with the list. I am considering what's the best way for back compatibility. Hi Chanwoo, Do you have any comments on this proposal? This proposal arise because opp lib reports error when cpufreq driver try to set required opp for non-genpd case. Another possible fix is to ignore non-genpd opp in opp lib. But a unified and recursive opp management looks nicer, just like clock tree management. > > Viresh Kumar (3): > OPP: Handle all genpd cases together in _set_required_opps() > OPP: Move required opps configuration to specialized callback > OPP: Allow platforms to add a set_required_opps() callback > > drivers/opp/core.c | 113 ++++++++++++++++++++++++++++------------- > drivers/opp/of.c | 3 ++ > drivers/opp/opp.h | 4 ++ > include/linux/pm_opp.h | 5 ++ > 4 files changed, 91 insertions(+), 34 deletions(-) > > -- > 2.31.1.272.g89b43f80a514 >
On 23-02-23, 17:56, Jun Nie wrote: > It looks promising. The function get_target_freq_with_cpufreq() can be wrapped > to act as set_required_opps() callback. > But my case is a bit complicated. CPU opp depends on both genpd opp and > devfreq opp. I was wondering if we will have such a case soon enough or not :) > So the genpd_virt_devs array need > to be modified or add another array for devfreq case. While genpd_virt_devs is > bounded with genpd directly and coupled with "power-domains" list in > device tree. > Current required-opp nodes are designed to be aligned with the list. I > am considering > what's the best way for back compatibility. Please look at the top commit here: git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git opp/propagate Will this be enough for your use case ? I will post everything again once we are settled on a solution.
On Fri, Feb 24, 2023 at 07:47:13AM +0530, Viresh Kumar wrote: > On 23-02-23, 17:56, Jun Nie wrote: > > It looks promising. The function get_target_freq_with_cpufreq() can be wrapped > > to act as set_required_opps() callback. > > > But my case is a bit complicated. CPU opp depends on both genpd opp and > > devfreq opp. > > I was wondering if we will have such a case soon enough or not :) > > > So the genpd_virt_devs array need > > to be modified or add another array for devfreq case. While genpd_virt_devs is > > bounded with genpd directly and coupled with "power-domains" list in > > device tree. > > Current required-opp nodes are designed to be aligned with the list. I > > am considering > > what's the best way for back compatibility. > > Please look at the top commit here: > > git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git opp/propagate > > Will this be enough for your use case ? I will post everything again once we are > settled on a solution. For the opp lib, this is right direction. We still need to find a method to pass devfreq device node to opp lib, just genpd_virt_devs for power domain. But I am not clear below is the right way yet and this also involves wider changes. Or the opp's owner, devfreq device can be referred via opp lib? If so, we do not need to add devfreq-devs to cpu node at all. cpu1: cpu@101 { compatible = "arm,cortex-a53"; device_type = "cpu"; power-domains = <&cpr>; power-domain-names = "cpr"; devfreq-devs = <&cci>; devfreq-names = "cci"; operating-points-v2 = <&cluster1_opp_table>; }; opp-200000000 { opp-hz = /bits/ 64 <200000000>; required-opps = <&cpr_opp3>, <&cci_opp1>; }; > > -- > viresh
On 24-02-23, 16:17, Jun Nie wrote: > For the opp lib, this is right direction. We still need to find a method to > pass devfreq device node to opp lib, just genpd_virt_devs for power domain. I am not really sure I understood it all. What is "device node" ? DT node or struct device ? What exactly do you need ? > But I am not clear below is the right way yet and this also involves wider > changes. Or the opp's owner, devfreq device can be referred via opp lib? > If so, we do not need to add devfreq-devs to cpu node at all. > > cpu1: cpu@101 { > compatible = "arm,cortex-a53"; > device_type = "cpu"; > power-domains = <&cpr>; > power-domain-names = "cpr"; > devfreq-devs = <&cci>; > devfreq-names = "cci"; Why do you need these ? > operating-points-v2 = <&cluster1_opp_table>; > }; > > opp-200000000 { > opp-hz = /bits/ 64 <200000000>; > required-opps = <&cpr_opp3>, <&cci_opp1>; This looks fine though. > };
Viresh Kumar <viresh.kumar@linaro.org> 于2023年2月27日周一 12:23写道： > > On 24-02-23, 16:17, Jun Nie wrote: > > For the opp lib, this is right direction. We still need to find a method to > > pass devfreq device node to opp lib, just genpd_virt_devs for power domain. > > I am not really sure I understood it all. What is "device node" ? DT node or > struct device ? What exactly do you need ? Sorry for not expressing it accurately. I should say devfreq devices pointers, just devfreq_virt_devs vs genpd_virt_devs. Then you know why I add devfreq-devs dts nodes below. > > > But I am not clear below is the right way yet and this also involves wider > > changes. Or the opp's owner, devfreq device can be referred via opp lib? > > If so, we do not need to add devfreq-devs to cpu node at all. > > > > cpu1: cpu@101 { > > compatible = "arm,cortex-a53"; > > device_type = "cpu"; > > power-domains = <&cpr>; > > power-domain-names = "cpr"; > > devfreq-devs = <&cci>; > > devfreq-names = "cci"; > > Why do you need these ? > > > operating-points-v2 = <&cluster1_opp_table>; > > }; > > > > opp-200000000 { > > opp-hz = /bits/ 64 <200000000>; > > required-opps = <&cpr_opp3>, <&cci_opp1>; > > This looks fine though. > > > }; > > -- > viresh
On 27-02-23, 17:21, Jun Nie wrote: > Sorry for not expressing it accurately. I should say devfreq devices > pointers, just > devfreq_virt_devs vs genpd_virt_devs. Then you know why I add devfreq-devs > dts nodes below. Won't something like dev_pm_opp_set_clkname() would be enough here too ? We already do this kind of work for clks and regulators. Having power domain specific information within CPU nodes isn't a requirement of the OPP core, but the general requirement of genpd core instead.
Viresh Kumar <viresh.kumar@linaro.org> 于2023年2月27日周一 17:29写道： > > On 27-02-23, 17:21, Jun Nie wrote: > > Sorry for not expressing it accurately. I should say devfreq devices > > pointers, just > > devfreq_virt_devs vs genpd_virt_devs. Then you know why I add devfreq-devs > > dts nodes below. > > Won't something like dev_pm_opp_set_clkname() would be enough here too ? We > already do this kind of work for clks and regulators. Thanks! It is a possible solution. I will try to spare time on this as higher priority tasks are on my list. > > Having power domain specific information within CPU nodes isn't a requirement of > the OPP core, but the general requirement of genpd core instead. > > -- > viresh
On 06-03-23, 18:48, Jun Nie wrote: > Viresh Kumar <viresh.kumar@linaro.org> 于2023年2月27日周一 17:29写道： > > > > On 27-02-23, 17:21, Jun Nie wrote: > > > Sorry for not expressing it accurately. I should say devfreq devices > > > pointers, just > > > devfreq_virt_devs vs genpd_virt_devs. Then you know why I add devfreq-devs > > > dts nodes below. > > > > Won't something like dev_pm_opp_set_clkname() would be enough here too ? We > > already do this kind of work for clks and regulators. > > Thanks! It is a possible solution. I will try to spare time on this as > higher priority tasks > are on my list. I have applied first two patches. I would like to apply the third one with some user code. I will wait for your code to merge that.