[alsa-devel] [PATCH v2] ASoC: fsl_ssi: Fix channel swap on playback start
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); } }
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
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.
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);}
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
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
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
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
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?
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:
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
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:
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.
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:
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.
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:
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.
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 0786432000 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:
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.
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 7864320000 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
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
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
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 :-)
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
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 :-)
(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
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.
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
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?
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.
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
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.
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?
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?
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.
participants (7)
-
Arnaud Mouiche -
Caleb Crome -
Fabio Estevam -
Max Krummenacher -
Max Krummenacher -
Nicolin Chen -
Timur Tabi