diff mbox series

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

Message ID 20200925155330.32301-4-m.felsch@pengutronix.de (mailing list archive)
State New, archived
Headers show
Series PWM i.MX27 fix disabled state for inverted signals | expand

Commit Message

Marco Felsch Sept. 25, 2020, 3:53 p.m. UTC 
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(-)

Comments

Uwe Kleine-König Sept. 28, 2020, 7:30 a.m. UTC | #1 
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
Marco Felsch Sept. 28, 2020, 9:29 a.m. UTC | #2 
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/ |
diff mbox series

Patch

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