mbox series

[0/8] arm64: dts: qcom: msm8916: Rework regulator constraints

Message ID 20230510-msm8916-regulators-v1-0-54d4960a05fc@gerhold.net
Headers show
Series arm64: dts: qcom: msm8916: Rework regulator constraints | expand

Message

Stephan Gerhold May 17, 2023, 6:48 p.m. UTC
Rework the regulator constraints for the MSM8916 device trees to be 
closer to reality. There are several mistakes in there, some of them 
taken over directly from Qualcomm's vendor kernel. Fortunately, none of 
the mistakes is absolutely critical because it turns out that the RPM 
firmware also validates the voltages and silently clamps the requests 
to a proper range. Still, this behavior should be clearly represented 
in the device tree rather than pretending to apply the wrong voltages.

To make the regulator constraints more easily maintainable with a large 
number of similar MSM8916 boards I propose moving the voltages for the 
standard components in the SoC to the shared msm8916-pm8916.dtsi 
include. With this only the actual board-specific regulators are 
described in the board DT.

Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
Stephan Gerhold (8):
      arm64: dts: qcom: apq8016-sbc: Fix regulator constraints
      arm64: dts: qcom: apq8016-sbc: Fix 1.8V power rail on LS expansion
      arm64: dts: qcom: msm8916: Fix regulator constraints
      arm64: dts: qcom: msm8916: Disable audio codecs by default
      arm64: dts: qcom: pm8916: Move default regulator "-supply"s
      arm64: dts: qcom: msm8916-pm8916: Clarify purpose
      arm64: dts: qcom: msm8916: Define regulator constraints next to usage
      arm64: dts: qcom: msm8916-pm8916: Mark always-on regulators

 arch/arm64/boot/dts/qcom/apq8016-sbc.dts           | 145 +++++----------------
 arch/arm64/boot/dts/qcom/msm8916-acer-a1-724.dts   | 115 ++--------------
 .../boot/dts/qcom/msm8916-alcatel-idol347.dts      | 110 +---------------
 arch/arm64/boot/dts/qcom/msm8916-asus-z00l.dts     | 110 +---------------
 arch/arm64/boot/dts/qcom/msm8916-gplus-fl8005a.dts | 110 +---------------
 arch/arm64/boot/dts/qcom/msm8916-huawei-g7.dts     | 125 ++++--------------
 .../boot/dts/qcom/msm8916-longcheer-l8150.dts      | 110 +---------------
 .../boot/dts/qcom/msm8916-longcheer-l8910.dts      | 110 +---------------
 arch/arm64/boot/dts/qcom/msm8916-pm8916.dtsi       | 121 ++++++++++++++---
 .../dts/qcom/msm8916-samsung-a2015-common.dtsi     | 110 +---------------
 .../boot/dts/qcom/msm8916-samsung-gt5-common.dtsi  | 110 +---------------
 .../boot/dts/qcom/msm8916-samsung-j5-common.dtsi   | 103 ---------------
 .../boot/dts/qcom/msm8916-samsung-serranove.dts    | 103 ---------------
 arch/arm64/boot/dts/qcom/msm8916-ufi.dtsi          | 103 ---------------
 .../boot/dts/qcom/msm8916-wingtech-wt88047.dts     | 119 +++--------------
 arch/arm64/boot/dts/qcom/msm8916.dtsi              |   1 +
 arch/arm64/boot/dts/qcom/pm8916.dtsi               |   4 +-
 17 files changed, 236 insertions(+), 1473 deletions(-)
---
base-commit: 4272e06e19f388ccfe1f04f19060ea84d2a19a8b
change-id: 20230510-msm8916-regulators-97fa33735efe

Best regards,

Comments

Konrad Dybcio May 25, 2023, 11:35 p.m. UTC | #1
On 17.05.2023 20:48, Stephan Gerhold wrote:
> Right now each MSM8916 device has a huge block of regulator constraints
> with allowed voltages for each regulator. For lack of better
> documentation these voltages are often copied as-is from the vendor
> device tree, without much extra thought.
> 
> Unfortunately, the voltages in the vendor device trees are often
> misleading or even wrong, e.g. because:
> 
>  - There is a large voltage range allowed and the actual voltage is
>    only set somewhere hidden in some messy vendor driver. This is often
>    the case for pm8916_{l14,l15,l16} because they have a broad range of
>    1.8-3.3V by default.
> 
>  - The voltage is actually wrong but thanks to the voltage constraints
>    in the RPM firmware it still ends up applying the correct voltage.
> 
> To have proper regulator constraints it is important to review them in
> context of the usage. The current setup in the MSM8916 device trees
> makes this quite hard because each device duplicates the standard
> voltages for components of the SoC and mixes those with minor
> device-specific additions and dummy voltages for completely unused
> regulators.
> 
> The actual usage of the regulators for the SoC components is in
> msm8916-pm8916.dtsi, so it can and should also define the related
> voltage constraints. These are not board-specific but defined in the
> APQ8016E/PM8916 Device Specification. The board DT can then focus on
> describing the actual board-specific regulators, which makes it much
> easier to review and spot potential mistakes there.
> 
> Note that this commit does not make any functional change. All used
> regulators still have the same regulator constraints as before. Unused
> regulators do not have regulator constraints anymore because most of
> these were too broad or even entirely wrong. They should be added back
> with proper voltage constraints when there is an actual usage.
> 
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> ---
I'm a bit torn between saying "this is very nice already" and "we should
probably override each regulator individually" like so:

&pm8916_l17 {
	[...]
}

to minimize mistakes..

Not sure what to make of it, I see Bjorn already applied this, so I guess
I'm just leaving some potential ideas for the future here.

Konrad

