diff mbox series

pwm: tegra: Optimize period calculation

Message ID 20220303175012.358613-1-u.kleine-koenig@pengutronix.de
State Changes Requested
Headers show
Series pwm: tegra: Optimize period calculation | expand

Commit Message

Uwe Kleine-König March 3, 2022, 5:50 p.m. UTC 
Dividing by the result of a division looses precision because the result is
rounded twice. E.g. with clk_rate = 48000000 and period = 32760033 the
following numbers result:

	rate = pc->clk_rate >> PWM_DUTY_WIDTH = 187500
	hz = DIV_ROUND_CLOSEST_ULL(100ULL * NSEC_PER_SEC, period_ns) = 3052
	rate = DIV_ROUND_CLOSEST_ULL(100ULL * rate, hz) = 6144

The exact result would be 6142.5061875 and (apart from rounding) this is
found by using a single division. As a side effect is also a tad
cheaper to calculate.

Also using clk_rate >> PWM_DUTY_WIDTH looses precision. Consider for
example clk_rate = 47999999 and period = 106667:

	mul_u64_u64_div_u64(pc->clk_rate >> PWM_DUTY_WIDTH, period_ns,
			    NSEC_PER_SEC) = 19

	mul_u64_u64_div_u64(pc->clk_rate, period_ns,
			    NSEC_PER_SEC << PWM_DUTY_WIDTH) = 20

(The exact result is 20.000062083332033.)

With this optimizations also switch from round-closest to round-down. Given
that the calculations were non-optimal for quite some time now which
nobody reported as a problem, this is the opportunity to align the driver's
behavior to the requirements of new drivers. (Note however that the
duty_cycle calculation isn't aligned yet.)

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-tegra.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)


base-commit: ed14d36498c8d15be098df4af9ca324f96e9de74

Comments

Thierry Reding April 22, 2022, 4:19 p.m. UTC | #1 
On Thu, Mar 03, 2022 at 06:50:12PM +0100, Uwe Kleine-König wrote:
> Dividing by the result of a division looses precision because the result is
> rounded twice. E.g. with clk_rate = 48000000 and period = 32760033 the
> following numbers result:
> 
> 	rate = pc->clk_rate >> PWM_DUTY_WIDTH = 187500
> 	hz = DIV_ROUND_CLOSEST_ULL(100ULL * NSEC_PER_SEC, period_ns) = 3052
> 	rate = DIV_ROUND_CLOSEST_ULL(100ULL * rate, hz) = 6144
> 
> The exact result would be 6142.5061875 and (apart from rounding) this is
> found by using a single division. As a side effect is also a tad
> cheaper to calculate.
> 
> Also using clk_rate >> PWM_DUTY_WIDTH looses precision. Consider for
> example clk_rate = 47999999 and period = 106667:
> 
> 	mul_u64_u64_div_u64(pc->clk_rate >> PWM_DUTY_WIDTH, period_ns,
> 			    NSEC_PER_SEC) = 19
> 
> 	mul_u64_u64_div_u64(pc->clk_rate, period_ns,
> 			    NSEC_PER_SEC << PWM_DUTY_WIDTH) = 20
> 
> (The exact result is 20.000062083332033.)
> 
> With this optimizations also switch from round-closest to round-down. Given
> that the calculations were non-optimal for quite some time now which
> nobody reported as a problem, this is the opportunity to align the driver's
> behavior to the requirements of new drivers. (Note however that the
> duty_cycle calculation isn't aligned yet.)
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/pwm/pwm-tegra.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
> index e5a9ffef4a71..7fc03a9ec154 100644
> --- a/drivers/pwm/pwm-tegra.c
> +++ b/drivers/pwm/pwm-tegra.c
> @@ -99,7 +99,7 @@ static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  			    int duty_ns, int period_ns)
>  {
>  	struct tegra_pwm_chip *pc = to_tegra_pwm_chip(chip);
> -	unsigned long long c = duty_ns, hz;
> +	unsigned long long c = duty_ns;
>  	unsigned long rate, required_clk_rate;
>  	u32 val = 0;
>  	int err;
> @@ -156,11 +156,9 @@ static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  		pc->clk_rate = clk_get_rate(pc->clk);
>  	}
>  
> -	rate = pc->clk_rate >> PWM_DUTY_WIDTH;
> -
>  	/* Consider precision in PWM_SCALE_WIDTH rate calculation */
> -	hz = DIV_ROUND_CLOSEST_ULL(100ULL * NSEC_PER_SEC, period_ns);
> -	rate = DIV_ROUND_CLOSEST_ULL(100ULL * rate, hz);
> +	rate = mul_u64_u64_div_u64(pc->clk_rate, period_ns,
> +				   (u64)NSEC_PER_SEC << PWM_DUTY_WIDTH);
>  
>  	/*
>  	 * Since the actual PWM divider is the register's frequency divider
> @@ -169,6 +167,8 @@ static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  	 */
>  	if (rate > 0)
>  		rate--;
> +	else
> +		return -EINVAL;

Can you elaborate on why this is needed? Previously rate == 0 was a
valid case and this could still happen with the above calculations.

