ssp: disable SSP port during ssp_start()

[keyonjie]

We should make sure the SSP port is disabled when trying to start new
transmission or reception, to avoid any potential race between
transmission and the reception on the same SSP port.

Fixes #3520 #3525

Signed-off-by: Keyon Jie yang.jie@linux.intel.com

[lgirdwood]

Can you confirm there are no audio artifacts caused by disabling SSP and then re-enabling during active playback or capture ? I would assume clocks would stop if in master/producer mode ?

[lgirdwood]

@keyonjie looks like there is CI issue on pause and release too ?

[bkokoszx]

@keyonjie
I've performed stop/start tests (10 iterations on each format) and it passes. However, I do not fully understand what kind of potential races we can hit. Could you try to explain more?

[keyonjie]

SOFCI TEST

[lgirdwood]

I'm not a fan of this since it switches OFF the SSP port.
@keyonjie can you try and only change SSCR1_RSRE and SSCR1_TSRE at stream start (for playback or capture). i.e. program SSRSA_RXEN, SSTSA_TXEN and SSCR0_SSE on first stream start and then only SSCR1_RSRE or SSCR1_TSRE based on direction.

[plbossart]

Can you confirm there are no audio artifacts caused by disabling SSP and then re-enabling during active playback or capture ? I would assume clocks would stop if in master/producer mode ?

That seems completely bonkers to me.

[keyonjie]

@plbossart @lgirdwood this is actually referring to pxa-ssp from the kernel written by Mark and Liam here:
https://github.com/thesofproject/linux/blob/37740626abc68e0f11918c86d35fb1526f721e77/sound/soc/pxa/pxa-ssp.c#L699

The switching off (then on) takes place pretty fast (e.g. 5us), but you are right, we do need to check if it will introduce any artifacts on any platforms, I will try it more.

[keyonjie]

I'm not a fan of this since it switches OFF the SSP port.
@keyonjie can you try and only change SSCR1_RSRE and SSCR1_TSRE at stream start (for playback or capture). i.e. program SSRSA_RXEN, SSTSA_TXEN and SSCR0_SSE on first stream start and then only SSCR1_RSRE or SSCR1_TSRE based on direction.

I had tried what you mentioned with all possible configurations that I can figure out without good enough solution to cover both internal/external loopback mode, the tricky thing is that even the SSRSA_RXEN is cleared but the RX FIFO and RNE can keep changing for ever. So to me, to reset the RX FIFO cleanly (while playback keeps active), a fast/rapid toggle of SSCR0_SSE is the only way we have.

[lgirdwood]

So to me, to reset the RX FIFO cleanly (while playback keeps active), a fast/rapid toggle of SSCR0_SSE is the only way we have.

It fails CI - See the internal CI dashboard, it's shows lots of RED in the SSP tests.

[keyonjie]

@lgirdwood @plbossart I do observe artifact on playback when stop/start capture frequently, so let me drop this solution.

@singalsu @jdrzaszcz and all, please be noticed that for internal SSP loopback capture (SSC1.LBM set), the capture channel can swap for different capture iteration, this is a known limitation as the Rx FIFO shifter is connected to the Tx one and the Rx FIFO entry is not guaranteed to be aligned with the channel index. I don't plan to fix it anymore.

For External loopback, we will add more test cases to our CI, I believe we should not hit channel swapped issue on cAVS, though it could happen on BYT/BSW (I will mimic a fix of 242be34 to fix it). @jdrzaszcz if that is not true for you in #3525, please raise.

[lgirdwood]

@keyonjie can you confirm for clarity this only affects the LBM mode and regular audio playback and capture are not affected ?

[plbossart]

@keyonjie agree that for LBM to work we probably need to start capture and playback at the same time, otherwise the phase of the capture will not be deterministic.

I also think external loopback is the only 'safe' solution - though this could lead to other issues on when to sample/hold timing. I vaguely recall we had to play with some obscure bits of the SSP on APL.

[lgirdwood]

@keyonjie agree that for LBM to work we probably need to start capture and playback at the same time, otherwise the phase of the capture will not be deterministic.

Ack - an atomic SSP enable from LBM makes perfect sense i.e. if LBM is true then do atomic SSP start for both directions.

[keyonjie]

Yes, @lgirdwood @plbossart if we intend to fix for the LBM, looks SSP 'restart' solution is inevitable, do you expect for this fix or let's just keep this as a known limitation for LBM?

For external loopback, we still have issues with results from @jdrzaszcz , we are trying to figure out a solution for that.

[lgirdwood]

Yes, @lgirdwood @plbossart if we intend to fix for the LBM, looks SSP 'restart' solution is inevitable, do you expect for this fix or let's just keep this as a known limitation for LBM?

Yes, we need a different start/stop flow for LBM only.

[singalsu]

@keyonjie This improves issue #3520 a lot but doesn't fix it completely. It failed 1 of 5 runs with BYT with internal loopbak used. Without this patch I never got a passed run.

I'll next add external loopback and remove the internal LBM set and update the result here later.

[singalsu]

@keyonjie I added to MB Max the SSP external loopback wire and removed LBM set from topology. However this about 1 or 2 errors per 5 runs seems to remain. Are you able to do this same test?

I'm trying to confirm the LBM is really cleared. I don't understand why I can't trace from ssp.c. I can't see in trace any ssp_set_config messages. Tried dai_info() and dai_err(). Also verbose trace. I'm checking with md5sum that the image that I run is correct and tried add the trace to all ssp.c modules but no luck.

[singalsu]

Anyway this, was my patch for topology that I used in test:

diff --git a/tools/topology/sof-cht-nocodec.m4 b/tools/topology/sof-cht-nocodec.m4
index cfc754f78..b80298d34 100644
--- a/tools/topology/sof-cht-nocodec.m4
+++ b/tools/topology/sof-cht-nocodec.m4
@@ -101,4 +101,4 @@ DAI_CONFIG(SSP, SSP_NUM, SSP_NUM, NoCodec-2,
                      SSP_CLOCK(bclk, 2400000, codec_slave),
                      SSP_CLOCK(fsync, 48000, codec_slave),
                      SSP_TDM(2, 25, 3, 3),
-                     SSP_CONFIG_DATA(SSP, SSP_NUM, 24, 0, SSP_QUIRK_LBM)))
+                     SSP_CONFIG_DATA(SSP, SSP_NUM, 24)))

[keyonjie]

Hi @singalsu I am planning to split the internal LBM and the external one, I am working on the external fixes, will submit PR soon once @jdrzaszcz confirmed it work.

For the internal LBM, I would leave the fix as stretch goal, I have the fix(quite similar to the previous stop/start trigger one) for the channel swap issue on TGL, but definitely it will introduce jitter on playback while performing pause/release on capture stream frequently. If that is acceptable, I can submit it later on.

[keyonjie]

@keyonjie I added to MB Max the SSP external loopback wire and removed LBM set from topology. However this about 1 or 2 errors per 5 runs seems to remain. Are you able to do this same test?

I'm trying to confirm the LBM is really cleared. I don't understand why I can't trace from ssp.c. I can't see in trace any ssp_set_config messages. Tried dai_info() and dai_err(). Also verbose trace. I'm checking with md5sum that the image that I run is correct and tried add the trace to all ssp.c modules but no luck.

Are you seeing other dai_info() outputs? if not, it could be filtered out by the recent added logger filter feature by default?

[keyonjie]

Anyway this, was my patch for topology that I used in test:

diff --git a/tools/topology/sof-cht-nocodec.m4 b/tools/topology/sof-cht-nocodec.m4
index cfc754f78..b80298d34 100644
--- a/tools/topology/sof-cht-nocodec.m4
+++ b/tools/topology/sof-cht-nocodec.m4
@@ -101,4 +101,4 @@ DAI_CONFIG(SSP, SSP_NUM, SSP_NUM, NoCodec-2,
                      SSP_CLOCK(bclk, 2400000, codec_slave),
                      SSP_CLOCK(fsync, 48000, codec_slave),
                      SSP_TDM(2, 25, 3, 3),
-                     SSP_CONFIG_DATA(SSP, SSP_NUM, 24, 0, SSP_QUIRK_LBM)))
+                     SSP_CONFIG_DATA(SSP, SSP_NUM, 24)))

this is correct, but please remember to run your test on SSP2 only with this change applied.

[keyonjie]

@singalsu @lgirdwood @plbossart the PR to fix the external channel swap issue is here:
#3550

It is for cAVS only @singalsu we might need to mimic the fix once it is verified work by @jdrzaszcz .