On Tue, Dec 13, 2011 at 8:52 PM, Ryan Mallon [off-list ref] wrote:

On 14/12/11 14:53, Mike Turquette wrote:

quoted

+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);
+
+ ? ? if (clk->ops->unprepare)
+ ? ? ? ? ? ? clk->ops->unprepare(clk);
+
+ ? ? __clk_unprepare(clk->parent);
+}

I think you can rewrite this to get rid of the recursion as below:

? ? ? ?while (clk) {
? ? ? ? ? ? ? ?if (WARN_ON(clk->prepare_count == 0))
? ? ? ? ? ? ? ? ? ? ? ?return;

? ? ? ? ? ? ? ?if (--clk->prepare_count > 0)
? ? ? ? ? ? ? ? ? ? ? ?return;

? ? ? ? ? ? ? ?WARN_ON(clk->enable_count > 0)

? ? ? ? ? ? ? ?if (clk->ops->unprepare)
? ? ? ? ? ? ? ? ? ? ? ?clk->ops->unprepare(clk);

? ? ? ? ? ? ? ?clk = clk->parent;
? ? ? ?}

Looks good.  I'll roll into next set.

Also, should this function be static?

No, since platform clk code will occasionally be forced to call
clk_(un)prepare while the prepare_lock mutex is held.  For a valid
example see:
http://git.linaro.org/gitweb?p=people/mturquette/linux.git;a=blob;f=arch/arm/mach-omap2/dpll3xxx.c;h=b2412574b63829944593c1a7a6eda5fa4350686f;hb=738bde65918ae1ac743b1498801da0b860e2ee32#l461

quoted

+void clk_unprepare(struct clk *clk)
+{
+ ? ? mutex_lock(&prepare_lock);
+ ? ? __clk_unprepare(clk);
+ ? ? mutex_unlock(&prepare_lock);
+}
+EXPORT_SYMBOL_GPL(clk_unprepare);
+
+int __clk_prepare(struct clk *clk)
+{
+ ? ? int ret = 0;
+
+ ? ? if (!clk)
+ ? ? ? ? ? ? return 0;
+
+ ? ? if (clk->prepare_count == 0) {
+ ? ? ? ? ? ? ret = __clk_prepare(clk->parent);
+ ? ? ? ? ? ? if (ret)
+ ? ? ? ? ? ? ? ? ? ? return ret;
+
+ ? ? ? ? ? ? if (clk->ops->prepare) {
+ ? ? ? ? ? ? ? ? ? ? ret = clk->ops->prepare(clk);
+ ? ? ? ? ? ? ? ? ? ? if (ret) {
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? __clk_unprepare(clk->parent);
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? return ret;
+ ? ? ? ? ? ? ? ? ? ? }
+ ? ? ? ? ? ? }
+ ? ? }
+
+ ? ? clk->prepare_count++;
+
+ ? ? return 0;
+}

This is unfortunately a bit tricky to remove the recursion from without
keeping a local stack of the clocks leading up to first unprepared
client :-/.

Again, should this be static? What outside this file needs to
prepare/unprepare clocks without the lock held?

Same as above.

quoted

