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 <[email protected]>
---
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 |\
--
2.7.4
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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 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.
>
> 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