On 02/28/2013 12:58 AM, Prashant Gaikwad wrote:

On Wednesday 06 February 2013 03:36 PM, Tomasz Figa wrote:

quoted

On Wednesday 06 of February 2013 08:34:32 Prashant Gaikwad wrote:

quoted

On Tuesday 05 February 2013 03:45 PM, Tomasz Figa wrote:

quoted

Hi Prashant,

Thank you for your patch. Please see some comments inline.

On Monday 04 of February 2013 13:41:22 Prashant Gaikwad wrote:

quoted

Not all clocks are required to be decomposed into basic clock
types but at the same time want to use the functionality
provided by these basic clock types instead of duplicating.

For example, Tegra SoC has ~100 clocks which can be decomposed
into Mux -> Div -> Gate clock types making the clock count to
~300. Also, parent change operation can not be performed on gate
clock which forces to use mux clock in driver if want to change
the parent.

Instead aggregate the basic clock types functionality into one
clock and just use this clock for all operations. This clock
type re-uses the functionality of basic clock types and not
limited to basic clock types but any hardware-specific
implementation.

quoted

quoted

quoted

quoted

diff --git a/drivers/clk/clk-composite.c

quoted

quoted

quoted

quoted

+static u8 clk_composite_get_parent(struct clk_hw *hw)
+{
+     struct clk_composite *composite = to_clk_composite(hw);
+     const struct clk_ops *mux_ops = composite->mux_ops;
+     struct clk_hw *mux_hw = composite->mux_hw;
+
+     mux_hw->clk = hw->clk;

Why is this needed? Looks like this filed is already being initialized
in clk_register_composite.

Some ops will get called during clk_init where this clk is not populated
hence doing here. I have done it for all ops to make sure that any
future change in clock framework don't break this clock.
Now, why duplicate it in clk_register_composite? It is possible that
none of these ops get called in clk_init.
For example, recalc_rate is called during init and this ops is supported
by div clock type, but what if div clock is not added.

I hope this explains the need.

Sorry, I don't understand your explanation.

I have asked why mux_hw->clk field has to be reinitialized in all the
ops.

In other words, is it even possible that this clk pointer changes since
calling clk_register from clk_register_composite and if yes, why?

Tomasz,

calling sequence is as

clk_register(struct clk_hw *hw)
    clk_init(struct clk_hw *hw)
        .
        .
        hw->clk = clk;
        clk->ops.recalc_rate(hw) => clk_composite_recalc_rate(hw) =>
composite->div_ops.recalc_rate(div_hw) => clk_divider_recalc_rate(hw)

Now if clk_divider_recalc_rate tries to access clk from hw then it will
get NULL since this is not assigned yet and that is what I am doing in
clk_composite_recalc_rate.

I am doing it in all ops because I can not assume which one will get
called first and always. If in future something changes the calling
sequence in ccf framework then it will break this clock.

Surely the CCF core should be taking care of this as part of
clk_register() or clk_init()?