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