On Fri, Dec 16, 2011 at 04:45:48PM -0800, Turquette, Mike wrote: > On Wed, Dec 14, 2011 at 5:18 AM, Thomas Gleixner <t...@linutronix.de> wrote: > > On Tue, 13 Dec 2011, Mike Turquette wrote: > >> +void __clk_unprepare(struct clk *clk) > >> +{ > >> + if (!clk) > >> + return; > >> + > >> + if (WARN_ON(clk->prepare_count == 0)) > >> + return; > >> + > >> + if (--clk->prepare_count > 0) > >> + return; > >> + > >> + WARN_ON(clk->enable_count > 0); > > > > So this leaves the clock enable count set. I'm a bit wary about > > that. Shouldn't it either return (including bumping the prepare_count > > again) or call clk_disable() ? > > I've hit this in my port of OMAP. It comes from this simple situation: > > driver 1 (adapted for clk_prepare/clk_unprepare): > clk_prepare(clk); > clk_enable(clk); > > ... > > driver2 (not adapted for clk_prepare/clk_unprepare): > clk_enable(clk); > > ... > > driver1: > clk_disable(clk); > clk_unprepare(clk); > > In such a case we don't want to bump the prepare_count, since the > offending driver2 won't decrement that count. Also we don't want to > shut down that clk since driver2 is using it. > > Returning after the WARN is maybe OK, but the point of this check is > really to find code which hasn't yet been made prepare-aware, and the > current implementation is noisy enough about it. > > >> +/** > >> + * 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. > > > > Holding the prepare lock in the caller will deadlock. So it's not a > > real good idea to document it as an option :) > > Oops. That comment is left over from before clk_get_parent held the > lock. Will remove. > > > > >> + */ > >> +struct clk *clk_get_parent(struct clk *clk) > >> +{ > >> + struct clk *parent; > >> + > >> + if (!clk) > >> + return NULL; > >> + > >> + mutex_lock(&prepare_lock); > > > >> +/** > >> + * 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 > > > > I'm not too happy about this construct. That leaves struct clk and its > > members exposed to the world instead of making it a real opaque > > cookie. I know from my own painful experience, that this will lead to > > random fiddling in that data structure in drivers and arch code just > > because the core code has a shortcoming. > > > > Why can't we make struct clk a real cookie and confine the data > > structure to the core code ? > > > > That would change the init call to something like: > > > > struct clk *clk_init(struct device *dev, const struct clk_hw *hw, > > struct clk *parent) > > > > And have: > > struct clk_hw { > > struct clk_hw_ops *ops; > > const char *name; > > unsigned long flags; > > }; > > > > Implementers can do: > > struct my_clk_hw { > > struct clk_hw hw; > > mydata; > > }; > > > > And then change the clk ops callbacks to take struct clk_hw * as an > > argument. We have to define static clocks before we adopt DT binding. If clk is opaque and allocate memory in clk core, it'll make hard to define static clocks. And register/init will pass a long parameter list. > > > > So the core code can allocate the clk data structure and you return a > > real opaque cookie. You do the same thing for the basic gated clock in > > one of the next patches, so doing it for struct clk itself is just > > consequent. > > The opaque cookie would work if platform code never needs any > information outside of the data a single clk_hw_whatever provides. > Unfortunately hardware doesn't make things that easy on us and the > operations we do for a single clk are affected by the state of other > clks. > > Here is an example I've been using a lot lately: on OMAP we have two > clock inputs to our PLLs: a bypass clk and reference clk (actually we > can have more than this). When changing the PLL rate both clks must > be enabled, regardless of which clk ends up driving the PLL. To > illustrate, the PLL may be driven by clk_ref at 400MHz, and then we > call clk_set_rate(pll_clk, 196000000); which will also leave it > clocked by clk_ref but at 196MHz. For this to happen however, the > clk_bypass had to be enabled during the rate change operation. Here > is the relevant .set_rate implementation: > http://git.linaro.org/gitweb?p=people/mturquette/linux.git;a=blob;f=arch/arm/mach-omap2/dpll3xxx.c;h=b2412574b63829944593c1a7a6eda5fa4350686f;hb=738bde65918ae1ac743b1498801da0b860e2ee32#l461 > > In order for the OMAP platform code to do this, we have to have two > things: firstly we need the struct clk so that we can call > clk_enable/disable and __clk_prepare/unprepare on the ref and bypass > clks from within .set_rate. The second thing is that we need > __clk_prepare and __clk_unprepare to be visible by this code so that > we don't nest the prepare_lock mutex. > > Is there a good way to solve such problems if we hide struct clk from > the platform code/clk driver implementation? You can add a function clk_to_hw to convert struct clk to struct clk_hw. It's the way I did with the patch you didn't expose struct clk.
clock driver gate2b needs to access clk_hw. clk_gate2b_set_val can set value for enable and disable at runtime. At opaque time, it have to get clk_hw first and then get struct clk_gate2b. > > Also note that if top-level clk APIs are going to be holding > prepare_lock (clk_get_rate, clk_get_parent, etc) then we can't call > these APIs to get the info we want from within the platform code. For > example, today most .recalc_rate implementations reference > clk->parent->rate and apply it to some divider or something. To get > around having an opaque cookie .recalc_rate would have to have > parent->rate passed into it since the implementation cannot call > clk_get_rate(clk_get_parent(clk)) due to not having 'clk' in the first > place, and also because of mutex nesting. why do we need lock in clk_get_rate? > > I agree that by not using an opaque cookie we open ourselves up to the > possibility of badness, but we'll just have to catch it in code > review. > > > > >> + * clk_init. A key assumption in the call to .get_parent is that clks > >> + * are being initialized in-order. > > > > We should not require that, see below. > > > >> + */ > > > >> +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); > >> + > >> + /* > >> + * .get_parent is mandatory for populating .parent for clks with > >> + * multiple possible parents. For clks with a fixed parent > >> + * .get_parent is not mandatory and .parent can be statically > >> + * initialized > >> + */ > >> + if (clk->ops->get_parent) > >> + clk->parent = clk->ops->get_parent(clk); > >> + > >> + /* clks without a parent are considered root clks */ > > > > I'd rather prefer that root clocks are explicitely marked with a flag > > instead of relying on clk->parent being NULL. > > I originally had CLK_IS_ROOT flag but removed it after thinking that > some clks might be a root sometimes, or a child at other times and I > had considered the flags to be constant. I didn't see part-time root clock in IMX. > > If the flags aren't going to be constant and can change at run-time > then I can move this back to a flag. Also if there are no use cases > where a clk can change from a root to a child then again this can > safely go into flags. Thoughts? > > > > >> + if (clk->parent) > >> + hlist_add_head(&clk->child_node, > >> + &clk->parent->children); > >> + else > >> + hlist_add_head(&clk->child_node, &clk_root_list); > > > > You could put clocks which have no parent and are not marked as root > > clock onto a separate list clk_orphaned or such. You might need this > > for handling clocks which are disconnected from any parent runtime as > > well. > > Agreed, that is a more well-defined approach. > > > And to avoid the whole in-order initialization issue, you could change > > clk_init to: > > > > struct clk *clk_init(struct device *dev, const struct clk_hw *hw, > > unsigned long flags, const char *parent_name) > > > > Then you can lookup the requested parent by name. If that fails, you > > put the clock into the orphaned list. When a new clock is initialized, > > then you walk the orphaned list and check whether something is waiting > > for that clock. > > > > To handle multi-parent clocks you need to go one step further and add: > > > > struct clk_hw { > > struct clk_hw_ops *ops; > > const char *name; > > const char **possible_parents; > > }; > > > > You also require a get_parent_idx() function in clk_hw_ops. So when > > clk_init() is called with parent_name = NULL and get_parent_idx() is > > implemented, then you call it and the clock returns you the index of > > the possible_parents array. If that parent clock is not yet > > registered, you store the requested name and do the lookup when new > > clocks are registered. > > This approach ends up having something like O(n^2) (similar to > discussion around driver re-probing). Split it to two functions? clk_add can add clks in any order, and clk_tree_init call .get_parent (if possible) create the tree. Adding clk after clk_tree_init must be in strong order. > > However, I agree that forcing folks to register/init clks in-order is > asking for trouble. I also think that for groups of well defined clks > (SoC clks, or clks in the same device) that are statically initialized > we should allow for having clk->parent populated statically and leave > it at that (mux clks will still need to run clk->get_parent() in > clk_init of course, since we can't trust the bootloader). right. > > > > > That has also the advantage, that you can validate parent settings > > upfront and for setting the parent during runtime, you don't have to > > acquire a pointer to the parent clock. It's enough to call > > clk_set_named_parent(). That'll cause clk core to call clkdev functions. But looks fine. Thanks Richard > > Right, but the usefulness of this last point hinges on whether or not > we want to hide struct clk from the rest of the world. I still think > that doing so will end up being a limitation that platforms end up > having to hack around. I'd like a lot more input from the folks > porting their platforms to this code on that point, as it is a > critical design element. > > Thanks for reviewing, > Mike > > > > > Thoughts ? > > > > tglx > > > > > > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-ker...@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html