[RFC PATCH 0/6] ASoC:Intel:Skylake: Enable HDaudio legacy fallback

[RFC PATCH 0/6] ASoC:Intel:Skylake: Enable HDaudio legacy fallback

[Pierre-Louis Bossart]

This patchset is provided as an RFC and should not be merged as is
(Turkey break in the USA and more validation needed). This is however
a good time to gather comments. This work is the result of multiple
email discussions to finally provide more flexibility for
distributions to chose whether to stick with the legacy HDaudio driver
or to enable the SST/Skylake driver for newer platforms (required
e.g. for digital microphone support)

The patches add support for CoffeeLake, simplify the probe for the
Skylake driver, introduce more compile-time granularity so that
platforms can be selected individually and last provide a dynamic
fallback mechanism when two drivers are registered for the same PCI
ID.

When the SOF driver is released, the same mechanism will be used to
enable the SOF-legacy fallback. There will be no plans to enable an
SOF->SST falback.

Pierre-Louis Bossart (4):
  ASoC: Intel: Skylake: stop init/probe if DSP is not present
  ASoC: Intel: Skylake: remove useless tests on DSP presence
  ASoC: Intel: Skylake: Add more platform granularity
  ALSA: hda: add fallback capabilities for SKL+ platforms

Takashi Iwai (2):
  ASoC: Intel: Skylake: Add CFL-S support
  ALSA: hda: Allow fallback binding with legacy HD-audio for Intel SKL+

 include/sound/hdaudio.h                |   6 +
 sound/pci/hda/Kconfig                  |  40 +++++++
 sound/pci/hda/hda_controller.h         |   2 +-
 sound/pci/hda/hda_intel.c              |  51 ++++++--
 sound/soc/intel/Kconfig                |  86 ++++++++++++--
 sound/soc/intel/boards/Kconfig         |  16 ++-
 sound/soc/intel/skylake/skl-messages.c |   8 ++
 sound/soc/intel/skylake/skl.c          | 154 +++++++++++++++++++------
 8 files changed, 311 insertions(+), 52 deletions(-)

-- 
2.17.1

[RFC PATCH 1/6] ASoC: Intel: Skylake: Add CFL-S support

[Pierre-Louis Bossart]

From: Takashi Iwai <tiwai@suse.de>

It's with CNP, supposed to be equivalent with CNL entry.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/intel/skylake/skl-messages.c | 8 ++++++++
 sound/soc/intel/skylake/skl.c          | 3 +++
 2 files changed, 11 insertions(+)

diff --git a/sound/soc/intel/skylake/skl-messages.c b/sound/soc/intel/skylake/skl-messages.c
index 8bfb8b0fa3d5..b0e6fb93eaf8 100644
--- a/sound/soc/intel/skylake/skl-messages.c
+++ b/sound/soc/intel/skylake/skl-messages.c
@@ -247,6 +247,14 @@ static const struct skl_dsp_ops dsp_ops[] = {
 		.init_fw = cnl_sst_init_fw,
 		.cleanup = cnl_sst_dsp_cleanup
 	},
+	{
+		.id = 0xa348,
+		.num_cores = 4,
+		.loader_ops = bxt_get_loader_ops,
+		.init = cnl_sst_dsp_init,
+		.init_fw = cnl_sst_init_fw,
+		.cleanup = cnl_sst_dsp_cleanup
+	},
 };
 
 const struct skl_dsp_ops *skl_get_dsp_ops(int pci_id)
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
index 3f0ac1312982..df36b8fe6d5e 100644
--- a/sound/soc/intel/skylake/skl.c
+++ b/sound/soc/intel/skylake/skl.c
@@ -1121,6 +1121,9 @@ static const struct pci_device_id skl_ids[] = {
 	/* CNL */
 	{ PCI_DEVICE(0x8086, 0x9dc8),
 		.driver_data = (unsigned long)&snd_soc_acpi_intel_cnl_machines},
+	/* CFL */
+	{ PCI_DEVICE(0x8086, 0xa348),
+		.driver_data = (unsigned long)&snd_soc_acpi_intel_cnl_machines},
 	{ 0, }
 };
 MODULE_DEVICE_TABLE(pci, skl_ids);
-- 
2.17.1

[RFC PATCH 2/6] ASoC: Intel: Skylake: stop init/probe if DSP is not present

[Pierre-Louis Bossart]

Check immediately if the DSP can be found, bail and avoid doing inits
to enable legacy fallback without delay.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/intel/skylake/skl.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
index df36b8fe6d5e..1d7146773d19 100644
--- a/sound/soc/intel/skylake/skl.c
+++ b/sound/soc/intel/skylake/skl.c
@@ -931,6 +931,12 @@ static int skl_first_init(struct hdac_bus *bus)
 
 	snd_hdac_bus_parse_capabilities(bus);
 
+	/* check if dsp is there */
+	if (!bus->ppcap) {
+		dev_err(bus->dev, "bus ppcap not set, DSP not present?\n");
+		return -ENODEV;
+	}
+
 	if (skl_acquire_irq(bus, 0) < 0)
 		return -EBUSY;
 
@@ -940,23 +946,25 @@ static int skl_first_init(struct hdac_bus *bus)
 	gcap = snd_hdac_chip_readw(bus, GCAP);
 	dev_dbg(bus->dev, "chipset global capabilities = 0x%x\n", gcap);
 
-	/* allow 64bit DMA address if supported by H/W */
-	if (!dma_set_mask(bus->dev, DMA_BIT_MASK(64))) {
-		dma_set_coherent_mask(bus->dev, DMA_BIT_MASK(64));
-	} else {
-		dma_set_mask(bus->dev, DMA_BIT_MASK(32));
-		dma_set_coherent_mask(bus->dev, DMA_BIT_MASK(32));
-	}
-
 	/* read number of streams from GCAP register */
 	cp_streams = (gcap >> 8) & 0x0f;
 	pb_streams = (gcap >> 12) & 0x0f;
 
-	if (!pb_streams && !cp_streams)
+	if (!pb_streams && !cp_streams) {
+		dev_err(bus->dev, "no streams found in GCAP definitions?\n");
 		return -EIO;
+	}
 
 	bus->num_streams = cp_streams + pb_streams;
 
+	/* allow 64bit DMA address if supported by H/W */
+	if (!dma_set_mask(bus->dev, DMA_BIT_MASK(64))) {
+		dma_set_coherent_mask(bus->dev, DMA_BIT_MASK(64));
+	} else {
+		dma_set_mask(bus->dev, DMA_BIT_MASK(32));
+		dma_set_coherent_mask(bus->dev, DMA_BIT_MASK(32));
+	}
+
 	/* initialize streams */
 	snd_hdac_ext_stream_init_all
 		(bus, 0, cp_streams, SNDRV_PCM_STREAM_CAPTURE);
-- 
2.17.1

[RFC PATCH 3/6] ASoC: Intel: Skylake: remove useless tests on DSP presence

[Pierre-Louis Bossart]

bus->ppcap is now tested upfront, there is no need to re-check if the
DSP is present. Remove tests and remove indentation.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/intel/skylake/skl.c | 40 ++++++++++++++++-------------------
 1 file changed, 18 insertions(+), 22 deletions(-)

diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
index 1d7146773d19..0bdb1f7fdd4a 100644
--- a/sound/soc/intel/skylake/skl.c
+++ b/sound/soc/intel/skylake/skl.c
@@ -826,12 +826,10 @@ static void skl_probe_work(struct work_struct *work)
 		return;
 	}
 
-	if (bus->ppcap) {
-		err = skl_machine_device_register(skl);
-		if (err < 0) {
-			dev_err(bus->dev, "machine register failed: %d\n", err);
-			goto out_err;
-		}
+	err = skl_machine_device_register(skl);
+	if (err < 0) {
+		dev_err(bus->dev, "machine register failed: %d\n", err);
+		goto out_err;
 	}
 
 	/*
@@ -1019,25 +1017,23 @@ static int skl_probe(struct pci_dev *pci,
 
 	pci_set_drvdata(skl->pci, bus);
 
-	/* check if dsp is there */
-	if (bus->ppcap) {
-		/* create device for dsp clk */
-		err = skl_clock_device_register(skl);
-		if (err < 0)
-			goto out_clk_free;
+	/* create device for dsp clk */
+	err = skl_clock_device_register(skl);
+	if (err < 0)
+		goto out_clk_free;
 
-		err = skl_find_machine(skl, (void *)pci_id->driver_data);
-		if (err < 0)
-			goto out_nhlt_free;
+	err = skl_find_machine(skl, (void *)pci_id->driver_data);
+	if (err < 0)
+		goto out_nhlt_free;
 
-		err = skl_init_dsp(skl);
-		if (err < 0) {
-			dev_dbg(bus->dev, "error failed to register dsp\n");
-			goto out_nhlt_free;
-		}
-		skl->skl_sst->enable_miscbdcge = skl_enable_miscbdcge;
-		skl->skl_sst->clock_power_gating = skl_clock_power_gating;
+	err = skl_init_dsp(skl);
+	if (err < 0) {
+		dev_dbg(bus->dev, "error failed to register dsp\n");
+		goto out_nhlt_free;
 	}
+	skl->skl_sst->enable_miscbdcge = skl_enable_miscbdcge;
+	skl->skl_sst->clock_power_gating = skl_clock_power_gating;
+
 	if (bus->mlcap)
 		snd_hdac_ext_bus_get_ml_capabilities(bus);
 
-- 
2.17.1

[RFC PATCH 4/6] ASoC: Intel: Skylake: Add more platform granularity

[Pierre-Louis Bossart]

The current SKYLAKE kconfig is a all-you-can-eat selection that will
support all known plaforms. This is however not necessarily a good
thing: most platforms for SKL and KBL don't support the DSP, but a
number of CNL/WHL ones do. Selecting this driver in all cases isn't
really smart and will require users to muck with blacklists.

Partition the configs to allow distributions to select on which
platform this driver is used. Keep the existing SND_SOC_INTEL_SKYLAKE
config to select everything for backwards compatibility. This patch does
not provide new functionality, only finer-grained choices in supported
platforms.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/intel/Kconfig        | 73 ++++++++++++++++++++++++++++++----
 sound/soc/intel/boards/Kconfig | 16 +++++++-
 sound/soc/intel/skylake/skl.c  | 12 ++++++
 3 files changed, 92 insertions(+), 9 deletions(-)

diff --git a/sound/soc/intel/Kconfig b/sound/soc/intel/Kconfig
index 18e717703685..99a62ba409df 100644
--- a/sound/soc/intel/Kconfig
+++ b/sound/soc/intel/Kconfig
@@ -102,15 +102,74 @@ config SND_SST_ATOM_HIFI2_PLATFORM_ACPI
 	  recommended option
 
 config SND_SOC_INTEL_SKYLAKE
-	tristate "SKL/BXT/KBL/GLK/CNL... Platforms"
+	tristate "All Skylake/SST Platforms"
 	depends on PCI && ACPI
-	select SND_SOC_INTEL_SKYLAKE_COMMON
+	select SND_SOC_INTEL_SKL
+	select SND_SOC_INTEL_APL
+	select SND_SOC_INTEL_KBL
+	select SND_SOC_INTEL_GLK
+	select SND_SOC_INTEL_CNL
+	select SND_SOC_INTEL_CFL
 	help
-	  If you have a Intel Skylake/Broxton/ApolloLake/KabyLake/
-	  GeminiLake or CannonLake platform with the DSP enabled in the BIOS
-	  then enable this option by saying Y or m.
+          This is a backwards-compatible option to select all devices
+	  supported by the Intel SST/Skylake driver. This option is no
+	  longer recommended and will be deprecated when the SOF
+	  driver is introduced.  Distributions should explicitly
+	  select which platform uses this driver.
+
+config SND_SOC_INTEL_SKL
+	tristate "Skylake Platforms"
+	depends on PCI && ACPI
+	select SND_SOC_INTEL_SKYLAKE_FAMILY
+	help
+	  If you have a Intel Skylake platform with the DSP enabled
+	  in the BIOS then enable this option by saying Y or m.
+
+config SND_SOC_INTEL_APL
+	tristate "Broxton/ApolloLake Platforms"
+	depends on PCI && ACPI
+	select SND_SOC_INTEL_SKYLAKE_FAMILY
+	help
+	  If you have a Intel Broxton/ApolloLake platform with the DSP
+	  enabled in the BIOS then enable this option by saying Y or m.
+
+config SND_SOC_INTEL_KBL
+	tristate "Kabylake Platforms"
+	depends on PCI && ACPI
+	select SND_SOC_INTEL_SKYLAKE_FAMILY
+	help
+	  If you have a Intel Kabylake platform with the DSP
+	  enabled in the BIOS then enable this option by saying Y or m.
+
+config SND_SOC_INTEL_GLK
+	tristate "GeminiLake Platforms"
+	depends on PCI && ACPI
+	select SND_SOC_INTEL_SKYLAKE_FAMILY
+	help
+	  If you have a Intel GeminiLake platform with the DSP
+	  enabled in the BIOS then enable this option by saying Y or m.
+
+config SND_SOC_INTEL_CNL
+	tristate "CannonLake/WhiskyLake Platforms"
+	depends on PCI && ACPI
+	select SND_SOC_INTEL_SKYLAKE_FAMILY
+	help
+	  If you have a Intel CNL/WHL platform with the DSP
+	  enabled in the BIOS then enable this option by saying Y or m.
+
+config SND_SOC_INTEL_CFL
+	tristate "CoffeeLake Platforms"
+	depends on PCI && ACPI
+	select SND_SOC_INTEL_SKYLAKE_FAMILY
+	help
+	  If you have a Intel CoffeeLake platform with the DSP
+	  enabled in the BIOS then enable this option by saying Y or m.
+
+config SND_SOC_INTEL_SKYLAKE_FAMILY
+	tristate
+	select SND_SOC_INTEL_SKYLAKE_COMMON
 
-if  SND_SOC_INTEL_SKYLAKE
+if SND_SOC_INTEL_SKYLAKE_FAMILY
 
 config SND_SOC_INTEL_SKYLAKE_SSP_CLK
 	tristate
@@ -135,7 +194,7 @@ config SND_SOC_INTEL_SKYLAKE_COMMON
 	  GeminiLake or CannonLake platform with the DSP enabled in the BIOS
 	  then enable this option by saying Y or m.
 
-endif ## SND_SOC_INTEL_SKYLAKE
+endif ## SND_SOC_INTEL_SKYLAKE_FAMILY
 
 config SND_SOC_ACPI_INTEL_MATCH
 	tristate
diff --git a/sound/soc/intel/boards/Kconfig b/sound/soc/intel/boards/Kconfig
index b177db2a0dbb..980bccaeb7aa 100644
--- a/sound/soc/intel/boards/Kconfig
+++ b/sound/soc/intel/boards/Kconfig
@@ -172,7 +172,7 @@ config SND_SOC_INTEL_BYT_CHT_NOCODEC_MACH
 
 endif ## SND_SST_ATOM_HIFI2_PLATFORM
 
-if SND_SOC_INTEL_SKYLAKE
+if SND_SOC_INTEL_SKL
 
 config SND_SOC_INTEL_SKL_RT286_MACH
 	tristate "SKL with RT286 I2S mode"
@@ -212,6 +212,10 @@ config SND_SOC_INTEL_SKL_NAU88L25_MAX98357A_MACH
 	  Say Y or m if you have such a device. This is a recommended option.
 	  If unsure select "N".
 
+endif ## SND_SOC_INTEL_SKL
+
+if SND_SOC_INTEL_APL
+
 config SND_SOC_INTEL_BXT_DA7219_MAX98357A_MACH
 	tristate "Broxton with DA7219 and MAX98357A in I2S Mode"
 	depends on MFD_INTEL_LPSS && I2C && ACPI
@@ -239,6 +243,10 @@ config SND_SOC_INTEL_BXT_RT298_MACH
 	   Say Y or m if you have such a device. This is a recommended option.
 	   If unsure select "N".
 
+endif ## SND_SOC_INTEL_APL
+
+if SND_SOC_INTEL_KBL
+
 config SND_SOC_INTEL_KBL_RT5663_MAX98927_MACH
 	tristate "KBL with RT5663 and MAX98927 in I2S Mode"
 	depends on MFD_INTEL_LPSS && I2C && ACPI
@@ -293,6 +301,10 @@ config SND_SOC_INTEL_KBL_DA7219_MAX98927_MACH
 	  Say Y if you have such a device.
 	  If unsure select "N".
 
+endif ## SND_SOC_INTEL_KBL
+
+if SND_SOC_INTEL_GLK
+
 config SND_SOC_INTEL_GLK_RT5682_MAX98357A_MACH
 	tristate "GLK with RT5682 and MAX98357A in I2S Mode"
 	depends on MFD_INTEL_LPSS && I2C && ACPI
@@ -307,7 +319,7 @@ config SND_SOC_INTEL_GLK_RT5682_MAX98357A_MACH
 	   Say Y if you have such a device.
 	   If unsure select "N".
 
-endif ## SND_SOC_INTEL_SKYLAKE
+endif ## SND_SOC_INTEL_GLK
 
 if SND_SOC_INTEL_SKYLAKE_HDAUDIO_CODEC
 
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
index 0bdb1f7fdd4a..73f82532770e 100644
--- a/sound/soc/intel/skylake/skl.c
+++ b/sound/soc/intel/skylake/skl.c
@@ -1110,24 +1110,36 @@ static void skl_remove(struct pci_dev *pci)
 
 /* PCI IDs */
 static const struct pci_device_id skl_ids[] = {
+#if IS_ENABLED(CONFIG_SND_SOC_INTEL_SKL)
 	/* Sunrise Point-LP */
 	{ PCI_DEVICE(0x8086, 0x9d70),
 		.driver_data = (unsigned long)&snd_soc_acpi_intel_skl_machines},
+#endif
+#if IS_ENABLED(CONFIG_SND_SOC_INTEL_APL)
 	/* BXT-P */
 	{ PCI_DEVICE(0x8086, 0x5a98),
 		.driver_data = (unsigned long)&snd_soc_acpi_intel_bxt_machines},
+#endif
+#if IS_ENABLED(CONFIG_SND_SOC_INTEL_KBL)
 	/* KBL */
 	{ PCI_DEVICE(0x8086, 0x9D71),
 		.driver_data = (unsigned long)&snd_soc_acpi_intel_kbl_machines},
+#endif
+#if IS_ENABLED(CONFIG_SND_SOC_INTEL_GLK)
 	/* GLK */
 	{ PCI_DEVICE(0x8086, 0x3198),
 		.driver_data = (unsigned long)&snd_soc_acpi_intel_glk_machines},
+#endif
+#if IS_ENABLED(CONFIG_SND_SOC_INTEL_CNL)
 	/* CNL */
 	{ PCI_DEVICE(0x8086, 0x9dc8),
 		.driver_data = (unsigned long)&snd_soc_acpi_intel_cnl_machines},
+#endif
+#if IS_ENABLED(CONFIG_SND_SOC_INTEL_CFL)
 	/* CFL */
 	{ PCI_DEVICE(0x8086, 0xa348),
 		.driver_data = (unsigned long)&snd_soc_acpi_intel_cnl_machines},
+#endif
 	{ 0, }
 };
 MODULE_DEVICE_TABLE(pci, skl_ids);
-- 
2.17.1

[RFC PATCH 5/6] ALSA: hda: Allow fallback binding with legacy HD-audio for Intel SKL+

[Pierre-Louis Bossart]

From: Takashi Iwai <tiwai@suse.de>

Previous patches enabled distributions to select for which platform
the Skylake driver will be used. This is not enough however since the
DSP may or may not enabled by the BIOS, and a probe-time dynamic
fallback is highly desired.

This patch introduces a new AZX_DCAPS value, and when this field is
set for the relevant PCI ID the HDaudio legacy driver will stop its
probe. Conversely the Skylake driver will attempt to probe, but in
case of errors (typically DSP not present or no HDAudio streams found)
it will fall back to the HDaudio legacy.

The behavior can be influenced by static Kconfig definitions to enable
or disable this fallback. A 'legacy' parameter also controls the
fallback, allowing testers to prevent the Skylake driver from probing
or conversely preventing the fallback from happening.

This patch only introduces the fallback capabilities and handling, the
platform definitions are provided in a follow-up patch.

Errors beyond the initial DSP capability parsing (e.g. missing
firmware file, firmware authentication issue, missing topology file,
bad topology configuration, etc) are considered out-of-scope. It is
nearly impossible to handle all possible error cases and most errors
are only detected after a timeout which isn't compatible with usual
probe expectations. It is expected that additional debug hooks will
have to be provided at a later point, e.g. module parameters to
override hard-coded firmware name and topology files.

Credits to Takashi for the initial code shared on the alsa-devel
mailing list. Testing with base-defconfig, sst-defconfig and
hdaudio-defconfigs provided at
https://github.com/thesofproject/kconfig

Signed-off-by: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 include/sound/hdaudio.h        |  6 +++
 sound/pci/hda/hda_controller.h |  2 +-
 sound/pci/hda/hda_intel.c      | 32 ++++++++++++++-
 sound/soc/intel/Kconfig        |  7 ++++
 sound/soc/intel/skylake/skl.c  | 73 ++++++++++++++++++++++++++++++++--
 5 files changed, 114 insertions(+), 6 deletions(-)

diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
index cd1773d0e08f..2ed67f315962 100644
--- a/include/sound/hdaudio.h
+++ b/include/sound/hdaudio.h
@@ -8,6 +8,7 @@
 
 #include <linux/device.h>
 #include <linux/interrupt.h>
+#include <linux/pci.h>
 #include <linux/pm_runtime.h>
 #include <linux/timecounter.h>
 #include <sound/core.h>
@@ -634,4 +635,9 @@ static inline unsigned int snd_array_index(struct snd_array *array, void *ptr)
 	for ((idx) = 0, (ptr) = (array)->list; (idx) < (array)->used; \
 	     (ptr) = snd_array_elem(array, ++(idx)))
 
+/* shared resource with ASoC and legacy HD-audio drivers */
+#ifdef CONFIG_SND_SOC_INTEL_SKL_LEGACY_SUPPORT
+const struct pci_driver *snd_hda_intel_probe(struct pci_dev *pci);
+#endif
+
 #endif /* __SOUND_HDAUDIO_H */
diff --git a/sound/pci/hda/hda_controller.h b/sound/pci/hda/hda_controller.h
index c95097bb5a0c..2351115f922e 100644
--- a/sound/pci/hda/hda_controller.h
+++ b/sound/pci/hda/hda_controller.h
@@ -37,7 +37,7 @@
 #else
 #define AZX_DCAPS_I915_COMPONENT 0		/* NOP */
 #endif
-/* 14 unused */
+#define AZX_DCAPS_INTEL_SHARED	(1 << 14)	/* shared with ASoC */
 #define AZX_DCAPS_CTX_WORKAROUND (1 << 15)	/* X-Fi workaround */
 #define AZX_DCAPS_POSFIX_LPIB	(1 << 16)	/* Use LPIB as default */
 /* 17 unused */
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index d8eb2b5f51ae..eb00e37c1c27 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -2081,8 +2081,8 @@ static const struct hda_controller_ops pci_hda_ops = {
 	.link_power = azx_intel_link_power,
 };
 
-static int azx_probe(struct pci_dev *pci,
-		     const struct pci_device_id *pci_id)
+static int __azx_probe(struct pci_dev *pci, const struct pci_device_id *pci_id,
+		       bool skip_shared)
 {
 	static int dev;
 	struct snd_card *card;
@@ -2091,6 +2091,10 @@ static int azx_probe(struct pci_dev *pci,
 	bool schedule_probe;
 	int err;
 
+	/* skip the entry if it's shared with ASoC */
+	if (skip_shared && (pci_id->driver_data & AZX_DCAPS_INTEL_SHARED))
+		return -ENODEV;
+
 	if (dev >= SNDRV_CARDS)
 		return -ENODEV;
 	if (!enable[dev]) {
@@ -2158,6 +2162,12 @@ static int azx_probe(struct pci_dev *pci,
 	return err;
 }
 
+static int azx_probe(struct pci_dev *pci, const struct pci_device_id *pci_id)
+{
+	return __azx_probe(pci, pci_id,
+			   IS_ENABLED(CONFIG_SND_SOC_INTEL_SKL_LEGACY_SUPPORT));
+}
+
 #ifdef CONFIG_PM
 /* On some boards setting power_save to a non 0 value leads to clicking /
  * popping sounds when ever we enter/leave powersaving mode. Ideally we would
@@ -2650,4 +2660,22 @@ static struct pci_driver azx_driver = {
 	},
 };
 
+#ifdef CONFIG_SND_SOC_INTEL_SKL_LEGACY_SUPPORT
+const struct pci_driver *
+snd_hda_intel_probe(struct pci_dev *pci)
+{
+	const struct pci_device_id *pci_id;
+	int ret;
+
+	pci_id = pci_match_id(azx_ids, pci);
+	if (!pci_id)
+		return ERR_PTR(-ENODEV);
+	ret = __azx_probe(pci, pci_id, false);
+	if (ret < 0)
+		return ERR_PTR(ret);
+	return &azx_driver;
+}
+EXPORT_SYMBOL_GPL(snd_hda_intel_probe);
+#endif
+
 module_pci_driver(azx_driver);
diff --git a/sound/soc/intel/Kconfig b/sound/soc/intel/Kconfig
index 99a62ba409df..c02d08d31d0d 100644
--- a/sound/soc/intel/Kconfig
+++ b/sound/soc/intel/Kconfig
@@ -194,6 +194,13 @@ config SND_SOC_INTEL_SKYLAKE_COMMON
 	  GeminiLake or CannonLake platform with the DSP enabled in the BIOS
 	  then enable this option by saying Y or m.
 
+config SND_SOC_INTEL_SKL_LEGACY_SUPPORT
+	bool "Fallback legacy HD-audio binding"
+	depends on SND_HDA_INTEL=y || SND_SOC_INTEL_SKYLAKE_FAMILY=SND_HDA_INTEL
+	help
+	  Fallback binding with the legacy HD-audio driver when no DSP is
+	  found
+
 endif ## SND_SOC_INTEL_SKYLAKE_FAMILY
 
 config SND_SOC_ACPI_INTEL_MATCH
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
index 73f82532770e..b67cb54d382c 100644
--- a/sound/soc/intel/skylake/skl.c
+++ b/sound/soc/intel/skylake/skl.c
@@ -41,6 +41,21 @@
 #include "../../../soc/codecs/hdac_hda.h"
 #endif
 
+enum {
+	SKL_BIND_AUTO,		/* fallback to legacy driver */
+	SKL_BIND_LEGACY,	/* bind only with legacy driver */
+	SKL_BIND_ASOC		/* bind only with ASoC driver */
+};
+
+#ifdef CONFIG_SND_SOC_INTEL_SKL_LEGACY_SUPPORT
+static const struct pci_driver *skl_fallback_driver;
+#define FALLBACK_PM_CALL(dev, method)					\
+	do {								\
+		if (skl_fallback_driver &&				\
+		    skl_fallback_driver->driver.pm->method)		\
+			return skl_fallback_driver->driver.pm->method(dev); \
+	} while (0)
+
 /*
  * initialize the PCI registers
  */
@@ -264,6 +279,9 @@ static int skl_suspend_late(struct device *dev)
 	struct hdac_bus *bus = pci_get_drvdata(pci);
 	struct skl *skl = bus_to_skl(bus);
 
+	if (skl_fallback_driver)
+		return 0;
+
 	return skl_suspend_late_dsp(skl);
 }
 
@@ -313,6 +331,8 @@ static int skl_suspend(struct device *dev)
 	struct skl *skl  = bus_to_skl(bus);
 	int ret = 0;
 
+	FALLBACK_PM_CALL(dev, suspend);
+
 	/*
 	 * Do not suspend if streams which are marked ignore suspend are
 	 * running, we need to save the state for these and continue
@@ -351,6 +371,8 @@ static int skl_resume(struct device *dev)
 	struct hdac_ext_link *hlink = NULL;
 	int ret;
 
+	FALLBACK_PM_CALL(dev, resume);
+
 	/* Turned OFF in HDMI codec driver after codec reconfiguration */
 	if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI)) {
 		ret = snd_hdac_display_power(bus, true);
@@ -405,6 +427,8 @@ static int skl_runtime_suspend(struct device *dev)
 	struct pci_dev *pci = to_pci_dev(dev);
 	struct hdac_bus *bus = pci_get_drvdata(pci);
 
+	FALLBACK_PM_CALL(dev, runtime_suspend);
+
 	dev_dbg(bus->dev, "in %s\n", __func__);
 
 	return _skl_suspend(bus);
@@ -415,15 +439,24 @@ static int skl_runtime_resume(struct device *dev)
 	struct pci_dev *pci = to_pci_dev(dev);
 	struct hdac_bus *bus = pci_get_drvdata(pci);
 
+	FALLBACK_PM_CALL(dev, runtime_resume);
+
 	dev_dbg(bus->dev, "in %s\n", __func__);
 
 	return _skl_resume(bus);
 }
+
+static int skl_runtime_idle(struct device *dev)
+{
+	FALLBACK_PM_CALL(dev, runtime_idle);
+	return 0;
+}
 #endif /* CONFIG_PM */
 
 static const struct dev_pm_ops skl_pm = {
 	SET_SYSTEM_SLEEP_PM_OPS(skl_suspend, skl_resume)
-	SET_RUNTIME_PM_OPS(skl_runtime_suspend, skl_runtime_resume, NULL)
+	SET_RUNTIME_PM_OPS(skl_runtime_suspend, skl_runtime_resume,
+			   skl_runtime_idle)
 	.suspend_late = skl_suspend_late,
 };
 
@@ -980,8 +1013,8 @@ static int skl_first_init(struct hdac_bus *bus)
 	return skl_init_chip(bus, true);
 }
 
