Re: pwm: atmel: PWM may not properly disable

[Thierry Reding <thierry.reding@gmail.com>]

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

On Wed, May 11, 2016 at 10:48:38AM +0200, Guillermo Rodriguez Garcia wrote:
> Hello all,
> 
> I am seeing a problem with the atmel-pwm driver where disabling a PWM
> channel sometimes leaves the output at the wrong level ("wrong" == not
> idle, as per the configured polarity). This causes problems when the
> PWM is used to drive a LED or certain types of buzzers.
> 
> The issue seems to be in the atmel_pwm_disable function [1], which
> disables the clock immediately after writing to the PWM_DIS register.
> As a result, the write does not seem to take effect.
> 
> I have verified that this is the cause of the problem this by waiting
> until the channel is effectively disabled (by checking the PWM_SR
> register) before disabling the clock. This fixes the issue and the PWM
> output now stays at the idle level after the channel is disabled.
> 
> For the above I used a modified version of a patch that was posted to
> the list some time ago [2]. Note that only atmel_pwm_disable needs to
> be patched, there seems to be no need to patch atmel_pwm_enable. The
> code is as follows:
> 
>      atmel_pwm_writel(atmel_pwm, PWM_DIS, 1 << pwm->hwpwm);
> +    while (atmel_pwm_readl(atmel_pwm, PWM_SR) & (1 << pwm->hwpwm)) {
> +        cpu_relax();
> +    }
> 
>      clk_disable(atmel_pwm->clk);
> 
> Is this acceptable? Should I submit an updated patch?
> 
>  [1]: http://lxr.free-electrons.com/source/drivers/pwm/pwm-atmel.c#L253
>  [2]: https://lkml.org/lkml/2014/10/1/605
> 
> (If possible, please CC me in any replies)

Okay, so the above makes a lot more sense than Robert's original patch.
The biggest issue that stuck out at the time was that the code kept
writing to the PWM_DIS register. Your example shows that PWM_SR is the
status register and your loop shows that it's enough to simply wait for
the PWM to become enabled (Robert's patch suggested that writes to
PWM_DIS might not make it through, and hence they had to be repeated).

As for atmel_pwm_enable() I'm tempted to request a similar change. The
reason is that the PWM API expects (though it isn't technically enforced
in any way) that the PWM output is enabled after the function exits. It
seems reasonable to me that disabling would take a while (the remainder
of the current period for example) whereas enabling might always be
immediate. As such it wouldn't be technically necessary to have the loop
so I'm not going to insist if everything indicates the above is indeed
how the hardware works.

One thing that I'd request is that instead of the cpu_relax() you use a
usleep_range() within the loop instead. I assume it can potentially take
a long time for the current period to finish, so busy looping isn't such
a great idea. You could possibly use the current period_ns to derive a
meaningful value to pass to usleep_range().

Otherwise it'd be great if you submitted this again. Also make sure to
use a proper commit message as I suggested in a response to Robert's
original patch.

Thanks,
Thierry

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