[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Mon, 2 Feb 2009 00:57:39 -0700 (MST)
From: Paul Walmsley <paul@...an.com>
To: Russell King - ARM Linux <linux@....linux.org.uk>
cc: linux-arm-kernel@...ts.arm.linux.org.uk,
linux-kernel@...r.kernel.org, linux-omap@...r.kernel.org,
Tony Lindgren <tony@...mide.com>
Subject: Re: [PATCH E 11/14] OMAP clock: track child clocks
Hello Russell,
On Thu, 29 Jan 2009, Russell King - ARM Linux wrote:
> On Wed, Jan 28, 2009 at 12:27:59PM -0700, Paul Walmsley wrote:
> > +static int omap_clk_for_each_child(struct clk *clk, unsigned long parent_rate,
> > + u8 rate_storage, int (*cb)(struct clk *, unsigned long, u8))
> > +{
> > + struct clk_child *child;
> > + int ret;
> > +
> > + list_for_each_entry(child, &clk->children, node) {
> > + ret = (*cb)(child->clk, parent_rate, rate_storage);
> > + if (ret)
> > + break;
> > + }
> > +
> > + return ret;
> > +}
>
> > +static int _do_propagate_rate(struct clk *clk, unsigned long parent_rate,
> > + u8 rate_storage)
> > +{
> > + if (clk->recalc)
> > + clk->recalc(clk, parent_rate, rate_storage);
> > + if (omap_clk_has_children(clk))
> > + propagate_rate(clk, rate_storage);
> > + return 0;
> > +}
>
> > /* Propagate rate to children */
> > void propagate_rate(struct clk *tclk, u8 rate_storage)
> > {
> > unsigned long parent_rate = 0;
> >
> > if (tclk == NULL || IS_ERR(tclk))
> > return;
> >
> > + if (rate_storage == CURRENT_RATE)
> > + parent_rate = tclk->rate;
> > + else if (rate_storage == TEMP_RATE)
> > + parent_rate = tclk->temp_rate;
> >
> > + omap_clk_for_each_child(tclk, parent_rate, rate_storage,
> > + _do_propagate_rate);
> > }
>
> This worries me. Calling this puts onto the stack:
>
> - a frame for propagate_rate()
> - a frame for omap_clk_for_each_child
> - a frame for _do_propagate_rate
>
> for every level of children. How close we get to overflowing the kernels
> depends on how much each of those functions puts on the kernel stack.
> However, since this is recursive, minimising the number of stack frames
> is a good idea.
>
> That's why I have in my patch:
>
> [ARM] omap: move propagate_rate() calls into generic omap clock code
>
> I've arranged for there to be the minimum of function nesting here.
> I suggest keeping this.
Those two new function calls are probaby not necessary for rate
propagation, so, keeping the original single-function recursion should be
fine. The original motivation behind using them was to share the clock
tree traversal code with the clock notifier patches, e.g.,
http://www.mail-archive.com/linux-omap@vger.kernel.org/msg08068.html
Would you like a patch to remove _do_propagate_rate() and
omap_clk_for_each_child()?
- Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists