[PATCH] pwm: atmel: Fix disabling of PWM channels

[PATCH] pwm: atmel: Fix disabling of PWM channels

[Guillermo Rodriguez]

When disabling a PWM channel, the PWM clock was being stopped
immediately after writing to PWM_DIS. As a result, the disabling
of the PWM channel did not complete properly, and the PWM output
might be left at the wrong level.

Fix this by waiting for the channel to be effectively disabled
(by checking the PWM_SR register) before disabling the clock.

Signed-off-by: Guillermo Rodriguez <guille.rodriguez@gmail.com>
---
 drivers/pwm/pwm-atmel.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
index 0e4bd4e..a714434 100644
--- a/drivers/pwm/pwm-atmel.c
+++ b/drivers/pwm/pwm-atmel.c
@@ -271,6 +271,16 @@ static void atmel_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 	mutex_unlock(&atmel_pwm->isr_lock);
 	atmel_pwm_writel(atmel_pwm, PWM_DIS, 1 << pwm->hwpwm);
 
+	/*
+	 * Wait for the PWM channel disable operation to be effective before
+	 * stopping the clock.
+	 */
+	timeout = jiffies + 2 * HZ;
+	while ((atmel_pwm_readl(atmel_pwm, PWM_SR)& (1 << pwm->hwpwm)) &&
+	       time_before(jiffies, timeout)) {
+		usleep_range(10, 100);
+	}
+
 	clk_disable(atmel_pwm->clk);
 }
 
-- 
1.7.9.5

Re: [PATCH] pwm: atmel: Fix disabling of PWM channels

[Guillermo Rodriguez Garcia]

Hello,

Any comments on this patch?

Guillermo

2016-05-13 13:09 GMT+02:00 Guillermo Rodriguez <guille.rodriguez@gmail.com>:
> When disabling a PWM channel, the PWM clock was being stopped
> immediately after writing to PWM_DIS. As a result, the disabling
> of the PWM channel did not complete properly, and the PWM output
> might be left at the wrong level.
>
> Fix this by waiting for the channel to be effectively disabled
> (by checking the PWM_SR register) before disabling the clock.
>
> Signed-off-by: Guillermo Rodriguez <guille.rodriguez@gmail.com>
> ---
>  drivers/pwm/pwm-atmel.c |   10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
> index 0e4bd4e..a714434 100644
> --- a/drivers/pwm/pwm-atmel.c
> +++ b/drivers/pwm/pwm-atmel.c
> @@ -271,6 +271,16 @@ static void atmel_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>         mutex_unlock(&atmel_pwm->isr_lock);
>         atmel_pwm_writel(atmel_pwm, PWM_DIS, 1 << pwm->hwpwm);
>
> +       /*
> +        * Wait for the PWM channel disable operation to be effective before
> +        * stopping the clock.
> +        */
> +       timeout = jiffies + 2 * HZ;
> +       while ((atmel_pwm_readl(atmel_pwm, PWM_SR)& (1 << pwm->hwpwm)) &&
> +              time_before(jiffies, timeout)) {
> +               usleep_range(10, 100);
> +       }
> +
>         clk_disable(atmel_pwm->clk);
>  }
>
> --
> 1.7.9.5
>



-- 
Guillermo Rodriguez Garcia
guille.rodriguez@gmail.com

Re: pwm: atmel: Fix disabling of PWM channels

[Alexandre Belloni]

Hi,

