| Message ID | 1585821635-28324-1-git-send-email-loic.poulain@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] arch: arm64: dts: msm8916: Add missing cpu opps | expand |
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 > >
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
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 */ + }; }; };
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(+)