[RFC,PATCH 2/2] clk: Generic support for fixed-rate clocks

[RFC,PATCH 2/2] clk: Generic support for fixed-rate clocks

[Jeremy Kerr]

Since most platforms will need a fixed-rate clock, add one. This will
also serve as a basic example of an implementation of struct clk.

Signed-off-by: Jeremy Kerr <jeremy.kerr@canonical.com>

---
 include/linux/clk.h |   13 +++++++++++++
 kernel/Makefile     |    1 +
 kernel/clk.c        |   25 +++++++++++++++++++++++++
 3 files changed, 39 insertions(+)

diff --git a/include/linux/clk.h b/include/linux/clk.h
index bb6957a..6269b44 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -123,6 +123,19 @@ static inline struct clk *clk_get_parent(struct clk *clk)
 	return ERR_PTR(-ENOSYS);
 }
 
+/* Simple fixed-rate clock */
+struct clk_fixed {
+	struct clk	clk;
+	unsigned long	rate;
+};
+
+extern struct clk_ops clk_fixed_ops;
+
+#define INIT_CLK_FIXED(name, r) { \
+	.clk = INIT_CLK(name.clk, clk_fixed_ops), \
+	.rate = (r) \
+}
+
 #else /* !CONFIG_USE_COMMON_STRUCT_CLK */
 
 /*
diff --git a/kernel/Makefile b/kernel/Makefile
index 057472f..1ae15aa 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -105,6 +105,7 @@ obj-$(CONFIG_PERF_EVENTS) += perf_event.o
 obj-$(CONFIG_HAVE_HW_BREAKPOINT) += hw_breakpoint.o
 obj-$(CONFIG_USER_RETURN_NOTIFIER) += user-return-notifier.o
 obj-$(CONFIG_PADATA) += padata.o
+obj-$(CONFIG_USE_COMMON_STRUCT_CLK) += clk.o
 
 ifneq ($(CONFIG_SCHED_OMIT_FRAME_POINTER),y)
 # According to Alan Modra <alan@linuxcare.com.au>, the -fno-omit-frame-pointer is
diff --git a/kernel/clk.c b/kernel/clk.c
new file mode 100644
index 0000000..85c0670
--- /dev/null
+++ b/kernel/clk.c
@@ -0,0 +1,25 @@
+/*
+ * Copyright (C) 2010 Canonical Ltd <jeremy.kerr@canonical.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * Standard functionality for the common clock API.
+ */
+
+#include <linux/module.h>
+#include <linux/clk.h>
+
+#define to_clk_fixed(clk) (container_of(clk, struct clk_fixed, clk))
+
+static unsigned long clk_fixed_get_rate(struct clk *clk)
+{
+	return to_clk_fixed(clk)->rate;
+}
+
+struct clk_ops clk_fixed_ops = {
+	.get_rate = clk_fixed_get_rate,
+};
+
+EXPORT_SYMBOL_GPL(clk_fixed_ops);

[RFC,PATCH 0/2] Common struct clk implementation, v4

[Jeremy Kerr]

Hi all,

These patches are an attempt to allow platforms to share clock code. At
present, the definitions of 'struct clk' are local to platform code,
which makes allocating and initialising cross-platform clock sources
difficult, and makes it impossible to compile a single image containing
support for two ARM platforms with different struct clks.

The two patches are for the architecture-independent kernel code,
introducing the common clk infrastructure. The changelog for the first
patch includes details about the new clock definitions.

As requested by rmk, I've put together a small series of patches
illustrating the usage of the common struct clock on the ARM imx51
platform. These are available in my git tree:

 git://kernel.ubuntu.com/jk/dt/linux-2.6

in the clk-common-mx51 branch (clk-common..clk-common-mx51).

The approach I've taken with the imx51 port is to temporarly duplicate
the platform-common clock code (ie, for all mxc-based boards) to enable
usage of the common struct clk on one machine (imx51), while leaving the
others as-is. For a proper platform-wide usage of the common struct clk,
we'd be better off doing the whole platform at once. However, mx51 is
the only mxc-based HW I have, hence the duplicated example port.

In the example port, the first change simply converts the mxc's struct
clk to a struct clk_mxc, using the new API.  The subsequent patches move
certain clocks to more specific data structures (eg clk_fixed and
clk_pll) where possible.

Ben Herrenschmidt is looking at using common struct clk code for powerpc
too, hence the kernel-wide approach.

Comments most welcome.

Cheers,


Jeremy

--
v4:
 * use mutex for enable/disable locking
 * DEFINE_CLK -> INIT_CLK, and pass the clk name for mutex init
 * struct clk_operations -> struct clk_ops

v3:
 * do clock usage refcounting in common code
 * provide sample port

v2:
 * no longer ARM-specific
 * use clk_operations

---
Jeremy Kerr (2):
      Add a common struct clk
      clk: Generic support for fixed-rate clocks

[RFC,PATCH 1/2] Add a common struct clk

[Jeremy Kerr]

We currently have 21 definitions of struct clk in the ARM architecture,
each defined on a per-platform basis. This makes it difficult to define
platform- (or architecture-) independent clock sources without making
assumptions about struct clk, and impossible to compile two
platforms with different struct clks into a single image.

This change is an effort to unify struct clk where possible, by defining
a common struct clk, containing a set of clock operations. Different
clock implementations can set their own operations, and have a standard
interface for generic code. The callback interface is exposed to the
kernel proper, while the clock implementations only need to be seen by
the platform internals.

This allows us to share clock code among platforms, and makes it
possible to dynamically create clock devices in platform-independent
code.

Platforms can enable the generic struct clock through
CONFIG_USE_COMMON_STRUCT_CLK. In this case, the clock infrastructure
consists of a common struct clk:

struct clk {
	const struct clk_ops	*ops;
	unsigned int		enable_count;
	struct mutex		mutex;
};

And a set of clock operations (defined per type of clock):

struct clk_operations {
       int             (*enable)(struct clk *);
       void            (*disable)(struct clk *);
       unsigned long   (*get_rate)(struct clk *);
       [...]
};

To define a hardware-specific clock, machine code can "subclass" the
struct clock into a new struct (adding any device-specific data), and
provide a set of operations:

struct clk_foo {
	struct clk	clk;
	void __iomem	*some_register;
};

struct clk_operations clk_foo_ops = {
	.get_rate = clk_foo_get_rate,
};

The common clock definitions are based on a development patch from Ben
Herrenschmidt <benh@kernel.crashing.org>.

Signed-off-by: Jeremy Kerr <jeremy.kerr@canonical.com>

---
 arch/Kconfig        |    3 
 include/linux/clk.h |  159 ++++++++++++++++++++++++++++++++++++--------
 2 files changed, 135 insertions(+), 27 deletions(-)

diff --git a/arch/Kconfig b/arch/Kconfig
index acda512..2458b5e 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -151,4 +151,7 @@ config HAVE_MIXED_BREAKPOINTS_REGS
 config HAVE_USER_RETURN_NOTIFIER
 	bool
 
+config USE_COMMON_STRUCT_CLK
+	bool
+
 source "kernel/gcov/Kconfig"
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 1d37f42..bb6957a 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -3,6 +3,7 @@
  *
  *  Copyright (C) 2004 ARM Limited.
  *  Written by Deep Blue Solutions Limited.
+ *  Copyright (c) 2010 Jeremy Kerr <jeremy.kerr@canonical.com>
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
@@ -11,36 +12,125 @@
 #ifndef __LINUX_CLK_H
 #define __LINUX_CLK_H
 
-struct device;
+#include <linux/err.h>
+#include <linux/mutex.h>
 
-/*
- * The base API.
+#ifdef CONFIG_USE_COMMON_STRUCT_CLK
+
+/* If we're using the common struct clk, we define the base clk object here,
+ * which will be 'subclassed' by device-specific implementations. For example:
+ *
+ *  struct clk_foo {
+ *      struct clk;
+ *      [device specific fields]
+ *  };
+ *
+ * We define the common clock API through a set of static inlines that call the
+ * corresponding clk_operations. The API is exactly the same as that documented
+ * in the !CONFIG_USE_COMMON_STRUCT_CLK case.
  */
 
+struct clk {
+	const struct clk_ops	*ops;
+	unsigned int		enable_count;
+	struct mutex		mutex;
+};
+
+#define INIT_CLK(name, o) \
+	{ .ops = &o, .enable_count = 0, \
+		.mutex = __MUTEX_INITIALIZER(name.mutex) }
+
+struct clk_ops {
+       int             (*enable)(struct clk *);
+       void            (*disable)(struct clk *);
+       unsigned long   (*get_rate)(struct clk *);
+       void            (*put)(struct clk *);
+       long            (*round_rate)(struct clk *, unsigned long);
+       int             (*set_rate)(struct clk *, unsigned long);
+       int             (*set_parent)(struct clk *, struct clk *);
+       struct clk*     (*get_parent)(struct clk *);
+};
+
+static inline int clk_enable(struct clk *clk)
+{
+	int ret = 0;
+
+	if (!clk->ops->enable)
+		return 0;
+
+	mutex_lock(&clk->mutex);
+	if (!clk->enable_count)
+		ret = clk->ops->enable(clk);
+
+	if (!ret)
+		clk->enable_count++;
+	mutex_unlock(&clk->mutex);
+
+	return ret;
+}
+
+static inline void clk_disable(struct clk *clk)
+{
+	if (!clk->ops->enable)
+		return;
+
+	mutex_lock(&clk->mutex);
+
+	if (!--clk->enable_count)
+		clk->ops->disable(clk);
+
+	mutex_unlock(&clk->mutex);
+}
+
+static inline unsigned long clk_get_rate(struct clk *clk)
+{
+	if (clk->ops->get_rate)
+		return clk->ops->get_rate(clk);
+	return 0;
+}
+
+static inline void clk_put(struct clk *clk)
+{
+	if (clk->ops->put)
+		clk->ops->put(clk);
+}
+
+static inline long clk_round_rate(struct clk *clk, unsigned long rate)
+{
+	if (clk->ops->round_rate)
+		return clk->ops->round_rate(clk, rate);
+	return -ENOSYS;
+}
+
+static inline int clk_set_rate(struct clk *clk, unsigned long rate)
+{
+	if (clk->ops->set_rate)
+		return clk->ops->set_rate(clk, rate);
+	return -ENOSYS;
+}
+
+static inline int clk_set_parent(struct clk *clk, struct clk *parent)
+{
+	if (clk->ops->set_parent)
+		return clk->ops->set_parent(clk, parent);
+	return -ENOSYS;
+}
+
+static inline struct clk *clk_get_parent(struct clk *clk)
+{
+	if (clk->ops->get_parent)
+		return clk->ops->get_parent(clk);
+	return ERR_PTR(-ENOSYS);
+}
+
+#else /* !CONFIG_USE_COMMON_STRUCT_CLK */
 
 /*
- * struct clk - an machine class defined object / cookie.
+ * Global clock object, actual structure is declared per-machine
  */
 struct clk;
 
 /**
- * clk_get - lookup and obtain a reference to a clock producer.
- * @dev: device for clock "consumer"
- * @id: clock comsumer ID
- *
- * Returns a struct clk corresponding to the clock producer, or
- * valid IS_ERR() condition containing errno.  The implementation
- * uses @dev and @id to determine the clock consumer, and thereby
- * the clock producer.  (IOW, @id may be identical strings, but
- * clk_get may return different clock producers depending on @dev.)
- *
- * Drivers must assume that the clock source is not enabled.
- *
- * clk_get should not be called from within interrupt context.
- */
-struct clk *clk_get(struct device *dev, const char *id);
-
-/**
  * clk_enable - inform the system when the clock source should be running.
  * @clk: clock source
  *
@@ -83,12 +173,6 @@ unsigned long clk_get_rate(struct clk *clk);
  */
 void clk_put(struct clk *clk);
 
-
-/*
- * The remaining APIs are optional for machine class support.
- */
-
-
 /**
  * clk_round_rate - adjust a rate to the exact rate a clock can provide
  * @clk: clock source
@@ -125,6 +209,27 @@ int clk_set_parent(struct clk *clk, struct clk *parent);
  */
 struct clk *clk_get_parent(struct clk *clk);
 
+#endif /* !CONFIG_USE_COMMON_STRUCT_CLK */
+
+struct device;
+
+/**
+ * clk_get - lookup and obtain a reference to a clock producer.
+ * @dev: device for clock "consumer"
+ * @id: clock comsumer ID
+ *
+ * Returns a struct clk corresponding to the clock producer, or
+ * valid IS_ERR() condition containing errno.  The implementation
+ * uses @dev and @id to determine the clock consumer, and thereby
+ * the clock producer.  (IOW, @id may be identical strings, but
+ * clk_get may return different clock producers depending on @dev.)
+ *
+ * Drivers must assume that the clock source is not enabled.
+ *
+ * clk_get should not be called from within interrupt context.
+ */
+struct clk *clk_get(struct device *dev, const char *id);
+
 /**
  * clk_get_sys - get a clock based upon the device name
  * @dev_id: device name

[RFC,PATCH 0/2] Common struct clk implementation, v4

[Jeremy Kerr]

Hi all,

These patches are an attempt to allow platforms to share clock code. At
present, the definitions of 'struct clk' are local to platform code,
which makes allocating and initialising cross-platform clock sources
difficult, and makes it impossible to compile a single image containing
support for two ARM platforms with different struct clks.

The two patches are for the architecture-independent kernel code,
introducing the common clk infrastructure. The changelog for the first
patch includes details about the new clock definitions.

As requested by rmk, I've put together a small series of patches
illustrating the usage of the common struct clock on the ARM imx51
platform. These are available in my git tree:

 git://kernel.ubuntu.com/jk/dt/linux-2.6

in the clk-common-mx51 branch (clk-common..clk-common-mx51).

The approach I've taken with the imx51 port is to temporarly duplicate
the platform-common clock code (ie, for all mxc-based boards) to enable
usage of the common struct clk on one machine (imx51), while leaving the
others as-is. For a proper platform-wide usage of the common struct clk,
we'd be better off doing the whole platform at once. However, mx51 is
the only mxc-based HW I have, hence the duplicated example port.

In the example port, the first change simply converts the mxc's struct
clk to a struct clk_mxc, using the new API.  The subsequent patches move
certain clocks to more specific data structures (eg clk_fixed and
clk_pll) where possible.

Ben Herrenschmidt is looking at using common struct clk code for powerpc
too, hence the kernel-wide approach.

Comments most welcome.

Cheers,


Jeremy

--
v4:
 * use mutex for enable/disable locking
 * DEFINE_CLK -> INIT_CLK, and pass the clk name for mutex init
 * struct clk_operations -> struct clk_ops

v3:
 * do clock usage refcounting in common code
 * provide sample port

v2:
 * no longer ARM-specific
 * use clk_operations

---
Jeremy Kerr (2):
      Add a common struct clk
      clk: Generic support for fixed-rate clocks

[RFC,PATCH 2/2] clk: Generic support for fixed-rate clocks

[Jeremy Kerr]

Since most platforms will need a fixed-rate clock, add one. This will
also serve as a basic example of an implementation of struct clk.

Signed-off-by: Jeremy Kerr <jeremy.kerr@canonical.com>

---
 include/linux/clk.h |   13 +++++++++++++
 kernel/Makefile     |    1 +
 kernel/clk.c        |   25 +++++++++++++++++++++++++
 3 files changed, 39 insertions(+)

diff --git a/include/linux/clk.h b/include/linux/clk.h
index bb6957a..6269b44 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -123,6 +123,19 @@ static inline struct clk *clk_get_parent(struct clk *clk)
 	return ERR_PTR(-ENOSYS);
 }
 
+/* Simple fixed-rate clock */
+struct clk_fixed {
+	struct clk	clk;
+	unsigned long	rate;
+};
+
+extern struct clk_ops clk_fixed_ops;
+
+#define INIT_CLK_FIXED(name, r) { \
+	.clk = INIT_CLK(name.clk, clk_fixed_ops), \
+	.rate = (r) \
+}
+
 #else /* !CONFIG_USE_COMMON_STRUCT_CLK */
 
 /*
diff --git a/kernel/Makefile b/kernel/Makefile
index 057472f..1ae15aa 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -105,6 +105,7 @@ obj-$(CONFIG_PERF_EVENTS) += perf_event.o
 obj-$(CONFIG_HAVE_HW_BREAKPOINT) += hw_breakpoint.o
 obj-$(CONFIG_USER_RETURN_NOTIFIER) += user-return-notifier.o
 obj-$(CONFIG_PADATA) += padata.o
+obj-$(CONFIG_USE_COMMON_STRUCT_CLK) += clk.o
 
 ifneq ($(CONFIG_SCHED_OMIT_FRAME_POINTER),y)
 # According to Alan Modra <alan@linuxcare.com.au>, the -fno-omit-frame-pointer is
diff --git a/kernel/clk.c b/kernel/clk.c
new file mode 100644
index 0000000..85c0670
--- /dev/null
+++ b/kernel/clk.c
@@ -0,0 +1,25 @@
+/*
+ * Copyright (C) 2010 Canonical Ltd <jeremy.kerr@canonical.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * Standard functionality for the common clock API.
+ */
+
+#include <linux/module.h>
+#include <linux/clk.h>
+
+#define to_clk_fixed(clk) (container_of(clk, struct clk_fixed, clk))
+
+static unsigned long clk_fixed_get_rate(struct clk *clk)
+{
+	return to_clk_fixed(clk)->rate;
+}
+
+struct clk_ops clk_fixed_ops = {
+	.get_rate = clk_fixed_get_rate,
+};
+
+EXPORT_SYMBOL_GPL(clk_fixed_ops);

[RFC,PATCH 1/2] Add a common struct clk

[Jeremy Kerr]

We currently have 21 definitions of struct clk in the ARM architecture,
each defined on a per-platform basis. This makes it difficult to define
platform- (or architecture-) independent clock sources without making
assumptions about struct clk, and impossible to compile two
platforms with different struct clks into a single image.

This change is an effort to unify struct clk where possible, by defining
a common struct clk, containing a set of clock operations. Different
clock implementations can set their own operations, and have a standard
interface for generic code. The callback interface is exposed to the
kernel proper, while the clock implementations only need to be seen by
the platform internals.

This allows us to share clock code among platforms, and makes it
possible to dynamically create clock devices in platform-independent
code.

Platforms can enable the generic struct clock through
CONFIG_USE_COMMON_STRUCT_CLK. In this case, the clock infrastructure
consists of a common struct clk:

struct clk {
	const struct clk_ops	*ops;
	unsigned int		enable_count;
	struct mutex		mutex;
};

And a set of clock operations (defined per type of clock):

struct clk_operations {
       int             (*enable)(struct clk *);
       void            (*disable)(struct clk *);
       unsigned long   (*get_rate)(struct clk *);
       [...]
};

To define a hardware-specific clock, machine code can "subclass" the
struct clock into a new struct (adding any device-specific data), and
provide a set of operations:

struct clk_foo {
	struct clk	clk;
	void __iomem	*some_register;
};

struct clk_operations clk_foo_ops = {
	.get_rate = clk_foo_get_rate,
};

The common clock definitions are based on a development patch from Ben
Herrenschmidt <benh@kernel.crashing.org>.

Signed-off-by: Jeremy Kerr <jeremy.kerr@canonical.com>

---
 arch/Kconfig        |    3 
 include/linux/clk.h |  159 ++++++++++++++++++++++++++++++++++++--------
 2 files changed, 135 insertions(+), 27 deletions(-)

diff --git a/arch/Kconfig b/arch/Kconfig
index acda512..2458b5e 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -151,4 +151,7 @@ config HAVE_MIXED_BREAKPOINTS_REGS
 config HAVE_USER_RETURN_NOTIFIER
 	bool
 
+config USE_COMMON_STRUCT_CLK
+	bool
+
 source "kernel/gcov/Kconfig"
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 1d37f42..bb6957a 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -3,6 +3,7 @@
  *
  *  Copyright (C) 2004 ARM Limited.
  *  Written by Deep Blue Solutions Limited.
+ *  Copyright (c) 2010 Jeremy Kerr <jeremy.kerr@canonical.com>
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
@@ -11,36 +12,125 @@
 #ifndef __LINUX_CLK_H
 #define __LINUX_CLK_H
 
-struct device;
+#include <linux/err.h>
+#include <linux/mutex.h>
 
-/*
- * The base API.
+#ifdef CONFIG_USE_COMMON_STRUCT_CLK
+
+/* If we're using the common struct clk, we define the base clk object here,
+ * which will be 'subclassed' by device-specific implementations. For example:
+ *
+ *  struct clk_foo {
+ *      struct clk;
+ *      [device specific fields]
+ *  };
+ *
+ * We define the common clock API through a set of static inlines that call the
+ * corresponding clk_operations. The API is exactly the same as that documented
+ * in the !CONFIG_USE_COMMON_STRUCT_CLK case.
  */
 
+struct clk {
+	const struct clk_ops	*ops;
+	unsigned int		enable_count;
+	struct mutex		mutex;
+};
+
+#define INIT_CLK(name, o) \
+	{ .ops = &o, .enable_count = 0, \
+		.mutex = __MUTEX_INITIALIZER(name.mutex) }
+
+struct clk_ops {
+       int             (*enable)(struct clk *);
+       void            (*disable)(struct clk *);
+       unsigned long   (*get_rate)(struct clk *);
+       void            (*put)(struct clk *);
+       long            (*round_rate)(struct clk *, unsigned long);
+       int             (*set_rate)(struct clk *, unsigned long);
+       int             (*set_parent)(struct clk *, struct clk *);
+       struct clk*     (*get_parent)(struct clk *);
+};
+
+static inline int clk_enable(struct clk *clk)
+{
+	int ret = 0;
+
+	if (!clk->ops->enable)
+		return 0;
+
+	mutex_lock(&clk->mutex);
+	if (!clk->enable_count)
+		ret = clk->ops->enable(clk);
+
+	if (!ret)
+		clk->enable_count++;
+	mutex_unlock(&clk->mutex);
+
+	return ret;
+}
+
+static inline void clk_disable(struct clk *clk)
+{
+	if (!clk->ops->enable)
+		return;
+
+	mutex_lock(&clk->mutex);
+
+	if (!--clk->enable_count)
+		clk->ops->disable(clk);
+
+	mutex_unlock(&clk->mutex);
+}
+
+static inline unsigned long clk_get_rate(struct clk *clk)
+{
+	if (clk->ops->get_rate)
+		return clk->ops->get_rate(clk);
+	return 0;
+}
+
+static inline void clk_put(struct clk *clk)
+{
+	if (clk->ops->put)
+		clk->ops->put(clk);
+}
+
+static inline long clk_round_rate(struct clk *clk, unsigned long rate)
+{
+	if (clk->ops->round_rate)
+		return clk->ops->round_rate(clk, rate);
+	return -ENOSYS;
+}
+
+static inline int clk_set_rate(struct clk *clk, unsigned long rate)
+{
+	if (clk->ops->set_rate)
+		return clk->ops->set_rate(clk, rate);
+	return -ENOSYS;
+}
+
+static inline int clk_set_parent(struct clk *clk, struct clk *parent)
+{
+	if (clk->ops->set_parent)
+		return clk->ops->set_parent(clk, parent);
+	return -ENOSYS;
+}
+
+static inline struct clk *clk_get_parent(struct clk *clk)
+{
+	if (clk->ops->get_parent)
+		return clk->ops->get_parent(clk);
+	return ERR_PTR(-ENOSYS);
+}
+
+#else /* !CONFIG_USE_COMMON_STRUCT_CLK */
 
 /*
- * struct clk - an machine class defined object / cookie.
+ * Global clock object, actual structure is declared per-machine
  */
 struct clk;
 
 /**
- * clk_get - lookup and obtain a reference to a clock producer.
- * @dev: device for clock "consumer"
- * @id: clock comsumer ID
- *
- * Returns a struct clk corresponding to the clock producer, or
- * valid IS_ERR() condition containing errno.  The implementation
- * uses @dev and @id to determine the clock consumer, and thereby
- * the clock producer.  (IOW, @id may be identical strings, but
- * clk_get may return different clock producers depending on @dev.)
- *
- * Drivers must assume that the clock source is not enabled.
- *
- * clk_get should not be called from within interrupt context.
- */
-struct clk *clk_get(struct device *dev, const char *id);
-
-/**
  * clk_enable - inform the system when the clock source should be running.
  * @clk: clock source
  *
@@ -83,12 +173,6 @@ unsigned long clk_get_rate(struct clk *clk);
  */
 void clk_put(struct clk *clk);
 
-
-/*
- * The remaining APIs are optional for machine class support.
- */
-
-
 /**
  * clk_round_rate - adjust a rate to the exact rate a clock can provide
  * @clk: clock source
@@ -125,6 +209,27 @@ int clk_set_parent(struct clk *clk, struct clk *parent);
  */
 struct clk *clk_get_parent(struct clk *clk);
 
+#endif /* !CONFIG_USE_COMMON_STRUCT_CLK */
+
+struct device;
+
+/**
+ * clk_get - lookup and obtain a reference to a clock producer.
+ * @dev: device for clock "consumer"
+ * @id: clock comsumer ID
+ *
+ * Returns a struct clk corresponding to the clock producer, or
+ * valid IS_ERR() condition containing errno.  The implementation
+ * uses @dev and @id to determine the clock consumer, and thereby
+ * the clock producer.  (IOW, @id may be identical strings, but
+ * clk_get may return different clock producers depending on @dev.)
+ *
+ * Drivers must assume that the clock source is not enabled.
+ *
+ * clk_get should not be called from within interrupt context.
+ */
+struct clk *clk_get(struct device *dev, const char *id);
+
 /**
  * clk_get_sys - get a clock based upon the device name
  * @dev_id: device name

Re: [RFC,PATCH 1/2] Add a common struct clk

[Ben Dooks]

On Fri, Jun 04, 2010 at 03:30:08PM +0800, Jeremy Kerr wrote:
> We currently have 21 definitions of struct clk in the ARM architecture,
> each defined on a per-platform basis. This makes it difficult to define
> platform- (or architecture-) independent clock sources without making
> assumptions about struct clk, and impossible to compile two
> platforms with different struct clks into a single image.

Whilst I agree that this is a useful thing to do, I'd like to ensure
we get a good base for our work before we get another reason for Linus
to complain about churn.
 
> This change is an effort to unify struct clk where possible, by defining
> a common struct clk, containing a set of clock operations. Different
> clock implementations can set their own operations, and have a standard
> interface for generic code. The callback interface is exposed to the
> kernel proper, while the clock implementations only need to be seen by
> the platform internals.
> 
> This allows us to share clock code among platforms, and makes it
> possible to dynamically create clock devices in platform-independent
> code.
> 
> Platforms can enable the generic struct clock through
> CONFIG_USE_COMMON_STRUCT_CLK. In this case, the clock infrastructure
> consists of a common struct clk:

What do people think of just changing everyone who is currently using
clk support to using this new implementation?
 
> struct clk {
> 	const struct clk_ops	*ops;
> 	unsigned int		enable_count;
> 	struct mutex		mutex;

I'm a little worried about the size of struct clk if all of them
have a mutex in them. If i'm right, this is 40 bytes of data each
clock has to take on board.

Doing the following:

find arch/arm -type f -name "*.c" | xargs grep -c -E "struct.*clk.*=.*{" | grep -v ":0" | awk 'BEGIN { count=0; FS=":" }
count += $2; END { print count }'

indicates that there's approximately 2241 clocks under arch/arm at the
moment. That's about 87KiB of mutexes if all are being compiled at
once.

~> };
> 
> And a set of clock operations (defined per type of clock):
> 
> struct clk_operations {
>        int             (*enable)(struct clk *);
still think that not passing an bool as a second argument is silly.

>        void            (*disable)(struct clk *);
~>        unsigned long   (*get_rate)(struct clk *);
>        [...]
> };
> 
> To define a hardware-specific clock, machine code can "subclass" the
> struct clock into a new struct (adding any device-specific data), and
> provide a set of operations:
> 
> struct clk_foo {
> 	struct clk	clk;
> 	void __iomem	*some_register;
> };
> 
> struct clk_operations clk_foo_ops = {
> 	.get_rate = clk_foo_get_rate,
> };
> 
> The common clock definitions are based on a development patch from Ben
> Herrenschmidt <benh@kernel.crashing.org>.
> 
> Signed-off-by: Jeremy Kerr <jeremy.kerr@canonical.com>
> 
> ---
>  arch/Kconfig        |    3 
>  include/linux/clk.h |  159 ++++++++++++++++++++++++++++++++++++--------
>  2 files changed, 135 insertions(+), 27 deletions(-)
~~~~> 
> diff --git a/arch/Kconfig b/arch/Kconfig
> index acda512..2458b5e 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -151,4 +151,7 @@ config HAVE_MIXED_BREAKPOINTS_REGS
>  config HAVE_USER_RETURN_NOTIFIER
>  	bool
>  
> +config USE_COMMON_STRUCT_CLK
> +	bool
> +
>  source "kernel/gcov/Kconfig"
> diff --git a/include/linux/clk.h b/include/linux/clk.h
> index 1d37f42..bb6957a 100644
> --- a/include/linux/clk.h
> +++ b/include/linux/clk.h
> @@ -3,6 +3,7 @@
>   *
>   *  Copyright (C) 2004 ARM Limited.
>   *  Written by Deep Blue Solutions Limited.
> + *  Copyright (c) 2010 Jeremy Kerr <jeremy.kerr@canonical.com>
>   *
>   * This program is free software; you can redistribute it and/or modify
>   * it under the terms of the GNU General Public License version 2 as
> @@ -11,36 +12,125 @@
>  #ifndef __LINUX_CLK_H
>  #define __LINUX_CLK_H
>  
> -struct device;
> +#include <linux/err.h>
> +#include <linux/mutex.h>
>  
> -/*
> - * The base API.
> +#ifdef CONFIG_USE_COMMON_STRUCT_CLK
> +
> +/* If we're using the common struct clk, we define the base clk object here,
> + * which will be 'subclassed' by device-specific implementations. For example:
> + *
> + *  struct clk_foo {
> + *      struct clk;
> + *      [device specific fields]
> + *  };
> + *
> + * We define the common clock API through a set of static inlines that call the
> + * corresponding clk_operations. The API is exactly the same as that documented
> + * in the !CONFIG_USE_COMMON_STRUCT_CLK case.
>   */
>  
> +struct clk {
> +	const struct clk_ops	*ops;
> +	unsigned int		enable_count;
> +	struct mutex		mutex;
> +};

how about defining a nice kerneldoc for this.

> +#define INIT_CLK(name, o) \
> +	{ .ops = &o, .enable_count = 0, \
> +		.mutex = __MUTEX_INITIALIZER(name.mutex) }

how about doing the mutex initinitialisation at registration
time, will save a pile of non-zero code in the image to mess up
the compression.

~> +struct clk_ops {
> +       int             (*enable)(struct clk *);
> +       void            (*disable)(struct clk *);
> +       unsigned long   (*get_rate)(struct clk *);
> +       void            (*put)(struct clk *);
> +       long            (*round_rate)(struct clk *, unsigned long);
> +       int             (*set_rate)(struct clk *, unsigned long);
> +       int             (*set_parent)(struct clk *, struct clk *);
> +       struct clk*     (*get_parent)(struct clk *);

should each clock carry a parent field and the this is returned by
the get parent call.~~

> +};
> +
> +static inline int clk_enable(struct clk *clk)
> +{
> +	int ret = 0;
> +
> +	if (!clk->ops->enable)
> +		return 0;
> +
> +	mutex_lock(&clk->mutex);
> +	if (!clk->enable_count)
> +		ret = clk->ops->enable(clk);
> +
> +	if (!ret)
> +		clk->enable_count++;
> +	mutex_unlock(&clk->mutex);
> +
> +	return ret;
> +}

So we're leaving the enable parent code now to each implementation?

I think this is a really bad decision, it leaves so much open to bad
code repetition, as well as something the core should really be doing
if it had a parent clock field.

~> +static inline void clk_disable(struct clk *clk)
> +{
> +	if (!clk->ops->enable)
> +		return;

so if we've no enable call we ignore disable too?

also, we don't keep an enable count if this fields are in use,
could people rely on this being correct even if the clock has
no enable/disable fields.

Would much rather see the enable_count being kept up-to-date
no matter what, given it may be watched by other parts of the
implementation, useful for debug info, and possibly useful if
later in the start sequence the clk_ops get changed to have this
field.~

> +~	mutex_lock(&clk->mutex);
> +
> +	if (!--clk->enable_count)
> +		clk->ops->disable(clk);
> +
> +	mutex_unlock(&clk->mutex);
> +}
> +
> +static inline unsigned long clk_get_rate(struct clk *clk)
> +{
> +	if (clk->ops->get_rate)
> +		return clk->ops->get_rate(clk);
> +	return 0;
> +}
> +
> +static inline void clk_put(struct clk *clk)
> +{
> +	if (clk->ops->put)
> +		clk->ops->put(clk);
> +}

I'm beginging to wonder if we don't just have a set of default ops
that get set into the clk+ops at registration time if these do
not have an implementation.
~
> +static inline long clk_round_rate(struct clk *clk, unsigned long rate)
> +{
> +	if (clk->ops->round_rate)
> +		return clk->ops->round_rate(clk, rate);
> +	return -ENOSYS;
> +}
> +
> +static inline int clk_set_rate(struct clk *clk, unsigned long rate)
>~ +{
> +	if (clk->ops->set_rate)
> +		return clk->ops->set_rate(clk, rate);
> +	return -ENOSYS;
> +}
> +
> +static inline int clk_set_parent(struct clk *clk, struct clk *parent)
> +{
> +	if (clk->ops->set_parent)
> +		return clk->ops->set_parent(clk, parent);
> +	return -ENOSYS;
> +}

We have an interesting problem here which I belive should be dealt
with, what happens when the clock's parent is changed with respect
to the enable count of the parent.

With the following instance:

	we have clocks a, b, c;
	a and b are possible parents for c;
	c starts off with a as parent

then the driver comes along:

	1) gets clocks a, b, c;
	2) clk_enable(c);
	3) clk_set_parent(c, b);
	
now we have the following:

	A) clk a now has an enable count of non-zero
	B) clk b may not be enabled
	C) even though clk a may now be unused, it is still running
	D) even though clk c was enabled, it isn't running since step3

this means that either any driver that is using a multi-parent clock
has to deal with the proper enable/disable of the parents (this is
is going to lead to code repetition, and I bet people will get it
badly wrong).

I belive the core of the clock code should deal with this, since
otherwise we end up with the situation of the same code being
repeated throughout the kernel.

> +static inline struct clk *clk_get_parent(struct clk *clk)
> +{
> +	if (clk->ops->get_parent)
> +		return clk->ops->get_parent(clk);
> +	return ERR_PTR(-ENOSYS);
> +}
>
> +#else /* !CONFIG_USE_COMMON_STRUCT_CLK */
>  
>  /*
> - * struct clk - an machine class defined object / cookie.
> + * Global clock object, actual structure is declared per-machine
>   */
>  struct clk;
>  
>  /**
> - * clk_get - lookup and obtain a reference to a clock producer.
> - * @dev: device for clock "consumer"
> - * @id: clock comsumer ID
> - *
> - * Returns a struct clk corresponding to the clock producer, or
> - * valid IS_ERR() condition containing errno.  The implementation
> - * uses @dev and @id to determine the clock consumer, and thereby
~> - * the clock producer.  (IOW, @id may be identical strings, but
> - * clk_get may return different clock producers depending on @dev.)
> - *
> - * Drivers must assume that the clock source is not enabled.
> - *
> - * clk_get should not be called from within interrupt context.
> - */
> -struct clk *clk_get(struct device *dev, const char *id);
> -
> -/**
>   * clk_enable - inform the system when the clock source should be running.
>   * @clk: clock source
>   *
> @@ -83,12 +173,6 @@ unsigned long clk_get_rate(struct clk *clk);
>   */
>  void clk_put(struct clk *clk);
>  
> -
> -/*
> - * The remaining APIs are optional for machine class support.
> - */
> -
> -
>  /**
>   * clk_round_rate - adjust a rate to the exact rate a clock can provide
>   * @clk: clock source
> @@ -125,6 +209,27 @@ int clk_set_parent(struct clk *clk, struct clk *parent);
>   */
>  struct clk *clk_get_parent(struct clk *clk);
>  
> +#endif /* !CONFIG_USE_COMMON_STRUCT_CLK */
> +
> +struct device;
> +
> +/**
> + * clk_get - lookup and obtain a reference to a clock producer.
> + * @dev: device for clock "consumer"
> + * @id: clock comsumer ID
> + *
> + * Returns a struct clk corresponding to the clock producer, or
> + * valid IS_ERR() condition containing errno.  The implementation
> + * uses @dev and @id to determine the clock consumer, and thereby
> + * the clock producer.  (IOW, @id may be identical strings, but
> + * clk_get may return different clock producers depending on @dev.)
> + *
> + * Drivers must assume that the clock source is not enabled.
> + *
> + * clk_get should not be called from within interrupt context.
~~> + */
> +struct clk *clk_get(struct device *dev, const char *id);
> +
>  /**
>   * clk_get_sys - get a clock based upon the device name
>   * @dev_id: device name
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

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

  'a smiley only costs 4 bytes'

[RFC,PATCH 1/2] Add a common struct clk

[Ben Dooks]

On Fri, Jun 04, 2010 at 03:30:08PM +0800, Jeremy Kerr wrote:
> We currently have 21 definitions of struct clk in the ARM architecture,
> each defined on a per-platform basis. This makes it difficult to define
> platform- (or architecture-) independent clock sources without making
> assumptions about struct clk, and impossible to compile two
> platforms with different struct clks into a single image.

Whilst I agree that this is a useful thing to do, I'd like to ensure
we get a good base for our work before we get another reason for Linus
to complain about churn.
 
> This change is an effort to unify struct clk where possible, by defining
> a common struct clk, containing a set of clock operations. Different
> clock implementations can set their own operations, and have a standard
> interface for generic code. The callback interface is exposed to the
> kernel proper, while the clock implementations only need to be seen by
> the platform internals.
> 
> This allows us to share clock code among platforms, and makes it
> possible to dynamically create clock devices in platform-independent
> code.
> 
> Platforms can enable the generic struct clock through
> CONFIG_USE_COMMON_STRUCT_CLK. In this case, the clock infrastructure
> consists of a common struct clk:

What do people think of just changing everyone who is currently using
clk support to using this new implementation?
 
> struct clk {
> 	const struct clk_ops	*ops;
> 	unsigned int		enable_count;
> 	struct mutex		mutex;

I'm a little worried about the size of struct clk if all of them
have a mutex in them. If i'm right, this is 40 bytes of data each
clock has to take on board.

Doing the following:

find arch/arm -type f -name "*.c" | xargs grep -c -E "struct.*clk.*=.*{" | grep -v ":0" | awk 'BEGIN { count=0; FS=":" }
count += $2; END { print count }'

indicates that there's approximately 2241 clocks under arch/arm at the
moment. That's about 87KiB of mutexes if all are being compiled at
once.

~> };
> 
> And a set of clock operations (defined per type of clock):
> 
> struct clk_operations {
>        int             (*enable)(struct clk *);
still think that not passing an bool as a second argument is silly.

>        void            (*disable)(struct clk *);
~>        unsigned long   (*get_rate)(struct clk *);
>        [...]
> };
> 
> To define a hardware-specific clock, machine code can "subclass" the
> struct clock into a new struct (adding any device-specific data), and
> provide a set of operations:
> 
> struct clk_foo {
> 	struct clk	clk;
> 	void __iomem	*some_register;
> };
> 
> struct clk_operations clk_foo_ops = {
> 	.get_rate = clk_foo_get_rate,
> };
> 
> The common clock definitions are based on a development patch from Ben
> Herrenschmidt <benh@kernel.crashing.org>.
> 
> Signed-off-by: Jeremy Kerr <jeremy.kerr@canonical.com>
> 
> ---
>  arch/Kconfig        |    3 
>  include/linux/clk.h |  159 ++++++++++++++++++++++++++++++++++++--------
>  2 files changed, 135 insertions(+), 27 deletions(-)
~~~~> 
> diff --git a/arch/Kconfig b/arch/Kconfig
> index acda512..2458b5e 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -151,4 +151,7 @@ config HAVE_MIXED_BREAKPOINTS_REGS
>  config HAVE_USER_RETURN_NOTIFIER
>  	bool
>  
> +config USE_COMMON_STRUCT_CLK
> +	bool
> +
>  source "kernel/gcov/Kconfig"
> diff --git a/include/linux/clk.h b/include/linux/clk.h
> index 1d37f42..bb6957a 100644
> --- a/include/linux/clk.h
> +++ b/include/linux/clk.h
> @@ -3,6 +3,7 @@
>   *
>   *  Copyright (C) 2004 ARM Limited.
>   *  Written by Deep Blue Solutions Limited.
> + *  Copyright (c) 2010 Jeremy Kerr <jeremy.kerr@canonical.com>
>   *
>   * This program is free software; you can redistribute it and/or modify
>   * it under the terms of the GNU General Public License version 2 as
> @@ -11,36 +12,125 @@
>  #ifndef __LINUX_CLK_H
>  #define __LINUX_CLK_H
>  
> -struct device;
> +#include <linux/err.h>
> +#include <linux/mutex.h>
>  
> -/*
> - * The base API.
> +#ifdef CONFIG_USE_COMMON_STRUCT_CLK
> +
> +/* If we're using the common struct clk, we define the base clk object here,
> + * which will be 'subclassed' by device-specific implementations. For example:
> + *
> + *  struct clk_foo {
> + *      struct clk;
> + *      [device specific fields]
> + *  };
> + *
> + * We define the common clock API through a set of static inlines that call the
> + * corresponding clk_operations. The API is exactly the same as that documented
> + * in the !CONFIG_USE_COMMON_STRUCT_CLK case.
>   */
>  
> +struct clk {
> +	const struct clk_ops	*ops;
> +	unsigned int		enable_count;
> +	struct mutex		mutex;
> +};

how about defining a nice kerneldoc for this.

> +#define INIT_CLK(name, o) \
> +	{ .ops = &o, .enable_count = 0, \
> +		.mutex = __MUTEX_INITIALIZER(name.mutex) }

how about doing the mutex initinitialisation at registration
time, will save a pile of non-zero code in the image to mess up
the compression.

~> +struct clk_ops {
> +       int             (*enable)(struct clk *);
> +       void            (*disable)(struct clk *);
> +       unsigned long   (*get_rate)(struct clk *);
> +       void            (*put)(struct clk *);
> +       long            (*round_rate)(struct clk *, unsigned long);
> +       int             (*set_rate)(struct clk *, unsigned long);
> +       int             (*set_parent)(struct clk *, struct clk *);
> +       struct clk*     (*get_parent)(struct clk *);

should each clock carry a parent field and the this is returned by
the get parent call.~~

> +};
> +
> +static inline int clk_enable(struct clk *clk)
> +{
> +	int ret = 0;
> +
> +	if (!clk->ops->enable)
> +		return 0;
> +
> +	mutex_lock(&clk->mutex);
> +	if (!clk->enable_count)
> +		ret = clk->ops->enable(clk);
> +
> +	if (!ret)
> +		clk->enable_count++;
> +	mutex_unlock(&clk->mutex);
> +
> +	return ret;
> +}

So we're leaving the enable parent code now to each implementation?

I think this is a really bad decision, it leaves so much open to bad
code repetition, as well as something the core should really be doing
if it had a parent clock field.

~> +static inline void clk_disable(struct clk *clk)
> +{
> +	if (!clk->ops->enable)
> +		return;

so if we've no enable call we ignore disable too?

also, we don't keep an enable count if this fields are in use,
could people rely on this being correct even if the clock has
no enable/disable fields.

Would much rather see the enable_count being kept up-to-date
no matter what, given it may be watched by other parts of the
implementation, useful for debug info, and possibly useful if
later in the start sequence the clk_ops get changed to have this
field.~

> +~	mutex_lock(&clk->mutex);
> +
> +	if (!--clk->enable_count)
> +		clk->ops->disable(clk);
> +
> +	mutex_unlock(&clk->mutex);
> +}
> +
> +static inline unsigned long clk_get_rate(struct clk *clk)
> +{
> +	if (clk->ops->get_rate)
> +		return clk->ops->get_rate(clk);
> +	return 0;
> +}
> +
> +static inline void clk_put(struct clk *clk)
> +{
> +	if (clk->ops->put)
> +		clk->ops->put(clk);
> +}

I'm beginging to wonder if we don't just have a set of default ops
that get set into the clk+ops at registration time if these do
not have an implementation.
~
> +static inline long clk_round_rate(struct clk *clk, unsigned long rate)
> +{
> +	if (clk->ops->round_rate)
> +		return clk->ops->round_rate(clk, rate);
> +	return -ENOSYS;
> +}
> +
> +static inline int clk_set_rate(struct clk *clk, unsigned long rate)
>~ +{
> +	if (clk->ops->set_rate)
> +		return clk->ops->set_rate(clk, rate);
> +	return -ENOSYS;
> +}
> +
> +static inline int clk_set_parent(struct clk *clk, struct clk *parent)
> +{
> +	if (clk->ops->set_parent)
> +		return clk->ops->set_parent(clk, parent);
> +	return -ENOSYS;
> +}

We have an interesting problem here which I belive should be dealt
with, what happens when the clock's parent is changed with respect
to the enable count of the parent.

With the following instance:

	we have clocks a, b, c;
	a and b are possible parents for c;
	c starts off with a as parent

then the driver comes along:

	1) gets clocks a, b, c;
	2) clk_enable(c);
	3) clk_set_parent(c, b);
	
now we have the following:

	A) clk a now has an enable count of non-zero
	B) clk b may not be enabled
	C) even though clk a may now be unused, it is still running
	D) even though clk c was enabled, it isn't running since step3

this means that either any driver that is using a multi-parent clock
has to deal with the proper enable/disable of the parents (this is
is going to lead to code repetition, and I bet people will get it
badly wrong).

I belive the core of the clock code should deal with this, since
otherwise we end up with the situation of the same code being
repeated throughout the kernel.

> +static inline struct clk *clk_get_parent(struct clk *clk)
> +{
> +	if (clk->ops->get_parent)
> +		return clk->ops->get_parent(clk);
> +	return ERR_PTR(-ENOSYS);
> +}
>
> +#else /* !CONFIG_USE_COMMON_STRUCT_CLK */
>  
>  /*
> - * struct clk - an machine class defined object / cookie.
> + * Global clock object, actual structure is declared per-machine
>   */
>  struct clk;
>  
>  /**
> - * clk_get - lookup and obtain a reference to a clock producer.
> - * @dev: device for clock "consumer"
> - * @id: clock comsumer ID
> - *
> - * Returns a struct clk corresponding to the clock producer, or
> - * valid IS_ERR() condition containing errno.  The implementation
> - * uses @dev and @id to determine the clock consumer, and thereby
~> - * the clock producer.  (IOW, @id may be identical strings, but
> - * clk_get may return different clock producers depending on @dev.)
> - *
> - * Drivers must assume that the clock source is not enabled.
> - *
> - * clk_get should not be called from within interrupt context.
> - */
> -struct clk *clk_get(struct device *dev, const char *id);
> -
> -/**
>   * clk_enable - inform the system when the clock source should be running.
>   * @clk: clock source
>   *
> @@ -83,12 +173,6 @@ unsigned long clk_get_rate(struct clk *clk);
>   */
>  void clk_put(struct clk *clk);
>  
> -
> -/*
> - * The remaining APIs are optional for machine class support.
> - */
> -
> -
>  /**
>   * clk_round_rate - adjust a rate to the exact rate a clock can provide
>   * @clk: clock source
> @@ -125,6 +209,27 @@ int clk_set_parent(struct clk *clk, struct clk *parent);
>   */
>  struct clk *clk_get_parent(struct clk *clk);
>  
> +#endif /* !CONFIG_USE_COMMON_STRUCT_CLK */
> +
> +struct device;
> +
> +/**
> + * clk_get - lookup and obtain a reference to a clock producer.
> + * @dev: device for clock "consumer"
> + * @id: clock comsumer ID
> + *
> + * Returns a struct clk corresponding to the clock producer, or
> + * valid IS_ERR() condition containing errno.  The implementation
> + * uses @dev and @id to determine the clock consumer, and thereby
> + * the clock producer.  (IOW, @id may be identical strings, but
> + * clk_get may return different clock producers depending on @dev.)
> + *
> + * Drivers must assume that the clock source is not enabled.
> + *
> + * clk_get should not be called from within interrupt context.
~~> + */
> +struct clk *clk_get(struct device *dev, const char *id);
> +
>  /**
>   * clk_get_sys - get a clock based upon the device name
>   * @dev_id: device name
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

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

  'a smiley only costs 4 bytes'

Re: [RFC,PATCH 1/2] Add a common struct clk

[Benjamin Herrenschmidt]

On Fri, 2010-06-11 at 05:20 +0100, Ben Dooks wrote:
> On Fri, Jun 04, 2010 at 03:30:08PM +0800, Jeremy Kerr wrote:
> > We currently have 21 definitions of struct clk in the ARM architecture,
> > each defined on a per-platform basis. This makes it difficult to define
> > platform- (or architecture-) independent clock sources without making
> > assumptions about struct clk, and impossible to compile two
> > platforms with different struct clks into a single image.
> 
> Whilst I agree that this is a useful thing to do, I'd like to ensure
> we get a good base for our work before we get another reason for Linus
> to complain about churn.

Well, in this case the goal is to unify things, both within ARM and
between architectures, so I fail to see Linus complaining about that :-)

> What do people think of just changing everyone who is currently using
> clk support to using this new implementation?

It's risky I suppose... there isn't many users of struct clk in powerpc
land today (I think one SoC platform only uses it upstream at the
moment) so I won't mind getting moved over at once but on ARM, you have
to deal with a lot more cruft that might warrant a more progressive
migration approach... but I'll let you guys judge.
 
> > struct clk {
> > 	const struct clk_ops	*ops;
> > 	unsigned int		enable_count;
> > 	struct mutex		mutex;
> 
> I'm a little worried about the size of struct clk if all of them
> have a mutex in them. If i'm right, this is 40 bytes of data each
> clock has to take on board.
> 
> Doing the following:
> 
> find arch/arm -type f -name "*.c" | xargs grep -c -E "struct.*clk.*=.*{" | grep -v ":0" | awk 'BEGIN { count=0; FS=":" }
> count += $2; END { print count }'
> 
> indicates that there's approximately 2241 clocks under arch/arm at the
> moment. That's about 87KiB of mutexes if all are being compiled at
> once.

I'm not convince this is relevant. You assume that all 2241 of those
clocks are statically allocated -and- compiled at the same time in the
same kernel binary.

I think both assumptions are terribly unlikely, especially in ARM
land :-) And in the event where you would actually manage to achieve
such a thing, I believe 87K is going to be the very last of your
worries :-)

In case it is of interest (and I know not everybody will want to do like
that) the way I intend to use this on powerpc is to have static clk_ops,
but instanciate the struct clk (or rather subclasses of struct clk)
on demand at clk_get time.

Basically, the device-tree (or platform code as a fallback) will bind
a device clock input to a clock provider / output pair. The clock
provider is something that produces struct clk * on demand for its
outputs.

Cheers,
Ben.

> ~> };
> > 
> > And a set of clock operations (defined per type of clock):
> > 
> > struct clk_operations {
> >        int             (*enable)(struct clk *);
> still think that not passing an bool as a second argument is silly.
> 
> >        void            (*disable)(struct clk *);
> ~>        unsigned long   (*get_rate)(struct clk *);
> >        [...]
> > };
> > 
> > To define a hardware-specific clock, machine code can "subclass" the
> > struct clock into a new struct (adding any device-specific data), and
> > provide a set of operations:
> > 
> > struct clk_foo {
> > 	struct clk	clk;
> > 	void __iomem	*some_register;
> > };
> > 
> > struct clk_operations clk_foo_ops = {
> > 	.get_rate = clk_foo_get_rate,
> > };
> > 
> > The common clock definitions are based on a development patch from Ben
> > Herrenschmidt <benh@kernel.crashing.org>.
> > 
> > Signed-off-by: Jeremy Kerr <jeremy.kerr@canonical.com>
> > 
> > ---
> >  arch/Kconfig        |    3 
> >  include/linux/clk.h |  159 ++++++++++++++++++++++++++++++++++++--------
> >  2 files changed, 135 insertions(+), 27 deletions(-)
> ~~~~> 
> > diff --git a/arch/Kconfig b/arch/Kconfig
> > index acda512..2458b5e 100644
> > --- a/arch/Kconfig
> > +++ b/arch/Kconfig
> > @@ -151,4 +151,7 @@ config HAVE_MIXED_BREAKPOINTS_REGS
> >  config HAVE_USER_RETURN_NOTIFIER
> >  	bool
> >  
> > +config USE_COMMON_STRUCT_CLK
> > +	bool
> > +
> >  source "kernel/gcov/Kconfig"
> > diff --git a/include/linux/clk.h b/include/linux/clk.h
> > index 1d37f42..bb6957a 100644
> > --- a/include/linux/clk.h
> > +++ b/include/linux/clk.h
> > @@ -3,6 +3,7 @@
> >   *
> >   *  Copyright (C) 2004 ARM Limited.
> >   *  Written by Deep Blue Solutions Limited.
> > + *  Copyright (c) 2010 Jeremy Kerr <jeremy.kerr@canonical.com>
> >   *
> >   * This program is free software; you can redistribute it and/or modify
> >   * it under the terms of the GNU General Public License version 2 as
> > @@ -11,36 +12,125 @@
> >  #ifndef __LINUX_CLK_H
> >  #define __LINUX_CLK_H
> >  
> > -struct device;
> > +#include <linux/err.h>
> > +#include <linux/mutex.h>
> >  
> > -/*
> > - * The base API.
> > +#ifdef CONFIG_USE_COMMON_STRUCT_CLK
> > +
> > +/* If we're using the common struct clk, we define the base clk object here,
> > + * which will be 'subclassed' by device-specific implementations. For example:
> > + *
> > + *  struct clk_foo {
> > + *      struct clk;
> > + *      [device specific fields]
> > + *  };
> > + *
> > + * We define the common clock API through a set of static inlines that call the
> > + * corresponding clk_operations. The API is exactly the same as that documented
> > + * in the !CONFIG_USE_COMMON_STRUCT_CLK case.
> >   */
> >  
> > +struct clk {
> > +	const struct clk_ops	*ops;
> > +	unsigned int		enable_count;
> > +	struct mutex		mutex;
> > +};
> 
> how about defining a nice kerneldoc for this.
> 
> > +#define INIT_CLK(name, o) \
> > +	{ .ops = &o, .enable_count = 0, \
> > +		.mutex = __MUTEX_INITIALIZER(name.mutex) }
> 
> how about doing the mutex initinitialisation at registration
> time, will save a pile of non-zero code in the image to mess up
> the compression.
> 
> ~> +struct clk_ops {
> > +       int             (*enable)(struct clk *);
> > +       void            (*disable)(struct clk *);
> > +       unsigned long   (*get_rate)(struct clk *);
> > +       void            (*put)(struct clk *);
> > +       long            (*round_rate)(struct clk *, unsigned long);
> > +       int             (*set_rate)(struct clk *, unsigned long);
> > +       int             (*set_parent)(struct clk *, struct clk *);
> > +       struct clk*     (*get_parent)(struct clk *);
> 
> should each clock carry a parent field and the this is returned by
> the get parent call.~~
> 
> > +};
> > +
> > +static inline int clk_enable(struct clk *clk)
> > +{
> > +	int ret = 0;
> > +
> > +	if (!clk->ops->enable)
> > +		return 0;
> > +
> > +	mutex_lock(&clk->mutex);
> > +	if (!clk->enable_count)
> > +		ret = clk->ops->enable(clk);
> > +
> > +	if (!ret)
> > +		clk->enable_count++;
> > +	mutex_unlock(&clk->mutex);
> > +
> > +	return ret;
> > +}
> 
> So we're leaving the enable parent code now to each implementation?
> 
> I think this is a really bad decision, it leaves so much open to bad
> code repetition, as well as something the core should really be doing
> if it had a parent clock field.
> 
> ~> +static inline void clk_disable(struct clk *clk)
> > +{
> > +	if (!clk->ops->enable)
> > +		return;
> 
> so if we've no enable call we ignore disable too?
> 
> also, we don't keep an enable count if this fields are in use,
> could people rely on this being correct even if the clock has
> no enable/disable fields.
> 
> Would much rather see the enable_count being kept up-to-date
> no matter what, given it may be watched by other parts of the
> implementation, useful for debug info, and possibly useful if
> later in the start sequence the clk_ops get changed to have this
> field.~
> 
> > +~	mutex_lock(&clk->mutex);
> > +
> > +	if (!--clk->enable_count)
> > +		clk->ops->disable(clk);
> > +
> > +	mutex_unlock(&clk->mutex);
> > +}
> > +
> > +static inline unsigned long clk_get_rate(struct clk *clk)
> > +{
> > +	if (clk->ops->get_rate)
> > +		return clk->ops->get_rate(clk);
> > +	return 0;
> > +}
> > +
> > +static inline void clk_put(struct clk *clk)
> > +{
> > +	if (clk->ops->put)
> > +		clk->ops->put(clk);
> > +}
> 
> I'm beginging to wonder if we don't just have a set of default ops
> that get set into the clk+ops at registration time if these do
> not have an implementation.
> ~
> > +static inline long clk_round_rate(struct clk *clk, unsigned long rate)
> > +{
> > +	if (clk->ops->round_rate)
> > +		return clk->ops->round_rate(clk, rate);
> > +	return -ENOSYS;
> > +}
> > +
> > +static inline int clk_set_rate(struct clk *clk, unsigned long rate)
> >~ +{
> > +	if (clk->ops->set_rate)
> > +		return clk->ops->set_rate(clk, rate);
> > +	return -ENOSYS;
> > +}
> > +
> > +static inline int clk_set_parent(struct clk *clk, struct clk *parent)
> > +{
> > +	if (clk->ops->set_parent)
> > +		return clk->ops->set_parent(clk, parent);
> > +	return -ENOSYS;
> > +}
> 
> We have an interesting problem here which I belive should be dealt
> with, what happens when the clock's parent is changed with respect
> to the enable count of the parent.
> 
> With the following instance:
> 
> 	we have clocks a, b, c;
> 	a and b are possible parents for c;
> 	c starts off with a as parent
> 
> then the driver comes along:
> 
> 	1) gets clocks a, b, c;
> 	2) clk_enable(c);
> 	3) clk_set_parent(c, b);
> 	
> now we have the following:
> 
> 	A) clk a now has an enable count of non-zero
> 	B) clk b may not be enabled
> 	C) even though clk a may now be unused, it is still running
> 	D) even though clk c was enabled, it isn't running since step3
> 
> this means that either any driver that is using a multi-parent clock
> has to deal with the proper enable/disable of the parents (this is
> is going to lead to code repetition, and I bet people will get it
> badly wrong).
> 
> I belive the core of the clock code should deal with this, since
> otherwise we end up with the situation of the same code being
> repeated throughout the kernel.
> 
> > +static inline struct clk *clk_get_parent(struct clk *clk)
> > +{
> > +	if (clk->ops->get_parent)
> > +		return clk->ops->get_parent(clk);
> > +	return ERR_PTR(-ENOSYS);
> > +}
> >
> > +#else /* !CONFIG_USE_COMMON_STRUCT_CLK */
> >  
> >  /*
> > - * struct clk - an machine class defined object / cookie.
> > + * Global clock object, actual structure is declared per-machine
> >   */
> >  struct clk;
> >  
> >  /**
> > - * clk_get - lookup and obtain a reference to a clock producer.
> > - * @dev: device for clock "consumer"
> > - * @id: clock comsumer ID
> > - *
> > - * Returns a struct clk corresponding to the clock producer, or
> > - * valid IS_ERR() condition containing errno.  The implementation
> > - * uses @dev and @id to determine the clock consumer, and thereby
> ~> - * the clock producer.  (IOW, @id may be identical strings, but
> > - * clk_get may return different clock producers depending on @dev.)
> > - *
> > - * Drivers must assume that the clock source is not enabled.
> > - *
> > - * clk_get should not be called from within interrupt context.
> > - */
> > -struct clk *clk_get(struct device *dev, const char *id);
> > -
> > -/**
> >   * clk_enable - inform the system when the clock source should be running.
> >   * @clk: clock source
> >   *
> > @@ -83,12 +173,6 @@ unsigned long clk_get_rate(struct clk *clk);
> >   */
> >  void clk_put(struct clk *clk);
> >  
> > -
> > -/*
> > - * The remaining APIs are optional for machine class support.
> > - */
> > -
> > -
> >  /**
> >   * clk_round_rate - adjust a rate to the exact rate a clock can provide
> >   * @clk: clock source
> > @@ -125,6 +209,27 @@ int clk_set_parent(struct clk *clk, struct clk *parent);
> >   */
> >  struct clk *clk_get_parent(struct clk *clk);
> >  
> > +#endif /* !CONFIG_USE_COMMON_STRUCT_CLK */
> > +
> > +struct device;
> > +
> > +/**
> > + * clk_get - lookup and obtain a reference to a clock producer.
> > + * @dev: device for clock "consumer"
> > + * @id: clock comsumer ID
> > + *
> > + * Returns a struct clk corresponding to the clock producer, or
> > + * valid IS_ERR() condition containing errno.  The implementation
> > + * uses @dev and @id to determine the clock consumer, and thereby
> > + * the clock producer.  (IOW, @id may be identical strings, but
> > + * clk_get may return different clock producers depending on @dev.)
> > + *
> > + * Drivers must assume that the clock source is not enabled.
> > + *
> > + * clk_get should not be called from within interrupt context.
> ~~> + */
> > +struct clk *clk_get(struct device *dev, const char *id);
> > +
> >  /**
> >   * clk_get_sys - get a clock based upon the device name
> >   * @dev_id: device name
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
> 

[RFC,PATCH 1/2] Add a common struct clk

[Benjamin Herrenschmidt]

On Fri, 2010-06-11 at 05:20 +0100, Ben Dooks wrote:
> On Fri, Jun 04, 2010 at 03:30:08PM +0800, Jeremy Kerr wrote:
> > We currently have 21 definitions of struct clk in the ARM architecture,
> > each defined on a per-platform basis. This makes it difficult to define
> > platform- (or architecture-) independent clock sources without making
> > assumptions about struct clk, and impossible to compile two
> > platforms with different struct clks into a single image.
> 
> Whilst I agree that this is a useful thing to do, I'd like to ensure
> we get a good base for our work before we get another reason for Linus
> to complain about churn.

Well, in this case the goal is to unify things, both within ARM and
between architectures, so I fail to see Linus complaining about that :-)

> What do people think of just changing everyone who is currently using
> clk support to using this new implementation?

It's risky I suppose... there isn't many users of struct clk in powerpc
land today (I think one SoC platform only uses it upstream at the
moment) so I won't mind getting moved over at once but on ARM, you have
to deal with a lot more cruft that might warrant a more progressive
migration approach... but I'll let you guys judge.
 
> > struct clk {
> > 	const struct clk_ops	*ops;
> > 	unsigned int		enable_count;
> > 	struct mutex		mutex;
> 
> I'm a little worried about the size of struct clk if all of them
> have a mutex in them. If i'm right, this is 40 bytes of data each
> clock has to take on board.
> 
> Doing the following:
> 
> find arch/arm -type f -name "*.c" | xargs grep -c -E "struct.*clk.*=.*{" | grep -v ":0" | awk 'BEGIN { count=0; FS=":" }
> count += $2; END { print count }'
> 
> indicates that there's approximately 2241 clocks under arch/arm at the
> moment. That's about 87KiB of mutexes if all are being compiled at
> once.

I'm not convince this is relevant. You assume that all 2241 of those
clocks are statically allocated -and- compiled at the same time in the
same kernel binary.

I think both assumptions are terribly unlikely, especially in ARM
land :-) And in the event where you would actually manage to achieve
such a thing, I believe 87K is going to be the very last of your
worries :-)

In case it is of interest (and I know not everybody will want to do like
that) the way I intend to use this on powerpc is to have static clk_ops,
but instanciate the struct clk (or rather subclasses of struct clk)
on demand at clk_get time.

Basically, the device-tree (or platform code as a fallback) will bind
a device clock input to a clock provider / output pair. The clock
provider is something that produces struct clk * on demand for its
outputs.

Cheers,
Ben.

> ~> };
> > 
> > And a set of clock operations (defined per type of clock):
> > 
> > struct clk_operations {
> >        int             (*enable)(struct clk *);
> still think that not passing an bool as a second argument is silly.
> 
> >        void            (*disable)(struct clk *);
> ~>        unsigned long   (*get_rate)(struct clk *);
> >        [...]
> > };
> > 
> > To define a hardware-specific clock, machine code can "subclass" the
> > struct clock into a new struct (adding any device-specific data), and
> > provide a set of operations:
> > 
> > struct clk_foo {
> > 	struct clk	clk;
> > 	void __iomem	*some_register;
> > };
> > 
> > struct clk_operations clk_foo_ops = {
> > 	.get_rate = clk_foo_get_rate,
> > };
> > 
> > The common clock definitions are based on a development patch from Ben
> > Herrenschmidt <benh@kernel.crashing.org>.
> > 
> > Signed-off-by: Jeremy Kerr <jeremy.kerr@canonical.com>
> > 
> > ---
> >  arch/Kconfig        |    3 
> >  include/linux/clk.h |  159 ++++++++++++++++++++++++++++++++++++--------
> >  2 files changed, 135 insertions(+), 27 deletions(-)
> ~~~~> 
> > diff --git a/arch/Kconfig b/arch/Kconfig
> > index acda512..2458b5e 100644
> > --- a/arch/Kconfig
> > +++ b/arch/Kconfig
> > @@ -151,4 +151,7 @@ config HAVE_MIXED_BREAKPOINTS_REGS
> >  config HAVE_USER_RETURN_NOTIFIER
> >  	bool
> >  
> > +config USE_COMMON_STRUCT_CLK
> > +	bool
> > +
> >  source "kernel/gcov/Kconfig"
> > diff --git a/include/linux/clk.h b/include/linux/clk.h
> > index 1d37f42..bb6957a 100644
> > --- a/include/linux/clk.h
> > +++ b/include/linux/clk.h
> > @@ -3,6 +3,7 @@
> >   *
> >   *  Copyright (C) 2004 ARM Limited.
> >   *  Written by Deep Blue Solutions Limited.
> > + *  Copyright (c) 2010 Jeremy Kerr <jeremy.kerr@canonical.com>
> >   *
> >   * This program is free software; you can redistribute it and/or modify
> >   * it under the terms of the GNU General Public License version 2 as
> > @@ -11,36 +12,125 @@
> >  #ifndef __LINUX_CLK_H
> >  #define __LINUX_CLK_H
> >  
> > -struct device;
> > +#include <linux/err.h>
> > +#include <linux/mutex.h>
> >  
> > -/*
> > - * The base API.
> > +#ifdef CONFIG_USE_COMMON_STRUCT_CLK
> > +
> > +/* If we're using the common struct clk, we define the base clk object here,
> > + * which will be 'subclassed' by device-specific implementations. For example:
> > + *
> > + *  struct clk_foo {
> > + *      struct clk;
> > + *      [device specific fields]
> > + *  };
> > + *
> > + * We define the common clock API through a set of static inlines that call the
> > + * corresponding clk_operations. The API is exactly the same as that documented
> > + * in the !CONFIG_USE_COMMON_STRUCT_CLK case.
> >   */
> >  
> > +struct clk {
> > +	const struct clk_ops	*ops;
> > +	unsigned int		enable_count;
> > +	struct mutex		mutex;
> > +};
> 
> how about defining a nice kerneldoc for this.
> 
> > +#define INIT_CLK(name, o) \
> > +	{ .ops = &o, .enable_count = 0, \
> > +		.mutex = __MUTEX_INITIALIZER(name.mutex) }
> 
> how about doing the mutex initinitialisation at registration
> time, will save a pile of non-zero code in the image to mess up
> the compression.
> 
> ~> +struct clk_ops {
> > +       int             (*enable)(struct clk *);
> > +       void            (*disable)(struct clk *);
> > +       unsigned long   (*get_rate)(struct clk *);
> > +       void            (*put)(struct clk *);
> > +       long            (*round_rate)(struct clk *, unsigned long);
> > +       int             (*set_rate)(struct clk *, unsigned long);
> > +       int             (*set_parent)(struct clk *, struct clk *);
> > +       struct clk*     (*get_parent)(struct clk *);
> 
> should each clock carry a parent field and the this is returned by
> the get parent call.~~
> 
> > +};
> > +
> > +static inline int clk_enable(struct clk *clk)
> > +{
> > +	int ret = 0;
> > +
> > +	if (!clk->ops->enable)
> > +		return 0;
> > +
> > +	mutex_lock(&clk->mutex);
> > +	if (!clk->enable_count)
> > +		ret = clk->ops->enable(clk);
> > +
> > +	if (!ret)
> > +		clk->enable_count++;
> > +	mutex_unlock(&clk->mutex);
> > +
> > +	return ret;
> > +}
> 
> So we're leaving the enable parent code now to each implementation?
> 
> I think this is a really bad decision, it leaves so much open to bad
> code repetition, as well as something the core should really be doing
> if it had a parent clock field.
> 
> ~> +static inline void clk_disable(struct clk *clk)
> > +{
> > +	if (!clk->ops->enable)
> > +		return;
> 
> so if we've no enable call we ignore disable too?
> 
> also, we don't keep an enable count if this fields are in use,
> could people rely on this being correct even if the clock has
> no enable/disable fields.
> 
> Would much rather see the enable_count being kept up-to-date
> no matter what, given it may be watched by other parts of the
> implementation, useful for debug info, and possibly useful if
> later in the start sequence the clk_ops get changed to have this
> field.~
> 
> > +~	mutex_lock(&clk->mutex);
> > +
> > +	if (!--clk->enable_count)
> > +		clk->ops->disable(clk);
> > +
> > +	mutex_unlock(&clk->mutex);
> > +}
> > +
> > +static inline unsigned long clk_get_rate(struct clk *clk)
> > +{
> > +	if (clk->ops->get_rate)
> > +		return clk->ops->get_rate(clk);
> > +	return 0;
> > +}
> > +
> > +static inline void clk_put(struct clk *clk)
> > +{
> > +	if (clk->ops->put)
> > +		clk->ops->put(clk);
> > +}
> 
> I'm beginging to wonder if we don't just have a set of default ops
> that get set into the clk+ops at registration time if these do
> not have an implementation.
> ~
> > +static inline long clk_round_rate(struct clk *clk, unsigned long rate)
> > +{
> > +	if (clk->ops->round_rate)
> > +		return clk->ops->round_rate(clk, rate);
> > +	return -ENOSYS;
> > +}
> > +
> > +static inline int clk_set_rate(struct clk *clk, unsigned long rate)
> >~ +{
> > +	if (clk->ops->set_rate)
> > +		return clk->ops->set_rate(clk, rate);
> > +	return -ENOSYS;
> > +}
> > +
> > +static inline int clk_set_parent(struct clk *clk, struct clk *parent)
> > +{
> > +	if (clk->ops->set_parent)
> > +		return clk->ops->set_parent(clk, parent);
> > +	return -ENOSYS;
> > +}
> 
> We have an interesting problem here which I belive should be dealt
> with, what happens when the clock's parent is changed with respect
> to the enable count of the parent.
> 
> With the following instance:
> 
> 	we have clocks a, b, c;
> 	a and b are possible parents for c;
> 	c starts off with a as parent
> 
> then the driver comes along:
> 
> 	1) gets clocks a, b, c;
> 	2) clk_enable(c);
> 	3) clk_set_parent(c, b);
> 	
> now we have the following:
> 
> 	A) clk a now has an enable count of non-zero
> 	B) clk b may not be enabled
> 	C) even though clk a may now be unused, it is still running
> 	D) even though clk c was enabled, it isn't running since step3
> 
> this means that either any driver that is using a multi-parent clock
> has to deal with the proper enable/disable of the parents (this is
> is going to lead to code repetition, and I bet people will get it
> badly wrong).
> 
> I belive the core of the clock code should deal with this, since
> otherwise we end up with the situation of the same code being
> repeated throughout the kernel.
> 
> > +static inline struct clk *clk_get_parent(struct clk *clk)
> > +{
> > +	if (clk->ops->get_parent)
> > +		return clk->ops->get_parent(clk);
> > +	return ERR_PTR(-ENOSYS);
> > +}
> >
> > +#else /* !CONFIG_USE_COMMON_STRUCT_CLK */
> >  
> >  /*
> > - * struct clk - an machine class defined object / cookie.
> > + * Global clock object, actual structure is declared per-machine
> >   */
> >  struct clk;
> >  
> >  /**
> > - * clk_get - lookup and obtain a reference to a clock producer.
> > - * @dev: device for clock "consumer"
> > - * @id: clock comsumer ID
> > - *
> > - * Returns a struct clk corresponding to the clock producer, or
> > - * valid IS_ERR() condition containing errno.  The implementation
> > - * uses @dev and @id to determine the clock consumer, and thereby
> ~> - * the clock producer.  (IOW, @id may be identical strings, but
> > - * clk_get may return different clock producers depending on @dev.)
> > - *
> > - * Drivers must assume that the clock source is not enabled.
> > - *
> > - * clk_get should not be called from within interrupt context.
> > - */
> > -struct clk *clk_get(struct device *dev, const char *id);
> > -
> > -/**
> >   * clk_enable - inform the system when the clock source should be running.
> >   * @clk: clock source
> >   *
> > @@ -83,12 +173,6 @@ unsigned long clk_get_rate(struct clk *clk);
> >   */
> >  void clk_put(struct clk *clk);
> >  
> > -
> > -/*
> > - * The remaining APIs are optional for machine class support.
> > - */
> > -
> > -
> >  /**
> >   * clk_round_rate - adjust a rate to the exact rate a clock can provide
> >   * @clk: clock source
> > @@ -125,6 +209,27 @@ int clk_set_parent(struct clk *clk, struct clk *parent);
> >   */
> >  struct clk *clk_get_parent(struct clk *clk);
> >  
> > +#endif /* !CONFIG_USE_COMMON_STRUCT_CLK */
> > +
> > +struct device;
> > +
> > +/**
> > + * clk_get - lookup and obtain a reference to a clock producer.
> > + * @dev: device for clock "consumer"
> > + * @id: clock comsumer ID
> > + *
> > + * Returns a struct clk corresponding to the clock producer, or
> > + * valid IS_ERR() condition containing errno.  The implementation
> > + * uses @dev and @id to determine the clock consumer, and thereby
> > + * the clock producer.  (IOW, @id may be identical strings, but
> > + * clk_get may return different clock producers depending on @dev.)
> > + *
> > + * Drivers must assume that the clock source is not enabled.
> > + *
> > + * clk_get should not be called from within interrupt context.
> ~~> + */
> > +struct clk *clk_get(struct device *dev, const char *id);
> > +
> >  /**
> >   * clk_get_sys - get a clock based upon the device name
> >   * @dev_id: device name
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo at vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [RFC,PATCH 1/2] Add a common struct clk

[Jeremy Kerr]

Hi Ben,

> > Platforms can enable the generic struct clock through
> > CONFIG_USE_COMMON_STRUCT_CLK. In this case, the clock infrastructure
> > consists of a common struct clk:
>
> What do people think of just changing everyone who is currently using
> clk support to using this new implementation?

I think it'd be too-big a task to take on for a single change; we'd be better 
off allowing the two implementations concurrently. Also, I'd say that there 
are many platforms that will never need the new code, so no need to change 
them over.

> > struct clk {
> > 
> > 	const struct clk_ops	*ops;
> > 	unsigned int		enable_count;
> > 	struct mutex		mutex;
> 
> I'm a little worried about the size of struct clk if all of them
> have a mutex in them. If i'm right, this is 40 bytes of data each
> clock has to take on board.

Yeah, I see your concern about adding a mutex per clock, but I do think it's 
the best solution. Of course, if there's a better way to do it, I'm happy to 
try it out.

I believe we need to ensure that clocks are enabled when clk_enable returns, 
so we'll need some mechanism for waiting on the thread doing the 
enable/disable. Since (as you say) some clocks may take 100s of microseconds 
to enable, we'll need a lock that we can hold while sleeping.

We could use a global mutex for all clocks, but that would complicate the 
interface by requiring non-locking versions of enable/disable, to allow 
recursive enable/disable of parent clocks. Also, we'd lose any parallelism 
possible in clock initialisation (we want boot to be fast, right?)

Without all the debug stuff, it looks like a mutex is 12 bytes on UP, 20 on 
SMP. That brings the maximum possible overhead to 26kB/43kB. More tolerable, 
but not great.

You know what would be awesome? if we could put platform-specific code in 
their own sections and the discard the ones we know we won't need after boot. 
But that's a discussion for another day...

> >        int             (*enable)(struct clk *);
> 
> still think that not passing an bool as a second argument is silly.

Still don't :D

> > +struct clk {
> > +	const struct clk_ops	*ops;
> > +	unsigned int		enable_count;
> > +	struct mutex		mutex;
> > +};
> 
> how about defining a nice kerneldoc for this.

Shall do. I've also decided to un-inline the base API, which means that I can 
use all of the existing prototypes in clk.h, which means the kerneldoc is 
shared too.

> > +#define INIT_CLK(name, o) \
> > +	{ .ops = &o, .enable_count = 0, \
> > +		.mutex = __MUTEX_INITIALIZER(name.mutex) }
> 
> how about doing the mutex initinitialisation at registration
> time, will save a pile of non-zero code in the image to mess up
> the compression.

I've just yesterday added the following to my tree, to allow dynamic 
initialisation:

static inline void clk_init(struct clk *clk, const struct clk_ops *ops)
{
	clk->ops = ops;
	clk->enable_count = 0;
	mutex_init(&clk->mutex);
}

So we can do this either way.

> ~> +struct clk_ops {
> 
> > +       int             (*enable)(struct clk *);
> > +       void            (*disable)(struct clk *);
> > +       unsigned long   (*get_rate)(struct clk *);
> > +       void            (*put)(struct clk *);
> > +       long            (*round_rate)(struct clk *, unsigned long);
> > +       int             (*set_rate)(struct clk *, unsigned long);
> > +       int             (*set_parent)(struct clk *, struct clk *);
> > +       struct clk*     (*get_parent)(struct clk *);
> 
> should each clock carry a parent field and the this is returned by
> the get parent call.~~

I've been debating dropping the get_parent and set_parent ops entirely, 
actually; setting a parent seems to be quite specific to hardware (you have to 
know that a particular clock can be a parent of another clock), so it seems 
like something that we shouldn't expose to drivers through this API. For the 
code that knows the hardware, it can probably access the underlying clock 
types directly.

However, I see that something in the samsung code uses clk_get_parent for what 
seems like a sane purpose, so have left them in for now. Or could this be done 
differently?

> > +static inline int clk_enable(struct clk *clk)
> > +{
> > +	int ret = 0;
> > +
> > +	if (!clk->ops->enable)
> > +		return 0;
> > +
> > +	mutex_lock(&clk->mutex);
> > +	if (!clk->enable_count)
> > +		ret = clk->ops->enable(clk);
> > +
> > +	if (!ret)
> > +		clk->enable_count++;
> > +	mutex_unlock(&clk->mutex);
> > +
> > +	return ret;
> > +}
> 
> So we're leaving the enable parent code now to each implementation?
> 
> I think this is a really bad decision, it leaves so much open to bad
> code repetition, as well as something the core should really be doing
> if it had a parent clock field.

I'm tempted to go in either direction - either no [sg]et_parent in the clk 
API, or add a parent to struct clk, but first I'd need to get up to speed on 
the external uses of the clk's parent. Want to catch me on IRC to discuss?

> ~> +static inline void clk_disable(struct clk *clk)
> 
> > +{
> > +	if (!clk->ops->enable)
> > +		return;
> 
> so if we've no enable call we ignore disable too?

Typo; this should be "if (!clk->ops->disable)". Will fix for the next 
revision.

> also, we don't keep an enable count if this fields are in use,
> could people rely on this being correct even if the clock has
> no enable/disable fields.
> 
> Would much rather see the enable_count being kept up-to-date
> no matter what, given it may be watched by other parts of the
> implementation, useful for debug info, and possibly useful if
> later in the start sequence the clk_ops get changed to have this
> field.~

Checking for the ops first allows us to skip the mutex acquire, but I'm happy 
either way.

> > +~	mutex_lock(&clk->mutex);
> > +
> > +	if (!--clk->enable_count)
> > +		clk->ops->disable(clk);
> > +
> > +	mutex_unlock(&clk->mutex);
> > +}
> > +
> > +static inline unsigned long clk_get_rate(struct clk *clk)
> > +{
> > +	if (clk->ops->get_rate)
> > +		return clk->ops->get_rate(clk);
> > +	return 0;
> > +}
> > +
> > +static inline void clk_put(struct clk *clk)
> > +{
> > +	if (clk->ops->put)
> > +		clk->ops->put(clk);
> > +}
> 
> I'm beginging to wonder if we don't just have a set of default ops
> that get set into the clk+ops at registration time if these do
> not have an implementation.

Using default ops would mean a couple of things:

1) we can no longer mark the real ops as const; and
2) we can no longer avoid the hard-to-predict indirect branch

> this means that either any driver that is using a multi-parent clock
> has to deal with the proper enable/disable of the parents (this is
> is going to lead to code repetition, and I bet people will get it
> badly wrong).
> 
> I belive the core of the clock code should deal with this, since
> otherwise we end up with the situation of the same code being
> repeated throughout the kernel.

I see the concern here, but I'll defer this one until we've decided on whether 
or not the parents should be exposed through the API.

Cheers,


Jeremy

[RFC,PATCH 1/2] Add a common struct clk

[Jeremy Kerr]

Hi Ben,

> > Platforms can enable the generic struct clock through
> > CONFIG_USE_COMMON_STRUCT_CLK. In this case, the clock infrastructure
> > consists of a common struct clk:
>
> What do people think of just changing everyone who is currently using
> clk support to using this new implementation?

I think it'd be too-big a task to take on for a single change; we'd be better 
off allowing the two implementations concurrently. Also, I'd say that there 
are many platforms that will never need the new code, so no need to change 
them over.

> > struct clk {
> > 
> > 	const struct clk_ops	*ops;
> > 	unsigned int		enable_count;
> > 	struct mutex		mutex;
> 
> I'm a little worried about the size of struct clk if all of them
> have a mutex in them. If i'm right, this is 40 bytes of data each
> clock has to take on board.

Yeah, I see your concern about adding a mutex per clock, but I do think it's 
the best solution. Of course, if there's a better way to do it, I'm happy to 
try it out.

I believe we need to ensure that clocks are enabled when clk_enable returns, 
so we'll need some mechanism for waiting on the thread doing the 
enable/disable. Since (as you say) some clocks may take 100s of microseconds 
to enable, we'll need a lock that we can hold while sleeping.

We could use a global mutex for all clocks, but that would complicate the 
interface by requiring non-locking versions of enable/disable, to allow 
recursive enable/disable of parent clocks. Also, we'd lose any parallelism 
possible in clock initialisation (we want boot to be fast, right?)

Without all the debug stuff, it looks like a mutex is 12 bytes on UP, 20 on 
SMP. That brings the maximum possible overhead to 26kB/43kB. More tolerable, 
but not great.

You know what would be awesome? if we could put platform-specific code in 
their own sections and the discard the ones we know we won't need after boot. 
But that's a discussion for another day...

> >        int             (*enable)(struct clk *);
> 
> still think that not passing an bool as a second argument is silly.

Still don't :D

> > +struct clk {
> > +	const struct clk_ops	*ops;
> > +	unsigned int		enable_count;
> > +	struct mutex		mutex;
> > +};
> 
> how about defining a nice kerneldoc for this.

Shall do. I've also decided to un-inline the base API, which means that I can 
use all of the existing prototypes in clk.h, which means the kerneldoc is 
shared too.

> > +#define INIT_CLK(name, o) \
> > +	{ .ops = &o, .enable_count = 0, \
> > +		.mutex = __MUTEX_INITIALIZER(name.mutex) }
> 
> how about doing the mutex initinitialisation at registration
> time, will save a pile of non-zero code in the image to mess up
> the compression.

I've just yesterday added the following to my tree, to allow dynamic 
initialisation:

static inline void clk_init(struct clk *clk, const struct clk_ops *ops)
{
	clk->ops = ops;
	clk->enable_count = 0;
	mutex_init(&clk->mutex);
}

So we can do this either way.

> ~> +struct clk_ops {
> 
> > +       int             (*enable)(struct clk *);
> > +       void            (*disable)(struct clk *);
> > +       unsigned long   (*get_rate)(struct clk *);
> > +       void            (*put)(struct clk *);
> > +       long            (*round_rate)(struct clk *, unsigned long);
> > +       int             (*set_rate)(struct clk *, unsigned long);
> > +       int             (*set_parent)(struct clk *, struct clk *);
> > +       struct clk*     (*get_parent)(struct clk *);
> 
> should each clock carry a parent field and the this is returned by
> the get parent call.~~

I've been debating dropping the get_parent and set_parent ops entirely, 
actually; setting a parent seems to be quite specific to hardware (you have to 
know that a particular clock can be a parent of another clock), so it seems 
like something that we shouldn't expose to drivers through this API. For the 
code that knows the hardware, it can probably access the underlying clock 
types directly.

However, I see that something in the samsung code uses clk_get_parent for what 
seems like a sane purpose, so have left them in for now. Or could this be done 
differently?

> > +static inline int clk_enable(struct clk *clk)
> > +{
> > +	int ret = 0;
> > +
> > +	if (!clk->ops->enable)
> > +		return 0;
> > +
> > +	mutex_lock(&clk->mutex);
> > +	if (!clk->enable_count)
> > +		ret = clk->ops->enable(clk);
> > +
> > +	if (!ret)
> > +		clk->enable_count++;
> > +	mutex_unlock(&clk->mutex);
> > +
> > +	return ret;
> > +}
> 
> So we're leaving the enable parent code now to each implementation?
> 
> I think this is a really bad decision, it leaves so much open to bad
> code repetition, as well as something the core should really be doing
> if it had a parent clock field.

I'm tempted to go in either direction - either no [sg]et_parent in the clk 
API, or add a parent to struct clk, but first I'd need to get up to speed on 
the external uses of the clk's parent. Want to catch me on IRC to discuss?

> ~> +static inline void clk_disable(struct clk *clk)
> 
> > +{
> > +	if (!clk->ops->enable)
> > +		return;
> 
> so if we've no enable call we ignore disable too?

Typo; this should be "if (!clk->ops->disable)". Will fix for the next 
revision.

> also, we don't keep an enable count if this fields are in use,
> could people rely on this being correct even if the clock has
> no enable/disable fields.
> 
> Would much rather see the enable_count being kept up-to-date
> no matter what, given it may be watched by other parts of the
> implementation, useful for debug info, and possibly useful if
> later in the start sequence the clk_ops get changed to have this
> field.~

Checking for the ops first allows us to skip the mutex acquire, but I'm happy 
either way.

> > +~	mutex_lock(&clk->mutex);
> > +
> > +	if (!--clk->enable_count)
> > +		clk->ops->disable(clk);
> > +
> > +	mutex_unlock(&clk->mutex);
> > +}
> > +
> > +static inline unsigned long clk_get_rate(struct clk *clk)
> > +{
> > +	if (clk->ops->get_rate)
> > +		return clk->ops->get_rate(clk);
> > +	return 0;
> > +}
> > +
> > +static inline void clk_put(struct clk *clk)
> > +{
> > +	if (clk->ops->put)
> > +		clk->ops->put(clk);
> > +}
> 
> I'm beginging to wonder if we don't just have a set of default ops
> that get set into the clk+ops at registration time if these do
> not have an implementation.

Using default ops would mean a couple of things:

1) we can no longer mark the real ops as const; and
2) we can no longer avoid the hard-to-predict indirect branch

> this means that either any driver that is using a multi-parent clock
> has to deal with the proper enable/disable of the parents (this is
> is going to lead to code repetition, and I bet people will get it
> badly wrong).
> 
> I belive the core of the clock code should deal with this, since
> otherwise we end up with the situation of the same code being
> repeated throughout the kernel.

I see the concern here, but I'll defer this one until we've decided on whether 
or not the parents should be exposed through the API.

Cheers,


Jeremy

Re: [RFC,PATCH 1/2] Add a common struct clk

[Lothar Waßmann]

Hi,

> > > +static inline int clk_enable(struct clk *clk)
> > > +{
> > > +	int ret = 0;
> > > +
> > > +	if (!clk->ops->enable)
> > > +		return 0;
> > > +
> > > +	mutex_lock(&clk->mutex);
> > > +	if (!clk->enable_count)
> > > +		ret = clk->ops->enable(clk);
> > > +
> > > +	if (!ret)
> > > +		clk->enable_count++;
> > > +	mutex_unlock(&clk->mutex);
> > > +
> > > +	return ret;
> > > +}
> > 
Using a mutex in clk_enable()/clk_disable() is a bad idea, since that
makes it impossible to call those functions in interrupt context.


Lothar Waßmann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info@karo-electronics.de
___________________________________________________________

[RFC,PATCH 1/2] Add a common struct clk

[Lothar Waßmann]

Hi,

> > > +static inline int clk_enable(struct clk *clk)
> > > +{
> > > +	int ret = 0;
> > > +
> > > +	if (!clk->ops->enable)
> > > +		return 0;
> > > +
> > > +	mutex_lock(&clk->mutex);
> > > +	if (!clk->enable_count)
> > > +		ret = clk->ops->enable(clk);
> > > +
> > > +	if (!ret)
> > > +		clk->enable_count++;
> > > +	mutex_unlock(&clk->mutex);
> > > +
> > > +	return ret;
> > > +}
> > 
Using a mutex in clk_enable()/clk_disable() is a bad idea, since that
makes it impossible to call those functions in interrupt context.


Lothar Wa?mann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Gesch?ftsf?hrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________

Re: [RFC,PATCH 1/2] Add a common struct clk

[Jeremy Kerr]

Hi Lothar,

> Using a mutex in clk_enable()/clk_disable() is a bad idea, since that
> makes it impossible to call those functions in interrupt context.

Do we do this at the moment? I know at least one implementation of clk_enable 
uses a mutex for locking.

Cheers,


Jeremy

[RFC,PATCH 1/2] Add a common struct clk

[Jeremy Kerr]

Hi Lothar,

> Using a mutex in clk_enable()/clk_disable() is a bad idea, since that
> makes it impossible to call those functions in interrupt context.

Do we do this at the moment? I know at least one implementation of clk_enable 
uses a mutex for locking.

Cheers,


Jeremy

Re: [RFC,PATCH 1/2] Add a common struct clk

[Lothar Waßmann]

Hi,

> > Using a mutex in clk_enable()/clk_disable() is a bad idea, since that
> > makes it impossible to call those functions in interrupt context.
> 
> Do we do this at the moment? I know at least one implementation of clk_enable 
> uses a mutex for locking.
> 
You are probably talking about the Freescale i.MX51 kernel, that won't
even boot, if you enable CONFIG_DEBUG_KERNEL, CONFIG_DEBUG_SPINLOCK,
CONFIG_DEBUG_LOCKDEP and CONFIG_DEBUG_SPINLOCK_SLEEP.
The mutex in the clock implementation is one of the reasons.


Lothar Waßmann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info@karo-electronics.de
___________________________________________________________

[RFC,PATCH 1/2] Add a common struct clk

[Lothar Waßmann]

Hi,

> > Using a mutex in clk_enable()/clk_disable() is a bad idea, since that
> > makes it impossible to call those functions in interrupt context.
> 
> Do we do this at the moment? I know at least one implementation of clk_enable 
> uses a mutex for locking.
> 
You are probably talking about the Freescale i.MX51 kernel, that won't
even boot, if you enable CONFIG_DEBUG_KERNEL, CONFIG_DEBUG_SPINLOCK,
CONFIG_DEBUG_LOCKDEP and CONFIG_DEBUG_SPINLOCK_SLEEP.
The mutex in the clock implementation is one of the reasons.


Lothar Wa?mann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Gesch?ftsf?hrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________

Re: [RFC,PATCH 1/2] Add a common struct clk

[Uwe Kleine-König]

On Fri, Jun 11, 2010 at 11:23:56AM +0200, Lothar Waßmann wrote:
> Hi,
> 
> > > Using a mutex in clk_enable()/clk_disable() is a bad idea, since that
> > > makes it impossible to call those functions in interrupt context.
IMHO if a device generates an irq its clock should already be on.  This
way you don't need to enable or disable a clock in irq context.
 
> > Do we do this at the moment? I know at least one implementation of clk_enable 
> > uses a mutex for locking.
> > 
> You are probably talking about the Freescale i.MX51 kernel, that won't
> even boot, if you enable CONFIG_DEBUG_KERNEL, CONFIG_DEBUG_SPINLOCK,
> CONFIG_DEBUG_LOCKDEP and CONFIG_DEBUG_SPINLOCK_SLEEP.
> The mutex in the clock implementation is one of the reasons.
I will have a look into this later today.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[RFC,PATCH 1/2] Add a common struct clk

[Uwe Kleine-König]

On Fri, Jun 11, 2010 at 11:23:56AM +0200, Lothar Wa?mann wrote:
> Hi,
> 
> > > Using a mutex in clk_enable()/clk_disable() is a bad idea, since that
> > > makes it impossible to call those functions in interrupt context.
IMHO if a device generates an irq its clock should already be on.  This
way you don't need to enable or disable a clock in irq context.
 
> > Do we do this at the moment? I know at least one implementation of clk_enable 
> > uses a mutex for locking.
> > 
> You are probably talking about the Freescale i.MX51 kernel, that won't
> even boot, if you enable CONFIG_DEBUG_KERNEL, CONFIG_DEBUG_SPINLOCK,
> CONFIG_DEBUG_LOCKDEP and CONFIG_DEBUG_SPINLOCK_SLEEP.
> The mutex in the clock implementation is one of the reasons.
I will have a look into this later today.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [RFC,PATCH 1/2] Add a common struct clk

[Lothar Waßmann]

Hi,

> > > > Using a mutex in clk_enable()/clk_disable() is a bad idea, since that
> > > > makes it impossible to call those functions in interrupt context.
> IMHO if a device generates an irq its clock should already be on.  This
> way you don't need to enable or disable a clock in irq context.
>  
You may want to disable a clock in the IRQ handler. The VPU driver in
the Freescale BSP for i.MX51 does exactly this.
Anyway I don't see any reason for using a mutex here instead of
spin_lock_irq_save() as all other implementations do.


Lothar Waßmann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info@karo-electronics.de
___________________________________________________________

[RFC,PATCH 1/2] Add a common struct clk

[Lothar Waßmann]

Hi,

> > > > Using a mutex in clk_enable()/clk_disable() is a bad idea, since that
> > > > makes it impossible to call those functions in interrupt context.
> IMHO if a device generates an irq its clock should already be on.  This
> way you don't need to enable or disable a clock in irq context.
>  
You may want to disable a clock in the IRQ handler. The VPU driver in
the Freescale BSP for i.MX51 does exactly this.
Anyway I don't see any reason for using a mutex here instead of
spin_lock_irq_save() as all other implementations do.


Lothar Wa?mann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Gesch?ftsf?hrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________

Re: [RFC,PATCH 1/2] Add a common struct clk

[Jeremy Kerr]

Hi Lothar,

> You may want to disable a clock in the IRQ handler. The VPU driver in
> the Freescale BSP for i.MX51 does exactly this.

Is this something we can defer to a sleepable context?

> Anyway I don't see any reason for using a mutex here instead of
> spin_lock_irq_save() as all other implementations do.

Just that enable/disable may take an arbitrarily long time to complete. Sounds 
like that hasn't been a problem in the past though.

Cheers,


Jeremy

[RFC,PATCH 1/2] Add a common struct clk

[Jeremy Kerr]

Hi Lothar,

> You may want to disable a clock in the IRQ handler. The VPU driver in
> the Freescale BSP for i.MX51 does exactly this.

Is this something we can defer to a sleepable context?

> Anyway I don't see any reason for using a mutex here instead of
> spin_lock_irq_save() as all other implementations do.

Just that enable/disable may take an arbitrarily long time to complete. Sounds 
like that hasn't been a problem in the past though.

Cheers,


Jeremy

Re: [RFC,PATCH 1/2] Add a common struct clk

[Uwe Kleine-König]

Hello Lothar,

On Fri, Jun 11, 2010 at 11:58:39AM +0200, Uwe Kleine-König wrote:
> > You are probably talking about the Freescale i.MX51 kernel, that won't
> > even boot, if you enable CONFIG_DEBUG_KERNEL, CONFIG_DEBUG_SPINLOCK,
> > CONFIG_DEBUG_LOCKDEP and CONFIG_DEBUG_SPINLOCK_SLEEP.
> > The mutex in the clock implementation is one of the reasons.
> I will have a look into this later today.
I take this back as this is not a mainline problem.
 
Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[RFC,PATCH 1/2] Add a common struct clk

[Uwe Kleine-König]

Hello Lothar,

On Fri, Jun 11, 2010 at 11:58:39AM +0200, Uwe Kleine-K?nig wrote:
> > You are probably talking about the Freescale i.MX51 kernel, that won't
> > even boot, if you enable CONFIG_DEBUG_KERNEL, CONFIG_DEBUG_SPINLOCK,
> > CONFIG_DEBUG_LOCKDEP and CONFIG_DEBUG_SPINLOCK_SLEEP.
> > The mutex in the clock implementation is one of the reasons.
> I will have a look into this later today.
I take this back as this is not a mainline problem.
 
Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [RFC,PATCH 1/2] Add a common struct clk

[Benjamin Herrenschmidt]

> Using a mutex in clk_enable()/clk_disable() is a bad idea, since that
> makes it impossible to call those functions in interrupt context.

And doing clk_enable/clk_disable from interrupt context is a bad idea as
well :-)

Cheers,
Ben.

[RFC,PATCH 1/2] Add a common struct clk

[Benjamin Herrenschmidt]

> Using a mutex in clk_enable()/clk_disable() is a bad idea, since that
> makes it impossible to call those functions in interrupt context.

And doing clk_enable/clk_disable from interrupt context is a bad idea as
well :-)

Cheers,
Ben.

Re: [RFC,PATCH 1/2] Add a common struct clk

[Benjamin Herrenschmidt]

On Fri, 2010-06-11 at 11:23 +0200, Lothar Waßmann wrote:
> Hi,
> 
> > > Using a mutex in clk_enable()/clk_disable() is a bad idea, since that
> > > makes it impossible to call those functions in interrupt context.
> > 
> > Do we do this at the moment? I know at least one implementation of clk_enable 
> > uses a mutex for locking.
> > 
> You are probably talking about the Freescale i.MX51 kernel, that won't
> even boot, if you enable CONFIG_DEBUG_KERNEL, CONFIG_DEBUG_SPINLOCK,
> CONFIG_DEBUG_LOCKDEP and CONFIG_DEBUG_SPINLOCK_SLEEP.
> The mutex in the clock implementation is one of the reasons.

Regardless. Clocks generally take time to enable. I don't believe doing
clock enable/disable at hard irq context is a great idea.

If you really want to do something like that, you can always use either
threaded interrupts, or if you know your clock is off, mask & defer your
handling to a work queue yourself.

Unless we have enough case of very fast switching clocks that would
really benefit for that but from my experience, when a device can issue
interrupts, it should have its clocks on, unless it's some kind of
"wakeup" interrupt in which case it can safely be delayed.

Cheers,
Ben.

[RFC,PATCH 1/2] Add a common struct clk

[Benjamin Herrenschmidt]

On Fri, 2010-06-11 at 11:23 +0200, Lothar Wa?mann wrote:
> Hi,
> 
> > > Using a mutex in clk_enable()/clk_disable() is a bad idea, since that
> > > makes it impossible to call those functions in interrupt context.
> > 
> > Do we do this at the moment? I know at least one implementation of clk_enable 
> > uses a mutex for locking.
> > 
> You are probably talking about the Freescale i.MX51 kernel, that won't
> even boot, if you enable CONFIG_DEBUG_KERNEL, CONFIG_DEBUG_SPINLOCK,
> CONFIG_DEBUG_LOCKDEP and CONFIG_DEBUG_SPINLOCK_SLEEP.
> The mutex in the clock implementation is one of the reasons.

Regardless. Clocks generally take time to enable. I don't believe doing
clock enable/disable at hard irq context is a great idea.

If you really want to do something like that, you can always use either
threaded interrupts, or if you know your clock is off, mask & defer your
handling to a work queue yourself.

Unless we have enough case of very fast switching clocks that would
really benefit for that but from my experience, when a device can issue
interrupts, it should have its clocks on, unless it's some kind of
"wakeup" interrupt in which case it can safely be delayed.

Cheers,
Ben.

Re: [RFC,PATCH 1/2] Add a common struct clk

[Benjamin Herrenschmidt]

On Fri, 2010-06-11 at 12:08 +0200, Lothar Waßmann wrote:
> Hi,
> 
> > > > > Using a mutex in clk_enable()/clk_disable() is a bad idea, since that
> > > > > makes it impossible to call those functions in interrupt context.
> > IMHO if a device generates an irq its clock should already be on.  This
> > way you don't need to enable or disable a clock in irq context.
> >  
> You may want to disable a clock in the IRQ handler. The VPU driver in
> the Freescale BSP for i.MX51 does exactly this.
> Anyway I don't see any reason for using a mutex here instead of
> spin_lock_irq_save() as all other implementations do.

Because you suddenly make it impossible to sleep inside enable/disable
unless I'm mistaken about the implementation details. Some PLLs can need
milliseconds to stabilize (especially if they need to be powered up
first). Doing that with a lock held is a BAD IDEA.

Cheers,
Ben.

[RFC,PATCH 1/2] Add a common struct clk

[Benjamin Herrenschmidt]

On Fri, 2010-06-11 at 12:08 +0200, Lothar Wa?mann wrote:
> Hi,
> 
> > > > > Using a mutex in clk_enable()/clk_disable() is a bad idea, since that
> > > > > makes it impossible to call those functions in interrupt context.
> > IMHO if a device generates an irq its clock should already be on.  This
> > way you don't need to enable or disable a clock in irq context.
> >  
> You may want to disable a clock in the IRQ handler. The VPU driver in
> the Freescale BSP for i.MX51 does exactly this.
> Anyway I don't see any reason for using a mutex here instead of
> spin_lock_irq_save() as all other implementations do.

Because you suddenly make it impossible to sleep inside enable/disable
unless I'm mistaken about the implementation details. Some PLLs can need
milliseconds to stabilize (especially if they need to be powered up
first). Doing that with a lock held is a BAD IDEA.

Cheers,
Ben.

Re: [RFC,PATCH 1/2] Add a common struct clk

[Ben Dooks]

On Fri, Jun 11, 2010 at 03:57:10PM +0800, Jeremy Kerr wrote:
> Hi Ben,
> 
> > > Platforms can enable the generic struct clock through
> > > CONFIG_USE_COMMON_STRUCT_CLK. In this case, the clock infrastructure
> > > consists of a common struct clk:
> >
> > What do people think of just changing everyone who is currently using
> > clk support to using this new implementation?
> 
> I think it'd be too-big a task to take on for a single change; we'd be better 
> off allowing the two implementations concurrently. Also, I'd say that there 
> are many platforms that will never need the new code, so no need to change 
> them over.
~
Ok, agreed on that.
 
> > > struct clk {
> > > 
> > > 	const struct clk_ops	*ops;
> > > 	unsigned int		enable_count;
> > > 	struct mutex		mutex;
> > 
> > I'm a little worried about the size of struct clk if all of them
> > have a mutex in them. If i'm right, this is 40 bytes of data each
> > clock has to take on board.
> 
> Yeah, I see your concern about adding a mutex per clock, but I do think it's 
> the best solution. Of course, if there's a better way to do it, I'm happy to 
> try it out.

You also need a warning that even if it protects the clock, it may not
protect any access to the hardware implementing it.
 
> I believe we need to ensure that clocks are enabled when clk_enable returns, 
> so we'll need some mechanism for waiting on the thread doing the 
> enable/disable. Since (as you say) some clocks may take 100s of microseconds 
> to enable, we'll need a lock that we can hold while sleeping.

Well, mutexes give us that, whilst enabling we hold the mutex.
 
> We could use a global mutex for all clocks, but that would complicate the 
> interface by requiring non-locking versions of enable/disable, to allow 
> recursive enable/disable of parent clocks. Also, we'd lose any parallelism 
> possible in clock initialisation (we want boot to be fast, right?)

More to the point, we need it to _work_ fast. It isn't just boot time.
 
> Without all the debug stuff, it looks like a mutex is 12 bytes on UP, 20 on 
> SMP. That brings the maximum possible overhead to 26kB/43kB. More tolerable, 
> but not great.
>
> You know what would be awesome? if we could put platform-specific code in 
> their own sections and the discard the ones we know we won't need after boot. 
> But that's a discussion for another day...
> 
> > >        int             (*enable)(struct clk *);
> > 
> > still think that not passing an bool as a second argument is silly.
> 
> Still don't :D

Pfft.

> > > +struct clk {
> > > +	const struct clk_ops	*ops;
> > > +	unsigned int		enable_count;
> > > +	struct mutex		mutex;
> > > +};
> > 
> > how about defining a nice kerneldoc for this.
> 
> Shall do. I've also decided to un-inline the base API, which means that I can 
> use all of the existing prototypes in clk.h, which means the kerneldoc is 
> shared too.

Ok, I was going to suggest that anyway.

> 
> > > +#define INIT_CLK(name, o) \
> > > +	{ .ops = &o, .enable_count = 0, \
> > > +		.mutex = __MUTEX_INITIALIZER(name.mutex) }
> > 
> > how about doing the mutex initinitialisation at registration
> > time, will save a pile of non-zero code in the image to mess up
> > the compression.
> 
> I've just yesterday added the following to my tree, to allow dynamic 
> initialisation:
> 
> static inline void clk_init(struct clk *clk, const struct clk_ops *ops)
> {
> 	clk->ops = ops;
> 	clk->enable_count = 0;
> 	mutex_init(&clk->mutex);
> }
> 
> So we can do this either way.

the above is in my view better.
 
> > ~> +struct clk_ops {
> > 
> > > +       int             (*enable)(struct clk *);
> > > +       void            (*disable)(struct clk *);
> > > +       unsigned long   (*get_rate)(struct clk *);
> > > +       void            (*put)(struct clk *);
> > > +       long            (*round_rate)(struct clk *, unsigned long);
> > > +       int             (*set_rate)(struct clk *, unsigned long);
> > > +       int             (*set_parent)(struct clk *, struct clk *);
> > > +       struct clk*     (*get_parent)(struct clk *);
> > 
> > should each clock carry a parent field and the this is returned by
> > the get parent call.~~
> 
> I've been debating dropping the get_parent and set_parent ops entirely, 
> actually; setting a parent seems to be quite specific to hardware (you have to 
> know that a particular clock can be a parent of another clock), so it seems 
> like something that we shouldn't expose to drivers through this API. For the 
> code that knows the hardware, it can probably access the underlying clock 
> types directly.
>

Not really, and it is in use with extant drivers, so not easily
removable either.

> However, I see that something in the samsung code uses clk_get_parent for what 
> seems like a sane purpose, so have left them in for now. Or could this be done 
> differently?

nope, and I really like this method.
 
> > > +static inline int clk_enable(struct clk *clk)
> > > +{
> > > +	int ret = 0;
> > > +
> > > +	if (!clk->ops->enable)
> > > +		return 0;
> > > +
> > > +	mutex_lock(&clk->mutex);
> > > +	if (!clk->enable_count)
> > > +		ret = clk->ops->enable(clk);
> > > +
> > > +	if (!ret)
> > > +		clk->enable_count++;
> > > +	mutex_unlock(&clk->mutex);
> > > +
> > > +	return ret;
> > > +}
> > 
> > So we're leaving the enable parent code now to each implementation?
> > 
> > I think this is a really bad decision, it leaves so much open to bad
> > code repetition, as well as something the core should really be doing
> > if it had a parent clock field.
> 
> I'm tempted to go in either direction - either no [sg]et_parent in the clk 
> API, or add a parent to struct clk, but first I'd need to get up to speed on 
> the external uses of the clk's parent. Want to catch me on IRC to discuss?

Possibly, still screwed on timezone.
 
> > ~> +static inline void clk_disable(struct clk *clk)
> > 
> > > +{
> > > +	if (!clk->ops->enable)
> > > +		return;
> > 
> > so if we've no enable call we ignore disable too?
> 
> Typo; this should be "if (!clk->ops->disable)". Will fix for the next 
> revision.

yes, pointing that out.
 
> > also, we don't keep an enable count if this fields are in use,
> > could people rely on this being correct even if the clock has
> > no enable/disable fields.
> > 
> > Would much rather see the enable_count being kept up-to-date
> > no matter what, given it may be watched by other parts of the
> > implementation, useful for debug info, and possibly useful if
> > later in the start sequence the clk_ops get changed to have this
> > field.~
> 
> Checking for the ops first allows us to skip the mutex acquire, but I'm happy 
> either way.

erm, sorry, yes, you can check for them before mutex. any chages
should be done with mutex held.
 
> > > +~	mutex_lock(&clk->mutex);
> > > +
> > > +	if (!--clk->enable_count)
> > > +		clk->ops->disable(clk);
> > > +
> > > +	mutex_unlock(&clk->mutex);
> > > +}
> > > +
> > > +static inline unsigned long clk_get_rate(struct clk *clk)
> > > +{
> > > +	if (clk->ops->get_rate)
> > > +		return clk->ops->get_rate(clk);
> > > +	return 0;
> > > +}
> > > +
> > > +static inline void clk_put(struct clk *clk)
> > > +{
> > > +	if (clk->ops->put)
> > > +		clk->ops->put(clk);
> > > +}
> > 
> > I'm beginging to wonder if we don't just have a set of default ops
> > that get set into the clk+ops at registration time if these do
> > not have an implementation.
> 
> Using default ops would mean a couple of things:
> 
> 1) we can no longer mark the real ops as const; and
> 2) we can no longer avoid the hard-to-predict indirect branch

ok, how about people have to mark these as a default non op in their
clock structure, and then error if they try and register a clock with
null ops. anyone changing these to NULL later deserves all the pain and agony
they get.
 
> > this means that either any driver that is using a multi-parent clock
> > has to deal with the proper enable/disable of the parents (this is
> > is going to lead to code repetition, and I bet people will get it
> > badly wrong).
> > 
> > I belive the core of the clock code should deal with this, since
> > otherwise we end up with the situation of the same code being
> > repeated throughout the kernel.
> 
> I see the concern here, but I'll defer this one until we've decided on whether 
> or not the parents should be exposed through the API.

Ok, let's find out what other people think too. I'm not the only user.

BTW, we do need to do some cleanups to the samsung code, we've a few
places where we explicitly name clocks. Discussion for seperate
thread.

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

  'a smiley only costs 4 bytes'

[RFC,PATCH 1/2] Add a common struct clk

[Ben Dooks]

On Fri, Jun 11, 2010 at 03:57:10PM +0800, Jeremy Kerr wrote:
> Hi Ben,
> 
> > > Platforms can enable the generic struct clock through
> > > CONFIG_USE_COMMON_STRUCT_CLK. In this case, the clock infrastructure
> > > consists of a common struct clk:
> >
> > What do people think of just changing everyone who is currently using
> > clk support to using this new implementation?
> 
> I think it'd be too-big a task to take on for a single change; we'd be better 
> off allowing the two implementations concurrently. Also, I'd say that there 
> are many platforms that will never need the new code, so no need to change 
> them over.
~
Ok, agreed on that.
 
> > > struct clk {
> > > 
> > > 	const struct clk_ops	*ops;
> > > 	unsigned int		enable_count;
> > > 	struct mutex		mutex;
> > 
> > I'm a little worried about the size of struct clk if all of them
> > have a mutex in them. If i'm right, this is 40 bytes of data each
> > clock has to take on board.
> 
> Yeah, I see your concern about adding a mutex per clock, but I do think it's 
> the best solution. Of course, if there's a better way to do it, I'm happy to 
> try it out.

You also need a warning that even if it protects the clock, it may not
protect any access to the hardware implementing it.
 
> I believe we need to ensure that clocks are enabled when clk_enable returns, 
> so we'll need some mechanism for waiting on the thread doing the 
> enable/disable. Since (as you say) some clocks may take 100s of microseconds 
> to enable, we'll need a lock that we can hold while sleeping.

Well, mutexes give us that, whilst enabling we hold the mutex.
 
> We could use a global mutex for all clocks, but that would complicate the 
> interface by requiring non-locking versions of enable/disable, to allow 
> recursive enable/disable of parent clocks. Also, we'd lose any parallelism 
> possible in clock initialisation (we want boot to be fast, right?)

More to the point, we need it to _work_ fast. It isn't just boot time.
 
> Without all the debug stuff, it looks like a mutex is 12 bytes on UP, 20 on 
> SMP. That brings the maximum possible overhead to 26kB/43kB. More tolerable, 
> but not great.
>
> You know what would be awesome? if we could put platform-specific code in 
> their own sections and the discard the ones we know we won't need after boot. 
> But that's a discussion for another day...
> 
> > >        int             (*enable)(struct clk *);
> > 
> > still think that not passing an bool as a second argument is silly.
> 
> Still don't :D

Pfft.

> > > +struct clk {
> > > +	const struct clk_ops	*ops;
> > > +	unsigned int		enable_count;
> > > +	struct mutex		mutex;
> > > +};
> > 
> > how about defining a nice kerneldoc for this.
> 
> Shall do. I've also decided to un-inline the base API, which means that I can 
> use all of the existing prototypes in clk.h, which means the kerneldoc is 
> shared too.

Ok, I was going to suggest that anyway.

> 
> > > +#define INIT_CLK(name, o) \
> > > +	{ .ops = &o, .enable_count = 0, \
> > > +		.mutex = __MUTEX_INITIALIZER(name.mutex) }
> > 
> > how about doing the mutex initinitialisation at registration
> > time, will save a pile of non-zero code in the image to mess up
> > the compression.
> 
> I've just yesterday added the following to my tree, to allow dynamic 
> initialisation:
> 
> static inline void clk_init(struct clk *clk, const struct clk_ops *ops)
> {
> 	clk->ops = ops;
> 	clk->enable_count = 0;
> 	mutex_init(&clk->mutex);
> }
> 
> So we can do this either way.

the above is in my view better.
 
> > ~> +struct clk_ops {
> > 
> > > +       int             (*enable)(struct clk *);
> > > +       void            (*disable)(struct clk *);
> > > +       unsigned long   (*get_rate)(struct clk *);
> > > +       void            (*put)(struct clk *);
> > > +       long            (*round_rate)(struct clk *, unsigned long);
> > > +       int             (*set_rate)(struct clk *, unsigned long);
> > > +       int             (*set_parent)(struct clk *, struct clk *);
> > > +       struct clk*     (*get_parent)(struct clk *);
> > 
> > should each clock carry a parent field and the this is returned by
> > the get parent call.~~
> 
> I've been debating dropping the get_parent and set_parent ops entirely, 
> actually; setting a parent seems to be quite specific to hardware (you have to 
> know that a particular clock can be a parent of another clock), so it seems 
> like something that we shouldn't expose to drivers through this API. For the 
> code that knows the hardware, it can probably access the underlying clock 
> types directly.
>

Not really, and it is in use with extant drivers, so not easily
removable either.

> However, I see that something in the samsung code uses clk_get_parent for what 
> seems like a sane purpose, so have left them in for now. Or could this be done 
> differently?

nope, and I really like this method.
 
> > > +static inline int clk_enable(struct clk *clk)
> > > +{
> > > +	int ret = 0;
> > > +
> > > +	if (!clk->ops->enable)
> > > +		return 0;
> > > +
> > > +	mutex_lock(&clk->mutex);
> > > +	if (!clk->enable_count)
> > > +		ret = clk->ops->enable(clk);
> > > +
> > > +	if (!ret)
> > > +		clk->enable_count++;
> > > +	mutex_unlock(&clk->mutex);
> > > +
> > > +	return ret;
> > > +}
> > 
> > So we're leaving the enable parent code now to each implementation?
> > 
> > I think this is a really bad decision, it leaves so much open to bad
> > code repetition, as well as something the core should really be doing
> > if it had a parent clock field.
> 
> I'm tempted to go in either direction - either no [sg]et_parent in the clk 
> API, or add a parent to struct clk, but first I'd need to get up to speed on 
> the external uses of the clk's parent. Want to catch me on IRC to discuss?

Possibly, still screwed on timezone.
 
> > ~> +static inline void clk_disable(struct clk *clk)
> > 
> > > +{
> > > +	if (!clk->ops->enable)
> > > +		return;
> > 
> > so if we've no enable call we ignore disable too?
> 
> Typo; this should be "if (!clk->ops->disable)". Will fix for the next 
> revision.

yes, pointing that out.
 
> > also, we don't keep an enable count if this fields are in use,
> > could people rely on this being correct even if the clock has
> > no enable/disable fields.
> > 
> > Would much rather see the enable_count being kept up-to-date
> > no matter what, given it may be watched by other parts of the
> > implementation, useful for debug info, and possibly useful if
> > later in the start sequence the clk_ops get changed to have this
> > field.~
> 
> Checking for the ops first allows us to skip the mutex acquire, but I'm happy 
> either way.

erm, sorry, yes, you can check for them before mutex. any chages
should be done with mutex held.
 
> > > +~	mutex_lock(&clk->mutex);
> > > +
> > > +	if (!--clk->enable_count)
> > > +		clk->ops->disable(clk);
> > > +
> > > +	mutex_unlock(&clk->mutex);
> > > +}
> > > +
> > > +static inline unsigned long clk_get_rate(struct clk *clk)
> > > +{
> > > +	if (clk->ops->get_rate)
> > > +		return clk->ops->get_rate(clk);
> > > +	return 0;
> > > +}
> > > +
> > > +static inline void clk_put(struct clk *clk)
> > > +{
> > > +	if (clk->ops->put)
> > > +		clk->ops->put(clk);
> > > +}
> > 
> > I'm beginging to wonder if we don't just have a set of default ops
> > that get set into the clk+ops at registration time if these do
> > not have an implementation.
> 
> Using default ops would mean a couple of things:
> 
> 1) we can no longer mark the real ops as const; and
> 2) we can no longer avoid the hard-to-predict indirect branch

ok, how about people have to mark these as a default non op in their
clock structure, and then error if they try and register a clock with
null ops. anyone changing these to NULL later deserves all the pain and agony
they get.
 
> > this means that either any driver that is using a multi-parent clock
> > has to deal with the proper enable/disable of the parents (this is
> > is going to lead to code repetition, and I bet people will get it
> > badly wrong).
> > 
> > I belive the core of the clock code should deal with this, since
> > otherwise we end up with the situation of the same code being
> > repeated throughout the kernel.
> 
> I see the concern here, but I'll defer this one until we've decided on whether 
> or not the parents should be exposed through the API.

Ok, let's find out what other people think too. I'm not the only user.

BTW, we do need to do some cleanups to the samsung code, we've a few
places where we explicitly name clocks. Discussion for seperate
thread.

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

  'a smiley only costs 4 bytes'

Re: [RFC,PATCH 1/2] Add a common struct clk

[Ben Dooks]

On Fri, Jun 11, 2010 at 10:14:35AM +0200, Lothar Wa?mann wrote:
> Hi,
> 
> > > > +static inline int clk_enable(struct clk *clk)
> > > > +{
> > > > +	int ret = 0;
> > > > +
> > > > +	if (!clk->ops->enable)
> > > > +		return 0;
> > > > +
> > > > +	mutex_lock(&clk->mutex);
> > > > +	if (!clk->enable_count)
> > > > +		ret = clk->ops->enable(clk);
> > > > +
> > > > +	if (!ret)
> > > > +		clk->enable_count++;
> > > > +	mutex_unlock(&clk->mutex);
> > > > +
> > > > +	return ret;
> > > > +}
> > > 
> Using a mutex in clk_enable()/clk_disable() is a bad idea, since that
> makes it impossible to call those functions in interrupt context.

I think that is a bad idea, unless you can provide otherwise. These
calls can sleep depending on implementation, and thus I would like to
ensure that they are marked as might-sleep.

Is there any specific reason? If so, we need to add some form of ops
where we have _nosleep specificially for this case.

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

  'a smiley only costs 4 bytes'

[RFC,PATCH 1/2] Add a common struct clk

[Ben Dooks]

On Fri, Jun 11, 2010 at 10:14:35AM +0200, Lothar Wa?mann wrote:
> Hi,
> 
> > > > +static inline int clk_enable(struct clk *clk)
> > > > +{
> > > > +	int ret = 0;
> > > > +
> > > > +	if (!clk->ops->enable)
> > > > +		return 0;
> > > > +
> > > > +	mutex_lock(&clk->mutex);
> > > > +	if (!clk->enable_count)
> > > > +		ret = clk->ops->enable(clk);
> > > > +
> > > > +	if (!ret)
> > > > +		clk->enable_count++;
> > > > +	mutex_unlock(&clk->mutex);
> > > > +
> > > > +	return ret;
> > > > +}
> > > 
> Using a mutex in clk_enable()/clk_disable() is a bad idea, since that
> makes it impossible to call those functions in interrupt context.

I think that is a bad idea, unless you can provide otherwise. These
calls can sleep depending on implementation, and thus I would like to
ensure that they are marked as might-sleep.

Is there any specific reason? If so, we need to add some form of ops
where we have _nosleep specificially for this case.

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

  'a smiley only costs 4 bytes'

Re: [RFC,PATCH 1/2] Add a common struct clk

[Ben Dooks]

On Fri, Jun 11, 2010 at 12:08:01PM +0200, Lothar Wa?mann wrote:
> Hi,
> 
> > > > > Using a mutex in clk_enable()/clk_disable() is a bad idea, since that
> > > > > makes it impossible to call those functions in interrupt context.
> > IMHO if a device generates an irq its clock should already be on.  This
> > way you don't need to enable or disable a clock in irq context.
> >  
> You may want to disable a clock in the IRQ handler. The VPU driver in
> the Freescale BSP for i.MX51 does exactly this.
> Anyway I don't see any reason for using a mutex here instead of
> spin_lock_irq_save() as all other implementations do.

Hmm, then again the VPU driver may just be a bit wrong here.

We could protect each clock with a spinlock, but that would end up
with a problem of spinning where we have clocks that takes 100s of
usec or so to init. See all PLLs on S3C devices, where it can take
100-300uS to get a stable clock out of the device.

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

  'a smiley only costs 4 bytes'

[RFC,PATCH 1/2] Add a common struct clk

[Ben Dooks]

On Fri, Jun 11, 2010 at 12:08:01PM +0200, Lothar Wa?mann wrote:
> Hi,
> 
> > > > > Using a mutex in clk_enable()/clk_disable() is a bad idea, since that
> > > > > makes it impossible to call those functions in interrupt context.
> > IMHO if a device generates an irq its clock should already be on.  This
> > way you don't need to enable or disable a clock in irq context.
> >  
> You may want to disable a clock in the IRQ handler. The VPU driver in
> the Freescale BSP for i.MX51 does exactly this.
> Anyway I don't see any reason for using a mutex here instead of
> spin_lock_irq_save() as all other implementations do.

Hmm, then again the VPU driver may just be a bit wrong here.

We could protect each clock with a spinlock, but that would end up
with a problem of spinning where we have clocks that takes 100s of
usec or so to init. See all PLLs on S3C devices, where it can take
100-300uS to get a stable clock out of the device.

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

  'a smiley only costs 4 bytes'

Re: [RFC,PATCH 1/2] Add a common struct clk

[Jeremy Kerr]

Hi Ben,

> You also need a warning that even if it protects the clock, it may not
> protect any access to the hardware implementing it.

Yep, agreed. HW clock implementations are free to acquire the mutex in their 
ops.

> > I believe we need to ensure that clocks are enabled when clk_enable
> > returns, so we'll need some mechanism for waiting on the thread doing
> > the enable/disable. Since (as you say) some clocks may take 100s of
> > microseconds to enable, we'll need a lock that we can hold while
> > sleeping.
> 
> Well, mutexes give us that, whilst enabling we hold the mutex.

Exactly, that's why I think the mutex option is the best way to go.

> > I've just yesterday added the following to my tree, to allow dynamic
> > initialisation:
> > 
> > static inline void clk_init(struct clk *clk, const struct clk_ops *ops)
> > {
> > 
> > 	clk->ops = ops;
> > 	clk->enable_count = 0;
> > 	mutex_init(&clk->mutex);
> > 
> > }
> > 
> > So we can do this either way.
> 
> the above is in my view better.

By 'the above' do you mean doing the mutex init at registration time, or the 
clk_init code above?

Either way should be fine; delaying the mutex_init until registration will has 
the nice property of not requiring the clock name to be passed to INIT_CLK.

> > I've been debating dropping the get_parent and set_parent ops entirely,
> > actually; setting a parent seems to be quite specific to hardware (you
> > have to know that a particular clock can be a parent of another clock),
> > so it seems like something that we shouldn't expose to drivers through
> > this API. For the code that knows the hardware, it can probably access
> > the underlying clock types directly.
> 
> Not really, and it is in use with extant drivers, so not easily
> removable either.

OK, is set_parent used much? I can see the use of get_parent, but calls 
set_parent need to know specifics of the clock hardware.

> > Checking for the ops first allows us to skip the mutex acquire, but I'm
> > happy either way.
> 
> erm, sorry, yes, you can check for them before mutex. any chages
> should be done with mutex held.

Yep.

> > Using default ops would mean a couple of things:
> > 
> > 1) we can no longer mark the real ops as const; and
> > 2) we can no longer avoid the hard-to-predict indirect branch
> 
> ok, how about people have to mark these as a default non op in their
> clock structure, and then error if they try and register a clock with
> null ops. anyone changing these to NULL later deserves all the pain and
> agony they get.

That addresses the first point, but still means we have an unnecessary 
indirect branch to a function that does nothing. Since I've unlined the code 
where this happens, the checks for null ops are pretty unobtrusive. If we 
require all ops to be not-null, then we'll need much larger chunks of code 
where the ops are defined. I like that you can just set the ops callbacks that 
you need, and the rest "just works".

Cheers,


Jeremy

[RFC,PATCH 1/2] Add a common struct clk

[Jeremy Kerr]

Hi Ben,

> You also need a warning that even if it protects the clock, it may not
> protect any access to the hardware implementing it.

Yep, agreed. HW clock implementations are free to acquire the mutex in their 
ops.

> > I believe we need to ensure that clocks are enabled when clk_enable
> > returns, so we'll need some mechanism for waiting on the thread doing
> > the enable/disable. Since (as you say) some clocks may take 100s of
> > microseconds to enable, we'll need a lock that we can hold while
> > sleeping.
> 
> Well, mutexes give us that, whilst enabling we hold the mutex.

Exactly, that's why I think the mutex option is the best way to go.

> > I've just yesterday added the following to my tree, to allow dynamic
> > initialisation:
> > 
> > static inline void clk_init(struct clk *clk, const struct clk_ops *ops)
> > {
> > 
> > 	clk->ops = ops;
> > 	clk->enable_count = 0;
> > 	mutex_init(&clk->mutex);
> > 
> > }
> > 
> > So we can do this either way.
> 
> the above is in my view better.

By 'the above' do you mean doing the mutex init at registration time, or the 
clk_init code above?

Either way should be fine; delaying the mutex_init until registration will has 
the nice property of not requiring the clock name to be passed to INIT_CLK.

> > I've been debating dropping the get_parent and set_parent ops entirely,
> > actually; setting a parent seems to be quite specific to hardware (you
> > have to know that a particular clock can be a parent of another clock),
> > so it seems like something that we shouldn't expose to drivers through
> > this API. For the code that knows the hardware, it can probably access
> > the underlying clock types directly.
> 
> Not really, and it is in use with extant drivers, so not easily
> removable either.

OK, is set_parent used much? I can see the use of get_parent, but calls 
set_parent need to know specifics of the clock hardware.

> > Checking for the ops first allows us to skip the mutex acquire, but I'm
> > happy either way.
> 
> erm, sorry, yes, you can check for them before mutex. any chages
> should be done with mutex held.

Yep.

> > Using default ops would mean a couple of things:
> > 
> > 1) we can no longer mark the real ops as const; and
> > 2) we can no longer avoid the hard-to-predict indirect branch
> 
> ok, how about people have to mark these as a default non op in their
> clock structure, and then error if they try and register a clock with
> null ops. anyone changing these to NULL later deserves all the pain and
> agony they get.

That addresses the first point, but still means we have an unnecessary 
indirect branch to a function that does nothing. Since I've unlined the code 
where this happens, the checks for null ops are pretty unobtrusive. If we 
require all ops to be not-null, then we'll need much larger chunks of code 
where the ops are defined. I like that you can just set the ops callbacks that 
you need, and the rest "just works".

Cheers,


Jeremy

Re: [RFC,PATCH 1/2] Add a common struct clk

[Lothar Waßmann]

Hi,

Benjamin Herrenschmidt writes:
> On Fri, 2010-06-11 at 12:08 +0200, Lothar Waßmann wrote:
> > Hi,
> > 
> > > > > > Using a mutex in clk_enable()/clk_disable() is a bad idea, since that
> > > > > > makes it impossible to call those functions in interrupt context.
> > > IMHO if a device generates an irq its clock should already be on.  This
> > > way you don't need to enable or disable a clock in irq context.
> > >  
> > You may want to disable a clock in the IRQ handler. The VPU driver in
> > the Freescale BSP for i.MX51 does exactly this.
> > Anyway I don't see any reason for using a mutex here instead of
> > spin_lock_irq_save() as all other implementations do.
> 
> Because you suddenly make it impossible to sleep inside enable/disable
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^
???
All implementations so far use spin_lock_irq_save()!

How would you be able to sleep with a mutex held?
If you hold a lock you must not sleep, no matter what sort of lock it
is.


Lothar Waßmann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info@karo-electronics.de
___________________________________________________________

[RFC,PATCH 1/2] Add a common struct clk

[Lothar Waßmann]

Hi,

Benjamin Herrenschmidt writes:
> On Fri, 2010-06-11 at 12:08 +0200, Lothar Wa?mann wrote:
> > Hi,
> > 
> > > > > > Using a mutex in clk_enable()/clk_disable() is a bad idea, since that
> > > > > > makes it impossible to call those functions in interrupt context.
> > > IMHO if a device generates an irq its clock should already be on.  This
> > > way you don't need to enable or disable a clock in irq context.
> > >  
> > You may want to disable a clock in the IRQ handler. The VPU driver in
> > the Freescale BSP for i.MX51 does exactly this.
> > Anyway I don't see any reason for using a mutex here instead of
> > spin_lock_irq_save() as all other implementations do.
> 
> Because you suddenly make it impossible to sleep inside enable/disable
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^
???
All implementations so far use spin_lock_irq_save()!

How would you be able to sleep with a mutex held?
If you hold a lock you must not sleep, no matter what sort of lock it
is.


Lothar Wa?mann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Gesch?ftsf?hrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________

Re: [RFC,PATCH 1/2] Add a common struct clk

[Uwe Kleine-König]

Hello Lothar,

On Mon, Jun 14, 2010 at 08:39:21AM +0200, Lothar Waßmann wrote:
> Hi,
> 
> Benjamin Herrenschmidt writes:
> > On Fri, 2010-06-11 at 12:08 +0200, Lothar Waßmann wrote:
> > > Hi,
> > > 
> > > > > > > Using a mutex in clk_enable()/clk_disable() is a bad idea, since that
> > > > > > > makes it impossible to call those functions in interrupt context.
> > > > IMHO if a device generates an irq its clock should already be on.  This
> > > > way you don't need to enable or disable a clock in irq context.
> > > >  
> > > You may want to disable a clock in the IRQ handler. The VPU driver in
> > > the Freescale BSP for i.MX51 does exactly this.
> > > Anyway I don't see any reason for using a mutex here instead of
> > > spin_lock_irq_save() as all other implementations do.
> > 
> > Because you suddenly make it impossible to sleep inside enable/disable
>               ^^^^^^^^^^^^^^^^^^^^^^^^^^^
> ???
> All implementations so far use spin_lock_irq_save()!
> 
> How would you be able to sleep with a mutex held?
> If you hold a lock you must not sleep, no matter what sort of lock it
> is.
That's wrong.  With a mutex hold you may sleep.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[RFC,PATCH 1/2] Add a common struct clk

[Uwe Kleine-König]

Hello Lothar,

On Mon, Jun 14, 2010 at 08:39:21AM +0200, Lothar Wa?mann wrote:
> Hi,
> 
> Benjamin Herrenschmidt writes:
> > On Fri, 2010-06-11 at 12:08 +0200, Lothar Wa?mann wrote:
> > > Hi,
> > > 
> > > > > > > Using a mutex in clk_enable()/clk_disable() is a bad idea, since that
> > > > > > > makes it impossible to call those functions in interrupt context.
> > > > IMHO if a device generates an irq its clock should already be on.  This
> > > > way you don't need to enable or disable a clock in irq context.
> > > >  
> > > You may want to disable a clock in the IRQ handler. The VPU driver in
> > > the Freescale BSP for i.MX51 does exactly this.
> > > Anyway I don't see any reason for using a mutex here instead of
> > > spin_lock_irq_save() as all other implementations do.
> > 
> > Because you suddenly make it impossible to sleep inside enable/disable
>               ^^^^^^^^^^^^^^^^^^^^^^^^^^^
> ???
> All implementations so far use spin_lock_irq_save()!
> 
> How would you be able to sleep with a mutex held?
> If you hold a lock you must not sleep, no matter what sort of lock it
> is.
That's wrong.  With a mutex hold you may sleep.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [RFC,PATCH 1/2] Add a common struct clk

[Lothar Waßmann]

Hi,

Uwe Kleine-König writes:
> Hello Lothar,
> 
> On Mon, Jun 14, 2010 at 08:39:21AM +0200, Lothar Waßmann wrote:
> > Hi,
> > 
> > Benjamin Herrenschmidt writes:
> > > On Fri, 2010-06-11 at 12:08 +0200, Lothar Waßmann wrote:
> > > > Hi,
> > > > 
> > > > > > > > Using a mutex in clk_enable()/clk_disable() is a bad idea, since that
> > > > > > > > makes it impossible to call those functions in interrupt context.
> > > > > IMHO if a device generates an irq its clock should already be on.  This
> > > > > way you don't need to enable or disable a clock in irq context.
> > > > >  
> > > > You may want to disable a clock in the IRQ handler. The VPU driver in
> > > > the Freescale BSP for i.MX51 does exactly this.
> > > > Anyway I don't see any reason for using a mutex here instead of
> > > > spin_lock_irq_save() as all other implementations do.
> > > 
> > > Because you suddenly make it impossible to sleep inside enable/disable
> >               ^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > ???
> > All implementations so far use spin_lock_irq_save()!
> > 
> > How would you be able to sleep with a mutex held?
> > If you hold a lock you must not sleep, no matter what sort of lock it
> > is.
> That's wrong.  With a mutex hold you may sleep.
> 
OK, you're right. But still all other implementations (omap, mxc,
davinci,...) use spin_lock_irqsave() to protect the enable/disable
functions and don't seem to have any problem with this.
Is there any reason to change this, or make it inconsistent
for one arch?

And arch/arm/plat-s3c/clock.c has the following comment:
|/* We originally used an mutex here, but some contexts (see resume)
| * are calling functions such as clk_set_parent() with IRQs disabled
| * causing an BUG to be triggered.
| */
|DEFINE_SPINLOCK(clocks_lock);


Lothar Waßmann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info@karo-electronics.de
___________________________________________________________

[RFC,PATCH 1/2] Add a common struct clk

[Lothar Waßmann]

Hi,

Uwe Kleine-K?nig writes:
> Hello Lothar,
> 
> On Mon, Jun 14, 2010 at 08:39:21AM +0200, Lothar Wa?mann wrote:
> > Hi,
> > 
> > Benjamin Herrenschmidt writes:
> > > On Fri, 2010-06-11 at 12:08 +0200, Lothar Wa?mann wrote:
> > > > Hi,
> > > > 
> > > > > > > > Using a mutex in clk_enable()/clk_disable() is a bad idea, since that
> > > > > > > > makes it impossible to call those functions in interrupt context.
> > > > > IMHO if a device generates an irq its clock should already be on.  This
> > > > > way you don't need to enable or disable a clock in irq context.
> > > > >  
> > > > You may want to disable a clock in the IRQ handler. The VPU driver in
> > > > the Freescale BSP for i.MX51 does exactly this.
> > > > Anyway I don't see any reason for using a mutex here instead of
> > > > spin_lock_irq_save() as all other implementations do.
> > > 
> > > Because you suddenly make it impossible to sleep inside enable/disable
> >               ^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > ???
> > All implementations so far use spin_lock_irq_save()!
> > 
> > How would you be able to sleep with a mutex held?
> > If you hold a lock you must not sleep, no matter what sort of lock it
> > is.
> That's wrong.  With a mutex hold you may sleep.
> 
OK, you're right. But still all other implementations (omap, mxc,
davinci,...) use spin_lock_irqsave() to protect the enable/disable
functions and don't seem to have any problem with this.
Is there any reason to change this, or make it inconsistent
for one arch?

And arch/arm/plat-s3c/clock.c has the following comment:
|/* We originally used an mutex here, but some contexts (see resume)
| * are calling functions such as clk_set_parent() with IRQs disabled
| * causing an BUG to be triggered.
| */
|DEFINE_SPINLOCK(clocks_lock);


Lothar Wa?mann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Gesch?ftsf?hrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________

Re: [RFC,PATCH 1/2] Add a common struct clk

[Benjamin Herrenschmidt]

On Mon, 2010-06-14 at 08:39 +0200, Lothar Waßmann wrote:
> All implementations so far use spin_lock_irq_save()!

Nothing prevents your implementation to be a tad smarter.

> How would you be able to sleep with a mutex held?
> If you hold a lock you must not sleep, no matter what sort of lock it
> is. 

You can perfectly sleep with a mutex held. You -do- have to be careful
of course to ensure you aren't going to do silly thing like re-enter and
try to take it twice, or A->B B->A deadlocks, but in the typical case of
wanting to use a msleep rather than udelay, it works very well :-)

Cheers,
Ben.

[RFC,PATCH 1/2] Add a common struct clk

[Benjamin Herrenschmidt]

On Mon, 2010-06-14 at 08:39 +0200, Lothar Wa?mann wrote:
> All implementations so far use spin_lock_irq_save()!

