[PATCH 0/5] pwm: jz4740: Fixes and some light changes

[PATCH 0/5] pwm: jz4740: Fixes and some light changes

[Paul Cercueil]

Hi Thierry, Uwe,

Here are two important fixes for the pwm-jz4740 driver. They address
problems with the PWM pin levels not being driven inactive when they
should be.

The patches 3-5 are simple changes and are not bugfixes.

Cheers,
-Paul


Paul Cercueil (5):
  pwm: jz4740: Fix pin level of disabled TCU2 channels, part 1
  pwm: jz4740: Fix pin level of disabled TCU2 channels, part 2
  pwm: jz4740: Force dependency on Device Tree
  pwm: jz4740: Depend on MACH_INGENIC instead of MIPS
  pwm: jz4740: Use regmap_{set,clear}_bits

 drivers/pwm/Kconfig      |  4 +-
 drivers/pwm/pwm-jz4740.c | 89 ++++++++++++++++++++++++----------------
 2 files changed, 55 insertions(+), 38 deletions(-)

-- 
2.35.1

[PATCH 1/5] pwm: jz4740: Fix pin level of disabled TCU2 channels, part 1

[Paul Cercueil]

The "duty > cycle" trick to force the pin level of a disabled TCU2
channel would only work when the channel had been enabled previously.

Address this issue by enabling the PWM mode in jz4740_pwm_disable
(I know, right) so that the "duty > cycle" trick works before disabling
the PWM channel right after.

This issue went unnoticed, as the PWM pins on the majority of the boards
tested would default to the inactive level once the corresponding TCU
clock was enabled, so the first call to jz4740_pwm_disable() would not
actually change the pin levels.

On the GCW Zero however, the PWM pin for the backlight (PWM1, which is
a TCU2 channel) goes active as soon as the timer1 clock is enabled.
Since the jz4740_pwm_disable() function did not work on channels not
previously enabled, the backlight would shine at full brightness from
the moment the backlight driver would probe, until the backlight driver
tried to *enable* the PWM output.

With this fix, the PWM pins will be forced inactive as soon as
jz4740_pwm_apply() is called (and might be reconfigured to active if
dictated by the pwm_state). This means that there is still a tiny time
frame between the .request() and .apply() callbacks where the PWM pin
might be active. Sadly, there is no way to fix this issue: it is
impossible to write a PWM channel's registers if the corresponding clock
is not enabled, and enabling the clock is what causes the PWM pin to go
active.

There is a workaround, though, which complements this fix: simply
starting the backlight driver (or any PWM client driver) with a "init"
pinctrl state that sets the pin as an inactive GPIO. Once the driver is
probed and the pinctrl state switches to "default", the regular PWM pin
configuration can be used as it will be properly driven.

Fixes: c2693514a0a1 ("pwm: jz4740: Obtain regmap from parent node")
Signed-off-by: Paul Cercueil <paul@crapouillou.net>
Cc: stable@vger.kernel.org
---
 drivers/pwm/pwm-jz4740.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
index a5fdf97c0d2e..228eb104bf1e 100644
--- a/drivers/pwm/pwm-jz4740.c
+++ b/drivers/pwm/pwm-jz4740.c
@@ -102,11 +102,14 @@ static void jz4740_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 	struct jz4740_pwm_chip *jz = to_jz4740(chip);
 
 	/*
-	 * Set duty > period. This trick allows the TCU channels in TCU2 mode to
-	 * properly return to their init level.
+	 * Set duty > period, then enable PWM mode and start the counter.
+	 * This trick allows to force the inactive pin level for the TCU2
+	 * channels.
 	 */
 	regmap_write(jz->map, TCU_REG_TDHRc(pwm->hwpwm), 0xffff);
 	regmap_write(jz->map, TCU_REG_TDFRc(pwm->hwpwm), 0x0);
+	regmap_set_bits(jz->map, TCU_REG_TCSRc(pwm->hwpwm), TCU_TCSR_PWM_EN);
+	regmap_write(jz->map, TCU_REG_TESR, BIT(pwm->hwpwm));
 
 	/*
 	 * Disable PWM output.
-- 
2.35.1

[PATCH 2/5] pwm: jz4740: Fix pin level of disabled TCU2 channels, part 2

[Paul Cercueil]

After commit a020f22a4ff5 ("pwm: jz4740: Make PWM start with the active part"),
the trick to set duty > period to properly shut down TCU2 channels did
not work anymore, because of the polarity inversion.

Address this issue by restoring the proper polarity before disabling the
channels.

Fixes: a020f22a4ff5 ("pwm: jz4740: Make PWM start with the active part")
Signed-off-by: Paul Cercueil <paul@crapouillou.net>
Cc: stable@vger.kernel.org
---
 drivers/pwm/pwm-jz4740.c | 62 ++++++++++++++++++++++++++--------------
 1 file changed, 40 insertions(+), 22 deletions(-)

diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
index 228eb104bf1e..65462a0052af 100644
--- a/drivers/pwm/pwm-jz4740.c
+++ b/drivers/pwm/pwm-jz4740.c
@@ -97,6 +97,19 @@ static int jz4740_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 	return 0;
 }
 
+static void jz4740_pwm_set_polarity(struct jz4740_pwm_chip *jz,
+				    unsigned int hwpwm,
+				    enum pwm_polarity polarity)
+{
+	unsigned int value = 0;
+
+	if (polarity == PWM_POLARITY_INVERSED)
+		value = TCU_TCSR_PWM_INITL_HIGH;
+
+	regmap_update_bits(jz->map, TCU_REG_TCSRc(hwpwm),
+			   TCU_TCSR_PWM_INITL_HIGH, value);
+}
+
 static void jz4740_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct jz4740_pwm_chip *jz = to_jz4740(chip);
@@ -130,6 +143,7 @@ static int jz4740_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	unsigned long long tmp = 0xffffull * NSEC_PER_SEC;
 	struct clk *clk = pwm_get_chip_data(pwm);
 	unsigned long period, duty;
+	enum pwm_polarity polarity;
 	long rate;
 	int err;
 
@@ -169,6 +183,9 @@ static int jz4740_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	if (duty >= period)
 		duty = period - 1;
 
+	/* Restore regular polarity before disabling the channel. */
+	jz4740_pwm_set_polarity(jz4740, pwm->hwpwm, state->polarity);
+
 	jz4740_pwm_disable(chip, pwm);
 
 	err = clk_set_rate(clk, rate);
@@ -190,29 +207,30 @@ static int jz4740_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm),
 			   TCU_TCSR_PWM_SD, TCU_TCSR_PWM_SD);
 
-	/*
-	 * Set polarity.
-	 *
-	 * The PWM starts in inactive state until the internal timer reaches the
-	 * duty value, then becomes active until the timer reaches the period
-	 * value. In theory, we should then use (period - duty) as the real duty
-	 * value, as a high duty value would otherwise result in the PWM pin
-	 * being inactive most of the time.
-	 *
-	 * Here, we don't do that, and instead invert the polarity of the PWM
-	 * when it is active. This trick makes the PWM start with its active
-	 * state instead of its inactive state.
-	 */
-	if ((state->polarity == PWM_POLARITY_NORMAL) ^ state->enabled)
-		regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm),
-				   TCU_TCSR_PWM_INITL_HIGH, 0);
-	else
-		regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm),
-				   TCU_TCSR_PWM_INITL_HIGH,
-				   TCU_TCSR_PWM_INITL_HIGH);
-
-	if (state->enabled)
+	if (state->enabled) {
+		/*
+		 * Set polarity.
+		 *
+		 * The PWM starts in inactive state until the internal timer
+		 * reaches the duty value, then becomes active until the timer
+		 * reaches the period value. In theory, we should then use
+		 * (period - duty) as the real duty value, as a high duty value
+		 * would otherwise result in the PWM pin being inactive most of
+		 * the time.
+		 *
+		 * Here, we don't do that, and instead invert the polarity of
+		 * the PWM when it is active. This trick makes the PWM start
+		 * with its active state instead of its inactive state.
+		 */
+		if (state->polarity == PWM_POLARITY_NORMAL)
+			polarity = PWM_POLARITY_INVERSED;
+		else
+			polarity = PWM_POLARITY_NORMAL;
+
+		jz4740_pwm_set_polarity(jz4740, pwm->hwpwm, polarity);
+
 		jz4740_pwm_enable(chip, pwm);
+	}
 
 	return 0;
 }
-- 
2.35.1

[PATCH 3/5] pwm: jz4740: Force dependency on Device Tree

[Paul Cercueil]

Ingenic SoCs all require CONFIG_OF, so there is no case where we want to
use this driver without CONFIG_OF.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/pwm/Kconfig      |  2 +-
 drivers/pwm/pwm-jz4740.c | 10 ++++------
 2 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 60d13a949bc5..1fe420a45f91 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -283,7 +283,7 @@ config PWM_IQS620A
 config PWM_JZ4740
 	tristate "Ingenic JZ47xx PWM support"
 	depends on MIPS || COMPILE_TEST
-	depends on COMMON_CLK
+	depends on COMMON_CLK && OF
 	select MFD_SYSCON
 	help
 	  Generic PWM framework driver for Ingenic JZ47xx based
diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
index 65462a0052af..c0afc0c316a8 100644
--- a/drivers/pwm/pwm-jz4740.c
+++ b/drivers/pwm/pwm-jz4740.c
@@ -269,19 +269,18 @@ static int jz4740_pwm_probe(struct platform_device *pdev)
 	return devm_pwmchip_add(dev, &jz4740->chip);
 }
 
-static const struct soc_info __maybe_unused jz4740_soc_info = {
+static const struct soc_info jz4740_soc_info = {
 	.num_pwms = 8,
 };
 
-static const struct soc_info __maybe_unused jz4725b_soc_info = {
+static const struct soc_info jz4725b_soc_info = {
 	.num_pwms = 6,
 };
 
-static const struct soc_info __maybe_unused x1000_soc_info = {
+static const struct soc_info x1000_soc_info = {
 	.num_pwms = 5,
 };
 
-#ifdef CONFIG_OF
 static const struct of_device_id jz4740_pwm_dt_ids[] = {
 	{ .compatible = "ingenic,jz4740-pwm", .data = &jz4740_soc_info },
 	{ .compatible = "ingenic,jz4725b-pwm", .data = &jz4725b_soc_info },
@@ -289,12 +288,11 @@ static const struct of_device_id jz4740_pwm_dt_ids[] = {
 	{},
 };
 MODULE_DEVICE_TABLE(of, jz4740_pwm_dt_ids);
-#endif
 
 static struct platform_driver jz4740_pwm_driver = {
 	.driver = {
 		.name = "jz4740-pwm",
-		.of_match_table = of_match_ptr(jz4740_pwm_dt_ids),
+		.of_match_table = jz4740_pwm_dt_ids,
 	},
 	.probe = jz4740_pwm_probe,
 };
-- 
2.35.1

[PATCH 4/5] pwm: jz4740: Depend on MACH_INGENIC instead of MIPS

[Paul Cercueil]

The MACH_INGENIC Kconfig option will be selected when building a kernel
targeting Ingenic SoCs, but also when compiling a generic MIPS kernel
that happens to support Ingenic SoCs.

Therefore, if MACH_INGENIC is not set, we know that we're not even
trying to build a generic kernel that supports these SoCs, and we can
hide the options to compile the SoC-specific drivers.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/pwm/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 1fe420a45f91..cb623d0702f6 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -282,7 +282,7 @@ config PWM_IQS620A
 
 config PWM_JZ4740
 	tristate "Ingenic JZ47xx PWM support"
-	depends on MIPS || COMPILE_TEST
+	depends on MACH_INGENIC || COMPILE_TEST
 	depends on COMMON_CLK && OF
 	select MFD_SYSCON
 	help
-- 
2.35.1

[PATCH 5/5] pwm: jz4740: Use regmap_{set,clear}_bits

[Paul Cercueil]

Simplify a bit the code by using regmap_set_bits() and
regmap_clear_bits() instead of regmap_update_bits() when possible.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/pwm/pwm-jz4740.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
index c0afc0c316a8..22fcdca66081 100644
--- a/drivers/pwm/pwm-jz4740.c
+++ b/drivers/pwm/pwm-jz4740.c
@@ -88,8 +88,7 @@ static int jz4740_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 	struct jz4740_pwm_chip *jz = to_jz4740(chip);
 
 	/* Enable PWM output */
-	regmap_update_bits(jz->map, TCU_REG_TCSRc(pwm->hwpwm),
-			   TCU_TCSR_PWM_EN, TCU_TCSR_PWM_EN);
+	regmap_set_bits(jz->map, TCU_REG_TCSRc(pwm->hwpwm), TCU_TCSR_PWM_EN);
 
 	/* Start counter */
 	regmap_write(jz->map, TCU_REG_TESR, BIT(pwm->hwpwm));
@@ -129,8 +128,7 @@ static void jz4740_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 	 * In TCU2 mode (channel 1/2 on JZ4750+), this must be done before the
 	 * counter is stopped, while in TCU1 mode the order does not matter.
 	 */
-	regmap_update_bits(jz->map, TCU_REG_TCSRc(pwm->hwpwm),
-			   TCU_TCSR_PWM_EN, 0);
+	regmap_clear_bits(jz->map, TCU_REG_TCSRc(pwm->hwpwm), TCU_TCSR_PWM_EN);
 
 	/* Stop counter */
 	regmap_write(jz->map, TCU_REG_TECR, BIT(pwm->hwpwm));
@@ -204,8 +202,8 @@ static int jz4740_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	regmap_write(jz4740->map, TCU_REG_TDFRc(pwm->hwpwm), period);
 
 	/* Set abrupt shutdown */
-	regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm),
-			   TCU_TCSR_PWM_SD, TCU_TCSR_PWM_SD);
+	regmap_set_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm),
+			TCU_TCSR_PWM_SD);
 
 	if (state->enabled) {
 		/*
-- 
2.35.1

Re: [PATCH 1/5] pwm: jz4740: Fix pin level of disabled TCU2 channels, part 1

[Uwe Kleine-König]

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

Hello,

On Mon, Oct 24, 2022 at 09:52:09PM +0100, Paul Cercueil wrote:
> The "duty > cycle" trick to force the pin level of a disabled TCU2
> channel would only work when the channel had been enabled previously.
> 
> Address this issue by enabling the PWM mode in jz4740_pwm_disable
> (I know, right) so that the "duty > cycle" trick works before disabling
> the PWM channel right after.
> 
> This issue went unnoticed, as the PWM pins on the majority of the boards
> tested would default to the inactive level once the corresponding TCU
> clock was enabled, so the first call to jz4740_pwm_disable() would not
> actually change the pin levels.
> 
> On the GCW Zero however, the PWM pin for the backlight (PWM1, which is
> a TCU2 channel) goes active as soon as the timer1 clock is enabled.
> Since the jz4740_pwm_disable() function did not work on channels not
> previously enabled, the backlight would shine at full brightness from
> the moment the backlight driver would probe, until the backlight driver
> tried to *enable* the PWM output.
> 
> With this fix, the PWM pins will be forced inactive as soon as
> jz4740_pwm_apply() is called (and might be reconfigured to active if
> dictated by the pwm_state). This means that there is still a tiny time
> frame between the .request() and .apply() callbacks where the PWM pin
> might be active. Sadly, there is no way to fix this issue: it is
> impossible to write a PWM channel's registers if the corresponding clock
> is not enabled, and enabling the clock is what causes the PWM pin to go
> active.
> 
> There is a workaround, though, which complements this fix: simply
> starting the backlight driver (or any PWM client driver) with a "init"
> pinctrl state that sets the pin as an inactive GPIO. Once the driver is
> probed and the pinctrl state switches to "default", the regular PWM pin
> configuration can be used as it will be properly driven.
> 
> Fixes: c2693514a0a1 ("pwm: jz4740: Obtain regmap from parent node")
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> Cc: stable@vger.kernel.org

OK, understood the issue. I think there is another similar issue: The
clk is get and enabled only in the .request() callback. The result is (I
think---depends on a few further conditions) that if you have the
backlight driver as a module and the bootloader enables the backlight to
show a splash screen, the backlight goes off because of the
clk_disable_unused initcall.

So the right thing to do is to get the clock in .probe(), and ensure it
is kept on if the PWM is running already. Then you can also enable the
counter in .probe() and don't care for it in the enable and disable
functions.

The init pinctrl then has to be on the PWM then, but that's IMHO ok.

Best regards
Uwe

PS: While looking into the driver I noticed that .request() uses
dev_err_probe(). That's wrong, this function is only supposed to be used
in .probe().

-- 
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 2/5] pwm: jz4740: Fix pin level of disabled TCU2 channels, part 2

[Uwe Kleine-König]

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

On Mon, Oct 24, 2022 at 09:52:10PM +0100, Paul Cercueil wrote:
> After commit a020f22a4ff5 ("pwm: jz4740: Make PWM start with the active part"),
> the trick to set duty > period to properly shut down TCU2 channels did
> not work anymore, because of the polarity inversion.
> 
> Address this issue by restoring the proper polarity before disabling the
> channels.
> 
> Fixes: a020f22a4ff5 ("pwm: jz4740: Make PWM start with the active part")
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> Cc: stable@vger.kernel.org
> ---
>  drivers/pwm/pwm-jz4740.c | 62 ++++++++++++++++++++++++++--------------
>  1 file changed, 40 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
> index 228eb104bf1e..65462a0052af 100644
> --- a/drivers/pwm/pwm-jz4740.c
> +++ b/drivers/pwm/pwm-jz4740.c
> @@ -97,6 +97,19 @@ static int jz4740_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>  	return 0;
>  }
>  
> +static void jz4740_pwm_set_polarity(struct jz4740_pwm_chip *jz,
> +				    unsigned int hwpwm,
> +				    enum pwm_polarity polarity)
> +{
> +	unsigned int value = 0;
> +
> +	if (polarity == PWM_POLARITY_INVERSED)
> +		value = TCU_TCSR_PWM_INITL_HIGH;
> +
> +	regmap_update_bits(jz->map, TCU_REG_TCSRc(hwpwm),
> +			   TCU_TCSR_PWM_INITL_HIGH, value);
> +}
> +
>  static void jz4740_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>  {
>  	struct jz4740_pwm_chip *jz = to_jz4740(chip);
> @@ -130,6 +143,7 @@ static int jz4740_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	unsigned long long tmp = 0xffffull * NSEC_PER_SEC;
>  	struct clk *clk = pwm_get_chip_data(pwm);
>  	unsigned long period, duty;
> +	enum pwm_polarity polarity;
>  	long rate;
>  	int err;
>  
> @@ -169,6 +183,9 @@ static int jz4740_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	if (duty >= period)
>  		duty = period - 1;
>  
> +	/* Restore regular polarity before disabling the channel. */
> +	jz4740_pwm_set_polarity(jz4740, pwm->hwpwm, state->polarity);
> +

Does this introduce a glitch?

>  	jz4740_pwm_disable(chip, pwm);
>  
>  	err = clk_set_rate(clk, rate);
> @@ -190,29 +207,30 @@ static int jz4740_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm),
>  			   TCU_TCSR_PWM_SD, TCU_TCSR_PWM_SD);
>  
> -	/*
> -	 * Set polarity.
> -	 *
> -	 * The PWM starts in inactive state until the internal timer reaches the
> -	 * duty value, then becomes active until the timer reaches the period
> -	 * value. In theory, we should then use (period - duty) as the real duty
> -	 * value, as a high duty value would otherwise result in the PWM pin
> -	 * being inactive most of the time.
> -	 *
> -	 * Here, we don't do that, and instead invert the polarity of the PWM
> -	 * when it is active. This trick makes the PWM start with its active
> -	 * state instead of its inactive state.
> -	 */
> -	if ((state->polarity == PWM_POLARITY_NORMAL) ^ state->enabled)
> -		regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm),
> -				   TCU_TCSR_PWM_INITL_HIGH, 0);
> -	else
> -		regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm),
> -				   TCU_TCSR_PWM_INITL_HIGH,
> -				   TCU_TCSR_PWM_INITL_HIGH);
> -
> -	if (state->enabled)
> +	if (state->enabled) {
> +		/*
> +		 * Set polarity.
> +		 *
> +		 * The PWM starts in inactive state until the internal timer
> +		 * reaches the duty value, then becomes active until the timer
> +		 * reaches the period value. In theory, we should then use
> +		 * (period - duty) as the real duty value, as a high duty value
> +		 * would otherwise result in the PWM pin being inactive most of
> +		 * the time.
> +		 *
> +		 * Here, we don't do that, and instead invert the polarity of
> +		 * the PWM when it is active. This trick makes the PWM start
> +		 * with its active state instead of its inactive state.
> +		 */
> +		if (state->polarity == PWM_POLARITY_NORMAL)
> +			polarity = PWM_POLARITY_INVERSED;
> +		else
> +			polarity = PWM_POLARITY_NORMAL;
> +
> +		jz4740_pwm_set_polarity(jz4740, pwm->hwpwm, polarity);
> +
>  		jz4740_pwm_enable(chip, pwm);
> +	}

Note that for disabled PWMs there is no official guaranty about the pin
state. So it would be ok (but admittedly not great) to simplify the
driver and accept that the pinstate is active while the PWM is off.
IMHO this is also better than a glitch.

If a consumer wants the PWM to be in its inactive state, they should
not disable it.

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 1/5] pwm: jz4740: Fix pin level of disabled TCU2 channels, part 1

[Paul Cercueil]

Le mar. 25 oct. 2022 à 08:21:29 +0200, Uwe Kleine-König 
<u.kleine-koenig@pengutronix.de> a écrit :
> Hello,
> 
> On Mon, Oct 24, 2022 at 09:52:09PM +0100, Paul Cercueil wrote:
>>  The "duty > cycle" trick to force the pin level of a disabled TCU2
>>  channel would only work when the channel had been enabled 
>> previously.
>> 
>>  Address this issue by enabling the PWM mode in jz4740_pwm_disable
>>  (I know, right) so that the "duty > cycle" trick works before 
>> disabling
>>  the PWM channel right after.
>> 
>>  This issue went unnoticed, as the PWM pins on the majority of the 
>> boards
>>  tested would default to the inactive level once the corresponding 
>> TCU
>>  clock was enabled, so the first call to jz4740_pwm_disable() would 
>> not
>>  actually change the pin levels.
>> 
>>  On the GCW Zero however, the PWM pin for the backlight (PWM1, which 
>> is
>>  a TCU2 channel) goes active as soon as the timer1 clock is enabled.
>>  Since the jz4740_pwm_disable() function did not work on channels not
>>  previously enabled, the backlight would shine at full brightness 
>> from
>>  the moment the backlight driver would probe, until the backlight 
>> driver
>>  tried to *enable* the PWM output.
>> 
>>  With this fix, the PWM pins will be forced inactive as soon as
>>  jz4740_pwm_apply() is called (and might be reconfigured to active if
>>  dictated by the pwm_state). This means that there is still a tiny 
>> time
>>  frame between the .request() and .apply() callbacks where the PWM 
>> pin
>>  might be active. Sadly, there is no way to fix this issue: it is
>>  impossible to write a PWM channel's registers if the corresponding 
>> clock
>>  is not enabled, and enabling the clock is what causes the PWM pin 
>> to go
>>  active.
>> 
>>  There is a workaround, though, which complements this fix: simply
>>  starting the backlight driver (or any PWM client driver) with a 
>> "init"
>>  pinctrl state that sets the pin as an inactive GPIO. Once the 
>> driver is
>>  probed and the pinctrl state switches to "default", the regular PWM 
>> pin
>>  configuration can be used as it will be properly driven.
>> 
>>  Fixes: c2693514a0a1 ("pwm: jz4740: Obtain regmap from parent node")
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  Cc: stable@vger.kernel.org
> 
> OK, understood the issue. I think there is another similar issue: The
> clk is get and enabled only in the .request() callback. The result is 
> (I
> think---depends on a few further conditions) that if you have the
> backlight driver as a module and the bootloader enables the backlight 
> to
> show a splash screen, the backlight goes off because of the
> clk_disable_unused initcall.

I will have to verify, but I'm pretty sure disabling the clock doesn't 
change the pin level back to inactive.

-Paul

> So the right thing to do is to get the clock in .probe(), and ensure 
> it
> is kept on if the PWM is running already. Then you can also enable the
> counter in .probe() and don't care for it in the enable and disable
> functions.
> 
> The init pinctrl then has to be on the PWM then, but that's IMHO ok.
> 
> Best regards
> Uwe
> 
> PS: While looking into the driver I noticed that .request() uses
> dev_err_probe(). That's wrong, this function is only supposed to be 
> used
> in .probe().
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König        
>     |
> Industrial Linux Solutions                 | 
> https://www.pengutronix.de/ |

Re: [PATCH 2/5] pwm: jz4740: Fix pin level of disabled TCU2 channels, part 2

[Paul Cercueil]

Le mar. 25 oct. 2022 à 08:44:10 +0200, Uwe Kleine-König 
<u.kleine-koenig@pengutronix.de> a écrit :
> On Mon, Oct 24, 2022 at 09:52:10PM +0100, Paul Cercueil wrote:
>>  After commit a020f22a4ff5 ("pwm: jz4740: Make PWM start with the 
>> active part"),
>>  the trick to set duty > period to properly shut down TCU2 channels 
>> did
>>  not work anymore, because of the polarity inversion.
>> 
>>  Address this issue by restoring the proper polarity before 
>> disabling the
>>  channels.
>> 
>>  Fixes: a020f22a4ff5 ("pwm: jz4740: Make PWM start with the active 
>> part")
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  Cc: stable@vger.kernel.org
>>  ---
>>   drivers/pwm/pwm-jz4740.c | 62 
>> ++++++++++++++++++++++++++--------------
>>   1 file changed, 40 insertions(+), 22 deletions(-)
>> 
>>  diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
>>  index 228eb104bf1e..65462a0052af 100644
>>  --- a/drivers/pwm/pwm-jz4740.c
>>  +++ b/drivers/pwm/pwm-jz4740.c
>>  @@ -97,6 +97,19 @@ static int jz4740_pwm_enable(struct pwm_chip 
>> *chip, struct pwm_device *pwm)
>>   	return 0;
>>   }
>> 
>>  +static void jz4740_pwm_set_polarity(struct jz4740_pwm_chip *jz,
>>  +				    unsigned int hwpwm,
>>  +				    enum pwm_polarity polarity)
>>  +{
>>  +	unsigned int value = 0;
>>  +
>>  +	if (polarity == PWM_POLARITY_INVERSED)
>>  +		value = TCU_TCSR_PWM_INITL_HIGH;
>>  +
>>  +	regmap_update_bits(jz->map, TCU_REG_TCSRc(hwpwm),
>>  +			   TCU_TCSR_PWM_INITL_HIGH, value);
>>  +}
>>  +
>>   static void jz4740_pwm_disable(struct pwm_chip *chip, struct 
>> pwm_device *pwm)
>>   {
>>   	struct jz4740_pwm_chip *jz = to_jz4740(chip);
>>  @@ -130,6 +143,7 @@ static int jz4740_pwm_apply(struct pwm_chip 
>> *chip, struct pwm_device *pwm,
>>   	unsigned long long tmp = 0xffffull * NSEC_PER_SEC;
>>   	struct clk *clk = pwm_get_chip_data(pwm);
>>   	unsigned long period, duty;
>>  +	enum pwm_polarity polarity;
>>   	long rate;
>>   	int err;
>> 
>>  @@ -169,6 +183,9 @@ static int jz4740_pwm_apply(struct pwm_chip 
>> *chip, struct pwm_device *pwm,
>>   	if (duty >= period)
>>   		duty = period - 1;
>> 
>>  +	/* Restore regular polarity before disabling the channel. */
>>  +	jz4740_pwm_set_polarity(jz4740, pwm->hwpwm, state->polarity);
>>  +
> 
> Does this introduce a glitch?

Maybe. But the PWM is shut down before finishing its period anyway, so 
there was already a glitch.

>>   	jz4740_pwm_disable(chip, pwm);
>> 
>>   	err = clk_set_rate(clk, rate);
>>  @@ -190,29 +207,30 @@ static int jz4740_pwm_apply(struct pwm_chip 
>> *chip, struct pwm_device *pwm,
>>   	regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm),
>>   			   TCU_TCSR_PWM_SD, TCU_TCSR_PWM_SD);
>> 
>>  -	/*
>>  -	 * Set polarity.
>>  -	 *
>>  -	 * The PWM starts in inactive state until the internal timer 
>> reaches the
>>  -	 * duty value, then becomes active until the timer reaches the 
>> period
>>  -	 * value. In theory, we should then use (period - duty) as the 
>> real duty
>>  -	 * value, as a high duty value would otherwise result in the PWM 
>> pin
>>  -	 * being inactive most of the time.
>>  -	 *
>>  -	 * Here, we don't do that, and instead invert the polarity of the 
>> PWM
>>  -	 * when it is active. This trick makes the PWM start with its 
>> active
>>  -	 * state instead of its inactive state.
>>  -	 */
>>  -	if ((state->polarity == PWM_POLARITY_NORMAL) ^ state->enabled)
>>  -		regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm),
>>  -				   TCU_TCSR_PWM_INITL_HIGH, 0);
>>  -	else
>>  -		regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm),
>>  -				   TCU_TCSR_PWM_INITL_HIGH,
>>  -				   TCU_TCSR_PWM_INITL_HIGH);
>>  -
>>  -	if (state->enabled)
>>  +	if (state->enabled) {
>>  +		/*
>>  +		 * Set polarity.
>>  +		 *
>>  +		 * The PWM starts in inactive state until the internal timer
>>  +		 * reaches the duty value, then becomes active until the timer
>>  +		 * reaches the period value. In theory, we should then use
>>  +		 * (period - duty) as the real duty value, as a high duty value
>>  +		 * would otherwise result in the PWM pin being inactive most of
>>  +		 * the time.
>>  +		 *
>>  +		 * Here, we don't do that, and instead invert the polarity of
>>  +		 * the PWM when it is active. This trick makes the PWM start
>>  +		 * with its active state instead of its inactive state.
>>  +		 */
>>  +		if (state->polarity == PWM_POLARITY_NORMAL)
>>  +			polarity = PWM_POLARITY_INVERSED;
>>  +		else
>>  +			polarity = PWM_POLARITY_NORMAL;
>>  +
>>  +		jz4740_pwm_set_polarity(jz4740, pwm->hwpwm, polarity);
>>  +
>>   		jz4740_pwm_enable(chip, pwm);
>>  +	}
> 
> Note that for disabled PWMs there is no official guaranty about the 
> pin
> state. So it would be ok (but admittedly not great) to simplify the
> driver and accept that the pinstate is active while the PWM is off.
> IMHO this is also better than a glitch.
> 
> If a consumer wants the PWM to be in its inactive state, they should
> not disable it.

Completely disagree. I absolutely do not want the backlight to go full 
bright mode when the PWM pin is disabled. And disabling the backlight 
is a thing (for screen blanking and during mode changes).

-Paul

Re: [PATCH 4/5] pwm: jz4740: Depend on MACH_INGENIC instead of MIPS

[Philippe Mathieu-Daudé]

On 24/10/22 22:52, Paul Cercueil wrote:
> The MACH_INGENIC Kconfig option will be selected when building a kernel
> targeting Ingenic SoCs, but also when compiling a generic MIPS kernel
> that happens to support Ingenic SoCs.
> 
> Therefore, if MACH_INGENIC is not set, we know that we're not even
> trying to build a generic kernel that supports these SoCs, and we can
> hide the options to compile the SoC-specific drivers.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>   drivers/pwm/Kconfig | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 5/5] pwm: jz4740: Use regmap_{set,clear}_bits

[Philippe Mathieu-Daudé]

On 24/10/22 22:52, Paul Cercueil wrote:
> Simplify a bit the code by using regmap_set_bits() and
> regmap_clear_bits() instead of regmap_update_bits() when possible.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>   drivers/pwm/pwm-jz4740.c | 10 ++++------
>   1 file changed, 4 insertions(+), 6 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 4/5] pwm: jz4740: Depend on MACH_INGENIC instead of MIPS

[Uwe Kleine-König]

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

On Mon, Oct 24, 2022 at 09:52:12PM +0100, Paul Cercueil wrote:
> The MACH_INGENIC Kconfig option will be selected when building a kernel
> targeting Ingenic SoCs, but also when compiling a generic MIPS kernel
> that happens to support Ingenic SoCs.
> 
> Therefore, if MACH_INGENIC is not set, we know that we're not even
> trying to build a generic kernel that supports these SoCs, and we can
> hide the options to compile the SoC-specific drivers.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>

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

(This patch even makes sense to apply independent of the other patches
in this series.)

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 5/5] pwm: jz4740: Use regmap_{set,clear}_bits

[Uwe Kleine-König]

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

Hello,

On Mon, Oct 24, 2022 at 09:52:13PM +0100, Paul Cercueil wrote:
> Simplify a bit the code by using regmap_set_bits() and
> regmap_clear_bits() instead of regmap_update_bits() when possible.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  drivers/pwm/pwm-jz4740.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
> index c0afc0c316a8..22fcdca66081 100644
> --- a/drivers/pwm/pwm-jz4740.c
> +++ b/drivers/pwm/pwm-jz4740.c
> @@ -88,8 +88,7 @@ static int jz4740_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>  	struct jz4740_pwm_chip *jz = to_jz4740(chip);
>  
>  	/* Enable PWM output */
> -	regmap_update_bits(jz->map, TCU_REG_TCSRc(pwm->hwpwm),
> -			   TCU_TCSR_PWM_EN, TCU_TCSR_PWM_EN);
> +	regmap_set_bits(jz->map, TCU_REG_TCSRc(pwm->hwpwm), TCU_TCSR_PWM_EN);
>  
>  	/* Start counter */
>  	regmap_write(jz->map, TCU_REG_TESR, BIT(pwm->hwpwm));
> @@ -129,8 +128,7 @@ static void jz4740_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>  	 * In TCU2 mode (channel 1/2 on JZ4750+), this must be done before the
>  	 * counter is stopped, while in TCU1 mode the order does not matter.
>  	 */
> -	regmap_update_bits(jz->map, TCU_REG_TCSRc(pwm->hwpwm),
> -			   TCU_TCSR_PWM_EN, 0);
> +	regmap_clear_bits(jz->map, TCU_REG_TCSRc(pwm->hwpwm), TCU_TCSR_PWM_EN);
>  
>  	/* Stop counter */
>  	regmap_write(jz->map, TCU_REG_TECR, BIT(pwm->hwpwm));
> @@ -204,8 +202,8 @@ static int jz4740_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	regmap_write(jz4740->map, TCU_REG_TDFRc(pwm->hwpwm), period);
>  
>  	/* Set abrupt shutdown */
> -	regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm),
> -			   TCU_TCSR_PWM_SD, TCU_TCSR_PWM_SD);
> +	regmap_set_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm),
> +			TCU_TCSR_PWM_SD);

Nitpick: the regmap_set_bits call is short enough to be put on a single
line.

Other than that (and even if the line is kept as is):

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 1/5] pwm: jz4740: Fix pin level of disabled TCU2 channels, part 1

[Uwe Kleine-König]

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

Hello Paul,

On Tue, Oct 25, 2022 at 11:02:00AM +0100, Paul Cercueil wrote:
> Le mar. 25 oct. 2022 à 08:21:29 +0200, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> a écrit :
> > Hello,
> > 
> > On Mon, Oct 24, 2022 at 09:52:09PM +0100, Paul Cercueil wrote:
> > >  The "duty > cycle" trick to force the pin level of a disabled TCU2
> > >  channel would only work when the channel had been enabled
> > > previously.
> > > 
> > >  Address this issue by enabling the PWM mode in jz4740_pwm_disable
> > >  (I know, right) so that the "duty > cycle" trick works before
> > > disabling
> > >  the PWM channel right after.
> > > 
> > >  This issue went unnoticed, as the PWM pins on the majority of the
> > > boards
> > >  tested would default to the inactive level once the corresponding
> > > TCU
> > >  clock was enabled, so the first call to jz4740_pwm_disable() would
> > > not
> > >  actually change the pin levels.
> > > 
> > >  On the GCW Zero however, the PWM pin for the backlight (PWM1, which
> > > is
> > >  a TCU2 channel) goes active as soon as the timer1 clock is enabled.
> > >  Since the jz4740_pwm_disable() function did not work on channels not
> > >  previously enabled, the backlight would shine at full brightness
> > > from
> > >  the moment the backlight driver would probe, until the backlight
> > > driver
> > >  tried to *enable* the PWM output.
> > > 
> > >  With this fix, the PWM pins will be forced inactive as soon as
> > >  jz4740_pwm_apply() is called (and might be reconfigured to active if
> > >  dictated by the pwm_state). This means that there is still a tiny
> > > time
> > >  frame between the .request() and .apply() callbacks where the PWM
> > > pin
> > >  might be active. Sadly, there is no way to fix this issue: it is
> > >  impossible to write a PWM channel's registers if the corresponding
> > > clock
> > >  is not enabled, and enabling the clock is what causes the PWM pin
> > > to go
> > >  active.
> > > 
> > >  There is a workaround, though, which complements this fix: simply
> > >  starting the backlight driver (or any PWM client driver) with a
> > > "init"
> > >  pinctrl state that sets the pin as an inactive GPIO. Once the
> > > driver is
> > >  probed and the pinctrl state switches to "default", the regular PWM
> > > pin
> > >  configuration can be used as it will be properly driven.
> > > 
> > >  Fixes: c2693514a0a1 ("pwm: jz4740: Obtain regmap from parent node")
> > >  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> > >  Cc: stable@vger.kernel.org
> > 
> > OK, understood the issue. I think there is another similar issue: The
> > clk is get and enabled only in the .request() callback. The result is (I
> > think---depends on a few further conditions) that if you have the
> > backlight driver as a module and the bootloader enables the backlight to
> > show a splash screen, the backlight goes off because of the
> > clk_disable_unused initcall.
> 
> I will have to verify, but I'm pretty sure disabling the clock doesn't
> change the pin level back to inactive.

Given that you set the clk's rate depending on the period to apply, I'd
claim that you need to keep the clk on. Maybe it doesn't hurt, because
another component of the system keeps the clk running, but it's wrong
anyhow. Assumptions like these tend to break on new chip revisions.

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 1/5] pwm: jz4740: Fix pin level of disabled TCU2 channels, part 1

[Paul Cercueil]

Hi Uwe,

Le jeu. 17 nov. 2022 à 14:29:27 +0100, Uwe Kleine-König 
<u.kleine-koenig@pengutronix.de> a écrit :
> Hello Paul,
> 
> On Tue, Oct 25, 2022 at 11:02:00AM +0100, Paul Cercueil wrote:
>>  Le mar. 25 oct. 2022 à 08:21:29 +0200, Uwe Kleine-König
>>  <u.kleine-koenig@pengutronix.de> a écrit :
>>  > Hello,
>>  >
>>  > On Mon, Oct 24, 2022 at 09:52:09PM +0100, Paul Cercueil wrote:
>>  > >  The "duty > cycle" trick to force the pin level of a disabled 
>> TCU2
>>  > >  channel would only work when the channel had been enabled
>>  > > previously.
>>  > >
>>  > >  Address this issue by enabling the PWM mode in 
>> jz4740_pwm_disable
>>  > >  (I know, right) so that the "duty > cycle" trick works before
>>  > > disabling
>>  > >  the PWM channel right after.
>>  > >
>>  > >  This issue went unnoticed, as the PWM pins on the majority of 
>> the
>>  > > boards
>>  > >  tested would default to the inactive level once the 
>> corresponding
>>  > > TCU
>>  > >  clock was enabled, so the first call to jz4740_pwm_disable() 
>> would
>>  > > not
>>  > >  actually change the pin levels.
>>  > >
>>  > >  On the GCW Zero however, the PWM pin for the backlight (PWM1, 
>> which
>>  > > is
>>  > >  a TCU2 channel) goes active as soon as the timer1 clock is 
>> enabled.
>>  > >  Since the jz4740_pwm_disable() function did not work on 
>> channels not
>>  > >  previously enabled, the backlight would shine at full 
>> brightness
>>  > > from
>>  > >  the moment the backlight driver would probe, until the 
>> backlight
>>  > > driver
>>  > >  tried to *enable* the PWM output.
>>  > >
>>  > >  With this fix, the PWM pins will be forced inactive as soon as
>>  > >  jz4740_pwm_apply() is called (and might be reconfigured to 
>> active if
>>  > >  dictated by the pwm_state). This means that there is still a 
>> tiny
>>  > > time
>>  > >  frame between the .request() and .apply() callbacks where the 
>> PWM
>>  > > pin
>>  > >  might be active. Sadly, there is no way to fix this issue: it 
>> is
>>  > >  impossible to write a PWM channel's registers if the 
>> corresponding
>>  > > clock
>>  > >  is not enabled, and enabling the clock is what causes the PWM 
>> pin
>>  > > to go
>>  > >  active.
>>  > >
>>  > >  There is a workaround, though, which complements this fix: 
>> simply
>>  > >  starting the backlight driver (or any PWM client driver) with a
>>  > > "init"
>>  > >  pinctrl state that sets the pin as an inactive GPIO. Once the
>>  > > driver is
>>  > >  probed and the pinctrl state switches to "default", the 
>> regular PWM
>>  > > pin
>>  > >  configuration can be used as it will be properly driven.
>>  > >
>>  > >  Fixes: c2693514a0a1 ("pwm: jz4740: Obtain regmap from parent 
>> node")
>>  > >  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  > >  Cc: stable@vger.kernel.org
>>  >
>>  > OK, understood the issue. I think there is another similar issue: 
>> The
>>  > clk is get and enabled only in the .request() callback. The 
>> result is (I
>>  > think---depends on a few further conditions) that if you have the
>>  > backlight driver as a module and the bootloader enables the 
>> backlight to
>>  > show a splash screen, the backlight goes off because of the
>>  > clk_disable_unused initcall.
>> 
>>  I will have to verify, but I'm pretty sure disabling the clock 
>> doesn't
>>  change the pin level back to inactive.
> 
> Given that you set the clk's rate depending on the period to apply, 
> I'd
> claim that you need to keep the clk on. Maybe it doesn't hurt, because
> another component of the system keeps the clk running, but it's wrong
> anyhow. Assumptions like these tend to break on new chip revisions.

If the backlight driver is a module then it will probe before the 
clk_disable_unused initcall, unless something is really wrong. So the 
backlight would stay ON if it was enabled by the bootloader, unless the 
DTB decides it doesn't have to be.

Anyway, I can try your suggestion, and move the trick to force-disable 
PWM pins in the probe(). After that, the clocks can be safely disabled, 
so I can disable them (for the disabled PWMs) at the end of the probe 
and re-enable them again in their respective .request() callback.

Cheers,
-Paul

Re: [PATCH 2/5] pwm: jz4740: Fix pin level of disabled TCU2 channels, part 2

[Uwe Kleine-König]

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

Hello,

On Tue, Oct 25, 2022 at 11:10:46AM +0100, Paul Cercueil wrote:
> Le mar. 25 oct. 2022 à 08:44:10 +0200, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> a écrit :
> > On Mon, Oct 24, 2022 at 09:52:10PM +0100, Paul Cercueil wrote:
> > >  After commit a020f22a4ff5 ("pwm: jz4740: Make PWM start with the
> > > active part"),
> > >  the trick to set duty > period to properly shut down TCU2 channels
> > > did
> > >  not work anymore, because of the polarity inversion.
> > > 
> > >  Address this issue by restoring the proper polarity before
> > > disabling the
> > >  channels.
> > > 
> > >  Fixes: a020f22a4ff5 ("pwm: jz4740: Make PWM start with the active
> > > part")
> > >  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> > >  Cc: stable@vger.kernel.org
> > >  ---
> > >   drivers/pwm/pwm-jz4740.c | 62
> > > ++++++++++++++++++++++++++--------------
> > >   1 file changed, 40 insertions(+), 22 deletions(-)
> > > 
> > >  diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
> > >  index 228eb104bf1e..65462a0052af 100644
> > >  --- a/drivers/pwm/pwm-jz4740.c
> > >  +++ b/drivers/pwm/pwm-jz4740.c
> > >  @@ -97,6 +97,19 @@ static int jz4740_pwm_enable(struct pwm_chip
> > > *chip, struct pwm_device *pwm)
> > >   	return 0;
> > >   }
> > > 
> > >  +static void jz4740_pwm_set_polarity(struct jz4740_pwm_chip *jz,
> > >  +				    unsigned int hwpwm,
> > >  +				    enum pwm_polarity polarity)
> > >  +{
> > >  +	unsigned int value = 0;
> > >  +
> > >  +	if (polarity == PWM_POLARITY_INVERSED)
> > >  +		value = TCU_TCSR_PWM_INITL_HIGH;
> > >  +
> > >  +	regmap_update_bits(jz->map, TCU_REG_TCSRc(hwpwm),
> > >  +			   TCU_TCSR_PWM_INITL_HIGH, value);
> > >  +}
> > >  +
> > >   static void jz4740_pwm_disable(struct pwm_chip *chip, struct
> > > pwm_device *pwm)
> > >   {
> > >   	struct jz4740_pwm_chip *jz = to_jz4740(chip);
> > >  @@ -130,6 +143,7 @@ static int jz4740_pwm_apply(struct pwm_chip
> > > *chip, struct pwm_device *pwm,
> > >   	unsigned long long tmp = 0xffffull * NSEC_PER_SEC;
> > >   	struct clk *clk = pwm_get_chip_data(pwm);
> > >   	unsigned long period, duty;
> > >  +	enum pwm_polarity polarity;
> > >   	long rate;
> > >   	int err;
> > > 
> > >  @@ -169,6 +183,9 @@ static int jz4740_pwm_apply(struct pwm_chip
> > > *chip, struct pwm_device *pwm,
> > >   	if (duty >= period)
> > >   		duty = period - 1;
> > > 
> > >  +	/* Restore regular polarity before disabling the channel. */
> > >  +	jz4740_pwm_set_polarity(jz4740, pwm->hwpwm, state->polarity);
> > >  +
> > 
> > Does this introduce a glitch?
> 
> Maybe. But the PWM is shut down before finishing its period anyway, so there
> was already a glitch.
> 
> > >   	jz4740_pwm_disable(chip, pwm);
> > > 
> > >   	err = clk_set_rate(clk, rate);
> > >  @@ -190,29 +207,30 @@ static int jz4740_pwm_apply(struct pwm_chip
> > > *chip, struct pwm_device *pwm,
> > >   	regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm),
> > >   			   TCU_TCSR_PWM_SD, TCU_TCSR_PWM_SD);
> > > 
> > >  -	/*
> > >  -	 * Set polarity.
> > >  -	 *
> > >  -	 * The PWM starts in inactive state until the internal timer
> > > reaches the
> > >  -	 * duty value, then becomes active until the timer reaches the
> > > period
> > >  -	 * value. In theory, we should then use (period - duty) as the
> > > real duty
> > >  -	 * value, as a high duty value would otherwise result in the PWM
> > > pin
> > >  -	 * being inactive most of the time.
> > >  -	 *
> > >  -	 * Here, we don't do that, and instead invert the polarity of the
> > > PWM
> > >  -	 * when it is active. This trick makes the PWM start with its
> > > active
> > >  -	 * state instead of its inactive state.
> > >  -	 */
> > >  -	if ((state->polarity == PWM_POLARITY_NORMAL) ^ state->enabled)
> > >  -		regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm),
> > >  -				   TCU_TCSR_PWM_INITL_HIGH, 0);
> > >  -	else
> > >  -		regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm),
> > >  -				   TCU_TCSR_PWM_INITL_HIGH,
> > >  -				   TCU_TCSR_PWM_INITL_HIGH);
> > >  -
> > >  -	if (state->enabled)
> > >  +	if (state->enabled) {
> > >  +		/*
> > >  +		 * Set polarity.
> > >  +		 *
> > >  +		 * The PWM starts in inactive state until the internal timer
> > >  +		 * reaches the duty value, then becomes active until the timer
> > >  +		 * reaches the period value. In theory, we should then use
> > >  +		 * (period - duty) as the real duty value, as a high duty value
> > >  +		 * would otherwise result in the PWM pin being inactive most of
> > >  +		 * the time.
> > >  +		 *
> > >  +		 * Here, we don't do that, and instead invert the polarity of
> > >  +		 * the PWM when it is active. This trick makes the PWM start
> > >  +		 * with its active state instead of its inactive state.
> > >  +		 */
> > >  +		if (state->polarity == PWM_POLARITY_NORMAL)
> > >  +			polarity = PWM_POLARITY_INVERSED;
> > >  +		else
> > >  +			polarity = PWM_POLARITY_NORMAL;
> > >  +
> > >  +		jz4740_pwm_set_polarity(jz4740, pwm->hwpwm, polarity);
> > >  +
> > >   		jz4740_pwm_enable(chip, pwm);
> > >  +	}
> > 
> > Note that for disabled PWMs there is no official guaranty about the pin
> > state. So it would be ok (but admittedly not great) to simplify the
> > driver and accept that the pinstate is active while the PWM is off.
> > IMHO this is also better than a glitch.
> > 
> > If a consumer wants the PWM to be in its inactive state, they should
> > not disable it.
> 
> Completely disagree. I absolutely do not want the backlight to go full
> bright mode when the PWM pin is disabled. And disabling the backlight is a
> thing (for screen blanking and during mode changes).

