Message-ID: <CAPDyKFqTtqYEFfaHq-jbxnp5gD7qm9TbLrah=k=VD2TRArvU8A@mail.gmail.com>
Date: Sun, 28 Jul 2024 22:05:18 +0200
From: Ulf Hansson <ulf.hansson@...aro.org>
To: Viresh Kumar <viresh.kumar@...aro.org>
Cc: Viresh Kumar <vireshk@...nel.org>, Nishanth Menon <nm@...com>, Stephen Boyd <sboyd@...nel.org>, 
	Nikunj Kela <nkela@...cinc.com>, Prasad Sodagudi <psodagud@...cinc.com>, linux-pm@...r.kernel.org, 
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org, 
	linux-tegra@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH] OPP: Fix support for required OPPs for multiple PM domains

On Thu, 25 Jul 2024 at 13:25, Viresh Kumar <viresh.kumar@...aro.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

Powered by blists - more mailing lists