| Message ID | 20230510-msm8916-regulators-v1-8-54d4960a05fc@gerhold.net (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | arm64: dts: qcom: msm8916: Rework regulator constraints | expand |
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 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 { >>
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
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
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 {
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). 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(-)