[RFC][PATCH 0/2] Clocklib: generic clocks framework

[RFC][PATCH 0/2] Clocklib: generic clocks framework

[Dmitry Baryshkov]

Hi,

In the followup mails I'd like to introduce a bit new approach for
"clocklib".
First, it's based on kobjects. So we do get sysfs integration nearly for
free. Also now struct clk is completely opaque. So we can change it's
implementation without impacting clocks providers.

Also in clk_ops I've merged (as suggested by Haavard) the set_rate and
round_rate calls since they usually have lots of common code.

The clk_set_parent() isn't implemented yet, but this code is only an RFC!

-- 
With best wishes
Dmitry

[RFC][PATCH 1/2] 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 |   64 +++++++
 lib/Kconfig              |    3 +
 lib/Makefile             |    1 +
 lib/clocklib.c           |  432 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 500 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..31ca048
--- /dev/null
+++ b/include/linux/clocklib.h
@@ -0,0 +1,64 @@
+/*
+ * Copyright (C) 2008 Dmitry Baryshkov
+ *
+ * This file is released under the GPL v2.
+ */
+#ifndef CLOCKLIB_H
+#define CLOCKLIB_H
+
+struct device;
+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
+ * @round_rate: adjust a reate to the exact rate a clock can provide and possibly apply it
+ * @format: output any additional information for a clock
+ *
+ * This structure specifies clock operations that are used to configure
+ * specific clock.
+ */
+struct clk_ops {
+	int (*can_get)(struct clk *clk, void *data, struct device *dev);
+	int (*set_parent)(struct clk *clk, void *data, struct clk *parent);
+	int (*set_mode)(struct clk *clk, void *data, bool enable);
+	unsigned long (*get_rate)(struct clk *clk, void *data);
+	long int (*round_rate)(struct clk *clk, void *data, unsigned long rate, bool apply);
+};
+
+
+struct clk *clk_alloc(struct clk *parent, const struct clk_ops *ops,
+		void * data, const char *fmt, ...)
+		__attribute__((format(printf, 4, 5)));
+void clk_free(struct clk *clk);
+
+/*
+ * struct clk_table - a helper struct to list clocks and their parents in the table
+ * @parent_name - the name of parent clock if you'd like to find it at the registration time
+ * @parent_dev - the device corresponding to the parent clock
+ * @name - the name of the clock
+ * @ops - operations corresponding to the clock
+ * @data - clock-specific data
+ * @clk - this receives the new clock
+ */
+struct clk_table {
+	const char *parent_name;
+	struct device *parent_dev;
+
+	const char *name;
+	const struct clk_ops *ops;
+	void *data;
+
+	struct clk *clk;
+};
+
+int clks_register(struct clk_table *clks, int size);
+
+extern const struct clk_ops clk_dev_ops;
+
+#endif
+
+
diff --git a/lib/Kconfig b/lib/Kconfig
index 8cc8e87..f50b04a 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 CLOCKLIB
+	tristate
+
 config CRC_CCITT
 	tristate "CRC-CCITT functions"
 	help
diff --git a/lib/Makefile b/lib/Makefile
index 74b0cfb..c5b4cc3 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
+lib-$(CONFIG_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..9fda0cd
--- /dev/null
+++ b/lib/clocklib.c
@@ -0,0 +1,432 @@
+/*
+ * lib/clocklib.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/clocklib.h>
+#include <linux/kobject.h>
+#include <linux/err.h>
+#include <linux/spinlock.h>
+#include <linux/clk.h>
+
+struct clk {
+	struct kobject kobj;
+
+	int usage;
+
+	const struct clk_ops *ops;
+	void *data;
+};
+
+
+static DEFINE_SPINLOCK(clocks_lock);
+
+static void clk_release(struct kobject *kobj)
+{
+	struct clk *clk = container_of(kobj, struct clk, kobj);
+
+	kfree(clk);
+}
+
+static ssize_t clk_usage_show(struct kobject *kobj,
+				struct kobj_attribute *attr,
+				char *buf)
+{
+	struct clk *clk = container_of(kobj, struct clk, kobj);
+	unsigned long flags;
+	int val;
+
+	spin_lock_irqsave(&clocks_lock, flags);
+
+	val = clk->usage;
+
+	spin_unlock_irqrestore(&clocks_lock, flags);
+
+	return snprintf(buf, PAGE_SIZE, "%d", val);
+}
+
+static struct kobj_attribute clk_usage_attr =
+	__ATTR(usage, 0444, clk_usage_show, NULL);
+
+static ssize_t clk_rate_show(struct kobject *kobj,
+				struct kobj_attribute *attr,
+				char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "%lu",
+		clk_get_rate(container_of(kobj, struct clk, kobj)));
+}
+
+static struct kobj_attribute clk_rate_attr =
+	__ATTR(rate, 0444, clk_rate_show, NULL);
+
+
+static struct attribute *clk_attrs[] = {
+	&clk_usage_attr.attr,
+	&clk_rate_attr.attr,
+	NULL,
+};
+
+static struct kobj_type clk_ktype = {
+	.release = clk_release,
+	.sysfs_ops = &kobj_sysfs_ops,
+	.default_attrs = clk_attrs,
+};
+
+static struct kset *clks_kset;
+
+static int clk_sysfs_init(void)
+{
+	clks_kset = kset_create_and_add("clocks", NULL, kernel_kobj);
+
+	if (!clks_kset)
+		return -ENOMEM;
+
+	return 0;
+}
+core_initcall(clk_sysfs_init);
+
+static inline struct clk *__clk_get_parent(struct clk *clk)
+{
+	return clk->kobj.parent == &clks_kset->kobj ?
+		NULL :
+		container_of(kobject_get(clk->kobj.parent), struct clk, kobj);
+}
+
+struct clk *clk_get_parent(struct clk *clk)
+{
+	unsigned long flags;
+	struct clk *parent;
+
+	spin_lock_irqsave(&clocks_lock, flags);
+
+	parent = __clk_get_parent(clk);
+
+	spin_unlock_irqrestore(&clocks_lock, flags);
+
+	return parent;
+}
+
+
+struct clk *clk_alloc(struct clk *parent, const struct clk_ops *ops,
+		void * data, const char *fmt, ...)
+{
+	struct clk *clk = kzalloc(sizeof(*clk), GFP_ATOMIC);
+	char *name;
+	va_list args;
+	int rc;
+
+	if (!clk)
+		return ERR_PTR(-ENOMEM);
+
+	clk->kobj.kset = clks_kset;
+
+	clk->ops = ops;
+	clk->data = data;
+
+	va_start(args, fmt);
+	name = kvasprintf(GFP_KERNEL, fmt, args);
+	va_end(args);
+	if (!name) {
+		kfree(clk);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	rc = kobject_init_and_add(&clk->kobj, &clk_ktype,
+			parent? &parent->kobj : NULL,
+			"%s", name);
+
+	kfree(name);
+
+	if (rc) {
+		kfree(clk);
+		clk = ERR_PTR(rc);
+	}
+
+	return clk;
+}
+EXPORT_SYMBOL(clk_alloc);
+
+void clk_free(struct clk *clk)
+{
+	unsigned long flags;
+	spin_lock_irqsave(&clocks_lock, flags);
+
+	clk->ops = NULL;
+	clk->data = NULL;
+
+	kobject_put(&clk->kobj);
+
+	spin_unlock_irqrestore(&clocks_lock, flags);
+}
+EXPORT_SYMBOL(clk_free);
+
+int clks_register(struct clk_table *clks, int size)
+{
+	int i;
+	int rc = 0;
+	for (i = 0; i < size; i++) {
+		struct clk *parent = NULL;
+		struct clk *clk;
+		if (clks[i].parent_name) {
+			parent = clk_get(clks[i].parent_dev, clks[i].parent_name);
+			if (!parent) {
+				rc = -ENOENT;
+				i--;
+				break;
+			}
+		}
+		clk = clk_alloc(parent, clks[i].ops,
+				clks[i].data, "%s", clks[i].name);
+		if (parent)
+			clk_put(parent);
+		if (IS_ERR(clk)) {
+			rc = PTR_ERR(clk);
+			i --;
+			break;
+		}
+
+		clks[i].clk = clk;
+	}
+
+	if (rc) {
+		for ( ; i >= 0; i--) {
+			clk_free(clks[i].clk);
+			clks[i].clk = NULL;
+		}
+	}
+
+	return rc;
+}
+EXPORT_SYMBOL(clks_register);
+
+
+struct clk *clk_get(struct device *dev, const char *name)
+{
+	struct clk *clk = ERR_PTR(-ENOENT);
+	struct clk *p;
+	unsigned long flags;
+	struct kobject *k;
+
+	/* Take both clocks_lock and kset lock */
+	spin_lock_irqsave(&clocks_lock, flags);
+	spin_lock(&clks_kset->list_lock);
+
+	list_for_each_entry(k, &clks_kset->list, entry) {
+		if (kobject_name(k) && !strcmp(kobject_name(k), name)
+				) {
+			p = container_of(kobject_get(k), struct clk, kobj);
+			if (p->ops && p->ops->can_get &&
+					p->ops->can_get(p, p->data, dev)) {
+				clk = p;
+				break;
+			}
+			kobject_put(k);
+		}
+	}
+
+	list_for_each_entry(k, &clks_kset->list, entry) {
+		if (kobject_name(k) && !strcmp(kobject_name(k), name)
+				) {
+			p = container_of(kobject_get(k), struct clk, kobj);
+			if (!p->ops || !p->ops->can_get) {
+				clk = p;
+				break;
+			}
+			kobject_put(k);
+			break;
+		}
+	}
+
+	spin_unlock(&clks_kset->list_lock);
+	spin_unlock_irqrestore(&clocks_lock, flags);
+
+	return clk;
+}
+EXPORT_SYMBOL(clk_get);
+
+void clk_put(struct clk *clk)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&clocks_lock, flags);
+
+	kobject_put(&clk->kobj);
+
+	spin_unlock_irqrestore(&clocks_lock, flags);
+}
+EXPORT_SYMBOL(clk_put);
+
+static int __clk_enable(struct clk *clk)
+{
+	int rc;
+
+	if (clk->usage ++ != 0)
+		return 0;
+
+	if (!clk->ops || !clk->ops->set_mode)
+		return 0;
+
+	rc = clk->ops->set_mode(clk, clk->data, true);
+	if (rc) {
+		clk->ops->set_mode(clk, clk->data, false);
+		clk->usage --;
+	}
+
+	return rc;
+}
+
+int clk_enable(struct clk *clk)
+{
+	unsigned long flags;
+	int rc = 0;
+
+	spin_lock_irqsave(&clocks_lock, flags);
+
+	rc = __clk_enable(clk);
+
+	spin_unlock_irqrestore(&clocks_lock, flags);
+	return rc;
+}
+EXPORT_SYMBOL(clk_enable);
+
+static int __clk_disable(struct clk *clk)
+{
+	clk->usage --;
+
+	WARN_ON(clk->usage < 0);
+
+	if (clk->usage != 0)
+		return 0;
+
+	if (!clk->ops || !clk->ops->set_mode)
+		return 0;
+
+	return clk->ops->set_mode(clk, clk->data, false);
+}
+
+void clk_disable(struct clk *clk)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&clocks_lock, flags);
+
+	__clk_disable(clk);
+
+	spin_unlock_irqrestore(&clocks_lock, flags);
+}
+EXPORT_SYMBOL(clk_disable);
+
+unsigned long clk_get_rate(struct clk *clk)
+{
+	unsigned long flags;
+
+	unsigned long rate = 0;
+
+	spin_lock_irqsave(&clocks_lock, flags);
+
+	if (!clk->ops || !clk->ops->get_rate)
+		goto out;
+
+	rate = clk->ops->get_rate(clk, clk->data);
+
+out:
+	spin_unlock_irqrestore(&clocks_lock, flags);
+
+	return rate;
+}
+EXPORT_SYMBOL(clk_get_rate);
+
+int clk_set_rate(struct clk *clk, unsigned long rate)
+{
+	unsigned long flags;
+	long rc = -EINVAL;
+
+	spin_lock_irqsave(&clocks_lock, flags);
+
+	if (!clk->ops || !clk->ops->round_rate)
+		goto out;
+
+	rc = clk->ops->round_rate(clk, clk->data, rate, true);
+
+out:
+	spin_unlock_irqrestore(&clocks_lock, flags);
+
+	return rc < 0 ? rc : 0;
+}
+EXPORT_SYMBOL(clk_set_rate);
+
+long clk_round_rate(struct clk *clk, unsigned long rate)
+{
+	unsigned long flags;
+	long rc = -EINVAL;
+
+	spin_lock_irqsave(&clocks_lock, flags);
+
+	if (!clk->ops || !clk->ops->round_rate)
+		goto out;
+
+	rc = clk->ops->round_rate(clk, clk->data, rate, false);
+
+out:
+	spin_unlock_irqrestore(&clocks_lock, flags);
+
+	return rc;
+}
+EXPORT_SYMBOL(clk_round_rate);
+
+
+static int clk_dev_can_get(struct clk *clk, void *data, struct device *dev)
+{
+	return data == dev;
+}
+
+static int clk_set_mode_parent(struct clk *clk, void *data, bool enable)
+{
+	struct clk *parent = __clk_get_parent(clk);
+	int rc;
+
+	printk("!!! %sable: %s (%s)\n", enable? "en": "dis",
+				kobject_name(&clk->kobj),
+				kobject_name(&parent->kobj));
+
+	BUG_ON(!parent);
+
+	if (enable)
+		rc = __clk_enable(parent);
+	else
+		rc = __clk_disable(parent);
+
+	clk_put(parent);
+
+	return rc;
+}
+
+static unsigned long clk_get_rate_parent(struct clk *clk, void *data)
+{
+	struct clk *parent = __clk_get_parent(clk);
+	unsigned long rate = 0;
+
+	BUG_ON(!parent);
+
+	if (!parent->ops || !parent->ops->get_rate)
+		WARN_ON(1);
+	else
+		rate = parent->ops->get_rate(parent, parent->data);
+
+	clk_put(parent);
+
+	return rate;
+}
+
+const struct clk_ops clk_dev_ops = {
+	.can_get = clk_dev_can_get,
+	.set_mode = clk_set_mode_parent,
+	.get_rate = clk_get_rate_parent,
+};
+EXPORT_SYMBOL(clk_dev_ops);
-- 
1.5.5.1


-- 
With best wishes
Dmitry

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

[Andrew Morton]

On Fri, 23 May 2008 01:21:42 +0400
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.
> 

Nobody interested in this?

Please feed the patches through scripts/checkpatch.pl sometime, have a
think about the things which it detects?

> 
> diff --git a/include/linux/clocklib.h b/include/linux/clocklib.h
> new file mode 100644
> index 0000000..31ca048
> --- /dev/null
> +++ b/include/linux/clocklib.h
> @@ -0,0 +1,64 @@
> +/*
> + * Copyright (C) 2008 Dmitry Baryshkov
> + *
> + * This file is released under the GPL v2.
> + */
> +#ifndef CLOCKLIB_H
> +#define CLOCKLIB_H
> +
> +struct device;
> +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
> + * @round_rate: adjust a reate to the exact rate a clock can provide and possibly apply it
> + * @format: output any additional information for a clock
> + *
> + * This structure specifies clock operations that are used to configure
> + * specific clock.
> + */
> +struct clk_ops {
> +	int (*can_get)(struct clk *clk, void *data, struct device *dev);
> +	int (*set_parent)(struct clk *clk, void *data, struct clk *parent);
> +	int (*set_mode)(struct clk *clk, void *data, bool enable);

`set_mode' seems to be misnamed?

We prefer to avoid functions which take a `heres-what-to-do' argument
like this.  I'd have thought that it would be cleaner to have separate
`enable' and `disable' operations?

The documentation should perhaps explain that set_mode() is called
under spin_lock_irqsave() and hence cannot do much.

> +	unsigned long (*get_rate)(struct clk *clk, void *data);
> +	long int (*round_rate)(struct clk *clk, void *data, unsigned long rate, bool apply);
> +};
> +
> +
> +struct clk *clk_alloc(struct clk *parent, const struct clk_ops *ops,
> +		void * data, const char *fmt, ...)
> +		__attribute__((format(printf, 4, 5)));
> +void clk_free(struct clk *clk);
> +
> +/*
> + * struct clk_table - a helper struct to list clocks and their parents in the table
> + * @parent_name - the name of parent clock if you'd like to find it at the registration time
> + * @parent_dev - the device corresponding to the parent clock
> + * @name - the name of the clock
> + * @ops - operations corresponding to the clock
> + * @data - clock-specific data
> + * @clk - this receives the new clock
> + */
> +struct clk_table {
> +	const char *parent_name;
> +	struct device *parent_dev;
> +
> +	const char *name;
> +	const struct clk_ops *ops;
> +	void *data;
> +
> +	struct clk *clk;
> +};
> +
> +int clks_register(struct clk_table *clks, int size);
> +
> +extern const struct clk_ops clk_dev_ops;
> +
> +#endif
> +
> +
> diff --git a/lib/Kconfig b/lib/Kconfig
> index 8cc8e87..f50b04a 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 CLOCKLIB
> +	tristate
> +
>  config CRC_CCITT
>  	tristate "CRC-CCITT functions"
>  	help
> diff --git a/lib/Makefile b/lib/Makefile
> index 74b0cfb..c5b4cc3 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
> +lib-$(CONFIG_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..9fda0cd
> --- /dev/null
> +++ b/lib/clocklib.c
> @@ -0,0 +1,432 @@
> +/*
> + * lib/clocklib.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/clocklib.h>
> +#include <linux/kobject.h>
> +#include <linux/err.h>
> +#include <linux/spinlock.h>
> +#include <linux/clk.h>
> +
> +struct clk {
> +	struct kobject kobj;
> +
> +	int usage;
> +
> +	const struct clk_ops *ops;
> +	void *data;
> +};
> +
> +
> +static DEFINE_SPINLOCK(clocks_lock);
> +
> +static void clk_release(struct kobject *kobj)
> +{
> +	struct clk *clk = container_of(kobj, struct clk, kobj);
> +
> +	kfree(clk);
> +}
> +
> +static ssize_t clk_usage_show(struct kobject *kobj,
> +				struct kobj_attribute *attr,
> +				char *buf)
> +{
> +	struct clk *clk = container_of(kobj, struct clk, kobj);
> +	unsigned long flags;
> +	int val;
> +
> +	spin_lock_irqsave(&clocks_lock, flags);
> +
> +	val = clk->usage;
> +
> +	spin_unlock_irqrestore(&clocks_lock, flags);
> +
> +	return snprintf(buf, PAGE_SIZE, "%d", val);
> +}
> +
> +static struct kobj_attribute clk_usage_attr =
> +	__ATTR(usage, 0444, clk_usage_show, NULL);
> +
> +static ssize_t clk_rate_show(struct kobject *kobj,
> +				struct kobj_attribute *attr,
> +				char *buf)
> +{
> +	return snprintf(buf, PAGE_SIZE, "%lu",
> +		clk_get_rate(container_of(kobj, struct clk, kobj)));
> +}
> +
> +static struct kobj_attribute clk_rate_attr =
> +	__ATTR(rate, 0444, clk_rate_show, NULL);
> +
> +
> +static struct attribute *clk_attrs[] = {
> +	&clk_usage_attr.attr,
> +	&clk_rate_attr.attr,
> +	NULL,
> +};
> +
> +static struct kobj_type clk_ktype = {
> +	.release = clk_release,
> +	.sysfs_ops = &kobj_sysfs_ops,
> +	.default_attrs = clk_attrs,
> +};
> +
> +static struct kset *clks_kset;
> +
> +static int clk_sysfs_init(void)
> +{
> +	clks_kset = kset_create_and_add("clocks", NULL, kernel_kobj);
> +
> +	if (!clks_kset)
> +		return -ENOMEM;
> +
> +	return 0;
> +}
> +core_initcall(clk_sysfs_init);
> +
> +static inline struct clk *__clk_get_parent(struct clk *clk)
> +{
> +	return clk->kobj.parent == &clks_kset->kobj ?
> +		NULL :
> +		container_of(kobject_get(clk->kobj.parent), struct clk, kobj);
> +}
> +
> +struct clk *clk_get_parent(struct clk *clk)
> +{
> +	unsigned long flags;
> +	struct clk *parent;
> +
> +	spin_lock_irqsave(&clocks_lock, flags);
> +
> +	parent = __clk_get_parent(clk);
> +
> +	spin_unlock_irqrestore(&clocks_lock, flags);
> +
> +	return parent;
> +}
> +
> +
> +struct clk *clk_alloc(struct clk *parent, const struct clk_ops *ops,
> +		void * data, const char *fmt, ...)
> +{
> +	struct clk *clk = kzalloc(sizeof(*clk), GFP_ATOMIC);

This could-and-should be GFP_KERNEL.

> +	char *name;
> +	va_list args;
> +	int rc;
> +
> +	if (!clk)
> +		return ERR_PTR(-ENOMEM);
> +
> +	clk->kobj.kset = clks_kset;
> +
> +	clk->ops = ops;
> +	clk->data = data;
> +
> +	va_start(args, fmt);
> +	name = kvasprintf(GFP_KERNEL, fmt, args);
> +	va_end(args);
> +	if (!name) {
> +		kfree(clk);
> +		return ERR_PTR(-ENOMEM);
> +	}
> +
> +	rc = kobject_init_and_add(&clk->kobj, &clk_ktype,
> +			parent? &parent->kobj : NULL,
> +			"%s", name);
> +
> +	kfree(name);
> +
> +	if (rc) {
> +		kfree(clk);
> +		clk = ERR_PTR(rc);
> +	}
> +
> +	return clk;
> +}
> +EXPORT_SYMBOL(clk_alloc);
> +
> +void clk_free(struct clk *clk)
> +{
> +	unsigned long flags;
> +	spin_lock_irqsave(&clocks_lock, flags);
> +
> +	clk->ops = NULL;
> +	clk->data = NULL;
> +
> +	kobject_put(&clk->kobj);
> +
> +	spin_unlock_irqrestore(&clocks_lock, flags);
> +}
> +EXPORT_SYMBOL(clk_free);
> +
> +int clks_register(struct clk_table *clks, int size)
> +{
> +	int i;
> +	int rc = 0;
> +	for (i = 0; i < size; i++) {
> +		struct clk *parent = NULL;
> +		struct clk *clk;
> +		if (clks[i].parent_name) {
> +			parent = clk_get(clks[i].parent_dev, clks[i].parent_name);
> +			if (!parent) {
> +				rc = -ENOENT;
> +				i--;
> +				break;
> +			}
> +		}
> +		clk = clk_alloc(parent, clks[i].ops,
> +				clks[i].data, "%s", clks[i].name);
> +		if (parent)
> +			clk_put(parent);
> +		if (IS_ERR(clk)) {
> +			rc = PTR_ERR(clk);
> +			i --;
> +			break;
> +		}
> +
> +		clks[i].clk = clk;
> +	}
> +
> +	if (rc) {
> +		for ( ; i >= 0; i--) {
> +			clk_free(clks[i].clk);
> +			clks[i].clk = NULL;
> +		}
> +	}
> +
> +	return rc;
> +}
> +EXPORT_SYMBOL(clks_register);

The patch generally has nice documentation, but these two major
interface functions were omitted.

> +
> +struct clk *clk_get(struct device *dev, const char *name)
> +{
> +	struct clk *clk = ERR_PTR(-ENOENT);
> +	struct clk *p;
> +	unsigned long flags;
> +	struct kobject *k;
> +
> +	/* Take both clocks_lock and kset lock */
> +	spin_lock_irqsave(&clocks_lock, flags);
> +	spin_lock(&clks_kset->list_lock);
> +
> +	list_for_each_entry(k, &clks_kset->list, entry) {
> +		if (kobject_name(k) && !strcmp(kobject_name(k), name)
> +				) {
> +			p = container_of(kobject_get(k), struct clk, kobj);
> +			if (p->ops && p->ops->can_get &&
> +					p->ops->can_get(p, p->data, dev)) {
> +				clk = p;
> +				break;
> +			}
> +			kobject_put(k);
> +		}
> +	}
> +
> +	list_for_each_entry(k, &clks_kset->list, entry) {
> +		if (kobject_name(k) && !strcmp(kobject_name(k), name)
> +				) {
> +			p = container_of(kobject_get(k), struct clk, kobj);
> +			if (!p->ops || !p->ops->can_get) {
> +				clk = p;
> +				break;
> +			}
> +			kobject_put(k);
> +			break;
> +		}
> +	}
> +
> +	spin_unlock(&clks_kset->list_lock);
> +	spin_unlock_irqrestore(&clocks_lock, flags);
> +
> +	return clk;
> +}
> +EXPORT_SYMBOL(clk_get);
> +
> +void clk_put(struct clk *clk)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&clocks_lock, flags);
> +
> +	kobject_put(&clk->kobj);
> +
> +	spin_unlock_irqrestore(&clocks_lock, flags);
> +}
> +EXPORT_SYMBOL(clk_put);

Hopefully this function will not be called at a high frequency.  If it
is, scalability will be poor.

Why does clocks_lock need to be taken in an irq-safe manner?

> +static int __clk_enable(struct clk *clk)
> +{
> +	int rc;
> +
> +	if (clk->usage ++ != 0)
> +		return 0;
> +
> +	if (!clk->ops || !clk->ops->set_mode)
> +		return 0;
> +
> +	rc = clk->ops->set_mode(clk, clk->data, true);
> +	if (rc) {
> +		clk->ops->set_mode(clk, clk->data, false);
> +		clk->usage --;
> +	}
> +
> +	return rc;
> +}
> +
> +int clk_enable(struct clk *clk)
> +{
> +	unsigned long flags;
> +	int rc = 0;
> +
> +	spin_lock_irqsave(&clocks_lock, flags);
> +
> +	rc = __clk_enable(clk);
> +
> +	spin_unlock_irqrestore(&clocks_lock, flags);
> +	return rc;
> +}
> +EXPORT_SYMBOL(clk_enable);
> +
> +static int __clk_disable(struct clk *clk)
> +{
> +	clk->usage --;
> +
> +	WARN_ON(clk->usage < 0);
> +
> +	if (clk->usage != 0)
> +		return 0;
> +
> +	if (!clk->ops || !clk->ops->set_mode)
> +		return 0;
> +
> +	return clk->ops->set_mode(clk, clk->data, false);
> +}
> +
> +void clk_disable(struct clk *clk)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&clocks_lock, flags);
> +
> +	__clk_disable(clk);
> +
> +	spin_unlock_irqrestore(&clocks_lock, flags);
> +}
> +EXPORT_SYMBOL(clk_disable);
> +
> +unsigned long clk_get_rate(struct clk *clk)
> +{
> +	unsigned long flags;
> +
> +	unsigned long rate = 0;

Unneeded newline.

> +	spin_lock_irqsave(&clocks_lock, flags);
> +
> +	if (!clk->ops || !clk->ops->get_rate)
> +		goto out;

Did we really need to hold the lock to perform this check?

Is it really valid for a clock to have no ->ops and no -ops->get_rate? 
Please consider mandating that these fields must be provided, then
remove this check.

> +	rate = clk->ops->get_rate(clk, clk->data);
> +
> +out:
> +	spin_unlock_irqrestore(&clocks_lock, flags);
> +
> +	return rate;
> +}
> +EXPORT_SYMBOL(clk_get_rate);
> +
> +int clk_set_rate(struct clk *clk, unsigned long rate)
> +{
> +	unsigned long flags;
> +	long rc = -EINVAL;
> +
> +	spin_lock_irqsave(&clocks_lock, flags);
> +
> +	if (!clk->ops || !clk->ops->round_rate)
> +		goto out;
> +
> +	rc = clk->ops->round_rate(clk, clk->data, rate, true);
> +
> +out:
> +	spin_unlock_irqrestore(&clocks_lock, flags);
> +
> +	return rc < 0 ? rc : 0;
> +}
> +EXPORT_SYMBOL(clk_set_rate);

I look at this and wonder what unit `rate' is in.  Some interface
documentation would clear that up, but even better would be to use a
more specific identifier - one which describes the quantity's units. 
For example, "hz".

> +long clk_round_rate(struct clk *clk, unsigned long rate)

ditto everywhere

> +{
> +	unsigned long flags;
> +	long rc = -EINVAL;
> +
> +	spin_lock_irqsave(&clocks_lock, flags);
> +
> +	if (!clk->ops || !clk->ops->round_rate)
> +		goto out;
> +
> +	rc = clk->ops->round_rate(clk, clk->data, rate, false);
> +
> +out:
> +	spin_unlock_irqrestore(&clocks_lock, flags);
> +
> +	return rc;
> +}
> +EXPORT_SYMBOL(clk_round_rate);

Was `long' an appropriate type here?  I'd have expected `unsigned long'.

Perhaps this can really retrun negative values.  Aditional
documentation would clear that up.

> +
> +static int clk_dev_can_get(struct clk *clk, void *data, struct device *dev)
> +{
> +	return data == dev;
> +}

This can_get operation is a bit mysterious and unusual-looking.  Why
would one not be able to get a clock device?

Again, additional changelogging and/or code commenting is needed,
because if I was wondering this now, we can expect that others will
wonder the same thing in the future.

> +static int clk_set_mode_parent(struct clk *clk, void *data, bool enable)
> +{
> +	struct clk *parent = __clk_get_parent(clk);
> +	int rc;
> +
> +	printk("!!! %sable: %s (%s)\n", enable? "en": "dis",
> +				kobject_name(&clk->kobj),
> +				kobject_name(&parent->kobj));

I guess that printk is temporary?

> +	BUG_ON(!parent);
> +
> +	if (enable)
> +		rc = __clk_enable(parent);
> +	else
> +		rc = __clk_disable(parent);
> +
> +	clk_put(parent);
> +
> +	return rc;
> +}
> +
> +static unsigned long clk_get_rate_parent(struct clk *clk, void *data)
> +{
> +	struct clk *parent = __clk_get_parent(clk);
> +	unsigned long rate = 0;
> +
> +	BUG_ON(!parent);
> +
> +	if (!parent->ops || !parent->ops->get_rate)
> +		WARN_ON(1);
> +	else
> +		rate = parent->ops->get_rate(parent, parent->data);
> +
> +	clk_put(parent);
> +
> +	return rate;
> +}

Can we avoid the (mysterious) void* here?  Do something which will
allow the C type system to check our stuff?

> +const struct clk_ops clk_dev_ops = {
> +	.can_get = clk_dev_can_get,
> +	.set_mode = clk_set_mode_parent,
> +	.get_rate = clk_get_rate_parent,
> +};
> +EXPORT_SYMBOL(clk_dev_ops);

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

[Dmitry Baryshkov]

Hi,

On Tue, May 27, 2008 at 03:09:19PM -0700, Andrew Morton wrote:
> On Fri, 23 May 2008 01:21:42 +0400
> 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.
> > 
> 
> Nobody interested in this?
> 
> Please feed the patches through scripts/checkpatch.pl sometime, have a
> think about the things which it detects?

OK, I'll try not to forget it before next round :)

> 
> > 
> > diff --git a/include/linux/clocklib.h b/include/linux/clocklib.h
> > new file mode 100644
> > index 0000000..31ca048
> > --- /dev/null
> > +++ b/include/linux/clocklib.h
> > @@ -0,0 +1,64 @@
> > +/*
> > + * Copyright (C) 2008 Dmitry Baryshkov
> > + *
> > + * This file is released under the GPL v2.
> > + */
> > +#ifndef CLOCKLIB_H
> > +#define CLOCKLIB_H
> > +
> > +struct device;
> > +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
> > + * @round_rate: adjust a reate to the exact rate a clock can provide and possibly apply it
> > + * @format: output any additional information for a clock
> > + *
> > + * This structure specifies clock operations that are used to configure
> > + * specific clock.
> > + */
> > +struct clk_ops {
> > +	int (*can_get)(struct clk *clk, void *data, struct device *dev);
> > +	int (*set_parent)(struct clk *clk, void *data, struct clk *parent);
> > +	int (*set_mode)(struct clk *clk, void *data, bool enable);
> 
> `set_mode' seems to be misnamed?
> 
> We prefer to avoid functions which take a `heres-what-to-do' argument
> like this.  I'd have thought that it would be cleaner to have separate
> `enable' and `disable' operations?

This was suggested before by Haavard. Probably the reason was space
reduction.

> The documentation should perhaps explain that set_mode() is called
> under spin_lock_irqsave() and hence cannot do much.

All functions from clk_ops are run with spinlock held.

> > +	unsigned long (*get_rate)(struct clk *clk, void *data);
> > +	long int (*round_rate)(struct clk *clk, void *data, unsigned long rate, bool apply);
> > +};
> > +
> > +

[skipped]

> > +
> > +struct clk *clk_alloc(struct clk *parent, const struct clk_ops *ops,
> > +		void * data, const char *fmt, ...)
> > +{
> > +	struct clk *clk = kzalloc(sizeof(*clk), GFP_ATOMIC);
> 
> This could-and-should be GFP_KERNEL.

ACK.

> 
> > +	char *name;
> > +	va_list args;
> > +	int rc;
> > +
> > +	if (!clk)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	clk->kobj.kset = clks_kset;
> > +
> > +	clk->ops = ops;
> > +	clk->data = data;
> > +
> > +	va_start(args, fmt);
> > +	name = kvasprintf(GFP_KERNEL, fmt, args);
> > +	va_end(args);
> > +	if (!name) {
> > +		kfree(clk);
> > +		return ERR_PTR(-ENOMEM);
> > +	}
> > +
> > +	rc = kobject_init_and_add(&clk->kobj, &clk_ktype,
> > +			parent? &parent->kobj : NULL,
> > +			"%s", name);
> > +
> > +	kfree(name);
> > +
> > +	if (rc) {
> > +		kfree(clk);
> > +		clk = ERR_PTR(rc);
> > +	}
> > +
> > +	return clk;
> > +}
> > +EXPORT_SYMBOL(clk_alloc);
> > +
> > +void clk_free(struct clk *clk)
> > +{
> > +	unsigned long flags;
> > +	spin_lock_irqsave(&clocks_lock, flags);
> > +
> > +	clk->ops = NULL;
> > +	clk->data = NULL;
> > +
> > +	kobject_put(&clk->kobj);
> > +
> > +	spin_unlock_irqrestore(&clocks_lock, flags);
> > +}
> > +EXPORT_SYMBOL(clk_free);
> > +
> > +int clks_register(struct clk_table *clks, int size)
> > +{
> > +	int i;
> > +	int rc = 0;
> > +	for (i = 0; i < size; i++) {
> > +		struct clk *parent = NULL;
> > +		struct clk *clk;
> > +		if (clks[i].parent_name) {
> > +			parent = clk_get(clks[i].parent_dev, clks[i].parent_name);
> > +			if (!parent) {
> > +				rc = -ENOENT;
> > +				i--;
> > +				break;
> > +			}
> > +		}
> > +		clk = clk_alloc(parent, clks[i].ops,
> > +				clks[i].data, "%s", clks[i].name);
> > +		if (parent)
> > +			clk_put(parent);
> > +		if (IS_ERR(clk)) {
> > +			rc = PTR_ERR(clk);
> > +			i --;
> > +			break;
> > +		}
> > +
> > +		clks[i].clk = clk;
> > +	}
> > +
> > +	if (rc) {
> > +		for ( ; i >= 0; i--) {
> > +			clk_free(clks[i].clk);
> > +			clks[i].clk = NULL;
> > +		}
> > +	}
> > +
> > +	return rc;
> > +}
> > +EXPORT_SYMBOL(clks_register);
> 
> The patch generally has nice documentation, but these two major
> interface functions were omitted.

Oops...

