* Re: Regression found (Stop-marking-clocks-as-CLK_IS_CRITICAL) [not found] <20181025232517.ywnw54qibemosjws@picard> @ 2018-10-29 17:45 ` Stephen Boyd 2018-10-29 17:52 ` Andy Shevchenko 0 siblings, 1 reply; 58+ messages in thread From: Stephen Boyd @ 2018-10-29 17:45 UTC (permalink / raw) To: Dean Wallace, Hans de Goede Cc: mturquette, linux-clk, stable, Johannes Stezenbach, Carlo Caione, Andy Shevchenko, linux-kernel 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. ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: Regression found (Stop-marking-clocks-as-CLK_IS_CRITICAL) 2018-10-29 17:45 ` Regression found (Stop-marking-clocks-as-CLK_IS_CRITICAL) Stephen Boyd @ 2018-10-29 17:52 ` Andy Shevchenko 2018-10-29 18:04 ` Andy Shevchenko 0 siblings, 1 reply; 58+ messages in thread From: Andy Shevchenko @ 2018-10-29 17:52 UTC (permalink / raw) To: Stephen Boyd, Pierre-Louis Bossart Cc: duffydack73, Hans de Goede, Michael Turquette, linux-clk, Stable, Johannes Stezenbach, Carlo Caione, Andy Shevchenko, Linux Kernel Mailing List 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 ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: Regression found (Stop-marking-clocks-as-CLK_IS_CRITICAL) 2018-10-29 17:52 ` Andy Shevchenko @ 2018-10-29 18:04 ` Andy Shevchenko 2018-10-29 19:08 ` Dean Wallace 0 siblings, 1 reply; 58+ messages in thread From: Andy Shevchenko @ 2018-10-29 18:04 UTC (permalink / raw) To: Stephen Boyd, Pierre-Louis Bossart Cc: duffydack73, Hans de Goede, Michael Turquette, linux-clk, Stable, Johannes Stezenbach, Carlo Caione, Andy Shevchenko, Linux Kernel Mailing List 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 ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: Regression found (Stop-marking-clocks-as-CLK_IS_CRITICAL) 2018-10-29 18:04 ` Andy Shevchenko @ 2018-10-29 19:08 ` Dean Wallace 2018-10-29 22:03 ` Pierre-Louis Bossart 0 siblings, 1 reply; 58+ messages in thread From: Dean Wallace @ 2018-10-29 19:08 UTC (permalink / raw) To: Andy Shevchenko Cc: Stephen Boyd, Pierre-Louis Bossart, Hans de Goede, Michael Turquette, linux-clk, Stable, Johannes Stezenbach, Carlo Caione, Andy Shevchenko, Linux Kernel Mailing List 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 ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: Regression found (Stop-marking-clocks-as-CLK_IS_CRITICAL) 2018-10-29 19:08 ` Dean Wallace @ 2018-10-29 22:03 ` Pierre-Louis Bossart 2018-10-30 10:17 ` Hans de Goede 2018-10-30 11:19 ` Hans de Goede 0 siblings, 2 replies; 58+ messages in thread From: Pierre-Louis Bossart @ 2018-10-29 22:03 UTC (permalink / raw) To: Dean Wallace, Andy Shevchenko Cc: Stephen Boyd, Hans de Goede, Michael Turquette, linux-clk, Stable, Johannes Stezenbach, Carlo Caione, Andy Shevchenko, Linux Kernel Mailing List 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 ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: Regression found (Stop-marking-clocks-as-CLK_IS_CRITICAL) 2018-10-29 22:03 ` Pierre-Louis Bossart @ 2018-10-30 10:17 ` Hans de Goede 2018-10-30 11:05 ` Hans de Goede 2018-10-30 11:19 ` Hans de Goede 1 sibling, 1 reply; 58+ messages in thread From: Hans de Goede @ 2018-10-30 10:17 UTC (permalink / raw) To: Pierre-Louis Bossart, Dean Wallace, Andy Shevchenko Cc: Stephen Boyd, Michael Turquette, linux-clk, Stable, Johannes Stezenbach, Carlo Caione, Andy Shevchenko, Linux Kernel Mailing List 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 ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: Regression found (Stop-marking-clocks-as-CLK_IS_CRITICAL) 2018-10-30 10:17 ` Hans de Goede @ 2018-10-30 11:05 ` Hans de Goede 2018-10-30 16:24 ` Andy Shevchenko 0 siblings, 1 reply; 58+ messages in thread From: Hans de Goede @ 2018-10-30 11:05 UTC (permalink / raw) To: Pierre-Louis Bossart, Dean Wallace, Andy Shevchenko Cc: Stephen Boyd, Michael Turquette, linux-clk, Stable, Johannes Stezenbach, Carlo Caione, Andy Shevchenko, Linux Kernel Mailing List, Mogens Jensen 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 ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: Regression found (Stop-marking-clocks-as-CLK_IS_CRITICAL) 2018-10-30 11:05 ` Hans de Goede @ 2018-10-30 16:24 ` Andy Shevchenko 0 siblings, 0 replies; 58+ messages in thread From: Andy Shevchenko @ 2018-10-30 16:24 UTC (permalink / raw) To: Hans de Goede Cc: Pierre-Louis Bossart, Dean Wallace, Stephen Boyd, Michael Turquette, linux-clk, Stable, Johannes Stezenbach, Carlo Caione, Andy Shevchenko, Linux Kernel Mailing List, mogens-jensen 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 ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: Regression found (Stop-marking-clocks-as-CLK_IS_CRITICAL) 2018-10-29 22:03 ` Pierre-Louis Bossart 2018-10-30 10:17 ` Hans de Goede @ 2018-10-30 11:19 ` Hans de Goede 2018-10-30 14:38 ` Dean Wallace 1 sibling, 1 reply; 58+ messages in thread From: Hans de Goede @ 2018-10-30 11:19 UTC (permalink / raw) To: Pierre-Louis Bossart, Dean Wallace, Andy Shevchenko Cc: Stephen Boyd, Michael Turquette, linux-clk, Stable, Johannes Stezenbach, Carlo Caione, Andy Shevchenko, Linux Kernel Mailing List, Mogens Jensen [-- 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 ^ permalink raw reply related [flat|nested] 58+ messages in thread
* Re: Regression found (Stop-marking-clocks-as-CLK_IS_CRITICAL) 2018-10-30 11:19 ` Hans de Goede @ 2018-10-30 14:38 ` Dean Wallace 2018-10-30 14:48 ` Hans de Goede 2018-10-30 15:04 ` Pierre-Louis Bossart 0 siblings, 2 replies; 58+ messages in thread From: Dean Wallace @ 2018-10-30 14:38 UTC (permalink / raw) To: Hans de Goede Cc: Pierre-Louis Bossart, Andy Shevchenko, Stephen Boyd, Michael Turquette, linux-clk, Stable, Johannes Stezenbach, Carlo Caione, Andy Shevchenko, Linux Kernel Mailing List, Mogens Jensen 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 ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: Regression found (Stop-marking-clocks-as-CLK_IS_CRITICAL) 2018-10-30 14:38 ` Dean Wallace @ 2018-10-30 14:48 ` Hans de Goede 2018-10-30 15:03 ` Andy Shevchenko 2018-10-30 15:04 ` Pierre-Louis Bossart 1 sibling, 1 reply; 58+ messages in thread From: Hans de Goede @ 2018-10-30 14:48 UTC (permalink / raw) To: Dean Wallace Cc: Pierre-Louis Bossart, Andy Shevchenko, Stephen Boyd, Michael Turquette, linux-clk, Stable, Johannes Stezenbach, Carlo Caione, Andy Shevchenko, Linux Kernel Mailing List, Mogens Jensen 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 ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: Regression found (Stop-marking-clocks-as-CLK_IS_CRITICAL) 2018-10-30 14:48 ` Hans de Goede @ 2018-10-30 15:03 ` Andy Shevchenko 2018-10-30 15:25 ` Pierre-Louis Bossart 0 siblings, 1 reply; 58+ messages in thread From: Andy Shevchenko @ 2018-10-30 15:03 UTC (permalink / raw) To: Hans de Goede Cc: duffydack73, Pierre-Louis Bossart, Stephen Boyd, Michael Turquette, linux-clk, Stable, Johannes Stezenbach, Carlo Caione, Andy Shevchenko, Linux Kernel Mailing List, mogens-jensen 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 ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: Regression found (Stop-marking-clocks-as-CLK_IS_CRITICAL) 2018-10-30 15:03 ` Andy Shevchenko @ 2018-10-30 15:25 ` Pierre-Louis Bossart 0 siblings, 0 replies; 58+ messages in thread From: Pierre-Louis Bossart @ 2018-10-30 15:25 UTC (permalink / raw) To: Andy Shevchenko, Hans de Goede Cc: duffydack73, Stephen Boyd, Michael Turquette, linux-clk, Stable, Johannes Stezenbach, Carlo Caione, Andy Shevchenko, Linux Kernel Mailing List, mogens-jensen 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. > ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: Regression found (Stop-marking-clocks-as-CLK_IS_CRITICAL) 2018-10-30 14:38 ` Dean Wallace 2018-10-30 14:48 ` Hans de Goede @ 2018-10-30 15:04 ` Pierre-Louis Bossart 2018-10-30 15:46 ` Hans de Goede 1 sibling, 1 reply; 58+ messages in thread From: Pierre-Louis Bossart @ 2018-10-30 15:04 UTC (permalink / raw) To: Dean Wallace, Hans de Goede Cc: Andy Shevchenko, Stephen Boyd, Michael Turquette, linux-clk, Stable, Johannes Stezenbach, Carlo Caione, Andy Shevchenko, Linux Kernel Mailing List, Mogens Jensen 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 ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: Regression found (Stop-marking-clocks-as-CLK_IS_CRITICAL) 2018-10-30 15:04 ` Pierre-Louis Bossart @ 2018-10-30 15:46 ` Hans de Goede 2018-10-30 16:02 ` Hans de Goede ` (2 more replies) 0 siblings, 3 replies; 58+ messages in thread From: Hans de Goede @ 2018-10-30 15:46 UTC (permalink / raw) To: Pierre-Louis Bossart, Dean Wallace Cc: Andy Shevchenko, Stephen Boyd, Michael Turquette, linux-clk, Stable, Johannes Stezenbach, Carlo Caione, Andy Shevchenko, Linux Kernel Mailing List, Mogens Jensen 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 ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: Regression found (Stop-marking-clocks-as-CLK_IS_CRITICAL) 2018-10-30 15:46 ` Hans de Goede @ 2018-10-30 16:02 ` Hans de Goede 2018-10-30 16:27 ` Pierre-Louis Bossart 2018-10-30 16:03 ` Pierre-Louis Bossart 2018-10-30 16:04 ` Hans de Goede 2 siblings, 1 reply; 58+ messages in thread From: Hans de Goede @ 2018-10-30 16:02 UTC (permalink / raw) To: Pierre-Louis Bossart, Dean Wallace Cc: Andy Shevchenko, Stephen Boyd, Michael Turquette, linux-clk, Stable, Johannes Stezenbach, Carlo Caione, Andy Shevchenko, Linux Kernel Mailing List, Mogens Jensen 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 ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: Regression found (Stop-marking-clocks-as-CLK_IS_CRITICAL) 2018-10-30 16:02 ` Hans de Goede @ 2018-10-30 16:27 ` Pierre-Louis Bossart 2018-10-30 18:31 ` Hans de Goede 0 siblings, 1 reply; 58+ messages in thread From: Pierre-Louis Bossart @ 2018-10-30 16:27 UTC (permalink / raw) To: Hans de Goede, Dean Wallace Cc: Andy Shevchenko, Stephen Boyd, Michael Turquette, linux-clk, Stable, Johannes Stezenbach, Carlo Caione, Andy Shevchenko, Linux Kernel Mailing List, Mogens Jensen 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 ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: Regression found (Stop-marking-clocks-as-CLK_IS_CRITICAL) 2018-10-30 16:27 ` Pierre-Louis Bossart @ 2018-10-30 18:31 ` Hans de Goede 0 siblings, 0 replies; 58+ messages in thread From: Hans de Goede @ 2018-10-30 18:31 UTC (permalink / raw) To: Pierre-Louis Bossart, Dean Wallace Cc: Andy Shevchenko, Stephen Boyd, Michael Turquette, linux-clk, Stable, Johannes Stezenbach, Carlo Caione, Andy Shevchenko, Linux Kernel Mailing List, Mogens Jensen 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 ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: Regression found (Stop-marking-clocks-as-CLK_IS_CRITICAL) 2018-10-30 15:46 ` Hans de Goede 2018-10-30 16:02 ` Hans de Goede @ 2018-10-30 16:03 ` Pierre-Louis Bossart 2018-10-30 16:04 ` Hans de Goede 2 siblings, 0 replies; 58+ messages in thread From: Pierre-Louis Bossart @ 2018-10-30 16:03 UTC (permalink / raw) To: Hans de Goede, Dean Wallace Cc: Andy Shevchenko, Stephen Boyd, Michael Turquette, linux-clk, Stable, Johannes Stezenbach, Carlo Caione, Andy Shevchenko, Linux Kernel Mailing List, Mogens Jensen 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 ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: Regression found (Stop-marking-clocks-as-CLK_IS_CRITICAL) 2018-10-30 15:46 ` Hans de Goede 2018-10-30 16:02 ` Hans de Goede 2018-10-30 16:03 ` Pierre-Louis Bossart @ 2018-10-30 16:04 ` Hans de Goede 2018-10-30 16:15 ` Dean Wallace 2018-10-30 18:56 ` Mogens Jensen 2 siblings, 2 replies; 58+ messages in thread From: Hans de Goede @ 2018-10-30 16:04 UTC (permalink / raw) To: Pierre-Louis Bossart, Dean Wallace Cc: Andy Shevchenko, Stephen Boyd, Michael Turquette, linux-clk, Stable, Johannes Stezenbach, Carlo Caione, Andy Shevchenko, Linux Kernel Mailing List, Mogens Jensen 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 ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: Regression found (Stop-marking-clocks-as-CLK_IS_CRITICAL) 2018-10-30 16:04 ` Hans de Goede @ 2018-10-30 16:15 ` Dean Wallace 2018-10-31 11:04 ` Hans de Goede 2018-10-30 18:56 ` Mogens Jensen 1 sibling, 1 reply; 58+ messages in thread From: Dean Wallace @ 2018-10-30 16:15 UTC (permalink / raw) To: Hans de Goede Cc: Pierre-Louis Bossart, Andy Shevchenko, Stephen Boyd, Michael Turquette, linux-clk, Stable, Johannes Stezenbach, Carlo Caione, Andy Shevchenko, Linux Kernel Mailing List, Mogens Jensen 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 ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: Regression found (Stop-marking-clocks-as-CLK_IS_CRITICAL) 2018-10-30 16:15 ` Dean Wallace @ 2018-10-31 11:04 ` Hans de Goede 2018-10-31 12:45 ` Dean Wallace 2018-10-31 20:07 ` Dean Wallace 0 siblings, 2 replies; 58+ messages in thread From: Hans de Goede @ 2018-10-31 11:04 UTC (permalink / raw) To: Dean Wallace Cc: Pierre-Louis Bossart, Andy Shevchenko, Stephen Boyd, Michael Turquette, linux-clk, Stable, Johannes Stezenbach, Carlo Caione, Andy Shevchenko, Linux Kernel Mailing List, Mogens Jensen [-- 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 ^ permalink raw reply related [flat|nested] 58+ messages in thread
* Re: Regression found (Stop-marking-clocks-as-CLK_IS_CRITICAL) 2018-10-31 11:04 ` Hans de Goede @ 2018-10-31 12:45 ` Dean Wallace 2018-10-31 20:07 ` Dean Wallace 1 sibling, 0 replies; 58+ messages in thread From: Dean Wallace @ 2018-10-31 12:45 UTC (permalink / raw) To: Hans de Goede Cc: Pierre-Louis Bossart, Andy Shevchenko, Stephen Boyd, Michael Turquette, linux-clk, Stable, Johannes Stezenbach, Andy Shevchenko, Linux Kernel Mailing List, Mogens Jensen 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 ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: Regression found (Stop-marking-clocks-as-CLK_IS_CRITICAL) 2018-10-31 11:04 ` Hans de Goede 2018-10-31 12:45 ` Dean Wallace @ 2018-10-31 20:07 ` Dean Wallace 2018-10-31 22:27 ` Pierre-Louis Bossart 1 sibling, 1 reply; 58+ messages in thread From: Dean Wallace @ 2018-10-31 20:07 UTC (permalink / raw) To: Hans de Goede Cc: Pierre-Louis Bossart, Andy Shevchenko, Stephen Boyd, Michael Turquette, linux-clk, Stable, Johannes Stezenbach, Andy Shevchenko, Linux Kernel Mailing List, Mogens Jensen 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 ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: Regression found (Stop-marking-clocks-as-CLK_IS_CRITICAL) 2018-10-31 20:07 ` Dean Wallace @ 2018-10-31 22:27 ` Pierre-Louis Bossart 2018-10-31 23:57 ` Dean Wallace ` (2 more replies) 0 siblings, 3 replies; 58+ messages in thread From: Pierre-Louis Bossart @ 2018-10-31 22:27 UTC (permalink / raw) To: Dean Wallace, Hans de Goede Cc: Andy Shevchenko, Stephen Boyd, Michael Turquette, linux-clk, Stable, Johannes Stezenbach, Andy Shevchenko, Linux Kernel Mailing List, Mogens Jensen > 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 ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: Regression found (Stop-marking-clocks-as-CLK_IS_CRITICAL) 2018-10-31 22:27 ` Pierre-Louis Bossart @ 2018-10-31 23:57 ` Dean Wallace 2018-11-01 10:37 ` Dean Wallace 2018-11-01 13:56 ` Hans de Goede 2 siblings, 0 replies; 58+ messages in thread From: Dean Wallace @ 2018-10-31 23:57 UTC (permalink / raw) To: Pierre-Louis Bossart Cc: Hans de Goede, Andy Shevchenko, Stephen Boyd, Michael Turquette, linux-clk, Stable, Johannes Stezenbach, Andy Shevchenko, Linux Kernel Mailing List, Mogens Jensen 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 ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: Regression found (Stop-marking-clocks-as-CLK_IS_CRITICAL) 2018-10-31 22:27 ` Pierre-Louis Bossart 2018-10-31 23:57 ` Dean Wallace @ 2018-11-01 10:37 ` Dean Wallace 2018-11-01 13:57 ` Hans de Goede 2018-11-01 13:56 ` Hans de Goede 2 siblings, 1 reply; 58+ messages in thread From: Dean Wallace @ 2018-11-01 10:37 UTC (permalink / raw) To: Pierre-Louis Bossart Cc: Hans de Goede, Andy Shevchenko, Stephen Boyd, Michael Turquette, linux-clk, Stable, Johannes Stezenbach, Andy Shevchenko, Linux Kernel Mailing List, Mogens Jensen 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 ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: Regression found (Stop-marking-clocks-as-CLK_IS_CRITICAL) 2018-11-01 10:37 ` Dean Wallace @ 2018-11-01 13:57 ` Hans de Goede 2018-11-01 14:28 ` Dean Wallace 0 siblings, 1 reply; 58+ messages in thread From: Hans de Goede @ 2018-11-01 13:57 UTC (permalink / raw) To: Dean Wallace, Pierre-Louis Bossart Cc: Andy Shevchenko, Stephen Boyd, Michael Turquette, linux-clk, Stable, Johannes Stezenbach, Andy Shevchenko, Linux Kernel Mailing List, Mogens Jensen 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 ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: Regression found (Stop-marking-clocks-as-CLK_IS_CRITICAL) 2018-11-01 13:57 ` Hans de Goede @ 2018-11-01 14:28 ` Dean Wallace 2018-11-01 14:49 ` Hans de Goede 0 siblings, 1 reply; 58+ messages in thread From: Dean Wallace @ 2018-11-01 14:28 UTC (permalink / raw) To: Hans de Goede Cc: Pierre-Louis Bossart, Andy Shevchenko, Stephen Boyd, Michael Turquette, linux-clk, Stable, Johannes Stezenbach, Andy Shevchenko, Linux Kernel Mailing List, Mogens Jensen 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 ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: Regression found (Stop-marking-clocks-as-CLK_IS_CRITICAL) 2018-11-01 14:28 ` Dean Wallace @ 2018-11-01 14:49 ` Hans de Goede 2018-11-01 15:29 ` Dean Wallace ` (2 more replies) 0 siblings, 3 replies; 58+ messages in thread From: Hans de Goede @ 2018-11-01 14:49 UTC (permalink / raw) To: Dean Wallace Cc: Pierre-Louis Bossart, Andy Shevchenko, Stephen Boyd, Michael Turquette, linux-clk, Stable, Johannes Stezenbach, Andy Shevchenko, Linux Kernel Mailing List, Mogens Jensen [-- 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 ^ permalink raw reply related [flat|nested] 58+ messages in thread
* Re: Regression found (Stop-marking-clocks-as-CLK_IS_CRITICAL) 2018-11-01 14:49 ` Hans de Goede @ 2018-11-01 15:29 ` Dean Wallace 2018-11-01 15:39 ` Dean Wallace 2018-11-01 15:50 ` Dean Wallace 2 siblings, 0 replies; 58+ messages in thread From: Dean Wallace @ 2018-11-01 15:29 UTC (permalink / raw) To: Hans de Goede Cc: Pierre-Louis Bossart, Andy Shevchenko, Stephen Boyd, Michael Turquette, linux-clk, Stable, Johannes Stezenbach, Andy Shevchenko, Linux Kernel Mailing List, Mogens Jensen 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 ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: Regression found (Stop-marking-clocks-as-CLK_IS_CRITICAL) 2018-11-01 14:49 ` Hans de Goede 2018-11-01 15:29 ` Dean Wallace @ 2018-11-01 15:39 ` Dean Wallace 2018-11-01 15:50 ` Dean Wallace 2 siblings, 0 replies; 58+ messages in thread From: Dean Wallace @ 2018-11-01 15:39 UTC (permalink / raw) To: Hans de Goede Cc: Pierre-Louis Bossart, Andy Shevchenko, Stephen Boyd, Michael Turquette, linux-clk, Stable, Johannes Stezenbach, Andy Shevchenko, Linux Kernel Mailing List, Mogens Jensen 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 ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: Regression found (Stop-marking-clocks-as-CLK_IS_CRITICAL) 2018-11-01 14:49 ` Hans de Goede 2018-11-01 15:29 ` Dean Wallace 2018-11-01 15:39 ` Dean Wallace @ 2018-11-01 15:50 ` Dean Wallace 2018-11-02 10:27 ` Hans de Goede 2 siblings, 1 reply; 58+ messages in thread From: Dean Wallace @ 2018-11-01 15:50 UTC (permalink / raw) To: Hans de Goede Cc: Pierre-Louis Bossart, Andy Shevchenko, Stephen Boyd, Michael Turquette, linux-clk, Stable, Johannes Stezenbach, Andy Shevchenko, Linux Kernel Mailing List, Mogens Jensen 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 ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: Regression found (Stop-marking-clocks-as-CLK_IS_CRITICAL) 2018-11-01 15:50 ` Dean Wallace @ 2018-11-02 10:27 ` Hans de Goede 2018-11-02 11:15 ` Dean Wallace 0 siblings, 1 reply; 58+ messages in thread From: Hans de Goede @ 2018-11-02 10:27 UTC (permalink / raw) To: Dean Wallace Cc: Pierre-Louis Bossart, Andy Shevchenko, Stephen Boyd, Michael Turquette, linux-clk, Stable, Johannes Stezenbach, Andy Shevchenko, Linux Kernel Mailing List, Mogens Jensen 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 ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: Regression found (Stop-marking-clocks-as-CLK_IS_CRITICAL) 2018-11-02 10:27 ` Hans de Goede @ 2018-11-02 11:15 ` Dean Wallace 0 siblings, 0 replies; 58+ messages in thread From: Dean Wallace @ 2018-11-02 11:15 UTC (permalink / raw) To: Hans de Goede Cc: Pierre-Louis Bossart, Andy Shevchenko, Stephen Boyd, Michael Turquette, linux-clk, Stable, Johannes Stezenbach, Andy Shevchenko, Linux Kernel Mailing List, Mogens Jensen 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 ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: Regression found (Stop-marking-clocks-as-CLK_IS_CRITICAL) 2018-10-31 22:27 ` Pierre-Louis Bossart 2018-10-31 23:57 ` Dean Wallace 2018-11-01 10:37 ` Dean Wallace @ 2018-11-01 13:56 ` Hans de Goede 2 siblings, 0 replies; 58+ messages in thread From: Hans de Goede @ 2018-11-01 13:56 UTC (permalink / raw) To: Pierre-Louis Bossart, Dean Wallace Cc: Andy Shevchenko, Stephen Boyd, Michael Turquette, linux-clk, Stable, Johannes Stezenbach, Andy Shevchenko, Linux Kernel Mailing List, Mogens Jensen 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 ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: Regression found (Stop-marking-clocks-as-CLK_IS_CRITICAL) 2018-10-30 16:04 ` Hans de Goede 2018-10-30 16:15 ` Dean Wallace @ 2018-10-30 18:56 ` Mogens Jensen 2018-10-30 19:10 ` Hans de Goede 1 sibling, 1 reply; 58+ messages in thread From: Mogens Jensen @ 2018-10-30 18:56 UTC (permalink / raw) To: Hans de Goede Cc: Pierre-Louis Bossart, Dean Wallace, Andy Shevchenko, Stephen Boyd, Michael Turquette, linux-clk, Stable, Johannes Stezenbach, Carlo Caione, Andy Shevchenko, Linux Kernel Mailing List [-- 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: ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: Regression found (Stop-marking-clocks-as-CLK_IS_CRITICAL) 2018-10-30 18:56 ` Mogens Jensen @ 2018-10-30 19:10 ` Hans de Goede 2018-10-31 6:02 ` Mogens Jensen 0 siblings, 1 reply; 58+ messages in thread From: Hans de Goede @ 2018-10-30 19:10 UTC (permalink / raw) To: Mogens Jensen Cc: Pierre-Louis Bossart, Dean Wallace, Andy Shevchenko, Stephen Boyd, Michael Turquette, linux-clk, Stable, Johannes Stezenbach, Carlo Caione, Andy Shevchenko, Linux Kernel Mailing List 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 ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: Regression found (Stop-marking-clocks-as-CLK_IS_CRITICAL) 2018-10-30 19:10 ` Hans de Goede @ 2018-10-31 6:02 ` Mogens Jensen 2018-10-31 9:29 ` Hans de Goede 0 siblings, 1 reply; 58+ messages in thread From: Mogens Jensen @ 2018-10-31 6:02 UTC (permalink / raw) To: Hans de Goede Cc: Pierre-Louis Bossart, Dean Wallace, Andy Shevchenko, Stephen Boyd, Michael Turquette, linux-clk, Stable, Johannes Stezenbach, Carlo Caione, Andy Shevchenko, Linux Kernel Mailing List ‐‐‐‐‐‐‐ 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 ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: Regression found (Stop-marking-clocks-as-CLK_IS_CRITICAL) 2018-10-31 6:02 ` Mogens Jensen @ 2018-10-31 9:29 ` Hans de Goede 2018-10-31 10:03 ` Dean Wallace 2018-11-01 6:55 ` Mogens Jensen 0 siblings, 2 replies; 58+ messages in thread From: Hans de Goede @ 2018-10-31 9:29 UTC (permalink / raw) To: Mogens Jensen Cc: Pierre-Louis Bossart, Dean Wallace, Andy Shevchenko, Stephen Boyd, Michael Turquette, linux-clk, Stable, Johannes Stezenbach, Carlo Caione, Andy Shevchenko, Linux Kernel Mailing List 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 ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: Regression found (Stop-marking-clocks-as-CLK_IS_CRITICAL) 2018-10-31 9:29 ` Hans de Goede @ 2018-10-31 10:03 ` Dean Wallace 2018-11-01 6:55 ` Mogens Jensen 1 sibling, 0 replies; 58+ messages in thread From: Dean Wallace @ 2018-10-31 10:03 UTC (permalink / raw) To: Hans de Goede Cc: Mogens Jensen, Pierre-Louis Bossart, Andy Shevchenko, Stephen Boyd, Michael Turquette, linux-clk, Stable, Johannes Stezenbach, Carlo Caione, Andy Shevchenko, Linux Kernel Mailing List 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 ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: Regression found (Stop-marking-clocks-as-CLK_IS_CRITICAL) 2018-10-31 9:29 ` Hans de Goede 2018-10-31 10:03 ` Dean Wallace @ 2018-11-01 6:55 ` Mogens Jensen 2018-12-02 12:25 ` Hans de Goede 1 sibling, 1 reply; 58+ messages in thread From: Mogens Jensen @ 2018-11-01 6:55 UTC (permalink / raw) To: Hans de Goede Cc: Pierre-Louis Bossart, Dean Wallace, Andy Shevchenko, Stephen Boyd, Michael Turquette, linux-clk, Stable, Johannes Stezenbach, Carlo Caione, Andy Shevchenko, Linux Kernel Mailing List ‐‐‐‐‐‐‐ 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. ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: Regression found (Stop-marking-clocks-as-CLK_IS_CRITICAL) 2018-11-01 6:55 ` Mogens Jensen @ 2018-12-02 12:25 ` Hans de Goede 2019-01-17 5:58 ` Mogens Jensen 0 siblings, 1 reply; 58+ messages in thread From: Hans de Goede @ 2018-12-02 12:25 UTC (permalink / raw) To: Mogens Jensen Cc: Pierre-Louis Bossart, Dean Wallace, Andy Shevchenko, Stephen Boyd, Michael Turquette, linux-clk, Stable, Johannes Stezenbach, Carlo Caione, Andy Shevchenko, Linux Kernel Mailing List 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 ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: Regression found (Stop-marking-clocks-as-CLK_IS_CRITICAL) 2018-12-02 12:25 ` Hans de Goede @ 2019-01-17 5:58 ` Mogens Jensen 2019-01-17 9:12 ` Dean Wallace 0 siblings, 1 reply; 58+ messages in thread From: Mogens Jensen @ 2019-01-17 5:58 UTC (permalink / raw) To: Hans de Goede Cc: Pierre-Louis Bossart, Dean Wallace, Andy Shevchenko, Stephen Boyd, Michael Turquette, linux-clk, Stable, Johannes Stezenbach, Carlo Caione, Andy Shevchenko, Linux Kernel Mailing List ‐‐‐‐‐‐‐ 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 ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: Regression found (Stop-marking-clocks-as-CLK_IS_CRITICAL) 2019-01-17 5:58 ` Mogens Jensen @ 2019-01-17 9:12 ` Dean Wallace 2019-01-17 12:05 ` Hans de Goede 0 siblings, 1 reply; 58+ messages in thread From: Dean Wallace @ 2019-01-17 9:12 UTC (permalink / raw) To: Mogens Jensen Cc: Hans de Goede, Pierre-Louis Bossart, Andy Shevchenko, Stephen Boyd, Michael Turquette, linux-clk, Stable, Johannes Stezenbach, Carlo Caione, Andy Shevchenko, Linux Kernel Mailing List 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 ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: Regression found (Stop-marking-clocks-as-CLK_IS_CRITICAL) 2019-01-17 9:12 ` Dean Wallace @ 2019-01-17 12:05 ` Hans de Goede 2019-01-17 13:05 ` Johannes Stezenbach ` (2 more replies) 0 siblings, 3 replies; 58+ messages in thread From: Hans de Goede @ 2019-01-17 12:05 UTC (permalink / raw) To: Dean Wallace, Mogens Jensen Cc: Pierre-Louis Bossart, Andy Shevchenko, Stephen Boyd, Michael Turquette, linux-clk, Stable, Johannes Stezenbach, Carlo Caione, Andy Shevchenko, Linux Kernel Mailing List [-- 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 ^ permalink raw reply related [flat|nested] 58+ messages in thread
* Re: Regression found (Stop-marking-clocks-as-CLK_IS_CRITICAL) 2019-01-17 12:05 ` Hans de Goede @ 2019-01-17 13:05 ` Johannes Stezenbach 2019-01-17 13:16 ` Dean Wallace 2019-01-17 19:30 ` Mogens Jensen 2 siblings, 0 replies; 58+ messages in thread From: Johannes Stezenbach @ 2019-01-17 13:05 UTC (permalink / raw) To: Hans de Goede Cc: Dean Wallace, Mogens Jensen, Pierre-Louis Bossart, Andy Shevchenko, Stephen Boyd, Michael Turquette, linux-clk, Stable, Carlo Caione, Andy Shevchenko, Linux Kernel Mailing List 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 ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: Regression found (Stop-marking-clocks-as-CLK_IS_CRITICAL) 2019-01-17 12:05 ` Hans de Goede 2019-01-17 13:05 ` Johannes Stezenbach @ 2019-01-17 13:16 ` Dean Wallace 2019-01-18 15:33 ` Hans de Goede 2019-01-17 19:30 ` Mogens Jensen 2 siblings, 1 reply; 58+ messages in thread From: Dean Wallace @ 2019-01-17 13:16 UTC (permalink / raw) To: Hans de Goede Cc: Mogens Jensen, Pierre-Louis Bossart, Andy Shevchenko, Stephen Boyd, Michael Turquette, linux-clk, Stable, Johannes Stezenbach, Carlo Caione, Andy Shevchenko, Linux Kernel Mailing List 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 ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: Regression found (Stop-marking-clocks-as-CLK_IS_CRITICAL) 2019-01-17 13:16 ` Dean Wallace @ 2019-01-18 15:33 ` Hans de Goede 0 siblings, 0 replies; 58+ messages in thread From: Hans de Goede @ 2019-01-18 15:33 UTC (permalink / raw) To: Dean Wallace Cc: Mogens Jensen, Pierre-Louis Bossart, Andy Shevchenko, Stephen Boyd, Michael Turquette, linux-clk, Stable, Johannes Stezenbach, Carlo Caione, Andy Shevchenko, Linux Kernel Mailing List 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 ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: Regression found (Stop-marking-clocks-as-CLK_IS_CRITICAL) 2019-01-17 12:05 ` Hans de Goede 2019-01-17 13:05 ` Johannes Stezenbach 2019-01-17 13:16 ` Dean Wallace @ 2019-01-17 19:30 ` Mogens Jensen 2019-01-18 15:35 ` Hans de Goede 2 siblings, 1 reply; 58+ messages in thread From: Mogens Jensen @ 2019-01-17 19:30 UTC (permalink / raw) To: Hans de Goede Cc: Dean Wallace, Pierre-Louis Bossart, Andy Shevchenko, Stephen Boyd, Michael Turquette, linux-clk, Stable, Johannes Stezenbach, Carlo Caione, Andy Shevchenko, Linux Kernel Mailing List ‐‐‐‐‐‐‐ 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 ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: Regression found (Stop-marking-clocks-as-CLK_IS_CRITICAL) 2019-01-17 19:30 ` Mogens Jensen @ 2019-01-18 15:35 ` Hans de Goede 2019-01-21 5:55 ` Mogens Jensen 0 siblings, 1 reply; 58+ messages in thread From: Hans de Goede @ 2019-01-18 15:35 UTC (permalink / raw) To: Mogens Jensen Cc: Dean Wallace, Pierre-Louis Bossart, Andy Shevchenko, Stephen Boyd, Michael Turquette, linux-clk, Stable, Johannes Stezenbach, Carlo Caione, Andy Shevchenko, Linux Kernel Mailing List 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 ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: Regression found (Stop-marking-clocks-as-CLK_IS_CRITICAL) 2019-01-18 15:35 ` Hans de Goede @ 2019-01-21 5:55 ` Mogens Jensen 2019-01-22 19:27 ` Pierre-Louis Bossart 2019-01-24 10:35 ` Hans de Goede 0 siblings, 2 replies; 58+ messages in thread From: Mogens Jensen @ 2019-01-21 5:55 UTC (permalink / raw) To: Hans de Goede Cc: Dean Wallace, Pierre-Louis Bossart, Andy Shevchenko, Stephen Boyd, Michael Turquette, linux-clk, Stable, Johannes Stezenbach, Carlo Caione, Andy Shevchenko, Linux Kernel Mailing List ‐‐‐‐‐‐‐ 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 ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: Regression found (Stop-marking-clocks-as-CLK_IS_CRITICAL) 2019-01-21 5:55 ` Mogens Jensen @ 2019-01-22 19:27 ` Pierre-Louis Bossart 2019-01-25 5:16 ` Mogens Jensen 2019-01-24 10:35 ` Hans de Goede 1 sibling, 1 reply; 58+ messages in thread From: Pierre-Louis Bossart @ 2019-01-22 19:27 UTC (permalink / raw) To: Mogens Jensen, Hans de Goede Cc: Dean Wallace, Andy Shevchenko, Stephen Boyd, Michael Turquette, linux-clk, Stable, Johannes Stezenbach, Carlo Caione, Andy Shevchenko, Linux Kernel Mailing List 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? ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: Regression found (Stop-marking-clocks-as-CLK_IS_CRITICAL) 2019-01-22 19:27 ` Pierre-Louis Bossart @ 2019-01-25 5:16 ` Mogens Jensen 2019-01-25 14:12 ` Pierre-Louis Bossart 2019-01-25 17:57 ` Pierre-Louis Bossart 0 siblings, 2 replies; 58+ messages in thread From: Mogens Jensen @ 2019-01-25 5:16 UTC (permalink / raw) To: Pierre-Louis Bossart Cc: Hans de Goede, Dean Wallace, Andy Shevchenko, Stephen Boyd, Michael Turquette, linux-clk, Stable, Johannes Stezenbach, Carlo Caione, Andy Shevchenko, Linux Kernel Mailing List ‐‐‐‐‐‐‐ 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 ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: Regression found (Stop-marking-clocks-as-CLK_IS_CRITICAL) 2019-01-25 5:16 ` Mogens Jensen @ 2019-01-25 14:12 ` Pierre-Louis Bossart 2019-01-25 17:57 ` Pierre-Louis Bossart 1 sibling, 0 replies; 58+ messages in thread From: Pierre-Louis Bossart @ 2019-01-25 14:12 UTC (permalink / raw) To: Mogens Jensen Cc: Hans de Goede, Dean Wallace, Andy Shevchenko, Stephen Boyd, Michael Turquette, linux-clk, Stable, Johannes Stezenbach, Carlo Caione, Andy Shevchenko, Linux Kernel Mailing List 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 ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: Regression found (Stop-marking-clocks-as-CLK_IS_CRITICAL) 2019-01-25 5:16 ` Mogens Jensen 2019-01-25 14:12 ` Pierre-Louis Bossart @ 2019-01-25 17:57 ` Pierre-Louis Bossart 2019-01-25 20:30 ` Andy Shevchenko 1 sibling, 1 reply; 58+ messages in thread From: Pierre-Louis Bossart @ 2019-01-25 17:57 UTC (permalink / raw) To: Mogens Jensen Cc: Hans de Goede, Dean Wallace, Andy Shevchenko, Stephen Boyd, Michael Turquette, linux-clk, Stable, Johannes Stezenbach, Carlo Caione, Andy Shevchenko, Linux Kernel Mailing List >>> 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. ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: Regression found (Stop-marking-clocks-as-CLK_IS_CRITICAL) 2019-01-25 17:57 ` Pierre-Louis Bossart @ 2019-01-25 20:30 ` Andy Shevchenko 0 siblings, 0 replies; 58+ messages in thread From: Andy Shevchenko @ 2019-01-25 20:30 UTC (permalink / raw) To: Pierre-Louis Bossart Cc: Mogens Jensen, Hans de Goede, Dean Wallace, Stephen Boyd, Michael Turquette, linux-clk, Stable, Johannes Stezenbach, Carlo Caione, Linux Kernel Mailing List 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 ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: Regression found (Stop-marking-clocks-as-CLK_IS_CRITICAL) 2019-01-21 5:55 ` Mogens Jensen 2019-01-22 19:27 ` Pierre-Louis Bossart @ 2019-01-24 10:35 ` Hans de Goede 1 sibling, 0 replies; 58+ messages in thread From: Hans de Goede @ 2019-01-24 10:35 UTC (permalink / raw) To: Mogens Jensen Cc: Dean Wallace, Pierre-Louis Bossart, Andy Shevchenko, Stephen Boyd, Michael Turquette, linux-clk, Stable, Johannes Stezenbach, Carlo Caione, Andy Shevchenko, Linux Kernel Mailing List 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 ^ permalink raw reply [flat|nested] 58+ messages in thread
end of thread, other threads:[~2019-01-25 20:31 UTC | newest] Thread overview: 58+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <20181025232517.ywnw54qibemosjws@picard> 2018-10-29 17:45 ` Regression found (Stop-marking-clocks-as-CLK_IS_CRITICAL) Stephen Boyd 2018-10-29 17:52 ` Andy Shevchenko 2018-10-29 18:04 ` Andy Shevchenko 2018-10-29 19:08 ` Dean Wallace 2018-10-29 22:03 ` Pierre-Louis Bossart 2018-10-30 10:17 ` Hans de Goede 2018-10-30 11:05 ` Hans de Goede 2018-10-30 16:24 ` Andy Shevchenko 2018-10-30 11:19 ` Hans de Goede 2018-10-30 14:38 ` Dean Wallace 2018-10-30 14:48 ` Hans de Goede 2018-10-30 15:03 ` Andy Shevchenko 2018-10-30 15:25 ` Pierre-Louis Bossart 2018-10-30 15:04 ` Pierre-Louis Bossart 2018-10-30 15:46 ` Hans de Goede 2018-10-30 16:02 ` Hans de Goede 2018-10-30 16:27 ` Pierre-Louis Bossart 2018-10-30 18:31 ` Hans de Goede 2018-10-30 16:03 ` Pierre-Louis Bossart 2018-10-30 16:04 ` Hans de Goede 2018-10-30 16:15 ` Dean Wallace 2018-10-31 11:04 ` Hans de Goede 2018-10-31 12:45 ` Dean Wallace 2018-10-31 20:07 ` Dean Wallace 2018-10-31 22:27 ` Pierre-Louis Bossart 2018-10-31 23:57 ` Dean Wallace 2018-11-01 10:37 ` Dean Wallace 2018-11-01 13:57 ` Hans de Goede 2018-11-01 14:28 ` Dean Wallace 2018-11-01 14:49 ` Hans de Goede 2018-11-01 15:29 ` Dean Wallace 2018-11-01 15:39 ` Dean Wallace 2018-11-01 15:50 ` Dean Wallace 2018-11-02 10:27 ` Hans de Goede 2018-11-02 11:15 ` Dean Wallace 2018-11-01 13:56 ` Hans de Goede 2018-10-30 18:56 ` Mogens Jensen 2018-10-30 19:10 ` Hans de Goede 2018-10-31 6:02 ` Mogens Jensen 2018-10-31 9:29 ` Hans de Goede 2018-10-31 10:03 ` Dean Wallace 2018-11-01 6:55 ` Mogens Jensen 2018-12-02 12:25 ` Hans de Goede 2019-01-17 5:58 ` Mogens Jensen 2019-01-17 9:12 ` Dean Wallace 2019-01-17 12:05 ` Hans de Goede 2019-01-17 13:05 ` Johannes Stezenbach 2019-01-17 13:16 ` Dean Wallace 2019-01-18 15:33 ` Hans de Goede 2019-01-17 19:30 ` Mogens Jensen 2019-01-18 15:35 ` Hans de Goede 2019-01-21 5:55 ` Mogens Jensen 2019-01-22 19:27 ` Pierre-Louis Bossart 2019-01-25 5:16 ` Mogens Jensen 2019-01-25 14:12 ` Pierre-Louis Bossart 2019-01-25 17:57 ` Pierre-Louis Bossart 2019-01-25 20:30 ` Andy Shevchenko 2019-01-24 10:35 ` Hans de Goede
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).