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

[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

[RFC PATCH 1/4] pwm: sun4i: Remove redundant needs_delay

[Pascal Roeleven]

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

Signed-off-by: Pascal Roeleven <dev@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

[RFC PATCH 2/4] pwm: sun4i: Disable pwm before turning off clock gate

[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 <dev@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

[RFC PATCH 3/4] pwm: sun4i: Move delay to function

[Pascal Roeleven]

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

Signed-off-by: Pascal Roeleven <dev@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);
 
 	spin_lock(&sun4i_pwm->ctrl_lock);
 	ctrl = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
-- 
2.20.1

[RFC PATCH 4/4] pwm: sun4i: Delay after writing the 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 <dev@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

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

[Emil Lenngren]

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

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

[Pascal Roeleven]

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.

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

[Emil Lenngren]

Hi,

Den tis 17 mars 2020 19:16Pascal Roeleven <dev@pascalroeleven.nl> skrev:
>
> 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?

Sure! I was thinking of sending a patch last year but never got the time :/
The devices I have tested on are Allwinner GR8 (A13) and V3s.

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

/Emil

Re: [RFC PATCH 1/4] pwm: sun4i: Remove redundant needs_delay

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 531 bytes --]

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 <dev@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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH 2/4] pwm: sun4i: Disable pwm before turning off clock gate

[Chen-Yu Tsai]

On Wed, Mar 18, 2020 at 12:00 AM Pascal Roeleven <dev@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 <dev@pascalroeleven.nl>

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

Re: [linux-sunxi] [RFC PATCH 3/4] pwm: sun4i: Move delay to function

[Chen-Yu Tsai]

On Wed, Mar 18, 2020 at 12:00 AM Pascal Roeleven <dev@pascalroeleven.nl> wrote:
>
> Move the delay to a function so we can reuse it.
>
> Signed-off-by: Pascal Roeleven <dev@pascalroeleven.nl>

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

Re: [RFC PATCH 4/4] pwm: sun4i: Delay after writing the period

[Chen-Yu Tsai]

On Wed, Mar 18, 2020 at 12:00 AM Pascal Roeleven <dev@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 <dev@pascalroeleven.nl>

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

Re: [linux-sunxi] [RFC PATCH 4/4] pwm: sun4i: Delay after writing the period

[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 <dev@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

>  	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);
> 

Re: [linux-sunxi] [RFC PATCH 4/4] pwm: sun4i: Delay after writing the period

[Pascal Roeleven]

On 2020-04-22 05:43, Samuel Holland wrote:
> 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 <dev@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
> 

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