arm: add a common struct clk

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 <[email protected]>.

---
 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(-)

index 233a222..b6eaf10 100644
@@ -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

index 76be7ff..6073dbe 100644
@@ -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

index aae5bc0..bd3866b 100644
@@ -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)
 {

new file mode 100644
index 0000000..87fcfae
@@ -0,0 +1,37 @@
+/*
+ *  arch/arm/include/asm/clk.h
+ *
+ *  Copyright (C) 2010 Canonical Ltd <[email protected]>
+ *
+ * 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

index b6ec7c6..e994a74 100644
@@ -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;

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

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

index 0357d02..699bb28 100644
@@ -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);

index 87fcfae..1547fa5 100644
@@ -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;

Use the common struct clk interface for the versatile clocks.

---
 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(-)

index b6eaf10..7e59bce 100644
@@ -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.

index 03468fd..8b0ec44 100644
@@ -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))

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

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

index 8b0ec44..569a847 100644
@@ -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);
 };

index fb66b20..df1bb4e 100644
@@ -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,
 };
 

Use the common struct clk interface for the realview clocks.

Compile tested only.

---
 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(-)

index 7e59bce..a11ee79 100644
@@ -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.

index ebbb0f0..b8023f9 100644
@@ -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);

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.

---
 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(-)

index 6d094c1..4c6cfb4 100644
@@ -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;
+}

index 554f128..2dd0255 100644
@@ -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

index e704edb..a01b76b 100644
@@ -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

deleted file mode 100644
index b8023f9..0000000
@@ -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);

index f57ae12..04712e9 100644
@@ -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,

index fe861e9..0b43c07 100644
@@ -42,7 +42,6 @@
 #include <mach/irqs.h>
 
 #include "core.h"
-#include "clock.h"
 
 static struct map_desc realview_pba8_io_desc[] __initdata = {
 	{

index ba81e70..97cf4d8 100644
@@ -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

deleted file mode 100644
index 569a847..0000000
@@ -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))

index df1bb4e..7ac1353 100644
@@ -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,

[snip]

{
	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;
}

Hi Hartley,

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

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

icst307_ps_to_vco isn't used, so remove it.

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

index 4c6cfb4..b4899c7 100644
@@ -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;

Hi Hartley,

OK, can remove this.

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

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

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

Hi Hartley,

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

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.

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.

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 "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.

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

See below...

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.

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.

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.

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

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.

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.

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.

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...

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.

Hi Russell,

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

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

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

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.

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.

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

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.

Ben,

Good point, will inline these.

Cheers,

Jeremy

Ben,

.. 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

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.