Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20200317155906.31288-1-dev@pascalroeleven.nl>
In-Reply-To: <20200317155906.31288-1-dev@pascalroeleven.nl>
From:   Emil Lenngren <emil.lenngren@gmail.com>
Date:   Tue, 17 Mar 2020 17:45:58 +0100
Message-ID: <CAO1O6sccq7c_S8ZMsChBKcVcCn-DDv6awZzNr2BEnh8TH6ZxGg@mail.gmail.com>
Subject: Re: [RFC PATCH 0/4] pwm: sun4i: Properly turn pwm off and fix stuck
 output state
To:     Pascal Roeleven <dev@pascalroeleven.nl>
Cc:     Thierry Reding <thierry.reding@gmail.com>,
        =?UTF-8?Q?Uwe_Kleine=2DK=C3=B6nig?= 
        <u.kleine-koenig@pengutronix.de>,
        Maxime Ripard <mripard@kernel.org>,
        Chen-Yu Tsai <wens@csie.org>,
        Philipp Zabel <p.zabel@pengutronix.de>,
        linux-pwm@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        LKML <linux-kernel@vger.kernel.org>, linux-sunxi@googlegroups.com
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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