Re: [PATCH v2] arch: arm64: dts: msm8916: Add missing cpu opps

[Bjorn Andersson <bjorn.andersson@linaro.org>]

On Thu 02 Apr 04:14 PDT 2020, Stephan Gerhold wrote:

> Hi,
> 
> On Thu, Apr 02, 2020 at 12:00:35PM +0200, Loic Poulain wrote:
> > The highest cpu frequency opps have been dropped because CPR is not
> > supported. However, we can simply specify operating voltage so that
> > they match the max corner voltages for each freq. With that, we can
> > support up to 1.2Ghz. Ideally, msm8916 CPR should be implemented to
> > fine tune operating voltages and optimize power consumption.
> > 
> > The SPMI interface is directly used for AP regulator control since
> > it offers a minimal transition latency (maximum transition latency
> > with spmi is 250us, with rpm is 970us as reported by cpufreq-info).
> > 
> > This patch:
> > - Adds missing opps and corresponding target voltages to msm8916.dtsi.
> > - Adds pm8916 spmi regulator node to pm8916.dtsi.
> > 
> > Tested with a dragonboard-410c.
> > 
> > Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
> > ---
> >  v2: - move cpu-supply to msm8916 since pm8916 s2 is tighly coupled
> >      to AP core (cf pm8916 specification) + other pm8916 supplies
> >      are already defined in msm8916.
> 
> Thanks for making these changes!
> 
> I will try to test this on my devices later today,
> and will ask some more people to test it on theirs.
> 
> What is a good way to test that it works correctly?
> If the device manages to reach the higher frequencies and still works
> correctly it's fine?
> 
> >      - s2 min/max are specified in pm8916 spec
> 
> Regarding this I have a small concern below.
> 
> >      - Removed 1.36GHz op since freq seems capped to 1.21 anyway
> > 
> >  arch/arm64/boot/dts/qcom/msm8916.dtsi | 25 +++++++++++++++++++++++++
> >  arch/arm64/boot/dts/qcom/pm8916.dtsi  | 13 +++++++++++++
> >  2 files changed, 38 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > index 9f31064..7407157 100644
> > --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > @@ -103,6 +103,7 @@
> >  			next-level-cache = <&L2_0>;
> >  			enable-method = "psci";
> >  			clocks = <&apcs>;
> > +			cpu-supply = <&pm8916_spmi_s2>;
> >  			operating-points-v2 = <&cpu_opp_table>;
> >  			#cooling-cells = <2>;
> >  			power-domains = <&CPU_PD0>;
> > @@ -116,6 +117,7 @@
> >  			next-level-cache = <&L2_0>;
> >  			enable-method = "psci";
> >  			clocks = <&apcs>;
> > +			cpu-supply = <&pm8916_spmi_s2>;
> >  			operating-points-v2 = <&cpu_opp_table>;
> >  			#cooling-cells = <2>;
> >  			power-domains = <&CPU_PD1>;
> > @@ -129,6 +131,7 @@
> >  			next-level-cache = <&L2_0>;
> >  			enable-method = "psci";
> >  			clocks = <&apcs>;
> > +			cpu-supply = <&pm8916_spmi_s2>;
> >  			operating-points-v2 = <&cpu_opp_table>;
> >  			#cooling-cells = <2>;
> >  			power-domains = <&CPU_PD2>;
> > @@ -142,6 +145,7 @@
> >  			next-level-cache = <&L2_0>;
> >  			enable-method = "psci";
> >  			clocks = <&apcs>;
> > +			cpu-supply = <&pm8916_spmi_s2>;
> >  			operating-points-v2 = <&cpu_opp_table>;
> >  			#cooling-cells = <2>;
> >  			power-domains = <&CPU_PD3>;
> > @@ -342,15 +346,35 @@
> >  
> >  		opp-200000000 {
> >  			opp-hz = /bits/ 64 <200000000>;
> > +			opp-microvolt = <1050000>;
> >  		};
> >  		opp-400000000 {
> >  			opp-hz = /bits/ 64 <400000000>;
> > +			opp-microvolt = <1050000>;
> > +		};
> > +		opp-533330000 {
> > +			opp-hz = /bits/ 64 <533330000>;
> > +			opp-microvolt = <1150000>;
> >  		};
> >  		opp-800000000 {
> >  			opp-hz = /bits/ 64 <800000000>;
> > +			opp-microvolt = <1150000>;
> >  		};
> >  		opp-998400000 {
> >  			opp-hz = /bits/ 64 <998400000>;
> > +			opp-microvolt = <1350000>;
> > +		};
> > +		opp-1094400000 {
> > +			opp-hz = /bits/ 64 <1094400000>;
> > +			opp-microvolt = <1350000>;
> > +		};
> > +		opp-1152000000 {
> > +			opp-hz = /bits/ 64 <1152000000>;
> > +			opp-microvolt = <1350000>;
> > +		};
> > +		opp-1209600000 {
> > +			opp-hz = /bits/ 64 <1209600000>;
> > +			opp-microvolt = <1350000>;
> >  		};
> >  	};
> >  
> > @@ -1605,6 +1629,7 @@
> >  					compatible = "qcom,rpm-pm8916-regulators";
> >  
> >  					pm8916_s1: s1 {};
> > +					/* s2 is directly controlled via spmi */
> >  					pm8916_s3: s3 {};
> >  					pm8916_s4: s4 {};
> >  
> > diff --git a/arch/arm64/boot/dts/qcom/pm8916.dtsi b/arch/arm64/boot/dts/qcom/pm8916.dtsi
> > index 0bcdf04..73d3b28 100644
> > --- a/arch/arm64/boot/dts/qcom/pm8916.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/pm8916.dtsi
> > @@ -157,5 +157,18 @@
> >  			vdd-micbias-supply = <&pm8916_l13>;
> >  			#sound-dai-cells = <1>;
> >  		};
> > +
> > +		spmi_regulators: spmi_regulators  {
> > +			compatible = "qcom,pm8916-regulators";
> > +			#address-cells = <1>;
> > +			#size-cells = <1>;
> > +
> > +			pm8916_spmi_s2: s2 {
> > +				regulator-always-on;
> > +				regulator-min-microvolt = <900000>;
> > +				regulator-max-microvolt = <1562000>;
> 
> This might be just me but I'm usually cautious when it comes to setting
> up the regulator constraints.
> 
> One way is to set the regulator constraints based on the capabilities of
> the regulator itself (which is what you did here I think)?
> 

The capabilities of the regulator goes in the regulator driver, what
should be specified in the DT are the constraints on this particular
board; i.e. the constraints of the devices attached to the regulator.

> The other way is to only allow voltages that actually make sense;
> to ensure that setting incorrect voltages (for whatever reason) will
> fail. (I actually know someone who managed to break a board by setting
> some regulator voltages incorrectly...)
> 
> We don't actually set anything < 1050000 or > 1350000.
> And if I'm reading the datasheet correctly, the CPU cores are not even
> specified to operate correctly at > 1.42V.
> 

Right, per the datasheet VDD_APC's operational range is 0.97V to 1.42V.
So I would like the min/max here to reflect that - to define the
valid range for this regulator on this (these) board(s).


The fact that we only vote for 1.05-1.35 is presumably a result of us
not using CPR, which possible would extend that further. So this feels
like a property of the client.

> I would personally prefer to keep the min/max voltages from your
> previous patch set, i.e.
> 
> 	regulator-min-microvolt = <1050000>;
> 	regulator-max-microvolt = <1350000>;
> 
> In case a higher/lower voltage is needed it could still be changed later.
> 
> But maybe that's just me being overly cautious?
> 

I prefer that adjustments in the operating points (or a move to CPR)
wouldn't require modifying the valid range of the regulator. I.e. that
we match the operating range of VDD_APC here.

But thanks for being cautious, I missed that the specified ranges was
outside VDD_APC.

Regards,
Bjorn

> Thanks,
> Stephan
> 
> > +			};
> > +			/* other regulators can be controlled via rpm */
> > +		};
> >  	};
> >  };
> > -- 
> > 2.7.4
> >