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

Shengjiu Wang shengjiu.wang at gmail.com
Tue Aug 4 06:22:53 CEST 2020


On Tue, Aug 4, 2020 at 11:23 AM Shengjiu Wang <shengjiu.wang at gmail.com> wrote:
>
> On Tue, Aug 4, 2020 at 11:00 AM Nicolin Chen <nicoleotsuka at gmail.com> wrote:
> >
> > On Tue, Aug 04, 2020 at 10:35:12AM +0800, Shengjiu Wang wrote:
> > > On Tue, Aug 4, 2020 at 10:11 AM Nicolin Chen <nicoleotsuka at gmail.com> wrote:
> > > >
> > > > On Tue, Aug 04, 2020 at 09:39:44AM +0800, Shengjiu Wang wrote:
> > > > > On Tue, Aug 4, 2020 at 5:57 AM Nicolin Chen <nicoleotsuka at gmail.com> wrote:
> > > > > >
> > > > > > On Mon, Aug 03, 2020 at 04:04:23PM +0800, Shengjiu Wang wrote:
> > > > > >
> > > > > > > > > clock generation. The TCSR.TE is no need to enabled when only RX
> > > > > > > > > is enabled.
> > > > > > > >
> > > > > > > > You are correct if there's only RX running without TX joining.
> > > > > > > > However, that's something we can't guarantee. Then we'd enable
> > > > > > > > TE after RE is enabled, which is against what RM recommends:
> > > > > > > >
> > > > > > > > # From 54.3.3.1 Synchronous mode in IMX6SXRM
> > > > > > > > # If the receiver bit clock and frame sync are to be used by
> > > > > > > > # both the transmitter and receiver, it is recommended that
> > > > > > > > # the receiver is the last enabled and the first disabled.
> > > > > > > >
> > > > > > > > I remember I did this "ugly" design by strictly following what
> > > > > > > > RM says. If hardware team has updated the RM or removed this
> > > > > > > > limitation, please quote in the commit logs.
> > > > > > >
> > > > > > > There is no change in RM and same recommandation.
> > > > > > >
> > > > > > > My change does not violate the RM. The direction which generates
> > > > > > > the clock is still last enabled.
> > > > > >
> > > > > > Using Tx syncing with Rx clock for example,
> > > > > > T1: arecord (non-stop) => set RE
> > > > > > T2: aplay => set TE then RE (but RE is already set at T1)
> > > > > >
> > > > > > Anything that I am missing?
> > > > >
> > > > > This is a good example.
> > > > > We have used this change locally for a long time, so I think it is
> > > > > safe to do this change, a little different with the recommandation.
> > > >
> > > > Any reason for we have to go against the recommendation?
> > >
> > > Previous code will enable TE and RE  together even for asynchronous
> > > mode.
> > > And for recommendation, previous code just consider the RX sync with
> > > TX, but still violates the recommendation for TX sync with RX case.
> > > So at least this new change is some kind of improvement.
> >
> > Okay. Let's change it then. Please make sure to update/remove
> > those old comments in the trigger(). And it's probably better
> > to mention that what we do now is a bit different from RM:
> >         /*
> >          * Enable the opposite direction for synchronous mode
> >          * 1. Tx sync with Rx: only set RE for Rx; set TE & RE for Tx
> >          * 2. Rx sync with Tx: only set TE for Tx; set RE & TE for Rx
> >          *
> >          * RM recommends to enable RE after TE for case 1 and to enable
> >          * TE after RE for case 2, but we here may not always guarantee
> >          * that happens: "arecord 1.wav; aplay 2.wav" in case 1 enables
> >          * TE after RE, which is against what RM recommends but should
> >          * be safe to do, judging by years of testing results.
> >          */
>
> Thank you for the agreement.
>
> >
> > 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.
>
> best regards
> wang shengjiu

Maybe we can do this change for STOP.

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 */
+               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);
+       }
+}
 static int fsl_sai_trigger(struct snd_pcm_substream *substream, int cmd,
                struct snd_soc_dai *cpu_dai)
@@ -525,7 +556,7 @@ static int fsl_sai_trigger(struct
snd_pcm_substream *substream, int cmd,
        unsigned int ofs = sai->soc_data->reg_offset;

        bool tx = substream->stream == SNDRV_PCM_STREAM_PLAYBACK;
-       u32 xcsr, count = 100;
+       u32 xcsr;

        /*
         * Asynchronous mode: Clear SYNC for both Tx and Rx.
@@ -579,43 +610,12 @@ static int fsl_sai_trigger(struct
snd_pcm_substream *substream, int cmd,

                /* Check if the opposite FRDE is also disabled */
                regmap_read(sai->regmap, FSL_SAI_xCSR(!tx, ofs), &xcsr);
-               if (!(xcsr & FSL_SAI_CSR_FRDE)) {
-                       /* Disable both directions and reset their FIFOs */
-                       regmap_update_bits(sai->regmap, FSL_SAI_TCSR(ofs),
-                                          FSL_SAI_CSR_TERE, 0);
-                       regmap_update_bits(sai->regmap, FSL_SAI_RCSR(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_TCSR(ofs),
-                                          FSL_SAI_CSR_FR, FSL_SAI_CSR_FR);
-                       regmap_update_bits(sai->regmap, FSL_SAI_RCSR(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 */
-                               regmap_write(sai->regmap, FSL_SAI_TCSR(ofs),
-                                            FSL_SAI_CSR_SR);
-                               regmap_write(sai->regmap, FSL_SAI_RCSR(ofs),
-                                            FSL_SAI_CSR_SR);
-                               /* Clear SR bit to finish the reset */
-                               regmap_write(sai->regmap, FSL_SAI_TCSR(ofs), 0);
-                               regmap_write(sai->regmap, FSL_SAI_RCSR(ofs), 0);
-                       }
-               }
+               if (sai->synchronous[tx] && !sai->synchronous[!tx] &&
!(xcsr & FSL_SAI_CSR_FRDE))
+                       fsl_sai_config_disable(sai, !tx);
+
+               if (sai->synchronous[tx] || !sai->synchronous[!tx] ||
!(xcsr & FSL_SAI_CSR_FRDE))
+                       fsl_sai_config_disable(sai, tx);
+


More information about the Alsa-devel mailing list