Message-ID: <49cd509b59c8172c880b0c8a50c0f02c.squirrel@www.codeaurora.org>
In-Reply-To: <4FBB9A08.2050803@gmail.com>
References: <4FB80F32.5090309@gmail.com>
    <20120520030653.GB5810@S2100-06.ap.freescale.net>
    <4FB9A5E7.2070000@gmail.com>
    <20120521064901.GE8140@S2101-09.ap.freescale.net>
    <4FBA89E3.7010106@gmail.com>
    <20120521232616.GF8140@S2101-09.ap.freescale.net>
    <4FBAD545.7060803@gmail.com>
    <20120522021535.GG8140@S2101-09.ap.freescale.net>
    <4FBB134D.6050409@codeaurora.org> <4FBB9A08.2050803@gmail.com>
Date: Tue, 22 May 2012 18:38:44 -0700 (PDT)
Subject: Re: [GIT PULL] DT clk binding support
From: "Saravana Kannan" <skannan@codeaurora.org>
To: "Rob Herring" <robherring2@gmail.com>
Cc: "Stephen Boyd" <sboyd@codeaurora.org>, "Shawn Guo" <shawn.guo@linaro.org>,
        "Mike Turquette" <mturquette@linaro.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "Grant Likely" <grant.likely@secretlab.ca>,
        "arm@kernel.org" <arm@kernel.org>,
        "Shawn Guo" <shawn.guo@freescale.com>,
        "linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>,
        "Saravana Kannan" <skannan@codeaurora.org>
User-Agent: SquirrelMail/1.4.17
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7BIT
Importance: Normal
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4076
Lines: 86


On Tue, May 22, 2012 6:52 am, Rob Herring wrote:
> On 05/21/2012 11:17 PM, Stephen Boyd wrote:
>> On 05/21/12 19:15, Shawn Guo wrote:
>>> On Mon, May 21, 2012 at 06:52:37PM -0500, Rob Herring wrote:
>>>> As Grant states: "This proposed binding is only about one thing:
>>>> attaching clock providers to clock consumers." This means you have to
>>>> have at least a single provider and a single consumer defined in the
>>>> DT.
>>>>
>>> I just read through Grant's comments over again.  I agree with the
>>> statement which implicitly requires the clk provider defined in DT.
>>> However, for some case, this provider in DT is just a skeleton which
>>> is backed by clock driver where the provider is actually defined.
>>>
>>> Looking at Grant's comment below, the second option is also to match
>>> the clock in driver just using name.  The only difference to my
>>> proposal is the name here is given by the argument of phandle pointing
>>> to that skeleton provider node.
>>>
>>> I'm fine with that.  So go ahead with your bindings.
>>>
>>
>> Can we do what the regulator framework has done and have a common
>> binding of <connection_name>-clk = <&phandle>? Something like:
>>
>>     core-clk = <&uart3_clk>
>>
>> and then have clk_get() use the of node of the device passed in to find
>> a property named %s-clk and find the clock with the matching phandle.
>
> Sigh... That is what we had in previous versions from over a year ago
> and we moved away from that approach. The current binding has been
> reviewed multiple times in the last 6 months...
>
> The current approach is aligned with how interrupts are handled (with
> the addition of a phandle). I think not having per clock property names
> is easier to parse and easier to document.
>
>> This looks like it's trying to cover both the end consumers (uart uses
>> uart3_clk) and the internal clock tree consumers (a crystal oscillator
>> connects to a PLL or a mux has multiple parents). We can certainly use
>> these bindings for muxes and internal parent-child relationships but I
>> would prefer we use different bindings for consumer bindings that match
>> what regulators do today.
>
> The binding supports either defining every last internal clock or just
> the leaf clocks. I took the former route on highbank since I don't have
> a lot of clocks. If I was doing imx or omap for example, I'd probably
> just define all the clock controller outputs.
>

If only the leaf nodes are defined in DT, then how is the clock platform
driver implementer supposed to instantiate the rest of the tree and
connect it up with the partial list of clocks in DT? So, they have to
switch back and forth between DT and the .c file which defines the rest
and make sure the parent<->child names match?

To me it looks that it might better to decouple the description of the
clock HW from the mapping of a clock leaf to a consumer device. If we just
use a string to identify the clock that's consumed by a device, we can
achieve this decoupling at a clean boundary -- clock consumers devices
(UART) vs clock producer devices (clock controller in the SoC, in a PMIC,
audio codec, etc).

With the decoupling, we don't have the inconsistency of having some of the
clocks of a clock producer device incompletely defined in DT and the rest
of the clocks of the same clock producer device hard coded in the kernel.
So, you either put your entire clock tree in the SoC in the DT or put all
of it in the kernel but you aren't forced to put just some of them in the
DT just to get DT working. I see no benefit in defining only some of the
clocks in DT -- it just adds more confusion in the clock tree definition.
What am I missing?

Thanks,
Saravana

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

--
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/