Re: [alsa-devel] [PATCH] ASoC: da7219: check SRM lock in trigger callback

From: Curtis Malainey <cujomalainey@google.com>
To: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Cc: ALSA development <alsa-devel@alsa-project.org>,
	Support Opensource <support.opensource@diasemi.com>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Liam Girdwood <lgirdwood@gmail.com>,
	Takashi Iwai <tiwai@suse.com>,
	"Chiang, Mac" <mac.chiang@intel.com>,
	Mark Brown <broonie@kernel.org>, Brent Lu <brent.lu@intel.com>,
	Jimmy Cheng-Yi Chiang <cychiang@google.com>
Subject: Re: [alsa-devel] [PATCH] ASoC: da7219: check SRM lock in trigger callback
Date: Mon, 10 Feb 2020 07:44:15 -0800	[thread overview]
Message-ID: <CAOReqxhHfTuj6mxeX2e_ejMY8N4u+BFLfzKDgn=y5EbWLL_joA@mail.gmail.com> (raw)
In-Reply-To: <00ed82c4-404a-ec70-970e-56ddce9285ae@linux.intel.com>

+Jimmy Cheng-Yi Chiang <cychiang@google.com>

This error is causing pcm_open commands to fail timing requirements,
sometimes taking +500ms to open the PCM as a result. This work around is
required so we can meet the timing requirements. The bug is explained in
detail here https://github.com/thesofproject/sof/issues/2124

On Mon, Feb 10, 2020 at 6:44 AM Pierre-Louis Bossart <
pierre-louis.bossart@linux.intel.com> wrote:

>
>
> On 2/10/20 2:16 AM, Brent Lu wrote:
> > Intel sst firmware turns on BCLK/WCLK in START Ioctl call which timing is
> > later than the DAPM SUPPLY event handler da7219_dai_event is called (in
> > PREPARED state). Therefore, the SRM lock check always fail.
> >
> > Moving the check to trigger callback could ensure the SRM is locked
> before
> > DSP starts to process data and avoid possisble noise.
>
> This codec is used quite a bit by Chromebooks across multiple
> generations and with both SST and SOF drivers, we need to be careful
> about changes.
> I am personally not aware of any issues and never saw an 'SRM failed to
> lock message'. On which platform did you see a problem?
>
> >
> > Signed-off-by: Brent Lu <brent.lu@intel.com>
> > ---
> >   sound/soc/codecs/da7219.c | 68
> +++++++++++++++++++++++++++++++----------------
> >   1 file changed, 45 insertions(+), 23 deletions(-)
> >
> > diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c
> > index f83a6ea..0fb5ea5 100644
> > --- a/sound/soc/codecs/da7219.c
> > +++ b/sound/soc/codecs/da7219.c
> > @@ -794,9 +794,7 @@ static int da7219_dai_event(struct
> snd_soc_dapm_widget *w,
> >       struct snd_soc_component *component =
> snd_soc_dapm_to_component(w->dapm);
> >       struct da7219_priv *da7219 =
> snd_soc_component_get_drvdata(component);
> >       struct clk *bclk = da7219->dai_clks[DA7219_DAI_BCLK_IDX];
> > -     u8 pll_ctrl, pll_status;
> > -     int i = 0, ret;
> > -     bool srm_lock = false;
> > +     int ret;
> >
> >       switch (event) {
> >       case SND_SOC_DAPM_PRE_PMU:
> > @@ -820,26 +818,6 @@ static int da7219_dai_event(struct
> snd_soc_dapm_widget *w,
> >               /* PC synchronised to DAI */
> >               snd_soc_component_update_bits(component, DA7219_PC_COUNT,
> >                                   DA7219_PC_FREERUN_MASK, 0);
> > -
> > -             /* Slave mode, if SRM not enabled no need for status
> checks */
> > -             pll_ctrl = snd_soc_component_read32(component,
> DA7219_PLL_CTRL);
> > -             if ((pll_ctrl & DA7219_PLL_MODE_MASK) !=
> DA7219_PLL_MODE_SRM)
> > -                     return 0;
> > -
> > -             /* Check SRM has locked */
> > -             do {
> > -                     pll_status = snd_soc_component_read32(component,
> DA7219_PLL_SRM_STS);
> > -                     if (pll_status & DA7219_PLL_SRM_STS_SRM_LOCK) {
> > -                             srm_lock = true;
> > -                     } else {
> > -                             ++i;
> > -                             msleep(50);
> > -                     }
> > -             } while ((i < DA7219_SRM_CHECK_RETRIES) && (!srm_lock));
> > -
> > -             if (!srm_lock)
> > -                     dev_warn(component->dev, "SRM failed to lock\n");
> > -
> >               return 0;
> >       case SND_SOC_DAPM_POST_PMD:
> >               /* PC free-running */
> > @@ -1658,12 +1636,56 @@ static int da7219_hw_params(struct
> snd_pcm_substream *substream,
> >       return 0;
> >   }
> >
> > +static int da7219_set_dai_trigger(struct snd_pcm_substream *substream,
> int cmd,
> > +                               struct snd_soc_dai *dai)
> > +{
> > +     struct snd_soc_component *component = dai->component;
> > +     u8 pll_ctrl, pll_status;
> > +     int i = 0;
> > +     bool srm_lock = false;
> > +
> > +     switch (cmd) {
> > +     case SNDRV_PCM_TRIGGER_START:
> > +             /* Slave mode, if SRM not enabled no need for status
> checks */
> > +             pll_ctrl = snd_soc_component_read32(component,
> DA7219_PLL_CTRL);
> > +             if ((pll_ctrl & DA7219_PLL_MODE_MASK) !=
> DA7219_PLL_MODE_SRM)
> > +                     return 0;
> > +
> > +             /* Check SRM has locked */
> > +             do {
> > +                     pll_status = snd_soc_component_read32(component,
> > +
>  DA7219_PLL_SRM_STS);
> > +                     if (pll_status & DA7219_PLL_SRM_STS_SRM_LOCK) {
> > +                             srm_lock = true;
> > +                     } else {
> > +                             ++i;
> > +                             msleep(50);
> > +                     }
> > +             } while ((i < DA7219_SRM_CHECK_RETRIES) && (!srm_lock));
> > +
> > +             if (!srm_lock)
> > +                     dev_warn(component->dev, "SRM failed to lock\n");
> > +
> > +             break;
> > +     case SNDRV_PCM_TRIGGER_RESUME:
> > +     case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> > +     case SNDRV_PCM_TRIGGER_STOP:
> > +     case SNDRV_PCM_TRIGGER_SUSPEND:
> > +     case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
> > +     default:
> > +             break;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> >   static const struct snd_soc_dai_ops da7219_dai_ops = {
> >       .hw_params      = da7219_hw_params,
> >       .set_sysclk     = da7219_set_dai_sysclk,
> >       .set_pll        = da7219_set_dai_pll,
> >       .set_fmt        = da7219_set_dai_fmt,
> >       .set_tdm_slot   = da7219_set_dai_tdm_slot,
> > +     .trigger        = da7219_set_dai_trigger,
> >   };
> >
> >   #define DA7219_FORMATS (SNDRV_PCM_FMTBIT_S16_LE |
> SNDRV_PCM_FMTBIT_S20_3LE |\
> >
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel