[PATCH] sound: soc: fsl: Do not set DAI sysclk when it is equal to system freq

[PATCH] sound: soc: fsl: Do not set DAI sysclk when it is equal to system freq

[Lukasz Majewski]

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

Re: [PATCH] sound: soc: fsl: Do not set DAI sysclk when it is equal to system freq

[Fabio Estevam]

[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

Re: [PATCH] sound: soc: fsl: Do not set DAI sysclk when it is equal to system freq

[Fabio Estevam]

[-- Attachment #1: Type: text/plain, Size: 2150 bytes --]

[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


[-- Attachment #2: Type: text/html, Size: 4079 bytes --]

Re: [PATCH] sound: soc: fsl: Do not set DAI sysclk when it is equal to system freq

[Łukasz Majewski]

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


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

Re: [PATCH] sound: soc: fsl: Do not set DAI sysclk when it is equal to system freq

[Fabio Estevam]

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>

Re: [PATCH] sound: soc: fsl: Do not set DAI sysclk when it is equal to system freq

[Fabio Estevam]

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>
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] sound: soc: fsl: Do not set DAI sysclk when it is equal to system freq

[Fabio Estevam]

On Sun, Sep 3, 2017 at 11:40 AM, =C5=81ukasz 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 =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 <fabio.estevam@nxp.com>

Re: [PATCH] sound: soc: fsl: Do not set DAI sysclk when it is equal to system freq

[Nicolin Chen]

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
> 

Re: [PATCH] sound: soc: fsl: Do not set DAI sysclk when it is equal to system freq

[Nicolin Chen]

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

Re: [PATCH] sound: soc: fsl: Do not set DAI sysclk when it is equal to system freq

[Łukasz Majewski]

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


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

Re: [PATCH] sound: soc: fsl: Do not set DAI sysclk when it is equal to system freq

[Nicolin Chen]

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.

Re: [PATCH] sound: soc: fsl: Do not set DAI sysclk when it is equal to system freq

[Łukasz Majewski]

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.


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

Re: [PATCH] sound: soc: fsl: Do not set DAI sysclk when it is equal to system freq

[Łukasz Majewski]

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.


-- 
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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] sound: soc: fsl: Do not set DAI sysclk when it is equal to system freq

[Łukasz Majewski]

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
> 


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

Re: [PATCH] sound: soc: fsl: Do not set DAI sysclk when it is equal to system freq

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 764 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] sound: soc: fsl: Do not set DAI sysclk when it is equal to system freq

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 764 bytes --]

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] sound: soc: fsl: Do not set DAI sysclk when it is equal to system freq

[Nicolin Chen]

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

Re: [PATCH] sound: soc: fsl: Do not set DAI sysclk when it is equal to system freq

[Nicolin Chen]

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?

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

Re: [PATCH] sound: soc: fsl: Do not set DAI sysclk when it is equal to system freq

[Fabio Estevam]

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.

Re: [PATCH] sound: soc: fsl: Do not set DAI sysclk when it is equal to system freq

[Fabio Estevam]

Hi Lukasz,

On Tue, Sep 5, 2017 at 5:35 AM, =C5=81ukasz Majewski <lukma@denx.de> 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.

Re: [PATCH] sound: soc: fsl: Do not set DAI sysclk when it is equal to system freq

[Łukasz Majewski]

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?
> 
>> 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?

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


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

Re: [PATCH] sound: soc: fsl: Do not set DAI sysclk when it is equal to system freq

[Łukasz Majewski]

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?
> 
>> 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?

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


-- 
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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] sound: soc: fsl: Do not set DAI sysclk when it is equal to system freq

[Łukasz Majewski]

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.

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

Re: [PATCH] sound: soc: fsl: Do not set DAI sysclk when it is equal to system freq

[Nicolin Chen]

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

Re: [PATCH] sound: soc: fsl: Do not set DAI sysclk when it is equal to system freq

[Nicolin Chen]

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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] sound: soc: fsl: Do not set DAI sysclk when it is equal to system freq

[Fabio Estevam]

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,

Re: [PATCH] sound: soc: fsl: Do not set DAI sysclk when it is equal to system freq

[Fabio Estevam]

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,
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] sound: soc: fsl: Do not set DAI sysclk when it is equal to system freq

[Fabio Estevam]

On Tue, Sep 5, 2017 at 6:13 PM, =C5=81ukasz Majewski <lukma@denx.de> wrote:

> &i2c1 {
>         clock-frequency =3D <400000>;
>         pinctrl-names =3D "default";
>         pinctrl-0 =3D <&pinctrl_i2c1>;
>         status =3D "okay";
>
>         codec: tfa9879@6C {
>                 #sound-dai-cells =3D <0>;
>                 compatible =3D "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[] =3D=
 {
 };
 MODULE_DEVICE_TABLE(i2c, tfa9879_i2c_id);

+static const struct of_device_id tfa9879_of_match[] =3D {
+       { .compatible =3D "nxp,tfa9879", },
+       { }
+};
+
 static struct i2c_driver tfa9879_i2c_driver =3D {
        .driver =3D {
                .name =3D "tfa9879",
+               .of_match_table =3D tfa9879_of_match,
        },
        .probe =3D tfa9879_i2c_probe,
        .remove =3D tfa9879_i2c_remove,

Re: [PATCH] sound: soc: fsl: Do not set DAI sysclk when it is equal to system freq

[Łukasz Majewski]

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.


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

Re: [PATCH] sound: soc: fsl: Do not set DAI sysclk when it is equal to system freq

[Łukasz Majewski]

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

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

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

Re: [PATCH] sound: soc: fsl: Do not set DAI sysclk when it is equal to system freq

[Łukasz Majewski]

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

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

> 
>> 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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] sound: soc: fsl: Do not set DAI sysclk when it is equal to system freq

[Nicolin Chen]

On Wed, Sep 06, 2017 at 11:22:48AM +0200, Łukasz Majewski wrote:

> >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]

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

Re: [PATCH] sound: soc: fsl: Do not set DAI sysclk when it is equal to system freq

[Nicolin Chen]

On Wed, Sep 06, 2017 at 11:22:48AM +0200, Łukasz Majewski wrote:

> >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]

> >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.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] sound: soc: fsl: Do not set DAI sysclk when it is equal to system freq

[Łukasz Majewski]

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:
>>> 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]
> 
>>> 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.

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

Re: [PATCH] sound: soc: fsl: Do not set DAI sysclk when it is equal to system freq

[Nicolin Chen]

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.

Re: [PATCH] sound: soc: fsl: Do not set DAI sysclk when it is equal to system freq

[Nicolin Chen]

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.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] sound: soc: fsl: Do not set DAI sysclk when it is equal to system freq

[Łukasz Majewski]

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.

> 


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

Re: [PATCH] sound: soc: fsl: Do not set DAI sysclk when it is equal to system freq

[Łukasz Majewski]

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.

> 


-- 
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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] sound: soc: fsl: Do not set DAI sysclk when it is equal to system freq

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1569 bytes --]

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.  

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] sound: soc: fsl: Do not set DAI sysclk when it is equal to system freq

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1569 bytes --]

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.  

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] sound: soc: fsl: Do not set DAI sysclk when it is equal to system freq

[Nicolin Chen]

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.

Re: [PATCH] sound: soc: fsl: Do not set DAI sysclk when it is equal to system freq

[Łukasz Majewski]

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


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

Re: [PATCH] sound: soc: fsl: Do not set DAI sysclk when it is equal to system freq

[Łukasz Majewski]

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


-- 
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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] sound: soc: fsl: Do not set DAI sysclk when it is equal to system freq

[Nicolin Chen]

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

Re: [PATCH] sound: soc: fsl: Do not set DAI sysclk when it is equal to system freq

[Nicolin Chen]

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).
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel