| Message ID | cover.1610362661.git.baruch@tkos.co.il |
|---|---|
| Headers | show |
| Series | gpio: mvebu: pwm fixes and improvements | expand |
Hello Baruch, $Subject ~= s/get_state/.get_state/ ? On Mon, Jan 11, 2021 at 01:17:02PM +0200, Baruch Siach wrote: > The period is the sum of on and off values. > > Reported-by: Russell King <linux@armlinux.org.uk> > Fixes: 757642f9a584e ("gpio: mvebu: Add limited PWM support") > Signed-off-by: Baruch Siach <baruch@tkos.co.il> > --- > drivers/gpio/gpio-mvebu.c | 19 ++++++++----------- > 1 file changed, 8 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c > index 672681a976f5..a912a8fed197 100644 > --- a/drivers/gpio/gpio-mvebu.c > +++ b/drivers/gpio/gpio-mvebu.c > @@ -676,20 +676,17 @@ static void mvebu_pwm_get_state(struct pwm_chip *chip, > else > state->duty_cycle = 1; > > + val = (unsigned long long) u; /* on duration */ > regmap_read(mvpwm->regs, mvebu_pwmreg_blink_off_duration(mvpwm), &u); > - val = (unsigned long long) u * NSEC_PER_SEC; > + val += (unsigned long long) u; /* period = on + off duration */ > + val *= NSEC_PER_SEC; > do_div(val, mvpwm->clk_rate); > - if (val < state->duty_cycle) { > + if (val > UINT_MAX) > + state->period = UINT_MAX; > + else if (val) > + state->period = val; > + else > state->period = 1; > - } else { > - val -= state->duty_cycle; > - if (val > UINT_MAX) > - state->period = UINT_MAX; > - else if (val) > - state->period = val; > - else > - state->period = 1; > - } The patch looks good, the patch description could be a bit more verbose. Something like: Calculate the period as ($on + $off) / clkrate instead of $off / clkrate - $on / clkrate . Best regards Uwe
On Mon, Jan 11, 2021 at 01:17:04PM +0200, Baruch Siach wrote: > Round up the result of division in period/duty_cycle calculation to make > the result closer to idempotent. > > Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > Signed-off-by: Baruch Siach <baruch@tkos.co.il> > --- > drivers/gpio/gpio-mvebu.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c > index c424d88e9e2b..6fc64846eda3 100644 > --- a/drivers/gpio/gpio-mvebu.c > +++ b/drivers/gpio/gpio-mvebu.c > @@ -668,7 +668,7 @@ static void mvebu_pwm_get_state(struct pwm_chip *chip, > > regmap_read(mvpwm->regs, mvebu_pwmreg_blink_on_duration(mvpwm), &u); > val = (unsigned long long) u * NSEC_PER_SEC; > - do_div(val, mvpwm->clk_rate); > + val = DIV_ROUND_UP_ULL(val, mvpwm->clk_rate); > if (val > UINT_MAX) > state->duty_cycle = UINT_MAX; > else if (val) > @@ -680,7 +680,7 @@ static void mvebu_pwm_get_state(struct pwm_chip *chip, > regmap_read(mvpwm->regs, mvebu_pwmreg_blink_off_duration(mvpwm), &u); > val += (unsigned long long) u; /* period = on + off duration */ > val *= NSEC_PER_SEC; > - do_div(val, mvpwm->clk_rate); > + val = DIV_ROUND_UP_ULL(val, mvpwm->clk_rate); > if (val > UINT_MAX) > state->period = UINT_MAX; > else if (val) > @@ -707,7 +707,7 @@ static int mvebu_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > unsigned int on, off; > > val = (unsigned long long) mvpwm->clk_rate * state->duty_cycle; > - do_div(val, NSEC_PER_SEC); > + val = DIV_ROUND_UP_ULL(val, NSEC_PER_SEC); .apply must continue to round down. Best regards Uwe > if (val > UINT_MAX) > return -EINVAL; > if (val) > @@ -716,7 +716,7 @@ static int mvebu_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > on = 1; > > val = (unsigned long long) mvpwm->clk_rate * state->period; > - do_div(val, NSEC_PER_SEC); > + val = DIV_ROUND_UP_ULL(val, NSEC_PER_SEC); > val -= on; > if (val > UINT_MAX) > return -EINVAL; > -- > 2.29.2 > >
On Mon, Jan 11, 2021 at 01:17:06PM +0200, Baruch Siach wrote: > Add a comment on why the code never sets the 'on' register to zero. > > Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > Signed-off-by: Baruch Siach <baruch@tkos.co.il> > --- > drivers/gpio/gpio-mvebu.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c > index eb7456fa6d86..4261e3b22b4e 100644 > --- a/drivers/gpio/gpio-mvebu.c > +++ b/drivers/gpio/gpio-mvebu.c > @@ -706,6 +706,7 @@ static int mvebu_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > val = DIV_ROUND_UP_ULL(val, NSEC_PER_SEC); > if (val > UINT_MAX) > return -EINVAL; > + /* zero 'on' value does not work as expected for some reason */ What does the reference manual say about this? If there is no information about this, please point this out, too. (Something like: The reference manual is silent about this issue though.) Also I'd prefer to read about the behaviour, so maybe mention that there is an occational peek even when on is configured to 0. Does '$off = 0' has a symmetrical issue? Best regards Uwe
On Mon, Jan 11, 2021 at 09:24:13PM +0100, Uwe Kleine-König wrote: > On Mon, Jan 11, 2021 at 01:17:06PM +0200, Baruch Siach wrote: > > Add a comment on why the code never sets the 'on' register to zero. > > > > Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > Signed-off-by: Baruch Siach <baruch@tkos.co.il> > > --- > > drivers/gpio/gpio-mvebu.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c > > index eb7456fa6d86..4261e3b22b4e 100644 > > --- a/drivers/gpio/gpio-mvebu.c > > +++ b/drivers/gpio/gpio-mvebu.c > > @@ -706,6 +706,7 @@ static int mvebu_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > val = DIV_ROUND_UP_ULL(val, NSEC_PER_SEC); > > if (val > UINT_MAX) > > return -EINVAL; > > + /* zero 'on' value does not work as expected for some reason */ > > What does the reference manual say about this? If there is no > information about this, please point this out, too. (Something like: The > reference manual is silent about this issue though.) Also I'd prefer to > read about the behaviour, so maybe mention that there is an occational > peek even when on is configured to 0. Does '$off = 0' has a symmetrical > issue? It isn't a proper PWM block - it's documented as being a "blink function". It contains two counters, one defines the "on" duration, and the other defines the "off" duration. The block is not well documented in the reference manual, so we have to resort to experimentation - and experimentation reveals that if we program both registers to zero, then we get about 17s on and 17s off. That is 2^32 / 250MHz seconds. So, a value of 0 in either register is interpreted by the hardware as a value of 2^32. So, let's say we want a 25kHz signal. If we program the "on" duration to 10000 and the "off" duration to 0, what we actually get a 40us on duration, and a 17.2s off duration - resulting in a frequency of 0.058Hz! -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Hi Uwe, On Mon, Jan 11 2021, Uwe Kleine-König wrote: > $Subject ~= s/get_state/.get_state/ ? Ack. > On Mon, Jan 11, 2021 at 01:17:02PM +0200, Baruch Siach wrote: >> The period is the sum of on and off values. >> >> Reported-by: Russell King <linux@armlinux.org.uk> >> Fixes: 757642f9a584e ("gpio: mvebu: Add limited PWM support") >> Signed-off-by: Baruch Siach <baruch@tkos.co.il> >> --- >> drivers/gpio/gpio-mvebu.c | 19 ++++++++----------- >> 1 file changed, 8 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c >> index 672681a976f5..a912a8fed197 100644 >> --- a/drivers/gpio/gpio-mvebu.c >> +++ b/drivers/gpio/gpio-mvebu.c >> @@ -676,20 +676,17 @@ static void mvebu_pwm_get_state(struct pwm_chip *chip, >> else >> state->duty_cycle = 1; >> >> + val = (unsigned long long) u; /* on duration */ >> regmap_read(mvpwm->regs, mvebu_pwmreg_blink_off_duration(mvpwm), &u); >> - val = (unsigned long long) u * NSEC_PER_SEC; >> + val += (unsigned long long) u; /* period = on + off duration */ >> + val *= NSEC_PER_SEC; >> do_div(val, mvpwm->clk_rate); >> - if (val < state->duty_cycle) { >> + if (val > UINT_MAX) >> + state->period = UINT_MAX; >> + else if (val) >> + state->period = val; >> + else >> state->period = 1; >> - } else { >> - val -= state->duty_cycle; >> - if (val > UINT_MAX) >> - state->period = UINT_MAX; >> - else if (val) >> - state->period = val; >> - else >> - state->period = 1; >> - } > > The patch looks good, the patch description could be a bit more verbose. > Something like: > > Calculate the period as > > ($on + $off) / clkrate > > instead of > > $off / clkrate - $on / clkrate > > . I take this to refer to the next patch (2/5). This patch changes from buggy $on / clkrate to ($on + $off) / clkrate baruch -- ~. .~ Tk Open Systems =}------------------------------------------------ooO--U--Ooo------------{= - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -
On Wed, Jan 13, 2021 at 08:36:12AM +0200, Baruch Siach wrote: > Hi Uwe, > > On Mon, Jan 11 2021, Uwe Kleine-König wrote: > > $Subject ~= s/get_state/.get_state/ ? > > Ack. > > > On Mon, Jan 11, 2021 at 01:17:02PM +0200, Baruch Siach wrote: > >> The period is the sum of on and off values. > >> > >> Reported-by: Russell King <linux@armlinux.org.uk> > >> Fixes: 757642f9a584e ("gpio: mvebu: Add limited PWM support") > >> Signed-off-by: Baruch Siach <baruch@tkos.co.il> > >> --- > >> drivers/gpio/gpio-mvebu.c | 19 ++++++++----------- > >> 1 file changed, 8 insertions(+), 11 deletions(-) > >> > >> diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c > >> index 672681a976f5..a912a8fed197 100644 > >> --- a/drivers/gpio/gpio-mvebu.c > >> +++ b/drivers/gpio/gpio-mvebu.c > >> @@ -676,20 +676,17 @@ static void mvebu_pwm_get_state(struct pwm_chip *chip, > >> else > >> state->duty_cycle = 1; > >> > >> + val = (unsigned long long) u; /* on duration */ > >> regmap_read(mvpwm->regs, mvebu_pwmreg_blink_off_duration(mvpwm), &u); > >> - val = (unsigned long long) u * NSEC_PER_SEC; > >> + val += (unsigned long long) u; /* period = on + off duration */ > >> + val *= NSEC_PER_SEC; > >> do_div(val, mvpwm->clk_rate); > >> - if (val < state->duty_cycle) { > >> + if (val > UINT_MAX) > >> + state->period = UINT_MAX; > >> + else if (val) > >> + state->period = val; > >> + else > >> state->period = 1; > >> - } else { > >> - val -= state->duty_cycle; > >> - if (val > UINT_MAX) > >> - state->period = UINT_MAX; > >> - else if (val) > >> - state->period = val; > >> - else > >> - state->period = 1; > >> - } > > > > The patch looks good, the patch description could be a bit more verbose. > > Something like: > > > > Calculate the period as > > > > ($on + $off) / clkrate > > > > instead of > > > > $off / clkrate - $on / clkrate > > > > . > > I take this to refer to the next patch (2/5). This patch changes from > buggy > > $on / clkrate No, the previous calculation had - val -= state->duty_cycle; which accounts for "- $on / clkrate". Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |