Skip to content

Conversation

@plbossart
Copy link
Member

@plbossart plbossart commented Dec 14, 2021

We have to figure out the presence of DMICs and which SSP is used without DMI quirks, otherwise we'll drown in support requests.

Completely untested since I don't have hardware...

@hli25 could you take this for a spin and report what you find on GLK/TGL/JSL devices? There should be dev_dbg messages that you can enable by copying this file
sof-dyndbg.conf.txt to /etc/modprobe.d/sof-dyndbg.conf
Enhancement tracked with #3337

Support requests with active users in
#3336
#3248

@plbossart
Copy link
Member Author

@kv2019i @juimonen this NHLT information could be used for BT offload and figure out which SSP is used if we ever support this for non-Chrome devices designed for Windows.

@yangxiaohua2009
Copy link

@plbossart Here is log glk_nhlt.log on glk.
Currently we define SSP number in the machine driver and use three tplg for SSP0,1,2.
It would be better for the Intel platform driver to read SSP number from NHLT and choose the corresponding tplg file.

@plbossart
Copy link
Member Author

plbossart commented Dec 16, 2021

@plbossart Here is log glk_nhlt.log on glk. Currently we define SSP number in the machine driver and use three tplg for SSP0,1,2. It would be better for the Intel platform driver to read SSP number from NHLT and choose the corresponding tplg file.

Thanks @yangxiaohua2009 indeed that's the plan to autodetect the SSP, but in your example the mask is zero

[ 8.746047] sof-audio-pci-intel-apl 0000:00:0e.0: intel_nhlt_get_dmic_geo: dmic number 0 max_ch 0
[ 8.746054] sof-audio-pci-intel-apl 0000:00:0e.0: NHLT_LINK_SSP detected, ssp_mask 0x0

That makes no sense really, something is wrong here.
Can you please try to trace ssp_mask in intel_nhlt_ssp_endpoint_mask(), I wonder what happens here.

@plbossart
Copy link
Member Author

@yangxiaohua2009 I screwed-up with the initial code, please try the updated branch and let me know what you detect. thanks!

@yangxiaohua2009
Copy link

@plbossart glk_2.log Here's the new log.

@plbossart plbossart force-pushed the fix/sof-es8336-quirk branch from 5e149dc to eef6c90 Compare January 10, 2022 22:12
@plbossart plbossart changed the title [TEST] [DO NOT MERGE] use NHLT information for ES8336 devices ASoC: SOF: Intel: use NHLT information for ES8336 devices Jan 10, 2022
@plbossart plbossart marked this pull request as ready for review January 10, 2022 22:13
@plbossart
Copy link
Member Author

@yangxiaohua2009 @hli25 can you take a look? This should remove the need for the SSP quirk and DMIC quirk. For the GPIO I have no solution.

@yangxiaohua2009
Copy link

@plbossart The kernel works well on glk glk_111.log. However on CML it doesn't select sof in intel-dsp-config.c cml_111.log and cannot recognize GPIO.
After force probing sof and GPIO, I see machine driver still using SSP2 instead of SSP0 on CML.

cml_111_2.log

@plbossart
Copy link
Member Author

Thanks for testing @yangxiaohua2009

@plbossart The kernel works well on glk glk_111.log. However on CML it doesn't select sof in intel-dsp-config.c [cml_111.log]
(https://github.com/thesofproject/linux/files/7844183/cml_111.log) and cannot recognize GPIO.

Sorry, I don't get how the SOF driver is not selected. I see two quirks for CML PCI IDs 0x06c8 and 0x02c8. Can you provide the result of 'cat /sys/bus/pci/devices/0000:00:1f.3/device'

Or did you take the patches and applied them on a different tree that's missing the CML quirks?

After force probing sof and GPIO, I see machine driver still using SSP2 instead of SSP0 on CML.

cml_111_2.log

The logs show SSP0 is used and passed to the machine driver?

[    2.847963] sof-audio-pci-intel-cnl 0000:00:1f.3: NHLT_DEVICE_I2S detected, ssp_mask 0x1

Are you sure you didn't modify the quirk module parameter?

@yangxiaohua2009
Copy link

yangxiaohua2009 commented Jan 12, 2022

Since !dmi_check_system(sof_es8336_quirk_table) always return 1, machine driver still uses SSP2. After removing it and making several changes the machine driver works #24. For SSP2 the i2s_link_mask is 5, for SSP1 i2s_link_mask is 3 and for SSP0 i2s_link_mask is 1 apl_112.log.

@yangxiaohua2009
Copy link

Also, since the dsp driver already know which SSP to use from NHLT, can it select the corresponding tplg file? For example, on GLK I see OEMs using SSP2, SSP1 and SSP0.
For the GPIO, can the machine driver neglect PA control instead of returing error so that at least the user can get mic, headphone and HDMI working. @plbossart

plbossart and others added 20 commits March 3, 2022 13:12
Add missing dmic_num mention and clarify that 'links' mean 'SoundWire
links', not to be used for other links.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
The platform driver may have information on which I2S/TDM link(s) to
enable in the machine driver. In the case of Intel devices, this may
be extracted from NHLT tables in platform firmware. This link
information is necessary to make sure machine driver and topology are
aligned.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
We currently extract the DMIC number only for HDaudio or SoundWire
platforms. For I2S/TDM platforms, this wasn't necessary until now, but
with devices with ES8336 we need to find a solution to detect dmics
more reliably than with a DMI quirk.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
The NHLT information can be used to figure out which SSPs are enabled
in a platform.

The 'SSP' link type is too broad for machine drivers, since it can
cover the Bluetooth sideband and the analog audio codec connections,
so this helper exposes a parameter to filter with the device
type (DEVICE_I2S refers to analog audio codec in NHLT parlance).

The helper returns a mask, since more than one SSP may be used for
analog audio, e.g. the NHLT spec describes the use of SSP0 for
amplifiers and SSP1 for headset codec. Note that if more than one bit
is set, it's impossible to determine which SSP is connected to what
external component. Additional platform-specific information based on
e.g. DMI quirks would still be required in the machine driver to
configure the relevant dailinks.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
For devices designed for Windows, the SSP information should be listed
in the NHLT, and when present can be used to set quirks automatically
in the machine driver.

The NHLT information exposes BT and analog audio connections
separately, for now we are only interested in the analog audio parts.

The use of dev_info() for the SSP mask is intentional so that we can
immediately flag devices with an ES8336 codec. Since NHLT is not used
for recent Chromebooks these messages should be rare.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Different topology filenames may be required depending on which SSP is
used, and whether or not digital mics are present.

This patch adds a tplg_quirk_mask and in the case of the SOF driver
adds the relevant configurations.

This is a short-term solution to the ES8336 support issues.

In a long-term solution, we would need an interface where the machine
driver or platform driver have the ability to alter the topology
hard-coded low-level hardware support, e.g. by substituting an
interface for another, or disabling an interface that is not supported
on a given skew.

BugLink: thesofproject#3248
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
We only saw ESSX8336 so far, but now with reports of 'ESSX8326' we
need to expand to a full list. Let's reuse the 'snd_soc_acpi_codecs'
structure to store the information.

Reported-by: anthony tonitch <d.tonitch@gmail.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
We only saw ESSX8336 so far, but now with reports of 'ESSX8326' we
need to expand to a full list. Let's reuse the 'snd_soc_acpi_codecs'
structure to store the information.

Note that ES8326 will need a dedicated codec driver, but the plan is
to use the same machine driver for all Everest Audio devices.

Reported-by: anthony tonitch <d.tonitch@gmail.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
We're missing this check for the CNL PCI id

Reported-by: Nikolai Kostrigin <nickel@altlinux.org>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Add the support for all ES83x6 devices

Signed-off-by: Nikolai Kostrigin <nickel@altlinux.org>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Do not fail if the GPIO used for speakers is not present, at least the
headphone, headset and internal mics should work.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
…arch

We have an existing 'adev' handle from which we can find the codec
device, no need for an I2C bus search.

This change aligns this driver will all other I2S-based machine
drivers.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
…5 2021"

This reverts commit ce6a70b.

The next patch will add run-time detection of the required SSP and
this hard-coded quirk is not needed.

Acked-by: Mauro Carvalho Chehab <mchehab@kernel.org>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Since we see a proliferation of devices with various configurations,
we want to automatically set the DMIC and SSP information. This patch
relies on the information extracted from NHLT and partially reverts
existing DMI quirks added by commit a164137 ("ASoC: Intel: add
machine driver for SOF+ES8336")

Note that NHLT can report multiple SSPs, choosing from the
ssp_link_mask in an MSB-first manner was found experimentally to work
fine.

The only thing that cannot be detected is the GPIO type, and users may
want to use the quirk override parameter if the 'wrong' solution is
provided.

Tested-by: Mauro Carvalho Chehab <mchehab@kernel.org>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
We only logged the SSP quirk, make sure the GPIO and DMIC quirks are
exposed.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Additional code was added and the comment on the speaker GPIO needs to
be moved before we actually try to get the GPIO.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
The codec driver exposes a set of properties that can be set from the
machine driver - as done in bytcht_es8316.c

Start by adding the JD_INVERTED quirk.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
The ES8326 requires a different codec driver than ES8316/8336, fixup
the codec name and dai name depending on the ACPI _HID exposed in the
DSDT.

Also add the select in Kconfig

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
The presence of DMICs needs to be signaled to UCM, follow the HDaudio
example and use the 'cfg-dmics' component string to report the number
of dmics present on the platform.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Additional code was added and the comment on the speaker GPIO needs to
be moved before we actually try to get the GPIO.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
@plbossart plbossart force-pushed the fix/sof-es8336-quirk branch from 0434208 to 92ba636 Compare March 3, 2022 19:37
@fredoh9
Copy link
Collaborator

fredoh9 commented Mar 3, 2022

SOFCI TEST

@plbossart plbossart requested a review from bardliao March 3, 2022 20:13
@plbossart
Copy link
Member Author

@ujfalusi can I please ask you to take a look, you had initial objections and I don't want to merge without your feedback. thanks!

@plbossart
Copy link
Member Author

https://sof-ci.01.org/linuxpr/PR3338/build7261/devicetest/ had CI issues, let's redo the tests

@plbossart
Copy link
Member Author

SOFCI TEST

Copy link
Collaborator

@ujfalusi ujfalusi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to my eyes.

u8 acpi_hid[ACPI_ID_LEN];
const struct dmi_system_id *dmi_table;
u8 codec_hid[ACPI_ID_LEN];
const struct snd_soc_acpi_codecs *codec_hid;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@plbossart, out of curiosity, what 'hid' stands for? Can this be simply named as 'codecs'?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's an ACPI acronym for 'Hardware ID'. There's also 'Compatible ID'.
this is represented in DSDT, e.g.

Device (ESSX)
                {
                    Name (_ADR, Zero)  // _ADR: Address
                    Name (_HID, "ESSX8336")  // _HID: Hardware ID
                    Name (_CID, "ESSX8336")  // _CID: Compatible ID
                    Name (_DDN, "ES8336")  // _DDN: DOS Device Name

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.