linux-kernel - Re: [PATCH 8/8] arm64: dts: qcom: msm8916-pm8916: Mark always-on regulators

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] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 26 May 2023 02:28:52 +0200
From:   Konrad Dybcio <konrad.dybcio@...aro.org>
To:     Stephan Gerhold <stephan@...hold.net>,
        Bjorn Andersson <andersson@...nel.org>
Cc:     Andy Gross <agross@...nel.org>,
        Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
        linux-arm-msm@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, phone-devel@...r.kernel.org,
        ~postmarketos/upstreaming@...ts.sr.ht
Subject: Re: [PATCH 8/8] arm64: dts: qcom: msm8916-pm8916: Mark always-on
 regulators



On 26.05.2023 01:39, Konrad Dybcio wrote:
> 
> 
> On 17.05.2023 20:48, Stephan Gerhold wrote:
>> Some of the regulators must be always-on to ensure correct operation of
>> the system, e.g. PM8916 L2 for the LPDDR RAM, L5 for most digital I/O
>> and L7 for the CPU PLL (strictly speaking the CPU PLL might only need
>> an active-only vote but this is not supported for regulators in
>> mainline currently).
> Would you be interested in implementing this?
Actually, I think currently all votes are active-only votes and what
we're missing is sleep-only (and active-sleep if we vote on both)

Konrad
> 
> Ancient downstream defines a second device (vregname_ao) and basically
> seems to select QCOM_SMD_(ACTIVE/SLEEP)_STATE based on that..
> 
> Looks like `struct regulator` stores voltage in an array that wouldn't
> you know it, depends on the PM state. Perhaps that could be something
> to explore!
> 
> Konrad
> 
>>
>> The RPM firmware seems to enforce that internally, these supplies stay
>> on even if we vote for them to power off (and there is no other
>> processor running). This means it's pointless to keep sending
>> enable/disable requests because they will just be ignored.
>> Also, drivers are much more likely to get a wrong impression of the
>> regulator status, because regulator_is_enabled() will return false when
>> there are no users, even though the regulator is always on.
>>
>> Describe this properly by marking the regulators as always-on.
>>
>> Signed-off-by: Stephan Gerhold <stephan@...hold.net>
>> ---
>>  arch/arm64/boot/dts/qcom/apq8016-sbc.dts     | 5 -----
>>  arch/arm64/boot/dts/qcom/msm8916-pm8916.dtsi | 5 +++++
>>  2 files changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/apq8016-sbc.dts b/arch/arm64/boot/dts/qcom/apq8016-sbc.dts
>> index ab8dfd858025..1c5d55854893 100644
>> --- a/arch/arm64/boot/dts/qcom/apq8016-sbc.dts
>> +++ b/arch/arm64/boot/dts/qcom/apq8016-sbc.dts
>> @@ -358,11 +358,6 @@ pm8916_l17: l17 {
>>  	};
>>  };
>>  
>> -&pm8916_s4 {
>> -	regulator-always-on;
>> -	regulator-boot-on;
>> -};
>> -
>>  &sdhc_1 {
>>  	status = "okay";
>>  
>> diff --git a/arch/arm64/boot/dts/qcom/msm8916-pm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916-pm8916.dtsi
>> index b38eecbd6253..64d7228bee07 100644
>> --- a/arch/arm64/boot/dts/qcom/msm8916-pm8916.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/msm8916-pm8916.dtsi
>> @@ -72,11 +72,13 @@ pm8916_rpm_regulators: regulators {
>>  		pm8916_s3: s3 {
>>  			regulator-min-microvolt = <1250000>;
>>  			regulator-max-microvolt = <1350000>;
>> +			regulator-always-on; /* Needed for L2 */
>>  		};
>>  
>>  		pm8916_s4: s4 {
>>  			regulator-min-microvolt = <1850000>;
>>  			regulator-max-microvolt = <2150000>;
>> +			regulator-always-on; /* Needed for L5/L7 */
>>  		};
>>  
>>  		/*
>> @@ -93,6 +95,7 @@ pm8916_s4: s4 {
>>  		pm8916_l2: l2 {
>>  			regulator-min-microvolt = <1200000>;
>>  			regulator-max-microvolt = <1200000>;
>> +			regulator-always-on; /* Needed for LPDDR RAM */
>>  		};
>>  
>>  		/* pm8916_l3 is managed by rpmpd (MSM8916_VDDMX) */
>> @@ -102,6 +105,7 @@ pm8916_l2: l2 {
>>  		pm8916_l5: l5 {
>>  			regulator-min-microvolt = <1800000>;
>>  			regulator-max-microvolt = <1800000>;
>> +			regulator-always-on; /* Needed for most digital I/O */
>>  		};
>>  
>>  		pm8916_l6: l6 {
>> @@ -112,6 +116,7 @@ pm8916_l6: l6 {
>>  		pm8916_l7: l7 {
>>  			regulator-min-microvolt = <1800000>;
>>  			regulator-max-microvolt = <1800000>;
>> +			regulator-always-on; /* Needed for CPU PLL */
>>  		};
>>  
>>  		pm8916_l8: l8 {
>>