On Mon, Nov 21, 2011 at 05:40:45PM -0800, Mike Turquette wrote:
[...]

+/**
+ * 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

Eh, this is how flag CLK_GATE_SET_RATE is born?  Does that mean to make
the call succeed all the clocks on the propagating path need to be gated
before clk_set_rate gets called?

+ */
+struct clk *__clk_set_rate(struct clk *clk, unsigned long rate)
+{
+	struct clk *fail_clk = NULL;
+	int ret;
+	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 */
+	ret = clk->ops->set_rate(clk, new_rate);

Yes, right here, the clock gets set to some unexpected rate, since the
parent clock has not been set to the target rate yet at this point.

+	if (ret)
+		return clk;
+
+	/*
+	 * change the rate of the parent clk if necessary
+	 *
+	 * hitting the nested 'if' path implies we have hit a .set_rate
+	 * failure somewhere upstream while propagating __clk_set_rate
+	 * up the clk tree.  roll back the clk rates one by one and
+	 * return the pointer to the clk that failed.  clk_set_rate will
+	 * use the pointer to propagate a rate-change abort notifier
+	 * from the "highest" point.
+	 */
+	if ((clk->flags & CLK_PARENT_SET_RATE) && parent_new_rate) {
+		parent_old_rate = clk->parent->rate;
+		fail_clk = __clk_set_rate(clk->parent, parent_new_rate);
+
+		/* roll back changes if parent rate change failed */
+		if (fail_clk) {
+			pr_warn("%s: failed to set parent %s rate to %lu\n",
+					__func__, fail_clk->name,
+					parent_new_rate);
+			clk->ops->set_rate(clk, old_rate);
+		}
+		return fail_clk;
+	}
+
+	/*
+	 * set clk's rate & recalculate the rates of clk's children
+	 *
+	 * hitting this path implies we have successfully finished
+	 * propagating recursive calls to __clk_set_rate up the clk tree
+	 * (if necessary) and it is safe to propagate clk_recalc_rates and
+	 * post-rate change notifiers down the clk tree from this point.
+	 */
+	clk->rate = new_rate;
+	/* FIXME propagate post-rate change notifier for only this clk */
+	hlist_for_each_entry(child, tmp, &clk->children, child_node)
+		clk_recalc_rates(child);
+
+	return fail_clk;
+}

[...]

Eh, why adding a duplication?

+#include <linux/errno.h>
+
 struct device;
 
+struct clk;

Do you really need this?

[...]

+struct clk_hw_ops {
+	int		(*prepare)(struct clk *clk);
+	void		(*unprepare)(struct clk *clk);
+	int		(*enable)(struct clk *clk);
+	void		(*disable)(struct clk *clk);
+	unsigned long	(*recalc_rate)(struct clk *clk);
+	long		(*round_rate)(struct clk *clk, unsigned long,

The return type seems interesting.  If we intend to return a rate, it
should be 'unsigned long' rather than 'long'.  I'm just curious about
the possible reason behind that.

+					unsigned long *);
+	int		(*set_parent)(struct clk *clk, struct clk *);
+	struct clk *	(*get_parent)(struct clk *clk);
+	int		(*set_rate)(struct clk *clk, unsigned long);

Nit: would it be reasonable to put set_rate after round_rate to group
*_rate functions together?

Do we really need to export all these common clk internal functions?

Regards,
Shawn

 /**
  * clk_get_rate - obtain the current clock rate (in Hz) for a clock source.
  *		  This is only valid once the clock source has been enabled.
+ *		  Returns zero if the clock rate is unknown.
  * @clk: clock source
  */
 unsigned long clk_get_rate(struct clk *clk);
-- 
1.7.4.1