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

["Łukasz Majewski" <lukma@denx.de>]

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