[RFC,PATCH 1/7] arm: add a common struct clk

[RFC,PATCH 1/7] arm: 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 change is an effort to unify struct clk where possible, by defining
a common struct clk, containing a set of function pointers. Different
clock implementations can set their own callbacks, 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_HAVE_COMMON_STRUCT_CLK.

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/arm/Kconfig              |    4 +
 arch/arm/common/Makefile      |    1 
 arch/arm/common/clk.c         |   78 ++++++++++++++++++++++++++++++++++
 arch/arm/common/clkdev.c      |    2 
 arch/arm/include/asm/clk.h    |   37 ++++++++++++++++
 arch/arm/include/asm/clkdev.h |    2 
 6 files changed, 123 insertions(+), 1 deletion(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 233a222..b6eaf10 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -46,6 +46,10 @@ config GENERIC_CLOCKEVENTS_BROADCAST
 	depends on GENERIC_CLOCKEVENTS
 	default y if SMP && !LOCAL_TIMERS
 
+config HAVE_COMMON_STRUCT_CLK
+	bool
+	default n
+
 config HAVE_TCM
 	bool
 	select GENERIC_ALLOCATOR
diff --git a/arch/arm/common/Makefile b/arch/arm/common/Makefile
index 76be7ff..6073dbe 100644
--- a/arch/arm/common/Makefile
+++ b/arch/arm/common/Makefile
@@ -17,3 +17,4 @@ obj-$(CONFIG_ARCH_IXP2000)	+= uengine.o
 obj-$(CONFIG_ARCH_IXP23XX)	+= uengine.o
 obj-$(CONFIG_PCI_HOST_ITE8152)  += it8152.o
 obj-$(CONFIG_COMMON_CLKDEV)	+= clkdev.o
+obj-$(CONFIG_HAVE_COMMON_STRUCT_CLK) += clk.o
diff --git a/arch/arm/common/clk.c b/arch/arm/common/clk.c
new file mode 100644
index 0000000..0357d02
--- /dev/null
+++ b/arch/arm/common/clk.c
@@ -0,0 +1,78 @@
+/*
+ *  arch/arm/common/clk.c
+ *
+ *  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.
+ *
+ * Helper for the clk API to assist looking up a struct clk.
+ */
+
+#include <linux/module.h>
+#include <linux/err.h>
+#include <linux/clk.h>
+#include <asm/clk.h>
+
+int clk_enable(struct clk *clk)
+{
+	if (clk->enable)
+		return clk->enable(clk);
+	return 0;
+}
+EXPORT_SYMBOL(clk_enable);
+ 
+void clk_disable(struct clk *clk)
+{
+	if (clk->disable)
+		clk->disable(clk);
+}
+EXPORT_SYMBOL(clk_disable);
+ 
+unsigned long clk_get_rate(struct clk *clk)
+{
+	if (clk->get_rate)
+		return clk->get_rate(clk);
+	return 0;
+}
+EXPORT_SYMBOL(clk_get_rate);
+ 
+void clk_put(struct clk *clk)
+{
+	if (clk->put)
+		clk->put(clk);
+}
+EXPORT_SYMBOL(clk_put);
+
+long clk_round_rate(struct clk *clk, unsigned long rate)
+{
+	if (clk->round_rate)
+		return clk->round_rate(clk, rate);
+	return -ENOSYS;
+}
+EXPORT_SYMBOL(clk_round_rate);
+ 
+int clk_set_rate(struct clk *clk, unsigned long rate)
+{
+	if (clk->set_rate)
+		return clk->set_rate(clk, rate);
+	return -ENOSYS;
+}
+EXPORT_SYMBOL(clk_set_rate);
+ 
+int clk_set_parent(struct clk *clk, struct clk *parent)
+{
+	if (clk->set_parent)
+		return clk->set_parent(clk, parent);
+	return -ENOSYS;
+}
+EXPORT_SYMBOL(clk_set_parent);
+
+struct clk *clk_get_parent(struct clk *clk)
+{
+	if (clk->get_parent)
+		return clk->get_parent(clk);
+	return ERR_PTR(-ENOSYS);
+}
+EXPORT_SYMBOL(clk_get_parent);
diff --git a/arch/arm/common/clkdev.c b/arch/arm/common/clkdev.c
index aae5bc0..bd3866b 100644
--- a/arch/arm/common/clkdev.c
+++ b/arch/arm/common/clkdev.c
@@ -85,11 +85,13 @@ struct clk *clk_get(struct device *dev, const char *con_id)
 }
 EXPORT_SYMBOL(clk_get);
 
+#ifndef CONFIG_HAVE_COMMON_STRUCT_CLK
 void clk_put(struct clk *clk)
 {
 	__clk_put(clk);
 }
 EXPORT_SYMBOL(clk_put);
+#endif /* CONFIG_HAVE_COMMON_STRUCT_CLK */
 
 void clkdev_add(struct clk_lookup *cl)
 {
diff --git a/arch/arm/include/asm/clk.h b/arch/arm/include/asm/clk.h
new file mode 100644
index 0000000..87fcfae
--- /dev/null
+++ b/arch/arm/include/asm/clk.h
@@ -0,0 +1,37 @@
+/*
+ *  arch/arm/include/asm/clk.h
+ *
+ *  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.
+ *
+ * Generic definitions for struct clk
+ */
+#ifndef __ASM_CLK_H
+#define __ASM_CLK_H
+
+#ifdef CONFIG_HAVE_COMMON_STRUCT_CLK
+
+/* The definition of struct clk is global, clock providers should
+ * generate "subclasses" embedding struct clk.
+ */
+struct clk {
+       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 *);
+};
+
+#else /* CONFIG_HAVE_COMMON_STRUCT_CLK */
+
+struct clk;
+
+#endif
+
+#endif
diff --git a/arch/arm/include/asm/clkdev.h b/arch/arm/include/asm/clkdev.h
index b6ec7c6..e994a74 100644
--- a/arch/arm/include/asm/clkdev.h
+++ b/arch/arm/include/asm/clkdev.h
@@ -12,7 +12,7 @@
 #ifndef __ASM_CLKDEV_H
 #define __ASM_CLKDEV_H
 
-struct clk;
+#include <asm/clk.h>
 
 struct clk_lookup {
 	struct list_head	node;

[RFC,PATCH 5/7] arm/realview: use generic struct clk

[Jeremy Kerr]

Use the common struct clk interface for the realview clocks.

Compile tested only.

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

---
 arch/arm/Kconfig               |    1 
 arch/arm/mach-realview/clock.c |   43 ++++++++++-------------------
 arch/arm/mach-realview/clock.h |   21 +++++++++-----
 arch/arm/mach-realview/core.c  |   48 +++++++++++++++++----------------
 4 files changed, 55 insertions(+), 58 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 7e59bce..a11ee79 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -236,6 +236,7 @@ config ARCH_REALVIEW
 	select ICST307
 	select GENERIC_TIME
 	select GENERIC_CLOCKEVENTS
+	select HAVE_COMMON_STRUCT_CLK
 	select ARCH_WANT_OPTIONAL_GPIOLIB
 	help
 	  This enables support for ARM Ltd RealView boards.
diff --git a/arch/arm/mach-realview/clock.c b/arch/arm/mach-realview/clock.c
index a704311..4c09166 100644
--- a/arch/arm/mach-realview/clock.c
+++ b/arch/arm/mach-realview/clock.c
@@ -18,47 +18,34 @@
 #include <linux/clk.h>
 #include <linux/mutex.h>
 
+#include <asm/clk.h>
 #include <asm/hardware/icst307.h>
 
 #include "clock.h"
 
-int clk_enable(struct clk *clk)
-{
-	return 0;
-}
-EXPORT_SYMBOL(clk_enable);
-
-void clk_disable(struct clk *clk)
-{
-}
-EXPORT_SYMBOL(clk_disable);
+#define to_clk_realview(clk) (container_of(clk, struct clk_realview, clk))
 
-unsigned long clk_get_rate(struct clk *clk)
+unsigned long clk_realview_get_rate(struct clk *clk)
 {
-	return clk->rate;
+	return to_clk_realview(clk)->rate;
 }
-EXPORT_SYMBOL(clk_get_rate);
 
-long clk_round_rate(struct clk *clk, unsigned long rate)
+long clk_realview_round_rate(struct clk *clk, unsigned long rate)
 {
+	const struct icst307_params *params = &to_clk_realview(clk)->params;
 	struct icst307_vco vco;
-	vco = icst307_khz_to_vco(clk->params, rate / 1000);
-	return icst307_khz(clk->params, vco) * 1000;
+	vco = icst307_khz_to_vco(params, rate / 1000);
+	return icst307_khz(params, vco) * 1000;
 }
-EXPORT_SYMBOL(clk_round_rate);
 
-int clk_set_rate(struct clk *clk, unsigned long rate)
+int clk_realview_set_rate(struct clk *clk, unsigned long rate)
 {
-	int ret = -EIO;
+	struct clk_realview *r_clk = to_clk_realview(clk);
+	struct icst307_vco vco;
 
-	if (clk->setvco) {
-		struct icst307_vco vco;
+	vco = icst307_khz_to_vco(&r_clk->params, rate / 1000);
+	r_clk->rate = icst307_khz(&r_clk->params, vco) * 1000;
+	r_clk->setvco(r_clk, vco);
 
-		vco = icst307_khz_to_vco(clk->params, rate / 1000);
-		clk->rate = icst307_khz(clk->params, vco) * 1000;
-		clk->setvco(clk, vco);
-		ret = 0;
-	}
-	return ret;
+	return 0;
 }
-EXPORT_SYMBOL(clk_set_rate);
diff --git a/arch/arm/mach-realview/clock.h b/arch/arm/mach-realview/clock.h
index ebbb0f0..b8023f9 100644
--- a/arch/arm/mach-realview/clock.h
+++ b/arch/arm/mach-realview/clock.h
@@ -8,12 +8,19 @@
  * it under the terms of the GNU General Public License version 2 as
  * published by the Free Software Foundation.
  */
-struct module;
-struct icst307_params;
 
-struct clk {
-	unsigned long		rate;
-	const struct icst307_params *params;
-	void			*data;
-	void			(*setvco)(struct clk *, struct icst307_vco vco);
+#include <asm/clk.h>
+
+struct clk_realview {
+	struct clk			clk;
+	unsigned long			rate;
+	const struct icst307_params	params;
+	void				(*setvco)(struct clk_realview *,
+						  struct icst307_vco);
 };
+
+unsigned long clk_realview_get_rate(struct clk *clk);
+
+long clk_realview_round_rate(struct clk *clk, unsigned long rate);
+
+int clk_realview_set_rate(struct clk *clk, unsigned long rate);
diff --git a/arch/arm/mach-realview/core.c b/arch/arm/mach-realview/core.c
index 9f29343..f57ae12 100644
--- a/arch/arm/mach-realview/core.c
+++ b/arch/arm/mach-realview/core.c
@@ -273,16 +273,8 @@ struct mmci_platform_data realview_mmc1_plat_data = {
 /*
  * Clock handling
  */
-static const struct icst307_params realview_oscvco_params = {
-	.ref		= 24000,
-	.vco_max	= 200000,
-	.vd_min		= 4 + 8,
-	.vd_max		= 511 + 8,
-	.rd_min		= 1 + 2,
-	.rd_max		= 127 + 2,
-};
 
-static void realview_oscvco_set(struct clk *clk, struct icst307_vco vco)
+static void realview_oscvco_set(struct clk_realview *clk, struct icst307_vco vco)
 {
 	void __iomem *sys_lock = __io_address(REALVIEW_SYS_BASE) + REALVIEW_SYS_LOCK_OFFSET;
 	void __iomem *sys_osc;
@@ -301,46 +293,56 @@ static void realview_oscvco_set(struct clk *clk, struct icst307_vco vco)
 	writel(0, sys_lock);
 }
 
-static struct clk oscvco_clk = {
-	.params	= &realview_oscvco_params,
+static struct clk_realview oscvco_clk = {
+	.clk = {
+		.get_rate = clk_realview_get_rate,
+		.round_rate = clk_realview_round_rate,
+		.set_rate = clk_realview_set_rate,
+	},
+	.params	= {
+		.ref		= 24000,
+		.vco_max	= 200000,
+		.vd_min		= 4 + 8,
+		.vd_max		= 511 + 8,
+		.rd_min		= 1 + 2,
+		.rd_max		= 127 + 2,
+	},
 	.setvco = realview_oscvco_set,
 };
 
 /*
  * These are fixed clocks.
  */
-static struct clk ref24_clk = {
-	.rate	= 24000000,
-};
+static struct clk_fixed ref24_clk = DEFINE_CLK_FIXED(24000000);
 
 static struct clk_lookup lookups[] = {
 	{	/* UART0 */
 		.dev_id		= "dev:uart0",
-		.clk		= &ref24_clk,
+		.clk		= &ref24_clk.clk,
 	}, {	/* UART1 */
 		.dev_id		= "dev:uart1",
-		.clk		= &ref24_clk,
+		.clk		= &ref24_clk.clk,
 	}, {	/* UART2 */
 		.dev_id		= "dev:uart2",
-		.clk		= &ref24_clk,
+		.clk		= &ref24_clk.clk,
 	}, {	/* UART3 */
 		.dev_id		= "fpga:uart3",
-		.clk		= &ref24_clk,
+		.clk		= &ref24_clk.clk,
 	}, {	/* KMI0 */
 		.dev_id		= "fpga:kmi0",
-		.clk		= &ref24_clk,
+		.clk		= &ref24_clk.clk,
 	}, {	/* KMI1 */
 		.dev_id		= "fpga:kmi1",
-		.clk		= &ref24_clk,
+		.clk		= &ref24_clk.clk,
 	}, {	/* MMC0 */
 		.dev_id		= "fpga:mmc0",
-		.clk		= &ref24_clk,
+		.clk		= &ref24_clk.clk,
 	}, {	/* EB:CLCD */
 		.dev_id		= "dev:clcd",
-		.clk		= &oscvco_clk,
+		.clk		= &oscvco_clk.clk,
 	}, {	/* PB:CLCD */
 		.dev_id		= "issp:clcd",
-		.clk		= &oscvco_clk,
+		.clk		= &oscvco_clk.clk,
 	}
 };
 

[RFC,PATCH 2/7] arm: generic support for fixed-rate clocks

[Jeremy Kerr]

Since we have a few fixed rate clocks, define a simple clk_fixed
with a single rate property.

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

---
 arch/arm/common/clk.c      |    8 ++++++++
 arch/arm/include/asm/clk.h |   13 +++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/arch/arm/common/clk.c b/arch/arm/common/clk.c
index 0357d02..699bb28 100644
--- a/arch/arm/common/clk.c
+++ b/arch/arm/common/clk.c
@@ -76,3 +76,11 @@ struct clk *clk_get_parent(struct clk *clk)
 	return ERR_PTR(-ENOSYS);
 }
 EXPORT_SYMBOL(clk_get_parent);
+
+#define to_clk_fixed(clk) (container_of(clk, struct clk_fixed, clk))
+
+unsigned long clk_fixed_get_rate(struct clk *clk)
+{
+	return to_clk_fixed(clk)->rate;
+}
+EXPORT_SYMBOL(clk_fixed_get_rate);
diff --git a/arch/arm/include/asm/clk.h b/arch/arm/include/asm/clk.h
index 87fcfae..1547fa5 100644
--- a/arch/arm/include/asm/clk.h
+++ b/arch/arm/include/asm/clk.h
@@ -28,6 +28,19 @@ struct clk {
        struct clk*     (*get_parent)(struct clk *);
 };
 
+/* Simple fixed-rate clock */
+struct clk_fixed {
+	struct clk	clk;
+	unsigned long	rate;
+};
+
+extern unsigned long clk_fixed_get_rate(struct clk *clk);
+
+#define DEFINE_CLK_FIXED(r) { \
+	.clk = { .get_rate = clk_fixed_get_rate }, \
+	.rate = (r) \
+};
+
 #else /* CONFIG_HAVE_COMMON_STRUCT_CLK */
 
 struct clk;

[RFC,PATCH 7/7] arm/icst307: remove icst307_ps_to_vco

[Jeremy Kerr]

icst307_ps_to_vco isn't used, so remove it.

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

---
 arch/arm/common/icst307.c |   62 --------------------------------------
 1 file changed, 62 deletions(-)

diff --git a/arch/arm/common/icst307.c b/arch/arm/common/icst307.c
index 4c6cfb4..b4899c7 100644
--- a/arch/arm/common/icst307.c
+++ b/arch/arm/common/icst307.c
@@ -96,68 +96,6 @@ icst307_khz_to_vco(const struct icst307_params *p, unsigned long freq)
 	return vco;
 }
 
-static struct icst307_vco
-icst307_ps_to_vco(const struct icst307_params *p, unsigned long period)
-{
-	struct icst307_vco vco = { .s = 1, .v = p->vd_max, .r = p->rd_max };
-	unsigned long f, ps;
-	unsigned int i = 0, rd, best = (unsigned int)-1;
-
-	ps = 1000000000UL / p->vco_max;
-
-	/*
-	 * First, find the PLL output divisor such
-	 * that the PLL output is within spec.
-	 */
-	do {
-		f = period / s2div[idx2s[i]];
-
-		/*
-		 * f must be between 6MHz and 200MHz (3.3 or 5V)
-		 */
-		if (f >= ps && f < 1000000000UL / 6000 + 1)
-			break;
-	} while (i < ARRAY_SIZE(idx2s));
-
-	if (i >= ARRAY_SIZE(idx2s))
-		return vco;
-
-	vco.s = idx2s[i];
-
-	ps = 500000000UL / p->ref;
-
-	/*
-	 * Now find the closest divisor combination
-	 * which gives a PLL output of 'f'.
-	 */
-	for (rd = p->rd_min; rd <= p->rd_max; rd++) {
-		unsigned long f_in_div, f_pll;
-		unsigned int vd;
-		int f_diff;
-
-		f_in_div = ps * rd;
-
-		vd = (f_in_div + f / 2) / f;
-		if (vd < p->vd_min || vd > p->vd_max)
-			continue;
-
-		f_pll = (f_in_div + vd / 2) / vd;
-		f_diff = f_pll - f;
-		if (f_diff < 0)
-			f_diff = -f_diff;
-
-		if ((unsigned)f_diff < best) {
-			vco.v = vd - 8;
-			vco.r = rd - 2;
-			if (f_diff == 0)
-				break;
-			best = f_diff;
-		}
-	}
-
-	return vco;
-}
-
 unsigned long clk_icst307_get_rate(struct clk *clk)
 {
 	return to_clk_icst307(clk)->rate;

[RFC,PATCH 4/7] arm/versatile: remove oscoff from clk_versatile

[Jeremy Kerr]

We only use one value for oscoff, so remove it from the struct.

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

---
 arch/arm/mach-versatile/clock.h |    1 -
 arch/arm/mach-versatile/core.c  |    6 +++---
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mach-versatile/clock.h b/arch/arm/mach-versatile/clock.h
index 8b0ec44..569a847 100644
--- a/arch/arm/mach-versatile/clock.h
+++ b/arch/arm/mach-versatile/clock.h
@@ -13,7 +13,6 @@ struct clk_versatile {
 	struct clk			clk;
 	unsigned long			rate;
 	const struct icst307_params	params;
-	u32				oscoff;
 	void				(*setvco)(struct clk_versatile *,
 						  struct icst307_vco);
 };
diff --git a/arch/arm/mach-versatile/core.c b/arch/arm/mach-versatile/core.c
index fb66b20..df1bb4e 100644
--- a/arch/arm/mach-versatile/core.c
+++ b/arch/arm/mach-versatile/core.c
@@ -384,13 +384,14 @@ static void versatile_oscvco_set(struct clk_versatile *clk, struct icst307_vco v
 {
 	void __iomem *sys = __io_address(VERSATILE_SYS_BASE);
 	void __iomem *sys_lock = sys + VERSATILE_SYS_LOCK_OFFSET;
+	void __iomem *sys_osc = sys + VERSATILE_SYS_OSCCLCD_OFFSET;
 	u32 val;
 
-	val = readl(sys + clk->oscoff) & ~0x7ffff;
+	val = readl(sys_osc) & ~0x7ffff;
 	val |= vco.v | (vco.r << 9) | (vco.s << 16);
 
 	writel(0xa05f, sys_lock);
-	writel(val, sys + clk->oscoff);
+	writel(val, sys_osc);
 	writel(0, sys_lock);
 }
 
@@ -408,7 +409,6 @@ static struct clk_versatile osc4_clk = {
 		.rd_min		= 1 + 2,
 		.rd_max		= 127 + 2,
 	},
-	.oscoff = VERSATILE_SYS_OSCCLCD_OFFSET,
 	.setvco = versatile_oscvco_set,
 };
 

[RFC,PATCH 3/7] arm/versatile: use generic struct clk

[Jeremy Kerr]

Use the common struct clk interface for the versatile clocks.

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

---
 arch/arm/Kconfig                |    1 
 arch/arm/mach-versatile/clock.c |   44 ++++++++-------------------
 arch/arm/mach-versatile/clock.h |   23 +++++++++-----
 arch/arm/mach-versatile/core.c  |   52 ++++++++++++++++----------------
 4 files changed, 57 insertions(+), 63 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index b6eaf10..7e59bce 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -249,6 +249,7 @@ config ARCH_VERSATILE
 	select ICST307
 	select GENERIC_TIME
 	select GENERIC_CLOCKEVENTS
+	select HAVE_COMMON_STRUCT_CLK
 	select ARCH_WANT_OPTIONAL_GPIOLIB
 	help
 	  This enables support for ARM Ltd Versatile board.
diff --git a/arch/arm/mach-versatile/clock.c b/arch/arm/mach-versatile/clock.c
index c50a44e..9b47e38 100644
--- a/arch/arm/mach-versatile/clock.c
+++ b/arch/arm/mach-versatile/clock.c
@@ -18,48 +18,32 @@
 #include <linux/clk.h>
 #include <linux/mutex.h>
 
-#include <asm/clkdev.h>
+#include <asm/clk.h>
 #include <asm/hardware/icst307.h>
 
 #include "clock.h"
 
-int clk_enable(struct clk *clk)
+unsigned long clk_versatile_get_rate(struct clk *clk)
 {
-	return 0;
+	return to_clk_versatile(clk)->rate;
 }
-EXPORT_SYMBOL(clk_enable);
 
-void clk_disable(struct clk *clk)
-{
-}
-EXPORT_SYMBOL(clk_disable);
-
-unsigned long clk_get_rate(struct clk *clk)
-{
-	return clk->rate;
-}
-EXPORT_SYMBOL(clk_get_rate);
-
-long clk_round_rate(struct clk *clk, unsigned long rate)
+long clk_versatile_round_rate(struct clk *clk, unsigned long rate)
 {
+	const struct icst307_params *params = &to_clk_versatile(clk)->params;
 	struct icst307_vco vco;
-	vco = icst307_khz_to_vco(clk->params, rate / 1000);
-	return icst307_khz(clk->params, vco) * 1000;
+	vco = icst307_khz_to_vco(params, rate / 1000);
+	return icst307_khz(params, vco) * 1000;
 }
-EXPORT_SYMBOL(clk_round_rate);
 
-int clk_set_rate(struct clk *clk, unsigned long rate)
+int clk_versatile_set_rate(struct clk *clk, unsigned long rate)
 {
-	int ret = -EIO;
+	struct clk_versatile *v_clk = to_clk_versatile(clk);
+	struct icst307_vco vco;
 
-	if (clk->setvco) {
-		struct icst307_vco vco;
+	vco = icst307_khz_to_vco(&v_clk->params, rate / 1000);
+	v_clk->rate = icst307_khz(&v_clk->params, vco) * 1000;
+	v_clk->setvco(v_clk, vco);
 
-		vco = icst307_khz_to_vco(clk->params, rate / 1000);
-		clk->rate = icst307_khz(clk->params, vco) * 1000;
-		clk->setvco(clk, vco);
-		ret = 0;
-	}
-	return ret;
+	return 0;
 }
-EXPORT_SYMBOL(clk_set_rate);
diff --git a/arch/arm/mach-versatile/clock.h b/arch/arm/mach-versatile/clock.h
index 03468fd..8b0ec44 100644
--- a/arch/arm/mach-versatile/clock.h
+++ b/arch/arm/mach-versatile/clock.h
@@ -8,13 +8,20 @@
  * it under the terms of the GNU General Public License version 2 as
  * published by the Free Software Foundation.
  */
-struct module;
-struct icst307_params;
 
-struct clk {
-	unsigned long		rate;
-	const struct icst307_params *params;
-	u32			oscoff;
-	void			*data;
-	void			(*setvco)(struct clk *, struct icst307_vco vco);
+struct clk_versatile {
+	struct clk			clk;
+	unsigned long			rate;
+	const struct icst307_params	params;
+	u32				oscoff;
+	void				(*setvco)(struct clk_versatile *,
+						  struct icst307_vco);
 };
+
+unsigned long clk_versatile_get_rate(struct clk *clk);
+
+long clk_versatile_round_rate(struct clk *clk, unsigned long rate);
+
+int clk_versatile_set_rate(struct clk *clk, unsigned long rate);
+
+#define to_clk_versatile(clk) (container_of(clk, struct clk_versatile, clk))
diff --git a/arch/arm/mach-versatile/core.c b/arch/arm/mach-versatile/core.c
index e13be7c..fb66b20 100644
--- a/arch/arm/mach-versatile/core.c
+++ b/arch/arm/mach-versatile/core.c
@@ -379,16 +379,8 @@ static struct mmci_platform_data mmc0_plat_data = {
 /*
  * Clock handling
  */
-static const struct icst307_params versatile_oscvco_params = {
-	.ref		= 24000,
-	.vco_max	= 200000,
-	.vd_min		= 4 + 8,
-	.vd_max		= 511 + 8,
-	.rd_min		= 1 + 2,
-	.rd_max		= 127 + 2,
-};
 
-static void versatile_oscvco_set(struct clk *clk, struct icst307_vco vco)
+static void versatile_oscvco_set(struct clk_versatile *clk, struct icst307_vco vco)
 {
 	void __iomem *sys = __io_address(VERSATILE_SYS_BASE);
 	void __iomem *sys_lock = sys + VERSATILE_SYS_LOCK_OFFSET;
@@ -402,47 +394,57 @@ static void versatile_oscvco_set(struct clk *clk, struct icst307_vco vco)
 	writel(0, sys_lock);
 }
 
-static struct clk osc4_clk = {
-	.params	= &versatile_oscvco_params,
-	.oscoff	= VERSATILE_SYS_OSCCLCD_OFFSET,
-	.setvco	= versatile_oscvco_set,
+static struct clk_versatile osc4_clk = {
+	.clk = {
+		.get_rate = clk_versatile_get_rate,
+		.round_rate = clk_versatile_round_rate,
+		.set_rate = clk_versatile_set_rate,
+	},
+	.params = {
+		.ref		= 24000,
+		.vco_max	= 200000,
+		.vd_min		= 4 + 8,
+		.vd_max		= 511 + 8,
+		.rd_min		= 1 + 2,
+		.rd_max		= 127 + 2,
+	},
+	.oscoff = VERSATILE_SYS_OSCCLCD_OFFSET,
+	.setvco = versatile_oscvco_set,
 };
 
 /*
  * These are fixed clocks.
  */
-static struct clk ref24_clk = {
-	.rate	= 24000000,
-};
+static struct clk_fixed ref24_clk = DEFINE_CLK_FIXED(24000000);
 
 static struct clk_lookup lookups[] = {
 	{	/* UART0 */
 		.dev_id		= "dev:f1",
-		.clk		= &ref24_clk,
+		.clk		= &ref24_clk.clk,
 	}, {	/* UART1 */
 		.dev_id		= "dev:f2",
-		.clk		= &ref24_clk,
+		.clk		= &ref24_clk.clk,
 	}, {	/* UART2 */
 		.dev_id		= "dev:f3",
-		.clk		= &ref24_clk,
+		.clk		= &ref24_clk.clk,
 	}, {	/* UART3 */
 		.dev_id		= "fpga:09",
-		.clk		= &ref24_clk,
+		.clk		= &ref24_clk.clk,
 	}, {	/* KMI0 */
 		.dev_id		= "fpga:06",
-		.clk		= &ref24_clk,
+		.clk		= &ref24_clk.clk,
 	}, {	/* KMI1 */
 		.dev_id		= "fpga:07",
-		.clk		= &ref24_clk,
+		.clk		= &ref24_clk.clk,
 	}, {	/* MMC0 */
 		.dev_id		= "fpga:05",
-		.clk		= &ref24_clk,
+		.clk		= &ref24_clk.clk,
 	}, {	/* MMC1 */
 		.dev_id		= "fpga:0b",
-		.clk		= &ref24_clk,
+		.clk		= &ref24_clk.clk,
 	}, {	/* CLCD */
 		.dev_id		= "dev:20",
-		.clk		= &osc4_clk,
+		.clk		= &osc4_clk.clk,
 	}
 };
 

[RFC, PATCH 6/7] arm/icst307: use common struct clk, unify realview and versatile clocks

[Jeremy Kerr]

Both the realview and versatile platforms use an icst307 clock, but have
their own clock structures.

This change introduces a struct icst307_clk, which is common between
realview and versatile. This allows us to take out the platform-specific
clock defintions.

Tested on versatile, compiled only on realview.

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

---
 arch/arm/common/icst307.c               |   33 +++++++++++++--
 arch/arm/include/asm/hardware/icst307.h |   18 +++++++-
 arch/arm/mach-realview/Makefile         |    2 
 arch/arm/mach-realview/clock.c          |   51 ------------------------
 arch/arm/mach-realview/clock.h          |   26 ------------
 arch/arm/mach-realview/core.c           |   11 ++---
 arch/arm/mach-realview/realview_pba8.c  |    1 
 arch/arm/mach-versatile/Makefile        |    2 
 arch/arm/mach-versatile/clock.c         |   49 -----------------------
 arch/arm/mach-versatile/clock.h         |   26 ------------
 arch/arm/mach-versatile/core.c          |   11 ++---
 11 files changed, 55 insertions(+), 175 deletions(-)

diff --git a/arch/arm/common/icst307.c b/arch/arm/common/icst307.c
index 6d094c1..4c6cfb4 100644
--- a/arch/arm/common/icst307.c
+++ b/arch/arm/common/icst307.c
@@ -19,6 +19,8 @@
 
 #include <asm/hardware/icst307.h>
 
+#define to_clk_icst307(clk) (container_of(clk, struct clk_icst307, clk))
+
 /*
  * Divisors for each OD setting.
  */
@@ -36,7 +38,7 @@ EXPORT_SYMBOL(icst307_khz);
  */
 static unsigned char idx2s[8] = { 1, 6, 3, 4, 7, 5, 2, 0 };
 
-struct icst307_vco
+static struct icst307_vco
 icst307_khz_to_vco(const struct icst307_params *p, unsigned long freq)
 {
 	struct icst307_vco vco = { .s = 1, .v = p->vd_max, .r = p->rd_max };
@@ -94,9 +96,7 @@ icst307_khz_to_vco(const struct icst307_params *p, unsigned long freq)
 	return vco;
 }
 
-EXPORT_SYMBOL(icst307_khz_to_vco);
-
-struct icst307_vco
+static struct icst307_vco
 icst307_ps_to_vco(const struct icst307_params *p, unsigned long period)
 {
 	struct icst307_vco vco = { .s = 1, .v = p->vd_max, .r = p->rd_max };
@@ -158,4 +158,27 @@ icst307_ps_to_vco(const struct icst307_params *p, unsigned long period)
 	return vco;
 }
 
-EXPORT_SYMBOL(icst307_ps_to_vco);
+unsigned long clk_icst307_get_rate(struct clk *clk)
+{
+	return to_clk_icst307(clk)->rate;
+}
+
+long clk_icst307_round_rate(struct clk *clk, unsigned long rate)
+{
+	const struct icst307_params *params = &to_clk_icst307(clk)->params;
+	struct icst307_vco vco;
+	vco = icst307_khz_to_vco(params, rate / 1000);
+	return icst307_khz(params, vco) * 1000;
+}
+
+int clk_icst307_set_rate(struct clk *clk, unsigned long rate)
+{
+	struct clk_icst307 *v_clk = to_clk_icst307(clk);
+	struct icst307_vco vco;
+
+	vco = icst307_khz_to_vco(&v_clk->params, rate / 1000);
+	v_clk->rate = icst307_khz(&v_clk->params, vco) * 1000;
+	v_clk->setvco(v_clk, vco);
+
+	return 0;
+}
diff --git a/arch/arm/include/asm/hardware/icst307.h b/arch/arm/include/asm/hardware/icst307.h
index 554f128..2dd0255 100644
--- a/arch/arm/include/asm/hardware/icst307.h
+++ b/arch/arm/include/asm/hardware/icst307.h
@@ -16,6 +16,8 @@
 #ifndef ASMARM_HARDWARE_ICST307_H
 #define ASMARM_HARDWARE_ICST307_H
 
+#include <asm/clk.h>
+
 struct icst307_params {
 	unsigned long	ref;
 	unsigned long	vco_max;	/* inclusive */
@@ -31,8 +33,18 @@ struct icst307_vco {
 	unsigned char	s;
 };
 
-unsigned long icst307_khz(const struct icst307_params *p, struct icst307_vco vco);
-struct icst307_vco icst307_khz_to_vco(const struct icst307_params *p, unsigned long freq);
-struct icst307_vco icst307_ps_to_vco(const struct icst307_params *p, unsigned long period);
+struct clk_icst307 {
+	struct clk			clk;
+	unsigned long			rate;
+	const struct icst307_params	params;
+	void				(*setvco)(struct clk_icst307 *,
+						  struct icst307_vco);
+};
+
+unsigned long clk_icst307_get_rate(struct clk *clk);
+
+long clk_icst307_round_rate(struct clk *clk, unsigned long rate);
+
+int clk_icst307_set_rate(struct clk *clk, unsigned long rate);
 
 #endif
diff --git a/arch/arm/mach-realview/Makefile b/arch/arm/mach-realview/Makefile
index e704edb..a01b76b 100644
--- a/arch/arm/mach-realview/Makefile
+++ b/arch/arm/mach-realview/Makefile
@@ -2,7 +2,7 @@
 # Makefile for the linux kernel.
 #
 
-obj-y					:= core.o clock.o
+obj-y					:= core.o
 obj-$(CONFIG_MACH_REALVIEW_EB)		+= realview_eb.o
 obj-$(CONFIG_MACH_REALVIEW_PB11MP)	+= realview_pb11mp.o
 obj-$(CONFIG_MACH_REALVIEW_PB1176)	+= realview_pb1176.o
diff --git a/arch/arm/mach-realview/clock.c b/arch/arm/mach-realview/clock.c
deleted file mode 100644
index 4c09166..0000000
--- a/arch/arm/mach-realview/clock.c
+++ /dev/null
@@ -1,51 +0,0 @@
-/*
- *  linux/arch/arm/mach-realview/clock.c
- *
- *  Copyright (C) 2004 ARM Limited.
- *  Written by Deep Blue Solutions Limited.
- *
- * 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.
- */
-#include <linux/module.h>
-#include <linux/kernel.h>
-#include <linux/device.h>
-#include <linux/list.h>
-#include <linux/errno.h>
-#include <linux/err.h>
-#include <linux/string.h>
-#include <linux/clk.h>
-#include <linux/mutex.h>
-
-#include <asm/clk.h>
-#include <asm/hardware/icst307.h>
-
-#include "clock.h"
-
-#define to_clk_realview(clk) (container_of(clk, struct clk_realview, clk))
-
-unsigned long clk_realview_get_rate(struct clk *clk)
-{
-	return to_clk_realview(clk)->rate;
-}
-
-long clk_realview_round_rate(struct clk *clk, unsigned long rate)
-{
-	const struct icst307_params *params = &to_clk_realview(clk)->params;
-	struct icst307_vco vco;
-	vco = icst307_khz_to_vco(params, rate / 1000);
-	return icst307_khz(params, vco) * 1000;
-}
-
-int clk_realview_set_rate(struct clk *clk, unsigned long rate)
-{
-	struct clk_realview *r_clk = to_clk_realview(clk);
-	struct icst307_vco vco;
-
-	vco = icst307_khz_to_vco(&r_clk->params, rate / 1000);
-	r_clk->rate = icst307_khz(&r_clk->params, vco) * 1000;
-	r_clk->setvco(r_clk, vco);
-
-	return 0;
-}
diff --git a/arch/arm/mach-realview/clock.h b/arch/arm/mach-realview/clock.h
deleted file mode 100644
index b8023f9..0000000
--- a/arch/arm/mach-realview/clock.h
+++ /dev/null
@@ -1,26 +0,0 @@
-/*
- *  linux/arch/arm/mach-realview/clock.h
- *
- *  Copyright (C) 2004 ARM Limited.
- *  Written by Deep Blue Solutions Limited.
- *
- * 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.
- */
-
-#include <asm/clk.h>
-
-struct clk_realview {
-	struct clk			clk;
-	unsigned long			rate;
-	const struct icst307_params	params;
-	void				(*setvco)(struct clk_realview *,
-						  struct icst307_vco);
-};
-
-unsigned long clk_realview_get_rate(struct clk *clk);
-
-long clk_realview_round_rate(struct clk *clk, unsigned long rate);
-
-int clk_realview_set_rate(struct clk *clk, unsigned long rate);
diff --git a/arch/arm/mach-realview/core.c b/arch/arm/mach-realview/core.c
index f57ae12..04712e9 100644
--- a/arch/arm/mach-realview/core.c
+++ b/arch/arm/mach-realview/core.c
@@ -52,7 +52,6 @@
 #include <mach/irqs.h>
 
 #include "core.h"
-#include "clock.h"
 
 #define REALVIEW_REFCOUNTER	(__io_address(REALVIEW_SYS_BASE) + REALVIEW_SYS_24MHz_OFFSET)
 
@@ -274,7 +273,7 @@ struct mmci_platform_data realview_mmc1_plat_data = {
  * Clock handling
  */
 
-static void realview_oscvco_set(struct clk_realview *clk, struct icst307_vco vco)
+static void realview_oscvco_set(struct clk_icst307 *clk, struct icst307_vco vco)
 {
 	void __iomem *sys_lock = __io_address(REALVIEW_SYS_BASE) + REALVIEW_SYS_LOCK_OFFSET;
 	void __iomem *sys_osc;
@@ -293,11 +292,11 @@ static void realview_oscvco_set(struct clk_realview *clk, struct icst307_vco vco
 	writel(0, sys_lock);
 }
 
-static struct clk_realview oscvco_clk = {
+static struct clk_icst307 oscvco_clk = {
 	.clk = {
-		.get_rate = clk_realview_get_rate,
-		.round_rate = clk_realview_round_rate,
-		.set_rate = clk_realview_set_rate,
+		.get_rate = clk_icst307_get_rate,
+		.round_rate = clk_icst307_round_rate,
+		.set_rate = clk_icst307_set_rate,
 	},
 	.params	= {
 		.ref		= 24000,
diff --git a/arch/arm/mach-realview/realview_pba8.c b/arch/arm/mach-realview/realview_pba8.c
index fe861e9..0b43c07 100644
--- a/arch/arm/mach-realview/realview_pba8.c
+++ b/arch/arm/mach-realview/realview_pba8.c
@@ -42,7 +42,6 @@
 #include <mach/irqs.h>
 
 #include "core.h"
-#include "clock.h"
 
 static struct map_desc realview_pba8_io_desc[] __initdata = {
 	{
diff --git a/arch/arm/mach-versatile/Makefile b/arch/arm/mach-versatile/Makefile
index ba81e70..97cf4d8 100644
--- a/arch/arm/mach-versatile/Makefile
+++ b/arch/arm/mach-versatile/Makefile
@@ -2,7 +2,7 @@
 # Makefile for the linux kernel.
 #
 
-obj-y					:= core.o clock.o
+obj-y					:= core.o
 obj-$(CONFIG_ARCH_VERSATILE_PB)		+= versatile_pb.o
 obj-$(CONFIG_MACH_VERSATILE_AB)		+= versatile_ab.o
 obj-$(CONFIG_PCI)			+= pci.o
diff --git a/arch/arm/mach-versatile/clock.c b/arch/arm/mach-versatile/clock.c
deleted file mode 100644
index 9b47e38..0000000
--- a/arch/arm/mach-versatile/clock.c
+++ /dev/null
@@ -1,49 +0,0 @@
-/*
- *  linux/arch/arm/mach-versatile/clock.c
- *
- *  Copyright (C) 2004 ARM Limited.
- *  Written by Deep Blue Solutions Limited.
- *
- * 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.
- */
-#include <linux/module.h>
-#include <linux/kernel.h>
-#include <linux/device.h>
-#include <linux/list.h>
-#include <linux/errno.h>
-#include <linux/err.h>
-#include <linux/string.h>
-#include <linux/clk.h>
-#include <linux/mutex.h>
-
-#include <asm/clk.h>
-#include <asm/hardware/icst307.h>
-
-#include "clock.h"
-
-unsigned long clk_versatile_get_rate(struct clk *clk)
-{
-	return to_clk_versatile(clk)->rate;
-}
-
-long clk_versatile_round_rate(struct clk *clk, unsigned long rate)
-{
-	const struct icst307_params *params = &to_clk_versatile(clk)->params;
-	struct icst307_vco vco;
-	vco = icst307_khz_to_vco(params, rate / 1000);
-	return icst307_khz(params, vco) * 1000;
-}
-
-int clk_versatile_set_rate(struct clk *clk, unsigned long rate)
-{
-	struct clk_versatile *v_clk = to_clk_versatile(clk);
-	struct icst307_vco vco;
-
-	vco = icst307_khz_to_vco(&v_clk->params, rate / 1000);
-	v_clk->rate = icst307_khz(&v_clk->params, vco) * 1000;
-	v_clk->setvco(v_clk, vco);
-
-	return 0;
-}
diff --git a/arch/arm/mach-versatile/clock.h b/arch/arm/mach-versatile/clock.h
deleted file mode 100644
index 569a847..0000000
--- a/arch/arm/mach-versatile/clock.h
+++ /dev/null
@@ -1,26 +0,0 @@
-/*
- *  linux/arch/arm/mach-versatile/clock.h
- *
- *  Copyright (C) 2004 ARM Limited.
- *  Written by Deep Blue Solutions Limited.
- *
- * 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.
- */
-
-struct clk_versatile {
-	struct clk			clk;
-	unsigned long			rate;
-	const struct icst307_params	params;
-	void				(*setvco)(struct clk_versatile *,
-						  struct icst307_vco);
-};
-
-unsigned long clk_versatile_get_rate(struct clk *clk);
-
-long clk_versatile_round_rate(struct clk *clk, unsigned long rate);
-
-int clk_versatile_set_rate(struct clk *clk, unsigned long rate);
-
-#define to_clk_versatile(clk) (container_of(clk, struct clk_versatile, clk))
diff --git a/arch/arm/mach-versatile/core.c b/arch/arm/mach-versatile/core.c
index df1bb4e..7ac1353 100644
--- a/arch/arm/mach-versatile/core.c
+++ b/arch/arm/mach-versatile/core.c
@@ -50,7 +50,6 @@
 #include <asm/mach/map.h>
 
 #include "core.h"
-#include "clock.h"
 
 /*
  * All IO addresses are mapped onto VA 0xFFFx.xxxx, where x.xxxx
@@ -380,7 +379,7 @@ static struct mmci_platform_data mmc0_plat_data = {
  * Clock handling
  */
 
-static void versatile_oscvco_set(struct clk_versatile *clk, struct icst307_vco vco)
+static void versatile_oscvco_set(struct clk_icst307 *clk, struct icst307_vco vco)
 {
 	void __iomem *sys = __io_address(VERSATILE_SYS_BASE);
 	void __iomem *sys_lock = sys + VERSATILE_SYS_LOCK_OFFSET;
@@ -395,11 +394,11 @@ static void versatile_oscvco_set(struct clk_versatile *clk, struct icst307_vco v
 	writel(0, sys_lock);
 }
 
-static struct clk_versatile osc4_clk = {
+static struct clk_icst307 osc4_clk = {
 	.clk = {
-		.get_rate = clk_versatile_get_rate,
-		.round_rate = clk_versatile_round_rate,
-		.set_rate = clk_versatile_set_rate,
+		.get_rate = clk_icst307_get_rate,
+		.round_rate = clk_icst307_round_rate,
+		.set_rate = clk_icst307_set_rate,
 	},
 	.params = {
 		.ref		= 24000,

[RFC, PATCH 6/7] arm/icst307: use common struct clk, unify realview and versatile clocks

[H Hartley Sweeten]

On Thursday, January 07, 2010 4:44 PM, Jeremy Kerr wrote:
> Both the realview and versatile platforms use an icst307 clock, but have
> their own clock structures.
> 
> This change introduces a struct icst307_clk, which is common between
> realview and versatile. This allows us to take out the platform-specific
> clock defintions.
> 
> Tested on versatile, compiled only on realview.
> 
> Signed-off-by: Jeremy Kerr <jeremy.kerr@canonical.com>
> 
> ---

[snip]

> +unsigned long clk_icst307_get_rate(struct clk *clk)
> +{
> +	return to_clk_icst307(clk)->rate;
> +}
> +
> +long clk_icst307_round_rate(struct clk *clk, unsigned long rate)

I think the return value here should be 'unsigned long'

> +{
> +	const struct icst307_params *params = &to_clk_icst307(clk)->params;
> +	struct icst307_vco vco;

Add a whitespace here please.

> +	vco = icst307_khz_to_vco(params, rate / 1000);
> +	return icst307_khz(params, vco) * 1000;
> +}
> +
> +int clk_icst307_set_rate(struct clk *clk, unsigned long rate)
> +{
> +	struct clk_icst307 *v_clk = to_clk_icst307(clk);
> +	struct icst307_vco vco;
> +
> +	vco = icst307_khz_to_vco(&v_clk->params, rate / 1000);
> +	v_clk->rate = icst307_khz(&v_clk->params, vco) * 1000;
> +	v_clk->setvco(v_clk, vco);
> +
> +	return 0;
> +}

Could struct icst307_vco be part of struct clk_icst307?

If so, clk_icst307_set_rate could use clk_icst307_round_rate like this:

int clk_icst307_set_rate(struct clk *clk, unsigned long rate)
{
	struct clk_icst307 *v_clk = to_clk_icst307(clk);

	v_clk->rate = clk_icst307_round_rate(clk, rate);
	v_clk->setvco(v_clk);

	return 0;
}

Regards,
Hartley

[RFC, PATCH 6/7] arm/icst307: use common struct clk, unify realview and versatile clocks

[Jeremy Kerr]

Hi Hartley,

> > +unsigned long clk_icst307_get_rate(struct clk *clk)
> > +{
> > +	return to_clk_icst307(clk)->rate;
> > +}
> > +
> > +long clk_icst307_round_rate(struct clk *clk, unsigned long rate)
> 
> I think the return value here should be 'unsigned long'

linux/clk.h defines clk_round_rate as returning 'long'.

> Could struct icst307_vco be part of struct clk_icst307?
> 
> If so, clk_icst307_set_rate could use clk_icst307_round_rate like this:
> 
> int clk_icst307_set_rate(struct clk *clk, unsigned long rate)
> {
> 	struct clk_icst307 *v_clk = to_clk_icst307(clk);
> 
> 	v_clk->rate = clk_icst307_round_rate(clk, rate);
> 	v_clk->setvco(v_clk);
> 
> 	return 0;
> }

If struct icst307_vco were a member of clk_icst307, then we'd only be using it 
to pass a value between two functions; it's not really a property of the clock 
itself.

But yes, set_rate should use round_rate, I'll update that.

Cheers,


Jeremy

[RFC,PATCH 1/7] arm: add a common struct clk

[H Hartley Sweeten]

On Thursday, January 07, 2010 4:44 PM, Jeremy Kerr wrote:
> We currently have 21 definitions of struct clk in the ARM architecture,
> each defined on a per-platform basis.
> 
> This change is an effort to unify struct clk where possible, by defining
> a common struct clk, containing a set of function pointers. Different
> clock implementations can set their own callbacks, 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_HAVE_COMMON_STRUCT_CLK.
> 
> 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/arm/Kconfig              |    4 +
>  arch/arm/common/Makefile      |    1 
>  arch/arm/common/clk.c         |   78 ++++++++++++++++++++++++++++++++++
>  arch/arm/common/clkdev.c      |    2 
>  arch/arm/include/asm/clk.h    |   37 ++++++++++++++++
>  arch/arm/include/asm/clkdev.h |    2 
>  6 files changed, 123 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 233a222..b6eaf10 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -46,6 +46,10 @@ config GENERIC_CLOCKEVENTS_BROADCAST
>  	depends on GENERIC_CLOCKEVENTS
>  	default y if SMP && !LOCAL_TIMERS
>  
> +config HAVE_COMMON_STRUCT_CLK
> +	bool
> +	default n

default n is not needed

> +
>  config HAVE_TCM
>  	bool
>  	select GENERIC_ALLOCATOR
> diff --git a/arch/arm/common/Makefile b/arch/arm/common/Makefile
> index 76be7ff..6073dbe 100644
> --- a/arch/arm/common/Makefile
> +++ b/arch/arm/common/Makefile
> @@ -17,3 +17,4 @@ obj-$(CONFIG_ARCH_IXP2000)	+= uengine.o
>  obj-$(CONFIG_ARCH_IXP23XX)	+= uengine.o
>  obj-$(CONFIG_PCI_HOST_ITE8152)  += it8152.o
>  obj-$(CONFIG_COMMON_CLKDEV)	+= clkdev.o
> +obj-$(CONFIG_HAVE_COMMON_STRUCT_CLK) += clk.o
> diff --git a/arch/arm/common/clk.c b/arch/arm/common/clk.c
> new file mode 100644
> index 0000000..0357d02
> --- /dev/null
> +++ b/arch/arm/common/clk.c
> @@ -0,0 +1,78 @@
> +/*
> + *  arch/arm/common/clk.c
> + *
> + *  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.
> + *
> + * Helper for the clk API to assist looking up a struct clk.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/err.h>
> +#include <linux/clk.h>
> +#include <asm/clk.h>
> +
> +int clk_enable(struct clk *clk)
> +{

Probably should have here, and in all the other functions, something like:

	if (!clk)
		return -EINVAL;

Also, is any locking needed?

> +	if (clk->enable)
> +		return clk->enable(clk);
> +	return 0;
> +}
> +EXPORT_SYMBOL(clk_enable);
> + 
> +void clk_disable(struct clk *clk)
> +{
> +	if (clk->disable)
> +		clk->disable(clk);
> +}
> +EXPORT_SYMBOL(clk_disable);
> + 
> +unsigned long clk_get_rate(struct clk *clk)
> +{
> +	if (clk->get_rate)
> +		return clk->get_rate(clk);
> +	return 0;

For clocks that are not configurable it might be a good idea to add:

	unsigned long	rate;

to struct clk and then:

	return clk->rate;

for the non-get_rate() condition.

> +}
> +EXPORT_SYMBOL(clk_get_rate);
> + 
> +void clk_put(struct clk *clk)
> +{
> +	if (clk->put)
> +		clk->put(clk);
> +}
> +EXPORT_SYMBOL(clk_put);
> +
> +long clk_round_rate(struct clk *clk, unsigned long rate)
> +{
> +	if (clk->round_rate)
> +		return clk->round_rate(clk, rate);
> +	return -ENOSYS;
> +}
> +EXPORT_SYMBOL(clk_round_rate);
> + 
> +int clk_set_rate(struct clk *clk, unsigned long rate)
> +{
> +	if (clk->set_rate)
> +		return clk->set_rate(clk, rate);
> +	return -ENOSYS;
> +}
> +EXPORT_SYMBOL(clk_set_rate);
> + 
> +int clk_set_parent(struct clk *clk, struct clk *parent)
> +{
> +	if (clk->set_parent)
> +		return clk->set_parent(clk, parent);
> +	return -ENOSYS;
> +}
> +EXPORT_SYMBOL(clk_set_parent);
> +
> +struct clk *clk_get_parent(struct clk *clk)
> +{
> +	if (clk->get_parent)
> +		return clk->get_parent(clk);
> +	return ERR_PTR(-ENOSYS);
> +}
> +EXPORT_SYMBOL(clk_get_parent);
> diff --git a/arch/arm/common/clkdev.c b/arch/arm/common/clkdev.c
> index aae5bc0..bd3866b 100644
> --- a/arch/arm/common/clkdev.c
> +++ b/arch/arm/common/clkdev.c
> @@ -85,11 +85,13 @@ struct clk *clk_get(struct device *dev, const char *con_id)
>  }
>  EXPORT_SYMBOL(clk_get);
>  
> +#ifndef CONFIG_HAVE_COMMON_STRUCT_CLK
>  void clk_put(struct clk *clk)
>  {
>  	__clk_put(clk);
>  }
>  EXPORT_SYMBOL(clk_put);
> +#endif /* CONFIG_HAVE_COMMON_STRUCT_CLK */
>  
>  void clkdev_add(struct clk_lookup *cl)
>  {
> diff --git a/arch/arm/include/asm/clk.h b/arch/arm/include/asm/clk.h
> new file mode 100644
> index 0000000..87fcfae
> --- /dev/null
> +++ b/arch/arm/include/asm/clk.h
> @@ -0,0 +1,37 @@
> +/*
> + *  arch/arm/include/asm/clk.h
> + *
> + *  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.
> + *
> + * Generic definitions for struct clk
> + */
> +#ifndef __ASM_CLK_H
> +#define __ASM_CLK_H
> +
> +#ifdef CONFIG_HAVE_COMMON_STRUCT_CLK
> +
> +/* The definition of struct clk is global, clock providers should
> + * generate "subclasses" embedding struct clk.
> + */
> +struct clk {
> +       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 *);
> +};
> +
> +#else /* CONFIG_HAVE_COMMON_STRUCT_CLK */
> +
> +struct clk;
> +
> +#endif
> +
> +#endif
> diff --git a/arch/arm/include/asm/clkdev.h b/arch/arm/include/asm/clkdev.h
> index b6ec7c6..e994a74 100644
> --- a/arch/arm/include/asm/clkdev.h
> +++ b/arch/arm/include/asm/clkdev.h
> @@ -12,7 +12,7 @@
>  #ifndef __ASM_CLKDEV_H
>  #define __ASM_CLKDEV_H
>  
> -struct clk;
> +#include <asm/clk.h>
>  
>  struct clk_lookup {
>  	struct list_head	node;
 
Regards,
Hartley

[RFC,PATCH 1/7] arm: add a common struct clk

[Jeremy Kerr]

Hi Hartley,

> > +config HAVE_COMMON_STRUCT_CLK
> > +	bool
> > +	default n
> 
> default n is not needed

OK, can remove this.

> Probably should have here, and in all the other functions, something like:
> 
> 	if (!clk)
> 		return -EINVAL;

Is it at all valid to call the clock API functions with a NULL clock?

> Also, is any locking needed?

That's up to the implementer. For example, fixed rate clocks do not require 
locking.

> For clocks that are not configurable it might be a good idea to add:
> 
> 	unsigned long	rate;
> 
> to struct clk and then:
> 
> 	return clk->rate;
> 
> for the non-get_rate() condition.

Check out the clk_fixed patch (2/7), this implements fixed-rate clocks. I'm 
relucant to add anything to struct clock that isn't part of the kernel-wide 
API.

Cheers,


Jeremy

[RFC,PATCH 1/7] arm: add a common struct clk

[Jeremy Kerr]

Hi Hartley,

> I'm relucant to add anything to struct clock that isn't part of the kernel-
> wide API.

This may have been a little cryptic, I'm sorry. The reason for not adding a 
'rate' member to the public clock API is that there would be now two methods 
for getting the rate of a clock: calling clk->get_rate or accessing clk->rate 
directly. The latter is not always guaranteed to work (perhaps there's a clock 
out there that needs an explicit HW query to get the rate), so we don't want 
generic drivers to be able to access this.

Cheers,


Jeremy

[RFC,PATCH 1/7] arm: add a common struct clk

[Russell King - ARM Linux]

On Fri, Jan 08, 2010 at 02:26:09PM +1100, Jeremy Kerr wrote:
> Hi Hartley,
> 
> > I'm relucant to add anything to struct clock that isn't part of the kernel-
> > wide API.
> 
> This may have been a little cryptic, I'm sorry. The reason for not adding a 
> 'rate' member to the public clock API is that there would be now two methods 
> for getting the rate of a clock: calling clk->get_rate or accessing clk->rate 
> directly. The latter is not always guaranteed to work (perhaps there's a clock 
> out there that needs an explicit HW query to get the rate), so we don't want 
> generic drivers to be able to access this.

Having struct clk be a set of function pointers gets really expensive on
some platforms due to the shere number of struct clks - about 900; this
will be a bar to them adopting this not only due to the size but the
problems of ensuring correct initialisation.

Conversely, having those platforms use a pointer to a set of clk operations
structures is also prohibitive - some operations such as enable and disable
can be common, but the rest are extremely variable.

Plus there are platforms where struct clk is _valid_ a NULL pointer; they
only have one clock to deal with.  Why should they need to declare a
struct clk structure just for one fixed clock?

What is clear from the clk API as implemented today is that what's right
for one platform isn't right for another platform.  That's why we have
each platform implementing struct clk in their own way.

[RFC,PATCH 1/7] arm: add a common struct clk

[Benjamin Herrenschmidt]

On Fri, 2010-01-08 at 09:42 +0000, Russell King - ARM Linux wrote:
> What is clear from the clk API as implemented today is that what's
> right
> for one platform isn't right for another platform.  That's why we have
> each platform implementing struct clk in their own way. 

Such platforms would just not set CONFIG_HAVE_COMMON_STRUCT_CLK then.

I think what Jeremy is trying to achieve is a way for platforms that
which to use the device-tree to retrieve the clock binding to be able to
chose to do so, using as much common code as possible.

The problem with the more "static" variants of struct clk is that while
they are probably fine for a single SoC with a reasonably unified clock
mechanism, having more than one clock source programmed differently and
wanting to deal with potentially out-of-SoC clock links between devices
becomes quickly prohibitive without function pointers.

The moment you start having function pointers, you may as well have a
common infrastructure for "indirect" clk ops since that eases code
re-use. Which you do already in some of the ARM platforms afaik.

For example, while discussing the porting of struct clk to powerpc a
while back with some ARM folks, some people told me one issue they had
was to use it to represent clocks between an external i2c PLL chip and
an external device.

Such a scenario becomes trivial to deal with using the device-tree, as
the i2c clock provider "registers" with the device-tree aware clock
infrastructure, which will then transparently, bind the device to the
clock provider and provide the right struct clk to the device.

But again, if a platform choice to use or not to use that facility. If a
platform prefers not to use the facility, for example for performances
reasons, or because of a lot of existing code which would be hard to
update or not worth changing, that platform then just needs to not set
CONFIG_HAVE_COMMON_STRUCT_CLK.

Cheers,
Ben.

[RFC,PATCH 1/7] arm: add a common struct clk

[Russell King - ARM Linux]

On Fri, Jan 08, 2010 at 09:01:13PM +1100, Benjamin Herrenschmidt wrote:
> On Fri, 2010-01-08 at 09:42 +0000, Russell King - ARM Linux wrote:
> > What is clear from the clk API as implemented today is that what's
> > right
> > for one platform isn't right for another platform.  That's why we have
> > each platform implementing struct clk in their own way. 
> 
> Such platforms would just not set CONFIG_HAVE_COMMON_STRUCT_CLK then.
> 
> I think what Jeremy is trying to achieve is a way for platforms that
> which to use the device-tree to retrieve the clock binding to be able to
> chose to do so, using as much common code as possible.

The contents of struct clk has nothing to do with binding though.
The binding of struct clk to devices is totally separate to the actual
contents of struct clk - and it has to be to cope with clocks being
shared between different devices.  So your statement makes no sense.

The binding issue is all about how you go from a driver wanting its
clocks to a representation of that clock which the code can deal with.
That's basically clk_get(), and we currently have that covered both by
both clkdev, and by simpler implementations in some platforms.

What a device-tree based implementation would have to do is provide its
own version of clk_get() and clk_put() - it shouldn't care about the
contents of the struct clk that it's returning at all.

> The problem with the more "static" variants of struct clk is that while
> they are probably fine for a single SoC with a reasonably unified clock
> mechanism, having more than one clock source programmed differently and
> wanting to deal with potentially out-of-SoC clock links between devices
> becomes quickly prohibitive without function pointers.

The "clock source programmed differently" argument I don't buy.  OMAP has
the most complicated clock setup there is on the planet - with lots of
on-chip PLLs, muxes, dividers and switches, all in a heirarchy, and it
seems to cope, propagating changes up and down the tree.

The out-of-SoC problem is something that the clk API doesn't address,
and it remains difficult to address all the time that SoCs can do their
own thing - I'll cover that below.

You really would not want the weight of the OMAP implementation on
something like Versatile.

Now we come to another problem: Versatile is one of the relatively simple
implementations.  It's not a "real" platform in the sense that it's a
development board with a fairly fixed clock relationship - most clocks
are fixed, only some are variable.  It only represents the simple SoCs
like PXA, where the only real control you have is being able to turn
clocks on and off. Apart from the LCD clock, there's not much other
control there.

Modern SoCs are far more complex beasts, with PLLs, dividers, muxes
and so forth - both Samsung S3C and TI OMAP SoCs are good examples of
these.

Basically, there are three classes when it comes to SoC clocks:

1. damned simple, no control what so ever, just need rate info maybe
   for one or two clocks, eg SA1100, LH7A40x, NETX.
2. fixed set of clocks, where the clocks can be turned on/off to
   logical sections of the CPU - eg, PXA, W90X900 etc.
3. complex setups which have a tree of clocks, with muxes, dividers,
   plls etc (Samsung, OMAP).

Trying to get a generic implementation which only addresses some of
those classes is, I believe, very misguided.  Maybe instead we should
be aiming for two or three "common" implementations rather than trying
for a one-size-fits-only-a-small-subset approach?

I don't buy the "don't use it on those platforms then" argument -
because that means not using it on the platforms which are the likely
places people brought up the concerns on - the PXA/Samsung/OMAP types
of platforms.

[RFC,PATCH 1/7] arm: add a common struct clk

[Benjamin Herrenschmidt]

On Fri, 2010-01-08 at 11:18 +0000, Russell King - ARM Linux wrote:
> On Fri, Jan 08, 2010 at 09:01:13PM +1100, Benjamin Herrenschmidt wrote:
> > On Fri, 2010-01-08 at 09:42 +0000, Russell King - ARM Linux wrote:
> > > What is clear from the clk API as implemented today is that what's
> > > right
> > > for one platform isn't right for another platform.  That's why we have
> > > each platform implementing struct clk in their own way. 
> > 
> > Such platforms would just not set CONFIG_HAVE_COMMON_STRUCT_CLK then.
> > 
> > I think what Jeremy is trying to achieve is a way for platforms that
> > which to use the device-tree to retrieve the clock binding to be able to
> > chose to do so, using as much common code as possible.
> 
> The contents of struct clk has nothing to do with binding though.
> The binding of struct clk to devices is totally separate to the actual
> contents of struct clk - and it has to be to cope with clocks being
> shared between different devices.  So your statement makes no sense.

I think you misunderstood me and that we actually agree :-)

See below...

> The binding issue is all about how you go from a driver wanting its
> clocks to a representation of that clock which the code can deal with.
> That's basically clk_get(), and we currently have that covered both by
> both clkdev, and by simpler implementations in some platforms.
> 
> What a device-tree based implementation would have to do is provide its
> own version of clk_get() and clk_put() - it shouldn't care about the
> contents of the struct clk that it's returning at all.

Yes, more or less that's the idea.

IE. The way I envisioned it is that a platform clk_get() can use the
device-tree to get to the clock provider (clock chip driver) own
clk_get() which in turn returns the struct clk.

However, for that concept of "clock object" to work, the struct clk must
carry methods, aka, function pointers. And the intend is to try to
standardize that part of the struct clk that is purely "API" ie. those
function pointers.

My understanding is that Jeremy's patch attempts at more or less
standardizing the way those function pointers are accessed to, and thus
standardize the API-side of the clock object so that clock providers
(aka clock chip drivers) can generate those without the platform needing
to know much about the details of the fields making up a given instance
of a clock object.

I am not too familiar with your clkdev approach, or rather I don't have
the details off the top of my mind right now, but my understanding is
that it's basically the same idea without the device-tree and using a
clock name based scheme.

> > The problem with the more "static" variants of struct clk is that while
> > they are probably fine for a single SoC with a reasonably unified clock
> > mechanism, having more than one clock source programmed differently and
> > wanting to deal with potentially out-of-SoC clock links between devices
> > becomes quickly prohibitive without function pointers.
> 
> The "clock source programmed differently" argument I don't buy.  OMAP has
> the most complicated clock setup there is on the planet - with lots of
> on-chip PLLs, muxes, dividers and switches, all in a heirarchy, and it
> seems to cope, propagating changes up and down the tree.

It does ... as long as you stick to the SoC clocks. You don't have the
ability to have a re-usable clock driver for an external cypress PLL for
example on an i2c bus sourcing some other external device at the same
time.

IE. The way the OMAP code works right now is that basically, all struct
clk has to be handled by the platform, and thus more or less, be the
OMAP SoC clocks. It makes it very hard to -also- use the clk API in the
same platform to deal with other clock net dependencies outside of the
main SoC... unless you can make the API-side of the clock object generic
enough so that the platform can still issue SoC orignated struct clk,
but other clock drivers can also provide their own, different variants
of struct clk, with different callbacks, and the end device doesn't have
to know about it.

> The out-of-SoC problem is something that the clk API doesn't address,
> and it remains difficult to address all the time that SoCs can do their
> own thing - I'll cover that below.

Right, and to some extent I believe Jeremy's patch helps and the
device-tree stuff on top of it has the potential to help even more.

> You really would not want the weight of the OMAP implementation on
> something like Versatile.

Possibly yes, I'm not familiar enough here to really take a stance.

> Now we come to another problem: Versatile is one of the relatively simple
> implementations.  It's not a "real" platform in the sense that it's a
> development board with a fairly fixed clock relationship - most clocks
> are fixed, only some are variable.  It only represents the simple SoCs
> like PXA, where the only real control you have is being able to turn
> clocks on and off. Apart from the LCD clock, there's not much other
> control there.
> 
> Modern SoCs are far more complex beasts, with PLLs, dividers, muxes
> and so forth - both Samsung S3C and TI OMAP SoCs are good examples of
> these.
> 
> Basically, there are three classes when it comes to SoC clocks:
> 
> 1. damned simple, no control what so ever, just need rate info maybe
>    for one or two clocks, eg SA1100, LH7A40x, NETX.
> 2. fixed set of clocks, where the clocks can be turned on/off to
>    logical sections of the CPU - eg, PXA, W90X900 etc.
> 3. complex setups which have a tree of clocks, with muxes, dividers,
>    plls etc (Samsung, OMAP).
> 
> Trying to get a generic implementation which only addresses some of
> those classes is, I believe, very misguided.  Maybe instead we should
> be aiming for two or three "common" implementations rather than trying
> for a one-size-fits-only-a-small-subset approach?

I don't think Jeremy (again he will have to confirm) is aiming toward a
common implementation of the entire clock subsystem. The idea is that
the struct clk is a clock object. You can have many struct clk, they can
have many different capabilities and features. What the patch tries to
do is purely to get the clk_* API -> struct clk part (aka where do you
get the function pointers) common. That way, a platform can indeed
provide a variety of very different clocks with different capabilities
using the same basic infrastructure to expose them.

> I don't buy the "don't use it on those platforms then" argument -
> because that means not using it on the platforms which are the likely
> places people brought up the concerns on - the PXA/Samsung/OMAP types
> of platforms.

The "don't use on those platforms" arguemnt was mostly aimed at
platforms that don't want the overhead of function pointers at all.

My point is that once you have function pointers, aka, different clock
sources with different methods for enable/disable/set_rate/..., then it
make sense to try to have at least the API part of it common, and leave
the differences to the implementation, aka, to what the clock provider
actually instanciate when clk_get() is called.

Look at it that way: struct clk is the base class that provides the
standard API methods. clk_get() returns an instance of a subclass that
can be a larger structure with all sort of SoC specific bits in it, but
at least how to go from struct clk to the ->enable() method is well
defined.

The way I thought of it initially is that clk_get() goes to the platform
which is ultimately responsible for giving out the right clock object.
It may or may not use the device-tree, if it does, then the DT provides
it with a link to a clock provider object which has its own clk_get()
and creates the struct clk & returns it, though it doesn't have to do
so.

Cheers,
Ben.

[RFC,PATCH 1/7] arm: add a common struct clk

[Russell King - ARM Linux]

On Fri, Jan 08, 2010 at 10:45:03PM +1100, Benjamin Herrenschmidt wrote:
> I don't think Jeremy (again he will have to confirm) is aiming toward a
> common implementation of the entire clock subsystem. The idea is that
> the struct clk is a clock object. You can have many struct clk, they can
> have many different capabilities and features.

Taking the two machine types that are most likely to want to use this:

On PXA, there are 67 struct clk's, each of which has a single pointer
to an operations structure.  Each operations structure is three pointers
and there are 11 such structures.  That's 67 * 4 + 11 * 12 = 400 bytes.
PXA is simple enough that these operations structures can be swallowed
up into this struct clk.  So, switching PXA to this will mean 67 * 32 =
2144 bytes.  That's an increase in overhead of 5x.

On OMAP, there are 16 clkops structures of 16 bytes each, and each
struct clk has a pointer to this structure.  Due to the way OMAP works,
these can't be directly called from the proposed generic version
because of the additional clock domain/power domain code, so we can
discount these.

There are two function pointers in the struct clk which would be
identical to the versions proposed in this generic struct clk.
There's a total of 219 clk structures in OMAP3.  So, 219 * (4 + 8)
= 2628.  Switching OMAP means 219 * (4 + 32) = 7884, which is an
increase in overhead of 3x.

This is the problem - you can have "generic" versions of stuff provided
you're willing to put up with the additional overhead that comes with
it.

[RFC,PATCH 1/7] arm: add a common struct clk

[Benjamin Herrenschmidt]

> Taking the two machine types that are most likely to want to use this:
> 
> On PXA, there are 67 struct clk's, each of which has a single pointer
> to an operations structure.  Each operations structure is three pointers
> and there are 11 such structures.  That's 67 * 4 + 11 * 12 = 400 bytes.
> PXA is simple enough that these operations structures can be swallowed
> up into this struct clk.  So, switching PXA to this will mean 67 * 32 =
> 2144 bytes.  That's an increase in overhead of 5x.

Right. Half a page, big deal :-) Now, again, we aren't necessarily
talking about converting existing / old platforms. I believe the main
target here is things like netbooks etc... and the whole new batch of
ARM stuff coming up which has at least half a GB of RAM if not more.

Also a lot of that overhead can be alleviated by using a pointer to an
"ops" structure which can be static in most case. Jeremy, that might be
a better approach if the memory footprint is such a concern.

But again, maybe the PXA platform will not want to use that stuff... it
doesn't -have- to.

Among other things, the idea of having this more flexible infrastructure
and the device-tree is also to allow multiple platforms to be more
easily built as a single kernel, which is something I believe
distributions want to do as much as possible for the incoming batch of
ARM based netbooks, tablets, etc... 

> On OMAP, there are 16 clkops structures of 16 bytes each, and each
> struct clk has a pointer to this structure.  Due to the way OMAP works,
> these can't be directly called from the proposed generic version
> because of the additional clock domain/power domain code, so we can
> discount these.
> 
> There are two function pointers in the struct clk which would be
> identical to the versions proposed in this generic struct clk.
> There's a total of 219 clk structures in OMAP3.  So, 219 * (4 + 8)
> = 2628.  Switching OMAP means 219 * (4 + 32) = 7884, which is an
> increase in overhead of 3x.
> 
> This is the problem - you can have "generic" versions of stuff provided
> you're willing to put up with the additional overhead that comes with
> it.

Right. On the other hand, as you mentioned earlier, the above more
"efficient" way of doing it is also mostly unable to deal with non-SoC
clocks. You cannot for example have a set of generic drivers for a bunch
of existing PLL chips etc...

It may not be a huge deal but I know from some discussions I had in the
past that there -is- interest in that.

So yes, you may use up a bit more memory (and again, I'm curious to see
how much really with an ops pointer) and or a bit more overhead going
through said function pointers, but there are significant benefits in
term of code re-usability between platforms, in some cases
simplification, etc... in addition to the device-tree stuff you may want
to use on top of that which brings its own benefits.

Cheers,
Ben.

[RFC,PATCH 1/7] arm: add a common struct clk

[Jeremy Kerr]

Hi Russell,

> Having struct clk be a set of function pointers gets really expensive on
> some platforms due to the shere number of struct clks - about 900; this
> will be a bar to them adopting this not only due to the size but the
> problems of ensuring correct initialisation.

That's OK, we should only use the common struct clk where it makes sense.

However in the 900 clocks case, I think we have the potential to use similar 
or less memory with a common struct clk that is 'subclassed' by different 
clock types.

As Benh has suggested, I've refactored my patchset to use a clk_operations 
structure:

struct clk_operations {
       int             (*enable)(struct clk *);
       void            (*disable)(struct clk *);
       unsigned long   (*get_rate)(struct clk *);
       void            (*put)(struct clk *);
       long            (*round_rate)(struct clk *, unsigned long);
       int             (*set_rate)(struct clk *, unsigned long);
       int             (*set_parent)(struct clk *, struct clk *);
       struct clk*     (*get_parent)(struct clk *);
};

struct clk {
	const struct clk_operations *ops;
};

This way, each clock will require the operations pointer, plus any clock-
specific fields. The fixed clocks on the system only require an unsigned long; 
more complex clocks get whatever fields they need.

The benefit of this is that we no longer need to allocate structures 
containing a superset of all possible fields. For example, on PXA we're 
reserving space for .delay, .rate  and .cken for all clocks, where only some 
types of clock use these. Subclassing struct clk allows us to only declare 
fields that are relevant to each clock type.

Also, we're now able to define generic clocks, separate from the platform. I 
have done this with icst307, with the realview and versatile platforms sharing 
the generic icst307 clock code. I'm sure there are other areas where we can 
provide similar platform-independent clock definitions.

Currently, there's no way to neatly initialise a struct clk outside of the 
platform code (without making bold assumptions about struct clk); this 
prevents us from parsing clocks directly from the device tree.

Cheers,


Jeremy




    

[RFC,PATCH 1/7] arm: add a common struct clk

[Francesco VIRLINZI]

Hi all
>
>    
I'm following this discussion as I'm interested on clocks API and
  as I wrote a generic clock framework (ELC-E 2009) to link clocks and 
devices and
  to notify the clock rate update to the interested devices
> As Benh has suggested, I've refactored my patchset to use a clk_operations
> structure:
>    
> struct clk_operations {
>         int             (*enable)(struct clk *);
>         void            (*disable)(struct clk *);
>         unsigned long   (*get_rate)(struct clk *);
>         void            (*put)(struct clk *);
>         long            (*round_rate)(struct clk *, unsigned long);
>         int             (*set_rate)(struct clk *, unsigned long);
>         int             (*set_parent)(struct clk *, struct clk *);
>         struct clk*     (*get_parent)(struct clk *);
> };
>
> struct clk {
> 	const struct clk_operations *ops;
> };
>
>    
On this proposal I would suggest:

struct clk_operations {
	int (*init)(struct clk *);	/* used on clk_register and during resume from hibernation */
	...
}
struct clk {
	const struct clk_operations *ops;
	spinlock_t	lock;	/* to serialize the clk_operations */
	const 		*name;
	int 		id;
	unsigned long	rate;		/* used when ops->get_rate is NULL */
};


Regards
  Francesco

[RFC,PATCH 1/7] arm: add a common struct clk

[Benjamin Herrenschmidt]

On Tue, 2010-01-12 at 09:00 +0100, Francesco VIRLINZI wrote:
> struct clk_operations {
>         int (*init)(struct clk *);      /* used on clk_register and
> during resume from hibernation */
>         ...
> }

I believe this is unnecessary. 

The way I see things, the struct clk is provided by clk_get() in one of
two ways:

 - Some static structure the platform is keeping around. In that case
there should be nothing to do for init. In addition, regarding
suspend/resume, those core clocks are way too sensitive beasts to be
left to the generic PM framework. They are likely to require being
turned on/off in a specific sequence or need specific re-init on resume
that only the platform code knows about, so I would leave all of that
there (for SoC clocks at least)

 - Via calling into a "clock provider" which instanciate the struct clk
(or returns a pre-instanciated one). That could be using the device-tree
based infrastructure that I proposed for powerpc and that Jeremy is
attempting to use on ARM, clkdev, or some other mechanism the platform
provides. In this case, the provider's get() method is your init (or
rather whtever it does to instanciate a clock) and is private to a given
"subclass" of struct clk. Again, whatever is needed for suspend/resume
also remains into the realm of that specific implementation of a struct
clk.

> struct clk {
>         const struct clk_operations *ops;
>         spinlock_t      lock;   /* to serialize the clk_operations */
>         const           *name;
>         int             id;
>         unsigned long   rate;           /* used when ops->get_rate is
> NULL */
> };

Here again. Things like lock, name, id, ... are really unnecessary in
many cases and I'd happily leave them to the specific details of a given
struct clk "subclass".

If one wants to find a way to "expose" struct clk in the system into
sysfs or something like that, and as such as a name etc... I would
recommend keeping that a specific CONFIG option which can be disabled as
while I was teasing folks a bit on the "bloat" argument for adding a
couple of function pointers, your proposal would grow the footprint a
lot more significantly.

Besides, the overhead of a spinlock might be utterly uncecessary for a
few clk implementations so why add it to all ?

Cheers,
Ben.

[RFC,PATCH 1/7] arm: add a common struct clk

[H Hartley Sweeten]

On Thursday, January 07, 2010 8:26 PM, Jeremy Kerr wrote:
> Hi Hartley,
>
>> I'm relucant to add anything to struct clock that isn't part of the kernel-
>> wide API.
>
> This may have been a little cryptic, I'm sorry. The reason for not adding a 
> 'rate' member to the public clock API is that there would be now two methods 
> for getting the rate of a clock: calling clk->get_rate or accessing clk->rate 
> directly. The latter is not always guaranteed to work (perhaps there's a clock 
> out there that needs an explicit HW query to get the rate), so we don't want 
> generic drivers to be able to access this.

No, I understood what you meant.  But, thanks for clarifying.

I was just trying to figure out if it was worth trying to use this in
mach-ep93xx.  It looks like it would be pretty straight forward but
the implementation is going to use quite a bit more memory because to the
size of the common struct.

For now I think I will leave ep93xx as it is.

Thanks,
Hartley

[RFC,PATCH 1/7] arm: add a common struct clk

[Benjamin Herrenschmidt]

On Fri, 2010-01-08 at 11:25 -0500, H Hartley Sweeten wrote:
> 
> I was just trying to figure out if it was worth trying to use this in
> mach-ep93xx.  It looks like it would be pretty straight forward but
> the implementation is going to use quite a bit more memory because to the
> size of the common struct.
> 
> For now I think I will leave ep93xx as it is. 

How much memory do you have on that platform ? How much more memory is
going to be used by the implementation ?

It may not be the case in this particular instance but I find it funny
when people complain about something that will use a few hundred more
bytes, maybe one or two k more, on machines that have hundreds of MB of
RAM at the expense of code flexibility, reusability and
maintainability :-)
 
Cheers,
Ben.

[RFC,PATCH 1/7] arm: add a common struct clk

[Benjamin Herrenschmidt]

> +int clk_enable(struct clk *clk)
> +{
> +	if (clk->enable)
> +		return clk->enable(clk);
> +	return 0;
> +}
> +EXPORT_SYMBOL(clk_enable);

 .../...

All those guys look like good candidates to be inlined :-)

Cheers,
Ben.

[RFC,PATCH 1/7] arm: add a common struct clk

[Jeremy Kerr]

Ben,

> > +int clk_enable(struct clk *clk)
> > +{
> > +	if (clk->enable)
> > +		return clk->enable(clk);
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(clk_enable);
> 
>  .../...
> 
> All those guys look like good candidates to be inlined :-)

Good point, will inline these.

Cheers,


Jeremy

[RFC,PATCH 1/7] arm: add a common struct clk

[Jeremy Kerr]

Ben,
 
> Good point, will inline these.

.. and now I remember why I didn't: linux/clk.h defines these as non-static.

Alternative is to move the COMMON_STRUCT_CLK API to general kernel land, and 
put the definitions in the global clk.h.

Cheers,


Jeremy

[RFC,PATCH 1/7] arm: add a common struct clk

[Mark Brown]

On Mon, Jan 11, 2010 at 04:38:12PM +1100, Jeremy Kerr wrote:

> Alternative is to move the COMMON_STRUCT_CLK API to general kernel land, and 
> put the definitions in the global clk.h.

Wouldn't that be sensible anyway?  There's nothing ARM specific about
any of this except that you've used it in some ARM platforms.

[RFC,PATCH 1/7] arm: add a common struct clk

[Benjamin Herrenschmidt]

On Mon, 2010-01-11 at 10:46 +0000, Mark Brown wrote:
> On Mon, Jan 11, 2010 at 04:38:12PM +1100, Jeremy Kerr wrote:
> 
> > Alternative is to move the COMMON_STRUCT_CLK API to general kernel land, and 
> > put the definitions in the global clk.h.
> 
> Wouldn't that be sensible anyway?  There's nothing ARM specific about
> any of this except that you've used it in some ARM platforms.

I wouldn't mind that in order to get the inlines on powerpc as well.

Cheers,
Ben.