ssp: move SSP blck enablement to ssp_set_config

[plbossart]

The SSP clocks are currently started in the trigger phase.

This is problematic for multiple reasons:

a) we should NEVER change clock sources in a trigger
phase. Switching clocks in and out based on start/stop is asking for
trouble.
b) we should enable the clocks in the prepare phase
c) we are missing a hw_free (dual of prepare) for DAIs. We should stop
the SSP and switch clocks in a hw_free

This patch moves some of the configuration to the prepare phase (which
internally calls ssp_set_config).

Initial results show that we can start the bclk 6+ms before starting
the transfers.

FIXME:

1. the bclk signal is zeroed out at some point. that's not normal
2. the fsync signal should start at the same time as the bclk
   according to the hardware documentation. It's not clear what causes
   the fsync to remain high for ~6ms.
3. there is a ~4.2ms delay between the fsync activation and the actual data
   transfers. This may be due to buffering in the DSP, but that's clearly
   not good at all.

Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com

[plbossart]

Logic pro grab before the changes (same 4.2 ms delay after fsync that looks odd).

Logic Pro grab with this PR:

[plbossart]

This is a WIP to try and see what can be done to address #4156

[plbossart]

To help with the debug of the SSP settings, if I just take the SOF main branch and add a 15ms delay before enabling the DMA, both the SSP blck and fsync start at the same time, and I measure a delay of 19.2 ms (=15 + the constant 4.2ms that seems to come from firmware). This means something is not correct in this PR, or there are multiple configurations of the SSP that add problems. On paper there is zero issue with adding a delay, it's just the firmware stages that make things complicated.

iff --git a/src/drivers/intel/ssp/ssp.c b/src/drivers/intel/ssp/ssp.c
index d03efb4e0..9d116dd5c 100644
--- a/src/drivers/intel/ssp/ssp.c
+++ b/src/drivers/intel/ssp/ssp.c
@@ -765,6 +765,9 @@ static void ssp_start(struct dai *dai, int direction)
                                             ssp->params.bclk_delay));
        }
 
+       wait_delay(clock_ms_to_ticks(PLATFORM_DEFAULT_CLOCK,
+                                    15));
+       
        /* enable DMA */
        if (direction == DAI_DIR_PLAYBACK) {
                ssp_update_bits(dai, SSCR1, SSCR1_TSRE, SSCR1_TSRE);

ssp-delay-in-trigger.diff.txt

[uhs987]

I thought ssp_set_config is called when receiving SOF_IPC_DAI_CONFIG command in the initialization? There is no IPC command sent to DSP during the PREPARE stage.

The SOF_IPC_STREAM_PCM_PARAMS may be a better place to turn on clocks since it's sent in HW_PARAMS stage.

Regards,
Brent

[lgirdwood]

Could it be the SSP port config during hw_params ? or prepare() that's enabling some SSP logic ?? We could try and validate the params only at hw_params() and program them at trigger(start) ?

[plbossart]

@lgirdwood @uhs987 this is super confusing
hw_params is called before ssp_set_config for some reason. the flow I reversed-engineer is component_prepare>ssp_set_config.
But we have no hw_free.

And no we need to enable the clock at the hw_params or prepare level, this is the ask to start the clock BEFORE transfering data.

[lgirdwood]

@lgirdwood @uhs987 this is super confusing
hw_params is called before ssp_set_config for some reason. the flow I reversed-engineer is component_prepare>ssp_set_config.
But we have no hw_free.

hw_free() on ALSA is aligned reset() ordering here.

And no we need to enable the clock at the hw_params or prepare level, this is the ask to start the clock BEFORE transfering data.

I also cant quite remember when ssp_config() is called, it could be during DAI config IPC ? However, the dai ops should support a prepare() callback to enable any clocks if needed prior to start ?>

[plbossart]

@lgirdwood the dai_driver doesn't have a prepare or a reset

struct dai_ops {
	int (*set_config)(struct dai *dai, struct sof_ipc_dai_config *config);
	int (*trigger)(struct dai *dai, int cmd, int direction);
	int (*pm_context_restore)(struct dai *dai);
	int (*pm_context_store)(struct dai *dai);
	int (*get_hw_params)(struct dai *dai,
			     struct sof_ipc_stream_params *params, int dir);
	int (*hw_params)(struct dai *dai, struct sof_ipc_stream_params *params);
	int (*get_handshake)(struct dai *dai, int direction, int stream_id);
	int (*get_fifo)(struct dai *dai, int direction, int stream_id);
	int (*probe)(struct dai *dai);
	int (*remove)(struct dai *dai);

I reverse-engineered that hw_params is called BEFORE set_config. There is no 'unset_config' or hw_free, this is really not so good IMHO.

[plbossart]

replaced by #4288