Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 26 May 2023 16:03:42 +0200
From:   Stephan Gerhold <stephan@gerhold.net>
To:     Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Cc:     Bjorn Andersson <andersson@kernel.org>,
        Andy Gross <agross@kernel.org>,
        Konrad Dybcio <konrad.dybcio@linaro.org>,
        Srinivas Kandagatla <srinivas.kandagatla@linaro.org>,
        linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, phone-devel@vger.kernel.org,
        ~postmarketos/upstreaming@lists.sr.ht
Subject: Re: [PATCH 3/8] arm64: dts: qcom: msm8916: Fix regulator constraints
Message-ID: <ZHC8PhUi0H4o4TUR@gerhold.net>
References: <20230510-msm8916-regulators-v1-0-54d4960a05fc@gerhold.net>
 <20230510-msm8916-regulators-v1-3-54d4960a05fc@gerhold.net>
 <cae30437-f9bc-6e7a-8371-bba7eeff1e8f@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <cae30437-f9bc-6e7a-8371-bba7eeff1e8f@linaro.org>
Precedence: bulk

On Fri, May 26, 2023 at 02:38:01PM +0100, Bryan O'Donoghue wrote:
> On 17/05/2023 19:48, Stephan Gerhold wrote:
> > The regulator constraints for most MSM8916 devices (except DB410c) were
> > originally taken from Qualcomm's msm-3.10 vendor device tree (for lack
> > of better documentation). Unfortunately it turns out that Qualcomm's
> > voltages are slightly off as well and do not match the voltage
> > constraints applied by the RPM firmware.
> > 
> > This means that we sometimes request a specific voltage but the RPM
> > firmware actually applies a much lower or higher voltage. This is
> > particularly critical for pm8916_l11 which is used as SD card VMMC
> > regulator: The SD card can choose a voltage from the current range of
> > 1.8 - 2.95V. If it chooses to run at 1.8V we pretend that this is fine
> > but the RPM firmware will still silently end up configuring 2.95V.
> > This can be easily reproduced with a multimeter or by checking the
> > SPMI hardware registers of the regulator.
> > 
> > Fix this by making the voltages match the actual "specified range" in
> > the PM8916 Device Specification which is enforced by the RPM firmware.
> > 
> > Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> > ---
> >   arch/arm64/boot/dts/qcom/msm8916-acer-a1-724.dts           | 14 +++++++-------
> >   arch/arm64/boot/dts/qcom/msm8916-alcatel-idol347.dts       | 14 +++++++-------
> >   arch/arm64/boot/dts/qcom/msm8916-asus-z00l.dts             | 14 +++++++-------
> >   arch/arm64/boot/dts/qcom/msm8916-gplus-fl8005a.dts         | 14 +++++++-------
> >   arch/arm64/boot/dts/qcom/msm8916-huawei-g7.dts             | 12 ++++++------
> >   arch/arm64/boot/dts/qcom/msm8916-longcheer-l8150.dts       | 14 +++++++-------
> >   arch/arm64/boot/dts/qcom/msm8916-longcheer-l8910.dts       | 14 +++++++-------
> >   arch/arm64/boot/dts/qcom/msm8916-samsung-a2015-common.dtsi | 14 +++++++-------
> >   arch/arm64/boot/dts/qcom/msm8916-samsung-gt5-common.dtsi   | 14 +++++++-------
> >   arch/arm64/boot/dts/qcom/msm8916-samsung-j5-common.dtsi    | 14 +++++++-------
> >   arch/arm64/boot/dts/qcom/msm8916-samsung-serranove.dts     | 14 +++++++-------
> >   arch/arm64/boot/dts/qcom/msm8916-ufi.dtsi                  | 14 +++++++-------
> >   arch/arm64/boot/dts/qcom/msm8916-wingtech-wt88047.dts      | 12 ++++++------
> >   13 files changed, 89 insertions(+), 89 deletions(-)
> > 
> > diff --git a/arch/arm64/boot/dts/qcom/msm8916-acer-a1-724.dts b/arch/arm64/boot/dts/qcom/msm8916-acer-a1-724.dts
> > index 13cd9ad167df..0d517804e44e 100644
> > --- a/arch/arm64/boot/dts/qcom/msm8916-acer-a1-724.dts
> > +++ b/arch/arm64/boot/dts/qcom/msm8916-acer-a1-724.dts
> > @@ -159,13 +159,13 @@ &smd_rpm_regulators {
> >   	vdd_l7-supply = <&pm8916_s4>;
> >   	s3 {
> > -		regulator-min-microvolt = <1200000>;
> > -		regulator-max-microvolt = <1300000>;
> > +		regulator-min-microvolt = <1250000>;
> > +		regulator-max-microvolt = <1350000>;
> 
> Where are you getting these 5s from ?
> 

I have two explanations for this, one is "documentation", the other is
"experimental testing". You can choose the one you find more convincing. :)

### Documentation ###

For documentation, the S3 range is defined in "PM8916/PM8916-1 Power
Management ICs - Device Specification - LM80-P0436-35 Rev.C - Table 3-16
Regulator high-level summary". The "Specified Range" for this regulator
is 1.25–1.35V.

Also, if you look at typical schematics (e.g. DB410c) you can see that
PM8916 S3 supplies the VDD_xxx_1P3 rails of the WCN3620/WCN3660/WCN3680
chip. Looking at the defined "Operating conditions" in the datasheets of
those they define "Min: 1.25V, Typ: 1.3V, Max: 1.38V". As such, 1.2V is
not even a valid voltage for the actual usage of this regulator.

### Experimental Testing ###

The reason why it still works in practice is that the RPM firmware does
not let you apply invalid voltages here. With experimental testing
I observed that it keeps the voltage always in the "Specified Range"
of 1.25–1.35V.

You can reproduce this easily by adding the SPMI regulators in
pm8916.dtsi. These represent the actual regulator hardware registers:

diff --git a/arch/arm64/boot/dts/qcom/pm8916.dtsi b/arch/arm64/boot/dts/qcom/pm8916.dtsi
index 864bb1cd68db..22ab0f59be4a 100644
--- a/arch/arm64/boot/dts/qcom/pm8916.dtsi
+++ b/arch/arm64/boot/dts/qcom/pm8916.dtsi
@@ -177,5 +177,9 @@ wcd_codec: audio-codec@f000 {
 			#sound-dai-cells = <1>;
 			status = "disabled";
 		};
+
+		regulators {
+			compatible = "qcom,pm8916-regulators";
+		};
 	};
 };

With this each of the regulators will show up twice in /sys/class/regulator.
Once the RPM version and once the SPMI version. If you check the
/sys/class/regulator/.../microvolts of the SPMI variant you can see the
actual voltage that was applied by the RPM firmware.

For example, if you think that 1.2V is really possible then try:

	s3 {
		regulator-min-microvolt = <1200000>;
		regulator-max-microvolt = <1200000>;
	};

In sysfs you will almost certainly see that the SPMI regulator is
actually configured with 1.25V by the RPM firmware. It does not allow
setting anything lower.

> >   	};
> >   	s4 {
> > -		regulator-min-microvolt = <1800000>;
> > -		regulator-max-microvolt = <2100000>;
> > +		regulator-min-microvolt = <1850000>;
> > +		regulator-max-microvolt = <2150000>;
> >   	};
> >   	l1 {
> > @@ -199,7 +199,7 @@ l7 {
> >   	};
> >   	l8 {
> > -		regulator-min-microvolt = <2850000>;
> > +		regulator-min-microvolt = <2900000>;
> >   		regulator-max-microvolt = <2900000>;
> >   	};
> > @@ -209,12 +209,12 @@ l9 {
> >   	};
> >   	l10 {
> > -		regulator-min-microvolt = <2700000>;
> > +		regulator-min-microvolt = <2800000>;
> >   		regulator-max-microvolt = <2800000>;
> >   	};
> >   	l11 {
> > -		regulator-min-microvolt = <1800000>;
> > +		regulator-min-microvolt = <2950000>;
> 
> Wouldn't 1v8 be the right voltage for eMMC !SD though have you tested eMMC
> instead of SD ?
> 

This is the supply voltage (not I/O voltage) which is also 2.9V for the
eMMC typically. But the point here is that only 2.95V can be set for
this regulator on most RPM firmware versions.

It does not matter what you connect there. I tried setting

	l11 {
		regulator-min-microvolt = <1800000>;
		regulator-max-microvolt = <1800000>;
	};

but the RPM firmware still applies 2.95V. In this case I even verified
this with a multimeter. The regulator is always 2.95V, even if you ask
the RPM firmware to apply 1.8V.

Thanks,
Stephan