On 13/05/2016 at 13:09:37 +0200, Guillermo Rodriguez wrote :
> When disabling a PWM channel, the PWM clock was being stopped
> immediately after writing to PWM_DIS. As a result, the disabling
> of the PWM channel did not complete properly, and the PWM output
> might be left at the wrong level.
> 
> Fix this by waiting for the channel to be effectively disabled
> (by checking the PWM_SR register) before disabling the clock.
> 
> Signed-off-by: Guillermo Rodriguez <guille.rodriguez@gmail.com>
> ---
>  drivers/pwm/pwm-atmel.c |   10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
> index 0e4bd4e..a714434 100644
> --- a/drivers/pwm/pwm-atmel.c
> +++ b/drivers/pwm/pwm-atmel.c
> @@ -271,6 +271,16 @@ static void atmel_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>  	mutex_unlock(&atmel_pwm->isr_lock);
>  	atmel_pwm_writel(atmel_pwm, PWM_DIS, 1 << pwm->hwpwm);
>  
> +	/*
> +	 * Wait for the PWM channel disable operation to be effective before
> +	 * stopping the clock.
> +	 */
> +	timeout = jiffies + 2 * HZ;
> +	while ((atmel_pwm_readl(atmel_pwm, PWM_SR)& (1 << pwm->hwpwm)) &&
> +	       time_before(jiffies, timeout)) {
> +		usleep_range(10, 100);
> +	}
> +

While this seems good, can you tell on which SoC you observed that? I'd
like to understand whether this is only on v1 or v2 or both.

Thanks!

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH] pwm: atmel: Fix disabling of PWM channels

[Guillermo Rodriguez Garcia]

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

Hi,

El viernes, 10 de junio de 2016, Alexandre Belloni <
alexandre.belloni@free-electrons.com> escribió:

> Hi,
>
> On 13/05/2016 at 13:09:37 +0200, Guillermo Rodriguez wrote :
> > When disabling a PWM channel, the PWM clock was being stopped
> > immediately after writing to PWM_DIS. As a result, the disabling
> > of the PWM channel did not complete properly, and the PWM output
> > might be left at the wrong level.
> >
> > Fix this by waiting for the channel to be effectively disabled
> > (by checking the PWM_SR register) before disabling the clock.
> >
> > Signed-off-by: Guillermo Rodriguez <guille.rodriguez@gmail.com
> <javascript:;>>
> > ---
> >  drivers/pwm/pwm-atmel.c |   10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
> > index 0e4bd4e..a714434 100644
> > --- a/drivers/pwm/pwm-atmel.c
> > +++ b/drivers/pwm/pwm-atmel.c
> > @@ -271,6 +271,16 @@ static void atmel_pwm_disable(struct pwm_chip
> *chip, struct pwm_device *pwm)
> >       mutex_unlock(&atmel_pwm->isr_lock);
> >       atmel_pwm_writel(atmel_pwm, PWM_DIS, 1 << pwm->hwpwm);
> >
> > +     /*
> > +      * Wait for the PWM channel disable operation to be effective
> before
> > +      * stopping the clock.
> > +      */
> > +     timeout = jiffies + 2 * HZ;
> > +     while ((atmel_pwm_readl(atmel_pwm, PWM_SR)& (1 << pwm->hwpwm)) &&
> > +            time_before(jiffies, timeout)) {
> > +             usleep_range(10, 100);
> > +     }
> > +
>
> While this seems good, can you tell on which SoC you observed that? I'd
> like to understand whether this is only on v1 or v2 or both.


I observed it on a sama5d3, however this article from atmel's KB would seem
to suggest that it is not specific to this SoC:

http://atmel.force.com/support/pkb_mobile#/articles/en_US/FAQ/Why-PWM-Channel-Polarity-Inversion

 Guillermo


-- 
Guillermo Rodriguez Garcia
guille.rodriguez@gmail.com

[-- Attachment #2: Type: text/html, Size: 2746 bytes --]

Re: pwm: atmel: Fix disabling of PWM channels

[Alexandre Belloni]

On 13/05/2016 at 13:09:37 +0200, Guillermo Rodriguez wrote :
> When disabling a PWM channel, the PWM clock was being stopped
> immediately after writing to PWM_DIS. As a result, the disabling
> of the PWM channel did not complete properly, and the PWM output
> might be left at the wrong level.
> 
> Fix this by waiting for the channel to be effectively disabled
> (by checking the PWM_SR register) before disabling the clock.
> 
> Signed-off-by: Guillermo Rodriguez <guille.rodriguez@gmail.com>
Acked-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>

> ---
>  drivers/pwm/pwm-atmel.c |   10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
> index 0e4bd4e..a714434 100644
> --- a/drivers/pwm/pwm-atmel.c
> +++ b/drivers/pwm/pwm-atmel.c
> @@ -271,6 +271,16 @@ static void atmel_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>  	mutex_unlock(&atmel_pwm->isr_lock);
>  	atmel_pwm_writel(atmel_pwm, PWM_DIS, 1 << pwm->hwpwm);
>  
> +	/*
> +	 * Wait for the PWM channel disable operation to be effective before
> +	 * stopping the clock.
> +	 */
> +	timeout = jiffies + 2 * HZ;
> +	while ((atmel_pwm_readl(atmel_pwm, PWM_SR)& (1 << pwm->hwpwm)) &&
> +	       time_before(jiffies, timeout)) {
> +		usleep_range(10, 100);
> +	}
> +
>  	clk_disable(atmel_pwm->clk);
>  }
>  

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: pwm: atmel: Fix disabling of PWM channels

[Guillermo Rodriguez Garcia]

(resending to MLs for the record -- previous message bounced)

El viernes, 10 de junio de 2016, Alexandre Belloni
<alexandre.belloni@free-electrons.com> escribió:
>
> Hi,
>
> On 13/05/2016 at 13:09:37 +0200, Guillermo Rodriguez wrote :
> > When disabling a PWM channel, the PWM clock was being stopped
> > immediately after writing to PWM_DIS. As a result, the disabling
> > of the PWM channel did not complete properly, and the PWM output
> > might be left at the wrong level.
> >
> > Fix this by waiting for the channel to be effectively disabled
> > (by checking the PWM_SR register) before disabling the clock.
> >
> > Signed-off-by: Guillermo Rodriguez <guille.rodriguez@gmail.com>
> > ---
> >  drivers/pwm/pwm-atmel.c |   10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
> > index 0e4bd4e..a714434 100644
> > --- a/drivers/pwm/pwm-atmel.c
> > +++ b/drivers/pwm/pwm-atmel.c
> > @@ -271,6 +271,16 @@ static void atmel_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> >       mutex_unlock(&atmel_pwm->isr_lock);
> >       atmel_pwm_writel(atmel_pwm, PWM_DIS, 1 << pwm->hwpwm);
> >
> > +     /*
> > +      * Wait for the PWM channel disable operation to be effective before
> > +      * stopping the clock.
> > +      */
> > +     timeout = jiffies + 2 * HZ;
> > +     while ((atmel_pwm_readl(atmel_pwm, PWM_SR)& (1 << pwm->hwpwm)) &&
> > +            time_before(jiffies, timeout)) {
> > +             usleep_range(10, 100);
> > +     }
> > +
>
> While this seems good, can you tell on which SoC you observed that? I'd
> like to understand whether this is only on v1 or v2 or both.

I observed it on a sama5d3, however this article from atmel's KB would
seem to suggest that it is not specific to this SoC:

http://atmel.force.com/support/pkb_mobile#/articles/en_US/FAQ/Why-PWM-Channel-Polarity-Inversion

Guillermo

Re: [PATCH] pwm: atmel: Fix disabling of PWM channels

[Thierry Reding]

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

On Fri, May 13, 2016 at 01:09:37PM +0200, Guillermo Rodriguez wrote:
> When disabling a PWM channel, the PWM clock was being stopped
> immediately after writing to PWM_DIS. As a result, the disabling
> of the PWM channel did not complete properly, and the PWM output
> might be left at the wrong level.
> 
> Fix this by waiting for the channel to be effectively disabled
> (by checking the PWM_SR register) before disabling the clock.
> 
> Signed-off-by: Guillermo Rodriguez <guille.rodriguez@gmail.com>
> ---
>  drivers/pwm/pwm-atmel.c |   10 ++++++++++
>  1 file changed, 10 insertions(+)

Applied, thanks.

Thierry

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