| Message ID | 20201222221319.2101107-1-u.kleine-koenig@pengutronix.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] pwm: bcm2835: Improve period and duty cycle calculation | expand |
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
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
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 */
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(-)