lkml.org 
[lkml]   [2011]   [Oct]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v2 1/7] clk: Add a generic clock infrastructure
    On 09/22/2011 03:26 PM, Mike Turquette wrote:
    > diff --git a/include/linux/clk.h b/include/linux/clk.h
    > index 1d37f42..d6ae10b 100644
    > --- a/include/linux/clk.h
    > +++ b/include/linux/clk.h
    > +#ifdef CONFIG_GENERIC_CLK
    > +
    > +struct clk_hw {
    > + struct clk *clk;
    > +};
    > +
    > +/**
    > + * struct clk_hw_ops - Callback operations for hardware clocks; these are to
    > + * be provided by the clock implementation, and will be called by drivers
    > + * through the clk_* API.
    > + *
    > + * @prepare: Prepare the clock for enabling. This must not return until
    > + * the clock is fully prepared, and it's safe to call clk_enable.
    > + * This callback is intended to allow clock implementations to
    > + * do any initialisation that may sleep. Called with
    > + * prepare_lock held.
    > + *
    > + * @unprepare: Release the clock from its prepared state. This will typically
    > + * undo any work done in the @prepare callback. Called with
    > + * prepare_lock held.
    > + *
    > + * @enable: Enable the clock atomically. This must not return until the
    > + * clock is generating a valid clock signal, usable by consumer
    > + * devices. Called with enable_lock held. This function must not
    > + * sleep.
    > + *
    > + * @disable: Disable the clock atomically. Called with enable_lock held.
    > + * This function must not sleep.
    > + *
    > + * @recalc_rate Recalculate the rate of this clock, by quering hardware
    > + * and/or the clock's parent. Called with the global clock mutex
    > + * held. Optional, but recommended - if this op is not set,
    > + * clk_get_rate will return 0.
    > + *
    > + * @get_parent Query the parent of this clock; for clocks with multiple
    > + * possible parents, query the hardware for the current
    > + * parent. Currently only called when the clock is first
    > + * registered.
    > + *
    > + * The clk_enable/clk_disable and clk_prepare/clk_unprepare pairs allow
    > + * implementations to split any work between atomic (enable) and sleepable
    > + * (prepare) contexts. If a clock requires sleeping code to be turned on, this
    > + * should be done in clk_prepare. Switching that will not sleep should be done
    > + * in clk_enable.
    > + *
    > + * Typically, drivers will call clk_prepare when a clock may be needed later
    > + * (eg. when a device is opened), and clk_enable when the clock is actually
    > + * required (eg. from an interrupt). Note that clk_prepare *must* have been
    > + * called before clk_enable.
    > */
    > +struct clk_hw_ops {
    > + int (*prepare)(struct clk_hw *);
    > + void (*unprepare)(struct clk_hw *);
    > + int (*enable)(struct clk_hw *);
    > + void (*disable)(struct clk_hw *);
    > + unsigned long (*recalc_rate)(struct clk_hw *);
    > + long (*round_rate)(struct clk_hw *, unsigned long);
    > + struct clk * (*get_parent)(struct clk_hw *);
    > +};

    I would like to understand the need for recalc rate if that's something
    that we want to go into the common framework (even if it's optional). I
    have mostly heard only second hand explanations of the need for
    recalc_rate(), so I might not have the full picture. But for all the
    cases that I can think of, recalc_rate seems like a paradox.

    If recalc_rate() is used to make sure the "current rate" of a "clock A"
    is always known even if it's parent "clock B"'s rate is changed, then it
    also means that the rate of "clock A" can change without
    clk_set_rate(clock A, new rate). That in turn means that the
    clk_get_rate() just gives the instantaneous snapshot of the rate. So,
    any use of clk_get_rate(clock A) for anything other than
    printing/logging the return value is broken code. In which case, do we
    really care for recalc_rate()? We could just return the rate that it was
    set to when clk_set_rate() was called and call it a day or return 0 for
    such clocks to indicate that the clock rate is "unknown".

    The whole concept of trying to recalculate the rate for a clock makes me
    feel uneasy since it promotes misunderstanding the behavior of the clock
    and writing bad code based on that misunderstanding.

    I would like to hear to real usecases before I propose some alternatives
    that I have in mind.

    Thanks,
    Saravana

    --
    Sent by an employee of the Qualcomm Innovation Center, Inc.
    The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.


    \
     
     \ /
      Last update: 2011-10-06 03:19    [W:4.295 / U:0.020 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site