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

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

[Fabio Estevam]

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>
---
 sound/soc/fsl/fsl_ssi.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index fde08660..17f92b8 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -93,6 +93,9 @@
 		CCSR_SSI_SIER_TLS_EN | CCSR_SSI_SIER_TFS_EN | \
 		CCSR_SSI_SIER_TUE0_EN | CCSR_SSI_SIER_TFRC_EN)
 
+#define FSLSSI_SSIEN_WORKAROUND (CCSR_SSI_SCR_SSIEN | CCSR_SSI_SCR_TE | \
+				 CCSR_SSI_SCR_RE)
+
 enum fsl_ssi_type {
 	FSL_SSI_MCP8610,
 	FSL_SSI_MX21,
@@ -559,7 +562,8 @@ static void fsl_ssi_config(struct fsl_ssi_private *ssi_private, bool enable,
 			int i;
 			int max_loop = 100;
 			regmap_update_bits(regs, CCSR_SSI_SCR,
-					CCSR_SSI_SCR_SSIEN, CCSR_SSI_SCR_SSIEN);
+					   FSLSSI_SSIEN_WORKAROUND,
+					   FSLSSI_SSIEN_WORKAROUND);
 			for (i = 0; i < max_loop; i++) {
 				u32 sfcsr;
 				regmap_read(regs, CCSR_SSI_SFCSR, &sfcsr);
@@ -650,8 +654,7 @@ static void fsl_ssi_setup_ac97(struct fsl_ssi_private *ssi_private)
 	 * codec before a stream is started.
 	 */
 	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);
+			   FSLSSI_SSIEN_WORKAROUND, FSLSSI_SSIEN_WORKAROUND);
 
 	regmap_write(regs, CCSR_SSI_SOR, CCSR_SSI_SOR_WAIT(3));
 }
-- 
2.7.4

Re: [PATCH] ASoC: fsl_ssi: Fix channel swap on playback start

[Nicolin Chen]

On Fri, Mar 31, 2017 at 07:15:03PM -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.

Hmm..so this bug also exists for imx6? I googled it and found that
only imx25 and imx35 ring the bell. I forgot if they keep the same
version for imx6 though.

> Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
> ---
>  sound/soc/fsl/fsl_ssi.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
> index fde08660..17f92b8 100644
> --- a/sound/soc/fsl/fsl_ssi.c
> +++ b/sound/soc/fsl/fsl_ssi.c
> @@ -93,6 +93,9 @@
>  		CCSR_SSI_SIER_TLS_EN | CCSR_SSI_SIER_TFS_EN | \
>  		CCSR_SSI_SIER_TUE0_EN | CCSR_SSI_SIER_TFRC_EN)
>  
> +#define FSLSSI_SSIEN_WORKAROUND (CCSR_SSI_SCR_SSIEN | CCSR_SSI_SCR_TE | \
> +				 CCSR_SSI_SCR_RE)

Enable RE??

>  enum fsl_ssi_type {
>  	FSL_SSI_MCP8610,
>  	FSL_SSI_MX21,
> @@ -559,7 +562,8 @@ static void fsl_ssi_config(struct fsl_ssi_private *ssi_private, bool enable,
>  			int i;
>  			int max_loop = 100;
>  			regmap_update_bits(regs, CCSR_SSI_SCR,
> -					CCSR_SSI_SCR_SSIEN, CCSR_SSI_SCR_SSIEN);
> +					   FSLSSI_SSIEN_WORKAROUND,
> +					   FSLSSI_SSIEN_WORKAROUND);
>  			for (i = 0; i < max_loop; i++) {
>  				u32 sfcsr;
>  				regmap_read(regs, CCSR_SSI_SFCSR, &sfcsr);

If this change is made, the whole "if (enable)" part here seems
to be meaningless (or even worse) as it aimed to set TE later
than SSIEN so as to offset the delay from DMA TX.

Check: https://patchwork.kernel.org/patch/9091051/

If this errata is mandatory, we probably should revert that the
commit and find other solution/workaround for Arnaud and Caleb.

Re: [PATCH] ASoC: fsl_ssi: Fix channel swap on playback start

[Fabio Estevam]

Hi Nicolin,

On Fri, Mar 31, 2017 at 8:53 PM, Nicolin Chen <nicoleotsuka@gmail.com> wrote:

>> +#define FSLSSI_SSIEN_WORKAROUND (CCSR_SSI_SCR_SSIEN | CCSR_SSI_SCR_TE | \
>> +                              CCSR_SSI_SCR_RE)
>
> Enable RE??

Yes, same idea as in f8fdf5375e2005f2 ("ASoC: fsl-ssi: add SSIEN
errata work around").

The idea here was not to restrict the erratum to AC97 mode only.

>
>>  enum fsl_ssi_type {
>>       FSL_SSI_MCP8610,
>>       FSL_SSI_MX21,
>> @@ -559,7 +562,8 @@ static void fsl_ssi_config(struct fsl_ssi_private *ssi_private, bool enable,
>>                       int i;
>>                       int max_loop = 100;
>>                       regmap_update_bits(regs, CCSR_SSI_SCR,
>> -                                     CCSR_SSI_SCR_SSIEN, CCSR_SSI_SCR_SSIEN);
>> +                                        FSLSSI_SSIEN_WORKAROUND,
>> +                                        FSLSSI_SSIEN_WORKAROUND);
>>                       for (i = 0; i < max_loop; i++) {
>>                               u32 sfcsr;
>>                               regmap_read(regs, CCSR_SSI_SFCSR, &sfcsr);
>
> If this change is made, the whole "if (enable)" part here seems
> to be meaningless (or even worse) as it aimed to set TE later
> than SSIEN so as to offset the delay from DMA TX.
>
> Check: https://patchwork.kernel.org/patch/9091051/
>
> If this errata is mandatory, we probably should revert that the
> commit and find other solution/workaround for Arnaud and Caleb.

Reverting 61fcf10a0ee44763e0 ("ASoC: fsl_ssi: Fix channel slipping in
Playback at startup") still causes channel swaps on my tests, so
better not to revert it.

Re: [PATCH] ASoC: fsl_ssi: Fix channel swap on playback start

[Nicolin Chen]

Hi

On Fri, Mar 31, 2017 at 09:59:46PM -0300, Fabio Estevam wrote:

> >> +#define FSLSSI_SSIEN_WORKAROUND (CCSR_SSI_SCR_SSIEN | CCSR_SSI_SCR_TE | \
> >> +                              CCSR_SSI_SCR_RE)
> >
> > Enable RE??
> 
> Yes, same idea as in f8fdf5375e2005f2 ("ASoC: fsl-ssi: add SSIEN
> errata work around").
> 
> The idea here was not to restrict the erratum to AC97 mode only.

I understood. Just enabling RE anyway (for I2S) had confused me.

> >> @@ -559,7 +562,8 @@ static void fsl_ssi_config(struct fsl_ssi_private *ssi_private, bool enable,
> >>                       int i;
> >>                       int max_loop = 100;
> >>                       regmap_update_bits(regs, CCSR_SSI_SCR,
> >> -                                     CCSR_SSI_SCR_SSIEN, CCSR_SSI_SCR_SSIEN);
> >> +                                        FSLSSI_SSIEN_WORKAROUND,
> >> +                                        FSLSSI_SSIEN_WORKAROUND);
> >>                       for (i = 0; i < max_loop; i++) {
> >>                               u32 sfcsr;
> >>                               regmap_read(regs, CCSR_SSI_SFCSR, &sfcsr);
> >
> > If this change is made, the whole "if (enable)" part here seems
> > to be meaningless (or even worse) as it aimed to set TE later
> > than SSIEN so as to offset the delay from DMA TX.
> >
> > Check: https://patchwork.kernel.org/patch/9091051/
> >
> > If this errata is mandatory, we probably should revert that the
> > commit and find other solution/workaround for Arnaud and Caleb.
> 
> Reverting 61fcf10a0ee44763e0 ("ASoC: fsl_ssi: Fix channel slipping in
> Playback at startup") still causes channel swaps on my tests, so
> better not to revert it.

That sounds weird to me. Prior to that commit, the code was setting
SSIEN and TE at the same time if I am not wrong. You could try to
revert it and check the SCR value before/after the last line:
    regmap_update_bits(regs, CCSR_SSI_SCR, vals->scr, vals->scr);