ASoC: fsl_sai: Clean code for synchronize mode
diff mbox series

Message ID 1596424674-32127-1-git-send-email-shengjiu.wang@nxp.com
State New
Headers show
Series
  • ASoC: fsl_sai: Clean code for synchronize mode
Related show

Commit Message

Shengjiu Wang Aug. 3, 2020, 3:17 a.m. UTC
TX synchronous with RX: The RMR is no need to be changed when
Tx is enabled, the other configuration in hw_params() is enough for
clock generation. The TCSR.TE is no need to enabled when only RX
is enabled.

RX synchronous with TX: The TMR is no need to be changed when
Rx is enabled, the other configuration in hw_params() is enough for
clock generation. The RCSR.RE is no need to enabled when only TX
is enabled.

Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
---
 sound/soc/fsl/fsl_sai.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

Comments

Nicolin Chen Aug. 3, 2020, 5:40 a.m. UTC | #1
On Mon, Aug 03, 2020 at 11:17:54AM +0800, Shengjiu Wang wrote:
> TX synchronous with RX: The RMR is no need to be changed when
> Tx is enabled, the other configuration in hw_params() is enough for

Probably you should explain why RMR can be removed, like what
it really does so as to make it clear that there's no such a
relationship between RMR and clock generating.

Anyway, this is against the warning comments in the driver:
	/*
	 * For SAI master mode, when Tx(Rx) sync with Rx(Tx) clock, Rx(Tx) will
	 * generate bclk and frame clock for Tx(Rx), we should set RCR4(TCR4),
	 * RCR5(TCR5) and RMR(TMR) for playback(capture), or there will be sync
	 * error.
	 */

So would need to update it.

> 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.

> +		if (!sai->synchronous[TX] && sai->synchronous[RX] && !tx) {
> +			regmap_update_bits(sai->regmap, FSL_SAI_xCSR((!tx), ofs),
> +					   FSL_SAI_CSR_TERE, FSL_SAI_CSR_TERE);
> +		} else if (!sai->synchronous[RX] && sai->synchronous[TX] && tx) {
> +			regmap_update_bits(sai->regmap, FSL_SAI_xCSR((!tx), ofs),
> +					   FSL_SAI_CSR_TERE, FSL_SAI_CSR_TERE);

Two identical regmap_update_bits calls -- both on !tx (RX?)
Shengjiu Wang Aug. 3, 2020, 8:04 a.m. UTC | #2
On Mon, Aug 3, 2020 at 1:42 PM Nicolin Chen <nicoleotsuka@gmail.com> wrote:
>
> On Mon, Aug 03, 2020 at 11:17:54AM +0800, Shengjiu Wang wrote:
> > TX synchronous with RX: The RMR is no need to be changed when
> > Tx is enabled, the other configuration in hw_params() is enough for
>
> Probably you should explain why RMR can be removed, like what
> it really does so as to make it clear that there's no such a
> relationship between RMR and clock generating.
>
> Anyway, this is against the warning comments in the driver:
>         /*
>          * For SAI master mode, when Tx(Rx) sync with Rx(Tx) clock, Rx(Tx) will
>          * generate bclk and frame clock for Tx(Rx), we should set RCR4(TCR4),
>          * RCR5(TCR5) and RMR(TMR) for playback(capture), or there will be sync
>          * error.
>          */
>
> So would need to update it.
>
> > 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.

>
> > +             if (!sai->synchronous[TX] && sai->synchronous[RX] && !tx) {
> > +                     regmap_update_bits(sai->regmap, FSL_SAI_xCSR((!tx), ofs),
> > +                                        FSL_SAI_CSR_TERE, FSL_SAI_CSR_TERE);
> > +             } else if (!sai->synchronous[RX] && sai->synchronous[TX] && tx) {
> > +                     regmap_update_bits(sai->regmap, FSL_SAI_xCSR((!tx), ofs),
> > +                                        FSL_SAI_CSR_TERE, FSL_SAI_CSR_TERE);
>
> Two identical regmap_update_bits calls -- both on !tx (RX?)
The content for regmap_update_bits is the same, but the precondition
is different.
The first one is for tx=false and enable TCSR.TE. (TX generate clock)
The second one is for tx=true and enable RSCR.RE (RX generate clock)

best regards
wang shengjiu
Nicolin Chen Aug. 3, 2020, 9:57 p.m. UTC | #3
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?

> > > +             if (!sai->synchronous[TX] && sai->synchronous[RX] && !tx) {
> > > +                     regmap_update_bits(sai->regmap, FSL_SAI_xCSR((!tx), ofs),
> > > +                                        FSL_SAI_CSR_TERE, FSL_SAI_CSR_TERE);
> > > +             } else if (!sai->synchronous[RX] && sai->synchronous[TX] && tx) {
> > > +                     regmap_update_bits(sai->regmap, FSL_SAI_xCSR((!tx), ofs),
> > > +                                        FSL_SAI_CSR_TERE, FSL_SAI_CSR_TERE);
> >
> > Two identical regmap_update_bits calls -- both on !tx (RX?)
> The content for regmap_update_bits is the same, but the precondition
> is different.
> The first one is for tx=false and enable TCSR.TE. (TX generate clock)
> The second one is for tx=true and enable RSCR.RE (RX generate clock)

Why not merge them?

+		if ((!sai->synchronous[TX] && sai->synchronous[RX] && !tx) ||
+		   ((!sai->synchronous[RX] && sai->synchronous[TX] && tx) {
Shengjiu Wang Aug. 4, 2020, 1:39 a.m. UTC | #4
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.

>
> > > > +             if (!sai->synchronous[TX] && sai->synchronous[RX] && !tx) {
> > > > +                     regmap_update_bits(sai->regmap, FSL_SAI_xCSR((!tx), ofs),
> > > > +                                        FSL_SAI_CSR_TERE, FSL_SAI_CSR_TERE);
> > > > +             } else if (!sai->synchronous[RX] && sai->synchronous[TX] && tx) {
> > > > +                     regmap_update_bits(sai->regmap, FSL_SAI_xCSR((!tx), ofs),
> > > > +                                        FSL_SAI_CSR_TERE, FSL_SAI_CSR_TERE);
> > >
> > > Two identical regmap_update_bits calls -- both on !tx (RX?)
> > The content for regmap_update_bits is the same, but the precondition
> > is different.
> > The first one is for tx=false and enable TCSR.TE. (TX generate clock)
> > The second one is for tx=true and enable RSCR.RE (RX generate clock)
>
> Why not merge them?
>
> +               if ((!sai->synchronous[TX] && sai->synchronous[RX] && !tx) ||
> +                  ((!sai->synchronous[RX] && sai->synchronous[TX] && tx) {

oh, yes, good point!

best regards
wang shengjiu
Nicolin Chen Aug. 4, 2020, 2:11 a.m. UTC | #5
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?
Shengjiu Wang Aug. 4, 2020, 2:35 a.m. UTC | #6
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.

best regards
wang shengjiu
Nicolin Chen Aug. 4, 2020, 3 a.m. UTC | #7
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.
	 */

Btw, do we need similar change for TRIGGER_STOP?
Shengjiu Wang Aug. 4, 2020, 3:23 a.m. UTC | #8
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
Shengjiu Wang Aug. 4, 2020, 4:22 a.m. UTC | #9
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);
+
Nicolin Chen Aug. 4, 2020, 7:03 a.m. UTC | #10
On Tue, Aug 04, 2020 at 12:22:53PM +0800, Shengjiu Wang wrote:

> > > 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.

> Maybe we can do this change for STOP.

The idea looks good to me...(check inline comments)
 
> 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"

>                 /* 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);

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

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

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 :)
Nicolin Chen Aug. 4, 2020, 7:08 a.m. UTC | #11
On Tue, Aug 04, 2020 at 12:03:46AM -0700, Nicolin Chen wrote:
> On Tue, Aug 04, 2020 at 12:22:53PM +0800, Shengjiu Wang wrote:
> 
> > > > 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.
> 
> > Maybe we can do this change for STOP.
> 
> The idea looks good to me...(check inline comments)
>  
> > 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"
> 
> >                 /* 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);
> 
> > +               if (sai->synchronous[tx] || !sai->synchronous[!tx] || !(xcsr & FSL_SAI_CSR_FRDE))
> > +                       fsl_sai_config_disable(sai, tx);
> 
> The first "||" should probably be "&&".
> 
> 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.

Aww..this should be a generic function, so drop "current":

  * or Receiver's for both streams. This function is used to check if clocks of
  * the stream's are synced by the opposite stream.

>  *
>  * @sai: SAI context
>  * @dir: direction of current stream

Ditto:
   * @dir: stream direction

Thanks.
-----
   
>  */
> 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 :)
Shengjiu Wang Aug. 4, 2020, 7:53 a.m. UTC | #12
On Tue, Aug 4, 2020 at 3:04 PM Nicolin Chen <nicoleotsuka@gmail.com> wrote:
>
> On Tue, Aug 04, 2020 at 12:22:53PM +0800, Shengjiu Wang wrote:
>
> > > > 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.
>
> > Maybe we can do this change for STOP.
>
> The idea looks good to me...(check inline comments)
>
> > 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"
>
> >                 /* 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);
>
> > +               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 :)
Nicolin Chen Aug. 4, 2020, 8:13 a.m. UTC | #13
On Tue, Aug 04, 2020 at 03:53:51PM +0800, Shengjiu Wang wrote:

> > >                 /* 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);
> >
> > > +               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.

Ahh..probably should be
if (!(sync[dir] && !sync[adir]) || !frde)

I have a (seemingly) correct version in my sample code.

And...please untangle the logic using the given example -- adding
helper function(s) and comments. And, though the driver does have
places using array[tx] and array[!tx], better not to use any more
boolean type variable as an array index, as it's hard to read.

Patch
diff mbox series

diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
index cdff739924e2..a210c9836a9a 100644
--- a/sound/soc/fsl/fsl_sai.c
+++ b/sound/soc/fsl/fsl_sai.c
@@ -482,8 +482,6 @@  static int fsl_sai_hw_params(struct snd_pcm_substream *substream,
 			regmap_update_bits(sai->regmap, FSL_SAI_TCR5(ofs),
 				FSL_SAI_CR5_WNW_MASK | FSL_SAI_CR5_W0W_MASK |
 				FSL_SAI_CR5_FBT_MASK, val_cr5);
-			regmap_write(sai->regmap, FSL_SAI_TMR,
-				~0UL - ((1 << channels) - 1));
 		} else if (!sai->synchronous[RX] && sai->synchronous[TX] && tx) {
 			regmap_update_bits(sai->regmap, FSL_SAI_RCR4(ofs),
 				FSL_SAI_CR4_SYWD_MASK | FSL_SAI_CR4_FRSZ_MASK,
@@ -491,8 +489,6 @@  static int fsl_sai_hw_params(struct snd_pcm_substream *substream,
 			regmap_update_bits(sai->regmap, FSL_SAI_RCR5(ofs),
 				FSL_SAI_CR5_WNW_MASK | FSL_SAI_CR5_W0W_MASK |
 				FSL_SAI_CR5_FBT_MASK, val_cr5);
-			regmap_write(sai->regmap, FSL_SAI_RMR,
-				~0UL - ((1 << channels) - 1));
 		}
 	}
 
@@ -553,11 +549,18 @@  static int fsl_sai_trigger(struct snd_pcm_substream *substream, int cmd,
 		regmap_update_bits(sai->regmap, FSL_SAI_xCSR(tx, ofs),
 				   FSL_SAI_CSR_FRDE, FSL_SAI_CSR_FRDE);
 
-		regmap_update_bits(sai->regmap, FSL_SAI_RCSR(ofs),
-				   FSL_SAI_CSR_TERE, FSL_SAI_CSR_TERE);
-		regmap_update_bits(sai->regmap, FSL_SAI_TCSR(ofs),
+		regmap_update_bits(sai->regmap, FSL_SAI_xCSR(tx, ofs),
 				   FSL_SAI_CSR_TERE, FSL_SAI_CSR_TERE);
 
+		/* Enable opposite direction when necessarily */
+		if (!sai->synchronous[TX] && sai->synchronous[RX] && !tx) {
+			regmap_update_bits(sai->regmap, FSL_SAI_xCSR((!tx), ofs),
+					   FSL_SAI_CSR_TERE, FSL_SAI_CSR_TERE);
+		} else if (!sai->synchronous[RX] && sai->synchronous[TX] && tx) {
+			regmap_update_bits(sai->regmap, FSL_SAI_xCSR((!tx), ofs),
+					   FSL_SAI_CSR_TERE, FSL_SAI_CSR_TERE);
+		}
+
 		regmap_update_bits(sai->regmap, FSL_SAI_xCSR(tx, ofs),
 				   FSL_SAI_CSR_xIE_MASK, FSL_SAI_FLAGS);
 		break;