[PATCH v3 0/5] common clk framework

[PATCH v3 0/5] common clk framework

[Mike Turquette]

Hi all,

A quick refresher: the clock framework APIs in include/linux/clk.h have
allowed platforms to develop their own platform-specific implementations
to manage clocks; this meant that everyone had their own definition of
struct clk, duplicated much code and contributed negatively to the
on-going quest for The One Image to Rule Them All.

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
well-defined set of operations against.

These five patches are the next iteration of the common clk framework.
Since the V2 submission back in late September I ported the OMAP4
portion of OMAP's platform-specific clk framework and actively developed
the generic code on a Panda board which revealed many bugs in V2.

The patches are based on Linus' v3.2-rc1 tag and can be pulled from:
git://git.linaro.org/people/mturquette/linux.git
http://git.linaro.org/gitweb?p=people/mturquette/linux.git;a=shortlog;h=refs/heads/v3.2-rc1-clkv3

A great deal of this work was first done by Jeremy Kerr, who in turn
based his patches off of work by Ben Herrenschmidt
(https://lkml.org/lkml/2011/5/20/81).  Many others contributed to those
patches and promptly had their work stolen by me.  Thanks to all for
their past contributions.

What to expect in this version:

  .the most notable change is the removal of struct clk_hw.  This extra
layer of abstraction is only necessary if we want hide the definition of
struct clk from platform code.  Many developers expressed the need to
know some details of the generic struct clk in the platform layer, and
rightly so.  Now struct clk is defined in include/linux/clk.h, protected
by #ifdef CONFIG_GENERIC_CLK.

  .flags have been introduced to struct clk, with several of them
defined and used in the common code.  These flags protect against
changing clk rates or switching the clk parent while that clk is
enabled; another flag is used to signal to clk_set_rate that it should
ask the parent to change it's rate too.

  .speaking of which, clk_set_rate has been overhauled and is now
recursive. *collective groan*.  clk_set_rate is still simple for the
common case of simply setting a single clk's rate.  But if your clk has
the CLK_PARENT_SET_RATE flag and the .round_rate callback recommends
changing the parent rate, then clk_set_rate will recurse upwards to the
parent and try it all over again.  In the event of a failure everything
unwinds and all the clks go out for drinks.

  .clk_register has been replaced by clk_init, which does NOT allocate
memory for you.  Platforms should allocate their own clk_hw_whatever
structure which contains struct clk.  clk_init is still necessary to
initialize struct clk internals.  clk_init also accepts struct device
*dev as an argument, but does nothing with it.  This is in anticipation
of device tree support.

  .Documentation!  I'm sure somebody reads it.

  .sysfs support.  Visualize your clk tree at /sys/clk!  Where would be
a better place to put the clk tree besides the root of /sys/?  When a
consensus on this is reached I'll submit the proper changes to
Documentation/ABI/testing/.

What's missing?

  .per tree locking.  I implemented this at the Linaro Connect
conference but the implementation was unpopular, so it didn't make the
cut.  There needs to be better understanding of everyone's needs for
this to work.

  .rate change notifications.  I simply didn't want to delay getting
these patches to the list any longer, so the notifiers didn't make it
in.  I'll submit them to the list soon, or roll them into the V4
patchset.  There are comments in the clk API definitions for where
PRECHANGE, POSTCHANGE and ABORT propagation will go.

  .basic mux clk, divider and dummy clk implementations.  I think others
have some code lying around to implement these, so I left them out.

  .device tree support.  I haven't looked much at the on-going
discussions on the dt clk bindings.  How compatible (or not) are the
device tree clk bindings and the way these patches want to initialize
clks?

  .what is the overlap between common clk and clkdev?  We're essentially
tracking the clks in two places (common clk's tree and clkdevs's list),
which feels a bit wasteful.

What else?

  .OMAP4 support will be posted to LOML and LAKML in a separate
patchset, since others might be interested in seeing a full port.  It is
a total hack, and is not ready for a formal submission.

Mike Turquette (5):
  clk: Kconfig: add entry for HAVE_CLK_PREPARE
  Documentation: common clk API
  clk: introduce the common clock framework
  clk: basic gateable and fixed-rate clks
  clk: export tree topology and clk data via sysfs

 Documentation/clk.txt   |  312 +++++++++++++++++++++++++++
 drivers/clk/Kconfig     |   24 ++
 drivers/clk/Makefile    |    5 +-
 drivers/clk/clk-basic.c |  208 ++++++++++++++++++
 drivers/clk/clk-sysfs.c |  199 +++++++++++++++++
 drivers/clk/clk.c       |  541 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/clk.h     |  199 +++++++++++++++++-
 7 files changed, 1484 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/clk.txt
 create mode 100644 drivers/clk/clk-basic.c
 create mode 100644 drivers/clk/clk-sysfs.c
 create mode 100644 drivers/clk/clk.c

-- 
1.7.4.1

[PATCH v3 1/5] 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>
---
 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.4.1

[PATCH v3 2/5] 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>
---
 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..ef4333d
--- /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_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;
+};
+
+The .name, .parent and .children members make up the core of the clk
+tree topology which can be visualized by enabling
+CONFIG_COMMON_CLK_SYSFS.  The ops member points to an instance of struct
+clk_hw_ops:
+
+	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);
+	};
+
+These callbacks correspond to the clk API that has existed in
+include/linux/clk.h for a while.  Below is a quick summary of the
+operations in that header, as implemented in drivers/clk/clk.c.  These
+comprise the driver-facing API:
+
+clk_prepare - does everything needed to get a clock ready to generate a
+proper signal which may include ungating the clk and actually generating
+that signal.  clk_prepare MUST be called before clk_enable.  This call
+holds the global prepare_mutex, which also prevents clk rates and
+topology from changing while held.  This API is meant to be the "slow"
+part of a clk enable sequence, if applicable.  This function must not be
+called in an interrupt context.
+
+clk_unprepare - the opposite of clk_prepare: does everything needed to
+make a clk no longer ready to generate a proper signal, which may
+include gating an active clk.  clk_disable must be called before
+clk_unprepare.  All of the same rules for clk_prepare apply.
+
+clk_enable - ungate a clock and immediately start generating a valid clk
+signal.  This is the "fast" part of a clk enable sequence, and maybe the
+only functional part of that sequence.  Regardless clk_prepare must be
+called BEFORE clk_enable.  The enable_spinlock is held across this call,
+which means that clk_enable must not sleep.
+
+clk_disable - the opposite of clk_enable: gates a clock immediately.
+clk_disable must be called before calling clk_unprepare.  All of the
+same rules for clk_enable apply.
+
+clk_get_rate - Returns the cached rate for the clk.  Does NOT query the
+hardware state.  No lock is held.
+
+clk_get_parent - Returns the cached parent for the clk.  Does NOT query
+the hardware state.  No lock is held.
+
+clk_set_rate - Attempts to change the clk rate to the one specified.
+Depending on a variety of common flags it may fail to maintain system
+stability or result in a variety of other clk rates changing.  Holds the
+same prepare_mutex held by clk_prepare/clk_unprepare and clk_set_parent.
+
+clk_set_parent - Switches the input source for a clk.  This only applies
+to mux clks with multiple parents.  Holds the same prepare_mutex held by
+clk_prepare/clk_unprepare and clk_set_rate.
+
+	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_hw_gate {
+	struct clk      clk;
+	struct clk      *fixed_parent;
+	void __iomem    *reg;
+	u8              bit_idx;
+};
+
+struct clk_hw_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_hw_gate_enable_set(clk);
+				clk_hw_gate_set_bit(clk);
+
+And the definition of clk_hw_gate_set_bit:
+
+static void clk_hw_gate_set_bit(struct clk *clk)
+{
+	struct clk_hw_gate *gate = to_clk_hw_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_hw_gate_set_bit there is use of
+to_clk_hw_gate, which is defined as:
+
+#define to_clk_hw_gate(ck) container_of(ck, struct clk_hw_gate, clk)
+
+This simple abstration 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_hw_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_hw_ops clk_hw_ops_your_clk {
+	.enable		= &clk_hw_your_clk_enable;
+	.disable	= &clk_hw_your_clk_disable;
+};
+
+Implement the above functions using container_of:
+
+int clk_hw_your_clk_enable(struct clk *clk)
+{
+	struct clk_hw_your_clk *yclk;
+
+	yclk = container_of(clk, struct clk_hw_your_clk, clk);
+
+	magic(yclk);
+};
+
+If you are statically allocating all of your clk_hw_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_hw_your_clk_register(const char *name, unsigned long flags,
+		unsigned long some_data,
+		struct your_struct *some_more_data)
+{
+	struct clk_hw_your_clk *yclk;
+
+	yclk = kmalloc(sizeof(struct clk_hw_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
+
+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
+2) 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.4.1

[PATCH v3 3/5] 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_hw_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_hw_ops.

See Documentation/clk.h 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>
---
 drivers/clk/Kconfig  |    4 +
 drivers/clk/Makefile |    1 +
 drivers/clk/clk.c    |  538 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/clk.h  |  134 ++++++++++++-
 4 files changed, 673 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..12c9994
--- /dev/null
+++ b/drivers/clk/clk.c
@@ -0,0 +1,538 @@
+/*
+ * 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/slab.h>
+#include <linux/spinlock.h>
+#include <linux/err.h>
+
+static DEFINE_SPINLOCK(enable_lock);
+static DEFINE_MUTEX(prepare_lock);
+
+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 slow 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)
+{
+	if (!clk)
+		return 0;
+
+	return clk->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)
+{
+	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);
+
+	if (old_rate == clk->rate)
+		return;
+
+	/* 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;
+	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 */
+	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.
+	 */
+	clk->rate = new_rate;
+	/* FIXME propagate post-rate change notifier for only this clk */
+	hlist_for_each_entry(child, tmp, &clk->children, child_node)
+		clk_recalc_rates(child);
+
+	return fail_clk;
+}
+
+/**
+ * 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;
+
+	if (!clk->ops->set_rate)
+		return -ENOSYS;
+
+	/* bail early if nothing to do */
+	if (rate == clk->rate)
+		return rate;
+
+	/* prevent racing with updates to the clock topology */
+	mutex_lock(&prepare_lock);
+	fail_clk = __clk_set_rate(clk, rate);
+	if (fail_clk) {
+		pr_warn("%s: failed to set %s rate to %lu\n",
+				__func__, fail_clk->name, rate);
+		/* FIXME propagate rate change abort notification here */
+		ret = -EIO;
+	}
+	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)
+{
+	if (!clk)
+		return NULL;
+
+	return clk->parent;
+}
+EXPORT_SYMBOL_GPL(clk_get_parent);
+
+void __clk_reparent(struct clk *clk, struct clk *new_parent)
+{
+	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;
+
+	if (clk->parent == parent)
+		return 0;
+
+	/* prevent racing with updates to the clock topology */
+	mutex_lock(&prepare_lock);
+
+	/* 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);
+
+	if (clk->ops->get_parent) {
+		clk->parent = clk->ops->get_parent(clk);
+		if (clk->parent)
+			hlist_add_head(&clk->child_node,
+					&clk->parent->children);
+	} else
+		clk->parent = NULL;
+
+	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..3b0eb3f 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
@@ -13,17 +15,134 @@
 
 #include <linux/kernel.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);
+
+#endif /* !CONFIG_GENERIC_CLK */
 
 /**
  * clk_get - lookup and obtain a reference to a clock producer.
@@ -107,9 +226,16 @@ static inline void clk_unprepare(struct clk *clk)
 }
 #endif
 
+int __clk_enable(struct clk *clk);
+void __clk_disable(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);
+
 /**
  * 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.4.1

[PATCH v3 4/5] 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 contribution by Jamie Iles.

Signed-off-by: Mike Turquette <mturquette@linaro.org>
---
 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..c039f94
--- /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 = __raw_readl(gate->reg);
+	reg |= BIT(gate->bit_idx);
+	__raw_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 = __raw_readl(gate->reg);
+	reg &= ~BIT(gate->bit_idx);
+	__raw_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 3b0eb3f..8ed354a 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -129,6 +129,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.4.1

[PATCH v3 5/5] clk: export tree topology and clk data via sysfs

[Mike Turquette]

Introduces kobject support for the common struct clk, exports per-clk
data via read-only callbacks and models the clk tree topology in sysfs.

Also adds support for generating the clk tree in clk_init and migrating
nodes when input sources are switches in clk_set_parent.

Signed-off-by: Mike Turquette <mturquette@linaro.org>
---
 drivers/clk/Kconfig     |   10 +++
 drivers/clk/Makefile    |    1 +
 drivers/clk/clk-sysfs.c |  199 +++++++++++++++++++++++++++++++++++++++++++++++
 drivers/clk/clk.c       |    5 +-
 include/linux/clk.h     |   36 ++++++++-
 5 files changed, 248 insertions(+), 3 deletions(-)
 create mode 100644 drivers/clk/clk-sysfs.c

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index ba7eb8c..8f8e7ac 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -19,3 +19,13 @@ 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_SYSFS
+	bool "Clock tree topology and debug info"
+	depends on EXPERIMENTAL && GENERIC_CLK
+	help
+	   Creates clock tree represenation in sysfs.  Directory names
+	   and hierarchy represent clock names and tree structure,
+	   respectively.  Each directory exports clock rate, flags,
+	   prepare_count and enable_count info as read-only for debug
+	   purposes.
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 68b20a1..806a9999 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -2,3 +2,4 @@
 obj-$(CONFIG_CLKDEV_LOOKUP)		+= clkdev.o
 obj-$(CONFIG_GENERIC_CLK)		+= clk.o
 obj-$(CONFIG_GENERIC_CLK_BASIC)		+= clk-basic.o
+obj-$(CONFIG_GENERIC_CLK_SYSFS)		+= clk-sysfs.o
diff --git a/drivers/clk/clk-sysfs.c b/drivers/clk/clk-sysfs.c
new file mode 100644
index 0000000..8ccf9e3
--- /dev/null
+++ b/drivers/clk/clk-sysfs.c
@@ -0,0 +1,199 @@
+/*
+ * 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.
+ *
+ * Clock tree topology and debug info for the common clock framework
+ */
+
+#include <linux/clk.h>
+#include <linux/kobject.h>
+#include <linux/sysfs.h>
+#include <linux/err.h>
+
+#define MAX_STRING_LENGTH	32
+
+static struct kobject *clk_kobj;
+LIST_HEAD(kobj_list);
+
+struct clk_attribute {
+	struct attribute attr;
+	ssize_t (*show)(struct clk *clk, char *buf);
+};
+
+static ssize_t clk_rate_show(struct clk *clk, char *buf)
+{
+	if (IS_ERR_OR_NULL(clk))
+		return -ENODEV;
+
+	return snprintf(buf, MAX_STRING_LENGTH, "%lu\n", clk->rate);
+}
+
+static ssize_t clk_flags_show(struct clk *clk, char *buf)
+{
+	if (IS_ERR_OR_NULL(clk))
+		return -ENODEV;
+
+	return snprintf(buf, MAX_STRING_LENGTH, "0x%lX\n", clk->flags);
+}
+
+static ssize_t clk_prepare_count_show(struct clk *clk, char *buf)
+{
+	if (IS_ERR_OR_NULL(clk))
+		return -ENODEV;
+
+	return snprintf(buf, MAX_STRING_LENGTH, "%d\n", clk->prepare_count);
+}
+
+static ssize_t clk_enable_count_show(struct clk *clk, char *buf)
+{
+	if (IS_ERR_OR_NULL(clk))
+		return -ENODEV;
+
+	return snprintf(buf, MAX_STRING_LENGTH, "%d\n", clk->enable_count);
+}
+
+static ssize_t clk_show(struct kobject *kobj, struct attribute *attr,
+		char *buf)
+{
+	struct clk *clk;
+	struct clk_attribute *clk_attr;
+	ssize_t ret = -EINVAL;
+
+	clk = container_of(kobj, struct clk, kobj);
+	clk_attr = container_of(attr, struct clk_attribute, attr);
+
+	if (!clk || !clk_attr)
+		goto out;
+
+	/* we don't do any locking for debug operations */
+
+	/* refcount++ */
+	kobject_get(&clk->kobj);
+
+	if (clk_attr->show)
+		ret = clk_attr->show(clk, buf);
+	else
+		ret = -EIO;
+
+	/* refcount-- */
+	kobject_put(&clk->kobj);
+
+out:
+	return ret;
+}
+
+static struct clk_attribute clk_rate = __ATTR_RO(clk_rate);
+static struct clk_attribute clk_flags = __ATTR_RO(clk_flags);
+static struct clk_attribute clk_prepare_count = __ATTR_RO(clk_prepare_count);
+static struct clk_attribute clk_enable_count = __ATTR_RO(clk_enable_count);
+
+static struct attribute *clk_default_attrs[] = {
+	&clk_rate.attr,
+	&clk_flags.attr,
+	&clk_prepare_count.attr,
+	&clk_enable_count.attr,
+	NULL,
+};
+
+static const struct sysfs_ops clk_ops = {
+	.show	= clk_show,
+};
+
+static void clk_release(struct kobject *kobj)
+{
+	struct clk *clk;
+
+	clk = container_of(kobj, struct clk, kobj);
+
+	complete(&clk->kobj_unregister);
+}
+
+static struct kobj_type clk_ktype = {
+	.sysfs_ops	= &clk_ops,
+	.default_attrs	= clk_default_attrs,
+	.release	= clk_release,
+};
+
+int clk_kobj_add(struct clk *clk)
+{
+	if (IS_ERR(clk))
+		return -EINVAL;
+
+	/*
+	 * Some kobject trickery!
+	 *
+	 * We want to (ab)use the kobject infrastructure to track our
+	 * tree topology for us, specifically the root clocks (which are
+	 * otherwise not remembered in a global list).
+	 *
+	 * Unfortunately we might not be able to allocate memory yet
+	 * when this path is hit.  This pretty much rules out anything
+	 * that looks or smells like kobject_add, since there are
+	 * allocations for kobject->name and a dependency on sysfs being
+	 * initialized.
+	 *
+	 * To get around this we initialize the kobjects and (ab)use
+	 * struct kobject's list_head member, "entry".  Later on we walk
+	 * this list in clk_sysfs_tree_create() to make proper
+	 * kobject_add calls once it is safe to do so.
+	 *
+	 * FIXME - this is starting to smell alot like clkdev (i.e.
+	 * tracking the clocks in a list)
+	 */
+
+	kobject_init(&clk->kobj, &clk_ktype);
+	list_add_tail(&clk->kobj.entry, &kobj_list);
+	return 0;
+}
+
+int clk_kobj_reparent(struct clk *clk, struct clk *parent)
+{
+	int ret;
+
+	if (!clk || !parent)
+		return -EINVAL;
+
+	ret = kobject_move(&clk->kobj, &parent->kobj);
+	if (ret)
+		pr_warning("%s: failed to reparent %s to %s in sysfs\n",
+				__func__, clk->name, parent->name);
+
+	return ret;
+}
+
+static int __init clk_sysfs_init(void)
+{
+	struct list_head *tmp;
+
+	clk_kobj = kobject_create_and_add("clk", NULL);
+
+	WARN_ON(!clk_kobj);
+
+	list_for_each(tmp, &kobj_list) {
+		struct kobject *kobj;
+		struct clk *clk;
+		struct kobject *parent_kobj = NULL;
+		int ret;
+
+		kobj = container_of(tmp, struct kobject, entry);
+
+		clk = container_of(kobj, struct clk, kobj);
+
+		/* assumes list is ordered */
+		if (clk->parent)
+			parent_kobj = &clk->parent->kobj;
+		else
+			parent_kobj = clk_kobj;
+
+		ret = kobject_add(kobj, parent_kobj, clk->name);
+		if (ret)
+			pr_warning("%s: failed to create sysfs entry for %s\n",
+					__func__, clk->name);
+	}
+
+	return 0;
+}
+late_initcall(clk_sysfs_init);
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 12c9994..85dabdb 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -436,7 +436,8 @@ void __clk_reparent(struct clk *clk, struct clk *new_parent)
 
 	clk->parent = new_parent;
 
-	/* FIXME update sysfs clock topology */
+	/* update sysfs clock topology */
+	clk_kobj_reparent(clk, clk->parent);
 }
 
 /**
@@ -531,6 +532,8 @@ void clk_init(struct device *dev, struct clk *clk)
 	else
 		clk->rate = 0;
 
+	clk_kobj_add(clk);
+
 	mutex_unlock(&prepare_lock);
 
 	return;
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 8ed354a..99337ca 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -14,8 +14,8 @@
 #define __LINUX_CLK_H
 
 #include <linux/kernel.h>
-
-#include <linux/kernel.h>
+#include <linux/kobject.h>
+#include <linux/completion.h>
 #include <linux/errno.h>
 
 struct device;
@@ -46,6 +46,10 @@ struct clk {
 	unsigned int		prepare_count;
 	struct hlist_head	children;
 	struct hlist_node	child_node;
+#ifdef CONFIG_GENERIC_CLK_SYSFS
+	struct kobject		kobj;
+	struct completion	kobj_unregister;
+#endif
 };
 
 /**
@@ -177,6 +181,34 @@ int clk_register_gate(struct device *dev, const char *name, unsigned long flags,
  */
 void clk_init(struct device *dev, struct clk *clk);
 
+#ifdef CONFIG_GENERIC_CLK_SYSFS
+/**
+ * clk_kobj_add - create a clk entry in sysfs
+ * @clk: clk to model in sysfs
+ *
+ * Create a directory in sysfs with the same name as clk.  Also creates
+ * read-only entries for the common struct clk members (rate, flags,
+ * prepare_count & enable_count).  The topology of the tree is
+ * represented by the sysfs directory structure itself.
+ */
+int clk_kobj_add(struct clk *clk);
+
+/**
+ * clk_kobj_reparent - reparent a clk entry in sysfs
+ * @clk: the child clk that is switching parents
+ * @parent: the new parent clk
+ *
+ * Simple call to kobject_move to keep sysfs up to date with the
+ * hardware clock topology
+ */
+int clk_kobj_reparent(struct clk *clk, struct clk *parent);
+#else
+static inline int clk_kobj_add(struct clk *clk)
+{ return 0; }
+static inline int clk_kobj_reparent(struct clk *clk, struct clk *parent)
+{ return 0; }
+#endif
+
 #endif /* !CONFIG_GENERIC_CLK */
 
 /**
-- 
1.7.4.1

Re: [PATCH v3 5/5] clk: export tree topology and clk data via sysfs

[Arnd Bergmann]

On Tuesday 22 November 2011, Mike Turquette wrote:
> Introduces kobject support for the common struct clk, exports per-clk
> data via read-only callbacks and models the clk tree topology in sysfs.
> 
> Also adds support for generating the clk tree in clk_init and migrating
> nodes when input sources are switches in clk_set_parent.
> 
> Signed-off-by: Mike Turquette <mturquette@linaro.org>
> ---
>  drivers/clk/Kconfig     |   10 +++
>  drivers/clk/Makefile    |    1 +
>  drivers/clk/clk-sysfs.c |  199 +++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/clk/clk.c       |    5 +-
>  include/linux/clk.h     |   36 ++++++++-
>  5 files changed, 248 insertions(+), 3 deletions(-)
>  create mode 100644 drivers/clk/clk-sysfs.c

Since this introduces a new top-level hierarchy in sysfs, it certainly needs
to be reviewed by Greg K-H (on Cc now). Also, you have to add a proper
documentation for every new node and attribute in Documentation/ABI along
with the code.

> +static struct kobject *clk_kobj;
> +LIST_HEAD(kobj_list);

The list head should be static.

	Arnd

Re: [PATCH v3 4/5] clk: basic gateable and fixed-rate clks

[Arnd Bergmann]

On Tuesday 22 November 2011, Mike Turquette wrote:
> +static void clk_hw_gate_set_bit(struct clk *clk)
> +{
> +       struct clk_hw_gate *gate = to_clk_hw_gate(clk);
> +       u32 reg;
> +
> +       reg = __raw_readl(gate->reg);
> +       reg |= BIT(gate->bit_idx);
> +       __raw_writel(reg, gate->reg);
> +}

You cannot rely on __raw_readl() to do the right thing, especially
in architecture independent code. The safe (but slightly inefficient)
solution would be readl/writel. For ARM-only code, it would be best
to use readl_relaxed()/writel_relaxed(), but most architectures do
not implement that. We can probably add a set of helpers in asm-generic/
to define them to the default functions, like "#define readl_relaxed(x)
readl(x)", which I think is a good idea anyway.

	Arnd

Re: [PATCH v3 4/5] clk: basic gateable and fixed-rate clks

[Mark Salter]

On Tue, 2011-11-22 at 13:11 +0000, Arnd Bergmann wrote:
> On Tuesday 22 November 2011, Mike Turquette wrote:
> > +static void clk_hw_gate_set_bit(struct clk *clk)
> > +{
> > +       struct clk_hw_gate *gate = to_clk_hw_gate(clk);
> > +       u32 reg;
> > +
> > +       reg = __raw_readl(gate->reg);
> > +       reg |= BIT(gate->bit_idx);
> > +       __raw_writel(reg, gate->reg);
> > +}
> 
> You cannot rely on __raw_readl() to do the right thing, especially
> in architecture independent code. The safe (but slightly inefficient)
> solution would be readl/writel. For ARM-only code, it would be best
> to use readl_relaxed()/writel_relaxed(), but most architectures do
> not implement that. We can probably add a set of helpers in asm-generic/
> to define them to the default functions, like "#define readl_relaxed(x)
> readl(x)", which I think is a good idea anyway.
> 

readl/writel won't work for big endian CPU when the registers are on a
bus that does the endian swabbing in hardware.

--Mark

Re: [PATCH v3 0/5] common clk framework

[Greg KH]

On Mon, Nov 21, 2011 at 05:40:42PM -0800, Mike Turquette wrote:
>   .sysfs support.  Visualize your clk tree at /sys/clk!  Where would be
> a better place to put the clk tree besides the root of /sys/?

Um, in the "proper" place for it under /sys/devices like the rest of the
device tree is?

> When a consensus on this is reached I'll submit the proper changes to
> Documentation/ABI/testing/.

I would like to see this documentation now, as it explains what you are
trying to do with the userspace api.

More comments on your sysfs patch...

thanks,

greg k-h

Re: [PATCH v3 5/5] clk: export tree topology and clk data via sysfs

[Greg KH]

On Mon, Nov 21, 2011 at 05:40:47PM -0800, Mike Turquette wrote:
> Introduces kobject support for the common struct clk, exports per-clk
> data via read-only callbacks and models the clk tree topology in sysfs.
> 
> Also adds support for generating the clk tree in clk_init and migrating
> nodes when input sources are switches in clk_set_parent.
> 
> Signed-off-by: Mike Turquette <mturquette@linaro.org>

Thanks Arnd for pointing me at this.

> +#define MAX_STRING_LENGTH	32

Why?

> +static struct kobject *clk_kobj;
> +LIST_HEAD(kobj_list);

Um, no, please NEVER use raw kobjects, you should be using struct device
instead.  Please change the code to do that.

> +static ssize_t clk_show(struct kobject *kobj, struct attribute *attr,
> +		char *buf)
> +{
> +	struct clk *clk;
> +	struct clk_attribute *clk_attr;
> +	ssize_t ret = -EINVAL;
> +
> +	clk = container_of(kobj, struct clk, kobj);
> +	clk_attr = container_of(attr, struct clk_attribute, attr);
> +
> +	if (!clk || !clk_attr)
> +		goto out;
> +
> +	/* we don't do any locking for debug operations */
> +
> +	/* refcount++ */
> +	kobject_get(&clk->kobj);
> +
> +	if (clk_attr->show)
> +		ret = clk_attr->show(clk, buf);
> +	else
> +		ret = -EIO;
> +
> +	/* refcount-- */
> +	kobject_put(&clk->kobj);

Why in the world would you be incrementing and decrementing the
reference count of the kobject in the show function?  What are you
trying to protect here that is not already protected by the core?


> +static void clk_release(struct kobject *kobj)
> +{
> +	struct clk *clk;
> +
> +	clk = container_of(kobj, struct clk, kobj);
> +
> +	complete(&clk->kobj_unregister);
> +}

Look Ma, a memory leak!

> +static struct kobj_type clk_ktype = {
> +	.sysfs_ops	= &clk_ops,
> +	.default_attrs	= clk_default_attrs,
> +	.release	= clk_release,
> +};
> +
> +int clk_kobj_add(struct clk *clk)
> +{
> +	if (IS_ERR(clk))
> +		return -EINVAL;
> +
> +	/*
> +	 * Some kobject trickery!
> +	 *
> +	 * We want to (ab)use the kobject infrastructure to track our
> +	 * tree topology for us, specifically the root clocks (which are
> +	 * otherwise not remembered in a global list).
> +	 * Unfortunately we might not be able to allocate memory yet
> +	 * when this path is hit.  This pretty much rules out anything
> +	 * that looks or smells like kobject_add, since there are
> +	 * allocations for kobject->name and a dependency on sysfs being
> +	 * initialized.
> +	 *
> +	 * To get around this we initialize the kobjects and (ab)use
> +	 * struct kobject's list_head member, "entry".  Later on we walk
> +	 * this list in clk_sysfs_tree_create() to make proper
> +	 * kobject_add calls once it is safe to do so.
> +	 *
> +	 * FIXME - this is starting to smell alot like clkdev (i.e.
> +	 * tracking the clocks in a list)

Ah, comments like this warm my heart.

Come on, no abusing the kobject code please, if have problems with how
the kernel core works, and it doesn't do things you want it to, then why
not change it to work properly for you, or at the least, ASK ME!!!

> +	 */
> +
> +	kobject_init(&clk->kobj, &clk_ktype);
> +	list_add_tail(&clk->kobj.entry, &kobj_list);

Yeah, no kobject lists for you, sorry, DO NOT DO THIS!

> +static int __init clk_sysfs_init(void)
> +{
> +	struct list_head *tmp;
> +
> +	clk_kobj = kobject_create_and_add("clk", NULL);
> +
> +	WARN_ON(!clk_kobj);

Why?  What is this helping with?

Please rewrite to use 'struct device'.

thanks,

greg k-h

Re: [PATCH v3 4/5] clk: basic gateable and fixed-rate clks

[Arnd Bergmann]

On Tuesday 22 November 2011, Mark Salter wrote:
> 
> On Tue, 2011-11-22 at 13:11 +0000, Arnd Bergmann wrote:
> > On Tuesday 22 November 2011, Mike Turquette wrote:
> > > +static void clk_hw_gate_set_bit(struct clk *clk)
> > > +{
> > > +       struct clk_hw_gate *gate = to_clk_hw_gate(clk);
> > > +       u32 reg;
> > > +
> > > +       reg = __raw_readl(gate->reg);
> > > +       reg |= BIT(gate->bit_idx);
> > > +       __raw_writel(reg, gate->reg);
> > > +}
> > 
> > You cannot rely on __raw_readl() to do the right thing, especially
> > in architecture independent code. The safe (but slightly inefficient)
> > solution would be readl/writel. For ARM-only code, it would be best
> > to use readl_relaxed()/writel_relaxed(), but most architectures do
> > not implement that. We can probably add a set of helpers in asm-generic/
> > to define them to the default functions, like "#define readl_relaxed(x)
> > readl(x)", which I think is a good idea anyway.
> > 
> 
> readl/writel won't work for big endian CPU when the registers are on a
> bus that does the endian swabbing in hardware.

That statement doesn't make any sense.

You obviously have to specify the bit index in a way that works with the
driver implementation and with the hardware.

__raw_readl has an unspecified endianess, which is normally the same
as the register endianess of the CPU (assuming a memory-mapped bus),
which means you have to do extra work if the register layout is
independent of the CPU endianess, which is about as common as
MMIO registers defined as being the same endianes as the CPU in
bi-endian implementations.

Considering that hardware makers cannot agree on how to count bits
(IBM calls the MSB bit 0 on big-endian systems), there is no way
to please everyone, though you could debate about what the clearest
semantics are that we should define.

IMHO it would be nicer to use a bit-mask in bus-endian notation, e.g.

      reg = readl(gate->reg);
      reg |= le32_to_cpu(gate->bit_mask);
      writel(reg, gate->reg);

but there are other ways to do this. The only thing that I would
definitely ask for is having the interface clearly documented as
being one of cpu-endian, bus-endian, fixed-endian or having the
endianess specified in the device definition (device tree or platform
data).

Note that I don't object to adding a new cpu-endian mmio accessor,
which has been discussed repeatedly in the past. It's just that
this accessor does not exist, and using __raw_readl as a substitute
causes additional problems.

	Arnd

Re: [PATCH v3 5/5] clk: export tree topology and clk data via sysfs

[Mike Turquette]

On Tue, Nov 22, 2011 at 5:07 AM, Arnd Bergmann <arnd.bergmann@linaro.org> wrote:
> On Tuesday 22 November 2011, Mike Turquette wrote:
>> Introduces kobject support for the common struct clk, exports per-clk
>> data via read-only callbacks and models the clk tree topology in sysfs.
>>
>> Also adds support for generating the clk tree in clk_init and migrating
>> nodes when input sources are switches in clk_set_parent.
>>
>> Signed-off-by: Mike Turquette <mturquette@linaro.org>
>> ---
>>  drivers/clk/Kconfig     |   10 +++
>>  drivers/clk/Makefile    |    1 +
>>  drivers/clk/clk-sysfs.c |  199 +++++++++++++++++++++++++++++++++++++++++++++++
>>  drivers/clk/clk.c       |    5 +-
>>  include/linux/clk.h     |   36 ++++++++-
>>  5 files changed, 248 insertions(+), 3 deletions(-)
>>  create mode 100644 drivers/clk/clk-sysfs.c
>
> Since this introduces a new top-level hierarchy in sysfs, it certainly needs
> to be reviewed by Greg K-H (on Cc now). Also, you have to add a proper
> documentation for every new node and attribute in Documentation/ABI along
> with the code.

I don't intend to keep it in the top-level /sys/ directory.  I
mentioned that in the coverletter, but not here.  I wasn't sure where
to put it, and I knew reviewers would be happy to point the right
place out to me.

>> +static struct kobject *clk_kobj;
>> +LIST_HEAD(kobj_list);
>
> The list head should be static.

Will put into V4.

Thanks,
Mike

>
>        Arnd
>

Re: [PATCH v3 0/5] common clk framework

[Russell King - ARM Linux]

On Tue, Nov 22, 2011 at 07:42:59AM -0800, Greg KH wrote:
> On Mon, Nov 21, 2011 at 05:40:42PM -0800, Mike Turquette wrote:
> >   .sysfs support.  Visualize your clk tree at /sys/clk!  Where would be
> > a better place to put the clk tree besides the root of /sys/?
> 
> Um, in the "proper" place for it under /sys/devices like the rest of the
> device tree is?

I'd suggest that making the clock tree visible in sysfs (and therefore
part of the kernel ABI) is not a good idea.  Some of the nodes in there
will be specific to the implementation.  Exposing the clock nodes means
that if you have to change the clock tree structure, you change the
visible userspace ABI.

So, I'd suggest that we need to see a justification for this, rather
than exposing this stuff via debugfs as has been done with existing
implementations.

Re: [PATCH v3 5/5] clk: export tree topology and clk data via sysfs

[Mike Turquette]

On Tue, Nov 22, 2011 at 7:49 AM, Greg KH <greg@kroah.com> wrote:
> On Mon, Nov 21, 2011 at 05:40:47PM -0800, Mike Turquette wrote:
>> Introduces kobject support for the common struct clk, exports per-clk
>> data via read-only callbacks and models the clk tree topology in sysfs.
>>
>> Also adds support for generating the clk tree in clk_init and migrating
>> nodes when input sources are switches in clk_set_parent.
>>
>> Signed-off-by: Mike Turquette <mturquette@linaro.org>
>
> Thanks Arnd for pointing me at this.
>
>> +#define MAX_STRING_LENGTH    32
>
> Why?

Copy paste from other implementations.  What do you recommend?

>
>> +static struct kobject *clk_kobj;
>> +LIST_HEAD(kobj_list);
>
> Um, no, please NEVER use raw kobjects, you should be using struct device
> instead.  Please change the code to do that.

Today the clk tree doesn't create a struct device for each clk, which
I assume would be necessary to do what you're talking about.  Is that
right?

Maybe that will be necessary with the device tree stuff anyways... any
feedback from Sascha or Grant on that?

>
>> +static ssize_t clk_show(struct kobject *kobj, struct attribute *attr,
>> +             char *buf)
>> +{
>> +     struct clk *clk;
>> +     struct clk_attribute *clk_attr;
>> +     ssize_t ret = -EINVAL;
>> +
>> +     clk = container_of(kobj, struct clk, kobj);
>> +     clk_attr = container_of(attr, struct clk_attribute, attr);
>> +
>> +     if (!clk || !clk_attr)
>> +             goto out;
>> +
>> +     /* we don't do any locking for debug operations */
>> +
>> +     /* refcount++ */
>> +     kobject_get(&clk->kobj);
>> +
>> +     if (clk_attr->show)
>> +             ret = clk_attr->show(clk, buf);
>> +     else
>> +             ret = -EIO;
>> +
>> +     /* refcount-- */
>> +     kobject_put(&clk->kobj);
>
> Why in the world would you be incrementing and decrementing the
> reference count of the kobject in the show function?  What are you
> trying to protect here that is not already protected by the core?

Will remove in v4.

>
>
>> +static void clk_release(struct kobject *kobj)
>> +{
>> +     struct clk *clk;
>> +
>> +     clk = container_of(kobj, struct clk, kobj);
>> +
>> +     complete(&clk->kobj_unregister);
>> +}
>
> Look Ma, a memory leak!

Oops.  Will fix in V4.

>
>> +static struct kobj_type clk_ktype = {
>> +     .sysfs_ops      = &clk_ops,
>> +     .default_attrs  = clk_default_attrs,
>> +     .release        = clk_release,
>> +};
>> +
>> +int clk_kobj_add(struct clk *clk)
>> +{
>> +     if (IS_ERR(clk))
>> +             return -EINVAL;
>> +
>> +     /*
>> +      * Some kobject trickery!
>> +      *
>> +      * We want to (ab)use the kobject infrastructure to track our
>> +      * tree topology for us, specifically the root clocks (which are
>> +      * otherwise not remembered in a global list).
>> +      * Unfortunately we might not be able to allocate memory yet
>> +      * when this path is hit.  This pretty much rules out anything
>> +      * that looks or smells like kobject_add, since there are
>> +      * allocations for kobject->name and a dependency on sysfs being
>> +      * initialized.
>> +      *
>> +      * To get around this we initialize the kobjects and (ab)use
>> +      * struct kobject's list_head member, "entry".  Later on we walk
>> +      * this list in clk_sysfs_tree_create() to make proper
>> +      * kobject_add calls once it is safe to do so.
>> +      *
>> +      * FIXME - this is starting to smell alot like clkdev (i.e.
>> +      * tracking the clocks in a list)
>
> Ah, comments like this warm my heart.
>
> Come on, no abusing the kobject code please, if have problems with how
> the kernel core works, and it doesn't do things you want it to, then why
> not change it to work properly for you, or at the least, ASK ME!!!

Ok, I'm asking you now.  There are two ways to solve this problem:

1) have kobject core create the lists linking the objects but defer
allocations and any interactions with sysfs until later in the boot
sequence, OR

2) my code can create a list of clks (the same way that clkdev does)
and defer kobject/sysfs stuff until later, which walks the list made
during early-boot

#1 is most closely aligned with the code I have here, #2 presents
challenges that I haven't really though through.  I know that OMAP
uses the clk framework VERY early in it's boot sequence, but as long
as the per-clk data is properly initialized then it should be OK.

What do you think?

Regards,
Mike

Re: [PATCH v3 5/5] clk: export tree topology and clk data via sysfs

[Mike Turquette]

On Tue, Nov 22, 2011 at 8:37 AM, Grant Likely <grant.likely@secretlab.ca> wrote:
>
> On Nov 21, 2011 6:43 PM, "Mike Turquette" <mturquette@ti.com> wrote:
>>
>> Introduces kobject support for the common struct clk, exports per-clk
>> data via read-only callbacks and models the clk tree topology in sysfs.
>>
>> Also adds support for generating the clk tree in clk_init and migrating
>> nodes when input sources are switches in clk_set_parent.
>
> I'm not convinced this is a good idea. What is the use case for exporting
> the clock tree? If it is debug, then I suggest using debugfs to avoid abi
> issues.

Each clk exports clk_rate, clk_flags, clk_enable_count &
clk_prepare_count as Read-Only.  I think this is very reasonable to
have in sysfs, which maps the topology of the system with key details.

Others have requested to have knobs made available for actually
performing clk_enable/clk_disable and even clk_set_rate from
userspace.  I hate this idea and won't implement it.  I encourage
anyone that needs this to do it in debugfs.

Does that work-split make sense to you, or do you still not like the
idea of having topology and read-only info in sysfs?

Regards,
Mike

>
> g.

Re: [PATCH v3 0/5] common clk framework

[Mike Turquette]

On Tue, Nov 22, 2011 at 9:45 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Tue, Nov 22, 2011 at 07:42:59AM -0800, Greg KH wrote:
>> On Mon, Nov 21, 2011 at 05:40:42PM -0800, Mike Turquette wrote:
>> >   .sysfs support.  Visualize your clk tree at /sys/clk!  Where would be
>> > a better place to put the clk tree besides the root of /sys/?
>>
>> Um, in the "proper" place for it under /sys/devices like the rest of the
>> device tree is?
>
> I'd suggest that making the clock tree visible in sysfs (and therefore
> part of the kernel ABI) is not a good idea.  Some of the nodes in there
> will be specific to the implementation.  Exposing the clock nodes means
> that if you have to change the clock tree structure, you change the
> visible userspace ABI.

It is true that the ABI will change dynamically.

>
> So, I'd suggest that we need to see a justification for this, rather
> than exposing this stuff via debugfs as has been done with existing
> implementations.

Userspace tools like powerdebug (and maybe someday powertop) hope to
use a reliable-looking interface to view clk data.  There are obvious
uses for this data in a debug tool, the most obvious of which is
"which clk isn't turning off when it should?".

I can migrate this stuff to debugfs, but it adds the burden of having
debugfs enabled for folks that want to view this data.  I would also
argue that sysfs is there to model various aspects of system topology
and a clk tree certainly fits the bill.

If others also agree that it should reside only in debugfs then I'll
move it there for V4.

Thanks,
Mike

Re: [PATCH v3 0/5] common clk framework

[Greg KH]

On Tue, Nov 22, 2011 at 10:09:29AM -0800, Mike Turquette wrote:
> On Tue, Nov 22, 2011 at 9:45 AM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Tue, Nov 22, 2011 at 07:42:59AM -0800, Greg KH wrote:
> >> On Mon, Nov 21, 2011 at 05:40:42PM -0800, Mike Turquette wrote:
> >> >   .sysfs support.  Visualize your clk tree at /sys/clk!  Where would be
> >> > a better place to put the clk tree besides the root of /sys/?
> >>
> >> Um, in the "proper" place for it under /sys/devices like the rest of the
> >> device tree is?
> >
> > I'd suggest that making the clock tree visible in sysfs (and therefore
> > part of the kernel ABI) is not a good idea.  Some of the nodes in there
> > will be specific to the implementation.  Exposing the clock nodes means
> > that if you have to change the clock tree structure, you change the
> > visible userspace ABI.
> 
> It is true that the ABI will change dynamically.
> 
> >
> > So, I'd suggest that we need to see a justification for this, rather
> > than exposing this stuff via debugfs as has been done with existing
> > implementations.
> 
> Userspace tools like powerdebug (and maybe someday powertop) hope to
> use a reliable-looking interface to view clk data.  There are obvious
> uses for this data in a debug tool, the most obvious of which is
> "which clk isn't turning off when it should?".
> 
> I can migrate this stuff to debugfs, but it adds the burden of having
> debugfs enabled for folks that want to view this data.  I would also
> argue that sysfs is there to model various aspects of system topology
> and a clk tree certainly fits the bill.
> 
> If others also agree that it should reside only in debugfs then I'll
> move it there for V4.

If it's only for debug stuff, then yes, it belongs in debugfs.  All
distros turn debugfs on these days, and for an embedded system, the code
is quite small so there should not be much overhead.

thanks,

greg k-h

Re: [PATCH v3 5/5] clk: export tree topology and clk data via sysfs

[Greg KH]

On Tue, Nov 22, 2011 at 09:57:41AM -0800, Mike Turquette wrote:
> > Ah, comments like this warm my heart.
> >
> > Come on, no abusing the kobject code please, if have problems with how
> > the kernel core works, and it doesn't do things you want it to, then why
> > not change it to work properly for you, or at the least, ASK ME!!!
> 
> Ok, I'm asking you now.  There are two ways to solve this problem:
> 
> 1) have kobject core create the lists linking the objects but defer
> allocations and any interactions with sysfs until later in the boot
> sequence, OR
> 
> 2) my code can create a list of clks (the same way that clkdev does)
> and defer kobject/sysfs stuff until later, which walks the list made
> during early-boot
> 
> #1 is most closely aligned with the code I have here, #2 presents
> challenges that I haven't really though through.  I know that OMAP
> uses the clk framework VERY early in it's boot sequence, but as long
> as the per-clk data is properly initialized then it should be OK.
> 
> What do you think?

#3 - use debugfs and don't try to create a sysfs interface for the clock
structures :)

thanks,

greg k-h

Re: [PATCH v3 5/5] clk: export tree topology and clk data via sysfs

[Grant Likely]

On Tue, Nov 22, 2011 at 11:01 AM, Mike Turquette <mturquette@linaro.org> wrote:
> On Tue, Nov 22, 2011 at 8:37 AM, Grant Likely <grant.likely@secretlab.ca> wrote:
>>
>> On Nov 21, 2011 6:43 PM, "Mike Turquette" <mturquette@ti.com> wrote:
>>>
>>> Introduces kobject support for the common struct clk, exports per-clk
>>> data via read-only callbacks and models the clk tree topology in sysfs.
>>>
>>> Also adds support for generating the clk tree in clk_init and migrating
>>> nodes when input sources are switches in clk_set_parent.
>>
>> I'm not convinced this is a good idea. What is the use case for exporting
>> the clock tree? If it is debug, then I suggest using debugfs to avoid abi
>> issues.
>
> Each clk exports clk_rate, clk_flags, clk_enable_count &
> clk_prepare_count as Read-Only.  I think this is very reasonable to
> have in sysfs, which maps the topology of the system with key details.
>
> Others have requested to have knobs made available for actually
> performing clk_enable/clk_disable and even clk_set_rate from
> userspace.  I hate this idea and won't implement it.  I encourage
> anyone that needs this to do it in debugfs.
>
> Does that work-split make sense to you, or do you still not like the
> idea of having topology and read-only info in sysfs?

Unless there is a solid real-world use case for exporting this data to
userspace, I do not think sysfs is a good idea.  As long as the usage
(beyond debug) is theoretical I think the whole thing should be in
debugfs.  It can always be moved at a later date If a real use case
does become important.

g.

Re: [PATCH v3 0/5] common clk framework

[Mark Brown]

On Tue, Nov 22, 2011 at 11:12:26AM -0800, Greg KH wrote:
> On Tue, Nov 22, 2011 at 10:09:29AM -0800, Mike Turquette wrote:

> > If others also agree that it should reside only in debugfs then I'll
> > move it there for V4.

> If it's only for debug stuff, then yes, it belongs in debugfs.  All
> distros turn debugfs on these days, and for an embedded system, the code
> is quite small so there should not be much overhead.

Most of the embedded distros included, there's too much useful stuff in
there.

Re: [PATCH v3 5/5] clk: export tree topology and clk data via sysfs

[Arnd Bergmann]

On Tuesday 22 November 2011 12:19:51 Grant Likely wrote:
> On Tue, Nov 22, 2011 at 11:01 AM, Mike Turquette <mturquette@linaro.org> wrote:
>
> > Others have requested to have knobs made available for actually
> > performing clk_enable/clk_disable and even clk_set_rate from
> > userspace.  I hate this idea and won't implement it.  I encourage
> > anyone that needs this to do it in debugfs.
> >
> > Does that work-split make sense to you, or do you still not like the
> > idea of having topology and read-only info in sysfs?
> 
> Unless there is a solid real-world use case for exporting this data to
> userspace, I do not think sysfs is a good idea.  As long as the usage
> (beyond debug) is theoretical I think the whole thing should be in
> debugfs.  It can always be moved at a later date If a real use case
> does become important.

I would recomment not to spend any time on implementing a debugfs interface
for this right now. As far as I can tell, nothing relies on exporting
the structure to user space, so Mike's time is better spent on getting the
other four patches merged.

Note that the static topology information about clocks will already be
visible in /proc/devicetree when that is enabled and the clocks are
described in the .dts file.

	Arnd

Re: [PATCH v3 5/5] clk: export tree topology and clk data via sysfs

[Turquette, Mike]

On Tue, Nov 22, 2011 at 12:02 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tuesday 22 November 2011 12:19:51 Grant Likely wrote:
>> On Tue, Nov 22, 2011 at 11:01 AM, Mike Turquette <mturquette@linaro.org> wrote:
>>
>> > Others have requested to have knobs made available for actually
>> > performing clk_enable/clk_disable and even clk_set_rate from
>> > userspace.  I hate this idea and won't implement it.  I encourage
>> > anyone that needs this to do it in debugfs.
>> >
>> > Does that work-split make sense to you, or do you still not like the
>> > idea of having topology and read-only info in sysfs?
>>
>> Unless there is a solid real-world use case for exporting this data to
>> userspace, I do not think sysfs is a good idea.  As long as the usage
>> (beyond debug) is theoretical I think the whole thing should be in
>> debugfs.  It can always be moved at a later date If a real use case
>> does become important.
>
> I would recomment not to spend any time on implementing a debugfs interface
> for this right now. As far as I can tell, nothing relies on exporting
> the structure to user space, so Mike's time is better spent on getting the
> other four patches merged.

Seems to be some consensus on this.  Will drop userspace-visible clk
tree from V4 patchset.

Regards,
Mike

>
> Note that the static topology information about clocks will already be
> visible in /proc/devicetree when that is enabled and the clocks are
> described in the .dts file.
>
>        Arnd
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

Re: [PATCH v3 2/5] Documentation: common clk API

[Saravana Kannan]

On 11/21/2011 05:40 PM, Mike Turquette wrote:
> 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>
> ---
>   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..ef4333d
> --- /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

Typo

> +is modified slightly for brevity:
> +
> +struct clk {
> +	const char              *name;
> +	const struct clk_hw_ops *ops;

No strong opinion, but can we call this clk_ops for brevity?

> +	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 which can be visualized by enabling
> +CONFIG_COMMON_CLK_SYSFS.  The ops member points to an instance of struct
> +clk_hw_ops:
> +
> +	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);
> +	};
> +
> +These callbacks correspond to the clk API that has existed in
> +include/linux/clk.h for a while.  Below is a quick summary of the
> +operations in that header, as implemented in drivers/clk/clk.c.  These
> +comprise the driver-facing API:
> +
> +clk_prepare - does everything needed to get a clock ready to generate a
> +proper signal which may include ungating the clk and actually generating
> +that signal.  clk_prepare MUST be called before clk_enable.  This call
> +holds the global prepare_mutex, which also prevents clk rates and
> +topology from changing while held.  This API is meant to be the "slow"
> +part of a clk enable sequence, if applicable.  This function must not be
> +called in an interrupt context.

in an atomic context?

> +
> +clk_unprepare - the opposite of clk_prepare: does everything needed to
> +make a clk no longer ready to generate a proper signal, which may
> +include gating an active clk.  clk_disable must be called before
> +clk_unprepare.  All of the same rules for clk_prepare apply.
> +
> +clk_enable - ungate a clock and immediately start generating a valid clk
> +signal.  This is the "fast" part of a clk enable sequence, and maybe the
> +only functional part of that sequence.  Regardless clk_prepare must be
> +called BEFORE clk_enable.  The enable_spinlock is held across this call,
> +which means that clk_enable must not sleep.
> +
> +clk_disable - the opposite of clk_enable: gates a clock immediately.
> +clk_disable must be called before calling clk_unprepare.  All of the
> +same rules for clk_enable apply.
> +
> +clk_get_rate - Returns the cached rate for the clk.  Does NOT query the
> +hardware state.  No lock is held.

I wrote the stuff below and then realized that this ops is not really 
present in the code. Looks like stale doc. Can you please remove this? 
But I think the comments below do hold true for the actual 
clk_set_rate()/get_rate() implementation. I will try to repeat this as 
part of the actual code review.

I will be looking into the other patches in order, so, forgive me if I'm 
asking a question that has an obvious answer in the remaining patches.

I think a lock needs to be taken for this call too. What prevents a 
clock set rate from getting called and modifying the cached rate 
variable while it's bring read. I don't think we should have a common 
code assume that read/write of longs are atomic.

> +
> +clk_get_parent - Returns the cached parent for the clk.  Does NOT query
> +the hardware state.  No lock is held.

Same question as above. Can we assume a read of a pointer variable is 
atomic? I'm not sure. I think this needs locking too.

> +
> +clk_set_rate - Attempts to change the clk rate to the one specified.
> +Depending on a variety of common flags it may fail to maintain system
> +stability or result in a variety of other clk rates changing.

I'm not sure if this is intentional or if it's a mistake in phrasing it. 
IMHO, the rates of other clocks that are actually made available to 
device drivers should not be changed. This call might trigger rate 
changes inside the tree to accommodate the request from various 
children, but should not affect the rate of the leaf nodes.

Can you please clarify the intention and/or the wording?

> Holds the
> +same prepare_mutex held by clk_prepare/clk_unprepare and clk_set_parent.
> +
> +clk_set_parent - Switches the input source for a clk.  This only applies
> +to mux clks with multiple parents.  Holds the same prepare_mutex held by
> +clk_prepare/clk_unprepare and clk_set_rate.
> +
> +	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_hw_gate {
> +	struct clk      clk;
> +	struct clk      *fixed_parent;
> +	void __iomem    *reg;
> +	u8              bit_idx;
> +};

This is exactly how we do it in the MSM code for various clock types. 
So, this is awesome. Will make my life easier. I can't wait to get the 
MSM clock code upstream without pissing off Linus.

> +
> +struct clk_hw_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_hw_gate_enable_set(clk);
> +				clk_hw_gate_set_bit(clk);
> +
> +And the definition of clk_hw_gate_set_bit:
> +
> +static void clk_hw_gate_set_bit(struct clk *clk)
> +{
> +	struct clk_hw_gate *gate = to_clk_hw_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_hw_gate_set_bit there is use of
> +to_clk_hw_gate, which is defined as:
> +
> +#define to_clk_hw_gate(ck) container_of(ck, struct clk_hw_gate, clk)
> +
> +This simple abstration 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.

Glad you went with this approach and killed clk_hw after our discussion.

> +
> +	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_hw_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_hw_ops clk_hw_ops_your_clk {
> +	.enable		=&clk_hw_your_clk_enable;
> +	.disable	=&clk_hw_your_clk_disable;
> +};
> +
> +Implement the above functions using container_of:
> +
> +int clk_hw_your_clk_enable(struct clk *clk)
> +{
> +	struct clk_hw_your_clk *yclk;
> +
> +	yclk = container_of(clk, struct clk_hw_your_clk, clk);
> +
> +	magic(yclk);
> +};
> +
> +If you are statically allocating all of your clk_hw_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_hw_your_clk_register(const char *name, unsigned long flags,
> +		unsigned long some_data,
> +		struct your_struct *some_more_data)
> +{
> +	struct clk_hw_your_clk *yclk;
> +
> +	yclk = kmalloc(sizeof(struct clk_hw_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
> +
> +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

Is this really true? Based on a quick glance at the other patches, it 
doesn't look it disallows set rate if the clock is enabled. Which is 
correct, because clock rates can be change while they are enabled (I'm 
referring to leaf clocks) as long as the hardware supports doing it 
correctly. So, this needs rewording? I'm guessing you are trying to say 
that you can't change the parent rate if it has multiple enabled child 
clocks running off of it and one of them wants to cause a parent rate 
change? I'm not sure if even that should be enforced in the common code 
-- doesn't look like you do either.

> +2) using clk rate change notifiers to allow devices to handle dynamic
Must be 3)
> +rate changes for clks which do support changing rates while enabled

Again, I guess this applies to the other clock. Not the one for which 
clk_set_rate() is being called.

> +
> +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]
> +

I will come back to this topic later on. I like the idea of propagating 
the needs to the parent, but not sure if the current implementation will 
work for all types of clock trees/set rates even though the HW might 
actually allow it to be done right.

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

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 v3 3/5] clk: introduce the common clock framework

[Saravana Kannan]

On 11/21/2011 05:40 PM, Mike Turquette wrote:
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> new file mode 100644
> index 0000000..12c9994
> --- /dev/null
> +++ b/drivers/clk/clk.c
> @@ -0,0 +1,538 @@
> +/*
> + * 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/slab.h>
> +#include<linux/spinlock.h>
> +#include<linux/err.h>
> +
> +static DEFINE_SPINLOCK(enable_lock);
> +static DEFINE_MUTEX(prepare_lock);
> +
> +void __clk_unprepare(struct clk *clk)
> +{
> +	if (!clk)
> +		return;
> +
> +	if (WARN_ON(clk->prepare_count == 0))
> +		return;
> +
> +	if (--clk->prepare_count>  0)

Space before ">"?

> +		return;
> +
> +	WARN_ON(clk->enable_count>  0);

The spacing comment applies to all such instances the follow too.

> +
> +	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;
EPERM? EBADFD?

> +
> +	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 slow 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)
> +{
> +	if (!clk)
> +		return 0;
> +
> +	return clk->rate;

I think you need to grab the prepare_mutex here too. Otherwise another 
call to clk_set_rate() could be changing clk->rate while it's being read 
here. It shouldn't be a problem in ARM where I think 32bit reads are 
atomic. But I'm not sure you can say the same for all archs.

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

May be call this functions __clk_recalc_rates() to match the naming 
convention of the other fns that don't grab locks?

> + */
> +static void clk_recalc_rates(struct clk *clk)
> +{
> +	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);

Any reason you can't just do "else return" instead of the check below? 
That would be more straight-forward and also remove the need for old_rate.

> +
> +	if (old_rate == clk->rate)
> +		return;
> +
> +	/* 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;
> +	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;

Spacing fix near & and &&.

> +
> +	/* find the new rate and see if parent rate should change too */
> +	WARN_ON(!clk->ops->round_rate);

It looks like you don't consider absence of round_rate as an error 
(going by clk_round_rate()), so why warn on this? See below for 
additional comments.

> +	new_rate = clk->ops->round_rate(clk, rate,&parent_new_rate);

Should we split the "figuring out the parent rate" and "round rate"?
If any clock driver doesn't care for propagation (too simple clock tree 
or very versatile clock tree), and doesn't want to implement 
ops->round_rate, this code is still forcing them to implement 
ops->round_rate(). But then clk_round_rate() thinks it's okay if that 
ops->round_rate() is not implemented. This is a bit inconsistent.

May be we should just add ops->propagate_parent() that is optional and 
takes in the result of ops->round_rate()? If a clock needs propagation, 
it will implement and return the new parent rate.

> +
> +	/* FIXME propagate pre-rate change notification here */
> +	/* XXX note that pre-rate change notifications will stack */
> +
> +	/* change the rate of this clk */
> +	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.
> +	 */
> +	clk->rate = new_rate;
> +	/* FIXME propagate post-rate change notifier for only this clk */
> +	hlist_for_each_entry(child, tmp,&clk->children, child_node)
> +		clk_recalc_rates(child);
> +
> +	return fail_clk;
> +}
> +
> +/**
> + * 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;
> +
> +	if (!clk->ops->set_rate)
> +		return -ENOSYS;
> +
> +	/* bail early if nothing to do */
> +	if (rate == clk->rate)
> +		return rate;

This check needs to be after the lock is taken.

> +
> +	/* prevent racing with updates to the clock topology */
> +	mutex_lock(&prepare_lock);
> +	fail_clk = __clk_set_rate(clk, rate);
> +	if (fail_clk) {
> +		pr_warn("%s: failed to set %s rate to %lu\n",
> +				__func__, fail_clk->name, rate);

Going by the current implementation, the "fail_clk" is not necessarily 
the same as "clk". But the "rate" you are printing is always the rate 
for "clk".

if (fail_clk == clk)
  print blah
else
  print blah bloo

?

> +		/* FIXME propagate rate change abort notification here */
> +		ret = -EIO;
> +	}
> +	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)
> +{
> +	if (!clk)
> +		return NULL;
> +
> +	return clk->parent;

Same comment as get_rate(). I think this needs locking too to avoid 
maligned reads.

> +}
> +EXPORT_SYMBOL_GPL(clk_get_parent);
> +
> +void __clk_reparent(struct clk *clk, struct clk *new_parent)
> +{
> +	hlist_del(&clk->child_node);
> +	hlist_add_head(&clk->child_node,&new_parent->children);

Check new_parent != NULL before trying to add the clock to its children 
list?

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

I would like NULL clocks to be treated as real/proper clocks. So, can we 
split this into two "if"s please? And return 0 for (!clk)?

> +		return -EINVAL;
> +
> +	if (!clk->ops->set_parent)
> +		return -ENOSYS;
> +
> +	if (clk->parent == parent)
> +		return 0;

This check should be done after taking the lock.

> +
> +	/* prevent racing with updates to the clock topology */
> +	mutex_lock(&prepare_lock);
> +
> +	/* 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);
> +
> +	if (clk->ops->get_parent) {
> +		clk->parent = clk->ops->get_parent(clk);
> +		if (clk->parent)
> +			hlist_add_head(&clk->child_node,
> +					&clk->parent->children);
> +	} else
> +		clk->parent = NULL;
> +
> +	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);

Didn't review closely past 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 v3 5/5] clk: export tree topology and clk data via sysfs

[Saravana Kannan]

On 11/22/2011 11:13 AM, Greg KH wrote:
> On Tue, Nov 22, 2011 at 09:57:41AM -0800, Mike Turquette wrote:
>>> Ah, comments like this warm my heart.
>>>
>>> Come on, no abusing the kobject code please, if have problems with how
>>> the kernel core works, and it doesn't do things you want it to, then why
>>> not change it to work properly for you, or at the least, ASK ME!!!
>>
>> Ok, I'm asking you now.  There are two ways to solve this problem:
>>
>> 1) have kobject core create the lists linking the objects but defer
>> allocations and any interactions with sysfs until later in the boot
>> sequence, OR
>>
>> 2) my code can create a list of clks (the same way that clkdev does)
>> and defer kobject/sysfs stuff until later, which walks the list made
>> during early-boot
>>
>> #1 is most closely aligned with the code I have here, #2 presents
>> challenges that I haven't really though through.  I know that OMAP
>> uses the clk framework VERY early in it's boot sequence, but as long
>> as the per-clk data is properly initialized then it should be OK.
>>
>> What do you think?
>
> #3 - use debugfs and don't try to create a sysfs interface for the clock
> structures :)

I would prefer debugfs too, but for my own selfish reasons. In our 
current implementation, we have debugfs files: turn on/off a clock, to 
measure a clock (yeah, we have a "measuring" hw block inside the SoC), 
list the supported rates of a clock, etc. We use these files to test the 
clock driver. These certainly would not be acceptable candidates for sysfs.

If the common clock framework uses sysfs for the tree, then the mach-msm 
will have to have its own implementation of debugfs. That's not so nice 
for two reasons:
1. I think these files would be useful for other arch/machs too, but it 
would be odd for the common clock code to expose sysfs and debugfs files 
for each clock.
2. Since it won't be in the common code, each arch/mach will be 
repeating the debugfs code to expose their own enable/disable files.

To me, the ideal choice would be for each clocks to have a directory 
under /debug/clk/ without following the clock topology. Then, inside 
each clock specific directory, there will be an enable, rate, parent 
(symlink to parent clock dir under /debug/clk/) files.

The clock specific drivers will be able to get the handle to the clk 
specific debugfs directory and add their own extra files. Or could be 
made to pass a list of file/ops/perms as part of the 
clock_init/registration.

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 v3 5/5] clk: export tree topology and clk data via sysfs

[Tony Lindgren]

Hi Mike,

* Greg KH <greg@kroah.com> [111122 10:51]:
> On Tue, Nov 22, 2011 at 09:57:41AM -0800, Mike Turquette wrote:
> > > Ah, comments like this warm my heart.
> > >
> > > Come on, no abusing the kobject code please, if have problems with how
> > > the kernel core works, and it doesn't do things you want it to, then why
> > > not change it to work properly for you, or at the least, ASK ME!!!
> > 
> > Ok, I'm asking you now.  There are two ways to solve this problem:
> > 
> > 1) have kobject core create the lists linking the objects but defer
> > allocations and any interactions with sysfs until later in the boot
> > sequence, OR

Please just make it all a loadable module using regular devices. That
makes the development a lot easier and as a side effect also guarantees
we're using standard interfaces.

Sure most platforms using it want it compiled in at least for now,
but in the long run it should be possible to load it when we get to
initramfs. For the early usage of clocks..

> > 2) my code can create a list of clks (the same way that clkdev does)
> > and defer kobject/sysfs stuff until later, which walks the list made
> > during early-boot

..let's plan on getting rid of the early usage of clocks instead so
you don't have the issue of deferring stuff.

> > #1 is most closely aligned with the code I have here, #2 presents
> > challenges that I haven't really though through.  I know that OMAP
> > uses the clk framework VERY early in it's boot sequence, but as long
> > as the per-clk data is properly initialized then it should be OK.
> > 
> > What do you think?

We initialize clocksource/clockevent clocks early, but we can do that
without clock fwk initially, then let a clockevent driver take over
later on. That should solve your issues #1 and #2.

> #3 - use debugfs and don't try to create a sysfs interface for the clock
> structures :)

Sounds like the debugfs is the way to go :)

Tony

Re: [PATCH v3 5/5] clk: export tree topology and clk data via sysfs

[Russell King - ARM Linux]

On Wed, Nov 23, 2011 at 08:59:04AM -0800, Tony Lindgren wrote:
> ..let's plan on getting rid of the early usage of clocks instead so
> you don't have the issue of deferring stuff.

No - we have too many platforms already using them early to do a change
like this - and to do such a change will force them to invent some other
way.  That's just stupid.

Keep the clk API as a fundamental thing which should be initialized early
so we don't have to invent new clk APIs to get around its unavailability.

Re: [PATCH v3 5/5] clk: export tree topology and clk data via sysfs

[Tony Lindgren]

* Russell King - ARM Linux <linux@arm.linux.org.uk> [111123 09:31]:
> On Wed, Nov 23, 2011 at 08:59:04AM -0800, Tony Lindgren wrote:
> > ..let's plan on getting rid of the early usage of clocks instead so
> > you don't have the issue of deferring stuff.
> 
> No - we have too many platforms already using them early to do a change
> like this - and to do such a change will force them to invent some other
> way.  That's just stupid.

Not necessarily for all the systems. For omap2+ we should be able to
boot the system initially with the perf counter as the clockevent and
32KiHz always running timer. Then a dmtimer using proper clockevent
driver could take over.
 
> Keep the clk API as a fundamental thing which should be initialized early
> so we don't have to invent new clk APIs to get around its unavailability.

What else are you aware of that is really needed early for clocks other
than clockevent?

We've already proven that SRAM init can be moved to happen later. The
optional debug serial port is already enabled by the bootloader.

Regards,

Tony

Re: [PATCH v3 5/5] clk: export tree topology and clk data via sysfs

[Mark Brown]

On Wed, Nov 23, 2011 at 10:55:19AM -0800, Tony Lindgren wrote:
> * Russell King - ARM Linux <linux@arm.linux.org.uk> [111123 09:31]:

> > Keep the clk API as a fundamental thing which should be initialized early
> > so we don't have to invent new clk APIs to get around its unavailability.

> What else are you aware of that is really needed early for clocks other
> than clockevent?

If nothing else we'd have to resolve all the device probe ordering
issues (Grant's patches seem a bit stalled) and convert all the systems
using the API to handle deferring probes until their clocks appear.
Using an early initcall of some kind would help with that but it does
seem like a lot of trouble and I'd expect it'll end up getting forced in
on most systems anyway.

Re: [PATCH v3 5/5] clk: export tree topology and clk data via sysfs

[Tony Lindgren]

* Mark Brown <broonie@opensource.wolfsonmicro.com> [111123 10:34]:
> On Wed, Nov 23, 2011 at 10:55:19AM -0800, Tony Lindgren wrote:
> > * Russell King - ARM Linux <linux@arm.linux.org.uk> [111123 09:31]:
> 
> > > Keep the clk API as a fundamental thing which should be initialized early
> > > so we don't have to invent new clk APIs to get around its unavailability.
> 
> > What else are you aware of that is really needed early for clocks other
> > than clockevent?
> 
> If nothing else we'd have to resolve all the device probe ordering
> issues (Grant's patches seem a bit stalled) and convert all the systems
> using the API to handle deferring probes until their clocks appear.
> Using an early initcall of some kind would help with that but it does
> seem like a lot of trouble and I'd expect it'll end up getting forced in
> on most systems anyway.

Yes the ordering depends on the system. In any case, initcalls are
not super early compared to setup_timer.

Tony

Re: [PATCH v3 2/5] Documentation: common clk API

[Turquette, Mike]

On Tue, Nov 22, 2011 at 6:03 PM, Saravana Kannan <skannan@codeaurora.org> wrote:
> On 11/21/2011 05:40 PM, Mike Turquette wrote:
>> +Below is the common struct clk definition from include/linux.clk.h.  It
>
> Typo

Will fix in V4.

>
>> +is modified slightly for brevity:
>> +
>> +struct clk {
>> +       const char              *name;
>> +       const struct clk_hw_ops *ops;
>
> No strong opinion, but can we call this clk_ops for brevity?

I prefer clk_hw_ops, as it clearly delineates that these operations
know hardware-specific details.

>> +clk_prepare - does everything needed to get a clock ready to generate a
>> +proper signal which may include ungating the clk and actually generating
>> +that signal.  clk_prepare MUST be called before clk_enable.  This call
>> +holds the global prepare_mutex, which also prevents clk rates and
>> +topology from changing while held.  This API is meant to be the "slow"
>> +part of a clk enable sequence, if applicable.  This function must not be
>> +called in an interrupt context.
>
> in an atomic context?

Will fix in V4.

>> +clk_get_rate - Returns the cached rate for the clk.  Does NOT query the
>> +hardware state.  No lock is held.
>
> I wrote the stuff below and then realized that this ops is not really present in the code. Looks like stale doc. Can you please remove this? But I think the comments below do hold true for the actual clk_set_rate()/get_rate() implementation. I will try to repeat this as part of the actual code review.

Firstly this is a summary of the clk API in clk.h, not clk_hw_ops.
There isn't a hardware op for this since we just return clk->parent.

Secondly it is up to the caller to hold a lock.  Any code calling
clk_get_rate might likely want to hold that lock anyways.  I'll update
the comments to be explicit about this.

>
> I will be looking into the other patches in order, so, forgive me if I'm asking a question that has an obvious answer in the remaining patches.
>
> I think a lock needs to be taken for this call too. What prevents a clock set rate from getting called and modifying the cached rate variable while it's bring read. I don't think we should have a common code assume that read/write of longs are atomic.
>
>> +
>> +clk_get_parent - Returns the cached parent for the clk.  Does NOT query
>> +the hardware state.  No lock is held.
>
> Same question as above. Can we assume a read of a pointer variable is atomic? I'm not sure. I think this needs locking too.

Same answer as above.  The caller must hold the lock.  I'll update the comments.

>
>> +
>> +clk_set_rate - Attempts to change the clk rate to the one specified.
>> +Depending on a variety of common flags it may fail to maintain system
>> +stability or result in a variety of other clk rates changing.
>
> I'm not sure if this is intentional or if it's a mistake in phrasing it. IMHO, the rates of other clocks that are actually made available to device drivers should not be changed. This call might trigger rate changes inside the tree to accommodate the request from various children, but should not affect the rate of the leaf nodes.
>
> Can you please clarify the intention and/or the wording?

Setting the flag CLK_GATE_SET_RATE tells clk_set_rate not to change
the rate if the clk is enabled.  This policy is not enforced
abritrarily: you don't have to set the flag on your clk.  I'll update
the doc to make explicit mention of this flag.

>> +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
>
> Is this really true? Based on a quick glance at the other patches, it doesn't look it disallows set rate if the clock is enabled. Which is correct, because clock rates can be change while they are enabled (I'm referring to leaf clocks) as long as the hardware supports doing it correctly. So, this needs rewording? I'm guessing you are trying to say that you can't change the parent rate if it has multiple enabled child clocks running off of it and one of them wants to cause a parent rate change? I'm not sure if even that should be enforced in the common code -- doesn't look like you do either.

Same as my answer above.  There is a flag which allows you to control
this behavior.

>
>> +2) using clk rate change notifiers to allow devices to handle dynamic
>
> Must be 3)

Haha good catch.

>>
>> +rate changes for clks which do support changing rates while enabled
>
> Again, I guess this applies to the other clock. Not the one for which clk_set_rate() is being called.

This applies to ANY clk which has the flag set and is called by
__clk_set_rate (which may be called many times in a recursive path).

>> +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]
>> +
>
> I will come back to this topic later on. I like the idea of propagating the needs to the parent, but not sure if the current implementation will work for all types of clock trees/set rates even though the HW might actually allow it to be done right.

I'll need more to go on than "it may not work".  As proof of concept I
converted OMAP4's CPUfreq driver to use this strategy.  Quick
explanation:

OMAP4's CPUfreq driver currently adjusts the rate of a PLL via
clk_set_rate.  However the PLL isn't *really* the clk closest to the
ARM IP, there are other divider clocks after the PLL, which typically
divide by 1.  So practically speaking the PLL is the one that matters
with respect to getting the Cortex-A9 rates to change.

To test the recursive clk_set_rate I wrote a new .round_rate for the
clks below the PLL and set the CLK_PARENT_SET_RATE flag on those clks.
 Then I changed the CPUfreq driver to call clk_set_rate on the lowest
clk in that path (instead of the PLL) which better reflects reality.
The code worked perfectly, propagating the request up to the PLL where
the rate was finally set.

This was a simple test since that PLL is dedicated to the Cortex-A9s,
but the code does work.  If there is a sequencing issue please let me
know and I'll see how things can be re-arranged or made more flexible
for your platform.

Regards,
Mike

Re: [PATCH v3 5/5] clk: export tree topology and clk data via sysfs

[Turquette, Mike]

On Tue, Nov 22, 2011 at 7:48 PM, Saravana Kannan <skannan@codeaurora.org> wrote:
> On 11/22/2011 11:13 AM, Greg KH wrote:
>>
>> On Tue, Nov 22, 2011 at 09:57:41AM -0800, Mike Turquette wrote:
>>>>
>>>> Ah, comments like this warm my heart.
>>>>
>>>> Come on, no abusing the kobject code please, if have problems with how
>>>> the kernel core works, and it doesn't do things you want it to, then why
>>>> not change it to work properly for you, or at the least, ASK ME!!!
>>>
>>> Ok, I'm asking you now.  There are two ways to solve this problem:
>>>
>>> 1) have kobject core create the lists linking the objects but defer
>>> allocations and any interactions with sysfs until later in the boot
>>> sequence, OR
>>>
>>> 2) my code can create a list of clks (the same way that clkdev does)
>>> and defer kobject/sysfs stuff until later, which walks the list made
>>> during early-boot
>>>
>>> #1 is most closely aligned with the code I have here, #2 presents
>>> challenges that I haven't really though through.  I know that OMAP
>>> uses the clk framework VERY early in it's boot sequence, but as long
>>> as the per-clk data is properly initialized then it should be OK.
>>>
>>> What do you think?
>>
>> #3 - use debugfs and don't try to create a sysfs interface for the clock
>> structures :)
>
> I would prefer debugfs too, but for my own selfish reasons. In our current

I'll cook up debugfs code for the fwk and drop sysfs.  Maybe not part
of V4 (per Arnd's suggestion to focus on upstreamable bits), or maybe
I will if I have time.

Regards,
Mike

Re: [PATCH v3 3/5] clk: introduce the common clock framework

[Turquette, Mike]

On Tue, Nov 22, 2011 at 7:12 PM, Saravana Kannan <skannan@codeaurora.org> wrote:
> On 11/21/2011 05:40 PM, 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)
>
> Space before ">"?

Will fix all spacing errors in v4.

>> +int __clk_enable(struct clk *clk)
>> +{
>> +       int ret = 0;
>> +
>> +       if (!clk)
>> +               return 0;
>> +
>> +       if (WARN_ON(clk->prepare_count == 0))
>> +               return -ESHUTDOWN;
>
> EPERM? EBADFD?

This came from discussion out of Jeremy's original patches and I'm
inclined to keep it there.

>> +unsigned long clk_get_rate(struct clk *clk)
>> +{
>> +       if (!clk)
>> +               return 0;
>> +
>> +       return clk->rate;
>
> I think you need to grab the prepare_mutex here too. Otherwise another call
> to clk_set_rate() could be changing clk->rate while it's being read here. It
> shouldn't be a problem in ARM where I think 32bit reads are atomic. But I'm
> not sure you can say the same for all archs.

Hmm, need to decide if code calling this will likely hold the mutex
itself.  The comment can be updated to say "caller must hold
prepare_mutex", but that may be undo burden for a driver that just
wants to know a clk rate.  Thoughts?

>> +/**
>> + * 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.
>
> May be call this functions __clk_recalc_rates() to match the naming
> convention of the other fns that don't grab locks?

Other functions have __private_func syntax for one of two reasons:

1) the outer function holds a lock, and sometimes we want to access
the inner function from some other code path that avoids nested
locking (e.g. clk_enable)

2) the outer function sets up some staging data for a recursive
mechanism to use (e.g. clk_set_rate)

clk_recalc_rates doesn't hold a lock nor does it have staging data, so
it would just end up looking like:

__clk_recalc_rates(struct clk *clk)
{
do the real work
}

clk_recalc_rates(struct clk *clk)
{
__clk_recalc_rates(clk)
}

There is no obvious gain for splitting the function.

>
>> + */
>> +static void clk_recalc_rates(struct clk *clk)
>> +{
>> +       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);
>
> Any reason you can't just do "else return" instead of the check below? That
> would be more straight-forward and also remove the need for old_rate.

old_rate doesn't protect against not having a .recalc_rate pointer.
It is an optimization for when a clks rate hasn't changed and there is
no reason to traverse the tree.  In the case where there is no
.recalc_rate pointer then it serves the dual-purpose of bailing out
early.

>> +struct clk *__clk_set_rate(struct clk *clk, unsigned long rate)
>> +{
>> +       struct clk *fail_clk = NULL;
>> +       int ret;
>> +       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;
>
> Spacing fix near & and &&.

Will fix in V4.

>
>> +
>> +       /* find the new rate and see if parent rate should change too */
>> +       WARN_ON(!clk->ops->round_rate);
>
> It looks like you don't consider absence of round_rate as an error (going by
> clk_round_rate()), so why warn on this? See below for additional comments.
>
>> +       new_rate = clk->ops->round_rate(clk, rate,&parent_new_rate);

The above will cause a NULL ptr deref if you don't have a .round_rate
defined, so I'd say having .round_rate isn't very optional for
clk_set_rate support :-)

The question is, should it be?  For very simple clocking schemes where
the PLL locking rates will never vary from board to board or the
oscillators won't get changed across products... I guess it's not
necessary.  I can conditionally check for it before calling unless
others feel that .round_rate should be mandatory for .set_rate.  Need
feedback on that.

>
> Should we split the "figuring out the parent rate" and "round rate"?

No.  These two are inherently linked.  If you set them independently
you will have some crazy code trying to make sure that the clk's rate
and the parent's rate that are being programmed make sense.  Best to
control is centrally.

> If any clock driver doesn't care for propagation (too simple clock tree or
> very versatile clock tree), and doesn't want to implement ops->round_rate,
> this code is still forcing them to implement ops->round_rate(). But then
> clk_round_rate() thinks it's okay if that ops->round_rate() is not
> implemented. This is a bit inconsistent.
>
> May be we should just add ops->propagate_parent() that is optional and takes

I don't like this at all.  ops->propagate_parent() would just be
another call to __clk_set_rate, so why obscure it?  I encourage you to
convert your platform over to this code and try it on for size first;
I think it will do the job for you.

> in the result of ops->round_rate()? If a clock needs propagation, it will
> implement and return the new parent rate.
>
>> +int clk_set_rate(struct clk *clk, unsigned long rate)
>> +{
>> +       struct clk *fail_clk;
>> +       int ret = 0;
>> +
>> +       if (!clk->ops->set_rate)
>> +               return -ENOSYS;
>> +
>> +       /* bail early if nothing to do */
>> +       if (rate == clk->rate)
>> +               return rate;
>
> This check needs to be after the lock is taken.

Will fix in v4.

>
>> +
>> +       /* prevent racing with updates to the clock topology */
>> +       mutex_lock(&prepare_lock);
>> +       fail_clk = __clk_set_rate(clk, rate);
>> +       if (fail_clk) {
>> +               pr_warn("%s: failed to set %s rate to %lu\n",
>> +                               __func__, fail_clk->name, rate);
>
> Going by the current implementation, the "fail_clk" is not necessarily the
> same as "clk". But the "rate" you are printing is always the rate for "clk".

Since this isn't python and we're not returning multiple values from a
function call I'll just change the error to:

pr_warn("%s: failed to set %s rate\n", __func__, fail_clk->name);

>
> if (fail_clk == clk)
>  print blah
> else
>  print blah bloo
>
> ?
>
>> +struct clk *clk_get_parent(struct clk *clk)
>> +{
>> +       if (!clk)
>> +               return NULL;
>> +
>> +       return clk->parent;
>
> Same comment as get_rate(). I think this needs locking too to avoid maligned
> reads.

I'm not so sure.  clk_get_rate is more difficult because I can imagine
driver code calling that for any number of reasons, so maybe the
locking should be in that function.

However clk_get_parent is a different beast. If you want the parent
pointer then odds are that you are already holding the prepare_mutex.
Again I feel that the comment should just be updated to say "caller
must hold prepare_mutex".  Objections to this approach?

>
>> +}
>> +EXPORT_SYMBOL_GPL(clk_get_parent);
>> +
>> +void __clk_reparent(struct clk *clk, struct clk *new_parent)
>> +{
>> +       hlist_del(&clk->child_node);
>> +       hlist_add_head(&clk->child_node,&new_parent->children);
>
> Check new_parent != NULL before trying to add the clock to its children
> list?

Will add to v4.

>> +int clk_set_parent(struct clk *clk, struct clk *parent)
>> +{
>> +       int ret = 0;
>> +
>> +       if (!clk || !clk->ops)
>
> I would like NULL clocks to be treated as real/proper clocks. So, can we
> split this into two "if"s please? And return 0 for (!clk)?

Eww that's gross. How about include/linux/clk.h has an "extern struct
clk dummy_clk" that has no ops and no parent and you can use to stub
out support for clks that you don't really manage.  I'd prefer that
over treating NULL clks as legit.

>
>> +               return -EINVAL;
>> +
>> +       if (!clk->ops->set_parent)
>> +               return -ENOSYS;
>> +
>> +       if (clk->parent == parent)
>> +               return 0;
>
> This check should be done after taking the lock.

Will fix in V4.

Thanks for the review,
Mike

Re: [PATCH v3 5/5] clk: export tree topology and clk data via sysfs

[Russell King - ARM Linux]

On Wed, Nov 23, 2011 at 10:55:19AM -0800, Tony Lindgren wrote:
> What else are you aware of that is really needed early for clocks other
> than clockevent?

TWD will lose its auto-calibration.  Then there's various clock source
and clock event implementations.  These all call for the clk API to be
up and running at init_early time.

Re: [PATCH v3 5/5] clk: export tree topology and clk data via sysfs

[Tony Lindgren]

* Russell King - ARM Linux <linux@arm.linux.org.uk> [111123 14:07]:
> On Wed, Nov 23, 2011 at 10:55:19AM -0800, Tony Lindgren wrote:
> > What else are you aware of that is really needed early for clocks other
> > than clockevent?
> 
> TWD will lose its auto-calibration.  Then there's various clock source
> and clock event implementations.  These all call for the clk API to be
> up and running at init_early time.

If we can't initialize those later on, then it sounds like some clocks
and some parts of the clock fwk code needs to be static.

However, we could still make big chunks of the clock framework into a
regular device driver for allocating non-static clocks, debugfs interface
and so on.

Regards,

Tony

Re: [PATCH v3 1/5] clk: Kconfig: add entry for HAVE_CLK_PREPARE

[Shawn Guo]

On Mon, Nov 21, 2011 at 05:40:43PM -0800, Mike Turquette wrote:
> 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>

Regards,
Shawn

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

Re: [PATCH v3 0/5] common clk framework

[Shawn Guo]

On Mon, Nov 21, 2011 at 05:40:42PM -0800, Mike Turquette wrote:
[...]

>   .the most notable change is the removal of struct clk_hw.

Happy to see that.

> This extra
> layer of abstraction is only necessary if we want hide the definition of
> struct clk from platform code.  Many developers expressed the need to
> know some details of the generic struct clk in the platform layer, and
> rightly so.  Now struct clk is defined in include/linux/clk.h, protected
> by #ifdef CONFIG_GENERIC_CLK.
> 
>   .flags have been introduced to struct clk, with several of them
> defined and used in the common code.  These flags protect against
> changing clk rates or switching the clk parent while that clk is
> enabled; another flag is used to signal to clk_set_rate that it should
> ask the parent to change it's rate too.
> 
>   .speaking of which, clk_set_rate has been overhauled and is now
> recursive. *collective groan*.  clk_set_rate is still simple for the
> common case of simply setting a single clk's rate.  But if your clk has
> the CLK_PARENT_SET_RATE flag and the .round_rate callback recommends
> changing the parent rate, then clk_set_rate will recurse upwards to the
> parent and try it all over again.  In the event of a failure everything
> unwinds and all the clks go out for drinks.
> 
I smell that this will be the part which makes the whole series risky
of being accepted in a desired time frame, with saying rate change
notifications are still missing for now.

>   .clk_register has been replaced by clk_init, which does NOT allocate
> memory for you.  Platforms should allocate their own clk_hw_whatever
> structure which contains struct clk.  clk_init is still necessary to
> initialize struct clk internals.  clk_init also accepts struct device
> *dev as an argument, but does nothing with it.  This is in anticipation
> of device tree support.
> 
I would say that we do not necessarily need to have 'struct device'
for each clk (for imx6 example, we have 110 clks, and I heard some
omap support has 225 clks?).  The device tree support can work out
everything it needs from the 'struct device_node', which can be a
clock blob constraining multiple clks (thanks to 'clock-cells').  That
said you may want to add the 'dev' argument for other reasons, but I
never used it when converting imx6 clock to device tree.

>   .Documentation!  I'm sure somebody reads it.
> 
>   .sysfs support.  Visualize your clk tree at /sys/clk!  Where would be
> a better place to put the clk tree besides the root of /sys/?  When a
> consensus on this is reached I'll submit the proper changes to
> Documentation/ABI/testing/.
> 
> What's missing?
> 
>   .per tree locking.  I implemented this at the Linaro Connect
> conference but the implementation was unpopular, so it didn't make the
> cut.  There needs to be better understanding of everyone's needs for
> this to work.
> 
>   .rate change notifications.  I simply didn't want to delay getting
> these patches to the list any longer, so the notifiers didn't make it
> in.  I'll submit them to the list soon, or roll them into the V4
> patchset.  There are comments in the clk API definitions for where
> PRECHANGE, POSTCHANGE and ABORT propagation will go.
> 
>   .basic mux clk, divider and dummy clk implementations.  I think others
> have some code lying around to implement these, so I left them out.
> 
>   .device tree support.  I haven't looked much at the on-going
> discussions on the dt clk bindings.  How compatible (or not) are the
> device tree clk bindings and the way these patches want to initialize
> clks?
> 
I have just converted imx6 clock to device tree based on this series
and Grant's of-clk series with one minor change on clk-basic which
technically is not part of the core support.  So I would say, do not
worry, it's perfectly compatible with device tree :)

>   .what is the overlap between common clk and clkdev?  We're essentially
> tracking the clks in two places (common clk's tree and clkdevs's list),
> which feels a bit wasteful.
> 
I do not see the overlap between these two.  The clkdev is nothing but
a list maintaining the mapping between necessary 'struct clk' and its
consumer's 'struct dev'.  It has no clock tree topology, and common
clk's tree has no that mapping.

-- 
Regards,
Shawn

Re: [PATCH v3 2/5] Documentation: common clk API

[Shawn Guo]

On Wed, Nov 23, 2011 at 12:33:47PM -0800, Turquette, Mike wrote:
> On Tue, Nov 22, 2011 at 6:03 PM, Saravana Kannan <skannan@codeaurora.org> wrote:
> > On 11/21/2011 05:40 PM, Mike Turquette wrote:

[...]

> >> +is modified slightly for brevity:
> >> +
> >> +struct clk {
> >> +       const char              *name;
> >> +       const struct clk_hw_ops *ops;
> >
> > No strong opinion, but can we call this clk_ops for brevity?
> 
> I prefer clk_hw_ops, as it clearly delineates that these operations
> know hardware-specific details.
> 
I tend to agree with Saravana for brevity.  Looking at clk-basic.c,
I do not think it's necessary to encode 'hw' in naming of clk_hw_fixed,
clk_hw_gate and any clocks wrapping 'struct clk' in there.  For
example, naming like clk_dummy, clk_imx seems brevity and clear,
and we do not need clk_hw_dummy and clk_hw_imx necessarily.

[...]

> >> +
> >> +clk_set_rate - Attempts to change the clk rate to the one specified.
> >> +Depending on a variety of common flags it may fail to maintain system
> >> +stability or result in a variety of other clk rates changing.
> >
> > I'm not sure if this is intentional or if it's a mistake in phrasing it. IMHO, the rates of other clocks that are actually made available to device drivers should not be changed. This call might trigger rate changes inside the tree to accommodate the request from various children, but should not affect the rate of the leaf nodes.
> >
> > Can you please clarify the intention and/or the wording?
> 
> Setting the flag CLK_GATE_SET_RATE tells clk_set_rate not to change
> the rate if the clk is enabled.  This policy is not enforced
> abritrarily: you don't have to set the flag on your clk.  I'll update
> the doc to make explicit mention of this flag.
> 
I guess the concern is not about the flag but the result of clk_set_rate
that might change a variety of other clocks, while Saravana said it
should not.  And I agree with Saravana.

> >> +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
> >
> > Is this really true? Based on a quick glance at the other patches, it doesn't look it disallows set rate if the clock is enabled. Which is correct, because clock rates can be change while they are enabled (I'm referring to leaf clocks) as long as the hardware supports doing it correctly. So, this needs rewording? I'm guessing you are trying to say that you can't change the parent rate if it has multiple enabled child clocks running off of it and one of them wants to cause a parent rate change? I'm not sure if even that should be enforced in the common code -- doesn't look like you do either.
> 
> Same as my answer above.  There is a flag which allows you to control
> this behavior.
> 
On the contrary, I have clocks on mxs platform which can be set rate
only when they are enabled, while there are users call clk_set_rate
when the clock is not enabled yet.  Do we need another flag
CLK_ON_SET_RATE for this type of clocks?

I'm unsure that clk API users will all agree with the use of the flags.
>From the code, the clock framework will make the call fail if users
attempt to clk_set_rate an enabled clock with flag CLK_GATE_SET_RATE.
And clk API users might argue that they do not (need to) know this
clock details, and it's clock itself (clock framework or/and clock
driver) who should handle this type of details.

> >
> >> +2) using clk rate change notifiers to allow devices to handle dynamic
> >
> > Must be 3)
> 
> Haha good catch.
> 
> >>
> >> +rate changes for clks which do support changing rates while enabled
> >
> > Again, I guess this applies to the other clock. Not the one for which clk_set_rate() is being called.
> 
> This applies to ANY clk which has the flag set and is called by
> __clk_set_rate (which may be called many times in a recursive path).
> 
> >> +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]
> >> +
> >
> > I will come back to this topic later on. I like the idea of propagating the needs to the parent, but not sure if the current implementation will work for all types of clock trees/set rates even though the HW might actually allow it to be done right.
> 
> I'll need more to go on than "it may not work".

		clk A
		  |
		  |
		  |
		clk B -----------\
		  |		 |
		  |		 |
		  |		 |
		clk C		clk D

You have stated "Another caveat is that child clks might run at weird
intermediate frequencies during a complex upwards propagation".  I'm
not sure that intermediate frequency will be safe/reasonable for all
the clocks.

Another thing is what we mentioned above, the set_rate propagating
should not change other leaf clocks' frequency.  For example, there is
timer_clk (clk D) as another child of clk B.  When the set_rate of
clk C gets propagated to change frequency for clk B, it will in turn
change the frequency for timer_clk.  I'm sure that some talk need to
happen between clk framework and timer driver before frequency of clk B
gets actually changed.  Is this something will be handled by rate
change notifications which is to be added?

> As proof of concept I
> converted OMAP4's CPUfreq driver to use this strategy.  Quick
> explanation:
> 
> OMAP4's CPUfreq driver currently adjusts the rate of a PLL via
> clk_set_rate.  However the PLL isn't *really* the clk closest to the
> ARM IP, there are other divider clocks after the PLL, which typically
> divide by 1.  So practically speaking the PLL is the one that matters
> with respect to getting the Cortex-A9 rates to change.
> 
> To test the recursive clk_set_rate I wrote a new .round_rate for the
> clks below the PLL and set the CLK_PARENT_SET_RATE flag on those clks.
>  Then I changed the CPUfreq driver to call clk_set_rate on the lowest
> clk in that path (instead of the PLL) which better reflects reality.
> The code worked perfectly, propagating the request up to the PLL where
> the rate was finally set.
> 
> This was a simple test since that PLL is dedicated to the Cortex-A9s,
> but the code does work.  If there is a sequencing issue please let me
> know and I'll see how things can be re-arranged or made more flexible
> for your platform.
> 
I'm currently looking at imx6 and mxs (imx28).  On imx6, I do not see
the need for set_rate propagation at all.  On mxs, I do see the need,
but it's a simple propagation, with only one level up and 1-1
parent-child relationship.  I'm not sure if it's a good idea to support
the full set_rate propagation from the beginning, since it makes the
implementation difficult dramatically.

-- 
Regards,
Shawn

Re: [PATCH v3 3/5] clk: introduce the common clock framework

[Shawn Guo]

On Mon, Nov 21, 2011 at 05:40:45PM -0800, Mike Turquette wrote:
[...]
> +/**
> + * 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

Eh, this is how flag CLK_GATE_SET_RATE is born?  Does that mean to make
the call succeed all the clocks on the propagating path need to be gated
before clk_set_rate gets called?

> + */
> +struct clk *__clk_set_rate(struct clk *clk, unsigned long rate)
> +{
> +	struct clk *fail_clk = NULL;
> +	int ret;
> +	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 */
> +	ret = clk->ops->set_rate(clk, new_rate);

Yes, right here, the clock gets set to some unexpected rate, since the
parent clock has not been set to the target rate yet at this point.

> +	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.
> +	 */
> +	clk->rate = new_rate;
> +	/* FIXME propagate post-rate change notifier for only this clk */
> +	hlist_for_each_entry(child, tmp, &clk->children, child_node)
> +		clk_recalc_rates(child);
> +
> +	return fail_clk;
> +}

[...]

> diff --git a/include/linux/clk.h b/include/linux/clk.h
> index 7213b52..3b0eb3f 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
> @@ -13,17 +15,134 @@
>  
>  #include <linux/kernel.h>
>  
> +#include <linux/kernel.h>

Eh, why adding a duplication?

> +#include <linux/errno.h>
> +
>  struct device;
>  
> +struct clk;

Do you really need this?

[...]

> +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,

The return type seems interesting.  If we intend to return a rate, it
should be 'unsigned long' rather than 'long'.  I'm just curious about
the possible reason behind that.

> +					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);

Nit: would it be reasonable to put set_rate after round_rate to group
*_rate functions together?

> +};
> +
> +/**
> + * 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);
> +
> +#endif /* !CONFIG_GENERIC_CLK */
>  
>  /**
>   * clk_get - lookup and obtain a reference to a clock producer.
> @@ -107,9 +226,16 @@ static inline void clk_unprepare(struct clk *clk)
>  }
>  #endif
>  
> +int __clk_enable(struct clk *clk);
> +void __clk_disable(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);
> +
Do we really need to export all these common clk internal functions?

Regards,
Shawn

>  /**
>   * 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.4.1
> 
> 

Re: [PATCH v3 4/5] clk: basic gateable and fixed-rate clks

[Shawn Guo]

On Mon, Nov 21, 2011 at 05:40:46PM -0800, 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.
> 
What I have seen is drivers/clk/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 contribution by Jamie Iles.
> 
> Signed-off-by: Mike Turquette <mturquette@linaro.org>
> ---
>  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

[...]

> +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;

The device tree support needs to get this 'struct clk *', so we may
want to have all these registering functions return the 'clk'.

> +}

-- 
Regards,
Shawn

Re: [PATCH v3 4/5] clk: basic gateable and fixed-rate clks

[Shawn Guo]

One comment was missed.

On Mon, Nov 21, 2011 at 05:40:46PM -0800, Mike Turquette wrote:
[...]
> +struct clk_hw_ops clk_hw_gate_set_enable_ops = {

const?

> +	.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 = {

ditto

Regards,
Shawn

> +	.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);

Re: [PATCH v3 0/5] common clk framework

[Turquette, Mike]

On Fri, Nov 25, 2011 at 11:06 PM, Shawn Guo <shawn.guo@freescale.com> wrote:
> On Mon, Nov 21, 2011 at 05:40:42PM -0800, Mike Turquette wrote:
>>   .speaking of which, clk_set_rate has been overhauled and is now
>> recursive. *collective groan*.  clk_set_rate is still simple for the
>> common case of simply setting a single clk's rate.  But if your clk has
>> the CLK_PARENT_SET_RATE flag and the .round_rate callback recommends
>> changing the parent rate, then clk_set_rate will recurse upwards to the
>> parent and try it all over again.  In the event of a failure everything
>> unwinds and all the clks go out for drinks.
>>
> I smell that this will be the part which makes the whole series risky
> of being accepted in a desired time frame, with saying rate change
> notifications are still missing for now.

I agree.  I'll send out V4 this coming week with the clk rate change
notifiers rolled in.  That means that there won't be any missing
pieces for my vision of how a complex clk tree should interact with
drivers.  If it is not what people want then we can probably de-scope
pieces of it, but I think the architecture is sound.

Thanks for reviewing Shawn.

Regards,
Mike

Re: [PATCH v3 2/5] Documentation: common clk API

[Turquette, Mike]

On Sat, Nov 26, 2011 at 12:47 AM, Shawn Guo <shawn.guo@freescale.com> wrote:
> On Wed, Nov 23, 2011 at 12:33:47PM -0800, Turquette, Mike wrote:
>> On Tue, Nov 22, 2011 at 6:03 PM, Saravana Kannan <skannan@codeaurora.org> wrote:
>> > On 11/21/2011 05:40 PM, Mike Turquette wrote:
>> > No strong opinion, but can we call this clk_ops for brevity?
>>
>> I prefer clk_hw_ops, as it clearly delineates that these operations
>> know hardware-specific details.
>>
> I tend to agree with Saravana for brevity.  Looking at clk-basic.c,

I will drop "hw" for V4.

>> >> +
>> >> +clk_set_rate - Attempts to change the clk rate to the one specified.
>> >> +Depending on a variety of common flags it may fail to maintain system
>> >> +stability or result in a variety of other clk rates changing.
>> >
>> > I'm not sure if this is intentional or if it's a mistake in phrasing it. IMHO, the rates of other clocks that are actually made available to device drivers should not be changed. This call might trigger rate changes inside the tree to accommodate the request from various children, but should not affect the rate of the leaf nodes.
>> >
>> > Can you please clarify the intention and/or the wording?
>>
>> Setting the flag CLK_GATE_SET_RATE tells clk_set_rate not to change
>> the rate if the clk is enabled.  This policy is not enforced
>> abritrarily: you don't have to set the flag on your clk.  I'll update
>> the doc to make explicit mention of this flag.
>>
> I guess the concern is not about the flag but the result of clk_set_rate
> that might change a variety of other clocks, while Saravana said it
> should not.  And I agree with Saravana.

This behavior is entirely within the control of whoever ports their
platform over to the common clk fwk.

The set of clks whose rates will be directly changed by a call to
clk_set_rate is: the clk specified in the call to clk_set_rate AND any
parent clks that __clk_set_rate propagates upwards to.  The set of
clks whose rates will be indirectly changed are the children of clks
in the direct set that are not themselves in the direct set.

I'll make it clear here that CLK_PARENT_SET_RATE only steps up one
parent and then whole thing is re-evaluated: meaning that if clk sets
CLK_PARENT_SET_RATE then we might go up to clk->parent (based on the
outcome of clk's .round_rate) and then if clk->parent does NOT set
CLK_PARENT_SET_RATE then propagation ends there.

Based on your comment below where iMX6 leaf clks only need their
parent rate changed, then this will work beautifully for you as
leaf_clk->parent won't set CLK_PARENT_SET_RATE in your case.

> On the contrary, I have clocks on mxs platform which can be set rate
> only when they are enabled, while there are users call clk_set_rate
> when the clock is not enabled yet.  Do we need another flag
> CLK_ON_SET_RATE for this type of clocks?

This brings up the point of where flags belong.  The point of
CLK_GATE_SET_RATE is to either avoid changing rates across a clk that
is not glitchless, or upsetting a functional module at the end of a
chain of clks which cannot gracefully withstand sudden rate change.
This is common enough that it merits being in the common clk code.

Likewise if CLK_ON_SET_RATE is very common, it too should belong in
the common clk code.  If iMX6 is the only platform like this, maybe
your .set_rate should implement this logic and return -ESHUTDOWN or
-EPERM or something so that __clk_set_rate can bail out gracefully.
Do any other platforms need that flag?

>
> I'm unsure that clk API users will all agree with the use of the flags.
> From the code, the clock framework will make the call fail if users
> attempt to clk_set_rate an enabled clock with flag CLK_GATE_SET_RATE.
> And clk API users might argue that they do not (need to) know this
> clock details, and it's clock itself (clock framework or/and clock
> driver) who should handle this type of details.

One way around this is to have clk_set_rate call
clk_disable/__clk_unprepare if CLK_GATE_SET_RATE is set.  Then if the
usecount is still > 0 then clk_set_rate will fail.  Personally I don't
like having the common clk_set_rate making cross-calls to the
enable/disable stuff, but for the sake of exploring the topic...

In your case it will be the opposite: clk_set_rate will call
__clk_prepare/clk_enable and if the usecount is 0 then it will fail.

This starts to get really complicated though and at some point all the
permutation eventually mean that drivers will have to know some
details.

If these flags don't exist I also think that the result will be
drivers that know exactly the details.  In my case it will be:

clk_disable
clk_set_rate
clk_enable

In your case it will be:

clk_enable
clk_set_rate
clk_disable

Maybe I'm over-thinking it?

>
>                clk A
>                  |
>                  |
>                  |
>                clk B -----------\
>                  |              |
>                  |              |
>                  |              |
>                clk C           clk D
>
> You have stated "Another caveat is that child clks might run at weird
> intermediate frequencies during a complex upwards propagation".  I'm
> not sure that intermediate frequency will be safe/reasonable for all
> the clocks.

The solution to this is to set CLK_GATE_SET_RATE on any clk that also
sets CLK_PARENT_SET_RATE and cannot withstand intermediate rates.
Intermediate rates are unavoidable any time you have the following:

child is not gated
child changes its rate
parent changes its rate

The second line ("child changes its rate") is in anticipation of the
third line ("parent changes its rate") and likely results in the child
running at a frequency other than the one it ultimately desires to run
at.

>
> Another thing is what we mentioned above, the set_rate propagating
> should not change other leaf clocks' frequency.  For example, there is
> timer_clk (clk D) as another child of clk B.  When the set_rate of
> clk C gets propagated to change frequency for clk B, it will in turn
> change the frequency for timer_clk.  I'm sure that some talk need to
> happen between clk framework and timer driver before frequency of clk B
> gets actually changed.  Is this something will be handled by rate
> change notifications which is to be added?

Yes, the point you make is exactly why clk rate change notifiers
exist, which includes aborting rate changes when drivers aren't OK
with the changes.  I'll have those notifiers as part of v4.

> I'm currently looking at imx6 and mxs (imx28).  On imx6, I do not see
> the need for set_rate propagation at all.  On mxs, I do see the need,
> but it's a simple propagation, with only one level up and 1-1
> parent-child relationship.  I'm not sure if it's a good idea to support
> the full set_rate propagation from the beginning, since it makes the
> implementation difficult dramatically.

The implementation isn't that complex.  I'll try to provide better
examples in v4 to visualize everything.

I think that practically speaking most SoCs have leaf clocks which
might want to change only their direct parent's rate and that is the
extent of all of this fancy propagation stuff.  However, targeting the
complex cases makes the code better, less narrowly focused and
hopefully a bit more future-proof.

I would also like to point out that as long as your clk doesn't set
the CLK_PARENT_SET_RATE flag then propagation will NEVER happen.  This
is likely going to be the case for 90% of your clks!  You don't have
to worry about this code running wild and hosing clks behind your
back.

If there is still hesitancy with v4 then we can talk about what should
be removed in the name of getting this stuff merged for 3.3.

Thanks for reviewing,
Mike

Re: [PATCH v3 3/5] clk: introduce the common clock framework

[Turquette, Mike]

On Sat, Nov 26, 2011 at 5:22 AM, Shawn Guo <shawn.guo@freescale.com> wrote:
> On Mon, Nov 21, 2011 at 05:40:45PM -0800, Mike Turquette wrote:
>> + * To prevent this consider setting the CLK_GATE_SET_RATE flag on any
>> + * clk where you also set the CLK_PARENT_SET_RATE flag
>
> Eh, this is how flag CLK_GATE_SET_RATE is born?  Does that mean to make
> the call succeed all the clocks on the propagating path need to be gated
> before clk_set_rate gets called?

Setting that flag on any clk implies nothing about the parents of that
clk.  Obviously if a clk's child is enabled then clk is enabled and
won't have it's rate change if the flag is set, which is the whole
point of the flag.  Again, you don't have to set the flag.

>> +     /* change the rate of this clk */
>> +     ret = clk->ops->set_rate(clk, new_rate);
>
> Yes, right here, the clock gets set to some unexpected rate, since the
> parent clock has not been set to the target rate yet at this point.

Sequence is irrelevant for the case where both parent and child change
dividers, since the output of the child clk will be "weird" at some
point.

>> diff --git a/include/linux/clk.h b/include/linux/clk.h
>> index 7213b52..3b0eb3f 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
>> @@ -13,17 +15,134 @@
>>
>>  #include <linux/kernel.h>
>>
>> +#include <linux/kernel.h>
>
> Eh, why adding a duplication?

Will fix in v4.

>
>> +#include <linux/errno.h>
>> +
>>  struct device;
>>
>> +struct clk;
>
> Do you really need this?

Strange.  This line existed pre-common clk patches.  I'll look into
why it is getting "added" back in.  This declaration is necessary for
the old-style clk frameworks which each defined their own struct clk.
I assume it is still needed but I'll look at the git history to figure
it out.

>> +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,
>
> The return type seems interesting.  If we intend to return a rate, it
> should be 'unsigned long' rather than 'long'.  I'm just curious about
> the possible reason behind that.

Yeah these are mostly from the original patch set.  I'll go over these
with a fine-tooth comb for v4.  To some extent I think they model the
return types of the clk API in clk.h.

>
>> +                                     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);
>
> Nit: would it be reasonable to put set_rate after round_rate to group
> *_rate functions together?

Will fix in v4.

>> +int __clk_enable(struct clk *clk);
>> +void __clk_disable(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);
>> +
> Do we really need to export all these common clk internal functions?

I think we do.  Saravana also felt this was necessary.

I know that for OMAP we need __clk_prepare, __clk_unprepare and
__clk_reparent just to handle our PLLs.  I don't think we need
__clk_enable or __clk_disable since we can call the proper APIs for
those with the prepare_mutex held.

If no one needs __clk_enable and __clk_disable then I am happy to
remove those declarations.

Thanks for reviewing,
Mike

Re: [PATCH v3 4/5] clk: basic gateable and fixed-rate clks

[Turquette, Mike]

On Sat, Nov 26, 2011 at 5:48 AM, Shawn Guo <shawn.guo@freescale.com> wrote:
> On Mon, Nov 21, 2011 at 05:40:46PM -0800, 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.
>>
> What I have seen is drivers/clk/clk-basic.c.

Will fix in v4.

>> +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;
>
> The device tree support needs to get this 'struct clk *', so we may
> want to have all these registering functions return the 'clk'.

Thanks for the input.  Truthfully I'm very DT-ignorant so I'm happy to
reshape any APIs for that topic.  Hope to fix that soon.

Thanks for reviewing,
Mike

Re: [PATCH v3 3/5] clk: introduce the common clock framework

[Paul Walmsley]

Hello,

Here are some initial comments on clk_get_rate().

On Mon, 21 Nov 2011, Mike Turquette wrote:

> +/**
> + * 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)
> +{
> +	if (!clk)
> +		return 0;
> +
> +	return clk->rate;
> +}
> +EXPORT_SYMBOL_GPL(clk_get_rate);

This implementation of clk_get_rate() is racy, and is, in general, unsafe. 
The problem is that, in many cases, the clock's rate may change between 
the time that clk_get_rate() is called and the time that the returned 
rate is used.  This is the case for many clocks which are part of a 
larger DVFS domain, for example.

Several changes are needed to fix this:

1. When a clock user calls clk_enable() on a clock, the clock framework 
should prevent other users of the clock from changing the clock's rate.  
This should persist until the clock user calls clk_disable() (but see also 
#2 below).  This will ensure that clock users can rely on the rate 
returned by clk_get_rate(), as long as it's called between clk_enable() 
and clk_disable().  And since the clock's rate is guaranteed to remain the 
same during this time, code that cannot tolerate clock rate changes 
without special handling (such as driver code for external I/O devices) 
will work safely without further modification.

2. Since the protocol described in #1 above will prevent DVFS from working 
when the clock is part of a larger DVFS clock group, functions need to be 
added to allow and prevent other clock users from changing the clock's 
rate.  I'll use the function names "clk_allow_rate_changes(struct clk *)" 
and "clk_block_rate_changes(struct clk *)" for this discussion.  These 
functions can be used by clock users to define critical sections during 
which other entities on the system are allowed to change a clock's rate - 
even if the clock is currently enabled.  (Note that when a clock is 
prevented from changing its rate, all of the clocks from it up to the root 
of the tree should also be prevented from changing rates, since parent 
rate changes generally cause disruptions in downstream clocks.)

3. For the above changes to work, the clock framework will need to 
discriminate between different clock users' calls to clock functions like 
clk_{get,set}_rate(), etc.  Luckily this should be possible within the 
current clock interface.  clk_get() can allocate and return a new struct 
clk that clk_put() can later free.  One useful side effect of doing this 
is that the clock framework could catch unbalanced clk_{enable,disable}() 
calls.

4. clk_get_rate() must return an error when it's called in situations 
where the caller hasn't ensured that the clock's rate won't be changed by 
other entities.  For non-fixed rate clocks, these forbidden sections would 
include code between a clk_get() and a clk_enable(), or between a 
clk_disable() and a clk_put() or clk_enable(), or between a 
clk_allow_rate_changes() and clk_block_rate_changes().  The first and 
second of those three cases will require some code auditing of 
clk_get_rate() users to ensure that they are only calling it after they've 
enabled the clock.  And presumably most of them are not checking for 
error.  include/linux/clk.h doesn't define an error return value, so this 
needs to be explicitly defined in clk.h.


- Paul

Re: [PATCH v3 3/5] clk: introduce the common clock framework

[Paul Walmsley]

Hi

a few initial comments on clk_get_parent().

On Mon, 21 Nov 2011, Mike Turquette wrote:

> +/**
> + * 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)
> +{
> +	if (!clk)
> +		return NULL;
> +
> +	return clk->parent;
> +}
> +EXPORT_SYMBOL_GPL(clk_get_parent);

This implementation of clk_get_parent() has similar problems to the 
clk_get_rate() implementation:

    http://lkml.org/lkml/2011/11/30/403

clk_get_parent() should return an error if some other entity can change 
the clock's parent between the time that clk_get_parent() returns, and the 
time that the returned struct clk * is used.

For this to work, we need to define when clk_get_parent() should succeed. 
If we follow the protocol outlined in the above URL about clk_get_rate(), 
and we stipulate that when we block rate changes, we also block parent 
changes, then this should be fairly straightforward.  clk_get_parent() can 
succeed:

1. between a clk_enable() and a clk_disable() or clk_allow_rate_changes(),

2. between a clk_block_rate_changes() and a clk_enable(), clk_disable(), 
   or clk_allow_rate_changes()

As with clk_get_rate(), include/linux/clk.h is missing documentation of 
what the error return value should be.  This should be a little easier to 
define than with clk_get_rate(), but I don't think the error value should 
be NULL.  This is because NULL is a valid return value when 
clk_get_parent() is called on root clocks.  Better to use 
ERR_PTR(-EINVAL/-EBUSY/etc.).


- Paul

Re: [PATCH v3 3/5] clk: introduce the common clock framework

[Turquette, Mike]

On Wed, Nov 30, 2011 at 5:20 PM, Paul Walmsley <paul@pwsan.com> wrote:
> This implementation of clk_get_rate() is racy, and is, in general, unsafe.
> The problem is that, in many cases, the clock's rate may change between
> the time that clk_get_rate() is called and the time that the returned
> rate is used.  This is the case for many clocks which are part of a
> larger DVFS domain, for example.

Hi Paul,

Thanks much for reviewing.  Just FYI, the v4 patchset I'm working on
has clk_get_rate and clk_get_parent hold the prepare mutex themselves,
so it solves some of the raciness of accessing struct clk members.
This change does not address your points below but I wanted to include
it for completeness sake.

> Several changes are needed to fix this:
>
> 1. When a clock user calls clk_enable() on a clock, the clock framework
> should prevent other users of the clock from changing the clock's rate.
> This should persist until the clock user calls clk_disable() (but see also
> #2 below).  This will ensure that clock users can rely on the rate
> returned by clk_get_rate(), as long as it's called between clk_enable()
> and clk_disable().  And since the clock's rate is guaranteed to remain the
> same during this time, code that cannot tolerate clock rate changes
> without special handling (such as driver code for external I/O devices)
> will work safely without further modification.

This is certainly imposing a default behavior in the clk framework core.

> 2. Since the protocol described in #1 above will prevent DVFS from working
> when the clock is part of a larger DVFS clock group, functions need to be
> added to allow and prevent other clock users from changing the clock's
> rate.  I'll use the function names "clk_allow_rate_changes(struct clk *)"
> and "clk_block_rate_changes(struct clk *)" for this discussion.  These
> functions can be used by clock users to define critical sections during
> which other entities on the system are allowed to change a clock's rate -
> even if the clock is currently enabled.  (Note that when a clock is
> prevented from changing its rate, all of the clocks from it up to the root
> of the tree should also be prevented from changing rates, since parent
> rate changes generally cause disruptions in downstream clocks.)

Likewise when a clk is requested to transition to a new frequency it
will have to clear it with all of the clks below it, so there is still
a need to propagate a request throughout the clk tree whenever
clk_set_rate is called and take a decision.

One way to solve this is for driver owners to sprinkle their code with
clk_block_rate_change and clk_allow_rate_change as you propose.  There
is a problem with this method if it is not supplemented with
rate-change notifications that drivers are allowed to handle
asynchronously.  Take for example a MMC driver which normally runs
it's functional clk at 24MHz.  However for a low-intensity, periodic
task such as playing an mp3 from SD card we would really like to lower
clk rates across the whole SoC.  Assuming that the MMC driver holds
clk_block_rate_change across any SD card transaction, our low power
scheme to lower clks across the SoC is stuck in a racy game of trying
to request a lower clk rate for the MMC functional clk while that same
MMC controller is mostly saturated serving up the mp3.

In this case it would be nice to have asynchronous notifiers that can
interrupt the MMC driver with a pre-change notifier and say "please
finish your current transaction, stall your next transaction, and then
return so I can change your clk rate".  Then after the clk rate change
occurs a post-change notification would also go out to the MMC driver
saying "ok I'm done changing rates.  here is your new clk rate and
please continue working".  The notification is very polite.

It is worth nothing that the MMC driver can return NOTIFY_STOP in it's
pre-change notification handler and this effectively does the same
thing as traversing the clk subtree and checking to make sure that
nobody is holding clk_block_rate_change against any of the children
clocks.

The point of all of that text above is to say: if we have to walk the
whole clk subtree below the point where we want to change rates AND we
want to make a dynamic case-by-case call on whether to allow the rate
change to happen (as opposed to contending with the allow/block
semantics) then I think that only having notifications will suffice.

> 3. For the above changes to work, the clock framework will need to
> discriminate between different clock users' calls to clock functions like
> clk_{get,set}_rate(), etc.  Luckily this should be possible within the
> current clock interface.  clk_get() can allocate and return a new struct
> clk that clk_put() can later free.  One useful side effect of doing this
> is that the clock framework could catch unbalanced clk_{enable,disable}()
> calls.

This is definitely worth thinking about.  Another way to accomplish
this is stop treating prepare_count and enable_count as scalars and
instead vectorize them by tracking a list of struct device *'s.  This
starts to get into can-of-worms territory if we want to consider
clk_set_rate_range(dev, clk, min, max) and the broader DVFS
implications.  I'm a little worried about under-designing now for
future DVFS stuff, but I also don't want the common clk fundamentals
to stall any more than it has (2+ years!).

> 4. clk_get_rate() must return an error when it's called in situations
> where the caller hasn't ensured that the clock's rate won't be changed by
> other entities.  For non-fixed rate clocks, these forbidden sections would
> include code between a clk_get() and a clk_enable(), or between a
> clk_disable() and a clk_put() or clk_enable(), or between a
> clk_allow_rate_changes() and clk_block_rate_changes().  The first and
> second of those three cases will require some code auditing of
> clk_get_rate() users to ensure that they are only calling it after they've
> enabled the clock.  And presumably most of them are not checking for
> error.  include/linux/clk.h doesn't define an error return value, so this
> needs to be explicitly defined in clk.h.

This adds a lot of burden to a driver that just wants to know a rate.
Especially if that purpose is for something as simple/transient as a
pr_debug message or something.

Thanks again for the review.  I'll chew on it a bit.

Regards,
Mike

Re: [PATCH v3 3/5] clk: introduce the common clock framework

[Paul Walmsley]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 5556 bytes --]

Hi Mike

a few brief comments.

On Wed, 30 Nov 2011, Turquette, Mike wrote:

> Likewise when a clk is requested to transition to a new frequency it
> will have to clear it with all of the clks below it, so there is still
> a need to propagate a request throughout the clk tree whenever
> clk_set_rate is called and take a decision.
> 
> One way to solve this is for driver owners to sprinkle their code with
> clk_block_rate_change and clk_allow_rate_change as you propose.
> There is a problem with this method if it is not supplemented with 
> rate-change notifications that drivers are allowed to handle 
> asynchronously.  

I don't think notifiers have a bearing on clk_get_rate().

> In this case it would be nice to have asynchronous notifiers that can
> interrupt the MMC driver with a pre-change notifier and say "please
> finish your current transaction, stall your next transaction, and then
> return so I can change your clk rate".  Then after the clk rate change
> occurs a post-change notification would also go out to the MMC driver
> saying "ok I'm done changing rates.  here is your new clk rate and
> please continue working".  The notification is very polite.

I haven't made any comments about clock notifiers yet, because my 
comments so far have only concerned clk_get_rate() and 
clk_get_parent().

Clock rate/parent-change notifiers are requirements for DVFS use-cases, 
and they must be paired with something like the 
clk_{allow,block}_rate_change() functions to work efficiently.  I intend 
to comment on this later; it's not a simple problem.  It might be worth 
noting that Tero and I implemented a simplified version of this for the 
N900.

> It is worth nothing that the MMC driver can return NOTIFY_STOP in it's
> pre-change notification handler and this effectively does the same
> thing as traversing the clk subtree and checking to make sure that
> nobody is holding clk_block_rate_change against any of the children
> clocks.
> 
> The point of all of that text above is to say: if we have to walk the
> whole clk subtree below the point where we want to change rates AND we
> want to make a dynamic case-by-case call on whether to allow the rate
> change to happen (as opposed to contending with the allow/block
> semantics) then I think that only having notifications will suffice.

That would not be good.  A notifier implementation without something like 
clk_{allow,block}_rate_change() is going to waste a lot of CPU time making 
pointless calls into the notifier chains by whatever is attempting to do 
top-down DVFS.

With clk_{allow,block}_rate_change(), only a single comparison is needed 
to determine whether or not the rate change can succeed.  A 
blocking clk_set_rate() variant could also be efficiently implemented with 
that information, if so desired.

In general, a clock rate change notifier should almost never return 
NOTIFY_STOP.  That should only happen if some event occurred that took 
place after the notifier chain started.

> > 3. For the above changes to work, the clock framework will need to
> > discriminate between different clock users' calls to clock functions like
> > clk_{get,set}_rate(), etc.  Luckily this should be possible within the
> > current clock interface.  clk_get() can allocate and return a new struct
> > clk that clk_put() can later free.  One useful side effect of doing this
> > is that the clock framework could catch unbalanced clk_{enable,disable}()
> > calls.
> 
> This is definitely worth thinking about.  Another way to accomplish
> this is stop treating prepare_count and enable_count as scalars and
> instead vectorize them by tracking a list of struct device *'s.

To return to my original comments on clk_get_rate(): how would this allow 
the clock framework to discriminate between callers of clk_get_rate()?  
(Or indeed any clock framework function?)  Or is your comment simply 
addressing the unbalanced clk_{enable,disable}() case?

> > 4. clk_get_rate() must return an error when it's called in situations
> > where the caller hasn't ensured that the clock's rate won't be changed by
> > other entities.  For non-fixed rate clocks, these forbidden sections would
> > include code between a clk_get() and a clk_enable(), or between a
> > clk_disable() and a clk_put() or clk_enable(), or between a
> > clk_allow_rate_changes() and clk_block_rate_changes().  The first and
> > second of those three cases will require some code auditing of
> > clk_get_rate() users to ensure that they are only calling it after they've
> > enabled the clock.  And presumably most of them are not checking for
> > error.  include/linux/clk.h doesn't define an error return value, so this
> > needs to be explicitly defined in clk.h.
> 
> This adds a lot of burden to a driver that just wants to know a rate.
> Especially if that purpose is for something as simple/transient as a
> pr_debug message or something.

Could you clarify what burden are you referring to?

The above protocol requires few (if any) changes to existing clock 
framework users.  So for example, the following code:

c = clk_get(dev, clk_name);
clk_enable(c);
rate = clk_get_rate(c);

would still continue to work, since the rate would be guaranteed not to 
change after the clk_enable().  However, the (unsafe):

c = clk_get(dev, clk_name);
rate = clk_get_rate(c);

would need to be modified to become safe, by adding a 
clk_block_rate_change() before the clk_get_rate().


- Paul

Re: [PATCH v3 3/5] clk: introduce the common clock framework

[Russell King - ARM Linux]

On Wed, Nov 30, 2011 at 06:20:50PM -0700, Paul Walmsley wrote:
> 1. When a clock user calls clk_enable() on a clock, the clock framework 
> should prevent other users of the clock from changing the clock's rate.  
> This should persist until the clock user calls clk_disable() (but see also 
> #2 below).  This will ensure that clock users can rely on the rate 
> returned by clk_get_rate(), as long as it's called between clk_enable() 
> and clk_disable().  And since the clock's rate is guaranteed to remain the 
> same during this time, code that cannot tolerate clock rate changes 
> without special handling (such as driver code for external I/O devices) 
> will work safely without further modification.

So, if you have a PLL whose parent clock is not used by anything else.
You want to program it to a certain rate.

You call clk_disable() on the PLL clock.  This walks up the tree and
disables the parent.  You then try to set the rate using clk_set_rate().
clk_set_rate() in this circumstance can't wait for the PLL to lock
because it can't - there's no reference clock for it.

You then call clk_enable().  The PLL now takes its time to lock.  You
can't sleep in clk_enable() because it might be called from atomic
contexts, so you have to spin waiting for this.

Overloading clk_disable/clk_enable in this way is a bad solution to
this problem.

Re: [PATCH v3 3/5] clk: introduce the common clock framework

[Mark Brown]

On Wed, Nov 30, 2011 at 11:39:59PM -0700, Paul Walmsley wrote:

> Clock rate/parent-change notifiers are requirements for DVFS use-cases, 
> and they must be paired with something like the 
> clk_{allow,block}_rate_change() functions to work efficiently.  I intend 
> to comment on this later; it's not a simple problem.  It might be worth 
> noting that Tero and I implemented a simplified version of this for the 
> N900.

I'm thinking that if we're going to have clk_{allow,block}_rate_change()
we may as well make that the main interface to enable rate changes - if
a device wants to change the clock rate it allows rate changes using
that interface rather than by disabling the clocks.  I've got devices
which can do glitch free updates of active clocks so having to disable
would be a real restriction, and cpufreq would have issues with actually
disabling the clock too I expect.

Re: [PATCH v3 3/5] clk: introduce the common clock framework

[Turquette, Mike]

On Thu, Dec 1, 2011 at 6:42 AM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Wed, Nov 30, 2011 at 11:39:59PM -0700, Paul Walmsley wrote:
>
>> Clock rate/parent-change notifiers are requirements for DVFS use-cases,
>> and they must be paired with something like the
>> clk_{allow,block}_rate_change() functions to work efficiently.  I intend
>> to comment on this later; it's not a simple problem.  It might be worth
>> noting that Tero and I implemented a simplified version of this for the
>> N900.
>
> I'm thinking that if we're going to have clk_{allow,block}_rate_change()
> we may as well make that the main interface to enable rate changes - if
> a device wants to change the clock rate it allows rate changes using
> that interface rather than by disabling the clocks.  I've got devices
> which can do glitch free updates of active clocks so having to disable
> would be a real restriction, and cpufreq would have issues with actually
> disabling the clock too I expect.
>

I agree that imposing a "disable clk before changing rate" policy in
the framework core is a bad idea.  During discussion on the
CLK_GATE_SET_RATE flag in the patch #2 Shawn commented that he has
some clks that must be enabled to change their rates (I assume he
means PLLs but that detail doesn't really matter).

Regards,
Mike

Re: [PATCH v3 3/5] clk: introduce the common clock framework

[Paul Walmsley]

Hi Mark,

On Thu, 1 Dec 2011, Mark Brown wrote:

> On Wed, Nov 30, 2011 at 11:39:59PM -0700, Paul Walmsley wrote:
> 
> > Clock rate/parent-change notifiers are requirements for DVFS use-cases, 
> > and they must be paired with something like the 
> > clk_{allow,block}_rate_change() functions to work efficiently.  I intend 
> > to comment on this later; it's not a simple problem.  It might be worth 
> > noting that Tero and I implemented a simplified version of this for the 
> > N900.
> 
> I'm thinking that if we're going to have clk_{allow,block}_rate_change()
> we may as well make that the main interface to enable rate changes - if
> a device wants to change the clock rate it allows rate changes using
> that interface rather than by disabling the clocks.  I've got devices
> which can do glitch free updates of active clocks so having to disable
> would be a real restriction, and cpufreq would have issues with actually
> disabling the clock too I expect.

The intention behind the clk_{allow,block}_rate_change() proposal was to 
allow the current user of the clock to change its rate without having to 
call clk_{allow,block}_rate_change(), if that driver was the sole user of 
the clock.

So for example, if you had a driver that did:

c = clk_get(dev, clk_name);
clk_enable(c);
clk_set_rate(c, clk_rate);

and c was currently not enabled by any other driver on the system, and 
nothing else had called clk_block_rate_change(c), then the rate change 
would be allowed to proceed.  (modulo any notifier activity, etc.)  

So clk_{allow,block}_rate_change() was simply intended to allow or 
restrict other users of the same clock, not the current user.


- Paul

Re: [PATCH v3 3/5] clk: introduce the common clock framework

[Mark Brown]

On Thu, Dec 01, 2011 at 11:30:16AM -0700, Paul Walmsley wrote:

> So for example, if you had a driver that did:

> c = clk_get(dev, clk_name);
> clk_enable(c);
> clk_set_rate(c, clk_rate);

> and c was currently not enabled by any other driver on the system, and 
> nothing else had called clk_block_rate_change(c), then the rate change 
> would be allowed to proceed.  (modulo any notifier activity, etc.)  

> So clk_{allow,block}_rate_change() was simply intended to allow or 
> restrict other users of the same clock, not the current user.

Ah, sorry!  I'd totally misunderstood what you were proposing.

Re: [PATCH v3 3/5] clk: introduce the common clock framework

[Russell King - ARM Linux]

On Thu, Dec 01, 2011 at 11:30:16AM -0700, Paul Walmsley wrote:
> The intention behind the clk_{allow,block}_rate_change() proposal was to 
> allow the current user of the clock to change its rate without having to 
> call clk_{allow,block}_rate_change(), if that driver was the sole user of 
> the clock.

And how does a driver know that?

Re: [PATCH v3 3/5] clk: introduce the common clock framework

[Paul Walmsley]

Hi Russell,

On Thu, 1 Dec 2011, Russell King - ARM Linux wrote:

> On Wed, Nov 30, 2011 at 06:20:50PM -0700, Paul Walmsley wrote:
> > 1. When a clock user calls clk_enable() on a clock, the clock framework 
> > should prevent other users of the clock from changing the clock's rate.  
> > This should persist until the clock user calls clk_disable() (but see also 
> > #2 below).  This will ensure that clock users can rely on the rate 
> > returned by clk_get_rate(), as long as it's called between clk_enable() 
> > and clk_disable().  And since the clock's rate is guaranteed to remain the 
> > same during this time, code that cannot tolerate clock rate changes 
> > without special handling (such as driver code for external I/O devices) 
> > will work safely without further modification.
> 
> So, if you have a PLL whose parent clock is not used by anything else.
> You want to program it to a certain rate.
> 
> You call clk_disable() on the PLL clock.

The approach described wouldn't require the PLL to be disabled before 
changing its rate.  If there are no other users of the PLL, or if the 
other users of the PLL have indicated that it's safe for others to change 
the PLL's rate, the clock framework would allow the PLL rate change, even 
if the PLL is enabled.  (modulo any notifier activity, and assuming that 
the underlying PLL hardware allows its frequency to be reprogrammed while 
the PLL is enabled)

> This walks up the tree and disables the parent.  You then try to set the 
> rate using clk_set_rate(). clk_set_rate() in this circumstance can't 
> wait for the PLL to lock because it can't - there's no reference clock 
> for it.

As an aside, this seems like a good time to mention that the behavior of 
clk_set_rate() on a disabled clock needs to be clarified.


- Paul

Re: [PATCH v3 3/5] clk: introduce the common clock framework

[Russell King - ARM Linux]

On Fri, Dec 02, 2011 at 10:13:10AM -0700, Paul Walmsley wrote:
> Hi Russell,
> 
> On Thu, 1 Dec 2011, Russell King - ARM Linux wrote:
> 
> > On Wed, Nov 30, 2011 at 06:20:50PM -0700, Paul Walmsley wrote:
> > > 1. When a clock user calls clk_enable() on a clock, the clock framework 
> > > should prevent other users of the clock from changing the clock's rate.  
> > > This should persist until the clock user calls clk_disable() (but see also 
> > > #2 below).  This will ensure that clock users can rely on the rate 
> > > returned by clk_get_rate(), as long as it's called between clk_enable() 
> > > and clk_disable().  And since the clock's rate is guaranteed to remain the 
> > > same during this time, code that cannot tolerate clock rate changes 
> > > without special handling (such as driver code for external I/O devices) 
> > > will work safely without further modification.
> > 
> > So, if you have a PLL whose parent clock is not used by anything else.
> > You want to program it to a certain rate.
> > 
> > You call clk_disable() on the PLL clock.
> 
> The approach described wouldn't require the PLL to be disabled before 
> changing its rate.  If there are no other users of the PLL, or if the 
> other users of the PLL have indicated that it's safe for others to change 
> the PLL's rate, the clock framework would allow the PLL rate change, even 
> if the PLL is enabled.  (modulo any notifier activity, and assuming that 
> the underlying PLL hardware allows its frequency to be reprogrammed while 
> the PLL is enabled)
> 
> > This walks up the tree and disables the parent.  You then try to set the 
> > rate using clk_set_rate(). clk_set_rate() in this circumstance can't 
> > wait for the PLL to lock because it can't - there's no reference clock 
> > for it.
> 
> As an aside, this seems like a good time to mention that the behavior of 
> clk_set_rate() on a disabled clock needs to be clarified.

It's more complicated than that.  Clocks have more states than just
enabled and disabled.

There is:
- unprepared
- prepared and disabled
- prepared and enabled

Implementations can chose at which point to enable their PLLs and wait
for them to lock - but if they want to sleep while waiting, they must
do this in the prepare method, not the enable method (remember, enable
is to be callable from atomic contexts.)

So, it's entirely possible that a prepared clock will have the PLLs up
and running, which means that clk_set_rate() can also wait for the PLL
to stablize (which would be a good idea.)

Now... we can say that PLLs should be locked when the prepare method
returns, or whenever clk_set_rate() returns.  The problem with that is
there's a race condition between clk_enable() and clk_set_rate() - if
we allow clk_set_rate() to sleep waiting for the PLL to lock, a
concurrent clk_enable() can't be prevented from returning because
that would involve holding a spinlock...

I think this is just one of those unsolvable issues - if you have
concurrent users of a PLL there's not much you can do - race free - to
prevent the PLL changing beneath some user.

Re: [PATCH v3 3/5] clk: introduce the common clock framework

[Uwe Kleine-König]

Hello,

On Fri, Dec 02, 2011 at 08:23:06PM +0000, Russell King - ARM Linux wrote:
> On Fri, Dec 02, 2011 at 10:13:10AM -0700, Paul Walmsley wrote:
> > Hi Russell,
> > 
> > On Thu, 1 Dec 2011, Russell King - ARM Linux wrote:
> > 
> > > On Wed, Nov 30, 2011 at 06:20:50PM -0700, Paul Walmsley wrote:
> > > > 1. When a clock user calls clk_enable() on a clock, the clock framework 
> > > > should prevent other users of the clock from changing the clock's rate.  
> > > > This should persist until the clock user calls clk_disable() (but see also 
> > > > #2 below).  This will ensure that clock users can rely on the rate 
> > > > returned by clk_get_rate(), as long as it's called between clk_enable() 
> > > > and clk_disable().  And since the clock's rate is guaranteed to remain the 
> > > > same during this time, code that cannot tolerate clock rate changes 
> > > > without special handling (such as driver code for external I/O devices) 
> > > > will work safely without further modification.
> > > 
> > > So, if you have a PLL whose parent clock is not used by anything else.
> > > You want to program it to a certain rate.
> > > 
> > > You call clk_disable() on the PLL clock.
> > 
> > The approach described wouldn't require the PLL to be disabled before 
> > changing its rate.  If there are no other users of the PLL, or if the 
> > other users of the PLL have indicated that it's safe for others to change 
> > the PLL's rate, the clock framework would allow the PLL rate change, even 
> > if the PLL is enabled.  (modulo any notifier activity, and assuming that 
> > the underlying PLL hardware allows its frequency to be reprogrammed while 
> > the PLL is enabled)
> > 
> > > This walks up the tree and disables the parent.  You then try to set the 
> > > rate using clk_set_rate(). clk_set_rate() in this circumstance can't 
> > > wait for the PLL to lock because it can't - there's no reference clock 
> > > for it.
> > 
> > As an aside, this seems like a good time to mention that the behavior of 
> > clk_set_rate() on a disabled clock needs to be clarified.
> 
> It's more complicated than that.  Clocks have more states than just
> enabled and disabled.
> 
> There is:
> - unprepared
> - prepared and disabled
> - prepared and enabled
> 
> Implementations can chose at which point to enable their PLLs and wait
> for them to lock - but if they want to sleep while waiting, they must
> do this in the prepare method, not the enable method (remember, enable
> is to be callable from atomic contexts.)
> 
> So, it's entirely possible that a prepared clock will have the PLLs up
> and running, which means that clk_set_rate() can also wait for the PLL
> to stablize (which would be a good idea.)
> 
> Now... we can say that PLLs should be locked when the prepare method
> returns, or whenever clk_set_rate() returns.  The problem with that is
> there's a race condition between clk_enable() and clk_set_rate() - if
> we allow clk_set_rate() to sleep waiting for the PLL to lock, a
> concurrent clk_enable() can't be prevented from returning because
> that would involve holding a spinlock...
But you can achieve that the concurrent clk_enable fails. Would that be
sane?

Best regards
Uwe

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

Re: [PATCH v3 3/5] clk: introduce the common clock framework

[Paul Walmsley]

On Thu, 1 Dec 2011, Russell King - ARM Linux wrote:

> On Thu, Dec 01, 2011 at 11:30:16AM -0700, Paul Walmsley wrote:
> > The intention behind the clk_{allow,block}_rate_change() proposal was to 
> > allow the current user of the clock to change its rate without having to 
> > call clk_{allow,block}_rate_change(), if that driver was the sole user of 
> > the clock.
> 
> And how does a driver know that?

The driver author might make this assumption when the device's upstream 
clock tree is simple.  This assumption simplifies the driver's clock code, 
since no clock rate change notifier callback would be needed.

This should be possible:

1. when there are no other users of the device's upstream clocks; or,

2. when a subset of the device's upstream clock tree can be used by
   other devices, but the rates or source clocks of that subset either 
   cannot be changed, or should never change.

For example, consider an external audio codec device, such as the 
TLV320AIC23.  The input clock source rate for this chip is likely to be 
fixed for a given board design[1].  So the driver wouldn't need a clock 
rate change notifier callback for its input clock source, since it would 
never be called.  The clock dividers inside the codec may be represented 
by clock nodes, but since they are handled by the same driver, the 
coordination between them doesn't require any notifier callbacks.

Another example would be a device that shares a portion of its upstream 
clock tree with other devices, where the upstream clock tree is designed 
to run at the same rate for all of the devices, such as the USIM IP block 
on OMAP3430/3630[2].  The USIM shares a dedicated DPLL with the USBHOST 
and USBTLL devices, but the DPLL is only intended to be programmed at a 
single rate.  When the USIM is using this DPLL as a clock source, a 
dedicated downstream clock divider is available that can be used to reduce 
the USIM's input clock rate.  So the USIM driver would also not require 
any clock rate change notifier callbacks, since the shared portions of its 
parent clock tree should never change.

Of course, it is possible that a new chip could appear with an identical 
IP block inside it, but where cases 1 and 2 above are no longer true.  
The worst outcome is that the driver's clk_set_rate() may no longer 
succeed, returning -EBUSY, since another device may be blocking the rate 
change.  The remedy is to update the driver to add the rate change 
callback, and the associated internal logic to deal with it.


- Paul

1. Texas Instruments, _TLV320AIC23 Stereo Audio CODEC, 8- to 96-kHz, With 
   Integrated Headphone Amplifier Data Manual_ (SLWS106D), May 2002, 
   Section 3.3.2 "Audio Sampling Rates" and section 1.2 "Functional Block 
   Diagram".   Available from http://www.ti.com/lit/gpn/tlv320aic23
   (among others)

2. Linux kernel v3.1-rc4, arch/arm/mach-omap2/clock3xxx_data.c, 
   usim_clksel (line 2300).  Available from 
   http://git.kernel.org/gitweb.cgi?p=linux/kernel/git/torvalds/linux.git;a=blob;f=arch/arm/mach-omap2/clock3xxx_data.c;h=5d0064a4fb5a638bb1141489354244e95899c1a1;hb=HEAD#l2300
   (among others)

Re: [PATCH v3 3/5] clk: introduce the common clock framework

[Paul Walmsley]

Hi

a brief comment concerning clock rates:

On Mon, 21 Nov 2011, Mike Turquette wrote:

> +unsigned long clk_get_rate(struct clk *clk)

...

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

...

> +int clk_set_rate(struct clk *clk, unsigned long rate)

...

> +struct clk {
...
> +	unsigned long		rate;
...
> +};

The types associated with clock rates in the clock interface 
(include/linux/clk.h) are inconsistent, and we should fix this. 
clk_round_rate() is the problematic case, returning a signed long rather 
than an unsigned long.  So clk_round_rate() won't work correctly with any 
rates higher than 2 GiHz.

We could fix the immediate problem by changing the prototype of 
clk_round_rate() to pass the rounded rate back to the caller via a pointer 
in one of the arguments, and return an error code (if any) via the return 
value:

int clk_round_rate(struct clk *clk, unsigned long rate, unsigned long 
                   *rounded_rate);

But I'd propose that we instead increase the size of struct clk.rate to be 
s64:

s64 clk_round_rate(struct clk *clk, s64 desired_rate);
int clk_set_rate(struct clk *clk, s64 rate);
s64 clk_get_rate(struct clk *clk);

struct clk {
...
     s64 rate;
...
};

That way the clock framework can accommodate current clock rates, as well 
as any conceivable future clock rate.  (Some production CPUs are already 
running at clock rates greater than 4 GiHZ[1].  RF devices with 4 GiHz+ 
clock rates are also common, such as 802.11a devices running in the 5.8 
GHz band, and drivers for those may eventually wish to use the clock 
framework.)



- Paul

1. www.cpu-world.com, "Intel Xeon X5698 - AT80614007314AA" 
   http://www.cpu-world.com/CPUs/Xeon/Intel-Xeon%20X5698%20-%20AT80614007314AA.html

Re: [PATCH v3 3/5] clk: introduce the common clock framework

[Turquette, Mike]

On Mon, Nov 21, 2011 at 5:40 PM, Mike Turquette <mturquette@ti.com> wrote:
> +/**
> + * 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)
> +{
> +       if (!clk)
> +               return NULL;
> +
> +       return clk->parent;
> +}
> +EXPORT_SYMBOL_GPL(clk_get_parent);

While auditing the uses/expectations of the current clk API users, I
see that clk_get_parent is rarely used; it is in fact only called in
11 files throughout the kernel.

I decided to have a little audit and here is what I found:

arch/arm/mach-shmobile/board-ap4evb.c: fsi_ak4642_set_rate
Used within clk_set_rate.  Can dereference clk->parent directly and
ideally should propagate up the clk tree using the new recursive
clk_set_rate.

arch/arm/mach-tegra/usb_phy.c: tegra_usb_phy_open
Used within a clk_get_rate.  pll_u should ideally have it's own
clk->rate populated, reducing the need for tegra_usb_phy_open to know
details of the clk tree.  The logic to pull pll_u's rate from it's
parent belongs to pll_u's .recalc_rate callback.

arch/arm/plat-s5p/clock.c: s5p_spdif_set_rate
Another clk_get_parent call from within a .set_rate callback.  Again:
use clk->parent directly and better yet, propagate the rate change up
via the recursive clk_set_rate.

arch/arm/plat-s5p/clock.c: s5p_spdif_get_rate
Another clk_get_parent call from within a .recalc_rate callback.
Again, clk->rate should be populated with parent's rate correctly,
otherwise dereference clk->parent directly without use of
clk_get_parent.

arch/arm/plat-s5p/s5p-time.c: s5p_clockevent_init
This problem would be solved by propagating rate_change requests up
two-levels of parents via the new recursive clk_set_rate.  There is
also a clk_set_parent call in here, which has nothing to do with the
clk_get_parent call, but it makes me wonder if we should revisit the
issue of switching parents as part of clk_set_rate:
clk_set_rate(tin_event, pclk->rate / 2);

arch/arm/plat-samsung/pwm.c: pwm_is_tdiv
Used in only two places: as part of pwm_calc_tin which could be
replaced wholesale by a better .round_rate implementation and for a
debug print (who cares).

arch/arm/plat-samsung/pwm.c: pwm_calc_tin
Just a .round_rate implementation.  Can dereference clk->parent directly.

arch/arm/plat-samsung/time.c: s3c2410_timer_setup
Same as s5p_clockevent_init above.

drivers/sh/clk/core.c: clk_round_parent
An elaborate .round_rate that should have resulted from recursive
propagation up to clk->parent.

drivers/video/omap2/dss/dss.c:
Every call to clk_get_parent in here is wrapped in clk_get_rate.  The
functions that use this are effectively .set_rate, .get_rate and
.recalc_rate doppelgangers.  Also a debug print calls clk_get_parent
but who cares.

drivers/video/sh_mobile_hdmi.c:
Used in a .set_rate and .round_rate implementation.  No reason not to
deref clk->parent directly.

The above explanations take a little liberty listing certain functions
as .round_rate, .set_rate and .recalc_rate callbacks, but that is
because a lot of the code mentioned could be neatly wrapped up that
way.

Do we really need clk_get_parent?  The primary problem with it is
ambiguity in the API: should the caller hold a lock?  Is the rate
guaranteed not the change after being called?  A top-level clk API
function that can be called within other top-level clk API functions
is inconsitent: most of the time this would incur nested locking.
Also having a top-level API expose the tree structure encourages
platform and driver developers to put clk tree details into their code
as opposed to having clever .round_rate and .set_rate callbacks hide
these details from them with the new recursive clk_set_rate.

Thoughts?

Thanks,
Mike

Re: [PATCH v3 3/5] clk: introduce the common clock framework

[Russell King - ARM Linux]

On Mon, Dec 05, 2011 at 02:15:56PM -0700, Paul Walmsley wrote:
> The types associated with clock rates in the clock interface 
> (include/linux/clk.h) are inconsistent, and we should fix this. 

Rubbish.  They're different with good reason.  Rates are primerily
unsigned quantities - and should be treated as such.

The exception is clk_round_rate() which returns the rate, but also
_may_ return an error.  Therefore, its return type has to be signed.

> We could fix the immediate problem by changing the prototype of 
> clk_round_rate() to pass the rounded rate back to the caller via a pointer 
> in one of the arguments, and return an error code (if any) via the return 
> value:
> 
> int clk_round_rate(struct clk *clk, unsigned long rate, unsigned long 
>                    *rounded_rate);

Yes that might have been a better solution.

> But I'd propose that we instead increase the size of struct clk.rate to be 
> s64:
> 
> s64 clk_round_rate(struct clk *clk, s64 desired_rate);
> int clk_set_rate(struct clk *clk, s64 rate);
> s64 clk_get_rate(struct clk *clk);
> 
> struct clk {
> ...
>      s64 rate;
> ...
> };
> 
> That way the clock framework can accommodate current clock rates, as well 
> as any conceivable future clock rate.  (Some production CPUs are already 
> running at clock rates greater than 4 GiHZ[1].  RF devices with 4 GiHz+ 
> clock rates are also common, such as 802.11a devices running in the 5.8 
> GHz band, and drivers for those may eventually wish to use the clock 
> framework.)

Yuck.  You are aware that 64-bit math on 32-bit CPUs sucks?  So burdening
_everything_ with 64-bit rate quantities is absurd.  As for making then
64-bit signed quantities, that's asking for horrid code from gcc for the
majority of cases.

Re: [PATCH v3 3/5] clk: introduce the common clock framework

[Paul Walmsley]

On Mon, 5 Dec 2011, Russell King - ARM Linux wrote:

> On Mon, Dec 05, 2011 at 02:15:56PM -0700, Paul Walmsley wrote:
>
> > But I'd propose that we instead increase the size of struct clk.rate to be 
> > s64:
> > 
> > s64 clk_round_rate(struct clk *clk, s64 desired_rate);
> > int clk_set_rate(struct clk *clk, s64 rate);
> > s64 clk_get_rate(struct clk *clk);
> > 
> > struct clk {
> > ...
> >      s64 rate;
> > ...
> > };
> > 
> > That way the clock framework can accommodate current clock rates, as well 
> > as any conceivable future clock rate.  (Some production CPUs are already 
> > running at clock rates greater than 4 GiHZ[1].  RF devices with 4 GiHz+ 
> > clock rates are also common, such as 802.11a devices running in the 5.8 
> > GHz band, and drivers for those may eventually wish to use the clock 
> > framework.)
> 
> Yuck.  You are aware that 64-bit math on 32-bit CPUs sucks? So burdening 
> _everything_ with 64-bit rate quantities is absurd.  As for making then 
> 64-bit signed quantities, that's asking for horrid code from gcc for the 
> majority of cases.

64-bit divides would be the only real issue in the clock framework 
context.  And those are easily avoided on clock hardware where the parent 
rate can never exceed 32 bits.

For example, here's a trivial implementation for rate recalculation for a 
integer divider clock node (that can't be handled with a right shift):

s64 div(struct clk *clk, u32 div) {
	if (clk->flags & CLK_PARENT_RATE_MAX_U32)
		return ((u32)(clk->parent->rate & 0xffffffff)) / div;

	clk->rate = clk->parent->rate;
	do_div(clk->rate, div);
	return clk->rate;
}

gcc generates this for ARMv6:

00000010 <div>:
  10:	e92d4038 	push	{r3, r4, r5, lr}
  14:	e1a05000 	mov	r5, r0         
  18:	e5d03010 	ldrb	r3, [r0, #16]  @ clk->flags
  1c:	e1a04001 	mov	r4, r1
  20:	e3130001 	tst	r3, #1         @ 32-bit parent rate?
  24:	1a000007 	bne	48 <div+0x38>  @ do 32-bit divide 

/* 64-bit divide by 32-bit follows */

  28:	e5903000 	ldr	r3, [r0]
  2c:	e1c300d8 	ldrd	r0, [r3, #8]
  30:	ebfffffe 	bl	0 <__do_div64>
  34:	e1a00002 	mov	r0, r2
  38:	e1a01003 	mov	r1, r3
  3c:	e5852008 	str	r2, [r5, #8]
  40:	e585300c 	str	r3, [r5, #12]
  44:	e8bd8038 	pop	{r3, r4, r5, pc}

/* 32-bit divide follows */

  48:	e5900000 	ldr	r0, [r0]
  4c:	e5900008 	ldr	r0, [r0, #8]
  50:	ebfffffe 	bl	0 <__aeabi_uidiv>
  54:	e3a01000 	mov	r1, #0
  58:	e8bd8038 	pop	{r3, r4, r5, pc}


Not bad at all, and this isn't even optimized.  (The conditional could be 
avoided completely with a little work during clock init.)  What little 
overhead there is seems quite trivial if it means that the clock framework 
will be useful for present and future devices.


- Paul

Re: [PATCH v3 3/5] clk: introduce the common clock framework

[Paul Walmsley]

On Mon, 5 Dec 2011, Paul Walmsley wrote:

> For example, here's a trivial implementation for rate recalculation for a 
> integer divider clock node (that can't be handled with a right shift):
> 
> s64 div(struct clk *clk, u32 div) {
> 	if (clk->flags & CLK_PARENT_RATE_MAX_U32)
> 		return ((u32)(clk->parent->rate & 0xffffffff)) / div;
> 
> 	clk->rate = clk->parent->rate;
> 	do_div(clk->rate, div);
> 	return clk->rate;
> }

(removing some useless cruft from the above function, for clarity's sake)

s64 div(struct clk *clk, u32 div) {
	s64 r;

	if (clk->flags & CLK_PARENT_RATE_MAX_U32)
		return ((u32)clk->parent->rate) / div;

	r = clk->parent->rate;
	do_div(r, div);
	return r;
}

00000000 <div>:
   0:	e92d4010 	push	{r4, lr}
   4:	e1a04001 	mov	r4, r1
   8:	e5d03010 	ldrb	r3, [r0, #16]
   c:	e3130001 	tst	r3, #1
  10:	1a000005 	bne	2c <div+0x2c>

  14:	e5903000 	ldr	r3, [r0]
  18:	e1c300d8 	ldrd	r0, [r3, #8]
  1c:	ebfffffe 	bl	0 <__do_div64>
  20:	e1a00002 	mov	r0, r2
  24:	e1a01003 	mov	r1, r3
  28:	e8bd8010 	pop	{r4, pc}

  2c:	e5900000 	ldr	r0, [r0]
  30:	e5900008 	ldr	r0, [r0, #8]
  34:	ebfffffe 	bl	0 <__aeabi_uidiv>
  38:	e3a01000 	mov	r1, #0
  3c:	e8bd8010 	pop	{r4, pc}


- Paul