linux-kernel - Re: [PATCH v3] arm64: dts: qcom: qdu/qru1000-idp: Fix the voltage setting

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <makbiq4pby5qoqpraonia3rorytb5uqhiab3tri5bjimdbcoc2@z6jkhg2u3reb>
Date: Tue, 26 Nov 2024 16:08:34 +0200
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Komal Bajaj <quic_kbajaj@...cinc.com>
Cc: Bjorn Andersson <andersson@...nel.org>, 
	Konrad Dybcio <konradybcio@...nel.org>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Melody Olvera <quic_molvera@...cinc.com>, linux-arm-msm@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] arm64: dts: qcom: qdu/qru1000-idp: Fix the voltage
 setting

On Mon, Nov 25, 2024 at 06:02:40PM +0530, Komal Bajaj wrote:
> On 11/20/2024 5:27 PM, Dmitry Baryshkov wrote:
> > On Tue, Nov 19, 2024 at 12:38:11PM +0530, Komal Bajaj wrote:
> > > While adding the USB support, it was found that the configuration
> > > for regulator smps5 was incorrectly set. Upon cross verifying for
> > > all the regulators, found that smps4, smps6 and smps8 are also
> > > incorrectly configured. The patch corrects these configurations.
> > > 
> > > In particular -
> > > - smps4 is 1.574V min and 2.04V max
> > > - smps5 is 1.2V min and 1.4V max
> > > - smps6 is 0.382V min and 1.12V max
> > > - smps8 is fixed at 0.752V
> > Could you please comment whether your values represent the min/max
> > supported by the regulators themselves or the shared min/max by all the
> > devices powered by the corresponding regulator?
> 
> values represent the min/max supported by the regulators themselves

This is incorrect. Please take a look around. The regulators in the
board files specify working ranges, which are suitable for the
particular board, not the ranges that are supported by the regulator
itself.

So, NAK. If working ranges need to be corrected, please send another
patch.

