| Message ID | 1321926047-14211-4-git-send-email-mturquette@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Mon, Nov 21, 2011 at 05:40:45PM -0800, Mike Turquette wrote: [...] > +/** > + * DOC: Using the CLK_PARENT_SET_RATE flag > + * > + * __clk_set_rate changes the child's rate before the parent's to more > + * easily handle failure conditions. > + * > + * This means clk might run out of spec for a short time if its rate is > + * increased before the parent's rate is updated. > + * > + * To prevent this consider setting the CLK_GATE_SET_RATE flag on any > + * clk where you also set the CLK_PARENT_SET_RATE flag Eh, this is how flag CLK_GATE_SET_RATE is born? Does that mean to make the call succeed all the clocks on the propagating path need to be gated before clk_set_rate gets called? > + */ > +struct clk *__clk_set_rate(struct clk *clk, unsigned long rate) > +{ > + struct clk *fail_clk = NULL; > + int ret; > + unsigned long old_rate = clk->rate; > + unsigned long new_rate; > + unsigned long parent_old_rate; > + unsigned long parent_new_rate = 0; > + struct clk *child; > + struct hlist_node *tmp; > + > + /* bail early if we can't change rate while clk is enabled */ > + if ((clk->flags & CLK_GATE_SET_RATE) && clk->enable_count) > + return clk; > + > + /* find the new rate and see if parent rate should change too */ > + WARN_ON(!clk->ops->round_rate); > + > + new_rate = clk->ops->round_rate(clk, rate, &parent_new_rate); > + > + /* FIXME propagate pre-rate change notification here */ > + /* XXX note that pre-rate change notifications will stack */ > + > + /* change the rate of this clk */ > + ret = clk->ops->set_rate(clk, new_rate); Yes, right here, the clock gets set to some unexpected rate, since the parent clock has not been set to the target rate yet at this point. > + if (ret) > + return clk; > + > + /* > + * change the rate of the parent clk if necessary > + * > + * hitting the nested 'if' path implies we have hit a .set_rate > + * failure somewhere upstream while propagating __clk_set_rate > + * up the clk tree. roll back the clk rates one by one and > + * return the pointer to the clk that failed. clk_set_rate will > + * use the pointer to propagate a rate-change abort notifier > + * from the "highest" point. > + */ > + if ((clk->flags & CLK_PARENT_SET_RATE) && parent_new_rate) { > + parent_old_rate = clk->parent->rate; > + fail_clk = __clk_set_rate(clk->parent, parent_new_rate); > + > + /* roll back changes if parent rate change failed */ > + if (fail_clk) { > + pr_warn("%s: failed to set parent %s rate to %lu\n", > + __func__, fail_clk->name, > + parent_new_rate); > + clk->ops->set_rate(clk, old_rate); > + } > + return fail_clk; > + } > + > + /* > + * set clk's rate & recalculate the rates of clk's children > + * > + * hitting this path implies we have successfully finished > + * propagating recursive calls to __clk_set_rate up the clk tree > + * (if necessary) and it is safe to propagate clk_recalc_rates and > + * post-rate change notifiers down the clk tree from this point. > + */ > + clk->rate = new_rate; > + /* FIXME propagate post-rate change notifier for only this clk */ > + hlist_for_each_entry(child, tmp, &clk->children, child_node) > + clk_recalc_rates(child); > + > + return fail_clk; > +} [...] > diff --git a/include/linux/clk.h b/include/linux/clk.h > index 7213b52..3b0eb3f 100644 > --- a/include/linux/clk.h > +++ b/include/linux/clk.h > @@ -3,6 +3,8 @@ > * > * Copyright (C) 2004 ARM Limited. > * Written by Deep Blue Solutions Limited. > + * Copyright (c) 2010-2011 Jeremy Kerr <jeremy.kerr@canonical.com> > + * Copyright (C) 2011 Linaro Ltd <mturquette@linaro.org> > * > * This program is free software; you can redistribute it and/or modify > * it under the terms of the GNU General Public License version 2 as > @@ -13,17 +15,134 @@ > > #include <linux/kernel.h> > > +#include <linux/kernel.h> Eh, why adding a duplication? > +#include <linux/errno.h> > + > struct device; > > +struct clk; Do you really need this? [...] > +struct clk_hw_ops { > + int (*prepare)(struct clk *clk); > + void (*unprepare)(struct clk *clk); > + int (*enable)(struct clk *clk); > + void (*disable)(struct clk *clk); > + unsigned long (*recalc_rate)(struct clk *clk); > + long (*round_rate)(struct clk *clk, unsigned long, The return type seems interesting. If we intend to return a rate, it should be 'unsigned long' rather than 'long'. I'm just curious about the possible reason behind that. > + unsigned long *); > + int (*set_parent)(struct clk *clk, struct clk *); > + struct clk * (*get_parent)(struct clk *clk); > + int (*set_rate)(struct clk *clk, unsigned long); Nit: would it be reasonable to put set_rate after round_rate to group *_rate functions together? > +}; > + > +/** > + * clk_init - initialize the data structures in a struct clk > + * @dev: device initializing this clk, placeholder for now > + * @clk: clk being initialized > + * > + * Initializes the lists in struct clk, queries the hardware for the > + * parent and rate and sets them both. Adds the clk to the sysfs tree > + * topology. > + * > + * Caller must populate .name, .flags and .ops before calling clk_init. > + */ > +void clk_init(struct device *dev, struct clk *clk); > + > +#endif /* !CONFIG_GENERIC_CLK */ > > /** > * clk_get - lookup and obtain a reference to a clock producer. > @@ -107,9 +226,16 @@ static inline void clk_unprepare(struct clk *clk) > } > #endif > > +int __clk_enable(struct clk *clk); > +void __clk_disable(struct clk *clk); > +int __clk_prepare(struct clk *clk); > +void __clk_unprepare(struct clk *clk); > +void __clk_reparent(struct clk *clk, struct clk *new_parent); > + Do we really need to export all these common clk internal functions? Regards, Shawn > /** > * clk_get_rate - obtain the current clock rate (in Hz) for a clock source. > * This is only valid once the clock source has been enabled. > + * Returns zero if the clock rate is unknown. > * @clk: clock source > */ > unsigned long clk_get_rate(struct clk *clk); > -- > 1.7.4.1 > >
On Sat, Nov 26, 2011 at 5:22 AM, Shawn Guo <shawn.guo@freescale.com> wrote: > On Mon, Nov 21, 2011 at 05:40:45PM -0800, Mike Turquette wrote: >> + * To prevent this consider setting the CLK_GATE_SET_RATE flag on any >> + * clk where you also set the CLK_PARENT_SET_RATE flag > > Eh, this is how flag CLK_GATE_SET_RATE is born? Does that mean to make > the call succeed all the clocks on the propagating path need to be gated > before clk_set_rate gets called? Setting that flag on any clk implies nothing about the parents of that clk. Obviously if a clk's child is enabled then clk is enabled and won't have it's rate change if the flag is set, which is the whole point of the flag. Again, you don't have to set the flag. >> + /* change the rate of this clk */ >> + ret = clk->ops->set_rate(clk, new_rate); > > Yes, right here, the clock gets set to some unexpected rate, since the > parent clock has not been set to the target rate yet at this point. Sequence is irrelevant for the case where both parent and child change dividers, since the output of the child clk will be "weird" at some point. >> diff --git a/include/linux/clk.h b/include/linux/clk.h >> index 7213b52..3b0eb3f 100644 >> --- a/include/linux/clk.h >> +++ b/include/linux/clk.h >> @@ -3,6 +3,8 @@ >> * >> * Copyright (C) 2004 ARM Limited. >> * Written by Deep Blue Solutions Limited. >> + * Copyright (c) 2010-2011 Jeremy Kerr <jeremy.kerr@canonical.com> >> + * Copyright (C) 2011 Linaro Ltd <mturquette@linaro.org> >> * >> * This program is free software; you can redistribute it and/or modify >> * it under the terms of the GNU General Public License version 2 as >> @@ -13,17 +15,134 @@ >> >> #include <linux/kernel.h> >> >> +#include <linux/kernel.h> > > Eh, why adding a duplication? Will fix in v4. > >> +#include <linux/errno.h> >> + >> struct device; >> >> +struct clk; > > Do you really need this? Strange. This line existed pre-common clk patches. I'll look into why it is getting "added" back in. This declaration is necessary for the old-style clk frameworks which each defined their own struct clk. I assume it is still needed but I'll look at the git history to figure it out. >> +struct clk_hw_ops { >> + int (*prepare)(struct clk *clk); >> + void (*unprepare)(struct clk *clk); >> + int (*enable)(struct clk *clk); >> + void (*disable)(struct clk *clk); >> + unsigned long (*recalc_rate)(struct clk *clk); >> + long (*round_rate)(struct clk *clk, unsigned long, > > The return type seems interesting. If we intend to return a rate, it > should be 'unsigned long' rather than 'long'. I'm just curious about > the possible reason behind that. Yeah these are mostly from the original patch set. I'll go over these with a fine-tooth comb for v4. To some extent I think they model the return types of the clk API in clk.h. > >> + unsigned long *); >> + int (*set_parent)(struct clk *clk, struct clk *); >> + struct clk * (*get_parent)(struct clk *clk); >> + int (*set_rate)(struct clk *clk, unsigned long); > > Nit: would it be reasonable to put set_rate after round_rate to group > *_rate functions together? Will fix in v4. >> +int __clk_enable(struct clk *clk); >> +void __clk_disable(struct clk *clk); >> +int __clk_prepare(struct clk *clk); >> +void __clk_unprepare(struct clk *clk); >> +void __clk_reparent(struct clk *clk, struct clk *new_parent); >> + > Do we really need to export all these common clk internal functions? I think we do. Saravana also felt this was necessary. I know that for OMAP we need __clk_prepare, __clk_unprepare and __clk_reparent just to handle our PLLs. I don't think we need __clk_enable or __clk_disable since we can call the proper APIs for those with the prepare_mutex held. If no one needs __clk_enable and __clk_disable then I am happy to remove those declarations. Thanks for reviewing, Mike
Hello, Here are some initial comments on clk_get_rate(). On Mon, 21 Nov 2011, Mike Turquette wrote: > +/** > + * clk_get_rate - return the rate of clk > + * @clk: the clk whose rate is being returned > + * > + * Simply returns the cached rate of the clk. Does not query the hardware. If > + * clk is NULL then returns 0. > + */ > +unsigned long clk_get_rate(struct clk *clk) > +{ > + if (!clk) > + return 0; > + > + return clk->rate; > +} > +EXPORT_SYMBOL_GPL(clk_get_rate); This implementation of clk_get_rate() is racy, and is, in general, unsafe. The problem is that, in many cases, the clock's rate may change between the time that clk_get_rate() is called and the time that the returned rate is used. This is the case for many clocks which are part of a larger DVFS domain, for example. Several changes are needed to fix this: 1. When a clock user calls clk_enable() on a clock, the clock framework should prevent other users of the clock from changing the clock's rate. This should persist until the clock user calls clk_disable() (but see also #2 below). This will ensure that clock users can rely on the rate returned by clk_get_rate(), as long as it's called between clk_enable() and clk_disable(). And since the clock's rate is guaranteed to remain the same during this time, code that cannot tolerate clock rate changes without special handling (such as driver code for external I/O devices) will work safely without further modification. 2. Since the protocol described in #1 above will prevent DVFS from working when the clock is part of a larger DVFS clock group, functions need to be added to allow and prevent other clock users from changing the clock's rate. I'll use the function names "clk_allow_rate_changes(struct clk *)" and "clk_block_rate_changes(struct clk *)" for this discussion. These functions can be used by clock users to define critical sections during which other entities on the system are allowed to change a clock's rate - even if the clock is currently enabled. (Note that when a clock is prevented from changing its rate, all of the clocks from it up to the root of the tree should also be prevented from changing rates, since parent rate changes generally cause disruptions in downstream clocks.) 3. For the above changes to work, the clock framework will need to discriminate between different clock users' calls to clock functions like clk_{get,set}_rate(), etc. Luckily this should be possible within the current clock interface. clk_get() can allocate and return a new struct clk that clk_put() can later free. One useful side effect of doing this is that the clock framework could catch unbalanced clk_{enable,disable}() calls. 4. clk_get_rate() must return an error when it's called in situations where the caller hasn't ensured that the clock's rate won't be changed by other entities. For non-fixed rate clocks, these forbidden sections would include code between a clk_get() and a clk_enable(), or between a clk_disable() and a clk_put() or clk_enable(), or between a clk_allow_rate_changes() and clk_block_rate_changes(). The first and second of those three cases will require some code auditing of clk_get_rate() users to ensure that they are only calling it after they've enabled the clock. And presumably most of them are not checking for error. include/linux/clk.h doesn't define an error return value, so this needs to be explicitly defined in clk.h. - Paul
Hi a few initial comments on clk_get_parent(). On Mon, 21 Nov 2011, Mike Turquette wrote: > +/** > + * clk_get_parent - return the parent of a clk > + * @clk: the clk whose parent gets returned > + * > + * Simply returns clk->parent. It is up to the caller to hold the > + * prepare_lock, if desired. Returns NULL if clk is NULL. > + */ > +struct clk *clk_get_parent(struct clk *clk) > +{ > + if (!clk) > + return NULL; > + > + return clk->parent; > +} > +EXPORT_SYMBOL_GPL(clk_get_parent); This implementation of clk_get_parent() has similar problems to the clk_get_rate() implementation: http://lkml.org/lkml/2011/11/30/403 clk_get_parent() should return an error if some other entity can change the clock's parent between the time that clk_get_parent() returns, and the time that the returned struct clk * is used. For this to work, we need to define when clk_get_parent() should succeed. If we follow the protocol outlined in the above URL about clk_get_rate(), and we stipulate that when we block rate changes, we also block parent changes, then this should be fairly straightforward. clk_get_parent() can succeed: 1. between a clk_enable() and a clk_disable() or clk_allow_rate_changes(), 2. between a clk_block_rate_changes() and a clk_enable(), clk_disable(), or clk_allow_rate_changes() As with clk_get_rate(), include/linux/clk.h is missing documentation of what the error return value should be. This should be a little easier to define than with clk_get_rate(), but I don't think the error value should be NULL. This is because NULL is a valid return value when clk_get_parent() is called on root clocks. Better to use ERR_PTR(-EINVAL/-EBUSY/etc.). - Paul
On Wed, Nov 30, 2011 at 5:20 PM, Paul Walmsley <paul@pwsan.com> wrote: > This implementation of clk_get_rate() is racy, and is, in general, unsafe. > The problem is that, in many cases, the clock's rate may change between > the time that clk_get_rate() is called and the time that the returned > rate is used. This is the case for many clocks which are part of a > larger DVFS domain, for example. Hi Paul, Thanks much for reviewing. Just FYI, the v4 patchset I'm working on has clk_get_rate and clk_get_parent hold the prepare mutex themselves, so it solves some of the raciness of accessing struct clk members. This change does not address your points below but I wanted to include it for completeness sake. > Several changes are needed to fix this: > > 1. When a clock user calls clk_enable() on a clock, the clock framework > should prevent other users of the clock from changing the clock's rate. > This should persist until the clock user calls clk_disable() (but see also > #2 below). This will ensure that clock users can rely on the rate > returned by clk_get_rate(), as long as it's called between clk_enable() > and clk_disable(). And since the clock's rate is guaranteed to remain the > same during this time, code that cannot tolerate clock rate changes > without special handling (such as driver code for external I/O devices) > will work safely without further modification. This is certainly imposing a default behavior in the clk framework core. > 2. Since the protocol described in #1 above will prevent DVFS from working > when the clock is part of a larger DVFS clock group, functions need to be > added to allow and prevent other clock users from changing the clock's > rate. I'll use the function names "clk_allow_rate_changes(struct clk *)" > and "clk_block_rate_changes(struct clk *)" for this discussion. These > functions can be used by clock users to define critical sections during > which other entities on the system are allowed to change a clock's rate - > even if the clock is currently enabled. (Note that when a clock is > prevented from changing its rate, all of the clocks from it up to the root > of the tree should also be prevented from changing rates, since parent > rate changes generally cause disruptions in downstream clocks.) Likewise when a clk is requested to transition to a new frequency it will have to clear it with all of the clks below it, so there is still a need to propagate a request throughout the clk tree whenever clk_set_rate is called and take a decision. One way to solve this is for driver owners to sprinkle their code with clk_block_rate_change and clk_allow_rate_change as you propose. There is a problem with this method if it is not supplemented with rate-change notifications that drivers are allowed to handle asynchronously. Take for example a MMC driver which normally runs it's functional clk at 24MHz. However for a low-intensity, periodic task such as playing an mp3 from SD card we would really like to lower clk rates across the whole SoC. Assuming that the MMC driver holds clk_block_rate_change across any SD card transaction, our low power scheme to lower clks across the SoC is stuck in a racy game of trying to request a lower clk rate for the MMC functional clk while that same MMC controller is mostly saturated serving up the mp3. In this case it would be nice to have asynchronous notifiers that can interrupt the MMC driver with a pre-change notifier and say "please finish your current transaction, stall your next transaction, and then return so I can change your clk rate". Then after the clk rate change occurs a post-change notification would also go out to the MMC driver saying "ok I'm done changing rates. here is your new clk rate and please continue working". The notification is very polite. It is worth nothing that the MMC driver can return NOTIFY_STOP in it's pre-change notification handler and this effectively does the same thing as traversing the clk subtree and checking to make sure that nobody is holding clk_block_rate_change against any of the children clocks. The point of all of that text above is to say: if we have to walk the whole clk subtree below the point where we want to change rates AND we want to make a dynamic case-by-case call on whether to allow the rate change to happen (as opposed to contending with the allow/block semantics) then I think that only having notifications will suffice. > 3. For the above changes to work, the clock framework will need to > discriminate between different clock users' calls to clock functions like > clk_{get,set}_rate(), etc. Luckily this should be possible within the > current clock interface. clk_get() can allocate and return a new struct > clk that clk_put() can later free. One useful side effect of doing this > is that the clock framework could catch unbalanced clk_{enable,disable}() > calls. This is definitely worth thinking about. Another way to accomplish this is stop treating prepare_count and enable_count as scalars and instead vectorize them by tracking a list of struct device *'s. This starts to get into can-of-worms territory if we want to consider clk_set_rate_range(dev, clk, min, max) and the broader DVFS implications. I'm a little worried about under-designing now for future DVFS stuff, but I also don't want the common clk fundamentals to stall any more than it has (2+ years!). > 4. clk_get_rate() must return an error when it's called in situations > where the caller hasn't ensured that the clock's rate won't be changed by > other entities. For non-fixed rate clocks, these forbidden sections would > include code between a clk_get() and a clk_enable(), or between a > clk_disable() and a clk_put() or clk_enable(), or between a > clk_allow_rate_changes() and clk_block_rate_changes(). The first and > second of those three cases will require some code auditing of > clk_get_rate() users to ensure that they are only calling it after they've > enabled the clock. And presumably most of them are not checking for > error. include/linux/clk.h doesn't define an error return value, so this > needs to be explicitly defined in clk.h. This adds a lot of burden to a driver that just wants to know a rate. Especially if that purpose is for something as simple/transient as a pr_debug message or something. Thanks again for the review. I'll chew on it a bit. Regards, Mike
Hi Mike a few brief comments. On Wed, 30 Nov 2011, Turquette, Mike wrote: > Likewise when a clk is requested to transition to a new frequency it > will have to clear it with all of the clks below it, so there is still > a need to propagate a request throughout the clk tree whenever > clk_set_rate is called and take a decision. > > One way to solve this is for driver owners to sprinkle their code with > clk_block_rate_change and clk_allow_rate_change as you propose. > There is a problem with this method if it is not supplemented with > rate-change notifications that drivers are allowed to handle > asynchronously. I don't think notifiers have a bearing on clk_get_rate(). > In this case it would be nice to have asynchronous notifiers that can > interrupt the MMC driver with a pre-change notifier and say "please > finish your current transaction, stall your next transaction, and then > return so I can change your clk rate". Then after the clk rate change > occurs a post-change notification would also go out to the MMC driver > saying "ok I'm done changing rates. here is your new clk rate and > please continue working". The notification is very polite. I haven't made any comments about clock notifiers yet, because my comments so far have only concerned clk_get_rate() and clk_get_parent(). Clock rate/parent-change notifiers are requirements for DVFS use-cases, and they must be paired with something like the clk_{allow,block}_rate_change() functions to work efficiently. I intend to comment on this later; it's not a simple problem. It might be worth noting that Tero and I implemented a simplified version of this for the N900. > It is worth nothing that the MMC driver can return NOTIFY_STOP in it's > pre-change notification handler and this effectively does the same > thing as traversing the clk subtree and checking to make sure that > nobody is holding clk_block_rate_change against any of the children > clocks. > > The point of all of that text above is to say: if we have to walk the > whole clk subtree below the point where we want to change rates AND we > want to make a dynamic case-by-case call on whether to allow the rate > change to happen (as opposed to contending with the allow/block > semantics) then I think that only having notifications will suffice. That would not be good. A notifier implementation without something like clk_{allow,block}_rate_change() is going to waste a lot of CPU time making pointless calls into the notifier chains by whatever is attempting to do top-down DVFS. With clk_{allow,block}_rate_change(), only a single comparison is needed to determine whether or not the rate change can succeed. A blocking clk_set_rate() variant could also be efficiently implemented with that information, if so desired. In general, a clock rate change notifier should almost never return NOTIFY_STOP. That should only happen if some event occurred that took place after the notifier chain started. > > 3. For the above changes to work, the clock framework will need to > > discriminate between different clock users' calls to clock functions like > > clk_{get,set}_rate(), etc. Luckily this should be possible within the > > current clock interface. clk_get() can allocate and return a new struct > > clk that clk_put() can later free. One useful side effect of doing this > > is that the clock framework could catch unbalanced clk_{enable,disable}() > > calls. > > This is definitely worth thinking about. Another way to accomplish > this is stop treating prepare_count and enable_count as scalars and > instead vectorize them by tracking a list of struct device *'s. To return to my original comments on clk_get_rate(): how would this allow the clock framework to discriminate between callers of clk_get_rate()? (Or indeed any clock framework function?) Or is your comment simply addressing the unbalanced clk_{enable,disable}() case? > > 4. clk_get_rate() must return an error when it's called in situations > > where the caller hasn't ensured that the clock's rate won't be changed by > > other entities. For non-fixed rate clocks, these forbidden sections would > > include code between a clk_get() and a clk_enable(), or between a > > clk_disable() and a clk_put() or clk_enable(), or between a > > clk_allow_rate_changes() and clk_block_rate_changes(). The first and > > second of those three cases will require some code auditing of > > clk_get_rate() users to ensure that they are only calling it after they've > > enabled the clock. And presumably most of them are not checking for > > error. include/linux/clk.h doesn't define an error return value, so this > > needs to be explicitly defined in clk.h. > > This adds a lot of burden to a driver that just wants to know a rate. > Especially if that purpose is for something as simple/transient as a > pr_debug message or something. Could you clarify what burden are you referring to? The above protocol requires few (if any) changes to existing clock framework users. So for example, the following code: c = clk_get(dev, clk_name); clk_enable(c); rate = clk_get_rate(c); would still continue to work, since the rate would be guaranteed not to change after the clk_enable(). However, the (unsafe): c = clk_get(dev, clk_name); rate = clk_get_rate(c); would need to be modified to become safe, by adding a clk_block_rate_change() before the clk_get_rate(). - Paul
On Wed, Nov 30, 2011 at 06:20:50PM -0700, Paul Walmsley wrote: > 1. When a clock user calls clk_enable() on a clock, the clock framework > should prevent other users of the clock from changing the clock's rate. > This should persist until the clock user calls clk_disable() (but see also > #2 below). This will ensure that clock users can rely on the rate > returned by clk_get_rate(), as long as it's called between clk_enable() > and clk_disable(). And since the clock's rate is guaranteed to remain the > same during this time, code that cannot tolerate clock rate changes > without special handling (such as driver code for external I/O devices) > will work safely without further modification. So, if you have a PLL whose parent clock is not used by anything else. You want to program it to a certain rate. You call clk_disable() on the PLL clock. This walks up the tree and disables the parent. You then try to set the rate using clk_set_rate(). clk_set_rate() in this circumstance can't wait for the PLL to lock because it can't - there's no reference clock for it. You then call clk_enable(). The PLL now takes its time to lock. You can't sleep in clk_enable() because it might be called from atomic contexts, so you have to spin waiting for this. Overloading clk_disable/clk_enable in this way is a bad solution to this problem.
On Wed, Nov 30, 2011 at 11:39:59PM -0700, Paul Walmsley wrote: > Clock rate/parent-change notifiers are requirements for DVFS use-cases, > and they must be paired with something like the > clk_{allow,block}_rate_change() functions to work efficiently. I intend > to comment on this later; it's not a simple problem. It might be worth > noting that Tero and I implemented a simplified version of this for the > N900. I'm thinking that if we're going to have clk_{allow,block}_rate_change() we may as well make that the main interface to enable rate changes - if a device wants to change the clock rate it allows rate changes using that interface rather than by disabling the clocks. I've got devices which can do glitch free updates of active clocks so having to disable would be a real restriction, and cpufreq would have issues with actually disabling the clock too I expect.
On Thu, Dec 1, 2011 at 6:42 AM, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote: > On Wed, Nov 30, 2011 at 11:39:59PM -0700, Paul Walmsley wrote: > >> Clock rate/parent-change notifiers are requirements for DVFS use-cases, >> and they must be paired with something like the >> clk_{allow,block}_rate_change() functions to work efficiently. I intend >> to comment on this later; it's not a simple problem. It might be worth >> noting that Tero and I implemented a simplified version of this for the >> N900. > > I'm thinking that if we're going to have clk_{allow,block}_rate_change() > we may as well make that the main interface to enable rate changes - if > a device wants to change the clock rate it allows rate changes using > that interface rather than by disabling the clocks. I've got devices > which can do glitch free updates of active clocks so having to disable > would be a real restriction, and cpufreq would have issues with actually > disabling the clock too I expect. > I agree that imposing a "disable clk before changing rate" policy in the framework core is a bad idea. During discussion on the CLK_GATE_SET_RATE flag in the patch #2 Shawn commented that he has some clks that must be enabled to change their rates (I assume he means PLLs but that detail doesn't really matter). Regards, Mike
Hi Mark, On Thu, 1 Dec 2011, Mark Brown wrote: > On Wed, Nov 30, 2011 at 11:39:59PM -0700, Paul Walmsley wrote: > > > Clock rate/parent-change notifiers are requirements for DVFS use-cases, > > and they must be paired with something like the > > clk_{allow,block}_rate_change() functions to work efficiently. I intend > > to comment on this later; it's not a simple problem. It might be worth > > noting that Tero and I implemented a simplified version of this for the > > N900. > > I'm thinking that if we're going to have clk_{allow,block}_rate_change() > we may as well make that the main interface to enable rate changes - if > a device wants to change the clock rate it allows rate changes using > that interface rather than by disabling the clocks. I've got devices > which can do glitch free updates of active clocks so having to disable > would be a real restriction, and cpufreq would have issues with actually > disabling the clock too I expect. The intention behind the clk_{allow,block}_rate_change() proposal was to allow the current user of the clock to change its rate without having to call clk_{allow,block}_rate_change(), if that driver was the sole user of the clock. So for example, if you had a driver that did: c = clk_get(dev, clk_name); clk_enable(c); clk_set_rate(c, clk_rate); and c was currently not enabled by any other driver on the system, and nothing else had called clk_block_rate_change(c), then the rate change would be allowed to proceed. (modulo any notifier activity, etc.) So clk_{allow,block}_rate_change() was simply intended to allow or restrict other users of the same clock, not the current user. - Paul
On Thu, Dec 01, 2011 at 11:30:16AM -0700, Paul Walmsley wrote: > So for example, if you had a driver that did: > c = clk_get(dev, clk_name); > clk_enable(c); > clk_set_rate(c, clk_rate); > and c was currently not enabled by any other driver on the system, and > nothing else had called clk_block_rate_change(c), then the rate change > would be allowed to proceed. (modulo any notifier activity, etc.) > So clk_{allow,block}_rate_change() was simply intended to allow or > restrict other users of the same clock, not the current user. Ah, sorry! I'd totally misunderstood what you were proposing.
On Thu, Dec 01, 2011 at 11:30:16AM -0700, Paul Walmsley wrote: > The intention behind the clk_{allow,block}_rate_change() proposal was to > allow the current user of the clock to change its rate without having to > call clk_{allow,block}_rate_change(), if that driver was the sole user of > the clock. And how does a driver know that?
Hi Russell, On Thu, 1 Dec 2011, Russell King - ARM Linux wrote: > On Wed, Nov 30, 2011 at 06:20:50PM -0700, Paul Walmsley wrote: > > 1. When a clock user calls clk_enable() on a clock, the clock framework > > should prevent other users of the clock from changing the clock's rate. > > This should persist until the clock user calls clk_disable() (but see also > > #2 below). This will ensure that clock users can rely on the rate > > returned by clk_get_rate(), as long as it's called between clk_enable() > > and clk_disable(). And since the clock's rate is guaranteed to remain the > > same during this time, code that cannot tolerate clock rate changes > > without special handling (such as driver code for external I/O devices) > > will work safely without further modification. > > So, if you have a PLL whose parent clock is not used by anything else. > You want to program it to a certain rate. > > You call clk_disable() on the PLL clock. The approach described wouldn't require the PLL to be disabled before changing its rate. If there are no other users of the PLL, or if the other users of the PLL have indicated that it's safe for others to change the PLL's rate, the clock framework would allow the PLL rate change, even if the PLL is enabled. (modulo any notifier activity, and assuming that the underlying PLL hardware allows its frequency to be reprogrammed while the PLL is enabled) > This walks up the tree and disables the parent. You then try to set the > rate using clk_set_rate(). clk_set_rate() in this circumstance can't > wait for the PLL to lock because it can't - there's no reference clock > for it. As an aside, this seems like a good time to mention that the behavior of clk_set_rate() on a disabled clock needs to be clarified. - Paul
On Fri, Dec 02, 2011 at 10:13:10AM -0700, Paul Walmsley wrote: > Hi Russell, > > On Thu, 1 Dec 2011, Russell King - ARM Linux wrote: > > > On Wed, Nov 30, 2011 at 06:20:50PM -0700, Paul Walmsley wrote: > > > 1. When a clock user calls clk_enable() on a clock, the clock framework > > > should prevent other users of the clock from changing the clock's rate. > > > This should persist until the clock user calls clk_disable() (but see also > > > #2 below). This will ensure that clock users can rely on the rate > > > returned by clk_get_rate(), as long as it's called between clk_enable() > > > and clk_disable(). And since the clock's rate is guaranteed to remain the > > > same during this time, code that cannot tolerate clock rate changes > > > without special handling (such as driver code for external I/O devices) > > > will work safely without further modification. > > > > So, if you have a PLL whose parent clock is not used by anything else. > > You want to program it to a certain rate. > > > > You call clk_disable() on the PLL clock. > > The approach described wouldn't require the PLL to be disabled before > changing its rate. If there are no other users of the PLL, or if the > other users of the PLL have indicated that it's safe for others to change > the PLL's rate, the clock framework would allow the PLL rate change, even > if the PLL is enabled. (modulo any notifier activity, and assuming that > the underlying PLL hardware allows its frequency to be reprogrammed while > the PLL is enabled) > > > This walks up the tree and disables the parent. You then try to set the > > rate using clk_set_rate(). clk_set_rate() in this circumstance can't > > wait for the PLL to lock because it can't - there's no reference clock > > for it. > > As an aside, this seems like a good time to mention that the behavior of > clk_set_rate() on a disabled clock needs to be clarified. It's more complicated than that. Clocks have more states than just enabled and disabled. There is: - unprepared - prepared and disabled - prepared and enabled Implementations can chose at which point to enable their PLLs and wait for them to lock - but if they want to sleep while waiting, they must do this in the prepare method, not the enable method (remember, enable is to be callable from atomic contexts.) So, it's entirely possible that a prepared clock will have the PLLs up and running, which means that clk_set_rate() can also wait for the PLL to stablize (which would be a good idea.) Now... we can say that PLLs should be locked when the prepare method returns, or whenever clk_set_rate() returns. The problem with that is there's a race condition between clk_enable() and clk_set_rate() - if we allow clk_set_rate() to sleep waiting for the PLL to lock, a concurrent clk_enable() can't be prevented from returning because that would involve holding a spinlock... I think this is just one of those unsolvable issues - if you have concurrent users of a PLL there's not much you can do - race free - to prevent the PLL changing beneath some user.
Hello, On Fri, Dec 02, 2011 at 08:23:06PM +0000, Russell King - ARM Linux wrote: > On Fri, Dec 02, 2011 at 10:13:10AM -0700, Paul Walmsley wrote: > > Hi Russell, > > > > On Thu, 1 Dec 2011, Russell King - ARM Linux wrote: > > > > > On Wed, Nov 30, 2011 at 06:20:50PM -0700, Paul Walmsley wrote: > > > > 1. When a clock user calls clk_enable() on a clock, the clock framework > > > > should prevent other users of the clock from changing the clock's rate. > > > > This should persist until the clock user calls clk_disable() (but see also > > > > #2 below). This will ensure that clock users can rely on the rate > > > > returned by clk_get_rate(), as long as it's called between clk_enable() > > > > and clk_disable(). And since the clock's rate is guaranteed to remain the > > > > same during this time, code that cannot tolerate clock rate changes > > > > without special handling (such as driver code for external I/O devices) > > > > will work safely without further modification. > > > > > > So, if you have a PLL whose parent clock is not used by anything else. > > > You want to program it to a certain rate. > > > > > > You call clk_disable() on the PLL clock. > > > > The approach described wouldn't require the PLL to be disabled before > > changing its rate. If there are no other users of the PLL, or if the > > other users of the PLL have indicated that it's safe for others to change > > the PLL's rate, the clock framework would allow the PLL rate change, even > > if the PLL is enabled. (modulo any notifier activity, and assuming that > > the underlying PLL hardware allows its frequency to be reprogrammed while > > the PLL is enabled) > > > > > This walks up the tree and disables the parent. You then try to set the > > > rate using clk_set_rate(). clk_set_rate() in this circumstance can't > > > wait for the PLL to lock because it can't - there's no reference clock > > > for it. > > > > As an aside, this seems like a good time to mention that the behavior of > > clk_set_rate() on a disabled clock needs to be clarified. > > It's more complicated than that. Clocks have more states than just > enabled and disabled. > > There is: > - unprepared > - prepared and disabled > - prepared and enabled > > Implementations can chose at which point to enable their PLLs and wait > for them to lock - but if they want to sleep while waiting, they must > do this in the prepare method, not the enable method (remember, enable > is to be callable from atomic contexts.) > > So, it's entirely possible that a prepared clock will have the PLLs up > and running, which means that clk_set_rate() can also wait for the PLL > to stablize (which would be a good idea.) > > Now... we can say that PLLs should be locked when the prepare method > returns, or whenever clk_set_rate() returns. The problem with that is > there's a race condition between clk_enable() and clk_set_rate() - if > we allow clk_set_rate() to sleep waiting for the PLL to lock, a > concurrent clk_enable() can't be prevented from returning because > that would involve holding a spinlock... But you can achieve that the concurrent clk_enable fails. Would that be sane? Best regards Uwe
On Thu, 1 Dec 2011, Russell King - ARM Linux wrote: > On Thu, Dec 01, 2011 at 11:30:16AM -0700, Paul Walmsley wrote: > > The intention behind the clk_{allow,block}_rate_change() proposal was to > > allow the current user of the clock to change its rate without having to > > call clk_{allow,block}_rate_change(), if that driver was the sole user of > > the clock. > > And how does a driver know that? The driver author might make this assumption when the device's upstream clock tree is simple. This assumption simplifies the driver's clock code, since no clock rate change notifier callback would be needed. This should be possible: 1. when there are no other users of the device's upstream clocks; or, 2. when a subset of the device's upstream clock tree can be used by other devices, but the rates or source clocks of that subset either cannot be changed, or should never change. For example, consider an external audio codec device, such as the TLV320AIC23. The input clock source rate for this chip is likely to be fixed for a given board design[1]. So the driver wouldn't need a clock rate change notifier callback for its input clock source, since it would never be called. The clock dividers inside the codec may be represented by clock nodes, but since they are handled by the same driver, the coordination between them doesn't require any notifier callbacks. Another example would be a device that shares a portion of its upstream clock tree with other devices, where the upstream clock tree is designed to run at the same rate for all of the devices, such as the USIM IP block on OMAP3430/3630[2]. The USIM shares a dedicated DPLL with the USBHOST and USBTLL devices, but the DPLL is only intended to be programmed at a single rate. When the USIM is using this DPLL as a clock source, a dedicated downstream clock divider is available that can be used to reduce the USIM's input clock rate. So the USIM driver would also not require any clock rate change notifier callbacks, since the shared portions of its parent clock tree should never change. Of course, it is possible that a new chip could appear with an identical IP block inside it, but where cases 1 and 2 above are no longer true. The worst outcome is that the driver's clk_set_rate() may no longer succeed, returning -EBUSY, since another device may be blocking the rate change. The remedy is to update the driver to add the rate change callback, and the associated internal logic to deal with it. - Paul 1. Texas Instruments, _TLV320AIC23 Stereo Audio CODEC, 8- to 96-kHz, With Integrated Headphone Amplifier Data Manual_ (SLWS106D), May 2002, Section 3.3.2 "Audio Sampling Rates" and section 1.2 "Functional Block Diagram". Available from http://www.ti.com/lit/gpn/tlv320aic23 (among others) 2. Linux kernel v3.1-rc4, arch/arm/mach-omap2/clock3xxx_data.c, usim_clksel (line 2300). Available from http://git.kernel.org/gitweb.cgi?p=linux/kernel/git/torvalds/linux.git;a=blob;f=arch/arm/mach-omap2/clock3xxx_data.c;h=5d0064a4fb5a638bb1141489354244e95899c1a1;hb=HEAD#l2300 (among others)
Hi a brief comment concerning clock rates: On Mon, 21 Nov 2011, Mike Turquette wrote: > +unsigned long clk_get_rate(struct clk *clk) ... > +long clk_round_rate(struct clk *clk, unsigned long rate) ... > +int clk_set_rate(struct clk *clk, unsigned long rate) ... > +struct clk { ... > + unsigned long rate; ... > +}; The types associated with clock rates in the clock interface (include/linux/clk.h) are inconsistent, and we should fix this. clk_round_rate() is the problematic case, returning a signed long rather than an unsigned long. So clk_round_rate() won't work correctly with any rates higher than 2 GiHz. We could fix the immediate problem by changing the prototype of clk_round_rate() to pass the rounded rate back to the caller via a pointer in one of the arguments, and return an error code (if any) via the return value: int clk_round_rate(struct clk *clk, unsigned long rate, unsigned long *rounded_rate); But I'd propose that we instead increase the size of struct clk.rate to be s64: s64 clk_round_rate(struct clk *clk, s64 desired_rate); int clk_set_rate(struct clk *clk, s64 rate); s64 clk_get_rate(struct clk *clk); struct clk { ... s64 rate; ... }; That way the clock framework can accommodate current clock rates, as well as any conceivable future clock rate. (Some production CPUs are already running at clock rates greater than 4 GiHZ[1]. RF devices with 4 GiHz+ clock rates are also common, such as 802.11a devices running in the 5.8 GHz band, and drivers for those may eventually wish to use the clock framework.) - Paul 1. www.cpu-world.com, "Intel Xeon X5698 - AT80614007314AA" http://www.cpu-world.com/CPUs/Xeon/Intel-Xeon%20X5698%20-%20AT80614007314AA.html
On Mon, Nov 21, 2011 at 5:40 PM, Mike Turquette <mturquette@ti.com> wrote: > +/** > + * clk_get_parent - return the parent of a clk > + * @clk: the clk whose parent gets returned > + * > + * Simply returns clk->parent. It is up to the caller to hold the > + * prepare_lock, if desired. Returns NULL if clk is NULL. > + */ > +struct clk *clk_get_parent(struct clk *clk) > +{ > + if (!clk) > + return NULL; > + > + return clk->parent; > +} > +EXPORT_SYMBOL_GPL(clk_get_parent); While auditing the uses/expectations of the current clk API users, I see that clk_get_parent is rarely used; it is in fact only called in 11 files throughout the kernel. I decided to have a little audit and here is what I found: arch/arm/mach-shmobile/board-ap4evb.c: fsi_ak4642_set_rate Used within clk_set_rate. Can dereference clk->parent directly and ideally should propagate up the clk tree using the new recursive clk_set_rate. arch/arm/mach-tegra/usb_phy.c: tegra_usb_phy_open Used within a clk_get_rate. pll_u should ideally have it's own clk->rate populated, reducing the need for tegra_usb_phy_open to know details of the clk tree. The logic to pull pll_u's rate from it's parent belongs to pll_u's .recalc_rate callback. arch/arm/plat-s5p/clock.c: s5p_spdif_set_rate Another clk_get_parent call from within a .set_rate callback. Again: use clk->parent directly and better yet, propagate the rate change up via the recursive clk_set_rate. arch/arm/plat-s5p/clock.c: s5p_spdif_get_rate Another clk_get_parent call from within a .recalc_rate callback. Again, clk->rate should be populated with parent's rate correctly, otherwise dereference clk->parent directly without use of clk_get_parent. arch/arm/plat-s5p/s5p-time.c: s5p_clockevent_init This problem would be solved by propagating rate_change requests up two-levels of parents via the new recursive clk_set_rate. There is also a clk_set_parent call in here, which has nothing to do with the clk_get_parent call, but it makes me wonder if we should revisit the issue of switching parents as part of clk_set_rate: clk_set_rate(tin_event, pclk->rate / 2); arch/arm/plat-samsung/pwm.c: pwm_is_tdiv Used in only two places: as part of pwm_calc_tin which could be replaced wholesale by a better .round_rate implementation and for a debug print (who cares). arch/arm/plat-samsung/pwm.c: pwm_calc_tin Just a .round_rate implementation. Can dereference clk->parent directly. arch/arm/plat-samsung/time.c: s3c2410_timer_setup Same as s5p_clockevent_init above. drivers/sh/clk/core.c: clk_round_parent An elaborate .round_rate that should have resulted from recursive propagation up to clk->parent. drivers/video/omap2/dss/dss.c: Every call to clk_get_parent in here is wrapped in clk_get_rate. The functions that use this are effectively .set_rate, .get_rate and .recalc_rate doppelgangers. Also a debug print calls clk_get_parent but who cares. drivers/video/sh_mobile_hdmi.c: Used in a .set_rate and .round_rate implementation. No reason not to deref clk->parent directly. The above explanations take a little liberty listing certain functions as .round_rate, .set_rate and .recalc_rate callbacks, but that is because a lot of the code mentioned could be neatly wrapped up that way. Do we really need clk_get_parent? The primary problem with it is ambiguity in the API: should the caller hold a lock? Is the rate guaranteed not the change after being called? A top-level clk API function that can be called within other top-level clk API functions is inconsitent: most of the time this would incur nested locking. Also having a top-level API expose the tree structure encourages platform and driver developers to put clk tree details into their code as opposed to having clever .round_rate and .set_rate callbacks hide these details from them with the new recursive clk_set_rate. Thoughts? Thanks, Mike
On Mon, Dec 05, 2011 at 02:15:56PM -0700, Paul Walmsley wrote: > The types associated with clock rates in the clock interface > (include/linux/clk.h) are inconsistent, and we should fix this. Rubbish. They're different with good reason. Rates are primerily unsigned quantities - and should be treated as such. The exception is clk_round_rate() which returns the rate, but also _may_ return an error. Therefore, its return type has to be signed. > We could fix the immediate problem by changing the prototype of > clk_round_rate() to pass the rounded rate back to the caller via a pointer > in one of the arguments, and return an error code (if any) via the return > value: > > int clk_round_rate(struct clk *clk, unsigned long rate, unsigned long > *rounded_rate); Yes that might have been a better solution. > But I'd propose that we instead increase the size of struct clk.rate to be > s64: > > s64 clk_round_rate(struct clk *clk, s64 desired_rate); > int clk_set_rate(struct clk *clk, s64 rate); > s64 clk_get_rate(struct clk *clk); > > struct clk { > ... > s64 rate; > ... > }; > > That way the clock framework can accommodate current clock rates, as well > as any conceivable future clock rate. (Some production CPUs are already > running at clock rates greater than 4 GiHZ[1]. RF devices with 4 GiHz+ > clock rates are also common, such as 802.11a devices running in the 5.8 > GHz band, and drivers for those may eventually wish to use the clock > framework.) Yuck. You are aware that 64-bit math on 32-bit CPUs sucks? So burdening _everything_ with 64-bit rate quantities is absurd. As for making then 64-bit signed quantities, that's asking for horrid code from gcc for the majority of cases.
On Mon, 5 Dec 2011, Russell King - ARM Linux wrote: > On Mon, Dec 05, 2011 at 02:15:56PM -0700, Paul Walmsley wrote: > > > But I'd propose that we instead increase the size of struct clk.rate to be > > s64: > > > > s64 clk_round_rate(struct clk *clk, s64 desired_rate); > > int clk_set_rate(struct clk *clk, s64 rate); > > s64 clk_get_rate(struct clk *clk); > > > > struct clk { > > ... > > s64 rate; > > ... > > }; > > > > That way the clock framework can accommodate current clock rates, as well > > as any conceivable future clock rate. (Some production CPUs are already > > running at clock rates greater than 4 GiHZ[1]. RF devices with 4 GiHz+ > > clock rates are also common, such as 802.11a devices running in the 5.8 > > GHz band, and drivers for those may eventually wish to use the clock > > framework.) > > Yuck. You are aware that 64-bit math on 32-bit CPUs sucks? So burdening > _everything_ with 64-bit rate quantities is absurd. As for making then > 64-bit signed quantities, that's asking for horrid code from gcc for the > majority of cases. 64-bit divides would be the only real issue in the clock framework context. And those are easily avoided on clock hardware where the parent rate can never exceed 32 bits. For example, here's a trivial implementation for rate recalculation for a integer divider clock node (that can't be handled with a right shift): s64 div(struct clk *clk, u32 div) { if (clk->flags & CLK_PARENT_RATE_MAX_U32) return ((u32)(clk->parent->rate & 0xffffffff)) / div; clk->rate = clk->parent->rate; do_div(clk->rate, div); return clk->rate; } gcc generates this for ARMv6: 00000010 <div>: 10: e92d4038 push {r3, r4, r5, lr} 14: e1a05000 mov r5, r0 18: e5d03010 ldrb r3, [r0, #16] @ clk->flags 1c: e1a04001 mov r4, r1 20: e3130001 tst r3, #1 @ 32-bit parent rate? 24: 1a000007 bne 48 <div+0x38> @ do 32-bit divide /* 64-bit divide by 32-bit follows */ 28: e5903000 ldr r3, [r0] 2c: e1c300d8 ldrd r0, [r3, #8] 30: ebfffffe bl 0 <__do_div64> 34: e1a00002 mov r0, r2 38: e1a01003 mov r1, r3 3c: e5852008 str r2, [r5, #8] 40: e585300c str r3, [r5, #12] 44: e8bd8038 pop {r3, r4, r5, pc} /* 32-bit divide follows */ 48: e5900000 ldr r0, [r0] 4c: e5900008 ldr r0, [r0, #8] 50: ebfffffe bl 0 <__aeabi_uidiv> 54: e3a01000 mov r1, #0 58: e8bd8038 pop {r3, r4, r5, pc} Not bad at all, and this isn't even optimized. (The conditional could be avoided completely with a little work during clock init.) What little overhead there is seems quite trivial if it means that the clock framework will be useful for present and future devices. - Paul
On Mon, 5 Dec 2011, Paul Walmsley wrote: > For example, here's a trivial implementation for rate recalculation for a > integer divider clock node (that can't be handled with a right shift): > > s64 div(struct clk *clk, u32 div) { > if (clk->flags & CLK_PARENT_RATE_MAX_U32) > return ((u32)(clk->parent->rate & 0xffffffff)) / div; > > clk->rate = clk->parent->rate; > do_div(clk->rate, div); > return clk->rate; > } (removing some useless cruft from the above function, for clarity's sake) s64 div(struct clk *clk, u32 div) { s64 r; if (clk->flags & CLK_PARENT_RATE_MAX_U32) return ((u32)clk->parent->rate) / div; r = clk->parent->rate; do_div(r, div); return r; } 00000000 <div>: 0: e92d4010 push {r4, lr} 4: e1a04001 mov r4, r1 8: e5d03010 ldrb r3, [r0, #16] c: e3130001 tst r3, #1 10: 1a000005 bne 2c <div+0x2c> 14: e5903000 ldr r3, [r0] 18: e1c300d8 ldrd r0, [r3, #8] 1c: ebfffffe bl 0 <__do_div64> 20: e1a00002 mov r0, r2 24: e1a01003 mov r1, r3 28: e8bd8010 pop {r4, pc} 2c: e5900000 ldr r0, [r0] 30: e5900008 ldr r0, [r0, #8] 34: ebfffffe bl 0 <__aeabi_uidiv> 38: e3a01000 mov r1, #0 3c: e8bd8010 pop {r4, pc} - Paul
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig index 7a9899bd..adc0586 100644 --- a/drivers/clk/Kconfig +++ b/drivers/clk/Kconfig @@ -8,3 +8,7 @@ config HAVE_MACH_CLKDEV config HAVE_CLK_PREPARE bool + +config GENERIC_CLK + bool + select HAVE_CLK_PREPARE diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile index 07613fa..570d5b9 100644 --- a/drivers/clk/Makefile +++ b/drivers/clk/Makefile @@ -1,2 +1,3 @@ obj-$(CONFIG_CLKDEV_LOOKUP) += clkdev.o +obj-$(CONFIG_GENERIC_CLK) += clk.o diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c new file mode 100644 index 0000000..12c9994 --- /dev/null +++ b/drivers/clk/clk.c @@ -0,0 +1,538 @@ +/* + * Copyright (C) 2010-2011 Canonical Ltd <jeremy.kerr@canonical.com> + * Copyright (C) 2011 Linaro Ltd <mturquette@linaro.org> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * Standard functionality for the common clock API. See Documentation/clk.txt + */ + +#include <linux/clk.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/slab.h> +#include <linux/spinlock.h> +#include <linux/err.h> + +static DEFINE_SPINLOCK(enable_lock); +static DEFINE_MUTEX(prepare_lock); + +void __clk_unprepare(struct clk *clk) +{ + if (!clk) + return; + + if (WARN_ON(clk->prepare_count == 0)) + return; + + if (--clk->prepare_count > 0) + return; + + WARN_ON(clk->enable_count > 0); + + if (clk->ops->unprepare) + clk->ops->unprepare(clk); + + __clk_unprepare(clk->parent); +} + +/** + * clk_unprepare - perform the slow part of a clk gate + * @clk: the clk being gated + * + * clk_unprepare may sleep, which differentiates it from clk_disable. In a + * simple case, clk_unprepare can be used instead of clk_disable to gate a clk + * if the operation may sleep. One example is a clk which is accessed over + * I2c. In the complex case a clk gate operation may require a fast and a slow + * part. It is this reason that clk_unprepare and clk_disable are not mutually + * exclusive. In fact clk_disable must be called before clk_unprepare. + */ +void clk_unprepare(struct clk *clk) +{ + mutex_lock(&prepare_lock); + __clk_unprepare(clk); + mutex_unlock(&prepare_lock); +} +EXPORT_SYMBOL_GPL(clk_unprepare); + +int __clk_prepare(struct clk *clk) +{ + int ret = 0; + + if (!clk) + return 0; + + if (clk->prepare_count == 0) { + ret = __clk_prepare(clk->parent); + if (ret) + return ret; + + if (clk->ops->prepare) { + ret = clk->ops->prepare(clk); + if (ret) { + __clk_unprepare(clk->parent); + return ret; + } + } + } + + clk->prepare_count++; + + return 0; +} + +/** + * clk_prepare - perform the slow part of a clk ungate + * @clk: the clk being ungated + * + * clk_prepare may sleep, which differentiates it from clk_enable. In a simple + * case, clk_prepare can be used instead of clk_enable to ungate a clk if the + * operation may sleep. One example is a clk which is accessed over I2c. In + * the complex case a clk ungate operation may require a fast and a slow part. + * It is this reason that clk_prepare and clk_enable are not mutually + * exclusive. In fact clk_prepare must be called before clk_enable. + * Returns 0 on success, -EERROR otherwise. + */ +int clk_prepare(struct clk *clk) +{ + int ret; + + mutex_lock(&prepare_lock); + ret = __clk_prepare(clk); + mutex_unlock(&prepare_lock); + + return ret; +} +EXPORT_SYMBOL_GPL(clk_prepare); + +void __clk_disable(struct clk *clk) +{ + if (!clk) + return; + + if (WARN_ON(clk->enable_count == 0)) + return; + + if (--clk->enable_count > 0) + return; + + if (clk->ops->disable) + clk->ops->disable(clk); + + if (clk->parent) + __clk_disable(clk->parent); +} + +/** + * clk_disable - perform the fast part of a clk gate + * @clk: the clk being gated + * + * clk_disable must not sleep, which differentiates it from clk_unprepare. In + * a simple case, clk_disable can be used instead of clk_unprepare to gate a + * clk if the operation is fast and will never sleep. One example is a + * SoC-internal clk which is controlled via simple register writes. In the + * complex case a clk gate operation may require a fast and a slow part. It is + * this reason that clk_unprepare and clk_disable are not mutually exclusive. + * In fact clk_disable must be called before clk_unprepare. + */ +void clk_disable(struct clk *clk) +{ + unsigned long flags; + + spin_lock_irqsave(&enable_lock, flags); + __clk_disable(clk); + spin_unlock_irqrestore(&enable_lock, flags); +} +EXPORT_SYMBOL_GPL(clk_disable); + +int __clk_enable(struct clk *clk) +{ + int ret = 0; + + if (!clk) + return 0; + + if (WARN_ON(clk->prepare_count == 0)) + return -ESHUTDOWN; + + if (clk->enable_count == 0) { + if (clk->parent) + ret = __clk_enable(clk->parent); + + if (ret) + return ret; + + if (clk->ops->enable) { + ret = clk->ops->enable(clk); + if (ret) { + __clk_disable(clk->parent); + return ret; + } + } + } + + clk->enable_count++; + return 0; +} + +/** + * clk_enable - perform the slow part of a clk ungate + * @clk: the clk being ungated + * + * clk_enable must not sleep, which differentiates it from clk_prepare. In a + * simple case, clk_enable can be used instead of clk_prepare to ungate a clk + * if the operation will never sleep. One example is a SoC-internal clk which + * is controlled via simple register writes. In the complex case a clk ungate + * operation may require a fast and a slow part. It is this reason that + * clk_enable and clk_prepare are not mutually exclusive. In fact clk_prepare + * must be called before clk_enable. Returns 0 on success, -EERROR + * otherwise. + */ +int clk_enable(struct clk *clk) +{ + unsigned long flags; + int ret; + + spin_lock_irqsave(&enable_lock, flags); + ret = __clk_enable(clk); + spin_unlock_irqrestore(&enable_lock, flags); + + return ret; +} +EXPORT_SYMBOL_GPL(clk_enable); + +/** + * clk_get_rate - return the rate of clk + * @clk: the clk whose rate is being returned + * + * Simply returns the cached rate of the clk. Does not query the hardware. If + * clk is NULL then returns 0. + */ +unsigned long clk_get_rate(struct clk *clk) +{ + if (!clk) + return 0; + + return clk->rate; +} +EXPORT_SYMBOL_GPL(clk_get_rate); + +/** + * clk_round_rate - round the given rate for a clk + * @clk: the clk for which we are rounding a rate + * @rate: the rate which is to be rounded + * + * Takes in a rate as input and rounds it to a rate that the clk can actually + * use which is then returned. If clk doesn't support round_rate operation + * then the rate passed in is returned. + */ +long clk_round_rate(struct clk *clk, unsigned long rate) +{ + if (clk && clk->ops->round_rate) + return clk->ops->round_rate(clk, rate, NULL); + + return rate; +} +EXPORT_SYMBOL_GPL(clk_round_rate); + +/** + * clk_recalc_rates + * @clk: first clk in the subtree + * + * Walks the subtree of clks starting with clk and recalculates rates as it + * goes. Note that if a clk does not implement the recalc_rate operation then + * propagation of that subtree stops and all of that clks children will not + * have their rates updated. Caller must hold prepare_lock. + */ +static void clk_recalc_rates(struct clk *clk) +{ + unsigned long old_rate; + struct hlist_node *tmp; + struct clk *child; + + old_rate = clk->rate; + + if (clk->ops->recalc_rate) + clk->rate = clk->ops->recalc_rate(clk); + + if (old_rate == clk->rate) + return; + + /* FIXME add post-rate change notification here */ + + hlist_for_each_entry(child, tmp, &clk->children, child_node) + clk_recalc_rates(child); +} + +/** + * DOC: Using the CLK_PARENT_SET_RATE flag + * + * __clk_set_rate changes the child's rate before the parent's to more + * easily handle failure conditions. + * + * This means clk might run out of spec for a short time if its rate is + * increased before the parent's rate is updated. + * + * To prevent this consider setting the CLK_GATE_SET_RATE flag on any + * clk where you also set the CLK_PARENT_SET_RATE flag + */ +struct clk *__clk_set_rate(struct clk *clk, unsigned long rate) +{ + struct clk *fail_clk = NULL; + int ret; + unsigned long old_rate = clk->rate; + unsigned long new_rate; + unsigned long parent_old_rate; + unsigned long parent_new_rate = 0; + struct clk *child; + struct hlist_node *tmp; + + /* bail early if we can't change rate while clk is enabled */ + if ((clk->flags & CLK_GATE_SET_RATE) && clk->enable_count) + return clk; + + /* find the new rate and see if parent rate should change too */ + WARN_ON(!clk->ops->round_rate); + + new_rate = clk->ops->round_rate(clk, rate, &parent_new_rate); + + /* FIXME propagate pre-rate change notification here */ + /* XXX note that pre-rate change notifications will stack */ + + /* change the rate of this clk */ + ret = clk->ops->set_rate(clk, new_rate); + if (ret) + return clk; + + /* + * change the rate of the parent clk if necessary + * + * hitting the nested 'if' path implies we have hit a .set_rate + * failure somewhere upstream while propagating __clk_set_rate + * up the clk tree. roll back the clk rates one by one and + * return the pointer to the clk that failed. clk_set_rate will + * use the pointer to propagate a rate-change abort notifier + * from the "highest" point. + */ + if ((clk->flags & CLK_PARENT_SET_RATE) && parent_new_rate) { + parent_old_rate = clk->parent->rate; + fail_clk = __clk_set_rate(clk->parent, parent_new_rate); + + /* roll back changes if parent rate change failed */ + if (fail_clk) { + pr_warn("%s: failed to set parent %s rate to %lu\n", + __func__, fail_clk->name, + parent_new_rate); + clk->ops->set_rate(clk, old_rate); + } + return fail_clk; + } + + /* + * set clk's rate & recalculate the rates of clk's children + * + * hitting this path implies we have successfully finished + * propagating recursive calls to __clk_set_rate up the clk tree + * (if necessary) and it is safe to propagate clk_recalc_rates and + * post-rate change notifiers down the clk tree from this point. + */ + clk->rate = new_rate; + /* FIXME propagate post-rate change notifier for only this clk */ + hlist_for_each_entry(child, tmp, &clk->children, child_node) + clk_recalc_rates(child); + + return fail_clk; +} + +/** + * clk_set_rate - specify a new rate for clk + * @clk: the clk whose rate is being changed + * @rate: the new rate for clk + * + * In the simplest case clk_set_rate will only change the rate of clk. + * + * If clk has the CLK_GATE_SET_RATE flag set and it is enabled this call + * will fail; only when the clk is disabled will it be able to change + * its rate. + * + * Setting the CLK_PARENT_SET_RATE flag allows clk_set_rate to + * recursively propagate up to clk's parent; whether or not this happens + * depends on the outcome of clk's .round_rate implementation. If + * *parent_rate is 0 after calling .round_rate then upstream parent + * propagation is ignored. If *parent_rate comes back with a new rate + * for clk's parent then we propagate up to clk's parent and set it's + * rate. Upward propagation will continue until either a clk does not + * support the CLK_PARENT_SET_RATE flag or .round_rate stops requesting + * changes to clk's parent_rate. If there is a failure during upstream + * propagation then clk_set_rate will unwind and restore each clk's rate + * that had been successfully changed. Afterwards a rate change abort + * notification will be propagated downstream, starting from the clk + * that failed. + * + * At the end of all of the rate setting, clk_set_rate internally calls + * clk_recalc_rates and propagates the rate changes downstream, starting + * from the highest clk whose rate was changed. This has the added + * benefit of propagating post-rate change notifiers. + * + * Note that while post-rate change and rate change abort notifications + * are guaranteed to be sent to a clk only once per call to + * clk_set_rate, pre-change notifications will be sent for every clk + * whose rate is changed. Stacking pre-change notifications is noisy + * for the drivers subscribed to them, but this allows drivers to react + * to intermediate clk rate changes up until the point where the final + * rate is achieved at the end of upstream propagation. + * + * Returns 0 on success, -EERROR otherwise. + */ +int clk_set_rate(struct clk *clk, unsigned long rate) +{ + struct clk *fail_clk; + int ret = 0; + + if (!clk->ops->set_rate) + return -ENOSYS; + + /* bail early if nothing to do */ + if (rate == clk->rate) + return rate; + + /* prevent racing with updates to the clock topology */ + mutex_lock(&prepare_lock); + fail_clk = __clk_set_rate(clk, rate); + if (fail_clk) { + pr_warn("%s: failed to set %s rate to %lu\n", + __func__, fail_clk->name, rate); + /* FIXME propagate rate change abort notification here */ + ret = -EIO; + } + mutex_unlock(&prepare_lock); + + return ret; +} +EXPORT_SYMBOL_GPL(clk_set_rate); + +/** + * clk_get_parent - return the parent of a clk + * @clk: the clk whose parent gets returned + * + * Simply returns clk->parent. It is up to the caller to hold the + * prepare_lock, if desired. Returns NULL if clk is NULL. + */ +struct clk *clk_get_parent(struct clk *clk) +{ + if (!clk) + return NULL; + + return clk->parent; +} +EXPORT_SYMBOL_GPL(clk_get_parent); + +void __clk_reparent(struct clk *clk, struct clk *new_parent) +{ + hlist_del(&clk->child_node); + hlist_add_head(&clk->child_node, &new_parent->children); + + clk->parent = new_parent; + + /* FIXME update sysfs clock topology */ +} + +/** + * clk_set_parent - switch the parent of a mux clk + * @clk: the mux clk whose input we are switching + * @parent: the new input to clk + * + * Re-parent clk to use parent as it's new input source. If clk has the + * CLK_GATE_SET_PARENT flag set then clk must be gated for this + * operation to succeed. After successfully changing clk's parent + * clk_set_parent will update the clk topology, sysfs topology and + * propagate rate recalculation via clk_recalc_rates. Returns 0 on + * success, -EERROR otherwise. + */ +int clk_set_parent(struct clk *clk, struct clk *parent) +{ + int ret = 0; + + if (!clk || !clk->ops) + return -EINVAL; + + if (!clk->ops->set_parent) + return -ENOSYS; + + if (clk->parent == parent) + return 0; + + /* prevent racing with updates to the clock topology */ + mutex_lock(&prepare_lock); + + /* FIXME add pre-rate change notification here */ + + /* only re-parent if the clock is not in use */ + if ((clk->flags & CLK_GATE_SET_PARENT) && clk->prepare_count) + ret = -EBUSY; + else + ret = clk->ops->set_parent(clk, parent); + + if (ret < 0) + /* FIXME add rate change abort notification here */ + goto out; + + /* update tree topology */ + __clk_reparent(clk, parent); + + /* + * If successful recalculate the rates of the clock, including + * children. + */ + clk_recalc_rates(clk); + +out: + mutex_unlock(&prepare_lock); + + return ret; +} +EXPORT_SYMBOL_GPL(clk_set_parent); + +/** + * clk_init - initialize the data structures in a struct clk + * @dev: device initializing this clk, placeholder for now + * @clk: clk being initialized + * + * Initializes the lists in struct clk, queries the hardware for the + * parent and rate and sets them both. Adds the clk to the sysfs tree + * topology. + * + * Caller must populate clk->name and clk->flags before calling + * clk_init. A key assumption in the call to .get_parent is that clks + * are being initialized in-order. + */ +void clk_init(struct device *dev, struct clk *clk) +{ + if (!clk) + return; + + INIT_HLIST_HEAD(&clk->children); + INIT_HLIST_NODE(&clk->child_node); + + mutex_lock(&prepare_lock); + + if (clk->ops->get_parent) { + clk->parent = clk->ops->get_parent(clk); + if (clk->parent) + hlist_add_head(&clk->child_node, + &clk->parent->children); + } else + clk->parent = NULL; + + if (clk->ops->recalc_rate) + clk->rate = clk->ops->recalc_rate(clk); + else + clk->rate = 0; + + mutex_unlock(&prepare_lock); + + return; +} +EXPORT_SYMBOL_GPL(clk_init); diff --git a/include/linux/clk.h b/include/linux/clk.h index 7213b52..3b0eb3f 100644 --- a/include/linux/clk.h +++ b/include/linux/clk.h @@ -3,6 +3,8 @@ * * Copyright (C) 2004 ARM Limited. * Written by Deep Blue Solutions Limited. + * Copyright (c) 2010-2011 Jeremy Kerr <jeremy.kerr@canonical.com> + * Copyright (C) 2011 Linaro Ltd <mturquette@linaro.org> * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License version 2 as @@ -13,17 +15,134 @@ #include <linux/kernel.h> +#include <linux/kernel.h> +#include <linux/errno.h> + struct device; +struct clk; + +#ifdef CONFIG_GENERIC_CLK + /* - * The base API. + * flags used across common struct clk. these flags should only affect the + * top-level framework. custom flags for dealing with hardware specifics + * belong in struct clk_hw_your_unique_device */ +#define CLK_GATE_SET_RATE BIT(0) /* must be gated across rate change */ +#define CLK_GATE_SET_PARENT BIT(1) /* must be gated across re-parent */ +#define CLK_PARENT_SET_RATE BIT(2) /* propagate rate change up one level */ +#define CLK_IGNORE_UNUSED BIT(3) /* do not gate even if unused */ +#define CLK_GATE_RATE_CHANGE (CLK_GATE_SET_RATE | CLK_GATE_SET_PARENT) -/* - * struct clk - an machine class defined object / cookie. +struct clk { + const char *name; + const struct clk_hw_ops *ops; + struct clk *parent; + unsigned long rate; + unsigned long flags; + unsigned int enable_count; + unsigned int prepare_count; + struct hlist_head children; + struct hlist_node child_node; +}; + +/** + * struct clk_hw_ops - Callback operations for hardware clocks; these are to + * be provided by the clock implementation, and will be called by drivers + * through the clk_* API. + * + * @prepare: Prepare the clock for enabling. This must not return until + * the clock is fully prepared, and it's safe to call clk_enable. + * This callback is intended to allow clock implementations to + * do any initialisation that may sleep. Called with + * prepare_lock held. + * + * @unprepare: Release the clock from its prepared state. This will typically + * undo any work done in the @prepare callback. Called with + * prepare_lock held. + * + * @enable: Enable the clock atomically. This must not return until the + * clock is generating a valid clock signal, usable by consumer + * devices. Called with enable_lock held. This function must not + * sleep. + * + * @disable: Disable the clock atomically. Called with enable_lock held. + * This function must not sleep. + * + * @recalc_rate Recalculate the rate of this clock, by quering hardware + * and/or the clock's parent. It is up to the caller to insure + * that the prepare_mutex is held across this call. Returns the + * calculated rate. Optional, but recommended - if this op is not + * set then clock rate will be initialized to 0. + * + * @round_rate XXX FIXME + * + * @get_parent Return the parent of this clock; for clocks with multiple + * possible parents, query the hardware for the current parent. + * Clocks with fixed parents should still implement this and + * return something like struct clk *fixed_parent from their + * respective struct clk_hw_* data. Currently called only when + * the clock is initialized. Clocks that do not implement this + * will have their parent set to NULL. + * + * @set_parent Change the input source of this clock; for clocks with multiple + * possible parents specify a new parent by passing in a struct + * clk *parent. Returns 0 on success, -EERROR otherwise. + * + * @set_rate Change the rate of this clock. If this callback returns + * CLK_PARENT_SET_RATE, the rate change will be propagated to the + * parent clock (which may propagate again if the parent clock + * also sets this flag). The requested rate of the parent is + * passed back from the callback in the second 'unsigned long *' + * argument. Note that it is up to the hardware clock's set_rate + * implementation to insure that clocks do not run out of spec + * when propgating the call to set_rate up to the parent. One way + * to do this is to gate the clock (via clk_disable and/or + * clk_unprepare) before calling clk_set_rate, then ungating it + * afterward. If your clock also has the CLK_GATE_SET_RATE flag + * set then this will insure safety. Returns 0 on success, + * -EERROR otherwise. + * + * The clk_enable/clk_disable and clk_prepare/clk_unprepare pairs allow + * implementations to split any work between atomic (enable) and sleepable + * (prepare) contexts. If enabling a clock requires code that might sleep, + * this must be done in clk_prepare. Clock enable code that will never be + * called in a sleepable context may be implement in clk_enable. + * + * Typically, drivers will call clk_prepare when a clock may be needed later + * (eg. when a device is opened), and clk_enable when the clock is actually + * required (eg. from an interrupt). Note that clk_prepare MUST have been + * called before clk_enable. */ -struct clk; +struct clk_hw_ops { + int (*prepare)(struct clk *clk); + void (*unprepare)(struct clk *clk); + int (*enable)(struct clk *clk); + void (*disable)(struct clk *clk); + unsigned long (*recalc_rate)(struct clk *clk); + long (*round_rate)(struct clk *clk, unsigned long, + unsigned long *); + int (*set_parent)(struct clk *clk, struct clk *); + struct clk * (*get_parent)(struct clk *clk); + int (*set_rate)(struct clk *clk, unsigned long); +}; + +/** + * clk_init - initialize the data structures in a struct clk + * @dev: device initializing this clk, placeholder for now + * @clk: clk being initialized + * + * Initializes the lists in struct clk, queries the hardware for the + * parent and rate and sets them both. Adds the clk to the sysfs tree + * topology. + * + * Caller must populate .name, .flags and .ops before calling clk_init. + */ +void clk_init(struct device *dev, struct clk *clk); + +#endif /* !CONFIG_GENERIC_CLK */ /** * clk_get - lookup and obtain a reference to a clock producer. @@ -107,9 +226,16 @@ static inline void clk_unprepare(struct clk *clk) } #endif +int __clk_enable(struct clk *clk); +void __clk_disable(struct clk *clk); +int __clk_prepare(struct clk *clk); +void __clk_unprepare(struct clk *clk); +void __clk_reparent(struct clk *clk, struct clk *new_parent); + /** * clk_get_rate - obtain the current clock rate (in Hz) for a clock source. * This is only valid once the clock source has been enabled. + * Returns zero if the clock rate is unknown. * @clk: clock source */ unsigned long clk_get_rate(struct clk *clk);
The common clk framework is an attempt to define a common struct clk which can be used by most platforms, and an API that drivers can always use safely for managing clks. The net result is consolidation of many different struct clk definitions and platform-specific clk framework implementations. This patch introduces the common struct clk, struct clk_hw_ops and definitions for the long-standing clk API in include/clk/clk.h. Platforms may define their own hardware-specific clk structure, so long as it wraps an instance of the common struct clk which is passed to clk_init at initialization time. From this point on all of the usual clk APIs should be supported, so long as the platform supports them through hardware-specific callbacks in struct clk_hw_ops. See Documentation/clk.h for more details. This patch is based on the work of Jeremy Kerr, which in turn was based on the work of Ben Herrenschmidt. Big thanks to both of them for kickstarting this effort. Signed-off-by: Mike Turquette <mturquette@linaro.org> --- drivers/clk/Kconfig | 4 + drivers/clk/Makefile | 1 + drivers/clk/clk.c | 538 ++++++++++++++++++++++++++++++++++++++++++++++++++ include/linux/clk.h | 134 ++++++++++++- 4 files changed, 673 insertions(+), 4 deletions(-) create mode 100644 drivers/clk/clk.c