gpio: mvebu: pwm fixes and improvements
v4 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.
v4:
* Take advantage of zero value being treated as 2^32 by hardware. Rewrite
patch 5/5 (Uwe).
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: improve handling of pwm zero on/off values
drivers/gpio/gpio-mvebu.c | 51 +++++++++++++++++++-------------------- 1 file changed, 25 insertions(+), 26 deletions(-)
Signature · 1 line
2.29.2
PATCH v4 1/5 gpio: mvebu: fix pwm .get_state period calculation
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.
--- 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
Bartosz Golaszewski
AppliedRe: [PATCH v4 1/5] gpio: mvebu: fix pwm .get_state period calculation
Quoted 53 lines · 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 <[email protected]>Reviewed-by Uwe Kleine-König <[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
-12@@ -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) --
Quoted 2 lines
2.29.2
I applied this for fixes and will send out a PR to Linus T this week. Once that's upstream I'll apply the rest for the next release.
Bartosz
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
PATCH v4 2/5 gpio: mvebu: improve pwm period calculation accuracy
In reply to Baruch SiachChange '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.
--- 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
PATCH v4 3/5 gpio: mvebu: make pwm .get_state closer to idempotent
In reply to Baruch SiachRound up the divisions in .get_state() to make applying the read out configuration idempotent in most cases as .apply rounds down.
--- drivers/gpio/gpio-mvebu.c | 4 ++-- 1 file changed, 2 insertions(+), 2 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)
Signature · 1 line
2.29.2
PATCH v4 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
PATCH v4 5/5 gpio: mvebu: improve handling of pwm zero on/off values
In reply to Baruch SiachHardware appears to treat zero value as 2^32. Take advantage of this fact to support on/off values of up to UINT_MAX+1 == 2^32. Adjust both .apply and .get_sate to handle zero as a special case.
Rounded up division result in .get_state can't be zero, since the dividend is now larger than 0. Remove check for this case.
--- drivers/gpio/gpio-mvebu.c | 38 ++++++++++++++++++++++---------------- 1 file changed, 22 insertions(+), 16 deletions(-)
drivers/gpio/gpio-mvebu.c +22 -16
@@ -667,22 +667,20 @@ static void mvebu_pwm_get_state(struct pwm_chip *chip,spin_lock_irqsave (&mvpwm->lock, flags);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) - state->duty_cycle = val; +/* Hardware treats zero as 2^32. See mvebu_pwm_apply(). */ +if (u >0 ) + val = (unsigned long long ) u *NSEC_PER_SEC ;else - state->duty_cycle =1 ; + val = ((unsigned long long )UINT_MAX +1 ) *NSEC_PER_SEC ; + state->duty_cycle =DIV_ROUND_UP_ULL (val, mvpwm->clk_rate); - val = (unsigned long long ) u;/* on duration */ regmap_read (mvpwm->regs,mvebu_pwmreg_blink_off_duration (mvpwm), &u); - val += (unsigned long long ) u;/* period = on + off duration */ - val *=NSEC_PER_SEC ; - val =DIV_ROUND_UP_ULL (val, mvpwm->clk_rate); -if (val) - state->period = val; +/* period = on + off duration */ +if (u >0 ) + val += (unsigned long long ) u *NSEC_PER_SEC ;else - state->period =1 ; + val += ((unsigned long long )UINT_MAX +1 ) *NSEC_PER_SEC ; + state->period =DIV_ROUND_UP_ULL (val, mvpwm->clk_rate);regmap_read (mvchip->regs,GPIO_BLINK_EN_OFF + mvchip->offset, &u);if (u)
@@ -704,9 +702,15 @@ static int mvebu_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, val = (unsigned long long ) mvpwm->clk_rate * state->duty_cycle;do_div (val,NSEC_PER_SEC ); -if (val >UINT_MAX ) +if (val > (unsigned long long )UINT_MAX +1 )return -EINVAL ; -if (val) +/* +* 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 == (unsigned long long )UINT_MAX +1 ) + on =0 ; +else if (val) on = val;else on =1 ;
@@ -714,9 +718,11 @@ static int mvebu_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, val = (unsigned long long ) mvpwm->clk_rate * state->period;do_div (val,NSEC_PER_SEC ); val -= on; -if (val >UINT_MAX ) +if (val > (unsigned long long )UINT_MAX +1 )return -EINVAL ; -if (val) +if (val == (unsigned long long )UINT_MAX +1 ) + off =0 ; +else if (val) off = val;else off =1 ;
Signature · 1 line
2.29.2
Re: [PATCH v4 5/5] gpio: mvebu: improve handling of pwm zero on/off values
Hello Baruch,
Quoted 3 lines · Baruch Siach
Hardware appears to treat zero value as 2^32. Take advantage of this fact to support on/off values of up to UINT_MAX+1 == 2^32. Adjust both .apply and .get_sate to handle zero as a special case.
s/get_sate/get_state/
Quoted 19 lines
Rounded up division result in .get_state can't be zero, since the dividend is now larger than 0. Remove check for this case.
Reported-by Uwe Kleine-König <[email protected]>Analyzed-by Russell King <[email protected]>Signed-off-by Baruch Siach <[email protected]>--- drivers/gpio/gpio-mvebu.c | 38 ++++++++++++++++++++++---------------- 1 file changed, 22 insertions(+), 16 deletions(-)drivers/gpio/gpio-mvebu.c +0
-1@@ -667,22 +667,20 @@ static void mvebu_pwm_get_state(struct pwm_chip *chip,spin_lock_irqsave (&mvpwm->lock, flags);regmap_read (mvpwm->regs,mvebu_pwmreg_blink_on_duration (mvpwm), &u); - val = (unsigned long long ) u *NSEC_PER_SEC ;
drivers/gpio/gpio-mvebu.c static void mvebu_pwm_get_state(struct pwm_chip *chip, · line 675
Quoted 10 lines
drivers/gpio/gpio-mvebu.c +5
-4- val =DIV_ROUND_UP_ULL (val, mvpwm->clk_rate); -if (val) - state->duty_cycle = val; +/* Hardware treats zero as 2^32. See mvebu_pwm_apply(). */ +if (u >0 ) + val = (unsigned long long ) u *NSEC_PER_SEC ;else - state->duty_cycle =1 ; + val = ((unsigned long long )UINT_MAX +1 ) *NSEC_PER_SEC ; + state->duty_cycle =DIV_ROUND_UP_ULL (val, mvpwm->clk_rate);
Instead of
if (u > 0) val = (unsigned long long) u * NSEC_PER_SEC; else val = ((unsigned long long) UINT_MAX+1) * NSEC_PER_SEC; state->duty_cycle = DIV_ROUND_UP_ULL(val, mvpwm->clk_rate);
you could also write
if (u > 0) val = u; else val = UINT_MAX + 1; state->duty_cycle = DIV_ROUND_UP_ULL(val * NSEC_PER_SEC, mvpwm->clk_rate);
which is a bit lighter (IMHO).
Quoted 20 lines
drivers/gpio/gpio-mvebu.c +5
-7- val = (unsigned long long ) u;/* on duration */ regmap_read (mvpwm->regs,mvebu_pwmreg_blink_off_duration (mvpwm), &u); - val += (unsigned long long ) u;/* period = on + off duration */ - val *=NSEC_PER_SEC ; - val =DIV_ROUND_UP_ULL (val, mvpwm->clk_rate); -if (val) - state->period = val; +/* period = on + off duration */ +if (u >0 ) + val += (unsigned long long ) u *NSEC_PER_SEC ;else - state->period =1 ; + val += ((unsigned long long )UINT_MAX +1 ) *NSEC_PER_SEC ; + state->period =DIV_ROUND_UP_ULL (val, mvpwm->clk_rate);regmap_read (mvchip->regs,GPIO_BLINK_EN_OFF + mvchip->offset, &u);if (u)
drivers/gpio/gpio-mvebu.c static int mvebu_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, · line 705
Quoted 5 lines
drivers/gpio/gpio-mvebu.c +1
-1@@ -704,9 +702,15 @@ static int mvebu_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, val = (unsigned long long ) mvpwm->clk_rate * state->duty_cycle;do_div (val,NSEC_PER_SEC ); -if (val >UINT_MAX ) +if (val > (unsigned long long )UINT_MAX +1 )
Please add whitespace around the +
Best regards
Uwe
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |