Message ID | 1316730422-20027-2-git-send-email-mturquette@ti.com |
---|---|
State | New |
Headers | show |
On Thu, Sep 22, 2011 at 03:26:56PM -0700, Mike Turquette wrote: > From: Jeremy Kerr <jeremy.kerr@canonical.com> > > 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); Shouldn't this be: clk_register(&clk_foo_ops, &my_clk_foo->clk, NULL); ? Also, this documentation would be good to have in the Documentation directory instead of lost in a commit header. Otherwise looks okay to me. Reviewed-by: Grant Likely <grant.likely@secretlab.ca>
On Sat, Sep 24, 2011 at 8:55 PM, Grant Likely <grant.likely@secretlab.ca> wrote: > On Thu, Sep 22, 2011 at 03:26:56PM -0700, Mike Turquette wrote: >> From: Jeremy Kerr <jeremy.kerr@canonical.com> >> >> 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); > > Shouldn't this be: > > clk_register(&clk_foo_ops, &my_clk_foo->clk, NULL); > > ? > > Also, this documentation would be good to have in the Documentation > directory instead of lost in a commit header. Thanks for your review Grant. Will fix the changelog and add proper Documentation/ in the next round. Regards, Mike > Otherwise looks okay to me. > > Reviewed-by: Grant Likely <grant.likely@secretlab.ca> > >
On 09/22/2011 05:26 PM, Mike Turquette wrote: > From: Jeremy Kerr <jeremy.kerr@canonical.com> > > 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 <tglx@linutronix.de>. > > The common clock definitions are based on a development patch from Ben > Herrenschmidt <benh@kernel.crashing.org>. > > TODO: > > * We don't keep any internal reference to the clock topology at present. > > Signed-off-by: Jeremy Kerr <jeremy.kerr@canonical.com> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com> > Signed-off-by: Mike Turquette <mturquette@ti.com> > --- > Changes since v1: > Create a dummy clk_unregister and prototype/document it and clk_register > Constify struct clk_hw_ops > Remove spinlock.h header, include kernel.h > Use EOPNOTSUPP instead of ENOTSUPP > Add might_sleep to clk_prepare/clk_unprepare stubs > Properly init children hlist and child_node > Whitespace and typo fixes > > drivers/clk/Kconfig | 3 + > drivers/clk/Makefile | 1 + > drivers/clk/clk.c | 232 ++++++++++++++++++++++++++++++++++++++++++++++++++ > drivers/clk/clkdev.c | 7 ++ > include/linux/clk.h | 140 +++++++++++++++++++++++++++--- > 5 files changed, 371 insertions(+), 12 deletions(-) > create mode 100644 drivers/clk/clk.c > > diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig > index 3530927..c53ed59 100644 > --- a/drivers/clk/Kconfig > +++ b/drivers/clk/Kconfig > @@ -5,3 +5,6 @@ config CLKDEV_LOOKUP > > config HAVE_MACH_CLKDEV > bool > + > +config GENERIC_CLK > + bool > diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile > index 07613fa..570d5b9 100644 > --- a/drivers/clk/Makefile > +++ b/drivers/clk/Makefile > @@ -1,2 +1,3 @@ > > obj-$(CONFIG_CLKDEV_LOOKUP) += clkdev.o > +obj-$(CONFIG_GENERIC_CLK) += clk.o > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > new file mode 100644 > index 0000000..1cd7315 > --- /dev/null > +++ b/drivers/clk/clk.c > @@ -0,0 +1,232 @@ > +/* > + * Copyright (C) 2010-2011 Canonical Ltd <jeremy.kerr@canonical.com> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + * > + * Standard functionality for the common clock API. > + */ > + > +#include <linux/clk.h> > +#include <linux/module.h> > +#include <linux/mutex.h> > +#include <linux/slab.h> > +#include <linux/spinlock.h> > + > +struct clk { > + const char *name; > + const struct clk_hw_ops *ops; > + struct clk_hw *hw; > + 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) > +{ > + 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); > + 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(const 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; > + > + INIT_HLIST_HEAD(&clk->children); > + INIT_HLIST_NODE(&clk->child_node); > + > + 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); I don't think this is going to work. This implies that the parent clock is already registered. For simple clk trees, that's probably not an issue, but for chips with lots of muxing it will be impossible to get the order correct for all cases. This is not an issue today as most clocks are statically created. I think what is needed is a 2 stage init. The 1st stage to create all the clocks and a 2nd stage to build the tree once all clocks are created. Tracking the parents using struct clk_hw instead would help as long as clocks are still statically allocated. However, that won't help for devicetree. Rob
On Mon, Oct 03, 2011 at 09:17:30AM -0500, Rob Herring wrote: > On 09/22/2011 05:26 PM, Mike Turquette wrote: A lot of stuff that should really have been cut plus... > > + 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); > I don't think this is going to work. This implies that the parent clock > is already registered. For simple clk trees, that's probably not an > issue, but for chips with lots of muxing it will be impossible to get > the order correct for all cases. This is not an issue today as most > clocks are statically created. > I think what is needed is a 2 stage init. The 1st stage to create all > the clocks and a 2nd stage to build the tree once all clocks are created. > Tracking the parents using struct clk_hw instead would help as long as > clocks are still statically allocated. However, that won't help for > devicetree. This isn't in any way specific to clocks, right now the likely solution looks to be Grant's changes for retrying probe() as new devices come on line. With that devices can return a code from their probe() which tells the driver core that they couldn't get all the resources they need and that it should retry the probe() if more devices come on-line.
On 10/03/2011 09:25 AM, Mark Brown wrote: > On Mon, Oct 03, 2011 at 09:17:30AM -0500, Rob Herring wrote: >> On 09/22/2011 05:26 PM, Mike Turquette wrote: > > A lot of stuff that should really have been cut plus... > >>> + 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); > >> I don't think this is going to work. This implies that the parent clock >> is already registered. For simple clk trees, that's probably not an >> issue, but for chips with lots of muxing it will be impossible to get >> the order correct for all cases. This is not an issue today as most >> clocks are statically created. > >> I think what is needed is a 2 stage init. The 1st stage to create all >> the clocks and a 2nd stage to build the tree once all clocks are created. > >> Tracking the parents using struct clk_hw instead would help as long as >> clocks are still statically allocated. However, that won't help for >> devicetree. > > This isn't in any way specific to clocks, right now the likely solution > looks to be Grant's changes for retrying probe() as new devices come on > line. With that devices can return a code from their probe() which > tells the driver core that they couldn't get all the resources they need > and that it should retry the probe() if more devices come on-line. Except SOC clocks are initialized very early before timers are up and there can be a very high number of dependencies (every clock except fixed clocks). With the driver probe retry, retrying is the exception, not the rule. Retrying would require every caller to maintain a list of clks to retry. With 2 stages, you can move that into the core clock code. There are not typically a large number of board-level/driver created clocks, so ensuring correct register order is not really a problem. In cases where there is a cross-driver dependency, the probe retry is a good solution. Rob
On Mon, Oct 03, 2011 at 10:24:52AM -0500, Rob Herring wrote: > On 10/03/2011 09:25 AM, Mark Brown wrote: > > This isn't in any way specific to clocks, right now the likely solution > > looks to be Grant's changes for retrying probe() as new devices come on > > line. With that devices can return a code from their probe() which > > tells the driver core that they couldn't get all the resources they need > > and that it should retry the probe() if more devices come on-line. > Except SOC clocks are initialized very early before timers are up and > there can be a very high number of dependencies (every clock except > fixed clocks). With the driver probe retry, retrying is the exception, > not the rule. > Retrying would require every caller to maintain a list of clks to > retry. With 2 stages, you can move that into the core clock code. They don't need to maintain a list of clocks to retry, they need to unwind when probe() fails. But yes. > There are not typically a large number of board-level/driver created > clocks, so ensuring correct register order is not really a problem. In > cases where there is a cross-driver dependency, the probe retry is a > good solution. I dunno, I get the impression that some of this is due to the current limitations of the clock API rather than due to a lack of clocks - perhaps that's specific to the applications I look at, though. applications
On Mon, Oct 03, 2011 at 05:31:08PM +0100, Mark Brown wrote: > I dunno, I get the impression that some of this is due to the current > limitations of the clock API rather than due to a lack of clocks - > perhaps that's specific to the applications I look at, though. > applications The clk API per-se has nothing to do with how clocks are registered. There are two things that are the clk API: 1. clkdev - which deals with translating devices + connection IDs to struct clk. This has no ordering requirements wrt requiring parents to be "initialized" before their children (it has no care about that at all because it's not within its definition.) For this, registration is about connecting device + connection IDs to a struct clk. 2. the driver API, defining how the opaque struct clk is looked up, obtained and then manipulated. This has no 'registration' stuff. So, whether clocks are a tree or flat is unspecified. It's unspecified whether there's any particular order required. In fact, with a clock tree, it's entirely possible that only the leaf clocks will be 'registered' with clkdev. How the rest of the clock tree is initialized is beyond the scope of the driver clk API.
Mike, On 09/22/2011 05:26 PM, Mike Turquette wrote: > diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c > index 6db161f..e2a9719 100644 > --- a/drivers/clk/clkdev.c > +++ b/drivers/clk/clkdev.c > @@ -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 > + This is dead code left from prior versions. Rob
On Mon, Oct 3, 2011 at 3:02 PM, Rob Herring <robherring2@gmail.com> wrote: > Mike, > > On 09/22/2011 05:26 PM, Mike Turquette wrote: > >> diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c >> index 6db161f..e2a9719 100644 >> --- a/drivers/clk/clkdev.c >> +++ b/drivers/clk/clkdev.c >> @@ -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 >> + > > This is dead code left from prior versions. Indeed it is. Will pull it out for V3. Thanks, Mike > Rob >
On Mon, Oct 03, 2011 at 09:17:30AM -0500, Rob Herring wrote: > On 09/22/2011 05:26 PM, Mike Turquette wrote: > > + /* 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); > > I don't think this is going to work. This implies that the parent clock > is already registered. For simple clk trees, that's probably not an > issue, but for chips with lots of muxing it will be impossible to get > the order correct for all cases. This is not an issue today as most > clocks are statically created. > > I think what is needed is a 2 stage init. The 1st stage to create all > the clocks and a 2nd stage to build the tree once all clocks are created. > > Tracking the parents using struct clk_hw instead would help as long as > clocks are still statically allocated. However, that won't help for > devicetree. I disagree. Clocks really need to be registered in dependency order. Even in the deferral case, the driver should hold of actually registering the clk (note: the struct clk, not the struct device) until the clocks it depends on are available. I also agree with the point that there are a lot of SoC clocks that may not even show up in clkdev, and for pragmatic considerations are better set up all at once early in the init process (ie. as part of common SoC setup code, and doesn't change between boards). That code should be clue-full enough that it can register its own clocks in the right order. g.
On 09/22/2011 03:26 PM, Mike Turquette wrote: > diff --git a/include/linux/clk.h b/include/linux/clk.h > index 1d37f42..d6ae10b 100644 > --- a/include/linux/clk.h > +++ b/include/linux/clk.h > +#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 *); > +}; I would like to understand the need for recalc rate if that's something that we want to go into the common framework (even if it's optional). I have mostly heard only second hand explanations of the need for recalc_rate(), so I might not have the full picture. But for all the cases that I can think of, recalc_rate seems like a paradox. If recalc_rate() is used to make sure the "current rate" of a "clock A" is always known even if it's parent "clock B"'s rate is changed, then it also means that the rate of "clock A" can change without clk_set_rate(clock A, new rate). That in turn means that the clk_get_rate() just gives the instantaneous snapshot of the rate. So, any use of clk_get_rate(clock A) for anything other than printing/logging the return value is broken code. In which case, do we really care for recalc_rate()? We could just return the rate that it was set to when clk_set_rate() was called and call it a day or return 0 for such clocks to indicate that the clock rate is "unknown". The whole concept of trying to recalculate the rate for a clock makes me feel uneasy since it promotes misunderstanding the behavior of the clock and writing bad code based on that misunderstanding. I would like to hear to real usecases before I propose some alternatives that I have in mind. Thanks, Saravana
On Wed, Oct 5, 2011 at 6:17 PM, Saravana Kannan <skannan@codeaurora.org> wrote: > On 09/22/2011 03:26 PM, Mike Turquette wrote: >> + unsigned long (*recalc_rate)(struct clk_hw *); >> + long (*round_rate)(struct clk_hw *, unsigned long); >> + struct clk * (*get_parent)(struct clk_hw *); >> +}; > > I would like to understand the need for recalc rate if that's something that > we want to go into the common framework (even if it's optional). I have > mostly heard only second hand explanations of the need for recalc_rate(), so > I might not have the full picture. But for all the cases that I can think > of, recalc_rate seems like a paradox. Recalc rate has four main uses that I can think of off the top of my head: 1) clk_set_rate is called on clock0, which is a non-leaf clock. All clocks "below" clock0 have had their rates changed, yet no one called clk_set_rate on those child clocks. We use recalc to walk the sub-tree of children and recalculate their rates based on the new rate of their parent, clock0. 2) exact same as #1, but using clk_set_parent instead of clk_set_rate. Again, changing the rate of a clock "high up" in the tree will affect the rates of many child clocks below it. 3) at boot-time/init-time when we don't trust the bootloader and need to determine the clock rates by parsing registers 4) modeling rates for clocks that we don't really control. This is not as common as the above three, but there are times when we're interested in knowing a clock rate (perhaps for debug purposes), but it's scaling logic is in firmware or somehow independent of the Linux clock framework. > If recalc_rate() is used to make sure the "current rate" of a "clock A" is > always known even if it's parent "clock B"'s rate is changed, then it also > means that the rate of "clock A" can change without clk_set_rate(clock A, > new rate). That in turn means that the clk_get_rate() just gives the > instantaneous snapshot of the rate. So, any use of clk_get_rate(clock A) for > anything other than printing/logging the return value is broken code. In For most clocks, the first three examples I give above will cover all of the times that a clock's rate will change. As long as a recalc/tree-walk is present then clk->rate is not out of sync and thus printing/reading that value is not broken. > which case, do we really care for recalc_rate()? We could just return the > rate that it was set to when clk_set_rate() was called and call it a day or > return 0 for such clocks to indicate that the clock rate is "unknown". What's the point of tracking a rate if it can't be trusted? Also, it is important to recalc rates whenever changes are made "high up" in the clock tree once we start to work on rate-change-arbitration issues, etc. Regards, Mike > The whole concept of trying to recalculate the rate for a clock makes me > feel uneasy since it promotes misunderstanding the behavior of the clock and > writing bad code based on that misunderstanding. > > I would like to hear to real usecases before I propose some alternatives > that I have in mind. > > Thanks, > Saravana > > -- > Sent by an employee of the Qualcomm Innovation Center, Inc. > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum. >
On Thu, Sep 22, 2011 at 03:26:56PM -0700, Mike Turquette wrote: > From: Jeremy Kerr <jeremy.kerr@canonical.com> > > 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 <tglx@linutronix.de>. > > The common clock definitions are based on a development patch from Ben > Herrenschmidt <benh@kernel.crashing.org>. > > TODO: > > * We don't keep any internal reference to the clock topology at present. > > Signed-off-by: Jeremy Kerr <jeremy.kerr@canonical.com> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com> > Signed-off-by: Mike Turquette <mturquette@ti.com> > --- > Changes since v1: > Create a dummy clk_unregister and prototype/document it and clk_register > Constify struct clk_hw_ops > Remove spinlock.h header, include kernel.h > Use EOPNOTSUPP instead of ENOTSUPP > Add might_sleep to clk_prepare/clk_unprepare stubs > Properly init children hlist and child_node > Whitespace and typo fixes > > drivers/clk/Kconfig | 3 + > drivers/clk/Makefile | 1 + > drivers/clk/clk.c | 232 ++++++++++++++++++++++++++++++++++++++++++++++++++ > drivers/clk/clkdev.c | 7 ++ > include/linux/clk.h | 140 +++++++++++++++++++++++++++--- > 5 files changed, 371 insertions(+), 12 deletions(-) > create mode 100644 drivers/clk/clk.c > > diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig > index 3530927..c53ed59 100644 > --- a/drivers/clk/Kconfig > +++ b/drivers/clk/Kconfig > @@ -5,3 +5,6 @@ config CLKDEV_LOOKUP > > config HAVE_MACH_CLKDEV > bool > + > +config GENERIC_CLK > + bool > diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile > index 07613fa..570d5b9 100644 > --- a/drivers/clk/Makefile > +++ b/drivers/clk/Makefile > @@ -1,2 +1,3 @@ > > obj-$(CONFIG_CLKDEV_LOOKUP) += clkdev.o > +obj-$(CONFIG_GENERIC_CLK) += clk.o > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > new file mode 100644 > index 0000000..1cd7315 > --- /dev/null > +++ b/drivers/clk/clk.c > @@ -0,0 +1,232 @@ > +/* > + * Copyright (C) 2010-2011 Canonical Ltd <jeremy.kerr@canonical.com> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + * > + * Standard functionality for the common clock API. > + */ > + > +#include <linux/clk.h> > +#include <linux/module.h> > +#include <linux/mutex.h> > +#include <linux/slab.h> > +#include <linux/spinlock.h> > + > +struct clk { > + const char *name; > + const struct clk_hw_ops *ops; > + struct clk_hw *hw; > + 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) > +{ > + 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); > + 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(const 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; > + > + INIT_HLIST_HEAD(&clk->children); > + INIT_HLIST_NODE(&clk->child_node); > + > + 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); Why not set it to parent's rate if recalc_rate is NULL? > + > + > + return clk; > +} > +EXPORT_SYMBOL_GPL(clk_register); > diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c > index 6db161f..e2a9719 100644 > --- a/drivers/clk/clkdev.c > +++ b/drivers/clk/clkdev.c > @@ -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: > diff --git a/include/linux/clk.h b/include/linux/clk.h > index 1d37f42..d6ae10b 100644 > --- a/include/linux/clk.h > +++ b/include/linux/clk.h > @@ -3,6 +3,7 @@ > * > * Copyright (C) 2004 ARM Limited. > * Written by Deep Blue Solutions Limited. > + * Copyright (c) 2010-2011 Jeremy Kerr <jeremy.kerr@canonical.com> > * > * This program is free software; you can redistribute it and/or modify > * it under the terms of the GNU General Public License version 2 as > @@ -11,17 +12,137 @@ > #ifndef __LINUX_CLK_H > #define __LINUX_CLK_H > > +#include <linux/kernel.h> > +#include <linux/errno.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. If a clock don't have any divider, recalc_rate may be NULL. In such case, clk_get_rate should return parent's rate. Thanks Richard > + * > + * @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); > + > +/** > + * clk_register - register and initialize a new clock > + * > + * @ops: ops for the new clock > + * @hw: struct clk_hw to be passed to the ops of the new clock > + * @name: name to use for the new clock > + * > + * Register a new clock with the clk subsystem. Returns either a > + * struct clk for the new clock or a NULL pointer. > + */ > +struct clk *clk_register(const struct clk_hw_ops *ops, struct clk_hw *hw, > + const char *name); > + > +/** > + * clk_unregister - remove a clock > + * > + * @clk: clock to unregister > + * > + * Remove a clock from the clk subsystem. This is currently not > + * implemented but is provided to allow unregistration code to be > + * written in drivers ready for use when an implementation is > + * provided. > + */ > +static inline int clk_unregister(struct clk *clk) > +{ > + return -EOPNOTSUPP; > +} > + > +#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) { > + might_sleep(); > + return 0; > +} > + > +static inline void clk_unprepare(struct clk *clk) { > + might_sleep(); > +} > + > +#endif /* !CONFIG_GENERIC_CLK */ > > /** > * clk_get - lookup and obtain a reference to a clock producer. > @@ -67,6 +188,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 +205,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 > @@ -97,7 +213,7 @@ void clk_put(struct clk *clk); > * Returns rounded clock rate in Hz, or negative errno. > */ > long clk_round_rate(struct clk *clk, unsigned long rate); > - > + > /** > * clk_set_rate - set the clock rate for a clock source > * @clk: clock source > @@ -106,7 +222,7 @@ long clk_round_rate(struct clk *clk, unsigned long rate); > * Returns success (0) or negative errno. > */ > int clk_set_rate(struct clk *clk, unsigned long rate); > - > + > /** > * clk_set_parent - set the parent clock source for this clock > * @clk: clock source > -- > 1.7.4.1 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
On Thu, Sep 22, 2011 at 03:26:56PM -0700, Mike Turquette wrote: > 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; > }; Eww, no, this really isn't going to scale for platforms like OMAP - to have all the operations indirected through a set of function pointers for every clock just because the enable register (or enable bit) is in a different position is completely absurd. I'm not soo concerned about the increase in memory usage (for 100 to 200 clock definitions its only 4 to 8k) but what worries me the most is initializing these damned things. It's an awful lot of initializers, which means the potential for an awful lot of conflicts should something change in this structure.
On Thu, Oct 13, 2011 at 7:44 AM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Thu, Sep 22, 2011 at 03:26:56PM -0700, Mike Turquette wrote: >> 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; >> }; > > Eww, no, this really isn't going to scale for platforms like OMAP - to > have all the operations indirected through a set of function pointers > for every clock just because the enable register (or enable bit) is > in a different position is completely absurd. Russel, I'm porting the OMAP clock framework to common clock right now and it is going well. For many clocks near the root of the tree I've been able to re-use struct clk_hw_fixed & clk_fixed_ops (see patch 3 or 4 in this series), which actually represents a decrease in memory consumption over the old OMAP struct clk (which populated many of the operations func pointers directly, causing duplication). So far I don't see scalability as an issue. In fact the design solves some problems neatly for us. For now I am creating one "uber clock", struct clk_hw_omap, which dumps all of the clksel, pll, gate control & mux crap directly from OMAP's old struct clk. This is not optimal for the long-term, but is a reasonable stepping stone which handles 90% of OMAP clocks. The new common struct clk makes it much easier for us to treat PLLs differently from typical dividers, which can be treated differently from dividers in modules, which can be treated differently from pure mux clocks, etc. > I'm not soo concerned about the increase in memory usage (for 100 to 200 > clock definitions its only 4 to 8k) but what worries me the most is > initializing these damned things. It's an awful lot of initializers, > which means the potential for an awful lot of conflicts should something > change in this structure. As I stated above, so far memory usage has actually *decreased* due to removing so many duplicated function pointers for OMAP's old struct clk. I wouldn't be surprised if other SoCs experienced the same lift. Also, in my own tree I've broken out clk_register into clk_init also. clk_register is still the thing to call if you have dynamically created clocks, as it handles the malloc, and it then calls clk_init. clk_init can be used by for static clock data and just handles initializing the mutex/list/whatever. Does this allay some of your concerns? Are you just trying to avoid allocating all of that memory dynamically? Regards, Mike
Hi Mike, On Thu, Sep 22, 2011 at 03:26:56PM -0700, Mike Turquette wrote: > From: Jeremy Kerr <jeremy.kerr@canonical.com> > > 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 <tglx@linutronix.de>. > > The common clock definitions are based on a development patch from Ben > Herrenschmidt <benh@kernel.crashing.org>. > > TODO: > > * We don't keep any internal reference to the clock topology at present. > > Signed-off-by: Jeremy Kerr <jeremy.kerr@canonical.com> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com> > Signed-off-by: Mike Turquette <mturquette@ti.com> > --- > Changes since v1: > Create a dummy clk_unregister and prototype/document it and clk_register > Constify struct clk_hw_ops > Remove spinlock.h header, include kernel.h > Use EOPNOTSUPP instead of ENOTSUPP > Add might_sleep to clk_prepare/clk_unprepare stubs > Properly init children hlist and child_node > Whitespace and typo fixes > > drivers/clk/Kconfig | 3 + > drivers/clk/Makefile | 1 + > drivers/clk/clk.c | 232 ++++++++++++++++++++++++++++++++++++++++++++++++++ > drivers/clk/clkdev.c | 7 ++ > include/linux/clk.h | 140 +++++++++++++++++++++++++++--- > 5 files changed, 371 insertions(+), 12 deletions(-) > create mode 100644 drivers/clk/clk.c > [...] > +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; > +} Could you expose __clk_enable/__clk_disable? I find it hard to implement clk group. clk group means, when a major clk enable/disable, it want a set of other clks enable/disable accordingly. > + > +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); > + [...] Thanks Richard
On Fri, Oct 14, 2011 at 04:10:26PM +0800, Richard Zhao wrote: > On Thu, Sep 22, 2011 at 03:26:56PM -0700, Mike Turquette wrote: snip essentially Mike's entire mail - *please* delete irrelevant quotes from your replies, it makes it very much easier to find the new text in your mail and is much more friendly to people reading mail on mobile devices. > > +static int __clk_enable(struct clk *clk) > > +{ > Could you expose __clk_enable/__clk_disable? I find it hard to implement > clk group. clk group means, when a major clk enable/disable, it want a set > of other clks enable/disable accordingly. Shouldn't this be something the core is implementing? I'd strongly expect that the clock drivers are relatively dumb and delegate all the decision making to the core API. Otherwise it's going to be hard for the core to implement any logic that involves working with more than one clock like rate change notification, or guarantee that driver requests made through the API are satisfied, as the state of the clocks will be changing underneath it.
On Thu, Sep 22, 2011 at 3:26 PM, Mike Turquette <mturquette@ti.com> wrote: > From: Jeremy Kerr <jeremy.kerr@canonical.com> > 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 *); In implementing recalc for divider clocks, I started to wonder, "why not just pass struct clk *clk into the clk_hw_ops func ptrs?". recalc is an obvious example whereby we need access to parent->rate. The code usually ends up looking something like: unsigned long omap_recalc_rate(struct clk_hw *hw) { struct clk *parent; struct clk_hw_omap *oclk; parent = hw->clk->parent; oclk = to_clk_omap(hw); ... } That's a bit of a song and dance to have to do in almost every op, and often these ops will need access to stuff like clk->rate also. Is there any opposition to just passing in struct clk? e.g: unsigned long omap_recalc_rate(struct clk *clk) { struct clk *parent; struct clk_hw_omap *oclk; parent = clk->parent; oclk = to_clk_omap(clk->hw); ... } It is a small nitpick, but it affects the API for everybody so best to get it right now before folks start migrating over to it. Thanks, Mike > 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 *); > };
On Fri, Oct 14, 2011 at 11:14:19AM -0700, Turquette, Mike wrote: > On Thu, Sep 22, 2011 at 3:26 PM, Mike Turquette <mturquette@ti.com> wrote: > > From: Jeremy Kerr <jeremy.kerr@canonical.com> > > 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 *); > > In implementing recalc for divider clocks, I started to wonder, "why > not just pass struct clk *clk into the clk_hw_ops func ptrs?". > > recalc is an obvious example whereby we need access to parent->rate. > The code usually ends up looking something like: > > unsigned long omap_recalc_rate(struct clk_hw *hw) > { > struct clk *parent; > struct clk_hw_omap *oclk; > > parent = hw->clk->parent; clk drivers can not see struct clk details. I use clk_get_parent. > oclk = to_clk_omap(hw); > ... > } > > That's a bit of a song and dance to have to do in almost every op, and > often these ops will need access to stuff like clk->rate also. Is > there any opposition to just passing in struct clk? e.g: > > unsigned long omap_recalc_rate(struct clk *clk) > { > struct clk *parent; > struct clk_hw_omap *oclk; > > parent = clk->parent; > oclk = to_clk_omap(clk->hw); > ... > } In my understanding, struct clk stores things specific to clk core, struct clk_hw stores common things needed by clk drivers. For static clk driver there' some problems: - For clocks without mux, I need duplicate a .parent and set .get_parent. Even when we adopt DT and dynamicly create clk, it's still a problem. Moving .parent to clk_hw can fix it. - When I define a clk array, I don't need to find another place to store .ops. It's not problem for dynamic creating clock. - As I mentioned in another mail, clk group need no lock version prepare/unprepare and enable/disable functions Another way is, add a "{struct clk_hw *clks; int count}" in clk_hw, let clk core handle it. I prefer the second way, but I'm not sure whether it's common enough. It's still a problem for dynamic creating clock. Thanks Richard > > It is a small nitpick, but it affects the API for everybody so best to > get it right now before folks start migrating over to it. > > Thanks, > Mike > > > 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 *); > > }; > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
On Fri, Oct 14, 2011 at 06:32:33PM +0800, Richard Zhao wrote: > On Fri, Oct 14, 2011 at 11:05:04AM +0100, Mark Brown wrote: > > On Fri, Oct 14, 2011 at 04:10:26PM +0800, Richard Zhao wrote: > > > On Thu, Sep 22, 2011 at 03:26:56PM -0700, Mike Turquette wrote: > > > > snip essentially Mike's entire mail - *please* delete irrelevant quotes > > from your replies, it makes it very much easier to find the new text in > > your mail and is much more friendly to people reading mail on mobile > > devices. > I snip not enough? sorry for that. I'll be carefull. > > > > > > +static int __clk_enable(struct clk *clk) > > > > +{ > > > > > Could you expose __clk_enable/__clk_disable? I find it hard to implement > > > clk group. clk group means, when a major clk enable/disable, it want a set > > > of other clks enable/disable accordingly. > > > > Shouldn't this be something the core is implementing? I'd strongly > > expect that the clock drivers are relatively dumb and delegate all the > > decision making to the core API. Otherwise it's going to be hard for > > the core to implement any logic that involves working with more than one > > clock like rate change notification, or guarantee that driver requests > > made through the API are satisfied, as the state of the clocks will be > > changing underneath it. > From my point of view, the first step of generic clk can be, easy to adopt > features of clocks in current mainline git. > Back to the clk group, I have a patch based on Sascha's work. > http://git.linaro.org/gitweb?p=people/riczhao/linux-2.6.git;a=shortlog;h=refs/heads/imx-clk I thought further about this and a clock group is not something we want to have at all. Clocks are supposed to be arranged in a tree and grouping clocks together violates this which leads to problems. This grouping should be done at driver level, so when a driver needs more than one clock it should request them all, maybe with a clk_get_all helper function. Sascha
On Fri, Oct 14, 2011 at 7:24 PM, Richard Zhao <richard.zhao@linaro.org> wrote: > On Fri, Oct 14, 2011 at 11:14:19AM -0700, Turquette, Mike wrote: >> On Thu, Sep 22, 2011 at 3:26 PM, Mike Turquette <mturquette@ti.com> wrote: >> unsigned long omap_recalc_rate(struct clk_hw *hw) >> { >> struct clk *parent; >> struct clk_hw_omap *oclk; >> >> parent = hw->clk->parent; > clk drivers can not see struct clk details. I use clk_get_parent. clk_get_parent should query the hardware to see what the parent is. This can have undesireable overhead. It is quite acceptable to reference a clock's parent through clk->parent, just as it is acceptable to get a clock rate through clk->rate. An analogous situation is a clk_get_rate call which uses a clk's .recalc. There is undesirable overhead involved in .recalc for clocks whose rates won't change behind our backs, so best to just treat the data in struct clk as cache and reference it directly. >> oclk = to_clk_omap(hw); >> ... >> } >> ... >> >> unsigned long omap_recalc_rate(struct clk *clk) >> { >> struct clk *parent; >> struct clk_hw_omap *oclk; >> >> parent = clk->parent; >> oclk = to_clk_omap(clk->hw); >> ... >> } > In my understanding, struct clk stores things specific to clk core, > struct clk_hw stores common things needed by clk drivers. For static clk driver > there' some problems: > - For clocks without mux, I need duplicate a .parent and set .get_parent. > Even when we adopt DT and dynamicly create clk, it's still a problem. > Moving .parent to clk_hw can fix it. For clocks with a fixed parent we should just pass it in at register-time. We should definitely not move .parent out of struct clk, since struct clk should be the platform agnostic bit that lets us do tree walks, build topology, etc etc. If you really want a .parent outside of struct clk then duplicate it in your struct clk_hw_imx and teach your .ops about it (analogous to struct clk_hw_fixed->rate). > - When I define a clk array, I don't need to find another place to store .ops. > It's not problem for dynamic creating clock. Something like the following? static struct clk aess_fclk; static const clk_hw_ops aess_fclk_ops = { .recalc = &omap2_clksel_recalc, .round_rate = &omap2_clksel_round_rate, .set_rate = &omap2_clksel_set_rate, }; static struct clk_hw_omap aess_fclk_hw = { .hw = { .clk = &aess_fclk, }, .clksel = &aess_fclk_div, .clksel_reg = OMAP4430_CM1_ABE_AESS_CLKCTRL, .clksel_mask = OMAP4430_CLKSEL_AESS_FCLK_MASK, }; static struct clk aess_fclk = { .name = "aess_fclk", .ops = &aess_fclk_ops, .hw = &aess_fclk_hw.hw, .parent = &abe_clk, }; > - As I mentioned in another mail, clk group need no lock version prepare/unprepare > and enable/disable functions Clock groups are out of scope for this first series. We should discuss more what the needs are for your clock groups. If it boils down to just enabling all of the clocks for a given device then you might want to abstract that away with pm_runtime_* calls, and maybe a supplementary layer like OMAP's hwmod. But I might be way off base, I really don't understand your use case for clock groups. > Another way is, add a "{struct clk_hw *clks; int count}" in clk_hw, let clk > core handle it. > I prefer the second way, but I'm not sure whether it's common enough. It's > still a problem for dynamic creating clock. struct clk_hw is just a pointer for navigating from struct clk -> struct your_custom_clk and vice versa. Again can you elaborate on your needs for managing multiple clocks with a single struct clk_hw? Thanks, Mike > > Thanks > Richard >> >> It is a small nitpick, but it affects the API for everybody so best to >> get it right now before folks start migrating over to it. >> >> Thanks, >> Mike >> >> > 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 *); >> > }; >> >> _______________________________________________ >> linux-arm-kernel mailing list >> linux-arm-kernel@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >> > >
On Mon, Oct 17, 2011 at 10:20:28AM +0100, Mark Brown wrote: > On Mon, Oct 17, 2011 at 04:48:52PM +0800, Richard Zhao wrote: > > > For example, devices that possible access to on-chip RAM, depend on OCRAM clock. > > On imx53, VPU depends on OCRAM clock, even when VPU does not use OCRAM. > > So if the VPU depends on OCRAM the VPU should be enabling the OCRAM > clock. The function of a given clock isn't terribly relevant, and > certainly grouping clocks together doesn't seem to be the obvious > solution from what you've said VPU don't know OCRAM clk, it's SoC specific. I know it's clock function replationship. But it's the only better place to refect the dependency in clock tree. Another dependency example, from SoC bus topology, some bus clk always depends on bus switch/hub. > - if the driver doesn't know about the > clock it seems like the core ought to be enabling it transparently > rather than gluing it together with some other random clock. If you mean clk core here, then we need things like below: struct clk_hw { struct clk *clk; struct dependency { struct clk_hw *clks; int count; }; }; Though Mike does not like to add things in clk_hw, but it's the only place to put common things of clk drivers. Thanks Richard > > Either way the point here is that individual drivers shouldn't be hand > coding this stuff, it should be being handled by core code. > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
On Mon, Oct 17, 2011 at 06:53:03PM +0800, Richard Zhao wrote: > On Mon, Oct 17, 2011 at 10:20:28AM +0100, Mark Brown wrote: > > On Mon, Oct 17, 2011 at 04:48:52PM +0800, Richard Zhao wrote: > > > > > For example, devices that possible access to on-chip RAM, depend on OCRAM clock. > > > On imx53, VPU depends on OCRAM clock, even when VPU does not use OCRAM. > > > > So if the VPU depends on OCRAM the VPU should be enabling the OCRAM > > clock. The function of a given clock isn't terribly relevant, and > > certainly grouping clocks together doesn't seem to be the obvious > > solution from what you've said > VPU don't know OCRAM clk, it's SoC specific. I know it's clock function > replationship. But it's the only better place to refect the dependency > in clock tree. Another dependency example, from SoC bus topology, some bus > clk always depends on bus switch/hub. > > - if the driver doesn't know about the > > clock it seems like the core ought to be enabling it transparently > > rather than gluing it together with some other random clock. > If you mean clk core here, then we need things like below: > struct clk_hw { > struct clk *clk; > struct dependency { > struct clk_hw *clks; > int count; > }; > }; > Though Mike does not like to add things in clk_hw, but it's the only place > to put common things of clk drivers. It's not a problem to associate multiple clocks to a device, we can do this now already. What a driver can't do now is give-me-all-clocks-I-need(dev), but this problem should not be solved at clock core level but at the clkdev level. The fact is that the different clocks for a device are really different clocks. A dumb driver may want to request/enable all relevant clocks at once while a more sophisticated driver may want to enable the clock for accessing registers in the probe function and a baud clock on device open time. Sascha
On Mon, Oct 17, 2011 at 01:05:39PM +0200, Sascha Hauer wrote: > It's not a problem to associate multiple clocks to a device, we can do > this now already. What a driver can't do now is give-me-all-clocks-I-need(dev), > but this problem should not be solved at clock core level but at the > clkdev level. I don't think it even needs solving at the clkdev level. In order to get all clocks for a specific device, we'd need variable length arrays to store the individual struct clk pointers, which isn't going to be all that nice. We'd need clk_get_alldev() and clk_put_alldev() to deal with these variable length arrays - and as far as the driver is concerned that's an opaque object - it can't know anything about any particular individual struct clk in the array. Then we'd need operations to operate on the array itself, which means much more API baggage. Also, if it did need to know about one particular struct clk, then there's problems with avoiding that struct clk in the alldev() versions (or we'll see drivers doing clk_get() followed by clk_disable() to work-around any enabling done via the array versions.) > The fact is that the different clocks for a device are really different > clocks. A dumb driver may want to request/enable all relevant clocks at > once while a more sophisticated driver may want to enable the clock for > accessing registers in the probe function and a baud clock on device > open time. I don't think you can get away from drivers having to know about their individual clocks in some form or other - and I don't think a dumb driver should be a justification for creating an API. If a dumb driver wants to get the clocks for a device it has to behave as if it were a smart driver and request each one (maybe having an array itself) such as this: static const char *con_ids[NR_CLKS] = { "foo", "bar", }; struct driver_priv { struct clk *clk[NR_CLKS]; }; for (err = i = 0; i < NR_CLKS; i++) { priv->clk[i] = clk_get(dev, con_ids[i]; if (IS_ERR(priv->clk[i])) { err = PTR_ERR(priv->clk[i]); break; } } if (err) { for (i = 0; i < NR_CLKS && !IS_ERR(priv->clk[i]); i++) clk_put(priv->clk[i]); return err; } This approach also has the advantage that we know what order the clocks will be in the array, and so we can sensibly index the array to obtain a particular clock. However, going back to the bus fabric case, a driver probably doesn't have the knowledge - it's a platform topology thing - one which can't be represented by a clock tree. It might help if we represented our busses closer to reality - like PCI does - rather than a flattened set of platform devices. Couple that with runtime PM and a driver for the bus fabric, and it should fall out fairly naturally from the infrastructure we already have.
On Thu, Sep 22, 2011 at 03:26:56PM -0700, Mike Turquette wrote: > +/* 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 change to CONFIG_GENERIC_CLK? > +extern int __clk_get(struct clk *clk); > +extern void __clk_put(struct clk *clk); clk.c does not define it. > +#endif Thanks Richard
Hi Mike, Some random comments/nits ... On Thu, Sep 22, 2011 at 03:26:56PM -0700, Mike Turquette wrote: > +struct clk *clk_register(const 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; > + > + INIT_HLIST_HEAD(&clk->children); > + INIT_HLIST_NODE(&clk->child_node); > + The children and child_node are introduced in patch #2, and should not be used in patch #1. > + 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 */ /* * Multiple line comments */ > + clk->parent = clk->ops->get_parent(clk->hw); > + > + if (clk->ops->recalc_rate) > + clk->rate = clk->ops->recalc_rate(clk->hw); > + > + One blank line is good enough. > + return clk; > +}
On Sun, Oct 23, 2011 at 5:55 AM, Shawn Guo <shawn.guo@freescale.com> wrote: > Hi Mike, > > Some random comments/nits ... Thanks for reviewing Shawn. Will roll changes into V3. Regards, Mike
Hi, On 09/22/2011 05:26 PM, Mike Turquette wrote: > diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig > index 3530927..c53ed59 100644 > --- a/drivers/clk/Kconfig > +++ b/drivers/clk/Kconfig > @@ -5,3 +5,6 @@ config CLKDEV_LOOKUP > > config HAVE_MACH_CLKDEV > bool > + > +config GENERIC_CLK > + bool Now that Russel's prepare/unprepare patch is mainlined, I think you want to select HAVE_CLK_PREPARE here (and remove your prepare/unprepare function declarations in clk.h). Regards, Domenico
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig index 3530927..c53ed59 100644 --- a/drivers/clk/Kconfig +++ b/drivers/clk/Kconfig @@ -5,3 +5,6 @@ config CLKDEV_LOOKUP config HAVE_MACH_CLKDEV bool + +config GENERIC_CLK + bool diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile index 07613fa..570d5b9 100644 --- a/drivers/clk/Makefile +++ b/drivers/clk/Makefile @@ -1,2 +1,3 @@ obj-$(CONFIG_CLKDEV_LOOKUP) += clkdev.o +obj-$(CONFIG_GENERIC_CLK) += clk.o diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c new file mode 100644 index 0000000..1cd7315 --- /dev/null +++ b/drivers/clk/clk.c @@ -0,0 +1,232 @@ +/* + * Copyright (C) 2010-2011 Canonical Ltd <jeremy.kerr@canonical.com> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * Standard functionality for the common clock API. + */ + +#include <linux/clk.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/slab.h> +#include <linux/spinlock.h> + +struct clk { + const char *name; + const struct clk_hw_ops *ops; + struct clk_hw *hw; + 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) +{ + 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); + 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(const 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; + + INIT_HLIST_HEAD(&clk->children); + INIT_HLIST_NODE(&clk->child_node); + + 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); + + + return clk; +} +EXPORT_SYMBOL_GPL(clk_register); diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c index 6db161f..e2a9719 100644 --- a/drivers/clk/clkdev.c +++ b/drivers/clk/clkdev.c @@ -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: diff --git a/include/linux/clk.h b/include/linux/clk.h index 1d37f42..d6ae10b 100644 --- a/include/linux/clk.h +++ b/include/linux/clk.h @@ -3,6 +3,7 @@ * * Copyright (C) 2004 ARM Limited. * Written by Deep Blue Solutions Limited. + * Copyright (c) 2010-2011 Jeremy Kerr <jeremy.kerr@canonical.com> * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License version 2 as @@ -11,17 +12,137 @@ #ifndef __LINUX_CLK_H #define __LINUX_CLK_H +#include <linux/kernel.h> +#include <linux/errno.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); + +/** + * clk_register - register and initialize a new clock + * + * @ops: ops for the new clock + * @hw: struct clk_hw to be passed to the ops of the new clock + * @name: name to use for the new clock + * + * Register a new clock with the clk subsystem. Returns either a + * struct clk for the new clock or a NULL pointer. + */ +struct clk *clk_register(const struct clk_hw_ops *ops, struct clk_hw *hw, + const char *name); + +/** + * clk_unregister - remove a clock + * + * @clk: clock to unregister + * + * Remove a clock from the clk subsystem. This is currently not + * implemented but is provided to allow unregistration code to be + * written in drivers ready for use when an implementation is + * provided. + */ +static inline int clk_unregister(struct clk *clk) +{ + return -EOPNOTSUPP; +} + +#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) { + might_sleep(); + return 0; +} + +static inline void clk_unprepare(struct clk *clk) { + might_sleep(); +} + +#endif /* !CONFIG_GENERIC_CLK */ /** * clk_get - lookup and obtain a reference to a clock producer. @@ -67,6 +188,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 +205,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 @@ -97,7 +213,7 @@ void clk_put(struct clk *clk); * Returns rounded clock rate in Hz, or negative errno. */ long clk_round_rate(struct clk *clk, unsigned long rate); - + /** * clk_set_rate - set the clock rate for a clock source * @clk: clock source @@ -106,7 +222,7 @@ long clk_round_rate(struct clk *clk, unsigned long rate); * Returns success (0) or negative errno. */ int clk_set_rate(struct clk *clk, unsigned long rate); - + /** * clk_set_parent - set the parent clock source for this clock * @clk: clock source