ARM clock API to PowerPC

ARM clock API to PowerPC

[Benjamin Herrenschmidt]

Hi folks !

(Russell, let us know if you want to dropped from the CC list, but I
felt you may have some useful input to provide here).

So I think it would be valuable to provide the ARM clock API exposed by
include/linux/clk.h on various PowerPC embedded hardware.

However, that brings various issues that I would like to discuss here,
as while the API exposed to driver is quite sane and something we might
want to keep mostly intact, I'm not too happy with what appears to be
the common practice on ARM which is to re-implement the whole API on
each machine type :-) I understand why they do that but that won't work
very well for us.

That leads to two major "items" to deal with basically:

 - What provides the implementation of clk_get & friends
 - How do we want (do we want at all ?) to use the device-tree to
   some extent to represent the clocks

Now, here are some ideas, mostly in random orders, that I'm throwing
to the list for discussion, and from there we might see a plan emerge
(or not).

 - Similar to what I did for interrupts, the core clk API implementation
(if there is such a thing) should not -require- some device-tree
representation or another, ie, we could and probably should provide
helpers, for example by allowing clk_get to use the device-tree to
lookup clocks (see discussions below) etc... but the base infrastructure
should work without and it should be possible (even if discouraged) for
a platform to provide the necessary stuff entirely from C code, like we
can create virq mappings without device-tree.

 - We aren't be able to cater for all possible clock net layouts with a
single "core". Thus I think we should provide a "think" layer. The clk
object should probably have methods and clk_get_rate etc.. end up
calling these. Thus the "core" basically boils down to finding a clock
object for a given device. IE, Implementing clk_get(), which thus could
become a ppc_md. callback. We can provide a default one that uses the
device-tree to some extent, do be discussed.

 - I looked at ARMs' mach-ns9xxx, which pretty much does the above
(without the ppc_md. bit) and I tend to think that maybe this would do a
nice base for a "core". I do wonder whether we should have the "ops" in
a separate struct so that it's easier for multiple instances of a clk to
share ops (when multiple clocks come from the same clock chip).

 - From the above, question: Do we want to keep that parent pointer ?
Does it make sense ? Will we have objects that are clock providers and
themselves source from multiple parent ? Or we don't care and it becomes
entirely the responsibility of a given struct clk instance to deal with
its own parenthood ? Parenthood in the core has the advantage of making
it potentially easier to represent clock nets in the device-tree.

 - Device-tree: The idea on top of my mind would be to define a
clock-map property that has the following format:

A list of:
	- zero terminated string clock ID, padded with zeros
          to a cell boundary
 	- a phandle to the clock provider
	- a numerical (string ?) ID for that clock within that provider

The core would thus be able to do a search in that list based on the
clock-id passed in, or if clk_get(dev, NULL), then, use the first one.

A bit like irq_hosts, we provide a way to register clock providers
associated with a device-node. They provide a matching clk_get()
routine, thus when clk_get is called, the above lookup is done in the
clock-map, the clock provider is found, and it's own clk_get() method is
called with the original ID, the numerical ID, the target device, all it
might need to sort things out.

That's a reasonably simple way. I was thinking about something more
fancy like interrupts, with a #clock-cells, clocks, clock-parent
proprerties etc... but that has several drawbacks and isn't that useful.
One drawback is that like interrupts, it's hard to use such a scheme
with more than one parent for a device, and that's a lot more common
with clocks. Also, the existing API uses string clock IDs while the
above would have worked well only with numerical IDs.

Anyways, you guys give it a good thought and we see how it goes, it
shouldn't be a big deal, what do you think ?

Note that clock providers can be any node, so they can themselves be
of_devices that are naturally probed as such. We of course end up with
the usual problem of who gets registered first, since obviously clock
providers need to come in before they are looked up, and so I might
suggest that we stick in there a clock-provider property with no value,
and we have a way to return a specific error code to the driver if there
seem to be a DT clock provider but nothing registered for it yet, but
that adds some complication.

Maybe threaded probe can get us out of it by having one guy wait and the
probe continue until the clock provider is there :-) Or in the meantime
we just manually register them from arch code... whatever.

Also, it would be nice to have a way to have "generic" clock provider
drivers. For example, have a driver for the FooBar Clock chip, which is
known to provide 4 fully programmable clocks. So there could be a
generic driver for that, attached to the clock provider by the probe
code or via the device-tree, the devices clock-map's just provide the
clock ID within the chip (which output of the chip they are connected
to), and so the remaining questions is what to program in each clocks.

That's where we may want to define a clock-rates property that the clock
provider uses. HOWEVER, I'm keen at first to make that device-specific
rather than try to go to far at generalizing. Clock ships are fancy
little things with all sort of weird issues and constraints, so while it
would be nice to have all the necessary infos to program it in the DT,
I'm tempted to make the 'format' of those specific to a given clock chip
device/driver combination. To be discussed.

