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

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

[Stephen Boyd]

Quoting Dean Wallace (2018-10-25 16:25:17)
> I have found a regression in 4.18.15 that means I lose sound on my old
> Toshiba Chromebook 2 (Swanky).  My system details are:-
> 
> Toshiba Chromebook (Swanky)
> MrChromebox UEFI coreboot
> Arch Linux running latest alsa/pulseaudio
> 
> Upgraded kernel from 4.18.14 to 4.18.15 and lost all sound output.  By
> output I mean, the card is still detected, the module loaded, all apps
> showing sound is being playing, but no actual audible sound comes
> through.  Upgraded to 4.18.16 same issue.
> 
> Dug around and found Upstream commit 648e921888ad96ea3dc922739e96716ad3225d7f
> clk: x86: Stop marking clocks as CLK_IS_CRITICAL
> "This commit removes the CLK_IS_CRITICAL marking, fixing Cherry Trail
> devices not being able to reach S0i3 greatly decreasing their battery
> drain when suspended."
> 
> I reverted it and compiled 4.18.16 and have sound back again.  Could
> this be looked into, with possibility of fix.
> 

Thanks for the bug report. I'm adding some people involved in the commit
you mention is causing audio regressions. The best plan is to probably
revert the commit from the 4.18 linux stable tree. Or there may be
another patch missing that would be useful to make this backported patch
work. Hopefully Hans or Andy knows.

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

[Andy Shevchenko]

Cc: Pierre as well.

On Mon, Oct 29, 2018 at 7:48 PM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Dean Wallace (2018-10-25 16:25:17)
> > I have found a regression in 4.18.15 that means I lose sound on my old
> > Toshiba Chromebook 2 (Swanky).  My system details are:-
> >
> > Toshiba Chromebook (Swanky)
> > MrChromebox UEFI coreboot
> > Arch Linux running latest alsa/pulseaudio
> >
> > Upgraded kernel from 4.18.14 to 4.18.15 and lost all sound output.  By
> > output I mean, the card is still detected, the module loaded, all apps
> > showing sound is being playing, but no actual audible sound comes
> > through.  Upgraded to 4.18.16 same issue.
> >
> > Dug around and found Upstream commit 648e921888ad96ea3dc922739e96716ad3225d7f
> > clk: x86: Stop marking clocks as CLK_IS_CRITICAL
> > "This commit removes the CLK_IS_CRITICAL marking, fixing Cherry Trail
> > devices not being able to reach S0i3 greatly decreasing their battery
> > drain when suspended."
> >
> > I reverted it and compiled 4.18.16 and have sound back again.  Could
> > this be looked into, with possibility of fix.
> >
>
> Thanks for the bug report. I'm adding some people involved in the commit
> you mention is causing audio regressions. The best plan is to probably
> revert the commit from the 4.18 linux stable tree. Or there may be
> another patch missing that would be useful to make this backported patch
> work. Hopefully Hans or Andy knows.

Hans has been investigating S0ix issues on Baytrail and Cherrytrail machines.
I have a feeling that the problem can be fixed by properly handling
clock in ASoC driver(s). Perhaps Hans and Pierre can figure this out
better than me.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Mon, Oct 29, 2018 at 7:52 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> Cc: Pierre as well.
>
> On Mon, Oct 29, 2018 at 7:48 PM Stephen Boyd <sboyd@kernel.org> wrote:
> >
> > Quoting Dean Wallace (2018-10-25 16:25:17)
> > > I have found a regression in 4.18.15 that means I lose sound on my old
> > > Toshiba Chromebook 2 (Swanky).  My system details are:-
> > >
> > > Toshiba Chromebook (Swanky)
> > > MrChromebox UEFI coreboot
> > > Arch Linux running latest alsa/pulseaudio
> > >
> > > Upgraded kernel from 4.18.14 to 4.18.15 and lost all sound output.  By
> > > output I mean, the card is still detected, the module loaded, all apps
> > > showing sound is being playing, but no actual audible sound comes
> > > through.  Upgraded to 4.18.16 same issue.
> > >
> > > Dug around and found Upstream commit 648e921888ad96ea3dc922739e96716ad3225d7f
> > > clk: x86: Stop marking clocks as CLK_IS_CRITICAL
> > > "This commit removes the CLK_IS_CRITICAL marking, fixing Cherry Trail
> > > devices not being able to reach S0i3 greatly decreasing their battery
> > > drain when suspended."
> > >
> > > I reverted it and compiled 4.18.16 and have sound back again.  Could
> > > this be looked into, with possibility of fix.
> > >
> >
> > Thanks for the bug report. I'm adding some people involved in the commit
> > you mention is causing audio regressions. The best plan is to probably
> > revert the commit from the 4.18 linux stable tree. Or there may be
> > another patch missing that would be useful to make this backported patch
> > work. Hopefully Hans or Andy knows.
>
> Hans has been investigating S0ix issues on Baytrail and Cherrytrail machines.
> I have a feeling that the problem can be fixed by properly handling
> clock in ASoC driver(s). Perhaps Hans and Pierre can figure this out
> better than me.

Looking to sound/soc/intel/boards/cht_bsw_rt5672.c I see no
suspend-resume hooks. Perhaps, adding them like in the commit
ac8bd9e13be2 ("r8169: Disable clk during suspend / resume") would
help.

Btw, what the drivers are in use for the machine you have? It's better
you run alsa-info.sh or alike to collect necessary information along
with output of `lsmod`, `dmesg`, etc.

-- 
With Best Regards,
Andy Shevchenko

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

[Dean Wallace]

On 29-10-18, Andy Shevchenko wrote:
> On Mon, Oct 29, 2018 at 7:52 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> >
> > Cc: Pierre as well.
> >
> > On Mon, Oct 29, 2018 at 7:48 PM Stephen Boyd <sboyd@kernel.org> wrote:
> > >
> > > Quoting Dean Wallace (2018-10-25 16:25:17)
> > > > I have found a regression in 4.18.15 that means I lose sound on my old
> > > > Toshiba Chromebook 2 (Swanky).  My system details are:-
> > > >
> > > > Toshiba Chromebook (Swanky)
> > > > MrChromebox UEFI coreboot
> > > > Arch Linux running latest alsa/pulseaudio
> > > >
> > > > Upgraded kernel from 4.18.14 to 4.18.15 and lost all sound output.  By
> > > > output I mean, the card is still detected, the module loaded, all apps
> > > > showing sound is being playing, but no actual audible sound comes
> > > > through.  Upgraded to 4.18.16 same issue.
> > > >
> > > > Dug around and found Upstream commit 648e921888ad96ea3dc922739e96716ad3225d7f
> > > > clk: x86: Stop marking clocks as CLK_IS_CRITICAL
> > > > "This commit removes the CLK_IS_CRITICAL marking, fixing Cherry Trail
> > > > devices not being able to reach S0i3 greatly decreasing their battery
> > > > drain when suspended."
> > > >
> > > > I reverted it and compiled 4.18.16 and have sound back again.  Could
> > > > this be looked into, with possibility of fix.
> > > >
> > >
> > > Thanks for the bug report. I'm adding some people involved in the commit
> > > you mention is causing audio regressions. The best plan is to probably
> > > revert the commit from the 4.18 linux stable tree. Or there may be
> > > another patch missing that would be useful to make this backported patch
> > > work. Hopefully Hans or Andy knows.
> >
> > Hans has been investigating S0ix issues on Baytrail and Cherrytrail machines.
> > I have a feeling that the problem can be fixed by properly handling
> > clock in ASoC driver(s). Perhaps Hans and Pierre can figure this out
> > better than me.
> 
> Looking to sound/soc/intel/boards/cht_bsw_rt5672.c I see no
> suspend-resume hooks. Perhaps, adding them like in the commit
> ac8bd9e13be2 ("r8169: Disable clk during suspend / resume") would
> help.
> 
> Btw, what the drivers are in use for the machine you have? It's better
> you run alsa-info.sh or alike to collect necessary information along
> with output of `lsmod`, `dmesg`, etc.
> 
> -- 
> With Best Regards,
> Andy Shevchenko

alsa-info.txt [1], cpuinfo [2], dmesg [3], and 'ls /sys/bus/i2c/devices' [4]
[1] https://gist.github.com/duffydack/480be8ddced44515dbf981dc09f593ec#file-alsa-info
[2] https://gist.github.com/duffydack/480be8ddced44515dbf981dc09f593ec#file-cpuinfo-log
[3] https://gist.github.com/duffydack/480be8ddced44515dbf981dc09f593ec#file-dmesg-log
[4] https://gist.github.com/duffydack/480be8ddced44515dbf981dc09f593ec#file-sys-bus-i2c-devices-log

Regards
-- 
Dean

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

[Pierre-Louis Bossart]

On 10/29/18 2:08 PM, Dean Wallace wrote:
> On 29-10-18, Andy Shevchenko wrote:
>> On Mon, Oct 29, 2018 at 7:52 PM Andy Shevchenko
>> <andy.shevchenko@gmail.com> wrote:
>>> Cc: Pierre as well.
>>>
>>> On Mon, Oct 29, 2018 at 7:48 PM Stephen Boyd <sboyd@kernel.org> wrote:
>>>> Quoting Dean Wallace (2018-10-25 16:25:17)
>>>>> I have found a regression in 4.18.15 that means I lose sound on my old
>>>>> Toshiba Chromebook 2 (Swanky).  My system details are:-
>>>>>
>>>>> Toshiba Chromebook (Swanky)
>>>>> MrChromebox UEFI coreboot
>>>>> Arch Linux running latest alsa/pulseaudio
>>>>>
>>>>> Upgraded kernel from 4.18.14 to 4.18.15 and lost all sound output.  By
>>>>> output I mean, the card is still detected, the module loaded, all apps
>>>>> showing sound is being playing, but no actual audible sound comes
>>>>> through.  Upgraded to 4.18.16 same issue.
>>>>>
>>>>> Dug around and found Upstream commit 648e921888ad96ea3dc922739e96716ad3225d7f
>>>>> clk: x86: Stop marking clocks as CLK_IS_CRITICAL
>>>>> "This commit removes the CLK_IS_CRITICAL marking, fixing Cherry Trail
>>>>> devices not being able to reach S0i3 greatly decreasing their battery
>>>>> drain when suspended."
>>>>>
>>>>> I reverted it and compiled 4.18.16 and have sound back again.  Could
>>>>> this be looked into, with possibility of fix.
>>>>>
>>>> Thanks for the bug report. I'm adding some people involved in the commit
>>>> you mention is causing audio regressions. The best plan is to probably
>>>> revert the commit from the 4.18 linux stable tree. Or there may be
>>>> another patch missing that would be useful to make this backported patch
>>>> work. Hopefully Hans or Andy knows.
>>> Hans has been investigating S0ix issues on Baytrail and Cherrytrail machines.
>>> I have a feeling that the problem can be fixed by properly handling
>>> clock in ASoC driver(s). Perhaps Hans and Pierre can figure this out
>>> better than me.
>> Looking to sound/soc/intel/boards/cht_bsw_rt5672.c I see no
>> suspend-resume hooks. Perhaps, adding them like in the commit
>> ac8bd9e13be2 ("r8169: Disable clk during suspend / resume") would
>> help.

I missed this change while i was away. It's indeed the expectation that 
the audio mclk is handled by the firmware, not sure I understand why 
removing the CLK_IS_CRITICAL was necessary or what it has to do with S0ix.


>>
>> Btw, what the drivers are in use for the machine you have? It's better
>> you run alsa-info.sh or alike to collect necessary information along
>> with output of `lsmod`, `dmesg`, etc.
>>
>> -- 
>> With Best Regards,
>> Andy Shevchenko
> alsa-info.txt [1], cpuinfo [2], dmesg [3], and 'ls /sys/bus/i2c/devices' [4]
> [1] https://gist.github.com/duffydack/480be8ddced44515dbf981dc09f593ec#file-alsa-info
> [2] https://gist.github.com/duffydack/480be8ddced44515dbf981dc09f593ec#file-cpuinfo-log
> [3] https://gist.github.com/duffydack/480be8ddced44515dbf981dc09f593ec#file-dmesg-log
> [4] https://gist.github.com/duffydack/480be8ddced44515dbf981dc09f593ec#file-sys-bus-i2c-devices-log
>
> Regards

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

[Hans de Goede]

Hi Pierre-Louis,

On 29-10-18 23:03, Pierre-Louis Bossart wrote:
> 
> On 10/29/18 2:08 PM, Dean Wallace wrote:
>> On 29-10-18, Andy Shevchenko wrote:
>>> On Mon, Oct 29, 2018 at 7:52 PM Andy Shevchenko
>>> <andy.shevchenko@gmail.com> wrote:
>>>> Cc: Pierre as well.
>>>>
>>>> On Mon, Oct 29, 2018 at 7:48 PM Stephen Boyd <sboyd@kernel.org> wrote:
>>>>> Quoting Dean Wallace (2018-10-25 16:25:17)
>>>>>> I have found a regression in 4.18.15 that means I lose sound on my old
>>>>>> Toshiba Chromebook 2 (Swanky).  My system details are:-
>>>>>>
>>>>>> Toshiba Chromebook (Swanky)
>>>>>> MrChromebox UEFI coreboot
>>>>>> Arch Linux running latest alsa/pulseaudio
>>>>>>
>>>>>> Upgraded kernel from 4.18.14 to 4.18.15 and lost all sound output.  By
>>>>>> output I mean, the card is still detected, the module loaded, all apps
>>>>>> showing sound is being playing, but no actual audible sound comes
>>>>>> through.  Upgraded to 4.18.16 same issue.
>>>>>>
>>>>>> Dug around and found Upstream commit 648e921888ad96ea3dc922739e96716ad3225d7f
>>>>>> clk: x86: Stop marking clocks as CLK_IS_CRITICAL
>>>>>> "This commit removes the CLK_IS_CRITICAL marking, fixing Cherry Trail
>>>>>> devices not being able to reach S0i3 greatly decreasing their battery
>>>>>> drain when suspended."
>>>>>>
>>>>>> I reverted it and compiled 4.18.16 and have sound back again.  Could
>>>>>> this be looked into, with possibility of fix.
>>>>>>
>>>>> Thanks for the bug report. I'm adding some people involved in the commit
>>>>> you mention is causing audio regressions. The best plan is to probably
>>>>> revert the commit from the 4.18 linux stable tree. Or there may be
>>>>> another patch missing that would be useful to make this backported patch
>>>>> work. Hopefully Hans or Andy knows.
>>>> Hans has been investigating S0ix issues on Baytrail and Cherrytrail machines.
>>>> I have a feeling that the problem can be fixed by properly handling
>>>> clock in ASoC driver(s). Perhaps Hans and Pierre can figure this out
>>>> better than me.
>>> Looking to sound/soc/intel/boards/cht_bsw_rt5672.c I see no
>>> suspend-resume hooks. Perhaps, adding them like in the commit
>>> ac8bd9e13be2 ("r8169: Disable clk during suspend / resume") would
>>> help.
> 
> I missed this change while i was away. It's indeed the expectation that the audio mclk is handled by the firmware,

I believe that the statement "It's indeed the expectation that the
audio mclk is handled by the firmware" is not correct for BYT/CHT
platforms (and the commit causing the regression only affects BYT/CHT
platforms). Various machine drivers under sound/soc/intel/boards have
code to deal with the mclk themselves, like this:

         drv->mclk = devm_clk_get(&pdev->dev, "pmc_plt_clk_3");
...

         if (SND_SOC_DAPM_EVENT_ON(event)) {
                 ret = clk_prepare_enable(ctx->mclk);
		...
         } else {
                 clk_disable_unprepare(ctx->mclk);
         }

The above code is from sound/soc/intel/boards/cht_bsw_max98090_ti.c
which is the machine driver used on the machines for which problems
are now being reported.

And my commit introducing the problem is in essence a revert of:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=d31fd43c0f9a41e2678a1e78c0f22f0384c6edd3

Which is from 2017-07-14 and the ASoC code for BYT/CHT platforms is
much older then that.

Also I asked Carlo why he wrote that patch and it was to fix a problem
with ethernet on some laptops.

> not sure I understand why removing the CLK_IS_CRITICAL was necessary or what it has to do with S0ix.

It is necessary, because if it is set the clock never gets disabled and
on x86 platforms using suspend2idle we are responsible for *all* the hardware
power-management as OS. If we do not disable the clocks then we can only
reach S0i1 instead of S0i3 when suspended leading to increased battery
drain during suspend.

Also note that this patch is only causing problems on CHT + max98090 codec
using machines. I've tested it on many other BYT/CHT machines myself and
we've not had any other bug reports related to this.

So this clearly points to a problem with the clock management in the
cht_bsw_max98090_ti.c machine driver.

I've some ideas how to fix this and I will prepare some patches to test.

Regards,

Hans

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

[Hans de Goede]

Hi,

On 30-10-18 11:17, Hans de Goede wrote:
> Hi Pierre-Louis,
> 
> On 29-10-18 23:03, Pierre-Louis Bossart wrote:
>>
>> On 10/29/18 2:08 PM, Dean Wallace wrote:
>>> On 29-10-18, Andy Shevchenko wrote:
>>>> On Mon, Oct 29, 2018 at 7:52 PM Andy Shevchenko
>>>> <andy.shevchenko@gmail.com> wrote:
>>>>> Cc: Pierre as well.
>>>>>
>>>>> On Mon, Oct 29, 2018 at 7:48 PM Stephen Boyd <sboyd@kernel.org> wrote:
>>>>>> Quoting Dean Wallace (2018-10-25 16:25:17)
>>>>>>> I have found a regression in 4.18.15 that means I lose sound on my old
>>>>>>> Toshiba Chromebook 2 (Swanky).  My system details are:-
>>>>>>>
>>>>>>> Toshiba Chromebook (Swanky)
>>>>>>> MrChromebox UEFI coreboot
>>>>>>> Arch Linux running latest alsa/pulseaudio
>>>>>>>
>>>>>>> Upgraded kernel from 4.18.14 to 4.18.15 and lost all sound output.  By
>>>>>>> output I mean, the card is still detected, the module loaded, all apps
>>>>>>> showing sound is being playing, but no actual audible sound comes
>>>>>>> through.  Upgraded to 4.18.16 same issue.
>>>>>>>
>>>>>>> Dug around and found Upstream commit 648e921888ad96ea3dc922739e96716ad3225d7f
>>>>>>> clk: x86: Stop marking clocks as CLK_IS_CRITICAL
>>>>>>> "This commit removes the CLK_IS_CRITICAL marking, fixing Cherry Trail
>>>>>>> devices not being able to reach S0i3 greatly decreasing their battery
>>>>>>> drain when suspended."
>>>>>>>
>>>>>>> I reverted it and compiled 4.18.16 and have sound back again.  Could
>>>>>>> this be looked into, with possibility of fix.
>>>>>>>
>>>>>> Thanks for the bug report. I'm adding some people involved in the commit
>>>>>> you mention is causing audio regressions. The best plan is to probably
>>>>>> revert the commit from the 4.18 linux stable tree. Or there may be
>>>>>> another patch missing that would be useful to make this backported patch
>>>>>> work. Hopefully Hans or Andy knows.
>>>>> Hans has been investigating S0ix issues on Baytrail and Cherrytrail machines.
>>>>> I have a feeling that the problem can be fixed by properly handling
>>>>> clock in ASoC driver(s). Perhaps Hans and Pierre can figure this out
>>>>> better than me.
>>>> Looking to sound/soc/intel/boards/cht_bsw_rt5672.c I see no
>>>> suspend-resume hooks. Perhaps, adding them like in the commit
>>>> ac8bd9e13be2 ("r8169: Disable clk during suspend / resume") would
>>>> help.
>>
>> I missed this change while i was away. It's indeed the expectation that the audio mclk is handled by the firmware,
> 
> I believe that the statement "It's indeed the expectation that the
> audio mclk is handled by the firmware" is not correct for BYT/CHT
> platforms (and the commit causing the regression only affects BYT/CHT
> platforms). Various machine drivers under sound/soc/intel/boards have
> code to deal with the mclk themselves, like this:
> 
>          drv->mclk = devm_clk_get(&pdev->dev, "pmc_plt_clk_3");
> ...
> 
>          if (SND_SOC_DAPM_EVENT_ON(event)) {
>                  ret = clk_prepare_enable(ctx->mclk);
>          ...
>          } else {
>                  clk_disable_unprepare(ctx->mclk);
>          }
> 
> The above code is from sound/soc/intel/boards/cht_bsw_max98090_ti.c
> which is the machine driver used on the machines for which problems
> are now being reported.
> 
> And my commit introducing the problem is in essence a revert of:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=d31fd43c0f9a41e2678a1e78c0f22f0384c6edd3
> 
> Which is from 2017-07-14 and the ASoC code for BYT/CHT platforms is
> much older then that.
> 
> Also I asked Carlo why he wrote that patch and it was to fix a problem
> with ethernet on some laptops.
> 
>> not sure I understand why removing the CLK_IS_CRITICAL was necessary or what it has to do with S0ix.
> 
> It is necessary, because if it is set the clock never gets disabled and
> on x86 platforms using suspend2idle we are responsible for *all* the hardware
> power-management as OS. If we do not disable the clocks then we can only
> reach S0i1 instead of S0i3 when suspended leading to increased battery
> drain during suspend.
> 
> Also note that this patch is only causing problems on CHT + max98090 codec
> using machines. I've tested it on many other BYT/CHT machines myself and
> we've not had any other bug reports related to this.
> 
> So this clearly points to a problem with the clock management in the
> cht_bsw_max98090_ti.c machine driver.
> 
> I've some ideas how to fix this and I will prepare some patches to test.

p.s.

It seems that a sub-thread of this thread where me and the reporter have
been working on debugging this has lost most of the people in the Cc.

So here is a quick status update. So far this issue has been seen on
Swanky and Clapper model chromebooks.

Reverting the patch causing this regression and then doing:

for i in /sys/kernel/debug/clk/pmc_plt_clk_?; do echo -n "$i: "; cat $i/clk_flags; echo; done

has shown that before my patch to not set CLK_IS_CRITICAL the
CLK_IS_CRITICAL was being set on pmc_plt_clk_0. Which shows
that that clock is somehow being used for sounds on these boards.

I think that pmc_plt_clk_0 is being used instead of pmc_plt_clk_3,
but it could also be the case that both are being used.

I'm currently preparing patches to test both scenarios.

Regards,

Hans

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

[Hans de Goede]

[-- Attachment #1: Type: text/plain, Size: 4779 bytes --]

Hi Dean,

Attached are 2 different attempts at fixing this.

When trying these patches do not forget to remove the revert of the
"Stop-marking-clocks-as-CLK_IS_CRITICAL" commit.

Please first try the 0001-ASoC-intel-cht_bsw_max98090_ti-Use-pmc_plt_clk_0-ins.patch
patch I expect that one to do the trick indicating that the Swanky model
uses a different pmc clk then which is normally used for the codec clock.

If that patch does not fix things, please give the other patch a try.

Regards,

Hans



On 29-10-18 23:03, Pierre-Louis Bossart wrote:
> 
> On 10/29/18 2:08 PM, Dean Wallace wrote:
>> On 29-10-18, Andy Shevchenko wrote:
>>> On Mon, Oct 29, 2018 at 7:52 PM Andy Shevchenko
>>> <andy.shevchenko@gmail.com> wrote:
>>>> Cc: Pierre as well.
>>>>
>>>> On Mon, Oct 29, 2018 at 7:48 PM Stephen Boyd <sboyd@kernel.org> wrote:
>>>>> Quoting Dean Wallace (2018-10-25 16:25:17)
>>>>>> I have found a regression in 4.18.15 that means I lose sound on my old
>>>>>> Toshiba Chromebook 2 (Swanky).  My system details are:-
>>>>>>
>>>>>> Toshiba Chromebook (Swanky)
>>>>>> MrChromebox UEFI coreboot
>>>>>> Arch Linux running latest alsa/pulseaudio
>>>>>>
>>>>>> Upgraded kernel from 4.18.14 to 4.18.15 and lost all sound output.  By
>>>>>> output I mean, the card is still detected, the module loaded, all apps
>>>>>> showing sound is being playing, but no actual audible sound comes
>>>>>> through.  Upgraded to 4.18.16 same issue.
>>>>>>
>>>>>> Dug around and found Upstream commit 648e921888ad96ea3dc922739e96716ad3225d7f
>>>>>> clk: x86: Stop marking clocks as CLK_IS_CRITICAL
>>>>>> "This commit removes the CLK_IS_CRITICAL marking, fixing Cherry Trail
>>>>>> devices not being able to reach S0i3 greatly decreasing their battery
>>>>>> drain when suspended."
>>>>>>
>>>>>> I reverted it and compiled 4.18.16 and have sound back again.  Could
>>>>>> this be looked into, with possibility of fix.
>>>>>>
>>>>> Thanks for the bug report. I'm adding some people involved in the commit
>>>>> you mention is causing audio regressions. The best plan is to probably
>>>>> revert the commit from the 4.18 linux stable tree. Or there may be
>>>>> another patch missing that would be useful to make this backported patch
>>>>> work. Hopefully Hans or Andy knows.
>>>> Hans has been investigating S0ix issues on Baytrail and Cherrytrail machines.
>>>> I have a feeling that the problem can be fixed by properly handling
>>>> clock in ASoC driver(s). Perhaps Hans and Pierre can figure this out
>>>> better than me.
>>> Looking to sound/soc/intel/boards/cht_bsw_rt5672.c I see no
>>> suspend-resume hooks. Perhaps, adding them like in the commit
>>> ac8bd9e13be2 ("r8169: Disable clk during suspend / resume") would
>>> help.
> 
> I missed this change while i was away. It's indeed the expectation that the audio mclk is handled by the firmware,

I believe that the statement "It's indeed the expectation that the
audio mclk is handled by the firmware" is not correct for BYT/CHT
platforms (and the commit causing the regression only affects BYT/CHT
platforms). Various machine drivers under sound/soc/intel/boards have
code to deal with the mclk themselves, like this:

         drv->mclk = devm_clk_get(&pdev->dev, "pmc_plt_clk_3");
...

         if (SND_SOC_DAPM_EVENT_ON(event)) {
                 ret = clk_prepare_enable(ctx->mclk);
		...
         } else {
                 clk_disable_unprepare(ctx->mclk);
         }

The above code is from sound/soc/intel/boards/cht_bsw_max98090_ti.c
which is the machine driver used on the machines for which problems
are now being reported.

And my commit introducing the problem is in essence a revert of:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=d31fd43c0f9a41e2678a1e78c0f22f0384c6edd3

Which is from 2017-07-14 and the ASoC code for BYT/CHT platforms is
much older then that.

Also I asked Carlo why he wrote that patch and it was to fix a problem
with ethernet on some laptops.

> not sure I understand why removing the CLK_IS_CRITICAL was necessary or what it has to do with S0ix.

It is necessary, because if it is set the clock never gets disabled and
on x86 platforms using suspend2idle we are responsible for *all* the hardware
power-management as OS. If we do not disable the clocks then we can only
reach S0i1 instead of S0i3 when suspended leading to increased battery
drain during suspend.

Also note that this patch is only causing problems on CHT + max98090 codec
using machines. I've tested it on many other BYT/CHT machines myself and
we've not had any other bug reports related to this.

So this clearly points to a problem with the clock management in the
cht_bsw_max98090_ti.c machine driver.

I've some ideas how to fix this and I will prepare some patches to test.

Regards,

Hans

[-- Attachment #2: 0001-ASoC-intel-cht_bsw_max98090_ti-Also-enable-disable-p.patch --]
[-- Type: text/x-patch, Size: 2835 bytes --]

From 52e56378721f2e0ffde337f38db96a92d1870dc7 Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@redhat.com>
Date: Tue, 30 Oct 2018 12:14:27 +0100
Subject: [PATCH] ASoC: intel: cht_bsw_max98090_ti: Also enable/disable
 pmc_plt_clk_0

Testing has shown that some boards not only use pmc_plt_clk_3 but also
use pmc_plt_clk_0.

This commit makes us also get and enable/disable pmc_plt_clk_0 as
necessary, fixing sound not working on these boards.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 sound/soc/intel/boards/cht_bsw_max98090_ti.c | 24 ++++++++++----------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/sound/soc/intel/boards/cht_bsw_max98090_ti.c b/sound/soc/intel/boards/cht_bsw_max98090_ti.c
index db6976f4ddaa..750ac2160200 100644
--- a/sound/soc/intel/boards/cht_bsw_max98090_ti.c
+++ b/sound/soc/intel/boards/cht_bsw_max98090_ti.c
@@ -36,7 +36,7 @@
 #define CHT_CODEC_DAI	"HiFi"
 
 struct cht_mc_private {
-	struct clk *mclk;
+	struct clk_bulk_data clks[2];
 	struct snd_soc_jack jack;
 	bool ts3a227e_present;
 };
@@ -57,14 +57,14 @@ static int platform_clock_control(struct snd_soc_dapm_widget *w,
 	}
 
 	if (SND_SOC_DAPM_EVENT_ON(event)) {
-		ret = clk_prepare_enable(ctx->mclk);
+		ret = clk_bulk_prepare_enable(ARRAY_SIZE(ctx->clks), ctx->clks);
 		if (ret < 0) {
 			dev_err(card->dev,
 				"could not configure MCLK state");
 			return ret;
 		}
 	} else {
-		clk_disable_unprepare(ctx->mclk);
+		clk_bulk_disable_unprepare(ARRAY_SIZE(ctx->clks), ctx->clks);
 	}
 
 	return 0;
@@ -229,11 +229,11 @@ static int cht_codec_init(struct snd_soc_pcm_runtime *runtime)
 	 * to disable a clock that has not been enabled,
 	 * we need to enable the clock first.
 	 */
-	ret = clk_prepare_enable(ctx->mclk);
+	ret = clk_prepare_enable(ctx->clks[0].clk);
 	if (!ret)
-		clk_disable_unprepare(ctx->mclk);
+		clk_disable_unprepare(ctx->clks[0].clk);
 
-	ret = clk_set_rate(ctx->mclk, CHT_PLAT_CLK_3_HZ);
+	ret = clk_set_rate(ctx->clks[0].clk, CHT_PLAT_CLK_3_HZ);
 
 	if (ret)
 		dev_err(runtime->dev, "unable to set MCLK rate\n");
@@ -411,12 +411,12 @@ static int snd_cht_mc_probe(struct platform_device *pdev)
 	snd_soc_card_cht.dev = &pdev->dev;
 	snd_soc_card_set_drvdata(&snd_soc_card_cht, drv);
 
-	drv->mclk = devm_clk_get(&pdev->dev, "pmc_plt_clk_3");
-	if (IS_ERR(drv->mclk)) {
-		dev_err(&pdev->dev,
-			"Failed to get MCLK from pmc_plt_clk_3: %ld\n",
-			PTR_ERR(drv->mclk));
-		return PTR_ERR(drv->mclk);
+	drv->clks[0].id = "pmc_plt_clk_3"; /* Standard codec clk */
+	drv->clks[1].id = "pmc_plt_clk_0"; /* Extra clk used on some boards */
+	ret_val = devm_clk_bulk_get(&pdev->dev, 2, drv->clks);
+	if (ret_val) {
+		dev_err(&pdev->dev, "Failed to get clocks: %d\n", ret_val);
+		return ret_val;
 	}
 
 	ret_val = devm_snd_soc_register_card(&pdev->dev, &snd_soc_card_cht);
-- 
2.19.0


[-- Attachment #3: 0001-ASoC-intel-cht_bsw_max98090_ti-Use-pmc_plt_clk_0-ins.patch --]
[-- Type: text/x-patch, Size: 1296 bytes --]

From 03c73955ccc8cf925fac48295e92e62b6b1b35f7 Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@redhat.com>
Date: Tue, 30 Oct 2018 11:55:28 +0100
Subject: [PATCH] ASoC: intel: cht_bsw_max98090_ti: Use pmc_plt_clk_0 instead
 of pmc_plt_clk_3

Testing has shown that CHT boards with a max98090 codec use pmc_plt_clk_0
instead of pmc_plt_clk_3, adjust the code to match this.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 sound/soc/intel/boards/cht_bsw_max98090_ti.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sound/soc/intel/boards/cht_bsw_max98090_ti.c b/sound/soc/intel/boards/cht_bsw_max98090_ti.c
index db6976f4ddaa..ce44ebee8d88 100644
--- a/sound/soc/intel/boards/cht_bsw_max98090_ti.c
+++ b/sound/soc/intel/boards/cht_bsw_max98090_ti.c
@@ -411,10 +411,10 @@ static int snd_cht_mc_probe(struct platform_device *pdev)
 	snd_soc_card_cht.dev = &pdev->dev;
 	snd_soc_card_set_drvdata(&snd_soc_card_cht, drv);
 
-	drv->mclk = devm_clk_get(&pdev->dev, "pmc_plt_clk_3");
+	drv->mclk = devm_clk_get(&pdev->dev, "pmc_plt_clk_0");
 	if (IS_ERR(drv->mclk)) {
 		dev_err(&pdev->dev,
-			"Failed to get MCLK from pmc_plt_clk_3: %ld\n",
+			"Failed to get MCLK from pmc_plt_clk_0: %ld\n",
 			PTR_ERR(drv->mclk));
 		return PTR_ERR(drv->mclk);
 	}
-- 
2.19.0

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

[Dean Wallace]

On 30-10-18, Hans de Goede wrote:
> Hi Dean,
> 
> Attached are 2 different attempts at fixing this.
> 
> When trying these patches do not forget to remove the revert of the
> "Stop-marking-clocks-as-CLK_IS_CRITICAL" commit.
> 
> Please first try the 0001-ASoC-intel-cht_bsw_max98090_ti-Use-pmc_plt_clk_0-ins.patch
> patch I expect that one to do the trick indicating that the Swanky model
> uses a different pmc clk then which is normally used for the codec clock.
> 
> If that patch does not fix things, please give the other patch a try.
> 
> Regards,
> 
> Hans
> 
> 
> 
> On 29-10-18 23:03, Pierre-Louis Bossart wrote:
> > 
> > On 10/29/18 2:08 PM, Dean Wallace wrote:
> > > On 29-10-18, Andy Shevchenko wrote:
> > > > On Mon, Oct 29, 2018 at 7:52 PM Andy Shevchenko
> > > > <andy.shevchenko@gmail.com> wrote:
> > > > > Cc: Pierre as well.
> > > > > 
> > > > > On Mon, Oct 29, 2018 at 7:48 PM Stephen Boyd <sboyd@kernel.org> wrote:
> > > > > > Quoting Dean Wallace (2018-10-25 16:25:17)
> > > > > > > I have found a regression in 4.18.15 that means I lose sound on my old
> > > > > > > Toshiba Chromebook 2 (Swanky).  My system details are:-
> > > > > > > 
> > > > > > > Toshiba Chromebook (Swanky)
> > > > > > > MrChromebox UEFI coreboot
> > > > > > > Arch Linux running latest alsa/pulseaudio
> > > > > > > 
> > > > > > > Upgraded kernel from 4.18.14 to 4.18.15 and lost all sound output.  By
> > > > > > > output I mean, the card is still detected, the module loaded, all apps
> > > > > > > showing sound is being playing, but no actual audible sound comes
> > > > > > > through.  Upgraded to 4.18.16 same issue.
> > > > > > > 
> > > > > > > Dug around and found Upstream commit 648e921888ad96ea3dc922739e96716ad3225d7f
> > > > > > > clk: x86: Stop marking clocks as CLK_IS_CRITICAL
> > > > > > > "This commit removes the CLK_IS_CRITICAL marking, fixing Cherry Trail
> > > > > > > devices not being able to reach S0i3 greatly decreasing their battery
> > > > > > > drain when suspended."
> > > > > > > 
> > > > > > > I reverted it and compiled 4.18.16 and have sound back again.  Could
> > > > > > > this be looked into, with possibility of fix.
> > > > > > > 
> > > > > > Thanks for the bug report. I'm adding some people involved in the commit
> > > > > > you mention is causing audio regressions. The best plan is to probably
> > > > > > revert the commit from the 4.18 linux stable tree. Or there may be
> > > > > > another patch missing that would be useful to make this backported patch
> > > > > > work. Hopefully Hans or Andy knows.
> > > > > Hans has been investigating S0ix issues on Baytrail and Cherrytrail machines.
> > > > > I have a feeling that the problem can be fixed by properly handling
> > > > > clock in ASoC driver(s). Perhaps Hans and Pierre can figure this out
> > > > > better than me.
> > > > Looking to sound/soc/intel/boards/cht_bsw_rt5672.c I see no
> > > > suspend-resume hooks. Perhaps, adding them like in the commit
> > > > ac8bd9e13be2 ("r8169: Disable clk during suspend / resume") would
> > > > help.
> > 
> > I missed this change while i was away. It's indeed the expectation that the audio mclk is handled by the firmware,
> 
> I believe that the statement "It's indeed the expectation that the
> audio mclk is handled by the firmware" is not correct for BYT/CHT
> platforms (and the commit causing the regression only affects BYT/CHT
> platforms). Various machine drivers under sound/soc/intel/boards have
> code to deal with the mclk themselves, like this:
> 
>         drv->mclk = devm_clk_get(&pdev->dev, "pmc_plt_clk_3");
> ...
> 
>         if (SND_SOC_DAPM_EVENT_ON(event)) {
>                 ret = clk_prepare_enable(ctx->mclk);
> 		...
>         } else {
>                 clk_disable_unprepare(ctx->mclk);
>         }
> 
> The above code is from sound/soc/intel/boards/cht_bsw_max98090_ti.c
> which is the machine driver used on the machines for which problems
> are now being reported.
> 
> And my commit introducing the problem is in essence a revert of:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=d31fd43c0f9a41e2678a1e78c0f22f0384c6edd3
> 
> Which is from 2017-07-14 and the ASoC code for BYT/CHT platforms is
> much older then that.
> 
> Also I asked Carlo why he wrote that patch and it was to fix a problem
> with ethernet on some laptops.
> 
> > not sure I understand why removing the CLK_IS_CRITICAL was necessary or what it has to do with S0ix.
> 
> It is necessary, because if it is set the clock never gets disabled and
> on x86 platforms using suspend2idle we are responsible for *all* the hardware
> power-management as OS. If we do not disable the clocks then we can only
> reach S0i1 instead of S0i3 when suspended leading to increased battery
> drain during suspend.
> 
> Also note that this patch is only causing problems on CHT + max98090 codec
> using machines. I've tested it on many other BYT/CHT machines myself and
> we've not had any other bug reports related to this.
> 
> So this clearly points to a problem with the clock management in the
> cht_bsw_max98090_ti.c machine driver.
> 
> I've some ideas how to fix this and I will prepare some patches to test.
> 
> Regards,
> 
> Hans

Excellent work Hans.  Compiled 4.19 with
0001-ASoC-intel-cht_bsw_max98090_ti-Use-pmc_plt_clk_0-ins.patch, sound
works as before.

for i in /sys/kernel/debug/clk/pmc_plt_clk_?; do echo -n "$i: "; cat $i/clk_flags; echo; done
/sys/kernel/debug/clk/pmc_plt_clk_0: 
/sys/kernel/debug/clk/pmc_plt_clk_1: 
/sys/kernel/debug/clk/pmc_plt_clk_2: 
/sys/kernel/debug/clk/pmc_plt_clk_3: 
/sys/kernel/debug/clk/pmc_plt_clk_4: 
/sys/kernel/debug/clk/pmc_plt_clk_5: 

Regards
-- 
Dean

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

[Hans de Goede]

Hi,

On 30-10-18 15:38, Dean Wallace wrote:
> On 30-10-18, Hans de Goede wrote:
>> Hi Dean,
>>
>> Attached are 2 different attempts at fixing this.
>>
>> When trying these patches do not forget to remove the revert of the
>> "Stop-marking-clocks-as-CLK_IS_CRITICAL" commit.
>>
>> Please first try the 0001-ASoC-intel-cht_bsw_max98090_ti-Use-pmc_plt_clk_0-ins.patch
>> patch I expect that one to do the trick indicating that the Swanky model
>> uses a different pmc clk then which is normally used for the codec clock.
>>
>> If that patch does not fix things, please give the other patch a try.
>>
>> Regards,
>>
>> Hans
>>
>>
>>
>> On 29-10-18 23:03, Pierre-Louis Bossart wrote:
>>>
>>> On 10/29/18 2:08 PM, Dean Wallace wrote:
>>>> On 29-10-18, Andy Shevchenko wrote:
>>>>> On Mon, Oct 29, 2018 at 7:52 PM Andy Shevchenko
>>>>> <andy.shevchenko@gmail.com> wrote:
>>>>>> Cc: Pierre as well.
>>>>>>
>>>>>> On Mon, Oct 29, 2018 at 7:48 PM Stephen Boyd <sboyd@kernel.org> wrote:
>>>>>>> Quoting Dean Wallace (2018-10-25 16:25:17)
>>>>>>>> I have found a regression in 4.18.15 that means I lose sound on my old
>>>>>>>> Toshiba Chromebook 2 (Swanky).  My system details are:-
>>>>>>>>
>>>>>>>> Toshiba Chromebook (Swanky)
>>>>>>>> MrChromebox UEFI coreboot
>>>>>>>> Arch Linux running latest alsa/pulseaudio
>>>>>>>>
>>>>>>>> Upgraded kernel from 4.18.14 to 4.18.15 and lost all sound output.  By
>>>>>>>> output I mean, the card is still detected, the module loaded, all apps
>>>>>>>> showing sound is being playing, but no actual audible sound comes
>>>>>>>> through.  Upgraded to 4.18.16 same issue.
>>>>>>>>
>>>>>>>> Dug around and found Upstream commit 648e921888ad96ea3dc922739e96716ad3225d7f
>>>>>>>> clk: x86: Stop marking clocks as CLK_IS_CRITICAL
>>>>>>>> "This commit removes the CLK_IS_CRITICAL marking, fixing Cherry Trail
>>>>>>>> devices not being able to reach S0i3 greatly decreasing their battery
>>>>>>>> drain when suspended."
>>>>>>>>
>>>>>>>> I reverted it and compiled 4.18.16 and have sound back again.  Could
>>>>>>>> this be looked into, with possibility of fix.
>>>>>>>>
>>>>>>> Thanks for the bug report. I'm adding some people involved in the commit
>>>>>>> you mention is causing audio regressions. The best plan is to probably
>>>>>>> revert the commit from the 4.18 linux stable tree. Or there may be
>>>>>>> another patch missing that would be useful to make this backported patch
>>>>>>> work. Hopefully Hans or Andy knows.
>>>>>> Hans has been investigating S0ix issues on Baytrail and Cherrytrail machines.
>>>>>> I have a feeling that the problem can be fixed by properly handling
>>>>>> clock in ASoC driver(s). Perhaps Hans and Pierre can figure this out
>>>>>> better than me.
>>>>> Looking to sound/soc/intel/boards/cht_bsw_rt5672.c I see no
>>>>> suspend-resume hooks. Perhaps, adding them like in the commit
>>>>> ac8bd9e13be2 ("r8169: Disable clk during suspend / resume") would
>>>>> help.
>>>
>>> I missed this change while i was away. It's indeed the expectation that the audio mclk is handled by the firmware,
>>
>> I believe that the statement "It's indeed the expectation that the
>> audio mclk is handled by the firmware" is not correct for BYT/CHT
>> platforms (and the commit causing the regression only affects BYT/CHT
>> platforms). Various machine drivers under sound/soc/intel/boards have
>> code to deal with the mclk themselves, like this:
>>
>>          drv->mclk = devm_clk_get(&pdev->dev, "pmc_plt_clk_3");
>> ...
>>
>>          if (SND_SOC_DAPM_EVENT_ON(event)) {
>>                  ret = clk_prepare_enable(ctx->mclk);
>> 		...
>>          } else {
>>                  clk_disable_unprepare(ctx->mclk);
>>          }
>>
>> The above code is from sound/soc/intel/boards/cht_bsw_max98090_ti.c
>> which is the machine driver used on the machines for which problems
>> are now being reported.
>>
>> And my commit introducing the problem is in essence a revert of:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=d31fd43c0f9a41e2678a1e78c0f22f0384c6edd3
>>
>> Which is from 2017-07-14 and the ASoC code for BYT/CHT platforms is
>> much older then that.
>>
>> Also I asked Carlo why he wrote that patch and it was to fix a problem
>> with ethernet on some laptops.
>>
>>> not sure I understand why removing the CLK_IS_CRITICAL was necessary or what it has to do with S0ix.
>>
>> It is necessary, because if it is set the clock never gets disabled and
>> on x86 platforms using suspend2idle we are responsible for *all* the hardware
>> power-management as OS. If we do not disable the clocks then we can only
>> reach S0i1 instead of S0i3 when suspended leading to increased battery
>> drain during suspend.
>>
>> Also note that this patch is only causing problems on CHT + max98090 codec
>> using machines. I've tested it on many other BYT/CHT machines myself and
>> we've not had any other bug reports related to this.
>>
>> So this clearly points to a problem with the clock management in the
>> cht_bsw_max98090_ti.c machine driver.
>>
>> I've some ideas how to fix this and I will prepare some patches to test.
>>
>> Regards,
>>
>> Hans
> 
> Excellent work Hans.  Compiled 4.19 with
> 0001-ASoC-intel-cht_bsw_max98090_ti-Use-pmc_plt_clk_0-ins.patch, sound
> works as before.
> 
> for i in /sys/kernel/debug/clk/pmc_plt_clk_?; do echo -n "$i: "; cat $i/clk_flags; echo; done
> /sys/kernel/debug/clk/pmc_plt_clk_0:
> /sys/kernel/debug/clk/pmc_plt_clk_1:
> /sys/kernel/debug/clk/pmc_plt_clk_2:
> /sys/kernel/debug/clk/pmc_plt_clk_3:
> /sys/kernel/debug/clk/pmc_plt_clk_4:
> /sys/kernel/debug/clk/pmc_plt_clk_5:

Ok, so as I expected the Swanky is using pmc_plt_clk_0
as mclk instead of pmc_plt_clk_3. Now the question
becomes is this true for all the designs using the
max98090 codec?

Mogens, can you give the 0001-ASoC-intel-cht_bsw_max98090_ti-Use-pmc_plt_clk_0-ins.patch
a try on your Clapper model, using the SND_SOC_INTEL_CHT_BSW_MAX98090_TI_MACH
kernel option together with the asoundrc from Dean?

I have the feeling that your Clapper is also using the
pmc_plt_clk_0.

Pierre-Louis, as maintainer/reviewer of all the drivers under
sound/soc/intel/boards, how would you like to proceed with this?

I see 2 possible ways forwards:

1) Unconditionally use pmc_plt_clk_0 as mclk (as my test patch does)
in the cht_bsw_max98090_ti.c machine driver

2) Use DMI quirks for the Swansky (and probably also the Clapper) to
use pmc_plt_clk_0 as mclk there while keeping pmc_plt_clk_3 as the
default.

If you (Pierre-Louis) can let me know which solution you prepare then
I can prepare and submit a patch to fix this.

Regards,

Hans

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

[Andy Shevchenko]

On Tue, Oct 30, 2018 at 4:48 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 30-10-18 15:38, Dean Wallace wrote:

> > Excellent work Hans.  Compiled 4.19 with
> > 0001-ASoC-intel-cht_bsw_max98090_ti-Use-pmc_plt_clk_0-ins.patch, sound
> > works as before.
> >
> > for i in /sys/kernel/debug/clk/pmc_plt_clk_?; do echo -n "$i: "; cat $i/clk_flags; echo; done
> > /sys/kernel/debug/clk/pmc_plt_clk_0:
> > /sys/kernel/debug/clk/pmc_plt_clk_1:
> > /sys/kernel/debug/clk/pmc_plt_clk_2:
> > /sys/kernel/debug/clk/pmc_plt_clk_3:
> > /sys/kernel/debug/clk/pmc_plt_clk_4:
> > /sys/kernel/debug/clk/pmc_plt_clk_5:
>
> Ok, so as I expected the Swanky is using pmc_plt_clk_0
> as mclk instead of pmc_plt_clk_3. Now the question
> becomes is this true for all the designs using the
> max98090 codec?

> 1) Unconditionally use pmc_plt_clk_0 as mclk (as my test patch does)
> in the cht_bsw_max98090_ti.c machine driver

I don't know the details, but the main question here indeed do we ever
had a working example of that machine with CLK #3 in use?

P.S. I would go your first proposal until the opposite will be proved.

-- 
With Best Regards,
Andy Shevchenko

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

[Pierre-Louis Bossart]

On 10/30/18 9:38 AM, Dean Wallace wrote:
> On 30-10-18, Hans de Goede wrote:
>> Hi Dean,
>>
>> Attached are 2 different attempts at fixing this.
>>
>> When trying these patches do not forget to remove the revert of the
>> "Stop-marking-clocks-as-CLK_IS_CRITICAL" commit.
>>
>> Please first try the 0001-ASoC-intel-cht_bsw_max98090_ti-Use-pmc_plt_clk_0-ins.patch
>> patch I expect that one to do the trick indicating that the Swanky model
>> uses a different pmc clk then which is normally used for the codec clock.
>>
>> If that patch does not fix things, please give the other patch a try.

For Baytrail devices, the audio platform clocks are not managed by the 
firmware. They are for CHT-based devices - as can be seen by clock 
resources being described in the DSDT. We used to have a if(baytrail) in 
the code which was replaced by this CRITICAL label, but the point 
remains that there is a difference between the two SOC versions.

In addition I am not aware of any baytrail device using plt_clk_0, so 
moving a common machine driver such a cht_bsw_max98090_ti to use 
plt_clk0 only would break other devices (e.g. Rambi/Orco). Asking for 
both clocks to be on might work though, however you still have the 
problem of trying to manage from the kernel what the firmware already 
manages.

>>
>> Regards,
>>
>> Hans
>>
>>
>>
>> On 29-10-18 23:03, Pierre-Louis Bossart wrote:
>>> On 10/29/18 2:08 PM, Dean Wallace wrote:
>>>> On 29-10-18, Andy Shevchenko wrote:
>>>>> On Mon, Oct 29, 2018 at 7:52 PM Andy Shevchenko
>>>>> <andy.shevchenko@gmail.com> wrote:
>>>>>> Cc: Pierre as well.
>>>>>>
>>>>>> On Mon, Oct 29, 2018 at 7:48 PM Stephen Boyd <sboyd@kernel.org> wrote:
>>>>>>> Quoting Dean Wallace (2018-10-25 16:25:17)
>>>>>>>> I have found a regression in 4.18.15 that means I lose sound on my old
>>>>>>>> Toshiba Chromebook 2 (Swanky).  My system details are:-
>>>>>>>>
>>>>>>>> Toshiba Chromebook (Swanky)
>>>>>>>> MrChromebox UEFI coreboot
>>>>>>>> Arch Linux running latest alsa/pulseaudio
>>>>>>>>
>>>>>>>> Upgraded kernel from 4.18.14 to 4.18.15 and lost all sound output.  By
>>>>>>>> output I mean, the card is still detected, the module loaded, all apps
>>>>>>>> showing sound is being playing, but no actual audible sound comes
>>>>>>>> through.  Upgraded to 4.18.16 same issue.
>>>>>>>>
>>>>>>>> Dug around and found Upstream commit 648e921888ad96ea3dc922739e96716ad3225d7f
>>>>>>>> clk: x86: Stop marking clocks as CLK_IS_CRITICAL
>>>>>>>> "This commit removes the CLK_IS_CRITICAL marking, fixing Cherry Trail
>>>>>>>> devices not being able to reach S0i3 greatly decreasing their battery
>>>>>>>> drain when suspended."
>>>>>>>>
>>>>>>>> I reverted it and compiled 4.18.16 and have sound back again.  Could
>>>>>>>> this be looked into, with possibility of fix.
>>>>>>>>
>>>>>>> Thanks for the bug report. I'm adding some people involved in the commit
>>>>>>> you mention is causing audio regressions. The best plan is to probably
>>>>>>> revert the commit from the 4.18 linux stable tree. Or there may be
>>>>>>> another patch missing that would be useful to make this backported patch
>>>>>>> work. Hopefully Hans or Andy knows.
>>>>>> Hans has been investigating S0ix issues on Baytrail and Cherrytrail machines.
>>>>>> I have a feeling that the problem can be fixed by properly handling
>>>>>> clock in ASoC driver(s). Perhaps Hans and Pierre can figure this out
>>>>>> better than me.
>>>>> Looking to sound/soc/intel/boards/cht_bsw_rt5672.c I see no
>>>>> suspend-resume hooks. Perhaps, adding them like in the commit
>>>>> ac8bd9e13be2 ("r8169: Disable clk during suspend / resume") would
>>>>> help.
>>> I missed this change while i was away. It's indeed the expectation that the audio mclk is handled by the firmware,
>> I believe that the statement "It's indeed the expectation that the
>> audio mclk is handled by the firmware" is not correct for BYT/CHT
>> platforms (and the commit causing the regression only affects BYT/CHT
>> platforms). Various machine drivers under sound/soc/intel/boards have
>> code to deal with the mclk themselves, like this:
>>
>>          drv->mclk = devm_clk_get(&pdev->dev, "pmc_plt_clk_3");
>> ...
>>
>>          if (SND_SOC_DAPM_EVENT_ON(event)) {
>>                  ret = clk_prepare_enable(ctx->mclk);
>> 		...
>>          } else {
>>                  clk_disable_unprepare(ctx->mclk);
>>          }
>>
>> The above code is from sound/soc/intel/boards/cht_bsw_max98090_ti.c
>> which is the machine driver used on the machines for which problems
>> are now being reported.
>>
>> And my commit introducing the problem is in essence a revert of:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=d31fd43c0f9a41e2678a1e78c0f22f0384c6edd3
>>
>> Which is from 2017-07-14 and the ASoC code for BYT/CHT platforms is
>> much older then that.
>>
>> Also I asked Carlo why he wrote that patch and it was to fix a problem
>> with ethernet on some laptops.
>>
>>> not sure I understand why removing the CLK_IS_CRITICAL was necessary or what it has to do with S0ix.
>> It is necessary, because if it is set the clock never gets disabled and
>> on x86 platforms using suspend2idle we are responsible for *all* the hardware
>> power-management as OS. If we do not disable the clocks then we can only
>> reach S0i1 instead of S0i3 when suspended leading to increased battery
>> drain during suspend.
>>
>> Also note that this patch is only causing problems on CHT + max98090 codec
>> using machines. I've tested it on many other BYT/CHT machines myself and
>> we've not had any other bug reports related to this.
>>
>> So this clearly points to a problem with the clock management in the
>> cht_bsw_max98090_ti.c machine driver.
>>
>> I've some ideas how to fix this and I will prepare some patches to test.
>>
>> Regards,
>>
>> Hans
> Excellent work Hans.  Compiled 4.19 with
> 0001-ASoC-intel-cht_bsw_max98090_ti-Use-pmc_plt_clk_0-ins.patch, sound
> works as before.
>
> for i in /sys/kernel/debug/clk/pmc_plt_clk_?; do echo -n "$i: "; cat $i/clk_flags; echo; done
> /sys/kernel/debug/clk/pmc_plt_clk_0:
> /sys/kernel/debug/clk/pmc_plt_clk_1:
> /sys/kernel/debug/clk/pmc_plt_clk_2:
> /sys/kernel/debug/clk/pmc_plt_clk_3:
> /sys/kernel/debug/clk/pmc_plt_clk_4:
> /sys/kernel/debug/clk/pmc_plt_clk_5:
>
> Regards

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

[Pierre-Louis Bossart]

On 10/30/18 10:03 AM, Andy Shevchenko wrote:
> On Tue, Oct 30, 2018 at 4:48 PM Hans de Goede <hdegoede@redhat.com> wrote:
>> On 30-10-18 15:38, Dean Wallace wrote:
>>> Excellent work Hans.  Compiled 4.19 with
>>> 0001-ASoC-intel-cht_bsw_max98090_ti-Use-pmc_plt_clk_0-ins.patch, sound
>>> works as before.
>>>
>>> for i in /sys/kernel/debug/clk/pmc_plt_clk_?; do echo -n "$i: "; cat $i/clk_flags; echo; done
>>> /sys/kernel/debug/clk/pmc_plt_clk_0:
>>> /sys/kernel/debug/clk/pmc_plt_clk_1:
>>> /sys/kernel/debug/clk/pmc_plt_clk_2:
>>> /sys/kernel/debug/clk/pmc_plt_clk_3:
>>> /sys/kernel/debug/clk/pmc_plt_clk_4:
>>> /sys/kernel/debug/clk/pmc_plt_clk_5:
>> Ok, so as I expected the Swanky is using pmc_plt_clk_0
>> as mclk instead of pmc_plt_clk_3. Now the question
>> becomes is this true for all the designs using the
>> max98090 codec?
>> 1) Unconditionally use pmc_plt_clk_0 as mclk (as my test patch does)
>> in the cht_bsw_max98090_ti.c machine driver
> I don't know the details, but the main question here indeed do we ever
> had a working example of that machine with CLK #3 in use?
yes, Rambi/Orco (Baytrail based).
>
> P.S. I would go your first proposal until the opposite will be proved.
>

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

[Hans de Goede]

Hi,

On 30-10-18 16:04, Pierre-Louis Bossart wrote:
> 
> On 10/30/18 9:38 AM, Dean Wallace wrote:
>> On 30-10-18, Hans de Goede wrote:
>>> Hi Dean,
>>>
>>> Attached are 2 different attempts at fixing this.
>>>
>>> When trying these patches do not forget to remove the revert of the
>>> "Stop-marking-clocks-as-CLK_IS_CRITICAL" commit.
>>>
>>> Please first try the 0001-ASoC-intel-cht_bsw_max98090_ti-Use-pmc_plt_clk_0-ins.patch
>>> patch I expect that one to do the trick indicating that the Swanky model
>>> uses a different pmc clk then which is normally used for the codec clock.
>>>
>>> If that patch does not fix things, please give the other patch a try.
> 
> For Baytrail devices, the audio platform clocks are not managed by the firmware. They are for CHT-based devices - as can be seen by clock resources being described in the DSDT. We used to have a if(baytrail) in the code which was replaced by this CRITICAL label, but the point remains that there is a difference between the two SOC versions.

As I mentioned before the CRITICAL flag was only added a year ago to workaround
an issue with on board ethernet needing plt_clk_4 on some laptops, this never
had anything to do with sound.

> In addition I am not aware of any baytrail device using plt_clk_0, so moving a common machine driver such a cht_bsw_max98090_ti to use plt_clk0 only would break other devices (e.g. Rambi/Orco). Asking for both clocks to be on might work though, 

Ok, so we need to have a DMI based quirk for the Swanky and maybe also
the clapper to use plt_clk_0 there. Asking for 2 clks if we only need
one does not seem like a good plan.

> however you still have the problem of trying to manage from the kernel what the firmware already manages.

The firmware only manages it when going to D3 state, with ASoC most of
the codecs gets turned off (and we no longer need the clock) but we do
not put the device in D3 / execute the _PS3 method. So from a pm pov
it is better if we manage the clk ourselves.

Once we do actually put the device in D3 (on suspend) the kernel will have
already turned off the clk and the _OFF method of the CLK3 power resource
which directly pokes mmio, will just set the enable bit to 0 when it
already is 0, so no problem.

Regards,

Hans







> 
>>>
>>> Regards,
>>>
>>> Hans
>>>
>>>
>>>
>>> On 29-10-18 23:03, Pierre-Louis Bossart wrote:
>>>> On 10/29/18 2:08 PM, Dean Wallace wrote:
>>>>> On 29-10-18, Andy Shevchenko wrote:
>>>>>> On Mon, Oct 29, 2018 at 7:52 PM Andy Shevchenko
>>>>>> <andy.shevchenko@gmail.com> wrote:
>>>>>>> Cc: Pierre as well.
>>>>>>>
>>>>>>> On Mon, Oct 29, 2018 at 7:48 PM Stephen Boyd <sboyd@kernel.org> wrote:
>>>>>>>> Quoting Dean Wallace (2018-10-25 16:25:17)
>>>>>>>>> I have found a regression in 4.18.15 that means I lose sound on my old
>>>>>>>>> Toshiba Chromebook 2 (Swanky).  My system details are:-
>>>>>>>>>
>>>>>>>>> Toshiba Chromebook (Swanky)
>>>>>>>>> MrChromebox UEFI coreboot
>>>>>>>>> Arch Linux running latest alsa/pulseaudio
>>>>>>>>>
>>>>>>>>> Upgraded kernel from 4.18.14 to 4.18.15 and lost all sound output.  By
>>>>>>>>> output I mean, the card is still detected, the module loaded, all apps
>>>>>>>>> showing sound is being playing, but no actual audible sound comes
>>>>>>>>> through.  Upgraded to 4.18.16 same issue.
>>>>>>>>>
>>>>>>>>> Dug around and found Upstream commit 648e921888ad96ea3dc922739e96716ad3225d7f
>>>>>>>>> clk: x86: Stop marking clocks as CLK_IS_CRITICAL
>>>>>>>>> "This commit removes the CLK_IS_CRITICAL marking, fixing Cherry Trail
>>>>>>>>> devices not being able to reach S0i3 greatly decreasing their battery
>>>>>>>>> drain when suspended."
>>>>>>>>>
>>>>>>>>> I reverted it and compiled 4.18.16 and have sound back again.  Could
>>>>>>>>> this be looked into, with possibility of fix.
>>>>>>>>>
>>>>>>>> Thanks for the bug report. I'm adding some people involved in the commit
>>>>>>>> you mention is causing audio regressions. The best plan is to probably
>>>>>>>> revert the commit from the 4.18 linux stable tree. Or there may be
>>>>>>>> another patch missing that would be useful to make this backported patch
>>>>>>>> work. Hopefully Hans or Andy knows.
>>>>>>> Hans has been investigating S0ix issues on Baytrail and Cherrytrail machines.
>>>>>>> I have a feeling that the problem can be fixed by properly handling
>>>>>>> clock in ASoC driver(s). Perhaps Hans and Pierre can figure this out
>>>>>>> better than me.
>>>>>> Looking to sound/soc/intel/boards/cht_bsw_rt5672.c I see no
>>>>>> suspend-resume hooks. Perhaps, adding them like in the commit
>>>>>> ac8bd9e13be2 ("r8169: Disable clk during suspend / resume") would
>>>>>> help.
>>>> I missed this change while i was away. It's indeed the expectation that the audio mclk is handled by the firmware,
>>> I believe that the statement "It's indeed the expectation that the
>>> audio mclk is handled by the firmware" is not correct for BYT/CHT
>>> platforms (and the commit causing the regression only affects BYT/CHT
>>> platforms). Various machine drivers under sound/soc/intel/boards have
>>> code to deal with the mclk themselves, like this:
>>>
>>>          drv->mclk = devm_clk_get(&pdev->dev, "pmc_plt_clk_3");
>>> ...
>>>
>>>          if (SND_SOC_DAPM_EVENT_ON(event)) {
>>>                  ret = clk_prepare_enable(ctx->mclk);
>>>         ...
>>>          } else {
>>>                  clk_disable_unprepare(ctx->mclk);
>>>          }
>>>
>>> The above code is from sound/soc/intel/boards/cht_bsw_max98090_ti.c
>>> which is the machine driver used on the machines for which problems
>>> are now being reported.
>>>
>>> And my commit introducing the problem is in essence a revert of:
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=d31fd43c0f9a41e2678a1e78c0f22f0384c6edd3
>>>
>>> Which is from 2017-07-14 and the ASoC code for BYT/CHT platforms is
>>> much older then that.
>>>
>>> Also I asked Carlo why he wrote that patch and it was to fix a problem
>>> with ethernet on some laptops.
>>>
>>>> not sure I understand why removing the CLK_IS_CRITICAL was necessary or what it has to do with S0ix.
>>> It is necessary, because if it is set the clock never gets disabled and
>>> on x86 platforms using suspend2idle we are responsible for *all* the hardware
>>> power-management as OS. If we do not disable the clocks then we can only
>>> reach S0i1 instead of S0i3 when suspended leading to increased battery
>>> drain during suspend.
>>>
>>> Also note that this patch is only causing problems on CHT + max98090 codec
>>> using machines. I've tested it on many other BYT/CHT machines myself and
>>> we've not had any other bug reports related to this.
>>>
>>> So this clearly points to a problem with the clock management in the
>>> cht_bsw_max98090_ti.c machine driver.
>>>
>>> I've some ideas how to fix this and I will prepare some patches to test.
>>>
>>> Regards,
>>>
>>> Hans
>> Excellent work Hans.  Compiled 4.19 with
>> 0001-ASoC-intel-cht_bsw_max98090_ti-Use-pmc_plt_clk_0-ins.patch, sound
>> works as before.
>>
>> for i in /sys/kernel/debug/clk/pmc_plt_clk_?; do echo -n "$i: "; cat $i/clk_flags; echo; done
>> /sys/kernel/debug/clk/pmc_plt_clk_0:
>> /sys/kernel/debug/clk/pmc_plt_clk_1:
>> /sys/kernel/debug/clk/pmc_plt_clk_2:
>> /sys/kernel/debug/clk/pmc_plt_clk_3:
>> /sys/kernel/debug/clk/pmc_plt_clk_4:
>> /sys/kernel/debug/clk/pmc_plt_clk_5:
>>
>> Regards

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

[Hans de Goede]

Hi,

On 30-10-18 16:46, Hans de Goede wrote:
> Hi,
> 
> On 30-10-18 16:04, Pierre-Louis Bossart wrote:
>>
>> On 10/30/18 9:38 AM, Dean Wallace wrote:
>>> On 30-10-18, Hans de Goede wrote:
>>>> Hi Dean,
>>>>
>>>> Attached are 2 different attempts at fixing this.
>>>>
>>>> When trying these patches do not forget to remove the revert of the
>>>> "Stop-marking-clocks-as-CLK_IS_CRITICAL" commit.
>>>>
>>>> Please first try the 0001-ASoC-intel-cht_bsw_max98090_ti-Use-pmc_plt_clk_0-ins.patch
>>>> patch I expect that one to do the trick indicating that the Swanky model
>>>> uses a different pmc clk then which is normally used for the codec clock.
>>>>
>>>> If that patch does not fix things, please give the other patch a try.
>>
>> For Baytrail devices, the audio platform clocks are not managed by the firmware. They are for CHT-based devices - as can be seen by clock resources being described in the DSDT. We used to have a if(baytrail) in the code which was replaced by this CRITICAL label, but the point remains that there is a difference between the two SOC versions.
> 
> As I mentioned before the CRITICAL flag was only added a year ago to workaround
> an issue with on board ethernet needing plt_clk_4 on some laptops, this never
> had anything to do with sound.

Ah I see now that you later made some changes based on the patch to fix the ethernet:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=7735bce05a9c0bb0eb0f08c9002d65843a7c5798

Note though that that does not touch the machine driver which we are discussing
now and the reporters machine is also BYT based.

As explained below (in my original reply) I think it is fine to always
manage the clk from within the kernel. But if you think this is a bad
idea, we could re-introduce the is_valleyview() checks in machine
drivers which are used on CHT devices.

>> In addition I am not aware of any baytrail device using plt_clk_0, so moving a common machine driver such a cht_bsw_max98090_ti to use plt_clk0 only would break other devices (e.g. Rambi/Orco). Asking for both clocks to be on might work though, 
> 
> Ok, so we need to have a DMI based quirk for the Swanky and maybe also
> the clapper to use plt_clk_0 there. Asking for 2 clks if we only need
> one does not seem like a good plan.
> 
>> however you still have the problem of trying to manage from the kernel what the firmware already manages.
> 
> The firmware only manages it when going to D3 state, with ASoC most of
> the codecs gets turned off (and we no longer need the clock) but we do
> not put the device in D3 / execute the _PS3 method. So from a pm pov
> it is better if we manage the clk ourselves.
> 
> Once we do actually put the device in D3 (on suspend) the kernel will have
> already turned off the clk and the _OFF method of the CLK3 power resource
> which directly pokes mmio, will just set the enable bit to 0 when it
> already is 0, so no problem.

Regards,

Hans

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

[Pierre-Louis Bossart]

On 10/30/18 10:46 AM, Hans de Goede wrote:
> Hi,
>
> On 30-10-18 16:04, Pierre-Louis Bossart wrote:
>>
>> On 10/30/18 9:38 AM, Dean Wallace wrote:
>>> On 30-10-18, Hans de Goede wrote:
>>>> Hi Dean,
>>>>
>>>> Attached are 2 different attempts at fixing this.
>>>>
>>>> When trying these patches do not forget to remove the revert of the
>>>> "Stop-marking-clocks-as-CLK_IS_CRITICAL" commit.
>>>>
>>>> Please first try the 
>>>> 0001-ASoC-intel-cht_bsw_max98090_ti-Use-pmc_plt_clk_0-ins.patch
>>>> patch I expect that one to do the trick indicating that the Swanky 
>>>> model
>>>> uses a different pmc clk then which is normally used for the codec 
>>>> clock.
>>>>
>>>> If that patch does not fix things, please give the other patch a try.
>>
>> For Baytrail devices, the audio platform clocks are not managed by 
>> the firmware. They are for CHT-based devices - as can be seen by 
>> clock resources being described in the DSDT. We used to have a 
>> if(baytrail) in the code which was replaced by this CRITICAL label, 
>> but the point remains that there is a difference between the two SOC 
>> versions.
>
> As I mentioned before the CRITICAL flag was only added a year ago to 
> workaround
> an issue with on board ethernet needing plt_clk_4 on some laptops, 
> this never
> had anything to do with sound.

see commit 7735bce05a9c ('ASoC: Intel: boards: use devm_clk_get() 
unconditionally')

>
>> In addition I am not aware of any baytrail device using plt_clk_0, so 
>> moving a common machine driver such a cht_bsw_max98090_ti to use 
>> plt_clk0 only would break other devices (e.g. Rambi/Orco). Asking for 
>> both clocks to be on might work though, 
>
> Ok, so we need to have a DMI based quirk for the Swanky and maybe also
> the clapper to use plt_clk_0 there. Asking for 2 clks if we only need
> one does not seem like a good plan.
>
>> however you still have the problem of trying to manage from the 
>> kernel what the firmware already manages.
>
> The firmware only manages it when going to D3 state, with ASoC most of
> the codecs gets turned off (and we no longer need the clock) but we do
> not put the device in D3 / execute the _PS3 method. So from a pm pov
> it is better if we manage the clk ourselves.
>
> Once we do actually put the device in D3 (on suspend) the kernel will 
> have
> already turned off the clk and the _OFF method of the CLK3 power resource
> which directly pokes mmio, will just set the enable bit to 0 when it
> already is 0, so no problem.
>
> Regards,
>
> Hans
>
>
>
>
>
>
>
>>
>>>>
>>>> Regards,
>>>>
>>>> Hans
>>>>
>>>>
>>>>
>>>> On 29-10-18 23:03, Pierre-Louis Bossart wrote:
>>>>> On 10/29/18 2:08 PM, Dean Wallace wrote:
>>>>>> On 29-10-18, Andy Shevchenko wrote:
>>>>>>> On Mon, Oct 29, 2018 at 7:52 PM Andy Shevchenko
>>>>>>> <andy.shevchenko@gmail.com> wrote:
>>>>>>>> Cc: Pierre as well.
>>>>>>>>
>>>>>>>> On Mon, Oct 29, 2018 at 7:48 PM Stephen Boyd <sboyd@kernel.org> 
>>>>>>>> wrote:
>>>>>>>>> Quoting Dean Wallace (2018-10-25 16:25:17)
>>>>>>>>>> I have found a regression in 4.18.15 that means I lose sound 
>>>>>>>>>> on my old
>>>>>>>>>> Toshiba Chromebook 2 (Swanky).  My system details are:-
>>>>>>>>>>
>>>>>>>>>> Toshiba Chromebook (Swanky)
>>>>>>>>>> MrChromebox UEFI coreboot
>>>>>>>>>> Arch Linux running latest alsa/pulseaudio
>>>>>>>>>>
>>>>>>>>>> Upgraded kernel from 4.18.14 to 4.18.15 and lost all sound 
>>>>>>>>>> output.  By
>>>>>>>>>> output I mean, the card is still detected, the module loaded, 
>>>>>>>>>> all apps
>>>>>>>>>> showing sound is being playing, but no actual audible sound 
>>>>>>>>>> comes
>>>>>>>>>> through.  Upgraded to 4.18.16 same issue.
>>>>>>>>>>
>>>>>>>>>> Dug around and found Upstream commit 
>>>>>>>>>> 648e921888ad96ea3dc922739e96716ad3225d7f
>>>>>>>>>> clk: x86: Stop marking clocks as CLK_IS_CRITICAL
>>>>>>>>>> "This commit removes the CLK_IS_CRITICAL marking, fixing 
>>>>>>>>>> Cherry Trail
>>>>>>>>>> devices not being able to reach S0i3 greatly decreasing their 
>>>>>>>>>> battery
>>>>>>>>>> drain when suspended."
>>>>>>>>>>
>>>>>>>>>> I reverted it and compiled 4.18.16 and have sound back 
>>>>>>>>>> again.  Could
>>>>>>>>>> this be looked into, with possibility of fix.
>>>>>>>>>>
>>>>>>>>> Thanks for the bug report. I'm adding some people involved in 
>>>>>>>>> the commit
>>>>>>>>> you mention is causing audio regressions. The best plan is to 
>>>>>>>>> probably
>>>>>>>>> revert the commit from the 4.18 linux stable tree. Or there 
>>>>>>>>> may be
>>>>>>>>> another patch missing that would be useful to make this 
>>>>>>>>> backported patch
>>>>>>>>> work. Hopefully Hans or Andy knows.
>>>>>>>> Hans has been investigating S0ix issues on Baytrail and 
>>>>>>>> Cherrytrail machines.
>>>>>>>> I have a feeling that the problem can be fixed by properly 
>>>>>>>> handling
>>>>>>>> clock in ASoC driver(s). Perhaps Hans and Pierre can figure 
>>>>>>>> this out
>>>>>>>> better than me.
>>>>>>> Looking to sound/soc/intel/boards/cht_bsw_rt5672.c I see no
>>>>>>> suspend-resume hooks. Perhaps, adding them like in the commit
>>>>>>> ac8bd9e13be2 ("r8169: Disable clk during suspend / resume") would
>>>>>>> help.
>>>>> I missed this change while i was away. It's indeed the expectation 
>>>>> that the audio mclk is handled by the firmware,
>>>> I believe that the statement "It's indeed the expectation that the
>>>> audio mclk is handled by the firmware" is not correct for BYT/CHT
>>>> platforms (and the commit causing the regression only affects BYT/CHT
>>>> platforms). Various machine drivers under sound/soc/intel/boards have
>>>> code to deal with the mclk themselves, like this:
>>>>
>>>>          drv->mclk = devm_clk_get(&pdev->dev, "pmc_plt_clk_3");
>>>> ...
>>>>
>>>>          if (SND_SOC_DAPM_EVENT_ON(event)) {
>>>>                  ret = clk_prepare_enable(ctx->mclk);
>>>>         ...
>>>>          } else {
>>>>                  clk_disable_unprepare(ctx->mclk);
>>>>          }
>>>>
>>>> The above code is from sound/soc/intel/boards/cht_bsw_max98090_ti.c
>>>> which is the machine driver used on the machines for which problems
>>>> are now being reported.
>>>>
>>>> And my commit introducing the problem is in essence a revert of:
>>>>
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=d31fd43c0f9a41e2678a1e78c0f22f0384c6edd3 
>>>>
>>>>
>>>> Which is from 2017-07-14 and the ASoC code for BYT/CHT platforms is
>>>> much older then that.
>>>>
>>>> Also I asked Carlo why he wrote that patch and it was to fix a problem
>>>> with ethernet on some laptops.
>>>>
>>>>> not sure I understand why removing the CLK_IS_CRITICAL was 
>>>>> necessary or what it has to do with S0ix.
>>>> It is necessary, because if it is set the clock never gets disabled 
>>>> and
>>>> on x86 platforms using suspend2idle we are responsible for *all* 
>>>> the hardware
>>>> power-management as OS. If we do not disable the clocks then we can 
>>>> only
>>>> reach S0i1 instead of S0i3 when suspended leading to increased battery
>>>> drain during suspend.
>>>>
>>>> Also note that this patch is only causing problems on CHT + 
>>>> max98090 codec
>>>> using machines. I've tested it on many other BYT/CHT machines 
>>>> myself and
>>>> we've not had any other bug reports related to this.
>>>>
>>>> So this clearly points to a problem with the clock management in the
>>>> cht_bsw_max98090_ti.c machine driver.
>>>>
>>>> I've some ideas how to fix this and I will prepare some patches to 
>>>> test.
>>>>
>>>> Regards,
>>>>
>>>> Hans
>>> Excellent work Hans.  Compiled 4.19 with
>>> 0001-ASoC-intel-cht_bsw_max98090_ti-Use-pmc_plt_clk_0-ins.patch, sound
>>> works as before.
>>>
>>> for i in /sys/kernel/debug/clk/pmc_plt_clk_?; do echo -n "$i: "; cat 
>>> $i/clk_flags; echo; done
>>> /sys/kernel/debug/clk/pmc_plt_clk_0:
>>> /sys/kernel/debug/clk/pmc_plt_clk_1:
>>> /sys/kernel/debug/clk/pmc_plt_clk_2:
>>> /sys/kernel/debug/clk/pmc_plt_clk_3:
>>> /sys/kernel/debug/clk/pmc_plt_clk_4:
>>> /sys/kernel/debug/clk/pmc_plt_clk_5:
>>>
>>> Regards

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

[Hans de Goede]

Hi,

On 30-10-18 16:46, Hans de Goede wrote:
> Hi,
> 
> On 30-10-18 16:04, Pierre-Louis Bossart wrote:
>> In addition I am not aware of any baytrail device using plt_clk_0, so moving a common machine driver such a cht_bsw_max98090_ti to use plt_clk0 only would break other devices (e.g. Rambi/Orco). Asking for both clocks to be on might work though, 
> 
> Ok, so we need to have a DMI based quirk for the Swanky and maybe also
> the clapper to use plt_clk_0 there. Asking for 2 clks if we only need
> one does not seem like a good plan.

Dean, Mogens,

To write a proper patch for this I'm going to need DMI strings
from your devices.

Can you please run (as normal user):

grep . /sys/class/dmi/id/* 2> /dev/null

And reply with the output of this command?

Thanks,

Hans

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

[Dean Wallace]

On 30-10-18, Hans de Goede wrote:
> Hi,
> 
> On 30-10-18 16:46, Hans de Goede wrote:
> > Hi,
> > 
> > On 30-10-18 16:04, Pierre-Louis Bossart wrote:
> > > In addition I am not aware of any baytrail device using plt_clk_0,
> > > so moving a common machine driver such a cht_bsw_max98090_ti to use
> > > plt_clk0 only would break other devices (e.g. Rambi/Orco). Asking
> > > for both clocks to be on might work though,
> > 
> > Ok, so we need to have a DMI based quirk for the Swanky and maybe also
> > the clapper to use plt_clk_0 there. Asking for 2 clks if we only need
> > one does not seem like a good plan.
> 
> Dean, Mogens,
> 
> To write a proper patch for this I'm going to need DMI strings
> from your devices.
> 
> Can you please run (as normal user):
> 
> grep . /sys/class/dmi/id/* 2> /dev/null
> 
> And reply with the output of this command?
Here's mine, for a coreboot uefi based swanky.

https://ptpb.pw/~swanky-dmi-log

--
Dean

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

[Andy Shevchenko]

On Tue, Oct 30, 2018 at 1:05 PM Hans de Goede <hdegoede@redhat.com> wrote:

> for i in /sys/kernel/debug/clk/pmc_plt_clk_?; do echo -n "$i: "; cat $i/clk_flags; echo; done

Just a side note about nice tool, called `grep` :-)

grep -H . /sys/kernel/debug/clk/pmc_plt_clk_*/clk_flags

does above in one call.

-- 
With Best Regards,
Andy Shevchenko

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

[Pierre-Louis Bossart]

On 10/30/18 11:02 AM, Hans de Goede wrote:
> Hi,
>
> On 30-10-18 16:46, Hans de Goede wrote:
>> Hi,
>>
>> On 30-10-18 16:04, Pierre-Louis Bossart wrote:
>>>
>>> On 10/30/18 9:38 AM, Dean Wallace wrote:
>>>> On 30-10-18, Hans de Goede wrote:
>>>>> Hi Dean,
>>>>>
>>>>> Attached are 2 different attempts at fixing this.
>>>>>
>>>>> When trying these patches do not forget to remove the revert of the
>>>>> "Stop-marking-clocks-as-CLK_IS_CRITICAL" commit.
>>>>>
>>>>> Please first try the 
>>>>> 0001-ASoC-intel-cht_bsw_max98090_ti-Use-pmc_plt_clk_0-ins.patch
>>>>> patch I expect that one to do the trick indicating that the Swanky 
>>>>> model
>>>>> uses a different pmc clk then which is normally used for the codec 
>>>>> clock.
>>>>>
>>>>> If that patch does not fix things, please give the other patch a try.
>>>
>>> For Baytrail devices, the audio platform clocks are not managed by 
>>> the firmware. They are for CHT-based devices - as can be seen by 
>>> clock resources being described in the DSDT. We used to have a 
>>> if(baytrail) in the code which was replaced by this CRITICAL label, 
>>> but the point remains that there is a difference between the two SOC 
>>> versions.
>>
>> As I mentioned before the CRITICAL flag was only added a year ago to 
>> workaround
>> an issue with on board ethernet needing plt_clk_4 on some laptops, 
>> this never
>> had anything to do with sound.
>
> Ah I see now that you later made some changes based on the patch to 
> fix the ethernet:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=7735bce05a9c0bb0eb0f08c9002d65843a7c5798 
>
>
> Note though that that does not touch the machine driver which we are 
> discussing
> now and the reporters machine is also BYT based.
>
> As explained below (in my original reply) I think it is fine to always
> manage the clk from within the kernel. But if you think this is a bad
> idea, we could re-introduce the is_valleyview() checks in machine
> drivers which are used on CHT devices.

I am having difficulties following the proposal

for audio, managing the platform clocks from the kernel is only useful 
for Baytrail, and the code handling the CRITICAL flags is really 
equivalent to having a is_valleyview() test in the machine drivers. I 
don't quite understand the S0ix-related changes and I also don't get how 
using plt_clk_0 makes things better or wonder if audio on Swanky worked 
before the commit 7735bce05a9c ('ASoC: Intel: boards: use devm_clk_get() 
unconditionally'.

In other words, I don't know if we are dealing with a platform that only 
started working with 7735bce05a9c and does need a quirk to make use of 
plt_clk_0 or a fundamental issue with clock/power management.


>
>>> In addition I am not aware of any baytrail device using plt_clk_0, 
>>> so moving a common machine driver such a cht_bsw_max98090_ti to use 
>>> plt_clk0 only would break other devices (e.g. Rambi/Orco). Asking 
>>> for both clocks to be on might work though, 
>>
>> Ok, so we need to have a DMI based quirk for the Swanky and maybe also
>> the clapper to use plt_clk_0 there. Asking for 2 clks if we only need
>> one does not seem like a good plan.
>>
>>> however you still have the problem of trying to manage from the 
>>> kernel what the firmware already manages.
>>
>> The firmware only manages it when going to D3 state, with ASoC most of
>> the codecs gets turned off (and we no longer need the clock) but we do
>> not put the device in D3 / execute the _PS3 method. So from a pm pov
>> it is better if we manage the clk ourselves.
>>
>> Once we do actually put the device in D3 (on suspend) the kernel will 
>> have
>> already turned off the clk and the _OFF method of the CLK3 power 
>> resource
>> which directly pokes mmio, will just set the enable bit to 0 when it
>> already is 0, so no problem.
>
> Regards,
>
> Hans

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

[Hans de Goede]

Hi Pierre-Louis,

On 30-10-18 17:27, Pierre-Louis Bossart wrote:
> 
> On 10/30/18 11:02 AM, Hans de Goede wrote:
>> Hi,
>>
>> On 30-10-18 16:46, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 30-10-18 16:04, Pierre-Louis Bossart wrote:
>>>>
>>>> On 10/30/18 9:38 AM, Dean Wallace wrote:
>>>>> On 30-10-18, Hans de Goede wrote:
>>>>>> Hi Dean,
>>>>>>
>>>>>> Attached are 2 different attempts at fixing this.
>>>>>>
>>>>>> When trying these patches do not forget to remove the revert of the
>>>>>> "Stop-marking-clocks-as-CLK_IS_CRITICAL" commit.
>>>>>>
>>>>>> Please first try the 0001-ASoC-intel-cht_bsw_max98090_ti-Use-pmc_plt_clk_0-ins.patch
>>>>>> patch I expect that one to do the trick indicating that the Swanky model
>>>>>> uses a different pmc clk then which is normally used for the codec clock.
>>>>>>
>>>>>> If that patch does not fix things, please give the other patch a try.
>>>>
>>>> For Baytrail devices, the audio platform clocks are not managed by the firmware. They are for CHT-based devices - as can be seen by clock resources being described in the DSDT. We used to have a if(baytrail) in the code which was replaced by this CRITICAL label, but the point remains that there is a difference between the two SOC versions.
>>>
>>> As I mentioned before the CRITICAL flag was only added a year ago to workaround
>>> an issue with on board ethernet needing plt_clk_4 on some laptops, this never
>>> had anything to do with sound.
>>
>> Ah I see now that you later made some changes based on the patch to fix the ethernet:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=7735bce05a9c0bb0eb0f08c9002d65843a7c5798
>>
>> Note though that that does not touch the machine driver which we are discussing
>> now and the reporters machine is also BYT based.
>>
>> As explained below (in my original reply) I think it is fine to always
>> manage the clk from within the kernel. But if you think this is a bad
>> idea, we could re-introduce the is_valleyview() checks in machine
>> drivers which are used on CHT devices.
> 
> I am having difficulties following the proposal
> 
> for audio, managing the platform clocks from the kernel is only useful for Baytrail, and the code handling the CRITICAL flags is really equivalent to having a is_valleyview() test in the machine drivers. I don't quite understand the S0ix-related changes and I also don't get how using plt_clk_0 makes things better or wonder if audio on Swanky worked before the commit 7735bce05a9c ('ASoC: Intel: boards: use devm_clk_get() unconditionally'.
> 
> In other words, I don't know if we are dealing with a platform that only started working with 7735bce05a9c and does need a quirk to make use of plt_clk_0 or a fundamental issue with clock/power management.

Ok let me try to clarify this by breaking this out into the 3 separate
things which I believe are in play here:


1) The PMC clocks should NOT be marked as CLK_IS_CRITICAL

Commit d31fd43c0f9a ("clk: x86: Do not gate clocks enabled by the firmware")
should, in hindsight, never have been merged. CLK_IS_CRITICAL disables all
management of the clk in question and is really intended for clks which are
e.g. driving the CPU core itself or DRAM. The proper fix for the ethernet
problem that patch was meant to address is for the ethernet driver to get
and enable the clock itself. As I've done in a recent series which has as
purpose to revert d31fd43c0f9a.

d31fd43c0f9a marks any PMC clock which is enabled by the firmware as boot
as critical, not only the codec mclk, but *any* clk.  This means that unless
all clks which are enabled at boot are controlled by the firmware as
either ACPI pwr-resources, or by a device's _PS3 method, some clks will
stay on during suspend, see e.g. :

https://bugzilla.kernel.org/show_bug.cgi?id=193891#c102
https://bugzilla.kernel.org/show_bug.cgi?id=196861

Also on BYT this means that if the firmware has enabled the coded mclk at
boot it will get marked as CLK_IS_CRITICAL and we will now never disable it.

If some clocks are still on during suspend then the SoC cannot reach its
deepest sleep states while suspended, which obviously is bad.

TL;DR: CLK_IS_CRITICAL block reaching the deepest sleep states so
d31fd43c0f9a needs to be reverted.


2) Dropping the CLK_IS_CRITICAL flag means we are now also controlling
the codec mclk on CHT.

Just like setting the CLK_IS_CRITICAL flag on the mclk meant that we were
effectively no longer controlling the mclk on BYT, reverting this change
combined with your 7735bce05a9c ("ASoC: Intel: boards: use devm_clk_get()
unconditionally") change means that we are now controlling the mclk on
CHT platforms.

As you point out the firmware also controls the mclk on these platforms,
so arguably this is undesirable. As I tried to explain I do not believe
that this is actually a problem. But if you think this is a problem then
we will need to revert 7735bce05a9c. If you prefer to go that route then
I can prepare a patch for this.


3) No longer marking PMC clocks enabled at boot as CLK_IS_CRITICAL has
caused a regression on Swanky and Clapper model chromebooks

You wrote: "I wonder if audio on Swanky worked before the commit 7735bce05a9c"
I don't think that that commit has impacted Swanky at all since Swanky is BYT
based. Instead I believe that d31fd43c0f9a ("clk: x86: Do not gate clocks
enabled by the firmware") has accidentally fixed audio on Swanky laptops.
This also explains why my "Stop-marking-clocks-as-CLK_IS_CRITICAL" commit
has broken it again as that is effectively a revert of d31fd43c0f9.

As I wrote before the as a result of d31fd43c0f9a the clk enable/disable
behavior is not only ignored on CHT, as you noticed when writing 7735bce05a9c,
but it also causes it to be ignored on BYT, including the initial disable of
unused clocks just before the kernel starts /sbin/init. This not only fixed
the ethernet on the laptop model that d31fd43c0f9a targeted but also caused
us to no longer disable plt_clk_0 on Swanky (debugging done outside of
this thread has shown that d31fd43c0f9a sets CLK_IS_CRITICAL on plt_clk_0).

TL;DR: d31fd43c0f9a ("clk: x86: Do not gate clocks enabled by the firmware")
caused plt_clk_0 to be marked as CLK_IS_CRITICAL on Swanky, accidentally
fixing audio. The proper fix for this is to make the cht_bsw_max98090_ti.c
code control pmc_plt_clk_0 instead of pmc_plt_clk_3 as the Swanky board is
actually using pmc_plt_clk_0 and since this is a BYT board we should be
controlling the mclk there.


I hope this helps to clarify things, if not please keep asking questions.

Regards,

Hans

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

[Mogens Jensen]

[-- Attachment #1: Type: text/plain, Size: 1273 bytes --]

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Tuesday, October 30, 2018 4:04 PM, Hans de Goede <hdegoede@redhat.com> wrote:

> Hi,
>
> On 30-10-18 16:46, Hans de Goede wrote:
>
> > Hi,
> > On 30-10-18 16:04, Pierre-Louis Bossart wrote:
> >
> > > In addition I am not aware of any baytrail device using plt_clk_0, so moving a common machine driver such a cht_bsw_max98090_ti to use plt_clk0 only would break other devices (e.g. Rambi/Orco). Asking for both clocks to be on might work though,
> >
> > Ok, so we need to have a DMI based quirk for the Swanky and maybe also
> > the clapper to use plt_clk_0 there. Asking for 2 clks if we only need
> > one does not seem like a good plan.
>
> Dean, Mogens,
>
> To write a proper patch for this I'm going to need DMI strings
> from your devices.
>
> Can you please run (as normal user):
>
> grep . /sys/class/dmi/id/* 2> /dev/null
>
> And reply with the output of this command?
I have attached the output from a coreboot seabios based clapper.

Should I still test 0001-ASoC-intel-cht_bsw_max98090_ti-Use-pmc_plt_clk_0-ins.patch with SND_SOC_INTEL_CHT_BSW_MAX98090_TI_MACH and asoundrc from Dean? There seems to have been some development in the case since that request was made.


[-- Attachment #2: mogens-clapper-dmi.txt --]
[-- Type: text/plain, Size: 483 bytes --]

/sys/class/dmi/id/bios_date:08/22/2014
/sys/class/dmi/id/bios_vendor:coreboot
/sys/class/dmi/id/chassis_type:3
/sys/class/dmi/id/chassis_vendor:GOOGLE
/sys/class/dmi/id/modalias:dmi:bvncoreboot:bvr:bd08/22/2014:svnGOOGLE:pnClapper:pvr1.0:cvnGOOGLE:ct3:cvr:
/sys/class/dmi/id/product_name:Clapper
/sys/class/dmi/id/product_version:1.0
/sys/class/dmi/id/sys_vendor:GOOGLE
/sys/class/dmi/id/uevent:MODALIAS=dmi:bvncoreboot:bvr:bd08/22/2014:svnGOOGLE:pnClapper:pvr1.0:cvnGOOGLE:ct3:cvr:

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

[Hans de Goede]

Hi,

On 30-10-18 19:56, Mogens Jensen wrote:
> ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
> On Tuesday, October 30, 2018 4:04 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> 
>> Hi,
>>
>> On 30-10-18 16:46, Hans de Goede wrote:
>>
>>> Hi,
>>> On 30-10-18 16:04, Pierre-Louis Bossart wrote:
>>>
>>>> In addition I am not aware of any baytrail device using plt_clk_0, so moving a common machine driver such a cht_bsw_max98090_ti to use plt_clk0 only would break other devices (e.g. Rambi/Orco). Asking for both clocks to be on might work though,
>>>
>>> Ok, so we need to have a DMI based quirk for the Swanky and maybe also
>>> the clapper to use plt_clk_0 there. Asking for 2 clks if we only need
>>> one does not seem like a good plan.
>>
>> Dean, Mogens,
>>
>> To write a proper patch for this I'm going to need DMI strings
>> from your devices.
>>
>> Can you please run (as normal user):
>>
>> grep . /sys/class/dmi/id/* 2> /dev/null
>>
>> And reply with the output of this command?
> I have attached the output from a coreboot seabios based clapper.

Thank you.

> Should I still test 0001-ASoC-intel-cht_bsw_max98090_ti-Use-pmc_plt_clk_0-ins.patch with SND_SOC_INTEL_CHT_BSW_MAX98090_TI_MACH and asoundrc from Dean? There seems to have been some development in the case since that request was made.

Yes please test that, I expect that to also fix things for the
Clapper, but I need to have that confirmed before submitting a
patch upstream adding a quirk for the Clapper to use pmc_plt_clk_0
instead of pmc_plt_clk_3.

Regards,

Hans

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

[Mogens Jensen]

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Tuesday, October 30, 2018 7:10 PM, Hans de Goede <hdegoede@redhat.com> wrote:

> Hi,
>
> On 30-10-18 19:56, Mogens Jensen wrote:
>
> > ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
> > On Tuesday, October 30, 2018 4:04 PM, Hans de Goede hdegoede@redhat.com wrote:
> >
> > > Hi,
> > > On 30-10-18 16:46, Hans de Goede wrote:
> > >
> > > > Hi,
> > > > On 30-10-18 16:04, Pierre-Louis Bossart wrote:
> > > >
> > > > > In addition I am not aware of any baytrail device using plt_clk_0, so moving a common machine driver such a cht_bsw_max98090_ti to use plt_clk0 only would break other devices (e.g. Rambi/Orco). Asking for both clocks to be on might work though,
> > > >
> > > > Ok, so we need to have a DMI based quirk for the Swanky and maybe also
> > > > the clapper to use plt_clk_0 there. Asking for 2 clks if we only need
> > > > one does not seem like a good plan.
> > >
> > > Dean, Mogens,
> > > To write a proper patch for this I'm going to need DMI strings
> > > from your devices.
> > > Can you please run (as normal user):
> > > grep . /sys/class/dmi/id/* 2> /dev/null
> > > And reply with the output of this command?
> > > I have attached the output from a coreboot seabios based clapper.
>
> Thank you.
>
> > Should I still test 0001-ASoC-intel-cht_bsw_max98090_ti-Use-pmc_plt_clk_0-ins.patch with SND_SOC_INTEL_CHT_BSW_MAX98090_TI_MACH and asoundrc from Dean? There seems to have been some development in the case since that request was made.
>
> Yes please test that, I expect that to also fix things for the
> Clapper, but I need to have that confirmed before submitting a
> patch upstream adding a quirk for the Clapper to use pmc_plt_clk_0
> instead of pmc_plt_clk_3.
>
> Regards,
>
> Hans
>
Unfortunately I only have access to longterm kernel 4.14 for building/running on this system, and 0001-ASoC-intel-cht_bsw_max98090_ti-Use-pmc_plt_clk_0-ins.patch does not patch against 4.14.78. Can a test patch for 4.14 be created?

Regards,

Mogens

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

[Hans de Goede]

Hi,

On 31-10-18 07:02, Mogens Jensen wrote:
> ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
> On Tuesday, October 30, 2018 7:10 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> 
>> Hi,
>>
>> On 30-10-18 19:56, Mogens Jensen wrote:
>>
>>> ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
>>> On Tuesday, October 30, 2018 4:04 PM, Hans de Goede hdegoede@redhat.com wrote:
>>>
>>>> Hi,
>>>> On 30-10-18 16:46, Hans de Goede wrote:
>>>>
>>>>> Hi,
>>>>> On 30-10-18 16:04, Pierre-Louis Bossart wrote:
>>>>>
>>>>>> In addition I am not aware of any baytrail device using plt_clk_0, so moving a common machine driver such a cht_bsw_max98090_ti to use plt_clk0 only would break other devices (e.g. Rambi/Orco). Asking for both clocks to be on might work though,
>>>>>
>>>>> Ok, so we need to have a DMI based quirk for the Swanky and maybe also
>>>>> the clapper to use plt_clk_0 there. Asking for 2 clks if we only need
>>>>> one does not seem like a good plan.
>>>>
>>>> Dean, Mogens,
>>>> To write a proper patch for this I'm going to need DMI strings
>>>> from your devices.
>>>> Can you please run (as normal user):
>>>> grep . /sys/class/dmi/id/* 2> /dev/null
>>>> And reply with the output of this command?
>>>> I have attached the output from a coreboot seabios based clapper.
>>
>> Thank you.
>>
>>> Should I still test 0001-ASoC-intel-cht_bsw_max98090_ti-Use-pmc_plt_clk_0-ins.patch with SND_SOC_INTEL_CHT_BSW_MAX98090_TI_MACH and asoundrc from Dean? There seems to have been some development in the case since that request was made.
>>
>> Yes please test that, I expect that to also fix things for the
>> Clapper, but I need to have that confirmed before submitting a
>> patch upstream adding a quirk for the Clapper to use pmc_plt_clk_0
>> instead of pmc_plt_clk_3.
>>
>> Regards,
>>
>> Hans
>>
> Unfortunately I only have access to longterm kernel 4.14 for building/running on this system, and 0001-ASoC-intel-cht_bsw_max98090_ti-Use-pmc_plt_clk_0-ins.patch does not patch against 4.14.78. Can a test patch for 4.14 be created?

Can you run (as root):

for i in /sys/kernel/debug/clk/pmc_plt_clk_?; do echo -n "$i: "; cat $i/clk_flags; echo; done

When running a kernel with working audio?

Then I can confirm that the Clapper is also using pmc_plt_clk_0, so that I can
fix this for the clapper for 4.18+

I've just checked the 4.14 sources and in 4.14 the SND_SOC_INTEL_CHT_BSW_MAX98090_TI_MACH
driver does not support mclk control yet, so for the 4.14 kernel the only way to
fix this is to revert the 648e921888ad ("clk: x86: Stop marking clocks as CLK_IS_CRITICAL")
commit.

Regards,

Hans

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

[Dean Wallace]

On 31-10-18, Hans de Goede wrote:
> Hi,
> 
> On 31-10-18 07:02, Mogens Jensen wrote:
> > ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
> > On Tuesday, October 30, 2018 7:10 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> > 
> > > Hi,
> > > 
> > > On 30-10-18 19:56, Mogens Jensen wrote:
> > > 
> > > > ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
> > > > On Tuesday, October 30, 2018 4:04 PM, Hans de Goede hdegoede@redhat.com wrote:
> > > > 
> > > > > Hi,
> > > > > On 30-10-18 16:46, Hans de Goede wrote:
> > > > > 
> > > > > > Hi,
> > > > > > On 30-10-18 16:04, Pierre-Louis Bossart wrote:
> > > > > > 
> > > > > > > In addition I am not aware of any baytrail device using plt_clk_0, so moving a common machine driver such a cht_bsw_max98090_ti to use plt_clk0 only would break other devices (e.g. Rambi/Orco). Asking for both clocks to be on might work though,
> > > > > > 
> > > > > > Ok, so we need to have a DMI based quirk for the Swanky and maybe also
> > > > > > the clapper to use plt_clk_0 there. Asking for 2 clks if we only need
> > > > > > one does not seem like a good plan.
> > > > > 
> > > > > Dean, Mogens,
> > > > > To write a proper patch for this I'm going to need DMI strings
> > > > > from your devices.
> > > > > Can you please run (as normal user):
> > > > > grep . /sys/class/dmi/id/* 2> /dev/null
> > > > > And reply with the output of this command?
> > > > > I have attached the output from a coreboot seabios based clapper.
> > > 
> > > Thank you.
> > > 
> > > > Should I still test 0001-ASoC-intel-cht_bsw_max98090_ti-Use-pmc_plt_clk_0-ins.patch with SND_SOC_INTEL_CHT_BSW_MAX98090_TI_MACH and asoundrc from Dean? There seems to have been some development in the case since that request was made.
> > > 
> > > Yes please test that, I expect that to also fix things for the
> > > Clapper, but I need to have that confirmed before submitting a
> > > patch upstream adding a quirk for the Clapper to use pmc_plt_clk_0
> > > instead of pmc_plt_clk_3.
> > > 
> > > Regards,
> > > 
> > > Hans
> > > 
> > Unfortunately I only have access to longterm kernel 4.14 for building/running on this system, and 0001-ASoC-intel-cht_bsw_max98090_ti-Use-pmc_plt_clk_0-ins.patch does not patch against 4.14.78. Can a test patch for 4.14 be created?
> 
> Can you run (as root):
> 
> for i in /sys/kernel/debug/clk/pmc_plt_clk_?; do echo -n "$i: "; cat $i/clk_flags; echo; done
> 
> When running a kernel with working audio?
> 
> Then I can confirm that the Clapper is also using pmc_plt_clk_0, so that I can
> fix this for the clapper for 4.18+
> 
> I've just checked the 4.14 sources and in 4.14 the SND_SOC_INTEL_CHT_BSW_MAX98090_TI_MACH
> driver does not support mclk control yet, so for the 4.14 kernel the only way to
> fix this is to revert the 648e921888ad ("clk: x86: Stop marking clocks as CLK_IS_CRITICAL")
> commit.
> 
> Regards,
> 
> Hans
> 

Hi guys,
Morgens, If you can't test a recent kernel on your Clapper, I might be able to
give you a 'live' usb to boot with the kernel on, so you can quickly
test.  It's basically just archlinux iso with xfce and tools, with arch's
LTS and latest stable kernels.  If it will help.
-- 
Dean

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

[Hans de Goede]

[-- Attachment #1: Type: text/plain, Size: 1400 bytes --]

Hi,

On 30-10-18 17:15, Dean Wallace wrote:
> On 30-10-18, Hans de Goede wrote:
>> Hi,
>>
>> On 30-10-18 16:46, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 30-10-18 16:04, Pierre-Louis Bossart wrote:
>>>> In addition I am not aware of any baytrail device using plt_clk_0,
>>>> so moving a common machine driver such a cht_bsw_max98090_ti to use
>>>> plt_clk0 only would break other devices (e.g. Rambi/Orco). Asking
>>>> for both clocks to be on might work though,
>>>
>>> Ok, so we need to have a DMI based quirk for the Swanky and maybe also
>>> the clapper to use plt_clk_0 there. Asking for 2 clks if we only need
>>> one does not seem like a good plan.
>>
>> Dean, Mogens,
>>
>> To write a proper patch for this I'm going to need DMI strings
>> from your devices.
>>
>> Can you please run (as normal user):
>>
>> grep . /sys/class/dmi/id/* 2> /dev/null
>>
>> And reply with the output of this command?
> Here's mine, for a coreboot uefi based swanky.
> 
> https://ptpb.pw/~swanky-dmi-log

Thanks, can you give the attached patch a try. This does the same
as the previous one you tested, but then only on the Swanky.

Note I've added:

Reported-and-tested-by: Dean Wallace <duffydack73@gmail.com>

To the commit message, I hope that is ok with you, if not let me
know and I will drop it.

Once you can confirm that this version also fixes things I will
submit this version upstream.

Regards,

Hans


[-- Attachment #2: 0001-ASoC-intel-cht_bsw_max98090_ti-Add-quirk-for-boards-.patch --]
[-- Type: text/x-patch, Size: 3170 bytes --]

From 0413a62e99a00e77392011a07c9392fc132a9b15 Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@redhat.com>
Date: Wed, 31 Oct 2018 11:35:52 +0100
Subject: [PATCH v2] ASoC: intel: cht_bsw_max98090_ti: Add quirk for boards
 using pmc_plt_clk_0

Some boards such as the Swanky model Chromebooks use pmc_plt_clk_0 for the
mclk instead of pmc_plt_clk_3.

This commit adds a DMI based quirk for this.

This fixing audio no longer working on these devices after
commit 648e921888ad ("clk: x86: Stop marking clocks as CLK_IS_CRITICAL")
that commit fixes us unnecessary keeping unused clocks on, but in case
of the Swanky that was breaking audio support since we were not using
the right clock in the cht_bsw_max98090_ti machine driver.

Cc: stable@vger.kernel.org
Fixes: 648e921888ad ("clk: x86: Stop marking clocks as CLK_IS_CRITICAL")
Reported-and-tested-by: Dean Wallace <duffydack73@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 sound/soc/intel/boards/cht_bsw_max98090_ti.c | 32 ++++++++++++++++++--
 1 file changed, 29 insertions(+), 3 deletions(-)

diff --git a/sound/soc/intel/boards/cht_bsw_max98090_ti.c b/sound/soc/intel/boards/cht_bsw_max98090_ti.c
index db6976f4ddaa..9d9f6e41d81c 100644
--- a/sound/soc/intel/boards/cht_bsw_max98090_ti.c
+++ b/sound/soc/intel/boards/cht_bsw_max98090_ti.c
@@ -19,6 +19,7 @@
  * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  */
 
+#include <linux/dmi.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
@@ -35,6 +36,8 @@
 #define CHT_PLAT_CLK_3_HZ	19200000
 #define CHT_CODEC_DAI	"HiFi"
 
+#define QUIRK_PMC_PLT_CLK_0				0x01
+
 struct cht_mc_private {
 	struct clk *mclk;
 	struct snd_soc_jack jack;
@@ -385,11 +388,29 @@ static struct snd_soc_card snd_soc_card_cht = {
 	.num_controls = ARRAY_SIZE(cht_mc_controls),
 };
 
+static const struct dmi_system_id cht_max98090_quirk_table[] = {
+	{
+		/* Swanky model Chromebook (Toshiba Chromebook 2) */
+		.matches = {
+			DMI_MATCH(DMI_PRODUCT_NAME, "Swanky"),
+		},
+		.driver_data = (void *)QUIRK_PMC_PLT_CLK_0,
+	},
+	{}
+};
+
 static int snd_cht_mc_probe(struct platform_device *pdev)
 {
+	const struct dmi_system_id *dmi_id;
 	struct device *dev = &pdev->dev;
 	int ret_val = 0;
 	struct cht_mc_private *drv;
+	const char *mclk_name;
+	int quirks = 0;
+
+	dmi_id = dmi_first_match(cht_max98090_quirk_table);
+	if (dmi_id)
+		quirks = (unsigned long)dmi_id->driver_data;
 
 	drv = devm_kzalloc(&pdev->dev, sizeof(*drv), GFP_KERNEL);
 	if (!drv)
@@ -411,11 +432,16 @@ static int snd_cht_mc_probe(struct platform_device *pdev)
 	snd_soc_card_cht.dev = &pdev->dev;
 	snd_soc_card_set_drvdata(&snd_soc_card_cht, drv);
 
-	drv->mclk = devm_clk_get(&pdev->dev, "pmc_plt_clk_3");
+	if (quirks & QUIRK_PMC_PLT_CLK_0)
+		mclk_name = "pmc_plt_clk_0";
+	else
+		mclk_name = "pmc_plt_clk_3";
+
+	drv->mclk = devm_clk_get(&pdev->dev, mclk_name);
 	if (IS_ERR(drv->mclk)) {
 		dev_err(&pdev->dev,
-			"Failed to get MCLK from pmc_plt_clk_3: %ld\n",
-			PTR_ERR(drv->mclk));
+			"Failed to get MCLK from %s: %ld\n",
+			mclk_name, PTR_ERR(drv->mclk));
 		return PTR_ERR(drv->mclk);
 	}
 
-- 
2.19.0

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

[Dean Wallace]

On 31-10-18, Hans de Goede wrote:
> Thanks, can you give the attached patch a try. This does the same
> as the previous one you tested, but then only on the Swanky.
> 
> Note I've added:
> 
> Reported-and-tested-by: Dean Wallace <duffydack73@gmail.com>
> 
> To the commit message, I hope that is ok with you, if not let me
> know and I will drop it.
> 
> Once you can confirm that this version also fixes things I will
> submit this version upstream.
> 
> Regards,
> 
Thanks, I don't mind at all.  I can confirm 4.19 compiled with your
quirk has working sound.
-- 
Dean

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

[Dean Wallace]

On 31-10-18, Hans de Goede wrote:
> Hi,
> 
> On 30-10-18 17:15, Dean Wallace wrote:
> > On 30-10-18, Hans de Goede wrote:
> > > Hi,
> > > 
> > > On 30-10-18 16:46, Hans de Goede wrote:
> > > > Hi,
> > > > 
> > > > On 30-10-18 16:04, Pierre-Louis Bossart wrote:
> > > > > In addition I am not aware of any baytrail device using plt_clk_0,
> > > > > so moving a common machine driver such a cht_bsw_max98090_ti to use
> > > > > plt_clk0 only would break other devices (e.g. Rambi/Orco). Asking
> > > > > for both clocks to be on might work though,
> > > > 
> > > > Ok, so we need to have a DMI based quirk for the Swanky and maybe also
> > > > the clapper to use plt_clk_0 there. Asking for 2 clks if we only need
> > > > one does not seem like a good plan.
> > > 
> > > Dean, Mogens,
> > > 
> > > To write a proper patch for this I'm going to need DMI strings
> > > from your devices.
> > > 
> > > Can you please run (as normal user):
> > > 
> > > grep . /sys/class/dmi/id/* 2> /dev/null
> > > 
> > > And reply with the output of this command?
> > Here's mine, for a coreboot uefi based swanky.
> > 
> > https://ptpb.pw/~swanky-dmi-log
> 
> Thanks, can you give the attached patch a try. This does the same
> as the previous one you tested, but then only on the Swanky.
> 
> Note I've added:
> 
> Reported-and-tested-by: Dean Wallace <duffydack73@gmail.com>
> 
> To the commit message, I hope that is ok with you, if not let me
> know and I will drop it.
> 
> Once you can confirm that this version also fixes things I will
> submit this version upstream.
> 
> Regards,
> 
> Hans
> 

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.

regards
-- 
Dean

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

[Pierre-Louis Bossart]

> 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? Are you 100% sure this was not present in the previous 
install you used (4.18.14 as mentioned earlier in the thread)?

Thanks

-Pierre

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

[Dean Wallace]

On 31-10-18, 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? Are you 100% sure this was not present in the previous install
> you used (4.18.14 as mentioned earlier in the thread)?
> 
> Thanks
> 
> -Pierre
> 
Hi Pierre.  I'm not overly bothered by it, I lived with it for a long
time when it was worse pre cht_bsw days.  It's very hit and miss, like
I have just done a few reboots and nothing is coming up in journal and
switching to headphones works fine.  Was quite a frequent issue back
before it was switched to chtmax, but after that, for me anyway, it
was all but nonexistant, apart from a very rare occassion once maybe
twice.  It's always been slightly unstable I guess, but seeing those
messages in journal, something I look at frequently, and not seeing
them for a long time, hmmm.  I'll do some more tests with older kernel
see how it compares and report back.  Then again, I could just ignore
it :)
Regards

-Dean

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

[Mogens Jensen]

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Wednesday, October 31, 2018 9:29 AM, Hans de Goede <hdegoede@redhat.com> wrote:

> Hi,
>
> On 31-10-18 07:02, Mogens Jensen wrote:
>
> > ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
> > On Tuesday, October 30, 2018 7:10 PM, Hans de Goede hdegoede@redhat.com wrote:
> >
> > > Hi,
> > > On 30-10-18 19:56, Mogens Jensen wrote:
> > >
> > > > ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
> > > > On Tuesday, October 30, 2018 4:04 PM, Hans de Goede hdegoede@redhat.com wrote:
> > > >
> > > > > Hi,
> > > > > On 30-10-18 16:46, Hans de Goede wrote:
> > > > >
> > > > > > Hi,
> > > > > > On 30-10-18 16:04, Pierre-Louis Bossart wrote:
> > > > > >
> > > > > > > In addition I am not aware of any baytrail device using plt_clk_0, so moving a common machine driver such a cht_bsw_max98090_ti to use plt_clk0 only would break other devices (e.g. Rambi/Orco). Asking for both clocks to be on might work though,
> > > > > >
> > > > > > Ok, so we need to have a DMI based quirk for the Swanky and maybe also
> > > > > > the clapper to use plt_clk_0 there. Asking for 2 clks if we only need
> > > > > > one does not seem like a good plan.
> > > > >
> > > > > Dean, Mogens,
> > > > > To write a proper patch for this I'm going to need DMI strings
> > > > > from your devices.
> > > > > Can you please run (as normal user):
> > > > > grep . /sys/class/dmi/id/* 2> /dev/null
> > > > > And reply with the output of this command?
> > > > > I have attached the output from a coreboot seabios based clapper.
> > >
> > > Thank you.
> > >
> > > > Should I still test 0001-ASoC-intel-cht_bsw_max98090_ti-Use-pmc_plt_clk_0-ins.patch with SND_SOC_INTEL_CHT_BSW_MAX98090_TI_MACH and asoundrc from Dean? There seems to have been some development in the case since that request was made.
> > >
> > > Yes please test that, I expect that to also fix things for the
> > > Clapper, but I need to have that confirmed before submitting a
> > > patch upstream adding a quirk for the Clapper to use pmc_plt_clk_0
> > > instead of pmc_plt_clk_3.
> > > Regards,
> > > Hans
> >
> > Unfortunately I only have access to longterm kernel 4.14 for building/running on this system, and 0001-ASoC-intel-cht_bsw_max98090_ti-Use-pmc_plt_clk_0-ins.patch does not patch against 4.14.78. Can a test patch for 4.14 be created?
>
> Can you run (as root):
>
> for i in /sys/kernel/debug/clk/pmc_plt_clk_?; do echo -n "$i: "; cat $i/clk_flags; echo; done
>
> When running a kernel with working audio?
>
> Then I can confirm that the Clapper is also using pmc_plt_clk_0, so that I can
> fix this for the clapper for 4.18+
>
> I've just checked the 4.14 sources and in 4.14 the SND_SOC_INTEL_CHT_BSW_MAX98090_TI_MACH
> driver does not support mclk control yet, so for the 4.14 kernel the only way to
> fix this is to revert the 648e921888ad ("clk: x86: Stop marking clocks as CLK_IS_CRITICAL")
> commit.
>
> Regards,
>
> Hans
>
Here is the output from the Clapper with 4.14.78 and working sound:

/sys/kernel/debug/clk/pmc_plt_clk_0: 0x00000800
/sys/kernel/debug/clk/pmc_plt_clk_1: 0x00000000
/sys/kernel/debug/clk/pmc_plt_clk_2: 0x00000000
/sys/kernel/debug/clk/pmc_plt_clk_3: 0x00000000
/sys/kernel/debug/clk/pmc_plt_clk_4: 0x00000000
/sys/kernel/debug/clk/pmc_plt_clk_5: 0x00000000

Will 648e921888ad ("clk: x86: Stop marking clocks as CLK_IS_CRITICAL") be reverted upstream for 4.14?

Dean, Thank you, but I will see if I can change the build system to accept a newer kernel, if I get some time later today.

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

[Dean Wallace]

On 31-10-18, 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? Are you 100% sure this was not present in the previous install
> you used (4.18.14 as mentioned earlier in the thread)?
> 
> Thanks
> 
> -Pierre
> 
Well, numerous boots, kernels, headphone inserting - no PLL or
'Daleks'.  My laptop must have been haunted that day (halloween).
I'll put it to bed.

-- 
Dean

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

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

[Hans de Goede]

Hi,

On 01-11-18 11:37, Dean Wallace wrote:
> On 31-10-18, 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? Are you 100% sure this was not present in the previous install
>> you used (4.18.14 as mentioned earlier in the thread)?
>>
>> Thanks
>>
>> -Pierre
>>
> Well, numerous boots, kernels, headphone inserting - no PLL or
> 'Daleks'.  My laptop must have been haunted that day (halloween).
> I'll put it to bed.

So you can no longer reproduce. Bummer. Note this might be caused by
the temperature of the laptop when you were running the tests...

Anyways if you hit this again and you can reproduce it, please
give adding a msleep(10) after code mucking with the clk a try.

Regards,

Hans

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

[Dean Wallace]

On 01-11-18, Hans de Goede wrote:
> Hi,
> 
> On 01-11-18 11:37, Dean Wallace wrote:
> > On 31-10-18, 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? Are you 100% sure this was not present in the previous install
> > > you used (4.18.14 as mentioned earlier in the thread)?
> > > 
> > > Thanks
> > > 
> > > -Pierre
> > > 
> > Well, numerous boots, kernels, headphone inserting - no PLL or
> > 'Daleks'.  My laptop must have been haunted that day (halloween).
> > I'll put it to bed.
> 
> So you can no longer reproduce. Bummer. Note this might be caused by
> the temperature of the laptop when you were running the tests...
> 
> Anyways if you hit this again and you can reproduce it, please
> give adding a msleep(10) after code mucking with the clk a try.
> 
> Regards,
> 
> Hans
> 
Right then, I can make it unlock and 'daleks' by going into
pavucontrol and switching the Profile back and forth from Stereo
Output to Stereo Output+Analog Mono Input, which is actually something
I've done to make it correct itself as well.  I don't use the mic or
anything so I've had it set to Stereo Ouput only which I 'think' has
somehow made it more stable for me.  With all my playing around, one
of the things I did was clean out my .config/pulse folder which meant
by default the 'Profile' in pavucontrol was set to Output+Input, which
seems to help trigger the PLL issue when inserting headphones.

So what would you like me to do, as I can trigger it on demand it
seems.
-- 
Dean

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

[Hans de Goede]

[-- Attachment #1: Type: text/plain, Size: 3305 bytes --]

Hi,

On 01-11-18 15:28, Dean Wallace wrote:
> On 01-11-18, Hans de Goede wrote:
>> Hi,
>>
>> On 01-11-18 11:37, Dean Wallace wrote:
>>> On 31-10-18, 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? Are you 100% sure this was not present in the previous install
>>>> you used (4.18.14 as mentioned earlier in the thread)?
>>>>
>>>> Thanks
>>>>
>>>> -Pierre
>>>>
>>> Well, numerous boots, kernels, headphone inserting - no PLL or
>>> 'Daleks'.  My laptop must have been haunted that day (halloween).
>>> I'll put it to bed.
>>
>> So you can no longer reproduce. Bummer. Note this might be caused by
>> the temperature of the laptop when you were running the tests...
>>
>> Anyways if you hit this again and you can reproduce it, please
>> give adding a msleep(10) after code mucking with the clk a try.
>>
>> Regards,
>>
>> Hans
>>
> Right then, I can make it unlock and 'daleks' by going into
> pavucontrol and switching the Profile back and forth from Stereo
> Output to Stereo Output+Analog Mono Input, which is actually something
> I've done to make it correct itself as well.  I don't use the mic or
> anything so I've had it set to Stereo Ouput only which I 'think' has
> somehow made it more stable for me.  With all my playing around, one
> of the things I did was clean out my .config/pulse folder which meant
> by default the 'Profile' in pavucontrol was set to Output+Input, which
> seems to help trigger the PLL issue when inserting headphones.
> 
> So what would you like me to do, as I can trigger it on demand it
> seems.

Please give the attached patch a try (on top of my patch for the clk quirk)
and let us know if that fixes these errors.

Regards,

Hans


[-- Attachment #2: 0001-ASoC-intel-cht_bsw_max98090_ti-Sleep-a-bit-after-ena.patch --]
[-- Type: text/x-patch, Size: 1257 bytes --]

From d81f1906f08bb6d3aa45a0403adc1d36754c13cc Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@redhat.com>
Date: Thu, 1 Nov 2018 15:47:42 +0100
Subject: [PATCH] ASoC: intel: cht_bsw_max98090_ti: Sleep a bit after enabling
 the mclk

Sleep a bit after enabling the mclk to give the PLL some time to lock,
this fixes the log showing a whole bunch of:

kernel: max98090 i2c-193C9890:00: PLL unlocked

errors.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 sound/soc/intel/boards/cht_bsw_max98090_ti.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/sound/soc/intel/boards/cht_bsw_max98090_ti.c b/sound/soc/intel/boards/cht_bsw_max98090_ti.c
index 9d9f6e41d81c..f3bc0bfdfe08 100644
--- a/sound/soc/intel/boards/cht_bsw_max98090_ti.c
+++ b/sound/soc/intel/boards/cht_bsw_max98090_ti.c
@@ -19,6 +19,7 @@
  * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  */
 
+#include <linux/delay.h>
 #include <linux/dmi.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
@@ -66,6 +67,7 @@ static int platform_clock_control(struct snd_soc_dapm_widget *w,
 				"could not configure MCLK state");
 			return ret;
 		}
+		usleep_range(5000, 10000);
 	} else {
 		clk_disable_unprepare(ctx->mclk);
 	}
-- 
2.19.0

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

[Dean Wallace]

On 01-11-18, Hans de Goede wrote:
> Hi,
> 
> On 01-11-18 15:28, Dean Wallace wrote:
> > On 01-11-18, Hans de Goede wrote:
> > > Hi,
> > > 
> > > On 01-11-18 11:37, Dean Wallace wrote:
> > > > On 31-10-18, 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? Are you 100% sure this was not present in the previous install
> > > > > you used (4.18.14 as mentioned earlier in the thread)?
> > > > > 
> > > > > Thanks
> > > > > 
> > > > > -Pierre
> > > > > 
> > > > Well, numerous boots, kernels, headphone inserting - no PLL or
> > > > 'Daleks'.  My laptop must have been haunted that day (halloween).
> > > > I'll put it to bed.
> > > 
> > > So you can no longer reproduce. Bummer. Note this might be caused by
> > > the temperature of the laptop when you were running the tests...
> > > 
> > > Anyways if you hit this again and you can reproduce it, please
> > > give adding a msleep(10) after code mucking with the clk a try.
> > > 
> > > Regards,
> > > 
> > > Hans
> > > 
> > Right then, I can make it unlock and 'daleks' by going into
> > pavucontrol and switching the Profile back and forth from Stereo
> > Output to Stereo Output+Analog Mono Input, which is actually something
> > I've done to make it correct itself as well.  I don't use the mic or
> > anything so I've had it set to Stereo Ouput only which I 'think' has
> > somehow made it more stable for me.  With all my playing around, one
> > of the things I did was clean out my .config/pulse folder which meant
> > by default the 'Profile' in pavucontrol was set to Output+Input, which
> > seems to help trigger the PLL issue when inserting headphones.
> > 
> > So what would you like me to do, as I can trigger it on demand it
> > seems.
> 
> Please give the attached patch a try (on top of my patch for the clk quirk)
> and let us know if that fixes these errors.
> 
> Regards,
> 
> Hans
> 
Ok Hans, I tried your patch on top of the quirk, and sound is
immediately garbled (daleks again), and switching pulseuadio profiles
or inserting headphones doesn't make it correct itself, whereas before
sound would be fine, UNTIL, I did something like insert earphone etc.

-Dean

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

[Dean Wallace]

On 01-11-18, Hans de Goede wrote:
> Hi,
> 
> On 01-11-18 15:28, Dean Wallace wrote:
> > On 01-11-18, Hans de Goede wrote:
> > > Hi,
> > > 
> > > On 01-11-18 11:37, Dean Wallace wrote:
> > > > On 31-10-18, 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? Are you 100% sure this was not present in the previous install
> > > > > you used (4.18.14 as mentioned earlier in the thread)?
> > > > > 
> > > > > Thanks
> > > > > 
> > > > > -Pierre
> > > > > 
> > > > Well, numerous boots, kernels, headphone inserting - no PLL or
> > > > 'Daleks'.  My laptop must have been haunted that day (halloween).
> > > > I'll put it to bed.
> > > 
> > > So you can no longer reproduce. Bummer. Note this might be caused by
> > > the temperature of the laptop when you were running the tests...
> > > 
> > > Anyways if you hit this again and you can reproduce it, please
> > > give adding a msleep(10) after code mucking with the clk a try.
> > > 
> > > Regards,
> > > 
> > > Hans
> > > 
> > Right then, I can make it unlock and 'daleks' by going into
> > pavucontrol and switching the Profile back and forth from Stereo
> > Output to Stereo Output+Analog Mono Input, which is actually something
> > I've done to make it correct itself as well.  I don't use the mic or
> > anything so I've had it set to Stereo Ouput only which I 'think' has
> > somehow made it more stable for me.  With all my playing around, one
> > of the things I did was clean out my .config/pulse folder which meant
> > by default the 'Profile' in pavucontrol was set to Output+Input, which
> > seems to help trigger the PLL issue when inserting headphones.
> > 
> > So what would you like me to do, as I can trigger it on demand it
> > seems.
> 
> Please give the attached patch a try (on top of my patch for the clk quirk)
> and let us know if that fixes these errors.
> 
> Regards,
> 
> Hans
> 
This is weird, now sound is ok on boot, until I plug in earphones,
like before.  Still get spammed with errors until I fool around with
pulse profile switching (it's hit and miss).

-Dean

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

[Dean Wallace]

On 01-11-18, Hans de Goede wrote:
> Hi,
> 
> On 01-11-18 15:28, Dean Wallace wrote:
> > On 01-11-18, Hans de Goede wrote:
> > > Hi,
> > > 
> > > On 01-11-18 11:37, Dean Wallace wrote:
> > > > On 31-10-18, 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? Are you 100% sure this was not present in the previous install
> > > > > you used (4.18.14 as mentioned earlier in the thread)?
> > > > > 
> > > > > Thanks
> > > > > 
> > > > > -Pierre
> > > > > 
> > > > Well, numerous boots, kernels, headphone inserting - no PLL or
> > > > 'Daleks'.  My laptop must have been haunted that day (halloween).
> > > > I'll put it to bed.
> > > 
> > > So you can no longer reproduce. Bummer. Note this might be caused by
> > > the temperature of the laptop when you were running the tests...
> > > 
> > > Anyways if you hit this again and you can reproduce it, please
> > > give adding a msleep(10) after code mucking with the clk a try.
> > > 
> > > Regards,
> > > 
> > > Hans
> > > 
> > Right then, I can make it unlock and 'daleks' by going into
> > pavucontrol and switching the Profile back and forth from Stereo
> > Output to Stereo Output+Analog Mono Input, which is actually something
> > I've done to make it correct itself as well.  I don't use the mic or
> > anything so I've had it set to Stereo Ouput only which I 'think' has
> > somehow made it more stable for me.  With all my playing around, one
> > of the things I did was clean out my .config/pulse folder which meant
> > by default the 'Profile' in pavucontrol was set to Output+Input, which
> > seems to help trigger the PLL issue when inserting headphones.
> > 
> > So what would you like me to do, as I can trigger it on demand it
> > seems.
> 
> Please give the attached patch a try (on top of my patch for the clk quirk)
> and let us know if that fixes these errors.
> 
> Regards,
> 
> Hans
> 
Sorry, it's not being consistent with me  Now, fresh boot again, no errors
in journal, no errors while plugging/unplugging earphones.  I think it's
definitely more stable with the profile in pavucontrol set to Output
only (no + Input), but I can still trigger it by switching it, and it
never corrects itself until I switch a few more times.

-Dean

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

[Hans de Goede]

Hi,

On 01-11-18 16:50, Dean Wallace wrote:
> On 01-11-18, Hans de Goede wrote:
>> Hi,
>>
>> On 01-11-18 15:28, Dean Wallace wrote:
>>> On 01-11-18, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 01-11-18 11:37, Dean Wallace wrote:
>>>>> On 31-10-18, 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? Are you 100% sure this was not present in the previous install
>>>>>> you used (4.18.14 as mentioned earlier in the thread)?
>>>>>>
>>>>>> Thanks
>>>>>>
>>>>>> -Pierre
>>>>>>
>>>>> Well, numerous boots, kernels, headphone inserting - no PLL or
>>>>> 'Daleks'.  My laptop must have been haunted that day (halloween).
>>>>> I'll put it to bed.
>>>>
>>>> So you can no longer reproduce. Bummer. Note this might be caused by
>>>> the temperature of the laptop when you were running the tests...
>>>>
>>>> Anyways if you hit this again and you can reproduce it, please
>>>> give adding a msleep(10) after code mucking with the clk a try.
>>>>
>>>> Regards,
>>>>
>>>> Hans
>>>>
>>> Right then, I can make it unlock and 'daleks' by going into
>>> pavucontrol and switching the Profile back and forth from Stereo
>>> Output to Stereo Output+Analog Mono Input, which is actually something
>>> I've done to make it correct itself as well.  I don't use the mic or
>>> anything so I've had it set to Stereo Ouput only which I 'think' has
>>> somehow made it more stable for me.  With all my playing around, one
>>> of the things I did was clean out my .config/pulse folder which meant
>>> by default the 'Profile' in pavucontrol was set to Output+Input, which
>>> seems to help trigger the PLL issue when inserting headphones.
>>>
>>> So what would you like me to do, as I can trigger it on demand it
>>> seems.
>>
>> Please give the attached patch a try (on top of my patch for the clk quirk)
>> and let us know if that fixes these errors.
>>
>> Regards,
>>
>> Hans
>>
> Sorry, it's not being consistent with me  Now, fresh boot again, no errors
> in journal, no errors while plugging/unplugging earphones.  I think it's
> definitely more stable with the profile in pavucontrol set to Output
> only (no + Input), but I can still trigger it by switching it, and it
> never corrects itself until I switch a few more times.

Ok, so there are 2 issues here as I see it:

1) It does not always reproduce
2) It does still reproduce sometimes with the patch, so the patch does not
fix it.

So I think we just have to live with this for now.

Regards,

Hans

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

[Dean Wallace]

On 02-11-18, Hans de Goede wrote:
> Hi,
> 
> On 01-11-18 16:50, Dean Wallace wrote:
> > On 01-11-18, Hans de Goede wrote:
> > > Hi,
> > > 
> > > On 01-11-18 15:28, Dean Wallace wrote:
> > > > On 01-11-18, Hans de Goede wrote:
> > > > > Hi,
> > > > > 
> > > > > On 01-11-18 11:37, Dean Wallace wrote:
> > > > > > On 31-10-18, 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? Are you 100% sure this was not present in the previous install
> > > > > > > you used (4.18.14 as mentioned earlier in the thread)?
> > > > > > > 
> > > > > > > Thanks
> > > > > > > 
> > > > > > > -Pierre
> > > > > > > 
> > > > > > Well, numerous boots, kernels, headphone inserting - no PLL or
> > > > > > 'Daleks'.  My laptop must have been haunted that day (halloween).
> > > > > > I'll put it to bed.
> > > > > 
> > > > > So you can no longer reproduce. Bummer. Note this might be caused by
> > > > > the temperature of the laptop when you were running the tests...
> > > > > 
> > > > > Anyways if you hit this again and you can reproduce it, please
> > > > > give adding a msleep(10) after code mucking with the clk a try.
> > > > > 
> > > > > Regards,
> > > > > 
> > > > > Hans
> > > > > 
> > > > Right then, I can make it unlock and 'daleks' by going into
> > > > pavucontrol and switching the Profile back and forth from Stereo
> > > > Output to Stereo Output+Analog Mono Input, which is actually something
> > > > I've done to make it correct itself as well.  I don't use the mic or
> > > > anything so I've had it set to Stereo Ouput only which I 'think' has
> > > > somehow made it more stable for me.  With all my playing around, one
> > > > of the things I did was clean out my .config/pulse folder which meant
> > > > by default the 'Profile' in pavucontrol was set to Output+Input, which
> > > > seems to help trigger the PLL issue when inserting headphones.
> > > > 
> > > > So what would you like me to do, as I can trigger it on demand it
> > > > seems.
> > > 
> > > Please give the attached patch a try (on top of my patch for the clk quirk)
> > > and let us know if that fixes these errors.
> > > 
> > > Regards,
> > > 
> > > Hans
> > > 
> > Sorry, it's not being consistent with me  Now, fresh boot again, no errors
> > in journal, no errors while plugging/unplugging earphones.  I think it's
> > definitely more stable with the profile in pavucontrol set to Output
> > only (no + Input), but I can still trigger it by switching it, and it
> > never corrects itself until I switch a few more times.
> 
> Ok, so there are 2 issues here as I see it:
> 
> 1) It does not always reproduce
> 2) It does still reproduce sometimes with the patch, so the patch does not
> fix it.
> 
> So I think we just have to live with this for now.
> 
> Regards,
> 
> Hans
>
Yeah, I've never known it act any different so I'm used to it.  I do
know a few people in the past who have just bought a usb sound device
because they want to use a mic as well, and that just didn't work
quite right for them, even if sound output worked they had issues as
soon as they tried using mic.  I forget which, 4.1 or 4.4, 1 guy
mentioned still using it just because output/input worked flawlessly.
¯\_(ツ)_/¯

-Dean

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

[Hans de Goede]

Hi,

On 01-11-18 07:55, Mogens Jensen wrote:
> ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
> On Wednesday, October 31, 2018 9:29 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> 
>> Hi,
>>
>> On 31-10-18 07:02, Mogens Jensen wrote:
>>
>>> ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
>>> On Tuesday, October 30, 2018 7:10 PM, Hans de Goede hdegoede@redhat.com wrote:
>>>
>>>> Hi,
>>>> On 30-10-18 19:56, Mogens Jensen wrote:
>>>>
>>>>> ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
>>>>> On Tuesday, October 30, 2018 4:04 PM, Hans de Goede hdegoede@redhat.com wrote:
>>>>>
>>>>>> Hi,
>>>>>> On 30-10-18 16:46, Hans de Goede wrote:
>>>>>>
>>>>>>> Hi,
>>>>>>> On 30-10-18 16:04, Pierre-Louis Bossart wrote:
>>>>>>>
>>>>>>>> In addition I am not aware of any baytrail device using plt_clk_0, so moving a common machine driver such a cht_bsw_max98090_ti to use plt_clk0 only would break other devices (e.g. Rambi/Orco). Asking for both clocks to be on might work though,
>>>>>>>
>>>>>>> Ok, so we need to have a DMI based quirk for the Swanky and maybe also
>>>>>>> the clapper to use plt_clk_0 there. Asking for 2 clks if we only need
>>>>>>> one does not seem like a good plan.
>>>>>>
>>>>>> Dean, Mogens,
>>>>>> To write a proper patch for this I'm going to need DMI strings
>>>>>> from your devices.
>>>>>> Can you please run (as normal user):
>>>>>> grep . /sys/class/dmi/id/* 2> /dev/null
>>>>>> And reply with the output of this command?
>>>>>> I have attached the output from a coreboot seabios based clapper.
>>>>
>>>> Thank you.
>>>>
>>>>> Should I still test 0001-ASoC-intel-cht_bsw_max98090_ti-Use-pmc_plt_clk_0-ins.patch with SND_SOC_INTEL_CHT_BSW_MAX98090_TI_MACH and asoundrc from Dean? There seems to have been some development in the case since that request was made.
>>>>
>>>> Yes please test that, I expect that to also fix things for the
>>>> Clapper, but I need to have that confirmed before submitting a
>>>> patch upstream adding a quirk for the Clapper to use pmc_plt_clk_0
>>>> instead of pmc_plt_clk_3.
>>>> Regards,
>>>> Hans
>>>
>>> Unfortunately I only have access to longterm kernel 4.14 for building/running on this system, and 0001-ASoC-intel-cht_bsw_max98090_ti-Use-pmc_plt_clk_0-ins.patch does not patch against 4.14.78. Can a test patch for 4.14 be created?
>>
>> Can you run (as root):
>>
>> for i in /sys/kernel/debug/clk/pmc_plt_clk_?; do echo -n "$i: "; cat $i/clk_flags; echo; done
>>
>> When running a kernel with working audio?
>>
>> Then I can confirm that the Clapper is also using pmc_plt_clk_0, so that I can
>> fix this for the clapper for 4.18+
>>
>> I've just checked the 4.14 sources and in 4.14 the SND_SOC_INTEL_CHT_BSW_MAX98090_TI_MACH
>> driver does not support mclk control yet, so for the 4.14 kernel the only way to
>> fix this is to revert the 648e921888ad ("clk: x86: Stop marking clocks as CLK_IS_CRITICAL")
>> commit.
>>
>> Regards,
>>
>> Hans
>>
> Here is the output from the Clapper with 4.14.78 and working sound:
> 
> /sys/kernel/debug/clk/pmc_plt_clk_0: 0x00000800
> /sys/kernel/debug/clk/pmc_plt_clk_1: 0x00000000
> /sys/kernel/debug/clk/pmc_plt_clk_2: 0x00000000
> /sys/kernel/debug/clk/pmc_plt_clk_3: 0x00000000
> /sys/kernel/debug/clk/pmc_plt_clk_4: 0x00000000
> /sys/kernel/debug/clk/pmc_plt_clk_5: 0x00000000

Ok, so your Clapper model indeed is also using clk 0 and not clk 3 as
expected. I've just submitted a patch upstream adding a quirk for this.

As for what the plan is with 4.14, I don't know. I believe that
reverting the commit causing the issue there is fine.

Regards,

Hans

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

[Mogens Jensen]

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Sunday, December 2, 2018 12:25 PM, Hans de Goede <hdegoede@redhat.com> wrote:

> Hi,
>
> On 01-11-18 07:55, Mogens Jensen wrote:
>
> > ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
> > On Wednesday, October 31, 2018 9:29 AM, Hans de Goede hdegoede@redhat.com wrote:
> >
> > > Hi,
> > > On 31-10-18 07:02, Mogens Jensen wrote:
> > >
> > > > ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
> > > > On Tuesday, October 30, 2018 7:10 PM, Hans de Goede hdegoede@redhat.com wrote:
> > > >
> > > > > Hi,
> > > > > On 30-10-18 19:56, Mogens Jensen wrote:
> > > > >
> > > > > > ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
> > > > > > On Tuesday, October 30, 2018 4:04 PM, Hans de Goede hdegoede@redhat.com wrote:
> > > > > >
> > > > > > > Hi,
> > > > > > > On 30-10-18 16:46, Hans de Goede wrote:
> > > > > > >
> > > > > > > > Hi,
> > > > > > > > On 30-10-18 16:04, Pierre-Louis Bossart wrote:
> > > > > > > >
> > > > > > > > > In addition I am not aware of any baytrail device using plt_clk_0, so moving a common machine driver such a cht_bsw_max98090_ti to use plt_clk0 only would break other devices (e.g. Rambi/Orco). Asking for both clocks to be on might work though,
> > > > > > > >
> > > > > > > > Ok, so we need to have a DMI based quirk for the Swanky and maybe also
> > > > > > > > the clapper to use plt_clk_0 there. Asking for 2 clks if we only need
> > > > > > > > one does not seem like a good plan.
> > > > > > >
> > > > > > > Dean, Mogens,
> > > > > > > To write a proper patch for this I'm going to need DMI strings
> > > > > > > from your devices.
> > > > > > > Can you please run (as normal user):
> > > > > > > grep . /sys/class/dmi/id/* 2> /dev/null
> > > > > > > And reply with the output of this command?
> > > > > > > I have attached the output from a coreboot seabios based clapper.
> > > > >
> > > > > Thank you.
> > > > >
> > > > > > Should I still test 0001-ASoC-intel-cht_bsw_max98090_ti-Use-pmc_plt_clk_0-ins.patch with SND_SOC_INTEL_CHT_BSW_MAX98090_TI_MACH and asoundrc from Dean? There seems to have been some development in the case since that request was made.
> > > > >
> > > > > Yes please test that, I expect that to also fix things for the
> > > > > Clapper, but I need to have that confirmed before submitting a
> > > > > patch upstream adding a quirk for the Clapper to use pmc_plt_clk_0
> > > > > instead of pmc_plt_clk_3.
> > > > > Regards,
> > > > > Hans
> > > >
> > > > Unfortunately I only have access to longterm kernel 4.14 for building/running on this system, and 0001-ASoC-intel-cht_bsw_max98090_ti-Use-pmc_plt_clk_0-ins.patch does not patch against 4.14.78. Can a test patch for 4.14 be created?
> > >
> > > Can you run (as root):
> > > for i in /sys/kernel/debug/clk/pmc_plt_clk_?; do echo -n "$i: "; cat $i/clk_flags; echo; done
> > > When running a kernel with working audio?
> > > Then I can confirm that the Clapper is also using pmc_plt_clk_0, so that I can
> > > fix this for the clapper for 4.18+
> > > I've just checked the 4.14 sources and in 4.14 the SND_SOC_INTEL_CHT_BSW_MAX98090_TI_MACH
> > > driver does not support mclk control yet, so for the 4.14 kernel the only way to
> > > fix this is to revert the 648e921888ad ("clk: x86: Stop marking clocks as CLK_IS_CRITICAL")
> > > commit.
> > > Regards,
> > > Hans
> >
> > Here is the output from the Clapper with 4.14.78 and working sound:
> > /sys/kernel/debug/clk/pmc_plt_clk_0: 0x00000800
> > /sys/kernel/debug/clk/pmc_plt_clk_1: 0x00000000
> > /sys/kernel/debug/clk/pmc_plt_clk_2: 0x00000000
> > /sys/kernel/debug/clk/pmc_plt_clk_3: 0x00000000
> > /sys/kernel/debug/clk/pmc_plt_clk_4: 0x00000000
> > /sys/kernel/debug/clk/pmc_plt_clk_5: 0x00000000
>
> Ok, so your Clapper model indeed is also using clk 0 and not clk 3 as
> expected. I've just submitted a patch upstream adding a quirk for this.
>
> As for what the plan is with 4.14, I don't know. I believe that
> reverting the commit causing the issue there is fine.
>
> Regards,
>
> Hans

Hi,

I have now tried the recently released kernel 4.19.14 on my Chromebook Clapper as this version now contains the DMI quirk (commit 984bfb398a3af6fa9b7e80165e524933b0616686 upstream).

Kernel is compiled with SND_SOC_INTEL_CHT_BSW_MAX98090_TI_MACH and the quirk seems to have fixed the problem caused by commit 648e921888ad ("clk: x86: Stop marking clocks as CLK_IS_CRITICAL"), as sound is now working if running "speaker-test" on my system which is clean ALSA.

Unfortunately, SND_SOC_INTEL_CHT_BSW_MAX98090_TI_MACH driver is unusable on Clapper Chromebooks as audio played from everything but "speaker-test" as video players or web browsers is extremly low and sounds like played at 10x speed. At the same time kernel log is spammed with messages like this:

max98090 i2c-193C9890:00: PLL unlocked
intel_sst_acpi 80860F28:00: FW Version 01.0c.00.01
writing to lpe: 00000000: 01 01 01 01 00 00 08 00 ff ff ff ff 55 00 00 00  ............U...
writing to lpe: 00000000: 01 01 01 01 00 00 1a 00 ff ff ff ff 75 00 12 00  ............u...

This is probably not related to the problem discussed in this thread, but the result is that I have to use the legacy driver SND_SOC_INTEL_BYT_MAX98090_MACH and therefore still has to revert commit 648e921888ad for sound to work.

Is it possible to create a fix for SND_SOC_INTEL_BYT_MAX98090_MACH on kernel 4.19? Kernel 4.19 is a long term release so it would be very nice to have fix for this version upstream.

Regards,

Mogens

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

[Dean Wallace]

Hi Hans, Mogens,

On 17-01-19, Mogens Jensen wrote:
 
> Kernel is compiled with SND_SOC_INTEL_CHT_BSW_MAX98090_TI_MACH and the quirk seems to have fixed the problem caused by commit 648e921888ad ("clk: x86: Stop marking clocks as CLK_IS_CRITICAL"), as sound is now working if running "speaker-test" on my system which is clean ALSA.
> 
> Unfortunately, SND_SOC_INTEL_CHT_BSW_MAX98090_TI_MACH driver is unusable on Clapper Chromebooks as audio played from everything but "speaker-test" as video players or web browsers is extremly low and sounds like played at 10x speed. At the same time kernel log is spammed with messages like this:
> 
> max98090 i2c-193C9890:00: PLL unlocked
> intel_sst_acpi 80860F28:00: FW Version 01.0c.00.01
> writing to lpe: 00000000: 01 01 01 01 00 00 08 00 ff ff ff ff 55 00 00 00  ............U...
> writing to lpe: 00000000: 01 01 01 01 00 00 1a 00 ff ff ff ff 75 00 12 00  ............u...
> 
> This is probably not related to the problem discussed in this thread, but the result is that I have to use the legacy driver SND_SOC_INTEL_BYT_MAX98090_MACH and therefore still has to revert commit 648e921888ad for sound to work.
> 
> Is it possible to create a fix for SND_SOC_INTEL_BYT_MAX98090_MACH on kernel 4.19? Kernel 4.19 is a long term release so it would be very nice to have fix for this version upstream.

I have been reverting "clk: x86: Stop marking clocks as CLK_IS_CRITICAL"
and the patch that initially added the quirk for swanky because of sound
instability issues as you described.  I'm compiling vanilla Archlinux
kernel with SND_SOC_INTEL_CHT_BSW_MAX98090_TI_MACH, using pulseaudio,
and have sound in all my apps.

Baytrail sound has always been a little touchy, especially using headset
with mic, but since the clk patch breaking sound and the quirk patch to
fix it, there is a lot more instability.  Just running pavucontrol, or
plugging in headset sets it off.  It's a head scratcher.

-Dean

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

[Hans de Goede]

[-- Attachment #1: Type: text/plain, Size: 2358 bytes --]

Hi,

On 17-01-19 10:12, Dean Wallace wrote:
> Hi Hans, Mogens,
> 
> On 17-01-19, Mogens Jensen wrote:
>   
>> Kernel is compiled with SND_SOC_INTEL_CHT_BSW_MAX98090_TI_MACH and the quirk seems to have fixed the problem caused by commit 648e921888ad ("clk: x86: Stop marking clocks as CLK_IS_CRITICAL"), as sound is now working if running "speaker-test" on my system which is clean ALSA.

Note being "clean ALSA" is really not a good thing now a days,
for lots of things we depend on pulseaudio (like setting
up UCM mixer profiles).

>>
>> Unfortunately, SND_SOC_INTEL_CHT_BSW_MAX98090_TI_MACH driver is unusable on Clapper Chromebooks as audio played from everything but "speaker-test" as video players or web browsers is extremly low and sounds like played at 10x speed. At the same time kernel log is spammed with messages like this:
>>
>> max98090 i2c-193C9890:00: PLL unlocked
>> intel_sst_acpi 80860F28:00: FW Version 01.0c.00.01
>> writing to lpe: 00000000: 01 01 01 01 00 00 08 00 ff ff ff ff 55 00 00 00  ............U...
>> writing to lpe: 00000000: 01 01 01 01 00 00 1a 00 ff ff ff ff 75 00 12 00  ............u...
>>
>> This is probably not related to the problem discussed in this thread, but the result is that I have to use the legacy driver SND_SOC_INTEL_BYT_MAX98090_MACH and therefore still has to revert commit 648e921888ad for sound to work.
>>
>> Is it possible to create a fix for SND_SOC_INTEL_BYT_MAX98090_MACH on kernel 4.19? Kernel 4.19 is a long term release so it would be very nice to have fix for this version upstream.
> 
> I have been reverting "clk: x86: Stop marking clocks as CLK_IS_CRITICAL"
> and the patch that initially added the quirk for swanky because of sound
> instability issues as you described.  I'm compiling vanilla Archlinux
> kernel with SND_SOC_INTEL_CHT_BSW_MAX98090_TI_MACH, using pulseaudio,
> and have sound in all my apps.
> 
> Baytrail sound has always been a little touchy, especially using headset
> with mic, but since the clk patch breaking sound and the quirk patch to
> fix it, there is a lot more instability.  Just running pavucontrol, or
> plugging in headset sets it off.  It's a head scratcher.

Mogens, Dean, can you please try the SND_SOC_INTEL_CHT_BSW_MAX98090_TI_MACH
driver, without reverting any patches, with the attached patch on top and
see if that helps?

Thanks & Regards,

Hans

[-- Attachment #2: 0001-ASoC-intel-cht_bsw_max98090_ti-Enable-codec-clock-on.patch --]
[-- Type: text/x-patch, Size: 5411 bytes --]

From b429076ee98094fe0ab5584980b178a5df1b3eb4 Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@redhat.com>
Date: Thu, 17 Jan 2019 12:47:07 +0100
Subject: [PATCH] ASoC: intel: cht_bsw_max98090_ti: Enable codec clock once and
 keep it enabled

Users have been seeing sound stability issues with max98090 codecs since:
commit 648e921888ad ("clk: x86: Stop marking clocks as CLK_IS_CRITICAL")

At first that commit broke sound for Chromebook Swanky and Clapper models,
the problem was that the machine-driver has been controlling the wrong
clock on those models since support for them was added. This was hidden by
clk-pmc-atom.c keeping the actual clk on unconditionally.

With the machine-driver controlling the proper clock, sound works again
but we are seeing bug reports describing it as: low volume,
"sounds like played at 10x speed" and instable.

When these issues are hit the following message is seen in dmesg:
"max98090 i2c-193C9890:00: PLL unlocked".

Attempts have been made to fix this by inserting a delay between enabling
the clk and enabling and checking the pll, but this has not helped.

It seems that if we ever disable the clk, the pll looses its lock and
then we get various issues.

This commit fixes this by enabling the clock once at probe time,
in essence restoring the old behavior of clk-pmc-atom.c always keeping
the clk on, but then only on machines with a max98090 codec.

Fixes: 648e921888ad ("clk: x86: Stop marking clocks as CLK_IS_CRITICAL")
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 sound/soc/intel/boards/cht_bsw_max98090_ti.c | 75 ++++++--------------
 1 file changed, 20 insertions(+), 55 deletions(-)

diff --git a/sound/soc/intel/boards/cht_bsw_max98090_ti.c b/sound/soc/intel/boards/cht_bsw_max98090_ti.c
index 08a5152e635a..549d6ce12ec4 100644
--- a/sound/soc/intel/boards/cht_bsw_max98090_ti.c
+++ b/sound/soc/intel/boards/cht_bsw_max98090_ti.c
@@ -44,43 +44,11 @@ struct cht_mc_private {
 	bool ts3a227e_present;
 };
 
-static int platform_clock_control(struct snd_soc_dapm_widget *w,
-					  struct snd_kcontrol *k, int  event)
-{
-	struct snd_soc_dapm_context *dapm = w->dapm;
-	struct snd_soc_card *card = dapm->card;
-	struct snd_soc_dai *codec_dai;
-	struct cht_mc_private *ctx = snd_soc_card_get_drvdata(card);
-	int ret;
-
-	codec_dai = snd_soc_card_get_codec_dai(card, CHT_CODEC_DAI);
-	if (!codec_dai) {
-		dev_err(card->dev, "Codec dai not found; Unable to set platform clock\n");
-		return -EIO;
-	}
-
-	if (SND_SOC_DAPM_EVENT_ON(event)) {
-		ret = clk_prepare_enable(ctx->mclk);
-		if (ret < 0) {
-			dev_err(card->dev,
-				"could not configure MCLK state");
-			return ret;
-		}
-	} else {
-		clk_disable_unprepare(ctx->mclk);
-	}
-
-	return 0;
-}
-
 static const struct snd_soc_dapm_widget cht_dapm_widgets[] = {
 	SND_SOC_DAPM_HP("Headphone", NULL),
 	SND_SOC_DAPM_MIC("Headset Mic", NULL),
 	SND_SOC_DAPM_MIC("Int Mic", NULL),
 	SND_SOC_DAPM_SPK("Ext Spk", NULL),
-	SND_SOC_DAPM_SUPPLY("Platform Clock", SND_SOC_NOPM, 0, 0,
-			    platform_clock_control, SND_SOC_DAPM_PRE_PMU |
-			    SND_SOC_DAPM_POST_PMD),
 };
 
 static const struct snd_soc_dapm_route cht_audio_map[] = {
@@ -97,10 +65,6 @@ static const struct snd_soc_dapm_route cht_audio_map[] = {
 	{"codec_in0", NULL, "ssp2 Rx" },
 	{"codec_in1", NULL, "ssp2 Rx" },
 	{"ssp2 Rx", NULL, "HiFi Capture"},
-	{"Headphone", NULL, "Platform Clock"},
-	{"Headset Mic", NULL, "Platform Clock"},
-	{"Int Mic", NULL, "Platform Clock"},
-	{"Ext Spk", NULL, "Platform Clock"},
 };
 
 static const struct snd_kcontrol_new cht_mc_controls[] = {
@@ -222,25 +186,6 @@ static int cht_codec_init(struct snd_soc_pcm_runtime *runtime)
 			"jack detection gpios not added, error %d\n", ret);
 	}
 
-	/*
-	 * The firmware might enable the clock at
-	 * boot (this information may or may not
-	 * be reflected in the enable clock register).
-	 * To change the rate we must disable the clock
-	 * first to cover these cases. Due to common
-	 * clock framework restrictions that do not allow
-	 * to disable a clock that has not been enabled,
-	 * we need to enable the clock first.
-	 */
-	ret = clk_prepare_enable(ctx->mclk);
-	if (!ret)
-		clk_disable_unprepare(ctx->mclk);
-
-	ret = clk_set_rate(ctx->mclk, CHT_PLAT_CLK_3_HZ);
-
-	if (ret)
-		dev_err(runtime->dev, "unable to set MCLK rate\n");
-
 	return ret;
 }
 
@@ -459,6 +404,16 @@ static int snd_cht_mc_probe(struct platform_device *pdev)
 		return PTR_ERR(drv->mclk);
 	}
 
+	/*
+	 * The MAX98090 does not seem to like it if we muck with its clock,
+	 * so we enable it here once and leave it at that.
+	 */
+	ret_val = clk_prepare_enable(drv->mclk);
+	if (ret_val < 0) {
+		dev_err(&pdev->dev, "Failed to enable MCLK: %d\n", ret_val);
+		return ret_val;
+	}
+
 	ret_val = devm_snd_soc_register_card(&pdev->dev, &snd_soc_card_cht);
 	if (ret_val) {
 		dev_err(&pdev->dev,
@@ -469,11 +424,21 @@ static int snd_cht_mc_probe(struct platform_device *pdev)
 	return ret_val;
 }
 
+static int snd_cht_mc_remove(struct platform_device *pdev)
+{
+	struct snd_soc_card *card = platform_get_drvdata(pdev);
+	struct cht_mc_private *ctx = snd_soc_card_get_drvdata(card);
+
+	clk_disable_unprepare(ctx->mclk);
+	return 0;
+}
+
 static struct platform_driver snd_cht_mc_driver = {
 	.driver = {
 		.name = "cht-bsw-max98090",
 	},
 	.probe = snd_cht_mc_probe,
+	.remove = snd_cht_mc_remove,
 };
 
 module_platform_driver(snd_cht_mc_driver)
-- 
2.20.1

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

[Johannes Stezenbach]

On Thu, Jan 17, 2019 at 01:05:35PM +0100, Hans de Goede wrote:
> On 17-01-19 10:12, Dean Wallace wrote:
> > On 17-01-19, Mogens Jensen wrote:
> > > Kernel is compiled with SND_SOC_INTEL_CHT_BSW_MAX98090_TI_MACH and the quirk seems to have fixed the problem caused by commit 648e921888ad ("clk: x86: Stop marking clocks as CLK_IS_CRITICAL"), as sound is now working if running "speaker-test" on my system which is clean ALSA.
> 
> Note being "clean ALSA" is really not a good thing now a days,
> for lots of things we depend on pulseaudio (like setting
> up UCM mixer profiles).

FWIW I disagree because PA never worked for me.  I simply used
"alsaucm -c chtcx2072x set _verb HiFi".  But I was surprised
that PA does the ALSA UCM setup but it's not documented well that
you need to do it by other means if you don't use PA.
https://bugzilla.kernel.org/show_bug.cgi?id=115531#c72


Regards,
Johannes

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

[Dean Wallace]

On 17-01-19, Hans de Goede wrote:
> Mogens, Dean, can you please try the SND_SOC_INTEL_CHT_BSW_MAX98090_TI_MACH
> driver, without reverting any patches, with the attached patch on top and
> see if that helps?

Hi Hans.   Just compiled 4.20.3 with your new patch and no other
patches.  First impressions are, this is now as stable as it has ever
been in my experiences with this baytrail drama.  A quick run down on my
tests/findings:-

No PLL messages on boot, good.
Played audio using mpd, all fine.  Plugged in headphones, switches,
stays stable, no messages.
Unplugged/plugged again few times, no issues.
Ran pavucontrol, stayed stable, no messages.
I can still make it lose the lock and turn on the dalek distortion by
switching profiles in pavucontrol (from stereo output to any other, like
putput + input) but that has always been the case afaik for this
machine.  So basically this is now back to working as it was before the
4.18.5 'clk' breakage.

One thing to mention, I don;t normally use UCM files, tho I have in the
past, which is where I assume my asound.state file has come from, which
makes my sound work /shrug.  Well I've just tested using the UCM, and
things are a little less stable regarding plugging/unplugging headset a
few times.  It loses lock, but, what's different now is after maybe 3-4
seconds it corrects itself.  Not sure if it matters, but shows things
are a little more stable with this patch than previously known.

I'll test a little longer and report (if) any problems.  It will be
interesting to see how my buddy who uses headset/mic a lot gets on, as
he needs to use usb audio device because internal sound and mic don't
work properly.

-Dean

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

[Mogens Jensen]

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Thursday, January 17, 2019 12:05 PM, Hans de Goede <hdegoede@redhat.com> wrote:

> Hi,
>
> On 17-01-19 10:12, Dean Wallace wrote:
>
> > Hi Hans, Mogens,
> > On 17-01-19, Mogens Jensen wrote:
> >
> > > Kernel is compiled with SND_SOC_INTEL_CHT_BSW_MAX98090_TI_MACH and the quirk seems to have fixed the problem caused by commit 648e921888ad ("clk: x86: Stop marking clocks as CLK_IS_CRITICAL"), as sound is now working if running "speaker-test" on my system which is clean ALSA.
>
> Note being "clean ALSA" is really not a good thing now a days,
> for lots of things we depend on pulseaudio (like setting
> up UCM mixer profiles).
>
I'm using UCM mixer profile from:

https://github.com/plbossart/UCM/tree/master/chtmax98090

This is enabled with:

alsaucm -c chtmax98090 set _verb HiFi set _enadev Speakers

> > > Unfortunately, SND_SOC_INTEL_CHT_BSW_MAX98090_TI_MACH driver is unusable on Clapper Chromebooks as audio played from everything but "speaker-test" as video players or web browsers is extremly low and sounds like played at 10x speed. At the same time kernel log is spammed with messages like this:
> > > max98090 i2c-193C9890:00: PLL unlocked
> > > intel_sst_acpi 80860F28:00: FW Version 01.0c.00.01
> > > writing to lpe: 00000000: 01 01 01 01 00 00 08 00 ff ff ff ff 55 00 00 00 ............U...
> > > writing to lpe: 00000000: 01 01 01 01 00 00 1a 00 ff ff ff ff 75 00 12 00 ............u...
> > > This is probably not related to the problem discussed in this thread, but the result is that I have to use the legacy driver SND_SOC_INTEL_BYT_MAX98090_MACH and therefore still has to revert commit 648e921888ad for sound to work.
> > > Is it possible to create a fix for SND_SOC_INTEL_BYT_MAX98090_MACH on kernel 4.19? Kernel 4.19 is a long term release so it would be very nice to have fix for this version upstream.
> >
> > I have been reverting "clk: x86: Stop marking clocks as CLK_IS_CRITICAL"
> > and the patch that initially added the quirk for swanky because of sound
> > instability issues as you described. I'm compiling vanilla Archlinux
> > kernel with SND_SOC_INTEL_CHT_BSW_MAX98090_TI_MACH, using pulseaudio,
> > and have sound in all my apps.
> > Baytrail sound has always been a little touchy, especially using headset
> > with mic, but since the clk patch breaking sound and the quirk patch to
> > fix it, there is a lot more instability. Just running pavucontrol, or
> > plugging in headset sets it off. It's a head scratcher.
>
> Mogens, Dean, can you please try the SND_SOC_INTEL_CHT_BSW_MAX98090_TI_MACH
> driver, without reverting any patches, with the attached patch on top and
> see if that helps?
>
> Thanks & Regards,
>
> Hans

I have applied the patch to kernel 4.19.15 and unfortunately this has not solved the problems.

Audio generated from "speaker-test" is normal, but from everything else is very low and played at 10x speed. However, I'm not seeing the "max98090 i2c-193C9890:00: PLL unlocked" message in kernel log anymore, but it's still spammed with "writing to lpe: ...".

Regards,

Mogens

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

[Hans de Goede]

Hi,

On 1/17/19 2:16 PM, Dean Wallace wrote:
> On 17-01-19, Hans de Goede wrote:
>> Mogens, Dean, can you please try the SND_SOC_INTEL_CHT_BSW_MAX98090_TI_MACH
>> driver, without reverting any patches, with the attached patch on top and
>> see if that helps?
> 
> Hi Hans.   Just compiled 4.20.3 with your new patch and no other
> patches.  First impressions are, this is now as stable as it has ever
> been in my experiences with this baytrail drama.  A quick run down on my
> tests/findings:-
> 
> No PLL messages on boot, good.
> Played audio using mpd, all fine.  Plugged in headphones, switches,
> stays stable, no messages.
> Unplugged/plugged again few times, no issues.
> Ran pavucontrol, stayed stable, no messages.
> I can still make it lose the lock and turn on the dalek distortion by
> switching profiles in pavucontrol (from stereo output to any other, like
> putput + input) but that has always been the case afaik for this
> machine.  So basically this is now back to working as it was before the
> 4.18.5 'clk' breakage.
> 
> One thing to mention, I don;t normally use UCM files, tho I have in the
> past, which is where I assume my asound.state file has come from, which
> makes my sound work /shrug.  Well I've just tested using the UCM, and
> things are a little less stable regarding plugging/unplugging headset a
> few times.  It loses lock, but, what's different now is after maybe 3-4
> seconds it corrects itself.  Not sure if it matters, but shows things
> are a little more stable with this patch than previously known.

Ok, thank you for testing. Lets wait a bit and then if no problems
show up I will submit the patch upstream.

Regards,

Hans

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

[Hans de Goede]

Hi,

On 1/17/19 8:30 PM, Mogens Jensen wrote:
> ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
> On Thursday, January 17, 2019 12:05 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> 
>> Hi,
>>
>> On 17-01-19 10:12, Dean Wallace wrote:
>>
>>> Hi Hans, Mogens,
>>> On 17-01-19, Mogens Jensen wrote:
>>>
>>>> Kernel is compiled with SND_SOC_INTEL_CHT_BSW_MAX98090_TI_MACH and the quirk seems to have fixed the problem caused by commit 648e921888ad ("clk: x86: Stop marking clocks as CLK_IS_CRITICAL"), as sound is now working if running "speaker-test" on my system which is clean ALSA.
>>
>> Note being "clean ALSA" is really not a good thing now a days,
>> for lots of things we depend on pulseaudio (like setting
>> up UCM mixer profiles).
>>
> I'm using UCM mixer profile from:
> 
> https://github.com/plbossart/UCM/tree/master/chtmax98090
> 
> This is enabled with:
> 
> alsaucm -c chtmax98090 set _verb HiFi set _enadev Speakers
> 
>>>> Unfortunately, SND_SOC_INTEL_CHT_BSW_MAX98090_TI_MACH driver is unusable on Clapper Chromebooks as audio played from everything but "speaker-test" as video players or web browsers is extremly low and sounds like played at 10x speed. At the same time kernel log is spammed with messages like this:
>>>> max98090 i2c-193C9890:00: PLL unlocked
>>>> intel_sst_acpi 80860F28:00: FW Version 01.0c.00.01
>>>> writing to lpe: 00000000: 01 01 01 01 00 00 08 00 ff ff ff ff 55 00 00 00 ............U...
>>>> writing to lpe: 00000000: 01 01 01 01 00 00 1a 00 ff ff ff ff 75 00 12 00 ............u...
>>>> This is probably not related to the problem discussed in this thread, but the result is that I have to use the legacy driver SND_SOC_INTEL_BYT_MAX98090_MACH and therefore still has to revert commit 648e921888ad for sound to work.
>>>> Is it possible to create a fix for SND_SOC_INTEL_BYT_MAX98090_MACH on kernel 4.19? Kernel 4.19 is a long term release so it would be very nice to have fix for this version upstream.
>>>
>>> I have been reverting "clk: x86: Stop marking clocks as CLK_IS_CRITICAL"
>>> and the patch that initially added the quirk for swanky because of sound
>>> instability issues as you described. I'm compiling vanilla Archlinux
>>> kernel with SND_SOC_INTEL_CHT_BSW_MAX98090_TI_MACH, using pulseaudio,
>>> and have sound in all my apps.
>>> Baytrail sound has always been a little touchy, especially using headset
>>> with mic, but since the clk patch breaking sound and the quirk patch to
>>> fix it, there is a lot more instability. Just running pavucontrol, or
>>> plugging in headset sets it off. It's a head scratcher.
>>
>> Mogens, Dean, can you please try the SND_SOC_INTEL_CHT_BSW_MAX98090_TI_MACH
>> driver, without reverting any patches, with the attached patch on top and
>> see if that helps?
>>
>> Thanks & Regards,
>>
>> Hans
> 
> I have applied the patch to kernel 4.19.15 and unfortunately this has not solved the problems.
> 
> Audio generated from "speaker-test" is normal, but from everything else is very low and played at 10x speed. However, I'm not seeing the "max98090 i2c-193C9890:00: PLL unlocked" message in kernel log anymore, but it's still spammed with "writing to lpe: ...".

Hmm, I've a feeling the problem is your using alsa directly, do you have
dmix enabled ? You probably need dmix since the SST sound support
only supports 48KHz AFAIK.

Can you perhaps give things a try with pulseaudio ?

Regards,

Hans

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

[Mogens Jensen]

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Friday, January 18, 2019 3:35 PM, Hans de Goede <hdegoede@redhat.com> wrote:

> Hi,
>
> On 1/17/19 8:30 PM, Mogens Jensen wrote:
>
> > ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
> > On Thursday, January 17, 2019 12:05 PM, Hans de Goede hdegoede@redhat.com wrote:
> >
> > > Hi,
> > > On 17-01-19 10:12, Dean Wallace wrote:
> > >
> > > > Hi Hans, Mogens,
> > > > On 17-01-19, Mogens Jensen wrote:
> > > >
> > > > > Kernel is compiled with SND_SOC_INTEL_CHT_BSW_MAX98090_TI_MACH and the quirk seems to have fixed the problem caused by commit 648e921888ad ("clk: x86: Stop marking clocks as CLK_IS_CRITICAL"), as sound is now working if running "speaker-test" on my system which is clean ALSA.
> > >
> > > Note being "clean ALSA" is really not a good thing now a days,
> > > for lots of things we depend on pulseaudio (like setting
> > > up UCM mixer profiles).
> >
> > I'm using UCM mixer profile from:
> > https://github.com/plbossart/UCM/tree/master/chtmax98090
> > This is enabled with:
> > alsaucm -c chtmax98090 set _verb HiFi set _enadev Speakers
> >
> > > > > Unfortunately, SND_SOC_INTEL_CHT_BSW_MAX98090_TI_MACH driver is unusable on Clapper Chromebooks as audio played from everything but "speaker-test" as video players or web browsers is extremly low and sounds like played at 10x speed. At the same time kernel log is spammed with messages like this:
> > > > > max98090 i2c-193C9890:00: PLL unlocked
> > > > > intel_sst_acpi 80860F28:00: FW Version 01.0c.00.01
> > > > > writing to lpe: 00000000: 01 01 01 01 00 00 08 00 ff ff ff ff 55 00 00 00 ............U...
> > > > > writing to lpe: 00000000: 01 01 01 01 00 00 1a 00 ff ff ff ff 75 00 12 00 ............u...
> > > > > This is probably not related to the problem discussed in this thread, but the result is that I have to use the legacy driver SND_SOC_INTEL_BYT_MAX98090_MACH and therefore still has to revert commit 648e921888ad for sound to work.
> > > > > Is it possible to create a fix for SND_SOC_INTEL_BYT_MAX98090_MACH on kernel 4.19? Kernel 4.19 is a long term release so it would be very nice to have fix for this version upstream.
> > > >
> > > > I have been reverting "clk: x86: Stop marking clocks as CLK_IS_CRITICAL"
> > > > and the patch that initially added the quirk for swanky because of sound
> > > > instability issues as you described. I'm compiling vanilla Archlinux
> > > > kernel with SND_SOC_INTEL_CHT_BSW_MAX98090_TI_MACH, using pulseaudio,
> > > > and have sound in all my apps.
> > > > Baytrail sound has always been a little touchy, especially using headset
> > > > with mic, but since the clk patch breaking sound and the quirk patch to
> > > > fix it, there is a lot more instability. Just running pavucontrol, or
> > > > plugging in headset sets it off. It's a head scratcher.
> > >
> > > Mogens, Dean, can you please try the SND_SOC_INTEL_CHT_BSW_MAX98090_TI_MACH
> > > driver, without reverting any patches, with the attached patch on top and
> > > see if that helps?
> > > Thanks & Regards,
> > > Hans
> >
> > I have applied the patch to kernel 4.19.15 and unfortunately this has not solved the problems.
> > Audio generated from "speaker-test" is normal, but from everything else is very low and played at 10x speed. However, I'm not seeing the "max98090 i2c-193C9890:00: PLL unlocked" message in kernel log anymore, but it's still spammed with "writing to lpe: ...".
>
> Hmm, I've a feeling the problem is your using alsa directly, do you have
> dmix enabled ? You probably need dmix since the SST sound support
> only supports 48KHz AFAIK.
>
> Can you perhaps give things a try with pulseaudio ?
>
> Regards,
>
> Hans

You are absolutely correct, software mixing was apparently not enabled on my system and this caused the audio problems. I thought that dmix was enabled by default if hardware mixing was not supported. Thank you very much.

I was completely wrong about "SND_SOC_INTEL_CHT_BSW_MAX98090_TI_MACH driver seems to be unusable on Clapper Chromebooks". Sorry about that.

To sum up, audio is working perfectly on my Clapper Chromebook running kernel 4.19.15 with SND_SOC_INTEL_CHT_BSW_MAX98090_TI_MACH + "0001-ASoC-intel-cht_bsw_max98090_ti-Enable-codec-clock-on.patch", even better than before with the legacy driver.

The only minor annoyance I'm experiencing now, is a large amount of debug output from something in kernel log when audio is played on the system:

writing to lpe: 00000000: 01 01 01 01 00 00 08 00 ff ff ff ff 55 00 00 00  ............U...
writing to lpe: 00000000: 01 01 01 01 00 00 1a 00 ff ff ff ff 75 00 12 00  ............u...
...

Regards,

Mogens

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

[Pierre-Louis Bossart]

On 1/20/19 11:55 PM, Mogens Jensen wrote:
> The only minor annoyance I'm experiencing now, is a large amount of debug output from something in kernel log when audio is played on the system:
>
> writing to lpe: 00000000: 01 01 01 01 00 00 08 00 ff ff ff ff 55 00 00 00  ............U...
> writing to lpe: 00000000: 01 01 01 01 00 00 1a 00 ff ff ff ff 75 00 12 00  ............u...
> ...
That's enabled via dynamic debug so that's rather a configuration issue 
than a kernel problem?

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

[Hans de Goede]

Hi,

On 21-01-19 06:55, Mogens Jensen wrote:
> ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
> On Friday, January 18, 2019 3:35 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> 
>> Hi,
>>
>> On 1/17/19 8:30 PM, Mogens Jensen wrote:
>>
>>> ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
>>> On Thursday, January 17, 2019 12:05 PM, Hans de Goede hdegoede@redhat.com wrote:
>>>
>>>> Hi,
>>>> On 17-01-19 10:12, Dean Wallace wrote:
>>>>
>>>>> Hi Hans, Mogens,
>>>>> On 17-01-19, Mogens Jensen wrote:
>>>>>
>>>>>> Kernel is compiled with SND_SOC_INTEL_CHT_BSW_MAX98090_TI_MACH and the quirk seems to have fixed the problem caused by commit 648e921888ad ("clk: x86: Stop marking clocks as CLK_IS_CRITICAL"), as sound is now working if running "speaker-test" on my system which is clean ALSA.
>>>>
>>>> Note being "clean ALSA" is really not a good thing now a days,
>>>> for lots of things we depend on pulseaudio (like setting
>>>> up UCM mixer profiles).
>>>
>>> I'm using UCM mixer profile from:
>>> https://github.com/plbossart/UCM/tree/master/chtmax98090
>>> This is enabled with:
>>> alsaucm -c chtmax98090 set _verb HiFi set _enadev Speakers
>>>
>>>>>> Unfortunately, SND_SOC_INTEL_CHT_BSW_MAX98090_TI_MACH driver is unusable on Clapper Chromebooks as audio played from everything but "speaker-test" as video players or web browsers is extremly low and sounds like played at 10x speed. At the same time kernel log is spammed with messages like this:
>>>>>> max98090 i2c-193C9890:00: PLL unlocked
>>>>>> intel_sst_acpi 80860F28:00: FW Version 01.0c.00.01
>>>>>> writing to lpe: 00000000: 01 01 01 01 00 00 08 00 ff ff ff ff 55 00 00 00 ............U...
>>>>>> writing to lpe: 00000000: 01 01 01 01 00 00 1a 00 ff ff ff ff 75 00 12 00 ............u...
>>>>>> This is probably not related to the problem discussed in this thread, but the result is that I have to use the legacy driver SND_SOC_INTEL_BYT_MAX98090_MACH and therefore still has to revert commit 648e921888ad for sound to work.
>>>>>> Is it possible to create a fix for SND_SOC_INTEL_BYT_MAX98090_MACH on kernel 4.19? Kernel 4.19 is a long term release so it would be very nice to have fix for this version upstream.
>>>>>
>>>>> I have been reverting "clk: x86: Stop marking clocks as CLK_IS_CRITICAL"
>>>>> and the patch that initially added the quirk for swanky because of sound
>>>>> instability issues as you described. I'm compiling vanilla Archlinux
>>>>> kernel with SND_SOC_INTEL_CHT_BSW_MAX98090_TI_MACH, using pulseaudio,
>>>>> and have sound in all my apps.
>>>>> Baytrail sound has always been a little touchy, especially using headset
>>>>> with mic, but since the clk patch breaking sound and the quirk patch to
>>>>> fix it, there is a lot more instability. Just running pavucontrol, or
>>>>> plugging in headset sets it off. It's a head scratcher.
>>>>
>>>> Mogens, Dean, can you please try the SND_SOC_INTEL_CHT_BSW_MAX98090_TI_MACH
>>>> driver, without reverting any patches, with the attached patch on top and
>>>> see if that helps?
>>>> Thanks & Regards,
>>>> Hans
>>>
>>> I have applied the patch to kernel 4.19.15 and unfortunately this has not solved the problems.
>>> Audio generated from "speaker-test" is normal, but from everything else is very low and played at 10x speed. However, I'm not seeing the "max98090 i2c-193C9890:00: PLL unlocked" message in kernel log anymore, but it's still spammed with "writing to lpe: ...".
>>
>> Hmm, I've a feeling the problem is your using alsa directly, do you have
>> dmix enabled ? You probably need dmix since the SST sound support
>> only supports 48KHz AFAIK.
>>
>> Can you perhaps give things a try with pulseaudio ?
>>
>> Regards,
>>
>> Hans
> 
> You are absolutely correct, software mixing was apparently not enabled on my system and this caused the audio problems. I thought that dmix was enabled by default if hardware mixing was not supported. Thank you very much.
> 
> I was completely wrong about "SND_SOC_INTEL_CHT_BSW_MAX98090_TI_MACH driver seems to be unusable on Clapper Chromebooks". Sorry about that.
> 
> To sum up, audio is working perfectly on my Clapper Chromebook running kernel 4.19.15 with SND_SOC_INTEL_CHT_BSW_MAX98090_TI_MACH + "0001-ASoC-intel-cht_bsw_max98090_ti-Enable-codec-clock-on.patch", even better than before with the legacy driver.

Thank you for testing this. Given that things now work for both you and Dean I've
submitted the patch you both have tested upstream.

Regards,

Hans

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

[Mogens Jensen]

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Tuesday, January 22, 2019 7:27 PM, Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> wrote:

> On 1/20/19 11:55 PM, Mogens Jensen wrote:
>
> > The only minor annoyance I'm experiencing now, is a large amount of debug output from something in kernel log when audio is played on the system:
> > writing to lpe: 00000000: 01 01 01 01 00 00 08 00 ff ff ff ff 55 00 00 00 ............U...
> > writing to lpe: 00000000: 01 01 01 01 00 00 1a 00 ff ff ff ff 75 00 12 00 ............u...
> > ...
>
> That's enabled via dynamic debug so that's rather a configuration issue
> than a kernel problem?

Do you have any suggestions on how to disable it?

My kernel is compiled without DYNAMIC_DEBUG, DEBUG_FS and other debug features, so I don't understand why all this debug output is flooding the kernel log.

It's a minor issue, but it would be nice to get rid of it.

Regards,

Mogens Jensen

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

[Pierre-Louis Bossart]

On 1/24/19 11:16 PM, Mogens Jensen wrote:
> ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
> On Tuesday, January 22, 2019 7:27 PM, Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> wrote:
>
>> On 1/20/19 11:55 PM, Mogens Jensen wrote:
>>
>>> The only minor annoyance I'm experiencing now, is a large amount of debug output from something in kernel log when audio is played on the system:
>>> writing to lpe: 00000000: 01 01 01 01 00 00 08 00 ff ff ff ff 55 00 00 00 ............U...
>>> writing to lpe: 00000000: 01 01 01 01 00 00 1a 00 ff ff ff ff 75 00 12 00 ............u...
>>> ...
>> That's enabled via dynamic debug so that's rather a configuration issue
>> than a kernel problem?
> Do you have any suggestions on how to disable it?
>
> My kernel is compiled without DYNAMIC_DEBUG, DEBUG_FS and other debug features, so I don't understand why all this debug output is flooding the kernel log.
>
> It's a minor issue, but it would be nice to get rid of it.

I don't see it so it's definitively a config issue. I use

make defconfig

scripts/kconfig/merge_config.sh .config base-defconfig sst-defconfig  
hdaudio-codecs-defconfig

with all 3 defconfigs fetched from https://github.com/thesofproject/kconfig

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

[Pierre-Louis Bossart]

>>> The only minor annoyance I'm experiencing now, is a large amount of debug output from something in kernel log when audio is played on the system:
>>> writing to lpe: 00000000: 01 01 01 01 00 00 08 00 ff ff ff ff 55 00 00 00 ............U...
>>> writing to lpe: 00000000: 01 01 01 01 00 00 1a 00 ff ff ff ff 75 00 12 00 ............u...
>>> ...
>> That's enabled via dynamic debug so that's rather a configuration issue
>> than a kernel problem?
> Do you have any suggestions on how to disable it?
>
> My kernel is compiled without DYNAMIC_DEBUG, DEBUG_FS and other debug features, so I don't understand why all this debug output is flooding the kernel log.
>
> It's a minor issue, but it would be nice to get rid of it.
I can confirm that this happens without DYNAMIC_DEBUG, and somehow 
changing the log level doesn't seem to matter. I tried changing the 
console log as a kernel parameter or with playing 
/proc/sys/kernel/printk, no luck. weird.

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

[Andy Shevchenko]

On Fri, Jan 25, 2019 at 11:57:57AM -0600, Pierre-Louis Bossart wrote:
> 
> > > > The only minor annoyance I'm experiencing now, is a large amount of debug output from something in kernel log when audio is played on the system:
> > > > writing to lpe: 00000000: 01 01 01 01 00 00 08 00 ff ff ff ff 55 00 00 00 ............U...
> > > > writing to lpe: 00000000: 01 01 01 01 00 00 1a 00 ff ff ff ff 75 00 12 00 ............u...
> > > > ...
> > > That's enabled via dynamic debug so that's rather a configuration issue
> > > than a kernel problem?
> > Do you have any suggestions on how to disable it?
> > 
> > My kernel is compiled without DYNAMIC_DEBUG, DEBUG_FS and other debug features, so I don't understand why all this debug output is flooding the kernel log.
> > 
> > It's a minor issue, but it would be nice to get rid of it.
> I can confirm that this happens without DYNAMIC_DEBUG, and somehow changing
> the log level doesn't seem to matter. I tried changing the console log as a
> kernel parameter or with playing /proc/sys/kernel/printk, no luck. weird.

Are you sure you did a clean build?

The logic behind print_hex_dump_bytes() is following:
- if !CONFIG_PRINTK — nothing should be printed at all
- otherwise if CONFIG_DYNAMIC_DEBUG — it goes thru its facilities
- else if goes to KERN_DEBUG level and thus loglevel should affect this either
  thru command line or via procfs

If none of the above works like it should, the couple of possibilities I can see:
- unclean build where previously it was compiled somehow with DEBUG
- ignore_loglevel is in the kernel command line

-- 
With Best Regards,
Andy Shevchenko