diff mbox

[v2,1/7] clk: Add a generic clock infrastructure

Message ID 1316730422-20027-2-git-send-email-mturquette@ti.com
State New
Headers show

Commit Message

Mike Turquette Sept. 22, 2011, 10:26 p.m. UTC
From: Jeremy Kerr <jeremy.kerr@canonical.com>

We currently have ~21 definitions of struct clk in the ARM architecture,
each defined on a per-platform basis. This makes it difficult to define
platform- (or architecture-) independent clock sources without making
assumptions about struct clk, and impossible to compile two
platforms with different struct clks into a single image.

This change is an effort to unify struct clk where possible, by defining
a common struct clk, and a set of clock operations. Different clock
implementations can set their own operations, and have a standard
interface for generic code. The callback interface is exposed to the
kernel proper, while the clock implementations only need to be seen by
the platform internals.

The interface is split into two halves:

 * struct clk, which is the generic-device-driver interface. This
   provides a set of functions which drivers may use to request
   enable/disable, query or manipulate in a hardware-independent manner.

 * struct clk_hw and struct clk_hw_ops, which is the hardware-specific
   interface. Clock drivers implement the ops, which allow the core
   clock code to implement the generic 'struct clk' API.

This allows us to share clock code among platforms, and makes it
possible to dynamically create clock devices in platform-independent
code.

Platforms can enable the generic struct clock through
CONFIG_GENERIC_CLK. In this case, the clock infrastructure consists of a
common, opaque struct clk, and a set of clock operations (defined per
type of clock):

  struct clk_hw_ops {
  	int		(*prepare)(struct clk_hw *);
  	void		(*unprepare)(struct clk_hw *);
  	int		(*enable)(struct clk_hw *);
  	void		(*disable)(struct clk_hw *);
  	unsigned long	(*recalc_rate)(struct clk_hw *);
  	int		(*set_rate)(struct clk_hw *,
  					unsigned long, unsigned long *);
  	long		(*round_rate)(struct clk_hw *, unsigned long);
  	int		(*set_parent)(struct clk_hw *, struct clk *);
  	struct clk *	(*get_parent)(struct clk_hw *);
  };

Platform clock code can register a clock through clk_register, passing a
set of operations, and a pointer to hardware-specific data:

  struct clk_hw_foo {
  	struct clk_hw clk;
  	void __iomem *enable_reg;
  };

  #define to_clk_foo(c) offsetof(c, clk_hw_foo, clk)

  static int clk_foo_enable(struct clk_hw *clk)
  {
  	struct clk_foo *foo = to_clk_foo(clk);
  	raw_writeb(foo->enable_reg, 1);
  	return 0;
  }

  struct clk_hw_ops clk_foo_ops = {
  	.enable = clk_foo_enable,
  };

And in the platform initialisation code:

  struct clk_foo my_clk_foo;

  void init_clocks(void)
  {
  	my_clk_foo.enable_reg = ioremap(...);

  	clk_register(&clk_foo_ops, &my_clk_foo, NULL);
  }

Changes from Thomas Gleixner <tglx@linutronix.de>.

The common clock definitions are based on a development patch from Ben
Herrenschmidt <benh@kernel.crashing.org>.

TODO:

 * We don't keep any internal reference to the clock topology at present.

Signed-off-by: Jeremy Kerr <jeremy.kerr@canonical.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
Signed-off-by: Mike Turquette <mturquette@ti.com>
---
Changes since v1:
Create a dummy clk_unregister and prototype/document it and clk_register
Constify struct clk_hw_ops
Remove spinlock.h header, include kernel.h
Use EOPNOTSUPP instead of ENOTSUPP
Add might_sleep to clk_prepare/clk_unprepare stubs
Properly init children hlist and child_node
Whitespace and typo fixes

 drivers/clk/Kconfig  |    3 +
 drivers/clk/Makefile |    1 +
 drivers/clk/clk.c    |  232 ++++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/clk/clkdev.c |    7 ++
 include/linux/clk.h  |  140 +++++++++++++++++++++++++++---
 5 files changed, 371 insertions(+), 12 deletions(-)
 create mode 100644 drivers/clk/clk.c

Comments

Grant Likely Sept. 25, 2011, 3:55 a.m. UTC | #1
On Thu, Sep 22, 2011 at 03:26:56PM -0700, Mike Turquette wrote:
> From: Jeremy Kerr <jeremy.kerr@canonical.com>
> 
> We currently have ~21 definitions of struct clk in the ARM architecture,
> each defined on a per-platform basis. This makes it difficult to define
> platform- (or architecture-) independent clock sources without making
> assumptions about struct clk, and impossible to compile two
> platforms with different struct clks into a single image.
> 
> This change is an effort to unify struct clk where possible, by defining
> a common struct clk, and a set of clock operations. Different clock
> implementations can set their own operations, and have a standard
> interface for generic code. The callback interface is exposed to the
> kernel proper, while the clock implementations only need to be seen by
> the platform internals.
> 
> The interface is split into two halves:
> 
>  * struct clk, which is the generic-device-driver interface. This
>    provides a set of functions which drivers may use to request
>    enable/disable, query or manipulate in a hardware-independent manner.
> 
>  * struct clk_hw and struct clk_hw_ops, which is the hardware-specific
>    interface. Clock drivers implement the ops, which allow the core
>    clock code to implement the generic 'struct clk' API.
> 
> This allows us to share clock code among platforms, and makes it
> possible to dynamically create clock devices in platform-independent
> code.
> 
> Platforms can enable the generic struct clock through
> CONFIG_GENERIC_CLK. In this case, the clock infrastructure consists of a
> common, opaque struct clk, and a set of clock operations (defined per
> type of clock):
> 
>   struct clk_hw_ops {
>   	int		(*prepare)(struct clk_hw *);
>   	void		(*unprepare)(struct clk_hw *);
>   	int		(*enable)(struct clk_hw *);
>   	void		(*disable)(struct clk_hw *);
>   	unsigned long	(*recalc_rate)(struct clk_hw *);
>   	int		(*set_rate)(struct clk_hw *,
>   					unsigned long, unsigned long *);
>   	long		(*round_rate)(struct clk_hw *, unsigned long);
>   	int		(*set_parent)(struct clk_hw *, struct clk *);
>   	struct clk *	(*get_parent)(struct clk_hw *);
>   };
> 
> Platform clock code can register a clock through clk_register, passing a
> set of operations, and a pointer to hardware-specific data:
> 
>   struct clk_hw_foo {
>   	struct clk_hw clk;
>   	void __iomem *enable_reg;
>   };
> 
>   #define to_clk_foo(c) offsetof(c, clk_hw_foo, clk)
> 
>   static int clk_foo_enable(struct clk_hw *clk)
>   {
>   	struct clk_foo *foo = to_clk_foo(clk);
>   	raw_writeb(foo->enable_reg, 1);
>   	return 0;
>   }
> 
>   struct clk_hw_ops clk_foo_ops = {
>   	.enable = clk_foo_enable,
>   };
> 
> And in the platform initialisation code:
> 
>   struct clk_foo my_clk_foo;
> 
>   void init_clocks(void)
>   {
>   	my_clk_foo.enable_reg = ioremap(...);
> 
>   	clk_register(&clk_foo_ops, &my_clk_foo, NULL);

Shouldn't this be:

	clk_register(&clk_foo_ops, &my_clk_foo->clk, NULL);

?

Also, this documentation would be good to have in the Documentation
directory instead of lost in a commit header.

Otherwise looks okay to me.

Reviewed-by: Grant Likely <grant.likely@secretlab.ca>
Mike Turquette Sept. 25, 2011, 5:26 a.m. UTC | #2
On Sat, Sep 24, 2011 at 8:55 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Thu, Sep 22, 2011 at 03:26:56PM -0700, Mike Turquette wrote:
>> From: Jeremy Kerr <jeremy.kerr@canonical.com>
>>
>> We currently have ~21 definitions of struct clk in the ARM architecture,
>> each defined on a per-platform basis. This makes it difficult to define
>> platform- (or architecture-) independent clock sources without making
>> assumptions about struct clk, and impossible to compile two
>> platforms with different struct clks into a single image.
>>
>> This change is an effort to unify struct clk where possible, by defining
>> a common struct clk, and a set of clock operations. Different clock
>> implementations can set their own operations, and have a standard
>> interface for generic code. The callback interface is exposed to the
>> kernel proper, while the clock implementations only need to be seen by
>> the platform internals.
>>
>> The interface is split into two halves:
>>
>>  * struct clk, which is the generic-device-driver interface. This
>>    provides a set of functions which drivers may use to request
>>    enable/disable, query or manipulate in a hardware-independent manner.
>>
>>  * struct clk_hw and struct clk_hw_ops, which is the hardware-specific
>>    interface. Clock drivers implement the ops, which allow the core
>>    clock code to implement the generic 'struct clk' API.
>>
>> This allows us to share clock code among platforms, and makes it
>> possible to dynamically create clock devices in platform-independent
>> code.
>>
>> Platforms can enable the generic struct clock through
>> CONFIG_GENERIC_CLK. In this case, the clock infrastructure consists of a
>> common, opaque struct clk, and a set of clock operations (defined per
>> type of clock):
>>
>>   struct clk_hw_ops {
>>       int             (*prepare)(struct clk_hw *);
>>       void            (*unprepare)(struct clk_hw *);
>>       int             (*enable)(struct clk_hw *);
>>       void            (*disable)(struct clk_hw *);
>>       unsigned long   (*recalc_rate)(struct clk_hw *);
>>       int             (*set_rate)(struct clk_hw *,
>>                                       unsigned long, unsigned long *);
>>       long            (*round_rate)(struct clk_hw *, unsigned long);
>>       int             (*set_parent)(struct clk_hw *, struct clk *);
>>       struct clk *    (*get_parent)(struct clk_hw *);
>>   };
>>
>> Platform clock code can register a clock through clk_register, passing a
>> set of operations, and a pointer to hardware-specific data:
>>
>>   struct clk_hw_foo {
>>       struct clk_hw clk;
>>       void __iomem *enable_reg;
>>   };
>>
>>   #define to_clk_foo(c) offsetof(c, clk_hw_foo, clk)
>>
>>   static int clk_foo_enable(struct clk_hw *clk)
>>   {
>>       struct clk_foo *foo = to_clk_foo(clk);
>>       raw_writeb(foo->enable_reg, 1);
>>       return 0;
>>   }
>>
>>   struct clk_hw_ops clk_foo_ops = {
>>       .enable = clk_foo_enable,
>>   };
>>
>> And in the platform initialisation code:
>>
>>   struct clk_foo my_clk_foo;
>>
>>   void init_clocks(void)
>>   {
>>       my_clk_foo.enable_reg = ioremap(...);
>>
>>       clk_register(&clk_foo_ops, &my_clk_foo, NULL);
>
> Shouldn't this be:
>
>        clk_register(&clk_foo_ops, &my_clk_foo->clk, NULL);
>
> ?
>
> Also, this documentation would be good to have in the Documentation
> directory instead of lost in a commit header.

Thanks for your review Grant.  Will fix the changelog and add proper
Documentation/ in the next round.

Regards,
Mike

> Otherwise looks okay to me.
>
> Reviewed-by: Grant Likely <grant.likely@secretlab.ca>
>
>
Rob Herring Oct. 3, 2011, 2:17 p.m. UTC | #3
On 09/22/2011 05:26 PM, Mike Turquette wrote:
> From: Jeremy Kerr <jeremy.kerr@canonical.com>
> 
> We currently have ~21 definitions of struct clk in the ARM architecture,
> each defined on a per-platform basis. This makes it difficult to define
> platform- (or architecture-) independent clock sources without making
> assumptions about struct clk, and impossible to compile two
> platforms with different struct clks into a single image.
> 
> This change is an effort to unify struct clk where possible, by defining
> a common struct clk, and a set of clock operations. Different clock
> implementations can set their own operations, and have a standard
> interface for generic code. The callback interface is exposed to the
> kernel proper, while the clock implementations only need to be seen by
> the platform internals.
> 
> The interface is split into two halves:
> 
>  * struct clk, which is the generic-device-driver interface. This
>    provides a set of functions which drivers may use to request
>    enable/disable, query or manipulate in a hardware-independent manner.
> 
>  * struct clk_hw and struct clk_hw_ops, which is the hardware-specific
>    interface. Clock drivers implement the ops, which allow the core
>    clock code to implement the generic 'struct clk' API.
> 
> This allows us to share clock code among platforms, and makes it
> possible to dynamically create clock devices in platform-independent
> code.
> 
> Platforms can enable the generic struct clock through
> CONFIG_GENERIC_CLK. In this case, the clock infrastructure consists of a
> common, opaque struct clk, and a set of clock operations (defined per
> type of clock):
> 
>   struct clk_hw_ops {
>   	int		(*prepare)(struct clk_hw *);
>   	void		(*unprepare)(struct clk_hw *);
>   	int		(*enable)(struct clk_hw *);
>   	void		(*disable)(struct clk_hw *);
>   	unsigned long	(*recalc_rate)(struct clk_hw *);
>   	int		(*set_rate)(struct clk_hw *,
>   					unsigned long, unsigned long *);
>   	long		(*round_rate)(struct clk_hw *, unsigned long);
>   	int		(*set_parent)(struct clk_hw *, struct clk *);
>   	struct clk *	(*get_parent)(struct clk_hw *);
>   };
> 
> Platform clock code can register a clock through clk_register, passing a
> set of operations, and a pointer to hardware-specific data:
> 
>   struct clk_hw_foo {
>   	struct clk_hw clk;
>   	void __iomem *enable_reg;
>   };
> 
>   #define to_clk_foo(c) offsetof(c, clk_hw_foo, clk)
> 
>   static int clk_foo_enable(struct clk_hw *clk)
>   {
>   	struct clk_foo *foo = to_clk_foo(clk);
>   	raw_writeb(foo->enable_reg, 1);
>   	return 0;
>   }
> 
>   struct clk_hw_ops clk_foo_ops = {
>   	.enable = clk_foo_enable,
>   };
> 
> And in the platform initialisation code:
> 
>   struct clk_foo my_clk_foo;
> 
>   void init_clocks(void)
>   {
>   	my_clk_foo.enable_reg = ioremap(...);
> 
>   	clk_register(&clk_foo_ops, &my_clk_foo, NULL);
>   }
> 
> Changes from Thomas Gleixner <tglx@linutronix.de>.
> 
> The common clock definitions are based on a development patch from Ben
> Herrenschmidt <benh@kernel.crashing.org>.
> 
> TODO:
> 
>  * We don't keep any internal reference to the clock topology at present.
> 
> Signed-off-by: Jeremy Kerr <jeremy.kerr@canonical.com>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
> Signed-off-by: Mike Turquette <mturquette@ti.com>
> ---
> Changes since v1:
> Create a dummy clk_unregister and prototype/document it and clk_register
> Constify struct clk_hw_ops
> Remove spinlock.h header, include kernel.h
> Use EOPNOTSUPP instead of ENOTSUPP
> Add might_sleep to clk_prepare/clk_unprepare stubs
> Properly init children hlist and child_node
> Whitespace and typo fixes
> 
>  drivers/clk/Kconfig  |    3 +
>  drivers/clk/Makefile |    1 +
>  drivers/clk/clk.c    |  232 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/clk/clkdev.c |    7 ++
>  include/linux/clk.h  |  140 +++++++++++++++++++++++++++---
>  5 files changed, 371 insertions(+), 12 deletions(-)
>  create mode 100644 drivers/clk/clk.c
> 
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index 3530927..c53ed59 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -5,3 +5,6 @@ config CLKDEV_LOOKUP
>  
>  config HAVE_MACH_CLKDEV
>  	bool
> +
> +config GENERIC_CLK
> +	bool
> 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..1cd7315
> --- /dev/null
> +++ b/drivers/clk/clk.c
> @@ -0,0 +1,232 @@
> +/*
> + * Copyright (C) 2010-2011 Canonical Ltd <jeremy.kerr@canonical.com>
> + *
> + * 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.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +
> +struct clk {
> +	const char		*name;
> +	const struct clk_hw_ops	*ops;
> +	struct clk_hw		*hw;
> +	unsigned int		enable_count;
> +	unsigned int		prepare_count;
> +	struct clk		*parent;
> +	unsigned long		rate;
> +};
> +
> +static DEFINE_SPINLOCK(enable_lock);
> +static DEFINE_MUTEX(prepare_lock);
> +
> +static 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->hw);
> +
> +	__clk_unprepare(clk->parent);
> +}
> +
> +void clk_unprepare(struct clk *clk)
> +{
> +	mutex_lock(&prepare_lock);
> +	__clk_unprepare(clk);
> +	mutex_unlock(&prepare_lock);
> +}
> +EXPORT_SYMBOL_GPL(clk_unprepare);
> +
> +static 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->hw);
> +			if (ret) {
> +				__clk_unprepare(clk->parent);
> +				return ret;
> +			}
> +		}
> +	}
> +
> +	clk->prepare_count++;
> +
> +	return 0;
> +}
> +
> +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);
> +
> +static 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->hw);
> +	__clk_disable(clk->parent);
> +}
> +
> +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);
> +
> +static int __clk_enable(struct clk *clk)
> +{
> +	int ret;
> +
> +	if (!clk)
> +		return 0;
> +
> +	if (WARN_ON(clk->prepare_count == 0))
> +		return -ESHUTDOWN;
> +
> +
> +	if (clk->enable_count == 0) {
> +		ret = __clk_enable(clk->parent);
> +		if (ret)
> +			return ret;
> +
> +		if (clk->ops->enable) {
> +			ret = clk->ops->enable(clk->hw);
> +			if (ret) {
> +				__clk_disable(clk->parent);
> +				return ret;
> +			}
> +		}
> +	}
> +
> +	clk->enable_count++;
> +	return 0;
> +}
> +
> +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);
> +
> +unsigned long clk_get_rate(struct clk *clk)
> +{
> +	if (!clk)
> +		return 0;
> +	return clk->rate;
> +}
> +EXPORT_SYMBOL_GPL(clk_get_rate);
> +
> +long clk_round_rate(struct clk *clk, unsigned long rate)
> +{
> +	if (clk && clk->ops->round_rate)
> +		return clk->ops->round_rate(clk->hw, rate);
> +	return rate;
> +}
> +EXPORT_SYMBOL_GPL(clk_round_rate);
> +
> +int clk_set_rate(struct clk *clk, unsigned long rate)
> +{
> +	/* not yet implemented */
> +	return -ENOSYS;
> +}
> +EXPORT_SYMBOL_GPL(clk_set_rate);
> +
> +struct clk *clk_get_parent(struct clk *clk)
> +{
> +	if (!clk)
> +		return NULL;
> +
> +	return clk->parent;
> +}
> +EXPORT_SYMBOL_GPL(clk_get_parent);
> +
> +int clk_set_parent(struct clk *clk, struct clk *parent)
> +{
> +	/* not yet implemented */
> +	return -ENOSYS;
> +}
> +EXPORT_SYMBOL_GPL(clk_set_parent);
> +
> +struct clk *clk_register(const struct clk_hw_ops *ops, struct clk_hw *hw,
> +		const char *name)
> +{
> +	struct clk *clk;
> +
> +	clk = kzalloc(sizeof(*clk), GFP_KERNEL);
> +	if (!clk)
> +		return NULL;
> +
> +	INIT_HLIST_HEAD(&clk->children);
> +	INIT_HLIST_NODE(&clk->child_node);
> +
> +	clk->name = name;
> +	clk->ops = ops;
> +	clk->hw = hw;
> +	hw->clk = clk;
> +
> +	/* Query the hardware for parent and initial rate */
> +
> +	if (clk->ops->get_parent)
> +		/* We don't to lock against prepare/enable here, as
> +		 * the clock is not yet accessible from anywhere */
> +		clk->parent = clk->ops->get_parent(clk->hw);

I don't think this is going to work. This implies that the parent clock
is already registered. For simple clk trees, that's probably not an
issue, but for chips with lots of muxing it will be impossible to get
the order correct for all cases. This is not an issue today as most
clocks are statically created.

I think what is needed is a 2 stage init. The 1st stage to create all
the clocks and a 2nd stage to build the tree once all clocks are created.

Tracking the parents using struct clk_hw instead would help as long as
clocks are still statically allocated. However, that won't help for
devicetree.

Rob
Mark Brown Oct. 3, 2011, 2:25 p.m. UTC | #4
On Mon, Oct 03, 2011 at 09:17:30AM -0500, Rob Herring wrote:
> On 09/22/2011 05:26 PM, Mike Turquette wrote:

A lot of stuff that should really have been cut plus...

> > +	if (clk->ops->get_parent)
> > +		/* We don't to lock against prepare/enable here, as
> > +		 * the clock is not yet accessible from anywhere */
> > +		clk->parent = clk->ops->get_parent(clk->hw);

> I don't think this is going to work. This implies that the parent clock
> is already registered. For simple clk trees, that's probably not an
> issue, but for chips with lots of muxing it will be impossible to get
> the order correct for all cases. This is not an issue today as most
> clocks are statically created.

> I think what is needed is a 2 stage init. The 1st stage to create all
> the clocks and a 2nd stage to build the tree once all clocks are created.

> Tracking the parents using struct clk_hw instead would help as long as
> clocks are still statically allocated. However, that won't help for
> devicetree.

This isn't in any way specific to clocks, right now the likely solution
looks to be Grant's changes for retrying probe() as new devices come on
line.  With that devices can return a code from their probe() which
tells the driver core that they couldn't get all the resources they need
and that it should retry the probe() if more devices come on-line.
Rob Herring Oct. 3, 2011, 3:24 p.m. UTC | #5
On 10/03/2011 09:25 AM, Mark Brown wrote:
> On Mon, Oct 03, 2011 at 09:17:30AM -0500, Rob Herring wrote:
>> On 09/22/2011 05:26 PM, Mike Turquette wrote:
> 
> A lot of stuff that should really have been cut plus...
> 
>>> +	if (clk->ops->get_parent)
>>> +		/* We don't to lock against prepare/enable here, as
>>> +		 * the clock is not yet accessible from anywhere */
>>> +		clk->parent = clk->ops->get_parent(clk->hw);
> 
>> I don't think this is going to work. This implies that the parent clock
>> is already registered. For simple clk trees, that's probably not an
>> issue, but for chips with lots of muxing it will be impossible to get
>> the order correct for all cases. This is not an issue today as most
>> clocks are statically created.
> 
>> I think what is needed is a 2 stage init. The 1st stage to create all
>> the clocks and a 2nd stage to build the tree once all clocks are created.
> 
>> Tracking the parents using struct clk_hw instead would help as long as
>> clocks are still statically allocated. However, that won't help for
>> devicetree.
> 
> This isn't in any way specific to clocks, right now the likely solution
> looks to be Grant's changes for retrying probe() as new devices come on
> line.  With that devices can return a code from their probe() which
> tells the driver core that they couldn't get all the resources they need
> and that it should retry the probe() if more devices come on-line.

Except SOC clocks are initialized very early before timers are up and
there can be a very high number of dependencies (every clock except
fixed clocks). With the driver probe retry, retrying is the exception,
not the rule.

Retrying would require every caller to maintain a list of clks to
retry. With 2 stages, you can move that into the core clock code.

There are not typically a large number of board-level/driver created
clocks, so ensuring correct register order is not really a problem. In
cases where there is a cross-driver dependency, the probe retry is a
good solution.

Rob
Mark Brown Oct. 3, 2011, 4:31 p.m. UTC | #6
On Mon, Oct 03, 2011 at 10:24:52AM -0500, Rob Herring wrote:
> On 10/03/2011 09:25 AM, Mark Brown wrote:

> > This isn't in any way specific to clocks, right now the likely solution
> > looks to be Grant's changes for retrying probe() as new devices come on
> > line.  With that devices can return a code from their probe() which
> > tells the driver core that they couldn't get all the resources they need
> > and that it should retry the probe() if more devices come on-line.

> Except SOC clocks are initialized very early before timers are up and
> there can be a very high number of dependencies (every clock except
> fixed clocks). With the driver probe retry, retrying is the exception,
> not the rule.

> Retrying would require every caller to maintain a list of clks to
> retry. With 2 stages, you can move that into the core clock code.

They don't need to maintain a list of clocks to retry, they need to
unwind when probe() fails.  But yes.

> There are not typically a large number of board-level/driver created
> clocks, so ensuring correct register order is not really a problem. In
> cases where there is a cross-driver dependency, the probe retry is a
> good solution.

I dunno, I get the impression that some of this is due to the current
limitations of the clock API rather than due to a lack of clocks -
perhaps that's specific to the applications I look at, though.
applications
Russell King - ARM Linux Oct. 3, 2011, 4:43 p.m. UTC | #7
On Mon, Oct 03, 2011 at 05:31:08PM +0100, Mark Brown wrote:
> I dunno, I get the impression that some of this is due to the current
> limitations of the clock API rather than due to a lack of clocks -
> perhaps that's specific to the applications I look at, though.
> applications 

The clk API per-se has nothing to do with how clocks are registered.
There are two things that are the clk API:

1. clkdev - which deals with translating devices + connection IDs to
   struct clk.  This has no ordering requirements wrt requiring
   parents to be "initialized" before their children (it has no care
   about that at all because it's not within its definition.)

   For this, registration is about connecting device + connection IDs
   to a struct clk.

2. the driver API, defining how the opaque struct clk is looked up,
   obtained and then manipulated.  This has no 'registration' stuff.

So, whether clocks are a tree or flat is unspecified.  It's unspecified
whether there's any particular order required.

In fact, with a clock tree, it's entirely possible that only the leaf
clocks will be 'registered' with clkdev.  How the rest of the clock tree
is initialized is beyond the scope of the driver clk API.
Rob Herring Oct. 3, 2011, 10:02 p.m. UTC | #8
Mike,

On 09/22/2011 05:26 PM, Mike Turquette wrote:

> diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
> index 6db161f..e2a9719 100644
> --- a/drivers/clk/clkdev.c
> +++ b/drivers/clk/clkdev.c
> @@ -23,6 +23,13 @@
>  static LIST_HEAD(clocks);
>  static DEFINE_MUTEX(clocks_mutex);
>  
> +/* For USE_COMMON_STRUCT_CLK, these are provided in clk.c, but not exported
> + * through other headers; we don't want them used anywhere but here. */
> +#ifdef CONFIG_USE_COMMON_STRUCT_CLK
> +extern int __clk_get(struct clk *clk);
> +extern void __clk_put(struct clk *clk);
> +#endif
> +

This is dead code left from prior versions.

Rob
Mike Turquette Oct. 3, 2011, 10:15 p.m. UTC | #9
On Mon, Oct 3, 2011 at 3:02 PM, Rob Herring <robherring2@gmail.com> wrote:
> Mike,
>
> On 09/22/2011 05:26 PM, Mike Turquette wrote:
>
>> diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
>> index 6db161f..e2a9719 100644
>> --- a/drivers/clk/clkdev.c
>> +++ b/drivers/clk/clkdev.c
>> @@ -23,6 +23,13 @@
>>  static LIST_HEAD(clocks);
>>  static DEFINE_MUTEX(clocks_mutex);
>>
>> +/* For USE_COMMON_STRUCT_CLK, these are provided in clk.c, but not exported
>> + * through other headers; we don't want them used anywhere but here. */
>> +#ifdef CONFIG_USE_COMMON_STRUCT_CLK
>> +extern int __clk_get(struct clk *clk);
>> +extern void __clk_put(struct clk *clk);
>> +#endif
>> +
>
> This is dead code left from prior versions.

Indeed it is.  Will pull it out for V3.

Thanks,
Mike

> Rob
>
Grant Likely Oct. 4, 2011, 6:09 p.m. UTC | #10
On Mon, Oct 03, 2011 at 09:17:30AM -0500, Rob Herring wrote:
> On 09/22/2011 05:26 PM, Mike Turquette wrote:
> > +	/* Query the hardware for parent and initial rate */
> > +
> > +	if (clk->ops->get_parent)
> > +		/* We don't to lock against prepare/enable here, as
> > +		 * the clock is not yet accessible from anywhere */
> > +		clk->parent = clk->ops->get_parent(clk->hw);
> 
> I don't think this is going to work. This implies that the parent clock
> is already registered. For simple clk trees, that's probably not an
> issue, but for chips with lots of muxing it will be impossible to get
> the order correct for all cases. This is not an issue today as most
> clocks are statically created.
> 
> I think what is needed is a 2 stage init. The 1st stage to create all
> the clocks and a 2nd stage to build the tree once all clocks are created.
> 
> Tracking the parents using struct clk_hw instead would help as long as
> clocks are still statically allocated. However, that won't help for
> devicetree.

I disagree.  Clocks really need to be registered in dependency order.
Even in the deferral case, the driver should hold of actually
registering the clk (note: the struct clk, not the struct device)
until the clocks it depends on are available.

I also agree with the point that there are a lot of SoC clocks that
may not even show up in clkdev, and for pragmatic considerations are
better set up all at once early in the init process (ie. as part of
common SoC setup code, and doesn't change between boards).  That code
should be clue-full enough that it can register its own clocks in the
right order.

g.
Saravana Kannan Oct. 6, 2011, 1:17 a.m. UTC | #11
On 09/22/2011 03:26 PM, Mike Turquette wrote:
> diff --git a/include/linux/clk.h b/include/linux/clk.h
> index 1d37f42..d6ae10b 100644
> --- a/include/linux/clk.h
> +++ b/include/linux/clk.h
> +#ifdef CONFIG_GENERIC_CLK
> +
> +struct clk_hw {
> +	struct clk *clk;
> +};
> +
> +/**
> + * 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. Called with the global clock mutex
> + *		held. Optional, but recommended - if this op is not set,
> + *		clk_get_rate will return 0.
> + *
> + * @get_parent	Query the parent of this clock; for clocks with multiple
> + *		possible parents, query the hardware for the current
> + *		parent. Currently only called when the clock is first
> + *		registered.
> + *
> + * 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 a clock requires sleeping code to be turned on, this
> + * should be done in clk_prepare. Switching that will not sleep should be done
> + * 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_hw_ops {
> +	int		(*prepare)(struct clk_hw *);
> +	void		(*unprepare)(struct clk_hw *);
> +	int		(*enable)(struct clk_hw *);
> +	void		(*disable)(struct clk_hw *);
> +	unsigned long	(*recalc_rate)(struct clk_hw *);
> +	long		(*round_rate)(struct clk_hw *, unsigned long);
> +	struct clk *	(*get_parent)(struct clk_hw *);
> +};

I would like to understand the need for recalc rate if that's something 
that we want to go into the common framework (even if it's optional). I 
have mostly heard only second hand explanations of the need for 
recalc_rate(), so I might not have the full picture. But for all the 
cases that I can think of, recalc_rate seems like a paradox.

If recalc_rate() is used to make sure the "current rate" of a "clock A" 
is always known even if it's parent "clock B"'s rate is changed, then it 
also means that the rate of "clock A" can change without 
clk_set_rate(clock A, new rate). That in turn means that the 
clk_get_rate() just gives the instantaneous snapshot of the rate. So, 
any use of clk_get_rate(clock A) for anything other than 
printing/logging the return value is broken code. In which case, do we 
really care for recalc_rate()? We could just return the rate that it was 
set to when clk_set_rate() was called and call it a day or return 0 for 
such clocks to indicate that the clock rate is "unknown".

The whole concept of trying to recalculate the rate for a clock makes me 
feel uneasy since it promotes misunderstanding the behavior of the clock 
and writing bad code based on that misunderstanding.

I would like to hear to real usecases before I propose some alternatives 
that I have in mind.

Thanks,
Saravana
Mike Turquette Oct. 6, 2011, 4:11 p.m. UTC | #12
On Wed, Oct 5, 2011 at 6:17 PM, Saravana Kannan <skannan@codeaurora.org> wrote:
> On 09/22/2011 03:26 PM, Mike Turquette wrote:
>> +       unsigned long   (*recalc_rate)(struct clk_hw *);
>> +       long            (*round_rate)(struct clk_hw *, unsigned long);
>> +       struct clk *    (*get_parent)(struct clk_hw *);
>> +};
>
> I would like to understand the need for recalc rate if that's something that
> we want to go into the common framework (even if it's optional). I have
> mostly heard only second hand explanations of the need for recalc_rate(), so
> I might not have the full picture. But for all the cases that I can think
> of, recalc_rate seems like a paradox.

Recalc rate has four main uses that I can think of off the top of my head:
1) clk_set_rate is called on clock0, which is a non-leaf clock.  All
clocks "below" clock0 have had their rates changed, yet no one called
clk_set_rate on those child clocks.  We use recalc to walk the
sub-tree of children and recalculate their rates based on the new rate
of their parent, clock0.

2) exact same as #1, but using clk_set_parent instead of clk_set_rate.
 Again, changing the rate of a clock "high up" in the tree will affect
the rates of many child clocks below it.

3) at boot-time/init-time when we don't trust the bootloader and need
to determine the clock rates by parsing registers

4) modeling rates for clocks that we don't really control.  This is
not as common as the above three, but there are times when we're
interested in knowing a clock rate (perhaps for debug purposes), but
it's scaling logic is in firmware or somehow independent of the Linux
clock framework.

> If recalc_rate() is used to make sure the "current rate" of a "clock A" is
> always known even if it's parent "clock B"'s rate is changed, then it also
> means that the rate of "clock A" can change without clk_set_rate(clock A,
> new rate). That in turn means that the clk_get_rate() just gives the
> instantaneous snapshot of the rate. So, any use of clk_get_rate(clock A) for
> anything other than printing/logging the return value is broken code. In

For most clocks, the first three examples I give above will cover all
of the times that a clock's rate will change.  As long as a
recalc/tree-walk is present then clk->rate is not out of sync and thus
printing/reading that value is not broken.

> which case, do we really care for recalc_rate()? We could just return the
> rate that it was set to when clk_set_rate() was called and call it a day or
> return 0 for such clocks to indicate that the clock rate is "unknown".

What's the point of tracking a rate if it can't be trusted?  Also, it
is important to recalc rates whenever changes are made "high up" in
the clock tree once we start to work on rate-change-arbitration
issues, etc.

Regards,
Mike

> The whole concept of trying to recalculate the rate for a clock makes me
> feel uneasy since it promotes misunderstanding the behavior of the clock and
> writing bad code based on that misunderstanding.
>
> I would like to hear to real usecases before I propose some alternatives
> that I have in mind.
>
> Thanks,
> Saravana
>
> --
> Sent by an employee of the Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
>
Richard Zhao Oct. 11, 2011, 11:25 a.m. UTC | #13
On Thu, Sep 22, 2011 at 03:26:56PM -0700, Mike Turquette wrote:
> From: Jeremy Kerr <jeremy.kerr@canonical.com>
> 
> We currently have ~21 definitions of struct clk in the ARM architecture,
> each defined on a per-platform basis. This makes it difficult to define
> platform- (or architecture-) independent clock sources without making
> assumptions about struct clk, and impossible to compile two
> platforms with different struct clks into a single image.
> 
> This change is an effort to unify struct clk where possible, by defining
> a common struct clk, and a set of clock operations. Different clock
> implementations can set their own operations, and have a standard
> interface for generic code. The callback interface is exposed to the
> kernel proper, while the clock implementations only need to be seen by
> the platform internals.
> 
> The interface is split into two halves:
> 
>  * struct clk, which is the generic-device-driver interface. This
>    provides a set of functions which drivers may use to request
>    enable/disable, query or manipulate in a hardware-independent manner.
> 
>  * struct clk_hw and struct clk_hw_ops, which is the hardware-specific
>    interface. Clock drivers implement the ops, which allow the core
>    clock code to implement the generic 'struct clk' API.
> 
> This allows us to share clock code among platforms, and makes it
> possible to dynamically create clock devices in platform-independent
> code.
> 
> Platforms can enable the generic struct clock through
> CONFIG_GENERIC_CLK. In this case, the clock infrastructure consists of a
> common, opaque struct clk, and a set of clock operations (defined per
> type of clock):
> 
>   struct clk_hw_ops {
>   	int		(*prepare)(struct clk_hw *);
>   	void		(*unprepare)(struct clk_hw *);
>   	int		(*enable)(struct clk_hw *);
>   	void		(*disable)(struct clk_hw *);
>   	unsigned long	(*recalc_rate)(struct clk_hw *);
>   	int		(*set_rate)(struct clk_hw *,
>   					unsigned long, unsigned long *);
>   	long		(*round_rate)(struct clk_hw *, unsigned long);
>   	int		(*set_parent)(struct clk_hw *, struct clk *);
>   	struct clk *	(*get_parent)(struct clk_hw *);
>   };
> 
> Platform clock code can register a clock through clk_register, passing a
> set of operations, and a pointer to hardware-specific data:
> 
>   struct clk_hw_foo {
>   	struct clk_hw clk;
>   	void __iomem *enable_reg;
>   };
> 
>   #define to_clk_foo(c) offsetof(c, clk_hw_foo, clk)
> 
>   static int clk_foo_enable(struct clk_hw *clk)
>   {
>   	struct clk_foo *foo = to_clk_foo(clk);
>   	raw_writeb(foo->enable_reg, 1);
>   	return 0;
>   }
> 
>   struct clk_hw_ops clk_foo_ops = {
>   	.enable = clk_foo_enable,
>   };
> 
> And in the platform initialisation code:
> 
>   struct clk_foo my_clk_foo;
> 
>   void init_clocks(void)
>   {
>   	my_clk_foo.enable_reg = ioremap(...);
> 
>   	clk_register(&clk_foo_ops, &my_clk_foo, NULL);
>   }
> 
> Changes from Thomas Gleixner <tglx@linutronix.de>.
> 
> The common clock definitions are based on a development patch from Ben
> Herrenschmidt <benh@kernel.crashing.org>.
> 
> TODO:
> 
>  * We don't keep any internal reference to the clock topology at present.
> 
> Signed-off-by: Jeremy Kerr <jeremy.kerr@canonical.com>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
> Signed-off-by: Mike Turquette <mturquette@ti.com>
> ---
> Changes since v1:
> Create a dummy clk_unregister and prototype/document it and clk_register
> Constify struct clk_hw_ops
> Remove spinlock.h header, include kernel.h
> Use EOPNOTSUPP instead of ENOTSUPP
> Add might_sleep to clk_prepare/clk_unprepare stubs
> Properly init children hlist and child_node
> Whitespace and typo fixes
> 
>  drivers/clk/Kconfig  |    3 +
>  drivers/clk/Makefile |    1 +
>  drivers/clk/clk.c    |  232 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/clk/clkdev.c |    7 ++
>  include/linux/clk.h  |  140 +++++++++++++++++++++++++++---
>  5 files changed, 371 insertions(+), 12 deletions(-)
>  create mode 100644 drivers/clk/clk.c
> 
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index 3530927..c53ed59 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -5,3 +5,6 @@ config CLKDEV_LOOKUP
>  
>  config HAVE_MACH_CLKDEV
>  	bool
> +
> +config GENERIC_CLK
> +	bool
> 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..1cd7315
> --- /dev/null
> +++ b/drivers/clk/clk.c
> @@ -0,0 +1,232 @@
> +/*
> + * Copyright (C) 2010-2011 Canonical Ltd <jeremy.kerr@canonical.com>
> + *
> + * 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.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +
> +struct clk {
> +	const char		*name;
> +	const struct clk_hw_ops	*ops;
> +	struct clk_hw		*hw;
> +	unsigned int		enable_count;
> +	unsigned int		prepare_count;
> +	struct clk		*parent;
> +	unsigned long		rate;
> +};
> +
> +static DEFINE_SPINLOCK(enable_lock);
> +static DEFINE_MUTEX(prepare_lock);
> +
> +static 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->hw);
> +
> +	__clk_unprepare(clk->parent);
> +}
> +
> +void clk_unprepare(struct clk *clk)
> +{
> +	mutex_lock(&prepare_lock);
> +	__clk_unprepare(clk);
> +	mutex_unlock(&prepare_lock);
> +}
> +EXPORT_SYMBOL_GPL(clk_unprepare);
> +
> +static 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->hw);
> +			if (ret) {
> +				__clk_unprepare(clk->parent);
> +				return ret;
> +			}
> +		}
> +	}
> +
> +	clk->prepare_count++;
> +
> +	return 0;
> +}
> +
> +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);
> +
> +static 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->hw);
> +	__clk_disable(clk->parent);
> +}
> +
> +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);
> +
> +static int __clk_enable(struct clk *clk)
> +{
> +	int ret;
> +
> +	if (!clk)
> +		return 0;
> +
> +	if (WARN_ON(clk->prepare_count == 0))
> +		return -ESHUTDOWN;
> +
> +
> +	if (clk->enable_count == 0) {
> +		ret = __clk_enable(clk->parent);
> +		if (ret)
> +			return ret;
> +
> +		if (clk->ops->enable) {
> +			ret = clk->ops->enable(clk->hw);
> +			if (ret) {
> +				__clk_disable(clk->parent);
> +				return ret;
> +			}
> +		}
> +	}
> +
> +	clk->enable_count++;
> +	return 0;
> +}
> +
> +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);
> +
> +unsigned long clk_get_rate(struct clk *clk)
> +{
> +	if (!clk)
> +		return 0;
> +	return clk->rate;
> +}
> +EXPORT_SYMBOL_GPL(clk_get_rate);
> +
> +long clk_round_rate(struct clk *clk, unsigned long rate)
> +{
> +	if (clk && clk->ops->round_rate)
> +		return clk->ops->round_rate(clk->hw, rate);
> +	return rate;
> +}
> +EXPORT_SYMBOL_GPL(clk_round_rate);
> +
> +int clk_set_rate(struct clk *clk, unsigned long rate)
> +{
> +	/* not yet implemented */
> +	return -ENOSYS;
> +}
> +EXPORT_SYMBOL_GPL(clk_set_rate);
> +
> +struct clk *clk_get_parent(struct clk *clk)
> +{
> +	if (!clk)
> +		return NULL;
> +
> +	return clk->parent;
> +}
> +EXPORT_SYMBOL_GPL(clk_get_parent);
> +
> +int clk_set_parent(struct clk *clk, struct clk *parent)
> +{
> +	/* not yet implemented */
> +	return -ENOSYS;
> +}
> +EXPORT_SYMBOL_GPL(clk_set_parent);
> +
> +struct clk *clk_register(const struct clk_hw_ops *ops, struct clk_hw *hw,
> +		const char *name)
> +{
> +	struct clk *clk;
> +
> +	clk = kzalloc(sizeof(*clk), GFP_KERNEL);
> +	if (!clk)
> +		return NULL;
> +
> +	INIT_HLIST_HEAD(&clk->children);
> +	INIT_HLIST_NODE(&clk->child_node);
> +
> +	clk->name = name;
> +	clk->ops = ops;
> +	clk->hw = hw;
> +	hw->clk = clk;
> +
> +	/* Query the hardware for parent and initial rate */
> +
> +	if (clk->ops->get_parent)
> +		/* We don't to lock against prepare/enable here, as
> +		 * the clock is not yet accessible from anywhere */
> +		clk->parent = clk->ops->get_parent(clk->hw);
> +
> +	if (clk->ops->recalc_rate)
> +		clk->rate = clk->ops->recalc_rate(clk->hw);
Why not set it to parent's rate if recalc_rate is NULL?
> +
> +
> +	return clk;
> +}
> +EXPORT_SYMBOL_GPL(clk_register);
> diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
> index 6db161f..e2a9719 100644
> --- a/drivers/clk/clkdev.c
> +++ b/drivers/clk/clkdev.c
> @@ -23,6 +23,13 @@
>  static LIST_HEAD(clocks);
>  static DEFINE_MUTEX(clocks_mutex);
>  
> +/* For USE_COMMON_STRUCT_CLK, these are provided in clk.c, but not exported
> + * through other headers; we don't want them used anywhere but here. */
> +#ifdef CONFIG_USE_COMMON_STRUCT_CLK
> +extern int __clk_get(struct clk *clk);
> +extern void __clk_put(struct clk *clk);
> +#endif
> +
>  /*
>   * Find the correct struct clk for the device and connection ID.
>   * We do slightly fuzzy matching here:
> diff --git a/include/linux/clk.h b/include/linux/clk.h
> index 1d37f42..d6ae10b 100644
> --- a/include/linux/clk.h
> +++ b/include/linux/clk.h
> @@ -3,6 +3,7 @@
>   *
>   *  Copyright (C) 2004 ARM Limited.
>   *  Written by Deep Blue Solutions Limited.
> + *  Copyright (c) 2010-2011 Jeremy Kerr <jeremy.kerr@canonical.com>
>   *
>   * 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
> @@ -11,17 +12,137 @@
>  #ifndef __LINUX_CLK_H
>  #define __LINUX_CLK_H
>  
> +#include <linux/kernel.h>
> +#include <linux/errno.h>
> +
>  struct device;
>  
> -/*
> - * The base API.
> +struct clk;
> +
> +#ifdef CONFIG_GENERIC_CLK
> +
> +struct clk_hw {
> +	struct clk *clk;
> +};
> +
> +/**
> + * 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. Called with the global clock mutex
> + *		held. Optional, but recommended - if this op is not set,
> + *		clk_get_rate will return 0.
If a clock don't have any divider, recalc_rate may be NULL. In such case,
clk_get_rate should return parent's rate.

Thanks
Richard
> + *
> + * @get_parent	Query the parent of this clock; for clocks with multiple
> + *		possible parents, query the hardware for the current
> + *		parent. Currently only called when the clock is first
> + *		registered.
> + *
> + * 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 a clock requires sleeping code to be turned on, this
> + * should be done in clk_prepare. Switching that will not sleep should be done
> + * 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_hw_ops {
> +	int		(*prepare)(struct clk_hw *);
> +	void		(*unprepare)(struct clk_hw *);
> +	int		(*enable)(struct clk_hw *);
> +	void		(*disable)(struct clk_hw *);
> +	unsigned long	(*recalc_rate)(struct clk_hw *);
> +	long		(*round_rate)(struct clk_hw *, unsigned long);
> +	struct clk *	(*get_parent)(struct clk_hw *);
> +};
>  
> +/**
> + * clk_prepare - prepare clock for atomic enabling.
> + *
> + * @clk: The clock to prepare
> + *
> + * Do any possibly sleeping initialisation on @clk, allowing the clock to be
> + * later enabled atomically (via clk_enable). This function may sleep.
> + */
> +int clk_prepare(struct clk *clk);
> +
> +/**
> + * clk_unprepare - release clock from prepared state
> + *
> + * @clk: The clock to release
> + *
> + * Do any (possibly sleeping) cleanup on clk. This function may sleep.
> + */
> +void clk_unprepare(struct clk *clk);
> +
> +/**
> + * clk_register - register and initialize a new clock
> + *
> + * @ops: ops for the new clock
> + * @hw: struct clk_hw to be passed to the ops of the new clock
> + * @name: name to use for the new clock
> + *
> + * Register a new clock with the clk subsystem.  Returns either a
> + * struct clk for the new clock or a NULL pointer.
> + */
> +struct clk *clk_register(const struct clk_hw_ops *ops, struct clk_hw *hw,
> +			 const char *name);
> +
> +/**
> + * clk_unregister - remove a clock
> + *
> + * @clk: clock to unregister
> + *
> + * Remove a clock from the clk subsystem.  This is currently not
> + * implemented but is provided to allow unregistration code to be
> + * written in drivers ready for use when an implementation is
> + * provided.
> + */
> +static inline int clk_unregister(struct clk *clk)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +#else /* !CONFIG_GENERIC_CLK */
>  
>  /*
> - * struct clk - an machine class defined object / cookie.
> + * For !CONFIG_GENERIC_CLK, we don't enforce any atomicity
> + * requirements for clk_enable/clk_disable, so the prepare and unprepare
> + * functions are no-ops
>   */
> -struct clk;
> +static inline int clk_prepare(struct clk *clk) {
> +	might_sleep();
> +	return 0;
> +}
> +
> +static inline void clk_unprepare(struct clk *clk) {
> +	might_sleep();
> +}
> +
> +#endif /* !CONFIG_GENERIC_CLK */
>  
>  /**
>   * clk_get - lookup and obtain a reference to a clock producer.
> @@ -67,6 +188,7 @@ void clk_disable(struct clk *clk);
>  /**
>   * 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);
> @@ -83,12 +205,6 @@ unsigned long clk_get_rate(struct clk *clk);
>   */
>  void clk_put(struct clk *clk);
>  
> -
> -/*
> - * The remaining APIs are optional for machine class support.
> - */
> -
> -
>  /**
>   * clk_round_rate - adjust a rate to the exact rate a clock can provide
>   * @clk: clock source
> @@ -97,7 +213,7 @@ void clk_put(struct clk *clk);
>   * Returns rounded clock rate in Hz, or negative errno.
>   */
>  long clk_round_rate(struct clk *clk, unsigned long rate);
> - 
> +
>  /**
>   * clk_set_rate - set the clock rate for a clock source
>   * @clk: clock source
> @@ -106,7 +222,7 @@ long clk_round_rate(struct clk *clk, unsigned long rate);
>   * Returns success (0) or negative errno.
>   */
>  int clk_set_rate(struct clk *clk, unsigned long rate);
> - 
> +
>  /**
>   * clk_set_parent - set the parent clock source for this clock
>   * @clk: clock source
> -- 
> 1.7.4.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
Russell King - ARM Linux Oct. 13, 2011, 2:44 p.m. UTC | #14
On Thu, Sep 22, 2011 at 03:26:56PM -0700, Mike Turquette wrote:
>   struct clk_hw_ops {
>   	int		(*prepare)(struct clk_hw *);
>   	void		(*unprepare)(struct clk_hw *);
>   	int		(*enable)(struct clk_hw *);
>   	void		(*disable)(struct clk_hw *);
>   	unsigned long	(*recalc_rate)(struct clk_hw *);
>   	int		(*set_rate)(struct clk_hw *,
>   					unsigned long, unsigned long *);
>   	long		(*round_rate)(struct clk_hw *, unsigned long);
>   	int		(*set_parent)(struct clk_hw *, struct clk *);
>   	struct clk *	(*get_parent)(struct clk_hw *);
>   };
> 
> Platform clock code can register a clock through clk_register, passing a
> set of operations, and a pointer to hardware-specific data:
> 
>   struct clk_hw_foo {
>   	struct clk_hw clk;
>   	void __iomem *enable_reg;
>   };

Eww, no, this really isn't going to scale for platforms like OMAP - to
have all the operations indirected through a set of function pointers
for every clock just because the enable register (or enable bit) is
in a different position is completely absurd.

I'm not soo concerned about the increase in memory usage (for 100 to 200
clock definitions its only 4 to 8k) but what worries me the most is
initializing these damned things.  It's an awful lot of initializers,
which means the potential for an awful lot of conflicts should something
change in this structure.
Mike Turquette Oct. 13, 2011, 5:16 p.m. UTC | #15
On Thu, Oct 13, 2011 at 7:44 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Thu, Sep 22, 2011 at 03:26:56PM -0700, Mike Turquette wrote:
>>   struct clk_hw_ops {
>>       int             (*prepare)(struct clk_hw *);
>>       void            (*unprepare)(struct clk_hw *);
>>       int             (*enable)(struct clk_hw *);
>>       void            (*disable)(struct clk_hw *);
>>       unsigned long   (*recalc_rate)(struct clk_hw *);
>>       int             (*set_rate)(struct clk_hw *,
>>                                       unsigned long, unsigned long *);
>>       long            (*round_rate)(struct clk_hw *, unsigned long);
>>       int             (*set_parent)(struct clk_hw *, struct clk *);
>>       struct clk *    (*get_parent)(struct clk_hw *);
>>   };
>>
>> Platform clock code can register a clock through clk_register, passing a
>> set of operations, and a pointer to hardware-specific data:
>>
>>   struct clk_hw_foo {
>>       struct clk_hw clk;
>>       void __iomem *enable_reg;
>>   };
>
> Eww, no, this really isn't going to scale for platforms like OMAP - to
> have all the operations indirected through a set of function pointers
> for every clock just because the enable register (or enable bit) is
> in a different position is completely absurd.

Russel,

I'm porting the OMAP clock framework to common clock right now and it
is going well.

For many clocks near the root of the tree I've been able to re-use
struct clk_hw_fixed & clk_fixed_ops (see patch 3 or 4 in this series),
which actually represents a decrease in memory consumption over the
old OMAP struct clk (which populated many of the operations func
pointers directly, causing duplication).

So far I don't see scalability as an issue.  In fact the design solves
some problems neatly for us.  For now I am creating one "uber clock",
struct clk_hw_omap, which dumps all of the clksel, pll, gate control &
mux crap directly from OMAP's old struct clk.  This is not optimal for
the long-term, but is a reasonable stepping stone which handles 90% of
OMAP clocks.  The new common struct clk makes it much easier for us to
treat PLLs differently from typical dividers, which can be treated
differently from dividers in modules, which can be treated differently
from pure mux clocks, etc.

> I'm not soo concerned about the increase in memory usage (for 100 to 200
> clock definitions its only 4 to 8k) but what worries me the most is
> initializing these damned things.  It's an awful lot of initializers,
> which means the potential for an awful lot of conflicts should something
> change in this structure.

As I stated above, so far memory usage has actually *decreased* due to
removing so many duplicated function pointers for OMAP's old struct
clk.  I wouldn't be surprised if other SoCs experienced the same lift.

Also, in my own tree I've broken out clk_register into clk_init also.
clk_register is still the thing to call if you have dynamically
created clocks, as it handles the malloc, and it then calls clk_init.
clk_init can be used by for static clock data and just handles
initializing the mutex/list/whatever.  Does this allay some of your
concerns?  Are you just trying to avoid allocating all of that memory
dynamically?

Regards,
Mike
Richard Zhao Oct. 14, 2011, 8:10 a.m. UTC | #16
Hi Mike,

On Thu, Sep 22, 2011 at 03:26:56PM -0700, Mike Turquette wrote:
> From: Jeremy Kerr <jeremy.kerr@canonical.com>
> 
> We currently have ~21 definitions of struct clk in the ARM architecture,
> each defined on a per-platform basis. This makes it difficult to define
> platform- (or architecture-) independent clock sources without making
> assumptions about struct clk, and impossible to compile two
> platforms with different struct clks into a single image.
> 
> This change is an effort to unify struct clk where possible, by defining
> a common struct clk, and a set of clock operations. Different clock
> implementations can set their own operations, and have a standard
> interface for generic code. The callback interface is exposed to the
> kernel proper, while the clock implementations only need to be seen by
> the platform internals.
> 
> The interface is split into two halves:
> 
>  * struct clk, which is the generic-device-driver interface. This
>    provides a set of functions which drivers may use to request
>    enable/disable, query or manipulate in a hardware-independent manner.
> 
>  * struct clk_hw and struct clk_hw_ops, which is the hardware-specific
>    interface. Clock drivers implement the ops, which allow the core
>    clock code to implement the generic 'struct clk' API.
> 
> This allows us to share clock code among platforms, and makes it
> possible to dynamically create clock devices in platform-independent
> code.
> 
> Platforms can enable the generic struct clock through
> CONFIG_GENERIC_CLK. In this case, the clock infrastructure consists of a
> common, opaque struct clk, and a set of clock operations (defined per
> type of clock):
> 
>   struct clk_hw_ops {
>   	int		(*prepare)(struct clk_hw *);
>   	void		(*unprepare)(struct clk_hw *);
>   	int		(*enable)(struct clk_hw *);
>   	void		(*disable)(struct clk_hw *);
>   	unsigned long	(*recalc_rate)(struct clk_hw *);
>   	int		(*set_rate)(struct clk_hw *,
>   					unsigned long, unsigned long *);
>   	long		(*round_rate)(struct clk_hw *, unsigned long);
>   	int		(*set_parent)(struct clk_hw *, struct clk *);
>   	struct clk *	(*get_parent)(struct clk_hw *);
>   };
> 
> Platform clock code can register a clock through clk_register, passing a
> set of operations, and a pointer to hardware-specific data:
> 
>   struct clk_hw_foo {
>   	struct clk_hw clk;
>   	void __iomem *enable_reg;
>   };
> 
>   #define to_clk_foo(c) offsetof(c, clk_hw_foo, clk)
> 
>   static int clk_foo_enable(struct clk_hw *clk)
>   {
>   	struct clk_foo *foo = to_clk_foo(clk);
>   	raw_writeb(foo->enable_reg, 1);
>   	return 0;
>   }
> 
>   struct clk_hw_ops clk_foo_ops = {
>   	.enable = clk_foo_enable,
>   };
> 
> And in the platform initialisation code:
> 
>   struct clk_foo my_clk_foo;
> 
>   void init_clocks(void)
>   {
>   	my_clk_foo.enable_reg = ioremap(...);
> 
>   	clk_register(&clk_foo_ops, &my_clk_foo, NULL);
>   }
> 
> Changes from Thomas Gleixner <tglx@linutronix.de>.
> 
> The common clock definitions are based on a development patch from Ben
> Herrenschmidt <benh@kernel.crashing.org>.
> 
> TODO:
> 
>  * We don't keep any internal reference to the clock topology at present.
> 
> Signed-off-by: Jeremy Kerr <jeremy.kerr@canonical.com>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
> Signed-off-by: Mike Turquette <mturquette@ti.com>
> ---
> Changes since v1:
> Create a dummy clk_unregister and prototype/document it and clk_register
> Constify struct clk_hw_ops
> Remove spinlock.h header, include kernel.h
> Use EOPNOTSUPP instead of ENOTSUPP
> Add might_sleep to clk_prepare/clk_unprepare stubs
> Properly init children hlist and child_node
> Whitespace and typo fixes
> 
>  drivers/clk/Kconfig  |    3 +
>  drivers/clk/Makefile |    1 +
>  drivers/clk/clk.c    |  232 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/clk/clkdev.c |    7 ++
>  include/linux/clk.h  |  140 +++++++++++++++++++++++++++---
>  5 files changed, 371 insertions(+), 12 deletions(-)
>  create mode 100644 drivers/clk/clk.c
> 
[...]
> +static 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->hw);
> +	__clk_disable(clk->parent);
> +}
> +
> +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);
> +
> +static int __clk_enable(struct clk *clk)
> +{
> +	int ret;
> +
> +	if (!clk)
> +		return 0;
> +
> +	if (WARN_ON(clk->prepare_count == 0))
> +		return -ESHUTDOWN;
> +
> +
> +	if (clk->enable_count == 0) {
> +		ret = __clk_enable(clk->parent);
> +		if (ret)
> +			return ret;
> +
> +		if (clk->ops->enable) {
> +			ret = clk->ops->enable(clk->hw);
> +			if (ret) {
> +				__clk_disable(clk->parent);
> +				return ret;
> +			}
> +		}
> +	}
> +
> +	clk->enable_count++;
> +	return 0;
> +}
Could you expose __clk_enable/__clk_disable? I find it hard to implement
clk group. clk group means, when a major clk enable/disable, it want a set
of other clks enable/disable accordingly.
> +
> +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);
> +
[...]

Thanks
Richard
Mark Brown Oct. 14, 2011, 10:05 a.m. UTC | #17
On Fri, Oct 14, 2011 at 04:10:26PM +0800, Richard Zhao wrote:
> On Thu, Sep 22, 2011 at 03:26:56PM -0700, Mike Turquette wrote:

snip essentially Mike's entire mail - *please* delete irrelevant quotes
from your replies, it makes it very much easier to find the new text in
your mail and is much more friendly to people reading mail on mobile
devices.

> > +static int __clk_enable(struct clk *clk)
> > +{

> Could you expose __clk_enable/__clk_disable? I find it hard to implement
> clk group. clk group means, when a major clk enable/disable, it want a set
> of other clks enable/disable accordingly.

Shouldn't this be something the core is implementing?  I'd strongly
expect that the clock drivers are relatively dumb and delegate all the
decision making to the core API.  Otherwise it's going to be hard for
the core to implement any logic that involves working with more than one
clock like rate change notification, or guarantee that driver requests
made through the API are satisfied, as the state of the clocks will be
changing underneath it.
Mike Turquette Oct. 14, 2011, 6:14 p.m. UTC | #18
On Thu, Sep 22, 2011 at 3:26 PM, Mike Turquette <mturquette@ti.com> wrote:
> From: Jeremy Kerr <jeremy.kerr@canonical.com>
>  struct clk_hw_ops {
>        int             (*prepare)(struct clk_hw *);
>        void            (*unprepare)(struct clk_hw *);
>        int             (*enable)(struct clk_hw *);
>        void            (*disable)(struct clk_hw *);
>        unsigned long   (*recalc_rate)(struct clk_hw *);

In implementing recalc for divider clocks, I started to wonder, "why
not just pass struct clk *clk into the clk_hw_ops func ptrs?".

recalc is an obvious example whereby we need access to parent->rate.
The code usually ends up looking something like:

unsigned long omap_recalc_rate(struct clk_hw *hw)
{
        struct clk *parent;
        struct clk_hw_omap *oclk;

        parent = hw->clk->parent;
        oclk = to_clk_omap(hw);
        ...
}

That's a bit of a song and dance to have to do in almost every op, and
often these ops will need access to stuff like clk->rate also.   Is
there any opposition to just passing in struct clk?  e.g:

unsigned long omap_recalc_rate(struct clk *clk)
{
        struct clk *parent;
        struct clk_hw_omap *oclk;

        parent = clk->parent;
        oclk = to_clk_omap(clk->hw);
        ...
}

It is a small nitpick, but it affects the API for everybody so best to
get it right now before folks start migrating over to it.

Thanks,
Mike

>        int             (*set_rate)(struct clk_hw *,
>                                        unsigned long, unsigned long *);
>        long            (*round_rate)(struct clk_hw *, unsigned long);
>        int             (*set_parent)(struct clk_hw *, struct clk *);
>        struct clk *    (*get_parent)(struct clk_hw *);
>  };
Richard Zhao Oct. 15, 2011, 2:24 a.m. UTC | #19
On Fri, Oct 14, 2011 at 11:14:19AM -0700, Turquette, Mike wrote:
> On Thu, Sep 22, 2011 at 3:26 PM, Mike Turquette <mturquette@ti.com> wrote:
> > From: Jeremy Kerr <jeremy.kerr@canonical.com>
> >  struct clk_hw_ops {
> >        int             (*prepare)(struct clk_hw *);
> >        void            (*unprepare)(struct clk_hw *);
> >        int             (*enable)(struct clk_hw *);
> >        void            (*disable)(struct clk_hw *);
> >        unsigned long   (*recalc_rate)(struct clk_hw *);
> 
> In implementing recalc for divider clocks, I started to wonder, "why
> not just pass struct clk *clk into the clk_hw_ops func ptrs?".
> 
> recalc is an obvious example whereby we need access to parent->rate.
> The code usually ends up looking something like:
> 
> unsigned long omap_recalc_rate(struct clk_hw *hw)
> {
>         struct clk *parent;
>         struct clk_hw_omap *oclk;
> 
>         parent = hw->clk->parent;
clk drivers can not see struct clk details. I use clk_get_parent.
>         oclk = to_clk_omap(hw);
>         ...
> }
> 
> That's a bit of a song and dance to have to do in almost every op, and
> often these ops will need access to stuff like clk->rate also.   Is
> there any opposition to just passing in struct clk?  e.g:
> 
> unsigned long omap_recalc_rate(struct clk *clk)
> {
>         struct clk *parent;
>         struct clk_hw_omap *oclk;
> 
>         parent = clk->parent;
>         oclk = to_clk_omap(clk->hw);
>         ...
> }
In my understanding, struct clk stores things specific to clk core,
struct clk_hw stores common things needed by clk drivers. For static clk driver
there' some problems:
 - For clocks without mux, I need duplicate a  .parent and set .get_parent.
   Even when we adopt DT and dynamicly create clk, it's still a problem.
   Moving .parent to clk_hw can fix it.
 - When I define a clk array, I don't need to find another place to store .ops.
   It's not problem for dynamic creating clock.
 - As I mentioned in another mail, clk group need no lock version prepare/unprepare
   and enable/disable functions
   Another way is, add a "{struct clk_hw *clks; int count}" in clk_hw, let clk
   core handle it.
   I prefer the second way, but I'm not sure whether it's common enough. It's
   still a problem for dynamic creating clock.

Thanks
Richard
> 
> It is a small nitpick, but it affects the API for everybody so best to
> get it right now before folks start migrating over to it.
> 
> Thanks,
> Mike
> 
> >        int             (*set_rate)(struct clk_hw *,
> >                                        unsigned long, unsigned long *);
> >        long            (*round_rate)(struct clk_hw *, unsigned long);
> >        int             (*set_parent)(struct clk_hw *, struct clk *);
> >        struct clk *    (*get_parent)(struct clk_hw *);
> >  };
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
Sascha Hauer Oct. 16, 2011, 5:55 p.m. UTC | #20
On Fri, Oct 14, 2011 at 06:32:33PM +0800, Richard Zhao wrote:
> On Fri, Oct 14, 2011 at 11:05:04AM +0100, Mark Brown wrote:
> > On Fri, Oct 14, 2011 at 04:10:26PM +0800, Richard Zhao wrote:
> > > On Thu, Sep 22, 2011 at 03:26:56PM -0700, Mike Turquette wrote:
> > 
> > snip essentially Mike's entire mail - *please* delete irrelevant quotes
> > from your replies, it makes it very much easier to find the new text in
> > your mail and is much more friendly to people reading mail on mobile
> > devices.
> I snip not enough? sorry for that. I'll be carefull.
> > 
> > > > +static int __clk_enable(struct clk *clk)
> > > > +{
> > 
> > > Could you expose __clk_enable/__clk_disable? I find it hard to implement
> > > clk group. clk group means, when a major clk enable/disable, it want a set
> > > of other clks enable/disable accordingly.
> > 
> > Shouldn't this be something the core is implementing?  I'd strongly
> > expect that the clock drivers are relatively dumb and delegate all the
> > decision making to the core API.  Otherwise it's going to be hard for
> > the core to implement any logic that involves working with more than one
> > clock like rate change notification, or guarantee that driver requests
> > made through the API are satisfied, as the state of the clocks will be
> > changing underneath it.
> From my point of view, the first step of generic clk can be, easy to adopt
> features of clocks in current mainline git.
> Back to the clk group, I have a patch based on Sascha's work. 
> http://git.linaro.org/gitweb?p=people/riczhao/linux-2.6.git;a=shortlog;h=refs/heads/imx-clk

I thought further about this and a clock group is not something we want
to have at all. Clocks are supposed to be arranged in a tree and
grouping clocks together violates this which leads to problems.
This grouping should be done at driver level, so when a driver needs
more than one clock it should request them all, maybe with a clk_get_all
helper function.

Sascha
Mike Turquette Oct. 16, 2011, 9:17 p.m. UTC | #21
On Fri, Oct 14, 2011 at 7:24 PM, Richard Zhao <richard.zhao@linaro.org> wrote:
> On Fri, Oct 14, 2011 at 11:14:19AM -0700, Turquette, Mike wrote:
>> On Thu, Sep 22, 2011 at 3:26 PM, Mike Turquette <mturquette@ti.com> wrote:
>> unsigned long omap_recalc_rate(struct clk_hw *hw)
>> {
>>         struct clk *parent;
>>         struct clk_hw_omap *oclk;
>>
>>         parent = hw->clk->parent;
> clk drivers can not see struct clk details. I use clk_get_parent.

clk_get_parent should query the hardware to see what the parent is.
This can have undesireable overhead.  It is quite acceptable to
reference a clock's parent through clk->parent, just as it is
acceptable to get a clock rate through clk->rate.

An analogous situation is a clk_get_rate call which uses a clk's
.recalc.  There is undesirable overhead involved in .recalc for clocks
whose rates won't change behind our backs, so best to just treat the
data in struct clk as cache and reference it directly.

>>         oclk = to_clk_omap(hw);
>>         ...
>> }
>>
...
>>
>> unsigned long omap_recalc_rate(struct clk *clk)
>> {
>>         struct clk *parent;
>>         struct clk_hw_omap *oclk;
>>
>>         parent = clk->parent;
>>         oclk = to_clk_omap(clk->hw);
>>         ...
>> }
> In my understanding, struct clk stores things specific to clk core,
> struct clk_hw stores common things needed by clk drivers. For static clk driver
> there' some problems:
>  - For clocks without mux, I need duplicate a  .parent and set .get_parent.
>   Even when we adopt DT and dynamicly create clk, it's still a problem.
>   Moving .parent to clk_hw can fix it.

For clocks with a fixed parent we should just pass it in at
register-time.  We should definitely not move .parent out of struct
clk, since struct clk should be the platform agnostic bit that lets us
do tree walks, build topology, etc etc.

If you really want a .parent outside of struct clk then duplicate it
in your struct clk_hw_imx and teach your .ops about it (analogous to
struct clk_hw_fixed->rate).

>  - When I define a clk array, I don't need to find another place to store .ops.
>   It's not problem for dynamic creating clock.

Something like the following?

static struct clk aess_fclk;

static const clk_hw_ops aess_fclk_ops = {
        .recalc = &omap2_clksel_recalc,
        .round_rate = &omap2_clksel_round_rate,
        .set_rate = &omap2_clksel_set_rate,
};

static struct clk_hw_omap aess_fclk_hw = {
        .hw = {
                .clk = &aess_fclk,
        },
        .clksel = &aess_fclk_div,
        .clksel_reg = OMAP4430_CM1_ABE_AESS_CLKCTRL,
        .clksel_mask = OMAP4430_CLKSEL_AESS_FCLK_MASK,
};

static struct clk aess_fclk = {
        .name = "aess_fclk",
        .ops = &aess_fclk_ops,
        .hw = &aess_fclk_hw.hw,
        .parent = &abe_clk,
};

>  - As I mentioned in another mail, clk group need no lock version prepare/unprepare
>   and enable/disable functions

Clock groups are out of scope for this first series.  We should
discuss more what the needs are for your clock groups.  If it boils
down to just enabling all of the clocks for a given device then you
might want to abstract that away with pm_runtime_* calls, and maybe a
supplementary layer like OMAP's hwmod.  But I might be way off base, I
really don't understand your use case for clock groups.

>   Another way is, add a "{struct clk_hw *clks; int count}" in clk_hw, let clk
>   core handle it.
>   I prefer the second way, but I'm not sure whether it's common enough. It's
>   still a problem for dynamic creating clock.

struct clk_hw is just a pointer for navigating from struct clk ->
struct your_custom_clk and vice versa.  Again can you elaborate on
your needs for managing multiple clocks with a single struct clk_hw?

Thanks,
Mike

>
> Thanks
> Richard
>>
>> It is a small nitpick, but it affects the API for everybody so best to
>> get it right now before folks start migrating over to it.
>>
>> Thanks,
>> Mike
>>
>> >        int             (*set_rate)(struct clk_hw *,
>> >                                        unsigned long, unsigned long *);
>> >        long            (*round_rate)(struct clk_hw *, unsigned long);
>> >        int             (*set_parent)(struct clk_hw *, struct clk *);
>> >        struct clk *    (*get_parent)(struct clk_hw *);
>> >  };
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
>
>
Richard Zhao Oct. 17, 2011, 10:53 a.m. UTC | #22
On Mon, Oct 17, 2011 at 10:20:28AM +0100, Mark Brown wrote:
> On Mon, Oct 17, 2011 at 04:48:52PM +0800, Richard Zhao wrote:
> 
> > For example, devices that possible access to on-chip RAM, depend on OCRAM clock. 
> > On imx53, VPU depends on OCRAM clock, even when VPU does not use OCRAM.
> 
> So if the VPU depends on OCRAM the VPU should be enabling the OCRAM
> clock.  The function of a given clock isn't terribly relevant, and
> certainly grouping clocks together doesn't seem to be the obvious
> solution from what you've said
VPU don't know OCRAM clk, it's SoC specific. I know it's clock function
replationship. But it's the only better place to refect the dependency 
in clock tree. Another dependency example, from SoC bus topology, some bus
clk always depends on bus switch/hub.
> - if the driver doesn't know about the
> clock it seems like the core ought to be enabling it transparently
> rather than gluing it together with some other random clock.
If you mean clk core here, then we need things like below:
struct clk_hw {
	struct clk *clk;
	struct dependency {
		struct clk_hw *clks;
		int count;
	};
};
Though Mike does not like to add things in clk_hw, but it's the only place
to put common things of clk drivers.

Thanks
Richard
> 
> Either way the point here is that individual drivers shouldn't be hand
> coding this stuff, it should be being handled by core code.
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
Sascha Hauer Oct. 17, 2011, 11:05 a.m. UTC | #23
On Mon, Oct 17, 2011 at 06:53:03PM +0800, Richard Zhao wrote:
> On Mon, Oct 17, 2011 at 10:20:28AM +0100, Mark Brown wrote:
> > On Mon, Oct 17, 2011 at 04:48:52PM +0800, Richard Zhao wrote:
> > 
> > > For example, devices that possible access to on-chip RAM, depend on OCRAM clock. 
> > > On imx53, VPU depends on OCRAM clock, even when VPU does not use OCRAM.
> > 
> > So if the VPU depends on OCRAM the VPU should be enabling the OCRAM
> > clock.  The function of a given clock isn't terribly relevant, and
> > certainly grouping clocks together doesn't seem to be the obvious
> > solution from what you've said
> VPU don't know OCRAM clk, it's SoC specific. I know it's clock function
> replationship. But it's the only better place to refect the dependency 
> in clock tree. Another dependency example, from SoC bus topology, some bus
> clk always depends on bus switch/hub.
> > - if the driver doesn't know about the
> > clock it seems like the core ought to be enabling it transparently
> > rather than gluing it together with some other random clock.
> If you mean clk core here, then we need things like below:
> struct clk_hw {
> 	struct clk *clk;
> 	struct dependency {
> 		struct clk_hw *clks;
> 		int count;
> 	};
> };
> Though Mike does not like to add things in clk_hw, but it's the only place
> to put common things of clk drivers.

It's not a problem to associate multiple clocks to a device, we can do
this now already. What a driver can't do now is give-me-all-clocks-I-need(dev),
but this problem should not be solved at clock core level but at the
clkdev level.
The fact is that the different clocks for a device are really different
clocks. A dumb driver may want to request/enable all relevant clocks at
once while a more sophisticated driver may want to enable the clock for
accessing registers in the probe function and a baud clock on device
open time.

Sascha
Russell King - ARM Linux Oct. 17, 2011, 11:30 a.m. UTC | #24
On Mon, Oct 17, 2011 at 01:05:39PM +0200, Sascha Hauer wrote:
> It's not a problem to associate multiple clocks to a device, we can do
> this now already. What a driver can't do now is give-me-all-clocks-I-need(dev),
> but this problem should not be solved at clock core level but at the
> clkdev level.

I don't think it even needs solving at the clkdev level.

In order to get all clocks for a specific device, we'd need variable
length arrays to store the individual struct clk pointers, which isn't
going to be all that nice.  We'd need clk_get_alldev() and
clk_put_alldev() to deal with these variable length arrays - and as
far as the driver is concerned that's an opaque object - it can't know
anything about any particular individual struct clk in the array.

Then we'd need operations to operate on the array itself, which means
much more API baggage.

Also, if it did need to know about one particular struct clk, then
there's problems with avoiding that struct clk in the alldev() versions
(or we'll see drivers doing clk_get() followed by clk_disable() to
work-around any enabling done via the array versions.)

> The fact is that the different clocks for a device are really different
> clocks. A dumb driver may want to request/enable all relevant clocks at
> once while a more sophisticated driver may want to enable the clock for
> accessing registers in the probe function and a baud clock on device
> open time.

I don't think you can get away from drivers having to know about their
individual clocks in some form or other - and I don't think a dumb
driver should be a justification for creating an API.  If a dumb driver
wants to get the clocks for a device it has to behave as if it were
a smart driver and request each one (maybe having an array itself)
such as this:

static const char *con_ids[NR_CLKS] = {
	"foo",
	"bar",
};

struct driver_priv {
	struct clk *clk[NR_CLKS];
};

	
	for (err = i = 0; i < NR_CLKS; i++) {
		priv->clk[i] = clk_get(dev, con_ids[i];
		if (IS_ERR(priv->clk[i])) {
			err = PTR_ERR(priv->clk[i]);
			break;
		}
	}

	if (err) {
		for (i = 0; i < NR_CLKS && !IS_ERR(priv->clk[i]); i++)
			clk_put(priv->clk[i]);
		return err;
	}

This approach also has the advantage that we know what order the clocks
will be in the array, and so we can sensibly index the array to obtain
a particular clock.

However, going back to the bus fabric case, a driver probably doesn't
have the knowledge - it's a platform topology thing - one which can't
be represented by a clock tree.

It might help if we represented our busses closer to reality - like PCI
does - rather than a flattened set of platform devices.  Couple that with
runtime PM and a driver for the bus fabric, and it should fall out
fairly naturally from the infrastructure we already have.
Richard Zhao Oct. 21, 2011, 9 a.m. UTC | #25
On Thu, Sep 22, 2011 at 03:26:56PM -0700, Mike Turquette wrote:
> +/* For USE_COMMON_STRUCT_CLK, these are provided in clk.c, but not exported
> + * through other headers; we don't want them used anywhere but here. */
> +#ifdef CONFIG_USE_COMMON_STRUCT_CLK
change to CONFIG_GENERIC_CLK?
> +extern int __clk_get(struct clk *clk);
> +extern void __clk_put(struct clk *clk);
clk.c does not define it.
> +#endif

Thanks
Richard
Shawn Guo Oct. 23, 2011, 12:55 p.m. UTC | #26
Hi Mike,

Some random comments/nits ...

On Thu, Sep 22, 2011 at 03:26:56PM -0700, Mike Turquette wrote:
> +struct clk *clk_register(const struct clk_hw_ops *ops, struct clk_hw *hw,
> +		const char *name)
> +{
> +	struct clk *clk;
> +
> +	clk = kzalloc(sizeof(*clk), GFP_KERNEL);
> +	if (!clk)
> +		return NULL;
> +
> +	INIT_HLIST_HEAD(&clk->children);
> +	INIT_HLIST_NODE(&clk->child_node);
> +
The children and child_node are introduced in patch #2, and should not
be used in patch #1.

> +	clk->name = name;
> +	clk->ops = ops;
> +	clk->hw = hw;
> +	hw->clk = clk;
> +
> +	/* Query the hardware for parent and initial rate */
> +
> +	if (clk->ops->get_parent)
> +		/* We don't to lock against prepare/enable here, as
> +		 * the clock is not yet accessible from anywhere */
/*
 * Multiple line comments
 */

> +		clk->parent = clk->ops->get_parent(clk->hw);
> +
> +	if (clk->ops->recalc_rate)
> +		clk->rate = clk->ops->recalc_rate(clk->hw);
> +
> +
One blank line is good enough.

> +	return clk;
> +}
Mike Turquette Oct. 23, 2011, 4:49 p.m. UTC | #27
On Sun, Oct 23, 2011 at 5:55 AM, Shawn Guo <shawn.guo@freescale.com> wrote:
> Hi Mike,
>
> Some random comments/nits ...

Thanks for reviewing Shawn.  Will roll changes into V3.

Regards,
Mike
Domenico Andreoli Oct. 27, 2011, 11:54 a.m. UTC | #28
Hi,

On 09/22/2011 05:26 PM, Mike Turquette wrote:
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index 3530927..c53ed59 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -5,3 +5,6 @@ config CLKDEV_LOOKUP
>  
>  config HAVE_MACH_CLKDEV
>  	bool
> +
> +config GENERIC_CLK
> +	bool

Now that Russel's prepare/unprepare patch is mainlined, I think you
want to select HAVE_CLK_PREPARE here (and remove your prepare/unprepare
function declarations in clk.h).

Regards,
Domenico
diff mbox

Patch

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 3530927..c53ed59 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -5,3 +5,6 @@  config CLKDEV_LOOKUP
 
 config HAVE_MACH_CLKDEV
 	bool
+
+config GENERIC_CLK
+	bool
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..1cd7315
--- /dev/null
+++ b/drivers/clk/clk.c
@@ -0,0 +1,232 @@ 
+/*
+ * Copyright (C) 2010-2011 Canonical Ltd <jeremy.kerr@canonical.com>
+ *
+ * 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.
+ */
+
+#include <linux/clk.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+
+struct clk {
+	const char		*name;
+	const struct clk_hw_ops	*ops;
+	struct clk_hw		*hw;
+	unsigned int		enable_count;
+	unsigned int		prepare_count;
+	struct clk		*parent;
+	unsigned long		rate;
+};
+
+static DEFINE_SPINLOCK(enable_lock);
+static DEFINE_MUTEX(prepare_lock);
+
+static 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->hw);
+
+	__clk_unprepare(clk->parent);
+}
+
+void clk_unprepare(struct clk *clk)
+{
+	mutex_lock(&prepare_lock);
+	__clk_unprepare(clk);
+	mutex_unlock(&prepare_lock);
+}
+EXPORT_SYMBOL_GPL(clk_unprepare);
+
+static 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->hw);
+			if (ret) {
+				__clk_unprepare(clk->parent);
+				return ret;
+			}
+		}
+	}
+
+	clk->prepare_count++;
+
+	return 0;
+}
+
+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);
+
+static 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->hw);
+	__clk_disable(clk->parent);
+}
+
+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);
+
+static int __clk_enable(struct clk *clk)
+{
+	int ret;
+
+	if (!clk)
+		return 0;
+
+	if (WARN_ON(clk->prepare_count == 0))
+		return -ESHUTDOWN;
+
+
+	if (clk->enable_count == 0) {
+		ret = __clk_enable(clk->parent);
+		if (ret)
+			return ret;
+
+		if (clk->ops->enable) {
+			ret = clk->ops->enable(clk->hw);
+			if (ret) {
+				__clk_disable(clk->parent);
+				return ret;
+			}
+		}
+	}
+
+	clk->enable_count++;
+	return 0;
+}
+
+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);
+
+unsigned long clk_get_rate(struct clk *clk)
+{
+	if (!clk)
+		return 0;
+	return clk->rate;
+}
+EXPORT_SYMBOL_GPL(clk_get_rate);
+
+long clk_round_rate(struct clk *clk, unsigned long rate)
+{
+	if (clk && clk->ops->round_rate)
+		return clk->ops->round_rate(clk->hw, rate);
+	return rate;
+}
+EXPORT_SYMBOL_GPL(clk_round_rate);
+
+int clk_set_rate(struct clk *clk, unsigned long rate)
+{
+	/* not yet implemented */
+	return -ENOSYS;
+}
+EXPORT_SYMBOL_GPL(clk_set_rate);
+
+struct clk *clk_get_parent(struct clk *clk)
+{
+	if (!clk)
+		return NULL;
+
+	return clk->parent;
+}
+EXPORT_SYMBOL_GPL(clk_get_parent);
+
+int clk_set_parent(struct clk *clk, struct clk *parent)
+{
+	/* not yet implemented */
+	return -ENOSYS;
+}
+EXPORT_SYMBOL_GPL(clk_set_parent);
+
+struct clk *clk_register(const struct clk_hw_ops *ops, struct clk_hw *hw,
+		const char *name)
+{
+	struct clk *clk;
+
+	clk = kzalloc(sizeof(*clk), GFP_KERNEL);
+	if (!clk)
+		return NULL;
+
+	INIT_HLIST_HEAD(&clk->children);
+	INIT_HLIST_NODE(&clk->child_node);
+
+	clk->name = name;
+	clk->ops = ops;
+	clk->hw = hw;
+	hw->clk = clk;
+
+	/* Query the hardware for parent and initial rate */
+
+	if (clk->ops->get_parent)
+		/* We don't to lock against prepare/enable here, as
+		 * the clock is not yet accessible from anywhere */
+		clk->parent = clk->ops->get_parent(clk->hw);
+
+	if (clk->ops->recalc_rate)
+		clk->rate = clk->ops->recalc_rate(clk->hw);
+
+
+	return clk;
+}
+EXPORT_SYMBOL_GPL(clk_register);
diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index 6db161f..e2a9719 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -23,6 +23,13 @@ 
 static LIST_HEAD(clocks);
 static DEFINE_MUTEX(clocks_mutex);
 
+/* For USE_COMMON_STRUCT_CLK, these are provided in clk.c, but not exported
+ * through other headers; we don't want them used anywhere but here. */
+#ifdef CONFIG_USE_COMMON_STRUCT_CLK
+extern int __clk_get(struct clk *clk);
+extern void __clk_put(struct clk *clk);
+#endif
+
 /*
  * Find the correct struct clk for the device and connection ID.
  * We do slightly fuzzy matching here:
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 1d37f42..d6ae10b 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -3,6 +3,7 @@ 
  *
  *  Copyright (C) 2004 ARM Limited.
  *  Written by Deep Blue Solutions Limited.
+ *  Copyright (c) 2010-2011 Jeremy Kerr <jeremy.kerr@canonical.com>
  *
  * 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
@@ -11,17 +12,137 @@ 
 #ifndef __LINUX_CLK_H
 #define __LINUX_CLK_H
 
+#include <linux/kernel.h>
+#include <linux/errno.h>
+
 struct device;
 
-/*
- * The base API.
+struct clk;
+
+#ifdef CONFIG_GENERIC_CLK
+
+struct clk_hw {
+	struct clk *clk;
+};
+
+/**
+ * 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. Called with the global clock mutex
+ *		held. Optional, but recommended - if this op is not set,
+ *		clk_get_rate will return 0.
+ *
+ * @get_parent	Query the parent of this clock; for clocks with multiple
+ *		possible parents, query the hardware for the current
+ *		parent. Currently only called when the clock is first
+ *		registered.
+ *
+ * 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 a clock requires sleeping code to be turned on, this
+ * should be done in clk_prepare. Switching that will not sleep should be done
+ * 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_hw_ops {
+	int		(*prepare)(struct clk_hw *);
+	void		(*unprepare)(struct clk_hw *);
+	int		(*enable)(struct clk_hw *);
+	void		(*disable)(struct clk_hw *);
+	unsigned long	(*recalc_rate)(struct clk_hw *);
+	long		(*round_rate)(struct clk_hw *, unsigned long);
+	struct clk *	(*get_parent)(struct clk_hw *);
+};
 
+/**
+ * clk_prepare - prepare clock for atomic enabling.
+ *
+ * @clk: The clock to prepare
+ *
+ * Do any possibly sleeping initialisation on @clk, allowing the clock to be
+ * later enabled atomically (via clk_enable). This function may sleep.
+ */
+int clk_prepare(struct clk *clk);
+
+/**
+ * clk_unprepare - release clock from prepared state
+ *
+ * @clk: The clock to release
+ *
+ * Do any (possibly sleeping) cleanup on clk. This function may sleep.
+ */
+void clk_unprepare(struct clk *clk);
+
+/**
+ * clk_register - register and initialize a new clock
+ *
+ * @ops: ops for the new clock
+ * @hw: struct clk_hw to be passed to the ops of the new clock
+ * @name: name to use for the new clock
+ *
+ * Register a new clock with the clk subsystem.  Returns either a
+ * struct clk for the new clock or a NULL pointer.
+ */
+struct clk *clk_register(const struct clk_hw_ops *ops, struct clk_hw *hw,
+			 const char *name);
+
+/**
+ * clk_unregister - remove a clock
+ *
+ * @clk: clock to unregister
+ *
+ * Remove a clock from the clk subsystem.  This is currently not
+ * implemented but is provided to allow unregistration code to be
+ * written in drivers ready for use when an implementation is
+ * provided.
+ */
+static inline int clk_unregister(struct clk *clk)
+{
+	return -EOPNOTSUPP;
+}
+
+#else /* !CONFIG_GENERIC_CLK */
 
 /*
- * struct clk - an machine class defined object / cookie.
+ * For !CONFIG_GENERIC_CLK, we don't enforce any atomicity
+ * requirements for clk_enable/clk_disable, so the prepare and unprepare
+ * functions are no-ops
  */
-struct clk;
+static inline int clk_prepare(struct clk *clk) {
+	might_sleep();
+	return 0;
+}
+
+static inline void clk_unprepare(struct clk *clk) {
+	might_sleep();
+}
+
+#endif /* !CONFIG_GENERIC_CLK */
 
 /**
  * clk_get - lookup and obtain a reference to a clock producer.
@@ -67,6 +188,7 @@  void clk_disable(struct clk *clk);
 /**
  * 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);
@@ -83,12 +205,6 @@  unsigned long clk_get_rate(struct clk *clk);
  */
 void clk_put(struct clk *clk);
 
-
-/*
- * The remaining APIs are optional for machine class support.
- */
-
-
 /**
  * clk_round_rate - adjust a rate to the exact rate a clock can provide
  * @clk: clock source
@@ -97,7 +213,7 @@  void clk_put(struct clk *clk);
  * Returns rounded clock rate in Hz, or negative errno.
  */
 long clk_round_rate(struct clk *clk, unsigned long rate);
- 
+
 /**
  * clk_set_rate - set the clock rate for a clock source
  * @clk: clock source
@@ -106,7 +222,7 @@  long clk_round_rate(struct clk *clk, unsigned long rate);
  * Returns success (0) or negative errno.
  */
 int clk_set_rate(struct clk *clk, unsigned long rate);
- 
+
 /**
  * clk_set_parent - set the parent clock source for this clock
  * @clk: clock source