diff mbox series

ASoC: Intel: sof_rt5682: add 512FS MCLK clock configuration

Message ID 20220118084952.9555-1-mac.chiang@intel.com (mailing list archive)
State Superseded
Headers show
Series ASoC: Intel: sof_rt5682: add 512FS MCLK clock configuration | expand

Commit Message

Chiang, Mac Jan. 18, 2022, 8:49 a.m. UTC 
codec system clock source support 512FS MCLK synchronous directly, so
no need to set PLL configuration when MCLK 24.576MHz.

Suggested-by: Shuming Fan <shumingf@realtek.com>
Signed-off-by: Mac Chiang <mac.chiang@intel.com>
---
 sound/soc/intel/boards/sof_rt5682.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

Comments

Pierre-Louis Bossart Jan. 18, 2022, 3:19 p.m. UTC | #1 
On 1/18/22 2:49 AM, Mac Chiang wrote:
> codec system clock source support 512FS MCLK synchronous directly, so
> no need to set PLL configuration when MCLK 24.576MHz.
> 
> Suggested-by: Shuming Fan <shumingf@realtek.com>
> Signed-off-by: Mac Chiang <mac.chiang@intel.com>
> ---
>  sound/soc/intel/boards/sof_rt5682.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/sound/soc/intel/boards/sof_rt5682.c b/sound/soc/intel/boards/sof_rt5682.c
> index bd6d2e7dea53..a6efffc14cad 100644
> --- a/sound/soc/intel/boards/sof_rt5682.c
> +++ b/sound/soc/intel/boards/sof_rt5682.c
> @@ -61,7 +61,6 @@
>  #define SOF_MAX98390_SPEAKER_AMP_PRESENT	BIT(24)
>  #define SOF_MAX98390_TWEETER_SPEAKER_PRESENT	BIT(25)
>  
> -
>  /* Default: MCLK on, MCLK 19.2M, SSP0  */
>  static unsigned long sof_rt5682_quirk = SOF_RT5682_MCLK_EN |
>  					SOF_RT5682_SSP_CODEC(0);
> @@ -362,6 +361,9 @@ static int sof_rt5682_hw_params(struct snd_pcm_substream *substream,
>  	if (sof_rt5682_quirk & SOF_RT5682S_HEADPHONE_CODEC_PRESENT) {
>  		pll_id = RT5682S_PLL2;
>  		clk_id = RT5682S_SCLK_S_PLL2;
> +
> +		if (pll_in == 24576000)
> +			clk_id = RT5682S_SCLK_S_MCLK;

this case only affects the RT5682s case.

>  	} else {
>  		pll_id = RT5682_PLL1;
>  		clk_id = RT5682_SCLK_S_PLL1;

for the RT5682 we keep using the PLL. Is this intentional?

I also wonder if we can avoid the hard-coding. Can we use e.g.

if (pll_in == params_rate(params) * 512)

?


> @@ -369,11 +371,14 @@ static int sof_rt5682_hw_params(struct snd_pcm_substream *substream,
>  
>  	pll_out = params_rate(params) * 512;
>  
> -	/* Configure pll for codec */
> -	ret = snd_soc_dai_set_pll(codec_dai, pll_id, pll_source, pll_in,
> -				  pll_out);
> -	if (ret < 0)
> -		dev_err(rtd->dev, "snd_soc_dai_set_pll err = %d\n", ret);
> +	/* when MCLK is 512FS, no need to set PLL configuration additionally. */
> +	if (pll_in != 24576000) {

can we use if (pll_in == pll_out) ?

> +		/* Configure pll for codec */
> +		ret = snd_soc_dai_set_pll(codec_dai, pll_id, pll_source, pll_in,
> +					  pll_out);
> +		if (ret < 0)
> +			dev_err(rtd->dev, "snd_soc_dai_set_pll err = %d\n", ret);
> +	}
>  
>  	/* Configure sysclk for codec */
>  	ret = snd_soc_dai_set_sysclk(codec_dai, clk_id,
>
Chiang, Mac Jan. 19, 2022, 6:40 a.m. UTC | #2 
> > @@ -362,6 +361,9 @@ static int sof_rt5682_hw_params(struct
> snd_pcm_substream *substream,
> >  	if (sof_rt5682_quirk & SOF_RT5682S_HEADPHONE_CODEC_PRESENT)
> {
> >  		pll_id = RT5682S_PLL2;
> >  		clk_id = RT5682S_SCLK_S_PLL2;
> > +
> > +		if (pll_in == 24576000)
> > +			clk_id = RT5682S_SCLK_S_MCLK;
> 
> this case only affects the RT5682s case.
Thanks Pierre, you are right. Per discussed with Shuming, both 5682vs and 5682vd have to affect.
> 
> >  	} else {
> >  		pll_id = RT5682_PLL1;
> >  		clk_id = RT5682_SCLK_S_PLL1;
> 
> for the RT5682 we keep using the PLL. Is this intentional?
> 
> I also wonder if we can avoid the hard-coding. Can we use e.g.
> 
> if (pll_in == params_rate(params) * 512)
Agree, no hard-coding.
> ?
>
> 
> > @@ -369,11 +371,14 @@ static int sof_rt5682_hw_params(struct
> > snd_pcm_substream *substream,
> >
> >  	pll_out = params_rate(params) * 512;
> >
> > -	/* Configure pll for codec */
> > -	ret = snd_soc_dai_set_pll(codec_dai, pll_id, pll_source, pll_in,
> > -				  pll_out);
> > -	if (ret < 0)
> > -		dev_err(rtd->dev, "snd_soc_dai_set_pll err = %d\n", ret);
> > +	/* when MCLK is 512FS, no need to set PLL configuration additionally.
> */
> > +	if (pll_in != 24576000) {
> 
> can we use if (pll_in == pll_out) ?
Agree, will modify as if (!(pll_in == pll_out)) {
> 
> > +		/* Configure pll for codec */
> > +		ret = snd_soc_dai_set_pll(codec_dai, pll_id, pll_source, pll_in,
> > +					  pll_out);
> > +		if (ret < 0)
> > +			dev_err(rtd->dev, "snd_soc_dai_set_pll err = %d\n",
> ret);
> > +	}
> >
> >  	/* Configure sysclk for codec */
> >  	ret = snd_soc_dai_set_sysclk(codec_dai, clk_id,
> >
Brent Lu Jan. 19, 2022, 8:54 a.m. UTC | #3 
> > > @@ -362,6 +361,9 @@ static int sof_rt5682_hw_params(struct
> > snd_pcm_substream *substream,
> > >  	if (sof_rt5682_quirk & SOF_RT5682S_HEADPHONE_CODEC_PRESENT)
> > {
> > >  		pll_id = RT5682S_PLL2;
> > >  		clk_id = RT5682S_SCLK_S_PLL2;
> > > +
> > > +		if (pll_in == 24576000)
> > > +			clk_id = RT5682S_SCLK_S_MCLK;
> >
> > this case only affects the RT5682s case.
> Thanks Pierre, you are right. Per discussed with Shuming, both 5682vs and
> 5682vd have to affect.
> >
> > >  	} else {
> > >  		pll_id = RT5682_PLL1;
> > >  		clk_id = RT5682_SCLK_S_PLL1;
> >
> > for the RT5682 we keep using the PLL. Is this intentional?
> >
> > I also wonder if we can avoid the hard-coding. Can we use e.g.
> >
> > if (pll_in == params_rate(params) * 512)
> Agree, no hard-coding.
> > ?
> >
> >
> > > @@ -369,11 +371,14 @@ static int sof_rt5682_hw_params(struct
> > > snd_pcm_substream *substream,
> > >
> > >  	pll_out = params_rate(params) * 512;
> > >
> > > -	/* Configure pll for codec */
> > > -	ret = snd_soc_dai_set_pll(codec_dai, pll_id, pll_source, pll_in,
> > > -				  pll_out);
> > > -	if (ret < 0)
> > > -		dev_err(rtd->dev, "snd_soc_dai_set_pll err = %d\n", ret);
> > > +	/* when MCLK is 512FS, no need to set PLL configuration additionally.
> > */
> > > +	if (pll_in != 24576000) {
> >
> > can we use if (pll_in == pll_out) ?
> Agree, will modify as if (!(pll_in == pll_out)) {
> >

Maybe something like this?

	if (pll_in == pll_out)
		clk_id = RT5682S_SCLK_S_MCLK;
	else {
		/* Configure pll for codec */
		ret = snd_soc_dai_set_pll(codec_dai, pll_id, pll_source, pll_in,
					  pll_out);
		if (ret < 0)
			dev_err(rtd->dev, "snd_soc_dai_set_pll err = %d\n", ret);
	}

> > > +		/* Configure pll for codec */
> > > +		ret = snd_soc_dai_set_pll(codec_dai, pll_id, pll_source, pll_in,
> > > +					  pll_out);
> > > +		if (ret < 0)
> > > +			dev_err(rtd->dev, "snd_soc_dai_set_pll err = %d\n",
> > ret);
> > > +	}
> > >
> > >  	/* Configure sysclk for codec */
> > >  	ret = snd_soc_dai_set_sysclk(codec_dai, clk_id,
> > >
diff mbox series

Patch

diff --git a/sound/soc/intel/boards/sof_rt5682.c b/sound/soc/intel/boards/sof_rt5682.c
index bd6d2e7dea53..a6efffc14cad 100644
--- a/sound/soc/intel/boards/sof_rt5682.c
+++ b/sound/soc/intel/boards/sof_rt5682.c
@@ -61,7 +61,6 @@ 
 #define SOF_MAX98390_SPEAKER_AMP_PRESENT	BIT(24)
 #define SOF_MAX98390_TWEETER_SPEAKER_PRESENT	BIT(25)
 
-
 /* Default: MCLK on, MCLK 19.2M, SSP0  */
 static unsigned long sof_rt5682_quirk = SOF_RT5682_MCLK_EN |
 					SOF_RT5682_SSP_CODEC(0);
@@ -362,6 +361,9 @@  static int sof_rt5682_hw_params(struct snd_pcm_substream *substream,
 	if (sof_rt5682_quirk & SOF_RT5682S_HEADPHONE_CODEC_PRESENT) {
 		pll_id = RT5682S_PLL2;
 		clk_id = RT5682S_SCLK_S_PLL2;
+
+		if (pll_in == 24576000)
+			clk_id = RT5682S_SCLK_S_MCLK;
 	} else {
 		pll_id = RT5682_PLL1;
 		clk_id = RT5682_SCLK_S_PLL1;
@@ -369,11 +371,14 @@  static int sof_rt5682_hw_params(struct snd_pcm_substream *substream,
 
 	pll_out = params_rate(params) * 512;
 
-	/* Configure pll for codec */
-	ret = snd_soc_dai_set_pll(codec_dai, pll_id, pll_source, pll_in,
-				  pll_out);
-	if (ret < 0)
-		dev_err(rtd->dev, "snd_soc_dai_set_pll err = %d\n", ret);
+	/* when MCLK is 512FS, no need to set PLL configuration additionally. */
+	if (pll_in != 24576000) {
+		/* Configure pll for codec */
+		ret = snd_soc_dai_set_pll(codec_dai, pll_id, pll_source, pll_in,
+					  pll_out);
+		if (ret < 0)
+			dev_err(rtd->dev, "snd_soc_dai_set_pll err = %d\n", ret);
+	}
 
 	/* Configure sysclk for codec */
 	ret = snd_soc_dai_set_sysclk(codec_dai, clk_id,