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

On Fri, Feb 13, 2009 at 12:01:37AM -0700, Paul Walmsley wrote:
> (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.

I'll need an answer on this before I can commit the updated bypass clock
support patch.

However, looking a little deeper, there's more issues in the reparenting
area.  I don't think this code has been tested at all...  In
_omap2_clksel_get_src_field, there is this:

	for (clkr = clks->rates; clkr->div; clkr++) {
		if (clkr->flags & (cpu_mask | DEFAULT_RATE))
			break; /* Found the default rate for this platform */
	}

which is bogus - it will find the first entry which is _either_ marked
as a default rate _or_ is supported by the SoC.  This means (for
instance) that:

static const struct clksel_rate core_l3_core_rates[] = {
        { .div = 1, .val = 1, .flags = RATE_IN_24XX },
        { .div = 2, .val = 2, .flags = RATE_IN_242X },
        { .div = 4, .val = 4, .flags = RATE_IN_24XX | DEFAULT_RATE },

will give us divisor 1 rather than presumably the one we want, that being
divisor 4.  I think the test above should be:

	for (clkr = clks->rates; clkr->div; clkr++) {
		if (clkr->flags & cpu_mask &&
		    clkr->flags & DEFAULT_RATE)
			break; /* Found the default rate for this platform */
	}

so we find an entry which is supported _and_ is the default for the SoC.

There's also a second issue - the comments before omap2_divisor_to_clksel()
indicate that this function returns 0xffffffff on error.  Unfortunately,
this is not so, it actually returns zero on error.  Moreover, we test
the result of the function against ~0, so we'll never deal with the error
case.  This really should be fixed so that we return the right value for
the error case.  (Further comments on this in a follow up.)

So, below is a patch which fixes both of these issues.

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

Commit Message

Comments

Patch