ASoC: soundwire: fix broken BIOS with _ADR workaround by plbossart · Pull Request #2701 · thesofproject/linux

plbossart · 2021-01-19T17:52:52Z

On some HP devices, the BIOS does not advertise the presence of RT711
on link0 and RT1308 on link1. This has been a known issue on reference
platforms, but this problem was not expected to spill into commercial
devices.

Unfortunately the BIOS remained broken and we can't reasonably expect
every Linux user to override the DSDT.

This patch suggests the use of a workaround. Broken BIOS expose the
part 0x701 on all links, which does not correspond to a real device or
configurations. We can however use this information to make existing
codec drivers can probe on this device instead.

BugLink: #2700
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com

plbossart · 2021-01-19T19:55:00Z

pushed the wrong version, fixed now.

plbossart · 2021-01-20T02:09:03Z

@bardliao @RanderWang I just pushed a new version that works on CML-U w/ SoundWire, where I aliased rt700-link0 to rt711 and rt700-link1 to rt1308-1.
Can you take a look? To test on the HP device, we only need to change 0x700 -> 0x701 for TGL. Thanks!

Also we need a rt711-rt1308-mono topology with DMICs for that configuration. Is this something you can prepare (should be similar to TGL-RVP but there's a single RT1308, not 2 on link1).

RanderWang · 2021-01-20T03:00:35Z

@bardliao @RanderWang I just pushed a new version that works on CML-U w/ SoundWire, where I aliased rt700-link0 to rt711 and rt700-link1 to rt1308-1.
Can you take a look? To test on the HP device, we only need to change 0x700 -> 0x701 for TGL. Thanks!

Also we need a rt711-rt1308-mono topology with DMICs for that configuration. Is this something you can prepare (should be similar to TGL-RVP but there's a single RT1308, not 2 on link1).

sure, I will test it on tgl rvp

RanderWang · 2021-01-20T06:57:03Z

@plbossart yes, we need to change 0x700 -> 0x701 (int codec driver) for TGL. With above fix, it can work on TGL RVP.

kv2019i

Complicated but in general looks ok. A few notes on readability from a not-so-familiar-with-sdw point of view.

plbossart · 2021-01-20T20:13:03Z

@RanderWang can you give this PR a try with no ACPI initrd override?

I was mistaken, we already have the sof-tpg-rt711-rt1308 which can work. However we are missing the DMIC autodetection, we need to figure out how to add the -2ch or -4ch suffix automagically based on NHLT information. We can't ask users to manually rename topology filenames.

RanderWang · 2021-01-21T01:31:50Z

@RanderWang can you give this PR a try with no ACPI initrd override?

I was mistaken, we already have the sof-tpg-rt711-rt1308 which can work. However we are missing the DMIC autodetection, we need to figure out how to add the -2ch or -4ch suffix automagically based on NHLT information. We can't ask users to manually rename topology filenames.

The RT711 on my TGL RVP is a external one and it can't work properly for many bus clash but rt1308 is in good status.
The status of this PR on my TGL:
(1) sound card is registered successfully and all pcm stream are set up correctly
(2) I didn't get any error for playback on RT1308

update: I have removed ACPI override yesterday so I could test with this workaround.

bardliao · 2021-01-21T02:31:07Z

@plbossart See sof-ci/jenkins/pr-device-test, the TGLH_RVP_HDA verify-pcm-list.sh case

kernel: [    4.311194] sof-audio-pci 0000:00:1f.3: found 701 at link 0
kernel: [    4.311195] sof-audio-pci 0000:00:1f.3: found 701 at link 1
kernel: [    4.311197] sof-audio-pci 0000:00:1f.3: SoundWire machine driver sof_sdw topology sof-tgl-rt711-rt1308.tplg

It is a HDA device and shouldn't use sof_sdw machine driver

And the TGLU_RVP_SDW check-playback-10sec.sh case

kernel: [ 98.850569] rt711 sdw:0:25d:711:0: DPN_PortCtrl register write failed for port 3 
kernel: [ 98.850570] soundwire sdw-master-0: Program transport params failed: -61
kernel: [ 98.850572] soundwire sdw-master-0: Program params failed: -61

and below is from the verify-pcm-list case

kernel: [    4.632547] rt711 sdw:0:25d:701:0: probe complete
kernel: [    4.632930] rt711 sdw:0:25d:711:0: probe complete

Look like rt711 codec driver is probed for both sdw:0:25d:711:0 and sdw:1:25d:700:0.

The PR do impact the existing devices. Does this PR for testing only or it is attempting to mere and upstream?

plbossart · 2021-01-21T03:06:23Z

Thanks @bardliao for the details, much appreciated.

This PR was an attempt to fix the HP issue, where there is no RT711 at all in the BIOS.

I do not understand at all how SoundWire could be selected on TGL-H RVP HDA. If this is an HDaudio platform, then the SoundWire link mask should be zero. Something is seriously wrong in the BIOS settings and I would assert that this PR did not change anything here.

see the dmesg trace on TGL-H:

kernel: [    4.272911] acpi PRP00001:00: ACPI reports 4 SDW Link devices
kernel: [    4.272918] acpi PRP00001:00: Link 2 not selected in firmware
kernel: [    4.272920] acpi PRP00001:00: Link 3 not selected in firmware
kernel: [    4.272921] acpi PRP00001:00: Creating 4 SDW Link devices

Link 0 and 1 should be disabled as well if HDaudio is supposed to be the configuration. This is just wrong and not something we can fix. @RanderWang @fredoh9 can you please look at BIOS settings here and the HDaudio advanced menu?

The PR has a fatal flow though in case the DSDT contains both RT711 and RT701, which is another level of brokenness. There is a hacky solution though: we can add a DMI quirk to only allow the _ADR override on specific devices and links, and report -ENODEV otherwise (it's harmless).

But first things first, why do we even have a SoundWire detection on that machine?

RanderWang · 2021-01-21T05:14:25Z

@aiChaoSONG @bardliao

Thanks @bardliao for the details, much appreciated.

This PR was an attempt to fix the HP issue, where there is no RT711 at all in the BIOS.

I do not understand at all how SoundWire could be selected on TGL-H RVP HDA. If this is an HDaudio platform, then the SoundWire link mask should be zero. Something is seriously wrong in the BIOS settings and I would assert that this PR did not change anything here.

see the dmesg trace on TGL-H:
kernel: [    4.272911] acpi PRP00001:00: ACPI reports 4 SDW Link devices
kernel: [    4.272918] acpi PRP00001:00: Link 2 not selected in firmware
kernel: [    4.272920] acpi PRP00001:00: Link 3 not selected in firmware
kernel: [    4.272921] acpi PRP00001:00: Creating 4 SDW Link devices
Link 0 and 1 should be disabled as well if HDaudio is supposed to be the configuration. This is just wrong and not something we can fix. @RanderWang @fredoh9 can you please look at BIOS settings here and the HDaudio advanced menu?

The PR has a fatal flow though in case the DSDT contains both RT711 and RT701, which is another level of brokenness. There is a hacky solution though: we can add a DMI quirk to only allow the _ADR override on specific devices and links, and report -ENODEV otherwise (it's harmless).

But first things first, why do we even have a SoundWire detection on that machine?

@aiChaoSONG could you help to check BIOS setting on this device ? Thanks! please check CI to get device name

bardliao · 2021-01-21T05:37:40Z

But first things first, why do we even have a SoundWire detection on that machine?

I am guessing HDA and SDW are not conflict in BIOS configuration. It will be a problem if we see the same configuration on real products.

aiChaoSONG · 2021-01-21T05:41:08Z

SOFCI TEST
@bardliao @RanderWang @plbossart On all three of our TGLH_RVP_HDA, SDW link 0 and 1 are enabled, and now all SDW links are disabled.

I am guessing HDA and SDW are not conflict in BIOS configuration

Yes, we can enable SDW links, SSPs, and HDA at the same time on TGLH_RVP_HDA.

RanderWang · 2021-01-21T06:15:31Z

@RanderWang can you give this PR a try with no ACPI initrd override?

I was mistaken, we already have the sof-tpg-rt711-rt1308 which can work. However we are missing the DMIC autodetection, we need to figure out how to add the -2ch or -4ch suffix automagically based on NHLT information. We can't ask users to manually rename topology filenames.

@plbossart I create a PR #2706 to address some issues in 2700

lyakh

a formal LGTM ack

plbossart · 2021-01-21T16:57:00Z

I restricted this PR with a DMI quirk, so now there should be no need to tweak CI devices.

RanderWang · 2021-01-22T09:07:52Z

@plbossart this pr is good for me. I remember in early stage of sdw development you fixed such id issue in sdw layer. So I have another idea, can we add quirk in sdw layer ? Now intel and qcom support sdw and we can create a quirk function for all platforms, just like HDA framework. Then we can fix sdw id issue at root point and we don't need to change codec driver, sof driver and machine driver.

plbossart · 2021-01-22T15:05:56Z

@plbossart this pr is good for me. I remember in early stage of sdw development you fixed such id issue in sdw layer. So I have another idea, can we add quirk in sdw layer ? Now intel and qcom support sdw and we can create a quirk function for all platforms, just like HDA framework. Then we can fix sdw id issue at root point and we don't need to change codec driver, sof driver and machine driver.

I don't know how we could do this at the ACPI layer.

None of the existing codec drivers set the sdw_driver.name, but instead set sdw_driver.driver.name. This leads to error messages such as [ 23.935355] rt700 sdw:2:25d:700:0: Probe of (null) failed: -19 We could remove this sdw_driver.name if it doesn't have any purpose. This patch only suggests using the proper indirection. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

On some HP devices, the DSDT does not advertise the presence of RT1308 on link1. This has been a known issue on reference platforms, but this problem was not expected to spill into commercial devices. Unfortunately the DSDT remained broken and we can't reasonably expect every Linux user to override the DSDT. This patch suggests the use of a workaround. Broken BIOS expose the part 0x701 on all links, which does not correspond to a real device or configurations. We can however use this information to make existing codec drivers probe on this device instead. A DMI-based quirk is introduce to avoid using this workaround on existing devices. BugLink: thesofproject#2700 Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

On some HP devices, the DSDT does not advertise the presence of RT711 on link0. This has been a known issue on reference platforms, but this problem was not expected to spill into commercial devices. Unfortunately the DSDT remained broken and we can't reasonably expect every Linux user to override the DSDT. This patch suggests the use of a workaround. Broken BIOS expose the part 0x701 on all links, which does not correspond to a real device or configurations. We can however use this information to make existing codec drivers probe on this device instead. A DMI-based quirk is introduce to avoid using this workaround on existing devices. BugLink: thesofproject#2700 Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

In some platforms, we need to alias the _ADR provided by the DSDT tables to one that is expected and handled by machine drivers. The 'adr_override' field will replace the information from BIOS. BugLink: thesofproject#2700 Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Add alias for HP platform, the DSDT exposes RT701 devices that are not physically present and can be aliased into what RT711 (link0) and RT1308 (link1). Unfortunately we have existing platforms where the workaround appliesd, so at this point we have no choice but limit this workaround with a DMI quirk. BugLink: thesofproject#2700 Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Make sure we use the information needed by the machine driver, after the _ADR override. BugLink: thesofproject#2700 Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

This device has SoundWire support along with DMICs. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

The pinmux allows for 2 SoundWire links to be enabled along with DMICs. This was the default configuration on the TGL-RVP. One issue with this configuration is that we don't have a means to automatically detect how many DMICs are used, which in turn requires the user to manually rename the topology file required on a platform. This was borderline acceptable for Intel RVPs, but now that this configuration is present in HP devices we need to automate the process. This patch makes use of the NHTL information and will pass the DMIC number to the machine driver as a parameter. A follow-up patch will expose the DMIC number to userspace/UCM with the configuration strings. The Google devices do make use of DMICs instead of SoundWire link 2 and 3, but their topology is unique enough that they do not need any NHTL support or topology renaming. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

We've used the machine_quirk field for I2C/I2S-based devices in the past to work-around bad information in the DSDT. Unfortunately we have now bad information in the DSDT for SoundWire devices, and we need to apply the same tricks Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

PR thesofproject/linux#2701 introduces a workaround which leads to errors being thrown when the DSDT contains a device with the 0x701 part_id. Ignore the error since it's a normal error flow. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

UCM needs to know which microphone is used (dmic or RT715-based), let's add the information in the component string. Note the slight change from HDAudio platforms, 'cfg-mics' is used here with the intent that this component string describes either PCH-attached microphones or RT715-attached ones. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

plbossart · 2021-01-26T19:00:17Z

closing, will follow @bardliao's proposal instead for a much simpler workaround solution.

plbossart requested review from RanderWang, bardliao, dbaluta, jack-cy-yu, kv2019i, lgirdwood, oder-chiou, ranj063 and shumingfan as code owners January 19, 2021 17:52

plbossart mentioned this pull request Jan 19, 2021

[BUG] No sound on HP Spectre x360 13-aw2022ur (2020, Tiger Lake) #2700

Closed

plbossart force-pushed the sdw/bios-workaround branch 2 times, most recently from 077416a to 5b85b69 Compare January 19, 2021 19:54

plbossart force-pushed the sdw/bios-workaround branch 2 times, most recently from ba0ae52 to 29a95ab Compare January 20, 2021 02:04

plbossart requested a review from ryans-lee as a code owner January 20, 2021 02:04

RanderWang reviewed Jan 20, 2021

View reviewed changes

Comment thread sound/soc/intel/common/soc-acpi-intel-tgl-match.c Outdated

RanderWang reviewed Jan 20, 2021

View reviewed changes

Comment thread sound/soc/intel/common/soc-acpi-intel-tgl-match.c

RanderWang reviewed Jan 20, 2021

View reviewed changes

Comment thread sound/soc/intel/common/soc-acpi-intel-tgl-match.c

lyakh requested changes Jan 20, 2021

View reviewed changes

Comment thread drivers/soundwire/bus_type.c Outdated

kv2019i reviewed Jan 20, 2021

View reviewed changes

Comment thread drivers/soundwire/bus_type.c

Comment thread sound/soc/codecs/rt711-sdw.c Outdated

Comment thread sound/soc/codecs/rt711-sdw.c Outdated

plbossart force-pushed the sdw/bios-workaround branch 2 times, most recently from 2695534 to e26db3a Compare January 20, 2021 20:10

lyakh previously approved these changes Jan 21, 2021

View reviewed changes

plbossart dismissed lyakh’s stale review via e5c505c January 21, 2021 16:54

plbossart force-pushed the sdw/bios-workaround branch from e26db3a to e5c505c Compare January 21, 2021 16:54

plbossart requested review from keyonjie and libinyang as code owners January 21, 2021 16:54

plbossart mentioned this pull request Jan 21, 2021

ASoC: SOF: Intel: detect DMIC number in SoundWire mixed config #2704

Closed

plbossart added 9 commits January 22, 2021 11:14

ASoC: Intel: sof_sdw: add support for _ADR override

b9ad423

Make sure we use the information needed by the machine driver, after the _ADR override. BugLink: thesofproject#2700 Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

ASoC: Intel: sof_sdw: add quirk for HP Spectre x360

cb037ab

This device has SoundWire support along with DMICs. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

plbossart force-pushed the sdw/bios-workaround branch from c19a33a to 4c8401c Compare January 22, 2021 17:16

plbossart mentioned this pull request Jan 22, 2021

sof-kernel-log-check: ignore ACPI _ADR workaround thesofproject/sof-test#590

Closed

plbossart force-pushed the sdw/bios-workaround branch from 4c8401c to c3c75fc Compare January 25, 2021 16:45

plbossart mentioned this pull request Jan 26, 2021

SoundWire/ HP spectre x360 support #2717

Merged

plbossart closed this Jan 26, 2021

Conversation

plbossart commented Jan 19, 2021

Uh oh!

plbossart commented Jan 19, 2021

Uh oh!

plbossart commented Jan 20, 2021

Uh oh!

RanderWang commented Jan 20, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RanderWang commented Jan 20, 2021

Uh oh!

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

plbossart commented Jan 20, 2021

Uh oh!

RanderWang commented Jan 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bardliao commented Jan 21, 2021

Uh oh!

plbossart commented Jan 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RanderWang commented Jan 21, 2021

Uh oh!

bardliao commented Jan 21, 2021

Uh oh!

aiChaoSONG commented Jan 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RanderWang commented Jan 21, 2021

Uh oh!

lyakh left a comment

Choose a reason for hiding this comment

Uh oh!

plbossart commented Jan 21, 2021

Uh oh!

RanderWang commented Jan 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

plbossart commented Jan 22, 2021

Uh oh!

plbossart commented Jan 26, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

RanderWang commented Jan 21, 2021 •

edited

Loading

plbossart commented Jan 21, 2021 •

edited

Loading

aiChaoSONG commented Jan 21, 2021 •

edited

Loading

RanderWang commented Jan 22, 2021 •

edited

Loading