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@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> >  > >  Cc: stable@xxxxxxxxxxxxxxx
> >  >
> >  > 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