[PATCH v4 4/4] regulator: Prevent falling too fast

[PATCH v4 4/4] regulator: Prevent falling too fast

[Matthias Kaehlcke]

From: Douglas Anderson <dianders@chromium.org>

On some boards it's possible that transitioning the PWM regulator
downwards too fast will trigger the over voltage protection (OVP) on the
regulator.  This is because until the voltage actually falls there is a
time when the requested voltage is much lower than the actual voltage.

We'll fix this OVP problem by allowing users to specify the maximum
voltage that we can safely fall.  Apparently this maximum safe voltage
should be specified as a percentage of the current voltage.  The
regulator will then break things into separate steps with a delay in
between.

In order to figure out what the delay should be we need to figure out
how slowly the voltage rail might fall in the worst (slowest) case.
We'll assume this worst case is present and delay so we know for sure
that we've finished each step.

In this patch we actually block returning from the set_voltage() call
until we've finished delaying.  A future patch atop this one might
choose to return more immediately and let the voltages fall in the
background.  That would possibly to allow us to cancel a slow downward
decay if there was a request to go back up.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
Changes in v4:
- Moved from PWM regulator to regulator core
- Added 'regulator' prefix to device tree properties

 .../devicetree/bindings/regulator/regulator.txt    |  7 +++
 drivers/regulator/core.c                           | 58 ++++++++++++++++++----
 drivers/regulator/of_regulator.c                   | 34 +++++++++++++
 include/linux/regulator/machine.h                  |  2 +
 4 files changed, 92 insertions(+), 9 deletions(-)

diff --git a/Documentation/devicetree/bindings/regulator/regulator.txt b/Documentation/devicetree/bindings/regulator/regulator.txt
index d2d1c4d..b7fbd9a 100644
--- a/Documentation/devicetree/bindings/regulator/regulator.txt
+++ b/Documentation/devicetree/bindings/regulator/regulator.txt
@@ -23,6 +23,13 @@ Optional properties:
   and board design issues such as trace capacitance and load on the supply.
 - regulator-settle-time-up-us: Time to settle down after a voltage increase
   (unit: us). For regulators with a ramp delay the two values are added.
+- regulator-safe-fall-percent:  If specified, it's not safe to transition the
+  regulator down faster than this amount and bigger jumps need to be broken into
+  more than one step.
+- regulator-slowest-decay-rate: Describes how slowly the regulator voltage will
+  decay down in the worst case (lightest expected load). Specified in uV / us
+  (like main regulator ramp rate). This is required when safe-fall-percent is
+  specified.
 - regulator-soft-start: Enable soft start so that voltage ramps slowly
 - regulator-state-mem sub-root node for Suspend-to-RAM mode
   : suspend to memory, the device goes to sleep, but all data stored in memory,
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index ed69fdc..9124532 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -105,8 +105,8 @@ static int _regulator_get_current_limit(struct regulator_dev *rdev);
 static unsigned int _regulator_get_mode(struct regulator_dev *rdev);
 static int _notifier_call_chain(struct regulator_dev *rdev,
 				  unsigned long event, void *data);
-static int _regulator_do_set_voltage(struct regulator_dev *rdev,
-				     int min_uV, int max_uV);
+static int _regulator_set_voltage(struct regulator_dev *rdev,
+				int min_uV, int max_uV);
 static struct regulator *create_regulator(struct regulator_dev *rdev,
 					  struct device *dev,
 					  const char *supply_name);
@@ -910,7 +910,7 @@ static int machine_constraints_voltage(struct regulator_dev *rdev,
 		if (target_min != current_uV || target_max != current_uV) {
 			rdev_info(rdev, "Bringing %duV into %d-%duV\n",
 				  current_uV, target_min, target_max);
-			ret = _regulator_do_set_voltage(
+			ret = _regulator_set_voltage(
 				rdev, target_min, target_max);
 			if (ret < 0) {
 				rdev_err(rdev,
@@ -2753,8 +2753,6 @@ static int _regulator_do_set_voltage(struct regulator_dev *rdev,
 	int old_selector = -1;
 	int old_uV = _regulator_get_voltage(rdev);
 
-	trace_regulator_set_voltage(rdev_get_name(rdev), min_uV, max_uV);
-
 	min_uV += rdev->constraints->uV_offset;
 	max_uV += rdev->constraints->uV_offset;
 
@@ -2842,11 +2840,53 @@ no_delay:
 				     (void *)data);
 	}
 
-	trace_regulator_set_voltage_complete(rdev_get_name(rdev), best_val);
-
 	return ret;
 }
 
+static int _regulator_set_voltage(struct regulator_dev *rdev,
+				     int min_uV, int max_uV)
+{
+	int safe_fall_percent = rdev->constraints->safe_fall_percent;
+	int slowest_decay_rate = rdev->constraints->slowest_decay_rate;
+	int orig_uV = _regulator_get_voltage(rdev);
+	int uV = orig_uV;
+	int ret;
+
+	trace_regulator_set_voltage(rdev_get_name(rdev), min_uV, max_uV);
+
+	 /* If we're rising or we're falling but don't need to slow; easy */
+	if (min_uV >= uV || !safe_fall_percent)
+		return _regulator_do_set_voltage(rdev, min_uV, max_uV);
+
+	while (uV > min_uV) {
+		int max_drop_uV = (uV * safe_fall_percent) / 100;
+		int next_uV;
+		int delay;
+
+		/* Make sure no infinite loop even in crazy cases */
+		if (max_drop_uV == 0)
+			max_drop_uV = 1;
+
+		next_uV = max_t(int, min_uV, uV - max_drop_uV);
+		delay = DIV_ROUND_UP(uV - next_uV, slowest_decay_rate);
+
+		ret = _regulator_do_set_voltage(rdev, uV, next_uV);
+		if (ret) {
+			/* Try to go back to original */
+			_regulator_do_set_voltage(rdev, uV, orig_uV);
+			return ret;
+		}
+
+		usleep_range(delay, delay + DIV_ROUND_UP(delay, 10));
+		uV = next_uV;
+	}
+
+	trace_regulator_set_voltage_complete(rdev_get_name(rdev),
+					_regulator_get_voltage(rdev));
+
+	return 0;
+}
+
 static int regulator_set_voltage_unlocked(struct regulator *regulator,
 					  int min_uV, int max_uV)
 {
@@ -2937,7 +2977,7 @@ static int regulator_set_voltage_unlocked(struct regulator *regulator,
 		}
 	}
 
-	ret = _regulator_do_set_voltage(rdev, min_uV, max_uV);
+	ret = _regulator_set_voltage(rdev, min_uV, max_uV);
 	if (ret < 0)
 		goto out2;
 
@@ -3144,7 +3184,7 @@ int regulator_sync_voltage(struct regulator *regulator)
 	if (ret < 0)
 		goto out;
 
-	ret = _regulator_do_set_voltage(rdev, min_uV, max_uV);
+	ret = _regulator_set_voltage(rdev, min_uV, max_uV);
 
 out:
 	mutex_unlock(&rdev->mutex);
diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c
index 056f242..fc5818f 100644
--- a/drivers/regulator/of_regulator.c
+++ b/drivers/regulator/of_regulator.c
@@ -94,6 +94,40 @@ static void of_get_regulation_constraints(struct device_node *np,
 	if (!ret)
 		constraints->settle_time_up = pval;
 
+	ret = of_property_read_u32(np, "regulator-slowest-decay-rate", &pval);
+	if (!ret)
+		constraints->slowest_decay_rate = pval;
+	else
+		constraints->slowest_decay_rate = INT_MAX;
+
+	ret = of_property_read_u32(np, "regulator-safe-fall-percent", &pval);
+	if (!ret)
+		constraints->safe_fall_percent = pval;
+
+	/* We use the value as int and as divider; sanity check */
+	if (constraints->slowest_decay_rate == 0) {
+		pr_err("%s: regulator-slowest-decay-rate must not be 0\n",
+			np->name);
+		constraints->slowest_decay_rate = INT_MAX;
+	} else if (constraints->slowest_decay_rate > INT_MAX) {
+		pr_err("%s: regulator-slowest-decay-rate (%u) too big\n",
+			np->name, constraints->slowest_decay_rate);
+		constraints->slowest_decay_rate = INT_MAX;
+	}
+
+	if (constraints->safe_fall_percent > 100) {
+		pr_err("%s: regulator-safe-fall-percent (%u) > 100\n",
+			np->name, constraints->safe_fall_percent);
+		constraints->safe_fall_percent = 0;
+       }
+
+	if (constraints->safe_fall_percent &&
+		!constraints->slowest_decay_rate) {
+		pr_err("%s: regulator-slowest-decay-rate requires "
+			"regulator-safe-fall-percent\n", np->name);
+		constraints->safe_fall_percent = 0;
+	}
+
 	constraints->soft_start = of_property_read_bool(np,
 					"regulator-soft-start");
 	ret = of_property_read_u32(np, "regulator-active-discharge", &pval);
diff --git a/include/linux/regulator/machine.h b/include/linux/regulator/machine.h
index 8681a82..4f49f92 100644
--- a/include/linux/regulator/machine.h
+++ b/include/linux/regulator/machine.h
@@ -152,6 +152,8 @@ struct regulation_constraints {
 	unsigned int ramp_delay;
 	unsigned int enable_time;
 	unsigned int settle_time_up;
+	unsigned int slowest_decay_rate;
+	unsigned int safe_fall_percent;
 
 	unsigned int active_discharge;
 
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH v4 4/4] regulator: Prevent falling too fast

[Mark Brown]

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

On Tue, Sep 06, 2016 at 12:05:24PM -0700, Matthias Kaehlcke wrote:

> On some boards it's possible that transitioning the PWM regulator
> downwards too fast will trigger the over voltage protection (OVP) on the
> regulator.  This is because until the voltage actually falls there is a
> time when the requested voltage is much lower than the actual voltage.

So your PWM regulators are not very good and overshooting?  Do you have
any numbers on this - what values do you end up setting for both the
delay and safe fall percentages?

> We'll fix this OVP problem by allowing users to specify the maximum
> voltage that we can safely fall.  Apparently this maximum safe voltage
> should be specified as a percentage of the current voltage.  The
> regulator will then break things into separate steps with a delay in
> between.

I'd like to see some more thorough analysis than just an "apparently"
here.  It's making the code more fiddly for something very handwavy.

> @@ -2753,8 +2753,6 @@ static int _regulator_do_set_voltage(struct regulator_dev *rdev,
>  	int old_selector = -1;
>  	int old_uV = _regulator_get_voltage(rdev);
>  
> -	trace_regulator_set_voltage(rdev_get_name(rdev), min_uV, max_uV);
> -
>  	min_uV += rdev->constraints->uV_offset;
>  	max_uV += rdev->constraints->uV_offset;
>  
> @@ -2842,11 +2840,53 @@ no_delay:
>  				     (void *)data);
>  	}
>  
> -	trace_regulator_set_voltage_complete(rdev_get_name(rdev), best_val);
> -
>  	return ret;

Let's remove some trace points too because...?  These *are* the actual
voltage changes in the device, we're just splitting things up into a
series of transitions.

> +static int _regulator_set_voltage(struct regulator_dev *rdev,
> +				     int min_uV, int max_uV)
> +{
> +	int safe_fall_percent = rdev->constraints->safe_fall_percent;
> +	int slowest_decay_rate = rdev->constraints->slowest_decay_rate;
> +	int orig_uV = _regulator_get_voltage(rdev);
> +	int uV = orig_uV;
> +	int ret;
> +
> +	trace_regulator_set_voltage(rdev_get_name(rdev), min_uV, max_uV);
> +
> +	 /* If we're rising or we're falling but don't need to slow; easy */
> +	if (min_uV >= uV || !safe_fall_percent)

Indentation is broken, the two lines above don't agree with each other.

> +	ret = of_property_read_u32(np, "regulator-slowest-decay-rate", &pval);
> +	if (!ret)
> +		constraints->slowest_decay_rate = pval;
> +	else
> +		constraints->slowest_decay_rate = INT_MAX;

The documentation said this is mandatory if we have a safe fall rate
specified but the code says it's optional and we'll silently default to
an absurdly high rate instead (it's odd that a large number in a field
marked slowest is fast BTW).  Complaining loudly seems better than
ignoring the error.

> +	/* We use the value as int and as divider; sanity check */
> +	if (constraints->slowest_decay_rate == 0) {
> +		pr_err("%s: regulator-slowest-decay-rate must not be 0\n",
> +			np->name);
> +		constraints->slowest_decay_rate = INT_MAX;
> +	} else if (constraints->slowest_decay_rate > INT_MAX) {
> +		pr_err("%s: regulator-slowest-decay-rate (%u) too big\n",
> +			np->name, constraints->slowest_decay_rate);
> +		constraints->slowest_decay_rate = INT_MAX;
> +	}

This should be with the parsing of slowest-decay-rate and checked only
if we have a safe-fall-percent, there's no error if the value is omitted.

> +	if (constraints->safe_fall_percent > 100) {
> +		pr_err("%s: regulator-safe-fall-percent (%u) > 100\n",
> +			np->name, constraints->safe_fall_percent);
> +		constraints->safe_fall_percent = 0;
> +       }

Again indentation is borked here, I think you have tab/space issues.

> +	if (constraints->safe_fall_percent &&
> +		!constraints->slowest_decay_rate) {
> +		pr_err("%s: regulator-slowest-decay-rate requires "
> +			"regulator-safe-fall-percent\n", np->name);

Don't align the second line of the condition with the body of the if,
that just makes things hard to read - do what the rest of the code does
and align with the (.

Don't split text messages over multiple lines, it makes it impossible to
grep for the error as printed.

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

Re: [PATCH v4 4/4] regulator: Prevent falling too fast

[Matthias Kaehlcke]

Hi Mark,

thanks for your review, please find my comments (including info from
our EE) below.

El Mon, Sep 12, 2016 at 07:56:33PM +0100 Mark Brown ha dit:

> On Tue, Sep 06, 2016 at 12:05:24PM -0700, Matthias Kaehlcke wrote:
> 
> > On some boards it's possible that transitioning the PWM regulator
> > downwards too fast will trigger the over voltage protection (OVP) on the
> > regulator.  This is because until the voltage actually falls there is a
> > time when the requested voltage is much lower than the actual voltage.
> 
> So your PWM regulators are not very good and overshooting?  Do you have
> any numbers on this - what values do you end up setting for both the
> delay and safe fall percentages?

No, they just don't actively discharge the rail. Instead, they depend
on the rail self-discharging via the load (the SoC). If the load is
small, it takes longer to transition.

Optimizing the delay time depends on the SoC; we have not measured
this across a wide variety of devices and thus have very conservative
numbers. The percentage is based on the trigger for OVP on the
regulator. In this case, OVP kicks in when the FB node is 20% over
regulation, which is equivalent to a 16% drop in voltage (1/1.2). For
our device safe-fall-percent is set to 16 and slowest-decay-rate is
225.

> > We'll fix this OVP problem by allowing users to specify the maximum
> > voltage that we can safely fall.  Apparently this maximum safe voltage
> > should be specified as a percentage of the current voltage.  The
> > regulator will then break things into separate steps with a delay in
> > between.
> 
> I'd like to see some more thorough analysis than just an "apparently"
> here.  It's making the code more fiddly for something very handwavy.

It's well-understood why it's a percentage. DVS is controlled by
offsetting the FB current, and as above, OVP is based on how far you
are from the FB target.

> > @@ -2753,8 +2753,6 @@ static int _regulator_do_set_voltage(struct regulator_dev *rdev,
> >  	int old_selector = -1;
> >  	int old_uV = _regulator_get_voltage(rdev);
> >  
> > -	trace_regulator_set_voltage(rdev_get_name(rdev), min_uV, max_uV);
> > -
> >  	min_uV += rdev->constraints->uV_offset;
> >  	max_uV += rdev->constraints->uV_offset;
> >  
> > @@ -2842,11 +2840,53 @@ no_delay:
> >  				     (void *)data);
> >  	}
> >  
> > -	trace_regulator_set_voltage_complete(rdev_get_name(rdev), best_val);
> > -
> >  	return ret;
> 
> Let's remove some trace points too because...?  These *are* the actual
> voltage changes in the device, we're just splitting things up into a
> series of transitions.

I wasn't sure whether to keep reporting a single voltage change or
the individual steps. Will leave the trace points at their original
location.

> > +static int _regulator_set_voltage(struct regulator_dev *rdev,
> > +				     int min_uV, int max_uV)
> > +{
> > +	int safe_fall_percent = rdev->constraints->safe_fall_percent;
> > +	int slowest_decay_rate = rdev->constraints->slowest_decay_rate;
> > +	int orig_uV = _regulator_get_voltage(rdev);
> > +	int uV = orig_uV;
> > +	int ret;
> > +
> > +	trace_regulator_set_voltage(rdev_get_name(rdev), min_uV, max_uV);
> > +
> > +	 /* If we're rising or we're falling but don't need to slow; easy */
> > +	if (min_uV >= uV || !safe_fall_percent)
> 
> Indentation is broken, the two lines above don't agree with each other.

Will fix

> > +	ret = of_property_read_u32(np, "regulator-slowest-decay-rate", &pval);
> > +	if (!ret)
> > +		constraints->slowest_decay_rate = pval;
> > +	else
> > +		constraints->slowest_decay_rate = INT_MAX;
> 
> The documentation said this is mandatory if we have a safe fall rate
> specified but the code says it's optional and we'll silently default to
> an absurdly high rate instead (it's odd that a large number in a field
> marked slowest is fast BTW).  Complaining loudly seems better than
> ignoring the error.

Agreed about complaining. Since there isn't really a reasonable
default value it's probably best to change the interface of the
function and return an error in this case.

> > +	/* We use the value as int and as divider; sanity check */
> > +	if (constraints->slowest_decay_rate == 0) {
> > +		pr_err("%s: regulator-slowest-decay-rate must not be 0\n",
> > +			np->name);
> > +		constraints->slowest_decay_rate = INT_MAX;
> > +	} else if (constraints->slowest_decay_rate > INT_MAX) {
> > +		pr_err("%s: regulator-slowest-decay-rate (%u) too big\n",
> > +			np->name, constraints->slowest_decay_rate);
> > +		constraints->slowest_decay_rate = INT_MAX;
> > +	}
> 
> This should be with the parsing of slowest-decay-rate and checked only
> if we have a safe-fall-percent, there's no error if the value is omitted.

Will change

> > +	if (constraints->safe_fall_percent > 100) {
> > +		pr_err("%s: regulator-safe-fall-percent (%u) > 100\n",
> > +			np->name, constraints->safe_fall_percent);
> > +		constraints->safe_fall_percent = 0;
> > +       }
> 
> Again indentation is borked here, I think you have tab/space issues.

Ok, I will keep an eye on it

> > +	if (constraints->safe_fall_percent &&
> > +		!constraints->slowest_decay_rate) {
> > +		pr_err("%s: regulator-slowest-decay-rate requires "
> > +			"regulator-safe-fall-percent\n", np->name);
> 
> Don't align the second line of the condition with the body of the if,
> that just makes things hard to read - do what the rest of the code does
> and align with the (.
> 
> Don't split text messages over multiple lines, it makes it impossible to
> grep for the error as printed.

ok

Re: [PATCH v4 4/4] regulator: Prevent falling too fast

[Mark Brown]

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

On Tue, Sep 13, 2016 at 10:21:40AM -0700, Matthias Kaehlcke wrote:

> Optimizing the delay time depends on the SoC; we have not measured
> this across a wide variety of devices and thus have very conservative
> numbers. The percentage is based on the trigger for OVP on the
> regulator. In this case, OVP kicks in when the FB node is 20% over
> regulation, which is equivalent to a 16% drop in voltage (1/1.2). For
> our device safe-fall-percent is set to 16 and slowest-decay-rate is
> 225.

The obvious question here is how the OVP hardware knows about the new
voltage and why we're bodging this in the regulator core rather than in
the OVP hardware.

> > I'd like to see some more thorough analysis than just an "apparently"
> > here.  It's making the code more fiddly for something very handwavy.

> It's well-understood why it's a percentage. DVS is controlled by
> offsetting the FB current, and as above, OVP is based on how far you
> are from the FB target.

You might think you understand this clearly but nobody reading the
commit message or looking at the code later on is going to be able
do so when your commit message only contains vague handwaving.

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

Re: [PATCH v4 4/4] regulator: Prevent falling too fast

[Matthias Kaehlcke]

Hi Mark,

El Thu, Sep 15, 2016 at 03:39:45PM +0100 Mark Brown ha dit:

> On Tue, Sep 13, 2016 at 10:21:40AM -0700, Matthias Kaehlcke wrote:
> 
> > Optimizing the delay time depends on the SoC; we have not measured
> > this across a wide variety of devices and thus have very conservative
> > numbers. The percentage is based on the trigger for OVP on the
> > regulator. In this case, OVP kicks in when the FB node is 20% over
> > regulation, which is equivalent to a 16% drop in voltage (1/1.2). For
> > our device safe-fall-percent is set to 16 and slowest-decay-rate is
> > 225.
> 
> The obvious question here is how the OVP hardware knows about the new
> voltage and why we're bodging this in the regulator core rather than in
> the OVP hardware.

The OVP hardware is part of the regulator and the regulator is not
notified directly about voltage changes. The regulator transforms the
PWM input into DC output and does the OVP internally with the limits
described above.

> > > I'd like to see some more thorough analysis than just an "apparently"
> > > here.  It's making the code more fiddly for something very handwavy.
> 
> > It's well-understood why it's a percentage. DVS is controlled by
> > offsetting the FB current, and as above, OVP is based on how far you
> > are from the FB target.
> 
> You might think you understand this clearly but nobody reading the
> commit message or looking at the code later on is going to be able
> do so when your commit message only contains vague handwaving.

In case there is a next revision of this patch (i.e. if it is deemed
useful beyond our specific use case) I can include the details in the
commit message of the next revision. Sorry for omitting them
initially, to me it seemed to be very device specific information, but
I understand that these details can be helpful to understand the
context.

Matthias

Re: [PATCH v4 4/4] regulator: Prevent falling too fast

[Mark Brown]

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

On Thu, Sep 15, 2016 at 11:02:23AM -0700, Matthias Kaehlcke wrote:
> El Thu, Sep 15, 2016 at 03:39:45PM +0100 Mark Brown ha dit:

> > The obvious question here is how the OVP hardware knows about the new
> > voltage and why we're bodging this in the regulator core rather than in
> > the OVP hardware.

> The OVP hardware is part of the regulator and the regulator is not
> notified directly about voltage changes. The regulator transforms the
> PWM input into DC output and does the OVP internally with the limits
> described above.

So the PWM is just configuring this external regulator chip (which
doesn't seem to be described in DT...) and that's just incredibly bad at
coping with voltage changes?  It does sound rather like we ought to be
representing this chip directly in case it needs other workarounds.

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

Re: [PATCH v4 4/4] regulator: Prevent falling too fast

[Matthias Kaehlcke]

El Fri, Sep 16, 2016 at 05:32:53PM +0100 Mark Brown ha dit:

> On Thu, Sep 15, 2016 at 11:02:23AM -0700, Matthias Kaehlcke wrote:
> > El Thu, Sep 15, 2016 at 03:39:45PM +0100 Mark Brown ha dit:
> 
> > > The obvious question here is how the OVP hardware knows about the new
> > > voltage and why we're bodging this in the regulator core rather than in
> > > the OVP hardware.
> 
> > The OVP hardware is part of the regulator and the regulator is not
> > notified directly about voltage changes. The regulator transforms the
> > PWM input into DC output and does the OVP internally with the limits
> > described above.
> 
> So the PWM is just configuring this external regulator chip (which
> doesn't seem to be described in DT...)

Exactly

> and that's just incredibly bad at coping with voltage changes?

Supposedly OVP is a feature of the chip, but it gets in our way on
"larger" voltage changes.

> It does sound rather like we ought to be representing this chip
> directly in case it needs other workarounds.

Ok, we'll consider this. It seems we can drop this patch since the
regulator core is not the best place to address this problem.

Thanks for your reviews!

Matthias

Re: [PATCH v4 4/4] regulator: Prevent falling too fast

[Mark Brown]

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

On Fri, Sep 16, 2016 at 11:31:45AM -0700, Matthias Kaehlcke wrote:
> El Fri, Sep 16, 2016 at 05:32:53PM +0100 Mark Brown ha dit:

> > It does sound rather like we ought to be representing this chip
> > directly in case it needs other workarounds.

> Ok, we'll consider this. It seems we can drop this patch since the
> regulator core is not the best place to address this problem.

Perhaps it is, perhaps it isn't - the above is a question about how we
describe this stuff.

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

Re: [PATCH v4 4/4] regulator: Prevent falling too fast

[Doug Anderson]

Hi,

On Fri, Sep 16, 2016 at 9:32 AM, Mark Brown <broonie@kernel.org> wrote:
> On Thu, Sep 15, 2016 at 11:02:23AM -0700, Matthias Kaehlcke wrote:
>> El Thu, Sep 15, 2016 at 03:39:45PM +0100 Mark Brown ha dit:
>
>> > The obvious question here is how the OVP hardware knows about the new
>> > voltage and why we're bodging this in the regulator core rather than in
>> > the OVP hardware.
>
>> The OVP hardware is part of the regulator and the regulator is not
>> notified directly about voltage changes. The regulator transforms the
>> PWM input into DC output and does the OVP internally with the limits
>> described above.
>
> So the PWM is just configuring this external regulator chip (which
> doesn't seem to be described in DT...) and that's just incredibly bad at
> coping with voltage changes?  It does sound rather like we ought to be
> representing this chip directly in case it needs other workarounds.

I'm not 100% sure you can blame the regulator chip.  What we describe
in the device tree as a "PWM Regulator" is actually:

1. Some discreet buck regulator whose output voltage is configured by
adjusting an input voltage.  AKA: the buck regulator has "3" inputs:
vin, vout, config.  You put a certain voltage on the "config" pin and
that controls the value that comes out of "vout".

2. A network of resistors, capacitors, and inductors that take the
output of a PWM and filter / smooth it enough that it can be a good
config input to the discreet buck.


The actual behavior of the PWM regulator depends as much (or more) on
what values you have for the resistors, capacitors, and inductors than
it does on the actual buck.  ...so two people using the same discreet
buck might have very different behaviors in terms of rise time and how
much they are impacted by the over voltage protection.


-Doug

Re: [PATCH v4 4/4] regulator: Prevent falling too fast

[Mark Brown]

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

On Mon, Sep 19, 2016 at 11:39:24AM -0700, Doug Anderson wrote:
> On Fri, Sep 16, 2016 at 9:32 AM, Mark Brown <broonie@kernel.org> wrote:

> > So the PWM is just configuring this external regulator chip (which
> > doesn't seem to be described in DT...) and that's just incredibly bad at
> > coping with voltage changes?  It does sound rather like we ought to be
> > representing this chip directly in case it needs other workarounds.

> I'm not 100% sure you can blame the regulator chip.  What we describe
> in the device tree as a "PWM Regulator" is actually:

> 1. Some discreet buck regulator whose output voltage is configured by
> adjusting an input voltage.  AKA: the buck regulator has "3" inputs:
> vin, vout, config.  You put a certain voltage on the "config" pin and
> that controls the value that comes out of "vout".

> 2. A network of resistors, capacitors, and inductors that take the
> output of a PWM and filter / smooth it enough that it can be a good
> config input to the discreet buck.

Ugh, right.  So you're using the PWM regulator output voltage as an
input to this other regulator.  TBH that sounds even more like this
other regulator should be represented in DT as the consumer of the PWM
regulator, the PWM regulator is not actually producing the voltages
claimed directly.

> The actual behavior of the PWM regulator depends as much (or more) on
> what values you have for the resistors, capacitors, and inductors than
> it does on the actual buck.  ...so two people using the same discreet
> buck might have very different behaviors in terms of rise time and how
> much they are impacted by the over voltage protection.

Right, these are properties of the PWM regulator.  But for some reason
the DCDC is still incapable of understanding it's own transitions and
flags out of spec too easily?  That isn't really a sign of high quality,
but then this does seem like the DCDC is really intended for a fixed
voltage application and is being abused in this system design to scale
dynamically so really it's a badly concieved hardware design I suppose.

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

Re: [PATCH v4 4/4] regulator: Prevent falling too fast

[Doug Anderson]

Hi,

On Sat, Sep 24, 2016 at 11:41 AM, Mark Brown <broonie@kernel.org> wrote:
> On Mon, Sep 19, 2016 at 11:39:24AM -0700, Doug Anderson wrote:
>> On Fri, Sep 16, 2016 at 9:32 AM, Mark Brown <broonie@kernel.org> wrote:
>
>> > So the PWM is just configuring this external regulator chip (which
>> > doesn't seem to be described in DT...) and that's just incredibly bad at
>> > coping with voltage changes?  It does sound rather like we ought to be
>> > representing this chip directly in case it needs other workarounds.
>
>> I'm not 100% sure you can blame the regulator chip.  What we describe
>> in the device tree as a "PWM Regulator" is actually:
>
>> 1. Some discreet buck regulator whose output voltage is configured by
>> adjusting an input voltage.  AKA: the buck regulator has "3" inputs:
>> vin, vout, config.  You put a certain voltage on the "config" pin and
>> that controls the value that comes out of "vout".
>
>> 2. A network of resistors, capacitors, and inductors that take the
>> output of a PWM and filter / smooth it enough that it can be a good
>> config input to the discreet buck.
>
> Ugh, right.  So you're using the PWM regulator output voltage as an
> input to this other regulator.  TBH that sounds even more like this
> other regulator should be represented in DT as the consumer of the PWM
> regulator, the PWM regulator is not actually producing the voltages
> claimed directly.

I guess I think of the whole network of components as the PWM
regulator and not the individual discreet BUCK.  I'm also not quite
sure how you would model it as you're asking.  I suppose you could say
that all of the resistors / capacitors / inductors end up producing a
voltage and this voltage is an input to the BUCK.  ...then the BUCK
looks as the voltage on this control input and uses that to decide how
to convert VIN to VOUT.  That's sorta how I think about it, but I
_think_ it's more intertwined than that.  Looking at the schematics
there are plenty of connections from the output voltage back to the
PWM (through various discreets).  I really think you can't model it as
two distinct things: this whole glom of stuff is one beast.

I know for sure that our EEs have massively modified the behavior of
the whole thing by just changing the resistors / capacitors /
inductors, changing the undershoot, OVP issue, voltage ranges, default
voltage, etc.  That's what leads me to believe it's not so separable.

You could possible include some sort of string indicating what the
model of the BUCK is, but I'm not sure how you would use it at the
moment.


>> The actual behavior of the PWM regulator depends as much (or more) on
>> what values you have for the resistors, capacitors, and inductors than
>> it does on the actual buck.  ...so two people using the same discreet
>> buck might have very different behaviors in terms of rise time and how
>> much they are impacted by the over voltage protection.
>
> Right, these are properties of the PWM regulator.  But for some reason
> the DCDC is still incapable of understanding it's own transitions and
> flags out of spec too easily?  That isn't really a sign of high quality,
> but then this does seem like the DCDC is really intended for a fixed
> voltage application and is being abused in this system design to scale
> dynamically so really it's a badly concieved hardware design I suppose.

Yes, it's not ideal hardware.  :(  I wish we could change it to avoid
this, but at this point I think we're stuck with it.

As I heard it described, the whole PWM regulator concept allows you to
take relatively low cost BUCKs and make them easy to adjust up or down
in software.  It may have its downsides, but if it is inexpensive and
can be made to work by adding a few delays for downward transitions I
have a feeling that people will want to use it.

-Doug

Re: [PATCH v4 4/4] regulator: Prevent falling too fast

[Mark Brown]

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

On Mon, Sep 26, 2016 at 10:41:59AM -0700, Doug Anderson wrote:

> I guess I think of the whole network of components as the PWM
> regulator and not the individual discreet BUCK.  I'm also not quite
> sure how you would model it as you're asking.  I suppose you could say
> that all of the resistors / capacitors / inductors end up producing a
> voltage and this voltage is an input to the BUCK.  ...then the BUCK

Yes, that's what's happening.

> I know for sure that our EEs have massively modified the behavior of
> the whole thing by just changing the resistors / capacitors /
> inductors, changing the undershoot, OVP issue, voltage ranges, default
> voltage, etc.  That's what leads me to believe it's not so separable.

What you're describing to me is a discrete DCDC that has an input
voltage that sets the output voltage which happens to be set with a PWM.
It's of course going to be the case that the passives are important to
the system performance but it seems we have two bits here - the PWM
regulator providing an input to the DCDC and the DCDC itself which is
sensitive to rate changes.

> You could possible include some sort of string indicating what the
> model of the BUCK is, but I'm not sure how you would use it at the
> moment.

Well, the main thing it's apparently doing is providing this over
voltage protection...   That's the bit that seems to warrant being
captured in this separate device.

> As I heard it described, the whole PWM regulator concept allows you to
> take relatively low cost BUCKs and make them easy to adjust up or down
> in software.  It may have its downsides, but if it is inexpensive and
> can be made to work by adding a few delays for downward transitions I
> have a feeling that people will want to use it.

If they were easy to adjust up or down in software there wouldn't be any
issue!  There doesn't seem to be anything PWM specific in the false
positive OVP issue, we could equally imagine someone shoving a regulator
like this on an otherwise unused LDO and experiencing the same problem.
The fact that a PWM is being used to generate the input voltage seems
like just a decision this particular system took to pair a cheap
controllable regualator with a not quite system appropriate high current
regulator, if this pattern does start to get wider use I'd expect to see
other systems using other regulators to set the input voltage.

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

Re: [PATCH v4 4/4] regulator: Prevent falling too fast

[Matthias Kaehlcke]

El Fri, Oct 28, 2016 at 07:15:21PM +0100 Mark Brown ha dit:

> On Mon, Sep 26, 2016 at 10:41:59AM -0700, Doug Anderson wrote:
> 
> > I guess I think of the whole network of components as the PWM
> > regulator and not the individual discreet BUCK.  I'm also not quite
> > sure how you would model it as you're asking.  I suppose you could say
> > that all of the resistors / capacitors / inductors end up producing a
> > voltage and this voltage is an input to the BUCK.  ...then the BUCK
> 
> Yes, that's what's happening.
> 
> > I know for sure that our EEs have massively modified the behavior of
> > the whole thing by just changing the resistors / capacitors /
> > inductors, changing the undershoot, OVP issue, voltage ranges, default
> > voltage, etc.  That's what leads me to believe it's not so separable.
> 
> What you're describing to me is a discrete DCDC that has an input
> voltage that sets the output voltage which happens to be set with a PWM.
> It's of course going to be the case that the passives are important to
> the system performance but it seems we have two bits here - the PWM
> regulator providing an input to the DCDC and the DCDC itself which is
> sensitive to rate changes.

I experimented a bit with this. Besides the question of how to model
the passives I wonder how the two regulators would interact. The
correct thing seems to be to specify the input regulator as a supply
of the DCDC. dcdc->set_voltage breaks down a voltage transition into
steps (if needed) and calls regulator_set_voltage(supply) for each
step. The problem with that is that regulator_set_voltage(dcdc)
acquires the supply lock(s), later regulator_set_voltage(supply) tries
to acquire its own lock which is already held. This can be worked
around by only using the supply regulator in the DCDC, but not
specify it as a supply. However this seems more a hack than a proper
solution.

Am I missing something obvious here or approaching this from a wrong
angle?

Thanks

Matthias

Re: [PATCH v4 4/4] regulator: Prevent falling too fast

[Mark Brown]

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

On Mon, Dec 12, 2016 at 01:15:02PM -0800, Matthias Kaehlcke wrote:
> El Fri, Oct 28, 2016 at 07:15:21PM +0100 Mark Brown ha dit:
> > On Mon, Sep 26, 2016 at 10:41:59AM -0700, Doug Anderson wrote:

> > What you're describing to me is a discrete DCDC that has an input
> > voltage that sets the output voltage which happens to be set with a PWM.

> I experimented a bit with this. Besides the question of how to model
> the passives I wonder how the two regulators would interact. The
> correct thing seems to be to specify the input regulator as a supply
> of the DCDC. dcdc->set_voltage breaks down a voltage transition into

No, not unless the prior descriptions of the hardware have been wildly
inaccurate - my understanding had been that the DCDC was a normal DCDC
with an analogue input intended to be biased to set the output voltage
(presumably in terms of a full rail supply) and that the PWM had been
connected to this analogue input.  If the PWM is supplying the DCDC then 
the hardware design just seems bizzare, I can't see how this would even
work.

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

Re: [PATCH v4 4/4] regulator: Prevent falling too fast

[Doug Anderson]

Hi,

On Tue, Dec 13, 2016 at 9:19 AM, Mark Brown <broonie@kernel.org> wrote:
> On Mon, Dec 12, 2016 at 01:15:02PM -0800, Matthias Kaehlcke wrote:
>> El Fri, Oct 28, 2016 at 07:15:21PM +0100 Mark Brown ha dit:
>> > On Mon, Sep 26, 2016 at 10:41:59AM -0700, Doug Anderson wrote:
>
>> > What you're describing to me is a discrete DCDC that has an input
>> > voltage that sets the output voltage which happens to be set with a PWM.
>
>> I experimented a bit with this. Besides the question of how to model
>> the passives I wonder how the two regulators would interact. The
>> correct thing seems to be to specify the input regulator as a supply
>> of the DCDC. dcdc->set_voltage breaks down a voltage transition into
>
> No, not unless the prior descriptions of the hardware have been wildly
> inaccurate - my understanding had been that the DCDC was a normal DCDC
> with an analogue input intended to be biased to set the output voltage
> (presumably in terms of a full rail supply) and that the PWM had been
> connected to this analogue input.  If the PWM is supplying the DCDC then
> the hardware design just seems bizzare, I can't see how this would even
> work.

Looking at one schematic, the discrete BUCK for at least one of the
rails is TPS65261RHBR, which appears to be described at
<https://store.ti.com/TPS65261RHBR.aspx>.  Data sheet appears to be at
<http://www.ti.com/product/tps65261/technicaldocuments?HQS=TI-null-null-octopart-df-pf-null-wwe>.

As you can see from the datasheet ("Adjusting the Output Voltage"
section), it is intended that you stuff a resistor to make a voltage
divider and that's how you select the output voltage.  In our case the
PWM interacts here and allows you to make a more dynamic output
voltage.  I've always thought about the input to the "FB" pin as
making an input voltage, but I guess it's not terribly simple since
the voltage divider ends up dividing between ground and the output
voltage.

Also as you can see, the datasheet never talks about using a PWM to
control the feedback and as I understand it the BUCK wasn't designed
for this, but it (mostly) works just fine.

...you can also see details about the Over Voltage Protection (at last
for this BUCK) in the TPS65261RHBR datasheet.


-Doug

Re: [PATCH v4 4/4] regulator: Prevent falling too fast

[Matthias Kaehlcke]

Hi,

El Tue, Dec 13, 2016 at 12:00:32PM -0800 Doug Anderson ha dit:

> On Tue, Dec 13, 2016 at 9:19 AM, Mark Brown <broonie@kernel.org> wrote:
> > On Mon, Dec 12, 2016 at 01:15:02PM -0800, Matthias Kaehlcke wrote:
> >> El Fri, Oct 28, 2016 at 07:15:21PM +0100 Mark Brown ha dit:
> >> > On Mon, Sep 26, 2016 at 10:41:59AM -0700, Doug Anderson wrote:
> >
> >> > What you're describing to me is a discrete DCDC that has an input
> >> > voltage that sets the output voltage which happens to be set with a PWM.
> >
> >> I experimented a bit with this. Besides the question of how to model
> >> the passives I wonder how the two regulators would interact. The
> >> correct thing seems to be to specify the input regulator as a supply
> >> of the DCDC. dcdc->set_voltage breaks down a voltage transition into
> >
> > No, not unless the prior descriptions of the hardware have been wildly
> > inaccurate - my understanding had been that the DCDC was a normal DCDC
> > with an analogue input intended to be biased to set the output voltage
> > (presumably in terms of a full rail supply) and that the PWM had been
> > connected to this analogue input.  If the PWM is supplying the DCDC then
> > the hardware design just seems bizzare, I can't see how this would even
> > work.
> 
> Looking at one schematic, the discrete BUCK for at least one of the
> rails is TPS65261RHBR, which appears to be described at
> <https://store.ti.com/TPS65261RHBR.aspx>.  Data sheet appears to be at
> <http://www.ti.com/product/tps65261/technicaldocuments?HQS=TI-null-null-octopart-df-pf-null-wwe>.
> 
> As you can see from the datasheet ("Adjusting the Output Voltage"
> section), it is intended that you stuff a resistor to make a voltage
> divider and that's how you select the output voltage.  In our case the
> PWM interacts here and allows you to make a more dynamic output
> voltage.  I've always thought about the input to the "FB" pin as
> making an input voltage, but I guess it's not terribly simple since
> the voltage divider ends up dividing between ground and the output
> voltage.

I also had put my mind on seeing the output of the PWM circuitry as an
input voltage, but technically it isn't a supply of the buck
regulator. It seems we could consider it a "control voltage" instead
and thus avoid the recursive lock acquisition.

Matthias

Re: [PATCH v4 4/4] regulator: Prevent falling too fast

[Mark Brown]

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

On Tue, Dec 13, 2016 at 12:00:32PM -0800, Doug Anderson wrote:
> On Tue, Dec 13, 2016 at 9:19 AM, Mark Brown <broonie@kernel.org> wrote:

> > No, not unless the prior descriptions of the hardware have been wildly
> > inaccurate - my understanding had been that the DCDC was a normal DCDC
> > with an analogue input intended to be biased to set the output voltage
> > (presumably in terms of a full rail supply) and that the PWM had been
> > connected to this analogue input.  If the PWM is supplying the DCDC then
> > the hardware design just seems bizzare, I can't see how this would even
> > work.

> As you can see from the datasheet ("Adjusting the Output Voltage"
> section), it is intended that you stuff a resistor to make a voltage
> divider and that's how you select the output voltage.  In our case the

OK, that's what I thought was happening.  That's definitely something
that should *not* end up in ->supply, that should hold the parent supply
that the DCDC is regulating down to the target voltage.

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