diff mbox

[5/6] clk: Support multiple instances of the same clock provider

Message ID 1310352837-4277-5-git-send-email-broonie@opensource.wolfsonmicro.com (mailing list archive)
State New, archived
Headers show

Commit Message

Mark Brown July 11, 2011, 2:53 a.m. UTC
Currently the generic clk API identifies clocks internally using the name
of the clock. This is OK for on-SoC clocks where we have enough control to
disambiguate but doesn't work well for clocks provided on external chips
where a system design may include more than one instance of the same chip
(the Wolfson Speyside system is an example of this) or may have namespace
collisions.

Address this by allowing the clock provider to supply a struct device for
the clock for use in disambiguation. As a first pass if it is provided we
prefix the clock name with the dev_name() of the device. With a device
tree binding for clocks it should be possible to remove this mangling and
instead use the struct device to provide access to the binding information.

In order to avoid needless noise in names and memory usage it is strongly
recommended that on-SoC clocks do not provide a struct device until the
implementation is improved.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 drivers/clk/clk.c   |   36 ++++++++++++++++++++++++++++++++----
 include/linux/clk.h |   14 ++++++++++----
 2 files changed, 42 insertions(+), 8 deletions(-)

Comments

Russell King - ARM Linux July 11, 2011, 9:34 a.m. UTC | #1
On Mon, Jul 11, 2011 at 11:53:56AM +0900, Mark Brown wrote:
> Currently the generic clk API identifies clocks internally using the name
> of the clock. This is OK for on-SoC clocks where we have enough control to
> disambiguate but doesn't work well for clocks provided on external chips
> where a system design may include more than one instance of the same chip
> (the Wolfson Speyside system is an example of this) or may have namespace
> collisions.
> 
> Address this by allowing the clock provider to supply a struct device for
> the clock for use in disambiguation. As a first pass if it is provided we
> prefix the clock name with the dev_name() of the device. With a device
> tree binding for clocks it should be possible to remove this mangling and
> instead use the struct device to provide access to the binding information.
> 
> In order to avoid needless noise in names and memory usage it is strongly
> recommended that on-SoC clocks do not provide a struct device until the
> implementation is improved.
> 
> Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
> ---
>  drivers/clk/clk.c   |   36 ++++++++++++++++++++++++++++++++----
>  include/linux/clk.h |   14 ++++++++++----
>  2 files changed, 42 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 1df6e23..f36f637 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -13,6 +13,7 @@
>  #include <linux/mutex.h>
>  #include <linux/slab.h>
>  #include <linux/spinlock.h>
> +#include <linux/device.h>
>  
>  struct clk {
>  	const char		*name;
> @@ -252,20 +253,44 @@ int clk_set_parent(struct clk *clk, struct clk *parent)
>  }
>  EXPORT_SYMBOL_GPL(clk_set_parent);
>  
> -struct clk *clk_register(const struct clk_hw_ops *ops, struct clk_hw *hw,
> -		const char *name)
> +struct clk *clk_register(struct device *dev, const struct clk_hw_ops *ops,
> +			 struct clk_hw *hw, const char *name)
>  {
>  	struct clk *clk;
> +	char *new_name;
> +	size_t name_len;
>  
>  	clk = kzalloc(sizeof(*clk), GFP_KERNEL);
>  	if (!clk)
>  		return NULL;
>  
> -	clk->name = name;
>  	clk->ops = ops;
>  	clk->hw = hw;
>  	hw->clk = clk;
>  
> +	/* Since we currently match clock providers on a purely string
> +	 * based method add a prefix based on the device name if a
> +	 * device is provided.  When we have support for device tree
> +	 * based clock matching it should be possible to avoid this
> +	 * mangling and instead use the struct device to hook into
> +	 * the bindings.
> +	 *
> +	 * As we don't currently support unregistering clocks we don't
> +	 * need to worry about cleanup as yet.
> +	 */
> +	if (dev) {
> +		name_len = strlen(name) + strlen(dev_name(dev)) + 2;
> +		new_name = kzalloc(name_len, GFP_KERNEL);
> +		if (!new_name)
> +			goto err;
> +
> +		snprintf(new_name, name_len, "%s-%s", dev_name(dev), name);
> +
> +		clk->name = new_name;
> +	} else {
> +		clk->name = name;
> +	}

This "clk consolidation" is really idiotic.  The clk matching mechanism
should have _nothing_ to do with the rest of the clk API, especially the
consolidation effort.

It should not matter whether clkdev is used, or an alternative method
to specify this stuff via DT.  Keep the clk_get()/clk_put() _separate_
from the consolidation of the rest.
Mark Brown July 11, 2011, 10:53 a.m. UTC | #2
On Mon, Jul 11, 2011 at 10:34:39AM +0100, Russell King - ARM Linux wrote:
> On Mon, Jul 11, 2011 at 11:53:56AM +0900, Mark Brown wrote:

> > +	/* Since we currently match clock providers on a purely string
> > +	 * based method add a prefix based on the device name if a
> > +	 * device is provided.  When we have support for device tree

> This "clk consolidation" is really idiotic.  The clk matching mechanism
> should have _nothing_ to do with the rest of the clk API, especially the
> consolidation effort.

It's not touching clkdev, the comment is somewhat misleading and is
mostly based on me thinking about how we'd deploy off-SoC clocks.
There's also the diagnostic issues Sacha mentioned, if we don't keep
some source information handy it's hard to tell what clock logging is
talking about.

> It should not matter whether clkdev is used, or an alternative method
> to specify this stuff via DT.  Keep the clk_get()/clk_put() _separate_
> from the consolidation of the rest.

We do need some way to have some idea which clocks we're talking about,
and for off-SoC stuff passing around struct clk pointers is rather
painful.  At some point some bit of code is going to have to get hold of
the actual struct clk and then map it onto the devices using it.

For device tree we should be able to do that fairly painlessly with just
the struct devices, without device tree you either have to have the
structs handy or use names.  At the minute the infrastructure is
somewhat lacking here.
Russell King - ARM Linux July 11, 2011, 11:11 a.m. UTC | #3
On Mon, Jul 11, 2011 at 07:53:45PM +0900, Mark Brown wrote:
> We do need some way to have some idea which clocks we're talking about,
> and for off-SoC stuff passing around struct clk pointers is rather
> painful.  At some point some bit of code is going to have to get hold of
> the actual struct clk and then map it onto the devices using it.

As I haven't seen any of this "passing around struct clk pointers" crap
recently, I can only guess at what you're saying.  I thought all that
had been solved with _proper_ use of clkdev.

clkdev can provide you with a struct clk for _any_ device in the system
where its name is known at build time.

For devices where the name is not known at build time, a new cl_lookup
entry can be created at runtime with clkdev_alloc() or clk_add_alias(),
and dropped with clkdev_drop().

The problem comes when you aren't able to hook into a subsystem which
creates an unstable device name (eg, USB).  I suspect that's also a
problem for DT too because there has to be some way to go from a struct
device to something stable.
Mark Brown July 11, 2011, 11:41 a.m. UTC | #4
On Mon, Jul 11, 2011 at 12:11:53PM +0100, Russell King - ARM Linux wrote:
> On Mon, Jul 11, 2011 at 07:53:45PM +0900, Mark Brown wrote:
> > We do need some way to have some idea which clocks we're talking about,
> > and for off-SoC stuff passing around struct clk pointers is rather
> > painful.  At some point some bit of code is going to have to get hold of
> > the actual struct clk and then map it onto the devices using it.

> As I haven't seen any of this "passing around struct clk pointers" crap
> recently, I can only guess at what you're saying.  I thought all that
> had been solved with _proper_ use of clkdev.

The problem is getting the data into clkdev in the first place for the
off-SoC devices.

> clkdev can provide you with a struct clk for _any_ device in the system
> where its name is known at build time.

Right, on the consumer side it's all sorted and works really well.  The
trick is on the provider side where you need to have the clock available
to be able to add the mappings for it.

Perhaps I'm just missing something about how this should all work -
actually, the regulator solution probably works fine here.  Pass an
array of mappings into either the driver or the clock subsystem and have
them add the mappings when the provider registers.  Whichever way around
should just be some hooks in the new subsystem.
diff mbox

Patch

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 1df6e23..f36f637 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -13,6 +13,7 @@ 
 #include <linux/mutex.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
+#include <linux/device.h>
 
 struct clk {
 	const char		*name;
@@ -252,20 +253,44 @@  int clk_set_parent(struct clk *clk, struct clk *parent)
 }
 EXPORT_SYMBOL_GPL(clk_set_parent);
 
-struct clk *clk_register(const struct clk_hw_ops *ops, struct clk_hw *hw,
-		const char *name)
+struct clk *clk_register(struct device *dev, const struct clk_hw_ops *ops,
+			 struct clk_hw *hw, const char *name)
 {
 	struct clk *clk;
+	char *new_name;
+	size_t name_len;
 
 	clk = kzalloc(sizeof(*clk), GFP_KERNEL);
 	if (!clk)
 		return NULL;
 
-	clk->name = name;
 	clk->ops = ops;
 	clk->hw = hw;
 	hw->clk = clk;
 
+	/* Since we currently match clock providers on a purely string
+	 * based method add a prefix based on the device name if a
+	 * device is provided.  When we have support for device tree
+	 * based clock matching it should be possible to avoid this
+	 * mangling and instead use the struct device to hook into
+	 * the bindings.
+	 *
+	 * As we don't currently support unregistering clocks we don't
+	 * need to worry about cleanup as yet.
+	 */
+	if (dev) {
+		name_len = strlen(name) + strlen(dev_name(dev)) + 2;
+		new_name = kzalloc(name_len, GFP_KERNEL);
+		if (!new_name)
+			goto err;
+
+		snprintf(new_name, name_len, "%s-%s", dev_name(dev), name);
+
+		clk->name = new_name;
+	} else {
+		clk->name = name;
+	}
+
 	/* Query the hardware for parent and initial rate. We may alter
 	 * the clock topology, making this clock available from the parent's
 	 * children list. So, we need to protect against concurrent
@@ -285,7 +310,10 @@  struct clk *clk_register(const struct clk_hw_ops *ops, struct clk_hw *hw,
 
 	mutex_unlock(&prepare_lock);
 
-
 	return clk;
+
+err:
+	kfree(clk);
+	return NULL;
 }
 EXPORT_SYMBOL_GPL(clk_register);
diff --git a/include/linux/clk.h b/include/linux/clk.h
index fb5e435..cb1879b 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -139,15 +139,21 @@  extern struct clk_hw_ops clk_gate_ops;
 /**
  * clk_register - register and initialize a new clock
  *
+ * @dev: device providing the clock or NULL
  * @ops: ops for the new clock
  * @hw: struct clk_hw to be passed to the ops of the new clock
  * @name: name to use for the new clock
  *
- * Register a new clock with the clk subsytem.  Returns either a
- * struct clk for the new clock or a NULL pointer.
+ * Register a new clock with the clk subsytem.  If dev is provided
+ * then it will be used to disambiguate between multiple instances of
+ * the same device in the system, typically this should only be done
+ * for devices that are not part of the core SoC unless device tree is
+ * in use.
+ *
+ * Returns either a struct clk for the new clock or a NULL pointer.
  */
-struct clk *clk_register(const struct clk_hw_ops *ops, struct clk_hw *hw,
-			 const char *name);
+struct clk *clk_register(struct device *dev, const struct clk_hw_ops *ops,
+			 struct clk_hw *hw, const char *name);
 
 /**
  * clk_unregister - remove a clock