[PATCH 0/7] ASoC: Intel: Skylake: Fix HDaudio and Dmic

[PATCH 0/7] ASoC: Intel: Skylake: Fix HDaudio and Dmic

[Cezary Rojewski]

Following is the list of fixes and updates targeting HDaudio +/- Dmic
configuration on Intel DSP platforms.


- ASoC: Intel: Skylake: Remove superfluous chip initialization
  ASoC: Intel: Skylake: Select hda configuration permissively

First patch addresses race condition issue between i915 and hda
controller. This is done by yielding priority to i915 so iDisp codec can
enumerate properly: same codec_mask is now observed regardless of driver
chosen (snd_hda_intel vs snd_soc_skl).

Second patch is a consequence of the first, to prevent driver from
incorrectly aborting probe - rather than reorganizing Skylake's boot
flow, small changed has been proposed.


- ASoC: Intel: Skylake: Shield against no-NHLT configurations

Some hardware has no NHLT exposed by BIOS (or an equivalent). Changes
have been made to ensure driver is shielded against null-dereferences and
such which occur when said table is absent.


- ASoC: Intel: skl_hda_dsp: Enable Dmic configuration

While DMIC is available on some production stuff, Intel platforms with
Skylake driver do not treat it as a valid option if no additional I2S
codec in present onboard. Update skl_hda_dsp board to expose Dmic
connections too.


- ASoC: Intel: Allow for ROM init retry on CNL platforms
  ASoC: Intel: Skylake: Await purge request ack on CNL

Both address rom init timeouts during CNL/ CFL/ CML/ WHL boot up
sequences. These provide retry mechanism and ensure purge request is
acked before proceding with FW load. bxt-sst.c has had these fixes
appended long ago - somehow someone forgotten about CNL family.

*****

Note: topology update is also needed to enable HDA +/- Dmic
configuration as existing ones do not contain any routes or widgets
required to enable it - these care about I2S only. We have prepared
corresponding UCM files too. Will be sharing them shortly.

This patchset has been prepared internally for topmost linux-stable 5.5
and 4.20 (no 4.19 as skl_hda_dsp did not exist there yet).

Apart from our RVPs, we have run tests also on:
- KBL Lenovo Carbon X1
- SKL Dell XPS 9350
- WHL Acer Swift 5

Honestly, I'd see HDaudio related patches being backport as low as 4.20
(although some changes had to be adjusted due to base differences
between 4.20 and 5.5, can share these too). One could argue HDA + Dmic
configuration should be available on 4.19 too - it's an LTS after all.
However, that time, some changes could be counted as "feature" rather
than fixes. Awaiting your replies and thoughts on that.

In consequence, I've appended "Fixes" only for last two patches for now
- once decisions are made, can append adequate tags wherever necessary.

Cezary Rojewski (6):
  ASoC: Intel: Skylake: Remove superfluous chip initialization
  ASoC: Intel: Skylake: Select hda configuration permissively
  ASoC: Intel: Skylake: Enable codec wakeup during chip init
  ASoC: Intel: Skylake: Shield against no-NHLT configurations
  ASoC: Intel: Allow for ROM init retry on CNL platforms
  ASoC: Intel: Skylake: Await purge request ack on CNL

Mateusz Gorski (1):
  ASoC: Intel: skl_hda_dsp: Enable Dmic configuration

 sound/soc/intel/boards/skl_hda_dsp_generic.c |  3 ++
 sound/soc/intel/skylake/bxt-sst.c            |  3 --
 sound/soc/intel/skylake/cnl-sst.c            | 35 ++++++++++++++++----
 sound/soc/intel/skylake/skl-nhlt.c           |  3 +-
 sound/soc/intel/skylake/skl-sst-dsp.h        |  2 ++
 sound/soc/intel/skylake/skl.c                | 29 ++++++++--------
 6 files changed, 48 insertions(+), 27 deletions(-)

-- 
2.17.1

[PATCH 1/7] ASoC: Intel: Skylake: Remove superfluous chip initialization

[Cezary Rojewski]

Skylake driver does the controller init operation twice:
- first during probe (only to stop it just before scheduling probe_work)
- and during said probe_work where the actual correct sequence is
executed

To properly complete boot sequence when iDisp codec is present, bus
initialization has to be called only after _i915_init() finishes.
With additional _reset_list preceding _i915_init(), iDisp codec never
gets the chance to enumerate on the link. Remove the superfluous
initialization to address the issue.

Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/soc/intel/skylake/skl.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
index f755ca2484cf..d66231525356 100644
--- a/sound/soc/intel/skylake/skl.c
+++ b/sound/soc/intel/skylake/skl.c
@@ -803,6 +803,9 @@ static void skl_probe_work(struct work_struct *work)
 			return;
 	}
 
+	skl_init_pci(skl);
+	skl_dum_set(bus);
+
 	err = skl_init_chip(bus, true);
 	if (err < 0) {
 		dev_err(bus->dev, "Init chip failed with err: %d\n", err);
@@ -918,8 +921,6 @@ static int skl_first_init(struct hdac_bus *bus)
 		return -ENXIO;
 	}
 
-	snd_hdac_bus_reset_link(bus, true);
-
 	snd_hdac_bus_parse_capabilities(bus);
 
 	/* check if PPCAP exists */
@@ -967,11 +968,7 @@ static int skl_first_init(struct hdac_bus *bus)
 	if (err < 0)
 		return err;
 
-	/* initialize chip */
-	skl_init_pci(skl);
-	skl_dum_set(bus);
-
-	return skl_init_chip(bus, true);
+	return 0;
 }
 
 static int skl_probe(struct pci_dev *pci,
@@ -1064,8 +1061,6 @@ static int skl_probe(struct pci_dev *pci,
 	if (bus->mlcap)
 		snd_hdac_ext_bus_get_ml_capabilities(bus);
 
-	snd_hdac_bus_stop_chip(bus);
-
 	/* create device for soc dmic */
 	err = skl_dmic_device_register(skl);
 	if (err < 0) {
-- 
2.17.1

[PATCH 2/7] ASoC: Intel: Skylake: Select hda configuration permissively

[Cezary Rojewski]

With _reset_link removed from the probe sequence, codec_mask at the time
skl_find_hda_machine() is invoked will always be 0, so hda machine will
never be chosen. Rather than reorganizing boot flow, be permissive about
invalid mask. codec_mask will be set to proper value during probe_work -
before skl_codec_create() ever gets called.

Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/soc/intel/skylake/skl.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
index d66231525356..4827fe6bc1cb 100644
--- a/sound/soc/intel/skylake/skl.c
+++ b/sound/soc/intel/skylake/skl.c
@@ -481,13 +481,8 @@ static struct skl_ssp_clk skl_ssp_clks[] = {
 static struct snd_soc_acpi_mach *skl_find_hda_machine(struct skl_dev *skl,
 					struct snd_soc_acpi_mach *machines)
 {
-	struct hdac_bus *bus = skl_to_bus(skl);
 	struct snd_soc_acpi_mach *mach;
 
-	/* check if we have any codecs detected on bus */
-	if (bus->codec_mask == 0)
-		return NULL;
-
 	/* point to common table */
 	mach = snd_soc_acpi_intel_hda_machines;
 
-- 
2.17.1

[PATCH 3/7] ASoC: Intel: Skylake: Enable codec wakeup during chip init

[Cezary Rojewski]

Follow the recommendation set by hda_intel.c and enable HDMI/DP codec
wakeup during bus initialization procedure. Disable wakeup once init
completes.

Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/soc/intel/skylake/skl.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
index 4827fe6bc1cb..e2e531c96dd1 100644
--- a/sound/soc/intel/skylake/skl.c
+++ b/sound/soc/intel/skylake/skl.c
@@ -130,6 +130,7 @@ static int skl_init_chip(struct hdac_bus *bus, bool full_reset)
 	struct hdac_ext_link *hlink;
 	int ret;
 
+	snd_hdac_set_codec_wakeup(bus, true);
 	skl_enable_miscbdcge(bus->dev, false);
 	ret = snd_hdac_bus_init_chip(bus, full_reset);
 
@@ -138,6 +139,7 @@ static int skl_init_chip(struct hdac_bus *bus, bool full_reset)
 		writel(0, hlink->ml_addr + AZX_REG_ML_LOSIDV);
 
 	skl_enable_miscbdcge(bus->dev, true);
+	snd_hdac_set_codec_wakeup(bus, false);
 
 	return ret;
 }
-- 
2.17.1

[PATCH 4/7] ASoC: Intel: Skylake: Shield against no-NHLT configurations

[Cezary Rojewski]

Some configurations expose no NHLT table at all within their
/sys/firmware/acpi/tables. To prevent NULL-dereference errors from
occurring, adjust probe flow and append additional safety checks in
functions involved in NHLT lifecycle.

Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/soc/intel/skylake/skl-nhlt.c | 3 ++-
 sound/soc/intel/skylake/skl.c      | 9 +++++++--
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/sound/soc/intel/skylake/skl-nhlt.c b/sound/soc/intel/skylake/skl-nhlt.c
index 19f328d71f24..d9c8f5cb389e 100644
--- a/sound/soc/intel/skylake/skl-nhlt.c
+++ b/sound/soc/intel/skylake/skl-nhlt.c
@@ -182,7 +182,8 @@ void skl_nhlt_remove_sysfs(struct skl_dev *skl)
 {
 	struct device *dev = &skl->pci->dev;
 
-	sysfs_remove_file(&dev->kobj, &dev_attr_platform_id.attr);
+	if (skl->nhlt)
+		sysfs_remove_file(&dev->kobj, &dev_attr_platform_id.attr);
 }
 
 /*
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
index e2e531c96dd1..7ad8a75759bd 100644
--- a/sound/soc/intel/skylake/skl.c
+++ b/sound/soc/intel/skylake/skl.c
@@ -633,6 +633,9 @@ static int skl_clock_device_register(struct skl_dev *skl)
 	struct platform_device_info pdevinfo = {NULL};
 	struct skl_clk_pdata *clk_pdata;
 
+	if (!skl->nhlt)
+		return 0;
+
 	clk_pdata = devm_kzalloc(&skl->pci->dev, sizeof(*clk_pdata),
 							GFP_KERNEL);
 	if (!clk_pdata)
@@ -1074,7 +1077,8 @@ static int skl_probe(struct pci_dev *pci,
 out_clk_free:
 	skl_clock_device_unregister(skl);
 out_nhlt_free:
-	intel_nhlt_free(skl->nhlt);
+	if (skl->nhlt)
+		intel_nhlt_free(skl->nhlt);
 out_free:
 	skl_free(bus);
 
@@ -1123,7 +1127,8 @@ static void skl_remove(struct pci_dev *pci)
 	skl_dmic_device_unregister(skl);
 	skl_clock_device_unregister(skl);
 	skl_nhlt_remove_sysfs(skl);
-	intel_nhlt_free(skl->nhlt);
+	if (skl->nhlt)
+		intel_nhlt_free(skl->nhlt);
 	skl_free(bus);
 	dev_set_drvdata(&pci->dev, NULL);
 }
-- 
2.17.1

[PATCH 5/7] ASoC: Intel: skl_hda_dsp: Enable Dmic configuration

[Cezary Rojewski]

From: Mateusz Gorski <mateusz.gorski@linux.intel.com>

Address missing Dmic configuration for Intel DSP platforms with HDAudio
codecs by updating DAPM route map with adequate connections.

Signed-off-by: Mateusz Gorski <mateusz.gorski@linux.intel.com>
Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/soc/intel/boards/skl_hda_dsp_generic.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/sound/soc/intel/boards/skl_hda_dsp_generic.c b/sound/soc/intel/boards/skl_hda_dsp_generic.c
index fe2d3a23a4ef..0126dfc7ac24 100644
--- a/sound/soc/intel/boards/skl_hda_dsp_generic.c
+++ b/sound/soc/intel/boards/skl_hda_dsp_generic.c
@@ -59,6 +59,9 @@ static const struct snd_soc_dapm_route skl_hda_map[] = {
 	{ "Digital CPU Capture", NULL, "Digital Codec Capture" },
 	{ "codec2_in", NULL, "Alt Analog CPU Capture" },
 	{ "Alt Analog CPU Capture", NULL, "Alt Analog Codec Capture" },
+
+	{ "dmic01_hifi", NULL, "DMIC01 Rx" },
+	{ "DMIC01 Rx", NULL, "DMIC AIF" },
 };
 
 SND_SOC_DAILINK_DEF(dummy_codec,
-- 
2.17.1

[PATCH 6/7] ASoC: Intel: Allow for ROM init retry on CNL platforms

[Cezary Rojewski]

Due to unconditional initial timeouts, firmware may fail to load during
its initialization. This issue cannot be resolved on driver side as it
is caused by external sources such as CSME but has to be accounted for
nonetheless.

Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
Fixes: cb6a55284629 ("ASoC: Intel: cnl: Add sst library functions for cnl platform")
---
 sound/soc/intel/skylake/bxt-sst.c     |  2 --
 sound/soc/intel/skylake/cnl-sst.c     | 15 ++++++++++-----
 sound/soc/intel/skylake/skl-sst-dsp.h |  1 +
 3 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/sound/soc/intel/skylake/bxt-sst.c b/sound/soc/intel/skylake/bxt-sst.c
index 92a82e6b5fe6..cdafade8abd6 100644
--- a/sound/soc/intel/skylake/bxt-sst.c
+++ b/sound/soc/intel/skylake/bxt-sst.c
@@ -38,8 +38,6 @@
 /* Delay before scheduling D0i3 entry */
 #define BXT_D0I3_DELAY 5000
 
-#define BXT_FW_ROM_INIT_RETRY 3
-
 static unsigned int bxt_get_errorcode(struct sst_dsp *ctx)
 {
 	 return sst_dsp_shim_read(ctx, BXT_ADSP_ERROR_CODE);
diff --git a/sound/soc/intel/skylake/cnl-sst.c b/sound/soc/intel/skylake/cnl-sst.c
index 4f64f097e9ae..060e47ae3391 100644
--- a/sound/soc/intel/skylake/cnl-sst.c
+++ b/sound/soc/intel/skylake/cnl-sst.c
@@ -109,7 +109,7 @@ static int cnl_load_base_firmware(struct sst_dsp *ctx)
 {
 	struct firmware stripped_fw;
 	struct skl_dev *cnl = ctx->thread_context;
-	int ret;
+	int ret, i;
 
 	if (!ctx->fw) {
 		ret = request_firmware(&ctx->fw, ctx->fw_name, ctx->dev);
@@ -131,12 +131,16 @@ static int cnl_load_base_firmware(struct sst_dsp *ctx)
 	stripped_fw.size = ctx->fw->size;
 	skl_dsp_strip_extended_manifest(&stripped_fw);
 
-	ret = cnl_prepare_fw(ctx, stripped_fw.data, stripped_fw.size);
-	if (ret < 0) {
-		dev_err(ctx->dev, "prepare firmware failed: %d\n", ret);
-		goto cnl_load_base_firmware_failed;
+	for (i = 0; i < BXT_FW_ROM_INIT_RETRY; i++) {
+		ret = cnl_prepare_fw(ctx, stripped_fw.data, stripped_fw.size);
+		if (!ret)
+			break;
+		dev_dbg(ctx->dev, "prepare firmware failed: %d\n", ret);
 	}
 
+	if (ret < 0)
+		goto cnl_load_base_firmware_failed;
+
 	ret = sst_transfer_fw_host_dma(ctx);
 	if (ret < 0) {
 		dev_err(ctx->dev, "transfer firmware failed: %d\n", ret);
@@ -158,6 +162,7 @@ static int cnl_load_base_firmware(struct sst_dsp *ctx)
 	return 0;
 
 cnl_load_base_firmware_failed:
+	dev_err(ctx->dev, "firmware load failed: %d\n", ret);
 	release_firmware(ctx->fw);
 	ctx->fw = NULL;
 
diff --git a/sound/soc/intel/skylake/skl-sst-dsp.h b/sound/soc/intel/skylake/skl-sst-dsp.h
index cdfec0fca577..067d1ea11cde 100644
--- a/sound/soc/intel/skylake/skl-sst-dsp.h
+++ b/sound/soc/intel/skylake/skl-sst-dsp.h
@@ -67,6 +67,7 @@ struct skl_dev;
 
 #define SKL_FW_INIT			0x1
 #define SKL_FW_RFW_START		0xf
+#define BXT_FW_ROM_INIT_RETRY		3
 
 #define SKL_ADSPIC_IPC			1
 #define SKL_ADSPIS_IPC			1
-- 
2.17.1

[PATCH 7/7] ASoC: Intel: Skylake: Await purge request ack on CNL

[Cezary Rojewski]

Each purge request is sent by driver after master core is powered up and
unresetted but before it is unstalled. On unstall, ROM begins processing
the request and initializing environment for FW load. Host should await
ROM's ack before moving forward. Without doing so, ROM init poll may
start too early and false timeouts can occur.

Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
Fixes: cb6a55284629 ("ASoC: Intel: cnl: Add sst library functions for cnl platform")
---
 sound/soc/intel/skylake/bxt-sst.c     |  1 -
 sound/soc/intel/skylake/cnl-sst.c     | 20 ++++++++++++++++++--
 sound/soc/intel/skylake/skl-sst-dsp.h |  1 +
 3 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/sound/soc/intel/skylake/bxt-sst.c b/sound/soc/intel/skylake/bxt-sst.c
index cdafade8abd6..38b9d7494083 100644
--- a/sound/soc/intel/skylake/bxt-sst.c
+++ b/sound/soc/intel/skylake/bxt-sst.c
@@ -17,7 +17,6 @@
 #include "skl.h"
 
 #define BXT_BASEFW_TIMEOUT	3000
-#define BXT_INIT_TIMEOUT	300
 #define BXT_ROM_INIT_TIMEOUT	70
 #define BXT_IPC_PURGE_FW	0x01004000
 
diff --git a/sound/soc/intel/skylake/cnl-sst.c b/sound/soc/intel/skylake/cnl-sst.c
index 060e47ae3391..c6abcd5aa67b 100644
--- a/sound/soc/intel/skylake/cnl-sst.c
+++ b/sound/soc/intel/skylake/cnl-sst.c
@@ -57,18 +57,34 @@ static int cnl_prepare_fw(struct sst_dsp *ctx, const void *fwdata, u32 fwsize)
 	ctx->dsp_ops.stream_tag = stream_tag;
 	memcpy(ctx->dmab.area, fwdata, fwsize);
 
+	ret = skl_dsp_core_power_up(ctx, SKL_DSP_CORE0_MASK);
+	if (ret < 0) {
+		dev_err(ctx->dev, "dsp core0 power up failed\n");
+		ret = -EIO;
+		goto base_fw_load_failed;
+	}
+
 	/* purge FW request */
 	sst_dsp_shim_write(ctx, CNL_ADSP_REG_HIPCIDR,
 			   CNL_ADSP_REG_HIPCIDR_BUSY | (CNL_IPC_PURGE |
 			   ((stream_tag - 1) << CNL_ROM_CTRL_DMA_ID)));
 
-	ret = cnl_dsp_enable_core(ctx, SKL_DSP_CORE0_MASK);
+	ret = skl_dsp_start_core(ctx, SKL_DSP_CORE0_MASK);
 	if (ret < 0) {
-		dev_err(ctx->dev, "dsp boot core failed ret: %d\n", ret);
+		dev_err(ctx->dev, "Start dsp core failed ret: %d\n", ret);
 		ret = -EIO;
 		goto base_fw_load_failed;
 	}
 
+	ret = sst_dsp_register_poll(ctx, CNL_ADSP_REG_HIPCIDA,
+				    CNL_ADSP_REG_HIPCIDA_DONE,
+				    CNL_ADSP_REG_HIPCIDA_DONE,
+				    BXT_INIT_TIMEOUT, "HIPCIDA Done");
+	if (ret < 0) {
+		dev_err(ctx->dev, "timeout for purge request: %d\n", ret);
+		goto base_fw_load_failed;
+	}
+
 	/* enable interrupt */
 	cnl_ipc_int_enable(ctx);
 	cnl_ipc_op_int_enable(ctx);
diff --git a/sound/soc/intel/skylake/skl-sst-dsp.h b/sound/soc/intel/skylake/skl-sst-dsp.h
index 067d1ea11cde..1df9ef422f61 100644
--- a/sound/soc/intel/skylake/skl-sst-dsp.h
+++ b/sound/soc/intel/skylake/skl-sst-dsp.h
@@ -68,6 +68,7 @@ struct skl_dev;
 #define SKL_FW_INIT			0x1
 #define SKL_FW_RFW_START		0xf
 #define BXT_FW_ROM_INIT_RETRY		3
+#define BXT_INIT_TIMEOUT		300
 
 #define SKL_ADSPIC_IPC			1
 #define SKL_ADSPIS_IPC			1
-- 
2.17.1

Re: [PATCH 5/7] ASoC: Intel: skl_hda_dsp: Enable Dmic configuration

[Kai Vehmanen]

Hey,

On Thu, 5 Mar 2020, Cezary Rojewski wrote:

> --- a/sound/soc/intel/boards/skl_hda_dsp_generic.c
> +++ b/sound/soc/intel/boards/skl_hda_dsp_generic.c
> @@ -59,6 +59,9 @@ static const struct snd_soc_dapm_route skl_hda_map[] = {
>  	{ "Digital CPU Capture", NULL, "Digital Codec Capture" },
>  	{ "codec2_in", NULL, "Alt Analog CPU Capture" },
>  	{ "Alt Analog CPU Capture", NULL, "Alt Analog Codec Capture" },
> +
> +	{ "dmic01_hifi", NULL, "DMIC01 Rx" },
> +	{ "DMIC01 Rx", NULL, "DMIC AIF" },

hmm, we need to figure out something else for this. This very same table 
already has:

»       /* digital mics */
»       {"DMic", NULL, "SoC DMIC"},

.. so now we have dmic entries two times in the same initializer list.

But a more pressing issue is that this breaks platforms using SOF 
firmware:

[   28.751756] skl_hda_dsp_generic skl_hda_dsp_generic: ASoC: no sink widget found for dmic01_hifi
[   28.751987] skl_hda_dsp_generic skl_hda_dsp_generic: ASoC: Failed to add route DMIC01 Rx -> direct -> dmic01_hifi

... maybe you can align the topology to mathc so we can reuse the same 
widget mapping for both SOF and SST firmwares..?

Br, Kai

Re: [PATCH 5/7] ASoC: Intel: skl_hda_dsp: Enable Dmic configuration

[Pierre-Louis Bossart]

> But a more pressing issue is that this breaks platforms using SOF
> firmware:
> 
> [   28.751756] skl_hda_dsp_generic skl_hda_dsp_generic: ASoC: no sink widget found for dmic01_hifi
> [   28.751987] skl_hda_dsp_generic skl_hda_dsp_generic: ASoC: Failed to add route DMIC01 Rx -> direct -> dmic01_hifi
> 
> ... maybe you can align the topology to mathc so we can reuse the same
> widget mapping for both SOF and SST firmwares..?

Yeah, I thought this would break userspace and installed topologies and 
that just confirms it. Adding hard-coded routes is really not recommended.

the alternate solution is what I suggested in another thread "No sound 
since 5.4 on skl_n88l25_s4567", we could mark this machine driver as 
having an incomplete topology and remove the topology checks in the core.

I couldn't really test my initial patch but that that Cezary 
unintentionally broke SOF actually that gives me a tool to test the 
solution.

Re: [PATCH 5/7] ASoC: Intel: skl_hda_dsp: Enable Dmic configuration

[Cezary Rojewski]

On 2020-03-06 16:49, Pierre-Louis Bossart wrote:
>> But a more pressing issue is that this breaks platforms using SOF
>> firmware:
>>
>> [   28.751756] skl_hda_dsp_generic skl_hda_dsp_generic: ASoC: no sink 
>> widget found for dmic01_hifi
>> [   28.751987] skl_hda_dsp_generic skl_hda_dsp_generic: ASoC: Failed 
>> to add route DMIC01 Rx -> direct -> dmic01_hifi
>>
>> ... maybe you can align the topology to mathc so we can reuse the same
>> widget mapping for both SOF and SST firmwares..?
> 
> Yeah, I thought this would break userspace and installed topologies and 
> that just confirms it. Adding hard-coded routes is really not recommended.
> 
> the alternate solution is what I suggested in another thread "No sound 
> since 5.4 on skl_n88l25_s4567", we could mark this machine driver as 
> having an incomplete topology and remove the topology checks in the core.
> 
> I couldn't really test my initial patch but that that Cezary 
> unintentionally broke SOF actually that gives me a tool to test the 
> solution.

Glad to help Pierre ^)^ and thanks for the review Kai.

Guys, we've simply taken long-standing working example such as skl_rt286 
or bxt_rt298 and applied the missing diff between skl_hda_dsp's and said 
machine boards DAPM routes. skl-pcm.c exposes BE: DMIC01 Rx which 
Intel's SST topologies link against via dmic01_hifi. That has always 
been the case. No bad intentions, the exact opposite is true: taken old 
path approach to make sure nothing is broken. Turns out SOF does things 
differently. Thanks for spotting/ testing this out on your end.

Not a problem to adjust topology on our end, though. In fact, I've 
already done that on your requested, tested it out and it works just 
fine. In consequence:
- this patch will be dropped from the series
- topology patch provided for alsa-ucm-conf will be updated accordingly 
(dmic01_hifi widget will cease to exist)

Czarek

Re: [PATCH 5/7] ASoC: Intel: skl_hda_dsp: Enable Dmic configuration

[Pierre-Louis Bossart]

> Guys, we've simply taken long-standing working example such as skl_rt286 
> or bxt_rt298 and applied the missing diff between skl_hda_dsp's and said 
> machine boards DAPM routes. skl-pcm.c exposes BE: DMIC01 Rx which 
> Intel's SST topologies link against via dmic01_hifi. That has always 
> been the case. No bad intentions, the exact opposite is true: taken old 
> path approach to make sure nothing is broken. Turns out SOF does things 
> differently. Thanks for spotting/ testing this out on your end.

It's actually not SOF, but recent changes in the asoc core that will 
stop the probe if a route cannot be added, instead of just spewing a 
warning.

I think it's a good change to force topologies to be complete, but in 
cases where a previously released topology cannot be adjusted we need a 
backwards compatible solution. that's my plan for the rest of the afternoon.

> Not a problem to adjust topology on our end, though. In fact, I've 
> already done that on your requested, tested it out and it works just 
> fine. In consequence:
> - this patch will be dropped from the series
> - topology patch provided for alsa-ucm-conf will be updated accordingly 
> (dmic01_hifi widget will cease to exist)

Sounds good, thanks Cezary.

Re: [PATCH 5/7] ASoC: Intel: skl_hda_dsp: Enable Dmic configuration

[Cezary Rojewski]

On 2020-03-06 20:49, Pierre-Louis Bossart wrote:
>> Guys, we've simply taken long-standing working example such as 
>> skl_rt286 or bxt_rt298 and applied the missing diff between 
>> skl_hda_dsp's and said machine boards DAPM routes. skl-pcm.c exposes 
>> BE: DMIC01 Rx which Intel's SST topologies link against via 
>> dmic01_hifi. That has always been the case. No bad intentions, the 
>> exact opposite is true: taken old path approach to make sure nothing 
>> is broken. Turns out SOF does things differently. Thanks for spotting/ 
>> testing this out on your end.
> 
> It's actually not SOF, but recent changes in the asoc core that will 
> stop the probe if a route cannot be added, instead of just spewing a 
> warning.
> 
> I think it's a good change to force topologies to be complete, but in 
> cases where a previously released topology cannot be adjusted we need a 
> backwards compatible solution. that's my plan for the rest of the 
> afternoon.

Agree, missing topology routes should not be permissive. Although my 
point was about the fact that those 2 routes actually exist in every SST 
machine driver (in essence linking DMIC01 Rx with platform via internal 
dmic01_hifi widget).

>> Not a problem to adjust topology on our end, though. In fact, I've 
>> already done that on your requested, tested it out and it works just 
>> fine. In consequence:
>> - this patch will be dropped from the series
>> - topology patch provided for alsa-ucm-conf will be updated 
>> accordingly (dmic01_hifi widget will cease to exist)
> 
> Sounds good, thanks Cezary.

Awaiting comments for the rest of the patches if any!
Especially how low - kernel version wise - should the fixes be backported.

Thanks in advance for your input and time.

Czarek

Re: [PATCH 0/7] ASoC: Intel: Skylake: Fix HDaudio and Dmic

[Pierre-Louis Bossart]

Hi Cezary,

Thank you for this series. You should have mentioned that this fixes an 
issue that's been around since September 2018, and for which there's 
currently no solution on KabyLake/AmberLake platforms - 
PCI_DEVICE(0x8086, 0x9D71)

https://bugzilla.kernel.org/show_bug.cgi?id=201251

> This patchset has been prepared internally for topmost linux-stable 5.5
> and 4.20 (no 4.19 as skl_hda_dsp did not exist there yet).
> 
> Apart from our RVPs, we have run tests also on:
> - KBL Lenovo Carbon X1
> - SKL Dell XPS 9350
> - WHL Acer Swift 5
> 
> Honestly, I'd see HDaudio related patches being backport as low as 4.20
> (although some changes had to be adjusted due to base differences
> between 4.20 and 5.5, can share these too). One could argue HDA + Dmic
> configuration should be available on 4.19 too - it's an LTS after all.
> However, that time, some changes could be counted as "feature" rather
> than fixes. Awaiting your replies and thoughts on that.
> 
> In consequence, I've appended "Fixes" only for last two patches for now
> - once decisions are made, can append adequate tags wherever necessary.

There's been a couple of accidental regressions already on stable, now 
fixed, and my understanding is that the bar for inclusion is higher, so 
let's assume this counts as a new feature: it never worked before with 
an upstream kernel and distributions haven't had access to topology 
files either. If a specific distribution wants to backport on top of a 
-stable version, that's entirely possible but that would be their 
decision. They would not only need to update the kernel but topology and 
UCM as well.

One comment though: in the absence of blacklist/voodoo magic, this 
solution will not be selected with GLK+/WHL, the default with dmics is 
SOF. Even on KBL, the legacy driver would be selected, we only select 
the SST driver for SKL and KBL Chromebooks.

You would have to modify the dsp-config stuff [1] to load the skl driver 
on KBL, which likely requires a FLAG_SST_ONLY_IF_DMIC definition, and 
probably add a Kconfig since SOF will at some point support KBL, and we 
want to prevent conflicts between PCI drivers registered for the same 
ID. The order in that table might be enough though, it's much better 
than the selection based on a Makefile inclusion.

Patches 6/7 don't seem to be related to DMICs, so we should probably 
discuss them separately.

Thanks
-Pierre

[1] 
https://elixir.bootlin.com/linux/latest/source/sound/hda/intel-dsp-config.c#L118

Re: [PATCH 1/7] ASoC: Intel: Skylake: Remove superfluous chip initialization

[Pierre-Louis Bossart]

On 3/5/20 8:53 AM, Cezary Rojewski wrote:
> Skylake driver does the controller init operation twice:
> - first during probe (only to stop it just before scheduling probe_work)
> - and during said probe_work where the actual correct sequence is
> executed
> 
> To properly complete boot sequence when iDisp codec is present, bus
> initialization has to be called only after _i915_init() finishes.
> With additional _reset_list preceding _i915_init(), iDisp codec never
> gets the chance to enumerate on the link. Remove the superfluous
> initialization to address the issue.

Have you tested with with DRM built-in and as a module? that was enough 
to trigger race conditions in the past on Dell XPS9350.

Re: [PATCH 2/7] ASoC: Intel: Skylake: Select hda configuration permissively

[Pierre-Louis Bossart]

On 3/5/20 8:53 AM, Cezary Rojewski wrote:
> With _reset_link removed from the probe sequence, codec_mask at the time
> skl_find_hda_machine() is invoked will always be 0, so hda machine will
> never be chosen. Rather than reorganizing boot flow, be permissive about
> invalid mask. codec_mask will be set to proper value during probe_work -
> before skl_codec_create() ever gets called.

humm, what would happen e.g. if you have select the SKL driver but there 
is no ACPI information to select an I2S-based machine driver, and 
HDaudio/iDISP are disabled? You would have no error checks then?


> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
> ---
>   sound/soc/intel/skylake/skl.c | 5 -----
>   1 file changed, 5 deletions(-)
> 
> diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
> index d66231525356..4827fe6bc1cb 100644
> --- a/sound/soc/intel/skylake/skl.c
> +++ b/sound/soc/intel/skylake/skl.c
> @@ -481,13 +481,8 @@ static struct skl_ssp_clk skl_ssp_clks[] = {
>   static struct snd_soc_acpi_mach *skl_find_hda_machine(struct skl_dev *skl,
>   					struct snd_soc_acpi_mach *machines)
>   {
> -	struct hdac_bus *bus = skl_to_bus(skl);
>   	struct snd_soc_acpi_mach *mach;
>   
> -	/* check if we have any codecs detected on bus */
> -	if (bus->codec_mask == 0)
> -		return NULL;
> -
>   	/* point to common table */
>   	mach = snd_soc_acpi_intel_hda_machines;
>   
> 

Re: [PATCH 4/7] ASoC: Intel: Skylake: Shield against no-NHLT configurations

[Pierre-Louis Bossart]

> -	intel_nhlt_free(skl->nhlt);
> +	if (skl->nhlt)
> +		intel_nhlt_free(skl->nhlt);

we could alternatively move the test in intel_nhlt_free, which seems 
like a more robust thing to do?

Re: [PATCH 0/7] ASoC: Intel: Skylake: Fix HDaudio and Dmic

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 545 bytes --]

On Fri, Mar 06, 2020 at 02:48:39PM -0600, Pierre-Louis Bossart wrote:

> There's been a couple of accidental regressions already on stable, now
> fixed, and my understanding is that the bar for inclusion is higher, so
> let's assume this counts as a new feature: it never worked before with an

There's been no change in the requirements for stable, those regressions
have come from patches that have been pulled in automatically by the bot
Sasha runs which doesn't pay attention to the requirements but tries to
identify patches automatically.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 4/7] ASoC: Intel: Skylake: Shield against no-NHLT configurations

[Cezary Rojewski]

On 2020-03-06 22:03, Pierre-Louis Bossart wrote:
> 
> 
>> -    intel_nhlt_free(skl->nhlt);
>> +    if (skl->nhlt)
>> +        intel_nhlt_free(skl->nhlt);
> 
> we could alternatively move the test in intel_nhlt_free, which seems 
> like a more robust thing to do?

Depends. In general kernel-internal API trusts its caller and appending 
'ifs' everywhere would unnecessarily slow entire kernel down. While 
intel_nhlt_free is called rarely, I'd still argue caller should be sane 
about its invocation.

'if' in skl_probe could be avoided had the function's structure been 
better. 'if' in skl_remove is just fine, though.

Let's leave it as is.

Czarek

Re: [PATCH 2/7] ASoC: Intel: Skylake: Select hda configuration permissively

[Cezary Rojewski]

On 2020-03-06 21:57, Pierre-Louis Bossart wrote:
> On 3/5/20 8:53 AM, Cezary Rojewski wrote:
>> With _reset_link removed from the probe sequence, codec_mask at the time
>> skl_find_hda_machine() is invoked will always be 0, so hda machine will
>> never be chosen. Rather than reorganizing boot flow, be permissive about
>> invalid mask. codec_mask will be set to proper value during probe_work -
>> before skl_codec_create() ever gets called.
> 
> humm, what would happen e.g. if you have select the SKL driver but there 
> is no ACPI information to select an I2S-based machine driver, and 
> HDaudio/iDISP are disabled? You would have no error checks then?
> 

Laptops I've been testing this with have had Realtek + iDisp present 
onboard. Now, if you disable Realtek + HDMI/DP modules within legacy 
HDaudio Kconfig and HD audio support within Intel Skylake tree then you 
end up with no required modules for said configuration at all. Nothing 
will happen really: no warnings, no sound card either.

Just run such kernel on my setup and results are quite obvious:
- skl boots
- no machines present
- drv stays dormant

Czarek

Re: [PATCH 1/7] ASoC: Intel: Skylake: Remove superfluous chip initialization

[Cezary Rojewski]

On 2020-03-06 21:52, Pierre-Louis Bossart wrote:
> On 3/5/20 8:53 AM, Cezary Rojewski wrote:
>> Skylake driver does the controller init operation twice:
>> - first during probe (only to stop it just before scheduling probe_work)
>> - and during said probe_work where the actual correct sequence is
>> executed
>>
>> To properly complete boot sequence when iDisp codec is present, bus
>> initialization has to be called only after _i915_init() finishes.
>> With additional _reset_list preceding _i915_init(), iDisp codec never
>> gets the chance to enumerate on the link. Remove the superfluous
>> initialization to address the issue.
> 
> Have you tested with with DRM built-in and as a module? that was enough 
> to trigger race conditions in the past on Dell XPS9350.
> 

DRM is quite a tree, you got to be more specific. Tested with i915=m and 
DRM=m. I hope we mean the same thing when mentioning 'race'. There is an 
obvious initialization race between hda bus drv and i915 which requires 
one to follow a tight operation order in order to not lose i915 codec on 
hda link and thus be able to enumerate it properly.

On top of that, as you mentioned (by the link) this series addresses 
missing DMIC configuration in conjunction with HDA +/- iDisp AND shields 
against no-NHLT configuration. On Dell XPS 9350 lack on of NHLT was the 
biggest problem - that's why I'd like that issue not to be forgotten about.

Czarek

Re: [PATCH 0/7] ASoC: Intel: Skylake: Fix HDaudio and Dmic

[Cezary Rojewski]

On 2020-03-09 12:38, Mark Brown wrote:
> On Fri, Mar 06, 2020 at 02:48:39PM -0600, Pierre-Louis Bossart wrote:
> 
>> There's been a couple of accidental regressions already on stable, now
>> fixed, and my understanding is that the bar for inclusion is higher, so
>> let's assume this counts as a new feature: it never worked before with an
> 
> There's been no change in the requirements for stable, those regressions
> have come from patches that have been pulled in automatically by the bot
> Sasha runs which doesn't pay attention to the requirements but tries to
> identify patches automatically.
> 

Mark, what's your take on backporting these to 4.19 LTS? Do we abandon 
the subject and "just" merge (once reviewed/ approved) into 5.5? I 
believe addressing all the issues mentioned on 4.19 is important.

Re: [PATCH 1/7] ASoC: Intel: Skylake: Remove superfluous chip initialization

[Pierre-Louis Bossart]

On 3/9/20 8:57 AM, Cezary Rojewski wrote:
> On 2020-03-06 21:52, Pierre-Louis Bossart wrote:
>> On 3/5/20 8:53 AM, Cezary Rojewski wrote:
>>> Skylake driver does the controller init operation twice:
>>> - first during probe (only to stop it just before scheduling probe_work)
>>> - and during said probe_work where the actual correct sequence is
>>> executed
>>>
>>> To properly complete boot sequence when iDisp codec is present, bus
>>> initialization has to be called only after _i915_init() finishes.
>>> With additional _reset_list preceding _i915_init(), iDisp codec never
>>> gets the chance to enumerate on the link. Remove the superfluous
>>> initialization to address the issue.
>>
>> Have you tested with with DRM built-in and as a module? that was 
>> enough to trigger race conditions in the past on Dell XPS9350.
>>
> 
> DRM is quite a tree, you got to be more specific. Tested with i915=m and 
> DRM=m. I hope we mean the same thing when mentioning 'race'. There is an 
> obvious initialization race between hda bus drv and i915 which requires 
> one to follow a tight operation order in order to not lose i915 codec on 
> hda link and thus be able to enumerate it properly.

I meant CONFIG_DRM=m, yes, thanks for the clarification.

With the DRM as module, it took more time to establish the 
communication. That's probably changed if we do all the inits in a 
workqueue now.

> 
> On top of that, as you mentioned (by the link) this series addresses 
> missing DMIC configuration in conjunction with HDA +/- iDisp AND shields 
> against no-NHLT configuration. On Dell XPS 9350 lack on of NHLT was the 
> biggest problem - that's why I'd like that issue not to be forgotten about.

yes, but we don't want the driver to be auto-selected on SKL w/o DMICs, 
since it'd break existing devices who don't have a topology file installed.

Re: [PATCH 0/7] ASoC: Intel: Skylake: Fix HDaudio and Dmic

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 578 bytes --]

On Mon, Mar 09, 2020 at 03:02:25PM +0100, Cezary Rojewski wrote:

> Mark, what's your take on backporting these to 4.19 LTS? Do we abandon the
> subject and "just" merge (once reviewed/ approved) into 5.5? I believe
> addressing all the issues mentioned on 4.19 is important.

I didn't actually look at the patches since by the time I went to look
at them it was clear that there was going to be a new version.  Pierre
was saying that they added new functionality which would generally not
be suitable.

See Documentation/process/stable-kernel-rules.rst for the official
rules.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 4/7] ASoC: Intel: Skylake: Shield against no-NHLT configurations

[Pierre-Louis Bossart]

On 3/9/20 8:03 AM, Cezary Rojewski wrote:
> On 2020-03-06 22:03, Pierre-Louis Bossart wrote:
>>
>>
>>> -    intel_nhlt_free(skl->nhlt);
>>> +    if (skl->nhlt)
>>> +        intel_nhlt_free(skl->nhlt);
>>
>> we could alternatively move the test in intel_nhlt_free, which seems 
>> like a more robust thing to do?
> 
> Depends. In general kernel-internal API trusts its caller and appending 
> 'ifs' everywhere would unnecessarily slow entire kernel down. While 
> intel_nhlt_free is called rarely, I'd still argue caller should be sane 
> about its invocation.
> 
> 'if' in skl_probe could be avoided had the function's structure been 
> better. 'if' in skl_remove is just fine, though.
> 
> Let's leave it as is.

it's also used in SOF:

sound/soc/sof/intel/hda.c:              intel_nhlt_free(nhlt);

that's why I suggested to factor the test so that both users don't need 
to add the if.

Re: [PATCH 2/7] ASoC: Intel: Skylake: Select hda configuration permissively

[Pierre-Louis Bossart]

On 3/9/20 8:47 AM, Cezary Rojewski wrote:
> On 2020-03-06 21:57, Pierre-Louis Bossart wrote:
>> On 3/5/20 8:53 AM, Cezary Rojewski wrote:
>>> With _reset_link removed from the probe sequence, codec_mask at the time
>>> skl_find_hda_machine() is invoked will always be 0, so hda machine will
>>> never be chosen. Rather than reorganizing boot flow, be permissive about
>>> invalid mask. codec_mask will be set to proper value during probe_work -
>>> before skl_codec_create() ever gets called.
>>
>> humm, what would happen e.g. if you have select the SKL driver but 
>> there is no ACPI information to select an I2S-based machine driver, 
>> and HDaudio/iDISP are disabled? You would have no error checks then?
>>
> 
> Laptops I've been testing this with have had Realtek + iDisp present 
> onboard. Now, if you disable Realtek + HDMI/DP modules within legacy 
> HDaudio Kconfig and HD audio support within Intel Skylake tree then you 
> end up with no required modules for said configuration at all. Nothing 
> will happen really: no warnings, no sound card either.

I meant enable the HDaudio controller but disable HDaudio codecs/HDMI at 
the BIOS level. In that case the codec_mask will never be set.

> Just run such kernel on my setup and results are quite obvious:
> - skl boots
> - no machines present
> - drv stays dormant
> 
> Czarek

Re: [PATCH 4/7] ASoC: Intel: Skylake: Shield against no-NHLT configurations

[Cezary Rojewski]

On 2020-03-09 18:01, Pierre-Louis Bossart wrote:
> On 3/9/20 8:03 AM, Cezary Rojewski wrote:
>> On 2020-03-06 22:03, Pierre-Louis Bossart wrote:
>>>
>>>
>>>> -    intel_nhlt_free(skl->nhlt);
>>>> +    if (skl->nhlt)
>>>> +        intel_nhlt_free(skl->nhlt);
>>>
>>> we could alternatively move the test in intel_nhlt_free, which seems 
>>> like a more robust thing to do?
>>
>> Depends. In general kernel-internal API trusts its caller and 
>> appending 'ifs' everywhere would unnecessarily slow entire kernel 
>> down. While intel_nhlt_free is called rarely, I'd still argue caller 
>> should be sane about its invocation.
>>
>> 'if' in skl_probe could be avoided had the function's structure been 
>> better. 'if' in skl_remove is just fine, though.
>>
>> Let's leave it as is.
> 
> it's also used in SOF:
> 
> sound/soc/sof/intel/hda.c:              intel_nhlt_free(nhlt);
> 
> that's why I suggested to factor the test so that both users don't need 
> to add the if.

I understand, and my explanation still applies.

SOF's intel_nhlt_free usage is great example actually. Caller is sane 
about its doings as it should be. Internal API needs not to suffer from 
callers irresponsibility.

PCM does not call ::free() if ::open() fails, same as device-driver 
model does not care about ::remove() if ::probe() fails.

Czarek

Re: [PATCH 1/7] ASoC: Intel: Skylake: Remove superfluous chip initialization

[Cezary Rojewski]

On 2020-03-09 17:48, Pierre-Louis Bossart wrote:
>> DRM is quite a tree, you got to be more specific. Tested with i915=m 
>> and DRM=m. I hope we mean the same thing when mentioning 'race'. There 
>> is an obvious initialization race between hda bus drv and i915 which 
>> requires one to follow a tight operation order in order to not lose 
>> i915 codec on hda link and thus be able to enumerate it properly.
> 
> I meant CONFIG_DRM=m, yes, thanks for the clarification.
> 
> With the DRM as module, it took more time to establish the 
> communication. That's probably changed if we do all the inits in a 
> workqueue now.
> 

So, does the DRM=m & i915=m test satisfy you?

>>
>> On top of that, as you mentioned (by the link) this series addresses 
>> missing DMIC configuration in conjunction with HDA +/- iDisp AND 
>> shields against no-NHLT configuration. On Dell XPS 9350 lack on of 
>> NHLT was the biggest problem - that's why I'd like that issue not to 
>> be forgotten about.
> 
> yes, but we don't want the driver to be auto-selected on SKL w/o DMICs, 
> since it'd break existing devices who don't have a topology file installed.

My patch does not touch any of kconfig stuff. I'd prefer anything that 
goes into disable/enable kconfig bucket to be split into a separate 
patch. Don't believe SKL somehow gets automatically selected on these. 
On the machines I've tested fixes with, legacy HDaudio driver was the 
primary option/ drv.

Re: [PATCH 0/7] ASoC: Intel: Skylake: Fix HDaudio and Dmic

[Cezary Rojewski]

On 2020-03-09 17:54, Mark Brown wrote:
> On Mon, Mar 09, 2020 at 03:02:25PM +0100, Cezary Rojewski wrote:
> 
>> Mark, what's your take on backporting these to 4.19 LTS? Do we abandon the
>> subject and "just" merge (once reviewed/ approved) into 5.5? I believe
>> addressing all the issues mentioned on 4.19 is important.
> 
> I didn't actually look at the patches since by the time I went to look
> at them it was clear that there was going to be a new version.  Pierre
> was saying that they added new functionality which would generally not
> be suitable.
> 
> See Documentation/process/stable-kernel-rules.rst for the official
> rules.
> 

Ok, sure. Should the 'Fixes' be appended regardless or leave it as is?

Re: [PATCH 0/7] ASoC: Intel: Skylake: Fix HDaudio and Dmic

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 463 bytes --]

On Mon, Mar 09, 2020 at 06:48:35PM +0100, Cezary Rojewski wrote:
> On 2020-03-09 17:54, Mark Brown wrote:

> > I didn't actually look at the patches since by the time I went to look
> > at them it was clear that there was going to be a new version.  Pierre
> > was saying that they added new functionality which would generally not
> > be suitable.

> Ok, sure. Should the 'Fixes' be appended regardless or leave it as is?

There's never any harm in adding them.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 4/7] ASoC: Intel: Skylake: Shield against no-NHLT configurations

[Pierre-Louis Bossart]

On 3/9/20 12:38 PM, Cezary Rojewski wrote:
> On 2020-03-09 18:01, Pierre-Louis Bossart wrote:
>> On 3/9/20 8:03 AM, Cezary Rojewski wrote:
>>> On 2020-03-06 22:03, Pierre-Louis Bossart wrote:
>>>>
>>>>
>>>>> -    intel_nhlt_free(skl->nhlt);
>>>>> +    if (skl->nhlt)
>>>>> +        intel_nhlt_free(skl->nhlt);
>>>>
>>>> we could alternatively move the test in intel_nhlt_free, which seems 
>>>> like a more robust thing to do?
>>>
>>> Depends. In general kernel-internal API trusts its caller and 
>>> appending 'ifs' everywhere would unnecessarily slow entire kernel 
>>> down. While intel_nhlt_free is called rarely, I'd still argue caller 
>>> should be sane about its invocation.
>>>
>>> 'if' in skl_probe could be avoided had the function's structure been 
>>> better. 'if' in skl_remove is just fine, though.
>>>
>>> Let's leave it as is.
>>
>> it's also used in SOF:
>>
>> sound/soc/sof/intel/hda.c:              intel_nhlt_free(nhlt);
>>
>> that's why I suggested to factor the test so that both users don't 
>> need to add the if.
> 
> I understand, and my explanation still applies.
> 
> SOF's intel_nhlt_free usage is great example actually. Caller is sane 
> about its doings as it should be. Internal API needs not to suffer from 
> callers irresponsibility.

ok, indeed looking at the code there's no need for the test.

Re: [PATCH 1/7] ASoC: Intel: Skylake: Remove superfluous chip initialization

[Pierre-Louis Bossart]

On 3/9/20 12:43 PM, Cezary Rojewski wrote:
> On 2020-03-09 17:48, Pierre-Louis Bossart wrote:
>>> DRM is quite a tree, you got to be more specific. Tested with i915=m 
>>> and DRM=m. I hope we mean the same thing when mentioning 'race'. 
>>> There is an obvious initialization race between hda bus drv and i915 
>>> which requires one to follow a tight operation order in order to not 
>>> lose i915 codec on hda link and thus be able to enumerate it properly.
>>
>> I meant CONFIG_DRM=m, yes, thanks for the clarification.
>>
>> With the DRM as module, it took more time to establish the 
>> communication. That's probably changed if we do all the inits in a 
>> workqueue now.
>>
> 
> So, does the DRM=m & i915=m test satisfy you?

Yes, that's all I wanted to confirm.

>>>
>>> On top of that, as you mentioned (by the link) this series addresses 
>>> missing DMIC configuration in conjunction with HDA +/- iDisp AND 
>>> shields against no-NHLT configuration. On Dell XPS 9350 lack on of 
>>> NHLT was the biggest problem - that's why I'd like that issue not to 
>>> be forgotten about.
>>
>> yes, but we don't want the driver to be auto-selected on SKL w/o 
>> DMICs, since it'd break existing devices who don't have a topology 
>> file installed.
> 
> My patch does not touch any of kconfig stuff. I'd prefer anything that 
> goes into disable/enable kconfig bucket to be split into a separate 
> patch. Don't believe SKL somehow gets automatically selected on these. 
> On the machines I've tested fixes with, legacy HDaudio driver was the 
> primary option/ drv.

Re: [PATCH 2/7] ASoC: Intel: Skylake: Select hda configuration permissively

[Cezary Rojewski]

On 2020-03-09 18:03, Pierre-Louis Bossart wrote:
> On 3/9/20 8:47 AM, Cezary Rojewski wrote:
>> On 2020-03-06 21:57, Pierre-Louis Bossart wrote:
>>> On 3/5/20 8:53 AM, Cezary Rojewski wrote:
>>>> With _reset_link removed from the probe sequence, codec_mask at the 
>>>> time
>>>> skl_find_hda_machine() is invoked will always be 0, so hda machine will
>>>> never be chosen. Rather than reorganizing boot flow, be permissive 
>>>> about
>>>> invalid mask. codec_mask will be set to proper value during 
>>>> probe_work -
>>>> before skl_codec_create() ever gets called.
>>>
>>> humm, what would happen e.g. if you have select the SKL driver but 
>>> there is no ACPI information to select an I2S-based machine driver, 
>>> and HDaudio/iDISP are disabled? You would have no error checks then?
>>>
>>
>> Laptops I've been testing this with have had Realtek + iDisp present 
>> onboard. Now, if you disable Realtek + HDMI/DP modules within legacy 
>> HDaudio Kconfig and HD audio support within Intel Skylake tree then 
>> you end up with no required modules for said configuration at all. 
>> Nothing will happen really: no warnings, no sound card either.
> 
> I meant enable the HDaudio controller but disable HDaudio codecs/HDMI at 
> the BIOS level. In that case the codec_mask will never be set.
> 

Applying diff (better than BIOS options which are not even present on 
some prod machines):

diff --git a/sound/hda/hdac_controller.c b/sound/hda/hdac_controller.c
index 7e7be8e4dcf9..b0e4579f26f6 100644
--- a/sound/hda/hdac_controller.c
+++ b/sound/hda/hdac_controller.c
@@ -421,7 +421,8 @@ int snd_hdac_bus_reset_link(struct hdac_bus *bus, 
bool full_reset)

         /* detect codecs */
         if (!bus->codec_mask) {
-               bus->codec_mask = snd_hdac_chip_readw(bus, STATESTS);
+               //bus->codec_mask = snd_hdac_chip_readw(bus, STATESTS);
+               bus->codec_mask = 0;
                 dev_dbg(bus->dev, "codec_mask = 0x%lx\n", bus->codec_mask);
         }

---

Results:
- skl boots
- no machines present
- drv stays dormant


Dumping boot log below (notice the '[   21.569291] snd_soc_skl 
0000:00:1f.3: codec_mask = 0x0' message):


[   21.462507] snd_soc_core:snd_soc_register_dai: snd-soc-dummy 
snd-soc-dummy: ASoC: dynamically register DAI snd-soc-dummy
[   21.462541] snd_soc_core:snd_soc_register_dai: snd-soc-dummy 
snd-soc-dummy: ASoC: Registered DAI 'snd-soc-dummy-dai'
[   21.536668] snd_soc_skl 0000:00:1f.3: DSP detected with PCI 
class/subclass/prog-if info 0x040380
[   21.542406] snd_hda_core:snd_hdac_bus_parse_capabilities: snd_soc_skl 
0000:00:1f.3: Capability version: 0x0
[   21.542412] snd_hda_core:snd_hdac_bus_parse_capabilities: snd_soc_skl 
0000:00:1f.3: HDA capability ID: 0x2
[   21.542416] snd_hda_core:snd_hdac_bus_parse_capabilities: snd_soc_skl 
0000:00:1f.3: Found ML capability
[   21.542421] snd_hda_core:snd_hdac_bus_parse_capabilities: snd_soc_skl 
0000:00:1f.3: Capability version: 0x0
[   21.542425] snd_hda_core:snd_hdac_bus_parse_capabilities: snd_soc_skl 
0000:00:1f.3: HDA capability ID: 0x3
[   21.542429] snd_hda_core:snd_hdac_bus_parse_capabilities: snd_soc_skl 
0000:00:1f.3: Found PP capability offset=800
[   21.542434] snd_hda_core:snd_hdac_bus_parse_capabilities: snd_soc_skl 
0000:00:1f.3: Capability version: 0x0
[   21.542438] snd_hda_core:snd_hdac_bus_parse_capabilities: snd_soc_skl 
0000:00:1f.3: HDA capability ID: 0x1
[   21.542443] snd_hda_core:snd_hdac_bus_parse_capabilities: snd_soc_skl 
0000:00:1f.3: Found GTS capability offset=500
[   21.542448] snd_hda_core:snd_hdac_bus_parse_capabilities: snd_soc_skl 
0000:00:1f.3: Capability version: 0x0
[   21.542451] snd_hda_core:snd_hdac_bus_parse_capabilities: snd_soc_skl 
0000:00:1f.3: HDA capability ID: 0x5
[   21.542455] snd_hda_core:snd_hdac_bus_parse_capabilities: snd_soc_skl 
0000:00:1f.3: Found DRSM capability
[   21.542460] snd_hda_core:snd_hdac_bus_parse_capabilities: snd_soc_skl 
0000:00:1f.3: Capability version: 0x0
[   21.542465] snd_hda_core:snd_hdac_bus_parse_capabilities: snd_soc_skl 
0000:00:1f.3: HDA capability ID: 0x4
[   21.542468] snd_hda_core:snd_hdac_bus_parse_capabilities: snd_soc_skl 
0000:00:1f.3: Found SPB capability
[   21.542672] snd_soc_skl:skl_first_init: snd_soc_skl 0000:00:1f.3: 
chipset global capabilities = 0x9701
[   21.556416] snd_soc_skl:skl_nhlt_update_topology_bin: snd_soc_skl 
0000:00:1f.3: oem_id LENOVO, oem_table_id TP-N23   oem_revision 4880
[   21.557053] snd_soc_skl:skl_find_machine: snd_soc_skl 0000:00:1f.3: 
No matching I2S machine driver found
[   21.557650] snd_soc_skl:skl_init_dsp: snd_soc_skl 0000:00:1f.3: dsp 
registration status=0
[   21.557658] snd_hda_ext_core:snd_hdac_ext_bus_get_ml_capabilities: 
snd_soc_skl 0000:00:1f.3: In snd_hdac_ext_bus_get_ml_capabilities Link 
count: 2
[   21.558254] snd_soc_skl 0000:00:1f.3: bound 0000:00:02.0 (ops 
i915_audio_component_bind_ops [i915])
[   21.558259] snd_hda_core:snd_hdac_display_power: snd_soc_skl 
0000:00:1f.3: display power enable
[   21.558272] snd_hda_core:snd_hdac_set_codec_wakeup: snd_soc_skl 
0000:00:1f.3: enable codec wakeup
[   21.561088] snd_hda_core:snd_hdac_set_codec_wakeup: snd_soc_skl 
0000:00:1f.3: disable codec wakeup
[   21.562606] snd_soc_skl:skl_init_pci: snd_soc_skl 0000:00:1f.3: 
Clearing TCSEL
[   21.562617] snd_hda_core:snd_hdac_set_codec_wakeup: snd_soc_skl 
0000:00:1f.3: enable codec wakeup
[   21.569291] snd_soc_skl 0000:00:1f.3: codec_mask = 0x0
[   21.569306] snd_hda_core:snd_hdac_set_codec_wakeup: snd_soc_skl 
0000:00:1f.3: disable codec wakeup
[   21.570856] snd_soc_skl 0000:00:1f.3: no hda codecs found!
[   21.570901] snd_soc_core:snd_soc_register_dai: snd_soc_skl 
0000:00:1f.3: ASoC: dynamically register DAI 0000:00:1f.3
[   21.570923] snd_soc_core:snd_soc_register_dai: snd_soc_skl 
0000:00:1f.3: ASoC: Registered DAI 'SSP0 Pin'
[   21.570927] snd_soc_core:snd_soc_register_dai: snd_soc_skl 
0000:00:1f.3: ASoC: dynamically register DAI 0000:00:1f.3
[   21.570947] snd_soc_core:snd_soc_register_dai: snd_soc_skl 
0000:00:1f.3: ASoC: Registered DAI 'SSP1 Pin'
[   21.570951] snd_soc_core:snd_soc_register_dai: snd_soc_skl 
0000:00:1f.3: ASoC: dynamically register DAI 0000:00:1f.3
[   21.570971] snd_soc_core:snd_soc_register_dai: snd_soc_skl 
0000:00:1f.3: ASoC: Registered DAI 'SSP2 Pin'
[   21.570976] snd_soc_core:snd_soc_register_dai: snd_soc_skl 
0000:00:1f.3: ASoC: dynamically register DAI 0000:00:1f.3
[   21.570996] snd_soc_core:snd_soc_register_dai: snd_soc_skl 
0000:00:1f.3: ASoC: Registered DAI 'SSP3 Pin'
[   21.571000] snd_soc_core:snd_soc_register_dai: snd_soc_skl 
0000:00:1f.3: ASoC: dynamically register DAI 0000:00:1f.3
[   21.571020] snd_soc_core:snd_soc_register_dai: snd_soc_skl 
0000:00:1f.3: ASoC: Registered DAI 'SSP4 Pin'
[   21.571024] snd_soc_core:snd_soc_register_dai: snd_soc_skl 
0000:00:1f.3: ASoC: dynamically register DAI 0000:00:1f.3
[   21.571044] snd_soc_core:snd_soc_register_dai: snd_soc_skl 
0000:00:1f.3: ASoC: Registered DAI 'SSP5 Pin'
[   21.571048] snd_soc_core:snd_soc_register_dai: snd_soc_skl 
0000:00:1f.3: ASoC: dynamically register DAI 0000:00:1f.3
[   21.571067] snd_soc_core:snd_soc_register_dai: snd_soc_skl 
0000:00:1f.3: ASoC: Registered DAI 'iDisp1 Pin'
[   21.571077] snd_soc_core:snd_soc_register_dai: snd_soc_skl 
0000:00:1f.3: ASoC: dynamically register DAI 0000:00:1f.3
[   21.571099] snd_soc_core:snd_soc_register_dai: snd_soc_skl 
0000:00:1f.3: ASoC: Registered DAI 'iDisp2 Pin'
[   21.571103] snd_soc_core:snd_soc_register_dai: snd_soc_skl 
0000:00:1f.3: ASoC: dynamically register DAI 0000:00:1f.3
[   21.571125] snd_soc_core:snd_soc_register_dai: snd_soc_skl 
0000:00:1f.3: ASoC: Registered DAI 'iDisp3 Pin'
[   21.571129] snd_soc_core:snd_soc_register_dai: snd_soc_skl 
0000:00:1f.3: ASoC: dynamically register DAI 0000:00:1f.3
[   21.571148] snd_soc_core:snd_soc_register_dai: snd_soc_skl 
0000:00:1f.3: ASoC: Registered DAI 'DMIC01 Pin'
[   21.571153] snd_soc_core:snd_soc_register_dai: snd_soc_skl 
0000:00:1f.3: ASoC: dynamically register DAI 0000:00:1f.3
[   21.571172] snd_soc_core:snd_soc_register_dai: snd_soc_skl 
0000:00:1f.3: ASoC: Registered DAI 'DMIC16k Pin'
[   21.571176] snd_soc_core:snd_soc_register_dai: snd_soc_skl 
0000:00:1f.3: ASoC: dynamically register DAI 0000:00:1f.3
[   21.571209] snd_soc_core:snd_soc_register_dai: snd_soc_skl 
0000:00:1f.3: ASoC: Registered DAI 'Analog CPU DAI'
[   21.571213] snd_soc_core:snd_soc_register_dai: snd_soc_skl 
0000:00:1f.3: ASoC: dynamically register DAI 0000:00:1f.3
[   21.571230] snd_soc_core:snd_soc_register_dai: snd_soc_skl 
0000:00:1f.3: ASoC: Registered DAI 'Alt Analog CPU DAI'
[   21.571233] snd_soc_core:snd_soc_register_dai: snd_soc_skl 
0000:00:1f.3: ASoC: dynamically register DAI 0000:00:1f.3
[   21.571250] snd_soc_core:snd_soc_register_dai: snd_soc_skl 
0000:00:1f.3: ASoC: Registered DAI 'Digital CPU DAI'
[   21.571566] snd_hda_core:snd_hdac_display_power: snd_soc_skl 
0000:00:1f.3: display power disable
[   21.571585] snd_soc_skl:skl_runtime_suspend: snd_soc_skl 
0000:00:1f.3: in skl_runtime_suspend

Re: [PATCH 0/7] ASoC: Intel: Skylake: Fix HDaudio and Dmic

[Pierre-Louis Bossart]

> I didn't actually look at the patches since by the time I went to look
> at them it was clear that there was going to be a new version.  Pierre
> was saying that they added new functionality which would generally not
> be suitable.

I am ok with the patches, as long as "[PATCH 5/7] ASoC: Intel: 
skl_hda_dsp: Enable Dmic configuration" is dropped as discussed.

I don't know if that requires a v2 or if Mark you can just drop this one 
patch?

So for all Patches except Patch5:

Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Thanks Cezary!

Applied "ASoC: Intel: Skylake: Await purge request ack on CNL" to the asoc tree

[Mark Brown]

The patch

   ASoC: Intel: Skylake: Await purge request ack on CNL

has been applied to the asoc tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

From 7693cadac86548b30389a6e11d78c38db654f393 Mon Sep 17 00:00:00 2001
From: Cezary Rojewski <cezary.rojewski@intel.com>
Date: Thu, 5 Mar 2020 15:53:14 +0100
Subject: [PATCH] ASoC: Intel: Skylake: Await purge request ack on CNL

Each purge request is sent by driver after master core is powered up and
unresetted but before it is unstalled. On unstall, ROM begins processing
the request and initializing environment for FW load. Host should await
ROM's ack before moving forward. Without doing so, ROM init poll may
start too early and false timeouts can occur.

Fixes: cb6a55284629 ("ASoC: Intel: cnl: Add sst library functions for cnl platform")
Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Link: https://lore.kernel.org/r/20200305145314.32579-8-cezary.rojewski@intel.com
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/intel/skylake/bxt-sst.c     |  1 -
 sound/soc/intel/skylake/cnl-sst.c     | 20 ++++++++++++++++++--
 sound/soc/intel/skylake/skl-sst-dsp.h |  1 +
 3 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/sound/soc/intel/skylake/bxt-sst.c b/sound/soc/intel/skylake/bxt-sst.c
index cdafade8abd6..38b9d7494083 100644
--- a/sound/soc/intel/skylake/bxt-sst.c
+++ b/sound/soc/intel/skylake/bxt-sst.c
@@ -17,7 +17,6 @@
 #include "skl.h"
 
 #define BXT_BASEFW_TIMEOUT	3000
-#define BXT_INIT_TIMEOUT	300
 #define BXT_ROM_INIT_TIMEOUT	70
 #define BXT_IPC_PURGE_FW	0x01004000
 
diff --git a/sound/soc/intel/skylake/cnl-sst.c b/sound/soc/intel/skylake/cnl-sst.c
index 060e47ae3391..c6abcd5aa67b 100644
--- a/sound/soc/intel/skylake/cnl-sst.c
+++ b/sound/soc/intel/skylake/cnl-sst.c
@@ -57,18 +57,34 @@ static int cnl_prepare_fw(struct sst_dsp *ctx, const void *fwdata, u32 fwsize)
 	ctx->dsp_ops.stream_tag = stream_tag;
 	memcpy(ctx->dmab.area, fwdata, fwsize);
 
+	ret = skl_dsp_core_power_up(ctx, SKL_DSP_CORE0_MASK);
+	if (ret < 0) {
+		dev_err(ctx->dev, "dsp core0 power up failed\n");
+		ret = -EIO;
+		goto base_fw_load_failed;
+	}
+
 	/* purge FW request */
 	sst_dsp_shim_write(ctx, CNL_ADSP_REG_HIPCIDR,
 			   CNL_ADSP_REG_HIPCIDR_BUSY | (CNL_IPC_PURGE |
 			   ((stream_tag - 1) << CNL_ROM_CTRL_DMA_ID)));
 
-	ret = cnl_dsp_enable_core(ctx, SKL_DSP_CORE0_MASK);
+	ret = skl_dsp_start_core(ctx, SKL_DSP_CORE0_MASK);
 	if (ret < 0) {
-		dev_err(ctx->dev, "dsp boot core failed ret: %d\n", ret);
+		dev_err(ctx->dev, "Start dsp core failed ret: %d\n", ret);
 		ret = -EIO;
 		goto base_fw_load_failed;
 	}
 
+	ret = sst_dsp_register_poll(ctx, CNL_ADSP_REG_HIPCIDA,
+				    CNL_ADSP_REG_HIPCIDA_DONE,
+				    CNL_ADSP_REG_HIPCIDA_DONE,
+				    BXT_INIT_TIMEOUT, "HIPCIDA Done");
+	if (ret < 0) {
+		dev_err(ctx->dev, "timeout for purge request: %d\n", ret);
+		goto base_fw_load_failed;
+	}
+
 	/* enable interrupt */
 	cnl_ipc_int_enable(ctx);
 	cnl_ipc_op_int_enable(ctx);
diff --git a/sound/soc/intel/skylake/skl-sst-dsp.h b/sound/soc/intel/skylake/skl-sst-dsp.h
index 067d1ea11cde..1df9ef422f61 100644
--- a/sound/soc/intel/skylake/skl-sst-dsp.h
+++ b/sound/soc/intel/skylake/skl-sst-dsp.h
@@ -68,6 +68,7 @@ struct skl_dev;
 #define SKL_FW_INIT			0x1
 #define SKL_FW_RFW_START		0xf
 #define BXT_FW_ROM_INIT_RETRY		3
+#define BXT_INIT_TIMEOUT		300
 
 #define SKL_ADSPIC_IPC			1
 #define SKL_ADSPIS_IPC			1
-- 
2.20.1

Applied "ASoC: Intel: Allow for ROM init retry on CNL platforms" to the asoc tree

[Mark Brown]

The patch

   ASoC: Intel: Allow for ROM init retry on CNL platforms

has been applied to the asoc tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

From 024aa45f55ccd40704cfdef61b2a8b6d0de9cdd1 Mon Sep 17 00:00:00 2001
From: Cezary Rojewski <cezary.rojewski@intel.com>
Date: Thu, 5 Mar 2020 15:53:13 +0100
Subject: [PATCH] ASoC: Intel: Allow for ROM init retry on CNL platforms

Due to unconditional initial timeouts, firmware may fail to load during
its initialization. This issue cannot be resolved on driver side as it
is caused by external sources such as CSME but has to be accounted for
nonetheless.

Fixes: cb6a55284629 ("ASoC: Intel: cnl: Add sst library functions for cnl platform")
Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Link: https://lore.kernel.org/r/20200305145314.32579-7-cezary.rojewski@intel.com
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/intel/skylake/bxt-sst.c     |  2 --
 sound/soc/intel/skylake/cnl-sst.c     | 15 ++++++++++-----
 sound/soc/intel/skylake/skl-sst-dsp.h |  1 +
 3 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/sound/soc/intel/skylake/bxt-sst.c b/sound/soc/intel/skylake/bxt-sst.c
index 92a82e6b5fe6..cdafade8abd6 100644
--- a/sound/soc/intel/skylake/bxt-sst.c
+++ b/sound/soc/intel/skylake/bxt-sst.c
@@ -38,8 +38,6 @@
 /* Delay before scheduling D0i3 entry */
 #define BXT_D0I3_DELAY 5000
 
-#define BXT_FW_ROM_INIT_RETRY 3
-
 static unsigned int bxt_get_errorcode(struct sst_dsp *ctx)
 {
 	 return sst_dsp_shim_read(ctx, BXT_ADSP_ERROR_CODE);
diff --git a/sound/soc/intel/skylake/cnl-sst.c b/sound/soc/intel/skylake/cnl-sst.c
index 4f64f097e9ae..060e47ae3391 100644
--- a/sound/soc/intel/skylake/cnl-sst.c
+++ b/sound/soc/intel/skylake/cnl-sst.c
@@ -109,7 +109,7 @@ static int cnl_load_base_firmware(struct sst_dsp *ctx)
 {
 	struct firmware stripped_fw;
 	struct skl_dev *cnl = ctx->thread_context;
-	int ret;
+	int ret, i;
 
 	if (!ctx->fw) {
 		ret = request_firmware(&ctx->fw, ctx->fw_name, ctx->dev);
@@ -131,12 +131,16 @@ static int cnl_load_base_firmware(struct sst_dsp *ctx)
 	stripped_fw.size = ctx->fw->size;
 	skl_dsp_strip_extended_manifest(&stripped_fw);
 
-	ret = cnl_prepare_fw(ctx, stripped_fw.data, stripped_fw.size);
-	if (ret < 0) {
-		dev_err(ctx->dev, "prepare firmware failed: %d\n", ret);
-		goto cnl_load_base_firmware_failed;
+	for (i = 0; i < BXT_FW_ROM_INIT_RETRY; i++) {
+		ret = cnl_prepare_fw(ctx, stripped_fw.data, stripped_fw.size);
+		if (!ret)
+			break;
+		dev_dbg(ctx->dev, "prepare firmware failed: %d\n", ret);
 	}
 
+	if (ret < 0)
+		goto cnl_load_base_firmware_failed;
+
 	ret = sst_transfer_fw_host_dma(ctx);
 	if (ret < 0) {
 		dev_err(ctx->dev, "transfer firmware failed: %d\n", ret);
@@ -158,6 +162,7 @@ static int cnl_load_base_firmware(struct sst_dsp *ctx)
 	return 0;
 
 cnl_load_base_firmware_failed:
+	dev_err(ctx->dev, "firmware load failed: %d\n", ret);
 	release_firmware(ctx->fw);
 	ctx->fw = NULL;
 
diff --git a/sound/soc/intel/skylake/skl-sst-dsp.h b/sound/soc/intel/skylake/skl-sst-dsp.h
index cdfec0fca577..067d1ea11cde 100644
--- a/sound/soc/intel/skylake/skl-sst-dsp.h
+++ b/sound/soc/intel/skylake/skl-sst-dsp.h
@@ -67,6 +67,7 @@ struct skl_dev;
 
 #define SKL_FW_INIT			0x1
 #define SKL_FW_RFW_START		0xf
+#define BXT_FW_ROM_INIT_RETRY		3
 
 #define SKL_ADSPIC_IPC			1
 #define SKL_ADSPIS_IPC			1
-- 
2.20.1

Applied "ASoC: Intel: Skylake: Shield against no-NHLT configurations" to the asoc tree

[Mark Brown]

The patch

   ASoC: Intel: Skylake: Shield against no-NHLT configurations

has been applied to the asoc tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

From 9e6c382f5a6161eb55115fb56614b9827f2e7da3 Mon Sep 17 00:00:00 2001
From: Cezary Rojewski <cezary.rojewski@intel.com>
Date: Thu, 5 Mar 2020 15:53:11 +0100
Subject: [PATCH] ASoC: Intel: Skylake: Shield against no-NHLT configurations

Some configurations expose no NHLT table at all within their
/sys/firmware/acpi/tables. To prevent NULL-dereference errors from
occurring, adjust probe flow and append additional safety checks in
functions involved in NHLT lifecycle.

Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Link: https://lore.kernel.org/r/20200305145314.32579-5-cezary.rojewski@intel.com
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/intel/skylake/skl-nhlt.c | 3 ++-
 sound/soc/intel/skylake/skl.c      | 9 +++++++--
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/sound/soc/intel/skylake/skl-nhlt.c b/sound/soc/intel/skylake/skl-nhlt.c
index 19f328d71f24..d9c8f5cb389e 100644
--- a/sound/soc/intel/skylake/skl-nhlt.c
+++ b/sound/soc/intel/skylake/skl-nhlt.c
@@ -182,7 +182,8 @@ void skl_nhlt_remove_sysfs(struct skl_dev *skl)
 {
 	struct device *dev = &skl->pci->dev;
 
-	sysfs_remove_file(&dev->kobj, &dev_attr_platform_id.attr);
+	if (skl->nhlt)
+		sysfs_remove_file(&dev->kobj, &dev_attr_platform_id.attr);
 }
 
 /*
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
index e2e531c96dd1..7ad8a75759bd 100644
--- a/sound/soc/intel/skylake/skl.c
+++ b/sound/soc/intel/skylake/skl.c
@@ -633,6 +633,9 @@ static int skl_clock_device_register(struct skl_dev *skl)
 	struct platform_device_info pdevinfo = {NULL};
 	struct skl_clk_pdata *clk_pdata;
 
+	if (!skl->nhlt)
+		return 0;
+
 	clk_pdata = devm_kzalloc(&skl->pci->dev, sizeof(*clk_pdata),
 							GFP_KERNEL);
 	if (!clk_pdata)
@@ -1074,7 +1077,8 @@ static int skl_probe(struct pci_dev *pci,
 out_clk_free:
 	skl_clock_device_unregister(skl);
 out_nhlt_free:
-	intel_nhlt_free(skl->nhlt);
+	if (skl->nhlt)
+		intel_nhlt_free(skl->nhlt);
 out_free:
 	skl_free(bus);
 
@@ -1123,7 +1127,8 @@ static void skl_remove(struct pci_dev *pci)
 	skl_dmic_device_unregister(skl);
 	skl_clock_device_unregister(skl);
 	skl_nhlt_remove_sysfs(skl);
-	intel_nhlt_free(skl->nhlt);
+	if (skl->nhlt)
+		intel_nhlt_free(skl->nhlt);
 	skl_free(bus);
 	dev_set_drvdata(&pci->dev, NULL);
 }
-- 
2.20.1

Applied "ASoC: Intel: Skylake: Enable codec wakeup during chip init" to the asoc tree

[Mark Brown]

The patch

   ASoC: Intel: Skylake: Enable codec wakeup during chip init

has been applied to the asoc tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

From e603f11d5df8997d104ab405ff27640b90baffaa Mon Sep 17 00:00:00 2001
From: Cezary Rojewski <cezary.rojewski@intel.com>
Date: Thu, 5 Mar 2020 15:53:10 +0100
Subject: [PATCH] ASoC: Intel: Skylake: Enable codec wakeup during chip init

Follow the recommendation set by hda_intel.c and enable HDMI/DP codec
wakeup during bus initialization procedure. Disable wakeup once init
completes.

Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Link: https://lore.kernel.org/r/20200305145314.32579-4-cezary.rojewski@intel.com
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/intel/skylake/skl.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
index 4827fe6bc1cb..e2e531c96dd1 100644
--- a/sound/soc/intel/skylake/skl.c
+++ b/sound/soc/intel/skylake/skl.c
@@ -130,6 +130,7 @@ static int skl_init_chip(struct hdac_bus *bus, bool full_reset)
 	struct hdac_ext_link *hlink;
 	int ret;
 
+	snd_hdac_set_codec_wakeup(bus, true);
 	skl_enable_miscbdcge(bus->dev, false);
 	ret = snd_hdac_bus_init_chip(bus, full_reset);
 
@@ -138,6 +139,7 @@ static int skl_init_chip(struct hdac_bus *bus, bool full_reset)
 		writel(0, hlink->ml_addr + AZX_REG_ML_LOSIDV);
 
 	skl_enable_miscbdcge(bus->dev, true);
+	snd_hdac_set_codec_wakeup(bus, false);
 
 	return ret;
 }
-- 
2.20.1

Applied "ASoC: Intel: Skylake: Select hda configuration permissively" to the asoc tree

[Mark Brown]

The patch

   ASoC: Intel: Skylake: Select hda configuration permissively

has been applied to the asoc tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

From a66f88394a78fec9a05fa6e517e9603e8eca8363 Mon Sep 17 00:00:00 2001
From: Cezary Rojewski <cezary.rojewski@intel.com>
Date: Thu, 5 Mar 2020 15:53:09 +0100
Subject: [PATCH] ASoC: Intel: Skylake: Select hda configuration permissively

With _reset_link removed from the probe sequence, codec_mask at the time
skl_find_hda_machine() is invoked will always be 0, so hda machine will
never be chosen. Rather than reorganizing boot flow, be permissive about
invalid mask. codec_mask will be set to proper value during probe_work -
before skl_codec_create() ever gets called.

Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Link: https://lore.kernel.org/r/20200305145314.32579-3-cezary.rojewski@intel.com
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/intel/skylake/skl.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
index d66231525356..4827fe6bc1cb 100644
--- a/sound/soc/intel/skylake/skl.c
+++ b/sound/soc/intel/skylake/skl.c
@@ -481,13 +481,8 @@ static struct skl_ssp_clk skl_ssp_clks[] = {
 static struct snd_soc_acpi_mach *skl_find_hda_machine(struct skl_dev *skl,
 					struct snd_soc_acpi_mach *machines)
 {
-	struct hdac_bus *bus = skl_to_bus(skl);
 	struct snd_soc_acpi_mach *mach;
 
-	/* check if we have any codecs detected on bus */
-	if (bus->codec_mask == 0)
-		return NULL;
-
 	/* point to common table */
 	mach = snd_soc_acpi_intel_hda_machines;
 
-- 
2.20.1

Applied "ASoC: Intel: Skylake: Remove superfluous chip initialization" to the asoc tree

[Mark Brown]

The patch

   ASoC: Intel: Skylake: Remove superfluous chip initialization

has been applied to the asoc tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

From 2ef81057d80456870b97890dd79c8f56a85b1242 Mon Sep 17 00:00:00 2001
From: Cezary Rojewski <cezary.rojewski@intel.com>
Date: Thu, 5 Mar 2020 15:53:08 +0100
Subject: [PATCH] ASoC: Intel: Skylake: Remove superfluous chip initialization

Skylake driver does the controller init operation twice:
- first during probe (only to stop it just before scheduling probe_work)
- and during said probe_work where the actual correct sequence is
executed

To properly complete boot sequence when iDisp codec is present, bus
initialization has to be called only after _i915_init() finishes.
With additional _reset_list preceding _i915_init(), iDisp codec never
gets the chance to enumerate on the link. Remove the superfluous
initialization to address the issue.

Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Link: https://lore.kernel.org/r/20200305145314.32579-2-cezary.rojewski@intel.com
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/intel/skylake/skl.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
index f755ca2484cf..d66231525356 100644
--- a/sound/soc/intel/skylake/skl.c
+++ b/sound/soc/intel/skylake/skl.c
@@ -803,6 +803,9 @@ static void skl_probe_work(struct work_struct *work)
 			return;
 	}
 
+	skl_init_pci(skl);
+	skl_dum_set(bus);
+
 	err = skl_init_chip(bus, true);
 	if (err < 0) {
 		dev_err(bus->dev, "Init chip failed with err: %d\n", err);
@@ -918,8 +921,6 @@ static int skl_first_init(struct hdac_bus *bus)
 		return -ENXIO;
 	}
 
-	snd_hdac_bus_reset_link(bus, true);
-
 	snd_hdac_bus_parse_capabilities(bus);
 
 	/* check if PPCAP exists */
@@ -967,11 +968,7 @@ static int skl_first_init(struct hdac_bus *bus)
 	if (err < 0)
 		return err;
 
-	/* initialize chip */
-	skl_init_pci(skl);
-	skl_dum_set(bus);
-
-	return skl_init_chip(bus, true);
+	return 0;
 }
 
 static int skl_probe(struct pci_dev *pci,
@@ -1064,8 +1061,6 @@ static int skl_probe(struct pci_dev *pci,
 	if (bus->mlcap)
 		snd_hdac_ext_bus_get_ml_capabilities(bus);
 
-	snd_hdac_bus_stop_chip(bus);
-
 	/* create device for soc dmic */
 	err = skl_dmic_device_register(skl);
 	if (err < 0) {
-- 
2.20.1

Re: [PATCH 0/7] ASoC: Intel: Skylake: Fix HDaudio and Dmic

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 414 bytes --]

On Wed, Mar 11, 2020 at 04:49:41PM +0100, Cezary Rojewski wrote:

> Not very familiar with the cherry-pick mechanism for stable kernels, but is
> it possible for patches that ain't flagged with 'Fixes' tag to get
> backported? Having all of these at least on 5.4 is in my opinion desirable.

The document I pointed you at for the stable process includes
information on how to submit patches for stable via e-mail.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]