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

[Stephan Gerhold <stephan@gerhold.net>]

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