diff mbox series

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

Message ID 20221024205213.327001-2-paul@crapouillou.net
State New
Headers
Series pwm: jz4740: Fixes and some light changes |

Commit Message

Paul Cercueil Oct. 24, 2022, 8:52 p.m. UTC 
  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(-)

Comments

Uwe Kleine-König Oct. 25, 2022, 6:21 a.m. UTC | #1 
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().
Paul Cercueil Oct. 25, 2022, 10:02 a.m. UTC | #2 
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/ |
Uwe Kleine-König Nov. 17, 2022, 1:29 p.m. UTC | #3 
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
Paul Cercueil Nov. 18, 2022, 9:55 a.m. UTC | #4 
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
Uwe Kleine-König Jan. 17, 2023, 9:35 p.m. UTC | #5 
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
Paul Cercueil Jan. 17, 2023, 11:05 p.m. UTC | #6 
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
Uwe Kleine-König Jan. 18, 2023, 8:16 a.m. UTC | #7 
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
diff mbox series

Patch

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.