Bug 201787 - Audio stops working
Summary: Audio stops working
Status: RESOLVED CODE_FIX
Alias: None
Product: Drivers
Classification: Unclassified
Component: Sound(ALSA) (show other bugs)
Hardware: Intel Linux
: P1 normal
Assignee: Jaroslav Kysela
URL: https://bugzilla.kernel.org/show_bug....
Keywords:
Depends on:
Blocks:
 
Reported: 2018-11-25 18:51 UTC by Jaime Pérez
Modified: 2020-04-25 14:16 UTC (History)
3 users (show)

See Also:
Kernel Version: 4.19-rc4+
Subsystem:
Regression: Yes
Bisected commit-id:


Attachments
lspci -vvnn (10.82 KB, text/plain)
2018-11-25 18:54 UTC, Jaime Pérez
Details
[PATCH] ASoC: intel: cht_bsw_max98090_ti: Add pmc_plt_clk_0 quirk for Chromebook Clapper (1.59 KB, patch)
2018-12-02 12:14 UTC, Hans de Goede
Details | Diff

Description Jaime Pérez 2018-11-25 18:51:31 UTC
This commit makes my audio stop working:

8e2aac333785f91ff74e219a1e78e6bdc1ef2c41

Introduced by linus.walleij@linaro.org to fix bug described on https://bugzilla.kernel.org/show_bug.cgi?id=200787
Comment 1 Jaime Pérez 2018-11-25 18:54:08 UTC
Created attachment 279645 [details]
lspci -vvnn
Comment 2 Jaime Pérez 2018-11-25 18:54:49 UTC
Card name: chtmax98090
Comment 3 Jaime Pérez 2018-11-25 19:21:47 UTC
not working on master neither
Comment 4 Takashi Iwai 2018-11-26 11:10:49 UTC
(In reply to Jaime Pérez from comment #0)
> This commit makes my audio stop working:
> 
> 8e2aac333785f91ff74e219a1e78e6bdc1ef2c41

Are you sure?  This is about Cannon Lake pinctrl driver, and yours is ATOM.
Please double-check.
Comment 5 Jaime Pérez 2018-11-26 19:43:49 UTC
In commit 7876320f88802b22d4e2daf7eb027dd14175a0f8 it does work. I'm compiling 8e2aac333785f91ff74e219a1e78e6bdc1ef2c41 again...
Comment 6 Jaime Pérez 2018-11-27 06:04:36 UTC
It works with 8e2aac333785f91ff74e219a1e78e6bdc1ef2c41. I'm bisecting again.
Comment 7 Jaime Pérez 2018-11-30 14:33:46 UTC
I got this now:

Author: Hans de Goede <hdegoede@redhat.com>
Date:   Wed Sep 12 11:34:56 2018 +0200

    clk: x86: Stop marking clocks as CLK_IS_CRITICAL
    
    Commit d31fd43c0f9a ("clk: x86: Do not gate clocks enabled by the
    firmware"), which added the code to mark clocks as CLK_IS_CRITICAL, causes
    all unclaimed PMC clocks on Cherry Trail devices to be on all the time,
    resulting on the device not being able to reach S0i3 when suspended.
    
    The reason for this commit is that on some Bay Trail / Cherry Trail devices
    the r8169 ethernet controller uses pmc_plt_clk_4. Now that the clk-pmc-atom
    driver exports an "ether_clk" alias for pmc_plt_clk_4 and the r8169 driver
    has been modified to get and enable this clock (if present) the marking of
    the clocks as CLK_IS_CRITICAL is no longer necessary.
    
    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.
    
    Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=193891#c102
    Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=196861
    Cc: Johannes Stezenbach <js@sig21.net>
    Cc: Carlo Caione <carlo@endlessm.com>
    Reported-by: Johannes Stezenbach <js@sig21.net>
    Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
    Acked-by: Stephen Boyd <sboyd@kernel.org>
    Signed-off-by: Hans de Goede <hdegoede@redhat.com>
    Signed-off-by: David S. Miller <davem@davemloft.net>


I will compile previous again to check it works
Comment 8 Jaime Pérez 2018-11-30 14:57:12 UTC
Sometimes with "broken" kernels sounds works after suspension, so for me it makes sense. And my cpu is Bay Trail.
Comment 9 Jaime Pérez 2018-11-30 16:06:03 UTC
I didn't write the commit that causess de problem.

648e921888ad96ea3dc922739e96716ad3225d7f
Comment 10 Jaime Pérez 2018-11-30 16:13:52 UTC
Double checked. Is that commit. I can test patches if needed.
Comment 11 Hans de Goede 2018-12-02 12:11:43 UTC
Hi Jaime,

Thank you for your bug report and sorry that my patch is causing trouble for you.

Other people have reported a similar problem on some Chromebooks using the chtmax98090 machine driver.

The root cause here is not my patch, but the cht_bsw_max98090_ti.c machine-driver using the wrong clock on some boards (for some reason some Chromebooks use a non standard clock). My patch just makes this pre-existing problem visible.

This has already been fixed for the Swanky model Chromebook:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=a182ecd3809c8d5a2da80c520f3602e301c5317e

I still had writing a similar patch for the Clapper Chromebook on my TODO list. I've just done that in case you've a Clapper model. I will attach the patch for the Clapper here.

If you have neither a Swanky nor a Clapper model Chromebook, or not a Chromebook at all, then a similar patch is needed for your model.

You can find out which clock you are using by *booting a working kernel* and then as root doing:

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

On the Swanky this returns:

/sys/kernel/debug/clk/pmc_plt_clk_0: CLK_IS_CRITICAL

/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:

Indicating that pmc_plt_clk_0 is affected by my 648e921888ad96ea3dc922739e96716ad3225d7f commit and thus is the one used by the audio (given that my commit breaks the audio).

Then you will want to find some unique (non generic) strings for the dmi match, do:

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

Note do no use serial fields :)   On Chromebooks typically the product_name field (DMI_PRODUCT_NAME in the kernel) is the best field to match on, also you can specify up to 4 fields to match on, e.g.:

                /* Chuwi Vi10 (CWI505) */
                .matches = {
                        DMI_MATCH(DMI_BOARD_VENDOR, "Hampoo"),
                        DMI_MATCH(DMI_BOARD_NAME, "BYT-PF02"),
                        DMI_MATCH(DMI_SYS_VENDOR, "ilife"),
                        DMI_MATCH(DMI_PRODUCT_NAME, "S165"),
                },

Regards,

Hans
Comment 12 Hans de Goede 2018-12-02 12:14:41 UTC
Created attachment 279783 [details]
[PATCH] ASoC: intel: cht_bsw_max98090_ti: Add pmc_plt_clk_0 quirk for Chromebook Clapper
Comment 13 Jaime Pérez 2018-12-02 18:59:37 UTC
Hi Hans, 

thanks for answering. Don't worry, as I have ubuntu 4.15 kernel as default and there I have audio. I was just testing new releases. 

I have a Gnawty chromebook (Acer cb3-111). I append outputs of commands:

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: 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


I suppose 0x00000800 is the CLK_IS_CRITICAL flag.

So I have done 

static const struct dmi_system_id cht_max98090_quirk_table[] = {
        {
                /* Swanky model Chromebook (Toshiba Chromebook 2) */
                .matches = {
                        DMI_MATCH(DMI_PRODUCT_NAME, "Swanky"),
                        DMI_MATCH(DMI_PRODUCT_NAME, "Gnawty")
                },
                .driver_data = (void *)QUIRK_PMC_PLT_CLK_0,
        },
        {}
};

over master and I'm compiling.
Comment 14 Jaime Pérez 2018-12-02 19:03:03 UTC
I think I don't have to do it in that way. I will do 

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,
        },
        {
                /* Gnawty model Chromebook (Acer Chromebook CB3-111) */
                .matches = {
                        DMI_MATCH(DMI_PRODUCT_NAME, "Gnawty"),
                },
                .driver_data = (void *)QUIRK_PMC_PLT_CLK_0,
        }
};


better
Comment 15 Jaime Pérez 2018-12-03 06:44:00 UTC
It works.
Comment 16 Hans de Goede 2018-12-03 10:27:41 UTC
(In reply to Jaime Pérez from comment #15)
> It works.

Great, thank you for the bug report and for helping with getting this fixed.

Do you want to submit a patch for this upstream yourself, or shall I submit a patch upstream?

If you want me to submit a patch, is it ok if I add:

Reported-and-tested-by: Jaime Pérez <19.jaime.91@XXXXXX.com>

To the commit message of the patch (with XXXXXX being your actual email address) ?

If you submit a patch upstream yourself please add my Clapper patch to your kernel tree with your patch on top to avoid conflicts (save the patch and git am it, then make your own changes).
Comment 17 Jaime Pérez 2018-12-03 11:13:35 UTC
I preffer if you submit, and is ok if you add my email. 

Thank you!
Comment 18 Pierre Bossart 2018-12-03 14:55:02 UTC
Should we add this sort of information to alsa-info.sh? there will likely be other skews where this quirk is needed.


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: 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
Comment 19 Takashi Iwai 2018-12-03 15:33:13 UTC
I'm fine to add that information, but please check the existence of sysfs entry at first.  Otherwise this will lead to many error messages :)
Comment 20 Hans de Goede 2018-12-03 20:45:44 UTC
(In reply to Jaime Pérez from comment #17)
> I preffer if you submit, and is ok if you add my email. 

Done, patch created and submitted upstream.

Note You need to log in before you can comment on or make changes to this bug.