[PATCH] ASoC: da7219: remove SRM lock check retry

[PATCH] ASoC: da7219: remove SRM lock check retry

[Brent Lu]

For platforms not able to provide WCLK in the PREPARED runtime state, it
takes 400ms for codec driver to print the message "SRM failed to lock" in
the da7219_dai_event() function which is called when DAPM widgets are
powering up. The latency penalty to audio input/output is too much so the
retry (8 times) and delay (50ms each retry) are removed.

Another reason is current Cold output latency requirement in Android CDD is
500ms but will be reduced to 200ms for 2021 platforms. With the 400ms
latency it would be difficult to pass the Android CTS test.

Signed-off-by: Brent Lu <brent.lu@intel.com>
---
 sound/soc/codecs/da7219.c | 3 ++-
 sound/soc/codecs/da7219.h | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c
index f83a6ea..042e701 100644
--- a/sound/soc/codecs/da7219.c
+++ b/sound/soc/codecs/da7219.c
@@ -833,7 +833,8 @@ static int da7219_dai_event(struct snd_soc_dapm_widget *w,
 				srm_lock = true;
 			} else {
 				++i;
-				msleep(50);
+				if (i < DA7219_SRM_CHECK_RETRIES)
+					msleep(50);
 			}
 		} while ((i < DA7219_SRM_CHECK_RETRIES) && (!srm_lock));
 
diff --git a/sound/soc/codecs/da7219.h b/sound/soc/codecs/da7219.h
index 88b67fe..3149986 100644
--- a/sound/soc/codecs/da7219.h
+++ b/sound/soc/codecs/da7219.h
@@ -770,7 +770,7 @@
 #define DA7219_PLL_INDIV_36_TO_54_MHZ_VAL	16
 
 /* SRM */
-#define DA7219_SRM_CHECK_RETRIES	8
+#define DA7219_SRM_CHECK_RETRIES	1
 
 /* System Controller */
 #define DA7219_SYS_STAT_CHECK_RETRIES	6
-- 
2.7.4

RE: [PATCH] ASoC: da7219: remove SRM lock check retry

[Adam Thomson]

On 03 December 2019 07:31, Brent Lu wrote:

> For platforms not able to provide WCLK in the PREPARED runtime state, it
> takes 400ms for codec driver to print the message "SRM failed to lock" in
> the da7219_dai_event() function which is called when DAPM widgets are
> powering up. The latency penalty to audio input/output is too much so the
> retry (8 times) and delay (50ms each retry) are removed.
> 
> Another reason is current Cold output latency requirement in Android CDD is
> 500ms but will be reduced to 200ms for 2021 platforms. With the 400ms
> latency it would be difficult to pass the Android CTS test.

We can potentially reduce the timings here for something shorter although I'd
need to speak with the HW team as to what, if any reduction is feasible. However
this is not a real fix as there's potential for audible noises when you don't
enable WCLK first. As far as I can tell the Intel platforms are capable of
enabling clocks early, as can be seen in this board file with early SCLK enable:

https://elixir.bootlin.com/linux/latest/source/sound/soc/intel/boards/kbl_rt5663_max98927.c#L99

I think there's a need to find some method to enable the WCLK signal otherwise
there's the potential for audible artefacts when SRM finally locks which is not
going to be pleasant.

> 
> Signed-off-by: Brent Lu <brent.lu@intel.com>
> ---
>  sound/soc/codecs/da7219.c | 3 ++-
>  sound/soc/codecs/da7219.h | 2 +-
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c
> index f83a6ea..042e701 100644
> --- a/sound/soc/codecs/da7219.c
> +++ b/sound/soc/codecs/da7219.c
> @@ -833,7 +833,8 @@ static int da7219_dai_event(struct snd_soc_dapm_widget
> *w,
>  				srm_lock = true;
>  			} else {
>  				++i;
> -				msleep(50);
> +				if (i < DA7219_SRM_CHECK_RETRIES)
> +					msleep(50);
>  			}
>  		} while ((i < DA7219_SRM_CHECK_RETRIES) && (!srm_lock));
> 
> diff --git a/sound/soc/codecs/da7219.h b/sound/soc/codecs/da7219.h
> index 88b67fe..3149986 100644
> --- a/sound/soc/codecs/da7219.h
> +++ b/sound/soc/codecs/da7219.h
> @@ -770,7 +770,7 @@
>  #define DA7219_PLL_INDIV_36_TO_54_MHZ_VAL	16
> 
>  /* SRM */
> -#define DA7219_SRM_CHECK_RETRIES	8
> +#define DA7219_SRM_CHECK_RETRIES	1
> 
>  /* System Controller */
>  #define DA7219_SYS_STAT_CHECK_RETRIES	6
> --
> 2.7.4

RE: [PATCH] ASoC: da7219: remove SRM lock check retry

[Lu, Brent]

> 
> We can potentially reduce the timings here for something shorter although
> I'd need to speak with the HW team as to what, if any reduction is feasible.
> However this is not a real fix as there's potential for audible noises when you
> don't enable WCLK first. As far as I can tell the Intel platforms are capable of
> enabling clocks early, as can be seen in this board file with early SCLK enable:
> 
> https://elixir.bootlin.com/linux/latest/source/sound/soc/intel/boards/kbl_rt
> 5663_max98927.c#L99
> 
> I think there's a need to find some method to enable the WCLK signal
> otherwise there's the potential for audible artefacts when SRM finally locks
> which is not going to be pleasant.
> 

Hi Adam,

Thanks for reply. This patch is not fixing any bug. It just shorten the audio latency
on our boards. Basically we are idling there for 400ms then print a warning message
about SRM not being locked. It seems to me that 400ms is too much even for those
platforms which are able to provide WCLK before calling snd_soc_dai_set_pll()
function but it relies on your HW team to provide the number.

On KBL platform we have interface to control MCLK and I2S clocks like the link
you mentioned but WCLK seems not working on my board. I can try to ask if
someone is working on it but since we are moving to SOF. The chance is slim for
legacy firmware.


Regards,
Brent

RE: [PATCH] ASoC: da7219: remove SRM lock check retry

[Adam Thomson]

On 03 December 2019 10:33, Brent Lu wrote:

> > We can potentially reduce the timings here for something shorter although
> > I'd need to speak with the HW team as to what, if any reduction is feasible.
> > However this is not a real fix as there's potential for audible noises when you
> > don't enable WCLK first. As far as I can tell the Intel platforms are capable of
> > enabling clocks early, as can be seen in this board file with early SCLK enable:
> >
> > https://elixir.bootlin.com/linux/latest/source/sound/soc/intel/boards/kbl_rt
> > 5663_max98927.c#L99
> >
> > I think there's a need to find some method to enable the WCLK signal
> > otherwise there's the potential for audible artefacts when SRM finally locks
> > which is not going to be pleasant.
> >
> 
> Hi Adam,
> 
> Thanks for reply. This patch is not fixing any bug. It just shorten the audio latency
> on our boards. Basically we are idling there for 400ms then print a warning
> message
> about SRM not being locked. It seems to me that 400ms is too much even for
> those
> platforms which are able to provide WCLK before calling snd_soc_dai_set_pll()
> function but it relies on your HW team to provide the number.

But on platforms where they can enable the WCLK early they shouldn't be looping
around here for anything like 400ms. In an ideal world when that widget is
run SRM should hopefully be already locked but the code does allow for some
delay. Actually, having a long delay also helps show the user that something
isn't right here so I'm somewhat loathed to change this.

Even if you do reduce the retry timings what you're still not protecting
against is the possibility of audio artefacts when SRM finally locks. You want
this to lock before the any of the audio path is up so I think we need to find
a way to resolve that rather than relying on getting lucky with a smooth
power-up.

> 
> On KBL platform we have interface to control MCLK and I2S clocks like the link
> you mentioned but WCLK seems not working on my board. I can try to ask if
> someone is working on it but since we are moving to SOF. The chance is slim for
> legacy firmware.
> 
> 
> Regards,
> Brent

RE: [PATCH] ASoC: da7219: remove SRM lock check retry

[Lu, Brent]

> 
> But on platforms where they can enable the WCLK early they shouldn't be
> looping around here for anything like 400ms. In an ideal world when that
> widget is run SRM should hopefully be already locked but the code does
> allow for some delay. Actually, having a long delay also helps show the user
> that something isn't right here so I'm somewhat loathed to change this.
> 
> Even if you do reduce the retry timings what you're still not protecting
> against is the possibility of audio artefacts when SRM finally locks. You want
> this to lock before the any of the audio path is up so I think we need to find a
> way to resolve that rather than relying on getting lucky with a smooth power-
> up.
> 
Hi Adam,

Thanks for the explanation. So the purpose of the code is providing some
timing tolerance for SRM to lock? If so, I would suggest adding warning message
for the lock fail so people have a clue if their design fails the CTS test. Hard to say
if Google further reduces the Cold latency again in the future.


Regards,
Brent

RE: [PATCH] ASoC: da7219: remove SRM lock check retry

[Adam Thomson]

On 03 December 2019 14:36, Brent Lu wrote:

> > But on platforms where they can enable the WCLK early they shouldn't be
> > looping around here for anything like 400ms. In an ideal world when that
> > widget is run SRM should hopefully be already locked but the code does
> > allow for some delay. Actually, having a long delay also helps show the user
> > that something isn't right here so I'm somewhat loathed to change this.
> >
> > Even if you do reduce the retry timings what you're still not protecting
> > against is the possibility of audio artefacts when SRM finally locks. You want
> > this to lock before the any of the audio path is up so I think we need to find a
> > way to resolve that rather than relying on getting lucky with a smooth power-
> > up.
> >
> Hi Adam,
> 
> Thanks for the explanation. So the purpose of the code is providing some
> timing tolerance for SRM to lock? If so, I would suggest adding warning message
> for the lock fail so people have a clue if their design fails the CTS test. Hard to say
> if Google further reduces the Cold latency again in the future.

Yes, that's right. I have put in a request with our HW team to again clarify
timings, but still awaiting feedback.

The driver already warns via the kernel logs when SRM lock fails as follows:

	dev_warn(component->dev, "SRM failed to lock\n");

What else do you think is needed?

> 
> 
> Regards,
> Brent

RE: [PATCH] ASoC: da7219: remove SRM lock check retry

[Lu, Brent]

> 
> Yes, that's right. I have put in a request with our HW team to again clarify
> timings, but still awaiting feedback.
> 
> The driver already warns via the kernel logs when SRM lock fails as follows:
> 
> 	dev_warn(component->dev, "SRM failed to lock\n");
> 
> What else do you think is needed?
> 

Hi Adam,

Let's say that the SRM locks in the second loop. The 50ms delay was applied
but there is no kernel log message about it because the value of srm_lock is
already true when exiting the loop. If we can print every SRM lock fail before
msleep() call, it would be a helpful for people resolving timing issues like Cold
latency.

do {
	pll_status = snd_soc_component_read32(component, DA7219_PLL_SRM_STS);
	if (pll_status & DA7219_PLL_SRM_STS_SRM_LOCK) {
		break;
	} else {
		++i;
		dev_warn(component->dev, "SRM failed to lock, retry in 50ms\n");
		msleep(50);
	}
} while (i < DA7219_SRM_CHECK_RETRIES);


Regards,
Brent

RE: [PATCH] ASoC: da7219: remove SRM lock check retry

[Adam Thomson]

On 03 December 2019 15:23, Brent Lu wrote:

> > Yes, that's right. I have put in a request with our HW team to again clarify
> > timings, but still awaiting feedback.
> >
> > The driver already warns via the kernel logs when SRM lock fails as follows:
> >
> > 	dev_warn(component->dev, "SRM failed to lock\n");
> >
> > What else do you think is needed?
> >
> 
> Hi Adam,
> 
> Let's say that the SRM locks in the second loop. The 50ms delay was applied
> but there is no kernel log message about it because the value of srm_lock is
> already true when exiting the loop. If we can print every SRM lock fail before
> msleep() call, it would be a helpful for people resolving timing issues like Cold
> latency.
> 
> do {
> 	pll_status = snd_soc_component_read32(component,
> DA7219_PLL_SRM_STS);
> 	if (pll_status & DA7219_PLL_SRM_STS_SRM_LOCK) {
> 		break;
> 	} else {
> 		++i;
> 		dev_warn(component->dev, "SRM failed to lock, retry in
> 50ms\n");
> 		msleep(50);
> 	}
> } while (i < DA7219_SRM_CHECK_RETRIES);

I have no real problem in providing debug like this, although this is probably
dev_info() rather than dev_warn(). Also I'd suggest the debug message should be
something like the following if we were to add anything here:

	dev_info(component->dev, "Waiting for SRM lock\n");

Timings can be ascertained from the kernel log (assuming timestamping is on) so
I don't think we need to explicitly state the delay information.

> 
> 
> Regards,
> Brent

Re: [PATCH] ASoC: da7219: remove SRM lock check retry

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1076 bytes --]

On Tue, Dec 03, 2019 at 03:31:05PM +0800, Brent Lu wrote:

> For platforms not able to provide WCLK in the PREPARED runtime state, it
> takes 400ms for codec driver to print the message "SRM failed to lock" in
> the da7219_dai_event() function which is called when DAPM widgets are
> powering up. The latency penalty to audio input/output is too much so the
> retry (8 times) and delay (50ms each retry) are removed.

> Another reason is current Cold output latency requirement in Android CDD is
> 500ms but will be reduced to 200ms for 2021 platforms. With the 400ms
> latency it would be difficult to pass the Android CTS test.

Adam pointed out some specific problems this causes here but at a
higher level this commit message isn't great since it just says
"I don't like these delays so I made them shorter" which doesn't
really explain what the delays are doing or why it's OK to make
them shorter - there are plenty of audio devices which require
longer ramp times than people would like out there but there's
usually good solid reasons why the delays have to be there.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]