[PATCH 0/5] Clocklib: generic clocks framework

[PATCH 0/5] Clocklib: generic clocks framework

[Dmitry Baryshkov]

Hi,

Here is my current version of the patchset.
Changes since last repost:

 * Clean up stuff noted by akpm.
 * Provide debugfs file description
 * Drop SIRCLK name, bind pxaficp driver to the STUARTCLK
 * Drop 3.6MHz clock renames
 * Rebase pxa part to latest Russel's arm:devel patchset

-- 
With best wishes
Dmitry

[PATCH 0/5] Clocklib: generic clocks framework

[Dmitry Baryshkov]

Hi,

Here is my latest version of the patchset.
I'd like to ask for it's inclusion into mainline.
Russell, since currently it's only ARM-only thing, I'd like to ask you
for your opinion on this.


Issues addressed in this version:

 * Simplify clk_function declarations but not messing with a lot of
   pointers.
 * Drop FUNC_TO_CLK as it's totally unused now.
 * Drop spurious kfree from the clk_alloc_function error path.

-- 
With best wishes
Dmitry

[PATCH 0/5] Clocklib: generic clocks framework

[Dmitry Baryshkov]

Hi,

Here is my latest version of the patchset.
I'd like to ask for it's inclusion into mainline.
Russell, since currently it's only ARM-only thing, I'd like to ask you
for your opinion on this.


Issues addressed in this version:

 * Simplify clk_function declarations but not messing with a lot of
   pointers.
 * Drop FUNC_TO_CLK as it's totally unused now.
 * Drop spurious kfree from the clk_alloc_function error path.

-- 
With best wishes
Dmitry

[PATCH 1/5] 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>
---
 MAINTAINERS            |    5 +
 include/linux/clklib.h |   91 +++++++++++
 init/Kconfig           |    7 +
 kernel/Makefile        |    1 +
 kernel/clklib.c        |  392 ++++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 496 insertions(+), 0 deletions(-)
 create mode 100644 include/linux/clklib.h
 create mode 100644 kernel/clklib.c

diff --git a/MAINTAINERS b/MAINTAINERS
index c2c4cb6..7bc6512 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1083,6 +1083,11 @@ P:	Joel Schopp
 M:	jschopp@austin.ibm.com
 S:	Supported
 
+CLOCKLIB INFRASTRUCTURE
+P:	Dmitry Baryshkov
+M:	dbaryshkov@gmail.com
+S:	Maintained
+
 COMMON INTERNET FILE SYSTEM (CIFS)
 P:	Steve French
 M:	sfrench@samba.org
diff --git a/include/linux/clklib.h b/include/linux/clklib.h
new file mode 100644
index 0000000..f0af195
--- /dev/null
+++ b/include/linux/clklib.h
@@ -0,0 +1,91 @@
+/*
+ * 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_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 (*set_mode)(struct clk *clk, bool enable);
+	unsigned long (*get_rate)(struct clk *clk);
+	int (*set_rate)(struct clk *, unsigned long);
+	long (*round_rate)(struct clk *, unsigned long);
+};
+
+/**
+ * struct clk - generic struct clk implementation used in the clocklib
+ * @node: used to place all clocks in a list
+ * @parent: the parent clock
+ * @owner: module holding all the functions
+ * @name: the name of this clock
+ * @users: count how many users have enabled this clock
+ * @ops: a pointer to clock management operations
+ */
+struct clk {
+	struct list_head node;
+	struct clk	*parent;
+	struct module *owner;
+
+	const char	*name;
+	int		users;
+
+	const struct clk_ops *ops;
+};
+
+int __must_check clk_register(struct clk *clk);
+void clk_unregister(struct clk *clk);
+
+int __must_check clks_register(struct clk **clks, size_t num);
+void clks_unregister(struct clk **clks, size_t num);
+
+/*
+ * struct clk_function - helper that allows easy registration of clock "functions"
+ * @parent: the name of the parent clock
+ * @clk: the clock that will be set up and installed
+ *
+ * Sometimes single clock will have multiple users or several clocks
+ * will be bound to similar devices. This allows one to register
+ * simple wrapper clocks that serve only naming purposes.
+ */
+struct clk_function {
+	const char *parent;
+	void *priv;
+	struct clk clk;
+};
+
+#define CLK_FUNC(_clock, _function, _ops, _data)	\
+	(struct clk_function) {						\
+		.parent = _clock,			\
+		.priv	= _data,			\
+		.clk = {				\
+			.name = _function,		\
+			.owner = THIS_MODULE,		\
+			.ops = _ops,			\
+		},					\
+	}
+
+int __must_check clk_alloc_function(const char *parent, struct clk *clk);
+int __must_check clk_alloc_functions(struct clk_function *funcs, int num);
+void clk_free_functions(struct clk_function *funcs, int num);
+
+#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..7b82554
--- /dev/null
+++ b/kernel/clklib.c
@@ -0,0 +1,392 @@
+/*
+ * kernel/clklib.c
+ *
+ * Copyright (C) 2008 Dmitry Baryshkov
+ *
+ * Generic clocks API implementation
+ *
+ * This file is released under the GPL v2.
+ */
+#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);
+
+void clks_unregister(struct clk **clks, size_t num)
+{
+	int i;
+	for (i = num - 1; i >= 0;  i++)
+		clk_unregister(clks[i]);
+}
+EXPORT_SYMBOL(clks_unregister);
+
+int __must_check 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);
+
+	return ret;
+}
+EXPORT_SYMBOL(clks_register);
+
+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->ops && p->ops->can_get && p->ops->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->ops || !p->ops->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) || !clk->ops || !clk->ops->set_parent)
+		return -EINVAL;
+
+	spin_lock_irqsave(&clocks_lock, flags);
+
+	rc = clk->ops->set_parent(clk, parent);
+	if (!rc)
+		clk->parent = parent;
+
+	spin_unlock_irqrestore(&clocks_lock, flags);
+
+	return rc;
+}
+EXPORT_SYMBOL(clk_set_parent);
+
+struct clk *clk_get_parent(struct clk *clk)
+{
+	struct clk *parent;
+	unsigned long flags;
+
+	spin_lock_irqsave(&clocks_lock, flags);
+
+	parent = clk->parent;
+
+	spin_unlock_irqrestore(&clocks_lock, flags);
+
+	return parent;
+
+}
+
+static void __clk_disable(struct clk *clk)
+{
+	if (clk->users <= 0) {
+		WARN_ON(1);
+		return;
+	}
+
+	if (--clk->users == 0)
+		if (clk->ops && clk->ops->set_mode)
+			clk->ops->set_mode(clk, false);
+
+	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->ops && clk->ops->set_mode) {
+		rc = clk->ops->set_mode(clk, true);
+		if (rc) {
+			if (clk->parent)
+				__clk_disable(clk->parent);
+
+			return rc;
+		}
+	}
+
+	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;
+
+	while (clk) {
+		if (clk->ops && clk->ops->get_rate) {
+			rate = clk->ops->get_rate(clk);
+			break;
+		} else
+			clk = clk->parent;
+	}
+
+	return rate;
+}
+
+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 = 0;
+	unsigned long flags;
+	struct clk *pclk;
+
+	if (!clk || IS_ERR(clk))
+		return -EINVAL;
+
+	spin_lock_irqsave(&clocks_lock, flags);
+
+	for (pclk = clk; !res && pclk ; pclk = pclk->parent) {
+		if (pclk->ops && pclk->ops->round_rate)
+			res = pclk->ops->round_rate(clk, rate);
+	}
+
+	if (!res)
+		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->ops && clk->ops->set_rate) {
+			rc = clk->ops->set_rate(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:
+	spin_unlock_irqrestore(&clocks_lock, flags);
+
+	return rc;
+}
+EXPORT_SYMBOL(clk_alloc_function);
+
+void clk_free_functions(
+		struct clk_function *funcs,
+		int num)
+{
+	int i;
+
+	for (i = num - 1; i >= 0; i--)
+		clk_unregister(&funcs[i].clk);
+}
+EXPORT_SYMBOL(clk_free_functions);
+
+int __must_check 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;
+}
+EXPORT_SYMBOL(clk_alloc_functions);
-- 
1.5.4.4


-- 
With best wishes
Dmitry

[PATCH 2/5] Clocklib: debugfs support

[Dmitry Baryshkov]

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

Signed-off-by: Dmitry Baryshkov <dbaryshkov@gmail.com>
---
 Documentation/ABI/testing/debugfs-clock |   49 ++++++++++++++++++++
 include/linux/clklib.h                  |    2 +
 kernel/clklib.c                         |   74 +++++++++++++++++++++++++++++++
 3 files changed, 125 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/ABI/testing/debugfs-clock

diff --git a/Documentation/ABI/testing/debugfs-clock b/Documentation/ABI/testing/debugfs-clock
new file mode 100644
index 0000000..79196e9
--- /dev/null
+++ b/Documentation/ABI/testing/debugfs-clock
@@ -0,0 +1,49 @@
+What:           /debug/clock
+Date:           Apr. 2008
+KernelVersion:  2.6.26
+Contact:        Dmitry Baryshkov <dbaryshkov@gmail.com>
+Description:
+
+debugfs interface
+-----------------
+		The /debug/clock file displays information about registered
+		clocks, their rates, usage counters, etc.
+
+Example:
+-------
+cat /sys/kernel/debug/clock
+
+GPIO11_CLK            use=4 rate=   3686400 Hz
+  3_6MHz_CLK          use=4 rate=   3686400 Hz
+    32K_CLK           use=1 rate=     32768 Hz
+      MMCCLK          use=1 rate=     32768 Hz for cell tmio-mmc
+    TMIOUSBCLK        use=1 rate=         0 Hz
+      USBCLK          use=1 rate=         0 Hz for cell tmio-ohci
+    TMIOMCLK          use=1 rate=  48000000 Hz
+      FBCLK           use=1 rate=  48000000 Hz for cell tmio-fb
+HWUARTCLK             use=0 rate=  14745600 Hz
+  UARTCLK             use=0 rate=  14745600 Hz for device pxa2xx-uart.3
+LCDCLK                use=0 rate=  99530000 Hz
+FFUARTCLK             use=1 rate=  14745600 Hz
+  UARTCLK             use=1 rate=  14745600 Hz for device pxa2xx-uart.0
+BTUARTCLK             use=0 rate=  14745600 Hz
+  UARTCLK             use=0 rate=  14745600 Hz for device pxa2xx-uart.1
+STUARTCLK             use=0 rate=  14745600 Hz
+  UARTCLK             use=0 rate=  14745600 Hz for device pxa2xx-uart.2
+  SIRCLK              use=0 rate=  14745600 Hz
+UDCCLK                use=0 rate=  47923000 Hz
+PXAMMCCLK             use=0 rate=  19169000 Hz
+  MMCCLK              use=0 rate=  19169000 Hz for device pxa2xx-mci.0
+I2CCLK                use=1 rate=  31949000 Hz
+SSP_CLK               use=0 rate=   3686400 Hz
+  SSPCLK              use=0 rate=   3686400 Hz for device pxa25x-ssp.0
+NSSPCLK               use=1 rate=   3686400 Hz
+  SSPCLK              use=1 rate=   3686400 Hz for device pxa25x-nssp.1
+ASSPCLK               use=0 rate=   3686400 Hz
+  SSPCLK              use=0 rate=   3686400 Hz for device pxa25x-nssp.2
+PWM0CLK               use=0 rate=   3686400 Hz
+  PWMCLK              use=0 rate=   3686400 Hz for device pxa25x-pwm.0
+PWM1CLK               use=0 rate=   3686400 Hz
+  PWMCLK              use=0 rate=   3686400 Hz for device pxa25x-pwm.1
+AC97CLK               use=0 rate=  24576000 Hz
+FICPCLK               use=0 rate=  47923000 Hz
diff --git a/include/linux/clklib.h b/include/linux/clklib.h
index f0af195..3bc8886 100644
--- a/include/linux/clklib.h
+++ b/include/linux/clklib.h
@@ -19,6 +19,7 @@ struct seq_file;
  * @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
+ * @format: output any additional information for a clock
  *
  * This structure specifies clock operations that are used to configure
  * specific clock.
@@ -30,6 +31,7 @@ struct clk_ops {
 	unsigned long (*get_rate)(struct clk *clk);
 	int (*set_rate)(struct clk *, unsigned long);
 	long (*round_rate)(struct clk *, unsigned long);
+	int (*format)(struct clk *, struct seq_file *);
 };
 
 /**
diff --git a/kernel/clklib.c b/kernel/clklib.c
index 7b82554..9234c99 100644
--- a/kernel/clklib.c
+++ b/kernel/clklib.c
@@ -390,3 +390,77 @@ int __must_check clk_alloc_functions(
 	return 0;
 }
 EXPORT_SYMBOL(clk_alloc_functions);
+
+#ifdef CONFIG_DEBUG_FS
+
+#include <linux/debugfs.h>
+#include <linux/seq_file.h>
+
+#define NAME_FIELD_LEN	20
+
+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_putc(s, ' ');
+			}
+			seq_puts(s, clk->name);
+
+			i = 2 * nest + strlen(clk->name);
+			if (i >= NAME_FIELD_LEN)
+				i = NAME_FIELD_LEN - 1;
+			for (; i < NAME_FIELD_LEN; i++) {
+				seq_putc(s, ' ');
+			}
+			seq_printf(s, "%c use=%d rate=%10lu Hz",
+				clk->ops && clk->ops->set_parent ? '*' : ' ',
+				clk->users,
+				__clk_get_rate(clk));
+			if (clk->ops && clk->ops->format)
+				clk->ops->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/5] 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 |  124 ++++++++----------------------------------
 2 files changed, 24 insertions(+), 101 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index cb79d50..113ead3 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -416,6 +416,7 @@ config ARCH_RPC
 	select ISA_DMA_API
 	select NO_IOPORT
 	select HAVE_IDE
+	select HAVE_CLOCK_LIB
 	help
 	  On the Acorn Risc-PC, Linux can support the internal IDE disk and
 	  CD-ROM interface, serial and parallel port, and the floppy drive.
diff --git a/arch/arm/mach-sa1100/clock.c b/arch/arm/mach-sa1100/clock.c
index fc97fe5..715d91c 100644
--- a/arch/arm/mach-sa1100/clock.c
+++ b/arch/arm/mach-sa1100/clock.c
@@ -8,127 +8,49 @@
 #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_set_enable(struct clk *clk, bool enable)
 {
 	/*
 	 * First, set up the 3.6864MHz clock on GPIO 27 for the SA-1111:
 	 * (SA-1110 Developer's Manual, section 9.1.2.1)
 	 */
-	GAFR |= GPIO_32_768kHz;
-	GPDR |= GPIO_32_768kHz;
-	TUCR = TUCR_3_6864MHz;
+	if (enable) {
+		GAFR |= GPIO_32_768kHz;
+		GPDR |= GPIO_32_768kHz;
+		TUCR = TUCR_3_6864MHz;
+	} else {
+		TUCR = 0;
+		GPDR &= ~GPIO_32_768kHz;
+		GAFR &= ~GPIO_32_768kHz;
+	}
+
+	return 0;
 }
 
-static void clk_gpio27_disable(void)
+static unsigned long clk_gpio27_get_rate(struct clk *clk)
 {
-	TUCR = 0;
-	GPDR &= ~GPIO_32_768kHz;
-	GAFR &= ~GPIO_32_768kHz;
+	return 3686400;
 }
 
+static struct clk_ops clk_gpio27_ops = {
+	.get_rate	= clk_gpio27_get_rate,
+	.set_mode	= clk_gpio27_set_enable,
+};
+
 static struct clk clk_gpio27 = {
 	.name		= "GPIO27_CLK",
-	.rate		= 3686400,
-	.enable		= clk_gpio27_enable,
-	.disable	= clk_gpio27_disable,
+	.ops		= &clk_gpio27_ops,
+	.owner		= THIS_MODULE,
 };
 
-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

[PATCH 4/5] 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  |  147 ++++++++++++------------------------
 arch/arm/mach-pxa/clock.h  |   63 +++++++---------
 arch/arm/mach-pxa/pxa25x.c |   79 ++++++++++++-------
 arch/arm/mach-pxa/pxa27x.c |   74 ++++++++++--------
 arch/arm/mach-pxa/pxa3xx.c |  182 ++++++++++++++++++++++++-------------------
 6 files changed, 271 insertions(+), 275 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 113ead3..7ade486 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -404,6 +404,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 e97dc59..74fedfb 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>
@@ -20,135 +21,83 @@
 #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)
+static int clk_gpio11_set_mode(struct clk *clk, bool enable)
 {
-	struct clk *p;
-
-	list_for_each_entry(p, &clocks, node)
-		if (strcmp(id, p->name) == 0 && p->dev == dev)
-			return p;
+	if (enable)
+		pxa_gpio_mode(GPIO11_3_6MHz_MD);
 
-	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;
+	return 0;
 }
-EXPORT_SYMBOL(clk_get);
 
-void clk_put(struct clk *clk)
+static unsigned long clk_gpio11_get_rate(struct clk *clk)
 {
+	return 3686400;
 }
-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);
+static const struct clk_ops clk_gpio11_ops = {
+	.get_rate	= clk_gpio11_get_rate,
+	.set_mode	= clk_gpio11_set_mode,
+};
 
-	return 0;
-}
-EXPORT_SYMBOL(clk_enable);
+static struct clk clk_gpio11 = {
+	/* For backwards compatibility untill sa1111 and boards are fixed */
+	.name		= "GPIO27_CLK",
+	.ops		= &clk_gpio11_ops,
+	.owner		= THIS_MODULE,
+};
 
-void clk_disable(struct clk *clk)
+int clk_cken_set_mode(struct clk *clk, bool enable)
 {
-	unsigned long flags;
+	struct clk_cken *cclk = container_of(clk, struct clk_cken, clk);
+	int cken = cclk->cken;
 
-	WARN_ON(clk->enabled == 0);
+	if (enable) {
+		CKEN |= 1 << cken;
+		if (cclk->delay)
+			udelay(cclk->delay);
+	} else
+		CKEN &= ~(1 << cken);
 
-	spin_lock_irqsave(&clocks_lock, flags);
-	if (--clk->enabled == 0)
-		clk->ops->disable(clk);
-	spin_unlock_irqrestore(&clocks_lock, flags);
+	return 0;
 }
-EXPORT_SYMBOL(clk_disable);
 
-unsigned long clk_get_rate(struct clk *clk)
+unsigned long clk_cken_get_rate(struct clk *clk)
 {
-	unsigned long rate;
-
-	rate = clk->rate;
-	if (clk->ops->getrate)
-		rate = clk->ops->getrate(clk);
+	struct clk_cken *cclk = container_of(clk, struct clk_cken, clk);
 
-	return rate;
+	return cclk->rate;
 }
-EXPORT_SYMBOL(clk_get_rate);
 
+const struct clk_ops clk_cken_ops = {
+	.set_mode = clk_cken_set_mode,
+	.get_rate = clk_cken_get_rate,
+};
 
-static void clk_gpio27_enable(struct clk *clk)
+static int clk_dev_can_get(struct clk *clk, struct device *dev)
 {
-	pxa_gpio_mode(GPIO11_3_6MHz_MD);
-}
+	struct clk_function *cfunc = container_of(clk, struct clk_function, clk);
 
-static void clk_gpio27_disable(struct clk *clk)
-{
+	return (dev == cfunc->priv);
 }
 
-static const struct clkops clk_gpio27_ops = {
-	.enable		= clk_gpio27_enable,
-	.disable	= clk_gpio27_disable,
-};
-
-
-void clk_cken_enable(struct clk *clk)
+static int clk_dev_format(struct clk *clk, struct seq_file *s)
 {
-	CKEN |= 1 << clk->cken;
-}
+	struct clk_function *cfunc = container_of(clk, struct clk_function, clk);
 
-void clk_cken_disable(struct clk *clk)
-{
-	CKEN &= ~(1 << clk->cken);
-}
+	BUG_ON(!cfunc->priv);
 
-const struct clkops clk_cken_ops = {
-	.enable		= clk_cken_enable,
-	.disable	= clk_cken_disable,
-};
+	seq_puts(s, " for device ");
+	seq_puts(s, ((struct device *)cfunc->priv)->bus_id);
+	return 0;
+}
 
-static struct clk common_clks[] = {
-	{
-		.name		= "GPIO27_CLK",
-		.ops		= &clk_gpio27_ops,
-		.rate		= 3686400,
-	},
+const struct clk_ops clk_dev_ops = {
+	.can_get = clk_dev_can_get,
+	.format = clk_dev_format,
 };
 
-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 clk_register(&clk_gpio11);
 }
 arch_initcall(clk_init);
diff --git a/arch/arm/mach-pxa/clock.h b/arch/arm/mach-pxa/clock.h
index bc6b77e..976cf8a 100644
--- a/arch/arm/mach-pxa/clock.h
+++ b/arch/arm/mach-pxa/clock.h
@@ -1,43 +1,36 @@
-struct clk;
+#include <linux/clk.h>
+#include <linux/clklib.h>
+#include <linux/seq_file.h>
 
-struct clkops {
-	void			(*enable)(struct clk *);
-	void			(*disable)(struct clk *);
-	unsigned long		(*getrate)(struct clk *);
-};
+extern int clk_cken_set_mode(struct clk *clk, bool enable);
+extern unsigned long clk_cken_get_rate(struct clk *clk);
+extern const struct clk_ops clk_cken_ops;
 
-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;
+struct clk_cken {
+	unsigned int	cken;
+	unsigned long	rate;
+	int		delay;
+	struct clk	clk;
 };
 
-#define INIT_CKEN(_name, _cken, _rate, _delay, _dev)	\
-	{						\
-		.name	= _name,			\
-		.dev	= _dev,				\
-		.ops	= &clk_cken_ops,		\
+#define INIT_CKEN(_name, _cken, _rate, _delay)		\
+	&(struct clk_cken) {				\
+		.cken = CKEN_##_cken,			\
 		.rate	= _rate,			\
-		.cken	= CKEN_##_cken,			\
 		.delay	= _delay,			\
-	}
-
-#define INIT_CK(_name, _cken, _ops, _dev)		\
-	{						\
-		.name	= _name,			\
-		.dev	= _dev,				\
-		.ops	= _ops,				\
-		.cken	= CKEN_##_cken,			\
-	}
-
-extern const struct clkops clk_cken_ops;
+		.clk = {				\
+			.name	= _name,		\
+			.ops	= &clk_cken_ops,	\
+		},					\
+	}.clk
 
-void clk_cken_enable(struct clk *clk);
-void clk_cken_disable(struct clk *clk);
+#define INIT_CK(_name, _cken, _ops)			\
+	&(struct clk_cken) {				\
+		.cken = CKEN_##_cken,			\
+		.clk = {				\
+			.name	= _name,		\
+			.ops	= _ops,			\
+		},					\
+	}.clk
 
-void clks_register(struct clk *clks, size_t num);
+extern const struct clk_ops clk_dev_ops;
diff --git a/arch/arm/mach-pxa/pxa25x.c b/arch/arm/mach-pxa/pxa25x.c
index e802c79..9f25ca3 100644
--- a/arch/arm/mach-pxa/pxa25x.c
+++ b/arch/arm/mach-pxa/pxa25x.c
@@ -102,10 +102,9 @@ 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,
+static const struct clk_ops clk_pxa25x_lcd_ops = {
+	.set_mode = clk_cken_set_mode,
+	.get_rate = clk_pxa25x_lcd_getrate,
 };
 
 /*
@@ -113,31 +112,46 @@ static const struct clkops clk_pxa25x_lcd_ops = {
  * 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_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_CKEN("PWMCLK", PWM0, 3686400, 0, &pxa25x_device_pwm0.dev),
-	INIT_CKEN("PWMCLK", PWM1, 3686400, 0, &pxa25x_device_pwm1.dev),
-
-	INIT_CKEN("AC97CLK",     AC97,     24576000, 0, NULL),
+static struct clk *pxa25x_hwuart_cken_clk =
+	INIT_CKEN("HWUARTCLK", HWUART, 14745600, 1);
+
+static struct clk_function pxa25x_hwuart_func =
+	CLK_FUNC("HWUARTCLK", "UARTCLK", &clk_dev_ops, &pxa_device_hwuart.dev);
+
+
+static struct clk *pxa25x_clks[] = {
+	INIT_CK("LCDCLK", LCD, &clk_pxa25x_lcd_ops),
+	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("PWM0CLK", PWM0, 3686400, 0),
+	INIT_CKEN("PWM1CLK", PWM1, 3686400, 0),
+
+	INIT_CKEN("AC97CLK",     AC97,     24576000, 0),
 
 	/*
-	INIT_CKEN("I2SCLK",  I2S,  14745600, 0, NULL),
+	INIT_CKEN("I2SCLK",  I2S,  14745600, 0),
 	*/
-	INIT_CKEN("FICPCLK", FICP, 47923000, 0, NULL),
+	INIT_CKEN("FICPCLK", FICP, 47923000, 0),
+};
+
+static struct clk_function pxa25x_clk_funcs[] = {
+	CLK_FUNC("FFUARTCLK", "UARTCLK", &clk_dev_ops, &pxa_device_ffuart.dev),
+	CLK_FUNC("BTUARTCLK", "UARTCLK", &clk_dev_ops, &pxa_device_btuart.dev),
+	CLK_FUNC("STUARTCLK", "UARTCLK", &clk_dev_ops, &pxa_device_stuart.dev),
+	CLK_FUNC("PXAMMCCLK", "MMCCLK", &clk_dev_ops, &pxa_device_mci.dev),
+	CLK_FUNC("SSP_CLK", "SSPCLK", &clk_dev_ops, &pxa25x_device_ssp.dev),
+	CLK_FUNC("NSSPCLK", "SSPCLK", &clk_dev_ops, &pxa25x_device_nssp.dev),
+	CLK_FUNC("ASSPCLK", "SSPCLK", &clk_dev_ops, &pxa25x_device_assp.dev),
+	CLK_FUNC("PWM0CLK", "PWMCLK", &clk_dev_ops, &pxa25x_device_pwm0.dev),
+	CLK_FUNC("PWM1CLK", "PWMCLK", &clk_dev_ops, &pxa25x_device_pwm1.dev),
 };
 
 #ifdef CONFIG_PM
@@ -279,17 +293,22 @@ static struct sys_device pxa25x_sysdev[] = {
 		.cls	= &pxa_gpio_sysclass,
 	},
 };
-
 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 = clk_register(pxa25x_hwuart_cken_clk);
+		if (!ret)
+			ret = clk_alloc_function(
+					pxa25x_hwuart_func.parent,
+					&pxa25x_hwuart_func.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 3291d6f..ecfe887 100644
--- a/arch/arm/mach-pxa/pxa27x.c
+++ b/arch/arm/mach-pxa/pxa27x.c
@@ -130,48 +130,59 @@ 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 const struct clk_ops clk_pxa27x_lcd_ops = {
+	.set_mode	= clk_cken_set_mode,
+	.get_rate	= 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),
+static struct clk *pxa27x_clks[] = {
+	INIT_CK("LCDCLK", LCD,    &clk_pxa27x_lcd_ops),
+	INIT_CK("CAMCLK", CAMERA, &clk_pxa27x_lcd_ops),
 
-	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, &pxa27x_device_keypad.dev),
+	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("PWMCLK", PWM0, 13000000, 0, &pxa27x_device_pwm0.dev),
-	INIT_CKEN("PWMCLK", PWM1, 13000000, 0, &pxa27x_device_pwm1.dev),
+	INIT_CKEN("SSP1CLK", SSP1, 13000000, 0),
+	INIT_CKEN("SSP2CLK", SSP2, 13000000, 0),
+	INIT_CKEN("SSP3CLK", SSP3, 13000000, 0),
+	INIT_CKEN("PWM0CLK", PWM0, 13000000, 0),
+	INIT_CKEN("PWM1CLK", PWM1, 13000000, 0),
 
-	INIT_CKEN("AC97CLK",     AC97,     24576000, 0, NULL),
-	INIT_CKEN("AC97CONFCLK", AC97CONF, 24576000, 0, NULL),
+	INIT_CKEN("AC97CLK",     AC97,     24576000, 0),
+	INIT_CKEN("AC97CONFCLK", AC97CONF, 24576000, 0),
 
 	/*
-	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("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 pxa27x_clk_funcs[] = {
+	CLK_FUNC("FFUARTCLK", "UARTCLK", &clk_dev_ops, &pxa_device_ffuart.dev),
+	CLK_FUNC("BTUARTCLK", "UARTCLK", &clk_dev_ops, &pxa_device_btuart.dev),
+	CLK_FUNC("STUARTCLK", "UARTCLK", &clk_dev_ops, &pxa_device_stuart.dev),
+	CLK_FUNC("PXAMMCCLK", "MMCCLK", &clk_dev_ops, &pxa_device_mci.dev),
+	CLK_FUNC("SSP1CLK", "SSPCLK", &clk_dev_ops, &pxa27x_device_ssp1.dev),
+	CLK_FUNC("SSP2CLK", "SSPCLK", &clk_dev_ops, &pxa27x_device_ssp2.dev),
+	CLK_FUNC("SSP3CLK", "SSPCLK", &clk_dev_ops, &pxa27x_device_ssp3.dev),
+	CLK_FUNC("PWM0CLK", "PWMCLK", &clk_dev_ops, &pxa27x_device_pwm0.dev),
+	CLK_FUNC("PWM1CLK", "PWMCLK", &clk_dev_ops, &pxa27x_device_pwm1.dev),
+};
+
 #ifdef CONFIG_PM
 
 #define SAVE(x)		sleep_save[SLEEP_SAVE_##x] = x
@@ -380,7 +391,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 7c061af..cef6d80 100644
--- a/arch/arm/mach-pxa/pxa3xx.c
+++ b/arch/arm/mach-pxa/pxa3xx.c
@@ -20,6 +20,7 @@
 #include <linux/platform_device.h>
 #include <linux/irq.h>
 #include <linux/io.h>
+#include <linux/delay.h>
 #include <linux/sysdev.h>
 
 #include <asm/hardware.h>
@@ -144,107 +145,127 @@ 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_set_mode(struct clk *clk, bool enable)
 {
-	unsigned long mask = 1ul << (clk->cken & 0x1f);
-
-	if (clk->cken < 32)
-		CKENA |= mask;
+	struct clk_cken *cclk = container_of(clk, struct clk_cken, clk);
+	int cken = cclk->cken;
+	unsigned long mask = 1ul << (cken & 0x1f);
+
+	if (cken < 32)
+		if (enable)
+			CKENA |= mask;
+		else
+			CKENA &= ~mask;
 	else
-		CKENB |= mask;
-}
+		if (enable)
+			CKENB |= mask;
+		else
+			CKENB &= ~mask;
 
-static void clk_pxa3xx_cken_disable(struct clk *clk)
-{
-	unsigned long mask = 1ul << (clk->cken & 0x1f);
-
-	if (clk->cken < 32)
-		CKENA &= ~mask;
-	else
-		CKENB &= ~mask;
+	return 0;
 }
 
-static const struct clkops clk_pxa3xx_cken_ops = {
-	.enable		= clk_pxa3xx_cken_enable,
-	.disable	= clk_pxa3xx_cken_disable,
+static const struct clk_ops clk_pxa3xx_cken_ops = {
+	.set_mode	= clk_pxa3xx_cken_set_mode,
+	.get_rate	= clk_cken_get_rate,
 };
 
-static const struct clkops clk_pxa3xx_hsio_ops = {
-	.enable		= clk_pxa3xx_cken_enable,
-	.disable	= clk_pxa3xx_cken_disable,
-	.getrate	= clk_pxa3xx_hsio_getrate,
+static const struct clk_ops clk_pxa3xx_hsio_ops = {
+	.set_mode	= clk_pxa3xx_cken_set_mode,
+	.get_rate	= clk_pxa3xx_hsio_getrate,
 };
 
-static const struct clkops clk_pxa3xx_ac97_ops = {
-	.enable		= clk_pxa3xx_cken_enable,
-	.disable	= clk_pxa3xx_cken_disable,
-	.getrate	= clk_pxa3xx_ac97_getrate,
+static const struct clk_ops clk_pxa3xx_ac97_ops = {
+	.set_mode	= clk_pxa3xx_cken_set_mode,
+	.get_rate	= clk_pxa3xx_ac97_getrate,
 };
 
-static void clk_pout_enable(struct clk *clk)
+static int clk_pout_set_mode(struct clk *clk, bool enable)
 {
-	OSCC |= OSCC_PEN;
-}
+	struct clk_cken *cclk = container_of(clk, struct clk_cken, clk);
+	if (enable) {
+		OSCC |= OSCC_PEN;
+		if (cclk->delay)
+			udelay(cclk->delay);
+	} else
+		OSCC &= ~OSCC_PEN;
 
-static void clk_pout_disable(struct clk *clk)
-{
-	OSCC &= ~OSCC_PEN;
+	return 0;
 }
 
-static const struct clkops clk_pout_ops = {
-	.enable		= clk_pout_enable,
-	.disable	= clk_pout_disable,
+static const struct clk_ops clk_pout_ops = {
+	.set_mode	= clk_pout_set_mode,
+	.get_rate	= clk_cken_get_rate,
 };
 
-#define PXA3xx_CKEN(_name, _cken, _rate, _delay, _dev)	\
-	{						\
-		.name	= _name,			\
-		.dev	= _dev,				\
-		.ops	= &clk_pxa3xx_cken_ops,		\
+#define PXA3xx_CKEN(_name, _cken, _rate, _delay)	\
+	&(struct clk_cken) {				\
+		.clk	= {				\
+			.name	= _name,		\
+			.ops	= &clk_pxa3xx_cken_ops,	\
+		},					\
 		.rate	= _rate,			\
 		.cken	= CKEN_##_cken,			\
 		.delay	= _delay,			\
-	}
-
-#define PXA3xx_CK(_name, _cken, _ops, _dev)		\
-	{						\
-		.name	= _name,			\
-		.dev	= _dev,				\
-		.ops	= _ops,				\
-		.cken	= CKEN_##_cken,			\
-	}
-
-static struct clk pxa3xx_clks[] = {
-	{
-		.name           = "CLK_POUT",
-		.ops            = &clk_pout_ops,
+	}.clk
+
+#define PXA3xx_CK(_name, _cken, _ops)			\
+	&(struct clk_cken) {				\
+		.cken = CKEN_##_cken,			\
+		.clk = {				\
+			.name	= _name,		\
+			.ops	= _ops,			\
+		},					\
+	}.clk
+
+static struct clk *pxa3xx_clks[] = {
+	&(&(struct clk_cken) {
+		.clk	= {
+			.name	= "CLK_POUT",
+			.ops	= &clk_pout_ops,
+		},
 		.rate           = 13000000,
-		.delay          = 70,
-	},
+		.delay		= 70,
+	})->clk,
+
+	PXA3xx_CK("LCDCLK",  LCD,    &clk_pxa3xx_hsio_ops),
+	PXA3xx_CK("CAMCLK",  CAMERA, &clk_pxa3xx_hsio_ops),
+	PXA3xx_CK("AC97CLK", AC97,   &clk_pxa3xx_ac97_ops),
+
+	PXA3xx_CKEN("FFUARTCLK", FFUART, 14857000, 1),
+	PXA3xx_CKEN("BTUARTCLK", BTUART, 14857000, 1),
+	PXA3xx_CKEN("STUARTCLK", STUART, 14857000, 1),
+
+	PXA3xx_CKEN("I2CCLK", I2C,  32842000, 0),
+	PXA3xx_CKEN("UDCCLK", UDC,  48000000, 5),
+	PXA3xx_CKEN("USBCLK", USBH, 48000000, 0),
+	PXA3xx_CKEN("KBDCLK", KEYPAD,  32768, 0),
+
+	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("PWM0CLK", PWM0, 13000000, 0),
+	PXA3xx_CKEN("PWM1CLK", PWM1, 13000000, 0),
+
+	PXA3xx_CKEN("MMC1CLK", MMC1, 19500000, 0),
+	PXA3xx_CKEN("MMC2CLK", MMC2, 19500000, 0),
+	PXA3xx_CKEN("MMC3CLK", MMC3, 19500000, 0),
+};
 
-	PXA3xx_CK("LCDCLK",  LCD,    &clk_pxa3xx_hsio_ops, &pxa_device_fb.dev),
-	PXA3xx_CK("CAMCLK",  CAMERA, &clk_pxa3xx_hsio_ops, NULL),
-	PXA3xx_CK("AC97CLK", AC97,   &clk_pxa3xx_ac97_ops, NULL),
-
-	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("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("KBDCLK", KEYPAD,  32768, 0, &pxa27x_device_keypad.dev),
-
-	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("PWMCLK", PWM0, 13000000, 0, &pxa27x_device_pwm0.dev),
-	PXA3xx_CKEN("PWMCLK", PWM1, 13000000, 0, &pxa27x_device_pwm1.dev),
-
-	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 pxa3xx_clk_funcs[] = {
+	CLK_FUNC("FFUARTCLK", "UARTCLK", &clk_dev_ops, &pxa_device_ffuart.dev),
+	CLK_FUNC("BTUARTCLK", "UARTCLK", &clk_dev_ops, &pxa_device_btuart.dev),
+	CLK_FUNC("STUARTCLK", "UARTCLK", &clk_dev_ops, &pxa_device_stuart.dev),
+	CLK_FUNC("SSP1CLK", "SSPCLK", &clk_dev_ops, &pxa27x_device_ssp1.dev),
+	CLK_FUNC("SSP2CLK", "SSPCLK", &clk_dev_ops, &pxa27x_device_ssp2.dev),
+	CLK_FUNC("SSP3CLK", "SSPCLK", &clk_dev_ops, &pxa27x_device_ssp3.dev),
+	CLK_FUNC("SSP4CLK", "SSPCLK", &clk_dev_ops, &pxa3xx_device_ssp4.dev),
+	CLK_FUNC("MMC1CLK", "MMCCLK", &clk_dev_ops, &pxa_device_mci.dev),
+	CLK_FUNC("MMC2CLK", "MMCCLK", &clk_dev_ops, &pxa3xx_device_mci2.dev),
+	CLK_FUNC("MMC3CLK", "MMCCLK", &clk_dev_ops, &pxa3xx_device_mci3.dev),
+	CLK_FUNC("PWM0CLK", "PWMCLK", &clk_dev_ops, &pxa27x_device_pwm0.dev),
+	CLK_FUNC("PWM1CLK", "PWMCLK", &clk_dev_ops, &pxa27x_device_pwm1.dev),
 };
 
 #ifdef CONFIG_PM
@@ -556,7 +577,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

[PATCH 5/5] Clocklib: Use correct clock for IrDA on pxa

[Dmitry Baryshkov]

Signed-off-by: Dmitry Baryshkov <dbaryshkov@gmail.com>
---
 drivers/net/irda/pxaficp_ir.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/irda/pxaficp_ir.c b/drivers/net/irda/pxaficp_ir.c
index 8db71ab..1aae2d6 100644
--- a/drivers/net/irda/pxaficp_ir.c
+++ b/drivers/net/irda/pxaficp_ir.c
@@ -815,7 +815,7 @@ static int pxa_irda_probe(struct platform_device *pdev)
 	si->dev = &pdev->dev;
 	si->pdata = pdev->dev.platform_data;
 
-	si->sir_clk = clk_get(&pdev->dev, "UARTCLK");
+	si->sir_clk = clk_get(&pdev->dev, "STUARTCLK");
 	si->fir_clk = clk_get(&pdev->dev, "FICPCLK");
 	if (IS_ERR(si->sir_clk) || IS_ERR(si->fir_clk)) {
 		err = PTR_ERR(IS_ERR(si->sir_clk) ? si->sir_clk : si->fir_clk);
-- 
1.5.4.4


-- 
With best wishes
Dmitry

Re: [PATCH 0/5] Clocklib: generic clocks framework

[Paul Walmsley]

Hello Dmitry,

By way of introduction, I've been working on the Linux-OMAP clock tree 
over the past several months.  I recently had the opportunity to take a 
brief look at these clocklib patches that you're posting, and had a few 
thoughts:

- I understand from the discussions in this thread that the usage of your 
  clocklib code will be optional. However, the way you implement various 
  parts of the clock interface may effectively become mandatory, and 
  clocklib may not be able to handle many of the platform-specific clock 
  details that are necessary with more complex clock layouts like OMAP.  
  Would you consider the main goal of your clocklib code to be simply the 
  removal of several of the simpler ARM clock tree implementations?  Or is 
  your intention for it to ultimately replace all of the current Linux 
  clock implementations?  If the latter, your patchset will presumably 
  need a higher standard of review, since once it is integrated, any 
  changes will affect several architectures, rather than simply one.

- As others have mentioned earlier on this thread, it seems difficult to 
  construct a good "one-size-fits-all" struct clk.  At the very least, 
  I would also suggest adding a 'void *' to allow storage of clock-specific 
  data.

- Hiroshi DOYU has proposed an alternate debugfs implementation for the
  Linux-OMAP clock tree.  I prefer it to yours, as it implements each 
  clock as a separate dentry, which makes it easy to implement additional 
  debugging functions, such as set_rate/set_parent/round_rate debugging.  
  Perhaps you'd consider it, or something similar to it, instead?  It is 
  proposed here:
     http://www.mail-archive.com/linux-omap@vger.kernel.org/msg00751.html

- I don't think that I understand the clk_functions part of your code.  
  Is this a shorthand to construct aliases to other struct clks?


regards,

- Paul

Re: [PATCH 0/5] Clocklib: generic clocks framework

[Dmitry]

Hi, Paul,

2008/4/21, Paul Walmsley <paul@pwsan.com>:
> Hello Dmitry,
>
>  By way of introduction, I've been working on the Linux-OMAP clock tree
>  over the past several months.  I recently had the opportunity to take a
>  brief look at these clocklib patches that you're posting, and had a few
>  thoughts:
>
>  - I understand from the discussions in this thread that the usage of your
>   clocklib code will be optional. However, the way you implement various
>   parts of the clock interface may effectively become mandatory, and
>   clocklib may not be able to handle many of the platform-specific clock
>   details that are necessary with more complex clock layouts like OMAP.
>   Would you consider the main goal of your clocklib code to be simply the
>   removal of several of the simpler ARM clock tree implementations?  Or is
>   your intention for it to ultimately replace all of the current Linux
>   clock implementations?  If the latter, your patchset will presumably
>   need a higher standard of review, since once it is integrated, any
>   changes will affect several architectures, rather than simply one.

Yes, I pretty much understand that. That's why I've posted the
patchserie to the LKML and added such long cc list full of arch.
maintainers. I hope it will be usefull not only to few arm arches, but
to a wide list of linux-supported systems.

I've reviewed the code for most linux/clk.h implementations in the
kernel. The OMAP code was... a bit scary for me. I don't have any deep
knowledge of this platform, and there were lots of structures, lots of
structs embedding struct clk, etc.
Tell me please, what do you need, that can't be done with this framework?


>  - As others have mentioned earlier on this thread, it seems difficult to
>   construct a good "one-size-fits-all" struct clk.  At the very least,
>   I would also suggest adding a 'void *' to allow storage of clock-specific
>   data.

This was already discussed. It was suggested to use struct embedding
and container_of,
instead of pointers. If you do really need a pointer, you can writes
struct my_clk {
      void *data;
      struct clk clk;
};

>  - Hiroshi DOYU has proposed an alternate debugfs implementation for the
>   Linux-OMAP clock tree.  I prefer it to yours, as it implements each
>   clock as a separate dentry, which makes it easy to implement additional
>   debugging functions, such as set_rate/set_parent/round_rate debugging.
>   Perhaps you'd consider it, or something similar to it, instead?  It is
>   proposed here:
>      http://www.mail-archive.com/linux-omap@vger.kernel.org/msg00751.html

I'll look at this. However, if we are going to use such
implementation, may we should
move instead to sysfs?

>  - I don't think that I understand the clk_functions part of your code.
>   Is this a shorthand to construct aliases to other struct clks?

Yes, that's one of usages for it. E.g. current AT91 code has same
functionality named
at91_clock_associate.  Also onece we get to multiple chips
providers/users, we'll see,
that the clock simply can't have just one record in the clocks tree.
It's provided by some
pin (provider_name) and then consumed by several devices (several
consumer_name + consumer_device pairs). That is it.

-- 
With best wishes
Dmitry

Re: [PATCH 0/5] Clocklib: generic clocks framework

[Hiroshi DOYU]

Hi Dmitry,

From: "ext Dmitry" <dbaryshkov@gmail.com>
Subject: Re: [PATCH 0/5] Clocklib: generic clocks framework
Date: Mon, 21 Apr 2008 12:48:24 +0400

> >  - Hiroshi DOYU has proposed an alternate debugfs implementation for the
> >   Linux-OMAP clock tree.  I prefer it to yours, as it implements each
> >   clock as a separate dentry, which makes it easy to implement additional
> >   debugging functions, such as set_rate/set_parent/round_rate debugging.
> >   Perhaps you'd consider it, or something similar to it, instead?  It is
> >   proposed here:
> >      http://www.mail-archive.com/linux-omap@vger.kernel.org/msg00751.html
> 
> I'll look at this. However, if we are going to use such
> implementation, may we should
> move instead to sysfs?

This has been discussed within the above thread in omap and you can
find the sysfs implementation sysfs at the bottom of the following:

http://www.mail-archive.com/linux-omap@vger.kernel.org/msg00696.html

    Hiroshi DOYU

Re: [PATCH 0/5] Clocklib: generic clocks framework

[Paul Walmsley]

Hello Dmitry, 

On Mon, 21 Apr 2008, Dmitry wrote:

> 2008/4/21, Paul Walmsley <paul@pwsan.com>:
> >
> >   If the latter, your patchset will presumably
> >   need a higher standard of review, since once it is integrated, any
> >   changes will affect several architectures, rather than simply one.
> [...]
> I've reviewed the code for most linux/clk.h implementations in the
> kernel. The OMAP code was... a bit scary for me. I don't have any deep
> knowledge of this platform, and there were lots of structures, lots of
> structs embedding struct clk, etc.
> Tell me please, what do you need, that can't be done with this framework?

I wouldn't pretend to have a comprehensive list at this point.  But from a 
brief look, your clk_round_rate() and clk_set_rate() implementations will 
not work for the present OMAP clock tree.  In OMAP, many parent clocks do 
not have the same rate as their children.

But that is not really the main issue.  Ultimately, as long as your 
implementation remains completely optional, and any public interfaces 
(like debugfs/sysfs interfaces) are coordinated with other clock interface 
implementors, I personally have no issues with your code going into the 
tree somewhere.  With the possible exception of the clk_functions code, 
clocklib looks pretty clean to me, and a good exemplar of a clock 
interface implementation.

However: if the ultimate goal is to make your implementation normative, 
then I concur with Russell, and I would recommend against merging.  
Assumptions that you make in clocklib may not work well for future chips.  
Changing clocklib will affect many architectures.  For example, perhaps 
someone may wish to implement clocks as an actual in-memory tree rather 
than a list.  Or perhaps someone may need to handle clock usecounting 
differently, for situations when multiple clocks might share the same 
enable/disable code, but with different parents.
 
I am concerned that having any implementation as an implicit standard in 
the tree would increase the risk that, over time, internal implementation 
assumptions will be considered normative.  This could easily cause more 
pain in the future for maintainers than it would be worth. 

> This was already discussed. It was suggested to use struct embedding and 
> container_of, instead of pointers. If you do really need a pointer, you 
> can writes 
> struct my_clk {
>       void *data;
>       struct clk clk;
> };

OK.

> >  - I don't think that I understand the clk_functions part of your code.
> >   Is this a shorthand to construct aliases to other struct clks?
> 
> Yes, that's one of usages for it. E.g. current AT91 code has same 
> functionality named at91_clock_associate.  Also onece we get to multiple 
> chips providers/users, we'll see, that the clock simply can't have just 
> one record in the clocks tree. It's provided by some pin (provider_name) 
> and then consumed by several devices (several consumer_name + 
> consumer_device pairs). That is it.

Perhaps you might consider renaming these functions, perhaps "dynamic" 
clocks or "alias" clocks or something similar?  The word "function" has 
some other strong associations which I found confusing when I read the 
code.


regards,

- Paul

Re: [PATCH 0/5] Clocklib: generic clocks framework

[Pavel Machek]

Hi!

> > >   If the latter, your patchset will presumably
> > >   need a higher standard of review, since once it is integrated, any
> > >   changes will affect several architectures, rather than simply one.
> > [...]
> > I've reviewed the code for most linux/clk.h implementations in the
> > kernel. The OMAP code was... a bit scary for me. I don't have any deep
> > knowledge of this platform, and there were lots of structures, lots of
> > structs embedding struct clk, etc.
> > Tell me please, what do you need, that can't be done with this framework?
> 
> I wouldn't pretend to have a comprehensive list at this point.  But from a 
> brief look, your clk_round_rate() and clk_set_rate() implementations will 
> not work for the present OMAP clock tree.  In OMAP, many parent clocks do 
> not have the same rate as their children.
> 
> But that is not really the main issue.  Ultimately, as long as your 
> implementation remains completely optional, and any public interfaces 
> (like debugfs/sysfs interfaces) are coordinated with other clock interface 
> implementors, I personally have no issues with your code going into the 
> tree somewhere.  With the possible exception of the clk_functions code, 
> clocklib looks pretty clean to me, and a good exemplar of a clock 
> interface implementation.
> 
> However: if the ultimate goal is to make your implementation normative, 
> then I concur with Russell, and I would recommend against merging.  
> Assumptions that you make in clocklib may not work well for future chips.  
> Changing clocklib will affect many architectures.  For example, perhaps 
> someone may wish to implement clocks as an actual in-memory tree rather 
> than a list.  Or perhaps someone may need to handle clock usecounting 
> differently, for situations when multiple clocks might share the same 
> enable/disable code, but with different parents.

WTF? There are currently around 10 copies of clock code in the tree,
every one slightly different. If this can help us get rid of all that
crap, that's a GOOD THING, normative or not.

> I am concerned that having any implementation as an implicit standard in 
> the tree would increase the risk that, over time, internal implementation 
> assumptions will be considered normative.  This could easily cause more 
> pain in the future for maintainers than it would be worth. 

_Of course_  new implementations should try to use existing
framework. And _of course_, if you have some special requirements, you
are still free to create your own version...
									Pavel

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

Re: [PATCH 0/5] Clocklib: generic clocks framework

[Russell King]

On Fri, Apr 25, 2008 at 12:39:42PM +0200, Pavel Machek wrote:
> WTF? There are currently around 10 copies of clock code in the tree,
> every one slightly different. If this can help us get rid of all that
> crap, that's a GOOD THING, normative or not.

At the expense of people going off and inventing their own APIs because
they find that the "normatived" clock API doesn't do what they need to?

That's what will happen if you try to force a framework on folk which
they don't agree with.

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

Re: [PATCH 0/5] Clocklib: generic clocks framework

[Dmitry]

Hi,

2008/4/26, Russell King <rmk+lkml@arm.linux.org.uk>:
> On Fri, Apr 25, 2008 at 12:39:42PM +0200, Pavel Machek wrote:
>  > WTF? There are currently around 10 copies of clock code in the tree,
>  > every one slightly different. If this can help us get rid of all that
>  > crap, that's a GOOD THING, normative or not.
>
>
> At the expense of people going off and inventing their own APIs because
>  they find that the "normatived" clock API doesn't do what they need to?

Why? We do already have the API. And it's pretty normative. And the
goal of my framework is to allow me and few other people not to
reinvent the API for non-platform clocks.

>  That's what will happen if you try to force a framework on folk which
>  they don't agree with.

If you don't want to use it, you are free to do so. E.g. you can use
your own set of functions to implement GPIO api.

-- 
With best wishes
Dmitry

Re: [PATCH 0/5] Clocklib: generic clocks framework

[Russell King]

On Sat, Apr 26, 2008 at 12:34:55AM +0400, Dmitry wrote:
> Hi,
> 
> 2008/4/26, Russell King <rmk+lkml@arm.linux.org.uk>:
> > On Fri, Apr 25, 2008 at 12:39:42PM +0200, Pavel Machek wrote:
> >  > WTF? There are currently around 10 copies of clock code in the tree,
> >  > every one slightly different. If this can help us get rid of all that
> >  > crap, that's a GOOD THING, normative or not.
> >
> >
> > At the expense of people going off and inventing their own APIs because
> >  they find that the "normatived" clock API doesn't do what they need to?
> 
> Why? We do already have the API. And it's pretty normative. And the
> goal of my framework is to allow me and few other people not to
> reinvent the API for non-platform clocks.
> 
> >  That's what will happen if you try to force a framework on folk which
> >  they don't agree with.
> 
> If you don't want to use it, you are free to do so. E.g. you can use
> your own set of functions to implement GPIO api.

Now go back and read what Pavel wrote (which I responsed to - the
implication that your clock API _will_ _be_ forced upon _everyone_) and
you'll see that he has a completely different perspective to what you've
just said.  So rather than replying to my response, why not respond to
Pavel with your points you've made above?

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

Re: [PATCH 0/5] Clocklib: generic clocks framework

[Pavel Machek]

Hi!

> > WTF? There are currently around 10 copies of clock code in the tree,
> > every one slightly different. If this can help us get rid of all that
> > crap, that's a GOOD THING, normative or not.
> 
> At the expense of people going off and inventing their own APIs because
> they find that the "normatived" clock API doesn't do what they need to?

Just now, everyone just cuts&copies clock.c. I do not think "new"
situation can worse than that.
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 0/5] Clocklib: generic clocks framework

[Russell King]

On Fri, Apr 25, 2008 at 10:51:51PM +0200, Pavel Machek wrote:
> Hi!
> 
> > > WTF? There are currently around 10 copies of clock code in the tree,
> > > every one slightly different. If this can help us get rid of all that
> > > crap, that's a GOOD THING, normative or not.
> > 
> > At the expense of people going off and inventing their own APIs because
> > they find that the "normatived" clock API doesn't do what they need to?
> 
> Just now, everyone just cuts&copies clock.c. I do not think "new"
> situation can worse than that.

That's certainly not what I've seen going on.  Each implementation is
customised to the needs of the SoC it's running on - OMAP has a complex
implementation, whereas simpler SoCs have a more simple implementation.

That's an entirely reasonable state of affairs - those who need complexity
are able to have it, whereas those who don't need complexity don't have
to be lumbered with it.

It's a long way from a "cut and copy" situation you're trying to suggest
it is.  Certainly on ARM, your viewpoint does not hold.

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

Re: [PATCH 0/5] Clocklib: generic clocks framework

[Dmitry]

Hi,

2008/4/26 Russell King <rmk+lkml@arm.linux.org.uk>:
> On Fri, Apr 25, 2008 at 10:51:51PM +0200, Pavel Machek wrote:
>  > Hi!
>  >
>  > > > WTF? There are currently around 10 copies of clock code in the tree,
>  > > > every one slightly different. If this can help us get rid of all that
>  > > > crap, that's a GOOD THING, normative or not.
>  > >
>  > > At the expense of people going off and inventing their own APIs because
>  > > they find that the "normatived" clock API doesn't do what they need to?
>  >
>  > Just now, everyone just cuts&copies clock.c. I do not think "new"
>  > situation can worse than that.
>
>  That's certainly not what I've seen going on.  Each implementation is
>  customised to the needs of the SoC it's running on - OMAP has a complex
>  implementation, whereas simpler SoCs have a more simple implementation.
>
>  That's an entirely reasonable state of affairs - those who need complexity
>  are able to have it, whereas those who don't need complexity don't have
>  to be lumbered with it.
>
>  It's a long way from a "cut and copy" situation you're trying to suggest
>  it is.  Certainly on ARM, your viewpoint does not hold.

Actually it is "cut and copy". I once have examined all arm clock subsystems
and converted most of them (excluding OMAP, at91, maybe some others)
to the clocklib. There is more code duplication
that one would think. E.g. DaVinci clock.h contains a few flags
definitions, that
are totally unused by the code (most probably direct c&p from omap code).

I don't understand why do you give such strong oppression to these patches.
Simple systems (like sa1100) will be reduced just to few lines of code.
Mediocre (like pxa) will be again highly reduced in terms of size,
maintainability, etc.
And highly comliex (like omap) do already provide some type of framework
like clocklib.

-- 
With best wishes
Dmitry

Re: [PATCH 0/5] Clocklib: generic clocks framework

[David Brownell]

On Monday 21 April 2008, Dmitry wrote:
> >  - I don't think that I understand the clk_functions part of your code.
> >   Is this a shorthand to construct aliases to other struct clks?
>
> Yes, that's one of usages for it. E.g. current AT91 code has same
> functionality named at91_clock_associate.

As in:

	at91_clock_associate("usart0_clk", &pdev0->dev, "usart");
	at91_clock_associate("usart1_clk", &pdev1->dev, "usart");
	at91_clock_associate("usart2_clk", &pdev2->dev, "usart");

That essentially maps a device and logical clock name to a specific
clock (those "usartX_clk" identifiers are global), so that drivers
can clk_get(dev, "usart") and get the right clock.  It decouples
clock and device declarations, which should help when integrating
external clocks too.

The assumption that clk_get(NULL, "usartX_clk") works is mostly
just a convenience, although it's probably a fair assumption for
many other SOC platforms.  Conceptually that first parameter is
a clock, not a name.


> > +struct clk_function {
> > +       const char *parent;
> > +       void *priv;
> > +       struct clk clk;
> > +};

That doesn't really seem attuned to the task of associating
logical clock names to devices.  Wouldn't it be better to
have something directly addressing that core mechanism?


> Also once we get to multiple chips providers/users, we'll see,
> that the clock simply can't have just one record in the clocks tree.

I don't follow.  Why not?  If a clock has multiple records, I'd
expect its refcounts would easily get borked.  I think I don't
like your notion of a "wrapper clock".


> It's provided by some
> pin (provider_name) and then consumed by several devices (several
> consumer_name + consumer_device pairs). That is it.

There would still be just one clock though.  It shouldn't matter
how clk_get(this_dev, "c1") and clk_get(that_dev, "c2") find it.
Hashtable, linked list, divination, ... so long as it works.

Those other records should just hold {dev, name} ==> clock mappings;
they wouldn't be records for the clock itself.  But those "functions"
do not seem to record just mappings.

- Dave

Re: [PATCH 0/5] Clocklib: generic clocks framework

[Dmitry]

Hi,

2008/4/26 David Brownell <david-b@pacbell.net>:
> On Monday 21 April 2008, Dmitry wrote:
>  > >  - I don't think that I understand the clk_functions part of your code.
>  > >   Is this a shorthand to construct aliases to other struct clks?
>  >
>  > Yes, that's one of usages for it. E.g. current AT91 code has same
>  > functionality named at91_clock_associate.
>
>  As in:
>
>         at91_clock_associate("usart0_clk", &pdev0->dev, "usart");
>         at91_clock_associate("usart1_clk", &pdev1->dev, "usart");
>         at91_clock_associate("usart2_clk", &pdev2->dev, "usart");
>
>  That essentially maps a device and logical clock name to a specific
>  clock (those "usartX_clk" identifiers are global), so that drivers
>  can clk_get(dev, "usart") and get the right clock.  It decouples
>  clock and device declarations, which should help when integrating
>  external clocks too.
>
>  The assumption that clk_get(NULL, "usartX_clk") works is mostly
>  just a convenience, although it's probably a fair assumption for
>  many other SOC platforms.  Conceptually that first parameter is
>  a clock, not a name.

Yes. It's just a convenient form to specify the parent (holding) clock
(usartX_clk).
Usually you register all such clocks via an array of various clocks,
so it's just
easier to specify the name, than the struct clk pointer. Hope this makes it more
clear.

>
>
>  > > +struct clk_function {
>  > > +       const char *parent;
>  > > +       void *priv;
>  > > +       struct clk clk;
>  > > +};
>
>  That doesn't really seem attuned to the task of associating
>  logical clock names to devices.  Wouldn't it be better to
>  have something directly addressing that core mechanism?

In reality it does. Some platforms prefer to specify name+device,
some prefer name + id of the device, etc. Thus this definition.
For example of dev+name association, you can check the PXA
patch (e.g. for {FF,BT,ST,HW}UARTCLK -> UARTCLK definitions).

The association is done with the help of the clk_dev_ops operations.
Maybe them should be moved to the generic library.

>  > Also once we get to multiple chips providers/users, we'll see,
>
> > that the clock simply can't have just one record in the clocks tree.
>
>  I don't follow.  Why not?  If a clock has multiple records, I'd
>  expect its refcounts would easily get borked.  I think I don't
>  like your notion of a "wrapper clock".

It seems, I failed to describe it correctly.
Let's suppose this situation:
We have CLK, that is connected to the pin CK1 of the device dev_A and to the pin
CK36M of the device dev_B.

The we'll have these clocks:
.CLK count=0
\- CK1 count=0 for dev_A
\- CK36M count=0 for dev_B

When device dev_A enables it's clock,
we'll have this:
.CLK count=1
\- CK1 count=1 for dev_A
\- CK36M count=0 for dev_B

After that dev_B enables it's clock:
.CLK count=2
\- CK1 count=1 for dev_A
\- CK36M count=1 for dev_B

So refcounting is correct.

>  > It's provided by some
>  > pin (provider_name) and then consumed by several devices (several
>  > consumer_name + consumer_device pairs). That is it.
>
>  There would still be just one clock though.  It shouldn't matter
>  how clk_get(this_dev, "c1") and clk_get(that_dev, "c2") find it.
>  Hashtable, linked list, divination, ... so long as it works.
>
>  Those other records should just hold {dev, name} ==> clock mappings;
>  they wouldn't be records for the clock itself.  But those "functions"
>  do not seem to record just mappings.

It's easier from the abstraction POV to make that "records" to be real clocks.
Why introduce another object, if we can unify access to both types?

-- 
With best wishes
Dmitry

Re: [PATCH 0/5] Clocklib: generic clocks framework

[Dmitry]

Hi,

2008/4/25, Paul Walmsley <paul@pwsan.com>:
> Hello Dmitry,
>
>
>  On Mon, 21 Apr 2008, Dmitry wrote:
>
>  > 2008/4/21, Paul Walmsley <paul@pwsan.com>:
>  > >
>
> > >   If the latter, your patchset will presumably
>  > >   need a higher standard of review, since once it is integrated, any
>  > >   changes will affect several architectures, rather than simply one.
>
> > [...]
>
> > I've reviewed the code for most linux/clk.h implementations in the
>  > kernel. The OMAP code was... a bit scary for me. I don't have any deep
>  > knowledge of this platform, and there were lots of structures, lots of
>  > structs embedding struct clk, etc.
>  > Tell me please, what do you need, that can't be done with this framework?
>
>
> I wouldn't pretend to have a comprehensive list at this point.  But from a
>  brief look, your clk_round_rate() and clk_set_rate() implementations will
>  not work for the present OMAP clock tree.  In OMAP, many parent clocks do
>  not have the same rate as their children.

You can easily override any calculations in your clk->set_rate/clk->round_rate/
clk->get_rate functions.

>
>  But that is not really the main issue.  Ultimately, as long as your
>  implementation remains completely optional, and any public interfaces
>  (like debugfs/sysfs interfaces) are coordinated with other clock interface
>  implementors, I personally have no issues with your code going into the
>  tree somewhere.  With the possible exception of the clk_functions code,
>  clocklib looks pretty clean to me, and a good exemplar of a clock
>  interface implementation.

My first goals are:
1) to have an infrastructure to plug in my clocks in a platform-independant way
2) to drop lots of copies of nearly the same code.

I certainly do not plan to force all platforms to use this framework. However,
I think that would be good if most of them can be converted to clklib.

>  However: if the ultimate goal is to make your implementation normative,
>  then I concur with Russell, and I would recommend against merging.
>  Assumptions that you make in clocklib may not work well for future chips.
>  Changing clocklib will affect many architectures.  For example, perhaps
>  someone may wish to implement clocks as an actual in-memory tree rather
>  than a list.  Or perhaps someone may need to handle clock usecounting
>  differently, for situations when multiple clocks might share the same
>  enable/disable code, but with different parents.

Sorry, but WTF? Do you prefer to keep a lot of code and disallow
merging a generification just because of some-that-may-even-not-exist
cases? That sounds
pretty... strange.
And your examples are really strange.

>
>  I am concerned that having any implementation as an implicit standard in
>  the tree would increase the risk that, over time, internal implementation
>  assumptions will be considered normative.  This could easily cause more
>  pain in the future for maintainers than it would be worth.
>
>
>  > This was already discussed. It was suggested to use struct embedding and
>  > container_of, instead of pointers. If you do really need a pointer, you
>  > can writes
>  > struct my_clk {
>  >       void *data;
>  >       struct clk clk;
>  > };
>
>
> OK.
>
>
>  > >  - I don't think that I understand the clk_functions part of your code.
>  > >   Is this a shorthand to construct aliases to other struct clks?
>  >
>  > Yes, that's one of usages for it. E.g. current AT91 code has same
>  > functionality named at91_clock_associate.  Also onece we get to multiple
>  > chips providers/users, we'll see, that the clock simply can't have just
>  > one record in the clocks tree. It's provided by some pin (provider_name)
>  > and then consumed by several devices (several consumer_name +
>  > consumer_device pairs). That is it.
>
>
> Perhaps you might consider renaming these functions, perhaps "dynamic"
>  clocks or "alias" clocks or something similar?  The word "function" has
>  some other strong associations which I found confusing when I read the
>  code.

I'll think about this.

-- 
With best wishes
Dmitry

Re: [PATCH 0/5] Clocklib: generic clocks framework

[David Brownell]

On Saturday 26 April 2008, Dmitry wrote:
> > > Also once we get to multiple chips providers/users, we'll see,
> > > that the clock simply can't have just one record in the clocks tree.
> >
> >  I don't follow.  Why not?  If a clock has multiple records, I'd
> >  expect its refcounts would easily get borked.  I think I don't
> >  like your notion of a "wrapper clock".
> 
> It seems, I failed to describe it correctly.
> Let's suppose this situation:
> We have CLK, that is connected to the pin CK1 of the device dev_A and to the pin
> CK36M of the device dev_B.
> 
> The we'll have these clocks:
> .CLK count=0
> \- CK1 count=0 for dev_A
> \- CK36M count=0 for dev_B

But that's incorrect.  What we have is

  CLK users=0
    aliased as CK1 for dev_A
    aliased as CK36M for dev_b

The difference being that you are showing that CK1 and CK36M have
independent clock gates ... which are not actually present.

The difference between this and the at91_clk_associate() example
is that with clk_associate(), there is only one alias per clock.
There are cases where multiple aliases would be better ... TCB
modules, the "system" clock, and so on.


> When device dev_A enables it's clock,
> we'll have this:
> .CLK count=1
> \- CK1 count=1 for dev_A
> \- CK36M count=0 for dev_B

No, we have

  CLK users=1
    ... same aliases


> After that dev_B enables it's clock:
> .CLK count=2
> \- CK1 count=1 for dev_A
> \- CK36M count=1 for dev_B

Should be

  CLK users=2
    ... same aliases


> So refcounting is correct.

No.  When someone's looking at the clock tree to see what clocks
are active, and thus deduce which silicon is incurring switching
costs, these "wrapper clocks" are not telling the correct story.

- Dave

Re: [PATCH 0/5] Clocklib: generic clocks framework

[David Brownell]

On Saturday 26 April 2008, Dmitry wrote:
> > I wouldn't pretend to have a comprehensive list at this point.  But from a
> > brief look, your clk_round_rate() and clk_set_rate() implementations will
> > not work for the present OMAP clock tree.  In OMAP, many parent clocks do
> > not have the same rate as their children.
> 
> You can easily override any calculations in your clk->set_rate/clk->round_rate/
> clk->get_rate functions.

Speaking of which:  those clk_*() interfaces need to be better
specified.  Right now they're too vague to support fully portable
callers.

 - What kind of "rounding" is provided?  Using a 10 MHz target
   rate as an example, with a 48 MHz base and a binary divider:
     * "not-lower-than" would give 12 MHz (divide-by-4)
     * "not-higher-than" would give 6 MHz (divide-by-8)
     * "closest" would give 12 MHz, only 2 MHz off
   The differences can matter, depending on what the clock drives.

   Similarly, for DPLL based clocks (out = (in/DIV)*MUL) there
   can be "lowest power" goals, like "use biggest DIV that
   produces an output within <this> error bound". 

 - Does clk_set_rate() round, or does it fail when it can't set
   that exact rate?  (Would an 0.05% difference matter?)

This issue is orthogonal to whether clocklib merges or not (and
if so, when) ... except that if it does merge, then the answers
from clocklib will become the de facto answer to those questions.

- Dave

Re: [PATCH 0/5] Clocklib: generic clocks framework

[Paul Walmsley]

Hello Dmitry,

On Sat, 26 Apr 2008, Dmitry wrote:

> 2008/4/25, Paul Walmsley <paul@pwsan.com>:
> >
> > I wouldn't pretend to have a comprehensive list at this point.  But from a
> >  brief look, your clk_round_rate() and clk_set_rate() implementations will
> >  not work for the present OMAP clock tree.  In OMAP, many parent clocks do
> >  not have the same rate as their children.
> 
> You can easily override any calculations in your clk->set_rate/clk->round_rate/
> clk->get_rate functions.

The problem is that parent clocks shouldn't be automatically set to the 
same rate as child clocks.  We could probably hack around it in 
clk->set_rate to ignore those spurious rate changes, but really they 
shouldn't be done in the first place, at least on OMAP.  clk->set_rate 
should handle parent rate changes if necessary.  Removing this from 
clocklib looks trivial - involves deleting about ten lines of clocklib 
code?

> My first goals are:
> 1) to have an infrastructure to plug in my clocks in a platform-independant way

Doesn't the existing clock framework do this already?  Or are you really 
referring to your clk_functions/multiple consumer additions?  If the 
latter, then we all would be better served by discussing why the existing 
clock interface doesn't meet your needs, and figuring out what to do about 
it, rather than by merging code that initially appears to be common, but 
with you having the ultimate intention of extending the common clock 
interface to implement the features that you want.

> 2) to drop lots of copies of nearly the same code.

I don't really see much of a problem with the current situation, as long 
as all of the implementations use the same interface.  Right now, 
architectures are free to change their underlying code without having to 
worry about breaking other architectures or dealing with new gatekeepers, 
and that's a big plus for the status quo.  Russell is the person who 
shoulders the burden here of multiple implementations, and when this 
becomes a pressing problem, I would expect him to be doing most of the 
advocacy for common code.

> >  Assumptions that you make in clocklib may not work well for future chips.
> >  Changing clocklib will affect many architectures.  For example, perhaps
> >  someone may wish to implement clocks as an actual in-memory tree rather
> >  than a list.  Or perhaps someone may need to handle clock usecounting
> >  differently, for situations when multiple clocks might share the same
> >  enable/disable code, but with different parents.
> 
> Sorry, but WTF? Do you prefer to keep a lot of code and disallow
> merging a generification just because of some-that-may-even-not-exist
> cases? That sounds
> pretty... strange.
> And your examples are really strange.

Those examples are real; they have been discussed in the past or proposed 
for the future for OMAP clock framework.  Regarding the second example, 
that's a very real issue for us right now with our interface clocks, and 
flexible usecounting is one of the ways that we've considered solving it.  
As far as the tree example goes, we've got about 200 struct clks in the 
latest OMAP clock framework, and this will probably increase by 50% over 
the next year.  We have clock tree operations that have to move both up 
and down the tree.  Right now we're doing sequential scans on a clock 
list, but chances are quite high that we will be doing this very 
differently a few months.

I cited these examples not because I was interested in having you tell me 
that my examples were unrealistic or "strange" or should be done 
differently, but simply to reinforce the message that a top-down approach 
to a generic clocklib is not likely to be appreciated much by maintainers.  
I know you keep saying that your code is intended to be be strictly 
optional.  If you want to prove it, why not consider a bottom-up approach 
instead?  Right now you've got patches that convert two architectures to 
use your clocklib.  Instead of using a common library, just duplicate your 
clock code for each architecture.  There's not much to it.  Propose 
patches to individual architecture maintainers, and persuade them to 
replace their existing code with your own.  

If you can get a large fraction to use your identical code, and agree that 
it's at least as good as what they're using right now, then you'd have a 
pretty strong case for creating some kind of common library.  If not, 
well, those maintainers probably would not switch to your current common 
clocklib either, so there's really no downside.


- Paul

Re: [PATCH 0/5] Clocklib: generic clocks framework

[Dmitry]

Hi,


2008/5/2, Paul Walmsley <paul@pwsan.com>:
> Hello Dmitry,
>
>
>  On Sat, 26 Apr 2008, Dmitry wrote:
>
>  > 2008/4/25, Paul Walmsley <paul@pwsan.com>:
>  > >
>
> > > I wouldn't pretend to have a comprehensive list at this point.  But from a
>  > >  brief look, your clk_round_rate() and clk_set_rate() implementations will
>  > >  not work for the present OMAP clock tree.  In OMAP, many parent clocks do
>  > >  not have the same rate as their children.
>  >
>  > You can easily override any calculations in your clk->set_rate/clk->round_rate/
>  > clk->get_rate functions.
>
>
> The problem is that parent clocks shouldn't be automatically set to the
>  same rate as child clocks.  We could probably hack around it in
>  clk->set_rate to ignore those spurious rate changes, but really they
>  shouldn't be done in the first place, at least on OMAP.  clk->set_rate
>  should handle parent rate changes if necessary.  Removing this from
>  clocklib looks trivial - involves deleting about ten lines of clocklib
>  code?

OK. I'll introduce the clk_set_rate_propagate that should be used as a
->set_rate callback. The get_rate logic (to default to the parent
clock) seems to be OK from my POV. Will that suite you?

>
>
>  > My first goals are:
>  > 1) to have an infrastructure to plug in my clocks in a platform-independant way
>
>
> Doesn't the existing clock framework do this already?  Or are you really
>  referring to your clk_functions/multiple consumer additions?  If the
>  latter, then we all would be better served by discussing why the existing
>  clock interface doesn't meet your needs, and figuring out what to do about
>  it, rather than by merging code that initially appears to be common, but
>  with you having the ultimate intention of extending the common clock
>  interface to implement the features that you want.

Yes and no. Nearly all platforms have some type of clk_register. But
to use it you have to burry deeply in the platform code. There is no
easy way for a driver to provide clocks on both the PXA and SA-1100
arms, not telling about other platforms.

>
>
>  > 2) to drop lots of copies of nearly the same code.
>
>
> I don't really see much of a problem with the current situation, as long
>  as all of the implementations use the same interface.  Right now,
>  architectures are free to change their underlying code without having to
>  worry about breaking other architectures or dealing with new gatekeepers,
>  and that's a big plus for the status quo.  Russell is the person who
>  shoulders the burden here of multiple implementations, and when this
>  becomes a pressing problem, I would expect him to be doing most of the
>  advocacy for common code.

Nearly all platform-dependant code is still left in hands of platform
maintainers. There are only few assumptions built into the clocklib.
And I've just agreed to remove one of them.

>
>
>  > >  Assumptions that you make in clocklib may not work well for future chips.
>  > >  Changing clocklib will affect many architectures.  For example, perhaps
>  > >  someone may wish to implement clocks as an actual in-memory tree rather
>  > >  than a list.  Or perhaps someone may need to handle clock usecounting
>  > >  differently, for situations when multiple clocks might share the same
>  > >  enable/disable code, but with different parents.
>  >
>  > Sorry, but WTF? Do you prefer to keep a lot of code and disallow
>  > merging a generification just because of some-that-may-even-not-exist
>  > cases? That sounds
>  > pretty... strange.
>  > And your examples are really strange.
>
>
> Those examples are real; they have been discussed in the past or proposed
>  for the future for OMAP clock framework.  Regarding the second example,
>  that's a very real issue for us right now with our interface clocks, and
>  flexible usecounting is one of the ways that we've considered solving it.
>  As far as the tree example goes, we've got about 200 struct clks in the
>  latest OMAP clock framework, and this will probably increase by 50% over
>  the next year.  We have clock tree operations that have to move both up
>  and down the tree.  Right now we're doing sequential scans on a clock
>  list, but chances are quite high that we will be doing this very
>  differently a few months.

Tell me more please about "flexible usecounting". Or please provide
pointers to relevant threads I can read.

>
>  I cited these examples not because I was interested in having you tell me
>  that my examples were unrealistic or "strange" or should be done
>  differently, but simply to reinforce the message that a top-down approach
>  to a generic clocklib is not likely to be appreciated much by maintainers.
>  I know you keep saying that your code is intended to be be strictly
>  optional.  If you want to prove it, why not consider a bottom-up approach
>  instead?  Right now you've got patches that convert two architectures to
>  use your clocklib.  Instead of using a common library, just duplicate your
>  clock code for each architecture.  There's not much to it.  Propose
>  patches to individual architecture maintainers, and persuade them to
>  replace their existing code with your own.

I don't think that's the way to go. It's pretty stupid to force all
maintainers to agree to worse code only to make it better.

I still didn't receive any strong objections regarding the PXA and
SA-1100. Few other ARM architectures that have simple clk.h would gain
from clocklib by being able to drop non-platform-specific code. The
AVR32 maintainer seems to be looking positievly to adapting clocklib
once it's merged.

Maybe we should merge the core part only for now, to let all platform
maintainers consider it, think about it, provide patches to clocklib
that will be carefully though out and later (when dust settles) merge
the remaining bits of platform support?

-- 
With best wishes
Dmitry

Re: [PATCH 0/5] Clocklib: generic clocks framework

[Pavel Machek]

Hi!

> > 2) to drop lots of copies of nearly the same code.
> 
> I don't really see much of a problem with the current situation, as long 
> as all of the implementations use the same interface.  Right now, 

There's no "same interface". The interface looks similar on the
surface, but it seems mach-* maintainers can (and do) hack it as they
see fit.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html