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

[PATCH v3 0/6] 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.


Version 3:
- rebase + fix + test on top of linux 4.4 + 'for-next'

Version 2:
- fixing a missing patch 
- checkpatch.pl
- ... well, learning how to send a patchset. Sorry. 


Bugs are detected and fixed on a imx6sl platform with linux 4.4 + 'for-next' branch, where 2 SSI interfaces are used. 
SSI3 configured as a master of the bus, SSI2 as a slave.
Similar tests were done on a imx6solo platform by Caleb Crome (He will propose a patch concerning fifo depth to fix DMA xrun at highest bitrates)


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 (6):
  ASoC: fsl_ssi: Real hardware channels max number is 32
  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 | 81 ++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 70 insertions(+), 11 deletions(-)

-- 
1.9.1

[PATCH v3 1/6] ASoC: fsl_ssi: Real hardware channels max number is 32

[Arnaud Mouiche]

The max number of slots in TDM mode is 32:
- Frame Rate Divider Control is a 5bit value
- Time slot mask registers control 32 slots.

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

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 40dfd8a..cfc78b8 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -1167,14 +1167,14 @@ static struct snd_soc_dai_driver fsl_ssi_dai_template = {
 	.playback = {
 		.stream_name = "CPU-Playback",
 		.channels_min = 1,
-		.channels_max = 2,
+		.channels_max = 32,
 		.rates = FSLSSI_I2S_RATES,
 		.formats = FSLSSI_I2S_FORMATS,
 	},
 	.capture = {
 		.stream_name = "CPU-Capture",
 		.channels_min = 1,
-		.channels_max = 2,
+		.channels_max = 32,
 		.rates = FSLSSI_I2S_RATES,
 		.formats = FSLSSI_I2S_FORMATS,
 	},
-- 
1.9.1

[PATCH v3 2/6] 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 cfc78b8..dbd5f10 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -679,6 +679,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 */
@@ -695,13 +704,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 v3 3/6] 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 dbd5f10..13613fd 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -275,6 +275,7 @@ struct fsl_ssi_private {
 	struct fsl_ssi_dbg dbg_stats;
 
 	const struct fsl_ssi_soc_data *soc;
+	struct device *dev;
 };
 
 /*
@@ -1412,6 +1413,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 v3 4/6] 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 13613fd..c9ba8ca 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -488,9 +488,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 v3 5/6] 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 | 34 +++++++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index c9ba8ca..530d592 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -520,8 +520,40 @@ 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 v3 6/6] 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 and IMX6solo.

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

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 530d592..50218ed 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -146,6 +146,7 @@ static bool fsl_ssi_volatile_reg(struct device *dev, unsigned int reg)
 	case CCSR_SSI_SRX1:
 	case CCSR_SSI_SISR:
 	case CCSR_SSI_SFCSR:
+	case CCSR_SSI_SOR:
 	case CCSR_SSI_SACNT:
 	case CCSR_SSI_SACADD:
 	case CCSR_SSI_SACDAT:
@@ -414,6 +415,26 @@ static void fsl_ssi_rxtx_config(struct fsl_ssi_private *ssi_private,
 }
 
 /*
+ * 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.
+ */
+static void fsl_ssi_fifo_clear(struct fsl_ssi_private *ssi_private,
+		bool is_rx)
+{
+	if (is_rx) {
+		regmap_update_bits(ssi_private->regs, CCSR_SSI_SOR,
+			CCSR_SSI_SOR_RX_CLR, CCSR_SSI_SOR_RX_CLR);
+	} else {
+		regmap_update_bits(ssi_private->regs, CCSR_SSI_SOR,
+			CCSR_SSI_SOR_TX_CLR, CCSR_SSI_SOR_TX_CLR);
+	}
+}
+
+/*
  * Calculate the bits that have to be disabled for the current stream that is
  * getting disabled. This keeps the bits enabled that are necessary for the
  * second stream to work if 'stream_active' is true.
@@ -488,6 +509,8 @@ static void fsl_ssi_config(struct fsl_ssi_private *ssi_private, bool enable,
 	 * (online configuration)
 	 */
 	if (enable) {
+		fsl_ssi_fifo_clear(ssi_private, vals->scr & CCSR_SSI_SCR_RE);
+
 		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 v3 1/6] ASoC: fsl_ssi: Real hardware channels max number is 32

[Timur Tabi]