> 
> > +
> > +struct clk *clk_get(struct device *dev, const char *name)
> > +{
> > +	struct clk *clk = ERR_PTR(-ENOENT);
> > +	struct clk *p;
> > +	unsigned long flags;
> > +	struct kobject *k;
> > +
> > +	/* Take both clocks_lock and kset lock */
> > +	spin_lock_irqsave(&clocks_lock, flags);
> > +	spin_lock(&clks_kset->list_lock);
> > +
> > +	list_for_each_entry(k, &clks_kset->list, entry) {
> > +		if (kobject_name(k) && !strcmp(kobject_name(k), name)
> > +				) {
> > +			p = container_of(kobject_get(k), struct clk, kobj);
> > +			if (p->ops && p->ops->can_get &&
> > +					p->ops->can_get(p, p->data, dev)) {
> > +				clk = p;
> > +				break;
> > +			}
> > +			kobject_put(k);
> > +		}
> > +	}
> > +
> > +	list_for_each_entry(k, &clks_kset->list, entry) {
> > +		if (kobject_name(k) && !strcmp(kobject_name(k), name)
> > +				) {
> > +			p = container_of(kobject_get(k), struct clk, kobj);
> > +			if (!p->ops || !p->ops->can_get) {
> > +				clk = p;
> > +				break;
> > +			}
> > +			kobject_put(k);
> > +			break;
> > +		}
> > +	}
> > +
> > +	spin_unlock(&clks_kset->list_lock);
> > +	spin_unlock_irqrestore(&clocks_lock, flags);
> > +
> > +	return clk;
> > +}
> > +EXPORT_SYMBOL(clk_get);
> > +
> > +void clk_put(struct clk *clk)
> > +{
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&clocks_lock, flags);
> > +
> > +	kobject_put(&clk->kobj);
> > +
> > +	spin_unlock_irqrestore(&clocks_lock, flags);
> > +}
> > +EXPORT_SYMBOL(clk_put);
> 
> Hopefully this function will not be called at a high frequency.  If it
> is, scalability will be poor.

Usually clk_get/clk_put are called only during probes/remove of the
device/driver.

> 
> Why does clocks_lock need to be taken in an irq-safe manner?

I just wanted to be sure that most functions will be irq-safe.
Probably clk_get/_put can use plain spin_lock/_unlock, but I'm
not sure if this is justifable.

> 
> > +static int __clk_enable(struct clk *clk)
> > +{
> > +	int rc;
> > +
> > +	if (clk->usage ++ != 0)
> > +		return 0;
> > +
> > +	if (!clk->ops || !clk->ops->set_mode)
> > +		return 0;
> > +
> > +	rc = clk->ops->set_mode(clk, clk->data, true);
> > +	if (rc) {
> > +		clk->ops->set_mode(clk, clk->data, false);
> > +		clk->usage --;
> > +	}
> > +
> > +	return rc;
> > +}
> > +
> > +int clk_enable(struct clk *clk)
> > +{
> > +	unsigned long flags;
> > +	int rc = 0;
> > +
> > +	spin_lock_irqsave(&clocks_lock, flags);
> > +
> > +	rc = __clk_enable(clk);
> > +
> > +	spin_unlock_irqrestore(&clocks_lock, flags);
> > +	return rc;
> > +}
> > +EXPORT_SYMBOL(clk_enable);
> > +
> > +static int __clk_disable(struct clk *clk)
> > +{
> > +	clk->usage --;
> > +
> > +	WARN_ON(clk->usage < 0);
> > +
> > +	if (clk->usage != 0)
> > +		return 0;
> > +
> > +	if (!clk->ops || !clk->ops->set_mode)
> > +		return 0;
> > +
> > +	return clk->ops->set_mode(clk, clk->data, false);
> > +}
> > +
> > +void clk_disable(struct clk *clk)
> > +{
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&clocks_lock, flags);
> > +
> > +	__clk_disable(clk);
> > +
> > +	spin_unlock_irqrestore(&clocks_lock, flags);
> > +}
> > +EXPORT_SYMBOL(clk_disable);
> > +
> > +unsigned long clk_get_rate(struct clk *clk)
> > +{
> > +	unsigned long flags;
> > +
> > +	unsigned long rate = 0;
> 
> Unneeded newline.

ack

> 
> > +	spin_lock_irqsave(&clocks_lock, flags);
> > +
> > +	if (!clk->ops || !clk->ops->get_rate)
> > +		goto out;
> 
> Did we really need to hold the lock to perform this check?
> 
> Is it really valid for a clock to have no ->ops and no -ops->get_rate? 
> Please consider mandating that these fields must be provided, then
> remove this check.

With this patch clocks can be registered and unregistered at any time.
When the clock in unregistered I drop the reference to the clk_ops (so
the module can be unregistered, but if the clock is still referenced,
those functions still can be called. That's why I have to check for the
presense of ->ops. On the other hand ->ops->get_rate can be made mandatory.

> 
> > +	rate = clk->ops->get_rate(clk, clk->data);
> > +
> > +out:
> > +	spin_unlock_irqrestore(&clocks_lock, flags);
> > +
> > +	return rate;
> > +}
> > +EXPORT_SYMBOL(clk_get_rate);
> > +
> > +int clk_set_rate(struct clk *clk, unsigned long rate)
> > +{
> > +	unsigned long flags;
> > +	long rc = -EINVAL;
> > +
> > +	spin_lock_irqsave(&clocks_lock, flags);
> > +
> > +	if (!clk->ops || !clk->ops->round_rate)
> > +		goto out;
> > +
> > +	rc = clk->ops->round_rate(clk, clk->data, rate, true);
> > +
> > +out:
> > +	spin_unlock_irqrestore(&clocks_lock, flags);
> > +
> > +	return rc < 0 ? rc : 0;
> > +}
> > +EXPORT_SYMBOL(clk_set_rate);
> 
> I look at this and wonder what unit `rate' is in.  Some interface
> documentation would clear that up, but even better would be to use a
> more specific identifier - one which describes the quantity's units. 
> For example, "hz".

It's documented in the <linux/clk.h>. However I can change it in both
files if you like.

> 
> > +long clk_round_rate(struct clk *clk, unsigned long rate)
> 
> ditto everywhere
> 
> > +{
> > +	unsigned long flags;
> > +	long rc = -EINVAL;
> > +
> > +	spin_lock_irqsave(&clocks_lock, flags);
> > +
> > +	if (!clk->ops || !clk->ops->round_rate)
> > +		goto out;
> > +
> > +	rc = clk->ops->round_rate(clk, clk->data, rate, false);
> > +
> > +out:
> > +	spin_unlock_irqrestore(&clocks_lock, flags);
> > +
> > +	return rc;
> > +}
> > +EXPORT_SYMBOL(clk_round_rate);
> 
> Was `long' an appropriate type here?  I'd have expected `unsigned long'.
> 
> Perhaps this can really retrun negative values.  Aditional
> documentation would clear that up.

See <linux/clk.h>.

> 
> > +
> > +static int clk_dev_can_get(struct clk *clk, void *data, struct device *dev)
> > +{
> > +	return data == dev;
> > +}
> 
> This can_get operation is a bit mysterious and unusual-looking.  Why
> would one not be able to get a clock device?

Maybe it's misnamed. It checks whether this particular "struct clk" is
suitable for passed device. E.g. for the uniformity of the driver the
PXA names all clocks "UARTCLK" and binds each of them to the particular
UART device in the system. Some other platforms would like to compare
the id of the device on the platform_bus with the value stored in the
clock. To make this check generic, I've provided the possibility for
each particular clock to define it's own way to be bound to devices.

> 
> Again, additional changelogging and/or code commenting is needed,
> because if I was wondering this now, we can expect that others will
> wonder the same thing in the future.

I'll add the text above as a comment.

> 
> > +static int clk_set_mode_parent(struct clk *clk, void *data, bool enable)
> > +{
> > +	struct clk *parent = __clk_get_parent(clk);
> > +	int rc;
> > +
> > +	printk("!!! %sable: %s (%s)\n", enable? "en": "dis",
> > +				kobject_name(&clk->kobj),
> > +				kobject_name(&parent->kobj));
> 
> I guess that printk is temporary?

Of course :)

> 
> > +	BUG_ON(!parent);
> > +
> > +	if (enable)
> > +		rc = __clk_enable(parent);
> > +	else
> > +		rc = __clk_disable(parent);
> > +
> > +	clk_put(parent);
> > +
> > +	return rc;
> > +}
> > +
> > +static unsigned long clk_get_rate_parent(struct clk *clk, void *data)
> > +{
> > +	struct clk *parent = __clk_get_parent(clk);
> > +	unsigned long rate = 0;
> > +
> > +	BUG_ON(!parent);
> > +
> > +	if (!parent->ops || !parent->ops->get_rate)
> > +		WARN_ON(1);
> > +	else
> > +		rate = parent->ops->get_rate(parent, parent->data);
> > +
> > +	clk_put(parent);
> > +
> > +	return rate;
> > +}
> 
> Can we avoid the (mysterious) void* here?  Do something which will
> allow the C type system to check our stuff?

I don't know how we can avoid it. Each clock can have "private" data
bound to it. Previously the proposed way was to embed struct clk. However
since struct clk is dynamic now, it's not directly possible. And all
solutions I can think upon are more or less hacky.

-- 
With best wishes
Dmitry

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

[Haavard Skinnemoen]

Dmitry Baryshkov <dbaryshkov@gmail.com> wrote:
> Hi,
> 
> On Tue, May 27, 2008 at 03:09:19PM -0700, Andrew Morton wrote:
> > On Fri, 23 May 2008 01:21:42 +0400
> > 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.
> > > 
> > 
> > Nobody interested in this?

Sorry. I am...or at least I used to be until struct clk went all porky
and got hidden away in a .c file...

Like I've said before, I think the size of this thing is a real problem
and will prevent most arch maintainers from even considering it. The
previous version looked very good in that respect, but now the
porkiness of struct clk is back with a vengeance.

While I actually liked the previous version, there's no way I'm going
to use this version on avr32 with its 50+ clocks, and I expect the omap
people to be even more unhappy.

> > > +struct clk_ops {
> > > +	int (*can_get)(struct clk *clk, void *data, struct device *dev);
> > > +	int (*set_parent)(struct clk *clk, void *data, struct clk *parent);
> > > +	int (*set_mode)(struct clk *clk, void *data, bool enable);
> > 
> > `set_mode' seems to be misnamed?
> > 
> > We prefer to avoid functions which take a `heres-what-to-do' argument
> > like this.  I'd have thought that it would be cleaner to have separate
> > `enable' and `disable' operations?
> 
> This was suggested before by Haavard. Probably the reason was space
> reduction.

Yeah, that's true, but it was before struct clk_ops was split out from
struct clk. The size of struct clk_ops is far less important than the
size of struct clk, so feel free to reintroduce separate enable/disable
hooks.

> > The documentation should perhaps explain that set_mode() is called
> > under spin_lock_irqsave() and hence cannot do much.
> 
> All functions from clk_ops are run with spinlock held.

Why is that necessary?

I've been thinking a bit lately about starting up oscillators through
the clk framework, and that may take up to several seconds. Doing that
under an irqsafe spinlock would be madness...

> > > +EXPORT_SYMBOL(clks_register);
> > 
> > The patch generally has nice documentation, but these two major
> > interface functions were omitted.
> 
> Oops...

Where's the interface to register a single clock btw?

I have to say I really hate this "struct clk_table" thing. Another
reason why struct clk should be in a header file, not a .c file.

> > 
> > > +
> > > +struct clk *clk_get(struct device *dev, const char *name)
> > > +{
> > > +	struct clk *clk = ERR_PTR(-ENOENT);
> > > +	struct clk *p;
> > > +	unsigned long flags;
> > > +	struct kobject *k;
> > > +
> > > +	/* Take both clocks_lock and kset lock */
> > > +	spin_lock_irqsave(&clocks_lock, flags);
> > > +	spin_lock(&clks_kset->list_lock);
> > > +
> > > +	list_for_each_entry(k, &clks_kset->list, entry) {
> > > +		if (kobject_name(k) && !strcmp(kobject_name(k), name)
> > > +				) {
> > > +			p = container_of(kobject_get(k), struct clk, kobj);
> > > +			if (p->ops && p->ops->can_get &&
> > > +					p->ops->can_get(p, p->data, dev)) {
> > > +				clk = p;
> > > +				break;
> > > +			}
> > > +			kobject_put(k);
> > > +		}
> > > +	}
> > > +
> > > +	list_for_each_entry(k, &clks_kset->list, entry) {
> > > +		if (kobject_name(k) && !strcmp(kobject_name(k), name)
> > > +				) {
> > > +			p = container_of(kobject_get(k), struct clk, kobj);
> > > +			if (!p->ops || !p->ops->can_get) {
> > > +				clk = p;
> > > +				break;
> > > +			}
> > > +			kobject_put(k);
> > > +			break;
> > > +		}
> > > +	}

Why do you have to iterate over clks_kset->list twice?

> > > +	spin_lock_irqsave(&clocks_lock, flags);
> > > +
> > > +	if (!clk->ops || !clk->ops->get_rate)
> > > +		goto out;
> > 
> > Did we really need to hold the lock to perform this check?
> > 
> > Is it really valid for a clock to have no ->ops and no -ops->get_rate? 
> > Please consider mandating that these fields must be provided, then
> > remove this check.
> 
> With this patch clocks can be registered and unregistered at any time.
> When the clock in unregistered I drop the reference to the clk_ops (so
> the module can be unregistered, but if the clock is still referenced,
> those functions still can be called. That's why I have to check for the
> presense of ->ops. On the other hand ->ops->get_rate can be made mandatory.

So...how does the user know that the clock disappeared? Is it really a
good idea to allow clocks to be unregistered while someone is using it?

> > > +static unsigned long clk_get_rate_parent(struct clk *clk, void *data)
> > > +{
> > > +	struct clk *parent = __clk_get_parent(clk);
> > > +	unsigned long rate = 0;
> > > +
> > > +	BUG_ON(!parent);
> > > +
> > > +	if (!parent->ops || !parent->ops->get_rate)
> > > +		WARN_ON(1);
> > > +	else
> > > +		rate = parent->ops->get_rate(parent, parent->data);
> > > +
> > > +	clk_put(parent);
> > > +
> > > +	return rate;
> > > +}
> > 
> > Can we avoid the (mysterious) void* here?  Do something which will
> > allow the C type system to check our stuff?
> 
> I don't know how we can avoid it. Each clock can have "private" data
> bound to it. Previously the proposed way was to embed struct clk. However
> since struct clk is dynamic now, it's not directly possible. And all
> solutions I can think upon are more or less hacky.

Can't you just add some sort of additional_size parameter to the alloc
function to make it possible for clock drivers to extend it?

Removing the kobject would of course solve this too. I forgot why we
wanted a kobject in there in the first place (I think I actually said
that I _didn't_ want it.)

Haavard

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

[Dmitry]

Hi,

Haavard, few words before detailed comments. In this version of the patchset
I tried to solve the problem of dynamic unregistration of clocks.
Consider this example:
1) Module A register CLKA
2) Module B clk_gets CLKA
3) Module A is unloaded
3.1) As it's unloaded module A unregisters CLKA
4) Module B tries to clk_get_rate on CLKA

I can think of several solutions for this problem, but IMO the most clear one is
to force struct clk to be fully dynamic and to use kobjects for it's management.
This helped me:
1) to cleanup the above case
2) to cleanup that "clk_alloc_function" mess (required for clock aliasing, etc)
3) to get sysfs integration for free

Also, please, bear in mind, that I titled this version of the patchset as "RFC".
I just wanted to know your opinion on this approach. If it's less acceptable,
I'll get back to my previous patchset.

2008/5/28 Haavard Skinnemoen <haavard.skinnemoen@atmel.com>:
> Dmitry Baryshkov <dbaryshkov@gmail.com> wrote:
>> Hi,
>>
>> On Tue, May 27, 2008 at 03:09:19PM -0700, Andrew Morton wrote:
>> > On Fri, 23 May 2008 01:21:42 +0400
>> > 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.
>> > >
>> >
>> > Nobody interested in this?
>
> Sorry. I am...or at least I used to be until struct clk went all porky
> and got hidden away in a .c file...

It get hidden because in this patchset it contains only private fields
not intended for public usage.

>
> Like I've said before, I think the size of this thing is a real problem
> and will prevent most arch maintainers from even considering it. The
> previous version looked very good in that respect, but now the
> porkiness of struct clk is back with a vengeance.
>
> While I actually liked the previous version, there's no way I'm going
> to use this version on avr32 with its 50+ clocks, and I expect the omap
> people to be even more unhappy.
>
>> > > +struct clk_ops {
>> > > + int (*can_get)(struct clk *clk, void *data, struct device *dev);
>> > > + int (*set_parent)(struct clk *clk, void *data, struct clk *parent);
>> > > + int (*set_mode)(struct clk *clk, void *data, bool enable);
>> >
>> > `set_mode' seems to be misnamed?
>> >
>> > We prefer to avoid functions which take a `heres-what-to-do' argument
>> > like this.  I'd have thought that it would be cleaner to have separate
>> > `enable' and `disable' operations?
>>
>> This was suggested before by Haavard. Probably the reason was space
>> reduction.
>
> Yeah, that's true, but it was before struct clk_ops was split out from
> struct clk. The size of struct clk_ops is far less important than the
> size of struct clk, so feel free to reintroduce separate enable/disable
> hooks.

ok.

>
>> > The documentation should perhaps explain that set_mode() is called
>> > under spin_lock_irqsave() and hence cannot do much.
>>
>> All functions from clk_ops are run with spinlock held.
>
> Why is that necessary?
>
> I've been thinking a bit lately about starting up oscillators through
> the clk framework, and that may take up to several seconds. Doing that
> under an irqsafe spinlock would be madness...

Consider two kernel threads trying to enable single clock...
I'm sure locking conditions can be relaxed, but I can't come up with
the solution yet. Maybe you can?

>
>> > > +EXPORT_SYMBOL(clks_register);
>> >
>> > The patch generally has nice documentation, but these two major
>> > interface functions were omitted.
>>
>> Oops...
>
> Where's the interface to register a single clock btw?
>
> I have to say I really hate this "struct clk_table" thing. Another
> reason why struct clk should be in a header file, not a .c file.

See above. The struct clk incorporates the kobject, so it should
never ever be declared.

>
>> >
>> > > +
>> > > +struct clk *clk_get(struct device *dev, const char *name)
>> > > +{
>> > > + struct clk *clk = ERR_PTR(-ENOENT);
>> > > + struct clk *p;
>> > > + unsigned long flags;
>> > > + struct kobject *k;
>> > > +
>> > > + /* Take both clocks_lock and kset lock */
>> > > + spin_lock_irqsave(&clocks_lock, flags);
>> > > + spin_lock(&clks_kset->list_lock);
>> > > +
>> > > + list_for_each_entry(k, &clks_kset->list, entry) {
>> > > +         if (kobject_name(k) && !strcmp(kobject_name(k), name)
>> > > +                         ) {
>> > > +                 p = container_of(kobject_get(k), struct clk, kobj);
>> > > +                 if (p->ops && p->ops->can_get &&
>> > > +                                 p->ops->can_get(p, p->data, dev)) {
>> > > +                         clk = p;
>> > > +                         break;
>> > > +                 }
>> > > +                 kobject_put(k);
>> > > +         }
>> > > + }
>> > > +
>> > > + list_for_each_entry(k, &clks_kset->list, entry) {
>> > > +         if (kobject_name(k) && !strcmp(kobject_name(k), name)
>> > > +                         ) {
>> > > +                 p = container_of(kobject_get(k), struct clk, kobj);
>> > > +                 if (!p->ops || !p->ops->can_get) {
>> > > +                         clk = p;
>> > > +                         break;
>> > > +                 }
>> > > +                 kobject_put(k);
>> > > +                 break;
>> > > +         }
>> > > + }
>
> Why do you have to iterate over clks_kset->list twice?

There are two types of clocks. Ones that are bound to devices
and ones that aren't. E.g. on my development board there is
a MMCCLK which is driven by the chip and is used by my driver
and there is a MMCCLK which is driven by PXA and used by
pxa2xx-mci driver. That's why first you try to find the clock from
"bound" ones (ones wich provide can_get) and then from
all other.

>> > > + spin_lock_irqsave(&clocks_lock, flags);
>> > > +
>> > > + if (!clk->ops || !clk->ops->get_rate)
>> > > +         goto out;
>> >
>> > Did we really need to hold the lock to perform this check?
>> >
>> > Is it really valid for a clock to have no ->ops and no -ops->get_rate?
>> > Please consider mandating that these fields must be provided, then
>> > remove this check.
>>
>> With this patch clocks can be registered and unregistered at any time.
>> When the clock in unregistered I drop the reference to the clk_ops (so
>> the module can be unregistered, but if the clock is still referenced,
>> those functions still can be called. That's why I have to check for the
>> presense of ->ops. On the other hand ->ops->get_rate can be made mandatory.
>
> So...how does the user know that the clock disappeared? Is it really a
> good idea to allow clocks to be unregistered while someone is using it?

What should we do if a user explicitly unbinds the driver that has registered
a clock? We can't fail that operation and say "keep the device bound".
So the only way is to let him use the reference, but return some error
for each call.

>
>> > > +static unsigned long clk_get_rate_parent(struct clk *clk, void *data)
>> > > +{
>> > > + struct clk *parent = __clk_get_parent(clk);
>> > > + unsigned long rate = 0;
>> > > +
>> > > + BUG_ON(!parent);
>> > > +
>> > > + if (!parent->ops || !parent->ops->get_rate)
>> > > +         WARN_ON(1);
>> > > + else
>> > > +         rate = parent->ops->get_rate(parent, parent->data);
>> > > +
>> > > + clk_put(parent);
>> > > +
>> > > + return rate;
>> > > +}
>> >
>> > Can we avoid the (mysterious) void* here?  Do something which will
>> > allow the C type system to check our stuff?
>>
>> I don't know how we can avoid it. Each clock can have "private" data
>> bound to it. Previously the proposed way was to embed struct clk. However
>> since struct clk is dynamic now, it's not directly possible. And all
>> solutions I can think upon are more or less hacky.
>
> Can't you just add some sort of additional_size parameter to the alloc
> function to make it possible for clock drivers to extend it?

... and fill the "extension" with private information. Wait! That information
is already present in some form in the kernel. So if we won't use a pointer
to it, we'll duplicate that info and in fact increase the memory cost
of the clock.
That seems like the real drawback.

> Removing the kobject would of course solve this too. I forgot why we
> wanted a kobject in there in the first place (I think I actually said
> that I _didn't_ want it.)

Yes, I remeber that mail. But things are so easy with kobjects...

-- 
With best wishes
Dmitry

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

[Haavard Skinnemoen]

Dmitry <dbaryshkov@gmail.com> wrote:
> Hi,
> 
> Haavard, few words before detailed comments. In this version of the patchset
> I tried to solve the problem of dynamic unregistration of clocks.
> Consider this example:
> 1) Module A register CLKA
> 2) Module B clk_gets CLKA
> 3) Module A is unloaded
> 3.1) As it's unloaded module A unregisters CLKA
> 4) Module B tries to clk_get_rate on CLKA

clk_get_rate() returns unsigned long, so we're screwed. I think we need
to prevent unloading module A until module B clk_puts CLKA.

> I can think of several solutions for this problem, but IMO the most clear one is
> to force struct clk to be fully dynamic and to use kobjects for it's management.
> This helped me:
> 1) to cleanup the above case

You forgot to audit all drivers to make sure they handle bogus return
values from the clk functions appropriately.

> 2) to cleanup that "clk_alloc_function" mess (required for clock aliasing, etc)

I have to say I'm glad the "clk function" stuff is gone as I never
really understood it...

> 3) to get sysfs integration for free

I wouldn't call making struct clk twice as big as it used to be "free".

> Also, please, bear in mind, that I titled this version of the patchset as "RFC".
> I just wanted to know your opinion on this approach. If it's less acceptable,
> I'll get back to my previous patchset.

Yes, I think it's less acceptable than the previous patchset. But if
other people like this patchset better, I can always stay with my own
implementation of the clk api on avr32.

> 2008/5/28 Haavard Skinnemoen <haavard.skinnemoen@atmel.com>:
> > Dmitry Baryshkov <dbaryshkov@gmail.com> wrote:
> >> Hi,
> >>
> >> On Tue, May 27, 2008 at 03:09:19PM -0700, Andrew Morton wrote:
> >> > On Fri, 23 May 2008 01:21:42 +0400
> >> > 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.
> >> > >
> >> >
> >> > Nobody interested in this?
> >
> > Sorry. I am...or at least I used to be until struct clk went all porky
> > and got hidden away in a .c file...
> 
> It get hidden because in this patchset it contains only private fields
> not intended for public usage.

"private" fields for a mid-layer that doesn't actually do anything
translates into useless fields in my book.

> >> > The documentation should perhaps explain that set_mode() is called
> >> > under spin_lock_irqsave() and hence cannot do much.
> >>
> >> All functions from clk_ops are run with spinlock held.
> >
> > Why is that necessary?
> >
> > I've been thinking a bit lately about starting up oscillators through
> > the clk framework, and that may take up to several seconds. Doing that
> > under an irqsafe spinlock would be madness...
> 
> Consider two kernel threads trying to enable single clock...
> I'm sure locking conditions can be relaxed, but I can't come up with
> the solution yet. Maybe you can?

No, I really wish I could, but I can't think of any way to remove the
lock. A mutex is not an option I guess since these things might get
called from interrupt handlers and whatnot.

> >
> >> > > +EXPORT_SYMBOL(clks_register);
> >> >
> >> > The patch generally has nice documentation, but these two major
> >> > interface functions were omitted.
> >>
> >> Oops...
> >
> > Where's the interface to register a single clock btw?
> >
> > I have to say I really hate this "struct clk_table" thing. Another
> > reason why struct clk should be in a header file, not a .c file.
> 
> See above. The struct clk incorporates the kobject, so it should
> never ever be declared.

I don't understand that reasoning. Why can't you declare a struct that
contains a kobject? I've allocated platform_devices statically lots of
times -- those have embedded kobjects too.

Why not split out the registration bits of clk_alloc() into a separate
clk_register() or clk_add() call?

> > Why do you have to iterate over clks_kset->list twice?
> 
> There are two types of clocks. Ones that are bound to devices
> and ones that aren't. E.g. on my development board there is
> a MMCCLK which is driven by the chip and is used by my driver
> and there is a MMCCLK which is driven by PXA and used by
> pxa2xx-mci driver. That's why first you try to find the clock from
> "bound" ones (ones wich provide can_get) and then from
> all other.

Oh.

How about making can_get() mandatory and provide a default
clk_can_always_get() function for clocks that aren't bound to devices?

> >> > > + spin_lock_irqsave(&clocks_lock, flags);
> >> > > +
> >> > > + if (!clk->ops || !clk->ops->get_rate)
> >> > > +         goto out;
> >> >
> >> > Did we really need to hold the lock to perform this check?
> >> >
> >> > Is it really valid for a clock to have no ->ops and no -ops->get_rate?
> >> > Please consider mandating that these fields must be provided, then
> >> > remove this check.
> >>
> >> With this patch clocks can be registered and unregistered at any time.
> >> When the clock in unregistered I drop the reference to the clk_ops (so
> >> the module can be unregistered, but if the clock is still referenced,
> >> those functions still can be called. That's why I have to check for the
> >> presense of ->ops. On the other hand ->ops->get_rate can be made mandatory.
> >
> > So...how does the user know that the clock disappeared? Is it really a
> > good idea to allow clocks to be unregistered while someone is using it?
> 
> What should we do if a user explicitly unbinds the driver that has registered
> a clock? We can't fail that operation and say "keep the device bound".
> So the only way is to let him use the reference, but return some error
> for each call.

Why can't we fail that operation? We certainly can't return an error
for all calls with the existing API.

Isn't it quite common to prevent a module from being unloaded if
another module is using it? try_module_get() in clk_get() and
module_put() in clk_put() should do the trick, no?

Or you can stall the unregistration until all users are gone. The DMA
engine layer does something like that.

> >> > > +static unsigned long clk_get_rate_parent(struct clk *clk, void *data)
> >> > > +{
> >> > > + struct clk *parent = __clk_get_parent(clk);
> >> > > + unsigned long rate = 0;
> >> > > +
> >> > > + BUG_ON(!parent);
> >> > > +
> >> > > + if (!parent->ops || !parent->ops->get_rate)
> >> > > +         WARN_ON(1);
> >> > > + else
> >> > > +         rate = parent->ops->get_rate(parent, parent->data);
> >> > > +
> >> > > + clk_put(parent);
> >> > > +
> >> > > + return rate;
> >> > > +}
> >> >
> >> > Can we avoid the (mysterious) void* here?  Do something which will
> >> > allow the C type system to check our stuff?
> >>
> >> I don't know how we can avoid it. Each clock can have "private" data
> >> bound to it. Previously the proposed way was to embed struct clk. However
> >> since struct clk is dynamic now, it's not directly possible. And all
> >> solutions I can think upon are more or less hacky.
> >
> > Can't you just add some sort of additional_size parameter to the alloc
> > function to make it possible for clock drivers to extend it?
> 
> ... and fill the "extension" with private information. Wait! That information
> is already present in some form in the kernel. So if we won't use a pointer
> to it, we'll duplicate that info and in fact increase the memory cost
> of the clock.
> That seems like the real drawback.

Huh? Isn't that a very strong argument against keeping struct clk
hidden? That's certainly going to cause duplication since you can't
share any data at all between the mid-layer and the hardware driver...

I guess what I'm not understanding is why this struct clk with an
embedded kobject in it is so special that you can't allocate it in a
normal way (even though clk_alloc() does a quite normal kmalloc() in
order to allocate it...)

> > Removing the kobject would of course solve this too. I forgot why we
> > wanted a kobject in there in the first place (I think I actually said
> > that I _didn't_ want it.)
> 
> Yes, I remeber that mail. But things are so easy with kobjects...

Easy?

You have to use different data structures for registration and normal
use, all users are burdened with the task of checking whether the clock
is actually valid every time they try to use it, sharing of data
between the mid-layer and the hardware driver is prevented, and all
driver hooks got an extra parameter.

Haavard

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

[Dmitry Baryshkov]

Hi,

On Wed, May 28, 2008 at 03:56:45PM +0200, Haavard Skinnemoen wrote:
> Dmitry <dbaryshkov@gmail.com> wrote:
> > Hi,
> > 
> > Haavard, few words before detailed comments. In this version of the patchset
> > I tried to solve the problem of dynamic unregistration of clocks.
> > Consider this example:
> > 1) Module A register CLKA
> > 2) Module B clk_gets CLKA
> > 3) Module A is unloaded
> > 3.1) As it's unloaded module A unregisters CLKA
> > 4) Module B tries to clk_get_rate on CLKA
> 
> clk_get_rate() returns unsigned long, so we're screwed. I think we need
> to prevent unloading module A until module B clk_puts CLKA.

That can be changed to plain long.

> 
> > I can think of several solutions for this problem, but IMO the most clear one is
> > to force struct clk to be fully dynamic and to use kobjects for it's management.
> > This helped me:
> > 1) to cleanup the above case
> 
> You forgot to audit all drivers to make sure they handle bogus return
> values from the clk functions appropriately.

I don't think it's a primary task...

> 
> > 2) to cleanup that "clk_alloc_function" mess (required for clock aliasing, etc)
> 
> I have to say I'm glad the "clk function" stuff is gone as I never
> really understood it...

:)

> 
> > 3) to get sysfs integration for free
> 
> I wouldn't call making struct clk twice as big as it used to be "free".
> 
> > Also, please, bear in mind, that I titled this version of the patchset as "RFC".
> > I just wanted to know your opinion on this approach. If it's less acceptable,
> > I'll get back to my previous patchset.
> 
> Yes, I think it's less acceptable than the previous patchset. But if
> other people like this patchset better, I can always stay with my own
> implementation of the clk api on avr32.

No, that's not my goal.

> > 2008/5/28 Haavard Skinnemoen <haavard.skinnemoen@atmel.com>:
> > > Dmitry Baryshkov <dbaryshkov@gmail.com> wrote:
> > >> Hi,
> > >>
> > >> On Tue, May 27, 2008 at 03:09:19PM -0700, Andrew Morton wrote:
> > >> > On Fri, 23 May 2008 01:21:42 +0400
> > >> > 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.
> > >> > >
> > >> >
> > >> > Nobody interested in this?
> > >
> > > Sorry. I am...or at least I used to be until struct clk went all porky
> > > and got hidden away in a .c file...
> > 
> > It get hidden because in this patchset it contains only private fields
> > not intended for public usage.
> 
> "private" fields for a mid-layer that doesn't actually do anything
> translates into useless fields in my book.

Sorry, I don't understand you. Here is the definition from the C file:
struct clk {
	struct kobject kobj;
	int usage;
	const struct clk_ops *ops;
	void *data;
};

Which fields are useless from your POV? The mid-layer manages clocks
registration, so kobj. It manages usage (enablement) counters.
It serves per-clock operations. What do you dislike here?

> > >> > The documentation should perhaps explain that set_mode() is called
> > >> > under spin_lock_irqsave() and hence cannot do much.
> > >>
> > >> All functions from clk_ops are run with spinlock held.
> > >
> > > Why is that necessary?
> > >
> > > I've been thinking a bit lately about starting up oscillators through
> > > the clk framework, and that may take up to several seconds. Doing that
> > > under an irqsafe spinlock would be madness...
> > 
> > Consider two kernel threads trying to enable single clock...
> > I'm sure locking conditions can be relaxed, but I can't come up with
> > the solution yet. Maybe you can?
> 
> No, I really wish I could, but I can't think of any way to remove the
> lock. A mutex is not an option I guess since these things might get
> called from interrupt handlers and whatnot.
> 
> > >
> > >> > > +EXPORT_SYMBOL(clks_register);
> > >> >
> > >> > The patch generally has nice documentation, but these two major
> > >> > interface functions were omitted.
> > >>
> > >> Oops...
> > >
> > > Where's the interface to register a single clock btw?
> > >
> > > I have to say I really hate this "struct clk_table" thing. Another
> > > reason why struct clk should be in a header file, not a .c file.
> > 
> > See above. The struct clk incorporates the kobject, so it should
> > never ever be declared.
> 
> I don't understand that reasoning. Why can't you declare a struct that
> contains a kobject? I've allocated platform_devices statically lots of
> times -- those have embedded kobjects too.

Citing Documentation/kobject.txt:
Because kobjects are dynamic, they must not be declared statically or on
the stack, but instead, always allocated dynamically.  Future versions of
the kernel will contain a run-time check for kobjects that are created
statically and will warn the developer of this improper usage.

However as static devices do work, we can probably go that way.

> 
> Why not split out the registration bits of clk_alloc() into a separate
> clk_register() or clk_add() call?
> 
> > > Why do you have to iterate over clks_kset->list twice?
> > 
> > There are two types of clocks. Ones that are bound to devices
> > and ones that aren't. E.g. on my development board there is
> > a MMCCLK which is driven by the chip and is used by my driver
> > and there is a MMCCLK which is driven by PXA and used by
> > pxa2xx-mci driver. That's why first you try to find the clock from
> > "bound" ones (ones wich provide can_get) and then from
> > all other.
> 
> Oh.
> 
> How about making can_get() mandatory and provide a default
> clk_can_always_get() function for clocks that aren't bound to devices?

ok.

> > >> > > + spin_lock_irqsave(&clocks_lock, flags);
> > >> > > +
> > >> > > + if (!clk->ops || !clk->ops->get_rate)
> > >> > > +         goto out;
> > >> >
> > >> > Did we really need to hold the lock to perform this check?
> > >> >
> > >> > Is it really valid for a clock to have no ->ops and no -ops->get_rate?
> > >> > Please consider mandating that these fields must be provided, then
> > >> > remove this check.
> > >>
> > >> With this patch clocks can be registered and unregistered at any time.
> > >> When the clock in unregistered I drop the reference to the clk_ops (so
> > >> the module can be unregistered, but if the clock is still referenced,
> > >> those functions still can be called. That's why I have to check for the
> > >> presense of ->ops. On the other hand ->ops->get_rate can be made mandatory.
> > >
> > > So...how does the user know that the clock disappeared? Is it really a
> > > good idea to allow clocks to be unregistered while someone is using it?
> > 
> > What should we do if a user explicitly unbinds the driver that has registered
> > a clock? We can't fail that operation and say "keep the device bound".
> > So the only way is to let him use the reference, but return some error
> > for each call.
> 
> Why can't we fail that operation? We certainly can't return an error
> for all calls with the existing API.
> 
> Isn't it quite common to prevent a module from being unloaded if
> another module is using it? try_module_get() in clk_get() and
> module_put() in clk_put() should do the trick, no?
> 
> Or you can stall the unregistration until all users are gone. The DMA
> engine layer does something like that.

try_module_get()/module_put() won't help.
Stalling the unregistration sounds pretty bad also. However maybe we
should just BUG() in such cases :)

