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

[PATCH] 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%

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
Note that this patch is atop Boris's recent PWM regulator fixes.  If
desired it wouldn't be too hard to write it atop the old code, though
quite honestly anyone using a PWM regulator should probably be using his
new code.

 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 fa1c74c77bb0..de94d19f6e1f 100644
--- a/drivers/regulator/pwm-regulator.c
+++ b/drivers/regulator/pwm-regulator.c
@@ -188,6 +188,7 @@ static int pwm_regulator_set_voltage(struct regulator_dev *rdev,
 	struct pwm_state pstate;
 	unsigned int diff_duty;
 	unsigned int dutycycle;
+	int old_uV = pwm_regulator_get_voltage(rdev);
 	int ret;
 
 	pwm_init_state(drvdata->pwm, &pstate);
@@ -219,8 +220,12 @@ static int pwm_regulator_set_voltage(struct regulator_dev *rdev,
 		return ret;
 	}
 
-	/* Delay required by PWM regulator to settle to the new voltage */
-	usleep_range(ramp_delay, ramp_delay + 1000);
+	if (ramp_delay == 0)
+		return 0;
+
+	/* Ramp delay is in uV/uS. Adjust to uS and delay */
+	ramp_delay = DIV_ROUND_UP(abs(req_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] regulator: pwm: Fix regulator ramp delay for continuous mode

[Caesar Wang]

On 2016年06月28日 12:53, 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%

I'm agree with the dynamic and use 10%.

>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Tested-by: Caesar Wang <wxt@rock-chips.com>

Tested for my rk3399 board. That's still happy work for my board.
..
[ 2891.541958] pwm_regulator_set_voltage: delay=38, min-v=800000, 
old-v=1024000
[ 2898.188785] pwm_regulator_set_voltage: delay=13, min-v=875000, 
old-v=800000
[ 2898.211873] pwm_regulator_set_voltage: delay=8, min-v=925000, 
old-v=877000
[ 2898.312026] pwm_regulator_set_voltage: delay=21, min-v=-800000, 
old-v=926000
..



> ---
> Note that this patch is atop Boris's recent PWM regulator fixes.  If
> desired it wouldn't be too hard to write it atop the old code, though
> quite honestly anyone using a PWM regulator should probably be using his
> new code.
>
>   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 fa1c74c77bb0..de94d19f6e1f 100644
> --- a/drivers/regulator/pwm-regulator.c
> +++ b/drivers/regulator/pwm-regulator.c
> @@ -188,6 +188,7 @@ static int pwm_regulator_set_voltage(struct regulator_dev *rdev,
>   	struct pwm_state pstate;
>   	unsigned int diff_duty;
>   	unsigned int dutycycle;
> +	int old_uV = pwm_regulator_get_voltage(rdev);
>   	int ret;
>   
>   	pwm_init_state(drvdata->pwm, &pstate);
> @@ -219,8 +220,12 @@ static int pwm_regulator_set_voltage(struct regulator_dev *rdev,
>   		return ret;
>   	}
>   
> -	/* Delay required by PWM regulator to settle to the new voltage */
> -	usleep_range(ramp_delay, ramp_delay + 1000);
> +	if (ramp_delay == 0)
> +		return 0;
> +
> +	/* Ramp delay is in uV/uS. Adjust to uS and delay */
> +	ramp_delay = DIV_ROUND_UP(abs(req_min_uV - old_uV), ramp_delay);
> +	usleep_range(ramp_delay, ramp_delay + DIV_ROUND_UP(ramp_delay, 10));
>   
>   	return 0;
>   }

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

[Brian Norris]

On Mon, Jun 27, 2016 at 9:53 PM, Douglas Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> 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).

My grepping agrees, though I'm sure I didn't do a very thorough job.

> 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%
>
> Signed-off-by: Douglas Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> ---
> Note that this patch is atop Boris's recent PWM regulator fixes.  If
> desired it wouldn't be too hard to write it atop the old code, though
> quite honestly anyone using a PWM regulator should probably be using his
> new code.
>
>  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 fa1c74c77bb0..de94d19f6e1f 100644
> --- a/drivers/regulator/pwm-regulator.c
> +++ b/drivers/regulator/pwm-regulator.c
> @@ -188,6 +188,7 @@ static int pwm_regulator_set_voltage(struct regulator_dev *rdev,
>         struct pwm_state pstate;
>         unsigned int diff_duty;
>         unsigned int dutycycle;
> +       int old_uV = pwm_regulator_get_voltage(rdev);
>         int ret;
>
>         pwm_init_state(drvdata->pwm, &pstate);
> @@ -219,8 +220,12 @@ static int pwm_regulator_set_voltage(struct regulator_dev *rdev,
>                 return ret;
>         }
>
> -       /* Delay required by PWM regulator to settle to the new voltage */
> -       usleep_range(ramp_delay, ramp_delay + 1000);

I was curious about the side effects of the unconditional
usleep_range(ramp_delay, ...), even when ramp_delay is 0 (e.g., would
we ever sleep here for up to 1ms?), but apparently the implementation
optimizes the '0' case to be an unconditional, immediate wake-up. So
refactoring this shouldn't have any accidental effects.

> +       if (ramp_delay == 0)
> +               return 0;
> +
> +       /* Ramp delay is in uV/uS. Adjust to uS and delay */
> +       ramp_delay = DIV_ROUND_UP(abs(req_min_uV - old_uV), ramp_delay);
> +       usleep_range(ramp_delay, ramp_delay + DIV_ROUND_UP(ramp_delay, 10));

Math checks out to me.

>
>         return 0;
>  }

Reviewed-by: Brian Norris <briannorris-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

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

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 493 bytes --]

On Mon, Jun 27, 2016 at 09:53:11PM -0700, Douglas Anderson wrote:

> Note that this patch is atop Boris's recent PWM regulator fixes.  If
> desired it wouldn't be too hard to write it atop the old code, though
> quite honestly anyone using a PWM regulator should probably be using his
> new code.

Oh, and the other thing I meant to say there: is that going anywhere?
It seems like the underlying PWM interface is still unclear for some
reason and there's been no comment on those bits of v3.

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

[-- Attachment #2: Type: text/plain, Size: 200 bytes --]

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

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

[Doug Anderson]

Hi,

On Tue, Jun 28, 2016 at 11:56 AM, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Mon, Jun 27, 2016 at 09:53:11PM -0700, Douglas Anderson wrote:
>
>> Note that this patch is atop Boris's recent PWM regulator fixes.  If
>> desired it wouldn't be too hard to write it atop the old code, though
>> quite honestly anyone using a PWM regulator should probably be using his
>> new code.
>
> Oh, and the other thing I meant to say there: is that going anywhere?
> It seems like the underlying PWM interface is still unclear for some
> reason and there's been no comment on those bits of v3.

As far as I know it is going somewhere, but it's not moving terribly
quickly.  At some point I think Boris got tired of posting huge patch
sets and so split things in two.  The first half did eventually land,
but the second half is still outstanding and things seem quiet fro the
last two weeks.  From patchwork we have these outstanding:

9175319  [v3,01/14] pwm: Add an helper to prepare a new PWM state
9175345  [v3,02/14] pwm: Add two helpers to ease relative duty cycle ...
9175361  [v3,03/14] pwm: rockchip: Fix period and duty_cycle approximation
9175341  [v3,04/14] pwm: rockchip: Add support for hardware readout
9175335  [v3,05/14] pwm: rockchip: Avoid glitches on already running PWMs
9175337  [v3,06/14] pwm: rockchip: Add support for atomic update
9175265  [v3,07/14] pwm: sti: Add support for hardware readout
9175317  [v3,08/14] pwm: sti: Avoid glitches on already running PWMs
9175323  [v3,09/14] regulator: pwm: Adjust PWM config at probe time
9175305  [v3,10/14] regulator: pwm: Switch to the atomic PWM API
9175295  [v3,11/14] regulator: pwm: Properly initialize the ->state field
9175267  [v3,12/14] regulator: pwm: Retrieve correct voltage
9175279  [v3,13/14] regulator: pwm: Support extra continuous mode cases
9175273  [v3,14/14] regulator: pwm: Document pwm-dutycycle-unit and ...

-Doug

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

[Mark Brown]

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

On Mon, Jun 27, 2016 at 09:53:11PM -0700, Douglas Anderson wrote:

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

Surely the upper bound here is just an upper bound and we're essentially
just saying that "anything over minimum is fine" here?  Though now I
look at the implementation it seems it's doing something entirely
unehelpful and actually trying to delay for the longest possible time
which doesn't seem like what we want or what the usleep_range() API
would suggest :(

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

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

[Doug Anderson]

Mark,

On Tue, Jun 28, 2016 at 11:52 AM, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Mon, Jun 27, 2016 at 09:53:11PM -0700, Douglas Anderson wrote:
>
>> 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%
>
> Surely the upper bound here is just an upper bound and we're essentially
> just saying that "anything over minimum is fine" here?  Though now I
> look at the implementation it seems it's doing something entirely
> unehelpful and actually trying to delay for the longest possible time
> which doesn't seem like what we want or what the usleep_range() API
> would suggest :(

Yeah, you're thinking like someone who wants your computer to perform
fast.  That's just simply the wrong mentality to have (how dare you?).
You need to think about power savings, and a sleeping computer is a
computer that doesn't draw much power.  Thus, you want to sleep as
long as you can.  :-P

In all seriousness, I think the design for usleep_range() is to try to
batch up wakeups to allow longer periods of sleeping and to optimize
power.  This you want to sleep as long as is allowable and then if you
happen to wakeup for some other reason anyway then you process all the
things whose minimum has passed.  IIRC it was trying to get back to
the good old days of only having jiffies where everyone was
synchronized on the tick and you could sleep till the next tick after
all the work was done.  When you think of it this way then sleeping to
the max makes sense.  ...but it also means that you need to be careful
and really not set the max too high.

Of course, in practice I've found that often usleep_range() 99% of the
time sleeps for the max.  That can lead to some very subtle bugs if
your min sleep is not long enough (!).


-Doug

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

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1400 bytes --]

On Tue, Jun 28, 2016 at 12:31:19PM -0700, Doug Anderson wrote:

> In all seriousness, I think the design for usleep_range() is to try to
> batch up wakeups to allow longer periods of sleeping and to optimize
> power.  This you want to sleep as long as is allowable and then if you
> happen to wakeup for some other reason anyway then you process all the
> things whose minimum has passed.  IIRC it was trying to get back to
> the good old days of only having jiffies where everyone was
> synchronized on the tick and you could sleep till the next tick after
> all the work was done.  When you think of it this way then sleeping to
> the max makes sense.  ...but it also means that you need to be careful
> and really not set the max too high.

That's what I *thought* it was doing, but when I looked at the
implementation down to the hrtimer level it was explicitly saying it was
actively trying to deliver the maximum delay but might do less.  I'd
have expected that the implementation would check to see if there was a
wakeup scheduled in the range, piggybacking on the first one if so, and 
if there was nothing it'd go for the minimum (perhaps with a bit of
rounding).  Oh well.

> Of course, in practice I've found that often usleep_range() 99% of the
> time sleeps for the max.  That can lead to some very subtle bugs if
> your min sleep is not long enough (!).

Yup.

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

[-- Attachment #2: Type: text/plain, Size: 200 bytes --]

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

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

[Mark Brown]

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

On Mon, Jun 27, 2016 at 09:53:11PM -0700, Douglas Anderson wrote:

> Note that this patch is atop Boris's recent PWM regulator fixes.  If
> desired it wouldn't be too hard to write it atop the old code, though
> quite honestly anyone using a PWM regulator should probably be using his
> new code.

Given that those don't seem likely to go in any time soon and aren't
going to go in as a fix can you please respin against current code?

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

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

[Doug Anderson]

Mark,

On Fri, Jul 1, 2016 at 1:06 AM, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Mon, Jun 27, 2016 at 09:53:11PM -0700, Douglas Anderson wrote:
>
>> Note that this patch is atop Boris's recent PWM regulator fixes.  If
>> desired it wouldn't be too hard to write it atop the old code, though
>> quite honestly anyone using a PWM regulator should probably be using his
>> new code.
>
> Given that those don't seem likely to go in any time soon and aren't
> going to go in as a fix can you please respin against current code?

I went to write this patch and it was pretty easy to write.  ...but
then I realized that I had no real way to test it.  I can compile test
it, but that's it.  Do you still want it?

Specifically without Boris's patches then I don't believe the devices
I have access to are bootable when I tell Linux about the PWM
regulator, especially when I configure the PWM regulator to use
continuous mode (which is where the bug is).  As I understand from
<https://patchwork.kernel.org/patch/9175279/> the current PWM
regulator assumes that 0% duty cycle is the lowest voltage and 100% is
the highest voltage.  My system is backwards (0% is highest, 100%
lowest).  Also: last I tried even the non-continuous mode without
Boris's patches the regulator would glitch enough at bootup that my
system would crash (the PWM regulator affects the memory controller).

Anyway, let me know if you want me to post the untested, rebased
patch...  Personally I'd prefer to wait for Boris's patches and the
land the tested version.

Boris: any idea for what the next steps on your series are?


-Doug

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

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 367 bytes --]

On Fri, Jul 01, 2016 at 01:17:48PM -0700, Doug Anderson wrote:

> Anyway, let me know if you want me to post the untested, rebased
> patch...  Personally I'd prefer to wait for Boris's patches and the
> land the tested version.

We've got some boards in kernelci.org using PWM regulators I think, or
perhaps someone else can test.  It does seem like a real fix so...

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

[-- Attachment #2: Type: text/plain, Size: 200 bytes --]

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

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

[Doug Anderson]

Mark,

On Sat, Jul 2, 2016 at 12:59 AM, Mark Brown <broonie@kernel.org> wrote:
> On Fri, Jul 01, 2016 at 01:17:48PM -0700, Doug Anderson wrote:
>
>> Anyway, let me know if you want me to post the untested, rebased
>> patch...  Personally I'd prefer to wait for Boris's patches and the
>> land the tested version.
>
> We've got some boards in kernelci.org using PWM regulators I think, or
> perhaps someone else can test.  It does seem like a real fix so...

OK, post coming up then.  Sorry for the delay--I was on holiday the
last two days.

Note: which boards are you thinking would test this?  My grep showed
zero boards using regulator-ramp-delay with PWM regulator.  While this
is an important fix for anyone using the ramp delay, I'm not convinced
that anyone in mainline is using it.

-Doug

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

[Mark Brown]

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

On Wed, Jul 06, 2016 at 11:30:59AM -0700, Doug Anderson wrote:

> Note: which boards are you thinking would test this?  My grep showed
> zero boards using regulator-ramp-delay with PWM regulator.  While this
> is an important fix for anyone using the ramp delay, I'm not convinced
> that anyone in mainline is using it.

I just meant there are boards out there using the regulator, not the
specific feature.

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

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

[Boris Brezillon]

+Thierry

Hi Doug,

On Fri, 1 Jul 2016 13:17:48 -0700
Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:

> Mark,
> 
> On Fri, Jul 1, 2016 at 1:06 AM, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> > On Mon, Jun 27, 2016 at 09:53:11PM -0700, Douglas Anderson wrote:
> >  
> >> Note that this patch is atop Boris's recent PWM regulator fixes.  If
> >> desired it wouldn't be too hard to write it atop the old code, though
> >> quite honestly anyone using a PWM regulator should probably be using his
> >> new code.  
> >
> > Given that those don't seem likely to go in any time soon and aren't
> > going to go in as a fix can you please respin against current code?  
> 
> I went to write this patch and it was pretty easy to write.  ...but
> then I realized that I had no real way to test it.  I can compile test
> it, but that's it.  Do you still want it?
> 
> Specifically without Boris's patches then I don't believe the devices
> I have access to are bootable when I tell Linux about the PWM
> regulator, especially when I configure the PWM regulator to use
> continuous mode (which is where the bug is).  As I understand from
> <https://patchwork.kernel.org/patch/9175279/> the current PWM
> regulator assumes that 0% duty cycle is the lowest voltage and 100% is
> the highest voltage.  My system is backwards (0% is highest, 100%
> lowest).  Also: last I tried even the non-continuous mode without
> Boris's patches the regulator would glitch enough at bootup that my
> system would crash (the PWM regulator affects the memory controller).
> 
> Anyway, let me know if you want me to post the untested, rebased
> patch...  Personally I'd prefer to wait for Boris's patches and the
> land the tested version.
> 
> Boris: any idea for what the next steps on your series are?

I'm waiting for reviews or acks. AFAIR, I addressed all the comments
Thierry made on my v2, so I guess we're good on the PWM side, but that
would be good to have a review from Mark now that we agreed on the PWM
APIs.

That shouldn't prevent you from rebasing this patch on Linus' tree and
tag it for stable (I'll rebase my series on top of the regulator tree if
needed). 

Regards,

Boris

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

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 559 bytes --]

On Sat, Jul 02, 2016 at 05:47:52PM +0200, Boris Brezillon wrote:

> I'm waiting for reviews or acks. AFAIR, I addressed all the comments
> Thierry made on my v2, so I guess we're good on the PWM side, but that
> would be good to have a review from Mark now that we agreed on the PWM
> APIs.

I've already have one round of review invalidated by changes in the PWM
bits so I've been deprioritising it until it's clear that the churn
there has stopped.  I understand things are supposed to be quieting down
but I also have a bunch of other things to look at...

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

[-- Attachment #2: Type: text/plain, Size: 200 bytes --]

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

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

[Boris Brezillon]

Hi Mark,

On Mon, 4 Jul 2016 10:53:36 +0200
Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:

> On Sat, Jul 02, 2016 at 05:47:52PM +0200, Boris Brezillon wrote:
> 
> > I'm waiting for reviews or acks. AFAIR, I addressed all the comments
> > Thierry made on my v2, so I guess we're good on the PWM side, but that
> > would be good to have a review from Mark now that we agreed on the PWM
> > APIs.  
> 
> I've already have one round of review invalidated by changes in the PWM
> bits so I've been deprioritising it until it's clear that the churn
> there has stopped.  I understand things are supposed to be quieting down
> but I also have a bunch of other things to look at...

I understand, but can we still expect to have it in 4.8 (I mean,
assuming Thierry is okay taking the first 2 patching for 4.8 too)?

Thanks,

Boris

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

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 356 bytes --]

On Mon, Jul 04, 2016 at 11:05:04AM +0200, Boris Brezillon wrote:

> I understand, but can we still expect to have it in 4.8 (I mean,
> assuming Thierry is okay taking the first 2 patching for 4.8 too)?

Possibly - we are getting very close to the merge window now, I don't
know what Thierry's policies are on applying things as the merge window
gets near.

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

[-- Attachment #2: Type: text/plain, Size: 200 bytes --]

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

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

[Thierry Reding]

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

On Mon, Jul 04, 2016 at 11:13:44AM +0200, Mark Brown wrote:
> On Mon, Jul 04, 2016 at 11:05:04AM +0200, Boris Brezillon wrote:
> 
> > I understand, but can we still expect to have it in 4.8 (I mean,
> > assuming Thierry is okay taking the first 2 patching for 4.8 too)?
> 
> Possibly - we are getting very close to the merge window now, I don't
> know what Thierry's policies are on applying things as the merge window
> gets near.

I don't have hard deadlines, especially not for new code that has zero
potential for regressing. I've now pulled that whole series into the PWM
tree.

Thierry

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

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

[Boris Brezillon]

On Mon, 27 Jun 2016 21:53:11 -0700
Douglas Anderson <dianders@chromium.org> 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%

I realize I may have introduced another bug when adding the
->enable()/disable() methods: we are only waiting for the ramp up delay
when changing the voltage, but it should probably be applied when
enabling the PWM, and conditionally applied when changing the voltage
only if the PWM is enabled.

> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> Note that this patch is atop Boris's recent PWM regulator fixes.  If
> desired it wouldn't be too hard to write it atop the old code, though
> quite honestly anyone using a PWM regulator should probably be using his
> new code.
> 
>  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 fa1c74c77bb0..de94d19f6e1f 100644
> --- a/drivers/regulator/pwm-regulator.c
> +++ b/drivers/regulator/pwm-regulator.c
> @@ -188,6 +188,7 @@ static int pwm_regulator_set_voltage(struct regulator_dev *rdev,
>  	struct pwm_state pstate;
>  	unsigned int diff_duty;
>  	unsigned int dutycycle;
> +	int old_uV = pwm_regulator_get_voltage(rdev);
>  	int ret;
>  
>  	pwm_init_state(drvdata->pwm, &pstate);
> @@ -219,8 +220,12 @@ static int pwm_regulator_set_voltage(struct regulator_dev *rdev,
>  		return ret;
>  	}
>  
> -	/* Delay required by PWM regulator to settle to the new voltage */
> -	usleep_range(ramp_delay, ramp_delay + 1000);
> +	if (ramp_delay == 0)
> +		return 0;
> +
> +	/* Ramp delay is in uV/uS. Adjust to uS and delay */
> +	ramp_delay = DIV_ROUND_UP(abs(req_min_uV - old_uV), ramp_delay);
> +	usleep_range(ramp_delay, ramp_delay + DIV_ROUND_UP(ramp_delay, 10));
>  
>  	return 0;
>  }

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

[Doug Anderson]

Boris,

On Wed, Jul 6, 2016 at 5:27 AM, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:
> On Mon, 27 Jun 2016 21:53:11 -0700
> Douglas Anderson <dianders@chromium.org> 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%
>
> I realize I may have introduced another bug when adding the
> ->enable()/disable() methods: we are only waiting for the ramp up delay
> when changing the voltage, but it should probably be applied when
> enabling the PWM, and conditionally applied when changing the voltage
> only if the PWM is enabled.

I'll certainly let Mark comment here, but:

* For enabling the PWM, I think you want want "regulator-enable-ramp-delay".

* Right, we probably shouldn't be delaying if the regulator is off.
I'll add that to my patch.

-Doug

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

[Mark Brown]

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

On Wed, Jul 06, 2016 at 11:31:45AM -0700, Doug Anderson wrote:

> I'll certainly let Mark comment here, but:

> * For enabling the PWM, I think you want want "regulator-enable-ramp-delay".

In the absence of that we should just be treating it as a voltage change
from zero.  Some regulators do have different characteristics on initial
ramp up but if we don't know about them we ought to just use the regular
one.  Note that the core does have standard code to apply delays which
may just be working here.

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

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

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 421 bytes --]

On Wed, Jul 06, 2016 at 02:27:19PM +0200, Boris Brezillon wrote:

> I realize I may have introduced another bug when adding the
> ->enable()/disable() methods: we are only waiting for the ramp up delay
> when changing the voltage, but it should probably be applied when
> enabling the PWM, and conditionally applied when changing the voltage
> only if the PWM is enabled.

Yes, it should be applied on enable.  

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

[-- Attachment #2: Type: text/plain, Size: 200 bytes --]

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip