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

[Pascal Roeleven]

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

[Pascal Roeleven]

'needs_delay' does now always evaluate to true, so remove all
occurrences.

Signed-off-by: Pascal Roeleven <d...@pascalroeleven.nl>
---
drivers/pwm/pwm-sun4i.c | 13 ++-----------
1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
index 3e3efa6c7..5c677c563 100644
--- a/drivers/pwm/pwm-sun4i.c
+++ b/drivers/pwm/pwm-sun4i.c
@@ -90,7 +90,6 @@ struct sun4i_pwm_chip {
spinlock_t ctrl_lock;
const struct sun4i_pwm_data *data;
unsigned long next_period[2];
- bool needs_delay[2];
};

static inline struct sun4i_pwm_chip *to_sun4i_pwm_chip(struct pwm_chip *chip)
@@ -287,7 +286,6 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
sun4i_pwm_writel(sun4i_pwm, val, PWM_CH_PRD(pwm->hwpwm));
sun4i_pwm->next_period[pwm->hwpwm] = jiffies +
usecs_to_jiffies(cstate.period / 1000 + 1);
- sun4i_pwm->needs_delay[pwm->hwpwm] = true;

if (state->polarity != PWM_POLARITY_NORMAL)
ctrl &= ~BIT_CH(PWM_ACT_STATE, pwm->hwpwm);
@@ -298,7 +296,7 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,

if (state->enabled) {
ctrl |= BIT_CH(PWM_EN, pwm->hwpwm);
- } else if (!sun4i_pwm->needs_delay[pwm->hwpwm]) {
+ } else {
ctrl &= ~BIT_CH(PWM_EN, pwm->hwpwm);
ctrl &= ~BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
}
@@ -310,15 +308,9 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
if (state->enabled)
return 0;

- if (!sun4i_pwm->needs_delay[pwm->hwpwm]) {
- clk_disable_unprepare(sun4i_pwm->clk);
- return 0;
- }
-
/* We need a full period to elapse before disabling the channel. */
now = jiffies;
- if (sun4i_pwm->needs_delay[pwm->hwpwm] &&
- time_before(now, sun4i_pwm->next_period[pwm->hwpwm])) {
+ if (time_before(now, sun4i_pwm->next_period[pwm->hwpwm])) {
delay_us = jiffies_to_usecs(sun4i_pwm->next_period[pwm->hwpwm] -
now);
if ((delay_us / 500) > MAX_UDELAY_MS)
@@ -326,7 +318,6 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
else
usleep_range(delay_us, delay_us * 2);
}
- sun4i_pwm->needs_delay[pwm->hwpwm] = false;

spin_lock(&sun4i_pwm->ctrl_lock);
ctrl = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
--
2.20.1

[Pascal Roeleven]

The clock gate must stay on when disabling to ensure proper turning off.
After one period it will still be disabled anyway.

Signed-off-by: Pascal Roeleven <d...@pascalroeleven.nl>
---

drivers/pwm/pwm-sun4i.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
index 5c677c563..56942036b 100644
--- a/drivers/pwm/pwm-sun4i.c
+++ b/drivers/pwm/pwm-sun4i.c
@@ -292,13 +292,12 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
else
ctrl |= BIT_CH(PWM_ACT_STATE, pwm->hwpwm);

- ctrl |= BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
-

if (state->enabled) {
ctrl |= BIT_CH(PWM_EN, pwm->hwpwm);

+ ctrl |= BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
} else {
+ /* Turn gate off after delay to ensure proper turning off */

ctrl &= ~BIT_CH(PWM_EN, pwm->hwpwm);

- ctrl &= ~BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
}

sun4i_pwm_writel(sun4i_pwm, ctrl, PWM_CTRL_REG);
--
2.20.1

[Pascal Roeleven]

Move the delay to a function so we can reuse it.

Signed-off-by: Pascal Roeleven <d...@pascalroeleven.nl>
---

drivers/pwm/pwm-sun4i.c | 32 ++++++++++++++++++--------------
1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
index 56942036b..a11d00f96 100644
--- a/drivers/pwm/pwm-sun4i.c
+++ b/drivers/pwm/pwm-sun4i.c
@@ -89,7 +89,6 @@ struct sun4i_pwm_chip {
void __iomem *base;

spinlock_t ctrl_lock;
const struct sun4i_pwm_data *data;

- unsigned long next_period[2];

};

static inline struct sun4i_pwm_chip *to_sun4i_pwm_chip(struct pwm_chip *chip)

@@ -228,6 +227,20 @@ static int sun4i_pwm_calculate(struct sun4i_pwm_chip *sun4i_pwm,
return 0;
}

+static void sun4i_pwm_wait(unsigned long next_period) {
+ unsigned int delay_us;
+ unsigned long now;
+
+ now = jiffies;
+ if (time_before(now, next_period)) {
+ delay_us = jiffies_to_usecs(next_period - now);
+ if ((delay_us / 500) > MAX_UDELAY_MS)
+ msleep(delay_us / 1000 + 1);
+ else
+ usleep_range(delay_us, delay_us * 2);
+ }
+}
+

static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,

const struct pwm_state *state)
{
@@ -235,8 +248,8 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
struct pwm_state cstate;
u32 ctrl, duty = 0, period = 0, val;
int ret;
- unsigned int delay_us, prescaler = 0;
- unsigned long now;
+ unsigned int prescaler = 0;
+ unsigned long next_period;
bool bypass;

pwm_get_state(pwm, &cstate);
@@ -284,8 +297,7 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,

val = (duty & PWM_DTY_MASK) | PWM_PRD(period);
sun4i_pwm_writel(sun4i_pwm, val, PWM_CH_PRD(pwm->hwpwm));
- sun4i_pwm->next_period[pwm->hwpwm] = jiffies +
- usecs_to_jiffies(cstate.period / 1000 + 1);
+ next_period = jiffies + usecs_to_jiffies(cstate.period / 1000 + 1);

if (state->polarity != PWM_POLARITY_NORMAL)
ctrl &= ~BIT_CH(PWM_ACT_STATE, pwm->hwpwm);

@@ -308,15 +320,7 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
return 0;

/* We need a full period to elapse before disabling the channel. */

- now = jiffies;
- if (time_before(now, sun4i_pwm->next_period[pwm->hwpwm])) {
- delay_us = jiffies_to_usecs(sun4i_pwm->next_period[pwm->hwpwm] -
- now);
- if ((delay_us / 500) > MAX_UDELAY_MS)
- msleep(delay_us / 1000 + 1);
- else
- usleep_range(delay_us, delay_us * 2);
- }
+ sun4i_pwm_wait(next_period);

[Pascal Roeleven]

When disabling, ensure the period write is complete before continuing.
This fixes an issue on some devices when the write isn't complete before
the panel is turned off but the clock gate is still on.

Signed-off-by: Pascal Roeleven <d...@pascalroeleven.nl>
---

drivers/pwm/pwm-sun4i.c | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
index a11d00f96..75250fd4c 100644
--- a/drivers/pwm/pwm-sun4i.c
+++ b/drivers/pwm/pwm-sun4i.c
@@ -299,6 +299,10 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
sun4i_pwm_writel(sun4i_pwm, val, PWM_CH_PRD(pwm->hwpwm));

next_period = jiffies + usecs_to_jiffies(cstate.period / 1000 + 1);

+ /* When disabling, make sure the period register is written first */
+ if (!state->enabled && cstate.enabled)
+ sun4i_pwm_wait(next_period);
+

if (state->polarity != PWM_POLARITY_NORMAL)
ctrl &= ~BIT_CH(PWM_ACT_STATE, pwm->hwpwm);

else
@@ -320,6 +324,7 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,

return 0;

/* We need a full period to elapse before disabling the channel. */

+ next_period = jiffies + usecs_to_jiffies(cstate.period / 1000 + 1);

sun4i_pwm_wait(next_period);

spin_lock(&sun4i_pwm->ctrl_lock);
--
2.20.1

[Pascal Roeleven]

On 2020-03-17 17:45, Emil Lenngren wrote:
> Hi all,
>
> Den tis 17 mars 2020 kl 17:00 skrev Pascal Roeleven
> <d...@pascalroeleven.nl>:

> 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.

[Thierry Reding]

On Tue, Mar 17, 2020 at 04:59:03PM +0100, Pascal Roeleven wrote:
> 'needs_delay' does now always evaluate to true, so remove all
> occurrences.
>
> Signed-off-by: Pascal Roeleven <d...@pascalroeleven.nl>
> ---
> drivers/pwm/pwm-sun4i.c | 13 ++-----------
> 1 file changed, 2 insertions(+), 11 deletions(-)

I've applied this one since it's obviously correct. I'll hold off on the
others until it can be more broadly tested. Hopefully Maxime or Chen-Yu
can help review the remainder of this series as well.

Thierry

[Chen-Yu Tsai]

On Wed, Mar 18, 2020 at 12:00 AM Pascal Roeleven <d...@pascalroeleven.nl> wrote:
>
> The clock gate must stay on when disabling to ensure proper turning off.
> After one period it will still be disabled anyway.
>
> Signed-off-by: Pascal Roeleven <d...@pascalroeleven.nl>

Reviewed-by: Chen-Yu Tsai <we...@csie.org>

[Chen-Yu Tsai]

On Wed, Mar 18, 2020 at 12:00 AM Pascal Roeleven <d...@pascalroeleven.nl> wrote:
>

> Move the delay to a function so we can reuse it.
>
> Signed-off-by: Pascal Roeleven <d...@pascalroeleven.nl>

Reviewed-by: Chen-Yu Tsai <we...@csie.org>

[Chen-Yu Tsai]

On Wed, Mar 18, 2020 at 12:00 AM Pascal Roeleven <d...@pascalroeleven.nl> wrote:
>

> When disabling, ensure the period write is complete before continuing.
> This fixes an issue on some devices when the write isn't complete before
> the panel is turned off but the clock gate is still on.
>
> Signed-off-by: Pascal Roeleven <d...@pascalroeleven.nl>

Reviewed-by: Chen-Yu Tsai <we...@csie.org>

[Samuel Holland]

Hello Pascal,

On 3/17/20 10:59 AM, Pascal Roeleven wrote:
> When disabling, ensure the period write is complete before continuing.
> This fixes an issue on some devices when the write isn't complete before
> the panel is turned off but the clock gate is still on.
>
> Signed-off-by: Pascal Roeleven <d...@pascalroeleven.nl>
> ---
> drivers/pwm/pwm-sun4i.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> index a11d00f96..75250fd4c 100644
> --- a/drivers/pwm/pwm-sun4i.c
> +++ b/drivers/pwm/pwm-sun4i.c
> @@ -299,6 +299,10 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> sun4i_pwm_writel(sun4i_pwm, val, PWM_CH_PRD(pwm->hwpwm));
> next_period = jiffies + usecs_to_jiffies(cstate.period / 1000 + 1);
>
> + /* When disabling, make sure the period register is written first */
> + if (!state->enabled && cstate.enabled)
> + sun4i_pwm_wait(next_period);
> +

It is not visible from the context of this patch, but this call to
sun4i_pwm_wait() ends up calling msleep() inside a spinlock, which isn't
allowed. The spinlock should probably be converted to a mutex, considering that
sun4i_pwm_apply() already sleeps and takes mutexes.

Regards,
Samuel

[Pascal Roeleven]

Yes you're right. A different implementation of this patch series is
being worked on, in which I'll take this into account. Unfortunately I
have other things to work on at the moment, so it might take a while.

Regards,
Pascal