[RFC,4/8] regulator: core: Check enabling bypass respects constraints
diff mbox

Message ID 1edff9bc610969b0c53fa1080d5db021c8e00b2d.1490199005.git.leonard.crestez@nxp.com
State RFC, archived
Headers show

Commit Message

Leonard Crestez March 22, 2017, 4:53 p.m. UTC
Enabling bypass mode makes a regulator passthrough the supply voltage
directly. It is possible that the supply voltage is set high enough that
it violates machine constraints so let's check for that.

The supply voltage might be higher because of min_dropout_uV or maybe
there is just an unrelated consumer who requested a higher voltage.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 drivers/regulator/core.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 50 insertions(+), 2 deletions(-)

Comments

Mark Brown March 24, 2017, 12:52 p.m. UTC | #1
On Wed, Mar 22, 2017 at 06:53:06PM +0200, Leonard Crestez wrote:

> Enabling bypass mode makes a regulator passthrough the supply voltage
> directly. It is possible that the supply voltage is set high enough that
> it violates machine constraints so let's check for that.
> 
> The supply voltage might be higher because of min_dropout_uV or maybe
> there is just an unrelated consumer who requested a higher voltage.

I would expect that if bypass is enabled then the constraints on the
parent regulator would be set appropriately to support this, I wouldn't
expect that we'd try to apply the operating constraints of the regulator
to the supply.  Usually bypass is used for low power retention modes
with different settings to those used in normal operation that wouldn't
be desired in normal operation, if we were going to have constraints for
this I'd expect a separate set used during bypass.
Leonard Crestez March 28, 2017, 12:39 p.m. UTC | #2
On Fri, 2017-03-24 at 12:52 +0000, Mark Brown wrote:
> On Wed, Mar 22, 2017 at 06:53:06PM +0200, Leonard Crestez wrote:
> 
> > 
> > Enabling bypass mode makes a regulator passthrough the supply voltage
> > directly. It is possible that the supply voltage is set high enough that
> > it violates machine constraints so let's check for that.
> > 
> > The supply voltage might be higher because of min_dropout_uV or maybe
> > there is just an unrelated consumer who requested a higher voltage.
> I would expect that if bypass is enabled then the constraints on the
> parent regulator would be set appropriately to support this, I wouldn't
> expect that we'd try to apply the operating constraints of the regulator
> to the supply.  Usually bypass is used for low power retention modes
> with different settings to those used in normal operation that wouldn't
> be desired in normal operation, if we were going to have constraints for
> this I'd expect a separate set used during bypass.

In this particular case it's not possible to set constraints on the parent
regulator so that both ldo-enable and ldo-bypass modes work. The maximum allowed
voltage for ldo-bypass is lower than the minimum required to support the chip at
max frequency wit ldo-enable.

It would be possible to also change the constraint values on the PMIC together
with ldo-bypass in the .dts files but that seems awful.

I'm not sure I understand why you are against applying constraints to the parent
when in bypass mode, it seems like the obvious thing to do if you want to
support flexible configuration. The check I introduced is probably not enough to
cover all cases, for example it would still be possible to explicitly change
parent voltage afterwards.

A regulator_dev registers a consumer for the supply. Right now this is being
used to propagate minimum voltages upwards since commit fc42112c0eaa ("Propagate
voltage changes to supply regulators"). It seems to me like it would be
reasonable to also use it to propagate maximum voltage from constraints, right?

Instead of asking for [best_uV + min_dropout_uV, INT_MAX] it could instead ask
for [min_uV + bypass ? min_dropout_uV : 0, min(max_uV, constraints->max_uV)].
The _regulator_do_set_voltage call on the supply can deal with stuff like
mapping it to a selector, just like it does for regulator consumers.

If more elaborate constraints are required instead of this simple behavior it
could be handled by adding an interface for drivers to expose explicit dynamic
min/max constraints.

--
Regards,
Leonard
Mark Brown March 28, 2017, 4:47 p.m. UTC | #3
On Tue, Mar 28, 2017 at 03:39:41PM +0300, Leonard Crestez wrote:
> On Fri, 2017-03-24 at 12:52 +0000, Mark Brown wrote:

Please fix your mail client to word wrap within paragraphs at something
substantially less than 80 columns.  Doing this makes your messages much
easier to read and reply to.

> > to the supply.  Usually bypass is used for low power retention modes
> > with different settings to those used in normal operation that wouldn't
> > be desired in normal operation, if we were going to have constraints for
> > this I'd expect a separate set used during bypass.

> In this particular case it's not possible to set constraints on the parent
> regulator so that both ldo-enable and ldo-bypass modes work. The maximum allowed
> voltage for ldo-bypass is lower than the minimum required to support the chip at
> max frequency wit ldo-enable.

If things are really so sensitive that you can't bypass without lowering
the voltage then it's hard to see how you can safely transition into and
out of bypass mode.  

> I'm not sure I understand why you are against applying constraints to the parent
> when in bypass mode, it seems like the obvious thing to do if you want to
> support flexible configuration. The check I introduced is probably not enough to
> cover all cases, for example it would still be possible to explicitly change
> parent voltage afterwards.

To repeat what I said previously the whole point of bypassing is to not
do regulation and generally the constraints in the unregulated idle case
are substantially more relaxed.  This would break use cases relying on
the existing behaviour which wouldn't expect to affect the parent
voltage at all, either stopping things working or making them less
efficient by needlessly regulating the voltage down which defeats the
main point of bypassing.
Leonard Crestez March 28, 2017, 7:49 p.m. UTC | #4
On Tue, 2017-03-28 at 17:47 +0100, Mark Brown wrote:
> On Tue, Mar 28, 2017 at 03:39:41PM +0300, Leonard Crestez wrote:
> > On Fri, 2017-03-24 at 12:52 +0000, Mark Brown wrote:

> Please fix your mail client to word wrap within paragraphs at something
> substantially less than 80 columns.  Doing this makes your messages much
> easier to read and reply to.

Sorry, still messing around with Evolution and Exchange.

> > > to the supply.  Usually bypass is used for low power retention modes
> > > with different settings to those used in normal operation that wouldn't
> > > be desired in normal operation, if we were going to have constraints for
> > > this I'd expect a separate set used during bypass.
> > 
> > In this particular case it's not possible to set constraints on the parent
> > regulator so that both ldo-enable and ldo-bypass modes work. The maximum allowed
> > voltage for ldo-bypass is lower than the minimum required to support the chip at
> > max frequency wit ldo-enable.
> 
> If things are really so sensitive that you can't bypass without lowering
> the voltage then it's hard to see how you can safely transition into and
> out of bypass mode.

The CPU frequency is set to the minimum value so that when bypass mode
is entered and voltage rises (because the dropout goes away) it is
still low enough. Transitioning out of bypass mode is not implemented
but you would presumably have to go to the minimum frequency again,
raise the voltage above what is required and the flip the switch.

> > I'm not sure I understand why you are against applying constraints to the parent
> > when in bypass mode, it seems like the obvious thing to do if you want to
> > support flexible configuration. The check I introduced is probably not enough to
> > cover all cases, for example it would still be possible to explicitly change
> > parent voltage afterwards.
> 
> To repeat what I said previously the whole point of bypassing is to not
> do regulation and generally the constraints in the unregulated idle case
> are substantially more relaxed.  This would break use cases relying on
> the existing behaviour which wouldn't expect to affect the parent
> voltage at all, either stopping things working or making them less
> efficient by needlessly regulating the voltage down which defeats the
> main point of bypassing.

So what you want is to prevent voltage changes unless strictly
required, even lowering? What I want is to get the minimum voltage in
the SOC because that's where power is being consumed.

It's not at all obvious that in bypass mode the output constraints of a
regulator need not be respected by the core. I expected the opposite,
this is something that should be documented.

But if the bypassed regulator has a downstream consumer then it's
requirements should definitely still be met in bypass mode, right? I
could set my maximum voltage directly from cpufreq in that case.

Or should a bypassed regulator ignore all other requests? One of the
behaviors that this patch series relies on is that calling set_voltage
on a bypassed regulator propagates this request to the supply and picks
the minimum voltage there. An alternative implementation would be to
call set_voltage directly on the supply regulator by changing the
"{arm,soc,pu}-supply" references in DT to point to the PMIC instead.
Would that be better?

Both approaches work. Relying on propagation feels like it is the
"right way" to handle this, even if it's harder to get right and the
regulator core does not entirely support it. But it's possible that
this is based on a misunderstanding of what "bypass" is actually
supposed to do.
Mark Brown April 6, 2017, 6:52 p.m. UTC | #5
On Tue, Mar 28, 2017 at 10:49:55PM +0300, Leonard Crestez wrote:
> On Tue, 2017-03-28 at 17:47 +0100, Mark Brown wrote:

> > To repeat what I said previously the whole point of bypassing is to not
> > do regulation and generally the constraints in the unregulated idle case
> > are substantially more relaxed.  This would break use cases relying on
> > the existing behaviour which wouldn't expect to affect the parent
> > voltage at all, either stopping things working or making them less
> > efficient by needlessly regulating the voltage down which defeats the
> > main point of bypassing.

> So what you want is to prevent voltage changes unless strictly
> required, even lowering? What I want is to get the minimum voltage in
> the SOC because that's where power is being consumed.

So your end goal here is to bypass a regulator which is forced into your
system design by being integrated into the SoC which isn't able to
regulate down to a low enough voltage for your use case?  I guess one
question is if you need to use the regulator at all?

> It's not at all obvious that in bypass mode the output constraints of a
> regulator need not be respected by the core. I expected the opposite,
> this is something that should be documented.

SubmittingPatches...  Bear in mind that most regulators are fixed
voltage in a given system so bypass would be very difficult to use if it
tried to pass the constraints upstream.

> But if the bypassed regulator has a downstream consumer then it's
> requirements should definitely still be met in bypass mode, right? I
> could set my maximum voltage directly from cpufreq in that case.

What we're interpreting bypass mode as meaning is "stop regulating".
There will still be some limits but we're expecting them to be enforced
in the extremes of the constraints in the parent regulators.

> Or should a bypassed regulator ignore all other requests? One of the
> behaviors that this patch series relies on is that calling set_voltage
> on a bypassed regulator propagates this request to the supply and picks
> the minimum voltage there. An alternative implementation would be to

Yes, the expectation is that if anything is being changed it won't have
any effect until regulation is reenabled but we're not particularly
expecting much activity on bypassed regulators.

> call set_voltage directly on the supply regulator by changing the
> "{arm,soc,pu}-supply" references in DT to point to the PMIC instead.
> Would that be better?

That's more what's expected here.

> Both approaches work. Relying on propagation feels like it is the
> "right way" to handle this, even if it's harder to get right and the
> regulator core does not entirely support it. But it's possible that
> this is based on a misunderstanding of what "bypass" is actually
> supposed to do.

Another option would be to add a regulator configuration which allowed
the regulator to transparently go into bypass mode if the parent could
do things directly, only enabling regulation if the parent couldn't
support.  That would mean you'd loose the supply cleanup function which
is typically part of why there are LDOs in the system but it sounds like
you're OK with that in at least your use case.

We could also perhaps have a way to set the regulator into permanent
bypass mode from the constraints for use cases that just never need it
and then remap the supply relationship at probe time (which avoids any
special runtime casing) for something even simpler.
Leonard Crestez April 7, 2017, 10:51 a.m. UTC | #6
On Thu, 2017-04-06 at 19:52 +0100, Mark Brown wrote:
> On Tue, Mar 28, 2017 at 10:49:55PM +0300, Leonard Crestez wrote:
> > On Tue, 2017-03-28 at 17:47 +0100, Mark Brown wrote:

> > > To repeat what I said previously the whole point of bypassing is to not
> > > do regulation and generally the constraints in the unregulated idle case
> > > are substantially more relaxed.  This would break use cases relying on
> > > the existing behaviour which wouldn't expect to affect the parent
> > > voltage at all, either stopping things working or making them less
> > > efficient by needlessly regulating the voltage down which defeats the
> > > main point of bypassing.

> > So what you want is to prevent voltage changes unless strictly
> > required, even lowering? What I want is to get the minimum voltage in
> > the SOC because that's where power is being consumed.

> So your end goal here is to bypass a regulator which is forced into your
> system design by being integrated into the SoC which isn't able to
> regulate down to a low enough voltage for your use case?  I guess one
> question is if you need to use the regulator at all?

Both the PMIC and the LDOs can provide the required voltage range. LDO
bypass mode is a design tradeoff: the PMIC provides more efficient
conversion but with more fluctuations.

My question was about how the regulator framework handles constraints
and consumer voltage requests. What I want is for the output to be set
to the minimum value that meets all constraints. Maybe other users
would prefer regulators to keep their output as much as possible?

This is relevant even if LDOs are enabled, it's still preferable to
have PMIC output as low as possible because then less of the voltage
reduction is performed in the LDO.

It currently seems to work how I expect but from your statement it's
not clear if it's entirely intentional.

> > It's not at all obvious that in bypass mode the output constraints of a
> > regulator need not be respected by the core. I expected the opposite,
> > this is something that should be documented.

> SubmittingPatches...  Bear in mind that most regulators are fixed
> voltage in a given system so bypass would be very difficult to use if it
> tried to pass the constraints upstream.

> > But if the bypassed regulator has a downstream consumer then it's
> > requirements should definitely still be met in bypass mode, right? I
> > could set my maximum voltage directly from cpufreq in that case.

> What we're interpreting bypass mode as meaning is "stop regulating".
> There will still be some limits but we're expecting them to be enforced
> in the extremes of the constraints in the parent regulators.

> > Or should a bypassed regulator ignore all other requests? One of the
> > behaviors that this patch series relies on is that calling set_voltage
> > on a bypassed regulator propagates this request to the supply and picks
> > the minimum voltage there. An alternative implementation would be to

> Yes, the expectation is that if anything is being changed it won't have
> any effect until regulation is reenabled but we're not particularly
> expecting much activity on bypassed regulators.

OK, so it is up to consumers to ensure the supply voltage is acceptable
before allowing bypass. If they want to do something clever they should
register as a consumer to the supply as well.

> > 
> > call set_voltage directly on the supply regulator by changing the
> > "{arm,soc,pu}-supply" references in DT to point to the PMIC instead.
> > Would that be better?

> That's more what's expected here.

Ok, I will try that approach instead.

> > Both approaches work. Relying on propagation feels like it is the
> > "right way" to handle this, even if it's harder to get right and the
> > regulator core does not entirely support it. But it's possible that
> > this is based on a misunderstanding of what "bypass" is actually
> > supposed to do.

> Another option would be to add a regulator configuration which allowed
> the regulator to transparently go into bypass mode if the parent could
> do things directly, only enabling regulation if the parent couldn't
> support.  That would mean you'd loose the supply cleanup function which
> is typically part of why there are LDOs in the system but it sounds like
> you're OK with that in at least your use case.

Dynamic enabling of bypass mode in the core seems very complex and not
terribly useful for my case. I pretty much want them always in bypass
or always enabled.
Mark Brown April 7, 2017, 11:22 a.m. UTC | #7
On Fri, Apr 07, 2017 at 01:51:52PM +0300, Leonard Crestez wrote:

> It currently seems to work how I expect but from your statement it's
> not clear if it's entirely intentional.

The current behaviour of bypassed regulators is intentional.

> > > Or should a bypassed regulator ignore all other requests? One of the
> > > behaviors that this patch series relies on is that calling set_voltage
> > > on a bypassed regulator propagates this request to the supply and picks
> > > the minimum voltage there. An alternative implementation would be to

> > Yes, the expectation is that if anything is being changed it won't have
> > any effect until regulation is reenabled but we're not particularly
> > expecting much activity on bypassed regulators.

> OK, so it is up to consumers to ensure the supply voltage is acceptable
> before allowing bypass. If they want to do something clever they should
> register as a consumer to the supply as well.

No, that's not expected either - one or the other is fine but both isn't
expected.  What you're looking for here is something logically different
to bypass even if the hardware implementation ends up the same.
Leonard Crestez April 13, 2017, 8:46 p.m. UTC | #8
On Fri, 2017-04-07 at 12:22 +0100, Mark Brown wrote:
> On Fri, Apr 07, 2017 at 01:51:52PM +0300, Leonard Crestez wrote:

> > It currently seems to work how I expect but from your statement it's
> > not clear if it's entirely intentional.

> The current behaviour of bypassed regulators is intentional.

I did not mean to imply that there is something wrong with bypassed
regulators. I just wanted more information about how regulators (non-
bypassed) pick their voltage when consumers allow a range.

After some more reading through the code it seems that the driver
itself receives the range (either through set_voltage or map_voltage)
and gets to make the choice.

So it seems fine for my concerns, sorry to bother you.

--
Regards,
Leonard

Patch
diff mbox

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 53d4fc7..9d893aa 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -3453,6 +3453,54 @@  int regulator_set_load(struct regulator *regulator, int uA_load)
 }
 EXPORT_SYMBOL_GPL(regulator_set_load);
 
+static int _regulator_set_bypass(struct regulator *regulator, bool bypass)
+{
+	struct regulator_dev *rdev = regulator->rdev;
+	int output_voltage;
+	int supply_voltage;
+
+	if (bypass && !rdev->supply) {
+		rdev_err(rdev, "Refuse to set bypass on regulator with no supply!\n");
+		return -EINVAL;
+	}
+
+	/* Check that enabling bypass won't break constraints */
+	if (bypass && _regulator_is_enabled(rdev)) {
+		output_voltage = _regulator_get_voltage(rdev);
+		if (output_voltage < 0) {
+			rdev_err(rdev, "Failed to get old output voltage before"
+					" enabling bypass: %d\n", output_voltage);
+			return output_voltage;
+		}
+		supply_voltage = _regulator_get_voltage(rdev->supply->rdev);
+		if (supply_voltage < 0) {
+			rdev_err(rdev, "Failed to get supply voltage before"
+					" enabling bypass: %d\n", supply_voltage);
+			return supply_voltage;
+		}
+		if (supply_voltage < rdev->constraints->min_uV ||
+				supply_voltage > rdev->constraints->max_uV) {
+			rdev_err(rdev, "Enabling bypass would change voltage"
+					" from %duV to %duV violating"
+					" constraint range %duV to %duV\n",
+				output_voltage,
+				supply_voltage,
+				rdev->constraints->min_uV,
+				rdev->constraints->max_uV);
+			return -EINVAL;
+		}
+		rdev_dbg(rdev, "Enabling bypass would change voltage"
+				" from %duV to %duV respecting"
+				" constraint range %duV to %duV\n",
+			output_voltage,
+			supply_voltage,
+			rdev->constraints->min_uV,
+			rdev->constraints->max_uV);
+	}
+
+	return rdev->desc->ops->set_bypass(rdev, bypass);
+}
+
 /**
  * regulator_allow_bypass - allow the regulator to go into bypass mode
  *
@@ -3481,7 +3529,7 @@  int regulator_allow_bypass(struct regulator *regulator, bool enable)
 		rdev->bypass_count++;
 
 		if (rdev->bypass_count == rdev->open_count) {
-			ret = rdev->desc->ops->set_bypass(rdev, enable);
+			ret = _regulator_set_bypass(regulator, enable);
 			if (ret != 0)
 				rdev->bypass_count--;
 		}
@@ -3490,7 +3538,7 @@  int regulator_allow_bypass(struct regulator *regulator, bool enable)
 		rdev->bypass_count--;
 
 		if (rdev->bypass_count != rdev->open_count) {
-			ret = rdev->desc->ops->set_bypass(rdev, enable);
+			ret = _regulator_set_bypass(regulator, enable);
 			if (ret != 0)
 				rdev->bypass_count++;
 		}