pwm: atmel: PWM may not properly disable

pwm: atmel: PWM may not properly disable

[Guillermo Rodriguez Garcia]

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)
-- 
Guillermo Rodriguez Garcia
guille.rodriguez@gmail.com

Re: pwm: atmel: PWM may not properly disable

[Thierry Reding]

[-- 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 --]

Re: pwm: atmel: PWM may not properly disable

[Guillermo Rodriguez Garcia]

Hello,

2016-05-11 15:39 GMT+02:00 Thierry Reding <thierry.reding@gmail.com>:
> 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().

I am not sure yet but I believe disabling does not really need to wait for the
current period to finish (at least the datasheets do not mention this anywhere).
I think that the after writing to PWM_DIS, the actual disable operation is
initiated immediately in the PWM subsystem, but is executed asynchronously
and requires the pwm_clk to complete. If this assumption is correct, perhaps
it is enough to do one single read from PWM_SR so that the disable operation
has had the chance to propagate. This is again assuming that all operations
are executed sequentially within the PWM subsystem.

If the above is correct, then we would not need a loop at all.

Let me do some checks on this and I'll come back with my findings.

>
> 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.

Perfect, will do.

Thank you,

Guillermo Rodriguez Garcia
guille.rodriguez@gmail.com

Re: pwm: atmel: PWM may not properly disable

[Guillermo Rodriguez Garcia]

2016-05-11 16:31 GMT+02:00 Guillermo Rodriguez Garcia
<guille.rodriguez@gmail.com>:
> Hello,
>
> 2016-05-11 15:39 GMT+02:00 Thierry Reding <thierry.reding@gmail.com>:
>> 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().
>
> I am not sure yet but I believe disabling does not really need to wait for the
> current period to finish (at least the datasheets do not mention this anywhere).
> I think that the after writing to PWM_DIS, the actual disable operation is
> initiated immediately in the PWM subsystem, but is executed asynchronously
> and requires the pwm_clk to complete. If this assumption is correct, perhaps
> it is enough to do one single read from PWM_SR so that the disable operation
> has had the chance to propagate. This is again assuming that all operations
> are executed sequentially within the PWM subsystem.
>
> If the above is correct, then we would not need a loop at all.

I was wrong. The required delay indeed seems to depend on the current PWM
frequency, suggesting that indeed disabling does not take effect until
the current
period is finished.

I will prepare a patch using usleep_range instead of cpu_relax.

Guillermo Rodriguez Garcia
guille.rodriguez@gmail.com

Re: pwm: atmel: PWM may not properly disable

[Guillermo Rodriguez Garcia]

Hello,

[...]
>>> 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().
>>
>> I am not sure yet but I believe disabling does not really need to wait for the
>> current period to finish (at least the datasheets do not mention this anywhere).
>> I think that the after writing to PWM_DIS, the actual disable operation is
>> initiated immediately in the PWM subsystem, but is executed asynchronously
>> and requires the pwm_clk to complete. If this assumption is correct, perhaps
>> it is enough to do one single read from PWM_SR so that the disable operation
>> has had the chance to propagate. This is again assuming that all operations
>> are executed sequentially within the PWM subsystem.
>>
>> If the above is correct, then we would not need a loop at all.
>
> I was wrong. The required delay indeed seems to depend on the current PWM
> frequency, suggesting that indeed disabling does not take effect until
> the current
> period is finished.
>
> I will prepare a patch using usleep_range instead of cpu_relax.

I have found a problem while preparing this. If I use usleep_range I
keep running
into "BUG: scheduling while atomic". This is because I am using the PWM to
drive a buzzer with pwm-beeper, and pwm-beeper currently crashes if the PWM
driver sleeps. Apparently this patch is needed:

https://lkml.org/lkml/2016/2/22/757

However this has not been merged yet.

How should I proceed ?

Guillermo Rodriguez Garcia
guille.rodriguez@gmail.com

Re: pwm: atmel: PWM may not properly disable

[Thierry Reding]

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