>  arch/arm64/boot/dts/qcom/apq8016-sbc.dts           | 153 +++++----------------
>  arch/arm64/boot/dts/qcom/msm8916-acer-a1-724.dts   | 115 ++--------------
>  .../boot/dts/qcom/msm8916-alcatel-idol347.dts      | 110 +--------------
>  arch/arm64/boot/dts/qcom/msm8916-asus-z00l.dts     | 110 +--------------
>  arch/arm64/boot/dts/qcom/msm8916-gplus-fl8005a.dts | 110 +--------------
>  arch/arm64/boot/dts/qcom/msm8916-huawei-g7.dts     | 120 +++-------------
>  .../boot/dts/qcom/msm8916-longcheer-l8150.dts      | 110 +--------------
>  .../boot/dts/qcom/msm8916-longcheer-l8910.dts      | 110 +--------------
>  arch/arm64/boot/dts/qcom/msm8916-pm8916.dtsi       | 102 +++++++++++---
>  .../dts/qcom/msm8916-samsung-a2015-common.dtsi     | 110 +--------------
>  .../boot/dts/qcom/msm8916-samsung-gt5-common.dtsi  | 110 +--------------
>  .../boot/dts/qcom/msm8916-samsung-j5-common.dtsi   | 103 --------------
>  .../boot/dts/qcom/msm8916-samsung-serranove.dts    | 103 --------------
>  arch/arm64/boot/dts/qcom/msm8916-ufi.dtsi          | 103 --------------
>  .../boot/dts/qcom/msm8916-wingtech-wt88047.dts     | 119 +++-------------
>  15 files changed, 210 insertions(+), 1478 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/apq8016-sbc.dts b/arch/arm64/boot/dts/qcom/apq8016-sbc.dts
> index 7d7af6406c39..ab8dfd858025 100644
> --- a/arch/arm64/boot/dts/qcom/apq8016-sbc.dts
> +++ b/arch/arm64/boot/dts/qcom/apq8016-sbc.dts
> @@ -329,6 +329,40 @@ &pm8916_resin {
>  	linux,code = <KEY_VOLUMEDOWN>;
>  };
>  
> +&pm8916_rpm_regulators {
> +	/*
> +	 * The 96Boards specification expects a 1.8V power rail on the low-speed
> +	 * expansion connector that is able to provide at least 0.18W / 100 mA.
> +	 * L15/L16 are connected in parallel to provide 55 mA each. A minimum load
> +	 * must be specified to ensure the regulators are not put in LPM where they
> +	 * would only provide 5 mA.
> +	 */
> +	pm8916_l15: l15 {
> +		regulator-min-microvolt = <1800000>;
> +		regulator-max-microvolt = <1800000>;
> +		regulator-system-load = <50000>;
> +		regulator-allow-set-load;
> +		regulator-always-on;
> +	};
> +	pm8916_l16: l16 {
> +		regulator-min-microvolt = <1800000>;
> +		regulator-max-microvolt = <1800000>;
> +		regulator-system-load = <50000>;
> +		regulator-allow-set-load;
> +		regulator-always-on;
> +	};
> +
> +	pm8916_l17: l17 {
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +	};
> +};
> +
> +&pm8916_s4 {
> +	regulator-always-on;
> +	regulator-boot-on;
> +};
> +
>  &sdhc_1 {
>  	status = "okay";
>  
> @@ -446,125 +480,6 @@ &wcnss_iris {
>  &stm { status = "okay"; };
>  &tpiu { status = "okay"; };
>  
> -&smd_rpm_regulators {
> -	vdd_l1_l2_l3-supply = <&pm8916_s3>;
> -	vdd_l4_l5_l6-supply = <&pm8916_s4>;
> -	vdd_l7-supply = <&pm8916_s4>;
> -
> -	s3 {
> -		regulator-min-microvolt = <1250000>;
> -		regulator-max-microvolt = <1350000>;
> -	};
> -
> -	s4 {
> -		regulator-min-microvolt = <1850000>;
> -		regulator-max-microvolt = <2150000>;
> -
> -		regulator-always-on;
> -		regulator-boot-on;
> -	};
> -
> -	l1 {
> -		regulator-min-microvolt = <1225000>;
> -		regulator-max-microvolt = <1225000>;
> -	};
> -
> -	l2 {
> -		regulator-min-microvolt = <1200000>;
> -		regulator-max-microvolt = <1200000>;
> -	};
> -
> -	l4 {
> -		regulator-min-microvolt = <2050000>;
> -		regulator-max-microvolt = <2050000>;
> -	};
> -
> -	l5 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <1800000>;
> -	};
> -
> -	l6 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <1800000>;
> -	};
> -
> -	l7 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <1800000>;
> -	};
> -
> -	l8 {
> -		regulator-min-microvolt = <2900000>;
> -		regulator-max-microvolt = <2900000>;
> -	};
> -
> -	l9 {
> -		regulator-min-microvolt = <3300000>;
> -		regulator-max-microvolt = <3300000>;
> -	};
> -
> -	l10 {
> -		regulator-min-microvolt = <2800000>;
> -		regulator-max-microvolt = <2800000>;
> -	};
> -
> -	l11 {
> -		regulator-min-microvolt = <2950000>;
> -		regulator-max-microvolt = <2950000>;
> -		regulator-allow-set-load;
> -		regulator-system-load = <200000>;
> -	};
> -
> -	l12 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <2950000>;
> -	};
> -
> -	l13 {
> -		regulator-min-microvolt = <3075000>;
> -		regulator-max-microvolt = <3075000>;
> -	};
> -
> -	l14 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <3300000>;
> -	};
> -
> -	/*
> -	 * The 96Boards specification expects a 1.8V power rail on the low-speed
> -	 * expansion connector that is able to provide at least 0.18W / 100 mA.
> -	 * L15/L16 are connected in parallel to provide 55 mA each. A minimum load
> -	 * must be specified to ensure the regulators are not put in LPM where they
> -	 * would only provide 5 mA.
> -	 */
> -	l15 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <1800000>;
> -		regulator-system-load = <50000>;
> -		regulator-allow-set-load;
> -		regulator-always-on;
> -	};
> -
> -	l16 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <1800000>;
> -		regulator-system-load = <50000>;
> -		regulator-allow-set-load;
> -		regulator-always-on;
> -	};
> -
> -	l17 {
> -		regulator-min-microvolt = <3300000>;
> -		regulator-max-microvolt = <3300000>;
> -	};
> -
> -	l18 {
> -		regulator-min-microvolt = <2700000>;
> -		regulator-max-microvolt = <2700000>;
> -	};
> -};
> -
>  /*
>   * 2mA drive strength is not enough when connecting multiple
>   * I2C devices with different pull up resistors.
> 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 0d517804e44e..753413b48751 100644
> --- a/arch/arm64/boot/dts/qcom/msm8916-acer-a1-724.dts
> +++ b/arch/arm64/boot/dts/qcom/msm8916-acer-a1-724.dts
> @@ -114,6 +114,18 @@ &pm8916_resin {
>  	status = "okay";
>  };
>  
> +&pm8916_rpm_regulators {
> +	pm8916_l16: l16 {
> +		regulator-min-microvolt = <2900000>;
> +		regulator-max-microvolt = <2900000>;
> +	};
> +
> +	pm8916_l17: l17 {
> +		regulator-min-microvolt = <2850000>;
> +		regulator-max-microvolt = <2850000>;
> +	};
> +};
> +
>  &pm8916_vib {
>  	status = "okay";
>  };
> @@ -153,109 +165,6 @@ &wcnss_iris {
>  	compatible = "qcom,wcn3620";
>  };
>  
> -&smd_rpm_regulators {
> -	vdd_l1_l2_l3-supply = <&pm8916_s3>;
> -	vdd_l4_l5_l6-supply = <&pm8916_s4>;
> -	vdd_l7-supply = <&pm8916_s4>;
> -
> -	s3 {
> -		regulator-min-microvolt = <1250000>;
> -		regulator-max-microvolt = <1350000>;
> -	};
> -
> -	s4 {
> -		regulator-min-microvolt = <1850000>;
> -		regulator-max-microvolt = <2150000>;
> -	};
> -
> -	l1 {
> -		regulator-min-microvolt = <1225000>;
> -		regulator-max-microvolt = <1225000>;
> -	};
> -
> -	l2 {
> -		regulator-min-microvolt = <1200000>;
> -		regulator-max-microvolt = <1200000>;
> -	};
> -
> -	l4 {
> -		regulator-min-microvolt = <2050000>;
> -		regulator-max-microvolt = <2050000>;
> -	};
> -
> -	l5 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <1800000>;
> -	};
> -
> -	l6 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <1800000>;
> -	};
> -
> -	l7 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <1800000>;
> -	};
> -
> -	l8 {
> -		regulator-min-microvolt = <2900000>;
> -		regulator-max-microvolt = <2900000>;
> -	};
> -
> -	l9 {
> -		regulator-min-microvolt = <3300000>;
> -		regulator-max-microvolt = <3300000>;
> -	};
> -
> -	l10 {
> -		regulator-min-microvolt = <2800000>;
> -		regulator-max-microvolt = <2800000>;
> -	};
> -
> -	l11 {
> -		regulator-min-microvolt = <2950000>;
> -		regulator-max-microvolt = <2950000>;
> -		regulator-system-load = <200000>;
> -		regulator-allow-set-load;
> -	};
> -
> -	l12 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <2950000>;
> -	};
> -
> -	l13 {
> -		regulator-min-microvolt = <3075000>;
> -		regulator-max-microvolt = <3075000>;
> -	};
> -
> -	l14 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <3300000>;
> -	};
> -
> -	l15 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <3300000>;
> -	};
> -
> -	l16 {
> -		regulator-min-microvolt = <2900000>;
> -		regulator-max-microvolt = <2900000>;
> -	};
> -
> -	l17 {
> -		regulator-min-microvolt = <2850000>;
> -		regulator-max-microvolt = <2850000>;
> -	};
> -
> -	l18 {
> -		regulator-min-microvolt = <2700000>;
> -		regulator-max-microvolt = <2700000>;
> -	};
> -};
> -
>  &msmgpio {
>  	accel_int_default: accel-int-default-state {
>  		pins = "gpio115";
> diff --git a/arch/arm64/boot/dts/qcom/msm8916-alcatel-idol347.dts b/arch/arm64/boot/dts/qcom/msm8916-alcatel-idol347.dts
> index ddd64cc46998..4bfbad669b1e 100644
> --- a/arch/arm64/boot/dts/qcom/msm8916-alcatel-idol347.dts
> +++ b/arch/arm64/boot/dts/qcom/msm8916-alcatel-idol347.dts
> @@ -156,6 +156,13 @@ &pm8916_resin {
>  	linux,code = <KEY_VOLUMEDOWN>;
>  };
>  
> +&pm8916_rpm_regulators {
> +	pm8916_l17: l17 {
> +		regulator-min-microvolt = <2850000>;
> +		regulator-max-microvolt = <2850000>;
> +	};
> +};
> +
>  &pm8916_vib {
>  	status = "okay";
>  };
> @@ -195,109 +202,6 @@ &wcnss_iris {
>  	compatible = "qcom,wcn3620";
>  };
>  
> -&smd_rpm_regulators {
> -	vdd_l1_l2_l3-supply = <&pm8916_s3>;
> -	vdd_l4_l5_l6-supply = <&pm8916_s4>;
> -	vdd_l7-supply = <&pm8916_s4>;
> -
> -	s3 {
> -		regulator-min-microvolt = <1250000>;
> -		regulator-max-microvolt = <1350000>;
> -	};
> -
> -	s4 {
> -		regulator-min-microvolt = <1850000>;
> -		regulator-max-microvolt = <2150000>;
> -	};
> -
> -	l1 {
> -		regulator-min-microvolt = <1225000>;
> -		regulator-max-microvolt = <1225000>;
> -	};
> -
> -	l2 {
> -		regulator-min-microvolt = <1200000>;
> -		regulator-max-microvolt = <1200000>;
> -	};
> -
> -	l4 {
> -		regulator-min-microvolt = <2050000>;
> -		regulator-max-microvolt = <2050000>;
> -	};
> -
> -	l5 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <1800000>;
> -	};
> -
> -	l6 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <1800000>;
> -	};
> -
> -	l7 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <1800000>;
> -	};
> -
> -	l8 {
> -		regulator-min-microvolt = <2900000>;
> -		regulator-max-microvolt = <2900000>;
> -	};
> -
> -	l9 {
> -		regulator-min-microvolt = <3300000>;
> -		regulator-max-microvolt = <3300000>;
> -	};
> -
> -	l10 {
> -		regulator-min-microvolt = <2800000>;
> -		regulator-max-microvolt = <2800000>;
> -	};
> -
> -	l11 {
> -		regulator-min-microvolt = <2950000>;
> -		regulator-max-microvolt = <2950000>;
> -		regulator-allow-set-load;
> -		regulator-system-load = <200000>;
> -	};
> -
> -	l12 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <2950000>;
> -	};
> -
> -	l13 {
> -		regulator-min-microvolt = <3075000>;
> -		regulator-max-microvolt = <3075000>;
> -	};
> -
> -	l14 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <3300000>;
> -	};
> -
> -	l15 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <3300000>;
> -	};
> -
> -	l16 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <3300000>;
> -	};
> -
> -	l17 {
> -		regulator-min-microvolt = <2850000>;
> -		regulator-max-microvolt = <2850000>;
> -	};
> -
> -	l18 {
> -		regulator-min-microvolt = <2700000>;
> -		regulator-max-microvolt = <2700000>;
> -	};
> -};
> -
>  &msmgpio {
>  	accel_int_default: accel-int-default-state {
>  		pins = "gpio31";
> diff --git a/arch/arm64/boot/dts/qcom/msm8916-asus-z00l.dts b/arch/arm64/boot/dts/qcom/msm8916-asus-z00l.dts
> index 982457503a3c..37755e885395 100644
> --- a/arch/arm64/boot/dts/qcom/msm8916-asus-z00l.dts
> +++ b/arch/arm64/boot/dts/qcom/msm8916-asus-z00l.dts
> @@ -128,6 +128,13 @@ &blsp1_uart2 {
>  	status = "okay";
>  };
>  
> +&pm8916_rpm_regulators {
> +	pm8916_l17: l17 {
> +		regulator-min-microvolt = <2850000>;
> +		regulator-max-microvolt = <2850000>;
> +	};
> +};
> +
>  &sdhc_1 {
>  	status = "okay";
>  
> @@ -163,109 +170,6 @@ &wcnss_iris {
>  	compatible = "qcom,wcn3620";
>  };
>  
> -&smd_rpm_regulators {
> -	vdd_l1_l2_l3-supply = <&pm8916_s3>;
> -	vdd_l4_l5_l6-supply = <&pm8916_s4>;
> -	vdd_l7-supply = <&pm8916_s4>;
> -
> -	s3 {
> -		regulator-min-microvolt = <1250000>;
> -		regulator-max-microvolt = <1350000>;
> -	};
> -
> -	s4 {
> -		regulator-min-microvolt = <1850000>;
> -		regulator-max-microvolt = <2150000>;
> -	};
> -
> -	l1 {
> -		regulator-min-microvolt = <1225000>;
> -		regulator-max-microvolt = <1225000>;
> -	};
> -
> -	l2 {
> -		regulator-min-microvolt = <1200000>;
> -		regulator-max-microvolt = <1200000>;
> -	};
> -
> -	l4 {
> -		regulator-min-microvolt = <2050000>;
> -		regulator-max-microvolt = <2050000>;
> -	};
> -
> -	l5 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <1800000>;
> -	};
> -
> -	l6 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <1800000>;
> -	};
> -
> -	l7 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <1800000>;
> -	};
> -
> -	l8 {
> -		regulator-min-microvolt = <2900000>;
> -		regulator-max-microvolt = <2900000>;
> -	};
> -
> -	l9 {
> -		regulator-min-microvolt = <3300000>;
> -		regulator-max-microvolt = <3300000>;
> -	};
> -
> -	l10 {
> -		regulator-min-microvolt = <2800000>;
> -		regulator-max-microvolt = <2800000>;
> -	};
> -
> -	l11 {
> -		regulator-min-microvolt = <2950000>;
> -		regulator-max-microvolt = <2950000>;
> -		regulator-allow-set-load;
> -		regulator-system-load = <200000>;
> -	};
> -
> -	l12 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <2950000>;
> -	};
> -
> -	l13 {
> -		regulator-min-microvolt = <3075000>;
> -		regulator-max-microvolt = <3075000>;
> -	};
> -
> -	l14 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <3300000>;
> -	};
> -
> -	l15 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <3300000>;
> -	};
> -
> -	l16 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <3300000>;
> -	};
> -
> -	l17 {
> -		regulator-min-microvolt = <2850000>;
> -		regulator-max-microvolt = <2850000>;
> -	};
> -
> -	l18 {
> -		regulator-min-microvolt = <2700000>;
> -		regulator-max-microvolt = <2700000>;
> -	};
> -};
> -
>  &msmgpio {
>  	gpio_keys_default: gpio-keys-default-state {
>  		pins = "gpio107", "gpio117";
> diff --git a/arch/arm64/boot/dts/qcom/msm8916-gplus-fl8005a.dts b/arch/arm64/boot/dts/qcom/msm8916-gplus-fl8005a.dts
> index 9584d271c526..4a6bf99c755f 100644
> --- a/arch/arm64/boot/dts/qcom/msm8916-gplus-fl8005a.dts
> +++ b/arch/arm64/boot/dts/qcom/msm8916-gplus-fl8005a.dts
> @@ -114,6 +114,13 @@ &pm8916_resin {
>  	status = "okay";
>  };
>  
> +&pm8916_rpm_regulators {
> +	pm8916_l17: l17 {
> +		regulator-min-microvolt = <2850000>;
> +		regulator-max-microvolt = <2850000>;
> +	};
> +};
> +
>  &pm8916_vib {
>  	status = "okay";
>  };
> @@ -153,109 +160,6 @@ &wcnss_iris {
>  	compatible = "qcom,wcn3620";
>  };
>  
> -&smd_rpm_regulators {
> -	vdd_l1_l2_l3-supply = <&pm8916_s3>;
> -	vdd_l4_l5_l6-supply = <&pm8916_s4>;
> -	vdd_l7-supply = <&pm8916_s4>;
> -
> -	s3 {
> -		regulator-min-microvolt = <1250000>;
> -		regulator-max-microvolt = <1350000>;
> -	};
> -
> -	s4 {
> -		regulator-min-microvolt = <1850000>;
> -		regulator-max-microvolt = <2150000>;
> -	};
> -
> -	l1 {
> -		regulator-min-microvolt = <1225000>;
> -		regulator-max-microvolt = <1225000>;
> -	};
> -
> -	l2 {
> -		regulator-min-microvolt = <1200000>;
> -		regulator-max-microvolt = <1200000>;
> -	};
> -
> -	l4 {
> -		regulator-min-microvolt = <2050000>;
> -		regulator-max-microvolt = <2050000>;
> -	};
> -
> -	l5 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <1800000>;
> -	};
> -
> -	l6 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <1800000>;
> -	};
> -
> -	l7 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <1800000>;
> -	};
> -
> -	l8 {
> -		regulator-min-microvolt = <2900000>;
> -		regulator-max-microvolt = <2900000>;
> -	};
> -
> -	l9 {
> -		regulator-min-microvolt = <3300000>;
> -		regulator-max-microvolt = <3300000>;
> -	};
> -
> -	l10 {
> -		regulator-min-microvolt = <2800000>;
> -		regulator-max-microvolt = <2800000>;
> -	};
> -
> -	l11 {
> -		regulator-min-microvolt = <2950000>;
> -		regulator-max-microvolt = <2950000>;
> -		regulator-system-load = <200000>;
> -		regulator-allow-set-load;
> -	};
> -
> -	l12 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <2950000>;
> -	};
> -
> -	l13 {
> -		regulator-min-microvolt = <3075000>;
> -		regulator-max-microvolt = <3075000>;
> -	};
> -
> -	l14 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <3300000>;
> -	};
> -
> -	l15 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <3300000>;
> -	};
> -
> -	l16 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <3300000>;
> -	};
> -
> -	l17 {
> -		regulator-min-microvolt = <2850000>;
> -		regulator-max-microvolt = <2850000>;
> -	};
> -
> -	l18 {
> -		regulator-min-microvolt = <2700000>;
> -		regulator-max-microvolt = <2700000>;
> -	};
> -};
> -
>  &msmgpio {
>  	camera_flash_default: camera-flash-default-state {
>  		pins = "gpio31", "gpio32";
> diff --git a/arch/arm64/boot/dts/qcom/msm8916-huawei-g7.dts b/arch/arm64/boot/dts/qcom/msm8916-huawei-g7.dts
> index 8197710372ad..29aaa35438a3 100644
> --- a/arch/arm64/boot/dts/qcom/msm8916-huawei-g7.dts
> +++ b/arch/arm64/boot/dts/qcom/msm8916-huawei-g7.dts
> @@ -222,11 +222,28 @@ &lpass_codec {
>  	status = "okay";
>  };
>  
> +&pm8916_l8 {
> +	regulator-min-microvolt = <2950000>;
> +	regulator-max-microvolt = <2950000>;
> +};
> +
>  &pm8916_resin {
>  	status = "okay";
>  	linux,code = <KEY_VOLUMEDOWN>;
>  };
>  
> +&pm8916_rpm_regulators {
> +	pm8916_l16: l16 {
> +		regulator-min-microvolt = <1800000>;
> +		regulator-max-microvolt = <1800000>;
> +	};
> +
> +	pm8916_l17: l17 {
> +		regulator-min-microvolt = <2850000>;
> +		regulator-max-microvolt = <2850000>;
> +	};
> +};
> +
>  &pm8916_vib {
>  	status = "okay";
>  };
> @@ -321,109 +338,6 @@ &wcnss_iris {
>  	compatible = "qcom,wcn3620";
>  };
>  
> -&smd_rpm_regulators {
> -	vdd_l1_l2_l3-supply = <&pm8916_s3>;
> -	vdd_l4_l5_l6-supply = <&pm8916_s4>;
> -	vdd_l7-supply = <&pm8916_s4>;
> -
> -	s3 {
> -		regulator-min-microvolt = <1250000>;
> -		regulator-max-microvolt = <1350000>;
> -	};
> -
> -	s4 {
> -		regulator-min-microvolt = <1850000>;
> -		regulator-max-microvolt = <2150000>;
> -	};
> -
> -	l1 {
> -		regulator-min-microvolt = <1225000>;
> -		regulator-max-microvolt = <1225000>;
> -	};
> -
> -	l2 {
> -		regulator-min-microvolt = <1200000>;
> -		regulator-max-microvolt = <1200000>;
> -	};
> -
> -	l4 {
> -		regulator-min-microvolt = <2050000>;
> -		regulator-max-microvolt = <2050000>;
> -	};
> -
> -	l5 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <1800000>;
> -	};
> -
> -	l6 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <1800000>;
> -	};
> -
> -	l7 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <1800000>;
> -	};
> -
> -	l8 {
> -		regulator-min-microvolt = <2950000>;
> -		regulator-max-microvolt = <2950000>;
> -	};
> -
> -	l9 {
> -		regulator-min-microvolt = <3300000>;
> -		regulator-max-microvolt = <3300000>;
> -	};
> -
> -	l10 {
> -		regulator-min-microvolt = <2800000>;
> -		regulator-max-microvolt = <2800000>;
> -	};
> -
> -	l11 {
> -		regulator-min-microvolt = <2950000>;
> -		regulator-max-microvolt = <2950000>;
> -		regulator-allow-set-load;
> -		regulator-system-load = <200000>;
> -	};
> -
> -	l12 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <2950000>;
> -	};
> -
> -	l13 {
> -		regulator-min-microvolt = <3075000>;
> -		regulator-max-microvolt = <3075000>;
> -	};
> -
> -	l14 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <3300000>;
> -	};
> -
> -	l15 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <3300000>;
> -	};
> -
> -	l16 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <1800000>;
> -	};
> -
> -	l17 {
> -		regulator-min-microvolt = <2850000>;
> -		regulator-max-microvolt = <2850000>;
> -	};
> -
> -	l18 {
> -		regulator-min-microvolt = <2700000>;
> -		regulator-max-microvolt = <2700000>;
> -	};
> -};
> -
>  &msmgpio {
>  	accel_irq_default: accel-irq-default-state {
>  		pins = "gpio115";
> diff --git a/arch/arm64/boot/dts/qcom/msm8916-longcheer-l8150.dts b/arch/arm64/boot/dts/qcom/msm8916-longcheer-l8150.dts
> index 68d1b76aaf77..b7b1f1ceaf1f 100644
> --- a/arch/arm64/boot/dts/qcom/msm8916-longcheer-l8150.dts
> +++ b/arch/arm64/boot/dts/qcom/msm8916-longcheer-l8150.dts
> @@ -223,6 +223,13 @@ &pm8916_resin {
>  	linux,code = <KEY_VOLUMEDOWN>;
>  };
>  
> +&pm8916_rpm_regulators {
> +	pm8916_l17: l17 {
> +		regulator-min-microvolt = <2850000>;
> +		regulator-max-microvolt = <2850000>;
> +	};
> +};
> +
>  &pm8916_usbin {
>  	status = "okay";
>  };
> @@ -267,109 +274,6 @@ &wcnss_iris {
>  	compatible = "qcom,wcn3620";
>  };
>  
> -&smd_rpm_regulators {
> -	vdd_l1_l2_l3-supply = <&pm8916_s3>;
> -	vdd_l4_l5_l6-supply = <&pm8916_s4>;
> -	vdd_l7-supply = <&pm8916_s4>;
> -
> -	s3 {
> -		regulator-min-microvolt = <1250000>;
> -		regulator-max-microvolt = <1350000>;
> -	};
> -
> -	s4 {
> -		regulator-min-microvolt = <1850000>;
> -		regulator-max-microvolt = <2150000>;
> -	};
> -
> -	l1 {
> -		regulator-min-microvolt = <1225000>;
> -		regulator-max-microvolt = <1225000>;
> -	};
> -
> -	l2 {
> -		regulator-min-microvolt = <1200000>;
> -		regulator-max-microvolt = <1200000>;
> -	};
> -
> -	l4 {
> -		regulator-min-microvolt = <2050000>;
> -		regulator-max-microvolt = <2050000>;
> -	};
> -
> -	l5 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <1800000>;
> -	};
> -
> -	l6 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <1800000>;
> -	};
> -
> -	l7 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <1800000>;
> -	};
> -
> -	l8 {
> -		regulator-min-microvolt = <2900000>;
> -		regulator-max-microvolt = <2900000>;
> -	};
> -
> -	l9 {
> -		regulator-min-microvolt = <3300000>;
> -		regulator-max-microvolt = <3300000>;
> -	};
> -
> -	l10 {
> -		regulator-min-microvolt = <2800000>;
> -		regulator-max-microvolt = <2800000>;
> -	};
> -
> -	l11 {
> -		regulator-min-microvolt = <2950000>;
> -		regulator-max-microvolt = <2950000>;
> -		regulator-allow-set-load;
> -		regulator-system-load = <200000>;
> -	};
> -
> -	l12 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <2950000>;
> -	};
> -
> -	l13 {
> -		regulator-min-microvolt = <3075000>;
> -		regulator-max-microvolt = <3075000>;
> -	};
> -
> -	l14 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <3300000>;
> -	};
> -
> -	l15 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <3300000>;
> -	};
> -
> -	l16 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <3300000>;
> -	};
> -
> -	l17 {
> -		regulator-min-microvolt = <2850000>;
> -		regulator-max-microvolt = <2850000>;
> -	};
> -
> -	l18 {
> -		regulator-min-microvolt = <2700000>;
> -		regulator-max-microvolt = <2700000>;
> -	};
> -};
> -
>  &msmgpio {
>  	accel_int_default: accel-int-default-state {
>  		pins = "gpio116";
> diff --git a/arch/arm64/boot/dts/qcom/msm8916-longcheer-l8910.dts b/arch/arm64/boot/dts/qcom/msm8916-longcheer-l8910.dts
> index 5ef51d3e9098..d1eb3f2ecf3f 100644
> --- a/arch/arm64/boot/dts/qcom/msm8916-longcheer-l8910.dts
> +++ b/arch/arm64/boot/dts/qcom/msm8916-longcheer-l8910.dts
> @@ -95,6 +95,13 @@ &pm8916_resin {
>  	linux,code = <KEY_VOLUMEDOWN>;
>  };
>  
> +&pm8916_rpm_regulators {
> +	pm8916_l17: l17 {
> +		regulator-min-microvolt = <2850000>;
> +		regulator-max-microvolt = <2850000>;
> +	};
> +};
> +
>  &pm8916_vib {
>  	status = "okay";
>  };
> @@ -134,109 +141,6 @@ &wcnss_iris {
>  	compatible = "qcom,wcn3620";
>  };
>  
> -&smd_rpm_regulators {
> -	vdd_l1_l2_l3-supply = <&pm8916_s3>;
> -	vdd_l4_l5_l6-supply = <&pm8916_s4>;
> -	vdd_l7-supply = <&pm8916_s4>;
> -
> -	s3 {
> -		regulator-min-microvolt = <1250000>;
> -		regulator-max-microvolt = <1350000>;
> -	};
> -
> -	s4 {
> -		regulator-min-microvolt = <1850000>;
> -		regulator-max-microvolt = <2150000>;
> -	};
> -
> -	l1 {
> -		regulator-min-microvolt = <1225000>;
> -		regulator-max-microvolt = <1225000>;
> -	};
> -
> -	l2 {
> -		regulator-min-microvolt = <1200000>;
> -		regulator-max-microvolt = <1200000>;
> -	};
> -
> -	l4 {
> -		regulator-min-microvolt = <2050000>;
> -		regulator-max-microvolt = <2050000>;
> -	};
> -
> -	l5 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <1800000>;
> -	};
> -
> -	l6 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <1800000>;
> -	};
> -
> -	l7 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <1800000>;
> -	};
> -
> -	l8 {
> -		regulator-min-microvolt = <2900000>;
> -		regulator-max-microvolt = <2900000>;
> -	};
> -
> -	l9 {
> -		regulator-min-microvolt = <3300000>;
> -		regulator-max-microvolt = <3300000>;
> -	};
> -
> -	l10 {
> -		regulator-min-microvolt = <2800000>;
> -		regulator-max-microvolt = <2800000>;
> -	};
> -
> -	l11 {
> -		regulator-min-microvolt = <2950000>;
> -		regulator-max-microvolt = <2950000>;
> -		regulator-allow-set-load;
> -		regulator-system-load = <200000>;
> -	};
> -
> -	l12 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <2950000>;
> -	};
> -
> -	l13 {
> -		regulator-min-microvolt = <3075000>;
> -		regulator-max-microvolt = <3075000>;
> -	};
> -
> -	l14 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <3300000>;
> -	};
> -
> -	l15 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <3300000>;
> -	};
> -
> -	l16 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <3300000>;
> -	};
> -
> -	l17 {
> -		regulator-min-microvolt = <2850000>;
> -		regulator-max-microvolt = <2850000>;
> -	};
> -
> -	l18 {
> -		regulator-min-microvolt = <2700000>;
> -		regulator-max-microvolt = <2700000>;
> -	};
> -};
> -
>  &msmgpio {
>  	button_backlight_default: button-backlight-default-state {
>  		pins = "gpio17";
> diff --git a/arch/arm64/boot/dts/qcom/msm8916-pm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916-pm8916.dtsi
> index 29ef46c33350..b38eecbd6253 100644
> --- a/arch/arm64/boot/dts/qcom/msm8916-pm8916.dtsi
> +++ b/arch/arm64/boot/dts/qcom/msm8916-pm8916.dtsi
> @@ -61,30 +61,92 @@ &wcnss_iris {
>  };
>  
>  &rpm_requests {
> -	smd_rpm_regulators: regulators {
> +	pm8916_rpm_regulators: regulators {
>  		compatible = "qcom,rpm-pm8916-regulators";
> +		vdd_l1_l2_l3-supply = <&pm8916_s3>;
> +		vdd_l4_l5_l6-supply = <&pm8916_s4>;
> +		vdd_l7-supply = <&pm8916_s4>;
>  
>  		/* pm8916_s1 is managed by rpmpd (MSM8916_VDDCX) */
> -		pm8916_s3: s3 {};
> -		pm8916_s4: s4 {};
>  
> -		pm8916_l1: l1 {};
> -		pm8916_l2: l2 {};
> +		pm8916_s3: s3 {
> +			regulator-min-microvolt = <1250000>;
> +			regulator-max-microvolt = <1350000>;
> +		};
> +
> +		pm8916_s4: s4 {
> +			regulator-min-microvolt = <1850000>;
> +			regulator-max-microvolt = <2150000>;
> +		};
> +
> +		/*
> +		 * Some of the regulators are unused or managed by another
> +		 * processor (e.g. the modem). We should still define nodes for
> +		 * them to ensure the vote from the application processor can be
> +		 * dropped in case the regulators are already on during boot.
> +		 *
> +		 * The labels for these nodes are omitted on purpose because
> +		 * boards should configure a proper voltage before using them.
> +		 */
> +		l1 {};
> +
> +		pm8916_l2: l2 {
> +			regulator-min-microvolt = <1200000>;
> +			regulator-max-microvolt = <1200000>;
> +		};
> +
>  		/* pm8916_l3 is managed by rpmpd (MSM8916_VDDMX) */
> -		pm8916_l4: l4 {};
> -		pm8916_l5: l5 {};
> -		pm8916_l6: l6 {};
> -		pm8916_l7: l7 {};
> -		pm8916_l8: l8 {};
> -		pm8916_l9: l9 {};
> -		pm8916_l10: l10 {};
> -		pm8916_l11: l11 {};
> -		pm8916_l12: l12 {};
> -		pm8916_l13: l13 {};
> -		pm8916_l14: l14 {};
> -		pm8916_l15: l15 {};
> -		pm8916_l16: l16 {};
> -		pm8916_l17: l17 {};
> -		pm8916_l18: l18 {};
> +
> +		l4 {};
> +
> +		pm8916_l5: l5 {
> +			regulator-min-microvolt = <1800000>;
> +			regulator-max-microvolt = <1800000>;
> +		};
> +
> +		pm8916_l6: l6 {
> +			regulator-min-microvolt = <1800000>;
> +			regulator-max-microvolt = <1800000>;
> +		};
> +
> +		pm8916_l7: l7 {
> +			regulator-min-microvolt = <1800000>;
> +			regulator-max-microvolt = <1800000>;
> +		};
> +
> +		pm8916_l8: l8 {
> +			regulator-min-microvolt = <2900000>;
> +			regulator-max-microvolt = <2900000>;
> +		};
> +
> +		pm8916_l9: l9 {
> +			regulator-min-microvolt = <3300000>;
> +			regulator-max-microvolt = <3300000>;
> +		};
> +
> +		l10 {};
> +
> +		pm8916_l11: l11 {
> +			regulator-min-microvolt = <2950000>;
> +			regulator-max-microvolt = <2950000>;
> +			regulator-allow-set-load;
> +			regulator-system-load = <200000>;
> +		};
> +
> +		pm8916_l12: l12 {
> +			regulator-min-microvolt = <1800000>;
> +			regulator-max-microvolt = <2950000>;
> +		};
> +
> +		pm8916_l13: l13 {
> +			regulator-min-microvolt = <3075000>;
> +			regulator-max-microvolt = <3075000>;
> +		};
> +
> +		l14 {};
> +		l15 {};
> +		l16 {};
> +		l17 {};
> +		l18 {};
>  	};
>  };
> diff --git a/arch/arm64/boot/dts/qcom/msm8916-samsung-a2015-common.dtsi b/arch/arm64/boot/dts/qcom/msm8916-samsung-a2015-common.dtsi
> index b362a76eebc9..37a872949851 100644
> --- a/arch/arm64/boot/dts/qcom/msm8916-samsung-a2015-common.dtsi
> +++ b/arch/arm64/boot/dts/qcom/msm8916-samsung-a2015-common.dtsi
> @@ -252,6 +252,13 @@ &pm8916_resin {
>  	linux,code = <KEY_VOLUMEDOWN>;
>  };
>  
> +&pm8916_rpm_regulators {
> +	pm8916_l17: l17 {
> +		regulator-min-microvolt = <2850000>;
> +		regulator-max-microvolt = <2850000>;
> +	};
> +};
> +
>  &sdhc_1 {
>  	status = "okay";
>  
> @@ -279,109 +286,6 @@ &usb_hs_phy {
>  	extcon = <&muic>;
>  };
>  
> -&smd_rpm_regulators {
> -	vdd_l1_l2_l3-supply = <&pm8916_s3>;
> -	vdd_l4_l5_l6-supply = <&pm8916_s4>;
> -	vdd_l7-supply = <&pm8916_s4>;
> -
> -	s3 {
> -		regulator-min-microvolt = <1250000>;
> -		regulator-max-microvolt = <1350000>;
> -	};
> -
> -	s4 {
> -		regulator-min-microvolt = <1850000>;
> -		regulator-max-microvolt = <2150000>;
> -	};
> -
> -	l1 {
> -		regulator-min-microvolt = <1225000>;
> -		regulator-max-microvolt = <1225000>;
> -	};
> -
> -	l2 {
> -		regulator-min-microvolt = <1200000>;
> -		regulator-max-microvolt = <1200000>;
> -	};
> -
> -	l4 {
> -		regulator-min-microvolt = <2050000>;
> -		regulator-max-microvolt = <2050000>;
> -	};
> -
> -	l5 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <1800000>;
> -	};
> -
> -	l6 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <1800000>;
> -	};
> -
> -	l7 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <1800000>;
> -	};
> -
> -	l8 {
> -		regulator-min-microvolt = <2900000>;
> -		regulator-max-microvolt = <2900000>;
> -	};
> -
> -	l9 {
> -		regulator-min-microvolt = <3300000>;
> -		regulator-max-microvolt = <3300000>;
> -	};
> -
> -	l10 {
> -		regulator-min-microvolt = <2800000>;
> -		regulator-max-microvolt = <2800000>;
> -	};
> -
> -	l11 {
> -		regulator-min-microvolt = <2950000>;
> -		regulator-max-microvolt = <2950000>;
> -		regulator-allow-set-load;
> -		regulator-system-load = <200000>;
> -	};
> -
> -	l12 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <2950000>;
> -	};
> -
> -	l13 {
> -		regulator-min-microvolt = <3075000>;
> -		regulator-max-microvolt = <3075000>;
> -	};
> -
> -	l14 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <3300000>;
> -	};
> -
> -	l15 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <3300000>;
> -	};
> -
> -	l16 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <3300000>;
> -	};
> -
> -	l17 {
> -		regulator-min-microvolt = <2850000>;
> -		regulator-max-microvolt = <2850000>;
> -	};
> -
> -	l18 {
> -		regulator-min-microvolt = <2700000>;
> -		regulator-max-microvolt = <2700000>;
> -	};
> -};
> -
>  &msmgpio {
>  	accel_int_default: accel-int-default-state {
>  		pins = "gpio115";
> diff --git a/arch/arm64/boot/dts/qcom/msm8916-samsung-gt5-common.dtsi b/arch/arm64/boot/dts/qcom/msm8916-samsung-gt5-common.dtsi
> index 4464beeeaab1..a49e1641e59b 100644
> --- a/arch/arm64/boot/dts/qcom/msm8916-samsung-gt5-common.dtsi
> +++ b/arch/arm64/boot/dts/qcom/msm8916-samsung-gt5-common.dtsi
> @@ -120,6 +120,13 @@ &pm8916_resin {
>  	status = "okay";
>  };
>  
> +&pm8916_rpm_regulators {
> +	pm8916_l17: l17 {
> +		regulator-min-microvolt = <2850000>;
> +		regulator-max-microvolt = <2850000>;
> +	};
> +};
> +
>  /* FIXME: Replace with MAX77849 MUIC when driver is available */
>  &pm8916_usbin {
>  	status = "okay";
> @@ -162,109 +169,6 @@ &wcnss_iris {
>  	compatible = "qcom,wcn3660b";
>  };
>  
> -&smd_rpm_regulators {
> -	vdd_l1_l2_l3-supply = <&pm8916_s3>;
> -	vdd_l4_l5_l6-supply = <&pm8916_s4>;
> -	vdd_l7-supply = <&pm8916_s4>;
> -
> -	s3 {
> -		regulator-min-microvolt = <1250000>;
> -		regulator-max-microvolt = <1350000>;
> -	};
> -
> -	s4 {
> -		regulator-min-microvolt = <1850000>;
> -		regulator-max-microvolt = <2150000>;
> -	};
> -
> -	l1 {
> -		regulator-min-microvolt = <1225000>;
> -		regulator-max-microvolt = <1225000>;
> -	};
> -
> -	l2 {
> -		regulator-min-microvolt = <1200000>;
> -		regulator-max-microvolt = <1200000>;
> -	};
> -
> -	l4 {
> -		regulator-min-microvolt = <2050000>;
> -		regulator-max-microvolt = <2050000>;
> -	};
> -
> -	l5 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <1800000>;
> -	};
> -
> -	l6 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <1800000>;
> -	};
> -
> -	l7 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <1800000>;
> -	};
> -
> -	l8 {
> -		regulator-min-microvolt = <2900000>;
> -		regulator-max-microvolt = <2900000>;
> -	};
> -
> -	l9 {
> -		regulator-min-microvolt = <3300000>;
> -		regulator-max-microvolt = <3300000>;
> -	};
> -
> -	l10 {
> -		regulator-min-microvolt = <2800000>;
> -		regulator-max-microvolt = <2800000>;
> -	};
> -
> -	l11 {
> -		regulator-min-microvolt = <2950000>;
> -		regulator-max-microvolt = <2950000>;
> -		regulator-system-load = <200000>;
> -		regulator-allow-set-load;
> -	};
> -
> -	l12 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <2950000>;
> -	};
> -
> -	l13 {
> -		regulator-min-microvolt = <3075000>;
> -		regulator-max-microvolt = <3075000>;
> -	};
> -
> -	l14 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <3300000>;
> -	};
> -
> -	l15 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <3300000>;
> -	};
> -
> -	l16 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <3300000>;
> -	};
> -
> -	l17 {
> -		regulator-min-microvolt = <2850000>;
> -		regulator-max-microvolt = <2850000>;
> -	};
> -
> -	l18 {
> -		regulator-min-microvolt = <2700000>;
> -		regulator-max-microvolt = <2700000>;
> -	};
> -};
> -
>  &msmgpio {
>  	accel_int_default: accel-int-default-state {
>  		pins = "gpio115";
> diff --git a/arch/arm64/boot/dts/qcom/msm8916-samsung-j5-common.dtsi b/arch/arm64/boot/dts/qcom/msm8916-samsung-j5-common.dtsi
> index 6e231e92e675..6192d04a58ae 100644
> --- a/arch/arm64/boot/dts/qcom/msm8916-samsung-j5-common.dtsi
> +++ b/arch/arm64/boot/dts/qcom/msm8916-samsung-j5-common.dtsi
> @@ -128,109 +128,6 @@ &wcnss_iris {
>  	compatible = "qcom,wcn3620";
>  };
>  
> -&smd_rpm_regulators {
> -	vdd_l1_l2_l3-supply = <&pm8916_s3>;
> -	vdd_l4_l5_l6-supply = <&pm8916_s4>;
> -	vdd_l7-supply = <&pm8916_s4>;
> -
> -	s3 {
> -		regulator-min-microvolt = <1250000>;
> -		regulator-max-microvolt = <1350000>;
> -	};
> -
> -	s4 {
> -		regulator-min-microvolt = <1850000>;
> -		regulator-max-microvolt = <2150000>;
> -	};
> -
> -	l1 {
> -		regulator-min-microvolt = <1225000>;
> -		regulator-max-microvolt = <1225000>;
> -	};
> -
> -	l2 {
> -		regulator-min-microvolt = <1200000>;
> -		regulator-max-microvolt = <1200000>;
> -	};
> -
> -	l4 {
> -		regulator-min-microvolt = <2050000>;
> -		regulator-max-microvolt = <2050000>;
> -	};
> -
> -	l5 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <1800000>;
> -	};
> -
> -	l6 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <1800000>;
> -	};
> -
> -	l7 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <1800000>;
> -	};
> -
> -	l8 {
> -		regulator-min-microvolt = <2900000>;
> -		regulator-max-microvolt = <2900000>;
> -	};
> -
> -	l9 {
> -		regulator-min-microvolt = <3300000>;
> -		regulator-max-microvolt = <3300000>;
> -	};
> -
> -	l10 {
> -		regulator-min-microvolt = <2800000>;
> -		regulator-max-microvolt = <2800000>;
> -	};
> -
> -	l11 {
> -		regulator-min-microvolt = <2950000>;
> -		regulator-max-microvolt = <2950000>;
> -		regulator-allow-set-load;
> -		regulator-system-load = <200000>;
> -	};
> -
> -	l12 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <2950000>;
> -	};
> -
> -	l13 {
> -		regulator-min-microvolt = <3075000>;
> -		regulator-max-microvolt = <3075000>;
> -	};
> -
> -	l14 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <3300000>;
> -	};
> -
> -	l15 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <3300000>;
> -	};
> -
> -	l16 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <3300000>;
> -	};
> -
> -	l17 {
> -		regulator-min-microvolt = <3000000>;
> -		regulator-max-microvolt = <3000000>;
> -	};
> -
> -	l18 {
> -		regulator-min-microvolt = <2700000>;
> -		regulator-max-microvolt = <2700000>;
> -	};
> -};
> -
>  &msmgpio {
>  	gpio_hall_sensor_default: gpio-hall-sensor-default-state {
>  		pins = "gpio52";
> diff --git a/arch/arm64/boot/dts/qcom/msm8916-samsung-serranove.dts b/arch/arm64/boot/dts/qcom/msm8916-samsung-serranove.dts
> index fa5b330aaeae..fc4c61c4e1e6 100644
> --- a/arch/arm64/boot/dts/qcom/msm8916-samsung-serranove.dts
> +++ b/arch/arm64/boot/dts/qcom/msm8916-samsung-serranove.dts
> @@ -320,109 +320,6 @@ &wcnss_iris {
>  	compatible = "qcom,wcn3660b";
>  };
>  
> -&smd_rpm_regulators {
> -	vdd_l1_l2_l3-supply = <&pm8916_s3>;
> -	vdd_l4_l5_l6-supply = <&pm8916_s4>;
> -	vdd_l7-supply = <&pm8916_s4>;
> -
> -	s3 {
> -		regulator-min-microvolt = <1250000>;
> -		regulator-max-microvolt = <1350000>;
> -	};
> -
> -	s4 {
> -		regulator-min-microvolt = <1850000>;
> -		regulator-max-microvolt = <2150000>;
> -	};
> -
> -	l1 {
> -		regulator-min-microvolt = <1225000>;
> -		regulator-max-microvolt = <1225000>;
> -	};
> -
> -	l2 {
> -		regulator-min-microvolt = <1200000>;
> -		regulator-max-microvolt = <1200000>;
> -	};
> -
> -	l4 {
> -		regulator-min-microvolt = <2050000>;
> -		regulator-max-microvolt = <2050000>;
> -	};
> -
> -	l5 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <1800000>;
> -	};
> -
> -	l6 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <1800000>;
> -	};
> -
> -	l7 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <1800000>;
> -	};
> -
> -	l8 {
> -		regulator-min-microvolt = <2900000>;
> -		regulator-max-microvolt = <2900000>;
> -	};
> -
> -	l9 {
> -		regulator-min-microvolt = <3300000>;
> -		regulator-max-microvolt = <3300000>;
> -	};
> -
> -	l10 {
> -		regulator-min-microvolt = <2800000>;
> -		regulator-max-microvolt = <2800000>;
> -	};
> -
> -	l11 {
> -		regulator-min-microvolt = <2950000>;
> -		regulator-max-microvolt = <2950000>;
> -		regulator-allow-set-load;
> -		regulator-system-load = <200000>;
> -	};
> -
> -	l12 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <2950000>;
> -	};
> -
> -	l13 {
> -		regulator-min-microvolt = <3075000>;
> -		regulator-max-microvolt = <3075000>;
> -	};
> -
> -	l14 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <3300000>;
> -	};
> -
> -	l15 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <3300000>;
> -	};
> -
> -	l16 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <3300000>;
> -	};
> -
> -	l17 {
> -		regulator-min-microvolt = <2850000>;
> -		regulator-max-microvolt = <2850000>;
> -	};
> -
> -	l18 {
> -		regulator-min-microvolt = <2700000>;
> -		regulator-max-microvolt = <2700000>;
> -	};
> -};
> -
>  &msmgpio {
>  	fg_alert_default: fg-alert-default-state {
>  		pins = "gpio121";
> diff --git a/arch/arm64/boot/dts/qcom/msm8916-ufi.dtsi b/arch/arm64/boot/dts/qcom/msm8916-ufi.dtsi
> index b27896e83a0e..c8ea2f6f6b3d 100644
> --- a/arch/arm64/boot/dts/qcom/msm8916-ufi.dtsi
> +++ b/arch/arm64/boot/dts/qcom/msm8916-ufi.dtsi
> @@ -126,109 +126,6 @@ &wcnss_iris {
>  	compatible = "qcom,wcn3620";
>  };
>  
> -&smd_rpm_regulators {
> -	vdd_l1_l2_l3-supply = <&pm8916_s3>;
> -	vdd_l4_l5_l6-supply = <&pm8916_s4>;
> -	vdd_l7-supply = <&pm8916_s4>;
> -
> -	s3 {
> -		regulator-min-microvolt = <1250000>;
> -		regulator-max-microvolt = <1350000>;
> -	};
> -
> -	s4 {
> -		regulator-min-microvolt = <1850000>;
> -		regulator-max-microvolt = <2150000>;
> -	};
> -
> -	l1 {
> -		regulator-min-microvolt = <1225000>;
> -		regulator-max-microvolt = <1225000>;
> -	};
> -
> -	l2 {
> -		regulator-min-microvolt = <1200000>;
> -		regulator-max-microvolt = <1200000>;
> -	};
> -
> -	l4 {
> -		regulator-min-microvolt = <2050000>;
> -		regulator-max-microvolt = <2050000>;
> -	};
> -
> -	l5 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <1800000>;
> -	};
> -
> -	l6 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <1800000>;
> -	};
> -
> -	l7 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <1800000>;
> -	};
> -
> -	l8 {
> -		regulator-min-microvolt = <2900000>;
> -		regulator-max-microvolt = <2900000>;
> -	};
> -
> -	l9 {
> -		regulator-min-microvolt = <3300000>;
> -		regulator-max-microvolt = <3300000>;
> -	};
> -
> -	l10 {
> -		regulator-min-microvolt = <2800000>;
> -		regulator-max-microvolt = <2800000>;
> -	};
> -
> -	l11 {
> -		regulator-min-microvolt = <2950000>;
> -		regulator-max-microvolt = <2950000>;
> -		regulator-system-load = <200000>;
> -		regulator-allow-set-load;
> -	};
> -
> -	l12 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <2950000>;
> -	};
> -
> -	l13 {
> -		regulator-min-microvolt = <3075000>;
> -		regulator-max-microvolt = <3075000>;
> -	};
> -
> -	l14 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <3300000>;
> -	};
> -
> -	l15 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <3300000>;
> -	};
> -
> -	l16 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <3300000>;
> -	};
> -
> -	l17 {
> -		regulator-min-microvolt = <2850000>;
> -		regulator-max-microvolt = <2850000>;
> -	};
> -
> -	l18 {
> -		regulator-min-microvolt = <2700000>;
> -		regulator-max-microvolt = <2700000>;
> -	};
> -};
> -
>  &msmgpio {
>  	/* pins are board-specific */
>  	button_default: button-default-state {
> diff --git a/arch/arm64/boot/dts/qcom/msm8916-wingtech-wt88047.dts b/arch/arm64/boot/dts/qcom/msm8916-wingtech-wt88047.dts
> index 78020a0db4e4..323590598113 100644
> --- a/arch/arm64/boot/dts/qcom/msm8916-wingtech-wt88047.dts
> +++ b/arch/arm64/boot/dts/qcom/msm8916-wingtech-wt88047.dts
> @@ -149,6 +149,22 @@ &pm8916_resin {
>  	linux,code = <KEY_VOLUMEDOWN>;
>  };
>  
> +&pm8916_rpm_regulators {
> +	pm8916_l16: l16 {
> +		/*
> +		 * L16 is only used for AW2013 which is fine with 2.5-3.3V.
> +		 * Use the recommended typical voltage of 2.8V as minimum.
> +		 */
> +		regulator-min-microvolt = <2800000>;
> +		regulator-max-microvolt = <3300000>;
> +	};
> +
> +	pm8916_l17: l17 {
> +		regulator-min-microvolt = <2850000>;
> +		regulator-max-microvolt = <2850000>;
> +	};
> +};
> +
>  &pm8916_vib {
>  	status = "okay";
>  };
> @@ -188,109 +204,6 @@ &wcnss_iris {
>  	compatible = "qcom,wcn3620";
>  };
>  
> -&smd_rpm_regulators {
> -	vdd_l1_l2_l3-supply = <&pm8916_s3>;
> -	vdd_l4_l5_l6-supply = <&pm8916_s4>;
> -	vdd_l7-supply = <&pm8916_s4>;
> -
> -	s3 {
> -		regulator-min-microvolt = <1250000>;
> -		regulator-max-microvolt = <1350000>;
> -	};
> -
> -	s4 {
> -		regulator-min-microvolt = <1850000>;
> -		regulator-max-microvolt = <2150000>;
> -	};
> -
> -	l1 {
> -		regulator-min-microvolt = <1225000>;
> -		regulator-max-microvolt = <1225000>;
> -	};
> -
> -	l2 {
> -		regulator-min-microvolt = <1200000>;
> -		regulator-max-microvolt = <1200000>;
> -	};
> -
> -	l4 {
> -		regulator-min-microvolt = <2050000>;
> -		regulator-max-microvolt = <2050000>;
> -	};
> -
> -	l5 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <1800000>;
> -	};
> -
> -	l6 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <1800000>;
> -	};
> -
> -	l7 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <1800000>;
> -	};
> -
> -	l8 {
> -		regulator-min-microvolt = <2900000>;
> -		regulator-max-microvolt = <2900000>;
> -	};
> -
> -	l9 {
> -		regulator-min-microvolt = <3300000>;
> -		regulator-max-microvolt = <3300000>;
> -	};
> -
> -	l10 {
> -		regulator-min-microvolt = <2800000>;
> -		regulator-max-microvolt = <2800000>;
> -	};
> -
> -	l11 {
> -		regulator-min-microvolt = <2950000>;
> -		regulator-max-microvolt = <2950000>;
> -		regulator-allow-set-load;
> -		regulator-system-load = <200000>;
> -	};
> -
> -	l12 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <2950000>;
> -	};
> -
> -	l13 {
> -		regulator-min-microvolt = <3075000>;
> -		regulator-max-microvolt = <3075000>;
> -	};
> -
> -	l14 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <3300000>;
> -	};
> -
> -	l15 {
> -		regulator-min-microvolt = <1800000>;
> -		regulator-max-microvolt = <3300000>;
> -	};
> -
> -	l16 {
> -		regulator-min-microvolt = <2800000>;
> -		regulator-max-microvolt = <3300000>;
> -	};
> -
> -	l17 {
> -		regulator-min-microvolt = <2850000>;
> -		regulator-max-microvolt = <2850000>;
> -	};
> -
> -	l18 {
> -		regulator-min-microvolt = <2700000>;
> -		regulator-max-microvolt = <2700000>;
> -	};
> -};
> -
>  &msmgpio {
>  	camera_flash_default: camera-flash-default-state {
>  		pins = "gpio31", "gpio32";
>
Konrad Dybcio May 25, 2023, 11:39 p.m. UTC | #2
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@gerhold.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 {
>
Konrad Dybcio May 26, 2023, 12:28 a.m. UTC | #3
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@gerhold.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 {
>>
Stephan Gerhold May 26, 2023, 6:36 a.m. UTC | #4
On Fri, May 26, 2023 at 02:28:52AM +0200, Konrad Dybcio wrote:
> 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?

At least on MSM8916 there is currently no advantage implementing this.
The "active-only" votes only have the CPU as limited use case. S1 (aka
MSM8916_VDDCX) and L3 (MSM8916_VDDMX) are both used via rpmpd/power
domains which already provides separate active-only variants. L7 (for
the CPU PLL) is the only other regulator used in "active-only" mode.
However, at least on MSM8916 L7 seems to stay always-on no matter what I
do, so having an active-only vote on L7 doesn't provide any advantage.

> 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)

If you only send the "active" votes but no "sleep" votes for a resource
then the RPM firmware treats it as active+sleep, see [1].
The active/sleep separation only starts once a separate sleep vote has
been sent for a resource for the first time.

Therefore, all requests from the SMD regulator driver apply for both
active+sleep at the moment.

[1]: https://git.codelinaro.org/clo/la/kernel/msm-3.10/-/blob/LA.BR.1.2.9.1-02310-8x16.0/drivers/regulator/rpm-smd-regulator.c#L202-204

> > 
> > 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!
> > 

Don't get confused by the similar naming here. RPM sleep votes are
unrelated to the "system suspend" voltages the regulator framework
supports. :)

RPM sleep votes become active if the cpuidle reaches the deepest state
for the (cpu/)cluster(/CCI). This can happen anytime at runtime when the
system is idle long enough. On the other hand, the regulator suspend
voltages are meant to become active during system suspend (where all the
devices get suspended as well).

Since we do have "active-only" support in rpmpd I think the question is
if it is worth bringing the feature also to regulators. Perhaps one
could simply treat all regulators that are needed by the CPU as power
domain.

For example, L7 on MSM8916 is fixed at 1.8V so while it doesn't have
corners the simple enable/disable votes could also be sent via rpmpd.
In some places in downstream L7 is also called VDDPX, similar to
VDDCX and VDDMX which are already in rpmpd.

Thanks,
Stephan
Stephan Gerhold May 26, 2023, 6:47 a.m. UTC | #5
On Fri, May 26, 2023 at 01:35:06AM +0200, Konrad Dybcio wrote:
> On 17.05.2023 20:48, Stephan Gerhold wrote:
> > Right now each MSM8916 device has a huge block of regulator constraints
> > with allowed voltages for each regulator. For lack of better
> > documentation these voltages are often copied as-is from the vendor
> > device tree, without much extra thought.
> > 
> > Unfortunately, the voltages in the vendor device trees are often
> > misleading or even wrong, e.g. because:
> > 
> >  - There is a large voltage range allowed and the actual voltage is
> >    only set somewhere hidden in some messy vendor driver. This is often
> >    the case for pm8916_{l14,l15,l16} because they have a broad range of
> >    1.8-3.3V by default.
> > 
> >  - The voltage is actually wrong but thanks to the voltage constraints
> >    in the RPM firmware it still ends up applying the correct voltage.
> > 
> > To have proper regulator constraints it is important to review them in
> > context of the usage. The current setup in the MSM8916 device trees
> > makes this quite hard because each device duplicates the standard
> > voltages for components of the SoC and mixes those with minor
> > device-specific additions and dummy voltages for completely unused
> > regulators.
> > 
> > The actual usage of the regulators for the SoC components is in
> > msm8916-pm8916.dtsi, so it can and should also define the related
> > voltage constraints. These are not board-specific but defined in the
> > APQ8016E/PM8916 Device Specification. The board DT can then focus on
> > describing the actual board-specific regulators, which makes it much
> > easier to review and spot potential mistakes there.
> > 
> > Note that this commit does not make any functional change. All used
> > regulators still have the same regulator constraints as before. Unused
> > regulators do not have regulator constraints anymore because most of
> > these were too broad or even entirely wrong. They should be added back
> > with proper voltage constraints when there is an actual usage.
> > 
> > Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> > ---
> I'm a bit torn between saying "this is very nice already" and "we should
> probably override each regulator individually" like so:
> 
> &pm8916_l17 {
> 	[...]
> }
> 
> to minimize mistakes..
> 
> Not sure what to make of it, I see Bjorn already applied this, so I guess
> I'm just leaving some potential ideas for the future here.
> 

Sorry, could you elaborate a bit on what changes you would make exactly?

The way it works in this patch is that regulators that are used by the
SoC are defined in msm8916-pm8916.dtsi. All other (board-specific)
regulators must be defined together with proper voltages in the board DT.

What kind of mistake are you thinking of?

Thanks,
Stephan
Konrad Dybcio May 26, 2023, 8:50 a.m. UTC | #6
On 26.05.2023 08:36, Stephan Gerhold wrote:
> On Fri, May 26, 2023 at 02:28:52AM +0200, Konrad Dybcio wrote:
>> 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?
> 
> At least on MSM8916 there is currently no advantage implementing this.
> The "active-only" votes only have the CPU as limited use case. S1 (aka
> MSM8916_VDDCX) and L3 (MSM8916_VDDMX) are both used via rpmpd/power
> domains which already provides separate active-only variants. L7 (for
> the CPU PLL) is the only other regulator used in "active-only" mode.
> However, at least on MSM8916 L7 seems to stay always-on no matter what I
> do, so having an active-only vote on L7 doesn't provide any advantage.
In this case it may be more important that we tell RPM that we want it
to be active-only, even if it ultimately makes a different decision.
You probably played with this more, but my guess would be that not letting
off of an a-s vote could confuse the algos

> 
>> 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)
> 
> If you only send the "active" votes but no "sleep" votes for a resource
> then the RPM firmware treats it as active+sleep, see [1].
> The active/sleep separation only starts once a separate sleep vote has
> been sent for a resource for the first time.
> 
> Therefore, all requests from the SMD regulator driver apply for both
> active+sleep at the moment.
> 
> [1]: https://git.codelinaro.org/clo/la/kernel/msm-3.10/-/blob/LA.BR.1.2.9.1-02310-8x16.0/drivers/regulator/rpm-smd-regulator.c#L202-204
/me *dies*

that's a design decision if i've ever seen one..

> 
>>>
>>> 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!
>>>
> 
> Don't get confused by the similar naming here. RPM sleep votes are
> unrelated to the "system suspend" voltages the regulator framework
> supports. :)
> 
> RPM sleep votes become active if the cpuidle reaches the deepest state
> for the (cpu/)cluster(/CCI). This can happen anytime at runtime when the
> system is idle long enough. On the other hand, the regulator suspend
> voltages are meant to become active during system suspend (where all the
> devices get suspended as well).
Yes and pm_genpd tracks that very meticulously, at least in the case of PSCI.

> 
> Since we do have "active-only" support in rpmpd I think the question is
> if it is worth bringing the feature also to regulators. Perhaps one
> could simply treat all regulators that are needed by the CPU as power
> domain.
That would make sense..

> 
> For example, L7 on MSM8916 is fixed at 1.8V so while it doesn't have
> corners the simple enable/disable votes could also be sent via rpmpd.
> In some places in downstream L7 is also called VDDPX, similar to
> VDDCX and VDDMX which are already in rpmpd.
Yeah, anything available from RPM is only vaguely categorized as being
a clock/regulator/bus, sometimes wrongly (see: bus clocks in rpmcc) so
there's some flexibility here.

Konrad
> 
> Thanks,
> Stephan
Stephan Gerhold May 26, 2023, 12:55 p.m. UTC | #7
On Fri, May 26, 2023 at 10:50:53AM +0200, Konrad Dybcio wrote:
> On 26.05.2023 08:36, Stephan Gerhold wrote:
> > On Fri, May 26, 2023 at 02:28:52AM +0200, Konrad Dybcio wrote:
> >> 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?
> > 
> > At least on MSM8916 there is currently no advantage implementing this.
> > The "active-only" votes only have the CPU as limited use case. S1 (aka
> > MSM8916_VDDCX) and L3 (MSM8916_VDDMX) are both used via rpmpd/power
> > domains which already provides separate active-only variants. L7 (for
> > the CPU PLL) is the only other regulator used in "active-only" mode.
> > However, at least on MSM8916 L7 seems to stay always-on no matter what I
> > do, so having an active-only vote on L7 doesn't provide any advantage.
> In this case it may be more important that we tell RPM that we want it
> to be active-only, even if it ultimately makes a different decision.
> You probably played with this more, but my guess would be that not letting
> off of an a-s vote could confuse the algos
> 

