Message-ID: <4FBA89E3.7010106@gmail.com>
Date: Mon, 21 May 2012 13:30:59 -0500
From: Rob Herring <robherring2@gmail.com>
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:12.0) Gecko/20120430 Thunderbird/12.0.1
MIME-Version: 1.0
To: Shawn Guo <shawn.guo@linaro.org>
CC: Shawn Guo <shawn.guo@freescale.com>,
        Mike Turquette <mturquette@linaro.org>,
        Grant Likely <grant.likely@secretlab.ca>,
        "arm@kernel.org" <arm@kernel.org>,
        "linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [GIT PULL] DT clk binding support
References: <4FB80F32.5090309@gmail.com> <20120520030653.GB5810@S2100-06.ap.freescale.net> <4FB9A5E7.2070000@gmail.com> <20120521064901.GE8140@S2101-09.ap.freescale.net>
In-Reply-To: <20120521064901.GE8140@S2101-09.ap.freescale.net>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3622
Lines: 78

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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/