[lkml]   [2009]   [Feb]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: [PATCH E 10/14] OMAP clock: support "dry run" rate and parent changes
    Hello Russell,

    On Sun, 8 Feb 2009, Russell King - ARM Linux wrote:

    > On Wed, Jan 28, 2009 at 12:27:56PM -0700, Paul Walmsley wrote:
    > > For upcoming notifier support, modify the rate recalculation code to
    > > take parent rate and rate storage parameters. The goal here is to
    > > allow the clock code to determine what the clock's rate would be after
    > > a parent change or a rate change, without actually changing the
    > > hardware registers. This is used by the upcoming notifier patches to
    > > pass a clock's current and planned rates to the notifier callbacks.
    > NAK. I'm not sure whether you've realised, but this is exactly what the
    > 'round_rate' method is supposed to be doing. It provides a method by
    > which you can find out what the clock rate would be if you asked for
    > 'set_rate' to set the hardware to the requested rate.

    There's been some miscommunication about the purpose of this patch - most
    likely my fault for not grouping this patch with the clock notifier patch
    set, which has not yet been submitted for upstream merging. Most of this
    patch is not needed until notifiers are implemented, so I'd propose that
    we defer consideration of most of this patch until the clock notifiers are
    submitted. What would be useful, though, is to split out the
    RECALC_ON_ENABLE portion of this patch and merge that. That is a bug fix,
    and I should have sent that as a separate patch.

    For the sake of the current discussion though, here's what this patch was
    intended to accomplish. We have some patches under development that
    implement clock rate change notifiers, allowing drivers to register for
    pre- and post-notification on clock rate changes. The notifier callbacks
    are passed the old clock rate and the new clock rate. Since a rate change
    on a clock in the middle of the clock tree (e.g., dpll3_m2_ck) can affect
    all clocks downstream, the clock code must compute the new rate for not
    only dpll3_m2_ck, but also all downstream clocks, before clk->rate is
    updated and the hardware registers are actually changed.

    This presents a problem with the current code. Consider the
    pre-notification case, in which the clock code needs to compute the target
    clock rate without actually updating clk->rate. But the round_rate()
    functions assume that the parent clock's current rate is what should be
    used. If the parent clock's rate would also change as part of the
    clk_set_rate(), the target rate computation will return an incorrect rate.

    So this patch allows temporary rates to be computed for all of the clocks
    that would be affected by the rate or parent change, so the correct target
    rates can be passed to each clock's notifier (assuming one exists for the

    N.B., this temporary rate computation is also the motivation behind the
    recursive propagate_rate() calls that we discussed earlier.

    It also may turn out that it's unnecessary to pass the new rate to the
    notifiers, in which case most of this code won't need to reappear.

    > A far better way to approach this would be to split the set_rate/recalc
    > functionality into two parts:
    > 1. a method which returns the both the new clock rate and the hardware
    > programming information
    > 2. a method to commit the hardware programming information to the registers
    > (1) can be used for implementing clk_round_rate() (which is totally lacking
    > in OMAP2+, yet is implemented in OMAP1), clk_set_rate(), clk_set_parent()
    > etc.

    The OMAP2/3 clock code does implement clk_round_rate() - most commonly via
    mach-omap2/clock.c:omap2_clksel_round_rate(). There are a few special
    cases which use other round_rate() functions.

    > (2) can be used when it's required to actually reprogram the hardware.
    > So, rather than the current situation where we have recalc, round_rate
    > and set_rate methods, we have calc_rate() and commit() methods instead
    > and have the core clock code sort out calling these.

    I haven't yet spent much time thinking about this new arrangement -- it
    seems to be orthogonal to the problem that this patch is intended to solve
    -- but it might make sense for a different purpose. Many of our
    set_rate() functions wind up calling round_rate() first to determine
    whether the passed-in rate is valid and to return the hardware programming
    data from the clksel structures. This extra call to round_rate() just
    wastes cycles. So this new layout might be more efficient, although it
    appears that calc_rate() is called twice in the new arrangement as well.

    I appreciate the ongoing technical review,

    - Paul

     \ /
      Last update: 2009-02-11 08:55    [W:0.023 / U:10.064 seconds]
    ©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site