Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1481841 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v4 4/4] regulator: Prevent falling too fast Mark Brown <broonie@kernel.org> - 2016-09-12 21:00 +0200
    Re: [PATCH v4 4/4] regulator: Prevent falling too fast Matthias Kaehlcke <mka@chromium.org> - 2016-09-13 19:30 +0200
      Re: [PATCH v4 4/4] regulator: Prevent falling too fast Mark Brown <broonie@kernel.org> - 2016-09-15 16:50 +0200
        Re: [PATCH v4 4/4] regulator: Prevent falling too fast Matthias Kaehlcke <mka@chromium.org> - 2016-09-15 20:10 +0200
          Re: [PATCH v4 4/4] regulator: Prevent falling too fast Mark Brown <broonie@kernel.org> - 2016-09-16 18:40 +0200
            Re: [PATCH v4 4/4] regulator: Prevent falling too fast Matthias Kaehlcke <mka@chromium.org> - 2016-09-16 20:40 +0200
              Re: [PATCH v4 4/4] regulator: Prevent falling too fast Mark Brown <broonie@kernel.org> - 2016-09-16 20:50 +0200
            Re: [PATCH v4 4/4] regulator: Prevent falling too fast Doug Anderson <dianders@chromium.org> - 2016-09-19 20:40 +0200

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

[Multipart message — attachments visible in raw view] — view raw

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.

[toc] | [next] | [standalone]

#1482663

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

[toc] | [prev] | [next] | [standalone]

#1484262

[Multipart message — attachments visible in raw view] — view raw

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.

[toc] | [prev] | [next] | [standalone]

#1484447

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

[toc] | [prev] | [next] | [standalone]

#1485167

[Multipart message — attachments visible in raw view] — view raw

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.

[toc] | [prev] | [next] | [standalone]

#1485235

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

[toc] | [prev] | [next] | [standalone]

#1485243

[Multipart message — attachments visible in raw view] — view raw

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.

[toc] | [prev] | [next] | [standalone]

#1486751

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web