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

Groups > linux.kernel > #1412901 > unrolled thread

[PATCH 03/14] pwm: rockchip: Fix period and duty_cycle approximation

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

  [PATCH 03/14] pwm: rockchip: Fix period and duty_cycle approximation Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-06-03 10:30 +0200
    Re: [PATCH 03/14] pwm: rockchip: Fix period and duty_cycle  approximation Brian Norris <briannorris@chromium.org> - 2016-06-03 22:10 +0200
      Re: [PATCH 03/14] pwm: rockchip: Fix period and duty_cycle  approximation Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-06-04 08:30 +0200
        Re: [PATCH 03/14] pwm: rockchip: Fix period and duty_cycle  approximation Brian Norris <briannorris@chromium.org> - 2016-06-07 19:30 +0200

#1412901 — [PATCH 03/14] pwm: rockchip: Fix period and duty_cycle approximation

The current implementation always round down the duty and period
values, while it would be better to round them to the closest integer.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/pwm/pwm-rockchip.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
index 7d9cc90..68d72ce 100644
--- a/drivers/pwm/pwm-rockchip.c
+++ b/drivers/pwm/pwm-rockchip.c
@@ -114,12 +114,11 @@ static int rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	 * default prescaler value for all practical clock rate values.
 	 */
 	div = clk_rate * period_ns;
-	do_div(div, pc->data->prescaler * NSEC_PER_SEC);
-	period = div;
+	period = DIV_ROUND_CLOSEST_ULL(div,
+				       pc->data->prescaler * NSEC_PER_SEC);
 
 	div = clk_rate * duty_ns;
-	do_div(div, pc->data->prescaler * NSEC_PER_SEC);
-	duty = div;
+	duty = DIV_ROUND_CLOSEST_ULL(div, pc->data->prescaler * NSEC_PER_SEC);
 
 	ret = clk_enable(pc->clk);
 	if (ret)
-- 
2.7.4

[toc] | [next] | [standalone]

#1413440 — Re: [PATCH 03/14] pwm: rockchip: Fix period and duty_cycle approximation

On Fri, Jun 03, 2016 at 10:23:01AM +0200, Boris Brezillon wrote:
> The current implementation always round down the duty and period
> values, while it would be better to round them to the closest integer.

Agreed. As I noted to you elsewhere, not having this change can cause
problems where doing a series of pwm_get_state() / modify /
pwm_apply_state() will propagate rounding errors, which will change the
period unexpectedly. e.g., I have an expected period of 3.333 us and a
clk rate of 112.666667 MHz -- the clock frequency doesn't divide evenly,
so the period (stashed in nanoseconds) shrinks when we convert to the
register value and back, as follows:

 pwm_apply_state(): register = period * 112666667 / 1000000000;
 pwm_get_state(): period = register * 1000000000 / 112666667;

or in other words:

 period = period * 112666667 / 1000000000 * 1000000000 / 112666667;

which yields a sequence like:

 3333 -> 3328
 3328 -> 3319
 3319 -> 3310
 3310 -> 3301
 3301 -> 3292
 3292 -> ... (etc) ...

With this patch, we'd see instead:

 period = div_round_closest(period * 112666667, 1000000000) * 1000000000 / 112666667;

which yields a stable sequence:

 3333 -> 3337
 3337 -> 3337
 3337 -> ... (etc) ...

Seems much saner to me.

Now, I note that in patch 10 you're now using pwm_prepare_new_state() to
avoid this propagation problem entirely (good idea anyway, IMO), but I
just wanted to further note what kind of real problems we can see when
we don't round to the closest value.

> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>

Reviewed-by: Brian Norris <briannorris@chromium.org>
Tested-by: Brian Norris <briannorris@chromium.org>

Tested this whole series on rk3399's PWM regulators used for the CPUs,
to clarify what my Tested-by means.

Thanks for the patches.

> ---
>  drivers/pwm/pwm-rockchip.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
> index 7d9cc90..68d72ce 100644
> --- a/drivers/pwm/pwm-rockchip.c
> +++ b/drivers/pwm/pwm-rockchip.c
> @@ -114,12 +114,11 @@ static int rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  	 * default prescaler value for all practical clock rate values.
>  	 */
>  	div = clk_rate * period_ns;
> -	do_div(div, pc->data->prescaler * NSEC_PER_SEC);
> -	period = div;
> +	period = DIV_ROUND_CLOSEST_ULL(div,
> +				       pc->data->prescaler * NSEC_PER_SEC);
>  
>  	div = clk_rate * duty_ns;
> -	do_div(div, pc->data->prescaler * NSEC_PER_SEC);
> -	duty = div;
> +	duty = DIV_ROUND_CLOSEST_ULL(div, pc->data->prescaler * NSEC_PER_SEC);
>  
>  	ret = clk_enable(pc->clk);
>  	if (ret)
> -- 
> 2.7.4
> 

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

#1413663 — Re: [PATCH 03/14] pwm: rockchip: Fix period and duty_cycle approximation

On Fri, 3 Jun 2016 13:03:26 -0700
Brian Norris <briannorris@chromium.org> wrote:

> On Fri, Jun 03, 2016 at 10:23:01AM +0200, Boris Brezillon wrote:
> > The current implementation always round down the duty and period
> > values, while it would be better to round them to the closest integer.  
> 
> Agreed. As I noted to you elsewhere, not having this change can cause
> problems where doing a series of pwm_get_state() / modify /
> pwm_apply_state() will propagate rounding errors, which will change the
> period unexpectedly. e.g., I have an expected period of 3.333 us and a
> clk rate of 112.666667 MHz -- the clock frequency doesn't divide evenly,
> so the period (stashed in nanoseconds) shrinks when we convert to the
> register value and back, as follows:
> 
>  pwm_apply_state(): register = period * 112666667 / 1000000000;
>  pwm_get_state(): period = register * 1000000000 / 112666667;
> 
> or in other words:
> 
>  period = period * 112666667 / 1000000000 * 1000000000 / 112666667;
> 
> which yields a sequence like:
> 
>  3333 -> 3328
>  3328 -> 3319
>  3319 -> 3310
>  3310 -> 3301
>  3301 -> 3292
>  3292 -> ... (etc) ...
> 
> With this patch, we'd see instead:
> 
>  period = div_round_closest(period * 112666667, 1000000000) * 1000000000 / 112666667;
> 
> which yields a stable sequence:
> 
>  3333 -> 3337
>  3337 -> 3337
>  3337 -> ... (etc) ...

Woh! Thanks for the detailed explanation. Do you want me to put that in
a comment explaining why we're using DIV_ROUND_CLOSEST_ULL()?


-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

#1416442 — Re: [PATCH 03/14] pwm: rockchip: Fix period and duty_cycle approximation

On Sat, Jun 04, 2016 at 08:19:55AM +0200, Boris Brezillon wrote:
> On Fri, 3 Jun 2016 13:03:26 -0700
> Brian Norris <briannorris@chromium.org> wrote:
> 
> > On Fri, Jun 03, 2016 at 10:23:01AM +0200, Boris Brezillon wrote:
> > > The current implementation always round down the duty and period
> > > values, while it would be better to round them to the closest integer.  
> > 
> > Agreed. As I noted to you elsewhere, not having this change can cause
> > problems where doing a series of pwm_get_state() / modify /
> > pwm_apply_state() will propagate rounding errors, which will change the
> > period unexpectedly. e.g., I have an expected period of 3.333 us and a
> > clk rate of 112.666667 MHz -- the clock frequency doesn't divide evenly,
> > so the period (stashed in nanoseconds) shrinks when we convert to the
> > register value and back, as follows:
> > 
> >  pwm_apply_state(): register = period * 112666667 / 1000000000;
> >  pwm_get_state(): period = register * 1000000000 / 112666667;
> > 
> > or in other words:
> > 
> >  period = period * 112666667 / 1000000000 * 1000000000 / 112666667;
> > 
> > which yields a sequence like:
> > 
> >  3333 -> 3328
> >  3328 -> 3319
> >  3319 -> 3310
> >  3310 -> 3301
> >  3301 -> 3292
> >  3292 -> ... (etc) ...
> > 
> > With this patch, we'd see instead:
> > 
> >  period = div_round_closest(period * 112666667, 1000000000) * 1000000000 / 112666667;
> > 
> > which yields a stable sequence:
> > 
> >  3333 -> 3337
> >  3337 -> 3337
> >  3337 -> ... (etc) ...
> 
> Woh! Thanks for the detailed explanation. Do you want me to put that in
> a comment explaining why we're using DIV_ROUND_CLOSEST_ULL()?

If you'd like, feel free to add some of this to your v2 description.

Brian

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web