-static int skl_probe(struct pci_dev *pci,
-		     const struct pci_device_id *pci_id)
+static int __skl_probe(struct pci_dev *pci,
+		       const struct pci_device_id *pci_id)
 {
 	struct skl *skl;
 	struct hdac_bus *bus = NULL;
@@ -1060,6 +1093,25 @@ static int skl_probe(struct pci_dev *pci,
 	return err;
 }
 
+static int skl_probe(struct pci_dev *pci,
+		     const struct pci_device_id *pci_id)
+{
+	int ret = -ENODEV;
+
+	if (skl_legacy_binding != SKL_BIND_LEGACY)
+		ret = __skl_probe(pci, pci_id);
+
+#ifdef CONFIG_SND_SOC_INTEL_SKL_LEGACY_SUPPORT
+	if (ret < 0 && skl_legacy_binding != SKL_BIND_ASOC) {
+		skl_fallback_driver = snd_hda_intel_probe(pci);
+		ret = PTR_ERR_OR_ZERO(skl_fallback_driver);
+		if (ret)
+			skl_fallback_driver = NULL;
+	}
+#endif
+	return ret;
+}
+
 static void skl_shutdown(struct pci_dev *pci)
 {
 	struct hdac_bus *bus = pci_get_drvdata(pci);
@@ -1067,6 +1119,13 @@ static void skl_shutdown(struct pci_dev *pci)
 	struct hdac_ext_stream *stream;
 	struct skl *skl;
 
+#ifdef CONFIG_SND_SOC_INTEL_SKL_LEGACY_SUPPORT
+	if (skl_fallback_driver) {
+		skl_fallback_driver->shutdown(pci);
+		return;
+	}
+#endif
+
 	if (!bus)
 		return;
 
@@ -1089,6 +1148,14 @@ static void skl_remove(struct pci_dev *pci)
 	struct hdac_bus *bus = pci_get_drvdata(pci);
 	struct skl *skl = bus_to_skl(bus);
 
+#ifdef CONFIG_SND_SOC_INTEL_SKL_LEGACY_SUPPORT
+	if (skl_fallback_driver) {
+		skl_fallback_driver->remove(pci);
+		skl_fallback_driver = NULL;
+		return;
+	}
+#endif
+
 	release_firmware(skl->tplg);
 
 	pm_runtime_get_noresume(&pci->dev);
-- 
2.17.1

[RFC PATCH 6/6] ALSA: hda: add fallback capabilities for SKL+ platforms

[Pierre-Louis Bossart]

Enable fallback for select PCI IDs

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/pci/hda/Kconfig     | 40 +++++++++++++++++++++++++++++++++++++++
 sound/pci/hda/hda_intel.c | 19 +++++++++++++------
 sound/soc/intel/Kconfig   |  6 ++++++
 3 files changed, 59 insertions(+), 6 deletions(-)

diff --git a/sound/pci/hda/Kconfig b/sound/pci/hda/Kconfig
index 4235907b7858..9bb317fb3507 100644
--- a/sound/pci/hda/Kconfig
+++ b/sound/pci/hda/Kconfig
@@ -228,4 +228,44 @@ config SND_HDA_POWER_SAVE_DEFAULT
 
 endif
 
+if SND_HDA_INTEL
+
+config SND_HDA_INTEL_LEGACY_FALLBACK_SKL
+	bool
+	help
+	  This option enables HD-audio legacy fallback for
+	  Skylake machines
+
+config SND_HDA_INTEL_LEGACY_FALLBACK_APL
+	bool
+	help
+	  This option enables HD-audio legacy fallback for
+	  Broxton/ApolloLake machines
+
+config SND_HDA_INTEL_LEGACY_FALLBACK_KBL
+	bool
+	help
+	  This option enables HD-audio legacy fallback for
+	  KabyLake machines
+
+config SND_HDA_INTEL_LEGACY_FALLBACK_GLK
+	bool
+	help
+	  This option enables HD-audio legacy fallback for
+	  GeminiLake machines
+
+config SND_HDA_INTEL_LEGACY_FALLBACK_CNL
+	bool
+	help
+	  This option enables HD-audio legacy fallback for
+	  CannonLake machines
+
+config SND_HDA_INTEL_LEGACY_FALLBACK_CFL
+	bool
+	help
+	  This option enables HD-audio legacy fallback for
+	  CoffeeLake machines
+
+endif ## SND_HDA_INTEL
+
 endmenu
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index eb00e37c1c27..569419242da3 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -360,6 +360,7 @@ enum {
 	 AZX_DCAPS_NO_64BIT |\
 	 AZX_DCAPS_4K_BDLE_BOUNDARY | AZX_DCAPS_SNOOP_OFF)
 
+#define AZX_DCAPS_INTEL_LEGACY_FALLBACK(conf) (IS_ENABLED(conf) ? AZX_DCAPS_INTEL_SHARED : 0)
 /*
  * vga_switcheroo support
  */
@@ -2416,34 +2417,40 @@ static const struct pci_device_id azx_ids[] = {
 	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE },
 	/* Sunrise Point-LP */
 	{ PCI_DEVICE(0x8086, 0x9d70),
-	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE },
+	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE |
+	  AZX_DCAPS_INTEL_LEGACY_FALLBACK(CONFIG_SND_HDA_INTEL_LEGACY_FALLBACK_SKL) },
 	/* Kabylake */
 	{ PCI_DEVICE(0x8086, 0xa171),
 	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE },
 	/* Kabylake-LP */
 	{ PCI_DEVICE(0x8086, 0x9d71),
-	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE },
+	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE |
+	  AZX_DCAPS_INTEL_LEGACY_FALLBACK(CONFIG_SND_HDA_INTEL_LEGACY_FALLBACK_KBL) },
 	/* Kabylake-H */
 	{ PCI_DEVICE(0x8086, 0xa2f0),
 	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE },
 	/* Coffelake */
 	{ PCI_DEVICE(0x8086, 0xa348),
-	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE},
+	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE |
+	  AZX_DCAPS_INTEL_LEGACY_FALLBACK(CONFIG_SND_HDA_INTEL_LEGACY_FALLBACK_CFL) },
 	/* Cannonlake */
 	{ PCI_DEVICE(0x8086, 0x9dc8),
-	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE},
+	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE |
+	  AZX_DCAPS_INTEL_LEGACY_FALLBACK(CONFIG_SND_HDA_INTEL_LEGACY_FALLBACK_CNL) },
 	/* Icelake */
 	{ PCI_DEVICE(0x8086, 0x34c8),
 	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE},
 	/* Broxton-P(Apollolake) */
 	{ PCI_DEVICE(0x8086, 0x5a98),
-	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_BROXTON },
+	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_BROXTON |
+	  AZX_DCAPS_INTEL_LEGACY_FALLBACK(CONFIG_SND_HDA_INTEL_LEGACY_FALLBACK_APL) },
 	/* Broxton-T */
 	{ PCI_DEVICE(0x8086, 0x1a98),
 	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_BROXTON },
 	/* Gemini-Lake */
 	{ PCI_DEVICE(0x8086, 0x3198),
-	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_BROXTON },
+	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_BROXTON |
+	  AZX_DCAPS_INTEL_LEGACY_FALLBACK(CONFIG_SND_HDA_INTEL_LEGACY_FALLBACK_GLK) },
 	/* Haswell */
 	{ PCI_DEVICE(0x8086, 0x0a0c),
 	  .driver_data = AZX_DRIVER_HDMI | AZX_DCAPS_INTEL_HASWELL },
