Hello, On Tue, Jun 02, 2020 at 03:31:10PM -0700, Guru Das Srinagesh wrote: > Since the PWM framework is switching struct pwm_state.period's > datatype to u64, prepare for this transition by using > DIV_ROUND_UP_ULL to handle a 64-bit dividend, and div64_u64 to handle a > 64-bit divisor. > > Also handle a possible overflow in the calculation of period_cycles when > both clk_rate and period are large numbers. This logic was unit-tested > out by calculating period_cycles using both the existing logic and the > proposed one, and the results are as below. > > ---------------------------------------------------------------------------- > clk_rate period existing proposed > ---------------------------------------------------------------------------- > 1000000000 18446744073709551615 18446744072 18446744073000000000 > (U64_MAX) > ---------------------------------------------------------------------------- > 1000000000 4294967291 4294967291 4294967291 > ---------------------------------------------------------------------------- > > Overflow occurs in the first case with the existing logic, whereas the > proposed logic handles it better, sacrificing some precision in a best-effort > attempt to handle overflow. As for the second case where there are > more typical values of period, the proposed logic handles that correctly > too. > > Cc: Shawn Guo <[email protected]> > Cc: Sascha Hauer <[email protected]> > Cc: Pengutronix Kernel Team <[email protected]> > Cc: Fabio Estevam <[email protected]> > Cc: NXP Linux Team <[email protected]> > Signed-off-by: Guru Das Srinagesh <[email protected]> > --- > drivers/pwm/pwm-imx27.c | 48 +++++++++++++++++++++++++++++++++++++++++++----- > 1 file changed, 43 insertions(+), 5 deletions(-) > > diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c > index 732a6f3..147729d 100644 > --- a/drivers/pwm/pwm-imx27.c > +++ b/drivers/pwm/pwm-imx27.c > @@ -202,7 +202,7 @@ static void pwm_imx27_wait_fifo_slot(struct pwm_chip > *chip, > sr = readl(imx->mmio_base + MX3_PWMSR); > fifoav = FIELD_GET(MX3_PWMSR_FIFOAV, sr); > if (fifoav == MX3_PWMSR_FIFOAV_4WORDS) { > - period_ms = DIV_ROUND_UP(pwm_get_period(pwm), > + period_ms = DIV_ROUND_UP_ULL(pwm_get_period(pwm), > NSEC_PER_MSEC); > msleep(period_ms); > > @@ -212,6 +212,45 @@ static void pwm_imx27_wait_fifo_slot(struct pwm_chip > *chip, > } > } > > +static int pwm_imx27_calc_period_cycles(const struct pwm_state *state, > + unsigned long clk_rate, > + unsigned long *period_cycles) > +{ > + u64 c = 0, c1, c2; > + > + c1 = clk_rate; > + c2 = state->period; > + if (c2 > c1) { > + c2 = c1; > + c1 = state->period; > + } > + > + if (!c1 || !c2) { > + pr_err("clk rate and period should be nonzero\n"); > + return -EINVAL; > + } > + > + if (c2 <= div64_u64(U64_MAX, c1)) { > + c = c1 * c2; > + do_div(c, 1000000000); > + } else if (c2 <= div64_u64(U64_MAX, div64_u64(c1, 1000))) { > + do_div(c1, 1000); > + c = c1 * c2; > + do_div(c, 1000000); > + } else if (c2 <= div64_u64(U64_MAX, div64_u64(c1, 1000000))) { > + do_div(c1, 1000000); > + c = c1 * c2; > + do_div(c, 1000); > + } else if (c2 <= div64_u64(U64_MAX, div64_u64(c1, 1000000000))) { > + do_div(c1, 1000000000); > + c = c1 * c2;
} else {
???
> + }
> +
> + *period_cycles = c;
> +
> + return 0;
> +}
> +
> static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> const struct pwm_state *state)
> {
> @@ -226,10 +265,9 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct
> pwm_device *pwm,
> pwm_get_state(pwm, &cstate);
>
> clkrate = clk_get_rate(imx->clk_per);
> - c = clkrate * state->period;
> -
> - do_div(c, NSEC_PER_SEC);
> - period_cycles = c;
> + ret = pwm_imx27_calc_period_cycles(state, clkrate, &period_cycles);
> + if (ret)
> + return ret;
I would expect this problem to be generic enough that it justifies a
function in pwm-core (or even more generic?).
Something like:
/*
* calculates *result = a * b / c.
* Returns false on overflow (and doesn't assign result in this
* case); true otherwise.
*/
static inline bool multdiv(u64 *result, u64 a, u64 b, u64 c)
I'd split this out in a separate patch though? (Or is imx27 the only
driver in this series with this problem?)
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
