Message-ID: <20111116221250.GA12884@ponder.secretlab.ca>
Date:	Wed, 16 Nov 2011 15:12:50 -0700
From:	Grant Likely <grant.likely@...retlab.ca>
To:	"Cousson, Benoit" <b-cousson@...com>
Cc:	Sascha Hauer <s.hauer@...gutronix.de>,
	devicetree-discuss@...ts.ozlabs.org, linux-kernel@...r.kernel.org,
	Rob Herring <rob.herring@...xeda.com>,
	Sascha Hauer <kernel@...gutronix.de>,
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [RFC 6/8] of: add clock providers

On Fri, Nov 11, 2011 at 08:57:59PM +0100, Cousson, Benoit wrote:
> On 11/9/2011 12:49 PM, Sascha Hauer wrote:
> >On Wed, Nov 09, 2011 at 12:23:25PM +0100, Cousson, Benoit wrote:
> >>On 11/9/2011 10:13 AM, Sascha Hauer wrote:
> >>>On Tue, Nov 08, 2011 at 06:19:41PM -0700, Grant Likely wrote:
> >>
> >>[...]
> >>
> >>>>+Sources of clock signal can be represented by any node in the device
> >>>>+tree.  Those nodes are designated as clock providers.  Clock consumer
> >>>>+nodes use a phandle and clock specifier pair to connect clock provider
> >>>>+outputs to clock inputs.  Similar to the gpio specifiers, a clock
> >>>>+specifier is an array of one more more cells identifying the clock
> >>>>+output on a device.  The length of a clock specifier is defined by the
> >>>>+value of a #clock-cells property in the clock provider node.
> >>>>+
> >>>>+[1] http://patchwork.ozlabs.org/patch/31551/
> >>>>+
> >>>>+==Clock providers==
> >>>>+
> >>>>+Required properties:
> >>>>+#clock-cells:	   Number of cells in a clock specifier; typically will be
> >>>>+		   set to 1
> >>>>+
> >>>>+Optional properties:
> >>>>+clock-output-name: Recommended to be a list of strings of clock output signal
> >>>>+		   names indexed by the first cell in the clock specifier.
> >>>>+		   However, the meaning of clock-output-names is domain
> >>>>+		   specific to the clock provider, and is only provided to
> >>>>+		   encourage using the same meaning for the majority of clock
> >>>>+		   providers.  This format may not work for clock providers
> >>>>+		   using a complex clock specifier format.  In those cases it
> >>>>+		   is recommended to omit this property and create a binding
> >>>>+		   specific names property.
> >>>
> >>>If the clock-output-name property is omitted, does this mean a clock
> >>>provider only has a single output or does it mean that it's not known
> >>>how many clock outputs a provider actually has?

As Rob said, it only means that there are no names assigned to those
inputs.  In fact, it leaves it up to the specific clock binding as to
what it really means.  One driver may require the clock-output-name
property to be present because it uses data from there to figure out
how to configure the clock.  Another might not require it at all and
the only reason for its existence is informational.

> >>Allowing several outputs for a single clock node might lead to a lot
> >>of confusion. What will be the meaning of a clock rate if you have
> >>several outputs at different frequency?

No, clock-frequency is specific to the fixed-clock binding (which I
need to document).  That may or may not be present in other clock
provider nodes.  For instance, there is a xilinx clock divider ip
block that doesn't have any frequency of its own, but rather has
multiple clock outputs that are a scale of the input clock.  The node
for that device would never have a clock-frequency property because it
relies entirely on the upstream clock.

This proposed binding is only about one thing: attaching clock
providers to clock consumers.  It doesn't make any statements about
how the clock provider driver is organized or how it manages its
clocks.

This is by design to prevent clock consumers from digging into a clock
provider node manually without consulting its driver.  The consumer
only has a reference to a clock provider node + arguments.  It is the
job of the producer driver to interpret the arguments.

The proposed API also reflects this.  The consumer driver requests a
clock, but it is the producer driver that determines what the clock
reference actually means, and which struct clk to return.

> >>I think it will be better to define a clock node as a single source
> >>of clock. If several outputs are needed, then we should define
> >>several clock nodes.

That is entirely up to the binding for the specific clock device(s).
If it makes sense to have one node per clock, so be it.  If it makes
more sense to have a clock nexus node for other hardware, then that is
fine too.

> >>If we let a clock node be any kind of big clock blob, we will never
> >>be able to define some generic reusable clock node API. Everybody
> >>will define its own custom clock blobs.
> >
> >Generally +1. I asked myself the same question whether it's a good thing
> >to allow a node to have multiple clocks. For i.MX I can say that I don't
> >need this and that I do not intend to use this feature. Grants concerns
> >about this are that the clock part of the device tree might explode when
> >we put each and every clock into the devicetree, so he wants to allow
> >bigger blobs of multiple clocks.
> 
> I can clearly understand this concern, because in the case of OMAP4,
> we will have to add ~250 nodes if we want to define the current
> clock tree.

I'm really not convinced that it is a good idea to break out the
entire clock tree into one node per struct clk.  To begin with, that
looks to be very centric around the current 'struct clk' Linux
abstraction, which is potentially in flux.  Also, looking at Sascha's
initial RFC for describing the clock tree, I see cases where it looks
like a clock nexus node really makes sense.  For instance, the
'divider-ipg@...3fd4014' node which has a list of child nodes
which merely provide a register offset and shift value
(reg=0x53fd4068..0x53fd4084, shift=0x0..0xf).  It would be natural to
instead encode that as part of the clock reference, or map it directly
from the clock reference (ie, assign names to each of the clocks, and
let the clock provider driver match up the name to the reg offset &
shift values).

I had originally thought that it would be better to use names directly
for references to clocks (ie. clock = <phandle>,"name") , but after
actually playing with it and looking at the existing DT conventions,
I've reverted to cell values for the arguments and a separate set of
clock-{input,output}-name properties for attaching meaningful names,
just like we decided to do for assigning names to 'reg' properties.

> >My impression gets more and more that we either put the clock tree in the
> >devicetree or we do not do it, but there's not much room in between.

I disagree here entirely because there is a lot of room in between.
Drivers still need to understand the hardware block they are driving,
and there will still be a lot of hardware-specific knowledge encoded
into clock provider drivers (just as there is for every other kind of
driver).  The trick (and even artistry) is to determine the ideal
balance between hard coding static hardware details and
parameterization of interconnects between blocks.

> 
> If we are not ready to accept 4k lines of clock data inside the
> devicetree, which is indeed huge and mostly useless, then I think we
> should just expose the leaf clocks. But then they will depend on
> parent that will not be in DT but in static files, which might be
> weird.
> 
> I'm not sure what the right answer is :-(

I think you're right.  Expose the leaf clocks, and maybe the
interconnects between clock providers.  There is no need to expose
every single intermediary clock, especially when those clocks are
merely internal to a device IP block and are never exposed to
anything outside of itself.

I'm even okay with having a single clock driver for an entire SoC if
there isn't a very high likelyhood of any of it's internal clock
components will ever get reused somewhere else.  When compared with
board designs, internal SoC clock structure really doesn't change much
at all so there isn't the same need for breaking all the detail out to
the device tree.

Oh, and it shouldn't be weird because even clock nexus nodes that
handle several clock outputs can have clock inputs from other sources.
For instance, there could be a node for the clock provided by the
on-board crystal.

g.

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

Powered by blists - more mailing lists