[PATCH 0/3] ASoC: qcom: check ADSP version when setting clocks

[PATCH 0/3] ASoC: qcom: check ADSP version when setting clocks

[Otto Pflüger]

The apq8016_sbc driver currently works on APQ8016 and MSM8916 devices. It
should also work on MSM8909 (and newer SoCs like MSM8917 and MSM8953 if
the quinary MI2S line is added); however, newer devices with these SoCs
ship with newer firmware that uses a different interface for controlling
the digital codec and bit clocks, which causes the driver to fail because
it cannot set LPAIF_BIT_CLK.

In order to fix this problem, modify the LPAIF_* clock implementation in
the qdsp6 driver to use the newer clock API if a newer firmware version is
detected. This seems to be a better solution than exposing the firmware
version to other drivers like apq8016_sbc and forcing them to figure out
which clock to use.

On MSM8916, a hack is currently used to control the LPAIF_DIG_CLK directly
through the GCC driver, but on devices with the newer firmware, the
INTERNAL_DIGITAL_CODEC_CORE clock provided by q6afe-clocks in the qdsp6
driver can be used instead. Add a fallback to make this clock work with
the older firmware too, allowing one to specify it as the codec's "mclk"
in the device tree:

  compatible = "qcom,msm8916-wcd-digital-codec";
  clocks = <&xo_board>,
           <&q6afecc LPASS_CLK_ID_INTERNAL_DIGITAL_CODEC_CORE
                     LPASS_CLK_ATTRIBUTE_COUPLE_NO>;
  clock-names = "ahbix", "mclk";
  assigned-clocks = <&q6afecc LPASS_CLK_ID_INTERNAL_DIGITAL_CODEC_CORE
                              LPASS_CLK_ATTRIBUTE_COUPLE_NO>;
  assigned-clock-rates = <9600000>;

This works both on MSM8916 and on the newer SoCs mentioned above.

Otto Pflüger (3):
  ASoC: qcom: q6core: expose ADSP core firmware version
  ASoC: qcom: q6afe: check ADSP version when setting clocks
  ASoC: qcom: q6afe: remove "port already open" error

 sound/soc/qcom/qdsp6/q6afe.c  | 85 +++++++++++++++++++++++++++++++++--
 sound/soc/qcom/qdsp6/q6core.c | 61 +++++++++++++++++++++++++
 sound/soc/qcom/qdsp6/q6core.h |  9 ++++
 3 files changed, 152 insertions(+), 3 deletions(-)


base-commit: e3b18f7200f45d66f7141136c25554ac1e82009b
-- 
2.39.2

[PATCH 1/3] ASoC: qcom: q6core: expose ADSP core firmware version

[Otto Pflüger]

Add a q6core_get_adsp_version() function that returns the version of the
ADSP firmware (2.6, 2.7 or 2.8), also known as the AVS version (see [1]
in downstream kernel).

Some APIs differ between these versions, e.g. the AFE clock APIs.

[1]: https://github.com/msm8916-mainline/linux-downstream/blob/LA.BR.1.2.9.1_rb1.5/sound/soc/msm/qdsp6v2/q6core.c#L193

Signed-off-by: Otto Pflüger <otto.pflueger@abscue.de>
---
 sound/soc/qcom/qdsp6/q6core.c | 61 +++++++++++++++++++++++++++++++++++
 sound/soc/qcom/qdsp6/q6core.h |  9 ++++++
 2 files changed, 70 insertions(+)

diff --git a/sound/soc/qcom/qdsp6/q6core.c b/sound/soc/qcom/qdsp6/q6core.c
index 49cfb32cd209..ea6232645c80 100644
--- a/sound/soc/qcom/qdsp6/q6core.c
+++ b/sound/soc/qcom/qdsp6/q6core.c
@@ -20,6 +20,9 @@
 #define AVCS_CMDRSP_ADSP_EVENT_GET_STATE	0x0001290D
 #define AVCS_GET_VERSIONS       0x00012905
 #define AVCS_GET_VERSIONS_RSP   0x00012906
+#define AVCS_CMDRSP_Q6_ID_2_6	0x00040000
+#define AVCS_CMDRSP_Q6_ID_2_7	0x00040001
+#define AVCS_CMDRSP_Q6_ID_2_8	0x00040002
 #define AVCS_CMD_GET_FWK_VERSION	0x001292c
 #define AVCS_CMDRSP_GET_FWK_VERSION	0x001292d
 
@@ -63,6 +66,7 @@ struct q6core {
 	bool get_state_supported;
 	bool get_version_supported;
 	bool is_version_requested;
+	enum q6core_version adsp_version;
 };
 
 static struct q6core *g_core;
@@ -108,6 +112,14 @@ static int q6core_callback(struct apr_device *adev, struct apr_resp_pkt *data)
 		if (!core->fwk_version)
 			return -ENOMEM;
 
+		/*
+		 * Since GET_VERSIONS is not called when GET_FWK_VERSION
+		 * is successful and these commands may return completely
+		 * different versions, assume that the version is 2.8 here.
+		 * Older versions do not support GET_FWK_VERSION and we do
+		 * not care if the version is newer than 2.8.
+		 */
+		core->adsp_version = Q6_ADSP_VERSION_2_8;
 		core->fwk_version_supported = true;
 		core->resp_received = true;
 
@@ -115,6 +127,7 @@ static int q6core_callback(struct apr_device *adev, struct apr_resp_pkt *data)
 	}
 	case AVCS_GET_VERSIONS_RSP: {
 		struct avcs_cmdrsp_get_version *v;
+		int i;
 
 		v = data->payload;
 
@@ -125,6 +138,28 @@ static int q6core_callback(struct apr_device *adev, struct apr_resp_pkt *data)
 		if (!core->svc_version)
 			return -ENOMEM;
 
+		for (i = 0; i < g_core->svc_version->num_services; i++) {
+			struct avcs_svc_info *info;
+
+			info = &g_core->svc_version->svc_api_info[i];
+			if (info->service_id != APR_SVC_ADSP_CORE)
+				continue;
+
+			switch (info->version) {
+			case AVCS_CMDRSP_Q6_ID_2_6:
+				core->adsp_version = Q6_ADSP_VERSION_2_6;
+				break;
+			case AVCS_CMDRSP_Q6_ID_2_7:
+				core->adsp_version = Q6_ADSP_VERSION_2_7;
+				break;
+			case AVCS_CMDRSP_Q6_ID_2_8:
+				core->adsp_version = Q6_ADSP_VERSION_2_8;
+				break;
+			}
+
+			break;
+		}
+
 		core->get_version_supported = true;
 		core->resp_received = true;
 
@@ -293,6 +328,31 @@ int q6core_get_svc_api_info(int svc_id, struct q6core_svc_api_info *ainfo)
 }
 EXPORT_SYMBOL_GPL(q6core_get_svc_api_info);
 
+/**
+ * q6core_get_adsp_version() - Get the core version number.
+ *
+ * Return: version code or Q6_ADSP_VERSION_UNKNOWN on failure
+ */
+enum q6core_version q6core_get_adsp_version(void)
+{
+	int ret;
+
+	if (!g_core)
+		return Q6_ADSP_VERSION_UNKNOWN;
+
+	mutex_lock(&g_core->lock);
+	if (!g_core->is_version_requested) {
+		if (q6core_get_fwk_versions(g_core) == -ENOTSUPP)
+			q6core_get_svc_versions(g_core);
+		g_core->is_version_requested = true;
+	}
+	ret = g_core->adsp_version;
+	mutex_unlock(&g_core->lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(q6core_get_adsp_version);
+
 /**
  * q6core_is_adsp_ready() - Get status of adsp
  *
@@ -334,6 +394,7 @@ static int q6core_probe(struct apr_device *adev)
 	dev_set_drvdata(&adev->dev, g_core);
 
 	mutex_init(&g_core->lock);
+	g_core->adsp_version = Q6_ADSP_VERSION_UNKNOWN;
 	g_core->adev = adev;
 	init_waitqueue_head(&g_core->wait);
 	return 0;
diff --git a/sound/soc/qcom/qdsp6/q6core.h b/sound/soc/qcom/qdsp6/q6core.h
index 4105b1d730be..472e06bf8efc 100644
--- a/sound/soc/qcom/qdsp6/q6core.h
+++ b/sound/soc/qcom/qdsp6/q6core.h
@@ -9,7 +9,16 @@ struct q6core_svc_api_info {
 	uint32_t api_branch_version;
 };
 
+/* Versions must be in order! */
+enum q6core_version {
+	Q6_ADSP_VERSION_UNKNOWN,
+	Q6_ADSP_VERSION_2_6,
+	Q6_ADSP_VERSION_2_7,
+	Q6_ADSP_VERSION_2_8,
+};
+
 bool q6core_is_adsp_ready(void);
+enum q6core_version q6core_get_adsp_version(void);
 int q6core_get_svc_api_info(int svc_id, struct q6core_svc_api_info *ainfo);
 
 #endif /* __Q6CORE_H__ */
-- 
2.39.2

[PATCH 2/3] ASoC: qcom: q6afe: check ADSP version when setting clocks

[Otto Pflüger]

There are two APIs for setting clocks: the old one that uses
AFE_PARAM_ID_INT_DIGITAL_CDC_CLK_CONFIG and AFE_PARAM_ID_LPAIF_CLK_CONFIG,
and the new one which uses AFE_PARAM_ID_CLOCK_SET.

ADSP firmware version 2.6 only provides the old API, while newer
firmware versions only provide the new API.

Implement LPAIF_BIT_CLK and LPAIF_DIG_CLK for both APIs so that users
don't have to care about the firmware version. Also fall back to
setting AFE_PARAM_ID_INT_DIGITAL_CDC_CLK_CONFIG in q6afe_set_lpass_clock
when setting the new Q6AFE_LPASS_CLK_ID_INTERNAL_DIGITAL_CODEC_CORE
clock is requested to allow specifying it in the device tree on older
platforms too.

Signed-off-by: Otto Pflüger <otto.pflueger@abscue.de>
---
 sound/soc/qcom/qdsp6/q6afe.c | 81 ++++++++++++++++++++++++++++++++++++
 1 file changed, 81 insertions(+)

diff --git a/sound/soc/qcom/qdsp6/q6afe.c b/sound/soc/qcom/qdsp6/q6afe.c
index 91d39f6ad0bd..87bdf741e5f6 100644
--- a/sound/soc/qcom/qdsp6/q6afe.c
+++ b/sound/soc/qcom/qdsp6/q6afe.c
@@ -1111,6 +1111,32 @@ int q6afe_set_lpass_clock(struct device *dev, int clk_id, int attri,
 	struct q6afe *afe = dev_get_drvdata(dev->parent);
 	struct afe_clk_set cset = {0,};
 
+	/*
+	 * v2 clocks specified in the device tree may not be supported by the
+	 * firmware. If this is the digital codec core clock, fall back to the
+	 * old method for setting it.
+	 */
+	if (q6core_get_adsp_version() < Q6_ADSP_VERSION_2_7) {
+		struct q6afe_port *port;
+		struct afe_digital_clk_cfg dcfg = {0,};
+		int ret;
+
+		if (clk_id != Q6AFE_LPASS_CLK_ID_INTERNAL_DIGITAL_CODEC_CORE)
+			return -EINVAL;
+
+		port = q6afe_port_get_from_id(dev, PRIMARY_MI2S_RX);
+		if (IS_ERR(port))
+			return PTR_ERR(port);
+
+		dcfg.i2s_cfg_minor_version = AFE_API_VERSION_I2S_CONFIG;
+		dcfg.clk_val = freq;
+		dcfg.clk_root = 5;
+		ret = q6afe_set_digital_codec_core_clock(port, &dcfg);
+
+		q6afe_port_put(port);
+		return ret;
+	}
+
 	cset.clk_set_minor_version = AFE_API_VERSION_CLOCK_SET;
 	cset.clk_id = clk_id;
 	cset.clk_freq_in_hz = freq;
@@ -1124,6 +1150,41 @@ int q6afe_set_lpass_clock(struct device *dev, int clk_id, int attri,
 }
 EXPORT_SYMBOL_GPL(q6afe_set_lpass_clock);
 
+static int q6afe_get_v2_bit_clk_id(struct q6afe_port *port)
+{
+	switch (port->id) {
+	case AFE_PORT_ID_PRIMARY_MI2S_RX:
+	case AFE_PORT_ID_PRIMARY_MI2S_TX:
+		return Q6AFE_LPASS_CLK_ID_PRI_MI2S_IBIT;
+	case AFE_PORT_ID_SECONDARY_MI2S_RX:
+	case AFE_PORT_ID_SECONDARY_MI2S_TX:
+		return Q6AFE_LPASS_CLK_ID_SEC_MI2S_IBIT;
+	case AFE_PORT_ID_TERTIARY_MI2S_RX:
+	case AFE_PORT_ID_TERTIARY_MI2S_TX:
+		return Q6AFE_LPASS_CLK_ID_TER_MI2S_IBIT;
+	case AFE_PORT_ID_QUATERNARY_MI2S_RX:
+	case AFE_PORT_ID_QUATERNARY_MI2S_TX:
+		return Q6AFE_LPASS_CLK_ID_TER_MI2S_IBIT;
+	case AFE_PORT_ID_QUINARY_MI2S_RX:
+	case AFE_PORT_ID_QUINARY_MI2S_TX:
+		return Q6AFE_LPASS_CLK_ID_QUI_MI2S_IBIT;
+
+	case AFE_PORT_ID_PRIMARY_TDM_RX ... AFE_PORT_ID_PRIMARY_TDM_TX_7:
+		return Q6AFE_LPASS_CLK_ID_PRI_TDM_IBIT;
+	case AFE_PORT_ID_SECONDARY_TDM_RX ... AFE_PORT_ID_SECONDARY_TDM_TX_7:
+		return Q6AFE_LPASS_CLK_ID_SEC_TDM_IBIT;
+	case AFE_PORT_ID_TERTIARY_TDM_RX ... AFE_PORT_ID_TERTIARY_TDM_TX_7:
+		return Q6AFE_LPASS_CLK_ID_TER_TDM_IBIT;
+	case AFE_PORT_ID_QUATERNARY_TDM_RX ... AFE_PORT_ID_QUATERNARY_TDM_TX_7:
+		return Q6AFE_LPASS_CLK_ID_QUAD_TDM_IBIT;
+	case AFE_PORT_ID_QUINARY_TDM_RX ... AFE_PORT_ID_QUINARY_TDM_TX_7:
+		return Q6AFE_LPASS_CLK_ID_QUIN_TDM_IBIT;
+
+	default:
+		return -EINVAL;
+	}
+}
+
 int q6afe_port_set_sysclk(struct q6afe_port *port, int clk_id,
 			  int clk_src, int clk_root,
 			  unsigned int freq, int dir)
@@ -1133,6 +1194,26 @@ int q6afe_port_set_sysclk(struct q6afe_port *port, int clk_id,
 	struct afe_digital_clk_cfg dcfg = {0,};
 	int ret;
 
+	if (q6core_get_adsp_version() >= Q6_ADSP_VERSION_2_7) {
+		/* Always use the new clock API on newer platforms. */
+		switch (clk_id) {
+		case LPAIF_DIG_CLK:
+			clk_src = Q6AFE_LPASS_CLK_ATTRIBUTE_COUPLE_NO;
+			clk_root = Q6AFE_LPASS_CLK_ROOT_DEFAULT;
+			clk_id = Q6AFE_LPASS_CLK_ID_INTERNAL_DIGITAL_CODEC_CORE;
+			break;
+		case LPAIF_BIT_CLK:
+			clk_src = Q6AFE_LPASS_CLK_ATTRIBUTE_COUPLE_NO;
+			clk_root = Q6AFE_LPASS_CLK_ROOT_DEFAULT;
+			clk_id = q6afe_get_v2_bit_clk_id(port);
+			if (clk_id < 0)
+				return clk_id;
+			break;
+		default:
+			break;
+		}
+	}
+
 	switch (clk_id) {
 	case LPAIF_DIG_CLK:
 		dcfg.i2s_cfg_minor_version = AFE_API_VERSION_I2S_CONFIG;
-- 
2.39.2

[PATCH 3/3] ASoC: qcom: q6afe: remove "port already open" error

[Otto Pflüger]

The clock fallback for older firmware versions now represents a use case
for having multiple references to a port. Stop logging an error when a
port is requested more than once because this does not indicate a problem
anymore and causes unnecessary spam in dmesg.

Signed-off-by: Otto Pflüger <otto.pflueger@abscue.de>
---
 sound/soc/qcom/qdsp6/q6afe.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/sound/soc/qcom/qdsp6/q6afe.c b/sound/soc/qcom/qdsp6/q6afe.c
index 87bdf741e5f6..d60c7b48e118 100644
--- a/sound/soc/qcom/qdsp6/q6afe.c
+++ b/sound/soc/qcom/qdsp6/q6afe.c
@@ -1647,10 +1647,8 @@ struct q6afe_port *q6afe_port_get_from_id(struct device *dev, int id)
 
 	/* if port is multiple times bind/unbind before callback finishes */
 	port = q6afe_find_port(afe, id);
-	if (port) {
-		dev_err(dev, "AFE Port already open\n");
+	if (port)
 		return port;
-	}
 
 	port_id = port_maps[id].port_id;
 
-- 
2.39.2

Re: [PATCH 1/3] ASoC: qcom: q6core: expose ADSP core firmware version

[Mark Brown]

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

On Sat, Oct 14, 2023 at 07:26:22PM +0200, Otto Pflüger wrote:

> +		for (i = 0; i < g_core->svc_version->num_services; i++) {
> +			struct avcs_svc_info *info;
> +
> +			info = &g_core->svc_version->svc_api_info[i];
> +			if (info->service_id != APR_SVC_ADSP_CORE)
> +				continue;
> +
> +			switch (info->version) {
> +			case AVCS_CMDRSP_Q6_ID_2_6:
> +				core->adsp_version = Q6_ADSP_VERSION_2_6;
> +				break;
> +			case AVCS_CMDRSP_Q6_ID_2_7:
> +				core->adsp_version = Q6_ADSP_VERSION_2_7;
> +				break;
> +			case AVCS_CMDRSP_Q6_ID_2_8:
> +				core->adsp_version = Q6_ADSP_VERSION_2_8;
> +				break;
> +			}

This doesn't handle unknown versions at all.

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

Re: [PATCH 1/3] ASoC: qcom: q6core: expose ADSP core firmware version

[Otto Pflüger]

On Mon, Oct 16, 2023 at 01:47:28PM +0100, Mark Brown wrote:
> On Sat, Oct 14, 2023 at 07:26:22PM +0200, Otto Pflüger wrote:
> 
> > +		for (i = 0; i < g_core->svc_version->num_services; i++) {
> > +			struct avcs_svc_info *info;
> > +
> > +			info = &g_core->svc_version->svc_api_info[i];
> > +			if (info->service_id != APR_SVC_ADSP_CORE)
> > +				continue;
> > +
> > +			switch (info->version) {
> > +			case AVCS_CMDRSP_Q6_ID_2_6:
> > +				core->adsp_version = Q6_ADSP_VERSION_2_6;
> > +				break;
> > +			case AVCS_CMDRSP_Q6_ID_2_7:
> > +				core->adsp_version = Q6_ADSP_VERSION_2_7;
> > +				break;
> > +			case AVCS_CMDRSP_Q6_ID_2_8:
> > +				core->adsp_version = Q6_ADSP_VERSION_2_8;
> > +				break;
> > +			}
> 
> This doesn't handle unknown versions at all.

The adsp_version is initialized to Q6_ADSP_VERSION_UNKNOWN in
q6core_probe, so it should stay unknown if it doesn't match any of
these values. I don't see any big problems here, but I agree that some
additional handling such as a warning message could be useful.

Re: [PATCH 2/3] ASoC: qcom: q6afe: check ADSP version when setting clocks

[Srinivas Kandagatla]

Thanks Otto for the patch,
some comments below

On 14/10/2023 18:26, Otto Pflüger wrote:
> There are two APIs for setting clocks: the old one that uses
> AFE_PARAM_ID_INT_DIGITAL_CDC_CLK_CONFIG and AFE_PARAM_ID_LPAIF_CLK_CONFIG,
> and the new one which uses AFE_PARAM_ID_CLOCK_SET.
> 
> ADSP firmware version 2.6 only provides the old API, while newer
> firmware versions only provide the new API.
> 
> Implement LPAIF_BIT_CLK and LPAIF_DIG_CLK for both APIs so that users
> don't have to care about the firmware version. Also fall back to
> setting AFE_PARAM_ID_INT_DIGITAL_CDC_CLK_CONFIG in q6afe_set_lpass_clock
> when setting the new Q6AFE_LPASS_CLK_ID_INTERNAL_DIGITAL_CODEC_CORE
> clock is requested to allow specifying it in the device tree on older
> platforms too.
> 
> Signed-off-by: Otto Pflüger <otto.pflueger@abscue.de>
> ---
>   sound/soc/qcom/qdsp6/q6afe.c | 81 ++++++++++++++++++++++++++++++++++++
>   1 file changed, 81 insertions(+)
> 
> diff --git a/sound/soc/qcom/qdsp6/q6afe.c b/sound/soc/qcom/qdsp6/q6afe.c
> index 91d39f6ad0bd..87bdf741e5f6 100644
> --- a/sound/soc/qcom/qdsp6/q6afe.c
> +++ b/sound/soc/qcom/qdsp6/q6afe.c
> @@ -1111,6 +1111,32 @@ int q6afe_set_lpass_clock(struct device *dev, int clk_id, int attri,
>   	struct q6afe *afe = dev_get_drvdata(dev->parent);
>   	struct afe_clk_set cset = {0,};
>   
> +	/*
> +	 * v2 clocks specified in the device tree may not be supported by the
> +	 * firmware. If this is the digital codec core clock, fall back to the
> +	 * old method for setting it.
> +	 */
> +	if (q6core_get_adsp_version() < Q6_ADSP_VERSION_2_7) {
> +		struct q6afe_port *port;
> +		struct afe_digital_clk_cfg dcfg = {0,};
> +		int ret;
> +
> +		if (clk_id != Q6AFE_LPASS_CLK_ID_INTERNAL_DIGITAL_CODEC_CORE)
> +			return -EINVAL;
> +

<---
> +		port = q6afe_port_get_from_id(dev, PRIMARY_MI2S_RX);
> +		if (IS_ERR(port))
> +			return PTR_ERR(port);
> +
> +		dcfg.i2s_cfg_minor_version = AFE_API_VERSION_I2S_CONFIG;
> +		dcfg.clk_val = freq;
> +		dcfg.clk_root = 5;
> +		ret = q6afe_set_digital_codec_core_clock(port, &dcfg);
> +
> +		q6afe_port_put(port);
--->

Could you pl explain what are we doing in this snippet?

Isn't this what is exactly done in q6afe_mi2s_set_sysclk(LPAIF_DIG_CLK...)




> +		return ret;
> +	}
> +
>   	cset.clk_set_minor_version = AFE_API_VERSION_CLOCK_SET;
>   	cset.clk_id = clk_id;
>   	cset.clk_freq_in_hz = freq;
> @@ -1124,6 +1150,41 @@ int q6afe_set_lpass_clock(struct device *dev, int clk_id, int attri,
>   }
>   EXPORT_SYMBOL_GPL(q6afe_set_lpass_clock);
>   
...

>   int q6afe_port_set_sysclk(struct q6afe_port *port, int clk_id,
>   			  int clk_src, int clk_root,
>   			  unsigned int freq, int dir)
> @@ -1133,6 +1194,26 @@ int q6afe_port_set_sysclk(struct q6afe_port *port, int clk_id,
>   	struct afe_digital_clk_cfg dcfg = {0,};
>   	int ret;
>   
> +	if (q6core_get_adsp_version() >= Q6_ADSP_VERSION_2_7) {
> +		/* Always use the new clock API on newer platforms. */
> +		switch (clk_id) {
> +		case LPAIF_DIG_CLK:
> +			clk_src = Q6AFE_LPASS_CLK_ATTRIBUTE_COUPLE_NO;
> +			clk_root = Q6AFE_LPASS_CLK_ROOT_DEFAULT;
> +			clk_id = Q6AFE_LPASS_CLK_ID_INTERNAL_DIGITAL_CODEC_CORE;
> +			break;
> +		case LPAIF_BIT_CLK:
> +			clk_src = Q6AFE_LPASS_CLK_ATTRIBUTE_COUPLE_NO;
> +			clk_root = Q6AFE_LPASS_CLK_ROOT_DEFAULT;
> +			clk_id = q6afe_get_v2_bit_clk_id(port);
> +			if (clk_id < 0)
> +				return clk_id;
> +			break;
> +		default:
> +			break;
> +		}
> +	}

This should be probably done in machine driver or q6afe-dai, not in q6afe.


> +
>   	switch (clk_id) {
>   	case LPAIF_DIG_CLK:
>   		dcfg.i2s_cfg_minor_version = AFE_API_VERSION_I2S_CONFIG;

Re: [PATCH 2/3] ASoC: qcom: q6afe: check ADSP version when setting clocks

[Otto Pflüger]

[...]

> 
> <---
> > +		port = q6afe_port_get_from_id(dev, PRIMARY_MI2S_RX);
> > +		if (IS_ERR(port))
> > +			return PTR_ERR(port);
> > +
> > +		dcfg.i2s_cfg_minor_version = AFE_API_VERSION_I2S_CONFIG;
> > +		dcfg.clk_val = freq;
> > +		dcfg.clk_root = 5;
> > +		ret = q6afe_set_digital_codec_core_clock(port, &dcfg);
> > +
> > +		q6afe_port_put(port);
> --->
> 
> Could you pl explain what are we doing in this snippet?
> 
> Isn't this what is exactly done in q6afe_mi2s_set_sysclk(LPAIF_DIG_CLK...)
> 
> 

Yes, this is the same, except that q6afe_mi2s_set_sysclk is in
q6afe-dai.c and relies on being part of the DAI, while this is called
from q6afe-clocks.c in a context which doesn't necessarily require a DAI
to be present, e.g. if q6afe-clocks is used as a simple clock controller
without any DAIs defined in the device tree.

Setting the digital codec clock always requires a port, but it isn't
relevant which port is used here because there is usually only one
codec clock.

> 
> 
> > +		return ret;
> > +	}
> > +
> >   	cset.clk_set_minor_version = AFE_API_VERSION_CLOCK_SET;
> >   	cset.clk_id = clk_id;
> >   	cset.clk_freq_in_hz = freq;
> > @@ -1124,6 +1150,41 @@ int q6afe_set_lpass_clock(struct device *dev, int clk_id, int attri,
> >   }
> >   EXPORT_SYMBOL_GPL(q6afe_set_lpass_clock);
> ...
> 
> >   int q6afe_port_set_sysclk(struct q6afe_port *port, int clk_id,
> >   			  int clk_src, int clk_root,
> >   			  unsigned int freq, int dir)
> > @@ -1133,6 +1194,26 @@ int q6afe_port_set_sysclk(struct q6afe_port *port, int clk_id,
> >   	struct afe_digital_clk_cfg dcfg = {0,};
> >   	int ret;
> > +	if (q6core_get_adsp_version() >= Q6_ADSP_VERSION_2_7) {
> > +		/* Always use the new clock API on newer platforms. */
> > +		switch (clk_id) {
> > +		case LPAIF_DIG_CLK:
> > +			clk_src = Q6AFE_LPASS_CLK_ATTRIBUTE_COUPLE_NO;
> > +			clk_root = Q6AFE_LPASS_CLK_ROOT_DEFAULT;
> > +			clk_id = Q6AFE_LPASS_CLK_ID_INTERNAL_DIGITAL_CODEC_CORE;
> > +			break;
> > +		case LPAIF_BIT_CLK:
> > +			clk_src = Q6AFE_LPASS_CLK_ATTRIBUTE_COUPLE_NO;
> > +			clk_root = Q6AFE_LPASS_CLK_ROOT_DEFAULT;
> > +			clk_id = q6afe_get_v2_bit_clk_id(port);
> > +			if (clk_id < 0)
> > +				return clk_id;
> > +			break;
> > +		default:
> > +			break;
> > +		}
> > +	}
> 
> This should be probably done in machine driver or q6afe-dai, not in q6afe.
> 

I'll put it in q6afe-dai since this fits nicely into the switch
statement in q6afe_mi2s_set_sysclk.

As stated in the cover letter, I don't think adding this to the machine
driver is the best option. The LPAIF_* clocks look simple and generic
enough to be usable by different drivers, and making those drivers care
about different clock versions in the firmware doesn't seem right.


> 
> > +
> >   	switch (clk_id) {
> >   	case LPAIF_DIG_CLK:
> >   		dcfg.i2s_cfg_minor_version = AFE_API_VERSION_I2S_CONFIG;