ASoC: Intel: common: work-around for incorrect ACPI HID for CML boards by aiChaoSONG · Pull Request #1565 · thesofproject/linux

aiChaoSONG · 2019-11-29T05:57:54Z

On CML platforms with rt1011 and rt5682 codecs, there are three acpi IDs: MX98357A, 10EC1011 and 10EC5682. Previously, MX98357A will be firstly matched, and sof_rt5682 codec driver will be wrongly loaded. Due to no proper speaker codec driver loaded, the speaker on such platforms doesn't work.

Therefore, this patch add machine_quirk for each item in cml acpi mach table, with machine_quirk and quirk_data, a fully codecs match will be performed, and thus ensure correct codec drivers to be loaded.

keyonjie

Good job, @aiChaoSONG

ranj063 · 2019-11-29T06:25:48Z

@aiChaoSONG I think the correct fix for this problem is to use the appropriately quitk_data instead of switching the order to make sure that you have the required codecs based on the board

keyonjie · 2019-11-29T06:40:33Z

@ranj063 the quitk_data is used for the same machine driver only, isn't it? here we are using 3 total different machine driver.

ranj063 · 2019-11-29T17:29:33Z

@ranj063 the quitk_data is used for the same machine driver only, isn't it? here we are using 3 total different machine driver.

@keyonjie no I think the quirk_data is used along with machine_quirk to make sure that the required list of codecs is present to make sure that it is full match.

keyonjie · 2019-12-02T02:36:09Z

@ranj063 the quitk_data is used for the same machine driver only, isn't it? here we are using 3 total different machine driver.

@keyonjie no I think the quirk_data is used along with machine_quirk to make sure that the required list of codecs is present to make sure that it is full match.

makes sense. @aiChaoSONG please follow this style to fix the issue with machine_quirk + quirk_data.

keyonjie · 2019-12-02T02:59:26Z

@aiChaoSONG please consider something like this(or you can remove the existed acpi one in the snd_soc_acpi_mach.id of the array, and no need to use quirk if there is no extra codec added):

diff --git a/sound/soc/intel/common/soc-acpi-intel-cml-match.c b/sound/soc/intel/common/soc-acpi-intel-cml-match.c
index 5d08ae066738..7d90bb3b401a 100644
--- a/sound/soc/intel/common/soc-acpi-intel-cml-match.c
+++ b/sound/soc/intel/common/soc-acpi-intel-cml-match.c
@@ -9,41 +9,57 @@
 #include <sound/soc-acpi.h>
 #include <sound/soc-acpi-intel-match.h>
 
-static struct snd_soc_acpi_codecs cml_codecs = {
+static struct snd_soc_acpi_codecs rt5682_codecs = {
        .num_codecs = 1,
        .codecs = {"10EC5682"}
 };
 
-static struct snd_soc_acpi_codecs cml_spk_codecs = {
-       .num_codecs = 1,
-       .codecs = {"MX98357A"}
+static struct snd_soc_acpi_codecs rt5682_spk_codecs = {
+       .num_codecs = 2,
+       .codecs = {"10EC5682", "MX98357A"}
+};
+
+static struct snd_soc_acpi_codecs da7219_spk_codecs = {
+       .num_codecs = 2,
+       .codecs = {"10EC5682", "MX98357A"}
+};
+
+static struct snd_soc_acpi_codecs rt1011_spk_codecs = {
+       .num_codecs = 3,
+       .codecs = {"10EC1011", "10EC5682", "MX98357A"}
 };
 
 struct snd_soc_acpi_mach snd_soc_acpi_intel_cml_machines[] = {
+       /* in num_codecs descending order */
+       {
+               .id = "10EC1011",
+               .drv_name = "cml_rt1011_rt5682",
+               .machine_quirk = snd_soc_acpi_codec_list,
+               .quirk_data = &rt1011_spk_codecs,
+               .sof_fw_filename = "sof-cml.ri",
+               .sof_tplg_filename = "sof-cml-rt1011-rt5682.tplg",
+       },
        {
                .id = "DLGS7219",
                .drv_name = "cml_da7219_max98357a",
-               .quirk_data = &cml_spk_codecs,
+               .machine_quirk = snd_soc_acpi_codec_list,
+               .quirk_data = &da7219_spk_codecs,
                .sof_fw_filename = "sof-cml.ri",
                .sof_tplg_filename = "sof-cml-da7219-max98357a.tplg",
        },
        {
                .id = "MX98357A",
                .drv_name = "sof_rt5682",
-               .quirk_data = &cml_codecs,
+               .machine_quirk = snd_soc_acpi_codec_list,
+               .quirk_data = &rt5682_spk_codecs,
                .sof_fw_filename = "sof-cml.ri",
                .sof_tplg_filename = "sof-cml-rt5682-max98357a.tplg",
        },
-       {
-               .id = "10EC1011",
-               .drv_name = "cml_rt1011_rt5682",
-               .quirk_data = &cml_codecs,
-               .sof_fw_filename = "sof-cml.ri",
-               .sof_tplg_filename = "sof-cml-rt1011-rt5682.tplg",
-       },
        {
                .id = "10EC5682",
                .drv_name = "sof_rt5682",
+               .machine_quirk = snd_soc_acpi_codec_list,
+               .quirk_data = &rt5682_codecs,
                .sof_fw_filename = "sof-cml.ri",
                .sof_tplg_filename = "sof-cml-rt5682.tplg",
        },

ranj063 · 2019-12-02T15:56:18Z

sound/soc/intel/common/soc-acpi-intel-cml-match.c

+
+static struct snd_soc_acpi_codecs da7219_spk_codecs = {
+	.num_codecs = 2,
+	.codecs = {"10EC5682", "MX98357A"}


@aiChaoSONG where is the da7219 is the codecs list?

Oh, forgive me, da7219 is the codecs list, and the '10EC5682' here should be 'DLGS7219'. will fix this in next push

ranj063 · 2019-12-02T15:57:12Z

sound/soc/intel/common/soc-acpi-intel-cml-match.c

+
+static struct snd_soc_acpi_codecs rt1011_spk_codecs = {
+	.num_codecs = 3,
+	.codecs = {"10EC1011", "10EC5682", "MX98357A"}


and is this correct? Does this mean the ACPI match will fail if all 3 codecs arent present?

if any one of the listed codecs not present, the match will fail, and continue to match the next item in mach table.

@aiChaoSONG yes I understand that. My question was whether this is an actual board config for CML?

@aiChaoSONG the cml_rt1011_rt5682 machine driver doesnt seem to handle the MX98357A. Are you sure this is the driver to use with this config?

@ranj063 the sof_rt5682 driver can handle two conditions: only rt5682 and rt5682 + max98357a, and the cml_rt1011_rt5682 driver uses the handler under sof_rt5682.

@plbossart As I understand from your text, we keep two items both with .id=10EC5682, and codec MX98357A in quirk_data of the first board, and 10EC1011 in the other. Do I get you right?

Codec list of board 2 is a super set of board 1, that means where board 1 can match, Board 2 matches, too.
Solution d): Let me push coreboot team to fix their issue, and eliminate MX98357A in board 2.

@aiChaoSONG @plbossart

Solution from Pierre will work , just that we need to swap the order
use .id=10EC5682 for both, and add a machine quirk with 10EC1011 for the first and MX98357a for the second.

To Answer @plbossart further,

CML device #1 : match RT1011 and RT5682
CML device #2: match max98357a and RT5682

If we use #2 as 1st entry, device #1 also pick that because

In Coreboot, Device #2 is used as base config and max98357a and RT5682 added.
Device #1 adds a override to add RT1011. Now since max98367a is not an i2c device, it continue to be present in ACPI.

Hence device #1 ACPI contains max98357a . ( This is what @aiChaoSONG reported and i'll check with Coreboot if possible)

If we have the order as below it can solve for all, let me know if you are ok

1st entry : RT5682 ( quirk of RT1011)
2nd entry: RT5682 (quirt of MAX98357a)
3rd entry: RT5682 ( no quirk - for sof_rt5682)
4th entry: da7219 (quirk of MAX98357a)

let me know if the match works for all with this.

@sathyap-chrome The order matters even with Pierre's solution, right? I am fine with this, but Pierre is the one who makes the final decision. @plbossart Are you fine with this?

yes, this is what I had in mind.

1st entry : RT5682 ( quirk of RT1011)
2nd entry: RT5682 (quirt of MAX98357a)
3rd entry: RT5682 ( no quirk - for sof_rt5682)
4th entry: da7219 (quirk of MAX98357a)

I am not sure why I wrote the order does not matter, it does indeed. the point was to use the speaker amplifier that's really present on the board to remove the ambiguity.

We'd of course need a comment to state that the order does matter and why, so that future generations don't break this by accident.

@plbossart repushed, and tested on board with max98357a and boad with rt1011, correct driver is loaded on both boards, please review again.

plbossart · 2019-12-02T16:41:04Z

I am not touching this PR until I have a clear picture of all CML platforms listed, each with the ACPI IDs expected.

aiChaoSONG · 2019-12-03T02:06:34Z

I am not touching this PR until I have a clear picture of all CML platforms listed, each with the ACPI IDs expected.
@plbossart I have synced with Mengdong, and there are three CML platforms with different codecs till now, they are CML with rt5682 and max98357a codecs(IDs:10EC5682 MX98357A), CML with rt1011 and rt5682 (IDs: 10EC1011 10EC5682) and CML with da7219 and max98357a codecs (IDs: DLGS7219 MX98357A)
Note: The MX98357A is present in CML with rt1011 and rt5682, through max98357a codec is not on the hardware, this is a known issue of coreboot

plbossart

minor nit-picks to make the explanations more self-explanatory.

I am not a native English speaker but I'd recommend avoiding sentences with 'there is' and follow the 'subject-verb-complement' pattern in an active form.

Thanks!

plbossart · 2019-12-05T03:44:45Z

sound/soc/intel/common/soc-acpi-intel-cml-match.c

+/*
+ * The order of the three items with .id = "10EC5682" matters
+ * here, because the ACPI ID of max98357a is wrongly present
+ * in the board with rt1011 speaker codec.


because the DSDT tables expose an ACPI HID for the MAX98357A speaker amplifier which is not populated on the board.

plbossart · 2019-12-05T03:52:49Z

sound/soc/intel/common/soc-acpi-intel-cml-match.c

@@ -9,45 +9,52 @@
 #include <sound/soc-acpi.h>
 #include <sound/soc-acpi-intel-match.h>



the commit message can be improved a bit:

ASoC: Intel: common: work-around incorrect ACPI HID for CML boards

On CML boards with the RT5682 headset codec and RT1011 speaker amplifier, the platform firmware exposes three ACPI HIDs (10EC5682, 10EC1011 and MX98357A). The last HID is a mistake in DSDT tables, which causes the wrong machine driver to be loaded.

This patch changes the key used to identify boards and changes the order of entries in the table to load the correct machine driver. The order does matter and should not be modified to work-around this firmware issue.

@plbossart thank you, Pierre, commit message from you is better, and will avoid the personal sytle of "there is" in later PRs.

On CML boards with the RT5682 headset codec and RT1011 speaker amplifier, the platform firmware exposes three ACPI HIDs (10EC5682, 10EC1011 and MX98357A). The last HID is a mistake in DSDT tables, which causes the wrong machine driver to be loaded. This patch changes the key used to identify boards and changes the order of entries in the table to load the correct machine driver. The order does matter and should not be modified to work-around this firmware issue. Signed-off-by: Amery Song <chao.song@intel.com> Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>

plbossart

Thanks!

plbossart · 2019-12-05T14:58:51Z

I'd like additional reviewers from the Chrome team, @sathyap-chrome @sathya-nujella

ranj063

LGTM but a minor nit-pick. work-around or workaround is a noun. so it should the commit title should be "ASoC: Intel: common: work-around for incorrect ACPI HID for CML boards"

sathya-nujella · 2019-12-05T19:30:47Z

Changes look good. Thanks!

"The last HID is a mistake in DSDT tables, which causes the wrong machine driver to be loaded." is mentioned.
This may be a bug in firmware which also should be fixed too (if problem with latest firmware too), so that no stale devices are in DSDT table.

aiChaoSONG · 2019-12-06T01:30:41Z

LGTM but a minor nit-pick. work-around or workaround is a noun. so it should the commit title should be "ASoC: Intel: common: work-around for incorrect ACPI HID for CML boards"

The commit title is already "ASoC: Intel: common: work-around for incorrect ACPI HID for CML boards", just a github PR title change, Thanks, Ranjani

[ Upstream commit 7175e13 ] I experience issues when putting a lkbsb on the stack and have sb_lvbptr field to a dangled pointer while not using DLM_LKF_VALBLK. It will crash with the following kernel message, the dangled pointer is here 0xdeadbeef as example: [ 102.749317] BUG: unable to handle page fault for address: 00000000deadbeef [ 102.749320] #PF: supervisor read access in kernel mode [ 102.749323] #PF: error_code(0x0000) - not-present page [ 102.749325] PGD 0 P4D 0 [ 102.749332] Oops: 0000 [thesofproject#1] PREEMPT SMP PTI [ 102.749336] CPU: 0 PID: 1567 Comm: lock_torture_wr Tainted: G W 5.19.0-rc3+ thesofproject#1565 [ 102.749343] Hardware name: Red Hat KVM/RHEL-AV, BIOS 1.16.0-2.module+el8.7.0+15506+033991b0 04/01/2014 [ 102.749344] RIP: 0010:memcpy_erms+0x6/0x10 [ 102.749353] Code: cc cc cc cc eb 1e 0f 1f 00 48 89 f8 48 89 d1 48 c1 e9 03 83 e2 07 f3 48 a5 89 d1 f3 a4 c3 66 0f 1f 44 00 00 48 89 f8 48 89 d1 <f3> a4 c3 0f 1f 80 00 00 00 00 48 89 f8 48 83 fa 20 72 7e 40 38 fe [ 102.749355] RSP: 0018:ffff97a58145fd08 EFLAGS: 00010202 [ 102.749358] RAX: ffff901778b77070 RBX: 0000000000000000 RCX: 0000000000000040 [ 102.749360] RDX: 0000000000000040 RSI: 00000000deadbeef RDI: ffff901778b77070 [ 102.749362] RBP: ffff97a58145fd10 R08: ffff901760b67a70 R09: 0000000000000001 [ 102.749364] R10: ffff9017008e2cb8 R11: 0000000000000001 R12: ffff901760b67a70 [ 102.749366] R13: ffff901760b78f00 R14: 0000000000000003 R15: 0000000000000001 [ 102.749368] FS: 0000000000000000(0000) GS:ffff901876e00000(0000) knlGS:0000000000000000 [ 102.749372] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 102.749374] CR2: 00000000deadbeef CR3: 000000017c49a004 CR4: 0000000000770ef0 [ 102.749376] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 [ 102.749378] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 [ 102.749379] PKRU: 55555554 [ 102.749381] Call Trace: [ 102.749382] <TASK> [ 102.749383] ? send_args+0xb2/0xd0 [ 102.749389] send_common+0xb7/0xd0 [ 102.749395] _unlock_lock+0x2c/0x90 [ 102.749400] unlock_lock.isra.56+0x62/0xa0 [ 102.749405] dlm_unlock+0x21e/0x330 [ 102.749411] ? lock_torture_stats+0x80/0x80 [dlm_locktorture] [ 102.749416] torture_unlock+0x5a/0x90 [dlm_locktorture] [ 102.749419] ? preempt_count_sub+0xba/0x100 [ 102.749427] lock_torture_writer+0xbd/0x150 [dlm_locktorture] [ 102.786186] kthread+0x10a/0x130 [ 102.786581] ? kthread_complete_and_exit+0x20/0x20 [ 102.787156] ret_from_fork+0x22/0x30 [ 102.787588] </TASK> [ 102.787855] Modules linked in: dlm_locktorture torture rpcsec_gss_krb5 intel_rapl_msr intel_rapl_common kvm_intel iTCO_wdt iTCO_vendor_support kvm vmw_vsock_virtio_transport qxl irqbypass vmw_vsock_virtio_transport_common drm_ttm_helper crc32_pclmul joydev crc32c_intel ttm vsock virtio_scsi virtio_balloon snd_pcm drm_kms_helper virtio_console snd_timer snd drm soundcore syscopyarea i2c_i801 sysfillrect sysimgblt i2c_smbus pcspkr fb_sys_fops lpc_ich serio_raw [ 102.792536] CR2: 00000000deadbeef [ 102.792930] ---[ end trace 0000000000000000 ]--- This patch fixes the issue by checking also on DLM_LKF_VALBLK on exflags is set when copying the lvbptr array instead of if it's just null which fixes for me the issue. I think this patch can fix other dlm users as well, depending how they handle the init, freeing memory handling of sb_lvbptr and don't set DLM_LKF_VALBLK for some dlm_lock() calls. It might a there could be a hidden issue all the time. However with checking on DLM_LKF_VALBLK the user always need to provide a sb_lvbptr non-null value. There might be more intelligent handling between per ls lvblen, DLM_LKF_VALBLK and non-null to report the user the way how DLM API is used is wrong but can be added for later, this will only fix the current behaviour. Cc: stable@vger.kernel.org Signed-off-by: Alexander Aring <aahringo@redhat.com> Signed-off-by: David Teigland <teigland@redhat.com> Signed-off-by: Sasha Levin <sashal@kernel.org>

aiChaoSONG requested review from lgirdwood and plbossart as code owners November 29, 2019 05:57

aiChaoSONG requested review from keyonjie, ranj063 and wwittbrx November 29, 2019 05:58

aiChaoSONG changed the title ~~ASoC: Intel: common: adjust mach table order for~~ ASoC: Intel: common: adjust mach table order for cml_rt1011_rt5682 Nov 29, 2019

aiChaoSONG mentioned this pull request Nov 29, 2019

[BUG][CML] ipc timeout when playing via speaker pipeline thesofproject/sof#2107

Closed

keyonjie previously approved these changes Nov 29, 2019

View reviewed changes

aiChaoSONG mentioned this pull request Dec 2, 2019

[BUB][CML] no sound output when aplay via speaker pipeline #1569

Closed

aiChaoSONG dismissed keyonjie’s stale review via a375474 December 2, 2019 06:11

aiChaoSONG changed the title ~~ASoC: Intel: common: adjust mach table order for cml_rt1011_rt5682~~ ASoC: Intel: common: add machine_quirk for CML acpi mach table Dec 2, 2019

ranj063 reviewed Dec 2, 2019

View reviewed changes

aiChaoSONG changed the title ~~ASoC: Intel: common: add machine_quirk for CML acpi mach table~~ ASoC: Intel: common: refine acpi mach table for CML Dec 5, 2019

plbossart requested changes Dec 5, 2019

View reviewed changes

plbossart approved these changes Dec 5, 2019

View reviewed changes

plbossart requested a review from ranj063 December 5, 2019 14:55

ranj063 approved these changes Dec 5, 2019

View reviewed changes

aiChaoSONG changed the title ~~ASoC: Intel: common: refine acpi mach table for CML~~ ASoC: Intel: common: work-around for incorrect ACPI HID for CML boards Dec 6, 2019

plbossart merged commit 7c7cc04 into thesofproject:topic/sof-dev Dec 7, 2019

aiChaoSONG mentioned this pull request Dec 12, 2019

kmod_scripts: add support for snd_soc_cml_rt1011_rt5682 thesofproject/sof#2198

Merged

		@@ -9,45 +9,52 @@
		#include <sound/soc-acpi.h>
		#include <sound/soc-acpi-intel-match.h>

Conversation

aiChaoSONG commented Nov 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

keyonjie left a comment

Choose a reason for hiding this comment

Uh oh!

ranj063 commented Nov 29, 2019

Uh oh!

keyonjie commented Nov 29, 2019

Uh oh!

ranj063 commented Nov 29, 2019

Uh oh!

keyonjie commented Dec 2, 2019

Uh oh!

keyonjie commented Dec 2, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aiChaoSONG Dec 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ranj063 Dec 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aiChaoSONG Dec 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

plbossart commented Dec 2, 2019

Uh oh!

aiChaoSONG commented Dec 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

plbossart commented Dec 5, 2019

Uh oh!

ranj063 left a comment

Choose a reason for hiding this comment

Uh oh!

sathya-nujella commented Dec 5, 2019

Uh oh!

aiChaoSONG commented Dec 6, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

aiChaoSONG commented Nov 29, 2019 •

edited

Loading

aiChaoSONG Dec 3, 2019 •

edited

Loading

ranj063 Dec 3, 2019 •

edited

Loading

aiChaoSONG Dec 4, 2019 •

edited

Loading

aiChaoSONG commented Dec 3, 2019 •

edited

Loading