Re: [PATCH v2 1/7] clk: Add a generic clock infrastructure

[Richard Zhao <richard.zhao@linaro.org>]

On Sun, Oct 16, 2011 at 02:17:29PM -0700, Turquette, Mike wrote:
> On Fri, Oct 14, 2011 at 7:24 PM, Richard Zhao <richard.zhao@linaro.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@ti.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.
IMHO, we only need to get parent from hw at register/init time.
clk_get_parent can get it from cache, like current code.
> 
> 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
I don't have clk_hw_imx. I just use generic clks like clk_hw_gate, clk_hw_divider,
clk_hw_mux, and some specific clks.
>  and teach your .ops about it (analogous to
> struct clk_hw_fixed->rate).
I have to define things like below:
stuct pair {
	struct clk_hw *clk = clk_hw_gate.hw;
	struct clk_hw_ops *ops;
};
and use for (.. ) to register the clk array.
> 
> >  - 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,
> };
If we don't protect struct clk members, how about the below:
struct clk_hw_omap aess_fclk = {
	.clk = {
		.name = "aess_fclk",
		.ops = &aess_fclk_ops,
		.parent = &abe_clk,
		};
         .clksel = &aess_fclk_div,
         .clksel_reg = OMAP4430_CM1_ABE_AESS_CLKCTRL,
         .clksel_mask = OMAP4430_CLKSEL_AESS_FCLK_MASK,
};

> 
> >  - 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.
clk group is clk function dependency. I talked about it in another email in this thread.
That's ok to leave it to other framework.
> 
> >   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@lists.infradead.org
> >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> >>
> >
> >
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>