Date:	Wed, 4 Jan 2012 10:15:42 +0800
From:	Richard Zhao <richard.zhao@...escale.com>
To:	"Turquette, Mike" <mturquette@...com>
CC:	Thomas Gleixner <tglx@...utronix.de>, <andrew@...n.ch>,
	<linaro-dev@...ts.linaro.org>, <eric.miao@...aro.org>,
	<grant.likely@...retlab.ca>, Colin Cross <ccross@...gle.com>,
	<jeremy.kerr@...onical.com>, <linux@....linux.org.uk>,
	<sboyd@...cinc.com>, <magnus.damm@...il.com>, <dsaxena@...aro.org>,
	<linux-arm-kernel@...ts.infradead.org>, <arnd.bergmann@...aro.org>,
	<patches@...aro.org>, <linux-omap@...r.kernel.org>,
	<richard.zhao@...aro.org>, <shawn.guo@...escale.com>,
	<paul@...an.com>, <linus.walleij@...ricsson.com>,
	<broonie@...nsource.wolfsonmicro.com>,
	<linux-kernel@...r.kernel.org>, <amit.kucheria@...aro.org>,
	<skannan@...cinc.com>
Subject: Re: [PATCH v4 3/6] clk: introduce the common clock framework

On Fri, Dec 16, 2011 at 04:45:48PM -0800, Turquette, Mike wrote:
> On Wed, Dec 14, 2011 at 5:18 AM, Thomas Gleixner <tglx@...utronix.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-kernel@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

--
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