On 10/31/2013 05:42 PM, Nishanth Menon wrote:

On 10/25/2013 10:57 AM, Tero Kristo wrote:

quoted

ti_dt_clk_init_provider() can now be used to initialize the contents of
a single clock IP block. This parses all the clocks under the IP block
and calls the corresponding init function for them.

Signed-off-by: Tero Kristo <redacted>
---
  drivers/clk/ti/clk.c   |   59 ++++++++++++++++++++++++++++++++++++++++++++++++
  include/linux/clk/ti.h |    1 +
  2 files changed, 60 insertions(+)

diff --git a/drivers/clk/ti/clk.c b/drivers/clk/ti/clk.c
index ad58b01..7f030d7 100644
--- a/drivers/clk/ti/clk.c
+++ b/drivers/clk/ti/clk.c

@@ -19,6 +19,9 @@
  #include <linux/clkdev.h>
  #include <linux/clk/ti.h>
  #include <linux/of.h>
+#include <linux/list.h>
+
+extern struct of_device_id __clk_of_table[];

  /**
   * ti_dt_clocks_register - register DT duplicate clocks during boot

@@ -50,3 +53,59 @@ void __init ti_dt_clocks_register(struct ti_dt_clk oclks[])
  		}
  	}
  }
+
+typedef int (*ti_of_clk_init_cb_t)(struct device_node *, struct regmap *);
+
+struct clk_init_item {
+	struct regmap *regmap;
+	struct device_node *np;
+	ti_of_clk_init_cb_t init_cb;
+	struct list_head node;
+};
+
+static LIST_HEAD(retry_list);
+
+void __init ti_dt_clk_init_provider(struct device_node *parent,
+				    struct regmap *regmap)
+{
+	const struct of_device_id *match;
+	struct device_node *np;
+	ti_of_clk_init_cb_t clk_init_cb;
+	struct clk_init_item *retry;
+	struct clk_init_item *tmp;
+	int ret;
+
+	for_each_child_of_node(parent, np) {
+		match = of_match_node(__clk_of_table, np);
+		if (!match)
+			continue;
+		clk_init_cb = match->data;

I must admit I am confused here.

Yea this patch is something I am not quite comfortable myself yet and 
would like improvement ideas....

a) of_clk_init (in the generic clk.c) uses of_clk_init_cb_t as match
data. The prototype of the generic of_clk_init_cb_t is typedef void
(*of_clk_init_cb_t)(struct device_node *);
b) both of_clk_init and ti_dt_clk_init_provider looks up clock nodes
from __clk_of_table

__clk_of_table contains the function pointers for the clock init 
functions, not clock nodes.

I assume we need ti_dt_clk_init_provider to be always called with
clock list, as a result of_clk_init(NULL); will never be invoked. This
is counter productive as you have have generic non SoC clock providers
as well who would have been invoked with of_clk_init(NULL);

Can't call of_clk_init(NULL) anymore, as Paul wants to map the clock 
nodes under respective IP blocks. Two reasons here:

a) This requires me to pass some info from the IP block (CM1/CM2/PRM) to 
the clock init functions, basically the pointer to the memory map region 
(regmap.)
b) of_clk_init(NULL) will initialize all the clock nodes in the system, 
irrespective of the hierarchy considerations.

Only thing that can be done, is to make the API introduced in this patch 
a generic API and call it something like of_clk_init_children().

I do strongly encourage using of_clk_init(NULL) and not having to
switch the clk_init call back with SoC specific one as it forces
un-intended dependency.

Can't do this as mentioned above.

Further such as SoC specific callback should have been in a header.

quoted

+		pr_debug("%s: initializing: %s\n", __func__, np->name);

one further comment - using pr_fmt will save us from using __func__
for every pr_* message :(

Hmm good comment, will fix that for next rev.

quoted

+		ret = clk_init_cb(np, regmap);
+		if (ret == -EAGAIN) {
+			pr_debug("%s: adding to again list...\n", np->name);
+			retry = kzalloc(sizeof(*retry), GFP_KERNEL);
+			retry->np = np;
+			retry->regmap = regmap;
+			retry->init_cb = clk_init_cb;
+			list_add(&retry->node, &retry_list);
+		} else if (ret) {
+			pr_err("%s: clock init failed for %s (%d)!\n", __func__,
+			       np->name, ret);
+		}
+	}
+
+	list_for_each_entry_safe(retry, tmp, &retry_list, node) {
+		pr_debug("%s: retry-init: %s\n", __func__, retry->np->name);
+		ret = retry->init_cb(retry->np, retry->regmap);
+		if (ret == -EAGAIN) {
+			pr_debug("%s failed again?\n", retry->np->name);
+		} else {
+			if (ret)
+				pr_err("%s: clock init failed for %s (%d)!\n",
+				       __func__, retry->np->name, ret);
+			list_del(&retry->node);
+			kfree(retry);
+		}
+	}
+}

diff --git a/include/linux/clk/ti.h b/include/linux/clk/ti.h
index 9786752..7ab02fa 100644
--- a/include/linux/clk/ti.h
+++ b/include/linux/clk/ti.h

@@ -177,6 +177,7 @@ int omap3_dpll4_set_rate(struct clk_hw *clk, unsigned long rate,
  			 unsigned long parent_rate);

  void ti_dt_clocks_register(struct ti_dt_clk *oclks);
+void ti_dt_clk_init_provider(struct device_node *np, struct regmap *regmap);

  extern const struct clk_hw_omap_ops clkhwops_omap3_dpll;
  extern const struct clk_hw_omap_ops clkhwops_omap4_dpllmx;