Skip to contentSkip to conversation

gpio: mvebu: pwm fixes and improvements

PATCH Superseded Baruch Siach 3 participants 14 messages Jan 11, 2021 – Jan 13, 2021

Subsystems (2)
GPIO PWM

v1 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Baruch Siach

author
#0 lore raw

cover gpio: mvebu: pwm fixes and improvements

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.

Baruch Siach (5):
gpio: mvebu: fix pwm get_state period calculation
gpio: mvebu: improve pwm period calculation accuracy
gpio: mvebu: make pwm apply/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 | 34 ++++++++++++++--------------------
 1 file changed, 14 insertions(+), 20 deletions(-)
Signature · 1 line
2.29.2
author
#1 lore raw

PATCH 1/5 gpio: mvebu: fix pwm get_state period calculation

The period is the sum of on and off values.

Reported-by Russell King <[email protected]>
Fixes 757642f9a584e ("gpio: mvebu: Add limited PWM support")
Signed-off-by Baruch Siach <[email protected]>
---
 drivers/gpio/gpio-mvebu.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)
drivers/gpio/gpio-mvebu.c +8 -11
index 672681a976f5..a912a8fed197 100644
@@ -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)
Signature · 1 line
2.29.2
#2 lore raw

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

Hello Baruch,

$Subject ~= s/get_state/.get_state/ ?

Quoted 29 lines · Baruch Siach

The period is the sum of on and off values.

Reported-by Russell King <[email protected]>
Fixes 757642f9a584e ("gpio: mvebu: Add limited PWM support")
Signed-off-by Baruch Siach <[email protected]>
---
 drivers/gpio/gpio-mvebu.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)
drivers/gpio/gpio-mvebu.c +8 -2
index 672681a976f5..a912a8fed197 100644
@@ -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

drivers/gpio/gpio-mvebu.c static void mvebu_pwm_get_state(struct pwm_chip *chip, · line 692

Quoted 10 lines
drivers/gpio/gpio-mvebu.c +0 -9
 		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

Signature · 2 lines
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |
author
#3 lore raw

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

Hi Uwe,

Quoted 1 line · Uwe Kleine-König

$Subject ~= s/get_state/.get_state/ ?

Ack.

Quoted 44 lines · Baruch Siach
Quoted 29 lines

The period is the sum of on and off values.

Reported-by Russell King <[email protected]>
Fixes 757642f9a584e ("gpio: mvebu: Add limited PWM support")
Signed-off-by Baruch Siach <[email protected]>
---
 drivers/gpio/gpio-mvebu.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)
drivers/gpio/gpio-mvebu.c +8 -2
index 672681a976f5..a912a8fed197 100644
@@ -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

drivers/gpio/gpio-mvebu.c static void mvebu_pwm_get_state(struct pwm_chip *chip, · line 692

Quoted 10 lines
drivers/gpio/gpio-mvebu.c +0 -9
 		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:

Quoted 9 lines
	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

Signature · 3 lines
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - [email protected] - tel: +972.52.368.4656, http://www.tkos.co.il -
#4 lore raw

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

Quoted 57 lines · Baruch Siach

Hi Uwe,

Quoted 1 line · Uwe Kleine-König

$Subject ~= s/get_state/.get_state/ ?

Ack.

Quoted 50 lines · Baruch Siach
Quoted 29 lines

The period is the sum of on and off values.

Reported-by Russell King <[email protected]>
Fixes 757642f9a584e ("gpio: mvebu: Add limited PWM support")
Signed-off-by Baruch Siach <[email protected]>
---
 drivers/gpio/gpio-mvebu.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)
drivers/gpio/gpio-mvebu.c +8 -2
index 672681a976f5..a912a8fed197 100644
@@ -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

drivers/gpio/gpio-mvebu.c static void mvebu_pwm_get_state(struct pwm_chip *chip, · line 692

Quoted 10 lines
drivers/gpio/gpio-mvebu.c +0 -9
 		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
Quoted 8 lines
Quoted 3 lines
		$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

Signature · 2 lines
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |
author
#5 lore raw

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

In reply to Baruch Siach

Divide the full period value to reduce rounding error.

Reported-by Uwe Kleine-König <[email protected]>
Signed-off-by Baruch Siach <[email protected]>
---
 drivers/gpio/gpio-mvebu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/gpio/gpio-mvebu.c +2 -2
index a912a8fed197..c424d88e9e2b 100644
@@ -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)
Signature · 1 line
2.29.2
Reviewed-by on v1 2/5
#6 lore raw

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

Quoted 4 lines · Baruch Siach

Divide the full period value to reduce rounding error.

Reported-by Uwe Kleine-König <[email protected]>
Signed-off-by Baruch Siach <[email protected]>
Reviewed-by Uwe Kleine-König <[email protected]>

Thanks
Uwe

Signature · 2 lines
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |
author
#7 lore raw

PATCH 3/5 gpio: mvebu: make pwm apply/get_state closer to idempotent

In reply to Baruch Siach

Round up the result of division in period/duty_cycle calculation to make the result closer to idempotent.

Reported-by Uwe Kleine-König <[email protected]>
Signed-off-by Baruch Siach <[email protected]>
---
 drivers/gpio/gpio-mvebu.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/gpio/gpio-mvebu.c +4 -4
index c424d88e9e2b..6fc64846eda3 100644
@@ -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);
 	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;
Signature · 1 line
2.29.2
#8 lore raw

Re: [PATCH 3/5] gpio: mvebu: make pwm apply/get_state closer to idempotent

Quoted 32 lines · Baruch Siach

Round up the result of division in period/duty_cycle calculation to make the result closer to idempotent.

Reported-by Uwe Kleine-König <[email protected]>
Signed-off-by Baruch Siach <[email protected]>
---
 drivers/gpio/gpio-mvebu.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/gpio/gpio-mvebu.c +2 -2
index c424d88e9e2b..6fc64846eda3 100644
@@ -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)

drivers/gpio/gpio-mvebu.c static int mvebu_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, · line 710

Quoted 5 lines
drivers/gpio/gpio-mvebu.c +1 -1
@@ -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

Quoted 16 lines
drivers/gpio/gpio-mvebu.c +1 -1
 	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;
Signature · 1 line
2.29.2
Signature · 2 lines
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |
author
#9 lore raw

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

In reply to 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 <[email protected]>
Signed-off-by Baruch Siach <[email protected]>
---
 drivers/gpio/gpio-mvebu.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)
drivers/gpio/gpio-mvebu.c +2 -6
index 6fc64846eda3..eb7456fa6d86 100644
@@ -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;
Signature · 1 line
2.29.2
Reviewed-by on v1 4/5
#10 lore raw

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

Quoted 6 lines · 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 <[email protected]>
Signed-off-by Baruch Siach <[email protected]>
Reviewed-by Uwe Kleine-König <[email protected]>

Thanks
Uwe

Signature · 2 lines
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |
author
#11 lore raw

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

In reply to Baruch Siach

Add a comment on why the code never sets the 'on' register to zero.

Reported-by Uwe Kleine-König <[email protected]>
Signed-off-by Baruch Siach <[email protected]>
---
 drivers/gpio/gpio-mvebu.c | 1 +
 1 file changed, 1 insertion(+)
drivers/gpio/gpio-mvebu.c +1 -0
index eb7456fa6d86..4261e3b22b4e 100644
@@ -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
Signature · 1 line
2.29.2
#12 lore raw

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

Quoted 13 lines · Baruch Siach

Add a comment on why the code never sets the 'on' register to zero.

Reported-by Uwe Kleine-König <[email protected]>
Signed-off-by Baruch Siach <[email protected]>
---
 drivers/gpio/gpio-mvebu.c | 1 +
 1 file changed, 1 insertion(+)
drivers/gpio/gpio-mvebu.c +0 -0
index eb7456fa6d86..4261e3b22b4e 100644

drivers/gpio/gpio-mvebu.c static int mvebu_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, · line 709

Quoted 4 lines
drivers/gpio/gpio-mvebu.c +1 -0
@@ -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

Signature · 2 lines
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |
#13 lore raw

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

Quoted 9 lines · Uwe Kleine-König
Quoted 8 lines · Baruch Siach

Add a comment on why the code never sets the 'on' register to zero.

Reported-by Uwe Kleine-König <[email protected]>
Signed-off-by Baruch Siach <[email protected]>
---
 drivers/gpio/gpio-mvebu.c | 1 +
 1 file changed, 1 insertion(+)
Quoted 16 lines

drivers/gpio/gpio-mvebu.c static int mvebu_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, · line 709

Quoted 9 lines
drivers/gpio/gpio-mvebu.c +1 -0
index eb7456fa6d86..4261e3b22b4e 100644
@@ -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!

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

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel