[PATCH v2 0/5] pwm: pxa: Fixes for enable/disable transitions

[PATCH v2 0/5] pwm: pxa: Fixes for enable/disable transitions

[Doug Brown]

I ran into a couple of problems while getting this driver working on the
PXA168. It wouldn't always activate properly when I turned it on, and it
wouldn't always deactivate properly when I turned it off. These patches
fix issues with the clock enable/disable transitions.

With these patches applied, the driver works flawlessly with my use
cases on the PXA168. I don't have any other PXAxxx devices to test with.

Changes since v1:

- Remove pxa_pwm_enable and pxa_pwm_disable.
- Reorganize pxa_pwm_apply based on Uwe's suggestions.
- Enable this driver with ARCH_MMP for the PXA168.
- Add delay after clock is disabled (and inhibit brief on/off blips).

After applying the first round of suggested fixes, I discovered (while
using it as a PWM backlight) that I was once again being affected by the
"too quick" on->off->on transitions, which resulted in silent failure.
Hopefully this isn't too crazy, but I fixed it by adding a delay to
allow the last period to finish after the clock is requested to be
disabled. When you turn the clock off, it stays latched on until the
period finishes (because PWMCR_SD is not set). If you try to turn it
back on during this time, the request is ignored and it shuts off
anyway. Waiting a full period before attempting to start the clock again
avoids the problem.

I also inhibited register setup if PWM is already disabled and staying
disabled in an attempt to avoid unnecessary brief on/off "blips".

Doug Brown (5):
  pwm: pxa: Remove pxa_pwm_enable/disable
  pwm: pxa: Set duty cycle to 0 when disabling PWM
  pwm: pxa: Remove clk enable/disable from pxa_pwm_config
  pwm: pxa: Wait for final PWM period to finish
  pwm: pxa: Enable for MMP platform

 drivers/pwm/Kconfig   |  2 +-
 drivers/pwm/pwm-pxa.c | 64 ++++++++++++++++++++++---------------------
 2 files changed, 34 insertions(+), 32 deletions(-)

-- 
2.34.1

[PATCH v2 1/5] pwm: pxa: Remove pxa_pwm_enable/disable

[Doug Brown]

These functions are only acting as wrappers for clk_prepare_enable and
clk_disable_unprepare now, so remove them to simplify the driver.

Suggested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Signed-off-by: Doug Brown <doug@schmorgal.com>
---
 drivers/pwm/pwm-pxa.c | 19 +++----------------
 1 file changed, 3 insertions(+), 16 deletions(-)

diff --git a/drivers/pwm/pwm-pxa.c b/drivers/pwm/pwm-pxa.c
index 0bcaa58c6a91..0ac052652c62 100644
--- a/drivers/pwm/pwm-pxa.c
+++ b/drivers/pwm/pwm-pxa.c
@@ -101,23 +101,10 @@ static int pxa_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	return 0;
 }
 
-static int pxa_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
-{
-	struct pxa_pwm_chip *pc = to_pxa_pwm_chip(chip);
-
-	return clk_prepare_enable(pc->clk);
-}
-
-static void pxa_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
-{
-	struct pxa_pwm_chip *pc = to_pxa_pwm_chip(chip);
-
-	clk_disable_unprepare(pc->clk);
-}
-
 static int pxa_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 			 const struct pwm_state *state)
 {
+	struct pxa_pwm_chip *pc = to_pxa_pwm_chip(chip);
 	int err;
 
 	if (state->polarity != PWM_POLARITY_NORMAL)
@@ -125,7 +112,7 @@ static int pxa_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	if (!state->enabled) {
 		if (pwm->state.enabled)
-			pxa_pwm_disable(chip, pwm);
+			clk_disable_unprepare(pc->clk);
 
 		return 0;
 	}
@@ -135,7 +122,7 @@ static int pxa_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 		return err;
 
 	if (!pwm->state.enabled)
-		return pxa_pwm_enable(chip, pwm);
+		return clk_prepare_enable(pc->clk);
 
 	return 0;
 }
-- 
2.34.1

[PATCH v2 2/5] pwm: pxa: Set duty cycle to 0 when disabling PWM

[Doug Brown]

When disabling PWM, the duty cycle needs to be set to 0. This prevents
the previous duty cycle from showing up momentarily when the clock is
re-enabled next time, and also prevents the output pin from being stuck
high when the clock is disabled.

Because the clock has to be running in order to configure the duty
cycle, unconditionally enable it early in pxa_pwm_apply and account for
the correct enable count at the end.

Suggested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Signed-off-by: Doug Brown <doug@schmorgal.com>
---
 drivers/pwm/pwm-pxa.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/drivers/pwm/pwm-pxa.c b/drivers/pwm/pwm-pxa.c
index 0ac052652c62..9ee9b41d62b8 100644
--- a/drivers/pwm/pwm-pxa.c
+++ b/drivers/pwm/pwm-pxa.c
@@ -105,24 +105,31 @@ static int pxa_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 			 const struct pwm_state *state)
 {
 	struct pxa_pwm_chip *pc = to_pxa_pwm_chip(chip);
+	u64 duty_cycle;
 	int err;
 
 	if (state->polarity != PWM_POLARITY_NORMAL)
 		return -EINVAL;
 
-	if (!state->enabled) {
-		if (pwm->state.enabled)
-			clk_disable_unprepare(pc->clk);
+	err = clk_prepare_enable(pc->clk);
+	if (err)
+		return err;
 
-		return 0;
-	}
+	duty_cycle = state->enabled ? state->duty_cycle : 0;
 
-	err = pxa_pwm_config(chip, pwm, state->duty_cycle, state->period);
-	if (err)
+	err = pxa_pwm_config(chip, pwm, duty_cycle, state->period);
+	if (err) {
+		clk_disable_unprepare(pc->clk);
 		return err;
+	}
+
+	if (state->enabled && !pwm->state.enabled)
+		return 0;
+
+	clk_disable_unprepare(pc->clk);
 
-	if (!pwm->state.enabled)
-		return clk_prepare_enable(pc->clk);
+	if (!state->enabled && pwm->state.enabled)
+		clk_disable_unprepare(pc->clk);
 
 	return 0;
 }
-- 
2.34.1

[PATCH v2 3/5] pwm: pxa: Remove clk enable/disable from pxa_pwm_config

[Doug Brown]

Now that pxa_pwm_apply always enables the clock first, there is no need
for pxa_pwm_config to do any clock enabling/disabling.

Signed-off-by: Doug Brown <doug@schmorgal.com>
---
 drivers/pwm/pwm-pxa.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/drivers/pwm/pwm-pxa.c b/drivers/pwm/pwm-pxa.c
index 9ee9b41d62b8..cf4d22c91929 100644
--- a/drivers/pwm/pwm-pxa.c
+++ b/drivers/pwm/pwm-pxa.c
@@ -64,7 +64,6 @@ static int pxa_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	unsigned long long c;
 	unsigned long period_cycles, prescale, pv, dc;
 	unsigned long offset;
-	int rc;
 
 	offset = pwm->hwpwm ? 0x10 : 0;
 
@@ -86,18 +85,10 @@ static int pxa_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	else
 		dc = mul_u64_u64_div_u64(pv + 1, duty_ns, period_ns);
 
-	/* NOTE: the clock to PWM has to be enabled first
-	 * before writing to the registers
-	 */
-	rc = clk_prepare_enable(pc->clk);
-	if (rc < 0)
-		return rc;
-
 	writel(prescale, pc->mmio_base + offset + PWMCR);
 	writel(dc, pc->mmio_base + offset + PWMDCR);
 	writel(pv, pc->mmio_base + offset + PWMPCR);
 
-	clk_disable_unprepare(pc->clk);
 	return 0;
 }
 
-- 
2.34.1

[PATCH v2 4/5] pwm: pxa: Wait for final PWM period to finish

[Doug Brown]

If the clock is turned on too quickly after being turned off, it won't
actually turn back on. Work around this problem by waiting for the final
period to complete when disabling the PWM. The delay logic is borrowed
from the pwm-sun4i driver.

To avoid unnecessary delays, skip the whole config process if the PWM is
already disabled and staying disabled.

Signed-off-by: Doug Brown <doug@schmorgal.com>
---
 drivers/pwm/pwm-pxa.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/pwm/pwm-pxa.c b/drivers/pwm/pwm-pxa.c
index cf4d22c91929..edcef67f7ffe 100644
--- a/drivers/pwm/pwm-pxa.c
+++ b/drivers/pwm/pwm-pxa.c
@@ -17,6 +17,7 @@
 #include <linux/io.h>
 #include <linux/pwm.h>
 #include <linux/of_device.h>
