On Wed, 2009-08-12 at 22:44 +0100, Mark Brown wrote:
On Thu, Aug 13, 2009 at 07:34:07AM +1000, Benjamin Herrenschmidt wrote:
quoted
On Wed, 2009-08-12 at 13:35 +0100, Mark Brown wrote:
quoted
quoted
What happens if another clock gets added or the list gets reordered for
some reason?
quoted
The device-tree is mostly static in that regard. I'm not sure what you
mean. Clocks are referenced by IDs so if you want to add more clocks you
can just add them to the end. The "NULL" case is typically for things
that have only one clock (which seems to be reasonably common in ARM
land).
The issue I'm worrying about here is what happens if the device has only
one clock in current revisions of the hardware but another revision of
the same hardware is produced which adds another clock. When that
happens your NULL lookup gets confused.
Well, then don't use clk_get() with NULL :-) I mean, ARM devices do that
today (ie, calling with NULL), it makes sense in the context of a
device-tree binding to provide a defined behaviour for it (pick the
first clock in the order it appears in the device-tree is one way to do
it), but we can also recommend to driver writers not to do it :-)
But if one was to use an existing driver that does it, then we need to
cater for it.
Note that adding a new clock in the future isn't a big deal. As long as
the first one in the list is the one the driver that calls clk_get with
NULL expects to obtain.
Maybe we can make clock-names non-optional though in the DT as an
incentive not to use the simple 1-clock "NULL" path.
quoted
I think it's reasonable to ask whoever produces the device-tree to keep
the two properties in sync.
It's not just the device tree, it's also the drivers which have to be
able to cope with whatever random device tree that's thrown at them.
Well, the clocks are named. At some stage, the binding for a given
device will define what clock names it expects. I don't see that
differing from what the ARM folks do.
This is less of a problem on ARM where it's fairly straightforward to
adjust the users at the same time as the driver but if you've got a
device tree delivered via a completely different distribution mechanism
it gets more sticky.
Possibly yes, and that's a reason why we tend to discourage that ;-) But
again, it boils down to settling with a naming convention for a given
device. If clocks are added later on, the driver will have to cope with
the new clocks not existing, of course, or the platform can cater for
it.
That's also one of the reason why I want to make clk_get() go through
ppc_md. first, so that the platform can always override the behaviour,
and for example, fill-in in case the device-tree is incomplete or
incorrect.
The problem with the API at the minute in this regard is that there is
no standard way of registering new clocks. Only something that knows
about the magic sauce for a given architecture (if any) is able to add
clocks. My concern here is that if PowerPC moves in this direction
without some general agreement from elsewhere then there may be problems
for drivers.
Well, the idea is to allow to register clock-providers associated with
device nodes. I think we should keep the rest in the hand of platforms
for now, and possibly make it evolve later.
Do you have maybe a precise example scenario where your above statement
about the lack of facility for registering new clocks is a problem ? I'm
curious to see a real life example so I can think better about how it
can be solved (or whether it needs to be solved).
Cheers,
Ben.
On Thu, Aug 13, 2009 at 07:56:32AM +1000, Benjamin Herrenschmidt wrote:
Maybe we can make clock-names non-optional though in the DT as an
incentive not to use the simple 1-clock "NULL" path.
Yeah, that was more what I was thinking - apply some pressure on people
not to use the NULL clock feature for the device tree stuff.
Possibly yes, and that's a reason why we tend to discourage that ;-) But
again, it boils down to settling with a naming convention for a given
device. If clocks are added later on, the driver will have to cope with
the new clocks not existing, of course, or the platform can cater for
it.
...which is much easier if you discourage people from using the NULL
name in the first place :) My concern is more about new device tree and
older driver code than the other way round (which wouldn't suprise me,
if only during things like bisection).
Do you have maybe a precise example scenario where your above statement
about the lack of facility for registering new clocks is a problem ? I'm
curious to see a real life example so I can think better about how it
can be solved (or whether it needs to be solved).
There was a recent thread on linux-kernel (last week) about the tmio_mmc
drivers - it's a MMC controller which is present in both some SH CPUs
and some MFD chips. I can probably dig up a more exact reference if
required.
Probably you will be able to, like the ARMs have, get a very long way
with just supporting the on-SoC clock tree just now and can punt on
dealing with other things for now. It's where a large part of the
interesting clocking in a lot of embedded systems is.
On Thu, Aug 13, 2009 at 07:56:32AM +1000, Benjamin Herrenschmidt wrote:
Maybe we can make clock-names non-optional though in the DT as an
incentive not to use the simple 1-clock "NULL" path.
We used to pass names. Everyone got the idea that they could ignore
the struct device argument, and chaos ensued in drivers - people wanted
to name each of their individual clk structures uniquely, and pass
clock names, or even struct clk pointers into drivers via platform data.
Some drivers conditionalized the clock name depending on the SoC they
were built for in the driver code.
Providing the clkdev infrastructure (which I'll talk about in another
email, probably tomorrow) and ensuring that single-clock drivers pass
a NULL name has ensured that people back away from that broken kind
of thinking. It has certainly cut down on the code size and the
complexity in drivers.
IIRC, there were some drivers shrunk by about 100 LOC by using the
clk API as I originally intended it to be used - which clkdev
facilitates.
quoted
It's not just the device tree, it's also the drivers which have to be
able to cope with whatever random device tree that's thrown at them.
Well, the clocks are named. At some stage, the binding for a given
device will define what clock names it expects. I don't see that
differing from what the ARM folks do.
The difference is that I'm trying to avoid the "name each clock source
and have each driver ask for the clock by name". Such an approach
at first seems simple and logical, but experience has shown that it
eventually creates more problems as things progress.
Take a look at these two commits:
39a80c7f379e1c1d3e63b204b8353b7381d0a3d5
4c5e1946b5f89c33e3bc8ed73fa7ba8f31e37cc5
to see how moving from a per-clk naming system to a dev+consumer naming
allowed omap_wdt to be cleaned up. (OMAP3 added more clk naming
conditions in the driver, so had this cleanup not happened the driver
would have more stuff in it.)
What I'm saying is that always passing a bunch of names has been well
proven to lead people down the wrong path of matching only by names
and then running into problems later. We need drivers passing a NULL
name to ensure that people get the right idea. Comments in code/headers
don't seem to work. ;(
--
Russell King
On Wed, Aug 12, 2009 at 11:28:43PM +0100, Russell King wrote:
We used to pass names. Everyone got the idea that they could ignore
the struct device argument, and chaos ensued in drivers - people wanted
to name each of their individual clk structures uniquely, and pass
clock names, or even struct clk pointers into drivers via platform data.
Some drivers conditionalized the clock name depending on the SoC they
were built for in the driver code.
Yes, that sort of stuff is obviously crazy - you just end up with more
code to pass the name/pointer around than you have to register things at
init time.
What I'm saying is that always passing a bunch of names has been well
proven to lead people down the wrong path of matching only by names
and then running into problems later. We need drivers passing a NULL
name to ensure that people get the right idea. Comments in code/headers
don't seem to work. ;(
I always suspected half the problem with people getting the wrong idea
is that having to implement the lookup and mapping stuff (which clkdev
now provides) seemed like too much work.