Re: pwm: atmel: PWM may not properly disable

[Thierry Reding]

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: signature.asc
Description: PGP signature