diff mbox

[1/6,v2] arm: use devicetree to get smp_twd clock

Message ID 1351882309-733-2-git-send-email-mark.langsdorf@calxeda.com (mailing list archive)
State New, archived
Headers show

Commit Message

Mark Langsdorf Nov. 2, 2012, 6:51 p.m. UTC 
From: Rob Herring <rob.herring@calxeda.com>

Signed-off-by: Rob Herring <rob.herring@calxeda.com>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: linux-arm-kernel@lists.infradead.org

Changes from v1
	None.
---
 arch/arm/kernel/smp_twd.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

Comments

Russell King - ARM Linux Nov. 4, 2012, 10:08 a.m. UTC | #1 
On Fri, Nov 02, 2012 at 01:51:44PM -0500, Mark Langsdorf wrote:
> -static struct clk *twd_get_clock(void)
> +static struct clk *twd_get_clock(struct device_node *np)
>  {
> -	struct clk *clk;
> +	struct clk *clk = NULL;
>  	int err;
>  
> -	clk = clk_get_sys("smp_twd", NULL);
> +	if (np)
> +		clk = of_clk_get(np, 0);
> +	if (!clk)

What does a NULL return from of_clk_get() mean?  Where is this defined?

> @@ -349,6 +348,10 @@ int __init twd_local_timer_register(struct twd_local_timer *tlt)
>  	if (!twd_base)
>  		return -ENOMEM;
>  
> +	twd_clk = twd_get_clock(NULL);
> +
> +	twd_clk = twd_get_clock(NULL);
> +

Why twice?
Mark Langsdorf Nov. 5, 2012, 10:28 p.m. UTC | #2 
On 11/04/2012 04:08 AM, Russell King - ARM Linux wrote:
> On Fri, Nov 02, 2012 at 01:51:44PM -0500, Mark Langsdorf wrote:
>> -static struct clk *twd_get_clock(void)
>> +static struct clk *twd_get_clock(struct device_node *np)
>>  {
>> -	struct clk *clk;
>> +	struct clk *clk = NULL;
>>  	int err;
>>  
>> -	clk = clk_get_sys("smp_twd", NULL);
>> +	if (np)
>> +		clk = of_clk_get(np, 0);
>> +	if (!clk)
> 
> What does a NULL return from of_clk_get() mean?  Where is this defined?

Well, it's a valid path if (np) is NULL. I'll add an IS_ERR(clk) and
resubmit.

>> @@ -349,6 +348,10 @@ int __init twd_local_timer_register(struct twd_local_timer *tlt)
>>  	if (!twd_base)
>>  		return -ENOMEM;
>>  
>> +	twd_clk = twd_get_clock(NULL);
>> +
>> +	twd_clk = twd_get_clock(NULL);
>> +
> 
> Why twice?

No good reason. I'll resubmit with it cleaned up. Thanks for the review.

--Mark Langsdorf
Calxeda, Inc.
Russell King - ARM Linux Nov. 5, 2012, 10:31 p.m. UTC | #3 
On Mon, Nov 05, 2012 at 04:28:05PM -0600, Mark Langsdorf wrote:
> On 11/04/2012 04:08 AM, Russell King - ARM Linux wrote:
> > On Fri, Nov 02, 2012 at 01:51:44PM -0500, Mark Langsdorf wrote:
> >> -static struct clk *twd_get_clock(void)
> >> +static struct clk *twd_get_clock(struct device_node *np)
> >>  {
> >> -	struct clk *clk;
> >> +	struct clk *clk = NULL;
> >>  	int err;
> >>  
> >> -	clk = clk_get_sys("smp_twd", NULL);
> >> +	if (np)
> >> +		clk = of_clk_get(np, 0);
> >> +	if (!clk)
> > 
> > What does a NULL return from of_clk_get() mean?  Where is this defined?
> 
> Well, it's a valid path if (np) is NULL. I'll add an IS_ERR(clk) and
> resubmit.

Hang on - what logic are you trying to achieve here?  Wouldn't:

	if (np)
		clk = of_clk_get(np, 0);
	else
		clk = clk_get_sys("smp_twd", NULL);

be sufficient?  If we have DT, why would we ever want to fall back to
"smp_twd" ?
Mark Langsdorf Nov. 5, 2012, 10:49 p.m. UTC | #4 
On 11/05/2012 04:31 PM, Russell King - ARM Linux wrote:
> On Mon, Nov 05, 2012 at 04:28:05PM -0600, Mark Langsdorf wrote:
>> On 11/04/2012 04:08 AM, Russell King - ARM Linux wrote:
>>> On Fri, Nov 02, 2012 at 01:51:44PM -0500, Mark Langsdorf wrote:
>>>> -static struct clk *twd_get_clock(void)
>>>> +static struct clk *twd_get_clock(struct device_node *np)
>>>>  {
>>>> -	struct clk *clk;
>>>> +	struct clk *clk = NULL;
>>>>  	int err;
>>>>  
>>>> -	clk = clk_get_sys("smp_twd", NULL);
>>>> +	if (np)
>>>> +		clk = of_clk_get(np, 0);
>>>> +	if (!clk)
>>>
>>> What does a NULL return from of_clk_get() mean?  Where is this defined?
>>
>> Well, it's a valid path if (np) is NULL. I'll add an IS_ERR(clk) and
>> resubmit.
> 
> Hang on - what logic are you trying to achieve here?  Wouldn't:
> 
> 	if (np)
> 		clk = of_clk_get(np, 0);
> 	else
> 		clk = clk_get_sys("smp_twd", NULL);
> 
> be sufficient?  If we have DT, why would we ever want to fall back to
> "smp_twd" ?

I'm just trying to make sure I have a clock so I can do cpufreq operations.

Your solution works and is sufficient. Thanks.

--Mark Langsdorf
Calxeda, Inc.
diff mbox

Patch

diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c
index b22d700..600fbcc 100644
--- a/arch/arm/kernel/smp_twd.c
+++ b/arch/arm/kernel/smp_twd.c
@@ -237,12 +237,15 @@  static irqreturn_t twd_handler(int irq, void *dev_id)
 	return IRQ_NONE;
 }
 
-static struct clk *twd_get_clock(void)
+static struct clk *twd_get_clock(struct device_node *np)
 {
-	struct clk *clk;
+	struct clk *clk = NULL;
 	int err;
 
-	clk = clk_get_sys("smp_twd", NULL);
+	if (np)
+		clk = of_clk_get(np, 0);
+	if (!clk)
+		clk = clk_get_sys("smp_twd", NULL);
 	if (IS_ERR(clk)) {
 		pr_err("smp_twd: clock not found: %d\n", (int)PTR_ERR(clk));
 		return clk;
@@ -263,6 +266,7 @@  static struct clk *twd_get_clock(void)
 		return ERR_PTR(err);
 	}
 
+	twd_timer_rate = clk_get_rate(clk);
 	return clk;
 }
 
@@ -273,12 +277,7 @@  static int __cpuinit twd_timer_setup(struct clock_event_device *clk)
 {
 	struct clock_event_device **this_cpu_clk;
 
-	if (!twd_clk)
-		twd_clk = twd_get_clock();
-
-	if (!IS_ERR_OR_NULL(twd_clk))
-		twd_timer_rate = clk_get_rate(twd_clk);
-	else
+	if (IS_ERR_OR_NULL(twd_clk))
 		twd_calibrate_rate();
 
 	__raw_writel(0, twd_base + TWD_TIMER_CONTROL);
@@ -349,6 +348,10 @@  int __init twd_local_timer_register(struct twd_local_timer *tlt)
 	if (!twd_base)
 		return -ENOMEM;
 
+	twd_clk = twd_get_clock(NULL);
+
+	twd_clk = twd_get_clock(NULL);
+
 	return twd_local_timer_common_register();
 }
 
@@ -383,6 +386,8 @@  void __init twd_local_timer_of_register(void)
 		goto out;
 	}
 
+	twd_clk = twd_get_clock(np);
+
 	err = twd_local_timer_common_register();
 
 out: