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

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

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