* [PATCH 0/2] pwm: meson: two small bug-fixes @ 2019-04-01 18:18 Martin Blumenstingl 2019-04-01 18:18 ` [PATCH 1/2] pwm: meson: consider 128 a valid pre-divider Martin Blumenstingl ` (2 more replies) 0 siblings, 3 replies; 9+ messages in thread From: Martin Blumenstingl @ 2019-04-01 18:18 UTC (permalink / raw) To: thierry.reding, linux-pwm, linux-amlogic Cc: narmstrong, jbrunet, linux-arm-kernel, linux-kernel, u.kleine-koenig, bichao.zheng, Martin Blumenstingl This series applies on top of my other fix "pwm: meson: fix scheduling while atomic issue" from [0] The first patch fixes an issue where the maximum possible pre-divider (128) could not be used because there was an off-by-one error in the code. I discovered this while testing with the longest supported period (349514407ns) when running from XTAL. This is verified to work on my Meson8b Odroid-C1 board using "pwm_b" on GPIOX_11. The second patch was suggested by Uwe Kleine-König but was actually implemented much earlier (back in mid 2018) by Bichao Zheng from Amlogic. This patch fixes changing the duty cycle by relying on the hardware to re-start the PWM output (instead of adding an artificial "constant LOW" of about 20ms - as measured by Bichao Zheng when stopping and re-starting the PWM output from within the driver). I tested this fix on my Meson8b Odroid-C1 board which uses a PWM driven CPU regulator (DVFS with all supported OPPs is still working fine for me, although I couldn't observe any issues before this patch). I also have some code-improvements queued which I'll send in the next days, see [1] [0] https://patchwork.kernel.org/cover/10880419/ [1] https://github.com/xdarklight/linux/commits/meson-pwm-for-5.2-v1 Bichao Zheng (1): pwm: meson: don't disable pwm when setting duty repeatedly Martin Blumenstingl (1): pwm: meson: consider 128 a valid pre-divider drivers/pwm/pwm-meson.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) -- 2.21.0 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] pwm: meson: consider 128 a valid pre-divider 2019-04-01 18:18 [PATCH 0/2] pwm: meson: two small bug-fixes Martin Blumenstingl @ 2019-04-01 18:18 ` Martin Blumenstingl 2019-04-01 18:38 ` Uwe Kleine-König 2019-04-03 11:19 ` Neil Armstrong 2019-04-01 18:18 ` [PATCH 2/2] pwm: meson: don't disable pwm when setting duty repeatedly Martin Blumenstingl 2019-05-09 14:52 ` [PATCH 0/2] pwm: meson: two small bug-fixes Thierry Reding 2 siblings, 2 replies; 9+ messages in thread From: Martin Blumenstingl @ 2019-04-01 18:18 UTC (permalink / raw) To: thierry.reding, linux-pwm, linux-amlogic Cc: narmstrong, jbrunet, linux-arm-kernel, linux-kernel, u.kleine-koenig, bichao.zheng, Martin Blumenstingl The pre-divider allows configuring longer PWM periods compared to using the input clock directly. The pre-divider is 7 bit wide, meaning it's maximum value is 128 (the register value is off-by-one: 0x7f or 127). Change the loop to also allow for the maximum possible value to be considered valid. Fixes: 211ed630753d2f ("pwm: Add support for Meson PWM Controller") Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> --- drivers/pwm/pwm-meson.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c index f6e738ad7bd9..4b708c1fcb1d 100644 --- a/drivers/pwm/pwm-meson.c +++ b/drivers/pwm/pwm-meson.c @@ -188,7 +188,7 @@ static int meson_pwm_calc(struct meson_pwm *meson, do_div(fin_ps, fin_freq); /* Calc pre_div with the period */ - for (pre_div = 0; pre_div < MISC_CLK_DIV_MASK; pre_div++) { + 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", @@ -197,7 +197,7 @@ static int meson_pwm_calc(struct meson_pwm *meson, break; } - if (pre_div == MISC_CLK_DIV_MASK) { + if (pre_div > MISC_CLK_DIV_MASK) { dev_err(meson->chip.dev, "unable to get period pre_div\n"); return -EINVAL; } -- 2.21.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] pwm: meson: consider 128 a valid pre-divider 2019-04-01 18:18 ` [PATCH 1/2] pwm: meson: consider 128 a valid pre-divider Martin Blumenstingl @ 2019-04-01 18:38 ` Uwe Kleine-König 2019-04-02 19:22 ` Martin Blumenstingl 2019-04-03 11:19 ` Neil Armstrong 1 sibling, 1 reply; 9+ messages in thread From: Uwe Kleine-König @ 2019-04-01 18:38 UTC (permalink / raw) To: Martin Blumenstingl Cc: thierry.reding, linux-pwm, linux-amlogic, narmstrong, jbrunet, linux-arm-kernel, linux-kernel, bichao.zheng Hello Martin, On Mon, Apr 01, 2019 at 08:18:16PM +0200, Martin Blumenstingl wrote: > diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c > index f6e738ad7bd9..4b708c1fcb1d 100644 > --- a/drivers/pwm/pwm-meson.c > +++ b/drivers/pwm/pwm-meson.c > @@ -188,7 +188,7 @@ static int meson_pwm_calc(struct meson_pwm *meson, > do_div(fin_ps, fin_freq); > > /* Calc pre_div with the period */ > - for (pre_div = 0; pre_div < MISC_CLK_DIV_MASK; pre_div++) { > + 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", You could even calculate pre_div without the loop. Something like: u64 pre_div = (u64)period * rate; do_div_round_up(pre_div, NSEC_PER_SEC * 0xffff); pre_div--; (I didn't check rounding and maybe its off by one and ...) This would also get rid of the strange 1000 that is currently used in the calculation without a real benefit (unless I missed something). Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ | ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] pwm: meson: consider 128 a valid pre-divider 2019-04-01 18:38 ` Uwe Kleine-König @ 2019-04-02 19:22 ` Martin Blumenstingl 2019-04-02 19:55 ` Uwe Kleine-König 0 siblings, 1 reply; 9+ messages in thread From: Martin Blumenstingl @ 2019-04-02 19:22 UTC (permalink / raw) To: Uwe Kleine-König Cc: thierry.reding, linux-pwm, linux-amlogic, Neil Armstrong, jbrunet, linux-arm-kernel, linux-kernel, bichao.zheng Hello Uwe, On Mon, Apr 1, 2019 at 8:38 PM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > Hello Martin, > > On Mon, Apr 01, 2019 at 08:18:16PM +0200, Martin Blumenstingl wrote: > > diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c > > index f6e738ad7bd9..4b708c1fcb1d 100644 > > --- a/drivers/pwm/pwm-meson.c > > +++ b/drivers/pwm/pwm-meson.c > > @@ -188,7 +188,7 @@ static int meson_pwm_calc(struct meson_pwm *meson, > > do_div(fin_ps, fin_freq); > > > > /* Calc pre_div with the period */ > > - for (pre_div = 0; pre_div < MISC_CLK_DIV_MASK; pre_div++) { > > + 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", > > You could even calculate pre_div without the loop. > > Something like: > > u64 pre_div = (u64)period * rate; > do_div_round_up(pre_div, NSEC_PER_SEC * 0xffff); > pre_div--; > > (I didn't check rounding and maybe its off by one and ...) This would > also get rid of the strange 1000 that is currently used in the > calculation without a real benefit (unless I missed something). personally I prefer using this simple patch applied first as it is easy to review and (due to the Fixes tag) may get backported to stable kernels. I'm not saying I don't like your suggestion, I propose to postpone implementing this cleanup. I need to have a closer look at the calculation because three values are derived from the input clock rate (pre_div, cnt, duty_cnt) and I don't want to mess up the cases that are already working as of today. Please let me know what you think. Regards Martin ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] pwm: meson: consider 128 a valid pre-divider 2019-04-02 19:22 ` Martin Blumenstingl @ 2019-04-02 19:55 ` Uwe Kleine-König 0 siblings, 0 replies; 9+ messages in thread From: Uwe Kleine-König @ 2019-04-02 19:55 UTC (permalink / raw) To: Martin Blumenstingl Cc: thierry.reding, linux-pwm, linux-amlogic, Neil Armstrong, jbrunet, linux-arm-kernel, linux-kernel, bichao.zheng On Tue, Apr 02, 2019 at 09:22:55PM +0200, Martin Blumenstingl wrote: > Hello Uwe, > > On Mon, Apr 1, 2019 at 8:38 PM Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > > > Hello Martin, > > > > On Mon, Apr 01, 2019 at 08:18:16PM +0200, Martin Blumenstingl wrote: > > > diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c > > > index f6e738ad7bd9..4b708c1fcb1d 100644 > > > --- a/drivers/pwm/pwm-meson.c > > > +++ b/drivers/pwm/pwm-meson.c > > > @@ -188,7 +188,7 @@ static int meson_pwm_calc(struct meson_pwm *meson, > > > do_div(fin_ps, fin_freq); > > > > > > /* Calc pre_div with the period */ > > > - for (pre_div = 0; pre_div < MISC_CLK_DIV_MASK; pre_div++) { > > > + 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", > > > > You could even calculate pre_div without the loop. > > > > Something like: > > > > u64 pre_div = (u64)period * rate; > > do_div_round_up(pre_div, NSEC_PER_SEC * 0xffff); > > pre_div--; > > > > (I didn't check rounding and maybe its off by one and ...) This would > > also get rid of the strange 1000 that is currently used in the > > calculation without a real benefit (unless I missed something). > personally I prefer using this simple patch applied first as it is > easy to review and (due to the Fixes tag) may get backported to stable > kernels. > I'm not saying I don't like your suggestion, I propose to postpone > implementing this cleanup. I need to have a closer look at the > calculation because three values are derived from the input clock rate > (pre_div, cnt, duty_cnt) and I don't want to mess up the cases that > are already working as of today. > > Please let me know what you think. That's also ok for me. In this case take my 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/ | ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] pwm: meson: consider 128 a valid pre-divider 2019-04-01 18:18 ` [PATCH 1/2] pwm: meson: consider 128 a valid pre-divider Martin Blumenstingl 2019-04-01 18:38 ` Uwe Kleine-König @ 2019-04-03 11:19 ` Neil Armstrong 1 sibling, 0 replies; 9+ messages in thread From: Neil Armstrong @ 2019-04-03 11:19 UTC (permalink / raw) To: Martin Blumenstingl, thierry.reding, linux-pwm, linux-amlogic Cc: jbrunet, linux-arm-kernel, linux-kernel, u.kleine-koenig, bichao.zheng On 01/04/2019 20:18, Martin Blumenstingl wrote: > The pre-divider allows configuring longer PWM periods compared to using > the input clock directly. The pre-divider is 7 bit wide, meaning it's > maximum value is 128 (the register value is off-by-one: 0x7f or 127). > > Change the loop to also allow for the maximum possible value to be > considered valid. > > Fixes: 211ed630753d2f ("pwm: Add support for Meson PWM Controller") > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > --- > drivers/pwm/pwm-meson.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c > index f6e738ad7bd9..4b708c1fcb1d 100644 > --- a/drivers/pwm/pwm-meson.c > +++ b/drivers/pwm/pwm-meson.c > @@ -188,7 +188,7 @@ static int meson_pwm_calc(struct meson_pwm *meson, > do_div(fin_ps, fin_freq); > > /* Calc pre_div with the period */ > - for (pre_div = 0; pre_div < MISC_CLK_DIV_MASK; pre_div++) { > + 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", > @@ -197,7 +197,7 @@ static int meson_pwm_calc(struct meson_pwm *meson, > break; > } > > - if (pre_div == MISC_CLK_DIV_MASK) { > + if (pre_div > MISC_CLK_DIV_MASK) { > dev_err(meson->chip.dev, "unable to get period pre_div\n"); > return -EINVAL; > } > Reviewed-by: Neil Armstrong <narmstrong@baylibre.com> ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2] pwm: meson: don't disable pwm when setting duty repeatedly 2019-04-01 18:18 [PATCH 0/2] pwm: meson: two small bug-fixes Martin Blumenstingl 2019-04-01 18:18 ` [PATCH 1/2] pwm: meson: consider 128 a valid pre-divider Martin Blumenstingl @ 2019-04-01 18:18 ` Martin Blumenstingl 2019-04-03 11:19 ` Neil Armstrong 2019-05-09 14:52 ` [PATCH 0/2] pwm: meson: two small bug-fixes Thierry Reding 2 siblings, 1 reply; 9+ messages in thread From: Martin Blumenstingl @ 2019-04-01 18:18 UTC (permalink / raw) To: thierry.reding, linux-pwm, linux-amlogic Cc: narmstrong, jbrunet, linux-arm-kernel, linux-kernel, u.kleine-koenig, bichao.zheng, Martin Blumenstingl From: Bichao Zheng <bichao.zheng@amlogic.com> There is an abnormally low about 20ms,when setting duty repeatedly. Because setting the duty will disable pwm and then enable. Delete this operation now. Fixes: 211ed630753d2f ("pwm: Add support for Meson PWM Controller") Signed-off-by: Bichao Zheng <bichao.zheng@amlogic.com> [ Dropped code instead of hiding it behind a comment ] Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> --- drivers/pwm/pwm-meson.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c index 4b708c1fcb1d..e247ab632530 100644 --- a/drivers/pwm/pwm-meson.c +++ b/drivers/pwm/pwm-meson.c @@ -325,11 +325,6 @@ static int meson_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, if (state->period != channel->state.period || state->duty_cycle != channel->state.duty_cycle || state->polarity != channel->state.polarity) { - if (channel->state.enabled) { - meson_pwm_disable(meson, pwm->hwpwm); - channel->state.enabled = false; - } - if (state->polarity != channel->state.polarity) { if (state->polarity == PWM_POLARITY_NORMAL) meson->inverter_mask |= BIT(pwm->hwpwm); -- 2.21.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] pwm: meson: don't disable pwm when setting duty repeatedly 2019-04-01 18:18 ` [PATCH 2/2] pwm: meson: don't disable pwm when setting duty repeatedly Martin Blumenstingl @ 2019-04-03 11:19 ` Neil Armstrong 0 siblings, 0 replies; 9+ messages in thread From: Neil Armstrong @ 2019-04-03 11:19 UTC (permalink / raw) To: Martin Blumenstingl, thierry.reding, linux-pwm, linux-amlogic Cc: jbrunet, linux-arm-kernel, linux-kernel, u.kleine-koenig, bichao.zheng On 01/04/2019 20:18, Martin Blumenstingl wrote: > From: Bichao Zheng <bichao.zheng@amlogic.com> > > There is an abnormally low about 20ms,when setting duty repeatedly. > Because setting the duty will disable pwm and then enable. Delete > this operation now. > > Fixes: 211ed630753d2f ("pwm: Add support for Meson PWM Controller") > Signed-off-by: Bichao Zheng <bichao.zheng@amlogic.com> > [ Dropped code instead of hiding it behind a comment ] > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > --- > drivers/pwm/pwm-meson.c | 5 ----- > 1 file changed, 5 deletions(-) > > diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c > index 4b708c1fcb1d..e247ab632530 100644 > --- a/drivers/pwm/pwm-meson.c > +++ b/drivers/pwm/pwm-meson.c > @@ -325,11 +325,6 @@ static int meson_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > if (state->period != channel->state.period || > state->duty_cycle != channel->state.duty_cycle || > state->polarity != channel->state.polarity) { > - if (channel->state.enabled) { > - meson_pwm_disable(meson, pwm->hwpwm); > - channel->state.enabled = false; > - } > - > if (state->polarity != channel->state.polarity) { > if (state->polarity == PWM_POLARITY_NORMAL) > meson->inverter_mask |= BIT(pwm->hwpwm); > Reviewed-by: Neil Armstrong <narmstrong@baylibre.com> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/2] pwm: meson: two small bug-fixes 2019-04-01 18:18 [PATCH 0/2] pwm: meson: two small bug-fixes Martin Blumenstingl 2019-04-01 18:18 ` [PATCH 1/2] pwm: meson: consider 128 a valid pre-divider Martin Blumenstingl 2019-04-01 18:18 ` [PATCH 2/2] pwm: meson: don't disable pwm when setting duty repeatedly Martin Blumenstingl @ 2019-05-09 14:52 ` Thierry Reding 2 siblings, 0 replies; 9+ messages in thread From: Thierry Reding @ 2019-05-09 14:52 UTC (permalink / raw) To: Martin Blumenstingl Cc: linux-pwm, linux-amlogic, narmstrong, jbrunet, linux-arm-kernel, linux-kernel, u.kleine-koenig, bichao.zheng [-- Attachment #1: Type: text/plain, Size: 1705 bytes --] On Mon, Apr 01, 2019 at 08:18:15PM +0200, Martin Blumenstingl wrote: > This series applies on top of my other fix "pwm: meson: fix scheduling > while atomic issue" from [0] > > The first patch fixes an issue where the maximum possible pre-divider > (128) could not be used because there was an off-by-one error in the > code. I discovered this while testing with the longest supported period > (349514407ns) when running from XTAL. This is verified to work on my > Meson8b Odroid-C1 board using "pwm_b" on GPIOX_11. > > The second patch was suggested by Uwe Kleine-König but was actually > implemented much earlier (back in mid 2018) by Bichao Zheng from > Amlogic. This patch fixes changing the duty cycle by relying on the > hardware to re-start the PWM output (instead of adding an artificial > "constant LOW" of about 20ms - as measured by Bichao Zheng when > stopping and re-starting the PWM output from within the driver). I > tested this fix on my Meson8b Odroid-C1 board which uses a PWM driven > CPU regulator (DVFS with all supported OPPs is still working fine for > me, although I couldn't observe any issues before this patch). > > I also have some code-improvements queued which I'll send in the next > days, see [1] > > > [0] https://patchwork.kernel.org/cover/10880419/ > [1] https://github.com/xdarklight/linux/commits/meson-pwm-for-5.2-v1 > > > Bichao Zheng (1): > pwm: meson: don't disable pwm when setting duty repeatedly > > Martin Blumenstingl (1): > pwm: meson: consider 128 a valid pre-divider > > drivers/pwm/pwm-meson.c | 9 ++------- > 1 file changed, 2 insertions(+), 7 deletions(-) Both patches applied, thanks. Thierry [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-05-09 14:53 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-04-01 18:18 [PATCH 0/2] pwm: meson: two small bug-fixes Martin Blumenstingl 2019-04-01 18:18 ` [PATCH 1/2] pwm: meson: consider 128 a valid pre-divider Martin Blumenstingl 2019-04-01 18:38 ` Uwe Kleine-König 2019-04-02 19:22 ` Martin Blumenstingl 2019-04-02 19:55 ` Uwe Kleine-König 2019-04-03 11:19 ` Neil Armstrong 2019-04-01 18:18 ` [PATCH 2/2] pwm: meson: don't disable pwm when setting duty repeatedly Martin Blumenstingl 2019-04-03 11:19 ` Neil Armstrong 2019-05-09 14:52 ` [PATCH 0/2] pwm: meson: two small bug-fixes Thierry Reding
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).