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%3Ba=shortlog%3Bh=...
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
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
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
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.
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 Turquettemturquette@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 FrameworkMike 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
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
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.
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
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);
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 Ltdjeremy.kerr@canonical.com
- Copyright (C) 2011 Linaro Ltdmturquette@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
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
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
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
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
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:
- 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.
- 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.
- 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!).
- 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
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.
- 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?
- 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
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.
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
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
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.
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?
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%3Ba=b... (among others)
On Wed, Nov 30, 2011 at 06:20:50PM -0700, Paul Walmsley wrote:
- 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.
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:
- 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
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:
- 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.
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:
- 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
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
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....
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.
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
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
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
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
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
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
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
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'.
+}
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
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);
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 */
/**
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
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
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
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
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:
- have kobject core create the lists linking the objects but defer
allocations and any interactions with sysfs until later in the boot sequence, OR
- 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
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:
- have kobject core create the lists linking the objects but defer
allocations and any interactions with sysfs until later in the boot sequence, OR
- 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
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:
- have kobject core create the lists linking the objects but defer
allocations and any interactions with sysfs until later in the boot sequence, OR
- 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
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:
- 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..
- 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
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.
* 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
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.
* 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
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.
* 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
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.
g.
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_CLKhelpCreates clock tree represenation in sysfs. Directory namesand hierarchy represent clock names and tree structure,respectively. Each directory exports clock rate, flags,prepare_count and enable_count info as read-only for debugpurposes.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);elseret = -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;elseparent_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
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.
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.
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
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
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
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.
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
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
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.
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.
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
-
Arnd Bergmann -
Arnd Bergmann -
Grant Likely -
Greg KH -
Mark Brown -
Mark Salter -
Mike Turquette -
Mike Turquette -
Paul Walmsley -
Russell King - ARM Linux -
Saravana Kannan -
Shawn Guo -
Tony Lindgren -
Turquette, Mike
-
Uwe Kleine-König