[v3,3/5] clk: introduce the common clock framework

Message ID 1321926047-14211-4-git-send-email-mturquette@linaro.org
State New
Headers show

Commit Message

Mike Turquette Nov. 22, 2011, 1:40 a.m.
The common clk framework is an attempt to define a common struct clk
which can be used by most platforms, and an API that drivers can always
use safely for managing clks.

The net result is consolidation of many different struct clk definitions
and platform-specific clk framework implementations.

This patch introduces the common struct clk, struct clk_hw_ops and
definitions for the long-standing clk API in include/clk/clk.h.
Platforms may define their own hardware-specific clk structure, so long
as it wraps an instance of the common struct clk which is passed to
clk_init at initialization time.  From this point on all of the usual
clk APIs should be supported, so long as the platform supports them
through hardware-specific callbacks in struct clk_hw_ops.

See Documentation/clk.h for more details.

This patch is based on the work of Jeremy Kerr, which in turn was based
on the work of Ben Herrenschmidt.  Big thanks to both of them for
kickstarting this effort.

Signed-off-by: Mike Turquette <mturquette@linaro.org>
---
 drivers/clk/Kconfig  |    4 +
 drivers/clk/Makefile |    1 +
 drivers/clk/clk.c    |  538 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/clk.h  |  134 ++++++++++++-
 4 files changed, 673 insertions(+), 4 deletions(-)
 create mode 100644 drivers/clk/clk.c

Comments

Shawn Guo Nov. 26, 2011, 1:22 p.m. | #1
On Mon, Nov 21, 2011 at 05:40:45PM -0800, Mike Turquette wrote:
[...]
> +/**
> + * DOC: Using the CLK_PARENT_SET_RATE flag
> + *
> + * __clk_set_rate changes the child's rate before the parent's to more
> + * easily handle failure conditions.
> + *
> + * This means clk might run out of spec for a short time if its rate is
> + * increased before the parent's rate is updated.
> + *
> + * To prevent this consider setting the CLK_GATE_SET_RATE flag on any
> + * clk where you also set the CLK_PARENT_SET_RATE flag

Eh, this is how flag CLK_GATE_SET_RATE is born?  Does that mean to make
the call succeed all the clocks on the propagating path need to be gated
before clk_set_rate gets called?

> + */
> +struct clk *__clk_set_rate(struct clk *clk, unsigned long rate)
> +{
> +	struct clk *fail_clk = NULL;
> +	int ret;
> +	unsigned long old_rate = clk->rate;
> +	unsigned long new_rate;
> +	unsigned long parent_old_rate;
> +	unsigned long parent_new_rate = 0;
> +	struct clk *child;
> +	struct hlist_node *tmp;
> +
> +	/* bail early if we can't change rate while clk is enabled */
> +	if ((clk->flags & CLK_GATE_SET_RATE) && clk->enable_count)
> +		return clk;
> +
> +	/* find the new rate and see if parent rate should change too */
> +	WARN_ON(!clk->ops->round_rate);
> +
> +	new_rate = clk->ops->round_rate(clk, rate, &parent_new_rate);
> +
> +	/* FIXME propagate pre-rate change notification here */
> +	/* XXX note that pre-rate change notifications will stack */
> +
> +	/* change the rate of this clk */
> +	ret = clk->ops->set_rate(clk, new_rate);

Yes, right here, the clock gets set to some unexpected rate, since the
parent clock has not been set to the target rate yet at this point.

> +	if (ret)
> +		return clk;
> +
> +	/*
> +	 * change the rate of the parent clk if necessary
> +	 *
> +	 * hitting the nested 'if' path implies we have hit a .set_rate
> +	 * failure somewhere upstream while propagating __clk_set_rate
> +	 * up the clk tree.  roll back the clk rates one by one and
> +	 * return the pointer to the clk that failed.  clk_set_rate will
> +	 * use the pointer to propagate a rate-change abort notifier
> +	 * from the "highest" point.
> +	 */
> +	if ((clk->flags & CLK_PARENT_SET_RATE) && parent_new_rate) {
> +		parent_old_rate = clk->parent->rate;
> +		fail_clk = __clk_set_rate(clk->parent, parent_new_rate);
> +
> +		/* roll back changes if parent rate change failed */
> +		if (fail_clk) {
> +			pr_warn("%s: failed to set parent %s rate to %lu\n",
> +					__func__, fail_clk->name,
> +					parent_new_rate);
> +			clk->ops->set_rate(clk, old_rate);
> +		}
> +		return fail_clk;
> +	}
> +
> +	/*
> +	 * set clk's rate & recalculate the rates of clk's children
> +	 *
> +	 * hitting this path implies we have successfully finished
> +	 * propagating recursive calls to __clk_set_rate up the clk tree
> +	 * (if necessary) and it is safe to propagate clk_recalc_rates and
> +	 * post-rate change notifiers down the clk tree from this point.
> +	 */
> +	clk->rate = new_rate;
> +	/* FIXME propagate post-rate change notifier for only this clk */
> +	hlist_for_each_entry(child, tmp, &clk->children, child_node)
> +		clk_recalc_rates(child);
> +
> +	return fail_clk;
> +}

[...]

> diff --git a/include/linux/clk.h b/include/linux/clk.h
> index 7213b52..3b0eb3f 100644
> --- a/include/linux/clk.h
> +++ b/include/linux/clk.h
> @@ -3,6 +3,8 @@
>   *
>   *  Copyright (C) 2004 ARM Limited.
>   *  Written by Deep Blue Solutions Limited.
> + *  Copyright (c) 2010-2011 Jeremy Kerr <jeremy.kerr@canonical.com>
> + *  Copyright (C) 2011 Linaro Ltd <mturquette@linaro.org>
>   *
>   * This program is free software; you can redistribute it and/or modify
>   * it under the terms of the GNU General Public License version 2 as
> @@ -13,17 +15,134 @@
>  
>  #include <linux/kernel.h>
>  
> +#include <linux/kernel.h>

Eh, why adding a duplication?

> +#include <linux/errno.h>
> +
>  struct device;
>  
> +struct clk;

Do you really need this?

[...]

> +struct clk_hw_ops {
> +	int		(*prepare)(struct clk *clk);
> +	void		(*unprepare)(struct clk *clk);
> +	int		(*enable)(struct clk *clk);
> +	void		(*disable)(struct clk *clk);
> +	unsigned long	(*recalc_rate)(struct clk *clk);
> +	long		(*round_rate)(struct clk *clk, unsigned long,

The return type seems interesting.  If we intend to return a rate, it
should be 'unsigned long' rather than 'long'.  I'm just curious about
the possible reason behind that.

> +					unsigned long *);
> +	int		(*set_parent)(struct clk *clk, struct clk *);
> +	struct clk *	(*get_parent)(struct clk *clk);
> +	int		(*set_rate)(struct clk *clk, unsigned long);

Nit: would it be reasonable to put set_rate after round_rate to group
*_rate functions together?

> +};
> +
> +/**
> + * clk_init - initialize the data structures in a struct clk
> + * @dev: device initializing this clk, placeholder for now
> + * @clk: clk being initialized
> + *
> + * Initializes the lists in struct clk, queries the hardware for the
> + * parent and rate and sets them both.  Adds the clk to the sysfs tree
> + * topology.
> + *
> + * Caller must populate .name, .flags and .ops before calling clk_init.
> + */
> +void clk_init(struct device *dev, struct clk *clk);
> +
> +#endif /* !CONFIG_GENERIC_CLK */
>  
>  /**
>   * clk_get - lookup and obtain a reference to a clock producer.
> @@ -107,9 +226,16 @@ static inline void clk_unprepare(struct clk *clk)
>  }
>  #endif
>  
> +int __clk_enable(struct clk *clk);
> +void __clk_disable(struct clk *clk);
> +int __clk_prepare(struct clk *clk);
> +void __clk_unprepare(struct clk *clk);
> +void __clk_reparent(struct clk *clk, struct clk *new_parent);
> +
Do we really need to export all these common clk internal functions?

Regards,
Shawn

>  /**
>   * clk_get_rate - obtain the current clock rate (in Hz) for a clock source.
>   *		  This is only valid once the clock source has been enabled.
> + *		  Returns zero if the clock rate is unknown.
>   * @clk: clock source
>   */
>  unsigned long clk_get_rate(struct clk *clk);
> -- 
> 1.7.4.1
> 
>
Mike Turquette Nov. 27, 2011, 6 a.m. | #2
On Sat, Nov 26, 2011 at 5:22 AM, Shawn Guo <shawn.guo@freescale.com> wrote:
> On Mon, Nov 21, 2011 at 05:40:45PM -0800, Mike Turquette wrote:
>> + * To prevent this consider setting the CLK_GATE_SET_RATE flag on any
>> + * clk where you also set the CLK_PARENT_SET_RATE flag
>
> Eh, this is how flag CLK_GATE_SET_RATE is born?  Does that mean to make
> the call succeed all the clocks on the propagating path need to be gated
> before clk_set_rate gets called?

Setting that flag on any clk implies nothing about the parents of that
clk.  Obviously if a clk's child is enabled then clk is enabled and
won't have it's rate change if the flag is set, which is the whole
point of the flag.  Again, you don't have to set the flag.

>> +     /* change the rate of this clk */
>> +     ret = clk->ops->set_rate(clk, new_rate);
>
> Yes, right here, the clock gets set to some unexpected rate, since the
> parent clock has not been set to the target rate yet at this point.

Sequence is irrelevant for the case where both parent and child change
dividers, since the output of the child clk will be "weird" at some
point.

>> diff --git a/include/linux/clk.h b/include/linux/clk.h
>> index 7213b52..3b0eb3f 100644
>> --- a/include/linux/clk.h
>> +++ b/include/linux/clk.h
>> @@ -3,6 +3,8 @@
>>   *
>>   *  Copyright (C) 2004 ARM Limited.
>>   *  Written by Deep Blue Solutions Limited.
>> + *  Copyright (c) 2010-2011 Jeremy Kerr <jeremy.kerr@canonical.com>
>> + *  Copyright (C) 2011 Linaro Ltd <mturquette@linaro.org>
>>   *
>>   * This program is free software; you can redistribute it and/or modify
>>   * it under the terms of the GNU General Public License version 2 as
>> @@ -13,17 +15,134 @@
>>
>>  #include <linux/kernel.h>
>>
>> +#include <linux/kernel.h>
>
> Eh, why adding a duplication?

Will fix in v4.

>
>> +#include <linux/errno.h>
>> +
>>  struct device;
>>
>> +struct clk;
>
> Do you really need this?

Strange.  This line existed pre-common clk patches.  I'll look into
why it is getting "added" back in.  This declaration is necessary for
the old-style clk frameworks which each defined their own struct clk.
I assume it is still needed but I'll look at the git history to figure
it out.

>> +struct clk_hw_ops {
>> +     int             (*prepare)(struct clk *clk);
>> +     void            (*unprepare)(struct clk *clk);
>> +     int             (*enable)(struct clk *clk);
>> +     void            (*disable)(struct clk *clk);
>> +     unsigned long   (*recalc_rate)(struct clk *clk);
>> +     long            (*round_rate)(struct clk *clk, unsigned long,
>
> The return type seems interesting.  If we intend to return a rate, it
> should be 'unsigned long' rather than 'long'.  I'm just curious about
> the possible reason behind that.

Yeah these are mostly from the original patch set.  I'll go over these
with a fine-tooth comb for v4.  To some extent I think they model the
return types of the clk API in clk.h.

>
>> +                                     unsigned long *);
>> +     int             (*set_parent)(struct clk *clk, struct clk *);
>> +     struct clk *    (*get_parent)(struct clk *clk);
>> +     int             (*set_rate)(struct clk *clk, unsigned long);
>
> Nit: would it be reasonable to put set_rate after round_rate to group
> *_rate functions together?

Will fix in v4.

>> +int __clk_enable(struct clk *clk);
>> +void __clk_disable(struct clk *clk);
>> +int __clk_prepare(struct clk *clk);
>> +void __clk_unprepare(struct clk *clk);
>> +void __clk_reparent(struct clk *clk, struct clk *new_parent);
>> +
> Do we really need to export all these common clk internal functions?

I think we do.  Saravana also felt this was necessary.

I know that for OMAP we need __clk_prepare, __clk_unprepare and
__clk_reparent just to handle our PLLs.  I don't think we need
__clk_enable or __clk_disable since we can call the proper APIs for
those with the prepare_mutex held.

If no one needs __clk_enable and __clk_disable then I am happy to
remove those declarations.

Thanks for reviewing,
Mike
Paul Walmsley Dec. 1, 2011, 1:20 a.m. | #3
Hello,

Here are some initial comments on clk_get_rate().

On Mon, 21 Nov 2011, Mike Turquette wrote:

> +/**
> + * clk_get_rate - return the rate of clk
> + * @clk: the clk whose rate is being returned
> + *
> + * Simply returns the cached rate of the clk.  Does not query the hardware.  If
> + * clk is NULL then returns 0.
> + */
> +unsigned long clk_get_rate(struct clk *clk)
> +{
> +	if (!clk)
> +		return 0;
> +
> +	return clk->rate;
> +}
> +EXPORT_SYMBOL_GPL(clk_get_rate);

This implementation of clk_get_rate() is racy, and is, in general, unsafe. 
The problem is that, in many cases, the clock's rate may change between 
the time that clk_get_rate() is called and the time that the returned 
rate is used.  This is the case for many clocks which are part of a 
larger DVFS domain, for example.

Several changes are needed to fix this:

1. When a clock user calls clk_enable() on a clock, the clock framework 
should prevent other users of the clock from changing the clock's rate.  
This should persist until the clock user calls clk_disable() (but see also 
#2 below).  This will ensure that clock users can rely on the rate 
returned by clk_get_rate(), as long as it's called between clk_enable() 
and clk_disable().  And since the clock's rate is guaranteed to remain the 
same during this time, code that cannot tolerate clock rate changes 
without special handling (such as driver code for external I/O devices) 
will work safely without further modification.

2. Since the protocol described in #1 above will prevent DVFS from working 
when the clock is part of a larger DVFS clock group, functions need to be 
added to allow and prevent other clock users from changing the clock's 
rate.  I'll use the function names "clk_allow_rate_changes(struct clk *)" 
and "clk_block_rate_changes(struct clk *)" for this discussion.  These 
functions can be used by clock users to define critical sections during 
which other entities on the system are allowed to change a clock's rate - 
even if the clock is currently enabled.  (Note that when a clock is 
prevented from changing its rate, all of the clocks from it up to the root 
of the tree should also be prevented from changing rates, since parent 
rate changes generally cause disruptions in downstream clocks.)

3. For the above changes to work, the clock framework will need to 
discriminate between different clock users' calls to clock functions like 
clk_{get,set}_rate(), etc.  Luckily this should be possible within the 
current clock interface.  clk_get() can allocate and return a new struct 
clk that clk_put() can later free.  One useful side effect of doing this 
is that the clock framework could catch unbalanced clk_{enable,disable}() 
calls.

4. clk_get_rate() must return an error when it's called in situations 
where the caller hasn't ensured that the clock's rate won't be changed by 
other entities.  For non-fixed rate clocks, these forbidden sections would 
include code between a clk_get() and a clk_enable(), or between a 
clk_disable() and a clk_put() or clk_enable(), or between a 
clk_allow_rate_changes() and clk_block_rate_changes().  The first and 
second of those three cases will require some code auditing of 
clk_get_rate() users to ensure that they are only calling it after they've 
enabled the clock.  And presumably most of them are not checking for 
error.  include/linux/clk.h doesn't define an error return value, so this 
needs to be explicitly defined in clk.h.


- Paul
Paul Walmsley Dec. 1, 2011, 2:13 a.m. | #4
Hi

a few initial comments on clk_get_parent().

On Mon, 21 Nov 2011, Mike Turquette wrote:

> +/**
> + * clk_get_parent - return the parent of a clk
> + * @clk: the clk whose parent gets returned
> + *
> + * Simply returns clk->parent.  It is up to the caller to hold the
> + * prepare_lock, if desired.  Returns NULL if clk is NULL.
> + */
> +struct clk *clk_get_parent(struct clk *clk)
> +{
> +	if (!clk)
> +		return NULL;
> +
> +	return clk->parent;
> +}
> +EXPORT_SYMBOL_GPL(clk_get_parent);

This implementation of clk_get_parent() has similar problems to the 
clk_get_rate() implementation:

    http://lkml.org/lkml/2011/11/30/403

clk_get_parent() should return an error if some other entity can change 
the clock's parent between the time that clk_get_parent() returns, and the 
time that the returned struct clk * is used.

For this to work, we need to define when clk_get_parent() should succeed. 
If we follow the protocol outlined in the above URL about clk_get_rate(), 
and we stipulate that when we block rate changes, we also block parent 
changes, then this should be fairly straightforward.  clk_get_parent() can 
succeed:

1. between a clk_enable() and a clk_disable() or clk_allow_rate_changes(),

2. between a clk_block_rate_changes() and a clk_enable(), clk_disable(), 
   or clk_allow_rate_changes()

As with clk_get_rate(), include/linux/clk.h is missing documentation of 
what the error return value should be.  This should be a little easier to 
define than with clk_get_rate(), but I don't think the error value should 
be NULL.  This is because NULL is a valid return value when 
clk_get_parent() is called on root clocks.  Better to use 
ERR_PTR(-EINVAL/-EBUSY/etc.).


- Paul
Mike Turquette Dec. 1, 2011, 5:53 a.m. | #5
On Wed, Nov 30, 2011 at 5:20 PM, Paul Walmsley <paul@pwsan.com> wrote:
> This implementation of clk_get_rate() is racy, and is, in general, unsafe.
> The problem is that, in many cases, the clock's rate may change between
> the time that clk_get_rate() is called and the time that the returned
> rate is used.  This is the case for many clocks which are part of a
> larger DVFS domain, for example.

Hi Paul,

Thanks much for reviewing.  Just FYI, the v4 patchset I'm working on
has clk_get_rate and clk_get_parent hold the prepare mutex themselves,
so it solves some of the raciness of accessing struct clk members.
This change does not address your points below but I wanted to include
it for completeness sake.

> Several changes are needed to fix this:
>
> 1. When a clock user calls clk_enable() on a clock, the clock framework
> should prevent other users of the clock from changing the clock's rate.
> This should persist until the clock user calls clk_disable() (but see also
> #2 below).  This will ensure that clock users can rely on the rate
> returned by clk_get_rate(), as long as it's called between clk_enable()
> and clk_disable().  And since the clock's rate is guaranteed to remain the
> same during this time, code that cannot tolerate clock rate changes
> without special handling (such as driver code for external I/O devices)
> will work safely without further modification.

This is certainly imposing a default behavior in the clk framework core.

> 2. Since the protocol described in #1 above will prevent DVFS from working
> when the clock is part of a larger DVFS clock group, functions need to be
> added to allow and prevent other clock users from changing the clock's
> rate.  I'll use the function names "clk_allow_rate_changes(struct clk *)"
> and "clk_block_rate_changes(struct clk *)" for this discussion.  These
> functions can be used by clock users to define critical sections during
> which other entities on the system are allowed to change a clock's rate -
> even if the clock is currently enabled.  (Note that when a clock is
> prevented from changing its rate, all of the clocks from it up to the root
> of the tree should also be prevented from changing rates, since parent
> rate changes generally cause disruptions in downstream clocks.)

Likewise when a clk is requested to transition to a new frequency it
will have to clear it with all of the clks below it, so there is still
a need to propagate a request throughout the clk tree whenever
clk_set_rate is called and take a decision.

One way to solve this is for driver owners to sprinkle their code with
clk_block_rate_change and clk_allow_rate_change as you propose.  There
is a problem with this method if it is not supplemented with
rate-change notifications that drivers are allowed to handle
asynchronously.  Take for example a MMC driver which normally runs
it's functional clk at 24MHz.  However for a low-intensity, periodic
task such as playing an mp3 from SD card we would really like to lower
clk rates across the whole SoC.  Assuming that the MMC driver holds
clk_block_rate_change across any SD card transaction, our low power
scheme to lower clks across the SoC is stuck in a racy game of trying
to request a lower clk rate for the MMC functional clk while that same
MMC controller is mostly saturated serving up the mp3.

In this case it would be nice to have asynchronous notifiers that can
interrupt the MMC driver with a pre-change notifier and say "please
finish your current transaction, stall your next transaction, and then
return so I can change your clk rate".  Then after the clk rate change
occurs a post-change notification would also go out to the MMC driver
saying "ok I'm done changing rates.  here is your new clk rate and
please continue working".  The notification is very polite.

It is worth nothing that the MMC driver can return NOTIFY_STOP in it's
pre-change notification handler and this effectively does the same
thing as traversing the clk subtree and checking to make sure that
nobody is holding clk_block_rate_change against any of the children
clocks.

The point of all of that text above is to say: if we have to walk the
whole clk subtree below the point where we want to change rates AND we
want to make a dynamic case-by-case call on whether to allow the rate
change to happen (as opposed to contending with the allow/block
semantics) then I think that only having notifications will suffice.

> 3. For the above changes to work, the clock framework will need to
> discriminate between different clock users' calls to clock functions like
> clk_{get,set}_rate(), etc.  Luckily this should be possible within the
> current clock interface.  clk_get() can allocate and return a new struct
> clk that clk_put() can later free.  One useful side effect of doing this
> is that the clock framework could catch unbalanced clk_{enable,disable}()
> calls.

This is definitely worth thinking about.  Another way to accomplish
this is stop treating prepare_count and enable_count as scalars and
instead vectorize them by tracking a list of struct device *'s.  This
starts to get into can-of-worms territory if we want to consider
clk_set_rate_range(dev, clk, min, max) and the broader DVFS
implications.  I'm a little worried about under-designing now for
future DVFS stuff, but I also don't want the common clk fundamentals
to stall any more than it has (2+ years!).

> 4. clk_get_rate() must return an error when it's called in situations
> where the caller hasn't ensured that the clock's rate won't be changed by
> other entities.  For non-fixed rate clocks, these forbidden sections would
> include code between a clk_get() and a clk_enable(), or between a
> clk_disable() and a clk_put() or clk_enable(), or between a
> clk_allow_rate_changes() and clk_block_rate_changes().  The first and
> second of those three cases will require some code auditing of
> clk_get_rate() users to ensure that they are only calling it after they've
> enabled the clock.  And presumably most of them are not checking for
> error.  include/linux/clk.h doesn't define an error return value, so this
> needs to be explicitly defined in clk.h.

This adds a lot of burden to a driver that just wants to know a rate.
Especially if that purpose is for something as simple/transient as a
pr_debug message or something.

Thanks again for the review.  I'll chew on it a bit.

Regards,
Mike
Paul Walmsley Dec. 1, 2011, 6:39 a.m. | #6
Hi Mike

a few brief comments.

On Wed, 30 Nov 2011, Turquette, Mike wrote:

> Likewise when a clk is requested to transition to a new frequency it
> will have to clear it with all of the clks below it, so there is still
> a need to propagate a request throughout the clk tree whenever
> clk_set_rate is called and take a decision.
> 
> One way to solve this is for driver owners to sprinkle their code with
> clk_block_rate_change and clk_allow_rate_change as you propose.
> There is a problem with this method if it is not supplemented with 
> rate-change notifications that drivers are allowed to handle 
> asynchronously.  

I don't think notifiers have a bearing on clk_get_rate().

> In this case it would be nice to have asynchronous notifiers that can
> interrupt the MMC driver with a pre-change notifier and say "please
> finish your current transaction, stall your next transaction, and then
> return so I can change your clk rate".  Then after the clk rate change
> occurs a post-change notification would also go out to the MMC driver
> saying "ok I'm done changing rates.  here is your new clk rate and
> please continue working".  The notification is very polite.

I haven't made any comments about clock notifiers yet, because my 
comments so far have only concerned clk_get_rate() and 
clk_get_parent().

Clock rate/parent-change notifiers are requirements for DVFS use-cases, 
and they must be paired with something like the 
clk_{allow,block}_rate_change() functions to work efficiently.  I intend 
to comment on this later; it's not a simple problem.  It might be worth 
noting that Tero and I implemented a simplified version of this for the 
N900.

> It is worth nothing that the MMC driver can return NOTIFY_STOP in it's
> pre-change notification handler and this effectively does the same
> thing as traversing the clk subtree and checking to make sure that
> nobody is holding clk_block_rate_change against any of the children
> clocks.
> 
> The point of all of that text above is to say: if we have to walk the
> whole clk subtree below the point where we want to change rates AND we
> want to make a dynamic case-by-case call on whether to allow the rate
> change to happen (as opposed to contending with the allow/block
> semantics) then I think that only having notifications will suffice.

That would not be good.  A notifier implementation without something like 
clk_{allow,block}_rate_change() is going to waste a lot of CPU time making 
pointless calls into the notifier chains by whatever is attempting to do 
top-down DVFS.

With clk_{allow,block}_rate_change(), only a single comparison is needed 
to determine whether or not the rate change can succeed.  A 
blocking clk_set_rate() variant could also be efficiently implemented with 
that information, if so desired.

In general, a clock rate change notifier should almost never return 
NOTIFY_STOP.  That should only happen if some event occurred that took 
place after the notifier chain started.

> > 3. For the above changes to work, the clock framework will need to
> > discriminate between different clock users' calls to clock functions like
> > clk_{get,set}_rate(), etc.  Luckily this should be possible within the
> > current clock interface.  clk_get() can allocate and return a new struct
> > clk that clk_put() can later free.  One useful side effect of doing this
> > is that the clock framework could catch unbalanced clk_{enable,disable}()
> > calls.
> 
> This is definitely worth thinking about.  Another way to accomplish
> this is stop treating prepare_count and enable_count as scalars and
> instead vectorize them by tracking a list of struct device *'s.

To return to my original comments on clk_get_rate(): how would this allow 
the clock framework to discriminate between callers of clk_get_rate()?  
(Or indeed any clock framework function?)  Or is your comment simply 
addressing the unbalanced clk_{enable,disable}() case?

> > 4. clk_get_rate() must return an error when it's called in situations
> > where the caller hasn't ensured that the clock's rate won't be changed by
> > other entities.  For non-fixed rate clocks, these forbidden sections would
> > include code between a clk_get() and a clk_enable(), or between a
> > clk_disable() and a clk_put() or clk_enable(), or between a
> > clk_allow_rate_changes() and clk_block_rate_changes().  The first and
> > second of those three cases will require some code auditing of
> > clk_get_rate() users to ensure that they are only calling it after they've
> > enabled the clock.  And presumably most of them are not checking for
> > error.  include/linux/clk.h doesn't define an error return value, so this
> > needs to be explicitly defined in clk.h.
> 
> This adds a lot of burden to a driver that just wants to know a rate.
> Especially if that purpose is for something as simple/transient as a
> pr_debug message or something.

Could you clarify what burden are you referring to?

The above protocol requires few (if any) changes to existing clock 
framework users.  So for example, the following code:

c = clk_get(dev, clk_name);
clk_enable(c);
rate = clk_get_rate(c);

would still continue to work, since the rate would be guaranteed not to 
change after the clk_enable().  However, the (unsafe):

c = clk_get(dev, clk_name);
rate = clk_get_rate(c);

would need to be modified to become safe, by adding a 
clk_block_rate_change() before the clk_get_rate().


- Paul
Russell King - ARM Linux Dec. 1, 2011, 8:45 a.m. | #7
On Wed, Nov 30, 2011 at 06:20:50PM -0700, Paul Walmsley wrote:
> 1. When a clock user calls clk_enable() on a clock, the clock framework 
> should prevent other users of the clock from changing the clock's rate.  
> This should persist until the clock user calls clk_disable() (but see also 
> #2 below).  This will ensure that clock users can rely on the rate 
> returned by clk_get_rate(), as long as it's called between clk_enable() 
> and clk_disable().  And since the clock's rate is guaranteed to remain the 
> same during this time, code that cannot tolerate clock rate changes 
> without special handling (such as driver code for external I/O devices) 
> will work safely without further modification.

So, if you have a PLL whose parent clock is not used by anything else.
You want to program it to a certain rate.

You call clk_disable() on the PLL clock.  This walks up the tree and
disables the parent.  You then try to set the rate using clk_set_rate().
clk_set_rate() in this circumstance can't wait for the PLL to lock
because it can't - there's no reference clock for it.

You then call clk_enable().  The PLL now takes its time to lock.  You
can't sleep in clk_enable() because it might be called from atomic
contexts, so you have to spin waiting for this.

Overloading clk_disable/clk_enable in this way is a bad solution to
this problem.
Mark Brown Dec. 1, 2011, 2:42 p.m. | #8
On Wed, Nov 30, 2011 at 11:39:59PM -0700, Paul Walmsley wrote:

> Clock rate/parent-change notifiers are requirements for DVFS use-cases, 
> and they must be paired with something like the 
> clk_{allow,block}_rate_change() functions to work efficiently.  I intend 
> to comment on this later; it's not a simple problem.  It might be worth 
> noting that Tero and I implemented a simplified version of this for the 
> N900.

I'm thinking that if we're going to have clk_{allow,block}_rate_change()
we may as well make that the main interface to enable rate changes - if
a device wants to change the clock rate it allows rate changes using
that interface rather than by disabling the clocks.  I've got devices
which can do glitch free updates of active clocks so having to disable
would be a real restriction, and cpufreq would have issues with actually
disabling the clock too I expect.
Mike Turquette Dec. 1, 2011, 6:25 p.m. | #9
On Thu, Dec 1, 2011 at 6:42 AM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Wed, Nov 30, 2011 at 11:39:59PM -0700, Paul Walmsley wrote:
>
>> Clock rate/parent-change notifiers are requirements for DVFS use-cases,
>> and they must be paired with something like the
>> clk_{allow,block}_rate_change() functions to work efficiently.  I intend
>> to comment on this later; it's not a simple problem.  It might be worth
>> noting that Tero and I implemented a simplified version of this for the
>> N900.
>
> I'm thinking that if we're going to have clk_{allow,block}_rate_change()
> we may as well make that the main interface to enable rate changes - if
> a device wants to change the clock rate it allows rate changes using
> that interface rather than by disabling the clocks.  I've got devices
> which can do glitch free updates of active clocks so having to disable
> would be a real restriction, and cpufreq would have issues with actually
> disabling the clock too I expect.
>

I agree that imposing a "disable clk before changing rate" policy in
the framework core is a bad idea.  During discussion on the
CLK_GATE_SET_RATE flag in the patch #2 Shawn commented that he has
some clks that must be enabled to change their rates (I assume he
means PLLs but that detail doesn't really matter).

Regards,
Mike
Paul Walmsley Dec. 1, 2011, 6:30 p.m. | #10
Hi Mark,

On Thu, 1 Dec 2011, Mark Brown wrote:

> On Wed, Nov 30, 2011 at 11:39:59PM -0700, Paul Walmsley wrote:
> 
> > Clock rate/parent-change notifiers are requirements for DVFS use-cases, 
> > and they must be paired with something like the 
> > clk_{allow,block}_rate_change() functions to work efficiently.  I intend 
> > to comment on this later; it's not a simple problem.  It might be worth 
> > noting that Tero and I implemented a simplified version of this for the 
> > N900.
> 
> I'm thinking that if we're going to have clk_{allow,block}_rate_change()
> we may as well make that the main interface to enable rate changes - if
> a device wants to change the clock rate it allows rate changes using
> that interface rather than by disabling the clocks.  I've got devices
> which can do glitch free updates of active clocks so having to disable
> would be a real restriction, and cpufreq would have issues with actually
> disabling the clock too I expect.

The intention behind the clk_{allow,block}_rate_change() proposal was to 
allow the current user of the clock to change its rate without having to 
call clk_{allow,block}_rate_change(), if that driver was the sole user of 
the clock.

So for example, if you had a driver that did:

c = clk_get(dev, clk_name);
clk_enable(c);
clk_set_rate(c, clk_rate);

and c was currently not enabled by any other driver on the system, and 
nothing else had called clk_block_rate_change(c), then the rate change 
would be allowed to proceed.  (modulo any notifier activity, etc.)  

So clk_{allow,block}_rate_change() was simply intended to allow or 
restrict other users of the same clock, not the current user.


- Paul
Mark Brown Dec. 1, 2011, 6:32 p.m. | #11
On Thu, Dec 01, 2011 at 11:30:16AM -0700, Paul Walmsley wrote:

> So for example, if you had a driver that did:

> c = clk_get(dev, clk_name);
> clk_enable(c);
> clk_set_rate(c, clk_rate);

> and c was currently not enabled by any other driver on the system, and 
> nothing else had called clk_block_rate_change(c), then the rate change 
> would be allowed to proceed.  (modulo any notifier activity, etc.)  

> So clk_{allow,block}_rate_change() was simply intended to allow or 
> restrict other users of the same clock, not the current user.

Ah, sorry!  I'd totally misunderstood what you were proposing.
Russell King - ARM Linux Dec. 1, 2011, 10:03 p.m. | #12
On Thu, Dec 01, 2011 at 11:30:16AM -0700, Paul Walmsley wrote:
> The intention behind the clk_{allow,block}_rate_change() proposal was to 
> allow the current user of the clock to change its rate without having to 
> call clk_{allow,block}_rate_change(), if that driver was the sole user of 
> the clock.

And how does a driver know that?
Paul Walmsley Dec. 2, 2011, 5:13 p.m. | #13
Hi Russell,

On Thu, 1 Dec 2011, Russell King - ARM Linux wrote:

> On Wed, Nov 30, 2011 at 06:20:50PM -0700, Paul Walmsley wrote:
> > 1. When a clock user calls clk_enable() on a clock, the clock framework 
> > should prevent other users of the clock from changing the clock's rate.  
> > This should persist until the clock user calls clk_disable() (but see also 
> > #2 below).  This will ensure that clock users can rely on the rate 
> > returned by clk_get_rate(), as long as it's called between clk_enable() 
> > and clk_disable().  And since the clock's rate is guaranteed to remain the 
> > same during this time, code that cannot tolerate clock rate changes 
> > without special handling (such as driver code for external I/O devices) 
> > will work safely without further modification.
> 
> So, if you have a PLL whose parent clock is not used by anything else.
> You want to program it to a certain rate.
> 
> You call clk_disable() on the PLL clock.

The approach described wouldn't require the PLL to be disabled before 
changing its rate.  If there are no other users of the PLL, or if the 
other users of the PLL have indicated that it's safe for others to change 
the PLL's rate, the clock framework would allow the PLL rate change, even 
if the PLL is enabled.  (modulo any notifier activity, and assuming that 
the underlying PLL hardware allows its frequency to be reprogrammed while 
the PLL is enabled)

> This walks up the tree and disables the parent.  You then try to set the 
> rate using clk_set_rate(). clk_set_rate() in this circumstance can't 
> wait for the PLL to lock because it can't - there's no reference clock 
> for it.

As an aside, this seems like a good time to mention that the behavior of 
clk_set_rate() on a disabled clock needs to be clarified.


- Paul
Russell King - ARM Linux Dec. 2, 2011, 8:23 p.m. | #14
On Fri, Dec 02, 2011 at 10:13:10AM -0700, Paul Walmsley wrote:
> Hi Russell,
> 
> On Thu, 1 Dec 2011, Russell King - ARM Linux wrote:
> 
> > On Wed, Nov 30, 2011 at 06:20:50PM -0700, Paul Walmsley wrote:
> > > 1. When a clock user calls clk_enable() on a clock, the clock framework 
> > > should prevent other users of the clock from changing the clock's rate.  
> > > This should persist until the clock user calls clk_disable() (but see also 
> > > #2 below).  This will ensure that clock users can rely on the rate 
> > > returned by clk_get_rate(), as long as it's called between clk_enable() 
> > > and clk_disable().  And since the clock's rate is guaranteed to remain the 
> > > same during this time, code that cannot tolerate clock rate changes 
> > > without special handling (such as driver code for external I/O devices) 
> > > will work safely without further modification.
> > 
> > So, if you have a PLL whose parent clock is not used by anything else.
> > You want to program it to a certain rate.
> > 
> > You call clk_disable() on the PLL clock.
> 
> The approach described wouldn't require the PLL to be disabled before 
> changing its rate.  If there are no other users of the PLL, or if the 
> other users of the PLL have indicated that it's safe for others to change 
> the PLL's rate, the clock framework would allow the PLL rate change, even 
> if the PLL is enabled.  (modulo any notifier activity, and assuming that 
> the underlying PLL hardware allows its frequency to be reprogrammed while 
> the PLL is enabled)
> 
> > This walks up the tree and disables the parent.  You then try to set the 
> > rate using clk_set_rate(). clk_set_rate() in this circumstance can't 
> > wait for the PLL to lock because it can't - there's no reference clock 
> > for it.
> 
> As an aside, this seems like a good time to mention that the behavior of 
> clk_set_rate() on a disabled clock needs to be clarified.

It's more complicated than that.  Clocks have more states than just
enabled and disabled.

There is:
- unprepared
- prepared and disabled
- prepared and enabled

Implementations can chose at which point to enable their PLLs and wait
for them to lock - but if they want to sleep while waiting, they must
do this in the prepare method, not the enable method (remember, enable
is to be callable from atomic contexts.)

So, it's entirely possible that a prepared clock will have the PLLs up
and running, which means that clk_set_rate() can also wait for the PLL
to stablize (which would be a good idea.)

Now... we can say that PLLs should be locked when the prepare method
returns, or whenever clk_set_rate() returns.  The problem with that is
there's a race condition between clk_enable() and clk_set_rate() - if
we allow clk_set_rate() to sleep waiting for the PLL to lock, a
concurrent clk_enable() can't be prevented from returning because
that would involve holding a spinlock...

I think this is just one of those unsolvable issues - if you have
concurrent users of a PLL there's not much you can do - race free - to
prevent the PLL changing beneath some user.
Uwe Kleine-König Dec. 2, 2011, 8:32 p.m. | #15
Hello,

On Fri, Dec 02, 2011 at 08:23:06PM +0000, Russell King - ARM Linux wrote:
> On Fri, Dec 02, 2011 at 10:13:10AM -0700, Paul Walmsley wrote:
> > Hi Russell,
> > 
> > On Thu, 1 Dec 2011, Russell King - ARM Linux wrote:
> > 
> > > On Wed, Nov 30, 2011 at 06:20:50PM -0700, Paul Walmsley wrote:
> > > > 1. When a clock user calls clk_enable() on a clock, the clock framework 
> > > > should prevent other users of the clock from changing the clock's rate.  
> > > > This should persist until the clock user calls clk_disable() (but see also 
> > > > #2 below).  This will ensure that clock users can rely on the rate 
> > > > returned by clk_get_rate(), as long as it's called between clk_enable() 
> > > > and clk_disable().  And since the clock's rate is guaranteed to remain the 
> > > > same during this time, code that cannot tolerate clock rate changes 
> > > > without special handling (such as driver code for external I/O devices) 
> > > > will work safely without further modification.
> > > 
> > > So, if you have a PLL whose parent clock is not used by anything else.
> > > You want to program it to a certain rate.
> > > 
> > > You call clk_disable() on the PLL clock.
> > 
> > The approach described wouldn't require the PLL to be disabled before 
> > changing its rate.  If there are no other users of the PLL, or if the 
> > other users of the PLL have indicated that it's safe for others to change 
> > the PLL's rate, the clock framework would allow the PLL rate change, even 
> > if the PLL is enabled.  (modulo any notifier activity, and assuming that 
> > the underlying PLL hardware allows its frequency to be reprogrammed while 
> > the PLL is enabled)
> > 
> > > This walks up the tree and disables the parent.  You then try to set the 
> > > rate using clk_set_rate(). clk_set_rate() in this circumstance can't 
> > > wait for the PLL to lock because it can't - there's no reference clock 
> > > for it.
> > 
> > As an aside, this seems like a good time to mention that the behavior of 
> > clk_set_rate() on a disabled clock needs to be clarified.
> 
> It's more complicated than that.  Clocks have more states than just
> enabled and disabled.
> 
> There is:
> - unprepared
> - prepared and disabled
> - prepared and enabled
> 
> Implementations can chose at which point to enable their PLLs and wait
> for them to lock - but if they want to sleep while waiting, they must
> do this in the prepare method, not the enable method (remember, enable
> is to be callable from atomic contexts.)
> 
> So, it's entirely possible that a prepared clock will have the PLLs up
> and running, which means that clk_set_rate() can also wait for the PLL
> to stablize (which would be a good idea.)
> 
> Now... we can say that PLLs should be locked when the prepare method
> returns, or whenever clk_set_rate() returns.  The problem with that is
> there's a race condition between clk_enable() and clk_set_rate() - if
> we allow clk_set_rate() to sleep waiting for the PLL to lock, a
> concurrent clk_enable() can't be prevented from returning because
> that would involve holding a spinlock...
But you can achieve that the concurrent clk_enable fails. Would that be
sane?

Best regards
Uwe
Paul Walmsley Dec. 3, 2011, 1:04 a.m. | #16
On Thu, 1 Dec 2011, Russell King - ARM Linux wrote:

> On Thu, Dec 01, 2011 at 11:30:16AM -0700, Paul Walmsley wrote:
> > The intention behind the clk_{allow,block}_rate_change() proposal was to 
> > allow the current user of the clock to change its rate without having to 
> > call clk_{allow,block}_rate_change(), if that driver was the sole user of 
> > the clock.
> 
> And how does a driver know that?

The driver author might make this assumption when the device's upstream 
clock tree is simple.  This assumption simplifies the driver's clock code, 
since no clock rate change notifier callback would be needed.

This should be possible:

1. when there are no other users of the device's upstream clocks; or,

2. when a subset of the device's upstream clock tree can be used by
   other devices, but the rates or source clocks of that subset either 
   cannot be changed, or should never change.

For example, consider an external audio codec device, such as the 
TLV320AIC23.  The input clock source rate for this chip is likely to be 
fixed for a given board design[1].  So the driver wouldn't need a clock 
rate change notifier callback for its input clock source, since it would 
never be called.  The clock dividers inside the codec may be represented 
by clock nodes, but since they are handled by the same driver, the 
coordination between them doesn't require any notifier callbacks.

Another example would be a device that shares a portion of its upstream 
clock tree with other devices, where the upstream clock tree is designed 
to run at the same rate for all of the devices, such as the USIM IP block 
on OMAP3430/3630[2].  The USIM shares a dedicated DPLL with the USBHOST 
and USBTLL devices, but the DPLL is only intended to be programmed at a 
single rate.  When the USIM is using this DPLL as a clock source, a 
dedicated downstream clock divider is available that can be used to reduce 
the USIM's input clock rate.  So the USIM driver would also not require 
any clock rate change notifier callbacks, since the shared portions of its 
parent clock tree should never change.

Of course, it is possible that a new chip could appear with an identical 
IP block inside it, but where cases 1 and 2 above are no longer true.  
The worst outcome is that the driver's clk_set_rate() may no longer 
succeed, returning -EBUSY, since another device may be blocking the rate 
change.  The remedy is to update the driver to add the rate change 
callback, and the associated internal logic to deal with it.


- Paul

1. Texas Instruments, _TLV320AIC23 Stereo Audio CODEC, 8- to 96-kHz, With 
   Integrated Headphone Amplifier Data Manual_ (SLWS106D), May 2002, 
   Section 3.3.2 "Audio Sampling Rates" and section 1.2 "Functional Block 
   Diagram".   Available from http://www.ti.com/lit/gpn/tlv320aic23
   (among others)

2. Linux kernel v3.1-rc4, arch/arm/mach-omap2/clock3xxx_data.c, 
   usim_clksel (line 2300).  Available from 
   http://git.kernel.org/gitweb.cgi?p=linux/kernel/git/torvalds/linux.git;a=blob;f=arch/arm/mach-omap2/clock3xxx_data.c;h=5d0064a4fb5a638bb1141489354244e95899c1a1;hb=HEAD#l2300
   (among others)
Paul Walmsley Dec. 5, 2011, 9:15 p.m. | #17
Hi

a brief comment concerning clock rates:

On Mon, 21 Nov 2011, Mike Turquette wrote:

> +unsigned long clk_get_rate(struct clk *clk)

...

> +long clk_round_rate(struct clk *clk, unsigned long rate)

...

> +int clk_set_rate(struct clk *clk, unsigned long rate)

...

> +struct clk {
...
> +	unsigned long		rate;
...
> +};

The types associated with clock rates in the clock interface 
(include/linux/clk.h) are inconsistent, and we should fix this. 
clk_round_rate() is the problematic case, returning a signed long rather 
than an unsigned long.  So clk_round_rate() won't work correctly with any 
rates higher than 2 GiHz.

We could fix the immediate problem by changing the prototype of 
clk_round_rate() to pass the rounded rate back to the caller via a pointer 
in one of the arguments, and return an error code (if any) via the return 
value:

int clk_round_rate(struct clk *clk, unsigned long rate, unsigned long 
                   *rounded_rate);

But I'd propose that we instead increase the size of struct clk.rate to be 
s64:

s64 clk_round_rate(struct clk *clk, s64 desired_rate);
int clk_set_rate(struct clk *clk, s64 rate);
s64 clk_get_rate(struct clk *clk);

struct clk {
...
     s64 rate;
...
};

That way the clock framework can accommodate current clock rates, as well 
as any conceivable future clock rate.  (Some production CPUs are already 
running at clock rates greater than 4 GiHZ[1].  RF devices with 4 GiHz+ 
clock rates are also common, such as 802.11a devices running in the 5.8 
GHz band, and drivers for those may eventually wish to use the clock 
framework.)



- Paul

1. www.cpu-world.com, "Intel Xeon X5698 - AT80614007314AA" 
   http://www.cpu-world.com/CPUs/Xeon/Intel-Xeon%20X5698%20-%20AT80614007314AA.html
Mike Turquette Dec. 5, 2011, 11:40 p.m. | #18
On Mon, Nov 21, 2011 at 5:40 PM, Mike Turquette <mturquette@ti.com> wrote:
> +/**
> + * clk_get_parent - return the parent of a clk
> + * @clk: the clk whose parent gets returned
> + *
> + * Simply returns clk->parent.  It is up to the caller to hold the
> + * prepare_lock, if desired.  Returns NULL if clk is NULL.
> + */
> +struct clk *clk_get_parent(struct clk *clk)
> +{
> +       if (!clk)
> +               return NULL;
> +
> +       return clk->parent;
> +}
> +EXPORT_SYMBOL_GPL(clk_get_parent);

While auditing the uses/expectations of the current clk API users, I
see that clk_get_parent is rarely used; it is in fact only called in
11 files throughout the kernel.

I decided to have a little audit and here is what I found:

arch/arm/mach-shmobile/board-ap4evb.c: fsi_ak4642_set_rate
Used within clk_set_rate.  Can dereference clk->parent directly and
ideally should propagate up the clk tree using the new recursive
clk_set_rate.

arch/arm/mach-tegra/usb_phy.c: tegra_usb_phy_open
Used within a clk_get_rate.  pll_u should ideally have it's own
clk->rate populated, reducing the need for tegra_usb_phy_open to know
details of the clk tree.  The logic to pull pll_u's rate from it's
parent belongs to pll_u's .recalc_rate callback.

arch/arm/plat-s5p/clock.c: s5p_spdif_set_rate
Another clk_get_parent call from within a .set_rate callback.  Again:
use clk->parent directly and better yet, propagate the rate change up
via the recursive clk_set_rate.

arch/arm/plat-s5p/clock.c: s5p_spdif_get_rate
Another clk_get_parent call from within a .recalc_rate callback.
Again, clk->rate should be populated with parent's rate correctly,
otherwise dereference clk->parent directly without use of
clk_get_parent.

arch/arm/plat-s5p/s5p-time.c: s5p_clockevent_init
This problem would be solved by propagating rate_change requests up
two-levels of parents via the new recursive clk_set_rate.  There is
also a clk_set_parent call in here, which has nothing to do with the
clk_get_parent call, but it makes me wonder if we should revisit the
issue of switching parents as part of clk_set_rate:
clk_set_rate(tin_event, pclk->rate / 2);

arch/arm/plat-samsung/pwm.c: pwm_is_tdiv
Used in only two places: as part of pwm_calc_tin which could be
replaced wholesale by a better .round_rate implementation and for a
debug print (who cares).

arch/arm/plat-samsung/pwm.c: pwm_calc_tin
Just a .round_rate implementation.  Can dereference clk->parent directly.

arch/arm/plat-samsung/time.c: s3c2410_timer_setup
Same as s5p_clockevent_init above.

drivers/sh/clk/core.c: clk_round_parent
An elaborate .round_rate that should have resulted from recursive
propagation up to clk->parent.

drivers/video/omap2/dss/dss.c:
Every call to clk_get_parent in here is wrapped in clk_get_rate.  The
functions that use this are effectively .set_rate, .get_rate and
.recalc_rate doppelgangers.  Also a debug print calls clk_get_parent
but who cares.

drivers/video/sh_mobile_hdmi.c:
Used in a .set_rate and .round_rate implementation.  No reason not to
deref clk->parent directly.

The above explanations take a little liberty listing certain functions
as .round_rate, .set_rate and .recalc_rate callbacks, but that is
because a lot of the code mentioned could be neatly wrapped up that
way.

Do we really need clk_get_parent?  The primary problem with it is
ambiguity in the API: should the caller hold a lock?  Is the rate
guaranteed not the change after being called?  A top-level clk API
function that can be called within other top-level clk API functions
is inconsitent: most of the time this would incur nested locking.
Also having a top-level API expose the tree structure encourages
platform and driver developers to put clk tree details into their code
as opposed to having clever .round_rate and .set_rate callbacks hide
these details from them with the new recursive clk_set_rate.

Thoughts?

Thanks,
Mike
Russell King - ARM Linux Dec. 5, 2011, 11:48 p.m. | #19
On Mon, Dec 05, 2011 at 02:15:56PM -0700, Paul Walmsley wrote:
> The types associated with clock rates in the clock interface 
> (include/linux/clk.h) are inconsistent, and we should fix this. 

Rubbish.  They're different with good reason.  Rates are primerily
unsigned quantities - and should be treated as such.

The exception is clk_round_rate() which returns the rate, but also
_may_ return an error.  Therefore, its return type has to be signed.

> We could fix the immediate problem by changing the prototype of 
> clk_round_rate() to pass the rounded rate back to the caller via a pointer 
> in one of the arguments, and return an error code (if any) via the return 
> value:
> 
> int clk_round_rate(struct clk *clk, unsigned long rate, unsigned long 
>                    *rounded_rate);

Yes that might have been a better solution.

> But I'd propose that we instead increase the size of struct clk.rate to be 
> s64:
> 
> s64 clk_round_rate(struct clk *clk, s64 desired_rate);
> int clk_set_rate(struct clk *clk, s64 rate);
> s64 clk_get_rate(struct clk *clk);
> 
> struct clk {
> ...
>      s64 rate;
> ...
> };
> 
> That way the clock framework can accommodate current clock rates, as well 
> as any conceivable future clock rate.  (Some production CPUs are already 
> running at clock rates greater than 4 GiHZ[1].  RF devices with 4 GiHz+ 
> clock rates are also common, such as 802.11a devices running in the 5.8 
> GHz band, and drivers for those may eventually wish to use the clock 
> framework.)

Yuck.  You are aware that 64-bit math on 32-bit CPUs sucks?  So burdening
_everything_ with 64-bit rate quantities is absurd.  As for making then
64-bit signed quantities, that's asking for horrid code from gcc for the
majority of cases.
Paul Walmsley Dec. 6, 2011, 1:37 a.m. | #20
On Mon, 5 Dec 2011, Russell King - ARM Linux wrote:

> On Mon, Dec 05, 2011 at 02:15:56PM -0700, Paul Walmsley wrote:
>
> > But I'd propose that we instead increase the size of struct clk.rate to be 
> > s64:
> > 
> > s64 clk_round_rate(struct clk *clk, s64 desired_rate);
> > int clk_set_rate(struct clk *clk, s64 rate);
> > s64 clk_get_rate(struct clk *clk);
> > 
> > struct clk {
> > ...
> >      s64 rate;
> > ...
> > };
> > 
> > That way the clock framework can accommodate current clock rates, as well 
> > as any conceivable future clock rate.  (Some production CPUs are already 
> > running at clock rates greater than 4 GiHZ[1].  RF devices with 4 GiHz+ 
> > clock rates are also common, such as 802.11a devices running in the 5.8 
> > GHz band, and drivers for those may eventually wish to use the clock 
> > framework.)
> 
> Yuck.  You are aware that 64-bit math on 32-bit CPUs sucks? So burdening 
> _everything_ with 64-bit rate quantities is absurd.  As for making then 
> 64-bit signed quantities, that's asking for horrid code from gcc for the 
> majority of cases.

64-bit divides would be the only real issue in the clock framework 
context.  And those are easily avoided on clock hardware where the parent 
rate can never exceed 32 bits.

For example, here's a trivial implementation for rate recalculation for a 
integer divider clock node (that can't be handled with a right shift):

s64 div(struct clk *clk, u32 div) {
	if (clk->flags & CLK_PARENT_RATE_MAX_U32)
		return ((u32)(clk->parent->rate & 0xffffffff)) / div;

	clk->rate = clk->parent->rate;
	do_div(clk->rate, div);
	return clk->rate;
}

gcc generates this for ARMv6:

00000010 <div>:
  10:	e92d4038 	push	{r3, r4, r5, lr}
  14:	e1a05000 	mov	r5, r0         
  18:	e5d03010 	ldrb	r3, [r0, #16]  @ clk->flags
  1c:	e1a04001 	mov	r4, r1
  20:	e3130001 	tst	r3, #1         @ 32-bit parent rate?
  24:	1a000007 	bne	48 <div+0x38>  @ do 32-bit divide 

/* 64-bit divide by 32-bit follows */

  28:	e5903000 	ldr	r3, [r0]
  2c:	e1c300d8 	ldrd	r0, [r3, #8]
  30:	ebfffffe 	bl	0 <__do_div64>
  34:	e1a00002 	mov	r0, r2
  38:	e1a01003 	mov	r1, r3
  3c:	e5852008 	str	r2, [r5, #8]
  40:	e585300c 	str	r3, [r5, #12]
  44:	e8bd8038 	pop	{r3, r4, r5, pc}

/* 32-bit divide follows */

  48:	e5900000 	ldr	r0, [r0]
  4c:	e5900008 	ldr	r0, [r0, #8]
  50:	ebfffffe 	bl	0 <__aeabi_uidiv>
  54:	e3a01000 	mov	r1, #0
  58:	e8bd8038 	pop	{r3, r4, r5, pc}


Not bad at all, and this isn't even optimized.  (The conditional could be 
avoided completely with a little work during clock init.)  What little 
overhead there is seems quite trivial if it means that the clock framework 
will be useful for present and future devices.


- Paul
Paul Walmsley Dec. 6, 2011, 6:28 a.m. | #21
On Mon, 5 Dec 2011, Paul Walmsley wrote:

> For example, here's a trivial implementation for rate recalculation for a 
> integer divider clock node (that can't be handled with a right shift):
> 
> s64 div(struct clk *clk, u32 div) {
> 	if (clk->flags & CLK_PARENT_RATE_MAX_U32)
> 		return ((u32)(clk->parent->rate & 0xffffffff)) / div;
> 
> 	clk->rate = clk->parent->rate;
> 	do_div(clk->rate, div);
> 	return clk->rate;
> }

(removing some useless cruft from the above function, for clarity's sake)

s64 div(struct clk *clk, u32 div) {
	s64 r;

	if (clk->flags & CLK_PARENT_RATE_MAX_U32)
		return ((u32)clk->parent->rate) / div;

	r = clk->parent->rate;
	do_div(r, div);
	return r;
}

00000000 <div>:
   0:	e92d4010 	push	{r4, lr}
   4:	e1a04001 	mov	r4, r1
   8:	e5d03010 	ldrb	r3, [r0, #16]
   c:	e3130001 	tst	r3, #1
  10:	1a000005 	bne	2c <div+0x2c>

  14:	e5903000 	ldr	r3, [r0]
  18:	e1c300d8 	ldrd	r0, [r3, #8]
  1c:	ebfffffe 	bl	0 <__do_div64>
  20:	e1a00002 	mov	r0, r2
  24:	e1a01003 	mov	r1, r3
  28:	e8bd8010 	pop	{r4, pc}

  2c:	e5900000 	ldr	r0, [r0]
  30:	e5900008 	ldr	r0, [r0, #8]
  34:	ebfffffe 	bl	0 <__aeabi_uidiv>
  38:	e3a01000 	mov	r1, #0
  3c:	e8bd8010 	pop	{r4, pc}


- Paul

Patch

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 7a9899bd..adc0586 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -8,3 +8,7 @@  config HAVE_MACH_CLKDEV
 
 config HAVE_CLK_PREPARE
 	bool
+
+config GENERIC_CLK
+	bool
+	select HAVE_CLK_PREPARE
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 07613fa..570d5b9 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -1,2 +1,3 @@ 
 
 obj-$(CONFIG_CLKDEV_LOOKUP)	+= clkdev.o
+obj-$(CONFIG_GENERIC_CLK)	+= clk.o
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
new file mode 100644
index 0000000..12c9994
--- /dev/null
+++ b/drivers/clk/clk.c
@@ -0,0 +1,538 @@ 
+/*
+ * Copyright (C) 2010-2011 Canonical Ltd <jeremy.kerr@canonical.com>
+ * Copyright (C) 2011 Linaro Ltd <mturquette@linaro.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * Standard functionality for the common clock API.  See Documentation/clk.txt
+ */
+
+#include <linux/clk.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+#include <linux/err.h>
+
+static DEFINE_SPINLOCK(enable_lock);
+static DEFINE_MUTEX(prepare_lock);
+
+void __clk_unprepare(struct clk *clk)
+{
+	if (!clk)
+		return;
+
+	if (WARN_ON(clk->prepare_count == 0))
+		return;
+
+	if (--clk->prepare_count > 0)
+		return;
+
+	WARN_ON(clk->enable_count > 0);
+
+	if (clk->ops->unprepare)
+		clk->ops->unprepare(clk);
+
+	__clk_unprepare(clk->parent);
+}
+
+/**
+ * clk_unprepare - perform the slow part of a clk gate
+ * @clk: the clk being gated
+ *
+ * clk_unprepare may sleep, which differentiates it from clk_disable.  In a
+ * simple case, clk_unprepare can be used instead of clk_disable to gate a clk
+ * if the operation may sleep.  One example is a clk which is accessed over
+ * I2c.  In the complex case a clk gate operation may require a fast and a slow
+ * part.  It is this reason that clk_unprepare and clk_disable are not mutually
+ * exclusive.  In fact clk_disable must be called before clk_unprepare.
+ */
+void clk_unprepare(struct clk *clk)
+{
+	mutex_lock(&prepare_lock);
+	__clk_unprepare(clk);
+	mutex_unlock(&prepare_lock);
+}
+EXPORT_SYMBOL_GPL(clk_unprepare);
+
+int __clk_prepare(struct clk *clk)
+{
+	int ret = 0;
+
+	if (!clk)
+		return 0;
+
+	if (clk->prepare_count == 0) {
+		ret = __clk_prepare(clk->parent);
+		if (ret)
+			return ret;
+
+		if (clk->ops->prepare) {
+			ret = clk->ops->prepare(clk);
+			if (ret) {
+				__clk_unprepare(clk->parent);
+				return ret;
+			}
+		}
+	}
+
+	clk->prepare_count++;
+
+	return 0;
+}
+
+/**
+ * clk_prepare - perform the slow part of a clk ungate
+ * @clk: the clk being ungated
+ *
+ * clk_prepare may sleep, which differentiates it from clk_enable.  In a simple
+ * case, clk_prepare can be used instead of clk_enable to ungate a clk if the
+ * operation may sleep.  One example is a clk which is accessed over I2c.  In
+ * the complex case a clk ungate operation may require a fast and a slow part.
+ * It is this reason that clk_prepare and clk_enable are not mutually
+ * exclusive.  In fact clk_prepare must be called before clk_enable.
+ * Returns 0 on success, -EERROR otherwise.
+ */
+int clk_prepare(struct clk *clk)
+{
+	int ret;
+
+	mutex_lock(&prepare_lock);
+	ret = __clk_prepare(clk);
+	mutex_unlock(&prepare_lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(clk_prepare);
+
+void __clk_disable(struct clk *clk)
+{
+	if (!clk)
+		return;
+
+	if (WARN_ON(clk->enable_count == 0))
+		return;
+
+	if (--clk->enable_count > 0)
+		return;
+
+	if (clk->ops->disable)
+		clk->ops->disable(clk);
+
+	if (clk->parent)
+		__clk_disable(clk->parent);
+}
+
+/**
+ * clk_disable - perform the fast part of a clk gate
+ * @clk: the clk being gated
+ *
+ * clk_disable must not sleep, which differentiates it from clk_unprepare.  In
+ * a simple case, clk_disable can be used instead of clk_unprepare to gate a
+ * clk if the operation is fast and will never sleep.  One example is a
+ * SoC-internal clk which is controlled via simple register writes.  In the
+ * complex case a clk gate operation may require a fast and a slow part.  It is
+ * this reason that clk_unprepare and clk_disable are not mutually exclusive.
+ * In fact clk_disable must be called before clk_unprepare.
+ */
+void clk_disable(struct clk *clk)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&enable_lock, flags);
+	__clk_disable(clk);
+	spin_unlock_irqrestore(&enable_lock, flags);
+}
+EXPORT_SYMBOL_GPL(clk_disable);
+
+int __clk_enable(struct clk *clk)
+{
+	int ret = 0;
+
+	if (!clk)
+		return 0;
+
+	if (WARN_ON(clk->prepare_count == 0))
+		return -ESHUTDOWN;
+
+	if (clk->enable_count == 0) {
+		if (clk->parent)
+			ret = __clk_enable(clk->parent);
+
+		if (ret)
+			return ret;
+
+		if (clk->ops->enable) {
+			ret = clk->ops->enable(clk);
+			if (ret) {
+				__clk_disable(clk->parent);
+				return ret;
+			}
+		}
+	}
+
+	clk->enable_count++;
+	return 0;
+}
+
+/**
+ * clk_enable - perform the slow part of a clk ungate
+ * @clk: the clk being ungated
+ *
+ * clk_enable must not sleep, which differentiates it from clk_prepare.  In a
+ * simple case, clk_enable can be used instead of clk_prepare to ungate a clk
+ * if the operation will never sleep.  One example is a SoC-internal clk which
+ * is controlled via simple register writes.  In the complex case a clk ungate
+ * operation may require a fast and a slow part.  It is this reason that
+ * clk_enable and clk_prepare are not mutually exclusive.  In fact clk_prepare
+ * must be called before clk_enable.  Returns 0 on success, -EERROR
+ * otherwise.
+ */
+int clk_enable(struct clk *clk)
+{
+	unsigned long flags;
+	int ret;
+
+	spin_lock_irqsave(&enable_lock, flags);
+	ret = __clk_enable(clk);
+	spin_unlock_irqrestore(&enable_lock, flags);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(clk_enable);
+
+/**
+ * clk_get_rate - return the rate of clk
+ * @clk: the clk whose rate is being returned
+ *
+ * Simply returns the cached rate of the clk.  Does not query the hardware.  If
+ * clk is NULL then returns 0.
+ */
+unsigned long clk_get_rate(struct clk *clk)
+{
+	if (!clk)
+		return 0;
+
+	return clk->rate;
+}
+EXPORT_SYMBOL_GPL(clk_get_rate);
+
+/**
+ * clk_round_rate - round the given rate for a clk
+ * @clk: the clk for which we are rounding a rate
+ * @rate: the rate which is to be rounded
+ *
+ * Takes in a rate as input and rounds it to a rate that the clk can actually
+ * use which is then returned.  If clk doesn't support round_rate operation
+ * then the rate passed in is returned.
+ */
+long clk_round_rate(struct clk *clk, unsigned long rate)
+{
+	if (clk && clk->ops->round_rate)
+		return clk->ops->round_rate(clk, rate, NULL);
+
+	return rate;
+}
+EXPORT_SYMBOL_GPL(clk_round_rate);
+
+/**
+ * clk_recalc_rates
+ * @clk: first clk in the subtree
+ *
+ * Walks the subtree of clks starting with clk and recalculates rates as it
+ * goes.  Note that if a clk does not implement the recalc_rate operation then
+ * propagation of that subtree stops and all of that clks children will not
+ * have their rates updated.  Caller must hold prepare_lock.
+ */
+static void clk_recalc_rates(struct clk *clk)
+{
+	unsigned long old_rate;
+	struct hlist_node *tmp;
+	struct clk *child;
+
+	old_rate = clk->rate;
+
+	if (clk->ops->recalc_rate)
+		clk->rate = clk->ops->recalc_rate(clk);
+
+	if (old_rate == clk->rate)
+		return;
+
+	/* FIXME add post-rate change notification here */
+
+	hlist_for_each_entry(child, tmp, &clk->children, child_node)
+		clk_recalc_rates(child);
+}
+
+/**
+ * DOC: Using the CLK_PARENT_SET_RATE flag
+ *
+ * __clk_set_rate changes the child's rate before the parent's to more
+ * easily handle failure conditions.
+ *
+ * This means clk might run out of spec for a short time if its rate is
+ * increased before the parent's rate is updated.
+ *
+ * To prevent this consider setting the CLK_GATE_SET_RATE flag on any
+ * clk where you also set the CLK_PARENT_SET_RATE flag
+ */
+struct clk *__clk_set_rate(struct clk *clk, unsigned long rate)
+{
+	struct clk *fail_clk = NULL;
+	int ret;
+	unsigned long old_rate = clk->rate;
+	unsigned long new_rate;
+	unsigned long parent_old_rate;
+	unsigned long parent_new_rate = 0;
+	struct clk *child;
+	struct hlist_node *tmp;
+
+	/* bail early if we can't change rate while clk is enabled */
+	if ((clk->flags & CLK_GATE_SET_RATE) && clk->enable_count)
+		return clk;
+
+	/* find the new rate and see if parent rate should change too */
+	WARN_ON(!clk->ops->round_rate);
+
+	new_rate = clk->ops->round_rate(clk, rate, &parent_new_rate);
+
+	/* FIXME propagate pre-rate change notification here */
+	/* XXX note that pre-rate change notifications will stack */
+
+	/* change the rate of this clk */
+	ret = clk->ops->set_rate(clk, new_rate);
+	if (ret)
+		return clk;
+
+	/*
+	 * change the rate of the parent clk if necessary
+	 *
+	 * hitting the nested 'if' path implies we have hit a .set_rate
+	 * failure somewhere upstream while propagating __clk_set_rate
+	 * up the clk tree.  roll back the clk rates one by one and
+	 * return the pointer to the clk that failed.  clk_set_rate will
+	 * use the pointer to propagate a rate-change abort notifier
+	 * from the "highest" point.
+	 */
+	if ((clk->flags & CLK_PARENT_SET_RATE) && parent_new_rate) {
+		parent_old_rate = clk->parent->rate;
+		fail_clk = __clk_set_rate(clk->parent, parent_new_rate);
+
+		/* roll back changes if parent rate change failed */
+		if (fail_clk) {
+			pr_warn("%s: failed to set parent %s rate to %lu\n",
+					__func__, fail_clk->name,
+					parent_new_rate);
+			clk->ops->set_rate(clk, old_rate);
+		}
+		return fail_clk;
+	}
+
+	/*
+	 * set clk's rate & recalculate the rates of clk's children
+	 *
+	 * hitting this path implies we have successfully finished
+	 * propagating recursive calls to __clk_set_rate up the clk tree
+	 * (if necessary) and it is safe to propagate clk_recalc_rates and
+	 * post-rate change notifiers down the clk tree from this point.
+	 */
+	clk->rate = new_rate;
+	/* FIXME propagate post-rate change notifier for only this clk */
+	hlist_for_each_entry(child, tmp, &clk->children, child_node)
+		clk_recalc_rates(child);
+
+	return fail_clk;
+}
+
+/**
+ * clk_set_rate - specify a new rate for clk
+ * @clk: the clk whose rate is being changed
+ * @rate: the new rate for clk
+ *
+ * In the simplest case clk_set_rate will only change the rate of clk.
+ *
+ * If clk has the CLK_GATE_SET_RATE flag set and it is enabled this call
+ * will fail; only when the clk is disabled will it be able to change
+ * its rate.
+ *
+ * Setting the CLK_PARENT_SET_RATE flag allows clk_set_rate to
+ * recursively propagate up to clk's parent; whether or not this happens
+ * depends on the outcome of clk's .round_rate implementation.  If
+ * *parent_rate is 0 after calling .round_rate then upstream parent
+ * propagation is ignored.  If *parent_rate comes back with a new rate
+ * for clk's parent then we propagate up to clk's parent and set it's
+ * rate.  Upward propagation will continue until either a clk does not
+ * support the CLK_PARENT_SET_RATE flag or .round_rate stops requesting
+ * changes to clk's parent_rate.  If there is a failure during upstream
+ * propagation then clk_set_rate will unwind and restore each clk's rate
+ * that had been successfully changed.  Afterwards a rate change abort
+ * notification will be propagated downstream, starting from the clk
+ * that failed.
+ *
+ * At the end of all of the rate setting, clk_set_rate internally calls
+ * clk_recalc_rates and propagates the rate changes downstream, starting
+ * from the highest clk whose rate was changed.  This has the added
+ * benefit of propagating post-rate change notifiers.
+ *
+ * Note that while post-rate change and rate change abort notifications
+ * are guaranteed to be sent to a clk only once per call to
+ * clk_set_rate, pre-change notifications will be sent for every clk
+ * whose rate is changed.  Stacking pre-change notifications is noisy
+ * for the drivers subscribed to them, but this allows drivers to react
+ * to intermediate clk rate changes up until the point where the final
+ * rate is achieved at the end of upstream propagation.
+ *
+ * Returns 0 on success, -EERROR otherwise.
+ */
+int clk_set_rate(struct clk *clk, unsigned long rate)
+{
+	struct clk *fail_clk;
+	int ret = 0;
+
+	if (!clk->ops->set_rate)
+		return -ENOSYS;
+
+	/* bail early if nothing to do */
+	if (rate == clk->rate)
+		return rate;
+
+	/* prevent racing with updates to the clock topology */
+	mutex_lock(&prepare_lock);
+	fail_clk = __clk_set_rate(clk, rate);
+	if (fail_clk) {
+		pr_warn("%s: failed to set %s rate to %lu\n",
+				__func__, fail_clk->name, rate);
+		/* FIXME propagate rate change abort notification here */
+		ret = -EIO;
+	}
+	mutex_unlock(&prepare_lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(clk_set_rate);
+
+/**
+ * clk_get_parent - return the parent of a clk
+ * @clk: the clk whose parent gets returned
+ *
+ * Simply returns clk->parent.  It is up to the caller to hold the
+ * prepare_lock, if desired.  Returns NULL if clk is NULL.
+ */
+struct clk *clk_get_parent(struct clk *clk)
+{
+	if (!clk)
+		return NULL;
+
+	return clk->parent;
+}
+EXPORT_SYMBOL_GPL(clk_get_parent);
+
+void __clk_reparent(struct clk *clk, struct clk *new_parent)
+{
+	hlist_del(&clk->child_node);
+	hlist_add_head(&clk->child_node, &new_parent->children);
+
+	clk->parent = new_parent;
+
+	/* FIXME update sysfs clock topology */
+}
+
+/**
+ * clk_set_parent - switch the parent of a mux clk
+ * @clk: the mux clk whose input we are switching
+ * @parent: the new input to clk
+ *
+ * Re-parent clk to use parent as it's new input source.  If clk has the
+ * CLK_GATE_SET_PARENT flag set then clk must be gated for this
+ * operation to succeed.  After successfully changing clk's parent
+ * clk_set_parent will update the clk topology, sysfs topology and
+ * propagate rate recalculation via clk_recalc_rates.  Returns 0 on
+ * success, -EERROR otherwise.
+ */
+int clk_set_parent(struct clk *clk, struct clk *parent)
+{
+	int ret = 0;
+
+	if (!clk || !clk->ops)
+		return -EINVAL;
+
+	if (!clk->ops->set_parent)
+		return -ENOSYS;
+
+	if (clk->parent == parent)
+		return 0;
+
+	/* prevent racing with updates to the clock topology */
+	mutex_lock(&prepare_lock);
+
+	/* FIXME add pre-rate change notification here */
+
+	/* only re-parent if the clock is not in use */
+	if ((clk->flags & CLK_GATE_SET_PARENT) && clk->prepare_count)
+		ret = -EBUSY;
+	else
+		ret = clk->ops->set_parent(clk, parent);
+
+	if (ret < 0)
+		/* FIXME add rate change abort notification here */
+		goto out;
+
+	/* update tree topology */
+	__clk_reparent(clk, parent);
+
+	/*
+	 * If successful recalculate the rates of the clock, including
+	 * children.
+	 */
+	clk_recalc_rates(clk);
+
+out:
+	mutex_unlock(&prepare_lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(clk_set_parent);
+
+/**
+ * clk_init - initialize the data structures in a struct clk
+ * @dev: device initializing this clk, placeholder for now
+ * @clk: clk being initialized
+ *
+ * Initializes the lists in struct clk, queries the hardware for the
+ * parent and rate and sets them both.  Adds the clk to the sysfs tree
+ * topology.
+ *
+ * Caller must populate clk->name and clk->flags before calling
+ * clk_init.  A key assumption in the call to .get_parent is that clks
+ * are being initialized in-order.
+ */
+void clk_init(struct device *dev, struct clk *clk)
+{
+	if (!clk)
+		return;
+
+	INIT_HLIST_HEAD(&clk->children);
+	INIT_HLIST_NODE(&clk->child_node);
+
+	mutex_lock(&prepare_lock);
+
+	if (clk->ops->get_parent) {
+		clk->parent = clk->ops->get_parent(clk);
+		if (clk->parent)
+			hlist_add_head(&clk->child_node,
+					&clk->parent->children);
+	} else
+		clk->parent = NULL;
+
+	if (clk->ops->recalc_rate)
+		clk->rate = clk->ops->recalc_rate(clk);
+	else
+		clk->rate = 0;
+
+	mutex_unlock(&prepare_lock);
+
+	return;
+}
+EXPORT_SYMBOL_GPL(clk_init);
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 7213b52..3b0eb3f 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -3,6 +3,8 @@ 
  *
  *  Copyright (C) 2004 ARM Limited.
  *  Written by Deep Blue Solutions Limited.
+ *  Copyright (c) 2010-2011 Jeremy Kerr <jeremy.kerr@canonical.com>
+ *  Copyright (C) 2011 Linaro Ltd <mturquette@linaro.org>
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
@@ -13,17 +15,134 @@ 
 
 #include <linux/kernel.h>
 
+#include <linux/kernel.h>
+#include <linux/errno.h>
+
 struct device;
 
+struct clk;
+
+#ifdef CONFIG_GENERIC_CLK
+
 /*
- * The base API.
+ * flags used across common struct clk.  these flags should only affect the
+ * top-level framework.  custom flags for dealing with hardware specifics
+ * belong in struct clk_hw_your_unique_device
  */
+#define CLK_GATE_SET_RATE	BIT(0) /* must be gated across rate change */
+#define CLK_GATE_SET_PARENT	BIT(1) /* must be gated across re-parent */
+#define CLK_PARENT_SET_RATE	BIT(2) /* propagate rate change up one level */
+#define CLK_IGNORE_UNUSED	BIT(3) /* do not gate even if unused */
 
+#define CLK_GATE_RATE_CHANGE	(CLK_GATE_SET_RATE | CLK_GATE_SET_PARENT)
 
-/*
- * struct clk - an machine class defined object / cookie.
+struct clk {
+	const char		*name;
+	const struct clk_hw_ops	*ops;
+	struct clk		*parent;
+	unsigned long		rate;
+	unsigned long		flags;
+	unsigned int		enable_count;
+	unsigned int		prepare_count;
+	struct hlist_head	children;
+	struct hlist_node	child_node;
+};
+
+/**
+ * struct clk_hw_ops -  Callback operations for hardware clocks; these are to
+ * be provided by the clock implementation, and will be called by drivers
+ * through the clk_* API.
+ *
+ * @prepare:	Prepare the clock for enabling. This must not return until
+ * 		the clock is fully prepared, and it's safe to call clk_enable.
+ * 		This callback is intended to allow clock implementations to
+ * 		do any initialisation that may sleep. Called with
+ * 		prepare_lock held.
+ *
+ * @unprepare:	Release the clock from its prepared state. This will typically
+ * 		undo any work done in the @prepare callback. Called with
+ * 		prepare_lock held.
+ *
+ * @enable:	Enable the clock atomically. This must not return until the
+ * 		clock is generating a valid clock signal, usable by consumer
+ * 		devices. Called with enable_lock held. This function must not
+ * 		sleep.
+ *
+ * @disable:	Disable the clock atomically. Called with enable_lock held.
+ * 		This function must not sleep.
+ *
+ * @recalc_rate	Recalculate the rate of this clock, by quering hardware
+ * 		and/or the clock's parent. It is up to the caller to insure
+ * 		that the prepare_mutex is held across this call.  Returns the
+ * 		calculated rate.  Optional, but recommended - if this op is not
+ * 		set then clock rate will be initialized to 0.
+ *
+ * @round_rate	XXX FIXME
+ *
+ * @get_parent	Return the parent of this clock; for clocks with multiple
+ * 		possible parents, query the hardware for the current parent.
+ * 		Clocks with fixed parents should still implement this and
+ * 		return something like struct clk *fixed_parent from their
+ * 		respective struct clk_hw_* data.  Currently called only when
+ * 		the clock is initialized.  Clocks that do not implement this
+ * 		will have their parent set to NULL.
+ *
+ * @set_parent	Change the input source of this clock; for clocks with multiple
+ * 		possible parents specify a new parent by passing in a struct
+ * 		clk *parent.  Returns 0 on success, -EERROR otherwise.
+ *
+ * @set_rate	Change the rate of this clock. If this callback returns
+ * 		CLK_PARENT_SET_RATE, the rate change will be propagated to the
+ * 		parent clock (which may propagate again if the parent clock
+ * 		also sets this flag). The requested rate of the parent is
+ * 		passed back from the callback in the second 'unsigned long *'
+ * 		argument.  Note that it is up to the hardware clock's set_rate
+ * 		implementation to insure that clocks do not run out of spec
+ * 		when propgating the call to set_rate up to the parent.  One way
+ * 		to do this is to gate the clock (via clk_disable and/or
+ * 		clk_unprepare) before calling clk_set_rate, then ungating it
+ * 		afterward.  If your clock also has the CLK_GATE_SET_RATE flag
+ * 		set then this will insure safety.  Returns 0 on success,
+ * 		-EERROR otherwise.
+ *
+ * The clk_enable/clk_disable and clk_prepare/clk_unprepare pairs allow
+ * implementations to split any work between atomic (enable) and sleepable
+ * (prepare) contexts.  If enabling a clock requires code that might sleep,
+ * this must be done in clk_prepare.  Clock enable code that will never be
+ * called in a sleepable context may be implement in clk_enable.
+ *
+ * Typically, drivers will call clk_prepare when a clock may be needed later
+ * (eg. when a device is opened), and clk_enable when the clock is actually
+ * required (eg. from an interrupt). Note that clk_prepare MUST have been
+ * called before clk_enable.
  */
-struct clk;
+struct clk_hw_ops {
+	int		(*prepare)(struct clk *clk);
+	void		(*unprepare)(struct clk *clk);
+	int		(*enable)(struct clk *clk);
+	void		(*disable)(struct clk *clk);
+	unsigned long	(*recalc_rate)(struct clk *clk);
+	long		(*round_rate)(struct clk *clk, unsigned long,
+					unsigned long *);
+	int		(*set_parent)(struct clk *clk, struct clk *);
+	struct clk *	(*get_parent)(struct clk *clk);
+	int		(*set_rate)(struct clk *clk, unsigned long);
+};
+
+/**
+ * clk_init - initialize the data structures in a struct clk
+ * @dev: device initializing this clk, placeholder for now
+ * @clk: clk being initialized
+ *
+ * Initializes the lists in struct clk, queries the hardware for the
+ * parent and rate and sets them both.  Adds the clk to the sysfs tree
+ * topology.
+ *
+ * Caller must populate .name, .flags and .ops before calling clk_init.
+ */
+void clk_init(struct device *dev, struct clk *clk);
+
+#endif /* !CONFIG_GENERIC_CLK */
 
 /**
  * clk_get - lookup and obtain a reference to a clock producer.
@@ -107,9 +226,16 @@  static inline void clk_unprepare(struct clk *clk)
 }
 #endif
 
+int __clk_enable(struct clk *clk);
+void __clk_disable(struct clk *clk);
+int __clk_prepare(struct clk *clk);
+void __clk_unprepare(struct clk *clk);
+void __clk_reparent(struct clk *clk, struct clk *new_parent);
+
 /**
  * clk_get_rate - obtain the current clock rate (in Hz) for a clock source.
  *		  This is only valid once the clock source has been enabled.
+ *		  Returns zero if the clock rate is unknown.
  * @clk: clock source
  */
 unsigned long clk_get_rate(struct clk *clk);