`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 togo this far, given that c1 is guaranteed to be a 32-bit number, andyou 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`