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