Arnaud Mouiche wrote:
> The max number of slots in TDM mode is 32:
> - Frame Rate Divider Control is a 5bit value
> - Time slot mask registers control 32 slots.
>
> Signed-off-by: Arnaud Mouiche<arnaud.mouiche@invoxia.com>
> ---
>   sound/soc/fsl/fsl_ssi.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
> index 40dfd8a..cfc78b8 100644
> --- a/sound/soc/fsl/fsl_ssi.c
> +++ b/sound/soc/fsl/fsl_ssi.c
> @@ -1167,14 +1167,14 @@ static struct snd_soc_dai_driver fsl_ssi_dai_template = {
>   	.playback = {
>   		.stream_name = "CPU-Playback",
>   		.channels_min = 1,
> -		.channels_max = 2,
> +		.channels_max = 32,

TDM mode is not normally used.  What happens if we're not in TDM mode, 
and the user tries to start a stream with more than 2 channels?

Re: [PATCH v3 1/6] ASoC: fsl_ssi: Real hardware channels max number is 32

[arnaud.mouiche]

Le 18/01/2016 16:46, Timur Tabi a écrit :
> Arnaud Mouiche wrote:
>> The max number of slots in TDM mode is 32:
>> - Frame Rate Divider Control is a 5bit value
>> - Time slot mask registers control 32 slots.
>>
>> Signed-off-by: Arnaud Mouiche<arnaud.mouiche@invoxia.com>
>> ---
>>   sound/soc/fsl/fsl_ssi.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
>> index 40dfd8a..cfc78b8 100644
>> --- a/sound/soc/fsl/fsl_ssi.c
>> +++ b/sound/soc/fsl/fsl_ssi.c
>> @@ -1167,14 +1167,14 @@ static struct snd_soc_dai_driver 
>> fsl_ssi_dai_template = {
>>       .playback = {
>>           .stream_name = "CPU-Playback",
>>           .channels_min = 1,
>> -        .channels_max = 2,
>> +        .channels_max = 32,
>
> TDM mode is not normally used.  What happens if we're not in TDM mode, 
> and the user tries to start a stream with more than 2 channels?
For the user point of view, the maximum of available channels (like 
available through aplay command) is the intersection of the CPU and 
Codec DAIs.
So the max will still be the max provided by the whole hardware.

In other words, if the SSI is connected to a simple 2 channels codec, 
"aplay -c 4" will still failed.
But if you build a multi-codec hardware, it will work.

Arnaud

Re: [PATCH v3 1/6] ASoC: fsl_ssi: Real hardware channels max number is 32

[Caleb Crome]

On Mon, Jan 18, 2016 at 7:26 AM, Arnaud Mouiche <arnaud.mouiche@invoxia.com>
wrote:
>
> The max number of slots in TDM mode is 32:
> - Frame Rate Divider Control is a 5bit value
> - Time slot mask registers control 32 slots.
>
> Signed-off-by: Arnaud Mouiche <arnaud.mouiche@invoxia.com>
> ---
>  sound/soc/fsl/fsl_ssi.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
> index 40dfd8a..cfc78b8 100644
> --- a/sound/soc/fsl/fsl_ssi.c
> +++ b/sound/soc/fsl/fsl_ssi.c
> @@ -1167,14 +1167,14 @@ static struct snd_soc_dai_driver
fsl_ssi_dai_template = {
>         .playback = {
>                 .stream_name = "CPU-Playback",
>                 .channels_min = 1,
> -               .channels_max = 2,
> +               .channels_max = 32,
>                 .rates = FSLSSI_I2S_RATES,
>                 .formats = FSLSSI_I2S_FORMATS,
>         },
>         .capture = {
>                 .stream_name = "CPU-Capture",
>                 .channels_min = 1,
> -               .channels_max = 2,
> +               .channels_max = 32,
>                 .rates = FSLSSI_I2S_RATES,
>                 .formats = FSLSSI_I2S_FORMATS,
>         },
> --
> 1.9.1
>

I haven't seen any movement on this patch for some time.  I just wanted to
add my Tested&Reviewed.  These patches work for me on the fsl_ssi.


Tested-By: Caleb Crome <caleb@crome.org>
Reviewed-"By: Caleb Crome <caleb@crome.org>

Re: [PATCH v3 2/6] ASoC: fsl_ssi: The IPG/5 limitation concerns the bitclk, not the sysclk.

[Caleb Crome]

On Mon, Jan 18, 2016 at 7:26 AM, Arnaud Mouiche <arnaud.mouiche@invoxia.com>
wrote:
>
> 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 cfc78b8..dbd5f10 100644
> --- a/sound/soc/fsl/fsl_ssi.c
> +++ b/sound/soc/fsl/fsl_ssi.c
> @@ -679,6 +679,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 */
> @@ -695,13 +704,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
>

Tested-By: Caleb Crome <caleb@crome.org>
Reviewed-"By: Caleb Crome <caleb@crome.org>

Re: [PATCH v3 3/6] ASoC: fsl_ssi: Save a dev reference for dev_err() purpose.

[Caleb Crome]

On Mon, Jan 18, 2016 at 7:26 AM, Arnaud Mouiche <arnaud.mouiche@invoxia.com>
wrote:
>
> 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 dbd5f10..13613fd 100644
> --- a/sound/soc/fsl/fsl_ssi.c
> +++ b/sound/soc/fsl/fsl_ssi.c
> @@ -275,6 +275,7 @@ struct fsl_ssi_private {
>         struct fsl_ssi_dbg dbg_stats;
>
>         const struct fsl_ssi_soc_data *soc;
> +       struct device *dev;
>  };
>
>  /*
> @@ -1412,6 +1413,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
>


Tested-By: Caleb Crome <caleb@crome.org>
Reviewed-"By: Caleb Crome <caleb@crome.org>

Re: [PATCH v3 4/6] ASoC: fsl_ssi: Fix samples being dropped as Playback startup

[Caleb Crome]

On Mon, Jan 18, 2016 at 7:26 AM, Arnaud Mouiche
<arnaud.mouiche@invoxia.com> wrote:
>
> 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 13613fd..c9ba8ca 100644
> --- a/sound/soc/fsl/fsl_ssi.c
> +++ b/sound/soc/fsl/fsl_ssi.c
> @@ -488,9 +488,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
>


Tested-By: Caleb Crome <caleb@crome.org>
Reviewed-"By: Caleb Crome <caleb@crome.org>

Re: [PATCH v3 5/6] ASoC: fsl_ssi: Fix channel slipping in Playback at startup

[Caleb Crome]

On Mon, Jan 18, 2016 at 7:26 AM, Arnaud Mouiche
<arnaud.mouiche@invoxia.com> wrote:
> 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 | 34 +++++++++++++++++++++++++++++++++-
>  1 file changed, 33 insertions(+), 1 deletion(-)
>
> diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
> index c9ba8ca..530d592 100644
> --- a/sound/soc/fsl/fsl_ssi.c
> +++ b/sound/soc/fsl/fsl_ssi.c
> @@ -520,8 +520,40 @@ 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
>

Tested-By: Caleb Crome <caleb@crome.org>
Reviewed-"By: Caleb Crome <caleb@crome.org>

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

[Caleb Crome]

On Mon, Jan 18, 2016 at 7:26 AM, Arnaud Mouiche
<arnaud.mouiche@invoxia.com> wrote:
> 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 and IMX6solo.
>
> Signed-off-by: Arnaud Mouiche <arnaud.mouiche@invoxia.com>
> ---
>  sound/soc/fsl/fsl_ssi.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
>
> diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
> index 530d592..50218ed 100644
> --- a/sound/soc/fsl/fsl_ssi.c
> +++ b/sound/soc/fsl/fsl_ssi.c
> @@ -146,6 +146,7 @@ static bool fsl_ssi_volatile_reg(struct device *dev, unsigned int reg)
>         case CCSR_SSI_SRX1:
>         case CCSR_SSI_SISR:
>         case CCSR_SSI_SFCSR:
> +       case CCSR_SSI_SOR:
>         case CCSR_SSI_SACNT:
>         case CCSR_SSI_SACADD:
>         case CCSR_SSI_SACDAT:
> @@ -414,6 +415,26 @@ static void fsl_ssi_rxtx_config(struct fsl_ssi_private *ssi_private,
>  }
>
>  /*
> + * 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.
> + */
> +static void fsl_ssi_fifo_clear(struct fsl_ssi_private *ssi_private,
> +               bool is_rx)
> +{
> +       if (is_rx) {
> +               regmap_update_bits(ssi_private->regs, CCSR_SSI_SOR,
> +                       CCSR_SSI_SOR_RX_CLR, CCSR_SSI_SOR_RX_CLR);
> +       } else {
> +               regmap_update_bits(ssi_private->regs, CCSR_SSI_SOR,
> +                       CCSR_SSI_SOR_TX_CLR, CCSR_SSI_SOR_TX_CLR);
> +       }
> +}
> +
> +/*
>   * Calculate the bits that have to be disabled for the current stream that is
>   * getting disabled. This keeps the bits enabled that are necessary for the
>   * second stream to work if 'stream_active' is true.
> @@ -488,6 +509,8 @@ static void fsl_ssi_config(struct fsl_ssi_private *ssi_private, bool enable,
>          * (online configuration)
>          */
>         if (enable) {
> +               fsl_ssi_fifo_clear(ssi_private, vals->scr & CCSR_SSI_SCR_RE);
> +
>                 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
>

Tested-By: Caleb Crome <caleb@crome.org>
Reviewed-"By: Caleb Crome <caleb@crome.org>

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

[Fabio Estevam]

Hi Nicolin and Timur,

On Mon, Jan 18, 2016 at 1:26 PM, 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.

Any feedback about this series?

It addresses important fixes for the fsl_ssi driver.

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

[Nicolin Chen]

On Fri, Apr 29, 2016 at 12:44:16AM -0300, Fabio Estevam wrote:
> Hi Nicolin and Timur,
> 
> On Mon, Jan 18, 2016 at 1:26 PM, 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.
> 
> Any feedback about this series?
> 
> It addresses important fixes for the fsl_ssi driver.

Patches look fine to me. I thought there was going to be another version.

Acked-by: Nicolin Chen <nicoleotsuka@gmail.com>

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

[Caleb Crome]

On Thu, Apr 28, 2016 at 8:54 PM, Nicolin Chen <nicoleotsuka@gmail.com> wrote:
> On Fri, Apr 29, 2016 at 12:44:16AM -0300, Fabio Estevam wrote:
>> Hi Nicolin and Timur,
>>
>> On Mon, Jan 18, 2016 at 1:26 PM, 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.
>>
>> Any feedback about this series?
>>
>> It addresses important fixes for the fsl_ssi driver.
>
> Patches look fine to me. I thought there was going to be another version.

There is another patch that comes after these, but that's relating the
the watermark issues we talked about before.  The watermark issues can
stand independently of this patch series, so I think it's best to keep
the complexity with watermarks separate this series.    Arnaud never
mentioned to me that there was a version after v3.

-Caleb

>
> Acked-by: Nicolin Chen <nicoleotsuka@gmail.com>

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

[arnaud.mouiche]

Hi all

Le 29/04/2016 18:49, Caleb Crome a écrit :
> On Thu, Apr 28, 2016 at 8:54 PM, Nicolin Chen <nicoleotsuka@gmail.com> wrote:
>> On Fri, Apr 29, 2016 at 12:44:16AM -0300, Fabio Estevam wrote:
>>> Hi Nicolin and Timur,
>>>
>>> On Mon, Jan 18, 2016 at 1:26 PM, 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.
>>> Any feedback about this series?
>>>
>>> It addresses important fixes for the fsl_ssi driver.
>> Patches look fine to me. I thought there was going to be another version.
> There is another patch that comes after these, but that's relating the
> the watermark issues we talked about before.  The watermark issues can
> stand independently of this patch series, so I think it's best to keep
> the complexity with watermarks separate this series.    Arnaud never
> mentioned to me that there was a version after v3.
No, no new version after this one.

Regards,
Arnaud
>
> -Caleb
>
>> Acked-by: Nicolin Chen <nicoleotsuka@gmail.com>

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

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

[Fabio Estevam]

Hi Arnaud,

On Mon, May 2, 2016 at 7:29 PM, arnaud.mouiche@invoxia.com
<arnaud.mouiche@invoxia.com> wrote:

> No, no new version after this one.

Do they still apply against linux-next? If not, care to rebase?

Thanks

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

[arnaud.mouiche]

Sorry, It didn't rebase since 4.4.
I can try tomorrow to find some time to do this...

arnaud

Le 03/05/2016 00:32, Fabio Estevam a écrit :
> Hi Arnaud,
>
> On Mon, May 2, 2016 at 7:29 PM, arnaud.mouiche@invoxia.com
> <arnaud.mouiche@invoxia.com> wrote:
>
>> No, no new version after this one.
> Do they still apply against linux-next? If not, care to rebase?
>
> Thanks

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