Re: [RFC PATCH 0/4] pwm: sun4i: Properly turn pwm off and fix stuck output state

[Pascal Roeleven <dev@pascalroeleven.nl>]

On 2020-03-17 17:45, Emil Lenngren wrote:
> Hi all,
> 
> Den tis 17 mars 2020 kl 17:00 skrev Pascal Roeleven 
> <dev@pascalroeleven.nl>:
>> 
>> Hi all,
>> 
>> For the last few days I've been debugging a lot to get pwm working 
>> again since
>> recent changes in 5.6-rc1 broke it for me.
>> 
>> Testing shows the pwm controller crashes (or the output gets stuck) 
>> when the
>> period register is written when the channel is disabled while the 
>> clock gate is
>> still on. Usually after multiple writes, but one write can also lead 
>> to
>> unpredictable behaviour. Patch 3 and 4 fix this.
>> 
>> Patch 2 contains a fix which wouldn't completely turn off the pwm if 
>> the
>> output is disabled. The clock gate needs to stay on for at least one 
>> more
>> period to ensure the output is properly disabled. This issue has been 
>> around
>> for a long time but has probably stayed unnoticed because if the 
>> duty_cycle is
>> also changed to 0, you can't tell the difference.
>> 
>> Patch 1 removes some leftovers which aren't needed anymore.
>> 
>> Obviously these patches work for my device, but I'd like to hear your 
>> opinion
>> if any of these changes make sense. After days, this one is a bit 
>> blurry for me.
>> 
>> Thanks to Uwe for some help with debugging.
>> 
>> Pascal.
>> 
>> Pascal Roeleven (4):
>>   pwm: sun4i: Remove redundant needs_delay
>>   pwm: sun4i: Disable pwm before turning off clock gate
>>   pwm: sun4i: Move delay to function
>>   pwm: sun4i: Delay after writing the period
>> 
>>  drivers/pwm/pwm-sun4i.c | 53 
>> ++++++++++++++++++++---------------------
>>  1 file changed, 26 insertions(+), 27 deletions(-)
>> 
>> --
>> 2.20.1
>> 
> 
> I also worked on sun4i-pwm some time ago, fixing a bunch of issues.
> One was that disabling the pwm sometimes didn't turn off the signal,
> because the gate and enable bit were modified in the same clock cycle.
> Another was that the current code used an unnecessary sleep of a whole
> period length (or more?) in case of an update to the period, which
> could be very time-consuming if it's a very long interval, like 2
> seconds.
> 
> Note that the behaviour is not unpredictable, if you know how it works 
> ;)
> I fiddled around a long time with devmem2, an oscilloscope and the
> prescaler set to max to figure out how works internally.
> 
> Please try my version I just posted at https://pastebin.com/GWrhWzPJ.
> It is based on this version from May 28, 2019:
> https://github.com/torvalds/linux/blob/f50a7f3d9225dd374455f28138f79ae3074a7a3d/drivers/pwm/pwm-sun4i.c.
> Sorry for not posting it inline, but GMail would break the formatting.
> It contains quite many comments about how it works internally. I also
> wrote a section at http://linux-sunxi.org/PWM_Controller, but it might
> be a bit old (two years), so please rather look at the code and the
> comments.
> 
> /Emil

Hi Emil,

Thank you very much, this is helpful. Ah it was your note on the wiki. 
That is indeed where I took the idea of keeping the gate on and 
disabling the panel from. As a scope is still on my wishlist, the rest 
was just trial-and-error. Judging from your code, there are more edge 
cases which might occur. I will test your code and try to integrate it. 
If it's okay with you, I can post it on your behalf?

If you ask me, it's really unfortunate Allwinner didn't provide a timing 
diagram for such a picky controller.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel