Re: [PATCH E 11/14] OMAP clock: track child clocks

[Russell King - ARM Linux]

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.

So, this has revealed a logical bug here.  We need to walk the
parent tree before and after the switch to ensure that things stay
sane.

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/