Re: [PATCH v2 4/5] pwm: imx27: fix disable state for inverted PWMs

[Marco Felsch <m.felsch@pengutronix.de>]

On 20-09-28 09:47, Uwe Kleine-König wrote:
> Hello,
> 
> On Fri, Sep 25, 2020 at 05:53:29PM +0200, Marco Felsch wrote:
> > Up to now disabling the PWM is done using the PWMCR.EN register bit.
> > Setting this bit to zero results in the output pin driving a low value
> > independent of the polarity setting (PWMCR.POUTC).
> > 
> > There is only little documentation about expectations and requirements
> > in the PWM framework but the usual expectation seems to be that
> > disabling a PWM together with setting .duty_cycle = 0 results in the
> > output driving the inactive level. The pwm-bl driver for example uses
> > this setting to disable the backlight and with the pwm-imx27 driver
> > this results in an enabled backlight if the pwm signal is inverted.
> 
> This sounds as if the pwm-imx27 behaviour is a reason to believe that
> .duty_cycle = 0 + .enabled = false should drive the inactive level.

That was what you suggested in v1.

> I'd write:
> 	The pwm-bl driver for example uses this setting to disable the
> 	backlight. Up to now however, this request makes the pwm-imx27
> 	enable the backlight if the PWM signal is inverted.

I don't wanna but a specific user (pwm-bl driver) into the commit
message since this assumes that this fix is only needed because
of the pwm-bl driver.

> > Keep the PWMCR.EN bit always enabled and simulate a disabled PWM using
> > duty_cycle = 0 to fix this. Furthermore we have to drop the sw-reset
> > from apply() otherwise the PWMCR.EN is cleared too. Therefore the
> > pwm_imx27_wait_fifo_slot() is extended and renamed to guarantee a free
> > FIFO slot and to reflect the new meaning.
> > 
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> > v2:
> > - fix driver remove function
> > - rename pwm_imx27_wait_fifo_slot
> > - pwm_imx27_get_fifo_slot now returns the number of used fifo slots
> >   rather than 0 on success (needed for next patch).
> > 
> >  drivers/pwm/pwm-imx27.c | 78 ++++++++++++++++++++++++++---------------
> >  1 file changed, 49 insertions(+), 29 deletions(-)
> > 
> > diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
> > index 3b6bcd8d58b7..07c6a263a39c 100644
> > --- a/drivers/pwm/pwm-imx27.c
> > +++ b/drivers/pwm/pwm-imx27.c
> > @@ -141,12 +141,9 @@ static void pwm_imx27_get_state(struct pwm_chip *chip,
> >  	if (ret < 0)
> >  		return;
> >  
> > -	val = readl(imx->mmio_base + MX3_PWMCR);
> > +	state->enabled = imx->enabled;
> >  
> > -	if (val & MX3_PWMCR_EN)
> > -		state->enabled = true;
> > -	else
> > -		state->enabled = false;
> > +	val = readl(imx->mmio_base + MX3_PWMCR);
> 
> I'm not a big fan. IMHO the driver should report about reality and the
> framework (and maybe the consumers) should be able to handle that
> .get_state() reports
> 
> 	.enabled = true
> 	.duty_cycle = 0
> 
> after
> 
> 	.enabled = false
> 
> was requested.

So your suggestions will spam the pwm user with the ugly details?
IMHO the framework should abstract this since it is a nasty HW detail
the pwm user should not take care off.

> >  	switch (FIELD_GET(MX3_PWMCR_POUTC, val)) {
> >  	case MX3_PWMCR_POUTC_NORMAL:
> > @@ -169,8 +166,8 @@ static void pwm_imx27_get_state(struct pwm_chip *chip,
> >  	state->period = DIV_ROUND_UP_ULL(tmp, pwm_clk);
> >  
> >  	/*
> > -	 * PWMSAR can be read only if PWM is enabled. If the PWM is disabled,
> > -	 * use the cached value.
> > +	 * Use the cached value if the PWM is disabled since we are using the
> > +	 * PWMSAR to disable the PWM (see the notes in pwm_imx27_apply())
> >  	 */
> >  	if (state->enabled)
> >  		val = readl(imx->mmio_base + MX3_PWMSAR);
> > @@ -199,8 +196,8 @@ static void pwm_imx27_sw_reset(struct pwm_imx27_chip *imx, struct device *dev)
> >  		dev_warn(dev, "software reset timeout\n");
> >  }
> >  
> > -static void pwm_imx27_wait_fifo_slot(struct pwm_chip *chip,
> > -				     struct pwm_device *pwm)
> > +static int pwm_imx27_get_fifo_slot(struct pwm_chip *chip,
> > +				   struct pwm_device *pwm)
> >  {
> >  	struct pwm_imx27_chip *imx = to_pwm_imx27_chip(chip);
> >  	struct device *dev = chip->dev;
> > @@ -216,9 +213,13 @@ static void pwm_imx27_wait_fifo_slot(struct pwm_chip *chip,
> >  		msleep(period_ms);
> >  
> >  		sr = readl(imx->mmio_base + MX3_PWMSR);
> > -		if (fifoav == FIELD_GET(MX3_PWMSR_FIFOAV, sr))
> > +		if (fifoav == FIELD_GET(MX3_PWMSR_FIFOAV, sr)) {
> >  			dev_warn(dev, "there is no free FIFO slot\n");
> > +			return -EBUSY;
> > +		}
> >  	}
> > +
> > +	return fifoav;
> >  }
> >  
> >  static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > @@ -257,16 +258,25 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> >  	else
> >  		period_cycles = 0;
> >  
> > +	/* Wait for a free FIFO slot */
> > +	ret = pwm_imx27_get_fifo_slot(chip, pwm);
> > +	if (ret < 0)
> > +		goto out;
> > +
> >  	/*
> > -	 * Wait for a free FIFO slot if the PWM is already enabled, and flush
> > -	 * the FIFO if the PWM was disabled and is about to be enabled.
> > +	 * We can't use the enable bit to control the en-/disable squence
> > +	 * correctly because the output pin is pulled low if setting this bit
> > +	 * to '0' regardless of the poutc value. Instead we have to use the
> > +	 * sample register. According the RM:
> 
> According to the reference manual:

K.

> > +	 * A value of zero in the sample register will result in the PWMO output
> > +	 * signal always being low/high (POUTC = 00 it will be low and
> > +	 * POUTC = 01 it will be high), and no output waveform will be produced.
> > +	 * If the value in this register is higher than the PERIOD
> 
> Did you forget to insert the end of this sentence here?

Ups, thanks for covering that.

> 
> >  	 */
> > -	if (imx->enabled)
> > -		pwm_imx27_wait_fifo_slot(chip, pwm);
> > +	if (state->enabled)
> > +		writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
> >  	else
> > -		pwm_imx27_sw_reset(imx, chip->dev);
> > -
> > -	writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
> > +		writel(0, imx->mmio_base + MX3_PWMSAR);
> >  	writel(period_cycles, imx->mmio_base + MX3_PWMPR);
> 
> I think you can simplify the code a bit using the following idiom:
> 
> 	/* 
> 	 * comment as above
> 	 */
> 	
> 	if (!state->enabled)
> 		duty_cycle = 0;
> 
> 	writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);

I don't think so because this will throw aways the duty_cycle. What
should happen if the user disable the pwm by: state->enable = false and
enable it later again e.g. if you configure the pwm from the sysfs?
My assumption is that the previouse set duty-cycle should be applied
which isn't possible with your solution.

> With the change from the next patch I could also imagine to write a
> smaller period in the !enabled case. The upside would be that the second
> call in:
> 
> 	pwm_apply(mypwm, { .enabled = false, .period = 3s });
> 	pwm_apply(mypwm, { .enabled = true, ... });
> 
> wouldn't take longer than a second in the average case.

Sorry I don't get this.

> @Thierry, we really need to agree on the expected behaviour in these
> cases and document them.

+1

> >  	/*
> > @@ -276,15 +286,10 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> >  	imx->duty_cycle = duty_cycles;
> >  
> >  	cr = MX3_PWMCR_PRESCALER_SET(prescale);
> > -
> >  	if (state->polarity == PWM_POLARITY_INVERSED)
> > -		cr |= FIELD_PREP(MX3_PWMCR_POUTC,
> > -				MX3_PWMCR_POUTC_INVERTED);
> > -
> > -	if (state->enabled)
> > -		cr |= MX3_PWMCR_EN;
> > +		cr |= FIELD_PREP(MX3_PWMCR_POUTC, MX3_PWMCR_POUTC_INVERTED);
> >  
> > -	mask = MX3_PWMCR_PRESCALER | MX3_PWMCR_POUTC | MX3_PWMCR_EN;
> > +	mask = MX3_PWMCR_PRESCALER | MX3_PWMCR_POUTC;
> >  
> >  	pwm_imx27_update_bits(imx->mmio_base + MX3_PWMCR, mask, cr);
> >  
> > @@ -373,10 +378,13 @@ static int pwm_imx27_probe(struct platform_device *pdev)
> >  	if (!(pwmcr & MX3_PWMCR_EN)) {
> >  		pwm_imx27_sw_reset(imx, &pdev->dev);
> >  		mask = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN |
> > -		       MX3_PWMCR_DBGEN | MX3_PWMCR_CLKSRC;
> > +		       MX3_PWMCR_DBGEN | MX3_PWMCR_CLKSRC | MX3_PWMCR_POUTC |
> > +		       MX3_PWMCR_EN;
> >  		pwmcr = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN |
> >  			MX3_PWMCR_DBGEN |
> > -			FIELD_PREP(MX3_PWMCR_CLKSRC, MX3_PWMCR_CLKSRC_IPG_HIGH);
> > +			FIELD_PREP(MX3_PWMCR_CLKSRC, MX3_PWMCR_CLKSRC_IPG_HIGH) |
> > +			FIELD_PREP(MX3_PWMCR_POUTC, MX3_PWMCR_POUTC_OFF) |
> > +			MX3_PWMCR_EN;
> >  		pwm_imx27_update_bits(imx->mmio_base + MX3_PWMCR, mask, pwmcr);
> >  		pwm_imx27_clk_disable_unprepare(imx);
> >  	} else {
> > @@ -385,6 +393,7 @@ static int pwm_imx27_probe(struct platform_device *pdev)
> >  		pwmcr = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN |
> >  			MX3_PWMCR_DBGEN;
> >  		pwm_imx27_update_bits(imx->mmio_base + MX3_PWMCR, mask, pwmcr);
> > +		imx->enabled = true;
> >  	}
> >  
> >  	return pwmchip_add(&imx->chip);
> > @@ -392,11 +401,22 @@ static int pwm_imx27_probe(struct platform_device *pdev)
> >  
> >  static int pwm_imx27_remove(struct platform_device *pdev)
> >  {
> > -	struct pwm_imx27_chip *imx;
> > +	struct pwm_imx27_chip *imx = platform_get_drvdata(pdev);
> > +	int ret;
> >  
> > -	imx = platform_get_drvdata(pdev);
> > +	ret = pwm_imx27_clk_prepare_enable(imx);
> > +	if (ret)
> > +		return ret;
> >  
> > -	return pwmchip_remove(&imx->chip);
> > +	ret = pwmchip_remove(&imx->chip);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Ensure module is disabled after remove */
> > +	pwm_imx27_update_bits(imx->mmio_base + MX3_PWMCR, MX3_PWMCR_EN, 0);
> > +	pwm_imx27_clk_disable_unprepare(imx);
> 
> This is wrong. You are supposed to assume the PWM is already off in
> .remove and don't touch it.

Nope it isn't. The hardware is still running after the remove call since
we don't enable/disable the HW anymore by toggling the PWMCR.EN bit. So
we need to do it here.

Regards,
  Marco

> 
> Best regards
> Uwe
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |



-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |