On Thu, 25 Jul 2024 at 08:02, Viresh Kumar <viresh.kumar@xxxxxxxxxx> wrote: > > 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. Right. The main issue in regards to the above, is that we may end up trying to vote for different devices, which votes correspond to the same OPP/OPP-table. The one that comes first will request the OPP, the other ones will be ignored as the OPP core thinks there is no reason to already set the current OPP. > > 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. Right, I think we are in agreement. Aggregation of the *performance-state* (opp-level) needs to be managed by genpd, solely. > > 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. Right. > > > 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. I don't quite understand what you are proposing. Do you want to add a separate path for opp-levels? The problem with that would be that platforms (Tegra at least) are already using a combination of opp-level and clocks. > > > 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 ? To be able to call dev_pm_opp_set_opp() on the required-dev (which would be the real device in this case), we need to add it to genpd's OPP table by calling _add_opp_dev() on it. See _opp_attach_genpd(). The problem with this, is that the real device already has its own OPP table (with the required-OPPs pointing to genpd's OPP table), which means that we would end up adding the device to two different OPP tables. Kind regards Uffe
