Message-ID: <CAJOA=zOvAV1f+rXP1jYNPOXAchyJPEEAvCdrNfGVg=iZn3Q3Ag@mail.gmail.com>
Date:	Sun, 16 Oct 2011 14:17:29 -0700
From:	"Turquette, Mike" <mturquette@...com>
To:	Richard Zhao <richard.zhao@...aro.org>
Cc:	linux-kernel@...r.kernel.org, paul@...an.com,
	linaro-dev@...ts.linaro.org, linus.walleij@...ricsson.com,
	patches@...aro.org, broonie@...nsource.wolfsonmicro.com,
	magnus.damm@...il.com, amit.kucheria@...aro.org,
	linux@....linux.org.uk, grant.likely@...retlab.ca,
	dsaxena@...aro.org, eric.miao@...aro.org, shawn.guo@...escale.com,
	skannan@...cinc.com, sboyd@...cinc.com, jeremy.kerr@...onical.com,
	tglx@...utronix.de, linux-arm-kernel@...ts.infradead.org,
	arnd.bergmann@...aro.org
Subject: Re: [PATCH v2 1/7] clk: Add a generic clock infrastructure

On Fri, Oct 14, 2011 at 7:24 PM, Richard Zhao <richard.zhao@...aro.org> wrote:
> On Fri, Oct 14, 2011 at 11:14:19AM -0700, Turquette, Mike wrote:
>> On Thu, Sep 22, 2011 at 3:26 PM, Mike Turquette <mturquette@...com> wrote:
>> unsigned long omap_recalc_rate(struct clk_hw *hw)
>> {
>>         struct clk *parent;
>>         struct clk_hw_omap *oclk;
>>
>>         parent = hw->clk->parent;
> clk drivers can not see struct clk details. I use clk_get_parent.

clk_get_parent should query the hardware to see what the parent is.
This can have undesireable overhead.  It is quite acceptable to
reference a clock's parent through clk->parent, just as it is
acceptable to get a clock rate through clk->rate.

An analogous situation is a clk_get_rate call which uses a clk's
.recalc.  There is undesirable overhead involved in .recalc for clocks
whose rates won't change behind our backs, so best to just treat the
data in struct clk as cache and reference it directly.

>>         oclk = to_clk_omap(hw);
>>         ...
>> }
>>
...
>>
>> unsigned long omap_recalc_rate(struct clk *clk)
>> {
>>         struct clk *parent;
>>         struct clk_hw_omap *oclk;
>>
>>         parent = clk->parent;
>>         oclk = to_clk_omap(clk->hw);
>>         ...
>> }
> In my understanding, struct clk stores things specific to clk core,
> struct clk_hw stores common things needed by clk drivers. For static clk driver
> there' some problems:
>  - For clocks without mux, I need duplicate a  .parent and set .get_parent.
>   Even when we adopt DT and dynamicly create clk, it's still a problem.
>   Moving .parent to clk_hw can fix it.

For clocks with a fixed parent we should just pass it in at
register-time.  We should definitely not move .parent out of struct
clk, since struct clk should be the platform agnostic bit that lets us
do tree walks, build topology, etc etc.

If you really want a .parent outside of struct clk then duplicate it
in your struct clk_hw_imx and teach your .ops about it (analogous to
struct clk_hw_fixed->rate).

>  - When I define a clk array, I don't need to find another place to store .ops.
>   It's not problem for dynamic creating clock.

Something like the following?

static struct clk aess_fclk;

static const clk_hw_ops aess_fclk_ops = {
        .recalc = &omap2_clksel_recalc,
        .round_rate = &omap2_clksel_round_rate,
        .set_rate = &omap2_clksel_set_rate,
};

static struct clk_hw_omap aess_fclk_hw = {
        .hw = {
                .clk = &aess_fclk,
        },
        .clksel = &aess_fclk_div,
        .clksel_reg = OMAP4430_CM1_ABE_AESS_CLKCTRL,
        .clksel_mask = OMAP4430_CLKSEL_AESS_FCLK_MASK,
};

static struct clk aess_fclk = {
        .name = "aess_fclk",
        .ops = &aess_fclk_ops,
        .hw = &aess_fclk_hw.hw,
        .parent = &abe_clk,
};

>  - As I mentioned in another mail, clk group need no lock version prepare/unprepare
>   and enable/disable functions

Clock groups are out of scope for this first series.  We should
discuss more what the needs are for your clock groups.  If it boils
down to just enabling all of the clocks for a given device then you
might want to abstract that away with pm_runtime_* calls, and maybe a
supplementary layer like OMAP's hwmod.  But I might be way off base, I
really don't understand your use case for clock groups.

>   Another way is, add a "{struct clk_hw *clks; int count}" in clk_hw, let clk
>   core handle it.
>   I prefer the second way, but I'm not sure whether it's common enough. It's
>   still a problem for dynamic creating clock.

struct clk_hw is just a pointer for navigating from struct clk ->
struct your_custom_clk and vice versa.  Again can you elaborate on
your needs for managing multiple clocks with a single struct clk_hw?

Thanks,
Mike

>
> Thanks
> Richard
>>
>> It is a small nitpick, but it affects the API for everybody so best to
>> get it right now before folks start migrating over to it.
>>
>> Thanks,
>> Mike
>>
>> >        int             (*set_rate)(struct clk_hw *,
>> >                                        unsigned long, unsigned long *);
>> >        long            (*round_rate)(struct clk_hw *, unsigned long);
>> >        int             (*set_parent)(struct clk_hw *, struct clk *);
>> >        struct clk *    (*get_parent)(struct clk_hw *);
>> >  };
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@...ts.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists