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