Re: [alsa-devel] [PATCH] ASoC: fsl_ssi: Override bit clock rate based on slot number

Hello Nicolin

On 08/09/2017 07:23, Nicolin Chen wrote:

The set_sysclk() now is used to override the output bit clock rate. But this is not a common way to implement a set_dai_sysclk(). And this creates a problem when a general machine driver (simple-card for example) tries to do set_dai_sysclk() by passing an input clock rate for the baud clock instead of setting the bit clock rate as fsl_ssi driver expected.

So this patch solves this problem by firstly removing set_sysclk() since the hw_params() can calculate the bit clock rate. Secondly, in order not to break those TDM use cases which previously might have been using set_sysclk() to override the bit clock rate, this patch changes the driver to override it based on the slot number.

The patch also removes an obsolete comment of the dir parameter.

Signed-off-by: Nicolin Chen nicoleotsuka@gmail.com

sound/soc/fsl/fsl_ssi.c | 26 ++++++++------------------ 1 file changed, 8 insertions(+), 18 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c index 64598d1..3657c88 100644 --- a/sound/soc/fsl/fsl_ssi.c +++ b/sound/soc/fsl/fsl_ssi.c @@ -197,12 +197,12 @@ struct fsl_ssi_soc_data {

• @use_dma: DMA is used or FIQ with stream filter
• @use_dual_fifo: DMA with support for both FIFOs used
• @fifo_deph: Depth of the SSI FIFOs

• • @slots: number of slots
  • @rxtx_reg_val: Specific register settings for receive/transmit configuration
  • @clk: SSI clock
  • @baudclk: SSI baud clock for master mode
  • @baudclk_streams: Active streams that are using baudclk

• • @bitclk_freq: bitclock frequency set by .set_dai_sysclk
  • @dma_params_tx: DMA transmit parameters
  • @dma_params_rx: DMA receive parameters

@@ -233,12 +233,12 @@ struct fsl_ssi_private { bool use_dual_fifo; bool has_ipg_clk_name; unsigned int fifo_depth;

• unsigned int slots; struct fsl_ssi_rxtx_reg_val rxtx_reg_val;

  struct clk *clk; struct clk *baudclk; unsigned int baudclk_streams;

• unsigned int bitclk_freq;

  /* regcache for volatile regs */ u32 regcache_sfcsr;

@@ -700,8 +700,7 @@ static void fsl_ssi_shutdown(struct snd_pcm_substream *substream,

• Note: This function can be only called when using SSI as DAI master
• Quick instruction for parameters:

• • freq: Output BCLK frequency = samplerate * 32 (fixed) * channels
• • dir: SND_SOC_CLOCK_OUT -> TxBCLK, SND_SOC_CLOCK_IN -> RxBCLK.

• • freq: Output BCLK frequency = samplerate * 32 (fixed) * slots (or channels)

Slots are not necessarily 32 bits width. Indeed, a simple grep of snd_soc_dai_set_tdm_slot shows 16, 24, 32 and 0 bits usage. (don't know the real meaning of 0 BTW...) So, it should be good to also remember the slots width given in snd_soc_dai_set_tdm_slot() call.

Anyway, there is something wrong in the snd_soc_codec_set_sysclk usage by fsl_ssi Let's get a look again at the description:

/** * snd_soc_dai_set_sysclk - configure DAI system or master clock. * @dai: DAI * @clk_id: DAI specific clock ID * @freq: new clock frequency in Hz * @dir: new clock direction - input/output. * * Configures the DAI master (MCLK) or system (SYSCLK) clocking. */ int snd_soc_dai_set_sysclk(struct snd_soc_dai *dai, int clk_id, unsigned int freq, int dir)

So, it can be used to configure 2 different clocks (and more) with different usages.

Lukasz complains that simple-card is configuring MCLK incorrectly. but simple-card, only want to configure the SYSCLK, whereas fsl_ssi understand "configure the MCLK..." (fsl_ssi doesn't check the clk_id at all)

I think the problem is here. I would propose a new clk_id

#define FSL_SSI_SYSCLK_MCLK 1

And leave fsl_ssi_set_dai_sysclk(xxx, FSL_SSI_SYSCLK_MCLK, ...) override the computed mlck. This will leave a way for obscure TDM users to specify a specific a random mclk frequency for obscure reasons... Unfortunately, such generic clock_id (sysclk, mclk) were never defined widely.

Is it really needed ? It is true that, up to now, we are using fsl_ssi_set_dai_sysclk() in addition to snd_soc_dai_set_tdm_slot() only because this was the only way to deal with correct mclk setting. And if now, snd_soc_dai_set_tdm_slot() do its job correctly, fsl_ssi_set_dai_sysclk() will become useless (except for obscure TDM users of course)

Will it break TDM users ? I will say yes, surely, but TDM users (at least myself) may consider something will break in TDM audio area every-time we jump to a new linux release...

Arnaud

*/ static int fsl_ssi_set_bclk(struct snd_pcm_substream *substream, struct snd_soc_dai *cpu_dai, @@ -716,9 +715,9 @@ static int fsl_ssi_set_bclk(struct snd_pcm_substream *substream, unsigned int freq; bool baudclk_is_used;

• /* Prefer the explicitly set bitclock frequency */
• if (ssi_private->bitclk_freq)

• freq = ssi_private->bitclk_freq;
  

• /* Generate bit clock based on the slot or channel number */
• if (ssi_private->slots)

• freq = ssi_private->slots * 32 * params_rate(hw_params);
  

  else freq = params_channels(hw_params) * 32 * params_rate(hw_params);

@@ -805,16 +804,6 @@ static int fsl_ssi_set_bclk(struct snd_pcm_substream *substream, return 0; }

-static int fsl_ssi_set_dai_sysclk(struct snd_soc_dai *cpu_dai,

• int clk_id, unsigned int freq, int dir)
  

-{

• struct fsl_ssi_private *ssi_private = snd_soc_dai_get_drvdata(cpu_dai);
• ssi_private->bitclk_freq = freq;
• return 0;

-}

• /**
  • fsl_ssi_hw_params - program the sample size

@@ -1121,6 +1110,8 @@ static int fsl_ssi_set_dai_tdm_slot(struct snd_soc_dai *cpu_dai, u32 tx_mask,

regmap_update_bits(regs, CCSR_SSI_SCR, CCSR_SSI_SCR_SSIEN, val);

• ssi_private->slots = slots;
• return 0; }

@@ -1191,7 +1182,6 @@ static const struct snd_soc_dai_ops fsl_ssi_dai_ops = { .hw_params = fsl_ssi_hw_params, .hw_free = fsl_ssi_hw_free, .set_fmt = fsl_ssi_set_dai_fmt,

• .set_sysclk = fsl_ssi_set_dai_sysclk, .set_tdm_slot = fsl_ssi_set_dai_tdm_slot, .trigger = fsl_ssi_trigger, };