| Message ID | 7c18dd67d3bf3e3ed9a8efa2edd33e8f29f09a7a.1610628807.git.baruch@tkos.co.il (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | gpio: mvebu: pwm fixes and improvements | expand |
Hello Baruch, On Thu, Jan 14, 2021 at 08:57:37PM +0200, Baruch Siach wrote: > Add a comment on why the code never sets on/off registers to zero. > > Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > Analyzed-by: Russell King <linux@armlinux.org.uk> > Signed-off-by: Baruch Siach <baruch@tkos.co.il> > --- > drivers/gpio/gpio-mvebu.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c > index 6b017854ce61..09780944bef9 100644 > --- a/drivers/gpio/gpio-mvebu.c > +++ b/drivers/gpio/gpio-mvebu.c > @@ -706,6 +706,10 @@ static int mvebu_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > do_div(val, NSEC_PER_SEC); > if (val > UINT_MAX) > return -EINVAL; > + /* > + * Zero on/off values don't work as expected. Experimentation shows > + * that zero value is treated as 2^32. This behavior is not documented. > + */ This is too easy. The right thing to do is to adapt .apply and .get_state to use this new information. Best regards Uwe
On Thu, Jan 14, 2021 at 09:25:45PM +0100, Uwe Kleine-König wrote: > Hello Baruch, > > On Thu, Jan 14, 2021 at 08:57:37PM +0200, Baruch Siach wrote: > > Add a comment on why the code never sets on/off registers to zero. > > > > Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > Analyzed-by: Russell King <linux@armlinux.org.uk> > > Signed-off-by: Baruch Siach <baruch@tkos.co.il> > > --- > > drivers/gpio/gpio-mvebu.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c > > index 6b017854ce61..09780944bef9 100644 > > --- a/drivers/gpio/gpio-mvebu.c > > +++ b/drivers/gpio/gpio-mvebu.c > > @@ -706,6 +706,10 @@ static int mvebu_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > do_div(val, NSEC_PER_SEC); > > if (val > UINT_MAX) > > return -EINVAL; > > + /* > > + * Zero on/off values don't work as expected. Experimentation shows > > + * that zero value is treated as 2^32. This behavior is not documented. > > + */ > > This is too easy. The right thing to do is to adapt .apply and > .get_state to use this new information. What exactly do you expect the changes to be? Bear in mind that the hardware is not capable of atomically updating e.g. the duty cycle without affecting the period, because any change in duty cycle needs the "on" and "off" durations to be separately programmed, and there's a chance that the hardware could start using either value mid-update. Also, disabling "blink" mode to achieve a steady output (for 0% or 100% duty cycle) would require further investigation to find out how the hardware behaves at the moment where blink mode is disabled: does the output retain its current state (does the bit in the output register toggle with the blink) or does it revert to the value in the output register that was programmed before blink mode was enabled. Again, none of that is documented, so would need experimentation with the hardware to work out how to achieve it. And then if you want even more complexity, I suppose we could try and read the current state of the pin, add a delay, recheck it and try and work out the optimal place to disable the blink mode. Exactly how far do you want to go with this? All of this is likely getting rediculously complicated for the use cases of it today that don't need it. Yes, it's annoying that we can't achieve 0% or 100% duty cycle with this hardware that was never designed as a PWM without jumping through a lot of hoops but currently settle for a minimum pulse width of 4ns at each end of the range.
On Thu, Jan 14, 2021 at 10:28:02PM +0000, Russell King - ARM Linux admin wrote: > On Thu, Jan 14, 2021 at 09:25:45PM +0100, Uwe Kleine-König wrote: > > Hello Baruch, > > > > On Thu, Jan 14, 2021 at 08:57:37PM +0200, Baruch Siach wrote: > > > Add a comment on why the code never sets on/off registers to zero. > > > > > > Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > > Analyzed-by: Russell King <linux@armlinux.org.uk> > > > Signed-off-by: Baruch Siach <baruch@tkos.co.il> > > > --- > > > drivers/gpio/gpio-mvebu.c | 4 ++++ > > > 1 file changed, 4 insertions(+) > > > > > > diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c > > > index 6b017854ce61..09780944bef9 100644 > > > --- a/drivers/gpio/gpio-mvebu.c > > > +++ b/drivers/gpio/gpio-mvebu.c > > > @@ -706,6 +706,10 @@ static int mvebu_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > > do_div(val, NSEC_PER_SEC); > > > if (val > UINT_MAX) > > > return -EINVAL; > > > + /* > > > + * Zero on/off values don't work as expected. Experimentation shows > > > + * that zero value is treated as 2^32. This behavior is not documented. > > > + */ > > > > This is too easy. The right thing to do is to adapt .apply and > > .get_state to use this new information. > > What exactly do you expect the changes to be? What I expect is: - let .apply() write 0 if the intention is to configure 2^32 clock steps for the on or off register; and symmetrically - let .get_state report 2^32 * NSEC_PER_SEC / clk_rate if the register value is 0. > Bear in mind that the hardware is not capable of atomically updating > e.g. the duty cycle without affecting the period, because any change > in duty cycle needs the "on" and "off" durations to be separately > programmed, and there's a chance that the hardware could start using > either value mid-update. > > Also, disabling "blink" mode to achieve a steady output (for 0% or 100% > duty cycle) would require further investigation to find out how the > hardware behaves at the moment where blink mode is disabled: does the > output retain its current state (does the bit in the output register > toggle with the blink) or does it revert to the value in the output > register that was programmed before blink mode was enabled. I have some plans here about what is the right behaviour, but this needs some preparatory work that I didn't do yet. I'll come back to this eventually. Best regards Uwe
diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c index 6b017854ce61..09780944bef9 100644 --- a/drivers/gpio/gpio-mvebu.c +++ b/drivers/gpio/gpio-mvebu.c @@ -706,6 +706,10 @@ static int mvebu_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, do_div(val, NSEC_PER_SEC); if (val > UINT_MAX) return -EINVAL; + /* + * 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) on = val; else
Add a comment on why the code never sets on/off registers to zero. Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Analyzed-by: Russell King <linux@armlinux.org.uk> Signed-off-by: Baruch Siach <baruch@tkos.co.il> --- drivers/gpio/gpio-mvebu.c | 4 ++++ 1 file changed, 4 insertions(+)