| Message ID | 1491058131-31366-1-git-send-email-festevam@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 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 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?
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 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
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
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.
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.
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 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.
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.
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.
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. >
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 --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); } }