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

On 12/09/2017 23:32, Nicolin Chen wrote:

On Tue, Sep 12, 2017 at 04:35:13PM +0200, Arnaud Mouiche wrote:

quoted

quoted

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

RM says that the word length is fixed to 32 in I2S Master mode.
So I used it here. But I think using it with the slots could be
wrong here as you stated. What kind of clock rates (bit and lr)
does a TDM case have?

The problem of slot width here is handled in the set_tdm_slot()
at all. So I tried to ignored that. But we probably do need it
when calculating things with the slot number.

Could you please give me a few set of examples of how you set
set_sysclk(), set_tdm_slot() with the current driver? The idea
here is to figure out a way to calculate the bclk in hw_params
without getting set_sysclk() involved any more.

Here is one, where a bclk = 4*16*fs is expected

    static int imx_hifi_hw_params(struct snd_pcm_substream *substream,
                          struct snd_pcm_hw_params *params)
    {
         struct snd_soc_pcm_runtime *rtd = substream->private_data;
         struct imx_priv *priv = &card_priv;
         struct device *dev = &priv->pdev->dev;

         int ret = 0;
         struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
         unsigned int freq;
         int ch;

         /* configuring cpu_dai
          * - bitclk  (fclk is computed automatically)
          *      16bit * 4 (max) channels * sampling rate
          * - tdm maskto select the active channels
          */

         freq = 4 * 16 * params_rate(params);
         if (freq != priv->current_freq) {
             /* clk_id and direction are not taken in count by fsl_ssi
    driver */
             ret = snd_soc_dai_set_sysclk( cpu_dai, 0, freq, 0 );
             if (ret) {
                 dev_err(dev, "failed to set cpu dai bitclk: %u\n", freq);
                 return ret;
             }
             priv->current_freq = freq;
         }
         ch = params_channels(params);
         if (ch != priv->current_ch) {
             ret = snd_soc_dai_set_tdm_slot( cpu_dai, (1 << ch)-1, (1 <<
    ch)-1, 4, 16 );
             if (ret) {
                 dev_err(dev, "failed to set cpu dai tdm slots:
    ch=%d\n", ch);
                 return ret;
             }
             priv->current_ch = ch;
         }
         return 0;
    }

In another setup, there are 8 x 16 bits slots, whatever the number of 
active channels is.
In this case bclk = 128 * fs
The number of slots is completely arbitrary. Some slots can even be 
reserved for communication between codecs that don't communicate with linux.

quoted

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)

It's more than a clock_id in my opinion. The driver now sets
the bit clock rate via set_sysclk() other than the MCLK rate
actually.

quoted

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.

Unfortunately, it looks like a work around to me. I understand
the idea of leaving set_sysclk() out there to override the bit
clock is convenient, but it is not a standard ALSA design and
may eventually introduce new problems like today.

I agree. I'm not conservative at all concerning this question.
I don't see a way to remove set_sysclk without breaking current TDM 
users anyway, at least for those who don't have their code upstreamed.


All information provided through snd_soc_dai_set_tdm_slot( cpu_dai, 
mask, mask, slots, width ) should be enough
In this case, for TDM users

    bclk = slots * width * fs   (where slots is != channels)



will manage 99 % of the cases.
And the remaining 1% will concern people who need to hack the kernel so 
widely they don't care about the set_sysclk removal.

Now, looking at the code currently in linus/master:sound/soc/fsl 
concerning TDM
- imx-mc13783.c : the codec is master => OK
- fsl-asoc-card.c : *something will break since snd_soc_dai_set_sysclk 
returned code is checked*
- eukrea-tlv320.c : based on imx-ssi.c, so not affected by changes in 
fsl_ssi.c.  Use set_sysclk() only to setup the clock direction
- wm1133-ev1.c : bclk generated by the codec. set_sysclk() not called 
for cpu_dai

Consequently, we can say that few things is broken in linux upstream if 
set_sysclk is removed, and this can be fixed easily for fsl-asoc-card.c


Arnaud