Thierry
Uwe Kleine-König April 25, 2022, 7:34 a.m. UTC | #2 
On Fri, Apr 22, 2022 at 06:19:03PM +0200, Thierry Reding wrote:
> On Thu, Mar 03, 2022 at 06:50:12PM +0100, Uwe Kleine-König wrote:
> > Dividing by the result of a division looses precision because the result is
> > rounded twice. E.g. with clk_rate = 48000000 and period = 32760033 the
> > following numbers result:
> > 
> > 	rate = pc->clk_rate >> PWM_DUTY_WIDTH = 187500
> > 	hz = DIV_ROUND_CLOSEST_ULL(100ULL * NSEC_PER_SEC, period_ns) = 3052
> > 	rate = DIV_ROUND_CLOSEST_ULL(100ULL * rate, hz) = 6144
> > 
> > The exact result would be 6142.5061875 and (apart from rounding) this is
> > found by using a single division. As a side effect is also a tad
> > cheaper to calculate.
> > 
> > Also using clk_rate >> PWM_DUTY_WIDTH looses precision. Consider for
> > example clk_rate = 47999999 and period = 106667:
> > 
> > 	mul_u64_u64_div_u64(pc->clk_rate >> PWM_DUTY_WIDTH, period_ns,
> > 			    NSEC_PER_SEC) = 19
> > 
> > 	mul_u64_u64_div_u64(pc->clk_rate, period_ns,
> > 			    NSEC_PER_SEC << PWM_DUTY_WIDTH) = 20
> > 
> > (The exact result is 20.000062083332033.)
> > 
> > With this optimizations also switch from round-closest to round-down. Given
> > that the calculations were non-optimal for quite some time now which
> > nobody reported as a problem, this is the opportunity to align the driver's
> > behavior to the requirements of new drivers. (Note however that the
> > duty_cycle calculation isn't aligned yet.)
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> >  drivers/pwm/pwm-tegra.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
> > index e5a9ffef4a71..7fc03a9ec154 100644
> > --- a/drivers/pwm/pwm-tegra.c
> > +++ b/drivers/pwm/pwm-tegra.c
> > @@ -99,7 +99,7 @@ static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> >  			    int duty_ns, int period_ns)
> >  {
> >  	struct tegra_pwm_chip *pc = to_tegra_pwm_chip(chip);
> > -	unsigned long long c = duty_ns, hz;
> > +	unsigned long long c = duty_ns;
> >  	unsigned long rate, required_clk_rate;
> >  	u32 val = 0;
> >  	int err;
> > @@ -156,11 +156,9 @@ static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> >  		pc->clk_rate = clk_get_rate(pc->clk);
> >  	}
> >  
> > -	rate = pc->clk_rate >> PWM_DUTY_WIDTH;
> > -
> >  	/* Consider precision in PWM_SCALE_WIDTH rate calculation */
> > -	hz = DIV_ROUND_CLOSEST_ULL(100ULL * NSEC_PER_SEC, period_ns);
> > -	rate = DIV_ROUND_CLOSEST_ULL(100ULL * rate, hz);
> > +	rate = mul_u64_u64_div_u64(pc->clk_rate, period_ns,
> > +				   (u64)NSEC_PER_SEC << PWM_DUTY_WIDTH);
> >  
> >  	/*
> >  	 * Since the actual PWM divider is the register's frequency divider
> > @@ -169,6 +167,8 @@ static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> >  	 */
> >  	if (rate > 0)
> >  		rate--;
> > +	else
> > +		return -EINVAL;
> 
> Can you elaborate on why this is needed? Previously rate == 0 was a
> valid case and this could still happen with the above calculations.

If the calculations before the if block result in rate = 0 this means
that the requested period is smaller than the minimal possible period.
So refusing this setting is part of the switch from
something-like-round-closest to round-down.

I will send a v2 explaining that in the commit log.

Best regards
Uwe
diff mbox series

Patch

diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
index e5a9ffef4a71..7fc03a9ec154 100644
--- a/drivers/pwm/pwm-tegra.c
+++ b/drivers/pwm/pwm-tegra.c
@@ -99,7 +99,7 @@  static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 			    int duty_ns, int period_ns)
 {
 	struct tegra_pwm_chip *pc = to_tegra_pwm_chip(chip);
-	unsigned long long c = duty_ns, hz;
+	unsigned long long c = duty_ns;
 	unsigned long rate, required_clk_rate;
 	u32 val = 0;
 	int err;
@@ -156,11 +156,9 @@  static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 		pc->clk_rate = clk_get_rate(pc->clk);
 	}
 
-	rate = pc->clk_rate >> PWM_DUTY_WIDTH;
-
 	/* Consider precision in PWM_SCALE_WIDTH rate calculation */
-	hz = DIV_ROUND_CLOSEST_ULL(100ULL * NSEC_PER_SEC, period_ns);
-	rate = DIV_ROUND_CLOSEST_ULL(100ULL * rate, hz);
+	rate = mul_u64_u64_div_u64(pc->clk_rate, period_ns,
+				   (u64)NSEC_PER_SEC << PWM_DUTY_WIDTH);
 
 	/*
 	 * Since the actual PWM divider is the register's frequency divider
@@ -169,6 +167,8 @@  static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	 */
 	if (rate > 0)
 		rate--;
+	else
+		return -EINVAL;
 
 	/*
 	 * Make sure that the rate will fit in the register's frequency