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

[PATCH 0/3] 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.

Comments and testers are welcome :)

Regards,
  Marco

Marco Felsch (3):
  pwm: imx27: track clock enable/disable to simplify code
  pwm: imx27: move static pwmcr values into probe() function
  pwm: imx27: fix disable state for inverted PWMs

 drivers/pwm/pwm-imx27.c | 119 ++++++++++++++++++++++++++--------------
 1 file changed, 79 insertions(+), 40 deletions(-)

-- 
2.20.1

[PATCH 0/3] 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.

Comments and testers are welcome :)

Regards,
  Marco

Marco Felsch (3):
  pwm: imx27: track clock enable/disable to simplify code
  pwm: imx27: move static pwmcr values into probe() function
  pwm: imx27: fix disable state for inverted PWMs

 drivers/pwm/pwm-imx27.c | 119 ++++++++++++++++++++++++++--------------
 1 file changed, 79 insertions(+), 40 deletions(-)

-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/3] pwm: imx27: track clock enable/disable to simplify code

[Marco Felsch]

Introduce a simple clock state so we can enable/disable the clock
without the need to check if we are running or not.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/pwm/pwm-imx27.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
index c50d453552bd..3cf9f1774244 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 clk_on;
 };
 
 #define to_pwm_imx27_chip(chip)	container_of(chip, struct pwm_imx27_chip, chip)
@@ -99,6 +100,9 @@ static int pwm_imx27_clk_prepare_enable(struct pwm_imx27_chip *imx)
 {
 	int ret;
 
+	if (imx->clk_on)
+		return 0;
+
 	ret = clk_prepare_enable(imx->clk_ipg);
 	if (ret)
 		return ret;
@@ -109,13 +113,20 @@ static int pwm_imx27_clk_prepare_enable(struct pwm_imx27_chip *imx)
 		return ret;
 	}
 
+	imx->clk_on = true;
+
 	return 0;
 }
 
 static void pwm_imx27_clk_disable_unprepare(struct pwm_imx27_chip *imx)
 {
+	if (!imx->clk_on)
+		return;
+
 	clk_disable_unprepare(imx->clk_per);
 	clk_disable_unprepare(imx->clk_ipg);
+
+	imx->clk_on = false;
 }
 
 static void pwm_imx27_get_state(struct pwm_chip *chip,
@@ -223,6 +234,10 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	int ret;
 	u32 cr;
 
+	ret = pwm_imx27_clk_prepare_enable(imx);
+	if (ret)
+		return ret;
+
 	pwm_get_state(pwm, &cstate);
 
 	clkrate = clk_get_rate(imx->clk_per);
@@ -254,10 +269,6 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	if (cstate.enabled) {
 		pwm_imx27_wait_fifo_slot(chip, pwm);
 	} else {
-		ret = pwm_imx27_clk_prepare_enable(imx);
-		if (ret)
-			return ret;
-
 		pwm_imx27_sw_reset(chip);
 	}
 
-- 
2.20.1

[PATCH 1/3] pwm: imx27: track clock enable/disable to simplify code

[Marco Felsch]

Introduce a simple clock state so we can enable/disable the clock
without the need to check if we are running or not.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/pwm/pwm-imx27.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
index c50d453552bd..3cf9f1774244 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 clk_on;
 };
 
 #define to_pwm_imx27_chip(chip)	container_of(chip, struct pwm_imx27_chip, chip)
@@ -99,6 +100,9 @@ static int pwm_imx27_clk_prepare_enable(struct pwm_imx27_chip *imx)
 {
 	int ret;
 
+	if (imx->clk_on)
+		return 0;
+
 	ret = clk_prepare_enable(imx->clk_ipg);
 	if (ret)
 		return ret;
@@ -109,13 +113,20 @@ static int pwm_imx27_clk_prepare_enable(struct pwm_imx27_chip *imx)
 		return ret;
 	}
 
+	imx->clk_on = true;
+
 	return 0;
 }
 
 static void pwm_imx27_clk_disable_unprepare(struct pwm_imx27_chip *imx)
 {
+	if (!imx->clk_on)
+		return;
+
 	clk_disable_unprepare(imx->clk_per);
 	clk_disable_unprepare(imx->clk_ipg);
+
+	imx->clk_on = false;
 }
 
 static void pwm_imx27_get_state(struct pwm_chip *chip,
@@ -223,6 +234,10 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	int ret;
 	u32 cr;
 
+	ret = pwm_imx27_clk_prepare_enable(imx);
+	if (ret)
+		return ret;
+
 	pwm_get_state(pwm, &cstate);
 
 	clkrate = clk_get_rate(imx->clk_per);
@@ -254,10 +269,6 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	if (cstate.enabled) {
 		pwm_imx27_wait_fifo_slot(chip, pwm);
 	} else {
-		ret = pwm_imx27_clk_prepare_enable(imx);
-		if (ret)
-			return ret;
-
 		pwm_imx27_sw_reset(chip);
 	}
 
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] pwm: imx27: track clock enable/disable to simplify code

[Uwe Kleine-König]

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

Hello,

On Wed, Sep 09, 2020 at 03:07:37PM +0200, Marco Felsch wrote:
> Introduce a simple clock state so we can enable/disable the clock
> without the need to check if we are running or not.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
>  drivers/pwm/pwm-imx27.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)

This doesn't suggest to be more simple.

> @@ -223,6 +234,10 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	int ret;
>  	u32 cr;
>  
> +	ret = pwm_imx27_clk_prepare_enable(imx);
> +	if (ret)
> +		return ret;
> +
>  	pwm_get_state(pwm, &cstate);
>  
>  	clkrate = clk_get_rate(imx->clk_per);
> @@ -254,10 +269,6 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	if (cstate.enabled) {
>  		pwm_imx27_wait_fifo_slot(chip, pwm);
>  	} else {
> -		ret = pwm_imx27_clk_prepare_enable(imx);
> -		if (ret)
> -			return ret;
> -
>  		pwm_imx27_sw_reset(chip);
>  	}

There are two clocks, I assume one if for register access and the other
to actually drive the PWM. With that what I would consider simple is to
enable the register clock at the start of each function and disable it
at the end. And for the hardware clock enable it when the hardware
should be enabled and disable it when it should be disabled.

Probably this doesn't reduce the line count, too, but it makes the
function more efficient (if this is measurable at all).

If you want to keep the pwm_imx27_clk_prepare_enable() function that
handles both clocks, just calling pwm_imx27_clk_prepare_enable
unconditionally at the function entry and
pwm_imx27_clk_disable_unprepare at the end should be easier and not
require the .on member in the driver struct.

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 1/3] pwm: imx27: track clock enable/disable to simplify code

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 1969 bytes --]

Hello,

On Wed, Sep 09, 2020 at 03:07:37PM +0200, Marco Felsch wrote:
> Introduce a simple clock state so we can enable/disable the clock
> without the need to check if we are running or not.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
>  drivers/pwm/pwm-imx27.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)

This doesn't suggest to be more simple.

> @@ -223,6 +234,10 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	int ret;
>  	u32 cr;
>  
> +	ret = pwm_imx27_clk_prepare_enable(imx);
> +	if (ret)
> +		return ret;
> +
>  	pwm_get_state(pwm, &cstate);
>  
>  	clkrate = clk_get_rate(imx->clk_per);
> @@ -254,10 +269,6 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	if (cstate.enabled) {
>  		pwm_imx27_wait_fifo_slot(chip, pwm);
>  	} else {
> -		ret = pwm_imx27_clk_prepare_enable(imx);
> -		if (ret)
> -			return ret;
> -
>  		pwm_imx27_sw_reset(chip);
>  	}

There are two clocks, I assume one if for register access and the other
to actually drive the PWM. With that what I would consider simple is to
enable the register clock at the start of each function and disable it
at the end. And for the hardware clock enable it when the hardware
should be enabled and disable it when it should be disabled.

Probably this doesn't reduce the line count, too, but it makes the
function more efficient (if this is measurable at all).

If you want to keep the pwm_imx27_clk_prepare_enable() function that
handles both clocks, just calling pwm_imx27_clk_prepare_enable
unconditionally at the function entry and
pwm_imx27_clk_disable_unprepare at the end should be easier and not
require the .on member in the driver struct.

Best regards
Uwe

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

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/3] pwm: imx27: move static pwmcr values into probe() function

[Marco Felsch]

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

Furthermore the patch adds the support to reset the pwm device during
probe() if the pwm device is disabled.

Both steps are required in preparation of the further patch which fixes
the "pwm-disabled" state for inverted pwms.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/pwm/pwm-imx27.c | 41 +++++++++++++++++++++++++++++------------
 1 file changed, 29 insertions(+), 12 deletions(-)

diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
index 3cf9f1774244..30388a9ece04 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;
@@ -183,10 +193,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;
 
@@ -232,7 +240,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)
@@ -269,7 +277,7 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	if (cstate.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);
@@ -281,10 +289,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,
@@ -293,7 +298,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 (!state->enabled)
 		pwm_imx27_clk_disable_unprepare(imx);
@@ -364,10 +371,20 @@ static int pwm_imx27_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	/* keep clks on if pwm is running */
+	/* Keep clks on and pwm settings unchanged if the PWM is already running */
 	pwmcr = readl(imx->mmio_base + MX3_PWMCR);
-	if (!(pwmcr & MX3_PWMCR_EN))
+	if (!(pwmcr & MX3_PWMCR_EN)) {
+		u32 mask;
+
+		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);
+	}
 
 	return pwmchip_add(&imx->chip);
 }
-- 
2.20.1

[PATCH 2/3] pwm: imx27: move static pwmcr values into probe() function

[Marco Felsch]

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

Furthermore the patch adds the support to reset the pwm device during
probe() if the pwm device is disabled.

Both steps are required in preparation of the further patch which fixes
the "pwm-disabled" state for inverted pwms.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/pwm/pwm-imx27.c | 41 +++++++++++++++++++++++++++++------------
 1 file changed, 29 insertions(+), 12 deletions(-)

diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
index 3cf9f1774244..30388a9ece04 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;
@@ -183,10 +193,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;
 
@@ -232,7 +240,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)
@@ -269,7 +277,7 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	if (cstate.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);
@@ -281,10 +289,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,
@@ -293,7 +298,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 (!state->enabled)
 		pwm_imx27_clk_disable_unprepare(imx);
@@ -364,10 +371,20 @@ static int pwm_imx27_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	/* keep clks on if pwm is running */
+	/* Keep clks on and pwm settings unchanged if the PWM is already running */
 	pwmcr = readl(imx->mmio_base + MX3_PWMCR);
-	if (!(pwmcr & MX3_PWMCR_EN))
+	if (!(pwmcr & MX3_PWMCR_EN)) {
+		u32 mask;
+
+		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);
+	}
 
 	return pwmchip_add(&imx->chip);
 }
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] pwm: imx27: move static pwmcr values into probe() function

[Uwe Kleine-König]

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

Hello,

the usage of "static" in the subject is unclear. I guess you mean:

	pwm: imx27: setup some PWMCR bits in .probe()

On Wed, Sep 09, 2020 at 03:07:38PM +0200, Marco Felsch wrote:
> The STOPEN, DOZEN, WAITEN, DBGEN and the CLKSRC bit values never change.
> So it should be save to move this bit settings into probe() and change

s/save/safe/

> only the necessary bits during apply(). Therefore I added the
> pwm_imx27_update_bits() helper.
> 
> Furthermore the patch adds the support to reset the pwm device during
> probe() if the pwm device is disabled.

IMHO this needs a better motivation ...

> Both steps are required in preparation of the further patch which fixes
> the "pwm-disabled" state for inverted pwms.

... and should maybe go into this other patch?

> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
>  drivers/pwm/pwm-imx27.c | 41 +++++++++++++++++++++++++++++------------
>  1 file changed, 29 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
> index 3cf9f1774244..30388a9ece04 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;
> @@ -183,10 +193,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;

This change is fine, but it does belong into a separate patch.

>  	int wait_count = 0;
>  	u32 cr;
>  
> @@ -232,7 +240,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)
> @@ -269,7 +277,7 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	if (cstate.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);
> @@ -281,10 +289,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,
> @@ -293,7 +298,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 (!state->enabled)
>  		pwm_imx27_clk_disable_unprepare(imx);
> @@ -364,10 +371,20 @@ static int pwm_imx27_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> -	/* keep clks on if pwm is running */
> +	/* Keep clks on and pwm settings unchanged if the PWM is already running */
>  	pwmcr = readl(imx->mmio_base + MX3_PWMCR);
> -	if (!(pwmcr & MX3_PWMCR_EN))
> +	if (!(pwmcr & MX3_PWMCR_EN)) {
> +		u32 mask;
> +
> +		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);
> +	}

So you don't enforce MX3_PWMCR_STOPEN (and the others) if the PWM is
already running. Is this by design?

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 2/3] pwm: imx27: move static pwmcr values into probe() function

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 4699 bytes --]

Hello,

the usage of "static" in the subject is unclear. I guess you mean:

	pwm: imx27: setup some PWMCR bits in .probe()

On Wed, Sep 09, 2020 at 03:07:38PM +0200, Marco Felsch wrote:
> The STOPEN, DOZEN, WAITEN, DBGEN and the CLKSRC bit values never change.
> So it should be save to move this bit settings into probe() and change

s/save/safe/

> only the necessary bits during apply(). Therefore I added the
> pwm_imx27_update_bits() helper.
> 
> Furthermore the patch adds the support to reset the pwm device during
> probe() if the pwm device is disabled.

IMHO this needs a better motivation ...

> Both steps are required in preparation of the further patch which fixes
> the "pwm-disabled" state for inverted pwms.

... and should maybe go into this other patch?

> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
>  drivers/pwm/pwm-imx27.c | 41 +++++++++++++++++++++++++++++------------
>  1 file changed, 29 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
> index 3cf9f1774244..30388a9ece04 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;
> @@ -183,10 +193,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;

This change is fine, but it does belong into a separate patch.

>  	int wait_count = 0;
>  	u32 cr;
>  
> @@ -232,7 +240,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)
> @@ -269,7 +277,7 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	if (cstate.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);
> @@ -281,10 +289,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,
> @@ -293,7 +298,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 (!state->enabled)
>  		pwm_imx27_clk_disable_unprepare(imx);
> @@ -364,10 +371,20 @@ static int pwm_imx27_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> -	/* keep clks on if pwm is running */
> +	/* Keep clks on and pwm settings unchanged if the PWM is already running */
>  	pwmcr = readl(imx->mmio_base + MX3_PWMCR);
> -	if (!(pwmcr & MX3_PWMCR_EN))
> +	if (!(pwmcr & MX3_PWMCR_EN)) {
> +		u32 mask;
> +
> +		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);
> +	}

So you don't enforce MX3_PWMCR_STOPEN (and the others) if the PWM is
already running. Is this by design?

Best regards
Uwe

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

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/3] 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 or 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() else the PWMCR.EN is cleared too. Therefore the
pwm_imx27_wait_fifo_slot() is extended to guarantee a free FIFO slot.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/pwm/pwm-imx27.c | 69 ++++++++++++++++++++++++-----------------
 1 file changed, 40 insertions(+), 29 deletions(-)

diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
index 30388a9ece04..d98e8df14eb9 100644
--- a/drivers/pwm/pwm-imx27.c
+++ b/drivers/pwm/pwm-imx27.c
@@ -92,6 +92,7 @@ struct pwm_imx27_chip {
 	 */
 	unsigned int duty_cycle;
 	bool clk_on;
+	bool enabled;
 };
 
 #define to_pwm_imx27_chip(chip)	container_of(chip, struct pwm_imx27_chip, chip)
@@ -151,12 +152,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:
@@ -179,8 +177,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);
@@ -209,8 +207,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_wait_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;
@@ -226,9 +224,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 0;
 }
 
 static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
@@ -270,17 +272,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_wait_fifo_slot(chip, pwm);
+	if (ret)
+		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 (cstate.enabled) {
-		pwm_imx27_wait_fifo_slot(chip, pwm);
-	} else {
-		pwm_imx27_sw_reset(imx, chip->dev);
-	}
-
-	writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
+	if (state->enabled)
+		writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
+	else
+		writel(0, imx->mmio_base + MX3_PWMSAR);
 	writel(period_cycles, imx->mmio_base + MX3_PWMPR);
 
 	/*
@@ -288,24 +298,21 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	 * MX3_PWMSAR register can't be read (i.e. when the PWM is disabled).
 	 */
 	imx->duty_cycle = duty_cycles;
+	imx->enabled = state->enabled;
 
 	cr = MX3_PWMCR_PRESCALER_SET(prescale);
-
 	if (state->polarity == PWM_POLARITY_INVERSED)
-		cr |= FIELD_PREP(MX3_PWMCR_POUTC,
-				MX3_PWMCR_POUTC_INVERTED);
+		cr |= FIELD_PREP(MX3_PWMCR_POUTC, MX3_PWMCR_POUTC_INVERTED);
 
-	if (state->enabled)
-		cr |= MX3_PWMCR_EN;
-
-	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);
 
+out:
 	if (!state->enabled)
 		pwm_imx27_clk_disable_unprepare(imx);
 
-	return 0;
+	return ret;
 }
 
 static const struct pwm_ops pwm_imx27_ops = {
@@ -378,12 +385,16 @@ static int pwm_imx27_probe(struct platform_device *pdev)
 
 		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 {
+		imx->enabled = true;
 	}
 
 	return pwmchip_add(&imx->chip);
-- 
2.20.1

[PATCH 3/3] 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 or 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() else the PWMCR.EN is cleared too. Therefore the
pwm_imx27_wait_fifo_slot() is extended to guarantee a free FIFO slot.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/pwm/pwm-imx27.c | 69 ++++++++++++++++++++++++-----------------
 1 file changed, 40 insertions(+), 29 deletions(-)

diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
index 30388a9ece04..d98e8df14eb9 100644
--- a/drivers/pwm/pwm-imx27.c
+++ b/drivers/pwm/pwm-imx27.c
@@ -92,6 +92,7 @@ struct pwm_imx27_chip {
 	 */
 	unsigned int duty_cycle;
 	bool clk_on;
+	bool enabled;
 };
 
 #define to_pwm_imx27_chip(chip)	container_of(chip, struct pwm_imx27_chip, chip)
@@ -151,12 +152,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:
@@ -179,8 +177,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);
@@ -209,8 +207,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_wait_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;
@@ -226,9 +224,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 0;
 }
 
 static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
@@ -270,17 +272,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_wait_fifo_slot(chip, pwm);
+	if (ret)
+		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 (cstate.enabled) {
-		pwm_imx27_wait_fifo_slot(chip, pwm);
-	} else {
-		pwm_imx27_sw_reset(imx, chip->dev);
-	}
-
-	writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
+	if (state->enabled)
+		writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
+	else
+		writel(0, imx->mmio_base + MX3_PWMSAR);
 	writel(period_cycles, imx->mmio_base + MX3_PWMPR);
 
 	/*
@@ -288,24 +298,21 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	 * MX3_PWMSAR register can't be read (i.e. when the PWM is disabled).
 	 */
 	imx->duty_cycle = duty_cycles;
+	imx->enabled = state->enabled;
 
 	cr = MX3_PWMCR_PRESCALER_SET(prescale);
-
 	if (state->polarity == PWM_POLARITY_INVERSED)
-		cr |= FIELD_PREP(MX3_PWMCR_POUTC,
-				MX3_PWMCR_POUTC_INVERTED);
+		cr |= FIELD_PREP(MX3_PWMCR_POUTC, MX3_PWMCR_POUTC_INVERTED);
 
-	if (state->enabled)
-		cr |= MX3_PWMCR_EN;
-
-	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);
 
+out:
 	if (!state->enabled)
 		pwm_imx27_clk_disable_unprepare(imx);
 
-	return 0;
+	return ret;
 }
 
 static const struct pwm_ops pwm_imx27_ops = {
@@ -378,12 +385,16 @@ static int pwm_imx27_probe(struct platform_device *pdev)
 
 		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 {
+		imx->enabled = true;
 	}
 
 	return pwmchip_add(&imx->chip);
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/3] pwm: imx27: fix disable state for inverted PWMs

[Uwe Kleine-König]

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

Hello Marco,

On Wed, Sep 09, 2020 at 03:07:39PM +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 or setting .duty_cycle = 0 results in the output driving

s/or/together with/

> 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() else the PWMCR.EN is cleared too. Therefore the

s/else/otherwise/

> pwm_imx27_wait_fifo_slot() is extended to guarantee a free FIFO slot.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>

Without having looked deeper into this patch the approach described here
looks sound to me. Dropping the sw-reset in .apply is also nice as this
results in a spike.

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 3/3] pwm: imx27: fix disable state for inverted PWMs

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 1446 bytes --]

Hello Marco,

On Wed, Sep 09, 2020 at 03:07:39PM +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 or setting .duty_cycle = 0 results in the output driving

s/or/together with/

> 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() else the PWMCR.EN is cleared too. Therefore the

s/else/otherwise/

> pwm_imx27_wait_fifo_slot() is extended to guarantee a free FIFO slot.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>

Without having looked deeper into this patch the approach described here
looks sound to me. Dropping the sw-reset in .apply is also nice as this
results in a spike.

Best regards
Uwe

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

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel