[PATCH 0/7] ASoC: Intel: SOF: clarifications on hardware support

[PATCH 0/7] ASoC: Intel: SOF: clarifications on hardware support

[Pierre-Louis Bossart]

This patchset revisits the Intel hardware support in SOF. The HDAudio
DMA position information was not following hardware recommended
programming sequences (similar changes are already part of the HDaudio
legacy driver), and the stream assignment applied a work-around that
was only needed on specific versions of hardware. These changes are
not tagged as 'Fixes' and don't need to be applied to -stable
versions.

While we're at it, DPIB handling is improved, useless fields removed,
a comment added on JasperLake support, and IceLake-specific routines
are isolated.

Pierre-Louis Bossart (6):
  ASoC: SOF: Intel: hda-stream: limit PROCEN workaround
  ASoC: SOF: Intel: hda-ctrl: apply symmetry for DPIB
  ASoC: SOF: hda-stream: only enable DPIB if needed
  ASoC: SOF: Intel: hda: add quirks for HDAudio DMA position information
  ASoC: SOF: Intel: hda-dai: remove unused fields
  ASoC: SOF: Intel: add comment on JasperLake support

Ranjani Sridharan (1):
  ASoC: SOF: Intel: ICL: move ICL-specific ops to icl.c

 sound/soc/sof/intel/apl.c        |  1 +
 sound/soc/sof/intel/cnl.c        |  7 +++
 sound/soc/sof/intel/hda-ctrl.c   |  2 +-
 sound/soc/sof/intel/hda-dai.c    |  4 --
 sound/soc/sof/intel/hda-loader.c | 64 ------------------------
 sound/soc/sof/intel/hda-pcm.c    | 86 ++++++++++++++++++++++----------
 sound/soc/sof/intel/hda-stream.c | 25 ++++++----
 sound/soc/sof/intel/hda.c        |  9 +++-
 sound/soc/sof/intel/hda.h        |  8 ++-
 sound/soc/sof/intel/icl.c        | 67 ++++++++++++++++++++++++-
 sound/soc/sof/intel/shim.h       |  4 ++
 11 files changed, 169 insertions(+), 108 deletions(-)

-- 
2.25.1

[PATCH 1/7] ASoC: SOF: Intel: ICL: move ICL-specific ops to icl.c

[Pierre-Louis Bossart]

From: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

Move the ICL specific ops to icl.c. Also introduce a
macro ICL_DSP_HPRO_CORE_ID to define the core that
should be powered up when HPRO is enabled.

Reviewed-by: Bard Liao <bard.liao@intel.com>
Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/sof/intel/hda-loader.c | 64 ------------------------------
 sound/soc/sof/intel/hda.h        |  2 -
 sound/soc/sof/intel/icl.c        | 67 +++++++++++++++++++++++++++++++-
 3 files changed, 65 insertions(+), 68 deletions(-)

diff --git a/sound/soc/sof/intel/hda-loader.c b/sound/soc/sof/intel/hda-loader.c
index 40201e5ac201..bfb0e374ebab 100644
--- a/sound/soc/sof/intel/hda-loader.c
+++ b/sound/soc/sof/intel/hda-loader.c
@@ -481,49 +481,6 @@ int hda_dsp_post_fw_run(struct snd_sof_dev *sdev)
 	return hda_dsp_ctrl_clock_power_gating(sdev, true);
 }
 
