On Thu, 25 Jul 2024 at 13:25, Viresh Kumar viresh.kumar@linaro.org wrote:
On 25-07-24, 11:21, Ulf Hansson wrote:
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.
Right, but that won't happen with the diff I shared earlier where we set "forced" to true. Isn't it ?
Correct.
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?
Not separate paths, but ignore clk/regulator changes if the table belongs to a genpd.
The problem with that would be that platforms (Tegra at least) are already using a combination of opp-level and clocks.
If they are using both for a genpd's OPP table (and changes are made for both opp-level and clock by the OPP core), then it should already be wrong, isn't it?
They are changing the clock through the device's OPP table and the level (performance-state) via genpd's table (using required OPPs). This works fine as of today.
Two simultaneous calls to dev_pm_opp_set_opp() would set the level correctly (as aggregation happens in the genpd core), but clock setting would always reflect the second caller. This should be fixed too, isn't it ?
As I said before, I don't see a need for this. The recursive call to dev_pm_opp_set_opp() is today superfluous.
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.
I was terrified for a minute after reading this and the current code, as I also thought there is an issue there. But I was confident that we used to take care of this case separately earlier. A short dive into git logs got me to this:
commit 6d366d0e5446 ("OPP: Use _set_opp_level() for single genpd case")
This should be working just fine I guess.
It's working today for *opp-level* only, because of the commit above. That's correct.
My point is that calling dev_pm_opp_set_opp() recursively from _set_required_opps() doesn't make sense for the single PM domain case, as we can't assign a required-dev for it. This leads to an inconsistent behaviour when managing the required-OPPs.
To make the behavior consistent (and to fix the bug), I still think it would be better to do something along what $subject patch proposes.
Kind regards Uffe