Re: [PATCH 2/2] OPP: Call dev_pm_opp_set_opp() for required OPPs

From: Ulf Hansson <ulf.hansson@linaro.org>
To: Viresh Kumar <viresh.kumar@linaro.org>
Cc: Stephan Gerhold <stephan@gerhold.net>,
	Stephan Gerhold <stephan.gerhold@kernkonzept.com>,
	Viresh Kumar <vireshk@kernel.org>, Nishanth Menon <nm@ti.com>,
	Stephen Boyd <sboyd@kernel.org>,
	"Rafael J. Wysocki" <rafael@kernel.org>,
	linux-pm@vger.kernel.org,
	Vincent Guittot <vincent.guittot@linaro.org>,
	Konrad Dybcio <konrad.dybcio@linaro.org>,
	Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2] OPP: Call dev_pm_opp_set_opp() for required OPPs
Date: Wed, 25 Oct 2023 18:03:30 +0200	[thread overview]
Message-ID: <CAPDyKFrEdHGVeKo0oh0f2uJjtASbg4eye2UbEaE8s3sy+2tWDA@mail.gmail.com> (raw)
In-Reply-To: <20231025152000.vt42gvti2e4rewwc@vireshk-i7>

On Wed, 25 Oct 2023 at 17:20, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 25-10-23, 14:17, Stephan Gerhold wrote:
> > Thanks, this seems to work fine.
>
> Thanks a lot.
>
> > I found another small problem: In my OPP setup for MSM8916, some of the
> > required-opps point to an OPP with "opp-level = <0>" (see [1], the
> > <&rpmpd_opp_none> in the cpu-opp-table). This is because the vote for
> > the "CX" domain is for the CPU PLL clock source, which is only used for
> > the higher CPU frequencies (>= 998.4 MHz). With the previous code you
> > made it possible for me to vote for opp-level = <0> in commit
> > a5663c9b1e31 ("opp: Allow opp-level to be set to 0", discussion in [2]).
> > I think now it's broken because the _set_opp_level() added by Uffe
> > checks for if (!opp->level) as a sign that there is no opp-level defined
> > at all.

I don't think my patch broke it, I just followed the similar semantics
of how we treated the OPPs for the required opps.

As far as I understood it, the only way we could request the
performance-level to be zero, was if the consumer driver would call
dev_pm_opp_set_opp(dev, NULL);

So, unless I am overlooking something, things can have been screwed up
earlier too?

>
> Yes, we broke that. I think a simple fix is to initialize the level
> with an impossible value, like -1 and then 0 becomes valid.

Yep, make sense, let's fix it!

Are you sending a patch?

[...]

Kind regards
Uffe