[PATCH v2] ASoC: fsl: sai: Fix underrrun for playback stream start

[PATCH v2] ASoC: fsl: sai: Fix underrrun for playback stream start

[Daniel Baluta]

From: Shengjiu Wang <shengjiu.wang@nxp.com>

For [dir=playback, cmd = PCM_TRIGGER_START], fsl_sai_trigger
does the following:
	* Enable DMA request
	* Enable Transmitter

Now, because DMA doesn't copy data to TDR fast enough we hit
TX FIFO underrun error:

[   37.175974] fsl-sai 308b0000.sai: isr: Transmit underrun detected

The explanation for this can be found in SAI documentation for example
chapter 16.16.3.5.2 FIFO pointers of the i.MX8DXP reference manual [1]

"
If the Transmit FIFO is empty, then to avoid a FIFO underrun,
the Transmit Data Register must be written at least 3 bit clocks
before the start of the next unmasked word. Before enabling the
transmitter, the Transmit FIFO should be initialized with data
(since after the transmitter is enabled, the transmitter will
start a new frame, and if no data is in the FIFO, then the
transmitter will immediately give an error).
"

[1] https://www.nxp.com/docs/en/reference-manual/IMX8DQXPRM.pdf

Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
---
Changes since v1:
 - as per Fabio's request clarify the issue and the solution
 - move words writing before enabling DMA request

 sound/soc/fsl/fsl_sai.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
index 2d376e3a3c80..a739baa99c1a 100644
--- a/sound/soc/fsl/fsl_sai.c
+++ b/sound/soc/fsl/fsl_sai.c
@@ -522,7 +522,9 @@ static int fsl_sai_trigger(struct snd_pcm_substream *substream, int cmd,
 {
 	struct fsl_sai *sai = snd_soc_dai_get_drvdata(cpu_dai);
 	bool tx = substream->stream == SNDRV_PCM_STREAM_PLAYBACK;
+	unsigned int channels = substream->runtime->channels;
 	u32 xcsr, count = 100;
+	int i;
 
 	/*
 	 * Asynchronous mode: Clear SYNC for both Tx and Rx.
@@ -542,6 +544,11 @@ static int fsl_sai_trigger(struct snd_pcm_substream *substream, int cmd,
 	case SNDRV_PCM_TRIGGER_START:
 	case SNDRV_PCM_TRIGGER_RESUME:
 	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
+		for (i = 0; tx && i < channels; i++)
+			regmap_write(sai->regmap, FSL_SAI_TDR, 0x0);
+		if (tx)
+			udelay(10);
+
 		regmap_update_bits(sai->regmap, FSL_SAI_xCSR(tx),
 				   FSL_SAI_CSR_FRDE, FSL_SAI_CSR_FRDE);
 
-- 
2.17.1

Re: [PATCH v2] ASoC: fsl: sai: Fix underrrun for playback stream start

[Nicolin Chen]

On Fri, Mar 08, 2019 at 05:39:30PM +0000, Daniel Baluta wrote:

> @@ -542,6 +544,11 @@ static int fsl_sai_trigger(struct snd_pcm_substream *substream, int cmd,
>  	case SNDRV_PCM_TRIGGER_START:
>  	case SNDRV_PCM_TRIGGER_RESUME:
>  	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> +		for (i = 0; tx && i < channels; i++)
> +			regmap_write(sai->regmap, FSL_SAI_TDR, 0x0);

Though it could reduce underrrun for sure, I don't feel it's a
thorough fix, especially when dealing with a duplex case where
RX is the first stream while the driver enables both TE and RE
at the same time. So the transmitter might have started before
TX DMA request gets enabled.

We actually have something similar in the ESAI driver as that
one is well documented. But SAI is apparently more tricky at
the trigger() function. And the paragraph you pasted from the
RM doesn't explicitly sound like we should do this.

Btw, Instead of writing dummy data, have you tried polling the
WFP pointer of the TFR0 register to make sure data is copied
by the DMA?

> +		if (tx)
> +			udelay(10);

Any justification for the delay? Missing a line of comments? I
guess it's to address the 3-bit clocks timing. However, putting
it before enabling DMA request doesn't make much sense to me.

> +
>  		regmap_update_bits(sai->regmap, FSL_SAI_xCSR(tx),
>  				   FSL_SAI_CSR_FRDE, FSL_SAI_CSR_FRDE);
>  
> -- 
> 2.17.1
> 

Re: [PATCH v2] ASoC: fsl: sai: Fix underrrun for playback stream start

[Daniel Baluta]

On Fri, Mar 8, 2019 at 10:59 PM Nicolin Chen <nicoleotsuka@gmail.com> wrote:
>
> On Fri, Mar 08, 2019 at 05:39:30PM +0000, Daniel Baluta wrote:
>
> > @@ -542,6 +544,11 @@ static int fsl_sai_trigger(struct snd_pcm_substream *substream, int cmd,
> >       case SNDRV_PCM_TRIGGER_START:
> >       case SNDRV_PCM_TRIGGER_RESUME:
> >       case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> > +             for (i = 0; tx && i < channels; i++)
> > +                     regmap_write(sai->regmap, FSL_SAI_TDR, 0x0);
>
> Though it could reduce underrrun for sure, I don't feel it's a
> thorough fix, especially when dealing with a duplex case where
> RX is the first stream while the driver enables both TE and RE
> at the same time. So the transmitter might have started before
> TX DMA request gets enabled.

I see your point here. This is trickier than I expected.
>
> We actually have something similar in the ESAI driver as that
> one is well documented. But SAI is apparently more tricky at
> the trigger() function. And the paragraph you pasted from the
> RM doesn't explicitly sound like we should do this.
>
> Btw, Instead of writing dummy data, have you tried polling the
> WFP pointer of the TFR0 register to make sure data is copied
> by the DMA?

This is a good suggestion. Will give it a try.

>
> > +             if (tx)
> > +                     udelay(10);
>
> Any justification for the delay? Missing a line of comments? I
> guess it's to address the 3-bit clocks timing. However, putting
> it before enabling DMA request doesn't make much sense to me.

Indeed. Will come back with a better patch in v3.

thanks,
Daniel.