+int clk_prepare(struct clk *clk)
+{
+ ? ? int ret;
+
+ ? ? mutex_lock(&prepare_lock);
+ ? ? ret = __clk_prepare(clk);
+ ? ? mutex_unlock(&prepare_lock);
+
+ ? ? return ret;
+}
+EXPORT_SYMBOL_GPL(clk_prepare);
+
+void __clk_disable(struct clk *clk)
+{
+ ? ? if (!clk)
+ ? ? ? ? ? ? return;
+
+ ? ? if (WARN_ON(clk->enable_count == 0))
+ ? ? ? ? ? ? return;
+
+ ? ? if (--clk->enable_count > 0)
+ ? ? ? ? ? ? return;
+
+ ? ? if (clk->ops->disable)
+ ? ? ? ? ? ? clk->ops->disable(clk);
+
+ ? ? if (clk->parent)
+ ? ? ? ? ? ? __clk_disable(clk->parent);

Easy to get rid of the recursion here. Also should be static?

Yes the enable/disable should be static.  I originally made them
non-static when I converted the prepare/unprepare set, but of course
it's possible to call these with the prepare_lock mutex held so I'll
fix this up in the next set.

quoted

+long clk_round_rate(struct clk *clk, unsigned long rate)
+{
+ ? ? if (clk && clk->ops->round_rate)
+ ? ? ? ? ? ? return clk->ops->round_rate(clk, rate, NULL);
+
+ ? ? return rate;
+}
+EXPORT_SYMBOL_GPL(clk_round_rate);

If the clock doesn't provide a round rate function then shouldn't we
return an error to the caller? Telling the caller that the rate is
perfect might lead to odd driver bugs?

Yes this code should so something better.  I've been focused mostly on
the "write" aspects of the clk API (set_rate, set_parent,
enable/disable and prepare/unprepare) and less on the "read" aspects
of the clk API (get_rate, get_parent, round_rate, etc).  I'll give
this a closer look for the next set.

quoted

+/**
+ * DOC: Using the CLK_PARENT_SET_RATE flag
+ *
+ * __clk_set_rate changes the child's rate before the parent's to more
+ * easily handle failure conditions.
+ *
+ * This means clk might run out of spec for a short time if its rate is
+ * increased before the parent's rate is updated.
+ *
+ * To prevent this consider setting the CLK_GATE_SET_RATE flag on any
+ * clk where you also set the CLK_PARENT_SET_RATE flag
+ */

Is this standard kerneldoc format?

It is:
http://git.linaro.org/gitweb?p=people/mturquette/linux.git;a=blob;f=Documentation/kernel-doc-nano-HOWTO.txt;h=3d8a97747f7731c801ca7d3a1483858feeb76b6c;hb=refs/heads/v3.2-rc5-clkv4-omap#l298

quoted

+struct clk *__clk_set_rate(struct clk *clk, unsigned long rate)

static?

I'll make it static.  I don't think any platform code needs to call
this (at least I hope not).

quoted

+{
+ ? ? struct clk *fail_clk = NULL;
+ ? ? int ret = 0;
+ ? ? unsigned long old_rate = clk->rate;
+ ? ? unsigned long new_rate;
+ ? ? unsigned long parent_old_rate;
+ ? ? unsigned long parent_new_rate = 0;
+ ? ? struct clk *child;
+ ? ? struct hlist_node *tmp;
+
+ ? ? /* bail early if we can't change rate while clk is enabled */
+ ? ? if ((clk->flags & CLK_GATE_SET_RATE) && clk->enable_count)
+ ? ? ? ? ? ? return clk;
+
+ ? ? /* find the new rate and see if parent rate should change too */
+ ? ? WARN_ON(!clk->ops->round_rate);
+
+ ? ? new_rate = clk->ops->round_rate(clk, rate, &parent_new_rate);
+
+ ? ? /* FIXME propagate pre-rate change notification here */
+ ? ? /* XXX note that pre-rate change notifications will stack */
+
+ ? ? /* change the rate of this clk */
+ ? ? if (clk->ops->set_rate)
+ ? ? ? ? ? ? ret = clk->ops->set_rate(clk, new_rate);
+
+ ? ? if (ret)
+ ? ? ? ? ? ? return clk;

Is there are reason to write it this way and not:

? ? ? ?if (clk->ops->set_rate) {
? ? ? ? ? ? ? ?ret = clk->ops->set_rate(clk, new_rate);
? ? ? ? ? ? ? ?if (ret)
? ? ? ? ? ? ? ? ? ? ? ?return clk;
? ? ? ?}

If !clk->ops->set_rate then ret is always zero right? Note, making this
change means that you don't need to init ret to zero at the top of this
function.

Looks good.  Will fix in the next set.

quoted

+int clk_set_rate(struct clk *clk, unsigned long rate)
+{
+ ? ? struct clk *fail_clk;
+ ? ? int ret = 0;
+
+ ? ? /* prevent racing with updates to the clock topology */
+ ? ? mutex_lock(&prepare_lock);
+
+ ? ? /* bail early if nothing to do */
+ ? ? if (rate == clk->rate)
+ ? ? ? ? ? ? goto out;

quoted

+
+ ? ? fail_clk = __clk_set_rate(clk, rate);
+ ? ? if (fail_clk) {
+ ? ? ? ? ? ? pr_warn("%s: failed to set %s rate\n", __func__,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? fail_clk->name);
+ ? ? ? ? ? ? /* FIXME propagate rate change abort notification here */
+ ? ? ? ? ? ? ret = -EIO;

Why does __clk_set_rate return a struct clk if you don't do anything
with it? You can move the pr_warn into __clk_set_rate and have it return
a proper errno value instead so that you get a reason why it failed to
set the rate.

The next patch in the series uses fail_clk to propagate
ABORT_RATE_CHANGE notifications to any drivers that have subscribed to
them.  The FIXME comment hints at that but doesn't make it clear.  The
idea is that the PRE_RATE_CHANGE notifiers will be noisy and stack up
(potentially), but I only want to propagate the POST_RATE_CHANGE and
ABORT_RATE_CHANGE notifications once for any single call to
clk_set_rate, which is why __clk_set_rate returns a struct clk *.

quoted

+void __clk_reparent(struct clk *clk, struct clk *new_parent)
+{

quoted

+ ? ? if (!clk || !new_parent)
+ ? ? ? ? ? ? return;

clk_reparent already checks for !clk, shouldn't it also check for
!new_parent and remove the check from here?

I need to take another look at this.  new_parent can be NULL if we
think it is plausible for a parented clk to suddenly become a root clk
(where clk->parent == NULL).

Thanks for reviewing,
Mike