From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
To: Paul Cercueil <paul@crapouillou.net>
Cc: Thierry Reding <thierry.reding@gmail.com>,
od@opendingux.net, linux-pwm@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-mips@vger.kernel.org,
stable@vger.kernel.org
Subject: Re: [PATCH 1/5] pwm: jz4740: Fix pin level of disabled TCU2 channels, part 1
Date: Tue, 17 Jan 2023 22:35:56 +0100 [thread overview]
Message-ID: <20230117213556.vdurctncvnjom62g@pengutronix.de> (raw)
In-Reply-To: <SKFJLR.07UMT1VWJOD52@crapouillou.net>
[-- 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 --]
next prev parent reply other threads:[~2023-01-17 23:04 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-24 20:52 [PATCH 0/5] pwm: jz4740: Fixes and some light changes Paul Cercueil
2022-10-24 20:52 ` [PATCH 1/5] pwm: jz4740: Fix pin level of disabled TCU2 channels, part 1 Paul Cercueil
2022-10-25 6:21 ` Uwe Kleine-König
2022-10-25 10:02 ` Paul Cercueil
2022-11-17 13:29 ` Uwe Kleine-König
2022-11-18 9:55 ` Paul Cercueil
2023-01-17 21:35 ` Uwe Kleine-König [this message]
2023-01-17 23:05 ` Paul Cercueil
2023-01-18 8:16 ` Uwe Kleine-König
2022-10-24 20:52 ` [PATCH 2/5] pwm: jz4740: Fix pin level of disabled TCU2 channels, part 2 Paul Cercueil
2022-10-25 6:44 ` Uwe Kleine-König
2022-10-25 10:10 ` Paul Cercueil
2022-11-28 14:39 ` Uwe Kleine-König
2022-11-29 12:16 ` Thierry Reding
2022-11-29 12:34 ` Paul Cercueil
2022-11-29 12:25 ` Paul Cercueil
2022-11-29 16:24 ` Uwe Kleine-König
2022-11-29 16:58 ` Paul Cercueil
2022-11-29 17:46 ` Uwe Kleine-König
2022-10-24 20:52 ` [PATCH 3/5] pwm: jz4740: Force dependency on Device Tree Paul Cercueil
2022-11-28 14:41 ` Uwe Kleine-König
2022-10-24 20:52 ` [PATCH 4/5] pwm: jz4740: Depend on MACH_INGENIC instead of MIPS Paul Cercueil
2022-10-25 10:32 ` Philippe Mathieu-Daudé
2022-11-15 10:40 ` Uwe Kleine-König
2022-10-24 20:52 ` [PATCH 5/5] pwm: jz4740: Use regmap_{set,clear}_bits Paul Cercueil
2022-10-25 10:46 ` Philippe Mathieu-Daudé
2022-11-15 10:51 ` Uwe Kleine-König
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230117213556.vdurctncvnjom62g@pengutronix.de \
--to=u.kleine-koenig@pengutronix.de \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mips@vger.kernel.org \
--cc=linux-pwm@vger.kernel.org \
--cc=od@opendingux.net \
--cc=paul@crapouillou.net \
--cc=stable@vger.kernel.org \
--cc=thierry.reding@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).