| Message ID | cover.1609917364.git.baruch@tkos.co.il |
|---|---|
| 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
Changes in v6: * Reduce rounding error in the get_state fix (RMK) Changes in v5: * Add a fix for get_state * Fix typo in patch #4 subject line * Add Rob's review tag on the binding documentation patch Changes in v4: * Remove patches that are in LinusW linux-gpio for-next and fixes * Rename the 'pwm-offset' property to 'marvell,pwm-offset' as suggested by Rob Herring The original cover letter follows (with DT property name updated). The gpio-mvebu driver supports the PWM functionality of the GPIO block for earlier Armada variants like XP, 370 and 38x. This series extends support to newer Armada variants that use CP11x and AP80x, like Armada 8K and 7K. This series adds adds the 'marvell,pwm-offset' property to DT binding. 'marvell,pwm-offset' points to the base of A/B counter registers that determine the PWM period and duty cycle. The existing PWM DT binding reflects an arbitrary decision to allocate the A counter to the first GPIO block, and B counter to the other one. In attempt to provide better future flexibility, the new 'marvell,pwm-offset' property always points to the base address of both A/B counters. The driver code still allocates the counters in the same way, but this might change in the future with no change to the DT. Tested AP806 and CP110 (both) on Armada 8040 based system. Baruch Siach (4): gpio: mvebu: fix pwm get_state period calculation gpio: mvebu: add pwm support for Armada 8K/7K arm64: dts: armada: add pwm offsets for ap/cp gpios dt-bindings: ap806: document gpio marvell,pwm-offset property .../arm/marvell/ap80x-system-controller.txt | 8 ++ arch/arm64/boot/dts/marvell/armada-ap80x.dtsi | 3 + arch/arm64/boot/dts/marvell/armada-cp11x.dtsi | 10 ++ drivers/gpio/gpio-mvebu.c | 120 +++++++++++------- 4 files changed, 97 insertions(+), 44 deletions(-)