[PATCH 0/3] Clocklib: generic clocks framework

[PATCH 0/3] Clocklib: generic clocks framework

[Dmitry Baryshkov]

Hi,

The <linux/clk.h> provides the API to use clocks.
However there is no API for clock providers. Commonly
clocks are provided by platform-specific code, which
implements full <linux/clk.h> API for itself. It results
in massive code duplication and lack of flexibility.
If I'd like to be able to provide clocks from the driver
for e.g. CPU companion chip, I'd either have to implement
a lot of platform-specific hooks, or invent some other
dirty hacks.

In the followup I'd like to propose the generic <linux/clk.h>
implementation, that can be used to hook clock definitions
from various sources. Also as an example there will be a patch
to convert ARM SA-1100 to the clocklib.

I'd like arch maintainers to check whether there is somthing
in this implementation that wouldn't work for their piece
of kernel, whether is's suitable for them to drop their own
clock implementation (if any) and to use clocklib.

-- 
With best wishes
Dmitry

[PATCH 1/3] Clocklib: add generic framework for managing clocks.

[Dmitry Baryshkov]

Provide a generic framework that platform may choose
to support clocks api. In particular this provides
platform-independant struct clk definition, a full
implementation of clocks api and a set of functions
for registering and unregistering clocks in a safe way.

Signed-off-by: Dmitry Baryshkov <dbaryshkov@gmail.com>
---
 include/linux/clklib.h |  120 ++++++++++++++++++
 init/Kconfig           |    7 +
 kernel/Makefile        |    1 +
 kernel/clklib.c        |  315 ++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 443 insertions(+), 0 deletions(-)
 create mode 100644 include/linux/clklib.h
 create mode 100644 kernel/clklib.c

diff --git a/include/linux/clklib.h b/include/linux/clklib.h
new file mode 100644
index 0000000..92f0a45
--- /dev/null
+++ b/include/linux/clklib.h
@@ -0,0 +1,120 @@
+/*
+ * Copyright (C) 2008 Dmitry Baryshkov
+ *
+ * This file is released under the GPL v2.
+ */
+
+#ifndef CLKLIB_H
+#define CLKLIB_H
+
+#include <linux/list.h>
+
+struct seq_file;
+
+struct clk {
+	struct list_head node;
+	struct clk	*parent;
+
+	const char	*name;
+	struct module	*owner;
+
+	int		users;
+	unsigned long	rate;
+	int		delay;
+
+	int (*can_get)	(struct clk *, struct device *);
+	int (*set_parent) (struct clk *, struct clk *);
+	int (*enable)	(struct clk *);
+	void (*disable)	(struct clk *);
+	unsigned long (*getrate) (struct clk*);
+	int (*setrate)	(struct clk *, unsigned long);
+	long (*roundrate) (struct clk *, unsigned long);
+
+	void		*priv;
+};
+
+int __must_check clk_register(struct clk *clk);
+void clk_unregister(struct clk *clk);
+static void __maybe_unused clks_unregister(struct clk *clks, size_t num)
+{
+	int i;
+	for (i = num - 1; i >= 0;  i++) {
+		clk_unregister(&clks[i]);
+	}
+}
+
+static int __must_check __maybe_unused clks_register(struct clk *clks, size_t num)
+{
+	int i;
+	int ret;
+	for (i = 0; i < num; i++) {
+		ret = clk_register(&clks[i]);
+		if (ret != 0)
+			goto cleanup;
+	}
+
+	return 0;
+
+cleanup:
+	clks_unregister(clks, i);
+
+	for (i -- ; i >= 0; i--) {
+		clk_unregister(&clks[i]);
+	}
+
+	return ret;
+}
+
+int __must_check clk_alloc_function(const char *parent, struct clk *clk);
+
+struct clk_function {
+	const char *parent;
+	struct clk *clk;
+};
+
+#define CLK_FUNC(_clock, _function, _can_get, _data, _format) \
+	{						\
+		.parent = _clock,			\
+		.clk = &(struct clk) {			\
+			.name= _function,		\
+			.owner = THIS_MODULE,		\
+			.can_get = _can_get,		\
+			.priv = _data,			\
+			.format = _format,		\
+		},					\
+	}
+
+static void __maybe_unused clk_free_functions(
+		struct clk_function *funcs,
+		int num)
+{
+	int i;
+
+	for (i = num - 1; i >= 0; i--) {
+		clk_unregister(funcs[i].clk);
+	}
+}
+
+static int __must_check __maybe_unused clk_alloc_functions(
+		struct clk_function *funcs,
+		int num)
+{
+	int i;
+	int rc;
+
+	for (i = 0; i < num; i++) {
+		rc = clk_alloc_function(funcs[i].parent, funcs[i].clk);
+
+		if (rc) {
+			printk(KERN_ERR "Error allocating %s.%s function.\n",
+					funcs[i].parent,
+					funcs[i].clk->name);
+			clk_free_functions(funcs, i);
+			return rc;
+		}
+	}
+
+	return 0;
+}
+
+#endif
diff --git a/init/Kconfig b/init/Kconfig
index a97924b..1dd9ce2 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -504,6 +504,13 @@ config CC_OPTIMIZE_FOR_SIZE
 config SYSCTL
 	bool
 
+config HAVE_CLOCK_LIB
+	bool
+	help
+	  Platforms select clocklib if they use this infrastructure
+	  for managing their clocks both built into SoC and provided
+	  by external devices.
+
 menuconfig EMBEDDED
 	bool "Configure standard kernel features (for small systems)"
 	help
diff --git a/kernel/Makefile b/kernel/Makefile
index 6c584c5..afaed51 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -68,6 +68,7 @@ obj-$(CONFIG_TASK_DELAY_ACCT) += delayacct.o
 obj-$(CONFIG_TASKSTATS) += taskstats.o tsacct.o
 obj-$(CONFIG_MARKERS) += marker.o
 obj-$(CONFIG_LATENCYTOP) += latencytop.o
+obj-$(CONFIG_HAVE_CLOCK_LIB) += clklib.o
 
 ifneq ($(CONFIG_SCHED_NO_NO_OMIT_FRAME_POINTER),y)
 # According to Alan Modra <alan@linuxcare.com.au>, the -fno-omit-frame-pointer is
diff --git a/kernel/clklib.c b/kernel/clklib.c
new file mode 100644
index 0000000..b41e7c2
--- /dev/null
+++ b/kernel/clklib.c
@@ -0,0 +1,315 @@
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/clk.h>
+#include <linux/clklib.h>
+#include <linux/spinlock.h>
+#include <linux/err.h>
+#include <linux/delay.h>
+
+static LIST_HEAD(clocks);
+static DEFINE_SPINLOCK(clocks_lock);
+
+static int __clk_register(struct clk *clk)
+{
+	if (clk->parent &&
+	    !try_module_get(clk->parent->owner))
+		return -EINVAL;
+
+	list_add_tail(&clk->node, &clocks);
+
+	return 0;
+}
+
+int __must_check clk_register(struct clk *clk)
+{
+	unsigned long flags;
+	int rc;
+
+	spin_lock_irqsave(&clocks_lock, flags);
+
+	rc = __clk_register(clk);
+
+	spin_unlock_irqrestore(&clocks_lock, flags);
+
+	return rc;
+}
+EXPORT_SYMBOL(clk_register);
+
+void clk_unregister(struct clk *clk)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&clocks_lock, flags);
+	list_del(&clk->node);
+	if (clk->parent)
+		module_put(clk->parent->owner);
+	spin_unlock_irqrestore(&clocks_lock, flags);
+}
+EXPORT_SYMBOL(clk_unregister);
+
+struct clk *clk_get(struct device *dev, const char *id)
+{
+	struct clk *p, *clk = ERR_PTR(-ENOENT);
+	unsigned long flags;
+
+	spin_lock_irqsave(&clocks_lock, flags);
+
+	list_for_each_entry(p, &clocks, node) {
+		if (strcmp(id, p->name) == 0 &&
+		    (p->can_get && p->can_get(p, dev)) &&
+		    try_module_get(p->owner)) {
+			clk = p;
+			break;
+		}
+	}
+
+	list_for_each_entry(p, &clocks, node) {
+		if (strcmp(id, p->name) == 0 &&
+		    !p->can_get &&
+		    try_module_get(p->owner)) {
+			clk = p;
+			break;
+		}
+	}
+
+	spin_unlock_irqrestore(&clocks_lock, flags);
+
+	return clk;
+}
+EXPORT_SYMBOL(clk_get);
+
+void clk_put(struct clk *clk)
+{
+	unsigned long flags;
+
+	if (!clk || IS_ERR(clk))
+		return;
+
+	spin_lock_irqsave(&clocks_lock, flags);
+
+	module_put(clk->owner);
+
+	spin_unlock_irqrestore(&clocks_lock, flags);
+}
+EXPORT_SYMBOL(clk_put);
+
+int clk_set_parent(struct clk *clk, struct clk *parent)
+{
+	int rc;
+	unsigned long flags;
+
+	if (!clk || IS_ERR(clk))
+		return -EINVAL;
+
+	if (!clk->set_parent)
+		return -EINVAL;
+
+	spin_lock_irqsave(&clocks_lock, flags);
+
+	rc = clk->set_parent(clk, parent);
+	if (!rc)
+		clk->parent = parent;
+
+	spin_unlock_irqrestore(&clocks_lock, flags);
+
+	return rc;
+}
+EXPORT_SYMBOL(clk_set_parent);
+
+static void __clk_disable(struct clk *clk)
+{
+	if (clk->users <= 0) {
+		WARN_ON(1);
+		return;
+	}
+
+	if (--clk->users == 0)
+		if (clk->disable)
+			clk->disable(clk);
+
+	if (clk->parent)
+		__clk_disable(clk->parent);
+}
+
+void clk_disable(struct clk *clk)
+{
+	unsigned long flags;
+
+	if (!clk || IS_ERR(clk))
+		return;
+
+	spin_lock_irqsave(&clocks_lock, flags);
+
+	__clk_disable(clk);
+
+	spin_unlock_irqrestore(&clocks_lock, flags);
+}
+EXPORT_SYMBOL(clk_disable);
+
+static int __clk_enable(struct clk *clk)
+{
+	int rc = 0;
+
+	if (clk->parent) {
+		rc = __clk_enable(clk->parent);
+
+		if (rc)
+			return rc;
+	}
+
+	if (clk->users++ != 0) {
+		return 0;
+	}
+
+	if (clk->enable) {
+		rc = clk->enable(clk);
+		if (rc) {
+			if (clk->parent)
+				__clk_disable(clk->parent);
+
+			return rc;
+		}
+	}
+
+	if (clk->delay)
+		udelay(clk->delay);
+
+	return rc;
+}
+
+int clk_enable(struct clk *clk)
+{
+	unsigned long flags;
+	int rc;
+
+	if (!clk || IS_ERR(clk))
+		return -EINVAL;
+
+	spin_lock_irqsave(&clocks_lock, flags);
+
+	rc = __clk_enable(clk);
+
+	spin_unlock_irqrestore(&clocks_lock, flags);
+
+	return rc;
+}
+EXPORT_SYMBOL(clk_enable);
+
+static unsigned long __clk_get_rate(struct clk *clk)
+{
+	unsigned long rate = 0;
+
+	for (;;) {
+		if (rate || !clk)
+			return rate;
+
+		if (clk->getrate)
+			rate = clk->getrate(clk);
+		else if (clk->rate)
+			rate = clk->rate;
+		else
+			clk = clk->parent;
+	}
+}
+
+unsigned long clk_get_rate(struct clk *clk)
+{
+	unsigned long rate = 0;
+	unsigned long flags;
+
+	if (!clk || IS_ERR(clk))
+		return -EINVAL;
+
+	spin_lock_irqsave(&clocks_lock, flags);
+
+	rate = __clk_get_rate(clk);
+
+	spin_unlock_irqrestore(&clocks_lock, flags);
+
+	return rate;
+}
+EXPORT_SYMBOL(clk_get_rate);
+
+long clk_round_rate(struct clk *clk, unsigned long rate)
+{
+	long res;
+	unsigned long flags;
+
+	if (!clk || IS_ERR(clk))
+		return -EINVAL;
+
+	spin_lock_irqsave(&clocks_lock, flags);
+
+	if (clk->roundrate)
+		res = clk->roundrate(clk, rate);
+	else
+		res = __clk_get_rate(clk);
+
+	spin_unlock_irqrestore(&clocks_lock, flags);
+
+	return res;
+}
+EXPORT_SYMBOL(clk_round_rate);
+
+int clk_set_rate(struct clk *clk, unsigned long rate)
+{
+	int rc = -EINVAL;
+	unsigned long flags;
+
+	spin_lock_irqsave(&clocks_lock, flags);
+
+	while (clk && !IS_ERR(clk)) {
+		if (clk->setrate) {
+			rc = clk->setrate(clk, rate);
+			break;
+		}
+
+		clk = clk->parent;
+	}
+
+	spin_unlock_irqrestore(&clocks_lock, flags);
+
+	return rc;
+}
+EXPORT_SYMBOL(clk_set_rate);
+
+int clk_alloc_function(const char *parent, struct clk *clk)
+{
+	int rc = 0;
+	unsigned long flags;
+	struct clk *pclk;
+	bool found = false;
+
+	spin_lock_irqsave(&clocks_lock, flags);
+
+	list_for_each_entry(pclk, &clocks, node) {
+		if (strcmp(parent, pclk->name) == 0 &&
+		    try_module_get(pclk->owner)) {
+			found = true;
+			break;
+		}
+	}
+
+	if (!found) {
+		rc = -ENODEV;
+		goto out;
+	}
+
+	clk->parent = pclk;
+
+	__clk_register(clk);
+	/*
+	 * We locked parent owner during search
+	 * and also in __clk_register. Free one reference
+	 */
+	module_put(pclk->owner);
+
+out:
+	if (rc) {
+		kfree(clk);
+	}
+	spin_unlock_irqrestore(&clocks_lock, flags);
+
+	return rc;
+}
+EXPORT_SYMBOL(clk_alloc_function);
-- 
1.5.4.4


-- 
With best wishes
Dmitry

[PATCH 2/3] Clocklib: debugfs support

[Dmitry Baryshkov]

Provide /sys/kernel/debug/clock to ease debugging.

Signed-off-by: Dmitry Baryshkov <dbaryshkov@gmail.com>
---
 include/linux/clklib.h |    5 +++
 kernel/clklib.c        |   70 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 75 insertions(+), 0 deletions(-)

diff --git a/include/linux/clklib.h b/include/linux/clklib.h
index 92f0a45..f9c1db4 100644
--- a/include/linux/clklib.h
+++ b/include/linux/clklib.h
@@ -30,6 +30,11 @@ struct clk {
 	int (*setrate)	(struct clk *, unsigned long);
 	long (*roundrate) (struct clk *, unsigned long);
 
+	/*
+	 * format any additional info
+	 */
+	int (*format)	(struct clk *, struct seq_file *);
+
 	void		*priv;
 };
 
diff --git a/kernel/clklib.c b/kernel/clklib.c
index b41e7c2..1c52e55 100644
--- a/kernel/clklib.c
+++ b/kernel/clklib.c
@@ -313,3 +313,73 @@ out:
 	return rc;
 }
 EXPORT_SYMBOL(clk_alloc_function);
+
+#ifdef CONFIG_DEBUG_FS
+
+#include <linux/debugfs.h>
+#include <linux/seq_file.h>
+static void dump_clocks(struct seq_file *s, struct clk *parent, int nest)
+{
+	struct clk *clk;
+	int i;
+
+	list_for_each_entry(clk, &clocks, node) {
+		if (clk->parent == parent) {
+			for (i = 0; i < nest; i++)
+				seq_putc(s, ' ');
+			seq_puts(s, clk->name);
+
+			i = nest + strlen(clk->name);
+			if (i >= 16)
+				i = 15;
+			for (; i < 16; i++) {
+				seq_putc(s, ' ');
+				seq_putc(s, ' ');
+			}
+			seq_printf(s, "%c use=%d rate=%10lu Hz",
+				clk->set_parent ? '*' : ' ',
+				clk->users,
+				__clk_get_rate(clk));
+			if (clk->format)
+				clk->format(clk, s);
+			seq_putc(s, '\n');
+
+			dump_clocks(s, clk, nest + 1);
+		}
+	}
+}
+
+static int clocklib_show(struct seq_file *s, void *unused)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&clocks_lock, flags);
+
+	dump_clocks(s, NULL, 0);
+
+	spin_unlock_irqrestore(&clocks_lock, flags);
+
+	return 0;
+}
+
+static int clocklib_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, clocklib_show, NULL);
+}
+
+static struct file_operations clocklib_operations = {
+	.open		= clocklib_open,
+	.read		= seq_read,
+	.llseek		= seq_lseek,
+	.release	= single_release,
+};
+
+static int __init clocklib_debugfs_init(void)
+{
+	debugfs_create_file("clock", S_IFREG | S_IRUGO,
+				NULL, NULL, &clocklib_operations);
+	return 0;
+}
+subsys_initcall(clocklib_debugfs_init);
+
+#endif /* DEBUG_FS */
-- 
1.5.4.4


-- 
With best wishes
Dmitry

[PATCH 3/3] Clocklib: support sa1100 sub-arch.

[Dmitry Baryshkov]

Signed-off-by: Dmitry Baryshkov <dbaryshkov@gmail.com>
---
 arch/arm/Kconfig             |    1 +
 arch/arm/mach-sa1100/clock.c |   98 +++---------------------------------------
 2 files changed, 7 insertions(+), 92 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 4039a13..6d78a27 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -426,6 +426,7 @@ config ARCH_SA1100
 	select GENERIC_GPIO
 	select GENERIC_TIME
 	select HAVE_IDE
+	select HAVE_CLOCK_LIB
 	help
 	  Support for StrongARM 11x0 based boards.
 
diff --git a/arch/arm/mach-sa1100/clock.c b/arch/arm/mach-sa1100/clock.c
index fc97fe5..e5c4e9f 100644
--- a/arch/arm/mach-sa1100/clock.c
+++ b/arch/arm/mach-sa1100/clock.c
@@ -8,83 +8,13 @@
 #include <linux/err.h>
 #include <linux/string.h>
 #include <linux/clk.h>
+#include <linux/clklib.h>
 #include <linux/spinlock.h>
 #include <linux/mutex.h>
 
 #include <asm/hardware.h>
 
