| Message ID | 22d1fe7b2137e3a2660ab2e6f1f127d41493fb16.1609917364.git.baruch@tkos.co.il |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: mvebu: Armada 8K/7K PWM support | expand |
On Wed, Jan 06, 2021 at 09:37:37AM +0200, Baruch Siach wrote: > 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> > --- > v6: divide (on + off) sum to reduce rounding error (RMK) > --- > 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; state->period is an u64, so there is no reason to not use values greater than UINT_MAX. > + else if (val) > + state->period = val; > + else > state->period = 1; This case assigning 1 looks strange. An explanation in a comment would be great. I wonder if this is a hardware property or if it is only used to not report 0 in case that mvpwm->clk_rate is high. I found a few further shortcommings in the mvebu_pwm implementation while looking through it: a) The rounding problem that RMK found is also present in .apply There we have: val = clk_rate * (period - duty_cycle) / NSEC_PER_SEC while val = clk_rate * period / NSEC_PER_SEC - on would be more exact. b) To make pwm_get_state(pwm, &state); pwm_apply_state(pwm, &state); idempotent .get_state should round up the division results. c) .apply also has a check for val being zero and configures at least 1 cycle for the on and off intervals. Is this a hardware imposed limitation? Best regards Uwe
Hi Uwe, Thanks for your review comments. On Thu, Jan 07 2021, Uwe Kleine-König wrote: > On Wed, Jan 06, 2021 at 09:37:37AM +0200, Baruch Siach wrote: >> 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> >> --- >> v6: divide (on + off) sum to reduce rounding error (RMK) >> --- >> 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; > > state->period is an u64, so there is no reason to not use values greater > than UINT_MAX. I'll post a patch for that. >> + else if (val) >> + state->period = val; >> + else >> state->period = 1; > > This case assigning 1 looks strange. An explanation in a comment would > be great. I wonder if this is a hardware property or if it is only used > to not report 0 in case that mvpwm->clk_rate is high. I guess that this is because 0 period does not make sense for pwm. It is like a zero divisor. What is the common behavior? > I found a few further shortcommings in the mvebu_pwm implementation while > looking through it: > > a) The rounding problem that RMK found is also present in .apply > > There we have: > > val = clk_rate * (period - duty_cycle) / NSEC_PER_SEC > > while > > val = clk_rate * period / NSEC_PER_SEC - on > > would be more exact. I'll post a patch for that. > b) To make > > pwm_get_state(pwm, &state); > pwm_apply_state(pwm, &state); > > idempotent .get_state should round up the division results. I'll post a patch for that as well. > c) .apply also has a check for val being zero and configures at least 1 > cycle for the on and off intervals. Is this a hardware imposed > limitation? Not sure what was the original intention. Maybe Andrew can explain. On my hardware (Armada 8040), zero 'on' value does not work as expected. There is a blink once in a long while. Maybe this is the reason? As I understand, all these issues should not block this patch, right? BTW, the key you used to sign your message is expired since 2020-01-10 on the key server I use (keys.gnupg.net). Where can I find your updated key? Thanks, baruch
On Sun, Jan 10, 2021 at 07:14:17PM +0200, Baruch Siach wrote: > Hi Uwe, > > Thanks for your review comments. > > On Thu, Jan 07 2021, Uwe Kleine-König wrote: > > On Wed, Jan 06, 2021 at 09:37:37AM +0200, Baruch Siach wrote: > >> 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> > >> --- > >> v6: divide (on + off) sum to reduce rounding error (RMK) > >> --- > >> 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; > > > > state->period is an u64, so there is no reason to not use values greater > > than UINT_MAX. > > I'll post a patch for that. > > >> + else if (val) > >> + state->period = val; > >> + else > >> state->period = 1; > > > > This case assigning 1 looks strange. An explanation in a comment would > > be great. I wonder if this is a hardware property or if it is only used > > to not report 0 in case that mvpwm->clk_rate is high. > > I guess that this is because 0 period does not make sense for pwm. It is > like a zero divisor. What is the common behavior? It depends on how the hardware behaves in this case. One measure is to use round-up (in .get_state) for the divisions -- as I already pointed out. Then the problem only triggers if both on and off registers are zero. > > I found a few further shortcommings in the mvebu_pwm implementation while > > looking through it: > > > > a) The rounding problem that RMK found is also present in .apply > > > > There we have: > > > > val = clk_rate * (period - duty_cycle) / NSEC_PER_SEC > > > > while > > > > val = clk_rate * period / NSEC_PER_SEC - on > > > > would be more exact. > > I'll post a patch for that. > > > b) To make > > > > pwm_get_state(pwm, &state); > > pwm_apply_state(pwm, &state); > > > > idempotent .get_state should round up the division results. > > I'll post a patch for that as well. > > > c) .apply also has a check for val being zero and configures at least 1 > > cycle for the on and off intervals. Is this a hardware imposed > > limitation? > > Not sure what was the original intention. Maybe Andrew can explain. > > On my hardware (Armada 8040), zero 'on' value does not work as > expected. There is a blink once in a long while. Maybe this is the > reason? It would be good to understand and document this. Hardware PWMs not supporting a zero duty cycle are always a surprise for me and probably also others. > As I understand, all these issues should not block this patch, right? Yes, having this patch in a series fixing all the stuff I pointed out would still be very welcome :-) > BTW, the key you used to sign your message is expired since 2020-01-10 > on the key server I use (keys.gnupg.net). Where can I find your updated > key? Hmm, I thought keyservers are out of fashion. Anyhow, I updated my key there. There is no WKD for @pengutronix.de addresses, but gpg --locate-external-keys uwe@kleine-koenig.org should work just fine, too. Best regards Uwe
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)
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> --- v6: divide (on + off) sum to reduce rounding error (RMK) --- drivers/gpio/gpio-mvebu.c | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-)