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

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

[Douglas Anderson]

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(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 1e54a833f2cf..cc817f5efc77 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -94,7 +94,8 @@ static int regulator_balance_voltage(struct regulator_dev *rdev,
 				     suspend_state_t state);
 static struct regulator *create_regulator(struct regulator_dev *rdev,
 					  struct device *dev,
-					  const char *supply_name);
+					  const char *supply_name,
+					  int uA_base_load);
 static void destroy_regulator(struct regulator *regulator);
 static void _regulator_put(struct regulator *regulator);
 
@@ -417,6 +418,29 @@ static struct device_node *of_get_regulator(struct device *dev, const char *supp
 	return regnode;
 }
 
+static int of_get_regulator_base_load(struct device *dev, const char *supply)
+{
+	char prop_name[64]; /* 64 is max size of property name */
+	u32 pval = 0;
+
+	snprintf(prop_name, 64, "%s-base-load", supply);
+	of_property_read_u32(dev->of_node, prop_name, &pval);
+
+	dev_dbg(dev, "Looking up %s-base-load from device tree: %d\n",
+		supply, (int)pval);
+
+	return pval;
+}
+
+static int regulator_eff_load(const struct regulator *regulator)
+{
+	/*
+	 * If a load has been specified then use that; otherwise fall back
+	 * to the base load.
+	 */
+	return regulator->uA_load ? regulator->uA_load : regulator->uA_base_load;
+}
+
 /* Platform voltage constraint check */
 int regulator_check_voltage(struct regulator_dev *rdev,
 			    int *min_uV, int *max_uV)
@@ -774,7 +798,7 @@ static ssize_t requested_microamps_show(struct device *dev,
 	regulator_lock(rdev);
 	list_for_each_entry(regulator, &rdev->consumer_list, list) {
 		if (regulator->enable_count)
-			uA += regulator->uA_load;
+			uA += regulator_eff_load(regulator);
 	}
 	regulator_unlock(rdev);
 	return sprintf(buf, "%d\n", uA);
@@ -965,7 +989,7 @@ static int drms_uA_update(struct regulator_dev *rdev)
 	/* calc total requested load */
 	list_for_each_entry(sibling, &rdev->consumer_list, list) {
 		if (sibling->enable_count)
-			current_uA += sibling->uA_load;
+			current_uA += regulator_eff_load(sibling);
 	}
 
 	current_uA += rdev->constraints->system_load;
@@ -1604,13 +1628,14 @@ static int set_machine_constraints(struct regulator_dev *rdev)
  * set_supply - set regulator supply regulator
  * @rdev: regulator name
  * @supply_rdev: supply regulator name
+ * @uA_base_load: default load to apply when we need the supply
  *
  * Called by platform initialisation code to set the supply regulator for this
  * regulator. This ensures that a regulators supply will also be enabled by the
  * core if it's child is enabled.
  */
 static int set_supply(struct regulator_dev *rdev,
-		      struct regulator_dev *supply_rdev)
+		      struct regulator_dev *supply_rdev, int uA_base_load)
 {
 	int err;
 
@@ -1619,7 +1644,8 @@ static int set_supply(struct regulator_dev *rdev,
 	if (!try_module_get(supply_rdev->owner))
 		return -ENODEV;
 
-	rdev->supply = create_regulator(supply_rdev, &rdev->dev, "SUPPLY");
+	rdev->supply = create_regulator(supply_rdev, &rdev->dev, "SUPPLY",
+					uA_base_load);
 	if (rdev->supply == NULL) {
 		err = -ENOMEM;
 		return err;
@@ -1769,7 +1795,8 @@ static const struct file_operations constraint_flags_fops = {
 
 static struct regulator *create_regulator(struct regulator_dev *rdev,
 					  struct device *dev,
-					  const char *supply_name)
+					  const char *supply_name,
+					  int uA_base_load)
 {
 	struct regulator *regulator;
 	int err = 0;
@@ -1800,6 +1827,7 @@ static struct regulator *create_regulator(struct regulator_dev *rdev,
 
 	regulator->rdev = rdev;
 	regulator->supply_name = supply_name;
+	regulator->uA_base_load = uA_base_load;
 
 	regulator_lock(rdev);
 	list_add(&regulator->list, &rdev->consumer_list);
@@ -1825,6 +1853,8 @@ static struct regulator *create_regulator(struct regulator_dev *rdev,
 	} else {
 		debugfs_create_u32("uA_load", 0444, regulator->debugfs,
 				   &regulator->uA_load);
+		debugfs_create_u32("uA_base_load", 0444, regulator->debugfs,
+				   &regulator->uA_base_load);
 		debugfs_create_u32("min_uV", 0444, regulator->debugfs,
 				   &regulator->voltage[PM_SUSPEND_ON].min_uV);
 		debugfs_create_u32("max_uV", 0444, regulator->debugfs,
@@ -1968,6 +1998,7 @@ static int regulator_resolve_supply(struct regulator_dev *rdev)
 {
 	struct regulator_dev *r;
 	struct device *dev = rdev->dev.parent;
+	int uA_base_load;
 	int ret = 0;
 
 	/* No supply to resolve? */
@@ -1997,6 +2028,8 @@ static int regulator_resolve_supply(struct regulator_dev *rdev)
 		}
 	}
 
+	uA_base_load = of_get_regulator_base_load(dev, rdev->supply_name);
+
 	if (r == rdev) {
 		dev_err(dev, "Supply for %s (%s) resolved to itself\n",
 			rdev->desc->name, rdev->supply_name);
@@ -2043,7 +2076,7 @@ static int regulator_resolve_supply(struct regulator_dev *rdev)
 		goto out;
 	}
 
-	ret = set_supply(rdev, r);
+	ret = set_supply(rdev, r, uA_base_load);
 	if (ret < 0) {
 		regulator_unlock(rdev);
 		put_device(&r->dev);
@@ -2077,6 +2110,7 @@ struct regulator *_regulator_get(struct device *dev, const char *id,
 	struct regulator_dev *rdev;
 	struct regulator *regulator;
 	struct device_link *link;
+	int uA_base_load;
 	int ret;
 
 	if (get_type >= MAX_GET_TYPE) {
@@ -2128,6 +2162,8 @@ struct regulator *_regulator_get(struct device *dev, const char *id,
 		}
 	}
 
+	uA_base_load = of_get_regulator_base_load(dev, id);
+
 	if (rdev->exclusive) {
 		regulator = ERR_PTR(-EPERM);
 		put_device(&rdev->dev);
@@ -2163,7 +2199,7 @@ struct regulator *_regulator_get(struct device *dev, const char *id,
 		return regulator;
 	}
 
-	regulator = create_regulator(rdev, dev, id);
+	regulator = create_regulator(rdev, dev, id, uA_base_load);
 	if (regulator == NULL) {
 		regulator = ERR_PTR(-ENOMEM);
 		module_put(rdev->owner);
@@ -2737,7 +2773,8 @@ static int _regulator_handle_consumer_enable(struct regulator *regulator)
 	lockdep_assert_held_once(&rdev->mutex.base);
 
 	regulator->enable_count++;
-	if (regulator->uA_load && regulator->enable_count == 1)
+	if ((regulator->uA_load || regulator->uA_base_load) &&
+	    regulator->enable_count == 1)
 		return drms_uA_update(rdev);
 
 	return 0;
@@ -2763,7 +2800,8 @@ static int _regulator_handle_consumer_disable(struct regulator *regulator)
 	}
 
 	regulator->enable_count--;
-	if (regulator->uA_load && regulator->enable_count == 0)
+	if ((regulator->uA_load || regulator->uA_base_load) &&
+	    regulator->enable_count == 0)
 		return drms_uA_update(rdev);
 
 	return 0;
@@ -5850,6 +5888,7 @@ static void regulator_summary_show_subtree(struct seq_file *s,
 	struct regulator *consumer;
 	struct summary_data summary_data;
 	unsigned int opmode;
+	int uA_load;
 
 	if (!rdev)
 		return;
@@ -5893,11 +5932,11 @@ static void regulator_summary_show_subtree(struct seq_file *s,
 
 		switch (rdev->desc->type) {
 		case REGULATOR_VOLTAGE:
+			uA_load = regulator_eff_load(consumer);
 			seq_printf(s, "%3d %33dmA%c%5dmV %5dmV",
 				   consumer->enable_count,
-				   consumer->uA_load / 1000,
-				   consumer->uA_load && !consumer->enable_count ?
-				   '*' : ' ',
+				   uA_load / 1000,
+				   uA_load && !consumer->enable_count ? '*' : ' ',
 				   consumer->voltage[PM_SUSPEND_ON].min_uV / 1000,
 				   consumer->voltage[PM_SUSPEND_ON].max_uV / 1000);
 			break;
diff --git a/drivers/regulator/internal.h b/drivers/regulator/internal.h
index 1e9c71642143..8e20808bfbba 100644
--- a/drivers/regulator/internal.h
+++ b/drivers/regulator/internal.h
@@ -49,6 +49,7 @@ struct regulator {
 	unsigned int bypass:1;
 	unsigned int device_link:1;
 	int uA_load;
+	int uA_base_load;
 	unsigned int enable_count;
 	unsigned int deferred_disables;
 	struct regulator_voltage voltage[REGULATOR_STATES_NUM];
-- 
2.37.1.359.gd136c6c3e2-goog

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 5016 bytes --]

On Thu, Jul 21, 2022 at 06:26:44PM -0700, Douglas Anderson wrote:

> 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.

Sure...  I guess these devices are just hoping that their idle currents
are near enough to zero to be safe.  Having refreshed my mind about how
all this works that's basically the assumption that Linux is making
here, that any idle currents will be low enough to be satisified by the
lowest power state of any practical regulator and we don't need to keep
track of them.

> Specifically it should be noted that many devices don't really have a
> need for the low power mode of regulators. Unfortunately the current

What exactly do you mean by "a need for the low power mode of
regulators"?

> 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.

We're only talking about a few lines of code here, and we could make a
bulk helper to set loads if this is a serious issue.  You could even
just add some more values in the bulk struct and have it be pure data
for the drivers, regulator_bulk_get() could see a load and set it.  I
think this may actually be the bulk of what's concerning you here?

> 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.

This feels a bit mangled, I think largely because you're using the term
"base load" to refer to something which I *think* you want to be the
peak load the device might cause when it's actively doing stuff which is
just very confusing.  If this were being specified per device I'd expect
that to be the idle rather than active current with a name like that.
That's just naming, but it does suggest that if we were to put this in
DT it should be more like the pinctrl equivalents and done as an array
of currents with matching array of names.

> 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.

I don't like the idea of having platform constraints which we ignore,
this goes against the general safety principal of the regulator API
which is that we won't touch the device state unless given explicit
permission to do so by the platform description.  The general idea is
to ensure that we can't do anything that might damage the platform
without approval from the system integrator.

> 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.

To be perfectly honest I'm not sure I see any great advantage here.
It's moving the performance numbers from the driver into DT which makes
them ABI which makes them harder to update, and the peak load is going
to be vulnerable to changes due to driver changes - if we do some
performance work and manage to start driving a device harder the numbers
specified in the DT may become wrong.  In general it seems safer to not
put things into ABI if we don't have to, and a substantial proportion of
things that might use regulators are off-SoC devices which don't
generally want or need DT fragments to describe them so we end up having
to duplicate things in multiple DTs which isn't ideal.

What's big push to put this in DT rather than just make the code pattern
easier to use?  I'm not totally against it but I'm having a hard time
getting enthusiastic about it.

I think the underlying thing here (and the reason you're seeing this a
lot with Qualcomm device specifically) is that Qualcomm have a firmware
interface to specify loads and wanted to implement selection of a low
power mode when things are idle in the way that Linux does with the
get_optimum_mode() interface.  Either they actually have some systems
where they were able to select different modes when things are running
or they don't have the thing we have where they discount loads from
consumers that have removed their enable vote, either way for those
platforms what's going on currently does seem like a sensible way of
setting things up.

Other platforms don't bother, I think that's likely to be some
combination of not caring about anything finer grained than system
suspend (which especially for Android is totally fair) and modern
regulators being much more able to to dynamically adapt to load than
those the code was written for.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Doug Anderson]

Hi,

On Fri, Jul 22, 2022 at 11:22 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Thu, Jul 21, 2022 at 06:26:44PM -0700, Douglas Anderson wrote:
>
> > 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.
>
> Sure...  I guess these devices are just hoping that their idle currents
> are near enough to zero to be safe.  Having refreshed my mind about how
> all this works that's basically the assumption that Linux is making
> here, that any idle currents will be low enough to be satisified by the
> lowest power state of any practical regulator and we don't need to keep
> track of them.
>
> > Specifically it should be noted that many devices don't really have a
> > need for the low power mode of regulators. Unfortunately the current
>
> What exactly do you mean by "a need for the low power mode of
> regulators"?

Let me try to come up with some examples. I'll assume a regulator that
can go up to 10mA in LPM (low power mode) and 100mA in HPM (high power
mode). Software needs to switch between LPM and HPM manually. I don't
have great numbers, but I'll assume that the LDO is 90% power
efficient in LPM and 40% power efficient in HPM.


Example 1: Trackpad that powers up on its own when it detects activity

Let's imagine a driver for a trackpad that talks over i2c. We power up
the trackpad's main supplies and the trackpad's firmware boots up. The
trackpad goes idle and starts drawing only a little power (1 mA?) When
someone gets near the trackpad (triggering the capacitive detectors)
then the firmware on the trackpad will kick into action and start
drawing a bunch of power (20mA?) to analyze the signals. If the
trackpad decides it got a real touch then it will send an interrupt to
the SoC.

This trackpad _cannot_ use software managed LPM/HPM because it doesn't
tell the main apps processor (AP) before it starts drawing more power
and the AP is in charge of selecting HPM/LPM. If we want to use this
trackpad with this regulator we just need to force it to HPM.

In my above terminology, this driver doesn't "need" the low power mode
of the regulators since it can't take advantage of it.


Example 2: Trackpad that warns the AP before it draws power

Let's imagine changing the firmware on the trackpad in the first
example. Now if the trackpad detects activity and wants to power up it
sends an interrupt to the AP. When the AP says "OK" it starts its high
power work. The AP can keep the trackpad in LPM most of the time and
switch to HPM only when there's activity. This saves power, but was it
a good design? It really depends. If this happens very very
infrequently then it might save power. If my math is right:

Idle trackpad in HPM (1mA @ 40% efficiency): 1 / .4 = 2.5mA
Idle trackpad in LPM (1mA @ 90% efficiency): 1 / .9 1.1mA

So we save 1.4mA of idle power but at the cost of complexity and
response time for the trackpad. We also may need to spin up the CPU
more often which could blow through our power savings.

The answer here is much less clear cut. Maybe this is totally not
worth it, but for a device that's very idle, that 1.4 mA might be
worth it. If the idle power was 8mA (8 / .4 = 20mA) it could really be
worth it.


Example 3: A SPI controller

Let's imagine a SPI controller that takes 5 mA when actively
transfering data and .1mA when idle (register retention). We have
several controllers powered by the same rail that may or may not be
transferring at the same time, so we might end up needing HPM. The
controller never powers up on its own--only when we tell it to
transfer.

We could design our driver in two ways:

3a) Turn on the regulators at driver probe time and init our
registers. Turn them off at system suspend and on at system resume
(and restore our registers). When actively doing a transfer we request
our 5mA load and when we're doing with our transfer we request our
.1mA load.

3b) Right before an active transfer, turn on our regulator and program
all registers (there aren't that many since SPI is a fairly simple
protocol). After the transfer is done, turn the regulator off.

Both 3a) and 3b) work. If reprogramming the registers was a chore or
slow we might choose 3a). If we choose 3b), the driver doesn't really
need to be HPM/LPM aware. Something needs to specify its load once but
it doesn't need to actively manage the load, which I guess is what I
meant by a driver not "needing" the low power mode of the regulator. I
meant that the driver doesn't need the API to actively manage its
load.


> > 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.
>
> We're only talking about a few lines of code here, and we could make a
> bulk helper to set loads if this is a serious issue.  You could even
> just add some more values in the bulk struct and have it be pure data
> for the drivers, regulator_bulk_get() could see a load and set it.  I
> think this may actually be the bulk of what's concerning you here?

Extending the bulk API would definitely be a good thing to do if we
reject my proposal. It would be easy to eliminate a lot of code across
a lot of drivers this way.

I guess the thing I don't love, though, is that to get to Dmitry's
dream of all regulators having their load specified the we have to add
regulator_set_load() calls to all sorts of drivers across the system.
Right now, at least in mainline, the regulator_set_load() calls are
almost entirely relegated to Qualcomm-related drivers and for
peripherals which are on the SoC itself.

Let's go back to the trackpad example, maybe looking specifically at
"hid-over-i2c". The driver here is fairly hardware agnostic and the
current used may not be very easy for the i2c-hid driver to specify in
code. I suppose one answer here is: use board constraints to force
this regulator to always use HPM (high power mode). That's fine, but
then if we have a way to turn this device "off" and it's on a shared
rail? Maybe then we'd want to be able to put the regulator in LPM
mode?

I guess this is really the place where being able to specify loads in
the device tree really seems like the right fit. We've got generic
(non-Qualcomm) components that perhaps are probable and handled by a
generic driver, but they're powered by these regulators where software
needs to change between HPM and LPM. It feels clean to specify the
load of the i2c-hid trackpad in a given board's device tree.


> > 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.
>
> This feels a bit mangled, I think largely because you're using the term
> "base load" to refer to something which I *think* you want to be the
> peak load the device might cause when it's actively doing stuff which is
> just very confusing.  If this were being specified per device I'd expect
> that to be the idle rather than active current with a name like that.
> That's just naming, but it does suggest that if we were to put this in
> DT it should be more like the pinctrl equivalents and done as an array
> of currents with matching array of names.

Sorry for the bad naming. Originally I was thinking that the dts would
specify the minimum current but as I thought about it more it felt
like it made more sense to specify the current that should be used for
a dumb driver (one that doesn't know anything about loads).

Doing an array would make sense. Like:

vdda-phy-load-names = "default", "off", "..."
vdda-phy-loads = <21800>, <500>, <...>;

Where "default" would be the load by default when enabled, "off" when
disabled off (if it's a shared rail it could conceivably draw power
even after regulator_disable()). Others states could be selected
manually.

If we did something like that, we'd have to figure out how those
meshed with existing driver calls. Happy for any thoughts.


> > 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.
>
> I don't like the idea of having platform constraints which we ignore,
> this goes against the general safety principal of the regulator API
> which is that we won't touch the device state unless given explicit
> permission to do so by the platform description.  The general idea is
> to ensure that we can't do anything that might damage the platform
> without approval from the system integrator.
>
> > 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.
>
> To be perfectly honest I'm not sure I see any great advantage here.
> It's moving the performance numbers from the driver into DT which makes
> them ABI which makes them harder to update, and the peak load is going
> to be vulnerable to changes due to driver changes - if we do some
> performance work and manage to start driving a device harder the numbers
> specified in the DT may become wrong.  In general it seems safer to not
> put things into ABI if we don't have to, and a substantial proportion of
> things that might use regulators are off-SoC devices which don't
> generally want or need DT fragments to describe them so we end up having
> to duplicate things in multiple DTs which isn't ideal.

Yeah, I know. Putting things in dts certainly has its downsides. The
argument you make can be taken to the extreme, of course. We can take
everything out of dts files and just specify a top-level board
compatible and everything else could be a table in source code (or,
here's an idea with extra flexibility, a board.c file). Now you never
worry about breaking ABI at all. ;-)

All snarkiness aside, though, I'm just trying to point out that
there's no definitive line to cross. No black and white. Anything we
put in dts from "assigned-clock-rates" to voltage ranges to whatever
could need to be updated as we discover more data. It is why I've also
said that for modern SoCs, I'll believe it's safe to ship device trees
separate from the kernel when I see someone add the "device tree fixup
stage" to the kernel boot flow.


> What's big push to put this in DT rather than just make the code pattern
> easier to use?  I'm not totally against it but I'm having a hard time
> getting enthusiastic about it.

It's nothing make or break and mostly it's just been kicking around in
the back of my head for the last few years. Mostly I gave a shot at
implementing something in response to your comment [1] where you said:
"You could add a way to specify constant base loads in DT on either a
per regulator or per consumer basis." ;-)

[1] https://lore.kernel.org/r/Ytk2dxEC2n/ffNpD@sirena.org.uk


> I think the underlying thing here (and the reason you're seeing this a
> lot with Qualcomm device specifically) is that Qualcomm have a firmware
> interface to specify loads and wanted to implement selection of a low
> power mode when things are idle in the way that Linux does with the
> get_optimum_mode() interface.  Either they actually have some systems
> where they were able to select different modes when things are running
> or they don't have the thing we have where they discount loads from
> consumers that have removed their enable vote, either way for those
> platforms what's going on currently does seem like a sensible way of
> setting things up.

I guess I'm a tad less charitable than you are. From what I've seen of
Qualcomm, there seems to be an in-house checklist that they have to go
through before landing code. Drivers have to pass the checklist. I
think it contains things like:

* All memory reads and writes must be the "relaxed" variant unless you
can strongly justify the non-relaxed one. It doesn't matter if it's in
performance critical code. The relaxed version is better, even if it
is more likely to cause bugs.

* Thou shalt always set a pin's drive strength to exactly 2 if it
doesn't have a reason to be otherwise. It doesn't matter if the pin is
an input and the drive strength has no effect. Someone said that 2 is
safest so always set it to 2.

* All regulators should be configured to be able to run in high power
mode and low power mode. You should always configure a load on them.
It doesn't matter if you never use low power mode in practice.


> Other platforms don't bother, I think that's likely to be some
> combination of not caring about anything finer grained than system
> suspend (which especially for Android is totally fair) and modern
> regulators being much more able to to dynamically adapt to load than
> those the code was written for.

Yeah. I think this comes up more in practice with Qualcomm because
they do their own PMICs and every one of their LDOs has this setting
for LPM / HPM. Since the setting is there I guess they decided they
should write software to use it. I agree that it can save some power
when used well, but I personally don't think the complexity is
justified except in a small number of cases.

I would hazard a guess that perhaps Qualcomm's LDOs all have this
feature because it's probably super critical for LTE modems and they
might as well add the feature for all the LDOs if they're adding it
for some of them.

-Doug

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 10861 bytes --]

On Fri, Jul 22, 2022 at 04:35:48PM -0700, Doug Anderson wrote:
> On Fri, Jul 22, 2022 at 11:22 AM Mark Brown <broonie@kernel.org> wrote:
> > On Thu, Jul 21, 2022 at 06:26:44PM -0700, Douglas Anderson wrote:

> > > Specifically it should be noted that many devices don't really have a
> > > need for the low power mode of regulators. Unfortunately the current

> > What exactly do you mean by "a need for the low power mode of
> > regulators"?

> Let me try to come up with some examples. I'll assume a regulator that
> can go up to 10mA in LPM (low power mode) and 100mA in HPM (high power
> mode). Software needs to switch between LPM and HPM manually. I don't
> have great numbers, but I'll assume that the LDO is 90% power
> efficient in LPM and 40% power efficient in HPM.

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?

> I guess the thing I don't love, though, is that to get to Dmitry's
> dream of all regulators having their load specified the we have to add
> regulator_set_load() calls to all sorts of drivers across the system.
> Right now, at least in mainline, the regulator_set_load() calls are
> almost entirely relegated to Qualcomm-related drivers and for
> peripherals which are on the SoC itself.

Sure, in order to actively manage this stuff someone is going to
have to type a bunch of numbers in somewhere.  It's just a
question of where we do the typing in that case and how much of
it is required.

> Let's go back to the trackpad example, maybe looking specifically at
> "hid-over-i2c". The driver here is fairly hardware agnostic and the
> current used may not be very easy for the i2c-hid driver to specify in
> code. I suppose one answer here is: use board constraints to force
> this regulator to always use HPM (high power mode). That's fine, but
> then if we have a way to turn this device "off" and it's on a shared
> rail? Maybe then we'd want to be able to put the regulator in LPM
> mode?

> I guess this is really the place where being able to specify loads in
> the device tree really seems like the right fit. We've got generic
> (non-Qualcomm) components that perhaps are probable and handled by a
> generic driver, but they're powered by these regulators where software
> needs to change between HPM and LPM. It feels clean to specify the
> load of the i2c-hid trackpad in a given board's device tree.

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.

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 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.

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.

> Sorry for the bad naming. Originally I was thinking that the dts would
> specify the minimum current but as I thought about it more it felt
> like it made more sense to specify the current that should be used for
> a dumb driver (one that doesn't know anything about loads).

> Doing an array would make sense. Like:

> vdda-phy-load-names = "default", "off", "..."
> vdda-phy-loads = <21800>, <500>, <...>;

> Where "default" would be the load by default when enabled, "off" when
> disabled off (if it's a shared rail it could conceivably draw power
> even after regulator_disable()). Others states could be selected
> manually.

I would talk in terms of active and idle but yes.

> All snarkiness aside, though, I'm just trying to point out that
> there's no definitive line to cross. No black and white. Anything we
> put in dts from "assigned-clock-rates" to voltage ranges to whatever

I see one of two things happening here.  Like I was saying above
my best guess is that a lot of the time we're just going to be
putting random values in the DT that are purely intended to
trigger a specific behaviour in the regulator in which case we
should probably just say what we're trying to accomplish rather
than adding in a pile of inaccurate data which might cause us
trouble later.  The other option is that something is actively
managing the load while things are active (which I'm not
immediately able to find any evidence of upstream) in which case
potentially things will likely vary with SoC integration so the
above binding becomes a bit more reasonable, but then you
absolutely have to have some code in the drivers.

> > What's big push to put this in DT rather than just make the code pattern
> > easier to use?  I'm not totally against it but I'm having a hard time
> > getting enthusiastic about it.

> It's nothing make or break and mostly it's just been kicking around in
> the back of my head for the last few years. Mostly I gave a shot at
> implementing something in response to your comment [1] where you said:
> "You could add a way to specify constant base loads in DT on either a
> per regulator or per consumer basis." ;-)

I was tending more towards the per regulator end of things there
TBH, and the more I think about it the more likely it is that the
underlying intent (or at least practical effect) is the mode
setting thing.

> > I think the underlying thing here (and the reason you're seeing this a
> > lot with Qualcomm device specifically) is that Qualcomm have a firmware
> > interface to specify loads and wanted to implement selection of a low
> > power mode when things are idle in the way that Linux does with the
> > get_optimum_mode() interface.  Either they actually have some systems
> > where they were able to select different modes when things are running
> > or they don't have the thing we have where they discount loads from
> > consumers that have removed their enable vote, either way for those
> > platforms what's going on currently does seem like a sensible way of
> > setting things up.
> 
> I guess I'm a tad less charitable than you are. From what I've seen of
> Qualcomm, there seems to be an in-house checklist that they have to go
> through before landing code. Drivers have to pass the checklist. I
> think it contains things like:

...

> * All regulators should be configured to be able to run in high power
> mode and low power mode. You should always configure a load on them.
> It doesn't matter if you never use low power mode in practice.

I think this one kind of falls out of the way they've set up
their firmware interfaces - having abstraced away modes in favour
of currents there's not the expressiveness to do much else unless
you jump around things at the AP level.  Unfortunately you then
run into the dual problems that typically nobody's actually doing
the fine grained power measurements and the amount you can save
with active management when the device is running is not worth
the power consumed doing that active management anyway.  The use
of currents isn't even a bad choice here, it's really hard to
abstract modes without using currents.

Some of this stuff is also going to be old stylistic things that
aren't causing enough of a problem to get in the way (eg, the
_set_load() calls are going to take hardly any time to implement,
you can write a lot of them before it's more work than deciding
not to bother) and were probably much more useful in the past.

> > Other platforms don't bother, I think that's likely to be some
> > combination of not caring about anything finer grained than system
> > suspend (which especially for Android is totally fair) and modern
> > regulators being much more able to to dynamically adapt to load than
> > those the code was written for.

> Yeah. I think this comes up more in practice with Qualcomm because
> they do their own PMICs and every one of their LDOs has this setting
> for LPM / HPM. Since the setting is there I guess they decided they
> should write software to use it. I agree that it can save some power
> when used well, but I personally don't think the complexity is
> justified except in a small number of cases.

> I would hazard a guess that perhaps Qualcomm's LDOs all have this
> feature because it's probably super critical for LTE modems and they
> might as well add the feature for all the LDOs if they're adding it
> for some of them.

The underlying hardware feature is very easy to implement and a
pretty standard feature for both DCDCs and LDOs, a good number of
upstream drivers have mode support and I'd expect more devices do
but just don't have it in the driver for whatever reason (I know
some of them are reverse engineered for example) or only provide
control along with suspend modes.  It's very desirable for
performance during suspend, but as you say the benefit of
actively mananging it outside of suspend is more questionable.
Simply having a CPU cluster and the RAM active is likely to push
the savings down towards the noise.  What makes things noticable
with these Qualcomm devices is their firmware abstraction.

The more I think about this the more I think that what's
practically going on here fits better with the mode interface
than the load one, normally I'd not like doing a run around the
abstractions the underlying thing is offering but it really does
seem like someone has to bite the bullet and do that runaround
and it's probably going to be less work all round at the
regulator level.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1029 bytes --]

On Sat, Jul 23, 2022 at 08:11:01PM +0100, Mark Brown wrote:

> The more I think about this the more I think that what's
> practically going on here fits better with the mode interface
> than the load one, normally I'd not like doing a run around the
> abstractions the underlying thing is offering but it really does
> seem like someone has to bite the bullet and do that runaround
> and it's probably going to be less work all round at the
> regulator level.

Actually thinking about it even more for most cases it may even
just be that we need to add some number to the load request while
the regulator is powered on, the existing system-load property
would probably do the trick here if putting things in DT.
Depending on if the firmware counts loads from things that voted
off or not the driver might need to do an update on the load when
enabling and disabling.  That is less of an end run around the
abstraction, though it does cause issues for the system-load's
intended use so perhaps a separate property might be safer.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Doug Anderson]

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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 3921 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]