-/*
- * Very simple clock implementation - we only have one clock to
- * deal with at the moment, so we only match using the "name".
- */
-struct clk {
-	struct list_head	node;
-	unsigned long		rate;
-	const char		*name;
-	unsigned int		enabled;
-	void			(*enable)(void);
-	void			(*disable)(void);
-};
-
-static LIST_HEAD(clocks);
-static DEFINE_MUTEX(clocks_mutex);
-static DEFINE_SPINLOCK(clocks_lock);
-
-struct clk *clk_get(struct device *dev, const char *id)
-{
-	struct clk *p, *clk = ERR_PTR(-ENOENT);
-
-	mutex_lock(&clocks_mutex);
-	list_for_each_entry(p, &clocks, node) {
-		if (strcmp(id, p->name) == 0) {
-			clk = p;
-			break;
-		}
-	}
-	mutex_unlock(&clocks_mutex);
-
-	return clk;
-}
-EXPORT_SYMBOL(clk_get);
-
-void clk_put(struct clk *clk)
-{
-}
-EXPORT_SYMBOL(clk_put);
-
-int clk_enable(struct clk *clk)
-{
-	unsigned long flags;
-
-	spin_lock_irqsave(&clocks_lock, flags);
-	if (clk->enabled++ == 0)
-		clk->enable();
-	spin_unlock_irqrestore(&clocks_lock, flags);
-	return 0;
-}
-EXPORT_SYMBOL(clk_enable);
-
-void clk_disable(struct clk *clk)
-{
-	unsigned long flags;
-
-	WARN_ON(clk->enabled == 0);
-
-	spin_lock_irqsave(&clocks_lock, flags);
-	if (--clk->enabled == 0)
-		clk->disable();
-	spin_unlock_irqrestore(&clocks_lock, flags);
-}
-EXPORT_SYMBOL(clk_disable);
-
-unsigned long clk_get_rate(struct clk *clk)
-{
-	return clk->rate;
-}
-EXPORT_SYMBOL(clk_get_rate);
-
-
-static void clk_gpio27_enable(void)
+static int clk_gpio27_enable(struct clk *clk)
 {
 	/*
 	 * First, set up the 3.6864MHz clock on GPIO 27 for the SA-1111:
@@ -93,9 +23,11 @@ static void clk_gpio27_enable(void)
 	GAFR |= GPIO_32_768kHz;
 	GPDR |= GPIO_32_768kHz;
 	TUCR = TUCR_3_6864MHz;
+
+	return 0;
 }
 
-static void clk_gpio27_disable(void)
+static void clk_gpio27_disable(struct clk *clk)
 {
 	TUCR = 0;
 	GPDR &= ~GPIO_32_768kHz;
@@ -109,26 +41,8 @@ static struct clk clk_gpio27 = {
 	.disable	= clk_gpio27_disable,
 };
 
-int clk_register(struct clk *clk)
-{
-	mutex_lock(&clocks_mutex);
-	list_add(&clk->node, &clocks);
-	mutex_unlock(&clocks_mutex);
-	return 0;
-}
-EXPORT_SYMBOL(clk_register);
-
-void clk_unregister(struct clk *clk)
-{
-	mutex_lock(&clocks_mutex);
-	list_del(&clk->node);
-	mutex_unlock(&clocks_mutex);
-}
-EXPORT_SYMBOL(clk_unregister);
-
 static int __init clk_init(void)
 {
-	clk_register(&clk_gpio27);
-	return 0;
+	return clk_register(&clk_gpio27);
 }
 arch_initcall(clk_init);
-- 
1.5.4.4


-- 
With best wishes
Dmitry

Re: [PATCH 1/3] Clocklib: add generic framework for managing clocks.

[Haavard Skinnemoen]

On Wed, 26 Mar 2008 18:52:03 +0300
Dmitry Baryshkov <dbaryshkov@gmail.com> wrote:

> +struct clk {
> +	struct list_head node;
> +	struct clk	*parent;
> +
> +	const char	*name;
> +	struct module	*owner;
> +
> +	int		users;
> +	unsigned long	rate;
> +	int		delay;
> +
> +	int (*can_get)	(struct clk *, struct device *);
> +	int (*set_parent) (struct clk *, struct clk *);
> +	int (*enable)	(struct clk *);
> +	void (*disable)	(struct clk *);
> +	unsigned long (*getrate) (struct clk*);
> +	int (*setrate)	(struct clk *, unsigned long);
> +	long (*roundrate) (struct clk *, unsigned long);
> +
> +	void		*priv;
> +};

Hmm...this is exactly twice as big as the struct I'm currently using,
it doesn't contain all the fields I need, and it's undocumented.

I have quite a few clocks, so the increased memory consumption is quite
significant. What are the advantages of this?

Haavard

Re: [PATCH 1/3] Clocklib: add generic framework for managing clocks.

[Paul Mundt]

On Wed, Mar 26, 2008 at 05:04:41PM +0100, Haavard Skinnemoen wrote:
> On Wed, 26 Mar 2008 18:52:03 +0300
> Dmitry Baryshkov <dbaryshkov@gmail.com> wrote:
> 
> > +struct clk {
> > +	struct list_head node;
> > +	struct clk	*parent;
> > +
> > +	const char	*name;
> > +	struct module	*owner;
> > +
> > +	int		users;
> > +	unsigned long	rate;
> > +	int		delay;
> > +
> > +	int (*can_get)	(struct clk *, struct device *);
> > +	int (*set_parent) (struct clk *, struct clk *);
> > +	int (*enable)	(struct clk *);
> > +	void (*disable)	(struct clk *);
> > +	unsigned long (*getrate) (struct clk*);
> > +	int (*setrate)	(struct clk *, unsigned long);
> > +	long (*roundrate) (struct clk *, unsigned long);
> > +
> > +	void		*priv;
> > +};
> 
> Hmm...this is exactly twice as big as the struct I'm currently using,
> it doesn't contain all the fields I need, and it's undocumented.
> 
Conversely it also has fields that I don't need. If struct clk could have
been done generically without growing to insane sizes, it would have been
done so in linux/clk.h a long time ago. The main thing there is API
consistency for drivers, leaving the details up to the architecture.

It's true that there is significant overlap between the different users
of the clock framework, but it's also not clear that there's any clean
way to share a common implementation (especially since struct clk means
totally different things on different architectures). I suspect everyone
in the CC list has been through this before, also.

[PATCH 4/4] Clocklib: support ARM pxa sub-arch.

[Dmitry Baryshkov]

Signed-off-by: Dmitry Baryshkov <dbaryshkov@gmail.com>
---
 arch/arm/Kconfig           |    1 +
 arch/arm/mach-pxa/clock.c  |  119 +++++--------------------------------------
 arch/arm/mach-pxa/clock.h  |   58 +++++++++++----------
 arch/arm/mach-pxa/pxa25x.c |   67 +++++++++++++++----------
 arch/arm/mach-pxa/pxa27x.c |   64 +++++++++++++-----------
 arch/arm/mach-pxa/pxa3xx.c |   93 +++++++++++++++++++---------------
 6 files changed, 173 insertions(+), 229 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 6d78a27..ce2ffe0 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -402,6 +402,7 @@ config ARCH_PXA
 	select GENERIC_TIME
 	select GENERIC_CLOCKEVENTS
 	select TICK_ONESHOT
+	select HAVE_CLOCK_LIB
 	help
 	  Support for Intel/Marvell's PXA2xx/PXA3xx processor line.
 
diff --git a/arch/arm/mach-pxa/clock.c b/arch/arm/mach-pxa/clock.c
index df5ae27..b050e64 100644
--- a/arch/arm/mach-pxa/clock.c
+++ b/arch/arm/mach-pxa/clock.c
@@ -8,6 +8,7 @@
 #include <linux/err.h>
 #include <linux/string.h>
 #include <linux/clk.h>
+#include <linux/clklib.h>
 #include <linux/spinlock.h>
 #include <linux/platform_device.h>
 #include <linux/delay.h>
@@ -19,135 +20,43 @@
 #include "generic.h"
 #include "clock.h"
 
-static LIST_HEAD(clocks);
-static DEFINE_MUTEX(clocks_mutex);
-static DEFINE_SPINLOCK(clocks_lock);
-
-static struct clk *clk_lookup(struct device *dev, const char *id)
-{
-	struct clk *p;
-
-	list_for_each_entry(p, &clocks, node)
-		if (strcmp(id, p->name) == 0 && p->dev == dev)
-			return p;
-
-	return NULL;
-}
-
-struct clk *clk_get(struct device *dev, const char *id)
-{
-	struct clk *p, *clk = ERR_PTR(-ENOENT);
-
-	mutex_lock(&clocks_mutex);
-	p = clk_lookup(dev, id);
-	if (!p)
-		p = clk_lookup(NULL, id);
-	if (p)
-		clk = p;
-	mutex_unlock(&clocks_mutex);
-
-	return clk;
-}
-EXPORT_SYMBOL(clk_get);
-
-void clk_put(struct clk *clk)
+static int clk_gpio27_enable(struct clk *clk)
 {
-}
-EXPORT_SYMBOL(clk_put);
-
-int clk_enable(struct clk *clk)
-{
-	unsigned long flags;
-
-	spin_lock_irqsave(&clocks_lock, flags);
-	if (clk->enabled++ == 0)
-		clk->ops->enable(clk);
-	spin_unlock_irqrestore(&clocks_lock, flags);
-
-	if (clk->delay)
-		udelay(clk->delay);
+	pxa_gpio_mode(GPIO11_3_6MHz_MD);
 
 	return 0;
 }
-EXPORT_SYMBOL(clk_enable);
-
-void clk_disable(struct clk *clk)
-{
-	unsigned long flags;
-
-	WARN_ON(clk->enabled == 0);
-
-	spin_lock_irqsave(&clocks_lock, flags);
-	if (--clk->enabled == 0)
-		clk->ops->disable(clk);
-	spin_unlock_irqrestore(&clocks_lock, flags);
-}
-EXPORT_SYMBOL(clk_disable);
-
-unsigned long clk_get_rate(struct clk *clk)
-{
-	unsigned long rate;
-
-	rate = clk->rate;
-	if (clk->ops->getrate)
-		rate = clk->ops->getrate(clk);
-
-	return rate;
-}
-EXPORT_SYMBOL(clk_get_rate);
-
-
-static void clk_gpio27_enable(struct clk *clk)
-{
-	pxa_gpio_mode(GPIO11_3_6MHz_MD);
-}
 
 static void clk_gpio27_disable(struct clk *clk)
 {
 }
 
-static const struct clkops clk_gpio27_ops = {
-	.enable		= clk_gpio27_enable,
-	.disable	= clk_gpio27_disable,
-};
-
-
-void clk_cken_enable(struct clk *clk)
+int clk_cken_enable(struct clk *clk)
 {
-	CKEN |= 1 << clk->cken;
+	int cken = ((struct clk_cken_priv *)clk->priv)->cken;
+	CKEN |= 1 << cken;
+
+	return 0;
 }
 
 void clk_cken_disable(struct clk *clk)
 {
-	CKEN &= ~(1 << clk->cken);
+	int cken = ((struct clk_cken_priv *)clk->priv)->cken;
+	CKEN &= ~(1 << cken);
 }
 
-const struct clkops clk_cken_ops = {
-	.enable		= clk_cken_enable,
-	.disable	= clk_cken_disable,
-};
-
 static struct clk common_clks[] = {
 	{
 		.name		= "GPIO27_CLK",
-		.ops		= &clk_gpio27_ops,
 		.rate		= 3686400,
+		.owner		= THIS_MODULE,
+		.enable		= clk_gpio27_enable,
+		.disable	= clk_gpio27_disable,
 	},
 };
 
-void clks_register(struct clk *clks, size_t num)
-{
-	int i;
-
-	mutex_lock(&clocks_mutex);
-	for (i = 0; i < num; i++)
-		list_add(&clks[i].node, &clocks);
-	mutex_unlock(&clocks_mutex);
-}
-
 static int __init clk_init(void)
 {
-	clks_register(common_clks, ARRAY_SIZE(common_clks));
-	return 0;
+	return clks_register(common_clks, ARRAY_SIZE(common_clks));
 }
 arch_initcall(clk_init);
diff --git a/arch/arm/mach-pxa/clock.h b/arch/arm/mach-pxa/clock.h
index bc6b77e..5ad603a 100644
--- a/arch/arm/mach-pxa/clock.h
+++ b/arch/arm/mach-pxa/clock.h
@@ -1,43 +1,45 @@
-struct clk;
+#include <linux/clklib.h>
+#include <linux/seq_file.h>
 
-struct clkops {
-	void			(*enable)(struct clk *);
-	void			(*disable)(struct clk *);
-	unsigned long		(*getrate)(struct clk *);
+struct clk_cken_priv {
+	unsigned int	cken;
 };
 
-struct clk {
-	struct list_head	node;
-	const char		*name;
-	struct device		*dev;
-	const struct clkops	*ops;
-	unsigned long		rate;
-	unsigned int		cken;
-	unsigned int		delay;
-	unsigned int		enabled;
-};
-
-#define INIT_CKEN(_name, _cken, _rate, _delay, _dev)	\
+#define INIT_CKEN(_name, _cken, _rate, _delay)		\
 	{						\
 		.name	= _name,			\
-		.dev	= _dev,				\
-		.ops	= &clk_cken_ops,		\
+		.enable = clk_cken_enable,		\
+		.disable = clk_cken_disable,		\
 		.rate	= _rate,			\
-		.cken	= CKEN_##_cken,			\
 		.delay	= _delay,			\
+		.priv	= &(struct clk_cken_priv) {	\
+			.cken = CKEN_##_cken,		\
+		},					\
 	}
 
-#define INIT_CK(_name, _cken, _ops, _dev)		\
+#define INIT_CK(_name, _cken, _getrate)			\
 	{						\
 		.name	= _name,			\
-		.dev	= _dev,				\
-		.ops	= _ops,				\
-		.cken	= CKEN_##_cken,			\
+		.enable = clk_cken_enable,		\
+		.disable = clk_cken_disable,		\
+		.getrate = _getrate,			\
+		.priv	= &(struct clk_cken_priv) {	\
+			.cken = CKEN_##_cken,		\
+		},					\
 	}
 
-extern const struct clkops clk_cken_ops;
-
-void clk_cken_enable(struct clk *clk);
+int clk_cken_enable(struct clk *clk);
 void clk_cken_disable(struct clk *clk);
 
-void clks_register(struct clk *clks, size_t num);
+static int __maybe_unused clk_dev_can_get(struct clk *clk, struct device *dev)
+{
+	return (dev == clk->priv);
+}
+
+static int __maybe_unused clk_dev_format(struct clk *clk, struct seq_file *s)
+{
+	BUG_ON(!clk->priv);
+	seq_puts(s, " for device ");
+	seq_puts(s, ((struct device *)clk->priv)->bus_id);
+	return 0;
+}
diff --git a/arch/arm/mach-pxa/pxa25x.c b/arch/arm/mach-pxa/pxa25x.c
index 599e53f..1d363d3 100644
--- a/arch/arm/mach-pxa/pxa25x.c
+++ b/arch/arm/mach-pxa/pxa25x.c
@@ -101,40 +101,51 @@ static unsigned long clk_pxa25x_lcd_getrate(struct clk *clk)
 	return pxa25x_get_memclk_frequency_10khz() * 10000;
 }
 
-static const struct clkops clk_pxa25x_lcd_ops = {
-	.enable		= clk_cken_enable,
-	.disable	= clk_cken_disable,
-	.getrate	= clk_pxa25x_lcd_getrate,
-};
-
 /*
  * 3.6864MHz -> OST, GPIO, SSP, PWM, PLLs (95.842MHz, 147.456MHz)
  * 95.842MHz -> MMC 19.169MHz, I2C 31.949MHz, FICP 47.923MHz, USB 47.923MHz
  * 147.456MHz -> UART 14.7456MHz, AC97 12.288MHz, I2S 5.672MHz (allegedly)
  */
-static struct clk pxa25x_hwuart_clk =
-	INIT_CKEN("UARTCLK", HWUART, 14745600, 1, &pxa_device_hwuart.dev)
-;
+static struct clk pxa25x_hwuart_clk[] = {
+	INIT_CKEN("HWUARTCLK", HWUART, 14745600, 1),
+	{
+		.parent =	&pxa25x_hwuart_clk[0],
+		.name	=	"UARTCLK",
+		.can_get =	clk_dev_can_get,
+		.priv	=	&pxa_device_hwuart.dev,
+	},
+};
 
 static struct clk pxa25x_clks[] = {
-	INIT_CK("LCDCLK", LCD, &clk_pxa25x_lcd_ops, &pxa_device_fb.dev),
-	INIT_CKEN("UARTCLK", FFUART, 14745600, 1, &pxa_device_ffuart.dev),
-	INIT_CKEN("UARTCLK", BTUART, 14745600, 1, &pxa_device_btuart.dev),
-	INIT_CKEN("UARTCLK", STUART, 14745600, 1, NULL),
-	INIT_CKEN("UDCCLK", USB, 47923000, 5, &pxa_device_udc.dev),
-	INIT_CKEN("MMCCLK", MMC, 19169000, 0, &pxa_device_mci.dev),
-	INIT_CKEN("I2CCLK", I2C, 31949000, 0, &pxa_device_i2c.dev),
-
-	INIT_CKEN("SSPCLK",  SSP, 3686400, 0, &pxa25x_device_ssp.dev),
-	INIT_CKEN("SSPCLK", NSSP, 3686400, 0, &pxa25x_device_nssp.dev),
-	INIT_CKEN("SSPCLK", ASSP, 3686400, 0, &pxa25x_device_assp.dev),
+	INIT_CK("LCDCLK", LCD, &clk_pxa25x_lcd_getrate),
+	INIT_CKEN("FFUARTCLK", FFUART, 14745600, 1),
+	INIT_CKEN("BTUARTCLK", BTUART, 14745600, 1),
+	INIT_CKEN("STUARTCLK", STUART, 14745600, 1),
+	INIT_CKEN("UDCCLK", USB, 47923000, 5),
+	INIT_CKEN("PXAMMCCLK", MMC, 19169000, 0),
+	INIT_CKEN("I2CCLK", I2C, 31949000, 0),
+
+	INIT_CKEN("SSP_CLK",  SSP, 3686400, 0),
+	INIT_CKEN("NSSPCLK", NSSP, 3686400, 0),
+	INIT_CKEN("ASSPCLK", ASSP, 3686400, 0),
 
 	/*
-	INIT_CKEN("PWMCLK",  PWM0, 3686400,  0, NULL),
-	INIT_CKEN("PWMCLK",  PWM0, 3686400,  0, NULL),
-	INIT_CKEN("I2SCLK",  I2S,  14745600, 0, NULL),
+	INIT_CKEN("PWMCLK",  PWM0, 3686400,  0),
+	INIT_CKEN("PWMCLK",  PWM0, 3686400,  0),
+	INIT_CKEN("I2SCLK",  I2S,  14745600, 0),
 	*/
-	INIT_CKEN("FICPCLK", FICP, 47923000, 0, NULL),
+	INIT_CKEN("FICPCLK", FICP, 47923000, 0),
+};
+
+static struct clk_function __initdata pxa25x_clk_funcs[] = {
+	CLK_FUNC("FFUARTCLK", "UARTCLK", clk_dev_can_get, &pxa_device_ffuart.dev, clk_dev_format),
+	CLK_FUNC("BTUARTCLK", "UARTCLK", clk_dev_can_get, &pxa_device_btuart.dev, clk_dev_format),
+	CLK_FUNC("STUARTCLK", "UARTCLK", clk_dev_can_get, &pxa_device_stuart.dev, clk_dev_format),
+	CLK_FUNC("STUARTCLK", "SIRCLK", NULL, NULL, NULL),
+	CLK_FUNC("PXAMMCCLK", "MMCCLK", clk_dev_can_get, &pxa_device_mci.dev, clk_dev_format),
+	CLK_FUNC("SSP_CLK", "SSPCLK", clk_dev_can_get, &pxa25x_device_ssp.dev, clk_dev_format),
+	CLK_FUNC("NSSPCLK", "SSPCLK", clk_dev_can_get, &pxa25x_device_nssp.dev, clk_dev_format),
+	CLK_FUNC("ASSPCLK", "SSPCLK", clk_dev_can_get, &pxa25x_device_assp.dev, clk_dev_format),
 };
 
 #ifdef CONFIG_PM
@@ -295,11 +306,13 @@ static int __init pxa25x_init(void)
 	int i, ret = 0;
 
 	/* Only add HWUART for PXA255/26x; PXA210/250/27x do not have it. */
-	if (cpu_is_pxa25x())
-		clks_register(&pxa25x_hwuart_clk, 1);
+	if (cpu_is_pxa25x()) {
+		ret = clks_register(pxa25x_hwuart_clk, ARRAY_SIZE(pxa25x_hwuart_clk));
+	}
 
 	if (cpu_is_pxa21x() || cpu_is_pxa25x()) {
-		clks_register(pxa25x_clks, ARRAY_SIZE(pxa25x_clks));
+		ret = clks_register(pxa25x_clks, ARRAY_SIZE(pxa25x_clks));
+		ret = clk_alloc_functions(pxa25x_clk_funcs, ARRAY_SIZE(pxa25x_clk_funcs));
 
 		if ((ret = pxa_init_dma(16)))
 			return ret;
diff --git a/arch/arm/mach-pxa/pxa27x.c b/arch/arm/mach-pxa/pxa27x.c
index 46a951c..304445c 100644
--- a/arch/arm/mach-pxa/pxa27x.c
+++ b/arch/arm/mach-pxa/pxa27x.c
@@ -129,44 +129,49 @@ static unsigned long clk_pxa27x_lcd_getrate(struct clk *clk)
 	return pxa27x_get_lcdclk_frequency_10khz() * 10000;
 }
 
-static const struct clkops clk_pxa27x_lcd_ops = {
-	.enable		= clk_cken_enable,
-	.disable	= clk_cken_disable,
-	.getrate	= clk_pxa27x_lcd_getrate,
-};
-
 static struct clk pxa27x_clks[] = {
-	INIT_CK("LCDCLK", LCD,    &clk_pxa27x_lcd_ops, &pxa_device_fb.dev),
-	INIT_CK("CAMCLK", CAMERA, &clk_pxa27x_lcd_ops, NULL),
+	INIT_CK("LCDCLK", LCD,    &clk_pxa27x_lcd_getrate),
+	INIT_CK("CAMCLK", CAMERA, &clk_pxa27x_lcd_getrate),
 
-	INIT_CKEN("UARTCLK", FFUART, 14857000, 1, &pxa_device_ffuart.dev),
-	INIT_CKEN("UARTCLK", BTUART, 14857000, 1, &pxa_device_btuart.dev),
-	INIT_CKEN("UARTCLK", STUART, 14857000, 1, NULL),
+	INIT_CKEN("FFUARTCLK", FFUART, 14857000, 1),
+	INIT_CKEN("BTUARTCLK", BTUART, 14857000, 1),
+	INIT_CKEN("STUARTCLK", STUART, 14857000, 1),
 
-	INIT_CKEN("I2SCLK",  I2S,  14682000, 0, &pxa_device_i2s.dev),
-	INIT_CKEN("I2CCLK",  I2C,  32842000, 0, &pxa_device_i2c.dev),
-	INIT_CKEN("UDCCLK",  USB,  48000000, 5, &pxa_device_udc.dev),
-	INIT_CKEN("MMCCLK",  MMC,  19500000, 0, &pxa_device_mci.dev),
-	INIT_CKEN("FICPCLK", FICP, 48000000, 0, &pxa_device_ficp.dev),
+	INIT_CKEN("I2SCLK",  I2S,  14682000, 0),
+	INIT_CKEN("I2CCLK",  I2C,  32842000, 0),
+	INIT_CKEN("UDCCLK",  USB,  48000000, 5),
+	INIT_CKEN("PXAMMCCLK",  MMC,  19500000, 0),
+	INIT_CKEN("FICPCLK", FICP, 48000000, 0),
 
-	INIT_CKEN("USBCLK", USBHOST, 48000000, 0, &pxa27x_device_ohci.dev),
-	INIT_CKEN("I2CCLK", PWRI2C, 13000000, 0, &pxa27x_device_i2c_power.dev),
-	INIT_CKEN("KBDCLK", KEYPAD, 32768, 0, NULL),
+	INIT_CKEN("USBCLK", USBHOST, 48000000, 0),
+	INIT_CKEN("I2CCLK", PWRI2C, 13000000, 0),
+	INIT_CKEN("KBDCLK", KEYPAD, 32768, 0),
 
-	INIT_CKEN("SSPCLK", SSP1, 13000000, 0, &pxa27x_device_ssp1.dev),
-	INIT_CKEN("SSPCLK", SSP2, 13000000, 0, &pxa27x_device_ssp2.dev),
-	INIT_CKEN("SSPCLK", SSP3, 13000000, 0, &pxa27x_device_ssp3.dev),
+	INIT_CKEN("SSP1CLK", SSP1, 13000000, 0),
+	INIT_CKEN("SSP2CLK", SSP2, 13000000, 0),
+	INIT_CKEN("SSP3CLK", SSP3, 13000000, 0),
 
 	/*
-	INIT_CKEN("PWMCLK",  PWM0, 13000000, 0, NULL),
-	INIT_CKEN("MSLCLK",  MSL,  48000000, 0, NULL),
-	INIT_CKEN("USIMCLK", USIM, 48000000, 0, NULL),
-	INIT_CKEN("MSTKCLK", MEMSTK, 19500000, 0, NULL),
-	INIT_CKEN("IMCLK",   IM,   0, 0, NULL),
-	INIT_CKEN("MEMCLK",  MEMC, 0, 0, NULL),
+	INIT_CKEN("PWMCLK",  PWM0, 13000000, 0),
+	INIT_CKEN("MSLCLK",  MSL,  48000000, 0),
+	INIT_CKEN("USIMCLK", USIM, 48000000, 0),
+	INIT_CKEN("MSTKCLK", MEMSTK, 19500000, 0),
+	INIT_CKEN("IMCLK",   IM,   0, 0),
+	INIT_CKEN("MEMCLK",  MEMC, 0, 0),
 	*/
 };
 
+static struct clk_function __initdata pxa27x_clk_funcs[] = {
+	CLK_FUNC("FFUARTCLK", "UARTCLK", clk_dev_can_get, &pxa_device_ffuart.dev, clk_dev_format),
+	CLK_FUNC("BTUARTCLK", "UARTCLK", clk_dev_can_get, &pxa_device_btuart.dev, clk_dev_format),
+	CLK_FUNC("STUARTCLK", "UARTCLK", clk_dev_can_get, &pxa_device_stuart.dev, clk_dev_format),
+	CLK_FUNC("STUARTCLK", "SIRCLK", NULL, NULL, NULL),
+	CLK_FUNC("PXAMMCCLK", "MMCCLK", clk_dev_can_get, &pxa_device_mci.dev, clk_dev_format),
+	CLK_FUNC("SSP1CLK", "SSPCLK", clk_dev_can_get, &pxa27x_device_ssp1.dev, clk_dev_format),
+	CLK_FUNC("SSP2CLK", "SSPCLK", clk_dev_can_get, &pxa27x_device_ssp2.dev, clk_dev_format),
+	CLK_FUNC("SSP3CLK", "SSPCLK", clk_dev_can_get, &pxa27x_device_ssp3.dev, clk_dev_format),
+};
+
 #ifdef CONFIG_PM
 
 #define SAVE(x)		sleep_save[SLEEP_SAVE_##x] = x
@@ -401,7 +406,8 @@ static int __init pxa27x_init(void)
 	int i, ret = 0;
 
 	if (cpu_is_pxa27x()) {
-		clks_register(pxa27x_clks, ARRAY_SIZE(pxa27x_clks));
+		ret = clks_register(pxa27x_clks, ARRAY_SIZE(pxa27x_clks));
+		ret = clk_alloc_functions(pxa27x_clk_funcs, ARRAY_SIZE(pxa27x_clk_funcs));
 
 		if ((ret = pxa_init_dma(32)))
 			return ret;
diff --git a/arch/arm/mach-pxa/pxa3xx.c b/arch/arm/mach-pxa/pxa3xx.c
index 35f25fd..8b0b80e 100644
--- a/arch/arm/mach-pxa/pxa3xx.c
+++ b/arch/arm/mach-pxa/pxa3xx.c
@@ -125,75 +125,87 @@ static unsigned long clk_pxa3xx_hsio_getrate(struct clk *clk)
 	return hsio_clk;
 }
 
-static void clk_pxa3xx_cken_enable(struct clk *clk)
+static int clk_pxa3xx_cken_enable(struct clk *clk)
 {
-	unsigned long mask = 1ul << (clk->cken & 0x1f);
+	int cken = ((struct clk_cken_priv *)clk->priv)->cken;
+	unsigned long mask = 1ul << (cken & 0x1f);
 
-	if (clk->cken < 32)
+	if (cken < 32)
 		CKENA |= mask;
 	else
 		CKENB |= mask;
+
+	return 0;
 }
 
 static void clk_pxa3xx_cken_disable(struct clk *clk)
 {
-	unsigned long mask = 1ul << (clk->cken & 0x1f);
+	int cken = ((struct clk_cken_priv *)clk->priv)->cken;
+	unsigned long mask = 1ul << (cken & 0x1f);
 
-	if (clk->cken < 32)
+	if (cken < 32)
 		CKENA &= ~mask;
 	else
 		CKENB &= ~mask;
 }
 
-static const struct clkops clk_pxa3xx_cken_ops = {
-	.enable		= clk_pxa3xx_cken_enable,
-	.disable	= clk_pxa3xx_cken_disable,
-};
-
-static const struct clkops clk_pxa3xx_hsio_ops = {
-	.enable		= clk_pxa3xx_cken_enable,
-	.disable	= clk_pxa3xx_cken_disable,
-	.getrate	= clk_pxa3xx_hsio_getrate,
-};
-
-#define PXA3xx_CKEN(_name, _cken, _rate, _delay, _dev)	\
+#define PXA3xx_CKEN(_name, _cken, _rate, _delay)	\
 	{						\
 		.name	= _name,			\
-		.dev	= _dev,				\
-		.ops	= &clk_pxa3xx_cken_ops,		\
+		.enable = clk_pxa3xx_cken_enable,	\
+		.disable = clk_pxa3xx_cken_disable,	\
 		.rate	= _rate,			\
-		.cken	= CKEN_##_cken,			\
 		.delay	= _delay,			\
+		.priv	= &(struct clk_cken_priv) {	\
+			.cken = CKEN_##_cken,		\
+		},					\
 	}
 
-#define PXA3xx_CK(_name, _cken, _ops, _dev)		\
+#define PXA3xx_CK(_name, _cken, _getrate)		\
 	{						\
 		.name	= _name,			\
-		.dev	= _dev,				\
-		.ops	= _ops,				\
-		.cken	= CKEN_##_cken,			\
+		.enable = clk_pxa3xx_cken_enable,	\
+		.disable = clk_pxa3xx_cken_disable,	\
+		.getrate = _getrate,			\
+		.priv	= &(struct clk_cken_priv) {	\
+			.cken = CKEN_##_cken,		\
+		},					\
 	}
 
 static struct clk pxa3xx_clks[] = {
-	PXA3xx_CK("LCDCLK", LCD,    &clk_pxa3xx_hsio_ops, &pxa_device_fb.dev),
-	PXA3xx_CK("CAMCLK", CAMERA, &clk_pxa3xx_hsio_ops, NULL),
+	PXA3xx_CK("LCDCLK", LCD,    &clk_pxa3xx_hsio_getrate),
+	PXA3xx_CK("CAMCLK", CAMERA, &clk_pxa3xx_hsio_getrate),
 
-	PXA3xx_CKEN("UARTCLK", FFUART, 14857000, 1, &pxa_device_ffuart.dev),
-	PXA3xx_CKEN("UARTCLK", BTUART, 14857000, 1, &pxa_device_btuart.dev),
-	PXA3xx_CKEN("UARTCLK", STUART, 14857000, 1, NULL),
+	PXA3xx_CKEN("FFUARTCLK", FFUART, 14857000, 1),
+	PXA3xx_CKEN("BTUARTCLK", BTUART, 14857000, 1),
+	PXA3xx_CKEN("STUARTCLK", STUART, 14857000, 1),
 
-	PXA3xx_CKEN("I2CCLK", I2C,  32842000, 0, &pxa_device_i2c.dev),
-	PXA3xx_CKEN("UDCCLK", UDC,  48000000, 5, &pxa_device_udc.dev),
-	PXA3xx_CKEN("USBCLK", USBH, 48000000, 0, &pxa27x_device_ohci.dev),
+	PXA3xx_CKEN("I2CCLK", I2C,  32842000, 0),
+	PXA3xx_CKEN("UDCCLK", UDC,  48000000, 5),
+	PXA3xx_CKEN("USBCLK", USBH, 48000000, 0),
 
-	PXA3xx_CKEN("SSPCLK", SSP1, 13000000, 0, &pxa27x_device_ssp1.dev),
-	PXA3xx_CKEN("SSPCLK", SSP2, 13000000, 0, &pxa27x_device_ssp2.dev),
-	PXA3xx_CKEN("SSPCLK", SSP3, 13000000, 0, &pxa27x_device_ssp3.dev),
-	PXA3xx_CKEN("SSPCLK", SSP4, 13000000, 0, &pxa3xx_device_ssp4.dev),
+	PXA3xx_CKEN("SSP1CLK", SSP1, 13000000, 0),
+	PXA3xx_CKEN("SSP2CLK", SSP2, 13000000, 0),
+	PXA3xx_CKEN("SSP3CLK", SSP3, 13000000, 0),
+	PXA3xx_CKEN("SSP4CLK", SSP4, 13000000, 0),
+
+	PXA3xx_CKEN("MMC1CLK", MMC1, 19500000, 0),
+	PXA3xx_CKEN("MMC2CLK", MMC2, 19500000, 0),
+	PXA3xx_CKEN("MMC3CLK", MMC3, 19500000, 0),
+};
 
-	PXA3xx_CKEN("MMCCLK", MMC1, 19500000, 0, &pxa_device_mci.dev),
-	PXA3xx_CKEN("MMCCLK", MMC2, 19500000, 0, &pxa3xx_device_mci2.dev),
-	PXA3xx_CKEN("MMCCLK", MMC3, 19500000, 0, &pxa3xx_device_mci3.dev),
+static struct clk_function __initdata pxa3xx_clk_funcs[] = {
+	CLK_FUNC("FFUARTCLK", "UARTCLK", clk_dev_can_get, &pxa_device_ffuart.dev, clk_dev_format),
+	CLK_FUNC("BTUARTCLK", "UARTCLK", clk_dev_can_get, &pxa_device_btuart.dev, clk_dev_format),
+	CLK_FUNC("STUARTCLK", "UARTCLK", clk_dev_can_get, &pxa_device_stuart.dev, clk_dev_format),
+	CLK_FUNC("STUARTCLK", "SIRCLK", NULL, NULL, NULL),
+	CLK_FUNC("SSP1CLK", "SSPCLK", clk_dev_can_get, &pxa27x_device_ssp1.dev, clk_dev_format),
+	CLK_FUNC("SSP2CLK", "SSPCLK", clk_dev_can_get, &pxa27x_device_ssp2.dev, clk_dev_format),
+	CLK_FUNC("SSP3CLK", "SSPCLK", clk_dev_can_get, &pxa27x_device_ssp3.dev, clk_dev_format),
+	CLK_FUNC("SSP4CLK", "SSPCLK", clk_dev_can_get, &pxa3xx_device_ssp4.dev, clk_dev_format),
+	CLK_FUNC("MMC1CLK", "MMCCLK", clk_dev_can_get, &pxa_device_mci.dev, clk_dev_format),
+	CLK_FUNC("MMC2CLK", "MMCCLK", clk_dev_can_get, &pxa3xx_device_mci2.dev, clk_dev_format),
+	CLK_FUNC("MMC3CLK", "MMCCLK", clk_dev_can_get, &pxa3xx_device_mci3.dev, clk_dev_format),
 };
 
 #ifdef CONFIG_PM
@@ -513,7 +525,8 @@ static int __init pxa3xx_init(void)
 		 */
 		ASCR &= ~(ASCR_RDH | ASCR_D1S | ASCR_D2S | ASCR_D3S);
 
-		clks_register(pxa3xx_clks, ARRAY_SIZE(pxa3xx_clks));
+		ret = clks_register(pxa3xx_clks, ARRAY_SIZE(pxa3xx_clks));
+		ret = clk_alloc_functions(pxa3xx_clk_funcs, ARRAY_SIZE(pxa3xx_clk_funcs));
 
 		if ((ret = pxa_init_dma(32)))
 			return ret;
-- 
1.5.4.4


-- 
With best wishes
Dmitry

Re: [PATCH 1/3] Clocklib: add generic framework for managing clocks.

[Dmitry]

Hi,

2008/3/26, Haavard Skinnemoen <haavard.skinnemoen@atmel.com>:
> On Wed, 26 Mar 2008 18:52:03 +0300
>  Dmitry Baryshkov <dbaryshkov@gmail.com> wrote:
>
>  > +struct clk {
>  > +     struct list_head node;
>  > +     struct clk      *parent;
>  > +
>  > +     const char      *name;
>  > +     struct module   *owner;
>  > +
>  > +     int             users;
>  > +     unsigned long   rate;
>  > +     int             delay;
>  > +
>  > +     int (*can_get)  (struct clk *, struct device *);
>  > +     int (*set_parent) (struct clk *, struct clk *);
>  > +     int (*enable)   (struct clk *);
>  > +     void (*disable) (struct clk *);
>  > +     unsigned long (*getrate) (struct clk*);
>  > +     int (*setrate)  (struct clk *, unsigned long);
>  > +     long (*roundrate) (struct clk *, unsigned long);
>  > +
>  > +     void            *priv;
>  > +};
>
>
> Hmm...this is exactly twice as big as the struct I'm currently using,
>  it doesn't contain all the fields I need, and it's undocumented.

I've added a more sofisticated arch convertion patch (the clocklib for
ARM PXA chips).

Basically mode becomes enable/disable (however it may be better to merge back
those pointers into one function). And dev and index go to priv data.

The documentation will come later.

>
>  I have quite a few clocks, so the increased memory consumption is quite
>  significant. What are the advantages of this?

At maximum 55, IIUC. I counted 32 or so additional bytes in the struct
(over avr32-specific one). That would count up to 1.5 K overhead. Is
that really too much for current kernels?


OTOH this would bring unification of platform code, allow
configurations when a non-platform driver would provide it's own
clocks (think about multi-function companion chips when there is a
"core" which manages "clocks" for it's "periferal" devices. Currently
if one tries to implement such driver, he is forced to either bind it
to platform code, or to implement non-standard
my_device_clock_enable()-like functions.

Also you aren't forced to use this API. simply don't select
HAVE_CLOCK_LIB and leave
all things as they are. E.g. gpiolib is now merged, however not all
gpio-providing platforms
are using it.

-- 
With best wishes
Dmitry

Re: [PATCH 1/3] Clocklib: add generic framework for managing clocks.

[Dmitry]

Hi,

2008/3/26, Paul Mundt <lethal@linux-sh.org>:
> On Wed, Mar 26, 2008 at 05:04:41PM +0100, Haavard Skinnemoen wrote:
>  > On Wed, 26 Mar 2008 18:52:03 +0300
>  > Dmitry Baryshkov <dbaryshkov@gmail.com> wrote:
>  >
>  > > +struct clk {
>  > > +   struct list_head node;
>  > > +   struct clk      *parent;
>  > > +
>  > > +   const char      *name;
>  > > +   struct module   *owner;
>  > > +
>  > > +   int             users;
>  > > +   unsigned long   rate;
>  > > +   int             delay;
>  > > +
>  > > +   int (*can_get)  (struct clk *, struct device *);
>  > > +   int (*set_parent) (struct clk *, struct clk *);
>  > > +   int (*enable)   (struct clk *);
>  > > +   void (*disable) (struct clk *);
>  > > +   unsigned long (*getrate) (struct clk*);
>  > > +   int (*setrate)  (struct clk *, unsigned long);
>  > > +   long (*roundrate) (struct clk *, unsigned long);
>  > > +
>  > > +   void            *priv;
>  > > +};
>  >
>  > Hmm...this is exactly twice as big as the struct I'm currently using,
>  > it doesn't contain all the fields I need, and it's undocumented.
>  >
>
> Conversely it also has fields that I don't need. If struct clk could have
>  been done generically without growing to insane sizes, it would have been
>  done so in linux/clk.h a long time ago. The main thing there is API
>  consistency for drivers, leaving the details up to the architecture.

In reality, as noted David Brownell on LAKML, there is no API consistency.
The driver has no way to know whether clk API is implemented at all or whether
the "optional" functions do exist.

Moreover clocks aren't limited only to platform-specific code. As an
example of the in-tree driver that will benefit from clocklib you can
check drivers/mfd/sm501.c which  contains  it's own set of functions
to manage clocks.

>  It's true that there is significant overlap between the different users
>  of the clock framework, but it's also not clear that there's any clean
>  way to share a common implementation (especially since struct clk means
>  totally different things on different architectures). I suspect everyone
>  in the CC list has been through this before, also.

That's one of the initial reasons of this patchserie: I have a device
that can be installed on several platforms. And I want for this device
to provide clocks to some other devices.

Simply saying "Oh, things are different" is behaving like a ostrich:
hiding a head in the sand.

As most generification patches do, this one has it's drawbacks, but
they are not so big, as you describe.

-- 
With best wishes
Dmitry

Re: [PATCH 1/3] Clocklib: add generic framework for managing clocks.

[Paul Mundt]

On Wed, Mar 26, 2008 at 07:52:34PM +0300, Dmitry wrote:
> 2008/3/26, Haavard Skinnemoen <haavard.skinnemoen@atmel.com>:
> > Hmm...this is exactly twice as big as the struct I'm currently using,
> >  it doesn't contain all the fields I need, and it's undocumented.
> 
> I've added a more sofisticated arch convertion patch (the clocklib for
> ARM PXA chips).
> 
What you've converted is still pretty tame in comparison to what this
sort of framework has to handle. Something like the OMAP struct clk is
more like a worst-case and is representative of how large this structure
will get for everyone if it is to be shared without dropping
functionality.

> >  I have quite a few clocks, so the increased memory consumption is quite
> >  significant. What are the advantages of this?
> 
> At maximum 55, IIUC. I counted 32 or so additional bytes in the struct
> (over avr32-specific one). That would count up to 1.5 K overhead. Is
> that really too much for current kernels?
> 
Your idea of maximum and my hardware's idea of maximum have very little
in common. Most of these platforms are dealing with hundreds of different
clocks, though they are not necessarily all interfaced in software
control (mostly because a lot of them auto-gate, and because writing up
struct clk definitions for 200+ clocks for 30 different CPUs isn't
exactly my idea of a good time). This does not mean that more clocks will
not be hooked up as the need arises.

On Wed, Mar 26, 2008 at 08:04:41PM +0300, Dmitry wrote:
> 2008/3/26, Paul Mundt <lethal@linux-sh.org>:
> > Conversely it also has fields that I don't need. If struct clk could have
> >  been done generically without growing to insane sizes, it would have been
> >  done so in linux/clk.h a long time ago. The main thing there is API
> >  consistency for drivers, leaving the details up to the architecture.
> 
> In reality, as noted David Brownell on LAKML, there is no API consistency.
> The driver has no way to know whether clk API is implemented at all or whether
> the "optional" functions do exist.
> 
There is certainly API consistency. If there were not, we wouldn't have
linux/clk.h. The fact that many platforms add on top of this does not
detract from the fact that we have a common API that is currently shared.

There is indeed no way to know what optional functionality exists, but
your proposed structure and interface largely works around that problem
by ignoring all of the optional functionality. This works great for an
idealized set of platforms and clock configurations, but quickly runs in
to the same issue that linux/clk.h has today. It's not clear how your
proposed interface helps any of this other than providing a struct clk
that can be used more generically.

> Moreover clocks aren't limited only to platform-specific code. As an
> example of the in-tree driver that will benefit from clocklib you can
> check drivers/mfd/sm501.c which  contains  it's own set of functions
> to manage clocks.
> 
That's certainly true, it's definitely worthwhile to try and unify as
much of the interface as possible. Perhaps it makes more sense to try and
have a common struct clk with the bare essentials and then allow the
platforms to extend that functionality based on their own capabilities.
This could be done through the private data I suppose.

> That's one of the initial reasons of this patchserie: I have a device
> that can be installed on several platforms. And I want for this device
> to provide clocks to some other devices.
> 
I don't disagree with your intentions, or that something like this would
be a good idea. What needs to happen first is having a look at all of the
different versions of the clock framework that are out there and coming
up with a consolidated struct clk, then seeing if the resulting size is
practical enough to actually use for any significant number of clocks.

Your current definition doesn't meet the requirements of all of the
struct clk users in the kernel, and it's already getting quite big
compared to what people (avr32, sh, some ARM platforms, etc.) are using
today. This is a good indicator of why a common definition wasn't a good
fit in the first place.

Re: [PATCH 1/3] Clocklib: add generic framework for managing clocks.

[Juergen Beisert]

On Wednesday 26 March 2008 17:52, Dmitry wrote:
>[...]
> >
> > Hmm...this is exactly twice as big as the struct I'm currently using,
> >  it doesn't contain all the fields I need, and it's undocumented.
>
> I've added a more sofisticated arch convertion patch (the clocklib for
> ARM PXA chips).
>
> Basically mode becomes enable/disable (however it may be better to merge
> back those pointers into one function). And dev and index go to priv data.
>
> The documentation will come later.
                              ^^^^^
Most of the time this means "never".

JB

Re: [PATCH 1/3] Clocklib: add generic framework for managing clocks.

[Russell King]

On Thu, Mar 27, 2008 at 01:14:56AM +0900, Paul Mundt wrote:
> Conversely it also has fields that I don't need. If struct clk could have
> been done generically without growing to insane sizes, it would have been
> done so in linux/clk.h a long time ago. The main thing there is API
> consistency for drivers, leaving the details up to the architecture.
> 
> It's true that there is significant overlap between the different users
> of the clock framework, but it's also not clear that there's any clean
> way to share a common implementation (especially since struct clk means
> totally different things on different architectures). I suspect everyone
> in the CC list has been through this before, also.

That's the exact reason why I never implemented any kind of framework
and just left it as an API for drivers to use.  What's behind the API
is very platform specific, and as can be seen from the comments, trying
to define something common results in something that just doesn't fit
in different ways.

Trying to make it expand to fit someone elses platform makes it unsuitable
for another due to it becoming too heavy weight.

Personally, I don't have much interest in these patches - had I been
interested in having a common framework behind it when I created the
API, I'd have written some code.

However, if folk think that they can solve the complexity problem while
still allowing for simple implementations...

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: [PATCH 1/3] Clocklib: add generic framework for managing clocks.

[Haavard Skinnemoen]

On Wed, 26 Mar 2008 19:52:34 +0300
Dmitry <dbaryshkov@gmail.com> wrote:

> Basically mode becomes enable/disable (however it may be better to merge back
> those pointers into one function). And dev and index go to priv data.

I think it would definitely help to combine some hooks into one.
enable/disable is one example, set_rate/round_rate is another.

> The documentation will come later.

Hmm. Missing documentation makes it harder to review this stuff...

> >  I have quite a few clocks, so the increased memory consumption is quite
> >  significant. What are the advantages of this?  
> 
> At maximum 55, IIUC. I counted 32 or so additional bytes in the struct
> (over avr32-specific one). That would count up to 1.5 K overhead. Is
> that really too much for current kernels?

If the only advantage is less code duplication, I'd say it's too much.
However...

> OTOH this would bring unification of platform code, allow
> configurations when a non-platform driver would provide it's own
> clocks (think about multi-function companion chips when there is a
> "core" which manages "clocks" for it's "periferal" devices. Currently
> if one tries to implement such driver, he is forced to either bind it
> to platform code, or to implement non-standard
> my_device_clock_enable()-like functions.

...that is a good argument. External clock generators come to
mind...they can even be parents for other clocks.

> Also you aren't forced to use this API. simply don't select
> HAVE_CLOCK_LIB and leave
> all things as they are. E.g. gpiolib is now merged, however not all
> gpio-providing platforms
> are using it.

Sure, but then I won't be able to use the suggested drivers that
depend on this stuff.

How about we try to slim things down a bit? Let's see...

> +struct clk {
> +	struct list_head node;
> +	struct clk	*parent;
> +
> +	const char	*name;

I guess these are always required if we're going to do dynamic
registration...

> +	struct module	*owner;

This will probably be unused for most platforms, but I guess we need it
to get the advantage you're talking about.

> +	int		users;

Reference counting, probably need that too.

> +	unsigned long	rate;

This one is redundant. Use getrate() instead.

> +	int		delay;

Huh? A delay after enabling the clock? Why can't the enable() hook do
that if it's really necessary?

> +	int (*can_get)	(struct clk *, struct device *);

What's this for? I'm assuming it's necessary to couple clocks to
specific devices?

> +	int (*set_parent) (struct clk *, struct clk *);

We probably need this.

> +	int (*enable)	(struct clk *);
> +	void (*disable)	(struct clk *);

Combine these into "mode" or something (with an extra parameter)

> +	unsigned long (*getrate) (struct clk*);

We need this one.

> +	int (*setrate)	(struct clk *, unsigned long);
> +	long (*roundrate) (struct clk *, unsigned long);

Combine these into one call with an extra "apply" parameter or
something. The underlying logic is pretty much the same apart from the
"actually write stuff to registers" step.

> +	void		*priv;

Remove this and let platforms extend the struct instead. They can use
container_of() to access the extra fields, which is faster too.

> +};

The result is something like this:

struct clk {
	struct list_head node;
	struct clk	*parent;

	const char	*name;
	struct module	*owner;

	int		users;

	int (*can_get)	(struct clk *, struct device *);
	int (*set_parent) (struct clk *, struct clk *);
	int (*mode)	(struct clk *, bool enabled);
	unsigned long (*getrate) (struct clk*);
	int (*setrate)	(struct clk *, unsigned long, bool apply);
};

which is 44 bytes, 12 bytes more than the avr32 version. That can
probably be explained by the "node" and "owner" fields, which really
are necessary in order to support dynamic registration of clocks.

Adding the "dev" and "index" fields takes us to 52 bytes, or 20 bytes
more. That's about 1.1K extra in total for 55 clocks, which is still a
bit much, but I can probably live with that.

Haavard

Re: [PATCH 1/3] Clocklib: add generic framework for managing clocks.

[Russell King]

On Thu, Mar 27, 2008 at 10:06:23AM +0100, Haavard Skinnemoen wrote:
> > +	int		users;
> 
> Reference counting, probably need that too.
> 
> > +	unsigned long	rate;
> 
> This one is redundant. Use getrate() instead.

... which means a separate getrate() functions for every clock.  Not really
practical for PXA.

> > +	int		delay;
> 
> Huh? A delay after enabling the clock? Why can't the enable() hook do
> that if it's really necessary?

... which means a separate enable() function for each clock.  The delay
there has not a lot to do with the actual register you're frobbing, but
the resy of the SoC.  So, again, not really practical for PXA.

The result for PXA is a tradeoff between reducing the data size and
increasing the text size, or increasing the data size and keeping
the existing data size.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: [PATCH 1/3] Clocklib: add generic framework for managing clocks.

[Haavard Skinnemoen]

On Thu, 27 Mar 2008 09:18:10 +0000
Russell King <rmk+lkml@arm.linux.org.uk> wrote:

> On Thu, Mar 27, 2008 at 10:06:23AM +0100, Haavard Skinnemoen wrote:
> > > +	int		users;
> > 
> > Reference counting, probably need that too.
> > 
> > > +	unsigned long	rate;
> > 
> > This one is redundant. Use getrate() instead.
> 
> ... which means a separate getrate() functions for every clock.  Not really
> practical for PXA.

You can extend the struct, put the rate there and use the same
getrate() function for all the clocks that need to keep track of the
current rate this way.

> > > +	int		delay;
> > 
> > Huh? A delay after enabling the clock? Why can't the enable() hook do
> > that if it's really necessary?
> 
> ... which means a separate enable() function for each clock.  The delay
> there has not a lot to do with the actual register you're frobbing, but
> the resy of the SoC.  So, again, not really practical for PXA.

Same thing, extend the struct and use the same enable() function for
all the clocks that need this delay. We can't add fields to the generic
struct clk for every platform quirk out there...

Haavard

Re: [PATCH 1/3] Clocklib: add generic framework for managing clocks.

[pHilipp Zabel]

On Thu, Mar 27, 2008 at 10:18 AM, Russell King
<rmk+lkml@arm.linux.org.uk> wrote:
> On Thu, Mar 27, 2008 at 10:06:23AM +0100, Haavard Skinnemoen wrote:
>  > > +   int             users;
>  >
>  > Reference counting, probably need that too.
>  >
>  > > +   unsigned long   rate;
>  >
>  > This one is redundant. Use getrate() instead.
>
>  ... which means a separate getrate() functions for every clock.  Not really
>  practical for PXA.

struct pxa_clk {
       struct clk generic_clk;
       int rate;
       int delay;
}

unsigned long pxa_clk_getrate (struct clk *clk)
{
        return container_of(clk, struct pxa_clk, generic_clk)->rate;
}

>  > > +   int             delay;
>  >
>  > Huh? A delay after enabling the clock? Why can't the enable() hook do
>  > that if it's really necessary?
>
>  ... which means a separate enable() function for each clock.  The delay
>  there has not a lot to do with the actual register you're frobbing, but
>  the resy of the SoC.  So, again, not really practical for PXA.

int pxa_clk_mode(struct clk *clk, bool enabled)
{
        if (enabled)
                udelay(container_of(clk, struct pxa_clk, generic_clk)->delay);
        generic_clk_mode(clk, enabled);
}

>  The result for PXA is a tradeoff between reducing the data size and
>  increasing the text size, or increasing the data size and keeping
>  the existing data size.

Wouldn't something like the above work without increasing text size too much?

regards
Philipp

Re: [PATCH 1/3] Clocklib: add generic framework for managing clocks.

[Russell King]

On Thu, Mar 27, 2008 at 10:26:48AM +0100, Haavard Skinnemoen wrote:
> You can extend the struct, put the rate there and use the same
> getrate() function for all the clocks that need to keep track of the
> current rate this way.

Well, if you're really concerned about size, you could do what I did with
PXA and introduce a struct clk_ops to contain all the constant function
pointers, rather than mashing the function pointers together - which
saves far more than trying to combine them.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: [PATCH 1/3] Clocklib: add generic framework for managing clocks.

[Russell King]

On Thu, Mar 27, 2008 at 10:29:27AM +0100, pHilipp Zabel wrote:
> >  The result for PXA is a tradeoff between reducing the data size and
> >  increasing the text size, or increasing the data size and keeping
> >  the existing data size.
> 
> Wouldn't something like the above work without increasing text size too much?

It still increases the overall size dramatically since all these function
pointers are replicated for each clock (which I've pointed to an existing
solution for).

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: [PATCH 1/3] Clocklib: add generic framework for managing clocks.

[Paul Mundt]

On Thu, Mar 27, 2008 at 09:33:01AM +0000, Russell King wrote:
> On Thu, Mar 27, 2008 at 10:26:48AM +0100, Haavard Skinnemoen wrote:
> > You can extend the struct, put the rate there and use the same
> > getrate() function for all the clocks that need to keep track of the
> > current rate this way.
> 
> Well, if you're really concerned about size, you could do what I did with
> PXA and introduce a struct clk_ops to contain all the constant function
> pointers, rather than mashing the function pointers together - which
> saves far more than trying to combine them.
> 
FWIW, this is also what we've done on SH.

Re: [PATCH 1/3] Clocklib: add generic framework for managing clocks.

[Dmitry]

Hi,

2008/3/26, Paul Mundt <lethal@linux-sh.org>:
> On Wed, Mar 26, 2008 at 07:52:34PM +0300, Dmitry wrote:
>  > 2008/3/26, Haavard Skinnemoen <haavard.skinnemoen@atmel.com>:
>
> > > Hmm...this is exactly twice as big as the struct I'm currently using,
>  > >  it doesn't contain all the fields I need, and it's undocumented.
>  >
>  > I've added a more sofisticated arch convertion patch (the clocklib for
>  > ARM PXA chips).
>  >
>
> What you've converted is still pretty tame in comparison to what this
>  sort of framework has to handle. Something like the OMAP struct clk is
>  more like a worst-case and is representative of how large this structure
>  will get for everyone if it is to be shared without dropping
>  functionality.

No, I don't want to add any more fields to generic struct clk. All
fancy fields should
go intro arch (or even clock-type) specific "priv" struct.

>
>
>  > >  I have quite a few clocks, so the increased memory consumption is quite
>  > >  significant. What are the advantages of this?
>  >
>  > At maximum 55, IIUC. I counted 32 or so additional bytes in the struct
>  > (over avr32-specific one). That would count up to 1.5 K overhead. Is
>  > that really too much for current kernels?
>  >
>
> Your idea of maximum and my hardware's idea of maximum have very little
>  in common. Most of these platforms are dealing with hundreds of different
>  clocks, though they are not necessarily all interfaced in software
>  control (mostly because a lot of them auto-gate, and because writing up
>  struct clk definitions for 200+ clocks for 30 different CPUs isn't
>  exactly my idea of a good time). This does not mean that more clocks will
>  not be hooked up as the need arises.

I have an idea of extendability and not "maximisation". And btw if
those clocks can be controlled  from the kernel, one will write a
patch to control them to get better power
management, ease driver interfaces, etc.

>  On Wed, Mar 26, 2008 at 08:04:41PM +0300, Dmitry wrote:
>  > 2008/3/26, Paul Mundt <lethal@linux-sh.org>:
>
> > > Conversely it also has fields that I don't need. If struct clk could have
>  > >  been done generically without growing to insane sizes, it would have been
>  > >  done so in linux/clk.h a long time ago. The main thing there is API
>  > >  consistency for drivers, leaving the details up to the architecture.
>  >
>  > In reality, as noted David Brownell on LAKML, there is no API consistency.
>  > The driver has no way to know whether clk API is implemented at all or whether
>  > the "optional" functions do exist.
>  >
>
> There is certainly API consistency. If there were not, we wouldn't have
>  linux/clk.h. The fact that many platforms add on top of this does not
>  detract from the fact that we have a common API that is currently shared.

I don't mean "additions". I meant optional "rate-controlling"
functions that some platforms
event don't try to implement.

>
>  There is indeed no way to know what optional functionality exists, but
>  your proposed structure and interface largely works around that problem
>  by ignoring all of the optional functionality. This works great for an
>  idealized set of platforms and clock configurations, but quickly runs in
>  to the same issue that linux/clk.h has today. It's not clear how your
>  proposed interface helps any of this other than providing a struct clk
>  that can be used more generically.

In the current situation if the "rate" or "parent" functionality
doesn't exist and the driver tries to use it, one would either get
compilation errors, or some non-standard -ENOsmth error.

With my patchset if the CLOCK_LIB is selected, the driver can assume
that it can safely call all linux/clk.h functions and if requested
feature isn't supported it'll get -EINVAL.

>  > Moreover clocks aren't limited only to platform-specific code. As an
>  > example of the in-tree driver that will benefit from clocklib you can
>  > check drivers/mfd/sm501.c which  contains  it's own set of functions
>  > to manage clocks.
>  >
>
> That's certainly true, it's definitely worthwhile to try and unify as
>  much of the interface as possible. Perhaps it makes more sense to try and
>  have a common struct clk with the bare essentials and then allow the
>  platforms to extend that functionality based on their own capabilities.
>  This could be done through the private data I suppose.

Yup! Or  as Haavard suggested, via clock extention. I took the first
way, because
I didn't want to conflict too much with OMAP clocks (plat-omap/clock.c
uses clocks
extension for self purposes. Probably this can be merged).

>  > That's one of the initial reasons of this patchserie: I have a device
>  > that can be installed on several platforms. And I want for this device
>  > to provide clocks to some other devices.
>  >
>
> I don't disagree with your intentions, or that something like this would
>  be a good idea. What needs to happen first is having a look at all of the
>  different versions of the clock framework that are out there and coming
>  up with a consolidated struct clk, then seeing if the resulting size is
>  practical enough to actually use for any significant number of clocks.

That sound constructive. Good!

>
>  Your current definition doesn't meet the requirements of all of the
>  struct clk users in the kernel, and it's already getting quite big
>  compared to what people (avr32, sh, some ARM platforms, etc.) are using
>  today. This is a good indicator of why a common definition wasn't a good
>  fit in the first place.
>

IMHO It wasn't good when the linux/clk.h first arrived. Now we already
have a few implmentations of it, so we can really judge what is common
and can be moved
to generic code, what is platform-specific.

-- 
With best wishes
Dmitry

Re: [PATCH 1/3] Clocklib: add generic framework for managing clocks.

[Haavard Skinnemoen]

[domen.puncer@telargo.com keeps bouncing on me, removed from Cc]

On Thu, 27 Mar 2008 09:33:01 +0000
Russell King <rmk+lkml@arm.linux.org.uk> wrote:

> On Thu, Mar 27, 2008 at 10:26:48AM +0100, Haavard Skinnemoen wrote:
> > You can extend the struct, put the rate there and use the same
> > getrate() function for all the clocks that need to keep track of the
> > current rate this way.  
> 
> Well, if you're really concerned about size, you could do what I did with
> PXA and introduce a struct clk_ops to contain all the constant function
> pointers, rather than mashing the function pointers together - which
> saves far more than trying to combine them.

I don't see what this has to do with the paragraph you quoted, but
yeah, good point. I don't think it should be used as an excuse for
filling up struct clk with platform-specific crap, however.

So how about something like this?

struct clk_ops {
	struct module	*owner;

	int (*can_get)	(struct clk *, struct device *);
	int (*set_parent) (struct clk *, struct clk *);
	int (*enable)	(struct clk *);
	void (*disable)	(struct clk *);
	unsigned long (*getrate) (struct clk*);
	int (*setrate)	(struct clk *, unsigned long);
	long (*roundrate) (struct clk *, unsigned long);
};

struct clk {
	struct list_head node;
	struct clk	*parent;

	const char	*name;
	int		users;

	const struct clk_ops *ops;
};

Haavard

Re: [PATCH 1/3] Clocklib: add generic framework for managing clocks.

[Dmitry]

Hi,

2008/3/27, Haavard Skinnemoen <haavard.skinnemoen@atmel.com>:
> [domen.puncer@telargo.com keeps bouncing on me, removed from Cc]
>
>  On Thu, 27 Mar 2008 09:33:01 +0000
>
> Russell King <rmk+lkml@arm.linux.org.uk> wrote:
>
>
> > On Thu, Mar 27, 2008 at 10:26:48AM +0100, Haavard Skinnemoen wrote:
>  > > You can extend the struct, put the rate there and use the same
>  > > getrate() function for all the clocks that need to keep track of the
>  > > current rate this way.
>  >
>  > Well, if you're really concerned about size, you could do what I did with
>  > PXA and introduce a struct clk_ops to contain all the constant function
>  > pointers, rather than mashing the function pointers together - which
>  > saves far more than trying to combine them.
>
>
> I don't see what this has to do with the paragraph you quoted, but
>  yeah, good point. I don't think it should be used as an excuse for
>  filling up struct clk with platform-specific crap, however.
>
>  So how about something like this?
>
>  struct clk_ops {
>         struct module   *owner;
>
>
>         int (*can_get)  (struct clk *, struct device *);
>         int (*set_parent) (struct clk *, struct clk *);
>
>         int (*enable)   (struct clk *);
>         void (*disable) (struct clk *);
>
>         unsigned long (*getrate) (struct clk*);
>
>         int (*setrate)  (struct clk *, unsigned long);
>
>         long (*roundrate) (struct clk *, unsigned long);
>
> };
>
>
>  struct clk {
>         struct list_head node;
>         struct clk      *parent;
>
>         const char      *name;
>
>         int             users;
>
>         const struct clk_ops *ops;
>  };

I like this idea! This would also allow to cleanup the references code, etc.
Also after I saw such refactored struct clk, I thought that it looks
nearly like kobject. Maybe we should switch to the kobject-based
structs? What do you think?

-- 
With best wishes
Dmitry

Re: [PATCH 1/3] Clocklib: add generic framework for managing clocks.

[Haavard Skinnemoen]