+#include <linux/delay.h>
 
 #include <asm/div64.h>
 
@@ -96,12 +97,16 @@ static int pxa_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 			 const struct pwm_state *state)
 {
 	struct pxa_pwm_chip *pc = to_pxa_pwm_chip(chip);
+	unsigned int delay_us;
 	u64 duty_cycle;
 	int err;
 
 	if (state->polarity != PWM_POLARITY_NORMAL)
 		return -EINVAL;
 
+	if (!state->enabled && !pwm->state.enabled)
+		return 0;
+
 	err = clk_prepare_enable(pc->clk);
 	if (err)
 		return err;
@@ -122,6 +127,18 @@ static int pxa_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	if (!state->enabled && pwm->state.enabled)
 		clk_disable_unprepare(pc->clk);
 
+	if (state->enabled)
+		return 0;
+
+	/* Wait for the final PWM period to finish. This prevents it from
+	 * being re-enabled too quickly (which can fail silently).
+	 */
+	delay_us = DIV_ROUND_UP_ULL(state->period, NSEC_PER_USEC);
+	if ((delay_us / 500) > MAX_UDELAY_MS)
+		msleep(delay_us / 1000 + 1);
+	else
+		usleep_range(delay_us, delay_us * 2);
+
 	return 0;
 }
 
-- 
2.34.1

[PATCH v2 5/5] pwm: pxa: Enable for MMP platform

[Doug Brown]

The PXA168, which is part of the MMP platform, also uses this driver.

Signed-off-by: Doug Brown <doug@schmorgal.com>
---
 drivers/pwm/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 60d13a949bc5..d0d4caebf12f 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -434,7 +434,7 @@ config PWM_PCA9685
 
 config PWM_PXA
 	tristate "PXA PWM support"
-	depends on ARCH_PXA || COMPILE_TEST
+	depends on ARCH_PXA || ARCH_MMP || COMPILE_TEST
 	depends on HAS_IOMEM
 	help
 	  Generic PWM framework driver for PXA.
-- 
2.34.1

Re: [PATCH v2 1/5] pwm: pxa: Remove pxa_pwm_enable/disable

[Uwe Kleine-König]

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

On Sun, Oct 02, 2022 at 06:55:42PM -0700, Doug Brown wrote:
> These functions are only acting as wrappers for clk_prepare_enable and
> clk_disable_unprepare now, so remove them to simplify the driver.
> 
> Suggested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Signed-off-by: Doug Brown <doug@schmorgal.com>

Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Thanks
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [PATCH v2 2/5] pwm: pxa: Set duty cycle to 0 when disabling PWM

[Uwe Kleine-König]

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

On Sun, Oct 02, 2022 at 06:55:43PM -0700, Doug Brown wrote:
> When disabling PWM, the duty cycle needs to be set to 0. This prevents
> the previous duty cycle from showing up momentarily when the clock is
> re-enabled next time, and also prevents the output pin from being stuck
> high when the clock is disabled.
> 
> Because the clock has to be running in order to configure the duty
> cycle, unconditionally enable it early in pxa_pwm_apply and account for
> the correct enable count at the end.
> 
> Suggested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Signed-off-by: Doug Brown <doug@schmorgal.com>
> ---
>  drivers/pwm/pwm-pxa.c | 25 ++++++++++++++++---------
>  1 file changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-pxa.c b/drivers/pwm/pwm-pxa.c
> index 0ac052652c62..9ee9b41d62b8 100644
> --- a/drivers/pwm/pwm-pxa.c
> +++ b/drivers/pwm/pwm-pxa.c
> @@ -105,24 +105,31 @@ static int pxa_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  			 const struct pwm_state *state)
>  {
>  	struct pxa_pwm_chip *pc = to_pxa_pwm_chip(chip);
> +	u64 duty_cycle;
>  	int err;
>  
>  	if (state->polarity != PWM_POLARITY_NORMAL)
>  		return -EINVAL;
>  
> -	if (!state->enabled) {
> -		if (pwm->state.enabled)
> -			clk_disable_unprepare(pc->clk);
> +	err = clk_prepare_enable(pc->clk);
> +	if (err)
> +		return err;
>  
> -		return 0;
> -	}
> +	duty_cycle = state->enabled ? state->duty_cycle : 0;
>  
> -	err = pxa_pwm_config(chip, pwm, state->duty_cycle, state->period);
> -	if (err)
> +	err = pxa_pwm_config(chip, pwm, duty_cycle, state->period);
> +	if (err) {
> +		clk_disable_unprepare(pc->clk);
>  		return err;
> +	}
> +
> +	if (state->enabled && !pwm->state.enabled)
> +		return 0;
> +
> +	clk_disable_unprepare(pc->clk);
>  
> -	if (!pwm->state.enabled)
> -		return clk_prepare_enable(pc->clk);
> +	if (!state->enabled && pwm->state.enabled)
> +		clk_disable_unprepare(pc->clk);

Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [PATCH v2 3/5] pwm: pxa: Remove clk enable/disable from pxa_pwm_config

[Uwe Kleine-König]

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

On Sun, Oct 02, 2022 at 06:55:44PM -0700, Doug Brown wrote:
> Now that pxa_pwm_apply always enables the clock first, there is no need
> for pxa_pwm_config to do any clock enabling/disabling.
> 
> Signed-off-by: Doug Brown <doug@schmorgal.com>
> ---
>  drivers/pwm/pwm-pxa.c | 9 ---------
>  1 file changed, 9 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-pxa.c b/drivers/pwm/pwm-pxa.c
> index 9ee9b41d62b8..cf4d22c91929 100644
> --- a/drivers/pwm/pwm-pxa.c
> +++ b/drivers/pwm/pwm-pxa.c
> @@ -64,7 +64,6 @@ static int pxa_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  	unsigned long long c;
>  	unsigned long period_cycles, prescale, pv, dc;
>  	unsigned long offset;
> -	int rc;
>  
>  	offset = pwm->hwpwm ? 0x10 : 0;
>  
> @@ -86,18 +85,10 @@ static int pxa_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  	else
>  		dc = mul_u64_u64_div_u64(pv + 1, duty_ns, period_ns);
>  
> -	/* NOTE: the clock to PWM has to be enabled first
> -	 * before writing to the registers
> -	 */
> -	rc = clk_prepare_enable(pc->clk);
> -	if (rc < 0)
> -		return rc;
> -
>  	writel(prescale, pc->mmio_base + offset + PWMCR);
>  	writel(dc, pc->mmio_base + offset + PWMDCR);
>  	writel(pv, pc->mmio_base + offset + PWMPCR);
>  
> -	clk_disable_unprepare(pc->clk);
>  	return 0;
>  }

Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [PATCH v2 4/5] pwm: pxa: Wait for final PWM period to finish

[Uwe Kleine-König]

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

On Sun, Oct 02, 2022 at 06:55:45PM -0700, Doug Brown wrote:
> If the clock is turned on too quickly after being turned off, it won't
> actually turn back on. Work around this problem by waiting for the final
> period to complete when disabling the PWM. The delay logic is borrowed
> from the pwm-sun4i driver.
> 
> To avoid unnecessary delays, skip the whole config process if the PWM is
> already disabled and staying disabled.

I wonder if there is some documentation available about this issue. This
feels like a workaround without knowledge about the details and so might
break at the next opportunity.

> [...]
> @@ -122,6 +127,18 @@ static int pxa_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	if (!state->enabled && pwm->state.enabled)
>  		clk_disable_unprepare(pc->clk);
>  
> +	if (state->enabled)
> +		return 0;
> +
> +	/* Wait for the final PWM period to finish. This prevents it from
> +	 * being re-enabled too quickly (which can fail silently).
> +	 */

