diff mbox series

ASoC: Intel: eve: Enable mclk and ssp sclk early

Message ID 1569919250-24472-1-git-send-email-brent.lu@intel.com (mailing list archive)
State New, archived
Headers show
Series ASoC: Intel: eve: Enable mclk and ssp sclk early | expand

Commit Message

Brent Lu Oct. 1, 2019, 8:40 a.m. UTC 
From: Naveen M <naveen.m@intel.com>

rt5663 and rt5514 needs mclk/sclk early to synchronize its internal
clocks.

Signed-off-by: Naveen M <naveen.m@intel.com>
Signed-off-by: Harsha Priya <harshapriya.n@intel.com>
Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
Signed-off-by: Brent Lu <brent.lu@intel.com>
---
 sound/soc/intel/boards/Kconfig                     |   1 +
 .../soc/intel/boards/kbl_rt5663_rt5514_max98927.c  | 100 +++++++++++++++++++++
 2 files changed, 101 insertions(+)

Comments

Pierre-Louis Bossart Oct. 1, 2019, 1:35 p.m. UTC | #1 
> rt5663 and rt5514 needs mclk/sclk early to synchronize its internal
> clocks.

There are a couple of differences with similar code used in 
kbl_rt5663_max98927, see below


> diff --git a/sound/soc/intel/boards/Kconfig b/sound/soc/intel/boards/Kconfig
> index 5c27f7a..d5f167e 100644
> --- a/sound/soc/intel/boards/Kconfig
> +++ b/sound/soc/intel/boards/Kconfig
> @@ -315,6 +315,7 @@ config SND_SOC_INTEL_KBL_RT5663_RT5514_MAX98927_MACH
>   	depends on I2C && ACPI
>   	depends on MFD_INTEL_LPSS || COMPILE_TEST
>           depends on SPI
> +	select SND_SOC_INTEL_SKYLAKE_SSP_CLK

It would be nicer to follow the same order as for kbl_rt5663_max98927, 
with this SKYLAKE_SSP_CLK added last after HDAC_HDMI


> +static int platform_clock_control(struct snd_soc_dapm_widget *w,
> +			struct snd_kcontrol *k, int  event)
> +{
> +	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);
> +	int ret = 0;
> +
> +	/*
> +	 * MCLK/SCLK need to be ON early for a successful synchronization of
> +	 * codec internal clock. And the clocks are turned off during
> +	 * POST_PMD after the stream is stopped.
> +	 */
> +	switch (event) {
> +	case SND_SOC_DAPM_PRE_PMU:
> +		if (__clk_is_enabled(priv->mclk))
> +			return 0;

Is this if() test needed? it's not part of the code for 
kbl_rt5663_max98927, despite all the comments and code structure being 
identical.

> +
> +		/* 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;
> +		}
> +
> +		ret = clk_prepare_enable(priv->mclk);
> +		if (ret < 0) {
> +			dev_err(card->dev, "Can't enable mclk, err: %d\n", ret);
> +			return ret;
> +		}
> +
> +		/* Enable SCLK */
> +		ret = clk_set_rate(priv->sclk, 3072000);
> +		if (ret < 0) {
> +			dev_err(card->dev, "Can't set rate for sclk, err: %d\n",
> +				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);
> +		}
> +		break;
> +	case SND_SOC_DAPM_POST_PMD:
> +		if (!__clk_is_enabled(priv->mclk))
> +			return 0;

same here, is this if() test needed? If yes, isn't it needed in 
kbl_rt5663_max98927?

> +
> +		clk_disable_unprepare(priv->mclk);
> +		clk_disable_unprepare(priv->sclk);
> +		break;
> +	default:
> +		return 0;
> +	}
> +
> +	return 0;
> +}
> +

While I am at it, this machine driver uses .ignore_pmdown_time, which is 
typically used to avoid clock issues. Now that you have an explicit 
control on clocks, is .ignore_pmdown_time actually required?
Brent Lu Oct. 2, 2019, 2:53 a.m. UTC | #2 
> > diff --git a/sound/soc/intel/boards/Kconfig
> b/sound/soc/intel/boards/Kconfig
> > index 5c27f7a..d5f167e 100644
> > --- a/sound/soc/intel/boards/Kconfig
> > +++ b/sound/soc/intel/boards/Kconfig
> > @@ -315,6 +315,7 @@ config
> SND_SOC_INTEL_KBL_RT5663_RT5514_MAX98927_MACH
> >   	depends on I2C && ACPI
> >   	depends on MFD_INTEL_LPSS || COMPILE_TEST
> >           depends on SPI
> > +	select SND_SOC_INTEL_SKYLAKE_SSP_CLK
> 
> It would be nicer to follow the same order as for kbl_rt5663_max98927,
> with this SKYLAKE_SSP_CLK added last after HDAC_HDMI
> 
> 

Will fix it.

> > +static int platform_clock_control(struct snd_soc_dapm_widget *w,
> > +			struct snd_kcontrol *k, int  event)
> > +{
> > +	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);
> > +	int ret = 0;
> > +
> > +	/*
> > +	 * MCLK/SCLK need to be ON early for a successful synchronization of
> > +	 * codec internal clock. And the clocks are turned off during
> > +	 * POST_PMD after the stream is stopped.
> > +	 */
> > +	switch (event) {
> > +	case SND_SOC_DAPM_PRE_PMU:
> > +		if (__clk_is_enabled(priv->mclk))
> > +			return 0;
> 
> Is this if() test needed? it's not part of the code for
> kbl_rt5663_max98927, despite all the comments and code structure being
> identical.
> 

This function call prevents the clk_set_rate() from being called when clock is 
enabled. It's removed during the upstream review and use the flag 
CLK_SET_RATE_GATE instead. Will fix it.

https://patchwork.kernel.org/patch/10070383/
https://patchwork.kernel.org/patch/10133179/

> > +
> > +		/* 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;
> > +		}
> > +
> > +		ret = clk_prepare_enable(priv->mclk);
> > +		if (ret < 0) {
> > +			dev_err(card->dev, "Can't enable mclk, err: %d\n",
> ret);
> > +			return ret;
> > +		}
> > +
> > +		/* Enable SCLK */
> > +		ret = clk_set_rate(priv->sclk, 3072000);
> > +		if (ret < 0) {
> > +			dev_err(card->dev, "Can't set rate for sclk, err:
> %d\n",
> > +				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);
> > +		}
> > +		break;
> > +	case SND_SOC_DAPM_POST_PMD:
> > +		if (!__clk_is_enabled(priv->mclk))
> > +			return 0;
> 
> same here, is this if() test needed? If yes, isn't it needed in
> kbl_rt5663_max98927?
> 

Will fix it.

> > +
> > +		clk_disable_unprepare(priv->mclk);
> > +		clk_disable_unprepare(priv->sclk);
> > +		break;
> > +	default:
> > +		return 0;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> 
> While I am at it, this machine driver uses .ignore_pmdown_time, which is
> typically used to avoid clock issues. Now that you have an explicit
> control on clocks, is .ignore_pmdown_time actually required?

Actually I don't know the purpose to use ignore_pmdown_time flag in the first
place but I think they are not related since this patch is to turn on ssp clocks earlier
while the ignore_pmdown_time flag is used to delay the DAPM power down for
playback stream in the soc_pcm_close(). Just my two cents and thank you for the
review.


Regards,
Brent
diff mbox series

Patch

diff --git a/sound/soc/intel/boards/Kconfig b/sound/soc/intel/boards/Kconfig
index 5c27f7a..d5f167e 100644
--- a/sound/soc/intel/boards/Kconfig
+++ b/sound/soc/intel/boards/Kconfig
@@ -315,6 +315,7 @@  config SND_SOC_INTEL_KBL_RT5663_RT5514_MAX98927_MACH
 	depends on I2C && ACPI
 	depends on MFD_INTEL_LPSS || COMPILE_TEST
         depends on SPI
+	select SND_SOC_INTEL_SKYLAKE_SSP_CLK
         select SND_SOC_RT5663
         select SND_SOC_RT5514
         select SND_SOC_RT5514_SPI
diff --git a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
index 74dda87..7b6e70b 100644
--- a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
+++ b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
@@ -22,6 +22,9 @@ 
 #include "../../codecs/rt5514.h"
 #include "../../codecs/rt5663.h"
 #include "../../codecs/hdac_hdmi.h"
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/clkdev.h>
 
 #define KBL_REALTEK_CODEC_DAI "rt5663-aif"
 #define KBL_REALTEK_DMIC_CODEC_DAI "rt5514-aif1"
@@ -50,6 +53,8 @@  struct kbl_codec_private {
 	struct snd_soc_jack kabylake_headset;
 	struct list_head hdmi_pcm_list;
 	struct snd_soc_jack kabylake_hdmi[2];
+	struct clk *mclk;
+	struct clk *sclk;
 };
 
 enum {
@@ -71,6 +76,67 @@  static const struct snd_kcontrol_new kabylake_controls[] = {
 	SOC_DAPM_PIN_SWITCH("DMIC"),
 };
 
+static int platform_clock_control(struct snd_soc_dapm_widget *w,
+			struct snd_kcontrol *k, int  event)
+{
+	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);
+	int ret = 0;
+
+	/*
+	 * MCLK/SCLK need to be ON early for a successful synchronization of
+	 * codec internal clock. And the clocks are turned off during
+	 * POST_PMD after the stream is stopped.
+	 */
+	switch (event) {
+	case SND_SOC_DAPM_PRE_PMU:
+		if (__clk_is_enabled(priv->mclk))
+			return 0;
+
+		/* 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;
+		}
+
+		ret = clk_prepare_enable(priv->mclk);
+		if (ret < 0) {
+			dev_err(card->dev, "Can't enable mclk, err: %d\n", ret);
+			return ret;
+		}
+
+		/* Enable SCLK */
+		ret = clk_set_rate(priv->sclk, 3072000);
+		if (ret < 0) {
+			dev_err(card->dev, "Can't set rate for sclk, err: %d\n",
+				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);
+		}
+		break;
+	case SND_SOC_DAPM_POST_PMD:
+		if (!__clk_is_enabled(priv->mclk))
+			return 0;
+
+		clk_disable_unprepare(priv->mclk);
+		clk_disable_unprepare(priv->sclk);
+		break;
+	default:
+		return 0;
+	}
+
+	return 0;
+}
+
 static const struct snd_soc_dapm_widget kabylake_widgets[] = {
 	SND_SOC_DAPM_HP("Headphone Jack", NULL),
 	SND_SOC_DAPM_MIC("Headset Mic", NULL),
@@ -79,11 +145,15 @@  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_POST_PMD),
 
 };
 
 static const struct snd_soc_dapm_route kabylake_map[] = {
 	/* Headphones */
+	{ "Headphone Jack", NULL, "Platform Clock" },
 	{ "Headphone Jack", NULL, "HPOL" },
 	{ "Headphone Jack", NULL, "HPOR" },
 
@@ -92,6 +162,7 @@  static const struct snd_soc_dapm_route kabylake_map[] = {
 	{ "Right Spk", NULL, "Right BE_OUT" },
 
 	/* other jacks */
+	{ "Headset Mic", NULL, "Platform Clock" },
 	{ "IN1P", NULL, "Headset Mic" },
 	{ "IN1N", NULL, "Headset Mic" },
 
@@ -643,6 +714,7 @@  static int kabylake_audio_probe(struct platform_device *pdev)
 {
 	struct kbl_codec_private *ctx;
 	struct snd_soc_acpi_mach *mach;
+	int ret = 0;
 
 	ctx = devm_kzalloc(&pdev->dev, sizeof(*ctx), GFP_KERNEL);
 	if (!ctx)
@@ -658,6 +730,34 @@  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);
+		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;
+		}
+
+		dev_err(&pdev->dev, "Failed to get ssp1_sclk with err:%d\n",
+								ret);
+		return ret;
+	}
+
 	return devm_snd_soc_register_card(&pdev->dev, &kabylake_audio_card);
 }