Messages in this thread |  | | From | Arnd Bergmann <> | Date | Thu, 2 Apr 2020 23:16:22 +0200 | Subject | Re: [PATCH v11 06/12] pwm: imx27: Use 64-bit division macro and function |
| |
On Thu, Apr 2, 2020 at 10:16 PM Guru Das Srinagesh <gurus@codeaurora.org> wrote: > On Tue, Mar 31, 2020 at 10:49:29PM +0200, Thierry Reding wrote: > > Doesn't that mean that anything below a 1 second period will be clamped > > to just 0? > > True. How about this then? > > int pwm_imx27_calc_period_cycles(struct pwm_state state, > unsigned long clk_rate, > unsigned long *period_cycles) > { > u64 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; > } > > ... > > ret = pwm_imx27_calc_period_cycles(state, clk_get_rate(imx->clk_per), > &period_cycles); > if (ret) > return ret; > > I unit tested this logic 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 correctly. As for the second case where there are > more typical values of period, the proposed logic handles that correctly > too.
This looks correct, but very expensive, and you don't really have to go this far, given that c1 is guaranteed to be a 32-bit number, and you divide by a constant in the end.
Why not do something like
#define SHIFT 41 /* arbitrarily picked, not too big, not too small */ #define MUL 2199 /* 2^SHIFT / NSEC_PER_SEC */ period_cycles = clk_get_rate(imx->clk_per) * ((state->period * MUL) >> SHIFT);
Arnd
|  |