| Message ID | 20200925155330.32301-6-m.felsch@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | PWM i.MX27 fix disabled state for inverted signals | expand |
On Fri, Sep 25, 2020 at 05:53:30PM +0200, Marco Felsch wrote: > Currently the driver don't check if the new state was applied or not. s/don't/doesn't/ > This can cause glitches on the output pin if the new state disables the > PWM. In this case the PWM clocks are disabled before the new duty cycle > value gets applied. Hmm, the problem that is addressed here is that .apply() might turn off the clock input for the counter before the inactive value is on the pin, right? So an alternative fix would be to not disable the clock, wouldn't it? > The fix is to wait till the desired duty cycle was applied. > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > --- > v2: > - new patch > > drivers/pwm/pwm-imx27.c | 25 +++++++++++++++++++++++++ > 1 file changed, 25 insertions(+) > > diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c > index 07c6a263a39c..ffa00bcd81da 100644 > --- a/drivers/pwm/pwm-imx27.c > +++ b/drivers/pwm/pwm-imx27.c > @@ -222,6 +222,26 @@ static int pwm_imx27_get_fifo_slot(struct pwm_chip *chip, > return fifoav; > } > > +static int pwm_imx27_wait_till_applied(struct pwm_chip *chip, > + struct pwm_device *pwm) > +{ > + unsigned int attempts = 4; > + unsigned int period_ms; > + int busy_slots; > + > + do { > + busy_slots = pwm_imx27_get_fifo_slot(chip, pwm); > + if (busy_slots == 0) > + return 0; > + > + period_ms = DIV_ROUND_UP(pwm_get_period(pwm), I was glad you removed the call to pwm_get_state() from .apply(), now it is back in disguised form here :-\ Also the value shouldn't change over the iteration of this loop, so determining it once should be enough. > + NSEC_PER_MSEC); > + msleep(period_ms); > + } while (attempts--); > + > + return -ETIMEDOUT; > +} > + > static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, > const struct pwm_state *state) > { > @@ -277,6 +297,11 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, > writel(duty_cycles, imx->mmio_base + MX3_PWMSAR); > else > writel(0, imx->mmio_base + MX3_PWMSAR); > + > + ret = pwm_imx27_wait_till_applied(chip, pwm); > + if (ret) > + goto out; > + The framework doesn't define (and this is a problem there) if .apply is supposed to sleep. OTOH at least sun4i has a similar behaviour. Thierry, what is your thought on this? Best regards Uwe
On 20-09-28 10:04, Uwe Kleine-König wrote: > On Fri, Sep 25, 2020 at 05:53:30PM +0200, Marco Felsch wrote: > > Currently the driver don't check if the new state was applied or not. > > s/don't/doesn't/ > > > This can cause glitches on the output pin if the new state disables the > > PWM. In this case the PWM clocks are disabled before the new duty cycle > > value gets applied. > > Hmm, the problem that is addressed here is that .apply() might turn off > the clock input for the counter before the inactive value is on the pin, > right? So an alternative fix would be to not disable the clock, wouldn't > it? Yes, till the new state is applied. > > The fix is to wait till the desired duty cycle was applied. > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > --- > > v2: > > - new patch > > > > drivers/pwm/pwm-imx27.c | 25 +++++++++++++++++++++++++ > > 1 file changed, 25 insertions(+) > > > > diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c > > index 07c6a263a39c..ffa00bcd81da 100644 > > --- a/drivers/pwm/pwm-imx27.c > > +++ b/drivers/pwm/pwm-imx27.c > > @@ -222,6 +222,26 @@ static int pwm_imx27_get_fifo_slot(struct pwm_chip *chip, > > return fifoav; > > } > > > > +static int pwm_imx27_wait_till_applied(struct pwm_chip *chip, > > + struct pwm_device *pwm) > > +{ > > + unsigned int attempts = 4; > > + unsigned int period_ms; > > + int busy_slots; > > + > > + do { > > + busy_slots = pwm_imx27_get_fifo_slot(chip, pwm); > > + if (busy_slots == 0) > > + return 0; > > + > > + period_ms = DIV_ROUND_UP(pwm_get_period(pwm), > > I was glad you removed the call to pwm_get_state() from .apply(), now it is > back in disguised form here :-\ I reused the code from pwm_imx27_get_fifo_slot(). > Also the value shouldn't change over the > iteration of this loop, so determining it once should be enough. Yes, you are right. I will change that. > > + NSEC_PER_MSEC); > > + msleep(period_ms); > > + } while (attempts--); > > + > > + return -ETIMEDOUT; > > +} > > + > > static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > const struct pwm_state *state) > > { > > @@ -277,6 +297,11 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > writel(duty_cycles, imx->mmio_base + MX3_PWMSAR); > > else > > writel(0, imx->mmio_base + MX3_PWMSAR); > > + > > + ret = pwm_imx27_wait_till_applied(chip, pwm); > > + if (ret) > > + goto out; > > + > > The framework doesn't define (and this is a problem there) if .apply is > supposed to sleep. Current upstream driver sleeps as well if pwm_imx27_wait_fifo_slot() waits. So this patch don't changes the bevhaviour. Regards, Marco > OTOH at least sun4i has a similar behaviour. > Thierry, what is your thought on this? > > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | https://www.pengutronix.de/ |
diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c index 07c6a263a39c..ffa00bcd81da 100644 --- a/drivers/pwm/pwm-imx27.c +++ b/drivers/pwm/pwm-imx27.c @@ -222,6 +222,26 @@ static int pwm_imx27_get_fifo_slot(struct pwm_chip *chip, return fifoav; } +static int pwm_imx27_wait_till_applied(struct pwm_chip *chip, + struct pwm_device *pwm) +{ + unsigned int attempts = 4; + unsigned int period_ms; + int busy_slots; + + do { + busy_slots = pwm_imx27_get_fifo_slot(chip, pwm); + if (busy_slots == 0) + return 0; + + period_ms = DIV_ROUND_UP(pwm_get_period(pwm), + NSEC_PER_MSEC); + msleep(period_ms); + } while (attempts--); + + return -ETIMEDOUT; +} + static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, const struct pwm_state *state) { @@ -277,6 +297,11 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, writel(duty_cycles, imx->mmio_base + MX3_PWMSAR); else writel(0, imx->mmio_base + MX3_PWMSAR); + + ret = pwm_imx27_wait_till_applied(chip, pwm); + if (ret) + goto out; + writel(period_cycles, imx->mmio_base + MX3_PWMPR); /*
Currently the driver don't check if the new state was applied or not. This can cause glitches on the output pin if the new state disables the PWM. In this case the PWM clocks are disabled before the new duty cycle value gets applied. The fix is to wait till the desired duty cycle was applied. Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> --- v2: - new patch drivers/pwm/pwm-imx27.c | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+)