diff mbox series

[5/5] gpio: mvebu: document zero pwm duty cycle limitation

Message ID fa809a148e2a008c124677d61ea4ecba55cea7d0.1610362661.git.baruch@tkos.co.il
State Superseded
Headers show
Series gpio: mvebu: pwm fixes and improvements | expand

Commit Message

Baruch Siach Jan. 11, 2021, 11:17 a.m. UTC
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(+)

Comments

Uwe Kleine-König Jan. 11, 2021, 8:24 p.m. UTC | #1
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
Russell King (Oracle) Jan. 11, 2021, 10:43 p.m. UTC | #2
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!
diff mbox series

Patch

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 */
 	if (val)
 		on = val;
 	else