[PATCH 0/5] ASoC: fsl_ssi: Fixing various channel slips and bad samples insertions

[PATCH 0/5] ASoC: fsl_ssi: Fixing various channel slips and bad samples insertions

[Arnaud Mouiche]

This series of patch is an attempt to fix the fsl_ssi driver to use it in TDM mode (DSP A or B) with a large number of channels/slots.

Bugs are detected and fixed on a imx6sl platform with linux 4.3, where 2 SSI interfaces are used. SSI3 configured as a master of the bus, SSI2 as a slave.

Various loopback scenario are tested:
* scenario 1: SSI3 (master) with TXD to RXD loopback

	         |
	         |---> TXD  --\
	SSI3     |<--- RXD  --/
	(master) |---> SYNC
	         |---> BCLK
	         |
	
* scenario 2: SSI3 connected to SSI2
	         |
	         |---> TXD  --------\
	SSI3     |<--- RXD  -----\  |
	(master) |---> SYNC --\  |  |
	         |---> BCLK   |  |  |
	         |       |    |  |  |
	         |       |    |  |  |
	         |<--- BCLK   |  |  |
	SSI2     |<--- SYNC --/  |  |
	(slave)  |---> TXD ------/  |
	         |<--- RXD  --------/
	         |



A test software (called atest) was developed and available. [1]
It basically generate/check specials frames with S16_LE sequence samples
       NNN0, NNN1, NNN2 ... NNNC     with NNN = frame number, and C the number of channels-1



Patch 1: 
	Limitation fix. Prerequisite to use the fsl_ssi driver with up to 32 channels / slots

Patch 2: 
	Bug fix. Prerequisite to setup a relative high bitclk for our tests in 8ch / 16bits / 48kHz

Patch 3:
	Simply save a 'dev' reference inside ssi_private for dev_err() purpose.
	(ssi_private is only available in lot of places, and 
	 ssi_private->pdev is deprecated and NULL indeed)

Patch 4: 
	Fix playback samples being dropped because the TX fifo was not ready at 
	the time the DMA starts filling (only in case where playback is started AFTER the capture)

	Detected in loopback scenario 1, with following script (> 80 % of reproducibility)
	$ atest -D SSI3 -c 8 -r 48000 capture play
	dbg: set period size: 960
	dbg: SSI3: capture_start
	dbg: SSI3: playback_start
	err: invalid frame after 1 null frames
	err:   0000 0000 0013 0014 0015 0016 0017 0020 
	dbg: SIGINT
	total number of sequence errors: 21119
	
	here we see that samples 0000 0001 ... 0012 are not present in the output.
	In addition, this the number of samples dropped is not a multiple of 
	the number of channel, we have a a channel slip.


Patch 5: 
	This is the Caleb Crome's case [2], where the SSI starts to generate samples while 
	the TX FIFO is not filled by the DMA yet. Void samples can be inserted before DMA streaming.

	Detected in loopback scenario 2, with following script (reproducibility < 1%)
	$ atest -D SSI3 -c 8 -r 48000 capture &
	$ sleep 0.2
	$ atest -d 1 -D SSI2 -c 8 -r 48000 play
	dbg: SSI3: capture_start
	dbg: set period size: 960
	dbg: SSI2: playback_start
	dbg: start a 1 seconds duration timer
	err: invalid frame after 11568 null frames
	err:   0000 0010 0011 0012 0013 0014 0015 0016 
	err:   0017 0020 0021 0022 0023 0024 0025 0026 



Patch 6: 
	Deals with Capture restart whereas Playback is still running
	(or the opposite, Playback restart whereas Capture is till running).
	
	Capture restart whereas Playback case is detected in 
	loopback scenario 1 (reproducibility 100%). 
	A playback session is running in background, and 
	2 consecutive Captures are performed. 
	The first is fine, the second fails.
	
	We see at the 2nd capture startup that we receive 15 samples
	still pending in the RX FIFO from the previous capture session.
	=> We are receiving invalid samples + slips the channels
	
	$ atest -D SSI3 -c 8 -r 48000 play &
	dbg: SSI3: playback_start
	$ atest -d 1 -D SSI3 -c 8 -r 48000 capture
	dbg: SSI3: capture_start
	dbg: start a 1 seconds duration timer
	warn: Valid frame after 1 null frames
	warn:   3a90 3a91 3a92 3a93 3a94 3a95 3a96 3a97 
	total number of sequence errors: 0
	
	$ atest -I 5 -d 1 -D SSI3 -c 8 -r 48000 capture  # restart the capture
	dbg: SSI3: capture_start
	dbg: start a 1 seconds duration timer
	err: invalid frame after 1 null frames
	err:   8dd6 8dd7 8de0 8de1 8de2 8de3 8de4 8de5 
	err:   8de6 8de7 8df0 8df1 8df2 8df3 8df4 c0c0 
	err:   c0c1 c0c2 c0c3 c0c4 c0c5 c0c6 c0c7 c0d0 
	err:   c0d1 c0d2 c0d3 c0d4 c0d5 c0d6 c0d7 c0e0 
	err:   c0e1 c0e2 c0e3 c0e4 c0e5 c0e6 c0e7 c0f0 


	Playback restart whereas Capture case is also detected with
	loopback scenario 1 (reproducibility 100%), capturing continuously,
	and playing by periods of time.
	
	$ atest -a -D SSI3 -c 8 -r 48000 capture play -r 1000,200
	dbg: SSI3: capture_start
	dbg: SSI3: playback_start
	dbg: SSI3: will stop every 1000 ms during 200 ms
	warn: Valid frame after 4 null frames
	warn:   0010 0011 0012 0013 0014 0015 0016 0017 
	warn: SSI3: PT_W4_STOP
	warn: SSI3: PT_W4_RESTART
	err: invalid frame after 1602 null frames
	err:   eaa1 eaa2 eaa3 eaa4 eaa5 eaa6 eaa7 eab0 
	err:   eab1 eab2 eab3 eab4 5810 5811 5812 5813 
	dbg: stop on first error
	total number of sequence errors: 143

	
	
	Both cases are resolved by using the mostly undocumented SOR.RX_CLR and 
	SOR TX_CLR (we can find the documentation in IMX51 reference manual 
	at section 56.3.3.15).
	
	
Arnaud



[1] https://github.com/amouiche/atest
[2] http://mailman.alsa-project.org/pipermail/alsa-devel/2015-October/099221.html


Arnaud Mouiche (5):
  ASoC: fsl_ssi: The IPG/5 limitation concerns the bitclk, not the
    sysclk.
  ASoC: fsl_ssi: Save a dev reference for dev_err() purpose.
  ASoC: fsl_ssi: Fix samples being dropped as Playback startup
  ASoC: fsl_ssi: Fix channel slipping in Playback at startup
  ASoC: fsl_ssi: Fix channel slipping on capture (or playback) restart
    in full duplex.

 sound/soc/fsl/fsl_ssi.c | 69 ++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 60 insertions(+), 9 deletions(-)

-- 
1.9.1

[PATCH 1/5] ASoC: fsl_ssi: The IPG/5 limitation concerns the bitclk, not the sysclk.

[Arnaud Mouiche]

im6sl reference manual 47.7.4:
"
Bit clock - Used to serially clock the data bits in and out of the SSI port.
This clock is either generated internally (from SSI's sys clock) or taken
from external clock source (through the Tx/Rx clock ports).
[...]
Care should be taken to ensure that the bit clock frequency (either
internally generated by dividing the SSI's sys clock or sourced from
external device through Tx/Rx clock ports) is never greater than 1/5
of the ipg_clk (from CCM) frequency.
"

Since, in master mode, the sysclk is a multiple of bitclk, we can
easily reach a high sysclk value, whereas keeping a reasonable bitclk.

ex: 8ch x 16bit x 48kHz = 6144000, requires a 24576000 sysclk (PM=1)
    yet ipg_clk/5 = 66Mhz/5 = 13.2

Signed-off-by: Arnaud Mouiche <arnaud.mouiche@invoxia.com>
---
 sound/soc/fsl/fsl_ssi.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 4e5d7db..b34c09f 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -595,6 +595,15 @@ static int fsl_ssi_set_bclk(struct snd_pcm_substream *substream,
 	if (IS_ERR(ssi_private->baudclk))
 		return -EINVAL;
 
+	/*
+	 * Hardware limitation: The bclk rate must be
+	 * never greater than 1/5 IPG clock rate
+	 */
+	if (freq * 5 > clk_get_rate(ssi_private->clk)) {
+		dev_err(cpu_dai->dev, "bitclk > ipgclk/5\n");
+		return -EINVAL;
+	}
+
 	baudclk_is_used = ssi_private->baudclk_streams & ~(BIT(substream->stream));
 
 	/* It should be already enough to divide clock by setting pm alone */
@@ -611,13 +620,6 @@ static int fsl_ssi_set_bclk(struct snd_pcm_substream *substream,
 		else
 			clkrate = clk_round_rate(ssi_private->baudclk, tmprate);
 
-		/*
-		 * Hardware limitation: The bclk rate must be
-		 * never greater than 1/5 IPG clock rate
-		 */
-		if (clkrate * 5 > clk_get_rate(ssi_private->clk))
-			continue;
-
 		clkrate /= factor;
 		afreq = clkrate / (i + 1);
 
-- 
1.9.1

[PATCH 2/5] ASoC: fsl_ssi: Save a dev reference for dev_err() purpose.

[Arnaud Mouiche]

Most of functions only receive the ssi_private reference and don't have
a knowledge of 'dev' pointer, even for debug purpose.

Signed-off-by: Arnaud Mouiche <arnaud.mouiche@invoxia.com>
---
 sound/soc/fsl/fsl_ssi.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index b34c09f..243c9af 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -191,6 +191,7 @@ struct fsl_ssi_private {
 	struct fsl_ssi_dbg dbg_stats;
 
 	const struct fsl_ssi_soc_data *soc;
+	struct device *dev;
 };
 
 /*
@@ -1329,6 +1330,7 @@ static int fsl_ssi_probe(struct platform_device *pdev)
 	}
 
 	ssi_private->soc = of_id->data;
+	ssi_private->dev = &pdev->dev;
 
 	sprop = of_get_property(np, "fsl,mode", NULL);
 	if (sprop) {
-- 
1.9.1

[PATCH 3/5] ASoC: fsl_ssi: Fix samples being dropped as Playback startup

[Arnaud Mouiche]

If the capture is already running while playback is started, it is highly
probable (>80% in a 8 channels scenario) that samples are lost between
the DMA and TX fifo.

The reason is that SIER.TDMAE is set before STCR.TFEN0, leaving a time
window where the FIFO doesn't receive the samples written by the DMA.

This particular case happened only if capture is already enabled as
SCR.SSIEN is already set at the playback startup instant.

Signed-off-by: Arnaud Mouiche <arnaud.mouiche@invoxia.com>
---
 sound/soc/fsl/fsl_ssi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 243c9af..78ea6e1 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -404,9 +404,9 @@ static void fsl_ssi_config(struct fsl_ssi_private *ssi_private, bool enable,
 	 * (online configuration)
 	 */
 	if (enable) {
-		regmap_update_bits(regs, CCSR_SSI_SIER, vals->sier, vals->sier);
 		regmap_update_bits(regs, CCSR_SSI_SRCR, vals->srcr, vals->srcr);
 		regmap_update_bits(regs, CCSR_SSI_STCR, vals->stcr, vals->stcr);
+		regmap_update_bits(regs, CCSR_SSI_SIER, vals->sier, vals->sier);
 	} else {
 		u32 sier;
 		u32 srcr;
-- 
1.9.1

[PATCH 4/5] ASoC: fsl_ssi: Fix channel slipping in Playback at startup

[Arnaud Mouiche]

Previously, SCR.SSIEN and SCR.TE were enabled at once if no capture
stream was also running.
This may not give a chance for the DMA to write the first sample in
TX FIFO before the streaming starts on the PCM bus, inserting void
samples first.
Those void samples are then responsible for slipping the channels.

Signed-off-by: Arnaud Mouiche <arnaud.mouiche@invoxia.com>
---
 sound/soc/fsl/fsl_ssi.c | 33 ++++++++++++++++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 78ea6e1..c71e194 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -436,8 +436,39 @@ static void fsl_ssi_config(struct fsl_ssi_private *ssi_private, bool enable,
 
 config_done:
 	/* Enabling of subunits is done after configuration */
-	if (enable)
+	if (enable) {
+		if (ssi_private->use_dma && (vals->scr & CCSR_SSI_SCR_TE)) {
+			/*
+			 * Be sure the Tx FIFO is filled when TE is set.
+			 * Otherwise, there are some chances to start the
+			 * playback with some void samples inserted first,
+			 * generating a channel slip.
+			 *
+			 * First, SSIEN must be set, to let the FIFO be filled.
+			 *
+			 * Notes:
+			 * - Limit this fix to the DMA case until FIQ cases can
+			 *   be tested.
+			 * - Limit the length of the busy loop to not lock the
+			 *   system too long, even if 1-2 loops are sufficient
+			 *   in general.
+			 */
+			int i;
+			int max_loop = 100;
+			regmap_update_bits(regs, CCSR_SSI_SCR, CCSR_SSI_SCR_SSIEN,
+					CCSR_SSI_SCR_SSIEN);
+			for (i=0; i < max_loop; i++) {
+				u32 sfcsr;
+				regmap_read(regs, CCSR_SSI_SFCSR, &sfcsr);
+				if (CCSR_SSI_SFCSR_TFCNT0(sfcsr)) break;
+			}
+			if (i == max_loop) {
+				dev_err(ssi_private->dev,
+						"Timeout waiting TX FIFO filling\n");
+			}
+		}
 		regmap_update_bits(regs, CCSR_SSI_SCR, vals->scr, vals->scr);
+	}
 }
 
 
-- 
1.9.1

[PATCH 5/5] ASoC: fsl_ssi: Fix channel slipping on capture (or playback) restart in full duplex.

[Arnaud Mouiche]

Happened when the Playback (or Capture) is running continuously
and Capture (or Playback) is restarted (xrun, manual stop/start...)

Since the RX (or TX) FIFO are only reset when the whole SSI is disabled,
pending samples from previous capture (or playback) session may still
be present. They must be erased to not introduce channel slipping.

FIFO Clear register fields are documented in IMX51, IMX35 reference manual.
They are not documented in IMX50 or IMX6 RM, despite they are
working as expected on IMX6SL.

Signed-off-by: Arnaud Mouiche <arnaud.mouiche@invoxia.com>
---
 sound/soc/fsl/fsl_ssi.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index c71e194..576a72b 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -404,6 +404,22 @@ static void fsl_ssi_config(struct fsl_ssi_private *ssi_private, bool enable,
 	 * (online configuration)
 	 */
 	if (enable) {
+		/*
+		 * Clear RX or TX FIFO to remove samples from the previous stream
+		 * session which may be still present in the FIFO and may
+		 * introduce bad samples and/or channel slipping.
+		 *
+		 * Note: The SOR is not documented in recent IMX datasheet, but
+		 * is described in IMX51 reference manual at section 56.3.3.15.
+		 */
+		if (vals->scr & CCSR_SSI_SCR_RE) {
+			regmap_update_bits(regs, CCSR_SSI_SOR,
+					CCSR_SSI_SOR_RX_CLR, CCSR_SSI_SOR_RX_CLR );
+		} else {
+			regmap_update_bits(regs, CCSR_SSI_SOR,
+					CCSR_SSI_SOR_TX_CLR, CCSR_SSI_SOR_TX_CLR );
+		}
+
 		regmap_update_bits(regs, CCSR_SSI_SRCR, vals->srcr, vals->srcr);
 		regmap_update_bits(regs, CCSR_SSI_STCR, vals->stcr, vals->stcr);
 		regmap_update_bits(regs, CCSR_SSI_SIER, vals->sier, vals->sier);
-- 
1.9.1

Re: [PATCH 0/5] ASoC: fsl_ssi: Fixing various channel slips and bad samples insertions

[Caleb Crome]

On Thu, Nov 26, 2015 at 6:07 AM, Arnaud Mouiche
<arnaud.mouiche@invoxia.com> wrote:
> This series of patch is an attempt to fix the fsl_ssi driver to use it in TDM mode (DSP A or B) with a large number of channels/slots.
>
> Bugs are detected and fixed on a imx6sl platform with linux 4.3, where 2 SSI interfaces are used. SSI3 configured as a master of the bus, SSI2 as a slave.
>
> Various loopback scenario are tested:
> * scenario 1: SSI3 (master) with TXD to RXD loopback
>
>                  |
>                  |---> TXD  --\
>         SSI3     |<--- RXD  --/
>         (master) |---> SYNC
>                  |---> BCLK
>                  |
>
> * scenario 2: SSI3 connected to SSI2
>                  |
>                  |---> TXD  --------\
>         SSI3     |<--- RXD  -----\  |
>         (master) |---> SYNC --\  |  |
>                  |---> BCLK   |  |  |
>                  |       |    |  |  |
>                  |       |    |  |  |
>                  |<--- BCLK   |  |  |
>         SSI2     |<--- SYNC --/  |  |
>         (slave)  |---> TXD ------/  |
>                  |<--- RXD  --------/
>                  |
>
>
>
> A test software (called atest) was developed and available. [1]
> It basically generate/check specials frames with S16_LE sequence samples
>        NNN0, NNN1, NNN2 ... NNNC     with NNN = frame number, and C the number of channels-1
>
>
>
> Patch 1:
>         Limitation fix. Prerequisite to use the fsl_ssi driver with up to 32 channels / slots
>
> Patch 2:
>         Bug fix. Prerequisite to setup a relative high bitclk for our tests in 8ch / 16bits / 48kHz
>
> Patch 3:
>         Simply save a 'dev' reference inside ssi_private for dev_err() purpose.
>         (ssi_private is only available in lot of places, and
>          ssi_private->pdev is deprecated and NULL indeed)
>
> Patch 4:
>         Fix playback samples being dropped because the TX fifo was not ready at
>         the time the DMA starts filling (only in case where playback is started AFTER the capture)
>
>         Detected in loopback scenario 1, with following script (> 80 % of reproducibility)
>         $ atest -D SSI3 -c 8 -r 48000 capture play
>         dbg: set period size: 960
>         dbg: SSI3: capture_start
>         dbg: SSI3: playback_start
>         err: invalid frame after 1 null frames
>         err:   0000 0000 0013 0014 0015 0016 0017 0020
>         dbg: SIGINT
>         total number of sequence errors: 21119
>
>         here we see that samples 0000 0001 ... 0012 are not present in the output.
>         In addition, this the number of samples dropped is not a multiple of
>         the number of channel, we have a a channel slip.
>
>
> Patch 5:
>         This is the Caleb Crome's case [2], where the SSI starts to generate samples while
>         the TX FIFO is not filled by the DMA yet. Void samples can be inserted before DMA streaming.
>
>         Detected in loopback scenario 2, with following script (reproducibility < 1%)
>         $ atest -D SSI3 -c 8 -r 48000 capture &
>         $ sleep 0.2
>         $ atest -d 1 -D SSI2 -c 8 -r 48000 play
>         dbg: SSI3: capture_start
>         dbg: set period size: 960
>         dbg: SSI2: playback_start
>         dbg: start a 1 seconds duration timer
>         err: invalid frame after 11568 null frames
>         err:   0000 0010 0011 0012 0013 0014 0015 0016
>         err:   0017 0020 0021 0022 0023 0024 0025 0026
>
>
>
> Patch 6:
>         Deals with Capture restart whereas Playback is still running
>         (or the opposite, Playback restart whereas Capture is till running).
>
>         Capture restart whereas Playback case is detected in
>         loopback scenario 1 (reproducibility 100%).
>         A playback session is running in background, and
>         2 consecutive Captures are performed.
>         The first is fine, the second fails.
>
>         We see at the 2nd capture startup that we receive 15 samples
>         still pending in the RX FIFO from the previous capture session.
>         => We are receiving invalid samples + slips the channels
>
>         $ atest -D SSI3 -c 8 -r 48000 play &
>         dbg: SSI3: playback_start
>         $ atest -d 1 -D SSI3 -c 8 -r 48000 capture
>         dbg: SSI3: capture_start
>         dbg: start a 1 seconds duration timer
>         warn: Valid frame after 1 null frames
>         warn:   3a90 3a91 3a92 3a93 3a94 3a95 3a96 3a97
>         total number of sequence errors: 0
>
>         $ atest -I 5 -d 1 -D SSI3 -c 8 -r 48000 capture  # restart the capture
>         dbg: SSI3: capture_start
>         dbg: start a 1 seconds duration timer
>         err: invalid frame after 1 null frames
>         err:   8dd6 8dd7 8de0 8de1 8de2 8de3 8de4 8de5
>         err:   8de6 8de7 8df0 8df1 8df2 8df3 8df4 c0c0
>         err:   c0c1 c0c2 c0c3 c0c4 c0c5 c0c6 c0c7 c0d0
>         err:   c0d1 c0d2 c0d3 c0d4 c0d5 c0d6 c0d7 c0e0
>         err:   c0e1 c0e2 c0e3 c0e4 c0e5 c0e6 c0e7 c0f0
>
>
>         Playback restart whereas Capture case is also detected with
>         loopback scenario 1 (reproducibility 100%), capturing continuously,
>         and playing by periods of time.
>
>         $ atest -a -D SSI3 -c 8 -r 48000 capture play -r 1000,200
>         dbg: SSI3: capture_start
>         dbg: SSI3: playback_start
>         dbg: SSI3: will stop every 1000 ms during 200 ms
>         warn: Valid frame after 4 null frames
>         warn:   0010 0011 0012 0013 0014 0015 0016 0017
>         warn: SSI3: PT_W4_STOP
>         warn: SSI3: PT_W4_RESTART
>         err: invalid frame after 1602 null frames
>         err:   eaa1 eaa2 eaa3 eaa4 eaa5 eaa6 eaa7 eab0
>         err:   eab1 eab2 eab3 eab4 5810 5811 5812 5813
>         dbg: stop on first error
>         total number of sequence errors: 143
>
>
>
>         Both cases are resolved by using the mostly undocumented SOR.RX_CLR and
>         SOR TX_CLR (we can find the documentation in IMX51 reference manual
>         at section 56.3.3.15).
>
>
> Arnaud
>
>
>
> [1] https://github.com/amouiche/atest
> [2] http://mailman.alsa-project.org/pipermail/alsa-devel/2015-October/099221.html
>
>
> Arnaud Mouiche (5):
>   ASoC: fsl_ssi: The IPG/5 limitation concerns the bitclk, not the
>     sysclk.
>   ASoC: fsl_ssi: Save a dev reference for dev_err() purpose.
>   ASoC: fsl_ssi: Fix samples being dropped as Playback startup
>   ASoC: fsl_ssi: Fix channel slipping in Playback at startup
>   ASoC: fsl_ssi: Fix channel slipping on capture (or playback) restart
>     in full duplex.
>
>  sound/soc/fsl/fsl_ssi.c | 69 ++++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 60 insertions(+), 9 deletions(-)
>
> --
> 1.9.1
>
Hello Arnaud,
  I have finally gotten to test your patches, and I'm still having
trouble with channel slips.

I applied your v2 patch set, along with your changes for using a dummy codec.

The full changes are here:
https://github.com/ccrome/linux-caleb-dev/tree/v4.4-rc8-armv7-x3

This ignores most of my previous patches, and uses your code to bring
up the SSI (without a codec) on a wandboard.

I am using SSI3, and doing a hardware loopback between TX and RX.

Here's what I run:
./atest -r 16000 -c 8 -p 2048   -D default play

which plays continuously.

and in another shell:
./atest -r 16000 -c 8 -p 2048  -D default -d 10 capture

which captures for 10 seconds.


The first time I run the capture command, it succeeds, no problem.
> dbg: dev: 'default'
> dbg: default: capture_start
> dbg: start a 10 seconds duration timer
> warn: First valid frame
> warn:   3400 3401 3402 3403 3404 3405 3406 3407
> dbg: end of tests
> total number of sequence errors: 0
> global tests exit status: OK

But the second and all subsequent captures, it fails with channel slips:

> dbg: dev: 'default'
> dbg: default: capture_start
> dbg: start a 10 seconds duration timer
> err: invalid frame after 0 null frames
> err:   d2a1 d2a2 d2a3 d2a4 d2a5 d2a6 d2a7 d2c0
> err:   d2c1 d2c2 d2c3 d2c4 d2c5 d2c6 d2c7 78e0
> dbg: end of tests
> total number of sequence errors: 430080
> global tests exit status: OK

I verified with a scope that the data on the data bus is correct, the
problem is with restarting RX, the RX doesn't synchronize to the frame
sync.  BTW, the RX slot is random. 1/8 times or so, the restarted
capture actually works.

My setup is a single core wandboard (i.mx6 solo), with J1.16 jumpered
over to J1.20 (tx->rx)

Thanks for the patches and for the atest program!  It's really handy :-)

So, I have not messed with water marks, dual fifo, etc with your patches yet.

Any other patches that you think might need to be applied to make the
RX restart work?
-Caleb

Re: [PATCH 0/5] ASoC: fsl_ssi: Fixing various channel slips and bad samples insertions

[arnaud.mouiche]

Hello Caleb

Le 09/01/2016 01:47, Caleb Crome a écrit :
> [...]
> Hello Arnaud,
>    I have finally gotten to test your patches, and I'm still having
> trouble with channel slips.
>
> I applied your v2 patch set, along with your changes for using a dummy codec.
>
> The full changes are here:
> https://github.com/ccrome/linux-caleb-dev/tree/v4.4-rc8-armv7-x3
>
> This ignores most of my previous patches, and uses your code to bring
> up the SSI (without a codec) on a wandboard.
>
> I am using SSI3, and doing a hardware loopback between TX and RX.
>
> Here's what I run:
> ./atest -r 16000 -c 8 -p 2048   -D default play
>
> which plays continuously.
>
> and in another shell:
> ./atest -r 16000 -c 8 -p 2048  -D default -d 10 capture
>
> which captures for 10 seconds.
>
>
> The first time I run the capture command, it succeeds, no problem.
>> dbg: dev: 'default'
>> dbg: default: capture_start
>> dbg: start a 10 seconds duration timer
>> warn: First valid frame
>> warn:   3400 3401 3402 3403 3404 3405 3406 3407
>> dbg: end of tests
>> total number of sequence errors: 0
>> global tests exit status: OK
> But the second and all subsequent captures, it fails with channel slips:
>
>> dbg: dev: 'default'
>> dbg: default: capture_start
>> dbg: start a 10 seconds duration timer
>> err: invalid frame after 0 null frames
>> err:   d2a1 d2a2 d2a3 d2a4 d2a5 d2a6 d2a7 d2c0
>> err:   d2c1 d2c2 d2c3 d2c4 d2c5 d2c6 d2c7 78e0
>> dbg: end of tests
>> total number of sequence errors: 430080
>> global tests exit status: OK

Can you use -I option to get a little more log of the error
$ ./atest -r 16000 -c 8 -p 2048  -D default -d 10 -I 10 capture

Just to know if the wrong frames comes from the previous record session, 
meaning the RX fifo was not cleared correctly,
as if the CCSR_SSI_SOR_RX_CLR bit is not working as we might expect.
(ie. d2a1 .. d2c7 may come from the previous recording, and 78e0 .. may 
be the start of the new recording session)

May be you can read the RX fifo level just avec applying 
CCSR_SSI_SOR_RX_CLR to assert the fifo is really empty ?

In case it is still not empty, we will still have the possibility to 
empty the fifo by ready the samples one by one manually.

Regards,
arnaud
> I verified with a scope that the data on the data bus is correct, the
> problem is with restarting RX, the RX doesn't synchronize to the frame
> sync.  BTW, the RX slot is random. 1/8 times or so, the restarted
> capture actually works.
>
> My setup is a single core wandboard (i.mx6 solo), with J1.16 jumpered
> over to J1.20 (tx->rx)
>
> Thanks for the patches and for the atest program!  It's really handy :-)
>
> So, I have not messed with water marks, dual fifo, etc with your patches yet.
>
> Any other patches that you think might need to be applied to make the
> RX restart work?
> -Caleb

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 0/5] ASoC: fsl_ssi: Fixing various channel slips and bad samples insertions

[Caleb Crome]

On Sat, Jan 9, 2016 at 3:02 AM, arnaud.mouiche@invoxia.com
<arnaud.mouiche@invoxia.com> wrote:
> Hello Caleb
>
> Le 09/01/2016 01:47, Caleb Crome a écrit :
>>
>> [...]
>>
>> Hello Arnaud,
>>    I have finally gotten to test your patches, and I'm still having
>> trouble with channel slips.
>>
>> I applied your v2 patch set, along with your changes for using a dummy
>> codec.
>>
>> The full changes are here:
>> https://github.com/ccrome/linux-caleb-dev/tree/v4.4-rc8-armv7-x3
>>
>> This ignores most of my previous patches, and uses your code to bring
>> up the SSI (without a codec) on a wandboard.
>>
>> I am using SSI3, and doing a hardware loopback between TX and RX.
>>
>> Here's what I run:
>> ./atest -r 16000 -c 8 -p 2048   -D default play
>>
>> which plays continuously.
>>
>> and in another shell:
>> ./atest -r 16000 -c 8 -p 2048  -D default -d 10 capture
>>
>> which captures for 10 seconds.
>>
>>
>> The first time I run the capture command, it succeeds, no problem.
>>>
>>> dbg: dev: 'default'
>>> dbg: default: capture_start
>>> dbg: start a 10 seconds duration timer
>>> warn: First valid frame
>>> warn:   3400 3401 3402 3403 3404 3405 3406 3407
>>> dbg: end of tests
>>> total number of sequence errors: 0
>>> global tests exit status: OK
>>
>> But the second and all subsequent captures, it fails with channel slips:
>>
>>> dbg: dev: 'default'
>>> dbg: default: capture_start
>>> dbg: start a 10 seconds duration timer
>>> err: invalid frame after 0 null frames
>>> err:   d2a1 d2a2 d2a3 d2a4 d2a5 d2a6 d2a7 d2c0
>>> err:   d2c1 d2c2 d2c3 d2c4 d2c5 d2c6 d2c7 78e0
>>> dbg: end of tests
>>> total number of sequence errors: 430080
>>> global tests exit status: OK
>
>
> Can you use -I option to get a little more log of the error
> $ ./atest -r 16000 -c 8 -p 2048  -D default -d 10 -I 10 capture
>
> Just to know if the wrong frames comes from the previous record session,
> meaning the RX fifo was not cleared correctly,
> as if the CCSR_SSI_SOR_RX_CLR bit is not working as we might expect.
> (ie. d2a1 .. d2c7 may come from the previous recording, and 78e0 .. may be
> the start of the new recording session)

That does appear to be what's happening.  I read the SFCSR before and
after you update the SOR register (SOR_RX_CLR), and in both cases the
rx buffer is full on the second enablement.

I can't seem to empty that buffer, either by using the SOR_RX_CLR or
by reading the SRX0 register. This is another one of those cases where
there you cannot modify the register (i.e. fifo or RX0) when RE is
disabled.

So, instead of clearing on enable, I'm clearing on shutdown and on
enable.  I attempted using the SOR_RX_CLR, but it doesn't seem to work
on the MX6.  Rather, I clear the fifo by reading all the words in a
loop, just before RE is disabled.



>
> May be you can read the RX fifo level just avec applying CCSR_SSI_SOR_RX_CLR
> to assert the fifo is really empty ?
>
> In case it is still not empty, we will still have the possibility to empty
> the fifo by ready the samples one by one manually.

so far, with cursory testing, this patch, on top of your V2 patches
seems to work.



From 15a218858a75163e2d72ed8329a26b0d22a0bfc0 Mon Sep 17 00:00:00 2001
From: Caleb Crome <caleb@crome.org>
Date: Mon, 11 Jan 2016 15:29:13 -0800
Subject: [PATCH] ASoC: fsl_ssi: Empty rx fifo on rx start and finish to
 eliminate channel slips

Arnaud's previous patches mostly, but not completely work on the MX6.
The previous patch used the SOR_RX_CLR register, which doesn't
seem to work on the MX6.  I guess it's undocumented for a reason.

This patch clears the RX fifo by using successive reads of the
SRX0 (and optionally SRX1) register until the SISR_RDR0/1 bit
is clear.  This ensure's an empty fifo to start capturing with.

Signed-off-by: Caleb Crome <caleb@crome.org>
---
 sound/soc/fsl/fsl_ssi.c | 44 +++++++++++++++++++++++++++++++++++++-------
 1 file changed, 37 insertions(+), 7 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index e44c9c3..5329aaa 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -366,6 +366,40 @@ static irqreturn_t fsl_ssi_isr(int irq, void *dev_id)
     return IRQ_HANDLED;
 }

+// empty fifo:  must be called when the unit is ENABLED!
+// otherwise it will not be able to empty the fifos.
+static u32 fsl_empty_fifo(struct device *dev, struct regmap *regs,
int is_rx, int dual_fifo)
+{
+    u32 val;
+    u32 sisr;
+    u32 err = 0;
+    int limit = 100;
+    if (is_rx) {
+        do {
+            regmap_read(regs, CCSR_SSI_SRX0, &val);
+            regmap_read(regs, CCSR_SSI_SISR, &sisr);
+        } while ((limit-- >=0) && (sisr & CCSR_SSI_SISR_RDR0));
+        if (limit <= 0)  {
+            dev_err(dev, "Couldn't empty out the RX "
+                "FIFO 0.  SISR = 0x%08x\n", sisr);
+            err = 1;
+        }
+        if (dual_fifo) {
+            do {
+                regmap_read(regs, CCSR_SSI_SRX1, &val);
+                regmap_read(regs, CCSR_SSI_SISR, &sisr);
+            } while ((limit-- >=0) &&
+                 (sisr & CCSR_SSI_SISR_RDR1));
+            if (limit <= 0)  {
+                dev_err(dev, "Couldn't empty out the RX "
+                    "FIFO 1.  SISR = 0x%08x\n", sisr);
+                err = 1;
+            }
+        }
+    }
+    return err;
+}
+
 /*
  * Enable/Disable all rx/tx config flags at once.
  */
@@ -428,6 +462,7 @@ static void fsl_ssi_config(struct fsl_ssi_private
*ssi_private, bool enable,
     u32 scr_val;
     int keep_active;

+    int is_rx = vals->scr & CCSR_SSI_SCR_RE;
     regmap_read(regs, CCSR_SSI_SCR, &scr_val);

     nr_active_streams = !!(scr_val & CCSR_SSI_SCR_TE) +
@@ -478,13 +513,7 @@ static void fsl_ssi_config(struct fsl_ssi_private
*ssi_private, bool enable,
          * Note: The SOR is not documented in recent IMX datasheet, but
          * is described in IMX51 reference manual at section 56.3.3.15.
          */
-        if (vals->scr & CCSR_SSI_SCR_RE) {
-            regmap_update_bits(regs, CCSR_SSI_SOR,
-                CCSR_SSI_SOR_RX_CLR, CCSR_SSI_SOR_RX_CLR);
-        } else {
-            regmap_update_bits(regs, CCSR_SSI_SOR,
-                CCSR_SSI_SOR_TX_CLR, CCSR_SSI_SOR_TX_CLR);
-        }
+        fsl_empty_fifo(ssi_private->dev, regs, is_rx,
ssi_private->use_dual_fifo);

         regmap_update_bits(regs, CCSR_SSI_SRCR, vals->srcr, vals->srcr);
         regmap_update_bits(regs, CCSR_SSI_STCR, vals->stcr, vals->stcr);
@@ -502,6 +531,7 @@ static void fsl_ssi_config(struct fsl_ssi_private
*ssi_private, bool enable,
          * disabled now. Otherwise we could alter flags of the other
          * stream
          */
+        fsl_empty_fifo(ssi_private->dev, regs, is_rx,
ssi_private->use_dual_fifo);

         /* These assignments are simply vals without bits set in avals*/
         sier = fsl_ssi_disable_val(vals->sier, avals->sier,
-- 
2.1.4


What do you think?

-Caleb



>
> Regards,
> arnaud
>
>> I verified with a scope that the data on the data bus is correct, the
>> problem is with restarting RX, the RX doesn't synchronize to the frame
>> sync.  BTW, the RX slot is random. 1/8 times or so, the restarted
>> capture actually works.
>>
>> My setup is a single core wandboard (i.mx6 solo), with J1.16 jumpered
>> over to J1.20 (tx->rx)
>>
>> Thanks for the patches and for the atest program!  It's really handy :-)
>>
>> So, I have not messed with water marks, dual fifo, etc with your patches
>> yet.
>>
>> Any other patches that you think might need to be applied to make the
>> RX restart work?
>> -Caleb
>
>
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 0/5] ASoC: fsl_ssi: Fixing various channel slips and bad samples insertions

[arnaud.mouiche]

Le 12/01/2016 00:44, Caleb Crome a écrit :
> On Sat, Jan 9, 2016 at 3:02 AM, arnaud.mouiche@invoxia.com
> <arnaud.mouiche@invoxia.com> wrote:
>> Hello Caleb
>>
>> Le 09/01/2016 01:47, Caleb Crome a écrit :
>>> [...]
>>>
>>> Hello Arnaud,
>>>     I have finally gotten to test your patches, and I'm still having
>>> trouble with channel slips.
>>>
>>> I applied your v2 patch set, along with your changes for using a dummy
>>> codec.
>>>
>>> The full changes are here:
>>> https://github.com/ccrome/linux-caleb-dev/tree/v4.4-rc8-armv7-x3
>>>
>>> This ignores most of my previous patches, and uses your code to bring
>>> up the SSI (without a codec) on a wandboard.
>>>
>>> I am using SSI3, and doing a hardware loopback between TX and RX.
>>>
>>> Here's what I run:
>>> ./atest -r 16000 -c 8 -p 2048   -D default play
>>>
>>> which plays continuously.
>>>
>>> and in another shell:
>>> ./atest -r 16000 -c 8 -p 2048  -D default -d 10 capture
>>>
>>> which captures for 10 seconds.
>>>
>>>
>>> The first time I run the capture command, it succeeds, no problem.
>>>> dbg: dev: 'default'
>>>> dbg: default: capture_start
>>>> dbg: start a 10 seconds duration timer
>>>> warn: First valid frame
>>>> warn:   3400 3401 3402 3403 3404 3405 3406 3407
>>>> dbg: end of tests
>>>> total number of sequence errors: 0
>>>> global tests exit status: OK
>>> But the second and all subsequent captures, it fails with channel slips:
>>>
>>>> dbg: dev: 'default'
>>>> dbg: default: capture_start
>>>> dbg: start a 10 seconds duration timer
>>>> err: invalid frame after 0 null frames
>>>> err:   d2a1 d2a2 d2a3 d2a4 d2a5 d2a6 d2a7 d2c0
>>>> err:   d2c1 d2c2 d2c3 d2c4 d2c5 d2c6 d2c7 78e0
>>>> dbg: end of tests
>>>> total number of sequence errors: 430080
>>>> global tests exit status: OK
>>
>> Can you use -I option to get a little more log of the error
>> $ ./atest -r 16000 -c 8 -p 2048  -D default -d 10 -I 10 capture
>>
>> Just to know if the wrong frames comes from the previous record session,
>> meaning the RX fifo was not cleared correctly,
>> as if the CCSR_SSI_SOR_RX_CLR bit is not working as we might expect.
>> (ie. d2a1 .. d2c7 may come from the previous recording, and 78e0 .. may be
>> the start of the new recording session)
> That does appear to be what's happening.  I read the SFCSR before and
> after you update the SOR register (SOR_RX_CLR), and in both cases the
> rx buffer is full on the second enablement.
>
> I can't seem to empty that buffer, either by using the SOR_RX_CLR or
> by reading the SRX0 register. This is another one of those cases where
> there you cannot modify the register (i.e. fifo or RX0) when RE is
> disabled.
>
> So, instead of clearing on enable, I'm clearing on shutdown and on
> enable.  I attempted using the SOR_RX_CLR, but it doesn't seem to work
> on the MX6.  Rather, I clear the fifo by reading all the words in a
> loop, just before RE is disabled.
so the imx6.sl SSI behaves differently from the imx6.solo SSI ... Or 
some things have changes between 4.3 and 4.4.

May be SOR_RX_CLR and/or RX0 registers can't be read on 'solo' because 
de the SRCR.RFEN0 [or1] are not enabled yet.

In fact, clearing the fifos at the end may still introduce a race since 
the RX path is still enabled when your read from fifo, and
Samples may still be received in the meantime...

Also, I see that the CCSR_SSI_SOR register is not in the regmap 
fsl_ssi_volatile_reg() list (line 140). May be there is an implication 
of some caching mechanism ?

Can you try to move the the fifo clearing part just before the write in 
CCSR_SSI_SIER with the following change in top of my patches ?
Also may be you can additionally add the RX0 / RX1 reading from clearing 
mechanism at the same place.

On my side, I will check the 4.4 tree on imx6sl

Regards,
Arnaud

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 0277097..bd163b2 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -420,6 +420,9 @@ static void fsl_ssi_config(struct fsl_ssi_private 
*ssi_private, int enable,
          * (online configuration)
          */
         if (enable) {
+
+               regmap_update_bits(regs, CCSR_SSI_SRCR, vals->srcr, 
vals->srcr);
+               regmap_update_bits(regs, CCSR_SSI_STCR, vals->stcr, 
vals->stcr);
                 /*
                  * Clear RX or TX FIFO to remove samples from the previous
                  * stream session which may be still present in the 
FIFO and
@@ -435,9 +438,6 @@ static void fsl_ssi_config(struct fsl_ssi_private 
*ssi_private, int enable,
                         regmap_update_bits(regs, CCSR_SSI_SOR,
                                 CCSR_SSI_SOR_TX_CLR, CCSR_SSI_SOR_TX_CLR);

                 }
-
-               regmap_update_bits(regs, CCSR_SSI_SRCR, vals->srcr, 
vals->srcr);
-               regmap_update_bits(regs, CCSR_SSI_STCR, vals->stcr, 
vals->stcr);
                 regmap_update_bits(regs, CCSR_SSI_SIER, vals->sier, 
vals->sier);
         } else {
                 u32 sier;



_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 0/5] ASoC: fsl_ssi: Fixing various channel slips and bad samples insertions

[Caleb Crome]

On Wed, Jan 13, 2016 at 6:45 AM, arnaud.mouiche@invoxia.com
<arnaud.mouiche@invoxia.com> wrote:
>
>
> Le 12/01/2016 00:44, Caleb Crome a écrit :
>>
>> On Sat, Jan 9, 2016 at 3:02 AM, arnaud.mouiche@invoxia.com
>> <arnaud.mouiche@invoxia.com> wrote:
>>>
>>> Hello Caleb
>>>
>>> Le 09/01/2016 01:47, Caleb Crome a écrit :
>>>>
>>>> [...]
>>>>
>>>> Hello Arnaud,
>>>>     I have finally gotten to test your patches, and I'm still having
>>>> trouble with channel slips.
>>>>
>>>> I applied your v2 patch set, along with your changes for using a dummy
>>>> codec.
>>>>
>>>> The full changes are here:
>>>> https://github.com/ccrome/linux-caleb-dev/tree/v4.4-rc8-armv7-x3
>>>>
>>>> This ignores most of my previous patches, and uses your code to bring
>>>> up the SSI (without a codec) on a wandboard.
>>>>
>>>> I am using SSI3, and doing a hardware loopback between TX and RX.
>>>>
>>>> Here's what I run:
>>>> ./atest -r 16000 -c 8 -p 2048   -D default play
>>>>
>>>> which plays continuously.
>>>>
>>>> and in another shell:
>>>> ./atest -r 16000 -c 8 -p 2048  -D default -d 10 capture
>>>>
>>>> which captures for 10 seconds.
>>>>
>>>>
>>>> The first time I run the capture command, it succeeds, no problem.
>>>>>
>>>>> dbg: dev: 'default'
>>>>> dbg: default: capture_start
>>>>> dbg: start a 10 seconds duration timer
>>>>> warn: First valid frame
>>>>> warn:   3400 3401 3402 3403 3404 3405 3406 3407
>>>>> dbg: end of tests
>>>>> total number of sequence errors: 0
>>>>> global tests exit status: OK
>>>>
>>>> But the second and all subsequent captures, it fails with channel slips:
>>>>
>>>>> dbg: dev: 'default'
>>>>> dbg: default: capture_start
>>>>> dbg: start a 10 seconds duration timer
>>>>> err: invalid frame after 0 null frames
>>>>> err:   d2a1 d2a2 d2a3 d2a4 d2a5 d2a6 d2a7 d2c0
>>>>> err:   d2c1 d2c2 d2c3 d2c4 d2c5 d2c6 d2c7 78e0
>>>>> dbg: end of tests
>>>>> total number of sequence errors: 430080
>>>>> global tests exit status: OK
>>>
>>>
>>> Can you use -I option to get a little more log of the error
>>> $ ./atest -r 16000 -c 8 -p 2048  -D default -d 10 -I 10 capture
>>>
>>> Just to know if the wrong frames comes from the previous record session,
>>> meaning the RX fifo was not cleared correctly,
>>> as if the CCSR_SSI_SOR_RX_CLR bit is not working as we might expect.
>>> (ie. d2a1 .. d2c7 may come from the previous recording, and 78e0 .. may
>>> be
>>> the start of the new recording session)
>>
>> That does appear to be what's happening.  I read the SFCSR before and
>> after you update the SOR register (SOR_RX_CLR), and in both cases the
>> rx buffer is full on the second enablement.
>>
>> I can't seem to empty that buffer, either by using the SOR_RX_CLR or
>> by reading the SRX0 register. This is another one of those cases where
>> there you cannot modify the register (i.e. fifo or RX0) when RE is
>> disabled.
>>
>> So, instead of clearing on enable, I'm clearing on shutdown and on
>> enable.  I attempted using the SOR_RX_CLR, but it doesn't seem to work
>> on the MX6.  Rather, I clear the fifo by reading all the words in a
>> loop, just before RE is disabled.
>
> so the imx6.sl SSI behaves differently from the imx6.solo SSI ... Or some
> things have changes between 4.3 and 4.4.
>
> May be SOR_RX_CLR and/or RX0 registers can't be read on 'solo' because de
> the SRCR.RFEN0 [or1] are not enabled yet.
>
> In fact, clearing the fifos at the end may still introduce a race since the
> RX path is still enabled when your read from fifo, and
> Samples may still be received in the meantime...

You are correct about the race condition.  It seems like a catch 22:
you can't empty the fifo if SCR.RE is disabled, and if the SCR.RE is
enabled, new samples might be received at any time.  in fact, when
disabling RE, it does keep receiving until the end of the frame.

>
> Also, I see that the CCSR_SSI_SOR register is not in the regmap
> fsl_ssi_volatile_reg() list (line 140). May be there is an implication of
> some caching mechanism ?

Ah!  Yes.  When I add the SOR to the volatile list, then writing the
SOR does clear the FIFO.  That's one for the patch for sure :-)

Now at least RX get's cleared on RX start.

However, now I've got another issue:

FYI I'm running at 16 channels/16-bits/frame, so 256 bits/frame, with
the codec as master.  (codec 0 anyway).

I'm running atest play in one shell, and atest capture in another shell.

At low sample rates, it works quite reliably (a few hundred simulated
xruns with no problem), but TX slips at higher sample rates.

I'm running
    shell a # ./atest -r SAMPLERATE -c 16 -p 1024   -D default play
    shell b # ./atest -r SAMPLERATE -c 16 -p 1024   -D default -d 100
capture  -x200,100

8kHz:  No problems after 100 seconds (~300 capture xruns)
16kHz:  No problems
32kHz: tx slips after a few RX xruns (12) and never recovers.
48kHz:  tx starts properly (usually), but slips immediately upon RX start.

FYI, I'm watching TX on a scope, and can verify that it is in fact the
TX slipping.   I caught it once, and found that it was a duplicated
sample in the TX stream, which seems to say that the DMA is not
keeping up maybe?  Perhaps tweaking with watermarks will help that, or
perhaps dual-fifo will help.  Will try a couple things and get back to
you.

In the meantime, any idea how to check to see what speed the SSI
peripheral clock is running?  I just want to make sure it's going fast
enough.

here's a log of the failure at 32kHz:
---------------------------------------------------
<first verified on the scope that play is running without slips>
... 20 or 30 valid xruns...
ALSA lib pcm.c:7843:(snd_pcm_recover) overrun occurred
warn: First valid frame
warn:   49a0 49a1 49a2 49a3 49a4 49a5 49a6 49a7 49a8 49a9 49aa 49ab
49ac 49ad 49ae 49af
warn: default: force capture xrun
warn: default: CT_W4_XRUN_END
warn: default: capture read failed: Broken pipe
ALSA lib pcm.c:7843:(snd_pcm_recover) overrun occurred
warn: First valid frame
warn:   3a40 3a41 3a42 3a43 3a44 3a45 3a46 3a47 3a48 3a49 3a4a 3a4b
3a4c 3a4d 3a4e 3a4f
warn: default: force capture xrun
warn: default: CT_W4_XRUN_END
warn: default: capture read failed: Broken pipe
ALSA lib pcm.c:7843:(snd_pcm_recover) overrun occurred
warn: First valid frame
warn:   3240 3241 3242 3243 3244 3245 3246 3247 3248 3249 324a 324b
324c 324d 324e 324f
warn: first invalid frame while expecting frame 0x0617
warn:   c2e0 c2e1 c2e2 c2e3 c2e4 c2e5 c2e6 c2e6 c2e7 c2e8 c2e9 c2ea
c2eb c2ec c2ed c2ee
warn:   c2ef c300 c301 c302 c303 c304 c305 c306 c307 c308 c309 c30a
c30b c30c c30d c30e
warn:   c30f c320 c321 c322 c323 c324 c325 c326 c327 c328 c329 c32a
c32b c32c c32d c32e
warn:   c32f c340 c341 c342 c343 c344 c345 c346 c347 c348 c349 c34a
c34b c34c c34d c34e
err:   c34f c360 c361 c362 c363 c364 c365 c366 c367 c368 c369 c36a
c36b c36c c36d c36e
err:   c36f c380 c381 c382 c383 c384 c385 c386 c387 c388 c389 c38a
c38b c38c c38d c38e
err:   c38f c3a0 c3a1 c3a2 c3a3 c3a4 c3a5 c3a6 c3a7 c3a8 c3a9 c3aa
c3ab c3ac c3ad c3ae
err:   c3af c3c0 c3c1 c3c2 c3c3 c3c4 c3c5 c3c6 c3c7 c3c8 c3c9 c3ca
c3cb c3cc c3cd c3ce
err:   c3cf c3e0 c3e1 c3e2 c3e3 c3e4 c3e5 c3e6 c3e7 c3e8 c3e9 c3ea
c3eb c3ec c3ed c3ee
err:   c3ef c400 c401 c402 c403 c404 c405 c406 c407 c408 c409 c40a
c40b c40c c40d c40e
err:   c40f c420 c421 c422 c423 c424 c425 c426 c427 c428 c429 c42a
c42b c42c c42d c42e
warn: default: force capture xrun
warn: default: CT_W4_XRUN_END

As you can see, this is caused by duplicated c2e6, which I guess is a
DMA underrun?

here's a log of the failure at 48kHz:
---------------------------------------------------
<first verified on the scope that play is running without slips>
caleb@arm:~/atest$ ./atest -r 48000 -c 16 -p 1024   -D default -d 100
capture  -x200,100
dbg: dev: 'default'
dbg: default: capture_start
dbg: default: will simulate xrun every 200 ms
dbg: start a 100 seconds duration timer
err: invalid frame after 0 null frames
err:   9b2f 9b40 9b41 9b42 9b43 9b44 9b45 9b46 9b47 9b48 9b49 9b4a
9b4b 9b4c 9b4d 9b4e
err:   9b4f 9b60 9b61 9b62 9b63 9b64 9b65 9b66 9b67 9b68 9b69 9b6a
9b6b 9b6c 9b6d 9b6e
warn: default: force capture xrun
warn: default: CT_W4_XRUN_END
warn: default: capture read failed: Broken pipe
ALSA lib pcm.c:7843:(snd_pcm_recover) overrun occurred
err: invalid frame after 0 null frames

Thanks,
-Caleb
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 0/5] ASoC: fsl_ssi: Fixing various channel slips and bad samples insertions

[Caleb Crome]

On Wed, Jan 13, 2016 at 12:20 PM, Caleb Crome <caleb@crome.org> wrote:
> On Wed, Jan 13, 2016 at 6:45 AM, arnaud.mouiche@invoxia.com
> <arnaud.mouiche@invoxia.com> wrote:
>>
>>
>> Le 12/01/2016 00:44, Caleb Crome a écrit :
>>>
>>> On Sat, Jan 9, 2016 at 3:02 AM, arnaud.mouiche@invoxia.com
>>> <arnaud.mouiche@invoxia.com> wrote:
>>>>
>>>> Hello Caleb
>>>>
>>>> Le 09/01/2016 01:47, Caleb Crome a écrit :
>>>>>
>>>>> [...]
>>>>>
>>>>> Hello Arnaud,
>>>>>     I have finally gotten to test your patches, and I'm still having
>>>>> trouble with channel slips.
>>>>>
>>>>> I applied your v2 patch set, along with your changes for using a dummy
>>>>> codec.
>>>>>
>>>>> The full changes are here:
>>>>> https://github.com/ccrome/linux-caleb-dev/tree/v4.4-rc8-armv7-x3
>>>>>
>>>>> This ignores most of my previous patches, and uses your code to bring
>>>>> up the SSI (without a codec) on a wandboard.
>>>>>
>>>>> I am using SSI3, and doing a hardware loopback between TX and RX.
>>>>>
>>>>> Here's what I run:
>>>>> ./atest -r 16000 -c 8 -p 2048   -D default play
>>>>>
>>>>> which plays continuously.
>>>>>
>>>>> and in another shell:
>>>>> ./atest -r 16000 -c 8 -p 2048  -D default -d 10 capture
>>>>>
>>>>> which captures for 10 seconds.
>>>>>
>>>>>
>>>>> The first time I run the capture command, it succeeds, no problem.
>>>>>>
>>>>>> dbg: dev: 'default'
>>>>>> dbg: default: capture_start
>>>>>> dbg: start a 10 seconds duration timer
>>>>>> warn: First valid frame
>>>>>> warn:   3400 3401 3402 3403 3404 3405 3406 3407
>>>>>> dbg: end of tests
>>>>>> total number of sequence errors: 0
>>>>>> global tests exit status: OK
>>>>>
>>>>> But the second and all subsequent captures, it fails with channel slips:
>>>>>
>>>>>> dbg: dev: 'default'
>>>>>> dbg: default: capture_start
>>>>>> dbg: start a 10 seconds duration timer
>>>>>> err: invalid frame after 0 null frames
>>>>>> err:   d2a1 d2a2 d2a3 d2a4 d2a5 d2a6 d2a7 d2c0
>>>>>> err:   d2c1 d2c2 d2c3 d2c4 d2c5 d2c6 d2c7 78e0
>>>>>> dbg: end of tests
>>>>>> total number of sequence errors: 430080
>>>>>> global tests exit status: OK
>>>>
>>>>
>>>> Can you use -I option to get a little more log of the error
>>>> $ ./atest -r 16000 -c 8 -p 2048  -D default -d 10 -I 10 capture
>>>>
>>>> Just to know if the wrong frames comes from the previous record session,
>>>> meaning the RX fifo was not cleared correctly,
>>>> as if the CCSR_SSI_SOR_RX_CLR bit is not working as we might expect.
>>>> (ie. d2a1 .. d2c7 may come from the previous recording, and 78e0 .. may
>>>> be
>>>> the start of the new recording session)
>>>
>>> That does appear to be what's happening.  I read the SFCSR before and
>>> after you update the SOR register (SOR_RX_CLR), and in both cases the
>>> rx buffer is full on the second enablement.
>>>
>>> I can't seem to empty that buffer, either by using the SOR_RX_CLR or
>>> by reading the SRX0 register. This is another one of those cases where
>>> there you cannot modify the register (i.e. fifo or RX0) when RE is
>>> disabled.
>>>
>>> So, instead of clearing on enable, I'm clearing on shutdown and on
>>> enable.  I attempted using the SOR_RX_CLR, but it doesn't seem to work
>>> on the MX6.  Rather, I clear the fifo by reading all the words in a
>>> loop, just before RE is disabled.
>>
>> so the imx6.sl SSI behaves differently from the imx6.solo SSI ... Or some
>> things have changes between 4.3 and 4.4.
>>
>> May be SOR_RX_CLR and/or RX0 registers can't be read on 'solo' because de
>> the SRCR.RFEN0 [or1] are not enabled yet.
>>
>> In fact, clearing the fifos at the end may still introduce a race since the
>> RX path is still enabled when your read from fifo, and
>> Samples may still be received in the meantime...
>
> You are correct about the race condition.  It seems like a catch 22:
> you can't empty the fifo if SCR.RE is disabled, and if the SCR.RE is
> enabled, new samples might be received at any time.  in fact, when
> disabling RE, it does keep receiving until the end of the frame.
>
>>
>> Also, I see that the CCSR_SSI_SOR register is not in the regmap
>> fsl_ssi_volatile_reg() list (line 140). May be there is an implication of
>> some caching mechanism ?
>
> Ah!  Yes.  When I add the SOR to the volatile list, then writing the
> SOR does clear the FIFO.  That's one for the patch for sure :-)
>
> Now at least RX get's cleared on RX start.
>
> However, now I've got another issue:
>
> FYI I'm running at 16 channels/16-bits/frame, so 256 bits/frame, with
> the codec as master.  (codec 0 anyway).
>
> I'm running atest play in one shell, and atest capture in another shell.
>
> At low sample rates, it works quite reliably (a few hundred simulated
> xruns with no problem), but TX slips at higher sample rates.
>
> I'm running
>     shell a # ./atest -r SAMPLERATE -c 16 -p 1024   -D default play
>     shell b # ./atest -r SAMPLERATE -c 16 -p 1024   -D default -d 100
> capture  -x200,100
>
> 8kHz:  No problems after 100 seconds (~300 capture xruns)
> 16kHz:  No problems
> 32kHz: tx slips after a few RX xruns (12) and never recovers.
> 48kHz:  tx starts properly (usually), but slips immediately upon RX start.
>
> FYI, I'm watching TX on a scope, and can verify that it is in fact the
> TX slipping.   I caught it once, and found that it was a duplicated
> sample in the TX stream, which seems to say that the DMA is not
> keeping up maybe?  Perhaps tweaking with watermarks will help that, or
> perhaps dual-fifo will help.  Will try a couple things and get back to
> you.
>
> In the meantime, any idea how to check to see what speed the SSI
> peripheral clock is running?  I just want to make sure it's going fast
> enough.
>
> here's a log of the failure at 32kHz:
> ---------------------------------------------------
> <first verified on the scope that play is running without slips>
> ... 20 or 30 valid xruns...
> ALSA lib pcm.c:7843:(snd_pcm_recover) overrun occurred
> warn: First valid frame
> warn:   49a0 49a1 49a2 49a3 49a4 49a5 49a6 49a7 49a8 49a9 49aa 49ab
> 49ac 49ad 49ae 49af
> warn: default: force capture xrun
> warn: default: CT_W4_XRUN_END
> warn: default: capture read failed: Broken pipe
> ALSA lib pcm.c:7843:(snd_pcm_recover) overrun occurred
> warn: First valid frame
> warn:   3a40 3a41 3a42 3a43 3a44 3a45 3a46 3a47 3a48 3a49 3a4a 3a4b
> 3a4c 3a4d 3a4e 3a4f
> warn: default: force capture xrun
> warn: default: CT_W4_XRUN_END
> warn: default: capture read failed: Broken pipe
> ALSA lib pcm.c:7843:(snd_pcm_recover) overrun occurred
> warn: First valid frame
> warn:   3240 3241 3242 3243 3244 3245 3246 3247 3248 3249 324a 324b
> 324c 324d 324e 324f
> warn: first invalid frame while expecting frame 0x0617
> warn:   c2e0 c2e1 c2e2 c2e3 c2e4 c2e5 c2e6 c2e6 c2e7 c2e8 c2e9 c2ea
> c2eb c2ec c2ed c2ee
> warn:   c2ef c300 c301 c302 c303 c304 c305 c306 c307 c308 c309 c30a
> c30b c30c c30d c30e
> warn:   c30f c320 c321 c322 c323 c324 c325 c326 c327 c328 c329 c32a
> c32b c32c c32d c32e
> warn:   c32f c340 c341 c342 c343 c344 c345 c346 c347 c348 c349 c34a
> c34b c34c c34d c34e
> err:   c34f c360 c361 c362 c363 c364 c365 c366 c367 c368 c369 c36a
> c36b c36c c36d c36e
> err:   c36f c380 c381 c382 c383 c384 c385 c386 c387 c388 c389 c38a
> c38b c38c c38d c38e
> err:   c38f c3a0 c3a1 c3a2 c3a3 c3a4 c3a5 c3a6 c3a7 c3a8 c3a9 c3aa
> c3ab c3ac c3ad c3ae
> err:   c3af c3c0 c3c1 c3c2 c3c3 c3c4 c3c5 c3c6 c3c7 c3c8 c3c9 c3ca
> c3cb c3cc c3cd c3ce
> err:   c3cf c3e0 c3e1 c3e2 c3e3 c3e4 c3e5 c3e6 c3e7 c3e8 c3e9 c3ea
> c3eb c3ec c3ed c3ee
> err:   c3ef c400 c401 c402 c403 c404 c405 c406 c407 c408 c409 c40a
> c40b c40c c40d c40e
> err:   c40f c420 c421 c422 c423 c424 c425 c426 c427 c428 c429 c42a
> c42b c42c c42d c42e
> warn: default: force capture xrun
> warn: default: CT_W4_XRUN_END
>
> As you can see, this is caused by duplicated c2e6, which I guess is a
> DMA underrun?
>


SUCCESS!  So far...

With your patches (including the latest SOR register update), plus
setting the watermark & DMA MAXBURST to 8, I don't get any more errors
at 48kHz ...  yet.

Even in single fifo mode, 48kHz, 16-bits+16channels seems to work now.

I'll keep you updated on if this really solves all the issues.
Here's the last patch for updating the watermark.

commit b634014b831b9527df319b404ac50e54a3790742
Author: Caleb Crome <caleb@crome.org>
Date:   Wed Jan 13 13:12:37 2016 -0800

    ASoC: fsl_ssi: Increase watermark and maxburst to allow SSI to work
    without slips at high data rates.

    The DMA cannot keep up with the SSI consumpation with the watermark
    set to be fifo_depth-2 when running at 48kHz/16-bits/16-channels
    (12288 words/second).  By increasing the watermark to 8, the DMA can
    keep up with the SSI.

    Signed-off-by: Caleb Crome <caleb@crome.org>

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 5cfc540..026df79 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -221,6 +221,12 @@ struct fsl_ssi_soc_data {
  * @dbg_stats: Debugging statistics
  *
  * @soc: SoC specific data
+ *
+ * @fifo_watermark: the FIFO watermark setting.  Notifies DMA when
+ *             there are @fifo_watermark or fewer words in TX fifo or
+ *             @fifo_watermark or more empty words in RX fifo.
+ * @dma_maxburst: max number of words to transfer in one go.  So far,
+ *             this is always the same as fifo_watermark.
  */
 struct fsl_ssi_private {
     struct regmap *regs;
@@ -259,6 +265,9 @@ struct fsl_ssi_private {

     const struct fsl_ssi_soc_data *soc;
     struct device *dev;
+
+    u32 fifo_watermark;
+    u32 dma_maxburst;
 };

 /*
@@ -1037,21 +1046,7 @@ static int _fsl_ssi_set_dai_fmt(struct device *dev,
     regmap_write(regs, CCSR_SSI_SRCR, srcr);
     regmap_write(regs, CCSR_SSI_SCR, scr);

-    /*
-     * Set the watermark for transmit FIFI 0 and receive FIFO 0. We don't
-     * use FIFO 1. We program the transmit water to signal a DMA transfer
-     * if there are only two (or fewer) elements left in the FIFO. Two
-     * elements equals one frame (left channel, right channel). This value,
-     * however, depends on the depth of the transmit buffer.
-     *
-     * We set the watermark on the same level as the DMA burstsize.  For
-     * fiq it is probably better to use the biggest possible watermark
-     * size.
-     */
-    if (ssi_private->use_dma)
-        wm = ssi_private->fifo_depth - 2;
-    else
-        wm = ssi_private->fifo_depth;
+    wm = ssi_private->watermark;

     regmap_write(regs, CCSR_SSI_SFCSR,
             CCSR_SSI_SFCSR_TFWM0(wm) | CCSR_SSI_SFCSR_RFWM0(wm) |
@@ -1359,12 +1354,8 @@ static int fsl_ssi_imx_probe(struct
platform_device *pdev,
         dev_dbg(&pdev->dev, "could not get baud clock: %ld\n",
              PTR_ERR(ssi_private->baudclk));

-    /*
-     * We have burstsize be "fifo_depth - 2" to match the SSI
-     * watermark setting in fsl_ssi_startup().
-     */
-    ssi_private->dma_params_tx.maxburst = ssi_private->fifo_depth - 2;
-    ssi_private->dma_params_rx.maxburst = ssi_private->fifo_depth - 2;
+    ssi_private->dma_params_tx.maxburst = ssi_private->dma_maxburst;
+    ssi_private->dma_params_rx.maxburst = ssi_private->dma_maxburst;
     ssi_private->dma_params_tx.addr = ssi_private->ssi_phys + CCSR_SSI_STX0;
     ssi_private->dma_params_rx.addr = ssi_private->ssi_phys + CCSR_SSI_SRX0;

@@ -1518,6 +1509,42 @@ static int fsl_ssi_probe(struct platform_device *pdev)
                 /* Older 8610 DTs didn't have the fifo-depth property */
         ssi_private->fifo_depth = 8;

+    /*
+     * Set the watermark for transmit FIFI 0 and receive FIFO 0. We don't
+     * use FIFO 1. We program the transmit water to signal a DMA transfer
+     * if there are only two (or fewer) elements left in the FIFO. Two
+     * elements equals one frame (left channel, right channel). This value,
+     * however, depends on the depth of the transmit buffer.
+     *
+     * We set the watermark on the same level as the DMA burstsize.  For
+     * fiq it is probably better to use the biggest possible watermark
+     * size.
+     */
+    switch (ssi_private->fifo_depth) {
+    case 15:
+        /*
+         * 2 samples is not enough when running at high data
+         * rates (like 48kHz @ 16 bits/channel, 16 channels)
+         * 8 seems to split things evenly and leave enough time
+         * for the DMA to fill the FIFO before it's over/under
+         * run.
+         */
+        ssi_private->fifo_watermark = 8;
+        ssi_private->dma_maxburst = 8;
+    case 8:
+    default:
+        /*
+         * maintain old behavior for older chips.
+         * Keeping it the same because I don't have an older
+         * board to test with.
+         * I suspect this could be changed to be something to
+         * leave some more space in the fifo.
+         */
+        ssi_private->fifo_watermark = ssi_private->fifo_depth - 2;
+        ssi_private->dma_maxburst = ssi_private->fifo_depth - 2;
+        break;
+    }
+
     dev_set_drvdata(&pdev->dev, ssi_private);

     if (ssi_private->soc->imx) {
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 0/5] ASoC: fsl_ssi: Fixing various channel slips and bad samples insertions

[arnaud.mouiche]

[..]
>
> SUCCESS!  So far...
Great :)

I'm preparing a v3 of my patches including the SOR register + rebased on 
top of v4.4.
I will let you propose the water mark / maxburst patch.
But it looks obvious to me that triggering the DMA when only 2 words are 
left in the FIFO can lead to DMA xruns at such data rate.

The downside is an increased number of DMA requests.
So I don't know if you should propose a configuration through the device 
tree, or a static configuration as done in your patch.

Arnaud
>
> With your patches (including the latest SOR register update), plus
> setting the watermark & DMA MAXBURST to 8, I don't get any more errors
> at 48kHz ...  yet.
>
> Even in single fifo mode, 48kHz, 16-bits+16channels seems to work now.
>
> I'll keep you updated on if this really solves all the issues.
> Here's the last patch for updating the watermark.
>
> commit b634014b831b9527df319b404ac50e54a3790742
> Author: Caleb Crome <caleb@crome.org>
> Date:   Wed Jan 13 13:12:37 2016 -0800
>
>      ASoC: fsl_ssi: Increase watermark and maxburst to allow SSI to work
>      without slips at high data rates.
>
>      The DMA cannot keep up with the SSI consumpation with the watermark
>      set to be fifo_depth-2 when running at 48kHz/16-bits/16-channels
>      (12288 words/second).  By increasing the watermark to 8, the DMA can
>      keep up with the SSI.
>
>      Signed-off-by: Caleb Crome <caleb@crome.org>
>
> diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
> index 5cfc540..026df79 100644
> --- a/sound/soc/fsl/fsl_ssi.c
> +++ b/sound/soc/fsl/fsl_ssi.c
> @@ -221,6 +221,12 @@ struct fsl_ssi_soc_data {
>    * @dbg_stats: Debugging statistics
>    *
>    * @soc: SoC specific data
> + *
> + * @fifo_watermark: the FIFO watermark setting.  Notifies DMA when
> + *             there are @fifo_watermark or fewer words in TX fifo or
> + *             @fifo_watermark or more empty words in RX fifo.
> + * @dma_maxburst: max number of words to transfer in one go.  So far,
> + *             this is always the same as fifo_watermark.
>    */
>   struct fsl_ssi_private {
>       struct regmap *regs;
> @@ -259,6 +265,9 @@ struct fsl_ssi_private {
>
>       const struct fsl_ssi_soc_data *soc;
>       struct device *dev;
> +
> +    u32 fifo_watermark;
> +    u32 dma_maxburst;
>   };
>
>   /*
> @@ -1037,21 +1046,7 @@ static int _fsl_ssi_set_dai_fmt(struct device *dev,
>       regmap_write(regs, CCSR_SSI_SRCR, srcr);
>       regmap_write(regs, CCSR_SSI_SCR, scr);
>
> -    /*
> -     * Set the watermark for transmit FIFI 0 and receive FIFO 0. We don't
> -     * use FIFO 1. We program the transmit water to signal a DMA transfer
> -     * if there are only two (or fewer) elements left in the FIFO. Two
> -     * elements equals one frame (left channel, right channel). This value,
> -     * however, depends on the depth of the transmit buffer.
> -     *
> -     * We set the watermark on the same level as the DMA burstsize.  For
> -     * fiq it is probably better to use the biggest possible watermark
> -     * size.
> -     */
> -    if (ssi_private->use_dma)
> -        wm = ssi_private->fifo_depth - 2;
> -    else
> -        wm = ssi_private->fifo_depth;
> +    wm = ssi_private->watermark;
>
>       regmap_write(regs, CCSR_SSI_SFCSR,
>               CCSR_SSI_SFCSR_TFWM0(wm) | CCSR_SSI_SFCSR_RFWM0(wm) |
> @@ -1359,12 +1354,8 @@ static int fsl_ssi_imx_probe(struct
> platform_device *pdev,
>           dev_dbg(&pdev->dev, "could not get baud clock: %ld\n",
>                PTR_ERR(ssi_private->baudclk));
>
> -    /*
> -     * We have burstsize be "fifo_depth - 2" to match the SSI
> -     * watermark setting in fsl_ssi_startup().
> -     */
> -    ssi_private->dma_params_tx.maxburst = ssi_private->fifo_depth - 2;
> -    ssi_private->dma_params_rx.maxburst = ssi_private->fifo_depth - 2;
> +    ssi_private->dma_params_tx.maxburst = ssi_private->dma_maxburst;
> +    ssi_private->dma_params_rx.maxburst = ssi_private->dma_maxburst;
>       ssi_private->dma_params_tx.addr = ssi_private->ssi_phys + CCSR_SSI_STX0;
>       ssi_private->dma_params_rx.addr = ssi_private->ssi_phys + CCSR_SSI_SRX0;
>
> @@ -1518,6 +1509,42 @@ static int fsl_ssi_probe(struct platform_device *pdev)
>                   /* Older 8610 DTs didn't have the fifo-depth property */
>           ssi_private->fifo_depth = 8;
>
> +    /*
> +     * Set the watermark for transmit FIFI 0 and receive FIFO 0. We don't
> +     * use FIFO 1. We program the transmit water to signal a DMA transfer
> +     * if there are only two (or fewer) elements left in the FIFO. Two
> +     * elements equals one frame (left channel, right channel). This value,
> +     * however, depends on the depth of the transmit buffer.
> +     *
> +     * We set the watermark on the same level as the DMA burstsize.  For
> +     * fiq it is probably better to use the biggest possible watermark
> +     * size.
> +     */
> +    switch (ssi_private->fifo_depth) {
> +    case 15:
> +        /*
> +         * 2 samples is not enough when running at high data
> +         * rates (like 48kHz @ 16 bits/channel, 16 channels)
> +         * 8 seems to split things evenly and leave enough time
> +         * for the DMA to fill the FIFO before it's over/under
> +         * run.
> +         */
> +        ssi_private->fifo_watermark = 8;
> +        ssi_private->dma_maxburst = 8;
> +    case 8:
> +    default:
> +        /*
> +         * maintain old behavior for older chips.
> +         * Keeping it the same because I don't have an older
> +         * board to test with.
> +         * I suspect this could be changed to be something to
> +         * leave some more space in the fifo.
> +         */
> +        ssi_private->fifo_watermark = ssi_private->fifo_depth - 2;
> +        ssi_private->dma_maxburst = ssi_private->fifo_depth - 2;
> +        break;
> +    }
> +
>       dev_set_drvdata(&pdev->dev, ssi_private);
>
>       if (ssi_private->soc->imx) {

Re: [PATCH 0/5] ASoC: fsl_ssi: Fixing various channel slips and bad samples insertions

[Caleb Crome]

On Thu, Jan 14, 2016 at 12:40 AM, arnaud.mouiche@invoxia.com
<arnaud.mouiche@invoxia.com> wrote:
>
> [..]
>>
>>
>> SUCCESS!  So far...
>
> Great :)
>
> I'm preparing a v3 of my patches including the SOR register + rebased on top
> of v4.4.
> I will let you propose the water mark / maxburst patch.
> But it looks obvious to me that triggering the DMA when only 2 words are
> left in the FIFO can lead to DMA xruns at such data rate.
>
> The downside is an increased number of DMA requests.
> So I don't know if you should propose a configuration through the device
> tree, or a static configuration as done in your patch.
>

Sounds to me like the device tree is the right place for it. I'll submit an RFC.

For some reason... looks like it can't yet quite keep up at 48kHz.  it
was working reliably for a while, but now I can only get it reliable
at 32kHz.  It seems it's the TX that's broken at 48k.

I'll dig in to see where the failure is.

-C

> Arnaud
>
>>
>> With your patches (including the latest SOR register update), plus
>> setting the watermark & DMA MAXBURST to 8, I don't get any more errors
>> at 48kHz ...  yet.
>>
>> Even in single fifo mode, 48kHz, 16-bits+16channels seems to work now.
>>
>> I'll keep you updated on if this really solves all the issues.
>> Here's the last patch for updating the watermark.
>>
>> commit b634014b831b9527df319b404ac50e54a3790742
>> Author: Caleb Crome <caleb@crome.org>
>> Date:   Wed Jan 13 13:12:37 2016 -0800
>>
>>      ASoC: fsl_ssi: Increase watermark and maxburst to allow SSI to work
>>      without slips at high data rates.
>>
>>      The DMA cannot keep up with the SSI consumpation with the watermark
>>      set to be fifo_depth-2 when running at 48kHz/16-bits/16-channels
>>      (12288 words/second).  By increasing the watermark to 8, the DMA can
>>      keep up with the SSI.
>>
>>      Signed-off-by: Caleb Crome <caleb@crome.org>
>>
>> diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
>> index 5cfc540..026df79 100644
>> --- a/sound/soc/fsl/fsl_ssi.c
>> +++ b/sound/soc/fsl/fsl_ssi.c
>> @@ -221,6 +221,12 @@ struct fsl_ssi_soc_data {
>>    * @dbg_stats: Debugging statistics
>>    *
>>    * @soc: SoC specific data
>> + *
>> + * @fifo_watermark: the FIFO watermark setting.  Notifies DMA when
>> + *             there are @fifo_watermark or fewer words in TX fifo or
>> + *             @fifo_watermark or more empty words in RX fifo.
>> + * @dma_maxburst: max number of words to transfer in one go.  So far,
>> + *             this is always the same as fifo_watermark.
>>    */
>>   struct fsl_ssi_private {
>>       struct regmap *regs;
>> @@ -259,6 +265,9 @@ struct fsl_ssi_private {
>>
>>       const struct fsl_ssi_soc_data *soc;
>>       struct device *dev;
>> +
>> +    u32 fifo_watermark;
>> +    u32 dma_maxburst;
>>   };
>>
>>   /*
>> @@ -1037,21 +1046,7 @@ static int _fsl_ssi_set_dai_fmt(struct device *dev,
>>       regmap_write(regs, CCSR_SSI_SRCR, srcr);
>>       regmap_write(regs, CCSR_SSI_SCR, scr);
>>
>> -    /*
>> -     * Set the watermark for transmit FIFI 0 and receive FIFO 0. We don't
>> -     * use FIFO 1. We program the transmit water to signal a DMA transfer
>> -     * if there are only two (or fewer) elements left in the FIFO. Two
>> -     * elements equals one frame (left channel, right channel). This
>> value,
>> -     * however, depends on the depth of the transmit buffer.
>> -     *
>> -     * We set the watermark on the same level as the DMA burstsize.  For
>> -     * fiq it is probably better to use the biggest possible watermark
>> -     * size.
>> -     */
>> -    if (ssi_private->use_dma)
>> -        wm = ssi_private->fifo_depth - 2;
>> -    else
>> -        wm = ssi_private->fifo_depth;
>> +    wm = ssi_private->watermark;
>>
>>       regmap_write(regs, CCSR_SSI_SFCSR,
>>               CCSR_SSI_SFCSR_TFWM0(wm) | CCSR_SSI_SFCSR_RFWM0(wm) |
>> @@ -1359,12 +1354,8 @@ static int fsl_ssi_imx_probe(struct
>> platform_device *pdev,
>>           dev_dbg(&pdev->dev, "could not get baud clock: %ld\n",
>>                PTR_ERR(ssi_private->baudclk));
>>
>> -    /*
>> -     * We have burstsize be "fifo_depth - 2" to match the SSI
>> -     * watermark setting in fsl_ssi_startup().
>> -     */
>> -    ssi_private->dma_params_tx.maxburst = ssi_private->fifo_depth - 2;
>> -    ssi_private->dma_params_rx.maxburst = ssi_private->fifo_depth - 2;
>> +    ssi_private->dma_params_tx.maxburst = ssi_private->dma_maxburst;
>> +    ssi_private->dma_params_rx.maxburst = ssi_private->dma_maxburst;
>>       ssi_private->dma_params_tx.addr = ssi_private->ssi_phys +
>> CCSR_SSI_STX0;
>>       ssi_private->dma_params_rx.addr = ssi_private->ssi_phys +
>> CCSR_SSI_SRX0;
>>
>> @@ -1518,6 +1509,42 @@ static int fsl_ssi_probe(struct platform_device
>> *pdev)
>>                   /* Older 8610 DTs didn't have the fifo-depth property */
>>           ssi_private->fifo_depth = 8;
>>
>> +    /*
>> +     * Set the watermark for transmit FIFI 0 and receive FIFO 0. We don't
>> +     * use FIFO 1. We program the transmit water to signal a DMA transfer
>> +     * if there are only two (or fewer) elements left in the FIFO. Two
>> +     * elements equals one frame (left channel, right channel). This
>> value,
>> +     * however, depends on the depth of the transmit buffer.
>> +     *
>> +     * We set the watermark on the same level as the DMA burstsize.  For
>> +     * fiq it is probably better to use the biggest possible watermark
>> +     * size.
>> +     */
>> +    switch (ssi_private->fifo_depth) {
>> +    case 15:
>> +        /*
>> +         * 2 samples is not enough when running at high data
>> +         * rates (like 48kHz @ 16 bits/channel, 16 channels)
>> +         * 8 seems to split things evenly and leave enough time
>> +         * for the DMA to fill the FIFO before it's over/under
>> +         * run.
>> +         */
>> +        ssi_private->fifo_watermark = 8;
>> +        ssi_private->dma_maxburst = 8;
>> +    case 8:
>> +    default:
>> +        /*
>> +         * maintain old behavior for older chips.
>> +         * Keeping it the same because I don't have an older
>> +         * board to test with.
>> +         * I suspect this could be changed to be something to
>> +         * leave some more space in the fifo.
>> +         */
>> +        ssi_private->fifo_watermark = ssi_private->fifo_depth - 2;
>> +        ssi_private->dma_maxburst = ssi_private->fifo_depth - 2;
>> +        break;
>> +    }
>> +
>>       dev_set_drvdata(&pdev->dev, ssi_private);
>>
>>       if (ssi_private->soc->imx) {
>
>

Re: [PATCH 0/5] ASoC: fsl_ssi: Fixing various channel slips and bad samples insertions

[Caleb Crome]

On Thu, Jan 14, 2016 at 6:25 AM, Caleb Crome <caleb@crome.org> wrote:
> On Thu, Jan 14, 2016 at 12:40 AM, arnaud.mouiche@invoxia.com
> <arnaud.mouiche@invoxia.com> wrote:
>>
>> [..]
>>>
>>>
>>> SUCCESS!  So far...
>>
>> Great :)
>>
>> I'm preparing a v3 of my patches including the SOR register + rebased on top
>> of v4.4.
>> I will let you propose the water mark / maxburst patch.
>> But it looks obvious to me that triggering the DMA when only 2 words are
>> left in the FIFO can lead to DMA xruns at such data rate.
>>
>> The downside is an increased number of DMA requests.
>> So I don't know if you should propose a configuration through the device
>> tree, or a static configuration as done in your patch.
>>
>
> Sounds to me like the device tree is the right place for it. I'll submit an RFC.
>
> For some reason... looks like it can't yet quite keep up at 48kHz.  it
> was working reliably for a while, but now I can only get it reliable
> at 32kHz.  It seems it's the TX that's broken at 48k.
>
> I'll dig in to see where the failure is.
>
> -C

Okay, setting maxburst and  watermark to 4 instead of 8 seems to solve
the problem at 48kHz.

-Caleb

Applied "ASoC: fsl_ssi: Fix samples being dropped at Playback startup" to the asoc tree

[Mark Brown]

The patch

   ASoC: fsl_ssi: Fix samples being dropped at Playback startup

has been applied to the asoc tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From d9f2a202877c15818d98268f47d6b4bcfcb84437 Mon Sep 17 00:00:00 2001
From: Arnaud Mouiche <arnaud.mouiche@invoxia.com>
Date: Tue, 3 May 2016 14:13:58 +0200
Subject: [PATCH] ASoC: fsl_ssi: Fix samples being dropped at Playback startup

If the capture is already running while playback is started, it is highly
probable (>80% in a 8 channels scenario) that samples are lost between
the DMA and TX fifo.

The reason is that SIER.TDMAE is set before STCR.TFEN0, leaving a time
window where the FIFO doesn't receive the samples written by the DMA.

This particular case happened only if capture is already enabled as
SCR.SSIEN is already set at the playback startup instant.

Signed-off-by: Arnaud Mouiche <arnaud.mouiche@invoxia.com>
Reviewed-by: Fabio Estevam <fabio.estevam@nxp.com>
Tested-by: Caleb Crome <caleb@crome.org>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/fsl/fsl_ssi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 149df3ca4f5e..47ebb835f3f5 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -475,9 +475,9 @@ static void fsl_ssi_config(struct fsl_ssi_private *ssi_private, bool enable,
 	 * (online configuration)
 	 */
 	if (enable) {
-		regmap_update_bits(regs, CCSR_SSI_SIER, vals->sier, vals->sier);
 		regmap_update_bits(regs, CCSR_SSI_SRCR, vals->srcr, vals->srcr);
 		regmap_update_bits(regs, CCSR_SSI_STCR, vals->stcr, vals->stcr);
+		regmap_update_bits(regs, CCSR_SSI_SIER, vals->sier, vals->sier);
 	} else {
 		u32 sier;
 		u32 srcr;
-- 
2.8.1

Applied "ASoC: fsl_ssi: Save a dev reference for dev_err() purpose." to the asoc tree

[Mark Brown]

The patch

   ASoC: fsl_ssi: Save a dev reference for dev_err() purpose.

has been applied to the asoc tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From 0096b693962d3abde4f41b13cf03c765f33e9d8d Mon Sep 17 00:00:00 2001
From: Arnaud Mouiche <arnaud.mouiche@invoxia.com>
Date: Tue, 3 May 2016 14:13:57 +0200
Subject: [PATCH] ASoC: fsl_ssi: Save a dev reference for dev_err() purpose.

Most of functions only receive the ssi_private reference and don't have
a knowledge of 'dev' pointer, even for debug purpose.

Signed-off-by: Arnaud Mouiche <arnaud.mouiche@invoxia.com>
Tested-by: Caleb Crome <caleb@crome.org>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/fsl/fsl_ssi.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 86229c8902d2..149df3ca4f5e 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -261,6 +261,7 @@ struct fsl_ssi_private {
 	struct fsl_ssi_dbg dbg_stats;
 
 	const struct fsl_ssi_soc_data *soc;
+	struct device *dev;
 };
 
 /*
@@ -1404,6 +1405,7 @@ static int fsl_ssi_probe(struct platform_device *pdev)
 	}
 
 	ssi_private->soc = of_id->data;
+	ssi_private->dev = &pdev->dev;
 
 	sprop = of_get_property(np, "fsl,mode", NULL);
 	if (sprop) {
-- 
2.8.1

Applied "ASoC: fsl_ssi: The IPG/5 limitation concerns the bitclk, not the sysclk." to the asoc tree

[Mark Brown]

The patch

   ASoC: fsl_ssi: The IPG/5 limitation concerns the bitclk, not the sysclk.

has been applied to the asoc tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From e09745f2e6a1f692fc63db01850aacf025475aad Mon Sep 17 00:00:00 2001
From: Arnaud Mouiche <arnaud.mouiche@invoxia.com>
Date: Tue, 3 May 2016 14:13:56 +0200
Subject: [PATCH] ASoC: fsl_ssi: The IPG/5 limitation concerns the bitclk, not
 the sysclk.

im6sl reference manual 47.7.4:
"
Bit clock - Used to serially clock the data bits in and out of the SSI port.
This clock is either generated internally (from SSI's sys clock) or taken
from external clock source (through the Tx/Rx clock ports).
[...]
Care should be taken to ensure that the bit clock frequency (either
internally generated by dividing the SSI's sys clock or sourced from
external device through Tx/Rx clock ports) is never greater than 1/5
of the ipg_clk (from CCM) frequency.
"

Since, in master mode, the sysclk is a multiple of bitclk, we can
easily reach a high sysclk value, whereas keeping a reasonable bitclk.

ex: 8ch x 16bit x 48kHz = 6144000, requires a 24576000 sysclk (PM=1)
    yet ipg_clk/5 = 66Mhz/5 = 13.2

Signed-off-by: Arnaud Mouiche <arnaud.mouiche@invoxia.com>
Reviewed-by: Fabio Estevam <fabio.estevam@nxp.com>
Tested-by: Caleb Crome <caleb@crome.org>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/fsl/fsl_ssi.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 8d5f3c192de2..86229c8902d2 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -670,6 +670,15 @@ static int fsl_ssi_set_bclk(struct snd_pcm_substream *substream,
 	if (IS_ERR(ssi_private->baudclk))
 		return -EINVAL;
 
+	/*
+	 * Hardware limitation: The bclk rate must be
+	 * never greater than 1/5 IPG clock rate
+	 */
+	if (freq * 5 > clk_get_rate(ssi_private->clk)) {
+		dev_err(cpu_dai->dev, "bitclk > ipgclk/5\n");
+		return -EINVAL;
+	}
+
 	baudclk_is_used = ssi_private->baudclk_streams & ~(BIT(substream->stream));
 
 	/* It should be already enough to divide clock by setting pm alone */
@@ -686,13 +695,6 @@ static int fsl_ssi_set_bclk(struct snd_pcm_substream *substream,
 		else
 			clkrate = clk_round_rate(ssi_private->baudclk, tmprate);
 
-		/*
-		 * Hardware limitation: The bclk rate must be
-		 * never greater than 1/5 IPG clock rate
-		 */
-		if (clkrate * 5 > clk_get_rate(ssi_private->clk))
-			continue;
-
 		clkrate /= factor;
 		afreq = clkrate / (i + 1);
 
-- 
2.8.1