(cc'ing Richard Woodruff) Hello Russell, On Mon, 9 Feb 2009, Russell King - ARM Linux wrote: > On Thu, Jan 29, 2009 at 10:06:08PM +0000, Russell King - ARM Linux wrote: > > @@ -780,7 +780,7 @@ int omap2_clk_set_parent(struct clk *clk, struct clk *new_parent) > > if (clk->usecount > 0) > > _omap2_clk_enable(clk); > > > > - clk->parent = new_parent; > > + clk_reparent(clk, new_parent); > > While looking at the DPLL patches, I've realised that omap2_clk_set_parent() > is buggy, as are any other places which reparent the clock (thankfully > the only other place is in the initialisation code where it doesn't > matter.) > > Consider what happens when a clock is enabled - we walk up the tree > enabling all parents. If we then change the clock's parent, and > then disable the child, we will again walk up the tree, but since > we've reparented it, it will be a different clock tree. The result > is that the ancestors clock usage counts, and therefore their enable > status, will end up getting screwed up. Agreed. > This brings up a question: what we currently do here is: > > - disable the child > - program clksel > - enable the child > - change child->parent > > If we add in the parent handling, there are two possibilities: > > - disable the child > - enable the new parent tree > - program clksel > - change child->parent > - disable the old parent tree > - enable the child > > OR > > - disable the child and the old parent tree > - program clksel > - change child->parent > - enable the new parent tree and the child > > (note those 'and's have implied ordering). > > Is there anything which dictates one approach over the other? > Obviously the latter approach results in something smaller and > cleaner, but might not be technically correct. I don't know of any hardware reason to prefer one approach over the other, but Richard might know better. - Paul -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
