[PATCH 0/4] ASoC: SOF: Intel: override mclk_id for ES8336 support

[PATCH 0/4] ASoC: SOF: Intel: override mclk_id for ES8336 support

[Pierre-Louis Bossart]

This patchset solves a known issue with ES8336 platforms wrt MCLK
selection. Most of the devices use the MCLK0 signal, but some devices
do use the MCLK1 signal.

The MCLK is defined in the topology, it would be a nightmare to
generate more topology files just for one MCLK difference. With a
minor extension to the intel-nhlt library, the MCLK information can be
found by parsing the NHLT table, and we can override the mclk_id at
boot time.

The only known issues for this platform remain the detection of GPIO
and microphone connections, currently only possible with manual
quirks.

Thanks to Eugene J. Markow for testing this patchset.

Pierre-Louis Bossart (4):
  ASoC: SOF: add quirk to override topology mclk_id
  ALSA: hda: intel-nhlt: add intel_nhlt_ssp_mclk_mask()
  ASoC: SOF: Intel: hda: override mclk_id after parsing NHLT SSP blob
  ASoC: SOF: Intel: hda: refine SSP count support

 include/sound/intel-nhlt.h    |  7 ++++
 sound/hda/intel-nhlt.c        | 61 +++++++++++++++++++++++++++++++++++
 sound/soc/sof/intel/hda.c     | 34 +++++++++++++++++++
 sound/soc/sof/intel/hda.h     |  2 ++
 sound/soc/sof/intel/mtl.c     |  2 +-
 sound/soc/sof/intel/tgl.c     |  8 ++---
 sound/soc/sof/ipc3-topology.c |  7 ++++
 sound/soc/sof/sof-priv.h      |  4 +++
 8 files changed, 120 insertions(+), 5 deletions(-)

-- 
2.34.1

[PATCH 1/4] ASoC: SOF: add quirk to override topology mclk_id

[Pierre-Louis Bossart]

Some Intel-based platforms rely on a topology file that hard-codes the
use of MCLK0. This is incorrect in 10% of the cases. Rather than
generating yet another set of topology files, this patch adds a kernel
module parameter to override the topology value.

In hindsight, we should never have allowed mclks to be specified in
topology, this is a hardware-level information that should not have
been visible in the topology.

Future patches will try to set this value automagically, e.g. by
parsing the NHLT content.

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/hda.c     | 11 +++++++++++
 sound/soc/sof/ipc3-topology.c |  7 +++++++
 sound/soc/sof/sof-priv.h      |  4 ++++
 3 files changed, 22 insertions(+)

diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c
index 6d4ecbe14adf3..ada2e67757494 100644
--- a/sound/soc/sof/intel/hda.c
+++ b/sound/soc/sof/intel/hda.c
@@ -376,6 +376,10 @@ static int dmic_num_override = -1;
 module_param_named(dmic_num, dmic_num_override, int, 0444);
 MODULE_PARM_DESC(dmic_num, "SOF HDA DMIC number");
 
+static int mclk_id_override = -1;
+module_param_named(mclk_id, mclk_id_override, int, 0444);
+MODULE_PARM_DESC(mclk_id, "SOF SSP mclk_id");
+
 #if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA)
 static bool hda_codec_use_common_hdmi = IS_ENABLED(CONFIG_SND_HDA_CODEC_HDMI);
 module_param_named(use_common_hdmi, hda_codec_use_common_hdmi, bool, 0444);
@@ -1565,6 +1569,13 @@ struct snd_soc_acpi_mach *hda_machine_select(struct snd_sof_dev *sdev)
 
 			sof_pdata->tplg_filename = tplg_filename;
 		}
+
+		/* check if mclk_id should be modified from topology defaults */
+		if (mclk_id_override >= 0) {
+			dev_info(sdev->dev, "Overriding topology with MCLK %d from kernel_parameter\n", mclk_id_override);
+			sdev->mclk_id_override = true;
+			sdev->mclk_id_quirk = mclk_id_override;
+		}
 	}
 
 	/*
diff --git a/sound/soc/sof/ipc3-topology.c b/sound/soc/sof/ipc3-topology.c
index 65923e7a5976f..a39b43850f0ed 100644
--- a/sound/soc/sof/ipc3-topology.c
+++ b/sound/soc/sof/ipc3-topology.c
@@ -1249,6 +1249,7 @@ static int sof_link_afe_load(struct snd_soc_component *scomp, struct snd_sof_dai
 static int sof_link_ssp_load(struct snd_soc_component *scomp, struct snd_sof_dai_link *slink,
 			     struct sof_ipc_dai_config *config, struct snd_sof_dai *dai)
 {
+	struct snd_sof_dev *sdev = snd_soc_component_get_drvdata(scomp);
 	struct snd_soc_tplg_hw_config *hw_config = slink->hw_configs;
 	struct sof_dai_private_data *private = dai->private;
 	u32 size = sizeof(*config);
@@ -1273,6 +1274,12 @@ static int sof_link_ssp_load(struct snd_soc_component *scomp, struct snd_sof_dai
 
 		config[i].hdr.size = size;
 
+		if (sdev->mclk_id_override) {
+			dev_dbg(scomp->dev, "tplg: overriding topology mclk_id %d by quirk %d\n",
+				config[i].ssp.mclk_id, sdev->mclk_id_quirk);
+			config[i].ssp.mclk_id = sdev->mclk_id_quirk;
+		}
+
 		/* copy differentiating hw configs to ipc structs */
 		config[i].ssp.mclk_rate = le32_to_cpu(hw_config[i].mclk_rate);
 		config[i].ssp.bclk_rate = le32_to_cpu(hw_config[i].bclk_rate);
diff --git a/sound/soc/sof/sof-priv.h b/sound/soc/sof/sof-priv.h
index 33165299a20f1..de08825915b35 100644
--- a/sound/soc/sof/sof-priv.h
+++ b/sound/soc/sof/sof-priv.h
@@ -601,6 +601,10 @@ struct snd_sof_dev {
 	/* to protect the ipc_rx_handler_list  and  dsp_state_handler_list list */
 	struct mutex client_event_handler_mutex;
 
+	/* quirks to override topology values */
+	bool mclk_id_override;
+	u16  mclk_id_quirk; /* same size as in IPC3 definitions */
+
 	void *private;			/* core does not touch this */
 };
 
-- 
2.34.1

[PATCH 2/4] ALSA: hda: intel-nhlt: add intel_nhlt_ssp_mclk_mask()

[Pierre-Louis Bossart]

SOF topologies hard-code the MCLK used for SSP connections. That was a
bad idea in hindsight, this information should really come from BIOS
and/or machine driver.

This patch introduces a helper to scan all SSP endpoints connected to
a codec, and all formats to see what MCLK is used. When BIT(0) of the
mdivc offset if set in the SSP blob, MCLK0 is used, and likewise when
BIT(1) is set MCLK1 is used.

The case where both MCLKs are used is possible but has never been seen
in practice so should be treated as an error by the caller.

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>
---
 include/sound/intel-nhlt.h |  7 +++++
 sound/hda/intel-nhlt.c     | 61 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 68 insertions(+)

diff --git a/include/sound/intel-nhlt.h b/include/sound/intel-nhlt.h
index 3d5cf201cd802..53470d6a28d65 100644
--- a/include/sound/intel-nhlt.h
+++ b/include/sound/intel-nhlt.h
@@ -136,6 +136,8 @@ bool intel_nhlt_has_endpoint_type(struct nhlt_acpi_table *nhlt, u8 link_type);
 
 int intel_nhlt_ssp_endpoint_mask(struct nhlt_acpi_table *nhlt, u8 device_type);
 
+int intel_nhlt_ssp_mclk_mask(struct nhlt_acpi_table *nhlt, int ssp_num);
+
 struct nhlt_specific_cfg *
 intel_nhlt_get_endpoint_blob(struct device *dev, struct nhlt_acpi_table *nhlt,
 			     u32 bus_id, u8 link_type, u8 vbps, u8 bps,
@@ -169,6 +171,11 @@ static inline int intel_nhlt_ssp_endpoint_mask(struct nhlt_acpi_table *nhlt, u8
 	return 0;
 }
 
+static inline int intel_nhlt_ssp_mclk_mask(struct nhlt_acpi_table *nhlt, int ssp_num)
+{
+	return 0;
+}
+
 static inline struct nhlt_specific_cfg *
 intel_nhlt_get_endpoint_blob(struct device *dev, struct nhlt_acpi_table *nhlt,
 			     u32 bus_id, u8 link_type, u8 vbps, u8 bps,
diff --git a/sound/hda/intel-nhlt.c b/sound/hda/intel-nhlt.c
index 13bb0ccfb36c0..0323aedb6ecf4 100644
--- a/sound/hda/intel-nhlt.c
+++ b/sound/hda/intel-nhlt.c
@@ -157,6 +157,67 @@ int intel_nhlt_ssp_endpoint_mask(struct nhlt_acpi_table *nhlt, u8 device_type)
 }
 EXPORT_SYMBOL(intel_nhlt_ssp_endpoint_mask);
 
+#define SSP_BLOB_V1_0_SIZE		84
+#define SSP_BLOB_V1_0_MDIVC_OFFSET	19 /* offset in u32 */
+#define SSP_BLOB_V1_5_SIZE		96
+#define SSP_BLOB_V1_5_MDIVC_OFFSET	21 /* offset in u32 */
+#define SSP_BLOB_VER_1_5		0xEE000105
+#define SSP_BLOB_V2_0_MDIVC_OFFSET	20 /* offset in u32 */
+#define SSP_BLOB_VER_2_0		0xEE000200
+
+int intel_nhlt_ssp_mclk_mask(struct nhlt_acpi_table *nhlt, int ssp_num)
+{
+	struct nhlt_endpoint *epnt;
+	struct nhlt_fmt *fmt;
+	struct nhlt_fmt_cfg *cfg;
+	int mclk_mask = 0;
+	int i, j;
+
+	if (!nhlt)
+		return 0;
+
+	epnt = (struct nhlt_endpoint *)nhlt->desc;
+	for (i = 0; i < nhlt->endpoint_count; i++) {
+
+		/* we only care about endpoints connected to an audio codec over SSP */
+		if (epnt->linktype == NHLT_LINK_SSP &&
+		    epnt->device_type == NHLT_DEVICE_I2S &&
+		    epnt->virtual_bus_id == ssp_num) {
+
+			fmt = (struct nhlt_fmt *)(epnt->config.caps + epnt->config.size);
+			cfg = fmt->fmt_config;
+
+			/*
+			 * In theory all formats should use the same MCLK but it doesn't hurt to
+			 * double-check that the configuration is consistent
+			 */
+			for (j = 0; j < fmt->fmt_count; j++) {
+				u32 *blob;
+				int mdivc_offset;
+
+				if (cfg->config.size >= SSP_BLOB_V1_0_SIZE) {
+					blob = (u32 *)cfg->config.caps;
+
+					if (blob[1] == SSP_BLOB_VER_2_0)
+						mdivc_offset = SSP_BLOB_V2_0_MDIVC_OFFSET;
+					else if (blob[1] == SSP_BLOB_VER_1_5)
+						mdivc_offset = SSP_BLOB_V1_5_MDIVC_OFFSET;
+					else
+						mdivc_offset = SSP_BLOB_V1_0_MDIVC_OFFSET;
+
+					mclk_mask |=  blob[mdivc_offset] & GENMASK(1, 0);
+				}
+
+				cfg = (struct nhlt_fmt_cfg *)(cfg->config.caps + cfg->config.size);
+			}
+		}
+		epnt = (struct nhlt_endpoint *)((u8 *)epnt + epnt->length);
+	}
+
+	return mclk_mask;
+}
+EXPORT_SYMBOL(intel_nhlt_ssp_mclk_mask);
+
 static struct nhlt_specific_cfg *
 nhlt_get_specific_cfg(struct device *dev, struct nhlt_fmt *fmt, u8 num_ch,
 		      u32 rate, u8 vbps, u8 bps)
-- 
2.34.1

[PATCH 3/4] ASoC: SOF: Intel: hda: override mclk_id after parsing NHLT SSP blob

[Pierre-Louis Bossart]

The NHLT is already used to determine which SSP is connected to an
audio codec, we can parse the SSP blob to get the mclk_id from NHLT.

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/hda.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c
index ada2e67757494..9048cc842d3f2 100644
--- a/sound/soc/sof/intel/hda.c
+++ b/sound/soc/sof/intel/hda.c
@@ -753,6 +753,18 @@ static int check_nhlt_ssp_mask(struct snd_sof_dev *sdev)
 	return ssp_mask;
 }
 
