* [PATCH v2] pwm: bcm2835: Improve period and duty cycle calculation
@ 2020-12-22 22:13 Uwe Kleine-König
2020-12-31 19:30 ` Lino Sanfilippo
0 siblings, 1 reply; 3+ messages in thread
From: Uwe Kleine-König @ 2020-12-22 22:13 UTC (permalink / raw)
To: Thierry Reding, Lee Jones, Nicolas Saenz Julienne,
Florian Fainelli, Ray Jui, Scott Branden, Lino Sanfilippo,
Sean Young
Cc: linux-pwm, bcm-kernel-feedback-list, linux-rpi-kernel, kernel
With an input clk rate bigger than 2000000000, scaler would have been
zero which then would have resulted in a division by zero.
Also the originally implemented algorithm divided by the result of a
division. This nearly always looses precision. Consider a requested period
of 1000000 ns. With an input clock frequency of 32786885 Hz the hardware
was configured with an actual period of 983869.007 ns (PERIOD = 32258)
while the hardware can provide 1000003.508 ns (PERIOD = 32787).
And note if the input clock frequency was 32786886 Hz instead, the hardware
was configured to 1016656.477 ns (PERIOD = 33333) while the optimal
setting results in 1000003.477 ns (PERIOD = 32787).
This patch implements proper range checking and only divides once for
the calculation of period (and similar for duty_cycle).
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Hello,
changes since v2:
- Keep the traditional behaviour to refuse requests with a too big
period. (v1 rounded down to the longest possible period.)
Thanks to Lino for catching the unintended change in behaviour.
- Improve precision in max_period calculation as this matters now.
Because of these changes I didn't add Lino's Tested-by.
Best regards
Uwe
drivers/pwm/pwm-bcm2835.c | 26 ++++++++++++++++++--------
1 file changed, 18 insertions(+), 8 deletions(-)
diff --git a/drivers/pwm/pwm-bcm2835.c b/drivers/pwm/pwm-bcm2835.c
index 6ff5f04b3e07..30db5d5d70f7 100644
--- a/drivers/pwm/pwm-bcm2835.c
+++ b/drivers/pwm/pwm-bcm2835.c
@@ -64,8 +64,9 @@ static int bcm2835_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
struct bcm2835_pwm *pc = to_bcm2835_pwm(chip);
unsigned long rate = clk_get_rate(pc->clk);
- unsigned long long period;
- unsigned long scaler;
+ unsigned long long period_cycles;
+ u64 max_period;
+
u32 val;
if (!rate) {
@@ -73,18 +74,27 @@ static int bcm2835_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
return -EINVAL;
}
- scaler = DIV_ROUND_CLOSEST(NSEC_PER_SEC, rate);
+ /*
+ * period_cycles must be a 32 bit value, so period * rate / NSEC_PER_SEC
+ * must be <= U32_MAX. As U32_MAX * NSEC_PER_SEC < U64_MAX the
+ * multiplication period * rate doesn't overflow.
+ */
+ max_period = DIV_ROUND_UP_ULL((u64)U32_MAX * NSEC_PER_SEC + NSEC_PER_SEC / 2, rate) - 1;
+
+ if (state->period > max_period)
+ return -EINVAL;
+
/* set period */
- period = DIV_ROUND_CLOSEST_ULL(state->period, scaler);
+ period_cycles = DIV_ROUND_CLOSEST_ULL(state->period * rate, NSEC_PER_SEC);
- /* dont accept a period that is too small or has been truncated */
- if ((period < PERIOD_MIN) || (period > U32_MAX))
+ /* don't accept a period that is too small */
+ if (period_cycles < PERIOD_MIN)
return -EINVAL;
- writel(period, pc->base + PERIOD(pwm->hwpwm));
+ writel(period_cycles, pc->base + PERIOD(pwm->hwpwm));
/* set duty cycle */
- val = DIV_ROUND_CLOSEST_ULL(state->duty_cycle, scaler);
+ val = DIV_ROUND_CLOSEST_ULL(state->duty_cycle * rate, NSEC_PER_SEC);
writel(val, pc->base + DUTY(pwm->hwpwm));
/* set polarity */
--
2.29.2
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] pwm: bcm2835: Improve period and duty cycle calculation
2020-12-22 22:13 [PATCH v2] pwm: bcm2835: Improve period and duty cycle calculation Uwe Kleine-König
@ 2020-12-31 19:30 ` Lino Sanfilippo
2021-01-12 18:47 ` Uwe Kleine-König
0 siblings, 1 reply; 3+ messages in thread
From: Lino Sanfilippo @ 2020-12-31 19:30 UTC (permalink / raw)
To: Uwe Kleine-König, Thierry Reding, Lee Jones,
Nicolas Saenz Julienne, Florian Fainelli, Ray Jui, Scott Branden,
Sean Young
Cc: linux-pwm, bcm-kernel-feedback-list, linux-rpi-kernel, kernel,
LinoSanfilippo
Hi Uwe,
just some nitpicks (maybe only worth fixing if there will be a v3 for other reasons):
On 22.12.20 at 23:13, Uwe Kleine-König wrote:
> + /*
> + * period_cycles must be a 32 bit value, so period * rate / NSEC_PER_SEC
> + * must be <= U32_MAX. As U32_MAX * NSEC_PER_SEC < U64_MAX the
> + * multiplication period * rate doesn't overflow.
> + */
> + max_period = DIV_ROUND_UP_ULL((u64)U32_MAX * NSEC_PER_SEC + NSEC_PER_SEC / 2, rate) - 1;
For someone looking at the formula it might be helpful to also have an explanation for the added
term "+ NSEC_PER_SEC / 2" in the comment. This line also exeeds the 80 chars limit...
> + period_cycles = DIV_ROUND_CLOSEST_ULL(state->period * rate, NSEC_PER_SEC);
...as well as this line.
Regards,
Lino
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] pwm: bcm2835: Improve period and duty cycle calculation
2020-12-31 19:30 ` Lino Sanfilippo
@ 2021-01-12 18:47 ` Uwe Kleine-König
0 siblings, 0 replies; 3+ messages in thread
From: Uwe Kleine-König @ 2021-01-12 18:47 UTC (permalink / raw)
To: Lino Sanfilippo
Cc: Thierry Reding, Lee Jones, Nicolas Saenz Julienne,
Florian Fainelli, Ray Jui, Scott Branden, Sean Young, linux-pwm,
bcm-kernel-feedback-list, linux-rpi-kernel, kernel
[-- Attachment #1: Type: text/plain, Size: 1975 bytes --]
Hello Lino,
On Thu, Dec 31, 2020 at 08:30:12PM +0100, Lino Sanfilippo wrote:
> just some nitpicks (maybe only worth fixing if there will be a v3 for other reasons):
>
> On 22.12.20 at 23:13, Uwe Kleine-König wrote:
>
> > + /*
> > + * period_cycles must be a 32 bit value, so period * rate / NSEC_PER_SEC
> > + * must be <= U32_MAX. As U32_MAX * NSEC_PER_SEC < U64_MAX the
> > + * multiplication period * rate doesn't overflow.
> > + */
> > + max_period = DIV_ROUND_UP_ULL((u64)U32_MAX * NSEC_PER_SEC + NSEC_PER_SEC / 2, rate) - 1;
>
> For someone looking at the formula it might be helpful to also have an explanation for the added
> term "+ NSEC_PER_SEC / 2" in the comment. This line also exeeds the 80 chars limit...
Yeah, the 80 char limit isn't that strict any more and (IMHO) adding a
line break anywhere in the formula hurts readibility, so I accepted
the long line as the lesser evil.
Regarding the + NSEC_PER_SEC / 2: Is it clear to you that it is right?
If so that would be great as confirmation that I got the maths right.
Adding a comment is a good idea, what about:
+ /*
+ * period_cycles must be a 32 bit value, so period * rate / NSEC_PER_SEC
+ * must be <= U32_MAX. As U32_MAX * NSEC_PER_SEC < U64_MAX the
+ * multiplication period * rate doesn't overflow.
+ * To calculate the maximal possible period that guarantees the
+ * above inequality:
+ *
+ * round(period * rate / NSEC_PER_SEC) <= U32_MAX
+ * <=> period * rate / NSEC_PER_SEC < U32_MAX + 0.5
+ * <=> period * rate < (U32_MAX + 0.5) * NSEC_PER_SEC
* <=> period < ((U32_MAX + 0.5) * NSEC_PER_SEC) / rate
* <=> period < ((U32_MAX * NSEC_PER_SEC + NSEC_PER_SEC/2) / rate
* <=> period <= ceil((U32_MAX * NSEC_PER_SEC + NSEC_PER_SEC/2) / rate) - 1
+ */
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:[~2021-01-12 18:48 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-22 22:13 [PATCH v2] pwm: bcm2835: Improve period and duty cycle calculation Uwe Kleine-König
2020-12-31 19:30 ` Lino Sanfilippo
2021-01-12 18:47 ` 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;
as well as URLs for NNTP newsgroup(s).