Please stick to the usual comment style. i.e. put the /* on a line for
itself.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [PATCH v2 5/5] pwm: pxa: Enable for MMP platform

[Uwe Kleine-König]

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

On Sun, Oct 02, 2022 at 06:55:46PM -0700, Doug Brown wrote:
> The PXA168, which is part of the MMP platform, also uses this driver.
> 
> Signed-off-by: Doug Brown <doug@schmorgal.com>
> ---
>  drivers/pwm/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 60d13a949bc5..d0d4caebf12f 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -434,7 +434,7 @@ config PWM_PCA9685
>  
>  config PWM_PXA
>  	tristate "PXA PWM support"
> -	depends on ARCH_PXA || COMPILE_TEST
> +	depends on ARCH_PXA || ARCH_MMP || COMPILE_TEST

Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [PATCH v2 4/5] pwm: pxa: Wait for final PWM period to finish

[Doug Brown]

Hi Uwe,

On 10/19/2022 12:39 AM, Uwe Kleine-König wrote:
> On Sun, Oct 02, 2022 at 06:55:45PM -0700, Doug Brown wrote:
>> If the clock is turned on too quickly after being turned off, it won't
>> actually turn back on. Work around this problem by waiting for the final
>> period to complete when disabling the PWM. The delay logic is borrowed
>> from the pwm-sun4i driver.
>>
>> To avoid unnecessary delays, skip the whole config process if the PWM is
>> already disabled and staying disabled.
> 
> I wonder if there is some documentation available about this issue. This
> feels like a workaround without knowledge about the details and so might
> break at the next opportunity.

Thanks for reviewing! Yes, it does feel like a crazy workaround. I'm not
super proud of it. The best documentation I've been able to look at is
the PXA168 software manual [1]. Page 502 of the PDF talks about a
"graceful shutdown" where turning off the clock enable bit doesn't
immediately stop the clock, and instead it waits for the current PWM
period to complete first. This driver is currently configuring it for
graceful shutdown mode, because the PWMCR_SD bit is not set (page 1257).

I've experimentally determined that if you try to turn the clock back on
when a graceful shutdown is still scheduled, it doesn't cancel the
graceful shutdown, so the clock ends up off afterward, even though the
common clock framework thinks it's still on. The hacky delay in this
commit works around that problem. This almost seems like a problem that
should be solved on the common clock framework side instead, but it
doesn't know what the PWM frequency is so it wouldn't know how long to
delay.

Do all the other similar drivers in the kernel do a graceful shutdown
like this when they are turned off? If not, a simpler solution would be
to start turning on the PWMCR_SD bit instead, so the clock stops
immediately (potentially resulting in the final duty cycle being short).
I tested that change in place of this commit and it seems to work pretty
well, although I can still cause it to fail if I turn my PWM backlight
off and back on quickly without a "sleep 0.000001" in between. It feels
to me like there are some weird interdependencies between the clock
enable bits and the actual PWM controller block, at least in the PXA168,
likely due to "graceful shutdown" mode's existence.

What do you think? Turning on the PWMCR_SD bit would be very simple, but
it doesn't fully fix the issue in my testing. I'd still be okay with it
though, because the only failure case I can reproduce is a minor edge
case (plus, I don't love the delay solution).

> 
>> [...]
>> @@ -122,6 +127,18 @@ static int pxa_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>>   	if (!state->enabled && pwm->state.enabled)
>>   		clk_disable_unprepare(pc->clk);
>>   
>> +	if (state->enabled)
>> +		return 0;
>> +
>> +	/* Wait for the final PWM period to finish. This prevents it from
>> +	 * being re-enabled too quickly (which can fail silently).
>> +	 */
> 
> Please stick to the usual comment style. i.e. put the /* on a line for
> itself.

My bad, thanks for pointing this out. If this comment still exists in
the next version of the patch, I'll fix it.

Thanks,
Doug

[1] 
https://web.archive.org/web/20160428154454/http://www.marvell.com/application-processors/armada-100/assets/armada_16x_software_manual.pdf

Re: [PATCH v2 4/5] pwm: pxa: Wait for final PWM period to finish

[Uwe Kleine-König]

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

Hello Doug,

On Sat, Oct 22, 2022 at 12:35:23PM -0700, Doug Brown wrote:
> On 10/19/2022 12:39 AM, Uwe Kleine-König wrote:
> > On Sun, Oct 02, 2022 at 06:55:45PM -0700, Doug Brown wrote:
> > > If the clock is turned on too quickly after being turned off, it won't
> > > actually turn back on. Work around this problem by waiting for the final
> > > period to complete when disabling the PWM. The delay logic is borrowed
> > > from the pwm-sun4i driver.
> > > 
> > > To avoid unnecessary delays, skip the whole config process if the PWM is
> > > already disabled and staying disabled.
> > 
> > I wonder if there is some documentation available about this issue. This
> > feels like a workaround without knowledge about the details and so might
> > break at the next opportunity.
> 
> Thanks for reviewing! Yes, it does feel like a crazy workaround. I'm not
> super proud of it. The best documentation I've been able to look at is
> the PXA168 software manual [1]. Page 502 of the PDF talks about a
> "graceful shutdown" where turning off the clock enable bit doesn't
> immediately stop the clock, and instead it waits for the current PWM
> period to complete first. This driver is currently configuring it for
> graceful shutdown mode, because the PWMCR_SD bit is not set (page 1257).
> 
> I've experimentally determined that if you try to turn the clock back on
> when a graceful shutdown is still scheduled, it doesn't cancel the
> graceful shutdown, so the clock ends up off afterward, even though the
> common clock framework thinks it's still on. The hacky delay in this
> commit works around that problem. This almost seems like a problem that
> should be solved on the common clock framework side instead, but it
> doesn't know what the PWM frequency is so it wouldn't know how long to
> delay.
> 
> Do all the other similar drivers in the kernel do a graceful shutdown
> like this when they are turned off? If not, a simpler solution would be
> to start turning on the PWMCR_SD bit instead, so the clock stops
> immediately (potentially resulting in the final duty cycle being short).

There are supported hardwares that (only) support immediate shutdown, so
consumers cannot rely on a graceful stop anyhow. So I'd say using
PWMCR_SD=1 is fine. The documentation suggests that in this case "PWM_OUTx stops
after at most one PSCLK_PWM" which might explain that the problem can
still be triggered. (i.e. if you reenable before the next PSCLK_PWM
cycle.)

BTW, the driver lacks information about how the hardware behaves on
disable. The most common cases are

 - emits the inactive output
 - freezes where it just happens to be (assuming PWMCR_SD=1)

Would you mind testing that and adding a patch to your series? (Look at
e.g. drivers/pwm/pwm-sl28cpld.c for the desired format to ease
grepping.)

> I tested that change in place of this commit and it seems to work pretty
> well, although I can still cause it to fail if I turn my PWM backlight
> off and back on quickly without a "sleep 0.000001" in between. It feels
> to me like there are some weird interdependencies between the clock
> enable bits and the actual PWM controller block, at least in the PXA168,
> likely due to "graceful shutdown" mode's existence.
> 
> What do you think? Turning on the PWMCR_SD bit would be very simple, but
> it doesn't fully fix the issue in my testing. I'd still be okay with it
> though, because the only failure case I can reproduce is a minor edge
> case (plus, I don't love the delay solution).

+1 for no love for the delay solution.
 
Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [PATCH v2 4/5] pwm: pxa: Wait for final PWM period to finish

[Doug Brown]

Hi Uwe,

On 11/9/2022 1:26 AM, Uwe Kleine-König wrote:

> There are supported hardwares that (only) support immediate shutdown, so
> consumers cannot rely on a graceful stop anyhow. So I'd say using
> PWMCR_SD=1 is fine. The documentation suggests that in this case "PWM_OUTx stops
> after at most one PSCLK_PWM" which might explain that the problem can
> still be triggered. (i.e. if you reenable before the next PSCLK_PWM
> cycle.)

Thanks for taking a look! Your point about PSCLK_PWM does make sense and
likely explains how I'm still able to trigger the problem.

> BTW, the driver lacks information about how the hardware behaves on
> disable. The most common cases are
> 
>   - emits the inactive output
>   - freezes where it just happens to be (assuming PWMCR_SD=1)
> 
> Would you mind testing that and adding a patch to your series? (Look at
> e.g. drivers/pwm/pwm-sl28cpld.c for the desired format to ease
> grepping.)

Will do. I assume you're referring to the "Limitations" section at the
top of a lot of the drivers. It appears that this hardware always emits
the inactive input when the clock disables -- almost immediately if
PWMCR_SD=1, or at the end of the current duty cycle if PWMCR_SD=0.

Doug

Re: [PATCH v2 2/5] pwm: pxa: Set duty cycle to 0 when disabling PWM

[Doug Brown]

Hi Uwe,

On 10/19/2022 12:34 AM, Uwe Kleine-König wrote:
> On Sun, Oct 02, 2022 at 06:55:43PM -0700, Doug Brown wrote:
>> When disabling PWM, the duty cycle needs to be set to 0. This prevents
>> the previous duty cycle from showing up momentarily when the clock is
>> re-enabled next time, and also prevents the output pin from being stuck
>> high when the clock is disabled.
> 
> Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

It appears I messed up when writing the commit message for this patch,
because I did some more testing today with raw register writes and the
output pin definitely doesn't get stuck high when I disable the clock --
no matter where we are in the PWM cycle. Either way, it still makes
sense to set the duty cycle to 0 first. Just wanted you to know that I'm
not changing the patch at all -- just the commit message -- on v3, so
I'm not carrying over your Reviewed-by tag on v3 of this patch. Sorry
about that!

Doug