+static int check_nhlt_ssp_mclk_mask(struct snd_sof_dev *sdev, int ssp_num)
+{
+	struct sof_intel_hda_dev *hdev = sdev->pdata->hw_pdata;
+	struct nhlt_acpi_table *nhlt;
+
+	nhlt = hdev->nhlt;
+	if (!nhlt)
+		return 0;
+
+	return intel_nhlt_ssp_mclk_mask(nhlt, ssp_num);
+}
+
 #if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA) || IS_ENABLED(CONFIG_SND_SOC_SOF_INTEL_SOUNDWIRE)
 
 static const char *fixup_tplg_name(struct snd_sof_dev *sdev,
@@ -1533,6 +1545,7 @@ struct snd_soc_acpi_mach *hda_machine_select(struct snd_sof_dev *sdev)
 		    mach->mach_params.i2s_link_mask) {
 			const struct sof_intel_dsp_desc *chip = get_chip_info(sdev->pdata);
 			int ssp_num;
+			int mclk_mask;
 
 			if (hweight_long(mach->mach_params.i2s_link_mask) > 1 &&
 			    !(mach->tplg_quirk_mask & SND_SOC_ACPI_TPLG_INTEL_SSP_MSB))
@@ -1557,6 +1570,16 @@ struct snd_soc_acpi_mach *hda_machine_select(struct snd_sof_dev *sdev)
 
 			sof_pdata->tplg_filename = tplg_filename;
 			add_extension = true;
+
+			mclk_mask = check_nhlt_ssp_mclk_mask(sdev, ssp_num);
+
+			dev_dbg(sdev->dev, "MCLK mask %#x found in NHLT\n", mclk_mask);
+
+			if (mclk_mask && mclk_mask != GENMASK(1, 0)) {
+				dev_info(sdev->dev, "Overriding topology with MCLK mask %#x from NHLT\n", mclk_mask);
+				sdev->mclk_id_override = true;
+				sdev->mclk_id_quirk = (mclk_mask & BIT(0)) ? 0 : 1;
+			}
 		}
 
 		if (tplg_fixup && add_extension) {
-- 
2.34.1

[PATCH 4/4] ASoC: SOF: Intel: hda: refine SSP count support

[Pierre-Louis Bossart]

The SSP count is incorrect for TGL and MTL devices, the SSP count is
limited to 3 (I2SPC parameter in the Integration HAS).

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Reviewed-by: Péter Ujfalusi <peter.ujfalusi@linux.intel.com>
---
 sound/soc/sof/intel/hda.h | 2 ++
 sound/soc/sof/intel/mtl.c | 2 +-
 sound/soc/sof/intel/tgl.c | 8 ++++----
 3 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/sound/soc/sof/intel/hda.h b/sound/soc/sof/intel/hda.h
index 5ef3e8775e364..34c5ebd9556c1 100644
--- a/sound/soc/sof/intel/hda.h
+++ b/sound/soc/sof/intel/hda.h
@@ -441,6 +441,8 @@
 #define APL_SSP_COUNT		6
 #define CNL_SSP_COUNT		3
 #define ICL_SSP_COUNT		6
+#define TGL_SSP_COUNT		3
+#define MTL_SSP_COUNT		3
 
 /* SSP Registers */
 #define SSP_SSC1_OFFSET		0x4
diff --git a/sound/soc/sof/intel/mtl.c b/sound/soc/sof/intel/mtl.c
index 96239ebb1eedb..27add99b7f947 100644
--- a/sound/soc/sof/intel/mtl.c
+++ b/sound/soc/sof/intel/mtl.c
@@ -782,7 +782,7 @@ const struct sof_intel_dsp_desc mtl_chip_info = {
 	.ipc_ctl = MTL_DSP_REG_HFIPCXCTL,
 	.rom_status_reg = MTL_DSP_ROM_STS,
 	.rom_init_timeout	= 300,
-	.ssp_count = ICL_SSP_COUNT,
+	.ssp_count = MTL_SSP_COUNT,
 	.ssp_base_offset = CNL_SSP_BASE_OFFSET,
 	.sdw_shim_base = SDW_SHIM_BASE_ACE,
 	.sdw_alh_base = SDW_ALH_BASE_ACE,
diff --git a/sound/soc/sof/intel/tgl.c b/sound/soc/sof/intel/tgl.c
index 6dfb4786c7824..2119973716809 100644
--- a/sound/soc/sof/intel/tgl.c
+++ b/sound/soc/sof/intel/tgl.c
@@ -121,7 +121,7 @@ const struct sof_intel_dsp_desc tgl_chip_info = {
 	.ipc_ctl = CNL_DSP_REG_HIPCCTL,
 	.rom_status_reg = HDA_DSP_SRAM_REG_ROM_STATUS,
 	.rom_init_timeout	= 300,
-	.ssp_count = ICL_SSP_COUNT,
+	.ssp_count = TGL_SSP_COUNT,
 	.ssp_base_offset = CNL_SSP_BASE_OFFSET,
 	.sdw_shim_base = SDW_SHIM_BASE,
 	.sdw_alh_base = SDW_ALH_BASE,
@@ -144,7 +144,7 @@ const struct sof_intel_dsp_desc tglh_chip_info = {
 	.ipc_ctl = CNL_DSP_REG_HIPCCTL,
 	.rom_status_reg = HDA_DSP_SRAM_REG_ROM_STATUS,
 	.rom_init_timeout	= 300,
-	.ssp_count = ICL_SSP_COUNT,
+	.ssp_count = TGL_SSP_COUNT,
 	.ssp_base_offset = CNL_SSP_BASE_OFFSET,
 	.sdw_shim_base = SDW_SHIM_BASE,
 	.sdw_alh_base = SDW_ALH_BASE,
@@ -167,7 +167,7 @@ const struct sof_intel_dsp_desc ehl_chip_info = {
 	.ipc_ctl = CNL_DSP_REG_HIPCCTL,
 	.rom_status_reg = HDA_DSP_SRAM_REG_ROM_STATUS,
 	.rom_init_timeout	= 300,
-	.ssp_count = ICL_SSP_COUNT,
+	.ssp_count = TGL_SSP_COUNT,
 	.ssp_base_offset = CNL_SSP_BASE_OFFSET,
 	.sdw_shim_base = SDW_SHIM_BASE,
 	.sdw_alh_base = SDW_ALH_BASE,
@@ -190,7 +190,7 @@ const struct sof_intel_dsp_desc adls_chip_info = {
 	.ipc_ctl = CNL_DSP_REG_HIPCCTL,
 	.rom_status_reg = HDA_DSP_SRAM_REG_ROM_STATUS,
 	.rom_init_timeout	= 300,
-	.ssp_count = ICL_SSP_COUNT,
+	.ssp_count = TGL_SSP_COUNT,
 	.ssp_base_offset = CNL_SSP_BASE_OFFSET,
 	.sdw_shim_base = SDW_SHIM_BASE,
 	.sdw_alh_base = SDW_ALH_BASE,
-- 
2.34.1

Re: [PATCH 2/4] ALSA: hda: intel-nhlt: add intel_nhlt_ssp_mclk_mask()

[Takashi Iwai]

On Mon, 22 Aug 2022 20:59:09 +0200,
Pierre-Louis Bossart wrote:
> 
> +#define SSP_BLOB_V1_0_SIZE		84
> +#define SSP_BLOB_V1_0_MDIVC_OFFSET	19 /* offset in u32 */
> +#define SSP_BLOB_V1_5_SIZE		96
> +#define SSP_BLOB_V1_5_MDIVC_OFFSET	21 /* offset in u32 */

This is 84 in bytes, which is equal with SSP_BLOB_V1_0_size.
So...

> +			for (j = 0; j < fmt->fmt_count; j++) {
> +				u32 *blob;
> +				int mdivc_offset;
> +
> +				if (cfg->config.size >= SSP_BLOB_V1_0_SIZE) {
> +					blob = (u32 *)cfg->config.caps;

... the size check is >= 84.  If cfg->config.size==84, it may be an
out-of-bound read at blob[SSP_BLOB_V1_5_MDIVC_OFFSET]?

I don't think this would really matter in practice, but it's better to
have a proper check, of course.


thanks,

Takashi

Re: [PATCH 2/4] ALSA: hda: intel-nhlt: add intel_nhlt_ssp_mclk_mask()

[Amadeusz Sławiński]

On 8/22/2022 8:59 PM, Pierre-Louis Bossart wrote:
> SOF topologies hard-code the MCLK used for SSP connections. That was a
> bad idea in hindsight, this information should really come from BIOS
> and/or machine driver.
> 
> This patch introduces a helper to scan all SSP endpoints connected to
> a codec, and all formats to see what MCLK is used. When BIT(0) of the
> mdivc offset if set in the SSP blob, MCLK0 is used, and likewise when
> BIT(1) is set MCLK1 is used.
> 
> The case where both MCLKs are used is possible but has never been seen
> in practice so should be treated as an error by the caller.
> 
> 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>
> ---
>   include/sound/intel-nhlt.h |  7 +++++
>   sound/hda/intel-nhlt.c     | 61 ++++++++++++++++++++++++++++++++++++++
>   2 files changed, 68 insertions(+)
> 
> diff --git a/include/sound/intel-nhlt.h b/include/sound/intel-nhlt.h
> index 3d5cf201cd802..53470d6a28d65 100644
> --- a/include/sound/intel-nhlt.h
> +++ b/include/sound/intel-nhlt.h
> @@ -136,6 +136,8 @@ bool intel_nhlt_has_endpoint_type(struct nhlt_acpi_table *nhlt, u8 link_type);
>   
>   int intel_nhlt_ssp_endpoint_mask(struct nhlt_acpi_table *nhlt, u8 device_type);
>   
> +int intel_nhlt_ssp_mclk_mask(struct nhlt_acpi_table *nhlt, int ssp_num);
> +
>   struct nhlt_specific_cfg *
>   intel_nhlt_get_endpoint_blob(struct device *dev, struct nhlt_acpi_table *nhlt,
>   			     u32 bus_id, u8 link_type, u8 vbps, u8 bps,
> @@ -169,6 +171,11 @@ static inline int intel_nhlt_ssp_endpoint_mask(struct nhlt_acpi_table *nhlt, u8
>   	return 0;
>   }
>   
> +static inline int intel_nhlt_ssp_mclk_mask(struct nhlt_acpi_table *nhlt, int ssp_num)
> +{
> +	return 0;
> +}
> +
>   static inline struct nhlt_specific_cfg *
>   intel_nhlt_get_endpoint_blob(struct device *dev, struct nhlt_acpi_table *nhlt,
>   			     u32 bus_id, u8 link_type, u8 vbps, u8 bps,
> diff --git a/sound/hda/intel-nhlt.c b/sound/hda/intel-nhlt.c
> index 13bb0ccfb36c0..0323aedb6ecf4 100644
> --- a/sound/hda/intel-nhlt.c
> +++ b/sound/hda/intel-nhlt.c
> @@ -157,6 +157,67 @@ int intel_nhlt_ssp_endpoint_mask(struct nhlt_acpi_table *nhlt, u8 device_type)
>   }
>   EXPORT_SYMBOL(intel_nhlt_ssp_endpoint_mask);
>   
> +#define SSP_BLOB_V1_0_SIZE		84
> +#define SSP_BLOB_V1_0_MDIVC_OFFSET	19 /* offset in u32 */
> +#define SSP_BLOB_V1_5_SIZE		96
> +#define SSP_BLOB_V1_5_MDIVC_OFFSET	21 /* offset in u32 */
> +#define SSP_BLOB_VER_1_5		0xEE000105
> +#define SSP_BLOB_V2_0_MDIVC_OFFSET	20 /* offset in u32 */
> +#define SSP_BLOB_VER_2_0		0xEE000200
> +
> +int intel_nhlt_ssp_mclk_mask(struct nhlt_acpi_table *nhlt, int ssp_num)
> +{
> +	struct nhlt_endpoint *epnt;
> +	struct nhlt_fmt *fmt;
> +	struct nhlt_fmt_cfg *cfg;
> +	int mclk_mask = 0;
> +	int i, j;
> +
> +	if (!nhlt)
> +		return 0;
> +
> +	epnt = (struct nhlt_endpoint *)nhlt->desc;
> +	for (i = 0; i < nhlt->endpoint_count; i++) {
> +
> +		/* we only care about endpoints connected to an audio codec over SSP */
> +		if (epnt->linktype == NHLT_LINK_SSP &&
> +		    epnt->device_type == NHLT_DEVICE_I2S &&
> +		    epnt->virtual_bus_id == ssp_num) {

if (epnt->linktype != NHLT_LINK_SSP ||
     epnt->device_type != NHLT_DEVICE_I2S ||
     epnt->virtual_bus_id != ssp_num)
	continue;

and then you can remove one indentation level below?

> +
> +			fmt = (struct nhlt_fmt *)(epnt->config.caps + epnt->config.size);
> +			cfg = fmt->fmt_config;
> +
> +			/*
> +			 * In theory all formats should use the same MCLK but it doesn't hurt to
> +			 * double-check that the configuration is consistent
> +			 */
> +			for (j = 0; j < fmt->fmt_count; j++) {
> +				u32 *blob;
> +				int mdivc_offset;
> +
> +				if (cfg->config.size >= SSP_BLOB_V1_0_SIZE) {
> +					blob = (u32 *)cfg->config.caps;
> +
> +					if (blob[1] == SSP_BLOB_VER_2_0)
> +						mdivc_offset = SSP_BLOB_V2_0_MDIVC_OFFSET;
> +					else if (blob[1] == SSP_BLOB_VER_1_5)
> +						mdivc_offset = SSP_BLOB_V1_5_MDIVC_OFFSET;
> +					else
> +						mdivc_offset = SSP_BLOB_V1_0_MDIVC_OFFSET;
> +
> +					mclk_mask |=  blob[mdivc_offset] & GENMASK(1, 0);
> +				}
> +
> +				cfg = (struct nhlt_fmt_cfg *)(cfg->config.caps + cfg->config.size);
> +			}
> +		}
> +		epnt = (struct nhlt_endpoint *)((u8 *)epnt + epnt->length);
> +	}
> +
> +	return mclk_mask;

Although I understand that it is relegated to the caller, but if both 
mclk being set is considered an error maybe add some kind of check here 
instead and free callers from having to remember about it?

if (hweight_long(mclk_mask) != 1)
	return -EINVAL;

return mclk_mask;

> +}
> +EXPORT_SYMBOL(intel_nhlt_ssp_mclk_mask);
> +
>   static struct nhlt_specific_cfg *
>   nhlt_get_specific_cfg(struct device *dev, struct nhlt_fmt *fmt, u8 num_ch,
>   		      u32 rate, u8 vbps, u8 bps)

Re: [PATCH 2/4] ALSA: hda: intel-nhlt: add intel_nhlt_ssp_mclk_mask()

[Pierre-Louis Bossart]

Hi Takashi,

>> +#define SSP_BLOB_V1_0_SIZE		84
>> +#define SSP_BLOB_V1_0_MDIVC_OFFSET	19 /* offset in u32 */
>> +#define SSP_BLOB_V1_5_SIZE		96
>> +#define SSP_BLOB_V1_5_MDIVC_OFFSET	21 /* offset in u32 */
> 
> This is 84 in bytes, which is equal with SSP_BLOB_V1_0_size.
> So...
> 
>> +			for (j = 0; j < fmt->fmt_count; j++) {
>> +				u32 *blob;
>> +				int mdivc_offset;
>> +
>> +				if (cfg->config.size >= SSP_BLOB_V1_0_SIZE) {
>> +					blob = (u32 *)cfg->config.caps;
> 
> ... the size check is >= 84.  If cfg->config.size==84, it may be an
> out-of-bound read at blob[SSP_BLOB_V1_5_MDIVC_OFFSET]?
> 
> I don't think this would really matter in practice, but it's better to
> have a proper check, of course.

The check was intended to be a minimal check but you're right that it
doesn't cover the 1.5 case.

it might make more sense to first make sure we have enough space to read
the version and then check for an exact match between expected size and
actual size before reading the mdivc value.

Will fix, thanks for the feedback.
-Pierre

Re: [PATCH 2/4] ALSA: hda: intel-nhlt: add intel_nhlt_ssp_mclk_mask()

[Pierre-Louis Bossart]

Hi Amadeusz,

>> +int intel_nhlt_ssp_mclk_mask(struct nhlt_acpi_table *nhlt, int ssp_num)
>> +{
>> +    struct nhlt_endpoint *epnt;
>> +    struct nhlt_fmt *fmt;
>> +    struct nhlt_fmt_cfg *cfg;
>> +    int mclk_mask = 0;
>> +    int i, j;
>> +
>> +    if (!nhlt)
>> +        return 0;
>> +
>> +    epnt = (struct nhlt_endpoint *)nhlt->desc;
>> +    for (i = 0; i < nhlt->endpoint_count; i++) {
>> +
>> +        /* we only care about endpoints connected to an audio codec
>> over SSP */
>> +        if (epnt->linktype == NHLT_LINK_SSP &&
>> +            epnt->device_type == NHLT_DEVICE_I2S &&
>> +            epnt->virtual_bus_id == ssp_num) {
> 
> if (epnt->linktype != NHLT_LINK_SSP ||
>     epnt->device_type != NHLT_DEVICE_I2S ||
>     epnt->virtual_bus_id != ssp_num)
>     continue;
> 
> and then you can remove one indentation level below?


Would that work? We still need to move the epnt pointer:

epnt = (struct nhlt_endpoint *)((u8 *)epnt + epnt->length);

and moving this in the endpoint_count loop would be ugly as well.


>> +
>> +            fmt = (struct nhlt_fmt *)(epnt->config.caps +
>> epnt->config.size);
>> +            cfg = fmt->fmt_config;
>> +
>> +            /*
>> +             * In theory all formats should use the same MCLK but it
>> doesn't hurt to
>> +             * double-check that the configuration is consistent
>> +             */
>> +            for (j = 0; j < fmt->fmt_count; j++) {
>> +                u32 *blob;
>> +                int mdivc_offset;
>> +
>> +                if (cfg->config.size >= SSP_BLOB_V1_0_SIZE) {
>> +                    blob = (u32 *)cfg->config.caps;
>> +
>> +                    if (blob[1] == SSP_BLOB_VER_2_0)
>> +                        mdivc_offset = SSP_BLOB_V2_0_MDIVC_OFFSET;
>> +                    else if (blob[1] == SSP_BLOB_VER_1_5)
>> +                        mdivc_offset = SSP_BLOB_V1_5_MDIVC_OFFSET;
>> +                    else
>> +                        mdivc_offset = SSP_BLOB_V1_0_MDIVC_OFFSET;
>> +
>> +                    mclk_mask |=  blob[mdivc_offset] & GENMASK(1, 0);
>> +                }
>> +
>> +                cfg = (struct nhlt_fmt_cfg *)(cfg->config.caps +
>> cfg->config.size);
>> +            }
>> +        }
>> +        epnt = (struct nhlt_endpoint *)((u8 *)epnt + epnt->length);
>> +    }
>> +
>> +    return mclk_mask;
> 
> Although I understand that it is relegated to the caller, but if both
> mclk being set is considered an error maybe add some kind of check here
> instead and free callers from having to remember about it?
> 
> if (hweight_long(mclk_mask) != 1)
>     return -EINVAL;
> 
> return mclk_mask;

I went back and forth multiple times on this one. I can't figure out if
this would be a bug or a feature, it could be e.g. a test capability and
it's supported in hardware. I decided to make the decision in the caller
rather than a lower level in the library.

If the tools used to generate NHLT don't support this multi-MCLK mode
then we could indeed move the test here.

> 
>> +}
>> +EXPORT_SYMBOL(intel_nhlt_ssp_mclk_mask);
>> +
>>   static struct nhlt_specific_cfg *
>>   nhlt_get_specific_cfg(struct device *dev, struct nhlt_fmt *fmt, u8
>> num_ch,
>>                 u32 rate, u8 vbps, u8 bps)
> 

Re: [PATCH 2/4] ALSA: hda: intel-nhlt: add intel_nhlt_ssp_mclk_mask()

[Amadeusz Sławiński]

On 8/23/2022 10:52 AM, Pierre-Louis Bossart wrote:
> Hi Amadeusz,
> 
>>> +int intel_nhlt_ssp_mclk_mask(struct nhlt_acpi_table *nhlt, int ssp_num)
>>> +{
>>> +    struct nhlt_endpoint *epnt;
>>> +    struct nhlt_fmt *fmt;
>>> +    struct nhlt_fmt_cfg *cfg;
>>> +    int mclk_mask = 0;
>>> +    int i, j;
>>> +
>>> +    if (!nhlt)
>>> +        return 0;
>>> +
>>> +    epnt = (struct nhlt_endpoint *)nhlt->desc;
>>> +    for (i = 0; i < nhlt->endpoint_count; i++) {
>>> +
>>> +        /* we only care about endpoints connected to an audio codec
>>> over SSP */
>>> +        if (epnt->linktype == NHLT_LINK_SSP &&
>>> +            epnt->device_type == NHLT_DEVICE_I2S &&
>>> +            epnt->virtual_bus_id == ssp_num) {
>>
>> if (epnt->linktype != NHLT_LINK_SSP ||
>>      epnt->device_type != NHLT_DEVICE_I2S ||
>>      epnt->virtual_bus_id != ssp_num)
>>      continue;
>>
>> and then you can remove one indentation level below?
> 
> 
> Would that work? We still need to move the epnt pointer:
> 
> epnt = (struct nhlt_endpoint *)((u8 *)epnt + epnt->length);
> 
> and moving this in the endpoint_count loop would be ugly as well.
> 
> 

Another solution would be goto to skip, but that also seems ugly :/
I guess it can stay as it is then.

>>> +
>>> +            fmt = (struct nhlt_fmt *)(epnt->config.caps +
>>> epnt->config.size);
>>> +            cfg = fmt->fmt_config;
>>> +
>>> +            /*
>>> +             * In theory all formats should use the same MCLK but it
>>> doesn't hurt to
>>> +             * double-check that the configuration is consistent
>>> +             */
>>> +            for (j = 0; j < fmt->fmt_count; j++) {
>>> +                u32 *blob;
>>> +                int mdivc_offset;
>>> +
>>> +                if (cfg->config.size >= SSP_BLOB_V1_0_SIZE) {
>>> +                    blob = (u32 *)cfg->config.caps;
>>> +
>>> +                    if (blob[1] == SSP_BLOB_VER_2_0)
>>> +                        mdivc_offset = SSP_BLOB_V2_0_MDIVC_OFFSET;
>>> +                    else if (blob[1] == SSP_BLOB_VER_1_5)
>>> +                        mdivc_offset = SSP_BLOB_V1_5_MDIVC_OFFSET;
>>> +                    else
>>> +                        mdivc_offset = SSP_BLOB_V1_0_MDIVC_OFFSET;
>>> +
>>> +                    mclk_mask |=  blob[mdivc_offset] & GENMASK(1, 0);

One more thing, where does this GENMASK come from, as far as I can tell 
HW specifies and FW uses one bit field to signal that MCLK is enabled? 
(mdivc is simply a value written to HW register to configure it).

>>> +                }
>>> +
>>> +                cfg = (struct nhlt_fmt_cfg *)(cfg->config.caps +
>>> cfg->config.size);
>>> +            }
>>> +        }
>>> +        epnt = (struct nhlt_endpoint *)((u8 *)epnt + epnt->length);
>>> +    }
>>> +
>>> +    return mclk_mask;
>>
>> Although I understand that it is relegated to the caller, but if both
>> mclk being set is considered an error maybe add some kind of check here
>> instead and free callers from having to remember about it?
>>
>> if (hweight_long(mclk_mask) != 1)
>>      return -EINVAL;
>>
>> return mclk_mask;
> 
> I went back and forth multiple times on this one. I can't figure out if
> this would be a bug or a feature, it could be e.g. a test capability and
> it's supported in hardware. I decided to make the decision in the caller
> rather than a lower level in the library.
> 
> If the tools used to generate NHLT don't support this multi-MCLK mode
> then we could indeed move the test here.
> 

Considering comment I added above I've asked Czarek to also check this 
series. I'm not sure it even makes sense to name the field "_mask" when 
it is one bit...

>>
>>> +}
>>> +EXPORT_SYMBOL(intel_nhlt_ssp_mclk_mask);
>>> +
>>>    static struct nhlt_specific_cfg *
>>>    nhlt_get_specific_cfg(struct device *dev, struct nhlt_fmt *fmt, u8
>>> num_ch,
>>>                  u32 rate, u8 vbps, u8 bps)
>>

Re: [PATCH 2/4] ALSA: hda: intel-nhlt: add intel_nhlt_ssp_mclk_mask()

[Pierre-Louis Bossart]

>>>> +
>>>> +            fmt = (struct nhlt_fmt *)(epnt->config.caps +
>>>> epnt->config.size);
>>>> +            cfg = fmt->fmt_config;
>>>> +
>>>> +            /*
>>>> +             * In theory all formats should use the same MCLK but it
>>>> doesn't hurt to
>>>> +             * double-check that the configuration is consistent
>>>> +             */
>>>> +            for (j = 0; j < fmt->fmt_count; j++) {
>>>> +                u32 *blob;
>>>> +                int mdivc_offset;
>>>> +
>>>> +                if (cfg->config.size >= SSP_BLOB_V1_0_SIZE) {
>>>> +                    blob = (u32 *)cfg->config.caps;
>>>> +
>>>> +                    if (blob[1] == SSP_BLOB_VER_2_0)
>>>> +                        mdivc_offset = SSP_BLOB_V2_0_MDIVC_OFFSET;
>>>> +                    else if (blob[1] == SSP_BLOB_VER_1_5)
>>>> +                        mdivc_offset = SSP_BLOB_V1_5_MDIVC_OFFSET;
>>>> +                    else
>>>> +                        mdivc_offset = SSP_BLOB_V1_0_MDIVC_OFFSET;
>>>> +
>>>> +                    mclk_mask |=  blob[mdivc_offset] & GENMASK(1, 0);
> 
> One more thing, where does this GENMASK come from, as far as I can tell
> HW specifies and FW uses one bit field to signal that MCLK is enabled?
> (mdivc is simply a value written to HW register to configure it).

There are two MCLK signals, that's the point of this patch. We need to
find which one is used. Platforms typically use MCLK0 except when they
don't..

BIT(0) set in mdivc enables MCLK0
BIT(1) set in mdivc enabled MCLK1

see
https://github.com/thesofproject/sof/blob/44a5200c87625588f0028aa08d560e68f2b8dc82/src/drivers/intel/ssp/mn.c#L150

>>>> +                }
>>>> +
>>>> +                cfg = (struct nhlt_fmt_cfg *)(cfg->config.caps +
>>>> cfg->config.size);
>>>> +            }
>>>> +        }
>>>> +        epnt = (struct nhlt_endpoint *)((u8 *)epnt + epnt->length);
>>>> +    }
>>>> +
>>>> +    return mclk_mask;
>>>
>>> Although I understand that it is relegated to the caller, but if both
>>> mclk being set is considered an error maybe add some kind of check here
>>> instead and free callers from having to remember about it?
>>>
>>> if (hweight_long(mclk_mask) != 1)
>>>      return -EINVAL;
>>>
>>> return mclk_mask;
>>
>> I went back and forth multiple times on this one. I can't figure out if
>> this would be a bug or a feature, it could be e.g. a test capability and
>> it's supported in hardware. I decided to make the decision in the caller
>> rather than a lower level in the library.
>>
>> If the tools used to generate NHLT don't support this multi-MCLK mode
>> then we could indeed move the test here.
>>
> 
> Considering comment I added above I've asked Czarek to also check this
> series. I'm not sure it even makes sense to name the field "_mask" when
> it is one bit...

it's two bits, see above.

Re: [PATCH 2/4] ALSA: hda: intel-nhlt: add intel_nhlt_ssp_mclk_mask()

[Amadeusz Sławiński]

On 8/23/2022 5:18 PM, Pierre-Louis Bossart wrote:
> 
>>>>> +
>>>>> +            fmt = (struct nhlt_fmt *)(epnt->config.caps +
>>>>> epnt->config.size);
>>>>> +            cfg = fmt->fmt_config;
>>>>> +
>>>>> +            /*
>>>>> +             * In theory all formats should use the same MCLK but it
>>>>> doesn't hurt to
>>>>> +             * double-check that the configuration is consistent
>>>>> +             */
>>>>> +            for (j = 0; j < fmt->fmt_count; j++) {
>>>>> +                u32 *blob;
>>>>> +                int mdivc_offset;
>>>>> +
>>>>> +                if (cfg->config.size >= SSP_BLOB_V1_0_SIZE) {
>>>>> +                    blob = (u32 *)cfg->config.caps;
>>>>> +
>>>>> +                    if (blob[1] == SSP_BLOB_VER_2_0)
>>>>> +                        mdivc_offset = SSP_BLOB_V2_0_MDIVC_OFFSET;
>>>>> +                    else if (blob[1] == SSP_BLOB_VER_1_5)
>>>>> +                        mdivc_offset = SSP_BLOB_V1_5_MDIVC_OFFSET;
>>>>> +                    else
>>>>> +                        mdivc_offset = SSP_BLOB_V1_0_MDIVC_OFFSET;
>>>>> +
>>>>> +                    mclk_mask |=  blob[mdivc_offset] & GENMASK(1, 0);
>>
>> One more thing, where does this GENMASK come from, as far as I can tell
>> HW specifies and FW uses one bit field to signal that MCLK is enabled?
>> (mdivc is simply a value written to HW register to configure it).
> 
> There are two MCLK signals, that's the point of this patch. We need to
> find which one is used. Platforms typically use MCLK0 except when they
> don't..
> 
> BIT(0) set in mdivc enables MCLK0
> BIT(1) set in mdivc enabled MCLK1
> 
> see
> https://github.com/thesofproject/sof/blob/44a5200c87625588f0028aa08d560e68f2b8dc82/src/drivers/intel/ssp/mn.c#L150
> 
>>>>> +                }
>>>>> +
>>>>> +                cfg = (struct nhlt_fmt_cfg *)(cfg->config.caps +
>>>>> cfg->config.size);
>>>>> +            }
>>>>> +        }
>>>>> +        epnt = (struct nhlt_endpoint *)((u8 *)epnt + epnt->length);
>>>>> +    }
>>>>> +
>>>>> +    return mclk_mask;
>>>>
>>>> Although I understand that it is relegated to the caller, but if both
>>>> mclk being set is considered an error maybe add some kind of check here
>>>> instead and free callers from having to remember about it?
>>>>
>>>> if (hweight_long(mclk_mask) != 1)
>>>>       return -EINVAL;
>>>>
>>>> return mclk_mask;
>>>
>>> I went back and forth multiple times on this one. I can't figure out if
>>> this would be a bug or a feature, it could be e.g. a test capability and
>>> it's supported in hardware. I decided to make the decision in the caller
>>> rather than a lower level in the library.
>>>
>>> If the tools used to generate NHLT don't support this multi-MCLK mode
>>> then we could indeed move the test here.
>>>
>>
>> Considering comment I added above I've asked Czarek to also check this
>> series. I'm not sure it even makes sense to name the field "_mask" when
>> it is one bit...
> 
> it's two bits, see above.

So I've spend a bit talking with FW team, and you are right, I got 
confused by one of the tables and some code that specified it as 1 bit 
field and rest as reserved, while other documents do specify it as a 
variable range of bits.

Going back to return value, the tool I have access to only has support 
for MCLK0. I guess we can make the assumption for now that everyone 
connects codec to one clock source and if someone later implements HW 
where somehow 2 different clocks are used (depending on format) we can 
refine the check later?

Re: [PATCH 2/4] ALSA: hda: intel-nhlt: add intel_nhlt_ssp_mclk_mask()

[Pierre-Louis Bossart]

>>>>>> +                }
>>>>>> +
>>>>>> +                cfg = (struct nhlt_fmt_cfg *)(cfg->config.caps +
>>>>>> cfg->config.size);
>>>>>> +            }
>>>>>> +        }
>>>>>> +        epnt = (struct nhlt_endpoint *)((u8 *)epnt + epnt->length);
>>>>>> +    }
>>>>>> +
>>>>>> +    return mclk_mask;
>>>>>
>>>>> Although I understand that it is relegated to the caller, but if both
>>>>> mclk being set is considered an error maybe add some kind of check
>>>>> here
>>>>> instead and free callers from having to remember about it?
>>>>>
>>>>> if (hweight_long(mclk_mask) != 1)
>>>>>       return -EINVAL;
>>>>>
>>>>> return mclk_mask;
>>>>
>>>> I went back and forth multiple times on this one. I can't figure out if
>>>> this would be a bug or a feature, it could be e.g. a test capability
>>>> and
>>>> it's supported in hardware. I decided to make the decision in the
>>>> caller
>>>> rather than a lower level in the library.
>>>>
>>>> If the tools used to generate NHLT don't support this multi-MCLK mode
>>>> then we could indeed move the test here.
>>>>
>>>
>>> Considering comment I added above I've asked Czarek to also check this
>>> series. I'm not sure it even makes sense to name the field "_mask" when
>>> it is one bit...
>>
>> it's two bits, see above.
> 
> So I've spend a bit talking with FW team, and you are right, I got
> confused by one of the tables and some code that specified it as 1 bit
> field and rest as reserved, while other documents do specify it as a
> variable range of bits.

Yeah, I had to ask multiple times as well. It's far from
self-explanatory and all the findings were based on NHLT shared with me.

See
https://github.com/thesofproject/linux/issues/3336#issuecomment-1206176141
for two examples with MCLK0 and MCLK1 used.

> Going back to return value, the tool I have access to only has support
> for MCLK0. I guess we can make the assumption for now that everyone
> connects codec to one clock source and if someone later implements HW
> where somehow 2 different clocks are used (depending on format) we can
> refine the check later?

Indeed it seems that depending on tools versions and targeted silicon,
MCLK1 may or may not be supported.

I guess it'd be fine to throw a big fat error in case this two-MCLK
configuration ever shows-up. That way we'll know for sure what was deployed.

Thanks for the feedback, I'll update this shortly.