[PATCH v3 0/5] gpio: mvebu: pwm fixes and improvements

[PATCH v3 0/5] gpio: mvebu: pwm fixes and improvements

[Baruch Siach]

This series adds a few related fixes to the pwm .apply and .get_state 
callbacks.

The first patch was originally part of the series adding Armada 8K/7K pwm 
support. I split it out to a separate series following review comments from 
Uwe Kleine-König who spotted a few more issues. There is no dependency between 
this and the Armada 8K/7K series.

v3:

  * Improve patch 3/5 description (Uwe)

  * Add more Reviewed-by tags from Uwe

v2:

Address Uwe Kleine-König comments.

  * Improve patch 1/5 summary line

  * Add more information to patch 1/5 description

  * Add more information to patch 2/5 description

  * Don't round period/duty_cycle up in .apply (patch 3/5)

  * Expand the comment in path 5/5 based on RMK's analysis of hardware 
    behaviour

  * Add Uwe's Reviewed-by tags

Baruch Siach (5):
  gpio: mvebu: fix pwm .get_state period calculation
  gpio: mvebu: improve pwm period calculation accuracy
  gpio: mvebu: make pwm .get_state closer to idempotent
  gpio: mvebu: don't limit pwm period/duty_cycle to UINT_MAX
  gpio: mvebu: document zero pwm duty cycle limitation

 drivers/gpio/gpio-mvebu.c | 33 +++++++++++++++------------------
 1 file changed, 15 insertions(+), 18 deletions(-)

-- 
2.29.2

[PATCH v3 1/5] gpio: mvebu: fix pwm .get_state period calculation

[Baruch Siach]

The period is the sum of on and off values. That is, calculate period as

  ($on + $off) / clkrate

instead of

  $off / clkrate - $on / clkrate

that makes no sense.

Reported-by: Russell King <linux@armlinux.org.uk>
Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
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;
-	}
 
 	regmap_read(mvchip->regs, GPIO_BLINK_EN_OFF + mvchip->offset, &u);
 	if (u)
-- 
2.29.2

[PATCH v3 2/5] gpio: mvebu: improve pwm period calculation accuracy

[Baruch Siach]

Change 'off' register value calculation from

  $off = (period - duty_cycle) * clkrate / NSEC_PER_SEC

to

  $off = (period * clkrate / NSEC_PER_SEC) - $on

That is, divide the full period value to reduce rounding error.

Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
 drivers/gpio/gpio-mvebu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
index a912a8fed197..c424d88e9e2b 100644
--- a/drivers/gpio/gpio-mvebu.c
+++ b/drivers/gpio/gpio-mvebu.c
@@ -715,9 +715,9 @@ static int mvebu_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	else
 		on = 1;
 
-	val = (unsigned long long) mvpwm->clk_rate *
-		(state->period - state->duty_cycle);
+	val = (unsigned long long) mvpwm->clk_rate * state->period;
 	do_div(val, NSEC_PER_SEC);
+	val -= on;
 	if (val > UINT_MAX)
 		return -EINVAL;
 	if (val)
-- 
2.29.2

[PATCH v3 3/5] gpio: mvebu: make pwm .get_state closer to idempotent

[Baruch Siach]

Round up the divisions in .get_state() to make applying the read out
configuration idempotent in most cases as .apply rounds down.

Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
 drivers/gpio/gpio-mvebu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
index c424d88e9e2b..8673ba77af5a 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)
-- 
2.29.2

[PATCH v3 4/5] gpio: mvebu: don't limit pwm period/duty_cycle to UINT_MAX

[Baruch Siach]

PWM on/off registers are limited to UINT_MAX. However the state period
and duty_cycle fields are ns values of type u64. There is no reason to
limit them to UINT_MAX.

Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Reviewed-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, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
index 8673ba77af5a..6b017854ce61 100644
--- a/drivers/gpio/gpio-mvebu.c
+++ b/drivers/gpio/gpio-mvebu.c
@@ -669,9 +669,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;
 	val = DIV_ROUND_UP_ULL(val, mvpwm->clk_rate);
-	if (val > UINT_MAX)
-		state->duty_cycle = UINT_MAX;
-	else if (val)
+	if (val)
 		state->duty_cycle = val;
 	else
 		state->duty_cycle = 1;
@@ -681,9 +679,7 @@ static void mvebu_pwm_get_state(struct pwm_chip *chip,
 	val += (unsigned long long) u; /* period = on + off duration */
 	val *= NSEC_PER_SEC;
 	val = DIV_ROUND_UP_ULL(val, mvpwm->clk_rate);
-	if (val > UINT_MAX)
-		state->period = UINT_MAX;
-	else if (val)
+	if (val)
 		state->period = val;
 	else
 		state->period = 1;
-- 
2.29.2

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

[Baruch Siach]

Add a comment on why the code never sets on/off registers to zero.

Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Analyzed-by: Russell King <linux@armlinux.org.uk>
Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
 drivers/gpio/gpio-mvebu.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
index 6b017854ce61..09780944bef9 100644
--- a/drivers/gpio/gpio-mvebu.c
+++ b/drivers/gpio/gpio-mvebu.c
@@ -706,6 +706,10 @@ static int mvebu_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	do_div(val, NSEC_PER_SEC);
 	if (val > UINT_MAX)
 		return -EINVAL;
+	/*
+	 * Zero on/off values don't work as expected. Experimentation shows
+	 * that zero value is treated as 2^32. This behavior is not documented.
+	 */
 	if (val)
 		on = val;
 	else
-- 
2.29.2

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

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1224 bytes --]

Hello Baruch,

On Thu, Jan 14, 2021 at 08:57:37PM +0200, Baruch Siach wrote:
> Add a comment on why the code never sets on/off registers to zero.
> 
> Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Analyzed-by: Russell King <linux@armlinux.org.uk>
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> ---
>  drivers/gpio/gpio-mvebu.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
> index 6b017854ce61..09780944bef9 100644
> --- a/drivers/gpio/gpio-mvebu.c
> +++ b/drivers/gpio/gpio-mvebu.c
> @@ -706,6 +706,10 @@ static int mvebu_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	do_div(val, NSEC_PER_SEC);
>  	if (val > UINT_MAX)
>  		return -EINVAL;
> +	/*
> +	 * Zero on/off values don't work as expected. Experimentation shows
> +	 * that zero value is treated as 2^32. This behavior is not documented.
> +	 */

This is too easy. The right thing to do is to adapt .apply and
.get_state to use this new information.

Best regards
Uwe

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Russell King - ARM Linux admin]

On Thu, Jan 14, 2021 at 09:25:45PM +0100, Uwe Kleine-König wrote:
> Hello Baruch,
> 
> On Thu, Jan 14, 2021 at 08:57:37PM +0200, Baruch Siach wrote:
> > Add a comment on why the code never sets on/off registers to zero.
> > 
> > Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > Analyzed-by: Russell King <linux@armlinux.org.uk>
> > Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> > ---
> >  drivers/gpio/gpio-mvebu.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
> > index 6b017854ce61..09780944bef9 100644
> > --- a/drivers/gpio/gpio-mvebu.c
> > +++ b/drivers/gpio/gpio-mvebu.c
> > @@ -706,6 +706,10 @@ static int mvebu_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> >  	do_div(val, NSEC_PER_SEC);
> >  	if (val > UINT_MAX)
> >  		return -EINVAL;
> > +	/*
> > +	 * Zero on/off values don't work as expected. Experimentation shows
> > +	 * that zero value is treated as 2^32. This behavior is not documented.
> > +	 */
> 
> This is too easy. The right thing to do is to adapt .apply and
> .get_state to use this new information.

What exactly do you expect the changes to be?

Bear in mind that the hardware is not capable of atomically updating
e.g. the duty cycle without affecting the period, because any change
in duty cycle needs the "on" and "off" durations to be separately
programmed, and there's a chance that the hardware could start using
either value mid-update.

Also, disabling "blink" mode to achieve a steady output (for 0% or 100%
duty cycle) would require further investigation to find out how the
hardware behaves at the moment where blink mode is disabled: does the
output retain its current state (does the bit in the output register
toggle with the blink) or does it revert to the value in the output
register that was programmed before blink mode was enabled.

Again, none of that is documented, so would need experimentation with
the hardware to work out how to achieve it.

And then if you want even more complexity, I suppose we could try and
read the current state of the pin, add a delay, recheck it and try and
work out the optimal place to disable the blink mode.

Exactly how far do you want to go with this?

All of this is likely getting rediculously complicated for the use
cases of it today that don't need it. Yes, it's annoying that we can't
achieve 0% or 100% duty cycle with this hardware that was never
designed as a PWM without jumping through a lot of hoops but currently
settle for a minimum pulse width of 4ns at each end of the range.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

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

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2674 bytes --]

On Thu, Jan 14, 2021 at 10:28:02PM +0000, Russell King - ARM Linux admin wrote:
> On Thu, Jan 14, 2021 at 09:25:45PM +0100, Uwe Kleine-König wrote:
> > Hello Baruch,
> > 
> > On Thu, Jan 14, 2021 at 08:57:37PM +0200, Baruch Siach wrote:
> > > Add a comment on why the code never sets on/off registers to zero.
> > > 
> > > Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > Analyzed-by: Russell King <linux@armlinux.org.uk>
> > > Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> > > ---
> > >  drivers/gpio/gpio-mvebu.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
> > > index 6b017854ce61..09780944bef9 100644
> > > --- a/drivers/gpio/gpio-mvebu.c
> > > +++ b/drivers/gpio/gpio-mvebu.c
> > > @@ -706,6 +706,10 @@ static int mvebu_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > >  	do_div(val, NSEC_PER_SEC);
> > >  	if (val > UINT_MAX)
> > >  		return -EINVAL;
> > > +	/*
> > > +	 * Zero on/off values don't work as expected. Experimentation shows
> > > +	 * that zero value is treated as 2^32. This behavior is not documented.
> > > +	 */
> > 
> > This is too easy. The right thing to do is to adapt .apply and
> > .get_state to use this new information.
> 
> What exactly do you expect the changes to be?

What I expect is:

 - let .apply() write 0 if the intention is to configure 2^32 clock
   steps for the on or off register; and symmetrically
 - let .get_state report 2^32 * NSEC_PER_SEC / clk_rate if the register
   value is 0.
 
> Bear in mind that the hardware is not capable of atomically updating
> e.g. the duty cycle without affecting the period, because any change
> in duty cycle needs the "on" and "off" durations to be separately
> programmed, and there's a chance that the hardware could start using
> either value mid-update.
> 
> Also, disabling "blink" mode to achieve a steady output (for 0% or 100%
> duty cycle) would require further investigation to find out how the
> hardware behaves at the moment where blink mode is disabled: does the
> output retain its current state (does the bit in the output register
> toggle with the blink) or does it revert to the value in the output
> register that was programmed before blink mode was enabled.

I have some plans here about what is the right behaviour, but this needs
some preparatory work that I didn't do yet. I'll come back to this
eventually.

Best regards
Uwe

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]