All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] ASoC: fsl_ssi: Fix channel swap on playback start
@ 2017-04-01 14:48 Fabio Estevam
  2017-04-01 15:13 ` Arnaud Mouiche
                   ` (3 more replies)
  0 siblings, 4 replies; 30+ messages in thread
From: Fabio Estevam @ 2017-04-01 14:48 UTC (permalink / raw)
  To: broonie
  Cc: alsa-devel, arnaud.mouiche, timur, caleb, nicoleotsuka,
	max.krummenacher, Fabio Estevam, kernel

From: Fabio Estevam <fabio.estevam@nxp.com>

Commit f8fdf5375e2005f2 ("ASoC: fsl-ssi: add SSIEN errata work around")
implemented  the workaround for the following erratum found on i.MX35
errata document:

ENGcm06222: SSI:Transmission does not take place in bit length early
frame sync configuration

and also for ENGcm06222 from the same document.

However it has been only applied for AC97 mode. Apply it to I2S mode
as well so that it can fix audio channel swap during playback start.

The channel swap can be noticed in about 10% of the times an audio track
starts.

With the recommended workaround in place no more channel swap
happened after running audio start/stop sequence in more than
2000 times.

Tested on a mx6dl-wandboard.

Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
---
Changes since v1:
- Do not impact  61fcf10a0ee44763e0 ("ASoC: fsl_ssi: Fix channel slipping in
Playback at startup")

 sound/soc/fsl/fsl_ssi.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 17f92b8..549b2a5 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -575,7 +575,9 @@ static void fsl_ssi_config(struct fsl_ssi_private *ssi_private, bool enable,
 					"Timeout waiting TX FIFO filling\n");
 			}
 		}
-		regmap_update_bits(regs, CCSR_SSI_SCR, vals->scr, vals->scr);
+		regmap_update_bits(regs, CCSR_SSI_SCR,
+			CCSR_SSI_SCR_SSIEN | CCSR_SSI_SCR_TE | CCSR_SSI_SCR_RE,
+			CCSR_SSI_SCR_SSIEN | CCSR_SSI_SCR_TE | CCSR_SSI_SCR_RE);
 	}
 }
 
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* Re: [PATCH v2] ASoC: fsl_ssi: Fix channel swap on playback start
  2017-04-01 14:48 [PATCH v2] ASoC: fsl_ssi: Fix channel swap on playback start Fabio Estevam
@ 2017-04-01 15:13 ` Arnaud Mouiche
  2017-04-01 16:03   ` Fabio Estevam
  2017-04-03  8:19 ` Max Krummenacher
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 30+ messages in thread
From: Arnaud Mouiche @ 2017-04-01 15:13 UTC (permalink / raw)
  To: Fabio Estevam, broonie
  Cc: alsa-devel, kernel, timur, caleb, nicoleotsuka, max.krummenacher,
	Fabio Estevam

hello.


On 01/04/2017 16:48, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@nxp.com>
>
> Commit f8fdf5375e2005f2 ("ASoC: fsl-ssi: add SSIEN errata work around")
> implemented  the workaround for the following erratum found on i.MX35
> errata document:
>
> ENGcm06222: SSI:Transmission does not take place in bit length early
> frame sync configuration
>
> and also for ENGcm06222 from the same document.
>
> However it has been only applied for AC97 mode. Apply it to I2S mode
> as well so that it can fix audio channel swap during playback start.
>
> The channel swap can be noticed in about 10% of the times an audio track
> starts.
>
> With the recommended workaround in place no more channel swap
> happened after running audio start/stop sequence in more than
> 2000 times.
>
> Tested on a mx6dl-wandboard.
>
> Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
> ---
> Changes since v1:
> - Do not impact  61fcf10a0ee44763e0 ("ASoC: fsl_ssi: Fix channel slipping in
> Playback at startup")
>
>   sound/soc/fsl/fsl_ssi.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
> index 17f92b8..549b2a5 100644
> --- a/sound/soc/fsl/fsl_ssi.c
> +++ b/sound/soc/fsl/fsl_ssi.c
> @@ -575,7 +575,9 @@ static void fsl_ssi_config(struct fsl_ssi_private *ssi_private, bool enable,
>   					"Timeout waiting TX FIFO filling\n");
>   			}
>   		}
> -		regmap_update_bits(regs, CCSR_SSI_SCR, vals->scr, vals->scr);
> +		regmap_update_bits(regs, CCSR_SSI_SCR,
> +			CCSR_SSI_SCR_SSIEN | CCSR_SSI_SCR_TE | CCSR_SSI_SCR_RE,
> +			CCSR_SSI_SCR_SSIEN | CCSR_SSI_SCR_TE | CCSR_SSI_SCR_RE);
>   	}
>   }
>   
I will take time to look at the possible impact on imx6 using my 
previous test method.
Regards,
Arnaud

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2] ASoC: fsl_ssi: Fix channel swap on playback start
  2017-04-01 15:13 ` Arnaud Mouiche
@ 2017-04-01 16:03   ` Fabio Estevam
  0 siblings, 0 replies; 30+ messages in thread
From: Fabio Estevam @ 2017-04-01 16:03 UTC (permalink / raw)
  To: Arnaud Mouiche
  Cc: alsa-devel, Sascha Hauer, Timur Tabi, Caleb Crome, Nicolin Chen,
	Mark Brown, Max Krummenacher, Fabio Estevam

Hi Arnaud,

On Sat, Apr 1, 2017 at 12:13 PM, Arnaud Mouiche
<arnaud.mouiche@invoxia.com> wrote:

> I will take time to look at the possible impact on imx6 using my previous
> test method.

Excellent, your testing will be appreciated, thanks.

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2] ASoC: fsl_ssi: Fix channel swap on playback start
  2017-04-01 14:48 [PATCH v2] ASoC: fsl_ssi: Fix channel swap on playback start Fabio Estevam
  2017-04-01 15:13 ` Arnaud Mouiche
@ 2017-04-03  8:19 ` Max Krummenacher
  2017-04-03 20:32 ` Caleb Crome
  2017-04-03 22:36 ` Nicolin Chen
  3 siblings, 0 replies; 30+ messages in thread
From: Max Krummenacher @ 2017-04-03  8:19 UTC (permalink / raw)
  To: alsa-devel

On Sat, 2017-04-01 at 11:48 -0300, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@nxp.com>
> 
> Commit f8fdf5375e2005f2 ("ASoC: fsl-ssi: add SSIEN errata work around")
> implemented  the workaround for the following erratum found on i.MX35
> errata document:
> 
> ENGcm06222: SSI:Transmission does not take place in bit length early
> frame sync configuration
> 
> and also for ENGcm06222 from the same document.
> 
> However it has been only applied for AC97 mode. Apply it to I2S mode
> as well so that it can fix audio channel swap during playback start.
> 
> The channel swap can be noticed in about 10% of the times an audio track
> starts.
> 
> With the recommended workaround in place no more channel swap
> happened after running audio start/stop sequence in more than
> 2000 times.
> 
> Tested on a mx6dl-wandboard.
> 
> Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>

Hi

I could reproduce the issue on a Colibri iMX6 Dual with a i.MX 6DualLite.
Without the patch I see channel swap in about 3% of playback starts.
The patch fixes the issue.

Tested-by: Max Krummenacher <max.krummenacher@toradex.com>

Max


> ---
> Changes since v1:
> - Do not impact  61fcf10a0ee44763e0 ("ASoC: fsl_ssi: Fix channel slipping in
> Playback at startup")
> 
>  sound/soc/fsl/fsl_ssi.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
> index 17f92b8..549b2a5 100644
> --- a/sound/soc/fsl/fsl_ssi.c
> +++ b/sound/soc/fsl/fsl_ssi.c
> @@ -575,7 +575,9 @@ static void fsl_ssi_config(struct fsl_ssi_private *ssi_private, bool enable,
>  					"Timeout waiting TX FIFO filling\n");
>  			}
>  		}
> -		regmap_update_bits(regs, CCSR_SSI_SCR, vals->scr, vals->scr);
> +		regmap_update_bits(regs, CCSR_SSI_SCR,
> +			CCSR_SSI_SCR_SSIEN | CCSR_SSI_SCR_TE | CCSR_SSI_SCR_RE,
> +			CCSR_SSI_SCR_SSIEN | CCSR_SSI_SCR_TE | CCSR_SSI_SCR_RE);
>  	}
>  }
>  

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2] ASoC: fsl_ssi: Fix channel swap on playback start
  2017-04-01 14:48 [PATCH v2] ASoC: fsl_ssi: Fix channel swap on playback start Fabio Estevam
  2017-04-01 15:13 ` Arnaud Mouiche
  2017-04-03  8:19 ` Max Krummenacher
@ 2017-04-03 20:32 ` Caleb Crome
  2017-04-03 20:50   ` Caleb Crome
                     ` (2 more replies)
  2017-04-03 22:36 ` Nicolin Chen
  3 siblings, 3 replies; 30+ messages in thread
From: Caleb Crome @ 2017-04-03 20:32 UTC (permalink / raw)
  To: Fabio Estevam
  Cc: alsa-devel, Arnaud Mouiche, Timur Tabi, Nicolin Chen, Mark Brown,
	max.krummenacher, Fabio Estevam, kernel

On Sat, Apr 1, 2017 at 7:48 AM, Fabio Estevam <festevam@gmail.com> wrote:
>
> From: Fabio Estevam <fabio.estevam@nxp.com>
>
> Commit f8fdf5375e2005f2 ("ASoC: fsl-ssi: add SSIEN errata work around")
> implemented  the workaround for the following erratum found on i.MX35
> errata document:
>
> ENGcm06222: SSI:Transmission does not take place in bit length early
> frame sync configuration
>
> and also for ENGcm06222 from the same document.
>
> However it has been only applied for AC97 mode. Apply it to I2S mode
> as well so that it can fix audio channel swap during playback start.
>
> The channel swap can be noticed in about 10% of the times an audio track
> starts.
>
> With the recommended workaround in place no more channel swap
> happened after running audio start/stop sequence in more than
> 2000 times.
>
> Tested on a mx6dl-wandboard.
>
> Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
> ---
> Changes since v1:
> - Do not impact  61fcf10a0ee44763e0 ("ASoC: fsl_ssi: Fix channel slipping in
> Playback at startup")
>
>  sound/soc/fsl/fsl_ssi.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
> index 17f92b8..549b2a5 100644
> --- a/sound/soc/fsl/fsl_ssi.c
> +++ b/sound/soc/fsl/fsl_ssi.c
> @@ -575,7 +575,9 @@ static void fsl_ssi_config(struct fsl_ssi_private *ssi_private, bool enable,
>                                         "Timeout waiting TX FIFO filling\n");
>                         }
>                 }
> -               regmap_update_bits(regs, CCSR_SSI_SCR, vals->scr, vals->scr);
> +               regmap_update_bits(regs, CCSR_SSI_SCR,
> +                       CCSR_SSI_SCR_SSIEN | CCSR_SSI_SCR_TE | CCSR_SSI_SCR_RE,
> +                       CCSR_SSI_SCR_SSIEN | CCSR_SSI_SCR_TE | CCSR_SSI_SCR_RE);
>         }
>  }
>
> --
> 2.7.4
>

This patch definitely breaks the i.mx6 channel alignment.  In fact it
breaks it so that the channels are never aligned properly.

My test setup is as follows:
* Get vanilla kernel, tag v4.11-rc5
* apply a couple patches to allow AUD4 on the wandboard external
connectors (and disable internal audio)
* Test results:  v4.11-rc5 works flawlessly using Arnaud's atest
program.  No channel slips, no issues at all.
* Apply this patch, recompile, build.
* Channel alignment fails.  The channels never get aligned properly.

Am I right that the *only* change is this one-liner, and ignore the
previous non V2 version of this patch?
> -               regmap_update_bits(regs, CCSR_SSI_SCR, vals->scr, vals->scr);
> +               regmap_update_bits(regs, CCSR_SSI_SCR,
> +                       CCSR_SSI_SCR_SSIEN | CCSR_SSI_SCR_TE | CCSR_SSI_SCR_RE,
> +                       CCSR_SSI_SCR_SSIEN | CCSR_SSI_SCR_TE | CCSR_SSI_SCR_RE);

See you,

-Caleb

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2] ASoC: fsl_ssi: Fix channel swap on playback start
  2017-04-03 20:32 ` Caleb Crome
