* [PATCH] pwm: tegra: Optimize period calculation
@ 2022-03-03 17:50 Uwe Kleine-König
2022-04-22 16:19 ` Thierry Reding
0 siblings, 1 reply; 3+ messages in thread
From: Uwe Kleine-König @ 2022-03-03 17:50 UTC (permalink / raw)
To: Thierry Reding, Lee Jones, Jonathan Hunter; +Cc: linux-pwm, linux-tegra, kernel
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;
/*
* Make sure that the rate will fit in the register's frequency
base-commit: ed14d36498c8d15be098df4af9ca324f96e9de74
--
2.34.1
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] pwm: tegra: Optimize period calculation
2022-03-03 17:50 [PATCH] pwm: tegra: Optimize period calculation Uwe Kleine-König
@ 2022-04-22 16:19 ` Thierry Reding
2022-04-25 7:34 ` Uwe Kleine-König
0 siblings, 1 reply; 3+ messages in thread
From: Thierry Reding @ 2022-04-22 16:19 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Lee Jones, Jonathan Hunter, linux-pwm, linux-tegra, kernel
[-- Attachment #1: Type: text/plain, Size: 2951 bytes --]
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] pwm: tegra: Optimize period calculation
2022-04-22 16:19 ` Thierry Reding
@ 2022-04-25 7:34 ` Uwe Kleine-König
0 siblings, 0 replies; 3+ messages in thread
From: Uwe Kleine-König @ 2022-04-25 7:34 UTC (permalink / raw)
To: Thierry Reding; +Cc: linux-tegra, linux-pwm, Lee Jones, kernel, Jonathan Hunter
[-- Attachment #1: Type: text/plain, Size: 3623 bytes --]
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
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-04-25 7:35 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-03-03 17:50 [PATCH] pwm: tegra: Optimize period calculation Uwe Kleine-König
2022-04-22 16:19 ` Thierry Reding
2022-04-25 7:34 ` Uwe Kleine-König
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox