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(+)
[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(+)
[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.
quoted hunk
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(+)
On Sun, Sep 3, 2017 at 11:40 AM, =C5=81ukasz Majewski [off-list ref] 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 =3D freq;).
Before the aforementioned check we do have:
if (ssi_private->bitclk_freq)
freq =3D ssi_private->bitclk_freq;
else
freq =3D params_channels(hw_params) * 32 *
params_rate(hw_params);
Which assigns freq =3D 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 =3D 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 <redacted>
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
On Sun, Sep 03, 2017 at 04:40:21PM +0200, Łukasz Majewski wrote:
quoted
/*
* 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 Sun, Sep 03, 2017 at 01:05:01PM +0200, Lukasz Majewski wrote:
quoted
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).
quoted
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.
On Tue, Sep 05, 2017 at 09:37:43AM +0200, Łukasz Majewski wrote:
quoted
quoted
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.
quoted
quoted
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 Tue, Sep 05, 2017 at 09:37:43AM +0200, Łukasz Majewski wrote:
quoted
quoted
quoted
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 ?
quoted
quoted
quoted
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.
--
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 Sun, Sep 03, 2017 at 04:40:21PM +0200, Łukasz Majewski wrote:
quoted
quoted
/*
* 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)
[...]
quoted
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).
quoted
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.
quoted
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
--
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
From: Mark Brown <broonie@kernel.org> Date: 2017-09-05 15:16:37
On Tue, Sep 05, 2017 at 10:19:05AM +0200, Łukasz Majewski wrote:
On 09/05/2017 09:52 AM, Nicolin Chen wrote:
quoted
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:
quoted
Just to be clear:
quoted
What clock shall be set with:
quoted
struct snd_soc_dai_ops {
int (*set_sysclk)(struct snd_soc_dai *dai,
int clk_id, unsigned int freq, int dir);
}
quoted
callback?
quoted
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:35:34AM +0200, Łukasz Majewski wrote:
quoted
And apparently, we shouldn't set bitclk to 66MHz either. Can
you help to find where this 66MHz comes from?
2. 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 Lukasz,
On Tue, Sep 5, 2017 at 5:35 AM, =C5=81ukasz Majewski [off-list ref] wrote:
Note:
[*] - I could workaround this problem by setting:
system-clock-frequency =3D <0> in
dailink_master: cpu {
sound-dai =3D <&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 board=
s.
On Tue, Sep 05, 2017 at 10:35:34AM +0200, Łukasz Majewski wrote:
quoted
quoted
And apparently, we shouldn't set bitclk to 66MHz either. Can
you help to find where this 66MHz comes from?
quoted
2. int asoc_simple_card_init_dai() @ simple-card-utils.c
Oh, I just searched in the simple-card.c but missed this file.
quoted
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.
quoted
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
quoted
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.
quoted
[*] - 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.
--
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
Hi Lukasz,
On Tue, Sep 5, 2017 at 5:35 AM, Łukasz Majewski [off-list ref] wrote:
quoted
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.
--
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 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.
quoted
quoted
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
Maybe it should be added, but the driver is probed via I2C "node" in dts.
I took the codec (tfa9879) driver as-is.
--
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 Tue, Sep 05, 2017 at 11:13:40PM +0200, Łukasz Majewski wrote:
quoted
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.
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.
quoted
quoted
quoted
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.
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.
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.
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.
quoted
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).
--
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 Wed, Sep 06, 2017 at 11:22:48AM +0200, Łukasz Majewski wrote:
quoted
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]
quoted
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.
On Wed, Sep 06, 2017 at 11:22:48AM +0200, Łukasz Majewski wrote:
quoted
quoted
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]
quoted
quoted
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.
quoted
-----> 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>;
If possible I do prefer a solution, which uses only DTS.
Side question - how to refer to baud clock from [1]?
quoted
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.
quoted
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.
--
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
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>;
quoted
quoted
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().
quoted
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.
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.
quoted
quoted
quoted
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.
quoted
quoted
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.
--
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
From: Mark Brown <broonie@kernel.org> Date: 2017-09-07 13:44:38
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:
quoted
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).
quoted
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.
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
quoted
quoted
quoted
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().
quoted
quoted
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.
--
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 Fri, Sep 08, 2017 at 01:10:12AM +0200, Łukasz Majewski wrote:
quoted
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).