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
