Re: [PATCH v5 4/4] ARM: dts: qcom: ipq8064: Add CPU OPP table

From: Christian Marangi <ansuelsmth@gmail.com>
To: Konrad Dybcio <konrad.dybcio@linaro.org>
Cc: Robert Marko <robimarko@gmail.com>,
	ilia.lin@kernel.org, vireshk@kernel.org, nm@ti.com,
	sboyd@kernel.org, robh+dt@kernel.org,
	krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org,
	agross@kernel.org, andersson@kernel.org, rafael@kernel.org,
	linux-pm@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org
Subject: Re: [PATCH v5 4/4] ARM: dts: qcom: ipq8064: Add CPU OPP table
Date: Tue, 10 Oct 2023 22:05:25 +0200	[thread overview]
Message-ID: <6525ae86.050a0220.827f2.c59d@mx.google.com> (raw)
In-Reply-To: <e2d6c9c7-ff59-408f-94cf-81e66d291963@linaro.org>

On Tue, Oct 10, 2023 at 09:55:26PM +0200, Konrad Dybcio wrote:
> 
> 
> On 9/30/23 12:21, Robert Marko wrote:
> > From: Christian Marangi <ansuelsmth@gmail.com>
> > 
> > Add CPU OPP table for IPQ8062, IPQ8064 and IPQ8065 SoC.
> > Use opp-supported-hw binding to correctly enable and disable the
> > frequency as IPQ8062 supports up to 1.0Ghz, IPQ8064 supports up to
> > 1.4GHz with 1.2GHz as an additional frequency and IPQ8065 supports
> > 1.7GHZ but doesn't have 1.2GHZ frequency and has to be disabled.
> > 
> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > Signed-off-by: Robert Marko <robimarko@gmail.com>
> > ---
> > Changes v4:
> > * Add OPP DTS patch for IPQ8064
> > 
> >   arch/arm/boot/dts/qcom/qcom-ipq8062.dtsi | 30 +++++++++++
> >   arch/arm/boot/dts/qcom/qcom-ipq8064.dtsi | 67 ++++++++++++++++++++++++
> >   arch/arm/boot/dts/qcom/qcom-ipq8065.dtsi | 65 +++++++++++++++++++++++
> >   3 files changed, 162 insertions(+)
> > 
> > diff --git a/arch/arm/boot/dts/qcom/qcom-ipq8062.dtsi b/arch/arm/boot/dts/qcom/qcom-ipq8062.dtsi
> > index 5d3ebd3e2e51..72d9782c3d6f 100644
> > --- a/arch/arm/boot/dts/qcom/qcom-ipq8062.dtsi
> > +++ b/arch/arm/boot/dts/qcom/qcom-ipq8062.dtsi
> > @@ -6,3 +6,33 @@ / {
> >   	model = "Qualcomm Technologies, Inc. IPQ8062";
> >   	compatible = "qcom,ipq8062", "qcom,ipq8064";
> >   };
> > +
> > +&opp_table_cpu {
> > +	opp-384000000 {
> > +		opp-microvolt-speed0-pvs0 = <1000000 950000 1050000>;
> > +		opp-microvolt-speed0-pvs1 = <925000 878750 971250>;
> > +		opp-microvolt-speed0-pvs2 = <875000 831250 918750>;
> > +		opp-microvolt-speed0-pvs3 = <800000 760000 840000>;
> We can just make use of opp-supported-hw and define opp-384...-0,
> opp-384..-1 etc. with a valid corresponding bitmask in opp-supported-hw
> 
> otherwise it's somewhat confusing to follow, I think..
>

Ehh should we really double the nodes for ipq8062 and ipq8065?
The idea here was to overwrite the one since the pvs always match and at
worst(ipq8065) have 7 pvs instead of 4. From the system it would be
easier to read since only one table is present in the final dts and not
2 and referring to the opp-supported-hw.

The original idea was to declare one opp table and reuse pvs version (by
faking it with hardcoded values) to put additional pvs for ipq8062 and
ipq8065, but Dmitry didn't like it and asked to move the opp in
different dtsi.

-- 
	Ansuel