[alsa-devel] [PATCH] sound: soc: fsl: Do not set DAI sysclk when it is equal to system freq
The problem is visible in the following setup (on the imx6q): "simple-audio-card" -> ssi2 -> I2S + I2C -> codec
The function call log (simple-card probe -> CONFIG_SND_SIMPLE_CARD):
asoc_simple_card_init_dai() @ sound/soc/generic/simple-card-utils.c snd_soc_dai_set_sysclk() fsl_ssi_set_dai_sysclk() @ sound/soc/fsl/fsl_ssi.c
The last call is changing the bit clock (BCLK) frequency to SSI's IP block clock (ipg = 66 MHz) [1]. This is wrong, since IMX SSI block requires the I2S BCLK to be less than 1/5 of [1].
As a result the driver initialization passes without any errors, but the speaker-test test case breaks.
This commit checks if the fsl_ssi_set_dai_sysclk() frequency passed is not equal to [1].
Signed-off-by: Lukasz Majewski lukma@denx.de --- sound/soc/fsl/fsl_ssi.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c index 173cb84..1186fa9 100644 --- a/sound/soc/fsl/fsl_ssi.c +++ b/sound/soc/fsl/fsl_ssi.c @@ -809,6 +809,8 @@ 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); + if (clk_get_rate(ssi_private->clk) == freq) + return 0;
ssi_private->bitclk_freq = freq;
[Sorry for the top-posting]
The driver currently has:
/* * Hardware limitation: The bclk rate must be * never greater than 1/5 IPG clock rate */ if (freq * 5 > clk_get_rate(ssi_private->clk)) { dev_err(cpu_dai->dev, "bitclk > ipgclk/5\n"); return -EINVAL; }
Isn't this properly taking care of the clock restriction?
________________________________ From: Lukasz Majewski lukma@denx.de Sent: Sunday, September 3, 2017 8:05:01 AM To: Timur Tabi; Nicolin Chen; Xiubo Li; Fabio Estevam; Liam Girdwood; Mark Brown; Jaroslav Kysela; Takashi Iwai Cc: alsa-devel@alsa-project.org; linuxppc-dev@lists.ozlabs.org; linux-kernel@vger.kernel.org; Lukasz Majewski Subject: [PATCH] sound: soc: fsl: Do not set DAI sysclk when it is equal to system freq
The problem is visible in the following setup (on the imx6q): "simple-audio-card" -> ssi2 -> I2S + I2C -> codec
The function call log (simple-card probe -> CONFIG_SND_SIMPLE_CARD):
asoc_simple_card_init_dai() @ sound/soc/generic/simple-card-utils.c snd_soc_dai_set_sysclk() fsl_ssi_set_dai_sysclk() @ sound/soc/fsl/fsl_ssi.c
The last call is changing the bit clock (BCLK) frequency to SSI's IP block clock (ipg = 66 MHz) [1]. This is wrong, since IMX SSI block requires the I2S BCLK to be less than 1/5 of [1].
As a result the driver initialization passes without any errors, but the speaker-test test case breaks.
This commit checks if the fsl_ssi_set_dai_sysclk() frequency passed is not equal to [1].
Signed-off-by: Lukasz Majewski lukma@denx.de --- sound/soc/fsl/fsl_ssi.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c index 173cb84..1186fa9 100644 --- a/sound/soc/fsl/fsl_ssi.c +++ b/sound/soc/fsl/fsl_ssi.c @@ -809,6 +809,8 @@ 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); + if (clk_get_rate(ssi_private->clk) == freq) + return 0;
ssi_private->bitclk_freq = freq;
-- 2.1.4
Hi Fabio,
[Sorry for the top-posting]
The driver currently has:
/*
- Hardware limitation: The bclk rate must be
- never greater than 1/5 IPG clock rate
*/ if (freq * 5 > clk_get_rate(ssi_private->clk)) { dev_err(cpu_dai->dev, "bitclk > ipgclk/5\n"); return -EINVAL; }
Unfortunately not.
This is the part of fsl_ssi_set_bclk() function which is called after fsl_ssi_set_dai_sysclk() (which sets ssi_private->bitclk_freq = freq;).
Before the aforementioned check we do have:
if (ssi_private->bitclk_freq) freq = ssi_private->bitclk_freq; else freq = params_channels(hw_params) * 32 * params_rate(hw_params);
Which assigns freq = bitclk_freq (66 MHz)
And then we break on this particular check:
66MHz * 5 > 66 MHz.
The culprit IMHO is the ssi_private->bitclk_freq = freq; in the fsl_ssi_set_dai_sysclk(), since we _should_ set SSI's IP block clock (ssi_private->clk), not the bit clock (BCLK).
This patch just quits early if it detects change, which don't need to be done.
Isn't this properly taking care of the clock restriction?
From: Lukasz Majewski lukma@denx.de Sent: Sunday, September 3, 2017 8:05:01 AM To: Timur Tabi; Nicolin Chen; Xiubo Li; Fabio Estevam; Liam Girdwood; Mark Brown; Jaroslav Kysela; Takashi Iwai Cc: alsa-devel@alsa-project.org; linuxppc-dev@lists.ozlabs.org; linux-kernel@vger.kernel.org; Lukasz Majewski Subject: [PATCH] sound: soc: fsl: Do not set DAI sysclk when it is equal to system freq
The problem is visible in the following setup (on the imx6q): "simple-audio-card" -> ssi2 -> I2S + I2C -> codec
The function call log (simple-card probe -> CONFIG_SND_SIMPLE_CARD):
asoc_simple_card_init_dai() @ sound/soc/generic/simple-card-utils.c snd_soc_dai_set_sysclk() fsl_ssi_set_dai_sysclk() @ sound/soc/fsl/fsl_ssi.c
The last call is changing the bit clock (BCLK) frequency to SSI's IP block clock (ipg = 66 MHz) [1]. This is wrong, since IMX SSI block requires the I2S BCLK to be less than 1/5 of [1].
As a result the driver initialization passes without any errors, but the speaker-test test case breaks.
This commit checks if the fsl_ssi_set_dai_sysclk() frequency passed is not equal to [1].
Signed-off-by: Lukasz Majewski lukma@denx.de
sound/soc/fsl/fsl_ssi.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c index 173cb84..1186fa9 100644 --- a/sound/soc/fsl/fsl_ssi.c +++ b/sound/soc/fsl/fsl_ssi.c @@ -809,6 +809,8 @@ 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);
if (clk_get_rate(ssi_private->clk) == freq)return 0; ssi_private->bitclk_freq = freq;-- 2.1.4
On Sun, Sep 3, 2017 at 11:40 AM, Łukasz Majewski lukma@denx.de wrote:
This is the part of fsl_ssi_set_bclk() function which is called after fsl_ssi_set_dai_sysclk() (which sets ssi_private->bitclk_freq = freq;).
Before the aforementioned check we do have:
if (ssi_private->bitclk_freq) freq = ssi_private->bitclk_freq; else freq = params_channels(hw_params) * 32 *params_rate(hw_params);
Which assigns freq = bitclk_freq (66 MHz)
And then we break on this particular check:
66MHz * 5 > 66 MHz.
The culprit IMHO is the ssi_private->bitclk_freq = freq; in the fsl_ssi_set_dai_sysclk(), since we _should_ set SSI's IP block clock (ssi_private->clk), not the bit clock (BCLK).
This patch just quits early if it detects change, which don't need to be done.
Thanks for the clarification.
Reviewed-by: Fabio Estevam fabio.estevam@nxp.com
On Sun, Sep 03, 2017 at 04:40:21PM +0200, Łukasz Majewski wrote:
/*
- Hardware limitation: The bclk rate must be
- never greater than 1/5 IPG clock rate
*/ if (freq * 5 > clk_get_rate(ssi_private->clk)) { dev_err(cpu_dai->dev, "bitclk > ipgclk/5\n"); return -EINVAL; }
Unfortunately not.
This is the part of fsl_ssi_set_bclk() function which is called after fsl_ssi_set_dai_sysclk() (which sets ssi_private->bitclk_freq = freq;).
Before the aforementioned check we do have:
if (ssi_private->bitclk_freq) freq = ssi_private->bitclk_freq; else freq = params_channels(hw_params) * 32 * params_rate(hw_params);
Which assigns freq = bitclk_freq (66 MHz)
[...]
And then we break on this particular check: 66MHz * 5 > 66 MHz.
[...]
Does the check fail and return an error here, or not?
The culprit IMHO is the ssi_private->bitclk_freq = freq; in the fsl_ssi_set_dai_sysclk(), since we _should_ set SSI's IP block clock (ssi_private->clk), not the bit clock (BCLK).
No. We should not set the IP block clock. That's from IPG bus on certain IMX SoCs. Setting it might change IPG bus clocks which might break the system.
And apparently, we shouldn't set bitclk to 66MHz either. Can you help to find where this 66MHz comes from?
This patch just quits early if it detects change, which don't need to be done.
I kinda see your point is to error out before hw_params(). So I feel it would be better to have a similar 5-times-check in the set_sysclk() too, if it's really necessary.
Btw, I don't see simple card calling set_sysclk() function in any earlier stage than hw_params(). I am wondering how did you encounter the problem?
Thanks Nicolin
On 09/05/2017 07:20 AM, Nicolin Chen wrote:
On Sun, Sep 03, 2017 at 04:40:21PM +0200, Łukasz Majewski wrote:
/*
- Hardware limitation: The bclk rate must be
- never greater than 1/5 IPG clock rate
*/ if (freq * 5 > clk_get_rate(ssi_private->clk)) { dev_err(cpu_dai->dev, "bitclk > ipgclk/5\n"); return -EINVAL; }
Unfortunately not.
This is the part of fsl_ssi_set_bclk() function which is called after fsl_ssi_set_dai_sysclk() (which sets ssi_private->bitclk_freq = freq;).
Before the aforementioned check we do have:
if (ssi_private->bitclk_freq) freq = ssi_private->bitclk_freq; else freq = params_channels(hw_params) * 32 * params_rate(hw_params);
Which assigns freq = bitclk_freq (66 MHz)
[...]
And then we break on this particular check: 66MHz * 5 > 66 MHz.
[...]
Does the check fail and return an error here, or not?
Yes, this check fails and return error (-EINVAL).
The culprit IMHO is the ssi_private->bitclk_freq = freq; in the fsl_ssi_set_dai_sysclk(), since we _should_ set SSI's IP block clock (ssi_private->clk), not the bit clock (BCLK).
No. We should not set the IP block clock. That's from IPG bus on certain IMX SoCs. Setting it might change IPG bus clocks which might break the system.
And apparently, we shouldn't set bitclk to 66MHz either. Can you help to find where this 66MHz comes from?
OK.
The fsi_ssi.c file defines:
ops->set_sysclk() routine:
.set_sysclk = fsl_ssi_set_dai_sysclk,
This routine is called in: int snd_soc_dai_set_sysclk() @ soc-core.c
which is called in two places for my setup:
1. asoc_simple_card_hw_params() @ simple-card.c
and
2. int asoc_simple_card_init_dai() @ simple-card-utils.c
In this function (point 2.) the
simple_dai->sysclk is set and:
snd_soc_dai_set_sysclk(dai, 0, simple_dai->sysclk, 0)
which sets frequency to 66 MHz [*].
The asoc_simple_card_init_dai() is called in
asoc_simple_card_dai_init() @ simple-card.c
which is assigned to dai_link->init
dai_link->init = asoc_simple_card_dai_init; @ simple_card.c
And the sysclk itself is defined at: -------------------------------------
dai_props->codec_dai->sysclk, which is used at:
asoc_simple_card_startup(), asoc_simple_card_shutdown() and others functions at simple-card.c
It is setup at:
asoc_simple_card_parse_clk() @ simple-card-utils.c from macro: #define asoc_simple_card_parse_clk_cpu()
And the problem is: -------------------
At the asoc_simple_card_parse_clk() we finally go to dts node:
/soc/aips-bus@02100000/i2c@021a0000/tfa9879@6C
which has clock from I2C (66 MHz).
(So the [*] hack may help here, since it is checked before checking the i2c node).
Note: [*] - I could workaround this problem by setting:
system-clock-frequency = <0> in
dailink_master: cpu { sound-dai = <&ssi2>; };
but this is IMHO even worse hack.... than this patch.
This patch just quits early if it detects change, which don't need to be done.
I kinda see your point is to error out before hw_params(). So I feel it would be better to have a similar 5-times-check in the set_sysclk() too, if it's really necessary.
Btw, I don't see simple card calling set_sysclk() function in any earlier stage than hw_params(). I am wondering how did you encounter the problem?
Thanks Nicolin
On Tue, Sep 05, 2017 at 10:35:34AM +0200, Łukasz Majewski wrote:
And apparently, we shouldn't set bitclk to 66MHz either. Can you help to find where this 66MHz comes from?
- int asoc_simple_card_init_dai() @ simple-card-utils.c
Oh, I just searched in the simple-card.c but missed this file.
In this function (point 2.) the simple_dai->sysclk is set and: snd_soc_dai_set_sysclk(dai, 0, simple_dai->sysclk, 0) which sets frequency to 66 MHz [*].
The asoc_simple_card_init_dai() is called in asoc_simple_card_dai_init() @ simple-card.c which is assigned to dai_link->init dai_link->init = asoc_simple_card_dai_init; @ simple_card.c
And the sysclk itself is defined at:
dai_props->codec_dai->sysclk, which is used at:a
Why codec_dai? Why not dai_props->cpu_dai->sysclk since we are talking about SSI?
asoc_simple_card_startup(), asoc_simple_card_shutdown() and others functions at simple-card.c It is setup at: asoc_simple_card_parse_clk() @ simple-card-utils.c from macro: #define asoc_simple_card_parse_clk_cpu() And the problem is:
At the asoc_simple_card_parse_clk() we finally go to dts node: /soc/aips-bus@02100000/i2c@021a0000/tfa9879@6C
This tfa9879 should be the CODEC right?
which has clock from I2C (66 MHz).
You mean I2C scl or I2S sclk?
-----------------------------------------------------------------
But anyway, I feel very confused here as you have 66MHz clock rate (regardless of it purpose) for a codec dai but it's been passed to a cpu dai (SSI).
[*] - I could workaround this problem by setting:
system-clock-frequency = <0> in
dailink_master: cpu { sound-dai = <&ssi2>; };but this is IMHO even worse hack.... than this patch.
I haven't used simple-card for a while so I forgot how to define its DT bindings specifically. But you should assign ssi2 as the CPU dai and assign tfa9879 as a CODEC dai.
Hi Nicolin,
On Tue, Sep 05, 2017 at 10:35:34AM +0200, Łukasz Majewski wrote:
And apparently, we shouldn't set bitclk to 66MHz either. Can you help to find where this 66MHz comes from?
- int asoc_simple_card_init_dai() @ simple-card-utils.c
Oh, I just searched in the simple-card.c but missed this file.
In this function (point 2.) the simple_dai->sysclk is set and: snd_soc_dai_set_sysclk(dai, 0, simple_dai->sysclk, 0) which sets frequency to 66 MHz [*].
The asoc_simple_card_init_dai() is called in asoc_simple_card_dai_init() @ simple-card.c which is assigned to dai_link->init dai_link->init = asoc_simple_card_dai_init; @ simple_card.c
And the sysclk itself is defined at:
dai_props->codec_dai->sysclk, which is used at:a
Why codec_dai? Why not dai_props->cpu_dai->sysclk since we are talking about SSI?
This is how the simple-card (simple-sound-card) is written.
asoc_simple_card_startup(), asoc_simple_card_shutdown() and others functions at simple-card.c It is setup at: asoc_simple_card_parse_clk() @ simple-card-utils.c from macro: #define asoc_simple_card_parse_clk_cpu() And the problem is:
At the asoc_simple_card_parse_clk() we finally go to dts node: /soc/aips-bus@02100000/i2c@021a0000/tfa9879@6C
This tfa9879 should be the CODEC right?
Yes. The tfa9879 is a codec (very simple -> I2S + I2C, mono).
They key point here is the asoc_simple_card_parse_clk() function from simple-card-utils.c
Please look how the clock is assigned; It first checks for cpu clock, then for "system-clock-frequency" DTS node and _finally_ looks for another "child" clock [1], which is the codec attached to I2C.
And from there it takes the 66 MHz CLK: /soc/aips-bus@02100000/i2c@021a0000/tfa9879@6C
which has clock from I2C (66 MHz).
You mean I2C scl or I2S sclk?
I2C scl.
But anyway, I feel very confused here as you have 66MHz clock rate (regardless of it purpose) for a codec dai but it's been passed to a cpu dai (SSI).
Please look into asoc_simple_card_parse_clk().
My DTS [1] (it is different than other in-tree supported codecs - at least I did not find similar setup in DTSes):
sound { compatible = "simple-audio-card"; label = "tfa9879-mono";
simple-audio-card,dai-link { /* DAC */ format = "i2s"; bitclock-master = <&dailink_master>; frame-master = <&dailink_master>;
dailink_master: cpu { sound-dai = <&ssi2>; }; codec { sound-dai = <&codec>; }; }; };
&i2c1 { clock-frequency = <400000>; pinctrl-names = "default"; pinctrl-0 = <&pinctrl_i2c1>; status = "okay";
codec: tfa9879@6C { #sound-dai-cells = <0>; compatible = "tfa9879"; reg = <0x6C>; }; };
&ssi2 { fsl,mode = "i2s-master"; status = "okay"; };
the ssi2 node is defined in imx6qdl.dtsi file (no changes).
The SOC is IMX6Q.
The TFA9879 is a slave for I2S transmission.
[*] - I could workaround this problem by setting:
system-clock-frequency = <0> in
dailink_master: cpu { sound-dai = <&ssi2>; };but this is IMHO even worse hack.... than this patch.
I haven't used simple-card for a while so I forgot how to define its DT bindings specifically. But you should assign ssi2 as the CPU dai and assign tfa9879 as a CODEC dai.
On Tue, Sep 05, 2017 at 11:13:40PM +0200, Łukasz Majewski wrote:
They key point here is the asoc_simple_card_parse_clk() function from simple-card-utils.c
Please look how the clock is assigned; It first checks for cpu clock, then for "system-clock-frequency" DTS node and _finally_ looks for another "child" clock [1], which is the codec attached to I2C.
Here is the routine that I understood from the code: 1) asoc_simple_card_parse_clk_cpu(dev, cpu, dai_link, cpu_dai); => asoc_simple_card_parse_clk(dev, cpu, // cpu node in sound{} [1] dai_link->cpu_of_node, // node ssi2 [2] cpu_dai, dai_link->cpu_dai_name); ==> 1.1) devm_get_clk_from_child(dev, node, NULL); // [1] ==> 1.2) of_property_read_u32(node, "system-clock-frequency", &val)// [1] ==> 1.3) devm_get_clk_from_child(dev, dai_of_node, NULL); // [2]
2) asoc_simple_card_parse_clk_codec(dev, codec, dai_link, codec_dai); => asoc_simple_card_parse_clk(dev, codec, // codec node in sound{} [3] dai_link->codec_of_node,// node tfa9879 [4] codec_dai, dai_link->codec_dai_name); ==> 2.1) devm_get_clk_from_child(dev, node, NULL); // [3] ==> 2.2) of_property_read_u32(node, "system-clock-frequency", &val)// [3] ==> 2.3) devm_get_clk_from_child(dev, dai_of_node, NULL); // [4]
For the cpu routine, it first checks for clock property under cpu node of simple-card, then for "system-clock-frequency" in the cpu node of simple-card, and finally looks for clock property in ssi2 node.
For codec routine, it first checks for clock property under codec node of simple-card, then for "system-clock-frequency" in codec node of simple-card, and finally looks for clock property in the tfa9879 node.
The cpu and codec are totally separate, aren't they? And I don't think it's right if not.
which has clock from I2C (66 MHz).
You mean I2C scl or I2S sclk?
I2C scl.
A couple of things here..... 1) I don't think I2C scl can run at 66MHz...The 66MHz is probably the ipg clock of I2C controller. 2) Even if the scl does run at 66MHz, there is no point in passing the clock of I2C scl into an I2S dai-link.
So I feel something must be wrong. I suggest you to add some printk to check the names of those nodes and x_of_node in the simple card driver.
My DTS [1] (it is different than other in-tree supported codecs - at least I did not find similar setup in DTSes):
sound { compatible = "simple-audio-card"; label = "tfa9879-mono";
simple-audio-card,dai-link { /* DAC */ format = "i2s"; bitclock-master = <&dailink_master>; frame-master = <&dailink_master>; dailink_master: cpu { sound-dai = <&ssi2>; }; codec { sound-dai = <&codec>; }; };
I remember there is another style for a single dai-link. Could you please try that one? There is an example in: Documentation/devicetree/bindings/sound/simple-card.txt
Hi Nicolin,
On Tue, Sep 05, 2017 at 11:13:40PM +0200, Łukasz Majewski wrote:
They key point here is the asoc_simple_card_parse_clk() function from simple-card-utils.c
Please look how the clock is assigned; It first checks for cpu clock, then for "system-clock-frequency" DTS node and _finally_ looks for another "child" clock [1], which is the codec attached to I2C.
Here is the routine that I understood from the code:
asoc_simple_card_parse_clk_cpu(dev, cpu, dai_link, cpu_dai); => asoc_simple_card_parse_clk(dev, cpu, // cpu node in sound{} [1] dai_link->cpu_of_node, // node ssi2 [2] cpu_dai, dai_link->cpu_dai_name); ==> 1.1) devm_get_clk_from_child(dev, node, NULL); // [1] ==> 1.2) of_property_read_u32(node, "system-clock-frequency", &val)// [1] ==> 1.3) devm_get_clk_from_child(dev, dai_of_node, NULL); // [2]
asoc_simple_card_parse_clk_codec(dev, codec, dai_link, codec_dai); => asoc_simple_card_parse_clk(dev, codec, // codec node in sound{} [3] dai_link->codec_of_node,// node tfa9879 [4] codec_dai, dai_link->codec_dai_name); ==> 2.1) devm_get_clk_from_child(dev, node, NULL); // [3] ==> 2.2) of_property_read_u32(node, "system-clock-frequency", &val)// [3] ==> 2.3) devm_get_clk_from_child(dev, dai_of_node, NULL); // [4]
For the cpu routine, it first checks for clock property under cpu node of simple-card, then for "system-clock-frequency" in the cpu node of simple-card, and finally looks for clock property in ssi2 node.
I've double checked this and the simple_dai->sysclk is assigned in the
asoc_simple_card_parse_clk()
-----> dev: sound -----> clk node: /soc/aips-bus@02000000/spba-bus@02000000/ssi@0202c000 -----> Clk asignment
And this clock is taken from this node. It looks like ipg clock for ssi...
For codec routine, it first checks for clock property under codec node of simple-card, then for "system-clock-frequency" in codec node of simple-card, and finally looks for clock property in the tfa9879 node.
The cpu and codec are totally separate, aren't they? And I don't think it's right if not.
Yes, they should be separated.
which has clock from I2C (66 MHz).
You mean I2C scl or I2S sclk?
I2C scl.
A couple of things here.....
- I don't think I2C scl can run at 66MHz...The 66MHz is probably the ipg clock of I2C controller.
I agree. I2C scl runs with 400 kHz (as recommended by I2C high speed spec).
And considering above, the 66 MHz clock seems to be from ssi IP block.
- Even if the scl does run at 66MHz, there is no point in passing the clock of I2C scl into an I2S dai-link.
Yes.
So I feel something must be wrong. I suggest you to add some printk to check the names of those nodes and x_of_node in the simple card driver.
The problem is with the "lack" of clock nodes/properties at
dailink_master: cpu { sound-dai = <&ssi2>; clock = <&SSSS>; system-clock-frequency = <XXXX>; };
and as a result we take some "default" clock.
My DTS [1] (it is different than other in-tree supported codecs - at least I did not find similar setup in DTSes):
sound { compatible = "simple-audio-card"; label = "tfa9879-mono";
simple-audio-card,dai-link { /* DAC */ format = "i2s"; bitclock-master = <&dailink_master>; frame-master = <&dailink_master>; dailink_master: cpu { sound-dai = <&ssi2>; }; codec { sound-dai = <&codec>; }; };I remember there is another style for a single dai-link. Could you please try that one? There is an example in: Documentation/devicetree/bindings/sound/simple-card.txt
Yes, the
simple-audio-card,cpu simple-audio-card,format, ....
etc.
I've omitted it since in the code it is regarded as "old": (simple-card.c line 384).
I think that the proper solution would be to add check for:
freq < sysclk/5 in fsl_ssi_set_dai_sysclk() and return -ENOTSUPP to make the simple-audo-card driver happy (and not introducing regressions).
On Wed, Sep 06, 2017 at 11:22:48AM +0200, Łukasz Majewski wrote:
Here is the routine that I understood from the code:
- asoc_simple_card_parse_clk_cpu(dev, cpu, dai_link, cpu_dai); => asoc_simple_card_parse_clk(dev, cpu, // cpu node in sound{} [1] dai_link->cpu_of_node, // node ssi2 [2] cpu_dai, dai_link->cpu_dai_name); ==> 1.1) devm_get_clk_from_child(dev, node, NULL); // [1] ==> 1.2) of_property_read_u32(node, "system-clock-frequency", &val)// [1] ==> 1.3) devm_get_clk_from_child(dev, dai_of_node, NULL); // [2]
For the cpu routine, it first checks for clock property under cpu node of simple-card, then for "system-clock-frequency" in the cpu node of simple-card, and finally looks for clock property in ssi2 node.
-----> dev: sound -----> clk node: /soc/aips-bus@02000000/spba-bus@02000000/ssi@0202c000 -----> Clk asignment
And this clock is taken from this node. It looks like ipg clock for ssi...
This makes sense now. The devm_get_clk_from_child() in 1.3) fetched the first clock of ssi2 -- ipg clock.
The problem is with the "lack" of clock nodes/properties at
dailink_master: cpu { sound-dai = <&ssi2>; clock = <&SSSS>; system-clock-frequency = <XXXX>; };
This is the right solution based on current simple-card driver. For SSI (having two clocks), you have to specify the baud clock in the cpu node like that. I believe this is what the simple-card designer expected users to do since the cpu node is the first place that the driver tries to look at.
I think that the proper solution would be to add check for:
freq < sysclk/5 in fsl_ssi_set_dai_sysclk() and return -ENOTSUPP to make the simple-audo-card driver happy (and not introducing regressions).
As I said in the first place, adding another check in set_sysclk() is not that essential but seems to be plausible to me. So I am okay if you really want to have that.
Hi Nicolin,
On Wed, Sep 06, 2017 at 11:22:48AM +0200, Łukasz Majewski wrote:
Here is the routine that I understood from the code:
- asoc_simple_card_parse_clk_cpu(dev, cpu, dai_link, cpu_dai); => asoc_simple_card_parse_clk(dev, cpu, // cpu node in sound{} [1] dai_link->cpu_of_node, // node ssi2 [2] cpu_dai, dai_link->cpu_dai_name); ==> 1.1) devm_get_clk_from_child(dev, node, NULL); // [1] ==> 1.2) of_property_read_u32(node, "system-clock-frequency", &val)// [1] ==> 1.3) devm_get_clk_from_child(dev, dai_of_node, NULL); // [2]
For the cpu routine, it first checks for clock property under cpu node of simple-card, then for "system-clock-frequency" in the cpu node of simple-card, and finally looks for clock property in ssi2 node.
-----> dev: sound -----> clk node: /soc/aips-bus@02000000/spba-bus@02000000/ssi@0202c000 -----> Clk asignment
And this clock is taken from this node. It looks like ipg clock for ssi...
This makes sense now. The devm_get_clk_from_child() in 1.3) fetched the first clock of ssi2 -- ipg clock.
Yes, exactly (for SSI2) [1]:
clocks = <&clks IMX6QDL_CLK_SSI2_IPG>, <&clks IMX6QDL_CLK_SSI2>; clock-names = "ipg", "baud";
The problem is with the "lack" of clock nodes/properties at
dailink_master: cpu { sound-dai = <&ssi2>; clock = <&SSSS>;
If possible I do prefer a solution, which uses only DTS. Side question - how to refer to baud clock from [1]?
system-clock-frequency = <XXXX>; };This is the right solution based on current simple-card driver. For SSI (having two clocks), you have to specify the baud clock in the cpu node like that. I believe this is what the simple-card designer expected users to do since the cpu node is the first place that the driver tries to look at.
I will give a shoot the option with adding the ipg clock.
I think that the proper solution would be to add check for:
freq < sysclk/5 in fsl_ssi_set_dai_sysclk() and return -ENOTSUPP to make the simple-audo-card driver happy (and not introducing regressions).
As I said in the first place, adding another check in set_sysclk() is not that essential but seems to be plausible to me. So I am okay if you really want to have that.
Thanks for help.
On Wed, Sep 06, 2017 at 08:35:50PM +0200, Łukasz Majewski wrote:
clocks = <&clks IMX6QDL_CLK_SSI2_IPG>, <&clks IMX6QDL_CLK_SSI2>; clock-names = "ipg", "baud";
dailink_master: cpu { sound-dai = <&ssi2>; clock = <&SSSS>;If possible I do prefer a solution, which uses only DTS. Side question - how to refer to baud clock from [1]?
Just add a property to this cpu node like: clock = <&clks IMX6QDL_CLK_SSI2>;
system-clock-frequency = <XXXX>;
This would not be necessary unless you want to specify a clock rate so as to override the clock rate configuration in hw_params().
This is the right solution based on current simple-card driver. For SSI (having two clocks), you have to specify the baud clock in the cpu node like that. I believe this is what the simple-card designer expected users to do since the cpu node is the first place that the driver tries to look at.
I will give a shoot the option with adding the ipg clock.
No, not ipg clock. You should use the second clock -- baud clock.
On 09/06/2017 09:47 PM, Nicolin Chen wrote:
On Wed, Sep 06, 2017 at 08:35:50PM +0200, Łukasz Majewski wrote:
clocks = <&clks IMX6QDL_CLK_SSI2_IPG>, <&clks IMX6QDL_CLK_SSI2>; clock-names = "ipg", "baud";
dailink_master: cpu { sound-dai = <&ssi2>; clock = <&SSSS>;If possible I do prefer a solution, which uses only DTS. Side question - how to refer to baud clock from [1]?
Just add a property to this cpu node like: clock = <&clks IMX6QDL_CLK_SSI2>;
Ok.
system-clock-frequency = <XXXX>;This would not be necessary unless you want to specify a clock rate so as to override the clock rate configuration in hw_params().
Ok.
This is the right solution based on current simple-card driver. For SSI (having two clocks), you have to specify the baud clock in the cpu node like that. I believe this is what the simple-card designer expected users to do since the cpu node is the first place that the driver tries to look at.
I will give a shoot the option with adding the ipg clock.
No, not ipg clock. You should use the second clock -- baud clock.
Yes. Correct - the baud clock.
Hi Nicolin,
On Wed, Sep 06, 2017 at 08:35:50PM +0200, Łukasz Majewski wrote:
clocks = <&clks IMX6QDL_CLK_SSI2_IPG>, <&clks IMX6QDL_CLK_SSI2>; clock-names = "ipg", "baud";
dailink_master: cpu { sound-dai = <&ssi2>; clock = <&SSSS>;If possible I do prefer a solution, which uses only DTS. Side question - how to refer to baud clock from [1]?
Just add a property to this cpu node like: clock = <&clks IMX6QDL_CLK_SSI2>;
This doesn't solve the issue:
root@display5:~# speaker-test
speaker-test 1.1.3
Playback device is default Stream parameters are 48000Hz, S16_LE, 1 channels Using 16 octaves of pink noise Rate set to 48000Hz (requested 48fsl-ssi-dai 202c000.ssi: bitclk > ipgclk/5 000Hz) Buffer size range from 64fsl-ssi-dai 202c000.ssi: ASoC: can't set 202c000.ssi hw params: -22 to 65536 Period size range from 32 to 8191 Using max buffer size 65536 Periods = 4 Unable to set hw params for playback: Invalid argument Setting of hwparams failed: Invalid argument
system-clock-frequency = <XXXX>;This would not be necessary unless you want to specify a clock rate so as to override the clock rate configuration in hw_params().
This is the right solution based on current simple-card driver. For SSI (having two clocks), you have to specify the baud clock in the cpu node like that. I believe this is what the simple-card designer expected users to do since the cpu node is the first place that the driver tries to look at.
I will give a shoot the option with adding the ipg clock.
No, not ipg clock. You should use the second clock -- baud clock.
On Fri, Sep 08, 2017 at 01:10:12AM +0200, Łukasz Majewski wrote:
Just add a property to this cpu node like: clock = <&clks IMX6QDL_CLK_SSI2>;
This doesn't solve the issue:
I have a patch locally that should be able to solve your problem. But I need to first verify on my board tonight and will send it later (will put you in the TO/CC list).
On Tue, Sep 5, 2017 at 6:13 PM, Łukasz Majewski lukma@denx.de wrote:
&i2c1 { clock-frequency = <400000>; pinctrl-names = "default"; pinctrl-0 = <&pinctrl_i2c1>; status = "okay";
codec: tfa9879@6C { #sound-dai-cells = <0>; compatible = "tfa9879";
This codec seems to miss device tree support. Don't you need something like this?
--- a/sound/soc/codecs/tfa9879.c +++ b/sound/soc/codecs/tfa9879.c @@ -312,9 +312,15 @@ static const struct i2c_device_id tfa9879_i2c_id[] = { }; MODULE_DEVICE_TABLE(i2c, tfa9879_i2c_id);
+static const struct of_device_id tfa9879_of_match[] = { + { .compatible = "nxp,tfa9879", }, + { } +}; + static struct i2c_driver tfa9879_i2c_driver = { .driver = { .name = "tfa9879", + .of_match_table = tfa9879_of_match, }, .probe = tfa9879_i2c_probe, .remove = tfa9879_i2c_remove,
Hi Fabio,
On Tue, Sep 5, 2017 at 6:13 PM, Łukasz Majewski lukma@denx.de wrote:
&i2c1 { clock-frequency = <400000>; pinctrl-names = "default"; pinctrl-0 = <&pinctrl_i2c1>; status = "okay";
codec: tfa9879@6C { #sound-dai-cells = <0>; compatible = "tfa9879";This codec seems to miss device tree support. Don't you need something like this?
--- a/sound/soc/codecs/tfa9879.c +++ b/sound/soc/codecs/tfa9879.c @@ -312,9 +312,15 @@ static const struct i2c_device_id tfa9879_i2c_id[] = { }; MODULE_DEVICE_TABLE(i2c, tfa9879_i2c_id);
+static const struct of_device_id tfa9879_of_match[] = {
{ .compatible = "nxp,tfa9879", },{ }+};
- static struct i2c_driver tfa9879_i2c_driver = { .driver = { .name = "tfa9879",
.of_match_table = tfa9879_of_match, }, .probe = tfa9879_i2c_probe, .remove = tfa9879_i2c_remove,
Maybe it should be added, but the driver is probed via I2C "node" in dts.
I took the codec (tfa9879) driver as-is.
Hi Lukasz,
On Tue, Sep 5, 2017 at 5:35 AM, Łukasz Majewski lukma@denx.de wrote:
Note: [*] - I could workaround this problem by setting:
system-clock-frequency = <0> in
dailink_master: cpu { sound-dai = <&ssi2>; };
Which mx6 type are you using? Can you post the complete audio related bindings of your dts?
Still trying to understand why we do not see this error on other imx6 boards.
Hi Fabio,
Hi Lukasz,
On Tue, Sep 5, 2017 at 5:35 AM, Łukasz Majewski lukma@denx.de wrote:
Note: [*] - I could workaround this problem by setting:
system-clock-frequency = <0> in
dailink_master: cpu { sound-dai = <&ssi2>; };Which mx6 type are you using? Can you post the complete audio related bindings of your dts?
Still trying to understand why we do not see this error on other imx6 boards.
I've sent my DTS in the other e-mail.
On Sun, Sep 03, 2017 at 01:05:01PM +0200, Lukasz Majewski wrote:
The problem is visible in the following setup (on the imx6q): "simple-audio-card" -> ssi2 -> I2S + I2C -> codec
The function call log (simple-card probe -> CONFIG_SND_SIMPLE_CARD):
asoc_simple_card_init_dai() @ sound/soc/generic/simple-card-utils.c snd_soc_dai_set_sysclk() fsl_ssi_set_dai_sysclk() @ sound/soc/fsl/fsl_ssi.c
The last call is changing the bit clock (BCLK) frequency to SSI's IP block clock (ipg = 66 MHz) [1].
I think a bigger question here is why the routine sets BCLK to 66MHz.
This is wrong, since IMX SSI block requires the I2S BCLK to be less than 1/5 of [1].
As a result the driver initialization passes without any errors, but the speaker-test test case breaks.
This commit checks if the fsl_ssi_set_dai_sysclk() frequency passed is not equal to [1].
I don't feel it's quite comprehensive...what if it's being set to 67MHz.
Thanks Nicolin
Signed-off-by: Lukasz Majewski lukma@denx.de
sound/soc/fsl/fsl_ssi.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c index 173cb84..1186fa9 100644 --- a/sound/soc/fsl/fsl_ssi.c +++ b/sound/soc/fsl/fsl_ssi.c @@ -809,6 +809,8 @@ 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);
if (clk_get_rate(ssi_private->clk) == freq)
return 0;ssi_private->bitclk_freq = freq;
-- 2.1.4
On 09/05/2017 07:06 AM, Nicolin Chen wrote:
On Sun, Sep 03, 2017 at 01:05:01PM +0200, Lukasz Majewski wrote:
The problem is visible in the following setup (on the imx6q): "simple-audio-card" -> ssi2 -> I2S + I2C -> codec
The function call log (simple-card probe -> CONFIG_SND_SIMPLE_CARD):
asoc_simple_card_init_dai() @ sound/soc/generic/simple-card-utils.c snd_soc_dai_set_sysclk() fsl_ssi_set_dai_sysclk() @ sound/soc/fsl/fsl_ssi.c
The last call is changing the bit clock (BCLK) frequency to SSI's IP block clock (ipg = 66 MHz) [1].
I think a bigger question here is why the routine sets BCLK to 66MHz.
Yes, exactly.
In my case the bclk is set to ipg clock, which is the SSI IP block clock (ipg).
This is wrong, since IMX SSI block requires the I2S BCLK to be less than 1/5 of [1].
As a result the driver initialization passes without any errors, but the speaker-test test case breaks.
This commit checks if the fsl_ssi_set_dai_sysclk() frequency passed is not equal to [1].
I don't feel it's quite comprehensive...what if it's being set to 67MHz.
I think that this clock is not changing for the SoC. It should be 66 MHz fixed.
Thanks Nicolin
Signed-off-by: Lukasz Majewski lukma@denx.de
sound/soc/fsl/fsl_ssi.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c index 173cb84..1186fa9 100644 --- a/sound/soc/fsl/fsl_ssi.c +++ b/sound/soc/fsl/fsl_ssi.c @@ -809,6 +809,8 @@ 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);
if (clk_get_rate(ssi_private->clk) == freq)
return 0;ssi_private->bitclk_freq = freq;
-- 2.1.4
On Tue, Sep 05, 2017 at 09:37:43AM +0200, Łukasz Majewski wrote:
The last call is changing the bit clock (BCLK) frequency to SSI's IP block clock (ipg = 66 MHz) [1].
I think a bigger question here is why the routine sets BCLK to 66MHz.
Yes, exactly.
In my case the bclk is set to ipg clock, which is the SSI IP block clock (ipg).
Can you elaborate why you set ipg clock as bclk? I don't remember SSI could derive bitclock from ipg clock.
This is wrong, since IMX SSI block requires the I2S BCLK to be less than 1/5 of [1].
As a result the driver initialization passes without any errors, but the speaker-test test case breaks.
This commit checks if the fsl_ssi_set_dai_sysclk() frequency passed is not equal to [1].
I don't feel it's quite comprehensive...what if it's being set to 67MHz.
I think that this clock is not changing for the SoC. It should be 66 MHz fixed.
What I mean is that we cannot just look at this SoC. Today is 66MHz for this SoC. Tomorrow could be 133MHz for another one. We should put a check that none of these shall pass -- the 1/5 limit.
On 09/05/2017 09:52 AM, Nicolin Chen wrote:
On Tue, Sep 05, 2017 at 09:37:43AM +0200, Łukasz Majewski wrote:
The last call is changing the bit clock (BCLK) frequency to SSI's IP block clock (ipg = 66 MHz) [1].
I think a bigger question here is why the routine sets BCLK to 66MHz.
Yes, exactly.
In my case the bclk is set to ipg clock, which is the SSI IP block clock (ipg).
Can you elaborate why you set ipg clock as bclk? I don't remember SSI could derive bitclock from ipg clock.
Just to be clear:
What clock shall be set with:
struct snd_soc_dai_ops {
int (*set_sysclk)(struct snd_soc_dai *dai, int clk_id, unsigned int freq, int dir); }
callback?
The SSI IP block or BCLK ?
This is wrong, since IMX SSI block requires the I2S BCLK to be less than 1/5 of [1].
As a result the driver initialization passes without any errors, but the speaker-test test case breaks.
This commit checks if the fsl_ssi_set_dai_sysclk() frequency passed is not equal to [1].
I don't feel it's quite comprehensive...what if it's being set to 67MHz.
I think that this clock is not changing for the SoC. It should be 66 MHz fixed.
What I mean is that we cannot just look at this SoC. Today is 66MHz for this SoC. Tomorrow could be 133MHz for another one. We should put a check that none of these shall pass -- the 1/5 limit.
Ok. Good point.
On Tue, Sep 05, 2017 at 10:19:05AM +0200, Łukasz Majewski wrote:
On 09/05/2017 09:52 AM, Nicolin Chen wrote:
Can you elaborate why you set ipg clock as bclk? I don't remember SSI could derive bitclock from ipg clock.
Just to be clear:
What clock shall be set with:
struct snd_soc_dai_ops { int (*set_sysclk)(struct snd_soc_dai *dai, int clk_id, unsigned int freq, int dir); }
callback?
The SSI IP block or BCLK ?
Not the BCLK, probably the IP clock but perhaps nothing. The bclk is usually derived from the sample rate and width, the system clock is the clock rate going into the device. It doesn't *have* to be configured (particuarly if it's discoverable by the IP and managable via the clock API).
On Tue, Sep 05, 2017 at 04:15:50PM +0100, Mark Brown wrote:
Just to be clear:
What clock shall be set with:
struct snd_soc_dai_ops { int (*set_sysclk)(struct snd_soc_dai *dai, int clk_id, unsigned int freq, int dir); }
callback?
The SSI IP block or BCLK ?
Not the BCLK, probably the IP clock but perhaps nothing. The bclk is usually derived from the sample rate and width, the system clock is the clock rate going into the device. It doesn't *have* to be configured (particuarly if it's discoverable by the IP and managable via the clock API).
Hmm...to clarify the things, some background here: SSI has mainly two different clock inputs internally (not external pads such as bclk or lrclk): IP block clock (ipg_clk in Reference Manual) and sys clock (ccm_ssi_clk in Reference Manual). According to RM, ccm_ssi_clk: "module/system clock for bit clock generation".
The ipg clock is merely used to access registers, and has nothing (directly) to do with external clock outputs. The driver shall not change the ipg clock as the system ipg clock (its parent clock) might be messed and even system time would get weird -- happened once when the fsl_spdif driver used to call clk_set_rate() on its ipg clock. Although the clock controller should have some kind of protection in my opinion, we just avoid IP clock rate change in all audio drivers as well.
On the other hand, the sys clock (baudclk in the driver) should be configured whenever it's related to external clock outputs. When I implemented this set_sysclk() for fsl_ssi.c, I used it to set this sys clock (baudclk) by a machine driver, in order to set bit clock. Then someone patched the driver by moving all the code to set_bclk() to make machine drivers simpler. Now the set_sysclk() is remained to give machine drivers a chance to override clock configurations in the hw_params(). This could be used in TDM or some other special cases (It could also have a purpose for backwards compatibility).
So here, we should set baudclk (BCLK generator).
On Tue, Sep 05, 2017 at 10:45:29AM -0700, Nicolin Chen wrote:
The ipg clock is merely used to access registers, and has nothing (directly) to do with external clock outputs. The driver shall not change the ipg clock as the system ipg clock (its parent clock) might be messed and even system time would get weird -- happened once when the fsl_spdif driver used to call clk_set_rate() on its ipg clock. Although the clock controller should have some kind of protection in my opinion, we just avoid IP clock rate change in all audio drivers as well.
Yes, the clock API needs constraints code.
On the other hand, the sys clock (baudclk in the driver) should be configured whenever it's related to external clock outputs. When I implemented this set_sysclk() for fsl_ssi.c, I used it to set this sys clock (baudclk) by a machine driver, in order to set bit clock. Then someone patched the driver by moving all the code to set_bclk() to make machine drivers simpler. Now the set_sysclk() is remained to give machine drivers a chance to override clock configurations in the hw_params(). This could be used in TDM or some other special cases (It could also have a purpose for backwards compatibility).
So here, we should set baudclk (BCLK generator).
No, that's just going to cause confusion - if all the other drivers are using set_sysclk() to set an input clock rate to the IP rather than an output clock but your driver does something else then sooner or later someone will run into trouble with that.
On Thu, Sep 07, 2017 at 02:44:11PM +0100, Mark Brown wrote:
On the other hand, the sys clock (baudclk in the driver) should be configured whenever it's related to external clock outputs. When I implemented this set_sysclk() for fsl_ssi.c, I used it to set this sys clock (baudclk) by a machine driver, in order to set bit clock. Then someone patched the driver by moving all the code to set_bclk() to make machine drivers simpler. Now the set_sysclk() is remained to give machine drivers a chance to override clock configurations in the hw_params(). This could be used in TDM or some other special cases (It could also have a purpose for backwards compatibility).
So here, we should set baudclk (BCLK generator).
No, that's just going to cause confusion - if all the other drivers are using set_sysclk() to set an input clock rate to the IP rather than an output clock but your driver does something else then sooner or later someone will run into trouble with that.
I admit I had that concern. Probably I should have deprecated this set_sysclk(). I will try to patch it and hw_params() accordingly.
participants (6)
-
Fabio Estevam -
Fabio Estevam -
Lukasz Majewski -
Mark Brown -
Nicolin Chen -
Łukasz Majewski