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

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

[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

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

[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

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