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

From: Shengjiu Wang
Date: Tue Aug 04 2020 - 00:23:06 EST

On Tue, Aug 4, 2020 at 11:23 AM Shengjiu Wang <shengjiu.wang@xxxxxxxxx> wrote:
>
> On Tue, Aug 4, 2020 at 11:00 AM Nicolin Chen <nicoleotsuka@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxx> 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);
+