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

[Stephan Gerhold]

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