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

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

[Loic Poulain]

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.
     - s2 min/max are specified in pm8916 spec
     - 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>;
+			};
+			/* other regulators can be controlled via rpm */
+		};
 	};
 };
-- 
2.7.4

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

[Stephan Gerhold]

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 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.

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?

Thanks,
Stephan

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

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

[Bjorn Andersson]

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
> > 

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

[Stephan Gerhold]

On Thu, Apr 02, 2020 at 06:10:11PM -0700, Bjorn Andersson wrote:
> 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.
> 

Okay, that seems reasonable :)

Thanks,
Stephan