linux-kernel - Re: [GIT PULL] DT clk binding support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4FBA89E3.7010106@gmail.com>
Date:	Mon, 21 May 2012 13:30:59 -0500
From:	Rob Herring <robherring2@...il.com>
To:	Shawn Guo <shawn.guo@...aro.org>
CC:	Shawn Guo <shawn.guo@...escale.com>,
	Mike Turquette <mturquette@...aro.org>,
	Grant Likely <grant.likely@...retlab.ca>,
	"arm@...nel.org" <arm@...nel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [GIT PULL] DT clk binding support

On 05/21/2012 01:49 AM, Shawn Guo wrote:
> On Sun, May 20, 2012 at 09:18:15PM -0500, Rob Herring wrote:
>>> As I said, any clock in the clock tree except root clock is not only
>>> a clock provider but also a consumer.  If you define "clocks" as a
>>> required property for clock consumers, you are essentially asking users
>>> to either define the whole clock tree in the device tree or stay away
>>> from device tree completely.
>>
>> So what are you proposing that a clock consumer have? The very
>> definition of a clock consumer is that it has a clocks property.
>>
> To support the cases that the clock tree is defined by clock driver
> and device tree together, the "clocks" property could be reasonably
> absent in case the parent clock (provider) is being defined in driver
> than DT.  And for such clocks, the "clock-names" than "clocks" should
> be required to find the clock provider/parent.
> 
> I do not like the idea to look for clock with name too much, but I
> do not see other way around to support those platforms that have clock
> tree definition split in clock driver and device tree. 
> 
>>>
>>> Are you sure this is the right thing to do?  If I remember correctly,
>>> Grant's position is it should be pretty reasonable to have most of
>>> the clock tree defined in clock driver and only define those leaf
>>> clocks which are very likely to become the clock providers for other
>>> peripherals.
>>
>> The minimum is you have to have a provider and consumer. It may be a
>> single provider that provides all clocks for a chip. If you don't want a
>> provider, then just define a clock-frequency property.
>>>
>>> Let me put a terrible example here.  Since clock tree is actually SoC
>>> specific, I can reasonably choose to define the entire imx6q clock tree
>>> and all the clk lookups for imx6q peripherals in clk-imx6q driver.
>>> On imx6q-sabrelite board, the audio codec sgtl5000 uses cko (an imx6q
>>> clock available on pad) as the clock source.  That said, I need a board
>>> specific clk lookup here, which should be the best user of clock DT
>>> bindings.  But sadly, with the current bindings, I can not give the
>>> required "clocks" property for sgtl5000 node.
>>
>> I don't understand your example. For the sgtl5000 on the sabrelite, you
>> would provide a phandle and cell entry that is interpreted as the cko
>> pin.
> 
> With the bindings here, I need something like below in device tree to
> replace the clk lookup registration that is currently done in imx6q
> sabrelite specific setup code.  However the problem here is I have cko
> defined in clock driver, and thus I can not give phandle to cko in
> device tree.  What I suggest is for such cases, we could require
> clock-names = "cko" than clocks = <&cko>, and of_clk_get() should also
> be able to find the clock with looking for the clk name.
> 
> imx6q-sabrelite.dts:
> 
> 	codec: sgtl5000@0a {
> 		compatible = "fsl,sgtl5000";
> 		reg = <0x0a>;
> 		clocks = <&cko>;
> 	};
> 
> mach-imx6q.c, imx6q_sabrelite_cko1_setup(): 
> 
>         cko1 = clk_get_sys(NULL, "cko1");
>         clk_register_clkdev(cko1, NULL, "0-000a");
> 

What!? This is a terrible abuse/hack of the clock binding and is in no
way what was intended. You cannot use half of the clock binding. You
have to have a provider. The primary binding is a phandle reference.
clock-names is just auxiliary data.

Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/