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

[Konrad Dybcio]

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