[PATCH v2 0/5] PWM i.MX27 fix disabled state for inverted signals

[PATCH v2 0/5] PWM i.MX27 fix disabled state for inverted signals

[Marco Felsch]

Hi,

this small series fixes the pwm disabled behaviour in case of an
inverted pwm signal. The current state is that in such a case the pwm
signal goes to 0V which means logical '1' for inverted pwm signals. IMO
this is wrong and should be fixed.

I've tested my patches on a custom imx6 board which uses the pwm as
backlight.

Each patch has a dedicated changelog so I skip it here.

Comments and testers are welcome :)

Regards,
  Marco

Marco Felsch (5):
  pwm: imx27: enable clock unconditional for register access
  pwm: imx27: move constant PWMCR register values into probe
  pwm: imx27: reset the PWM if it is not running
  pwm: imx27: fix disable state for inverted PWMs
  pwm: imx27: wait till the duty cycle is applied

 drivers/pwm/pwm-imx27.c | 166 +++++++++++++++++++++++++++++-----------
 1 file changed, 120 insertions(+), 46 deletions(-)

-- 
2.20.1

[PATCH v2 1/5] pwm: imx27: enable clock unconditional for register access

[Marco Felsch]

The current implementation enables the clock if the current PWM state
is '!enabled' to ensure the register access and left the clock on if the
new state is 'enabled'. Further apply calls don't enable the clock since
they relying on the fact the the clock is already running. Change this
behaviour since it is not very intuitive.

This commit changes this behaviour. Now the clocks are unconditional
enabled/disabled before/after the register access. If the PWM should be
turned on (state.enabled) we enable the clock again and vice versa if
the PWM should be turned off (!state.enabled).

Therefore I added the enable member to the driver state struct since
the usage of cstate and pwm_get_state() is a layer violation. I removed
this violation while on it.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
v2:
- use enable var which can be shared later on
- remove cstate and pwm_get_state() layer violation
- explicite enable/disable the clock twice if the pwm should be
  enabled/disabled rather than tracking the clock usage within the
  pwm_imx27_clk_prepare_enable() state.
- rename commit message

 drivers/pwm/pwm-imx27.c | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
index c50d453552bd..7edac4ac6395 100644
--- a/drivers/pwm/pwm-imx27.c
+++ b/drivers/pwm/pwm-imx27.c
@@ -91,6 +91,7 @@ struct pwm_imx27_chip {
 	 * value to return in that case.
 	 */
 	unsigned int duty_cycle;
+	bool enabled;
 };
 
 #define to_pwm_imx27_chip(chip)	container_of(chip, struct pwm_imx27_chip, chip)
@@ -217,13 +218,14 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 {
 	unsigned long period_cycles, duty_cycles, prescale;
 	struct pwm_imx27_chip *imx = to_pwm_imx27_chip(chip);
-	struct pwm_state cstate;
 	unsigned long long c;
 	unsigned long long clkrate;
 	int ret;
 	u32 cr;
 
-	pwm_get_state(pwm, &cstate);
+	ret = pwm_imx27_clk_prepare_enable(imx);
+	if (ret)
+		return ret;
 
 	clkrate = clk_get_rate(imx->clk_per);
 	c = clkrate * state->period;
@@ -251,15 +253,10 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	 * 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.
 	 */
-	if (cstate.enabled) {
+	if (imx->enabled)
 		pwm_imx27_wait_fifo_slot(chip, pwm);
-	} else {
-		ret = pwm_imx27_clk_prepare_enable(imx);
-		if (ret)
-			return ret;
-
+	else
 		pwm_imx27_sw_reset(chip);
-	}
 
 	writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
 	writel(period_cycles, imx->mmio_base + MX3_PWMPR);
@@ -284,10 +281,21 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	writel(cr, imx->mmio_base + MX3_PWMCR);
 
-	if (!state->enabled)
-		pwm_imx27_clk_disable_unprepare(imx);
+	if (imx->enabled != state->enabled) {
+		if (state->enabled) {
+			ret = pwm_imx27_clk_prepare_enable(imx);
+			if (ret)
+				goto out;
+		} else {
+			pwm_imx27_clk_disable_unprepare(imx);
+		}
+		imx->enabled = state->enabled;
+	}
 
-	return 0;
+out:
+	pwm_imx27_clk_disable_unprepare(imx);
+
+	return ret;
 }
 
 static const struct pwm_ops pwm_imx27_ops = {
-- 
2.20.1

Re: [PATCH v2 1/5] pwm: imx27: enable clock unconditional for register access

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1251 bytes --]

On Fri, Sep 25, 2020 at 05:53:26PM +0200, Marco Felsch wrote:
> The current implementation enables the clock if the current PWM state
> is '!enabled' to ensure the register access and left the clock on if the
> new state is 'enabled'. Further apply calls don't enable the clock since
> they relying on the fact the the clock is already running. Change this

s/relying/rely/

> behaviour since it is not very intuitive.
> 
> This commit changes this behaviour. Now the clocks are unconditional

s/unconditional/unconditionally/

> enabled/disabled before/after the register access. If the PWM should be
> turned on (state.enabled) we enable the clock again and vice versa if
> the PWM should be turned off (!state.enabled).
> 
> Therefore I added the enable member to the driver state struct since
> the usage of cstate and pwm_get_state() is a layer violation. I removed
> this violation while on it.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>

I like it.

Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Best regards and thanks
Uwe

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/5] pwm: imx27: enable clock unconditional for register access

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1162 bytes --]

Hello,

On Fri, Sep 25, 2020 at 05:53:26PM +0200, Marco Felsch wrote:
> The current implementation enables the clock if the current PWM state
> is '!enabled' to ensure the register access and left the clock on if the
> new state is 'enabled'. Further apply calls don't enable the clock since
> they relying on the fact the the clock is already running. Change this
> behaviour since it is not very intuitive.
> 
> This commit changes this behaviour. Now the clocks are unconditional
> enabled/disabled before/after the register access. If the PWM should be
> turned on (state.enabled) we enable the clock again and vice versa if
> the PWM should be turned off (!state.enabled).
> 
> Therefore I added the enable member to the driver state struct since
> the usage of cstate and pwm_get_state() is a layer violation. I removed
> this violation while on it.

while looking through patch 2 I found something missing here:
You don't initialize .enabled in .probe().

Best regards
Uwe

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 484 bytes --]

Re: [PATCH v2 1/5] pwm: imx27: enable clock unconditional for register access

[Marco Felsch]

On 20-09-26 15:48, Uwe Kleine-König wrote:
> Hello,
> 
> On Fri, Sep 25, 2020 at 05:53:26PM +0200, Marco Felsch wrote:
> > The current implementation enables the clock if the current PWM state
> > is '!enabled' to ensure the register access and left the clock on if the
> > new state is 'enabled'. Further apply calls don't enable the clock since
> > they relying on the fact the the clock is already running. Change this
> > behaviour since it is not very intuitive.
> > 
> > This commit changes this behaviour. Now the clocks are unconditional
> > enabled/disabled before/after the register access. If the PWM should be
> > turned on (state.enabled) we enable the clock again and vice versa if
> > the PWM should be turned off (!state.enabled).
> > 
> > Therefore I added the enable member to the driver state struct since
> > the usage of cstate and pwm_get_state() is a layer violation. I removed
> > this violation while on it.
> 
> while looking through patch 2 I found something missing here:
> You don't initialize .enabled in .probe().

Arg.. you are right. Thanks for covering this. Didn't recognized it
since I added this in patch 4/5.

Any comments left on this series?

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 |

[PATCH v2 2/5] pwm: imx27: move constant PWMCR register values into probe

[Marco Felsch]

The STOPEN, DOZEN, WAITEN, DBGEN and the CLKSRC bit values never change.
So it should be safe to move this bit settings into probe() and change
only the necessary bits during apply(). Therefore I added the
pwm_imx27_update_bits() helper.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
v2:
- drop software reset from the logic
- fix setting STOPEN, DOZEN, WAITEN and DBGEN bits in case the pwm is
  already enabled

 drivers/pwm/pwm-imx27.c | 37 ++++++++++++++++++++++++++++---------
 1 file changed, 28 insertions(+), 9 deletions(-)

diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
index 7edac4ac6395..b761764b8375 100644
--- a/drivers/pwm/pwm-imx27.c
+++ b/drivers/pwm/pwm-imx27.c
@@ -96,6 +96,16 @@ struct pwm_imx27_chip {
 
 #define to_pwm_imx27_chip(chip)	container_of(chip, struct pwm_imx27_chip, chip)
 
+static void pwm_imx27_update_bits(void __iomem *reg, u32 mask, u32 val)
+{
+	u32 tmp;
+
+	tmp = readl(reg);
+	tmp &= ~mask;
+	tmp |= val & mask;
+	return writel(tmp, reg);
+}
+
 static int pwm_imx27_clk_prepare_enable(struct pwm_imx27_chip *imx)
 {
 	int ret;
@@ -221,7 +231,7 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	unsigned long long c;
 	unsigned long long clkrate;
 	int ret;
-	u32 cr;
+	u32 cr, mask;
 
 	ret = pwm_imx27_clk_prepare_enable(imx);
 	if (ret)
@@ -267,10 +277,7 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	 */
 	imx->duty_cycle = duty_cycles;
 
-	cr = MX3_PWMCR_PRESCALER_SET(prescale) |
-	     MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN |
-	     FIELD_PREP(MX3_PWMCR_CLKSRC, MX3_PWMCR_CLKSRC_IPG_HIGH) |
-	     MX3_PWMCR_DBGEN;
+	cr = MX3_PWMCR_PRESCALER_SET(prescale);
 
 	if (state->polarity == PWM_POLARITY_INVERSED)
 		cr |= FIELD_PREP(MX3_PWMCR_POUTC,
@@ -279,7 +286,9 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	if (state->enabled)
 		cr |= MX3_PWMCR_EN;
 
-	writel(cr, imx->mmio_base + MX3_PWMCR);
+	mask = MX3_PWMCR_PRESCALER | MX3_PWMCR_POUTC | MX3_PWMCR_EN;
+
+	pwm_imx27_update_bits(imx->mmio_base + MX3_PWMCR, mask, cr);
 
 	if (imx->enabled != state->enabled) {
 		if (state->enabled) {
@@ -314,7 +323,7 @@ static int pwm_imx27_probe(struct platform_device *pdev)
 {
 	struct pwm_imx27_chip *imx;
 	int ret;
-	u32 pwmcr;
+	u32 pwmcr, mask;
 
 	imx = devm_kzalloc(&pdev->dev, sizeof(*imx), GFP_KERNEL);
 	if (imx == NULL)
@@ -361,10 +370,20 @@ static int pwm_imx27_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	/* keep clks on if pwm is running */
+	mask = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN |
+	       MX3_PWMCR_DBGEN;
+	pwmcr = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN |
+		MX3_PWMCR_DBGEN;
+	pwm_imx27_update_bits(imx->mmio_base + MX3_PWMCR, mask, pwmcr);
+
+	/* keep clks on and clk settings unchanged if pwm is running */
 	pwmcr = readl(imx->mmio_base + MX3_PWMCR);
-	if (!(pwmcr & MX3_PWMCR_EN))
+	if (!(pwmcr & MX3_PWMCR_EN)) {
+		mask = MX3_PWMCR_CLKSRC;
+		pwmcr = FIELD_PREP(MX3_PWMCR_CLKSRC, MX3_PWMCR_CLKSRC_IPG_HIGH);
+		pwm_imx27_update_bits(imx->mmio_base + MX3_PWMCR, mask, pwmcr);
 		pwm_imx27_clk_disable_unprepare(imx);
+	}
 
 	return pwmchip_add(&imx->chip);
 }
-- 
2.20.1

Re: [PATCH v2 2/5] pwm: imx27: move constant PWMCR register values into probe

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 4666 bytes --]

On Fri, Sep 25, 2020 at 05:53:27PM +0200, Marco Felsch wrote:
> The STOPEN, DOZEN, WAITEN, DBGEN and the CLKSRC bit values never change.
> So it should be safe to move this bit settings into probe() and change
> only the necessary bits during apply(). Therefore I added the
> pwm_imx27_update_bits() helper.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
> v2:
> - drop software reset from the logic
> - fix setting STOPEN, DOZEN, WAITEN and DBGEN bits in case the pwm is
>   already enabled
> 
>  drivers/pwm/pwm-imx27.c | 37 ++++++++++++++++++++++++++++---------
>  1 file changed, 28 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
> index 7edac4ac6395..b761764b8375 100644
> --- a/drivers/pwm/pwm-imx27.c
> +++ b/drivers/pwm/pwm-imx27.c
> @@ -96,6 +96,16 @@ struct pwm_imx27_chip {
>  
>  #define to_pwm_imx27_chip(chip)	container_of(chip, struct pwm_imx27_chip, chip)
>  
> +static void pwm_imx27_update_bits(void __iomem *reg, u32 mask, u32 val)
> +{
> +	u32 tmp;
> +
> +	tmp = readl(reg);
> +	tmp &= ~mask;
> +	tmp |= val & mask;
> +	return writel(tmp, reg);
> +}
> +
>  static int pwm_imx27_clk_prepare_enable(struct pwm_imx27_chip *imx)
>  {
>  	int ret;
> @@ -221,7 +231,7 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	unsigned long long c;
>  	unsigned long long clkrate;
>  	int ret;
> -	u32 cr;
> +	u32 cr, mask;
>  
>  	ret = pwm_imx27_clk_prepare_enable(imx);
>  	if (ret)
> @@ -267,10 +277,7 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	 */
>  	imx->duty_cycle = duty_cycles;
>  
> -	cr = MX3_PWMCR_PRESCALER_SET(prescale) |
> -	     MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN |
> -	     FIELD_PREP(MX3_PWMCR_CLKSRC, MX3_PWMCR_CLKSRC_IPG_HIGH) |
> -	     MX3_PWMCR_DBGEN;
> +	cr = MX3_PWMCR_PRESCALER_SET(prescale);
>  
>  	if (state->polarity == PWM_POLARITY_INVERSED)
>  		cr |= FIELD_PREP(MX3_PWMCR_POUTC,
> @@ -279,7 +286,9 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	if (state->enabled)
>  		cr |= MX3_PWMCR_EN;
>  
> -	writel(cr, imx->mmio_base + MX3_PWMCR);
> +	mask = MX3_PWMCR_PRESCALER | MX3_PWMCR_POUTC | MX3_PWMCR_EN;
> +
> +	pwm_imx27_update_bits(imx->mmio_base + MX3_PWMCR, mask, cr);
>  
>  	if (imx->enabled != state->enabled) {
>  		if (state->enabled) {
> @@ -314,7 +323,7 @@ static int pwm_imx27_probe(struct platform_device *pdev)
>  {
>  	struct pwm_imx27_chip *imx;
>  	int ret;
> -	u32 pwmcr;
> +	u32 pwmcr, mask;
>  
>  	imx = devm_kzalloc(&pdev->dev, sizeof(*imx), GFP_KERNEL);
>  	if (imx == NULL)
> @@ -361,10 +370,20 @@ static int pwm_imx27_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> -	/* keep clks on if pwm is running */
> +	mask = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN |
> +	       MX3_PWMCR_DBGEN;
> +	pwmcr = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN |
> +		MX3_PWMCR_DBGEN;
> +	pwm_imx27_update_bits(imx->mmio_base + MX3_PWMCR, mask, pwmcr);
> +
> +	/* keep clks on and clk settings unchanged if pwm is running */
>  	pwmcr = readl(imx->mmio_base + MX3_PWMCR);
> -	if (!(pwmcr & MX3_PWMCR_EN))
> +	if (!(pwmcr & MX3_PWMCR_EN)) {
> +		mask = MX3_PWMCR_CLKSRC;
> +		pwmcr = FIELD_PREP(MX3_PWMCR_CLKSRC, MX3_PWMCR_CLKSRC_IPG_HIGH);
> +		pwm_imx27_update_bits(imx->mmio_base + MX3_PWMCR, mask, pwmcr);
>  		pwm_imx27_clk_disable_unprepare(imx);
> +	}

You're doing more register accesses here than necessary, that is 3 reads
and two writes while one read and (maybe) one write would be enough.
(Though this doesn't work if you want to use the pwm_imx27_update_bits
helper.)

You'd need to do something like:

	val = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN | MX3_PWMCR_DBGEN;
	mask = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN | MX3_PWMCR_DBGEN;

	pwmcr = readl(imx->mmio_base + MX3_PWMCR);

	if (pwmcr & MX3_PWMCR_EN) {
		imx->enabled = true;
	} else {
		val |= FIELD_PREP(MX3_PWMCR_CLKSRC, MX3_PWMCR_CLKSRC_IPG_HIGH);
		mask |= MX3_PWMCR_CLKSRC;
	}

	pwmcr_new = (pwmcr & ~mask) | val;

	if (pwmcr_new != val)
		writel(imx->mmio_base + MX3_PWMCR, pwmcr_new);

	if (!imx->enabled)
		pwm_imx27_clk_disable_unprepare(imx);

Hmm, not sure this is pretty enough to actually recommend this. I let
you decide.

>  	return pwmchip_add(&imx->chip);

Best regards
Uwe

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 2/5] pwm: imx27: move constant PWMCR register values into probe

[Marco Felsch]

On 20-09-26 15:46, Uwe Kleine-König wrote:
> On Fri, Sep 25, 2020 at 05:53:27PM +0200, Marco Felsch wrote:
> > The STOPEN, DOZEN, WAITEN, DBGEN and the CLKSRC bit values never change.
> > So it should be safe to move this bit settings into probe() and change
> > only the necessary bits during apply(). Therefore I added the
> > pwm_imx27_update_bits() helper.
> > 
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> > v2:
> > - drop software reset from the logic
> > - fix setting STOPEN, DOZEN, WAITEN and DBGEN bits in case the pwm is
> >   already enabled
> > 
> >  drivers/pwm/pwm-imx27.c | 37 ++++++++++++++++++++++++++++---------
> >  1 file changed, 28 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
> > index 7edac4ac6395..b761764b8375 100644
> > --- a/drivers/pwm/pwm-imx27.c
> > +++ b/drivers/pwm/pwm-imx27.c
> > @@ -96,6 +96,16 @@ struct pwm_imx27_chip {
> >  
> >  #define to_pwm_imx27_chip(chip)	container_of(chip, struct pwm_imx27_chip, chip)
> >  
> > +static void pwm_imx27_update_bits(void __iomem *reg, u32 mask, u32 val)
> > +{
> > +	u32 tmp;
> > +
> > +	tmp = readl(reg);
> > +	tmp &= ~mask;
> > +	tmp |= val & mask;
> > +	return writel(tmp, reg);
> > +}
> > +
> >  static int pwm_imx27_clk_prepare_enable(struct pwm_imx27_chip *imx)
> >  {
> >  	int ret;
> > @@ -221,7 +231,7 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> >  	unsigned long long c;
> >  	unsigned long long clkrate;
> >  	int ret;
> > -	u32 cr;
> > +	u32 cr, mask;
> >  
> >  	ret = pwm_imx27_clk_prepare_enable(imx);
> >  	if (ret)
> > @@ -267,10 +277,7 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> >  	 */
> >  	imx->duty_cycle = duty_cycles;
> >  
> > -	cr = MX3_PWMCR_PRESCALER_SET(prescale) |
> > -	     MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN |
> > -	     FIELD_PREP(MX3_PWMCR_CLKSRC, MX3_PWMCR_CLKSRC_IPG_HIGH) |
> > -	     MX3_PWMCR_DBGEN;
> > +	cr = MX3_PWMCR_PRESCALER_SET(prescale);
> >  
> >  	if (state->polarity == PWM_POLARITY_INVERSED)
> >  		cr |= FIELD_PREP(MX3_PWMCR_POUTC,
> > @@ -279,7 +286,9 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> >  	if (state->enabled)
> >  		cr |= MX3_PWMCR_EN;
> >  
> > -	writel(cr, imx->mmio_base + MX3_PWMCR);
> > +	mask = MX3_PWMCR_PRESCALER | MX3_PWMCR_POUTC | MX3_PWMCR_EN;
> > +
> > +	pwm_imx27_update_bits(imx->mmio_base + MX3_PWMCR, mask, cr);
> >  
> >  	if (imx->enabled != state->enabled) {
> >  		if (state->enabled) {
> > @@ -314,7 +323,7 @@ static int pwm_imx27_probe(struct platform_device *pdev)
> >  {
> >  	struct pwm_imx27_chip *imx;
> >  	int ret;
> > -	u32 pwmcr;
> > +	u32 pwmcr, mask;
> >  
> >  	imx = devm_kzalloc(&pdev->dev, sizeof(*imx), GFP_KERNEL);
> >  	if (imx == NULL)
> > @@ -361,10 +370,20 @@ static int pwm_imx27_probe(struct platform_device *pdev)
> >  	if (ret)
> >  		return ret;
> >  
> > -	/* keep clks on if pwm is running */
> > +	mask = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN |
> > +	       MX3_PWMCR_DBGEN;
> > +	pwmcr = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN |
> > +		MX3_PWMCR_DBGEN;
> > +	pwm_imx27_update_bits(imx->mmio_base + MX3_PWMCR, mask, pwmcr);
> > +
> > +	/* keep clks on and clk settings unchanged if pwm is running */
> >  	pwmcr = readl(imx->mmio_base + MX3_PWMCR);
> > -	if (!(pwmcr & MX3_PWMCR_EN))
> > +	if (!(pwmcr & MX3_PWMCR_EN)) {
> > +		mask = MX3_PWMCR_CLKSRC;
> > +		pwmcr = FIELD_PREP(MX3_PWMCR_CLKSRC, MX3_PWMCR_CLKSRC_IPG_HIGH);
> > +		pwm_imx27_update_bits(imx->mmio_base + MX3_PWMCR, mask, pwmcr);
> >  		pwm_imx27_clk_disable_unprepare(imx);
> > +	}
> 
> You're doing more register accesses here than necessary, that is 3 reads
> and two writes while one read and (maybe) one write would be enough.
> (Though this doesn't work if you want to use the pwm_imx27_update_bits
> helper.)

This shouldn't be a real timing issue.

> You'd need to do something like:
> 
> 	val = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN | MX3_PWMCR_DBGEN;
> 	mask = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN | MX3_PWMCR_DBGEN;
> 
> 	pwmcr = readl(imx->mmio_base + MX3_PWMCR);
> 
> 	if (pwmcr & MX3_PWMCR_EN) {
> 		imx->enabled = true;
> 	} else {
> 		val |= FIELD_PREP(MX3_PWMCR_CLKSRC, MX3_PWMCR_CLKSRC_IPG_HIGH);
> 		mask |= MX3_PWMCR_CLKSRC;
> 	}
> 
> 	pwmcr_new = (pwmcr & ~mask) | val;
> 
> 	if (pwmcr_new != val)
> 		writel(imx->mmio_base + MX3_PWMCR, pwmcr_new);
> 
> 	if (!imx->enabled)
> 		pwm_imx27_clk_disable_unprepare(imx);
> 
> Hmm, not sure this is pretty enough to actually recommend this. I let
> you decide.

I don't like this version since it is more confusing.

Regards,
  Marco

> >  	return pwmchip_add(&imx->chip);
> 
> 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 |

[PATCH v2 3/5] pwm: imx27: reset the PWM if it is not running

[Marco Felsch]

Trigger a software reset during probe to clear the FIFO and reset the
register values to their default. According the datasheet the DBGEN,
STOPEN, DOZEN and WAITEN bits should be untouched by the software reset
but this is not the case.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
v2:
- new patch

 drivers/pwm/pwm-imx27.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
index b761764b8375..3b6bcd8d58b7 100644
--- a/drivers/pwm/pwm-imx27.c
+++ b/drivers/pwm/pwm-imx27.c
@@ -183,10 +183,8 @@ static void pwm_imx27_get_state(struct pwm_chip *chip,
 	pwm_imx27_clk_disable_unprepare(imx);
 }
 
-static void pwm_imx27_sw_reset(struct pwm_chip *chip)
+static void pwm_imx27_sw_reset(struct pwm_imx27_chip *imx, struct device *dev)
 {
-	struct pwm_imx27_chip *imx = to_pwm_imx27_chip(chip);
-	struct device *dev = chip->dev;
 	int wait_count = 0;
 	u32 cr;
 
@@ -266,7 +264,7 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	if (imx->enabled)
 		pwm_imx27_wait_fifo_slot(chip, pwm);
 	else
-		pwm_imx27_sw_reset(chip);
+		pwm_imx27_sw_reset(imx, chip->dev);
 
 	writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
 	writel(period_cycles, imx->mmio_base + MX3_PWMPR);
@@ -370,19 +368,23 @@ static int pwm_imx27_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	mask = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN |
-	       MX3_PWMCR_DBGEN;
-	pwmcr = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN |
-		MX3_PWMCR_DBGEN;
-	pwm_imx27_update_bits(imx->mmio_base + MX3_PWMCR, mask, pwmcr);
-
 	/* keep clks on and clk settings unchanged if pwm is running */
 	pwmcr = readl(imx->mmio_base + MX3_PWMCR);
 	if (!(pwmcr & MX3_PWMCR_EN)) {
-		mask = MX3_PWMCR_CLKSRC;
-		pwmcr = FIELD_PREP(MX3_PWMCR_CLKSRC, MX3_PWMCR_CLKSRC_IPG_HIGH);
+		pwm_imx27_sw_reset(imx, &pdev->dev);
+		mask = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN |
+		       MX3_PWMCR_DBGEN | MX3_PWMCR_CLKSRC;
+		pwmcr = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN |
+			MX3_PWMCR_DBGEN |
+			FIELD_PREP(MX3_PWMCR_CLKSRC, MX3_PWMCR_CLKSRC_IPG_HIGH);
 		pwm_imx27_update_bits(imx->mmio_base + MX3_PWMCR, mask, pwmcr);
 		pwm_imx27_clk_disable_unprepare(imx);
+	} else {
+		mask = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN |
+			MX3_PWMCR_DBGEN;
+		pwmcr = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN |
+			MX3_PWMCR_DBGEN;
+		pwm_imx27_update_bits(imx->mmio_base + MX3_PWMCR, mask, pwmcr);
 	}
 
 	return pwmchip_add(&imx->chip);
-- 
2.20.1

Re: [PATCH v2 3/5] pwm: imx27: reset the PWM if it is not running

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 3550 bytes --]

On Fri, Sep 25, 2020 at 05:53:28PM +0200, Marco Felsch wrote:
> Trigger a software reset during probe to clear the FIFO and reset the
> register values to their default. According the datasheet the DBGEN,
> STOPEN, DOZEN and WAITEN bits should be untouched by the software reset
> but this is not the case.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
> v2:
> - new patch
> 
>  drivers/pwm/pwm-imx27.c | 26 ++++++++++++++------------
>  1 file changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
> index b761764b8375..3b6bcd8d58b7 100644
> --- a/drivers/pwm/pwm-imx27.c
> +++ b/drivers/pwm/pwm-imx27.c
> @@ -183,10 +183,8 @@ static void pwm_imx27_get_state(struct pwm_chip *chip,
>  	pwm_imx27_clk_disable_unprepare(imx);
>  }
>  
> -static void pwm_imx27_sw_reset(struct pwm_chip *chip)
> +static void pwm_imx27_sw_reset(struct pwm_imx27_chip *imx, struct device *dev)
>  {
> -	struct pwm_imx27_chip *imx = to_pwm_imx27_chip(chip);
> -	struct device *dev = chip->dev;
>  	int wait_count = 0;
>  	u32 cr;

This is an unrelated hunk that I don't expect to result in any changes
in the code. If you consider it better this way, you should at least
mention it in the commit log.

> @@ -266,7 +264,7 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	if (imx->enabled)
>  		pwm_imx27_wait_fifo_slot(chip, pwm);
>  	else
> -		pwm_imx27_sw_reset(chip);
> +		pwm_imx27_sw_reset(imx, chip->dev);
>  
>  	writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
>  	writel(period_cycles, imx->mmio_base + MX3_PWMPR);
> @@ -370,19 +368,23 @@ static int pwm_imx27_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> -	mask = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN |
> -	       MX3_PWMCR_DBGEN;
> -	pwmcr = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN |
> -		MX3_PWMCR_DBGEN;
> -	pwm_imx27_update_bits(imx->mmio_base + MX3_PWMCR, mask, pwmcr);
> -
>  	/* keep clks on and clk settings unchanged if pwm is running */
>  	pwmcr = readl(imx->mmio_base + MX3_PWMCR);
>  	if (!(pwmcr & MX3_PWMCR_EN)) {
> -		mask = MX3_PWMCR_CLKSRC;
> -		pwmcr = FIELD_PREP(MX3_PWMCR_CLKSRC, MX3_PWMCR_CLKSRC_IPG_HIGH);
> +		pwm_imx27_sw_reset(imx, &pdev->dev);
> +		mask = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN |
> +		       MX3_PWMCR_DBGEN | MX3_PWMCR_CLKSRC;
> +		pwmcr = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN |
> +			MX3_PWMCR_DBGEN |
> +			FIELD_PREP(MX3_PWMCR_CLKSRC, MX3_PWMCR_CLKSRC_IPG_HIGH);
>  		pwm_imx27_update_bits(imx->mmio_base + MX3_PWMCR, mask, pwmcr);
>  		pwm_imx27_clk_disable_unprepare(imx);
> +	} else {
> +		mask = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN |
> +			MX3_PWMCR_DBGEN;
> +		pwmcr = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN |
> +			MX3_PWMCR_DBGEN;
> +		pwm_imx27_update_bits(imx->mmio_base + MX3_PWMCR, mask, pwmcr);
>  	}

IMHO this is worse than the stuff I suggested for one of the earlier
patches because there is much repetition. I'd put

	mask = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN | MX3_PWMCR_DBGEN;
	value = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN | MX3_PWMCR_DBGEN;

before the if and just modify as necessary in the first branch of the
if.

Best regards
Uwe

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 3/5] pwm: imx27: reset the PWM if it is not running

[Marco Felsch]

On 20-09-28 09:30, Uwe Kleine-König wrote:
> On Fri, Sep 25, 2020 at 05:53:28PM +0200, Marco Felsch wrote:
> > Trigger a software reset during probe to clear the FIFO and reset the
> > register values to their default. According the datasheet the DBGEN,
> > STOPEN, DOZEN and WAITEN bits should be untouched by the software reset
> > but this is not the case.
> > 
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> > v2:
> > - new patch
> > 
> >  drivers/pwm/pwm-imx27.c | 26 ++++++++++++++------------
> >  1 file changed, 14 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
> > index b761764b8375..3b6bcd8d58b7 100644
> > --- a/drivers/pwm/pwm-imx27.c
> > +++ b/drivers/pwm/pwm-imx27.c
> > @@ -183,10 +183,8 @@ static void pwm_imx27_get_state(struct pwm_chip *chip,
> >  	pwm_imx27_clk_disable_unprepare(imx);
> >  }
> >  
> > -static void pwm_imx27_sw_reset(struct pwm_chip *chip)
> > +static void pwm_imx27_sw_reset(struct pwm_imx27_chip *imx, struct device *dev)
> >  {
> > -	struct pwm_imx27_chip *imx = to_pwm_imx27_chip(chip);
> > -	struct device *dev = chip->dev;
> >  	int wait_count = 0;
> >  	u32 cr;
> 
> This is an unrelated hunk that I don't expect to result in any changes
> in the code. If you consider it better this way, you should at least
> mention it in the commit log.

IMO this is required due to the usage from the probe. I'm not a fan of
passing the 'struct pwm_chip' before initializing it. So it is not
unrelated. I will mention it in v3.

> 
> > @@ -266,7 +264,7 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> >  	if (imx->enabled)
> >  		pwm_imx27_wait_fifo_slot(chip, pwm);
> >  	else
> > -		pwm_imx27_sw_reset(chip);
> > +		pwm_imx27_sw_reset(imx, chip->dev);
> >  
> >  	writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
> >  	writel(period_cycles, imx->mmio_base + MX3_PWMPR);
> > @@ -370,19 +368,23 @@ static int pwm_imx27_probe(struct platform_device *pdev)
> >  	if (ret)
> >  		return ret;
> >  
> > -	mask = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN |
> > -	       MX3_PWMCR_DBGEN;
> > -	pwmcr = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN |
> > -		MX3_PWMCR_DBGEN;
> > -	pwm_imx27_update_bits(imx->mmio_base + MX3_PWMCR, mask, pwmcr);
> > -
> >  	/* keep clks on and clk settings unchanged if pwm is running */
> >  	pwmcr = readl(imx->mmio_base + MX3_PWMCR);
> >  	if (!(pwmcr & MX3_PWMCR_EN)) {
> > -		mask = MX3_PWMCR_CLKSRC;
> > -		pwmcr = FIELD_PREP(MX3_PWMCR_CLKSRC, MX3_PWMCR_CLKSRC_IPG_HIGH);
> > +		pwm_imx27_sw_reset(imx, &pdev->dev);
> > +		mask = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN |
> > +		       MX3_PWMCR_DBGEN | MX3_PWMCR_CLKSRC;
> > +		pwmcr = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN |
> > +			MX3_PWMCR_DBGEN |
> > +			FIELD_PREP(MX3_PWMCR_CLKSRC, MX3_PWMCR_CLKSRC_IPG_HIGH);
> >  		pwm_imx27_update_bits(imx->mmio_base + MX3_PWMCR, mask, pwmcr);
> >  		pwm_imx27_clk_disable_unprepare(imx);
> > +	} else {
> > +		mask = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN |
> > +			MX3_PWMCR_DBGEN;
> > +		pwmcr = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN |
> > +			MX3_PWMCR_DBGEN;
> > +		pwm_imx27_update_bits(imx->mmio_base + MX3_PWMCR, mask, pwmcr);
> >  	}
> 
> IMHO this is worse than the stuff I suggested for one of the earlier
> patches because there is much repetition. I'd put
> 
> 	mask = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN | MX3_PWMCR_DBGEN;
> 	value = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN | MX3_PWMCR_DBGEN;
> 
> before the if and just modify as necessary in the first branch of the
> if.

I've changed the behaviour in v3.

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 |

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

[Marco Felsch]

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.

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);
 
 	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:
+	 * 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
 	 */
-	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);
 
 	/*
@@ -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);
+
+	return 0;
 }
 
 static struct platform_driver imx_pwm_driver = {
-- 
2.20.1

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

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 8836 bytes --]

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.

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.

> 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.

>  	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:

> +	 * 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?

>  	 */
> -	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);

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.

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

>  	/*
> @@ -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.

Best regards
Uwe

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Marco Felsch]

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 |

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

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 11080 bytes --]

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?

> > > 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.

> > >  	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. Also duty_cycle
is a local variable here only. (That's at least what I assumed writing
the above snippet.)

> 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?) 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.

> > @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. 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).

Best regards
Uwe

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Marco Felsch]

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 |

[PATCH v2 5/5] pwm: imx27: wait till the duty cycle is applied

[Marco Felsch]

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(+)

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);
 
 	/*
-- 
2.20.1

Re: [PATCH v2 5/5] pwm: imx27: wait till the duty cycle is applied

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2595 bytes --]

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

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 5/5] pwm: imx27: wait till the duty cycle is applied

[Marco Felsch]

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/ |



-- 
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 |