ARM: OMAP2+: Fix bogus struct clk comparison for timer clock

PATCH New Tony Lindgren 2 participants 6 messages May 5, 2015 – May 7, 2015

Subsystems (3)

Arm (32-bit) Arm sub-architectures OMAP2+ support

lore

Share & tools

lore

Tony Lindgren

author maintainer

2015-05-05 18:02 UTC #0 lore raw

PATCH 1/1 ARM: OMAP2+: Fix bogus struct clk comparison for timer clock

With recent changes to use determine_rate, the comparison of two clocks won't work without doing __clk_get_hw on the clocks first.

Cc Michael Turquette <[email protected]>

Cc Paul Walmsley <[email protected]>

Cc Stephen Boyd <[email protected]>

Cc Tero Kristo <[email protected]>

Cc Tomeu Vizoso <[email protected]>

Signed-off-by Tony Lindgren <[email protected]>

---

Found one more of these, the coccinelle scripts do not seem to match this?

---
 arch/arm/mach-omap2/timer.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

arch/arm/mach-omap2/timer.c +2 -1

index cef67af..22c7a8e 100644
@@ -35,6 +35,7 @@
 #include <linux/irq.h>
 #include <linux/clocksource.h>
 #include <linux/clockchips.h>
+#include <linux/clk-provider.h>
 #include <linux/slab.h>
 #include <linux/of.h>
 #include <linux/of_address.h>

@@ -298,7 +299,7 @@ static int __init omap_dm_timer_init_one(struct omap_dm_timer *timer,
 	if (IS_ERR(src))
 		return PTR_ERR(src);
 
-	if (clk_get_parent(timer->fclk) != src) {
+	if (__clk_get_hw(clk_get_parent(timer->fclk)) != __clk_get_hw(src)) {
 		r = clk_set_parent(timer->fclk, src);
 		if (r < 0) {
 			pr_warn("%s: %s cannot set source\n", __func__,

Signature · 1 line

2.1.4

Stephen Boyd

2015-05-06 05:35 UTC #1 lore raw

PATCH 1/1 ARM: OMAP2+: Fix bogus struct clk comparison for timer clock

Quoted 10 lines · Tony Lindgren

With recent changes to use determine_rate, the comparison of two clocks won't work without doing __clk_get_hw on the clocks first.
Cc Michael Turquette <[email protected]>
Cc Paul Walmsley <[email protected]>
Cc Stephen Boyd <[email protected]>
Cc Tero Kristo <[email protected]>
Cc Tomeu Vizoso <[email protected]>
Signed-off-by Tony Lindgren <[email protected]>

Quoted 4 lines

---
Found one more of these, the coccinelle scripts do not seem to match this?

I don't think the coccinelle script was ever merged...

Quoted 18 lines

---
 arch/arm/mach-omap2/timer.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

arch/arm/mach-omap2/timer.c +1 -0

index cef67af..22c7a8e 100644
@@ -35,6 +35,7 @@
 #include <linux/irq.h>
 #include <linux/clocksource.h>
 #include <linux/clockchips.h>
+#include <linux/clk-provider.h>
 #include <linux/slab.h>
 #include <linux/of.h>
 #include <linux/of_address.h>

arch/arm/mach-omap2/timer.c static int __init omap_dm_timer_init_one(struct omap_dm_timer *timer, · line 302

Quoted 5 lines

arch/arm/mach-omap2/timer.c +1 -1

@@ -298,7 +299,7 @@ static int __init omap_dm_timer_init_one(struct omap_dm_timer *timer,
 	if (IS_ERR(src))
 		return PTR_ERR(src);
 
-	if (clk_get_parent(timer->fclk) != src) {
+	if (__clk_get_hw(clk_get_parent(timer->fclk)) != __clk_get_hw(src)) {

Please use clk_is_match() instead so we can find these instances with an easy grep.

Quoted 3 lines

arch/arm/mach-omap2/timer.c +0 -0

 		r = clk_set_parent(timer->fclk, src);
 		if (r < 0) {
 			pr_warn("%s: %s cannot set source\n", __func__,

Signature · 2 lines

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Tony Lindgren

author maintainer

2015-05-06 14:55 UTC #2 lore raw

PATCH 1/1 ARM: OMAP2+: Fix bogus struct clk comparison for timer clock

Quoted 13 lines · Stephen Boyd

Quoted 12 lines · Tony Lindgren
With recent changes to use determine_rate, the comparison of two clocks won't work without doing __clk_get_hw on the clocks first.
Cc Michael Turquette <[email protected]>
Cc Paul Walmsley <[email protected]>
Cc Stephen Boyd <[email protected]>
Cc Tero Kristo <[email protected]>
Cc Tomeu Vizoso <[email protected]>
Signed-off-by Tony Lindgren <[email protected]>
---

Quoted 4 lines

Quoted 2 lines
Found one more of these, the coccinelle scripts do not seem to match this?
I don't think the coccinelle script was ever merged...

Oh OK, but the wrong comparisons in the kernel code got fixed, right?

Quoted 9 lines

Review static int __init omap_dm_timer_init_one(struct omap_dm_timer *timer, · line 302
Quoted 6 lines
(quoted diff) +1 -1
@@ -298,7 +299,7 @@ static int __init omap_dm_timer_init_one(struct omap_dm_timer *timer,
 	if (IS_ERR(src))
 		return PTR_ERR(src);
 
-	if (clk_get_parent(timer->fclk) != src) {
+	if (__clk_get_hw(clk_get_parent(timer->fclk)) != __clk_get_hw(src)) {
Please use clk_is_match() instead so we can find these instances with an easy grep.

Thanks here's an updated version with that.

Regards,

Tony

8< -----------------------------
From: Tony Lindgren <[email protected]>
Date: Tue, 5 May 2015 09:03:34 -0700
Subject: [PATCH] ARM: OMAP2+: Fix bogus struct clk comparison for timer clock

With recent changes to use determine_rate, the comparison of two clocks won't work without clk_is_match that does __clk_get_hw on the clocks first.

Cc Michael Turquette <[email protected]>

Cc Paul Walmsley <[email protected]>

Cc Stephen Boyd <[email protected]>

Cc Tero Kristo <[email protected]>

Cc Tomeu Vizoso <[email protected]>

Signed-off-by Tony Lindgren <[email protected]>

arch/arm/mach-omap2/timer.c +1 -1

@@ -298,7 +298,7 @@ static int __init omap_dm_timer_init_one(struct omap_dm_timer *timer,
 	if (IS_ERR(src))
 		return PTR_ERR(src);
 
-	if (clk_get_parent(timer->fclk) != src) {
+	if (!clk_is_match(clk_get_parent(timer->fclk), src)) {
 		r = clk_set_parent(timer->fclk, src);
 		if (r < 0) {
 			pr_warn("%s: %s cannot set source\n", __func__,

Stephen Boyd

2015-05-07 00:29 UTC #3 lore raw

PATCH 1/1 ARM: OMAP2+: Fix bogus struct clk comparison for timer clock

Quoted 14 lines · Tony Lindgren

Quoted 13 lines · Stephen Boyd
Quoted 12 lines · Tony Lindgren
With recent changes to use determine_rate, the comparison of two clocks won't work without doing __clk_get_hw on the clocks first.
Cc Michael Turquette <[email protected]>
Cc Paul Walmsley <[email protected]>
Cc Stephen Boyd <[email protected]>
Cc Tero Kristo <[email protected]>
Cc Tomeu Vizoso <[email protected]>
Signed-off-by Tony Lindgren <[email protected]>
---

Quoted 5 lines

Quoted 3 lines
Quoted 2 lines
Found one more of these, the coccinelle scripts do not seem to match this?
I don't think the coccinelle script was ever merged...
Oh OK, but the wrong comparisons in the kernel code got fixed, right?

Yes the ones that mattered were fixed.

Quoted 9 lines

Quoted 8 lines
arch/arm/mach-omap2/timer.c static int __init omap_dm_timer_init_one(struct omap_dm_timer *timer, · line 302
Quoted 6 lines
arch/arm/mach-omap2/timer.c +1 -1
@@ -298,7 +299,7 @@ static int __init omap_dm_timer_init_one(struct omap_dm_timer *timer,
 	if (IS_ERR(src))
 		return PTR_ERR(src);
 
-	if (clk_get_parent(timer->fclk) != src) {
+	if (__clk_get_hw(clk_get_parent(timer->fclk)) != __clk_get_hw(src)) {
Please use clk_is_match() instead so we can find these instances with an easy grep.
Thanks here's an updated version with that.

Are you taking this through arm-soc?

Quoted 23 lines

Regards,
Tony
8< -----------------------------
From: Tony Lindgren <[email protected]>
Date: Tue, 5 May 2015 09:03:34 -0700
Subject: [PATCH] ARM: OMAP2+: Fix bogus struct clk comparison for timer clock
With recent changes to use determine_rate, the comparison of two clocks won't work without clk_is_match that does __clk_get_hw on the clocks first.
Cc Michael Turquette <[email protected]>
Cc Paul Walmsley <[email protected]>
Cc Stephen Boyd <[email protected]>
Cc Tero Kristo <[email protected]>
Cc Tomeu Vizoso <[email protected]>
Signed-off-by Tony Lindgren <[email protected]>

arch/arm/mach-omap2/timer.c static int __init omap_dm_timer_init_one(struct omap_dm_timer *timer, · line 302

Quoted 6 lines

arch/arm/mach-omap2/timer.c +1 -1

@@ -298,7 +298,7 @@ static int __init omap_dm_timer_init_one(struct omap_dm_timer *timer,
 	if (IS_ERR(src))
 		return PTR_ERR(src);
 
-	if (clk_get_parent(timer->fclk) != src) {
+	if (!clk_is_match(clk_get_parent(timer->fclk), src)) {
 		r = clk_set_parent(timer->fclk, src);

I also wonder why we can't just call clk_set_parent() and skip the "is the parent already src" check?

If there's a good reason for not just calling clk_set_parent() then it makes sense to do the clk_is_match() thing, and you can have my ack

Acked-by Stephen Boyd <[email protected]>

Signature · 2 lines

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Tony Lindgren

author maintainer

2015-05-07 14:44 UTC #4 lore raw

PATCH 1/1 ARM: OMAP2+: Fix bogus struct clk comparison for timer clock

Quoted 7 lines · Stephen Boyd

Quoted 4 lines · Tony Lindgren
Quoted 1 line
I don't think the coccinelle script was ever merged...
Oh OK, but the wrong comparisons in the kernel code got fixed, right?
Yes the ones that mattered were fixed.

OK thanks for confirming that.

Quoted 3 lines

Quoted 1 line
Thanks here's an updated version with that.
Are you taking this through arm-soc?

Yes that's the plan.

Quoted 13 lines

arch/arm/mach-omap2/timer.c static int __init omap_dm_timer_init_one(struct omap_dm_timer *timer, · line 302
Quoted 9 lines
arch/arm/mach-omap2/timer.c +1 -1
@@ -298,7 +298,7 @@ static int __init omap_dm_timer_init_one(struct omap_dm_timer *timer,
 	if (IS_ERR(src))
 		return PTR_ERR(src);
 
-	if (clk_get_parent(timer->fclk) != src) {
+	if (!clk_is_match(clk_get_parent(timer->fclk), src)) {
 		r = clk_set_parent(timer->fclk, src);
I also wonder why we can't just call clk_set_parent() and skip the "is the parent already src" check?

Quoted 4 lines

If there's a good reason for not just calling clk_set_parent() then it makes sense to do the clk_is_match() thing, and you can have my ack
Acked-by Stephen Boyd <[email protected]>

Good point. We've been unconditionally calling it anyways and nobody has complained of any issues. So let's just remove the check.

Regards,

Tony

8< ----------------------
From: Tony Lindgren <[email protected]>
Date: Tue, 5 May 2015 09:03:34 -0700
Subject: [PATCH] ARM: OMAP2+: Remove bogus struct clk comparison for timer
clock

With recent changes to use determine_rate, the comparison of two clocks won't work without clk_is_match that does __clk_get_hw on the clocks first.

As we've been unconditionally already calling clk_set_parent already because of the bogus comparison, let's just remove the check as suggested by Stephen Boyd <[email protected]>.

Cc Michael Turquette <[email protected]>

Cc Paul Walmsley <[email protected]>

Cc Stephen Boyd <[email protected]>

Cc Tero Kristo <[email protected]>

Cc Tomeu Vizoso <[email protected]>

Signed-off-by Tony Lindgren <[email protected]>

arch/arm/mach-omap2/timer.c +5 -8

@@ -298,14 +298,11 @@ static int __init omap_dm_timer_init_one(struct omap_dm_timer *timer,
 	if (IS_ERR(src))
 		return PTR_ERR(src);
 
-	if (clk_get_parent(timer->fclk) != src) {
-		r = clk_set_parent(timer->fclk, src);
-		if (r < 0) {
-			pr_warn("%s: %s cannot set source\n", __func__,
-				oh->name);
-			clk_put(src);
-			return r;
-		}
+	r = clk_set_parent(timer->fclk, src);
+	if (r < 0) {
+		pr_warn("%s: %s cannot set source\n", __func__, oh->name);
+		clk_put(src);
+		return r;
 	}
 
 	clk_put(src);

Stephen Boyd

2015-05-07 19:01 UTC #5 lore raw

PATCH 1/1 ARM: OMAP2+: Fix bogus struct clk comparison for timer clock

Quoted 41 lines · Tony Lindgren

Quoted 14 lines · Stephen Boyd
arch/arm/mach-omap2/timer.c static int __init omap_dm_timer_init_one(struct omap_dm_timer *timer, · line 302
Quoted 9 lines · Tony Lindgren
arch/arm/mach-omap2/timer.c +1 -1
@@ -298,7 +298,7 @@ static int __init omap_dm_timer_init_one(struct omap_dm_timer *timer,
 	if (IS_ERR(src))
 		return PTR_ERR(src);
 
-	if (clk_get_parent(timer->fclk) != src) {
+	if (!clk_is_match(clk_get_parent(timer->fclk), src)) {
 		r = clk_set_parent(timer->fclk, src);
I also wonder why we can't just call clk_set_parent() and skip the "is the parent already src" check?
Quoted 4 lines
If there's a good reason for not just calling clk_set_parent() then it makes sense to do the clk_is_match() thing, and you can have my ack
Acked-by Stephen Boyd <[email protected]>
Good point. We've been unconditionally calling it anyways and nobody has complained of any issues. So let's just remove the check.
Regards,
Tony
8< ----------------------
From: Tony Lindgren <[email protected]>
Date: Tue, 5 May 2015 09:03:34 -0700
Subject: [PATCH] ARM: OMAP2+: Remove bogus struct clk comparison for timer
clock
With recent changes to use determine_rate, the comparison of two clocks won't work without clk_is_match that does __clk_get_hw on the clocks first.
As we've been unconditionally already calling clk_set_parent already because of the bogus comparison, let's just remove the check as suggested by Stephen Boyd <[email protected]>.

Quoted 6 lines

Cc Michael Turquette <[email protected]>
Cc Paul Walmsley <[email protected]>
Cc Stephen Boyd <[email protected]>
Cc Tero Kristo <[email protected]>
Cc Tomeu Vizoso <[email protected]>
Signed-off-by Tony Lindgren <[email protected]>

Acked-by Stephen Boyd <[email protected]>

Signature · 2 lines

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project