[RFC] regulator: core: Allow for a base-load per client to come from dts

Looking through the current users of regulator_set_load(), all but one
or two basically follow the pattern:

1. Get all the regulators.
2. Go through each regulator and call regulator_set_load() on it with
   some value that's sufficient to get the regulator to run in "HPM"
   mode.
3. Enable / disable the regulator as needed.

Specifically it should be noted that many devices don't really have a
need for the low power mode of regulators. Unfortunately the current
state of the world means that all drivers are cluttered with tables of
loads and extra cruft at init time to tell the regulator framework
about said loads.

Let's add a way to specify the "base load" of a regulator in the
device tree and hopefully make this a bit better.

There are lots of ways we could combine this "base load" with a load
that the driver requests. We could:
- Add them.
- Take the maximum (assume that the "base" is the min).
- Only use the "base" load if the driver didn't request one.

I have chosen the third option here. If the driver knows better then
it can override. Note that the driver can't override to "0". To do
that it would just disable the regulator.

The choice above means that we can effectively treat the "base" load
as the "normal usage" load. It's expected that with this load set we
could use the device normally. If a driver could go into a low power
state then it would only need to set a load when in that low power
state and it could effectively set its requested load back to 0
afterwards and the system would bounce back to the base
load. Presumably someone could also set the load higher than the base
before putting a device in "turbo mode".

This property is specified as a separate property from the supply
because it's expected that the load for a peripheral could be
specified in a SoC's dtsi file. As an example:

SoC.dtsi:
  nifty_phy: phy@1234 {
    vdda-phy-base-load = <21800>;
  };

Board.dts:
  &nifty_phy {
    vdda-phy-supply = <&pp1800_l4a>;
  };

NOTE: if we want to keep old binary dtb files working with new kernels
then we'd still have to keep existing crufty regulator_set_load() in
drivers, but hopefully we can stop adding new instances of this, and
perhaps eventually people will agree to deprecate old binary dtb files
and we can get rid of all the extra code.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
I have only done incredibly light testing with this but I wanted to
get the idea out there. We'd obviously need to get device-tree folks
on board with this and adjust the dt-schema for it.

 drivers/regulator/core.c     | 65 ++++++++++++++++++++++++++++--------
 drivers/regulator/internal.h |  1 +
 2 files changed, 53 insertions(+), 13 deletions(-)

Hi,

On Sat, Jul 23, 2022 at 12:11 PM Mark Brown <broonie@kernel.org> wrote:
>
> I'm familiar with the broad concept of regulators having the
> ability to operate in more efficient (but typically lower
> quality) modes, we do have the whole infrastructure for selecting
> mode based on load after all.  I'm not sure what you mean by
> devices having a *need* for it though?

All I meant by *need* was just whether it was practical for a device
to actively take advantage of the low power mode. A driver that
"needs" to know about loads is one that purposely puts the device into
a low power state so that it can re-configure the regulator. I was
arguing that most drivers don't need to do this.


> Does this need actually exist or is this just a we built it so we
> must use it thing?  There's a lot of power microoptimisation that
> goes on and sometimes it's hard to tell how much power is saved
> relative to the power consumed managing the feature.

It has always felt like a microoptimizatoin to me. I would love to see
evidence to the contrary, though.

Specifically we don't seem to use low power mode (LPM) anywhere on the
sc7180 trogdor laptops today. Maybe it would be possible to use it in
some cases, but nobody ever did the analysis. I haven't spent much
time analyzing downstream Qualcomm solutions, though.

I guess the answer is that if it's a micro-optimization then we should
be ripping more of this code out. ;-) That would go contrary to
Dmitry's request that all regulators have a load set on them...


> To the extent this is needed it does smell a bit like "these
> regulators should default to setting their load to 0 when
> disabled" or something more along those lines TBH, some of your
> previous comments suggested that the per device loads being
> specified in a lot of the driver are just random values intended
> to trigger a specific behaviour in the regulator but forced to be
> done in terms of a load due to the firmware inteface that's
> available on these platforms having an interface in those terms.

It's actually _not_ the firmware interface as far as I can tell, at
least for newer Qualcomm chips (those using RPMH). The firmware
interface seems to be for modes. See, for instance,
rpmh_regulator_vrm_set_load() which translates loads into modes before
passing to the firmware. Ironically, my memory tells me that Qualcomm
actually said that this turned out to be a problem in the past for
them, though, since some rails went to both the main apps processor
(AP) and the modem processor. Each could independently decide that low
power mode was fine but the total of both usages could bump you into
needing high power mode...

As far as whether the numbers are random values or mean something: I
don't know that. I'd personally have a bit of a hard time trusting
them, though. Mostly ending up at LPM when you need HPM seems like it
could be really hard to debug.

One point of evidence of these numbers being pointless and just noise
is code that seems to have made its way upstream to set a "disable
load". This hasn't done anything since 2018 when we landed commit
5451781dadf8 ("regulator: core: Only count load for enabled
consumers") but is still instructive of Qualcomm's thinking. Taking a
sampling of the loads in the tables in the DSI driver / phy, I see:
* Many specify 100 uA.
* Some seem to pick based on throwing a dart at a dartboard. 16 uA, 2
uA, 4 uA, 32 uA, etc.

I can't imagine any of these numbers having done anything useful even
prior to the 2018 commit. From `qcom-rpmh-regulator.c` the changeover
from LPM to HPM is at 10000 uA or 30000 uA, so even a load of 100 uA
is really just a rounding error. Yet despite their uselessness,
Engineers at Qualcomm have dutifully filled in all these numbers...

Looking at the loads passed to "active" devices, I see almost nothing
at all specifying a load that's less than 10mA. That means it's gonna
be really hard to use any of that class of regulators in LPM.

If we happen to be using an LDO that changes over at 30 mA, though,
these ones _could_ use LDO. I guess this is where the whole
"specifying in uA" makes sense? If you've got a regulator that changes
at 30mA and only one ~20mA consumer is active then it can stay in LPM.
When two ~20mA consumers are active then it needs to switch to HPM?
Having lots of consumers on a given rail is really common w/ Qulaocmm
setups. On trogdor, rail "L4A" is all of these:

vdd_qlink_lv:
vdd_qlink_lv_ck:
vdd_qusb_hs0_core:
vdd_ufs1_core:
vdda_mipi_csi0_0p9:
vdda_mipi_csi1_0p9:
vdda_mipi_csi2_0p9:
vdda_mipi_csi3_0p9:
vdda_mipi_dsi0_pll:
vdda_pll_cc_ebi01:
vdda_qrefs_0p9:
vdda_usb_ss_dp_core:

...and that's a regulator that can go up to 30mA in LDO mode... Of
course "MIPI DSI", by complete chance I'm sure, says its load is
_just_ over the 30 mA threshold...


> It didn't sound like they were actual specified/measured physical
> values for the on-SoC stuff, some of the panel drivers do look
> like they have plausuble numbers from datasheets though.

I just don't know. :(


> It might even make sense to have the regulator drivers implement
> the mode interface, that's possibly more useful to work with here
> even if it's not what the interfaces say, it does feel like
> practicaly how people are working with this stuff.  There's
> obviously issues there if anyone *does* work usefully with loads
> and how that integrates, though I think in this day and age the
> need for active management outside of super idle states is
> typically minimal.  As a first pass you could just disable the
> DRMS stuff if mode setting permission is enabled, I suspect
> that'd work fine in these cases.  Or just model the modes as a
> vote for "add X to the load" if they're set.

The current RPMH driver _does_ implement the mode interface. ;-)


I guess the above doesn't really give us a lot of good answers. :(

I guess the problem is that, like a lot of Qualcomm stuff, I still
don't have a good big picture despite having been working on Qualcomm
SoCs for years now. I do know that code keeps showing up to set
regulator loads all over the tree and, I presume, most maintainers
just take the code without questioning the need to set the load at
all.

Perhaps the low hanging fruit is to just accept that the current API
of setting the load is here to stay, even if it does seem mostly
pointless in many cases. I can submit a patch that adds the load to
the "bulk" API and at least it would clean up a bunch of stuff even if
it doesn't fundamentally overhaul the system...


-Doug

On Mon, Jul 25, 2022 at 05:31:34PM -0700, Doug Anderson wrote:
> On Sat, Jul 23, 2022 at 12:11 PM Mark Brown <broonie@kernel.org> wrote:

> I guess the answer is that if it's a micro-optimization then we should
> be ripping more of this code out. ;-) That would go contrary to
> Dmitry's request that all regulators have a load set on them...

I've not seen this request.

> > To the extent this is needed it does smell a bit like "these
> > regulators should default to setting their load to 0 when
> > disabled" or something more along those lines TBH, some of your
> > previous comments suggested that the per device loads being
> > specified in a lot of the driver are just random values intended
> > to trigger a specific behaviour in the regulator but forced to be
> > done in terms of a load due to the firmware inteface that's
> > available on these platforms having an interface in those terms.

> It's actually _not_ the firmware interface as far as I can tell, at
> least for newer Qualcomm chips (those using RPMH). The firmware
> interface seems to be for modes. See, for instance,
> rpmh_regulator_vrm_set_load() which translates loads into modes before
> passing to the firmware. Ironically, my memory tells me that Qualcomm
> actually said that this turned out to be a problem in the past for
> them, though, since some rails went to both the main apps processor
> (AP) and the modem processor. Each could independently decide that low
> power mode was fine but the total of both usages could bump you into
> needing high power mode...

Oh, that's just completely broken.  The set_load() support should be
removed and the drivers should be implementing get_optimum_mode(), it's
actually got an open coded version of it in there already.  That'll only
have snuck past due to being hidden behind a layer of abstraction that
shouldn't be there.

> consumers") but is still instructive of Qualcomm's thinking. Taking a
> sampling of the loads in the tables in the DSI driver / phy, I see:
> * Many specify 100 uA.
> * Some seem to pick based on throwing a dart at a dartboard. 16 uA, 2
> uA, 4 uA, 32 uA, etc.

The load for a PHY is going to be immaterial when you're driving a
panel.

> If we happen to be using an LDO that changes over at 30 mA, though,
> these ones _could_ use LDO. I guess this is where the whole
> "specifying in uA" makes sense? If you've got a regulator that changes
> at 30mA and only one ~20mA consumer is active then it can stay in LPM.
> When two ~20mA consumers are active then it needs to switch to HPM?
> Having lots of consumers on a given rail is really common w/ Qulaocmm
> setups. On trogdor, rail "L4A" is all of these:

We specify in uA and uV partly to ensure we never run into the bottom
limit of resolution and partly because for loads uA is a very relevant
number, as you've noticed it's where things tend to transition into
their lowest power modes and it's also commonly a relevant unit for the
draw from devices in idle modes.  Something like an audio CODEC doing
jack detection will probably be able to get into a low power mode for
example.

> I guess the above doesn't really give us a lot of good answers. :(

All the load setting that doesn't get passed to the firmware in the
regulator drivers should be removed and replaced with get_optimum_mode()
operations, this has no impact on the client drivers and removes some
open coding from the regulator drivers.  Load management in client
drivers that doesn't go below say 100uA is probably noise but it's also
fairly harmless.

> Perhaps the low hanging fruit is to just accept that the current API
> of setting the load is here to stay, even if it does seem mostly
> pointless in many cases. I can submit a patch that adds the load to
> the "bulk" API and at least it would clean up a bunch of stuff even if
> it doesn't fundamentally overhaul the system...

Adding loads to the bulk API would also be useful.