[4/4] pwm: imx27: Unconditionally write state to hardware
diff mbox series

Message ID 20191021105739.1357629-4-thierry.reding@gmail.com
State New
Headers show
Series
  • [1/4] pwm: Read initial hardware state at request time
Related show

Commit Message

Thierry Reding Oct. 21, 2019, 10:57 a.m. UTC
The i.MX driver currently uses a shortcut and doesn't write all of the
state through to the hardware when the PWM is disabled. This causes an
inconsistent state to be read back by consumers with the result of them
malfunctioning.

Fix this by always writing the full state through to the hardware
registers so that the correct state can always be read back.

Signed-off-by: Thierry Reding <thierry.reding@gmail.com>
---
 drivers/pwm/pwm-imx27.c | 120 ++++++++++++++++++++--------------------
 1 file changed, 59 insertions(+), 61 deletions(-)

Comments

Michal Vokáč Oct. 21, 2019, 1:49 p.m. UTC | #1
+Adam

On 21. 10. 19 12:57, Thierry Reding wrote:
> The i.MX driver currently uses a shortcut and doesn't write all of the
> state through to the hardware when the PWM is disabled. This causes an
> inconsistent state to be read back by consumers with the result of them
> malfunctioning.
> 
> Fix this by always writing the full state through to the hardware
> registers so that the correct state can always be read back.

Gave it another shot and got expected results.

Tested-by: Michal Vokáč <michal.vokac@ysoft.com>

> Signed-off-by: Thierry Reding <thierry.reding@gmail.com>
> ---
>   drivers/pwm/pwm-imx27.c | 120 ++++++++++++++++++++--------------------
>   1 file changed, 59 insertions(+), 61 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
> index 4113d5cd4c62..59d8b1289808 100644
> --- a/drivers/pwm/pwm-imx27.c
> +++ b/drivers/pwm/pwm-imx27.c
> @@ -230,70 +230,68 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>   
>   	pwm_get_state(pwm, &cstate);
>   
> -	if (state->enabled) {
> -		c = clk_get_rate(imx->clk_per);
> -		c *= state->period;
> -
> -		do_div(c, 1000000000);
> -		period_cycles = c;
> -
> -		prescale = period_cycles / 0x10000 + 1;
> -
> -		period_cycles /= prescale;
> -		c = (unsigned long long)period_cycles * state->duty_cycle;
> -		do_div(c, state->period);
> -		duty_cycles = c;
> -
> -		/*
> -		 * according to imx pwm RM, the real period value should be
> -		 * PERIOD value in PWMPR plus 2.
> -		 */
> -		if (period_cycles > 2)
> -			period_cycles -= 2;
> -		else
> -			period_cycles = 0;
> -
> -		/*
> -		 * 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) {
> -			pwm_imx27_wait_fifo_slot(chip, pwm);
> -		} else {
> -			ret = pwm_imx27_clk_prepare_enable(chip);
> -			if (ret)
> -				return ret;
> -
> -			pwm_imx27_sw_reset(chip);
> -		}
> -
> -		writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
> -		writel(period_cycles, imx->mmio_base + MX3_PWMPR);
> -
> -		/*
> -		 * Store the duty cycle for future reference in cases where
> -		 * the MX3_PWMSAR register can't be read (i.e. when the PWM
> -		 * is disabled).
> -		 */
> -		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 | MX3_PWMCR_EN;
> -
> -		if (state->polarity == PWM_POLARITY_INVERSED)
> -			cr |= FIELD_PREP(MX3_PWMCR_POUTC,
> -					MX3_PWMCR_POUTC_INVERTED);
> -
> -		writel(cr, imx->mmio_base + MX3_PWMCR);
> -	} else if (cstate.enabled) {
> -		writel(0, imx->mmio_base + MX3_PWMCR);
> +	c = clk_get_rate(imx->clk_per);
> +	c *= state->period;
>   
> -		pwm_imx27_clk_disable_unprepare(chip);
> +	do_div(c, 1000000000);
> +	period_cycles = c;
> +
> +	prescale = period_cycles / 0x10000 + 1;
> +
> +	period_cycles /= prescale;
> +	c = (unsigned long long)period_cycles * state->duty_cycle;
> +	do_div(c, state->period);
> +	duty_cycles = c;
> +
> +	/*
> +	 * according to imx pwm RM, the real period value should be PERIOD
> +	 * value in PWMPR plus 2.
> +	 */
> +	if (period_cycles > 2)
> +		period_cycles -= 2;
> +	else
> +		period_cycles = 0;
> +
> +	/*
> +	 * 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) {
> +		pwm_imx27_wait_fifo_slot(chip, pwm);
> +	} else {
> +		ret = pwm_imx27_clk_prepare_enable(chip);
> +		if (ret)
> +			return ret;
> +
> +		pwm_imx27_sw_reset(chip);
>   	}
>   
> +	writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
> +	writel(period_cycles, imx->mmio_base + MX3_PWMPR);
> +
> +	/*
> +	 * Store the duty cycle for future reference in cases where the
> +	 * MX3_PWMSAR register can't be read (i.e. when the PWM is disabled).
> +	 */
> +	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;
> +
> +	if (state->polarity == PWM_POLARITY_INVERSED)
> +		cr |= FIELD_PREP(MX3_PWMCR_POUTC,
> +				MX3_PWMCR_POUTC_INVERTED);
> +
> +	if (state->enabled)
> +		cr |= MX3_PWMCR_EN;
> +
> +	writel(cr, imx->mmio_base + MX3_PWMCR);
> +
> +	if (!state->enabled && cstate.enabled)
> +		pwm_imx27_clk_disable_unprepare(chip);
> +
>   	return 0;
>   }
>   
>
Adam Ford Oct. 21, 2019, 2:21 p.m. UTC | #2
On Mon, Oct 21, 2019 at 8:49 AM Michal Vokáč <michal.vokac@ysoft.com> wrote:
>
> +Adam
>
> On 21. 10. 19 12:57, Thierry Reding wrote:
> > The i.MX driver currently uses a shortcut and doesn't write all of the
> > state through to the hardware when the PWM is disabled. This causes an
> > inconsistent state to be read back by consumers with the result of them
> > malfunctioning.
> >
> > Fix this by always writing the full state through to the hardware
> > registers so that the correct state can always be read back.
>
> Gave it another shot and got expected results.
>
> Tested-by: Michal Vokáč <michal.vokac@ysoft.com>

Tested-by: Adam Ford <aford173@gmail.com>

>
> > Signed-off-by: Thierry Reding <thierry.reding@gmail.com>
> > ---
> >   drivers/pwm/pwm-imx27.c | 120 ++++++++++++++++++++--------------------
> >   1 file changed, 59 insertions(+), 61 deletions(-)
> >
> > diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
> > index 4113d5cd4c62..59d8b1289808 100644
> > --- a/drivers/pwm/pwm-imx27.c
> > +++ b/drivers/pwm/pwm-imx27.c
> > @@ -230,70 +230,68 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> >
> >       pwm_get_state(pwm, &cstate);
> >
> > -     if (state->enabled) {
> > -             c = clk_get_rate(imx->clk_per);
> > -             c *= state->period;
> > -
> > -             do_div(c, 1000000000);
> > -             period_cycles = c;
> > -
> > -             prescale = period_cycles / 0x10000 + 1;
> > -
> > -             period_cycles /= prescale;
> > -             c = (unsigned long long)period_cycles * state->duty_cycle;
> > -             do_div(c, state->period);
> > -             duty_cycles = c;
> > -
> > -             /*
> > -              * according to imx pwm RM, the real period value should be
> > -              * PERIOD value in PWMPR plus 2.
> > -              */
> > -             if (period_cycles > 2)
> > -                     period_cycles -= 2;
> > -             else
> > -                     period_cycles = 0;
> > -
> > -             /*
> > -              * 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) {
> > -                     pwm_imx27_wait_fifo_slot(chip, pwm);
> > -             } else {
> > -                     ret = pwm_imx27_clk_prepare_enable(chip);
> > -                     if (ret)
> > -                             return ret;
> > -
> > -                     pwm_imx27_sw_reset(chip);
> > -             }
> > -
> > -             writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
> > -             writel(period_cycles, imx->mmio_base + MX3_PWMPR);
> > -
> > -             /*
> > -              * Store the duty cycle for future reference in cases where
> > -              * the MX3_PWMSAR register can't be read (i.e. when the PWM
> > -              * is disabled).
> > -              */
> > -             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 | MX3_PWMCR_EN;
> > -
> > -             if (state->polarity == PWM_POLARITY_INVERSED)
> > -                     cr |= FIELD_PREP(MX3_PWMCR_POUTC,
> > -                                     MX3_PWMCR_POUTC_INVERTED);
> > -
> > -             writel(cr, imx->mmio_base + MX3_PWMCR);
> > -     } else if (cstate.enabled) {
> > -             writel(0, imx->mmio_base + MX3_PWMCR);
> > +     c = clk_get_rate(imx->clk_per);
> > +     c *= state->period;
> >
> > -             pwm_imx27_clk_disable_unprepare(chip);
> > +     do_div(c, 1000000000);
> > +     period_cycles = c;
> > +
> > +     prescale = period_cycles / 0x10000 + 1;
> > +
> > +     period_cycles /= prescale;
> > +     c = (unsigned long long)period_cycles * state->duty_cycle;
> > +     do_div(c, state->period);
> > +     duty_cycles = c;
> > +
> > +     /*
> > +      * according to imx pwm RM, the real period value should be PERIOD
> > +      * value in PWMPR plus 2.
> > +      */
> > +     if (period_cycles > 2)
> > +             period_cycles -= 2;
> > +     else
> > +             period_cycles = 0;
> > +
> > +     /*
> > +      * 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) {
> > +             pwm_imx27_wait_fifo_slot(chip, pwm);
> > +     } else {
> > +             ret = pwm_imx27_clk_prepare_enable(chip);
> > +             if (ret)
> > +                     return ret;
> > +
> > +             pwm_imx27_sw_reset(chip);
> >       }
> >
> > +     writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
> > +     writel(period_cycles, imx->mmio_base + MX3_PWMPR);
> > +
> > +     /*
> > +      * Store the duty cycle for future reference in cases where the
> > +      * MX3_PWMSAR register can't be read (i.e. when the PWM is disabled).
> > +      */
> > +     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;
> > +
> > +     if (state->polarity == PWM_POLARITY_INVERSED)
> > +             cr |= FIELD_PREP(MX3_PWMCR_POUTC,
> > +                             MX3_PWMCR_POUTC_INVERTED);
> > +
> > +     if (state->enabled)
> > +             cr |= MX3_PWMCR_EN;
> > +
> > +     writel(cr, imx->mmio_base + MX3_PWMCR);
> > +
> > +     if (!state->enabled && cstate.enabled)
> > +             pwm_imx27_clk_disable_unprepare(chip);
> > +
> >       return 0;
> >   }
> >
> >
>

