Re: Regression found (Stop-marking-clocks-as-CLK_IS_CRITICAL)

[Hans de Goede]

Hi,

On 31-10-18 23:27, Pierre-Louis Bossart wrote:
> > Just thought it worth mentioning, this new patch that fixes sound
> > again, seems to have ressurected an old issue with PLL unlock. I'm
> > seeing journal entries after fresh boot ......
> >
> > ```
> > picard kernel: max98090 i2c-193C9890:00: PLL unlocked
> > picard systemd[462]: Started Sound Service.
> > picard kernel: max98090 i2c-193C9890:00: PLL unlocked
> > picard kernel: max98090 i2c-193C9890:00: PLL unlocked
> > picard kernel: max98090 i2c-193C9890:00: PLL unlocked
> > picard kernel: max98090 i2c-193C9890:00: PLL unlocked
> > picard kernel: max98090 i2c-193C9890:00: PLL unlocked
> > picard kernel: max98090 i2c-193C9890:00: PLL unlocked
> > picard kernel: max98090 i2c-193C9890:00: PLL unlocked
> > picard kernel: max98090 i2c-193C9890:00: PLL unlocked
> > picard kernel: max98090_pll_work: 141 callbacks suppressed
> > picard kernel: max98090 i2c-193C9890:00: PLL unlocked
> > ```
> >
> > sound is ok, but sometimes plugging in headphones spams journal with
> > those PLL messages, and sound turns into "daleks", and I have to
> > remove/insert headphones few times or stop/start audio to fix it.
> > It's a very old issue, maybe you'd know more about it.
>
> I noticed this error on my Orco device used for tests many moons ago, but I could never find out what led to this error case, it wasn't deterministic and didn't impact the audio quality. All I could do is rate_limit it... If we have an A vs. B situation it'd be really helpful to diagnose further.
>
> Is there really a causality between the changes from Hans and this PLL unlock error?

That is actually not unlikely. We have the mclk as the source clk for the PLL
and if we enable/disable it and/or change the src-clk then the PLL will loose
its lock and it needs some time to re-lock.

So these errors indicate that we need to either:

1) Sleep a bit after enabling the mclk;
   (and/or after changing the PLL src clk I've not checked if this driver does this)
2) Or poll the PLL locked bit after enabling the mclk until it indicate it has locked
   (and/or after changing the PLL src clk)

I've had to do similar things in u-boot when turning on PLL-s for DRAM and
stuff, so this sounds familiar.

Dean since you have the hardware, this is probably easiest for you to fix
(since you can reproduce the issue) do you feel up to giving fixing this
a shot? I think it is fine if you just go with the simple solution of
adding a msleep(xx) at the right place. I would expect 10 (ms) to do
the trick. If that works you may also try to give 5ms a shot.

Regards,

Hans