| Message ID | 1581322611-25695-1-git-send-email-brent.lu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: da7219: check SRM lock in trigger callback | expand |
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 |\ >
On 10 February 2020 08:17, 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. Could ensure? This change seems specific to Intel DSP based systems, at least from the description. Having looked through the core, the trigger code for a codec is seemingly always called before the trigger for the CPU. How will this work for other platforms, assuming their clocks are enabled in the CPU DAI trigger function by default? Can we always guarantee the CPU side isn't going to send anything other than 0s until after SRM has locked? > > 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); I was under the impression that 'trigger()' was atomic? We'd have to have some kind of workqueue to do all of this, which means we'd almost certainly lose some PCM data at the start of a stream. > + 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 |\ > -- > 2.7.4
+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 >
On 2/10/20 9:44 AM, Curtis Malainey wrote: > +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 Ah, thanks for the pointer, but this still does not tell me if it's a GLK-only issue or not. And if yes, there are alternate proposals discussed in that issue #2124, which has been idle since November 25. What I am really worried is side effects on unrelated platforms.
+Fletcher Woodruff <fletcherw@google.com> See comment #3 in the bug. This is not a GLK specific issue. If I remember correctly Fletcher found that this was occurring on 2-3 platforms. SST has the ability to turn on clocks on demand which is why this has not been an issue previously from what I understand on the bug. And that is a fair point, we do need to consider other users of the codec. On Mon, Feb 10, 2020 at 8:07 AM Pierre-Louis Bossart < pierre-louis.bossart@linux.intel.com> wrote: > > > On 2/10/20 9:44 AM, Curtis Malainey wrote: > > +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 > > Ah, thanks for the pointer, but this still does not tell me if it's a > GLK-only issue or not. And if yes, there are alternate proposals > discussed in that issue #2124, which has been idle since November 25. > > What I am really worried is side effects on unrelated platforms. >
On Mon, Feb 10, 2020 at 04:16:51PM +0800, 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. Independently of any other discussion trigger is expected to run very fast so doesn't feel like a good place to do this - given that we're talking about doing this to avoid noise the mute operation seems like a more idiomatic place to do this, it exists to avoid playing back glitches from the digitial interface during startup.
On Tue, Feb 11, 2020 at 1:18 AM Curtis Malainey <cujomalainey@google.com> wrote: > > +Fletcher Woodruff > See comment #3 in the bug. This is not a GLK specific issue. If I remember correctly Fletcher found that this was occurring on 2-3 platforms. Yes, I tested this and saw the same bug on a Pixelbook Go which I believe is Comet Lake. >SST has the ability to turn on clocks on demand which is why this has not been an issue previously from what I understand on the bug. > > And that is a fair point, we do need to consider other users of the codec. > > > On Mon, Feb 10, 2020 at 8:07 AM Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> wrote: >> >> >> >> On 2/10/20 9:44 AM, Curtis Malainey wrote: >> > +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 >> >> Ah, thanks for the pointer, but this still does not tell me if it's a >> GLK-only issue or not. And if yes, there are alternate proposals >> discussed in that issue #2124, which has been idle since November 25. >> >> What I am really worried is side effects on unrelated platforms.
> > Could ensure? This change seems specific to Intel DSP based systems, at > least from the description. Having looked through the core, the trigger code > for a codec is seemingly always called before the trigger for the CPU. How will > this work for other platforms, assuming their clocks are enabled in the CPU > DAI trigger function by default? > > Can we always guarantee the CPU side isn't going to send anything other > than 0s until after SRM has locked? > I think the patch is for those systems which enable I2S clocks in pcm_start instead of pcm_prepare. It has no effect on systems already be able to turn on clocks in supply widgets or set_bias_level() function. If the trigger type in the DAI link is TRIGGER_PRE, then the trigger function of FE port (component or CPU DAI) will be called before codec driver's trigger function. In this case we will be able to turn on the clock in time. However, if the trigger type is TRIGGER_POST, then the patch does not help because just like what you said, codec driver's trigger function is called first. In my experiment with the patch, the SRM is locked in the second read and cost 50ms to wait. > > I was under the impression that 'trigger()' was atomic? We'd have to have > some kind of workqueue to do all of this, which means we'd almost certainly > lose some PCM data at the start of a stream. >
> > Independently of any other discussion trigger is expected to run very fast so > doesn't feel like a good place to do this - given that we're talking about doing > this to avoid noise the mute operation seems like a more idiomatic place to > do this, it exists to avoid playing back glitches from the digitial interface > during startup. It still take 50ms waiting for lock on in the trigger so I guess it's not a good implementation here. And I thought digital mute is called in the pcm_prepare? I'm afraid it does not work in our case... Regards, Brent
>> Could ensure? This change seems specific to Intel DSP based systems, at >> least from the description. Having looked through the core, the trigger code >> for a codec is seemingly always called before the trigger for the CPU. How will >> this work for other platforms, assuming their clocks are enabled in the CPU >> DAI trigger function by default? >> >> Can we always guarantee the CPU side isn't going to send anything other >> than 0s until after SRM has locked? Not with the default mode for the SSP, all clocks are enabled at trigger time. We can enable the MCLK and BCLK ahead of time (which would require a change in firmware). But if we want to enable the FSYNC (word-clock), then we have to trigger DMA transfers with pretend-buffers, this is a lot more invasive. So just to be clear, which of the MCLK, BCLK, FSYNC do you need enabled? > I think the patch is for those systems which enable I2S clocks in pcm_start instead > of pcm_prepare. It has no effect on systems already be able to turn on clocks in > supply widgets or set_bias_level() function. > > If the trigger type in the DAI link is TRIGGER_PRE, then the trigger function of FE port > (component or CPU DAI) will be called before codec driver's trigger function. In this > case we will be able to turn on the clock in time. However, if the trigger type is > TRIGGER_POST, then the patch does not help because just like what you said, codec > driver's trigger function is called first. IIRC we recently did a change to deal with underflows. Ranjani, can you remind us what the issue was? Thanks -Pierre
> > > > I think the patch is for those systems which enable I2S clocks in > pcm_start instead > > of pcm_prepare. It has no effect on systems already be able to turn on > clocks in > > supply widgets or set_bias_level() function. > > > > If the trigger type in the DAI link is TRIGGER_PRE, then the trigger > function of FE port > > (component or CPU DAI) will be called before codec driver's trigger > function. In this > > case we will be able to turn on the clock in time. However, if the > trigger type is > > TRIGGER_POST, then the patch does not help because just like what you > said, codec > > driver's trigger function is called first. > > IIRC we recently did a change to deal with underflows. Ranjani, can you > remind us what the issue was? Hi Pierre, Are you talking about the change in this commit acbf27746ecfa96b "ASoC: pcm: update FE/BE trigger order based on the command"? We made this change to handle xruns during pause/release particularly on the Intel HDA platforms. Thanks, Ranjani
On 2/11/20 2:37 PM, Sridharan, Ranjani wrote: > > > I think the patch is for those systems which enable I2S clocks in > pcm_start instead > > of pcm_prepare. It has no effect on systems already be able to > turn on clocks in > > supply widgets or set_bias_level() function. > > > > If the trigger type in the DAI link is TRIGGER_PRE, then the > trigger function of FE port > > (component or CPU DAI) will be called before codec driver's > trigger function. In this > > case we will be able to turn on the clock in time. However, if > the trigger type is > > TRIGGER_POST, then the patch does not help because just like what > you said, codec > > driver's trigger function is called first. > > IIRC we recently did a change to deal with underflows. Ranjani, can you > remind us what the issue was? > > Hi Pierre, > > Are you talking about the change in this commit acbf27746ecfa96b > "ASoC: pcm: update FE/BE trigger order based on the command"? > > We made this change to handle xruns during pause/release particularly on > the Intel HDA platforms. this change was just to mirror the behavior between start/stop, I thought there was a patch where we moved to TRIGGER_POST by default? What I am trying to figure out if whether using TRIGGER_PRE is ok or not for the SOF firmware. Thanks! -Pierre
> > > > > > Hi Pierre, > > > > Are you talking about the change in this commit acbf27746ecfa96b > > "ASoC: pcm: update FE/BE trigger order based on the command"? > > > > We made this change to handle xruns during pause/release particularly on > > the Intel HDA platforms. > > this change was just to mirror the behavior between start/stop, I > thought there was a patch where we moved to TRIGGER_POST by default? > > What I am trying to figure out if whether using TRIGGER_PRE is ok or not > for the SOF firmware. > Ahh yes, it was part of the same series as this one. fd274c2b7267b "ASoC: SOF: topology: set trigger order for FE DAI link" TRIGGER_PRE won't really work in the case of SOF. We need the BE DAI to be triggered before the FE DAI during start to prevent the xruns during pause/release. Thanks, Ranjani
>>> Are you talking about the change in this commit acbf27746ecfa96b >>> "ASoC: pcm: update FE/BE trigger order based on the command"? >>> >>> We made this change to handle xruns during pause/release particularly on >>> the Intel HDA platforms. >> >> this change was just to mirror the behavior between start/stop, I >> thought there was a patch where we moved to TRIGGER_POST by default? >> >> What I am trying to figure out if whether using TRIGGER_PRE is ok or not >> for the SOF firmware. >> > > Ahh yes, it was part of the same series as this one. fd274c2b7267b "ASoC: > SOF: topology: set trigger order for FE DAI link" > > TRIGGER_PRE won't really work in the case of SOF. We need the BE DAI to be > triggered before the FE DAI during start to prevent the xruns during > pause/release. Thanks Ranjani. That information closes the door on the idea of playing with the trigger order suggested earlier in the thread, so my guess is that we really need to expose the MCLK/BCLK with the clk API and turn them on/off from the machine driver as needed. I hope is that we don't need the FSYNC as well, that would be rather painful to implement.
On 11 February 2020 21:49, Pierre-Louis Bossart wrote: > -----Original Message----- > From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > Sent: 11 February 2020 21:49 > To: Sridharan, Ranjani <ranjani.sridharan@intel.com> > Cc: alsa-devel@alsa-project.org; Support Opensource > <Support.Opensource@diasemi.com>; Takashi Iwai <tiwai@suse.com>; Liam > Girdwood <lgirdwood@gmail.com>; linux-kernel@vger.kernel.org; Chiang, Mac > <mac.chiang@intel.com>; Mark Brown <broonie@kernel.org>; Ranjani Sridharan > <ranjani.sridharan@linux.intel.com>; Adam Thomson > <Adam.Thomson.Opensource@diasemi.com>; Lu, Brent <brent.lu@intel.com>; > cychiang@google.com > Subject: Re: [alsa-devel] [PATCH] ASoC: da7219: check SRM lock in trigger callback > Importance: High > > > > >>> Are you talking about the change in this commit acbf27746ecfa96b > >>> "ASoC: pcm: update FE/BE trigger order based on the command"? > >>> > >>> We made this change to handle xruns during pause/release particularly on > >>> the Intel HDA platforms. > >> > >> this change was just to mirror the behavior between start/stop, I > >> thought there was a patch where we moved to TRIGGER_POST by default? > >> > >> What I am trying to figure out if whether using TRIGGER_PRE is ok or not > >> for the SOF firmware. > >> > > > > Ahh yes, it was part of the same series as this one. fd274c2b7267b "ASoC: > > SOF: topology: set trigger order for FE DAI link" > > > > TRIGGER_PRE won't really work in the case of SOF. We need the BE DAI to be > > triggered before the FE DAI during start to prevent the xruns during > > pause/release. > > Thanks Ranjani. That information closes the door on the idea of playing > with the trigger order suggested earlier in the thread, so my guess is > that we really need to expose the MCLK/BCLK with the clk API and turn > them on/off from the machine driver as needed. I hope is that we don't > need the FSYNC as well, that would be rather painful to implement. Am not going to make myself popular here. It's MCLK and FSYNC (or WCLK as it's termed for our device) that is required for SRM to lock in the PLL. So far I've not found a way in the codec driver to be able to get around this. I spent a very long time with Sathya in the early days (Apollo Lake) looking at options but nothing would fit which is why I have the solution that's in place right now. We could probably reduce the number of rechecks before timeout in the driver but that's really just papering over the crack and there's still the possibility of noise later when SRM finally does lock.
On Wed, Feb 12, 2020 at 10:16:54AM +0000, Adam Thomson wrote: > So far I've not found a way in the codec driver to be able to get around this. > I spent a very long time with Sathya in the early days (Apollo Lake) looking at > options but nothing would fit which is why I have the solution that's in place > right now. We could probably reduce the number of rechecks before timeout in the > driver but that's really just papering over the crack and there's still the > possibility of noise later when SRM finally does lock. This really needs the componentisation refactoring I think, that way we can annotate individual devices and links with what they need rather than essentially guessing about what works most of the time which is more or less what we do at the minute. Like you say as things are at the minute there's a lot of crack papering going on.
>> Thanks Ranjani. That information closes the door on the idea of playing >> with the trigger order suggested earlier in the thread, so my guess is >> that we really need to expose the MCLK/BCLK with the clk API and turn >> them on/off from the machine driver as needed. I hope is that we don't >> need the FSYNC as well, that would be rather painful to implement. > > Am not going to make myself popular here. It's MCLK and FSYNC (or WCLK as it's > termed for our device) that is required for SRM to lock in the PLL. > > So far I've not found a way in the codec driver to be able to get around this. > I spent a very long time with Sathya in the early days (Apollo Lake) looking at > options but nothing would fit which is why I have the solution that's in place > right now. We could probably reduce the number of rechecks before timeout in the > driver but that's really just papering over the crack and there's still the > possibility of noise later when SRM finally does lock. Sorry, you lost me at "the solution that's in place right now". There is nothing in the bxt_da7219_max98357a.c code that deals with clocks or defines a trigger order?
On 12 February 2020 15:48, Pierre-Louis Bossart wrote: > >> Thanks Ranjani. That information closes the door on the idea of playing > >> with the trigger order suggested earlier in the thread, so my guess is > >> that we really need to expose the MCLK/BCLK with the clk API and turn > >> them on/off from the machine driver as needed. I hope is that we don't > >> need the FSYNC as well, that would be rather painful to implement. > > > > Am not going to make myself popular here. It's MCLK and FSYNC (or WCLK as > it's > > termed for our device) that is required for SRM to lock in the PLL. > > > > So far I've not found a way in the codec driver to be able to get around this. > > I spent a very long time with Sathya in the early days (Apollo Lake) looking at > > options but nothing would fit which is why I have the solution that's in place > > right now. We could probably reduce the number of rechecks before timeout in > the > > driver but that's really just papering over the crack and there's still the > > possibility of noise later when SRM finally does lock. > > Sorry, you lost me at "the solution that's in place right now". There is > nothing in the bxt_da7219_max98357a.c code that deals with clocks or > defines a trigger order? I meant solution in the context of the codec driver. The approach or expectation (maybe more suitable wording) for the driver is that the required clocking can be enabled prior to the DAI widget for the codec being powered via DAPM as part of an audio path. This approach has been there since the beginning, for want of a better option, and I've always highlighted this need when platforms are using our device with SRM. You're right though that this isn't taken care of in existing mainline Linux machine files that use this device with SRM.
> > Am not going to make myself popular here. It's MCLK and FSYNC (or WCLK as > it's termed for our device) that is required for SRM to lock in the PLL. > > So far I've not found a way in the codec driver to be able to get around this. > I spent a very long time with Sathya in the early days (Apollo Lake) looking at > options but nothing would fit which is why I have the solution that's in place > right now. We could probably reduce the number of rechecks before > timeout in the driver but that's really just papering over the crack and there's > still the possibility of noise later when SRM finally does lock. Hi Adam, For Google CTS requirement (200ms cold output latency), we plan to upload a patch which reduces the recheck number to 4 and interval to 20ms so the total delay here would be 80ms for our platform. We think the time is still sufficient for other platforms to generate a stable WCLK and for the codec SRM to lock but still needs your confirmation. How do you think? Regards, Brent
On 19 February 2020 05:57, Lu, Brent wrote: > > Am not going to make myself popular here. It's MCLK and FSYNC (or WCLK as > > it's termed for our device) that is required for SRM to lock in the PLL. > > > > So far I've not found a way in the codec driver to be able to get around this. > > I spent a very long time with Sathya in the early days (Apollo Lake) looking at > > options but nothing would fit which is why I have the solution that's in place > > right now. We could probably reduce the number of rechecks before > > timeout in the driver but that's really just papering over the crack and there's > > still the possibility of noise later when SRM finally does lock. > > Hi Adam, > > For Google CTS requirement (200ms cold output latency), we plan to upload a > patch which reduces the recheck number to 4 and interval to 20ms so the total > delay here would be 80ms for our platform. We think the time is still sufficient > for other platforms to generate a stable WCLK and for the codec SRM to lock but > still needs your confirmation. How do you think? Hi Brent, I'm concerned that just setting a timeout to suit the Google CTS requirement isn't necessarily suitable for all targets, and this doesn't actually fix the real problem here. How long do you determine platforms will take to generate a stable WCLK? Do we have an idea of how long that might be in a worst case scenario? If so then we can look at adjusting this down, but I'd like to be clear. > > > Regards, > Brent
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 |\
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. Signed-off-by: Brent Lu <brent.lu@intel.com> --- sound/soc/codecs/da7219.c | 68 +++++++++++++++++++++++++++++++---------------- 1 file changed, 45 insertions(+), 23 deletions(-)