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.z...@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 <mturque...@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-ker...@lists.infradead.org
> >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> >>
> >
> >
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
_______________________________________________
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
