[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <SKFJLR.07UMT1VWJOD52@crapouillou.net>
Date: Fri, 18 Nov 2022 09:55:40 +0000
From: Paul Cercueil <paul@...pouillou.net>
To: Uwe Kleine-König
<u.kleine-koenig@...gutronix.de>
Cc: Thierry Reding <thierry.reding@...il.com>, od@...ndingux.net,
linux-pwm@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-mips@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH 1/5] pwm: jz4740: Fix pin level of disabled TCU2 channels,
part 1
Hi Uwe,
Le jeu. 17 nov. 2022 à 14:29:27 +0100, Uwe Kleine-König
<u.kleine-koenig@...gutronix.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@...gutronix.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@...pouillou.net>
>> > > Cc: stable@...r.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
Powered by blists - more mailing lists