Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1400000

Re: pwm: atmel: PWM may not properly disable

Show all headers | View raw

[Multipart message — attachments visible in raw view] - view raw

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

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

pwm: atmel: PWM may not properly disable Guillermo Rodriguez Garcia <guille.rodriguez@gmail.com> - 2016-05-11 10:50 +0200
  Re: pwm: atmel: PWM may not properly disable Thierry Reding <thierry.reding@gmail.com> - 2016-05-11 15:40 +0200
    Re: pwm: atmel: PWM may not properly disable Guillermo Rodriguez Garcia <guille.rodriguez@gmail.com> - 2016-05-11 16:40 +0200
      Re: pwm: atmel: PWM may not properly disable Guillermo Rodriguez Garcia <guille.rodriguez@gmail.com> - 2016-05-12 12:20 +0200
        Re: pwm: atmel: PWM may not properly disable Guillermo Rodriguez Garcia <guille.rodriguez@gmail.com> - 2016-05-12 13:50 +0200
          Re: pwm: atmel: PWM may not properly disable Thierry Reding <thierry.reding@gmail.com> - 2016-05-12 14:20 +0200
            Re: pwm: atmel: PWM may not properly disable Guillermo Rodriguez Garcia <guille.rodriguez@gmail.com> - 2016-05-12 15:30 +0200
              Re: pwm: atmel: PWM may not properly disable Guillermo Rodriguez Garcia <guille.rodriguez@gmail.com> - 2016-05-13 13:20 +0200

csiph-web