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

From: Viresh Kumar <viresh.kumar@linaro.org>
To: Stephan Gerhold <stephan@gerhold.net>
Cc: 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>,
	Ulf Hansson <ulf.hansson@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 20:50:00 +0530	[thread overview]
Message-ID: <20231025152000.vt42gvti2e4rewwc@vireshk-i7> (raw)
In-Reply-To: <ZTkHVVq61lrpRUOv@gerhold.net>

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.

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.

> Based on my longer discussion with Uffe recently [3] it's not entirely
> clear yet if I will still have the reference to &rpmpd_opp_none in the
> future. Alternatively, we discussed describing this differently, e.g. as
> a parent power domain (which would bypass most of the OPP code), or
> moving it directly to an OPP table of CPU PLL device node (which would
> only describe the actual "active" required-opps).
> 
> I'm not sure if anyone else has a reasonable use case for pointing to a
> required-opp with opp-level = <0>, so we could potentially also postpone
> solving this to later.

I would like to fix this respectively. Thanks for bringing this up.

-- 
viresh