Anyway, thoughts ?

Cheers,
Ben.

Re: ARM clock API to PowerPC

[Benjamin Herrenschmidt]

On Wed, 2009-08-12 at 17:57 +1000, Benjamin Herrenschmidt wrote:

>  - Device-tree: The idea on top of my mind would be to define a
> clock-map property that has the following format:
> 
> A list of:
> 	- zero terminated string clock ID, padded with zeros
>           to a cell boundary
>  	- a phandle to the clock provider
> 	- a numerical (string ?) ID for that clock within that provider
> 
> The core would thus be able to do a search in that list based on the
> clock-id passed in, or if clk_get(dev, NULL), then, use the first one.

Thinking a bit more about that one, mixing strings and numbers in a
property sucks. What about instead:

 clock-map is a list of phandle, id

 clock-names is an optional list of 0 terminated strings

If there's only one clock, and the ID can be ommited, then the
clock-names property can be ommited completely too. Else, the
entries in clock-names match the entries in clock-map.

It's a bit strange to separate the list into two properties but
I think it will generally suck less than having them mixed, especially
with ASCII representations such as lsprop output.

Cheers,
Ben

Re: ARM clock API to PowerPC

[Mitch Bradley]

>
> On Wed, 2009-08-12 at 17:57 +1000, Benjamin Herrenschmidt wrote:
>
>   
>> >  - Device-tree: The idea on top of my mind would be to define a
>> > clock-map property that has the following format:
>> > 
>> > A list of:
>> > 	- zero terminated string clock ID, padded with zeros
>> >           to a cell boundary
>>     


Padding a string violates a core principle of property representation, 
namely the "no alignment assumptions" one.  The reason for that 
principle was the fact that alignment needs are not stable across 
processor families, or even within a processor family .

>> >  	- a phandle to the clock provider
>> > 	- a numerical (string ?) ID for that clock within that provider
>> > 
>> > The core would thus be able to do a search in that list based on the
>> > clock-id passed in, or if clk_get(dev, NULL), then, use the first one.
>>     
>
> Thinking a bit more about that one, mixing strings and numbers in a
> property sucks. What about instead:
>
>  clock-map is a list of phandle, id
>
>  clock-names is an optional list of 0 terminated strings
>   

This approach finesses the problem nicely.

> If there's only one clock, and the ID can be ommited, then the
> clock-names property can be ommited completely too. Else, the
> entries in clock-names match the entries in clock-map.
>
> It's a bit strange to separate the list into two properties but
> I think it will generally suck less than having them mixed, especially
> with ASCII representations such as lsprop output.
>
> Cheers,
> Ben
>   

Re: ARM clock API to PowerPC

[Benjamin Herrenschmidt]

On Wed, 2009-08-12 at 07:31 -1000, Mitch Bradley wrote:

> Padding a string violates a core principle of property representation, 
> namely the "no alignment assumptions" one.  The reason for that 
> principle was the fact that alignment needs are not stable across 
> processor families, or even within a processor family .

Right, which is why I replied to myself with a different idea :-)

> This approach finesses the problem nicely.

Thanks.

Cheers,
Ben.

Re: ARM clock API to PowerPC

[Josh Boyer]

On Wed, Aug 12, 2009 at 05:57:05PM +1000, Benjamin Herrenschmidt wrote:
>Hi folks !
>
>(Russell, let us know if you want to dropped from the CC list, but I
>felt you may have some useful input to provide here).
>
>So I think it would be valuable to provide the ARM clock API exposed by
>include/linux/clk.h on various PowerPC embedded hardware.

I'm probably being particularly dense, and I haven't had my coffee yet this
morning, but you never actually said _why_ you think this is valuable?

I'm a bit confused as to what problem you are trying to solve with the API.

josh

Re: ARM clock API to PowerPC

[Kumar Gala]

On Aug 12, 2009, at 6:19 AM, Josh Boyer wrote:

> On Wed, Aug 12, 2009 at 05:57:05PM +1000, Benjamin Herrenschmidt  
> wrote:
>> Hi folks !
>>
>> (Russell, let us know if you want to dropped from the CC list, but I
>> felt you may have some useful input to provide here).
>>
>> So I think it would be valuable to provide the ARM clock API  
>> exposed by
>> include/linux/clk.h on various PowerPC embedded hardware.
>
> I'm probably being particularly dense, and I haven't had my coffee  
> yet this
> morning, but you never actually said _why_ you think this is valuable?
>
> I'm a bit confused as to what problem you are trying to solve with  
> the API.

I'm in the same boat as you Josh.  I think there is value and utility  
here but not sure what problem Ben's trying to solve.

- k

Re: ARM clock API to PowerPC

[Benjamin Herrenschmidt]

On Wed, 2009-08-12 at 08:40 -0500, Kumar Gala wrote:

> I'm in the same boat as you Josh.  I think there is value and utility  
> here but not sure what problem Ben's trying to solve.

Well, there's several things here. 

First, it would be nice to improve clock management on some existing
SoCs. I'm not going to rewrite the powermac clock handling though :-)
But it might allow to cleanup some hacks we have there or add dynamic
clock handling in areas where we could improve power consumption etc...
by doing it but aren't today. Not critical but it's nice to have the
base layout there.

Don't you have parts where you'd like some more dynamic clock
management ? This API is a reasonably simple and nice way to get there.

Now, I know there is at least one person on earth contemplating sharing
some drivers between PPC and ARM. I won't tell much more at this stage,
but it makes sense in the grand scheme of things to see SoC vendors put
similar IO cores into either PPC or ARM and providing that clock API is
a good way to also allow these drivers to work since the drivers in
questions make use of it.

Also, note that the clk API is in include/linux/clk.h, ie, it's not
really ARM specific anymore in that sense, it's also very simple (it
leaves a lot to the implementation which is good) and thus my proposal
resolves in very little support code for us (mostly the clk_get()
implementation that hooks into ppc_md, the wrappers to call the
"methods" of struct clk, and the helper to parse the OF bits).

Cheers,
Ben.

RE: ARM clock API to PowerPC

[Li Yang-R58472]

>Now, I know there is at least one person on earth 
>contemplating sharing some drivers between PPC and ARM. I 
>won't tell much more at this stage, but it makes sense in the 
>grand scheme of things to see SoC vendors put similar IO cores 
>into either PPC or ARM and providing that clock API is a good 
>way to also allow these drivers to work since the drivers in 
>questions make use of it.

Freescale USB UDC driver is another example that shared between PowerPC
and ARM(i.mx).  Currently, the imx part of the driver uses clk API, but
PowerPC part uses static initialization.  It will be better if we can
unify the clk setting part of the driver.

- Leo

RE: ARM clock API to PowerPC

[Benjamin Herrenschmidt]

On Thu, 2009-08-13 at 16:59 +0800, Li Yang-R58472 wrote:
> >Now, I know there is at least one person on earth 
> >contemplating sharing some drivers between PPC and ARM. I 
> >won't tell much more at this stage, but it makes sense in the 
> >grand scheme of things to see SoC vendors put similar IO cores 
> >into either PPC or ARM and providing that clock API is a good 
> >way to also allow these drivers to work since the drivers in 
> >questions make use of it.
> 
> Freescale USB UDC driver is another example that shared between PowerPC
> and ARM(i.mx).  Currently, the imx part of the driver uses clk API, but
> PowerPC part uses static initialization.  It will be better if we can
> unify the clk setting part of the driver.

I had a look at it looks like it uses the API in a way that would fit
nicely with my plans, ie, it should be possible to use the same driver
on both archs pretty much without changes provided the ppc platform
provides a clock source driver and hooks it up to the device-tree.

I'll work on some proof-of-concept implementation of the core bits
early next week.

Cheers,
Ben.

RE: ARM clock API to PowerPC

[Guennadi Liakhovetski]

On Fri, 14 Aug 2009, Benjamin Herrenschmidt wrote:

> On Thu, 2009-08-13 at 16:59 +0800, Li Yang-R58472 wrote:
> > >Now, I know there is at least one person on earth 
> > >contemplating sharing some drivers between PPC and ARM. I 
> > >won't tell much more at this stage, but it makes sense in the 
> > >grand scheme of things to see SoC vendors put similar IO cores 
> > >into either PPC or ARM and providing that clock API is a good 
> > >way to also allow these drivers to work since the drivers in 
> > >questions make use of it.
> > 
> > Freescale USB UDC driver is another example that shared between PowerPC
> > and ARM(i.mx).  Currently, the imx part of the driver uses clk API, but
> > PowerPC part uses static initialization.  It will be better if we can
> > unify the clk setting part of the driver.
> 
> I had a look at it looks like it uses the API in a way that would fit
> nicely with my plans, ie, it should be possible to use the same driver
> on both archs pretty much without changes provided the ppc platform
> provides a clock source driver and hooks it up to the device-tree.
> 
> I'll work on some proof-of-concept implementation of the core bits
> early next week.

You might have a look at these threads:

http://marc.info/?t=124876760500001&r=1&w=2
http://marc.info/?t=124782904600005&r=2&w=2

but since they are quite long, in short, in them a patch has been 
discussed, that allowed to re-use an MMC driver, used on some MFDs, on 
SuperH SoCs. The patch was taking the "easy route" of adding the 
possibility to use the clock API to the tmio_mmc.c driver, while leaving 
it to use static clock configurations with MFD drivers. This approach has 
been rejected and initially it has been suggested to implement a 
platform-independent clock API like what had been proposed by clocklib, 
but since the future of clocklib is unclear, it has then been decided to 
remove the clock (and power) management from the driver proper and move 
them to some callbacks. I.e., there would be more users interested in a 
unified clock API, including other platforms and platform-independent 
drivers like MFD. Currently the reason, why MFD drivers cannot implement 
their own clock devices is that the "struct clk" differs between 
platforms.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

RE: ARM clock API to PowerPC

[Benjamin Herrenschmidt]

On Fri, 2009-08-14 at 13:29 +0200, Guennadi Liakhovetski wrote:
> but since they are quite long, in short, in them a patch has been 
> discussed, that allowed to re-use an MMC driver, used on some MFDs, on 
> SuperH SoCs. The patch was taking the "easy route" of adding the 
> possibility to use the clock API to the tmio_mmc.c driver, while leaving 
> it to use static clock configurations with MFD drivers. This approach has 
> been rejected and initially it has been suggested to implement a 
> platform-independent clock API like what had been proposed by clocklib, 
> but since the future of clocklib is unclear, it has then been decided to 
> remove the clock (and power) management from the driver proper and move 
> them to some callbacks. I.e., there would be more users interested in a 
> unified clock API, including other platforms and platform-independent 
> drivers like MFD. Currently the reason, why MFD drivers cannot implement 
> their own clock devices is that the "struct clk" differs between 
> platforms.

But there is no reason for it to differ !

My idea is that struct clock would contain function pointers for the
enable/disable/get_rate/ etc... methods

Thus it's up to clk_get() to provide an object with the right pointers.

Now, on ARM, it's currently done in such a way that it's mostly up to
the platform (though that's less true with clkdev).

But with the help of the device-tree, it becomes trivial to have
somebody register clock providers (ie, objects that can product struct
clk *) and bind them to driver.

I think struct clk is the way to go. The problem is to sort out the
binding between the clock provider and the driver. The DT is an easy and
nice way to do it for archs that have it. But there are other ways.

Cheers,
Ben.

Re: ARM clock API to PowerPC

[Russell King]

On Fri, Aug 14, 2009 at 10:07:44PM +1000, Benjamin Herrenschmidt wrote:
> My idea is that struct clock would contain function pointers for the
> enable/disable/get_rate/ etc... methods

If you look at OMAP, doing that gets very expensive, both in terms of
number of lines of code, size of structure and maintainence thereof.
Neither does a 'clk_ops' structure containing all of the function
pointers work either for OMAP (OMAP has such a structure just for
enable and disable methods, of which there are about two or three to
chose from, but the rounding, set_rate and propagation methods are
per-clk.  This balance seems to work well for OMAP.)

FYI, there are 140 struct clk definitions for OMAP24xx, and 215 for
OMAP34xx, all statically initialized.  See arch/arm/mach-omap2/clock?4xx.h

-- 
Russell King

Re: ARM clock API to PowerPC

[Benjamin Herrenschmidt]

On Sat, 2009-08-15 at 13:43 +0100, Russell King wrote:
> On Fri, Aug 14, 2009 at 10:07:44PM +1000, Benjamin Herrenschmidt wrote:
> > My idea is that struct clock would contain function pointers for the
> > enable/disable/get_rate/ etc... methods
> 
> If you look at OMAP, doing that gets very expensive, both in terms of
> number of lines of code, size of structure and maintainence thereof. 

Well, it depends. Having the function pointer or ops structure allows
you to use different function pointers/ops per clock ... but doesn't
force you to do so :-)

For example, in the OMAP case, you could just have one struct
omap_soc_clk_ops for example that contains the existing implementations
going through the existing table etc... 

But having the func. pointers allow designers to still use struct clk
for other clock sources if they wish to, which is pretty much precluded
if they link straight into the arch.

This cover two things: external clock sources (i2c PLLs etc...) used to
clock devices, and big combo-devices that have their own sets of clocks
and subsystems that may want to use the clk API separately from the main
SoC and platform code. 

For ppc we really don't have much of a choice here anyway because we
support multiple platforms compiled in the same kernel as long as they
have a CPU core that's the same overall family, and that can be very
wide. For example, 440-type cores can exist in all sort of IBM/AMCC
cores, but also Xilinx FPGAs, and when you start saying FPGA the
possibilities go wild :-)

> Neither does a 'clk_ops' structure containing all of the function
> pointers work either for OMAP (OMAP has such a structure just for
> enable and disable methods, of which there are about two or three to
> chose from, but the rounding, set_rate and propagation methods are
> per-clk.  This balance seems to work well for OMAP.)
> 
> FYI, there are 140 struct clk definitions for OMAP24xx, and 215 for
> OMAP34xx, all statically initialized.  See arch/arm/mach-omap2/clock?4xx.h

I had a look, and it doesn't appear to be a huge deal either way. In the
relatively minor detail as to use fun. pointers vs, ops, the func
pointers may win the OMAP case becaues ops would require pretty much
allocating & populating a new ops structure for each clock ... unless
you have one standard one whose set_rate that calls back into the table
via another function pointer ;-) but then it depends how hot those code
path are.

I might initially go with flat function pointers in struct clk on ppc
for now, and we'll see how things pan out.

Cheers,
Ben.

Re: ARM clock API to PowerPC

[Grant Likely]

On Sat, Aug 15, 2009 at 4:18 PM, Benjamin
Herrenschmidt<benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org> wrote:
> For ppc we really don't have much of a choice here anyway because we
> support multiple platforms compiled in the same kernel as long as they
> have a CPU core that's the same overall family, and that can be very
> wide. For example, 440-type cores can exist in all sort of IBM/AMCC
> cores, but also Xilinx FPGAs, and when you start saying FPGA the
> possibilities go wild :-)

Yes, exactly!  In fact, FPGAs are somewhat nicer in that only the
hardware actually needed is present on the running system (fewer data
instances), but the flip side is that the set of instances changes at
the whim of the FPGA engineer.  Static definition isn't an option
unless you want to change the platform source code for each new FPGA
bistream revision.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: ARM clock API to PowerPC

[Mark Brown]

On Wed, Aug 12, 2009 at 05:57:05PM +1000, Benjamin Herrenschmidt wrote:

>  - From the above, question: Do we want to keep that parent pointer ?
> Does it make sense ? Will we have objects that are clock providers and
> themselves source from multiple parent ? Or we don't care and it becomes

That happens and at times one or more of the sources is off-chip.

> entirely the responsibility of a given struct clk instance to deal with
> its own parenthood ? Parenthood in the core has the advantage of making
> it potentially easier to represent clock nets in the device-tree.

> The core would thus be able to do a search in that list based on the
> clock-id passed in, or if clk_get(dev, NULL), then, use the first one.

What happens if another clock gets added or the list gets reordered for
some reason?

> Also, it would be nice to have a way to have "generic" clock provider
> drivers. For example, have a driver for the FooBar Clock chip, which is
> known to provide 4 fully programmable clocks. So there could be a
> generic driver for that, attached to the clock provider by the probe
> code or via the device-tree, the devices clock-map's just provide the
> clock ID within the chip (which output of the chip they are connected
> to), and so the remaining questions is what to program in each clocks.

Note that the present ARM implementations don't handle anything except
the core SoC normally.

Re: ARM clock API to PowerPC

[Benjamin Herrenschmidt]

On Wed, 2009-08-12 at 13:35 +0100, Mark Brown wrote:
> On Wed, Aug 12, 2009 at 05:57:05PM +1000, Benjamin Herrenschmidt wrote:
> 
> >  - From the above, question: Do we want to keep that parent pointer ?
> > Does it make sense ? Will we have objects that are clock providers and
> > themselves source from multiple parent ? Or we don't care and it becomes
> 
> That happens and at times one or more of the sources is off-chip.

Right. That's somewhat what I thought. I think the best approach
initially is not to impose a hierarchy in our "core" and keep that to
the actual clock provider implementation. We'll see in the long term
with practice whether we then want to move some of that back into core.

> What happens if another clock gets added or the list gets reordered for
> some reason?

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).

I think it's reasonable to ask whoever produces the device-tree to keep
the two properties in sync.

> Note that the present ARM implementations don't handle anything except
> the core SoC normally.

Ok, interesting. I don't see why the API would prevent going further
tho. In any case, that isn't a big deal, at least until somebody proves
me wrong, I believe what I proposed remains simple enough and leaves
enough to the actual implementations to allow pretty much anything in
that area.

Cheers,
Ben.

Re: ARM clock API to PowerPC

[Mark Brown]

On Thu, Aug 13, 2009 at 07:34:07AM +1000, Benjamin Herrenschmidt wrote:
> On Wed, 2009-08-12 at 13:35 +0100, Mark Brown wrote:

> > What happens if another clock gets added or the list gets reordered for
> > some reason?

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

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

> > Note that the present ARM implementations don't handle anything except
> > the core SoC normally.

> Ok, interesting. I don't see why the API would prevent going further
> tho. In any case, that isn't a big deal, at least until somebody proves
> me wrong, I believe what I proposed remains simple enough and leaves
> enough to the actual implementations to allow pretty much anything in
> that area.

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.

Re: ARM clock API to PowerPC

