Hi all,
These patches are an attempt to allow platforms to share clock code. At
present, the definitions of 'struct clk' are local to platform code,
which makes allocating and initialising cross-platform clock sources
difficult, and makes it impossible to compile a single image containing
support for two ARM platforms with different struct clks.
The two patches are for the architecture-independent kernel code,
introducing the common clk infrastructure. The changelog for the first
patch includes details about the new clock definitions.
As requested by rmk, I've put together a small series of patches
illustrating the usage of the common struct clock on the ARM imx51
platform. These are available in my git tree:
git://kernel.ubuntu.com/jk/dt/linux-2.6
in the clk-common-mx51 branch (clk-common..clk-common-mx51).
The approach I've taken with the imx51 port is to temporarly duplicate
the platform-common clock code (ie, for all mxc-based boards) to enable
usage of the common struct clk on one machine (imx51), while leaving the
others as-is. For a proper platform-wide usage of the common struct clk,
we'd be better off doing the whole platform at once. However, mx51 is
the only mxc-based HW I have, hence the duplicated example port.
In the example port, the first change simply converts the mxc's struct
clk to a struct clk_mxc, using the new API. The subsequent patches move
certain clocks to more specific data structures (eg clk_fixed and
clk_pll) where possible.
Ben Herrenschmidt is looking at using common struct clk code for powerpc
too, hence the kernel-wide approach.
Comments most welcome.
Cheers,
Jeremy
--
v3:
* do clock usage refcounting in common code
* provide sample port
v2:
* no longer ARM-specific
* use clk_operations
---
Jeremy Kerr (2):
Add a common struct clk
clk: Generic support for fixed-rate clocks
Since most platforms will need a fixed-rate clock, add one. This will
also serve as a basic example of an implementation of struct clk.
Signed-off-by: Jeremy Kerr <[email protected]>
---
include/linux/clk.h | 13 +++++++++++++
kernel/Makefile | 1 +
kernel/clk.c | 25 +++++++++++++++++++++++++
3 files changed, 39 insertions(+)
diff --git a/include/linux/clk.h b/include/linux/clk.h
index f70137a..009ca73 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -112,6 +112,19 @@ static inline struct clk *clk_get_parent(struct clk *clk)
return ERR_PTR(-ENOSYS);
}
+/* Simple fixed-rate clock */
+struct clk_fixed {
+ struct clk clk;
+ unsigned long rate;
+};
+
+extern struct clk_operations clk_fixed_operations;
+
+#define DEFINE_CLK_FIXED(r) { \
+ .clk = DEFINE_CLK(clk_fixed_operations), \
+ .rate = (r) \
+}
+
#else /* !CONFIG_USE_COMMON_STRUCT_CLK */
/*
diff --git a/kernel/Makefile b/kernel/Makefile
index 057472f..1ae15aa 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -105,6 +105,7 @@ obj-$(CONFIG_PERF_EVENTS) += perf_event.o
obj-$(CONFIG_HAVE_HW_BREAKPOINT) += hw_breakpoint.o
obj-$(CONFIG_USER_RETURN_NOTIFIER) += user-return-notifier.o
obj-$(CONFIG_PADATA) += padata.o
+obj-$(CONFIG_USE_COMMON_STRUCT_CLK) += clk.o
ifneq ($(CONFIG_SCHED_OMIT_FRAME_POINTER),y)
# According to Alan Modra <[email protected]>, the -fno-omit-frame-pointer is
diff --git a/kernel/clk.c b/kernel/clk.c
new file mode 100644
index 0000000..353736c
--- /dev/null
+++ b/kernel/clk.c
@@ -0,0 +1,25 @@
+/*
+ * Copyright (C) 2010 Canonical Ltd <[email protected]>
+ *
+ * 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/module.h>
+#include <linux/clk.h>
+
+#define to_clk_fixed(clk) (container_of(clk, struct clk_fixed, clk))
+
+static unsigned long clk_fixed_get_rate(struct clk *clk)
+{
+ return to_clk_fixed(clk)->rate;
+}
+
+struct clk_operations clk_fixed_operations = {
+ .get_rate = clk_fixed_get_rate,
+};
+
+EXPORT_SYMBOL_GPL(clk_fixed_operations);
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, containing 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.
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_USE_COMMON_STRUCT_CLK. In this case, the clock infrastructure
consists of a common struct clk:
struct clk {
struct clk_operations *ops;
atomic_t enable_count;
};
And a set of clock operations (defined per type of clock):
struct clk_operations {
int (*enable)(struct clk *);
void (*disable)(struct clk *);
unsigned long (*get_rate)(struct clk *);
[...]
};
To define a hardware-specific clock, machine code can "subclass" the
struct clock into a new struct (adding any device-specific data), and
provide a set of operations:
struct clk_foo {
struct clk clk;
void __iomem *some_register;
};
struct clk_operations clk_foo_ops = {
.get_rate = clk_foo_get_rate,
};
The common clock definitions are based on a development patch from Ben
Herrenschmidt <[email protected]>.
Signed-off-by: Jeremy Kerr <[email protected]>
---
arch/Kconfig | 3
include/linux/clk.h | 148 +++++++++++++++++++++++++++++++++++---------
2 files changed, 124 insertions(+), 27 deletions(-)
diff --git a/arch/Kconfig b/arch/Kconfig
index acda512..2458b5e 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -151,4 +151,7 @@ config HAVE_MIXED_BREAKPOINTS_REGS
config HAVE_USER_RETURN_NOTIFIER
bool
+config USE_COMMON_STRUCT_CLK
+ bool
+
source "kernel/gcov/Kconfig"
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 1d37f42..f70137a 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 Jeremy Kerr <[email protected]>
*
* 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,36 +12,114 @@
#ifndef __LINUX_CLK_H
#define __LINUX_CLK_H
-struct device;
+#include <linux/err.h>
+#include <asm/atomic.h>
-/*
- * The base API.
+#ifdef CONFIG_USE_COMMON_STRUCT_CLK
+
+/* If we're using the common struct clk, we define the base clk object here,
+ * which will be 'subclassed' by device-specific implementations. For example:
+ *
+ * struct clk_foo {
+ * struct clk;
+ * [device specific fields]
+ * };
+ *
+ * We define the common clock API through a set of static inlines that call the
+ * corresponding clk_operations. The API is exactly the same as that documented
+ * in the !CONFIG_USE_COMMON_STRUCT_CLK case.
*/
+struct clk {
+ const struct clk_operations *ops;
+ atomic_t enable_count;
+};
+
+#define DEFINE_CLK(o) \
+ { .ops = &o, .enable_count = ATOMIC_INIT(0) }
+
+struct clk_operations {
+ int (*enable)(struct clk *);
+ void (*disable)(struct clk *);
+ unsigned long (*get_rate)(struct clk *);
+ void (*put)(struct clk *);
+ long (*round_rate)(struct clk *, unsigned long);
+ int (*set_rate)(struct clk *, unsigned long);
+ int (*set_parent)(struct clk *, struct clk *);
+ struct clk* (*get_parent)(struct clk *);
+};
+
+static inline int clk_enable(struct clk *clk)
+{
+ if (!clk->ops->enable)
+ return 0;
+
+ if (atomic_inc_return(&clk->enable_count) != 1)
+ return 0;
+
+ return clk->ops->enable(clk);
+}
+
+static inline void clk_disable(struct clk *clk)
+{
+ if (!clk->ops->disable)
+ return;
+
+ if (atomic_dec_return(&clk->enable_count) != 0)
+ return;
+
+ clk->ops->disable(clk);
+}
+
+static inline unsigned long clk_get_rate(struct clk *clk)
+{
+ if (clk->ops->get_rate)
+ return clk->ops->get_rate(clk);
+ return 0;
+}
+
+static inline void clk_put(struct clk *clk)
+{
+ if (clk->ops->put)
+ clk->ops->put(clk);
+}
+
+static inline long clk_round_rate(struct clk *clk, unsigned long rate)
+{
+ if (clk->ops->round_rate)
+ return clk->ops->round_rate(clk, rate);
+ return -ENOSYS;
+}
+
+static inline int clk_set_rate(struct clk *clk, unsigned long rate)
+{
+ if (clk->ops->set_rate)
+ return clk->ops->set_rate(clk, rate);
+ return -ENOSYS;
+}
+
+static inline int clk_set_parent(struct clk *clk, struct clk *parent)
+{
+ if (clk->ops->set_parent)
+ return clk->ops->set_parent(clk, parent);
+ return -ENOSYS;
+}
+
+static inline struct clk *clk_get_parent(struct clk *clk)
+{
+ if (clk->ops->get_parent)
+ return clk->ops->get_parent(clk);
+ return ERR_PTR(-ENOSYS);
+}
+
+#else /* !CONFIG_USE_COMMON_STRUCT_CLK */
/*
- * struct clk - an machine class defined object / cookie.
+ * Global clock object, actual structure is declared per-machine
*/
struct clk;
/**
- * clk_get - lookup and obtain a reference to a clock producer.
- * @dev: device for clock "consumer"
- * @id: clock comsumer ID
- *
- * Returns a struct clk corresponding to the clock producer, or
- * valid IS_ERR() condition containing errno. The implementation
- * uses @dev and @id to determine the clock consumer, and thereby
- * the clock producer. (IOW, @id may be identical strings, but
- * clk_get may return different clock producers depending on @dev.)
- *
- * Drivers must assume that the clock source is not enabled.
- *
- * clk_get should not be called from within interrupt context.
- */
-struct clk *clk_get(struct device *dev, const char *id);
-
-/**
* clk_enable - inform the system when the clock source should be running.
* @clk: clock source
*
@@ -83,12 +162,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
@@ -125,6 +198,27 @@ int clk_set_parent(struct clk *clk, struct clk *parent);
*/
struct clk *clk_get_parent(struct clk *clk);
+#endif /* !CONFIG_USE_COMMON_STRUCT_CLK */
+
+struct device;
+
+/**
+ * clk_get - lookup and obtain a reference to a clock producer.
+ * @dev: device for clock "consumer"
+ * @id: clock comsumer ID
+ *
+ * Returns a struct clk corresponding to the clock producer, or
+ * valid IS_ERR() condition containing errno. The implementation
+ * uses @dev and @id to determine the clock consumer, and thereby
+ * the clock producer. (IOW, @id may be identical strings, but
+ * clk_get may return different clock producers depending on @dev.)
+ *
+ * Drivers must assume that the clock source is not enabled.
+ *
+ * clk_get should not be called from within interrupt context.
+ */
+struct clk *clk_get(struct device *dev, const char *id);
+
/**
* clk_get_sys - get a clock based upon the device name
* @dev_id: device name
On Wed, Jun 02, 2010 at 07:56:44PM +0800, Jeremy Kerr wrote:
> 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, containing 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.
>
> 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_USE_COMMON_STRUCT_CLK. In this case, the clock infrastructure
> consists of a common struct clk:
>
> struct clk {
> struct clk_operations *ops;
> atomic_t enable_count;
> };
>
> And a set of clock operations (defined per type of clock):
>
> struct clk_operations {
> int (*enable)(struct clk *);
I'd rather the enable/disable calls where simply a set
and a bool on/off, very rarelyt is the enable and disable
operartions different.
an aside, you might want to just clal these clk_ops to get into the
spirit of the original naming.
> void (*disable)(struct clk *);
> unsigned long (*get_rate)(struct clk *);
> [...]
> };
>
> To define a hardware-specific clock, machine code can "subclass" the
> struct clock into a new struct (adding any device-specific data), and
> provide a set of operations:
>
> struct clk_foo {
> struct clk clk;
> void __iomem *some_register;
> };
>
> struct clk_operations clk_foo_ops = {
> .get_rate = clk_foo_get_rate,
> };
>
> The common clock definitions are based on a development patch from Ben
> Herrenschmidt <[email protected]>.
>
> Signed-off-by: Jeremy Kerr <[email protected]>
>
> ---
> arch/Kconfig | 3
> include/linux/clk.h | 148 +++++++++++++++++++++++++++++++++++---------
> 2 files changed, 124 insertions(+), 27 deletions(-)
>
> diff --git a/arch/Kconfig b/arch/Kconfig
> index acda512..2458b5e 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -151,4 +151,7 @@ config HAVE_MIXED_BREAKPOINTS_REGS
> config HAVE_USER_RETURN_NOTIFIER
> bool
>
> +config USE_COMMON_STRUCT_CLK
> + bool
> +
> source "kernel/gcov/Kconfig"
> diff --git a/include/linux/clk.h b/include/linux/clk.h
> index 1d37f42..f70137a 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 Jeremy Kerr <[email protected]>
> *
> * 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,36 +12,114 @@
> #ifndef __LINUX_CLK_H
> #define __LINUX_CLK_H
>
> -struct device;
> +#include <linux/err.h>
> +#include <asm/atomic.h>
>
> -/*
> - * The base API.
> +#ifdef CONFIG_USE_COMMON_STRUCT_CLK
> +
> +/* If we're using the common struct clk, we define the base clk object here,
> + * which will be 'subclassed' by device-specific implementations. For example:
> + *
> + * struct clk_foo {
> + * struct clk;
> + * [device specific fields]
> + * };
> + *
> + * We define the common clock API through a set of static inlines that call the
> + * corresponding clk_operations. The API is exactly the same as that documented
> + * in the !CONFIG_USE_COMMON_STRUCT_CLK case.
> */
>
> +struct clk {
> + const struct clk_operations *ops;
> + atomic_t enable_count;
> +};
> +
> +#define DEFINE_CLK(o) \
> + { .ops = &o, .enable_count = ATOMIC_INIT(0) }
> +
> +struct clk_operations {
> + int (*enable)(struct clk *);
> + void (*disable)(struct clk *);
> + unsigned long (*get_rate)(struct clk *);
> + void (*put)(struct clk *);
> + long (*round_rate)(struct clk *, unsigned long);
> + int (*set_rate)(struct clk *, unsigned long);
> + int (*set_parent)(struct clk *, struct clk *);
> + struct clk* (*get_parent)(struct clk *);
> +};
> +
> +static inline int clk_enable(struct clk *clk)
> +{
> + if (!clk->ops->enable)
> + return 0;
> +
> + if (atomic_inc_return(&clk->enable_count) != 1)
> + return 0;
> +
> + return clk->ops->enable(clk);
> +}
> +
> +static inline void clk_disable(struct clk *clk)
> +{
> + if (!clk->ops->disable)
> + return;
> +
> + if (atomic_dec_return(&clk->enable_count) != 0)
> + return;
> +
> + clk->ops->disable(clk);
> +}
> +
> +static inline unsigned long clk_get_rate(struct clk *clk)
> +{
> + if (clk->ops->get_rate)
> + return clk->ops->get_rate(clk);
> + return 0;
> +}
> +
> +static inline void clk_put(struct clk *clk)
> +{
> + if (clk->ops->put)
> + clk->ops->put(clk);
> +}
> +
> +static inline long clk_round_rate(struct clk *clk, unsigned long rate)
> +{
> + if (clk->ops->round_rate)
> + return clk->ops->round_rate(clk, rate);
> + return -ENOSYS;
> +}
> +
> +static inline int clk_set_rate(struct clk *clk, unsigned long rate)
> +{
> + if (clk->ops->set_rate)
> + return clk->ops->set_rate(clk, rate);
> + return -ENOSYS;
> +}
> +
> +static inline int clk_set_parent(struct clk *clk, struct clk *parent)
> +{
> + if (clk->ops->set_parent)
> + return clk->ops->set_parent(clk, parent);
> + return -ENOSYS;
> +}
> +
> +static inline struct clk *clk_get_parent(struct clk *clk)
> +{
> + if (clk->ops->get_parent)
> + return clk->ops->get_parent(clk);
> + return ERR_PTR(-ENOSYS);
> +}
> +
> +#else /* !CONFIG_USE_COMMON_STRUCT_CLK */
>
> /*
> - * struct clk - an machine class defined object / cookie.
> + * Global clock object, actual structure is declared per-machine
> */
> struct clk;
>
> /**
> - * clk_get - lookup and obtain a reference to a clock producer.
> - * @dev: device for clock "consumer"
> - * @id: clock comsumer ID
> - *
> - * Returns a struct clk corresponding to the clock producer, or
> - * valid IS_ERR() condition containing errno. The implementation
> - * uses @dev and @id to determine the clock consumer, and thereby
> - * the clock producer. (IOW, @id may be identical strings, but
> - * clk_get may return different clock producers depending on @dev.)
> - *
> - * Drivers must assume that the clock source is not enabled.
> - *
> - * clk_get should not be called from within interrupt context.
> - */
> -struct clk *clk_get(struct device *dev, const char *id);
> -
> -/**
> * clk_enable - inform the system when the clock source should be running.
> * @clk: clock source
> *
> @@ -83,12 +162,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
> @@ -125,6 +198,27 @@ int clk_set_parent(struct clk *clk, struct clk *parent);
> */
> struct clk *clk_get_parent(struct clk *clk);
>
> +#endif /* !CONFIG_USE_COMMON_STRUCT_CLK */
> +
> +struct device;
> +
> +/**
> + * clk_get - lookup and obtain a reference to a clock producer.
> + * @dev: device for clock "consumer"
> + * @id: clock comsumer ID
> + *
> + * Returns a struct clk corresponding to the clock producer, or
> + * valid IS_ERR() condition containing errno. The implementation
> + * uses @dev and @id to determine the clock consumer, and thereby
> + * the clock producer. (IOW, @id may be identical strings, but
> + * clk_get may return different clock producers depending on @dev.)
> + *
> + * Drivers must assume that the clock source is not enabled.
> + *
> + * clk_get should not be called from within interrupt context.
> + */
> +struct clk *clk_get(struct device *dev, const char *id);
> +
> /**
> * clk_get_sys - get a clock based upon the device name
> * @dev_id: device name
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
--
Ben
Q: What's a light-year?
A: One-third less calories than a regular year.
Hi Ben,
> > And a set of clock operations (defined per type of clock):
> >
> > struct clk_operations {
> >
> > int (*enable)(struct clk *);
>
> I'd rather the enable/disable calls where simply a set
> and a bool on/off, very rarelyt is the enable and disable
> operartions different.
I thought about merging these, but decided against it. It does work for the
simple case where we're setting a bit in a register:
static int clk_foo_set_state(struct clk *_clk, int enable)
{
struct clk_foo *clk = to_clk_foo(_clk)
u32 reg;
reg = raw_readl(foo->some_register);
if (enable)
reg |= FOO_ENABLE;
else
reg &= ~FOO_ENABLE;
raw_writel(foo->some_register, reg);
return 0;
}
However, for anything more complex than this - for example, if there's a
parent clock - then we start getting pretty messy:
static int clk_foo_set_state(struct clk *_clk, int enable)
{
struct clk_foo *clk = to_clk_foo(_clk)
u32 reg;
if (enable) {
int ret = clk_enable(clk->parent);
if (ret)
return ret;
}
reg = raw_readl(foo->some_register);
if (enable)
reg |= FOO_ENABLE;
else
reg &= ~FOO_ENABLE;
raw_writel(foo->some_register, reg);
if (!enable)
clk_disable(clk->parent);
return 0;
}
- where most of the function becomes surrounded by "if (enable)" statements.
I'm aware that we can turn this into a conditional call of clk_foo_enable or
clk_foo_disable, but then we're back to square 1. I also think that the simple
case is clearer (if a little more verbose) with separate functions.
Also, enable and disable in the external clock API have different return
types.
> an aside, you might want to just clal these clk_ops to get into the
> spirit of the original naming.
Either is fine with me - looks like 'ops' is more commonly used:
$ git grep -E '^struct \w*operations\s*\{' include/ | wc -l
30
$ git grep -E '^struct \w*ops\s*{' include/ | wc -l
138
Cheers,
Jeremy
On Thu, Jun 03, 2010 at 11:21:19AM +0800, Jeremy Kerr wrote:
> Hi Ben,
>
> > > And a set of clock operations (defined per type of clock):
> > >
> > > struct clk_operations {
> > >
> > > int (*enable)(struct clk *);
> >
> > I'd rather the enable/disable calls where simply a set
> > and a bool on/off, very rarelyt is the enable and disable
> > operartions different.
>
> I thought about merging these, but decided against it. It does work for the
> simple case where we're setting a bit in a register:
>
> static int clk_foo_set_state(struct clk *_clk, int enable)
> {
> struct clk_foo *clk = to_clk_foo(_clk)
> u32 reg;
>
> reg = raw_readl(foo->some_register);
> if (enable)
> reg |= FOO_ENABLE;
> else
> reg &= ~FOO_ENABLE;
> raw_writel(foo->some_register, reg);
>
> return 0;
> }
>
> However, for anything more complex than this - for example, if there's a
> parent clock - then we start getting pretty messy:
>
> static int clk_foo_set_state(struct clk *_clk, int enable)
> {
> struct clk_foo *clk = to_clk_foo(_clk)
> u32 reg;
Yuck. I think this should really be handled by the base clk_enable()
and clk_disable() calls. Roughly based on what is currently in the
plat-samsung clock implementation:
clk_enable(struct clk *clk)
{
if (clk->parent)
clk_enable(clk->parent)
...
}
clk_disable(struct clk *clk)
{
...
if (clk->parent)
clk_disable(clk->parent)
}
I think it is a really bad idea for each implementation to have to worry
about this. It sounds like a recipie for people to get wrong, especially
if we have a number of these implementations kicking around.
> if (enable) {
> int ret = clk_enable(clk->parent);
> if (ret)
> return ret;
> }
>
> reg = raw_readl(foo->some_register);
> if (enable)
> reg |= FOO_ENABLE;
> else
> reg &= ~FOO_ENABLE;
>
> raw_writel(foo->some_register, reg);
>
> if (!enable)
> clk_disable(clk->parent);
>
> return 0;
> }
>
> - where most of the function becomes surrounded by "if (enable)" statements.
>
> I'm aware that we can turn this into a conditional call of clk_foo_enable or
> clk_foo_disable, but then we're back to square 1. I also think that the simple
> case is clearer (if a little more verbose) with separate functions.
If we do decided to move the parent control functionality to the clock
core, then I would prefer to see the change to a single enable/disable
callback. Especially as it fits my current implementations well.
As a note, I also left the enable callback in the 'struct clk' instead
of in the ops, enable/disable is the most used case of these clock
functions, and as such should probably be the easiest to get to.
Also, wheras plat-samsung has very few sets of clk_ops sitting about,
there are more enable/disable calls, and adding more fields to the
clocks to deal with this would add extra space to the kernel.
> Also, enable and disable in the external clock API have different return
> types.
does that really matter?
> > an aside, you might want to just clal these clk_ops to get into the
> > spirit of the original naming.
>
> Either is fine with me - looks like 'ops' is more commonly used:
My pref. is for less typing.
> $ git grep -E '^struct \w*operations\s*\{' include/ | wc -l
> 30
>
> $ git grep -E '^struct \w*ops\s*{' include/ | wc -l
> 138
>
> Cheers,
>
>
> Jeremy
--
--
Ben
Q: What's a light-year?
A: One-third less calories than a regular year.
Hi Ben,
> clk_enable(struct clk *clk)
> {
> if (clk->parent)
> clk_enable(clk->parent)
> ...
> }
>
> clk_disable(struct clk *clk)
> {
> ...
> if (clk->parent)
> clk_disable(clk->parent)
> }
>
> I think it is a really bad idea for each implementation to have to worry
> about this. It sounds like a recipie for people to get wrong, especially
> if we have a number of these implementations kicking around.
OK, this would mean adding parent to struct clk:
struct clk {
struct clk_operations *ops;
atomic_t enable_count;
struct clk *parent;
};
I was originally intending for struct clk to have the absolute minimum of
fields, only those necessary for every clock in the system. parent didn't make
the cut, as some clocks don't have a parent.
However, lets explore this a little - handling parents in the infrastructure
code this may simplify the hardware-specific code somewhat. We'd add the
parent-handling stuff to the global clk_enable/clk_disable:
static inline int clk_enable(struct clk *clk)
{
int ret;
if (atomic_inc(clk->enable_count) != 1))
return 0;
if (clk->parent) {
ret = clk_enable(clk->parent);
if (ret)
goto out_dec;
}
ret = clk->ops->enable(clk)
if (!ret)
return 0;
out_dec:
atomic_dec(clk->enable_count);
return ret;
}
The downside here is that the static inline becomes quite bloated, and we can
no longer avoid the atomic operation if there is no enable op. I guess we
could add a:
if (!clk->ops->enable && !clk->parent)
return 0;
but now were in serious "don't do that as a inline" territory. So we'd be
better off making this a proper function.
[as an aside, I need to add the atomic_dec to the error path of my current
code, will respin a new version. But even then, it's small enough to inline]
I think that handling the enable/disable in the hardware-specific op is an
acceptable solution. It's only one line of code (or two if you want to check
for the presence of a parent first), and is, after all, a hardware-specific
property of the clock.
So, we either:
1) keep it as is, with the hw-specific code handling parent enable/disable; or
2) add the parent-handling code to the clock infrastructure and move the core
API functions out-of-line.
> As a note, I also left the enable callback in the 'struct clk' instead
> of in the ops, enable/disable is the most used case of these clock
> functions, and as such should probably be the easiest to get to.
I strongly disagree on this one. I want all of the ops in one place, not
scattered around the API.
> Also, wheras plat-samsung has very few sets of clk_ops sitting about,
> there are more enable/disable calls, and adding more fields to the
> clocks to deal with this would add extra space to the kernel.
Sorry, I don't think I understand your point here - you're saying that moving
the enable/disable callbacks to struct clk will increase the size of the
kernel (which is correct, as there are more struct clks than there are struct
clk_operations), so doesn't this provide an argument against doing this?
> > Also, enable and disable in the external clock API have different return
> > types.
>
> does that really matter?
Only if someone expects a failed disable to be detected by the driver.
> > Either is fine with me - looks like 'ops' is more commonly used:
> My pref. is for less typing.
OK, will do this in the next revision.
Cheers,
Jeremy
On Thu, Jun 03, 2010 at 06:24:50PM +0800, Jeremy Kerr wrote:
> OK, this would mean adding parent to struct clk:
>
> struct clk {
> struct clk_operations *ops;
> atomic_t enable_count;
I don't think it makes sense for this to be an atomic type.
> static inline int clk_enable(struct clk *clk)
> {
> int ret;
>
> if (atomic_inc(clk->enable_count) != 1))
> return 0;
Okay, so what if we sleep at this point, and someone else comes along
and calls clk_enable(), which'll merely increment the count and return.
Unfortunately, the clock is still disabled at that point - which is a
violation of what's supposed to happen.
Or to put it another way, the above method results in clk_enable()
sometimes returning with the clock enabled and sometimes with the
clock still disabled.
That's not nice behaviour for drivers which may need the clock enabled
to read/write the device registers.
Ben Dooks wrote:
> On Thu, Jun 03, 2010 at 11:21:19AM +0800, Jeremy Kerr wrote:
>> Hi Ben,
>>
>>>> And a set of clock operations (defined per type of clock):
>>>>
>>>> struct clk_operations {
>>>>
>>>> int (*enable)(struct clk *);
>>> I'd rather the enable/disable calls where simply a set
>>> and a bool on/off, very rarelyt is the enable and disable
>>> operartions different.
>> I thought about merging these, but decided against it. It does work for the
>> simple case where we're setting a bit in a register:
>>
>> static int clk_foo_set_state(struct clk *_clk, int enable)
>> {
>> struct clk_foo *clk = to_clk_foo(_clk)
>> u32 reg;
>>
>> reg = raw_readl(foo->some_register);
>> if (enable)
>> reg |= FOO_ENABLE;
>> else
>> reg &= ~FOO_ENABLE;
>> raw_writel(foo->some_register, reg);
>>
>> return 0;
>> }
>>
>> However, for anything more complex than this - for example, if there's a
>> parent clock - then we start getting pretty messy:
>>
>> static int clk_foo_set_state(struct clk *_clk, int enable)
>> {
>> struct clk_foo *clk = to_clk_foo(_clk)
>> u32 reg;
>
> Yuck. I think this should really be handled by the base clk_enable()
> and clk_disable() calls. Roughly based on what is currently in the
> plat-samsung clock implementation:
I think its a good idea to do this incrementally. The proposed patches
don't require much code rewrite because the interface is basically the
same. I think the best approach is to get the proposed patches applied,
which basically just makes the common interface from
include/linux/clock.h generic, and _all_ of the mach implementations
(and possibly other archs such as powerpc) converted and tested first.
Then we can go from there to see what other common functionality can be
moved into the generic clock framework.
~Ryan
--
Bluewater Systems Ltd - ARM Technology Solution Centre
Ryan Mallon 5 Amuri Park, 404 Barbadoes St
[email protected] PO Box 13 889, Christchurch 8013
http://www.bluewatersys.com New Zealand
Phone: +64 3 3779127 Freecall: Australia 1800 148 751
Fax: +64 3 3779135 USA 1800 261 2934
On Fri, Jun 04, 2010 at 09:09:29AM +1200, Ryan Mallon wrote:
> Ben Dooks wrote:
> > On Thu, Jun 03, 2010 at 11:21:19AM +0800, Jeremy Kerr wrote:
> >> Hi Ben,
> >>
> >>>> And a set of clock operations (defined per type of clock):
> >>>>
> >>>> struct clk_operations {
> >>>>
> >>>> int (*enable)(struct clk *);
> >>> I'd rather the enable/disable calls where simply a set
> >>> and a bool on/off, very rarelyt is the enable and disable
> >>> operartions different.
> >> I thought about merging these, but decided against it. It does work for the
> >> simple case where we're setting a bit in a register:
> >>
> >> static int clk_foo_set_state(struct clk *_clk, int enable)
> >> {
> >> struct clk_foo *clk = to_clk_foo(_clk)
> >> u32 reg;
> >>
> >> reg = raw_readl(foo->some_register);
> >> if (enable)
> >> reg |= FOO_ENABLE;
> >> else
> >> reg &= ~FOO_ENABLE;
> >> raw_writel(foo->some_register, reg);
> >>
> >> return 0;
> >> }
> >>
> >> However, for anything more complex than this - for example, if there's a
> >> parent clock - then we start getting pretty messy:
> >>
> >> static int clk_foo_set_state(struct clk *_clk, int enable)
> >> {
> >> struct clk_foo *clk = to_clk_foo(_clk)
> >> u32 reg;
> >
> > Yuck. I think this should really be handled by the base clk_enable()
> > and clk_disable() calls. Roughly based on what is currently in the
> > plat-samsung clock implementation:
>
> I think its a good idea to do this incrementally. The proposed patches
> don't require much code rewrite because the interface is basically the
> same. I think the best approach is to get the proposed patches applied,
> which basically just makes the common interface from
Given the latest comments by Linus on churn, it would be better to
get a well specified <linux/clk.h> decided on before it goes in so
that everyone can move over to it. We're moving to a system where
any change in functionality is going to cause problems with respect
to a wide range of systems.
If the new <linux/clk.h> is not well specified it is just goign to
cause problems down the line of people infering behaviour from other
implementations (a bad idea) and/or causing large tracts of changes.
> include/linux/clock.h generic, and _all_ of the mach implementations
> (and possibly other archs such as powerpc) converted and tested first.
> Then we can go from there to see what other common functionality can be
> moved into the generic clock framework.
--
Ben
Q: What's a light-year?
A: One-third less calories than a regular year.
On Thu, Jun 03, 2010 at 12:05:33PM +0100, Russell King - ARM Linux wrote:
> On Thu, Jun 03, 2010 at 06:24:50PM +0800, Jeremy Kerr wrote:
> > OK, this would mean adding parent to struct clk:
> >
> > struct clk {
> > struct clk_operations *ops;
> > atomic_t enable_count;
>
> I don't think it makes sense for this to be an atomic type.
>
> > static inline int clk_enable(struct clk *clk)
> > {
> > int ret;
> >
> > if (atomic_inc(clk->enable_count) != 1))
> > return 0;
>
> Okay, so what if we sleep at this point, and someone else comes along
> and calls clk_enable(), which'll merely increment the count and return.
>
> Unfortunately, the clock is still disabled at that point - which is a
> violation of what's supposed to happen.
>
> Or to put it another way, the above method results in clk_enable()
> sometimes returning with the clock enabled and sometimes with the
> clock still disabled.
>
> That's not nice behaviour for drivers which may need the clock enabled
> to read/write the device registers.
You're right, especially if the clock takes time to enable.
Either we need some form of locking to deal with this. An overall lock
will cause problems with respect to re-using clk_enable() internally
and per-clock locks are probably a bad idea with respect of trying to
lock the parents (deadlock) depending on the implementation.
I'm not sure if something like a wake queue is required, where any caller
who is calling when the clock is disabled gets placed on the queue
awaiting the original enabler finishing the enable operation.
if (atomic_increment(&clk->usage_count) == 1) {
/* start enable */
atomic_set(&clk->is_enabled, 1);
wake_up(&clk->wait_for_enable);
} else if (atomic_read(&clk->is_enabled) == 0) {
wait_event(&clk->wait_for_enable);
}
Also, what about the behaviour across >1 CPU? There's a pile of
up-comming SoCs with SMP ARM cores. If CPU1 enables the clock, and CPU2
comes along and tries to do that also, we have the same race condition
again.
As an addendum to Russell's comments, we need to specify the behaviour
with resepect to what happens about clock stabilisation as well, if the
clock we enabled takes time to stabilise, then should we wait? This will
be even worse when we realise some of these architectures have clocks
which are source from PLLs that can be turned on/off, and these PLLs
have settling time in 100s of usecs.
Even if the clock is enabled, it requires time to stabilise to avoid
problems when the driver then goes to try and use it.
--
Ben
Q: What's a light-year?
A: One-third less calories than a regular year.
Hi Russell,
> Or to put it another way, the above method results in clk_enable()
> sometimes returning with the clock enabled and sometimes with the
> clock still disabled.
>
> That's not nice behaviour for drivers which may need the clock enabled
> to read/write the device registers.
OK, I'll rework with proper locking to ensure that the clock is
enabled/disabled on return from clk_enable.
Cheers,
Jeremy
Hi Ben,
> As an addendum to Russell's comments, we need to specify the behaviour
> with resepect to what happens about clock stabilisation as well, if the
> clock we enabled takes time to stabilise, then should we wait?
Yes. If we're going to the effort of ensuring that the clock is enabled before
clk_enable returns, we should also guarantee that the clock is fully-
functional.
If that takes some time, then that's just the cost of ensuring correct
behaviour.
Cheers,
Jeremy