Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200910154056.30962-1-rad@semihalf.com>
In-Reply-To: <20200910154056.30962-1-rad@semihalf.com>
From:   =?UTF-8?Q?Rados=C5=82aw_Biernacki?= <rad@semihalf.com>
Date:   Thu, 10 Sep 2020 17:48:34 +0200
Message-ID: <CAOs-w0JSFOZf_H3puAAkwbhZH1cWKit8R_2BqmEtx8y9n=ebEA@mail.gmail.com>
Subject: Re: [PATCH V4] ASoC: Intel: boards: Use FS as nau8825 sysclk in
 nau88125_* machine
To:     Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>,
        Liam Girdwood <liam.r.girdwood@linux.intel.com>,
        Jie Yang <yang.jie@linux.intel.com>,
        Jaroslav Kysela <perex@perex.cz>, Takashi Iwai <tiwai@suse.com>
Cc:     Ben Zhang <benzh@chromium.org>, Marcin Wojtas <mw@semihalf.com>,
        Todd Broch <tbroch@google.com>,
        Alex Levin <levinale@google.com>,
        Vamshi Krishna <vamshi.krishna.gopal@intel.com>,
        Harshapriya <harshapriya.n@intel.com>,
        "Sienkiewicz, Michal" <michal.sienkiewicz@intel.com>,
        Lech Betlej <Lech.Betlej@intel.com>,
        alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org,
        John Hsu <KCHSU0@nuvoton.com>, Yong Zhi <yong.zhi@intel.com>,
        Mac Chiang <mac.chiang@intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Sorry, V4 got one stray change for SSP0 ops.
Need to send V5.

czw., 10 wrz 2020 o 17:40 Radoslaw Biernacki <rad@semihalf.com> napisa=C5=
=82(a):
>
> Since 256xFS clocks cannot be generated by SKL, 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
>
> 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>
> ---
>
> Notes:
>     v1 -> v2:
>     - adding same changes to skl_nau88l25_max98357a.c
>     v2 -> v3:
>     - removing msleep() in SNDRV_PCM_TRIGGER_RESUME as it unnecessarily i=
ncrease
>       playback/capture latency while actually FLL does not require it.
>     - simplifing commit message
>     v3 -> v4:
>     - simplifing the PM resume callback code for setting the FLL
>     - adding comment for the stream START/RESUME sequence which prevent a=
udio pops
>     - fixing mising var initialization in platform_clock_control()
>
>  .../soc/intel/boards/skl_nau88l25_max98357a.c | 63 ++++++++++++------
>  sound/soc/intel/boards/skl_nau88l25_ssm4567.c | 65 +++++++++++++------
>  2 files changed, 86 insertions(+), 42 deletions(-)
>
> diff --git a/sound/soc/intel/boards/skl_nau88l25_max98357a.c b/sound/soc/=
intel/boards/skl_nau88l25_max98357a.c
> index d7b8154c43a4..2f0abbd2dd8d 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,12 +48,12 @@ 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 =3D w->dapm;
>         struct snd_soc_card *card =3D dapm->card;
>         struct snd_soc_dai *codec_dai;
> -       int ret;
> +       int ret =3D 0;
>
>         codec_dai =3D snd_soc_card_get_codec_dai(card, SKL_NUVOTON_CODEC_=
DAI);
>         if (!codec_dai) {
> @@ -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 =3D 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 =3D %d\n", ret=
);
> -                       return -EIO;
> -               }
> -       } else {
> +       if (!SND_SOC_DAPM_EVENT_ON(event)) {
>                 ret =3D snd_soc_dai_set_sysclk(codec_dai,
>                                 NAU8825_CLK_INTERNAL, 0, SND_SOC_CLOCK_IN=
);
>                 if (ret < 0) {
> @@ -292,24 +286,51 @@ static const struct snd_soc_ops skylake_nau8825_fe_=
ops =3D {
>         .startup =3D 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 =3D substream->private_data;
> +       struct snd_pcm_runtime *runtime =3D substream->runtime;
>         struct snd_soc_dai *codec_dai =3D asoc_rtd_to_codec(rtd, 0);
> -       int ret;
> -
> -       ret =3D snd_soc_dai_set_sysclk(codec_dai,
> -                       NAU8825_CLK_MCLK, 24000000, SND_SOC_CLOCK_IN);
> -
> -       if (ret < 0)
> -               dev_err(rtd->dev, "snd_soc_dai_set_sysclk err =3D %d\n", =
ret);
> +       int ret =3D 0;
> +
> +       switch (cmd) {
> +       case SNDRV_PCM_TRIGGER_START:
> +               /* Since 256xFS clocks cannot be generated by SKL, the NA=
U8825
> +                * is configured to re-generate its system clock from the=
 BCLK
> +                * using the FLL.
> +                * We must switch system clock (FLL to use BCLK) here as =
it is
> +                * not given eariler by FW (like in hw_param). We let nau=
8825 to
> +                * use internal VCO clock till now which reduces the audi=
able
> +                * pop's. */
> +
> +               /* fall through */
> +
> +       case SNDRV_PCM_TRIGGER_RESUME:
> +               /* 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 switc=
hed to
> +                * internal clock. Thus, the playback signal distorted wi=
thout
> +                * correct PLL parameters. Therefore we need to configure=
 PLL
> +                * again */
> +               ret =3D 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 =3D snd_soc_dai_set_pll(codec_dai, 0, 0, runtime->rat=
e,
> +                                         runtime->rate * 256);
> +               if (ret < 0)
> +                       dev_err(codec_dai->dev, "can't set FLL: %d\n", re=
t);
> +               break;
> +       }
>
>         return ret;
>  }
>
> -static const struct snd_soc_ops skylake_nau8825_ops =3D {
> -       .hw_params =3D skylake_nau8825_hw_params,
> +static struct snd_soc_ops skylake_nau8825_ops =3D {
> +       .trigger =3D skylake_nau8825_trigger,
>  };
>
>  static int skylake_dmic_fixup(struct snd_soc_pcm_runtime *rtd,
> diff --git a/sound/soc/intel/boards/skl_nau88l25_ssm4567.c b/sound/soc/in=
tel/boards/skl_nau88l25_ssm4567.c
> index 4b317bcf6ea0..d076f19f9b78 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,12 +58,12 @@ static const struct snd_kcontrol_new skylake_controls=
[] =3D {
>  };
>
>  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 =3D w->dapm;
>         struct snd_soc_card *card =3D dapm->card;
>         struct snd_soc_dai *codec_dai;
> -       int ret;
> +       int ret =3D 0;
>
>         codec_dai =3D snd_soc_card_get_codec_dai(card, SKL_NUVOTON_CODEC_=
DAI);
>         if (!codec_dai) {
> @@ -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 =3D 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 =3D %d\n", ret=
);
> -                       return -EIO;
> -               }
> -       } else {
> +       if (!SND_SOC_DAPM_EVENT_ON(event)) {
>                 ret =3D 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_w=
idget *w,
>                         return -EIO;
>                 }
>         }
> +
>         return ret;
>  }
>
> @@ -344,24 +339,51 @@ static int skylake_dmic_fixup(struct snd_soc_pcm_ru=
ntime *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 =3D substream->private_data;
> +       struct snd_pcm_runtime *runtime =3D substream->runtime;
>         struct snd_soc_dai *codec_dai =3D asoc_rtd_to_codec(rtd, 0);
> -       int ret;
> -
> -       ret =3D snd_soc_dai_set_sysclk(codec_dai,
> -                       NAU8825_CLK_MCLK, 24000000, SND_SOC_CLOCK_IN);
> -
> -       if (ret < 0)
> -               dev_err(rtd->dev, "snd_soc_dai_set_sysclk err =3D %d\n", =
ret);
> +       int ret =3D 0;
> +
> +       switch (cmd) {
> +       case SNDRV_PCM_TRIGGER_START:
> +               /* Since 256xFS clocks cannot be generated by SKL, the NA=
U8825
> +                * is configured to re-generate its system clock from the=
 BCLK
> +                * using the FLL.
> +                * We must switch system clock (FLL to use BCLK) here as =
it is
> +                * not given eariler by FW (like in hw_param). We let nau=
8825 to
> +                * use internal VCO clock till now which reduces the audi=
able
> +                * pop's. */
> +
> +               /* fall through */
> +
> +       case SNDRV_PCM_TRIGGER_RESUME:
> +               /* 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 switc=
hed to
> +                * internal clock. Thus, the playback signal distorted wi=
thout
> +                * correct PLL parameters. Therefore we need to configure=
 PLL
> +                * again */
> +               ret =3D 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 =3D snd_soc_dai_set_pll(codec_dai, 0, 0, runtime->rat=
e,
> +                                         runtime->rate * 256);
> +               if (ret < 0)
> +                       dev_err(codec_dai->dev, "can't set FLL: %d\n", re=
t);
> +               break;
> +       }
>
>         return ret;
>  }
>
> -static const struct snd_soc_ops skylake_nau8825_ops =3D {
> -       .hw_params =3D skylake_nau8825_hw_params,
> +static struct snd_soc_ops skylake_nau8825_ops =3D {
> +       .trigger =3D skylake_nau8825_trigger,
>  };
>
>  static const unsigned int channels_dmic[] =3D {
> @@ -582,6 +604,7 @@ static struct snd_soc_dai_link skylake_dais[] =3D {
>                 .init =3D skylake_ssm4567_codec_init,
>                 .ignore_pmdown_time =3D 1,
>                 .be_hw_params_fixup =3D skylake_ssp_fixup,
> +               .ops =3D &skylake_nau8825_ops,
>                 .dpcm_playback =3D 1,
>                 .dpcm_capture =3D 1,
>                 SND_SOC_DAILINK_REG(ssp0_pin, ssp0_codec, platform),
> --
> 2.17.1
>