[PATCH v2] ASoC: Intel: boards: eve: Fix DMIC records zero

[PATCH v2] ASoC: Intel: boards: eve: Fix DMIC records zero

[Harsha Priya]

This patch adds a dapm route to provide early mclk/sclk for
for DMIC on SSP0(rt5514) and Headset on SSP1(rt5663).

The sclk rate for both codecs is different which is taken care
of in the platform_clock_control().The platform has one mclk and
one sclk. Two variables sclk0 and sclk1 are used for the
two codecs on differnet ssp that use different clock rate.

This change ensures the DMIC PCM port will return valid data

Signed-off-by: Brent Lu <brent.lu@intel.com>
Signed-off-by: Vamshi Krishna Gopal <vamshi.krishna.gopal@intel.com>
Signed-off-by: Harsha Priya <harshapriya.n@intel.com>
---
v1 -> v2:
- Only one mclk with same rate is used, so changed to using one variable
- dropping ssp_ prefix from sclk variable names to make them sound right.
- removing a return statment that was not required
- fixed commit message accordingly

 .../soc/intel/boards/kbl_rt5663_rt5514_max98927.c  | 146 ++++++++++++++-------
 1 file changed, 97 insertions(+), 49 deletions(-)

diff --git a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
index b34cf6c..155f2b4 100644
--- a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
+++ b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
@@ -54,7 +54,8 @@ struct kbl_codec_private {
 	struct list_head hdmi_pcm_list;
 	struct snd_soc_jack kabylake_hdmi[2];
 	struct clk *mclk;
-	struct clk *sclk;
+	struct clk *sclk0;
+	struct clk *sclk1;
 };
 
 enum {
@@ -77,13 +78,29 @@ static const struct snd_kcontrol_new kabylake_controls[] = {
 };
 
 static int platform_clock_control(struct snd_soc_dapm_widget *w,
-			struct snd_kcontrol *k, int  event)
+			struct snd_kcontrol *k, int event, int ssp_num)
 {
 	struct snd_soc_dapm_context *dapm = w->dapm;
 	struct snd_soc_card *card = dapm->card;
 	struct kbl_codec_private *priv = snd_soc_card_get_drvdata(card);
+	struct clk *sclk;
+	unsigned long sclk_rate;
 	int ret = 0;
 
+	switch (ssp_num) {
+	case 0:
+		sclk = priv->sclk0;
+		sclk_rate = 6144000;
+		break;
+	case 1:
+		sclk = priv->sclk1;
+		sclk_rate = 3072000;
+		break;
+	default:
+		dev_err(card->dev, "Invalid ssp_num %d\n", ssp_num);
+		return -EINVAL;
+	}
+
 	/*
 	 * MCLK/SCLK need to be ON early for a successful synchronization of
 	 * codec internal clock. And the clocks are turned off during
@@ -91,38 +108,48 @@ static int platform_clock_control(struct snd_soc_dapm_widget *w,
 	 */
 	switch (event) {
 	case SND_SOC_DAPM_PRE_PMU:
-		/* Enable MCLK */
 		ret = clk_set_rate(priv->mclk, 24000000);
 		if (ret < 0) {
-			dev_err(card->dev, "Can't set rate for mclk, err: %d\n",
-				ret);
-			return ret;
+			dev_err(card->dev, "Can't set rate for mclk for ssp%d, err: %d\n",
+				ssp_num, ret);
+				return ret;
 		}
 
-		ret = clk_prepare_enable(priv->mclk);
-		if (ret < 0) {
-			dev_err(card->dev, "Can't enable mclk, err: %d\n", ret);
-			return ret;
+		if (!__clk_is_enabled(priv->mclk)) {
+			/* Enable MCLK */
+			ret = clk_prepare_enable(priv->mclk);
+			if (ret < 0) {
+				dev_err(card->dev, "Can't enable mclk for ssp%d, err: %d\n",
+					ssp_num, ret);
+				return ret;
+			}
 		}
 
-		/* Enable SCLK */
-		ret = clk_set_rate(priv->sclk, 3072000);
+		ret = clk_set_rate(sclk, sclk_rate);
 		if (ret < 0) {
-			dev_err(card->dev, "Can't set rate for sclk, err: %d\n",
-				ret);
+			dev_err(card->dev, "Can't set rate for sclk for ssp%d, err: %d\n",
+				ssp_num, ret);
 			clk_disable_unprepare(priv->mclk);
 			return ret;
 		}
 
-		ret = clk_prepare_enable(priv->sclk);
-		if (ret < 0) {
-			dev_err(card->dev, "Can't enable sclk, err: %d\n", ret);
-			clk_disable_unprepare(priv->mclk);
+		if (!__clk_is_enabled(sclk)) {
+			/* Enable SCLK */
+			ret = clk_prepare_enable(sclk);
+			if (ret < 0) {
+				dev_err(card->dev, "Can't enable sclk for ssp%d, err: %d\n",
+					ssp_num, ret);
+				clk_disable_unprepare(priv->mclk);
+				return ret;
+			}
 		}
 		break;
 	case SND_SOC_DAPM_POST_PMD:
-		clk_disable_unprepare(priv->mclk);
-		clk_disable_unprepare(priv->sclk);
+		if (__clk_is_enabled(priv->mclk))
+			clk_disable_unprepare(priv->mclk);
+
+		if (__clk_is_enabled(sclk))
+			clk_disable_unprepare(sclk);
 		break;
 	default:
 		return 0;
@@ -131,6 +158,18 @@ static int platform_clock_control(struct snd_soc_dapm_widget *w,
 	return 0;
 }
 
+static int platform_clock_control_ssp0(struct snd_soc_dapm_widget *w,
+			struct snd_kcontrol *k, int event)
+{
+	return platform_clock_control(w, k, event, 0);
+}
+
+static int platform_clock_control_ssp1(struct snd_soc_dapm_widget *w,
+			struct snd_kcontrol *k, int event)
+{
+	return platform_clock_control(w, k, event, 1);
+}
+
 static const struct snd_soc_dapm_widget kabylake_widgets[] = {
 	SND_SOC_DAPM_HP("Headphone Jack", NULL),
 	SND_SOC_DAPM_MIC("Headset Mic", NULL),
@@ -139,15 +178,17 @@ static const struct snd_soc_dapm_widget kabylake_widgets[] = {
 	SND_SOC_DAPM_MIC("DMIC", NULL),
 	SND_SOC_DAPM_SPK("HDMI1", NULL),
 	SND_SOC_DAPM_SPK("HDMI2", NULL),
-	SND_SOC_DAPM_SUPPLY("Platform Clock", SND_SOC_NOPM, 0, 0,
-			platform_clock_control, SND_SOC_DAPM_PRE_PMU |
+	SND_SOC_DAPM_SUPPLY("Platform Clock SSP0", SND_SOC_NOPM, 0, 0,
+			platform_clock_control_ssp0, SND_SOC_DAPM_PRE_PMU |
+			SND_SOC_DAPM_POST_PMD),
+	SND_SOC_DAPM_SUPPLY("Platform Clock SSP1", SND_SOC_NOPM, 0, 0,
+			platform_clock_control_ssp1, SND_SOC_DAPM_PRE_PMU |
 			SND_SOC_DAPM_POST_PMD),
-
 };
 
 static const struct snd_soc_dapm_route kabylake_map[] = {
 	/* Headphones */
-	{ "Headphone Jack", NULL, "Platform Clock" },
+	{ "Headphone Jack", NULL, "Platform Clock SSP1" },
 	{ "Headphone Jack", NULL, "HPOL" },
 	{ "Headphone Jack", NULL, "HPOR" },
 
@@ -156,7 +197,7 @@ static const struct snd_soc_dapm_route kabylake_map[] = {
 	{ "Right Spk", NULL, "Right BE_OUT" },
 
 	/* other jacks */
-	{ "Headset Mic", NULL, "Platform Clock" },
+	{ "Headset Mic", NULL, "Platform Clock SSP1" },
 	{ "IN1P", NULL, "Headset Mic" },
 	{ "IN1N", NULL, "Headset Mic" },
 
@@ -180,6 +221,7 @@ static const struct snd_soc_dapm_route kabylake_map[] = {
 	{ "ssp0 Rx", NULL, "Right HiFi Capture" },
 
 	/* DMIC */
+	{ "DMIC", NULL, "Platform Clock SSP0" },
 	{ "DMIC1L", NULL, "DMIC" },
 	{ "DMIC1R", NULL, "DMIC" },
 	{ "DMIC2L", NULL, "DMIC" },
@@ -666,7 +708,7 @@ static int kabylake_set_bias_level(struct snd_soc_card *card,
 	if (!component || strcmp(component->name, RT5514_DEV_NAME))
 		return 0;
 
-	if (IS_ERR(priv->mclk))
+	if (IS_ERR(priv->mclk0))
 		return 0;
 
 	/*
@@ -757,6 +799,28 @@ static struct snd_soc_card kabylake_audio_card = {
 	.late_probe = kabylake_card_late_probe,
 };
 
+static int kabylake_audio_clk_get(struct device *dev, const char *id,
+	struct clk **clk)
+{
+	int ret = 0;
+
+	if (!clk)
+		return -EINVAL;
+
+	*clk = devm_clk_get(dev, id);
+	if (IS_ERR(*clk)) {
+		ret = PTR_ERR(*clk);
+		if (ret == -ENOENT) {
+			dev_info(dev, "Failed to get %s, defer probe\n", id);
+			return -EPROBE_DEFER;
+		}
+
+		dev_err(dev, "Failed to get %s with err:%d\n", id, ret);
+	}
+
+	return ret;
+}
+
 static int kabylake_audio_probe(struct platform_device *pdev)
 {
 	struct kbl_codec_private *ctx;
@@ -777,33 +841,17 @@ static int kabylake_audio_probe(struct platform_device *pdev)
 		dmic_constraints = mach->mach_params.dmic_num == 2 ?
 			&constraints_dmic_2ch : &constraints_dmic_channels;
 
-	ctx->mclk = devm_clk_get(&pdev->dev, "ssp1_mclk");
-	if (IS_ERR(ctx->mclk)) {
-		ret = PTR_ERR(ctx->mclk);
-		if (ret == -ENOENT) {
-			dev_info(&pdev->dev,
-				"Failed to get ssp1_mclk, defer probe\n");
-			return -EPROBE_DEFER;
-		}
-
-		dev_err(&pdev->dev, "Failed to get ssp1_mclk with err:%d\n",
-								ret);
+	ret = kabylake_audio_clk_get(&pdev->dev, "ssp0_sclk", &ctx->sclk0);
+	if (ret != 0)
 		return ret;
-	}
 
-	ctx->sclk = devm_clk_get(&pdev->dev, "ssp1_sclk");
-	if (IS_ERR(ctx->sclk)) {
-		ret = PTR_ERR(ctx->sclk);
-		if (ret == -ENOENT) {
-			dev_info(&pdev->dev,
-				"Failed to get ssp1_sclk, defer probe\n");
-			return -EPROBE_DEFER;
-		}
+	ret = kabylake_audio_clk_get(&pdev->dev, "ssp1_mclk", &ctx->mclk);
+	if (ret != 0)
+		return ret;
 
-		dev_err(&pdev->dev, "Failed to get ssp1_sclk with err:%d\n",
-								ret);
+	ret = kabylake_audio_clk_get(&pdev->dev, "ssp1_sclk", &ctx->sclk1);
+	if (ret != 0)
 		return ret;
-	}
 
 	return devm_snd_soc_register_card(&pdev->dev, &kabylake_audio_card);
 }
-- 
2.7.4

Re: [PATCH v2] ASoC: Intel: boards: eve: Fix DMIC records zero

[Pierre-Louis Bossart]

>   	/*
>   	 * MCLK/SCLK need to be ON early for a successful synchronization of
>   	 * codec internal clock. And the clocks are turned off during
> @@ -91,38 +108,48 @@ static int platform_clock_control(struct snd_soc_dapm_widget *w,
>   	 */
>   	switch (event) {
>   	case SND_SOC_DAPM_PRE_PMU:
> -		/* Enable MCLK */
>   		ret = clk_set_rate(priv->mclk, 24000000);
>   		if (ret < 0) {
> -			dev_err(card->dev, "Can't set rate for mclk, err: %d\n",
> -				ret);
> -			return ret;
> +			dev_err(card->dev, "Can't set rate for mclk for ssp%d, err: %d\n",
> +				ssp_num, ret);
> +				return ret;

nit-pick: alignment is off for the 'return ret'.

>   		}
>   
> -		ret = clk_prepare_enable(priv->mclk);
> -		if (ret < 0) {
> -			dev_err(card->dev, "Can't enable mclk, err: %d\n", ret);
> -			return ret;
> +		if (!__clk_is_enabled(priv->mclk)) {
> +			/* Enable MCLK */
> +			ret = clk_prepare_enable(priv->mclk);

That seems correct since you share the mclk between two resources but 
see [1] below

> +			if (ret < 0) {
> +				dev_err(card->dev, "Can't enable mclk for ssp%d, err: %d\n",
> +					ssp_num, ret);
> +				return ret;
> +			}
>   		}
>   
> -		/* Enable SCLK */
> -		ret = clk_set_rate(priv->sclk, 3072000);
> +		ret = clk_set_rate(sclk, sclk_rate);
>   		if (ret < 0) {
> -			dev_err(card->dev, "Can't set rate for sclk, err: %d\n",
> -				ret);
> +			dev_err(card->dev, "Can't set rate for sclk for ssp%d, err: %d\n",
> +				ssp_num, ret);
>   			clk_disable_unprepare(priv->mclk);
>   			return ret;
>   		}
>   
> -		ret = clk_prepare_enable(priv->sclk);
> -		if (ret < 0) {
> -			dev_err(card->dev, "Can't enable sclk, err: %d\n", ret);
> -			clk_disable_unprepare(priv->mclk);
> +		if (!__clk_is_enabled(sclk)) {

Why do you need this test? the sclocks are not shared? see also [2] below

> +			/* Enable SCLK */
> +			ret = clk_prepare_enable(sclk);
> +			if (ret < 0) {
> +				dev_err(card->dev, "Can't enable sclk for ssp%d, err: %d\n",
> +					ssp_num, ret);
> +				clk_disable_unprepare(priv->mclk);
> +				return ret;
> +			}
>   		}
>   		break;
>   	case SND_SOC_DAPM_POST_PMD:
> -		clk_disable_unprepare(priv->mclk);
> -		clk_disable_unprepare(priv->sclk);
> +		if (__clk_is_enabled(priv->mclk))
> +			clk_disable_unprepare(priv->mclk);
> +

[1] this seems wrong in case you have two SSPs working, and stop one. 
This would turn off the mclk while one of the two SSPs is still working.

> +		if (__clk_is_enabled(sclk))

[2] Again is this test needed since sclk is not shared between SSPs

> +			clk_disable_unprepare(sclk);

RE: [PATCH v2] ASoC: Intel: boards: eve: Fix DMIC records zero

[N, Harshapriya]

> 
> >   	/*
> >   	 * MCLK/SCLK need to be ON early for a successful synchronization of
> >   	 * codec internal clock. And the clocks are turned off during @@
> > -91,38 +108,48 @@ static int platform_clock_control(struct
> snd_soc_dapm_widget *w,
> >   	 */
> >   	switch (event) {
> >   	case SND_SOC_DAPM_PRE_PMU:
> > -		/* Enable MCLK */
> >   		ret = clk_set_rate(priv->mclk, 24000000);
> >   		if (ret < 0) {
> > -			dev_err(card->dev, "Can't set rate for mclk, err: %d\n",
> > -				ret);
> > -			return ret;
> > +			dev_err(card->dev, "Can't set rate for mclk for ssp%d,
> err: %d\n",
> > +				ssp_num, ret);
> > +				return ret;
> 
> nit-pick: alignment is off for the 'return ret'.
my bad... will change that
> 
> >   		}
> >
> > -		ret = clk_prepare_enable(priv->mclk);
> > -		if (ret < 0) {
> > -			dev_err(card->dev, "Can't enable mclk, err: %d\n",
> ret);
> > -			return ret;
> > +		if (!__clk_is_enabled(priv->mclk)) {
> > +			/* Enable MCLK */
> > +			ret = clk_prepare_enable(priv->mclk);
> 
> That seems correct since you share the mclk between two resources but see [1]
> below
> 
> > +			if (ret < 0) {
> > +				dev_err(card->dev, "Can't enable mclk for
> ssp%d, err: %d\n",
> > +					ssp_num, ret);
> > +				return ret;
> > +			}
> >   		}
> >
> > -		/* Enable SCLK */
> > -		ret = clk_set_rate(priv->sclk, 3072000);
> > +		ret = clk_set_rate(sclk, sclk_rate);
> >   		if (ret < 0) {
> > -			dev_err(card->dev, "Can't set rate for sclk, err: %d\n",
> > -				ret);
> > +			dev_err(card->dev, "Can't set rate for sclk for ssp%d,
> err: %d\n",
> > +				ssp_num, ret);
> >   			clk_disable_unprepare(priv->mclk);
> >   			return ret;
> >   		}
> >
> > -		ret = clk_prepare_enable(priv->sclk);
> > -		if (ret < 0) {
> > -			dev_err(card->dev, "Can't enable sclk, err: %d\n", ret);
> > -			clk_disable_unprepare(priv->mclk);
> > +		if (!__clk_is_enabled(sclk)) {
> 
> Why do you need this test? the sclocks are not shared? see also [2] below
My thought process was if the clock is already enabled, then we don't have to enable it. 
Isee your point, I can skip this check. This check will always be true.
> 
> > +			/* Enable SCLK */
> > +			ret = clk_prepare_enable(sclk);
> > +			if (ret < 0) {
> > +				dev_err(card->dev, "Can't enable sclk for
> ssp%d, err: %d\n",
> > +					ssp_num, ret);
> > +				clk_disable_unprepare(priv->mclk);
> > +				return ret;
> > +			}
> >   		}
> >   		break;
> >   	case SND_SOC_DAPM_POST_PMD:
> > -		clk_disable_unprepare(priv->mclk);
> > -		clk_disable_unprepare(priv->sclk);
> > +		if (__clk_is_enabled(priv->mclk))
> > +			clk_disable_unprepare(priv->mclk);
> > +
> 
> [1] this seems wrong in case you have two SSPs working, and stop one.
> This would turn off the mclk while one of the two SSPs is still working.
For this platform we use either headset or dmic. 
There is no way we can record simultaneously using different devices.
So disabling mclk might not be harmful here. But this case will always be true too :). 
> 
> > +		if (__clk_is_enabled(sclk))
> 
> [2] Again is this test needed since sclk is not shared between SSPs
Same thought process to check if its enabled or not. Will remove that.
> 
> > +			clk_disable_unprepare(sclk);

Re: [PATCH v2] ASoC: Intel: boards: eve: Fix DMIC records zero

[Pierre-Louis Bossart]

>>>    	case SND_SOC_DAPM_POST_PMD:
>>> -		clk_disable_unprepare(priv->mclk);
>>> -		clk_disable_unprepare(priv->sclk);
>>> +		if (__clk_is_enabled(priv->mclk))
>>> +			clk_disable_unprepare(priv->mclk);
>>> +
>>
>> [1] this seems wrong in case you have two SSPs working, and stop one.
>> This would turn off the mclk while one of the two SSPs is still working.
> For this platform we use either headset or dmic.
> There is no way we can record simultaneously using different devices.
> So disabling mclk might not be harmful here. But this case will always be true too :).

Maybe CRAS prevents you from recording on two inputs, but it looks like 
you have independent front-ends so in theory couldn't you record at the 
alsa hw: device level? Is this really mutually exclusive at the hardware 
level?

Also is the clock only needed for the rt5663 and rt5514, the amplifier 
does not need it?

>>
>>> +		if (__clk_is_enabled(sclk))
>>
>> [2] Again is this test needed since sclk is not shared between SSPs
> Same thought process to check if its enabled or not. Will remove that.
>>
>>> +			clk_disable_unprepare(sclk);
> 

RE: [PATCH v2] ASoC: Intel: boards: eve: Fix DMIC records zero

[Lu, Brent]

> 
> This patch adds a dapm route to provide early mclk/sclk for for DMIC on
> SSP0(rt5514) and Headset on SSP1(rt5663).
> 
> The sclk rate for both codecs is different which is taken care of in the
> platform_clock_control().The platform has one mclk and one sclk. Two
> variables sclk0 and sclk1 are used for the two codecs on differnet ssp that use
> different clock rate.
> 
> This change ensures the DMIC PCM port will return valid data
> 
> Signed-off-by: Brent Lu <brent.lu@intel.com>
> Signed-off-by: Vamshi Krishna Gopal <vamshi.krishna.gopal@intel.com>
> Signed-off-by: Harsha Priya <harshapriya.n@intel.com>
Hi Harsha,

Isn't it working? I tested it on a eve before upstreaming. Thanks.

commit 15747a80207585fe942416025540c0ff34e2aef8
Author: Brent Lu <brent.lu@intel.com>
Date:   Fri Oct 25 17:11:31 2019 +0800

    ASoC: eve: implement set_bias_level function for rt5514

    The first DMIC capture always fail (zero sequence data from PCM port)
    after using DSP hotwording function (i.e. Google assistant).

    This rt5514 codec requires to control mclk directly in the set_bias_level
    function. Implement this function in machine driver to control the
    ssp1_mclk clock explicitly could fix this issue.

    Signed-off-by: Brent Lu <brent.lu@intel.com>
    Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
    Link: https://lore.kernel.org/r/1571994691-20199-1-git-send-email-brent.lu@intel.com
    Signed-off-by: Mark Brown <broonie@kernel.org>

Regards,
Brent

> ---
> v1 -> v2:
> - Only one mclk with same rate is used, so changed to using one variable
> - dropping ssp_ prefix from sclk variable names to make them sound right.
> - removing a return statment that was not required
> - fixed commit message accordingly
> 
>  .../soc/intel/boards/kbl_rt5663_rt5514_max98927.c  | 146
> ++++++++++++++-------
>  1 file changed, 97 insertions(+), 49 deletions(-)
> 
> diff --git a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
> b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
> index b34cf6c..155f2b4 100644
> --- a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
> +++ b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
> @@ -54,7 +54,8 @@ struct kbl_codec_private {
>  	struct list_head hdmi_pcm_list;
>  	struct snd_soc_jack kabylake_hdmi[2];
>  	struct clk *mclk;
> -	struct clk *sclk;
> +	struct clk *sclk0;
> +	struct clk *sclk1;
>  };
> 
>  enum {
> @@ -77,13 +78,29 @@ static const struct snd_kcontrol_new
> kabylake_controls[] = {  };
> 
>  static int platform_clock_control(struct snd_soc_dapm_widget *w,
> -			struct snd_kcontrol *k, int  event)
> +			struct snd_kcontrol *k, int event, int ssp_num)
>  {
>  	struct snd_soc_dapm_context *dapm = w->dapm;
>  	struct snd_soc_card *card = dapm->card;
>  	struct kbl_codec_private *priv = snd_soc_card_get_drvdata(card);
> +	struct clk *sclk;
> +	unsigned long sclk_rate;
>  	int ret = 0;
> 
> +	switch (ssp_num) {
> +	case 0:
> +		sclk = priv->sclk0;
> +		sclk_rate = 6144000;
> +		break;
> +	case 1:
> +		sclk = priv->sclk1;
> +		sclk_rate = 3072000;
> +		break;
> +	default:
> +		dev_err(card->dev, "Invalid ssp_num %d\n", ssp_num);
> +		return -EINVAL;
> +	}
> +
>  	/*
>  	 * MCLK/SCLK need to be ON early for a successful synchronization of
>  	 * codec internal clock. And the clocks are turned off during @@ -
> 91,38 +108,48 @@ static int platform_clock_control(struct
> snd_soc_dapm_widget *w,
>  	 */
>  	switch (event) {
>  	case SND_SOC_DAPM_PRE_PMU:
> -		/* Enable MCLK */
>  		ret = clk_set_rate(priv->mclk, 24000000);
>  		if (ret < 0) {
> -			dev_err(card->dev, "Can't set rate for mclk, err:
> %d\n",
> -				ret);
> -			return ret;
> +			dev_err(card->dev, "Can't set rate for mclk for ssp%d,
> err: %d\n",
> +				ssp_num, ret);
> +				return ret;
>  		}
> 
> -		ret = clk_prepare_enable(priv->mclk);
> -		if (ret < 0) {
> -			dev_err(card->dev, "Can't enable mclk, err: %d\n",
> ret);
> -			return ret;
> +		if (!__clk_is_enabled(priv->mclk)) {
> +			/* Enable MCLK */
> +			ret = clk_prepare_enable(priv->mclk);
> +			if (ret < 0) {
> +				dev_err(card->dev, "Can't enable mclk for
> ssp%d, err: %d\n",
> +					ssp_num, ret);
> +				return ret;
> +			}
>  		}
> 
> -		/* Enable SCLK */
> -		ret = clk_set_rate(priv->sclk, 3072000);
> +		ret = clk_set_rate(sclk, sclk_rate);
>  		if (ret < 0) {
> -			dev_err(card->dev, "Can't set rate for sclk, err:
> %d\n",
> -				ret);
> +			dev_err(card->dev, "Can't set rate for sclk for ssp%d,
> err: %d\n",
> +				ssp_num, ret);
>  			clk_disable_unprepare(priv->mclk);
>  			return ret;
>  		}
> 
> -		ret = clk_prepare_enable(priv->sclk);
> -		if (ret < 0) {
> -			dev_err(card->dev, "Can't enable sclk, err: %d\n",
> ret);
> -			clk_disable_unprepare(priv->mclk);
> +		if (!__clk_is_enabled(sclk)) {
> +			/* Enable SCLK */
> +			ret = clk_prepare_enable(sclk);
> +			if (ret < 0) {
> +				dev_err(card->dev, "Can't enable sclk for
> ssp%d, err: %d\n",
> +					ssp_num, ret);
> +				clk_disable_unprepare(priv->mclk);
> +				return ret;
> +			}
>  		}
>  		break;
>  	case SND_SOC_DAPM_POST_PMD:
> -		clk_disable_unprepare(priv->mclk);
> -		clk_disable_unprepare(priv->sclk);
> +		if (__clk_is_enabled(priv->mclk))
> +			clk_disable_unprepare(priv->mclk);
> +
> +		if (__clk_is_enabled(sclk))
> +			clk_disable_unprepare(sclk);
>  		break;
>  	default:
>  		return 0;
> @@ -131,6 +158,18 @@ static int platform_clock_control(struct
> snd_soc_dapm_widget *w,
>  	return 0;
>  }
> 
> +static int platform_clock_control_ssp0(struct snd_soc_dapm_widget *w,
> +			struct snd_kcontrol *k, int event)
> +{
> +	return platform_clock_control(w, k, event, 0); }
> +
> +static int platform_clock_control_ssp1(struct snd_soc_dapm_widget *w,
> +			struct snd_kcontrol *k, int event)
> +{
> +	return platform_clock_control(w, k, event, 1); }
> +
>  static const struct snd_soc_dapm_widget kabylake_widgets[] = {
>  	SND_SOC_DAPM_HP("Headphone Jack", NULL),
>  	SND_SOC_DAPM_MIC("Headset Mic", NULL), @@ -139,15 +178,17
> @@ static const struct snd_soc_dapm_widget kabylake_widgets[] = {
>  	SND_SOC_DAPM_MIC("DMIC", NULL),
>  	SND_SOC_DAPM_SPK("HDMI1", NULL),
>  	SND_SOC_DAPM_SPK("HDMI2", NULL),
> -	SND_SOC_DAPM_SUPPLY("Platform Clock", SND_SOC_NOPM, 0, 0,
> -			platform_clock_control, SND_SOC_DAPM_PRE_PMU
> |
> +	SND_SOC_DAPM_SUPPLY("Platform Clock SSP0", SND_SOC_NOPM,
> 0, 0,
> +			platform_clock_control_ssp0,
> SND_SOC_DAPM_PRE_PMU |
> +			SND_SOC_DAPM_POST_PMD),
> +	SND_SOC_DAPM_SUPPLY("Platform Clock SSP1", SND_SOC_NOPM,
> 0, 0,
> +			platform_clock_control_ssp1,
> SND_SOC_DAPM_PRE_PMU |
>  			SND_SOC_DAPM_POST_PMD),
> -
>  };
> 
>  static const struct snd_soc_dapm_route kabylake_map[] = {
>  	/* Headphones */
> -	{ "Headphone Jack", NULL, "Platform Clock" },
> +	{ "Headphone Jack", NULL, "Platform Clock SSP1" },
>  	{ "Headphone Jack", NULL, "HPOL" },
>  	{ "Headphone Jack", NULL, "HPOR" },
> 
> @@ -156,7 +197,7 @@ static const struct snd_soc_dapm_route
> kabylake_map[] = {
>  	{ "Right Spk", NULL, "Right BE_OUT" },
> 
>  	/* other jacks */
> -	{ "Headset Mic", NULL, "Platform Clock" },
> +	{ "Headset Mic", NULL, "Platform Clock SSP1" },
>  	{ "IN1P", NULL, "Headset Mic" },
>  	{ "IN1N", NULL, "Headset Mic" },
> 
> @@ -180,6 +221,7 @@ static const struct snd_soc_dapm_route
> kabylake_map[] = {
>  	{ "ssp0 Rx", NULL, "Right HiFi Capture" },
> 
>  	/* DMIC */
> +	{ "DMIC", NULL, "Platform Clock SSP0" },
>  	{ "DMIC1L", NULL, "DMIC" },
>  	{ "DMIC1R", NULL, "DMIC" },
>  	{ "DMIC2L", NULL, "DMIC" },
> @@ -666,7 +708,7 @@ static int kabylake_set_bias_level(struct
> snd_soc_card *card,
>  	if (!component || strcmp(component->name, RT5514_DEV_NAME))
>  		return 0;
> 
> -	if (IS_ERR(priv->mclk))
> +	if (IS_ERR(priv->mclk0))
>  		return 0;
> 
>  	/*
> @@ -757,6 +799,28 @@ static struct snd_soc_card kabylake_audio_card = {
>  	.late_probe = kabylake_card_late_probe,  };
> 
> +static int kabylake_audio_clk_get(struct device *dev, const char *id,
> +	struct clk **clk)
> +{
> +	int ret = 0;
> +
> +	if (!clk)
> +		return -EINVAL;
> +
> +	*clk = devm_clk_get(dev, id);
> +	if (IS_ERR(*clk)) {
> +		ret = PTR_ERR(*clk);
> +		if (ret == -ENOENT) {
> +			dev_info(dev, "Failed to get %s, defer probe\n", id);
> +			return -EPROBE_DEFER;
> +		}
> +
> +		dev_err(dev, "Failed to get %s with err:%d\n", id, ret);
> +	}
> +
> +	return ret;
> +}
> +
>  static int kabylake_audio_probe(struct platform_device *pdev)  {
>  	struct kbl_codec_private *ctx;
> @@ -777,33 +841,17 @@ static int kabylake_audio_probe(struct
> platform_device *pdev)
>  		dmic_constraints = mach->mach_params.dmic_num == 2 ?
>  			&constraints_dmic_2ch :
> &constraints_dmic_channels;
> 
> -	ctx->mclk = devm_clk_get(&pdev->dev, "ssp1_mclk");
> -	if (IS_ERR(ctx->mclk)) {
> -		ret = PTR_ERR(ctx->mclk);
> -		if (ret == -ENOENT) {
> -			dev_info(&pdev->dev,
> -				"Failed to get ssp1_mclk, defer probe\n");
> -			return -EPROBE_DEFER;
> -		}
> -
> -		dev_err(&pdev->dev, "Failed to get ssp1_mclk with
> err:%d\n",
> -								ret);
> +	ret = kabylake_audio_clk_get(&pdev->dev, "ssp0_sclk", &ctx->sclk0);
> +	if (ret != 0)
>  		return ret;
> -	}
> 
> -	ctx->sclk = devm_clk_get(&pdev->dev, "ssp1_sclk");
> -	if (IS_ERR(ctx->sclk)) {
> -		ret = PTR_ERR(ctx->sclk);
> -		if (ret == -ENOENT) {
> -			dev_info(&pdev->dev,
> -				"Failed to get ssp1_sclk, defer probe\n");
> -			return -EPROBE_DEFER;
> -		}
> +	ret = kabylake_audio_clk_get(&pdev->dev, "ssp1_mclk", &ctx-
> >mclk);
> +	if (ret != 0)
> +		return ret;
> 
> -		dev_err(&pdev->dev, "Failed to get ssp1_sclk with err:%d\n",
> -								ret);
> +	ret = kabylake_audio_clk_get(&pdev->dev, "ssp1_sclk", &ctx->sclk1);
> +	if (ret != 0)
>  		return ret;
> -	}
> 
>  	return devm_snd_soc_register_card(&pdev->dev,
> &kabylake_audio_card);  }
> --
> 2.7.4

RE: [PATCH v2] ASoC: Intel: boards: eve: Fix DMIC records zero

[N, Harshapriya]

> 
> >>>    	case SND_SOC_DAPM_POST_PMD:
> >>> -		clk_disable_unprepare(priv->mclk);
> >>> -		clk_disable_unprepare(priv->sclk);
> >>> +		if (__clk_is_enabled(priv->mclk))
> >>> +			clk_disable_unprepare(priv->mclk);
> >>> +
> >>
> >> [1] this seems wrong in case you have two SSPs working, and stop one.
> >> This would turn off the mclk while one of the two SSPs is still working.
> > For this platform we use either headset or dmic.
> > There is no way we can record simultaneously using different devices.
> > So disabling mclk might not be harmful here. But this case will always be true
> too :).
> 
> Maybe CRAS prevents you from recording on two inputs, but it looks like you
> have independent front-ends so in theory couldn't you record at the alsa hw:
> device level? Is this really mutually exclusive at the hardware level?
True. Its not mutually exclusive at hardware level. the following might be safe
if (!__clk_is_enabled(priv->sclk0)) &&  (!__clk_is_enabled(priv->sclk1))
	clk_disable_unprepare(priv->mclk);
> 
> Also is the clock only needed for the rt5663 and rt5514, the amplifier does not
> need it?
That’s right. max98927 does not need mclk.
> 
> >>
> >>> +		if (__clk_is_enabled(sclk))
> >>
> >> [2] Again is this test needed since sclk is not shared between SSPs
> > Same thought process to check if its enabled or not. Will remove that.
> >>
> >>> +			clk_disable_unprepare(sclk);
> >

RE: [PATCH v2] ASoC: Intel: boards: eve: Fix DMIC records zero

[N, Harshapriya]

> 
> >
> > This patch adds a dapm route to provide early mclk/sclk for for DMIC
> > on
> > SSP0(rt5514) and Headset on SSP1(rt5663).
> >
> > The sclk rate for both codecs is different which is taken care of in
> > the platform_clock_control().The platform has one mclk and one sclk.
> > Two variables sclk0 and sclk1 are used for the two codecs on differnet
> > ssp that use different clock rate.
> >
> > This change ensures the DMIC PCM port will return valid data
> >
> > Signed-off-by: Brent Lu <brent.lu@intel.com>
> > Signed-off-by: Vamshi Krishna Gopal <vamshi.krishna.gopal@intel.com>
> > Signed-off-by: Harsha Priya <harshapriya.n@intel.com>
> Hi Harsha,
> 
> Isn't it working? I tested it on a eve before upstreaming. Thanks.
> 
> commit 15747a80207585fe942416025540c0ff34e2aef8
> Author: Brent Lu <brent.lu@intel.com>
> Date:   Fri Oct 25 17:11:31 2019 +0800
> 
>     ASoC: eve: implement set_bias_level function for rt5514
> 
>     The first DMIC capture always fail (zero sequence data from PCM port)
>     after using DSP hotwording function (i.e. Google assistant).
> 
>     This rt5514 codec requires to control mclk directly in the set_bias_level
>     function. Implement this function in machine driver to control the
>     ssp1_mclk clock explicitly could fix this issue.
> 
>     Signed-off-by: Brent Lu <brent.lu@intel.com>
>     Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>     Link: https://lore.kernel.org/r/1571994691-20199-1-git-send-email-
> brent.lu@intel.com
>     Signed-off-by: Mark Brown <broonie@kernel.org>
Your patch is necessary and is being used.
But we found few issues during kernel uprev where we need this current patch 
also to get it working
> 
> Regards,
> Brent
> 
> > ---
> > v1 -> v2:
> > - Only one mclk with same rate is used, so changed to using one
> > variable
> > - dropping ssp_ prefix from sclk variable names to make them sound right.
> > - removing a return statment that was not required
> > - fixed commit message accordingly
> >
> >  .../soc/intel/boards/kbl_rt5663_rt5514_max98927.c  | 146
> > ++++++++++++++-------
> >  1 file changed, 97 insertions(+), 49 deletions(-)
> >
> > diff --git a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
> > b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
> > index b34cf6c..155f2b4 100644
> > --- a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
> > +++ b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
> > @@ -54,7 +54,8 @@ struct kbl_codec_private {
> >  	struct list_head hdmi_pcm_list;
> >  	struct snd_soc_jack kabylake_hdmi[2];
> >  	struct clk *mclk;
> > -	struct clk *sclk;
> > +	struct clk *sclk0;
> > +	struct clk *sclk1;
> >  };
> >
> >  enum {
> > @@ -77,13 +78,29 @@ static const struct snd_kcontrol_new
> > kabylake_controls[] = {  };
> >
> >  static int platform_clock_control(struct snd_soc_dapm_widget *w,
> > -			struct snd_kcontrol *k, int  event)
> > +			struct snd_kcontrol *k, int event, int ssp_num)
> >  {
> >  	struct snd_soc_dapm_context *dapm = w->dapm;
> >  	struct snd_soc_card *card = dapm->card;
> >  	struct kbl_codec_private *priv = snd_soc_card_get_drvdata(card);
> > +	struct clk *sclk;
> > +	unsigned long sclk_rate;
> >  	int ret = 0;
> >
> > +	switch (ssp_num) {
> > +	case 0:
> > +		sclk = priv->sclk0;
> > +		sclk_rate = 6144000;
> > +		break;
> > +	case 1:
> > +		sclk = priv->sclk1;
> > +		sclk_rate = 3072000;
> > +		break;
> > +	default:
> > +		dev_err(card->dev, "Invalid ssp_num %d\n", ssp_num);
> > +		return -EINVAL;
> > +	}
> > +
> >  	/*
> >  	 * MCLK/SCLK need to be ON early for a successful synchronization of
> >  	 * codec internal clock. And the clocks are turned off during @@ -
> > 91,38 +108,48 @@ static int platform_clock_control(struct
> > snd_soc_dapm_widget *w,
> >  	 */
> >  	switch (event) {
> >  	case SND_SOC_DAPM_PRE_PMU:
> > -		/* Enable MCLK */
> >  		ret = clk_set_rate(priv->mclk, 24000000);
> >  		if (ret < 0) {
> > -			dev_err(card->dev, "Can't set rate for mclk, err:
> > %d\n",
> > -				ret);
> > -			return ret;
> > +			dev_err(card->dev, "Can't set rate for mclk for ssp%d,
> > err: %d\n",
> > +				ssp_num, ret);
> > +				return ret;
> >  		}
> >
> > -		ret = clk_prepare_enable(priv->mclk);
> > -		if (ret < 0) {
> > -			dev_err(card->dev, "Can't enable mclk, err: %d\n",
> > ret);
> > -			return ret;
> > +		if (!__clk_is_enabled(priv->mclk)) {
> > +			/* Enable MCLK */
> > +			ret = clk_prepare_enable(priv->mclk);
> > +			if (ret < 0) {
> > +				dev_err(card->dev, "Can't enable mclk for
> > ssp%d, err: %d\n",
> > +					ssp_num, ret);
> > +				return ret;
> > +			}
> >  		}
> >
> > -		/* Enable SCLK */
> > -		ret = clk_set_rate(priv->sclk, 3072000);
> > +		ret = clk_set_rate(sclk, sclk_rate);
> >  		if (ret < 0) {
> > -			dev_err(card->dev, "Can't set rate for sclk, err:
> > %d\n",
> > -				ret);
> > +			dev_err(card->dev, "Can't set rate for sclk for ssp%d,
> > err: %d\n",
> > +				ssp_num, ret);
> >  			clk_disable_unprepare(priv->mclk);
> >  			return ret;
> >  		}
> >
> > -		ret = clk_prepare_enable(priv->sclk);
> > -		if (ret < 0) {
> > -			dev_err(card->dev, "Can't enable sclk, err: %d\n",
> > ret);
> > -			clk_disable_unprepare(priv->mclk);
> > +		if (!__clk_is_enabled(sclk)) {
> > +			/* Enable SCLK */
> > +			ret = clk_prepare_enable(sclk);
> > +			if (ret < 0) {
> > +				dev_err(card->dev, "Can't enable sclk for
> > ssp%d, err: %d\n",
> > +					ssp_num, ret);
> > +				clk_disable_unprepare(priv->mclk);
> > +				return ret;
> > +			}
> >  		}
> >  		break;
> >  	case SND_SOC_DAPM_POST_PMD:
> > -		clk_disable_unprepare(priv->mclk);
> > -		clk_disable_unprepare(priv->sclk);
> > +		if (__clk_is_enabled(priv->mclk))
> > +			clk_disable_unprepare(priv->mclk);
> > +
> > +		if (__clk_is_enabled(sclk))
> > +			clk_disable_unprepare(sclk);
> >  		break;
> >  	default:
> >  		return 0;
> > @@ -131,6 +158,18 @@ static int platform_clock_control(struct
> > snd_soc_dapm_widget *w,
> >  	return 0;
> >  }
> >
> > +static int platform_clock_control_ssp0(struct snd_soc_dapm_widget *w,
> > +			struct snd_kcontrol *k, int event) {
> > +	return platform_clock_control(w, k, event, 0); }
> > +
> > +static int platform_clock_control_ssp1(struct snd_soc_dapm_widget *w,
> > +			struct snd_kcontrol *k, int event) {
> > +	return platform_clock_control(w, k, event, 1); }
> > +
> >  static const struct snd_soc_dapm_widget kabylake_widgets[] = {
> >  	SND_SOC_DAPM_HP("Headphone Jack", NULL),
> >  	SND_SOC_DAPM_MIC("Headset Mic", NULL), @@ -139,15 +178,17
> @@ static
> > const struct snd_soc_dapm_widget kabylake_widgets[] = {
> >  	SND_SOC_DAPM_MIC("DMIC", NULL),
> >  	SND_SOC_DAPM_SPK("HDMI1", NULL),
> >  	SND_SOC_DAPM_SPK("HDMI2", NULL),
> > -	SND_SOC_DAPM_SUPPLY("Platform Clock", SND_SOC_NOPM, 0, 0,
> > -			platform_clock_control, SND_SOC_DAPM_PRE_PMU
> > |
> > +	SND_SOC_DAPM_SUPPLY("Platform Clock SSP0", SND_SOC_NOPM,
> > 0, 0,
> > +			platform_clock_control_ssp0,
> > SND_SOC_DAPM_PRE_PMU |
> > +			SND_SOC_DAPM_POST_PMD),
> > +	SND_SOC_DAPM_SUPPLY("Platform Clock SSP1", SND_SOC_NOPM,
> > 0, 0,
> > +			platform_clock_control_ssp1,
> > SND_SOC_DAPM_PRE_PMU |
> >  			SND_SOC_DAPM_POST_PMD),
> > -
> >  };
> >
> >  static const struct snd_soc_dapm_route kabylake_map[] = {
> >  	/* Headphones */
> > -	{ "Headphone Jack", NULL, "Platform Clock" },
> > +	{ "Headphone Jack", NULL, "Platform Clock SSP1" },
> >  	{ "Headphone Jack", NULL, "HPOL" },
> >  	{ "Headphone Jack", NULL, "HPOR" },
> >
> > @@ -156,7 +197,7 @@ static const struct snd_soc_dapm_route
> > kabylake_map[] = {
> >  	{ "Right Spk", NULL, "Right BE_OUT" },
> >
> >  	/* other jacks */
> > -	{ "Headset Mic", NULL, "Platform Clock" },
> > +	{ "Headset Mic", NULL, "Platform Clock SSP1" },
> >  	{ "IN1P", NULL, "Headset Mic" },
> >  	{ "IN1N", NULL, "Headset Mic" },
> >
> > @@ -180,6 +221,7 @@ static const struct snd_soc_dapm_route
> > kabylake_map[] = {
> >  	{ "ssp0 Rx", NULL, "Right HiFi Capture" },
> >
> >  	/* DMIC */
> > +	{ "DMIC", NULL, "Platform Clock SSP0" },
> >  	{ "DMIC1L", NULL, "DMIC" },
> >  	{ "DMIC1R", NULL, "DMIC" },
> >  	{ "DMIC2L", NULL, "DMIC" },
> > @@ -666,7 +708,7 @@ static int kabylake_set_bias_level(struct
> > snd_soc_card *card,
> >  	if (!component || strcmp(component->name, RT5514_DEV_NAME))
> >  		return 0;
> >
> > -	if (IS_ERR(priv->mclk))
> > +	if (IS_ERR(priv->mclk0))
> >  		return 0;
> >
> >  	/*
> > @@ -757,6 +799,28 @@ static struct snd_soc_card kabylake_audio_card = {
> >  	.late_probe = kabylake_card_late_probe,  };
> >
> > +static int kabylake_audio_clk_get(struct device *dev, const char *id,
> > +	struct clk **clk)
> > +{
> > +	int ret = 0;
> > +
> > +	if (!clk)
> > +		return -EINVAL;
> > +
> > +	*clk = devm_clk_get(dev, id);
> > +	if (IS_ERR(*clk)) {
> > +		ret = PTR_ERR(*clk);
> > +		if (ret == -ENOENT) {
> > +			dev_info(dev, "Failed to get %s, defer probe\n", id);
> > +			return -EPROBE_DEFER;
> > +		}
> > +
> > +		dev_err(dev, "Failed to get %s with err:%d\n", id, ret);
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> >  static int kabylake_audio_probe(struct platform_device *pdev)  {
> >  	struct kbl_codec_private *ctx;
> > @@ -777,33 +841,17 @@ static int kabylake_audio_probe(struct
> > platform_device *pdev)
> >  		dmic_constraints = mach->mach_params.dmic_num == 2 ?
> >  			&constraints_dmic_2ch :
> > &constraints_dmic_channels;
> >
> > -	ctx->mclk = devm_clk_get(&pdev->dev, "ssp1_mclk");
> > -	if (IS_ERR(ctx->mclk)) {
> > -		ret = PTR_ERR(ctx->mclk);
> > -		if (ret == -ENOENT) {
> > -			dev_info(&pdev->dev,
> > -				"Failed to get ssp1_mclk, defer probe\n");
> > -			return -EPROBE_DEFER;
> > -		}
> > -
> > -		dev_err(&pdev->dev, "Failed to get ssp1_mclk with
> > err:%d\n",
> > -								ret);
> > +	ret = kabylake_audio_clk_get(&pdev->dev, "ssp0_sclk", &ctx->sclk0);
> > +	if (ret != 0)
> >  		return ret;
> > -	}
> >
> > -	ctx->sclk = devm_clk_get(&pdev->dev, "ssp1_sclk");
> > -	if (IS_ERR(ctx->sclk)) {
> > -		ret = PTR_ERR(ctx->sclk);
> > -		if (ret == -ENOENT) {
> > -			dev_info(&pdev->dev,
> > -				"Failed to get ssp1_sclk, defer probe\n");
> > -			return -EPROBE_DEFER;
> > -		}
> > +	ret = kabylake_audio_clk_get(&pdev->dev, "ssp1_mclk", &ctx-
> > >mclk);
> > +	if (ret != 0)
> > +		return ret;
> >
> > -		dev_err(&pdev->dev, "Failed to get ssp1_sclk with err:%d\n",
> > -								ret);
> > +	ret = kabylake_audio_clk_get(&pdev->dev, "ssp1_sclk", &ctx->sclk1);
> > +	if (ret != 0)
> >  		return ret;
> > -	}
> >
> >  	return devm_snd_soc_register_card(&pdev->dev,
> > &kabylake_audio_card);  }
> > --
> > 2.7.4

Re: [PATCH v2] ASoC: Intel: boards: eve: Fix DMIC records zero

[Pierre-Louis Bossart]

>>>>>     	case SND_SOC_DAPM_POST_PMD:
>>>>> -		clk_disable_unprepare(priv->mclk);
>>>>> -		clk_disable_unprepare(priv->sclk);
>>>>> +		if (__clk_is_enabled(priv->mclk))
>>>>> +			clk_disable_unprepare(priv->mclk);
>>>>> +
>>>>
>>>> [1] this seems wrong in case you have two SSPs working, and stop one.
>>>> This would turn off the mclk while one of the two SSPs is still working.
>>> For this platform we use either headset or dmic.
>>> There is no way we can record simultaneously using different devices.
>>> So disabling mclk might not be harmful here. But this case will always be true
>> too :).
>>
>> Maybe CRAS prevents you from recording on two inputs, but it looks like you
>> have independent front-ends so in theory couldn't you record at the alsa hw:
>> device level? Is this really mutually exclusive at the hardware level?
> True. Its not mutually exclusive at hardware level. the following might be safe
> if (!__clk_is_enabled(priv->sclk0)) &&  (!__clk_is_enabled(priv->sclk1))
> 	clk_disable_unprepare(priv->mclk);

I don't understand DAPM well-enough to know if these independent 
platform clock control routines are serialized by design or if this 
could be racy?

Re: [PATCH v2] ASoC: Intel: boards: eve: Fix DMIC records zero

[kernel test robot]

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

Hi Harsha,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on asoc/for-next]
[also build test ERROR on sound/for-next v5.8-rc7 next-20200730]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Harsha-Priya/ASoC-Intel-boards-eve-Fix-DMIC-records-zero/20200731-012912
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
config: x86_64-allyesconfig (attached as .config)
compiler: gcc-9 (Debian 9.3.0-14) 9.3.0
reproduce (this is a W=1 build):
        # save the attached .config to linux build tree
        make W=1 ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c: In function 'kabylake_set_bias_level':
>> sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c:734:19: error: 'struct kbl_codec_private' has no member named 'mclk0'; did you mean 'mclk'?
     734 |  if (IS_ERR(priv->mclk0))
         |                   ^~~~~
         |                   mclk

vim +734 sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c

   723	
   724	static int kabylake_set_bias_level(struct snd_soc_card *card,
   725		struct snd_soc_dapm_context *dapm, enum snd_soc_bias_level level)
   726	{
   727		struct snd_soc_component *component = dapm->component;
   728		struct kbl_codec_private *priv = snd_soc_card_get_drvdata(card);
   729		int ret = 0;
   730	
   731		if (!component || strcmp(component->name, RT5514_DEV_NAME))
   732			return 0;
   733	
 > 734		if (IS_ERR(priv->mclk0))
   735			return 0;
   736	
   737		/*
   738		 * It's required to control mclk directly in the set_bias_level
   739		 * function for rt5514 codec or the recording function could
   740		 * break.
   741		 */
   742		switch (level) {
   743		case SND_SOC_BIAS_PREPARE:
   744			if (dapm->bias_level == SND_SOC_BIAS_ON) {
   745				dev_dbg(card->dev, "Disable mclk");
   746				clk_disable_unprepare(priv->mclk);
   747			} else {
   748				dev_dbg(card->dev, "Enable mclk");
   749				ret = clk_set_rate(priv->mclk, 24000000);
   750				if (ret) {
   751					dev_err(card->dev, "Can't set rate for mclk, err: %d\n",
   752						ret);
   753					return ret;
   754				}
   755	
   756				ret = clk_prepare_enable(priv->mclk);
   757				if (ret) {
   758					dev_err(card->dev, "Can't enable mclk, err: %d\n",
   759						ret);
   760	
   761					/* mclk is already enabled in FW */
   762					ret = 0;
   763				}
   764			}
   765			break;
   766		default:
   767			break;
   768		}
   769	
   770		return ret;
   771	}
   772	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 75288 bytes --]

RE: [PATCH v2] ASoC: Intel: boards: eve: Fix DMIC records zero

[N, Harshapriya]

Abandoning this patch as it some clock changes that is already fixed in Brent's older patch
(handled it in a different way). 
Will make a separate patch for other fixes once the complete solution is ready.

> 
> >> > >
> > > This patch adds a dapm route to provide early mclk/sclk for for DMIC
> > > on
> > > SSP0(rt5514) and Headset on SSP1(rt5663).
> > >
> > > The sclk rate for both codecs is different which is taken care of in
> > > the platform_clock_control().The platform has one mclk and one sclk.
> > > Two variables sclk0 and sclk1 are used for the two codecs on
> > > differnet ssp that use different clock rate.
> > >
> > > This change ensures the DMIC PCM port will return valid data
> > >
> > > Signed-off-by: Brent Lu <brent.lu@intel.com>
> > > Signed-off-by: Vamshi Krishna Gopal <vamshi.krishna.gopal@intel.com>
> > > Signed-off-by: Harsha Priya <harshapriya.n@intel.com>
> > Hi Harsha,
> >
> > Isn't it working? I tested it on a eve before upstreaming. Thanks.
> >
> > commit 15747a80207585fe942416025540c0ff34e2aef8
> > Author: Brent Lu <brent.lu@intel.com>
> > Date:   Fri Oct 25 17:11:31 2019 +0800
> >
> >     ASoC: eve: implement set_bias_level function for rt5514
> >
> >     The first DMIC capture always fail (zero sequence data from PCM port)
> >     after using DSP hotwording function (i.e. Google assistant).
> >
> >     This rt5514 codec requires to control mclk directly in the set_bias_level
> >     function. Implement this function in machine driver to control the
> >     ssp1_mclk clock explicitly could fix this issue.
> >
> >     Signed-off-by: Brent Lu <brent.lu@intel.com>
> >     Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> >     Link: https://lore.kernel.org/r/1571994691-20199-1-git-send-email-
> > brent.lu@intel.com
> >     Signed-off-by: Mark Brown <broonie@kernel.org>
> Your patch is necessary and is being used.
> But we found few issues during kernel uprev where we need this current patch
> also to get it working
> >
> > Regards,
> > Brent
> >
> > > ---
> > > v1 -> v2:
> > > - Only one mclk with same rate is used, so changed to using one
> > > variable
> > > - dropping ssp_ prefix from sclk variable names to make them sound right.
> > > - removing a return statment that was not required
> > > - fixed commit message accordingly
> > >
> > >  .../soc/intel/boards/kbl_rt5663_rt5514_max98927.c  | 146
> > > ++++++++++++++-------
> > >  1 file changed, 97 insertions(+), 49 deletions(-)
> > >
> > > diff --git a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
> > > b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
> > > index b34cf6c..155f2b4 100644
> > > --- a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
> > > +++ b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
> > > @@ -54,7 +54,8 @@ struct kbl_codec_private {
> > >  	struct list_head hdmi_pcm_list;
> > >  	struct snd_soc_jack kabylake_hdmi[2];
> > >  	struct clk *mclk;
> > > -	struct clk *sclk;
> > > +	struct clk *sclk0;
> > > +	struct clk *sclk1;
> > >  };
> > >
> > >  enum {
> > > @@ -77,13 +78,29 @@ static const struct snd_kcontrol_new
> > > kabylake_controls[] = {  };
> > >
> > >  static int platform_clock_control(struct snd_soc_dapm_widget *w,
> > > -			struct snd_kcontrol *k, int  event)
> > > +			struct snd_kcontrol *k, int event, int ssp_num)
> > >  {
> > >  	struct snd_soc_dapm_context *dapm = w->dapm;
> > >  	struct snd_soc_card *card = dapm->card;
> > >  	struct kbl_codec_private *priv = snd_soc_card_get_drvdata(card);
> > > +	struct clk *sclk;
> > > +	unsigned long sclk_rate;
> > >  	int ret = 0;
> > >
> > > +	switch (ssp_num) {
> > > +	case 0:
> > > +		sclk = priv->sclk0;
> > > +		sclk_rate = 6144000;
> > > +		break;
> > > +	case 1:
> > > +		sclk = priv->sclk1;
> > > +		sclk_rate = 3072000;
> > > +		break;
> > > +	default:
> > > +		dev_err(card->dev, "Invalid ssp_num %d\n", ssp_num);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > >  	/*
> > >  	 * MCLK/SCLK need to be ON early for a successful synchronization of
> > >  	 * codec internal clock. And the clocks are turned off during @@ -
> > > 91,38 +108,48 @@ static int platform_clock_control(struct
> > > snd_soc_dapm_widget *w,
> > >  	 */
> > >  	switch (event) {
> > >  	case SND_SOC_DAPM_PRE_PMU:
> > > -		/* Enable MCLK */
> > >  		ret = clk_set_rate(priv->mclk, 24000000);
> > >  		if (ret < 0) {
> > > -			dev_err(card->dev, "Can't set rate for mclk, err:
> > > %d\n",
> > > -				ret);
> > > -			return ret;
> > > +			dev_err(card->dev, "Can't set rate for mclk for ssp%d,
> > > err: %d\n",
> > > +				ssp_num, ret);
> > > +				return ret;
> > >  		}
> > >
> > > -		ret = clk_prepare_enable(priv->mclk);
> > > -		if (ret < 0) {
> > > -			dev_err(card->dev, "Can't enable mclk, err: %d\n",
> > > ret);
> > > -			return ret;
> > > +		if (!__clk_is_enabled(priv->mclk)) {
> > > +			/* Enable MCLK */
> > > +			ret = clk_prepare_enable(priv->mclk);
> > > +			if (ret < 0) {
> > > +				dev_err(card->dev, "Can't enable mclk for
> > > ssp%d, err: %d\n",
> > > +					ssp_num, ret);
> > > +				return ret;
> > > +			}
> > >  		}
> > >
> > > -		/* Enable SCLK */
> > > -		ret = clk_set_rate(priv->sclk, 3072000);
> > > +		ret = clk_set_rate(sclk, sclk_rate);
> > >  		if (ret < 0) {
> > > -			dev_err(card->dev, "Can't set rate for sclk, err:
> > > %d\n",
> > > -				ret);
> > > +			dev_err(card->dev, "Can't set rate for sclk for ssp%d,
> > > err: %d\n",
> > > +				ssp_num, ret);
> > >  			clk_disable_unprepare(priv->mclk);
> > >  			return ret;
> > >  		}
> > >
> > > -		ret = clk_prepare_enable(priv->sclk);
> > > -		if (ret < 0) {
> > > -			dev_err(card->dev, "Can't enable sclk, err: %d\n",
> > > ret);
> > > -			clk_disable_unprepare(priv->mclk);
> > > +		if (!__clk_is_enabled(sclk)) {
> > > +			/* Enable SCLK */
> > > +			ret = clk_prepare_enable(sclk);
> > > +			if (ret < 0) {
> > > +				dev_err(card->dev, "Can't enable sclk for
> > > ssp%d, err: %d\n",
> > > +					ssp_num, ret);
> > > +				clk_disable_unprepare(priv->mclk);
> > > +				return ret;
> > > +			}
> > >  		}
> > >  		break;
> > >  	case SND_SOC_DAPM_POST_PMD:
> > > -		clk_disable_unprepare(priv->mclk);
> > > -		clk_disable_unprepare(priv->sclk);
> > > +		if (__clk_is_enabled(priv->mclk))
> > > +			clk_disable_unprepare(priv->mclk);
> > > +
> > > +		if (__clk_is_enabled(sclk))
> > > +			clk_disable_unprepare(sclk);
> > >  		break;
> > >  	default:
> > >  		return 0;
> > > @@ -131,6 +158,18 @@ static int platform_clock_control(struct
> > > snd_soc_dapm_widget *w,
> > >  	return 0;
> > >  }
> > >
> > > +static int platform_clock_control_ssp0(struct snd_soc_dapm_widget *w,
> > > +			struct snd_kcontrol *k, int event) {
> > > +	return platform_clock_control(w, k, event, 0); }
> > > +
> > > +static int platform_clock_control_ssp1(struct snd_soc_dapm_widget *w,
> > > +			struct snd_kcontrol *k, int event) {
> > > +	return platform_clock_control(w, k, event, 1); }
> > > +
> > >  static const struct snd_soc_dapm_widget kabylake_widgets[] = {
> > >  	SND_SOC_DAPM_HP("Headphone Jack", NULL),
> > >  	SND_SOC_DAPM_MIC("Headset Mic", NULL), @@ -139,15 +178,17
> > @@ static
> > > const struct snd_soc_dapm_widget kabylake_widgets[] = {
> > >  	SND_SOC_DAPM_MIC("DMIC", NULL),
> > >  	SND_SOC_DAPM_SPK("HDMI1", NULL),
> > >  	SND_SOC_DAPM_SPK("HDMI2", NULL),
> > > -	SND_SOC_DAPM_SUPPLY("Platform Clock", SND_SOC_NOPM, 0, 0,
> > > -			platform_clock_control, SND_SOC_DAPM_PRE_PMU
> > > |
> > > +	SND_SOC_DAPM_SUPPLY("Platform Clock SSP0", SND_SOC_NOPM,
> > > 0, 0,
> > > +			platform_clock_control_ssp0,
> > > SND_SOC_DAPM_PRE_PMU |
> > > +			SND_SOC_DAPM_POST_PMD),
> > > +	SND_SOC_DAPM_SUPPLY("Platform Clock SSP1", SND_SOC_NOPM,
> > > 0, 0,
> > > +			platform_clock_control_ssp1,
> > > SND_SOC_DAPM_PRE_PMU |
> > >  			SND_SOC_DAPM_POST_PMD),
> > > -
> > >  };
> > >
> > >  static const struct snd_soc_dapm_route kabylake_map[] = {
> > >  	/* Headphones */
> > > -	{ "Headphone Jack", NULL, "Platform Clock" },
> > > +	{ "Headphone Jack", NULL, "Platform Clock SSP1" },
> > >  	{ "Headphone Jack", NULL, "HPOL" },
> > >  	{ "Headphone Jack", NULL, "HPOR" },
> > >
> > > @@ -156,7 +197,7 @@ static const struct snd_soc_dapm_route
> > > kabylake_map[] = {
> > >  	{ "Right Spk", NULL, "Right BE_OUT" },
> > >
> > >  	/* other jacks */
> > > -	{ "Headset Mic", NULL, "Platform Clock" },
> > > +	{ "Headset Mic", NULL, "Platform Clock SSP1" },
> > >  	{ "IN1P", NULL, "Headset Mic" },
> > >  	{ "IN1N", NULL, "Headset Mic" },
> > >
> > > @@ -180,6 +221,7 @@ static const struct snd_soc_dapm_route
> > > kabylake_map[] = {
> > >  	{ "ssp0 Rx", NULL, "Right HiFi Capture" },
> > >
> > >  	/* DMIC */
> > > +	{ "DMIC", NULL, "Platform Clock SSP0" },
> > >  	{ "DMIC1L", NULL, "DMIC" },
> > >  	{ "DMIC1R", NULL, "DMIC" },
> > >  	{ "DMIC2L", NULL, "DMIC" },
> > > @@ -666,7 +708,7 @@ static int kabylake_set_bias_level(struct
> > > snd_soc_card *card,
> > >  	if (!component || strcmp(component->name, RT5514_DEV_NAME))
> > >  		return 0;
> > >
> > > -	if (IS_ERR(priv->mclk))
> > > +	if (IS_ERR(priv->mclk0))
> > >  		return 0;
> > >
> > >  	/*
> > > @@ -757,6 +799,28 @@ static struct snd_soc_card kabylake_audio_card =
> {
> > >  	.late_probe = kabylake_card_late_probe,  };
> > >
> > > +static int kabylake_audio_clk_get(struct device *dev, const char *id,
> > > +	struct clk **clk)
> > > +{
> > > +	int ret = 0;
> > > +
> > > +	if (!clk)
> > > +		return -EINVAL;
> > > +
> > > +	*clk = devm_clk_get(dev, id);
> > > +	if (IS_ERR(*clk)) {
> > > +		ret = PTR_ERR(*clk);
> > > +		if (ret == -ENOENT) {
> > > +			dev_info(dev, "Failed to get %s, defer probe\n", id);
> > > +			return -EPROBE_DEFER;
> > > +		}
> > > +
> > > +		dev_err(dev, "Failed to get %s with err:%d\n", id, ret);
> > > +	}
> > > +
> > > +	return ret;
> > > +}
> > > +
> > >  static int kabylake_audio_probe(struct platform_device *pdev)  {
> > >  	struct kbl_codec_private *ctx;
> > > @@ -777,33 +841,17 @@ static int kabylake_audio_probe(struct
> > > platform_device *pdev)
> > >  		dmic_constraints = mach->mach_params.dmic_num == 2 ?
> > >  			&constraints_dmic_2ch :
> > > &constraints_dmic_channels;
> > >
> > > -	ctx->mclk = devm_clk_get(&pdev->dev, "ssp1_mclk");
> > > -	if (IS_ERR(ctx->mclk)) {
> > > -		ret = PTR_ERR(ctx->mclk);
> > > -		if (ret == -ENOENT) {
> > > -			dev_info(&pdev->dev,
> > > -				"Failed to get ssp1_mclk, defer probe\n");
> > > -			return -EPROBE_DEFER;
> > > -		}
> > > -
> > > -		dev_err(&pdev->dev, "Failed to get ssp1_mclk with
> > > err:%d\n",
> > > -								ret);
> > > +	ret = kabylake_audio_clk_get(&pdev->dev, "ssp0_sclk", &ctx->sclk0);
> > > +	if (ret != 0)
> > >  		return ret;
> > > -	}
> > >
> > > -	ctx->sclk = devm_clk_get(&pdev->dev, "ssp1_sclk");
> > > -	if (IS_ERR(ctx->sclk)) {
> > > -		ret = PTR_ERR(ctx->sclk);
> > > -		if (ret == -ENOENT) {
> > > -			dev_info(&pdev->dev,
> > > -				"Failed to get ssp1_sclk, defer probe\n");
> > > -			return -EPROBE_DEFER;
> > > -		}
> > > +	ret = kabylake_audio_clk_get(&pdev->dev, "ssp1_mclk", &ctx-
> > > >mclk);
> > > +	if (ret != 0)
> > > +		return ret;
> > >
> > > -		dev_err(&pdev->dev, "Failed to get ssp1_sclk with err:%d\n",
> > > -								ret);
> > > +	ret = kabylake_audio_clk_get(&pdev->dev, "ssp1_sclk", &ctx->sclk1);
> > > +	if (ret != 0)
> > >  		return ret;
> > > -	}
> > >
> > >  	return devm_snd_soc_register_card(&pdev->dev,
> > > &kabylake_audio_card);  }
> > > --
> > > 2.7.4