Re: [PATCH] ASoC: fsl_sai: Clean code for synchronize mode

On Tue, Aug 4, 2020 at 3:04 PM Nicolin Chen [off-list ref] wrote:

On Tue, Aug 04, 2020 at 12:22:53PM +0800, Shengjiu Wang wrote:

quoted

quoted

quoted

Btw, do we need similar change for TRIGGER_STOP?

This is a good question. It is better to do change for STOP,
but I am afraid that there is no much test to guarantee the result.

quoted

Maybe we can do this change for STOP.

The idea looks good to me...(check inline comments)

quoted

diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
index 1c0e06bb3783..6e4be398eaee 100644
--- a/sound/soc/fsl/fsl_sai.c
+++ b/sound/soc/fsl/fsl_sai.c

@@ -517,6 +517,37 @@ static int fsl_sai_hw_free(struct

snd_pcm_substream *substream,
        return 0;
 }

+static void fsl_sai_config_disable(struct fsl_sai *sai, bool tx)
+{
+       unsigned int ofs = sai->soc_data->reg_offset;
+       u32 xcsr, count = 100;
+
+       regmap_update_bits(sai->regmap, FSL_SAI_xCSR(tx, ofs),
+                          FSL_SAI_CSR_TERE, 0);
+
+       /* TERE will remain set till the end of current frame */
+       do {
+               udelay(10);
+               regmap_read(sai->regmap, FSL_SAI_xCSR(tx, ofs), &xcsr);
+       } while (--count && xcsr & FSL_SAI_CSR_TERE);
+
+       regmap_update_bits(sai->regmap, FSL_SAI_xCSR(tx, ofs),
+                          FSL_SAI_CSR_FR, FSL_SAI_CSR_FR);
+
+       /*
+        * For sai master 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_slave_mode) {
+               /* Software Reset for both Tx and Rx */

Remove "for both Tx and Rx"

quoted

                /* Check if the opposite FRDE is also disabled */
                regmap_read(sai->regmap, FSL_SAI_xCSR(!tx, ofs), &xcsr);
+               if (sai->synchronous[tx] && !sai->synchronous[!tx] && !(xcsr & FSL_SAI_CSR_FRDE))
+                       fsl_sai_config_disable(sai, !tx);

quoted

+               if (sai->synchronous[tx] || !sai->synchronous[!tx] || !(xcsr & FSL_SAI_CSR_FRDE))
+                       fsl_sai_config_disable(sai, tx);

The first "||" should probably be "&&".

No. it is !(!sai->synchronous[tx] && sai->synchronous[!tx] && (xcsr &
FSL_SAI_CSR_FRDE))
so then convert to
(sai->synchronous[tx] || !sai->synchronous[!tx] || !(xcsr & FSL_SAI_CSR_FRDE))

if change to &&, then it won't work for:
sai->synchronous[tx] = false, sai->synchronous[!tx]=false.
or
sai->synchronous[tx] = true, sai->synchronous[!tx]=true.
(even we don't have such a case).

The trigger() logic is way more complicated than a simple cleanup
in my opinion. Would suggest to split RMR part out of this change.

And for conditions like "sync[tx] && !sync[!tx]", it'd be better
to have a helper function to improve readability:

/**
 * fsl_sai_dir_is_synced - Check if stream is synced by the opposite stream
 *
 * SAI supports synchronous mode using bit/frame clocks of either Transmitter's
 * or Receiver's for both streams. This function is used to check if clocks of
 * current stream's are synced by the opposite stream.
 *
 * @sai: SAI context
 * @dir: direction of current stream
 */
static inline bool fsl_sai_dir_is_synced(struct fsl_sai *sai, int dir)
{
        int adir = (dir == TX) ? RX : TX;

        /* current dir in async mode while opposite dir in sync mode */
        return !sai->synchronous[dir] && sai->synchronous[adir];
}

Then add more comments in trigger:

static ...trigger()
{
        bool tx = substream->stream == SNDRV_PCM_STREAM_PLAYBACK;
        int adir = tx ? RX : TX;
        int dir = tx ? TX : RX;

        // ....
        {
                // ...

                /* Check if the opposite FRDE is also disabled */
                regmap_read(sai->regmap, FSL_SAI_xCSR(!tx, ofs), &xcsr);

                /*
                 * If opposite stream provides clocks for synchronous mode and
                 * it is inactive, disable it before disabling the current one
                 */
                if (fsl_sai_dir_is_synced(adir) && !(xcsr & FSL_SAI_CSR_FRDE))
                        fsl_sai_config_disable(sai, adir);

                /*
                 * Disable current stream if either of:
                 * 1. current stream doesn't provide clocks for synchronous mode
                 * 2. current stream provides clocks for synchronous mode but no
                 *    more stream is active.
                 */
                if (!fsl_sai_dir_is_synced(dir) || !(xcsr & FSL_SAI_CSR_FRDE))
                        fsl_sai_config_disable(sai, dir);

                // ...
        }
        // ....
}

Note, in fsl_sai_config_disable(sai, dir):
        bool tx = dir == TX;

Above all, I am just drafting, so please test it thoroughly :)