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(-)
--
2.29.2
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
The period is the sum of on and off values.
Reported-by: Russell King <linux@armlinux.org.uk>
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(-)
@@ -676,20 +676,17 @@ static void mvebu_pwm_get_state(struct pwm_chip *chip,elsestate->duty_cycle=1;+val=(unsignedlonglong)u;/* on duration */regmap_read(mvpwm->regs,mvebu_pwmreg_blink_off_duration(mvpwm),&u);-val=(unsignedlonglong)u*NSEC_PER_SEC;+val+=(unsignedlonglong)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;+elseif(val)+state->period=val;+elsestate->period=1;-}else{-val-=state->duty_cycle;-if(val>UINT_MAX)-state->period=UINT_MAX;-elseif(val)-state->period=val;-else-state->period=1;-}regmap_read(mvchip->regs,GPIO_BLINK_EN_OFF+mvchip->offset,&u);if(u)
--
2.29.2
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Round up the result of division in period/duty_cycle calculation to make
the result closer to idempotent.
Reported-by: Uwe Kleine-König <redacted>
Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
drivers/gpio/gpio-mvebu.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
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 <redacted>
Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
drivers/gpio/gpio-mvebu.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
@@ -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
--
2.29.2
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hello Baruch,
$Subject ~= s/get_state/.get_state/ ?
On Mon, Jan 11, 2021 at 01:17:02PM +0200, Baruch Siach wrote:
quoted hunk
The period is the sum of on and off values.
Reported-by: Russell King <linux@armlinux.org.uk>
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(-)
@@ -676,20 +676,17 @@ static void mvebu_pwm_get_state(struct pwm_chip *chip,elsestate->duty_cycle=1;+val=(unsignedlonglong)u;/* on duration */regmap_read(mvpwm->regs,mvebu_pwmreg_blink_off_duration(mvpwm),&u);-val=(unsignedlonglong)u*NSEC_PER_SEC;+val+=(unsignedlonglong)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;+elseif(val)+state->period=val;+elsestate->period=1;-}else{-val-=state->duty_cycle;-if(val>UINT_MAX)-state->period=UINT_MAX;-elseif(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
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
On Mon, Jan 11, 2021 at 01:17:04PM +0200, Baruch Siach wrote:
quoted hunk
Round up the result of division in period/duty_cycle calculation to make
the result closer to idempotent.
Reported-by: Uwe Kleine-König <redacted>
Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
drivers/gpio/gpio-mvebu.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
On Mon, Jan 11, 2021 at 01:17:05PM +0200, Baruch Siach wrote:
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 <redacted>
Signed-off-by: Baruch Siach <baruch@tkos.co.il>
@@ -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
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
@@ -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!
--
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
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi Uwe,
On Mon, Jan 11 2021, Uwe Kleine-König wrote:
$Subject ~= s/get_state/.get_state/ ?
Ack.
On Mon, Jan 11, 2021 at 01:17:02PM +0200, Baruch Siach wrote:
quoted
The period is the sum of on and off values.
Reported-by: Russell King <linux@armlinux.org.uk>
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(-)
@@ -676,20 +676,17 @@ static void mvebu_pwm_get_state(struct pwm_chip *chip,elsestate->duty_cycle=1;+val=(unsignedlonglong)u;/* on duration */regmap_read(mvpwm->regs,mvebu_pwmreg_blink_off_duration(mvpwm),&u);-val=(unsignedlonglong)u*NSEC_PER_SEC;+val+=(unsignedlonglong)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;+elseif(val)+state->period=val;+elsestate->period=1;-}else{-val-=state->duty_cycle;-if(val>UINT_MAX)-state->period=UINT_MAX;-elseif(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
.
I take this to refer to the next patch (2/5). This patch changes from
buggy
$on / clkrate
to
($on + $off) / clkrate
baruch
--
~. .~ Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
- baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Wed, Jan 13, 2021 at 08:36:12AM +0200, Baruch Siach wrote:
Hi Uwe,
On Mon, Jan 11 2021, Uwe Kleine-König wrote:
quoted
$Subject ~= s/get_state/.get_state/ ?
Ack.
quoted
On Mon, Jan 11, 2021 at 01:17:02PM +0200, Baruch Siach wrote:
quoted
The period is the sum of on and off values.
Reported-by: Russell King <linux@armlinux.org.uk>
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(-)
@@ -676,20 +676,17 @@ static void mvebu_pwm_get_state(struct pwm_chip *chip,elsestate->duty_cycle=1;+val=(unsignedlonglong)u;/* on duration */regmap_read(mvpwm->regs,mvebu_pwmreg_blink_off_duration(mvpwm),&u);-val=(unsignedlonglong)u*NSEC_PER_SEC;+val+=(unsignedlonglong)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;+elseif(val)+state->period=val;+elsestate->period=1;-}else{-val-=state->duty_cycle;-if(val>UINT_MAX)-state->period=UINT_MAX;-elseif(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
.
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
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |