diff mbox

[v2] ASoC: fsl_ssi: Fix channel swap on playback start

Message ID 1491058131-31366-1-git-send-email-festevam@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Fabio Estevam April 1, 2017, 2:48 p.m. UTC
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(-)

Comments

Arnaud Mouiche April 1, 2017, 3:13 p.m. UTC | #1
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
Fabio Estevam April 1, 2017, 4:03 p.m. UTC | #2
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.
Max Krummenacher April 3, 2017, 8:19 a.m. UTC | #3
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);
>  	}
>  }
>
Caleb Crome April 3, 2017, 8:32 p.m. UTC | #4
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
Caleb Crome April 3, 2017, 8:50 p.m. UTC | #5
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
Fabio Estevam April 3, 2017, 9:53 p.m. UTC | #6
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
Arnaud Mouiche April 3, 2017, 10:05 p.m. UTC | #7
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
Nicolin Chen April 3, 2017, 10:08 p.m. UTC | #8
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.
Nicolin Chen April 3, 2017, 10:20 p.m. UTC | #9
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?
Nicolin Chen April 3, 2017, 10:36 p.m. UTC | #10
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?
Fabio Estevam April 3, 2017, 10:54 p.m. UTC | #11
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?
Caleb Crome April 3, 2017, 11:22 p.m. UTC | #12
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
Caleb Crome April 3, 2017, 11:31 p.m. UTC | #13
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
Fabio Estevam April 3, 2017, 11:40 p.m. UTC | #14
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.
Caleb Crome April 3, 2017, 11:42 p.m. UTC | #15
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.
Nicolin Chen April 3, 2017, 11:55 p.m. UTC | #16
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?
Timur Tabi April 4, 2017, 12:07 a.m. UTC | #17
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.
Nicolin Chen April 4, 2017, 12:08 a.m. UTC | #18
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.
Caleb Crome April 4, 2017, 12:39 a.m. UTC | #19
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
Nicolin Chen April 4, 2017, 1:25 a.m. UTC | #20
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.
Arnaud Mouiche April 4, 2017, 8:59 a.m. UTC | #21
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.
Arnaud Mouiche April 4, 2017, 9:03 a.m. UTC | #22
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.
>
Fabio Estevam April 4, 2017, 11:38 a.m. UTC | #23
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
diff mbox

Patch

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);
 	}
 }