Skip to contentSkip to conversation

arc: get rate from clk driver instead of reading device tree

PATCH In review Vlad Zakharov 3 participants 6 messages Feb 22, 2017 – Mar 10, 2017 Synopsys ARC architecture

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 arc: get rate from clk driver instead of reading device tree

We were reading clock rate directly from device tree "clock-frequency" property of corresponding clock node in show_cpuinfo function.

Such approach is correct only in case cpu is always clocked by "fixed-clock". If we use clock driver that allows rate to be changed this won't work as rate may change during the time or even "clock-frequency" property may not be presented at all.

So this commit replaces reading device tree with getting rate from clock driver. This approach is much more flexible and will work for both fixed and mutable clocks.

Signed-off-by Vlad Zakharov <vzakhar at synopsys.com>
---
 arch/arc/kernel/setup.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
arch/arc/kernel/setup.c +4 -2
index 3093fa8..49b0804 100644
@@ -10,6 +10,7 @@
 #include <linux/fs.h>
 #include <linux/delay.h>
 #include <linux/root_dev.h>
+#include <linux/clk.h>
 #include <linux/clk-provider.h>
 #include <linux/clocksource.h>
 #include <linux/console.h>
@@ -488,7 +489,8 @@ static int show_cpuinfo(struct seq_file *m, void *v)
 {
 	char *str;
 	int cpu_id = ptr_to_cpu(v);
-	struct device_node *core_clk = of_find_node_by_name(NULL, "core_clk");
+	struct device *cpu_dev = get_cpu_device(cpu_id);
+	struct clk *cpu_clk = clk_get(cpu_dev, "core_clk");
 	u32 freq = 0;
 
 	if (!cpu_online(cpu_id)) {
@@ -502,7 +504,7 @@ static int show_cpuinfo(struct seq_file *m, void *v)
 
 	seq_printf(m, arc_cpu_mumbojumbo(cpu_id, str, PAGE_SIZE));
 
-	of_property_read_u32(core_clk, "clock-frequency", &freq);
+	freq = clk_get_rate(cpu_clk);
 	if (freq)
 		seq_printf(m, "CPU speed\t: %u.%02u Mhz\n",
 			   freq / 1000000, (freq / 10000) % 100);
Signature · 1 line
2.7.4
#1 lore raw
Quoted 40 lines · Vlad Zakharov

We were reading clock rate directly from device tree "clock-frequency" property of corresponding clock node in show_cpuinfo function.

Such approach is correct only in case cpu is always clocked by "fixed-clock". If we use clock driver that allows rate to be changed this won't work as rate may change during the time or even "clock-frequency" property may not be presented at all.

So this commit replaces reading device tree with getting rate from clock driver. This approach is much more flexible and will work for both fixed and mutable clocks.

Signed-off-by Vlad Zakharov <vzakhar at synopsys.com>
---
 arch/arc/kernel/setup.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
arch/arc/kernel/setup.c +3 -1
index 3093fa8..49b0804 100644
@@ -10,6 +10,7 @@
 #include <linux/fs.h>
 #include <linux/delay.h>
 #include <linux/root_dev.h>
+#include <linux/clk.h>
 #include <linux/clk-provider.h>
 #include <linux/clocksource.h>
 #include <linux/console.h>
@@ -488,7 +489,8 @@ static int show_cpuinfo(struct seq_file *m, void *v)
 {
 	char *str;
 	int cpu_id = ptr_to_cpu(v);
-	struct device_node *core_clk = of_find_node_by_name(NULL, "core_clk");
+	struct device *cpu_dev = get_cpu_device(cpu_id);
+	struct clk *cpu_clk = clk_get(cpu_dev, "core_clk");
 	u32 freq = 0;
 
 	if (!cpu_online(cpu_id)) {

arch/arc/kernel/setup.c static int show_cpuinfo(struct seq_file *m, void *v) · line 510

Quoted 8 lines
arch/arc/kernel/setup.c +1 -1
@@ -502,7 +504,7 @@ static int show_cpuinfo(struct seq_file *m, void *v)
 
 	seq_printf(m, arc_cpu_mumbojumbo(cpu_id, str, PAGE_SIZE));
 
-	of_property_read_u32(core_clk, "clock-frequency", &freq);
+	freq = clk_get_rate(cpu_clk);
 	if (freq)
 		seq_printf(m, "CPU speed\t: %u.%02u Mhz\n",
 			   freq / 1000000, (freq / 10000) % 100);

I'm not too familiar with clk API, do u need to clk_put() afterwards ? @Alexey can u please check !

#2 lore raw

Hi Vineet,

Quoted 45 lines · Vineet Gupta
Quoted 44 lines · Vlad Zakharov

We were reading clock rate directly from device tree "clock-frequency" property of corresponding clock node in show_cpuinfo function.

Such approach is correct only in case cpu is always clocked by "fixed-clock". If we use clock driver that allows rate to be changed this won't work as rate may change during the time or even "clock-frequency" property may not be presented at all.

So this commit replaces reading device tree with getting rate from clock driver. This approach is much more flexible and will work for both fixed and mutable clocks.

Signed-off-by Vlad Zakharov <vzakhar at synopsys.com>

---
?arch/arc/kernel/setup.c | 6 ++++--
?1 file changed, 4 insertions(+), 2 deletions(-)

arch/arc/kernel/setup.c +0 -0
index 3093fa8..49b0804 100644
@@ -10,6 +10,7 @@

?#include <linux/fs.h>
?#include <linux/delay.h>
?#include <linux/root_dev.h>
+#include <linux/clk.h>
?#include <linux/clk-provider.h>
?#include <linux/clocksource.h>
?#include <linux/console.h>

arch/arc/kernel/setup.c +2 -1
@@ -488,7 +489,8 @@ static int show_cpuinfo(struct seq_file *m, void *v)
 ?{
 ?	char *str;
 ?	int cpu_id = ptr_to_cpu(v);
-	struct device_node *core_clk = of_find_node_by_name(NULL, "core_clk");
+	struct device *cpu_dev = get_cpu_device(cpu_id);
+	struct clk *cpu_clk = clk_get(cpu_dev, "core_clk");
 ?	u32 freq = 0;

?
? if (!cpu_online(cpu_id)) {

arch/arc/kernel/setup.c +0 -0
@@ -502,7 +504,7 @@ static int show_cpuinfo(struct seq_file *m, void *v)

?
? seq_printf(m, arc_cpu_mumbojumbo(cpu_id, str, PAGE_SIZE));
?

Quoted 8 lines
Quoted 5 lines
arch/arc/kernel/setup.c +1 -1
-	of_property_read_u32(core_clk, "clock-frequency", &freq);
+	freq = clk_get_rate(cpu_clk);

? if (freq)
? seq_printf(m, "CPU speed\t: %u.%02u Mhz\n",
? ???freq / 1000000, (freq / 10000) % 100);

I'm not too familiar with clk API, do u need to clk_put() afterwards ? @Alexey can u please check !

I'm not really sure why clk_put() got in the picture here at all.
As it is mentioned here?http://elixir.free-electrons.com/source/include/linux/clk.h?v=4.10#L305 clk_put() means "free" the clock source.

Anyways this patch alone was not sufficient due to missing changes in .dts files and Vlad is going to send v2 soonish.

-Alexey

#3 lore raw
In reply to Vlad Zakharov
Quoted 11 lines · Vlad Zakharov

We were reading clock rate directly from device tree "clock-frequency" property of corresponding clock node in show_cpuinfo function.

Such approach is correct only in case cpu is always clocked by "fixed-clock". If we use clock driver that allows rate to be changed this won't work as rate may change during the time or even "clock-frequency" property may not be presented at all.

So this commit replaces reading device tree with getting rate from clock driver. This approach is much more flexible and will work for both fixed and mutable clocks.

So except the clk_put() comment - this looks much better to me.

And while we are at it - one of my wish list has been to print the core clk in early boot - i.e. in setup_processor()->arc_cpu_mumbojumbo() call path which is different from the other case u fixed. Any ideas how to do that - clk framework will not have init by then - so we need to read out from FDT or some such !

Quoted 38 lines
Signed-off-by Vlad Zakharov <vzakhar at synopsys.com>
---
 arch/arc/kernel/setup.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
arch/arc/kernel/setup.c +4 -2
index 3093fa8..49b0804 100644
@@ -10,6 +10,7 @@
 #include <linux/fs.h>
 #include <linux/delay.h>
 #include <linux/root_dev.h>
+#include <linux/clk.h>
 #include <linux/clk-provider.h>
 #include <linux/clocksource.h>
 #include <linux/console.h>
@@ -488,7 +489,8 @@ static int show_cpuinfo(struct seq_file *m, void *v)
 {
 	char *str;
 	int cpu_id = ptr_to_cpu(v);
-	struct device_node *core_clk = of_find_node_by_name(NULL, "core_clk");
+	struct device *cpu_dev = get_cpu_device(cpu_id);
+	struct clk *cpu_clk = clk_get(cpu_dev, "core_clk");
 	u32 freq = 0;
 
 	if (!cpu_online(cpu_id)) {
@@ -502,7 +504,7 @@ static int show_cpuinfo(struct seq_file *m, void *v)
 
 	seq_printf(m, arc_cpu_mumbojumbo(cpu_id, str, PAGE_SIZE));
 
-	of_property_read_u32(core_clk, "clock-frequency", &freq);
+	freq = clk_get_rate(cpu_clk);
 	if (freq)
 		seq_printf(m, "CPU speed\t: %u.%02u Mhz\n",
 			   freq / 1000000, (freq / 10000) % 100);
#4 lore raw

PATCH 1/1 arc: get rate from clk driver instead of reading device tree

Hi Vineet,

Quoted 13 lines · Vineet Gupta
Quoted 11 lines · Vlad Zakharov

We were reading clock rate directly from device tree "clock-frequency" property of corresponding clock node in show_cpuinfo function. ?
Such approach is correct only in case cpu is always clocked by "fixed-clock". If we use clock driver that allows rate to be changed this won't work as rate may change during the time or even "clock-frequency" property may not be presented at all.
?
So this commit replaces reading device tree with getting rate from clock driver. This approach is much more flexible and will work for both fixed and mutable clocks.

Quoted 6 lines

So except the clk_put() comment - this looks much better to me.

And while we are at it - one of my wish list has been to print the core clk in early boot - i.e. in setup_processor()->arc_cpu_mumbojumbo() call path which is different from the other case u fixed. Any ideas how to do that - clk framework will not have init by then - so we need to read out from FDT or some such !

We can either parse FDT or read from real hardware - but is that the point? I thought the goal is to get rid of as much board-dependent code as possible, isn't it?

In fact there are two ways of getting frequencies before clk framework inits:?

? 1) Parse FDT - this way is not platform-dependent, but if someone adds clock nodes without "clock-frequency" property this is gonna fail. The second issue here is that device tree property is not believed to be accurate - actual value may differ. So, there are a lot of disadvantages.

? 2) Read values from real hardware: this is platform-dependent way and we will have add such "hacks" for each supported platform even if clock driver is provided for such platform. And that's why I don't like this way.

Of course clk framework initializes a bit later. But maybe we can wait for this and add just a couple of code lines that read rate using common clk API?

Signature · 2 lines
Best regards,
Vlad Zakharov <vzakhar at synopsys.com>
#5 lore raw

PATCH 1/1 arc: get rate from clk driver instead of reading device tree

Quoted 7 lines · Vlad Zakharov
Quoted 5 lines

And while we are at it - one of my wish list has been to print the core clk in early boot - i.e. in setup_processor()->arc_cpu_mumbojumbo() call path which is different from the other case u fixed. Any ideas how to do that - clk framework will not have init by then - so we need to read out from FDT or some such !

We can either parse FDT or read from real hardware - but is that the point? I thought the goal is to get rid of as much board-dependent code as possible, isn't it?

I'm not asking to add board specific code in setup.c - but we do want to print the frequency as available. Currently we only print for AXS but for some new board it will likely not be present so at the very least print what is in the DT.

So the idea would be to have a default which reads from FDT and board/platform can override it with more accurate info (from say hardware). You can start off with using weak function approach for prototyping and later use a callback approach registered by boards etc !

Quoted 11 lines

In fact there are two ways of getting frequencies before clk framework inits:

1) Parse FDT - this way is not platform-dependent, but if someone adds clock nodes without "clock-frequency" property
this is gonna fail. The second issue here is that device tree property is not believed to be accurate - actual value may differ. So, there are a lot of disadvantages.

2) Read values from real hardware: this is platform-dependent way and we will have add such "hacks" for each supported
platform even if clock driver is provided for such platform. And that's why I don't like this way.

Of course clk framework initializes a bit later. But maybe we can wait for this and add just a couple of code lines that read rate using common clk API?

I'd still prefer to see the value provided in DT and any updates since !

-Vineet
-Vineet