gpio: mvebu: pwm fixes and improvements
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
PATCH 1/5 gpio: mvebu: fix pwm get_state period calculation
The period is the sum of on and off values.
--- drivers/gpio/gpio-mvebu.c | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-)
drivers/gpio/gpio-mvebu.c +8 -11
@@ -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
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@@ -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
-9state->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/ |
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@@ -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
-9state->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 -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@@ -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
-9state->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/ |
PATCH 2/5 gpio: mvebu: improve pwm period calculation accuracy
In reply to Baruch SiachDivide the full period value to reduce rounding error.
--- drivers/gpio/gpio-mvebu.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/gpio/gpio-mvebu.c +2 -2
@@ -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
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]>
Thanks
Uwe
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
PATCH 3/5 gpio: mvebu: make pwm apply/get_state closer to idempotent
In reply to Baruch SiachRound up the result of division in period/duty_cycle calculation to make the result closer to idempotent.
--- drivers/gpio/gpio-mvebu.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/gpio/gpio-mvebu.c +4 -4
@@ -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
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@@ -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
-1if (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/ |
PATCH 4/5 gpio: mvebu: don't limit pwm period/duty_cycle to UINT_MAX
In reply to Baruch SiachPWM 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.
--- drivers/gpio/gpio-mvebu.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
drivers/gpio/gpio-mvebu.c +2 -6
@@ -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
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 andduty_cyclefields are ns values of type u64. There is no reason to limit them toUINT_MAX.Reported-by Uwe Kleine-König <[email protected]>Signed-off-by Baruch Siach <[email protected]>
Thanks
Uwe
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
PATCH 5/5 gpio: mvebu: document zero pwm duty cycle limitation
In reply to Baruch SiachAdd a comment on why the code never sets the 'on' register to zero.
--- drivers/gpio/gpio-mvebu.c | 1 + 1 file changed, 1 insertion(+)
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 */ if (val) on = val;else
Signature · 1 line
2.29.2
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
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/ |
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@@ -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