Bug 201787

Created attachment 279645 [details]
lspci -vvnn

Card name: chtmax98090

not working on master neither

(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.

In commit 7876320f88802b22d4e2daf7eb027dd14175a0f8 it does work. I'm compiling 8e2aac333785f91ff74e219a1e78e6bdc1ef2c41 again...

It works with 8e2aac333785f91ff74e219a1e78e6bdc1ef2c41. I'm bisecting again.

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

Sometimes with "broken" kernels sounds works after suspension, so for me it makes sense. And my cpu is Bay Trail.

I didn't write the commit that causess de problem.

648e921888ad96ea3dc922739e96716ad3225d7f

Double checked. Is that commit. I can test patches if needed.

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

Created attachment 279783 [details]
[PATCH] ASoC: intel: cht_bsw_max98090_ti: Add pmc_plt_clk_0 quirk for Chromebook Clapper

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.

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

It works.

(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).

I preffer if you submit, and is ok if you add my email. 

Thank you!

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

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

(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.