> 
> > >> > > +static unsigned long clk_get_rate_parent(struct clk *clk, void *data)
> > >> > > +{
> > >> > > + struct clk *parent = __clk_get_parent(clk);
> > >> > > + unsigned long rate = 0;
> > >> > > +
> > >> > > + BUG_ON(!parent);
> > >> > > +
> > >> > > + if (!parent->ops || !parent->ops->get_rate)
> > >> > > +         WARN_ON(1);
> > >> > > + else
> > >> > > +         rate = parent->ops->get_rate(parent, parent->data);
> > >> > > +
> > >> > > + clk_put(parent);
> > >> > > +
> > >> > > + return rate;
> > >> > > +}
> > >> >
> > >> > Can we avoid the (mysterious) void* here?  Do something which will
> > >> > allow the C type system to check our stuff?
> > >>
> > >> I don't know how we can avoid it. Each clock can have "private" data
> > >> bound to it. Previously the proposed way was to embed struct clk. However
> > >> since struct clk is dynamic now, it's not directly possible. And all
> > >> solutions I can think upon are more or less hacky.
> > >
> > > Can't you just add some sort of additional_size parameter to the alloc
> > > function to make it possible for clock drivers to extend it?
> > 
> > ... and fill the "extension" with private information. Wait! That information
> > is already present in some form in the kernel. So if we won't use a pointer
> > to it, we'll duplicate that info and in fact increase the memory cost
> > of the clock.
> > That seems like the real drawback.
> 
> Huh? Isn't that a very strong argument against keeping struct clk
> hidden? That's certainly going to cause duplication since you can't
> share any data at all between the mid-layer and the hardware driver...
> 
> I guess what I'm not understanding is why this struct clk with an
> embedded kobject in it is so special that you can't allocate it in a
> normal way (even though clk_alloc() does a quite normal kmalloc() in
> order to allocate it...)

OK.

> 
> > > Removing the kobject would of course solve this too. I forgot why we
> > > wanted a kobject in there in the first place (I think I actually said
> > > that I _didn't_ want it.)
> > 
> > Yes, I remeber that mail. But things are so easy with kobjects...
> 
> Easy?
> 
> You have to use different data structures for registration and normal
> use, all users are burdened with the task of checking whether the clock
> is actually valid every time they try to use it, sharing of data
> between the mid-layer and the hardware driver is prevented, and all
> driver hooks got an extra parameter.

Haavard, if I return struct clk definition to header, permit clocks to
be allocated statically, drop again that "priv" in favour of embedding,
would you agree on kobject-based implementation? I'd really like to use
them. Because otherwise we are nearly reimplementing them from scratch.

-- 
With best wishes
Dmitry

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

[Haavard Skinnemoen]

On Wed, 28 May 2008 19:34:34 +0400
Dmitry Baryshkov <dbaryshkov@gmail.com> wrote:
> On Wed, May 28, 2008 at 03:56:45PM +0200, Haavard Skinnemoen wrote:
> > clk_get_rate() returns unsigned long, so we're screwed. I think we need
> > to prevent unloading module A until module B clk_puts CLKA.
> 
> That can be changed to plain long.

Yeah, but current users aren't aware that it can fail at all.

> > You forgot to audit all drivers to make sure they handle bogus return
> > values from the clk functions appropriately.
> 
> I don't think it's a primary task...

If we change the semantics of the clk API, someone has to do it...

> > "private" fields for a mid-layer that doesn't actually do anything
> > translates into useless fields in my book.
> 
> Sorry, I don't understand you. Here is the definition from the C file:
> struct clk {
> 	struct kobject kobj;
> 	int usage;
> 	const struct clk_ops *ops;
> 	void *data;
> };
> 
> Which fields are useless from your POV? The mid-layer manages clocks
> registration, so kobj. It manages usage (enablement) counters.
> It serves per-clock operations. What do you dislike here?

Most of the 36 bytes that the kobject take up are useless. The fields
that _are_ useful are often useful to the hardware driver too, e.g.
"parent" and possibly "usage".

> > I don't understand that reasoning. Why can't you declare a struct that
> > contains a kobject? I've allocated platform_devices statically lots of
> > times -- those have embedded kobjects too.
> 
> Citing Documentation/kobject.txt:
> Because kobjects are dynamic, they must not be declared statically or on
> the stack, but instead, always allocated dynamically.  Future versions of
> the kernel will contain a run-time check for kobjects that are created
> statically and will warn the developer of this improper usage.
> 
> However as static devices do work, we can probably go that way.

Right...I forgot about that restriction. Maybe static platform_devices
work mostly by chance.

> > Isn't it quite common to prevent a module from being unloaded if
> > another module is using it? try_module_get() in clk_get() and
> > module_put() in clk_put() should do the trick, no?
> > 
> > Or you can stall the unregistration until all users are gone. The DMA
> > engine layer does something like that.
> 
> try_module_get()/module_put() won't help.

Why not? Because a clock provider may decide to remove a clock
unrelated to module removal?

> Stalling the unregistration sounds pretty bad also. However maybe we
> should just BUG() in such cases :)

I don't think BUG()ing is appropriate since the module providing the
clock has no way of knowing whether it's ok to unregister the clock.

Maybe we can do the try_module_get()/module_put() thing and BUG() if
the module tries to remove a clock that is in use?

> Haavard, if I return struct clk definition to header, permit clocks to
> be allocated statically, drop again that "priv" in favour of embedding,
> would you agree on kobject-based implementation? I'd really like to use
> them. Because otherwise we are nearly reimplementing them from scratch.

Well...I still think the struct clk is too big. And I'm also not too
happy about changing the semantics of the API so that users have to be
prepared to handle that a clock they hold a reference to may go away at
any time.

But if we really do end up having to deal with "clock hotplugging",
maybe a kref is what we really need, not a whole kobject?

Haavard

[RFC][PATCH 2/2] Clocklib: Refactor PXA arm arch to use clocklib.

[Dmitry Baryshkov]

Make PXA use clocklib for clocks support.

Signed-off-by: Dmitry Baryshkov <dbaryshkov@gmail.com>
---
 arch/arm/Kconfig           |    1 +
 arch/arm/mach-pxa/clock.c  |  132 ++++++++------------------------------
 arch/arm/mach-pxa/clock.h  |   48 +++++++-------
 arch/arm/mach-pxa/pxa25x.c |   60 ++++++++++--------
 arch/arm/mach-pxa/pxa27x.c |   81 +++++++++++++-----------
 arch/arm/mach-pxa/pxa3xx.c |  153 +++++++++++++++++++++++++-------------------
 6 files changed, 214 insertions(+), 261 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index b786e68..04c3eef 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -397,6 +397,7 @@ config ARCH_PXA
 	select GENERIC_TIME
 	select GENERIC_CLOCKEVENTS
 	select TICK_ONESHOT
+	select CLOCKLIB
 	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..b4dbce4 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/clocklib.h>
 #include <linux/spinlock.h>
 #include <linux/platform_device.h>
 #include <linux/delay.h>
@@ -20,135 +21,56 @@
 #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)
-{
-}
-EXPORT_SYMBOL(clk_put);
-
-int clk_enable(struct clk *clk)
+static int clk_gpio11_set_mode(struct clk *clk, void *data, bool enable)
 {
-	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);
+	if (enable)
+		pxa_gpio_mode(GPIO11_3_6MHz_MD);
 
 	return 0;
 }
-EXPORT_SYMBOL(clk_enable);
 
-void clk_disable(struct clk *clk)
+static unsigned long clk_gpio11_get_rate(struct clk *clk, void *data)
 {
-	unsigned long flags;
+	return 3686400;
+}
 
-	WARN_ON(clk->enabled == 0);
+static const struct clk_ops clk_gpio11_ops = {
+	.set_mode =	clk_gpio11_set_mode,
+	.get_rate =	clk_gpio11_get_rate,
+};
 
-	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)
+int clk_cken_set_mode(struct clk *clk, void *data, bool enable)
 {
-	unsigned long rate;
-
-	rate = clk->rate;
-	if (clk->ops->getrate)
-		rate = clk->ops->getrate(clk);
+	struct clk_cken_priv *priv = data;
 
-	return rate;
-}
-EXPORT_SYMBOL(clk_get_rate);
+	if (enable)
+		CKEN |= 1 << priv->cken;
+	else
+		CKEN &= ~(1 << priv->cken);
 
+	if (priv->delay)
+		udelay(priv->delay);
 
-static void clk_gpio27_enable(struct clk *clk)
-{
-	pxa_gpio_mode(GPIO11_3_6MHz_MD);
+	return 0;
 }
 
-static void clk_gpio27_disable(struct clk *clk)
+unsigned long clk_cken_get_rate(struct clk *clk, void *data)
 {
-}
-
-static const struct clkops clk_gpio27_ops = {
-	.enable		= clk_gpio27_enable,
-	.disable	= clk_gpio27_disable,
-};
-
+	struct clk_cken_priv *priv = data;
 
-void clk_cken_enable(struct clk *clk)
-{
-	CKEN |= 1 << clk->cken;
-}
+	return priv->rate;
 
-void clk_cken_disable(struct clk *clk)
-{
-	CKEN &= ~(1 << clk->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,
-	},
+const struct clk_ops clk_cken_ops = {
+	.set_mode	= clk_cken_set_mode,
+	.get_rate	= clk_cken_get_rate,
 };
 
-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));
+	clk_alloc(NULL, &clk_gpio11_ops, NULL, "GPIO27_CLK");
 	return 0;
 }
 arch_initcall(clk_init);
diff --git a/arch/arm/mach-pxa/clock.h b/arch/arm/mach-pxa/clock.h
index bc6b77e..407a80f 100644
--- a/arch/arm/mach-pxa/clock.h
+++ b/arch/arm/mach-pxa/clock.h
@@ -1,43 +1,41 @@
-struct clk;
+#include <linux/clk.h>
+#include <linux/clocklib.h>
 
-struct clkops {
-	void			(*enable)(struct clk *);
-	void			(*disable)(struct clk *);
-	unsigned long		(*getrate)(struct clk *);
-};
-
-struct clk {
-	struct list_head	node;
-	const char		*name;
-	struct device		*dev;
-	const struct clkops	*ops;
+struct clk_cken_priv {
 	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,		\
-		.rate	= _rate,			\
-		.cken	= CKEN_##_cken,			\
-		.delay	= _delay,			\
+		.data	= &(struct clk_cken_priv) {	\
+			.cken	= CKEN_##_cken,		\
+			.rate	= _rate,		\
+			.delay	= _delay,		\
+		},					\
 	}
 
-#define INIT_CK(_name, _cken, _ops, _dev)		\
+#define INIT_CK(_name, _cken, _ops)			\
 	{						\
 		.name	= _name,			\
-		.dev	= _dev,				\
 		.ops	= _ops,				\
-		.cken	= CKEN_##_cken,			\
+		.data	= &(struct clk_cken_priv) {	\
+			.cken	= CKEN_##_cken,		\
+		},					\
 	}
 
-extern const struct clkops clk_cken_ops;
+#define INIT_SUBCK(_parent_name, _name, _dev)		\
+	{						\
+		.parent_name	= _parent_name,		\
+		.name		= _name,		\
+		.ops		= &clk_dev_ops,		\
+		.data		= _dev,			\
+	}
 
-void clk_cken_enable(struct clk *clk);
-void clk_cken_disable(struct clk *clk);
+extern const struct clk_ops clk_cken_ops;
 
-void clks_register(struct clk *clks, size_t num);
+int clk_cken_set_mode(struct clk *clk, void *data, bool enable);
+unsigned long clk_cken_get_rate(struct clk *clk, void *data);
diff --git a/arch/arm/mach-pxa/pxa25x.c b/arch/arm/mach-pxa/pxa25x.c
index e5b417d..3410ce7 100644
--- a/arch/arm/mach-pxa/pxa25x.c
+++ b/arch/arm/mach-pxa/pxa25x.c
@@ -97,15 +97,14 @@ unsigned int pxa25x_get_memclk_frequency_10khz(void)
 	return L_clk_mult[(CCCR >> 0) & 0x1f] * BASE_CLK / 10000;
 }
 
-static unsigned long clk_pxa25x_lcd_getrate(struct clk *clk)
+static unsigned long clk_pxa25x_lcd_getrate(struct clk *clk, void *data)
 {
 	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,38 @@ 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),
+static struct clk_table pxa25x_hwuart_clk[] = {
+	INIT_CKEN("HWUARTCLK", HWUART, 14745600, 1),
+	INIT_SUBCK("HWUARTCLK", "UARTCLK", &pxa_device_hwuart.dev),
+};
 
-	INIT_CKEN("AC97CLK",     AC97,     24576000, 0, NULL),
+static struct clk_table pxa25x_clks[] = {
+	INIT_CK("LCDCLK", LCD, &clk_pxa25x_lcd_ops), /* &pxa_device_fb.dev */
+	INIT_CKEN("FFUARTCLK", FFUART, 14745600, 1),
+	INIT_SUBCK("FFUARTCLK", "UARTCLK", &pxa_device_ffuart.dev),
+	INIT_CKEN("BTUARTCLK", BTUART, 14745600, 1),
+	INIT_SUBCK("BTUARTCLK", "UARTCLK", &pxa_device_btuart.dev),
+	INIT_CKEN("STUARTCLK", STUART, 14745600, 1),
+	INIT_SUBCK("STUARTCLK", "UARTCLK", &pxa_device_stuart.dev),
+	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("SSP_CLK",  SSP, 3686400, 0),
+	INIT_SUBCK("SSP_CLK", "SSPCLK", &pxa25x_device_ssp.dev),
+	INIT_CKEN("NSSPCLK", NSSP, 3686400, 0),
+	INIT_SUBCK("NSSPCLK", "SSPCLK", &pxa25x_device_nssp.dev),
+	INIT_CKEN("ASSPCLK", ASSP, 3686400, 0),
+	INIT_SUBCK("ASSPCLK", "SSPCLK", &pxa25x_device_assp.dev),
+
+	INIT_CKEN("AC97CLK",     AC97,     24576000, 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),
 };
 
 #ifdef CONFIG_PM
@@ -285,7 +291,7 @@ static int __init pxa25x_init(void)
 
 	/* Only add HWUART for PXA255/26x; PXA210/250/27x do not have it. */
 	if (cpu_is_pxa25x())
-		clks_register(&pxa25x_hwuart_clk, 1);
+		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));
diff --git a/arch/arm/mach-pxa/pxa27x.c b/arch/arm/mach-pxa/pxa27x.c
index 7e94583..ec13349 100644
--- a/arch/arm/mach-pxa/pxa27x.c
+++ b/arch/arm/mach-pxa/pxa27x.c
@@ -45,7 +45,7 @@ unsigned int pxa27x_get_clk_frequency_khz(int info)
 {
 	unsigned long ccsr, clkcfg;
 	unsigned int l, L, m, M, n2, N, S;
-       	int cccr_a, t, ht, b;
+	int cccr_a, t, ht, b;
 
 	ccsr = CCSR;
 	cccr_a = CCCR & (1 << 25);
@@ -88,7 +88,7 @@ unsigned int pxa27x_get_memclk_frequency_10khz(void)
 {
 	unsigned long ccsr, clkcfg;
 	unsigned int l, L, m, M;
-       	int cccr_a, b;
+	int cccr_a, b;
 
 	ccsr = CCSR;
 	cccr_a = CCCR & (1 << 25);
@@ -125,49 +125,56 @@ static unsigned int pxa27x_get_lcdclk_frequency_10khz(void)
 	return (K / 10000);
 }
 
-static unsigned long clk_pxa27x_lcd_getrate(struct clk *clk)
+static unsigned long clk_pxa27x_lcd_getrate(struct clk *clk, void *data)
 {
 	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),
-
-	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("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("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("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("AC97CLK",     AC97,     24576000, 0, NULL),
-	INIT_CKEN("AC97CONFCLK", AC97CONF, 24576000, 0, NULL),
+static struct clk_table pxa27x_clks[] = {
+	INIT_CK("LCDCLK", LCD,    &clk_pxa27x_lcd_ops), /* &pxa_device_fb.dev */
+	INIT_CK("CAMCLK", CAMERA, &clk_pxa27x_lcd_ops),
+
+	INIT_CKEN("FFUARTCLK", FFUART, 14857000, 1),
+	INIT_SUBCK("FFUARTCLK", "UARTCLK", &pxa_device_ffuart.dev),
+	INIT_CKEN("BTUARTCLK", BTUART, 14857000, 1),
+	INIT_SUBCK("BTUARTCLK", "UARTCLK", &pxa_device_btuart.dev),
+	INIT_CKEN("STUARTCLK", STUART, 14857000, 1),
+	INIT_SUBCK("STUARTCLK", "UARTCLK", &pxa_device_stuart.dev),
+
+	INIT_CKEN("I2SCLK",  I2S,  14682000, 0), /* &pxa_device_i2s.dev */
+	INIT_CKEN("I2C_CLK",  I2C,  32842000, 0),
+	INIT_SUBCK("I2C_CLK", "I2CCLK", &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("USBCLK", USBHOST, 48000000, 0), /* &pxa27x_device_ohci.dev */
+	INIT_CKEN("PWRI2CCLK", PWRI2C, 13000000, 0),
+	INIT_SUBCK("PWRI2CCLK", "I2CCLK", &pxa27x_device_i2c_power.dev),
+	INIT_CKEN("KBDCLK", KEYPAD, 32768, 0), /* &pxa27x_device_keypad.dev */
+
+	INIT_CKEN("SSP1CLK", SSP1, 13000000, 0),
+	INIT_SUBCK("SSP1CLK", "SSPCLK", &pxa27x_device_ssp1.dev),
+	INIT_CKEN("SSP2CLK", SSP2, 13000000, 0),
+	INIT_SUBCK("SSP2CLK", "SSPCLK", &pxa27x_device_ssp2.dev),
+	INIT_CKEN("SSP3CLK", SSP3, 13000000, 0),
+	INIT_SUBCK("SSP3CLK", "SSPCLK", &pxa27x_device_ssp3.dev),
+
+	INIT_CKEN("AC97CLK",     AC97,     24576000, 0),
+	INIT_CKEN("AC97CONFCLK", AC97CONF, 24576000, 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),
 	*/
 };
 
diff --git a/arch/arm/mach-pxa/pxa3xx.c b/arch/arm/mach-pxa/pxa3xx.c
index 644550b..e394a1a 100644
--- a/arch/arm/mach-pxa/pxa3xx.c
+++ b/arch/arm/mach-pxa/pxa3xx.c
@@ -21,6 +21,7 @@
 #include <linux/irq.h>
 #include <linux/io.h>
 #include <linux/sysdev.h>
+#include <linux/delay.h>
 
 #include <asm/hardware.h>
 #include <asm/arch/pxa3xx-regs.h>
@@ -112,7 +113,7 @@ unsigned int pxa3xx_get_memclk_frequency_10khz(void)
 /*
  * Return the current AC97 clock frequency.
  */
-static unsigned long clk_pxa3xx_ac97_getrate(struct clk *clk)
+static unsigned long clk_pxa3xx_ac97_getrate(struct clk *clk, void *data)
 {
 	unsigned long rate = 312000000;
 	unsigned long ac97_div;
@@ -131,7 +132,7 @@ static unsigned long clk_pxa3xx_ac97_getrate(struct clk *clk)
 /*
  * Return the current HSIO bus clock frequency
  */
-static unsigned long clk_pxa3xx_hsio_getrate(struct clk *clk)
+static unsigned long clk_pxa3xx_hsio_getrate(struct clk *clk, void *data)
 {
 	unsigned long acsr;
 	unsigned int hss, hsio_clk;
@@ -144,105 +145,123 @@ 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, void *data, bool enable)
 {
-	unsigned long mask = 1ul << (clk->cken & 0x1f);
-
-	if (clk->cken < 32)
-		CKENA |= mask;
-	else
-		CKENB |= mask;
-}
+	struct clk_cken_priv *priv = data;
+	unsigned long mask = 1ul << (priv->cken & 0x1f);
+
+	if (enable) {
+		if (priv->cken < 32)
+			CKENA |= mask;
+		else
+			CKENB |= mask;
+	} else {
+		if (priv->cken < 32)
+			CKENA &= ~mask;
+		else
+			CKENB &= ~mask;
+	}
 
-static void clk_pxa3xx_cken_disable(struct clk *clk)
-{
-	unsigned long mask = 1ul << (clk->cken & 0x1f);
+	if (priv->delay)
+		udelay(priv->delay);
 
-	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, void *data, bool enable)
 {
-	OSCC |= OSCC_PEN;
+	if (enable)
+		OSCC |= OSCC_PEN;
+	else
+		OSCC &= ~OSCC_PEN;
+
+	udelay(70);
+
+	return 0;
 }
 
-static void clk_pout_disable(struct clk *clk)
+static unsigned long clk_pout_get_rate(struct clk *clk, void *data)
 {
-	OSCC &= ~OSCC_PEN;
+	return 13000000;
 }
 
-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_pout_get_rate,
 };
 
-#define PXA3xx_CKEN(_name, _cken, _rate, _delay, _dev)	\
+#define PXA3xx_CKEN(_name, _cken, _rate, _delay)	\
 	{						\
 		.name	= _name,			\
-		.dev	= _dev,				\
 		.ops	= &clk_pxa3xx_cken_ops,		\
-		.rate	= _rate,			\
-		.cken	= CKEN_##_cken,			\
-		.delay	= _delay,			\
+		.data	= &(struct clk_cken_priv) {	\
+			.cken	= CKEN_##_cken,		\
+			.rate	= _rate,		\
+			.delay	= _delay,		\
+		},					\
 	}
 
-#define PXA3xx_CK(_name, _cken, _ops, _dev)		\
+#define PXA3xx_CK(_name, _cken, _ops)			\
 	{						\
 		.name	= _name,			\
-		.dev	= _dev,				\
 		.ops	= _ops,				\
-		.cken	= CKEN_##_cken,			\
+		.data	= &(struct clk_cken_priv) {	\
+			.cken	= CKEN_##_cken,		\
+		},					\
 	}
 
-static struct clk pxa3xx_clks[] = {
+static struct clk_table pxa3xx_clks[] = {
 	{
 		.name           = "CLK_POUT",
 		.ops            = &clk_pout_ops,
-		.rate           = 13000000,
-		.delay          = 70,
 	},
 
-	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("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),
+	PXA3xx_CK("LCDCLK",  LCD,    &clk_pxa3xx_hsio_ops), /* &pxa_device_fb.dev */
+	PXA3xx_CK("CAMCLK",  CAMERA, &clk_pxa3xx_hsio_ops),
+	PXA3xx_CK("AC97CLK", AC97,   &clk_pxa3xx_ac97_ops),
+
+	PXA3xx_CKEN("FFUARTCLK", FFUART, 14857000, 1),
+	INIT_SUBCK("FFUARTCLK", "UARTCLK", &pxa_device_ffuart.dev),
+	PXA3xx_CKEN("BTUARTCLK", BTUART, 14857000, 1),
+	INIT_SUBCK("BTUARTCLK", "UARTCLK", &pxa_device_btuart.dev),
+	PXA3xx_CKEN("STUARTCLK", STUART, 14857000, 1),
+	INIT_SUBCK("STUARTCLK", "UARTCLK", &pxa_device_stuart.dev),
+
+	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("SSP1CLK", SSP1, 13000000, 0),
+	INIT_SUBCK("SSP1CLK", "SSPCLK", &pxa27x_device_ssp1.dev),
+	PXA3xx_CKEN("SSP2CLK", SSP2, 13000000, 0), /* &pxa27x_device_ssp2.dev */
+	INIT_SUBCK("SSP2CLK", "SSPCLK", &pxa27x_device_ssp2.dev),
+	PXA3xx_CKEN("SSP3CLK", SSP3, 13000000, 0), /* &pxa27x_device_ssp3.dev */
+	INIT_SUBCK("SSP3CLK", "SSPCLK", &pxa27x_device_ssp3.dev),
+	PXA3xx_CKEN("SSP4CLK", SSP4, 13000000, 0), /* &pxa3xx_device_ssp4.dev */
+	INIT_SUBCK("SSP4CLK", "SSPCLK", &pxa3xx_device_ssp4.dev),
+
+	PXA3xx_CKEN("MMC1CLK", MMC1, 19500000, 0),
+	INIT_SUBCK("MMC2CLK", "MMCCLK", &pxa_device_mci.dev),
+	PXA3xx_CKEN("MMC2CLK", MMC2, 19500000, 0),
+	INIT_SUBCK("MMC2CLK", "MMCCLK", &pxa3xx_device_mci2.dev),
+	PXA3xx_CKEN("MMC3CLK", MMC3, 19500000, 0),
+	INIT_SUBCK("MMC3CLK", "MMCCLK", &pxa3xx_device_mci3.dev),
 };
 
 #ifdef CONFIG_PM
-- 
1.5.5.1


-- 
With best wishes
Dmitry