diff --git a/sound/soc/intel/Kconfig b/sound/soc/intel/Kconfig
index c02d08d31d0d..4c6abdbb0b90 100644
--- a/sound/soc/intel/Kconfig
+++ b/sound/soc/intel/Kconfig
@@ -197,6 +197,12 @@ config SND_SOC_INTEL_SKYLAKE_COMMON
 config SND_SOC_INTEL_SKL_LEGACY_SUPPORT
 	bool "Fallback legacy HD-audio binding"
 	depends on SND_HDA_INTEL=y || SND_SOC_INTEL_SKYLAKE_FAMILY=SND_HDA_INTEL
+	select SND_HDA_INTEL_LEGACY_FALLBACK_SKL if SND_SOC_INTEL_SKL
+	select SND_HDA_INTEL_LEGACY_FALLBACK_APL if SND_SOC_INTEL_APL
+	select SND_HDA_INTEL_LEGACY_FALLBACK_KBL if SND_SOC_INTEL_KBL
+	select SND_HDA_INTEL_LEGACY_FALLBACK_GLK if SND_SOC_INTEL_GLK
+	select SND_HDA_INTEL_LEGACY_FALLBACK_CNL if SND_SOC_INTEL_CNL
+	select SND_HDA_INTEL_LEGACY_FALLBACK_CFL if SND_SOC_INTEL_CFL
 	help
 	  Fallback binding with the legacy HD-audio driver when no DSP is
 	  found
-- 
2.17.1

Re: [RFC PATCH 0/6] ASoC:Intel:Skylake: Enable HDaudio legacy fallback

[Takashi Iwai]

On Tue, 20 Nov 2018 22:36:38 +0100,
Pierre-Louis Bossart wrote:
> 
> This patchset is provided as an RFC and should not be merged as is
> (Turkey break in the USA and more validation needed). This is however
> a good time to gather comments. This work is the result of multiple
> email discussions to finally provide more flexibility for
> distributions to chose whether to stick with the legacy HDaudio driver
> or to enable the SST/Skylake driver for newer platforms (required
> e.g. for digital microphone support)
> 
> The patches add support for CoffeeLake, simplify the probe for the
> Skylake driver, introduce more compile-time granularity so that
> platforms can be selected individually and last provide a dynamic
> fallback mechanism when two drivers are registered for the same PCI
> ID.
> 
> When the SOF driver is released, the same mechanism will be used to
> enable the SOF-legacy fallback. There will be no plans to enable an
> SOF->SST falback.

This looks like a sensible way to go, thanks for working on this!

While the fallback stuff might need more testing, the other patches
(addition of CFL-S and split of configs) are rather systematic, so we
can merge this at first soonish.

And we may need a bit more comments in Kconfig help for the fallback
behavior.  Or document it properly and refer to it from Kconfig help.
The git commit log isn't present in the released kernel code, after
all.


thanks,

Takashi

> 
> Pierre-Louis Bossart (4):
>   ASoC: Intel: Skylake: stop init/probe if DSP is not present
>   ASoC: Intel: Skylake: remove useless tests on DSP presence
>   ASoC: Intel: Skylake: Add more platform granularity
>   ALSA: hda: add fallback capabilities for SKL+ platforms
> 
> Takashi Iwai (2):
>   ASoC: Intel: Skylake: Add CFL-S support
>   ALSA: hda: Allow fallback binding with legacy HD-audio for Intel SKL+
> 
>  include/sound/hdaudio.h                |   6 +
>  sound/pci/hda/Kconfig                  |  40 +++++++
>  sound/pci/hda/hda_controller.h         |   2 +-
>  sound/pci/hda/hda_intel.c              |  51 ++++++--
>  sound/soc/intel/Kconfig                |  86 ++++++++++++--
>  sound/soc/intel/boards/Kconfig         |  16 ++-
>  sound/soc/intel/skylake/skl-messages.c |   8 ++
>  sound/soc/intel/skylake/skl.c          | 154 +++++++++++++++++++------
>  8 files changed, 311 insertions(+), 52 deletions(-)
> 
> -- 
> 2.17.1
> 

Re: [RFC PATCH 1/6] ASoC: Intel: Skylake: Add CFL-S support

[Andy Shevchenko]

On Tue, Nov 20, 2018 at 03:36:39PM -0600, Pierre-Louis Bossart wrote:
> From: Takashi Iwai <tiwai@suse.de>
> 
> It's with CNP, supposed to be equivalent with CNL entry.
> 

May you consider to switch to PCI_DEVICE_DATA() first?

> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  sound/soc/intel/skylake/skl-messages.c | 8 ++++++++
>  sound/soc/intel/skylake/skl.c          | 3 +++
>  2 files changed, 11 insertions(+)
> 
> diff --git a/sound/soc/intel/skylake/skl-messages.c b/sound/soc/intel/skylake/skl-messages.c
> index 8bfb8b0fa3d5..b0e6fb93eaf8 100644
> --- a/sound/soc/intel/skylake/skl-messages.c
> +++ b/sound/soc/intel/skylake/skl-messages.c
> @@ -247,6 +247,14 @@ static const struct skl_dsp_ops dsp_ops[] = {
>  		.init_fw = cnl_sst_init_fw,
>  		.cleanup = cnl_sst_dsp_cleanup
>  	},
> +	{
> +		.id = 0xa348,
> +		.num_cores = 4,
> +		.loader_ops = bxt_get_loader_ops,
> +		.init = cnl_sst_dsp_init,
> +		.init_fw = cnl_sst_init_fw,
> +		.cleanup = cnl_sst_dsp_cleanup
> +	},
>  };
>  
>  const struct skl_dsp_ops *skl_get_dsp_ops(int pci_id)
> diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
> index 3f0ac1312982..df36b8fe6d5e 100644
> --- a/sound/soc/intel/skylake/skl.c
> +++ b/sound/soc/intel/skylake/skl.c
> @@ -1121,6 +1121,9 @@ static const struct pci_device_id skl_ids[] = {
>  	/* CNL */
>  	{ PCI_DEVICE(0x8086, 0x9dc8),
>  		.driver_data = (unsigned long)&snd_soc_acpi_intel_cnl_machines},
> +	/* CFL */
> +	{ PCI_DEVICE(0x8086, 0xa348),
> +		.driver_data = (unsigned long)&snd_soc_acpi_intel_cnl_machines},
>  	{ 0, }
>  };
>  MODULE_DEVICE_TABLE(pci, skl_ids);
> -- 
> 2.17.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH 2/6] ASoC: Intel: Skylake: stop init/probe if DSP is not present

[Andy Shevchenko]

On Tue, Nov 20, 2018 at 03:36:40PM -0600, Pierre-Louis Bossart wrote:
> Check immediately if the DSP can be found, bail and avoid doing inits
> to enable legacy fallback without delay.

It does slightly more than described. Please do either remove unrelated changes, or fill the gap in the commit message. In the latter case it may require to split patch to two.

> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  sound/soc/intel/skylake/skl.c | 26 +++++++++++++++++---------
>  1 file changed, 17 insertions(+), 9 deletions(-)
> 
> diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
> index df36b8fe6d5e..1d7146773d19 100644
> --- a/sound/soc/intel/skylake/skl.c
> +++ b/sound/soc/intel/skylake/skl.c
> @@ -931,6 +931,12 @@ static int skl_first_init(struct hdac_bus *bus)
>  
>  	snd_hdac_bus_parse_capabilities(bus);
>  
> +	/* check if dsp is there */
> +	if (!bus->ppcap) {
> +		dev_err(bus->dev, "bus ppcap not set, DSP not present?\n");
> +		return -ENODEV;
> +	}
> +
>  	if (skl_acquire_irq(bus, 0) < 0)
>  		return -EBUSY;
>  
> @@ -940,23 +946,25 @@ static int skl_first_init(struct hdac_bus *bus)
>  	gcap = snd_hdac_chip_readw(bus, GCAP);
>  	dev_dbg(bus->dev, "chipset global capabilities = 0x%x\n", gcap);
>  
> -	/* allow 64bit DMA address if supported by H/W */
> -	if (!dma_set_mask(bus->dev, DMA_BIT_MASK(64))) {
> -		dma_set_coherent_mask(bus->dev, DMA_BIT_MASK(64));
> -	} else {
> -		dma_set_mask(bus->dev, DMA_BIT_MASK(32));
> -		dma_set_coherent_mask(bus->dev, DMA_BIT_MASK(32));
> -	}
> -
>  	/* read number of streams from GCAP register */
>  	cp_streams = (gcap >> 8) & 0x0f;
>  	pb_streams = (gcap >> 12) & 0x0f;
>  
> -	if (!pb_streams && !cp_streams)
> +	if (!pb_streams && !cp_streams) {
> +		dev_err(bus->dev, "no streams found in GCAP definitions?\n");
>  		return -EIO;
> +	}
>  
>  	bus->num_streams = cp_streams + pb_streams;
>  
> +	/* allow 64bit DMA address if supported by H/W */
> +	if (!dma_set_mask(bus->dev, DMA_BIT_MASK(64))) {
> +		dma_set_coherent_mask(bus->dev, DMA_BIT_MASK(64));
> +	} else {
> +		dma_set_mask(bus->dev, DMA_BIT_MASK(32));
> +		dma_set_coherent_mask(bus->dev, DMA_BIT_MASK(32));
> +	}
> +
>  	/* initialize streams */
>  	snd_hdac_ext_stream_init_all
>  		(bus, 0, cp_streams, SNDRV_PCM_STREAM_CAPTURE);
> -- 
> 2.17.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH 6/6] ALSA: hda: add fallback capabilities for SKL+ platforms

[Andy Shevchenko]

On Tue, Nov 20, 2018 at 03:36:44PM -0600, Pierre-Louis Bossart wrote:
> Enable fallback for select PCI IDs
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  sound/pci/hda/Kconfig     | 40 +++++++++++++++++++++++++++++++++++++++
>  sound/pci/hda/hda_intel.c | 19 +++++++++++++------
>  sound/soc/intel/Kconfig   |  6 ++++++
>  3 files changed, 59 insertions(+), 6 deletions(-)
> 
> diff --git a/sound/pci/hda/Kconfig b/sound/pci/hda/Kconfig
> index 4235907b7858..9bb317fb3507 100644
> --- a/sound/pci/hda/Kconfig
> +++ b/sound/pci/hda/Kconfig
> @@ -228,4 +228,44 @@ config SND_HDA_POWER_SAVE_DEFAULT
>  
>  endif
>  
> +if SND_HDA_INTEL
> +
> +config SND_HDA_INTEL_LEGACY_FALLBACK_SKL
> +	bool
> +	help
> +	  This option enables HD-audio legacy fallback for
> +	  Skylake machines
> +
> +config SND_HDA_INTEL_LEGACY_FALLBACK_APL
> +	bool
> +	help
> +	  This option enables HD-audio legacy fallback for
> +	  Broxton/ApolloLake machines
> +
> +config SND_HDA_INTEL_LEGACY_FALLBACK_KBL
> +	bool
> +	help
> +	  This option enables HD-audio legacy fallback for
> +	  KabyLake machines
> +
> +config SND_HDA_INTEL_LEGACY_FALLBACK_GLK
> +	bool
> +	help
> +	  This option enables HD-audio legacy fallback for
> +	  GeminiLake machines
> +
> +config SND_HDA_INTEL_LEGACY_FALLBACK_CNL
> +	bool
> +	help
> +	  This option enables HD-audio legacy fallback for
> +	  CannonLake machines
> +
> +config SND_HDA_INTEL_LEGACY_FALLBACK_CFL
> +	bool
> +	help
> +	  This option enables HD-audio legacy fallback for
> +	  CoffeeLake machines
> +
> +endif ## SND_HDA_INTEL
> +
>  endmenu
> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> index eb00e37c1c27..569419242da3 100644
> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -360,6 +360,7 @@ enum {
>  	 AZX_DCAPS_NO_64BIT |\
>  	 AZX_DCAPS_4K_BDLE_BOUNDARY | AZX_DCAPS_SNOOP_OFF)
>  
> +#define AZX_DCAPS_INTEL_LEGACY_FALLBACK(conf) (IS_ENABLED(conf) ? AZX_DCAPS_INTEL_SHARED : 0)
>  /*
>   * vga_switcheroo support
>   */
> @@ -2416,34 +2417,40 @@ static const struct pci_device_id azx_ids[] = {
>  	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE },
>  	/* Sunrise Point-LP */
>  	{ PCI_DEVICE(0x8086, 0x9d70),
> -	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE },
> +	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE |
> +	  AZX_DCAPS_INTEL_LEGACY_FALLBACK(CONFIG_SND_HDA_INTEL_LEGACY_FALLBACK_SKL) },

Preprocessor may concatenate the same prefix for you.
I expect to see something like ..._FALLBACK(SKL) and so on.

Moreover, you can go further and create a macro that would consolidate all bits together.


>  	/* Kabylake */
>  	{ PCI_DEVICE(0x8086, 0xa171),
>  	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE },
>  	/* Kabylake-LP */
>  	{ PCI_DEVICE(0x8086, 0x9d71),
> -	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE },
> +	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE |
> +	  AZX_DCAPS_INTEL_LEGACY_FALLBACK(CONFIG_SND_HDA_INTEL_LEGACY_FALLBACK_KBL) },
>  	/* Kabylake-H */
>  	{ PCI_DEVICE(0x8086, 0xa2f0),
>  	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE },
>  	/* Coffelake */
>  	{ PCI_DEVICE(0x8086, 0xa348),
> -	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE},
> +	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE |
> +	  AZX_DCAPS_INTEL_LEGACY_FALLBACK(CONFIG_SND_HDA_INTEL_LEGACY_FALLBACK_CFL) },
>  	/* Cannonlake */
>  	{ PCI_DEVICE(0x8086, 0x9dc8),
> -	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE},
> +	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE |
> +	  AZX_DCAPS_INTEL_LEGACY_FALLBACK(CONFIG_SND_HDA_INTEL_LEGACY_FALLBACK_CNL) },
>  	/* Icelake */
>  	{ PCI_DEVICE(0x8086, 0x34c8),
>  	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE},
>  	/* Broxton-P(Apollolake) */
>  	{ PCI_DEVICE(0x8086, 0x5a98),
> -	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_BROXTON },
> +	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_BROXTON |
> +	  AZX_DCAPS_INTEL_LEGACY_FALLBACK(CONFIG_SND_HDA_INTEL_LEGACY_FALLBACK_APL) },
>  	/* Broxton-T */
>  	{ PCI_DEVICE(0x8086, 0x1a98),
>  	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_BROXTON },
>  	/* Gemini-Lake */
>  	{ PCI_DEVICE(0x8086, 0x3198),
> -	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_BROXTON },
> +	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_BROXTON |
> +	  AZX_DCAPS_INTEL_LEGACY_FALLBACK(CONFIG_SND_HDA_INTEL_LEGACY_FALLBACK_GLK) },
>  	/* Haswell */
>  	{ PCI_DEVICE(0x8086, 0x0a0c),
>  	  .driver_data = AZX_DRIVER_HDMI | AZX_DCAPS_INTEL_HASWELL },
> diff --git a/sound/soc/intel/Kconfig b/sound/soc/intel/Kconfig
> index c02d08d31d0d..4c6abdbb0b90 100644
> --- a/sound/soc/intel/Kconfig
> +++ b/sound/soc/intel/Kconfig
> @@ -197,6 +197,12 @@ config SND_SOC_INTEL_SKYLAKE_COMMON
>  config SND_SOC_INTEL_SKL_LEGACY_SUPPORT
>  	bool "Fallback legacy HD-audio binding"
>  	depends on SND_HDA_INTEL=y || SND_SOC_INTEL_SKYLAKE_FAMILY=SND_HDA_INTEL
> +	select SND_HDA_INTEL_LEGACY_FALLBACK_SKL if SND_SOC_INTEL_SKL
> +	select SND_HDA_INTEL_LEGACY_FALLBACK_APL if SND_SOC_INTEL_APL
> +	select SND_HDA_INTEL_LEGACY_FALLBACK_KBL if SND_SOC_INTEL_KBL
> +	select SND_HDA_INTEL_LEGACY_FALLBACK_GLK if SND_SOC_INTEL_GLK
> +	select SND_HDA_INTEL_LEGACY_FALLBACK_CNL if SND_SOC_INTEL_CNL
> +	select SND_HDA_INTEL_LEGACY_FALLBACK_CFL if SND_SOC_INTEL_CFL
>  	help
>  	  Fallback binding with the legacy HD-audio driver when no DSP is
>  	  found
> -- 
> 2.17.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH 2/6] ASoC: Intel: Skylake: stop init/probe if DSP is not present

[Pierre-Louis Bossart]

On 11/21/18 8:29 AM, Andy Shevchenko wrote:
> On Tue, Nov 20, 2018 at 03:36:40PM -0600, Pierre-Louis Bossart wrote:
>> Check immediately if the DSP can be found, bail and avoid doing inits
>> to enable legacy fallback without delay.
> It does slightly more than described. Please do either remove unrelated changes, or fill the gap in the commit message. In the latter case it may require to split patch to two.
ok, maybe I should change the commit message since there are really two 
test conditions: DSP can be found and DSP streams found. The code really 
does just that, nothing else.
>> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>> ---
>>   sound/soc/intel/skylake/skl.c | 26 +++++++++++++++++---------
>>   1 file changed, 17 insertions(+), 9 deletions(-)
>>
>> diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
>> index df36b8fe6d5e..1d7146773d19 100644
>> --- a/sound/soc/intel/skylake/skl.c
>> +++ b/sound/soc/intel/skylake/skl.c
>> @@ -931,6 +931,12 @@ static int skl_first_init(struct hdac_bus *bus)
>>   
>>   	snd_hdac_bus_parse_capabilities(bus);
>>   
>> +	/* check if dsp is there */
>> +	if (!bus->ppcap) {
>> +		dev_err(bus->dev, "bus ppcap not set, DSP not present?\n");
>> +		return -ENODEV;
>> +	}
>> +
>>   	if (skl_acquire_irq(bus, 0) < 0)
>>   		return -EBUSY;
>>   
>> @@ -940,23 +946,25 @@ static int skl_first_init(struct hdac_bus *bus)
>>   	gcap = snd_hdac_chip_readw(bus, GCAP);
>>   	dev_dbg(bus->dev, "chipset global capabilities = 0x%x\n", gcap);
>>   
>> -	/* allow 64bit DMA address if supported by H/W */
>> -	if (!dma_set_mask(bus->dev, DMA_BIT_MASK(64))) {
>> -		dma_set_coherent_mask(bus->dev, DMA_BIT_MASK(64));
>> -	} else {
>> -		dma_set_mask(bus->dev, DMA_BIT_MASK(32));
>> -		dma_set_coherent_mask(bus->dev, DMA_BIT_MASK(32));
>> -	}
>> -
>>   	/* read number of streams from GCAP register */
>>   	cp_streams = (gcap >> 8) & 0x0f;
>>   	pb_streams = (gcap >> 12) & 0x0f;
>>   
>> -	if (!pb_streams && !cp_streams)
>> +	if (!pb_streams && !cp_streams) {
>> +		dev_err(bus->dev, "no streams found in GCAP definitions?\n");
>>   		return -EIO;
>> +	}
>>   
>>   	bus->num_streams = cp_streams + pb_streams;
>>   
>> +	/* allow 64bit DMA address if supported by H/W */
>> +	if (!dma_set_mask(bus->dev, DMA_BIT_MASK(64))) {
>> +		dma_set_coherent_mask(bus->dev, DMA_BIT_MASK(64));
>> +	} else {
>> +		dma_set_mask(bus->dev, DMA_BIT_MASK(32));
>> +		dma_set_coherent_mask(bus->dev, DMA_BIT_MASK(32));
>> +	}
>> +
>>   	/* initialize streams */
>>   	snd_hdac_ext_stream_init_all
>>   		(bus, 0, cp_streams, SNDRV_PCM_STREAM_CAPTURE);
>> -- 
>> 2.17.1
>>

Re: [RFC PATCH 1/6] ASoC: Intel: Skylake: Add CFL-S support

[Pierre-Louis Bossart]

On 11/21/18 8:27 AM, Andy Shevchenko wrote:
> On Tue, Nov 20, 2018 at 03:36:39PM -0600, Pierre-Louis Bossart wrote:
>> From: Takashi Iwai <tiwai@suse.de>
>>
>> It's with CNP, supposed to be equivalent with CNL entry.
>>
> May you consider to switch to PCI_DEVICE_DATA() first?

Is this really the recommended path?

The macro generates PCI_DEVICE_ID_##vend##_##dev, and I don't have a 
turn key #define PCI_DEVICE_ID_INTEL_AUDIO_CFL 0xa348 I can use. In a 
number of cases we have multiple variants of the same hardware, and it 
starts being painful to use a 20-letter macro to differentiate between 
INTEL_AUDIO_CFL_Y and INTEL_AUDIO_CFL_H. The explicit code and a short 
comment are more readable really.

git grep PCI_DEVICE_ID_INTEL gives me hundreds of definitions, some 
global, some local to specific drivers, doesn't seem like there is a 
well-agreed usage of this macro, is there? I don't mind making the 
change but I don't sense an strong argument for it?

>
>> Signed-off-by: Takashi Iwai <tiwai@suse.de>
>> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>> ---
>>   sound/soc/intel/skylake/skl-messages.c | 8 ++++++++
>>   sound/soc/intel/skylake/skl.c          | 3 +++
>>   2 files changed, 11 insertions(+)
>>
>> diff --git a/sound/soc/intel/skylake/skl-messages.c b/sound/soc/intel/skylake/skl-messages.c
>> index 8bfb8b0fa3d5..b0e6fb93eaf8 100644
>> --- a/sound/soc/intel/skylake/skl-messages.c
>> +++ b/sound/soc/intel/skylake/skl-messages.c
>> @@ -247,6 +247,14 @@ static const struct skl_dsp_ops dsp_ops[] = {
>>   		.init_fw = cnl_sst_init_fw,
>>   		.cleanup = cnl_sst_dsp_cleanup
>>   	},
>> +	{
>> +		.id = 0xa348,
>> +		.num_cores = 4,
>> +		.loader_ops = bxt_get_loader_ops,
>> +		.init = cnl_sst_dsp_init,
>> +		.init_fw = cnl_sst_init_fw,
>> +		.cleanup = cnl_sst_dsp_cleanup
>> +	},
>>   };
>>   
>>   const struct skl_dsp_ops *skl_get_dsp_ops(int pci_id)
>> diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
>> index 3f0ac1312982..df36b8fe6d5e 100644
>> --- a/sound/soc/intel/skylake/skl.c
>> +++ b/sound/soc/intel/skylake/skl.c
>> @@ -1121,6 +1121,9 @@ static const struct pci_device_id skl_ids[] = {
>>   	/* CNL */
>>   	{ PCI_DEVICE(0x8086, 0x9dc8),
>>   		.driver_data = (unsigned long)&snd_soc_acpi_intel_cnl_machines},
>> +	/* CFL */
>> +	{ PCI_DEVICE(0x8086, 0xa348),
>> +		.driver_data = (unsigned long)&snd_soc_acpi_intel_cnl_machines},
>>   	{ 0, }
>>   };
>>   MODULE_DEVICE_TABLE(pci, skl_ids);
>> -- 
>> 2.17.1
>>

Re: [RFC PATCH 2/6] ASoC: Intel: Skylake: stop init/probe if DSP is not present

[Andy Shevchenko]

On Wed, Nov 21, 2018 at 10:48:57AM -0600, Pierre-Louis Bossart wrote:
> 
> On 11/21/18 8:29 AM, Andy Shevchenko wrote:
> > On Tue, Nov 20, 2018 at 03:36:40PM -0600, Pierre-Louis Bossart wrote:
> > > Check immediately if the DSP can be found, bail and avoid doing inits
> > > to enable legacy fallback without delay.
> > It does slightly more than described. Please do either remove unrelated changes, or fill the gap in the commit message. In the latter case it may require to split patch to two.
> ok, maybe I should change the commit message since there are really two test
> conditions: DSP can be found and DSP streams found. The code really does
> just that, nothing else.

How shuffling DMA mask related to any?

> > > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > > ---
> > >   sound/soc/intel/skylake/skl.c | 26 +++++++++++++++++---------
> > >   1 file changed, 17 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
> > > index df36b8fe6d5e..1d7146773d19 100644
> > > --- a/sound/soc/intel/skylake/skl.c
> > > +++ b/sound/soc/intel/skylake/skl.c
> > > @@ -931,6 +931,12 @@ static int skl_first_init(struct hdac_bus *bus)
> > >   	snd_hdac_bus_parse_capabilities(bus);
> > > +	/* check if dsp is there */
> > > +	if (!bus->ppcap) {
> > > +		dev_err(bus->dev, "bus ppcap not set, DSP not present?\n");
> > > +		return -ENODEV;
> > > +	}
> > > +
> > >   	if (skl_acquire_irq(bus, 0) < 0)
> > >   		return -EBUSY;
> > > @@ -940,23 +946,25 @@ static int skl_first_init(struct hdac_bus *bus)
> > >   	gcap = snd_hdac_chip_readw(bus, GCAP);
> > >   	dev_dbg(bus->dev, "chipset global capabilities = 0x%x\n", gcap);
> > > -	/* allow 64bit DMA address if supported by H/W */
> > > -	if (!dma_set_mask(bus->dev, DMA_BIT_MASK(64))) {
> > > -		dma_set_coherent_mask(bus->dev, DMA_BIT_MASK(64));
> > > -	} else {
> > > -		dma_set_mask(bus->dev, DMA_BIT_MASK(32));
> > > -		dma_set_coherent_mask(bus->dev, DMA_BIT_MASK(32));
> > > -	}
> > > -
> > >   	/* read number of streams from GCAP register */
> > >   	cp_streams = (gcap >> 8) & 0x0f;
> > >   	pb_streams = (gcap >> 12) & 0x0f;
> > > -	if (!pb_streams && !cp_streams)
> > > +	if (!pb_streams && !cp_streams) {
> > > +		dev_err(bus->dev, "no streams found in GCAP definitions?\n");
> > >   		return -EIO;
> > > +	}
> > >   	bus->num_streams = cp_streams + pb_streams;
> > > +	/* allow 64bit DMA address if supported by H/W */
> > > +	if (!dma_set_mask(bus->dev, DMA_BIT_MASK(64))) {
> > > +		dma_set_coherent_mask(bus->dev, DMA_BIT_MASK(64));
> > > +	} else {
> > > +		dma_set_mask(bus->dev, DMA_BIT_MASK(32));
> > > +		dma_set_coherent_mask(bus->dev, DMA_BIT_MASK(32));
> > > +	}
> > > +
> > >   	/* initialize streams */
> > >   	snd_hdac_ext_stream_init_all
> > >   		(bus, 0, cp_streams, SNDRV_PCM_STREAM_CAPTURE);
> > > -- 
> > > 2.17.1
> > > 

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH 1/6] ASoC: Intel: Skylake: Add CFL-S support

[Andy Shevchenko]

On Wed, Nov 21, 2018 at 11:16:50AM -0600, Pierre-Louis Bossart wrote:
> On 11/21/18 8:27 AM, Andy Shevchenko wrote:

> > May you consider to switch to PCI_DEVICE_DATA() first?
> 
> Is this really the recommended path?
> 
> The macro generates PCI_DEVICE_ID_##vend##_##dev, and I don't have a turn
> key #define PCI_DEVICE_ID_INTEL_AUDIO_CFL 0xa348 I can use. In a number of
> cases we have multiple variants of the same hardware, and it starts being
> painful to use a 20-letter macro to differentiate between INTEL_AUDIO_CFL_Y
> and INTEL_AUDIO_CFL_H. The explicit code and a short comment are more
> readable really.
> 
> git grep PCI_DEVICE_ID_INTEL gives me hundreds of definitions, some global,
> some local to specific drivers, doesn't seem like there is a well-agreed
> usage of this macro, is there? I don't mind making the change but I don't
> sense an strong argument for it?

Compare:

	/* CFL */
	{ PCI_DEVICE(0x8086, 0xa348),
		.driver_data = (unsigned long)&snd_soc_acpi_intel_cnl_machines},

to something like:

#define PCI_DEVICE_ID_INTEL_AUDIO_CFL	0xa348
...

	{PCI_DEVICE_DATA(INTEL, AUDIO_CFL, &snd_soc_acpi_intel_cnl_machines)},


Macro is recently introduced, that's why not many users of it. At least I'm
planning to clean up dwc3-pci.c using it.

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH 1/6] ASoC: Intel: Skylake: Add CFL-S support

[Takashi Iwai]

On Wed, 21 Nov 2018 18:38:41 +0100,
Andy Shevchenko wrote:
> 
> On Wed, Nov 21, 2018 at 11:16:50AM -0600, Pierre-Louis Bossart wrote:
> > On 11/21/18 8:27 AM, Andy Shevchenko wrote:
> 
> > > May you consider to switch to PCI_DEVICE_DATA() first?
> > 
> > Is this really the recommended path?
> > 
> > The macro generates PCI_DEVICE_ID_##vend##_##dev, and I don't have a turn
> > key #define PCI_DEVICE_ID_INTEL_AUDIO_CFL 0xa348 I can use. In a number of
> > cases we have multiple variants of the same hardware, and it starts being
> > painful to use a 20-letter macro to differentiate between INTEL_AUDIO_CFL_Y
> > and INTEL_AUDIO_CFL_H. The explicit code and a short comment are more
> > readable really.
> > 
> > git grep PCI_DEVICE_ID_INTEL gives me hundreds of definitions, some global,
> > some local to specific drivers, doesn't seem like there is a well-agreed
> > usage of this macro, is there? I don't mind making the change but I don't
> > sense an strong argument for it?
> 
> Compare:
> 
> 	/* CFL */
> 	{ PCI_DEVICE(0x8086, 0xa348),
> 		.driver_data = (unsigned long)&snd_soc_acpi_intel_cnl_machines},
> 
> to something like:
> 
> #define PCI_DEVICE_ID_INTEL_AUDIO_CFL	0xa348
> ...
> 
> 	{PCI_DEVICE_DATA(INTEL, AUDIO_CFL, &snd_soc_acpi_intel_cnl_machines)},

The former gives a better grep-ability, though.

I have no big preference over two, just want to mention that both have
merits and demerits.


thanks,

Takashi

Re: [RFC PATCH 6/6] ALSA: hda: add fallback capabilities for SKL+ platforms

[Takashi Iwai]

On Wed, 21 Nov 2018 15:39:39 +0100,
Andy Shevchenko wrote:
> 
> On Tue, Nov 20, 2018 at 03:36:44PM -0600, Pierre-Louis Bossart wrote:
> > Enable fallback for select PCI IDs
> > 
> > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > ---
> >  sound/pci/hda/Kconfig     | 40 +++++++++++++++++++++++++++++++++++++++
> >  sound/pci/hda/hda_intel.c | 19 +++++++++++++------
> >  sound/soc/intel/Kconfig   |  6 ++++++
> >  3 files changed, 59 insertions(+), 6 deletions(-)
> > 
> > diff --git a/sound/pci/hda/Kconfig b/sound/pci/hda/Kconfig
> > index 4235907b7858..9bb317fb3507 100644
> > --- a/sound/pci/hda/Kconfig
> > +++ b/sound/pci/hda/Kconfig
> > @@ -228,4 +228,44 @@ config SND_HDA_POWER_SAVE_DEFAULT
> >  
> >  endif
> >  
> > +if SND_HDA_INTEL
> > +
> > +config SND_HDA_INTEL_LEGACY_FALLBACK_SKL
> > +	bool
> > +	help
> > +	  This option enables HD-audio legacy fallback for
> > +	  Skylake machines
> > +
> > +config SND_HDA_INTEL_LEGACY_FALLBACK_APL
> > +	bool
> > +	help
> > +	  This option enables HD-audio legacy fallback for
> > +	  Broxton/ApolloLake machines
> > +
> > +config SND_HDA_INTEL_LEGACY_FALLBACK_KBL
> > +	bool
> > +	help
> > +	  This option enables HD-audio legacy fallback for
> > +	  KabyLake machines
> > +
> > +config SND_HDA_INTEL_LEGACY_FALLBACK_GLK
> > +	bool
> > +	help
> > +	  This option enables HD-audio legacy fallback for
> > +	  GeminiLake machines
> > +
> > +config SND_HDA_INTEL_LEGACY_FALLBACK_CNL
> > +	bool
> > +	help
> > +	  This option enables HD-audio legacy fallback for
> > +	  CannonLake machines
> > +
> > +config SND_HDA_INTEL_LEGACY_FALLBACK_CFL
> > +	bool
> > +	help
> > +	  This option enables HD-audio legacy fallback for
> > +	  CoffeeLake machines
> > +
> > +endif ## SND_HDA_INTEL
> > +
> >  endmenu
> > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> > index eb00e37c1c27..569419242da3 100644
> > --- a/sound/pci/hda/hda_intel.c
> > +++ b/sound/pci/hda/hda_intel.c
> > @@ -360,6 +360,7 @@ enum {
> >  	 AZX_DCAPS_NO_64BIT |\
> >  	 AZX_DCAPS_4K_BDLE_BOUNDARY | AZX_DCAPS_SNOOP_OFF)
> >  
> > +#define AZX_DCAPS_INTEL_LEGACY_FALLBACK(conf) (IS_ENABLED(conf) ? AZX_DCAPS_INTEL_SHARED : 0)
> >  /*
> >   * vga_switcheroo support
> >   */
> > @@ -2416,34 +2417,40 @@ static const struct pci_device_id azx_ids[] = {
> >  	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE },
> >  	/* Sunrise Point-LP */
> >  	{ PCI_DEVICE(0x8086, 0x9d70),
> > -	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE },
> > +	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE |
> > +	  AZX_DCAPS_INTEL_LEGACY_FALLBACK(CONFIG_SND_HDA_INTEL_LEGACY_FALLBACK_SKL) },
> 
> Preprocessor may concatenate the same prefix for you.
> I expect to see something like ..._FALLBACK(SKL) and so on.

