[PATCH] ASoC: fsl_sai: Force a software reset when starting in consumer mode

[PATCH] ASoC: fsl_sai: Force a software reset when starting in consumer mode

[Arun Raghavan]

From: Arun Raghavan <arun@asymptotic.io>

In a setup with an external clock provider, when running the receiver
(arecord) and triggering an xrun with xrun_injection, we see a channel
swap/offset. This happens sometimes when running only the receiver, but
occurs reliably if a transmitter (aplay) is also concurrently running.

The theory is that SAI seems to lose track of frame sync during the
trigger stop -> trigger start cycle that occurs during an xrun. Doing
just a FIFO reset in this case does not suffice, and only a software
reset seems to get it back on track.

Signed-off-by: Arun Raghavan <arun@asymptotic.io>
Reported-by: Pieterjan Camerlynck <p.camerlynck@televic.com>
---
 sound/soc/fsl/fsl_sai.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
index af1a168d35e3..3a5ebf32903f 100644
--- a/sound/soc/fsl/fsl_sai.c
+++ b/sound/soc/fsl/fsl_sai.c
@@ -841,6 +841,18 @@ static int fsl_sai_trigger(struct snd_pcm_substream *substream, int cmd,
 	case SNDRV_PCM_TRIGGER_START:
 	case SNDRV_PCM_TRIGGER_RESUME:
 	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
+		/*
+		 * Force a software reset if we are not the clock provider, as we
+		 * might have lost frame sync during xrun recovery.
+		 */
+		if (sai->is_consumer_mode) {
+			regmap_update_bits(sai->regmap,
+					FSL_SAI_xCSR(tx, ofs), FSL_SAI_CSR_SR,
+					FSL_SAI_CSR_SR);
+			regmap_update_bits(sai->regmap,
+					FSL_SAI_xCSR(tx, ofs), FSL_SAI_CSR_SR,
+					0);
+		}
 		regmap_update_bits(sai->regmap, FSL_SAI_xCSR(tx, ofs),
 				   FSL_SAI_CSR_FRDE, FSL_SAI_CSR_FRDE);
 
-- 
2.49.0

Re: [PATCH] ASoC: fsl_sai: Force a software reset when starting in consumer mode

[kernel test robot]

Hi Arun,

kernel test robot noticed the following build warnings:

[auto build test WARNING on broonie-sound/for-next]
[also build test WARNING on linus/master v6.16-rc3 next-20250625]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Arun-Raghavan/ASoC-fsl_sai-Force-a-software-reset-when-starting-in-consumer-mode/20250625-210824
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
patch link:    https://lore.kernel.org/r/20250625130648.201331-1-arun%40arunraghavan.net
patch subject: [PATCH] ASoC: fsl_sai: Force a software reset when starting in consumer mode
config: arc-randconfig-002-20250626 (https://download.01.org/0day-ci/archive/20250626/202506260745.Z7PLEr3i-lkp@intel.com/config)
compiler: arc-linux-gcc (GCC) 13.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250626/202506260745.Z7PLEr3i-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202506260745.Z7PLEr3i-lkp@intel.com/

All warnings (new ones prefixed by >>):

   sound/soc/fsl/fsl_sai.c: In function 'fsl_sai_trigger':
>> sound/soc/fsl/fsl_sai.c:848:21: warning: the comparison will always evaluate as 'true' for the address of 'is_consumer_mode' will never be NULL [-Waddress]
     848 |                 if (sai->is_consumer_mode) {
         |                     ^~~
   In file included from sound/soc/fsl/fsl_sai.c:24:
   sound/soc/fsl/fsl_sai.h:287:14: note: 'is_consumer_mode' declared here
     287 |         bool is_consumer_mode[2];
         |              ^~~~~~~~~~~~~~~~


vim +848 sound/soc/fsl/fsl_sai.c

   814	
   815	static int fsl_sai_trigger(struct snd_pcm_substream *substream, int cmd,
   816			struct snd_soc_dai *cpu_dai)
   817	{
   818		struct fsl_sai *sai = snd_soc_dai_get_drvdata(cpu_dai);
   819		unsigned int ofs = sai->soc_data->reg_offset;
   820	
   821		bool tx = substream->stream == SNDRV_PCM_STREAM_PLAYBACK;
   822		int adir = tx ? RX : TX;
   823		int dir = tx ? TX : RX;
   824		u32 xcsr;
   825	
   826		/*
   827		 * Asynchronous mode: Clear SYNC for both Tx and Rx.
   828		 * Rx sync with Tx clocks: Clear SYNC for Tx, set it for Rx.
   829		 * Tx sync with Rx clocks: Clear SYNC for Rx, set it for Tx.
   830		 */
   831		regmap_update_bits(sai->regmap, FSL_SAI_TCR2(ofs), FSL_SAI_CR2_SYNC,
   832				   sai->synchronous[TX] ? FSL_SAI_CR2_SYNC : 0);
   833		regmap_update_bits(sai->regmap, FSL_SAI_RCR2(ofs), FSL_SAI_CR2_SYNC,
   834				   sai->synchronous[RX] ? FSL_SAI_CR2_SYNC : 0);
   835	
   836		/*
   837		 * It is recommended that the transmitter is the last enabled
   838		 * and the first disabled.
   839		 */
   840		switch (cmd) {
   841		case SNDRV_PCM_TRIGGER_START:
   842		case SNDRV_PCM_TRIGGER_RESUME:
   843		case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
   844			/*
   845			 * Force a software reset if we are not the clock provider, as we
   846			 * might have lost frame sync during xrun recovery.
   847			 */
 > 848			if (sai->is_consumer_mode) {
   849				regmap_update_bits(sai->regmap,
   850						FSL_SAI_xCSR(tx, ofs), FSL_SAI_CSR_SR,
   851						FSL_SAI_CSR_SR);
   852				regmap_update_bits(sai->regmap,
   853						FSL_SAI_xCSR(tx, ofs), FSL_SAI_CSR_SR,
   854						0);
   855			}
   856			regmap_update_bits(sai->regmap, FSL_SAI_xCSR(tx, ofs),
   857					   FSL_SAI_CSR_FRDE, FSL_SAI_CSR_FRDE);
   858	
   859			regmap_update_bits(sai->regmap, FSL_SAI_xCSR(tx, ofs),
   860					   FSL_SAI_CSR_TERE, FSL_SAI_CSR_TERE);
   861			/*
   862			 * Enable the opposite direction for synchronous mode
   863			 * 1. Tx sync with Rx: only set RE for Rx; set TE & RE for Tx
   864			 * 2. Rx sync with Tx: only set TE for Tx; set RE & TE for Rx
   865			 *
   866			 * RM recommends to enable RE after TE for case 1 and to enable
   867			 * TE after RE for case 2, but we here may not always guarantee
   868			 * that happens: "arecord 1.wav; aplay 2.wav" in case 1 enables
   869			 * TE after RE, which is against what RM recommends but should
   870			 * be safe to do, judging by years of testing results.
   871			 */
   872			if (fsl_sai_dir_is_synced(sai, adir))
   873				regmap_update_bits(sai->regmap, FSL_SAI_xCSR((!tx), ofs),
   874						   FSL_SAI_CSR_TERE, FSL_SAI_CSR_TERE);
   875	
   876			regmap_update_bits(sai->regmap, FSL_SAI_xCSR(tx, ofs),
   877					   FSL_SAI_CSR_xIE_MASK, FSL_SAI_FLAGS);
   878			break;
   879		case SNDRV_PCM_TRIGGER_STOP:
   880		case SNDRV_PCM_TRIGGER_SUSPEND:
   881		case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
   882			regmap_update_bits(sai->regmap, FSL_SAI_xCSR(tx, ofs),
   883					   FSL_SAI_CSR_FRDE, 0);
   884			regmap_update_bits(sai->regmap, FSL_SAI_xCSR(tx, ofs),
   885					   FSL_SAI_CSR_xIE_MASK, 0);
   886	
   887			/* Check if the opposite FRDE is also disabled */
   888			regmap_read(sai->regmap, FSL_SAI_xCSR(!tx, ofs), &xcsr);
   889	
   890			/*
   891			 * If opposite stream provides clocks for synchronous mode and
   892			 * it is inactive, disable it before disabling the current one
   893			 */
   894			if (fsl_sai_dir_is_synced(sai, adir) && !(xcsr & FSL_SAI_CSR_FRDE))
   895				fsl_sai_config_disable(sai, adir);
   896	
   897			/*
   898			 * Disable current stream if either of:
   899			 * 1. current stream doesn't provide clocks for synchronous mode
   900			 * 2. current stream provides clocks for synchronous mode but no
   901			 *    more stream is active.
   902			 */
   903			if (!fsl_sai_dir_is_synced(sai, dir) || !(xcsr & FSL_SAI_CSR_FRDE))
   904				fsl_sai_config_disable(sai, dir);
   905	
   906			break;
   907		default:
   908			return -EINVAL;
   909		}
   910	
   911		return 0;
   912	}
   913	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH v2] ASoC: fsl_sai: Force a software reset when starting in consumer mode

[Arun Raghavan]

From: Arun Raghavan <arun@asymptotic.io>

In a setup with an external clock provider, when running the receiver
(arecord) and triggering an xrun with xrun_injection, we see a channel
swap/offset. This happens sometimes when running only the receiver, but
occurs reliably if a transmitter (aplay) is also concurrently running.

The theory is that SAI seems to lose track of frame sync during the
trigger stop -> trigger start cycle that occurs during an xrun. Doing
just a FIFO reset in this case does not suffice, and only a software
reset seems to get it back on track.

Signed-off-by: Arun Raghavan <arun@asymptotic.io>
Reported-by: Pieterjan Camerlynck <p.camerlynck@televic.com>
---

v2:
- Address build warning from kernel test robot

 sound/soc/fsl/fsl_sai.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
index af1a168d35e3..d158352c7640 100644
--- a/sound/soc/fsl/fsl_sai.c
+++ b/sound/soc/fsl/fsl_sai.c
@@ -841,6 +841,18 @@ static int fsl_sai_trigger(struct snd_pcm_substream *substream, int cmd,
 	case SNDRV_PCM_TRIGGER_START:
 	case SNDRV_PCM_TRIGGER_RESUME:
 	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
+		/*
+		 * Force a software reset if we are not the clock provider, as we
+		 * might have lost frame sync during xrun recovery.
+		 */
+		if (sai->is_consumer_mode[tx]) {
+			regmap_update_bits(sai->regmap,
+					FSL_SAI_xCSR(tx, ofs), FSL_SAI_CSR_SR,
+					FSL_SAI_CSR_SR);
+			regmap_update_bits(sai->regmap,
+					FSL_SAI_xCSR(tx, ofs), FSL_SAI_CSR_SR,
+					0);
+		}
 		regmap_update_bits(sai->regmap, FSL_SAI_xCSR(tx, ofs),
 				   FSL_SAI_CSR_FRDE, FSL_SAI_CSR_FRDE);
 
-- 
2.49.0

Re: [PATCH v2] ASoC: fsl_sai: Force a software reset when starting in consumer mode

[Shengjiu Wang]

On Thu, Jun 26, 2025 at 7:58 AM Arun Raghavan <arun@arunraghavan.net> wrote:
>
> From: Arun Raghavan <arun@asymptotic.io>
>
> In a setup with an external clock provider, when running the receiver
> (arecord) and triggering an xrun with xrun_injection, we see a channel
> swap/offset. This happens sometimes when running only the receiver, but
> occurs reliably if a transmitter (aplay) is also concurrently running.
>
> The theory is that SAI seems to lose track of frame sync during the
> trigger stop -> trigger start cycle that occurs during an xrun. Doing
> just a FIFO reset in this case does not suffice, and only a software
> reset seems to get it back on track.
>
> Signed-off-by: Arun Raghavan <arun@asymptotic.io>
> Reported-by: Pieterjan Camerlynck <p.camerlynck@televic.com>
> ---
>
> v2:
> - Address build warning from kernel test robot
>
>  sound/soc/fsl/fsl_sai.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
> index af1a168d35e3..d158352c7640 100644
> --- a/sound/soc/fsl/fsl_sai.c
> +++ b/sound/soc/fsl/fsl_sai.c
> @@ -841,6 +841,18 @@ static int fsl_sai_trigger(struct snd_pcm_substream *substream, int cmd,
>         case SNDRV_PCM_TRIGGER_START:
>         case SNDRV_PCM_TRIGGER_RESUME:
>         case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> +               /*
> +                * Force a software reset if we are not the clock provider, as we
> +                * might have lost frame sync during xrun recovery.
> +                */
> +               if (sai->is_consumer_mode[tx]) {
> +                       regmap_update_bits(sai->regmap,
> +                                       FSL_SAI_xCSR(tx, ofs), FSL_SAI_CSR_SR,
> +                                       FSL_SAI_CSR_SR);
> +                       regmap_update_bits(sai->regmap,
> +                                       FSL_SAI_xCSR(tx, ofs), FSL_SAI_CSR_SR,
> +                                       0);
> +               }

Which platform are you using?  and please add chip info in your commit
message.

This change can be moved to fsl_sai_config_disable(). that is:

--- a/sound/soc/fsl/fsl_sai.c
+++ b/sound/soc/fsl/fsl_sai.c
@@ -798,18 +798,16 @@ static void fsl_sai_config_disable(struct
fsl_sai *sai, int dir)
                           FSL_SAI_CSR_FR, FSL_SAI_CSR_FR);

        /*
-        * For sai master mode, after several open/close sai,
+        * For sai master/slave mode, after several open/close sai,
         * there will be no frame clock, and can't recover
         * anymore. Add software reset to fix this issue.
         * This is a hardware bug, and will be fix in the
         * next sai version.
         */
-       if (!sai->is_consumer_mode[tx]) {
-               /* Software Reset */
-               regmap_write(sai->regmap, FSL_SAI_xCSR(tx, ofs),
FSL_SAI_CSR_SR);
-               /* Clear SR bit to finish the reset */
-               regmap_write(sai->regmap, FSL_SAI_xCSR(tx, ofs), 0);
-       }
+       /* Software Reset */
+       regmap_write(sai->regmap, FSL_SAI_xCSR(tx, ofs), FSL_SAI_CSR_SR);
+       /* Clear SR bit to finish the reset */
+       regmap_write(sai->regmap, FSL_SAI_xCSR(tx, ofs), 0);
 }

Could you please try the above change to also work for your case?

Best regards
Shengjiu Wang

>                 regmap_update_bits(sai->regmap, FSL_SAI_xCSR(tx, ofs),
>                                    FSL_SAI_CSR_FRDE, FSL_SAI_CSR_FRDE);
>
> --
> 2.49.0
>

Re: [PATCH v2] ASoC: fsl_sai: Force a software reset when starting in consumer mode

[Arun Raghavan]

On Thu, 26 Jun 2025, at 2:55 AM, Shengjiu Wang wrote:
> On Thu, Jun 26, 2025 at 7:58 AM Arun Raghavan <arun@arunraghavan.net> wrote:
>>
>> From: Arun Raghavan <arun@asymptotic.io>
>>
>> In a setup with an external clock provider, when running the receiver
>> (arecord) and triggering an xrun with xrun_injection, we see a channel
>> swap/offset. This happens sometimes when running only the receiver, but
>> occurs reliably if a transmitter (aplay) is also concurrently running.
>>
>> The theory is that SAI seems to lose track of frame sync during the
>> trigger stop -> trigger start cycle that occurs during an xrun. Doing
>> just a FIFO reset in this case does not suffice, and only a software
>> reset seems to get it back on track.
>>
>> Signed-off-by: Arun Raghavan <arun@asymptotic.io>
>> Reported-by: Pieterjan Camerlynck <p.camerlynck@televic.com>
>> ---
>>
>> v2:
>> - Address build warning from kernel test robot
>>
>>  sound/soc/fsl/fsl_sai.c | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
>> index af1a168d35e3..d158352c7640 100644
>> --- a/sound/soc/fsl/fsl_sai.c
>> +++ b/sound/soc/fsl/fsl_sai.c
>> @@ -841,6 +841,18 @@ static int fsl_sai_trigger(struct snd_pcm_substream *substream, int cmd,
>>         case SNDRV_PCM_TRIGGER_START:
>>         case SNDRV_PCM_TRIGGER_RESUME:
>>         case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
>> +               /*
>> +                * Force a software reset if we are not the clock provider, as we
>> +                * might have lost frame sync during xrun recovery.
>> +                */
>> +               if (sai->is_consumer_mode[tx]) {
>> +                       regmap_update_bits(sai->regmap,
>> +                                       FSL_SAI_xCSR(tx, ofs), FSL_SAI_CSR_SR,
>> +                                       FSL_SAI_CSR_SR);
>> +                       regmap_update_bits(sai->regmap,
>> +                                       FSL_SAI_xCSR(tx, ofs), FSL_SAI_CSR_SR,
>> +                                       0);
>> +               }
>
> Which platform are you using?  and please add chip info in your commit
> message.

It's an imx8mm. I'll add that to the commit message.

> This change can be moved to fsl_sai_config_disable(). that is:
>
> --- a/sound/soc/fsl/fsl_sai.c
> +++ b/sound/soc/fsl/fsl_sai.c
> @@ -798,18 +798,16 @@ static void fsl_sai_config_disable(struct
> fsl_sai *sai, int dir)
>                            FSL_SAI_CSR_FR, FSL_SAI_CSR_FR);
>
>         /*
> -        * For sai master mode, after several open/close sai,
> +        * For sai master/slave mode, after several open/close sai,
>          * there will be no frame clock, and can't recover
>          * anymore. Add software reset to fix this issue.
>          * This is a hardware bug, and will be fix in the
>          * next sai version.
>          */
> -       if (!sai->is_consumer_mode[tx]) {
> -               /* Software Reset */
> -               regmap_write(sai->regmap, FSL_SAI_xCSR(tx, ofs),
> FSL_SAI_CSR_SR);
> -               /* Clear SR bit to finish the reset */
> -               regmap_write(sai->regmap, FSL_SAI_xCSR(tx, ofs), 0);
> -       }
> +       /* Software Reset */
> +       regmap_write(sai->regmap, FSL_SAI_xCSR(tx, ofs), FSL_SAI_CSR_SR);
> +       /* Clear SR bit to finish the reset */
> +       regmap_write(sai->regmap, FSL_SAI_xCSR(tx, ofs), 0);
>  }
>
> Could you please try the above change to also work for your case?

Ah, I should've spotted that! Yes, doing it this way works just fine. Updated patch incoming.

Thank you,
Arun

Re: [PATCH v2] ASoC: fsl_sai: Force a software reset when starting in consumer mode

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 525 bytes --]

On Wed, Jun 25, 2025 at 07:56:16PM -0400, Arun Raghavan wrote:
> From: Arun Raghavan <arun@asymptotic.io>
> 
> In a setup with an external clock provider, when running the receiver
> (arecord) and triggering an xrun with xrun_injection, we see a channel
> swap/offset. This happens sometimes when running only the receiver, but
> occurs reliably if a transmitter (aplay) is also concurrently running.

Please don't send new patches or versions in reply to old threads, it
makes it harder to follow what's going on.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] ASoC: fsl_sai: Force a software reset when starting in consumer mode

[Arun Raghavan]

On Thu, 26 Jun 2025, at 7:08 AM, Mark Brown wrote:
> On Wed, Jun 25, 2025 at 07:56:16PM -0400, Arun Raghavan wrote:
>> From: Arun Raghavan <arun@asymptotic.io>
>> 
>> In a setup with an external clock provider, when running the receiver
>> (arecord) and triggering an xrun with xrun_injection, we see a channel
>> swap/offset. This happens sometimes when running only the receiver, but
>> occurs reliably if a transmitter (aplay) is also concurrently running.
>
> Please don't send new patches or versions in reply to old threads, it
> makes it harder to follow what's going on.

Ack, sorry about that!

-- Arun