[Benjamin Herrenschmidt]

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:
> > On Wed, 2009-08-12 at 13:35 +0100, Mark Brown wrote:
> 
> > > What happens if another clock gets added or the list gets reordered for
> > > some reason?
> 
> > 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.

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

Re: ARM clock API to PowerPC

[Mark Brown]

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.

Re: ARM clock API to PowerPC

[Benjamin Herrenschmidt]

On Wed, 2009-08-12 at 23:20 +0100, Mark Brown wrote:

> ...which is much easier if you discourage people from using the NULL
> name in the first place :)  

Agreed.

> 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).

Right. That would only be a problem with NULL name -and- the new
device-tree changing the first clock in the list instead of adding to
the end, but I see your point, and it's a valid concern.

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

Or maybe just explain quickly how it needs to "register new clocks" in
ways that can be problematic. I'm not trying to be dense, I'm really not
sure what the problem you are trying to highlight is :-)

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

Right. And having the platform able to always trump the device-tree will
allow for hacks if necessary. If they start growing out of control, that
will tell us that we need to do something differently :-)

I'm afraid of over-design, so I'd rather go for something minimalist yet
flexible, which seems to be the case here, as the actual clock provider
implementation and platform code have pretty much all latitude to do
whatever they want.

Having more "generic" clock providers for off-SoC clock chips is an idea
that went through my mind but you may be right that it's not necessarily
something we need to cater for initially, it can be handled by platform
for now easily enough.

Cheers,
Ben.

Re: ARM clock API to PowerPC

[Mark Brown]

On Thu, Aug 13, 2009 at 08:32:53AM +1000, Benjamin Herrenschmidt wrote:
> On Wed, 2009-08-12 at 23:20 +0100, Mark Brown wrote:

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

> Or maybe just explain quickly how it needs to "register new clocks" in
> ways that can be problematic. I'm not trying to be dense, I'm really not
> sure what the problem you are trying to highlight is :-)

The problem is that you've got a chip which has a clock tree of its own
which could benefit from using the clock API internally (in this case
because it helps generalisation to the case where it's on the CPU for
the MMC block to be able to just use the clock API for its clocks).

Ideally the MFD core for the tmio would be able to extend the clock tree
so that the MMC driver can work without knowing what sort of device it's
part of.  Having the platform know about the clocks in the MFD means
teaching each platform that might use the chip about the clocking
structure of the chip in some way.  However, there's a concern about
making the clock API too heavyweight for the on-SoC clocks that are the
major application.  Things like per-clock memory consumption are an
issue on bigger chips.

> Having more "generic" clock providers for off-SoC clock chips is an idea
> that went through my mind but you may be right that it's not necessarily
> something we need to cater for initially, it can be handled by platform
> for now easily enough.

So long as that's clear to device tree users that should be fine.

Re: ARM clock API to PowerPC

[Benjamin Herrenschmidt]

> The problem is that you've got a chip which has a clock tree of its own
> which could benefit from using the clock API internally (in this case
> because it helps generalisation to the case where it's on the CPU for
> the MMC block to be able to just use the clock API for its clocks).

I see.

> Ideally the MFD core for the tmio would be able to extend the clock tree
> so that the MMC driver can work without knowing what sort of device it's
> part of.  Having the platform know about the clocks in the MFD means
> teaching each platform that might use the chip about the clocking
> structure of the chip in some way.  However, there's a concern about
> making the clock API too heavyweight for the on-SoC clocks that are the
> major application.  Things like per-clock memory consumption are an
> issue on bigger chips.

Right. Well, my proposal of linkage of clock providers via the
device-tree would definitely solve that problem for us at least,
provided the various parts of the chip are represented as nodes in the
DT.

> > Having more "generic" clock providers for off-SoC clock chips is an idea
> > that went through my mind but you may be right that it's not necessarily
> > something we need to cater for initially, it can be handled by platform
> > for now easily enough.
> 
> So long as that's clear to device tree users that should be fine.

That's my thought too.

Cheers,
Ben.

Re: ARM clock API to PowerPC

[Russell King]

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.

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

Re: ARM clock API to PowerPC

[Mark Brown]

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.

Re: ARM clock API to PowerPC

[Benjamin Herrenschmidt]

On Wed, 2009-08-12 at 23:28 +0100, Russell King wrote:

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

Hi Russell ! Thanks a lot for your feedback.

Ok. So I may have misunderstood what names were for. In my mind, those
were the name of the clock input on the HW device :-) Hence my
clock-map, which maps a clock input to a clock provider. IE. It was all
to be taken as a tuple (device,name) that defines a given clock input on
a given device.

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

Ok, thanks, I need to read up on clkdev then, I've missed that bit.

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

Ok.

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

Right. I didn't intend to name the clock sources. I intended to name the
clock -inputs- of a given device. IE. clk_get(dev, name) in my mind
meant "give me the clock provider that feeds my "name" input).

> Take a look at these two commits:
> 
> 39a80c7f379e1c1d3e63b204b8353b7381d0a3d5
> 4c5e1946b5f89c33e3bc8ed73fa7ba8f31e37cc5

Thanks, I will.

> 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. ;(

Allright but passing a NULL doesn't help for drivers with multiple clock
inputs. IE. How do you want to deal with that ? Do you want to deprecate
the named API and instead provide a new clk_get_for_input(dev,
clk_input) (clk_input could be name or numerical ... tbd) ?

Or am I missing a piece of the puzzle ?

Cheers,
Ben.

Re: ARM clock API to PowerPC

[Russell King]

On Thu, Aug 13, 2009 at 08:52:49AM +1000, Benjamin Herrenschmidt wrote:
> On Wed, 2009-08-12 at 23:28 +0100, Russell King wrote:
> 
> > 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.
> 
> Hi Russell ! Thanks a lot for your feedback.
> 
> Ok. So I may have misunderstood what names were for. In my mind, those
> were the name of the clock input on the HW device :-)

Oh, I do hope I didn't say that was wrong, because that is quite
correct.  What the idea with passing a NULL 'name' with drivers
which only had one input was to force people to avoid using 'name'
as the sole way to match.

When I originally wrote the AMBA primecell drivers, I did use things
like 'UARTCLK' and 'AACICLK' for the clk_get() name - since these
were the name on the device, and that really only provided people
with a bad example to follow.  Especially if you consider that the
hardware I had was FPGA based development boards where the clocking
layout lent itself well to ignoring the 'dev' argument.

> Right. I didn't intend to name the clock sources. I intended to name the
> clock -inputs- of a given device. IE. clk_get(dev, name) in my mind
> meant "give me the clock provider that feeds my "name" input).

That's absolutely the right way to look at it!

> Allright but passing a NULL doesn't help for drivers with multiple clock
> inputs. IE. How do you want to deal with that ? Do you want to deprecate
> the named API and instead provide a new clk_get_for_input(dev,
> clk_input) (clk_input could be name or numerical ... tbd) ?

NULL certainly doesn't help for drivers with multiple clock inputs.
For those, you have to explicitly name the input.  To take the
example commits I pointed at (the OMAP watchdog driver) OMAP blocks
generally have two clock inputs - a functional clock and an
interface clock.

Originally, the driver was effectively setup to match by clock source
name (wdt2_fck, wdt2_ick) which was SoC specific.  What the commits
did was convert things to looking using the names of the inputs
(aka consumer name) - so dev + "fck" and dev + "ick".

That results in clkdev looking up the clocks for device "omap_wdt"
for inputs "fck" and "ick" respectively.  On OMAP1 platforms, there
isn't an "ick" as such, so there's a match-any-device dummy "ick"
entry.  On OMAP2 and OMAP3 (the later revs) there are specific
clocks for these, and so the dummy entry is missing.

So, the approach I took was: where it is well defined that a device
has only one clock input, we pass a NULL name.  If it has more than
one clock input, we pass the specific consumer name required.

> Or am I missing a piece of the puzzle ?

Maybe - and since you're just starting to look at clkdev, I'll point
out that it's actually not intuitive which way around the "wildcard"
matching works in clkdev.  The clk_get() arguments aren't the
wildcards, they're in the clk_lookup structure.  Yes, it seems odd,
but if you consider it from the point of view of the platform code
wanting to match clocks to a specific set of devices and clock inputs,
it's actually the way around that you want.

-- 
Russell King

Re: ARM clock API to PowerPC

[Benjamin Herrenschmidt]

> > Ok. So I may have misunderstood what names were for. In my mind, those
> > were the name of the clock input on the HW device :-)
> 
> Oh, I do hope I didn't say that was wrong, because that is quite
> correct.  What the idea with passing a NULL 'name' with drivers
> which only had one input was to force people to avoid using 'name'
> as the sole way to match.

I see. So basically, they are meant to be the input names, but have been
abused by people to be "global scope" clock names and hence mess
happened.

> When I originally wrote the AMBA primecell drivers, I did use things
> like 'UARTCLK' and 'AACICLK' for the clk_get() name - since these
> were the name on the device, and that really only provided people
> with a bad example to follow.  Especially if you consider that the
> hardware I had was FPGA based development boards where the clocking
> layout lent itself well to ignoring the 'dev' argument.

Right.

> > Right. I didn't intend to name the clock sources. I intended to name the
> > clock -inputs- of a given device. IE. clk_get(dev, name) in my mind
> > meant "give me the clock provider that feeds my "name" input).
> 
> That's absolutely the right way to look at it!

Cool :-) It did feel right indeed.

> > Allright but passing a NULL doesn't help for drivers with multiple clock
> > inputs. IE. How do you want to deal with that ? Do you want to deprecate
> > the named API and instead provide a new clk_get_for_input(dev,
> > clk_input) (clk_input could be name or numerical ... tbd) ?
> 
> NULL certainly doesn't help for drivers with multiple clock inputs.
> For those, you have to explicitly name the input.  To take the
> example commits I pointed at (the OMAP watchdog driver) OMAP blocks
> generally have two clock inputs - a functional clock and an
> interface clock.

Right, that's a common setup.

> Originally, the driver was effectively setup to match by clock source
> name (wdt2_fck, wdt2_ick) which was SoC specific.  What the commits
> did was convert things to looking using the names of the inputs
> (aka consumer name) - so dev + "fck" and dev + "ick".

Ok, cool, so we are on the same page.

> That results in clkdev looking up the clocks for device "omap_wdt"
> for inputs "fck" and "ick" respectively.  On OMAP1 platforms, there
> isn't an "ick" as such, so there's a match-any-device dummy "ick"
> entry.  On OMAP2 and OMAP3 (the later revs) there are specific
> clocks for these, and so the dummy entry is missing.
> 
> So, the approach I took was: where it is well defined that a device
> has only one clock input, we pass a NULL name.  If it has more than
> one clock input, we pass the specific consumer name required.

Makes sense. I think this will work beautifully with the device-tree too
with the idea of having properties that then provide the binding for a
given clock input to the clock provider and the clock ID on that
provider (my proposal makes the later a number because doing strings can
be awkward in OF land in such mapping tables but I might go back to
strings, we'll see).

> > Or am I missing a piece of the puzzle ?
> 
> Maybe - and since you're just starting to look at clkdev, I'll point
> out that it's actually not intuitive which way around the "wildcard"
> matching works in clkdev.  The clk_get() arguments aren't the
> wildcards, they're in the clk_lookup structure.  Yes, it seems odd,
> but if you consider it from the point of view of the platform code
> wanting to match clocks to a specific set of devices and clock inputs,
> it's actually the way around that you want.

Ok. I'll read up a bit and see if I can get my head around it.

Thanks !

Cheers,
Ben.

Re: ARM clock API to PowerPC

[Benjamin Herrenschmidt]

On Thu, 2009-08-13 at 00:40 +0100, Russell King wrote:

> Maybe - and since you're just starting to look at clkdev, I'll point
> out that it's actually not intuitive which way around the "wildcard"
> matching works in clkdev.  The clk_get() arguments aren't the
> wildcards, they're in the clk_lookup structure.  Yes, it seems odd,
> but if you consider it from the point of view of the platform code
> wanting to match clocks to a specific set of devices and clock inputs,
> it's actually the way around that you want.

Ok so I had a look, it's basically a list of bindings between a device
"ID" (dev_name(dev)) and a clock input name, matched to a struct clk *.

I'm not entirely sure I want to port that over, because I believe that I
can do better with the device-tree, but then, it will mostly depend
whether we end up wanting to use drivers that do rely on the clkdev
mechanism.

For one, I'm not 100% certain that our dev_id's are unique. Then, that
means that at some stage, the clock source provider need to create the
struct clk for anything devices -may- need, so for all the device clock
connections in the system, it cannot be done on-demand.

I'm thus tempted to pursue for now an approach where the connection is
provided by the device-tree exclusively (or override platform code but
that's to be avoided most of the time), we'll see where it takes us.
IE. 

In case you haven't looked at what I had in mind there, basically the
device-node of a device contains properties linking each clock input to
the device node of the clock provider, along with the ID of that clock
within the provider space. So all I need is register clock providers,
that get passed in clk_get() with the "translated" clock ID and that can
spit out struct clk* "on demand".

For example, if I have an external PLL chip that provides 4 clocks, I
can register a clock provider driver attached to that device-node (which
can then also contain properties indicating to the driver how to program
the said PLL chip if so desired).

Then, if 2 of those lines (PLL1 and PLL3) go respectively to a device
SYS_CLK and PHY_CLK inputs, for example, then all I need is to have a
"clock-names" and "clock-map" properties in that device node. The first
one contains "SYS_CLK" and "PHY_CLK", and the second one, a list of two
items, each being the the phandle to the PLL and respectively 1 and 3.

I could just parse the tree and generate all the struct clk* at boot
time and somewhat generate clkdev for them etc... but I believe that
isn't needed, ie, clkdev is an ARM specific mechanism for expressing the
linkage between devices and clock sources but not something that we
would benefit from moving over since we can do better, I believe, with
the device-tree.

Or am I missing something still ? :-)

Cheers,
Ben.