| Message ID | 20190525181133.4875-11-martin.blumenstingl@googlemail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | pwm-meson: cleanups and improvements | expand |
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
On 26/05/2019 21:41, Uwe Kleine-König wrote: > 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 > I don't have a strong view on this, Martin showed similar or much greater accuracy in the 2 principal use cases of the driver, so I'm ok with it. Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>
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; + } + dev_dbg(meson->chip.dev, "period=%u pre_div=%u cnt=%u\n", period, pre_div, cnt); @@ -195,8 +190,8 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm, channel->lo = cnt; } else { /* Then check is we can have the duty with the same pre_div */ - duty_cnt = DIV_ROUND_CLOSEST_ULL((u64)duty * 1000, - fin_ps * (pre_div + 1)); + duty_cnt = div64_u64(fin_freq * (u64)duty, + NSEC_PER_SEC * (pre_div + 1)); if (duty_cnt > 0xffff) { dev_err(meson->chip.dev, "unable to get duty cycle\n"); return -EINVAL;
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(-)