@ 2017-04-03 20:50   ` Caleb Crome
  2017-04-03 21:53   ` Fabio Estevam
  2017-04-03 22:08   ` Nicolin Chen
  2 siblings, 0 replies; 30+ messages in thread
From: Caleb Crome @ 2017-04-03 20:50 UTC (permalink / raw)
  To: Fabio Estevam
  Cc: alsa-devel, Arnaud Mouiche, Timur Tabi, Nicolin Chen, Mark Brown,
	max.krummenacher, Fabio Estevam, kernel

On Mon, Apr 3, 2017 at 1:32 PM, Caleb Crome <caleb@crome.org> wrote:
> On Sat, Apr 1, 2017 at 7:48 AM, Fabio Estevam <festevam@gmail.com> wrote:
>>
>> From: Fabio Estevam <fabio.estevam@nxp.com>
>>
>> Commit f8fdf5375e2005f2 ("ASoC: fsl-ssi: add SSIEN errata work around")
>> implemented  the workaround for the following erratum found on i.MX35
>> errata document:
>>
>> ENGcm06222: SSI:Transmission does not take place in bit length early
>> frame sync configuration
>>
>> and also for ENGcm06222 from the same document.
>>
>> However it has been only applied for AC97 mode. Apply it to I2S mode
>> as well so that it can fix audio channel swap during playback start.
>>
>> The channel swap can be noticed in about 10% of the times an audio track
>> starts.
>>
>> With the recommended workaround in place no more channel swap
>> happened after running audio start/stop sequence in more than
>> 2000 times.
>>
>> Tested on a mx6dl-wandboard.
>>
>> Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
>> ---
>> Changes since v1:
>> - Do not impact  61fcf10a0ee44763e0 ("ASoC: fsl_ssi: Fix channel slipping in
>> Playback at startup")
>>
>>  sound/soc/fsl/fsl_ssi.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
>> index 17f92b8..549b2a5 100644
>> --- a/sound/soc/fsl/fsl_ssi.c
>> +++ b/sound/soc/fsl/fsl_ssi.c
>> @@ -575,7 +575,9 @@ static void fsl_ssi_config(struct fsl_ssi_private *ssi_private, bool enable,
>>                                         "Timeout waiting TX FIFO filling\n");
>>                         }
>>                 }
>> -               regmap_update_bits(regs, CCSR_SSI_SCR, vals->scr, vals->scr);
>> +               regmap_update_bits(regs, CCSR_SSI_SCR,
>> +                       CCSR_SSI_SCR_SSIEN | CCSR_SSI_SCR_TE | CCSR_SSI_SCR_RE,
>> +                       CCSR_SSI_SCR_SSIEN | CCSR_SSI_SCR_TE | CCSR_SSI_SCR_RE);
>>         }
>>  }
>>
>> --
>> 2.7.4
>>
>
> This patch definitely breaks the i.mx6 channel alignment.  In fact it
> breaks it so that the channels are never aligned properly.
>
> My test setup is as follows:
> * Get vanilla kernel, tag v4.11-rc5
> * apply a couple patches to allow AUD4 on the wandboard external
> connectors (and disable internal audio)

FYI, for anybody that wants to test for themselves, I just posted the
patches for the wandboard here.

https://github.com/ccrome/linux-caleb-dev/wiki

Now that the SSI patches are in the mainline kernel, these 2 simple
patches should make testing the i.mx6 ssi pretty much trivial for
anybody.

-Caleb

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2] ASoC: fsl_ssi: Fix channel swap on playback start
  2017-04-03 20:32 ` Caleb Crome
  2017-04-03 20:50   ` Caleb Crome
@ 2017-04-03 21:53   ` Fabio Estevam
  2017-04-03 22:05     ` Arnaud Mouiche
  2017-04-03 23:22     ` Caleb Crome
  2017-04-03 22:08   ` Nicolin Chen
  2 siblings, 2 replies; 30+ messages in thread
From: Fabio Estevam @ 2017-04-03 21:53 UTC (permalink / raw)
  To: Caleb Crome
  Cc: alsa-devel, Arnaud Mouiche, Timur Tabi, Nicolin Chen, Mark Brown,
	Max Krummenacher, Fabio Estevam, Sascha Hauer

Hi Caleb,

On Mon, Apr 3, 2017 at 5:32 PM, Caleb Crome <caleb@crome.org> wrote:

> This patch definitely breaks the i.mx6 channel alignment.  In fact it
> breaks it so that the channels are never aligned properly.
>
> My test setup is as follows:
> * Get vanilla kernel, tag v4.11-rc5

Thanks for testing.

Just tested 4.11-rc5. It needs this additional patch:
https://patchwork.ozlabs.org/patch/745349/

otherwise pinctrl hog is broken and then sgtl5000 does not probe due
to the lack of MCLK.

I am using the original imx6dl-wandboard.dtb on my tests with no
hardware changes.

The test I am running is simple: just run the following script on the wandboard:

#!/bin/bash

while true
do
aplay swap_test.wav& sleep 1; killall aplay
done

You can get swap_test.wav file that consists of silence in the left
channel and none silence in the right channel from here:
https://www.dropbox.com/s/4zt0jvmtx34ic9x/swap_test.wav?dl=0

Then keep listening the left channel. In about one out of ten times
you will get non-silence there, indicating a swap.

> * apply a couple patches to allow AUD4 on the wandboard external
> connectors (and disable internal audio)
> * Test results:  v4.11-rc5 works flawlessly using Arnaud's atest
> program.  No channel slips, no issues at all.
> * Apply this patch, recompile, build.
> * Channel alignment fails.  The channels never get aligned properly.
>
> Am I right that the *only* change is this one-liner, and ignore the
> previous non V2 version of this patch?
>> -               regmap_update_bits(regs, CCSR_SSI_SCR, vals->scr, vals->scr);
>> +               regmap_update_bits(regs, CCSR_SSI_SCR,
>> +                       CCSR_SSI_SCR_SSIEN | CCSR_SSI_SCR_TE | CCSR_SSI_SCR_RE,
>> +                       CCSR_SSI_SCR_SSIEN | CCSR_SSI_SCR_TE | CCSR_SSI_SCR_RE);

Yes, with only this change I do not get the swap anymore.

If you have a chance to run this method, please let me know how it goes.

Thanks

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2] ASoC: fsl_ssi: Fix channel swap on playback start
  2017-04-03 21:53   ` Fabio Estevam
@ 2017-04-03 22:05     ` Arnaud Mouiche
  2017-04-03 22:20       ` Nicolin Chen
  2017-04-03 23:22     ` Caleb Crome
  1 sibling, 1 reply; 30+ messages in thread
From: Arnaud Mouiche @ 2017-04-03 22:05 UTC (permalink / raw)
  To: Fabio Estevam, Caleb Crome
  Cc: alsa-devel, Sascha Hauer, Timur Tabi, Nicolin Chen, Mark Brown,
	Max Krummenacher, Fabio Estevam

Hello.

On 03/04/2017 23:53, Fabio Estevam wrote:
> Hi Caleb,
>
> On Mon, Apr 3, 2017 at 5:32 PM, Caleb Crome <caleb@crome.org> wrote:
>
>> This patch definitely breaks the i.mx6 channel alignment.  In fact it
>> breaks it so that the channels are never aligned properly.
>>
>> My test setup is as follows:
>> * Get vanilla kernel, tag v4.11-rc5

I'm also testing  on a imx6sl board on v4.11-rc5 vanilla.

The Patch break something. I'm not even able to to make two consecutives 
'aplay' in order to generate something correct  on the external SSI bus.
- boot
- aplay -D hw:1,0 -r 48000 -c 2 -f S16_LE /dev/urandom
Playing raw data '/dev/urandom' : Signed 16 bit Little Endian, Rate 
48000 Hz, Stereo
^CAborted by signal Interrupt...

=> ok

- aplay -D hw:1,0 -r 48000 -c 2 -f S16_LE /dev/urandom
Playing raw data '/dev/urandom' : Signed 16 bit Little Endian, Rate 
48000 Hz, Stereo
aplay: pcm_write:1702: write error: Input/output error

=> no clock on external bus.

I confirm it works correctly without the patch.
On my board, the SSI device hw:1,0 is master of the bus (clock and sync) 
and is working in DSP mode.

I will continue the tests tomorrow.

Arnaud

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2] ASoC: fsl_ssi: Fix channel swap on playback start
  2017-04-03 20:32 ` Caleb Crome
  2017-04-03 20:50   ` Caleb Crome
  2017-04-03 21:53   ` Fabio Estevam
@ 2017-04-03 22:08   ` Nicolin Chen
  2017-04-03 23:31     ` Caleb Crome
  2 siblings, 1 reply; 30+ messages in thread
From: Nicolin Chen @ 2017-04-03 22:08 UTC (permalink / raw)
  To: Caleb Crome
  Cc: alsa-devel, Arnaud Mouiche, Timur Tabi, Mark Brown,
	max.krummenacher, Fabio Estevam, Fabio Estevam, kernel

On Mon, Apr 03, 2017 at 01:32:42PM -0700, Caleb Crome wrote:

> This patch definitely breaks the i.mx6 channel alignment.  In fact it
> breaks it so that the channels are never aligned properly.
> 
> My test setup is as follows:
> * Get vanilla kernel, tag v4.11-rc5
> * apply a couple patches to allow AUD4 on the wandboard external
> connectors (and disable internal audio)
> * Test results:  v4.11-rc5 works flawlessly using Arnaud's atest
> program.  No channel slips, no issues at all.
> * Apply this patch, recompile, build.
> * Channel alignment fails.  The channels never get aligned properly.

What's your test case for the alignment? IIRC, you only needed
the FIFO to be pre-filled with data input so that SSI will not
encounter any FIFO underrun after enabling TE bit. That's why
there is a for-loop before this regmap_update_bits().

> Am I right that the *only* change is this one-liner, and ignore the
> previous non V2 version of this patch?
> > -               regmap_update_bits(regs, CCSR_SSI_SCR, vals->scr, vals->scr);
> > +               regmap_update_bits(regs, CCSR_SSI_SCR,
> > +                       CCSR_SSI_SCR_SSIEN | CCSR_SSI_SCR_TE | CCSR_SSI_SCR_RE,
> > +                       CCSR_SSI_SCR_SSIEN | CCSR_SSI_SCR_TE | CCSR_SSI_SCR_RE);

However, this patch seems to merely set the RE bit. It shouldn't
affect that test case since the SSIEN bit is still set prior to
the TE bit.

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2] ASoC: fsl_ssi: Fix channel swap on playback start
  2017-04-03 22:05     ` Arnaud Mouiche
@ 2017-04-03 22:20       ` Nicolin Chen
  0 siblings, 0 replies; 30+ messages in thread
From: Nicolin Chen @ 2017-04-03 22:20 UTC (permalink / raw)
  To: Arnaud Mouiche
  Cc: alsa-devel, Sascha Hauer, Timur Tabi, Caleb Crome, Mark Brown,
	Max Krummenacher, Fabio Estevam, Fabio Estevam

On Tue, Apr 04, 2017 at 12:05:57AM +0200, Arnaud Mouiche wrote:

> The Patch break something. I'm not even able to to make two
> consecutives 'aplay' in order to generate something correct  on the
> external SSI bus.
> - boot
> - aplay -D hw:1,0 -r 48000 -c 2 -f S16_LE /dev/urandom
> Playing raw data '/dev/urandom' : Signed 16 bit Little Endian, Rate
> 48000 Hz, Stereo
> ^CAborted by signal Interrupt...
> 
> => ok
> 
> - aplay -D hw:1,0 -r 48000 -c 2 -f S16_LE /dev/urandom
> Playing raw data '/dev/urandom' : Signed 16 bit Little Endian, Rate
> 48000 Hz, Stereo
> aplay: pcm_write:1702: write error: Input/output error
> 
> => no clock on external bus.

It's probably because of the nr_active_streams and keep_active in
the fsl_ssi_config() are not working correctly any more since the
TE abd RE are always set -- wondering why Fabio and Max don't see
any problem; is there any diff between vanilla and -next?

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2] ASoC: fsl_ssi: Fix channel swap on playback start
  2017-04-01 14:48 [PATCH v2] ASoC: fsl_ssi: Fix channel swap on playback start Fabio Estevam
                   ` (2 preceding siblings ...)
  2017-04-03 20:32 ` Caleb Crome
@ 2017-04-03 22:36 ` Nicolin Chen
  2017-04-03 22:54   ` Fabio Estevam
  3 siblings, 1 reply; 30+ messages in thread
From: Nicolin Chen @ 2017-04-03 22:36 UTC (permalink / raw)
  To: Fabio Estevam
  Cc: alsa-devel, arnaud.mouiche, timur, caleb, broonie,
	max.krummenacher, Fabio Estevam, kernel

Hi Fabio,

On Sat, Apr 01, 2017 at 11:48:51AM -0300, Fabio Estevam wrote:

> ENGcm06222: SSI:Transmission does not take place in bit length early
> frame sync configuration
[...]
> @@ -575,7 +575,9 @@ static void fsl_ssi_config(struct fsl_ssi_private *ssi_private, bool enable,
>  					"Timeout waiting TX FIFO filling\n");
>  			}
>  		}
> -		regmap_update_bits(regs, CCSR_SSI_SCR, vals->scr, vals->scr);
> +		regmap_update_bits(regs, CCSR_SSI_SCR,
> +			CCSR_SSI_SCR_SSIEN | CCSR_SSI_SCR_TE | CCSR_SSI_SCR_RE,
> +			CCSR_SSI_SCR_SSIEN | CCSR_SSI_SCR_TE | CCSR_SSI_SCR_RE);

My extra concern for this change is that ENGcm06222 suggests to
set TE and SSIEN together. However, we are still not setting the
SSIEN and TE together -- SSIEN is set already before this line
in the "ssi_private->use_dma && (vals->scr & CCSR_SSI_SCR_TE)".

On the other hand, ENGcm06222 doesn't mention anything related
to the RE bit. Although ENGcm06474 suggests to set TE and RE
together, yet it's for another bug (when TE is set after RE, the
TX channels might be swapped.)

Then, the test case: aplay swap_test.wav& sleep 1; killall aplay

It doesn't involve RE at all. So I don't get why setting RE and
TE together after setting SSIEN (three bits are not set together
here.) could solve the channel swapping problem for a test case
which has never involved RE at all. Am I missing something?

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2] ASoC: fsl_ssi: Fix channel swap on playback start
  2017-04-03 22:36 ` Nicolin Chen
@ 2017-04-03 22:54   ` Fabio Estevam
  2017-04-04  0:08     ` Nicolin Chen
  0 siblings, 1 reply; 30+ messages in thread
From: Fabio Estevam @ 2017-04-03 22:54 UTC (permalink / raw)
  To: Nicolin Chen
  Cc: alsa-devel, Arnaud Mouiche, Timur Tabi, Caleb Crome, Mark Brown,
	Max Krummenacher, Fabio Estevam, Sascha Hauer

Hi Nicolin,

On Mon, Apr 3, 2017 at 7:36 PM, Nicolin Chen <nicoleotsuka@gmail.com> wrote:

> My extra concern for this change is that ENGcm06222 suggests to
> set TE and SSIEN together. However, we are still not setting the
> SSIEN and TE together -- SSIEN is set already before this line
> in the "ssi_private->use_dma && (vals->scr & CCSR_SSI_SCR_TE)".
>
> On the other hand, ENGcm06222 doesn't mention anything related
> to the RE bit. Although ENGcm06474 suggests to set TE and RE
> together, yet it's for another bug (when TE is set after RE, the
> TX channels might be swapped.)

The idea for this patch came from commit f8fdf5375e2005 ("ASoC:
fsl-ssi: add SSIEN errata work around").

In this commit SSIEN, TE and RE are written at the same time as a
workaround to ENGcm06222 and ENGcm06532
from the MX35 errata document. The workaround was only applied to AC97
context (not sure why?).

ENGcm06222 is about "SSI:Transmission does not take place in bit
length early frame sync configuration"

As we use bit length frame sync in I2S mode, I thought this could
impact us and when I try the patch  it does not swap anymore on this
simple usecase:
aplay swap_test.wav& sleep 1; killall aplay

Seems to cause other issues though as reported by Caleb and Arnaud, so
we need to find other way to solve this.

> Then, the test case: aplay swap_test.wav& sleep 1; killall aplay
>
> It doesn't involve RE at all. So I don't get why setting RE and
> TE together after setting SSIEN (three bits are not set together
> here.) could solve the channel swapping problem for a test case
> which has never involved RE at all. Am I missing something?

Your understanding is correct. In my usecase there is no audio capture
involved at all, just stereo audio playback.

The only explanation I can give is the same one from commit
f8fdf5375e2005 ("ASoC: fsl-ssi: add SSIEN errata work around").

Do you have access to any imx board with SSI?

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2] ASoC: fsl_ssi: Fix channel swap on playback start
  2017-04-03 21:53   ` Fabio Estevam
  2017-04-03 22:05     ` Arnaud Mouiche
@ 2017-04-03 23:22     ` Caleb Crome
  2017-04-03 23:40       ` Fabio Estevam
  1 sibling, 1 reply; 30+ messages in thread
From: Caleb Crome @ 2017-04-03 23:22 UTC (permalink / raw)
  To: Fabio Estevam
  Cc: alsa-devel, Arnaud Mouiche, Timur Tabi, Nicolin Chen, Mark Brown,
	Max Krummenacher, Fabio Estevam, Sascha Hauer

On Mon, Apr 3, 2017 at 2:53 PM, Fabio Estevam <festevam@gmail.com> wrote:
> Hi Caleb,
>
> On Mon, Apr 3, 2017 at 5:32 PM, Caleb Crome <caleb@crome.org> wrote:
>
>> This patch definitely breaks the i.mx6 channel alignment.  In fact it
>> breaks it so that the channels are never aligned properly.
>>
>> My test setup is as follows:
>> * Get vanilla kernel, tag v4.11-rc5
>
> Thanks for testing.
>
> Just tested 4.11-rc5. It needs this additional patch:
> https://patchwork.ozlabs.org/patch/745349/
>
> otherwise pinctrl hog is broken and then sgtl5000 does not probe due
> to the lack of MCLK.
>
> I am using the original imx6dl-wandboard.dtb on my tests with no
> hardware changes.
>
> The test I am running is simple: just run the following script on the wandboard:
>
> #!/bin/bash
>
> while true
> do
> aplay swap_test.wav& sleep 1; killall aplay
> done

With a vanilla kernel, it works perfectly with the pinctrl patch.
In this case, I ran a cable from the wandboard over to my computer and
recorded with audacity, using your wile true script above.
Here you can see that with 4.11-rc5 plus the pinctrl patch, there is
no channel swapping:

http://imgur.com/od0LoJP

With this fsl_ssi patch, it also doesn't fail.


However, the playback only test is fine insofar as it goes, but it
doesn't cover many important test cases:
* multi-channel operation
* Playback only
* Record only
* Playback running, then record starts
* record running, then playback starts
* playback & record running, record stops
* playback & record running, playback stops
* repeats of some of these (ie.. sometimes fifos don't get cleared)

These were all meticulously tested before, and it's rock solid now for
me on the MX6 with the 4.11-rc5.

I can say for 100% sure, this patch breaks multi-channel operation on i.MX6.


>
> You can get swap_test.wav file that consists of silence in the left
> channel and none silence in the right channel from here:
> https://www.dropbox.com/s/4zt0jvmtx34ic9x/swap_test.wav?dl=0

Ah, swap_test is at 44.1kHz.  I get
Playing WAVE 'swap_test.wav' : Signed 16 bit Little Endian, Rate 44100
Hz, Stereo
./asdf: line 5: kill: pts/0: arguments must be process or job IDs
aplay: main:722: audio open error: Device or resource busy
./asdf: line 5: kill: pts/0: arguments must be process or job IDs
aplay: main:722: audio open error: Device or resource busy

It works at 48kHz.

>
> Then keep listening the left channel. In about one out of ten times
> you will get non-silence there, indicating a swap.

I never saw this in either case with stereo only, 48kHz. .

-Caleb

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2] ASoC: fsl_ssi: Fix channel swap on playback start
  2017-04-03 22:08   ` Nicolin Chen
@ 2017-04-03 23:31     ` Caleb Crome
  2017-04-03 23:55       ` Nicolin Chen
  0 siblings, 1 reply; 30+ messages in thread
From: Caleb Crome @ 2017-04-03 23:31 UTC (permalink / raw)
  To: Nicolin Chen
  Cc: alsa-devel, Arnaud Mouiche, Timur Tabi, Mark Brown,
	Max Krummenacher, Fabio Estevam, Fabio Estevam, Sascha Hauer

On Mon, Apr 3, 2017 at 3:08 PM, Nicolin Chen <nicoleotsuka@gmail.com> wrote:
> On Mon, Apr 03, 2017 at 01:32:42PM -0700, Caleb Crome wrote:
>
>> This patch definitely breaks the i.mx6 channel alignment.  In fact it
>> breaks it so that the channels are never aligned properly.
>>
>> My test setup is as follows:
>> * Get vanilla kernel, tag v4.11-rc5
>> * apply a couple patches to allow AUD4 on the wandboard external
>> connectors (and disable internal audio)
>> * Test results:  v4.11-rc5 works flawlessly using Arnaud's atest
>> program.  No channel slips, no issues at all.
>> * Apply this patch, recompile, build.
>> * Channel alignment fails.  The channels never get aligned properly.
>
> What's your test case for the alignment?

I'm not sure what you are asking.  The test case I'm testing is:
connect SSI to AUD4 on wandboard & physically connect TX -> RX.  (as
per https://github.com/ccrome/linux-caleb-dev/wiki), then use atest to
verify bit-perfection of TX->RX transmission.

Also, you must use a scope on the pins to verify that TX is also in
the right location (i.e. it's possible that bot TX and RX are rotated
by the same number of samples, thus you could be fooled by a
software-only test).

> IIRC, you only needed
> the FIFO to be pre-filled with data input so that SSI will not
> encounter any FIFO underrun after enabling TE bit. That's why
> there is a for-loop before this regmap_update_bits().

Well, there was more to it than that IIRC.  There were several patches
that made it all hang together.

>
>> Am I right that the *only* change is this one-liner, and ignore the
>> previous non V2 version of this patch?
>> > -               regmap_update_bits(regs, CCSR_SSI_SCR, vals->scr, vals->scr);
>> > +               regmap_update_bits(regs, CCSR_SSI_SCR,
>> > +                       CCSR_SSI_SCR_SSIEN | CCSR_SSI_SCR_TE | CCSR_SSI_SCR_RE,
>> > +                       CCSR_SSI_SCR_SSIEN | CCSR_SSI_SCR_TE | CCSR_SSI_SCR_RE);
>
> However, this patch seems to merely set the RE bit. It shouldn't
> affect that test case since the SSIEN bit is still set prior to
> the TE bit.

Heh, well, this patch causes audio to be utterly broken on
multi-channel audio :-/

-Caleb

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2] ASoC: fsl_ssi: Fix channel swap on playback start
  2017-04-03 23:22     ` Caleb Crome
@ 2017-04-03 23:40       ` Fabio Estevam
  2017-04-03 23:42         ` Caleb Crome
  0 siblings, 1 reply; 30+ messages in thread
From: Fabio Estevam @ 2017-04-03 23:40 UTC (permalink / raw)
  To: Caleb Crome
  Cc: alsa-devel, Arnaud Mouiche, Timur Tabi, Nicolin Chen, Mark Brown,
	Max Krummenacher, Fabio Estevam, Sascha Hauer

Hi Caleb,

On Mon, Apr 3, 2017 at 8:22 PM, Caleb Crome <caleb@crome.org> wrote:

> With a vanilla kernel, it works perfectly with the pinctrl patch.
> In this case, I ran a cable from the wandboard over to my computer and
> recorded with audacity, using your wile true script above.
> Here you can see that with 4.11-rc5 plus the pinctrl patch, there is
> no channel swapping:
>
> http://imgur.com/od0LoJP

Which wandboard type do you have? mx6solo,dl or quad?

I am using  imx6dl-wandboard.

I am surprised that the channel swap does not happen on your case.
Maybe you need to run the test for an extended period of time?

On my case I usually get the swap in 1/10 of times. Max uses a Toradex
MX6DL Colibri board and sees the swap in 3-4% of the times.

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2] ASoC: fsl_ssi: Fix channel swap on playback start
  2017-04-03 23:40       ` Fabio Estevam
@ 2017-04-03 23:42         ` Caleb Crome
  2017-04-04  8:59           ` Arnaud Mouiche
  0 siblings, 1 reply; 30+ messages in thread
From: Caleb Crome @ 2017-04-03 23:42 UTC (permalink / raw)
  To: Fabio Estevam
  Cc: alsa-devel, Arnaud Mouiche, Timur Tabi, Nicolin Chen, Mark Brown,
	Max Krummenacher, Fabio Estevam, Sascha Hauer

On Mon, Apr 3, 2017 at 4:40 PM, Fabio Estevam <festevam@gmail.com> wrote:
> Hi Caleb,
>
> On Mon, Apr 3, 2017 at 8:22 PM, Caleb Crome <caleb@crome.org> wrote:
>
>> With a vanilla kernel, it works perfectly with the pinctrl patch.
>> In this case, I ran a cable from the wandboard over to my computer and
>> recorded with audacity, using your wile true script above.
>> Here you can see that with 4.11-rc5 plus the pinctrl patch, there is
>> no channel swapping:
>>
>> http://imgur.com/od0LoJP
>
> Which wandboard type do you have? mx6solo,dl or quad?
>
> I am using  imx6dl-wandboard.
>
> I am surprised that the channel swap does not happen on your case.
> Maybe you need to run the test for an extended period of time?

Running on a quad.

>
> On my case I usually get the swap in 1/10 of times. Max uses a Toradex
> MX6DL Colibri board and sees the swap in 3-4% of the times.

I'll run it for a much longer time and see what happens.

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2] ASoC: fsl_ssi: Fix channel swap on playback start
  2017-04-03 23:31     ` Caleb Crome
@ 2017-04-03 23:55       ` Nicolin Chen
  2017-04-04  0:07         ` Timur Tabi
  2017-04-04  0:39         ` Caleb Crome
  0 siblings, 2 replies; 30+ messages in thread
From: Nicolin Chen @ 2017-04-03 23:55 UTC (permalink / raw)
  To: Caleb Crome
  Cc: alsa-devel, Arnaud Mouiche, Timur Tabi, Mark Brown,
	Max Krummenacher, Fabio Estevam, Fabio Estevam, Sascha Hauer

On Mon, Apr 03, 2017 at 04:31:59PM -0700, Caleb Crome wrote:

> > What's your test case for the alignment?
> 
> I'm not sure what you are asking.  The test case I'm testing is:
> connect SSI to AUD4 on wandboard & physically connect TX -> RX.  (as
> per https://github.com/ccrome/linux-caleb-dev/wiki), then use atest to
> verify bit-perfection of TX->RX transmission.

So your test case involve both TX and RX. That's why this change
would impact it. My understanding is: because you can not enable
TX and RX in the same time from user space but only through two
separate back-to-back system calls. So when the 2nd system call
happens (RX for example), the RE bit, supposed to be enabled by
this 2nd system call, has already been set by the 1st TX system
call -- there's some random data in the RX FIFO already.

> >> > -               regmap_update_bits(regs, CCSR_SSI_SCR, vals->scr, vals->scr);
> >> > +               regmap_update_bits(regs, CCSR_SSI_SCR,
> >> > +                       CCSR_SSI_SCR_SSIEN | CCSR_SSI_SCR_TE | CCSR_SSI_SCR_RE,
> >> > +                       CCSR_SSI_SCR_SSIEN | CCSR_SSI_SCR_TE | CCSR_SSI_SCR_RE);
> >
> > However, this patch seems to merely set the RE bit. It shouldn't
> > affect that test case since the SSIEN bit is still set prior to
> > the TE bit.
> 
> Heh, well, this patch causes audio to be utterly broken on
> multi-channel audio :-/

If possible, could you try to confirm what's the diff between the
two SCR values of before-regmap and after-regmap in your case?

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2] ASoC: fsl_ssi: Fix channel swap on playback start
  2017-04-03 23:55       ` Nicolin Chen
@ 2017-04-04  0:07         ` Timur Tabi
  2017-04-04  0:39         ` Caleb Crome
  1 sibling, 0 replies; 30+ messages in thread
From: Timur Tabi @ 2017-04-04  0:07 UTC (permalink / raw)
  To: Nicolin Chen, Caleb Crome
  Cc: alsa-devel, Max Krummenacher, Arnaud Mouiche, Mark Brown,
	Sascha Hauer, Fabio Estevam, Fabio Estevam

Nicolin Chen wrote:
> So your test case involve both TX and RX. That's why this change
> would impact it. My understanding is: because you can not enable
> TX and RX in the same time from user space but only through two
> separate back-to-back system calls. So when the 2nd system call
> happens (RX for example), the RE bit, supposed to be enabled by
> this 2nd system call, has already been set by the 1st TX system
> call -- there's some random data in the RX FIFO already.

This makes sense to me.  I don't have the bandwidth to test it just yet, but I 
suspect that this patch will break PowerPC systems as well.

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2] ASoC: fsl_ssi: Fix channel swap on playback start
  2017-04-03 22:54   ` Fabio Estevam
@ 2017-04-04  0:08     ` Nicolin Chen
  0 siblings, 0 replies; 30+ messages in thread
From: Nicolin Chen @ 2017-04-04  0:08 UTC (permalink / raw)
  To: Fabio Estevam
  Cc: alsa-devel, Arnaud Mouiche, Timur Tabi, Caleb Crome, Mark Brown,
	Max Krummenacher, Fabio Estevam, Sascha Hauer

On Mon, Apr 03, 2017 at 07:54:26PM -0300, Fabio Estevam wrote:
> Hi Nicolin,
> 
> On Mon, Apr 3, 2017 at 7:36 PM, Nicolin Chen <nicoleotsuka@gmail.com> wrote:
> 
> > My extra concern for this change is that ENGcm06222 suggests to
> > set TE and SSIEN together. However, we are still not setting the
> > SSIEN and TE together -- SSIEN is set already before this line
> > in the "ssi_private->use_dma && (vals->scr & CCSR_SSI_SCR_TE)".
> >
> > On the other hand, ENGcm06222 doesn't mention anything related
> > to the RE bit. Although ENGcm06474 suggests to set TE and RE
> > together, yet it's for another bug (when TE is set after RE, the
> > TX channels might be swapped.)
> 
> The idea for this patch came from commit f8fdf5375e2005 ("ASoC:
> fsl-ssi: add SSIEN errata work around").

I understand what's this patch doing.

> In this commit SSIEN, TE and RE are written at the same time as a
> workaround to ENGcm06222 and ENGcm06532 from the errata document.

Are you sure? Because there is a line of code set SSIEN separately
right before the line that this patch applies to. So this patch
actually only applies the workaround for ENGcm06532 (the bug when
setting RX prior to TX.)


> Seems to cause other issues though as reported by Caleb and Arnaud, so
> we need to find other way to solve this.
> > Then, the test case: aplay swap_test.wav& sleep 1; killall aplay
> >
> > It doesn't involve RE at all. So I don't get why setting RE and
> > TE together after setting SSIEN (three bits are not set together
> > here.) could solve the channel swapping problem for a test case
> > which has never involved RE at all. Am I missing something?
> 
> Your understanding is correct. In my usecase there is no audio capture
> involved at all, just stereo audio playback.
> 
> The only explanation I can give is the same one from commit
> f8fdf5375e2005 ("ASoC: fsl-ssi: add SSIEN errata work around").

I don't think that's an explanation. For non-AC97 cases, we are not
setting SSIEN and TE together at all, even by applying this change.

> Do you have access to any imx board with SSI?
I do. Will try to test it later.

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2] ASoC: fsl_ssi: Fix channel swap on playback start
  2017-04-03 23:55       ` Nicolin Chen
  2017-04-04  0:07         ` Timur Tabi
@ 2017-04-04  0:39         ` Caleb Crome
  2017-04-04  1:25           ` Nicolin Chen
  1 sibling, 1 reply; 30+ messages in thread
From: Caleb Crome @ 2017-04-04  0:39 UTC (permalink / raw)
  To: Nicolin Chen
  Cc: alsa-devel, Arnaud Mouiche, Timur Tabi, Mark Brown,
	Max Krummenacher, Fabio Estevam, Fabio Estevam, Sascha Hauer

On Mon, Apr 3, 2017 at 4:55 PM, Nicolin Chen <nicoleotsuka@gmail.com> wrote:
> On Mon, Apr 03, 2017 at 04:31:59PM -0700, Caleb Crome wrote:
>
>> > What's your test case for the alignment?
>>
>> I'm not sure what you are asking.  The test case I'm testing is:
>> connect SSI to AUD4 on wandboard & physically connect TX -> RX.  (as
>> per https://github.com/ccrome/linux-caleb-dev/wiki), then use atest to
>> verify bit-perfection of TX->RX transmission.
>
> So your test case involve both TX and RX. That's why this change
> would impact it. My understanding is: because you can not enable
> TX and RX in the same time from user space but only through two
> separate back-to-back system calls. So when the 2nd system call
> happens (RX for example), the RE bit, supposed to be enabled by
> this 2nd system call, has already been set by the 1st TX system
> call -- there's some random data in the RX FIFO already.
>
>> >> > -               regmap_update_bits(regs, CCSR_SSI_SCR, vals->scr, vals->scr);
>> >> > +               regmap_update_bits(regs, CCSR_SSI_SCR,
>> >> > +                       CCSR_SSI_SCR_SSIEN | CCSR_SSI_SCR_TE | CCSR_SSI_SCR_RE,
>> >> > +                       CCSR_SSI_SCR_SSIEN | CCSR_SSI_SCR_TE | CCSR_SSI_SCR_RE);
>> >
>> > However, this patch seems to merely set the RE bit. It shouldn't
>> > affect that test case since the SSIEN bit is still set prior to
>> > the TE bit.
>>
>> Heh, well, this patch causes audio to be utterly broken on
>> multi-channel audio :-/
>
> If possible, could you try to confirm what's the diff between the
> two SCR values of before-regmap and after-regmap in your case?

With this patch (broken audio, includes tx and rx, so 2 updates.
Running atest software)
------------------------
Apr  4 00:35:03 arm kernel: [   33.678451] Before update: 0x00001098
Apr  4 00:35:03 arm kernel: [   33.682339] After update: 0x0000109f
Apr  4 00:35:04 arm kernel: [   33.687196] Before update: 0x0000109f
Apr  4 00:35:04 arm kernel: [   33.690916] After update: 0x0000109f


Before this patch (working audio, running atest software)
------------------------
Apr  4 00:38:24 arm kernel: [   68.261765] Before update: 0x00001098
Apr  4 00:38:24 arm kernel: [   68.265653] After update: 0x0000109d
Apr  4 00:38:24 arm kernel: [   68.270865] Before update: 0x0000109d
Apr  4 00:38:24 arm kernel: [   68.274560] After update: 0x0000109f


Oh what a difference 1 bit makes!

-Caleb

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2] ASoC: fsl_ssi: Fix channel swap on playback start
  2017-04-04  0:39         ` Caleb Crome
@ 2017-04-04  1:25           ` Nicolin Chen
  0 siblings, 0 replies; 30+ messages in thread
From: Nicolin Chen @ 2017-04-04  1:25 UTC (permalink / raw)
  To: Caleb Crome
  Cc: alsa-devel, Arnaud Mouiche, Timur Tabi, Mark Brown,
	Max Krummenacher, Fabio Estevam, Fabio Estevam, Sascha Hauer

On Mon, Apr 03, 2017 at 05:39:22PM -0700, Caleb Crome wrote:

> > If possible, could you try to confirm what's the diff between the
> > two SCR values of before-regmap and after-regmap in your case?
> 
> With this patch (broken audio, includes tx and rx, so 2 updates.
> Running atest software)
> ------------------------
> Apr  4 00:35:03 arm kernel: [   33.678451] Before update: 0x00001098
> Apr  4 00:35:03 arm kernel: [   33.682339] After update: 0x0000109f
> Apr  4 00:35:04 arm kernel: [   33.687196] Before update: 0x0000109f
> Apr  4 00:35:04 arm kernel: [   33.690916] After update: 0x0000109f
> 
> 
> Before this patch (working audio, running atest software)
> ------------------------
> Apr  4 00:38:24 arm kernel: [   68.261765] Before update: 0x00001098
> Apr  4 00:38:24 arm kernel: [   68.265653] After update: 0x0000109d
> Apr  4 00:38:24 arm kernel: [   68.270865] Before update: 0x0000109d
> Apr  4 00:38:24 arm kernel: [   68.274560] After update: 0x0000109f

So my understanding is correct. For 2-channel use cases, this
change probably wouldn't matter because the data is correctly
aligned -- there'd be only some zero data in the beginning.
But it is hard to tell for multi-channels.

SSI FIFO depth is 15 -- for dual-fifo settings, data wouldd be
only aligned if the channel number is 2, 6, 10. If you are able
to test 6 or 10 channels, I would like to see the result.

Overall, we probably need some support from i.MX hardware team.
Instead of setting three bits together, we need an alternative
solution which would create some flexibility for multi channel
cases. Otherwise, we may try to get rid of the NETWORK mode,
and the TE/RE-together-set accordingly.

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2] ASoC: fsl_ssi: Fix channel swap on playback start
  2017-04-03 23:42         ` Caleb Crome
@ 2017-04-04  8:59           ` Arnaud Mouiche
  2017-04-04  9:03             ` Arnaud Mouiche
  2017-04-04 11:38             ` Fabio Estevam
  0 siblings, 2 replies; 30+ messages in thread
From: Arnaud Mouiche @ 2017-04-04  8:59 UTC (permalink / raw)
  To: Caleb Crome, Fabio Estevam
  Cc: alsa-devel, Sascha Hauer, Timur Tabi, Nicolin Chen, Mark Brown,
	Max Krummenacher, Fabio Estevam

So to summarize:

- Caleb and I don't see the issue without the patch, but we are working 
on DSP mode @ 48K (mostly as master of the bus). But the patch break 
none trivial "playback only" cases. platforms: imx6 quad for Caleb, 
imx6sl for me. We are working without codec, checking the bit stream 
generated by one SSI by recording and checking the content using another 
SSI.
- Fabio and Max experience the issue very easily in I2S mode, acting as 
slave (I guess otherwise generating precise 44.1Khz would be hard), 
connecting to a STGL5000 codec.

When you are master of the bus, it is important to start EN before TE 
for the FIFO pre-fill reasons. The samples need to be ready as soon as 
TE starts.
I also guess that ENGcm06222 doesn't affect us when the SSI is master 
(since the SSI is govern only by its own timing)

As slave, this is less important to start EN before TE because you have 
little chance to receive the SYNC trigger as soon as EN+TE starts => the 
DMA did get time to fill the FIFO.
Yet, as slave, ENGcm06222 affect the order of channels, as experienced 
by Fabio.

So I switch on I2S mode for my SSI => SSI tests and, sadly, I didn't 
experience issues without the patch.
I did the test on vanilla 4.11.0-rc5.

which branch/repository are Fabio using for his tests ?

The way clocks are configured may explain the difference:
Dumping /sys/kernel/debug/clk/clk_summary and checking the differences 
can give some clues.
In my case, I have, for the slave SSI #2, while the PCM bus is running 
at 48khz+I2Smode+2 channels.

     pll4                                  0            0 
786432000          0 0
        pll4_bypass                        0            0 
786432000          0 0
           pll4_audio                      0            0 
786432000          0 0
              pll4_post_div                0            0 
786432000          0 0
                 pll4_audio_div            0            0 
786432000          0 0
                    ssi2_sel               0            0 
786432000          0 0
                       ssi2_pred           0            0 
196608000          0 0
                          ssi2_podf           0 0    98304000          0 0
                             ssi2           0 0    98304000          0 0

Strangely, the 'enable_cnt' is kept equal to zero while the SSI transmit 
frames correctly...
Is there a patch or a branch somewhere that fix this issue ?

Also, here is a dump of SSI registers.
/var/root # cat /sys/kernel/debug/regmap/202c000.ssi/registers
00: 00000000
04: 00000000
10: 0000105b
18: 009031a3
1c: 00000285
20: 00000205
24: 0004e100
28: 00040100
2c: 00880888
30: 00000000
34: 00000000
38: 00000000
48: fffffffc
4c: fffffffc
50: 00000000
54: 00000000
58: 00000000


Arnaud

On 04/04/2017 01:42, Caleb Crome wrote:
> On Mon, Apr 3, 2017 at 4:40 PM, Fabio Estevam <festevam@gmail.com> wrote:
>> Hi Caleb,
>>
>> On Mon, Apr 3, 2017 at 8:22 PM, Caleb Crome <caleb@crome.org> wrote:
>>
>>> With a vanilla kernel, it works perfectly with the pinctrl patch.
>>> In this case, I ran a cable from the wandboard over to my computer and
>>> recorded with audacity, using your wile true script above.
>>> Here you can see that with 4.11-rc5 plus the pinctrl patch, there is
>>> no channel swapping:
>>>
>>> http://imgur.com/od0LoJP
>> Which wandboard type do you have? mx6solo,dl or quad?
>>
>> I am using  imx6dl-wandboard.
>>
>> I am surprised that the channel swap does not happen on your case.
>> Maybe you need to run the test for an extended period of time?
> Running on a quad.
>
>> On my case I usually get the swap in 1/10 of times. Max uses a Toradex
>> MX6DL Colibri board and sees the swap in 3-4% of the times.
> I'll run it for a much longer time and see what happens.

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2] ASoC: fsl_ssi: Fix channel swap on playback start
  2017-04-04  8:59           ` Arnaud Mouiche
@ 2017-04-04  9:03             ` Arnaud Mouiche
  2017-04-04 11:38             ` Fabio Estevam
  1 sibling, 0 replies; 30+ messages in thread
From: Arnaud Mouiche @ 2017-04-04  9:03 UTC (permalink / raw)
  To: Caleb Crome, Fabio Estevam
  Cc: alsa-devel, Sascha Hauer, Timur Tabi, Nicolin Chen, Mark Brown,
	Max Krummenacher, Fabio Estevam



On 04/04/2017 10:59, Arnaud Mouiche wrote:
> So to summarize:
>
> - Caleb and I don't see the issue without the patch, but we are 
> working on DSP mode @ 48K (mostly as master of the bus). But the patch 
> break none trivial "playback only" cases. platforms: imx6 quad for 
> Caleb, imx6sl for me. We are working without codec, checking the bit 
> stream generated by one SSI by recording and checking the content 
> using another SSI.
> - Fabio and Max experience the issue very easily in I2S mode, acting 
> as slave (I guess otherwise generating precise 44.1Khz would be hard), 
> connecting to a STGL5000 codec.
>
> When you are master of the bus, it is important to start EN before TE 
> for the FIFO pre-fill reasons. The samples need to be ready as soon as 
> TE starts.
> I also guess that ENGcm06222 doesn't affect us when the SSI is master 
> (since the SSI is govern only by its own timing)
>
> As slave, this is less important to start EN before TE because you 
> have little chance to receive the SYNC trigger as soon as EN+TE starts 
> => the DMA did get time to fill the FIFO.
> Yet, as slave, ENGcm06222 affect the order of channels, as experienced 
> by Fabio.
>
> So I switch on I2S mode for my SSI => SSI tests and, sadly, I didn't 
> experience issues without the patch.
> I did the test on vanilla 4.11.0-rc5.
>
> which branch/repository are Fabio using for his tests ?
>
> The way clocks are configured may explain the difference:
> Dumping /sys/kernel/debug/clk/clk_summary and checking the differences 
> can give some clues.
> In my case, I have, for the slave SSI #2, while the PCM bus is running 
> at 48khz+I2Smode+2 channels.
>
>     pll4                                  0            0 
> 786432000          0 0
>        pll4_bypass                        0            0 
> 786432000          0 0
>           pll4_audio                      0            0 
> 786432000          0 0
>              pll4_post_div                0            0 
> 786432000          0 0
>                 pll4_audio_div            0            0 
> 786432000          0 0
>                    ssi2_sel               0            0 
> 786432000          0 0
>                       ssi2_pred           0            0 
> 196608000          0 0
>                          ssi2_podf           0 0 98304000          0 0
>                             ssi2           0 0 98304000          0 0
>
> Strangely, the 'enable_cnt' is kept equal to zero while the SSI 
> transmit frames correctly...
> Is there a patch or a branch somewhere that fix this issue ?

Ok, I remember that only IPG clock is necessary when SSI is slave. So 
this is normal.
Arnaud

>
> Also, here is a dump of SSI registers.
> /var/root # cat /sys/kernel/debug/regmap/202c000.ssi/registers
> 00: 00000000
> 04: 00000000
> 10: 0000105b
> 18: 009031a3
> 1c: 00000285
> 20: 00000205
> 24: 0004e100
> 28: 00040100
> 2c: 00880888
> 30: 00000000
> 34: 00000000
> 38: 00000000
> 48: fffffffc
> 4c: fffffffc
> 50: 00000000
> 54: 00000000
> 58: 00000000
>
>
> Arnaud
>
> On 04/04/2017 01:42, Caleb Crome wrote:
>> On Mon, Apr 3, 2017 at 4:40 PM, Fabio Estevam <festevam@gmail.com> 
>> wrote:
>>> Hi Caleb,
>>>
>>> On Mon, Apr 3, 2017 at 8:22 PM, Caleb Crome <caleb@crome.org> wrote:
>>>
>>>> With a vanilla kernel, it works perfectly with the pinctrl patch.
>>>> In this case, I ran a cable from the wandboard over to my computer and
>>>> recorded with audacity, using your wile true script above.
>>>> Here you can see that with 4.11-rc5 plus the pinctrl patch, there is
>>>> no channel swapping:
>>>>
>>>> http://imgur.com/od0LoJP
>>> Which wandboard type do you have? mx6solo,dl or quad?
>>>
>>> I am using  imx6dl-wandboard.
>>>
>>> I am surprised that the channel swap does not happen on your case.
>>> Maybe you need to run the test for an extended period of time?
>> Running on a quad.
>>
>>> On my case I usually get the swap in 1/10 of times. Max uses a Toradex
>>> MX6DL Colibri board and sees the swap in 3-4% of the times.
>> I'll run it for a much longer time and see what happens.
>

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2] ASoC: fsl_ssi: Fix channel swap on playback start
  2017-04-04  8:59           ` Arnaud Mouiche
  2017-04-04  9:03             ` Arnaud Mouiche
@ 2017-04-04 11:38             ` Fabio Estevam
  2017-04-04 17:12               ` Fabio Estevam
  1 sibling, 1 reply; 30+ messages in thread
From: Fabio Estevam @ 2017-04-04 11:38 UTC (permalink / raw)
  To: Arnaud Mouiche
  Cc: alsa-devel, Sascha Hauer, Timur Tabi, Caleb Crome, Nicolin Chen,
	Mark Brown, Max Krummenacher, Fabio Estevam

Hi Arnaud,

On Tue, Apr 4, 2017 at 5:59 AM, Arnaud Mouiche
<arnaud.mouiche@invoxia.com> wrote:
> So to summarize:
>
> - Caleb and I don't see the issue without the patch, but we are working on
> DSP mode @ 48K (mostly as master of the bus). But the patch break none
> trivial "playback only" cases. platforms: imx6 quad for Caleb, imx6sl for
> me. We are working without codec, checking the bit stream generated by one
> SSI by recording and checking the content using another SSI.
> - Fabio and Max experience the issue very easily in I2S mode, acting as
> slave (I guess otherwise generating precise 44.1Khz would be hard),
> connecting to a STGL5000 codec.

That's correct. The mode is I2S slave in our case.

>
> When you are master of the bus, it is important to start EN before TE for
> the FIFO pre-fill reasons. The samples need to be ready as soon as TE
> starts.
> I also guess that ENGcm06222 doesn't affect us when the SSI is master (since
> the SSI is govern only by its own timing)
>
> As slave, this is less important to start EN before TE because you have
> little chance to receive the SYNC trigger as soon as EN+TE starts => the DMA
> did get time to fill the FIFO.
> Yet, as slave, ENGcm06222 affect the order of channels, as experienced by
> Fabio.
>
> So I switch on I2S mode for my SSI => SSI tests and, sadly, I didn't
> experience issues without the patch.
> I did the test on vanilla 4.11.0-rc5.
>
> which branch/repository are Fabio using for his tests ?

Right now I am using 4.11-rc5 + the pinctrl patch
https://patchwork.ozlabs.org/patch/745349/ .

The swap also happens with 3.14, 4.1.15 and 4.10.8.

> The way clocks are configured may explain the difference:
> Dumping /sys/kernel/debug/clk/clk_summary and checking the differences can
> give some clues.
> In my case, I have, for the slave SSI #2, while the PCM bus is running at
> 48khz+I2Smode+2 channels.


>     pll4                                  0            0 786432000
> 0 0
>        pll4_bypass                        0            0 786432000
> 0 0
>           pll4_audio                      0            0 786432000
> 0 0
>              pll4_post_div                0            0 786432000
> 0 0
>                 pll4_audio_div            0            0 786432000
> 0 0
>                    ssi2_sel               0            0 786432000
> 0 0
>                       ssi2_pred           0            0 196608000
> 0 0
>                          ssi2_podf           0 0    98304000          0 0
>                             ssi2           0 0    98304000          0 0


In the slave mode case while the wav is playing:

                         ipg              5            6    66000000
       0 0
                            usboh3           0            0
66000000          0 0
                            uart_ipg           1            2
66000000          0 0
                            ssi3_ipg           0            0
66000000          0 0
                            ssi2_ipg           0            0
66000000          0 0
                            ssi1_ipg           1            2
66000000          0 0


> Strangely, the 'enable_cnt' is kept equal to zero while the SSI transmit
> frames correctly...
> Is there a patch or a branch somewhere that fix this issue ?
>
> Also, here is a dump of SSI registers.
> /var/root # cat /sys/kernel/debug/regmap/202c000.ssi/registers
> 00: 00000000
> 04: 00000000
> 10: 0000105b
> 18: 009031a3
> 1c: 00000285
> 20: 00000205
> 24: 0004e100
> 28: 00040100
> 2c: 00880888
> 30: 00000000
> 34: 00000000
> 38: 00000000
> 48: fffffffc
> 4c: fffffffc
> 50: 00000000
> 54: 00000000
> 58: 00000000

I have the following SSI1 values (with the original 4.1-rc5 + pictrl patch):

# cat /sys/kernel/debug/regmap/2028000.ssi/registers
00: 00000200
04: 00000000
10: 0000105b
18: 009031a3
1c: 0000028d
20: 0000020d
24: 0004e100
28: 00040100
2c: 00880d88
30: 00000000
34: 00000000
38: 00000000
48: 00000000
4c: 00000000
50: 00000000
54: 00000000
58: 00000000

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2] ASoC: fsl_ssi: Fix channel swap on playback start
  2017-04-04 11:38             ` Fabio Estevam
@ 2017-04-04 17:12               ` Fabio Estevam
  2017-04-04 20:09                 ` Arnaud Mouiche
  0 siblings, 1 reply; 30+ messages in thread
From: Fabio Estevam @ 2017-04-04 17:12 UTC (permalink / raw)
  To: Arnaud Mouiche
  Cc: alsa-devel, Sascha Hauer, Timur Tabi, Caleb Crome, Nicolin Chen,
	Mark Brown, Max Krummenacher, Fabio Estevam

On Tue, Apr 4, 2017 at 8:38 AM, Fabio Estevam <festevam@gmail.com> wrote:

> I have the following SSI1 values (with the original 4.1-rc5 + pictrl patch):
>
> # cat /sys/kernel/debug/regmap/2028000.ssi/registers
> 00: 00000200
> 04: 00000000
> 10: 0000105b

Bits 6-5 (I2S_MODE) of register SCR are 10 of register SCR, which
means I2S slave mode.

The MX6 Reference Manual states:

"In I2S slave mode(SSI_SCR[6:5]=10), the following settings are
internally overridden by
the hardware:

Normal mode is selected (SSI_SCR[3]=0)
Tx frame sync length set to one-bit-long-frame (SSI_STCR[1]=1)
Rx frame sync length set to one-bit-long-frame (SSI_SRCR[1]=1)"

so I tried not to use the the one-bit-long-frame (since ENGcm06222 is
about bit length frame sync) override and changed it to I2S normal
mode instead:

--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -973,7 +973,7 @@ static int _fsl_ssi_set_dai_fmt(struct device *dev,
                        ssi_private->i2s_mode |= CCSR_SSI_SCR_I2S_MODE_MASTER;
                        break;
                case SND_SOC_DAIFMT_CBM_CFM:
-                       ssi_private->i2s_mode |= CCSR_SSI_SCR_I2S_MODE_SLAVE;
+                       ssi_private->i2s_mode |= CCSR_SSI_SCR_I2S_MODE_NORMAL;
                        break;
                default:
                        return -EINVAL;

and I do not get the channel swap anymore.

In what cases could we safely switch to I2S normal mode?

Thanks

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2] ASoC: fsl_ssi: Fix channel swap on playback start
  2017-04-04 17:12               ` Fabio Estevam
@ 2017-04-04 20:09                 ` Arnaud Mouiche
  2017-04-04 20:28                   ` Fabio Estevam
  0 siblings, 1 reply; 30+ messages in thread
From: Arnaud Mouiche @ 2017-04-04 20:09 UTC (permalink / raw)
  To: Fabio Estevam
  Cc: alsa-devel, Sascha Hauer, Timur Tabi, Caleb Crome, Nicolin Chen,
	Mark Brown, Max Krummenacher, Fabio Estevam



On 04/04/2017 19:12, Fabio Estevam wrote:
> On Tue, Apr 4, 2017 at 8:38 AM, Fabio Estevam <festevam@gmail.com> wrote:
>
>> I have the following SSI1 values (with the original 4.1-rc5 + pictrl patch):
>>
>> # cat /sys/kernel/debug/regmap/2028000.ssi/registers
>> 00: 00000200
>> 04: 00000000
>> 10: 0000105b
> Bits 6-5 (I2S_MODE) of register SCR are 10 of register SCR, which
> means I2S slave mode.
>
> The MX6 Reference Manual states:
>
> "In I2S slave mode(SSI_SCR[6:5]=10), the following settings are
> internally overridden by
> the hardware:
>
> Normal mode is selected (SSI_SCR[3]=0)
> Tx frame sync length set to one-bit-long-frame (SSI_STCR[1]=1)
> Rx frame sync length set to one-bit-long-frame (SSI_SRCR[1]=1)"
>
> so I tried not to use the the one-bit-long-frame (since ENGcm06222 is
> about bit length frame sync) override and changed it to I2S normal
> mode instead:
>
> --- a/sound/soc/fsl/fsl_ssi.c
> +++ b/sound/soc/fsl/fsl_ssi.c
> @@ -973,7 +973,7 @@ static int _fsl_ssi_set_dai_fmt(struct device *dev,
>                          ssi_private->i2s_mode |= CCSR_SSI_SCR_I2S_MODE_MASTER;
>                          break;
>                  case SND_SOC_DAIFMT_CBM_CFM:
> -                       ssi_private->i2s_mode |= CCSR_SSI_SCR_I2S_MODE_SLAVE;
> +                       ssi_private->i2s_mode |= CCSR_SSI_SCR_I2S_MODE_NORMAL;
>                          break;
>                  default:
>                          return -EINVAL;
>
> and I do not get the channel swap anymore.
>
> In what cases could we safely switch to I2S normal mode?

SCR bit 3 (NET) is also set, so you should be in network mode with a 
long frame sync.
In fact, you can entirely simulate a I2S behavior using Network mode. 
you should just be careful about the way everything is configured (eg. 
place of samples in the stream)


Also, I just read the ENGcm06222 chip errata.
http://www.nxp.com/assets/documents/data/en/errata/IMX35CE.pdf

and I don't understand why it affects us in this case.
- you are slave in your case and you don't send the Fsync
- it talk about writing EN and TE in the same frame (so with less than 
1/44100s).
=> Writing the register at once is simply a good way to be sure it is 
effective... except if it takes more than a frame to configure the whole 
stuff.
And I also don't understand how all of this could have work so long 
before in "Capture, then later, playback scenario", were TE is set very 
long after EN...

Arnaud

>
> Thanks

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2] ASoC: fsl_ssi: Fix channel swap on playback start
  2017-04-04 20:09                 ` Arnaud Mouiche
@ 2017-04-04 20:28                   ` Fabio Estevam
  2017-04-05  7:54                     ` Arnaud Mouiche
  0 siblings, 1 reply; 30+ messages in thread
From: Fabio Estevam @ 2017-04-04 20:28 UTC (permalink / raw)
  To: Arnaud Mouiche
  Cc: alsa-devel, Sascha Hauer, Timur Tabi, Caleb Crome, Nicolin Chen,
	Mark Brown, Max Krummenacher, Fabio Estevam

On Tue, Apr 4, 2017 at 5:09 PM, Arnaud Mouiche
<arnaud.mouiche@invoxia.com> wrote:

> SCR bit 3 (NET) is also set, so you should be in network mode with a long
> frame sync.
> In fact, you can entirely simulate a I2S behavior using Network mode. you
> should just be careful about the way everything is configured (eg. place of
> samples in the stream)

While debugging this issue I noticed that when I put the oscilloscope
probe in the LRCLK SGTL5000 pin the swap did not occur anymore.

After removing the probe the swap occurred frequently.

So decided to change the SGTL5000 LRCLK pin strength value:

--- a/sound/soc/codecs/sgtl5000.c
+++ b/sound/soc/codecs/sgtl5000.c
@@ -1118,7 +1118,7 @@ static int sgtl5000_probe(struct snd_soc_codec *codec)
                        SGTL5000_DAC_MUTE_RIGHT |
                        SGTL5000_DAC_MUTE_LEFT);

-       snd_soc_write(codec, SGTL5000_CHIP_PAD_STRENGTH, 0x015f);
+       snd_soc_write(codec, SGTL5000_CHIP_PAD_STRENGTH, 0x035f);

        snd_soc_write(codec, SGTL5000_CHIP_ANA_CTRL,
                        SGTL5000_HP_ZCD_EN |

and the swap does not happen.

So it seems that no change is needed on the imx-ssi side :-)

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2] ASoC: fsl_ssi: Fix channel swap on playback start
  2017-04-04 20:28                   ` Fabio Estevam
@ 2017-04-05  7:54                     ` Arnaud Mouiche
  2017-04-05 13:43                       ` Fabio Estevam
  2017-04-05 14:04                       ` Max Krummenacher
  0 siblings, 2 replies; 30+ messages in thread
From: Arnaud Mouiche @ 2017-04-05  7:54 UTC (permalink / raw)
  To: Fabio Estevam
  Cc: alsa-devel, Sascha Hauer, Timur Tabi, Caleb Crome, Nicolin Chen,
	Mark Brown, Max Krummenacher, Fabio Estevam



On 04/04/2017 22:28, Fabio Estevam wrote:
> On Tue, Apr 4, 2017 at 5:09 PM, Arnaud Mouiche
> <arnaud.mouiche@invoxia.com> wrote:
>
>> SCR bit 3 (NET) is also set, so you should be in network mode with a long
>> frame sync.
>> In fact, you can entirely simulate a I2S behavior using Network mode. you
>> should just be careful about the way everything is configured (eg. place of
>> samples in the stream)
> While debugging this issue I noticed that when I put the oscilloscope
> probe in the LRCLK SGTL5000 pin the swap did not occur anymore.
>
> After removing the probe the swap occurred frequently.
>
> So decided to change the SGTL5000 LRCLK pin strength value:
>
> --- a/sound/soc/codecs/sgtl5000.c
> +++ b/sound/soc/codecs/sgtl5000.c
> @@ -1118,7 +1118,7 @@ static int sgtl5000_probe(struct snd_soc_codec *codec)
>                          SGTL5000_DAC_MUTE_RIGHT |
>                          SGTL5000_DAC_MUTE_LEFT);
>
> -       snd_soc_write(codec, SGTL5000_CHIP_PAD_STRENGTH, 0x015f);
> +       snd_soc_write(codec, SGTL5000_CHIP_PAD_STRENGTH, 0x035f);
>
>          snd_soc_write(codec, SGTL5000_CHIP_ANA_CTRL,
>                          SGTL5000_HP_ZCD_EN |
>
> and the swap does not happen.
>
> So it seems that no change is needed on the imx-ssi side :-)
Good catch. All of this makes sense.
The SSI surely detect a glitch at the start of the stream and takes it 
for a sync frame, but not followed by the expected 32x2 bits.
It also explain why Caleb and I are not able to reproduce, since we 
connect SSI internally using the audmux, leaving no place for such glitch.

If only Max can validate this fix...
But what is strange is that writing TE and EN at once also avoid the 
issue... or it means the issue was really timing dependent.

Do you know which one is started first ?
- fsl_ssi_trigger(SNDRV_PCM_TRIGGER_START)
or
- stgl5000 PCM bus being turned on

We can expect that stgl5000 turns the PCM clocks first, and then SSI is 
turned on. Otherwise anything can happened when the codec starts its 
clocking.

Maybe we should look at the Fsync state when idle, and see how it behave 
during the startup. Depending of pull-up /down-down configuration of the 
pads, it may be leaved in a undefined state with undefined transitions 
when stgl5000 turns its output on...

Another way to definitively fix this kind of issue is to use 
SND_SOC_DAIFMT_CBS_CFS
- the codec generates the N*8*64 kHz or 44.1*64 kHz precise bitclock 
(something which is not flexible for the SSI who is connected to a fix 
PLL output clock)
- but the SSI generate the Sync, leaving no place for wrong detection.
Unfortunately, stgl5000 doesn't seem to support this mode.

Arnaud

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2] ASoC: fsl_ssi: Fix channel swap on playback start
  2017-04-05  7:54                     ` Arnaud Mouiche
@ 2017-04-05 13:43                       ` Fabio Estevam
  2017-04-05 14:04                       ` Max Krummenacher
  1 sibling, 0 replies; 30+ messages in thread
From: Fabio Estevam @ 2017-04-05 13:43 UTC (permalink / raw)
  To: Arnaud Mouiche
  Cc: alsa-devel, Sascha Hauer, Timur Tabi, Caleb Crome, Nicolin Chen,
	Mark Brown, Max Krummenacher, Fabio Estevam

On Wed, Apr 5, 2017 at 4:54 AM, Arnaud Mouiche
<arnaud.mouiche@invoxia.com> wrote:

> Good catch. All of this makes sense.
> The SSI surely detect a glitch at the start of the stream and takes it for a
> sync frame, but not followed by the expected 32x2 bits.
> It also explain why Caleb and I are not able to reproduce, since we connect
> SSI internally using the audmux, leaving no place for such glitch.
>
> If only Max can validate this fix...

Yes, that would be nice.

> But what is strange is that writing TE and EN at once also avoid the
> issue... or it means the issue was really timing dependent.
>
> Do you know which one is started first ?
> - fsl_ssi_trigger(SNDRV_PCM_TRIGGER_START)
> or
> - stgl5000 PCM bus being turned on
>
> We can expect that stgl5000 turns the PCM clocks first, and then SSI is
> turned on. Otherwise anything can happened when the codec starts its
> clocking.

Correct: stgl5000 turns the PCM clocks first.

> Maybe we should look at the Fsync state when idle, and see how it behave
> during the startup. Depending of pull-up /down-down configuration of the
> pads, it may be leaved in a undefined state with undefined transitions when
> stgl5000 turns its output on...
>
> Another way to definitively fix this kind of issue is to use
> SND_SOC_DAIFMT_CBS_CFS
> - the codec generates the N*8*64 kHz or 44.1*64 kHz precise bitclock
> (something which is not flexible for the SSI who is connected to a fix PLL
> output clock)
> - but the SSI generate the Sync, leaving no place for wrong detection.
> Unfortunately, stgl5000 doesn't seem to support this mode.

Correct. I plan to submitting a sgtl5000 patch that allows setting the
lrclk pad strength via device tree.

Will wait for Max to confirm if this solves the swap channel issue on
his board as well.

I would like to thank you and Caleb for the tests.

Glad to see your atest application working as expected with the SSI driver :-)

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2] ASoC: fsl_ssi: Fix channel swap on playback start
  2017-04-05  7:54                     ` Arnaud Mouiche
  2017-04-05 13:43                       ` Fabio Estevam
@ 2017-04-05 14:04                       ` Max Krummenacher
  1 sibling, 0 replies; 30+ messages in thread
From: Max Krummenacher @ 2017-04-05 14:04 UTC (permalink / raw)
  To: Arnaud Mouiche, Fabio Estevam
  Cc: alsa-devel, Timur Tabi, Caleb Crome, Nicolin Chen, Mark Brown,
	Sascha Hauer, Fabio Estevam

(resend from my alsa-devel registered email address)

On Wed, 2017-04-05 at 09:54 +0200, Arnaud Mouiche wrote:
> 
> 
> On 04/04/2017 22:28, Fabio Estevam wrote:
> > 
> > 
> > On Tue, Apr 4, 2017 at 5:09 PM, Arnaud Mouiche
> > <arnaud.mouiche@invoxia.com> wrote:
> > 
> > > 
> > > 
> > > SCR bit 3 (NET) is also set, so you should be in network mode with a long
> > > frame sync.
> > > In fact, you can entirely simulate a I2S behavior using Network mode. you
> > > should just be careful about the way everything is configured (eg. place of
> > > samples in the stream)
> > While debugging this issue I noticed that when I put the oscilloscope
> > probe in the LRCLK SGTL5000 pin the swap did not occur anymore.
> > 
> > After removing the probe the swap occurred frequently.
> > 
> > So decided to change the SGTL5000 LRCLK pin strength value:
> > 
> > --- a/sound/soc/codecs/sgtl5000.c
> > +++ b/sound/soc/codecs/sgtl5000.c
> > @@ -1118,7 +1118,7 @@ static int sgtl5000_probe(struct snd_soc_codec *codec)
> >                          SGTL5000_DAC_MUTE_RIGHT |
> >                          SGTL5000_DAC_MUTE_LEFT);
> > 
> > -       snd_soc_write(codec, SGTL5000_CHIP_PAD_STRENGTH, 0x015f);
> > +       snd_soc_write(codec, SGTL5000_CHIP_PAD_STRENGTH, 0x035f);
> > 
> >          snd_soc_write(codec, SGTL5000_CHIP_ANA_CTRL,
> >                          SGTL5000_HP_ZCD_EN |
> > 
> > and the swap does not happen.
> > 
> > So it seems that no change is needed on the imx-ssi side :-)
> Good catch. All of this makes sense.
> The SSI surely detect a glitch at the start of the stream and takes it 
> for a sync frame, but not followed by the expected 32x2 bits.
> It also explain why Caleb and I are not able to reproduce, since we 
> connect SSI internally using the audmux, leaving no place for such glitch.
> 
> If only Max can validate this fix...

Reverting the previous patch (setting TE and RE) and changing the drive
strenght to 0x035f fixes the channel swap on the Colibri iMX6DL.
e.g. 1000 playback starts do all have the correct channel assignment.

Looks like having the the LRCLK a bit earlier through the increased drive
strength changes the timing in a way that when the SSI samples that signal
with its correct state.
Probably the previous patch with TE and RE changes the sampling time in
the SSI which made it work in our use case.

Max

> 
> But what is strange is that writing TE and EN at once also avoid the 
> issue... or it means the issue was really timing dependent.
> 
> Do you know which one is started first ?
> - fsl_ssi_trigger(SNDRV_PCM_TRIGGER_START)
> or
> - stgl5000 PCM bus being turned on
> 
> We can expect that stgl5000 turns the PCM clocks first, and then SSI is 
> turned on. Otherwise anything can happened when the codec starts its 
> clocking.
> 
> Maybe we should look at the Fsync state when idle, and see how it behave 
> during the startup. Depending of pull-up /down-down configuration of the 
> pads, it may be leaved in a undefined state with undefined transitions 
> when stgl5000 turns its output on...
> 
> Another way to definitively fix this kind of issue is to use 
> SND_SOC_DAIFMT_CBS_CFS
> - the codec generates the N*8*64 kHz or 44.1*64 kHz precise bitclock 
> (something which is not flexible for the SSI who is connected to a fix 
> PLL output clock)
> - but the SSI generate the Sync, leaving no place for wrong detection.
> Unfortunately, stgl5000 doesn't seem to support this mode.
> 
> Arnaud
> 

^ permalink raw reply	[flat|nested] 30+ messages in thread

end of thread, other threads:[~2017-04-05 14:04 UTC | newest]

Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-01 14:48 [PATCH v2] ASoC: fsl_ssi: Fix channel swap on playback start Fabio Estevam
2017-04-01 15:13 ` Arnaud Mouiche
2017-04-01 16:03   ` Fabio Estevam
2017-04-03  8:19 ` Max Krummenacher
2017-04-03 20:32 ` Caleb Crome
2017-04-03 20:50   ` Caleb Crome
2017-04-03 21:53   ` Fabio Estevam
2017-04-03 22:05     ` Arnaud Mouiche
2017-04-03 22:20       ` Nicolin Chen
2017-04-03 23:22     ` Caleb Crome
2017-04-03 23:40       ` Fabio Estevam
2017-04-03 23:42         ` Caleb Crome
2017-04-04  8:59           ` Arnaud Mouiche
2017-04-04  9:03             ` Arnaud Mouiche
2017-04-04 11:38             ` Fabio Estevam
2017-04-04 17:12               ` Fabio Estevam
2017-04-04 20:09                 ` Arnaud Mouiche
2017-04-04 20:28                   ` Fabio Estevam
2017-04-05  7:54                     ` Arnaud Mouiche
2017-04-05 13:43                       ` Fabio Estevam
2017-04-05 14:04                       ` Max Krummenacher
2017-04-03 22:08   ` Nicolin Chen
2017-04-03 23:31     ` Caleb Crome
2017-04-03 23:55       ` Nicolin Chen
2017-04-04  0:07         ` Timur Tabi
2017-04-04  0:39         ` Caleb Crome
2017-04-04  1:25           ` Nicolin Chen
2017-04-03 22:36 ` Nicolin Chen
2017-04-03 22:54   ` Fabio Estevam
2017-04-04  0:08     ` Nicolin Chen

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.