Virtual devices (cpufreq etc) and DT

Virtual devices (cpufreq etc) and DT

[Jamie Iles]

I'm trying to work out how our cpufreq driver fits in with device tree 
bindings.  We have a simple driver that just takes a struct clk and 
calls clk_set_rate() on it.  Is a node in the device tree the right way 
to do this as it isn't really a physical device?  I have the PLL in the 
clocks group of the DT:

	clocks {
		...

		arm_clk: clock@11 {
			compatible = "picochip,pc3x3-pll";
			reg = <0x800a0050 0x8>;
			picoxcell,min-freq = <140000000>;
			picoxcell,max-freq = <700000000>;
			ref-clock = <&ref_clk>, "ref";
			clock-outputs = "cpu";
		};
	};

so I could reference that.  The of clk interface also requires a struct 
device for getting the clk so I guess this is needed...

Jamie

Re: Virtual devices (cpufreq etc) and DT

[Rob Herring]

On 08/03/2011 04:50 AM, Jamie Iles wrote:
> I'm trying to work out how our cpufreq driver fits in with device tree 
> bindings.  We have a simple driver that just takes a struct clk and 
> calls clk_set_rate() on it.  Is a node in the device tree the right way 
> to do this as it isn't really a physical device?  I have the PLL in the 
> clocks group of the DT:

Sounds generically useful...

The OF clock bindings are not really completely finalized and work on
the OF clk code is basically blocked waiting on the common struct clk
infrastructure.

> 
> 	clocks {
> 		...
> 
> 		arm_clk: clock@11 {
> 			compatible = "picochip,pc3x3-pll";
> 			reg = <0x800a0050 0x8>;
> 			picoxcell,min-freq = <140000000>;
> 			picoxcell,max-freq = <700000000>;
> 			ref-clock = <&ref_clk>, "ref";
> 			clock-outputs = "cpu";
> 		};
> 	};
> 

This describes the clock output. You still need to describe the
connection which is what the cpufreq driver should get. For that you
need something like this:

	cpu@0 {
		compatible = "arm,cortex-a9";
		reg = <0>;
		cpu-clock = <&arm_clk>, "cpu";
	};

	cpu@1 {
		compatible = "arm,cortex-a9";
		cpu-clock = <&arm_clk>, "cpu";
	};

Then look for the cpu node(s) and get it's clock.

> so I could reference that.  The of clk interface also requires a struct 
> device for getting the clk so I guess this is needed...

I ran into that problem as well. Making of_clk_get take a struct
device_node ptr instead of struct device fixes the problem. Here's a
patch that does that.

Rob

From: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
Subject: [PATCH] of/clk: use struct device_node for of_clk_get

In order to allow clock retrieval without having a struct device,
use the OF node pointer instead for of_clk_get.

Signed-off-by: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
---
 drivers/clk/clkdev.c   |    3 ++-
 drivers/of/clock.c     |   12 +++++-------
 include/linux/of_clk.h |    4 ++--
 3 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index 9252b4f..a8f1d29 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -89,7 +89,8 @@ struct clk *clk_get(struct device *dev, const char
*con_id)
 	const char *dev_id = dev ? dev_name(dev) : NULL;
 	struct clk *clk;

-	clk = of_clk_get(dev, con_id);
+	if (dev)
+		clk = of_clk_get(dev->of_node, con_id);
 	if (clk && __clk_get(clk))
 		return clk;

diff --git a/drivers/of/clock.c b/drivers/of/clock.c
index 4a6eb0a..7c90994 100644
--- a/drivers/of/clock.c
+++ b/drivers/of/clock.c
@@ -89,7 +89,7 @@ static struct clk *__of_clk_get_from_provider(struct
device_node *np, const char
 	return clk;
 }

-struct clk *of_clk_get(struct device *dev, const char *id)
+struct clk *of_clk_get(struct device_node *node, const char *id)
 {
 	struct device_node *provnode;
 	u32 provhandle;
@@ -98,12 +98,10 @@ struct clk *of_clk_get(struct device *dev, const
char *id)
 	char prop_name[32]; /* 32 is max size of property name */
 	const void *prop;

-	if (!dev)
-		return NULL;
-	dev_dbg(dev, "Looking up %s-clock from device tree\n", id);
+	pr_debug("Looking up %s-clock from device tree\n", id);

 	snprintf(prop_name, 32, "%s-clock", id ? id : "bus");
-	prop = of_get_property(dev->of_node, prop_name, &sz);
+	prop = of_get_property(node, prop_name, &sz);
 	if (!prop || sz < 4)
 		return NULL;

@@ -122,12 +120,12 @@ struct clk *of_clk_get(struct device *dev, const
char *id)
 	provnode = of_find_node_by_phandle(provhandle);
 	if (!provnode) {
 		pr_warn("%s: %s property in node %s references invalid phandle",
-			__func__, prop_name, dev->of_node->full_name);
+			__func__, prop_name, node->full_name);
 		return NULL;
 	}
 	clk = __of_clk_get_from_provider(provnode, prop);
 	if (clk)
-		dev_dbg(dev, "Using clock from %s\n", provnode->full_name);
+		pr_debug("Using clock from %s\n", provnode->full_name);

 	of_node_put(provnode);

diff --git a/include/linux/of_clk.h b/include/linux/of_clk.h
index 71a29eb..848b1c27 100644
--- a/include/linux/of_clk.h
+++ b/include/linux/of_clk.h
@@ -23,12 +23,12 @@ void of_clk_del_provider(struct device_node *np,
 			void *data),
 		void *data);

-struct clk *of_clk_get(struct device *dev, const char *id);
+struct clk *of_clk_get(struct device_node *node, const char *id);

 int of_clk_fixed_parse(void);

 #else
-static inline struct clk *of_clk_get(struct device *dev, const char *id)
+struct clk *of_clk_get(struct device_node *node, const char *id)
 {
 	return NULL;
 }
-- 
1.7.4.1

Re: Virtual devices (cpufreq etc) and DT

[Jamie Iles]

On Wed, Aug 03, 2011 at 11:29:16AM -0500, Rob Herring wrote:
> On 08/03/2011 04:50 AM, Jamie Iles wrote:
> > I'm trying to work out how our cpufreq driver fits in with device tree 
> > bindings.  We have a simple driver that just takes a struct clk and 
> > calls clk_set_rate() on it.  Is a node in the device tree the right way 
> > to do this as it isn't really a physical device?  I have the PLL in the 
> > clocks group of the DT:
> 
> Sounds generically useful...

Yes, once I've got it working internally I'll submit this as a generic 
thing for drivers/cpufreq.

> The OF clock bindings are not really completely finalized and work on
> the OF clk code is basically blocked waiting on the common struct clk
> infrastructure.

OK, so for the platform I'm working on mainlining at the moment does 
that mean I should leave the clock bindings for now or is that something 
that can be revised at a later date?

> > 
> > 	clocks {
> > 		...
> > 
> > 		arm_clk: clock@11 {
> > 			compatible = "picochip,pc3x3-pll";
> > 			reg = <0x800a0050 0x8>;
> > 			picoxcell,min-freq = <140000000>;
> > 			picoxcell,max-freq = <700000000>;
> > 			ref-clock = <&ref_clk>, "ref";
> > 			clock-outputs = "cpu";
> > 		};
> > 	};
> > 
> 
> This describes the clock output. You still need to describe the
> connection which is what the cpufreq driver should get. For that you
> need something like this:
> 
> 	cpu@0 {
> 		compatible = "arm,cortex-a9";
> 		reg = <0>;
> 		cpu-clock = <&arm_clk>, "cpu";
> 	};
> 
> 	cpu@1 {
> 		compatible = "arm,cortex-a9";
> 		cpu-clock = <&arm_clk>, "cpu";
> 	};
> 
> Then look for the cpu node(s) and get it's clock.

Ahh, I hadn't thought of adding it to the cpu node, that's a nice way of 
representing it!

> > so I could reference that.  The of clk interface also requires a struct 
> > device for getting the clk so I guess this is needed...
> 
> I ran into that problem as well. Making of_clk_get take a struct
> device_node ptr instead of struct device fixes the problem. Here's a
> patch that does that.

Nice, thanks Rob!

Jamie

Re: Virtual devices (cpufreq etc) and DT

[Rob Herring]

On 08/03/2011 11:41 AM, Jamie Iles wrote:
> On Wed, Aug 03, 2011 at 11:29:16AM -0500, Rob Herring wrote:
>> On 08/03/2011 04:50 AM, Jamie Iles wrote:
>>> I'm trying to work out how our cpufreq driver fits in with device tree 
>>> bindings.  We have a simple driver that just takes a struct clk and 
>>> calls clk_set_rate() on it.  Is a node in the device tree the right way 
>>> to do this as it isn't really a physical device?  I have the PLL in the 
>>> clocks group of the DT:
>>
>> Sounds generically useful...
> 
> Yes, once I've got it working internally I'll submit this as a generic 
> thing for drivers/cpufreq.
> 
>> The OF clock bindings are not really completely finalized and work on
>> the OF clk code is basically blocked waiting on the common struct clk
>> infrastructure.
> 
> OK, so for the platform I'm working on mainlining at the moment does 
> that mean I should leave the clock bindings for now or is that something 
> that can be revised at a later date?
> 
I'm separating it out for mine and just doing limited clk implementation
now based on the rate common struct clk is going.

There's a 3rd option. Implement DT clk binding parsing and clk node
creation within your platform. Perhaps the struct clk details could be
abstracted out from the binding parsing code so some could still be common.

Rob

>>>
>>> 	clocks {
>>> 		...
>>>
>>> 		arm_clk: clock@11 {
>>> 			compatible = "picochip,pc3x3-pll";
>>> 			reg = <0x800a0050 0x8>;
>>> 			picoxcell,min-freq = <140000000>;
>>> 			picoxcell,max-freq = <700000000>;
>>> 			ref-clock = <&ref_clk>, "ref";
>>> 			clock-outputs = "cpu";
>>> 		};
>>> 	};
>>>
>>
>> This describes the clock output. You still need to describe the
>> connection which is what the cpufreq driver should get. For that you
>> need something like this:
>>
>> 	cpu@0 {
>> 		compatible = "arm,cortex-a9";
>> 		reg = <0>;
>> 		cpu-clock = <&arm_clk>, "cpu";
>> 	};
>>
>> 	cpu@1 {
>> 		compatible = "arm,cortex-a9";
>> 		cpu-clock = <&arm_clk>, "cpu";
>> 	};
>>
>> Then look for the cpu node(s) and get it's clock.
> 
> Ahh, I hadn't thought of adding it to the cpu node, that's a nice way of 
> representing it!
> 
>>> so I could reference that.  The of clk interface also requires a struct 
>>> device for getting the clk so I guess this is needed...
>>
>> I ran into that problem as well. Making of_clk_get take a struct
>> device_node ptr instead of struct device fixes the problem. Here's a
>> patch that does that.
> 
> Nice, thanks Rob!
> 
> Jamie

Re: Virtual devices (cpufreq etc) and DT

[Jamie Iles]

On Wed, Aug 03, 2011 at 11:54:01AM -0500, Rob Herring wrote:
> On 08/03/2011 11:41 AM, Jamie Iles wrote:
> > On Wed, Aug 03, 2011 at 11:29:16AM -0500, Rob Herring wrote:
> >> On 08/03/2011 04:50 AM, Jamie Iles wrote:
> >>> I'm trying to work out how our cpufreq driver fits in with device tree 
> >>> bindings.  We have a simple driver that just takes a struct clk and 
> >>> calls clk_set_rate() on it.  Is a node in the device tree the right way 
> >>> to do this as it isn't really a physical device?  I have the PLL in the 
> >>> clocks group of the DT:
> >>
> >> Sounds generically useful...
> > 
> > Yes, once I've got it working internally I'll submit this as a generic 
> > thing for drivers/cpufreq.
> > 
> >> The OF clock bindings are not really completely finalized and work on
> >> the OF clk code is basically blocked waiting on the common struct clk
> >> infrastructure.
> > 
> > OK, so for the platform I'm working on mainlining at the moment does 
> > that mean I should leave the clock bindings for now or is that something 
> > that can be revised at a later date?
> > 
> I'm separating it out for mine and just doing limited clk implementation
> now based on the rate common struct clk is going.
> 
> There's a 3rd option. Implement DT clk binding parsing and clk node
> creation within your platform. Perhaps the struct clk details could be
> abstracted out from the binding parsing code so some could still be common.

OK, that sounds like pretty much what I have at the moment.  I have a 
struct clk and struct clk_ops then separate binding parsers so it should 
be fairly easy to port over.  I'll post some patches after the merge 
window closes.

Thanks,

Jamie

Re: Virtual devices (cpufreq etc) and DT

[Jamie Iles]

Hi Rob,

On Wed, Aug 03, 2011 at 11:29:16AM -0500, Rob Herring wrote:
[...]
> Making of_clk_get take a struct device_node ptr instead of struct 
> device fixes the problem. Here's a patch that does that.
> 
> Rob
> 
> From: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
> Subject: [PATCH] of/clk: use struct device_node for of_clk_get
> 
> In order to allow clock retrieval without having a struct device,
> use the OF node pointer instead for of_clk_get.
> 
> Signed-off-by: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
> ---
>  drivers/clk/clkdev.c   |    3 ++-
>  drivers/of/clock.c     |   12 +++++-------
>  include/linux/of_clk.h |    4 ++--
>  3 files changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
> index 9252b4f..a8f1d29 100644
> --- a/drivers/clk/clkdev.c
> +++ b/drivers/clk/clkdev.c
> @@ -89,7 +89,8 @@ struct clk *clk_get(struct device *dev, const char
> *con_id)
>  	const char *dev_id = dev ? dev_name(dev) : NULL;
>  	struct clk *clk;
> 
> -	clk = of_clk_get(dev, con_id);
> +	if (dev)
> +		clk = of_clk_get(dev->of_node, con_id);
>  	if (clk && __clk_get(clk))
>  		return clk;

I've just tried this patch and it works a treat, but I had to change 
this hunk to the one below to stop the compiler warning about a possible 
uninitialized use of clk.

Jamie

diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index f2b1224..bc5ae55 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -80,9 +80,10 @@ EXPORT_SYMBOL(clk_get_sys);
 struct clk *clk_get(struct device *dev, const char *con_id)
 {
 	const char *dev_id = dev ? dev_name(dev) : NULL;
-	struct clk *clk;
+	struct clk *clk = NULL;
 
-	clk = of_clk_get(dev, con_id);
+	if (dev->of_node)
+		clk = of_clk_get(dev->of_node, con_id);
 	if (clk && __clk_get(clk))
		return clk;

Re: Virtual devices (cpufreq etc) and DT

[Grant Likely]

On Thu, Aug 4, 2011 at 11:23 AM, Jamie Iles <jamie-wmLquQDDieKakBO8gow8eQ@public.gmane.org> wrote:
> Hi Rob,
>
> On Wed, Aug 03, 2011 at 11:29:16AM -0500, Rob Herring wrote:
> [...]
>> Making of_clk_get take a struct device_node ptr instead of struct
>> device fixes the problem. Here's a patch that does that.
>>
>> Rob
>>
>> From: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
>> Subject: [PATCH] of/clk: use struct device_node for of_clk_get
>>
>> In order to allow clock retrieval without having a struct device,
>> use the OF node pointer instead for of_clk_get.
>>
>> Signed-off-by: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
>> ---
>>  drivers/clk/clkdev.c   |    3 ++-
>>  drivers/of/clock.c     |   12 +++++-------
>>  include/linux/of_clk.h |    4 ++--
>>  3 files changed, 9 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
>> index 9252b4f..a8f1d29 100644
>> --- a/drivers/clk/clkdev.c
>> +++ b/drivers/clk/clkdev.c
>> @@ -89,7 +89,8 @@ struct clk *clk_get(struct device *dev, const char
>> *con_id)
>>       const char *dev_id = dev ? dev_name(dev) : NULL;
>>       struct clk *clk;
>>
>> -     clk = of_clk_get(dev, con_id);
>> +     if (dev)
>> +             clk = of_clk_get(dev->of_node, con_id);
>>       if (clk && __clk_get(clk))
>>               return clk;
>
> I've just tried this patch and it works a treat, but I had to change
> this hunk to the one below to stop the compiler warning about a possible
> uninitialized use of clk.

You could also change of_clk_get() to simply return null if it is
passed a null of_node.  That would allow drivers to be a tad simpler.

g.

Re: Virtual devices (cpufreq etc) and DT

[Jamie Iles]

On Thu, Aug 04, 2011 at 11:33:37AM +0100, Grant Likely wrote:
> On Thu, Aug 4, 2011 at 11:23 AM, Jamie Iles <jamie-wmLquQDDieKakBO8gow8eQ@public.gmane.org> wrote:
> > Hi Rob,
> >
> > On Wed, Aug 03, 2011 at 11:29:16AM -0500, Rob Herring wrote:
> > [...]
> >> Making of_clk_get take a struct device_node ptr instead of struct
> >> device fixes the problem. Here's a patch that does that.
> >>
> >> Rob
> >>
> >> From: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
> >> Subject: [PATCH] of/clk: use struct device_node for of_clk_get
> >>
> >> In order to allow clock retrieval without having a struct device,
> >> use the OF node pointer instead for of_clk_get.
> >>
> >> Signed-off-by: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
> >> ---
> >>  drivers/clk/clkdev.c   |    3 ++-
> >>  drivers/of/clock.c     |   12 +++++-------
> >>  include/linux/of_clk.h |    4 ++--
> >>  3 files changed, 9 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
> >> index 9252b4f..a8f1d29 100644
> >> --- a/drivers/clk/clkdev.c
> >> +++ b/drivers/clk/clkdev.c
> >> @@ -89,7 +89,8 @@ struct clk *clk_get(struct device *dev, const char
> >> *con_id)
> >>       const char *dev_id = dev ? dev_name(dev) : NULL;
> >>       struct clk *clk;
> >>
> >> -     clk = of_clk_get(dev, con_id);
> >> +     if (dev)
> >> +             clk = of_clk_get(dev->of_node, con_id);
> >>       if (clk && __clk_get(clk))
> >>               return clk;
> >
> > I've just tried this patch and it works a treat, but I had to change
> > this hunk to the one below to stop the compiler warning about a possible
> > uninitialized use of clk.
> 
> You could also change of_clk_get() to simply return null if it is
> passed a null of_node.  That would allow drivers to be a tad simpler.

Yes, that would be a lot neater.

Jamie