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;
> +}
[...]
> diff --git a/include/linux/clk.h b/include/linux/clk.h
> index 7213b52..3b0eb3f 100644
> --- a/include/linux/clk.h
> +++ b/include/linux/clk.h
> @@ -3,6 +3,8 @@
> *
> * Copyright (C) 2004 ARM Limited.
> * Written by Deep Blue Solutions Limited.
> + * Copyright (c) 2010-2011 Jeremy Kerr <jeremy.kerr@xxxxxxxxxxxxx>
> + * Copyright (C) 2011 Linaro Ltd <mturquette@xxxxxxxxxx>
> *
> * This program is free software; you can redistribute it and/or modify
> * it under the terms of the GNU General Public License version 2 as
> @@ -13,17 +15,134 @@
>
> #include <linux/kernel.h>
>
> +#include <linux/kernel.h>
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?
> +};
> +
> +/**
> + * 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 .name, .flags and .ops before calling clk_init.
> + */
> +void clk_init(struct device *dev, struct clk *clk);
> +
> +#endif /* !CONFIG_GENERIC_CLK */
>
> /**
> * clk_get - lookup and obtain a reference to a clock producer.
> @@ -107,9 +226,16 @@ static inline void clk_unprepare(struct clk *clk)
> }
> #endif
>
> +int __clk_enable(struct clk *clk);
> +void __clk_disable(struct clk *clk);
> +int __clk_prepare(struct clk *clk);
> +void __clk_unprepare(struct clk *clk);
> +void __clk_reparent(struct clk *clk, struct clk *new_parent);
> +
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
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
