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

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

[Jeremy Kerr]

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

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

---
 include/linux/clk.h |   13 +++++++++++++
 kernel/clk.c        |   14 ++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/include/linux/clk.h b/include/linux/clk.h
index a95cc82..85e1d44 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -90,6 +90,19 @@ static inline void clk_common_init(struct clk *clk)
 	clk->enable_count = 0;
 }
 
+/* Simple fixed-rate clock */
+struct clk_fixed {
+	struct clk	clk;
+	unsigned long	rate;
+};
+
+extern struct clk_ops clk_fixed_ops;
+
+#define INIT_CLK_FIXED(r) { \
+	.clk = INIT_CLK(clk_fixed_ops), \
+	.rate = (r) \
+}
+
 #else /* !CONFIG_USE_COMMON_STRUCT_CLK */
 
 /*
diff --git a/kernel/clk.c b/kernel/clk.c
index cdea25f..32f25ef 100644
--- a/kernel/clk.c
+++ b/kernel/clk.c
@@ -99,3 +99,17 @@ struct clk *clk_get_parent(struct clk *clk)
 	return ERR_PTR(-ENOSYS);
 }
 EXPORT_SYMBOL_GPL(clk_get_parent);
+
+/* clk_fixed support */
+
+#define to_clk_fixed(clk) (container_of(clk, struct clk_fixed, clk))
+
+static unsigned long clk_fixed_get_rate(struct clk *clk)
+{
+	return to_clk_fixed(clk)->rate;
+}
+
+struct clk_ops clk_fixed_ops = {
+	.get_rate = clk_fixed_get_rate,
+};
+EXPORT_SYMBOL_GPL(clk_fixed_ops);

[PATCH 1/3] Add a common struct clk

[Jeremy Kerr]

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

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

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

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

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

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

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

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

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

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

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

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

---
 arch/Kconfig        |    3 +
 include/linux/clk.h |   90 +++++++++++++++++++++++++++++++++++----
 kernel/Makefile     |    1 
 kernel/clk.c        |  101 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 186 insertions(+), 9 deletions(-)

diff --git a/arch/Kconfig b/arch/Kconfig
index acda512..2458b5e 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -151,4 +151,7 @@ config HAVE_MIXED_BREAKPOINTS_REGS
 config HAVE_USER_RETURN_NOTIFIER
 	bool
 
+config USE_COMMON_STRUCT_CLK
+	bool
+
 source "kernel/gcov/Kconfig"
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 1d37f42..a95cc82 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -3,6 +3,7 @@
  *
  *  Copyright (C) 2004 ARM Limited.
  *  Written by Deep Blue Solutions Limited.
+ *  Copyright (c) 2010 Jeremy Kerr <jeremy.kerr@canonical.com>
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
@@ -11,18 +12,95 @@
 #ifndef __LINUX_CLK_H
 #define __LINUX_CLK_H
 
+#include <linux/err.h>
+#include <linux/mutex.h>
+
 struct device;
 
-/*
- * The base API.
+#ifdef CONFIG_USE_COMMON_STRUCT_CLK
+
+/* If we're using the common struct clk, we define the base clk object here */
+
+/**
+ * struct clk - hardware independent clock structure
+ * @clk_ops:		implementation-specific ops for this clock
+ * @enable_count:	count of clk_enable() calls active on this clock
+ * @mutex:		lock for enable/disable or other HW-specific ops
+ *
+ * The base clock object, used by drivers for hardware-independent manipulation
+ * of clock lines. This will be 'subclassed' by device-specific implementations,
+ * which add device-specific data to struct clk. For example:
+ *
+ *  struct clk_foo {
+ *      struct clk;
+ *      [device specific fields]
+ *  };
+ *
+ * The clock driver code will manage the device-specific data, and pass
+ * clk_foo.clk to the common clock code. The clock driver will be called
+ * through the @ops callbacks.
+ *
+ * The @enable_count and @mutex members are initialised when a clock is
+ * registered with the arch-specific clock management code; the clock driver
+ * code does not need to handle these.
  */
+struct clk {
+	const struct clk_ops	*ops;
+	unsigned int		enable_count;
+	struct mutex		mutex;
+};
+
+#define INIT_CLK(o) { .ops = &o, }
+
+struct clk_ops {
+       int		(*enable)(struct clk *);
+       void		(*disable)(struct clk *);
+       int		(*get)(struct clk *);
+       void		(*put)(struct clk *);
+       unsigned long	(*get_rate)(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 *);
+};
 
+/**
+ * __clk_get - update clock-specific refcounter
+ *
+ * @clk: The clock to refcount
+ *
+ * Before a clock is returned from clk_get, this function should be called
+ * to update any clock-specific refcounting.
+ *
+ * Returns non-zero on success, zero on failure.
+ *
+ * Drivers should not need this function; it is only needed by the
+ * arch-specific clk_get() implementations.
+ */
+int __clk_get(struct clk *clk);
+
+/**
+ * clk_common_init - initialise a clock for driver usage
+ *
+ * Used by arch code on registration with the clock infrastructure.
+ */
+static inline void clk_common_init(struct clk *clk)
+{
+	mutex_init(&clk->mutex);
+	clk->enable_count = 0;
+}
+
+#else /* !CONFIG_USE_COMMON_STRUCT_CLK */
 
 /*
- * struct clk - an machine class defined object / cookie.
+ * Global clock object, actual structure is declared per-machine
  */
 struct clk;
 
+static inline void clk_common_init(struct clk *clk) { }
+
+#endif /* !CONFIG_USE_COMMON_STRUCT_CLK */
+
 /**
  * clk_get - lookup and obtain a reference to a clock producer.
  * @dev: device for clock "consumer"
@@ -83,12 +161,6 @@ unsigned long clk_get_rate(struct clk *clk);
  */
 void clk_put(struct clk *clk);
 
-
-/*
- * The remaining APIs are optional for machine class support.
- */
-
-
 /**
  * clk_round_rate - adjust a rate to the exact rate a clock can provide
  * @clk: clock source
diff --git a/kernel/Makefile b/kernel/Makefile
index 057472f..1ae15aa 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -105,6 +105,7 @@ obj-$(CONFIG_PERF_EVENTS) += perf_event.o
 obj-$(CONFIG_HAVE_HW_BREAKPOINT) += hw_breakpoint.o
 obj-$(CONFIG_USER_RETURN_NOTIFIER) += user-return-notifier.o
 obj-$(CONFIG_PADATA) += padata.o
+obj-$(CONFIG_USE_COMMON_STRUCT_CLK) += clk.o
 
 ifneq ($(CONFIG_SCHED_OMIT_FRAME_POINTER),y)
 # According to Alan Modra <alan@linuxcare.com.au>, the -fno-omit-frame-pointer is
diff --git a/kernel/clk.c b/kernel/clk.c
new file mode 100644
index 0000000..cdea25f
--- /dev/null
+++ b/kernel/clk.c
@@ -0,0 +1,101 @@
+/*
+ * Copyright (C) 2010 Canonical Ltd <jeremy.kerr@canonical.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * Standard functionality for the common clock API.
+ */
+
+#include <linux/clk.h>
+#include <linux/mutex.h>
+#include <linux/module.h>
+
+int clk_enable(struct clk *clk)
+{
+	int ret = 0;
+
+	if (!clk->ops->enable)
+		return 0;
+
+	mutex_lock(&clk->mutex);
+	if (!clk->enable_count)
+		ret = clk->ops->enable(clk);
+
+	if (!ret)
+		clk->enable_count++;
+	mutex_unlock(&clk->mutex);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(clk_enable);
+
+void clk_disable(struct clk *clk)
+{
+	if (!clk->ops->disable)
+		return;
+
+	mutex_lock(&clk->mutex);
+
+	if (!--clk->enable_count)
+		clk->ops->disable(clk);
+
+	mutex_unlock(&clk->mutex);
+}
+EXPORT_SYMBOL_GPL(clk_disable);
+
+unsigned long clk_get_rate(struct clk *clk)
+{
+	if (clk->ops->get_rate)
+		return clk->ops->get_rate(clk);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(clk_get_rate);
+
+int __clk_get(struct clk *clk)
+{
+	if (clk->ops->get)
+		return clk->ops->get(clk);
+	return 1;
+}
+EXPORT_SYMBOL_GPL(__clk_get);
+
+void clk_put(struct clk *clk)
+{
+	if (clk->ops->put)
+		clk->ops->put(clk);
+}
+EXPORT_SYMBOL_GPL(clk_put);
+
+long clk_round_rate(struct clk *clk, unsigned long rate)
+{
+	if (clk->ops->round_rate)
+		return clk->ops->round_rate(clk, rate);
+	return -ENOSYS;
+}
+EXPORT_SYMBOL_GPL(clk_round_rate);
+
+int clk_set_rate(struct clk *clk, unsigned long rate)
+{
+	if (clk->ops->set_rate)
+		return clk->ops->set_rate(clk, rate);
+	return -ENOSYS;
+}
+EXPORT_SYMBOL_GPL(clk_set_rate);
+
+int clk_set_parent(struct clk *clk, struct clk *parent)
+{
+	if (clk->ops->set_parent)
+		return clk->ops->set_parent(clk, parent);
+	return -ENOSYS;
+}
+EXPORT_SYMBOL_GPL(clk_set_parent);
+
+struct clk *clk_get_parent(struct clk *clk)
+{
+	if (clk->ops->get_parent)
+		return clk->ops->get_parent(clk);
+	return ERR_PTR(-ENOSYS);
+}
+EXPORT_SYMBOL_GPL(clk_get_parent);

[PATCH 3/3] arm/clkdev: Allow common struct clk usage

[Jeremy Kerr]

A couple of small changes to allow the common struct clk infrastructure
on ARM. The common clock API provides clk_put, and we no longer need
mach/clkdev.h (as we don't need __clk_put anymore).

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

---
 arch/arm/common/clkdev.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm/common/clkdev.c b/arch/arm/common/clkdev.c
index e2b2bb6..d7dc1f8 100644
--- a/arch/arm/common/clkdev.c
+++ b/arch/arm/common/clkdev.c
@@ -21,7 +21,10 @@
 #include <linux/slab.h>
 
 #include <asm/clkdev.h>
+
+#ifndef CONFIG_USE_COMMON_STRUCT_CLK
 #include <mach/clkdev.h>
+#endif
 
 static LIST_HEAD(clocks);
 static DEFINE_MUTEX(clocks_mutex);
@@ -87,14 +90,21 @@ struct clk *clk_get(struct device *dev, const char *con_id)
 }
 EXPORT_SYMBOL(clk_get);
 
+#ifndef CONFIG_USE_COMMON_STRUCT_CLK
+/* For the common struct clk case, clk_put is provided by the kernel-wide
+ * API. */
+
 void clk_put(struct clk *clk)
 {
 	__clk_put(clk);
 }
 EXPORT_SYMBOL(clk_put);
 
+#endif
+
 void clkdev_add(struct clk_lookup *cl)
 {
+	clk_common_init(cl->clk);
 	mutex_lock(&clocks_mutex);
 	list_add_tail(&cl->node, &clocks);
 	mutex_unlock(&clocks_mutex);

[PATCH 0/3] Common struct clk implementation, v7

[Jeremy Kerr]

Hi all,

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

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

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

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

in the clk-common-mx51 branch (clk-common..clk-common-mx51). There is
also a port  for versatile (clk-common-versatile) in this tree too.

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

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

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

Many thanks to the following for their input:
 * Ben Dooks <ben-linux@fluff.org>
 * Baruch Siach <baruch@tkos.co.il>
 * Russell King <linux@arm.linux.org.uk>
 * Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
 * Lorenzo Pieralisi <Lorenzo.Pieralisi@arm.com>

Russell - now that we've had a few platforms ported to the common clk
infrastructure, I believe it's ready to merge. If so, do you want this
in the patch tracker? Otherwise, let me know what needs changing.

This series includes a patch to hook the common clk API into ARM.

Cheers,


Jeremy

--
v7:
 * change CLK_INIT to initialise clk->mutex statically

v6:
 * fixed up references to 'clk_operations' in the changelog

v5:
 * uninline main API, and share definitions with !USE_COMMON_STRUCT_CLK
 * add __clk_get
 * delay mutex init
 * kerneldoc for struct clk

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

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

v2:
 * no longer ARM-specific
 * use clk_operations

---
Jeremy Kerr (3):
      Add a common struct clk
      clk: Generic support for fixed-rate clocks
      arm/clkdev: Allow common struct clk usage

Re: [PATCH 0/3] Common struct clk implementation, v7

[Jean-Christophe PLAGNIOL-VILLARD]

Hi,

	please put sh ml in cc too as we use the same clkdev

Best Regards,
J.
On 11:40 Wed 15 Sep     , Jeremy Kerr wrote:
> Hi all,
> 
> These patches are an attempt to allow platforms to share clock code. At
> present, the definitions of 'struct clk' are local to platform code,
> which makes allocating and initialising cross-platform clock sources
> difficult, and makes it impossible to compile a single image containing
> support for two ARM platforms with different struct clks.
> 
> The two patches are for the architecture-independent kernel code,
> introducing the common clk infrastructure. The changelog for the first
> patch includes details about the new clock definitions.
> 
> As requested by rmk, I've put together a small series of patches
> illustrating the usage of the common struct clock on the ARM imx51
> platform. These are available in my git tree:
> 
>  git://kernel.ubuntu.com/jk/dt/linux-2.6
> 
> in the clk-common-mx51 branch (clk-common..clk-common-mx51). There is
> also a port  for versatile (clk-common-versatile) in this tree too.
> 
> The approach I've taken with the imx51 port is to temporarly duplicate
> the platform-common clock code (ie, for all mxc-based boards) to enable
> usage of the common struct clk on one machine (imx51), while leaving the
> others as-is. For a proper platform-wide usage of the common struct clk,
> we'd be better off doing the whole platform at once. However, mx51 is
> the only mxc-based HW I have, hence the duplicated example port.
> 
> In the example port, the first change simply converts the mxc's struct
> clk to a struct clk_mxc, using the new API.  The subsequent patches move
> certain clocks to more specific data structures (eg clk_fixed and
> clk_pll) where possible.
> 
> Ben Herrenschmidt is looking at using common struct clk code for powerpc
> too, hence the kernel-wide approach.
> 
> Many thanks to the following for their input:
>  * Ben Dooks <ben-linux@fluff.org>
>  * Baruch Siach <baruch@tkos.co.il>
>  * Russell King <linux@arm.linux.org.uk>
>  * Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>  * Lorenzo Pieralisi <Lorenzo.Pieralisi@arm.com>
> 
> Russell - now that we've had a few platforms ported to the common clk
> infrastructure, I believe it's ready to merge. If so, do you want this
> in the patch tracker? Otherwise, let me know what needs changing.
> 
> This series includes a patch to hook the common clk API into ARM.
> 
> Cheers,
> 
> 
> Jeremy
> 
> --
> v7:
>  * change CLK_INIT to initialise clk->mutex statically
> 
> v6:
>  * fixed up references to 'clk_operations' in the changelog
> 
> v5:
>  * uninline main API, and share definitions with !USE_COMMON_STRUCT_CLK
>  * add __clk_get
>  * delay mutex init
>  * kerneldoc for struct clk
> 
> v4:
>  * use mutex for enable/disable locking
>  * DEFINE_CLK -> INIT_CLK, and pass the clk name for mutex init
>  * struct clk_operations -> struct clk_ops
> 
> v3:
>  * do clock usage refcounting in common code
>  * provide sample port
> 
> v2:
>  * no longer ARM-specific
>  * use clk_operations
> 
> ---
> Jeremy Kerr (3):
>       Add a common struct clk
>       clk: Generic support for fixed-rate clocks
>       arm/clkdev: Allow common struct clk usage
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 0/3] Common struct clk implementation, v7

[Jean-Christophe PLAGNIOL-VILLARD]

Hi,

	please put sh ml in cc too as we use the same clkdev

Best Regards,
J.
On 11:40 Wed 15 Sep     , Jeremy Kerr wrote:
> Hi all,
> 
> These patches are an attempt to allow platforms to share clock code. At
> present, the definitions of 'struct clk' are local to platform code,
> which makes allocating and initialising cross-platform clock sources
> difficult, and makes it impossible to compile a single image containing
> support for two ARM platforms with different struct clks.
> 
> The two patches are for the architecture-independent kernel code,
> introducing the common clk infrastructure. The changelog for the first
> patch includes details about the new clock definitions.
> 
> As requested by rmk, I've put together a small series of patches
> illustrating the usage of the common struct clock on the ARM imx51
> platform. These are available in my git tree:
> 
>  git://kernel.ubuntu.com/jk/dt/linux-2.6
> 
> in the clk-common-mx51 branch (clk-common..clk-common-mx51). There is
> also a port  for versatile (clk-common-versatile) in this tree too.
> 
> The approach I've taken with the imx51 port is to temporarly duplicate
> the platform-common clock code (ie, for all mxc-based boards) to enable
> usage of the common struct clk on one machine (imx51), while leaving the
> others as-is. For a proper platform-wide usage of the common struct clk,
> we'd be better off doing the whole platform at once. However, mx51 is
> the only mxc-based HW I have, hence the duplicated example port.
> 
> In the example port, the first change simply converts the mxc's struct
> clk to a struct clk_mxc, using the new API.  The subsequent patches move
> certain clocks to more specific data structures (eg clk_fixed and
> clk_pll) where possible.
> 
> Ben Herrenschmidt is looking at using common struct clk code for powerpc
> too, hence the kernel-wide approach.
> 
> Many thanks to the following for their input:
>  * Ben Dooks <ben-linux@fluff.org>
>  * Baruch Siach <baruch@tkos.co.il>
>  * Russell King <linux@arm.linux.org.uk>
>  * Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
>  * Lorenzo Pieralisi <Lorenzo.Pieralisi@arm.com>
> 
> Russell - now that we've had a few platforms ported to the common clk
> infrastructure, I believe it's ready to merge. If so, do you want this
> in the patch tracker? Otherwise, let me know what needs changing.
> 
> This series includes a patch to hook the common clk API into ARM.
> 
> Cheers,
> 
> 
> Jeremy
> 
> --
> v7:
>  * change CLK_INIT to initialise clk->mutex statically
> 
> v6:
>  * fixed up references to 'clk_operations' in the changelog
> 
> v5:
>  * uninline main API, and share definitions with !USE_COMMON_STRUCT_CLK
>  * add __clk_get
>  * delay mutex init
>  * kerneldoc for struct clk
> 
> v4:
>  * use mutex for enable/disable locking
>  * DEFINE_CLK -> INIT_CLK, and pass the clk name for mutex init
>  * struct clk_operations -> struct clk_ops
> 
> v3:
>  * do clock usage refcounting in common code
>  * provide sample port
> 
> v2:
>  * no longer ARM-specific
>  * use clk_operations
> 
> ---
> Jeremy Kerr (3):
>       Add a common struct clk
>       clk: Generic support for fixed-rate clocks
>       arm/clkdev: Allow common struct clk usage
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/3] Common struct clk implementation, v7

[Jeremy Kerr]

Hi Jean,

> 	please put sh ml in cc too as we use the same clkdev

Sure, the sh folks may be interested in this too; would be nice to share
the common clock with sh.

However, these changes won't affect any platform unless
CONFIG_USE_COMMON_STRUCT_CLK is set; so the changes won't affect sh by
default. When/if this is merged, I'd like to look at integrating other
arches (powerpc and sh). Of course, comments are welcome in the
meantime.

sh folks: to avoid fragmenting the discussion and filling up everyone's
inbox I'll refrain from re-posting this series. Patches are here if
you're interested:

http://kernel.ubuntu.com/git?p=jk/dt/linux-2.6.git;a=shortlog;h=refs/heads/clk-common

Cheers,


Jeremy

[PATCH 0/3] Common struct clk implementation, v7

[Jeremy Kerr]

Hi Jean,

> 	please put sh ml in cc too as we use the same clkdev

Sure, the sh folks may be interested in this too; would be nice to share
the common clock with sh.

However, these changes won't affect any platform unless
CONFIG_USE_COMMON_STRUCT_CLK is set; so the changes won't affect sh by
default. When/if this is merged, I'd like to look at integrating other
arches (powerpc and sh). Of course, comments are welcome in the
meantime.

sh folks: to avoid fragmenting the discussion and filling up everyone's
inbox I'll refrain from re-posting this series. Patches are here if
you're interested:

http://kernel.ubuntu.com/git?p=jk/dt/linux-2.6.git;a=shortlog;h=refs/heads/clk-common

Cheers,


Jeremy

[PATCH 0/3] Common struct clk implementation, v7

[Paulius Zaleckas]

On 09/15/2010 06:40 AM, Jeremy Kerr wrote:
> Hi all,
>
> These patches are an attempt to allow platforms to share clock code. At
> present, the definitions of 'struct clk' are local to platform code,
> which makes allocating and initialising cross-platform clock sources
> difficult, and makes it impossible to compile a single image containing
> support for two ARM platforms with different struct clks.
>
> The two patches are for the architecture-independent kernel code,
> introducing the common clk infrastructure. The changelog for the first
> patch includes details about the new clock definitions.
>
> As requested by rmk, I've put together a small series of patches
> illustrating the usage of the common struct clock on the ARM imx51
> platform. These are available in my git tree:
>
>   git://kernel.ubuntu.com/jk/dt/linux-2.6
>
> in the clk-common-mx51 branch (clk-common..clk-common-mx51). There is
> also a port  for versatile (clk-common-versatile) in this tree too.
>
> The approach I've taken with the imx51 port is to temporarly duplicate
> the platform-common clock code (ie, for all mxc-based boards) to enable
> usage of the common struct clk on one machine (imx51), while leaving the
> others as-is. For a proper platform-wide usage of the common struct clk,
> we'd be better off doing the whole platform at once. However, mx51 is
> the only mxc-based HW I have, hence the duplicated example port.
>
> In the example port, the first change simply converts the mxc's struct
> clk to a struct clk_mxc, using the new API.  The subsequent patches move
> certain clocks to more specific data structures (eg clk_fixed and
> clk_pll) where possible.
>
> Ben Herrenschmidt is looking at using common struct clk code for powerpc
> too, hence the kernel-wide approach.
>
> Many thanks to the following for their input:
>   * Ben Dooks<ben-linux@fluff.org>
>   * Baruch Siach<baruch@tkos.co.il>
>   * Russell King<linux@arm.linux.org.uk>
>   * Uwe Kleine-K?nig<u.kleine-koenig@pengutronix.de>
>   * Lorenzo Pieralisi<Lorenzo.Pieralisi@arm.com>

Funny enough, this morning on my way to work I was thinking exactly
about the same issue! I was very surprised to discover that you just
did it :)

Acked-by: Paulius Zaleckas <paulius.zaleckas@gmail.com>

Re: [PATCH 0/3] Common struct clk implementation, v7

[Colin Cross]

On Tue, Sep 14, 2010 at 8:40 PM, Jeremy Kerr <jeremy.kerr@canonical.com> wrote:
> Hi all,
>
> These patches are an attempt to allow platforms to share clock code. At
> present, the definitions of 'struct clk' are local to platform code,
> which makes allocating and initialising cross-platform clock sources
> difficult, and makes it impossible to compile a single image containing
> support for two ARM platforms with different struct clks.

Having just finished implementing the clock tree support on Tegra, I
have a few concerns about these patches, and how they relate to the
clk api.

On the spinlock vs. mutex issue (I know, it's been beaten to death).
The clk api does not specify whether or not implementations can sleep.
 In fact, it explicitly states that clk_get and clk_put can't be
called from atomic context, but does not say anything about the rest
of the api, which vaguely implies that they can be called from atomic
context.

There are situations in which a non-sleeping clk api is useful, if not
necessary.  On msm with a smart panel, the vsync is connected to a
gpio, and the vsync interrupt enables the dma controller clock is to
start the dma of the framebuffer to the display block.  A similar
problem can happen in dma audio, where a low watermark interrupt from
the audio block needs to trigger dma to refill the buffer.  Leaving
the dma clock on before the vsync or low watermark interrupt would
prevent entering the lowest power idle state.

There is also a very common use case for clk_disable in an interrupt.
When an I2C transaction completes, the irq handler should disable the
clock if there are no other queued transactions.  With a non-sleeping
clk api, this is easy.  With a sleeping clk api, the clk_disable needs
to be deferred to non-atomic context.  Because of the refcounting in
clk_disable, this is not particularly difficult, but a
clk_disable_later helper function might simplify it.

However, some of the clocks on Tegra can not be controlled from atomic
context.  Some clocks have voltage requirements, so enabling the clock
may require a call to the regulator api to increase the voltage, which
will call into the i2c api, which sleeps.

Additionally, using a lock per clk struct can cause AB-BA locking
issues.  In my implementation, calling clk_enable on a child clock in
the clock tree can call clk_enable on its parent if the child clock
was not already enabled.  This leads to the child clock lock being
taken, followed by the parent clock lock.  Calling clk_set_rate on the
parent requires recalculating the rate of all of the children, which
leads to the parent clock lock being taken first, followed by the
child clock lock.  Deadlock.

I solved these problems by using a single spinlock around all of the
clocks, plus an additional mutex around the clocks that need to sleep.
 I also locally added clk_*_cansleep functions to the clk api, and
used might_sleep to prevent calls to the non-cansleep clk api on
clocks that require sleeping.  I'm not happy with this solution, as it
uses a local extension to a cross-platform api, but I don't see any
way around it.  Some clocks really need to be enabled or disabled from
atomic context, and some clocks are impossible to use in atomic
context.

I think something a little like the gpio api style of cansleep and non
cansleep functions would help would preserve the ability for clocks to
avoid sleeping on platforms that don't need to sleep, while allowing
drivers that can handle sleeping clocks to remain as widely
cross-platform as possible.  Instead of changing all the calls, add a
clk_get_nosleep for drivers that will call the clk apis in atomic
context, which will catch drivers that are not compatible with the
clock they are requesting early on during probe.

Either way, the clk api could use some notes on the restrictions for
both the callers and the implementers to ensure compatibility between
architectures.  Which calls can sleep?  Can clk_set_rate or
clk_set_parent be called on a clock that is already enabled, and, if
so, what is the expected behavior for the children of the clock that
is changed?

[PATCH 0/3] Common struct clk implementation, v7

[Colin Cross]

On Tue, Sep 14, 2010 at 8:40 PM, Jeremy Kerr <jeremy.kerr@canonical.com> wrote:
> Hi all,
>
> These patches are an attempt to allow platforms to share clock code. At
> present, the definitions of 'struct clk' are local to platform code,
> which makes allocating and initialising cross-platform clock sources
> difficult, and makes it impossible to compile a single image containing
> support for two ARM platforms with different struct clks.

Having just finished implementing the clock tree support on Tegra, I
have a few concerns about these patches, and how they relate to the
clk api.

On the spinlock vs. mutex issue (I know, it's been beaten to death).
The clk api does not specify whether or not implementations can sleep.
 In fact, it explicitly states that clk_get and clk_put can't be
called from atomic context, but does not say anything about the rest
of the api, which vaguely implies that they can be called from atomic
context.

There are situations in which a non-sleeping clk api is useful, if not
necessary.  On msm with a smart panel, the vsync is connected to a
gpio, and the vsync interrupt enables the dma controller clock is to
start the dma of the framebuffer to the display block.  A similar
problem can happen in dma audio, where a low watermark interrupt from
the audio block needs to trigger dma to refill the buffer.  Leaving
the dma clock on before the vsync or low watermark interrupt would
prevent entering the lowest power idle state.

There is also a very common use case for clk_disable in an interrupt.
When an I2C transaction completes, the irq handler should disable the
clock if there are no other queued transactions.  With a non-sleeping
clk api, this is easy.  With a sleeping clk api, the clk_disable needs
to be deferred to non-atomic context.  Because of the refcounting in
clk_disable, this is not particularly difficult, but a
clk_disable_later helper function might simplify it.

However, some of the clocks on Tegra can not be controlled from atomic
context.  Some clocks have voltage requirements, so enabling the clock
may require a call to the regulator api to increase the voltage, which
will call into the i2c api, which sleeps.

Additionally, using a lock per clk struct can cause AB-BA locking
issues.  In my implementation, calling clk_enable on a child clock in
the clock tree can call clk_enable on its parent if the child clock
was not already enabled.  This leads to the child clock lock being
taken, followed by the parent clock lock.  Calling clk_set_rate on the
parent requires recalculating the rate of all of the children, which
leads to the parent clock lock being taken first, followed by the
child clock lock.  Deadlock.

I solved these problems by using a single spinlock around all of the
clocks, plus an additional mutex around the clocks that need to sleep.
 I also locally added clk_*_cansleep functions to the clk api, and
used might_sleep to prevent calls to the non-cansleep clk api on
clocks that require sleeping.  I'm not happy with this solution, as it
uses a local extension to a cross-platform api, but I don't see any
way around it.  Some clocks really need to be enabled or disabled from
atomic context, and some clocks are impossible to use in atomic
context.

I think something a little like the gpio api style of cansleep and non
cansleep functions would help would preserve the ability for clocks to
avoid sleeping on platforms that don't need to sleep, while allowing
drivers that can handle sleeping clocks to remain as widely
cross-platform as possible.  Instead of changing all the calls, add a
clk_get_nosleep for drivers that will call the clk apis in atomic
context, which will catch drivers that are not compatible with the
clock they are requesting early on during probe.

Either way, the clk api could use some notes on the restrictions for
both the callers and the implementers to ensure compatibility between
architectures.  Which calls can sleep?  Can clk_set_rate or
clk_set_parent be called on a clock that is already enabled, and, if
so, what is the expected behavior for the children of the clock that
is changed?

Re: [PATCH 0/3] Common struct clk implementation, v7

[Paul Mundt]

On Wed, Sep 15, 2010 at 02:08:44PM +0800, Jeremy Kerr wrote:
> sh folks: to avoid fragmenting the discussion and filling up everyone's
> inbox I'll refrain from re-posting this series. Patches are here if
> you're interested:
> 
> http://kernel.ubuntu.com/git?p=jk/dt/linux-2.6.git;a=shortlog;h=refs/heads/clk-common
> 
I've been passively monitoring these patches as they were going by on
l-k, so there aren't really any surprises here. We'll need to do a bit of
refactoring to start using it once it is merged, but that shouldn't be
too problematic.

Right now we're using a shared clock framework between sh and ARM-based
SH-Mobile CPUs (drivers/sh/clk.c and include/linux/sh_clk.h), so both
should convert fairly easily at the same time.

It also doesn't seem like this really poses any problems or overlap with
Jean-Christophe's work for making the clkdev bits generic, so we should
be able to do both independently. If you haven't seen that, it's
currently pending in the ARM patch tracker:

	http://www.arm.linux.org.uk/developer/patches/viewpatch.php?idc90/1

[PATCH 0/3] Common struct clk implementation, v7

[Paul Mundt]

On Wed, Sep 15, 2010 at 02:08:44PM +0800, Jeremy Kerr wrote:
> sh folks: to avoid fragmenting the discussion and filling up everyone's
> inbox I'll refrain from re-posting this series. Patches are here if
> you're interested:
> 
> http://kernel.ubuntu.com/git?p=jk/dt/linux-2.6.git;a=shortlog;h=refs/heads/clk-common
> 
I've been passively monitoring these patches as they were going by on
l-k, so there aren't really any surprises here. We'll need to do a bit of
refactoring to start using it once it is merged, but that shouldn't be
too problematic.

Right now we're using a shared clock framework between sh and ARM-based
SH-Mobile CPUs (drivers/sh/clk.c and include/linux/sh_clk.h), so both
should convert fairly easily at the same time.

It also doesn't seem like this really poses any problems or overlap with
Jean-Christophe's work for making the clkdev bits generic, so we should
be able to do both independently. If you haven't seen that, it's
currently pending in the ARM patch tracker:

	http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=6390/1

Re: [PATCH 0/3] Common struct clk implementation, v7

[Jeremy Kerr]

Hi Colin,

> Having just finished implementing the clock tree support on Tegra, I
> have a few concerns about these patches, and how they relate to the
> clk api.
> 
Thanks for taking a look through the patches - I'm definitely keen to
hear more from those doing implementations of either clock API.

> On the spinlock vs. mutex issue (I know, it's been beaten to death).

I wouldn't say beaten to death, I just haven't heard a good case for the
atomic lock option yet (well, until your email..) :)

> The clk api does not specify whether or not implementations can sleep.
>  In fact, it explicitly states that clk_get and clk_put can't be
> called from atomic context, but does not say anything about the rest
> of the api, which vaguely implies that they can be called from atomic
> context.

At present, clk_enable and clk_disable can only be called from
non-atomic contexts.

All other operations can be called from atomic contexts, *provided that*
the underlying clock implementation does not block. I'm not planning to
enforce any rules about the implementations, so this will probably stay
as an "it depends".

> There are situations in which a non-sleeping clk api is useful, if not
> necessary.  On msm with a smart panel, the vsync is connected to a
> gpio, and the vsync interrupt enables the dma controller clock is to
> start the dma of the framebuffer to the display block.  A similar
> problem can happen in dma audio, where a low watermark interrupt from
> the audio block needs to trigger dma to refill the buffer.

Just to clarify - these two examples definitely can't defer the enable,
right?

> Because of the refcounting in
> clk_disable, this is not particularly difficult, but a
> clk_disable_later helper function might simplify it.

Sounds reasonable; something like clk_disable_later would be good.

> Additionally, using a lock per clk struct can cause AB-BA locking
> issues.  In my implementation, calling clk_enable on a child clock in
> the clock tree can call clk_enable on its parent if the child clock
> was not already enabled.

Yep, and my implementation does the same.

>   This leads to the child clock lock being
> taken, followed by the parent clock lock.  Calling clk_set_rate on the
> parent requires recalculating the rate of all of the children, which
> leads to the parent clock lock being taken first, followed by the
> child clock lock.  Deadlock.

I don't currently enforce any locking in set_rate; if a clock
implementation does locking in this order (child before parent), then
it'd have to use a separate lock, rather than acquiring the clk->mutex
in the set_rate operation.

> I solved these problems by using a single spinlock around all of the
> clocks, plus an additional mutex around the clocks that need to sleep.
>  I also locally added clk_*_cansleep functions to the clk api, and
> used might_sleep to prevent calls to the non-cansleep clk api on
> clocks that require sleeping.  I'm not happy with this solution, as it
> uses a local extension to a cross-platform api, but I don't see any
> way around it.  Some clocks really need to be enabled or disabled from
> atomic context, and some clocks are impossible to use in atomic
> context.
> 
> I think something a little like the gpio api style of cansleep and non
> cansleep functions would help would preserve the ability for clocks to
> avoid sleeping on platforms that don't need to sleep, while allowing
> drivers that can handle sleeping clocks to remain as widely
> cross-platform as possible.  Instead of changing all the calls, add a
> clk_get_nosleep for drivers that will call the clk apis in atomic
> context, which will catch drivers that are not compatible with the
> clock they are requesting early on during probe.
> 
> Either way, the clk api could use some notes on the restrictions for
> both the callers and the implementers to ensure compatibility between
> architectures.

Yes, agreed.

Basically, the reason for choosing a mutex rather than a spinlock is
based on how easy it is to workaround and use it in the "other" context;
ie, how we can use a mutex-based clock in an atomic environment, or use
a spinlock in a non-atomic environment.

With the mutex-based clock, enabling from a atomic environment means
that the callee needs to defer work to non-atomic context, a fairly
standard design pattern which (up until your email) I thought would
cater for all cases.

With a spinlock-based clock, the callee does not need to defer any work,
but it means that the clock implementation needs to enable/disable the
clock atomically. For clock implementations that need to sleep, we can
defer the enabling, but this means that clk_enable() returns before the
clock is actually enabled, which is not acceptable.

If we really do need both locking semantics, it'd be ugly, we could do
something like:

struct clk {
	union {
		struct mutex mutex;
		spintlock_t spinlock;
	} lock;
	const struct clk_ops *ops;
	[...]
};

struct clk_ops {
	void (*lock)(struct clk *clk);
	void (*unlock)(struct clk *clk);
	[ existing clk_ops function pointers ]
};

And provide the corresponding implementations for lock/unlock:

void clk_mutex_lock(struct clk *clk)
{
	mutex_acquire(&clk->lock.mutex);
}

void clk_spinlock_lock(struct clk *clk)
{
	spin_lock(&clk->lock.spinlock);
}

void clk_noop_lock(struct clk *clk)
{
}

Then the implementation can choose the appropriate lock for the clock,
based on whether it needs to sleep to enable & disable. The core code
would then call clk->lock() and clk->unlock() instead of manipulating
clk->mutex directly.

[I've used a union for clarity here - we'd need to do some trickery to
properly initialise the clock's lock statically, or we could drop the
union and steal the mutex's spinlock for the atomic case]

The big downside of this is that the locking semantics are no longer
obvious to the device driver API, which may be getting an atomic clock
or a non-atomic one. As you suggest, we could add a helper:

 clk_get_atomic()

- which would just do a clk_get() and then BUG_ON() if we have the wrong
type.

Any thoughts?

Cheers,


Jeremy

[PATCH 0/3] Common struct clk implementation, v7

[Jeremy Kerr]

Hi Colin,

> Having just finished implementing the clock tree support on Tegra, I
> have a few concerns about these patches, and how they relate to the
> clk api.
> 
Thanks for taking a look through the patches - I'm definitely keen to
hear more from those doing implementations of either clock API.

> On the spinlock vs. mutex issue (I know, it's been beaten to death).

I wouldn't say beaten to death, I just haven't heard a good case for the
atomic lock option yet (well, until your email..) :)

> The clk api does not specify whether or not implementations can sleep.
>  In fact, it explicitly states that clk_get and clk_put can't be
> called from atomic context, but does not say anything about the rest
> of the api, which vaguely implies that they can be called from atomic
> context.

At present, clk_enable and clk_disable can only be called from
non-atomic contexts.

All other operations can be called from atomic contexts, *provided that*
the underlying clock implementation does not block. I'm not planning to
enforce any rules about the implementations, so this will probably stay
as an "it depends".

> There are situations in which a non-sleeping clk api is useful, if not
> necessary.  On msm with a smart panel, the vsync is connected to a
> gpio, and the vsync interrupt enables the dma controller clock is to
> start the dma of the framebuffer to the display block.  A similar
> problem can happen in dma audio, where a low watermark interrupt from
> the audio block needs to trigger dma to refill the buffer.

Just to clarify - these two examples definitely can't defer the enable,
right?

> Because of the refcounting in
> clk_disable, this is not particularly difficult, but a
> clk_disable_later helper function might simplify it.

Sounds reasonable; something like clk_disable_later would be good.

> Additionally, using a lock per clk struct can cause AB-BA locking
> issues.  In my implementation, calling clk_enable on a child clock in
> the clock tree can call clk_enable on its parent if the child clock
> was not already enabled.

Yep, and my implementation does the same.

>   This leads to the child clock lock being
> taken, followed by the parent clock lock.  Calling clk_set_rate on the
> parent requires recalculating the rate of all of the children, which
> leads to the parent clock lock being taken first, followed by the
> child clock lock.  Deadlock.

I don't currently enforce any locking in set_rate; if a clock
implementation does locking in this order (child before parent), then
it'd have to use a separate lock, rather than acquiring the clk->mutex
in the set_rate operation.

> I solved these problems by using a single spinlock around all of the
> clocks, plus an additional mutex around the clocks that need to sleep.
>  I also locally added clk_*_cansleep functions to the clk api, and
> used might_sleep to prevent calls to the non-cansleep clk api on
> clocks that require sleeping.  I'm not happy with this solution, as it
> uses a local extension to a cross-platform api, but I don't see any
> way around it.  Some clocks really need to be enabled or disabled from
> atomic context, and some clocks are impossible to use in atomic
> context.
> 
> I think something a little like the gpio api style of cansleep and non
> cansleep functions would help would preserve the ability for clocks to
> avoid sleeping on platforms that don't need to sleep, while allowing
> drivers that can handle sleeping clocks to remain as widely
> cross-platform as possible.  Instead of changing all the calls, add a
> clk_get_nosleep for drivers that will call the clk apis in atomic
> context, which will catch drivers that are not compatible with the
> clock they are requesting early on during probe.
> 
> Either way, the clk api could use some notes on the restrictions for
> both the callers and the implementers to ensure compatibility between
> architectures.

Yes, agreed.

Basically, the reason for choosing a mutex rather than a spinlock is
based on how easy it is to workaround and use it in the "other" context;
ie, how we can use a mutex-based clock in an atomic environment, or use
a spinlock in a non-atomic environment.

With the mutex-based clock, enabling from a atomic environment means
that the callee needs to defer work to non-atomic context, a fairly
standard design pattern which (up until your email) I thought would
cater for all cases.

With a spinlock-based clock, the callee does not need to defer any work,
but it means that the clock implementation needs to enable/disable the
clock atomically. For clock implementations that need to sleep, we can
defer the enabling, but this means that clk_enable() returns before the
clock is actually enabled, which is not acceptable.

If we really do need both locking semantics, it'd be ugly, we could do
something like:

struct clk {
	union {
		struct mutex mutex;
		spintlock_t spinlock;
	} lock;
	const struct clk_ops *ops;
	[...]
};

struct clk_ops {
	void (*lock)(struct clk *clk);
	void (*unlock)(struct clk *clk);
	[ existing clk_ops function pointers ]
};

And provide the corresponding implementations for lock/unlock:

void clk_mutex_lock(struct clk *clk)
{
	mutex_acquire(&clk->lock.mutex);
}

void clk_spinlock_lock(struct clk *clk)
{
	spin_lock(&clk->lock.spinlock);
}

void clk_noop_lock(struct clk *clk)
{
}

Then the implementation can choose the appropriate lock for the clock,
based on whether it needs to sleep to enable & disable. The core code
would then call clk->lock() and clk->unlock() instead of manipulating
clk->mutex directly.

[I've used a union for clarity here - we'd need to do some trickery to
properly initialise the clock's lock statically, or we could drop the
union and steal the mutex's spinlock for the atomic case]

The big downside of this is that the locking semantics are no longer
obvious to the device driver API, which may be getting an atomic clock
or a non-atomic one. As you suggest, we could add a helper:

 clk_get_atomic()

- which would just do a clk_get() and then BUG_ON() if we have the wrong
type.

Any thoughts?

Cheers,


Jeremy

[PATCH 1/3] Add a common struct clk

[Jassi Brar]

On Wed, Sep 15, 2010 at 12:40 PM, Jeremy Kerr <jeremy.kerr@canonical.com> wrote:
> We currently have 21 definitions of struct clk in the ARM architecture,
> each defined on a per-platform basis. This makes it difficult to define
> platform- (or architecture-) independent clock sources without making
> assumptions about struct clk, and impossible to compile two
> platforms with different struct clks into a single image.
>
> This change is an effort to unify struct clk where possible, by defining
> a common struct clk, containing a set of clock operations. Different
> clock implementations can set their own operations, and have a standard
> interface for generic code. The callback interface is exposed to the
> kernel proper, while the clock implementations only need to be seen by
> the platform internals.
>
> This allows us to share clock code among platforms, and makes it
> possible to dynamically create clock devices in platform-independent
> code.
>
> Platforms can enable the generic struct clock through
> CONFIG_USE_COMMON_STRUCT_CLK. In this case, the clock infrastructure
> consists of a common struct clk:
>
> struct clk {
> ? ? ? ?const struct clk_ops ? ?*ops;
> ? ? ? ?unsigned int ? ? ? ? ? ?enable_count;
> ? ? ? ?struct mutex ? ? ? ? ? ?mutex;
> };
>
> And a set of clock operations (defined per type of clock):
>
> struct clk_ops {
> ? ? ? int ? ? ? ? ? ? (*enable)(struct clk *);
> ? ? ? void ? ? ? ? ? ?(*disable)(struct clk *);
> ? ? ? unsigned long ? (*get_rate)(struct clk *);
> ? ? ? [...]
> };
>
> To define a hardware-specific clock, machine code can "subclass" the
> struct clock into a new struct (adding any device-specific data), and
> provide a set of operations:
>
> struct clk_foo {
> ? ? ? ?struct clk ? ? ?clk;
> ? ? ? ?void __iomem ? ?*some_register;
> };
>
> struct clk_ops clk_foo_ops = {
> ? ? ? ?.get_rate = clk_foo_get_rate,
> };
>
> The common clock definitions are based on a development patch from Ben
> Herrenschmidt <benh@kernel.crashing.org>.
>
> Signed-off-by: Jeremy Kerr <jeremy.kerr@canonical.com>

I am not sure about the scope of your assumed task, but I would still
like to bring
to your notice some points.

IMHO any clock api that aims to be used by multiple platforms should be generic
more than this one. Let me explain.
Clocks that are simply output of MUX'es inherit the rate of the parent
clock - they do have
rate yet this API would return 0 (IIRUIC).
Another scenario could have a source clock parenting more than one
child clock via
different MUX'es. It would be nice to be able to set rate of that
parent clock (after ensuring
other siblings won't complain) and have some mechanism to notify other
siblings of the change.
Or some better solution.

[PATCH 1/3] Add a common struct clk

[Jeremy Kerr]

Hi Jassi,

> I am not sure about the scope of your assumed task, but I would still
> like to bring
> to your notice some points.
> 
> IMHO any clock api that aims to be used by multiple platforms should be generic
> more than this one. Let me explain.

None of my patches change the clock API itself. The clock API is already
in use on ARM (ie, multiple platforms) and seems to work for most cases.

> Clocks that are simply output of MUX'es inherit the rate of the parent
> clock - they do have
> rate yet this API would return 0 (IIRUIC).

The proposed clock infrastructure does not provide implementations for
the clock operation functions (like get_rate, which you're referring to
here). The behaviour of the clocks is dictated by each implementation of
struct clk. For MUXes, the callback for clk->ops.get_rate would be
something like:

unsigned long clk_mux_get_rate(struct clk *clk)
{
	return clk_get_rate(to_clk_mux(clk)->parent);
}

- so it just returns the rate of the parent (not 0). However, if the
clock implementation has a reason to return 0, it is free to do so.

> Another scenario could have a source clock parenting more than one
> child clock via
> different MUX'es. It would be nice to be able to set rate of that
> parent clock (after ensuring
> other siblings won't complain) and have some mechanism to notify other
> siblings of the change.
> Or some better solution.

Again, these patches don't change the API itself; if we want to
implement clock-change notifications, that would best be done in a
separate discussion.

Cheers,


Jeremy

[PATCH 1/3] Add a common struct clk

[Jassi Brar]

On Fri, Sep 17, 2010 at 9:24 AM, Jeremy Kerr <jeremy.kerr@canonical.com> wrote:
> Hi Jassi,
>
>> I am not sure about the scope of your assumed task, but I would still
>> like to bring
>> to your notice some points.
>>
>> IMHO any clock api that aims to be used by multiple platforms should be generic
>> more than this one. Let me explain.
>
> None of my patches change the clock API itself. The clock API is already
> in use on ARM (ie, multiple platforms) and seems to work for most cases.
>
>> Clocks that are simply output of MUX'es inherit the rate of the parent
>> clock - they do have
>> rate yet this API would return 0 (IIRUIC).
>
> The proposed clock infrastructure does not provide implementations for
> the clock operation functions (like get_rate, which you're referring to
> here). The behaviour of the clocks is dictated by each implementation of
> struct clk. For MUXes, the callback for clk->ops.get_rate would be
> something like:
>
> unsigned long clk_mux_get_rate(struct clk *clk)
> {
> ? ? ? ?return clk_get_rate(to_clk_mux(clk)->parent);
> }
>
> - so it just returns the rate of the parent (not 0). However, if the
> clock implementation has a reason to return 0, it is free to do so.

IMHO, the API needs to define a more generic clock that covers the 'classic'
and mux'ed clock by same data structure.
That implies fixing the API and modify the drivers (if need be) rather than have
multiple implementations of API. Let platforms move to the new one at their own
pace.
But, as you said, this is perhaps a matter for different thread.

regards.

[PATCH 1/3] Add a common struct clk

[Jassi Brar]

On Fri, Sep 17, 2010 at 9:55 AM, Jassi Brar <jassisinghbrar@gmail.com> wrote:
> On Fri, Sep 17, 2010 at 9:24 AM, Jeremy Kerr <jeremy.kerr@canonical.com> wrote:
>> Hi Jassi,
>>
>>> I am not sure about the scope of your assumed task, but I would still
>>> like to bring
>>> to your notice some points.
>>>
>>> IMHO any clock api that aims to be used by multiple platforms should be generic
>>> more than this one. Let me explain.
>>
>> None of my patches change the clock API itself. The clock API is already
>> in use on ARM (ie, multiple platforms) and seems to work for most cases.
>>
>>> Clocks that are simply output of MUX'es inherit the rate of the parent
>>> clock - they do have
>>> rate yet this API would return 0 (IIRUIC).
>>
>> The proposed clock infrastructure does not provide implementations for
>> the clock operation functions (like get_rate, which you're referring to
>> here). The behaviour of the clocks is dictated by each implementation of
>> struct clk. For MUXes, the callback for clk->ops.get_rate would be
>> something like:
>>
>> unsigned long clk_mux_get_rate(struct clk *clk)
>> {
>> ? ? ? ?return clk_get_rate(to_clk_mux(clk)->parent);
>> }
>>
>> - so it just returns the rate of the parent (not 0). However, if the
>> clock implementation has a reason to return 0, it is free to do so.
>
> IMHO, the API needs to define a more generic clock that covers the 'classic'
> and mux'ed clock by same data structure.
> That implies fixing the API and modify the drivers (if need be) rather than have
> multiple implementations of API. Let platforms move to the new one at their own
> pace.
In retrospect, I think I needed to add more...
Clock API should assume that output clock of a scaling block provides
it's clk_get/set_rate,
while output clock of a MUX/Input-Pad are indicated by absence of
clk_get/set_rate.
IMHO, most users would want to set/get rate of the parent in latter
case because that
tells the reality better. You might want to do that by default in the
clk_get/set_rate function. Let the exception(0 return) be defined in
the clk's get_rate.

+unsigned long clk_get_rate(struct clk *clk)
+{
+       if (clk->ops->get_rate)
+               return clk->ops->get_rate(clk);
+      else
+               return clk_get_rate(clk->parent);
+}

The assumed definition of clock also dictates the enable/disable be
recursively called for the parent as well -- input to scaling block or
a MUX need
not be kept enabled if none needs it or be disabled while someone needs it.

I think these assumptions are modest and (may I daresay) represent
majority, while
still leaving room for exceptions(MUX output implement it's
get/set_rate, enable/disable
to override them).
I feel relieved :)

Regards.

Re: [PATCH 0/3] Common struct clk implementation, v7

[Ben Dooks]

On 16/09/10 00:15, Colin Cross wrote:
> On Tue, Sep 14, 2010 at 8:40 PM, Jeremy Kerr <jeremy.kerr@canonical.com> wrote:
>> Hi all,
>>
>> These patches are an attempt to allow platforms to share clock code. At
>> present, the definitions of 'struct clk' are local to platform code,
>> which makes allocating and initialising cross-platform clock sources
>> difficult, and makes it impossible to compile a single image containing
>> support for two ARM platforms with different struct clks.
> 
> Having just finished implementing the clock tree support on Tegra, I
> have a few concerns about these patches, and how they relate to the
> clk api.
> 
> On the spinlock vs. mutex issue (I know, it's been beaten to death).
> The clk api does not specify whether or not implementations can sleep.
>  In fact, it explicitly states that clk_get and clk_put can't be
> called from atomic context, but does not say anything about the rest
> of the api, which vaguely implies that they can be called from atomic
> context.

Personally, I took the view that they where never allowable from an
atomic context. We may need to lock the relevant clock hardware and
as such it makes it a bit of a headache if we have clock implementations
which communicate via something like i2c or spi. It could be that
clocks are sourced form some form of PMIC, PLL or other external device.

> There are situations in which a non-sleeping clk api is useful, if not
> necessary.  On msm with a smart panel, the vsync is connected to a
> gpio, and the vsync interrupt enables the dma controller clock is to
> start the dma of the framebuffer to the display block.  A similar
> problem can happen in dma audio, where a low watermark interrupt from
> the audio block needs to trigger dma to refill the buffer.  Leaving
> the dma clock on before the vsync or low watermark interrupt would
> prevent entering the lowest power idle state.

Is the DMA clock seperate from the device bus clock in this case?

> There is also a very common use case for clk_disable in an interrupt.
> When an I2C transaction completes, the irq handler should disable the
> clock if there are no other queued transactions.  With a non-sleeping
> clk api, this is easy.  With a sleeping clk api, the clk_disable needs
> to be deferred to non-atomic context.  Because of the refcounting in
> clk_disable, this is not particularly difficult, but a
> clk_disable_later helper function might simplify it.

IIRC, the I2C APIs aren't call-able from an atomic context, so the
clk_enable() and clk_disable() calls could be done from the context
initiating and completing the transaction, and not the interrupt
handler.

> However, some of the clocks on Tegra can not be controlled from atomic
> context.  Some clocks have voltage requirements, so enabling the clock
> may require a call to the regulator api to increase the voltage, which
> will call into the i2c api, which sleeps.
>
> Additionally, using a lock per clk struct can cause AB-BA locking
> issues.  In my implementation, calling clk_enable on a child clock in
> the clock tree can call clk_enable on its parent if the child clock
> was not already enabled.  This leads to the child clock lock being
> taken, followed by the parent clock lock.  Calling clk_set_rate on the
> parent requires recalculating the rate of all of the children, which
> leads to the parent clock lock being taken first, followed by the
> child clock lock.  Deadlock.

Hmm, either the lock has to be explicitly for the 'enable' count or
we end up with some really nasty locking problems.

> I solved these problems by using a single spinlock around all of the
> clocks, plus an additional mutex around the clocks that need to sleep.
>  I also locally added clk_*_cansleep functions to the clk api, and
> used might_sleep to prevent calls to the non-cansleep clk api on
> clocks that require sleeping.  I'm not happy with this solution, as it
> uses a local extension to a cross-platform api, but I don't see any
> way around it.  Some clocks really need to be enabled or disabled from
> atomic context, and some clocks are impossible to use in atomic
> context.

I'd like to see some comment about the use of the clk_ api from atomic
context, and then sorting this out before we try and get a unified API
sorted out.

> I think something a little like the gpio api style of cansleep and non
> cansleep functions would help would preserve the ability for clocks to
> avoid sleeping on platforms that don't need to sleep, while allowing
> drivers that can handle sleeping clocks to remain as widely
> cross-platform as possible.  Instead of changing all the calls, add a
> clk_get_nosleep for drivers that will call the clk apis in atomic
> context, which will catch drivers that are not compatible with the
> clock they are requesting early on during probe.
> 
> Either way, the clk api could use some notes on the restrictions for
> both the callers and the implementers to ensure compatibility between
> architectures.  Which calls can sleep?  Can clk_set_rate or
> clk_set_parent be called on a clock that is already enabled, and, if
> so, what is the expected behavior for the children of the clock that
> is changed?

Some of that is implementation specific, IIRC if they can't return an
error they should be able to. In the case of most of the Samsung code
the user is allowed to set_parent and set_rate on a running clock, and
in some cases the clock has to be temporarily stopped whilst the MUX
change is being sorted out.

For the child case, on the Samsung code it is very rare that we've
ended up having to change the rate of the parent clocks, as we often
have fixed-rate sources for the child clocks, and thus use these as
a really useful stable source. Often the set_rate and the set_parent
calls are being done on the child clocks directly connected to the
peripherals.

I would propose that if we do have a common clk_api, then we have a
common set of notifiers (a-la, or use the current in kernel ones)
that can be called when a clock changes rate so that drivers can take
notice of this. It may be also a good idea for drivers to register
a range of clock rates they can tolerate so that setting the rate of
the parent can take these into account.

(On the above, I belive someone at ST has alreayd been looking at
 this sort of thing, but their implementation was rather large and
 needed a pile of tidying up).

However, I think we should carefully sort out who handles the set_parent
implementation, whether it is in the clk core api, or has at-least
a useful helper in there to ensure that the following is met:

- when parent changes, the parent's child count is changed
- if the child is enabled, the following needs to be done:
  - call clk_enable() for the new parent
  - call the implementation's set_parent
  - call clk_disbale() for the old parent

I believe this sort of thing is very useful to have in the core
implemetation, as it is the sort of thing people will get wrong
on a regular basis, as well as being common to all but the simplest
of clock implementations.

note, this may then cause problems on how you deal with a system
which has booted and is just setting the clocks up, as we may end
up disabling a clock that is being used elsewhere (but we just
don't know it).

second note, this is also even more fun for resume, where clock
control registers may not have been kept over resume. Simply
rewriting these registers from store may end up causing problems
with glitching clocks, etc.

[PATCH 0/3] Common struct clk implementation, v7

[Ben Dooks]

On 16/09/10 00:15, Colin Cross wrote:
> On Tue, Sep 14, 2010 at 8:40 PM, Jeremy Kerr <jeremy.kerr@canonical.com> wrote:
>> Hi all,
>>
>> These patches are an attempt to allow platforms to share clock code. At
>> present, the definitions of 'struct clk' are local to platform code,
>> which makes allocating and initialising cross-platform clock sources
>> difficult, and makes it impossible to compile a single image containing
>> support for two ARM platforms with different struct clks.
> 
> Having just finished implementing the clock tree support on Tegra, I
> have a few concerns about these patches, and how they relate to the
> clk api.
> 
> On the spinlock vs. mutex issue (I know, it's been beaten to death).
> The clk api does not specify whether or not implementations can sleep.
>  In fact, it explicitly states that clk_get and clk_put can't be
> called from atomic context, but does not say anything about the rest
> of the api, which vaguely implies that they can be called from atomic
> context.

Personally, I took the view that they where never allowable from an
atomic context. We may need to lock the relevant clock hardware and
as such it makes it a bit of a headache if we have clock implementations
which communicate via something like i2c or spi. It could be that
clocks are sourced form some form of PMIC, PLL or other external device.

> There are situations in which a non-sleeping clk api is useful, if not
> necessary.  On msm with a smart panel, the vsync is connected to a
> gpio, and the vsync interrupt enables the dma controller clock is to
> start the dma of the framebuffer to the display block.  A similar
> problem can happen in dma audio, where a low watermark interrupt from
> the audio block needs to trigger dma to refill the buffer.  Leaving
> the dma clock on before the vsync or low watermark interrupt would
> prevent entering the lowest power idle state.

Is the DMA clock seperate from the device bus clock in this case?

> There is also a very common use case for clk_disable in an interrupt.
> When an I2C transaction completes, the irq handler should disable the
> clock if there are no other queued transactions.  With a non-sleeping
> clk api, this is easy.  With a sleeping clk api, the clk_disable needs
> to be deferred to non-atomic context.  Because of the refcounting in
> clk_disable, this is not particularly difficult, but a
> clk_disable_later helper function might simplify it.

IIRC, the I2C APIs aren't call-able from an atomic context, so the
clk_enable() and clk_disable() calls could be done from the context
initiating and completing the transaction, and not the interrupt
handler.

> However, some of the clocks on Tegra can not be controlled from atomic
> context.  Some clocks have voltage requirements, so enabling the clock
> may require a call to the regulator api to increase the voltage, which
> will call into the i2c api, which sleeps.
>
> Additionally, using a lock per clk struct can cause AB-BA locking
> issues.  In my implementation, calling clk_enable on a child clock in
> the clock tree can call clk_enable on its parent if the child clock
> was not already enabled.  This leads to the child clock lock being
> taken, followed by the parent clock lock.  Calling clk_set_rate on the
> parent requires recalculating the rate of all of the children, which
> leads to the parent clock lock being taken first, followed by the
> child clock lock.  Deadlock.

Hmm, either the lock has to be explicitly for the 'enable' count or
we end up with some really nasty locking problems.

> I solved these problems by using a single spinlock around all of the
> clocks, plus an additional mutex around the clocks that need to sleep.
>  I also locally added clk_*_cansleep functions to the clk api, and
> used might_sleep to prevent calls to the non-cansleep clk api on
> clocks that require sleeping.  I'm not happy with this solution, as it
> uses a local extension to a cross-platform api, but I don't see any
> way around it.  Some clocks really need to be enabled or disabled from
> atomic context, and some clocks are impossible to use in atomic
> context.

I'd like to see some comment about the use of the clk_ api from atomic
context, and then sorting this out before we try and get a unified API
sorted out.

> I think something a little like the gpio api style of cansleep and non
> cansleep functions would help would preserve the ability for clocks to
> avoid sleeping on platforms that don't need to sleep, while allowing
> drivers that can handle sleeping clocks to remain as widely
> cross-platform as possible.  Instead of changing all the calls, add a
> clk_get_nosleep for drivers that will call the clk apis in atomic
> context, which will catch drivers that are not compatible with the
> clock they are requesting early on during probe.
> 
> Either way, the clk api could use some notes on the restrictions for
> both the callers and the implementers to ensure compatibility between
> architectures.  Which calls can sleep?  Can clk_set_rate or
> clk_set_parent be called on a clock that is already enabled, and, if
> so, what is the expected behavior for the children of the clock that
> is changed?

Some of that is implementation specific, IIRC if they can't return an
error they should be able to. In the case of most of the Samsung code
the user is allowed to set_parent and set_rate on a running clock, and
in some cases the clock has to be temporarily stopped whilst the MUX
change is being sorted out.

For the child case, on the Samsung code it is very rare that we've
ended up having to change the rate of the parent clocks, as we often
have fixed-rate sources for the child clocks, and thus use these as
a really useful stable source. Often the set_rate and the set_parent
calls are being done on the child clocks directly connected to the
peripherals.

I would propose that if we do have a common clk_api, then we have a
common set of notifiers (a-la, or use the current in kernel ones)
that can be called when a clock changes rate so that drivers can take
notice of this. It may be also a good idea for drivers to register
a range of clock rates they can tolerate so that setting the rate of
the parent can take these into account.

(On the above, I belive someone at ST has alreayd been looking at
 this sort of thing, but their implementation was rather large and
 needed a pile of tidying up).

However, I think we should carefully sort out who handles the set_parent
implementation, whether it is in the clk core api, or has at-least
a useful helper in there to ensure that the following is met:

- when parent changes, the parent's child count is changed
- if the child is enabled, the following needs to be done:
  - call clk_enable() for the new parent
  - call the implementation's set_parent
  - call clk_disbale() for the old parent

I believe this sort of thing is very useful to have in the core
implemetation, as it is the sort of thing people will get wrong
on a regular basis, as well as being common to all but the simplest
of clock implementations.

note, this may then cause problems on how you deal with a system
which has booted and is just setting the clocks up, as we may end
up disabling a clock that is being used elsewhere (but we just
don't know it).

second note, this is also even more fun for resume, where clock
control registers may not have been kept over resume. Simply
rewriting these registers from store may end up causing problems
with glitching clocks, etc.

[PATCH 1/3] Add a common struct clk

[Jassi Brar]

On Wed, Sep 15, 2010 at 12:40 PM, Jeremy Kerr <jeremy.kerr@canonical.com> wrote:
> We currently have 21 definitions of struct clk in the ARM architecture,
> each defined on a per-platform basis. This makes it difficult to define
> platform- (or architecture-) independent clock sources without making
> assumptions about struct clk, and impossible to compile two
> platforms with different struct clks into a single image.
>
> This change is an effort to unify struct clk where possible, by defining
> a common struct clk, containing a set of clock operations. Different
> clock implementations can set their own operations, and have a standard
> interface for generic code. The callback interface is exposed to the
> kernel proper, while the clock implementations only need to be seen by
> the platform internals.
>
> This allows us to share clock code among platforms, and makes it
> possible to dynamically create clock devices in platform-independent
> code.
>
> Platforms can enable the generic struct clock through
> CONFIG_USE_COMMON_STRUCT_CLK. In this case, the clock infrastructure
> consists of a common struct clk:
>
> struct clk {
> ? ? ? ?const struct clk_ops ? ?*ops;
> ? ? ? ?unsigned int ? ? ? ? ? ?enable_count;
> ? ? ? ?struct mutex ? ? ? ? ? ?mutex;
> };
>
> And a set of clock operations (defined per type of clock):
>
> struct clk_ops {
> ? ? ? int ? ? ? ? ? ? (*enable)(struct clk *);
> ? ? ? void ? ? ? ? ? ?(*disable)(struct clk *);
> ? ? ? unsigned long ? (*get_rate)(struct clk *);
> ? ? ? [...]
> };
>
> To define a hardware-specific clock, machine code can "subclass" the
> struct clock into a new struct (adding any device-specific data), and
> provide a set of operations:
>
> struct clk_foo {
> ? ? ? ?struct clk ? ? ?clk;
> ? ? ? ?void __iomem ? ?*some_register;
> };
>
> struct clk_ops clk_foo_ops = {
> ? ? ? ?.get_rate = clk_foo_get_rate,
> };
>
> The common clock definitions are based on a development patch from Ben
> Herrenschmidt <benh@kernel.crashing.org>.
>
> Signed-off-by: Jeremy Kerr <jeremy.kerr@canonical.com>

Hi Jeremy,

  Are you planning to revise the patch-set or just taking time to
resubmit as such?

Thanks.

[PATCH 1/3] Add a common struct clk

[Jeremy Kerr]

Hi Jassi,

>   Are you planning to revise the patch-set or just taking time to
> resubmit as such?

I've reworked this patch to allow clocks that are enabled/disabled in atomic 
contexts, it's here if you'd like a preview:

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

I'm just waiting on some initial feedback, will post to l-a-k once that's in.

Regards,


Jeremy

[PATCH 1/3] Add a common struct clk

[Uwe Kleine-König]

On Mon, Nov 29, 2010 at 03:59:21PM +0800, Jeremy Kerr wrote:
> Hi Jassi,
> 
> >   Are you planning to revise the patch-set or just taking time to
> > resubmit as such?
> 
> I've reworked this patch to allow clocks that are enabled/disabled in atomic 
> contexts, it's here if you'd like a preview:
> 
> http://kernel.ubuntu.com/git?p=jk/dt/linux-2.6.git;a=commitdiff;h=33220e119d3213282d4234fec7438baa6d04b5f0
> 
> I'm just waiting on some initial feedback, will post to l-a-k once that's in.
I assume the initial feedback should be provided from someone internal
to Canonical or Linaro?  Can you give an estimate when you can post it,
I really thing that's the way to go for simplifying the clock code on
imx which is on my todo list.

While reading quickly over the patch I wondered if there isn't a better
way to get that spinlock/mutex thingy implemented.

You currently have:

	struct clk {
	       const struct clk_ops    *ops;
	       unsigned int            enable_count;
	       int                     flags;
	       union {
	               struct mutex    mutex;
	               spinlock_t      spinlock;
	       } lock;
	};

What about using this one instead?:

	struct clk_base {
		/* merge that with ops?  Probably not */
		const struct clk_lock_ops *lock_ops;
		const struct clk_ops *ops;
		unsigned int enable_count;
	};

	struct clk {
		struct clk_base base;
		struct mutex lock;
	};

	struct clk_atomic {
		struct clk_base base;
		spinlock_t lock;
	};


with the obvious definition of struct clk_lock_ops and the two instances
for clk and clk_atomic etc. pp.

This way and when I prefer to use the sleeping variant only I don't need
to bother with spinlocks at all.

Just an idea ...

Uwe

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

[PATCH 1/3] Add a common struct clk

[Jeremy Kerr]

Hi Uwe,

> I assume the initial feedback should be provided from someone internal
> to Canonical or Linaro?  Can you give an estimate when you can post it,
> I really thing that's the way to go for simplifying the clock code on
> imx which is on my todo list.

No, I was waiting on feedback from the ST-E platform folks, who will need the 
atomic clocks. However, I've been out of action for a couple of weeks, hence 
the delay.

I'll get the next revision posted this week.

> While reading quickly over the patch I wondered if there isn't a better
> way to get that spinlock/mutex thingy implemented.
> 
> You currently have:
> 
> 	struct clk {
> 	       const struct clk_ops    *ops;
> 	       unsigned int            enable_count;
> 	       int                     flags;
> 	       union {
> 	               struct mutex    mutex;
> 	               spinlock_t      spinlock;
> 	       } lock;
> 	};
> 
> What about using this one instead?:
> 
> 	struct clk_base {
> 		/* merge that with ops?  Probably not */
> 		const struct clk_lock_ops *lock_ops;
> 		const struct clk_ops *ops;
> 		unsigned int enable_count;
> 	};
> 
> 	struct clk {
> 		struct clk_base base;
> 		struct mutex lock;
> 	};
> 
> 	struct clk_atomic {
> 		struct clk_base base;
> 		spinlock_t lock;
> 	};

This means we'll need a separate API (clk_get_rate, etc) for the atomic 
clocks, or change the API to take a clk_base (and then fix up all the users of 
the API).

Regardless, I'd prefer to keep the separation to just the lock itself, rather 
than percolating down to other interfaces.


> This way and when I prefer to use the sleeping variant only I don't need
> to bother with spinlocks at all.

How do you mean? You shouldn't need to deal with spinlocks with the current 
code if you're just using non-atomic clocks.

Cheers,


Jeremy

[PATCH 1/3] Add a common struct clk

[Uwe Kleine-König]

Hello Jeremy,

On Wed, Dec 08, 2010 at 09:02:37AM +0800, Jeremy Kerr wrote:
> > I assume the initial feedback should be provided from someone internal
> > to Canonical or Linaro?  Can you give an estimate when you can post it,
> > I really thing that's the way to go for simplifying the clock code on
> > imx which is on my todo list.
> 
> No, I was waiting on feedback from the ST-E platform folks, who will need the 
> atomic clocks. However, I've been out of action for a couple of weeks, hence 
> the delay.
> 
> I'll get the next revision posted this week.
Great.
 
> > While reading quickly over the patch I wondered if there isn't a better
> > way to get that spinlock/mutex thingy implemented.
> > 
> > You currently have:
> > 
> > 	struct clk {
> > 	       const struct clk_ops    *ops;
> > 	       unsigned int            enable_count;
> > 	       int                     flags;
> > 	       union {
> > 	               struct mutex    mutex;
> > 	               spinlock_t      spinlock;
> > 	       } lock;
> > 	};
> > 
> > What about using this one instead?:
> > 
> > 	struct clk_base {
> > 		/* merge that with ops?  Probably not */
> > 		const struct clk_lock_ops *lock_ops;
> > 		const struct clk_ops *ops;
> > 		unsigned int enable_count;
> > 	};
> > 
> > 	struct clk {
> > 		struct clk_base base;
> > 		struct mutex lock;
> > 	};
> > 
> > 	struct clk_atomic {
> > 		struct clk_base base;
> > 		spinlock_t lock;
> > 	};
> 
> This means we'll need a separate API (clk_get_rate, etc) for the atomic 
> clocks, or change the API to take a clk_base (and then fix up all the users of 
> the API).
Ah, that's true.  As I said, I didnt' thought it to an end, just seemed
to be clearer to me.
 
> Regardless, I'd prefer to keep the separation to just the lock itself, rather 
> than percolating down to other interfaces.
> 
> 
> > This way and when I prefer to use the sleeping variant only I don't need
> > to bother with spinlocks at all.
> 
> How do you mean? You shouldn't need to deal with spinlocks with the current 
> code if you're just using non-atomic clocks.
Of course I can ignore them, this is more that I don't like having
members in structs or unions that are unused.  (As a mutex contains a
spinlock anyhow this is admittedly a bit strange when thinking again.
:-)

Best regards
Uwe

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

[PATCH 1/3] Add a common struct clk

[Ben Dooks]

On 08/12/10 01:02, Jeremy Kerr wrote:
> Hi Uwe,
> 
>> I assume the initial feedback should be provided from someone internal
>> to Canonical or Linaro?  Can you give an estimate when you can post it,
>> I really thing that's the way to go for simplifying the clock code on
>> imx which is on my todo list.
> 
> No, I was waiting on feedback from the ST-E platform folks, who will need the 
> atomic clocks. However, I've been out of action for a couple of weeks, hence 
> the delay.

Why do they need atomic clocks?

This is starting to look like a real mess.

[PATCH 1/3] Add a common struct clk

[Jeremy Kerr]

Hi Ben,

> Why do they need atomic clocks?

Some of the clocks need enabling/disabling at interrupt time, and apparently 
can't be deferred to a non-atomic context. You'd have to query them for more 
details though, this is all I know at this stage.

> This is starting to look like a real mess.

The only external change this brings is that the clock driver uses 
INIT_CLK_ATOMIC rather than INIT_CLK.

Cheers,


Jeremy

[PATCH 1/3] Add a common struct clk

[Richard Zhao]

On Thu, Dec 9, 2010 at 10:16 AM, Jeremy Kerr <jeremy.kerr@canonical.com> wrote:
> Hi Ben,
>
>> Why do they need atomic clocks?
>
> Some of the clocks need enabling/disabling at interrupt time, and apparently
> can't be deferred to a non-atomic context. You'd have to query them for more
> details though, this is all I know at this stage.
atomic clock make it easy to use, or we have to move many irq tasks to
kthread context. we may involves many kthreads. If we put tasks to
global work queue,  sometimes it conflicts each other, and causes dead
lock.
Cases to use atomic clock: when insert a sd card, we have to enable sd
host controller clock. when a dma call back tell you everything is
done, you may want to disable controller clock. ...
And what can cause clock not atomic? At least for freescale imx, it
won't sleep if you just operate clock. I mean no voltage change.

Thanks
Richard
>
>> This is starting to look like a real mess.
>
> The only external change this brings is that the clock driver uses
> INIT_CLK_ATOMIC rather than INIT_CLK.
>
> Cheers,
>
>
> Jeremy
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

[PATCH 1/3] Add a common struct clk

[Jassi Brar]

On Sat, Dec 11, 2010 at 12:09 AM, Richard Zhao <linuxzsc@gmail.com> wrote:

>>> Why do they need atomic clocks?
just the question in my mind...

> atomic clock make it easy to use, or we have to move many irq tasks to
> kthread context. we may involves many kthreads. If we put tasks to
> global work queue, ?sometimes it conflicts each other, and causes dead
> lock.
Sounds like more of a requirement of convenience, than technical limitation.
See below..

> Cases to use atomic clock: when insert a sd card, we have to enable sd
> host controller clock.
One does have to enable the HC clock ... but why only in the irq context
of the OOB signal ?

> when a dma call back tell you everything is
> done, you may want to disable controller clock. ...
Again, why can't you disable the clock later?
I doubt if power saving should be the reason here.

IMHO, we should have atomic clocks only if there is really
no other alternative.
Making it available in standard API will only make its (ab)use
more common.
Also, it might get difficult to upgrade the API in future if there
are two different types of clocks to manage.

Thanks.

[PATCH 1/3] Add a common struct clk

[Jeremy Kerr]

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

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

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

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

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

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

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

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

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

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

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

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

---
 arch/Kconfig        |    3 +
 include/linux/clk.h |   90 +++++++++++++++++++++++++++++++++++----
 kernel/Makefile     |    1 
 kernel/clk.c        |  101 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 186 insertions(+), 9 deletions(-)

diff --git a/arch/Kconfig b/arch/Kconfig
index acda512..2458b5e 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -151,4 +151,7 @@ config HAVE_MIXED_BREAKPOINTS_REGS
 config HAVE_USER_RETURN_NOTIFIER
 	bool
 
+config USE_COMMON_STRUCT_CLK
+	bool
+
 source "kernel/gcov/Kconfig"
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 1d37f42..a95cc82 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -3,6 +3,7 @@
  *
  *  Copyright (C) 2004 ARM Limited.
  *  Written by Deep Blue Solutions Limited.
+ *  Copyright (c) 2010 Jeremy Kerr <jeremy.kerr@canonical.com>
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
@@ -11,18 +12,95 @@
 #ifndef __LINUX_CLK_H
 #define __LINUX_CLK_H
 
+#include <linux/err.h>
+#include <linux/mutex.h>
+
 struct device;
 
-/*
- * The base API.
+#ifdef CONFIG_USE_COMMON_STRUCT_CLK
+
+/* If we're using the common struct clk, we define the base clk object here */
+
+/**
+ * struct clk - hardware independent clock structure
+ * @clk_ops:		implementation-specific ops for this clock
+ * @enable_count:	count of clk_enable() calls active on this clock
+ * @mutex:		lock for enable/disable or other HW-specific ops
+ *
+ * The base clock object, used by drivers for hardware-independent manipulation
+ * of clock lines. This will be 'subclassed' by device-specific implementations,
+ * which add device-specific data to struct clk. For example:
+ *
+ *  struct clk_foo {
+ *      struct clk;
+ *      [device specific fields]
+ *  };
+ *
+ * The clock driver code will manage the device-specific data, and pass
+ * clk_foo.clk to the common clock code. The clock driver will be called
+ * through the @ops callbacks.
+ *
+ * The @enable_count and @mutex members are initialised when a clock is
+ * registered with the arch-specific clock management code; the clock driver
+ * code does not need to handle these.
  */
+struct clk {
+	const struct clk_ops	*ops;
+	unsigned int		enable_count;
+	struct mutex		mutex;
+};
+
+#define INIT_CLK(o) { .ops = &o, }
+
+struct clk_ops {
+       int		(*enable)(struct clk *);
+       void		(*disable)(struct clk *);
+       int		(*get)(struct clk *);
+       void		(*put)(struct clk *);
+       unsigned long	(*get_rate)(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 *);
+};
 
+/**
+ * __clk_get - update clock-specific refcounter
+ *
+ * @clk: The clock to refcount
+ *
+ * Before a clock is returned from clk_get, this function should be called
+ * to update any clock-specific refcounting.
+ *
+ * Returns non-zero on success, zero on failure.
+ *
+ * Drivers should not need this function; it is only needed by the
+ * arch-specific clk_get() implementations.
+ */
+int __clk_get(struct clk *clk);
+
+/**
+ * clk_common_init - initialise a clock for driver usage
+ *
+ * Used by arch code on registration with the clock infrastructure.
+ */
+static inline void clk_common_init(struct clk *clk)
+{
+	mutex_init(&clk->mutex);
+	clk->enable_count = 0;
+}
+
+#else /* !CONFIG_USE_COMMON_STRUCT_CLK */
 
 /*
- * struct clk - an machine class defined object / cookie.
+ * Global clock object, actual structure is declared per-machine
  */
 struct clk;
 
+static inline void clk_common_init(struct clk *clk) { }
+
+#endif /* !CONFIG_USE_COMMON_STRUCT_CLK */
+
 /**
  * clk_get - lookup and obtain a reference to a clock producer.
  * @dev: device for clock "consumer"
@@ -83,12 +161,6 @@ unsigned long clk_get_rate(struct clk *clk);
  */
 void clk_put(struct clk *clk);
 
-
-/*
- * The remaining APIs are optional for machine class support.
- */
-
-
 /**
  * clk_round_rate - adjust a rate to the exact rate a clock can provide
  * @clk: clock source
diff --git a/kernel/Makefile b/kernel/Makefile
index 057472f..1ae15aa 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -105,6 +105,7 @@ obj-$(CONFIG_PERF_EVENTS) += perf_event.o
 obj-$(CONFIG_HAVE_HW_BREAKPOINT) += hw_breakpoint.o
 obj-$(CONFIG_USER_RETURN_NOTIFIER) += user-return-notifier.o
 obj-$(CONFIG_PADATA) += padata.o
+obj-$(CONFIG_USE_COMMON_STRUCT_CLK) += clk.o
 
 ifneq ($(CONFIG_SCHED_OMIT_FRAME_POINTER),y)
 # According to Alan Modra <alan@linuxcare.com.au>, the -fno-omit-frame-pointer is
diff --git a/kernel/clk.c b/kernel/clk.c
new file mode 100644
index 0000000..cdea25f
--- /dev/null
+++ b/kernel/clk.c
@@ -0,0 +1,101 @@
+/*
+ * Copyright (C) 2010 Canonical Ltd <jeremy.kerr@canonical.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * Standard functionality for the common clock API.
+ */
+
+#include <linux/clk.h>
+#include <linux/mutex.h>
+#include <linux/module.h>
+
+int clk_enable(struct clk *clk)
+{
+	int ret = 0;
+
+	if (!clk->ops->enable)
+		return 0;
+
+	mutex_lock(&clk->mutex);
+	if (!clk->enable_count)
+		ret = clk->ops->enable(clk);
+
+	if (!ret)
+		clk->enable_count++;
+	mutex_unlock(&clk->mutex);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(clk_enable);
+
+void clk_disable(struct clk *clk)
+{
+	if (!clk->ops->disable)
+		return;
+
+	mutex_lock(&clk->mutex);
+
+	if (!--clk->enable_count)
+		clk->ops->disable(clk);
+
+	mutex_unlock(&clk->mutex);
+}
+EXPORT_SYMBOL_GPL(clk_disable);
+
+unsigned long clk_get_rate(struct clk *clk)
+{
+	if (clk->ops->get_rate)
+		return clk->ops->get_rate(clk);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(clk_get_rate);
+
+int __clk_get(struct clk *clk)
+{
+	if (clk->ops->get)
+		return clk->ops->get(clk);
+	return 1;
+}
+EXPORT_SYMBOL_GPL(__clk_get);
+
+void clk_put(struct clk *clk)
+{
+	if (clk->ops->put)
+		clk->ops->put(clk);
+}
+EXPORT_SYMBOL_GPL(clk_put);
+
+long clk_round_rate(struct clk *clk, unsigned long rate)
+{
+	if (clk->ops->round_rate)
+		return clk->ops->round_rate(clk, rate);
+	return -ENOSYS;
+}
+EXPORT_SYMBOL_GPL(clk_round_rate);
+
+int clk_set_rate(struct clk *clk, unsigned long rate)
+{
+	if (clk->ops->set_rate)
+		return clk->ops->set_rate(clk, rate);
+	return -ENOSYS;
+}
+EXPORT_SYMBOL_GPL(clk_set_rate);
+
+int clk_set_parent(struct clk *clk, struct clk *parent)
+{
+	if (clk->ops->set_parent)
+		return clk->ops->set_parent(clk, parent);
+	return -ENOSYS;
+}
+EXPORT_SYMBOL_GPL(clk_set_parent);
+
+struct clk *clk_get_parent(struct clk *clk)
+{
+	if (clk->ops->get_parent)
+		return clk->ops->get_parent(clk);
+	return ERR_PTR(-ENOSYS);
+}
+EXPORT_SYMBOL_GPL(clk_get_parent);