[RFC] clocktree representation in the devicetree

[Rob Herring]

Sascha,

On 10/18/2011 02:16 AM, Sascha Hauer wrote:
> On Mon, Oct 17, 2011 at 06:11:56PM -0500, Rob Herring wrote:
>> On 10/17/2011 01:43 PM, Sascha Hauer wrote:

snip

>>
>> Okay. Missed that not going far enough down into the dts...
>>
>> So either a clock can have an explicit parent (or list) or can inherit
>> from the parent node. That aligns pretty well with how interrupts are done.
>>
>> Perhaps it should be "clock-parent" rather than just parent.
> 
> Yes. Also we should make the difference clear between the possible
> parents of a mux and the one the user wants to select. So maybe
> 
> 'clock-parent' for the possible parents of a mux
> 
> and
> 
> 'selected-clock-parent' for the one the user wants to have.
> 

Looks good.

>>
>>>
>>> So the entry in imx53-qsb.dts is used to describe what a board wants
>>> and not what the current status is.
>>>
>> I worry that that could result in a lot of combinations of DT's that
>> won't boot. If it's all generic code, how do you ensure things are done
>> in the right order. There's lots of gotchas ensuring clocks stay in the
>> right ranges and ratios when you change them. I don't think the clock
>> hierarchy alone is enough information. As a simple example, what is the
>> maximum frequency of internal bus clocks. That is one of the primary
>> differences between MX51 and MX53 clocks.
> 
> If a user selects higher rates than allowed for a SoC he's doomed, just
> like he's doomed when he screws up the devicetree in other ways
> nowadays.
> 
> I agree that there will be problems when critical bus timings should be
> changed via the devicetree. This would either need special handling in
> the kernel or simply should be dissallowed.
> 
>>
>> Granted this problem exists already, but is it just making it easier to
>> hang yourself?
>>
>>>>
>>>> Will clocks ever become generic enough that it makes sense to describe
>>>> clocks in DT at the level of muxes, dividers, gates, etc.?
>>>
>>> I think yes. On i.MX processors you only need dividers, gates, muxes and
>>> plls. On other SoCs there may be table based dividers, power-of-2
>>> dividers or similar stuff, but overall there should be a quite limited
>>> set of features to be described.
>>>
>>
>> So how do you bind a "fsl,imx51-clk-mux" to yet to be written generic
>> clock mux code?
> 
> Well my first thought was to use normal (of-)platform devices, but as
> mentioned this may have some unwanted overhead. In case of the mux
> a generic mux could be used, so maybe compatible = "fsl,imx51-clk-mux",
> "clk-mux" should do it. In case of a gate we'll probably need a i.MX5
> specific variant as these SoCs use two bits for a single gate.
> 

You can easily point a specific compatible property to a generic
implementation. My concern is just that the binding properties be
presented generically so everyone pays attention (hopefully).

>> More importantly, are the properties exposed sufficient?
> 
> No :)
> 
> At least there should be a propagates flag, but there may be more.
> 

At least in the FSL BSP, that was always just an optimization to avoid
walking the tree needlessly. I think it could be determined from parent
and child information in the DT.

>>
>> While we may ultimately want the compatible strings to be SOC specific,
>> it would be good to start by generically defining bindings for dividers,
>> muxes, and gates and ensure we have something that works for all SOCs.
>>
>>>> Perhaps it
>>>> makes more sense to just describe the clock controller to device
>>>> connections and any board level clocks in the DT.
>>>
>>> Describing the clock tree in the device tree also makes it possible for
>>> a board to customize the divider/PLL/mux settings.
>>> Consider a SoC with a PLL where several different devices can derive its
>>> clock from.  One board may want to move all other devices away from this
>>> PLL and use it exclusively for sound to get an exact rate. Another might
>>> use it for the pixel clock and a third one selects a good compromise
>>> between an exact sound clock and the pixel clock. Not describing this in
>>> the device tree means that we need board specific code with
>>> clk_get/clk_get_parent/clk_set_rate orgies. 
>>> I gave up on creating clock code that magically tries to do everything
>>> right based on clk_* functions. With the example above how should the
>>> clock code know how to adjust the mentioned PLL? If you managed to get
>>> it right for one SoC the next totally different SoC will already be in
>>> the pipeline.
>>>
>> Good point. I guess the board specifics can go all the way back up the
>> clock tree.
>>
>> It's good to see a real example. but it would be nice to see some
>> documentation to update this:
>>
>> http://www.devicetree.org/ClockBindings
>>
>> Do you have any code using this? I've updated the OF clock support based
>> on Mike's latest common clk code that I need to send out. But it's based
>> on the above clock binding.
> 
> I wasn't aware of this page. As I see it I could rewrite my suggestion
> so that it extends these clock bindings without really changing them.
> I have no code working yet, I first wanted to see how the reactions are.
> 

To be clear, these are just proposed bindings, so they can be changed
still. In fact, Grant said at LPC that he wants to make some changes.
Perhaps Grant can chime in.

Honestly, I like your proposal better. I think having a single clock per
DT node is a good thing. This can simplify parsing the tree and should
make implementing generic code easier.

Do we need clock name strings on outputs or just phandles? Many
intermediate clocks don't really need a name other than the phandle name.

IIRC, Grant suggesting aligning with how doing reg names (aka
resource_byname) was agreed upon at LPC. Something like this:

clocks = <&clk_phandle1, &clk_phandle2>;
clock-names = <"clk1", "clk2">;

clock-names could then be optional.

Rob