> 
> 
> > > Fixes: d1f2cfe2f669 ("arm64: dts: qcom: Add base QDU1000/QRU1000 IDP DTs")
> > > Signed-off-by: Komal Bajaj <quic_kbajaj@...cinc.com>
> > > ---
> > > Changes in v3 -
> > > * Minor nit pick in commit message
> > > * Link to v2: https://lore.kernel.org/all/20240524082236.24112-1-quic_kbajaj@quicinc.com/
> > > 
> > > Changes in v2-
> > > * Updated the commit message as suggested by Krzysztof
> > > * Link to v1: https://lore.kernel.org/linux-arm-msm/20240514131038.28036-1-quic_kbajaj@quicinc.com/
> > > ---
> > >   arch/arm64/boot/dts/qcom/qdu1000-idp.dts | 16 ++++++++--------
> > >   arch/arm64/boot/dts/qcom/qru1000-idp.dts | 16 ++++++++--------
> > >   2 files changed, 16 insertions(+), 16 deletions(-)
> > > 
> > > diff --git a/arch/arm64/boot/dts/qcom/qdu1000-idp.dts b/arch/arm64/boot/dts/qcom/qdu1000-idp.dts
> > > index e65305f8136c..6e8f9007068b 100644
> > > --- a/arch/arm64/boot/dts/qcom/qdu1000-idp.dts
> > > +++ b/arch/arm64/boot/dts/qcom/qdu1000-idp.dts
> > > @@ -96,20 +96,20 @@ vreg_s3a_1p05: smps3 {
> > > 
> > >   		vreg_s4a_1p8: smps4 {
> > >   			regulator-name = "vreg_s4a_1p8";
> > > -			regulator-min-microvolt = <1800000>;
> > > -			regulator-max-microvolt = <1800000>;
> > > +			regulator-min-microvolt = <1574000>;
> > > +			regulator-max-microvolt = <2040000>;
> > >   		};
> > > 
> > >   		vreg_s5a_2p0: smps5 {
> > >   			regulator-name = "vreg_s5a_2p0";
> > > -			regulator-min-microvolt = <1904000>;
> > > -			regulator-max-microvolt = <2000000>;
> > > +			regulator-min-microvolt = <1200000>;
> > > +			regulator-max-microvolt = <1400000>;
> > Having 2.0 V regulator with the range of 1.2V - 1.4V is strange.
> 
> The configuration for this regulator was incorrectly set, as its nominal
> range is 1.2 V.
> 
> There was no scenario utilizing this regulator, but we discovered this
> misconfiguration while enabling USB.

Well. Then the name also needs to be corrected. You can not have
1.2-1.4 V regulator having the 2p0 name.

> > >   		};
> > > 
> > >   		vreg_s6a_0p9: smps6 {
> > >   			regulator-name = "vreg_s6a_0p9";
> > > -			regulator-min-microvolt = <920000>;
> > > -			regulator-max-microvolt = <1128000>;
> > > +			regulator-min-microvolt = <382000>;
> > > +			regulator-max-microvolt = <1120000>;
> > The same applies to this regulator, 0.9V usually can not go to 0.382 V
> > and still let the devices to continue working.
> 
> 
> This regulator supports a minimum voltage, but it won't actually drop to
> that level. The previous voltage values did not align with the Power Grid.

What is the actual minimal voltage for the regulator on this board?
Please use it instead.

> 
> 
> > 
> > >   		};
> > > 
> > >   		vreg_s7a_1p2: smps7 {
> > > @@ -120,8 +120,8 @@ vreg_s7a_1p2: smps7 {
> > > 
> > >   		vreg_s8a_1p3: smps8 {
> > >   			regulator-name = "vreg_s8a_1p3";
> > > -			regulator-min-microvolt = <1352000>;
> > > -			regulator-max-microvolt = <1352000>;
> > > +			regulator-min-microvolt = <752000>;
> > > +			regulator-max-microvolt = <752000>;
> > 1.3V at 0.752V?
> 
> 
> same applies here as well.

Same comment. No 0.752V for 1.3V regulator.

> Thanks
> 
> Komal
> 
> 
> > 
> > >   		};
> > > 
> > >   		vreg_l1a_0p91: ldo1 {
> > > diff --git a/arch/arm64/boot/dts/qcom/qru1000-idp.dts b/arch/arm64/boot/dts/qcom/qru1000-idp.dts
> > > index 1c781d9e24cf..8b0ddc187ca0 100644
> > > --- a/arch/arm64/boot/dts/qcom/qru1000-idp.dts
> > > +++ b/arch/arm64/boot/dts/qcom/qru1000-idp.dts
> > > @@ -96,20 +96,20 @@ vreg_s3a_1p05: smps3 {
> > > 
> > >   		vreg_s4a_1p8: smps4 {
> > >   			regulator-name = "vreg_s4a_1p8";
> > > -			regulator-min-microvolt = <1800000>;
> > > -			regulator-max-microvolt = <1800000>;
> > > +			regulator-min-microvolt = <1574000>;
> > > +			regulator-max-microvolt = <2040000>;
> > >   		};
> > > 
> > >   		vreg_s5a_2p0: smps5 {
> > >   			regulator-name = "vreg_s5a_2p0";
> > > -			regulator-min-microvolt = <1904000>;
> > > -			regulator-max-microvolt = <2000000>;
> > > +			regulator-min-microvolt = <1200000>;
> > > +			regulator-max-microvolt = <1400000>;
> > >   		};
> > > 
> > >   		vreg_s6a_0p9: smps6 {
> > >   			regulator-name = "vreg_s6a_0p9";
> > > -			regulator-min-microvolt = <920000>;
> > > -			regulator-max-microvolt = <1128000>;
> > > +			regulator-min-microvolt = <382000>;
> > > +			regulator-max-microvolt = <1120000>;
> > >   		};
> > > 
> > >   		vreg_s7a_1p2: smps7 {
> > > @@ -120,8 +120,8 @@ vreg_s7a_1p2: smps7 {
> > > 
> > >   		vreg_s8a_1p3: smps8 {
> > >   			regulator-name = "vreg_s8a_1p3";
> > > -			regulator-min-microvolt = <1352000>;
> > > -			regulator-max-microvolt = <1352000>;
> > > +			regulator-min-microvolt = <752000>;
> > > +			regulator-max-microvolt = <752000>;
> > >   		};
> > > 
> > >   		vreg_l1a_0p91: ldo1 {
> > > --
> > > 2.46.0
> > > 

-- 
With best wishes
Dmitry