Re: pwm: atmel: PWM may not properly disable

[Guillermo Rodriguez Garcia]

2016-05-12 14:14 GMT+02:00 Thierry Reding <thierry.reding@xxxxxxxxx>:
> 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@xxxxxxxxx