On Sun, Oct 16, 2011 at 02:17:29PM -0700, Turquette, Mike wrote:

On Fri, Oct 14, 2011 at 7:24 PM, Richard Zhao [off-list ref] wrote:

quoted

On Fri, Oct 14, 2011 at 11:14:19AM -0700, Turquette, Mike wrote:

quoted

On Thu, Sep 22, 2011 at 3:26 PM, Mike Turquette [off-list ref] 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.

quoted

quoted

? ? ? ? oclk = to_clk_omap(hw);
? ? ? ? ...
}

...

quoted

quoted

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.

quoted

?- 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,
};

quoted

?- 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.

quoted

? 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

quoted

Thanks
Richard

quoted

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

quoted

? ? ? ?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 at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel