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.
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().
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/ |
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
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
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
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
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
linux-stable-mirror@lists.linaro.org
-
Paul Cercueil -
Uwe Kleine-König