Messages in this thread | | | From | Ulf Hansson <> | Date | Fri, 3 Nov 2023 12:58:26 +0100 | Subject | Re: [PATCH 1/2] OPP: Use _set_opp_level() for single genpd case |
| |
On Mon, 30 Oct 2023 at 11:29, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 26-10-23, 11:53, Ulf Hansson wrote: > > On Wed, 25 Oct 2023 at 15:49, Stephan Gerhold <stephan@gerhold.net> wrote: > > > 2. The OPP WARNing triggers with both variants because it just checks > > > if "required-opps" has a single entry. I guess we need extra checks > > > to exclude the "parent genpd" case compared to the "OPP" case. > > > > > > [ 1.116244] WARNING: CPU: 2 PID: 36 at drivers/opp/of.c:331 _link_required_opps+0x180/0x1cc > > > [ 1.125897] Hardware name: Qualcomm Technologies, Inc. APQ 8016 SBC (DT) > > > [ 1.146887] pc : _link_required_opps+0x180/0x1cc > > > [ 1.146902] lr : _link_required_opps+0xdc/0x1cc > > > [ 1.276408] Call trace: > > > [ 1.283519] _link_required_opps+0x180/0x1cc > > > [ 1.285779] _of_add_table_indexed+0x61c/0xd40 > > > [ 1.290292] dev_pm_opp_of_add_table+0x10/0x18 > > > [ 1.294546] of_genpd_add_provider_simple+0x80/0x160 > > > [ 1.298974] cpr_probe+0x6a0/0x97c > > > [ 1.304092] platform_probe+0x64/0xbc > > > > > > It does seem to work correctly, with and without this patch. So I guess > > > another option might be to simply silence this WARN_ON(). :') > > > > Oh, thanks for pointing this out! This case haven't crossed my mind yet! > > > > Allow me to think a bit more about it. I will get back to you again > > with a suggestion soon, unless Viresh comes back first. :-) > > I have resent the series now. > > Stephan, please give it a try again. Thanks. > > Regarding this case where a genpd's table points to a parent genpd's table via > the required-opps, it is a bit tricky to solve and the only way around that I > could think of is that someone needs to call dev_pm_opp_set_config() with the > right device pointer, with that we won't hit the warning anymore and things will > work as expected. > > In this case the OPP core needs to call dev_pm_domain_set_performance_state() > for device and then its genpd. We need the right device pointers :( > > Ulf, also another important thing here is that maybe we would want the genpd > core to not propagate the voting anymore to the parent genpd's ? The > dev_pm_opp_set_opp() call is better placed at handling all things and not just > the performance state, like clk, regulator, bandwidth and so the recursion > should happen at OPP level only.
Are you saying that the OPP library should be capable of managing the parent-clock-rates too, when there is a new rate being requested for a clock that belongs to an OPP? To me, that sounds like replicating framework specific knowledge into the OPP library, no? Why do we want this?
Unless I totally misunderstood your suggestion, I think it would be better if the OPP library remained simple and didn't run recursive calls, but instead relied on each framework to manage the aggregation and propagation to parents.
> For now my series shouldn't break anything, > just that we will try to set performance state twice for the parent genpd, the > second call should silently return as the target state should be equal to > current state. > > -- > viresh
Kind regards Uffe
| |