On Thu, May 12, 2016 at 01:49:12PM +0200, Guillermo Rodriguez Garcia wrote:
> Hello,
> 
> [...]
> >>> 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().
> >>
> >> I am not sure yet but I believe disabling does not really need to wait for the
> >> current period to finish (at least the datasheets do not mention this anywhere).
> >> I think that the after writing to PWM_DIS, the actual disable operation is
> >> initiated immediately in the PWM subsystem, but is executed asynchronously
> >> and requires the pwm_clk to complete. If this assumption is correct, perhaps
> >> it is enough to do one single read from PWM_SR so that the disable operation
> >> has had the chance to propagate. This is again assuming that all operations
> >> are executed sequentially within the PWM subsystem.
> >>
> >> If the above is correct, then we would not need a loop at all.
> >
> > I was wrong. The required delay indeed seems to depend on the current PWM
> > frequency, suggesting that indeed disabling does not take effect until
> > the current
> > period is finished.
> >
> > I will prepare a patch using usleep_range instead of cpu_relax.
> 
> I have found a problem while preparing this. If I use usleep_range I
> keep running
> into "BUG: scheduling while atomic". This is because I am using the PWM to
> drive a buzzer with pwm-beeper, and pwm-beeper currently crashes if the PWM
> driver sleeps. Apparently this patch is needed:
> 
> https://lkml.org/lkml/2016/2/22/757
> 
> However this has not been merged yet.
> 
> How should I proceed ?

The PWM API really shouldn't be used within atomic contexts. There was a
change recently that marked all of the PWM devices as "might sleep". The
reason for the change was that we introduced a mutex in pwm_enable() and
hence every user would have to deal with this eventually. That mutex has
since been removed again, but the fact remains that users shouldn't
assume that a PWM can be used in atomic context, because the PWM chip
could equally well be behind a slow bus such as I2C and hence sleep for
every register access.

So the correct thing to do would be to follow what leds-pwm did and
implement a workqueue. Also might as well make it the only code path as
Dmitry suggested in the linked thread, I don't see any point in any kind
of fast path here.

Thierry

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

Re: pwm: atmel: PWM may not properly disable

[Guillermo Rodriguez Garcia]

2016-05-12 14:14 GMT+02:00 Thierry Reding <thierry.reding@gmail.com>:
> On Thu, May 12, 2016 at 01:49:12PM +0200, Guillermo Rodriguez Garcia wrote:
>> Hello,
>>
>> [...]
>> >>> 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().
>> >>
>> >> I am not sure yet but I believe disabling does not really need to wait for the
>> >> current period to finish (at least the datasheets do not mention this anywhere).
>> >> I think that the after writing to PWM_DIS, the actual disable operation is
>> >> initiated immediately in the PWM subsystem, but is executed asynchronously
>> >> and requires the pwm_clk to complete. If this assumption is correct, perhaps
>> >> it is enough to do one single read from PWM_SR so that the disable operation
>> >> has had the chance to propagate. This is again assuming that all operations
>> >> are executed sequentially within the PWM subsystem.
>> >>
>> >> If the above is correct, then we would not need a loop at all.
>> >
>> > I was wrong. The required delay indeed seems to depend on the current PWM
>> > frequency, suggesting that indeed disabling does not take effect until
>> > the current
>> > period is finished.
>> >
>> > I will prepare a patch using usleep_range instead of cpu_relax.
>>
>> I have found a problem while preparing this. If I use usleep_range I
>> keep running
>> into "BUG: scheduling while atomic". This is because I am using the PWM to
>> drive a buzzer with pwm-beeper, and pwm-beeper currently crashes if the PWM
>> driver sleeps. Apparently this patch is needed:
>>
>> https://lkml.org/lkml/2016/2/22/757
>>
>> However this has not been merged yet.
>>
>> How should I proceed ?
>
> The PWM API really shouldn't be used within atomic contexts. There was a
> change recently that marked all of the PWM devices as "might sleep". The
> reason for the change was that we introduced a mutex in pwm_enable() and
> hence every user would have to deal with this eventually. That mutex has
> since been removed again, but the fact remains that users shouldn't
> assume that a PWM can be used in atomic context, because the PWM chip
> could equally well be behind a slow bus such as I2C and hence sleep for
> every register access.
>
> So the correct thing to do would be to follow what leds-pwm did and
> implement a workqueue. Also might as well make it the only code path as
> Dmitry suggested in the linked thread, I don't see any point in any kind
> of fast path here.

I understand. So I assume this pwm-beeper patch will be merged sooner
or later.

The reason why I was asking is that if I patch the Atmel PWM driver in
order to use usleep_range in pwm_disable, then pwm-beeper will not work
anymore on Atmel platforms until the pwm-beeper patch is merged.
I was not sure about the status of the pwm-beeper patch (the last message
in the linked thread is from March 30), hence my question.

I'll go ahead with the Atmel PWM patch then.

Thank you,

Guillermo Rodriguez Garcia
guille.rodriguez@gmail.com

Re: pwm: atmel: PWM may not properly disable

[Guillermo Rodriguez Garcia]

Hello,

2016-05-12 15:24 GMT+02:00 Guillermo Rodriguez Garcia
<guille.rodriguez@gmail.com>:
> 2016-05-12 14:14 GMT+02:00 Thierry Reding <thierry.reding@gmail.com>:
>> On Thu, May 12, 2016 at 01:49:12PM +0200, Guillermo Rodriguez Garcia wrote:
>>> Hello,
>>>
>>> [...]
>>> >>> 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().
>>> >>
>>> >> I am not sure yet but I believe disabling does not really need to wait for the
>>> >> current period to finish (at least the datasheets do not mention this anywhere).
>>> >> I think that the after writing to PWM_DIS, the actual disable operation is
>>> >> initiated immediately in the PWM subsystem, but is executed asynchronously
>>> >> and requires the pwm_clk to complete. If this assumption is correct, perhaps
>>> >> it is enough to do one single read from PWM_SR so that the disable operation
>>> >> has had the chance to propagate. This is again assuming that all operations
>>> >> are executed sequentially within the PWM subsystem.
>>> >>
>>> >> If the above is correct, then we would not need a loop at all.
>>> >
>>> > I was wrong. The required delay indeed seems to depend on the current PWM
>>> > frequency, suggesting that indeed disabling does not take effect until
>>> > the current
>>> > period is finished.
>>> >
>>> > I will prepare a patch using usleep_range instead of cpu_relax.
>>>
>>> I have found a problem while preparing this. If I use usleep_range I
>>> keep running
>>> into "BUG: scheduling while atomic". This is because I am using the PWM to
>>> drive a buzzer with pwm-beeper, and pwm-beeper currently crashes if the PWM
>>> driver sleeps. Apparently this patch is needed:
>>>
>>> https://lkml.org/lkml/2016/2/22/757
>>>
>>> However this has not been merged yet.
>>>
>>> How should I proceed ?
>>
>> The PWM API really shouldn't be used within atomic contexts. There was a
>> change recently that marked all of the PWM devices as "might sleep". The
>> reason for the change was that we introduced a mutex in pwm_enable() and
>> hence every user would have to deal with this eventually. That mutex has
>> since been removed again, but the fact remains that users shouldn't
>> assume that a PWM can be used in atomic context, because the PWM chip
>> could equally well be behind a slow bus such as I2C and hence sleep for
>> every register access.
>>
>> So the correct thing to do would be to follow what leds-pwm did and
>> implement a workqueue. Also might as well make it the only code path as
>> Dmitry suggested in the linked thread, I don't see any point in any kind
>> of fast path here.
>
> I understand. So I assume this pwm-beeper patch will be merged sooner
> or later.
>
> The reason why I was asking is that if I patch the Atmel PWM driver in
> order to use usleep_range in pwm_disable, then pwm-beeper will not work
> anymore on Atmel platforms until the pwm-beeper patch is merged.
> I was not sure about the status of the pwm-beeper patch (the last message
> in the linked thread is from March 30), hence my question.
>
> I'll go ahead with the Atmel PWM patch then.

I have just submitted a patch.

By the way, the following article in Atmel's KB indicates that writing
to PWM_DIS
is not enough for the channel to be effectively disabled -- the PWM_SR register
must be polled until the corresponding bit goes to 0.

It does not explicitly mention that the actual disabling happens at
the end of the
current period, although my tests seem to suggest that this is indeed the case.

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

As mentioned in my previous mail, this patch fixes the disabling of PWM channels
in the Atmel PWM driver, but it immediately exposes the problem in pwm-beeper
when running on Atmel platforms (https://lkml.org/lkml/2016/2/22/757).

Best,

Guillermo Rodriguez Garcia
guille.rodriguez@gmail.com