Patch
diff mbox series

diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
index 4113d5cd4c62..59d8b1289808 100644
--- a/drivers/pwm/pwm-imx27.c
+++ b/drivers/pwm/pwm-imx27.c
@@ -230,70 +230,68 @@  static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	pwm_get_state(pwm, &cstate);
 
-	if (state->enabled) {
-		c = clk_get_rate(imx->clk_per);
-		c *= state->period;
-
-		do_div(c, 1000000000);
-		period_cycles = c;
-
-		prescale = period_cycles / 0x10000 + 1;
-
-		period_cycles /= prescale;
-		c = (unsigned long long)period_cycles * state->duty_cycle;
-		do_div(c, state->period);
-		duty_cycles = c;
-
-		/*
-		 * according to imx pwm RM, the real period value should be
-		 * PERIOD value in PWMPR plus 2.
-		 */
-		if (period_cycles > 2)
-			period_cycles -= 2;
-		else
-			period_cycles = 0;
-
-		/*
-		 * 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) {
-			pwm_imx27_wait_fifo_slot(chip, pwm);
-		} else {
-			ret = pwm_imx27_clk_prepare_enable(chip);
-			if (ret)
-				return ret;
-
-			pwm_imx27_sw_reset(chip);
-		}
-
-		writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
-		writel(period_cycles, imx->mmio_base + MX3_PWMPR);
-
-		/*
-		 * Store the duty cycle for future reference in cases where
-		 * the MX3_PWMSAR register can't be read (i.e. when the PWM
-		 * is disabled).
-		 */
-		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 | MX3_PWMCR_EN;
-
-		if (state->polarity == PWM_POLARITY_INVERSED)
-			cr |= FIELD_PREP(MX3_PWMCR_POUTC,
-					MX3_PWMCR_POUTC_INVERTED);
-
-		writel(cr, imx->mmio_base + MX3_PWMCR);
-	} else if (cstate.enabled) {
-		writel(0, imx->mmio_base + MX3_PWMCR);
+	c = clk_get_rate(imx->clk_per);
+	c *= state->period;
 
-		pwm_imx27_clk_disable_unprepare(chip);
+	do_div(c, 1000000000);
+	period_cycles = c;
+
+	prescale = period_cycles / 0x10000 + 1;
+
+	period_cycles /= prescale;
+	c = (unsigned long long)period_cycles * state->duty_cycle;
+	do_div(c, state->period);
+	duty_cycles = c;
+
+	/*
+	 * according to imx pwm RM, the real period value should be PERIOD
+	 * value in PWMPR plus 2.
+	 */
+	if (period_cycles > 2)
+		period_cycles -= 2;
+	else
+		period_cycles = 0;
+
+	/*
+	 * 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) {
+		pwm_imx27_wait_fifo_slot(chip, pwm);
+	} else {
+		ret = pwm_imx27_clk_prepare_enable(chip);
+		if (ret)
+			return ret;
+
+		pwm_imx27_sw_reset(chip);
 	}
 
+	writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
+	writel(period_cycles, imx->mmio_base + MX3_PWMPR);
+
+	/*
+	 * Store the duty cycle for future reference in cases where the
+	 * MX3_PWMSAR register can't be read (i.e. when the PWM is disabled).
+	 */
+	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;
+
+	if (state->polarity == PWM_POLARITY_INVERSED)
+		cr |= FIELD_PREP(MX3_PWMCR_POUTC,
+				MX3_PWMCR_POUTC_INVERTED);
+
+	if (state->enabled)
+		cr |= MX3_PWMCR_EN;
+
+	writel(cr, imx->mmio_base + MX3_PWMCR);
+
+	if (!state->enabled && cstate.enabled)
+		pwm_imx27_clk_disable_unprepare(chip);
+
 	return 0;
 }