Add a generic struct clk
Subsystems (4)
cover Add a generic struct clk
[This series was originally titled 'Add a common struct clk', but the goals have changed since that first set of patches. We're now aiming for a more complete generic clock infrastructure, rather than just abstracting struct clk]
[This series still needs work, see the TODO section below]
[Totally RFC at the moment]
Hi all,
These patches are an attempt to allow platforms to share clock code. At present, the definitions of 'struct clk' are local to platform code, which makes allocating and initialising cross-platform clock sources difficult, and makes it impossible to compile a single image containing support for two ARM platforms with different struct clks.
The three patches are for the architecture-independent kernel code, introducing the common clk infrastructure. The changelog for the first patch includes details about the new clock definitions.
The second patch implements clk_set_rate, and in doing so adds functionality to walk the clock tree in both directions.
clk_set_parent is left unimplemented, see TODO below for why.
The third and fourth patches provide some basic clocks (fixed-rate and gated), mostly to serve as an example of the hardware implementation. I'm intending to later provide similar base clocks for mux and divider hardware clocks.
Many thanks to the following for their input:
* Benjamin Herrenschmidt <[email protected]>
* Thomas Gleixner <[email protected]>
* Ben Dooks <[email protected]>
* Baruch Siach <[email protected]>
* Russell King <[email protected]>
* Uwe Kleine-K?nig <[email protected]>
* Lorenzo Pieralisi <[email protected]>
* Vincent Guittot <[email protected]>
* Sascha Hauer <[email protected]>
* Ryan Mallon <[email protected]>
* Colin Cross <[email protected]>
* Jassi Brar <[email protected]>
* Saravana Kannan <[email protected]>
TODO:
* Need to figure out the locking around clk_set_parent. Changing the in-kernel clock topology requires acquiring both the mutex (to prevent against races with clk_prepare, which may propagate to the parent clock) and the spinlock (to prevent the same race with clk_enable).
However, we should also be changing the hardware clock topology in sync with the in-kernel clock topology, which would imply that both locks *also* need to be held while updating the parent in the hardware (ie, in clk_hw_ops->set_parent) too. However, I believe some platform clock implementations may require this callback to be able to sleep. Comments?
* tglx and I have been discussing different ways of passing clock information to the clock hardware implementation. I'm proposing providing a base 'struct clk_hw', which implementations subclass by wrapping in their own structure (or one of a set of simple 'library' structures, like clk_hw_mux or clk_hw_gate). The clk_hw base is passed as the first argument to all clk_hw_ops callbacks.
tglx's plan is to create a separate struct clk_hwdata, which contains a union of base data structures for common clocks: div, mux, gate, etc. The ops callbacks are passed a clk_hw, plus a clk_hwdata, and most of the base hwdata fields are handled within the core clock code. This means less encapsulation of clock implementation logic, but more coverage of clock basics through the core code.
tglx, could you send some details about your approach? I'm aiming to refine this series with the good bits from each technique.
* The clock registration code probably needs work. This is the domain of the platform/board maintainers, any wishes here?
Cheers,
Jeremy
--
---
Jeremy Kerr (4):
clk: Add a generic clock infrastructure
clk: Implement clk_set_rate
clk: Add fixed-rate clock
clk: Add simple gated clock
PATCH 1/4 clk: Add a generic clock infrastructure
We currently have ~21 definitions of struct clk in the ARM architecture, each defined on a per-platform basis. This makes it difficult to define platform- (or architecture-) independent clock sources without making assumptions about struct clk, and impossible to compile two platforms with different struct clks into a single image.
This change is an effort to unify struct clk where possible, by defining a common struct clk, and a set of clock operations. Different clock implementations can set their own operations, and have a standard interface for generic code. The callback interface is exposed to the kernel proper, while the clock implementations only need to be seen by the platform internals.
The interface is split into two halves:
* struct clk, which is the generic-device-driver interface. This provides a set of functions which drivers may use to request enable/disable, query or manipulate in a hardware-independent manner.
* struct clk_hw and struct clk_hw_ops, which is the hardware-specific interface. Clock drivers implement the ops, which allow the core clock code to implement the generic 'struct clk' API.
This allows us to share clock code among platforms, and makes it possible to dynamically create clock devices in platform-independent code.
Platforms can enable the generic struct clock through CONFIG_GENERIC_CLK. In this case, the clock infrastructure consists of a common, opaque struct clk, and a set of clock operations (defined per type of clock):
struct clk_hw_ops {
int (*prepare)(struct clk_hw *);
void (*unprepare)(struct clk_hw *);
int (*enable)(struct clk_hw *);
void (*disable)(struct clk_hw *);
unsigned long (*recalc_rate)(struct clk_hw *);
int (*set_rate)(struct clk_hw *,
unsigned long, unsigned long *);
long (*round_rate)(struct clk_hw *, unsigned long);
int (*set_parent)(struct clk_hw *, struct clk *);
struct clk * (*get_parent)(struct clk_hw *);
};Platform clock code can register a clock through clk_register, passing a set of operations, and a pointer to hardware-specific data:
struct clk_hw_foo {
struct clk_hw clk;
void __iomem *enable_reg;
};
#define to_clk_foo(c) offsetof(c, clk_hw_foo, clk)
static int clk_foo_enable(struct clk_hw *clk)
{
struct clk_foo *foo = to_clk_foo(clk);
raw_writeb(foo->enable_reg, 1);
return 0;
}
struct clk_hw_ops clk_foo_ops = {
.enable = clk_foo_enable,
};And in the platform initialisation code:
struct clk_foo my_clk_foo;
void init_clocks(void)
{
my_clk_foo.enable_reg = ioremap(...);
clk_register(&clk_foo_ops, &my_clk_foo, NULL);
}Changes from Thomas Gleixner <[email protected]>.
The common clock definitions are based on a development patch from Ben Herrenschmidt <[email protected]>.
TODO:
* We don't keep any internal reference to the clock topology at present.
--- drivers/clk/Kconfig | 3 drivers/clk/Makefile | 1 drivers/clk/clk.c | 229 +++++++++++++++++++++++++++++++++++++++++++ drivers/clk/clkdev.c | 7 + include/linux/clk.h | 102 +++++++++++++++++-- 5 files changed, 332 insertions(+), 10 deletions(-)
drivers/clk/Kconfig +3 -0
@@ -2,3 +2,6 @@ config CLKDEV_LOOKUP bool select HAVE_CLK + +config GENERIC_CLK + bool
drivers/clk/Makefile +1 -0
@@ -1,2 +1,3 @@ obj-$(CONFIG_CLKDEV_LOOKUP) += clkdev.o +obj-$(CONFIG_GENERIC_CLK) += clk.o
drivers/clk/clk.c new file +229 -0
229 lines not on this page · raw message
drivers/clk/clkdev.c +7 -0
@@ -23,6 +23,13 @@static LIST_HEAD (clocks);static DEFINE_MUTEX (clocks_mutex); +/* For USE_COMMON_STRUCT_CLK, these are provided in clk.c, but not exported +* through other headers; we don't want them used anywhere but here. */ +#ifdef CONFIG_USE_COMMON_STRUCT_CLK +extern int __clk_get (struct clk *clk); +extern void __clk_put (struct clk *clk); +#endif +/* * Find the correct struct clk for the device and connection ID. * We do slightly fuzzy matching here:
include/linux/clk.h +92 -10
133 lines not on this page · raw message
Re: [PATCH 1/4] clk: Add a generic clock infrastructure
Hi Jeremy,
Nice to see progress in this area :)
Quoted 4 lines · Jeremy Kerr
drivers/clk/clk.c new file +4
-0+ +struct clk *clk_register (struct clk_hw_ops *ops,struct clk_hw *hw, +const char *name) +{
drivers/clk/clk.c line 214
Quoted 10 lines
drivers/clk/clk.c new file +10
-0+struct clk *clk; + + clk =kzalloc (sizeof (*clk),GFP_KERNEL ); +if (!clk) +return NULL ; + + clk->name = name; + clk->ops = ops; + clk->hw = hw; + hw->clk = clk;
is it worth to add a CLK_REGISTERED flag here? Up to now clocks do not have to be registered at all and the registering might be forgotten for some clocks. I suppose funny things can happen when we operate on unregistered clocks.
drivers/clk/clk.c line 224
Quoted 10 lines
drivers/clk/clk.c new file +10
-0+ +/* Query the hardware for parent and initial rate */ + +if (clk->ops->get_parent) +/* We don't to lock against prepare/enable here, as +* the clock is not yet accessible from anywhere */ + clk->parent = clk->ops->get_parent (clk->hw); + +if (clk->ops->recalc_rate) + clk->rate = clk->ops->recalc_rate (clk->hw);
This implicitely requires that we never register a clock before its parent is registered. This makes perfect sense but I think we should catch it here when the user tries to register a clock with a not yet registered parent.
drivers/clk/clk.c line 229
Quoted 5 lines
drivers/clk/clk.c new file +5
-0+ + +return clk; +} +EXPORT_SYMBOL_GPL (clk_register);
BTW we planned to let the linker do the job of registering static clocks, but this is only convenience and can be added later.
Sascha
Signature · 4 lines
Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Re: [PATCH 1/4] clk: Add a generic clock infrastructure
Quoted 16 lines · Sascha Hauer
Hi Jeremy,
Nice to see progress in this area :)
Quoted 11 lines · Jeremy Kerr
drivers/clk/clk.c new file +11
-0+ +struct clk *clk_register (struct clk_hw_ops *ops,struct clk_hw *hw, +const char *name) +{ +struct clk *clk; + + clk =kzalloc (sizeof (*clk),GFP_KERNEL ); +if (!clk) +return NULL ; + + clk->name = name;
Quoted 8 lines
drivers/clk/clk.c line 214
Quoted 3 lines
drivers/clk/clk.c new file +3
-0+ clk->ops = ops; + clk->hw = hw; + hw->clk = clk;is it worth to add a
CLK_REGISTEREDflag here? Up to now clocks do not have to be registered at all and the registering might be forgotten for some clocks. I suppose funny things can happen when we operate on unregistered clocks.
No, because not registered clocks are not accessible :)
Quoted 15 lines
+
drivers/clk/clk.c line 224
Quoted 9 lines
drivers/clk/clk.c new file +9
-0+/* Query the hardware for parent and initial rate */ + +if (clk->ops->get_parent) +/* We don't to lock against prepare/enable here, as +* the clock is not yet accessible from anywhere */ + clk->parent = clk->ops->get_parent (clk->hw); + +if (clk->ops->recalc_rate) + clk->rate = clk->ops->recalc_rate (clk->hw);This implicitely requires that we never register a clock before its parent is registered. This makes perfect sense but I think we should catch it here when the user tries to register a clock with a not yet registered parent.
We can agree on that, but we can make it more clever than that.
Quoted 8 lines
drivers/clk/clk.c line 229
Quoted 5 lines
drivers/clk/clk.c new file +5
-0+ + +return clk; +} +EXPORT_SYMBOL_GPL (clk_register);BTW we planned to let the linker do the job of registering static clocks, but this is only convenience and can be added later.
Right, you just need a helper function which runs through the list of static stuff.
Re: [PATCH 1/4] clk: Add a generic clock infrastructure
Quoted 27 lines · Thomas Gleixner
Quoted 16 lines · Sascha Hauer
Hi Jeremy,
Nice to see progress in this area :)
Quoted 11 lines · Jeremy Kerr
drivers/clk/clk.c new file +11
-0+ +struct clk *clk_register (struct clk_hw_ops *ops,struct clk_hw *hw, +const char *name) +{ +struct clk *clk; + + clk =kzalloc (sizeof (*clk),GFP_KERNEL ); +if (!clk) +return NULL ; + + clk->name = name;Quoted 8 lines
drivers/clk/clk.c line 214
Quoted 3 lines
drivers/clk/clk.c new file +3
-0+ clk->ops = ops; + clk->hw = hw; + hw->clk = clk;is it worth to add a
CLK_REGISTEREDflag here? Up to now clocks do not have to be registered at all and the registering might be forgotten for some clocks. I suppose funny things can happen when we operate on unregistered clocks.
Quoted 1 line
No, because not registered clocks are not accessible :)
D'oh :)
Quoted 17 lines
Quoted 16 lines
+
drivers/clk/clk.c line 224
Quoted 9 lines
drivers/clk/clk.c new file +9
-0+/* Query the hardware for parent and initial rate */ + +if (clk->ops->get_parent) +/* We don't to lock against prepare/enable here, as +* the clock is not yet accessible from anywhere */ + clk->parent = clk->ops->get_parent (clk->hw); + +if (clk->ops->recalc_rate) + clk->rate = clk->ops->recalc_rate (clk->hw);This implicitely requires that we never register a clock before its parent is registered. This makes perfect sense but I think we should catch it here when the user tries to register a clock with a not yet registered parent.
Quoted 1 line
We can agree on that, but we can make it more clever than that.
Great.
Sascha
Signature · 4 lines
Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Re: [PATCH 1/4] clk: Add a generic clock infrastructure
In reply to Jeremy KerrQuoted 32 lines · Jeremy Kerr
We currently have ~21 definitions of struct clk in the ARM architecture, each defined on a per-platform basis. This makes it difficult to define platform- (or architecture-) independent clock sources without making assumptions about struct clk, and impossible to compile two platforms with different struct clks into a single image.
This change is an effort to unify struct clk where possible, by defining a common struct clk, and a set of clock operations. Different clock implementations can set their own operations, and have a standard interface for generic code. The callback interface is exposed to the kernel proper, while the clock implementations only need to be seen by the platform internals.
The interface is split into two halves:
?* struct clk, which is the generic-device-driver interface. This ? provides a set of functions which drivers may use to request ? enable/disable, query or manipulate in a hardware-independent manner.
?* struct
clk_hwand structclk_hw_ops, which is the hardware-specific ? interface. Clock drivers implement the ops, which allow the core ? clock code to implement the generic 'struct clk' API.This allows us to share clock code among platforms, and makes it possible to dynamically create clock devices in platform-independent code.
Platforms can enable the generic struct clock through
CONFIG_GENERIC_CLK. In this case, the clock infrastructure consists of a common, opaque struct clk, and a set of clock operations (defined per type of clock):
Quoted 12 lines
?struct
clk_hw_ops{
? ? ? ?int ? ? ? ? ? ? (*prepare)(structclk_hw*);
? ? ? ?void ? ? ? ? ? ?(*unprepare)(structclk_hw*);
? ? ? ?int ? ? ? ? ? ? (*enable)(structclk_hw*);
? ? ? ?void ? ? ? ? ? ?(*disable)(structclk_hw*);
? ? ? ?unsigned long ? (*recalc_rate)(structclk_hw*);
? ? ? ?int ? ? ? ? ? ? (*set_rate)(structclk_hw*,
? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?unsigned long, unsigned long *); ? ? ? ?long ? ? ? ? ? ?(*round_rate)(structclk_hw*, unsigned long); ? ? ? ?int ? ? ? ? ? ? (*set_parent)(structclk_hw*, struct clk *); ? ? ? ?struct clk * ? ?(*get_parent)(structclk_hw*);
?};
You might want to split these into three separate structs, for mux ops, rate ops, and gate ops. That way, multiple building blocks (a gate and a divider, for example), can be easily combined into a single clock node. Also, an init op that reads the clock tree state from the hardware has been very useful on Tegra - there are often clocks that you can't or won't change, and being able to view their state as configured by the bootloader, and base other clocks off of them, is helpful. It also allows you to turn off clocks that are enabled by the bootloader, but never enabled by the kernel (enabled=1, enable_count=0).
Also, OMAP currently has a second level of global ops that are called before the per-clk ops, which handle the common parts of clock enable (the "clockdomains" part), which I modeled as each clk having a clk_chip, and the clk_chip having some ops. It does add a lot of complexity to the error handling, and OMAP doesn't have very many op implementations, so unless other architectures need it, I don't see a problem pushing those down into each op implementation.
Quoted 90 lines
Platform clock code can register a clock through
clk_register, passing a set of operations, and a pointer to hardware-specific data:?struct
clk_hw_foo{
? ? ? ?structclk_hwclk;
? ? ? ?void__iomem*enable_reg;
?};?#define
to_clk_foo(c)offsetof(c, clk_hw_foo, clk)?static int
clk_foo_enable(struct clk_hw *clk)
?{
? ? ? ?structclk_foo*foo =to_clk_foo(clk);
? ? ? ?raw_writeb(foo->enable_reg, 1);
? ? ? ?return 0;
?}?struct
clk_hw_opsclk_foo_ops= {
? ? ? ?.enable =clk_foo_enable,
?};And in the platform initialisation code:
?struct
clk_foomy_clk_foo;?void
init_clocks(void)
?{
? ? ? ?my_clk_foo.enable_reg = ioremap(...);? ? ? ?clk_register(&clk_foo_ops,
&my_clk_foo, NULL); ?}Changes from Thomas Gleixner <[email protected]>.
The common clock definitions are based on a development patch from Ben Herrenschmidt <[email protected]>.
TODO:
?* We don't keep any internal reference to the clock topology at present.
Signed-off-by Jeremy Kerr <[email protected]>Signed-off-by Thomas Gleixner <[email protected]>---
?drivers/clk/Kconfig ?| ? ?3 ?drivers/clk/Makefile | ? ?1 ?drivers/clk/clk.c ? ?| ?229 +++++++++++++++++++++++++++++++++++++++++++ ?drivers/clk/clkdev.c | ? ?7 + ?include/linux/clk.h ?| ?102 +++++++++++++++++--?5 files changed, 332 insertions(+), 10 deletions(-)
drivers/clk/Kconfig +0
-0@@ -2,3 +2,6 @@?config
CLKDEV_LOOKUP
? ? ? ?bool
? ? ? ?selectHAVE_CLKdrivers/clk/Kconfig +3
-0+ +config GENERIC_CLK + ? ? ? booldrivers/clk/Makefile +0
-0@@ -1,2 +1,3 @@?obj-$(CONFIG_CLKDEV_LOOKUP) ? ?+= clkdev.o
drivers/clk/Makefile +1
-0+obj-$(CONFIG_GENERIC_CLK) ? ? ?+= clk.odrivers/clk/clk.c new file +13
-0@@ -0,0 +1,229 @@ +/* +* Copyright (C) 2010-2011 Canonical Ltd <[email protected]> +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License version 2 as +* published by the Free Software Foundation. +* +* Standard functionality for the common clock API. +*/ + +#include <linux/clk.h> +#include <linux/module.h> +#include <linux/mutex.h>
drivers/clk/clk.c line 23
Quoted 10 lines
drivers/clk/clk.c new file +10
-0+#include <linux/slab.h> +#include <linux/spinlock.h> + +struct clk { + ? ? ? const char ? ? ? ? ? ? ?*name; + ? ? ? struct clk_hw_ops ? ? ? *ops; + ? ? ? struct clk_hw ? ? ? ? ? *hw; + ? ? ? unsigned int ? ? ? ? ? ?enable_count; + ? ? ? unsigned int ? ? ? ? ? ?prepare_count; + ? ? ? struct clk ? ? ? ? ? ? ?*parent;
Storing the list of possible parents at this level can help abstract some common code from mux ops if you pass the index into the list of the new parent into the op - most mux ops only need to know which of their mux inputs needs to be enabled.
drivers/clk/clk.c line 24
Quoted 1 line
drivers/clk/clk.c new file +1
-0+ ? ? ? unsigned long ? ? ? ? ? rate;
If you add an init op, an enabled flag here is also useful to track whether the bootloader left the clock on, which allows turning off unnecessary clocks.
I think you also need a list of current children of the clock to allow propagating rate changes from parent to children.
drivers/clk/clk.c line 28
Quoted 4 lines
drivers/clk/clk.c new file +4
-0+}; + +static DEFINE_SPINLOCK (enable_lock); +static DEFINE_MUTEX (prepare_lock);
There was some discussion earlier of per-tree locking instead of global locking. I have a clock tree that does per-tree locking, as well as runtime addition of clocks to the tree (for example, a codec chip that is complex enough to treat the internal clocks using the clk api, but fed off a clock output from the main SoC), but it adds a ton of complexity to the locking.
Quoted 9 lines
drivers/clk/clk.c new file +9
-0+ +static void __clk_unprepare (struct clk *clk) +{ + ? ? ? if (!clk) + ? ? ? ? ? ? ? return; + + ? ? ? if (WARN_ON(clk->prepare_count == 0)) + ? ? ? ? ? ? ? return; +
drivers/clk/clk.c line 47
Quoted 10 lines
drivers/clk/clk.c new file +10
-0+ ? ? ? if (--clk->prepare_count > 0) + ? ? ? ? ? ? ? return; + + ? ? ? WARN_ON(clk->enable_count > 0); + + ? ? ? if (clk->ops->unprepare) + ? ? ? ? ? ? ? clk->ops->unprepare(clk->hw); + + ? ? ? __clk_unprepare(clk->parent); +}
Are there any cases where the unlocked versions of the clk calls need to be exposed to the ops implementations? For example, a set_rate op may want to call clk_set_parent on itself to change its parent to a better source, and then set its rate. It would need to call __clk_set_parent to avoid deadlocking on the prepare_lock.
Quoted 119 lines
drivers/clk/clk.c new file +119
-0+void clk_unprepare (struct clk *clk) +{ + ? ? ? mutex_lock(&prepare_lock); + ? ? ? __clk_unprepare(clk); + ? ? ? mutex_unlock(&prepare_lock); +} +EXPORT_SYMBOL_GPL (clk_unprepare); + +static 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->hw); + ? ? ? ? ? ? ? ? ? ? ? if (ret) { + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? __clk_unprepare(clk->parent); + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? return ret; + ? ? ? ? ? ? ? ? ? ? ? } + ? ? ? ? ? ? ? } + ? ? ? } + + ? ? ? clk->prepare_count++; + + ? ? ? return 0; +} + +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); + +static 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->hw); + ? ? ? __clk_disable(clk->parent); +} + +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); + +static int __clk_enable (struct clk *clk) +{ + ? ? ? int ret; + + ? ? ? if (!clk) + ? ? ? ? ? ? ? return 0; + + ? ? ? if (WARN_ON(clk->prepare_count == 0)) + ? ? ? ? ? ? ? return -ESHUTDOWN; + + + ? ? ? if (clk->enable_count == 0) { + ? ? ? ? ? ? ? ret = __clk_enable(clk->parent); + ? ? ? ? ? ? ? if (ret) + ? ? ? ? ? ? ? ? ? ? ? return ret; + + ? ? ? ? ? ? ? if (clk->ops->enable) { + ? ? ? ? ? ? ? ? ? ? ? ret = clk->ops->enable(clk->hw); + ? ? ? ? ? ? ? ? ? ? ? if (ret) { + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? __clk_disable(clk->parent); + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? return ret; + ? ? ? ? ? ? ? ? ? ? ? } + ? ? ? ? ? ? ? } + ? ? ? } + + ? ? ? clk->enable_count++; + ? ? ? return 0; +} + +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); + +unsigned long clk_get_rate (struct clk *clk) +{ + ? ? ? if (!clk) + ? ? ? ? ? ? ? return 0; + ? ? ? return clk->rate;
drivers/clk/clk.c line 176
Quoted 10 lines
drivers/clk/clk.c new file +10
-0+} +EXPORT_SYMBOL_GPL (clk_get_rate); + +long clk_round_rate (struct clk *clk,unsigned long rate) +{ + ? ? ? if (clk && clk->ops->round_rate) + ? ? ? ? ? ? ? return clk->ops->round_rate(clk->hw, rate); + ? ? ? return rate; +} +EXPORT_SYMBOL_GPL (clk_round_rate);
I think you should hold the prepare mutex around calls to clk_round_rate, which will allow some code simplification similar to what Russell suggested in another thread. If you hold the mutex here, as well as in clk_set_rate, and you call the round_rate op before the set_rate op in clk_set_rate, op implementers can compute the rate in their round_rate op, and save the register values needed to get that rate in private temporary storage. The set_rate op can then assume that those register values are correct, because the lock is still held, and just write them. That moves all the computation to one place, and it only needs to run once.
drivers/clk/clk.c line 184
Quoted 6 lines
drivers/clk/clk.c new file +6
-0+int clk_set_rate (struct clk *clk,unsigned long rate) +{ + ? ? ? /* not yet implemented */ + ? ? ? return -ENOSYS; +} +EXPORT_SYMBOL_GPL (clk_set_rate);
Here's my implementation, slightly modified and untested to work with your variable names (sorry if whitespace gets mangled):
/*
* called on a clock when it or any of its ancestors change parents or rates
* must be called with prepare_lock held
*/
static void _propagate_rate(struct clk *clk)
{
struct clk *child;
if (clk->ops->recalc_rate)
clk->rate = clk->ops->recalc_rate(clk);
else if (clk->parent)
clk->rate = clk->parent->rate;
list_for_each_entry(child, &clk->children, parent_node)
_propagate_rate(child);
}long __clk_round_rate(struct clk *clk, unsigned long rate)
{
if (clk->ops->round_rate)
return clk->ops->round_rate(clk, rate);
return clk->rate;
}int __clk_set_rate(struct clk *clk, unsigned long rate)
{
long new_rate;
int ret;
if (!clk->ops->set_rate)
return -ENOSYS;
new_rate = __clk_round_rate(clk, rate);
if (new_rate < 0)
return new_rate;
ret = clk->ops->set_rate(clk, new_rate);
if (ret)
return ret;
clk->rate = new_rate;
_propagate_rate(clk);
return 0;
}int clk_set_rate(struct clk *clk, unsigned long rate)
{
int ret;
mutex_lock(&prepare_lock);
ret = __clk_set_rate(clk, rate);
mutex_unlock(prepare_lock);
return ret;
}Quoted 34 lines
drivers/clk/clk.c new file +34
-0+struct clk *clk_get_parent (struct clk *clk) +{ + ? ? ? if (!clk) + ? ? ? ? ? ? ? return NULL; + + ? ? ? return clk->parent; +} +EXPORT_SYMBOL_GPL (clk_get_parent); + +int clk_set_parent (struct clk *clk,struct clk *parent) +{ + ? ? ? /* not yet implemented */ + ? ? ? return -ENOSYS; +} +EXPORT_SYMBOL_GPL (clk_set_parent); + +struct clk *clk_register (struct clk_hw_ops *ops,struct clk_hw *hw, + ? ? ? ? ? ? ? const char *name) +{ + ? ? ? struct clk *clk; + + ? ? ? clk = kzalloc(sizeof(*clk), GFP_KERNEL); + ? ? ? if (!clk) + ? ? ? ? ? ? ? return NULL; + + ? ? ? clk->name = name; + ? ? ? clk->ops = ops; + ? ? ? clk->hw = hw; + ? ? ? hw->clk = clk; + + ? ? ? /* Query the hardware for parent and initial rate */ + + ? ? ? if (clk->ops->get_parent) + ? ? ? ? ? ? ? /* We don't to lock against prepare/enable here, as
drivers/clk/clk.c line 229
Quoted 10 lines
drivers/clk/clk.c new file +10
-0+? ? ? ? ? ? ? ?* the clock is not yet accessible from anywhere */ + ? ? ? ? ? ? ? clk->parent = clk->ops->get_parent(clk->hw); + + ? ? ? if (clk->ops->recalc_rate) + ? ? ? ? ? ? ? clk->rate = clk->ops->recalc_rate(clk->hw); + + + ? ? ? return clk; +} +EXPORT_SYMBOL_GPL (clk_register);
If you are requiring clk's parents (or possible parents?) to be registered before clk, you could put the clk_lookup struct inside the clk struct and call clkdev_add from clk_register, saving some boilerplate in the platforms.
Quoted 66 lines
drivers/clk/clkdev.c +7
-0@@ -23,6 +23,13 @@ ?static LIST_HEAD(clocks); ?static DEFINE_MUTEX(clocks_mutex); +/* For USE_COMMON_STRUCT_CLK, these are provided in clk.c, but not exported +* through other headers; we don't want them used anywhere but here. */ +#ifdef CONFIG_USE_COMMON_STRUCT_CLK +extern int __clk_get (struct clk *clk); +extern void __clk_put (struct clk *clk); +#endif +?/*
?* Find the correct struct clk for the device and connection ID. ?* We do slightly fuzzy matching here:include/linux/clk.h +0
-0@@ -3,6 +3,7 @@?*
?* ?Copyright (C) 2004 ARM Limited.
?* ?Written by Deep Blue Solutions Limited.
+ * ?Copyright (c) 2010-2011 Jeremy Kerr <[email protected]> ?*
?* This program is free software; you can redistribute it and/or modify ?* it under the terms of the GNU General Public License version 2 asinclude/linux/clk.h +0
-0@@ -11,17 +12,103 @@?#ifndef
__LINUX_CLK_H
?#define__LINUX_CLK_Hinclude/linux/clk.h +3
-0+#include <linux/err.h> +#include <linux/spinlock.h> +?struct device;
include/linux/clk.h +25
-2-/* -* The base API. +struct clk ; + +#ifdef CONFIG_GENERIC_CLK + +struct clk_hw { + ? ? ? struct clk *clk; +}; + +/** +* 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
include/linux/clk.h
Quoted 10 lines
include/linux/clk.h +10
-0+* ? ? ? ? ? ? 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. Called with the global clock mutex +* ? ? ? ? ? ? held. Optional, but recommended - if this op is not set, +* ? ? ? ? ? ? clk_get_rate will return 0.
You need a callback to update the rate when the parent or parent's rate changes, which I would call recalc_rate, as well as this init-type call.
Quoted 89 lines
+ *
+ * @get_parent Query the parent of this clock; for clocks with multiple
+ * ? ? ? ? ? ? possible parents, query the hardware for the current
+ * ? ? ? ? ? ? parent. Currently only called when the clock is first
+ * ? ? ? ? ? ? registered.
+ *
+ * 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 a clock requires sleeping code to be turned on, this
+ * should be done inclk_prepare. Switching that will not sleep should be done
+ * inclk_enable.
+ *
+ * Typically, drivers will callclk_preparewhen a clock may be needed later
+ * (eg. when a device is opened), andclk_enablewhen the clock is actually
+ * required (eg. from an interrupt). Note thatclk_prepare*must* have been
+ * called beforeclk_enable.
+ */include/linux/clk.h +17
-0+struct clk_hw_ops { + ? ? ? int ? ? ? ? ? ? (*prepare)(struct clk_hw *); + ? ? ? void ? ? ? ? ? ?(*unprepare)(struct clk_hw *); + ? ? ? int ? ? ? ? ? ? (*enable)(struct clk_hw *); + ? ? ? void ? ? ? ? ? ?(*disable)(struct clk_hw *); + ? ? ? unsigned long ? (*recalc_rate)(struct clk_hw *); + ? ? ? long ? ? ? ? ? ?(*round_rate)(struct clk_hw *, unsigned long); + ? ? ? struct clk * ? ?(*get_parent)(struct clk_hw *); +}; + +/** +* clk_prepare - prepare clock for atomic enabling. +* +* @clk: The clock to prepare +* +* Do any possibly sleeping initialisation on @clk, allowing the clock to be +* later enabled atomically (via clk_enable). This function may sleep. ?*/
include/linux/clk.h +11
-0+int clk_prepare (struct clk *clk); +/** +* clk_unprepare - release clock from prepared state +* +* @clk: The clock to release +* +* Do any (possibly sleeping) cleanup on clk. This function may sleep. +*/ +void clk_unprepare (struct clk *clk); + +#else /* !CONFIG_GENERIC_CLK */ ?/*
- * struct clk - an machine class defined object / cookie.
+ * For !CONFIG_GENERIC_CLK, we don't enforce any atomicity
+ * requirements for clk_enable/clk_disable, so the prepare and unprepare
+ * functions are no-ops
?*/include/linux/clk.h +4
-1-struct clk ; +static inline int clk_prepare (struct clk *clk) {return 0 ; } +static inline void clk_unprepare (struct clk *clk) { } + +#endif /* !CONFIG_GENERIC_CLK */ ?/**
?*clk_get- lookup and obtain a reference to a clock producer.include/linux/clk.h +0
-0@@ -67,6 +154,7 @@ void clk_disable(struct clk *clk);?/**
?*clk_get_rate- obtain the current clock rate (in Hz) for a clock source. ?* ? ? ? ? ? ? ? This is only valid once the clock source has been enabled.
+ * ? ? ? ? ? ? ? Returns zero if the clock rate is unknown.
?* @clk: clock source
?*/
?unsigned longclk_get_rate(struct clk *clk);include/linux/clk.h +0
-0@@ -83,12 +171,6 @@ unsigned long clk_get_rate(struct clk *clk);?*/
?voidclk_put(struct clk *clk);include/linux/clk.h +0
-6- -/* -* The remaining APIs are optional for machine class support. -*/ - -?/**
?*clk_round_rate- adjust a rate to the exact rate a clock can provide ?* @clk: clock source_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Re: [PATCH 1/4] clk: Add a generic clock infrastructure
Quoted 46 lines · Colin Cross
Quoted 32 lines · Jeremy Kerr
We currently have ~21 definitions of struct clk in the ARM architecture, each defined on a per-platform basis. This makes it difficult to define platform- (or architecture-) independent clock sources without making assumptions about struct clk, and impossible to compile two platforms with different struct clks into a single image.
This change is an effort to unify struct clk where possible, by defining a common struct clk, and a set of clock operations. Different clock implementations can set their own operations, and have a standard interface for generic code. The callback interface is exposed to the kernel proper, while the clock implementations only need to be seen by the platform internals.
The interface is split into two halves:
?* struct clk, which is the generic-device-driver interface. This ? provides a set of functions which drivers may use to request ? enable/disable, query or manipulate in a hardware-independent manner.
?* struct
clk_hwand structclk_hw_ops, which is the hardware-specific ? interface. Clock drivers implement the ops, which allow the core ? clock code to implement the generic 'struct clk' API.This allows us to share clock code among platforms, and makes it possible to dynamically create clock devices in platform-independent code.
Platforms can enable the generic struct clock through
CONFIG_GENERIC_CLK. In this case, the clock infrastructure consists of a common, opaque struct clk, and a set of clock operations (defined per type of clock):Quoted 12 lines
?struct
clk_hw_ops{
? ? ? ?int ? ? ? ? ? ? (*prepare)(structclk_hw*);
? ? ? ?void ? ? ? ? ? ?(*unprepare)(structclk_hw*);
? ? ? ?int ? ? ? ? ? ? (*enable)(structclk_hw*);
? ? ? ?void ? ? ? ? ? ?(*disable)(structclk_hw*);
? ? ? ?unsigned long ? (*recalc_rate)(structclk_hw*);
? ? ? ?int ? ? ? ? ? ? (*set_rate)(structclk_hw*,
? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?unsigned long, unsigned long *); ? ? ? ?long ? ? ? ? ? ?(*round_rate)(structclk_hw*, unsigned long); ? ? ? ?int ? ? ? ? ? ? (*set_parent)(structclk_hw*, struct clk *); ? ? ? ?struct clk * ? ?(*get_parent)(structclk_hw*);
?};
Quoted 8 lines
You might want to split these into three separate structs, for mux ops, rate ops, and gate ops. That way, multiple building blocks (a gate and a divider, for example), can be easily combined into a single clock node. Also, an init op that reads the clock tree state from the hardware has been very useful on Tegra - there are often clocks that you can't or won't change, and being able to view their state as configured by the bootloader, and base other clocks off of them, is helpful.
The clock state is read initially from the hardware with the recalc_rate/get_parent ops. What do we need an additional init op for?
Quoted 3 lines
It also allows you to turn off clocks that are enabled by the bootloader, but never enabled by the kernel (enabled=1, enable_count=0).
The enable count indeed is a problem. I don't think an init hook would be the right solution for this though as this sounds platform specific. struct clk_hw_ops should be specific to the type of clock (mux, divider, gate) and should be present only once per type.
For the enable count (and whether a clock should initially be enabled or not) I can think of something like this:
- A platform/SoC registers all clocks.
- It then calls clk_prepare/enable for all vital core clocks (SDRAM, CPU,...). At this time the enable counters are correct.
- Then some hook in the generic clock layer is called. This hook iterates over the tree and disables all clocks in hardware which have a enable count of 0.
Quoted 5 lines
Quoted 5 lines
(quoted diff) +5
-0+ +struct clk { + ? ? ? const char ? ? ? ? ? ? ?*name; + ? ? ? struct clk_hw_ops ? ? ? *ops; + ? ? ? struct clk_hw ? ? ? ? ? *hw;
Quoted 8 lines
Quoted 3 lines
+ ? ? ? unsigned int ? ? ? ? ? ?enable_count;
+ ? ? ? unsigned int ? ? ? ? ? ?prepare_count;
+ ? ? ? struct clk ? ? ? ? ? ? ?*parent;Storing the list of possible parents at this level can help abstract some common code from mux ops if you pass the index into the list of the new parent into the op - most mux ops only need to know which of their mux inputs needs to be enabled.
Please don't. Only muxes have more than one possible parent, so this should be handled there.
Quoted 9 lines
Quoted 2 lines
+ ? ? ? unsigned long ? ? ? ? ? rate;
If you add an init op, an enabled flag here is also useful to track whether the bootloader left the clock on, which allows turning off unnecessary clocks.
I think you also need a list of current children of the clock to allow propagating rate changes from parent to children.
This is added in another patch in this series implementing clk_set_rate.
Quoted 16 lines
Quoted 16 lines
(quoted diff) +16
-0+ +static 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->hw);
Quoted 8 lines
drivers/clk/clk.c line 47
Quoted 3 lines
drivers/clk/clk.c new file +3
-0+ + ? ? ? __clk_unprepare(clk->parent); +}Are there any cases where the unlocked versions of the clk calls need to be exposed to the ops implementations? For example, a
set_rateop may want to callclk_set_parenton itself to change its parent to a better source, and then set its rate. It would need to call__clk_set_parentto avoid deadlocking on theprepare_lock.
I hope we can avoid that. The decision of the best parent should be left up to the user. Doing this in the mux/divider implementation would torpedo attempts to implement generic building blocks.
Quoted 19 lines
Quoted 6 lines
drivers/clk/clk.c new file +6
-0+ +unsigned long clk_get_rate (struct clk *clk) +{ + ? ? ? if (!clk) + ? ? ? ? ? ? ? return 0; + ? ? ? return clk->rate;drivers/clk/clk.c line 177
Quoted 10 lines
drivers/clk/clk.c new file +10
-0+} +EXPORT_SYMBOL_GPL (clk_get_rate); + +long clk_round_rate (struct clk *clk,unsigned long rate) +{ + ? ? ? if (clk && clk->ops->round_rate) + ? ? ? ? ? ? ? return clk->ops->round_rate(clk->hw, rate); + ? ? ? return rate; +} +EXPORT_SYMBOL_GPL (clk_round_rate);I think you should hold the prepare mutex around calls to
clk_round_rate, which will allow some code simplification similar to
Quoted 8 lines
what Russell suggested in another thread. If you hold the mutex here, as well as in
clk_set_rate, and you call theround_rateop before theset_rateop inclk_set_rate, op implementers can compute the rate in theirround_rateop, and save the register values needed to get that rate in private temporary storage. Theset_rateop can then assume that those register values are correct, because the lock is still held, and just write them. That moves all the computation to one place, and it only needs to run once.
This won't work in the case of cascaded dividers. These have to call clk_round_rate in their set_rate op for each possible divider value to get the best result. They can't do this when both set_rate and round_rate acquire the lock.
[...]
Quoted 25 lines
Quoted 25 lines
(quoted diff) +25
-0+struct clk *clk_register(struct clk_hw_ops *ops, struct clk_hw *hw, + ? ? ? ? ? ? ? const char *name) +{ + ? ? ? struct clk *clk; + + ? ? ? clk = kzalloc(sizeof(*clk), GFP_KERNEL); + ? ? ? if (!clk) + ? ? ? ? ? ? ? return NULL; + + ? ? ? clk->name = name; + ? ? ? clk->ops = ops; + ? ? ? clk->hw = hw; + ? ? ? hw->clk = clk; + + ? ? ? /* Query the hardware for parent and initial rate */ + + ? ? ? if (clk->ops->get_parent) + ? ? ? ? ? ? ? /* We don't to lock against prepare/enable here, as + ? ? ? ? ? ? ? ?* the clock is not yet accessible from anywhere */ + ? ? ? ? ? ? ? clk->parent = clk->ops->get_parent(clk->hw); + + ? ? ? if (clk->ops->recalc_rate) + ? ? ? ? ? ? ? clk->rate = clk->ops->recalc_rate(clk->hw); + +
Quoted 8 lines
Review
Quoted 3 lines
+ ? ? ? return clk;
(quoted diff) +2
-0+} +EXPORT_SYMBOL_GPL(clk_register);If you are requiring clk's parents (or possible parents?) to be registered before clk, you could put the
clk_lookupstruct inside the clk struct and callclkdev_addfromclk_register, saving some boilerplate in the platforms.
There can be multiple struct clk_lookup for each clock.
Quoted 23 lines
Quoted 15 lines
+ *
+ * @prepare: ? Prepare the clock for enabling. This must not return until
+ * ? ? ? ? ? ? the clock is fully prepared, and it's safe to callclk_enable.
+ * ? ? ? ? ? ? This callback is intended to allow clock implementations to
+ * ? ? ? ? ? ? do any initialisation that may sleep. Called with
+ * ? ? ? ? ? ?prepare_lockheld.
+ *
+ * @unprepare: Release the clock from its prepared state. This will typically
+ * ? ? ? ? ? ? undo any work done in the @prepare callback. Called with
+ * ? ? ? ? ? ?prepare_lockheld.
+ *
+ * @enable: ? ?Enable the clock atomically. This must not return until the
+ * ? ? ? ? ? ? clock is generating a valid clock signal, usable by consumer
+ * ? ? ? ? ? ? devices. Called withenable_lockheld. This function must not
+ * ? ? ? ? ? ? sleep.Quoted 8 lines
+ *
+ * @disable: ? Disable the clock atomically. Called withenable_lockheld.
+ * ? ? ? ? ? ? This function must not sleep.
+ *
+ * @recalc_rate ? ? ? ?Recalculate the rate of this clock, by quering hardware
+ * ? ? ? ? ? ? and/or the clock's parent. Called with the global clock mutex
+ * ? ? ? ? ? ? held. Optional, but recommended - if this op is not set,
+ * ? ? ? ? ? ?clk_get_ratewill return 0.
Quoted 3 lines
You need a callback to update the rate when the parent or parent's rate changes, which I would call
recalc_rate, as well as this init-type call.
This is already done on framework level, I think in the clk_set_rate patch.
Sascha
Signature · 4 lines
Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Re: [PATCH 1/4] clk: Add a generic clock infrastructure
Quoted 56 lines · Sascha Hauer
Quoted 46 lines · Colin Cross
Quoted 32 lines · Jeremy Kerr
We currently have ~21 definitions of struct clk in the ARM architecture, each defined on a per-platform basis. This makes it difficult to define platform- (or architecture-) independent clock sources without making assumptions about struct clk, and impossible to compile two platforms with different struct clks into a single image.
This change is an effort to unify struct clk where possible, by defining a common struct clk, and a set of clock operations. Different clock implementations can set their own operations, and have a standard interface for generic code. The callback interface is exposed to the kernel proper, while the clock implementations only need to be seen by the platform internals.
The interface is split into two halves:
?* struct clk, which is the generic-device-driver interface. This ? provides a set of functions which drivers may use to request ? enable/disable, query or manipulate in a hardware-independent manner.
?* struct
clk_hwand structclk_hw_ops, which is the hardware-specific ? interface. Clock drivers implement the ops, which allow the core ? clock code to implement the generic 'struct clk' API.This allows us to share clock code among platforms, and makes it possible to dynamically create clock devices in platform-independent code.
Platforms can enable the generic struct clock through
CONFIG_GENERIC_CLK. In this case, the clock infrastructure consists of a common, opaque struct clk, and a set of clock operations (defined per type of clock):Quoted 12 lines
?struct
clk_hw_ops{
? ? ? ?int ? ? ? ? ? ? (*prepare)(structclk_hw*);
? ? ? ?void ? ? ? ? ? ?(*unprepare)(structclk_hw*);
? ? ? ?int ? ? ? ? ? ? (*enable)(structclk_hw*);
? ? ? ?void ? ? ? ? ? ?(*disable)(structclk_hw*);
? ? ? ?unsigned long ? (*recalc_rate)(structclk_hw*);
? ? ? ?int ? ? ? ? ? ? (*set_rate)(structclk_hw*,
? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?unsigned long, unsigned long *); ? ? ? ?long ? ? ? ? ? ?(*round_rate)(structclk_hw*, unsigned long); ? ? ? ?int ? ? ? ? ? ? (*set_parent)(structclk_hw*, struct clk *); ? ? ? ?struct clk * ? ?(*get_parent)(structclk_hw*);
?};Quoted 8 lines
You might want to split these into three separate structs, for mux ops, rate ops, and gate ops. ?That way, multiple building blocks (a gate and a divider, for example), can be easily combined into a single clock node. ?Also, an init op that reads the clock tree state from the hardware has been very useful on Tegra - there are often clocks that you can't or won't change, and being able to view their state as configured by the bootloader, and base other clocks off of them, is helpful.
Quoted 2 lines
The clock state is read initially from the hardware with the recalc_rate/get_parent ops. What do we need an additional init op for?
I see - I would rename them to make it clear they are for init, maybe init_rate and init_parent, and not call them later - reading clock state can be very expensive on some platforms, if not impossible - Qualcomm requires RPCs to the modem to get clock state. If every clk_set_rate triggers state reads all the way through the descendants, that could be a huge performance hit. If you separate init and recalculate, mux implementations can store their divider settings and very easily recalculate their rate.
Quoted 12 lines
Quoted 3 lines
It also allows you to turn off clocks that are enabled by the bootloader, but never enabled by the kernel (enabled=1, enable_count=0).
The enable count indeed is a problem. I don't think an init hook would be the right solution for this though as this sounds platform specific. struct
clk_hw_opsshould be specific to the type of clock (mux, divider, gate) and should be present only once per type.For the enable count (and whether a clock should initially be enabled or not) I can think of something like this:
Quoted 6 lines
- A platform/SoC registers all clocks.
- It then calls clk_prepare/enable for all vital core clocks ?(SDRAM, CPU,...). At this time the enable counters are correct.
- Then some hook in the generic clock layer is called. This hook ?iterates over the tree and disables all clocks in hardware which ?have a enable count of 0.
Is it always safe to disable a clock that is already disabled? An init_enable hook that set an enabled flag would let you only disable clocks that were actually left on by the bootloader, and report to the user which ones are actually being turned off (which has caught a lot of problems on Tegra).
Quoted 14 lines
Quoted 5 lines
Quoted 5 lines
(quoted diff) +5
-0+ +struct clk { + ? ? ? const char ? ? ? ? ? ? ?*name; + ? ? ? struct clk_hw_ops ? ? ? *ops; + ? ? ? struct clk_hw ? ? ? ? ? *hw;Quoted 8 lines
Quoted 3 lines
+ ? ? ? unsigned int ? ? ? ? ? ?enable_count;
+ ? ? ? unsigned int ? ? ? ? ? ?prepare_count;
+ ? ? ? struct clk ? ? ? ? ? ? ?*parent;Storing the list of possible parents at this level can help abstract some common code from mux ops if you pass the index into the list of the new parent into the op - most mux ops only need to know which of their mux inputs needs to be enabled.
Quoted 2 lines
Please don't. Only muxes have more than one possible parent, so this should be handled there.
The cost is one pointer per clock that is not actually a mux, and the benefit is that you can move a very common search loop out of every mux implementation into the framework. It also lets you determine which clocks are connected, which becomes necessary if you try to do per-tree locking or sysfs controls for clocks.
Quoted 11 lines
Quoted 9 lines
Quoted 2 lines
+ ? ? ? unsigned long ? ? ? ? ? rate;
If you add an init op, an enabled flag here is also useful to track whether the bootloader left the clock on, which allows turning off unnecessary clocks.
I think you also need a list of current children of the clock to allow propagating rate changes from parent to children.
This is added in another patch in this series implementing
clk_set_rate.
Thanks, I'll take a look.
Quoted 25 lines
Quoted 16 lines
Quoted 16 lines
(quoted diff) +16
-0+ +static 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->hw);Quoted 8 lines
drivers/clk/clk.c line 47
Quoted 3 lines
drivers/clk/clk.c new file +3
-0+ + ? ? ? __clk_unprepare(clk->parent); +}Are there any cases where the unlocked versions of the clk calls need to be exposed to the ops implementations? ?For example, a
set_rateop may want to callclk_set_parenton itself to change its parent to a better source, and then set its rate. ?It would need to call__clk_set_parentto avoid deadlocking on theprepare_lock.
Quoted 3 lines
I hope we can avoid that. The decision of the best parent should be left up to the user. Doing this in the mux/divider implementation would torpedo attempts to implement generic building blocks.
I agree it would be nice, but it does push some knowledge of the clock tree into device drivers. For example, on Tegra the display driver may need to change the source pll of the display clock to get the required pclk, which requires passing all the possible parents of the display clock into the display driver. If this is a common usage pattern, there needs to be a hook in the ops to allow the clock or clock chip to make a more global decision.
Quoted 28 lines
Quoted 19 lines
Quoted 6 lines
drivers/clk/clk.c new file +6
-0+ +unsigned long clk_get_rate (struct clk *clk) +{ + ? ? ? if (!clk) + ? ? ? ? ? ? ? return 0; + ? ? ? return clk->rate;drivers/clk/clk.c line 177
Quoted 10 lines
drivers/clk/clk.c new file +10
-0+} +EXPORT_SYMBOL_GPL (clk_get_rate); + +long clk_round_rate (struct clk *clk,unsigned long rate) +{ + ? ? ? if (clk && clk->ops->round_rate) + ? ? ? ? ? ? ? return clk->ops->round_rate(clk->hw, rate); + ? ? ? return rate; +} +EXPORT_SYMBOL_GPL (clk_round_rate);I think you should hold the prepare mutex around calls to
clk_round_rate, which will allow some code simplification similar toQuoted 8 lines
what Russell suggested in another thread. ?If you hold the mutex here, as well as in
clk_set_rate, and you call theround_rateop before theset_rateop inclk_set_rate, op implementers can compute the rate in theirround_rateop, and save the register values needed to get that rate in private temporary storage. ?Theset_rateop can then assume that those register values are correct, because the lock is still held, and just write them. ?That moves all the computation to one place, and it only needs to run once.
Quoted 4 lines
This won't work in the case of cascaded dividers. These have to call
clk_round_ratein theirset_rateop for each possible divider value to get the best result. They can't do this when bothset_rateandround_rateacquire the lock.
Then they call __clk_round_rate if they already have the lock?
Quoted 36 lines
[...]
Quoted 25 lines
Quoted 25 lines
(quoted diff) +25
-0+struct clk *clk_register(struct clk_hw_ops *ops, struct clk_hw *hw, + ? ? ? ? ? ? ? const char *name) +{ + ? ? ? struct clk *clk; + + ? ? ? clk = kzalloc(sizeof(*clk), GFP_KERNEL); + ? ? ? if (!clk) + ? ? ? ? ? ? ? return NULL; + + ? ? ? clk->name = name; + ? ? ? clk->ops = ops; + ? ? ? clk->hw = hw; + ? ? ? hw->clk = clk; + + ? ? ? /* Query the hardware for parent and initial rate */ + + ? ? ? if (clk->ops->get_parent) + ? ? ? ? ? ? ? /* We don't to lock against prepare/enable here, as + ? ? ? ? ? ? ? ?* the clock is not yet accessible from anywhere */ + ? ? ? ? ? ? ? clk->parent = clk->ops->get_parent(clk->hw); + + ? ? ? if (clk->ops->recalc_rate) + ? ? ? ? ? ? ? clk->rate = clk->ops->recalc_rate(clk->hw); + +Quoted 8 lines
Review
Quoted 3 lines
+ ? ? ? return clk;
(quoted diff) +2
-0+} +EXPORT_SYMBOL_GPL(clk_register);If you are requiring clk's parents (or possible parents?) to be registered before clk, you could put the
clk_lookupstruct inside the clk struct and callclkdev_addfromclk_register, saving some boilerplate in the platforms.
Quoted 1 line
There can be multiple struct
clk_lookupfor each clock.
Sure, and that could be handled by clk_register_alias. Most of the clocks have a single clk_lookup.
Quoted 23 lines
Quoted 23 lines
Quoted 23 lines
+ *
+ * @prepare: ? Prepare the clock for enabling. This must not return until
+ * ? ? ? ? ? ? the clock is fully prepared, and it's safe to callclk_enable.
+ * ? ? ? ? ? ? This callback is intended to allow clock implementations to
+ * ? ? ? ? ? ? do any initialisation that may sleep. Called with
+ * ? ? ? ? ? ?prepare_lockheld.
+ *
+ * @unprepare: Release the clock from its prepared state. This will typically
+ * ? ? ? ? ? ? undo any work done in the @prepare callback. Called with
+ * ? ? ? ? ? ?prepare_lockheld.
+ *
+ * @enable: ? ?Enable the clock atomically. This must not return until the
+ * ? ? ? ? ? ? clock is generating a valid clock signal, usable by consumer
+ * ? ? ? ? ? ? devices. Called withenable_lockheld. This function must not
+ * ? ? ? ? ? ? sleep.
+ *
+ * @disable: ? Disable the clock atomically. Called withenable_lockheld.
+ * ? ? ? ? ? ? This function must not sleep.
+ *
+ * @recalc_rate ? ? ? ?Recalculate the rate of this clock, by quering hardware
+ * ? ? ? ? ? ? and/or the clock's parent. Called with the global clock mutex
+ * ? ? ? ? ? ? held. Optional, but recommended - if this op is not set,
+ * ? ? ? ? ? ?clk_get_ratewill return 0.
Quoted 6 lines
Quoted 3 lines
You need a callback to update the rate when the parent or parent's rate changes, which I would call
recalc_rate, as well as this init-type call.This is already done on framework level, I think in the
clk_set_ratepatch.
I'll comment there.
Quoted 8 lines
Sascha
--
Pengutronix e.K. ? ? ? ? ? ? ? ? ? ? ? ? ? | ? ? ? ? ? ? ? ? ? ? ? ? ? ? | Industrial Linux Solutions ? ? ? ? ? ? ? ? | http://www.pengutronix.de/ ?| Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 ? ?| Amtsgericht Hildesheim, HRA 2686 ? ? ? ? ? | Fax: ? +49-5121-206917-5555 |
Re: [PATCH 1/4] clk: Add a generic clock infrastructure
Quoted 29 lines · Colin Cross
Quoted 25 lines · Sascha Hauer
Quoted 15 lines · Colin Cross
Quoted 13 lines · Jeremy Kerr
?struct
clk_hw_ops{
? ? ? ?int ? ? ? ? ? ? (*prepare)(structclk_hw*);
? ? ? ?void ? ? ? ? ? ?(*unprepare)(structclk_hw*);
? ? ? ?int ? ? ? ? ? ? (*enable)(structclk_hw*);
? ? ? ?void ? ? ? ? ? ?(*disable)(structclk_hw*);
? ? ? ?unsigned long ? (*recalc_rate)(structclk_hw*);
? ? ? ?int ? ? ? ? ? ? (*set_rate)(structclk_hw*,
? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?unsigned long, unsigned long *); ? ? ? ?long ? ? ? ? ? ?(*round_rate)(structclk_hw*, unsigned long); ? ? ? ?int ? ? ? ? ? ? (*set_parent)(structclk_hw*, struct clk *); ? ? ? ?struct clk * ? ?(*get_parent)(structclk_hw*);
?};Quoted 8 lines
You might want to split these into three separate structs, for mux ops, rate ops, and gate ops. ?That way, multiple building blocks (a gate and a divider, for example), can be easily combined into a single clock node. ?Also, an init op that reads the clock tree state from the hardware has been very useful on Tegra - there are often clocks that you can't or won't change, and being able to view their state as configured by the bootloader, and base other clocks off of them, is helpful.
Quoted 2 lines
The clock state is read initially from the hardware with the recalc_rate/get_parent ops. What do we need an additional init op for?
Quoted 8 lines
I see - I would rename them to make it clear they are for init, maybe
init_rateandinit_parent, and not call them later - reading clock state can be very expensive on some platforms, if not impossible - Qualcomm requires RPCs to the modem to get clock state. If everyclk_set_ratetriggers state reads all the way through the descendants, that could be a huge performance hit. If you separate init and recalculate, mux implementations can store their divider settings and very easily recalculate their rate.
Even without additional hooks divider and muxes can decide to cache the actual register values.
Quoted 13 lines
Quoted 13 lines
Quoted 4 lines
It also allows you to turn off clocks that are enabled by the bootloader, but never enabled by the kernel (enabled=1, enable_count=0).
The enable count indeed is a problem. I don't think an init hook would be the right solution for this though as this sounds platform specific. struct
clk_hw_opsshould be specific to the type of clock (mux, divider, gate) and should be present only once per type.For the enable count (and whether a clock should initially be enabled or not) I can think of something like this:
Quoted 8 lines
Quoted 6 lines
- A platform/SoC registers all clocks.
- It then calls clk_prepare/enable for all vital core clocks ?(SDRAM, CPU,...). At this time the enable counters are correct.
- Then some hook in the generic clock layer is called. This hook ?iterates over the tree and disables all clocks in hardware which ?have a enable count of 0.Is it always safe to disable a clock that is already disabled?
I'm not aware of any clock where that's not the case, but your mileage may vary. At least the implementation should be able to determine whether a clock already is disabled and just skip another disable.
Quoted 20 lines
An
init_enablehook that set an enabled flag would let you only disable clocks that were actually left on by the bootloader, and report to the user which ones are actually being turned off (which has caught a lot of problems on Tegra).Quoted 14 lines
Quoted 13 lines
Review
Quoted 8 lines
(quoted diff) +8
-0+ +struct clk { + ? ? ? const char ? ? ? ? ? ? ?*name; + ? ? ? struct clk_hw_ops ? ? ? *ops; + ? ? ? struct clk_hw ? ? ? ? ? *hw; + ? ? ? unsigned int ? ? ? ? ? ?enable_count; + ? ? ? unsigned int ? ? ? ? ? ?prepare_count; + ? ? ? struct clk ? ? ? ? ? ? ?*parent;Storing the list of possible parents at this level can help abstract some common code from mux ops if you pass the index into the list of the new parent into the op - most mux ops only need to know which of their mux inputs needs to be enabled.
Quoted 8 lines
Quoted 2 lines
Please don't. Only muxes have more than one possible parent, so this should be handled there.
The cost is one pointer per clock that is not actually a mux, and the benefit is that you can move a very common search loop out of every mux implementation into the framework. It also lets you determine which clocks are connected, which becomes necessary if you try to do per-tree locking or sysfs controls for clocks.
I agree that some sort of possible_parents iteration would be nice.
Quoted 26 lines
Quoted 22 lines
Quoted 13 lines
Quoted 13 lines
(quoted diff) +11
-0+ + ? ? ? 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->hw);Quoted 8 lines
drivers/clk/clk.c line 47
Quoted 3 lines
drivers/clk/clk.c new file +3
-0+ + ? ? ? __clk_unprepare(clk->parent); +}Are there any cases where the unlocked versions of the clk calls need to be exposed to the ops implementations? ?For example, a
set_rateop may want to callclk_set_parenton itself to change its parent to a better source, and then set its rate. ?It would need to call__clk_set_parentto avoid deadlocking on theprepare_lock.Quoted 3 lines
I hope we can avoid that. The decision of the best parent should be left up to the user. Doing this in the mux/divider implementation would torpedo attempts to implement generic building blocks.
Quoted 7 lines
I agree it would be nice, but it does push some knowledge of the clock tree into device drivers. For example, on Tegra the display driver may need to change the source pll of the display clock to get the required pclk, which requires passing all the possible parents of the display clock into the display driver. If this is a common usage pattern, there needs to be a hook in the ops to allow the clock or clock chip to make a more global decision.
I think this is a common pattern. Another solution to this would be that the platform implements a clock whose only purpose is to build a bridge between the driver and the clock tree. There may be more constrains, for example in some cases you may need a clock which also runs in different sleep states whereas sometimes you may need a clock which is turned of when in sleep mode. I agree that this must be handled somewhere, but the clock framework is not the place to implement this stuff.
Quoted 12 lines
Quoted 12 lines
Quoted 11 lines
I think you should hold the prepare mutex around calls to
clk_round_rate, which will allow some code simplification similar to what Russell suggested in another thread. ?If you hold the mutex here, as well as inclk_set_rate, and you call theround_rateop before theset_rateop inclk_set_rate, op implementers can compute the rate in theirround_rateop, and save the register values needed to get that rate in private temporary storage. ?Theset_rateop can then assume that those register values are correct, because the lock is still held, and just write them. ?That moves all the computation to one place, and it only needs to run once.
Quoted 6 lines
Quoted 4 lines
This won't work in the case of cascaded dividers. These have to call
clk_round_ratein theirset_rateop for each possible divider value to get the best result. They can't do this when bothset_rateandround_rateacquire the lock.Then they call
__clk_round_rateif they already have the lock?
I think this is an implementation detail. As our idea of how a clock framework should work still is quite different we can discuss this later ;)
Quoted 37 lines
Quoted 37 lines
[...]
Quoted 33 lines
Quoted 18 lines
(quoted diff) +18
-0+struct clk *clk_register(struct clk_hw_ops *ops, struct clk_hw *hw, + ? ? ? ? ? ? ? const char *name) +{ + ? ? ? struct clk *clk; + + ? ? ? clk = kzalloc(sizeof(*clk), GFP_KERNEL); + ? ? ? if (!clk) + ? ? ? ? ? ? ? return NULL; + + ? ? ? clk->name = name; + ? ? ? clk->ops = ops; + ? ? ? clk->hw = hw; + ? ? ? hw->clk = clk; + + ? ? ? /* Query the hardware for parent and initial rate */ + + ? ? ? if (clk->ops->get_parent) + ? ? ? ? ? ? ? /* We don't to lock against prepare/enable here, asReview
Quoted 10 lines
(quoted diff) +10
-0+ ? ? ? ? ? ? ? ?* the clock is not yet accessible from anywhere */ + ? ? ? ? ? ? ? clk->parent = clk->ops->get_parent(clk->hw); + + ? ? ? if (clk->ops->recalc_rate) + ? ? ? ? ? ? ? clk->rate = clk->ops->recalc_rate(clk->hw); + + + ? ? ? return clk; +} +EXPORT_SYMBOL_GPL(clk_register);If you are requiring clk's parents (or possible parents?) to be registered before clk, you could put the
clk_lookupstruct inside the clk struct and callclkdev_addfromclk_register, saving some boilerplate in the platforms.
Quoted 4 lines
Quoted 1 line
There can be multiple struct
clk_lookupfor each clock.Sure, and that could be handled by
clk_register_alias. Most of the clocks have a singleclk_lookup.
In my idea only few of the clocks have a clk_lookup (you mentioned a clock between the i2c divider and i2c gate elsewhere, this would never be passed to a device).
Sascha
Signature · 4 lines
Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Re: [PATCH 1/4] clk: Add a generic clock infrastructure
Quoted 62 lines · Sascha Hauer
Quoted 29 lines · Colin Cross
Quoted 25 lines · Sascha Hauer
Quoted 15 lines · Colin Cross
Quoted 13 lines · Jeremy Kerr
?struct
clk_hw_ops{
? ? ? ?int ? ? ? ? ? ? (*prepare)(structclk_hw*);
? ? ? ?void ? ? ? ? ? ?(*unprepare)(structclk_hw*);
? ? ? ?int ? ? ? ? ? ? (*enable)(structclk_hw*);
? ? ? ?void ? ? ? ? ? ?(*disable)(structclk_hw*);
? ? ? ?unsigned long ? (*recalc_rate)(structclk_hw*);
? ? ? ?int ? ? ? ? ? ? (*set_rate)(structclk_hw*,
? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?unsigned long, unsigned long *); ? ? ? ?long ? ? ? ? ? ?(*round_rate)(structclk_hw*, unsigned long); ? ? ? ?int ? ? ? ? ? ? (*set_parent)(structclk_hw*, struct clk *); ? ? ? ?struct clk * ? ?(*get_parent)(structclk_hw*);
?};Quoted 8 lines
You might want to split these into three separate structs, for mux ops, rate ops, and gate ops. ?That way, multiple building blocks (a gate and a divider, for example), can be easily combined into a single clock node. ?Also, an init op that reads the clock tree state from the hardware has been very useful on Tegra - there are often clocks that you can't or won't change, and being able to view their state as configured by the bootloader, and base other clocks off of them, is helpful.
Quoted 2 lines
The clock state is read initially from the hardware with the recalc_rate/get_parent ops. What do we need an additional init op for?
Quoted 8 lines
I see - I would rename them to make it clear they are for init, maybe
init_rateandinit_parent, and not call them later - reading clock state can be very expensive on some platforms, if not impossible - Qualcomm requires RPCs to the modem to get clock state. If everyclk_set_ratetriggers state reads all the way through the descendants, that could be a huge performance hit. If you separate init and recalculate, mux implementations can store their divider settings and very easily recalculate their rate.Even without additional hooks divider and muxes can decide to cache the actual register values.
Quoted 20 lines
Quoted 19 lines
Quoted 4 lines
It also allows you to turn off clocks that are enabled by the bootloader, but never enabled by the kernel (enabled=1, enable_count=0).
The enable count indeed is a problem. I don't think an init hook would be the right solution for this though as this sounds platform specific. struct
clk_hw_opsshould be specific to the type of clock (mux, divider, gate) and should be present only once per type.For the enable count (and whether a clock should initially be enabled or not) I can think of something like this:
- A platform/SoC registers all clocks.
- It then calls clk_prepare/enable for all vital core clocks ?(SDRAM, CPU,...). At this time the enable counters are correct.
- Then some hook in the generic clock layer is called. This hook ?iterates over the tree and disables all clocks in hardware which ?have a enable count of 0.
Quoted 5 lines
Quoted 1 line
Is it always safe to disable a clock that is already disabled?
I'm not aware of any clock where that's not the case, but your mileage may vary. At least the implementation should be able to determine whether a clock already is disabled and just skip another disable.
Might be one possible case: arm AP and modem boot simultaneously, you can not disable modem clock before modem driver increase the enable_count. But nice to have a helper function to disable zero enable_count clocks.
Quoted 24 lines
Quoted 24 lines
An
init_enablehook that set an enabled flag would let you only disable clocks that were actually left on by the bootloader, and report to the user which ones are actually being turned off (which has caught a lot of problems on Tegra).Quoted 16 lines
Quoted 5 lines
Quoted 5 lines
(quoted diff) +5
-0+ +struct clk { + ? ? ? const char ? ? ? ? ? ? ?*name; + ? ? ? struct clk_hw_ops ? ? ? *ops; + ? ? ? struct clk_hw ? ? ? ? ? *hw;Quoted 8 lines
Quoted 3 lines
+ ? ? ? unsigned int ? ? ? ? ? ?enable_count;
+ ? ? ? unsigned int ? ? ? ? ? ?prepare_count;
+ ? ? ? struct clk ? ? ? ? ? ? ?*parent;Storing the list of possible parents at this level can help abstract some common code from mux ops if you pass the index into the list of the new parent into the op - most mux ops only need to know which of their mux inputs needs to be enabled.
Please don't. Only muxes have more than one possible parent, so this should be handled there.
Quoted 33 lines
Quoted 5 lines
The cost is one pointer per clock that is not actually a mux, and the benefit is that you can move a very common search loop out of every mux implementation into the framework. It also lets you determine which clocks are connected, which becomes necessary if you try to do per-tree locking or sysfs controls for clocks.
I agree that some sort of
possible_parentsiteration would be nice.Quoted 25 lines
Quoted 25 lines
Quoted 13 lines
Quoted 13 lines
(quoted diff) +11
-0+ + ? ? ? 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->hw);Quoted 8 lines
drivers/clk/clk.c line 47
Quoted 3 lines
drivers/clk/clk.c new file +3
-0+ + ? ? ? __clk_unprepare(clk->parent); +}Are there any cases where the unlocked versions of the clk calls need to be exposed to the ops implementations? ?For example, a
set_rateop may want to callclk_set_parenton itself to change its parent to a better source, and then set its rate. ?It would need to call__clk_set_parentto avoid deadlocking on theprepare_lock.I hope we can avoid that. The decision of the best parent should be left up to the user. Doing this in the mux/divider implementation would torpedo attempts to implement generic building blocks.
I believe some day, when we have to call child clock consumers' on-rate-change hooks, we will need it.
Thanks
Richard
Quoted 97 lines
Quoted 8 lines
I agree it would be nice, but it does push some knowledge of the clock tree into device drivers. For example, on Tegra the display driver may need to change the source pll of the display clock to get the required pclk, which requires passing all the possible parents of the display clock into the display driver. If this is a common usage pattern, there needs to be a hook in the ops to allow the clock or clock chip to make a more global decision.
I think this is a common pattern. Another solution to this would be that the platform implements a clock whose only purpose is to build a bridge between the driver and the clock tree. There may be more constrains, for example in some cases you may need a clock which also runs in different sleep states whereas sometimes you may need a clock which is turned of when in sleep mode. I agree that this must be handled somewhere, but the clock framework is not the place to implement this stuff.
Quoted 12 lines
Quoted 12 lines
Quoted 11 lines
I think you should hold the prepare mutex around calls to
clk_round_rate, which will allow some code simplification similar to what Russell suggested in another thread. ?If you hold the mutex here, as well as inclk_set_rate, and you call theround_rateop before theset_rateop inclk_set_rate, op implementers can compute the rate in theirround_rateop, and save the register values needed to get that rate in private temporary storage. ?Theset_rateop can then assume that those register values are correct, because the lock is still held, and just write them. ?That moves all the computation to one place, and it only needs to run once.Quoted 6 lines
Quoted 4 lines
This won't work in the case of cascaded dividers. These have to call
clk_round_ratein theirset_rateop for each possible divider value to get the best result. They can't do this when bothset_rateandround_rateacquire the lock.Then they call
__clk_round_rateif they already have the lock?I think this is an implementation detail. As our idea of how a clock framework should work still is quite different we can discuss this later ;)
Quoted 37 lines
Quoted 37 lines
[...]
Quoted 33 lines
Quoted 18 lines
(quoted diff) +18
-0+struct clk *clk_register(struct clk_hw_ops *ops, struct clk_hw *hw, + ? ? ? ? ? ? ? const char *name) +{ + ? ? ? struct clk *clk; + + ? ? ? clk = kzalloc(sizeof(*clk), GFP_KERNEL); + ? ? ? if (!clk) + ? ? ? ? ? ? ? return NULL; + + ? ? ? clk->name = name; + ? ? ? clk->ops = ops; + ? ? ? clk->hw = hw; + ? ? ? hw->clk = clk; + + ? ? ? /* Query the hardware for parent and initial rate */ + + ? ? ? if (clk->ops->get_parent) + ? ? ? ? ? ? ? /* We don't to lock against prepare/enable here, asReview
Quoted 10 lines
(quoted diff) +10
-0+ ? ? ? ? ? ? ? ?* the clock is not yet accessible from anywhere */ + ? ? ? ? ? ? ? clk->parent = clk->ops->get_parent(clk->hw); + + ? ? ? if (clk->ops->recalc_rate) + ? ? ? ? ? ? ? clk->rate = clk->ops->recalc_rate(clk->hw); + + + ? ? ? return clk; +} +EXPORT_SYMBOL_GPL(clk_register);If you are requiring clk's parents (or possible parents?) to be registered before clk, you could put the
clk_lookupstruct inside the clk struct and callclkdev_addfromclk_register, saving some boilerplate in the platforms.Quoted 4 lines
Quoted 1 line
There can be multiple struct
clk_lookupfor each clock.Sure, and that could be handled by
clk_register_alias. Most of the clocks have a singleclk_lookup.In my idea only few of the clocks have a
clk_lookup(you mentioned a clock between the i2c divider and i2c gate elsewhere, this would never be passed to a device).Sascha
Signature · 9 lines
Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel at lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Re: [PATCH 1/4] clk: Add a generic clock infrastructure
In reply to Colin CrossQuoted 8 lines · Colin Cross
Quoted 7 lines · Sascha Hauer
Quoted 6 lines
Quoted 1 line
+ ? ? ? struct clk ? ? ? ? ? ? ?*parent;
Storing the list of possible parents at this level can help abstract some common code from mux ops if you pass the index into the list of the new parent into the op - most mux ops only need to know which of their mux inputs needs to be enabled.
Quoted 8 lines
Quoted 2 lines
Please don't. Only muxes have more than one possible parent, so this should be handled there.
The cost is one pointer per clock that is not actually a mux, and the benefit is that you can move a very common search loop out of every mux implementation into the framework. It also lets you determine which clocks are connected, which becomes necessary if you try to do per-tree locking or sysfs controls for clocks.
We can be more clever than storing the world and some more in the internal implementation.
struct clk should be just the basic data structure to hold the information which is shared by all clk types. The way I see it is:
struct clk {
struct clk_ops *ops;
struct clk_data *data;
void *hwdata;
unsigned long flags;
....
};Now have
struct clk_data {
union {
struct clk_divider_data;
struct clk_mux_data;
struct clk_pll_data;
.....
};
};Now on registration time you tell the core which type you are handing in. struct clk_mux_data would contain the list of possible parent clocks and clk_pll_data the possible multiplier values (linear or table based). Same for dividers. Now the core can do the evaluation of clock rate settings or parents and hand in the index or the linear range value into ops->set_rate/set_parent. And with the information available to the core you can do even complex settings which involve selecting the proper pll for your pixelclock example below.
Quoted 11 lines
Quoted 3 lines
I hope we can avoid that. The decision of the best parent should be left up to the user. Doing this in the mux/divider implementation would torpedo attempts to implement generic building blocks.
I agree it would be nice, but it does push some knowledge of the clock tree into device drivers. For example, on Tegra the display driver may need to change the source pll of the display clock to get the required pclk, which requires passing all the possible parents of the display clock into the display driver. If this is a common usage pattern, there needs to be a hook in the ops to allow the clock or clock chip to make a more global decision.
No, we really dont want to have that knowledge at the driver level. A driver should just say: Enable "pixelclk" at rate X.
So then a mapper converts pixelclk to the particular clock on the SoC it's running on and the clk framework level needs to figure out how to achieve that by looking at the full chain down from the clk gate which feeds the pixelclk. We don't want drivers to be aware about that at all. They simply do not care and that's a preliminary for making drivers usable on different SoC incarnations.
Thanks,
tglx
Re: [PATCH 1/4] clk: Add a generic clock infrastructure
In reply to Jeremy Kerrdrivers/clk/clk.c line 28
Quoted 2 lines · Jeremy Kerr
drivers/clk/clk.c new file +2
-0+static DEFINE_SPINLOCK (enable_lock); +static DEFINE_MUTEX (prepare_lock);
Probably all clocks can be handled separately, i.e. single lock for all of them will make system slower. Suppose i want to enable UART's clock then why should spi code be waiting for the lock.
So, we should have per clk lock.
<...>
Quoted 6 lines
drivers/clk/clk.c new file +6
-0+struct clk *clk_register (struct clk_hw_ops *ops,struct clk_hw *hw, +const char *name) +{ +struct clk *clk; + + clk =kzalloc (sizeof (*clk),GFP_KERNEL );
drivers/clk/clk.c line 217
Quoted 10 lines
drivers/clk/clk.c new file +10
-0+if (!clk) +return NULL ; + + clk->name = name; + clk->ops = ops; + clk->hw = hw; + hw->clk = clk; + +/* Query the hardware for parent and initial rate */ +
Can remove this blank line.
drivers/clk/clk.c line 220
Quoted 3 lines
drivers/clk/clk.c new file +3
-0+if (clk->ops->get_parent) +/* We don't to lock against prepare/enable here, as +* the clock is not yet accessible from anywhere */
Shouldn't we use following style for multi-line comments. /*
* ....
*/
drivers/clk/clk.c line 226
Quoted 6 lines
drivers/clk/clk.c new file +6
-0+ clk->parent = clk->ops->get_parent (clk->hw); + +if (clk->ops->recalc_rate) + clk->rate = clk->ops->recalc_rate (clk->hw); + +
Can remove one of these blank lines.
Signature · 1 line
viresh
Re: [PATCH 1/4] clk: Add a generic clock infrastructure
In reply to Jeremy KerrQuoted 32 lines · Jeremy Kerr
We currently have ~21 definitions of struct clk in the ARM architecture, each defined on a per-platform basis. This makes it difficult to define platform- (or architecture-) independent clock sources without making assumptions about struct clk, and impossible to compile two platforms with different struct clks into a single image.
This change is an effort to unify struct clk where possible, by defining a common struct clk, and a set of clock operations. Different clock implementations can set their own operations, and have a standard interface for generic code. The callback interface is exposed to the kernel proper, while the clock implementations only need to be seen by the platform internals.
The interface is split into two halves:
* struct clk, which is the generic-device-driver interface. This provides a set of functions which drivers may use to request enable/disable, query or manipulate in a hardware-independent manner.
* struct
clk_hwand structclk_hw_ops, which is the hardware-specific interface. Clock drivers implement the ops, which allow the core clock code to implement the generic 'struct clk' API.This allows us to share clock code among platforms, and makes it possible to dynamically create clock devices in platform-independent code.
Platforms can enable the generic struct clock through
CONFIG_GENERIC_CLK. In this case, the clock infrastructure consists of a common, opaque struct clk, and a set of clock operations (defined per type of clock):
Quoted 6 lines
struct clk_hw_ops {
int (*prepare)(struct clk_hw *);
void (*unprepare)(struct clk_hw *);
int (*enable)(struct clk_hw *);
void (*disable)(struct clk_hw *);
unsigned long (*recalc_rate)(struct clk_hw *);why not pass down parent rate?
Quoted 3 lines
int (*set_rate)(struct
clk_hw*,
unsigned long, unsigned long *);
long (*round_rate)(structclk_hw*, unsigned long);
ditto
Quoted 4 lines
int (*set_parent)(struct clk_hw *, struct clk *); struct clk * (*get_parent)(struct clk_hw *); };
[...]
Quoted 16 lines
drivers/clk/clk.c new file +10
-0@@ -0,0 +1,229 @@ +/* +* Copyright (C) 2010-2011 Canonical Ltd <[email protected]> +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License version 2 as +* published by the Free Software Foundation. +* +* Standard functionality for the common clock API. +*/ +
drivers/clk/clk.c line 20
Quoted 10 lines
drivers/clk/clk.c new file +10
-0+#include <linux/clk.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/slab.h> +#include <linux/spinlock.h> + +struct clk { +const char *name; +struct clk_hw_ops *ops; +struct clk_hw *hw;
If we don't let it void *, why not put clk_hw_ops into clk_hw too? It'll be more easy to define static clocks.
Quoted 144 lines
drivers/clk/clk.c new file +144
-0+unsigned int enable_count; +unsigned int prepare_count; +struct clk *parent; +unsigned long rate; +}; + +static DEFINE_SPINLOCK (enable_lock); +static DEFINE_MUTEX (prepare_lock); + +static 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->hw); + +__clk_unprepare (clk->parent); +} + +void clk_unprepare (struct clk *clk) +{ +mutex_lock (&prepare_lock); +__clk_unprepare (clk); +mutex_unlock (&prepare_lock); +} +EXPORT_SYMBOL_GPL (clk_unprepare); + +static 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->hw); +if (ret) { +__clk_unprepare (clk->parent); +return ret; + } + } + } + + clk->prepare_count++; + +return 0 ; +} + +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); + +static 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->hw); +__clk_disable (clk->parent); +} + +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); + +static int __clk_enable (struct clk *clk) +{ +int ret; + +if (!clk) +return 0 ; + +if (WARN_ON (clk->prepare_count ==0 )) +return -ESHUTDOWN ; + + +if (clk->enable_count ==0 ) { + ret =__clk_enable (clk->parent); +if (ret) +return ret; + +if (clk->ops->enable) { + ret = clk->ops->enable (clk->hw); +if (ret) { +__clk_disable (clk->parent); +return ret; + } + } + } + + clk->enable_count++; +return 0 ; +} + +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); + +unsigned long clk_get_rate (struct clk *clk) +{
drivers/clk/clk.c line 174
Quoted 10 lines
drivers/clk/clk.c new file +10
-0+if (!clk) +return 0 ; +return clk->rate; +} +EXPORT_SYMBOL_GPL (clk_get_rate); + +long clk_round_rate (struct clk *clk,unsigned long rate) +{ +if (clk && clk->ops->round_rate) +return clk->ops->round_rate (clk->hw, rate);
pass parent rate? IMHO, common code can handle parent as much as possible.
Quoted 40 lines
drivers/clk/clk.c new file +40
-0+return rate; +} +EXPORT_SYMBOL_GPL (clk_round_rate); + +int clk_set_rate (struct clk *clk,unsigned long rate) +{ +/* not yet implemented */ +return -ENOSYS ; +} +EXPORT_SYMBOL_GPL (clk_set_rate); + +struct clk *clk_get_parent (struct clk *clk) +{ +if (!clk) +return NULL ; + +return clk->parent; +} +EXPORT_SYMBOL_GPL (clk_get_parent); + +int clk_set_parent (struct clk *clk,struct clk *parent) +{ +/* not yet implemented */ +return -ENOSYS ; +} +EXPORT_SYMBOL_GPL (clk_set_parent); + +struct clk *clk_register (struct clk_hw_ops *ops,struct clk_hw *hw, +const char *name) +{ +struct clk *clk; + + clk =kzalloc (sizeof (*clk),GFP_KERNEL ); +if (!clk) +return NULL ; + + clk->name = name; + clk->ops = ops; + clk->hw = hw; + hw->clk = clk;
drivers/clk/clk.c line 224
Quoted 10 lines
drivers/clk/clk.c new file +10
-0+ +/* Query the hardware for parent and initial rate */ + +if (clk->ops->get_parent) +/* We don't to lock against prepare/enable here, as +* the clock is not yet accessible from anywhere */ + clk->parent = clk->ops->get_parent (clk->hw); + +if (clk->ops->recalc_rate) + clk->rate = clk->ops->recalc_rate (clk->hw);
ditto
Thanks
Richard
Quoted 169 lines
drivers/clk/clk.c new file +5
-0+ + +return clk; +} +EXPORT_SYMBOL_GPL (clk_register);drivers/clk/clkdev.c +7
-0@@ -23,6 +23,13 @@static LIST_HEAD (clocks);static DEFINE_MUTEX (clocks_mutex); +/* For USE_COMMON_STRUCT_CLK, these are provided in clk.c, but not exported +* through other headers; we don't want them used anywhere but here. */ +#ifdef CONFIG_USE_COMMON_STRUCT_CLK +extern int __clk_get (struct clk *clk); +extern void __clk_put (struct clk *clk); +#endif +/* * Find the correct struct clk for the device and connection ID. * We do slightly fuzzy matching here: include/linux/clk.h +92
-10@@ -3,6 +3,7 @@* * Copyright (C) 2004 ARM Limited. * Written by Deep Blue Solutions Limited. +* Copyright (c) 2010-2011 Jeremy Kerr <[email protected]> * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License version 2 as @@ -11,17 +12,103 @@#ifndef __LINUX_CLK_H#define __LINUX_CLK_H +#include <linux/err.h> +#include <linux/spinlock.h> +struct device ; -/* -* The base API. +struct clk ; + +#ifdef CONFIG_GENERIC_CLK + +struct clk_hw { +struct clk *clk; +}; + +/** +* 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. Called with the global clock mutex +* held. Optional, but recommended - if this op is not set, +* clk_get_rate will return 0. +* +* @get_parent Query the parent of this clock; for clocks with multiple +* possible parents, query the hardware for the current +* parent. Currently only called when the clock is first +* registered. +* +* 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 a clock requires sleeping code to be turned on, this +* should be done in clk_prepare. Switching that will not sleep should be done +* 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_hw_ops { +int (*prepare)(struct clk_hw *); +void (*unprepare)(struct clk_hw *); +int (*enable)(struct clk_hw *); +void (*disable)(struct clk_hw *); +unsigned long (*recalc_rate)(struct clk_hw *); +long (*round_rate)(struct clk_hw *,unsigned long ); +struct clk * (*get_parent)(struct clk_hw *); +}; + +/** +* clk_prepare - prepare clock for atomic enabling. +* +* @clk: The clock to prepare +* +* Do any possibly sleeping initialisation on @clk, allowing the clock to be +* later enabled atomically (via clk_enable). This function may sleep. */ +int clk_prepare (struct clk *clk); +/** +* clk_unprepare - release clock from prepared state +* +* @clk: The clock to release +* +* Do any (possibly sleeping) cleanup on clk. This function may sleep. +*/ +void clk_unprepare (struct clk *clk); + +#else /* !CONFIG_GENERIC_CLK */ /* -* struct clk - an machine class defined object / cookie. +* For !CONFIG_GENERIC_CLK, we don't enforce any atomicity +* requirements for clk_enable/clk_disable, so the prepare and unprepare +* functions are no-ops */ -struct clk ; +static inline int clk_prepare (struct clk *clk) {return 0 ; } +static inline void clk_unprepare (struct clk *clk) { } + +#endif /* !CONFIG_GENERIC_CLK */ /** * clk_get - lookup and obtain a reference to a clock producer. @@ -67,6 +154,7 @@ void clk_disable(struct clk *clk);/** * clk_get_rate - obtain the current clock rate (in Hz) for a clock source. * This is only valid once the clock source has been enabled. +* Returns zero if the clock rate is unknown. * @clk: clock source */ unsigned long clk_get_rate (struct clk *clk);@@ -83,12 +171,6 @@ unsigned long clk_get_rate(struct clk *clk);*/ void clk_put (struct clk *clk); - -/* -* The remaining APIs are optional for machine class support. -*/ - -/** * clk_round_rate - adjust a rate to the exact rate a clock can provide * @clk: clock source _______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Re: [PATCH 1/4] clk: Add a generic clock infrastructure
Quoted 8 lines · On Wednesday, May 25, 2011 06:47:50 Richard Zhao wrote:
drivers/clk/clk.c line 20
Quoted 4 lines · Jeremy Kerr
drivers/clk/clk.c new file +4
-0+struct clk { +const char *name; +struct clk_hw_ops *ops; +struct clk_hw *hw;If we don't let it void *, why not put
clk_hw_opsintoclk_hwtoo? It'll be more easy to define static clocks.
because the ops should be const, and possibly shared among multiple clocks.
i.e. if i have 4 SPI buses, there should only be one copy of the ops structure
but 4 clk_hw instances.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110530/617b00bc/attachment.sig>
PATCH 2/4 clk: Implement clk_set_rate
In reply to Jeremy KerrImplemenent clk_set_rate by adding a set_rate callback to clk_hw_ops, and core code to handle propagation of rate changes up and down the clock tree.
--- drivers/clk/clk.c | 74 ++++++++++++++++++++++++++++++++++++++++---- include/linux/clk.h | 12 +++++++ 2 files changed, 80 insertions(+), 6 deletions(-)
drivers/clk/clk.c +68 -6
@@ -21,6 +21,8 @@ struct clk {unsigned int enable_count;unsigned int prepare_count;struct clk *parent; +struct hlist_head children; +struct hlist_node child_node;unsigned long rate; };
@@ -176,10 +178,61 @@ long clk_round_rate(struct clk *clk, unsigned long rate) }EXPORT_SYMBOL_GPL (clk_round_rate); +/* +* clk_recalc_rates - Given a clock (with a recently updated clk->rate), +* notify its children that the rate may need to be recalculated, using +* ops->recalc_rate(). +*/ +static void clk_recalc_rates (struct clk *clk) +{ +struct hlist_node *tmp; +struct clk *child; + +if (clk->ops->recalc_rate) + clk->rate = clk->ops->recalc_rate (clk->hw); + +hlist_for_each_entry (child, tmp, &clk->children, child_node) +clk_recalc_rates (child); +} +int clk_set_rate (struct clk *clk,unsigned long rate) { -/* not yet implemented */ -return -ENOSYS ; +unsigned long parent_rate, new_rate; +int ret; + +if (!clk->ops->set_rate) +return -ENOSYS ; + + new_rate = rate; + +/* prevent racing with updates to the clock topology */ +mutex_lock (&prepare_lock); + +propagate: + ret = clk->ops->set_rate (clk->hw, new_rate, &parent_rate); + +if (ret <0 ) +return ret; + +/* ops->set_rate may require the parent's rate to change (to +* parent_rate), we need to propagate the set_rate call to the +* parent. +*/ +if (ret ==CLK_SET_RATE_PROPAGATE ) { + new_rate = parent_rate; + clk = clk->parent; + goto propagate; + } + +/* If successful (including propagation to the parent clock(s)), +* recalculate the rates of the clock, including children. We'll be +* calling this on the 'parent-most' clock that we propagated to. +*/ +clk_recalc_rates (clk); + +mutex_unlock (&prepare_lock); + +return 0 ; }EXPORT_SYMBOL_GPL (clk_set_rate);
@@ -213,16 +266,25 @@ struct clk *clk_register(struct clk_hw_ops *ops, struct clk_hw *hw, clk->hw = hw; hw->clk = clk; -/* Query the hardware for parent and initial rate */ +/* Query the hardware for parent and initial rate. We may alter +* the clock topology, making this clock available from the parent's +* children list. So, we need to protect against concurrent +* accesses through set_rate +*/ +mutex_lock (&prepare_lock); -if (clk->ops->get_parent) -/* We don't to lock against prepare/enable here, as -* the clock is not yet accessible from anywhere */ +if (clk->ops->get_parent) { clk->parent = clk->ops->get_parent (clk->hw); +if (clk->parent) +hlist_add_head (&clk->child_node, + &clk->parent->children); + }if (clk->ops->recalc_rate) clk->rate = clk->ops->recalc_rate (clk->hw); +mutex_unlock (&prepare_lock); +return clk; }
include/linux/clk.h +12 -0
@@ -58,6 +58,12 @@ struct clk_hw {* parent. Currently only called when the clock is first * registered. * +* @set_rate Change the rate of this clock. If this callback returns +* CLK_SET_RATE_PROPAGATE, the rate change will be propagated to +* the parent clock (which may propagate again). The requested +* rate of the parent is passed back from the callback in the +* second 'unsigned long *' argument. +* * 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 a clock requires sleeping code to be turned on, this
@@ -76,9 +82,15 @@ struct clk_hw_ops {void (*disable)(struct clk_hw *);unsigned long (*recalc_rate)(struct clk_hw *);long (*round_rate)(struct clk_hw *,unsigned long ); +int (*set_rate)(struct clk_hw *, +unsigned long ,unsigned long *);struct clk * (*get_parent)(struct clk_hw *); }; +enum { +CLK_SET_RATE_PROPAGATE =1 , +}; +/** * clk_prepare - prepare clock for atomic enabling. *
Re: [PATCH 2/4] clk: Implement clk_set_rate
Quoted 55 lines · Jeremy Kerr
Implemenent
clk_set_rateby adding aset_ratecallback toclk_hw_ops, and core code to handle propagation of rate changes up and down the clock tree.Signed-off-by Jeremy Kerr <[email protected]>--- drivers/clk/clk.c | 74 ++++++++++++++++++++++++++++++++++++++++---- include/linux/clk.h | 12 +++++++ 2 files changed, 80 insertions(+), 6 deletions(-)drivers/clk/clk.c +25
-2@@ -21,6 +21,8 @@ struct clk {unsigned int enable_count;unsigned int prepare_count;struct clk *parent; +struct hlist_head children; +struct hlist_node child_node;unsigned long rate; };@@ -176,10 +178,61 @@ long clk_round_rate(struct clk *clk, unsigned long rate) }EXPORT_SYMBOL_GPL (clk_round_rate); +/* +* clk_recalc_rates - Given a clock (with a recently updated clk->rate), +* notify its children that the rate may need to be recalculated, using +* ops->recalc_rate(). +*/ +static void clk_recalc_rates (struct clk *clk) +{ +struct hlist_node *tmp; +struct clk *child; + +if (clk->ops->recalc_rate) + clk->rate = clk->ops->recalc_rate (clk->hw); + +hlist_for_each_entry (child, tmp, &clk->children, child_node) +clk_recalc_rates (child); +} +int clk_set_rate (struct clk *clk,unsigned long rate) { -/* not yet implemented */ -return -ENOSYS ; +unsigned long parent_rate, new_rate; +int ret; + +if (!clk->ops->set_rate) +return -ENOSYS ; +
drivers/clk/clk.c long clk_round_rate(struct clk *clk, unsigned long rate) · line 215
Quoted 10 lines
drivers/clk/clk.c +10
-0+ new_rate = rate; + +/* prevent racing with updates to the clock topology */ +mutex_lock (&prepare_lock); + +propagate: + ret = clk->ops->set_rate (clk->hw, new_rate, &parent_rate); + +if (ret <0 ) +return ret;
Should be 'goto out_recalc' instead.
If the first ops->set_rate succeedes but then the parent fails the rate already has changed. I can't see whether this becomes a problem when clk_set_rate fails but still changes the rate, but in any way we have to call clk_recalc_rates() before leaving to keep the tree in sync with the hardware. Also mutex_unlock is missing.
Quoted 92 lines
drivers/clk/clk.c +33
-4+ +/* ops->set_rate may require the parent's rate to change (to +* parent_rate), we need to propagate the set_rate call to the +* parent. +*/ +if (ret ==CLK_SET_RATE_PROPAGATE ) { + new_rate = parent_rate; + clk = clk->parent; + goto propagate; + } + +/* If successful (including propagation to the parent clock(s)), +* recalculate the rates of the clock, including children. We'll be +* calling this on the 'parent-most' clock that we propagated to. +*/ +clk_recalc_rates (clk); + +mutex_unlock (&prepare_lock); + +return 0 ; }EXPORT_SYMBOL_GPL (clk_set_rate);@@ -213,16 +266,25 @@ struct clk *clk_register(struct clk_hw_ops *ops, struct clk_hw *hw, clk->hw = hw; hw->clk = clk; -/* Query the hardware for parent and initial rate */ +/* Query the hardware for parent and initial rate. We may alter +* the clock topology, making this clock available from the parent's +* children list. So, we need to protect against concurrent +* accesses through set_rate +*/ +mutex_lock (&prepare_lock); -if (clk->ops->get_parent) -/* We don't to lock against prepare/enable here, as -* the clock is not yet accessible from anywhere */ +if (clk->ops->get_parent) { clk->parent = clk->ops->get_parent (clk->hw); +if (clk->parent) +hlist_add_head (&clk->child_node, + &clk->parent->children); + }if (clk->ops->recalc_rate) clk->rate = clk->ops->recalc_rate (clk->hw); +mutex_unlock (&prepare_lock); +return clk; }include/linux/clk.h +12
-0@@ -58,6 +58,12 @@ struct clk_hw {* parent. Currently only called when the clock is first * registered. * +* @set_rate Change the rate of this clock. If this callback returns +* CLK_SET_RATE_PROPAGATE, the rate change will be propagated to +* the parent clock (which may propagate again). The requested +* rate of the parent is passed back from the callback in the +* second 'unsigned long *' argument. +* * 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 a clock requires sleeping code to be turned on, this @@ -76,9 +82,15 @@ struct clk_hw_ops {void (*disable)(struct clk_hw *);unsigned long (*recalc_rate)(struct clk_hw *);long (*round_rate)(struct clk_hw *,unsigned long ); +int (*set_rate)(struct clk_hw *, +unsigned long ,unsigned long *);struct clk * (*get_parent)(struct clk_hw *); }; +enum { +CLK_SET_RATE_PROPAGATE =1 , +}; +/** * clk_prepare - prepare clock for atomic enabling. * _______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Signature · 4 lines
Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Re: [PATCH 2/4] clk: Implement clk_set_rate
In reply to Jeremy KerrQuoted 123 lines · Jeremy Kerr
Implemenent
clk_set_rateby adding aset_ratecallback toclk_hw_ops, and core code to handle propagation of rate changes up and down the clock tree.Signed-off-by Jeremy Kerr <[email protected]>---
?drivers/clk/clk.c ? | ? 74 ++++++++++++++++++++++++++++++++++++++++---- ?include/linux/clk.h | ? 12 +++++++
?2 files changed, 80 insertions(+), 6 deletions(-)drivers/clk/clk.c +68
-6@@ -21,6 +21,8 @@ struct clk { ? ? ? ?unsigned int ? ? ? ? ? ?enable_count; ? ? ? ?unsigned int ? ? ? ? ? ?prepare_count; ? ? ? ?struct clk ? ? ? ? ? ? ?*parent; + ? ? ? struct hlist_head ? ? ? children; + ? ? ? struct hlist_node ? ? ? child_node; ? ? ? ?unsigned long ? ? ? ? ? rate; ?};@@ -176,10 +178,61 @@ long clk_round_rate(struct clk *clk, unsigned long rate) ?} ?EXPORT_SYMBOL_GPL(clk_round_rate); +/* +* clk_recalc_rates - Given a clock (with a recently updated clk->rate), +* ? ? notify its children that the rate may need to be recalculated, using +* ? ? ops->recalc_rate(). +*/ +static void clk_recalc_rates (struct clk *clk) +{ + ? ? ? struct hlist_node *tmp; + ? ? ? struct clk *child; + + ? ? ? if (clk->ops->recalc_rate) + ? ? ? ? ? ? ? clk->rate = clk->ops->recalc_rate(clk->hw); + + ? ? ? hlist_for_each_entry(child, tmp, &clk->children, child_node) + ? ? ? ? ? ? ? clk_recalc_rates(child); +} + ?int clk_set_rate(struct clk *clk, unsigned long rate) ?{ - ? ? ? /* not yet implemented */ - ? ? ? return -ENOSYS; + ? ? ? unsigned long parent_rate, new_rate; + ? ? ? int ret; + + ? ? ? if (!clk->ops->set_rate) + ? ? ? ? ? ? ? return -ENOSYS; + + ? ? ? new_rate = rate; + + ? ? ? /* prevent racing with updates to the clock topology */ + ? ? ? mutex_lock(&prepare_lock); + +propagate: + ? ? ? ret = clk->ops->set_rate(clk->hw, new_rate, &parent_rate); + + ? ? ? if (ret < 0) + ? ? ? ? ? ? ? return ret; + + ? ? ? /* ops->set_rate may require the parent's rate to change (to + ? ? ? ?* parent_rate), we need to propagate the set_rate call to the + ? ? ? ?* parent. + ? ? ? ?*/ + ? ? ? if (ret == CLK_SET_RATE_PROPAGATE) { + ? ? ? ? ? ? ? new_rate = parent_rate; + ? ? ? ? ? ? ? clk = clk->parent; + ? ? ? ? ? ? ? goto propagate; + ? ? ? } + + ? ? ? /* If successful (including propagation to the parent clock(s)), + ? ? ? ?* recalculate the rates of the clock, including children. ?We'll be + ? ? ? ?* calling this on the 'parent-most' clock that we propagated to. + ? ? ? ?*/ + ? ? ? clk_recalc_rates(clk); + + ? ? ? mutex_unlock(&prepare_lock); + + ? ? ? return 0; ?} ?EXPORT_SYMBOL_GPL(clk_set_rate);@@ -213,16 +266,25 @@ struct clk *clk_register(struct clk_hw_ops *ops, struct clk_hw *hw, ? ? ? ?clk->hw = hw; ? ? ? ?hw->clk = clk; - ? ? ? /* Query the hardware for parent and initial rate */ + ? ? ? /* Query the hardware for parent and initial rate. We may alter + ? ? ? ?* the clock topology, making this clock available from the parent's + ? ? ? ?* children list. So, we need to protect against concurrent + ? ? ? ?* accesses through set_rate + ? ? ? ?*/ + ? ? ? mutex_lock(&prepare_lock); - ? ? ? if (clk->ops->get_parent) - ? ? ? ? ? ? ? /* We don't to lock against prepare/enable here, as - ? ? ? ? ? ? ? ?* the clock is not yet accessible from anywhere */ + ? ? ? if (clk->ops->get_parent) { ? ? ? ? ? ? ? ?clk->parent = clk->ops->get_parent(clk->hw); + ? ? ? ? ? ? ? if (clk->parent) + ? ? ? ? ? ? ? ? ? ? ? hlist_add_head(&clk->child_node, + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? &clk->parent->children); + ? ? ? } ? ? ? ?if (clk->ops->recalc_rate) ? ? ? ? ? ? ? ?clk->rate = clk->ops->recalc_rate(clk->hw); + ? ? ? mutex_unlock(&prepare_lock); + ? ? ? ?return clk; ?}include/linux/clk.h +0
-0@@ -58,6 +58,12 @@ struct clk_hw {
Quoted 8 lines
?* ? ? ? ? ? ? parent. Currently only called when the clock is first ?* ? ? ? ? ? ? registered.
?*
+ * @set_rate ? Change the rate of this clock. If this callback returns
+ * ? ? ? ? ? ?CLK_SET_RATE_PROPAGATE, the rate change will be propagated to
+ * ? ? ? ? ? ? the parent clock (which may propagate again). The requested
+ * ? ? ? ? ? ? rate of the parent is passed back from the callback in the
+ * ? ? ? ? ? ? second 'unsigned long *' argument.
This seems backwards to me, it requires children to have knowledge of the best states for their parents. I don't think it can ever be implemented safely, either. If the child's set rate needs to decrease its divider value to increase its rate, but increase the parents divider to decrease the rate, the clock can end up running too fast, and out of spec, after set_rate on the child clock has finished, but set_rate on the parent clock has not been called yet. And if the parent clock errors out, clk_set_rate returns an error, but the rate has still changed to some random intermediate value.
Can you explain a use case where propagation is necessary? It doesn't match the Sascha's reply to my comments on the main patch, where he said users are best suited to make the decision on the correct parent, but child clocks are best suited to make the decision on the parent's rate? Can you point me to a current clock implementation that does anything like this?
Quoted 26 lines
+ *
?* 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 a clock requires sleeping code to be turned on, thisinclude/linux/clk.h +6
-0@@ -76,9 +82,15 @@ struct clk_hw_ops { ? ? ? ?void ? ? ? ? ? ?(*disable)(struct clk_hw *); ? ? ? ?unsigned long ? (*recalc_rate)(struct clk_hw *); ? ? ? ?long ? ? ? ? ? ?(*round_rate)(struct clk_hw *, unsigned long); + ? ? ? int ? ? ? ? ? ? (*set_rate)(struct clk_hw *, + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? unsigned long, unsigned long *); ? ? ? ?struct clk * ? ?(*get_parent)(struct clk_hw *); ?}; +enum { + ? ? ? CLK_SET_RATE_PROPAGATE = 1, +}; +?/**
?*clk_prepare- prepare clock for atomic enabling. ?*_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Re: [PATCH 2/4] clk: Implement clk_set_rate
In reply to Jeremy KerrHi Jeremy,
Quoted 12 lines · Jeremy Kerr
Implemenent
clk_set_rateby adding aset_ratecallback toclk_hw_ops, and core code to handle propagation of rate changes up and down the clock tree.Signed-off-by Jeremy Kerr <[email protected]>drivers/clk/clk.c +6
-0+ +propagate: + ret = clk->ops->set_rate (clk->hw, new_rate, &parent_rate); + +if (ret <0 ) +return ret;
drivers/clk/clk.c long clk_round_rate(struct clk *clk, unsigned long rate) · line 225
Quoted 10 lines
drivers/clk/clk.c +10
-0+ +/* ops->set_rate may require the parent's rate to change (to +* parent_rate), we need to propagate the set_rate call to the +* parent. +*/ +if (ret ==CLK_SET_RATE_PROPAGATE ) { + new_rate = parent_rate; + clk = clk->parent; + goto propagate; + }
I'm unsure about this one. Every clock should have the ability to stop or continue the rate propagation to the parent. This suggests to leave the decision whether or not to propagate to the core and not to the individual clocks.
Right now each mux/div/gate needs an individual propagate flag. By adding the flag to the core the building block implementations could be simpler and the knowledge about propagatability might become handy for the core later.
Sascha
Signature · 4 lines
Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
A message that is not in the archive (sent off-list or not yet received): 1306373867.2875.162.camel@pororo
Re: [PATCH 2/4] clk: Implement clk_set_rate
[put the lists back on cc]
Quoted 24 lines · Jeremy Kerr
Hi Sascha,
Quoted 13 lines
Quoted 13 lines
drivers/clk/clk.c +13
-0+ +propagate: + ret = clk->ops->set_rate (clk->hw, new_rate, &parent_rate); + +if (ret <0 ) +return ret; + +/* ops->set_rate may require the parent's rate to change (to +* parent_rate), we need to propagate the set_rate call to the +* parent. +*/ +if (ret ==CLK_SET_RATE_PROPAGATE ) { + new_rate = parent_rate;Quoted 8 lines
drivers/clk/clk.c long clk_round_rate(struct clk *clk, unsigned long rate) · line 225
Quoted 3 lines
drivers/clk/clk.c +3
-0+ clk = clk->parent; + goto propagate; + }I'm unsure about this one. Every clock should have the ability to stop or continue the rate propagation to the parent. This suggests to leave the decision whether or not to propagate to the core and not to the individual clocks.
Quoted 5 lines
Maybe I'm not understanding you correctly, but that's exactly what this code does. The decision to propagate is left up to the implementation-specific
set_ratecallback - if it returnsCLK_SET_RATE_PROPAGATE(and populate theparent_rateargument with the requested parent rate), then we propagate the rate change to the parent.
I understood how the code is meant to work. It's just that IMO the place where the propagation flag is stored is the wrong one, given that it's a flag that all clocks (can) have.
Quoted 9 lines
Quoted 5 lines
Right now each mux/div/gate needs an individual propagate flag. By adding the flag to the core the building block implementations could be simpler and the knowledge about propagatability might become handy for the core later.
We could do this with a flag too, yes. But then there's no way of altering the rate (which we need to do with a divider) as we propagate it upwards. The current
set_ratecode lets us do that.
Hm, the core could pass a NULL pointer as the third argument to set_rate to indicate that the parent rate is not allowed to change. Then we could initialize &parent_rate to zero before calling set_rate. If the set_rate function does not change it, we don't have to propagate, otherwise yes. Or instead we could just use the CLK_SET_RATE_PROPAGATE return value like we do in the current version.
Sascha
Signature · 4 lines
Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Re: [PATCH 2/4] clk: Implement clk_set_rate
In reply to Jeremy Kerrdrivers/clk/clk.c long clk_round_rate(struct clk *clk, unsigned long rate) · line 204
Quoted 9 lines · On Friday, May 20, 2011 03:27:49 Jeremy Kerr wrote:
int
clk_set_rate(struct clk *clk, unsigned long rate){drivers/clk/clk.c +5
-2-/* not yet implemented */ -return -ENOSYS ; +unsigned long parent_rate, new_rate; +int ret; + +if (!clk->ops->set_rate) +return -ENOSYS ;
i thought ENOSYS is only for syscalls. shouldnt this be ENODEV or perhaps
EOPNOTSUPP ?
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110530/64fac7f1/attachment.sig>
PATCH 3/4 clk: Add fixed-rate clock
In reply to Jeremy Kerr--- drivers/clk/Kconfig | 4 ++++ drivers/clk/Makefile | 1 + drivers/clk/clk-fixed.c | 17 +++++++++++++++++ include/linux/clk.h | 14 ++++++++++++++ 4 files changed, 36 insertions(+)
drivers/clk/Kconfig +4 -0
@@ -5,3 +5,7 @@ config CLKDEV_LOOKUP config GENERIC_CLK bool + +config GENERIC_CLK_FIXED + bool + depends on GENERIC_CLK
drivers/clk/Makefile +1 -0
@@ -1,3 +1,4 @@ obj-$(CONFIG_CLKDEV_LOOKUP) += clkdev.o obj-$(CONFIG_GENERIC_CLK) += clk.o +obj-$(CONFIG_GENERIC_CLK_FIXED) += clk-fixed.o
drivers/clk/clk-fixed.c new file +17 -0
@@ -0,0 +1,17 @@ + +#include <linux/clk.h> +#include <linux/module.h> + +#define to_clk_fixed (c) container_of(c, struct clk_hw_fixed, hw) + +static unsigned long clk_fixed_recalc_rate (struct clk_hw *hw) +{ +return to_clk_fixed (hw)->rate; +} + +struct clk_hw_ops clk_fixed_ops = { + .recalc_rate = clk_fixed_recalc_rate, +}; +EXPORT_SYMBOL_GPL (clk_fixed_ops); + +
include/linux/clk.h +14 -0
@@ -110,6 +110,20 @@ int clk_prepare(struct clk *clk);*/ void clk_unprepare (struct clk *clk); +/* Base clock implementations. Platform clock implementations can use these +* directly, or 'subclass' as approprate */ + +#ifdef CONFIG_GENERIC_CLK_FIXED + +struct clk_hw_fixed { +struct clk_hw hw; +unsigned long rate; +}; + +extern struct clk_hw_ops clk_fixed_ops; + +#endif /* CONFIG_GENERIC_CLK_FIXED */ +#else /* !CONFIG_GENERIC_CLK */ /*
PATCH 3/4 clk: Add fixed-rate clock
Hi Jeremy
Is this patch really required?
If I have just a static clk with only a name and a rate; I can obtain the rate with theclk_get_rate(..)
and I don't need extra code/data to manage the fixed_clk (in my
view it's already fixed due to a null struct clk_hw_ops).
Could you clarify what the would address?
Thanks
Regards
Francesco
Quoted 84 lines · Jeremy Kerr
Signed-off-by Jeremy Kerr<[email protected]>--- drivers/clk/Kconfig | 4 ++++ drivers/clk/Makefile | 1 + drivers/clk/clk-fixed.c | 17 +++++++++++++++++ include/linux/clk.h | 14 ++++++++++++++ 4 files changed, 36 insertions(+)drivers/clk/Kconfig +4
-0@@ -5,3 +5,7 @@ config CLKDEV_LOOKUP config GENERIC_CLK bool + +config GENERIC_CLK_FIXED + bool + depends on GENERIC_CLKdrivers/clk/Makefile +1
-0@@ -1,3 +1,4 @@ obj-$(CONFIG_CLKDEV_LOOKUP) += clkdev.o obj-$(CONFIG_GENERIC_CLK) += clk.o +obj-$(CONFIG_GENERIC_CLK_FIXED) += clk-fixed.odrivers/clk/clk-fixed.c new file +17
-0@@ -0,0 +1,17 @@ + +#include <linux/clk.h> +#include <linux/module.h> + +#define to_clk_fixed (c) container_of(c, struct clk_hw_fixed, hw) + +static unsigned long clk_fixed_recalc_rate (struct clk_hw *hw) +{ +return to_clk_fixed (hw)->rate; +} + +struct clk_hw_ops clk_fixed_ops = { + .recalc_rate = clk_fixed_recalc_rate, +}; +EXPORT_SYMBOL_GPL (clk_fixed_ops); + +include/linux/clk.h +14
-0@@ -110,6 +110,20 @@ int clk_prepare(struct clk *clk);*/ void clk_unprepare (struct clk *clk); +/* Base clock implementations. Platform clock implementations can use these +* directly, or 'subclass' as approprate */ + +#ifdef CONFIG_GENERIC_CLK_FIXED + +struct clk_hw_fixed { +struct clk_hw hw; +unsigned long rate; +}; + +extern struct clk_hw_ops clk_fixed_ops; + +#endif /* CONFIG_GENERIC_CLK_FIXED */ +#else /* !CONFIG_GENERIC_CLK */ /* _______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Re: [PATCH 3/4] clk: Add fixed-rate clock
In reply to Jeremy Kerrdrivers/clk/clk-fixed.c line 2
Quoted 5 lines · On Friday, May 20, 2011 03:27:49 Jeremy Kerr wrote:
drivers/clk/clk-fixed.c new file +2
-0@@ -0,0 +1,17 @@ + +#include <linux/clk.h>
missing intro/copyright/license comment block at top of file
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110530/d6882cc7/attachment-0001.sig>
Re: [PATCH 3/4] clk: Add fixed-rate clock
In reply to Jeremy Kerrinclude/linux/clk.h int clk_prepare(struct clk *clk); · line 123
Quoted 8 lines · On Friday, May 20, 2011 03:27:49 Jeremy Kerr wrote:
drivers/clk/clk-fixed.c new file +3
-0+struct clk_hw_ops clk_fixed_ops = { + .recalc_rate = clk_fixed_recalc_rate, +};include/linux/clk.h +1
-0+extern struct clk_hw_ops clk_fixed_ops;
should be const
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110530/6c35116f/attachment.sig>
PATCH 4/4 clk: Add simple gated clock
In reply to Jeremy Kerr--- drivers/clk/Kconfig | 4 ++++ drivers/clk/Makefile | 1 + drivers/clk/clk-gate.c | 41 +++++++++++++++++++++++++++++++++++++++++ include/linux/clk.h | 13 +++++++++++++ 4 files changed, 59 insertions(+)
drivers/clk/Kconfig +4 -0
@@ -9,3 +9,7 @@ config GENERIC_CLK config GENERIC_CLK_FIXED bool depends on GENERIC_CLK + +config GENERIC_CLK_GATE + bool + depends on GENERIC_CLK
drivers/clk/Makefile +1 -0
@@ -2,3 +2,4 @@ obj-$(CONFIG_CLKDEV_LOOKUP) += clkdev.o obj-$(CONFIG_GENERIC_CLK) += clk.o obj-$(CONFIG_GENERIC_CLK_FIXED) += clk-fixed.o +obj-$(CONFIG_GENERIC_CLK_GATE) += clk-gate.o
drivers/clk/clk-gate.c new file +41 -0
41 lines not on this page · raw message
include/linux/clk.h +13 -0
@@ -124,6 +124,19 @@ extern struct clk_hw_ops clk_fixed_ops;#endif /* CONFIG_GENERIC_CLK_FIXED */ +#ifdef CONFIG_GENERIC_CLK_GATE + +struct clk_gate { +struct clk_hw hw; +void __iomem *reg; +u8 bit_idx; +}; + +extern struct clk_hw_ops clk_gate_ops; + +#endif /* CONFIG_GENERIC_CLK_GATE */ + +#else /* !CONFIG_GENERIC_CLK */ /*
Re: [PATCH 4/4] clk: Add simple gated clock
drivers/clk/clk-gate.c line 23
Quoted 11 lines · On Friday 20 May 2011 09:27:49 Jeremy Kerr wrote:
drivers/clk/clk-gate.c new file +11
-0+static int clk_gate_enable (struct clk_hw *clk) +{ +struct clk_gate *gate =to_clk_gate (clk); +u32 reg; + + reg =__raw_readl (gate->reg); + reg |=1 << gate->bit_idx; +__raw_writel (reg, gate->reg); + +return 0 ; +}
__raw_readl/__raw_writel is a rather bad choice for a common driver, it might not do what you need. Unfortunately, readl() also wouldn't do the right thing on all architectures, because it might only be available on PCI buses. Maybe iowrite32 would be best here?
Instead of the bit_idx, a mask value might be better to avoid confusion with the different ways of counting bits from one or the other end.
Arnd
Re: [PATCH 4/4] clk: Add simple gated clock
In reply to Jeremy KerrQuoted 46 lines · Jeremy Kerr
Signed-off-by Jeremy Kerr<[email protected]>--- drivers/clk/Kconfig | 4 ++++ drivers/clk/Makefile | 1 + drivers/clk/clk-gate.c | 41 +++++++++++++++++++++++++++++++++++++++++ include/linux/clk.h | 13 +++++++++++++ 4 files changed, 59 insertions(+)drivers/clk/Kconfig +4
-0@@ -9,3 +9,7 @@ config GENERIC_CLK config GENERIC_CLK_FIXED bool depends on GENERIC_CLK + +config GENERIC_CLK_GATE + bool + depends on GENERIC_CLKdrivers/clk/Makefile +1
-0@@ -2,3 +2,4 @@ obj-$(CONFIG_CLKDEV_LOOKUP) += clkdev.o obj-$(CONFIG_GENERIC_CLK) += clk.o obj-$(CONFIG_GENERIC_CLK_FIXED) += clk-fixed.o +obj-$(CONFIG_GENERIC_CLK_GATE) += clk-gate.odrivers/clk/clk-gate.c new file +10
-0@@ -0,0 +1,41 @@ + +#include <linux/clk.h> +#include <linux/module.h> +#include <asm/io.h> + +#define to_clk_gate (clk) container_of(clk, struct clk_gate, hw) + +static unsigned long clk_gate_get_rate (struct clk_hw *clk) +{ +return clk_get_rate (clk_get_parent (clk->clk));
drivers/clk/clk-gate.c line 20
Quoted 10 lines
drivers/clk/clk-gate.c new file +10
-0+} + +static int clk_gate_enable (struct clk_hw *clk) +{ +struct clk_gate *gate =to_clk_gate (clk); +u32 reg; + + reg =__raw_readl (gate->reg); + reg |=1 << gate->bit_idx; +__raw_writel (reg, gate->reg);
This should be protected with a spinlock for h/w access. You could have a mixture of enable bits with divider fields in one register, so the enable lock won't help here. However, you would have to find a way to have a common spinlock for all accesses to the h/w.
Rob
Quoted 31 lines · Jeremy Kerr
[This series was originally titled 'Add a common struct clk', but the goals have changed since that first set of patches. We're now aiming for a more complete generic clock infrastructure, rather than just abstracting struct clk]
[This series still needs work, see the TODO section below]
[Totally RFC at the moment]
Hi all,
These patches are an attempt to allow platforms to share clock code. At present, the definitions of 'struct clk' are local to platform code, which makes allocating and initialising cross-platform clock sources difficult, and makes it impossible to compile a single image containing support for two ARM platforms with different struct clks.
The three patches are for the architecture-independent kernel code, introducing the common clk infrastructure. The changelog for the first patch includes details about the new clock definitions.
The second patch implements
clk_set_rate, and in doing so adds functionality to walk the clock tree in both directions.
clk_set_parentis left unimplemented, see TODO below for why.The third and fourth patches provide some basic clocks (fixed-rate and gated), mostly to serve as an example of the hardware implementation. I'm intending to later provide similar base clocks for mux and divider hardware clocks.
Quoted 14 lines
Many thanks to the following for their input:
?* Benjamin Herrenschmidt <[email protected]> ?* Thomas Gleixner <[email protected]>
?* Ben Dooks <[email protected]>
?* Baruch Siach <[email protected]>
?* Russell King <[email protected]>
?* Uwe Kleine-K?nig <[email protected]> ?* Lorenzo Pieralisi <[email protected]> ?* Vincent Guittot <[email protected]> ?* Sascha Hauer <[email protected]>
?* Ryan Mallon <[email protected]>
?* Colin Cross <[email protected]>
?* Jassi Brar <[email protected]>
?* Saravana Kannan <[email protected]>
I have a similar set of patches I was working on that handles a little more of the common code than these. I can post them if you want, but for now I'll just point out where I had different ideas, and not muddy the waters with conflicting patches.
Quoted 7 lines
TODO:
?* Need to figure out the locking around
clk_set_parent. Changing the in-kernel ? clock topology requires acquiring both the mutex (to prevent against races ? withclk_prepare, which may propagate to the parent clock) and the spinlock ? (to prevent the same race withclk_enable).
Quoted 5 lines
? However, we should also be changing the hardware clock topology in sync with ? the in-kernel clock topology, which would imply that both locks *also* need ? to be held while updating the parent in the hardware (ie, in
?clk_hw_ops->set_parent) too. ?However, I believe some platform clock ? implementations may require this callback to be able to sleep. Comments?
This sequence is the best I could come up with without adding a temporary 2nd parent:
1. lock prepare mutex
2. call prepare on the new parent if prepare_count > 0
3. lock the enable spinlock
4. call enable on the new parent if enable_count > 0
5. change the parent pointer to the new parent
6. unlock the spinlock
7. call the set_parent callback
8. lock the enable spinlock
9. call disable on the old parent iff you called enable on the new parent (enable_count may have changed)
10. unlock the enable spinlock
11. call unprepare on the old parent if prepare_count
12. unlock prepare mutex
The only possible problem I see is that if a clock starts disabled at step 1., and clk_enable is called on it between steps 6 and 7, clk_enable will return having enabled the new parent, but the clock is still running off the old parent. As soon as the clock gets switched to the new parent, the clock will be properly enabled. I don't see this as a huge problem - calling clk_set_parent on a clock while it is enabled may not even work without causing glitches on some platforms.
I guess the only way around it would be to store a temporary "new_parent" pointer between steps 5 and 9, and have clk_enable/disable operate on both the current parent and the new parent. They would also need to refcount the extra enables separately to undo on the old parent.
Quoted 7 lines
?* tglx and I have been discussing different ways of passing clock information ? to the clock hardware implementation. I'm proposing providing a base 'struct ?
clk_hw', which implementations subclass by wrapping in their own structure ? (or one of a set of simple 'library' structures, likeclk_hw_muxor ?clk_hw_gate). ?Theclk_hwbase is passed as the first argument to all ?clk_hw_opscallbacks.
Quoted 6 lines
? tglx's plan is to create a separate struct
clk_hwdata, which contains a ? union of base data structures for common clocks: div, mux, gate, etc. The ? ops callbacks are passed aclk_hw, plus aclk_hwdata, and most of the base ? hwdata fields are handled within the core clock code. This means less ? encapsulation of clock implementation logic, but more coverage of ? clock basics through the core code.
I don't think they should be a union, I think there should be 3 separate private datas, and three sets of clock ops, for the three different types of clock blocks: rate changers (dividers and plls), muxes, and gates. These blocks are very often combined - a device clock often has a mux and a divider, and clk_set_parent and clk_set_rate on the same struct clk both need to work.
Quoted 26 lines
? tglx, could you send some details about your approach? I'm aiming to refine ? this series with the good bits from each technique.
?* The clock registration code probably needs work. This is the domain ? of the platform/board maintainers, any wishes here?
Cheers,
Jeremy
--
---
Jeremy Kerr (4):
? ? ?clk: Add a generic clock infrastructure
? ? ?clk: Implementclk_set_rate
? ? ?clk: Add fixed-rate clock
? ? ?clk: Add simple gated clock--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo at vger.kernel.org
More majordomo info at ?http://vger.kernel.org/majordomo-info.html Please read the FAQ at ?http://www.tux.org/lkml/
Quoted 8 lines · Colin Cross
Quoted 7 lines
? tglx's plan is to create a separate struct
clk_hwdata, which contains a ? union of base data structures for common clocks: div, mux, gate, etc. The ? ops callbacks are passed aclk_hw, plus aclk_hwdata, and most of the base ? hwdata fields are handled within the core clock code. This means less ? encapsulation of clock implementation logic, but more coverage of ? clock basics through the core code.
Quoted 6 lines
I don't think they should be a union, I think there should be 3 separate private datas, and three sets of clock ops, for the three different types of clock blocks: rate changers (dividers and plls), muxes, and gates. These blocks are very often combined - a device clock often has a mux and a divider, and
clk_set_parentandclk_set_rateon the same struct clk both need to work.
The idea is to being able to propagate functions to the parent. It's very convenient for the implementation of clocks when they only implement either a divider, a mux or a gate. Combining all of these into a single clock leads to complicated clock trees and many different clocks where you can't factor out the common stuff.
Sascha
Signature · 4 lines
Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Quoted 16 lines · Sascha Hauer
Quoted 8 lines · Colin Cross
Quoted 7 lines
? tglx's plan is to create a separate struct
clk_hwdata, which contains a ? union of base data structures for common clocks: div, mux, gate, etc. The ? ops callbacks are passed aclk_hw, plus aclk_hwdata, and most of the base ? hwdata fields are handled within the core clock code. This means less ? encapsulation of clock implementation logic, but more coverage of ? clock basics through the core code.Quoted 6 lines
I don't think they should be a union, I think there should be 3 separate private datas, and three sets of clock ops, for the three different types of clock blocks: rate changers (dividers and plls), muxes, and gates. ?These blocks are very often combined - a device clock often has a mux and a divider, and
clk_set_parentandclk_set_rateon the same struct clk both need to work.
Quoted 5 lines
The idea is to being able to propagate functions to the parent. It's very convenient for the implementation of clocks when they only implement either a divider, a mux or a gate. Combining all of these into a single clock leads to complicated clock trees and many different clocks where you can't factor out the common stuff.
That seems complicated. You end up with lots of extra clocks (meaning more boilerplate in the platform files) that have no meaning in the system (what is the clock between the mux and the divider in Tegra's i2c1 clock, it can never feed any devices), and you have to figure out at the framework level when to propagate and when to error out. I'm not even sure you can always find the right place to stop propagating
- do you just keep going up until the set_parent callback succeeds, or exists, or what?
I think you can still factor out all the common code if you treat each clock as a possible mux, divider, and gate combination. Each part of the clock is still just as abstractable as before - you can set the rate_ops to be the generic single register divider implementation, and the gate ops to be the generic single bit enable implementation. The idea of what a clock node is matches the HW design, and op propagation is easy - if it has rate ops, you can call clk_set_rate on it, otherwise you can't.
Quoted 23 lines · Colin Cross
Quoted 16 lines · Sascha Hauer
Quoted 8 lines · Colin Cross
Quoted 7 lines
? tglx's plan is to create a separate struct
clk_hwdata, which contains a ? union of base data structures for common clocks: div, mux, gate, etc. The ? ops callbacks are passed aclk_hw, plus aclk_hwdata, and most of the base ? hwdata fields are handled within the core clock code. This means less ? encapsulation of clock implementation logic, but more coverage of ? clock basics through the core code.Quoted 6 lines
I don't think they should be a union, I think there should be 3 separate private datas, and three sets of clock ops, for the three different types of clock blocks: rate changers (dividers and plls), muxes, and gates. ?These blocks are very often combined - a device clock often has a mux and a divider, and
clk_set_parentandclk_set_rateon the same struct clk both need to work.Quoted 5 lines
The idea is to being able to propagate functions to the parent. It's very convenient for the implementation of clocks when they only implement either a divider, a mux or a gate. Combining all of these into a single clock leads to complicated clock trees and many different clocks where you can't factor out the common stuff.
Quoted 8 lines
That seems complicated. You end up with lots of extra clocks (meaning more boilerplate in the platform files) that have no meaning in the system (what is the clock between the mux and the divider in Tegra's i2c1 clock, it can never feed any devices), and you have to figure out at the framework level when to propagate and when to error out. I'm not even sure you can always find the right place to stop propagating
- do you just keep going up until theset_parentcallback succeeds, or exists, or what?
For the set_parent there would be no propagating at all. For set_rate I can imagine a flag in the generic clock which tells whether to propagate set_rate or not.
Quoted 7 lines
I think you can still factor out all the common code if you treat each clock as a possible mux, divider, and gate combination. Each part of the clock is still just as abstractable as before - you can set the
rate_opsto be the generic single register divider implementation, and the gate ops to be the generic single bit enable implementation. The idea of what a clock node is matches the HW design,
The hardware design consists of only discrete rate changers (plls, dividers), muxes and gates. These are the only building blocks *every* hardware design has. I believe that many of the problems the current implementations have are due to the multiple building blocks stuffed into one clock. If you haven't already take a look at my i.MX5 clock patches:
http://thread.gmane.org/gmane.linux.ports.arm.kernel/113631
They need changes to fit onto the current patches and the rate propagation problem is not solved there, but the resulting clock data files are really short and nice to read. Furthermore it's easy to implement. Just look at the diagrams in the datasheet and go through them.
Sascha
Signature · 4 lines
Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Quoted 52 lines · Sascha Hauer
Quoted 23 lines · Colin Cross
Quoted 16 lines · Sascha Hauer
Quoted 8 lines · Colin Cross
Quoted 7 lines
? tglx's plan is to create a separate struct
clk_hwdata, which contains a ? union of base data structures for common clocks: div, mux, gate, etc. The ? ops callbacks are passed aclk_hw, plus aclk_hwdata, and most of the base ? hwdata fields are handled within the core clock code. This means less ? encapsulation of clock implementation logic, but more coverage of ? clock basics through the core code.Quoted 6 lines
I don't think they should be a union, I think there should be 3 separate private datas, and three sets of clock ops, for the three different types of clock blocks: rate changers (dividers and plls), muxes, and gates. ?These blocks are very often combined - a device clock often has a mux and a divider, and
clk_set_parentandclk_set_rateon the same struct clk both need to work.Quoted 5 lines
The idea is to being able to propagate functions to the parent. It's very convenient for the implementation of clocks when they only implement either a divider, a mux or a gate. Combining all of these into a single clock leads to complicated clock trees and many different clocks where you can't factor out the common stuff.
Quoted 8 lines
That seems complicated. ?You end up with lots of extra clocks (meaning more boilerplate in the platform files) that have no meaning in the system (what is the clock between the mux and the divider in Tegra's i2c1 clock, it can never feed any devices), and you have to figure out at the framework level when to propagate and when to error out. ?I'm not even sure you can always find the right place to stop propagating
- do you just keep going up until theset_parentcallback succeeds, or exists, or what?For the
set_parentthere would be no propagating at all. Forset_rateI can imagine a flag in the generic clock which tells whether to propagateset_rateor not.Quoted 7 lines
I think you can still factor out all the common code if you treat each clock as a possible mux, divider, and gate combination. ?Each part of the clock is still just as abstractable as before - you can set the
rate_opsto be the generic single register divider implementation, and the gate ops to be the generic single bit enable implementation. ?The idea of what a clock node is matches the HW design,The hardware design consists of only discrete rate changers (plls, dividers), muxes and gates. These are the only building blocks *every* hardware design has. I believe that many of the problems the current implementations have are due to the multiple building blocks stuffed into one clock. If you haven't already take a look at my i.MX5 clock patches:
Quoted 7 lines
http://thread.gmane.org/gmane.linux.ports.arm.kernel/113631
They need changes to fit onto the current patches and the rate propagation problem is not solved there, but the resulting clock data files are really short and nice to read. Furthermore it's easy to implement. Just look at the diagrams in the datasheet and go through them.
Propagation is what I'm trying simplify, because it's impossible at the framework level to determine the right time to propagate, and the right time to return an error, and I don't like pushing it down to each clock implementation either, because then you need a "propagating clk gate" and a "non-propagating clk gate".
Your building blocks implement every op - clk_gate_ops implements set_rate as clk_parent_set_rate (won't that cause a deadlock on prepare_lock when it calls clk_set_rate?). I'm suggesting breaking out the clock gate ops into a struct that only contains:
enable
disable
prepare
unprepare
And a rate ops struct that contains:round_rateset_rate
And a mux ops struct that containsset_parent
For a single HW clock that has a mux, a gate, and a divider, the existing implementation requires:INIT_CLK(..., clk_mux_ops)INIT_CLK(..., clk_div_ops)INIT_CLK(..., clk_gate_ops)
which creates 3 clocks, and requires lots of propagation logic to figure out how to call clk_set_rate on the single clock that is exposed to the device, but not propagate it past the device clock if the device clock doesn't have a divider (propagating it up to an active pll feeding other devices results in disaster, at least on Tegra). This combination doesn't seem to be common in your MX code, but _every_ Tegra device clock has these three parts.
With multiple smaller building blocks that can fit inside a clock, all you need is:INIT_CLK(..., clk_mux_ops, clk_div_ops, clk_gate_ops)
You have one struct clk, which is exposed to the device and matches the datasheet. If the clock has rate ops, clk_set_rate works with no propagation. If it doesn't have a rate, clk_rate_ops is NULL, and the framework deal with propagation only in the case where it is really propagation - a child clock that requires changing a parent clock. The block abstraction is still in place, there are just 3 slots for blocks within each clock.
Using a flag to mark clocks as "non-propagatable" is also not correct
- propagatability is a feature of the parent, not the child.
Quoted 81 lines · Colin Cross
Quoted 52 lines · Sascha Hauer
Quoted 23 lines · Colin Cross
Quoted 16 lines · Sascha Hauer
Quoted 8 lines · Colin Cross
Quoted 7 lines
? tglx's plan is to create a separate struct
clk_hwdata, which contains a ? union of base data structures for common clocks: div, mux, gate, etc. The ? ops callbacks are passed aclk_hw, plus aclk_hwdata, and most of the base ? hwdata fields are handled within the core clock code. This means less ? encapsulation of clock implementation logic, but more coverage of ? clock basics through the core code.Quoted 6 lines
I don't think they should be a union, I think there should be 3 separate private datas, and three sets of clock ops, for the three different types of clock blocks: rate changers (dividers and plls), muxes, and gates. ?These blocks are very often combined - a device clock often has a mux and a divider, and
clk_set_parentandclk_set_rateon the same struct clk both need to work.Quoted 5 lines
The idea is to being able to propagate functions to the parent. It's very convenient for the implementation of clocks when they only implement either a divider, a mux or a gate. Combining all of these into a single clock leads to complicated clock trees and many different clocks where you can't factor out the common stuff.
Quoted 8 lines
That seems complicated. ?You end up with lots of extra clocks (meaning more boilerplate in the platform files) that have no meaning in the system (what is the clock between the mux and the divider in Tegra's i2c1 clock, it can never feed any devices), and you have to figure out at the framework level when to propagate and when to error out. ?I'm not even sure you can always find the right place to stop propagating
- do you just keep going up until theset_parentcallback succeeds, or exists, or what?For the
set_parentthere would be no propagating at all. Forset_rateI can imagine a flag in the generic clock which tells whether to propagateset_rateor not.Quoted 7 lines
I think you can still factor out all the common code if you treat each clock as a possible mux, divider, and gate combination. ?Each part of the clock is still just as abstractable as before - you can set the
rate_opsto be the generic single register divider implementation, and the gate ops to be the generic single bit enable implementation. ?The idea of what a clock node is matches the HW design,The hardware design consists of only discrete rate changers (plls, dividers), muxes and gates. These are the only building blocks *every* hardware design has. I believe that many of the problems the current implementations have are due to the multiple building blocks stuffed into one clock. If you haven't already take a look at my i.MX5 clock patches:
Quoted 7 lines
http://thread.gmane.org/gmane.linux.ports.arm.kernel/113631
They need changes to fit onto the current patches and the rate propagation problem is not solved there, but the resulting clock data files are really short and nice to read. Furthermore it's easy to implement. Just look at the diagrams in the datasheet and go through them.
Propagation is what I'm trying simplify, because it's impossible at the framework level to determine the right time to propagate, and the right time to return an error, and I don't like pushing it down to each clock implementation either, because then you need a "propagating clk gate" and a "non-propagating clk gate".
Your building blocks implement every op -
clk_gate_opsimplementsset_rateasclk_parent_set_rate(won't that cause a deadlock onprepare_lockwhen it callsclk_set_rate?). I'm suggesting breaking out the clock gate ops into a struct that only contains:
enable
disable
prepare
unprepare
And a rate ops struct that contains:round_rateset_rate
And a mux ops struct that containsset_parent
Quoted 5 lines
For a single HW clock that has a mux, a gate, and a divider, the existing implementation requires:
INIT_CLK(..., clk_mux_ops)INIT_CLK(..., clk_div_ops)INIT_CLK(..., clk_gate_ops)
Not all clocks are like:
pll -> [mux] -> [gate] -> [divider] -> clk A
Some imx233 clocks are like:
pll -> [divider] -> -----------\
[mux] -> clk Bxtal -> [gate] -> [divider] -> /
Thanks
Richard
Quoted 27 lines
which creates 3 clocks, and requires lots of propagation logic to figure out how to call
clk_set_rateon the single clock that is exposed to the device, but not propagate it past the device clock if the device clock doesn't have a divider (propagating it up to an active pll feeding other devices results in disaster, at least on Tegra). This combination doesn't seem to be common in your MX code, but _every_ Tegra device clock has these three parts.With multiple smaller building blocks that can fit inside a clock, all you need is:
INIT_CLK(..., clk_mux_ops, clk_div_ops, clk_gate_ops)
You have one struct clk, which is exposed to the device and matches the datasheet. If the clock has rate ops,clk_set_rateworks with no propagation. If it doesn't have a rate,clk_rate_opsis NULL, and the framework deal with propagation only in the case where it is really propagation - a child clock that requires changing a parent clock. The block abstraction is still in place, there are just 3 slots for blocks within each clock.Using a flag to mark clocks as "non-propagatable" is also not correct
- propagatability is a feature of the parent, not the child._______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Quoted 61 lines · Colin Cross
Quoted 52 lines · Sascha Hauer
Quoted 23 lines · Colin Cross
Quoted 16 lines · Sascha Hauer
Quoted 8 lines · Colin Cross
Quoted 7 lines
? tglx's plan is to create a separate struct
clk_hwdata, which contains a ? union of base data structures for common clocks: div, mux, gate, etc. The ? ops callbacks are passed aclk_hw, plus aclk_hwdata, and most of the base ? hwdata fields are handled within the core clock code. This means less ? encapsulation of clock implementation logic, but more coverage of ? clock basics through the core code.Quoted 6 lines
I don't think they should be a union, I think there should be 3 separate private datas, and three sets of clock ops, for the three different types of clock blocks: rate changers (dividers and plls), muxes, and gates. ?These blocks are very often combined - a device clock often has a mux and a divider, and
clk_set_parentandclk_set_rateon the same struct clk both need to work.Quoted 5 lines
The idea is to being able to propagate functions to the parent. It's very convenient for the implementation of clocks when they only implement either a divider, a mux or a gate. Combining all of these into a single clock leads to complicated clock trees and many different clocks where you can't factor out the common stuff.
Quoted 8 lines
That seems complicated. ?You end up with lots of extra clocks (meaning more boilerplate in the platform files) that have no meaning in the system (what is the clock between the mux and the divider in Tegra's i2c1 clock, it can never feed any devices), and you have to figure out at the framework level when to propagate and when to error out. ?I'm not even sure you can always find the right place to stop propagating
- do you just keep going up until theset_parentcallback succeeds, or exists, or what?For the
set_parentthere would be no propagating at all. Forset_rateI can imagine a flag in the generic clock which tells whether to propagateset_rateor not.Quoted 7 lines
I think you can still factor out all the common code if you treat each clock as a possible mux, divider, and gate combination. ?Each part of the clock is still just as abstractable as before - you can set the
rate_opsto be the generic single register divider implementation, and the gate ops to be the generic single bit enable implementation. ?The idea of what a clock node is matches the HW design,The hardware design consists of only discrete rate changers (plls, dividers), muxes and gates. These are the only building blocks *every* hardware design has. I believe that many of the problems the current implementations have are due to the multiple building blocks stuffed into one clock. If you haven't already take a look at my i.MX5 clock patches:
Quoted 7 lines
http://thread.gmane.org/gmane.linux.ports.arm.kernel/113631
They need changes to fit onto the current patches and the rate propagation problem is not solved there, but the resulting clock data files are really short and nice to read. Furthermore it's easy to implement. Just look at the diagrams in the datasheet and go through them.
Quoted 5 lines
Propagation is what I'm trying simplify, because it's impossible at the framework level to determine the right time to propagate, and the right time to return an error, and I don't like pushing it down to each clock implementation either, because then you need a "propagating clk gate" and a "non-propagating clk gate".
Or a single flag on either the gate/mux/div or in the clock framework.
Quoted 12 lines
Your building blocks implement every op -
clk_gate_opsimplementsset_rateasclk_parent_set_rate(won't that cause a deadlock onprepare_lockwhen it callsclk_set_rate?). I'm suggesting breaking out the clock gate ops into a struct that only contains:
enable
disable
prepare
unprepare
And a rate ops struct that contains:round_rateset_rate
+ get_rate?
Quoted 15 lines
And a mux ops struct that contains
set_parentFor a single HW clock that has a mux, a gate, and a divider, the existing implementation requires:
INIT_CLK(..., clk_mux_ops)INIT_CLK(..., clk_div_ops)INIT_CLK(..., clk_gate_ops)
which creates 3 clocks, and requires lots of propagation logic to figure out how to callclk_set_rateon the single clock that is exposed to the device, but not propagate it past the device clock if the device clock doesn't have a divider (propagating it up to an active pll feeding other devices results in disaster, at least on Tegra). This combination doesn't seem to be common in your MX code, but _every_ Tegra device clock has these three parts.
Lucky you. Maybe the Tegra hardware designers have better drugs or just take less of them...
Quoted 7 lines
With multiple smaller building blocks that can fit inside a clock, all you need is:
INIT_CLK(..., clk_mux_ops, clk_div_ops, clk_gate_ops)
You have one struct clk, which is exposed to the device and matches the datasheet. If the clock has rate ops,clk_set_rateworks with no propagation.
No, we have cascaded dividers and thus still need rate propagation. Also we have the situation where a device is not allowed to adjust the divider because other clocks are derived from the same divider. Still the driver needs the rate. Here's an example from the i.MX51 (simplified):
-> PLL ----> divider -----> gate ---> MMC1
\
\
---> gate ---> MMC2Neither MMC1 nor MMC2 can be allowed to change the rate because it would change the rate of the other MMC. Both MMCs need the rate to adjust the card to the correct speed. In this case the platform code needs a possibility to adjust the rate initially to a value suitable for both MMC cores (50MHz). For this is needs access to the divider, preferably as a clock to be able to use standard clock functions.
There are also many many dividers and muxes which are standalone somewhere in the clock tree, not directly associated with any device. With a clock which can be a mux and a divider together one is tempted to combine these into the least possible number of clocks leading to some random combination of clocks.
Quoted 8 lines
If it doesn't have a rate,
clk_rate_opsis NULL, and the framework deal with propagation only in the case where it is really propagation - a child clock that requires changing a parent clock. The block abstraction is still in place, there are just 3 slots for blocks within each clock.Using a flag to mark clocks as "non-propagatable" is also not correct
- propagatability is a feature of the parent, not the child.
this flag could also be added to the parent if we wish to.
Sascha
Signature · 4 lines
Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Quoted 5 lines · Colin Cross
With multiple smaller building blocks that can fit inside a clock, all you need is:
INIT_CLK(..., clk_mux_ops, clk_div_ops, clk_gate_ops)
You have one struct clk, which is exposed to the device and matches the datasheet. If the clock has rate ops,clk_set_rateworks with no
It matches what the datasheet shows you, but it's still separate building blocks in silicon. Creating uberclocks will fall flat in no time.
Simply expand your example to:
mux -> divider1 -> gate1
|
---divider2--> gate2That's not doable with a combo clock and you end up with a mux and two combos of divider/gates.
Having a straight simple building blocks view is way more easier to handle and allows arbitrary combinations and ordering at the tree level instead of moving the ordering decision into the hardware implementation level. We want the lowlevel ops implementation to be as simple and stupid as it gets and want to have as much complexity as necessary at the core code level.
And it's not that complex. We can propagate set_rate from gateN down to the divider and it's not rocket science to make the divider consult the mux for a better suiting frequency, which implies that the mux decision checks and verifies the resulting damage for the other child. And this wants to be done at the core level not in the low level implementations.
Thanks,
tglx
Quoted 4 lines · Colin Cross
figure out how to call
clk_set_rateon the single clock that is exposed to the device, but not propagate it past the device clock if the device clock doesn't have a divider (propagating it up to an active pll feeding other devices results in disaster, at least on
This is usually a function of how shared the thing being changed is rather than the specific implementation, though PLLs and FLLs are obviously problematic due to the lock time (if they don't have glitchless handover logic).
Quoted 2 lines
Tegra). This combination doesn't seem to be common in your MX code, but _every_ Tegra device clock has these three parts.
That's Tegra. Other devices will have other structures.
Quoted 65 lines · Colin Cross
Quoted 31 lines · Jeremy Kerr
[This series was originally titled 'Add a common struct clk', but the goals have changed since that first set of patches. We're now aiming for a more complete generic clock infrastructure, rather than just abstracting struct clk]
[This series still needs work, see the TODO section below]
[Totally RFC at the moment]
Hi all,
These patches are an attempt to allow platforms to share clock code. At present, the definitions of 'struct clk' are local to platform code, which makes allocating and initialising cross-platform clock sources difficult, and makes it impossible to compile a single image containing support for two ARM platforms with different struct clks.
The three patches are for the architecture-independent kernel code, introducing the common clk infrastructure. The changelog for the first patch includes details about the new clock definitions.
The second patch implements
clk_set_rate, and in doing so adds functionality to walk the clock tree in both directions.
clk_set_parentis left unimplemented, see TODO below for why.The third and fourth patches provide some basic clocks (fixed-rate and gated), mostly to serve as an example of the hardware implementation. I'm intending to later provide similar base clocks for mux and divider hardware clocks.
Quoted 14 lines
Many thanks to the following for their input:
?* Benjamin Herrenschmidt <[email protected]> ?* Thomas Gleixner <[email protected]>
?* Ben Dooks <[email protected]>
?* Baruch Siach <[email protected]>
?* Russell King <[email protected]>
?* Uwe Kleine-K?nig <[email protected]> ?* Lorenzo Pieralisi <[email protected]> ?* Vincent Guittot <[email protected]> ?* Sascha Hauer <[email protected]>
?* Ryan Mallon <[email protected]>
?* Colin Cross <[email protected]>
?* Jassi Brar <[email protected]>
?* Saravana Kannan <[email protected]>I have a similar set of patches I was working on that handles a little more of the common code than these. I can post them if you want, but for now I'll just point out where I had different ideas, and not muddy the waters with conflicting patches.
Quoted 7 lines
TODO:
?* Need to figure out the locking around
clk_set_parent. Changing the in-kernel ? clock topology requires acquiring both the mutex (to prevent against races ? withclk_prepare, which may propagate to the parent clock) and the spinlock ? (to prevent the same race withclk_enable).Quoted 5 lines
? However, we should also be changing the hardware clock topology in sync with ? the in-kernel clock topology, which would imply that both locks *also* need ? to be held while updating the parent in the hardware (ie, in
?clk_hw_ops->set_parent) too. ?However, I believe some platform clock ? implementations may require this callback to be able to sleep. Comments?
Quoted 3 lines
This sequence is the best I could come up with without adding a temporary 2nd parent:
1. lock prepare mutex
Maybe tell child clocks "I'm going to change clock rate, please stop work if needed"
Quoted 6 lines
2. call prepare on the new parent if
prepare_count> 0
3. lock the enable spinlock
4. call enable on the new parent ifenable_count> 0
5. change the parent pointer to the new parent
6. unlock the spinlock
7. call theset_parentcallback
Why do it need to sleep if it only set some hw registers?
Quoted 5 lines
8. lock the enable spinlock
9. call disable on the old parent iff you called enable on the new parent (enable_countmay have changed)
10. unlock the enable spinlock
11. call unprepare on the old parent ifprepare_count
propagate rate here and also tell child clocks "rate changed already, change your parameters and go on to work".
Quoted 9 lines
12. unlock prepare mutex
The only possible problem I see is that if a clock starts disabled at step 1., and
clk_enableis called on it between steps 6 and 7,clk_enablewill return having enabled the new parent, but the clock is still running off the old parent. As soon as the clock gets switched to the new parent, the clock will be properly enabled. I don't see this as a huge problem - callingclk_set_parenton a clock while it is enabled may not even work without causing glitches on some platforms.
some do be glitch free, especially for cpu clock parents.
Quoted 21 lines
I guess the only way around it would be to store a temporary "
new_parent" pointer between steps 5 and 9, and have clk_enable/disable operate on both the current parent and the new parent. They would also need to refcount the extra enables separately to undo on the old parent.Quoted 7 lines
?* tglx and I have been discussing different ways of passing clock information ? to the clock hardware implementation. I'm proposing providing a base 'struct ?
clk_hw', which implementations subclass by wrapping in their own structure ? (or one of a set of simple 'library' structures, likeclk_hw_muxor ?clk_hw_gate). ?Theclk_hwbase is passed as the first argument to all ?clk_hw_opscallbacks.Quoted 6 lines
? tglx's plan is to create a separate struct
clk_hwdata, which contains a ? union of base data structures for common clocks: div, mux, gate, etc. The ? ops callbacks are passed aclk_hw, plus aclk_hwdata, and most of the base ? hwdata fields are handled within the core clock code. This means less ? encapsulation of clock implementation logic, but more coverage of ? clock basics through the core code.
Quoted 12 lines
I don't think they should be a union, I think there should be 3 separate private datas, and three sets of clock ops, for the three different types of clock blocks: rate changers (dividers and plls), muxes, and gates. These blocks are very often combined - a device clock often has a mux and a divider, and
clk_set_parentandclk_set_rateon the same struct clk both need to work.Quoted 5 lines
? tglx, could you send some details about your approach? I'm aiming to refine ? this series with the good bits from each technique.
?* The clock registration code probably needs work. This is the domain ? of the platform/board maintainers, any wishes here?
When clock init, data in struct clk may not be synced with hw registers. After enabled minimal needed clk (cpu, core bus etc), we need sync the whole tree, disable zero enable_count clocks, set correct .rate ... . The sync function is also common code, right? but not have to be called all times I think.
Thanks
Richard
Quoted 26 lines
Quoted 15 lines
Cheers,
Jeremy
--
---
Jeremy Kerr (4):
? ? ?clk: Add a generic clock infrastructure
? ? ?clk: Implementclk_set_rate
? ? ?clk: Add fixed-rate clock
? ? ?clk: Add simple gated clockQuoted 6 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo at vger.kernel.org
More majordomo info at ?http://vger.kernel.org/majordomo-info.html Please read the FAQ at ?http://www.tux.org/lkml/_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Quoted 69 lines · Richard Zhao
Quoted 65 lines · Colin Cross
Quoted 31 lines · Jeremy Kerr
[This series was originally titled 'Add a common struct clk', but the goals have changed since that first set of patches. We're now aiming for a more complete generic clock infrastructure, rather than just abstracting struct clk]
[This series still needs work, see the TODO section below]
[Totally RFC at the moment]
Hi all,
These patches are an attempt to allow platforms to share clock code. At present, the definitions of 'struct clk' are local to platform code, which makes allocating and initialising cross-platform clock sources difficult, and makes it impossible to compile a single image containing support for two ARM platforms with different struct clks.
The three patches are for the architecture-independent kernel code, introducing the common clk infrastructure. The changelog for the first patch includes details about the new clock definitions.
The second patch implements
clk_set_rate, and in doing so adds functionality to walk the clock tree in both directions.
clk_set_parentis left unimplemented, see TODO below for why.The third and fourth patches provide some basic clocks (fixed-rate and gated), mostly to serve as an example of the hardware implementation. I'm intending to later provide similar base clocks for mux and divider hardware clocks.
Quoted 14 lines
Many thanks to the following for their input:
?* Benjamin Herrenschmidt <[email protected]> ?* Thomas Gleixner <[email protected]>
?* Ben Dooks <[email protected]>
?* Baruch Siach <[email protected]>
?* Russell King <[email protected]>
?* Uwe Kleine-K?nig <[email protected]> ?* Lorenzo Pieralisi <[email protected]> ?* Vincent Guittot <[email protected]> ?* Sascha Hauer <[email protected]>
?* Ryan Mallon <[email protected]>
?* Colin Cross <[email protected]>
?* Jassi Brar <[email protected]>
?* Saravana Kannan <[email protected]>I have a similar set of patches I was working on that handles a little more of the common code than these. ?I can post them if you want, but for now I'll just point out where I had different ideas, and not muddy the waters with conflicting patches.
Quoted 7 lines
TODO:
?* Need to figure out the locking around
clk_set_parent. Changing the in-kernel ? clock topology requires acquiring both the mutex (to prevent against races ? withclk_prepare, which may propagate to the parent clock) and the spinlock ? (to prevent the same race withclk_enable).Quoted 5 lines
? However, we should also be changing the hardware clock topology in sync with ? the in-kernel clock topology, which would imply that both locks *also* need ? to be held while updating the parent in the hardware (ie, in
?clk_hw_ops->set_parent) too. ?However, I believe some platform clock ? implementations may require this callback to be able to sleep. Comments?Quoted 3 lines
This sequence is the best I could come up with without adding a temporary 2nd parent:
1. lock prepare mutex
Quoted 8 lines
Maybe tell child clocks "I'm going to change clock rate, please stop work if needed"
Quoted 6 lines
2. call prepare on the new parent if
prepare_count> 0
3. lock the enable spinlock
4. call enable on the new parent ifenable_count> 0
5. change the parent pointer to the new parent
6. unlock the spinlock
7. call theset_parentcallbackWhy do it need to sleep if it only set some hw registers?
Most implementations don't, and I would be fine with saying clk_set_parent sleeps, but the set_parent op does not, but that prevents clock chips on sleeping busses like i2c.
Quoted 7 lines
Quoted 5 lines
8. lock the enable spinlock
9. call disable on the old parent iff you called enable on the new parent (enable_countmay have changed)
10. unlock the enable spinlock
11. call unprepare on the old parent ifprepare_countpropagate rate here and also tell child clocks "rate changed already, change your parameters and go on to work".
Yes, propagate rate is needed.
Quoted 41 lines
Quoted 9 lines
12. unlock prepare mutex
The only possible problem I see is that if a clock starts disabled at step 1., and
clk_enableis called on it between steps 6 and 7,clk_enablewill return having enabled the new parent, but the clock is still running off the old parent. ?As soon as the clock gets switched to the new parent, the clock will be properly enabled. ?I don't see this as a huge problem - callingclk_set_parenton a clock while it is enabled may not even work without causing glitches on some platforms.some do be glitch free, especially for cpu clock parents.
Quoted 31 lines
I guess the only way around it would be to store a temporary "
new_parent" pointer between steps 5 and 9, and have clk_enable/disable operate on both the current parent and the new parent. ?They would also need to refcount the extra enables separately to undo on the old parent.Quoted 7 lines
?* tglx and I have been discussing different ways of passing clock information ? to the clock hardware implementation. I'm proposing providing a base 'struct ?
clk_hw', which implementations subclass by wrapping in their own structure ? (or one of a set of simple 'library' structures, likeclk_hw_muxor ?clk_hw_gate). ?Theclk_hwbase is passed as the first argument to all ?clk_hw_opscallbacks.Quoted 6 lines
? tglx's plan is to create a separate struct
clk_hwdata, which contains a ? union of base data structures for common clocks: div, mux, gate, etc. The ? ops callbacks are passed aclk_hw, plus aclk_hwdata, and most of the base ? hwdata fields are handled within the core clock code. This means less ? encapsulation of clock implementation logic, but more coverage of ? clock basics through the core code.I don't think they should be a union, I think there should be 3 separate private datas, and three sets of clock ops, for the three different types of clock blocks: rate changers (dividers and plls), muxes, and gates. ?These blocks are very often combined - a device clock often has a mux and a divider, and
clk_set_parentandclk_set_rateon the same struct clk both need to work.Quoted 3 lines
? tglx, could you send some details about your approach? I'm aiming to refine ? this series with the good bits from each technique.
Quoted 6 lines
Quoted 2 lines
Quoted 2 lines
?* The clock registration code probably needs work. This is the domain ? of the platform/board maintainers, any wishes here?
When clock init, data in struct clk may not be synced with hw registers. After enabled minimal needed clk (cpu, core bus etc), we need sync the whole tree, disable zero
enable_countclocks, set correct .rate ... . The sync function is also common code, right? but not have to be called all times I think.
I believe each clock is synced with its hardware during clk_register by calling the recalc_rate and get_parent callbacks.
Quoted 29 lines
Thanks
RichardQuoted 26 lines
Quoted 15 lines
Cheers,
Jeremy
--
---
Jeremy Kerr (4):
? ? ?clk: Add a generic clock infrastructure
? ? ?clk: Implementclk_set_rate
? ? ?clk: Add fixed-rate clock
? ? ?clk: Add simple gated clockQuoted 6 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo at vger.kernel.org
More majordomo info at ?http://vger.kernel.org/majordomo-info.html Please read the FAQ at ?http://www.tux.org/lkml/_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Quoted 77 lines · Colin Cross
Quoted 76 lines · Richard Zhao
Quoted 65 lines · Colin Cross
Quoted 31 lines · Jeremy Kerr
[This series was originally titled 'Add a common struct clk', but the goals have changed since that first set of patches. We're now aiming for a more complete generic clock infrastructure, rather than just abstracting struct clk]
[This series still needs work, see the TODO section below]
[Totally RFC at the moment]
Hi all,
These patches are an attempt to allow platforms to share clock code. At present, the definitions of 'struct clk' are local to platform code, which makes allocating and initialising cross-platform clock sources difficult, and makes it impossible to compile a single image containing support for two ARM platforms with different struct clks.
The three patches are for the architecture-independent kernel code, introducing the common clk infrastructure. The changelog for the first patch includes details about the new clock definitions.
The second patch implements
clk_set_rate, and in doing so adds functionality to walk the clock tree in both directions.
clk_set_parentis left unimplemented, see TODO below for why.The third and fourth patches provide some basic clocks (fixed-rate and gated), mostly to serve as an example of the hardware implementation. I'm intending to later provide similar base clocks for mux and divider hardware clocks.
Quoted 14 lines
Many thanks to the following for their input:
?* Benjamin Herrenschmidt <[email protected]> ?* Thomas Gleixner <[email protected]>
?* Ben Dooks <[email protected]>
?* Baruch Siach <[email protected]>
?* Russell King <[email protected]>
?* Uwe Kleine-K?nig <[email protected]> ?* Lorenzo Pieralisi <[email protected]> ?* Vincent Guittot <[email protected]> ?* Sascha Hauer <[email protected]>
?* Ryan Mallon <[email protected]>
?* Colin Cross <[email protected]>
?* Jassi Brar <[email protected]>
?* Saravana Kannan <[email protected]>I have a similar set of patches I was working on that handles a little more of the common code than these. ?I can post them if you want, but for now I'll just point out where I had different ideas, and not muddy the waters with conflicting patches.
Quoted 7 lines
TODO:
?* Need to figure out the locking around
clk_set_parent. Changing the in-kernel ? clock topology requires acquiring both the mutex (to prevent against races ? withclk_prepare, which may propagate to the parent clock) and the spinlock ? (to prevent the same race withclk_enable).Quoted 5 lines
? However, we should also be changing the hardware clock topology in sync with ? the in-kernel clock topology, which would imply that both locks *also* need ? to be held while updating the parent in the hardware (ie, in
?clk_hw_ops->set_parent) too. ?However, I believe some platform clock ? implementations may require this callback to be able to sleep. Comments?Quoted 3 lines
This sequence is the best I could come up with without adding a temporary 2nd parent:
1. lock prepare mutexMaybe tell child clocks "I'm going to change clock rate, please stop work if needed"
Quoted 6 lines
2. call prepare on the new parent if
prepare_count> 0
3. lock the enable spinlock
4. call enable on the new parent ifenable_count> 0
5. change the parent pointer to the new parent
6. unlock the spinlock
7. call theset_parentcallback
Quoted 5 lines
Quoted 1 line
Why do it need to sleep if it only set some hw registers?
Most implementations don't, and I would be fine with saying
clk_set_parentsleeps, but theset_parentop does not, but that prevents clock chips on sleeping busses like i2c.
So it worth to involve a flag here, which says hw_ops may sleep. At least for on-SoC clocks, we don't need to take risk.
Quoted 54 lines
Quoted 8 lines
Quoted 6 lines
8. lock the enable spinlock
9. call disable on the old parent iff you called enable on the new parent (enable_countmay have changed)
10. unlock the enable spinlock
11. call unprepare on the old parent ifprepare_countpropagate rate here and also tell child clocks "rate changed already, change your parameters and go on to work".
Yes, propagate rate is needed.
Quoted 43 lines
Quoted 9 lines
12. unlock prepare mutex
The only possible problem I see is that if a clock starts disabled at step 1., and
clk_enableis called on it between steps 6 and 7,clk_enablewill return having enabled the new parent, but the clock is still running off the old parent. ?As soon as the clock gets switched to the new parent, the clock will be properly enabled. ?I don't see this as a huge problem - callingclk_set_parenton a clock while it is enabled may not even work without causing glitches on some platforms.some do be glitch free, especially for cpu clock parents.
Quoted 33 lines
I guess the only way around it would be to store a temporary "
new_parent" pointer between steps 5 and 9, and have clk_enable/disable operate on both the current parent and the new parent. ?They would also need to refcount the extra enables separately to undo on the old parent.Quoted 7 lines
?* tglx and I have been discussing different ways of passing clock information ? to the clock hardware implementation. I'm proposing providing a base 'struct ?
clk_hw', which implementations subclass by wrapping in their own structure ? (or one of a set of simple 'library' structures, likeclk_hw_muxor ?clk_hw_gate). ?Theclk_hwbase is passed as the first argument to all ?clk_hw_opscallbacks.Quoted 6 lines
? tglx's plan is to create a separate struct
clk_hwdata, which contains a ? union of base data structures for common clocks: div, mux, gate, etc. The ? ops callbacks are passed aclk_hw, plus aclk_hwdata, and most of the base ? hwdata fields are handled within the core clock code. This means less ? encapsulation of clock implementation logic, but more coverage of ? clock basics through the core code.I don't think they should be a union, I think there should be 3 separate private datas, and three sets of clock ops, for the three different types of clock blocks: rate changers (dividers and plls), muxes, and gates. ?These blocks are very often combined - a device clock often has a mux and a divider, and
clk_set_parentandclk_set_rateon the same struct clk both need to work.Quoted 5 lines
? tglx, could you send some details about your approach? I'm aiming to refine ? this series with the good bits from each technique.
?* The clock registration code probably needs work. This is the domain ? of the platform/board maintainers, any wishes here?
Quoted 7 lines
Quoted 4 lines
When clock init, data in struct clk may not be synced with hw registers. After enabled minimal needed clk (cpu, core bus etc), we need sync the whole tree, disable zero
enable_countclocks, set correct .rate ... . The sync function is also common code, right? but not have to be called all times I think.I believe each clock is synced with its hardware during
clk_registerby calling therecalc_rateandget_parentcallbacks.
so how to sync gate bits? Let subarch to set registers directly or clk_enable and clk_disable to make sure clk is disabled? Neither way is good I think.
Thanks
Richard
Quoted 36 lines
Quoted 30 lines
Thanks
RichardQuoted 26 lines
Quoted 15 lines
Cheers,
Jeremy
--
---
Jeremy Kerr (4):
? ? ?clk: Add a generic clock infrastructure
? ? ?clk: Implementclk_set_rate
? ? ?clk: Add fixed-rate clock
? ? ?clk: Add simple gated clockQuoted 6 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo at vger.kernel.org
More majordomo info at ?http://vger.kernel.org/majordomo-info.html Please read the FAQ at ?http://www.tux.org/lkml/_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Quoted 6 lines · On Friday, May 20, 2011 03:27:49 Jeremy Kerr wrote:
[This series was originally titled 'Add a common struct clk', but the goals have changed since that first set of patches. We're now aiming for a more complete generic clock infrastructure, rather than just abstracting struct clk]
[This series still needs work, see the TODO section below]
for future series, could you cc uclinux-dist-devel at blackfin.uclinux.org ? we dont do clock management on Blackfin parts atm, but it's something we would like to start doing. our hardware can easily benefit from this.
basically, we have:
xtal -> pll -> divider -> core clock -> divider -> core timer
\-> divider -> system clock |-> spi divider -> spi clock |-> i2c divider -> i2c clock |-> uart divider -> uart clock ...etc...
and future parts are only going to get more complicated trees.
atm we only support changing core clock on the fly since it requires
reprogramming one device (the core timer). we'd like to support changing the
other clocks on the fly, but have no framework for doing so. this obviously
would address that.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110530/ef7cabf9/attachment.sig>
Quoted 6 lines · Jeremy Kerr
[This series was originally titled 'Add a common struct clk', but the goals have changed since that first set of patches. We're now aiming for a more complete generic clock infrastructure, rather than just abstracting struct clk]
[This series still needs work, see the TODO section below]
Is there a git tree with the current state of this code in it somewhere? It's not mentioned here and my google skills are failing me.
Quoted 5 lines · Mark Brown
Quoted 4 lines · Jeremy Kerr
[This series was originally titled 'Add a common struct clk', but the goals have changed since that first set of patches. We're now aiming for a more complete generic clock infrastructure, rather than just abstracting struct clk]
... which will mean the platforms at the simpler end of the spectrum will ignore it because it'll be far too heavy weight for them.
If we're going to do something generic with it, then we need something that _can_ scale from damned simple right up to OMAP-type complex. We don't want to start with something OMAP-type complex and have everyone use it even for just one or two (or even 30 simple enable/disable non-tree like) clocks. And those platforms should not have to have several K of unused code in their kernel because of it.
Otherwise, we'll just end up going back to people trying to directly access clock mask registers from drivers.
Note that because of the develtech.com fuckup, I don't have your patch series because I had to drop off the lists for about six hours to stop the deluge of emails.
Quoted 17 lines · Russell King - ARM Linux
Quoted 5 lines · Mark Brown
Quoted 4 lines · Jeremy Kerr
[This series was originally titled 'Add a common struct clk', but the goals have changed since that first set of patches. We're now aiming for a more complete generic clock infrastructure, rather than just abstracting struct clk]
... which will mean the platforms at the simpler end of the spectrum will ignore it because it'll be far too heavy weight for them.
If we're going to do something generic with it, then we need something that _can_ scale from damned simple right up to OMAP-type complex. We don't want to start with something OMAP-type complex and have everyone use it even for just one or two (or even 30 simple enable/disable non-tree like) clocks. And those platforms should not have to have several K of unused code in their kernel because of it.
Quoted 2 lines
Otherwise, we'll just end up going back to people trying to directly access clock mask registers from drivers.
And... btw... I'm getting the impression that clk API stuff is being done behind my back, with decisions on the direction being taken in closed room scenarios - maybe because I objected fairly strongly to the previous set of patches.
I hope I'm wrong, but that's the impression I now have, based upon where the discussion got to last time around and the quoted part of Jeremy's mail (which is all I have to go on.)
Quoted 3 lines · Russell King - ARM Linux
Note that because of the develtech.com fuckup, I don't have your patch series because I had to drop off the lists for about six hours to stop the deluge of emails.
Sorry, in case I've caused confusion here I'm replying to an old posting from May - there's not been any new versions of the code posted that I'm aware of. I was asking partly because I didn't want to start taking a more detailed look at things with code that was stale.
Quoted 10 lines · Mark Brown
Quoted 3 lines · Russell King - ARM Linux
Note that because of the develtech.com fuckup, I don't have your patch series because I had to drop off the lists for about six hours to stop the deluge of emails.
Sorry, in case I've caused confusion here I'm replying to an old posting from May - there's not been any new versions of the code posted that I'm aware of. I was asking partly because I didn't want to start taking a more detailed look at things with code that was stale.
Oh, that's the develtech.com M$ Exchange problem getting us yet again then.
Hi Mark,
Quoted 2 lines
Is there a git tree with the current state of this code in it somewhere? It's not mentioned here and my google skills are failing me.
No, these are just RFC patches at the moment, so I'm reluctant to encourage anyone basing other work on them for now. Regardless, they're the most recent versions, so would be the most suitable place to start hacking if you're interested.
On a related note, I've not had sufficient time to dedicate to the clk work recently, so Linus Walleij will be taking over the clk patches, and he'll be able to spend the necessary time to get things into shape :)
He's on vacation at the moment, but will start at the upcoming Linaro event in Cambridge, I believe.
Cheers,
Jeremy
Quoted 3 lines · Jeremy Kerr
On a related note, I've not had sufficient time to dedicate to the clk work recently, so Linus Walleij will be taking over the clk patches, and he'll be able to spend the necessary time to get things into shape :)
Oh, that's bad timing given that I just sent you a small stack of patches :/
Quoted 2 lines
He's on vacation at the moment, but will start at the upcoming Linaro event in Cambridge, I believe.
No idea when that is, and given that I'm not involved in Linaro I'm unlikely to attend.