Re: [PATCH v3 0/5] common clk framework

[Shawn Guo]

On Mon, Nov 21, 2011 at 05:40:42PM -0800, Mike Turquette wrote:
[...]

>   .the most notable change is the removal of struct clk_hw.

Happy to see that.

> This extra
> layer of abstraction is only necessary if we want hide the definition of
> struct clk from platform code.  Many developers expressed the need to
> know some details of the generic struct clk in the platform layer, and
> rightly so.  Now struct clk is defined in include/linux/clk.h, protected
> by #ifdef CONFIG_GENERIC_CLK.
> 
>   .flags have been introduced to struct clk, with several of them
> defined and used in the common code.  These flags protect against
> changing clk rates or switching the clk parent while that clk is
> enabled; another flag is used to signal to clk_set_rate that it should
> ask the parent to change it's rate too.
> 
>   .speaking of which, clk_set_rate has been overhauled and is now
> recursive. *collective groan*.  clk_set_rate is still simple for the
> common case of simply setting a single clk's rate.  But if your clk has
> the CLK_PARENT_SET_RATE flag and the .round_rate callback recommends
> changing the parent rate, then clk_set_rate will recurse upwards to the
> parent and try it all over again.  In the event of a failure everything
> unwinds and all the clks go out for drinks.
> 
I smell that this will be the part which makes the whole series risky
of being accepted in a desired time frame, with saying rate change
notifications are still missing for now.

>   .clk_register has been replaced by clk_init, which does NOT allocate
> memory for you.  Platforms should allocate their own clk_hw_whatever
> structure which contains struct clk.  clk_init is still necessary to
> initialize struct clk internals.  clk_init also accepts struct device
> *dev as an argument, but does nothing with it.  This is in anticipation
> of device tree support.
> 
I would say that we do not necessarily need to have 'struct device'
for each clk (for imx6 example, we have 110 clks, and I heard some
omap support has 225 clks?).  The device tree support can work out
everything it needs from the 'struct device_node', which can be a
clock blob constraining multiple clks (thanks to 'clock-cells').  That
said you may want to add the 'dev' argument for other reasons, but I
never used it when converting imx6 clock to device tree.

>   .Documentation!  I'm sure somebody reads it.
> 
>   .sysfs support.  Visualize your clk tree at /sys/clk!  Where would be
> a better place to put the clk tree besides the root of /sys/?  When a
> consensus on this is reached I'll submit the proper changes to
> Documentation/ABI/testing/.
> 
> What's missing?
> 
>   .per tree locking.  I implemented this at the Linaro Connect
> conference but the implementation was unpopular, so it didn't make the
> cut.  There needs to be better understanding of everyone's needs for
> this to work.
> 
>   .rate change notifications.  I simply didn't want to delay getting
> these patches to the list any longer, so the notifiers didn't make it
> in.  I'll submit them to the list soon, or roll them into the V4
> patchset.  There are comments in the clk API definitions for where
> PRECHANGE, POSTCHANGE and ABORT propagation will go.
> 
>   .basic mux clk, divider and dummy clk implementations.  I think others
> have some code lying around to implement these, so I left them out.
> 
>   .device tree support.  I haven't looked much at the on-going
> discussions on the dt clk bindings.  How compatible (or not) are the
> device tree clk bindings and the way these patches want to initialize
> clks?
> 
I have just converted imx6 clock to device tree based on this series
and Grant's of-clk series with one minor change on clk-basic which
technically is not part of the core support.  So I would say, do not
worry, it's perfectly compatible with device tree :)

>   .what is the overlap between common clk and clkdev?  We're essentially
> tracking the clks in two places (common clk's tree and clkdevs's list),
> which feels a bit wasteful.
> 
I do not see the overlap between these two.  The clkdev is nothing but
a list maintaining the mapping between necessary 'struct clk' and its
consumer's 'struct dev'.  It has no clock tree topology, and common
clk's tree has no that mapping.

-- 
Regards,
Shawn

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/