| Message ID | bb43ff8ad9e0b9884722d85e51c8fdb3ef4154d1.1591136989.git.gurus@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
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 <shawnguo@kernel.org> > Cc: Sascha Hauer <s.hauer@pengutronix.de> > Cc: Pengutronix Kernel Team <kernel@pengutronix.de> > Cc: Fabio Estevam <festevam@gmail.com> > Cc: NXP Linux Team <linux-imx@nxp.com> > Signed-off-by: Guru Das Srinagesh <gurus@codeaurora.org> > --- > 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
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; + } + + *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; prescale = period_cycles / 0x10000 + 1;
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 <shawnguo@kernel.org> Cc: Sascha Hauer <s.hauer@pengutronix.de> Cc: Pengutronix Kernel Team <kernel@pengutronix.de> Cc: Fabio Estevam <festevam@gmail.com> Cc: NXP Linux Team <linux-imx@nxp.com> Signed-off-by: Guru Das Srinagesh <gurus@codeaurora.org> --- drivers/pwm/pwm-imx27.c | 48 +++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 43 insertions(+), 5 deletions(-)