[PATCH v2] regulator: pwm: Fix regulator ramp delay for continuous mode

[PATCH v2] regulator: pwm: Fix regulator ramp delay for continuous mode

[Douglas Anderson]

The original commit adding support for continuous voltage mode didn't
handle the regulator ramp delay properly.  It treated the delay as a
fixed delay in uS despite the property being defined as uV / uS.  Let's
adjust it.  Luckily there appear to be no users of this ramp delay for
PWM regulators (as per grepping through device trees in linuxnext).

Note also that the upper bound of usleep_range probably shouldn't be a
full 1 ms longer than the lower bound since I've seen plenty of hardware
with a ramp rate of ~5000 uS / uV and for small jumps the total delays
are in the tens of uS.  1000 is way too much.  We'll try to be dynamic
and use 10%.

NOTE: This commit doesn't add support for regulator-enable-ramp-delay.
That could be done in a future patch when someone has a user of that
featre.

Though this patch is shows as "fixing" a bug, there are no actual known
users of continuous mode PWM regulator w/ ramp delay in mainline and so
this likely won't have any effect on anyone unless they are working
out-of-tree with private patches.  For anyone in this state, it is
highly encouraged to also pick Boris Brezillon's WIP patches to get
yourself a reliable and glitch-free regulator.

Fixes: 4773be185a0f ("regulator: pwm-regulator: Add support for continuous-voltage")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
Note: I don't have a board that works against mainline and can use the
PWM regulator in continuous mode without Boris's patches.  That means
that this patch has only been compile tested.  The previous version of
the patches (atop Boris's changes) was tested more thoroughly.  YMMV.

Changes in v2:
- No longer atop Boris PWM regulator fixes (Mark).
- Don't delay if the regulator is not enabled (Boris).

 drivers/regulator/pwm-regulator.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c
index ab3cc0235843..75d9f9b1ad62 100644
--- a/drivers/regulator/pwm-regulator.c
+++ b/drivers/regulator/pwm-regulator.c
@@ -132,6 +132,7 @@ static int pwm_regulator_set_voltage(struct regulator_dev *rdev,
 	unsigned int duty_pulse;
 	u64 req_period;
 	u32 rem;
+	int old_uV = pwm_regulator_get_voltage(rdev);
 	int ret;
 
 	pwm_get_args(drvdata->pwm, &pargs);
@@ -161,8 +162,12 @@ static int pwm_regulator_set_voltage(struct regulator_dev *rdev,
 
 	drvdata->volt_uV = min_uV;
 
-	/* Delay required by PWM regulator to settle to the new voltage */
-	usleep_range(ramp_delay, ramp_delay + 1000);
+	if ((ramp_delay == 0) || !pwm_regulator_is_enabled(rdev))
+		return 0;
+
+	/* Ramp delay is in uV/uS. Adjust to uS and delay */
+	ramp_delay = DIV_ROUND_UP(abs(min_uV - old_uV), ramp_delay);
+	usleep_range(ramp_delay, ramp_delay + DIV_ROUND_UP(ramp_delay, 10));
 
 	return 0;
 }
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH v2] regulator: pwm: Fix regulator ramp delay for continuous mode

[Laxman Dewangan]

On Thursday 07 July 2016 12:12 AM, Douglas Anderson wrote:
> The original commit adding support for continuous voltage mode didn't
> handle the regulator ramp delay properly.  It treated the delay as a
> fixed delay in uS despite the property being defined as uV / uS.  Let's
> adjust it.  Luckily there appear to be no users of this ramp delay for
> PWM regulators (as per grepping through device trees in linuxnext).
>
> Note also that the upper bound of usleep_range probably shouldn't be a
> full 1 ms longer than the lower bound since I've seen plenty of hardware
> with a ramp rate of ~5000 uS / uV and for small jumps the total delays
> are in the tens of uS.  1000 is way too much.  We'll try to be dynamic
> and use 10%.
>
> NOTE: This commit doesn't add support for regulator-enable-ramp-delay.
> That could be done in a future patch when someone has a user of that
> featre.
>
> Though this patch is shows as "fixing" a bug, there are no actual known
> users of continuous mode PWM regulator w/ ramp delay in mainline and so
> this likely won't have any effect on anyone unless they are working
> out-of-tree with private patches.  For anyone in this state, it is
> highly encouraged to also pick Boris Brezillon's WIP patches to get
> yourself a reliable and glitch-free regulator.
>
> Fixes: 4773be185a0f ("regulator: pwm-regulator: Add support for continuous-voltage")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>


Looks fine here.
Acked-by: Laxman Dewangan <ldewangan@nvidia.com>

BTW, for some PWM regulator, the settling time for voltage change is 
same for any steps.

Re: [PATCH v2] regulator: pwm: Fix regulator ramp delay for continuous mode

[Doug Anderson]

Hi,

On Thu, Jul 7, 2016 at 1:36 AM, Laxman Dewangan <ldewangan@nvidia.com> wrote:
>
> On Thursday 07 July 2016 12:12 AM, Douglas Anderson wrote:
>>
>> The original commit adding support for continuous voltage mode didn't
>> handle the regulator ramp delay properly.  It treated the delay as a
>> fixed delay in uS despite the property being defined as uV / uS.  Let's
>> adjust it.  Luckily there appear to be no users of this ramp delay for
>> PWM regulators (as per grepping through device trees in linuxnext).
>>
>> Note also that the upper bound of usleep_range probably shouldn't be a
>> full 1 ms longer than the lower bound since I've seen plenty of hardware
>> with a ramp rate of ~5000 uS / uV and for small jumps the total delays
>> are in the tens of uS.  1000 is way too much.  We'll try to be dynamic
>> and use 10%.
>>
>> NOTE: This commit doesn't add support for regulator-enable-ramp-delay.
>> That could be done in a future patch when someone has a user of that
>> featre.
>>
>> Though this patch is shows as "fixing" a bug, there are no actual known
>> users of continuous mode PWM regulator w/ ramp delay in mainline and so
>> this likely won't have any effect on anyone unless they are working
>> out-of-tree with private patches.  For anyone in this state, it is
>> highly encouraged to also pick Boris Brezillon's WIP patches to get
>> yourself a reliable and glitch-free regulator.
>>
>> Fixes: 4773be185a0f ("regulator: pwm-regulator: Add support for
>> continuous-voltage")
>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>
>
>
> Looks fine here.
> Acked-by: Laxman Dewangan <ldewangan@nvidia.com>
>
> BTW, for some PWM regulator, the settling time for voltage change is same
> for any steps.

In that case we should probably add a new PWM regulator property and
not abuse the existing one.  Maybe you use "pwm-regulator-settle-us"
or something?  ...and actually the right thing is probably to
implement 'regulator-ramp-delay' as doing several small steps in that
case.  So if you've got:

* settle time: 10 us
* ramp delay = 5000 uV / us
* voltage change of 200000 uV

In that case you'd probably want to break your 200 mV voltage change
into a few steps?  So you could do bump by 50mV, delay 10us, bump by
50mV, delay 10us, bump by 50mV, delay by 10us.

The final delay would be the same as with my patch applied but you'd
get there more smoothly.

-Doug

RE: [PATCH v2] regulator: pwm: Fix regulator ramp delay for continuous mode

[Aleksandr Frid]

Hi,

>>
In that case we should probably add a new PWM regulator property and not abuse the existing one.  Maybe you use "pwm-regulator-settle-us"
or something?  
>>
Looks reasonable to me.

>>
actually the right thing is probably to implement 'regulator-ramp-delay' as doing several small steps in that case
>>
Ramp delay uV/us  is not a "real" metric for some PWM regulators with exponential transition -- as opposite to fixed slew-rate linear transition on other regulators. So splitting transition into multiple steps to implement artificial  (in this case) metric seems questionable. 

-----Original Message-----
From: dianders@google.com [mailto:dianders@google.com] On Behalf Of Doug Anderson
Sent: Thursday, July 07, 2016 9:31 AM
To: Laxman Dewangan
Cc: Mark Brown; Boris Brezillon; Lee Jones; Brian Norris; open list:ARM/Rockchip SoC...; Heiko Stuebner; Thierry Reding; Liam Girdwood; linux-kernel@vger.kernel.org; Aleksandr Frid
Subject: Re: [PATCH v2] regulator: pwm: Fix regulator ramp delay for continuous mode

Hi,

On Thu, Jul 7, 2016 at 1:36 AM, Laxman Dewangan <ldewangan@nvidia.com> wrote:
>
> On Thursday 07 July 2016 12:12 AM, Douglas Anderson wrote:
>>
>> The original commit adding support for continuous voltage mode didn't 
>> handle the regulator ramp delay properly.  It treated the delay as a 
>> fixed delay in uS despite the property being defined as uV / uS.  
>> Let's adjust it.  Luckily there appear to be no users of this ramp 
>> delay for PWM regulators (as per grepping through device trees in linuxnext).
>>
>> Note also that the upper bound of usleep_range probably shouldn't be 
>> a full 1 ms longer than the lower bound since I've seen plenty of 
>> hardware with a ramp rate of ~5000 uS / uV and for small jumps the 
>> total delays are in the tens of uS.  1000 is way too much.  We'll try 
>> to be dynamic and use 10%.
>>
>> NOTE: This commit doesn't add support for regulator-enable-ramp-delay.
>> That could be done in a future patch when someone has a user of that 
>> featre.
>>
>> Though this patch is shows as "fixing" a bug, there are no actual 
>> known users of continuous mode PWM regulator w/ ramp delay in 
>> mainline and so this likely won't have any effect on anyone unless 
>> they are working out-of-tree with private patches.  For anyone in 
>> this state, it is highly encouraged to also pick Boris Brezillon's 
>> WIP patches to get yourself a reliable and glitch-free regulator.
>>
>> Fixes: 4773be185a0f ("regulator: pwm-regulator: Add support for
>> continuous-voltage")
>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>
>
>
> Looks fine here.
> Acked-by: Laxman Dewangan <ldewangan@nvidia.com>
>
> BTW, for some PWM regulator, the settling time for voltage change is 
> same for any steps.

In that case we should probably add a new PWM regulator property and not abuse the existing one.  Maybe you use "pwm-regulator-settle-us"
or something?  ...and actually the right thing is probably to implement 'regulator-ramp-delay' as doing several small steps in that case.  So if you've got:

* settle time: 10 us
* ramp delay = 5000 uV / us
* voltage change of 200000 uV

In that case you'd probably want to break your 200 mV voltage change into a few steps?  So you could do bump by 50mV, delay 10us, bump by 50mV, delay 10us, bump by 50mV, delay by 10us.

The final delay would be the same as with my patch applied but you'd get there more smoothly.

-Doug

Re: [PATCH v2] regulator: pwm: Fix regulator ramp delay for continuous mode

[Doug Anderson]

Hi,

On Thu, Jul 7, 2016 at 11:23 AM, Aleksandr Frid <afrid@nvidia.com> wrote:
> Hi,
>
>>>
> In that case we should probably add a new PWM regulator property and not abuse the existing one.  Maybe you use "pwm-regulator-settle-us"
> or something?
>>>
> Looks reasonable to me.
>
>>>
> actually the right thing is probably to implement 'regulator-ramp-delay' as doing several small steps in that case
>>>
> Ramp delay uV/us  is not a "real" metric for some PWM regulators with exponential transition -- as opposite to fixed slew-rate linear transition on other regulators. So splitting transition into multiple steps to implement artificial  (in this case) metric seems questionable.

In the case that you don't need multiple steps then don't specify a
regulator-ramp-delay, just a pwm-regulator-settle-us.

...the suggestion for multiple steps is because (so I'm told) it helps
avoid overshoot or undershoot problems.  In general the whole point of
ramping a regulator slowly is to avoid overshoot or undershoot
problems.  The "regulator-ramp-delay" property in Linux is a little
odd because it sort of "describes" the ramp delay and sort of "sets"
the ramp delay.  Many PMICs allow you to set how fast the regulator
will ramp and this property is used to specify how the register in the
PMIC should be set.  However, it is also used as the actual delay in
Linux.


-Doug

RE: [PATCH v2] regulator: pwm: Fix regulator ramp delay for continuous mode

[Aleksandr Frid]

>>
In the case that you don't need multiple steps then don't specify a regulator-ramp-delay, just a pwm-regulator-settle-us.
>>
Agreed

-----Original Message-----
From: dianders@google.com [mailto:dianders@google.com] On Behalf Of Doug Anderson
Sent: Thursday, July 07, 2016 11:32 AM
To: Aleksandr Frid
Cc: Laxman Dewangan; Mark Brown; Boris Brezillon; Lee Jones; Brian Norris; open list:ARM/Rockchip SoC...; Heiko Stuebner; Thierry Reding; Liam Girdwood; linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] regulator: pwm: Fix regulator ramp delay for continuous mode

Hi,

On Thu, Jul 7, 2016 at 11:23 AM, Aleksandr Frid <afrid@nvidia.com> wrote:
> Hi,
>
>>>
> In that case we should probably add a new PWM regulator property and not abuse the existing one.  Maybe you use "pwm-regulator-settle-us"
> or something?
>>>
> Looks reasonable to me.
>
>>>
> actually the right thing is probably to implement 
> 'regulator-ramp-delay' as doing several small steps in that case
>>>
> Ramp delay uV/us  is not a "real" metric for some PWM regulators with exponential transition -- as opposite to fixed slew-rate linear transition on other regulators. So splitting transition into multiple steps to implement artificial  (in this case) metric seems questionable.

In the case that you don't need multiple steps then don't specify a regulator-ramp-delay, just a pwm-regulator-settle-us.

...the suggestion for multiple steps is because (so I'm told) it helps avoid overshoot or undershoot problems.  In general the whole point of ramping a regulator slowly is to avoid overshoot or undershoot problems.  The "regulator-ramp-delay" property in Linux is a little odd because it sort of "describes" the ramp delay and sort of "sets"
the ramp delay.  Many PMICs allow you to set how fast the regulator will ramp and this property is used to specify how the register in the PMIC should be set.  However, it is also used as the actual delay in Linux.


-Doug

Re: [PATCH v2] regulator: pwm: Fix regulator ramp delay for continuous mode

[Mark Brown]

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

On Thu, Jul 07, 2016 at 06:43:33PM +0000, Aleksandr Frid wrote:

I'm not entirely sure what's wrong with your mail client here but your
mails are essentially illegible.  There appears to be some combination
of top posting, reflowing quoted content to remove line breaks and extra
levels of quoting.

> >>
> In the case that you don't need multiple steps then don't specify a regulator-ramp-delay, just a pwm-regulator-settle-us.
> >>
> Agreed
> 
> -----Original Message-----
> From: dianders@google.com [mailto:dianders@google.com] On Behalf Of Doug Anderson
> Sent: Thursday, July 07, 2016 11:32 AM
> To: Aleksandr Frid
> Cc: Laxman Dewangan; Mark Brown; Boris Brezillon; Lee Jones; Brian Norris; open list:ARM/Rockchip SoC...; Heiko Stuebner; Thierry Reding; Liam Girdwood; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v2] regulator: pwm: Fix regulator ramp delay for continuous mode
> 
> Hi,
> 
> On Thu, Jul 7, 2016 at 11:23 AM, Aleksandr Frid <afrid@nvidia.com> wrote:
> > Hi,
> >
> >>>
> > In that case we should probably add a new PWM regulator property and not abuse the existing one.  Maybe you use "pwm-regulator-settle-us"
> > or something?
> >>>
> > Looks reasonable to me.
> >
> >>>
> > actually the right thing is probably to implement 
> > 'regulator-ramp-delay' as doing several small steps in that case
> >>>
> > Ramp delay uV/us  is not a "real" metric for some PWM regulators with exponential transition -- as opposite to fixed slew-rate linear transition on other regulators. So splitting transition into multiple steps to implement artificial  (in this case) metric seems questionable.
> 
> In the case that you don't need multiple steps then don't specify a regulator-ramp-delay, just a pwm-regulator-settle-us.
> 
> ...the suggestion for multiple steps is because (so I'm told) it helps avoid overshoot or undershoot problems.  In general the whole point of ramping a regulator slowly is to avoid overshoot or undershoot problems.  The "regulator-ramp-delay" property in Linux is a little odd because it sort of "describes" the ramp delay and sort of "sets"
> the ramp delay.  Many PMICs allow you to set how fast the regulator will ramp and this property is used to specify how the register in the PMIC should be set.  However, it is also used as the actual delay in Linux.
> 
> 
> -Doug

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

Applied "regulator: pwm: Fix regulator ramp delay for continuous mode" to the regulator tree

[Mark Brown]

The patch

   regulator: pwm: Fix regulator ramp delay for continuous mode

has been applied to the regulator tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From c2588393e6315ab68207323d37d2a73713d6bc81 Mon Sep 17 00:00:00 2001
From: Douglas Anderson <dianders@chromium.org>
Date: Wed, 6 Jul 2016 11:42:01 -0700
Subject: [PATCH] regulator: pwm: Fix regulator ramp delay for continuous mode

The original commit adding support for continuous voltage mode didn't
handle the regulator ramp delay properly.  It treated the delay as a
fixed delay in uS despite the property being defined as uV / uS.  Let's
adjust it.  Luckily there appear to be no users of this ramp delay for
PWM regulators (as per grepping through device trees in linuxnext).

Note also that the upper bound of usleep_range probably shouldn't be a
full 1 ms longer than the lower bound since I've seen plenty of hardware
with a ramp rate of ~5000 uS / uV and for small jumps the total delays
are in the tens of uS.  1000 is way too much.  We'll try to be dynamic
and use 10%.

NOTE: This commit doesn't add support for regulator-enable-ramp-delay.
That could be done in a future patch when someone has a user of that
featre.

Though this patch is shows as "fixing" a bug, there are no actual known
users of continuous mode PWM regulator w/ ramp delay in mainline and so
this likely won't have any effect on anyone unless they are working
out-of-tree with private patches.  For anyone in this state, it is
highly encouraged to also pick Boris Brezillon's WIP patches to get
yourself a reliable and glitch-free regulator.

Fixes: 4773be185a0f ("regulator: pwm-regulator: Add support for continuous-voltage")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Acked-by: Laxman Dewangan <ldewangan@nvidia.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/regulator/pwm-regulator.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c
index 90f8b7fd0437..666bc3bb52ef 100644
--- a/drivers/regulator/pwm-regulator.c
+++ b/drivers/regulator/pwm-regulator.c
@@ -145,6 +145,7 @@ static int pwm_regulator_set_voltage(struct regulator_dev *rdev,
 	unsigned int duty_pulse;
 	u64 req_period;
 	u32 rem;
+	int old_uV = pwm_regulator_get_voltage(rdev);
 	int ret;
 
 	pwm_get_args(drvdata->pwm, &pargs);
@@ -174,8 +175,12 @@ static int pwm_regulator_set_voltage(struct regulator_dev *rdev,
 
 	drvdata->volt_uV = min_uV;
 
-	/* Delay required by PWM regulator to settle to the new voltage */
-	usleep_range(ramp_delay, ramp_delay + 1000);
+	if ((ramp_delay == 0) || !pwm_regulator_is_enabled(rdev))
+		return 0;
+
+	/* Ramp delay is in uV/uS. Adjust to uS and delay */
+	ramp_delay = DIV_ROUND_UP(abs(min_uV - old_uV), ramp_delay);
+	usleep_range(ramp_delay, ramp_delay + DIV_ROUND_UP(ramp_delay, 10));
 
 	return 0;
 }
-- 
2.8.1