For some hardwares there is no pretty choice. So the gist is: If the
backlight driver wants to ensure that the PWM pin is driven to its
inactive level, it should use:

	pwm_apply(pwm, { .period = ..., .duty_cycle = 0, .enabled = true });

and better not

	pwm_apply(pwm, { ..., .enabled = false });

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 3/5] pwm: jz4740: Force dependency on Device Tree

[Uwe Kleine-König]

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

On Mon, Oct 24, 2022 at 09:52:11PM +0100, Paul Cercueil wrote:
> Ingenic SoCs all require CONFIG_OF, so there is no case where we want to
> use this driver without CONFIG_OF.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>

> ---
>  drivers/pwm/Kconfig      |  2 +-
>  drivers/pwm/pwm-jz4740.c | 10 ++++------
>  2 files changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 60d13a949bc5..1fe420a45f91 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -283,7 +283,7 @@ config PWM_IQS620A
>  config PWM_JZ4740
>  	tristate "Ingenic JZ47xx PWM support"
>  	depends on MIPS || COMPILE_TEST
> -	depends on COMMON_CLK
> +	depends on COMMON_CLK && OF

I think you don't even need to enforce OF here. For the compile-test
case having OF=n should work fine.

Anyhow:

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 2/5] pwm: jz4740: Fix pin level of disabled TCU2 channels, part 2

[Thierry Reding]

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

On Mon, Nov 28, 2022 at 03:39:11PM +0100, Uwe Kleine-König wrote:
> Hello,
> 
> On Tue, Oct 25, 2022 at 11:10:46AM +0100, Paul Cercueil wrote:
> > Le mar. 25 oct. 2022 à 08:44:10 +0200, Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> a écrit :
> > > On Mon, Oct 24, 2022 at 09:52:10PM +0100, Paul Cercueil wrote:
> > > >  After commit a020f22a4ff5 ("pwm: jz4740: Make PWM start with the
> > > > active part"),
> > > >  the trick to set duty > period to properly shut down TCU2 channels
> > > > did
> > > >  not work anymore, because of the polarity inversion.
> > > > 
> > > >  Address this issue by restoring the proper polarity before
> > > > disabling the
> > > >  channels.
> > > > 
> > > >  Fixes: a020f22a4ff5 ("pwm: jz4740: Make PWM start with the active
> > > > part")
> > > >  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> > > >  Cc: stable@vger.kernel.org
> > > >  ---
> > > >   drivers/pwm/pwm-jz4740.c | 62
> > > > ++++++++++++++++++++++++++--------------
> > > >   1 file changed, 40 insertions(+), 22 deletions(-)
> > > > 
> > > >  diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
> > > >  index 228eb104bf1e..65462a0052af 100644
> > > >  --- a/drivers/pwm/pwm-jz4740.c
> > > >  +++ b/drivers/pwm/pwm-jz4740.c
> > > >  @@ -97,6 +97,19 @@ static int jz4740_pwm_enable(struct pwm_chip
> > > > *chip, struct pwm_device *pwm)
> > > >   	return 0;
> > > >   }
> > > > 
> > > >  +static void jz4740_pwm_set_polarity(struct jz4740_pwm_chip *jz,
> > > >  +				    unsigned int hwpwm,
> > > >  +				    enum pwm_polarity polarity)
> > > >  +{
> > > >  +	unsigned int value = 0;
> > > >  +
> > > >  +	if (polarity == PWM_POLARITY_INVERSED)
> > > >  +		value = TCU_TCSR_PWM_INITL_HIGH;
> > > >  +
> > > >  +	regmap_update_bits(jz->map, TCU_REG_TCSRc(hwpwm),
> > > >  +			   TCU_TCSR_PWM_INITL_HIGH, value);
> > > >  +}
> > > >  +
> > > >   static void jz4740_pwm_disable(struct pwm_chip *chip, struct
> > > > pwm_device *pwm)
> > > >   {
> > > >   	struct jz4740_pwm_chip *jz = to_jz4740(chip);
> > > >  @@ -130,6 +143,7 @@ static int jz4740_pwm_apply(struct pwm_chip
> > > > *chip, struct pwm_device *pwm,
> > > >   	unsigned long long tmp = 0xffffull * NSEC_PER_SEC;
> > > >   	struct clk *clk = pwm_get_chip_data(pwm);
> > > >   	unsigned long period, duty;
> > > >  +	enum pwm_polarity polarity;
> > > >   	long rate;
> > > >   	int err;
> > > > 
> > > >  @@ -169,6 +183,9 @@ static int jz4740_pwm_apply(struct pwm_chip
> > > > *chip, struct pwm_device *pwm,
> > > >   	if (duty >= period)
> > > >   		duty = period - 1;
> > > > 
> > > >  +	/* Restore regular polarity before disabling the channel. */
> > > >  +	jz4740_pwm_set_polarity(jz4740, pwm->hwpwm, state->polarity);
> > > >  +
> > > 
> > > Does this introduce a glitch?
> > 
> > Maybe. But the PWM is shut down before finishing its period anyway, so there
> > was already a glitch.
> > 
> > > >   	jz4740_pwm_disable(chip, pwm);
> > > > 
> > > >   	err = clk_set_rate(clk, rate);
> > > >  @@ -190,29 +207,30 @@ static int jz4740_pwm_apply(struct pwm_chip
> > > > *chip, struct pwm_device *pwm,
> > > >   	regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm),
> > > >   			   TCU_TCSR_PWM_SD, TCU_TCSR_PWM_SD);
> > > > 
> > > >  -	/*
> > > >  -	 * Set polarity.
> > > >  -	 *
> > > >  -	 * The PWM starts in inactive state until the internal timer
> > > > reaches the
> > > >  -	 * duty value, then becomes active until the timer reaches the
> > > > period
> > > >  -	 * value. In theory, we should then use (period - duty) as the
> > > > real duty
> > > >  -	 * value, as a high duty value would otherwise result in the PWM
> > > > pin
> > > >  -	 * being inactive most of the time.
> > > >  -	 *
> > > >  -	 * Here, we don't do that, and instead invert the polarity of the
> > > > PWM
> > > >  -	 * when it is active. This trick makes the PWM start with its
> > > > active
> > > >  -	 * state instead of its inactive state.
> > > >  -	 */
> > > >  -	if ((state->polarity == PWM_POLARITY_NORMAL) ^ state->enabled)
> > > >  -		regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm),
> > > >  -				   TCU_TCSR_PWM_INITL_HIGH, 0);
> > > >  -	else
> > > >  -		regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm),
> > > >  -				   TCU_TCSR_PWM_INITL_HIGH,
> > > >  -				   TCU_TCSR_PWM_INITL_HIGH);
> > > >  -
> > > >  -	if (state->enabled)
> > > >  +	if (state->enabled) {
> > > >  +		/*
> > > >  +		 * Set polarity.
> > > >  +		 *
> > > >  +		 * The PWM starts in inactive state until the internal timer
> > > >  +		 * reaches the duty value, then becomes active until the timer
> > > >  +		 * reaches the period value. In theory, we should then use
> > > >  +		 * (period - duty) as the real duty value, as a high duty value
> > > >  +		 * would otherwise result in the PWM pin being inactive most of
> > > >  +		 * the time.
> > > >  +		 *
> > > >  +		 * Here, we don't do that, and instead invert the polarity of
> > > >  +		 * the PWM when it is active. This trick makes the PWM start
> > > >  +		 * with its active state instead of its inactive state.
> > > >  +		 */
> > > >  +		if (state->polarity == PWM_POLARITY_NORMAL)
> > > >  +			polarity = PWM_POLARITY_INVERSED;
> > > >  +		else
> > > >  +			polarity = PWM_POLARITY_NORMAL;
> > > >  +
> > > >  +		jz4740_pwm_set_polarity(jz4740, pwm->hwpwm, polarity);
> > > >  +
> > > >   		jz4740_pwm_enable(chip, pwm);
> > > >  +	}
> > > 
> > > Note that for disabled PWMs there is no official guaranty about the pin
> > > state. So it would be ok (but admittedly not great) to simplify the
> > > driver and accept that the pinstate is active while the PWM is off.
> > > IMHO this is also better than a glitch.
> > > 
> > > If a consumer wants the PWM to be in its inactive state, they should
> > > not disable it.
> > 
> > Completely disagree. I absolutely do not want the backlight to go full
> > bright mode when the PWM pin is disabled. And disabling the backlight is a
> > thing (for screen blanking and during mode changes).
> 
> For some hardwares there is no pretty choice. So the gist is: If the
> backlight driver wants to ensure that the PWM pin is driven to its
> inactive level, it should use:
> 
> 	pwm_apply(pwm, { .period = ..., .duty_cycle = 0, .enabled = true });
> 
> and better not
> 
> 	pwm_apply(pwm, { ..., .enabled = false });

Depending on your hardware capabilities you may also be able to use
pinctrl to configure the pin to behave properly when the PWM is
disabled. Not all hardware can do that, though.

Thierry

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

Re: [PATCH 2/5] pwm: jz4740: Fix pin level of disabled TCU2 channels, part 2

[Paul Cercueil]

Hi Uwe,

Le lun. 28 nov. 2022 à 15:39:11 +0100, Uwe Kleine-König 
<u.kleine-koenig@pengutronix.de> a écrit :
> Hello,
> 
> On Tue, Oct 25, 2022 at 11:10:46AM +0100, Paul Cercueil wrote:
>>  Le mar. 25 oct. 2022 à 08:44:10 +0200, Uwe Kleine-König
>>  <u.kleine-koenig@pengutronix.de> a écrit :
>>  > On Mon, Oct 24, 2022 at 09:52:10PM +0100, Paul Cercueil wrote:
>>  > >  After commit a020f22a4ff5 ("pwm: jz4740: Make PWM start with 
>> the
>>  > > active part"),
>>  > >  the trick to set duty > period to properly shut down TCU2 
>> channels
>>  > > did
>>  > >  not work anymore, because of the polarity inversion.
>>  > >
>>  > >  Address this issue by restoring the proper polarity before
>>  > > disabling the
>>  > >  channels.
>>  > >
>>  > >  Fixes: a020f22a4ff5 ("pwm: jz4740: Make PWM start with the 
>> active
>>  > > part")
>>  > >  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  > >  Cc: stable@vger.kernel.org
>>  > >  ---
>>  > >   drivers/pwm/pwm-jz4740.c | 62
>>  > > ++++++++++++++++++++++++++--------------
>>  > >   1 file changed, 40 insertions(+), 22 deletions(-)
>>  > >
>>  > >  diff --git a/drivers/pwm/pwm-jz4740.c 
>> b/drivers/pwm/pwm-jz4740.c
>>  > >  index 228eb104bf1e..65462a0052af 100644
>>  > >  --- a/drivers/pwm/pwm-jz4740.c
>>  > >  +++ b/drivers/pwm/pwm-jz4740.c
>>  > >  @@ -97,6 +97,19 @@ static int jz4740_pwm_enable(struct pwm_chip
>>  > > *chip, struct pwm_device *pwm)
>>  > >   	return 0;
>>  > >   }
>>  > >
>>  > >  +static void jz4740_pwm_set_polarity(struct jz4740_pwm_chip 
>> *jz,
>>  > >  +				    unsigned int hwpwm,
>>  > >  +				    enum pwm_polarity polarity)
>>  > >  +{
>>  > >  +	unsigned int value = 0;
>>  > >  +
>>  > >  +	if (polarity == PWM_POLARITY_INVERSED)
>>  > >  +		value = TCU_TCSR_PWM_INITL_HIGH;
>>  > >  +
>>  > >  +	regmap_update_bits(jz->map, TCU_REG_TCSRc(hwpwm),
>>  > >  +			   TCU_TCSR_PWM_INITL_HIGH, value);
>>  > >  +}
>>  > >  +
>>  > >   static void jz4740_pwm_disable(struct pwm_chip *chip, struct
>>  > > pwm_device *pwm)
>>  > >   {
>>  > >   	struct jz4740_pwm_chip *jz = to_jz4740(chip);
>>  > >  @@ -130,6 +143,7 @@ static int jz4740_pwm_apply(struct pwm_chip
>>  > > *chip, struct pwm_device *pwm,
>>  > >   	unsigned long long tmp = 0xffffull * NSEC_PER_SEC;
>>  > >   	struct clk *clk = pwm_get_chip_data(pwm);
>>  > >   	unsigned long period, duty;
>>  > >  +	enum pwm_polarity polarity;
>>  > >   	long rate;
>>  > >   	int err;
>>  > >
>>  > >  @@ -169,6 +183,9 @@ static int jz4740_pwm_apply(struct pwm_chip
>>  > > *chip, struct pwm_device *pwm,
>>  > >   	if (duty >= period)
>>  > >   		duty = period - 1;
>>  > >
>>  > >  +	/* Restore regular polarity before disabling the channel. */
>>  > >  +	jz4740_pwm_set_polarity(jz4740, pwm->hwpwm, state->polarity);
>>  > >  +
>>  >
>>  > Does this introduce a glitch?
>> 
>>  Maybe. But the PWM is shut down before finishing its period anyway, 
>> so there
>>  was already a glitch.
>> 
>>  > >   	jz4740_pwm_disable(chip, pwm);
>>  > >
>>  > >   	err = clk_set_rate(clk, rate);
>>  > >  @@ -190,29 +207,30 @@ static int jz4740_pwm_apply(struct 
>> pwm_chip
>>  > > *chip, struct pwm_device *pwm,
>>  > >   	regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm),
>>  > >   			   TCU_TCSR_PWM_SD, TCU_TCSR_PWM_SD);
>>  > >
>>  > >  -	/*
>>  > >  -	 * Set polarity.
>>  > >  -	 *
>>  > >  -	 * The PWM starts in inactive state until the internal timer
>>  > > reaches the
>>  > >  -	 * duty value, then becomes active until the timer reaches 
>> the
>>  > > period
>>  > >  -	 * value. In theory, we should then use (period - duty) as 
>> the
>>  > > real duty
>>  > >  -	 * value, as a high duty value would otherwise result in the 
>> PWM
>>  > > pin
>>  > >  -	 * being inactive most of the time.
>>  > >  -	 *
>>  > >  -	 * Here, we don't do that, and instead invert the polarity 
>> of the
>>  > > PWM
>>  > >  -	 * when it is active. This trick makes the PWM start with its
>>  > > active
>>  > >  -	 * state instead of its inactive state.
>>  > >  -	 */
>>  > >  -	if ((state->polarity == PWM_POLARITY_NORMAL) ^ 
>> state->enabled)
>>  > >  -		regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm),
>>  > >  -				   TCU_TCSR_PWM_INITL_HIGH, 0);
>>  > >  -	else
>>  > >  -		regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm),
>>  > >  -				   TCU_TCSR_PWM_INITL_HIGH,
>>  > >  -				   TCU_TCSR_PWM_INITL_HIGH);
>>  > >  -
>>  > >  -	if (state->enabled)
>>  > >  +	if (state->enabled) {
>>  > >  +		/*
>>  > >  +		 * Set polarity.
>>  > >  +		 *
>>  > >  +		 * The PWM starts in inactive state until the internal timer
>>  > >  +		 * reaches the duty value, then becomes active until the 
>> timer
>>  > >  +		 * reaches the period value. In theory, we should then use
>>  > >  +		 * (period - duty) as the real duty value, as a high duty 
>> value
>>  > >  +		 * would otherwise result in the PWM pin being inactive 
>> most of
>>  > >  +		 * the time.
>>  > >  +		 *
>>  > >  +		 * Here, we don't do that, and instead invert the polarity 
>> of
>>  > >  +		 * the PWM when it is active. This trick makes the PWM start
>>  > >  +		 * with its active state instead of its inactive state.
>>  > >  +		 */
>>  > >  +		if (state->polarity == PWM_POLARITY_NORMAL)
>>  > >  +			polarity = PWM_POLARITY_INVERSED;
>>  > >  +		else
>>  > >  +			polarity = PWM_POLARITY_NORMAL;
>>  > >  +
>>  > >  +		jz4740_pwm_set_polarity(jz4740, pwm->hwpwm, polarity);
>>  > >  +
>>  > >   		jz4740_pwm_enable(chip, pwm);
>>  > >  +	}
>>  >
>>  > Note that for disabled PWMs there is no official guaranty about 
>> the pin
>>  > state. So it would be ok (but admittedly not great) to simplify 
>> the
>>  > driver and accept that the pinstate is active while the PWM is 
>> off.
>>  > IMHO this is also better than a glitch.
>>  >
>>  > If a consumer wants the PWM to be in its inactive state, they 
>> should
>>  > not disable it.
>> 
>>  Completely disagree. I absolutely do not want the backlight to go 
>> full
>>  bright mode when the PWM pin is disabled. And disabling the 
>> backlight is a
>>  thing (for screen blanking and during mode changes).
> 
> For some hardwares there is no pretty choice. So the gist is: If the
> backlight driver wants to ensure that the PWM pin is driven to its
> inactive level, it should use:
> 
> 	pwm_apply(pwm, { .period = ..., .duty_cycle = 0, .enabled = true });
> 
> and better not
> 
> 	pwm_apply(pwm, { ..., .enabled = false });

Well that sounds pretty stupid to me; why doesn't the PWM subsystem 
enforce that the pins must be driven to their inactive level when the 
PWM function is disabled? Then for such hardware you describe, the 
corresponding PWM driver could itself apply a duty_cycle = 0 if that's 
what it takes to get an inactive state.

Cheers,
-Paul

Re: [PATCH 2/5] pwm: jz4740: Fix pin level of disabled TCU2 channels, part 2

[Paul Cercueil]

Hi Thierry,

Le mar. 29 nov. 2022 à 13:16:05 +0100, Thierry Reding 
<thierry.reding@gmail.com> a écrit :
> On Mon, Nov 28, 2022 at 03:39:11PM +0100, Uwe Kleine-König wrote:
>>  Hello,
>> 
>>  On Tue, Oct 25, 2022 at 11:10:46AM +0100, Paul Cercueil wrote:
>>  > Le mar. 25 oct. 2022 à 08:44:10 +0200, Uwe Kleine-König
>>  > <u.kleine-koenig@pengutronix.de> a écrit :
>>  > > On Mon, Oct 24, 2022 at 09:52:10PM +0100, Paul Cercueil wrote:
>>  > > >  After commit a020f22a4ff5 ("pwm: jz4740: Make PWM start with 
>> the
>>  > > > active part"),
>>  > > >  the trick to set duty > period to properly shut down TCU2 
>> channels
>>  > > > did
>>  > > >  not work anymore, because of the polarity inversion.
>>  > > >
>>  > > >  Address this issue by restoring the proper polarity before
>>  > > > disabling the
>>  > > >  channels.
>>  > > >
>>  > > >  Fixes: a020f22a4ff5 ("pwm: jz4740: Make PWM start with the 
>> active
>>  > > > part")
>>  > > >  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  > > >  Cc: stable@vger.kernel.org
>>  > > >  ---
>>  > > >   drivers/pwm/pwm-jz4740.c | 62
>>  > > > ++++++++++++++++++++++++++--------------
>>  > > >   1 file changed, 40 insertions(+), 22 deletions(-)
>>  > > >
>>  > > >  diff --git a/drivers/pwm/pwm-jz4740.c 
>> b/drivers/pwm/pwm-jz4740.c
>>  > > >  index 228eb104bf1e..65462a0052af 100644
>>  > > >  --- a/drivers/pwm/pwm-jz4740.c
>>  > > >  +++ b/drivers/pwm/pwm-jz4740.c
>>  > > >  @@ -97,6 +97,19 @@ static int jz4740_pwm_enable(struct 
>> pwm_chip
>>  > > > *chip, struct pwm_device *pwm)
>>  > > >   	return 0;
>>  > > >   }
>>  > > >
>>  > > >  +static void jz4740_pwm_set_polarity(struct jz4740_pwm_chip 
>> *jz,
>>  > > >  +				    unsigned int hwpwm,
>>  > > >  +				    enum pwm_polarity polarity)
>>  > > >  +{
>>  > > >  +	unsigned int value = 0;
>>  > > >  +
>>  > > >  +	if (polarity == PWM_POLARITY_INVERSED)
>>  > > >  +		value = TCU_TCSR_PWM_INITL_HIGH;
>>  > > >  +
>>  > > >  +	regmap_update_bits(jz->map, TCU_REG_TCSRc(hwpwm),
>>  > > >  +			   TCU_TCSR_PWM_INITL_HIGH, value);
>>  > > >  +}
>>  > > >  +
>>  > > >   static void jz4740_pwm_disable(struct pwm_chip *chip, struct
>>  > > > pwm_device *pwm)
>>  > > >   {
>>  > > >   	struct jz4740_pwm_chip *jz = to_jz4740(chip);
>>  > > >  @@ -130,6 +143,7 @@ static int jz4740_pwm_apply(struct 
>> pwm_chip
>>  > > > *chip, struct pwm_device *pwm,
>>  > > >   	unsigned long long tmp = 0xffffull * NSEC_PER_SEC;
>>  > > >   	struct clk *clk = pwm_get_chip_data(pwm);
>>  > > >   	unsigned long period, duty;
>>  > > >  +	enum pwm_polarity polarity;
>>  > > >   	long rate;
>>  > > >   	int err;
>>  > > >
>>  > > >  @@ -169,6 +183,9 @@ static int jz4740_pwm_apply(struct 
>> pwm_chip
>>  > > > *chip, struct pwm_device *pwm,
>>  > > >   	if (duty >= period)
>>  > > >   		duty = period - 1;
>>  > > >
>>  > > >  +	/* Restore regular polarity before disabling the channel. 
>> */
>>  > > >  +	jz4740_pwm_set_polarity(jz4740, pwm->hwpwm, 
>> state->polarity);
>>  > > >  +
>>  > >
>>  > > Does this introduce a glitch?
>>  >
>>  > Maybe. But the PWM is shut down before finishing its period 
>> anyway, so there
>>  > was already a glitch.
>>  >
>>  > > >   	jz4740_pwm_disable(chip, pwm);
>>  > > >
>>  > > >   	err = clk_set_rate(clk, rate);
>>  > > >  @@ -190,29 +207,30 @@ static int jz4740_pwm_apply(struct 
>> pwm_chip
>>  > > > *chip, struct pwm_device *pwm,
>>  > > >   	regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm),
>>  > > >   			   TCU_TCSR_PWM_SD, TCU_TCSR_PWM_SD);
>>  > > >
>>  > > >  -	/*
>>  > > >  -	 * Set polarity.
>>  > > >  -	 *
>>  > > >  -	 * The PWM starts in inactive state until the internal 
>> timer
>>  > > > reaches the
>>  > > >  -	 * duty value, then becomes active until the timer reaches 
>> the
>>  > > > period
>>  > > >  -	 * value. In theory, we should then use (period - duty) as 
>> the
>>  > > > real duty
>>  > > >  -	 * value, as a high duty value would otherwise result in 
>> the PWM
>>  > > > pin
>>  > > >  -	 * being inactive most of the time.
>>  > > >  -	 *
>>  > > >  -	 * Here, we don't do that, and instead invert the polarity 
>> of the
>>  > > > PWM
>>  > > >  -	 * when it is active. This trick makes the PWM start with 
>> its
>>  > > > active
>>  > > >  -	 * state instead of its inactive state.
>>  > > >  -	 */
>>  > > >  -	if ((state->polarity == PWM_POLARITY_NORMAL) ^ 
>> state->enabled)
>>  > > >  -		regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm),
>>  > > >  -				   TCU_TCSR_PWM_INITL_HIGH, 0);
>>  > > >  -	else
>>  > > >  -		regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm),
>>  > > >  -				   TCU_TCSR_PWM_INITL_HIGH,
>>  > > >  -				   TCU_TCSR_PWM_INITL_HIGH);
>>  > > >  -
>>  > > >  -	if (state->enabled)
>>  > > >  +	if (state->enabled) {
>>  > > >  +		/*
>>  > > >  +		 * Set polarity.
>>  > > >  +		 *
>>  > > >  +		 * The PWM starts in inactive state until the internal 
>> timer
>>  > > >  +		 * reaches the duty value, then becomes active until the 
>> timer
>>  > > >  +		 * reaches the period value. In theory, we should then use
>>  > > >  +		 * (period - duty) as the real duty value, as a high duty 
>> value
>>  > > >  +		 * would otherwise result in the PWM pin being inactive 
>> most of
>>  > > >  +		 * the time.
>>  > > >  +		 *
>>  > > >  +		 * Here, we don't do that, and instead invert the 
>> polarity of
>>  > > >  +		 * the PWM when it is active. This trick makes the PWM 
>> start
>>  > > >  +		 * with its active state instead of its inactive state.
>>  > > >  +		 */
>>  > > >  +		if (state->polarity == PWM_POLARITY_NORMAL)
>>  > > >  +			polarity = PWM_POLARITY_INVERSED;
>>  > > >  +		else
>>  > > >  +			polarity = PWM_POLARITY_NORMAL;
>>  > > >  +
>>  > > >  +		jz4740_pwm_set_polarity(jz4740, pwm->hwpwm, polarity);
>>  > > >  +
>>  > > >   		jz4740_pwm_enable(chip, pwm);
>>  > > >  +	}
>>  > >
>>  > > Note that for disabled PWMs there is no official guaranty about 
>> the pin
>>  > > state. So it would be ok (but admittedly not great) to simplify 
>> the
>>  > > driver and accept that the pinstate is active while the PWM is 
>> off.
>>  > > IMHO this is also better than a glitch.
>>  > >
>>  > > If a consumer wants the PWM to be in its inactive state, they 
>> should
>>  > > not disable it.
>>  >
>>  > Completely disagree. I absolutely do not want the backlight to go 
>> full
>>  > bright mode when the PWM pin is disabled. And disabling the 
>> backlight is a
>>  > thing (for screen blanking and during mode changes).
>> 
>>  For some hardwares there is no pretty choice. So the gist is: If the
>>  backlight driver wants to ensure that the PWM pin is driven to its
>>  inactive level, it should use:
>> 
>>  	pwm_apply(pwm, { .period = ..., .duty_cycle = 0, .enabled = true 
>> });
>> 
>>  and better not
>> 
>>  	pwm_apply(pwm, { ..., .enabled = false });
> 
> Depending on your hardware capabilities you may also be able to use
> pinctrl to configure the pin to behave properly when the PWM is
> disabled. Not all hardware can do that, though.

Been there, done that. It got refused.
https://lkml.org/lkml/2019/5/22/607

Cheers,
-Paul

Re: [PATCH 2/5] pwm: jz4740: Fix pin level of disabled TCU2 channels, part 2

[Uwe Kleine-König]

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

Hello Paul,

On Tue, Nov 29, 2022 at 12:25:56PM +0000, Paul Cercueil wrote:
> Hi Uwe,
> 
> Le lun. 28 nov. 2022 à 15:39:11 +0100, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> a écrit :
> > Hello,
> > 
> > On Tue, Oct 25, 2022 at 11:10:46AM +0100, Paul Cercueil wrote:
> > > > Note that for disabled PWMs there is no official guaranty about the pin
> > > > state. So it would be ok (but admittedly not great) to simplify the
> > > > driver and accept that the pinstate is active while the PWM is off.
> > > > IMHO this is also better than a glitch.
> > > >
> > > > If a consumer wants the PWM to be in its inactive state, they should
> > > > not disable it.
> > > 
> > > Completely disagree. I absolutely do not want the backlight to go full
> > > bright mode when the PWM pin is disabled. And disabling the backlight is a
> > > thing (for screen blanking and during mode changes).
> > 
> > For some hardwares there is no pretty choice. So the gist is: If the
> > backlight driver wants to ensure that the PWM pin is driven to its
> > inactive level, it should use:
> > 
> > 	pwm_apply(pwm, { .period = ..., .duty_cycle = 0, .enabled = true });
> > 
> > and better not
> > 
> > 	pwm_apply(pwm, { ..., .enabled = false });
> 
> Well that sounds pretty stupid to me; why doesn't the PWM subsystem enforce
> that the pins must be driven to their inactive level when the PWM function
> is disabled?
> 
> Then for such hardware you describe, the corresponding PWM
> driver could itself apply a duty_cycle = 0 if that's what it takes to get an
> inactive state.

Let's assume we claim that on disable the pin is driven to the inactive level.

The (bad) effect is that for a use case where the pin state doesn't
matter (e.g. a backlight where the power regulator is off), the PWM
keeps running even though it could be disabled and so save some power.

So to make this use case properly supported, we need another flag in
struct pwm_state that allows the consumer to tell the lowlevel driver
that it's ok to disable the hardware even with the output being UB.
Let's call this new flag "spam" and the pin is allowed to do whatever it
wants with .spam = false.

After that you can realize that applying any state with:

	.duty_cycle = A,
	.period = B,
	.polarity = C,
	.enabled = false,
	.spam = true,

semantically (i.e. just looking at the output) has the same effect as

	.duty_cycle = 0,
	.period = $something,
	.polarity = C,
	.enabled = true,
	.spam = true,

So having .enabled doesn't add to the expressiveness of pwm_apply(),
because you can specify any configuration without having to resort to
.enabled = false. So the enabled member of struct pwm_state can be
dropped.

Then we end up with the exact scenario we have now, just that the flag
that specifies if the output should be held in the inactive state has a
bad name.

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 2/5] pwm: jz4740: Fix pin level of disabled TCU2 channels, part 2

[Paul Cercueil]

Le mardi 29 novembre 2022 à 17:24 +0100, Uwe Kleine-König a écrit :
> Hello Paul,
> 
> On Tue, Nov 29, 2022 at 12:25:56PM +0000, Paul Cercueil wrote:
> > Hi Uwe,
> > 
> > Le lun. 28 nov. 2022 à 15:39:11 +0100, Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> a écrit :
> > > Hello,
> > > 
> > > On Tue, Oct 25, 2022 at 11:10:46AM +0100, Paul Cercueil wrote:
> > > > > Note that for disabled PWMs there is no official guaranty
> > > > > about the pin
> > > > > state. So it would be ok (but admittedly not great) to
> > > > > simplify the
> > > > > driver and accept that the pinstate is active while the PWM
> > > > > is off.
> > > > > IMHO this is also better than a glitch.
> > > > > 
> > > > > If a consumer wants the PWM to be in its inactive state, they
> > > > > should
> > > > > not disable it.
> > > > 
> > > > Completely disagree. I absolutely do not want the backlight to
> > > > go full
> > > > bright mode when the PWM pin is disabled. And disabling the
> > > > backlight is a
> > > > thing (for screen blanking and during mode changes).
> > > 
> > > For some hardwares there is no pretty choice. So the gist is: If
> > > the
> > > backlight driver wants to ensure that the PWM pin is driven to
> > > its
> > > inactive level, it should use:
> > > 
> > >         pwm_apply(pwm, { .period = ..., .duty_cycle = 0, .enabled
> > > = true });
> > > 
> > > and better not
> > > 
> > >         pwm_apply(pwm, { ..., .enabled = false });
> > 
> > Well that sounds pretty stupid to me; why doesn't the PWM subsystem
> > enforce
> > that the pins must be driven to their inactive level when the PWM
> > function
> > is disabled?
> > 
> > Then for such hardware you describe, the corresponding PWM
> > driver could itself apply a duty_cycle = 0 if that's what it takes
> > to get an
> > inactive state.
> 
> Let's assume we claim that on disable the pin is driven to the
> inactive level.
> 
> The (bad) effect is that for a use case where the pin state doesn't
> matter (e.g. a backlight where the power regulator is off), the PWM
> keeps running even though it could be disabled and so save some
> power.
> 
> So to make this use case properly supported, we need another flag in
> struct pwm_state that allows the consumer to tell the lowlevel driver
> that it's ok to disable the hardware even with the output being UB.
> Let's call this new flag "spam" and the pin is allowed to do whatever
> it
> wants with .spam = false.
> 
> After that you can realize that applying any state with:
> 
>         .duty_cycle = A,
>         .period = B,
>         .polarity = C,
>         .enabled = false,
>         .spam = true,
> 
> semantically (i.e. just looking at the output) has the same effect as
> 
>         .duty_cycle = 0,
>         .period = $something,
>         .polarity = C,
>         .enabled = true,
>         .spam = true,
> 
> So having .enabled doesn't add to the expressiveness of pwm_apply(),
> because you can specify any configuration without having to resort to
> .enabled = false. So the enabled member of struct pwm_state can be
> dropped.
> 
> Then we end up with the exact scenario we have now, just that the
> flag
> that specifies if the output should be held in the inactive state has
> a
> bad name.

If I follow you, then it means that the PWM backlight driver pwm_bl.c
should set state.enabled=true in pwm_backlight_power_off() to make sure
that the pin is inactive?

-Paul

Re: [PATCH 2/5] pwm: jz4740: Fix pin level of disabled TCU2 channels, part 2

[Uwe Kleine-König]

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

On Tue, Nov 29, 2022 at 04:58:28PM +0000, Paul Cercueil wrote:
> Le mardi 29 novembre 2022 à 17:24 +0100, Uwe Kleine-König a écrit :
> > Hello Paul,
> > 
> > On Tue, Nov 29, 2022 at 12:25:56PM +0000, Paul Cercueil wrote:
> > > Hi Uwe,
> > > 
> > > Le lun. 28 nov. 2022 à 15:39:11 +0100, Uwe Kleine-König
> > > <u.kleine-koenig@pengutronix.de> a écrit :
> > > > Hello,
> > > > 
> > > > On Tue, Oct 25, 2022 at 11:10:46AM +0100, Paul Cercueil wrote:
> > > > > > Note that for disabled PWMs there is no official guaranty
> > > > > > about the pin
> > > > > > state. So it would be ok (but admittedly not great) to
> > > > > > simplify the
> > > > > > driver and accept that the pinstate is active while the PWM
> > > > > > is off.
> > > > > > IMHO this is also better than a glitch.
> > > > > > 
> > > > > > If a consumer wants the PWM to be in its inactive state, they
> > > > > > should
> > > > > > not disable it.
> > > > > 
> > > > > Completely disagree. I absolutely do not want the backlight to
> > > > > go full
> > > > > bright mode when the PWM pin is disabled. And disabling the
> > > > > backlight is a
> > > > > thing (for screen blanking and during mode changes).
> > > > 
> > > > For some hardwares there is no pretty choice. So the gist is: If
> > > > the
> > > > backlight driver wants to ensure that the PWM pin is driven to
> > > > its
> > > > inactive level, it should use:
> > > > 
> > > >         pwm_apply(pwm, { .period = ..., .duty_cycle = 0, .enabled
> > > > = true });
> > > > 
> > > > and better not
> > > > 
> > > >         pwm_apply(pwm, { ..., .enabled = false });
> > > 
> > > Well that sounds pretty stupid to me; why doesn't the PWM subsystem
> > > enforce
> > > that the pins must be driven to their inactive level when the PWM
> > > function
> > > is disabled?
> > > 
> > > Then for such hardware you describe, the corresponding PWM
> > > driver could itself apply a duty_cycle = 0 if that's what it takes
> > > to get an
> > > inactive state.
> > 
> > Let's assume we claim that on disable the pin is driven to the
> > inactive level.
> > 
> > The (bad) effect is that for a use case where the pin state doesn't
> > matter (e.g. a backlight where the power regulator is off), the PWM
> > keeps running even though it could be disabled and so save some
> > power.
> > 
> > So to make this use case properly supported, we need another flag in
> > struct pwm_state that allows the consumer to tell the lowlevel driver
> > that it's ok to disable the hardware even with the output being UB.
> > Let's call this new flag "spam" and the pin is allowed to do whatever
> > it
> > wants with .spam = false.
> > 
> > After that you can realize that applying any state with:
> > 
> >         .duty_cycle = A,
> >         .period = B,
> >         .polarity = C,
> >         .enabled = false,
> >         .spam = true,
> > 
> > semantically (i.e. just looking at the output) has the same effect as
> > 
> >         .duty_cycle = 0,
> >         .period = $something,
> >         .polarity = C,
> >         .enabled = true,
> >         .spam = true,
> > 
> > So having .enabled doesn't add to the expressiveness of pwm_apply(),
> > because you can specify any configuration without having to resort to
> > .enabled = false. So the enabled member of struct pwm_state can be
> > dropped.
> > 
> > Then we end up with the exact scenario we have now, just that the
> > flag
> > that specifies if the output should be held in the inactive state has
> > a
> > bad name.
> 
> If I follow you, then it means that the PWM backlight driver pwm_bl.c
> should set state.enabled=true in pwm_backlight_power_off() to make sure
> that the pin is inactive?

Correct, that's the only way to ensure that the pinlevel stays at the
intended level.

And lowlevel PWM drivers can be improved to disable the hardware when
they are asked for .duty_cycle = 0 (maybe under some additional
conditions).

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 1/5] pwm: jz4740: Fix pin level of disabled TCU2 channels, part 1

[Uwe Kleine-König]

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

Hello Paul,

On Fri, Nov 18, 2022 at 09:55:40AM +0000, Paul Cercueil wrote:
> Le jeu. 17 nov. 2022 à 14:29:27 +0100, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> a écrit :
> > Hello Paul,
> > 
> > On Tue, Oct 25, 2022 at 11:02:00AM +0100, Paul Cercueil wrote:
> > >  Le mar. 25 oct. 2022 à 08:21:29 +0200, Uwe Kleine-König
> > >  <u.kleine-koenig@pengutronix.de> a écrit :
> > >  > Hello,
> > >  >
> > >  > On Mon, Oct 24, 2022 at 09:52:09PM +0100, Paul Cercueil wrote:
> > >  > >  The "duty > cycle" trick to force the pin level of a disabled
> > > TCU2
> > >  > >  channel would only work when the channel had been enabled
> > >  > > previously.
> > >  > >
> > >  > >  Address this issue by enabling the PWM mode in
> > > jz4740_pwm_disable
> > >  > >  (I know, right) so that the "duty > cycle" trick works before
> > >  > > disabling
> > >  > >  the PWM channel right after.
> > >  > >
> > >  > >  This issue went unnoticed, as the PWM pins on the majority of
> > > the
> > >  > > boards
> > >  > >  tested would default to the inactive level once the
> > > corresponding
> > >  > > TCU
> > >  > >  clock was enabled, so the first call to jz4740_pwm_disable()
> > > would
> > >  > > not
> > >  > >  actually change the pin levels.
> > >  > >
> > >  > >  On the GCW Zero however, the PWM pin for the backlight (PWM1,
> > > which
> > >  > > is
> > >  > >  a TCU2 channel) goes active as soon as the timer1 clock is
> > > enabled.
> > >  > >  Since the jz4740_pwm_disable() function did not work on
> > > channels not
> > >  > >  previously enabled, the backlight would shine at full
> > > brightness
> > >  > > from
> > >  > >  the moment the backlight driver would probe, until the
> > > backlight
> > >  > > driver
> > >  > >  tried to *enable* the PWM output.
> > >  > >
> > >  > >  With this fix, the PWM pins will be forced inactive as soon as
> > >  > >  jz4740_pwm_apply() is called (and might be reconfigured to
> > > active if
> > >  > >  dictated by the pwm_state). This means that there is still a
> > > tiny
> > >  > > time
> > >  > >  frame between the .request() and .apply() callbacks where the
> > > PWM
> > >  > > pin
> > >  > >  might be active. Sadly, there is no way to fix this issue: it
> > > is
> > >  > >  impossible to write a PWM channel's registers if the
> > > corresponding
> > >  > > clock
> > >  > >  is not enabled, and enabling the clock is what causes the PWM
> > > pin
> > >  > > to go
> > >  > >  active.
> > >  > >
> > >  > >  There is a workaround, though, which complements this fix:
> > > simply
> > >  > >  starting the backlight driver (or any PWM client driver) with a
> > >  > > "init"
> > >  > >  pinctrl state that sets the pin as an inactive GPIO. Once the
> > >  > > driver is
> > >  > >  probed and the pinctrl state switches to "default", the
> > > regular PWM
> > >  > > pin
> > >  > >  configuration can be used as it will be properly driven.
> > >  > >
> > >  > >  Fixes: c2693514a0a1 ("pwm: jz4740: Obtain regmap from parent
> > > node")
> > >  > >  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> > >  > >  Cc: stable@vger.kernel.org
> > >  >
> > >  > OK, understood the issue. I think there is another similar issue:
> > > The
> > >  > clk is get and enabled only in the .request() callback. The
> > > result is (I
> > >  > think---depends on a few further conditions) that if you have the
> > >  > backlight driver as a module and the bootloader enables the
> > > backlight to
> > >  > show a splash screen, the backlight goes off because of the
> > >  > clk_disable_unused initcall.
> > > 
> > >  I will have to verify, but I'm pretty sure disabling the clock
> > > doesn't
> > >  change the pin level back to inactive.
> > 
> > Given that you set the clk's rate depending on the period to apply, I'd
> > claim that you need to keep the clk on. Maybe it doesn't hurt, because
> > another component of the system keeps the clk running, but it's wrong
> > anyhow. Assumptions like these tend to break on new chip revisions.
> 
> If the backlight driver is a module then it will probe before the
> clk_disable_unused initcall, unless something is really wrong.

I'd claim the clk_disable_unused initcall is called before userspace
starts and so before the module can be loaded. Who is wrong here?

> So the backlight would stay ON if it was enabled by the bootloader,
> unless the DTB decides it doesn't have to be.

Don't understand that. How could hte DTB decide the backlight can be
disabled?
 
> Anyway, I can try your suggestion, and move the trick to force-disable PWM
> pins in the probe(). After that, the clocks can be safely disabled, so I can
> disable them (for the disabled PWMs) at the end of the probe and re-enable
> them again in their respective .request() callback.

I really lost track of the problem here and would appreciate a new
submission of the remaining (and improved?) patches.

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 1/5] pwm: jz4740: Fix pin level of disabled TCU2 channels, part 1

[Paul Cercueil]

Hi Uwe,

Le mardi 17 janvier 2023 à 22:35 +0100, Uwe Kleine-König a écrit :
> Hello Paul,
> 
> On Fri, Nov 18, 2022 at 09:55:40AM +0000, Paul Cercueil wrote:
> > Le jeu. 17 nov. 2022 à 14:29:27 +0100, Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> a écrit :
> > > Hello Paul,
> > > 
> > > On Tue, Oct 25, 2022 at 11:02:00AM +0100, Paul Cercueil wrote:
> > > >  Le mar. 25 oct. 2022 à 08:21:29 +0200, Uwe Kleine-König
> > > >  <u.kleine-koenig@pengutronix.de> a écrit :
> > > >  > Hello,
> > > >  >
> > > >  > On Mon, Oct 24, 2022 at 09:52:09PM +0100, Paul Cercueil
> > > > wrote:
> > > >  > >  The "duty > cycle" trick to force the pin level of a
> > > > disabled
> > > > TCU2
> > > >  > >  channel would only work when the channel had been enabled
> > > >  > > previously.
> > > >  > >
> > > >  > >  Address this issue by enabling the PWM mode in
> > > > jz4740_pwm_disable
> > > >  > >  (I know, right) so that the "duty > cycle" trick works
> > > > before
> > > >  > > disabling
> > > >  > >  the PWM channel right after.
> > > >  > >
> > > >  > >  This issue went unnoticed, as the PWM pins on the
> > > > majority of
> > > > the
> > > >  > > boards
> > > >  > >  tested would default to the inactive level once the
> > > > corresponding
> > > >  > > TCU
> > > >  > >  clock was enabled, so the first call to
> > > > jz4740_pwm_disable()
> > > > would
> > > >  > > not
> > > >  > >  actually change the pin levels.
> > > >  > >
> > > >  > >  On the GCW Zero however, the PWM pin for the backlight
> > > > (PWM1,
> > > > which
> > > >  > > is
> > > >  > >  a TCU2 channel) goes active as soon as the timer1 clock
> > > > is
> > > > enabled.
> > > >  > >  Since the jz4740_pwm_disable() function did not work on
> > > > channels not
> > > >  > >  previously enabled, the backlight would shine at full
> > > > brightness
> > > >  > > from
> > > >  > >  the moment the backlight driver would probe, until the
> > > > backlight
> > > >  > > driver
> > > >  > >  tried to *enable* the PWM output.
> > > >  > >
> > > >  > >  With this fix, the PWM pins will be forced inactive as
> > > > soon as
> > > >  > >  jz4740_pwm_apply() is called (and might be reconfigured
> > > > to
> > > > active if
> > > >  > >  dictated by the pwm_state). This means that there is
> > > > still a
> > > > tiny
> > > >  > > time
> > > >  > >  frame between the .request() and .apply() callbacks where
> > > > the
> > > > PWM
> > > >  > > pin
> > > >  > >  might be active. Sadly, there is no way to fix this
> > > > issue: it
> > > > is
> > > >  > >  impossible to write a PWM channel's registers if the
> > > > corresponding
> > > >  > > clock
> > > >  > >  is not enabled, and enabling the clock is what causes the
> > > > PWM
> > > > pin
> > > >  > > to go
> > > >  > >  active.
> > > >  > >
> > > >  > >  There is a workaround, though, which complements this
> > > > fix:
> > > > simply
> > > >  > >  starting the backlight driver (or any PWM client driver)
> > > > with a
> > > >  > > "init"
> > > >  > >  pinctrl state that sets the pin as an inactive GPIO. Once
> > > > the
> > > >  > > driver is
> > > >  > >  probed and the pinctrl state switches to "default", the
> > > > regular PWM
> > > >  > > pin
> > > >  > >  configuration can be used as it will be properly driven.
> > > >  > >
> > > >  > >  Fixes: c2693514a0a1 ("pwm: jz4740: Obtain regmap from
> > > > parent
> > > > node")
> > > >  > >  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> > > >  > >  Cc: stable@vger.kernel.org
> > > >  >
> > > >  > OK, understood the issue. I think there is another similar
> > > > issue:
> > > > The
> > > >  > clk is get and enabled only in the .request() callback. The
> > > > result is (I
> > > >  > think---depends on a few further conditions) that if you
> > > > have the
> > > >  > backlight driver as a module and the bootloader enables the
> > > > backlight to
> > > >  > show a splash screen, the backlight goes off because of the
> > > >  > clk_disable_unused initcall.
> > > > 
> > > >  I will have to verify, but I'm pretty sure disabling the clock
> > > > doesn't
> > > >  change the pin level back to inactive.
> > > 
> > > Given that you set the clk's rate depending on the period to
> > > apply, I'd
> > > claim that you need to keep the clk on. Maybe it doesn't hurt,
> > > because
> > > another component of the system keeps the clk running, but it's
> > > wrong
> > > anyhow. Assumptions like these tend to break on new chip
> > > revisions.
> > 
> > If the backlight driver is a module then it will probe before the
> > clk_disable_unused initcall, unless something is really wrong.
> 
> I'd claim the clk_disable_unused initcall is called before userspace
> starts and so before the module can be loaded. Who is wrong here?

Probably me.

> > So the backlight would stay ON if it was enabled by the bootloader,
> > unless the DTB decides it doesn't have to be.
> 
> Don't understand that. How could hte DTB decide the backlight can be
> disabled?

I don't remember what I meant by that :)
 
> > Anyway, I can try your suggestion, and move the trick to force-
> > disable PWM
> > pins in the probe(). After that, the clocks can be safely disabled,
> > so I can
> > disable them (for the disabled PWMs) at the end of the probe and
> > re-enable
> > them again in their respective .request() callback.
> 
> I really lost track of the problem here and would appreciate a new
> submission of the remaining (and improved?) patches.

Sure. I still have the patchset on the backburner and plan to
(eventually) send an updated version.

If you are fishing for patches I think you can take patches 3/5 and 4/5
of this patchset. Then I won't have to send them again in v2.

Cheers,
-Paul

Re: [PATCH 1/5] pwm: jz4740: Fix pin level of disabled TCU2 channels, part 1

[Uwe Kleine-König]

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

Hello Paul,

On Tue, Jan 17, 2023 at 11:05:10PM +0000, Paul Cercueil wrote:
> > I really lost track of the problem here and would appreciate a new
> > submission of the remaining (and improved?) patches.
> 
> Sure. I still have the patchset on the backburner and plan to
> (eventually) send an updated version.
> 
> If you are fishing for patches I think you can take patches 3/5 and 4/5
> of this patchset. Then I won't have to send them again in v2.

These are already in Linus' tree :-)

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