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(-)
@@ -716,9 +715,9 @@ static int fsl_ssi_set_bclk(struct snd_pcm_substream *substream,unsignedintfreq;boolbaudclk_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);elsefreq=params_channels(hw_params)*32*params_rate(hw_params);
@@ -805,16 +804,6 @@ static int fsl_ssi_set_bclk(struct snd_pcm_substream *substream,return0;}-staticintfsl_ssi_set_dai_sysclk(structsnd_soc_dai*cpu_dai,-intclk_id,unsignedintfreq,intdir)-{-structfsl_ssi_private*ssi_private=snd_soc_dai_get_drvdata(cpu_dai);--ssi_private->bitclk_freq=freq;--return0;-}-/***fsl_ssi_hw_params-programthesamplesize*
On Thu, Sep 07, 2017 at 10:23:43PM -0700, 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>
Forgot to mention, I think that it's better to wait for a couple of
Tested-by from those who use the TDM mode of SSI before applying it.
Thanks
Nicolin
On Thu, Sep 07, 2017 at 10:23:43PM -0700, Nicolin Chen wrote:
quoted
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>
Forgot to mention, I think that it's better to wait for a couple of
Tested-by from those who use the TDM mode of SSI before applying it.
On Thu, Sep 07, 2017 at 10:23:43PM -0700, Nicolin Chen wrote:
quoted
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>
Forgot to mention, I think that it's better to wait for a couple of
Tested-by from those who use the TDM mode of SSI before applying it.
Although, I'm not the PCM user, I've tested your patch and it works :-)
Tested-by: Łukasz Majewski <lukma@denx.de>
Test HW: imx6q + tfa9879 codec
Thanks
Nicolin
--
Best regards,
Lukasz Majewski
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
Hello Nicolin
On 08/09/2017 07:23, Nicolin Chen wrote:
quoted hunk
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(-)
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
quoted hunk
*/
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 *
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.
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.
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.
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)
The idea here is to drop the set_sysclk() for all user cases
including TDM cases. So we need a solid patch to calculate the
bit clock rate in hw_params() for TDM user cases..
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
On Wed, Sep 13, 2017 at 10:02:20AM +0200, Arnaud Mouiche wrote:
quoted
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
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.
In summary, bclk = sample rate * slots * slot_width;
I will update my patch soon.
quoted
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.
Which TDM case would be broken by this removal? The only impact
that I can see is that the ASoC core returns an ENOTSUPP for a
set_sysclk() call now, which is something that a dai-link driver
should have taken care of anyway.
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.
A patch from those people will be always welcome.
- fsl-asoc-card.c : *something will break since
snd_soc_dai_set_sysclk returned code is checked*
I've already submitted a patch to ignore all ENOTSUPP.
On Wed, Sep 13, 2017 at 10:02:20AM +0200, Arnaud Mouiche wrote:
quoted
quoted
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.
quoted
Here is one, where a bclk = 4*16*fs is expected
quoted
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.
In summary, bclk = sample rate * slots * slot_width;
I will update my patch soon.
quoted
quoted
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.
Which TDM case would be broken by this removal? The only impact
that I can see is that the ASoC core returns an ENOTSUPP for a
set_sysclk() call now, which is something that a dai-link driver
should have taken care of anyway.
quoted
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)
quoted
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.
A patch from those people will be always welcome.
quoted
- fsl-asoc-card.c : *something will break since
snd_soc_dai_set_sysclk returned code is checked*
I've already submitted a patch to ignore all ENOTSUPP.
Nicolin, do you know what happened with this patch? I couldn't find it
in current linux/master.
Has it been applied to any asoc tree for being upstreamed?
Best regards,
Lukasz Majewski
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
On Sat, Nov 25, 2017 at 11:29:48PM +0100, Lukasz Majewski wrote:
Nicolin, do you know what happened with this patch? I couldn't find it
in current linux/master.
Has it been applied to any asoc tree for being upstreamed?
On Sat, Nov 25, 2017 at 11:29:48PM +0100, Lukasz Majewski wrote:
quoted
Nicolin, do you know what happened with this patch? I couldn't find
it in current linux/master.
Has it been applied to any asoc tree for being upstreamed?
Thanks for update - I was not aware of it.
Tested-by: Lukasz Majewski <lukma@denx.de>
HW: iMX6Q.
Best regards,
Lukasz Majewski
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de