[lkml]   [2012]   [Mar]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: [PATCH v6 2/3] clk: introduce the common clock framework
    On Sun, Mar 11, 2012 at 4:34 AM, Sascha Hauer <> wrote:
    > Hi Mike,
    > I was about to give my tested-by when I decided to test the set_rate
    > function. Unfortunately this is broken for several reasons. I'll try
    > to come up with a fixup series later the day.

    I haven't tested clk_set_rate since V4, but I also haven't changed the
    code appreciably. I'll retest on my end also.

    > On Fri, Mar 09, 2012 at 11:54:23PM -0800, Mike Turquette wrote:
    >> +     /* 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->hw, rate, &parent_new_rate);
    > You don't need a WARN_ON when you derefence clk->ops->round_rate anyway.

    Agreed that the WARN_ON should not be there.

    The v6 Documentation/clk.txt states that .round_rate is mandatory for
    clocks that can adjust their rate, but I need to clarify this a bit
    more. Ideally we want to be able to call clk_set_rate on any clock
    and get a changed rate (if possible) by either adjusting that clocks
    rate direction (e.g. a PLL or an adjustable divider) or by propagating
    __clk_set_rate up the parents (assuming of course that
    CLK_SET_RATE_PARENT flag is set appropriately).

    > Also, even when the current clock does not have a set_rate function it
    > can still change its rate when the CLK_SET_RATE_PARENT is set.

    Correct. I'll clean this up and make the documentation a bit more
    verbose on when .set_rate/.round_rate/.recalc_rate are mandatory.

    >> +
    >> +     /* NOTE: pre-rate change notifications will stack */
    >> +     if (clk->notifier_count)
    >> +             ret = __clk_notify(clk, PRE_RATE_CHANGE, clk->rate, new_rate);
    >> +
    >> +     if (ret == NOTIFY_BAD)
    >> +             return clk;
    >> +
    >> +     /* speculate rate changes down the tree */
    >> +     hlist_for_each_entry(child, tmp, &clk->children, child_node) {
    >> +             ret = __clk_speculate_rates(child, new_rate);
    >> +             if (ret == NOTIFY_BAD)
    >> +                     return clk;
    >> +     }
    >> +
    >> +     /* change the rate of this clk */
    >> +     if (clk->ops->set_rate)
    >> +             ret = clk->ops->set_rate(clk->hw, new_rate);
    > I don't know the reason why you change the child clock before the parent
    > clock, but it cannot work since this clock will change its rate based on
    > the old parent rate and not the new one.

    This depends on the .round_rate implementation, which I admit to
    having lost some sleep over. A clever .round_rate will request the
    "intermediate" rate for a clock when propagating a request to change
    the parent rate later on. Take for instance the following:

    pll @ 200MHz (locked)
    parent @ 100MHz (can divide by 1 or 2; currently divider is 2)
    child @ 25MHz (can divide by 2 or 4; currently divider is 4)

    If we want child to run at 100MHz then the desirable configuration
    would be to have parent divide-by-1 and child divide-by-2. When we

    clk_set_rate(child, 100MHz);

    Its .round_rate should return 50MHz, and &parent_new_rate should be
    200MHz. So 50MHz is an "intermediate" rate, but it gets us the
    divider we want. And in fact 50MHz reflects reality because that will
    be the rate of child until the parent propagation completes and we can
    adjust parent's dividers. (this is one reason why I prefer for
    pre-rate change notifiers to stack on top of each other).

    So now that &parent_new_rate is > 0, __clk_set_rate will propagate the
    request up and parent's .round_rate will simply return 200MHz and
    leave it's own &parent_new_rate at 0. This will change from
    divide-by-2 to divide-by-1 and from this highest point in the tree we
    will propagate post-rate change notifiers downstream, as part of the
    recalc_rate tree walk.

    I have tested this with OMAP4's CPUfreq driver and I think, while
    complicated, it is a sound way to approach the problem. Maybe the API
    can be cleaned up, if you have any suggestions.


    > There are more things, as said I'll try to come up with a fixup series.
    > Sascha
    > --
    > Pengutronix e.K.                           |                             |
    > Industrial Linux Solutions                 |  |
    > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
    > Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to
    More majordomo info at
    Please read the FAQ at

     \ /
      Last update: 2012-03-11 22:29    [W:0.026 / U:0.396 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site