clk: Allocate and add clock lookups from clk_register()

PATCH New Viresh Kumar 5 participants 8 messages Apr 13, 2012 – Apr 16, 2012 Common clk

Share & tools

Viresh Kumar

author

2012-04-13 06:34 UTC #0 lore raw

PATCH 1/1 clk: Allocate and add clock lookups from clk_register()

clock lookups are required for few clocks for which we do clk_get() or clk_get_sys(). Adding clock lookups for them with new clock framework, would mean a lot of code for each platform. Simplify this by doing this allocation and addition to global list in clk_register() only.

This is done on request by the caller, i.e. only if user passes valid pointers in "dev_id" OR "con_id" fields.

A lot of changes at other places/files/comments are also required, that i would do separately. This is the initial version to get initial feedback.

Pointer to original discussion:
http://www.spinics.net/lists/arm-kernel/msg169133.html

Signed-off-by Viresh Kumar <[email protected]>

---
 drivers/clk/clk.c            |   33 ++++++++++++++++++++++++++-------
 include/linux/clk-provider.h |    4 +++-
 2 files changed, 29 insertions(+), 8 deletions(-)

drivers/clk/clk.c +26 -7

index 2bcce5a..16ce136 100644
@@ -9,6 +9,7 @@
  * Standard functionality for the common clock API.  See Documentation/clk.txt
  */
 
+#include <linux/clkdev.h>
 #include <linux/clk-private.h>
 #include <linux/module.h>
 #include <linux/mutex.h>

@@ -1341,7 +1342,8 @@ out:
  */
 struct clk *clk_register(struct device *dev, const char *name,
 		const struct clk_ops *ops, struct clk_hw *hw,
-		const char **parent_names, u8 num_parents, unsigned long flags)
+		const char **parent_names, u8 num_parents, unsigned long flags,
+		struct clk_lookup **cl, const char *dev_id, const char *con_id)
 {
 	int i, ret = -ENOMEM;
 	struct clk *clk;

@@ -1365,7 +1367,7 @@ struct clk *clk_register(struct device *dev, const char *name,
 
 	if (!clk->parent_names) {
 		pr_err("%s: could not allocate clk->parent_names\n", __func__);
-		goto fail_parent_names;
+		goto free_clk;
 	}
 
 	/* copy each string name in case parent_names is __initdata */

@@ -1373,19 +1375,36 @@ struct clk *clk_register(struct device *dev, const char *name,
 		clk->parent_names[i] = kstrdup(parent_names[i], GFP_KERNEL);
 		if (!clk->parent_names[i]) {
 			pr_err("%s: could not copy parent_names\n", __func__);
-			goto fail_parent_names_copy;
+			goto free_parents;
 		}
 	}
 
 	ret = __clk_init(dev, clk);
-	if (!ret)
-		return clk;
+	if (ret)
+		goto free_parents;
+
+	if (dev_id || con_id) {
+		struct clk_lookup *lookup;
+
+		lookup = clkdev_alloc(clk, dev_id, con_id);
+		if (!lookup) {
+			ret = ENODEV;
+			goto free_parents;
+		}
+
+		clkdev_add(lookup);
+
+		if (cl)
+			*cl = lookup;
+	}
+
+	return clk;
 
-fail_parent_names_copy:
+free_parents:
 	while (--i >= 0)
 		kfree(clk->parent_names[i]);
 	kfree(clk->parent_names);
-fail_parent_names:
+free_clk:
 	kfree(clk);
 fail_out:
 	return ERR_PTR(ret);

include/linux/clk-provider.h +3 -1

index 8bb93c4..a50119d 100644
@@ -12,6 +12,7 @@
 #define __LINUX_CLK_PROVIDER_H
 
 #include <linux/clk.h>
+#include <linux/clkdev.h>
 
 #ifdef CONFIG_COMMON_CLK

@@ -281,7 +282,8 @@ struct clk *clk_register_fixed_factor(struct device *dev, const char *name,
  */
 struct clk *clk_register(struct device *dev, const char *name,
 		const struct clk_ops *ops, struct clk_hw *hw,
-		const char **parent_names, u8 num_parents, unsigned long flags);
+		const char **parent_names, u8 num_parents, unsigned long flags,
+		struct clk_lookup **cl, const char *dev_id, const char *con_id);
 
 /* helper functions */
 const char *__clk_get_name(struct clk *clk);

Signature · 1 line

1.7.9

Richard Zhao

2012-04-13 08:56 UTC #1 lore raw

PATCH 1/1 clk: Allocate and add clock lookups from clk_register()

drivers/clk/clk.c out: · line 1346

Quoted 5 lines

struct clk *clk_register(struct device *dev, const char *name,

drivers/clk/clk.c +2 -1

 		const struct clk_ops *ops, struct clk_hw *hw,
-		const char **parent_names, u8 num_parents, unsigned long flags)
+		const char **parent_names, u8 num_parents, unsigned long flags,
+		struct clk_lookup **cl, const char *dev_id, const char *con_id)

10 arguments. It worths to use a struct ?

Thanks
Richard

Viresh Kumar

author

2012-04-13 09:25 UTC #2 lore raw

PATCH 1/1 clk: Allocate and add clock lookups from clk_register()

[Added you question from both threads here :) ]

Quoted 7 lines · Richard Zhao

Quoted 5 lines · Viresh Kumar
Quoted 1 line · Mark Brown
Or we need to dump the arguments into a struct.
Then we would end up creating too many structures in our mach/clock.c files. That will look bad. May perform better. :)
or pass struct clk_lookup* ? For non-leaf clks, it's NULL.

Non leaf nodes may also need to have clk_lookups, if you want to do clk_get*() on them, to set their rate or parents.

Quoted 6 lines · Richard Zhao

Quoted 5 lines
drivers/clk/clk.c out: · line 1346
Quoted 5 lines
struct clk *clk_register(struct device *dev, const char *name,
drivers/clk/clk.c +2 -1
 		const struct clk_ops *ops, struct clk_hw *hw,
-		const char **parent_names, u8 num_parents, unsigned long flags)
+		const char **parent_names, u8 num_parents, unsigned long flags,
+		struct clk_lookup **cl, const char *dev_id, const char *con_id)
10 arguments. It worths to use a struct ?

I am not saying no here.

But, with that we pass structs all clk_register_*() routines too. So, we will create a lot of structures in our mach clock.c file.

Because in most cases this is one time stuff, only at boot, i vote for the current implementation. :)

I am not an expert but, with my logic i feel with more args system will not be slower. Performance wise, both should be same. As all args would always be read from memory and not registers. In case of struct, only address of struct will be read from cpu registers.

Signature · 1 line

viresh

Sascha Hauer

2012-04-13 10:01 UTC #3 lore raw

PATCH 1/1 clk: Allocate and add clock lookups from clk_register()

In reply to Viresh Kumar

Quoted 5 lines · Viresh Kumar

clock lookups are required for few clocks for which we do clk_get() or clk_get_sys(). Adding clock lookups for them with new clock framework, would mean a lot of code for each platform. Simplify this by doing this allocation and addition to global list in clk_register() only.

Quoted 8 lines

This is done on request by the caller, i.e. only if user passes valid pointers in "dev_id" OR "con_id" fields.
A lot of changes at other places/files/comments are also required, that i would do separately. This is the initial version to get initial feedback.
Pointer to original discussion:
http://www.spinics.net/lists/arm-kernel/msg169133.html

For reasons mentioned in this thread (long argument list, line wraps in clk_register calls, only leaf nodes need lookups) I don't think this is a good idea.

drivers/clk/clk.c out: · line 1347

Quoted 7 lines

*/
struct clk *clk_register(struct device *dev, const char *name,

drivers/clk/clk.c +2 -1

 		const struct clk_ops *ops, struct clk_hw *hw,
-		const char **parent_names, u8 num_parents, unsigned long flags)
+		const char **parent_names, u8 num_parents, unsigned long flags,
+		struct clk_lookup **cl, const char *dev_id, const char *con_id)
 {

Also this doesn't handle the case when multiple lookups are associated with a single clock (which is quite a common case on some SoCs). Yes, we could still use the returned clk to register the additional lookups, but that would make the advantage of this patch even smaller.

Sascha

Signature · 4 lines

Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Viresh Kumar

author

2012-04-13 10:10 UTC #4 lore raw

PATCH 1/1 clk: Allocate and add clock lookups from clk_register()

Quoted 5 lines · Sascha Hauer

Quoted 2 lines
Quoted 2 lines
Pointer to original discussion:
http://www.spinics.net/lists/arm-kernel/msg169133.html
For reasons mentioned in this thread (long argument list, line wraps in clk_register calls, only leaf nodes need lookups) I don't think this is a good idea.

For that we can pass struct as argument.

Quoted 11 lines

Quoted 7 lines
drivers/clk/clk.c out: · line 1347
Quoted 7 lines
*/
struct clk *clk_register(struct device *dev, const char *name,
drivers/clk/clk.c +2 -1
 		const struct clk_ops *ops, struct clk_hw *hw,
-		const char **parent_names, u8 num_parents, unsigned long flags)
+		const char **parent_names, u8 num_parents, unsigned long flags,
+		struct clk_lookup **cl, const char *dev_id, const char *con_id)
 {
Also this doesn't handle the case when multiple lookups are associated with a single clock (which is quite a common case on some SoCs). Yes, we could still use the returned clk to register the additional lookups, but that would make the advantage of this patch even smaller.

If there is a better way of managing this within the clock framework, without platforms having to do clkdev management, we can choose that. My main idea was to make platform code lighter.

We can do one more thing here to support multiple lookups for same clocks:
We can pass array of struct having these three args as its fields.

Signature · 1 line

viresh

Russell King - ARM Linux

2012-04-13 10:30 UTC #5 lore raw

PATCH 1/1 clk: Allocate and add clock lookups from clk_register()

Quoted 21 lines · Viresh Kumar

Quoted 5 lines · Sascha Hauer
Quoted 2 lines
Quoted 2 lines
Pointer to original discussion:
http://www.spinics.net/lists/arm-kernel/msg169133.html
For reasons mentioned in this thread (long argument list, line wraps in clk_register calls, only leaf nodes need lookups) I don't think this is a good idea.
For that we can pass struct as argument.
Quoted 11 lines
Quoted 7 lines
drivers/clk/clk.c out: · line 1347
Quoted 7 lines
*/
struct clk *clk_register(struct device *dev, const char *name,
drivers/clk/clk.c +2 -1
 		const struct clk_ops *ops, struct clk_hw *hw,
-		const char **parent_names, u8 num_parents, unsigned long flags)
+		const char **parent_names, u8 num_parents, unsigned long flags,
+		struct clk_lookup **cl, const char *dev_id, const char *con_id)
 {
Also this doesn't handle the case when multiple lookups are associated with a single clock (which is quite a common case on some SoCs). Yes, we could still use the returned clk to register the additional lookups, but that would make the advantage of this patch even smaller.

Quoted 6 lines

If there is a better way of managing this within the clock framework, without platforms having to do clkdev management, we can choose that. My main idea was to make platform code lighter.
We can do one more thing here to support multiple lookups for same clocks:
We can pass array of struct having these three args as its fields.

You can do something else too:

int clk_register_clkdev(struct clk *clk, struct clk_lookup *cl, size_t num)

{
	unsigned i;

	if (!clk)
		return -ENOMEM;

	for (i = 0; i < num; i++, cl++) {
		cl->clk = clk;
		clkdev_add(cl);
	}
	return 0;
}

and:

int clk_register_single_clkdev(struct clk *clk, const char *devname, const char *conname)

{
	struct clk_lookup *cl;

	if (!clk)
		return -ENOMEM;

	cl = clkdev_alloc(clk, conname, "%s", devname);
	if (!cl)
		return -ENOMEM;
	clkdev_add(cl);
	return 0;
}

and do:

static struct clk_lookup clk_foo_lookups[] = {
	{
		.dev_name = foo,
		.con_name = bar,
	}, {
		...
	},
};

	clk = clk_register(dev, NULL, &ops, &hw, NULL, 0, 0);
	ret = clk_register_clkdev(clk, clk_foo_lookups, ARRAY_SIZE(clk_foo_lookups));
	if (ret)
		...

and:

	clk = clk_register(dev, NULL, &ops, &hw, NULL, 0, 0);
	ret = clk_register_simple_clkdev(clk, "devname", "conname");
	if (ret)
		...

which are both much easier to use than a function taking some 11 arguments, and doesn't require each and every clk_register*() function to be modified to take the clkdev stuff.

Domenico Andreoli

2012-04-13 11:16 UTC #6 lore raw

PATCH 1/1 clk: Allocate and add clock lookups from clk_register()

In reply to Viresh Kumar

Quoted 40 lines · Viresh Kumar

clock lookups are required for few clocks for which we do clk_get() or clk_get_sys(). Adding clock lookups for them with new clock framework, would mean a lot of code for each platform. Simplify this by doing this allocation and addition to global list in clk_register() only.
This is done on request by the caller, i.e. only if user passes valid pointers in "dev_id" OR "con_id" fields.
A lot of changes at other places/files/comments are also required, that i would do separately. This is the initial version to get initial feedback.
Pointer to original discussion:
http://www.spinics.net/lists/arm-kernel/msg169133.html
Signed-off-by Viresh Kumar <[email protected]>
---
 drivers/clk/clk.c            |   33 ++++++++++++++++++++++++++-------
 include/linux/clk-provider.h |    4 +++-
 2 files changed, 29 insertions(+), 8 deletions(-)
drivers/clk/clk.c +9 -2
index 2bcce5a..16ce136 100644
@@ -1373,19 +1375,36 @@ struct clk *clk_register(struct device *dev, const char *name,
 		}
 	}
 
 	ret = __clk_init(dev, clk);
-	if (!ret)
-		return clk;
+	if (ret)
+		goto free_parents;
+
+	if (dev_id || con_id) {
+		struct clk_lookup *lookup;
+
+		lookup = clkdev_alloc(clk, dev_id, con_id);
+		if (!lookup) {
+			ret = ENODEV;

drivers/clk/clk.c struct clk *clk_register(struct device *dev, const char *name, · line 1397

Quoted 10 lines

drivers/clk/clk.c +10 -0

+			goto free_parents;
+		}
+
+		clkdev_add(lookup);
+
+		if (cl)
+			*cl = lookup;
+	}
+
+	return clk;

Basically this is the amount of code you want to save all around, right?

Why can't be this moved in a different function? So only who wants to register the lookup will invoke it after clk_register().

cheers,
Domenico

Viresh Kumar

author

2012-04-16 03:48 UTC #7 lore raw

PATCH 1/1 clk: Allocate and add clock lookups from clk_register()

Quoted 4 lines · Domenico Andreoli

Basically this is the amount of code you want to save all around, right?
Why can't be this moved in a different function? So only who wants to register the lookup will invoke it after clk_register().

Yes, this is discussed at the end of following thread too. :)

http://www.spinics.net/lists/arm-kernel/msg169133.html

Signature · 1 line

viresh