[alsa-devel] Questions about max98090 codec driver

[alsa-devel] Questions about max98090 codec driver

[Tzung-Bi Shih]

Hi,

I am studying an odd issue of max98090 codec on Baytrail-based
chromebook.  The issue is: when user playback and capture
simultaneously, it seems the PLL never get locked if msleep(10)
between the SHDN off and on
(https://elixir.bootlin.com/linux/v5.4-rc2/source/sound/soc/codecs/max98090.c#L2120).
The playback stream becomes silent and the console keeps printing "PLL
unlocked".  But, if comment out the msleep(10) between the SHDN off
and on, the issue fixed.  I am trying to find the reason but facing
further more questions and may need your inputs.

1. The commit b8a3ee820f7b ("ASoC: max98090: Add recovery for PLL lock
failure") enables ULK interrupts
(https://elixir.bootlin.com/linux/v5.4-rc2/source/sound/soc/codecs/max98090.c#L2088)
when PCM stream starting.  If max98090 claims its PLL is unlocked,
max98090_pll_work() get scheduled to workaround it by SHDN off and on
(https://elixir.bootlin.com/linux/v5.4-rc2/source/sound/soc/codecs/max98090.c#L2106).

I feel it is weird to sleep in max98090_pll_work().  Especially, at
this line https://elixir.bootlin.com/linux/v5.4-rc2/source/sound/soc/codecs/max98090.c#L2125
(it makes less sense to "wait" in another thread).  Note that, the
threaded IRQF_ONESHOT handler and max98090_pll_work() are in 2
different threads.

I guess the original intention is:
- disable ULK interrupt in IRQ handler
(https://elixir.bootlin.com/linux/v5.4-rc2/source/sound/soc/codecs/max98090.c#L2260)
- schedule max98090_pll_work() to workaround it
- wait 10ms to give PLL chance to lock
- enable ULK interrupt again
If max98090 claims its PLL is unlocked again, repeat the above by
receiving another ULK interrupt.

Unfortunately, the odd issue seems not be fixed by my rough
implementation of these.

2. According to the datasheet page 164 table 90
(https://datasheets.maximintegrated.com/en/ds/MAX98090.pdf), there are
some registers should only be adjusted when SHDN==0.  But I fail to
find max98090.c tries to set SHDN to 0 and restore it afterwards when
writing to these registers.  For example,
https://elixir.bootlin.com/linux/v5.4-rc2/source/sound/soc/codecs/max98090.c#L1897.
I am wondering if it would bring any side effects because the
datasheet states "Changing these settings during normal operation
(SHDN=1) can compromise device stability and performance
specifications."

3. By searching some history data, I found a previous version did not
have the msleep(10) between the SHDN off and on
(https://crrev.com/c/191740, click the file name in the middle of the
window to see the diff.  Pardon me, I do not find another public
repository for this).  I am curious if anyone of you still remember
why the upstream version contains the msleep(10).  I am also curious
if anyone of your environment works well with the upstream version
max98090.c.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] Questions about max98090 codec driver

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 3360 bytes --]

On Fri, Oct 25, 2019 at 02:23:18AM +0800, Tzung-Bi Shih wrote:

> The playback stream becomes silent and the console keeps printing "PLL
> unlocked".  But, if comment out the msleep(10) between the SHDN off
> and on, the issue fixed.  I am trying to find the reason but facing
> further more questions and may need your inputs.

Wow, that's a bit special.  I'm wondering if the PLL unlock error
handling isn't connected to the PLL configuration?

> I feel it is weird to sleep in max98090_pll_work().  Especially, at
> this line https://elixir.bootlin.com/linux/v5.4-rc2/source/sound/soc/codecs/max98090.c#L2125
> (it makes less sense to "wait" in another thread).  Note that, the
> threaded IRQF_ONESHOT handler and max98090_pll_work() are in 2
> different threads.

Sleeping after starting a PLL to give it time to lock is pretty normal.
Doing so after stopping is a bit more fun.

> I guess the original intention is:
> - disable ULK interrupt in IRQ handler
> (https://elixir.bootlin.com/linux/v5.4-rc2/source/sound/soc/codecs/max98090.c#L2260)
> - schedule max98090_pll_work() to workaround it
> - wait 10ms to give PLL chance to lock
> - enable ULK interrupt again
> If max98090 claims its PLL is unlocked again, repeat the above by
> receiving another ULK interrupt.

I think so.

> 2. According to the datasheet page 164 table 90
> (https://datasheets.maximintegrated.com/en/ds/MAX98090.pdf), there are
> some registers should only be adjusted when SHDN==0.  But I fail to
> find max98090.c tries to set SHDN to 0 and restore it afterwards when
> writing to these registers.  For example,
> https://elixir.bootlin.com/linux/v5.4-rc2/source/sound/soc/codecs/max98090.c#L1897.
> I am wondering if it would bring any side effects because the
> datasheet states "Changing these settings during normal operation
> (SHDN=1) can compromise device stability and performance
> specifications."

That does sound like it might be causing problems, yes - even if it's
not the problem you're seeing it's probably a good idea to try to follow
the datsheet recommendation in case it's causing some other problem.

> 3. By searching some history data, I found a previous version did not
> have the msleep(10) between the SHDN off and on
> (https://crrev.com/c/191740, click the file name in the middle of the
> window to see the diff.  Pardon me, I do not find another public
> repository for this).  I am curious if anyone of you still remember
> why the upstream version contains the msleep(10).  I am also curious
> if anyone of your environment works well with the upstream version
> max98090.c.

No idea from me on any of that.  Upstream the sleep between shutdown and
on was added in the original code to do the recovery from Jarkko,
b8a3ee820f7b0 (ASoC: max98090: Add recovery for PLL lock failure) - the
ChromeOS patch you linked to claims to be a backport but clearly isn't a
backport from upstream.  It's missing the first sleep, the second sleep
is shorter but it polls for success instead of just dead reckoning and
not reading back.  The "upstream" commit that the ChromeOS commit
references just doesn't exist upstream so no idea what they were
backporting from.

If the ChromeOS code is working for you we may as well get it upstream,
if we can start the PLL faster than the 10ms that's a win and the
confirmation that we got lock looks like a win too.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] Questions about max98090 codec driver

[Pierre-Louis Bossart]

On 10/24/19 1:23 PM, Tzung-Bi Shih wrote:
> Hi,
> 
> I am studying an odd issue of max98090 codec on Baytrail-based
> chromebook.  The issue is: when user playback and capture
> simultaneously, it seems the PLL never get locked if msleep(10)
> between the SHDN off and on
> (https://elixir.bootlin.com/linux/v5.4-rc2/source/sound/soc/codecs/max98090.c#L2120).
> The playback stream becomes silent and the console keeps printing "PLL
> unlocked".  But, if comment out the msleep(10) between the SHDN off
> and on, the issue fixed.  I am trying to find the reason but facing
> further more questions and may need your inputs.

IIRC this PPL unlocked problem also affects Cherrytrail/Braswell 
Chromebooks with the same codec, but strangely with a lower probability 
of occurrence (maybe due to clocking differences?). My contribution was 
to make the dmesg log ratelimited, but I could never figure out either 
what the root cause was.

> 
> 1. The commit b8a3ee820f7b ("ASoC: max98090: Add recovery for PLL lock
> failure") enables ULK interrupts
> (https://elixir.bootlin.com/linux/v5.4-rc2/source/sound/soc/codecs/max98090.c#L2088)
> when PCM stream starting.  If max98090 claims its PLL is unlocked,
> max98090_pll_work() get scheduled to workaround it by SHDN off and on
> (https://elixir.bootlin.com/linux/v5.4-rc2/source/sound/soc/codecs/max98090.c#L2106).
> 
> I feel it is weird to sleep in max98090_pll_work().  Especially, at
> this line https://elixir.bootlin.com/linux/v5.4-rc2/source/sound/soc/codecs/max98090.c#L2125
> (it makes less sense to "wait" in another thread).  Note that, the
> threaded IRQF_ONESHOT handler and max98090_pll_work() are in 2
> different threads.
> 
> I guess the original intention is:
> - disable ULK interrupt in IRQ handler
> (https://elixir.bootlin.com/linux/v5.4-rc2/source/sound/soc/codecs/max98090.c#L2260)
> - schedule max98090_pll_work() to workaround it
> - wait 10ms to give PLL chance to lock
> - enable ULK interrupt again
> If max98090 claims its PLL is unlocked again, repeat the above by
> receiving another ULK interrupt.
> 
> Unfortunately, the odd issue seems not be fixed by my rough
> implementation of these.
> 
> 2. According to the datasheet page 164 table 90
> (https://datasheets.maximintegrated.com/en/ds/MAX98090.pdf), there are
> some registers should only be adjusted when SHDN==0.  But I fail to
> find max98090.c tries to set SHDN to 0 and restore it afterwards when
> writing to these registers.  For example,
> https://elixir.bootlin.com/linux/v5.4-rc2/source/sound/soc/codecs/max98090.c#L1897.
> I am wondering if it would bring any side effects because the
> datasheet states "Changing these settings during normal operation
> (SHDN=1) can compromise device stability and performance
> specifications."
> 
> 3. By searching some history data, I found a previous version did not
> have the msleep(10) between the SHDN off and on
> (https://crrev.com/c/191740, click the file name in the middle of the
> window to see the diff.  Pardon me, I do not find another public
> repository for this).  I am curious if anyone of you still remember
> why the upstream version contains the msleep(10).  I am also curious
> if anyone of your environment works well with the upstream version
> max98090.c.
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] Questions about max98090 codec driver

[Tzung-Bi Shih]

On Fri, Oct 25, 2019 at 3:14 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Fri, Oct 25, 2019 at 02:23:18AM +0800, Tzung-Bi Shih wrote:
>
> > The playback stream becomes silent and the console keeps printing "PLL
> > unlocked".  But, if comment out the msleep(10) between the SHDN off
> > and on, the issue fixed.  I am trying to find the reason but facing
> > further more questions and may need your inputs.
>
> Wow, that's a bit special.  I'm wondering if the PLL unlock error
> handling isn't connected to the PLL configuration?

I don't quite understand here.  Did you mean: when max98090_pll_work()
get called, the PLL may have locked.  The code doesn't check
M98090_REG_DEVICE_STATUS but shutdown and on anyway.  In the case
max98090 may generate a new interrupt and again and again?

> > I feel it is weird to sleep in max98090_pll_work().  Especially, at
> > this line https://elixir.bootlin.com/linux/v5.4-rc2/source/sound/soc/codecs/max98090.c#L2125
> > (it makes less sense to "wait" in another thread).  Note that, the
> > threaded IRQF_ONESHOT handler and max98090_pll_work() are in 2
> > different threads.
>
> Sleeping after starting a PLL to give it time to lock is pretty normal.

I was trying to say: sleeping after starting in max98090_pll_work()
makes less sense by current implementation.  During the sleep time,
max98090 may generate a new ULK interrupt and try to schedule
max98090_pll_work() again.

> Doing so after stopping is a bit more fun.

Yes, and I couldn't find a recommendation of SHDN hold time in the
datasheet.  I guess we're safe to remove the sleep after stopping.
But I also don't expect it would be causing any problems if sleeping
after stopping.

> > I guess the original intention is:
> > - disable ULK interrupt in IRQ handler
> > (https://elixir.bootlin.com/linux/v5.4-rc2/source/sound/soc/codecs/max98090.c#L2260)
> > - schedule max98090_pll_work() to workaround it
> > - wait 10ms to give PLL chance to lock
> > - enable ULK interrupt again
> > If max98090 claims its PLL is unlocked again, repeat the above by
> > receiving another ULK interrupt.
>
> I think so.

I will fix toward this direction.

> > 2. According to the datasheet page 164 table 90
> > (https://datasheets.maximintegrated.com/en/ds/MAX98090.pdf), there are
> > some registers should only be adjusted when SHDN==0.  But I fail to
> > find max98090.c tries to set SHDN to 0 and restore it afterwards when
> > writing to these registers.  For example,
> > https://elixir.bootlin.com/linux/v5.4-rc2/source/sound/soc/codecs/max98090.c#L1897.
> > I am wondering if it would bring any side effects because the
> > datasheet states "Changing these settings during normal operation
> > (SHDN=1) can compromise device stability and performance
> > specifications."
>
> That does sound like it might be causing problems, yes - even if it's
> not the problem you're seeing it's probably a good idea to try to follow
> the datsheet recommendation in case it's causing some other problem.

I will try to fix toward this direction too.

One observed drawback of the fix is: when playbacking something, the
playback stream will be silent shortly if starting to capture (because
of the SHDN off and on).  Is it acceptable?
One workaround of this issue in ChromeOS is: enable DMIC-related
functions even if only speaker will be used.  More power consumption
would be expected in the case.  Would it be acceptable?

> If the ChromeOS code is working for you we may as well get it upstream,
> if we can start the PLL faster than the 10ms that's a win and the
> confirmation that we got lock looks like a win too.

From the datasheet page 26, the table states "PLL Lock Time" needs 2ms
typically and 7ms at most.  I will fix toward this direction: to see
if PLL could get locked less than 10ms and confirm we got lock in
max98090_pll_work().

With all proper fixes above, I should re-insert the msleep between
SHDN off and on to observe if the odd issue happens again.  We haven't
figured out the root cause afterall.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] Questions about max98090 codec driver

[Jarkko Nikula]

Hi

On 10/24/19 9:23 PM, Tzung-Bi Shih wrote:
> Hi,
> 
> I am studying an odd issue of max98090 codec on Baytrail-based
> chromebook.  The issue is: when user playback and capture
> simultaneously, it seems the PLL never get locked if msleep(10)
> between the SHDN off and on
> (https://elixir.bootlin.com/linux/v5.4-rc2/source/sound/soc/codecs/max98090.c#L2120).
> The playback stream becomes silent and the console keeps printing "PLL
> unlocked".  But, if comment out the msleep(10) between the SHDN off
> and on, the issue fixed.  I am trying to find the reason but facing
> further more questions and may need your inputs.
> 
My memory is vague but one thing came to my mind - is codec I2S master 
or slave in your setup? I think we were using codec in slave mode only. 
However don't know does PLL unlock issue occurs when codec is master.

> 1. The commit b8a3ee820f7b ("ASoC: max98090: Add recovery for PLL lock
> failure") enables ULK interrupts
> (https://elixir.bootlin.com/linux/v5.4-rc2/source/sound/soc/codecs/max98090.c#L2088)
> when PCM stream starting.  If max98090 claims its PLL is unlocked,
> max98090_pll_work() get scheduled to workaround it by SHDN off and on
> (https://elixir.bootlin.com/linux/v5.4-rc2/source/sound/soc/codecs/max98090.c#L2106).
> 
> I feel it is weird to sleep in max98090_pll_work().  Especially, at
> this line https://elixir.bootlin.com/linux/v5.4-rc2/source/sound/soc/codecs/max98090.c#L2125
> (it makes less sense to "wait" in another thread).  Note that, the
> threaded IRQF_ONESHOT handler and max98090_pll_work() are in 2
> different threads.
> 
> I guess the original intention is:
> - disable ULK interrupt in IRQ handler
> (https://elixir.bootlin.com/linux/v5.4-rc2/source/sound/soc/codecs/max98090.c#L2260)
> - schedule max98090_pll_work() to workaround it
> - wait 10ms to give PLL chance to lock
> - enable ULK interrupt again
> If max98090 claims its PLL is unlocked again, repeat the above by
> receiving another ULK interrupt.
> 
No ULK interrupt is disabled until max98090_pll_det_enable_work() 
enables it 10 ms after stream start.

If I remember correctly we were getting quite often or always PLL unlock 
interrupts shortly after stream start even PLL locked itself 
successfully. On those cases workaround was kind of needless and 
probably caused pop noise or similar.

So idea was to activate PLL unlock detection after 10 ms and do the 
workaround only if interrupt happens after that 10 ms "do not detect" 
period. Liam please correct if I remember wrong.

> Unfortunately, the odd issue seems not be fixed by my rough
> implementation of these.
> 
> 2. According to the datasheet page 164 table 90
> (https://datasheets.maximintegrated.com/en/ds/MAX98090.pdf), there are
> some registers should only be adjusted when SHDN==0.  But I fail to
> find max98090.c tries to set SHDN to 0 and restore it afterwards when
> writing to these registers.  For example,
> https://elixir.bootlin.com/linux/v5.4-rc2/source/sound/soc/codecs/max98090.c#L1897.
> I am wondering if it would bring any side effects because the
> datasheet states "Changing these settings during normal operation
> (SHDN=1) can compromise device stability and performance
> specifications."
> 
> 3. By searching some history data, I found a previous version did not
> have the msleep(10) between the SHDN off and on
> (https://crrev.com/c/191740, click the file name in the middle of the
> window to see the diff.  Pardon me, I do not find another public
> repository for this).  I am curious if anyone of you still remember
> why the upstream version contains the msleep(10).  I am also curious
> if anyone of your environment works well with the upstream version
> max98090.c.
> 
Yeah, that's looks like the one of the Liam's original workarounds for 
the issue. In that case looping 10 times. We were experimenting a lot 
around the issue and the commit b8a3ee820f7b ("ASoC: max98090: Add 
recovery for PLL lock failure") is the one combining our efforts 
together in the single commit.

Don't remember what incarnation brought these 10 ms sleeps between 
shutdown toggling and after. I'm guessing one for making sure codec 
reaches the shutdown and another perhaps for limiting that workaround is 
not run too soon again.

-- 
Jarkko
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] Questions about max98090 codec driver

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 563 bytes --]

On Fri, Oct 25, 2019 at 01:09:06PM +0800, Tzung-Bi Shih wrote:
> On Fri, Oct 25, 2019 at 3:14 AM Mark Brown <broonie@kernel.org> wrote:

> > Wow, that's a bit special.  I'm wondering if the PLL unlock error
> > handling isn't connected to the PLL configuration?

> I don't quite understand here.  Did you mean: when max98090_pll_work()
> get called, the PLL may have locked.  The code doesn't check
> M98090_REG_DEVICE_STATUS but shutdown and on anyway.  In the case
> max98090 may generate a new interrupt and again and again?

Something along those lines yeah.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel