Re: [PATCH] OPP: Fix support for required OPPs for multiple PM domains

[Viresh Kumar]

On 18-07-24, 12:38, Ulf Hansson wrote:
> I understand your point, but we don't need to call
> dev_pm_opp_set_opp() from _set_required_opps() to accomplish this.

I _strongly_ feel that the OPP core should be doing what other frameworks, like
clk, regulator, genpd, are doing in this case. Call recursively.

> In fact, I have realized that calling dev_pm_opp_set_opp() from there
> doesn't work the way we expected.
>
> More precisely, at each recursive call to dev_pm_opp_set_opp() we are
> changing the OPP for a genpd's OPP table for a device that has been
> attached to it. The problem with this, is that we may have several
> devices being attached to the same genpd, thus sharing the same
> OPP-table that is being used for their required OPPs. So, we may have
> several active requests for different OPPs for a genpd's OPP table
> simultaneously. It seems wrong from the OPP library point of view. To
> me, it's would be better to leave any kind of aggregation to be
> managed by genpd itself.

Right. I see this problem too and that's why I said earlier that OPP core was
designed for a different use case and genpd doesn't fit perfectly. Though I
don't see how several calls the dev_pm_opp_set_opp() simultaneously is a
problem. This can happen without recursive calling too, where simultaneous calls
for genpds occur.

I think the main problem here, on how genpd doesn't fit with OPP core, is that
the OPP core is trying to do some sort of aggregation generally at its level,
like avoiding a change of OPP if the OPP is same. I think the right way to fix
this is by not doing any aggregation at OPP core level and genpd handle it all.
Which you are also aligned with I guess. This would also mean that OPP core
shouldn't try configuring, clk, regulator, bandwidth, etc for a genpd. The Genpd
core should handle that, else we may end up incorrectly configuring things.

I guess this is what you were trying to say as well, when you were trying to
replace the recursive call with set-level only.

I think, we don't need that change but rather avoid all these extra settings
from dev_pm_opp_set_opp() itself.

Also consider that genpd configuration doesn't only happen with recursive call,
but can happen with a call to dev_pm_opp_set_opp() directly too for the genpd.

> The API as such isn't the problem, but rather that we are recursivly
> calling dev_pm_opp_set_opp() for the required-devs.

I think that design is rather correct, just like other frameworks. Just that we
need to do only set-level for genpds and nothing else. That will have exactly
the same behavior that you want.

> In the single PM domain case, this would simply not work, as there is
> not a separate virtual device we can assign to the required-dev to.

We can assign the real device in that case, why is that a problem ?

-- 
viresh