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

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

On 20-09-28 21:06, Uwe Kleine-König wrote:
> On Mon, Sep 28, 2020 at 11:52:30AM +0200, Marco Felsch wrote:
> > 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.
> 
> I think this is fine because for lack of definitive documentation
> looking at the expectations of consumers is the only source we have to
> somewhat justify what the lowlevel driver is expected to do. And if I
> understood you correctly the pwm-bl driver is the one that the problems
> surfaced with, isn't it?

Yep the pwm-bl driver is the one causing the issue.

> > > > 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.
> 
> So we're on one line here.

But you're suggestion is to reflect those details to the user-space or
do I miss something in your snippet?

> > > >  	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?
> 
> sysfs caches the value anyhow and so do most consumers.

Hm.. this seems a bit weird and and unexpected from the user
perspective. Is there any chance to document the behaviour of apply?

> Also duty_cycle
> is a local variable here only. (That's at least what I assumed writing
> the above snippet.)

Which gets applied later on to the imx->duty_cycle.

> > 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.
> 
> The first call configures the PWM with .duty_cycle = 3s and the second
> call then waits until a period is completed (doesn't it?)

Why does it wait till the period is completetd? It writes the 3s
duty_cycle into the PWMSAR register and goes on.

> So completing
> the 2nd command takes up to 3 seconds and 1.5 seconds on average. Before
> your patch the second command was done instantanious.

This patch changes nothing on this behaviour.

> > > @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.
> 
> Ah ok, there are now two different "off"s.

The whole patch is about dropping the PWMCR.EN from the apply hook, so
yes there are two kinds of "off".

> Anyhow, I oppose to modify
> the hardware state in .remove(). There are (I think) corner cases like
> the backlight should remain on during reboot to show logs (or a splash
> screen).

No one should expect that a pwm-bl should work if the pwm driver is
going to be removed. Also this assumes that the pwm-bl driver don't
turn off the regulator. On my laptop the backlight is turned off/on
during reboot.

Thanks for the review :)
  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 |