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

[Konrad Dybcio]

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?

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@xxxxxxxxxxx>
> ---
>  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 {
>