Re: [PATCH v4 3/6] clk: introduce the common clock framework

From: Turquette, Mike
Date: Wed Dec 14 2011 - 14:07:33 EST


On Tue, Dec 13, 2011 at 8:52 PM, Ryan Mallon <rmallon@xxxxxxxxx> wrote:
> On 14/12/11 14:53, 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);
>> +
>> +     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

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

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

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

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

>> +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).

>> +{
>> +     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.

>> +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;
>
>> +
>> +     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 *.

>> +void __clk_reparent(struct clk *clk, struct clk *new_parent)
>> +{
>
>> +     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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/