diff mbox

[PATCHv10,03/41] CLK: ti: add init support for clock IP blocks

Message ID 1385453182-24421-4-git-send-email-t-kristo@ti.com (mailing list archive)
State New, archived
Headers show

Commit Message

Tero Kristo Nov. 26, 2013, 8:05 a.m. UTC
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.

This patch also introduces a helper function for the TI clock drivers
to get register info from DT and append the master IP info to this.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
---
 drivers/clk/ti/clk.c   |   98 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/clk/ti.h |   14 +++++++
 2 files changed, 112 insertions(+)

Comments

Paul Walmsley Dec. 17, 2013, 8:14 a.m. UTC | #1
On Tue, 26 Nov 2013, Tero Kristo wrote:

> 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.
> 
> This patch also introduces a helper function for the TI clock drivers
> to get register info from DT and append the master IP info to this.
> 
> Signed-off-by: Tero Kristo <t-kristo@ti.com>

...

> diff --git a/drivers/clk/ti/clk.c b/drivers/clk/ti/clk.c
> index ef1a7cd..63f85e9 100644
> --- a/drivers/clk/ti/clk.c
> +++ b/drivers/clk/ti/clk.c
> @@ -19,10 +19,15 @@
>  #include <linux/clkdev.h>
>  #include <linux/clk/ti.h>
>  #include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/list.h>
>  
>  #undef pr_fmt
>  #define pr_fmt(fmt) "%s: " fmt, __func__
>  
> +extern struct of_device_id __clk_of_table[];

This results in a checkpatch.pl warning:

WARNING: externs should be avoided in .c files
#33: FILE: drivers/clk/ti/clk.c:28:
+extern struct of_device_id __clk_of_table[];

Please make sure your patches are checkpatch.pl-clean, with the exception 
of the 80-column warnings and any const-related warnings related to the 
clock framework.

> +static int ti_dt_clk_regmap_index;
> +
>  /**
>   * ti_dt_clocks_register - register DT alias clocks during boot
>   * @oclks: list of clocks to register
> @@ -53,3 +58,96 @@ void __init ti_dt_clocks_register(struct ti_dt_clk oclks[])
>  		}
>  	}
>  }
> +
> +typedef int (*ti_of_clk_init_cb_t)(struct device_node *);

Normally typedefs should be a red flag to reviewers due to 
Documentation/CodingStyle chapter 5.  Still it seems this patch is just 
duplicating the way that the CCF does it, so I'm not too worried about it.

> +
> +struct clk_init_item {
> +	int index;
> +	struct device_node *np;
> +	ti_of_clk_init_cb_t init_cb;
> +	struct list_head node;
> +};
> +
> +static LIST_HEAD(retry_list);
> +
> +/**
> + * ti_clk_get_reg_addr - get register address for a clock register
> + * @node: device node for the clock
> + * @index: register index from the clock node
> + *
> + * Builds clock register address from device tree information. This
> + * is a struct of type clk_omap_reg.
> + */
> +void __iomem *ti_clk_get_reg_addr(struct device_node *node, int index)
> +{
> +	struct clk_omap_reg *reg;
> +	u32 val;
> +	u32 tmp;
> +
> +	reg = (struct clk_omap_reg *)&tmp;
> +	reg->index = ti_dt_clk_regmap_index;
> +
> +	if (of_property_read_u32_index(node, "reg", index, &val)) {
> +		pr_err("%s must have reg[%d]!\n", node->name, index);
> +		return NULL;
> +	}
> +
> +	reg->offset = val;
> +
> +	return (void __iomem *)tmp;
> +}
> +
> +/**
> + * ti_dt_clk_init_provider - init master clock provider
> + * @parent: master node
> + * @index: internal index for clk_reg_ops
> + *
> + * Initializes a master clock IP block and its child clock nodes.
> + * Regmap is provided for accessing the register space for the
> + * IP block and all the clocks under it.
> + */
> +void ti_dt_clk_init_provider(struct device_node *parent, int index)
> +{
> +	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;
> +
> +	ti_dt_clk_regmap_index = index;
> +
> +	for_each_child_of_node(parent, np) {
> +		match = of_match_node(__clk_of_table, np);
> +		if (!match)
> +			continue;
> +		clk_init_cb = (ti_of_clk_init_cb_t)match->data;
> +		pr_debug("%s: initializing: %s\n", __func__, np->name);
> +		ret = clk_init_cb(np);
> +		if (ret == -EAGAIN) {
> +			pr_debug("%s: adding to again list...\n", np->name);
> +			retry = kzalloc(sizeof(*retry), GFP_KERNEL);
> +			retry->np = np;
> +			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);
> +		ti_dt_clk_regmap_index = retry->index;
> +		ret = retry->init_cb(retry->np);
> +		if (ret == -EAGAIN) {
> +			pr_debug("%s failed again?\n", retry->np->name);

This is presumably a serious error condition and should be a pr_warn() or 
pr_err(), right?

If retry_list won't be walked again, then it seems best to delete and free 
the list_entry no matter what the return value is from retry->init_cb(), 
since it's not like it will be retried.  Otherwise the code will leak 
memory.

> +		} 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 df94c24..d6bf530 100644
> --- a/include/linux/clk/ti.h
> +++ b/include/linux/clk/ti.h
> @@ -36,7 +36,21 @@ struct ti_dt_clk {
>  		.node_name = name,	\
>  	}
>  
> +/* Maximum number of clock regmaps */
> +#define CLK_MAX_REGMAPS			4
>  
> +/**
> + * struct clk_omap_reg - OMAP register declaration
> + * @offset: offset from the master IP module base address
> + * @index: index of the master IP module
> + */
> +struct clk_omap_reg {
> +	u16 offset;
> +	u16 index;
> +};
> +
> +void __iomem *ti_clk_get_reg_addr(struct device_node *node, int index);
>  void ti_dt_clocks_register(struct ti_dt_clk *oclks);
> +void ti_dt_clk_init_provider(struct device_node *np, int index);
>  
>  #endif
> -- 
> 1.7.9.5
> 


- Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tero Kristo Dec. 17, 2013, 8:21 a.m. UTC | #2
On 12/17/2013 10:14 AM, Paul Walmsley wrote:
> On Tue, 26 Nov 2013, Tero Kristo wrote:
>
>> 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.
>>
>> This patch also introduces a helper function for the TI clock drivers
>> to get register info from DT and append the master IP info to this.
>>
>> Signed-off-by: Tero Kristo <t-kristo@ti.com>
>
> ...
>
>> diff --git a/drivers/clk/ti/clk.c b/drivers/clk/ti/clk.c
>> index ef1a7cd..63f85e9 100644
>> --- a/drivers/clk/ti/clk.c
>> +++ b/drivers/clk/ti/clk.c
>> @@ -19,10 +19,15 @@
>>   #include <linux/clkdev.h>
>>   #include <linux/clk/ti.h>
>>   #include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/list.h>
>>
>>   #undef pr_fmt
>>   #define pr_fmt(fmt) "%s: " fmt, __func__
>>
>> +extern struct of_device_id __clk_of_table[];
>
> This results in a checkpatch.pl warning:
>
> WARNING: externs should be avoided in .c files
> #33: FILE: drivers/clk/ti/clk.c:28:
> +extern struct of_device_id __clk_of_table[];

This extern is only needed from this single file, and this code is 
duplicated from drivers/clk/clk.c.

> Please make sure your patches are checkpatch.pl-clean, with the exception
> of the 80-column warnings and any const-related warnings related to the
> clock framework.
>
>> +static int ti_dt_clk_regmap_index;
>> +
>>   /**
>>    * ti_dt_clocks_register - register DT alias clocks during boot
>>    * @oclks: list of clocks to register
>> @@ -53,3 +58,96 @@ void __init ti_dt_clocks_register(struct ti_dt_clk oclks[])
>>   		}
>>   	}
>>   }
>> +
>> +typedef int (*ti_of_clk_init_cb_t)(struct device_node *);
>
> Normally typedefs should be a red flag to reviewers due to
> Documentation/CodingStyle chapter 5.  Still it seems this patch is just
> duplicating the way that the CCF does it, so I'm not too worried about it.

This will be removed in next rev, and the standard typedef done by CCF 
will be used instead.

>
>> +
>> +struct clk_init_item {
>> +	int index;
>> +	struct device_node *np;
>> +	ti_of_clk_init_cb_t init_cb;
>> +	struct list_head node;
>> +};
>> +
>> +static LIST_HEAD(retry_list);
>> +
>> +/**
>> + * ti_clk_get_reg_addr - get register address for a clock register
>> + * @node: device node for the clock
>> + * @index: register index from the clock node
>> + *
>> + * Builds clock register address from device tree information. This
>> + * is a struct of type clk_omap_reg.
>> + */
>> +void __iomem *ti_clk_get_reg_addr(struct device_node *node, int index)
>> +{
>> +	struct clk_omap_reg *reg;
>> +	u32 val;
>> +	u32 tmp;
>> +
>> +	reg = (struct clk_omap_reg *)&tmp;
>> +	reg->index = ti_dt_clk_regmap_index;
>> +
>> +	if (of_property_read_u32_index(node, "reg", index, &val)) {
>> +		pr_err("%s must have reg[%d]!\n", node->name, index);
>> +		return NULL;
>> +	}
>> +
>> +	reg->offset = val;
>> +
>> +	return (void __iomem *)tmp;
>> +}
>> +
>> +/**
>> + * ti_dt_clk_init_provider - init master clock provider
>> + * @parent: master node
>> + * @index: internal index for clk_reg_ops
>> + *
>> + * Initializes a master clock IP block and its child clock nodes.
>> + * Regmap is provided for accessing the register space for the
>> + * IP block and all the clocks under it.
>> + */
>> +void ti_dt_clk_init_provider(struct device_node *parent, int index)
>> +{
>> +	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;
>> +
>> +	ti_dt_clk_regmap_index = index;
>> +
>> +	for_each_child_of_node(parent, np) {
>> +		match = of_match_node(__clk_of_table, np);
>> +		if (!match)
>> +			continue;
>> +		clk_init_cb = (ti_of_clk_init_cb_t)match->data;
>> +		pr_debug("%s: initializing: %s\n", __func__, np->name);
>> +		ret = clk_init_cb(np);
>> +		if (ret == -EAGAIN) {
>> +			pr_debug("%s: adding to again list...\n", np->name);
>> +			retry = kzalloc(sizeof(*retry), GFP_KERNEL);
>> +			retry->np = np;
>> +			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);
>> +		ti_dt_clk_regmap_index = retry->index;
>> +		ret = retry->init_cb(retry->np);
>> +		if (ret == -EAGAIN) {
>> +			pr_debug("%s failed again?\n", retry->np->name);
>
> This is presumably a serious error condition and should be a pr_warn() or
> pr_err(), right?

Not a serious, I think this can happen still if we have dependencies 
towards clocks from other IP blocks which provide clocks (e.g. DPLL uses 
source clocks from both PRM + CM IPs.)

> If retry_list won't be walked again, then it seems best to delete and free
> the list_entry no matter what the return value is from retry->init_cb(),
> since it's not like it will be retried.  Otherwise the code will leak
> memory.

It is actually retried, once the next IP block does its init.

-Tero

>
>> +		} 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 df94c24..d6bf530 100644
>> --- a/include/linux/clk/ti.h
>> +++ b/include/linux/clk/ti.h
>> @@ -36,7 +36,21 @@ struct ti_dt_clk {
>>   		.node_name = name,	\
>>   	}
>>
>> +/* Maximum number of clock regmaps */
>> +#define CLK_MAX_REGMAPS			4
>>
>> +/**
>> + * struct clk_omap_reg - OMAP register declaration
>> + * @offset: offset from the master IP module base address
>> + * @index: index of the master IP module
>> + */
>> +struct clk_omap_reg {
>> +	u16 offset;
>> +	u16 index;
>> +};
>> +
>> +void __iomem *ti_clk_get_reg_addr(struct device_node *node, int index);
>>   void ti_dt_clocks_register(struct ti_dt_clk *oclks);
>> +void ti_dt_clk_init_provider(struct device_node *np, int index);
>>
>>   #endif
>> --
>> 1.7.9.5
>>
>
>
> - Paul
>

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Paul Walmsley Dec. 17, 2013, 8:32 a.m. UTC | #3
On Tue, 17 Dec 2013, Tero Kristo wrote:

> On 12/17/2013 10:14 AM, Paul Walmsley wrote:
> > On Tue, 26 Nov 2013, Tero Kristo wrote:
> > 
> > > 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.
> > > 
> > > This patch also introduces a helper function for the TI clock drivers
> > > to get register info from DT and append the master IP info to this.
> > > 
> > > Signed-off-by: Tero Kristo <t-kristo@ti.com>
> > 
> > ...
> > 
> > > diff --git a/drivers/clk/ti/clk.c b/drivers/clk/ti/clk.c
> > > index ef1a7cd..63f85e9 100644
> > > --- a/drivers/clk/ti/clk.c
> > > +++ b/drivers/clk/ti/clk.c
> > > @@ -19,10 +19,15 @@
> > >   #include <linux/clkdev.h>
> > >   #include <linux/clk/ti.h>
> > >   #include <linux/of.h>
> > > +#include <linux/of_address.h>
> > > +#include <linux/list.h>
> > > 
> > >   #undef pr_fmt
> > >   #define pr_fmt(fmt) "%s: " fmt, __func__
> > > 
> > > +extern struct of_device_id __clk_of_table[];
> > 
> > This results in a checkpatch.pl warning:
> > 
> > WARNING: externs should be avoided in .c files
> > #33: FILE: drivers/clk/ti/clk.c:28:
> > +extern struct of_device_id __clk_of_table[];
> 
> This extern is only needed from this single file, and this code is duplicated
> from drivers/clk/clk.c.

So the right thing to do here is to move it into a separate header file 
that both drivers/clk/clk.c and drivers/clk/ti/clk.c either already 
#include, or can add a new #include for.


- Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/clk/ti/clk.c b/drivers/clk/ti/clk.c
index ef1a7cd..63f85e9 100644
--- a/drivers/clk/ti/clk.c
+++ b/drivers/clk/ti/clk.c
@@ -19,10 +19,15 @@ 
 #include <linux/clkdev.h>
 #include <linux/clk/ti.h>
 #include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/list.h>
 
 #undef pr_fmt
 #define pr_fmt(fmt) "%s: " fmt, __func__
 
+extern struct of_device_id __clk_of_table[];
+static int ti_dt_clk_regmap_index;
+
 /**
  * ti_dt_clocks_register - register DT alias clocks during boot
  * @oclks: list of clocks to register
@@ -53,3 +58,96 @@  void __init ti_dt_clocks_register(struct ti_dt_clk oclks[])
 		}
 	}
 }
+
+typedef int (*ti_of_clk_init_cb_t)(struct device_node *);
+
+struct clk_init_item {
+	int index;
+	struct device_node *np;
+	ti_of_clk_init_cb_t init_cb;
+	struct list_head node;
+};
+
+static LIST_HEAD(retry_list);
+
+/**
+ * ti_clk_get_reg_addr - get register address for a clock register
+ * @node: device node for the clock
+ * @index: register index from the clock node
+ *
+ * Builds clock register address from device tree information. This
+ * is a struct of type clk_omap_reg.
+ */
+void __iomem *ti_clk_get_reg_addr(struct device_node *node, int index)
+{
+	struct clk_omap_reg *reg;
+	u32 val;
+	u32 tmp;
+
+	reg = (struct clk_omap_reg *)&tmp;
+	reg->index = ti_dt_clk_regmap_index;
+
+	if (of_property_read_u32_index(node, "reg", index, &val)) {
+		pr_err("%s must have reg[%d]!\n", node->name, index);
+		return NULL;
+	}
+
+	reg->offset = val;
+
+	return (void __iomem *)tmp;
+}
+
+/**
+ * ti_dt_clk_init_provider - init master clock provider
+ * @parent: master node
+ * @index: internal index for clk_reg_ops
+ *
+ * Initializes a master clock IP block and its child clock nodes.
+ * Regmap is provided for accessing the register space for the
+ * IP block and all the clocks under it.
+ */
+void ti_dt_clk_init_provider(struct device_node *parent, int index)
+{
+	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;
+
+	ti_dt_clk_regmap_index = index;
+
+	for_each_child_of_node(parent, np) {
+		match = of_match_node(__clk_of_table, np);
+		if (!match)
+			continue;
+		clk_init_cb = (ti_of_clk_init_cb_t)match->data;
+		pr_debug("%s: initializing: %s\n", __func__, np->name);
+		ret = clk_init_cb(np);
+		if (ret == -EAGAIN) {
+			pr_debug("%s: adding to again list...\n", np->name);
+			retry = kzalloc(sizeof(*retry), GFP_KERNEL);
+			retry->np = np;
+			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);
+		ti_dt_clk_regmap_index = retry->index;
+		ret = retry->init_cb(retry->np);
+		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 df94c24..d6bf530 100644
--- a/include/linux/clk/ti.h
+++ b/include/linux/clk/ti.h
@@ -36,7 +36,21 @@  struct ti_dt_clk {
 		.node_name = name,	\
 	}
 
+/* Maximum number of clock regmaps */
+#define CLK_MAX_REGMAPS			4
 
+/**
+ * struct clk_omap_reg - OMAP register declaration
+ * @offset: offset from the master IP module base address
+ * @index: index of the master IP module
+ */
+struct clk_omap_reg {
+	u16 offset;
+	u16 index;
+};
+
+void __iomem *ti_clk_get_reg_addr(struct device_node *node, int index);
 void ti_dt_clocks_register(struct ti_dt_clk *oclks);
+void ti_dt_clk_init_provider(struct device_node *np, int index);
 
 #endif