I think in this case it does not make any difference. There is no
difference to downstream for the power consumption during VMIN suspend
(with these changes and my hack patches). In fact the power consumption
is so ridiculously low (about 0.008W / 0.096 A @ 12V) that my
measurement thing can barely measure it. :D

There are definitely more important things to work on right now that
will make a much larger difference. Perhaps one day when we have the
important things like cpuidle, bus scaling/interconnect etc we can look
again at this tiny little regulator that probably will never turn off
anyway. :D

> > 
> >> 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)
> > 
> > If you only send the "active" votes but no "sleep" votes for a resource
> > then the RPM firmware treats it as active+sleep, see [1].
> > The active/sleep separation only starts once a separate sleep vote has
> > been sent for a resource for the first time.
> > 
> > Therefore, all requests from the SMD regulator driver apply for both
> > active+sleep at the moment.
> > 
> > [1]: https://git.codelinaro.org/clo/la/kernel/msm-3.10/-/blob/LA.BR.1.2.9.1-02310-8x16.0/drivers/regulator/rpm-smd-regulator.c#L202-204
> /me *dies*
> 
> that's a design decision if i've ever seen one..
> 

:D

> > 
> >>>
> >>> 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!
> >>>
> > 
> > Don't get confused by the similar naming here. RPM sleep votes are
> > unrelated to the "system suspend" voltages the regulator framework
> > supports. :)
> > 
> > RPM sleep votes become active if the cpuidle reaches the deepest state
> > for the (cpu/)cluster(/CCI). This can happen anytime at runtime when the
> > system is idle long enough. On the other hand, the regulator suspend
> > voltages are meant to become active during system suspend (where all the
> > devices get suspended as well).
> Yes and pm_genpd tracks that very meticulously, at least in the case of PSCI.

Meh, having a proper PSCI implementation is luxury! I have to mess with
the good old way of poking the SPM/SAWs from Linux... :P

Thanks,
Stephan
Bryan O'Donoghue May 26, 2023, 1:38 p.m. UTC | #8
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 ?

>   	};
>   
>   	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 ?

---
bod
Stephan Gerhold May 26, 2023, 2:03 p.m. UTC | #9
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
Bryan O'Donoghue May 26, 2023, 3:42 p.m. UTC | #10
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>

Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Bryan O'Donoghue May 26, 2023, 3:43 p.m. UTC | #11
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(-)

Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Konrad Dybcio May 26, 2023, 9:11 p.m. UTC | #12
On 26.05.2023 08:47, Stephan Gerhold wrote:
> On Fri, May 26, 2023 at 01:35:06AM +0200, Konrad Dybcio wrote:
>> On 17.05.2023 20:48, Stephan Gerhold wrote:
>>> Right now each MSM8916 device has a huge block of regulator constraints
>>> with allowed voltages for each regulator. For lack of better
>>> documentation these voltages are often copied as-is from the vendor
>>> device tree, without much extra thought.
>>>
>>> Unfortunately, the voltages in the vendor device trees are often
>>> misleading or even wrong, e.g. because:
>>>
>>>  - There is a large voltage range allowed and the actual voltage is
>>>    only set somewhere hidden in some messy vendor driver. This is often
>>>    the case for pm8916_{l14,l15,l16} because they have a broad range of
>>>    1.8-3.3V by default.
>>>
>>>  - The voltage is actually wrong but thanks to the voltage constraints
>>>    in the RPM firmware it still ends up applying the correct voltage.
>>>
>>> To have proper regulator constraints it is important to review them in
>>> context of the usage. The current setup in the MSM8916 device trees
>>> makes this quite hard because each device duplicates the standard
>>> voltages for components of the SoC and mixes those with minor
>>> device-specific additions and dummy voltages for completely unused
>>> regulators.
>>>
>>> The actual usage of the regulators for the SoC components is in
>>> msm8916-pm8916.dtsi, so it can and should also define the related
>>> voltage constraints. These are not board-specific but defined in the
>>> APQ8016E/PM8916 Device Specification. The board DT can then focus on
>>> describing the actual board-specific regulators, which makes it much
>>> easier to review and spot potential mistakes there.
>>>
>>> Note that this commit does not make any functional change. All used
>>> regulators still have the same regulator constraints as before. Unused
>>> regulators do not have regulator constraints anymore because most of
>>> these were too broad or even entirely wrong. They should be added back
>>> with proper voltage constraints when there is an actual usage.
>>>
>>> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
>>> ---
>> I'm a bit torn between saying "this is very nice already" and "we should
>> probably override each regulator individually" like so:
>>
>> &pm8916_l17 {
>> 	[...]
>> }
>>
>> to minimize mistakes..
>>
>> Not sure what to make of it, I see Bjorn already applied this, so I guess
>> I'm just leaving some potential ideas for the future here.
>>
> 
> Sorry, could you elaborate a bit on what changes you would make exactly?
Assigning the voltage ranges through direct reference to each individual
regulator, instead of overwriting them through referencing the
pm8916_rpm_regulators label and (essentially) redefining them.

> 
> The way it works in this patch is that regulators that are used by the
> SoC are defined in msm8916-pm8916.dtsi. All other (board-specific)
> regulators must be defined together with proper voltages in the board DT.
> 
> What kind of mistake are you thinking of?
Fat fingers, mostly

So suppose your device needs a different voltage on L18, so you do

&pm8916_rpm_regulators {
	l19 { //fat fingers burn devices
		regulator-min-microvolt = <12341234>;
		regulator-max-microvolt = <43143144>;
	};
};

DTC will happily eat that


since we use labels, one would have to fatfinger twice, like so:
&pm8916_rpm_regulators {
	pm8916_l19: l19 { //this was still supposed to be l18
...


as these two combinations will trigger a build error


&pm8916_rpm_regulators {
	pm8916_l19: l18 { //duplicate label vs actual l19

---

&pm8916_rpm_regulators {
	pm8916_l18: l19 { //duplicate label vs actual l18


Konrad

> 
> Thanks,
> Stephan