[PATCH v4 0/6] common clk framework

[PATCH v4 0/6] common clk framework

[Mike Turquette]

From: Mike Turquette <mturquette@ti.com>

The common clk framework is an attempt to define a generic struct clk
which most platforms can use to build a clk tree and perform a set of
well-defined operations.

The previous patchset, v3, can be found at,
http://article.gmane.org/gmane.linux.kernel/1218622

New stuff in v4:
 * clk rate change notifiers
 * clk debug info via debugfs (instead of sysfs)
 * lots of bug fixes

Stuff that is known to be missing in v4:
 * basic mux and divider clk types
 * fix for migrating clk_prepare_count/clk_enable_count in
   clk_set_parent
 * minor rework comments from v3
 * Documentation/clk.txt needs love

All of the mising items above will be rolled into v5 ASAP.  I wanted to
go ahead and push out the new notifier changes for review and gather
comments on those since those were a big gap in the v3 patchset.

Paul W. also had some good comments about the greater clk API, and the
opportunity to fix some of that stuff while this patchset is still under
discussion.  I didn't address those here because they require more
thought, and more comments from reviewers.

Finally, OMAP4 support for the common struct clk will be posted
immediately after this patch series to LAKML and LOML, along with some
hack patches that show how to use the recursive clk_set_rate for
propagating rate changes up the tree for CPUfreq and how to use the new
clk rate change notifiers in a driver.

Mike Turquette (6):
  clk: Kconfig: add entry for HAVE_CLK_PREPARE
  Documentation: common clk API
  clk: introduce the common clock framework
  clk: introduce rate change notifiers
  clk: basic gateable and fixed-rate clks
  clk: export the clk tree topology to debugfs

 Documentation/clk.txt   |  312 +++++++++++++++
 drivers/clk/Kconfig     |   23 ++
 drivers/clk/Makefile    |    4 +-
 drivers/clk/clk-basic.c |  208 ++++++++++
 drivers/clk/clk.c       |  992 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/clk.h     |  230 +++++++++++-
 6 files changed, 1765 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/clk.txt
 create mode 100644 drivers/clk/clk-basic.c
 create mode 100644 drivers/clk/clk.c

-- 
1.7.5.4

[PATCH v4 1/6] clk: Kconfig: add entry for HAVE_CLK_PREPARE

[Mike Turquette]

The common clk framework provides clk_prepare and clk_unprepare
implementations.  Create an entry for HAVE_CLK_PREPARE so that
GENERIC_CLK can select it.

Signed-off-by: Mike Turquette <mturquette@linaro.org>
Acked-by: Shawn Guo <shawn.guo@linaro.org>
---
 drivers/clk/Kconfig |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 3530927..7a9899bd 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -5,3 +5,6 @@ config CLKDEV_LOOKUP
 
 config HAVE_MACH_CLKDEV
 	bool
+
+config HAVE_CLK_PREPARE
+	bool
-- 
1.7.5.4

[PATCH v4 2/6] Documentation: common clk API

[Mike Turquette]

Provide documentation for the common clk structures and APIs.  This code
can be found in drivers/clk/ and include/linux/clk.h.

Signed-off-by: Mike Turquette <mturquette@linaro.org>
Cc: Jeremy Kerr <jeremy.kerr@canonical.com>
---
 Documentation/clk.txt |  312 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 312 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/clk.txt

diff --git a/Documentation/clk.txt b/Documentation/clk.txt
new file mode 100644
index 0000000..ff75539c
--- /dev/null
+++ b/Documentation/clk.txt
@@ -0,0 +1,312 @@
+		The Common Clk Framework
+		Mike Turquette <mturquette@ti.com>
+
+	Part 1 - common data structures and API
+
+The common clk framework is a combination of a common definition of
+struct clk which can be used across most platforms as well as a set of
+driver-facing APIs which operate on those clks.  Platforms can enable it
+by selecting CONFIG_GENERIC_CLK.
+
+Below is the common struct clk definition from include/linux/clk.h.  It
+is modified slightly for brevity:
+
+struct clk {
+	const char		*name;
+	const struct clk_ops	*ops;
+	struct clk		*parent;
+	unsigned long		rate;
+	unsigned long		flags;
+	unsigned int		enable_count;
+	unsigned int		prepare_count;
+	struct hlist_head	children;
+	struct hlist_node	child_node;
+};
+
+The .name, .parent and .children members make up the core of the clk
+tree topology.  The clk API itself defines several driver-facing
+functions which operate on struct clk.  Below is an abbreviated
+description of some of those functions taken from the kerneldoc in
+include/linux/clk.h:
+
+clk_prepare - This prepares the clock source for use.  Must not be
+called from within atomic context.  Must be called before clk_enable.
+
+clk_unprepare - This undoes a previously prepared clock.  The caller
+must balance the number of prepare and unprepare calls.  Must not be
+called from within atomic context.  Must be called after clk_disable.
+
+clk_enable - Inform the system when the clock source should be running.
+If the clock can not be enabled/disabled, this should return success.
+May be called from atomic contexts.  Returns success (0) or negative
+errno.  Must be called after clk_prepare.
+
+clk_disable - Inform the system that a clock source is no longer
+required by a driver and may be shut down.  May be called from atomic
+contexts.  Must be called before clk_unprepare.
+
+clk_get_rate - Obtain the current clock rate (in Hz) for a clock source.
+This is only valid once the clock source has been enabled.  Returns zero
+if the clock rate is unknown.  Does not touch the hardware, just the
+cached rate value.
+
+clk_round_rate - Adjust a rate to the exact rate a clock can provide.
+Returns rounded clock rate in Hz, or negative errno.
+
+clk_set_rate - Set the clock rate for a clock source.  Returns success
+(0) or negative errno.
+
+clk_get_parent - Get the parent clock source for this clock.  Returns
+struct clk corresponding to parent clock source, or valid IS_ERR()
+condition containing errno.  Does not touch the hardware, just the
+cached parent value.
+
+clk_set_parent - Set the parent clock source for this clock.  Returns
+success (0) or negative errno.
+
+For clk implementations which use the common definition of struct clk,
+the above functions use the struct clk_ops pointer in struct clk to
+perform the hardware-specific parts of those operations.  The definition
+of struct clk_ops:
+
+	struct clk_ops {
+		int		(*prepare)(struct clk *clk);
+		void		(*unprepare)(struct clk *clk);
+		int		(*enable)(struct clk *clk);
+		void		(*disable)(struct clk *clk);
+		unsigned long	(*recalc_rate)(struct clk *clk);
+		long		(*round_rate)(struct clk *clk, unsigned long,
+				unsigned long *);
+		int		(*set_rate)(struct clk *clk, unsigned long);
+		int		(*set_parent)(struct clk *clk, struct clk *);
+		struct clk *	(*get_parent)(struct clk *clk);
+	};
+
+	Part 2 - hardware clk implementations
+
+The strength of the common struct clk comes from its .ops pointer and
+the ability for platform and driver code to wrap the struct clk instance
+with hardware-specific data which the operations in the .ops pointer
+have knowledge of.  To illustrate consider the simple gateable clk
+implementation in drivers/clk/clk-basic.c:
+
+struct clk_gate {
+	struct clk      clk;
+	struct clk      *fixed_parent;
+	void __iomem    *reg;
+	u8              bit_idx;
+};
+
+struct clk_gate contains the clk as well as hardware-specific knowledge
+about which register and bit controls this clk's gating.  The
+fixed-parent member is also there as a way to initialize the topology.
+
+Let's walk through enabling this clk from driver code:
+
+	struct clk *clk;
+	clk = clk_get(NULL, "my_gateable_clk");
+
+	clk_prepare(clk);
+	clk_enable(clk);
+
+Note that clk_prepare MUST be called before clk_enable even if
+clk_prepare does nothing (which in this case is true).
+
+The call graph for clk_enable is very simple:
+
+clk_enable(clk);
+	clk->enable(clk);
+			clk_gate_enable_set(clk);
+				clk_gate_set_bit(clk);
+
+And the definition of clk_gate_set_bit:
+
+static void clk_gate_set_bit(struct clk *clk)
+{
+	struct clk_gate *gate = to_clk_gate(clk);
+	u32 reg;
+
+	reg = __raw_readl(gate->reg);
+	reg |= BIT(gate->bit_idx);
+	__raw_writel(reg, gate->reg);
+}
+
+Note that in the final call to clk_gate_set_bit there is use of
+to_clk_gate, which is defined as:
+
+#define to_clk_gate(ck) container_of(ck, struct clk_gate, clk)
+
+This simple abstraction is what allows the common clk framework to scale
+across many platforms.  The struct clk definition remains the same while
+the hardware operations in the .ops pointer know the details of the clk
+hardware.  A little pointer arithmetic to get to the data is all that
+the ops need.
+
+	Part 3 - Supporting your own clk hardware
+
+To construct a clk hardware structure for your platform you simply need
+to define the following:
+
+struct clk_your_clk {
+	struct clk;
+	unsigned long some_data;
+	struct your_struct *some_more_data;
+};
+
+To take advantage of your data you'll need to support valid operations
+for your clk:
+
+struct clk_ops clk_ops_your_clk {
+	.enable		= &clk_your_clk_enable;
+	.disable	= &clk_your_clk_disable;
+};
+
+Implement the above functions using container_of:
+
+int clk_your_clk_enable(struct clk *clk)
+{
+	struct clk_your_clk *yclk;
+
+	yclk = container_of(clk, struct clk_your_clk, clk);
+
+	magic(yclk);
+};
+
+If you are statically allocating all of your clk_your_clk instances then
+you will still need to initialize some stuff in struct clk with the
+clk_init function from include/linux/clk.h:
+
+clk_init(&yclk->clk);
+
+If you are dynamically creating clks or using device tree then you might
+want a hardware-specific register function:
+
+int clk_your_clk_register(const char *name, unsigned long flags,
+		unsigned long some_data,
+		struct your_struct *some_more_data)
+{
+	struct clk_your_clk *yclk;
+
+	yclk = kmalloc(sizeof(struct clk_your_clk), GFP_KERNEL);
+
+	yclk->some_data = some_data;
+	yclk->some_more_data = some_more_data;
+
+	yclk->clk.name = name;
+	yclk->clk.flags = flags;
+
+	clk_init(&yclk->clk);
+
+	return 0;
+}
+
+	Part 4 - clk_set_rate & rate change notifications
+
+FIXME - need to flesh this out with notifer info
+
+clk_set_rate deserves a special mention because it is more complex than
+the other operations.  There are three key concepts to the common
+clk_set_rate implementation:
+
+1) recursively traversing up the clk tree and changing clk rates, one
+parent at a time, if each clk allows it
+2) failing to change rate if the clk is enabled and must only change
+rates while disabled
+3) using clk rate change notifiers to allow devices to handle dynamic
+rate changes for clks which do support changing rates while enabled
+
+For the simple, non-recursive case the call graph looks like:
+
+clk_set_rate(clk, rate);
+	__clk_set_rate(clk, rate);
+		clk->round_rate(clk, rate *parent_rate);
+		clk->set_rate(clk, rate);
+
+You might be wondering what that third paramater in .round_rate is.  If
+a clk supports the CLK_PARENT_SET_RATE flag then that enables it's
+hardware-specific .round_rate function to provide a new rate that the
+parent should transition to.  For example, imagine a rate-adjustable clk
+A that is the parent of clk B, which has a fixed divider of 2.
+
+	clk A	(rate = 10MHz) (possible rates = 5MHz, 10MHz, 20MHz)
+	  |
+	  |
+	  |
+	clk B	(rate = 5MHz) (fixed divider of 2)
+
+In the above scenario clk B will always have half the rate of clk A.  If
+clk B is to generate a 10MHz clk then clk A must generate 20MHz in turn.
+The driver writer could hack in knowledge of clk A, but in reality clk B
+drives the devices operation and the driver shouldn't know the details
+of the clk tree topology.  In this case it would be nice for clk B to
+propagate it's request up to clk A.
+
+Here the call graph for our above example:
+
+clk_set_rate(clk, rate);
+	__clk_set_rate(clk, rate);
+		clk->round_rate(clk, rate *parent_rate);
+		clk->set_rate(clk, rate);
+		__clk_set_rate(clk->parent, parent_rate);
+			clk->round_rate(clk, rate *parent_rate);
+			clk->set_rate(clk, rate);
+
+Note that the burden of figuring out whether to recurse upwards falls on
+the hardware-specific .round_rate function.  The common clk framework
+does not have the context to make such complicated decisions in a
+generic way for all platforms.
+
+Another caveat is that child clks might run at weird intermediate
+frequencies during a complex upwards propagation, as illustrated below:
+
+	clk A  (pll 100MHz - 300MHz)	(currently locked at 200MHz)
+	  |
+	  |
+	  |
+	clk B  (divide by 1 or 2)	(currently divide by 2, 100MHz)
+	  |
+	  |
+	  |
+	clk C  (divide by 1 or 2)	(currently divide by 1, 100MHz)
+
+The call graph below, with some bracketed annotations, describes how
+this might work with some clever .round_rate callbacks when trying to
+set clk C to run at 26MHz:
+
+clk_set_rate(C, 26MHz);
+	__clk_set_rate(C, 26MHz);
+		clk->round_rate(C, 26MHz, *parent_rate);
+		[ round_rate returns 50MHz ]
+		[ &parent_rate is 52MHz ]
+		clk->set_rate(C, 50Mhz);
+		[ clk C is set to 50MHz, which sets divider to 2 ]
+		__clk_set_rate(clk->parent, parent_rate);
+			clk->round_rate(B, 52MHz, *parent_rate);
+			[ round_rate returns 100MHz ]
+			[ &parent_rate is 104MHz ]
+			clk->set_rate(B, 100MHz);
+			[ clk B stays at 100MHz, divider stays at 2 ]
+			__clk_set_rate(clk->parent, parent_rate);
+				[ round_rate returns 104MHz ]
+				[ &parent_rate is ignored ]
+				clk->set_rate(A, 104MHz);
+				[ clk A is set to 104MHz]
+
+The end result is that clk C runs at 26MHz.  Each .set_rate callback
+actually sets the intermediate rate, which nicely reflects reality.
+Once clk rate change notifiers are supported then it is expected that
+PRECHANGE notifiers will "stack" in situations with recursive
+clk_set_rate calls.
+
+Thus a driver X which subcribes to rate-change notifers for clk C would
+have received 2 PRECHANGE notifiers in the above example.  The first
+would have notified the driver that clk C was changing from 100MHz to
+50MHz.  The second PRECHANGE notifier would have told driver X that clk
+C had changed from 50MHz to 26MHz.  There would not be a PRECHANGE
+notifier corresponding to __clk_set_rate(B, 50MHz) since B is already
+running at that rate and the notification would be unnecessary.
+
+clk_set_rate is written in such a way that POSTCHANGE notifiers and
+ABORTCHANGE notifiers will only be sent once.  Each will start
+propagation from the highest point in the tree which was affected by the
+operation.
-- 
1.7.5.4

[PATCH v4 3/6] clk: introduce the common clock framework

[Mike Turquette]

The common clk framework is an attempt to define a common struct clk
which can be used by most platforms, and an API that drivers can always
use safely for managing clks.

The net result is consolidation of many different struct clk definitions
and platform-specific clk framework implementations.

This patch introduces the common struct clk, struct clk_ops and
definitions for the long-standing clk API in include/clk/clk.h.
Platforms may define their own hardware-specific clk structure, so long
as it wraps an instance of the common struct clk which is passed to
clk_init at initialization time.  From this point on all of the usual
clk APIs should be supported, so long as the platform supports them
through hardware-specific callbacks in struct clk_ops.

See Documentation/clk.txt for more details.

This patch is based on the work of Jeremy Kerr, which in turn was based
on the work of Ben Herrenschmidt.  Big thanks to both of them for
kickstarting this effort.

Signed-off-by: Mike Turquette <mturquette@linaro.org>
Cc: Jeremy Kerr <jeremy.kerr@canonical.com>
---
 drivers/clk/Kconfig  |    4 +
 drivers/clk/Makefile |    1 +
 drivers/clk/clk.c    |  564 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/clk.h  |  130 +++++++++++-
 4 files changed, 695 insertions(+), 4 deletions(-)
 create mode 100644 drivers/clk/clk.c

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 7a9899bd..adc0586 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -8,3 +8,7 @@ config HAVE_MACH_CLKDEV
 
 config HAVE_CLK_PREPARE
 	bool
+
+config GENERIC_CLK
+	bool
+	select HAVE_CLK_PREPARE
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 07613fa..570d5b9 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -1,2 +1,3 @@
 
 obj-$(CONFIG_CLKDEV_LOOKUP)	+= clkdev.o
+obj-$(CONFIG_GENERIC_CLK)	+= clk.o
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
new file mode 100644
index 0000000..8cadadd
--- /dev/null
+++ b/drivers/clk/clk.c
@@ -0,0 +1,564 @@
+/*
+ * Copyright (C) 2010-2011 Canonical Ltd <jeremy.kerr@canonical.com>
+ * Copyright (C) 2011 Linaro Ltd <mturquette@linaro.org>
+ *
+ * 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.  See Documentation/clk.txt
+ */
+
+#include <linux/clk.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/spinlock.h>
+#include <linux/err.h>
+#include <linux/list.h>
+
+static DEFINE_SPINLOCK(enable_lock);
+static DEFINE_MUTEX(prepare_lock);
+
+static HLIST_HEAD(clk_root_list);
+
+/***        clk API        ***/
+
+void __clk_unprepare(struct clk *clk)
+{
+	if (!clk)
+		return;
+
+	if (WARN_ON(clk->prepare_count == 0))
+		return;
+
+	if (--clk->prepare_count > 0)
+		return;
+
+	WARN_ON(clk->enable_count > 0);
+
+	if (clk->ops->unprepare)
+		clk->ops->unprepare(clk);
+
+	__clk_unprepare(clk->parent);
+}
+
+/**
+ * clk_unprepare - perform the slow part of a clk gate
+ * @clk: the clk being gated
+ *
+ * clk_unprepare may sleep, which differentiates it from clk_disable.  In a
+ * simple case, clk_unprepare can be used instead of clk_disable to gate a clk
+ * if the operation may sleep.  One example is a clk which is accessed over
+ * I2c.  In the complex case a clk gate operation may require a fast and a slow
+ * part.  It is this reason that clk_unprepare and clk_disable are not mutually
+ * exclusive.  In fact clk_disable must be called before clk_unprepare.
+ */
+void clk_unprepare(struct clk *clk)
+{
+	mutex_lock(&prepare_lock);
+	__clk_unprepare(clk);
+	mutex_unlock(&prepare_lock);
+}
+EXPORT_SYMBOL_GPL(clk_unprepare);
+
+int __clk_prepare(struct clk *clk)
+{
+	int ret = 0;
+
+	if (!clk)
+		return 0;
+
+	if (clk->prepare_count == 0) {
+		ret = __clk_prepare(clk->parent);
+		if (ret)
+			return ret;
+
+		if (clk->ops->prepare) {
+			ret = clk->ops->prepare(clk);
+			if (ret) {
+				__clk_unprepare(clk->parent);
+				return ret;
+			}
+		}
+	}
+
+	clk->prepare_count++;
+
+	return 0;
+}
+
+/**
+ * clk_prepare - perform the slow part of a clk ungate
+ * @clk: the clk being ungated
+ *
+ * clk_prepare may sleep, which differentiates it from clk_enable.  In a simple
+ * case, clk_prepare can be used instead of clk_enable to ungate a clk if the
+ * operation may sleep.  One example is a clk which is accessed over I2c.  In
+ * the complex case a clk ungate operation may require a fast and a slow part.
+ * It is this reason that clk_prepare and clk_enable are not mutually
+ * exclusive.  In fact clk_prepare must be called before clk_enable.
+ * Returns 0 on success, -EERROR otherwise.
+ */
+int clk_prepare(struct clk *clk)
+{
+	int ret;
+
+	mutex_lock(&prepare_lock);
+	ret = __clk_prepare(clk);
+	mutex_unlock(&prepare_lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(clk_prepare);
+
+void __clk_disable(struct clk *clk)
+{
+	if (!clk)
+		return;
+
+	if (WARN_ON(clk->enable_count == 0))
+		return;
+
+	if (--clk->enable_count > 0)
+		return;
+
+	if (clk->ops->disable)
+		clk->ops->disable(clk);
+
+	if (clk->parent)
+		__clk_disable(clk->parent);
+}
+
+/**
+ * clk_disable - perform the fast part of a clk gate
+ * @clk: the clk being gated
+ *
+ * clk_disable must not sleep, which differentiates it from clk_unprepare.  In
+ * a simple case, clk_disable can be used instead of clk_unprepare to gate a
+ * clk if the operation is fast and will never sleep.  One example is a
+ * SoC-internal clk which is controlled via simple register writes.  In the
+ * complex case a clk gate operation may require a fast and a slow part.  It is
+ * this reason that clk_unprepare and clk_disable are not mutually exclusive.
+ * In fact clk_disable must be called before clk_unprepare.
+ */
+void clk_disable(struct clk *clk)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&enable_lock, flags);
+	__clk_disable(clk);
+	spin_unlock_irqrestore(&enable_lock, flags);
+}
+EXPORT_SYMBOL_GPL(clk_disable);
+
+int __clk_enable(struct clk *clk)
+{
+	int ret = 0;
+
+	if (!clk)
+		return 0;
+
+	if (WARN_ON(clk->prepare_count == 0))
+		return -ESHUTDOWN;
+
+	if (clk->enable_count == 0) {
+		if (clk->parent)
+			ret = __clk_enable(clk->parent);
+
+		if (ret)
+			return ret;
+
+		if (clk->ops->enable) {
+			ret = clk->ops->enable(clk);
+			if (ret) {
+				__clk_disable(clk->parent);
+				return ret;
+			}
+		}
+	}
+
+	clk->enable_count++;
+	return 0;
+}
+
+/**
+ * clk_enable - perform the fast part of a clk ungate
+ * @clk: the clk being ungated
+ *
+ * clk_enable must not sleep, which differentiates it from clk_prepare.  In a
+ * simple case, clk_enable can be used instead of clk_prepare to ungate a clk
+ * if the operation will never sleep.  One example is a SoC-internal clk which
+ * is controlled via simple register writes.  In the complex case a clk ungate
+ * operation may require a fast and a slow part.  It is this reason that
+ * clk_enable and clk_prepare are not mutually exclusive.  In fact clk_prepare
+ * must be called before clk_enable.  Returns 0 on success, -EERROR
+ * otherwise.
+ */
+int clk_enable(struct clk *clk)
+{
+	unsigned long flags;
+	int ret;
+
+	spin_lock_irqsave(&enable_lock, flags);
+	ret = __clk_enable(clk);
+	spin_unlock_irqrestore(&enable_lock, flags);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(clk_enable);
+
+/**
+ * clk_get_rate - return the rate of clk
+ * @clk: the clk whose rate is being returned
+ *
+ * Simply returns the cached rate of the clk.  Does not query the hardware.  If
+ * clk is NULL then returns 0.
+ */
+unsigned long clk_get_rate(struct clk *clk)
+{
+	unsigned long rate;
+
+	if (!clk)
+		return 0;
+
+	mutex_lock(&prepare_lock);
+	rate = clk->rate;
+	mutex_unlock(&prepare_lock);
+
+	return rate;
+}
+EXPORT_SYMBOL_GPL(clk_get_rate);
+
+/**
+ * clk_round_rate - round the given rate for a clk
+ * @clk: the clk for which we are rounding a rate
+ * @rate: the rate which is to be rounded
+ *
+ * Takes in a rate as input and rounds it to a rate that the clk can actually
+ * use which is then returned.  If clk doesn't support round_rate operation
+ * then the rate passed in is returned.
+ */
+long clk_round_rate(struct clk *clk, unsigned long rate)
+{
+	if (clk && clk->ops->round_rate)
+		return clk->ops->round_rate(clk, rate, NULL);
+
+	return rate;
+}
+EXPORT_SYMBOL_GPL(clk_round_rate);
+
+/**
+ * __clk_recalc_rates
+ * @clk: first clk in the subtree
+ *
+ * Walks the subtree of clks starting with clk and recalculates rates as it
+ * goes.  Note that if a clk does not implement the recalc_rate operation then
+ * propagation of that subtree stops and all of that clks children will not
+ * have their rates updated.  Caller must hold prepare_lock.
+ */
+static void __clk_recalc_rates(struct clk *clk)
+{
+	struct hlist_node *tmp;
+	struct clk *child;
+
+	if (!clk->ops->recalc_rate) {
+		pr_debug("%s: no .recalc_rate for clk %s\n",
+				__func__, clk->name);
+		return;
+	}
+
+	clk->rate = clk->ops->recalc_rate(clk);
+
+	/* FIXME add post-rate change notification here */
+
+	hlist_for_each_entry(child, tmp, &clk->children, child_node)
+		__clk_recalc_rates(child);
+}
+
+/**
+ * DOC: Using the CLK_PARENT_SET_RATE flag
+ *
+ * __clk_set_rate changes the child's rate before the parent's to more
+ * easily handle failure conditions.
+ *
+ * This means clk might run out of spec for a short time if its rate is
+ * increased before the parent's rate is updated.
+ *
+ * To prevent this consider setting the CLK_GATE_SET_RATE flag on any
+ * clk where you also set the CLK_PARENT_SET_RATE flag
+ */
+struct clk *__clk_set_rate(struct clk *clk, unsigned long rate)
+{
+	struct clk *fail_clk = NULL;
+	int ret = 0;
+	unsigned long old_rate = clk->rate;
+	unsigned long new_rate;
+	unsigned long parent_old_rate;
+	unsigned long parent_new_rate = 0;
+	struct clk *child;
+	struct hlist_node *tmp;
+
+	/* bail early if we can't change rate while clk is enabled */
+	if ((clk->flags & CLK_GATE_SET_RATE) && clk->enable_count)
+		return clk;
+
+	/* find the new rate and see if parent rate should change too */
+	WARN_ON(!clk->ops->round_rate);
+
+	new_rate = clk->ops->round_rate(clk, rate, &parent_new_rate);
+
+	/* FIXME propagate pre-rate change notification here */
+	/* XXX note that pre-rate change notifications will stack */
+
+	/* change the rate of this clk */
+	if (clk->ops->set_rate)
+		ret = clk->ops->set_rate(clk, new_rate);
+
+	if (ret)
+		return clk;
+
+	/*
+	 * change the rate of the parent clk if necessary
+	 *
+	 * hitting the nested 'if' path implies we have hit a .set_rate
+	 * failure somewhere upstream while propagating __clk_set_rate
+	 * up the clk tree.  roll back the clk rates one by one and
+	 * return the pointer to the clk that failed.  clk_set_rate will
+	 * use the pointer to propagate a rate-change abort notifier
+	 * from the "highest" point.
+	 */
+	if ((clk->flags & CLK_PARENT_SET_RATE) && parent_new_rate) {
+		parent_old_rate = clk->parent->rate;
+		fail_clk = __clk_set_rate(clk->parent, parent_new_rate);
+
+		/* roll back changes if parent rate change failed */
+		if (fail_clk) {
+			pr_warn("%s: failed to set parent %s rate to %lu\n",
+					__func__, fail_clk->name,
+					parent_new_rate);
+			clk->ops->set_rate(clk, old_rate);
+		}
+		return fail_clk;
+	}
+
+	/*
+	 * set clk's rate & recalculate the rates of clk's children
+	 *
+	 * hitting this path implies we have successfully finished
+	 * propagating recursive calls to __clk_set_rate up the clk tree
+	 * (if necessary) and it is safe to propagate __clk_recalc_rates
+	 * and post-rate change notifiers down the clk tree from this
+	 * point.
+	 */
+	/* FIXME propagate post-rate change notifier starting with this clk */
+	__clk_recalc_rates(clk);
+
+	return NULL;
+}
+
+/**
+ * clk_set_rate - specify a new rate for clk
+ * @clk: the clk whose rate is being changed
+ * @rate: the new rate for clk
+ *
+ * In the simplest case clk_set_rate will only change the rate of clk.
+ *
+ * If clk has the CLK_GATE_SET_RATE flag set and it is enabled this call
+ * will fail; only when the clk is disabled will it be able to change
+ * its rate.
+ *
+ * Setting the CLK_PARENT_SET_RATE flag allows clk_set_rate to
+ * recursively propagate up to clk's parent; whether or not this happens
+ * depends on the outcome of clk's .round_rate implementation.  If
+ * *parent_rate is 0 after calling .round_rate then upstream parent
+ * propagation is ignored.  If *parent_rate comes back with a new rate
+ * for clk's parent then we propagate up to clk's parent and set it's
+ * rate.  Upward propagation will continue until either a clk does not
+ * support the CLK_PARENT_SET_RATE flag or .round_rate stops requesting
+ * changes to clk's parent_rate.  If there is a failure during upstream
+ * propagation then clk_set_rate will unwind and restore each clk's rate
+ * that had been successfully changed.  Afterwards a rate change abort
+ * notification will be propagated downstream, starting from the clk
+ * that failed.
+ *
+ * At the end of all of the rate setting, clk_set_rate internally calls
+ * __clk_recalc_rates and propagates the rate changes downstream,
+ * starting from the highest clk whose rate was changed.  This has the
+ * added benefit of propagating post-rate change notifiers.
+ *
+ * Note that while post-rate change and rate change abort notifications
+ * are guaranteed to be sent to a clk only once per call to
+ * clk_set_rate, pre-change notifications will be sent for every clk
+ * whose rate is changed.  Stacking pre-change notifications is noisy
+ * for the drivers subscribed to them, but this allows drivers to react
+ * to intermediate clk rate changes up until the point where the final
+ * rate is achieved at the end of upstream propagation.
+ *
+ * Returns 0 on success, -EERROR otherwise.
+ */
+int clk_set_rate(struct clk *clk, unsigned long rate)
+{
+	struct clk *fail_clk;
+	int ret = 0;
+
+	/* prevent racing with updates to the clock topology */
+	mutex_lock(&prepare_lock);
+
+	/* bail early if nothing to do */
+	if (rate == clk->rate)
+		goto out;
+
+	fail_clk = __clk_set_rate(clk, rate);
+	if (fail_clk) {
+		pr_warn("%s: failed to set %s rate\n", __func__,
+				fail_clk->name);
+		/* FIXME propagate rate change abort notification here */
+		ret = -EIO;
+	}
+out:
+	mutex_unlock(&prepare_lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(clk_set_rate);
+
+/**
+ * clk_get_parent - return the parent of a clk
+ * @clk: the clk whose parent gets returned
+ *
+ * Simply returns clk->parent.  It is up to the caller to hold the
+ * prepare_lock, if desired.  Returns NULL if clk is NULL.
+ */
+struct clk *clk_get_parent(struct clk *clk)
+{
+	struct clk *parent;
+
+	if (!clk)
+		return NULL;
+
+	mutex_lock(&prepare_lock);
+	parent = clk->parent;
+	mutex_unlock(&prepare_lock);
+
+	return parent;
+}
+EXPORT_SYMBOL_GPL(clk_get_parent);
+
+void __clk_reparent(struct clk *clk, struct clk *new_parent)
+{
+	if (!clk || !new_parent)
+		return;
+
+	hlist_del(&clk->child_node);
+	hlist_add_head(&clk->child_node, &new_parent->children);
+
+	clk->parent = new_parent;
+
+	/* FIXME update sysfs clock topology */
+}
+
+/**
+ * clk_set_parent - switch the parent of a mux clk
+ * @clk: the mux clk whose input we are switching
+ * @parent: the new input to clk
+ *
+ * Re-parent clk to use parent as it's new input source.  If clk has the
+ * CLK_GATE_SET_PARENT flag set then clk must be gated for this
+ * operation to succeed.  After successfully changing clk's parent
+ * clk_set_parent will update the clk topology, sysfs topology and
+ * propagate rate recalculation via __clk_recalc_rates.  Returns 0 on
+ * success, -EERROR otherwise.
+ */
+int clk_set_parent(struct clk *clk, struct clk *parent)
+{
+	int ret = 0;
+
+	if (!clk || !clk->ops)
+		return -EINVAL;
+
+	if (!clk->ops->set_parent)
+		return -ENOSYS;
+
+	/* prevent racing with updates to the clock topology */
+	mutex_lock(&prepare_lock);
+
+	if (clk->parent == parent)
+		goto out;
+
+	/* FIXME add pre-rate change notification here */
+
+	/* only re-parent if the clock is not in use */
+	if ((clk->flags & CLK_GATE_SET_PARENT) && clk->prepare_count)
+		ret = -EBUSY;
+	else
+		ret = clk->ops->set_parent(clk, parent);
+
+	if (ret < 0)
+		/* FIXME add rate change abort notification here */
+		goto out;
+
+	/* update tree topology */
+	__clk_reparent(clk, parent);
+
+	/*
+	 * If successful recalculate the rates of the clock, including
+	 * children.
+	 */
+	__clk_recalc_rates(clk);
+
+out:
+	mutex_unlock(&prepare_lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(clk_set_parent);
+
+/**
+ * clk_init - initialize the data structures in a struct clk
+ * @dev: device initializing this clk, placeholder for now
+ * @clk: clk being initialized
+ *
+ * Initializes the lists in struct clk, queries the hardware for the
+ * parent and rate and sets them both.  Adds the clk to the sysfs tree
+ * topology.
+ *
+ * Caller must populate clk->name and clk->flags before calling
+ * clk_init.  A key assumption in the call to .get_parent is that clks
+ * are being initialized in-order.
+ */
+void clk_init(struct device *dev, struct clk *clk)
+{
+	if (!clk)
+		return;
+
+	INIT_HLIST_HEAD(&clk->children);
+	INIT_HLIST_NODE(&clk->child_node);
+
+	mutex_lock(&prepare_lock);
+
+	/*
+	 * .get_parent is mandatory for populating .parent for clks with
+	 * multiple possible parents.  For clks with a fixed parent
+	 * .get_parent is not mandatory and .parent can be statically
+	 * initialized
+	 */
+	if (clk->ops->get_parent)
+		clk->parent = clk->ops->get_parent(clk);
+
+	/* clks without a parent are considered root clks */
+	if (clk->parent)
+		hlist_add_head(&clk->child_node,
+				&clk->parent->children);
+	else
+		hlist_add_head(&clk->child_node, &clk_root_list);
+
+	if (clk->ops->recalc_rate)
+		clk->rate = clk->ops->recalc_rate(clk);
+	else
+		clk->rate = 0;
+
+	mutex_unlock(&prepare_lock);
+
+	return;
+}
+EXPORT_SYMBOL_GPL(clk_init);
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 7213b52..9cb8d72 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -3,6 +3,8 @@
  *
  *  Copyright (C) 2004 ARM Limited.
  *  Written by Deep Blue Solutions Limited.
+ *  Copyright (c) 2010-2011 Jeremy Kerr <jeremy.kerr@canonical.com>
+ *  Copyright (C) 2011 Linaro Ltd <mturquette@linaro.org>
  *
  * 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
@@ -12,18 +14,137 @@
 #define __LINUX_CLK_H
 
 #include <linux/kernel.h>
+#include <linux/errno.h>
 
 struct device;
 
+struct clk;
+
+#ifdef CONFIG_GENERIC_CLK
+
 /*
- * The base API.
+ * flags used across common struct clk.  these flags should only affect the
+ * top-level framework.  custom flags for dealing with hardware specifics
+ * belong in struct clk_hw_your_unique_device
  */
+#define CLK_GATE_SET_RATE	BIT(0) /* must be gated across rate change */
+#define CLK_GATE_SET_PARENT	BIT(1) /* must be gated across re-parent */
+#define CLK_PARENT_SET_RATE	BIT(2) /* propagate rate change up one level */
+#define CLK_IGNORE_UNUSED	BIT(3) /* do not gate even if unused */
 
+#define CLK_GATE_RATE_CHANGE	(CLK_GATE_SET_RATE | CLK_GATE_SET_PARENT)
 
-/*
- * struct clk - an machine class defined object / cookie.
+struct clk {
+	const char		*name;
+	const struct clk_hw_ops	*ops;
+	struct clk		*parent;
+	unsigned long		rate;
+	unsigned long		flags;
+	unsigned int		enable_count;
+	unsigned int		prepare_count;
+	struct hlist_head	children;
+	struct hlist_node	child_node;
+};
+
+/**
+ * struct clk_hw_ops -  Callback operations for hardware clocks; these are to
+ * be provided by the clock implementation, and will be called by drivers
+ * through the clk_* API.
+ *
+ * @prepare:	Prepare the clock for enabling. This must not return until
+ * 		the clock is fully prepared, and it's safe to call clk_enable.
+ * 		This callback is intended to allow clock implementations to
+ * 		do any initialisation that may sleep. Called with
+ * 		prepare_lock held.
+ *
+ * @unprepare:	Release the clock from its prepared state. This will typically
+ * 		undo any work done in the @prepare callback. Called with
+ * 		prepare_lock held.
+ *
+ * @enable:	Enable the clock atomically. This must not return until the
+ * 		clock is generating a valid clock signal, usable by consumer
+ * 		devices. Called with enable_lock held. This function must not
+ * 		sleep.
+ *
+ * @disable:	Disable the clock atomically. Called with enable_lock held.
+ * 		This function must not sleep.
+ *
+ * @recalc_rate	Recalculate the rate of this clock, by quering hardware
+ * 		and/or the clock's parent. It is up to the caller to insure
+ * 		that the prepare_mutex is held across this call.  Returns the
+ * 		calculated rate.  Optional, but recommended - if this op is not
+ * 		set then clock rate will be initialized to 0.
+ *
+ * @round_rate	XXX FIXME
+ *
+ * @get_parent	Return the parent of this clock; for clocks with multiple
+ * 		possible parents, query the hardware for the current parent.
+ * 		Clocks with fixed parents should still implement this and
+ * 		return something like struct clk *fixed_parent from their
+ * 		respective struct clk_hw_* data.  Currently called only when
+ * 		the clock is initialized.  Clocks that do not implement this
+ * 		will have their parent set to NULL.
+ *
+ * @set_parent	Change the input source of this clock; for clocks with multiple
+ * 		possible parents specify a new parent by passing in a struct
+ * 		clk *parent.  Returns 0 on success, -EERROR otherwise.
+ *
+ * @set_rate	Change the rate of this clock. If this callback returns
+ * 		CLK_PARENT_SET_RATE, the rate change will be propagated to the
+ * 		parent clock (which may propagate again if the parent clock
+ * 		also sets this flag). The requested rate of the parent is
+ * 		passed back from the callback in the second 'unsigned long *'
+ * 		argument.  Note that it is up to the hardware clock's set_rate
+ * 		implementation to insure that clocks do not run out of spec
+ * 		when propgating the call to set_rate up to the parent.  One way
+ * 		to do this is to gate the clock (via clk_disable and/or
+ * 		clk_unprepare) before calling clk_set_rate, then ungating it
+ * 		afterward.  If your clock also has the CLK_GATE_SET_RATE flag
+ * 		set then this will insure safety.  Returns 0 on success,
+ * 		-EERROR otherwise.
+ *
+ * The clk_enable/clk_disable and clk_prepare/clk_unprepare pairs allow
+ * implementations to split any work between atomic (enable) and sleepable
+ * (prepare) contexts.  If enabling a clock requires code that might sleep,
+ * this must be done in clk_prepare.  Clock enable code that will never be
+ * called in a sleepable context may be implement in clk_enable.
+ *
+ * Typically, drivers will call clk_prepare when a clock may be needed later
+ * (eg. when a device is opened), and clk_enable when the clock is actually
+ * required (eg. from an interrupt). Note that clk_prepare MUST have been
+ * called before clk_enable.
  */
-struct clk;
+struct clk_hw_ops {
+	int		(*prepare)(struct clk *clk);
+	void		(*unprepare)(struct clk *clk);
+	int		(*enable)(struct clk *clk);
+	void		(*disable)(struct clk *clk);
+	unsigned long	(*recalc_rate)(struct clk *clk);
+	long		(*round_rate)(struct clk *clk, unsigned long,
+					unsigned long *);
+	int		(*set_parent)(struct clk *clk, struct clk *);
+	struct clk *	(*get_parent)(struct clk *clk);
+	int		(*set_rate)(struct clk *clk, unsigned long);
+};
+
+/**
+ * clk_init - initialize the data structures in a struct clk
+ * @dev: device initializing this clk, placeholder for now
+ * @clk: clk being initialized
+ *
+ * Initializes the lists in struct clk, queries the hardware for the
+ * parent and rate and sets them both.  Adds the clk to the sysfs tree
+ * topology.
+ *
+ * Caller must populate .name, .flags and .ops before calling clk_init.
+ */
+void clk_init(struct device *dev, struct clk *clk);
+
+int __clk_prepare(struct clk *clk);
+void __clk_unprepare(struct clk *clk);
+void __clk_reparent(struct clk *clk, struct clk *new_parent);
+
+#endif /* !CONFIG_GENERIC_CLK */
 
 /**
  * clk_get - lookup and obtain a reference to a clock producer.
@@ -110,6 +231,7 @@ static inline void clk_unprepare(struct clk *clk)
 /**
  * clk_get_rate - obtain the current clock rate (in Hz) for a clock source.
  *		  This is only valid once the clock source has been enabled.
+ *		  Returns zero if the clock rate is unknown.
  * @clk: clock source
  */
 unsigned long clk_get_rate(struct clk *clk);
-- 
1.7.5.4

[PATCH v4 4/6] clk: introduce rate change notifiers

[Mike Turquette]

Many devices support dynamic clk frequency changes, but often need to
handle these clk frequency changes with care.  Examples of these devices
range from processors such as CPUs and GPUs to IO controllers such as
DDR controllers or MMC/SD card controllers and fixed-function hardware
such as video decoding logic.

To accomodate these needs this patch introduces clk rate change
notifiers to the common clk framework.  Drivers may subscribe to a
specific clk and recieve asynchronous notifications before a clk's rate
changes, after a clk's rate has been changed and also if the rate change
operation was aborted.

This patch is heavily based on Paul Walmsley's OMAP clk notifier
patches, of which several version have been posted over time:
http://article.gmane.org/gmane.linux.ports.arm.omap/18374
http://article.gmane.org/gmane.linux.ports.arm.omap/15200

There are subtle differences in this implementation, such as use of SRCU
notifers instead of blocking, and the use of a new .speculate_rate
function pointer (similar .recalc_rate) for predicting rate changes down
the sub-tree of clks.

Signed-off-by: Mike Turquette <mturquette@linaro.org>
Cc: Paul Walmsley <paul@pwsan.com>
---
 drivers/clk/clk.c   |  296 +++++++++++++++++++++++++++++++++++++++++++++++----
 include/linux/clk.h |   64 +++++++++++
 2 files changed, 340 insertions(+), 20 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 8cadadd..f86cb98 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -15,11 +15,13 @@
 #include <linux/spinlock.h>
 #include <linux/err.h>
 #include <linux/list.h>
+#include <linux/slab.h>
 
 static DEFINE_SPINLOCK(enable_lock);
 static DEFINE_MUTEX(prepare_lock);
 
 static HLIST_HEAD(clk_root_list);
+static LIST_HEAD(clk_notifier_list);
 
 /***        clk API        ***/
 
@@ -248,31 +250,128 @@ long clk_round_rate(struct clk *clk, unsigned long rate)
 EXPORT_SYMBOL_GPL(clk_round_rate);
 
 /**
+ * __clk_notify - call clk notifier chain
+ * @clk: struct clk * that is changing rate
+ * @msg: clk notifier type (see include/linux/clk.h)
+ * @old_rate: old clk rate
+ * @new_rate: new clk rate
+ *
+ * Triggers a notifier call chain on the clk rate-change notification
+ * for 'clk'.  Passes a pointer to the struct clk and the previous
+ * and current rates to the notifier callback.  Intended to be called by
+ * internal clock code only.  Returns NOTIFY_DONE from the last driver
+ * called if all went well, or NOTIFY_STOP or NOTIFY_BAD immediately if
+ * a driver returns that.
+ */
+static int __clk_notify(struct clk *clk, unsigned long msg,
+		unsigned long old_rate, unsigned long new_rate)
+{
+	struct clk_notifier *cn;
+	struct clk_notifier_data cnd;
+	int ret = NOTIFY_DONE;
+
+	cnd.clk = clk;
+	cnd.old_rate = old_rate;
+	cnd.new_rate = new_rate;
+
+	list_for_each_entry(cn, &clk_notifier_list, node) {
+		if (cn->clk == clk) {
+			ret = srcu_notifier_call_chain(&cn->notifier_head, msg,
+					&cnd);
+			break;
+		}
+	}
+
+	return ret;
+}
+
+/**
  * __clk_recalc_rates
  * @clk: first clk in the subtree
+ * @msg: notification type (see include/linux/clk.h)
  *
  * Walks the subtree of clks starting with clk and recalculates rates as it
  * goes.  Note that if a clk does not implement the recalc_rate operation then
  * propagation of that subtree stops and all of that clks children will not
- * have their rates updated.  Caller must hold prepare_lock.
+ * have their rates updated.
+ *
+ * clk_recalc_rates also propagates the POST_RATE_CHANGE notification,
+ * if necessary.
+ *
+ * Caller must hold prepare_lock.
+ */
+static void __clk_recalc_rates(struct clk *clk, unsigned long msg)
+{
+	unsigned long old_rate;
+	struct hlist_node *tmp;
+	struct clk *child;
+
+	old_rate = clk->rate;
+
+	if (clk->ops->recalc_rate)
+		clk->rate = clk->ops->recalc_rate(clk);
+
+	/* ignore return value for POST_RATE_CHANGE & ABORT_RATE_CHANGE */
+	if (clk->notifier_count)
+		__clk_notify(clk, msg, old_rate, clk->rate);
+
+	hlist_for_each_entry(child, tmp, &clk->children, child_node)
+		__clk_recalc_rates(child, msg);
+}
+
+/**
+ * __clk_speculate_rates
+ * @clk: first clk in the subtree
+ * @parent_rate: the "future" rate of clk's parent
+ *
+ * Walks the subtree of clks starting with clk, speculating rates as it
+ * goes and firing off PRE_RATE_CHANGE notifications as necessary.  Note
+ * that if a clk does not implement the speculate_rate operation then
+ * propagation of that subtree stops and all of that clks children will
+ * not have their rates speculated.
+ *
+ * Unlike clk_recalc_rates, clk_speculate_rates exists only for sending
+ * pre-rate change notifications and returns early if no clks in the
+ * subtree have subscribed to the notifications.
+ *
+ * It is not required to have a .speculate_rate callback, but without it
+ * a driver can never recieve a PRE_RATE_CHANGE notifier.  clks that
+ * only have a .recalc_rate callback still allow drivers to listen for
+ * POST_RATE_CHANGE notifications even in the absence of
+ * .speculate_rate.
+ *
+ * Caller must hold prepare_lock.
  */
-static void __clk_recalc_rates(struct clk *clk)
+static int __clk_speculate_rates(struct clk *clk, unsigned long parent_rate)
 {
 	struct hlist_node *tmp;
 	struct clk *child;
+	unsigned long new_rate;
+	int ret = NOTIFY_DONE;
 
-	if (!clk->ops->recalc_rate) {
-		pr_debug("%s: no .recalc_rate for clk %s\n",
+	if (!clk->ops->speculate_rate) {
+		pr_debug("%s: no .speculate_rate for clk %s\n",
 				__func__, clk->name);
-		return;
+		goto out;
 	}
 
-	clk->rate = clk->ops->recalc_rate(clk);
+	new_rate = clk->ops->speculate_rate(clk, parent_rate);
 
-	/* FIXME add post-rate change notification here */
+	/* abort the rate change if a driver returns NOTIFY_BAD */
+	if (clk->notifier_count)
+		ret = __clk_notify(clk, PRE_RATE_CHANGE, clk->rate, new_rate);
 
-	hlist_for_each_entry(child, tmp, &clk->children, child_node)
-		__clk_recalc_rates(child);
+	if (ret == NOTIFY_BAD)
+		goto out;
+
+	hlist_for_each_entry(child, tmp, &clk->children, child_node) {
+		ret = __clk_speculate_rates(child, new_rate);
+		if (ret == NOTIFY_BAD)
+			break;
+	}
+
+out:
+	return ret;
 }
 
 /**
@@ -286,11 +385,16 @@ static void __clk_recalc_rates(struct clk *clk)
  *
  * To prevent this consider setting the CLK_GATE_SET_RATE flag on any
  * clk where you also set the CLK_PARENT_SET_RATE flag
+ *
+ * PRE_RATE_CHANGE notifications are supposed to stack as a rate change
+ * request propagates up the clk tree.  This reflects the different
+ * rates that a downstream clk might experience if left enabled while
+ * upstream parents change their rates.
  */
 struct clk *__clk_set_rate(struct clk *clk, unsigned long rate)
 {
 	struct clk *fail_clk = NULL;
-	int ret = 0;
+	int ret = NOTIFY_DONE;
 	unsigned long old_rate = clk->rate;
 	unsigned long new_rate;
 	unsigned long parent_old_rate;
@@ -307,14 +411,25 @@ struct clk *__clk_set_rate(struct clk *clk, unsigned long rate)
 
 	new_rate = clk->ops->round_rate(clk, rate, &parent_new_rate);
 
-	/* FIXME propagate pre-rate change notification here */
-	/* XXX note that pre-rate change notifications will stack */
+	/* NOTE: pre-rate change notifications will stack */
+	if (clk->notifier_count)
+		ret = __clk_notify(clk, PRE_RATE_CHANGE, clk->rate, new_rate);
+
+	if (ret == NOTIFY_BAD)
+		return clk;
+
+	/* speculate rate changes down the tree */
+	hlist_for_each_entry(child, tmp, &clk->children, child_node) {
+		ret = __clk_speculate_rates(child, new_rate);
+		if (ret == NOTIFY_BAD)
+			return clk;
+	}
 
 	/* change the rate of this clk */
 	if (clk->ops->set_rate)
 		ret = clk->ops->set_rate(clk, new_rate);
 
-	if (ret)
+	if (ret == NOTIFY_BAD)
 		return clk;
 
 	/*
@@ -336,6 +451,19 @@ struct clk *__clk_set_rate(struct clk *clk, unsigned long rate)
 			pr_warn("%s: failed to set parent %s rate to %lu\n",
 					__func__, fail_clk->name,
 					parent_new_rate);
+
+			/*
+			 * Send PRE_RATE_CHANGE notifiers down the tree
+			 * again, since we're rolling back the rate
+			 * changes due to the abort.
+			 *
+			 * Ignore any NOTIFY_BAD's since this *is* the
+			 * exception handler.
+			 *
+			 * NOTE: pre-rate change notifications will stack
+			 */
+			__clk_speculate_rates(clk, clk->parent->rate);
+
 			clk->ops->set_rate(clk, old_rate);
 		}
 		return fail_clk;
@@ -350,8 +478,7 @@ struct clk *__clk_set_rate(struct clk *clk, unsigned long rate)
 	 * and post-rate change notifiers down the clk tree from this
 	 * point.
 	 */
-	/* FIXME propagate post-rate change notifier starting with this clk */
-	__clk_recalc_rates(clk);
+	__clk_recalc_rates(clk, POST_RATE_CHANGE);
 
 	return NULL;
 }
@@ -412,9 +539,10 @@ int clk_set_rate(struct clk *clk, unsigned long rate)
 	if (fail_clk) {
 		pr_warn("%s: failed to set %s rate\n", __func__,
 				fail_clk->name);
-		/* FIXME propagate rate change abort notification here */
+		__clk_recalc_rates(clk, ABORT_RATE_CHANGE);
 		ret = -EIO;
 	}
+
 out:
 	mutex_unlock(&prepare_lock);
 
@@ -485,7 +613,13 @@ int clk_set_parent(struct clk *clk, struct clk *parent)
 	if (clk->parent == parent)
 		goto out;
 
-	/* FIXME add pre-rate change notification here */
+	/* propagate PRE_RATE_CHANGE notifications */
+	if (clk->notifier_count)
+		ret = __clk_speculate_rates(clk, parent->rate);
+
+	/* abort if a driver objects */
+	if (ret == NOTIFY_STOP)
+		goto out;
 
 	/* only re-parent if the clock is not in use */
 	if ((clk->flags & CLK_GATE_SET_PARENT) && clk->prepare_count)
@@ -493,10 +627,16 @@ int clk_set_parent(struct clk *clk, struct clk *parent)
 	else
 		ret = clk->ops->set_parent(clk, parent);
 
-	if (ret < 0)
-		/* FIXME add rate change abort notification here */
+	/* propagate ABORT_RATE_CHANGE if .set_parent failed */
+	if (ret) {
+		__clk_recalc_rates(clk, ABORT_RATE_CHANGE);
 		goto out;
+	}
 
+	/*
+	 * FIXME prepare_count, enable_count & notifier_count should
+	 * migrate which switching parents
+	 */
 	/* update tree topology */
 	__clk_reparent(clk, parent);
 
@@ -504,7 +644,7 @@ int clk_set_parent(struct clk *clk, struct clk *parent)
 	 * If successful recalculate the rates of the clock, including
 	 * children.
 	 */
-	__clk_recalc_rates(clk);
+	__clk_recalc_rates(clk, POST_RATE_CHANGE);
 
 out:
 	mutex_unlock(&prepare_lock);
@@ -562,3 +702,119 @@ void clk_init(struct device *dev, struct clk *clk)
 	return;
 }
 EXPORT_SYMBOL_GPL(clk_init);
+
+/***        clk rate change notifiers        ***/
+
+/**
+ * clk_notifier_register - add a clk rate change notifier
+ * @clk: struct clk * to watch
+ * @nb: struct notifier_block * with callback info
+ *
+ * Request notification when clk's rate changes.  This uses an SRCU
+ * notifier because we want it to block and notifier unregistrations are
+ * uncommon.  The callbacks associated with the notifier must not
+ * re-enter into the clk framework by calling any top-level clk APIs;
+ * this will cause a nested prepare_lock mutex.
+ *
+ * Pre-change notifier callbacks will be passed the current, pre-change
+ * rate of the clk via struct clk_notifier_data.old_rate.  The new,
+ * post-change rate of the clk is passed via struct
+ * clk_notifier.new_rate.
+ *
+ * Post-change notifiers will pass the now-current, post-change rate of
+ * the clk in both struct clk_notifier_data.old_rate and struct
+ * clk_notifier_data.new_rate.
+ *
+ * Abort-change notifiers are effectively the opposite of pre-change
+ * notifiers: the original pre-change clk rate is passed in via struct
+ * clk_notifier_data.new_rate and the failed post-change rate is passed
+ * in via struct clk_notifier_data.old_rate.
+ *
+ * clk_notifier_register() must be called from non-atomic context.
+ * Returns -EINVAL if called with null arguments, -ENOMEM upon
+ * allocation failure; otherwise, passes along the return value of
+ * srcu_notifier_chain_register().
+ */
+int clk_notifier_register(struct clk *clk, struct notifier_block *nb)
+{
+	struct clk_notifier *cn;
+	int ret = -ENOMEM;
+
+	if (!clk || !nb)
+		return -EINVAL;
+
+	mutex_lock(&prepare_lock);
+
+	/* search the list of notifiers for this clk */
+	list_for_each_entry(cn, &clk_notifier_list, node)
+		if (cn->clk == clk)
+			break;
+
+	/* if clk wasn't in the notifier list, allocate new clk_notifier */
+	if (cn->clk != clk) {
+		cn = kzalloc(sizeof(struct clk_notifier), GFP_KERNEL);
+		if (!cn)
+			goto out;
+
+		cn->clk = clk;
+		srcu_init_notifier_head(&cn->notifier_head);
+
+		list_add(&cn->node, &clk_notifier_list);
+	}
+
+	ret = srcu_notifier_chain_register(&cn->notifier_head, nb);
+
+	clk->notifier_count++;
+
+out:
+	mutex_unlock(&prepare_lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(clk_notifier_register);
+
+/**
+ * clk_notifier_unregister - remove a clk rate change notifier
+ * @clk: struct clk *
+ * @nb: struct notifier_block * with callback info
+ *
+ * Request no further notification for changes to 'clk' and frees memory
+ * allocated in clk_notifier_register.
+ *
+ * Returns -EINVAL if called with null arguments; otherwise, passes
+ * along the return value of srcu_notifier_chain_unregister().
+ */
+int clk_notifier_unregister(struct clk *clk, struct notifier_block *nb)
+{
+	struct clk_notifier *cn = NULL;
+	int ret = -EINVAL;
+
+	if (!clk || !nb)
+		return -EINVAL;
+
+	mutex_lock(&prepare_lock);
+
+	list_for_each_entry(cn, &clk_notifier_list, node)
+		if (cn->clk == clk)
+			break;
+
+	if (cn->clk == clk) {
+		ret = srcu_notifier_chain_unregister(&cn->notifier_head, nb);
+
+		clk->notifier_count--;
+
+		/* XXX the notifier code should handle this better */
+		if (!cn->notifier_head.head) {
+			srcu_cleanup_notifier_head(&cn->notifier_head);
+			kfree(cn);
+		}
+
+	} else {
+		ret = -ENOENT;
+	}
+
+	mutex_unlock(&prepare_lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(clk_notifier_unregister);
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 9cb8d72..63a72f2 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -14,6 +14,7 @@
 #define __LINUX_CLK_H
 
 #include <linux/kernel.h>
+#include <linux/notifier.h>
 #include <linux/errno.h>
 
 struct device;
@@ -44,6 +45,7 @@ struct clk {
 	unsigned int		prepare_count;
 	struct hlist_head	children;
 	struct hlist_node	child_node;
+	unsigned int		notifier_count;
 };
 
 /**
@@ -120,6 +122,8 @@ struct clk_hw_ops {
 	int		(*enable)(struct clk *clk);
 	void		(*disable)(struct clk *clk);
 	unsigned long	(*recalc_rate)(struct clk *clk);
+	unsigned long	(*speculate_rate)(struct clk *clk,
+					unsigned long parent_rate);
 	long		(*round_rate)(struct clk *clk, unsigned long,
 					unsigned long *);
 	int		(*set_parent)(struct clk *clk, struct clk *);
@@ -144,6 +148,66 @@ int __clk_prepare(struct clk *clk);
 void __clk_unprepare(struct clk *clk);
 void __clk_reparent(struct clk *clk, struct clk *new_parent);
 
+/**
+ * DOC: clk notifier callback types
+ *
+ * PRE_RATE_CHANGE - called immediately before the clk rate is changed,
+ *     to indicate that the rate change will proceed.  Drivers must
+ *     immediately terminate any operations that will be affected by the
+ *     rate change.  Callbacks may either return NOTIFY_DONE or
+ *     NOTIFY_STOP.
+ *
+ * ABORT_RATE_CHANGE: called if the rate change failed for some reason
+ *     after PRE_RATE_CHANGE.  In this case, all registered notifiers on
+ *     the clk will be called with ABORT_RATE_CHANGE. Callbacks must
+ *     always return NOTIFY_DONE.
+ *
+ * POST_RATE_CHANGE - called after the clk rate change has successfully
+ *     completed.  Callbacks must always return NOTIFY_DONE.
+ *
+ */
+#define PRE_RATE_CHANGE			BIT(0)
+#define POST_RATE_CHANGE		BIT(1)
+#define ABORT_RATE_CHANGE		BIT(2)
+
+/**
+ * struct clk_notifier - associate a clk with a notifier
+ * @clk: struct clk * to associate the notifier with
+ * @notifier_head: a blocking_notifier_head for this clk
+ * @node: linked list pointers
+ *
+ * A list of struct clk_notifier is maintained by the notifier code.
+ * An entry is created whenever code registers the first notifier on a
+ * particular @clk.  Future notifiers on that @clk are added to the
+ * @notifier_head.
+ */
+struct clk_notifier {
+	struct clk			*clk;
+	struct srcu_notifier_head	notifier_head;
+	struct list_head		node;
+};
+
+/**
+ * struct clk_notifier_data - rate data to pass to the notifier callback
+ * @clk: struct clk * being changed
+ * @old_rate: previous rate of this clk
+ * @new_rate: new rate of this clk
+ *
+ * For a pre-notifier, old_rate is the clk's rate before this rate
+ * change, and new_rate is what the rate will be in the future.  For a
+ * post-notifier, old_rate and new_rate are both set to the clk's
+ * current rate (this was done to optimize the implementation).
+ */
+struct clk_notifier_data {
+	struct clk		*clk;
+	unsigned long		old_rate;
+	unsigned long		new_rate;
+};
+
+int clk_notifier_register(struct clk *clk, struct notifier_block *nb);
+
+int clk_notifier_unregister(struct clk *clk, struct notifier_block *nb);
+
 #endif /* !CONFIG_GENERIC_CLK */
 
 /**
-- 
1.7.5.4

[PATCH v4 5/6] clk: basic gateable and fixed-rate clks

[Mike Turquette]

Many platforms support simple gateable clks and fixed-rate clks that
should not be re-implemented by every platform.

This patch introduces a gateable clk with a common programming model of
gate control via a write of 1 bit to a register.  Both set-to-enable and
clear-to-enable are supported.

Also introduced is a fixed-rate clk which has no reprogrammable aspects.

The purpose of both types of clks is documented in drivers/clk/basic.c.

TODO: add support for a simple divider, simple mux and a dummy clk for
stubbing out platform support.

Based on original patch by Jeremy Kerr and contribution by Jamie Iles.

Signed-off-by: Mike Turquette <mturquette@linaro.org>
Cc: Jeremy Kerr <jeremy.kerr@canonical.com>
Cc: Jamie Iles <jamie@jamieiles.com>
---
 drivers/clk/Kconfig     |    7 ++
 drivers/clk/Makefile    |    5 +-
 drivers/clk/clk-basic.c |  208 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/clk.h     |   35 ++++++++
 4 files changed, 253 insertions(+), 2 deletions(-)
 create mode 100644 drivers/clk/clk-basic.c

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index adc0586..ba7eb8c 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -12,3 +12,10 @@ config HAVE_CLK_PREPARE
 config GENERIC_CLK
 	bool
 	select HAVE_CLK_PREPARE
+
+config GENERIC_CLK_BASIC
+	bool "Basic clock definitions"
+	depends on GENERIC_CLK
+	help
+	   Allow use of basic, single-function clock types.  These
+	   common definitions can be used across many platforms.
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 570d5b9..68b20a1 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -1,3 +1,4 @@
 
-obj-$(CONFIG_CLKDEV_LOOKUP)	+= clkdev.o
-obj-$(CONFIG_GENERIC_CLK)	+= clk.o
+obj-$(CONFIG_CLKDEV_LOOKUP)		+= clkdev.o
+obj-$(CONFIG_GENERIC_CLK)		+= clk.o
+obj-$(CONFIG_GENERIC_CLK_BASIC)		+= clk-basic.o
diff --git a/drivers/clk/clk-basic.c b/drivers/clk/clk-basic.c
new file mode 100644
index 0000000..a065bc8
--- /dev/null
+++ b/drivers/clk/clk-basic.c
@@ -0,0 +1,208 @@
+/*
+ * Copyright (C) 2010-2011 Canonical Ltd <jeremy.kerr@canonical.com>
+ * Copyright (C) 2011 Linaro Ltd <mturquette@linaro.org>
+ *
+ * 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.
+ *
+ * Basic single-function clk implementations
+ */
+
+/* TODO add basic divider clk, basic mux clk and dummy clk */
+
+#include <linux/clk.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/io.h>
+
+/*
+ * NOTE: all of the basic clks here are just that: single-function
+ * simple clks.  One assumption in their implementation is that existing
+ * locking mechanisms (prepare_mutex and enable_spinlock) are enough to
+ * prevent race conditions during register accesses.  If this is not
+ * true for your platform then please implement your own version of
+ * these clks which take such issues into account.
+ */
+
+#define to_clk_hw_gate(ck) container_of(ck, struct clk_hw_gate, clk)
+#define to_clk_hw_fixed(ck) container_of(ck, struct clk_hw_fixed, clk)
+
+/**
+ * DOC: basic gatable clock which can gate and ungate it's ouput
+ *
+ * Traits of this clock:
+ * prepare - clk_prepare & clk_unprepare do nothing
+ * enable - clk_enable and clk_disable are functional & control gating
+ * rate - inherits rate from parent.  No clk_set_rate support
+ * parent - fixed parent.  No clk_set_parent support
+ *
+ * note: parent should not be NULL for this clock, but we check because we're
+ * paranoid
+ */
+
+static unsigned long clk_hw_gate_recalc_rate(struct clk *clk)
+{
+	if (clk->parent)
+		return clk->parent->rate;
+	else
+		return 0;
+}
+
+static struct clk *clk_hw_gate_get_parent(struct clk *clk)
+{
+	return to_clk_hw_gate(clk)->fixed_parent;
+}
+
+static void clk_hw_gate_set_bit(struct clk *clk)
+{
+	struct clk_hw_gate *gate = to_clk_hw_gate(clk);
+	u32 reg;
+
+	reg = readl(gate->reg);
+	reg |= BIT(gate->bit_idx);
+	writel(reg, gate->reg);
+}
+
+static void clk_hw_gate_clear_bit(struct clk *clk)
+{
+	struct clk_hw_gate *gate = to_clk_hw_gate(clk);
+	u32 reg;
+
+	reg = readl(gate->reg);
+	reg &= ~BIT(gate->bit_idx);
+	writel(reg, gate->reg);
+}
+
+static int clk_hw_gate_enable_set(struct clk *clk)
+{
+	clk_hw_gate_set_bit(clk);
+
+	return 0;
+}
+
+static void clk_hw_gate_disable_clear(struct clk *clk)
+{
+	clk_hw_gate_clear_bit(clk);
+}
+
+struct clk_hw_ops clk_hw_gate_set_enable_ops = {
+	.enable = clk_hw_gate_enable_set,
+	.disable = clk_hw_gate_disable_clear,
+	.recalc_rate = clk_hw_gate_recalc_rate,
+	.get_parent = clk_hw_gate_get_parent,
+};
+EXPORT_SYMBOL_GPL(clk_hw_gate_set_enable_ops);
+
+static int clk_hw_gate_enable_clear(struct clk *clk)
+{
+	clk_hw_gate_clear_bit(clk);
+
+	return 0;
+}
+
+static void clk_hw_gate_disable_set(struct clk *clk)
+{
+	clk_hw_gate_set_bit(clk);
+}
+
+struct clk_hw_ops clk_hw_gate_set_disable_ops = {
+	.enable = clk_hw_gate_enable_clear,
+	.disable = clk_hw_gate_disable_set,
+	.recalc_rate = clk_hw_gate_recalc_rate,
+	.get_parent = clk_hw_gate_get_parent,
+};
+EXPORT_SYMBOL_GPL(clk_hw_gate_set_disable_ops);
+
+int clk_register_gate(struct device *dev, const char *name, unsigned long flags,
+		struct clk *fixed_parent, void __iomem *reg, u8 bit_idx,
+		int set_to_enable)
+{
+	struct clk_hw_gate *gclk;
+	struct clk *clk;
+
+	gclk = kmalloc(sizeof(struct clk_hw_gate), GFP_KERNEL);
+
+	if (!gclk) {
+		pr_err("%s: could not allocate gated clk\n", __func__);
+		return -ENOMEM;
+	}
+
+	clk = &gclk->clk;
+
+	/* struct clk_hw_gate assignments */
+	gclk->fixed_parent = fixed_parent;
+	gclk->reg = reg;
+	gclk->bit_idx = bit_idx;
+
+	/* struct clk assignments */
+	clk->name = name;
+	clk->flags = flags;
+
+	if (set_to_enable)
+		clk->ops = &clk_hw_gate_set_enable_ops;
+	else
+		clk->ops = &clk_hw_gate_set_disable_ops;
+
+	clk_init(NULL, clk);
+
+	return 0;
+}
+
+/*
+ * DOC: basic fixed-rate clock that cannot gate
+ *
+ * Traits of this clock:
+ * prepare - clock never gates.  clk_prepare & clk_unprepare do nothing
+ * enable - clock never gates.  clk_enable & clk_disable do nothing
+ * rate - rate is always a fixed value.  No clk_set_rate support
+ * parent - fixed parent.  No clk_set_parent support
+ *
+ * note: parent can be NULL, which implies that this clock is a root clock.
+ */
+
+static unsigned long clk_hw_fixed_recalc_rate(struct clk *clk)
+{
+	return to_clk_hw_fixed(clk)->fixed_rate;
+}
+
+static struct clk *clk_hw_fixed_get_parent(struct clk *clk)
+{
+	return to_clk_hw_fixed(clk)->fixed_parent;
+}
+
+struct clk_hw_ops clk_hw_fixed_ops = {
+	.recalc_rate = clk_hw_fixed_recalc_rate,
+	.get_parent = clk_hw_fixed_get_parent,
+};
+EXPORT_SYMBOL_GPL(clk_hw_fixed_ops);
+
+int clk_register_fixed(struct device *dev, const char *name,
+		unsigned long flags, struct clk *fixed_parent,
+		unsigned long fixed_rate)
+{
+	struct clk_hw_fixed *fclk;
+	struct clk *clk;
+
+	fclk = kmalloc(sizeof(struct clk_hw_fixed), GFP_KERNEL);
+
+	if (!fclk) {
+		pr_err("%s: could not allocate fixed clk\n", __func__);
+		return -ENOMEM;
+	}
+
+	clk = &fclk->clk;
+
+	/* struct clk_hw_fixed assignments */
+	fclk->fixed_parent = fixed_parent;
+	fclk->fixed_rate = fixed_rate;
+
+	/* struct clk assignments */
+	clk->name = name;
+	clk->flags = flags;
+	clk->ops = &clk_hw_fixed_ops;
+
+	clk_init(NULL, clk);
+
+	return 0;
+}
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 63a72f2..55049ee 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -131,6 +131,41 @@ struct clk_hw_ops {
 	int		(*set_rate)(struct clk *clk, unsigned long);
 };
 
+/*
+ * Base clock implementations. Platform clock implementations can use these
+ * directly, or 'subclass' as approprate
+ */
+
+#ifdef CONFIG_GENERIC_CLK_BASIC
+
+struct clk_hw_fixed {
+	struct clk	clk;
+	struct clk	*fixed_parent;
+	unsigned long	fixed_rate;
+};
+
+extern struct clk_hw_ops clk_hw_fixed_ops;
+
+int clk_register_fixed(struct device *dev, const char *name,
+		unsigned long flags, struct clk *fixed_parent,
+		unsigned long fixed_rate);
+
+struct clk_hw_gate {
+	struct clk	clk;
+	struct clk	*fixed_parent;
+	void __iomem	*reg;
+	u8		bit_idx;
+};
+
+extern struct clk_hw_ops clk_hw_gate_set_enable_ops;
+extern struct clk_hw_ops clk_hw_gate_set_disable_ops;
+
+int clk_register_gate(struct device *dev, const char *name, unsigned long flags,
+		struct clk *fixed_parent, void __iomem *reg, u8 bit_idx,
+		int set_to_enable);
+
+#endif
+
 /**
  * clk_init - initialize the data structures in a struct clk
  * @dev: device initializing this clk, placeholder for now
-- 
1.7.5.4

[PATCH v4 6/6] clk: export the clk tree topology to debugfs

[Mike Turquette]

Represents the clk tree as a directory hieraching in debugfs.  Each clk
is a directory filled with the following read-only entries:

clk_rate
clk_flags
clk_prepare_count
clk_enable_count
clk_notifier_count

This commit borrows some code from Yong Shen's patch to export clkdev
clk's to debugfs:
http://git.pengutronix.de/?p=imx/linux-2.6.git;a=commit;h=30aa15230747b3b92da16d841b1cf369f07192e7

Signed-off-by: Mike Turquette <mturquette@linaro.org>
Cc: Yong Shen <yong.shen@linaro.org>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/clk/Kconfig |    9 +++
 drivers/clk/clk.c   |  176 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 include/linux/clk.h |    3 +
 3 files changed, 186 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index ba7eb8c..09cc198 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -19,3 +19,12 @@ config GENERIC_CLK_BASIC
 	help
 	   Allow use of basic, single-function clock types.  These
 	   common definitions can be used across many platforms.
+
+config GENERIC_CLK_DEBUG
+	bool "Clock tree representation in debugs"
+	depends on GENERIC_CLK
+	help
+	  Creates a directory hierchy in debugfs for visualizing the clk
+	  tree structure as well.  Each directory contains read-only
+	  members that export information specific to that clk node:
+	  clk_rate, clk_flags, clk_prepare_count & clk_enable_count.
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index f86cb98..a6ddbb1 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -23,6 +23,166 @@ static DEFINE_MUTEX(prepare_lock);
 static HLIST_HEAD(clk_root_list);
 static LIST_HEAD(clk_notifier_list);
 
+/***        debugfs support        ***/
+
+#ifdef CONFIG_GENERIC_CLK_DEBUG
+#include <linux/debugfs.h>
+
+static struct dentry *rootdir;
+static int inited = 0;
+
+/* caller must hold prepare_lock */
+static int clk_debug_create_one(struct clk *clk, struct dentry *pdentry)
+{
+	struct dentry *d;
+	int ret = -ENOMEM;
+
+	if (!clk || !pdentry) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	d = debugfs_create_dir(clk->name, pdentry);
+	if (!d)
+		goto out;
+
+	clk->dentry = d;
+
+	d = debugfs_create_u64("clk_rate", S_IRUGO, clk->dentry,
+			(u64 *)&clk->rate);
+	if (!d)
+		goto err_out;
+
+	d = debugfs_create_x32("clk_flags", S_IRUGO, clk->dentry,
+			(u32 *)&clk->flags);
+	if (!d)
+		goto err_out;
+
+	d = debugfs_create_u32("clk_prepare_count", S_IRUGO, clk->dentry,
+			(u32 *)&clk->prepare_count);
+	if (!d)
+		goto err_out;
+
+	d = debugfs_create_u32("clk_enable_count", S_IRUGO, clk->dentry,
+			(u32 *)&clk->enable_count);
+	if (!d)
+		goto err_out;
+
+	d = debugfs_create_u32("clk_notifier_count", S_IRUGO, clk->dentry,
+			(u32 *)&clk->notifier_count);
+	if (!d)
+		goto err_out;
+
+	ret = 0;
+	goto out;
+
+err_out:
+	debugfs_remove(clk->dentry);
+out:
+	return ret;
+}
+
+/* caller must hold prepare_lock */
+static int clk_debug_create_subtree(struct clk *clk, struct dentry *pdentry)
+{
+	struct clk *child;
+	struct hlist_node *tmp;
+	int ret = -EINVAL;;
+
+	if (!clk || !pdentry)
+		goto out;
+
+	ret = clk_debug_create_one(clk, pdentry);
+
+	if (ret)
+		goto out;
+
+	hlist_for_each_entry(child, tmp, &clk->children, child_node)
+		clk_debug_create_subtree(child, clk->dentry);
+
+	ret = 0;
+out:
+	return ret;
+}
+
+/**
+ * clk_debug_register - add a clk node to the debugfs clk tree
+ * @clk: the clk being added to the debugfs clk tree
+ *
+ * Dynamically adds a clk to the debugfs clk tree if debugfs has been
+ * initialized.  Otherwise it bails out early since the debugfs clk tree
+ * will be created lazily by clk_debug_init as part of a late_initcall.
+ *
+ * Caller must hold prepare_lock.  Only clk_init calls this function (so
+ * far) so this is taken care.
+ */
+static int clk_debug_register(struct clk *clk)
+{
+	struct clk *parent;
+	struct dentry *pdentry;
+	int ret = 0;
+
+	if (!inited)
+		goto out;
+
+	parent = clk->parent;
+
+	/*
+	 * Check to see if a clk is a root clk.  Also check that it is
+	 * safe to add this clk to debugfs
+	 */
+	if (!parent)
+		pdentry = rootdir;
+	else
+		if (parent->dentry)
+			pdentry = parent->dentry;
+		else
+			goto out;
+
+	ret = clk_debug_create_subtree(clk, pdentry);
+
+out:
+	return ret;
+}
+
+/**
+ * clk_debug_init - lazily create the debugfs clk tree visualization
+ *
+ * clks are often initialized very early during boot before memory can
+ * be dynamically allocated and well before debugfs is setup.
+ * clk_debug_init walks the clk tree hierarchy while holding
+ * prepare_lock and creates the topology as part of a late_initcall,
+ * thus insuring that clks initialized very early will still be
+ * represented in the debugfs clk tree.  This function should only be
+ * called once at boot-time, and all other clks added dynamically will
+ * be done so with clk_debug_register.
+ */
+static int __init clk_debug_init(void)
+{
+	struct clk *root_clk;
+	struct hlist_node *tmp;
+
+	rootdir = debugfs_create_dir("clk", NULL);
+
+	if (!rootdir)
+		return -ENOMEM;
+
+	mutex_lock(&prepare_lock);
+
+	hlist_for_each_entry(root_clk, tmp, &clk_root_list, child_node)
+		clk_debug_create_subtree(root_clk, rootdir);
+
+	inited = 1;
+
+	mutex_unlock(&prepare_lock);
+
+	return 0;
+}
+late_initcall(clk_debug_init);
+#else
+static inline int clk_debug_register(struct clk *clk) { return 0; }
+#endif /* CONFIG_GENERIC_CLK_DEBUG */
+
 /***        clk API        ***/
 
 void __clk_unprepare(struct clk *clk)
@@ -574,15 +734,25 @@ EXPORT_SYMBOL_GPL(clk_get_parent);
 
 void __clk_reparent(struct clk *clk, struct clk *new_parent)
 {
+	struct dentry *d;
+
 	if (!clk || !new_parent)
 		return;
 
 	hlist_del(&clk->child_node);
 	hlist_add_head(&clk->child_node, &new_parent->children);
 
-	clk->parent = new_parent;
+#ifdef CONFIG_GENERIC_CLK_DEBUG
+	d = debugfs_rename(clk->parent->dentry, clk->dentry,
+			new_parent->dentry, clk->name);
+	if (d)
+		clk->dentry = d;
+	else
+		pr_debug("%s: failed to reparent debugfs entry for %s\n",
+				__func__, clk->name);
+#endif
 
-	/* FIXME update sysfs clock topology */
+	clk->parent = new_parent;
 }
 
 /**
@@ -697,6 +867,8 @@ void clk_init(struct device *dev, struct clk *clk)
 	else
 		clk->rate = 0;
 
+	clk_debug_register(clk);
+
 	mutex_unlock(&prepare_lock);
 
 	return;
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 55049ee..6cf3e2b 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -46,6 +46,9 @@ struct clk {
 	struct hlist_head	children;
 	struct hlist_node	child_node;
 	unsigned int		notifier_count;
+#ifdef CONFIG_GENERIC_CLK_DEBUG
+	struct dentry		*dentry;
+#endif
 };
 
 /**
-- 
1.7.5.4

Re: [PATCH v4 0/6] common clk framework

[Turquette, Mike]

On Tue, Dec 13, 2011 at 7:53 PM, Mike Turquette <mturquette@linaro.org> wrote:
> From: Mike Turquette <mturquette@ti.com>
>
> The common clk framework is an attempt to define a generic struct clk
> which most platforms can use to build a clk tree and perform a set of
> well-defined operations.

Forgot to mention: these patches are based on v3.2-rc5 and can be pulled from:
http://git.linaro.org/gitweb?p=people/mturquette/linux.git;a=shortlog;h=refs/heads/v3.2-rc5-clkv4
git://git.linaro.org/people/mturquette/linux.git v3.2-rc5-clk-v4

Regards,
Mike

> The previous patchset, v3, can be found at,
> http://article.gmane.org/gmane.linux.kernel/1218622
>
> New stuff in v4:
>  * clk rate change notifiers
>  * clk debug info via debugfs (instead of sysfs)
>  * lots of bug fixes
>
> Stuff that is known to be missing in v4:
>  * basic mux and divider clk types
>  * fix for migrating clk_prepare_count/clk_enable_count in
>   clk_set_parent
>  * minor rework comments from v3
>  * Documentation/clk.txt needs love
>
> All of the mising items above will be rolled into v5 ASAP.  I wanted to
> go ahead and push out the new notifier changes for review and gather
> comments on those since those were a big gap in the v3 patchset.
>
> Paul W. also had some good comments about the greater clk API, and the
> opportunity to fix some of that stuff while this patchset is still under
> discussion.  I didn't address those here because they require more
> thought, and more comments from reviewers.
>
> Finally, OMAP4 support for the common struct clk will be posted
> immediately after this patch series to LAKML and LOML, along with some
> hack patches that show how to use the recursive clk_set_rate for
> propagating rate changes up the tree for CPUfreq and how to use the new
> clk rate change notifiers in a driver.
>
> Mike Turquette (6):
>  clk: Kconfig: add entry for HAVE_CLK_PREPARE
>  Documentation: common clk API
>  clk: introduce the common clock framework
>  clk: introduce rate change notifiers
>  clk: basic gateable and fixed-rate clks
>  clk: export the clk tree topology to debugfs
>
>  Documentation/clk.txt   |  312 +++++++++++++++
>  drivers/clk/Kconfig     |   23 ++
>  drivers/clk/Makefile    |    4 +-
>  drivers/clk/clk-basic.c |  208 ++++++++++
>  drivers/clk/clk.c       |  992 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/clk.h     |  230 +++++++++++-
>  6 files changed, 1765 insertions(+), 4 deletions(-)
>  create mode 100644 Documentation/clk.txt
>  create mode 100644 drivers/clk/clk-basic.c
>  create mode 100644 drivers/clk/clk.c
>
> --
> 1.7.5.4
>

Re: [PATCH v4 3/6] clk: introduce the common clock framework

[Ryan Mallon]

On 14/12/11 14:53, Mike Turquette wrote:

> The common clk framework is an attempt to define a common struct clk
> which can be used by most platforms, and an API that drivers can always
> use safely for managing clks.
> 
> The net result is consolidation of many different struct clk definitions
> and platform-specific clk framework implementations.
> 
> This patch introduces the common struct clk, struct clk_ops and
> definitions for the long-standing clk API in include/clk/clk.h.
> Platforms may define their own hardware-specific clk structure, so long
> as it wraps an instance of the common struct clk which is passed to
> clk_init at initialization time.  From this point on all of the usual
> clk APIs should be supported, so long as the platform supports them
> through hardware-specific callbacks in struct clk_ops.
> 
> See Documentation/clk.txt for more details.
> 
> This patch is based on the work of Jeremy Kerr, which in turn was based
> on the work of Ben Herrenschmidt.  Big thanks to both of them for
> kickstarting this effort.
> 
> Signed-off-by: Mike Turquette <mturquette@linaro.org>
> Cc: Jeremy Kerr <jeremy.kerr@canonical.com>


Hi Mike,

Some comments below,

~Ryan

> ---
>  drivers/clk/Kconfig  |    4 +
>  drivers/clk/Makefile |    1 +
>  drivers/clk/clk.c    |  564 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/clk.h  |  130 +++++++++++-
>  4 files changed, 695 insertions(+), 4 deletions(-)
>  create mode 100644 drivers/clk/clk.c
> 
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index 7a9899bd..adc0586 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -8,3 +8,7 @@ config HAVE_MACH_CLKDEV
>  
>  config HAVE_CLK_PREPARE
>  	bool
> +
> +config GENERIC_CLK
> +	bool
> +	select HAVE_CLK_PREPARE
> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index 07613fa..570d5b9 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -1,2 +1,3 @@
>  
>  obj-$(CONFIG_CLKDEV_LOOKUP)	+= clkdev.o
> +obj-$(CONFIG_GENERIC_CLK)	+= clk.o
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> new file mode 100644
> index 0000000..8cadadd
> --- /dev/null
> +++ b/drivers/clk/clk.c
> @@ -0,0 +1,564 @@
> +/*
> + * Copyright (C) 2010-2011 Canonical Ltd <jeremy.kerr@canonical.com>
> + * Copyright (C) 2011 Linaro Ltd <mturquette@linaro.org>
> + *
> + * 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.  See Documentation/clk.txt
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/spinlock.h>
> +#include <linux/err.h>
> +#include <linux/list.h>
> +
> +static DEFINE_SPINLOCK(enable_lock);
> +static DEFINE_MUTEX(prepare_lock);
> +
> +static HLIST_HEAD(clk_root_list);
> +
> +/***        clk API        ***/
> +
> +void __clk_unprepare(struct clk *clk)
> +{
> +	if (!clk)
> +		return;
> +
> +	if (WARN_ON(clk->prepare_count == 0))
> +		return;
> +
> +	if (--clk->prepare_count > 0)
> +		return;
> +
> +	WARN_ON(clk->enable_count > 0);
> +
> +	if (clk->ops->unprepare)
> +		clk->ops->unprepare(clk);
> +
> +	__clk_unprepare(clk->parent);
> +}


I think you can rewrite this to get rid of the recursion as below:

	while (clk) {
		if (WARN_ON(clk->prepare_count == 0))
			return;

		if (--clk->prepare_count > 0)
			return;

		WARN_ON(clk->enable_count > 0)

		if (clk->ops->unprepare)
			clk->ops->unprepare(clk);

		clk = clk->parent;
	}

Also, should this function be static?

> +
> +/**
> + * clk_unprepare - perform the slow part of a clk gate
> + * @clk: the clk being gated
> + *
> + * clk_unprepare may sleep, which differentiates it from clk_disable.  In a
> + * simple case, clk_unprepare can be used instead of clk_disable to gate a clk
> + * if the operation may sleep.  One example is a clk which is accessed over
> + * I2c.  In the complex case a clk gate operation may require a fast and a slow
> + * part.  It is this reason that clk_unprepare and clk_disable are not mutually
> + * exclusive.  In fact clk_disable must be called before clk_unprepare.
> + */
> +void clk_unprepare(struct clk *clk)
> +{
> +	mutex_lock(&prepare_lock);
> +	__clk_unprepare(clk);
> +	mutex_unlock(&prepare_lock);
> +}
> +EXPORT_SYMBOL_GPL(clk_unprepare);
> +
> +int __clk_prepare(struct clk *clk)
> +{
> +	int ret = 0;
> +
> +	if (!clk)
> +		return 0;
> +
> +	if (clk->prepare_count == 0) {
> +		ret = __clk_prepare(clk->parent);
> +		if (ret)
> +			return ret;
> +
> +		if (clk->ops->prepare) {
> +			ret = clk->ops->prepare(clk);
> +			if (ret) {
> +				__clk_unprepare(clk->parent);
> +				return ret;
> +			}
> +		}
> +	}
> +
> +	clk->prepare_count++;
> +
> +	return 0;
> +}


This is unfortunately a bit tricky to remove the recursion from without
keeping a local stack of the clocks leading up to first unprepared
client :-/.

Again, should this be static? What outside this file needs to
prepare/unprepare clocks without the lock held?

> +

> +/**
> + * clk_prepare - perform the slow part of a clk ungate
> + * @clk: the clk being ungated
> + *
> + * clk_prepare may sleep, which differentiates it from clk_enable.  In a simple
> + * case, clk_prepare can be used instead of clk_enable to ungate a clk if the
> + * operation may sleep.  One example is a clk which is accessed over I2c.  In
> + * the complex case a clk ungate operation may require a fast and a slow part.
> + * It is this reason that clk_prepare and clk_enable are not mutually
> + * exclusive.  In fact clk_prepare must be called before clk_enable.
> + * Returns 0 on success, -EERROR otherwise.
> + */
> +int clk_prepare(struct clk *clk)
> +{
> +	int ret;
> +
> +	mutex_lock(&prepare_lock);
> +	ret = __clk_prepare(clk);
> +	mutex_unlock(&prepare_lock);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(clk_prepare);
> +
> +void __clk_disable(struct clk *clk)
> +{
> +	if (!clk)
> +		return;
> +
> +	if (WARN_ON(clk->enable_count == 0))
> +		return;
> +
> +	if (--clk->enable_count > 0)
> +		return;
> +
> +	if (clk->ops->disable)
> +		clk->ops->disable(clk);
> +
> +	if (clk->parent)
> +		__clk_disable(clk->parent);


Easy to get rid of the recursion here. Also should be static?

> +}
> +
> +/**
> + * clk_disable - perform the fast part of a clk gate
> + * @clk: the clk being gated
> + *
> + * clk_disable must not sleep, which differentiates it from clk_unprepare.  In
> + * a simple case, clk_disable can be used instead of clk_unprepare to gate a
> + * clk if the operation is fast and will never sleep.  One example is a
> + * SoC-internal clk which is controlled via simple register writes.  In the
> + * complex case a clk gate operation may require a fast and a slow part.  It is
> + * this reason that clk_unprepare and clk_disable are not mutually exclusive.
> + * In fact clk_disable must be called before clk_unprepare.
> + */
> +void clk_disable(struct clk *clk)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&enable_lock, flags);
> +	__clk_disable(clk);
> +	spin_unlock_irqrestore(&enable_lock, flags);
> +}
> +EXPORT_SYMBOL_GPL(clk_disable);
> +
> +int __clk_enable(struct clk *clk)
> +{
> +	int ret = 0;
> +
> +	if (!clk)
> +		return 0;
> +
> +	if (WARN_ON(clk->prepare_count == 0))
> +		return -ESHUTDOWN;
> +
> +	if (clk->enable_count == 0) {
> +		if (clk->parent)
> +			ret = __clk_enable(clk->parent);
> +
> +		if (ret)
> +			return ret;
> +
> +		if (clk->ops->enable) {
> +			ret = clk->ops->enable(clk);
> +			if (ret) {
> +				__clk_disable(clk->parent);
> +				return ret;
> +			}
> +		}
> +	}
> +
> +	clk->enable_count++;
> +	return 0;
> +}
> +
> +/**
> + * clk_enable - perform the fast part of a clk ungate
> + * @clk: the clk being ungated
> + *
> + * clk_enable must not sleep, which differentiates it from clk_prepare.  In a
> + * simple case, clk_enable can be used instead of clk_prepare to ungate a clk
> + * if the operation will never sleep.  One example is a SoC-internal clk which
> + * is controlled via simple register writes.  In the complex case a clk ungate
> + * operation may require a fast and a slow part.  It is this reason that
> + * clk_enable and clk_prepare are not mutually exclusive.  In fact clk_prepare
> + * must be called before clk_enable.  Returns 0 on success, -EERROR
> + * otherwise.
> + */
> +int clk_enable(struct clk *clk)
> +{
> +	unsigned long flags;
> +	int ret;
> +
> +	spin_lock_irqsave(&enable_lock, flags);
> +	ret = __clk_enable(clk);
> +	spin_unlock_irqrestore(&enable_lock, flags);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(clk_enable);
> +
> +/**
> + * clk_get_rate - return the rate of clk
> + * @clk: the clk whose rate is being returned
> + *
> + * Simply returns the cached rate of the clk.  Does not query the hardware.  If
> + * clk is NULL then returns 0.
> + */
> +unsigned long clk_get_rate(struct clk *clk)
> +{
> +	unsigned long rate;
> +
> +	if (!clk)
> +		return 0;
> +
> +	mutex_lock(&prepare_lock);
> +	rate = clk->rate;
> +	mutex_unlock(&prepare_lock);
> +
> +	return rate;
> +}
> +EXPORT_SYMBOL_GPL(clk_get_rate);
> +
> +/**
> + * clk_round_rate - round the given rate for a clk
> + * @clk: the clk for which we are rounding a rate
> + * @rate: the rate which is to be rounded
> + *
> + * Takes in a rate as input and rounds it to a rate that the clk can actually
> + * use which is then returned.  If clk doesn't support round_rate operation
> + * then the rate passed in is returned.
> + */
> +long clk_round_rate(struct clk *clk, unsigned long rate)
> +{
> +	if (clk && clk->ops->round_rate)
> +		return clk->ops->round_rate(clk, rate, NULL);
> +
> +	return rate;
> +}
> +EXPORT_SYMBOL_GPL(clk_round_rate);


If the clock doesn't provide a round rate function then shouldn't we
return an error to the caller? Telling the caller that the rate is
perfect might lead to odd driver bugs?

> +
> +/**
> + * __clk_recalc_rates
> + * @clk: first clk in the subtree
> + *
> + * Walks the subtree of clks starting with clk and recalculates rates as it
> + * goes.  Note that if a clk does not implement the recalc_rate operation then
> + * propagation of that subtree stops and all of that clks children will not
> + * have their rates updated.  Caller must hold prepare_lock.
> + */
> +static void __clk_recalc_rates(struct clk *clk)
> +{
> +	struct hlist_node *tmp;
> +	struct clk *child;
> +
> +	if (!clk->ops->recalc_rate) {
> +		pr_debug("%s: no .recalc_rate for clk %s\n",
> +				__func__, clk->name);
> +		return;
> +	}
> +
> +	clk->rate = clk->ops->recalc_rate(clk);
> +
> +	/* FIXME add post-rate change notification here */
> +
> +	hlist_for_each_entry(child, tmp, &clk->children, child_node)
> +		__clk_recalc_rates(child);
> +}
> +
> +/**
> + * DOC: Using the CLK_PARENT_SET_RATE flag
> + *
> + * __clk_set_rate changes the child's rate before the parent's to more
> + * easily handle failure conditions.
> + *
> + * This means clk might run out of spec for a short time if its rate is
> + * increased before the parent's rate is updated.
> + *
> + * To prevent this consider setting the CLK_GATE_SET_RATE flag on any
> + * clk where you also set the CLK_PARENT_SET_RATE flag
> + */


Is this standard kerneldoc format?

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


static?

> +{
> +	struct clk *fail_clk = NULL;
> +	int ret = 0;
> +	unsigned long old_rate = clk->rate;
> +	unsigned long new_rate;
> +	unsigned long parent_old_rate;
> +	unsigned long parent_new_rate = 0;
> +	struct clk *child;
> +	struct hlist_node *tmp;
> +
> +	/* bail early if we can't change rate while clk is enabled */
> +	if ((clk->flags & CLK_GATE_SET_RATE) && clk->enable_count)
> +		return clk;
> +
> +	/* find the new rate and see if parent rate should change too */
> +	WARN_ON(!clk->ops->round_rate);
> +
> +	new_rate = clk->ops->round_rate(clk, rate, &parent_new_rate);
> +
> +	/* FIXME propagate pre-rate change notification here */
> +	/* XXX note that pre-rate change notifications will stack */
> +
> +	/* change the rate of this clk */
> +	if (clk->ops->set_rate)
> +		ret = clk->ops->set_rate(clk, new_rate);
> +
> +	if (ret)
> +		return clk;


Is there are reason to write it this way and not:

	if (clk->ops->set_rate) {
		ret = clk->ops->set_rate(clk, new_rate);
		if (ret)
			return clk;
	}

If !clk->ops->set_rate then ret is always zero right? Note, making this
change means that you don't need to init ret to zero at the top of this
function.

> +
> +	/*
> +	 * change the rate of the parent clk if necessary
> +	 *
> +	 * hitting the nested 'if' path implies we have hit a .set_rate
> +	 * failure somewhere upstream while propagating __clk_set_rate
> +	 * up the clk tree.  roll back the clk rates one by one and
> +	 * return the pointer to the clk that failed.  clk_set_rate will
> +	 * use the pointer to propagate a rate-change abort notifier
> +	 * from the "highest" point.
> +	 */
> +	if ((clk->flags & CLK_PARENT_SET_RATE) && parent_new_rate) {
> +		parent_old_rate = clk->parent->rate;
> +		fail_clk = __clk_set_rate(clk->parent, parent_new_rate);
> +
> +		/* roll back changes if parent rate change failed */
> +		if (fail_clk) {
> +			pr_warn("%s: failed to set parent %s rate to %lu\n",
> +					__func__, fail_clk->name,
> +					parent_new_rate);
> +			clk->ops->set_rate(clk, old_rate);
> +		}
> +		return fail_clk;
> +	}
> +
> +	/*
> +	 * set clk's rate & recalculate the rates of clk's children
> +	 *
> +	 * hitting this path implies we have successfully finished
> +	 * propagating recursive calls to __clk_set_rate up the clk tree
> +	 * (if necessary) and it is safe to propagate __clk_recalc_rates
> +	 * and post-rate change notifiers down the clk tree from this
> +	 * point.
> +	 */
> +	/* FIXME propagate post-rate change notifier starting with this clk */
> +	__clk_recalc_rates(clk);
> +
> +	return NULL;
> +}
> +
> +/**
> + * clk_set_rate - specify a new rate for clk
> + * @clk: the clk whose rate is being changed
> + * @rate: the new rate for clk
> + *
> + * In the simplest case clk_set_rate will only change the rate of clk.
> + *
> + * If clk has the CLK_GATE_SET_RATE flag set and it is enabled this call
> + * will fail; only when the clk is disabled will it be able to change
> + * its rate.
> + *
> + * Setting the CLK_PARENT_SET_RATE flag allows clk_set_rate to
> + * recursively propagate up to clk's parent; whether or not this happens
> + * depends on the outcome of clk's .round_rate implementation.  If
> + * *parent_rate is 0 after calling .round_rate then upstream parent
> + * propagation is ignored.  If *parent_rate comes back with a new rate
> + * for clk's parent then we propagate up to clk's parent and set it's
> + * rate.  Upward propagation will continue until either a clk does not
> + * support the CLK_PARENT_SET_RATE flag or .round_rate stops requesting
> + * changes to clk's parent_rate.  If there is a failure during upstream
> + * propagation then clk_set_rate will unwind and restore each clk's rate
> + * that had been successfully changed.  Afterwards a rate change abort
> + * notification will be propagated downstream, starting from the clk
> + * that failed.
> + *
> + * At the end of all of the rate setting, clk_set_rate internally calls
> + * __clk_recalc_rates and propagates the rate changes downstream,
> + * starting from the highest clk whose rate was changed.  This has the
> + * added benefit of propagating post-rate change notifiers.
> + *
> + * Note that while post-rate change and rate change abort notifications
> + * are guaranteed to be sent to a clk only once per call to
> + * clk_set_rate, pre-change notifications will be sent for every clk
> + * whose rate is changed.  Stacking pre-change notifications is noisy
> + * for the drivers subscribed to them, but this allows drivers to react
> + * to intermediate clk rate changes up until the point where the final
> + * rate is achieved at the end of upstream propagation.
> + *
> + * Returns 0 on success, -EERROR otherwise.
> + */
> +int clk_set_rate(struct clk *clk, unsigned long rate)
> +{
> +	struct clk *fail_clk;
> +	int ret = 0;
> +
> +	/* prevent racing with updates to the clock topology */
> +	mutex_lock(&prepare_lock);
> +
> +	/* bail early if nothing to do */
> +	if (rate == clk->rate)
> +		goto out;

> +
> +	fail_clk = __clk_set_rate(clk, rate);
> +	if (fail_clk) {
> +		pr_warn("%s: failed to set %s rate\n", __func__,
> +				fail_clk->name);
> +		/* FIXME propagate rate change abort notification here */
> +		ret = -EIO;


Why does __clk_set_rate return a struct clk if you don't do anything
with it? You can move the pr_warn into __clk_set_rate and have it return
a proper errno value instead so that you get a reason why it failed to
set the rate.

> +	}
> +out:
> +	mutex_unlock(&prepare_lock);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(clk_set_rate);
> +
> +/**
> + * clk_get_parent - return the parent of a clk
> + * @clk: the clk whose parent gets returned
> + *
> + * Simply returns clk->parent.  It is up to the caller to hold the
> + * prepare_lock, if desired.  Returns NULL if clk is NULL.
> + */
> +struct clk *clk_get_parent(struct clk *clk)
> +{
> +	struct clk *parent;
> +
> +	if (!clk)
> +		return NULL;
> +
> +	mutex_lock(&prepare_lock);
> +	parent = clk->parent;
> +	mutex_unlock(&prepare_lock);
> +
> +	return parent;
> +}
> +EXPORT_SYMBOL_GPL(clk_get_parent);
> +
> +void __clk_reparent(struct clk *clk, struct clk *new_parent)
> +{

> +	if (!clk || !new_parent)
> +		return;


clk_reparent already checks for !clk, shouldn't it also check for
!new_parent and remove the check from here?

> +
> +	hlist_del(&clk->child_node);
> +	hlist_add_head(&clk->child_node, &new_parent->children);
> +
> +	clk->parent = new_parent;
> +
> +	/* FIXME update sysfs clock topology */
> +}
> +
> +/**
> + * clk_set_parent - switch the parent of a mux clk
> + * @clk: the mux clk whose input we are switching
> + * @parent: the new input to clk
> + *
> + * Re-parent clk to use parent as it's new input source.  If clk has the
> + * CLK_GATE_SET_PARENT flag set then clk must be gated for this
> + * operation to succeed.  After successfully changing clk's parent
> + * clk_set_parent will update the clk topology, sysfs topology and
> + * propagate rate recalculation via __clk_recalc_rates.  Returns 0 on
> + * success, -EERROR otherwise.
> + */
> +int clk_set_parent(struct clk *clk, struct clk *parent)
> +{
> +	int ret = 0;
> +
> +	if (!clk || !clk->ops)
> +		return -EINVAL;
> +
> +	if (!clk->ops->set_parent)
> +		return -ENOSYS;
> +
> +	/* prevent racing with updates to the clock topology */
> +	mutex_lock(&prepare_lock);
> +
> +	if (clk->parent == parent)
> +		goto out;
> +
> +	/* FIXME add pre-rate change notification here */
> +
> +	/* only re-parent if the clock is not in use */
> +	if ((clk->flags & CLK_GATE_SET_PARENT) && clk->prepare_count)
> +		ret = -EBUSY;
> +	else
> +		ret = clk->ops->set_parent(clk, parent);
> +
> +	if (ret < 0)
> +		/* FIXME add rate change abort notification here */
> +		goto out;
> +
> +	/* update tree topology */
> +	__clk_reparent(clk, parent);
> +
> +	/*
> +	 * If successful recalculate the rates of the clock, including
> +	 * children.
> +	 */
> +	__clk_recalc_rates(clk);
> +
> +out:
> +	mutex_unlock(&prepare_lock);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(clk_set_parent);
> +
> +/**
> + * clk_init - initialize the data structures in a struct clk
> + * @dev: device initializing this clk, placeholder for now
> + * @clk: clk being initialized
> + *
> + * Initializes the lists in struct clk, queries the hardware for the
> + * parent and rate and sets them both.  Adds the clk to the sysfs tree
> + * topology.
> + *
> + * Caller must populate clk->name and clk->flags before calling
> + * clk_init.  A key assumption in the call to .get_parent is that clks
> + * are being initialized in-order.
> + */
> +void clk_init(struct device *dev, struct clk *clk)
> +{
> +	if (!clk)
> +		return;
> +
> +	INIT_HLIST_HEAD(&clk->children);
> +	INIT_HLIST_NODE(&clk->child_node);
> +
> +	mutex_lock(&prepare_lock);
> +
> +	/*
> +	 * .get_parent is mandatory for populating .parent for clks with
> +	 * multiple possible parents.  For clks with a fixed parent
> +	 * .get_parent is not mandatory and .parent can be statically
> +	 * initialized
> +	 */
> +	if (clk->ops->get_parent)

> +		clk->parent = clk->ops->get_parent(clk);
> +
> +	/* clks without a parent are considered root clks */
> +	if (clk->parent)
> +		hlist_add_head(&clk->child_node,
> +				&clk->parent->children);
> +	else
> +		hlist_add_head(&clk->child_node, &clk_root_list);
> +
> +	if (clk->ops->recalc_rate)
> +		clk->rate = clk->ops->recalc_rate(clk);
> +	else
> +		clk->rate = 0;
> +
> +	mutex_unlock(&prepare_lock);
> +
> +	return;
> +}
> +EXPORT_SYMBOL_GPL(clk_init);
> diff --git a/include/linux/clk.h b/include/linux/clk.h
> index 7213b52..9cb8d72 100644
> --- a/include/linux/clk.h
> +++ b/include/linux/clk.h
> @@ -3,6 +3,8 @@
>   *
>   *  Copyright (C) 2004 ARM Limited.
>   *  Written by Deep Blue Solutions Limited.
> + *  Copyright (c) 2010-2011 Jeremy Kerr <jeremy.kerr@canonical.com>
> + *  Copyright (C) 2011 Linaro Ltd <mturquette@linaro.org>
>   *
>   * 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
> @@ -12,18 +14,137 @@
>  #define __LINUX_CLK_H
>  
>  #include <linux/kernel.h>
> +#include <linux/errno.h>
>  
>  struct device;
>  
> +struct clk;
> +
> +#ifdef CONFIG_GENERIC_CLK
> +
>  /*
> - * The base API.
> + * flags used across common struct clk.  these flags should only affect the
> + * top-level framework.  custom flags for dealing with hardware specifics
> + * belong in struct clk_hw_your_unique_device
>   */
> +#define CLK_GATE_SET_RATE	BIT(0) /* must be gated across rate change */
> +#define CLK_GATE_SET_PARENT	BIT(1) /* must be gated across re-parent */
> +#define CLK_PARENT_SET_RATE	BIT(2) /* propagate rate change up one level */
> +#define CLK_IGNORE_UNUSED	BIT(3) /* do not gate even if unused */
>  
> +#define CLK_GATE_RATE_CHANGE	(CLK_GATE_SET_RATE | CLK_GATE_SET_PARENT)
>  
> -/*
> - * struct clk - an machine class defined object / cookie.
> +struct clk {
> +	const char		*name;
> +	const struct clk_hw_ops	*ops;
> +	struct clk		*parent;
> +	unsigned long		rate;
> +	unsigned long		flags;
> +	unsigned int		enable_count;
> +	unsigned int		prepare_count;
> +	struct hlist_head	children;
> +	struct hlist_node	child_node;
> +};
> +
> +/**
> + * struct clk_hw_ops -  Callback operations for hardware clocks; these are to
> + * be provided by the clock implementation, and will be called by drivers
> + * through the clk_* API.
> + *
> + * @prepare:	Prepare the clock for enabling. This must not return until
> + * 		the clock is fully prepared, and it's safe to call clk_enable.
> + * 		This callback is intended to allow clock implementations to
> + * 		do any initialisation that may sleep. Called with
> + * 		prepare_lock held.
> + *
> + * @unprepare:	Release the clock from its prepared state. This will typically
> + * 		undo any work done in the @prepare callback. Called with
> + * 		prepare_lock held.
> + *
> + * @enable:	Enable the clock atomically. This must not return until the
> + * 		clock is generating a valid clock signal, usable by consumer
> + * 		devices. Called with enable_lock held. This function must not
> + * 		sleep.
> + *
> + * @disable:	Disable the clock atomically. Called with enable_lock held.
> + * 		This function must not sleep.
> + *
> + * @recalc_rate	Recalculate the rate of this clock, by quering hardware
> + * 		and/or the clock's parent. It is up to the caller to insure
> + * 		that the prepare_mutex is held across this call.  Returns the
> + * 		calculated rate.  Optional, but recommended - if this op is not
> + * 		set then clock rate will be initialized to 0.
> + *
> + * @round_rate	XXX FIXME
> + *
> + * @get_parent	Return the parent of this clock; for clocks with multiple
> + * 		possible parents, query the hardware for the current parent.
> + * 		Clocks with fixed parents should still implement this and
> + * 		return something like struct clk *fixed_parent from their
> + * 		respective struct clk_hw_* data.  Currently called only when
> + * 		the clock is initialized.  Clocks that do not implement this
> + * 		will have their parent set to NULL.
> + *
> + * @set_parent	Change the input source of this clock; for clocks with multiple
> + * 		possible parents specify a new parent by passing in a struct
> + * 		clk *parent.  Returns 0 on success, -EERROR otherwise.
> + *
> + * @set_rate	Change the rate of this clock. If this callback returns
> + * 		CLK_PARENT_SET_RATE, the rate change will be propagated to the
> + * 		parent clock (which may propagate again if the parent clock
> + * 		also sets this flag). The requested rate of the parent is
> + * 		passed back from the callback in the second 'unsigned long *'
> + * 		argument.  Note that it is up to the hardware clock's set_rate
> + * 		implementation to insure that clocks do not run out of spec
> + * 		when propgating the call to set_rate up to the parent.  One way
> + * 		to do this is to gate the clock (via clk_disable and/or
> + * 		clk_unprepare) before calling clk_set_rate, then ungating it
> + * 		afterward.  If your clock also has the CLK_GATE_SET_RATE flag
> + * 		set then this will insure safety.  Returns 0 on success,
> + * 		-EERROR otherwise.
> + *
> + * The clk_enable/clk_disable and clk_prepare/clk_unprepare pairs allow
> + * implementations to split any work between atomic (enable) and sleepable
> + * (prepare) contexts.  If enabling a clock requires code that might sleep,
> + * this must be done in clk_prepare.  Clock enable code that will never be
> + * called in a sleepable context may be implement in clk_enable.
> + *
> + * Typically, drivers will call clk_prepare when a clock may be needed later
> + * (eg. when a device is opened), and clk_enable when the clock is actually
> + * required (eg. from an interrupt). Note that clk_prepare MUST have been
> + * called before clk_enable.
>   */
> -struct clk;
> +struct clk_hw_ops {
> +	int		(*prepare)(struct clk *clk);
> +	void		(*unprepare)(struct clk *clk);
> +	int		(*enable)(struct clk *clk);
> +	void		(*disable)(struct clk *clk);
> +	unsigned long	(*recalc_rate)(struct clk *clk);
> +	long		(*round_rate)(struct clk *clk, unsigned long,
> +					unsigned long *);
> +	int		(*set_parent)(struct clk *clk, struct clk *);
> +	struct clk *	(*get_parent)(struct clk *clk);
> +	int		(*set_rate)(struct clk *clk, unsigned long);
> +};
> +
> +/**
> + * clk_init - initialize the data structures in a struct clk
> + * @dev: device initializing this clk, placeholder for now
> + * @clk: clk being initialized
> + *
> + * Initializes the lists in struct clk, queries the hardware for the
> + * parent and rate and sets them both.  Adds the clk to the sysfs tree
> + * topology.
> + *
> + * Caller must populate .name, .flags and .ops before calling clk_init.
> + */
> +void clk_init(struct device *dev, struct clk *clk);
> +
> +int __clk_prepare(struct clk *clk);
> +void __clk_unprepare(struct clk *clk);
> +void __clk_reparent(struct clk *clk, struct clk *new_parent);
> +
> +#endif /* !CONFIG_GENERIC_CLK */
>  
>  /**
>   * clk_get - lookup and obtain a reference to a clock producer.
> @@ -110,6 +231,7 @@ static inline void clk_unprepare(struct clk *clk)
>  /**
>   * clk_get_rate - obtain the current clock rate (in Hz) for a clock source.
>   *		  This is only valid once the clock source has been enabled.
> + *		  Returns zero if the clock rate is unknown.
>   * @clk: clock source
>   */
>  unsigned long clk_get_rate(struct clk *clk);

Re: [PATCH v4 5/6] clk: basic gateable and fixed-rate clks

[Ryan Mallon]

On 14/12/11 14:53, Mike Turquette wrote:

> Many platforms support simple gateable clks and fixed-rate clks that
> should not be re-implemented by every platform.
> 
> This patch introduces a gateable clk with a common programming model of
> gate control via a write of 1 bit to a register.  Both set-to-enable and
> clear-to-enable are supported.
> 
> Also introduced is a fixed-rate clk which has no reprogrammable aspects.
> 
> The purpose of both types of clks is documented in drivers/clk/basic.c.
> 
> TODO: add support for a simple divider, simple mux and a dummy clk for
> stubbing out platform support.
> 
> Based on original patch by Jeremy Kerr and contribution by Jamie Iles.
> 
> Signed-off-by: Mike Turquette <mturquette@linaro.org>
> Cc: Jeremy Kerr <jeremy.kerr@canonical.com>
> Cc: Jamie Iles <jamie@jamieiles.com>

<snip>

> +int clk_register_gate(struct device *dev, const char *name, unsigned long flags,
> +		struct clk *fixed_parent, void __iomem *reg, u8 bit_idx,
> +		int set_to_enable)
> +{
> +	struct clk_hw_gate *gclk;
> +	struct clk *clk;
> +
> +	gclk = kmalloc(sizeof(struct clk_hw_gate), GFP_KERNEL);
> +
> +	if (!gclk) {
> +		pr_err("%s: could not allocate gated clk\n", __func__);
> +		return -ENOMEM;
> +	}
> +
> +	clk = &gclk->clk;
> +
> +	/* struct clk_hw_gate assignments */
> +	gclk->fixed_parent = fixed_parent;
> +	gclk->reg = reg;
> +	gclk->bit_idx = bit_idx;
> +
> +	/* struct clk assignments */
> +	clk->name = name;
> +	clk->flags = flags;
> +
> +	if (set_to_enable)
> +		clk->ops = &clk_hw_gate_set_enable_ops;
> +	else
> +		clk->ops = &clk_hw_gate_set_disable_ops;


You could avoid having two sets of operations if you stored the
set_to_enable value in struct clk_hw_gate. It might be useful to store
additional information in struct clk_hw_gate if you also want to extend
to supporting non-32bit registers (readb, etc), clocks with write only
registers, or support clocks which require more than one bit to be set
or cleared to enable them, etc. See the basic mmio gpio driver for a
similar case.

~Ryan

Re: [PATCH v4 3/6] clk: introduce the common clock framework

[Richard Zhao]

Hi Mike,
> + *
> + * @recalc_rate	Recalculate the rate of this clock, by quering hardware
> + * 		and/or the clock's parent. It is up to the caller to insure
> + * 		that the prepare_mutex is held across this call.  Returns the
> + * 		calculated rate.  Optional, but recommended - if this op is not
> + * 		set then clock rate will be initialized to 0.
I'm still concerned, if it's NULL, why don't just retrun parent's rate if it has? 
 
Thanks
Richard

Re: [PATCH v4 3/6] clk: introduce the common clock framework

[Thomas Gleixner]

On Tue, 13 Dec 2011, Mike Turquette wrote:
> +void __clk_unprepare(struct clk *clk)
> +{
> +	if (!clk)
> +		return;
> +
> +	if (WARN_ON(clk->prepare_count == 0))
> +		return;
> +
> +	if (--clk->prepare_count > 0)
> +		return;
> +
> +	WARN_ON(clk->enable_count > 0);

So this leaves the clock enable count set. I'm a bit wary about
that. Shouldn't it either return (including bumping the prepare_count
again) or call clk_disable() ?

> +/**
> + * clk_get_parent - return the parent of a clk
> + * @clk: the clk whose parent gets returned
> + *
> + * Simply returns clk->parent.  It is up to the caller to hold the
> + * prepare_lock, if desired.  Returns NULL if clk is NULL.

Holding the prepare lock in the caller will deadlock. So it's not a
real good idea to document it as an option :)

> + */
> +struct clk *clk_get_parent(struct clk *clk)
> +{
> +	struct clk *parent;
> +
> +	if (!clk)
> +		return NULL;
> +
> +	mutex_lock(&prepare_lock);

> +/**
> + * clk_init - initialize the data structures in a struct clk
> + * @dev: device initializing this clk, placeholder for now
> + * @clk: clk being initialized
> + *
> + * Initializes the lists in struct clk, queries the hardware for the
> + * parent and rate and sets them both.  Adds the clk to the sysfs tree
> + * topology.
> + *
> + * Caller must populate clk->name and clk->flags before calling

I'm not too happy about this construct. That leaves struct clk and its
members exposed to the world instead of making it a real opaque
cookie. I know from my own painful experience, that this will lead to
random fiddling in that data structure in drivers and arch code just
because the core code has a shortcoming.

Why can't we make struct clk a real cookie and confine the data
structure to the core code ?

That would change the init call to something like:

struct clk *clk_init(struct device *dev, const struct clk_hw *hw,
                     struct clk *parent)

And have:
struct clk_hw {
       struct clk_hw_ops *ops;
       const char 	 *name;
       unsigned long	 flags;
};       

Implementers can do:
struct my_clk_hw {
       struct clk_hw	hw;
       mydata;
};

And then change the clk ops callbacks to take struct clk_hw * as an
argument.

So the core code can allocate the clk data structure and you return a
real opaque cookie. You do the same thing for the basic gated clock in
one of the next patches, so doing it for struct clk itself is just
consequent.

> + * clk_init.  A key assumption in the call to .get_parent is that clks
> + * are being initialized in-order.

We should not require that, see below.

> + */

> +void clk_init(struct device *dev, struct clk *clk)
> +{
> +	if (!clk)
> +		return;
> +
> +	INIT_HLIST_HEAD(&clk->children);
> +	INIT_HLIST_NODE(&clk->child_node);
> +
> +	mutex_lock(&prepare_lock);
> +
> +	/*
> +	 * .get_parent is mandatory for populating .parent for clks with
> +	 * multiple possible parents.  For clks with a fixed parent
> +	 * .get_parent is not mandatory and .parent can be statically
> +	 * initialized
> +	 */
> +	if (clk->ops->get_parent)
> +		clk->parent = clk->ops->get_parent(clk);
> +
> +	/* clks without a parent are considered root clks */

I'd rather prefer that root clocks are explicitely marked with a flag
instead of relying on clk->parent being NULL.

> +	if (clk->parent)
> +		hlist_add_head(&clk->child_node,
> +				&clk->parent->children);
> +	else
> +		hlist_add_head(&clk->child_node, &clk_root_list);

You could put clocks which have no parent and are not marked as root
clock onto a separate list clk_orphaned or such. You might need this
for handling clocks which are disconnected from any parent runtime as
well.

And to avoid the whole in-order initialization issue, you could change
clk_init to:

struct clk *clk_init(struct device *dev, const struct clk_hw *hw,
                     unsigned long flags, const char *parent_name)

Then you can lookup the requested parent by name. If that fails, you
put the clock into the orphaned list. When a new clock is initialized,
then you walk the orphaned list and check whether something is waiting
for that clock.

To handle multi-parent clocks you need to go one step further and add:

struct clk_hw {
       struct clk_hw_ops *ops;
       const char 	 *name;
       const char	 **possible_parents;
};

You also require a get_parent_idx() function in clk_hw_ops. So when
clk_init() is called with parent_name = NULL and get_parent_idx() is
implemented, then you call it and the clock returns you the index of
the possible_parents array. If that parent clock is not yet
registered, you store the requested name and do the lookup when new
clocks are registered.

That has also the advantage, that you can validate parent settings
upfront and for setting the parent during runtime, you don't have to
acquire a pointer to the parent clock. It's enough to call
clk_set_named_parent().

Thoughts ?

	 tglx

Re: [PATCH v4 3/6] clk: introduce the common clock framework

[Turquette, Mike]

On Tue, Dec 13, 2011 at 8:52 PM, Ryan Mallon <rmallon@gmail.com> wrote:
> On 14/12/11 14:53, Mike Turquette wrote:
>> +void __clk_unprepare(struct clk *clk)
>> +{
>> +     if (!clk)
>> +             return;
>> +
>> +     if (WARN_ON(clk->prepare_count == 0))
>> +             return;
>> +
>> +     if (--clk->prepare_count > 0)
>> +             return;
>> +
>> +     WARN_ON(clk->enable_count > 0);
>> +
>> +     if (clk->ops->unprepare)
>> +             clk->ops->unprepare(clk);
>> +
>> +     __clk_unprepare(clk->parent);
>> +}
>
>
> I think you can rewrite this to get rid of the recursion as below:
>
>        while (clk) {
>                if (WARN_ON(clk->prepare_count == 0))
>                        return;
>
>                if (--clk->prepare_count > 0)
>                        return;
>
>                WARN_ON(clk->enable_count > 0)
>
>                if (clk->ops->unprepare)
>                        clk->ops->unprepare(clk);
>
>                clk = clk->parent;
>        }

Looks good.  I'll roll into next set.

> Also, should this function be static?

No, since platform clk code will occasionally be forced to call
clk_(un)prepare while the prepare_lock mutex is held.  For a valid
example see:
http://git.linaro.org/gitweb?p=people/mturquette/linux.git;a=blob;f=arch/arm/mach-omap2/dpll3xxx.c;h=b2412574b63829944593c1a7a6eda5fa4350686f;hb=738bde65918ae1ac743b1498801da0b860e2ee32#l461

>> +void clk_unprepare(struct clk *clk)
>> +{
>> +     mutex_lock(&prepare_lock);
>> +     __clk_unprepare(clk);
>> +     mutex_unlock(&prepare_lock);
>> +}
>> +EXPORT_SYMBOL_GPL(clk_unprepare);
>> +
>> +int __clk_prepare(struct clk *clk)
>> +{
>> +     int ret = 0;
>> +
>> +     if (!clk)
>> +             return 0;
>> +
>> +     if (clk->prepare_count == 0) {
>> +             ret = __clk_prepare(clk->parent);
>> +             if (ret)
>> +                     return ret;
>> +
>> +             if (clk->ops->prepare) {
>> +                     ret = clk->ops->prepare(clk);
>> +                     if (ret) {
>> +                             __clk_unprepare(clk->parent);
>> +                             return ret;
>> +                     }
>> +             }
>> +     }
>> +
>> +     clk->prepare_count++;
>> +
>> +     return 0;
>> +}
>
>
> This is unfortunately a bit tricky to remove the recursion from without
> keeping a local stack of the clocks leading up to first unprepared
> client :-/.
>
> Again, should this be static? What outside this file needs to
> prepare/unprepare clocks without the lock held?

Same as above.

>> +int clk_prepare(struct clk *clk)
>> +{
>> +     int ret;
>> +
>> +     mutex_lock(&prepare_lock);
>> +     ret = __clk_prepare(clk);
>> +     mutex_unlock(&prepare_lock);
>> +
>> +     return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(clk_prepare);
>> +
>> +void __clk_disable(struct clk *clk)
>> +{
>> +     if (!clk)
>> +             return;
>> +
>> +     if (WARN_ON(clk->enable_count == 0))
>> +             return;
>> +
>> +     if (--clk->enable_count > 0)
>> +             return;
>> +
>> +     if (clk->ops->disable)
>> +             clk->ops->disable(clk);
>> +
>> +     if (clk->parent)
>> +             __clk_disable(clk->parent);
>
>
> Easy to get rid of the recursion here. Also should be static?

Yes the enable/disable should be static.  I originally made them
non-static when I converted the prepare/unprepare set, but of course
it's possible to call these with the prepare_lock mutex held so I'll
fix this up in the next set.

>> +long clk_round_rate(struct clk *clk, unsigned long rate)
>> +{
>> +     if (clk && clk->ops->round_rate)
>> +             return clk->ops->round_rate(clk, rate, NULL);
>> +
>> +     return rate;
>> +}
>> +EXPORT_SYMBOL_GPL(clk_round_rate);
>
>
> If the clock doesn't provide a round rate function then shouldn't we
> return an error to the caller? Telling the caller that the rate is
> perfect might lead to odd driver bugs?

Yes this code should so something better.  I've been focused mostly on
the "write" aspects of the clk API (set_rate, set_parent,
enable/disable and prepare/unprepare) and less on the "read" aspects
of the clk API (get_rate, get_parent, round_rate, etc).  I'll give
this a closer look for the next set.

>> +/**
>> + * DOC: Using the CLK_PARENT_SET_RATE flag
>> + *
>> + * __clk_set_rate changes the child's rate before the parent's to more
>> + * easily handle failure conditions.
>> + *
>> + * This means clk might run out of spec for a short time if its rate is
>> + * increased before the parent's rate is updated.
>> + *
>> + * To prevent this consider setting the CLK_GATE_SET_RATE flag on any
>> + * clk where you also set the CLK_PARENT_SET_RATE flag
>> + */
>
>
> Is this standard kerneldoc format?

It is:
http://git.linaro.org/gitweb?p=people/mturquette/linux.git;a=blob;f=Documentation/kernel-doc-nano-HOWTO.txt;h=3d8a97747f7731c801ca7d3a1483858feeb76b6c;hb=refs/heads/v3.2-rc5-clkv4-omap#l298

>> +struct clk *__clk_set_rate(struct clk *clk, unsigned long rate)
>
>
> static?

I'll make it static.  I don't think any platform code needs to call
this (at least I hope not).

>> +{
>> +     struct clk *fail_clk = NULL;
>> +     int ret = 0;
>> +     unsigned long old_rate = clk->rate;
>> +     unsigned long new_rate;
>> +     unsigned long parent_old_rate;
>> +     unsigned long parent_new_rate = 0;
>> +     struct clk *child;
>> +     struct hlist_node *tmp;
>> +
>> +     /* bail early if we can't change rate while clk is enabled */
>> +     if ((clk->flags & CLK_GATE_SET_RATE) && clk->enable_count)
>> +             return clk;
>> +
>> +     /* find the new rate and see if parent rate should change too */
>> +     WARN_ON(!clk->ops->round_rate);
>> +
>> +     new_rate = clk->ops->round_rate(clk, rate, &parent_new_rate);
>> +
>> +     /* FIXME propagate pre-rate change notification here */
>> +     /* XXX note that pre-rate change notifications will stack */
>> +
>> +     /* change the rate of this clk */
>> +     if (clk->ops->set_rate)
>> +             ret = clk->ops->set_rate(clk, new_rate);
>> +
>> +     if (ret)
>> +             return clk;
>
>
> Is there are reason to write it this way and not:
>
>        if (clk->ops->set_rate) {
>                ret = clk->ops->set_rate(clk, new_rate);
>                if (ret)
>                        return clk;
>        }
>
> If !clk->ops->set_rate then ret is always zero right? Note, making this
> change means that you don't need to init ret to zero at the top of this
> function.

Looks good.  Will fix in the next set.

>> +int clk_set_rate(struct clk *clk, unsigned long rate)
>> +{
>> +     struct clk *fail_clk;
>> +     int ret = 0;
>> +
>> +     /* prevent racing with updates to the clock topology */
>> +     mutex_lock(&prepare_lock);
>> +
>> +     /* bail early if nothing to do */
>> +     if (rate == clk->rate)
>> +             goto out;
>
>> +
>> +     fail_clk = __clk_set_rate(clk, rate);
>> +     if (fail_clk) {
>> +             pr_warn("%s: failed to set %s rate\n", __func__,
>> +                             fail_clk->name);
>> +             /* FIXME propagate rate change abort notification here */
>> +             ret = -EIO;
>
>
> Why does __clk_set_rate return a struct clk if you don't do anything
> with it? You can move the pr_warn into __clk_set_rate and have it return
> a proper errno value instead so that you get a reason why it failed to
> set the rate.

The next patch in the series uses fail_clk to propagate
ABORT_RATE_CHANGE notifications to any drivers that have subscribed to
them.  The FIXME comment hints at that but doesn't make it clear.  The
idea is that the PRE_RATE_CHANGE notifiers will be noisy and stack up
(potentially), but I only want to propagate the POST_RATE_CHANGE and
ABORT_RATE_CHANGE notifications once for any single call to
clk_set_rate, which is why __clk_set_rate returns a struct clk *.

>> +void __clk_reparent(struct clk *clk, struct clk *new_parent)
>> +{
>
>> +     if (!clk || !new_parent)
>> +             return;
>
>
> clk_reparent already checks for !clk, shouldn't it also check for
> !new_parent and remove the check from here?

I need to take another look at this.  new_parent can be NULL if we
think it is plausible for a parented clk to suddenly become a root clk
(where clk->parent == NULL).

Thanks for reviewing,
Mike

Re: [PATCH v4 3/6] clk: introduce the common clock framework

[Turquette, Mike]

On Wed, Dec 14, 2011 at 5:18 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Tue, 13 Dec 2011, Mike Turquette wrote:
>> +void __clk_unprepare(struct clk *clk)
>> +{
>> +     if (!clk)
>> +             return;
>> +
>> +     if (WARN_ON(clk->prepare_count == 0))
>> +             return;
>> +
>> +     if (--clk->prepare_count > 0)
>> +             return;
>> +
>> +     WARN_ON(clk->enable_count > 0);
>
> So this leaves the clock enable count set. I'm a bit wary about
> that. Shouldn't it either return (including bumping the prepare_count
> again) or call clk_disable() ?

I've hit this in my port of OMAP.  It comes from this simple situation:

driver 1 (adapted for clk_prepare/clk_unprepare):
clk_prepare(clk);
clk_enable(clk);

...

driver2 (not adapted for clk_prepare/clk_unprepare):
clk_enable(clk);

...

driver1:
clk_disable(clk);
clk_unprepare(clk);

In such a case we don't want to bump the prepare_count, since the
offending driver2 won't decrement that count.  Also we don't want to
shut down that clk since driver2 is using it.

Returning after the WARN is maybe OK, but the point of this check is
really to find code which hasn't yet been made prepare-aware, and the
current implementation is noisy enough about it.

>> +/**
>> + * clk_get_parent - return the parent of a clk
>> + * @clk: the clk whose parent gets returned
>> + *
>> + * Simply returns clk->parent.  It is up to the caller to hold the
>> + * prepare_lock, if desired.  Returns NULL if clk is NULL.
>
> Holding the prepare lock in the caller will deadlock. So it's not a
> real good idea to document it as an option :)

Oops.  That comment is left over from before clk_get_parent held the
lock.  Will remove.

>
>> + */
>> +struct clk *clk_get_parent(struct clk *clk)
>> +{
>> +     struct clk *parent;
>> +
>> +     if (!clk)
>> +             return NULL;
>> +
>> +     mutex_lock(&prepare_lock);
>
>> +/**
>> + * clk_init - initialize the data structures in a struct clk
>> + * @dev: device initializing this clk, placeholder for now
>> + * @clk: clk being initialized
>> + *
>> + * Initializes the lists in struct clk, queries the hardware for the
>> + * parent and rate and sets them both.  Adds the clk to the sysfs tree
>> + * topology.
>> + *
>> + * Caller must populate clk->name and clk->flags before calling
>
> I'm not too happy about this construct. That leaves struct clk and its
> members exposed to the world instead of making it a real opaque
> cookie. I know from my own painful experience, that this will lead to
> random fiddling in that data structure in drivers and arch code just
> because the core code has a shortcoming.
>
> Why can't we make struct clk a real cookie and confine the data
> structure to the core code ?
>
> That would change the init call to something like:
>
> struct clk *clk_init(struct device *dev, const struct clk_hw *hw,
>                     struct clk *parent)
>
> And have:
> struct clk_hw {
>       struct clk_hw_ops *ops;
>       const char        *name;
>       unsigned long     flags;
> };
>
> Implementers can do:
> struct my_clk_hw {
>       struct clk_hw    hw;
>       mydata;
> };
>
> And then change the clk ops callbacks to take struct clk_hw * as an
> argument.
>
> So the core code can allocate the clk data structure and you return a
> real opaque cookie. You do the same thing for the basic gated clock in
> one of the next patches, so doing it for struct clk itself is just
> consequent.

The opaque cookie would work if platform code never needs any
information outside of the data a single clk_hw_whatever provides.
Unfortunately hardware doesn't make things that easy on us and the
operations we do for a single clk are affected by the state of other
clks.

Here is an example I've been using a lot lately: on OMAP we have two
clock inputs to our PLLs: a bypass clk and reference clk (actually we
can have more than this).  When changing the PLL rate both clks must
be enabled, regardless of which clk ends up driving the PLL.  To
illustrate, the PLL may be driven by clk_ref at 400MHz, and then we
call clk_set_rate(pll_clk, 196000000); which will also leave it
clocked by clk_ref but at 196MHz.  For this to happen however, the
clk_bypass had to be enabled during the rate change operation.  Here
is the relevant .set_rate implementation:
http://git.linaro.org/gitweb?p=people/mturquette/linux.git;a=blob;f=arch/arm/mach-omap2/dpll3xxx.c;h=b2412574b63829944593c1a7a6eda5fa4350686f;hb=738bde65918ae1ac743b1498801da0b860e2ee32#l461

In order for the OMAP platform code to do this, we have to have two
things: firstly we need the struct clk so that we can call
clk_enable/disable and __clk_prepare/unprepare on the ref and bypass
clks from within .set_rate.  The second thing is that we need
__clk_prepare and __clk_unprepare to be visible by this code so that
we don't nest the prepare_lock mutex.

Is there a good way to solve such problems if we hide struct clk from
the platform code/clk driver implementation?

Also note that if top-level clk APIs are going to be holding
prepare_lock (clk_get_rate, clk_get_parent, etc) then we can't call
these APIs to get the info we want from within the platform code.  For
example, today most .recalc_rate implementations reference
clk->parent->rate and apply it to some divider or something.  To get
around having an opaque cookie .recalc_rate would have to have
parent->rate passed into it since the implementation cannot call
clk_get_rate(clk_get_parent(clk)) due to not having 'clk' in the first
place, and also because of mutex nesting.

I agree that by not using an opaque cookie we open ourselves up to the
possibility of badness, but we'll just have to catch it in code
review.

>
>> + * clk_init.  A key assumption in the call to .get_parent is that clks
>> + * are being initialized in-order.
>
> We should not require that, see below.
>
>> + */
>
>> +void clk_init(struct device *dev, struct clk *clk)
>> +{
>> +     if (!clk)
>> +             return;
>> +
>> +     INIT_HLIST_HEAD(&clk->children);
>> +     INIT_HLIST_NODE(&clk->child_node);
>> +
>> +     mutex_lock(&prepare_lock);
>> +
>> +     /*
>> +      * .get_parent is mandatory for populating .parent for clks with
>> +      * multiple possible parents.  For clks with a fixed parent
>> +      * .get_parent is not mandatory and .parent can be statically
>> +      * initialized
>> +      */
>> +     if (clk->ops->get_parent)
>> +             clk->parent = clk->ops->get_parent(clk);
>> +
>> +     /* clks without a parent are considered root clks */
>
> I'd rather prefer that root clocks are explicitely marked with a flag
> instead of relying on clk->parent being NULL.

I originally had CLK_IS_ROOT flag but removed it after thinking that
some clks might be a root sometimes, or a child at other times and I
had considered the flags to be constant.

If the flags aren't going to be constant and can change at run-time
then I can move this back to a flag.  Also if there are no use cases
where a clk can change from a root to a child then again this can
safely go into flags.  Thoughts?

>
>> +     if (clk->parent)
>> +             hlist_add_head(&clk->child_node,
>> +                             &clk->parent->children);
>> +     else
>> +             hlist_add_head(&clk->child_node, &clk_root_list);
>
> You could put clocks which have no parent and are not marked as root
> clock onto a separate list clk_orphaned or such. You might need this
> for handling clocks which are disconnected from any parent runtime as
> well.

Agreed, that is a more well-defined approach.

> And to avoid the whole in-order initialization issue, you could change
> clk_init to:
>
> struct clk *clk_init(struct device *dev, const struct clk_hw *hw,
>                     unsigned long flags, const char *parent_name)
>
> Then you can lookup the requested parent by name. If that fails, you
> put the clock into the orphaned list. When a new clock is initialized,
> then you walk the orphaned list and check whether something is waiting
> for that clock.
>
> To handle multi-parent clocks you need to go one step further and add:
>
> struct clk_hw {
>       struct clk_hw_ops *ops;
>       const char        *name;
>       const char        **possible_parents;
> };
>
> You also require a get_parent_idx() function in clk_hw_ops. So when
> clk_init() is called with parent_name = NULL and get_parent_idx() is
> implemented, then you call it and the clock returns you the index of
> the possible_parents array. If that parent clock is not yet
> registered, you store the requested name and do the lookup when new
> clocks are registered.

This approach ends up having something like O(n^2) (similar to
discussion around driver re-probing).

However, I agree that forcing folks to register/init clks in-order is
asking for trouble.  I also think that for groups of well defined clks
(SoC clks, or clks in the same device) that are statically initialized
we should allow for having clk->parent populated statically and leave
it at that (mux clks will still need to run clk->get_parent() in
clk_init of course, since we can't trust the bootloader).

>
> That has also the advantage, that you can validate parent settings
> upfront and for setting the parent during runtime, you don't have to
> acquire a pointer to the parent clock. It's enough to call
> clk_set_named_parent().

Right, but the usefulness of this last point hinges on whether or not
we want to hide struct clk from the rest of the world.  I still think
that doing so will end up being a limitation that platforms end up
having to hack around.  I'd like a lot more input from the folks
porting their platforms to this code on that point, as it is a
critical design element.

Thanks for reviewing,
Mike

>
> Thoughts ?
>
>         tglx
>
>
>

Re: [PATCH v4 5/6] clk: basic gateable and fixed-rate clks

[Turquette, Mike]

On Tue, Dec 13, 2011 at 9:15 PM, Ryan Mallon <rmallon@gmail.com> wrote:
> On 14/12/11 14:53, Mike Turquette wrote:
>
>> Many platforms support simple gateable clks and fixed-rate clks that
>> should not be re-implemented by every platform.
>>
>> This patch introduces a gateable clk with a common programming model of
>> gate control via a write of 1 bit to a register.  Both set-to-enable and
>> clear-to-enable are supported.
>>
>> Also introduced is a fixed-rate clk which has no reprogrammable aspects.
>>
>> The purpose of both types of clks is documented in drivers/clk/basic.c.
>>
>> TODO: add support for a simple divider, simple mux and a dummy clk for
>> stubbing out platform support.
>>
>> Based on original patch by Jeremy Kerr and contribution by Jamie Iles.
>>
>> Signed-off-by: Mike Turquette <mturquette@linaro.org>
>> Cc: Jeremy Kerr <jeremy.kerr@canonical.com>
>> Cc: Jamie Iles <jamie@jamieiles.com>
>
> <snip>
>
>> +int clk_register_gate(struct device *dev, const char *name, unsigned long flags,
>> +             struct clk *fixed_parent, void __iomem *reg, u8 bit_idx,
>> +             int set_to_enable)
>> +{
>> +     struct clk_hw_gate *gclk;
>> +     struct clk *clk;
>> +
>> +     gclk = kmalloc(sizeof(struct clk_hw_gate), GFP_KERNEL);
>> +
>> +     if (!gclk) {
>> +             pr_err("%s: could not allocate gated clk\n", __func__);
>> +             return -ENOMEM;
>> +     }
>> +
>> +     clk = &gclk->clk;
>> +
>> +     /* struct clk_hw_gate assignments */
>> +     gclk->fixed_parent = fixed_parent;
>> +     gclk->reg = reg;
>> +     gclk->bit_idx = bit_idx;
>> +
>> +     /* struct clk assignments */
>> +     clk->name = name;
>> +     clk->flags = flags;
>> +
>> +     if (set_to_enable)
>> +             clk->ops = &clk_hw_gate_set_enable_ops;
>> +     else
>> +             clk->ops = &clk_hw_gate_set_disable_ops;
>
>
> You could avoid having two sets of operations if you stored the
> set_to_enable value in struct clk_hw_gate. It might be useful to store
> additional information in struct clk_hw_gate if you also want to extend
> to supporting non-32bit registers (readb, etc), clocks with write only
> registers, or support clocks which require more than one bit to be set
> or cleared to enable them, etc. See the basic mmio gpio driver for a
> similar case.

I haven't given the basic clks enough attention, mostly because I
haven't started using them yet in my own platform's conversion to
common struct clk :-/

I think the original reason for the extra operations is to avoid the
conditional in the enable/disable path, but if we're going to end up
adding other conditionals anyways (such as the register locking in the
iMX platform implementation) then we should probably forget about that
approach and just deal with the complexity in one set of common
enable/disable functions.

Thanks for the review,
Mike

>
> ~Ryan
>

Re: [PATCH v4 3/6] clk: introduce the common clock framework

[Russell King - ARM Linux]

On Fri, Dec 16, 2011 at 04:45:48PM -0800, Turquette, Mike wrote:
> On Wed, Dec 14, 2011 at 5:18 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > On Tue, 13 Dec 2011, Mike Turquette wrote:
> >> +void __clk_unprepare(struct clk *clk)
> >> +{
> >> +     if (!clk)
> >> +             return;
> >> +
> >> +     if (WARN_ON(clk->prepare_count == 0))
> >> +             return;
> >> +
> >> +     if (--clk->prepare_count > 0)
> >> +             return;
> >> +
> >> +     WARN_ON(clk->enable_count > 0);
> >
> > So this leaves the clock enable count set. I'm a bit wary about
> > that. Shouldn't it either return (including bumping the prepare_count
> > again) or call clk_disable() ?

No it should not.

> I've hit this in my port of OMAP.  It comes from this simple situation:
> 
> driver 1 (adapted for clk_prepare/clk_unprepare):
> clk_prepare(clk);
> clk_enable(clk);
> 
> ...
> 
> driver2 (not adapted for clk_prepare/clk_unprepare):
> clk_enable(clk);

So this is basically buggy.  Look, it's quite simple.  Convert _all_
your drivers to clk_prepare/clk_unprepare _before_ you start switching
your platform to use these new functions.  You can do that _today_
without exception.

We must refuse to merge _any_ user which does this the old way - and
we should have been doing this since my commit was merged into mainline
to allow drivers to be converted.

And stop trying to think of ways around this inside clk_prepare/
clk_unprepare/clk_enable/clk_disable.  You can't do it.  Just fix _all_
the drivers.  Now.  Before you start implementing clk_prepare/clk_unprepare.

Re: [PATCH v4 3/6] clk: introduce the common clock framework

[Richard Zhao]

On Fri, Dec 16, 2011 at 04:45:48PM -0800, Turquette, Mike wrote:
> On Wed, Dec 14, 2011 at 5:18 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > On Tue, 13 Dec 2011, Mike Turquette wrote:
> >> +void __clk_unprepare(struct clk *clk)
> >> +{
> >> +     if (!clk)
> >> +             return;
> >> +
> >> +     if (WARN_ON(clk->prepare_count == 0))
> >> +             return;
> >> +
> >> +     if (--clk->prepare_count > 0)
> >> +             return;
> >> +
> >> +     WARN_ON(clk->enable_count > 0);
> >
> > So this leaves the clock enable count set. I'm a bit wary about
> > that. Shouldn't it either return (including bumping the prepare_count
> > again) or call clk_disable() ?
> 
> I've hit this in my port of OMAP.  It comes from this simple situation:
> 
> driver 1 (adapted for clk_prepare/clk_unprepare):
> clk_prepare(clk);
> clk_enable(clk);
> 
> ...
> 
> driver2 (not adapted for clk_prepare/clk_unprepare):
> clk_enable(clk);
> 
> ...
> 
> driver1:
> clk_disable(clk);
> clk_unprepare(clk);
> 
> In such a case we don't want to bump the prepare_count, since the
> offending driver2 won't decrement that count.  Also we don't want to
> shut down that clk since driver2 is using it.
> 
> Returning after the WARN is maybe OK, but the point of this check is
> really to find code which hasn't yet been made prepare-aware, and the
> current implementation is noisy enough about it.
> 
> >> +/**
> >> + * clk_get_parent - return the parent of a clk
> >> + * @clk: the clk whose parent gets returned
> >> + *
> >> + * Simply returns clk->parent.  It is up to the caller to hold the
> >> + * prepare_lock, if desired.  Returns NULL if clk is NULL.
> >
> > Holding the prepare lock in the caller will deadlock. So it's not a
> > real good idea to document it as an option :)
> 
> Oops.  That comment is left over from before clk_get_parent held the
> lock.  Will remove.
> 
> >
> >> + */
> >> +struct clk *clk_get_parent(struct clk *clk)
> >> +{
> >> +     struct clk *parent;
> >> +
> >> +     if (!clk)
> >> +             return NULL;
> >> +
> >> +     mutex_lock(&prepare_lock);
> >
> >> +/**
> >> + * clk_init - initialize the data structures in a struct clk
> >> + * @dev: device initializing this clk, placeholder for now
> >> + * @clk: clk being initialized
> >> + *
> >> + * Initializes the lists in struct clk, queries the hardware for the
> >> + * parent and rate and sets them both.  Adds the clk to the sysfs tree
> >> + * topology.
> >> + *
> >> + * Caller must populate clk->name and clk->flags before calling
> >
> > I'm not too happy about this construct. That leaves struct clk and its
> > members exposed to the world instead of making it a real opaque
> > cookie. I know from my own painful experience, that this will lead to
> > random fiddling in that data structure in drivers and arch code just
> > because the core code has a shortcoming.
> >
> > Why can't we make struct clk a real cookie and confine the data
> > structure to the core code ?
> >
> > That would change the init call to something like:
> >
> > struct clk *clk_init(struct device *dev, const struct clk_hw *hw,
> >                     struct clk *parent)
> >
> > And have:
> > struct clk_hw {
> >       struct clk_hw_ops *ops;
> >       const char        *name;
> >       unsigned long     flags;
> > };
> >
> > Implementers can do:
> > struct my_clk_hw {
> >       struct clk_hw    hw;
> >       mydata;
> > };
> >
> > And then change the clk ops callbacks to take struct clk_hw * as an
> > argument.
We have to define static clocks before we adopt DT binding.
If clk is opaque and allocate memory in clk core, it'll make hard
to define static clocks. And register/init will pass a long parameter
list.
> >
> > So the core code can allocate the clk data structure and you return a
> > real opaque cookie. You do the same thing for the basic gated clock in
> > one of the next patches, so doing it for struct clk itself is just
> > consequent.
> 
> The opaque cookie would work if platform code never needs any
> information outside of the data a single clk_hw_whatever provides.
> Unfortunately hardware doesn't make things that easy on us and the
> operations we do for a single clk are affected by the state of other
> clks.
> 
> Here is an example I've been using a lot lately: on OMAP we have two
> clock inputs to our PLLs: a bypass clk and reference clk (actually we
> can have more than this).  When changing the PLL rate both clks must
> be enabled, regardless of which clk ends up driving the PLL.  To
> illustrate, the PLL may be driven by clk_ref at 400MHz, and then we
> call clk_set_rate(pll_clk, 196000000); which will also leave it
> clocked by clk_ref but at 196MHz.  For this to happen however, the
> clk_bypass had to be enabled during the rate change operation.  Here
> is the relevant .set_rate implementation:
> http://git.linaro.org/gitweb?p=people/mturquette/linux.git;a=blob;f=arch/arm/mach-omap2/dpll3xxx.c;h=b2412574b63829944593c1a7a6eda5fa4350686f;hb=738bde65918ae1ac743b1498801da0b860e2ee32#l461
> 
> In order for the OMAP platform code to do this, we have to have two
> things: firstly we need the struct clk so that we can call
> clk_enable/disable and __clk_prepare/unprepare on the ref and bypass
> clks from within .set_rate.  The second thing is that we need
> __clk_prepare and __clk_unprepare to be visible by this code so that
> we don't nest the prepare_lock mutex.
> 
> Is there a good way to solve such problems if we hide struct clk from
> the platform code/clk driver implementation?
You can add a function clk_to_hw to convert struct clk to struct clk_hw.
It's the way I did with the patch you didn't expose struct clk.

clock driver gate2b needs to access clk_hw. clk_gate2b_set_val can set
value for enable and disable at runtime. At opaque time, it have to get
clk_hw first and then get struct clk_gate2b.
> 
> Also note that if top-level clk APIs are going to be holding
> prepare_lock (clk_get_rate, clk_get_parent, etc) then we can't call
> these APIs to get the info we want from within the platform code.  For
> example, today most .recalc_rate implementations reference
> clk->parent->rate and apply it to some divider or something.  To get
> around having an opaque cookie .recalc_rate would have to have
> parent->rate passed into it since the implementation cannot call
> clk_get_rate(clk_get_parent(clk)) due to not having 'clk' in the first
> place, and also because of mutex nesting.
why do we need lock in clk_get_rate?
> 
> I agree that by not using an opaque cookie we open ourselves up to the
> possibility of badness, but we'll just have to catch it in code
> review.
> 
> >
> >> + * clk_init.  A key assumption in the call to .get_parent is that clks
> >> + * are being initialized in-order.
> >
> > We should not require that, see below.
> >
> >> + */
> >
> >> +void clk_init(struct device *dev, struct clk *clk)
> >> +{
> >> +     if (!clk)
> >> +             return;
> >> +
> >> +     INIT_HLIST_HEAD(&clk->children);
> >> +     INIT_HLIST_NODE(&clk->child_node);
> >> +
> >> +     mutex_lock(&prepare_lock);
> >> +
> >> +     /*
> >> +      * .get_parent is mandatory for populating .parent for clks with
> >> +      * multiple possible parents.  For clks with a fixed parent
> >> +      * .get_parent is not mandatory and .parent can be statically
> >> +      * initialized
> >> +      */
> >> +     if (clk->ops->get_parent)
> >> +             clk->parent = clk->ops->get_parent(clk);
> >> +
> >> +     /* clks without a parent are considered root clks */
> >
> > I'd rather prefer that root clocks are explicitely marked with a flag
> > instead of relying on clk->parent being NULL.
> 
> I originally had CLK_IS_ROOT flag but removed it after thinking that
> some clks might be a root sometimes, or a child at other times and I
> had considered the flags to be constant.
I didn't see part-time root clock in IMX.
> 
> If the flags aren't going to be constant and can change at run-time
> then I can move this back to a flag.  Also if there are no use cases
> where a clk can change from a root to a child then again this can
> safely go into flags.  Thoughts?
> 
> >
> >> +     if (clk->parent)
> >> +             hlist_add_head(&clk->child_node,
> >> +                             &clk->parent->children);
> >> +     else
> >> +             hlist_add_head(&clk->child_node, &clk_root_list);
> >
> > You could put clocks which have no parent and are not marked as root
> > clock onto a separate list clk_orphaned or such. You might need this
> > for handling clocks which are disconnected from any parent runtime as
> > well.
> 
> Agreed, that is a more well-defined approach.
> 
> > And to avoid the whole in-order initialization issue, you could change
> > clk_init to:
> >
> > struct clk *clk_init(struct device *dev, const struct clk_hw *hw,
> >                     unsigned long flags, const char *parent_name)
> >
> > Then you can lookup the requested parent by name. If that fails, you
> > put the clock into the orphaned list. When a new clock is initialized,
> > then you walk the orphaned list and check whether something is waiting
> > for that clock.
> >
> > To handle multi-parent clocks you need to go one step further and add:
> >
> > struct clk_hw {
> >       struct clk_hw_ops *ops;
> >       const char        *name;
> >       const char        **possible_parents;
> > };
> >
> > You also require a get_parent_idx() function in clk_hw_ops. So when
> > clk_init() is called with parent_name = NULL and get_parent_idx() is
> > implemented, then you call it and the clock returns you the index of
> > the possible_parents array. If that parent clock is not yet
> > registered, you store the requested name and do the lookup when new
> > clocks are registered.
> 
> This approach ends up having something like O(n^2) (similar to
> discussion around driver re-probing).
Split it to two functions? clk_add can add clks in any order, and
clk_tree_init call .get_parent (if possible) create the tree.
Adding clk after clk_tree_init must be in strong order.
> 
> However, I agree that forcing folks to register/init clks in-order is
> asking for trouble.  I also think that for groups of well defined clks
> (SoC clks, or clks in the same device) that are statically initialized
> we should allow for having clk->parent populated statically and leave
> it at that (mux clks will still need to run clk->get_parent() in
> clk_init of course, since we can't trust the bootloader).
right.
> 
> >
> > That has also the advantage, that you can validate parent settings
> > upfront and for setting the parent during runtime, you don't have to
> > acquire a pointer to the parent clock. It's enough to call
> > clk_set_named_parent().
That'll cause clk core to call clkdev functions. But looks fine.

Thanks
Richard
> 
> Right, but the usefulness of this last point hinges on whether or not
> we want to hide struct clk from the rest of the world.  I still think
> that doing so will end up being a limitation that platforms end up
> having to hack around.  I'd like a lot more input from the folks
> porting their platforms to this code on that point, as it is a
> critical design element.
> 
> Thanks for reviewing,
> Mike
> 
> >
> > Thoughts ?
> >
> >         tglx
> >
> >
> >
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

Re: [PATCH v4 3/6] clk: introduce the common clock framework

[Rob Herring]

On 01/03/2012 08:15 PM, Richard Zhao wrote:
> On Fri, Dec 16, 2011 at 04:45:48PM -0800, Turquette, Mike wrote:
>> On Wed, Dec 14, 2011 at 5:18 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
>>> On Tue, 13 Dec 2011, Mike Turquette wrote:

snip

>>>> +/**
>>>> + * clk_init - initialize the data structures in a struct clk
>>>> + * @dev: device initializing this clk, placeholder for now
>>>> + * @clk: clk being initialized
>>>> + *
>>>> + * Initializes the lists in struct clk, queries the hardware for the
>>>> + * parent and rate and sets them both.  Adds the clk to the sysfs tree
>>>> + * topology.
>>>> + *
>>>> + * Caller must populate clk->name and clk->flags before calling
>>>
>>> I'm not too happy about this construct. That leaves struct clk and its
>>> members exposed to the world instead of making it a real opaque
>>> cookie. I know from my own painful experience, that this will lead to
>>> random fiddling in that data structure in drivers and arch code just
>>> because the core code has a shortcoming.
>>>
>>> Why can't we make struct clk a real cookie and confine the data
>>> structure to the core code ?
>>>
>>> That would change the init call to something like:
>>>
>>> struct clk *clk_init(struct device *dev, const struct clk_hw *hw,
>>>                     struct clk *parent)
>>>
>>> And have:
>>> struct clk_hw {
>>>       struct clk_hw_ops *ops;
>>>       const char        *name;
>>>       unsigned long     flags;
>>> };
>>>
>>> Implementers can do:
>>> struct my_clk_hw {
>>>       struct clk_hw    hw;
>>>       mydata;
>>> };
>>>
>>> And then change the clk ops callbacks to take struct clk_hw * as an
>>> argument.
> We have to define static clocks before we adopt DT binding.
> If clk is opaque and allocate memory in clk core, it'll make hard
> to define static clocks. And register/init will pass a long parameter
> list.

DT is not a prerequisite for having dynamically created clocks. You can
make clock init dynamic without DT.

What data goes in struct clk vs. struct clk_hw could change over time.
So perhaps we can start with some data in clk_hw and plan to move it to
struct clk later. Even if almost everything ends up in clk_hw initially,
at least the structure is in place to have common, core-only data
separate from platform data.

What is the actual data you need to be static and accessible to the
platform code? A ptr to parent clk is the main thing I've seen for
static initialization. So make the parent ptr be struct clk_hw* and
allow the platforms to access.

Rob

Re: [PATCH v4 3/6] clk: introduce the common clock framework

[Turquette, Mike]

On Wed, Jan 4, 2012 at 6:32 AM, Rob Herring <robherring2@gmail.com> wrote:
> On 01/03/2012 08:15 PM, Richard Zhao wrote:
>> On Fri, Dec 16, 2011 at 04:45:48PM -0800, Turquette, Mike wrote:
>>> On Wed, Dec 14, 2011 at 5:18 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
>>>> On Tue, 13 Dec 2011, Mike Turquette wrote:
>
> snip
>
>>>>> +/**
>>>>> + * clk_init - initialize the data structures in a struct clk
>>>>> + * @dev: device initializing this clk, placeholder for now
>>>>> + * @clk: clk being initialized
>>>>> + *
>>>>> + * Initializes the lists in struct clk, queries the hardware for the
>>>>> + * parent and rate and sets them both.  Adds the clk to the sysfs tree
>>>>> + * topology.
>>>>> + *
>>>>> + * Caller must populate clk->name and clk->flags before calling
>>>>
>>>> I'm not too happy about this construct. That leaves struct clk and its
>>>> members exposed to the world instead of making it a real opaque
>>>> cookie. I know from my own painful experience, that this will lead to
>>>> random fiddling in that data structure in drivers and arch code just
>>>> because the core code has a shortcoming.
>>>>
>>>> Why can't we make struct clk a real cookie and confine the data
>>>> structure to the core code ?
>>>>
>>>> That would change the init call to something like:
>>>>
>>>> struct clk *clk_init(struct device *dev, const struct clk_hw *hw,
>>>>                     struct clk *parent)
>>>>
>>>> And have:
>>>> struct clk_hw {
>>>>       struct clk_hw_ops *ops;
>>>>       const char        *name;
>>>>       unsigned long     flags;
>>>> };
>>>>
>>>> Implementers can do:
>>>> struct my_clk_hw {
>>>>       struct clk_hw    hw;
>>>>       mydata;
>>>> };
>>>>
>>>> And then change the clk ops callbacks to take struct clk_hw * as an
>>>> argument.
>> We have to define static clocks before we adopt DT binding.
>> If clk is opaque and allocate memory in clk core, it'll make hard
>> to define static clocks. And register/init will pass a long parameter
>> list.
>
> DT is not a prerequisite for having dynamically created clocks. You can
> make clock init dynamic without DT.

Agreed.

> What data goes in struct clk vs. struct clk_hw could change over time.
> So perhaps we can start with some data in clk_hw and plan to move it to
> struct clk later. Even if almost everything ends up in clk_hw initially,
> at least the structure is in place to have common, core-only data
> separate from platform data.

What is the point of this?

The original clk_hw was defined simply as:

struct clk_hw {
        struct clk *clk;
};

It's only purpose in life was as a handle for navigation between the
opaque struct clk and the hardware-specific struct my_clk_hw.  struct
clk_hw is defined in clk.h and everyone can see it.  If we're suddenly
OK putting clk data in this structure then why bother with an opaque
struct clk at all?

> What is the actual data you need to be static and accessible to the
> platform code? A ptr to parent clk is the main thing I've seen for
> static initialization. So make the parent ptr be struct clk_hw* and
> allow the platforms to access.

To answer your question on what data we're trying to expose: platform
code commonly needs the parent pointer and the clk rate (and by
extension, the rate of the parent).  For debug/error prints it is also
nice to have the clk name.  Generic clk flags are also conceivably
something that platform code might want.

I'd like to spin the question around: if we're OK exposing some stuff
(in your example above, the parent pointer), then what clk data are
you trying to hide?

Regards,
Mike

>
> Rob

Re: [PATCH v4 3/6] clk: introduce the common clock framework

[Richard Zhao]

On Wed, Jan 04, 2012 at 05:01:43PM -0800, Turquette, Mike wrote:
> On Wed, Jan 4, 2012 at 6:32 AM, Rob Herring <robherring2@gmail.com> wrote:
> > On 01/03/2012 08:15 PM, Richard Zhao wrote:
> >> On Fri, Dec 16, 2011 at 04:45:48PM -0800, Turquette, Mike wrote:
> >>> On Wed, Dec 14, 2011 at 5:18 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> >>>> On Tue, 13 Dec 2011, Mike Turquette wrote:
> >
> > snip
> >
> >>>>> +/**
> >>>>> + * clk_init - initialize the data structures in a struct clk
> >>>>> + * @dev: device initializing this clk, placeholder for now
> >>>>> + * @clk: clk being initialized
> >>>>> + *
> >>>>> + * Initializes the lists in struct clk, queries the hardware for the
> >>>>> + * parent and rate and sets them both.  Adds the clk to the sysfs tree
> >>>>> + * topology.
> >>>>> + *
> >>>>> + * Caller must populate clk->name and clk->flags before calling
> >>>>
> >>>> I'm not too happy about this construct. That leaves struct clk and its
> >>>> members exposed to the world instead of making it a real opaque
> >>>> cookie. I know from my own painful experience, that this will lead to
> >>>> random fiddling in that data structure in drivers and arch code just
> >>>> because the core code has a shortcoming.
> >>>>
> >>>> Why can't we make struct clk a real cookie and confine the data
> >>>> structure to the core code ?
> >>>>
> >>>> That would change the init call to something like:
> >>>>
> >>>> struct clk *clk_init(struct device *dev, const struct clk_hw *hw,
> >>>>                     struct clk *parent)
> >>>>
> >>>> And have:
> >>>> struct clk_hw {
> >>>>       struct clk_hw_ops *ops;
> >>>>       const char        *name;
> >>>>       unsigned long     flags;
> >>>> };
> >>>>
> >>>> Implementers can do:
> >>>> struct my_clk_hw {
> >>>>       struct clk_hw    hw;
> >>>>       mydata;
> >>>> };
> >>>>
> >>>> And then change the clk ops callbacks to take struct clk_hw * as an
> >>>> argument.
> >> We have to define static clocks before we adopt DT binding.
> >> If clk is opaque and allocate memory in clk core, it'll make hard
> >> to define static clocks. And register/init will pass a long parameter
> >> list.
> >
> > DT is not a prerequisite for having dynamically created clocks. You can
> > make clock init dynamic without DT.
I can not find clock info at runtime without DT. If I use static info, I
find it was hard/strange to define and register it, using Mike's early patches.
> 
> Agreed.
> 
> > What data goes in struct clk vs. struct clk_hw could change over time.
> > So perhaps we can start with some data in clk_hw and plan to move it to
> > struct clk later. Even if almost everything ends up in clk_hw initially,
> > at least the structure is in place to have common, core-only data
> > separate from platform data.
> 
> What is the point of this?
> 
> The original clk_hw was defined simply as:
> 
> struct clk_hw {
>         struct clk *clk;
> };
> 
> It's only purpose in life was as a handle for navigation between the
> opaque struct clk and the hardware-specific struct my_clk_hw.  struct
> clk_hw is defined in clk.h and everyone can see it.  If we're suddenly
> OK putting clk data in this structure then why bother with an opaque
> struct clk at all?
I think Rob meant one time a step to make it opaque. But it'll make
clk core always changing, easier mess, and let clk driver confused.
> 
> > What is the actual data you need to be static and accessible to the
> > platform code? A ptr to parent clk is the main thing I've seen for
> > static initialization. So make the parent ptr be struct clk_hw* and
> > allow the platforms to access.
> 
> To answer your question on what data we're trying to expose: platform
> code commonly needs the parent pointer and the clk rate (and by
> extension, the rate of the parent).  For debug/error prints it is also
> nice to have the clk name.  Generic clk flags are also conceivably
> something that platform code might want.
> 
> I'd like to spin the question around: if we're OK exposing some stuff
> (in your example above, the parent pointer), then what clk data are
> you trying to hide?
> 
> Regards,
> Mike
> 
> >
> > Rob
> 

Re: [PATCH v4 3/6] clk: introduce the common clock framework

[Rob Herring]

On 01/04/2012 07:01 PM, Turquette, Mike wrote:
> On Wed, Jan 4, 2012 at 6:32 AM, Rob Herring <robherring2@gmail.com> wrote:
>> On 01/03/2012 08:15 PM, Richard Zhao wrote:
>>> On Fri, Dec 16, 2011 at 04:45:48PM -0800, Turquette, Mike wrote:
>>>> On Wed, Dec 14, 2011 at 5:18 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
>>>>> On Tue, 13 Dec 2011, Mike Turquette wrote:
>>
>> snip
>>
>>>>>> +/**
>>>>>> + * clk_init - initialize the data structures in a struct clk
>>>>>> + * @dev: device initializing this clk, placeholder for now
>>>>>> + * @clk: clk being initialized
>>>>>> + *
>>>>>> + * Initializes the lists in struct clk, queries the hardware for the
>>>>>> + * parent and rate and sets them both.  Adds the clk to the sysfs tree
>>>>>> + * topology.
>>>>>> + *
>>>>>> + * Caller must populate clk->name and clk->flags before calling
>>>>>
>>>>> I'm not too happy about this construct. That leaves struct clk and its
>>>>> members exposed to the world instead of making it a real opaque
>>>>> cookie. I know from my own painful experience, that this will lead to
>>>>> random fiddling in that data structure in drivers and arch code just
>>>>> because the core code has a shortcoming.
>>>>>
>>>>> Why can't we make struct clk a real cookie and confine the data
>>>>> structure to the core code ?
>>>>>
>>>>> That would change the init call to something like:
>>>>>
>>>>> struct clk *clk_init(struct device *dev, const struct clk_hw *hw,
>>>>>                     struct clk *parent)
>>>>>
>>>>> And have:
>>>>> struct clk_hw {
>>>>>       struct clk_hw_ops *ops;
>>>>>       const char        *name;
>>>>>       unsigned long     flags;
>>>>> };
>>>>>
>>>>> Implementers can do:
>>>>> struct my_clk_hw {
>>>>>       struct clk_hw    hw;
>>>>>       mydata;
>>>>> };
>>>>>
>>>>> And then change the clk ops callbacks to take struct clk_hw * as an
>>>>> argument.
>>> We have to define static clocks before we adopt DT binding.
>>> If clk is opaque and allocate memory in clk core, it'll make hard
>>> to define static clocks. And register/init will pass a long parameter
>>> list.
>>
>> DT is not a prerequisite for having dynamically created clocks. You can
>> make clock init dynamic without DT.
> 
> Agreed.
> 
>> What data goes in struct clk vs. struct clk_hw could change over time.
>> So perhaps we can start with some data in clk_hw and plan to move it to
>> struct clk later. Even if almost everything ends up in clk_hw initially,
>> at least the structure is in place to have common, core-only data
>> separate from platform data.
> 
> What is the point of this?

To have a way forward. It would be nice to have a clk infrastructure
before I retire...

> The original clk_hw was defined simply as:
> 
> struct clk_hw {
>         struct clk *clk;
> };
> 
> It's only purpose in life was as a handle for navigation between the
> opaque struct clk and the hardware-specific struct my_clk_hw.  struct
> clk_hw is defined in clk.h and everyone can see it.  If we're suddenly
> OK putting clk data in this structure then why bother with an opaque
> struct clk at all?
> 
>> What is the actual data you need to be static and accessible to the
>> platform code? A ptr to parent clk is the main thing I've seen for
>> static initialization. So make the parent ptr be struct clk_hw* and
>> allow the platforms to access.
> 
> To answer your question on what data we're trying to expose: platform
> code commonly needs the parent pointer and the clk rate (and by
> extension, the rate of the parent).  For debug/error prints it is also
> nice to have the clk name.  Generic clk flags are also conceivably
> something that platform code might want.

I agree with the need to have the parent and flags from a static init
perspective. There's not really a good reason the others can't be
accessed thru accessors though.

> I'd like to spin the question around: if we're OK exposing some stuff
> (in your example above, the parent pointer), then what clk data are
> you trying to hide?

Well, everything from drivers which the current clk implementations do
hide. Catching abuse in with drivers coming in from all different trees
and lists will be impossible.

For platform code it is more fuzzy. I don't think platform code should
be allowed to muck with prepare/enable counts for example.

Rob

> 
> Regards,
> Mike
> 
>>
>> Rob

Re: [PATCH v4 3/6] clk: introduce the common clock framework

[Turquette, Mike]

On Wed, Jan 4, 2012 at 6:11 PM, Rob Herring <robherring2@gmail.com> wrote:
> On 01/04/2012 07:01 PM, Turquette, Mike wrote:
>> On Wed, Jan 4, 2012 at 6:32 AM, Rob Herring <robherring2@gmail.com> wrote:
>>> On 01/03/2012 08:15 PM, Richard Zhao wrote:
>>>> On Fri, Dec 16, 2011 at 04:45:48PM -0800, Turquette, Mike wrote:
>>>>> On Wed, Dec 14, 2011 at 5:18 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
>>>>>> On Tue, 13 Dec 2011, Mike Turquette wrote:
>>>
>>> snip
>>>
>>>>>>> +/**
>>>>>>> + * clk_init - initialize the data structures in a struct clk
>>>>>>> + * @dev: device initializing this clk, placeholder for now
>>>>>>> + * @clk: clk being initialized
>>>>>>> + *
>>>>>>> + * Initializes the lists in struct clk, queries the hardware for the
>>>>>>> + * parent and rate and sets them both.  Adds the clk to the sysfs tree
>>>>>>> + * topology.
>>>>>>> + *
>>>>>>> + * Caller must populate clk->name and clk->flags before calling
>>>>>>
>>>>>> I'm not too happy about this construct. That leaves struct clk and its
>>>>>> members exposed to the world instead of making it a real opaque
>>>>>> cookie. I know from my own painful experience, that this will lead to
>>>>>> random fiddling in that data structure in drivers and arch code just
>>>>>> because the core code has a shortcoming.
>>>>>>
>>>>>> Why can't we make struct clk a real cookie and confine the data
>>>>>> structure to the core code ?
>>>>>>
>>>>>> That would change the init call to something like:
>>>>>>
>>>>>> struct clk *clk_init(struct device *dev, const struct clk_hw *hw,
>>>>>>                     struct clk *parent)
>>>>>>
>>>>>> And have:
>>>>>> struct clk_hw {
>>>>>>       struct clk_hw_ops *ops;
>>>>>>       const char        *name;
>>>>>>       unsigned long     flags;
>>>>>> };
>>>>>>
>>>>>> Implementers can do:
>>>>>> struct my_clk_hw {
>>>>>>       struct clk_hw    hw;
>>>>>>       mydata;
>>>>>> };
>>>>>>
>>>>>> And then change the clk ops callbacks to take struct clk_hw * as an
>>>>>> argument.
>>>> We have to define static clocks before we adopt DT binding.
>>>> If clk is opaque and allocate memory in clk core, it'll make hard
>>>> to define static clocks. And register/init will pass a long parameter
>>>> list.
>>>
>>> DT is not a prerequisite for having dynamically created clocks. You can
>>> make clock init dynamic without DT.
>>
>> Agreed.
>>
>>> What data goes in struct clk vs. struct clk_hw could change over time.
>>> So perhaps we can start with some data in clk_hw and plan to move it to
>>> struct clk later. Even if almost everything ends up in clk_hw initially,
>>> at least the structure is in place to have common, core-only data
>>> separate from platform data.
>>
>> What is the point of this?
>
> To have a way forward. It would be nice to have a clk infrastructure
> before I retire...

Haha, agreed.

>
>> The original clk_hw was defined simply as:
>>
>> struct clk_hw {
>>         struct clk *clk;
>> };
>>
>> It's only purpose in life was as a handle for navigation between the
>> opaque struct clk and the hardware-specific struct my_clk_hw.  struct
>> clk_hw is defined in clk.h and everyone can see it.  If we're suddenly
>> OK putting clk data in this structure then why bother with an opaque
>> struct clk at all?
>>
>>> What is the actual data you need to be static and accessible to the
>>> platform code? A ptr to parent clk is the main thing I've seen for
>>> static initialization. So make the parent ptr be struct clk_hw* and
>>> allow the platforms to access.
>>
>> To answer your question on what data we're trying to expose: platform
>> code commonly needs the parent pointer and the clk rate (and by
>> extension, the rate of the parent).  For debug/error prints it is also
>> nice to have the clk name.  Generic clk flags are also conceivably
>> something that platform code might want.
>
> I agree with the need to have the parent and flags from a static init
> perspective. There's not really a good reason the others can't be
> accessed thru accessors though.
>
>> I'd like to spin the question around: if we're OK exposing some stuff
>> (in your example above, the parent pointer), then what clk data are
>> you trying to hide?
>
> Well, everything from drivers which the current clk implementations do
> hide. Catching abuse in with drivers coming in from all different trees
> and lists will be impossible.
>
> For platform code it is more fuzzy. I don't think platform code should
> be allowed to muck with prepare/enable counts for example.

So there is a clear dichotomy: drivers shouldn't be exposed to any of
it and platform code should be exposed to some of it.

How about a drivers/clk/clk-private.h which will define struct clk and
must only be included by clk drivers in drivers/clk/*?

This establishes a bright line between those things which are allowed
to know the details of struct clk and those that are not: namely that
clk drivers in drivers/clk/ may use '#include "clk-private.h"'.
Obviously struct clk is opaque to the rest of the kernel (in the same
way it has been prior to the common clk patches) and there is no need
for struct clk_hw anymore.  Also helper functions are no longer needed
for clock driver code, which I think *is* a manageable set of code to
review.  Also clk drivers must live in drivers/clk/ for this to work
(without a big ugly path in someone's #include directive somewhere).

Thoughts?

Regards,
Mike

>
> Rob
>
>>
>> Regards,
>> Mike
>>
>>>
>>> Rob
>

Re: [PATCH v4 2/6] Documentation: common clk API

[Amit Kucheria]

Tiny, tiny typo...

On 11 Dec 13, Mike Turquette wrote:

> +clk_set_rate deserves a special mention because it is more complex than
> +the other operations.  There are three key concepts to the common
> +clk_set_rate implementation:
> +
> +1) recursively traversing up the clk tree and changing clk rates, one
> +parent at a time, if each clk allows it
> +2) failing to change rate if the clk is enabled and must only change
> +rates while disabled
> +3) using clk rate change notifiers to allow devices to handle dynamic
> +rate changes for clks which do support changing rates while enabled
> +
> +For the simple, non-recursive case the call graph looks like:
> +
> +clk_set_rate(clk, rate);
> +	__clk_set_rate(clk, rate);
> +		clk->round_rate(clk, rate *parent_rate);
                                      ^^^^^^^^

need a comma here? The next sentence kept me busy for 5 mins.

> +		clk->set_rate(clk, rate);
> +
> +You might be wondering what that third paramater in .round_rate is.  If
> +a clk supports the CLK_PARENT_SET_RATE flag then that enables it's
> +hardware-specific .round_rate function to provide a new rate that the
> +parent should transition to.  For example, imagine a rate-adjustable clk
> +A that is the parent of clk B, which has a fixed divider of 2.

Re: [PATCH v4 2/6] Documentation: common clk API

[Turquette, Mike]

On Thu, Jan 5, 2012 at 6:31 AM, Amit Kucheria <amit.kucheria@linaro.org> wrote:
> Tiny, tiny typo...
>
> On 11 Dec 13, Mike Turquette wrote:
>
>> +clk_set_rate deserves a special mention because it is more complex than
>> +the other operations.  There are three key concepts to the common
>> +clk_set_rate implementation:
>> +
>> +1) recursively traversing up the clk tree and changing clk rates, one
>> +parent at a time, if each clk allows it
>> +2) failing to change rate if the clk is enabled and must only change
>> +rates while disabled
>> +3) using clk rate change notifiers to allow devices to handle dynamic
>> +rate changes for clks which do support changing rates while enabled
>> +
>> +For the simple, non-recursive case the call graph looks like:
>> +
>> +clk_set_rate(clk, rate);
>> +     __clk_set_rate(clk, rate);
>> +             clk->round_rate(clk, rate *parent_rate);
>                                      ^^^^^^^^
>
> need a comma here? The next sentence kept me busy for 5 mins.

Thanks, will fix in next submission (along with general rework of
messy documentation).

Mike

>
>> +             clk->set_rate(clk, rate);
>> +
>> +You might be wondering what that third paramater in .round_rate is.  If
>> +a clk supports the CLK_PARENT_SET_RATE flag then that enables it's
>> +hardware-specific .round_rate function to provide a new rate that the
>> +parent should transition to.  For example, imagine a rate-adjustable clk
>> +A that is the parent of clk B, which has a fixed divider of 2.

Re: [PATCH v4 3/6] clk: introduce the common clock framework

[Amit Kucheria]

On 12 Jan 04, Turquette, Mike wrote:
> On Wed, Jan 4, 2012 at 6:11 PM, Rob Herring <robherring2@gmail.com> wrote:
> > On 01/04/2012 07:01 PM, Turquette, Mike wrote:
> >> On Wed, Jan 4, 2012 at 6:32 AM, Rob Herring <robherring2@gmail.com> wrote:
> >>> On 01/03/2012 08:15 PM, Richard Zhao wrote:
> >>>> On Fri, Dec 16, 2011 at 04:45:48PM -0800, Turquette, Mike wrote:
> >>>>> On Wed, Dec 14, 2011 at 5:18 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> >>>>>> On Tue, 13 Dec 2011, Mike Turquette wrote:
> >>>
> >>> snip
> >>>
> >>>>>>> +/**
> >>>>>>> + * clk_init - initialize the data structures in a struct clk
> >>>>>>> + * @dev: device initializing this clk, placeholder for now
> >>>>>>> + * @clk: clk being initialized
> >>>>>>> + *
> >>>>>>> + * Initializes the lists in struct clk, queries the hardware for the
> >>>>>>> + * parent and rate and sets them both.  Adds the clk to the sysfs tree
> >>>>>>> + * topology.
> >>>>>>> + *
> >>>>>>> + * Caller must populate clk->name and clk->flags before calling
> >>>>>>
> >>>>>> I'm not too happy about this construct. That leaves struct clk and its
> >>>>>> members exposed to the world instead of making it a real opaque
> >>>>>> cookie. I know from my own painful experience, that this will lead to
> >>>>>> random fiddling in that data structure in drivers and arch code just
> >>>>>> because the core code has a shortcoming.
> >>>>>>
> >>>>>> Why can't we make struct clk a real cookie and confine the data
> >>>>>> structure to the core code ?
> >>>>>>
> >>>>>> That would change the init call to something like:
> >>>>>>
> >>>>>> struct clk *clk_init(struct device *dev, const struct clk_hw *hw,
> >>>>>>                     struct clk *parent)
> >>>>>>
> >>>>>> And have:
> >>>>>> struct clk_hw {
> >>>>>>       struct clk_hw_ops *ops;
> >>>>>>       const char        *name;
> >>>>>>       unsigned long     flags;
> >>>>>> };
> >>>>>>
> >>>>>> Implementers can do:
> >>>>>> struct my_clk_hw {
> >>>>>>       struct clk_hw    hw;
> >>>>>>       mydata;
> >>>>>> };
> >>>>>>
> >>>>>> And then change the clk ops callbacks to take struct clk_hw * as an
> >>>>>> argument.
> >>>> We have to define static clocks before we adopt DT binding.
> >>>> If clk is opaque and allocate memory in clk core, it'll make hard
> >>>> to define static clocks. And register/init will pass a long parameter
> >>>> list.
> >>>
> >>> DT is not a prerequisite for having dynamically created clocks. You can
> >>> make clock init dynamic without DT.
> >>
> >> Agreed.
> >>
> >>> What data goes in struct clk vs. struct clk_hw could change over time.
> >>> So perhaps we can start with some data in clk_hw and plan to move it to
> >>> struct clk later. Even if almost everything ends up in clk_hw initially,
> >>> at least the structure is in place to have common, core-only data
> >>> separate from platform data.
> >>
> >> What is the point of this?
> >
> > To have a way forward. It would be nice to have a clk infrastructure
> > before I retire...
> 
> Haha, agreed.
> 
> >
> >> The original clk_hw was defined simply as:
> >>
> >> struct clk_hw {
> >>         struct clk *clk;
> >> };
> >>
> >> It's only purpose in life was as a handle for navigation between the
> >> opaque struct clk and the hardware-specific struct my_clk_hw.  struct
> >> clk_hw is defined in clk.h and everyone can see it.  If we're suddenly
> >> OK putting clk data in this structure then why bother with an opaque
> >> struct clk at all?
> >>
> >>> What is the actual data you need to be static and accessible to the
> >>> platform code? A ptr to parent clk is the main thing I've seen for
> >>> static initialization. So make the parent ptr be struct clk_hw* and
> >>> allow the platforms to access.
> >>
> >> To answer your question on what data we're trying to expose: platform
> >> code commonly needs the parent pointer and the clk rate (and by
> >> extension, the rate of the parent).  For debug/error prints it is also
> >> nice to have the clk name.  Generic clk flags are also conceivably
> >> something that platform code might want.
> >
> > I agree with the need to have the parent and flags from a static init
> > perspective. There's not really a good reason the others can't be
> > accessed thru accessors though.
> >
> >> I'd like to spin the question around: if we're OK exposing some stuff
> >> (in your example above, the parent pointer), then what clk data are
> >> you trying to hide?
> >
> > Well, everything from drivers which the current clk implementations do
> > hide. Catching abuse in with drivers coming in from all different trees
> > and lists will be impossible.
> >
> > For platform code it is more fuzzy. I don't think platform code should
> > be allowed to muck with prepare/enable counts for example.
> 
> So there is a clear dichotomy: drivers shouldn't be exposed to any of
> it and platform code should be exposed to some of it.
> 
> How about a drivers/clk/clk-private.h which will define struct clk and
> must only be included by clk drivers in drivers/clk/*?
> 
> This establishes a bright line between those things which are allowed
> to know the details of struct clk and those that are not: namely that
> clk drivers in drivers/clk/ may use '#include "clk-private.h"'.
> Obviously struct clk is opaque to the rest of the kernel (in the same
> way it has been prior to the common clk patches) and there is no need
> for struct clk_hw anymore.  Also helper functions are no longer needed
> for clock driver code, which I think *is* a manageable set of code to
> review.  Also clk drivers must live in drivers/clk/ for this to work
> (without a big ugly path in someone's #include directive somewhere).
> 
> Thoughts?
> 
> Regards,
> Mike

Thomas? 

We're stuck on this fundamental point for a while now. And v5 of the
patchset doesn't make much sense without resolving it.

/Amit

Re: [PATCH v4 3/6] clk: introduce the common clock framework

[Saravana Kannan]

On 01/04/2012 08:07 PM, Turquette, Mike wrote:
> On Wed, Jan 4, 2012 at 6:11 PM, Rob Herring<robherring2@gmail.com>  wrote:
>> On 01/04/2012 07:01 PM, Turquette, Mike wrote:
>>> On Wed, Jan 4, 2012 at 6:32 AM, Rob Herring<robherring2@gmail.com>  wrote:
>>>> On 01/03/2012 08:15 PM, Richard Zhao wrote:
>>>>> On Fri, Dec 16, 2011 at 04:45:48PM -0800, Turquette, Mike wrote:
>>>>>> On Wed, Dec 14, 2011 at 5:18 AM, Thomas Gleixner<tglx@linutronix.de>  wrote:
>>>>>>> On Tue, 13 Dec 2011, Mike Turquette wrote:
>>>>
>>>> snip
>>>>
>>>>>>>> +/**
>>>>>>>> + * clk_init - initialize the data structures in a struct clk
>>>>>>>> + * @dev: device initializing this clk, placeholder for now
>>>>>>>> + * @clk: clk being initialized
>>>>>>>> + *
>>>>>>>> + * Initializes the lists in struct clk, queries the hardware for the
>>>>>>>> + * parent and rate and sets them both.  Adds the clk to the sysfs tree
>>>>>>>> + * topology.
>>>>>>>> + *
>>>>>>>> + * Caller must populate clk->name and clk->flags before calling
>>>>>>>
>>>>>>> I'm not too happy about this construct. That leaves struct clk and its
>>>>>>> members exposed to the world instead of making it a real opaque
>>>>>>> cookie. I know from my own painful experience, that this will lead to
>>>>>>> random fiddling in that data structure in drivers and arch code just
>>>>>>> because the core code has a shortcoming.
>>>>>>>
>>>>>>> Why can't we make struct clk a real cookie and confine the data
>>>>>>> structure to the core code ?
>>>>>>>
>>>>>>> That would change the init call to something like:
>>>>>>>
>>>>>>> struct clk *clk_init(struct device *dev, const struct clk_hw *hw,
>>>>>>>                      struct clk *parent)
>>>>>>>
>>>>>>> And have:
>>>>>>> struct clk_hw {
>>>>>>>        struct clk_hw_ops *ops;
>>>>>>>        const char        *name;
>>>>>>>        unsigned long     flags;
>>>>>>> };
>>>>>>>
>>>>>>> Implementers can do:
>>>>>>> struct my_clk_hw {
>>>>>>>        struct clk_hw    hw;
>>>>>>>        mydata;
>>>>>>> };
>>>>>>>
>>>>>>> And then change the clk ops callbacks to take struct clk_hw * as an
>>>>>>> argument.
>>>>> We have to define static clocks before we adopt DT binding.
>>>>> If clk is opaque and allocate memory in clk core, it'll make hard
>>>>> to define static clocks. And register/init will pass a long parameter
>>>>> list.
>>>>
>>>> DT is not a prerequisite for having dynamically created clocks. You can
>>>> make clock init dynamic without DT.
>>>
>>> Agreed.
>>>
>>>> What data goes in struct clk vs. struct clk_hw could change over time.
>>>> So perhaps we can start with some data in clk_hw and plan to move it to
>>>> struct clk later. Even if almost everything ends up in clk_hw initially,
>>>> at least the structure is in place to have common, core-only data
>>>> separate from platform data.
>>>
>>> What is the point of this?
>>
>> To have a way forward. It would be nice to have a clk infrastructure
>> before I retire...
>
> Haha, agreed.
>
>>
>>> The original clk_hw was defined simply as:
>>>
>>> struct clk_hw {
>>>          struct clk *clk;
>>> };
>>>
>>> It's only purpose in life was as a handle for navigation between the
>>> opaque struct clk and the hardware-specific struct my_clk_hw.  struct
>>> clk_hw is defined in clk.h and everyone can see it.  If we're suddenly
>>> OK putting clk data in this structure then why bother with an opaque
>>> struct clk at all?
>>>
>>>> What is the actual data you need to be static and accessible to the
>>>> platform code? A ptr to parent clk is the main thing I've seen for
>>>> static initialization. So make the parent ptr be struct clk_hw* and
>>>> allow the platforms to access.
>>>
>>> To answer your question on what data we're trying to expose: platform
>>> code commonly needs the parent pointer and the clk rate (and by
>>> extension, the rate of the parent).  For debug/error prints it is also
>>> nice to have the clk name.  Generic clk flags are also conceivably
>>> something that platform code might want.
>>
>> I agree with the need to have the parent and flags from a static init
>> perspective. There's not really a good reason the others can't be
>> accessed thru accessors though.
>>
>>> I'd like to spin the question around: if we're OK exposing some stuff
>>> (in your example above, the parent pointer), then what clk data are
>>> you trying to hide?
>>
>> Well, everything from drivers which the current clk implementations do
>> hide. Catching abuse in with drivers coming in from all different trees
>> and lists will be impossible.
>>
>> For platform code it is more fuzzy. I don't think platform code should
>> be allowed to muck with prepare/enable counts for example.
>
> So there is a clear dichotomy: drivers shouldn't be exposed to any of
> it and platform code should be exposed to some of it.
>
> How about a drivers/clk/clk-private.h which will define struct clk and
> must only be included by clk drivers in drivers/clk/*?
>
> This establishes a bright line between those things which are allowed
> to know the details of struct clk and those that are not: namely that
> clk drivers in drivers/clk/ may use '#include "clk-private.h"'.
> Obviously struct clk is opaque to the rest of the kernel (in the same
> way it has been prior to the common clk patches) and there is no need
> for struct clk_hw anymore.  Also helper functions are no longer needed
> for clock driver code, which I think *is* a manageable set of code to
> review.  Also clk drivers must live in drivers/clk/ for this to work
> (without a big ugly path in someone's #include directive somewhere).

While the original clk_hw suggestion was well intentioned, it just 
forces too many unnecessary dereferences and indirection. It also 
prevents static init of some fields as others have mentioned. Overall, 
it made the MSM clock code a mess when I tried to convert it to the 
common clock framework during Linaro Connect Q4 2011.

The current off-tree MSM clock code uses a very similar approach to what 
the original patches that Jeremy sent out did. When Mike sent out the 
patches that removed clk_hw, the MSM code was much clearer and easier to 
convert to the common clock framework.

The clk-private.h suggestion by Mike is reasonable seems like a good 
compromise. It support the idea of not letting the world peek into the 
clock struct (I want this too) while letting the clock driver use the 
struct without jumping through hoops. It has my vote (not the whole 
patch series, but the idea of having clk driver/framework specific stuff 
in clk-private.h). I really hope we move ahead with this.

Thanks,
Saravana

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH v4 3/6] clk: introduce the common clock framework

[Rob Herring]

On 01/12/2012 06:04 PM, Saravana Kannan wrote:
> On 01/04/2012 08:07 PM, Turquette, Mike wrote:
>> On Wed, Jan 4, 2012 at 6:11 PM, Rob Herring<robherring2@gmail.com> 
>> wrote:
>>> On 01/04/2012 07:01 PM, Turquette, Mike wrote:
>>>> On Wed, Jan 4, 2012 at 6:32 AM, Rob Herring<robherring2@gmail.com> 
>>>> wrote:
>>>>> On 01/03/2012 08:15 PM, Richard Zhao wrote:
>>>>>> On Fri, Dec 16, 2011 at 04:45:48PM -0800, Turquette, Mike wrote:
>>>>>>> On Wed, Dec 14, 2011 at 5:18 AM, Thomas
>>>>>>> Gleixner<tglx@linutronix.de>  wrote:
>>>>>>>> On Tue, 13 Dec 2011, Mike Turquette wrote:
>>>>>
>>>>> snip
>>>>>
>>>>>>>>> +/**
>>>>>>>>> + * clk_init - initialize the data structures in a struct clk
>>>>>>>>> + * @dev: device initializing this clk, placeholder for now
>>>>>>>>> + * @clk: clk being initialized
>>>>>>>>> + *
>>>>>>>>> + * Initializes the lists in struct clk, queries the hardware
>>>>>>>>> for the
>>>>>>>>> + * parent and rate and sets them both.  Adds the clk to the
>>>>>>>>> sysfs tree
>>>>>>>>> + * topology.
>>>>>>>>> + *
>>>>>>>>> + * Caller must populate clk->name and clk->flags before calling
>>>>>>>>
>>>>>>>> I'm not too happy about this construct. That leaves struct clk
>>>>>>>> and its
>>>>>>>> members exposed to the world instead of making it a real opaque
>>>>>>>> cookie. I know from my own painful experience, that this will
>>>>>>>> lead to
>>>>>>>> random fiddling in that data structure in drivers and arch code
>>>>>>>> just
>>>>>>>> because the core code has a shortcoming.
>>>>>>>>
>>>>>>>> Why can't we make struct clk a real cookie and confine the data
>>>>>>>> structure to the core code ?
>>>>>>>>
>>>>>>>> That would change the init call to something like:
>>>>>>>>
>>>>>>>> struct clk *clk_init(struct device *dev, const struct clk_hw *hw,
>>>>>>>>                      struct clk *parent)
>>>>>>>>
>>>>>>>> And have:
>>>>>>>> struct clk_hw {
>>>>>>>>        struct clk_hw_ops *ops;
>>>>>>>>        const char        *name;
>>>>>>>>        unsigned long     flags;
>>>>>>>> };
>>>>>>>>
>>>>>>>> Implementers can do:
>>>>>>>> struct my_clk_hw {
>>>>>>>>        struct clk_hw    hw;
>>>>>>>>        mydata;
>>>>>>>> };
>>>>>>>>
>>>>>>>> And then change the clk ops callbacks to take struct clk_hw * as an
>>>>>>>> argument.
>>>>>> We have to define static clocks before we adopt DT binding.
>>>>>> If clk is opaque and allocate memory in clk core, it'll make hard
>>>>>> to define static clocks. And register/init will pass a long parameter
>>>>>> list.
>>>>>
>>>>> DT is not a prerequisite for having dynamically created clocks. You
>>>>> can
>>>>> make clock init dynamic without DT.
>>>>
>>>> Agreed.
>>>>
>>>>> What data goes in struct clk vs. struct clk_hw could change over time.
>>>>> So perhaps we can start with some data in clk_hw and plan to move
>>>>> it to
>>>>> struct clk later. Even if almost everything ends up in clk_hw
>>>>> initially,
>>>>> at least the structure is in place to have common, core-only data
>>>>> separate from platform data.
>>>>
>>>> What is the point of this?
>>>
>>> To have a way forward. It would be nice to have a clk infrastructure
>>> before I retire...
>>
>> Haha, agreed.
>>
>>>
>>>> The original clk_hw was defined simply as:
>>>>
>>>> struct clk_hw {
>>>>          struct clk *clk;
>>>> };
>>>>
>>>> It's only purpose in life was as a handle for navigation between the
>>>> opaque struct clk and the hardware-specific struct my_clk_hw.  struct
>>>> clk_hw is defined in clk.h and everyone can see it.  If we're suddenly
>>>> OK putting clk data in this structure then why bother with an opaque
>>>> struct clk at all?
>>>>
>>>>> What is the actual data you need to be static and accessible to the
>>>>> platform code? A ptr to parent clk is the main thing I've seen for
>>>>> static initialization. So make the parent ptr be struct clk_hw* and
>>>>> allow the platforms to access.
>>>>
>>>> To answer your question on what data we're trying to expose: platform
>>>> code commonly needs the parent pointer and the clk rate (and by
>>>> extension, the rate of the parent).  For debug/error prints it is also
>>>> nice to have the clk name.  Generic clk flags are also conceivably
>>>> something that platform code might want.
>>>
>>> I agree with the need to have the parent and flags from a static init
>>> perspective. There's not really a good reason the others can't be
>>> accessed thru accessors though.
>>>
>>>> I'd like to spin the question around: if we're OK exposing some stuff
>>>> (in your example above, the parent pointer), then what clk data are
>>>> you trying to hide?
>>>
>>> Well, everything from drivers which the current clk implementations do
>>> hide. Catching abuse in with drivers coming in from all different trees
>>> and lists will be impossible.
>>>
>>> For platform code it is more fuzzy. I don't think platform code should
>>> be allowed to muck with prepare/enable counts for example.
>>
>> So there is a clear dichotomy: drivers shouldn't be exposed to any of
>> it and platform code should be exposed to some of it.
>>
>> How about a drivers/clk/clk-private.h which will define struct clk and
>> must only be included by clk drivers in drivers/clk/*?
>>
>> This establishes a bright line between those things which are allowed
>> to know the details of struct clk and those that are not: namely that
>> clk drivers in drivers/clk/ may use '#include "clk-private.h"'.
>> Obviously struct clk is opaque to the rest of the kernel (in the same
>> way it has been prior to the common clk patches) and there is no need
>> for struct clk_hw anymore.  Also helper functions are no longer needed
>> for clock driver code, which I think *is* a manageable set of code to
>> review.  Also clk drivers must live in drivers/clk/ for this to work
>> (without a big ugly path in someone's #include directive somewhere).
> 
> While the original clk_hw suggestion was well intentioned, it just
> forces too many unnecessary dereferences and indirection. It also
> prevents static init of some fields as others have mentioned. Overall,
> it made the MSM clock code a mess when I tried to convert it to the
> common clock framework during Linaro Connect Q4 2011.
> 
> The current off-tree MSM clock code uses a very similar approach to what
> the original patches that Jeremy sent out did. When Mike sent out the
> patches that removed clk_hw, the MSM code was much clearer and easier to
> convert to the common clock framework.
> 
> The clk-private.h suggestion by Mike is reasonable seems like a good
> compromise. It support the idea of not letting the world peek into the
> clock struct (I want this too) while letting the clock driver use the
> struct without jumping through hoops. It has my vote (not the whole
> patch series, but the idea of having clk driver/framework specific stuff
> in clk-private.h). I really hope we move ahead with this.
> 

I'm fine with this approach. We're certainly no worse off as platforms
today have full access. However, it not me that has to be convinced.

My suggestion was to build into the data structures at least the option
to have core only and core+platform data. Maybe the core only part is
mostly empty at first. This at least shows some intent to hide some of
the data. Which fields give you pain not having access to them? So far
this mainly seems to be parent and rate.

Rob

Re: [PATCH v4 3/6] clk: introduce the common clock framework

[Saravana Kannan]

On 01/12/2012 04:48 PM, Rob Herring wrote:
> On 01/12/2012 06:04 PM, Saravana Kannan wrote:
>> On 01/04/2012 08:07 PM, Turquette, Mike wrote:
>>> On Wed, Jan 4, 2012 at 6:11 PM, Rob Herring<robherring2@gmail.com>
>>> wrote:
>>>> On 01/04/2012 07:01 PM, Turquette, Mike wrote:
>>>>> On Wed, Jan 4, 2012 at 6:32 AM, Rob Herring<robherring2@gmail.com>
>>>>> wrote:
>>>>>> On 01/03/2012 08:15 PM, Richard Zhao wrote:
>>>>>>> On Fri, Dec 16, 2011 at 04:45:48PM -0800, Turquette, Mike wrote:
>>>>>>>> On Wed, Dec 14, 2011 at 5:18 AM, Thomas
>>>>>>>> Gleixner<tglx@linutronix.de>   wrote:
>>>>>>>>> On Tue, 13 Dec 2011, Mike Turquette wrote:
>>>>>>
>>>>>> snip
>>>>>>
>>>>>>>>>> +/**
>>>>>>>>>> + * clk_init - initialize the data structures in a struct clk
>>>>>>>>>> + * @dev: device initializing this clk, placeholder for now
>>>>>>>>>> + * @clk: clk being initialized
>>>>>>>>>> + *
>>>>>>>>>> + * Initializes the lists in struct clk, queries the hardware
>>>>>>>>>> for the
>>>>>>>>>> + * parent and rate and sets them both.  Adds the clk to the
>>>>>>>>>> sysfs tree
>>>>>>>>>> + * topology.
>>>>>>>>>> + *
>>>>>>>>>> + * Caller must populate clk->name and clk->flags before calling
>>>>>>>>>
>>>>>>>>> I'm not too happy about this construct. That leaves struct clk
>>>>>>>>> and its
>>>>>>>>> members exposed to the world instead of making it a real opaque
>>>>>>>>> cookie. I know from my own painful experience, that this will
>>>>>>>>> lead to
>>>>>>>>> random fiddling in that data structure in drivers and arch code
>>>>>>>>> just
>>>>>>>>> because the core code has a shortcoming.
>>>>>>>>>
>>>>>>>>> Why can't we make struct clk a real cookie and confine the data
>>>>>>>>> structure to the core code ?
>>>>>>>>>
>>>>>>>>> That would change the init call to something like:
>>>>>>>>>
>>>>>>>>> struct clk *clk_init(struct device *dev, const struct clk_hw *hw,
>>>>>>>>>                       struct clk *parent)
>>>>>>>>>
>>>>>>>>> And have:
>>>>>>>>> struct clk_hw {
>>>>>>>>>         struct clk_hw_ops *ops;
>>>>>>>>>         const char        *name;
>>>>>>>>>         unsigned long     flags;
>>>>>>>>> };
>>>>>>>>>
>>>>>>>>> Implementers can do:
>>>>>>>>> struct my_clk_hw {
>>>>>>>>>         struct clk_hw    hw;
>>>>>>>>>         mydata;
>>>>>>>>> };
>>>>>>>>>
>>>>>>>>> And then change the clk ops callbacks to take struct clk_hw * as an
>>>>>>>>> argument.
>>>>>>> We have to define static clocks before we adopt DT binding.
>>>>>>> If clk is opaque and allocate memory in clk core, it'll make hard
>>>>>>> to define static clocks. And register/init will pass a long parameter
>>>>>>> list.
>>>>>>
>>>>>> DT is not a prerequisite for having dynamically created clocks. You
>>>>>> can
>>>>>> make clock init dynamic without DT.
>>>>>
>>>>> Agreed.
>>>>>
>>>>>> What data goes in struct clk vs. struct clk_hw could change over time.
>>>>>> So perhaps we can start with some data in clk_hw and plan to move
>>>>>> it to
>>>>>> struct clk later. Even if almost everything ends up in clk_hw
>>>>>> initially,
>>>>>> at least the structure is in place to have common, core-only data
>>>>>> separate from platform data.
>>>>>
>>>>> What is the point of this?
>>>>
>>>> To have a way forward. It would be nice to have a clk infrastructure
>>>> before I retire...
>>>
>>> Haha, agreed.
>>>
>>>>
>>>>> The original clk_hw was defined simply as:
>>>>>
>>>>> struct clk_hw {
>>>>>           struct clk *clk;
>>>>> };
>>>>>
>>>>> It's only purpose in life was as a handle for navigation between the
>>>>> opaque struct clk and the hardware-specific struct my_clk_hw.  struct
>>>>> clk_hw is defined in clk.h and everyone can see it.  If we're suddenly
>>>>> OK putting clk data in this structure then why bother with an opaque
>>>>> struct clk at all?
>>>>>
>>>>>> What is the actual data you need to be static and accessible to the
>>>>>> platform code? A ptr to parent clk is the main thing I've seen for
>>>>>> static initialization. So make the parent ptr be struct clk_hw* and
>>>>>> allow the platforms to access.
>>>>>
>>>>> To answer your question on what data we're trying to expose: platform
>>>>> code commonly needs the parent pointer and the clk rate (and by
>>>>> extension, the rate of the parent).  For debug/error prints it is also
>>>>> nice to have the clk name.  Generic clk flags are also conceivably
>>>>> something that platform code might want.
>>>>
>>>> I agree with the need to have the parent and flags from a static init
>>>> perspective. There's not really a good reason the others can't be
>>>> accessed thru accessors though.
>>>>
>>>>> I'd like to spin the question around: if we're OK exposing some stuff
>>>>> (in your example above, the parent pointer), then what clk data are
>>>>> you trying to hide?
>>>>
>>>> Well, everything from drivers which the current clk implementations do
>>>> hide. Catching abuse in with drivers coming in from all different trees
>>>> and lists will be impossible.
>>>>
>>>> For platform code it is more fuzzy. I don't think platform code should
>>>> be allowed to muck with prepare/enable counts for example.
>>>
>>> So there is a clear dichotomy: drivers shouldn't be exposed to any of
>>> it and platform code should be exposed to some of it.
>>>
>>> How about a drivers/clk/clk-private.h which will define struct clk and
>>> must only be included by clk drivers in drivers/clk/*?
>>>
>>> This establishes a bright line between those things which are allowed
>>> to know the details of struct clk and those that are not: namely that
>>> clk drivers in drivers/clk/ may use '#include "clk-private.h"'.
>>> Obviously struct clk is opaque to the rest of the kernel (in the same
>>> way it has been prior to the common clk patches) and there is no need
>>> for struct clk_hw anymore.  Also helper functions are no longer needed
>>> for clock driver code, which I think *is* a manageable set of code to
>>> review.  Also clk drivers must live in drivers/clk/ for this to work
>>> (without a big ugly path in someone's #include directive somewhere).
>>
>> While the original clk_hw suggestion was well intentioned, it just
>> forces too many unnecessary dereferences and indirection. It also
>> prevents static init of some fields as others have mentioned. Overall,
>> it made the MSM clock code a mess when I tried to convert it to the
>> common clock framework during Linaro Connect Q4 2011.
>>
>> The current off-tree MSM clock code uses a very similar approach to what
>> the original patches that Jeremy sent out did. When Mike sent out the
>> patches that removed clk_hw, the MSM code was much clearer and easier to
>> convert to the common clock framework.
>>
>> The clk-private.h suggestion by Mike is reasonable seems like a good
>> compromise. It support the idea of not letting the world peek into the
>> clock struct (I want this too) while letting the clock driver use the
>> struct without jumping through hoops. It has my vote (not the whole
>> patch series, but the idea of having clk driver/framework specific stuff
>> in clk-private.h). I really hope we move ahead with this.
>>
>
> I'm fine with this approach. We're certainly no worse off as platforms
> today have full access. However, it not me that has to be convinced.

Sorry for not being clear. My previous mail was a general comment to the 
community and not directed specifically at you Rob.

> My suggestion was to build into the data structures at least the option
> to have core only and core+platform data. Maybe the core only part is
> mostly empty at first. This at least shows some intent to hide some of
> the data. Which fields give you pain not having access to them? So far
> this mainly seems to be parent and rate.

It's been a while, but if I'm not mistaken, it was messy to statically 
initialize the parent field and to tie up the clock specific struct 
(say, clk_fixed) to the generic clock struct (clk) without having to 
define each of them separately.

With the clk-private.h approach, I could do something like this:

static struct fixed_clk cxo_clk = {
	.reg = 0x12345678
         .c = {
                .dbg_name = "cxo_clk",
                .ops = &clk_ops_cxo,
                CLK_INIT(cxo_clk.c),
        },
}

And without it, it would look something like:

struct clk _cxo_clk;

static struct msm_fixed_clk cxo_clk = {
	.reg = 0x12345678
	.c.clk = &_cxo_clk,
};

struct clk _cxo_clk = {
        .name = "cxo_clk",
        .ops = &clk_ops_cxo,
        .hw = &cxo_clk.c
};

As you can see, I now have to give a name for a struct that I don't 
really care about after init and pollute the name space. It's also 
clumsy since both the structs try to reference each other and I have to 
use forward declaration for every single clock I try to statically 
initialize.

I will also need to do several pointer derefs instead of using 
container_of(), etc which is more efficient and less confusing than 
multiple levels of pointer deref.

Thanks,
Saravana

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH v4 3/6] clk: introduce the common clock framework

[Shawn Guo]

On Thu, Jan 12, 2012 at 04:04:23PM -0800, Saravana Kannan wrote:
> While the original clk_hw suggestion was well intentioned, it just
> forces too many unnecessary dereferences and indirection. It also
> prevents static init of some fields as others have mentioned.
> Overall, it made the MSM clock code a mess when I tried to convert
> it to the common clock framework during Linaro Connect Q4 2011.
> 
> The current off-tree MSM clock code uses a very similar approach to
> what the original patches that Jeremy sent out did. When Mike sent
> out the patches that removed clk_hw, the MSM code was much clearer
> and easier to convert to the common clock framework.
> 
I share the same feeling with migrating imx6 clock support to v2 and
v3 of this series.  (v2 uses clk_hw, and v3 removes clk_hw.)

-- 
Regards,
Shawn

Re: [PATCH v4 3/6] clk: introduce the common clock framework

[Saravana Kannan]

On 12/17/2011 03:04 AM, Russell King - ARM Linux wrote:
> On Fri, Dec 16, 2011 at 04:45:48PM -0800, Turquette, Mike wrote:
>> On Wed, Dec 14, 2011 at 5:18 AM, Thomas Gleixner<tglx@linutronix.de>  wrote:
>>> On Tue, 13 Dec 2011, Mike Turquette wrote:
>>>> +void __clk_unprepare(struct clk *clk)
>>>> +{
>>>> +     if (!clk)
>>>> +             return;
>>>> +
>>>> +     if (WARN_ON(clk->prepare_count == 0))
>>>> +             return;
>>>> +
>>>> +     if (--clk->prepare_count>  0)
>>>> +             return;
>>>> +
>>>> +     WARN_ON(clk->enable_count>  0);
>>>
>>> So this leaves the clock enable count set. I'm a bit wary about
>>> that. Shouldn't it either return (including bumping the prepare_count
>>> again) or call clk_disable() ?
>
> No it should not.
>
>> I've hit this in my port of OMAP.  It comes from this simple situation:
>>
>> driver 1 (adapted for clk_prepare/clk_unprepare):
>> clk_prepare(clk);
>> clk_enable(clk);
>>
>> ...
>>
>> driver2 (not adapted for clk_prepare/clk_unprepare):
>> clk_enable(clk);
>
> So this is basically buggy.  Look, it's quite simple.  Convert _all_
> your drivers to clk_prepare/clk_unprepare _before_ you start switching
> your platform to use these new functions.  You can do that _today_
> without exception.
>
> We must refuse to merge _any_ user which does this the old way - and
> we should have been doing this since my commit was merged into mainline
> to allow drivers to be converted.
>
> And stop trying to think of ways around this inside clk_prepare/
> clk_unprepare/clk_enable/clk_disable.  You can't do it.  Just fix _all_
> the drivers.  Now.  Before you start implementing clk_prepare/clk_unprepare.

I agree with Russell's suggestion. This is what I'm trying to do with 
the MSM platform. Not sure if I'm too optimistic, but as of today, I'm 
still optimistic I can push the MSM driver devs to get this done before 
we enable real prepare/unprepare support.

Thanks,
Saravana

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH v4 3/6] clk: introduce the common clock framework

[Turquette, Mike]

On Fri, Jan 13, 2012 at 8:18 PM, Saravana Kannan <skannan@codeaurora.org> wrote:
> On 12/17/2011 03:04 AM, Russell King - ARM Linux wrote:
>>
>> On Fri, Dec 16, 2011 at 04:45:48PM -0800, Turquette, Mike wrote:
>>>
>>> On Wed, Dec 14, 2011 at 5:18 AM, Thomas Gleixner<tglx@linutronix.de>
>>>  wrote:
>>>>
>>>> On Tue, 13 Dec 2011, Mike Turquette wrote:
>>>>>
>>>>> +void __clk_unprepare(struct clk *clk)
>>>>> +{
>>>>> +     if (!clk)
>>>>> +             return;
>>>>> +
>>>>> +     if (WARN_ON(clk->prepare_count == 0))
>>>>> +             return;
>>>>> +
>>>>> +     if (--clk->prepare_count>  0)
>>>>> +             return;
>>>>> +
>>>>> +     WARN_ON(clk->enable_count>  0);
>>>>
>>>>
>>>> So this leaves the clock enable count set. I'm a bit wary about
>>>> that. Shouldn't it either return (including bumping the prepare_count
>>>> again) or call clk_disable() ?
>>
>>
>> No it should not.
>>
>>> I've hit this in my port of OMAP.  It comes from this simple situation:
>>>
>>> driver 1 (adapted for clk_prepare/clk_unprepare):
>>> clk_prepare(clk);
>>> clk_enable(clk);
>>>
>>> ...
>>>
>>> driver2 (not adapted for clk_prepare/clk_unprepare):
>>> clk_enable(clk);
>>
>>
>> So this is basically buggy.  Look, it's quite simple.  Convert _all_
>> your drivers to clk_prepare/clk_unprepare _before_ you start switching
>> your platform to use these new functions.  You can do that _today_
>> without exception.
>>
>> We must refuse to merge _any_ user which does this the old way - and
>> we should have been doing this since my commit was merged into mainline
>> to allow drivers to be converted.
>>
>> And stop trying to think of ways around this inside clk_prepare/
>> clk_unprepare/clk_enable/clk_disable.  You can't do it.  Just fix _all_
>> the drivers.  Now.  Before you start implementing
>> clk_prepare/clk_unprepare.
>
>
> I agree with Russell's suggestion. This is what I'm trying to do with the
> MSM platform. Not sure if I'm too optimistic, but as of today, I'm still
> optimistic I can push the MSM driver devs to get this done before we enable
> real prepare/unprepare support.

Just to reach closure on this topic: I don't plan to change
__clk_unprepare in the next version of the patches.  The warnings are
doing a fine job of catching code which has yet to be properly
converted to use clk_(un)prepare.

Mike

>
>
> Thanks,
> Saravana
>
> --
> Sent by an employee of the Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH v4 3/6] clk: introduce the common clock framework

[Saravana Kannan]

On 01/13/2012 08:39 PM, Turquette, Mike wrote:
> On Fri, Jan 13, 2012 at 8:18 PM, Saravana Kannan<skannan@codeaurora.org>  wrote:
>> On 12/17/2011 03:04 AM, Russell King - ARM Linux wrote:
>>>
>>> On Fri, Dec 16, 2011 at 04:45:48PM -0800, Turquette, Mike wrote:
>>>>
>>>> On Wed, Dec 14, 2011 at 5:18 AM, Thomas Gleixner<tglx@linutronix.de>
>>>>   wrote:
>>>>>
>>>>> On Tue, 13 Dec 2011, Mike Turquette wrote:
>>>>>>
>>>>>> +void __clk_unprepare(struct clk *clk)
>>>>>> +{
>>>>>> +     if (!clk)
>>>>>> +             return;
>>>>>> +
>>>>>> +     if (WARN_ON(clk->prepare_count == 0))
>>>>>> +             return;
>>>>>> +
>>>>>> +     if (--clk->prepare_count>    0)
>>>>>> +             return;
>>>>>> +
>>>>>> +     WARN_ON(clk->enable_count>    0);
>>>>>
>>>>>
>>>>> So this leaves the clock enable count set. I'm a bit wary about
>>>>> that. Shouldn't it either return (including bumping the prepare_count
>>>>> again) or call clk_disable() ?
>>>
>>>
>>> No it should not.
>>>
>>>> I've hit this in my port of OMAP.  It comes from this simple situation:
>>>>
>>>> driver 1 (adapted for clk_prepare/clk_unprepare):
>>>> clk_prepare(clk);
>>>> clk_enable(clk);
>>>>
>>>> ...
>>>>
>>>> driver2 (not adapted for clk_prepare/clk_unprepare):
>>>> clk_enable(clk);
>>>
>>>
>>> So this is basically buggy.  Look, it's quite simple.  Convert _all_
>>> your drivers to clk_prepare/clk_unprepare _before_ you start switching
>>> your platform to use these new functions.  You can do that _today_
>>> without exception.
>>>
>>> We must refuse to merge _any_ user which does this the old way - and
>>> we should have been doing this since my commit was merged into mainline
>>> to allow drivers to be converted.
>>>
>>> And stop trying to think of ways around this inside clk_prepare/
>>> clk_unprepare/clk_enable/clk_disable.  You can't do it.  Just fix _all_
>>> the drivers.  Now.  Before you start implementing
>>> clk_prepare/clk_unprepare.
>>
>>
>> I agree with Russell's suggestion. This is what I'm trying to do with the
>> MSM platform. Not sure if I'm too optimistic, but as of today, I'm still
>> optimistic I can push the MSM driver devs to get this done before we enable
>> real prepare/unprepare support.
>
> Just to reach closure on this topic: I don't plan to change
> __clk_unprepare in the next version of the patches.  The warnings are
> doing a fine job of catching code which has yet to be properly
> converted to use clk_(un)prepare.

To be fair, I also have to improve the stub clk_prepare/unprepare to 
maintain ref counts and do refcount checking before I plan to cut off to 
the real prepare/unprepare implementations. So, I'm guessing Mike is 
just trying to partly add that support in this patch series.

My goal is to have MSM converted fully before switching to this. So, 
this code that we are debating about won't directly impact MSM. For that 
reason, I won't be trying to hold off the more important common clock 
framework due to unconventional error handling.

Thanks,
Saravana

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.