Nothing prevents your implementation to be a tad smarter.

> How would you be able to sleep with a mutex held?
> If you hold a lock you must not sleep, no matter what sort of lock it
> is. 

You can perfectly sleep with a mutex held. You -do- have to be careful
of course to ensure you aren't going to do silly thing like re-enter and
try to take it twice, or A->B B->A deadlocks, but in the typical case of
wanting to use a msleep rather than udelay, it works very well :-)

Cheers,
Ben.

Re: [RFC,PATCH 1/2] Add a common struct clk

[Lothar Waßmann]

Benjamin Herrenschmidt writes:
> On Mon, 2010-06-14 at 08:39 +0200, Lothar Waßmann wrote:
> > All implementations so far use spin_lock_irq_save()!
> 
> Nothing prevents your implementation to be a tad smarter.
> 
I vote for consistency, so that device drivers can be kept arch
independent instead of having to care about implentation details of
each arch.


Lothar Waßmann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info@karo-electronics.de
___________________________________________________________

[RFC,PATCH 1/2] Add a common struct clk

[Lothar Waßmann]

Benjamin Herrenschmidt writes:
> On Mon, 2010-06-14 at 08:39 +0200, Lothar Wa?mann wrote:
> > All implementations so far use spin_lock_irq_save()!
> 
> Nothing prevents your implementation to be a tad smarter.
> 
I vote for consistency, so that device drivers can be kept arch
independent instead of having to care about implentation details of
each arch.


Lothar Wa?mann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Gesch?ftsf?hrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________

Re: [RFC,PATCH 1/2] Add a common struct clk

[Uwe Kleine-König]

Hello,

> And arch/arm/plat-s3c/clock.c has the following comment:
> |/* We originally used an mutex here, but some contexts (see resume)
> | * are calling functions such as clk_set_parent() with IRQs disabled
> | * causing an BUG to be triggered.
> | */
> |DEFINE_SPINLOCK(clocks_lock);
I wonder why it's needed to reparent clocks during resume.  And where
exactly IRQs are disabled.  Hmm, this comment was initially introduced
by v2.6.28-rc7-180-gc3391e3, its commit log talks about cpufreq, not
resume.

Ben (Dooks): Is this still relevant?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[RFC,PATCH 1/2] Add a common struct clk

[Uwe Kleine-König]

Hello,

> And arch/arm/plat-s3c/clock.c has the following comment:
> |/* We originally used an mutex here, but some contexts (see resume)
> | * are calling functions such as clk_set_parent() with IRQs disabled
> | * causing an BUG to be triggered.
> | */
> |DEFINE_SPINLOCK(clocks_lock);
I wonder why it's needed to reparent clocks during resume.  And where
exactly IRQs are disabled.  Hmm, this comment was initially introduced
by v2.6.28-rc7-180-gc3391e3, its commit log talks about cpufreq, not
resume.

Ben (Dooks): Is this still relevant?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [RFC,PATCH 1/2] Add a common struct clk

[Uwe Kleine-König]

On Mon, Jun 14, 2010 at 11:30:08AM +0200, Lothar Waßmann wrote:
> Benjamin Herrenschmidt writes:
> > On Mon, 2010-06-14 at 08:39 +0200, Lothar Waßmann wrote:
> > > All implementations so far use spin_lock_irq_save()!
> > 
> > Nothing prevents your implementation to be a tad smarter.
> > 
> I vote for consistency, so that device drivers can be kept arch
> independent instead of having to care about implentation details of
> each arch.
Back when I implemented clock support for ns9xxx (unfortunately not in
mainline) I tried with a spinlock first and later switched to a mutex.
IIRC the reason was that on ns9215 enabling the rtc clock took long
(don't remember a number) and successfull enabling was signaled by an
irq.  So I would have had to implement irq polling in the clock code.

I think you can find different examples that make both possiblities bad.
All in all I think that a sleeping clock implementation is preferable as
it improves (general) latencies.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[RFC,PATCH 1/2] Add a common struct clk

[Uwe Kleine-König]

On Mon, Jun 14, 2010 at 11:30:08AM +0200, Lothar Wa?mann wrote:
> Benjamin Herrenschmidt writes:
> > On Mon, 2010-06-14 at 08:39 +0200, Lothar Wa?mann wrote:
> > > All implementations so far use spin_lock_irq_save()!
> > 
> > Nothing prevents your implementation to be a tad smarter.
> > 
> I vote for consistency, so that device drivers can be kept arch
> independent instead of having to care about implentation details of
> each arch.
Back when I implemented clock support for ns9xxx (unfortunately not in
mainline) I tried with a spinlock first and later switched to a mutex.
IIRC the reason was that on ns9215 enabling the rtc clock took long
(don't remember a number) and successfull enabling was signaled by an
irq.  So I would have had to implement irq polling in the clock code.

I think you can find different examples that make both possiblities bad.
All in all I think that a sleeping clock implementation is preferable as
it improves (general) latencies.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [RFC,PATCH 1/2] Add a common struct clk

[Jeremy Kerr]

Hi Ben,

I've just pushed to my clk-common branch, mostly involving the changes we've 
discussed so far.

http://kernel.ubuntu.com/git?p=jk/dt/linux-2.6.git;a=commitdiff;h=f2c5141c88aa4c5ba82aee1b5b8abe5e9ff09146

The clk-common-mx51 and clk-common-versatile branches both contain basic ports 
of the mx51 and versatile platforms to the new clock infrastructure.

Cheers,


Jeremy

[RFC,PATCH 1/2] Add a common struct clk

[Jeremy Kerr]

Hi Ben,

I've just pushed to my clk-common branch, mostly involving the changes we've 
discussed so far.

http://kernel.ubuntu.com/git?p=jk/dt/linux-2.6.git;a=commitdiff;h=f2c5141c88aa4c5ba82aee1b5b8abe5e9ff09146

The clk-common-mx51 and clk-common-versatile branches both contain basic ports 
of the mx51 and versatile platforms to the new clock infrastructure.

Cheers,


Jeremy

Re: [RFC,PATCH 1/2] Add a common struct clk

[Ben Dooks]

On Mon, Jun 14, 2010 at 08:52:25AM +0200, Lothar Wa?mann wrote:
> Hi,
> 
> Uwe Kleine-K?nig writes:
> > Hello Lothar,
> > 
> > On Mon, Jun 14, 2010 at 08:39:21AM +0200, Lothar Wa?mann wrote:
> > > Hi,
> > > 
> > > Benjamin Herrenschmidt writes:
> > > > On Fri, 2010-06-11 at 12:08 +0200, Lothar Wa?mann wrote:
> > > > > Hi,
> > > > > 
> > > > > > > > > Using a mutex in clk_enable()/clk_disable() is a bad idea, since that
> > > > > > > > > makes it impossible to call those functions in interrupt context.
> > > > > > IMHO if a device generates an irq its clock should already be on.  This
> > > > > > way you don't need to enable or disable a clock in irq context.
> > > > > >  
> > > > > You may want to disable a clock in the IRQ handler. The VPU driver in
> > > > > the Freescale BSP for i.MX51 does exactly this.
> > > > > Anyway I don't see any reason for using a mutex here instead of
> > > > > spin_lock_irq_save() as all other implementations do.
> > > > 
> > > > Because you suddenly make it impossible to sleep inside enable/disable
> > >               ^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > ???
> > > All implementations so far use spin_lock_irq_save()!
> > > 
> > > How would you be able to sleep with a mutex held?
> > > If you hold a lock you must not sleep, no matter what sort of lock it
> > > is.
> > That's wrong.  With a mutex hold you may sleep.
> > 
> OK, you're right. But still all other implementations (omap, mxc,
> davinci,...) use spin_lock_irqsave() to protect the enable/disable
> functions and don't seem to have any problem with this.
> Is there any reason to change this, or make it inconsistent
> for one arch?
> 
> And arch/arm/plat-s3c/clock.c has the following comment:
> |/* We originally used an mutex here, but some contexts (see resume)
> | * are calling functions such as clk_set_parent() with IRQs disabled
> | * causing an BUG to be triggered.
> | */
> |DEFINE_SPINLOCK(clocks_lock);

It entirely depends on what you are protecting, in some cases a mutex
is good enough for finding clocks, sometimes you may well end up with
the case you need to spin.

However, none of these clock implementations really thought through
all the subtleties of how it works. The s3c gets the clock naming
wrong (will be trying to sort that out soon).

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

  'a smiley only costs 4 bytes'

[RFC,PATCH 1/2] Add a common struct clk

[Ben Dooks]

On Mon, Jun 14, 2010 at 08:52:25AM +0200, Lothar Wa?mann wrote:
> Hi,
> 
> Uwe Kleine-K?nig writes:
> > Hello Lothar,
> > 
> > On Mon, Jun 14, 2010 at 08:39:21AM +0200, Lothar Wa?mann wrote:
> > > Hi,
> > > 
> > > Benjamin Herrenschmidt writes:
> > > > On Fri, 2010-06-11 at 12:08 +0200, Lothar Wa?mann wrote:
> > > > > Hi,
> > > > > 
> > > > > > > > > Using a mutex in clk_enable()/clk_disable() is a bad idea, since that
> > > > > > > > > makes it impossible to call those functions in interrupt context.
> > > > > > IMHO if a device generates an irq its clock should already be on.  This
> > > > > > way you don't need to enable or disable a clock in irq context.
> > > > > >  
> > > > > You may want to disable a clock in the IRQ handler. The VPU driver in
> > > > > the Freescale BSP for i.MX51 does exactly this.
> > > > > Anyway I don't see any reason for using a mutex here instead of
> > > > > spin_lock_irq_save() as all other implementations do.
> > > > 
> > > > Because you suddenly make it impossible to sleep inside enable/disable
> > >               ^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > ???
> > > All implementations so far use spin_lock_irq_save()!
> > > 
> > > How would you be able to sleep with a mutex held?
> > > If you hold a lock you must not sleep, no matter what sort of lock it
> > > is.
> > That's wrong.  With a mutex hold you may sleep.
> > 
> OK, you're right. But still all other implementations (omap, mxc,
> davinci,...) use spin_lock_irqsave() to protect the enable/disable
> functions and don't seem to have any problem with this.
> Is there any reason to change this, or make it inconsistent
> for one arch?
> 
> And arch/arm/plat-s3c/clock.c has the following comment:
> |/* We originally used an mutex here, but some contexts (see resume)
> | * are calling functions such as clk_set_parent() with IRQs disabled
> | * causing an BUG to be triggered.
> | */
> |DEFINE_SPINLOCK(clocks_lock);

It entirely depends on what you are protecting, in some cases a mutex
is good enough for finding clocks, sometimes you may well end up with
the case you need to spin.

However, none of these clock implementations really thought through
all the subtleties of how it works. The s3c gets the clock naming
wrong (will be trying to sort that out soon).

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

  'a smiley only costs 4 bytes'

Re: [RFC,PATCH 1/2] Add a common struct clk

[Ben Dooks]

On Mon, Jun 14, 2010 at 11:34:59AM +0200, Uwe Kleine-K?nig wrote:
> Hello,
> 
> > And arch/arm/plat-s3c/clock.c has the following comment:
> > |/* We originally used an mutex here, but some contexts (see resume)
> > | * are calling functions such as clk_set_parent() with IRQs disabled
> > | * causing an BUG to be triggered.
> > | */
> > |DEFINE_SPINLOCK(clocks_lock);
> I wonder why it's needed to reparent clocks during resume.  And where
> exactly IRQs are disabled.  Hmm, this comment was initially introduced
> by v2.6.28-rc7-180-gc3391e3, its commit log talks about cpufreq, not
> resume.
> 
> Ben (Dooks): Is this still relevant?

Yes, unfortunately the system may not resume with the clock registers
in the same state as they where before, and there are things that may
need to set the parents before the resume can continue.

We may have ended up saving quite a bit of the clock state by force,
but it isn't the nicest way, and there's still issues with any clocks
that require pll stabilisation.

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

  'a smiley only costs 4 bytes'

[RFC,PATCH 1/2] Add a common struct clk

[Ben Dooks]

On Mon, Jun 14, 2010 at 11:34:59AM +0200, Uwe Kleine-K?nig wrote:
> Hello,
> 
> > And arch/arm/plat-s3c/clock.c has the following comment:
> > |/* We originally used an mutex here, but some contexts (see resume)
> > | * are calling functions such as clk_set_parent() with IRQs disabled
> > | * causing an BUG to be triggered.
> > | */
> > |DEFINE_SPINLOCK(clocks_lock);
> I wonder why it's needed to reparent clocks during resume.  And where
> exactly IRQs are disabled.  Hmm, this comment was initially introduced
> by v2.6.28-rc7-180-gc3391e3, its commit log talks about cpufreq, not
> resume.
> 
> Ben (Dooks): Is this still relevant?

Yes, unfortunately the system may not resume with the clock registers
in the same state as they where before, and there are things that may
need to set the parents before the resume can continue.

We may have ended up saving quite a bit of the clock state by force,
but it isn't the nicest way, and there's still issues with any clocks
that require pll stabilisation.

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

  'a smiley only costs 4 bytes'

Re: [RFC,PATCH 1/2] Add a common struct clk

[Ben Dooks]

On Mon, Jun 14, 2010 at 11:43:10AM +0200, Uwe Kleine-K?nig wrote:
> On Mon, Jun 14, 2010 at 11:30:08AM +0200, Lothar Wa?mann wrote:
> > Benjamin Herrenschmidt writes:
> > > On Mon, 2010-06-14 at 08:39 +0200, Lothar Wa?mann wrote:
> > > > All implementations so far use spin_lock_irq_save()!
> > > 
> > > Nothing prevents your implementation to be a tad smarter.
> > > 
> > I vote for consistency, so that device drivers can be kept arch
> > independent instead of having to care about implentation details of
> > each arch.
> Back when I implemented clock support for ns9xxx (unfortunately not in
> mainline) I tried with a spinlock first and later switched to a mutex.
> IIRC the reason was that on ns9215 enabling the rtc clock took long
> (don't remember a number) and successfull enabling was signaled by an
> irq.  So I would have had to implement irq polling in the clock code.

Ok, you could have implemented a lock ot update the state, then had
some form of wake-queue to wake up the task once it did.

> I think you can find different examples that make both possiblities bad.
> All in all I think that a sleeping clock implementation is preferable as
> it improves (general) latencies.

It may be that we need to do a bit of work on some of the drivers to
ensure that they don't fully re-set their clocks until they are able
to sleep.

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

  'a smiley only costs 4 bytes'

[RFC,PATCH 1/2] Add a common struct clk

[Ben Dooks]

On Mon, Jun 14, 2010 at 11:43:10AM +0200, Uwe Kleine-K?nig wrote:
> On Mon, Jun 14, 2010 at 11:30:08AM +0200, Lothar Wa?mann wrote:
> > Benjamin Herrenschmidt writes:
> > > On Mon, 2010-06-14 at 08:39 +0200, Lothar Wa?mann wrote:
> > > > All implementations so far use spin_lock_irq_save()!
> > > 
> > > Nothing prevents your implementation to be a tad smarter.
> > > 
> > I vote for consistency, so that device drivers can be kept arch
> > independent instead of having to care about implentation details of
> > each arch.
> Back when I implemented clock support for ns9xxx (unfortunately not in
> mainline) I tried with a spinlock first and later switched to a mutex.
> IIRC the reason was that on ns9215 enabling the rtc clock took long
> (don't remember a number) and successfull enabling was signaled by an
> irq.  So I would have had to implement irq polling in the clock code.

Ok, you could have implemented a lock ot update the state, then had
some form of wake-queue to wake up the task once it did.

> I think you can find different examples that make both possiblities bad.
> All in all I think that a sleeping clock implementation is preferable as
> it improves (general) latencies.

It may be that we need to do a bit of work on some of the drivers to
ensure that they don't fully re-set their clocks until they are able
to sleep.

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

  'a smiley only costs 4 bytes'

Re: [RFC,PATCH 1/2] Add a common struct clk

[Benjamin Herrenschmidt]

On Wed, 2010-06-16 at 22:16 +0100, Ben Dooks wrote:
> > I think you can find different examples that make both possiblities
> bad.
> > All in all I think that a sleeping clock implementation is
> preferable as
> > it improves (general) latencies.
> 
> It may be that we need to do a bit of work on some of the drivers to
> ensure that they don't fully re-set their clocks until they are able
> to sleep. 

With the move of the interrupt disable we did upstream not long ago,
most drivers now can sleep at any stage of resume. sysdev's can't so
those are the main issue, but then, I don't like sysdevs :-)

I must say that on powermac I avoid some of these problems by, in
addition to the normal suspend/resume process, having some platform code
that (almost) blindly saves & restores a bunch of ASIC regs on late
suspend/early resume. It ends up being faster and simpler than going
through convolutions of suspend/resume callbacks left and right that
need to happen in the right order.

Cheers,
Ben.

[RFC,PATCH 1/2] Add a common struct clk

[Benjamin Herrenschmidt]

On Wed, 2010-06-16 at 22:16 +0100, Ben Dooks wrote:
> > I think you can find different examples that make both possiblities
> bad.
> > All in all I think that a sleeping clock implementation is
> preferable as
> > it improves (general) latencies.
> 
> It may be that we need to do a bit of work on some of the drivers to
> ensure that they don't fully re-set their clocks until they are able
> to sleep. 

With the move of the interrupt disable we did upstream not long ago,
most drivers now can sleep at any stage of resume. sysdev's can't so
those are the main issue, but then, I don't like sysdevs :-)

I must say that on powermac I avoid some of these problems by, in
addition to the normal suspend/resume process, having some platform code
that (almost) blindly saves & restores a bunch of ASIC regs on late
suspend/early resume. It ends up being faster and simpler than going
through convolutions of suspend/resume callbacks left and right that
need to happen in the right order.

Cheers,
Ben.

Re: [RFC,PATCH 1/2] Add a common struct clk

[Jeremy Kerr]

Hi Ben,

> > > > +#define INIT_CLK(name, o) \
> > > > +	{ .ops = &o, .enable_count = 0, \
> > > > +		.mutex = __MUTEX_INITIALIZER(name.mutex) }
> > > 
> > > how about doing the mutex initinitialisation at registration
> > > time, will save a pile of non-zero code in the image to mess up
> > > the compression.
> > 
> > I've just yesterday added the following to my tree, to allow dynamic 
> > initialisation:
> > 
> > static inline void clk_init(struct clk *clk, const struct clk_ops *ops)
> > {
> > 	clk->ops = ops;
> > 	clk->enable_count = 0;
> > 	mutex_init(&clk->mutex);
> > }
> > 
> > So we can do this either way.
> 
> the above is in my view better.

After implementing this, it turns out it won't work - we don't have
access to all clocks in order to do the mutex init (thanks to Jason Hui
for pointing this out).

At present, I'm calling clk_init_common() (which initialises the mutex)
from clkdev_add, but there are clocks that don't get initialised. For
example:

	struct clk_fixed parent_clk = INIT_CLK(32768);
	struct clk_foo child_clk = INIT_CLK_FOO(parent.clk);

	struct clk_lookup lookup = {
		.dev_id = "foo",
		.clk = child_clk,
	};

	function platform_clk_init(void)
	{
		clkdev_add(lookup);
	}

In this case, the child_clk's mutex will get initialised, but
parent_clk's won't. We can't walk the parents of child_clk and
initialise, as we may re-initialise the mutexes of parents with > 1
child.

Now, we *could* do the clk_common_init() from board-specific code, and
require that that code guarantees to call clk_common_init() on every
clock defined. I think that's a recipe for pain, as there will
undoubtedly be clocks missed, causing an oops when the clock is first
used.

So I think the solution will have to be to do the mutex initialisation
statically. I'm planning to leave clk_common_init() available for clocks
initalised at runtime, but it will no longer be called automatically
from core code.

Any thoughts?

Cheers,


Jeremy

[RFC,PATCH 1/2] Add a common struct clk

[Jeremy Kerr]

Hi Ben,

> > > > +#define INIT_CLK(name, o) \
> > > > +	{ .ops = &o, .enable_count = 0, \
> > > > +		.mutex = __MUTEX_INITIALIZER(name.mutex) }
> > > 
> > > how about doing the mutex initinitialisation at registration
> > > time, will save a pile of non-zero code in the image to mess up
> > > the compression.
> > 
> > I've just yesterday added the following to my tree, to allow dynamic 
> > initialisation:
> > 
> > static inline void clk_init(struct clk *clk, const struct clk_ops *ops)
> > {
> > 	clk->ops = ops;
> > 	clk->enable_count = 0;
> > 	mutex_init(&clk->mutex);
> > }
> > 
> > So we can do this either way.
> 
> the above is in my view better.

After implementing this, it turns out it won't work - we don't have
access to all clocks in order to do the mutex init (thanks to Jason Hui
for pointing this out).

At present, I'm calling clk_init_common() (which initialises the mutex)
from clkdev_add, but there are clocks that don't get initialised. For
example:

	struct clk_fixed parent_clk = INIT_CLK(32768);
	struct clk_foo child_clk = INIT_CLK_FOO(parent.clk);

	struct clk_lookup lookup = {
		.dev_id = "foo",
		.clk = child_clk,
	};

	function platform_clk_init(void)
	{
		clkdev_add(lookup);
	}

In this case, the child_clk's mutex will get initialised, but
parent_clk's won't. We can't walk the parents of child_clk and
initialise, as we may re-initialise the mutexes of parents with > 1
child.

Now, we *could* do the clk_common_init() from board-specific code, and
require that that code guarantees to call clk_common_init() on every
clock defined. I think that's a recipe for pain, as there will
undoubtedly be clocks missed, causing an oops when the clock is first
used.

So I think the solution will have to be to do the mutex initialisation
statically. I'm planning to leave clk_common_init() available for clocks
initalised at runtime, but it will no longer be called automatically
from core code.

Any thoughts?

Cheers,


Jeremy