-/*
- * post fw run operations for ICL,
- * Core 3 will be powered up and in stall when HPRO is enabled
- */
-int hda_dsp_post_fw_run_icl(struct snd_sof_dev *sdev)
-{
-	struct sof_intel_hda_dev *hda = sdev->pdata->hw_pdata;
-	int ret;
-
-	if (sdev->first_boot) {
-		ret = hda_sdw_startup(sdev);
-		if (ret < 0) {
-			dev_err(sdev->dev,
-				"error: could not startup SoundWire links\n");
-			return ret;
-		}
-	}
-
-	hda_sdw_int_enable(sdev, true);
-
-	/*
-	 * The recommended HW programming sequence for ICL is to
-	 * power up core 3 and keep it in stall if HPRO is enabled.
-	 * Major difference between ICL and TGL, on ICL core 3 is managed by
-	 * the host whereas on TGL it is handled by the firmware.
-	 */
-	if (!hda->clk_config_lpro) {
-		ret = hda_dsp_enable_core(sdev, BIT(3));
-		if (ret < 0) {
-			dev_err(sdev->dev, "error: dsp core power up failed on core 3\n");
-			return ret;
-		}
-
-		sdev->enabled_cores_mask |= BIT(3);
-		sdev->dsp_core_ref_count[3]++;
-
-		snd_sof_dsp_stall(sdev, BIT(3));
-	}
-
-	/* re-enable clock gating and power gating */
-	return hda_dsp_ctrl_clock_power_gating(sdev, true);
-}
-
 int hda_dsp_ext_man_get_cavs_config_data(struct snd_sof_dev *sdev,
 					 const struct sof_ext_man_elem_header *hdr)
 {
@@ -561,24 +518,3 @@ int hda_dsp_ext_man_get_cavs_config_data(struct snd_sof_dev *sdev,
 
 	return 0;
 }
-
-int hda_dsp_core_stall_icl(struct snd_sof_dev *sdev, unsigned int core_mask)
-{
-	struct sof_intel_hda_dev *hda = sdev->pdata->hw_pdata;
-	const struct sof_intel_dsp_desc *chip = hda->desc;
-
-	/* make sure core_mask in host managed cores */
-	core_mask &= chip->host_managed_cores_mask;
-	if (!core_mask) {
-		dev_err(sdev->dev, "error: core_mask is not in host managed cores\n");
-		return -EINVAL;
-	}
-
-	/* stall core */
-	snd_sof_dsp_update_bits_unlocked(sdev, HDA_DSP_BAR,
-					 HDA_DSP_REG_ADSPCS,
-					 HDA_DSP_ADSPCS_CSTALL_MASK(core_mask),
-					 HDA_DSP_ADSPCS_CSTALL_MASK(core_mask));
-
-	return 0;
-}
diff --git a/sound/soc/sof/intel/hda.h b/sound/soc/sof/intel/hda.h
index 72e78c449aa8..e2055b6c8139 100644
--- a/sound/soc/sof/intel/hda.h
+++ b/sound/soc/sof/intel/hda.h
@@ -618,8 +618,6 @@ int hda_dsp_cl_boot_firmware_iccmax(struct snd_sof_dev *sdev);
 /* pre and post fw run ops */
 int hda_dsp_pre_fw_run(struct snd_sof_dev *sdev);
 int hda_dsp_post_fw_run(struct snd_sof_dev *sdev);
-int hda_dsp_post_fw_run_icl(struct snd_sof_dev *sdev);
-int hda_dsp_core_stall_icl(struct snd_sof_dev *sdev, unsigned int core_mask);
 
 /* parse platform specific ext manifest ops */
 int hda_dsp_ext_man_get_cavs_config_data(struct snd_sof_dev *sdev,
diff --git a/sound/soc/sof/intel/icl.c b/sound/soc/sof/intel/icl.c
index 343c1af7c453..f75e3983969f 100644
--- a/sound/soc/sof/intel/icl.c
+++ b/sound/soc/sof/intel/icl.c
@@ -18,12 +18,75 @@
 #include "hda-ipc.h"
 #include "../sof-audio.h"
 
+#define ICL_DSP_HPRO_CORE_ID 3
+
 static const struct snd_sof_debugfs_map icl_dsp_debugfs[] = {
 	{"hda", HDA_DSP_HDA_BAR, 0, 0x4000, SOF_DEBUGFS_ACCESS_ALWAYS},
 	{"pp", HDA_DSP_PP_BAR,  0, 0x1000, SOF_DEBUGFS_ACCESS_ALWAYS},
 	{"dsp", HDA_DSP_BAR,  0, 0x10000, SOF_DEBUGFS_ACCESS_ALWAYS},
 };
 
+static int icl_dsp_core_stall(struct snd_sof_dev *sdev, unsigned int core_mask)
+{
+	struct sof_intel_hda_dev *hda = sdev->pdata->hw_pdata;
+	const struct sof_intel_dsp_desc *chip = hda->desc;
+
+	/* make sure core_mask in host managed cores */
+	core_mask &= chip->host_managed_cores_mask;
+	if (!core_mask) {
+		dev_err(sdev->dev, "error: core_mask is not in host managed cores\n");
+		return -EINVAL;
+	}
+
+	/* stall core */
+	snd_sof_dsp_update_bits_unlocked(sdev, HDA_DSP_BAR, HDA_DSP_REG_ADSPCS,
+					 HDA_DSP_ADSPCS_CSTALL_MASK(core_mask),
+					 HDA_DSP_ADSPCS_CSTALL_MASK(core_mask));
+
+	return 0;
+}
+
+/*
+ * post fw run operation for ICL.
+ * Core 3 will be powered up and in stall when HPRO is enabled
+ */
+static int icl_dsp_post_fw_run(struct snd_sof_dev *sdev)
+{
+	struct sof_intel_hda_dev *hda = sdev->pdata->hw_pdata;
+	int ret;
+
+	if (sdev->first_boot) {
+		ret = hda_sdw_startup(sdev);
+		if (ret < 0) {
+			dev_err(sdev->dev, "error: could not startup SoundWire links\n");
+			return ret;
+		}
+	}
+
+	hda_sdw_int_enable(sdev, true);
+
+	/*
+	 * The recommended HW programming sequence for ICL is to
+	 * power up core 3 and keep it in stall if HPRO is enabled.
+	 */
+	if (!hda->clk_config_lpro) {
+		ret = hda_dsp_enable_core(sdev, BIT(ICL_DSP_HPRO_CORE_ID));
+		if (ret < 0) {
+			dev_err(sdev->dev, "error: dsp core power up failed on core %d\n",
+				ICL_DSP_HPRO_CORE_ID);
+			return ret;
+		}
+
+		sdev->enabled_cores_mask |= BIT(ICL_DSP_HPRO_CORE_ID);
+		sdev->dsp_core_ref_count[ICL_DSP_HPRO_CORE_ID]++;
+
+		snd_sof_dsp_stall(sdev, BIT(ICL_DSP_HPRO_CORE_ID));
+	}
+
+	/* re-enable clock gating and power gating */
+	return hda_dsp_ctrl_clock_power_gating(sdev, true);
+}
+
 /* Icelake ops */
 const struct snd_sof_dsp_ops sof_icl_ops = {
 	/* probe/remove/shutdown */
@@ -93,7 +156,7 @@ const struct snd_sof_dsp_ops sof_icl_ops = {
 
 	/* pre/post fw run */
 	.pre_fw_run = hda_dsp_pre_fw_run,
-	.post_fw_run = hda_dsp_post_fw_run_icl,
+	.post_fw_run = icl_dsp_post_fw_run,
 
 	/* parse platform specific extended manifest */
 	.parse_platform_ext_manifest = hda_dsp_ext_man_get_cavs_config_data,
@@ -103,7 +166,7 @@ const struct snd_sof_dsp_ops sof_icl_ops = {
 
 	/* firmware run */
 	.run = hda_dsp_cl_boot_firmware_iccmax,
-	.stall = hda_dsp_core_stall_icl,
+	.stall = icl_dsp_core_stall,
 
 	/* trace callback */
 	.trace_init = hda_dsp_trace_init,
-- 
2.25.1

[PATCH 2/7] ASoC: SOF: Intel: hda-stream: limit PROCEN workaround

[Pierre-Louis Bossart]

The work-around enabled in hda-stream.c is only required on earlier
versions of SOCs/PCH (Skylake, KabyLake, ApolloLake,
GeminiLake). Before setting the format on the host DMA, it is required
to couple the host and link DMA - which as a consequence shall use the
same format.

This patch introduces a quirk field in the platform descriptor and
makes the work-around conditional. Newer platforms have
no limitations on the use of host and link DMA, which can use
different formats.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Reviewed-by: Péter Ujfalusi <peter.ujfalusi@linux.intel.com>
Reviewed-by: Rander Wang <rander.wang@intel.com>
---
 sound/soc/sof/intel/apl.c        |  1 +
 sound/soc/sof/intel/hda-stream.c | 18 ++++++++++++------
 sound/soc/sof/intel/shim.h       |  4 ++++
 3 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/sound/soc/sof/intel/apl.c b/sound/soc/sof/intel/apl.c
index 8778f46f1d37..810b8b6748a0 100644
--- a/sound/soc/sof/intel/apl.c
+++ b/sound/soc/sof/intel/apl.c
@@ -147,5 +147,6 @@ const struct sof_intel_dsp_desc apl_chip_info = {
 	.rom_init_timeout	= 150,
 	.ssp_count = APL_SSP_COUNT,
 	.ssp_base_offset = APL_SSP_BASE_OFFSET,
+	.quirks = SOF_INTEL_PROCEN_FMT_QUIRK,
 };
 EXPORT_SYMBOL_NS(apl_chip_info, SND_SOC_SOF_INTEL_HDA_COMMON);
diff --git a/sound/soc/sof/intel/hda-stream.c b/sound/soc/sof/intel/hda-stream.c
index 440827ce390d..5f9eb5bdcdba 100644
--- a/sound/soc/sof/intel/hda-stream.c
+++ b/sound/soc/sof/intel/hda-stream.c
@@ -472,6 +472,7 @@ int hda_dsp_stream_hw_params(struct snd_sof_dev *sdev,
 			     struct snd_dma_buffer *dmab,
 			     struct snd_pcm_hw_params *params)
 {
+	const struct sof_intel_dsp_desc *chip = get_chip_info(sdev->pdata);
 	struct hdac_bus *bus = sof_to_bus(sdev);
 	struct hdac_stream *hstream = &stream->hstream;
 	int sd_offset = SOF_STREAM_SD_OFFSET(hstream);
@@ -584,6 +585,7 @@ int hda_dsp_stream_hw_params(struct snd_sof_dev *sdev,
 
 	/*
 	 * Recommended hardware programming sequence for HDAudio DMA format
+	 * on earlier platforms - this is not needed on newer platforms
 	 *
 	 * 1. Put DMA into coupled mode by clearing PPCTL.PROCEN bit
 	 *    for corresponding stream index before the time of writing
@@ -593,9 +595,11 @@ int hda_dsp_stream_hw_params(struct snd_sof_dev *sdev,
 	 *    enable decoupled mode
 	 */
 
-	/* couple host and link DMA, disable DSP features */
-	snd_sof_dsp_update_bits(sdev, HDA_DSP_PP_BAR, SOF_HDA_REG_PP_PPCTL,
-				mask, 0);
+	if (chip->quirks & SOF_INTEL_PROCEN_FMT_QUIRK) {
+		/* couple host and link DMA, disable DSP features */
+		snd_sof_dsp_update_bits(sdev, HDA_DSP_PP_BAR, SOF_HDA_REG_PP_PPCTL,
+					mask, 0);
+	}
 
 	/* program stream format */
 	snd_sof_dsp_update_bits(sdev, HDA_DSP_HDA_BAR,
@@ -603,9 +607,11 @@ int hda_dsp_stream_hw_params(struct snd_sof_dev *sdev,
 				SOF_HDA_ADSP_REG_CL_SD_FORMAT,
 				0xffff, hstream->format_val);
 
-	/* decouple host and link DMA, enable DSP features */
-	snd_sof_dsp_update_bits(sdev, HDA_DSP_PP_BAR, SOF_HDA_REG_PP_PPCTL,
-				mask, mask);
+	if (chip->quirks & SOF_INTEL_PROCEN_FMT_QUIRK) {
+		/* decouple host and link DMA, enable DSP features */
+		snd_sof_dsp_update_bits(sdev, HDA_DSP_PP_BAR, SOF_HDA_REG_PP_PPCTL,
+					mask, mask);
+	}
 
 	/* program last valid index */
 	snd_sof_dsp_update_bits(sdev, HDA_DSP_HDA_BAR,
diff --git a/sound/soc/sof/intel/shim.h b/sound/soc/sof/intel/shim.h
index 08c53cb41ea7..f36cd9d5eb94 100644
--- a/sound/soc/sof/intel/shim.h
+++ b/sound/soc/sof/intel/shim.h
@@ -151,6 +151,9 @@
 #define PCI_PMCS		0x84
 #define PCI_PMCS_PS_MASK	0x3
 
+/* Intel quirks */
+#define SOF_INTEL_PROCEN_FMT_QUIRK BIT(0)
+
 /* DSP hardware descriptor */
 struct sof_intel_dsp_desc {
 	int cores_num;
@@ -166,6 +169,7 @@ struct sof_intel_dsp_desc {
 	int ssp_base_offset;		/* base address of the SSPs */
 	u32 sdw_shim_base;
 	u32 sdw_alh_base;
+	u32 quirks;
 	bool (*check_sdw_irq)(struct snd_sof_dev *sdev);
 };
 
-- 
2.25.1

[PATCH 3/7] ASoC: SOF: Intel: hda-ctrl: apply symmetry for DPIB

[Pierre-Louis Bossart]

we use 'bus->use_posbuf && bus->posbuf.addr' in
hda_dsp_ctrl_init_chip(), use the same for hda_dsp_ctrl_stop_chip()

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Reviewed-by: Péter Ujfalusi <peter.ujfalusi@linux.intel.com>
---
 sound/soc/sof/intel/hda-ctrl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/sof/intel/hda-ctrl.c b/sound/soc/sof/intel/hda-ctrl.c
index fa5f0a718901..0c29bb196e59 100644
--- a/sound/soc/sof/intel/hda-ctrl.c
+++ b/sound/soc/sof/intel/hda-ctrl.c
@@ -353,7 +353,7 @@ void hda_dsp_ctrl_stop_chip(struct snd_sof_dev *sdev)
 	snd_hdac_bus_stop_cmd_io(bus);
 #endif
 	/* disable position buffer */
-	if (bus->posbuf.addr) {
+	if (bus->use_posbuf && bus->posbuf.addr) {
 		snd_sof_dsp_write(sdev, HDA_DSP_HDA_BAR,
 				  SOF_HDA_ADSP_DPLBASE, 0);
 		snd_sof_dsp_write(sdev, HDA_DSP_HDA_BAR,
-- 
2.25.1

[PATCH 4/7] ASoC: SOF: hda-stream: only enable DPIB if needed

[Pierre-Louis Bossart]

The existing code is inconsistent, we should only enable DPIB if the
'use_posbuf' field is true.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Reviewed-by: Péter Ujfalusi <peter.ujfalusi@linux.intel.com>
---
 sound/soc/sof/intel/hda-stream.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/sound/soc/sof/intel/hda-stream.c b/sound/soc/sof/intel/hda-stream.c
index 5f9eb5bdcdba..e910f68706d9 100644
--- a/sound/soc/sof/intel/hda-stream.c
+++ b/sound/soc/sof/intel/hda-stream.c
@@ -626,9 +626,10 @@ int hda_dsp_stream_hw_params(struct snd_sof_dev *sdev,
 			  sd_offset + SOF_HDA_ADSP_REG_CL_SD_BDLPU,
 			  upper_32_bits(hstream->bdl.addr));
 
-	/* enable position buffer */
-	if (!(snd_sof_dsp_read(sdev, HDA_DSP_HDA_BAR, SOF_HDA_ADSP_DPLBASE)
-				& SOF_HDA_ADSP_DPLBASE_ENABLE)) {
+	/* enable position buffer, if needed */
+	if (bus->use_posbuf && bus->posbuf.addr &&
+	    !(snd_sof_dsp_read(sdev, HDA_DSP_HDA_BAR, SOF_HDA_ADSP_DPLBASE)
+	      & SOF_HDA_ADSP_DPLBASE_ENABLE)) {
 		snd_sof_dsp_write(sdev, HDA_DSP_HDA_BAR, SOF_HDA_ADSP_DPUBASE,
 				  upper_32_bits(bus->posbuf.addr));
 		snd_sof_dsp_write(sdev, HDA_DSP_HDA_BAR, SOF_HDA_ADSP_DPLBASE,
-- 
2.25.1

[PATCH 5/7] ASoC: SOF: Intel: hda: add quirks for HDAudio DMA position information

[Pierre-Louis Bossart]

The code inherited from the Skylake driver does not seem to follow any
known hardware recommendations.

The only two recommended options are
a) use DPIB registers if VC1 traffic is not allowed
b) use DPIB DDR update if VC1 traffic is used

In all of SOF-based updated, VC1 is not supported so we can 'safely'
move to using DPIB registers only.

This patch keeps the legacy code, in case there was an undocumented
issue lost to history, and adds the DPIB DDR update for additional
debug.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Reviewed-by: Péter Ujfalusi <peter.ujfalusi@linux.intel.com>
---
 sound/soc/sof/intel/hda-pcm.c | 86 +++++++++++++++++++++++++----------
 sound/soc/sof/intel/hda.c     |  9 +++-
 sound/soc/sof/intel/hda.h     |  6 +++
 3 files changed, 75 insertions(+), 26 deletions(-)

diff --git a/sound/soc/sof/intel/hda-pcm.c b/sound/soc/sof/intel/hda-pcm.c
index 974383cd0440..d78aa5d8552d 100644
--- a/sound/soc/sof/intel/hda-pcm.c
+++ b/sound/soc/sof/intel/hda-pcm.c
@@ -202,38 +202,74 @@ snd_pcm_uframes_t hda_dsp_pcm_pointer(struct snd_sof_dev *sdev,
 		goto found;
 	}
 
-	/*
-	 * DPIB/posbuf position mode:
-	 * For Playback, Use DPIB register from HDA space which
-	 * reflects the actual data transferred.
-	 * For Capture, Use the position buffer for pointer, as DPIB
-	 * is not accurate enough, its update may be completed
-	 * earlier than the data written to DDR.
-	 */
-	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+	switch (sof_hda_position_quirk) {
+	case SOF_HDA_POSITION_QUIRK_USE_SKYLAKE_LEGACY:
+		/*
+		 * This legacy code, inherited from the Skylake driver,
+		 * mixes DPIB registers and DPIB DDR updates and
+		 * does not seem to follow any known hardware recommendations.
+		 * It's not clear e.g. why there is a different flow
+		 * for capture and playback, the only information that matters is
+		 * what traffic class is used, and on all SOF-enabled platforms
+		 * only VC0 is supported so the work-around was likely not necessary
+		 * and quite possibly wrong.
+		 */
+
+		/* DPIB/posbuf position mode:
+		 * For Playback, Use DPIB register from HDA space which
+		 * reflects the actual data transferred.
+		 * For Capture, Use the position buffer for pointer, as DPIB
+		 * is not accurate enough, its update may be completed
+		 * earlier than the data written to DDR.
+		 */
+		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+			pos = snd_sof_dsp_read(sdev, HDA_DSP_HDA_BAR,
+					       AZX_REG_VS_SDXDPIB_XBASE +
+					       (AZX_REG_VS_SDXDPIB_XINTERVAL *
+						hstream->index));
+		} else {
+			/*
+			 * For capture stream, we need more workaround to fix the
+			 * position incorrect issue:
+			 *
+			 * 1. Wait at least 20us before reading position buffer after
+			 * the interrupt generated(IOC), to make sure position update
+			 * happens on frame boundary i.e. 20.833uSec for 48KHz.
+			 * 2. Perform a dummy Read to DPIB register to flush DMA
+			 * position value.
+			 * 3. Read the DMA Position from posbuf. Now the readback
+			 * value should be >= period boundary.
+			 */
+			usleep_range(20, 21);
+			snd_sof_dsp_read(sdev, HDA_DSP_HDA_BAR,
+					 AZX_REG_VS_SDXDPIB_XBASE +
+					 (AZX_REG_VS_SDXDPIB_XINTERVAL *
+					  hstream->index));
+			pos = snd_hdac_stream_get_pos_posbuf(hstream);
+		}
+		break;
+	case SOF_HDA_POSITION_QUIRK_USE_DPIB_REGISTERS:
+		/*
+		 * In case VC1 traffic is disabled this is the recommended option
+		 */
 		pos = snd_sof_dsp_read(sdev, HDA_DSP_HDA_BAR,
 				       AZX_REG_VS_SDXDPIB_XBASE +
 				       (AZX_REG_VS_SDXDPIB_XINTERVAL *
 					hstream->index));
-	} else {
+		break;
+	case SOF_HDA_POSITION_QUIRK_USE_DPIB_DDR_UPDATE:
 		/*
-		 * For capture stream, we need more workaround to fix the
-		 * position incorrect issue:
-		 *
-		 * 1. Wait at least 20us before reading position buffer after
-		 * the interrupt generated(IOC), to make sure position update
-		 * happens on frame boundary i.e. 20.833uSec for 48KHz.
-		 * 2. Perform a dummy Read to DPIB register to flush DMA
-		 * position value.
-		 * 3. Read the DMA Position from posbuf. Now the readback
-		 * value should be >= period boundary.
+		 * This is the recommended option when VC1 is enabled.
+		 * While this isn't needed for SOF platforms it's added for
+		 * consistency and debug.
 		 */
-		usleep_range(20, 21);
-		snd_sof_dsp_read(sdev, HDA_DSP_HDA_BAR,
-				 AZX_REG_VS_SDXDPIB_XBASE +
-				 (AZX_REG_VS_SDXDPIB_XINTERVAL *
-				  hstream->index));
 		pos = snd_hdac_stream_get_pos_posbuf(hstream);
+		break;
+	default:
+		dev_err_once(sdev->dev, "hda_position_quirk value %d not supported\n",
+			     sof_hda_position_quirk);
+		pos = 0;
+		break;
 	}
 
 	if (pos >= hstream->bufsize)
diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c
index cfe026dbf124..dabbd5d908f6 100644
--- a/sound/soc/sof/intel/hda.c
+++ b/sound/soc/sof/intel/hda.c
@@ -432,6 +432,10 @@ MODULE_PARM_DESC(use_msi, "SOF HDA use PCI MSI mode");
 #define hda_use_msi	(1)
 #endif
 
+int sof_hda_position_quirk = SOF_HDA_POSITION_QUIRK_USE_DPIB_REGISTERS;
+module_param_named(position_quirk, sof_hda_position_quirk, int, 0444);
+MODULE_PARM_DESC(position_quirk, "SOF HDaudio position quirk");
+
 static char *hda_model;
 module_param(hda_model, charp, 0444);
 MODULE_PARM_DESC(hda_model, "Use the given HDA board model.");
@@ -610,7 +614,10 @@ static int hda_init(struct snd_sof_dev *sdev)
 	/* HDA bus init */
 	sof_hda_bus_init(bus, &pci->dev);
 
-	bus->use_posbuf = 1;
+	if (sof_hda_position_quirk == SOF_HDA_POSITION_QUIRK_USE_DPIB_REGISTERS)
+		bus->use_posbuf = 0;
+	else
+		bus->use_posbuf = 1;
 	bus->bdl_pos_adj = 0;
 	bus->sync_write = 1;
 
diff --git a/sound/soc/sof/intel/hda.h b/sound/soc/sof/intel/hda.h
index e2055b6c8139..cb71d9d5cf6c 100644
--- a/sound/soc/sof/intel/hda.h
+++ b/sound/soc/sof/intel/hda.h
@@ -740,4 +740,10 @@ struct sof_ipc_dai_config;
 int hda_ctrl_dai_widget_setup(struct snd_soc_dapm_widget *w, unsigned int quirk_flags);
 int hda_ctrl_dai_widget_free(struct snd_soc_dapm_widget *w, unsigned int quirk_flags);
 
+#define SOF_HDA_POSITION_QUIRK_USE_SKYLAKE_LEGACY	(0) /* previous implementation */
+#define SOF_HDA_POSITION_QUIRK_USE_DPIB_REGISTERS	(1) /* recommended if VC0 only */
+#define SOF_HDA_POSITION_QUIRK_USE_DPIB_DDR_UPDATE	(2) /* recommended with VC0 or VC1 */
+
+extern int sof_hda_position_quirk;
+
 #endif
-- 
2.25.1

[PATCH 6/7] ASoC: SOF: Intel: hda-dai: remove unused fields

[Pierre-Louis Bossart]

The existing code does not use the 'host_dma_id', 'link_dma_id',
'host_bps' fields remove them.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Kai Vehmanen <kai.vehmanen@intel.com>
Reviewed-by: Péter Ujfalusi <peter.ujfalusi@linux.intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
---
 sound/soc/sof/intel/hda-dai.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/sound/soc/sof/intel/hda-dai.c b/sound/soc/sof/intel/hda-dai.c
index 8c1d7ddb00e2..35ffb71116c6 100644
--- a/sound/soc/sof/intel/hda-dai.c
+++ b/sound/soc/sof/intel/hda-dai.c
@@ -21,8 +21,6 @@
 #endif
 
 struct hda_pipe_params {
-	u8 host_dma_id;
-	u8 link_dma_id;
 	u32 ch;
 	u32 s_freq;
 	u32 s_fmt;
@@ -30,7 +28,6 @@ struct hda_pipe_params {
 	snd_pcm_format_t format;
 	int link_index;
 	int stream;
-	unsigned int host_bps;
 	unsigned int link_bps;
 };
 
@@ -256,7 +253,6 @@ static int hda_link_hw_params(struct snd_pcm_substream *substream,
 	p_params.ch = params_channels(params);
 	p_params.s_freq = params_rate(params);
 	p_params.stream = substream->stream;
-	p_params.link_dma_id = stream_tag - 1;
 	p_params.link_index = link->index;
 	p_params.format = params_format(params);
 
-- 
2.25.1

[PATCH 7/7] ASoC: SOF: Intel: add comment on JasperLake support

[Pierre-Louis Bossart]

Explain why JasperLake is exposed in cnl.c instead of icl.c
No functionality change.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
---
 sound/soc/sof/intel/cnl.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/sound/soc/sof/intel/cnl.c b/sound/soc/sof/intel/cnl.c
index 04daaa6100f1..3da158d08980 100644
--- a/sound/soc/sof/intel/cnl.c
+++ b/sound/soc/sof/intel/cnl.c
@@ -358,6 +358,13 @@ const struct sof_intel_dsp_desc cnl_chip_info = {
 };
 EXPORT_SYMBOL_NS(cnl_chip_info, SND_SOC_SOF_INTEL_HDA_COMMON);
 
+/*
+ * JasperLake is technically derived from IceLake, and should be in
+ * described in icl.c. However since JasperLake was designed with
+ * two cores, it cannot support the IceLake-specific power-up sequences
+ * which rely on core3. To simplify, JasperLake uses the CannonLake ops and
+ * is described in cnl.c
+ */
 const struct sof_intel_dsp_desc jsl_chip_info = {
 	/* Jasperlake */
 	.cores_num = 2,
-- 
2.25.1

Re: [PATCH 0/7] ASoC: Intel: SOF: clarifications on hardware support

[Mark Brown]

On Tue, 7 Dec 2021 13:39:40 -0600, Pierre-Louis Bossart wrote:
> This patchset revisits the Intel hardware support in SOF. The HDAudio
> DMA position information was not following hardware recommended
> programming sequences (similar changes are already part of the HDaudio
> legacy driver), and the stream assignment applied a work-around that
> was only needed on specific versions of hardware. These changes are
> not tagged as 'Fixes' and don't need to be applied to -stable
> versions.
> 
> [...]

Applied to

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

Thanks!

[1/7] ASoC: SOF: Intel: ICL: move ICL-specific ops to icl.c
      commit: c697ef868f596aba7a5e90be8eb10bf4d4a98990
[2/7] ASoC: SOF: Intel: hda-stream: limit PROCEN workaround
      commit: a792bfc1c2bc4b5e2311edc62e0efe5adec5d079
[3/7] ASoC: SOF: Intel: hda-ctrl: apply symmetry for DPIB
      commit: 12ce213821b77242b2217d08850ff972e1fb50bb
[4/7] ASoC: SOF: hda-stream: only enable DPIB if needed
      commit: ae81d8fd57ff7d2b421c80f0f9426d9e775023b5
[5/7] ASoC: SOF: Intel: hda: add quirks for HDAudio DMA position information
      commit: 288fad2f71fa0b989c075d4984879c26d47cfb06
[6/7] ASoC: SOF: Intel: hda-dai: remove unused fields
      commit: 924631df4134d62b51a9442d97355eeba7ff613c
[7/7] ASoC: SOF: Intel: add comment on JasperLake support
      commit: 290a7c5509b6f14c28e959392f3cbc4d5b2c9318

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