On Thu, 27 Mar 2008 13:08:37 +0300
Dmitry <dbaryshkov@gmail.com> wrote:

> I like this idea! This would also allow to cleanup the references code, etc.

Great!

> Also after I saw such refactored struct clk, I thought that it looks
> nearly like kobject. Maybe we should switch to the kobject-based
> structs? What do you think?

I think that would be overkill. We should try to keep this stuff as
lightweight as possible, and I'm not sure if we can give the kobject
"parent" field the meaning we want it to have...or use the kref thing
to do something unrelated to object lifecycle management (i.e. we don't
want to destroy the object when the refcount goes to zero, we just want
to stop the clock.)

Haavard

Re: [PATCH 1/3] Clocklib: add generic framework for managing clocks.

[Dmitry]

Hi,

2008/3/27, Haavard Skinnemoen <haavard.skinnemoen@atmel.com>:
> On Thu, 27 Mar 2008 13:08:37 +0300
>
> Dmitry <dbaryshkov@gmail.com> wrote:
>
>
> > I like this idea! This would also allow to cleanup the references code, etc.
>
>
> Great!
>
>
>  > Also after I saw such refactored struct clk, I thought that it looks
>  > nearly like kobject. Maybe we should switch to the kobject-based
>  > structs? What do you think?
>
>
> I think that would be overkill. We should try to keep this stuff as
>  lightweight as possible, and I'm not sure if we can give the kobject
>  "parent" field the meaning we want it to have...or use the kref thing
>  to do something unrelated to object lifecycle management (i.e. we don't
>  want to destroy the object when the refcount goes to zero, we just want
>  to stop the clock.)

ACK. Then I'll repost the updated patchset later.

-- 
With best wishes
Dmitry

Re: [PATCH 1/3] Clocklib: add generic framework for managing clocks.

[Pavel Machek]

On Wed 2008-03-26 17:04:41, Haavard Skinnemoen wrote:
> On Wed, 26 Mar 2008 18:52:03 +0300
> Dmitry Baryshkov <dbaryshkov@gmail.com> wrote:
> 
> > +struct clk {
> > +	struct list_head node;
> > +	struct clk	*parent;
> > +
> > +	const char	*name;
> > +	struct module	*owner;
> > +
> > +	int		users;
> > +	unsigned long	rate;
> > +	int		delay;
> > +
> > +	int (*can_get)	(struct clk *, struct device *);
> > +	int (*set_parent) (struct clk *, struct clk *);
> > +	int (*enable)	(struct clk *);
> > +	void (*disable)	(struct clk *);
> > +	unsigned long (*getrate) (struct clk*);
> > +	int (*setrate)	(struct clk *, unsigned long);
> > +	long (*roundrate) (struct clk *, unsigned long);
> > +
> > +	void		*priv;
> > +};
> 
> Hmm...this is exactly twice as big as the struct I'm currently using,
> it doesn't contain all the fields I need, and it's undocumented.

Like unifying 15-or-so versions of clock framework that are out there?

> I have quite a few clocks, so the increased memory consumption is quite
> significant. What are the advantages of this?

How many clocks do you have?



-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 1/3] Clocklib: add generic framework for managing clocks.

[Haavard Skinnemoen]

Pavel Machek <pavel@ucw.cz> wrote:
> > Hmm...this is exactly twice as big as the struct I'm currently using,
> > it doesn't contain all the fields I need, and it's undocumented.  
> 
> Like unifying 15-or-so versions of clock framework that are out there?

I'm not complaining about the goal of this patchset, I'm just arguing
about the price. And I do think we managed to come to an agreement
later in the thread (which is actually cheaper than what I currently
have!)

> > I have quite a few clocks, so the increased memory consumption is quite
> > significant. What are the advantages of this?  
> 
> How many clocks do you have?

55 currently, and there are still a few clocks that haven't been wired
up yet. So shaving off a few bytes can make a big difference, and other
platforms have even more clocks.

Haavard

Re: [PATCH 1/3] Clocklib: add generic framework for managing clocks.

[Haavard Skinnemoen]

Dmitry Baryshkov <dbaryshkov@gmail.com> wrote:
> > > +#ifdef CONFIG_DEBUG_FS
> > > +	struct dentry *dir;
> > > +	struct dentry *info;
> > > +#endif  
> > 
> > Can't you hide this in the code, say by wrappering the
> > struct with something else when it is registered?  
> 
> It is allocated dynamically by drivers. I can move this to
> struct clk_private to specify that it's private, but it should be
> visible outside

Actually, I don't think it _should_ be private. Low-level clock drivers
might want to provide debugfs nodes on their own, and those nodes
naturally belong in the same directory as the clklib ones. So the
debugfs root node must be exposed somehow.

You can get rid of the "info" field if you apply this patch:

http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/patches/driver-core/debugfs-implement-debugfs_remove_recursive.patch

Haavard

Re: [PATCH 1/3] Clocklib: add generic framework for managing clocks.

[Dmitry Baryshkov]

On Thu, Jul 03, 2008 at 09:31:57PM +0100, Ben Dooks wrote:
> On Thu, Jun 26, 2008 at 04:51:38PM +0400, Dmitry Baryshkov wrote:
> > Provide a generic framework that platform may choose
> > to support clocks api. In particular this provides
> > platform-independant struct clk definition, a full
> > implementation of clocks api and a set of functions
> > for registering and unregistering clocks in a safe way.
> > 
> > Signed-off-by: Dmitry Baryshkov <dbaryshkov@gmail.com>
> > ---
> >  include/linux/clocklib.h |   58 ++++++++
> >  lib/Kconfig              |    3 +
> >  lib/Makefile             |    1 +
> >  lib/clocklib.c           |  353 ++++++++++++++++++++++++++++++++++++++++++++++
> 
> why under lib? drivers/clk might be a better place for this.

Because there will be probably no "clock only" drivers. Probably it
should go into kernel/, but definitely not into drivers/clk/

> 
> >  4 files changed, 415 insertions(+), 0 deletions(-)
> >  create mode 100644 include/linux/clocklib.h
> >  create mode 100644 lib/clocklib.c
> > 
> > diff --git a/include/linux/clocklib.h b/include/linux/clocklib.h
> > new file mode 100644
> > index 0000000..cf2b41e
> > --- /dev/null
> > +++ b/include/linux/clocklib.h
> > @@ -0,0 +1,58 @@
> > +/*
> > + * Copyright (C) 2008 Dmitry Baryshkov
> > + *
> > + * This file is released under the GPL v2.
> > + */
> > +
> > +#ifndef CLKLIB_H
> > +#define CLKLIB_H
> > +
> > +#include <linux/spinlock.h>
> > +#include <linux/kref.h>
> > +
> > +struct clk;
> > +
> > +/**
> > + * struct clk_ops - generic clock management operations
> > + * @can_get: checks whether passed device can get this clock
> > + * @set_parent: reconfigures the clock to use specified parent
> > + * @set_mode: enable or disable specified clock
> > + * @get_rate: obtain the current clock rate of a specified clock
> > + * @set_rate: set the clock rate for a specified clock
> > + * @round_rate: adjust a reate to the exact rate a clock can provide
> > + *
> > + * This structure specifies clock operations that are used to configure
> > + * specific clock.
> > + */
> > +struct clk_ops {
> > +	int (*can_get)(struct clk *clk, struct device *dev);
> > +	int (*set_parent)(struct clk *clk, struct clk *parent);
> > +	int (*enable)(struct clk *clk);
> > +	void (*disable)(struct clk *clk);
> > +	unsigned long (*get_rate)(struct clk *clk);
> > +	long (*round_rate)(struct clk *clk, unsigned long hz, bool apply);
> > +};
> 
> 
> I'd much prefer to see the following changes:
> 
> 1) enable and disable merged into one, and pass an enable
>    parameter, ie:
> 
> 	int (*set_enable)(struct clk *clk, bool on);
> 
> as most code I see is of the following:
> 
> int myclk_enable(struct clk *clk, bool on)
> {
> 	clkbit = bit_for_clk(clk);
> 	reg = read_reg(reg_for_clk(clk));
> 	
> 	if (on)
> 		reg |= clkbit;
> 	else
> 		reg &= ~clkbit;
> 
> 	write_reg(reg, reg_for_clk(clk));
> 	return 0;
> }

It was discussed before. Andrew Morton specifically said, that he
doesn't want set_enable-like functions with bool param.

> whereas if you have seperate enable and disable methods you
> end up duplicating quite a lot of that code, as so:
> 
> int myclk_enable(struct clk *clk)
> {
> 	clkbit = bit_for_clk(clk);
> 	reg = read_reg(reg_for_clk(clk));
> 	
> 	reg |= clkbit;
> 
> 	write_reg(reg, reg_for_clk(clk));
> 	return 0;
> }
> 
> int myclk_disable(struct clk *clk)
> {
> 	clkbit = bit_for_clk(clk);
> 	reg = read_reg(reg_for_clk(clk));
> 	
> 	reg &= ~clkbit;
> 
> 	write_reg(reg, reg_for_clk(clk));
> 	return 0;
> }
> 
> > +
> > +
> > +struct clk {
> > +	struct list_head node;
> > +	spinlock_t *lock;
> > +	struct kref ref;
> > +	int usage;
> > +#ifdef CONFIG_DEBUG_FS
> > +	struct dentry *dir;
> > +	struct dentry *info;
> > +#endif
> 
> Can't you hide this in the code, say by wrappering the
> struct with something else when it is registered?

It is allocated dynamically by drivers. I can move this to
struct clk_private to specify that it's private, but it should be
visible outside

> 
> > +
> > +	const char *name;
> > +	struct clk *parent;
> > +	struct clk_ops *ops;
> > +	void (*release)(struct clk *clk);
> > +};
> > +
> > +int clk_register(struct clk *clk);
> > +void clk_unregister(struct clk *clk);
> > +int clks_register(struct clk **clk, size_t num);
> > +void clks_unregister(struct clk **clk, size_t num);
> > +
> > +#endif
> > diff --git a/lib/Kconfig b/lib/Kconfig
> > index 8cc8e87..592f5e1 100644
> > --- a/lib/Kconfig
> > +++ b/lib/Kconfig
> > @@ -13,6 +13,9 @@ config GENERIC_FIND_FIRST_BIT
> >  config GENERIC_FIND_NEXT_BIT
> >  	def_bool n
> >  
> > +config HAVE_CLOCKLIB
> > +	tristate
> > +
> >  config CRC_CCITT
> >  	tristate "CRC-CCITT functions"
> >  	help
> > diff --git a/lib/Makefile b/lib/Makefile
> > index 74b0cfb..cee74e1 100644
> > --- a/lib/Makefile
> > +++ b/lib/Makefile
> > @@ -37,6 +37,7 @@ obj-$(CONFIG_PLIST) += plist.o
> >  obj-$(CONFIG_DEBUG_PREEMPT) += smp_processor_id.o
> >  obj-$(CONFIG_DEBUG_LIST) += list_debug.o
> >  obj-$(CONFIG_DEBUG_OBJECTS) += debugobjects.o
> > +obj-$(CONFIG_HAVE_CLOCKLIB) += clocklib.o
> 
> why not call this CONFIG_GENERIC_CLK ?

it was modelled after CONFIG_HAVE_GPIO_LIB

>   
> >  ifneq ($(CONFIG_HAVE_DEC_LOCK),y)
> >    lib-y += dec_and_lock.o
> > diff --git a/lib/clocklib.c b/lib/clocklib.c
> > new file mode 100644
> > index 0000000..590a665
> > --- /dev/null
> > +++ b/lib/clocklib.c
> > @@ -0,0 +1,353 @@
> > +/*
> > + * Generic clocks API implementation
> > + *
> > + * Copyright (c) 2008 Dmitry Baryshkov
> > + *
> > + * This file is released under the GPL v2.
> > + */
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/clk.h>
> > +#include <linux/clocklib.h>
> > +#include <linux/spinlock.h>
> > +
> > +static LIST_HEAD(clocks);
> > +static DEFINE_SPINLOCK(clocks_lock);
> > +
> > +#ifdef CONFIG_DEBUG_FS
> > +#include <linux/debugfs.h>
> > +#include <linux/seq_file.h>
> > +static struct dentry *clkdir;
> 
> possibly seperate the code out into a seperate file

fine

> 
> > +
> > +static int clocklib_show(struct seq_file *s, void *data)
> > +{
> > +	struct clk *clk = s->private;
> > +
> > +	BUG_ON(!clk);
> > +
> > +	seq_printf(s, "set_parent=%savailable\nusage=%d/%d\nrate=%10lu Hz\n",
> > +			clk->ops && clk->ops->set_parent ? "not " : "",
> > +			clk->usage, atomic_read(&clk->ref.refcount),
> > +			clk_get_rate(clk));
> > +//	if (clk->ops && clk->ops->format)
> > +//			clk->ops->format(clk, s);
> > +
> > +	return 0;
> > +}
> > +
> > +static int clocklib_open(struct inode *inode, struct file *file)
> > +{
> > +	return single_open(file, clocklib_show, inode->i_private);
> > +}
> > +
> > +static struct file_operations clocklib_operations = {
> > +	.open		= clocklib_open,
> > +	.read		= seq_read,
> > +	.llseek		= seq_lseek,
> > +	.release	= single_release,
> > +};
> > +
> > +static int clk_debugfs_init(struct clk *clk)
> > +{
> > +	struct dentry *dir;
> > +	struct dentry *info;
> > +
> > +	if (!clkdir)
> > +		dump_stack();
> > +
> > +	dir = debugfs_create_dir(clk->name,
> > +			clk->parent ? clk->parent->dir : clkdir);
> > +
> > +	if (IS_ERR(dir))
> > +		return PTR_ERR(dir);
> > +
> > +	info = debugfs_create_file("info", S_IFREG | S_IRUGO,
> > +			dir, clk, &clocklib_operations);
> > +
> > +	if (IS_ERR(info)) {
> > +		debugfs_remove(dir);
> > +		return PTR_ERR(info);
> > +	}
> > +
> > +	clk->dir = dir;
> > +	clk->info = info;
> > +
> > +	return 0;
> > +}
> > +
> > +static void clk_debugfs_clean(struct clk *clk)
> > +{
> > +	if (clk->info)
> > +		debugfs_remove(clk->info);
> > +	clk->info = NULL;
> > +
> > +	if (clk->dir)
> > +		debugfs_remove(clk->dir);
> > +	clk->dir = NULL;
> > +}
> > +
> > +static void clk_debugfs_reparent(struct clk *clk, struct clk *old, struct clk *new)
> > +{
> > +	struct dentry *oldd = old ? old->dir : clkdir;
> > +	struct dentry *newd = new ? new->dir : clkdir;
> > +	struct dentry *dir = debugfs_rename(oldd, clk->dir, newd, clk->name);
> > +
> > +	if (IS_ERR(dir))
> > +		WARN_ON(1);
> > +	else
> > +		clk->dir = dir;
> > +}
> > +
> > +static int __init clocklib_debugfs_init(void)
> > +{
> > +	clkdir = debugfs_create_dir("clocks", NULL);
> > +	return 0;
> > +}
> > +core_initcall(clocklib_debugfs_init);
> > +#else
> > +#define clk_debugfs_init(clk) ({0;})
> > +#define clk_debugfs_clean(clk) do {} while (0);
> > +#define clk_debugfs_reparent(clk, old, new) do {} while (0);
> > +#endif
> > +
> > +static int clk_can_get_def(struct clk *clk, struct device *dev)
> > +{
> > +	return 1;
> > +}
> > +
> > +static unsigned long clk_get_rate_def(struct clk *clk)
> > +{
> > +	return 0;
> > +}
> > +
> > +static long clk_round_rate_def(struct clk *clk, unsigned long hz, bool apply)
> > +{
> > +	long rate = clk->ops->get_rate(clk);
> > +
> > +	if (apply && hz != rate)
> > +		return -EINVAL;
> > +
> > +	return rate;
> > +}
> > +
> > +static void clk_release(struct kref *ref)
> > +{
> > +	struct clk *clk = container_of(ref, struct clk, ref);
> > +
> > +	BUG_ON(!clk->release);
> > +
> > +	if (clk->parent)
> > +		kref_get(&clk->parent->ref);
> > +
> > +	clk->release(clk);
> > +}
> > +EXPORT_SYMBOL(clk_round_rate);
> > +
> > +struct clk* clk_get_parent(struct clk *clk)
> > +{
> > +	struct clk *parent;
> > +
> > +	spin_lock(clk->lock);
> > +
> > +	parent = clk->parent;
> > +	kref_get(&parent->ref);
> > +
> > +	spin_unlock(clk->lock);
> > +
> > +	return parent;
> > +}
> > +EXPORT_SYMBOL(clk_get_parent);
> > +
> > +int clk_set_parent(struct clk *clk, struct clk *parent)
> > +{
> > +	int rc = -EINVAL;
> > +	struct clk *old;
> > +
> > +	spin_lock(clk->lock);
> > +
> > +	if (!clk->ops->set_parent)
> > +		goto out;
> > +
> > +	old = clk->parent;
> > +
> > +	rc = clk->ops->set_parent(clk, parent);
> > +	if (rc)
> > +		goto out;
> > +
> > +	kref_get(&parent->ref);
> > +	clk->parent = parent;
> > +
> > +	clk_debugfs_reparent(clk, old, parent);
> > +
> > +	kref_put(&old->ref, clk_release);
> > +
> > +out:
> > +	spin_unlock(clk->lock);
> > +
> > +	return rc;
> > +}
> > +EXPORT_SYMBOL(clk_set_parent);
> > +
> > +int clk_register(struct clk *clk)
> > +{
> > +	int rc;
> > +
> > +	BUG_ON(!clk->ops);
> > +	BUG_ON(!clk->ops->enable || !clk->ops->disable);
> > +
> > +	if (!clk->ops->can_get)
> > +		clk->ops->can_get = clk_can_get_def;
> > +	if (!clk->ops->get_rate)
> > +		clk->ops->get_rate = clk_get_rate_def;
> > +	if (!clk->ops->round_rate)
> > +		clk->ops->round_rate = clk_round_rate_def;
> > +
> > +	kref_init(&clk->ref);
> > +
> > +	spin_lock(&clocks_lock);
> > +	if (clk->parent)
> > +		kref_get(&clk->parent->ref);
> > +	list_add_tail(&clk->node, &clocks);
> > +
> > +	rc = clk_debugfs_init(clk);
> > +	if (rc) {
> > +		list_del(&clk->node);
> > +		kref_put(&clk->ref, clk_release);
> > +	}
> > +
> > +	spin_unlock(&clocks_lock);
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(clk_register);
> > +
> > +void clk_unregister(struct clk *clk)
> > +{
> > +	spin_lock(&clocks_lock);
> > +	clk_debugfs_clean(clk);
> > +	list_del(&clk->node);
> > +	kref_put(&clk->ref, clk_release);
> > +	spin_unlock(&clocks_lock);
> > +}
> > +EXPORT_SYMBOL(clk_unregister);
> > +
> > +int clks_register(struct clk **clk, size_t num)
> > +{
> > +	int i;
> > +	int rc;
> > +	for (i = 0; i < num; i++) {
> > +		rc = clk_register(clk[i]);
> > +		if (rc)
> > +			return rc;
> > +	}
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(clks_register);
> > +
> > +void clks_unregister(struct clk **clk, size_t num)
> > +{
> > +	int i;
> > +	for (i = 0; i < num; i++)
> > +		clk_unregister(clk[i]);
> > +}
> > +EXPORT_SYMBOL(clks_unregister);
> > +
> > +struct clk *clk_get(struct device *dev, const char *id)
> > +{
> > +	struct clk *clk = NULL, *p;
> > +
> > +	spin_lock(&clocks_lock);
> > +	list_for_each_entry(p, &clocks, node)
> > +		if (strcmp(id, p->name) == 0 && p->ops->can_get(p, dev)) {
> > +			clk = p;
> > +			kref_get(&clk->ref);
> > +			break;
> > +		}
> > +
> > +	spin_unlock(&clocks_lock);
> > +
> > +	return clk;
> > +}
> > +EXPORT_SYMBOL(clk_get);
> > +
> > +void clk_put(struct clk *clk)
> > +{
> > +	kref_put(&clk->ref, clk_release);
> > +}
> > +EXPORT_SYMBOL(clk_put);
> > +
> > +int clk_enable(struct clk *clk)
> > +{
> > +	int rc = 0;
> > +
> > +	spin_lock(clk->lock);
> > +
> > +	clk->usage++;
> > +	if (clk->usage == 1)
> > +		rc = clk->ops->enable(clk);
> > +
> > +	if (rc)
> > +		clk->usage--;
> 
> clk->usage = 0; is the same, maybe also easier to encase
> the whole lot in the same if block:
> 
> 	if (clk->usage == 1) {
> 		rc = clk->ops->enable(clk);
> 
> 		if (rc)
> 			clk->usage = 0;
> 	}

ok

> 
> > +
> > +	spin_unlock(clk->lock);
> > +
> > +	return rc;
> > +}
> > +EXPORT_SYMBOL(clk_enable);
> > +
> > +void clk_disable(struct clk *clk)
> > +{
> > +	spin_lock(clk->lock);
> > +
> > +	WARN_ON(clk->usage <= 0);
> > +
> > +	clk->usage--;
> > +	if (clk->usage == 0)
> > +		clk->ops->disable(clk);
> > +
> > +	spin_unlock(clk->lock);
> > +}
> > +EXPORT_SYMBOL(clk_disable);
> > +
> > +unsigned long clk_get_rate(struct clk *clk)
> > +{
> > +	unsigned long hz;
> > +
> > +	spin_lock(clk->lock);
> > +
> > +	hz = clk->ops->get_rate(clk);
> > +
> > +	spin_unlock(clk->lock);
> > +
> > +	return hz;
> > +}
> > +EXPORT_SYMBOL(clk_get_rate);
> > +
> > +int clk_set_rate(struct clk *clk, unsigned long hz)
> > +{
> > +	long rc;
> > +
> > +	spin_lock(clk->lock);
> > +
> > +	rc = clk->ops->round_rate(clk, hz, 1);
> > +
> > +	spin_unlock(clk->lock);
> > +
> > +	return rc < 0 ? rc : 0;
> > +}
> > +EXPORT_SYMBOL(clk_set_rate);
> > +
> > +long clk_round_rate(struct clk *clk, unsigned long hz)
> > +{
> > +	long rc;
> > +
> > +	spin_lock(clk->lock);
> > +
> > +	rc = clk->ops->round_rate(clk, hz, 0);
> > +
> > +	spin_unlock(clk->lock);
> > +
> > +	return rc;
> > +}
> 
> -- 
> Ben (ben@fluff.org, http://www.fluff.org/)
> 
>   'a smiley only costs 4 bytes'

-- 
With best wishes
Dmitry

Re: [PATCH 1/3] Clocklib: add generic framework for managing clocks.

[Pavel Machek]

On Thu 2008-07-03 21:31:57, Ben Dooks wrote:
> On Thu, Jun 26, 2008 at 04:51:38PM +0400, Dmitry Baryshkov wrote:
> > Provide a generic framework that platform may choose
> > to support clocks api. In particular this provides
> > platform-independant struct clk definition, a full
> > implementation of clocks api and a set of functions
> > for registering and unregistering clocks in a safe way.
> > 
> > Signed-off-by: Dmitry Baryshkov <dbaryshkov@gmail.com>
> > ---
> >  include/linux/clocklib.h |   58 ++++++++
> >  lib/Kconfig              |    3 +
> >  lib/Makefile             |    1 +
> >  lib/clocklib.c           |  353 ++++++++++++++++++++++++++++++++++++++++++++++
> 
> why under lib? drivers/clk might be a better place for this.

This does not belong into lib/, really. drivers/clk seems ok.
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 1/3] Clocklib: add generic framework for managing clocks.

[Ben Dooks]

On Thu, Jun 26, 2008 at 04:51:38PM +0400, Dmitry Baryshkov wrote:
> Provide a generic framework that platform may choose
> to support clocks api. In particular this provides
> platform-independant struct clk definition, a full
> implementation of clocks api and a set of functions
> for registering and unregistering clocks in a safe way.
> 
> Signed-off-by: Dmitry Baryshkov <dbaryshkov@gmail.com>
> ---
>  include/linux/clocklib.h |   58 ++++++++
>  lib/Kconfig              |    3 +
>  lib/Makefile             |    1 +
>  lib/clocklib.c           |  353 ++++++++++++++++++++++++++++++++++++++++++++++

why under lib? drivers/clk might be a better place for this.

>  4 files changed, 415 insertions(+), 0 deletions(-)
>  create mode 100644 include/linux/clocklib.h
>  create mode 100644 lib/clocklib.c
> 
> diff --git a/include/linux/clocklib.h b/include/linux/clocklib.h
> new file mode 100644
> index 0000000..cf2b41e
> --- /dev/null
> +++ b/include/linux/clocklib.h
> @@ -0,0 +1,58 @@
> +/*
> + * Copyright (C) 2008 Dmitry Baryshkov
> + *
> + * This file is released under the GPL v2.
> + */
> +
> +#ifndef CLKLIB_H
> +#define CLKLIB_H
> +
> +#include <linux/spinlock.h>
> +#include <linux/kref.h>
> +
> +struct clk;
> +
> +/**
> + * struct clk_ops - generic clock management operations
> + * @can_get: checks whether passed device can get this clock
> + * @set_parent: reconfigures the clock to use specified parent
> + * @set_mode: enable or disable specified clock
> + * @get_rate: obtain the current clock rate of a specified clock
> + * @set_rate: set the clock rate for a specified clock
> + * @round_rate: adjust a reate to the exact rate a clock can provide
> + *
> + * This structure specifies clock operations that are used to configure
> + * specific clock.
> + */
> +struct clk_ops {
> +	int (*can_get)(struct clk *clk, struct device *dev);
> +	int (*set_parent)(struct clk *clk, struct clk *parent);
> +	int (*enable)(struct clk *clk);
> +	void (*disable)(struct clk *clk);
> +	unsigned long (*get_rate)(struct clk *clk);
> +	long (*round_rate)(struct clk *clk, unsigned long hz, bool apply);
> +};


I'd much prefer to see the following changes:

1) enable and disable merged into one, and pass an enable
   parameter, ie:

	int (*set_enable)(struct clk *clk, bool on);

as most code I see is of the following:

int myclk_enable(struct clk *clk, bool on)
{
	clkbit = bit_for_clk(clk);
	reg = read_reg(reg_for_clk(clk));
	
	if (on)
		reg |= clkbit;
	else
		reg &= ~clkbit;

	write_reg(reg, reg_for_clk(clk));
	return 0;
}

whereas if you have seperate enable and disable methods you
end up duplicating quite a lot of that code, as so:

int myclk_enable(struct clk *clk)
{
	clkbit = bit_for_clk(clk);
	reg = read_reg(reg_for_clk(clk));
	
	reg |= clkbit;

	write_reg(reg, reg_for_clk(clk));
	return 0;
}

int myclk_disable(struct clk *clk)
{
	clkbit = bit_for_clk(clk);
	reg = read_reg(reg_for_clk(clk));
	
	reg &= ~clkbit;

	write_reg(reg, reg_for_clk(clk));
	return 0;
}

> +
> +
> +struct clk {
> +	struct list_head node;
> +	spinlock_t *lock;
> +	struct kref ref;
> +	int usage;
> +#ifdef CONFIG_DEBUG_FS
> +	struct dentry *dir;
> +	struct dentry *info;
> +#endif

Can't you hide this in the code, say by wrappering the
struct with something else when it is registered?

> +
> +	const char *name;
> +	struct clk *parent;
> +	struct clk_ops *ops;
> +	void (*release)(struct clk *clk);
> +};
> +
> +int clk_register(struct clk *clk);
> +void clk_unregister(struct clk *clk);
> +int clks_register(struct clk **clk, size_t num);
> +void clks_unregister(struct clk **clk, size_t num);
> +
> +#endif
> diff --git a/lib/Kconfig b/lib/Kconfig
> index 8cc8e87..592f5e1 100644
> --- a/lib/Kconfig
> +++ b/lib/Kconfig
> @@ -13,6 +13,9 @@ config GENERIC_FIND_FIRST_BIT
>  config GENERIC_FIND_NEXT_BIT
>  	def_bool n
>  
> +config HAVE_CLOCKLIB
> +	tristate
> +
>  config CRC_CCITT
>  	tristate "CRC-CCITT functions"
>  	help
> diff --git a/lib/Makefile b/lib/Makefile
> index 74b0cfb..cee74e1 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -37,6 +37,7 @@ obj-$(CONFIG_PLIST) += plist.o
>  obj-$(CONFIG_DEBUG_PREEMPT) += smp_processor_id.o
>  obj-$(CONFIG_DEBUG_LIST) += list_debug.o
>  obj-$(CONFIG_DEBUG_OBJECTS) += debugobjects.o
> +obj-$(CONFIG_HAVE_CLOCKLIB) += clocklib.o

why not call this CONFIG_GENERIC_CLK ?
  
>  ifneq ($(CONFIG_HAVE_DEC_LOCK),y)
>    lib-y += dec_and_lock.o
> diff --git a/lib/clocklib.c b/lib/clocklib.c
> new file mode 100644
> index 0000000..590a665
> --- /dev/null
> +++ b/lib/clocklib.c
> @@ -0,0 +1,353 @@
> +/*
> + * Generic clocks API implementation
> + *
> + * Copyright (c) 2008 Dmitry Baryshkov
> + *
> + * This file is released under the GPL v2.
> + */
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/clk.h>
> +#include <linux/clocklib.h>
> +#include <linux/spinlock.h>
> +
> +static LIST_HEAD(clocks);
> +static DEFINE_SPINLOCK(clocks_lock);
> +
> +#ifdef CONFIG_DEBUG_FS
> +#include <linux/debugfs.h>
> +#include <linux/seq_file.h>
> +static struct dentry *clkdir;

possibly seperate the code out into a seperate file

> +
> +static int clocklib_show(struct seq_file *s, void *data)
> +{
> +	struct clk *clk = s->private;
> +
> +	BUG_ON(!clk);
> +
> +	seq_printf(s, "set_parent=%savailable\nusage=%d/%d\nrate=%10lu Hz\n",
> +			clk->ops && clk->ops->set_parent ? "not " : "",
> +			clk->usage, atomic_read(&clk->ref.refcount),
> +			clk_get_rate(clk));
> +//	if (clk->ops && clk->ops->format)
> +//			clk->ops->format(clk, s);
> +
> +	return 0;
> +}
> +
> +static int clocklib_open(struct inode *inode, struct file *file)
> +{
> +	return single_open(file, clocklib_show, inode->i_private);
> +}
> +
> +static struct file_operations clocklib_operations = {
> +	.open		= clocklib_open,
> +	.read		= seq_read,
> +	.llseek		= seq_lseek,
> +	.release	= single_release,
> +};
> +
> +static int clk_debugfs_init(struct clk *clk)
> +{
> +	struct dentry *dir;
> +	struct dentry *info;
> +
> +	if (!clkdir)
> +		dump_stack();
> +
> +	dir = debugfs_create_dir(clk->name,
> +			clk->parent ? clk->parent->dir : clkdir);
> +
> +	if (IS_ERR(dir))
> +		return PTR_ERR(dir);
> +
> +	info = debugfs_create_file("info", S_IFREG | S_IRUGO,
> +			dir, clk, &clocklib_operations);
> +
> +	if (IS_ERR(info)) {
> +		debugfs_remove(dir);
> +		return PTR_ERR(info);
> +	}
> +
> +	clk->dir = dir;
> +	clk->info = info;
> +
> +	return 0;
> +}
> +
> +static void clk_debugfs_clean(struct clk *clk)
> +{
> +	if (clk->info)
> +		debugfs_remove(clk->info);
> +	clk->info = NULL;
> +
> +	if (clk->dir)
> +		debugfs_remove(clk->dir);
> +	clk->dir = NULL;
> +}
> +
> +static void clk_debugfs_reparent(struct clk *clk, struct clk *old, struct clk *new)
> +{
> +	struct dentry *oldd = old ? old->dir : clkdir;
> +	struct dentry *newd = new ? new->dir : clkdir;
> +	struct dentry *dir = debugfs_rename(oldd, clk->dir, newd, clk->name);
> +
> +	if (IS_ERR(dir))
> +		WARN_ON(1);
> +	else
> +		clk->dir = dir;
> +}
> +
> +static int __init clocklib_debugfs_init(void)
> +{
> +	clkdir = debugfs_create_dir("clocks", NULL);
> +	return 0;
> +}
> +core_initcall(clocklib_debugfs_init);
> +#else
> +#define clk_debugfs_init(clk) ({0;})
> +#define clk_debugfs_clean(clk) do {} while (0);
> +#define clk_debugfs_reparent(clk, old, new) do {} while (0);
> +#endif
> +
> +static int clk_can_get_def(struct clk *clk, struct device *dev)
> +{
> +	return 1;
> +}
> +
> +static unsigned long clk_get_rate_def(struct clk *clk)
> +{
> +	return 0;
> +}
> +
> +static long clk_round_rate_def(struct clk *clk, unsigned long hz, bool apply)
> +{
> +	long rate = clk->ops->get_rate(clk);
> +
> +	if (apply && hz != rate)
> +		return -EINVAL;
> +
> +	return rate;
> +}
> +
> +static void clk_release(struct kref *ref)
> +{
> +	struct clk *clk = container_of(ref, struct clk, ref);
> +
> +	BUG_ON(!clk->release);
> +
> +	if (clk->parent)
> +		kref_get(&clk->parent->ref);
> +
> +	clk->release(clk);
> +}
> +EXPORT_SYMBOL(clk_round_rate);
> +
> +struct clk* clk_get_parent(struct clk *clk)
> +{
> +	struct clk *parent;
> +
> +	spin_lock(clk->lock);
> +
> +	parent = clk->parent;
> +	kref_get(&parent->ref);
> +
> +	spin_unlock(clk->lock);
> +
> +	return parent;
> +}
> +EXPORT_SYMBOL(clk_get_parent);
> +
> +int clk_set_parent(struct clk *clk, struct clk *parent)
> +{
> +	int rc = -EINVAL;
> +	struct clk *old;
> +
> +	spin_lock(clk->lock);
> +
> +	if (!clk->ops->set_parent)
> +		goto out;
> +
> +	old = clk->parent;
> +
> +	rc = clk->ops->set_parent(clk, parent);
> +	if (rc)
> +		goto out;
> +
> +	kref_get(&parent->ref);
> +	clk->parent = parent;
> +
> +	clk_debugfs_reparent(clk, old, parent);
> +
> +	kref_put(&old->ref, clk_release);
> +
> +out:
> +	spin_unlock(clk->lock);
> +
> +	return rc;
> +}
> +EXPORT_SYMBOL(clk_set_parent);
> +
> +int clk_register(struct clk *clk)
> +{
> +	int rc;
> +
> +	BUG_ON(!clk->ops);
> +	BUG_ON(!clk->ops->enable || !clk->ops->disable);
> +
> +	if (!clk->ops->can_get)
> +		clk->ops->can_get = clk_can_get_def;
> +	if (!clk->ops->get_rate)
> +		clk->ops->get_rate = clk_get_rate_def;
> +	if (!clk->ops->round_rate)
> +		clk->ops->round_rate = clk_round_rate_def;
> +
> +	kref_init(&clk->ref);
> +
> +	spin_lock(&clocks_lock);
> +	if (clk->parent)
> +		kref_get(&clk->parent->ref);
> +	list_add_tail(&clk->node, &clocks);
> +
> +	rc = clk_debugfs_init(clk);
> +	if (rc) {
> +		list_del(&clk->node);
> +		kref_put(&clk->ref, clk_release);
> +	}
> +
> +	spin_unlock(&clocks_lock);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(clk_register);
> +
> +void clk_unregister(struct clk *clk)
> +{
> +	spin_lock(&clocks_lock);
> +	clk_debugfs_clean(clk);
> +	list_del(&clk->node);
> +	kref_put(&clk->ref, clk_release);
> +	spin_unlock(&clocks_lock);
> +}
> +EXPORT_SYMBOL(clk_unregister);
> +
> +int clks_register(struct clk **clk, size_t num)
> +{
> +	int i;
> +	int rc;
> +	for (i = 0; i < num; i++) {
> +		rc = clk_register(clk[i]);
> +		if (rc)
> +			return rc;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(clks_register);
> +
> +void clks_unregister(struct clk **clk, size_t num)
> +{
> +	int i;
> +	for (i = 0; i < num; i++)
> +		clk_unregister(clk[i]);
> +}
> +EXPORT_SYMBOL(clks_unregister);
> +
> +struct clk *clk_get(struct device *dev, const char *id)
> +{
> +	struct clk *clk = NULL, *p;
> +
> +	spin_lock(&clocks_lock);
> +	list_for_each_entry(p, &clocks, node)
> +		if (strcmp(id, p->name) == 0 && p->ops->can_get(p, dev)) {
> +			clk = p;
> +			kref_get(&clk->ref);
> +			break;
> +		}
> +
> +	spin_unlock(&clocks_lock);
> +
> +	return clk;
> +}
> +EXPORT_SYMBOL(clk_get);
> +
> +void clk_put(struct clk *clk)
> +{
> +	kref_put(&clk->ref, clk_release);
> +}
> +EXPORT_SYMBOL(clk_put);
> +
> +int clk_enable(struct clk *clk)
> +{
> +	int rc = 0;
> +
> +	spin_lock(clk->lock);
> +
> +	clk->usage++;
> +	if (clk->usage == 1)
> +		rc = clk->ops->enable(clk);
> +
> +	if (rc)
> +		clk->usage--;

clk->usage = 0; is the same, maybe also easier to encase
the whole lot in the same if block:

	if (clk->usage == 1) {
		rc = clk->ops->enable(clk);

		if (rc)
			clk->usage = 0;
	}

> +
> +	spin_unlock(clk->lock);
> +
> +	return rc;
> +}
> +EXPORT_SYMBOL(clk_enable);
> +
> +void clk_disable(struct clk *clk)
> +{
> +	spin_lock(clk->lock);
> +
> +	WARN_ON(clk->usage <= 0);
> +
> +	clk->usage--;
> +	if (clk->usage == 0)
> +		clk->ops->disable(clk);
> +
> +	spin_unlock(clk->lock);
> +}
> +EXPORT_SYMBOL(clk_disable);
> +
> +unsigned long clk_get_rate(struct clk *clk)
> +{
> +	unsigned long hz;
> +
> +	spin_lock(clk->lock);
> +
> +	hz = clk->ops->get_rate(clk);
> +
> +	spin_unlock(clk->lock);
> +
> +	return hz;
> +}
> +EXPORT_SYMBOL(clk_get_rate);
> +
> +int clk_set_rate(struct clk *clk, unsigned long hz)
> +{
> +	long rc;
> +
> +	spin_lock(clk->lock);
> +
> +	rc = clk->ops->round_rate(clk, hz, 1);
> +
> +	spin_unlock(clk->lock);
> +
> +	return rc < 0 ? rc : 0;
> +}
> +EXPORT_SYMBOL(clk_set_rate);
> +
> +long clk_round_rate(struct clk *clk, unsigned long hz)
> +{
> +	long rc;
> +
> +	spin_lock(clk->lock);
> +
> +	rc = clk->ops->round_rate(clk, hz, 0);
> +
> +	spin_unlock(clk->lock);
> +
> +	return rc;
> +}

-- 
Ben (ben@fluff.org, http://www.fluff.org/)

  'a smiley only costs 4 bytes'

Re: [PATCH 1/3] Clocklib: add generic framework for managing clocks.

[Dmitry Baryshkov]

On Thu, Jun 26, 2008 at 05:00:26PM +0200, pHilipp Zabel wrote:
> On Thu, Jun 26, 2008 at 2:51 PM, Dmitry Baryshkov <dbaryshkov@gmail.com> wrote:
> > Provide a generic framework that platform may choose
> > to support clocks api. In particular this provides
> > platform-independant struct clk definition, a full
> > implementation of clocks api and a set of functions
> > for registering and unregistering clocks in a safe way.
> >
> > Signed-off-by: Dmitry Baryshkov <dbaryshkov@gmail.com>
> > ---
> >  include/linux/clocklib.h |   58 ++++++++
> >  lib/Kconfig              |    3 +
> >  lib/Makefile             |    1 +
> >  lib/clocklib.c           |  353 ++++++++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 415 insertions(+), 0 deletions(-)
> >  create mode 100644 include/linux/clocklib.h
> >  create mode 100644 lib/clocklib.c
> >
> > diff --git a/include/linux/clocklib.h b/include/linux/clocklib.h
> > new file mode 100644
> > index 0000000..cf2b41e
> > --- /dev/null
> > +++ b/include/linux/clocklib.h
> > @@ -0,0 +1,58 @@
> > +/*
> > + * Copyright (C) 2008 Dmitry Baryshkov
> > + *
> > + * This file is released under the GPL v2.
> > + */
> > +
> > +#ifndef CLKLIB_H
> > +#define CLKLIB_H
> > +
> > +#include <linux/spinlock.h>
> > +#include <linux/kref.h>
> > +
> > +struct clk;
> > +
> > +/**
> > + * struct clk_ops - generic clock management operations
> > + * @can_get: checks whether passed device can get this clock
> > + * @set_parent: reconfigures the clock to use specified parent
> > + * @set_mode: enable or disable specified clock
> > + * @get_rate: obtain the current clock rate of a specified clock
> > + * @set_rate: set the clock rate for a specified clock
> > + * @round_rate: adjust a reate to the exact rate a clock can provide
> > + *
> > + * This structure specifies clock operations that are used to configure
> > + * specific clock.
> > + */
> > +struct clk_ops {
> > +       int (*can_get)(struct clk *clk, struct device *dev);
> > +       int (*set_parent)(struct clk *clk, struct clk *parent);
> > +       int (*enable)(struct clk *clk);
> > +       void (*disable)(struct clk *clk);
> > +       unsigned long (*get_rate)(struct clk *clk);
> > +       long (*round_rate)(struct clk *clk, unsigned long hz, bool apply);
> > +};
> > +
> > +
> > +struct clk {
> > +       struct list_head node;
> > +       spinlock_t *lock;
> > +       struct kref ref;
> > +       int usage;
> > +#ifdef CONFIG_DEBUG_FS
> > +       struct dentry *dir;
> > +       struct dentry *info;
> > +#endif
> > +
> > +       const char *name;
> > +       struct clk *parent;
> > +       struct clk_ops *ops;
> > +       void (*release)(struct clk *clk);
> > +};
> > +
> > +int clk_register(struct clk *clk);
> > +void clk_unregister(struct clk *clk);
> > +int clks_register(struct clk **clk, size_t num);
> > +void clks_unregister(struct clk **clk, size_t num);
> > +
> > +#endif
> > diff --git a/lib/Kconfig b/lib/Kconfig
> > index 8cc8e87..592f5e1 100644
> > --- a/lib/Kconfig
> > +++ b/lib/Kconfig
> > @@ -13,6 +13,9 @@ config GENERIC_FIND_FIRST_BIT
> >  config GENERIC_FIND_NEXT_BIT
> >        def_bool n
> >
> > +config HAVE_CLOCKLIB
> > +       tristate
> > +
> >  config CRC_CCITT
> >        tristate "CRC-CCITT functions"
> >        help
> > diff --git a/lib/Makefile b/lib/Makefile
> > index 74b0cfb..cee74e1 100644
> > --- a/lib/Makefile
> > +++ b/lib/Makefile
> > @@ -37,6 +37,7 @@ obj-$(CONFIG_PLIST) += plist.o
> >  obj-$(CONFIG_DEBUG_PREEMPT) += smp_processor_id.o
> >  obj-$(CONFIG_DEBUG_LIST) += list_debug.o
> >  obj-$(CONFIG_DEBUG_OBJECTS) += debugobjects.o
> > +obj-$(CONFIG_HAVE_CLOCKLIB) += clocklib.o
> >
> >  ifneq ($(CONFIG_HAVE_DEC_LOCK),y)
> >   lib-y += dec_and_lock.o
> > diff --git a/lib/clocklib.c b/lib/clocklib.c
> > new file mode 100644
> > index 0000000..590a665
> > --- /dev/null
> > +++ b/lib/clocklib.c
> > @@ -0,0 +1,353 @@
> > +/*
> > + * Generic clocks API implementation
> > + *
> > + * Copyright (c) 2008 Dmitry Baryshkov
> > + *
> > + * This file is released under the GPL v2.
> > + */
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/clk.h>
> > +#include <linux/clocklib.h>
> > +#include <linux/spinlock.h>
> > +
> > +static LIST_HEAD(clocks);
> > +static DEFINE_SPINLOCK(clocks_lock);
> > +
> > +#ifdef CONFIG_DEBUG_FS
> > +#include <linux/debugfs.h>
> > +#include <linux/seq_file.h>
> > +static struct dentry *clkdir;
> > +
> > +static int clocklib_show(struct seq_file *s, void *data)
> > +{
> > +       struct clk *clk = s->private;
> > +
> > +       BUG_ON(!clk);
> > +
> > +       seq_printf(s, "set_parent=%savailable\nusage=%d/%d\nrate=%10lu Hz\n",
> > +                       clk->ops && clk->ops->set_parent ? "not " : "",
> > +                       clk->usage, atomic_read(&clk->ref.refcount),
> > +                       clk_get_rate(clk));
> > +//     if (clk->ops && clk->ops->format)
> > +//                     clk->ops->format(clk, s);
> 
> What about those?

That was an idea to be able to supply some additional info from a clock.
For now commented, but can (and probably will) be restored later.

> 
> > +
> > +       return 0;
> > +}
> > +
> > +static int clocklib_open(struct inode *inode, struct file *file)
> > +{
> > +       return single_open(file, clocklib_show, inode->i_private);
> > +}
> > +
> > +static struct file_operations clocklib_operations = {
> > +       .open           = clocklib_open,
> > +       .read           = seq_read,
> > +       .llseek         = seq_lseek,
> > +       .release        = single_release,
> > +};
> > +
> > +static int clk_debugfs_init(struct clk *clk)
> > +{
> > +       struct dentry *dir;
> > +       struct dentry *info;
> > +
> > +       if (!clkdir)
> > +               dump_stack();
> > +
> > +       dir = debugfs_create_dir(clk->name,
> > +                       clk->parent ? clk->parent->dir : clkdir);
> > +
> > +       if (IS_ERR(dir))
> > +               return PTR_ERR(dir);
> > +
> > +       info = debugfs_create_file("info", S_IFREG | S_IRUGO,
> > +                       dir, clk, &clocklib_operations);
> > +
> > +       if (IS_ERR(info)) {
> > +               debugfs_remove(dir);
> > +               return PTR_ERR(info);
> > +       }
> > +
> > +       clk->dir = dir;
> > +       clk->info = info;
> > +
> > +       return 0;
> > +}
> > +
> > +static void clk_debugfs_clean(struct clk *clk)
> > +{
> > +       if (clk->info)
> > +               debugfs_remove(clk->info);
> > +       clk->info = NULL;
> > +
> > +       if (clk->dir)
> > +               debugfs_remove(clk->dir);
> > +       clk->dir = NULL;
> > +}
> > +
> > +static void clk_debugfs_reparent(struct clk *clk, struct clk *old, struct clk *new)
> > +{
> > +       struct dentry *oldd = old ? old->dir : clkdir;
> > +       struct dentry *newd = new ? new->dir : clkdir;
> > +       struct dentry *dir = debugfs_rename(oldd, clk->dir, newd, clk->name);
> > +
> > +       if (IS_ERR(dir))
> > +               WARN_ON(1);
> > +       else
> > +               clk->dir = dir;
> > +}
> > +
> > +static int __init clocklib_debugfs_init(void)
> > +{
> > +       clkdir = debugfs_create_dir("clocks", NULL);
> > +       return 0;
> > +}
> > +core_initcall(clocklib_debugfs_init);
> > +#else
> > +#define clk_debugfs_init(clk) ({0;})
> > +#define clk_debugfs_clean(clk) do {} while (0);
> > +#define clk_debugfs_reparent(clk, old, new) do {} while (0);
> > +#endif
> > +
> > +static int clk_can_get_def(struct clk *clk, struct device *dev)
> > +{
> > +       return 1;
> > +}
> > +
> > +static unsigned long clk_get_rate_def(struct clk *clk)
> > +{
> > +       return 0;
> > +}
> > +
> > +static long clk_round_rate_def(struct clk *clk, unsigned long hz, bool apply)
> > +{
> > +       long rate = clk->ops->get_rate(clk);
> > +
> > +       if (apply && hz != rate)
> > +               return -EINVAL;
> > +
> > +       return rate;
> > +}
> > +
> > +static void clk_release(struct kref *ref)
> > +{
> > +       struct clk *clk = container_of(ref, struct clk, ref);
> > +
> > +       BUG_ON(!clk->release);
> > +
> > +       if (clk->parent)
> > +               kref_get(&clk->parent->ref);
> > +
> > +       clk->release(clk);
> > +}
> > +EXPORT_SYMBOL(clk_round_rate);
> > +
> > +struct clk* clk_get_parent(struct clk *clk)
> > +{
> > +       struct clk *parent;
> > +
> > +       spin_lock(clk->lock);
> > +
> > +       parent = clk->parent;
> > +       kref_get(&parent->ref);
> > +
> > +       spin_unlock(clk->lock);
> > +
> > +       return parent;
> > +}
> > +EXPORT_SYMBOL(clk_get_parent);
> > +
> > +int clk_set_parent(struct clk *clk, struct clk *parent)
> > +{
> > +       int rc = -EINVAL;
> > +       struct clk *old;
> > +
> > +       spin_lock(clk->lock);
> > +
> > +       if (!clk->ops->set_parent)
> > +               goto out;
> > +
> > +       old = clk->parent;
> > +
> > +       rc = clk->ops->set_parent(clk, parent);
> > +       if (rc)
> > +               goto out;
> > +
> > +       kref_get(&parent->ref);
> > +       clk->parent = parent;
> > +
> > +       clk_debugfs_reparent(clk, old, parent);
> > +
> > +       kref_put(&old->ref, clk_release);
> > +
> > +out:
> > +       spin_unlock(clk->lock);
> > +
> > +       return rc;
> > +}
> > +EXPORT_SYMBOL(clk_set_parent);
> > +
> > +int clk_register(struct clk *clk)
> > +{
> > +       int rc;
> > +
> > +       BUG_ON(!clk->ops);
> > +       BUG_ON(!clk->ops->enable || !clk->ops->disable);
> > +
> > +       if (!clk->ops->can_get)
> > +               clk->ops->can_get = clk_can_get_def;
> > +       if (!clk->ops->get_rate)
> > +               clk->ops->get_rate = clk_get_rate_def;
> > +       if (!clk->ops->round_rate)
> > +               clk->ops->round_rate = clk_round_rate_def;
> > +
> > +       kref_init(&clk->ref);
> > +
> > +       spin_lock(&clocks_lock);
> > +       if (clk->parent)
> > +               kref_get(&clk->parent->ref);
> > +       list_add_tail(&clk->node, &clocks);
> > +
> > +       rc = clk_debugfs_init(clk);
> > +       if (rc) {
> > +               list_del(&clk->node);
> > +               kref_put(&clk->ref, clk_release);
> > +       }
> > +
> > +       spin_unlock(&clocks_lock);
> > +
> > +       return 0;
> > +}
> > +EXPORT_SYMBOL(clk_register);
> > +
> > +void clk_unregister(struct clk *clk)
> > +{
> > +       spin_lock(&clocks_lock);
> > +       clk_debugfs_clean(clk);
> > +       list_del(&clk->node);
> > +       kref_put(&clk->ref, clk_release);
> > +       spin_unlock(&clocks_lock);
> > +}
> > +EXPORT_SYMBOL(clk_unregister);
> > +
> > +int clks_register(struct clk **clk, size_t num)
> > +{
> > +       int i;
> > +       int rc;
> > +       for (i = 0; i < num; i++) {
> > +               rc = clk_register(clk[i]);
> > +               if (rc)
> > +                       return rc;
> > +       }
> > +
> > +       return 0;
> > +}
> > +EXPORT_SYMBOL(clks_register);
> > +
> > +void clks_unregister(struct clk **clk, size_t num)
> > +{
> > +       int i;
> > +       for (i = 0; i < num; i++)
> > +               clk_unregister(clk[i]);
> > +}
> > +EXPORT_SYMBOL(clks_unregister);
> > +
> > +struct clk *clk_get(struct device *dev, const char *id)
> > +{
> > +       struct clk *clk = NULL, *p;
> > +
> > +       spin_lock(&clocks_lock);
> > +       list_for_each_entry(p, &clocks, node)
> > +               if (strcmp(id, p->name) == 0 && p->ops->can_get(p, dev)) {
> > +                       clk = p;
> > +                       kref_get(&clk->ref);
> > +                       break;
> > +               }
> > +
> > +       spin_unlock(&clocks_lock);
> > +
> > +       return clk;
> > +}
> > +EXPORT_SYMBOL(clk_get);
> > +
> > +void clk_put(struct clk *clk)
> > +{
> > +       kref_put(&clk->ref, clk_release);
> > +}
> > +EXPORT_SYMBOL(clk_put);
> > +
> > +int clk_enable(struct clk *clk)
> > +{
> > +       int rc = 0;
> > +
> > +       spin_lock(clk->lock);
> > +
> > +       clk->usage++;
> > +       if (clk->usage == 1)
> > +               rc = clk->ops->enable(clk);
> > +
> > +       if (rc)
> > +               clk->usage--;
> > +
> > +       spin_unlock(clk->lock);
> > +
> > +       return rc;
> > +}
> > +EXPORT_SYMBOL(clk_enable);
> > +
> > +void clk_disable(struct clk *clk)
> > +{
> > +       spin_lock(clk->lock);
> > +
> > +       WARN_ON(clk->usage <= 0);
> > +
> > +       clk->usage--;
> > +       if (clk->usage == 0)
> > +               clk->ops->disable(clk);
> > +
> > +       spin_unlock(clk->lock);
> > +}
> > +EXPORT_SYMBOL(clk_disable);
> > +
> > +unsigned long clk_get_rate(struct clk *clk)
> > +{
> > +       unsigned long hz;
> > +
> > +       spin_lock(clk->lock);
> > +
> > +       hz = clk->ops->get_rate(clk);
> > +
> > +       spin_unlock(clk->lock);
> > +
> > +       return hz;
> > +}
> > +EXPORT_SYMBOL(clk_get_rate);
> > +
> > +int clk_set_rate(struct clk *clk, unsigned long hz)
> > +{
> > +       long rc;
> > +
> > +       spin_lock(clk->lock);
> > +
> > +       rc = clk->ops->round_rate(clk, hz, 1);
> > +
> > +       spin_unlock(clk->lock);
> > +
> > +       return rc < 0 ? rc : 0;
> > +}
> > +EXPORT_SYMBOL(clk_set_rate);
> > +
> > +long clk_round_rate(struct clk *clk, unsigned long hz)
> > +{
> > +       long rc;
> > +
> > +       spin_lock(clk->lock);
> > +
> > +       rc = clk->ops->round_rate(clk, hz, 0);
> > +
> > +       spin_unlock(clk->lock);
> > +
> > +       return rc;
> > +}
> > +
> > --
> > 1.5.5.4
> >
> >
> > --
> > With best wishes
> > Dmitry
> >
> >
> 
> regards
> Philipp

-- 
With best wishes
Dmitry

Re: [PATCH 1/3] Clocklib: add generic framework for managing clocks.

[pHilipp Zabel]

On Thu, Jun 26, 2008 at 2:51 PM, Dmitry Baryshkov <dbaryshkov@gmail.com> wrote:
> Provide a generic framework that platform may choose
> to support clocks api. In particular this provides
> platform-independant struct clk definition, a full
> implementation of clocks api and a set of functions
> for registering and unregistering clocks in a safe way.
>
> Signed-off-by: Dmitry Baryshkov <dbaryshkov@gmail.com>
> ---
>  include/linux/clocklib.h |   58 ++++++++
>  lib/Kconfig              |    3 +
>  lib/Makefile             |    1 +
>  lib/clocklib.c           |  353 ++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 415 insertions(+), 0 deletions(-)
>  create mode 100644 include/linux/clocklib.h
>  create mode 100644 lib/clocklib.c
>
> diff --git a/include/linux/clocklib.h b/include/linux/clocklib.h
> new file mode 100644
> index 0000000..cf2b41e
> --- /dev/null
> +++ b/include/linux/clocklib.h
> @@ -0,0 +1,58 @@
> +/*
> + * Copyright (C) 2008 Dmitry Baryshkov
> + *
> + * This file is released under the GPL v2.
> + */
> +
> +#ifndef CLKLIB_H
> +#define CLKLIB_H
> +
> +#include <linux/spinlock.h>
> +#include <linux/kref.h>
> +
> +struct clk;
> +
> +/**
> + * struct clk_ops - generic clock management operations
> + * @can_get: checks whether passed device can get this clock
> + * @set_parent: reconfigures the clock to use specified parent
> + * @set_mode: enable or disable specified clock
> + * @get_rate: obtain the current clock rate of a specified clock
> + * @set_rate: set the clock rate for a specified clock
> + * @round_rate: adjust a reate to the exact rate a clock can provide
> + *
> + * This structure specifies clock operations that are used to configure
> + * specific clock.
> + */
> +struct clk_ops {
> +       int (*can_get)(struct clk *clk, struct device *dev);
> +       int (*set_parent)(struct clk *clk, struct clk *parent);
> +       int (*enable)(struct clk *clk);
> +       void (*disable)(struct clk *clk);
> +       unsigned long (*get_rate)(struct clk *clk);
> +       long (*round_rate)(struct clk *clk, unsigned long hz, bool apply);
> +};
> +
> +
> +struct clk {
> +       struct list_head node;
> +       spinlock_t *lock;
> +       struct kref ref;
> +       int usage;
> +#ifdef CONFIG_DEBUG_FS
> +       struct dentry *dir;
> +       struct dentry *info;
> +#endif
> +
> +       const char *name;
> +       struct clk *parent;
> +       struct clk_ops *ops;
> +       void (*release)(struct clk *clk);
> +};
> +
> +int clk_register(struct clk *clk);
> +void clk_unregister(struct clk *clk);
> +int clks_register(struct clk **clk, size_t num);
> +void clks_unregister(struct clk **clk, size_t num);
> +
> +#endif
> diff --git a/lib/Kconfig b/lib/Kconfig
> index 8cc8e87..592f5e1 100644
> --- a/lib/Kconfig
> +++ b/lib/Kconfig
> @@ -13,6 +13,9 @@ config GENERIC_FIND_FIRST_BIT
>  config GENERIC_FIND_NEXT_BIT
>        def_bool n
>
> +config HAVE_CLOCKLIB
> +       tristate
> +
>  config CRC_CCITT
>        tristate "CRC-CCITT functions"
>        help
> diff --git a/lib/Makefile b/lib/Makefile
> index 74b0cfb..cee74e1 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -37,6 +37,7 @@ obj-$(CONFIG_PLIST) += plist.o
>  obj-$(CONFIG_DEBUG_PREEMPT) += smp_processor_id.o
>  obj-$(CONFIG_DEBUG_LIST) += list_debug.o
>  obj-$(CONFIG_DEBUG_OBJECTS) += debugobjects.o
> +obj-$(CONFIG_HAVE_CLOCKLIB) += clocklib.o
>
>  ifneq ($(CONFIG_HAVE_DEC_LOCK),y)
>   lib-y += dec_and_lock.o
> diff --git a/lib/clocklib.c b/lib/clocklib.c
> new file mode 100644
> index 0000000..590a665
> --- /dev/null
> +++ b/lib/clocklib.c
> @@ -0,0 +1,353 @@
> +/*
> + * Generic clocks API implementation
> + *
> + * Copyright (c) 2008 Dmitry Baryshkov
> + *
> + * This file is released under the GPL v2.
> + */
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/clk.h>
> +#include <linux/clocklib.h>
> +#include <linux/spinlock.h>
> +
> +static LIST_HEAD(clocks);
> +static DEFINE_SPINLOCK(clocks_lock);
> +
> +#ifdef CONFIG_DEBUG_FS
> +#include <linux/debugfs.h>
> +#include <linux/seq_file.h>
> +static struct dentry *clkdir;
> +
> +static int clocklib_show(struct seq_file *s, void *data)
> +{
> +       struct clk *clk = s->private;
> +
> +       BUG_ON(!clk);
> +
> +       seq_printf(s, "set_parent=%savailable\nusage=%d/%d\nrate=%10lu Hz\n",
> +                       clk->ops && clk->ops->set_parent ? "not " : "",
> +                       clk->usage, atomic_read(&clk->ref.refcount),
> +                       clk_get_rate(clk));
> +//     if (clk->ops && clk->ops->format)
> +//                     clk->ops->format(clk, s);

What about those?

> +
> +       return 0;
> +}
> +
> +static int clocklib_open(struct inode *inode, struct file *file)
> +{
> +       return single_open(file, clocklib_show, inode->i_private);
> +}
> +
> +static struct file_operations clocklib_operations = {
> +       .open           = clocklib_open,
> +       .read           = seq_read,
> +       .llseek         = seq_lseek,
> +       .release        = single_release,
> +};
> +
> +static int clk_debugfs_init(struct clk *clk)
> +{
> +       struct dentry *dir;
> +       struct dentry *info;
> +
> +       if (!clkdir)
> +               dump_stack();
> +
> +       dir = debugfs_create_dir(clk->name,
> +                       clk->parent ? clk->parent->dir : clkdir);
> +
> +       if (IS_ERR(dir))
> +               return PTR_ERR(dir);
> +
> +       info = debugfs_create_file("info", S_IFREG | S_IRUGO,
> +                       dir, clk, &clocklib_operations);
> +
> +       if (IS_ERR(info)) {
> +               debugfs_remove(dir);
> +               return PTR_ERR(info);
> +       }
> +
> +       clk->dir = dir;
> +       clk->info = info;
> +
> +       return 0;
> +}
> +
> +static void clk_debugfs_clean(struct clk *clk)
> +{
> +       if (clk->info)
> +               debugfs_remove(clk->info);
> +       clk->info = NULL;
> +
> +       if (clk->dir)
> +               debugfs_remove(clk->dir);
> +       clk->dir = NULL;
> +}
> +
> +static void clk_debugfs_reparent(struct clk *clk, struct clk *old, struct clk *new)
> +{
> +       struct dentry *oldd = old ? old->dir : clkdir;
> +       struct dentry *newd = new ? new->dir : clkdir;
> +       struct dentry *dir = debugfs_rename(oldd, clk->dir, newd, clk->name);
> +
> +       if (IS_ERR(dir))
> +               WARN_ON(1);
> +       else
> +               clk->dir = dir;
> +}
> +
> +static int __init clocklib_debugfs_init(void)
> +{
> +       clkdir = debugfs_create_dir("clocks", NULL);
> +       return 0;
> +}
> +core_initcall(clocklib_debugfs_init);
> +#else
> +#define clk_debugfs_init(clk) ({0;})
> +#define clk_debugfs_clean(clk) do {} while (0);
> +#define clk_debugfs_reparent(clk, old, new) do {} while (0);
> +#endif
> +
> +static int clk_can_get_def(struct clk *clk, struct device *dev)
> +{
> +       return 1;
> +}
> +
> +static unsigned long clk_get_rate_def(struct clk *clk)
> +{
> +       return 0;
> +}
> +
> +static long clk_round_rate_def(struct clk *clk, unsigned long hz, bool apply)
> +{
> +       long rate = clk->ops->get_rate(clk);
> +
> +       if (apply && hz != rate)
> +               return -EINVAL;
> +
> +       return rate;
> +}
> +
> +static void clk_release(struct kref *ref)
> +{
> +       struct clk *clk = container_of(ref, struct clk, ref);
> +
> +       BUG_ON(!clk->release);
> +
> +       if (clk->parent)
> +               kref_get(&clk->parent->ref);
> +
> +       clk->release(clk);
> +}
> +EXPORT_SYMBOL(clk_round_rate);
> +
> +struct clk* clk_get_parent(struct clk *clk)
> +{
> +       struct clk *parent;
> +
> +       spin_lock(clk->lock);
> +
> +       parent = clk->parent;
> +       kref_get(&parent->ref);
> +
> +       spin_unlock(clk->lock);
> +
> +       return parent;
> +}
> +EXPORT_SYMBOL(clk_get_parent);
> +
> +int clk_set_parent(struct clk *clk, struct clk *parent)
> +{
> +       int rc = -EINVAL;
> +       struct clk *old;
> +
> +       spin_lock(clk->lock);
> +
> +       if (!clk->ops->set_parent)
> +               goto out;
> +
> +       old = clk->parent;
> +
> +       rc = clk->ops->set_parent(clk, parent);
> +       if (rc)
> +               goto out;
> +
> +       kref_get(&parent->ref);
> +       clk->parent = parent;
> +
> +       clk_debugfs_reparent(clk, old, parent);
> +
> +       kref_put(&old->ref, clk_release);
> +
> +out:
> +       spin_unlock(clk->lock);
> +
> +       return rc;
> +}
> +EXPORT_SYMBOL(clk_set_parent);
> +
> +int clk_register(struct clk *clk)
> +{
> +       int rc;
> +
> +       BUG_ON(!clk->ops);
> +       BUG_ON(!clk->ops->enable || !clk->ops->disable);
> +
> +       if (!clk->ops->can_get)
> +               clk->ops->can_get = clk_can_get_def;
> +       if (!clk->ops->get_rate)
> +               clk->ops->get_rate = clk_get_rate_def;
> +       if (!clk->ops->round_rate)
> +               clk->ops->round_rate = clk_round_rate_def;
> +
> +       kref_init(&clk->ref);
> +
> +       spin_lock(&clocks_lock);
> +       if (clk->parent)
> +               kref_get(&clk->parent->ref);
> +       list_add_tail(&clk->node, &clocks);
> +
> +       rc = clk_debugfs_init(clk);
> +       if (rc) {
> +               list_del(&clk->node);
> +               kref_put(&clk->ref, clk_release);
> +       }
> +
> +       spin_unlock(&clocks_lock);
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL(clk_register);
> +
> +void clk_unregister(struct clk *clk)
> +{
> +       spin_lock(&clocks_lock);
> +       clk_debugfs_clean(clk);
> +       list_del(&clk->node);
> +       kref_put(&clk->ref, clk_release);
> +       spin_unlock(&clocks_lock);
> +}
> +EXPORT_SYMBOL(clk_unregister);
> +
> +int clks_register(struct clk **clk, size_t num)
> +{
> +       int i;
> +       int rc;
> +       for (i = 0; i < num; i++) {
> +               rc = clk_register(clk[i]);
> +               if (rc)
> +                       return rc;
> +       }
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL(clks_register);
> +
> +void clks_unregister(struct clk **clk, size_t num)
> +{
> +       int i;
> +       for (i = 0; i < num; i++)
> +               clk_unregister(clk[i]);
> +}
> +EXPORT_SYMBOL(clks_unregister);
> +
> +struct clk *clk_get(struct device *dev, const char *id)
> +{
> +       struct clk *clk = NULL, *p;
> +
> +       spin_lock(&clocks_lock);
> +       list_for_each_entry(p, &clocks, node)
> +               if (strcmp(id, p->name) == 0 && p->ops->can_get(p, dev)) {
> +                       clk = p;
> +                       kref_get(&clk->ref);
> +                       break;
> +               }
> +
> +       spin_unlock(&clocks_lock);
> +
> +       return clk;
> +}
> +EXPORT_SYMBOL(clk_get);
> +
> +void clk_put(struct clk *clk)
> +{
> +       kref_put(&clk->ref, clk_release);
> +}
> +EXPORT_SYMBOL(clk_put);
> +
> +int clk_enable(struct clk *clk)
> +{
> +       int rc = 0;
> +
> +       spin_lock(clk->lock);
> +
> +       clk->usage++;
> +       if (clk->usage == 1)
> +               rc = clk->ops->enable(clk);
> +
> +       if (rc)
> +               clk->usage--;
> +
> +       spin_unlock(clk->lock);
> +
> +       return rc;
> +}
> +EXPORT_SYMBOL(clk_enable);
> +
> +void clk_disable(struct clk *clk)
> +{
> +       spin_lock(clk->lock);
> +
> +       WARN_ON(clk->usage <= 0);
> +
> +       clk->usage--;
> +       if (clk->usage == 0)
> +               clk->ops->disable(clk);
> +
> +       spin_unlock(clk->lock);
> +}
> +EXPORT_SYMBOL(clk_disable);
> +
> +unsigned long clk_get_rate(struct clk *clk)
> +{
> +       unsigned long hz;
> +
> +       spin_lock(clk->lock);
> +
> +       hz = clk->ops->get_rate(clk);
> +
> +       spin_unlock(clk->lock);
> +
> +       return hz;
> +}
> +EXPORT_SYMBOL(clk_get_rate);
> +
> +int clk_set_rate(struct clk *clk, unsigned long hz)
> +{
> +       long rc;
> +
> +       spin_lock(clk->lock);
> +
> +       rc = clk->ops->round_rate(clk, hz, 1);
> +
> +       spin_unlock(clk->lock);
> +
> +       return rc < 0 ? rc : 0;
> +}
> +EXPORT_SYMBOL(clk_set_rate);
> +
> +long clk_round_rate(struct clk *clk, unsigned long hz)
> +{
> +       long rc;
> +
> +       spin_lock(clk->lock);
> +
> +       rc = clk->ops->round_rate(clk, hz, 0);
> +
> +       spin_unlock(clk->lock);
> +
> +       return rc;
> +}
> +
> --
> 1.5.5.4
>
>
> --
> With best wishes
> Dmitry
>
>

regards
Philipp

[PATCH 1/3] Clocklib: add generic framework for managing clocks.

[Dmitry Baryshkov]

Provide a generic framework that platform may choose
to support clocks api. In particular this provides
platform-independant struct clk definition, a full
implementation of clocks api and a set of functions
for registering and unregistering clocks in a safe way.

Signed-off-by: Dmitry Baryshkov <dbaryshkov@gmail.com>
---
 include/linux/clocklib.h |   58 ++++++++
 lib/Kconfig              |    3 +
 lib/Makefile             |    1 +
 lib/clocklib.c           |  353 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 415 insertions(+), 0 deletions(-)
 create mode 100644 include/linux/clocklib.h
 create mode 100644 lib/clocklib.c

diff --git a/include/linux/clocklib.h b/include/linux/clocklib.h
new file mode 100644
index 0000000..cf2b41e
--- /dev/null
+++ b/include/linux/clocklib.h
@@ -0,0 +1,58 @@
+/*
+ * Copyright (C) 2008 Dmitry Baryshkov
+ *
+ * This file is released under the GPL v2.
+ */
+
+#ifndef CLKLIB_H
+#define CLKLIB_H
+
+#include <linux/spinlock.h>
+#include <linux/kref.h>
+
+struct clk;
+
+/**
+ * struct clk_ops - generic clock management operations
+ * @can_get: checks whether passed device can get this clock
+ * @set_parent: reconfigures the clock to use specified parent
+ * @set_mode: enable or disable specified clock
+ * @get_rate: obtain the current clock rate of a specified clock
+ * @set_rate: set the clock rate for a specified clock
+ * @round_rate: adjust a reate to the exact rate a clock can provide
+ *
+ * This structure specifies clock operations that are used to configure
+ * specific clock.
+ */
+struct clk_ops {
+	int (*can_get)(struct clk *clk, struct device *dev);
+	int (*set_parent)(struct clk *clk, struct clk *parent);
+	int (*enable)(struct clk *clk);
+	void (*disable)(struct clk *clk);
+	unsigned long (*get_rate)(struct clk *clk);
+	long (*round_rate)(struct clk *clk, unsigned long hz, bool apply);
+};
+
+
+struct clk {
+	struct list_head node;
+	spinlock_t *lock;
+	struct kref ref;
+	int usage;
+#ifdef CONFIG_DEBUG_FS
+	struct dentry *dir;
+	struct dentry *info;
+#endif
+
+	const char *name;
+	struct clk *parent;
+	struct clk_ops *ops;
+	void (*release)(struct clk *clk);
+};
+
+int clk_register(struct clk *clk);
+void clk_unregister(struct clk *clk);
+int clks_register(struct clk **clk, size_t num);
+void clks_unregister(struct clk **clk, size_t num);
+
+#endif
diff --git a/lib/Kconfig b/lib/Kconfig
index 8cc8e87..592f5e1 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -13,6 +13,9 @@ config GENERIC_FIND_FIRST_BIT
 config GENERIC_FIND_NEXT_BIT
 	def_bool n
 
+config HAVE_CLOCKLIB
+	tristate
+
 config CRC_CCITT
 	tristate "CRC-CCITT functions"
 	help
diff --git a/lib/Makefile b/lib/Makefile
index 74b0cfb..cee74e1 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -37,6 +37,7 @@ obj-$(CONFIG_PLIST) += plist.o
 obj-$(CONFIG_DEBUG_PREEMPT) += smp_processor_id.o
 obj-$(CONFIG_DEBUG_LIST) += list_debug.o
 obj-$(CONFIG_DEBUG_OBJECTS) += debugobjects.o
+obj-$(CONFIG_HAVE_CLOCKLIB) += clocklib.o
 
 ifneq ($(CONFIG_HAVE_DEC_LOCK),y)
   lib-y += dec_and_lock.o
diff --git a/lib/clocklib.c b/lib/clocklib.c
new file mode 100644
index 0000000..590a665
--- /dev/null
+++ b/lib/clocklib.c
@@ -0,0 +1,353 @@
+/*
+ * Generic clocks API implementation
+ *
+ * Copyright (c) 2008 Dmitry Baryshkov
+ *
+ * This file is released under the GPL v2.
+ */
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/clk.h>
+#include <linux/clocklib.h>
+#include <linux/spinlock.h>
+
+static LIST_HEAD(clocks);
+static DEFINE_SPINLOCK(clocks_lock);
+
+#ifdef CONFIG_DEBUG_FS
+#include <linux/debugfs.h>
+#include <linux/seq_file.h>
+static struct dentry *clkdir;
+
+static int clocklib_show(struct seq_file *s, void *data)
+{
+	struct clk *clk = s->private;
+
+	BUG_ON(!clk);
+
+	seq_printf(s, "set_parent=%savailable\nusage=%d/%d\nrate=%10lu Hz\n",
+			clk->ops && clk->ops->set_parent ? "not " : "",
+			clk->usage, atomic_read(&clk->ref.refcount),
+			clk_get_rate(clk));
+//	if (clk->ops && clk->ops->format)
+//			clk->ops->format(clk, s);
+
+	return 0;
+}
+
+static int clocklib_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, clocklib_show, inode->i_private);
+}
+
+static struct file_operations clocklib_operations = {
+	.open		= clocklib_open,
+	.read		= seq_read,
+	.llseek		= seq_lseek,
+	.release	= single_release,
+};
+
+static int clk_debugfs_init(struct clk *clk)
+{
+	struct dentry *dir;
+	struct dentry *info;
+
+	if (!clkdir)
+		dump_stack();
+
+	dir = debugfs_create_dir(clk->name,
+			clk->parent ? clk->parent->dir : clkdir);
+
+	if (IS_ERR(dir))
+		return PTR_ERR(dir);
+
+	info = debugfs_create_file("info", S_IFREG | S_IRUGO,
+			dir, clk, &clocklib_operations);
+
+	if (IS_ERR(info)) {
+		debugfs_remove(dir);
+		return PTR_ERR(info);
+	}
+
+	clk->dir = dir;
+	clk->info = info;
+
+	return 0;
+}
+
+static void clk_debugfs_clean(struct clk *clk)
+{
+	if (clk->info)
+		debugfs_remove(clk->info);
+	clk->info = NULL;
+
+	if (clk->dir)
+		debugfs_remove(clk->dir);
+	clk->dir = NULL;
+}
+
+static void clk_debugfs_reparent(struct clk *clk, struct clk *old, struct clk *new)
+{
+	struct dentry *oldd = old ? old->dir : clkdir;
+	struct dentry *newd = new ? new->dir : clkdir;
+	struct dentry *dir = debugfs_rename(oldd, clk->dir, newd, clk->name);
+
+	if (IS_ERR(dir))
+		WARN_ON(1);
+	else
+		clk->dir = dir;
+}
+
+static int __init clocklib_debugfs_init(void)
+{
+	clkdir = debugfs_create_dir("clocks", NULL);
+	return 0;
+}
+core_initcall(clocklib_debugfs_init);
+#else
+#define clk_debugfs_init(clk) ({0;})
+#define clk_debugfs_clean(clk) do {} while (0);
+#define clk_debugfs_reparent(clk, old, new) do {} while (0);
+#endif
+
+static int clk_can_get_def(struct clk *clk, struct device *dev)
+{
+	return 1;
+}
+
+static unsigned long clk_get_rate_def(struct clk *clk)
+{
+	return 0;
+}
+
+static long clk_round_rate_def(struct clk *clk, unsigned long hz, bool apply)
+{
+	long rate = clk->ops->get_rate(clk);
+
+	if (apply && hz != rate)
+		return -EINVAL;
+
+	return rate;
+}
+
+static void clk_release(struct kref *ref)
+{
+	struct clk *clk = container_of(ref, struct clk, ref);
+
+	BUG_ON(!clk->release);
+
+	if (clk->parent)
+		kref_get(&clk->parent->ref);
+
+	clk->release(clk);
+}
+EXPORT_SYMBOL(clk_round_rate);
+
+struct clk* clk_get_parent(struct clk *clk)
+{
+	struct clk *parent;
+
+	spin_lock(clk->lock);
+
+	parent = clk->parent;
+	kref_get(&parent->ref);
+
+	spin_unlock(clk->lock);
+
+	return parent;
+}
+EXPORT_SYMBOL(clk_get_parent);
+
+int clk_set_parent(struct clk *clk, struct clk *parent)
+{
+	int rc = -EINVAL;
+	struct clk *old;
+
+	spin_lock(clk->lock);
+
+	if (!clk->ops->set_parent)
+		goto out;
+
+	old = clk->parent;
+
+	rc = clk->ops->set_parent(clk, parent);
+	if (rc)
+		goto out;
+
+	kref_get(&parent->ref);
+	clk->parent = parent;
+
+	clk_debugfs_reparent(clk, old, parent);
+
+	kref_put(&old->ref, clk_release);
+
+out:
+	spin_unlock(clk->lock);
+
+	return rc;
+}
+EXPORT_SYMBOL(clk_set_parent);
+
+int clk_register(struct clk *clk)
+{
+	int rc;
+
+	BUG_ON(!clk->ops);
+	BUG_ON(!clk->ops->enable || !clk->ops->disable);
+
+	if (!clk->ops->can_get)
+		clk->ops->can_get = clk_can_get_def;
+	if (!clk->ops->get_rate)
+		clk->ops->get_rate = clk_get_rate_def;
+	if (!clk->ops->round_rate)
+		clk->ops->round_rate = clk_round_rate_def;
+
+	kref_init(&clk->ref);
+
+	spin_lock(&clocks_lock);
+	if (clk->parent)
+		kref_get(&clk->parent->ref);
+	list_add_tail(&clk->node, &clocks);
+
+	rc = clk_debugfs_init(clk);
+	if (rc) {
+		list_del(&clk->node);
+		kref_put(&clk->ref, clk_release);
+	}
+
+	spin_unlock(&clocks_lock);
+
+	return 0;
+}
+EXPORT_SYMBOL(clk_register);
+
+void clk_unregister(struct clk *clk)
+{
+	spin_lock(&clocks_lock);
+	clk_debugfs_clean(clk);
+	list_del(&clk->node);
+	kref_put(&clk->ref, clk_release);
+	spin_unlock(&clocks_lock);
+}
+EXPORT_SYMBOL(clk_unregister);
+
+int clks_register(struct clk **clk, size_t num)
+{
+	int i;
+	int rc;
+	for (i = 0; i < num; i++) {
+		rc = clk_register(clk[i]);
+		if (rc)
+			return rc;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(clks_register);
+
+void clks_unregister(struct clk **clk, size_t num)
+{
+	int i;
+	for (i = 0; i < num; i++)
+		clk_unregister(clk[i]);
+}
+EXPORT_SYMBOL(clks_unregister);
+
+struct clk *clk_get(struct device *dev, const char *id)
+{
+	struct clk *clk = NULL, *p;
+
+	spin_lock(&clocks_lock);
+	list_for_each_entry(p, &clocks, node)
+		if (strcmp(id, p->name) == 0 && p->ops->can_get(p, dev)) {
+			clk = p;
+			kref_get(&clk->ref);
+			break;
+		}
+
+	spin_unlock(&clocks_lock);
+
+	return clk;
+}
+EXPORT_SYMBOL(clk_get);
+
+void clk_put(struct clk *clk)
+{
+	kref_put(&clk->ref, clk_release);
+}
+EXPORT_SYMBOL(clk_put);
+
+int clk_enable(struct clk *clk)
+{
+	int rc = 0;
+
+	spin_lock(clk->lock);
+
+	clk->usage++;
+	if (clk->usage == 1)
+		rc = clk->ops->enable(clk);
+
+	if (rc)
+		clk->usage--;
+
+	spin_unlock(clk->lock);
+
+	return rc;
+}
+EXPORT_SYMBOL(clk_enable);
+
+void clk_disable(struct clk *clk)
+{
+	spin_lock(clk->lock);
+
+	WARN_ON(clk->usage <= 0);
+
+	clk->usage--;
+	if (clk->usage == 0)
+		clk->ops->disable(clk);
+
+	spin_unlock(clk->lock);
+}
+EXPORT_SYMBOL(clk_disable);
+
+unsigned long clk_get_rate(struct clk *clk)
+{
+	unsigned long hz;
+
+	spin_lock(clk->lock);
+
+	hz = clk->ops->get_rate(clk);
+
+	spin_unlock(clk->lock);
+
+	return hz;
+}
+EXPORT_SYMBOL(clk_get_rate);
+
+int clk_set_rate(struct clk *clk, unsigned long hz)
+{
+	long rc;
+
+	spin_lock(clk->lock);
+
+	rc = clk->ops->round_rate(clk, hz, 1);
+
+	spin_unlock(clk->lock);
+
+	return rc < 0 ? rc : 0;
+}
+EXPORT_SYMBOL(clk_set_rate);
+
+long clk_round_rate(struct clk *clk, unsigned long hz)
+{
+	long rc;
+
+	spin_lock(clk->lock);
+
+	rc = clk->ops->round_rate(clk, hz, 0);
+
+	spin_unlock(clk->lock);
+
+	return rc;
+}
+
-- 
1.5.5.4


-- 
With best wishes
Dmitry