It' look shorter and better readable, but OTOH, keeping CONFIG_XYZ
allows to search the kconfig more easily over the code.
Again, we need to consider some drawback.


thanks,

Takashi

> 
> Moreover, you can go further and create a macro that would consolidate all bits together.
> 
> 
> >  	/* Kabylake */
> >  	{ PCI_DEVICE(0x8086, 0xa171),
> >  	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE },
> >  	/* Kabylake-LP */
> >  	{ PCI_DEVICE(0x8086, 0x9d71),
> > -	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE },
> > +	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE |
> > +	  AZX_DCAPS_INTEL_LEGACY_FALLBACK(CONFIG_SND_HDA_INTEL_LEGACY_FALLBACK_KBL) },
> >  	/* Kabylake-H */
> >  	{ PCI_DEVICE(0x8086, 0xa2f0),
> >  	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE },
> >  	/* Coffelake */
> >  	{ PCI_DEVICE(0x8086, 0xa348),
> > -	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE},
> > +	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE |
> > +	  AZX_DCAPS_INTEL_LEGACY_FALLBACK(CONFIG_SND_HDA_INTEL_LEGACY_FALLBACK_CFL) },
> >  	/* Cannonlake */
> >  	{ PCI_DEVICE(0x8086, 0x9dc8),
> > -	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE},
> > +	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE |
> > +	  AZX_DCAPS_INTEL_LEGACY_FALLBACK(CONFIG_SND_HDA_INTEL_LEGACY_FALLBACK_CNL) },
> >  	/* Icelake */
> >  	{ PCI_DEVICE(0x8086, 0x34c8),
> >  	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE},
> >  	/* Broxton-P(Apollolake) */
> >  	{ PCI_DEVICE(0x8086, 0x5a98),
> > -	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_BROXTON },
> > +	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_BROXTON |
> > +	  AZX_DCAPS_INTEL_LEGACY_FALLBACK(CONFIG_SND_HDA_INTEL_LEGACY_FALLBACK_APL) },
> >  	/* Broxton-T */
> >  	{ PCI_DEVICE(0x8086, 0x1a98),
> >  	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_BROXTON },
> >  	/* Gemini-Lake */
> >  	{ PCI_DEVICE(0x8086, 0x3198),
> > -	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_BROXTON },
> > +	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_BROXTON |
> > +	  AZX_DCAPS_INTEL_LEGACY_FALLBACK(CONFIG_SND_HDA_INTEL_LEGACY_FALLBACK_GLK) },
> >  	/* Haswell */
> >  	{ PCI_DEVICE(0x8086, 0x0a0c),
> >  	  .driver_data = AZX_DRIVER_HDMI | AZX_DCAPS_INTEL_HASWELL },
> > diff --git a/sound/soc/intel/Kconfig b/sound/soc/intel/Kconfig
> > index c02d08d31d0d..4c6abdbb0b90 100644
> > --- a/sound/soc/intel/Kconfig
> > +++ b/sound/soc/intel/Kconfig
> > @@ -197,6 +197,12 @@ config SND_SOC_INTEL_SKYLAKE_COMMON
> >  config SND_SOC_INTEL_SKL_LEGACY_SUPPORT
> >  	bool "Fallback legacy HD-audio binding"
> >  	depends on SND_HDA_INTEL=y || SND_SOC_INTEL_SKYLAKE_FAMILY=SND_HDA_INTEL
> > +	select SND_HDA_INTEL_LEGACY_FALLBACK_SKL if SND_SOC_INTEL_SKL
> > +	select SND_HDA_INTEL_LEGACY_FALLBACK_APL if SND_SOC_INTEL_APL
> > +	select SND_HDA_INTEL_LEGACY_FALLBACK_KBL if SND_SOC_INTEL_KBL
> > +	select SND_HDA_INTEL_LEGACY_FALLBACK_GLK if SND_SOC_INTEL_GLK
> > +	select SND_HDA_INTEL_LEGACY_FALLBACK_CNL if SND_SOC_INTEL_CNL
> > +	select SND_HDA_INTEL_LEGACY_FALLBACK_CFL if SND_SOC_INTEL_CFL
> >  	help
> >  	  Fallback binding with the legacy HD-audio driver when no DSP is
> >  	  found
> > -- 
> > 2.17.1
> > 
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Re: [RFC PATCH 1/6] ASoC: Intel: Skylake: Add CFL-S support

[Andy Shevchenko]

On Wed, Nov 21, 2018 at 11:17:41PM +0100, Takashi Iwai wrote:
> On Wed, 21 Nov 2018 18:38:41 +0100, Andy Shevchenko wrote:

> > Compare:
> > 
> > 	/* CFL */
> > 	{ PCI_DEVICE(0x8086, 0xa348),
> > 		.driver_data = (unsigned long)&snd_soc_acpi_intel_cnl_machines},
> > 
> > to something like:
> > 
> > #define PCI_DEVICE_ID_INTEL_AUDIO_CFL	0xa348
> > ...
> > 
> > 	{PCI_DEVICE_DATA(INTEL, AUDIO_CFL, &snd_soc_acpi_intel_cnl_machines)},
> 
> The former gives a better grep-ability, though.
> 
> I have no big preference over two, just want to mention that both have
> merits and demerits.

True.
So, up to you, guys, what to choose.

-- 
With Best Regards,
Andy Shevchenko

Re: [alsa-devel] [RFC PATCH 1/6] ASoC: Intel: Skylake: Add CFL-S support

[Pierre-Louis Bossart]

On 11/22/18 3:56 AM, Andy Shevchenko wrote:
> On Wed, Nov 21, 2018 at 11:17:41PM +0100, Takashi Iwai wrote:
>> On Wed, 21 Nov 2018 18:38:41 +0100, Andy Shevchenko wrote:
>>> Compare:
>>>
>>> 	/* CFL */
>>> 	{ PCI_DEVICE(0x8086, 0xa348),
>>> 		.driver_data = (unsigned long)&snd_soc_acpi_intel_cnl_machines},
>>>
>>> to something like:
>>>
>>> #define PCI_DEVICE_ID_INTEL_AUDIO_CFL	0xa348
>>> ...
>>>
>>> 	{PCI_DEVICE_DATA(INTEL, AUDIO_CFL, &snd_soc_acpi_intel_cnl_machines)},
>> The former gives a better grep-ability, though.
>>
>> I have no big preference over two, just want to mention that both have
>> merits and demerits.
> True.
> So, up to you, guys, what to choose.

I am leaning to leave the PCI stuff alone for now. If we change the PCI 
definitions I'd like to do it on the SKL and legacy sides at the same 
time, to avoid multiple definitions or redundancies.

However I like your suggestion for the macros on the other patch so will 
change the code.

Thanks for the reviews!

>

Re: [RFC PATCH 0/6] ASoC:Intel:Skylake: Enable HDaudio legacy fallback

[Pierre-Louis Bossart]

On 11/21/18 4:32 AM, Takashi Iwai wrote:
> On Tue, 20 Nov 2018 22:36:38 +0100,
> Pierre-Louis Bossart wrote:
>> This patchset is provided as an RFC and should not be merged as is
>> (Turkey break in the USA and more validation needed). This is however
>> a good time to gather comments. This work is the result of multiple
>> email discussions to finally provide more flexibility for
>> distributions to chose whether to stick with the legacy HDaudio driver
>> or to enable the SST/Skylake driver for newer platforms (required
>> e.g. for digital microphone support)
>>
>> The patches add support for CoffeeLake, simplify the probe for the
>> Skylake driver, introduce more compile-time granularity so that
>> platforms can be selected individually and last provide a dynamic
>> fallback mechanism when two drivers are registered for the same PCI
>> ID.
>>
>> When the SOF driver is released, the same mechanism will be used to
>> enable the SOF-legacy fallback. There will be no plans to enable an
>> SOF->SST falback.
> This looks like a sensible way to go, thanks for working on this!
>
> While the fallback stuff might need more testing, the other patches
> (addition of CFL-S and split of configs) are rather systematic, so we
> can merge this at first soonish.
>
> And we may need a bit more comments in Kconfig help for the fallback
> behavior.  Or document it properly and refer to it from Kconfig help.
> The git commit log isn't present in the released kernel code, after
> all.

I will indeed put the dynamic fallback on the back-burner, of course the 
tests work on NUCs and recent devices but fail on a HP SKL device I 
tried on, so additional code needs to be added to check if the DSP is 
present or not, remove silly dependencies on NHLT that make no sense for 
HDaudio, etc.

I'd like however to follow your idea of a 'shared' caps but use it for 
static mutual-exclusion to start, e.g. to let distributions use legacy 
for Broadwell but SST for SKL/KBL without requiring the user to play 
with blacklists.  Once we have this in place, and the additional code 
added for DSP detection the dynamic fallback is an 'easy' transition.

>
>
> thanks,
>
> Takashi
>
>> Pierre-Louis Bossart (4):
>>    ASoC: Intel: Skylake: stop init/probe if DSP is not present
>>    ASoC: Intel: Skylake: remove useless tests on DSP presence
>>    ASoC: Intel: Skylake: Add more platform granularity
>>    ALSA: hda: add fallback capabilities for SKL+ platforms
>>
>> Takashi Iwai (2):
>>    ASoC: Intel: Skylake: Add CFL-S support
>>    ALSA: hda: Allow fallback binding with legacy HD-audio for Intel SKL+
>>
>>   include/sound/hdaudio.h                |   6 +
>>   sound/pci/hda/Kconfig                  |  40 +++++++
>>   sound/pci/hda/hda_controller.h         |   2 +-
>>   sound/pci/hda/hda_intel.c              |  51 ++++++--
>>   sound/soc/intel/Kconfig                |  86 ++++++++++++--
>>   sound/soc/intel/boards/Kconfig         |  16 ++-
>>   sound/soc/intel/skylake/skl-messages.c |   8 ++
>>   sound/soc/intel/skylake/skl.c          | 154 +++++++++++++++++++------
>>   8 files changed, 311 insertions(+), 52 deletions(-)
>>
>> -- 
>> 2.17.1
>>