linux-pwm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [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).