Re: [alsa-devel] [PATCH v3] ALSA: hda: add Intel DSP configuration / probe code

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

On 10/6/19 10:22 AM, Jaroslav Kysela wrote:
> For distributions, we need one place where we can decide
> which driver will be activated for the auto-configation of the
> Intel's HDA hardware with DSP. Actually, we cover three drivers:
> 
> * Legacy HDA
> * Intel SST
> * Intel Sound Open Firmware (SOF)
> 
> All those drivers registers similar PCI IDs, so the first
> driver probed from the PCI stack can win. But... it is not
> guaranteed that the correct driver wins.
> 
> This commit changes Intel's NHLT ACPI module to a common
> DSP probe module for the Intel's hardware. All above sound
> drivers calls this code. The user can force another behaviour
> using the module parameter 'dsp_driver' located in
> the 'snd-intel-dspcfg' module.
> 
> This change allows to add specific dmi checks for the specific
> systems. The examples are taken from the pull request:
> 
>    https://github.com/thesofproject/linux/pull/927
> 
> Tested on Lenovo Carbon X1 7th gen.

Thanks Jaroslav, I like the ideas in the patch, the flags+DMI table is 
quite elegant.

We will need to do additional checks for the quirks, e.g. I know there 
is a 'Phaser' Chromebook with HDaudio and I don't recall if they use 
DMICs. I also don't know if we always have the NHTL information when the 
legacy BIOS is used.

It's likely that we will have multiple iterations before getting this 
right. And we'll have to add SoundWire support as well (which isn't that 
hard, I already have all the ACPI parsing needed to detect if a link is 
enabled).

Some additional comments below.

> +module_param(dsp_driver, int, 0444);
> +MODULE_PARM_DESC(dsp_driver, "Force the DSP driver for Intel DSP (0=auto, 1=legacy, 2=SST, 3=SOF)");
> +
> +#define FLAG_SST		(1<<0)
> +#define FLAG_SOF		(1<<1)
> +#define FLAG_SOF_ONLY_IF_DMIC	(1<<16)
> +
> +struct config_entry {
> +	u32 flags;
> +	u16 device;
> +	const struct dmi_system_id *dmi_table;
> +};
> +
> +/*
> + * configuration table - the order of entries is important!

It's not really the order but the exclusion between SST and SOF for the 
same PCI ID?

> + */
> +static const struct config_entry config_table[] = {
> +/* Cometlake-LP */
> +#if IS_ENABLED(CONFIG_SND_SOC_INTEL_CML_LP)
> +	{
> +		/* prefer SST */
> +		.flags = FLAG_SST,
> +		.device = 0x02c8,
> +	},
> +#elif IS_ENABLED(CONFIG_SND_SOC_SOF_COMETLAKE_LP)
> +	{
> +		.flags = FLAG_SOF,

need to add FLAG_SOF_ONLY_IF_DMIC

> +		.device = 0x02c8,
> +	},
> +#endif
> +/* Cometlake-H */
> +#if IS_ENABLED(CONFIG_SND_SOC_INTEL_CML_H)
> +	{
> +		.flags = FLAG_SST,
> +		.device = 0x06c8,
> +	},
> +#elif IS_ENABLED(CONFIG_SND_SOC_SOF_COMETLAKE_H)
> +	{
> +		.flags = FLAG_SOF,

| FLAG_SOF_ONLY_IF_DMIC

> +		.device = 0x06c8,
> +	},
> +#endif
> +/* Merrifield */
> +#if IS_ENABLED(CONFIG_SND_SOC_SOF_MERRIFIELD)
> +	{
> +		.flags = FLAG_SOF,
> +		.device = 0x119a,
> +	},
> +#endif
> +/* Broxton-T */
> +#if IS_ENABLED(CONFIG_SND_SOC_SOF_APOLLOLAKE)
> +	{
> +		.flags = FLAG_SOF | FLAG_SOF_ONLY_IF_DMIC,
> +		.device = 0x1a98,
> +	},
> +#endif
> +/* Geminilake */
> +#if IS_ENABLED(CONFIG_SND_SOC_SOF_GEMINILAKE)
> +	{
> +		.flags = FLAG_SOF,

can we have more than one table per PCI ID? e.g. in this case it'd be 
good to have the DMIC case separate from Google.

> +		.device = 0x3198,
> +		.dmi_table = (const struct dmi_system_id []) {
> +			{
> +				.ident = "Google Chromebooks",
> +				.matches = {
> +					DMI_MATCH(DMI_SYS_VENDOR, "Google"),
> +				}
> +			},
> +			{}
> +		}
> +	},
> +#endif
> +#if IS_ENABLED(CONFIG_SND_SOC_INTEL_GLK)

should it be elif, as done for CometLake/CML?

> +	{
> +		.flags = FLAG_SST,
> +		.device = 0x3198,
> +	},
> +#endif
> +/* Icelake */
> +#if IS_ENABLED(CONFIG_SND_SOC_SOF_ICELAKE)
> +	{
> +		.flags = FLAG_SOF | FLAG_SOF_ONLY_IF_DMIC,
> +		.device = 0x34c8,
> +	},
> +#endif
> +/* Elkhart Lake */
> +#if IS_ENABLED(CONFIG_SND_SOC_SOF_ELKHARTLAKE)
> +	{
> +		.flags = FLAG_SOF | FLAG_SOF_ONLY_IF_DMIC,
> +		.device = 0x4b55,
> +	},
> +#endif
> +/* Appololake (Broxton-P) */
> +#if IS_ENABLED(CONFIG_SND_SOC_SOF_APOLLOLAKE)
> +	{
> +		.flags = FLAG_SOF,
> +		.device = 0x5a98,
> +		.dmi_table = (const struct dmi_system_id []) {
> +			{
> +				.ident = "Up Squared",
> +				.matches = {
> +					DMI_MATCH(DMI_SYS_VENDOR, "AAEON"),
> +					DMI_MATCH(DMI_BOARD_NAME, "UP-APL01"),
> +				}
> +			},
> +			{}
> +		}
> +	},
> +#endif
> +#if IS_ENABLED(CONFIG_SND_SOC_INTEL_APL)

elif?

> +	{
> +		.flags = FLAG_SST,
> +		.device = 0x5a98,
> +	},
> +#endif
> +/* Cannonlake */
> +#if IS_ENABLED(CONFIG_SND_SOC_SOF_CANNONLAKE)
> +	{
> +		.flags = FLAG_SOF | FLAG_SOF_ONLY_IF_DMIC,
> +		.device = 0x9dc8,
> +	},
> +#endif
> +/* Sunrise Point-LP */
> +#if IS_ENABLED(CONFIG_SND_SOC_SOF_SKYLAKE)
> +	{
> +		.flags = FLAG_SOF | FLAG_SOF_ONLY_IF_DMIC,
> +		.device = 0x9d70,
> +	},
> +#endif
> +/* Kabylake-LP */
> +#if IS_ENABLED(CONFIG_SND_SOC_SOF_KABYLAKE)
> +	{
> +		.flags = FLAG_SOF | FLAG_SOF_ONLY_IF_DMIC,
> +		.device = 0x9d71,
> +	},
> +#endif

SKL and SKL are not supported by SOF for now, and SST doesn't support 
HDaudio+DMIC combinations

This should be FLAG_SST but only for Google Chromebooks, e.g.


		.flags = FLAG_SST,
		.device = 0x9d70 or 0x9d71
		.dmi_table = (const struct dmi_system_id []) {
			{
				.ident = "Google Chromebooks",
				.matches = {
					DMI_MATCH(DMI_SYS_VENDOR, "Google"),
				}
			},
			{}
		}
	},

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel