Hi all,
The goal of this series is to provide a cross-platform clock framework that platforms can use to model their clock trees and perform common operations on them. Currently everyone re-invents their own clock tree inside platform code which makes it impossible for drivers to use the clock APIs safely across many platforms and for distro's to compile multi-platform kernels which all redefine struct clk and its operations.
This is the second version of the common clock patches which were originally posted by Jeremy Kerr and then re-posted with some additional patches by Mark Brown. Mark's re-post didn't have any changes done to the original four patches from Jeremy which is why this series is "v2".
The changes in this series are minimal: I've folded in some of Mark's fixes and most of the comments posted to his series as well as rebasing on top of v3.1-rc7. The design and functionality hasn't changed much since Jeremy posted v1 of this series. Propagating the rate change up to the parent has been removed from clk_set_rate since that needs some more thought. I also dropped Mark's change to append a device's name to a clk name since device tree might solve this neatly. Again more discussion around that would be good.
v1 of the series can be found at, http://article.gmane.org/gmane.linux.kernel/1143182
Mark's re-post (v1+) can be found at, http://article.gmane.org/gmane.linux.ports.arm.kernel/129889
Todo in follow-up patches: clk_set_parent Implement a *safe* upstream propagation for parent rate changes Track tree topology and export to userspace (sysfs or debugfs) Manage root clocks globally Device tree support Per-tree locking instead of framework-wide locking Device driver rate-change notifiers
Jeremy Kerr (4): clk: Add a generic clock infrastructure clk: Implement clk_set_rate clk: Add fixed-rate clock clk: Add simple gated clock
Mark Brown (3): clk: Add Kconfig option to build all generic clk drivers clk: Add initial WM831x clock driver x86: Enable generic clk API on x86
MAINTAINERS | 1 + arch/x86/Kconfig | 1 + drivers/clk/Kconfig | 27 +++- drivers/clk/Makefile | 4 + drivers/clk/clk-fixed.c | 24 +++ drivers/clk/clk-gate.c | 78 ++++++++++ drivers/clk/clk-wm831x.c | 386 ++++++++++++++++++++++++++++++++++++++++++++++ drivers/clk/clk.c | 291 ++++++++++++++++++++++++++++++++++ drivers/clk/clkdev.c | 7 + include/linux/clk.h | 179 ++++++++++++++++++++-- 10 files changed, 985 insertions(+), 13 deletions(-) create mode 100644 drivers/clk/clk-fixed.c create mode 100644 drivers/clk/clk-gate.c create mode 100644 drivers/clk/clk-wm831x.c create mode 100644 drivers/clk/clk.c
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); + + + 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
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
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
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
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.
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
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
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.
On Mon, Oct 03, 2011 at 05:43:09PM +0100, Russell King - ARM Linux wrote:
On Mon, Oct 03, 2011 at 05:31:08PM +0100, Mark Brown wrote:
[Not being many off-SoC clocks]
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:
Right, it's consumer side only which is the limitation I'm talking about - at the minute it's only possible to use the clk API for on-SoC clocks (or at least clocks that the SoC cares about), we don't have a clock API for cross platform clock providers. I don't know if getting a cross platform implementation will change that situation much.
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.
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
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
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
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
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.
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
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.
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
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
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.
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%3Ba=shortlog%3Bh...
Thanks Richard
linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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%3Ba=shortlog%3Bh...
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
On Sun, Oct 16, 2011 at 07:55:21PM +0200, Sascha Hauer wrote:
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%3Ba=shortlog%3Bh...
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.
clock group is not limited to help driver get clock. It refects clock dependency. 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.
Thanks Richard
Sascha
-- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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 - 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.
Either way the point here is that individual drivers shouldn't be hand coding this stuff, it should be being handled by core code.
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
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
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.
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 *); };
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
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
On Sun, Oct 16, 2011 at 02:17:29PM -0700, Turquette, Mike wrote:
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.
IMHO, we only need to get parent from hw at register/init time. clk_get_parent can get it from cache, like current code.
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
I don't have clk_hw_imx. I just use generic clks like clk_hw_gate, clk_hw_divider, clk_hw_mux, and some specific clks.
and teach your .ops about it (analogous to struct clk_hw_fixed->rate).
I have to define things like below: stuct pair { struct clk_hw *clk = clk_hw_gate.hw; struct clk_hw_ops *ops; }; and use for (.. ) to register the clk array.
- 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, };
If we don't protect struct clk members, how about the below: struct clk_hw_omap aess_fclk = { .clk = { .name = "aess_fclk", .ops = &aess_fclk_ops, .parent = &abe_clk, }; .clksel = &aess_fclk_div, .clksel_reg = OMAP4430_CM1_ABE_AESS_CLKCTRL, .clksel_mask = OMAP4430_CLKSEL_AESS_FCLK_MASK, };
- 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.
clk group is clk function dependency. I talked about it in another email in this thread. That's ok to leave it to other framework.
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
linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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
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;
+}
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
From: Jeremy Kerr jeremy.kerr@canonical.com
Implement clk_set_rate by adding a set_rate callback to clk_hw_ops. Rates are propagated down the clock tree and recalculated. Also adds a flag for signaling that parents must change rates to achieve the desired frequency (upstream propagation).
TODO: Upstream propagation is not yet implemented. Device pre-change and post-change notifications are not implemented, but are marked up as FIXME comments.
Signed-off-by: Jeremy Kerr jeremy.kerr@canonical.com Signed-off-by: Mark Brown broonie@opensource.wolfsonmicro.com Signed-off-by: Mike Turquette mturquette@ti.com --- Changes since v1: Remove upstream propagation (for now) Rename CLK_SET_RATE_PROPAGATE to CLK_PARENT_RATE_CHANGE
drivers/clk/clk.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++---- include/linux/clk.h | 12 ++++++++ 2 files changed, 77 insertions(+), 6 deletions(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 1cd7315..86636c2 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -21,6 +21,8 @@ struct clk { unsigned int enable_count; unsigned int prepare_count; struct clk *parent; + struct hlist_head children; + struct hlist_node child_node; unsigned long rate; };
@@ -176,10 +178,57 @@ long clk_round_rate(struct clk *clk, unsigned long rate) } EXPORT_SYMBOL_GPL(clk_round_rate);
+/* + * clk_recalc_rates - Given a clock (with a recently updated clk->rate), + * notify its children that the rate may need to be recalculated, using + * ops->recalc_rate(). + */ +static void clk_recalc_rates(struct clk *clk) +{ + struct hlist_node *tmp; + struct clk *child; + + if (clk->ops->recalc_rate) + clk->rate = clk->ops->recalc_rate(clk->hw); + + /* FIXME add post-rate change notification here */ + + hlist_for_each_entry(child, tmp, &clk->children, child_node) + clk_recalc_rates(child); +} + int clk_set_rate(struct clk *clk, unsigned long rate) { - /* not yet implemented */ - return -ENOSYS; + unsigned long parent_rate, new_rate; + int ret = 0; + + if (!clk->ops->set_rate) + return -ENOSYS; + + new_rate = rate; + + /* prevent racing with updates to the clock topology */ + mutex_lock(&prepare_lock); + + /* FIXME add pre-rate change notification here */ + + ret = clk->ops->set_rate(clk->hw, new_rate, &parent_rate); + + /* FIXME ignores CLK_PARENT_RATE_CHANGE */ + if (ret < 0) + /* FIXME add rate change abort notification here */ + goto out; + + /* + * If successful recalculate the rates of the clock, including + * children. + */ + clk_recalc_rates(clk); + +out: + mutex_unlock(&prepare_lock); + + return ret; } EXPORT_SYMBOL_GPL(clk_set_rate);
@@ -216,16 +265,26 @@ struct clk *clk_register(const struct clk_hw_ops *ops, struct clk_hw *hw, clk->hw = hw; hw->clk = clk;
- /* Query the hardware for parent and initial rate */ + /* + * Query the hardware for parent and initial rate. We may alter + * the clock topology, making this clock available from the parent's + * children list. So, we need to protect against concurrent + * accesses through set_rate + */ + mutex_lock(&prepare_lock);
- if (clk->ops->get_parent) - /* We don't to lock against prepare/enable here, as - * the clock is not yet accessible from anywhere */ + if (clk->ops->get_parent) { clk->parent = clk->ops->get_parent(clk->hw); + if (clk->parent) + hlist_add_head(&clk->child_node, + &clk->parent->children); + }
if (clk->ops->recalc_rate) clk->rate = clk->ops->recalc_rate(clk->hw);
+ mutex_unlock(&prepare_lock); +
return clk; } diff --git a/include/linux/clk.h b/include/linux/clk.h index d6ae10b..0d2cd5e 100644 --- a/include/linux/clk.h +++ b/include/linux/clk.h @@ -58,6 +58,12 @@ struct clk_hw { * parent. Currently only called when the clock is first * registered. * + * @set_rate Change the rate of this clock. If this callback returns + * CLK_SET_RATE_PROPAGATE, the rate change will be propagated to + * the parent clock (which may propagate again). The requested + * rate of the parent is passed back from the callback in the + * second 'unsigned long *' argument. + * * 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 @@ -76,9 +82,15 @@ struct clk_hw_ops { void (*disable)(struct clk_hw *); unsigned long (*recalc_rate)(struct clk_hw *); long (*round_rate)(struct clk_hw *, unsigned long); + int (*set_rate)(struct clk_hw *, + unsigned long, unsigned long *); struct clk * (*get_parent)(struct clk_hw *); };
+enum { + CLK_PARENT_RATE_CHANGE = 1, +}; + /** * clk_prepare - prepare clock for atomic enabling. *
On Thu, Sep 22, 2011 at 03:26:57PM -0700, Mike Turquette wrote:
From: Jeremy Kerr jeremy.kerr@canonical.com
Implement clk_set_rate by adding a set_rate callback to clk_hw_ops. Rates are propagated down the clock tree and recalculated. Also adds a flag for signaling that parents must change rates to achieve the desired frequency (upstream propagation).
TODO: Upstream propagation is not yet implemented. Device pre-change and post-change notifications are not implemented, but are marked up as FIXME comments.
Signed-off-by: Jeremy Kerr jeremy.kerr@canonical.com Signed-off-by: Mark Brown broonie@opensource.wolfsonmicro.com Signed-off-by: Mike Turquette mturquette@ti.com
Changes since v1: Remove upstream propagation (for now) Rename CLK_SET_RATE_PROPAGATE to CLK_PARENT_RATE_CHANGE
drivers/clk/clk.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++---- include/linux/clk.h | 12 ++++++++ 2 files changed, 77 insertions(+), 6 deletions(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 1cd7315..86636c2 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -21,6 +21,8 @@ struct clk { unsigned int enable_count; unsigned int prepare_count; struct clk *parent;
- struct hlist_head children;
- struct hlist_node child_node; unsigned long rate;
}; @@ -176,10 +178,57 @@ long clk_round_rate(struct clk *clk, unsigned long rate) } EXPORT_SYMBOL_GPL(clk_round_rate); +/*
- clk_recalc_rates - Given a clock (with a recently updated clk->rate),
- notify its children that the rate may need to be recalculated, using
- ops->recalc_rate().
- */
+static void clk_recalc_rates(struct clk *clk) +{
- struct hlist_node *tmp;
- struct clk *child;
- if (clk->ops->recalc_rate)
clk->rate = clk->ops->recalc_rate(clk->hw);
pass it parent rate if recalc_rate is NULL?
- /* FIXME add post-rate change notification here */
- hlist_for_each_entry(child, tmp, &clk->children, child_node)
clk_recalc_rates(child);+}
int clk_set_rate(struct clk *clk, unsigned long rate) {
- /* not yet implemented */
- return -ENOSYS;
- unsigned long parent_rate, new_rate;
- int ret = 0;
- if (!clk->ops->set_rate)
return -ENOSYS;- new_rate = rate;
- /* prevent racing with updates to the clock topology */
- mutex_lock(&prepare_lock);
- /* FIXME add pre-rate change notification here */
- ret = clk->ops->set_rate(clk->hw, new_rate, &parent_rate);
- /* FIXME ignores CLK_PARENT_RATE_CHANGE */
why do we need upstream propagate? for cascaded dividers? It may effect parent's other children. Does rate have to be rate returned by round_rate? I guess we can accept none exact rate.
Thanks Richard
- if (ret < 0)
/* FIXME add rate change abort notification here */goto out;- /*
* If successful recalculate the rates of the clock, including* children.*/- clk_recalc_rates(clk);
+out:
- mutex_unlock(&prepare_lock);
- return ret;
} EXPORT_SYMBOL_GPL(clk_set_rate); @@ -216,16 +265,26 @@ struct clk *clk_register(const struct clk_hw_ops *ops, struct clk_hw *hw, clk->hw = hw; hw->clk = clk;
- /* Query the hardware for parent and initial rate */
- /*
* Query the hardware for parent and initial rate. We may alter* the clock topology, making this clock available from the parent's* children list. So, we need to protect against concurrent* accesses through set_rate*/- mutex_lock(&prepare_lock);
- if (clk->ops->get_parent)
/* We don't to lock against prepare/enable here, as* the clock is not yet accessible from anywhere */
- if (clk->ops->get_parent) { clk->parent = clk->ops->get_parent(clk->hw);
if (clk->parent)hlist_add_head(&clk->child_node,&clk->parent->children);- }
if (clk->ops->recalc_rate) clk->rate = clk->ops->recalc_rate(clk->hw);
- mutex_unlock(&prepare_lock);
return clk; } diff --git a/include/linux/clk.h b/include/linux/clk.h index d6ae10b..0d2cd5e 100644 --- a/include/linux/clk.h +++ b/include/linux/clk.h @@ -58,6 +58,12 @@ struct clk_hw {
parent. Currently only called when the clock is firstregistered.
- @set_rate Change the rate of this clock. If this callback returns
CLK_SET_RATE_PROPAGATE, the rate change will be propagated to
the parent clock (which may propagate again). The requested
rate of the parent is passed back from the callback in the
second 'unsigned long *' argument.
- 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
@@ -76,9 +82,15 @@ struct clk_hw_ops { void (*disable)(struct clk_hw *); unsigned long (*recalc_rate)(struct clk_hw *); long (*round_rate)(struct clk_hw *, unsigned long);
- int (*set_rate)(struct clk_hw *,
unsigned long, unsigned long *);}; +enum {
- CLK_PARENT_RATE_CHANGE = 1,
+};
/**
- clk_prepare - prepare clock for atomic enabling.
-- 1.7.4.1
linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Thu, Sep 22, 2011 at 03:26:57PM -0700, Mike Turquette wrote:
From: Jeremy Kerr jeremy.kerr@canonical.com
Implement clk_set_rate by adding a set_rate callback to clk_hw_ops. Rates are propagated down the clock tree and recalculated. Also adds a flag for signaling that parents must change rates to achieve the desired frequency (upstream propagation).
TODO: Upstream propagation is not yet implemented. Device pre-change and post-change notifications are not implemented, but are marked up as FIXME comments.
Signed-off-by: Jeremy Kerr jeremy.kerr@canonical.com Signed-off-by: Mark Brown broonie@opensource.wolfsonmicro.com Signed-off-by: Mike Turquette mturquette@ti.com
Changes since v1: Remove upstream propagation (for now) Rename CLK_SET_RATE_PROPAGATE to CLK_PARENT_RATE_CHANGE
[...]
- @set_rate Change the rate of this clock. If this callback returns
CLK_SET_RATE_PROPAGATE, the rate change will be propagated to
s/CLK_SET_RATE_PROPAGATE/CLK_PARENT_RATE_CHANGE, as suggested by your change log above?
the parent clock (which may propagate again). The requested
rate of the parent is passed back from the callback in the
second 'unsigned long *' argument.
On Sun, Oct 23, 2011 at 7:24 AM, Shawn Guo shawn.guo@freescale.com wrote:
On Thu, Sep 22, 2011 at 03:26:57PM -0700, Mike Turquette wrote:
From: Jeremy Kerr jeremy.kerr@canonical.com
[...]
- @set_rate Change the rate of this clock. If this callback returns
- CLK_SET_RATE_PROPAGATE, the rate change will be propagated to
s/CLK_SET_RATE_PROPAGATE/CLK_PARENT_RATE_CHANGE, as suggested by your change log above?
Thanks for reviewing. Will roll into V3 patchset.
Regards, Mike
- the parent clock (which may propagate again). The requested
- rate of the parent is passed back from the callback in the
- second 'unsigned long *' argument.
-- Regards, Shawn
From: Jeremy Kerr jeremy.kerr@canonical.com
Signed-off-by: Jeremy Kerr jeremy.kerr@canonical.com Signed-off-by: Mark Brown broonie@opensource.wolfsonmicro.com Signed-off-by: Mike Turquette mturquette@ti.com --- Changes since v1: Add copyright header
drivers/clk/Kconfig | 4 ++++ drivers/clk/Makefile | 1 + drivers/clk/clk-fixed.c | 24 ++++++++++++++++++++++++ include/linux/clk.h | 14 ++++++++++++++ 4 files changed, 43 insertions(+), 0 deletions(-) create mode 100644 drivers/clk/clk-fixed.c
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig index c53ed59..d8313d7 100644 --- a/drivers/clk/Kconfig +++ b/drivers/clk/Kconfig @@ -8,3 +8,7 @@ config HAVE_MACH_CLKDEV
config GENERIC_CLK bool + +config GENERIC_CLK_FIXED + bool + depends on GENERIC_CLK diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile index 570d5b9..9a3325a 100644 --- a/drivers/clk/Makefile +++ b/drivers/clk/Makefile @@ -1,3 +1,4 @@
obj-$(CONFIG_CLKDEV_LOOKUP) += clkdev.o obj-$(CONFIG_GENERIC_CLK) += clk.o +obj-$(CONFIG_GENERIC_CLK_FIXED) += clk-fixed.o diff --git a/drivers/clk/clk-fixed.c b/drivers/clk/clk-fixed.c new file mode 100644 index 0000000..956fb9a --- /dev/null +++ b/drivers/clk/clk-fixed.c @@ -0,0 +1,24 @@ +/* + * 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. + * + * Simple fixed-rate clock implementation + */ + +#include <linux/clk.h> +#include <linux/module.h> + +#define to_clk_fixed(c) container_of(c, struct clk_hw_fixed, hw) + +static unsigned long clk_fixed_recalc_rate(struct clk_hw *hw) +{ + return to_clk_fixed(hw)->rate; +} + +struct clk_hw_ops clk_fixed_ops = { + .recalc_rate = clk_fixed_recalc_rate, +}; +EXPORT_SYMBOL_GPL(clk_fixed_ops); diff --git a/include/linux/clk.h b/include/linux/clk.h index 0d2cd5e..1903dd8 100644 --- a/include/linux/clk.h +++ b/include/linux/clk.h @@ -110,6 +110,20 @@ int clk_prepare(struct clk *clk); */ void clk_unprepare(struct clk *clk);
+/* Base clock implementations. Platform clock implementations can use these + * directly, or 'subclass' as approprate */ + +#ifdef CONFIG_GENERIC_CLK_FIXED + +struct clk_hw_fixed { + struct clk_hw hw; + unsigned long rate; +}; + +extern struct clk_hw_ops clk_fixed_ops; + +#endif /* CONFIG_GENERIC_CLK_FIXED */ + /** * clk_register - register and initialize a new clock *
On Thu, Sep 22, 2011 at 03:26:58PM -0700, Mike Turquette wrote:
From: Jeremy Kerr jeremy.kerr@canonical.com
Signed-off-by: Jeremy Kerr jeremy.kerr@canonical.com Signed-off-by: Mark Brown broonie@opensource.wolfsonmicro.com Signed-off-by: Mike Turquette mturquette@ti.com
Changes since v1: Add copyright header
drivers/clk/Kconfig | 4 ++++ drivers/clk/Makefile | 1 + drivers/clk/clk-fixed.c | 24 ++++++++++++++++++++++++ include/linux/clk.h | 14 ++++++++++++++ 4 files changed, 43 insertions(+), 0 deletions(-) create mode 100644 drivers/clk/clk-fixed.c
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig index c53ed59..d8313d7 100644 --- a/drivers/clk/Kconfig +++ b/drivers/clk/Kconfig @@ -8,3 +8,7 @@ config HAVE_MACH_CLKDEV config GENERIC_CLK bool
+config GENERIC_CLK_FIXED
- bool
- depends on GENERIC_CLK
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile index 570d5b9..9a3325a 100644 --- a/drivers/clk/Makefile +++ b/drivers/clk/Makefile @@ -1,3 +1,4 @@ obj-$(CONFIG_CLKDEV_LOOKUP) += clkdev.o obj-$(CONFIG_GENERIC_CLK) += clk.o +obj-$(CONFIG_GENERIC_CLK_FIXED) += clk-fixed.o diff --git a/drivers/clk/clk-fixed.c b/drivers/clk/clk-fixed.c new file mode 100644 index 0000000..956fb9a --- /dev/null +++ b/drivers/clk/clk-fixed.c @@ -0,0 +1,24 @@ +/*
- 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.
- Simple fixed-rate clock implementation
- */
+#include <linux/clk.h> +#include <linux/module.h>
+#define to_clk_fixed(c) container_of(c, struct clk_hw_fixed, hw)
+static unsigned long clk_fixed_recalc_rate(struct clk_hw *hw) +{
- return to_clk_fixed(hw)->rate;
+}
+struct clk_hw_ops clk_fixed_ops = {
- .recalc_rate = clk_fixed_recalc_rate,
+}; +EXPORT_SYMBOL_GPL(clk_fixed_ops); diff --git a/include/linux/clk.h b/include/linux/clk.h index 0d2cd5e..1903dd8 100644 --- a/include/linux/clk.h +++ b/include/linux/clk.h @@ -110,6 +110,20 @@ int clk_prepare(struct clk *clk); */ void clk_unprepare(struct clk *clk); +/* Base clock implementations. Platform clock implementations can use these
- directly, or 'subclass' as approprate */
/* * Multiple lines comments */
Regards, Shawn
+#ifdef CONFIG_GENERIC_CLK_FIXED
+struct clk_hw_fixed {
- struct clk_hw hw;
- unsigned long rate;
+};
+extern struct clk_hw_ops clk_fixed_ops;
+#endif /* CONFIG_GENERIC_CLK_FIXED */
/**
- clk_register - register and initialize a new clock
-- 1.7.4.1
On Sun, Oct 23, 2011 at 7:30 AM, Shawn Guo shawn.guo@freescale.com wrote:
On Thu, Sep 22, 2011 at 03:26:58PM -0700, Mike Turquette wrote:
From: Jeremy Kerr jeremy.kerr@canonical.com +/* Base clock implementations. Platform clock implementations can use these
- directly, or 'subclass' as approprate */
/* * Multiple lines comments */
Thanks for the review Shawn. Will roll into V3 patchset.
Regards, Mike
Regards, Shawn
From: Jeremy Kerr jeremy.kerr@canonical.com
Signed-off-by: Jeremy Kerr jeremy.kerr@canonical.com Signed-off-by: Mark Brown broonie@opensource.wolfsonmicro.com Signed-off-by: Jamie Iles jamie@jamieiles.com Signed-off-by: Mike Turquette mturquette@ti.com --- Changes since v1: Add copyright header Fold in Jamie's patch for set-to-disable clks Use BIT macro instead of shift
drivers/clk/Kconfig | 4 ++ drivers/clk/Makefile | 1 + drivers/clk/clk-gate.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++ include/linux/clk.h | 13 ++++++++ 4 files changed, 96 insertions(+), 0 deletions(-) create mode 100644 drivers/clk/clk-gate.c
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig index d8313d7..a78967c 100644 --- a/drivers/clk/Kconfig +++ b/drivers/clk/Kconfig @@ -12,3 +12,7 @@ config GENERIC_CLK config GENERIC_CLK_FIXED bool depends on GENERIC_CLK + +config GENERIC_CLK_GATE + bool + depends on GENERIC_CLK diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile index 9a3325a..d186446 100644 --- a/drivers/clk/Makefile +++ b/drivers/clk/Makefile @@ -2,3 +2,4 @@ obj-$(CONFIG_CLKDEV_LOOKUP) += clkdev.o obj-$(CONFIG_GENERIC_CLK) += clk.o obj-$(CONFIG_GENERIC_CLK_FIXED) += clk-fixed.o +obj-$(CONFIG_GENERIC_CLK_GATE) += clk-gate.o diff --git a/drivers/clk/clk-gate.c b/drivers/clk/clk-gate.c new file mode 100644 index 0000000..a1d8e79 --- /dev/null +++ b/drivers/clk/clk-gate.c @@ -0,0 +1,78 @@ +/* + * 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. + * + * Simple clk gate implementation + */ + +#include <linux/clk.h> +#include <linux/module.h> +#include <asm/io.h> + +#define to_clk_gate(clk) container_of(clk, struct clk_gate, hw) + +static unsigned long clk_gate_get_rate(struct clk_hw *clk) +{ + return clk_get_rate(clk_get_parent(clk->clk)); +} + +static void clk_gate_set_bit(struct clk_hw *clk) +{ + struct clk_gate *gate = to_clk_gate(clk); + u32 reg; + + reg = __raw_readl(gate->reg); + reg |= BIT(gate->bit_idx); + __raw_writel(reg, gate->reg); +} + +static void clk_gate_clear_bit(struct clk_hw *clk) +{ + struct clk_gate *gate = to_clk_gate(clk); + u32 reg; + + reg = __raw_readl(gate->reg); + reg &= ~BIT(gate->bit_idx); + __raw_writel(reg, gate->reg); +} + +static int clk_gate_enable_set(struct clk_hw *clk) +{ + clk_gate_set_bit(clk); + + return 0; +} + +static void clk_gate_disable_clear(struct clk_hw *clk) +{ + clk_gate_clear_bit(clk); +} + +struct clk_hw_ops clk_gate_set_enable_ops = { + .recalc_rate = clk_gate_get_rate, + .enable = clk_gate_enable_set, + .disable = clk_gate_disable_clear, +}; +EXPORT_SYMBOL_GPL(clk_gate_set_enable_ops); + +static int clk_gate_enable_clear(struct clk_hw *clk) +{ + clk_gate_clear_bit(clk); + + return 0; +} + +static void clk_gate_disable_set(struct clk_hw *clk) +{ + clk_gate_set_bit(clk); +} + +struct clk_hw_ops clk_gate_set_disable_ops = { + .recalc_rate = clk_gate_get_rate, + .enable = clk_gate_enable_clear, + .disable = clk_gate_disable_set, +}; +EXPORT_SYMBOL_GPL(clk_gate_set_disable_ops); diff --git a/include/linux/clk.h b/include/linux/clk.h index 1903dd8..626fd0d 100644 --- a/include/linux/clk.h +++ b/include/linux/clk.h @@ -124,6 +124,19 @@ extern struct clk_hw_ops clk_fixed_ops;
#endif /* CONFIG_GENERIC_CLK_FIXED */
+#ifdef CONFIG_GENERIC_CLK_GATE + +struct clk_gate { + struct clk_hw hw; + void __iomem *reg; + u8 bit_idx; +}; + +extern struct clk_hw_ops clk_gate_set_enable_ops; +extern struct clk_hw_ops clk_gate_set_disable_ops; + +#endif /* CONFIG_GENERIC_CLK_GATE */ + /** * clk_register - register and initialize a new clock *
On Thu, Sep 22, 2011 at 03:26:59PM -0700, Mike Turquette wrote:
From: Jeremy Kerr jeremy.kerr@canonical.com
Signed-off-by: Jeremy Kerr jeremy.kerr@canonical.com Signed-off-by: Mark Brown broonie@opensource.wolfsonmicro.com Signed-off-by: Jamie Iles jamie@jamieiles.com Signed-off-by: Mike Turquette mturquette@ti.com
Changes since v1: Add copyright header Fold in Jamie's patch for set-to-disable clks Use BIT macro instead of shift
drivers/clk/Kconfig | 4 ++ drivers/clk/Makefile | 1 + drivers/clk/clk-gate.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++ include/linux/clk.h | 13 ++++++++ 4 files changed, 96 insertions(+), 0 deletions(-) create mode 100644 drivers/clk/clk-gate.c
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig index d8313d7..a78967c 100644 --- a/drivers/clk/Kconfig +++ b/drivers/clk/Kconfig @@ -12,3 +12,7 @@ config GENERIC_CLK config GENERIC_CLK_FIXED bool depends on GENERIC_CLK
+config GENERIC_CLK_GATE
- bool
- depends on GENERIC_CLK
I see zero documentation on what a "gated clock" is supposed to be or how it works, and there are zero comments in the code. It's kind of hard to review that way, and even harder to use.
g.
On Sat, Sep 24, 2011 at 9:02 PM, Grant Likely grant.likely@secretlab.ca wrote:
On Thu, Sep 22, 2011 at 03:26:59PM -0700, Mike Turquette wrote:
From: Jeremy Kerr jeremy.kerr@canonical.com
Signed-off-by: Jeremy Kerr jeremy.kerr@canonical.com Signed-off-by: Mark Brown broonie@opensource.wolfsonmicro.com Signed-off-by: Jamie Iles jamie@jamieiles.com Signed-off-by: Mike Turquette mturquette@ti.com
Changes since v1: Add copyright header Fold in Jamie's patch for set-to-disable clks Use BIT macro instead of shift
drivers/clk/Kconfig | 4 ++ drivers/clk/Makefile | 1 + drivers/clk/clk-gate.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++ include/linux/clk.h | 13 ++++++++ 4 files changed, 96 insertions(+), 0 deletions(-) create mode 100644 drivers/clk/clk-gate.c
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig index d8313d7..a78967c 100644 --- a/drivers/clk/Kconfig +++ b/drivers/clk/Kconfig @@ -12,3 +12,7 @@ config GENERIC_CLK config GENERIC_CLK_FIXED bool depends on GENERIC_CLK
+config GENERIC_CLK_GATE
- bool
- depends on GENERIC_CLK
I see zero documentation on what a "gated clock" is supposed to be or how it works, and there are zero comments in the code. It's kind of hard to review that way, and even harder to use.
Will add Documentation and re-post.
Thanks, Mike
g.
Mike,
On 09/22/2011 05:26 PM, Mike Turquette wrote:
From: Jeremy Kerr jeremy.kerr@canonical.com
Signed-off-by: Jeremy Kerr jeremy.kerr@canonical.com Signed-off-by: Mark Brown broonie@opensource.wolfsonmicro.com Signed-off-by: Jamie Iles jamie@jamieiles.com Signed-off-by: Mike Turquette mturquette@ti.com
Changes since v1: Add copyright header Fold in Jamie's patch for set-to-disable clks Use BIT macro instead of shift
drivers/clk/Kconfig | 4 ++ drivers/clk/Makefile | 1 + drivers/clk/clk-gate.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++ include/linux/clk.h | 13 ++++++++ 4 files changed, 96 insertions(+), 0 deletions(-) create mode 100644 drivers/clk/clk-gate.c
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig index d8313d7..a78967c 100644 --- a/drivers/clk/Kconfig +++ b/drivers/clk/Kconfig @@ -12,3 +12,7 @@ config GENERIC_CLK config GENERIC_CLK_FIXED bool depends on GENERIC_CLK
+config GENERIC_CLK_GATE
- bool
- depends on GENERIC_CLK
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile index 9a3325a..d186446 100644 --- a/drivers/clk/Makefile +++ b/drivers/clk/Makefile @@ -2,3 +2,4 @@ obj-$(CONFIG_CLKDEV_LOOKUP) += clkdev.o obj-$(CONFIG_GENERIC_CLK) += clk.o obj-$(CONFIG_GENERIC_CLK_FIXED) += clk-fixed.o +obj-$(CONFIG_GENERIC_CLK_GATE) += clk-gate.o diff --git a/drivers/clk/clk-gate.c b/drivers/clk/clk-gate.c new file mode 100644 index 0000000..a1d8e79 --- /dev/null +++ b/drivers/clk/clk-gate.c @@ -0,0 +1,78 @@ +/*
- 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.
- Simple clk gate implementation
- */
+#include <linux/clk.h> +#include <linux/module.h> +#include <asm/io.h>
use linux/io.h
+#define to_clk_gate(clk) container_of(clk, struct clk_gate, hw)
+static unsigned long clk_gate_get_rate(struct clk_hw *clk) +{
- return clk_get_rate(clk_get_parent(clk->clk));
+}
+static void clk_gate_set_bit(struct clk_hw *clk) +{
- struct clk_gate *gate = to_clk_gate(clk);
- u32 reg;
- reg = __raw_readl(gate->reg);
- reg |= BIT(gate->bit_idx);
- __raw_writel(reg, gate->reg);
Don't these read-mod-writes need a spinlock around it?
It's possible to have an enable bits and dividers in the same register. If you did a set_rate and while doing an enable/disable, there would be a problem. Also, it may be 2 different clocks in the same register, so the spinlock needs to be shared and not per clock.
+}
+static void clk_gate_clear_bit(struct clk_hw *clk) +{
- struct clk_gate *gate = to_clk_gate(clk);
- u32 reg;
- reg = __raw_readl(gate->reg);
- reg &= ~BIT(gate->bit_idx);
- __raw_writel(reg, gate->reg);
+}
+static int clk_gate_enable_set(struct clk_hw *clk) +{
- clk_gate_set_bit(clk);
- return 0;
+}
+static void clk_gate_disable_clear(struct clk_hw *clk) +{
- clk_gate_clear_bit(clk);
+}
+struct clk_hw_ops clk_gate_set_enable_ops = {
const?
- .recalc_rate = clk_gate_get_rate,
- .enable = clk_gate_enable_set,
- .disable = clk_gate_disable_clear,
+}; +EXPORT_SYMBOL_GPL(clk_gate_set_enable_ops);
+static int clk_gate_enable_clear(struct clk_hw *clk) +{
- clk_gate_clear_bit(clk);
- return 0;
+}
+static void clk_gate_disable_set(struct clk_hw *clk) +{
- clk_gate_set_bit(clk);
+}
Are these wrapper functions really needed? Just assign set_bit and clear_bit functions directly to the ops structs. Only the ops struct name is exposed to the user.
+struct clk_hw_ops clk_gate_set_disable_ops = {
const?
Rob
Hi Rob,
On Mon, Sep 26, 2011 at 01:33:08PM -0500, Rob Herring wrote:
Mike,
On 09/22/2011 05:26 PM, Mike Turquette wrote:
From: Jeremy Kerr jeremy.kerr@canonical.com
Signed-off-by: Jeremy Kerr jeremy.kerr@canonical.com Signed-off-by: Mark Brown broonie@opensource.wolfsonmicro.com Signed-off-by: Jamie Iles jamie@jamieiles.com Signed-off-by: Mike Turquette mturquette@ti.com
Changes since v1: Add copyright header Fold in Jamie's patch for set-to-disable clks Use BIT macro instead of shift
drivers/clk/Kconfig | 4 ++ drivers/clk/Makefile | 1 + drivers/clk/clk-gate.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++ include/linux/clk.h | 13 ++++++++ 4 files changed, 96 insertions(+), 0 deletions(-) create mode 100644 drivers/clk/clk-gate.c
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig index d8313d7..a78967c 100644 --- a/drivers/clk/Kconfig +++ b/drivers/clk/Kconfig @@ -12,3 +12,7 @@ config GENERIC_CLK config GENERIC_CLK_FIXED bool depends on GENERIC_CLK
+config GENERIC_CLK_GATE
- bool
- depends on GENERIC_CLK
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile index 9a3325a..d186446 100644 --- a/drivers/clk/Makefile +++ b/drivers/clk/Makefile @@ -2,3 +2,4 @@ obj-$(CONFIG_CLKDEV_LOOKUP) += clkdev.o obj-$(CONFIG_GENERIC_CLK) += clk.o obj-$(CONFIG_GENERIC_CLK_FIXED) += clk-fixed.o +obj-$(CONFIG_GENERIC_CLK_GATE) += clk-gate.o diff --git a/drivers/clk/clk-gate.c b/drivers/clk/clk-gate.c new file mode 100644 index 0000000..a1d8e79 --- /dev/null +++ b/drivers/clk/clk-gate.c @@ -0,0 +1,78 @@ +/*
- 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.
- Simple clk gate implementation
- */
+#include <linux/clk.h> +#include <linux/module.h> +#include <asm/io.h>
use linux/io.h
+#define to_clk_gate(clk) container_of(clk, struct clk_gate, hw)
+static unsigned long clk_gate_get_rate(struct clk_hw *clk) +{
- return clk_get_rate(clk_get_parent(clk->clk));
+}
+static void clk_gate_set_bit(struct clk_hw *clk) +{
- struct clk_gate *gate = to_clk_gate(clk);
- u32 reg;
- reg = __raw_readl(gate->reg);
- reg |= BIT(gate->bit_idx);
- __raw_writel(reg, gate->reg);
Don't these read-mod-writes need a spinlock around it?
It's possible to have an enable bits and dividers in the same register. If you did a set_rate and while doing an enable/disable, there would be a problem. Also, it may be 2 different clocks in the same register, so the spinlock needs to be shared and not per clock.
Well the prepare lock will be held here and I believe that would be sufficient.
+}
+static void clk_gate_clear_bit(struct clk_hw *clk) +{
- struct clk_gate *gate = to_clk_gate(clk);
- u32 reg;
- reg = __raw_readl(gate->reg);
- reg &= ~BIT(gate->bit_idx);
- __raw_writel(reg, gate->reg);
+}
+static int clk_gate_enable_set(struct clk_hw *clk) +{
- clk_gate_set_bit(clk);
- return 0;
+}
+static void clk_gate_disable_clear(struct clk_hw *clk) +{
- clk_gate_clear_bit(clk);
+}
+struct clk_hw_ops clk_gate_set_enable_ops = {
const?
Yup.
- .recalc_rate = clk_gate_get_rate,
- .enable = clk_gate_enable_set,
- .disable = clk_gate_disable_clear,
+}; +EXPORT_SYMBOL_GPL(clk_gate_set_enable_ops);
+static int clk_gate_enable_clear(struct clk_hw *clk) +{
- clk_gate_clear_bit(clk);
- return 0;
+}
+static void clk_gate_disable_set(struct clk_hw *clk) +{
- clk_gate_set_bit(clk);
+}
Are these wrapper functions really needed? Just assign set_bit and clear_bit functions directly to the ops structs. Only the ops struct name is exposed to the user.
I used the wrappers because the .enable method has to return an int, but the disable needs to return void. It's either that or open code the set/clear in each.
+struct clk_hw_ops clk_gate_set_disable_ops = {
const?
Yes.
Jamie
On 09/26/2011 01:40 PM, Jamie Iles wrote:
Hi Rob,
On Mon, Sep 26, 2011 at 01:33:08PM -0500, Rob Herring wrote:
Mike,
On 09/22/2011 05:26 PM, Mike Turquette wrote:
From: Jeremy Kerr jeremy.kerr@canonical.com
Signed-off-by: Jeremy Kerr jeremy.kerr@canonical.com Signed-off-by: Mark Brown broonie@opensource.wolfsonmicro.com Signed-off-by: Jamie Iles jamie@jamieiles.com Signed-off-by: Mike Turquette mturquette@ti.com
Changes since v1: Add copyright header Fold in Jamie's patch for set-to-disable clks Use BIT macro instead of shift
drivers/clk/Kconfig | 4 ++ drivers/clk/Makefile | 1 + drivers/clk/clk-gate.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++ include/linux/clk.h | 13 ++++++++ 4 files changed, 96 insertions(+), 0 deletions(-) create mode 100644 drivers/clk/clk-gate.c
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig index d8313d7..a78967c 100644 --- a/drivers/clk/Kconfig +++ b/drivers/clk/Kconfig @@ -12,3 +12,7 @@ config GENERIC_CLK config GENERIC_CLK_FIXED bool depends on GENERIC_CLK
+config GENERIC_CLK_GATE
- bool
- depends on GENERIC_CLK
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile index 9a3325a..d186446 100644 --- a/drivers/clk/Makefile +++ b/drivers/clk/Makefile @@ -2,3 +2,4 @@ obj-$(CONFIG_CLKDEV_LOOKUP) += clkdev.o obj-$(CONFIG_GENERIC_CLK) += clk.o obj-$(CONFIG_GENERIC_CLK_FIXED) += clk-fixed.o +obj-$(CONFIG_GENERIC_CLK_GATE) += clk-gate.o diff --git a/drivers/clk/clk-gate.c b/drivers/clk/clk-gate.c new file mode 100644 index 0000000..a1d8e79 --- /dev/null +++ b/drivers/clk/clk-gate.c @@ -0,0 +1,78 @@ +/*
- 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.
- Simple clk gate implementation
- */
+#include <linux/clk.h> +#include <linux/module.h> +#include <asm/io.h>
use linux/io.h
+#define to_clk_gate(clk) container_of(clk, struct clk_gate, hw)
+static unsigned long clk_gate_get_rate(struct clk_hw *clk) +{
- return clk_get_rate(clk_get_parent(clk->clk));
+}
+static void clk_gate_set_bit(struct clk_hw *clk) +{
- struct clk_gate *gate = to_clk_gate(clk);
- u32 reg;
- reg = __raw_readl(gate->reg);
- reg |= BIT(gate->bit_idx);
- __raw_writel(reg, gate->reg);
Don't these read-mod-writes need a spinlock around it?
It's possible to have an enable bits and dividers in the same register. If you did a set_rate and while doing an enable/disable, there would be a problem. Also, it may be 2 different clocks in the same register, so the spinlock needs to be shared and not per clock.
Well the prepare lock will be held here and I believe that would be sufficient.
No, the enable spinlock is protecting enable/disable. But set_rate is protected by the prepare mutex. So you clearly don't need locking if you have a register of only 1 bit enables. If you have a register accessed by both enable/disable and prepare/unprepare/set_rate, then you need some protection.
+}
+static void clk_gate_clear_bit(struct clk_hw *clk) +{
- struct clk_gate *gate = to_clk_gate(clk);
- u32 reg;
- reg = __raw_readl(gate->reg);
- reg &= ~BIT(gate->bit_idx);
- __raw_writel(reg, gate->reg);
+}
+static int clk_gate_enable_set(struct clk_hw *clk) +{
- clk_gate_set_bit(clk);
- return 0;
+}
+static void clk_gate_disable_clear(struct clk_hw *clk) +{
- clk_gate_clear_bit(clk);
+}
+struct clk_hw_ops clk_gate_set_enable_ops = {
const?
Yup.
- .recalc_rate = clk_gate_get_rate,
- .enable = clk_gate_enable_set,
- .disable = clk_gate_disable_clear,
+}; +EXPORT_SYMBOL_GPL(clk_gate_set_enable_ops);
+static int clk_gate_enable_clear(struct clk_hw *clk) +{
- clk_gate_clear_bit(clk);
- return 0;
+}
+static void clk_gate_disable_set(struct clk_hw *clk) +{
- clk_gate_set_bit(clk);
+}
Are these wrapper functions really needed? Just assign set_bit and clear_bit functions directly to the ops structs. Only the ops struct name is exposed to the user.
I used the wrappers because the .enable method has to return an int, but the disable needs to return void. It's either that or open code the set/clear in each.
Okay. I missed that detail...
Rob
On Mon, Sep 26, 2011 at 02:10:32PM -0500, Rob Herring wrote:
On 09/26/2011 01:40 PM, Jamie Iles wrote:
On Mon, Sep 26, 2011 at 01:33:08PM -0500, Rob Herring wrote:
+static void clk_gate_set_bit(struct clk_hw *clk) +{
- struct clk_gate *gate = to_clk_gate(clk);
- u32 reg;
- reg = __raw_readl(gate->reg);
- reg |= BIT(gate->bit_idx);
- __raw_writel(reg, gate->reg);
Don't these read-mod-writes need a spinlock around it?
It's possible to have an enable bits and dividers in the same register. If you did a set_rate and while doing an enable/disable, there would be a problem. Also, it may be 2 different clocks in the same register, so the spinlock needs to be shared and not per clock.
Well the prepare lock will be held here and I believe that would be sufficient.
No, the enable spinlock is protecting enable/disable. But set_rate is protected by the prepare mutex. So you clearly don't need locking if you have a register of only 1 bit enables. If you have a register accessed by both enable/disable and prepare/unprepare/set_rate, then you need some protection.
OK fair point, but I would guess that if you had a clock like this then you probably wouldn't use this simple gated clock would you? (speaking from my world where we have quite simple clocks ;-))
Jamie
On Mon, Sep 26, 2011 at 12:37 PM, Jamie Iles jamie@jamieiles.com wrote:
On Mon, Sep 26, 2011 at 02:10:32PM -0500, Rob Herring wrote:
On 09/26/2011 01:40 PM, Jamie Iles wrote:
On Mon, Sep 26, 2011 at 01:33:08PM -0500, Rob Herring wrote:
+static void clk_gate_set_bit(struct clk_hw *clk) +{
- struct clk_gate *gate = to_clk_gate(clk);
- u32 reg;
- reg = __raw_readl(gate->reg);
- reg |= BIT(gate->bit_idx);
- __raw_writel(reg, gate->reg);
Don't these read-mod-writes need a spinlock around it?
It's possible to have an enable bits and dividers in the same register. If you did a set_rate and while doing an enable/disable, there would be a problem. Also, it may be 2 different clocks in the same register, so the spinlock needs to be shared and not per clock.
Well the prepare lock will be held here and I believe that would be sufficient.
No, the enable spinlock is protecting enable/disable. But set_rate is protected by the prepare mutex. So you clearly don't need locking if you have a register of only 1 bit enables. If you have a register accessed by both enable/disable and prepare/unprepare/set_rate, then you need some protection.
OK fair point, but I would guess that if you had a clock like this then you probably wouldn't use this simple gated clock would you? (speaking from my world where we have quite simple clocks ;-))
I think it is a safe assumption that if a register controls both enable/disable and some programmable divider then,
1) those controls are probably for the same clock 2) that clock won't be using the cookie-cutter gated-clock implementation anyways
Rob, do you feel these assumptions are OK and locking can remain the same in this patch?
Regards, Mike
Jamie
On 09/26/2011 05:37 PM, Turquette, Mike wrote:
On Mon, Sep 26, 2011 at 12:37 PM, Jamie Iles jamie@jamieiles.com wrote:
On Mon, Sep 26, 2011 at 02:10:32PM -0500, Rob Herring wrote:
On 09/26/2011 01:40 PM, Jamie Iles wrote:
On Mon, Sep 26, 2011 at 01:33:08PM -0500, Rob Herring wrote:
+static void clk_gate_set_bit(struct clk_hw *clk) +{
- struct clk_gate *gate = to_clk_gate(clk);
- u32 reg;
- reg = __raw_readl(gate->reg);
- reg |= BIT(gate->bit_idx);
- __raw_writel(reg, gate->reg);
Don't these read-mod-writes need a spinlock around it?
It's possible to have an enable bits and dividers in the same register. If you did a set_rate and while doing an enable/disable, there would be a problem. Also, it may be 2 different clocks in the same register, so the spinlock needs to be shared and not per clock.
Well the prepare lock will be held here and I believe that would be sufficient.
No, the enable spinlock is protecting enable/disable. But set_rate is protected by the prepare mutex. So you clearly don't need locking if you have a register of only 1 bit enables. If you have a register accessed by both enable/disable and prepare/unprepare/set_rate, then you need some protection.
OK fair point, but I would guess that if you had a clock like this then you probably wouldn't use this simple gated clock would you? (speaking from my world where we have quite simple clocks ;-))
I think it is a safe assumption that if a register controls both enable/disable and some programmable divider then,
- those controls are probably for the same clock
- that clock won't be using the cookie-cutter gated-clock
implementation anyways
By definition of simple gated clock, the other bits have to be for another clock. The restriction is that all the other bits can only be clock gate bits.
Rob, do you feel these assumptions are OK and locking can remain the same in this patch?
Perhaps it is rare enough that it is not worth it use generic code in this case. If so, the documentation should be clear about this constraint. It is not something anyone will have hit before because everyone used a single global lock. Now with the api being split between 2 locks, this adds a new complexity.
I think the simple gated clock code should be usable for any clock controlled by a single bit in a 32-bit register independent of other things in that register.
One example is MX1 in (mach-imx/clock-imx1.c). The CSCR register has single bit enable for clk16m while hclk and clk48m have dividers in that register.
Rob
On 09/26/2011 04:30 PM, Rob Herring wrote:
On 09/26/2011 05:37 PM, Turquette, Mike wrote:
On Mon, Sep 26, 2011 at 12:37 PM, Jamie Ilesjamie@jamieiles.com wrote:
On Mon, Sep 26, 2011 at 02:10:32PM -0500, Rob Herring wrote:
On 09/26/2011 01:40 PM, Jamie Iles wrote:
On Mon, Sep 26, 2011 at 01:33:08PM -0500, Rob Herring wrote:
> +static void clk_gate_set_bit(struct clk_hw *clk) > +{ > + struct clk_gate *gate = to_clk_gate(clk); > + u32 reg; > + > + reg = __raw_readl(gate->reg); > + reg |= BIT(gate->bit_idx); > + __raw_writel(reg, gate->reg);
Don't these read-mod-writes need a spinlock around it?
It's possible to have an enable bits and dividers in the same register. If you did a set_rate and while doing an enable/disable, there would be a problem. Also, it may be 2 different clocks in the same register, so the spinlock needs to be shared and not per clock.
Well the prepare lock will be held here and I believe that would be sufficient.
No, the enable spinlock is protecting enable/disable. But set_rate is protected by the prepare mutex. So you clearly don't need locking if you have a register of only 1 bit enables. If you have a register accessed by both enable/disable and prepare/unprepare/set_rate, then you need some protection.
OK fair point, but I would guess that if you had a clock like this then you probably wouldn't use this simple gated clock would you? (speaking from my world where we have quite simple clocks ;-))
I think it is a safe assumption that if a register controls both enable/disable and some programmable divider then,
- those controls are probably for the same clock
- that clock won't be using the cookie-cutter gated-clock
implementation anyways
By definition of simple gated clock, the other bits have to be for another clock. The restriction is that all the other bits can only be clock gate bits.
Rob, do you feel these assumptions are OK and locking can remain the same in this patch?
Perhaps it is rare enough that it is not worth it use generic code in this case. If so, the documentation should be clear about this constraint. It is not something anyone will have hit before because everyone used a single global lock. Now with the api being split between 2 locks, this adds a new complexity.
I kinda agree with Rob on this. There are very few, if any, such simple clocks on MSM chips. It's very easy to a SoC clock developer to accidentally use these simple clocks without realizing the point that Rob brings up.
I think the simple gated clock code should be usable for any clock controlled by a single bit in a 32-bit register independent of other things in that register.
To take care of the scenario Rob bring up, the prepare/unprepare and enable/disable code will have to grab a per-tree register-lock before accessing any registers. The prepare/unprepare code should obviously be written to hold this register-lock for as small of a duration as possible. For example, if the prepare code is doing voltage increase, the register-lock should be grabber _after_ the voltage is increased. At least, this is approximately how the MSM clock code can be mapped onto this generic framework.
I think we should just go ahead and implement the per-tree register lock so that the generic clock implementations are more useful. The lock will really be held only for a very short time and hence shouldn't matter that there is a single lock for all the clocks in a tree.
Thomas,
Did you get a chance to send out your patches with support for per-tree locking? I would really like to see that as part of the first patch series that gets pulled in.
Thanks, Saravana
On Thu, Sep 22, 2011 at 03:26:59PM -0700, Mike Turquette wrote:
From: Jeremy Kerr jeremy.kerr@canonical.com
Signed-off-by: Jeremy Kerr jeremy.kerr@canonical.com Signed-off-by: Mark Brown broonie@opensource.wolfsonmicro.com Signed-off-by: Jamie Iles jamie@jamieiles.com Signed-off-by: Mike Turquette mturquette@ti.com
Changes since v1: Add copyright header Fold in Jamie's patch for set-to-disable clks Use BIT macro instead of shift
drivers/clk/Kconfig | 4 ++ drivers/clk/Makefile | 1 + drivers/clk/clk-gate.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++ include/linux/clk.h | 13 ++++++++ 4 files changed, 96 insertions(+), 0 deletions(-) create mode 100644 drivers/clk/clk-gate.c
I feel hard to tell the tree the clk parent, at register/init time. For the simple gate clk, the only way is to set .get_parent. But normally, for clk without any divider we set .get_parent to NULL. Maybe we can put .parent to struct clk_hw?
Thanks Richard
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig index d8313d7..a78967c 100644 --- a/drivers/clk/Kconfig +++ b/drivers/clk/Kconfig @@ -12,3 +12,7 @@ config GENERIC_CLK config GENERIC_CLK_FIXED bool depends on GENERIC_CLK
+config GENERIC_CLK_GATE
- bool
- depends on GENERIC_CLK
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile index 9a3325a..d186446 100644 --- a/drivers/clk/Makefile +++ b/drivers/clk/Makefile @@ -2,3 +2,4 @@ obj-$(CONFIG_CLKDEV_LOOKUP) += clkdev.o obj-$(CONFIG_GENERIC_CLK) += clk.o obj-$(CONFIG_GENERIC_CLK_FIXED) += clk-fixed.o +obj-$(CONFIG_GENERIC_CLK_GATE) += clk-gate.o diff --git a/drivers/clk/clk-gate.c b/drivers/clk/clk-gate.c new file mode 100644 index 0000000..a1d8e79 --- /dev/null +++ b/drivers/clk/clk-gate.c @@ -0,0 +1,78 @@ +/*
- 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.
- Simple clk gate implementation
- */
+#include <linux/clk.h> +#include <linux/module.h> +#include <asm/io.h>
+#define to_clk_gate(clk) container_of(clk, struct clk_gate, hw)
+static unsigned long clk_gate_get_rate(struct clk_hw *clk) +{
- return clk_get_rate(clk_get_parent(clk->clk));
+}
+static void clk_gate_set_bit(struct clk_hw *clk) +{
- struct clk_gate *gate = to_clk_gate(clk);
- u32 reg;
- reg = __raw_readl(gate->reg);
- reg |= BIT(gate->bit_idx);
- __raw_writel(reg, gate->reg);
+}
+static void clk_gate_clear_bit(struct clk_hw *clk) +{
- struct clk_gate *gate = to_clk_gate(clk);
- u32 reg;
- reg = __raw_readl(gate->reg);
- reg &= ~BIT(gate->bit_idx);
- __raw_writel(reg, gate->reg);
+}
+static int clk_gate_enable_set(struct clk_hw *clk) +{
- clk_gate_set_bit(clk);
- return 0;
+}
+static void clk_gate_disable_clear(struct clk_hw *clk) +{
- clk_gate_clear_bit(clk);
+}
+struct clk_hw_ops clk_gate_set_enable_ops = {
- .recalc_rate = clk_gate_get_rate,
- .enable = clk_gate_enable_set,
- .disable = clk_gate_disable_clear,
+}; +EXPORT_SYMBOL_GPL(clk_gate_set_enable_ops);
+static int clk_gate_enable_clear(struct clk_hw *clk) +{
- clk_gate_clear_bit(clk);
- return 0;
+}
+static void clk_gate_disable_set(struct clk_hw *clk) +{
- clk_gate_set_bit(clk);
+}
+struct clk_hw_ops clk_gate_set_disable_ops = {
- .recalc_rate = clk_gate_get_rate,
- .enable = clk_gate_enable_clear,
- .disable = clk_gate_disable_set,
+}; +EXPORT_SYMBOL_GPL(clk_gate_set_disable_ops); diff --git a/include/linux/clk.h b/include/linux/clk.h index 1903dd8..626fd0d 100644 --- a/include/linux/clk.h +++ b/include/linux/clk.h @@ -124,6 +124,19 @@ extern struct clk_hw_ops clk_fixed_ops; #endif /* CONFIG_GENERIC_CLK_FIXED */ +#ifdef CONFIG_GENERIC_CLK_GATE
+struct clk_gate {
- struct clk_hw hw;
- void __iomem *reg;
- u8 bit_idx;
+};
+extern struct clk_hw_ops clk_gate_set_enable_ops; +extern struct clk_hw_ops clk_gate_set_disable_ops;
+#endif /* CONFIG_GENERIC_CLK_GATE */
/**
- clk_register - register and initialize a new clock
-- 1.7.4.1
linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Tue, Oct 11, 2011 at 11:46 PM, Richard Zhao richard.zhao@freescale.com wrote:
On Thu, Sep 22, 2011 at 03:26:59PM -0700, Mike Turquette wrote:
From: Jeremy Kerr jeremy.kerr@canonical.com
Signed-off-by: Jeremy Kerr jeremy.kerr@canonical.com Signed-off-by: Mark Brown broonie@opensource.wolfsonmicro.com Signed-off-by: Jamie Iles jamie@jamieiles.com Signed-off-by: Mike Turquette mturquette@ti.com
Changes since v1: Add copyright header Fold in Jamie's patch for set-to-disable clks Use BIT macro instead of shift
drivers/clk/Kconfig | 4 ++ drivers/clk/Makefile | 1 + drivers/clk/clk-gate.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++ include/linux/clk.h | 13 ++++++++ 4 files changed, 96 insertions(+), 0 deletions(-) create mode 100644 drivers/clk/clk-gate.c
I feel hard to tell the tree the clk parent, at register/init time. For the simple gate clk, the only way is to set .get_parent. But normally, for clk without any divider we set .get_parent to NULL. Maybe we can put .parent to struct clk_hw?
For non-mux clocks, whose parent is *always* going to be the same, you should create a duplicate .parent in the clk_hw_* structure and then have .get_parent return clk_hw_*->parent.
This is analogous to the way clk_hw_fixed returns clk_hw_fixed->rate when .recalc is called on it.
Regards, Mike
Thanks Richard
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig index d8313d7..a78967c 100644 --- a/drivers/clk/Kconfig +++ b/drivers/clk/Kconfig @@ -12,3 +12,7 @@ config GENERIC_CLK config GENERIC_CLK_FIXED bool depends on GENERIC_CLK
+config GENERIC_CLK_GATE
- bool
- depends on GENERIC_CLK
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile index 9a3325a..d186446 100644 --- a/drivers/clk/Makefile +++ b/drivers/clk/Makefile @@ -2,3 +2,4 @@ obj-$(CONFIG_CLKDEV_LOOKUP) += clkdev.o obj-$(CONFIG_GENERIC_CLK) += clk.o obj-$(CONFIG_GENERIC_CLK_FIXED) += clk-fixed.o +obj-$(CONFIG_GENERIC_CLK_GATE) += clk-gate.o diff --git a/drivers/clk/clk-gate.c b/drivers/clk/clk-gate.c new file mode 100644 index 0000000..a1d8e79 --- /dev/null +++ b/drivers/clk/clk-gate.c @@ -0,0 +1,78 @@ +/*
- 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.
- Simple clk gate implementation
- */
+#include <linux/clk.h> +#include <linux/module.h> +#include <asm/io.h>
+#define to_clk_gate(clk) container_of(clk, struct clk_gate, hw)
+static unsigned long clk_gate_get_rate(struct clk_hw *clk) +{
- return clk_get_rate(clk_get_parent(clk->clk));
+}
+static void clk_gate_set_bit(struct clk_hw *clk) +{
- struct clk_gate *gate = to_clk_gate(clk);
- u32 reg;
- reg = __raw_readl(gate->reg);
- reg |= BIT(gate->bit_idx);
- __raw_writel(reg, gate->reg);
+}
+static void clk_gate_clear_bit(struct clk_hw *clk) +{
- struct clk_gate *gate = to_clk_gate(clk);
- u32 reg;
- reg = __raw_readl(gate->reg);
- reg &= ~BIT(gate->bit_idx);
- __raw_writel(reg, gate->reg);
+}
+static int clk_gate_enable_set(struct clk_hw *clk) +{
- clk_gate_set_bit(clk);
- return 0;
+}
+static void clk_gate_disable_clear(struct clk_hw *clk) +{
- clk_gate_clear_bit(clk);
+}
+struct clk_hw_ops clk_gate_set_enable_ops = {
- .recalc_rate = clk_gate_get_rate,
- .enable = clk_gate_enable_set,
- .disable = clk_gate_disable_clear,
+}; +EXPORT_SYMBOL_GPL(clk_gate_set_enable_ops);
+static int clk_gate_enable_clear(struct clk_hw *clk) +{
- clk_gate_clear_bit(clk);
- return 0;
+}
+static void clk_gate_disable_set(struct clk_hw *clk) +{
- clk_gate_set_bit(clk);
+}
+struct clk_hw_ops clk_gate_set_disable_ops = {
- .recalc_rate = clk_gate_get_rate,
- .enable = clk_gate_enable_clear,
- .disable = clk_gate_disable_set,
+}; +EXPORT_SYMBOL_GPL(clk_gate_set_disable_ops); diff --git a/include/linux/clk.h b/include/linux/clk.h index 1903dd8..626fd0d 100644 --- a/include/linux/clk.h +++ b/include/linux/clk.h @@ -124,6 +124,19 @@ extern struct clk_hw_ops clk_fixed_ops;
#endif /* CONFIG_GENERIC_CLK_FIXED */
+#ifdef CONFIG_GENERIC_CLK_GATE
+struct clk_gate {
- struct clk_hw hw;
- void __iomem *reg;
- u8 bit_idx;
+};
+extern struct clk_hw_ops clk_gate_set_enable_ops; +extern struct clk_hw_ops clk_gate_set_disable_ops;
+#endif /* CONFIG_GENERIC_CLK_GATE */
/** * clk_register - register and initialize a new clock * -- 1.7.4.1
linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Wed, Oct 12, 2011 at 07:59:19AM -0700, Turquette, Mike wrote:
On Tue, Oct 11, 2011 at 11:46 PM, Richard Zhao richard.zhao@freescale.com wrote:
On Thu, Sep 22, 2011 at 03:26:59PM -0700, Mike Turquette wrote:
From: Jeremy Kerr jeremy.kerr@canonical.com
Signed-off-by: Jeremy Kerr jeremy.kerr@canonical.com Signed-off-by: Mark Brown broonie@opensource.wolfsonmicro.com Signed-off-by: Jamie Iles jamie@jamieiles.com Signed-off-by: Mike Turquette mturquette@ti.com
Changes since v1: Add copyright header Fold in Jamie's patch for set-to-disable clks Use BIT macro instead of shift
drivers/clk/Kconfig | 4 ++ drivers/clk/Makefile | 1 + drivers/clk/clk-gate.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++ include/linux/clk.h | 13 ++++++++ 4 files changed, 96 insertions(+), 0 deletions(-) create mode 100644 drivers/clk/clk-gate.c
I feel hard to tell the tree the clk parent, at register/init time. For the simple gate clk, the only way is to set .get_parent. But normally, for clk without any divider we set .get_parent to NULL. Maybe we can put .parent to struct clk_hw?
For non-mux clocks, whose parent is *always* going to be the same, you should create a duplicate .parent in the clk_hw_* structure and then have .get_parent return clk_hw_*->parent.
Maybe I do not understand what you mean here, but I think there is something missing in the gate.
This is analogous to the way clk_hw_fixed returns clk_hw_fixed->rate when .recalc is called on it.
+static unsigned long clk_gate_get_rate(struct clk_hw *clk) +{
- return clk_get_rate(clk_get_parent(clk->clk));
+}
clk_get_parent goes down to clk_gate_set_enable_ops.get_parent below...
+struct clk_hw_ops clk_gate_set_enable_ops = {
- .recalc_rate = clk_gate_get_rate,
- .enable = clk_gate_enable_set,
- .disable = clk_gate_disable_clear,
+};
...but this does not have a get_parent pointer, so clk_get_parent() for a gate always returns NULL which means that a gate never has a valid rate.
Sascha
On Sun, Oct 16, 2011 at 08:26:49PM +0200, Sascha Hauer wrote:
On Wed, Oct 12, 2011 at 07:59:19AM -0700, Turquette, Mike wrote:
On Tue, Oct 11, 2011 at 11:46 PM, Richard Zhao richard.zhao@freescale.com wrote:
On Thu, Sep 22, 2011 at 03:26:59PM -0700, Mike Turquette wrote:
From: Jeremy Kerr jeremy.kerr@canonical.com
Signed-off-by: Jeremy Kerr jeremy.kerr@canonical.com Signed-off-by: Mark Brown broonie@opensource.wolfsonmicro.com Signed-off-by: Jamie Iles jamie@jamieiles.com Signed-off-by: Mike Turquette mturquette@ti.com
Changes since v1: Add copyright header Fold in Jamie's patch for set-to-disable clks Use BIT macro instead of shift
drivers/clk/Kconfig | 4 ++ drivers/clk/Makefile | 1 + drivers/clk/clk-gate.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++ include/linux/clk.h | 13 ++++++++ 4 files changed, 96 insertions(+), 0 deletions(-) create mode 100644 drivers/clk/clk-gate.c
I feel hard to tell the tree the clk parent, at register/init time. For the simple gate clk, the only way is to set .get_parent. But normally, for clk without any divider we set .get_parent to NULL. Maybe we can put .parent to struct clk_hw?
For non-mux clocks, whose parent is *always* going to be the same, you should create a duplicate .parent in the clk_hw_* structure and then have .get_parent return clk_hw_*->parent.
Maybe I do not understand what you mean here, but I think there is something missing in the gate.
yes, clk_gate can not set fixed parent in v2 patch. Mike means add .parent int clk_gate and set .get_parent in the ops.
This is analogous to the way clk_hw_fixed returns clk_hw_fixed->rate when .recalc is called on it.
+static unsigned long clk_gate_get_rate(struct clk_hw *clk) +{
- return clk_get_rate(clk_get_parent(clk->clk));
+}
clk_get_parent goes down to clk_gate_set_enable_ops.get_parent below...
+struct clk_hw_ops clk_gate_set_enable_ops = {
- .recalc_rate = clk_gate_get_rate,
- .enable = clk_gate_enable_set,
- .disable = clk_gate_disable_clear,
+};
...but this does not have a get_parent pointer, so clk_get_parent() for a gate always returns NULL which means that a gate never has a valid rate.
Sascha
-- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Sun, Oct 16, 2011 at 11:42 PM, Richard Zhao richard.zhao@linaro.org wrote:
On Sun, Oct 16, 2011 at 08:26:49PM +0200, Sascha Hauer wrote:
On Wed, Oct 12, 2011 at 07:59:19AM -0700, Turquette, Mike wrote:
On Tue, Oct 11, 2011 at 11:46 PM, Richard Zhao richard.zhao@freescale.com wrote:
On Thu, Sep 22, 2011 at 03:26:59PM -0700, Mike Turquette wrote:
From: Jeremy Kerr jeremy.kerr@canonical.com
Signed-off-by: Jeremy Kerr jeremy.kerr@canonical.com Signed-off-by: Mark Brown broonie@opensource.wolfsonmicro.com Signed-off-by: Jamie Iles jamie@jamieiles.com Signed-off-by: Mike Turquette mturquette@ti.com
Changes since v1: Add copyright header Fold in Jamie's patch for set-to-disable clks Use BIT macro instead of shift
drivers/clk/Kconfig | 4 ++ drivers/clk/Makefile | 1 + drivers/clk/clk-gate.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++ include/linux/clk.h | 13 ++++++++ 4 files changed, 96 insertions(+), 0 deletions(-) create mode 100644 drivers/clk/clk-gate.c
I feel hard to tell the tree the clk parent, at register/init time. For the simple gate clk, the only way is to set .get_parent. But normally, for clk without any divider we set .get_parent to NULL. Maybe we can put .parent to struct clk_hw?
For non-mux clocks, whose parent is *always* going to be the same, you should create a duplicate .parent in the clk_hw_* structure and then have .get_parent return clk_hw_*->parent.
Maybe I do not understand what you mean here, but I think there is something missing in the gate.
yes, clk_gate can not set fixed parent in v2 patch. Mike means add .parent int clk_gate and set .get_parent in the ops.
Yes, it seems I was unclear. I mean to say that a .parent can be added to any clk hw implementation (clk_hw_imx or whatever) and the paired .ops (in this case, .get_parent) will know about it's existence.
Regards, Mike
This is analogous to the way clk_hw_fixed returns clk_hw_fixed->rate when .recalc is called on it.
+static unsigned long clk_gate_get_rate(struct clk_hw *clk) +{
- return clk_get_rate(clk_get_parent(clk->clk));
+}
clk_get_parent goes down to clk_gate_set_enable_ops.get_parent below...
+struct clk_hw_ops clk_gate_set_enable_ops = {
- .recalc_rate = clk_gate_get_rate,
- .enable = clk_gate_enable_set,
- .disable = clk_gate_disable_clear,
+};
...but this does not have a get_parent pointer, so clk_get_parent() for a gate always returns NULL which means that a gate never has a valid rate.
Sascha
-- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Thu, Sep 22, 2011 at 03:26:59PM -0700, Mike Turquette wrote:
diff --git a/drivers/clk/clk-gate.c b/drivers/clk/clk-gate.c new file mode 100644 index 0000000..a1d8e79 --- /dev/null +++ b/drivers/clk/clk-gate.c @@ -0,0 +1,78 @@ +/*
- 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.
- Simple clk gate implementation
- */
+#include <linux/clk.h> +#include <linux/module.h> +#include <asm/io.h>
linux/io.h please.
On Thu, Oct 13, 2011 at 7:45 AM, Russell King - ARM Linux linux@arm.linux.org.uk wrote:
On Thu, Sep 22, 2011 at 03:26:59PM -0700, Mike Turquette wrote:
diff --git a/drivers/clk/clk-gate.c b/drivers/clk/clk-gate.c new file mode 100644 index 0000000..a1d8e79 --- /dev/null +++ b/drivers/clk/clk-gate.c @@ -0,0 +1,78 @@ +/*
- 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.
- Simple clk gate implementation
- */
+#include <linux/clk.h> +#include <linux/module.h> +#include <asm/io.h>
linux/io.h please.
Will roll into v3.
Thanks for reviewing, Mike
From: Mark Brown broonie@opensource.wolfsonmicro.com
Currently drivers for the generic clk subsystem must be selected by platforms using them in order to enable build. When doing development on the API or generic build time testing it is useful to be able to build unused drivers in order to improve coverage so supply a Kconfig option which allows this.
Signed-off-by: Mark Brown broonie@opensource.wolfsonmicro.com --- No change since v1
drivers/clk/Kconfig | 11 ++++++++++- 1 files changed, 10 insertions(+), 1 deletions(-)
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig index a78967c..95b42a3 100644 --- a/drivers/clk/Kconfig +++ b/drivers/clk/Kconfig @@ -1,4 +1,3 @@ - config CLKDEV_LOOKUP bool select HAVE_CLK @@ -9,6 +8,16 @@ config HAVE_MACH_CLKDEV config GENERIC_CLK bool
+config GENERIC_CLK_BUILD_TEST + bool "Build all generic clock drivers" + depends on EXPERIMENTAL && GENERIC_CLK + select GENERIC_CLK_FIXED + select GENERIC_CLK_GATE + help + Enable all possible generic clock drivers. This is only + useful for improving build coverage, it is not useful for + production kernel builds. + config GENERIC_CLK_FIXED bool depends on GENERIC_CLK
From: Mark Brown broonie@opensource.wolfsonmicro.com
The WM831x and WM832x series of PMICs contain a flexible clocking subsystem intended to provide always on and system core clocks. It features:
- A 32.768kHz crystal oscillator which can optionally be used to pass through an externally generated clock. - A FLL which can be clocked from either the 32.768kHz oscillator or the CLKIN pin. - A CLKOUT pin which can bring out either the oscillator or the FLL output. - The 32.768kHz clock can also optionally be brought out on the GPIO pins of the device.
This driver fully supports the 32.768kHz oscillator and CLKOUT. The FLL is supported only in AUTO mode, the full flexibility of the FLL cannot currently be used. The use of clock references other than the internal oscillator is not currently supported, and since clk_set_parent() is not implemented in the generic clock API the clock tree configuration cannot be changed at runtime.
Due to a lack of access to systems where the core SoC has been converted to use the generic clock API this driver has been compile tested only.
Signed-off-by: Mark Brown broonie@opensource.wolfsonmicro.com Signed-off-by: Mike Turquette mturquette@ti.com --- Changes since v1: Changed CLK_SET_RATE_PROPAGATE to CLK_PARENT_RATE_CHANGE Converted to clk_register to the original style, without passing in struct device *dev. This is due to Mark's clock name collision patch being dropped.
MAINTAINERS | 1 + drivers/clk/Kconfig | 5 + drivers/clk/Makefile | 1 + drivers/clk/clk-wm831x.c | 386 ++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 393 insertions(+), 0 deletions(-) create mode 100644 drivers/clk/clk-wm831x.c
diff --git a/MAINTAINERS b/MAINTAINERS index ae8820e..a100ea0 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -7208,6 +7208,7 @@ T: git git://opensource.wolfsonmicro.com/linux-2.6-audioplus W: http://opensource.wolfsonmicro.com/content/linux-drivers-wolfson-devices S: Supported F: Documentation/hwmon/wm83?? +F: drivers/clk/clk-wm83*.c F: drivers/leds/leds-wm83*.c F: drivers/input/misc/wm831x-on.c F: drivers/input/touchscreen/wm831x-ts.c diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig index 95b42a3..8aca5ab 100644 --- a/drivers/clk/Kconfig +++ b/drivers/clk/Kconfig @@ -13,6 +13,7 @@ config GENERIC_CLK_BUILD_TEST depends on EXPERIMENTAL && GENERIC_CLK select GENERIC_CLK_FIXED select GENERIC_CLK_GATE + select GENERIC_CLK_WM831X if MFD_WM831X=y help Enable all possible generic clock drivers. This is only useful for improving build coverage, it is not useful for @@ -25,3 +26,7 @@ config GENERIC_CLK_FIXED config GENERIC_CLK_GATE bool depends on GENERIC_CLK + +config GENERIC_CLK_WM831X + tristate + depends on GENERIC_CLK && MFD_WM831X diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile index d186446..6628ad5 100644 --- a/drivers/clk/Makefile +++ b/drivers/clk/Makefile @@ -3,3 +3,4 @@ obj-$(CONFIG_CLKDEV_LOOKUP) += clkdev.o obj-$(CONFIG_GENERIC_CLK) += clk.o obj-$(CONFIG_GENERIC_CLK_FIXED) += clk-fixed.o obj-$(CONFIG_GENERIC_CLK_GATE) += clk-gate.o +obj-$(CONFIG_GENERIC_CLK_WM831X) += clk-wm831x.o diff --git a/drivers/clk/clk-wm831x.c b/drivers/clk/clk-wm831x.c new file mode 100644 index 0000000..bfcbcb5 --- /dev/null +++ b/drivers/clk/clk-wm831x.c @@ -0,0 +1,386 @@ +/* + * WM831x clock control + * + * Copyright 2011 Wolfson Microelectronics PLC. + * + * Author: Mark Brown broonie@opensource.wolfsonmicro.com + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the + * Free Software Foundation; either version 2 of the License, or (at your + * option) any later version. + * + */ + +#include <linux/clk.h> +#include <linux/module.h> +#include <linux/slab.h> +#include <linux/platform_device.h> +#include <linux/mfd/wm831x/core.h> + +struct wm831x_clk { + struct wm831x *wm831x; + struct clk_hw xtal_hw; + struct clk_hw fll_hw; + struct clk_hw clkout_hw; + bool xtal_ena; +}; + +static int wm831x_xtal_enable(struct clk_hw *hw) +{ + struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk, + xtal_hw); + + if (clkdata->xtal_ena) + return 0; + else + return -EPERM; +} + +static unsigned long wm831x_xtal_recalc_rate(struct clk_hw *hw) +{ + struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk, + xtal_hw); + + if (clkdata->xtal_ena) + return 32768; + else + return 0; +} + +static long wm831x_xtal_round_rate(struct clk_hw *hw, unsigned long rate) +{ + return wm831x_xtal_recalc_rate(hw); +} + +static const struct clk_hw_ops wm831x_xtal_ops = { + .enable = wm831x_xtal_enable, + .recalc_rate = wm831x_xtal_recalc_rate, + .round_rate = wm831x_xtal_round_rate, +}; + +static const unsigned long wm831x_fll_auto_rates[] = { + 2048000, + 11289600, + 12000000, + 12288000, + 19200000, + 22579600, + 24000000, + 24576000, +}; + +static bool wm831x_fll_enabled(struct wm831x *wm831x) +{ + int ret; + + ret = wm831x_reg_read(wm831x, WM831X_FLL_CONTROL_1); + if (ret < 0) { + dev_err(wm831x->dev, "Unable to read FLL_CONTROL_1: %d\n", + ret); + return true; + } + + return ret & WM831X_FLL_ENA; +} + +static int wm831x_fll_prepare(struct clk_hw *hw) +{ + struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk, + fll_hw); + struct wm831x *wm831x = clkdata->wm831x; + int ret; + + ret = wm831x_set_bits(wm831x, WM831X_FLL_CONTROL_2, + WM831X_FLL_ENA, WM831X_FLL_ENA); + if (ret != 0) + dev_crit(wm831x->dev, "Failed to enable FLL: %d\n", ret); + + return ret; +} + +static void wm831x_fll_unprepare(struct clk_hw *hw) +{ + struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk, + fll_hw); + struct wm831x *wm831x = clkdata->wm831x; + int ret; + + ret = wm831x_set_bits(wm831x, WM831X_FLL_CONTROL_2, WM831X_FLL_ENA, 0); + if (ret != 0) + dev_crit(wm831x->dev, "Failed to disaable FLL: %d\n", ret); +} + +static unsigned long wm831x_fll_recalc_rate(struct clk_hw *hw) +{ + struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk, + fll_hw); + struct wm831x *wm831x = clkdata->wm831x; + int ret; + + ret = wm831x_reg_read(wm831x, WM831X_CLOCK_CONTROL_2); + if (ret < 0) { + dev_err(wm831x->dev, "Unable to read CLOCK_CONTROL_2: %d\n", + ret); + return 0; + } + + if (ret & WM831X_FLL_AUTO) + return wm831x_fll_auto_rates[ret & WM831X_FLL_AUTO_FREQ_MASK]; + + dev_err(wm831x->dev, "FLL only supported in AUTO mode\n"); + return 0; +} + +static int wm831x_fll_set_rate(struct clk_hw *hw, unsigned long rate, + unsigned long *parent_rate) +{ + struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk, + fll_hw); + struct wm831x *wm831x = clkdata->wm831x; + int i; + + for (i = 0; i < ARRAY_SIZE(wm831x_fll_auto_rates); i++) + if (wm831x_fll_auto_rates[i] == rate) + break; + if (i == ARRAY_SIZE(wm831x_fll_auto_rates)) + return -EINVAL; + + if (wm831x_fll_enabled(wm831x)) + return -EPERM; + + return wm831x_set_bits(wm831x, WM831X_CLOCK_CONTROL_2, + WM831X_FLL_AUTO_FREQ_MASK, i); +} + +static struct clk *wm831x_fll_get_parent(struct clk_hw *hw) +{ + struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk, + fll_hw); + struct wm831x *wm831x = clkdata->wm831x; + int ret; + + /* AUTO mode is always clocked from the crystal */ + ret = wm831x_reg_read(wm831x, WM831X_CLOCK_CONTROL_2); + if (ret < 0) { + dev_err(wm831x->dev, "Unable to read CLOCK_CONTROL_2: %d\n", + ret); + return NULL; + } + + if (ret & WM831X_FLL_AUTO) + return clkdata->xtal_hw.clk; + + ret = wm831x_reg_read(wm831x, WM831X_FLL_CONTROL_5); + if (ret < 0) { + dev_err(wm831x->dev, "Unable to read FLL_CONTROL_5: %d\n", + ret); + return NULL; + } + + switch (ret & WM831X_FLL_CLK_SRC_MASK) { + case 0: + return clkdata->xtal_hw.clk; + case 1: + dev_warn(wm831x->dev, + "FLL clocked from CLKIN not yet supported\n"); + return NULL; + default: + dev_err(wm831x->dev, "Unsupported FLL clock source %d\n", + ret & WM831X_FLL_CLK_SRC_MASK); + return NULL; + } +} + +static const struct clk_hw_ops wm831x_fll_ops = { + .prepare = wm831x_fll_prepare, + .unprepare = wm831x_fll_unprepare, + .recalc_rate = wm831x_fll_recalc_rate, + .set_rate = wm831x_fll_set_rate, + .get_parent = wm831x_fll_get_parent, +}; + +static int wm831x_clkout_prepare(struct clk_hw *hw) +{ + struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk, + clkout_hw); + struct wm831x *wm831x = clkdata->wm831x; + int ret; + + ret = wm831x_reg_unlock(wm831x); + if (ret != 0) { + dev_crit(wm831x->dev, "Failed to lock registers: %d\n", ret); + return ret; + } + + ret = wm831x_set_bits(wm831x, WM831X_CLOCK_CONTROL_1, + WM831X_CLKOUT_ENA, WM831X_CLKOUT_ENA); + if (ret != 0) + dev_crit(wm831x->dev, "Failed to enable CLKOUT: %d\n", ret); + + wm831x_reg_lock(wm831x); + + return ret; +} + +static void wm831x_clkout_unprepare(struct clk_hw *hw) +{ + struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk, + clkout_hw); + struct wm831x *wm831x = clkdata->wm831x; + int ret; + + ret = wm831x_reg_unlock(wm831x); + if (ret != 0) { + dev_crit(wm831x->dev, "Failed to lock registers: %d\n", ret); + return; + } + + ret = wm831x_set_bits(wm831x, WM831X_CLOCK_CONTROL_1, + WM831X_CLKOUT_ENA, 0); + if (ret != 0) + dev_crit(wm831x->dev, "Failed to disable CLKOUT: %d\n", ret); + + wm831x_reg_lock(wm831x); +} + +static unsigned long wm831x_clkout_recalc_rate(struct clk_hw *hw) +{ + return clk_get_rate(clk_get_parent(hw->clk)); +} + +static long wm831x_clkout_round_rate(struct clk_hw *hw, unsigned long rate) +{ + return clk_round_rate(clk_get_parent(hw->clk), rate); +} + +static int wm831x_clkout_set_rate(struct clk_hw *hw, unsigned long rate, + unsigned long *parent_rate) +{ + *parent_rate = rate; + return CLK_PARENT_RATE_CHANGE; +} + +static struct clk *wm831x_clkout_get_parent(struct clk_hw *hw) +{ + struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk, + clkout_hw); + struct wm831x *wm831x = clkdata->wm831x; + int ret; + + ret = wm831x_reg_read(wm831x, WM831X_CLOCK_CONTROL_1); + if (ret < 0) { + dev_err(wm831x->dev, "Unable to read CLOCK_CONTROL_1: %d\n", + ret); + return NULL; + } + + if (ret & WM831X_CLKOUT_SRC) + return clkdata->xtal_hw.clk; + else + return clkdata->fll_hw.clk; +} + +static const struct clk_hw_ops wm831x_clkout_ops = { + .prepare = wm831x_clkout_prepare, + .unprepare = wm831x_clkout_unprepare, + .recalc_rate = wm831x_clkout_recalc_rate, + .round_rate = wm831x_clkout_round_rate, + .set_rate = wm831x_clkout_set_rate, + .get_parent = wm831x_clkout_get_parent, +}; + +static __devinit int wm831x_clk_probe(struct platform_device *pdev) +{ + struct wm831x *wm831x = dev_get_drvdata(pdev->dev.parent); + struct wm831x_clk *clkdata; + int ret; + + clkdata = kzalloc(sizeof(*clkdata), GFP_KERNEL); + if (!clkdata) + return -ENOMEM; + + /* XTAL_ENA can only be set via OTP/InstantConfig so just read once */ + ret = wm831x_reg_read(wm831x, WM831X_CLOCK_CONTROL_2); + if (ret < 0) { + dev_err(wm831x->dev, "Unable to read CLOCK_CONTROL_2: %d\n", + ret); + goto err_alloc; + } + clkdata->xtal_ena = ret & WM831X_XTAL_ENA; + + if (!clk_register(&wm831x_xtal_ops, &clkdata->xtal_hw, + "xtal")) { + ret = -EINVAL; + goto err_alloc; + } + + if (!clk_register(&wm831x_fll_ops, &clkdata->fll_hw, + "fll")) { + ret = -EINVAL; + goto err_xtal; + } + + if (!clk_register(&wm831x_clkout_ops, &clkdata->clkout_hw, + "clkout")) { + ret = -EINVAL; + goto err_fll; + } + + dev_set_drvdata(&pdev->dev, clkdata); + + return 0; + +err_fll: + clk_unregister(clkdata->fll_hw.clk); +err_xtal: + clk_unregister(clkdata->xtal_hw.clk); +err_alloc: + kfree(clkdata); + return ret; +} + +static __devexit int wm831x_clk_remove(struct platform_device *pdev) +{ + struct wm831x_clk *clkdata = dev_get_drvdata(&pdev->dev); + + clk_unregister(clkdata->clkout_hw.clk); + clk_unregister(clkdata->fll_hw.clk); + clk_unregister(clkdata->xtal_hw.clk); + kfree(clkdata); + + return 0; +} + +static struct platform_driver wm831x_clk_driver = { + .probe = wm831x_clk_probe, + .remove = __devexit_p(wm831x_clk_remove), + .driver = { + .name = "wm831x-clk", + .owner = THIS_MODULE, + }, +}; + +static int __init wm831x_clk_init(void) +{ + int ret; + + ret = platform_driver_register(&wm831x_clk_driver); + if (ret != 0) + pr_err("Failed to register WM831x clock driver: %d\n", ret); + + return ret; +} +module_init(wm831x_clk_init); + +static void __exit wm831x_clk_exit(void) +{ + platform_driver_unregister(&wm831x_clk_driver); +} +module_exit(wm831x_clk_exit); + +/* Module information */ +MODULE_AUTHOR("Mark Brown broonie@opensource.wolfsonmicro.com"); +MODULE_DESCRIPTION("WM831x clock driver"); +MODULE_LICENSE("GPL"); +MODULE_ALIAS("platform:wm831x-clk");
On Thu, Sep 22, 2011 at 03:27:01PM -0700, Mike Turquette wrote:
From: Mark Brown broonie@opensource.wolfsonmicro.com
The WM831x and WM832x series of PMICs contain a flexible clocking subsystem intended to provide always on and system core clocks. It features:
- A 32.768kHz crystal oscillator which can optionally be used to pass through an externally generated clock.
- A FLL which can be clocked from either the 32.768kHz oscillator or the CLKIN pin.
- A CLKOUT pin which can bring out either the oscillator or the FLL output.
- The 32.768kHz clock can also optionally be brought out on the GPIO pins of the device.
This driver fully supports the 32.768kHz oscillator and CLKOUT. The FLL is supported only in AUTO mode, the full flexibility of the FLL cannot currently be used. The use of clock references other than the internal oscillator is not currently supported, and since clk_set_parent() is not implemented in the generic clock API the clock tree configuration cannot be changed at runtime.
Due to a lack of access to systems where the core SoC has been converted to use the generic clock API this driver has been compile tested only.
Signed-off-by: Mark Brown broonie@opensource.wolfsonmicro.com Signed-off-by: Mike Turquette mturquette@ti.com
A few minor comments below. Otherwise looks fine to me.
+static __devinit int wm831x_clk_probe(struct platform_device *pdev) +{
- struct wm831x *wm831x = dev_get_drvdata(pdev->dev.parent);
- struct wm831x_clk *clkdata;
- int ret;
- clkdata = kzalloc(sizeof(*clkdata), GFP_KERNEL);
If devm_kzalloc() is used, then all the kfree unwinding can be dropped.
+static int __init wm831x_clk_init(void) +{
- int ret;
- ret = platform_driver_register(&wm831x_clk_driver);
- if (ret != 0)
pr_err("Failed to register WM831x clock driver: %d\n", ret);- return ret;
No need for this song-and-dance. The driver core is pretty well debugged. Just use "return platform_driver_register(...);"
g.
On Sat, Sep 24, 2011 at 9:08 PM, Grant Likely grant.likely@secretlab.ca wrote:
On Thu, Sep 22, 2011 at 03:27:01PM -0700, Mike Turquette wrote:
From: Mark Brown broonie@opensource.wolfsonmicro.com
The WM831x and WM832x series of PMICs contain a flexible clocking subsystem intended to provide always on and system core clocks. It features:
- A 32.768kHz crystal oscillator which can optionally be used to pass
through an externally generated clock.
- A FLL which can be clocked from either the 32.768kHz oscillator or
the CLKIN pin.
- A CLKOUT pin which can bring out either the oscillator or the FLL
output.
- The 32.768kHz clock can also optionally be brought out on the GPIO
pins of the device.
This driver fully supports the 32.768kHz oscillator and CLKOUT. The FLL is supported only in AUTO mode, the full flexibility of the FLL cannot currently be used. The use of clock references other than the internal oscillator is not currently supported, and since clk_set_parent() is not implemented in the generic clock API the clock tree configuration cannot be changed at runtime.
Due to a lack of access to systems where the core SoC has been converted to use the generic clock API this driver has been compile tested only.
Signed-off-by: Mark Brown broonie@opensource.wolfsonmicro.com Signed-off-by: Mike Turquette mturquette@ti.com
A few minor comments below. Otherwise looks fine to me.
+static __devinit int wm831x_clk_probe(struct platform_device *pdev) +{
- struct wm831x *wm831x = dev_get_drvdata(pdev->dev.parent);
- struct wm831x_clk *clkdata;
- int ret;
- clkdata = kzalloc(sizeof(*clkdata), GFP_KERNEL);
If devm_kzalloc() is used, then all the kfree unwinding can be dropped.
+static int __init wm831x_clk_init(void) +{
- int ret;
- ret = platform_driver_register(&wm831x_clk_driver);
- if (ret != 0)
- pr_err("Failed to register WM831x clock driver: %d\n", ret);
- return ret;
No need for this song-and-dance. The driver core is pretty well debugged. Just use "return platform_driver_register(...);"
Grant,
Thanks for the review.
Mark,
I know you're not carrying this whole set of patches but do you want to rework this and resend or do you just want me to fix it up? Changes are trivial if you don't want to touch it.
Regards, Mike
g.
On Sat, Sep 24, 2011 at 10:08:36PM -0600, Grant Likely wrote:
On Thu, Sep 22, 2011 at 03:27:01PM -0700, Mike Turquette wrote:
- ret = platform_driver_register(&wm831x_clk_driver);
- if (ret != 0)
pr_err("Failed to register WM831x clock driver: %d\n", ret);- return ret;
No need for this song-and-dance. The driver core is pretty well debugged. Just use "return platform_driver_register(...);"
No, that's not helpful. The issue isn't the device probe code itself, the issue is things like module unload not doing what they're supposed to do and leaving the device lying around or something - there's rather more going on than just the plain API call.
On Mon, Sep 26, 2011 at 10:38:58AM +0100, Mark Brown wrote:
On Sat, Sep 24, 2011 at 10:08:36PM -0600, Grant Likely wrote:
On Thu, Sep 22, 2011 at 03:27:01PM -0700, Mike Turquette wrote:
- ret = platform_driver_register(&wm831x_clk_driver);
- if (ret != 0)
pr_err("Failed to register WM831x clock driver: %d\n", ret);- return ret;
No need for this song-and-dance. The driver core is pretty well debugged. Just use "return platform_driver_register(...);"
No, that's not helpful. The issue isn't the device probe code itself, the issue is things like module unload not doing what they're supposed to do and leaving the device lying around or something - there's rather more going on than just the plain API call.
Then lets fix the core code. I see this pattern show up again and again of extra boilerplate going around platform_driver_{register,unregister}(). That says to me that there either needs to be a new helper, or the core code needs to be made more verbose.
In fact, I've been considering adding a macro for {platform,i2c,spi,...}_drivers that does all the module boilerplate for the common case of only registering a driver at init time. Something like:
#define module_platform_driver(__driver) \ int __driver##_init(void) \ { \ return platform_driver_register(&(__driver)); \ } \ module_init(__driver##_init); \ void ##__driver##_exit(void) \ { \ platform_driver_unregister(&(__driver)); \ } \ module_exit(##__driver##_exit);
It's not a lot of code, but I dislike how much boilerplate every single driver has to use if it doesn't do anything special.
g.
On Tue, Oct 04, 2011 at 12:18:18PM -0600, Grant Likely wrote:
On Mon, Sep 26, 2011 at 10:38:58AM +0100, Mark Brown wrote:
No, that's not helpful. The issue isn't the device probe code itself, the issue is things like module unload not doing what they're supposed to do and leaving the device lying around or something - there's rather more going on than just the plain API call.
Then lets fix the core code. I see this pattern show up again and again of extra boilerplate going around platform_driver_{register,unregister}(). That says to me that there either needs to be a new helper, or the core code needs to be made more verbose.
I'd go with the latter, it's pretty much the approach the subsystems I help maintain have been taking. In fast paths it's a bit different but in slow paths it tends to be helpful to know why things just fell over.
In fact, I've been considering adding a macro for {platform,i2c,spi,...}_drivers that does all the module boilerplate for the common case of only registering a driver at init time. Something like:
#define module_platform_driver(__driver) \ int __driver##_init(void) \ { \ return platform_driver_register(&(__driver)); \ } \ module_init(__driver##_init); \ void ##__driver##_exit(void) \ { \ platform_driver_unregister(&(__driver)); \ } \ module_exit(##__driver##_exit);
It's not a lot of code, but I dislike how much boilerplate every single driver has to use if it doesn't do anything special.
Yeah, this sort of stuff would be helpful - there's quite a bit of boilerplate you end up having to write to get drivers going.
On Tue, Oct 04, 2011 at 09:50:02PM +0100, Mark Brown wrote:
On Tue, Oct 04, 2011 at 12:18:18PM -0600, Grant Likely wrote:
#define module_platform_driver(__driver) \ int __driver##_init(void) \ { \ return platform_driver_register(&(__driver)); \ } \ module_init(__driver##_init); \ void ##__driver##_exit(void) \ { \ platform_driver_unregister(&(__driver)); \ } \ module_exit(##__driver##_exit);
It's not a lot of code, but I dislike how much boilerplate every single driver has to use if it doesn't do anything special.
Yeah, this sort of stuff would be helpful - there's quite a bit of boilerplate you end up having to write to get drivers going.
Draft patch written, and posting to the list soon...
g.
From: Mark Brown broonie@opensource.wolfsonmicro.com
Enable the generic clk API on x86, enabling use of the API by drivers for x86 modules and also improving build coverage for clock API using devices.
Signed-off-by: Mark Brown broonie@opensource.wolfsonmicro.com --- No change since v1
arch/x86/Kconfig | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index 6a47bb2..571b2c5 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -73,6 +73,7 @@ config X86 select HAVE_BPF_JIT if (X86_64 && NET) select CLKEVT_I8253 select ARCH_HAVE_NMI_SAFE_CMPXCHG + select GENERIC_CLK
config INSTRUCTION_DECODER def_bool (KPROBES || PERF_EVENTS)
On Thu, Sep 22, 2011 at 3:26 PM, Mike Turquette mturquette@ti.com wrote:
Hi all,
The goal of this series is to provide a cross-platform clock framework that platforms can use to model their clock trees and perform common operations on them. Currently everyone re-invents their own clock tree inside platform code which makes it impossible for drivers to use the clock APIs safely across many platforms and for distro's to compile multi-platform kernels which all redefine struct clk and its operations.
This is the second version of the common clock patches which were originally posted by Jeremy Kerr and then re-posted with some additional patches by Mark Brown. Mark's re-post didn't have any changes done to the original four patches from Jeremy which is why this series is "v2".
The changes in this series are minimal: I've folded in some of Mark's fixes and most of the comments posted to his series as well as rebasing on top of v3.1-rc7. The design and functionality hasn't changed much since Jeremy posted v1 of this series. Propagating the rate change up to the parent has been removed from clk_set_rate since that needs some more thought. I also dropped Mark's change to append a device's name to a clk name since device tree might solve this neatly. Again more discussion around that would be good.
For convenience these patches can be found at: git://git.linaro.org/people/mturquette/linux.git v3.1-rc7-clkv2
Regards, Mike
On Thu, Sep 22, 2011 at 03:26:55PM -0700, Mike Turquette wrote:
Hi all,
The goal of this series is to provide a cross-platform clock framework that platforms can use to model their clock trees and perform common operations on them. Currently everyone re-invents their own clock tree inside platform code which makes it impossible for drivers to use the clock APIs safely across many platforms and for distro's to compile multi-platform kernels which all redefine struct clk and its operations.
This is the second version of the common clock patches which were originally posted by Jeremy Kerr and then re-posted with some additional patches by Mark Brown. Mark's re-post didn't have any changes done to the original four patches from Jeremy which is why this series is "v2".
The changes in this series are minimal: I've folded in some of Mark's fixes and most of the comments posted to his series as well as rebasing on top of v3.1-rc7. The design and functionality hasn't changed much since Jeremy posted v1 of this series. Propagating the rate change up to the parent has been removed from clk_set_rate since that needs some more thought. I also dropped Mark's change to append a device's name to a clk name since device tree might solve this neatly. Again more discussion around that would be good.
v1 of the series can be found at, http://article.gmane.org/gmane.linux.kernel/1143182
Mark's re-post (v1+) can be found at, http://article.gmane.org/gmane.linux.ports.arm.kernel/129889
Looks good at first review to me. I had a few comments, but nothing major. It really needs some documentation though.
g.
On Thu, Sep 22, 2011 at 03:26:55PM -0700, Mike Turquette wrote:
more thought. I also dropped Mark's change to append a device's name to a clk name since device tree might solve this neatly. Again more discussion around that would be good.
Some of that was for debugfs type stuff - if we end up providing information about the clocks via a diagnostic interface being able to distinguish between things in a textual fashion would be handy (and I'd guess the struct device may also be needed by any OF bindings to get at the bindings?).
-
Domenico Andreoli -
Grant Likely -
Jamie Iles -
Mark Brown -
Mike Turquette -
Richard Zhao -
Richard Zhao -
Rob Herring -
Russell King - ARM Linux -
Saravana Kannan -
Sascha Hauer -
Shawn Guo -
Turquette, Mike