| Message ID | 20230510-msm8916-regulators-v1-0-54d4960a05fc@gerhold.net |
|---|---|
| Headers | show |
| Series | arm64: dts: qcom: msm8916: Rework regulator constraints | expand |
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"; >
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 { >
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
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
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>
On Fri, May 26, 2023 at 11:11:44PM +0200, Konrad Dybcio wrote: > 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 > Yeah I was also a bit torn between (pre-)defining labels for all regulators vs the approach I chose in this patch. However, thinking about it some more I realized that having the pre-defined labels makes it far too easy to make another fairly hidden mistake: Let's assume we would define labels for all regulators in msm8916-pm8916.dtsi. That is, even for board-specific regulators we add: /* ... */ pm8916_l10: l10 {}; /* ... */ pm8916_l14: l14 {}; pm8916:l15: l15 {}; pm8916_l16: l16 {}; /* ... */ Now someone new to device porting thinks "c'mon, gettin' camera workin' can't be *that* hard". They look at downstream and quickly define a device tree node with the necessary regulators: camera@1a { compatible = "sony,imx214"; reg = <0x1a>; vddo-supply = <&pm8916_l10>; vdda-supply = <&pm8916_l16>; vddd-supply = <&pm8916_l2>; /* ... */ }; They build this successfully and try to run it. Perhaps it even works somewhat but it's quite dangerous: They did not define any voltage constraints for l10 and l16! In this case, the regulator core does not allow *changing* the regulator voltage, but it still allows *enabling*/disabling the regulator with "who knows what RPM uses as default voltage". They might even submit it upstream and reviewers assume the voltages are already defined somewhere. This cannot happen with my patch. The labels for the board-specific regulators are not defined anywhere, so they would immediately get a build error. They would probably look for examples how to define the additional regulators. For pm8916_l16 they find examples like: &pm8916_rpm_regulators { pm8916_l16: l16 { regulator-min-microvolt = <1800000>; regulator-max-microvolt = <1800000>; }; }; They copy this into their own board DT but (hopefully) wonder "Is this actually the correct voltage for my case?". They look closer at the downstream camera setup and see that vdda/l16 actually needs 2.7V. For each additional regulator they need to actively add something and think about the changes before it will build successfully. I believe that in this case being unaware of additional required changes is far more likely than making "fat finger" mistakes. Regulators is simply something one needs to be careful about. Even if we add fail-safes for regulator name typos, you could just as easily have typos in the specified voltages. I always look twice before testing regulator changes and would hope this applies to most people. :) Thanks, Stephan
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,