[3/4] pwm: imx27: Cache duty cycle register value
diff mbox series

Message ID 20191021105739.1357629-3-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 hardware register containing the duty cycle value cannot be accessed
when the PWM is disabled. This causes the ->get_state() callback to read
back a duty cycle value of 0, which can confuse consumer drivers.

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

Comments

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

On 21. 10. 19 12:57, Thierry Reding wrote:
> The hardware register containing the duty cycle value cannot be accessed
> when the PWM is disabled. This causes the ->get_state() callback to read
> back a duty cycle value of 0, which can confuse consumer drivers.

Me and Adam Ford already tested the patches [3/4] and [4/4] and gave ours
Tested-by tags in the previous thread but do not see those here.
I re-tested these again and have no issues.

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

Thank you Thierry,
Michal


> Signed-off-by: Thierry Reding <thierry.reding@gmail.com>
> ---
>   drivers/pwm/pwm-imx27.c | 31 ++++++++++++++++++++++++-------
>   1 file changed, 24 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
> index ae11d8577f18..4113d5cd4c62 100644
> --- a/drivers/pwm/pwm-imx27.c
> +++ b/drivers/pwm/pwm-imx27.c
> @@ -85,6 +85,13 @@ struct pwm_imx27_chip {
>   	struct clk	*clk_per;
>   	void __iomem	*mmio_base;
>   	struct pwm_chip	chip;
> +
> +	/*
> +	 * The driver cannot read the current duty cycle from the hardware if
> +	 * the hardware is disabled. Cache the last programmed duty cycle
> +	 * value to return in that case.
> +	 */
> +	unsigned int duty_cycle;
>   };
>   
>   #define to_pwm_imx27_chip(chip)	container_of(chip, struct pwm_imx27_chip, chip)
> @@ -155,14 +162,17 @@ static void pwm_imx27_get_state(struct pwm_chip *chip,
>   	tmp = NSEC_PER_SEC * (u64)(period + 2);
>   	state->period = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk);
>   
> -	/* PWMSAR can be read only if PWM is enabled */
> -	if (state->enabled) {
> +	/*
> +	 * PWMSAR can be read only if PWM is enabled. If the PWM is disabled,
> +	 * use the cached value.
> +	 */
> +	if (state->enabled)
>   		val = readl(imx->mmio_base + MX3_PWMSAR);
> -		tmp = NSEC_PER_SEC * (u64)(val);
> -		state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk);
> -	} else {
> -		state->duty_cycle = 0;
> -	}
> +	else
> +		val = imx->duty_cycle;
> +
> +	tmp = NSEC_PER_SEC * (u64)(val);
> +	state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk);
>   
>   	if (!state->enabled)
>   		pwm_imx27_clk_disable_unprepare(chip);
> @@ -261,6 +271,13 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>   		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) |
>
Adam Ford Oct. 21, 2019, 2:21 p.m. UTC | #2
On Mon, Oct 21, 2019 at 8:46 AM Michal Vokáč <michal.vokac@ysoft.com> wrote:
>
> +Adam
>
> On 21. 10. 19 12:57, Thierry Reding wrote:
> > The hardware register containing the duty cycle value cannot be accessed
> > when the PWM is disabled. This causes the ->get_state() callback to read
> > back a duty cycle value of 0, which can confuse consumer drivers.
>
> Me and Adam Ford already tested the patches [3/4] and [4/4] and gave ours
> Tested-by tags in the previous thread but do not see those here.
> I re-tested these again and have no issues.
>
> Tested-by: Michal Vokáč <michal.vokac@ysoft.com>

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

>
> Thank you Thierry,
> Michal
>
>
> > Signed-off-by: Thierry Reding <thierry.reding@gmail.com>
> > ---
> >   drivers/pwm/pwm-imx27.c | 31 ++++++++++++++++++++++++-------
> >   1 file changed, 24 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
> > index ae11d8577f18..4113d5cd4c62 100644
> > --- a/drivers/pwm/pwm-imx27.c
> > +++ b/drivers/pwm/pwm-imx27.c
> > @@ -85,6 +85,13 @@ struct pwm_imx27_chip {
> >       struct clk      *clk_per;
> >       void __iomem    *mmio_base;
> >       struct pwm_chip chip;
> > +
> > +     /*
> > +      * The driver cannot read the current duty cycle from the hardware if
> > +      * the hardware is disabled. Cache the last programmed duty cycle
> > +      * value to return in that case.
> > +      */
> > +     unsigned int duty_cycle;
> >   };
> >
> >   #define to_pwm_imx27_chip(chip)     container_of(chip, struct pwm_imx27_chip, chip)
> > @@ -155,14 +162,17 @@ static void pwm_imx27_get_state(struct pwm_chip *chip,
> >       tmp = NSEC_PER_SEC * (u64)(period + 2);
> >       state->period = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk);
> >
> > -     /* PWMSAR can be read only if PWM is enabled */
> > -     if (state->enabled) {
> > +     /*
> > +      * PWMSAR can be read only if PWM is enabled. If the PWM is disabled,
> > +      * use the cached value.
> > +      */
> > +     if (state->enabled)
> >               val = readl(imx->mmio_base + MX3_PWMSAR);
> > -             tmp = NSEC_PER_SEC * (u64)(val);
> > -             state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk);
> > -     } else {
> > -             state->duty_cycle = 0;
> > -     }
> > +     else
> > +             val = imx->duty_cycle;
> > +
> > +     tmp = NSEC_PER_SEC * (u64)(val);
> > +     state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk);
> >
> >       if (!state->enabled)
> >               pwm_imx27_clk_disable_unprepare(chip);
> > @@ -261,6 +271,13 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> >               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) |
> >
>

Patch
diff mbox series

diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
index ae11d8577f18..4113d5cd4c62 100644
--- a/drivers/pwm/pwm-imx27.c
+++ b/drivers/pwm/pwm-imx27.c
@@ -85,6 +85,13 @@  struct pwm_imx27_chip {
 	struct clk	*clk_per;
 	void __iomem	*mmio_base;
 	struct pwm_chip	chip;
+
+	/*
+	 * The driver cannot read the current duty cycle from the hardware if
+	 * the hardware is disabled. Cache the last programmed duty cycle
+	 * value to return in that case.
+	 */
+	unsigned int duty_cycle;
 };
 
 #define to_pwm_imx27_chip(chip)	container_of(chip, struct pwm_imx27_chip, chip)
@@ -155,14 +162,17 @@  static void pwm_imx27_get_state(struct pwm_chip *chip,
 	tmp = NSEC_PER_SEC * (u64)(period + 2);
 	state->period = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk);
 
-	/* PWMSAR can be read only if PWM is enabled */
-	if (state->enabled) {
+	/*
+	 * PWMSAR can be read only if PWM is enabled. If the PWM is disabled,
+	 * use the cached value.
+	 */
+	if (state->enabled)
 		val = readl(imx->mmio_base + MX3_PWMSAR);
-		tmp = NSEC_PER_SEC * (u64)(val);
-		state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk);
-	} else {
-		state->duty_cycle = 0;
-	}
+	else
+		val = imx->duty_cycle;
+
+	tmp = NSEC_PER_SEC * (u64)(val);
+	state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk);
 
 	if (!state->enabled)
 		pwm_imx27_clk_disable_unprepare(chip);
@@ -261,6 +271,13 @@  static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 		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) |