lkml.org 
[lkml]   [2019]   [May]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 10/14] pwm: meson: simplify the calculation of the pre-divider and count
On Sat, May 25, 2019 at 08:11:29PM +0200, Martin Blumenstingl wrote:
> Replace the loop to calculate the pre-divider and count with two
> separate div64_u64() calculations. This makes the code easier to read
> and improves the precision.
>
> Two example cases:
> 1) 32.768kHz LPO clock for the SDIO wifi chip on Khadas VIM
> clock input: 500MHz (FCLK_DIV4)
> period: 30518ns
> duty cycle: 15259ns
> old algorithm: pre_div=0, cnt=15259
> new algorithm: pre_div=0, cnt=15259
> (no difference in calculated values)
>
> 2) PWM LED on Khadas VIM
> clock input: 24MHz (XTAL)
> period: 7812500ns
> duty cycle: 7812500ns
> old algorithm: pre_div=2, cnt=62004
> new algorithm: pre_div=2, cnt=62500
> Using a scope (24MHz sampling rate) shows the actual difference:
> - old: 7753000ns, off by -59500ns (0.7616%)
> - new: 7815000ns, off by +2500ns (0.032%)
>
> Suggested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
> drivers/pwm/pwm-meson.c | 25 ++++++++++---------------
> 1 file changed, 10 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
> index 27915d6475e3..9afa1e5aaebf 100644
> --- a/drivers/pwm/pwm-meson.c
> +++ b/drivers/pwm/pwm-meson.c
> @@ -12,6 +12,7 @@
> #include <linux/err.h>
> #include <linux/io.h>
> #include <linux/kernel.h>
> +#include <linux/math64.h>
> #include <linux/module.h>
> #include <linux/of.h>
> #include <linux/of_device.h>
> @@ -145,7 +146,6 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm,
> struct meson_pwm_channel *channel = pwm_get_chip_data(pwm);
> unsigned int duty, period, pre_div, cnt, duty_cnt;
> unsigned long fin_freq = -1;
> - u64 fin_ps;
>
> duty = state->duty_cycle;
> period = state->period;
> @@ -164,24 +164,19 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm,
> }
>
> dev_dbg(meson->chip.dev, "fin_freq: %lu Hz\n", fin_freq);
> - fin_ps = (u64)NSEC_PER_SEC * 1000;
> - do_div(fin_ps, fin_freq);
> -
> - /* Calc pre_div with the period */
> - for (pre_div = 0; pre_div <= MISC_CLK_DIV_MASK; pre_div++) {
> - cnt = DIV_ROUND_CLOSEST_ULL((u64)period * 1000,
> - fin_ps * (pre_div + 1));
> - dev_dbg(meson->chip.dev, "fin_ps=%llu pre_div=%u cnt=%u\n",
> - fin_ps, pre_div, cnt);
> - if (cnt <= 0xffff)
> - break;
> - }
>
> + pre_div = div64_u64(fin_freq * (u64)period, NSEC_PER_SEC * 0xffffLL);
> if (pre_div > MISC_CLK_DIV_MASK) {
> dev_err(meson->chip.dev, "unable to get period pre_div\n");
> return -EINVAL;
> }
>
> + cnt = div64_u64(fin_freq * (u64)period, NSEC_PER_SEC * (pre_div + 1));
> + if (cnt > 0xffff) {
> + dev_err(meson->chip.dev, "unable to get period cnt\n");
> + return -EINVAL;
> + }
> +

There is a slight modification in the calculation of pre_div that isn't
catched by the examples above.

Before this patch we had:

pick smallest pre_div such that
round_closest(period * 1000 / (round_down(1e12 / fin_freq) * (pre_div + 1)) <= 0xffff

New approach is:

pre_div = round_down(fin_freq * period / (1e9 * 0xffff))

An advantage of the new approach is better as it rounds only once and is
easier.

Consider fin_freq = 99990001 and period = 655355, then the old algorithm
picks pre_div = 1 while the new picks pre_div = 0.

I didn't continue here to check which are the resulting waveforms, I
assume they are different though.

As there is currently no definition what is a "better" approximation for
a given requested pair (duty_cycle, period) I cannot say if these
changes are good or not.

And that's a pity, so I still think there should be a documented
definition that lays down how a lowlevel driver should round. Without
that a consumer that cares about fine differences can not rely an the
abstraction provided by the PWM framework because each low-level driver
might behave differently.

@Thierry: So can you please continue the discussion about this topic.
The longer this is delayed the more patches are created and submitted
that eventually might be wrong which is a waste of developer and
reviewer time.

Assuming the people who care about meson don't object after reading this
I wouldn't want to stop this patch going in though. So:

Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Best regards
Uwe

--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |

\
 
 \ /
  Last update: 2019-05-26 21:42    [W:0.159 / U:3.852 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site