[PATCH V2] ASoC: Intel: boards: Use FS as nau8825 sysclk in nau88125_* machine

[PATCH V2] ASoC: Intel: boards: Use FS as nau8825 sysclk in nau88125_* machine

[Radoslaw Biernacki]

This single fix address two issues on machines with nau88125:
1) Audio distortion, due to lack of required clock rate on MCLK line
2) Loud audible "pops" on headphones if there is no sysclk during nau8825
   playback power up sequence

Explanation for:
1) Due to Skylake HW limitation, MCLK pin can only output 24MHz clk
   rate (it can be only connected to XTAL parent clk). The BCLK pin
   can be driven by dividers and therefore FW is able to set it to rate
   required by chosen audio format. According to nau8825 datasheet, 256*FS
   sysclk gives the best audio quality and the only way to achieve this
   (taking into account the above limitations) its to regenerate the MCLK
   from BCLK on nau8825 side by FFL. Without required clk rate, audio is
   distorted by added harmonics.

2) Currently Skylake does not output MCLK/FS when the back-end DAI op
   hw_param is called, so we cannot switch to MCLK/FS in hw_param.  This
   patch reduces pop by letting nau8825 keep using its internal VCO clock
   during widget power up sequence, until SNDRV_PCM_TRIGGER_START when
   MCLK/FS is available. Once device resumes, the system will only enable
   power sequence for playback without doing hardware parameter, audio
   format, and PLL configure. In the mean time, the jack detecion sequence
   has changed PLL parameters and switched to internal clock. Thus, the
   playback signal distorted without correct PLL parameters.  That is why
   we need to configure the PLL again in SNDRV_PCM_TRIGGER_RESUME case.

Signed-off-by: John Hsu <KCHSU0@nuvoton.com>
Signed-off-by: Yong Zhi <yong.zhi@intel.com>
Signed-off-by: Mac Chiang <mac.chiang@intel.com>
Signed-off-by: Ben Zhang <benzh@chromium.org>
Signed-off-by: Radoslaw Biernacki <rad@semihalf.com>
---
 .../soc/intel/boards/skl_nau88l25_max98357a.c | 72 +++++++++++++-----
 sound/soc/intel/boards/skl_nau88l25_ssm4567.c | 73 ++++++++++++++-----
 2 files changed, 107 insertions(+), 38 deletions(-)

diff --git a/sound/soc/intel/boards/skl_nau88l25_max98357a.c b/sound/soc/intel/boards/skl_nau88l25_max98357a.c
index d7b8154c43a4..0f3cea1342d1 100644
--- a/sound/soc/intel/boards/skl_nau88l25_max98357a.c
+++ b/sound/soc/intel/boards/skl_nau88l25_max98357a.c
@@ -8,6 +8,7 @@
 
 #include <linux/module.h>
 #include <linux/platform_device.h>
+#include <linux/delay.h>
 #include <sound/core.h>
 #include <sound/jack.h>
 #include <sound/pcm.h>
@@ -47,7 +48,7 @@ enum {
 };
 
 static int platform_clock_control(struct snd_soc_dapm_widget *w,
-	struct snd_kcontrol *k, int  event)
+	struct snd_kcontrol *k, int event)
 {
 	struct snd_soc_dapm_context *dapm = w->dapm;
 	struct snd_soc_card *card = dapm->card;
@@ -60,14 +61,7 @@ static int platform_clock_control(struct snd_soc_dapm_widget *w,
 		return -EIO;
 	}
 
-	if (SND_SOC_DAPM_EVENT_ON(event)) {
-		ret = snd_soc_dai_set_sysclk(codec_dai,
-				NAU8825_CLK_MCLK, 24000000, SND_SOC_CLOCK_IN);
-		if (ret < 0) {
-			dev_err(card->dev, "set sysclk err = %d\n", ret);
-			return -EIO;
-		}
-	} else {
+	if (!SND_SOC_DAPM_EVENT_ON(event)) {
 		ret = snd_soc_dai_set_sysclk(codec_dai,
 				NAU8825_CLK_INTERNAL, 0, SND_SOC_CLOCK_IN);
 		if (ret < 0) {
@@ -292,24 +286,40 @@ static const struct snd_soc_ops skylake_nau8825_fe_ops = {
 	.startup = skl_fe_startup,
 };
 
-static int skylake_nau8825_hw_params(struct snd_pcm_substream *substream,
-	struct snd_pcm_hw_params *params)
+static int skylake_nau8825_trigger(struct snd_pcm_substream *substream, int cmd)
 {
 	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+	struct snd_pcm_runtime *runtime = substream->runtime;
 	struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
-	int ret;
-
-	ret = snd_soc_dai_set_sysclk(codec_dai,
-			NAU8825_CLK_MCLK, 24000000, SND_SOC_CLOCK_IN);
+	int ret = 0;
 
-	if (ret < 0)
-		dev_err(rtd->dev, "snd_soc_dai_set_sysclk err = %d\n", ret);
+	switch (cmd) {
+	case SNDRV_PCM_TRIGGER_START:
+		ret = snd_soc_dai_set_sysclk(codec_dai, NAU8825_CLK_FLL_FS, 0,
+					     SND_SOC_CLOCK_IN);
+		if (ret < 0) {
+			dev_err(codec_dai->dev, "can't set FS clock %d\n", ret);
+			break;
+		}
+		ret = snd_soc_dai_set_pll(codec_dai, 0, 0, runtime->rate,
+					  runtime->rate * 256);
+		if (ret < 0)
+			dev_err(codec_dai->dev, "can't set FLL: %d\n", ret);
+		break;
+	case SNDRV_PCM_TRIGGER_RESUME:
+		ret = snd_soc_dai_set_pll(codec_dai, 0, 0, runtime->rate,
+					  runtime->rate * 256);
+		if (ret < 0)
+			dev_err(codec_dai->dev, "can't set FLL: %d\n", ret);
+		msleep(20);
+		break;
+	}
 
 	return ret;
 }
 
-static const struct snd_soc_ops skylake_nau8825_ops = {
-	.hw_params = skylake_nau8825_hw_params,
+static struct snd_soc_ops skylake_nau8825_ops = {
+	.trigger = skylake_nau8825_trigger,
 };
 
 static int skylake_dmic_fixup(struct snd_soc_pcm_runtime *rtd,
@@ -630,10 +640,34 @@ static int skylake_card_late_probe(struct snd_soc_card *card)
 	return hdac_hdmi_jack_port_init(component, &card->dapm);
 }
 
+static int __maybe_unused skylake_nau8825_resume_post(struct snd_soc_card *card)
+{
+	struct snd_soc_dai *codec_dai;
+
+	codec_dai = snd_soc_card_get_codec_dai(card, SKL_NUVOTON_CODEC_DAI);
+	if (!codec_dai) {
+		dev_err(card->dev, "Codec dai not found\n");
+		return -EIO;
+	}
+
+	dev_dbg(codec_dai->dev, "playback_active:%d playback_widget->active:%d codec_dai->rate:%d\n",
+		codec_dai->stream_active[SNDRV_PCM_STREAM_PLAYBACK],
+		codec_dai->playback_widget->active,
+		codec_dai->rate);
+
+	if (codec_dai->stream_active[SNDRV_PCM_STREAM_PLAYBACK] &&
+	    codec_dai->playback_widget->active)
+		snd_soc_dai_set_sysclk(codec_dai, NAU8825_CLK_FLL_FS, 0,
+				       SND_SOC_CLOCK_IN);
+
+	return 0;
+}
+
 /* skylake audio machine driver for SPT + NAU88L25 */
 static struct snd_soc_card skylake_audio_card = {
 	.name = "sklnau8825max",
 	.owner = THIS_MODULE,
+	.resume_post = skylake_nau8825_resume_post,
 	.dai_link = skylake_dais,
 	.num_links = ARRAY_SIZE(skylake_dais),
 	.controls = skylake_controls,
diff --git a/sound/soc/intel/boards/skl_nau88l25_ssm4567.c b/sound/soc/intel/boards/skl_nau88l25_ssm4567.c
index 4b317bcf6ea0..be44a40067f0 100644
--- a/sound/soc/intel/boards/skl_nau88l25_ssm4567.c
+++ b/sound/soc/intel/boards/skl_nau88l25_ssm4567.c
@@ -12,6 +12,7 @@
 
 #include <linux/module.h>
 #include <linux/platform_device.h>
+#include <linux/delay.h>
 #include <sound/core.h>
 #include <sound/pcm.h>
 #include <sound/soc.h>
@@ -57,7 +58,7 @@ static const struct snd_kcontrol_new skylake_controls[] = {
 };
 
 static int platform_clock_control(struct snd_soc_dapm_widget *w,
-		struct snd_kcontrol *k, int  event)
+		struct snd_kcontrol *k, int event)
 {
 	struct snd_soc_dapm_context *dapm = w->dapm;
 	struct snd_soc_card *card = dapm->card;
@@ -70,14 +71,7 @@ static int platform_clock_control(struct snd_soc_dapm_widget *w,
 		return -EIO;
 	}
 
-	if (SND_SOC_DAPM_EVENT_ON(event)) {
-		ret = snd_soc_dai_set_sysclk(codec_dai,
-				NAU8825_CLK_MCLK, 24000000, SND_SOC_CLOCK_IN);
-		if (ret < 0) {
-			dev_err(card->dev, "set sysclk err = %d\n", ret);
-			return -EIO;
-		}
-	} else {
+	if (!SND_SOC_DAPM_EVENT_ON(event)) {
 		ret = snd_soc_dai_set_sysclk(codec_dai,
 				NAU8825_CLK_INTERNAL, 0, SND_SOC_CLOCK_IN);
 		if (ret < 0) {
@@ -85,6 +79,7 @@ static int platform_clock_control(struct snd_soc_dapm_widget *w,
 			return -EIO;
 		}
 	}
+
 	return ret;
 }
 
@@ -344,24 +339,40 @@ static int skylake_dmic_fixup(struct snd_soc_pcm_runtime *rtd,
 	return 0;
 }
 
-static int skylake_nau8825_hw_params(struct snd_pcm_substream *substream,
-	struct snd_pcm_hw_params *params)
+static int skylake_nau8825_trigger(struct snd_pcm_substream *substream, int cmd)
 {
 	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+	struct snd_pcm_runtime *runtime = substream->runtime;
 	struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
-	int ret;
-
-	ret = snd_soc_dai_set_sysclk(codec_dai,
-			NAU8825_CLK_MCLK, 24000000, SND_SOC_CLOCK_IN);
+	int ret = 0;
 
-	if (ret < 0)
-		dev_err(rtd->dev, "snd_soc_dai_set_sysclk err = %d\n", ret);
+	switch (cmd) {
+	case SNDRV_PCM_TRIGGER_START:
+		ret = snd_soc_dai_set_sysclk(codec_dai, NAU8825_CLK_FLL_FS, 0,
+					     SND_SOC_CLOCK_IN);
+		if (ret < 0) {
+			dev_err(codec_dai->dev, "can't set FS clock %d\n", ret);
+			break;
+		}
+		ret = snd_soc_dai_set_pll(codec_dai, 0, 0, runtime->rate,
+					  runtime->rate * 256);
+		if (ret < 0)
+			dev_err(codec_dai->dev, "can't set FLL: %d\n", ret);
+		break;
+	case SNDRV_PCM_TRIGGER_RESUME:
+		ret = snd_soc_dai_set_pll(codec_dai, 0, 0, runtime->rate,
+					  runtime->rate * 256);
+		if (ret < 0)
+			dev_err(codec_dai->dev, "can't set FLL: %d\n", ret);
+		msleep(20);
+		break;
+	}
 
 	return ret;
 }
 
-static const struct snd_soc_ops skylake_nau8825_ops = {
-	.hw_params = skylake_nau8825_hw_params,
+static struct snd_soc_ops skylake_nau8825_ops = {
+	.trigger = skylake_nau8825_trigger,
 };
 
 static const unsigned int channels_dmic[] = {
@@ -671,10 +682,34 @@ static int skylake_card_late_probe(struct snd_soc_card *card)
 	return hdac_hdmi_jack_port_init(component, &card->dapm);
 }
 
+static int __maybe_unused skylake_nau8825_resume_post(struct snd_soc_card *card)
+{
+	struct snd_soc_dai *codec_dai;
+
+	codec_dai = snd_soc_card_get_codec_dai(card, SKL_NUVOTON_CODEC_DAI);
+	if (!codec_dai) {
+		dev_err(card->dev, "Codec dai not found\n");
+		return -EIO;
+	}
+
+	dev_dbg(codec_dai->dev, "playback_active:%d playback_widget->active:%d codec_dai->rate:%d\n",
+		codec_dai->stream_active[SNDRV_PCM_STREAM_PLAYBACK],
+		codec_dai->playback_widget->active,
+		codec_dai->rate);
+
+	if (codec_dai->stream_active[SNDRV_PCM_STREAM_PLAYBACK] &&
+	    codec_dai->playback_widget->active)
+		snd_soc_dai_set_sysclk(codec_dai, NAU8825_CLK_FLL_FS, 0,
+				       SND_SOC_CLOCK_IN);
+
+	return 0;
+}
+
 /* skylake audio machine driver for SPT + NAU88L25 */
 static struct snd_soc_card skylake_audio_card = {
 	.name = "sklnau8825adi",
 	.owner = THIS_MODULE,
+	.resume_post = skylake_nau8825_resume_post,
 	.dai_link = skylake_dais,
 	.num_links = ARRAY_SIZE(skylake_dais),
 	.controls = skylake_controls,
-- 
2.17.1

Re: [PATCH V2] ASoC: Intel: boards: Use FS as nau8825 sysclk in nau88125_* machine

[Pierre-Louis Bossart]

On 5/1/20 2:31 PM, Radoslaw Biernacki wrote:
> This single fix address two issues on machines with nau88125:
> 1) Audio distortion, due to lack of required clock rate on MCLK line
> 2) Loud audible "pops" on headphones if there is no sysclk during nau8825
>     playback power up sequence
> 
> Explanation for:
> 1) Due to Skylake HW limitation, MCLK pin can only output 24MHz clk
>     rate (it can be only connected to XTAL parent clk). The BCLK pin
>     can be driven by dividers and therefore FW is able to set it to rate
>     required by chosen audio format. According to nau8825 datasheet, 256*FS
>     sysclk gives the best audio quality and the only way to achieve this
>     (taking into account the above limitations) its to regenerate the MCLK
>     from BCLK on nau8825 side by FFL. Without required clk rate, audio is
>     distorted by added harmonics.

The BCLK is going to be a multiple of 50 * Fs due to clocking 
restrictions. Can the codec regenerate a good-enough sysclk from this?
> 
> 2) Currently Skylake does not output MCLK/FS when the back-end DAI op
>     hw_param is called, so we cannot switch to MCLK/FS in hw_param.  This
>     patch reduces pop by letting nau8825 keep using its internal VCO clock
>     during widget power up sequence, until SNDRV_PCM_TRIGGER_START when
>     MCLK/FS is available. Once device resumes, the system will only enable
>     power sequence for playback without doing hardware parameter, audio
>     format, and PLL configure. In the mean time, the jack detecion sequence
>     has changed PLL parameters and switched to internal clock. Thus, the
>     playback signal distorted without correct PLL parameters.  That is why
>     we need to configure the PLL again in SNDRV_PCM_TRIGGER_RESUME case.

IIRC the FS can be controlled with the clk_ api with the Skylake driver, 
as done for some KBL platforms. Or is this not supported by the firmware 
used by this machine?

> -static int skylake_nau8825_hw_params(struct snd_pcm_substream *substream,
> -	struct snd_pcm_hw_params *params)
> +static int skylake_nau8825_trigger(struct snd_pcm_substream *substream, int cmd)
>   {
>   	struct snd_soc_pcm_runtime *rtd = substream->private_data;
> +	struct snd_pcm_runtime *runtime = substream->runtime;
>   	struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
> -	int ret;
> -
> -	ret = snd_soc_dai_set_sysclk(codec_dai,
> -			NAU8825_CLK_MCLK, 24000000, SND_SOC_CLOCK_IN);
> +	int ret = 0;
>   
> -	if (ret < 0)
> -		dev_err(rtd->dev, "snd_soc_dai_set_sysclk err = %d\n", ret);
> +	switch (cmd) {
> +	case SNDRV_PCM_TRIGGER_START:
> +		ret = snd_soc_dai_set_sysclk(codec_dai, NAU8825_CLK_FLL_FS, 0,
> +					     SND_SOC_CLOCK_IN);
> +		if (ret < 0) {
> +			dev_err(codec_dai->dev, "can't set FS clock %d\n", ret);
> +			break;
> +		}
> +		ret = snd_soc_dai_set_pll(codec_dai, 0, 0, runtime->rate,
> +					  runtime->rate * 256);
> +		if (ret < 0)
> +			dev_err(codec_dai->dev, "can't set FLL: %d\n", ret);
> +		break;
> +	case SNDRV_PCM_TRIGGER_RESUME:
> +		ret = snd_soc_dai_set_pll(codec_dai, 0, 0, runtime->rate,
> +					  runtime->rate * 256);
> +		if (ret < 0)
> +			dev_err(codec_dai->dev, "can't set FLL: %d\n", ret);
> +		msleep(20);

is there a reason why you'd need a msleep for resume and not for start?

Re: [PATCH V2] ASoC: Intel: boards: Use FS as nau8825 sysclk in nau88125_* machine

[kbuild test robot]

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

Hi Radoslaw,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on asoc/for-next]
[also build test WARNING on sound/for-next v5.7-rc3 next-20200501]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Radoslaw-Biernacki/ASoC-Intel-boards-Use-FS-as-nau8825-sysclk-in-nau88125_-machine/20200502-044538
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
config: i386-allyesconfig (attached as .config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

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

Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings

All warnings (new ones prefixed by >>):

   sound/soc/intel/boards/skl_nau88l25_max98357a.c: In function 'platform_clock_control':
>> sound/soc/intel/boards/skl_nau88l25_max98357a.c:56:6: warning: 'ret' may be used uninitialized in this function [-Wmaybe-uninitialized]
     int ret;
         ^~~

vim +/ret +56 sound/soc/intel/boards/skl_nau88l25_max98357a.c

0ab338ff33762d Sathyanarayana Nujella 2016-02-24  49  
8eaf2b31dd316f Rohit Ainapure         2015-12-11  50  static int platform_clock_control(struct snd_soc_dapm_widget *w,
8eaf2b31dd316f Rohit Ainapure         2015-12-11  51  	struct snd_kcontrol *k, int event)
8eaf2b31dd316f Rohit Ainapure         2015-12-11  52  {
8eaf2b31dd316f Rohit Ainapure         2015-12-11  53  	struct snd_soc_dapm_context *dapm = w->dapm;
8eaf2b31dd316f Rohit Ainapure         2015-12-11  54  	struct snd_soc_card *card = dapm->card;
8eaf2b31dd316f Rohit Ainapure         2015-12-11  55  	struct snd_soc_dai *codec_dai;
8eaf2b31dd316f Rohit Ainapure         2015-12-11 @56  	int ret;
8eaf2b31dd316f Rohit Ainapure         2015-12-11  57  
dfb6ec7ae57d33 Pierre-Louis Bossart   2017-10-12  58  	codec_dai = snd_soc_card_get_codec_dai(card, SKL_NUVOTON_CODEC_DAI);
8eaf2b31dd316f Rohit Ainapure         2015-12-11  59  	if (!codec_dai) {
8eaf2b31dd316f Rohit Ainapure         2015-12-11  60  		dev_err(card->dev, "Codec dai not found; Unable to set platform clock\n");
8eaf2b31dd316f Rohit Ainapure         2015-12-11  61  		return -EIO;
8eaf2b31dd316f Rohit Ainapure         2015-12-11  62  	}
8eaf2b31dd316f Rohit Ainapure         2015-12-11  63  
4adaffea05f2fa Radoslaw Biernacki     2020-05-01  64  	if (!SND_SOC_DAPM_EVENT_ON(event)) {
8eaf2b31dd316f Rohit Ainapure         2015-12-11  65  		ret = snd_soc_dai_set_sysclk(codec_dai,
8eaf2b31dd316f Rohit Ainapure         2015-12-11  66  				NAU8825_CLK_INTERNAL, 0, SND_SOC_CLOCK_IN);
8eaf2b31dd316f Rohit Ainapure         2015-12-11  67  		if (ret < 0) {
8eaf2b31dd316f Rohit Ainapure         2015-12-11  68  			dev_err(card->dev, "set sysclk err = %d\n", ret);
8eaf2b31dd316f Rohit Ainapure         2015-12-11  69  			return -EIO;
8eaf2b31dd316f Rohit Ainapure         2015-12-11  70  		}
8eaf2b31dd316f Rohit Ainapure         2015-12-11  71  	}
8eaf2b31dd316f Rohit Ainapure         2015-12-11  72  
8eaf2b31dd316f Rohit Ainapure         2015-12-11  73  	return ret;
8eaf2b31dd316f Rohit Ainapure         2015-12-11  74  }
8eaf2b31dd316f Rohit Ainapure         2015-12-11  75  

:::::: The code at line 56 was first introduced by commit
:::::: 8eaf2b31dd316ff5ffbdad14853d2bf8779bab13 ASoC: Intel: Skylake: Add Nuvoton Maxim machine driver

:::::: TO: Rohit Ainapure <rohit.m.ainapure@intel.com>
:::::: CC: Mark Brown <broonie@kernel.org>

---
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: 72296 bytes --]

Re: [PATCH V2] ASoC: Intel: boards: Use FS as nau8825 sysclk in nau88125_* machine

[Radosław Biernacki]

Thank you Pierre for the review!
answers inline

pt., 1 maj 2020 o 22:16 Pierre-Louis Bossart
<pierre-louis.bossart@linux.intel.com> wrote:
>
>
>
> On 5/1/20 2:31 PM, Radoslaw Biernacki wrote:
> > This single fix address two issues on machines with nau88125:
> > 1) Audio distortion, due to lack of required clock rate on MCLK line
> > 2) Loud audible "pops" on headphones if there is no sysclk during nau8825
> >     playback power up sequence
> >
> > Explanation for:
> > 1) Due to Skylake HW limitation, MCLK pin can only output 24MHz clk
> >     rate (it can be only connected to XTAL parent clk). The BCLK pin
> >     can be driven by dividers and therefore FW is able to set it to rate
> >     required by chosen audio format. According to nau8825 datasheet, 256*FS
> >     sysclk gives the best audio quality and the only way to achieve this
> >     (taking into account the above limitations) its to regenerate the MCLK
> >     from BCLK on nau8825 side by FFL. Without required clk rate, audio is
> >     distorted by added harmonics.
>
> The BCLK is going to be a multiple of 50 * Fs due to clocking
> restrictions. Can the codec regenerate a good-enough sysclk from this?

According to Intel, silicon has a limitation, on SKL/KBL only clk_id =
SKL_XTAL, .name = "xtal" is available for IO domain.
As mentioned in the commit:
MCLK is generated by using 24MHz Xtal directly or applying a divider
(so no way of achieving the rate required by audio format).
BCLK/FS is generated from 24MHz and uses dividers and additional
padding bits are used to match the clock source.
Next gen silicon has the possibility of using additional clock sources.

Summing up, using MCLK from SKL to NAU88L25 is not an option.
The only option we found is to use BCLK and regen the required clock
rate by FLL on the NAU88l25 side.

> >
> > 2) Currently Skylake does not output MCLK/FS when the back-end DAI op
> >     hw_param is called, so we cannot switch to MCLK/FS in hw_param.  This
> >     patch reduces pop by letting nau8825 keep using its internal VCO clock
> >     during widget power up sequence, until SNDRV_PCM_TRIGGER_START when
> >     MCLK/FS is available. Once device resumes, the system will only enable
> >     power sequence for playback without doing hardware parameter, audio
> >     format, and PLL configure. In the mean time, the jack detecion sequence
> >     has changed PLL parameters and switched to internal clock. Thus, the
> >     playback signal distorted without correct PLL parameters.  That is why
> >     we need to configure the PLL again in SNDRV_PCM_TRIGGER_RESUME case.
>
> IIRC the FS can be controlled with the clk_ api with the Skylake driver,
> as done for some KBL platforms. Or is this not supported by the firmware
> used by this machine?

According to Ben, SKL had limitations in FW for managing the clk's
back in the days.
Can you point to the other driver you mention so we can cross check?

>
> > -static int skylake_nau8825_hw_params(struct snd_pcm_substream *substream,
> > -     struct snd_pcm_hw_params *params)
> > +static int skylake_nau8825_trigger(struct snd_pcm_substream *substream, int cmd)
> >   {
> >       struct snd_soc_pcm_runtime *rtd = substream->private_data;
> > +     struct snd_pcm_runtime *runtime = substream->runtime;
> >       struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
> > -     int ret;
> > -
> > -     ret = snd_soc_dai_set_sysclk(codec_dai,
> > -                     NAU8825_CLK_MCLK, 24000000, SND_SOC_CLOCK_IN);
> > +     int ret = 0;
> >
> > -     if (ret < 0)
> > -             dev_err(rtd->dev, "snd_soc_dai_set_sysclk err = %d\n", ret);
> > +     switch (cmd) {
> > +     case SNDRV_PCM_TRIGGER_START:
> > +             ret = snd_soc_dai_set_sysclk(codec_dai, NAU8825_CLK_FLL_FS, 0,
> > +                                          SND_SOC_CLOCK_IN);
> > +             if (ret < 0) {
> > +                     dev_err(codec_dai->dev, "can't set FS clock %d\n", ret);
> > +                     break;
> > +             }
> > +             ret = snd_soc_dai_set_pll(codec_dai, 0, 0, runtime->rate,
> > +                                       runtime->rate * 256);
> > +             if (ret < 0)
> > +                     dev_err(codec_dai->dev, "can't set FLL: %d\n", ret);
> > +             break;
> > +     case SNDRV_PCM_TRIGGER_RESUME:
> > +             ret = snd_soc_dai_set_pll(codec_dai, 0, 0, runtime->rate,
> > +                                       runtime->rate * 256);
> > +             if (ret < 0)
> > +                     dev_err(codec_dai->dev, "can't set FLL: %d\n", ret);
> > +             msleep(20);
>
> is there a reason why you'd need a msleep for resume and not for start?

No. I think we missed this.
msleep() is needed to stabilize FLL.
Will fix in next rev.

Re: [PATCH V2] ASoC: Intel: boards: Use FS as nau8825 sysclk in nau88125_* machine

[Pierre-Louis Bossart]

>>> This single fix address two issues on machines with nau88125:
>>> 1) Audio distortion, due to lack of required clock rate on MCLK line
>>> 2) Loud audible "pops" on headphones if there is no sysclk during nau8825
>>>      playback power up sequence
>>>
>>> Explanation for:
>>> 1) Due to Skylake HW limitation, MCLK pin can only output 24MHz clk
>>>      rate (it can be only connected to XTAL parent clk). The BCLK pin
>>>      can be driven by dividers and therefore FW is able to set it to rate
>>>      required by chosen audio format. According to nau8825 datasheet, 256*FS
>>>      sysclk gives the best audio quality and the only way to achieve this
>>>      (taking into account the above limitations) its to regenerate the MCLK
>>>      from BCLK on nau8825 side by FFL. Without required clk rate, audio is
>>>      distorted by added harmonics.
>>
>> The BCLK is going to be a multiple of 50 * Fs due to clocking
>> restrictions. Can the codec regenerate a good-enough sysclk from this?
> 
> According to Intel, silicon has a limitation, on SKL/KBL only clk_id =
> SKL_XTAL, .name = "xtal" is available for IO domain.
> As mentioned in the commit:
> MCLK is generated by using 24MHz Xtal directly or applying a divider
> (so no way of achieving the rate required by audio format).
> BCLK/FS is generated from 24MHz and uses dividers and additional
> padding bits are used to match the clock source.
> Next gen silicon has the possibility of using additional clock sources.
> 
> Summing up, using MCLK from SKL to NAU88L25 is not an option.
> The only option we found is to use BCLK and regen the required clock
> rate by FLL on the NAU88l25 side.

Right, this 24 MHz is a recurring problem.
But what I was asking was if the NAU8825 is fine working with e.g. a 
2.4MHz bit clock. i.e. with 25 bit slots or padding at the end of the frame?

> 
>>>
>>> 2) Currently Skylake does not output MCLK/FS when the back-end DAI op
>>>      hw_param is called, so we cannot switch to MCLK/FS in hw_param.  This
>>>      patch reduces pop by letting nau8825 keep using its internal VCO clock
>>>      during widget power up sequence, until SNDRV_PCM_TRIGGER_START when
>>>      MCLK/FS is available. Once device resumes, the system will only enable
>>>      power sequence for playback without doing hardware parameter, audio
>>>      format, and PLL configure. In the mean time, the jack detecion sequence
>>>      has changed PLL parameters and switched to internal clock. Thus, the
>>>      playback signal distorted without correct PLL parameters.  That is why
>>>      we need to configure the PLL again in SNDRV_PCM_TRIGGER_RESUME case.
>>
>> IIRC the FS can be controlled with the clk_ api with the Skylake driver,
>> as done for some KBL platforms. Or is this not supported by the firmware
>> used by this machine?
> 
> According to Ben, SKL had limitations in FW for managing the clk's
> back in the days.
> Can you point to the other driver you mention so we can cross check?

There are two KBL drivers that control the SSP clocks from the machine 
driver, but indeed I don't know if this would work on Firmware, it'd be 
a question for Lech/Cezary.

kbl_rt5663_max98927.c:          ret = clk_prepare_enable(priv->mclk);
kbl_rt5663_max98927.c:          ret = clk_prepare_enable(priv->sclk);
kbl_rt5663_rt5514_max98927.c:           ret = 
clk_prepare_enable(priv->mclk);
kbl_rt5663_rt5514_max98927.c:           ret = 
clk_prepare_enable(priv->sclk);
kbl_rt5663_rt5514_max98927.c:                   ret = 
clk_prepare_enable(priv->mclk);

Re: [PATCH V2] ASoC: Intel: boards: Use FS as nau8825 sysclk in nau88125_* machine

[Cezary Rojewski]

>>>
>>> 2) Currently Skylake does not output MCLK/FS when the back-end DAI op
>>>      hw_param is called, so we cannot switch to MCLK/FS in hw_param.  This
>>>      patch reduces pop by letting nau8825 keep using its internal VCO clock
>>>      during widget power up sequence, until SNDRV_PCM_TRIGGER_START when
>>>      MCLK/FS is available. Once device resumes, the system will only enable
>>>      power sequence for playback without doing hardware parameter, audio
>>>      format, and PLL configure. In the mean time, the jack detecion sequence
>>>      has changed PLL parameters and switched to internal clock. Thus, the
>>>      playback signal distorted without correct PLL parameters.  That is why
>>>      we need to configure the PLL again in SNDRV_PCM_TRIGGER_RESUME case.
>>
>> IIRC the FS can be controlled with the clk_ api with the Skylake driver,
>> as done for some KBL platforms. Or is this not supported by the firmware
>> used by this machine?
> 
> According to Ben, SKL had limitations in FW for managing the clk's
> back in the days.
> Can you point to the other driver you mention so we can cross check?
> 

Skylake driver is found within:
	/sound/soc/intel/skylake
directory.

"SKL had limitations in FW" - that's misleading. This is neither FW 
issue nor HW 'limitation'. SKL is an older platform and its goals and 
design was different than say APL+. Basically, your needs do not align 
with what's present on SKL hw.

Czarek

Re: [PATCH V2] ASoC: Intel: boards: Use FS as nau8825 sysclk in nau88125_* machine

[Radosław Biernacki]

Sorry for missing the response for so long.
Somehow lost this thread in my mailbox.

śr., 6 maj 2020 o 00:04 Pierre-Louis Bossart
<pierre-louis.bossart@linux.intel.com> napisał(a):
>
>
> >>> This single fix address two issues on machines with nau88125:
> >>> 1) Audio distortion, due to lack of required clock rate on MCLK line
> >>> 2) Loud audible "pops" on headphones if there is no sysclk during nau8825
> >>>      playback power up sequence
> >>>
> >>> Explanation for:
> >>> 1) Due to Skylake HW limitation, MCLK pin can only output 24MHz clk
> >>>      rate (it can be only connected to XTAL parent clk). The BCLK pin
> >>>      can be driven by dividers and therefore FW is able to set it to rate
> >>>      required by chosen audio format. According to nau8825 datasheet, 256*FS
> >>>      sysclk gives the best audio quality and the only way to achieve this
> >>>      (taking into account the above limitations) its to regenerate the MCLK
> >>>      from BCLK on nau8825 side by FFL. Without required clk rate, audio is
> >>>      distorted by added harmonics.
> >>
> >> The BCLK is going to be a multiple of 50 * Fs due to clocking
> >> restrictions. Can the codec regenerate a good-enough sysclk from this?
> >
> > According to Intel, silicon has a limitation, on SKL/KBL only clk_id =
> > SKL_XTAL, .name = "xtal" is available for IO domain.
> > As mentioned in the commit:
> > MCLK is generated by using 24MHz Xtal directly or applying a divider
> > (so no way of achieving the rate required by audio format).
> > BCLK/FS is generated from 24MHz and uses dividers and additional
> > padding bits are used to match the clock source.
> > Next gen silicon has the possibility of using additional clock sources.
> >
> > Summing up, using MCLK from SKL to NAU88L25 is not an option.
> > The only option we found is to use BCLK and regen the required clock
> > rate by FLL on the NAU88l25 side.
>
> Right, this 24 MHz is a recurring problem.
> But what I was asking was if the NAU8825 is fine working with e.g. a
> 2.4MHz bit clock. i.e. with 25 bit slots or padding at the end of the frame?

From our tests NAU8825 is working fine with these parameters.
Also the output audio signal looks fine on the scope and FFT and there
are no audible glitches.

>
> >
> >>>
> >>> 2) Currently Skylake does not output MCLK/FS when the back-end DAI op
> >>>      hw_param is called, so we cannot switch to MCLK/FS in hw_param.  This
> >>>      patch reduces pop by letting nau8825 keep using its internal VCO clock
> >>>      during widget power up sequence, until SNDRV_PCM_TRIGGER_START when
> >>>      MCLK/FS is available. Once device resumes, the system will only enable
> >>>      power sequence for playback without doing hardware parameter, audio
> >>>      format, and PLL configure. In the mean time, the jack detecion sequence
> >>>      has changed PLL parameters and switched to internal clock. Thus, the
> >>>      playback signal distorted without correct PLL parameters.  That is why
> >>>      we need to configure the PLL again in SNDRV_PCM_TRIGGER_RESUME case.
> >>
> >> IIRC the FS can be controlled with the clk_ api with the Skylake driver,
> >> as done for some KBL platforms. Or is this not supported by the firmware
> >> used by this machine?
> >
> > According to Ben, SKL had limitations in FW for managing the clk's
> > back in the days.
> > Can you point to the other driver you mention so we can cross check?
>
> There are two KBL drivers that control the SSP clocks from the machine
> driver, but indeed I don't know if this would work on Firmware, it'd be
> a question for Lech/Cezary.
>
> kbl_rt5663_max98927.c:          ret = clk_prepare_enable(priv->mclk);
> kbl_rt5663_max98927.c:          ret = clk_prepare_enable(priv->sclk);
> kbl_rt5663_rt5514_max98927.c:           ret =
> clk_prepare_enable(priv->mclk);
> kbl_rt5663_rt5514_max98927.c:           ret =
> clk_prepare_enable(priv->sclk);
> kbl_rt5663_rt5514_max98927.c:                   ret =
> clk_prepare_enable(priv->mclk);
>

Czarek answered this and we got the same response from other Intel
devs while consulting this change:
FW cannot request a chosen rate (48k) for MCLK pin as it does not
"align with what's present on SKL hw".

The only way we found out for NAU8825 to cooperate at chosen rate with
SKL HW is to regen the MCLK from BCLK by FLL as mentioned above.
NHTL is used to set SSP0 (48k, 24/25 bit on 24MHz crystal).
If I get all of this right, use of NHTL and HW "abilities" would
explain why there is no call to change SSP from a machine driver.


If all of this is ok I will send V3 with msleep() removed.

Re: [PATCH V2] ASoC: Intel: boards: Use FS as nau8825 sysclk in nau88125_* machine

[Pierre-Louis Bossart]

On 9/8/20 12:42 PM, Radosław Biernacki wrote:
> Sorry for missing the response for so long.
> Somehow lost this thread in my mailbox.

That happens to all of us!

> śr., 6 maj 2020 o 00:04 Pierre-Louis Bossart
> <pierre-louis.bossart@linux.intel.com> napisał(a):
>>
>>
>>>>> This single fix address two issues on machines with nau88125:
>>>>> 1) Audio distortion, due to lack of required clock rate on MCLK line
>>>>> 2) Loud audible "pops" on headphones if there is no sysclk during nau8825
>>>>>       playback power up sequence
>>>>>
>>>>> Explanation for:
>>>>> 1) Due to Skylake HW limitation, MCLK pin can only output 24MHz clk
>>>>>       rate (it can be only connected to XTAL parent clk). The BCLK pin
>>>>>       can be driven by dividers and therefore FW is able to set it to rate
>>>>>       required by chosen audio format. According to nau8825 datasheet, 256*FS
>>>>>       sysclk gives the best audio quality and the only way to achieve this
>>>>>       (taking into account the above limitations) its to regenerate the MCLK
>>>>>       from BCLK on nau8825 side by FFL. Without required clk rate, audio is
>>>>>       distorted by added harmonics.
>>>>
>>>> The BCLK is going to be a multiple of 50 * Fs due to clocking
>>>> restrictions. Can the codec regenerate a good-enough sysclk from this?
>>>
>>> According to Intel, silicon has a limitation, on SKL/KBL only clk_id =
>>> SKL_XTAL, .name = "xtal" is available for IO domain.
>>> As mentioned in the commit:
>>> MCLK is generated by using 24MHz Xtal directly or applying a divider
>>> (so no way of achieving the rate required by audio format).
>>> BCLK/FS is generated from 24MHz and uses dividers and additional
>>> padding bits are used to match the clock source.
>>> Next gen silicon has the possibility of using additional clock sources.
>>>
>>> Summing up, using MCLK from SKL to NAU88L25 is not an option.
>>> The only option we found is to use BCLK and regen the required clock
>>> rate by FLL on the NAU88l25 side.
>>
>> Right, this 24 MHz is a recurring problem.
>> But what I was asking was if the NAU8825 is fine working with e.g. a
>> 2.4MHz bit clock. i.e. with 25 bit slots or padding at the end of the frame?
> 
>  From our tests NAU8825 is working fine with these parameters.
> Also the output audio signal looks fine on the scope and FFT and there
> are no audible glitches.
> 
>>
>>>
>>>>>
>>>>> 2) Currently Skylake does not output MCLK/FS when the back-end DAI op
>>>>>       hw_param is called, so we cannot switch to MCLK/FS in hw_param.  This
>>>>>       patch reduces pop by letting nau8825 keep using its internal VCO clock
>>>>>       during widget power up sequence, until SNDRV_PCM_TRIGGER_START when
>>>>>       MCLK/FS is available. Once device resumes, the system will only enable
>>>>>       power sequence for playback without doing hardware parameter, audio
>>>>>       format, and PLL configure. In the mean time, the jack detecion sequence
>>>>>       has changed PLL parameters and switched to internal clock. Thus, the
>>>>>       playback signal distorted without correct PLL parameters.  That is why
>>>>>       we need to configure the PLL again in SNDRV_PCM_TRIGGER_RESUME case.
>>>>
>>>> IIRC the FS can be controlled with the clk_ api with the Skylake driver,
>>>> as done for some KBL platforms. Or is this not supported by the firmware
>>>> used by this machine?
>>>
>>> According to Ben, SKL had limitations in FW for managing the clk's
>>> back in the days.
>>> Can you point to the other driver you mention so we can cross check?
>>
>> There are two KBL drivers that control the SSP clocks from the machine
>> driver, but indeed I don't know if this would work on Firmware, it'd be
>> a question for Lech/Cezary.
>>
>> kbl_rt5663_max98927.c:          ret = clk_prepare_enable(priv->mclk);
>> kbl_rt5663_max98927.c:          ret = clk_prepare_enable(priv->sclk);
>> kbl_rt5663_rt5514_max98927.c:           ret =
>> clk_prepare_enable(priv->mclk);
>> kbl_rt5663_rt5514_max98927.c:           ret =
>> clk_prepare_enable(priv->sclk);
>> kbl_rt5663_rt5514_max98927.c:                   ret =
>> clk_prepare_enable(priv->mclk);
>>
> 
> Czarek answered this and we got the same response from other Intel
> devs while consulting this change:
> FW cannot request a chosen rate (48k) for MCLK pin as it does not
> "align with what's present on SKL hw".
> 
> The only way we found out for NAU8825 to cooperate at chosen rate with
> SKL HW is to regen the MCLK from BCLK by FLL as mentioned above.
> NHTL is used to set SSP0 (48k, 24/25 bit on 24MHz crystal).
> If I get all of this right, use of NHTL and HW "abilities" would
> explain why there is no call to change SSP from a machine driver.
> 
> 
> If all of this is ok I will send V3 with msleep() removed.

Sounds good.

You may want to simplify your commit message and just state what you 
described, e.g.

"Since 64xfs clocks cannot be generated, the NAU8825 is configured to 
re-generate its system clock from the BCLK using the FLL. The link is 
configured to use a 48kHz frame rate, and 24 bits in 25-bit slot. The 
SSP configuration is extracted from NHLT settings and not dynamically 
changed. Listening tests and measurements do not show any distortion or 
issues".

Re: [PATCH V2] ASoC: Intel: boards: Use FS as nau8825 sysclk in nau88125_* machine

[Radosław Biernacki]

Thanks for the tip!
Let me reformat the patch.

wt., 8 wrz 2020 o 20:06 Pierre-Louis Bossart
<pierre-louis.bossart@linux.intel.com> napisał(a):

>
>
>
> On 9/8/20 12:42 PM, Radosław Biernacki wrote:
> > Sorry for missing the response for so long.
> > Somehow lost this thread in my mailbox.
>
> That happens to all of us!
>
> > śr., 6 maj 2020 o 00:04 Pierre-Louis Bossart
> > <pierre-louis.bossart@linux.intel.com> napisał(a):
> >>
> >>
> >>>>> This single fix address two issues on machines with nau88125:
> >>>>> 1) Audio distortion, due to lack of required clock rate on MCLK line
> >>>>> 2) Loud audible "pops" on headphones if there is no sysclk during nau8825
> >>>>>       playback power up sequence
> >>>>>
> >>>>> Explanation for:
> >>>>> 1) Due to Skylake HW limitation, MCLK pin can only output 24MHz clk
> >>>>>       rate (it can be only connected to XTAL parent clk). The BCLK pin
> >>>>>       can be driven by dividers and therefore FW is able to set it to rate
> >>>>>       required by chosen audio format. According to nau8825 datasheet, 256*FS
> >>>>>       sysclk gives the best audio quality and the only way to achieve this
> >>>>>       (taking into account the above limitations) its to regenerate the MCLK
> >>>>>       from BCLK on nau8825 side by FFL. Without required clk rate, audio is
> >>>>>       distorted by added harmonics.
> >>>>
> >>>> The BCLK is going to be a multiple of 50 * Fs due to clocking
> >>>> restrictions. Can the codec regenerate a good-enough sysclk from this?
> >>>
> >>> According to Intel, silicon has a limitation, on SKL/KBL only clk_id =
> >>> SKL_XTAL, .name = "xtal" is available for IO domain.
> >>> As mentioned in the commit:
> >>> MCLK is generated by using 24MHz Xtal directly or applying a divider
> >>> (so no way of achieving the rate required by audio format).
> >>> BCLK/FS is generated from 24MHz and uses dividers and additional
> >>> padding bits are used to match the clock source.
> >>> Next gen silicon has the possibility of using additional clock sources.
> >>>
> >>> Summing up, using MCLK from SKL to NAU88L25 is not an option.
> >>> The only option we found is to use BCLK and regen the required clock
> >>> rate by FLL on the NAU88l25 side.
> >>
> >> Right, this 24 MHz is a recurring problem.
> >> But what I was asking was if the NAU8825 is fine working with e.g. a
> >> 2.4MHz bit clock. i.e. with 25 bit slots or padding at the end of the frame?
> >
> >  From our tests NAU8825 is working fine with these parameters.
> > Also the output audio signal looks fine on the scope and FFT and there
> > are no audible glitches.
> >
> >>
> >>>
> >>>>>
> >>>>> 2) Currently Skylake does not output MCLK/FS when the back-end DAI op
> >>>>>       hw_param is called, so we cannot switch to MCLK/FS in hw_param.  This
> >>>>>       patch reduces pop by letting nau8825 keep using its internal VCO clock
> >>>>>       during widget power up sequence, until SNDRV_PCM_TRIGGER_START when
> >>>>>       MCLK/FS is available. Once device resumes, the system will only enable
> >>>>>       power sequence for playback without doing hardware parameter, audio
> >>>>>       format, and PLL configure. In the mean time, the jack detecion sequence
> >>>>>       has changed PLL parameters and switched to internal clock. Thus, the
> >>>>>       playback signal distorted without correct PLL parameters.  That is why
> >>>>>       we need to configure the PLL again in SNDRV_PCM_TRIGGER_RESUME case.
> >>>>
> >>>> IIRC the FS can be controlled with the clk_ api with the Skylake driver,
> >>>> as done for some KBL platforms. Or is this not supported by the firmware
> >>>> used by this machine?
> >>>
> >>> According to Ben, SKL had limitations in FW for managing the clk's
> >>> back in the days.
> >>> Can you point to the other driver you mention so we can cross check?
> >>
> >> There are two KBL drivers that control the SSP clocks from the machine
> >> driver, but indeed I don't know if this would work on Firmware, it'd be
> >> a question for Lech/Cezary.
> >>
> >> kbl_rt5663_max98927.c:          ret = clk_prepare_enable(priv->mclk);
> >> kbl_rt5663_max98927.c:          ret = clk_prepare_enable(priv->sclk);
> >> kbl_rt5663_rt5514_max98927.c:           ret =
> >> clk_prepare_enable(priv->mclk);
> >> kbl_rt5663_rt5514_max98927.c:           ret =
> >> clk_prepare_enable(priv->sclk);
> >> kbl_rt5663_rt5514_max98927.c:                   ret =
> >> clk_prepare_enable(priv->mclk);
> >>
> >
> > Czarek answered this and we got the same response from other Intel
> > devs while consulting this change:
> > FW cannot request a chosen rate (48k) for MCLK pin as it does not
> > "align with what's present on SKL hw".
> >
> > The only way we found out for NAU8825 to cooperate at chosen rate with
> > SKL HW is to regen the MCLK from BCLK by FLL as mentioned above.
> > NHTL is used to set SSP0 (48k, 24/25 bit on 24MHz crystal).
> > If I get all of this right, use of NHTL and HW "abilities" would
> > explain why there is no call to change SSP from a machine driver.
> >
> >
> > If all of this is ok I will send V3 with msleep() removed.
>
> Sounds good.
>
> You may want to simplify your commit message and just state what you
> described, e.g.
>
> "Since 64xfs clocks cannot be generated, the NAU8825 is configured to
> re-generate its system clock from the BCLK using the FLL. The link is
> configured to use a 48kHz frame rate, and 24 bits in 25-bit slot. The
> SSP configuration is extracted from NHLT settings and not dynamically
> changed. Listening tests and measurements do not show any distortion or
> issues".
>
>
>