| Message ID | 1595762417-2190-1-git-send-email-shengjiu.wang@nxp.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | ASoC: fsl-asoc-card: Remove fsl_asoc_card_set_bias_level function | expand |
On Sun, Jul 26, 2020 at 07:20:17PM +0800, Shengjiu Wang wrote: > With this case: > aplay -Dhw:x 16khz.wav 24khz.wav > There is sound distortion for 24khz.wav. The reason is that setting > PLL of WM8962 with set_bias_level function, the bias level is not > changed when 24khz.wav is played, then the PLL won't be reset, the > clock is not correct, so distortion happens. > > The resolution of this issue is to remove fsl_asoc_card_set_bias_level. > Move PLL configuration to hw_params and hw_free. Hmm...using set_bias_level() instead of hw_params/hw_free() was strongly suggested by Mark when I got imx-wm8962 machine driver upstream. So we will need his input here, although I personally don't have a problem with it... > After removing fsl_asoc_card_set_bias_level, also test WM8960 case, > it can work. > > Fixes: 708b4351f08c ("ASoC: fsl: Add Freescale Generic ASoC Sound Card with ASRC support") > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com> > --- > sound/soc/fsl/fsl-asoc-card.c | 149 +++++++++++++++------------------- > 1 file changed, 66 insertions(+), 83 deletions(-) > > diff --git a/sound/soc/fsl/fsl-asoc-card.c b/sound/soc/fsl/fsl-asoc-card.c > index 4848ba61d083..0517dbb3e908 100644 > --- a/sound/soc/fsl/fsl-asoc-card.c > +++ b/sound/soc/fsl/fsl-asoc-card.c > @@ -73,6 +73,7 @@ struct cpu_priv { > * @codec_priv: CODEC private data > * @cpu_priv: CPU private data > * @card: ASoC card structure > + * @is_stream_in_use: flags for release resource in hw_free Would love to see something shorter... Could we reuse similar one below, borrowing from fsl_ssi driver? * @streams: Mask of current active streams: BIT(TX) and BIT(RX) > static int fsl_asoc_card_audmux_init(struct device_node *np, > struct fsl_asoc_card_priv *priv) > { > @@ -611,7 +600,6 @@ static int fsl_asoc_card_probe(struct platform_device *pdev) > /* Diversify the card configurations */ > if (of_device_is_compatible(np, "fsl,imx-audio-cs42888")) { > codec_dai_name = "cs42888"; > - priv->card.set_bias_level = NULL; Can check if set_bias_level is still being used with this change.
On Mon, Jul 27, 2020 at 8:58 AM Nicolin Chen <nicoleotsuka@gmail.com> wrote: > > On Sun, Jul 26, 2020 at 07:20:17PM +0800, Shengjiu Wang wrote: > > With this case: > > aplay -Dhw:x 16khz.wav 24khz.wav > > There is sound distortion for 24khz.wav. The reason is that setting > > PLL of WM8962 with set_bias_level function, the bias level is not > > changed when 24khz.wav is played, then the PLL won't be reset, the > > clock is not correct, so distortion happens. > > > > The resolution of this issue is to remove fsl_asoc_card_set_bias_level. > > Move PLL configuration to hw_params and hw_free. > > Hmm...using set_bias_level() instead of hw_params/hw_free() was > strongly suggested by Mark when I got imx-wm8962 machine driver > upstream. So we will need his input here, although I personally > don't have a problem with it... > > > After removing fsl_asoc_card_set_bias_level, also test WM8960 case, > > it can work. > > > > Fixes: 708b4351f08c ("ASoC: fsl: Add Freescale Generic ASoC Sound Card with ASRC support") > > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com> > > --- > > sound/soc/fsl/fsl-asoc-card.c | 149 +++++++++++++++------------------- > > 1 file changed, 66 insertions(+), 83 deletions(-) > > > > diff --git a/sound/soc/fsl/fsl-asoc-card.c b/sound/soc/fsl/fsl-asoc-card.c > > index 4848ba61d083..0517dbb3e908 100644 > > --- a/sound/soc/fsl/fsl-asoc-card.c > > +++ b/sound/soc/fsl/fsl-asoc-card.c > > @@ -73,6 +73,7 @@ struct cpu_priv { > > * @codec_priv: CODEC private data > > * @cpu_priv: CPU private data > > * @card: ASoC card structure > > + * @is_stream_in_use: flags for release resource in hw_free > > Would love to see something shorter... Could we reuse similar > one below, borrowing from fsl_ssi driver? > > * @streams: Mask of current active streams: BIT(TX) and BIT(RX) > > > static int fsl_asoc_card_audmux_init(struct device_node *np, > > struct fsl_asoc_card_priv *priv) > > { > > @@ -611,7 +600,6 @@ static int fsl_asoc_card_probe(struct platform_device *pdev) > > /* Diversify the card configurations */ > > if (of_device_is_compatible(np, "fsl,imx-audio-cs42888")) { > > codec_dai_name = "cs42888"; > > - priv->card.set_bias_level = NULL; > > Can check if set_bias_level is still being used with this change. Do you mean to keep this line: priv->card.set_bias_level = NULL; ? best regards wang shengjiu
On Mon, Jul 27, 2020 at 02:33:18PM +0800, Shengjiu Wang wrote: > > > static int fsl_asoc_card_audmux_init(struct device_node *np, > > > struct fsl_asoc_card_priv *priv) > > > { > > > @@ -611,7 +600,6 @@ static int fsl_asoc_card_probe(struct platform_device *pdev) > > > /* Diversify the card configurations */ > > > if (of_device_is_compatible(np, "fsl,imx-audio-cs42888")) { > > > codec_dai_name = "cs42888"; > > > - priv->card.set_bias_level = NULL; > > > > Can check if set_bias_level is still being used with this change. > > Do you mean to keep this line: > priv->card.set_bias_level = NULL; ? Sorry. You can just ignore this part -- just double checked the code, and I think I misread something :-/
On Mon, Jul 27, 2020 at 8:58 AM Nicolin Chen <nicoleotsuka@gmail.com> wrote: > > On Sun, Jul 26, 2020 at 07:20:17PM +0800, Shengjiu Wang wrote: > > With this case: > > aplay -Dhw:x 16khz.wav 24khz.wav > > There is sound distortion for 24khz.wav. The reason is that setting > > PLL of WM8962 with set_bias_level function, the bias level is not > > changed when 24khz.wav is played, then the PLL won't be reset, the > > clock is not correct, so distortion happens. > > > > The resolution of this issue is to remove fsl_asoc_card_set_bias_level. > > Move PLL configuration to hw_params and hw_free. > > Hmm...using set_bias_level() instead of hw_params/hw_free() was > strongly suggested by Mark when I got imx-wm8962 machine driver > upstream. So we will need his input here, although I personally > don't have a problem with it... > > > After removing fsl_asoc_card_set_bias_level, also test WM8960 case, > > it can work. > > > > Fixes: 708b4351f08c ("ASoC: fsl: Add Freescale Generic ASoC Sound Card with ASRC support") > > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com> > > --- > > sound/soc/fsl/fsl-asoc-card.c | 149 +++++++++++++++------------------- > > 1 file changed, 66 insertions(+), 83 deletions(-) > > > > diff --git a/sound/soc/fsl/fsl-asoc-card.c b/sound/soc/fsl/fsl-asoc-card.c > > index 4848ba61d083..0517dbb3e908 100644 > > --- a/sound/soc/fsl/fsl-asoc-card.c > > +++ b/sound/soc/fsl/fsl-asoc-card.c > > @@ -73,6 +73,7 @@ struct cpu_priv { > > * @codec_priv: CODEC private data > > * @cpu_priv: CPU private data > > * @card: ASoC card structure > > + * @is_stream_in_use: flags for release resource in hw_free > > Would love to see something shorter... Could we reuse similar > one below, borrowing from fsl_ssi driver? > > * @streams: Mask of current active streams: BIT(TX) and BIT(RX) > will send v2 for this change. > > static int fsl_asoc_card_audmux_init(struct device_node *np, > > struct fsl_asoc_card_priv *priv) > > { > > @@ -611,7 +600,6 @@ static int fsl_asoc_card_probe(struct platform_device *pdev) > > /* Diversify the card configurations */ > > if (of_device_is_compatible(np, "fsl,imx-audio-cs42888")) { > > codec_dai_name = "cs42888"; > > - priv->card.set_bias_level = NULL; > > Can check if set_bias_level is still being used with this change.
diff --git a/sound/soc/fsl/fsl-asoc-card.c b/sound/soc/fsl/fsl-asoc-card.c index 4848ba61d083..0517dbb3e908 100644 --- a/sound/soc/fsl/fsl-asoc-card.c +++ b/sound/soc/fsl/fsl-asoc-card.c @@ -73,6 +73,7 @@ struct cpu_priv { * @codec_priv: CODEC private data * @cpu_priv: CPU private data * @card: ASoC card structure + * @is_stream_in_use: flags for release resource in hw_free * @sample_rate: Current sample rate * @sample_format: Current sample format * @asrc_rate: ASRC sample rate used by Back-Ends @@ -89,6 +90,7 @@ struct fsl_asoc_card_priv { struct codec_priv codec_priv; struct cpu_priv cpu_priv; struct snd_soc_card card; + bool is_stream_in_use[2]; u32 sample_rate; snd_pcm_format_t sample_format; u32 asrc_rate; @@ -151,21 +153,17 @@ static int fsl_asoc_card_hw_params(struct snd_pcm_substream *substream, struct snd_soc_pcm_runtime *rtd = substream->private_data; struct fsl_asoc_card_priv *priv = snd_soc_card_get_drvdata(rtd->card); bool tx = substream->stream == SNDRV_PCM_STREAM_PLAYBACK; + struct codec_priv *codec_priv = &priv->codec_priv; struct cpu_priv *cpu_priv = &priv->cpu_priv; struct device *dev = rtd->card->dev; + unsigned int pll_out; int ret; priv->sample_rate = params_rate(params); priv->sample_format = params_format(params); + priv->is_stream_in_use[tx] = true; - /* - * If codec-dai is DAI Master and all configurations are already in the - * set_bias_level(), bypass the remaining settings in hw_params(). - * Note: (dai_fmt & CBM_CFM) includes CBM_CFM and CBM_CFS. - */ - if ((priv->card.set_bias_level && - priv->dai_fmt & SND_SOC_DAIFMT_CBM_CFM) || - fsl_asoc_card_is_ac97(priv)) + if (fsl_asoc_card_is_ac97(priv)) return 0; /* Specific configurations of DAIs starts from here */ @@ -185,12 +183,72 @@ static int fsl_asoc_card_hw_params(struct snd_pcm_substream *substream, return ret; } } + /* Specific configuration for PLL */ + if (codec_priv->pll_id && codec_priv->fll_id) { + if (priv->sample_format == SNDRV_PCM_FORMAT_S24_LE) + pll_out = priv->sample_rate * 384; + else + pll_out = priv->sample_rate * 256; + + ret = snd_soc_dai_set_pll(asoc_rtd_to_codec(rtd, 0), + codec_priv->pll_id, + codec_priv->mclk_id, + codec_priv->mclk_freq, pll_out); + if (ret) { + dev_err(dev, "failed to start FLL: %d\n", ret); + return ret; + } + + ret = snd_soc_dai_set_sysclk(asoc_rtd_to_codec(rtd, 0), + codec_priv->fll_id, + pll_out, SND_SOC_CLOCK_IN); + + if (ret && ret != -ENOTSUPP) { + dev_err(dev, "failed to set SYSCLK: %d\n", ret); + return ret; + } + } + + return 0; +} + +static int fsl_asoc_card_hw_free(struct snd_pcm_substream *substream) +{ + struct snd_soc_pcm_runtime *rtd = substream->private_data; + struct fsl_asoc_card_priv *priv = snd_soc_card_get_drvdata(rtd->card); + bool tx = substream->stream == SNDRV_PCM_STREAM_PLAYBACK; + struct codec_priv *codec_priv = &priv->codec_priv; + struct device *dev = rtd->card->dev; + int ret; + + priv->is_stream_in_use[tx] = false; + + if (!priv->is_stream_in_use[!tx] && codec_priv->pll_id && + codec_priv->fll_id) { + /* Force freq to be 0 to avoid error message in codec */ + ret = snd_soc_dai_set_sysclk(asoc_rtd_to_codec(rtd, 0), + codec_priv->mclk_id, + 0, + SND_SOC_CLOCK_IN); + if (ret) { + dev_err(dev, "failed to switch away from FLL: %d\n", ret); + return ret; + } + + ret = snd_soc_dai_set_pll(asoc_rtd_to_codec(rtd, 0), + codec_priv->pll_id, 0, 0, 0); + if (ret && ret != -ENOTSUPP) { + dev_err(dev, "failed to stop FLL: %d\n", ret); + return ret; + } + } return 0; } static const struct snd_soc_ops fsl_asoc_card_ops = { .hw_params = fsl_asoc_card_hw_params, + .hw_free = fsl_asoc_card_hw_free, }; static int be_hw_params_fixup(struct snd_soc_pcm_runtime *rtd, @@ -254,75 +312,6 @@ static struct snd_soc_dai_link fsl_asoc_card_dai[] = { }, }; -static int fsl_asoc_card_set_bias_level(struct snd_soc_card *card, - struct snd_soc_dapm_context *dapm, - enum snd_soc_bias_level level) -{ - struct fsl_asoc_card_priv *priv = snd_soc_card_get_drvdata(card); - struct snd_soc_pcm_runtime *rtd; - struct snd_soc_dai *codec_dai; - struct codec_priv *codec_priv = &priv->codec_priv; - struct device *dev = card->dev; - unsigned int pll_out; - int ret; - - rtd = snd_soc_get_pcm_runtime(card, &card->dai_link[0]); - codec_dai = asoc_rtd_to_codec(rtd, 0); - if (dapm->dev != codec_dai->dev) - return 0; - - switch (level) { - case SND_SOC_BIAS_PREPARE: - if (dapm->bias_level != SND_SOC_BIAS_STANDBY) - break; - - if (priv->sample_format == SNDRV_PCM_FORMAT_S24_LE) - pll_out = priv->sample_rate * 384; - else - pll_out = priv->sample_rate * 256; - - ret = snd_soc_dai_set_pll(codec_dai, codec_priv->pll_id, - codec_priv->mclk_id, - codec_priv->mclk_freq, pll_out); - if (ret) { - dev_err(dev, "failed to start FLL: %d\n", ret); - return ret; - } - - ret = snd_soc_dai_set_sysclk(codec_dai, codec_priv->fll_id, - pll_out, SND_SOC_CLOCK_IN); - if (ret && ret != -ENOTSUPP) { - dev_err(dev, "failed to set SYSCLK: %d\n", ret); - return ret; - } - break; - - case SND_SOC_BIAS_STANDBY: - if (dapm->bias_level != SND_SOC_BIAS_PREPARE) - break; - - ret = snd_soc_dai_set_sysclk(codec_dai, codec_priv->mclk_id, - codec_priv->mclk_freq, - SND_SOC_CLOCK_IN); - if (ret && ret != -ENOTSUPP) { - dev_err(dev, "failed to switch away from FLL: %d\n", ret); - return ret; - } - - ret = snd_soc_dai_set_pll(codec_dai, codec_priv->pll_id, 0, 0, 0); - if (ret) { - dev_err(dev, "failed to stop FLL: %d\n", ret); - return ret; - } - break; - - default: - break; - } - - return 0; -} - static int fsl_asoc_card_audmux_init(struct device_node *np, struct fsl_asoc_card_priv *priv) { @@ -611,7 +600,6 @@ static int fsl_asoc_card_probe(struct platform_device *pdev) /* Diversify the card configurations */ if (of_device_is_compatible(np, "fsl,imx-audio-cs42888")) { codec_dai_name = "cs42888"; - priv->card.set_bias_level = NULL; priv->cpu_priv.sysclk_freq[TX] = priv->codec_priv.mclk_freq; priv->cpu_priv.sysclk_freq[RX] = priv->codec_priv.mclk_freq; priv->cpu_priv.sysclk_dir[TX] = SND_SOC_CLOCK_OUT; @@ -628,26 +616,22 @@ static int fsl_asoc_card_probe(struct platform_device *pdev) priv->dai_fmt |= SND_SOC_DAIFMT_CBM_CFM; } else if (of_device_is_compatible(np, "fsl,imx-audio-wm8962")) { codec_dai_name = "wm8962"; - priv->card.set_bias_level = fsl_asoc_card_set_bias_level; priv->codec_priv.mclk_id = WM8962_SYSCLK_MCLK; priv->codec_priv.fll_id = WM8962_SYSCLK_FLL; priv->codec_priv.pll_id = WM8962_FLL; priv->dai_fmt |= SND_SOC_DAIFMT_CBM_CFM; } else if (of_device_is_compatible(np, "fsl,imx-audio-wm8960")) { codec_dai_name = "wm8960-hifi"; - priv->card.set_bias_level = fsl_asoc_card_set_bias_level; priv->codec_priv.fll_id = WM8960_SYSCLK_AUTO; priv->codec_priv.pll_id = WM8960_SYSCLK_AUTO; priv->dai_fmt |= SND_SOC_DAIFMT_CBM_CFM; } else if (of_device_is_compatible(np, "fsl,imx-audio-ac97")) { codec_dai_name = "ac97-hifi"; - priv->card.set_bias_level = NULL; priv->dai_fmt = SND_SOC_DAIFMT_AC97; priv->card.dapm_routes = audio_map_ac97; priv->card.num_dapm_routes = ARRAY_SIZE(audio_map_ac97); } else if (of_device_is_compatible(np, "fsl,imx-audio-mqs")) { codec_dai_name = "fsl-mqs-dai"; - priv->card.set_bias_level = NULL; priv->dai_fmt = SND_SOC_DAIFMT_LEFT_J | SND_SOC_DAIFMT_CBS_CFS | SND_SOC_DAIFMT_NB_NF; @@ -657,7 +641,6 @@ static int fsl_asoc_card_probe(struct platform_device *pdev) priv->card.num_dapm_routes = ARRAY_SIZE(audio_map_tx); } else if (of_device_is_compatible(np, "fsl,imx-audio-wm8524")) { codec_dai_name = "wm8524-hifi"; - priv->card.set_bias_level = NULL; priv->dai_fmt |= SND_SOC_DAIFMT_CBS_CFS; priv->dai_link[1].dpcm_capture = 0; priv->dai_link[2].dpcm_capture = 0;
With this case: aplay -Dhw:x 16khz.wav 24khz.wav There is sound distortion for 24khz.wav. The reason is that setting PLL of WM8962 with set_bias_level function, the bias level is not changed when 24khz.wav is played, then the PLL won't be reset, the clock is not correct, so distortion happens. The resolution of this issue is to remove fsl_asoc_card_set_bias_level. Move PLL configuration to hw_params and hw_free. After removing fsl_asoc_card_set_bias_level, also test WM8960 case, it can work. Fixes: 708b4351f08c ("ASoC: fsl: Add Freescale Generic ASoC Sound Card with ASRC support") Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com> --- sound/soc/fsl/fsl-asoc-card.c | 149 +++++++++++++++------------------- 1 file changed, 66 insertions(+), 83 deletions(-)