diff mbox series

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

Message ID 20231014172624.75301-3-otto.pflueger@abscue.de (mailing list archive)
State Superseded
Headers show
Series ASoC: qcom: check ADSP version when setting clocks | expand

Commit Message

Otto Pflüger Oct. 14, 2023, 5:26 p.m. UTC
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(+)

Comments

Srinivas Kandagatla Oct. 17, 2023, 1:08 p.m. UTC | #1
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;
Otto Pflüger Oct. 19, 2023, 5:27 p.m. UTC | #2
[...]

> 
> <---
> > +		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;
diff mbox series

Patch

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;