[PATCH v2 0/2] ARM: topology: Allow to set the frequency through a clock

[PATCH v2 0/2] ARM: topology: Allow to set the frequency through a clock

[Maxime Ripard]

Hi,

This patch series add the possibility for the topology code to get the
CPU frequency through a DT clock handle instead of needing a
clock-frequency property.

Indeed, this information can be quite redundant if the clock tree
defined in the DT is already describing the CPU parent clock.

Maxime

Changes from v1:
  - Rebased on top of 3.16
  - Reworked a bit the code in the first patch as requested by Rob
    Herring

Maxime Ripard (2):
  ARM: topology: Use a clock if possible to get the CPU frequency
  ARM: sunxi: Add clocks node to the CPU nodes

 arch/arm/boot/dts/sun6i-a31.dtsi |  4 ++++
 arch/arm/boot/dts/sun7i-a20.dtsi |  2 ++
 arch/arm/kernel/topology.c       | 25 ++++++++++++++++++-------
 3 files changed, 24 insertions(+), 7 deletions(-)

-- 
2.0.0

[PATCH v2 1/2] ARM: topology: Use a clock if possible to get the CPU frequency

[Maxime Ripard]

The Cortex-A7 and Cortex-A15 based SoCs need a clock-frequency property in the
topology code.

Allow to use a clock to provide the same information.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 arch/arm/kernel/topology.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/arch/arm/kernel/topology.c b/arch/arm/kernel/topology.c
index 9d853189028b..268443d88094 100644
--- a/arch/arm/kernel/topology.c
+++ b/arch/arm/kernel/topology.c
@@ -11,6 +11,7 @@
  * for more details.
  */
 
+#include <linux/clk.h>
 #include <linux/cpu.h>
 #include <linux/cpumask.h>
 #include <linux/export.h>
@@ -100,8 +101,8 @@ static void __init parse_dt_topology(void)
 				 GFP_NOWAIT);
 
 	for_each_possible_cpu(cpu) {
-		const u32 *rate;
-		int len;
+		struct clk *clk;
+		u32 rate = 0;
 
 		/* too early to use cpu->of_node */
 		cn = of_get_cpu_node(cpu, NULL);
@@ -117,14 +118,24 @@ static void __init parse_dt_topology(void)
 		if (cpu_eff->compatible == NULL)
 			continue;
 
-		rate = of_get_property(cn, "clock-frequency", &len);
-		if (!rate || len != 4) {
-			pr_err("%s missing clock-frequency property\n",
-				cn->full_name);
+		clk = of_clk_get(cn, 0);
+		if (!IS_ERR(clk))
+			rate = clk_get_rate(clk);
+		else
+			of_property_read_u32(cn, "clock-frequency", &rate);
+
+		if (!rate) {
+			pr_err("%s missing clocks or clock-frequency properties\n",
+			       cn->full_name);
+			continue;
+		}
+
+		if (!rate) {
+			pr_err("%s invalid CPU frequency", cn->full_name);
 			continue;
 		}
 
-		capacity = ((be32_to_cpup(rate)) >> 20) * cpu_eff->efficiency;
+		capacity = ((rate) >> 20) * cpu_eff->efficiency;
 
 		/* Save min capacity of the system */
 		if (capacity < min_capacity)
-- 
2.0.0

[PATCH v2 1/2] ARM: topology: Use a clock if possible to get the CPU frequency

[Russell King - ARM Linux]

On Mon, Jun 30, 2014 at 12:12:22PM +0200, Maxime Ripard wrote:
> The Cortex-A7 and Cortex-A15 based SoCs need a clock-frequency property in the
> topology code.
> 
> Allow to use a clock to provide the same information.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>

This looks fine to me, but I don't know this code.  Maybe Vincent Guittot
should be on the To: list (added)?

> ---
>  arch/arm/kernel/topology.c | 25 ++++++++++++++++++-------
>  1 file changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm/kernel/topology.c b/arch/arm/kernel/topology.c
> index 9d853189028b..268443d88094 100644
> --- a/arch/arm/kernel/topology.c
> +++ b/arch/arm/kernel/topology.c
> @@ -11,6 +11,7 @@
>   * for more details.
>   */
>  
> +#include <linux/clk.h>
>  #include <linux/cpu.h>
>  #include <linux/cpumask.h>
>  #include <linux/export.h>
> @@ -100,8 +101,8 @@ static void __init parse_dt_topology(void)
>  				 GFP_NOWAIT);
>  
>  	for_each_possible_cpu(cpu) {
> -		const u32 *rate;
> -		int len;
> +		struct clk *clk;
> +		u32 rate = 0;
>  
>  		/* too early to use cpu->of_node */
>  		cn = of_get_cpu_node(cpu, NULL);
> @@ -117,14 +118,24 @@ static void __init parse_dt_topology(void)
>  		if (cpu_eff->compatible == NULL)
>  			continue;
>  
> -		rate = of_get_property(cn, "clock-frequency", &len);
> -		if (!rate || len != 4) {
> -			pr_err("%s missing clock-frequency property\n",
> -				cn->full_name);
> +		clk = of_clk_get(cn, 0);
> +		if (!IS_ERR(clk))
> +			rate = clk_get_rate(clk);
> +		else
> +			of_property_read_u32(cn, "clock-frequency", &rate);
> +
> +		if (!rate) {
> +			pr_err("%s missing clocks or clock-frequency properties\n",
> +			       cn->full_name);
> +			continue;
> +		}
> +
> +		if (!rate) {
> +			pr_err("%s invalid CPU frequency", cn->full_name);
>  			continue;
>  		}
>  
> -		capacity = ((be32_to_cpup(rate)) >> 20) * cpu_eff->efficiency;
> +		capacity = ((rate) >> 20) * cpu_eff->efficiency;
>  
>  		/* Save min capacity of the system */
>  		if (capacity < min_capacity)
> -- 
> 2.0.0
> 

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

[PATCH v2 1/2] ARM: topology: Use a clock if possible to get the CPU frequency

[Vincent Guittot]

On 30 June 2014 12:29, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> On Mon, Jun 30, 2014 at 12:12:22PM +0200, Maxime Ripard wrote:
>> The Cortex-A7 and Cortex-A15 based SoCs need a clock-frequency property in the
>> topology code.
>>
>> Allow to use a clock to provide the same information.
>>
>> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
>
> This looks fine to me, but I don't know this code.  Maybe Vincent Guittot
> should be on the To: list (added)?

Thanks

>
>> ---
>>  arch/arm/kernel/topology.c | 25 ++++++++++++++++++-------
>>  1 file changed, 18 insertions(+), 7 deletions(-)
>>
>> diff --git a/arch/arm/kernel/topology.c b/arch/arm/kernel/topology.c
>> index 9d853189028b..268443d88094 100644
>> --- a/arch/arm/kernel/topology.c
>> +++ b/arch/arm/kernel/topology.c
>> @@ -11,6 +11,7 @@
>>   * for more details.
>>   */
>>
>> +#include <linux/clk.h>
>>  #include <linux/cpu.h>
>>  #include <linux/cpumask.h>
>>  #include <linux/export.h>
>> @@ -100,8 +101,8 @@ static void __init parse_dt_topology(void)
>>                                GFP_NOWAIT);
>>
>>       for_each_possible_cpu(cpu) {
>> -             const u32 *rate;
>> -             int len;
>> +             struct clk *clk;
>> +             u32 rate = 0;
>>
>>               /* too early to use cpu->of_node */
>>               cn = of_get_cpu_node(cpu, NULL);
>> @@ -117,14 +118,24 @@ static void __init parse_dt_topology(void)
>>               if (cpu_eff->compatible == NULL)
>>                       continue;
>>
>> -             rate = of_get_property(cn, "clock-frequency", &len);
>> -             if (!rate || len != 4) {
>> -                     pr_err("%s missing clock-frequency property\n",
>> -                             cn->full_name);
>> +             clk = of_clk_get(cn, 0);
>> +             if (!IS_ERR(clk))
>> +                     rate = clk_get_rate(clk);

We need the max frequency as it will be used to weight the different
CPUs capacity. How do you ensure that the current clock rate is the
max one ?

Vincent

>> +             else
>> +                     of_property_read_u32(cn, "clock-frequency", &rate);
>> +
>> +             if (!rate) {
>> +                     pr_err("%s missing clocks or clock-frequency properties\n",
>> +                            cn->full_name);
>> +                     continue;
>> +             }
>> +
>> +             if (!rate) {
>> +                     pr_err("%s invalid CPU frequency", cn->full_name);
>>                       continue;
>>               }
>>
>> -             capacity = ((be32_to_cpup(rate)) >> 20) * cpu_eff->efficiency;
>> +             capacity = ((rate) >> 20) * cpu_eff->efficiency;
>>
>>               /* Save min capacity of the system */
>>               if (capacity < min_capacity)
>> --
>> 2.0.0
>>
>
> --
> FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
> improving, and getting towards what was expected from it.

[PATCH v2 1/2] ARM: topology: Use a clock if possible to get the CPU frequency

[Maxime Ripard]

On Mon, Jun 30, 2014 at 12:39:26PM +0200, Vincent Guittot wrote:
> On 30 June 2014 12:29, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> > On Mon, Jun 30, 2014 at 12:12:22PM +0200, Maxime Ripard wrote:
> >> The Cortex-A7 and Cortex-A15 based SoCs need a clock-frequency property in the
> >> topology code.
> >>
> >> Allow to use a clock to provide the same information.
> >>
> >> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> >
> > This looks fine to me, but I don't know this code.  Maybe Vincent Guittot
> > should be on the To: list (added)?
> 
> Thanks

Ah, sorry for that.

> 
> >
> >> ---
> >>  arch/arm/kernel/topology.c | 25 ++++++++++++++++++-------
> >>  1 file changed, 18 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/arch/arm/kernel/topology.c b/arch/arm/kernel/topology.c
> >> index 9d853189028b..268443d88094 100644
> >> --- a/arch/arm/kernel/topology.c
> >> +++ b/arch/arm/kernel/topology.c
> >> @@ -11,6 +11,7 @@
> >>   * for more details.
> >>   */
> >>
> >> +#include <linux/clk.h>
> >>  #include <linux/cpu.h>
> >>  #include <linux/cpumask.h>
> >>  #include <linux/export.h>
> >> @@ -100,8 +101,8 @@ static void __init parse_dt_topology(void)
> >>                                GFP_NOWAIT);
> >>
> >>       for_each_possible_cpu(cpu) {
> >> -             const u32 *rate;
> >> -             int len;
> >> +             struct clk *clk;
> >> +             u32 rate = 0;
> >>
> >>               /* too early to use cpu->of_node */
> >>               cn = of_get_cpu_node(cpu, NULL);
> >> @@ -117,14 +118,24 @@ static void __init parse_dt_topology(void)
> >>               if (cpu_eff->compatible == NULL)
> >>                       continue;
> >>
> >> -             rate = of_get_property(cn, "clock-frequency", &len);
> >> -             if (!rate || len != 4) {
> >> -                     pr_err("%s missing clock-frequency property\n",
> >> -                             cn->full_name);
> >> +             clk = of_clk_get(cn, 0);
> >> +             if (!IS_ERR(clk))
> >> +                     rate = clk_get_rate(clk);
> 
> We need the max frequency as it will be used to weight the different
> CPUs capacity. How do you ensure that the current clock rate is the
> max one ?

Hmm, the clock-frequency attribute in the ePAPR is defined at the
current CPU frequency, not the max one.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140630/72cfdc8d/attachment-0001.sig>

[PATCH v2 1/2] ARM: topology: Use a clock if possible to get the CPU frequency

[Vincent Guittot]

On 30 June 2014 14:49, Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
> On Mon, Jun 30, 2014 at 12:39:26PM +0200, Vincent Guittot wrote:
>> On 30 June 2014 12:29, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
>> > On Mon, Jun 30, 2014 at 12:12:22PM +0200, Maxime Ripard wrote:
>> >> The Cortex-A7 and Cortex-A15 based SoCs need a clock-frequency property in the
>> >> topology code.
>> >>
>> >> Allow to use a clock to provide the same information.
>> >>
>> >> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
>> >
>> > This looks fine to me, but I don't know this code.  Maybe Vincent Guittot
>> > should be on the To: list (added)?
>>
>> Thanks
>
> Ah, sorry for that.
>
>>
>> >
>> >> ---
>> >>  arch/arm/kernel/topology.c | 25 ++++++++++++++++++-------
>> >>  1 file changed, 18 insertions(+), 7 deletions(-)
>> >>
>> >> diff --git a/arch/arm/kernel/topology.c b/arch/arm/kernel/topology.c
>> >> index 9d853189028b..268443d88094 100644
>> >> --- a/arch/arm/kernel/topology.c
>> >> +++ b/arch/arm/kernel/topology.c
>> >> @@ -11,6 +11,7 @@
>> >>   * for more details.
>> >>   */
>> >>
>> >> +#include <linux/clk.h>
>> >>  #include <linux/cpu.h>
>> >>  #include <linux/cpumask.h>
>> >>  #include <linux/export.h>
>> >> @@ -100,8 +101,8 @@ static void __init parse_dt_topology(void)
>> >>                                GFP_NOWAIT);
>> >>
>> >>       for_each_possible_cpu(cpu) {
>> >> -             const u32 *rate;
>> >> -             int len;
>> >> +             struct clk *clk;
>> >> +             u32 rate = 0;
>> >>
>> >>               /* too early to use cpu->of_node */
>> >>               cn = of_get_cpu_node(cpu, NULL);
>> >> @@ -117,14 +118,24 @@ static void __init parse_dt_topology(void)
>> >>               if (cpu_eff->compatible == NULL)
>> >>                       continue;
>> >>
>> >> -             rate = of_get_property(cn, "clock-frequency", &len);
>> >> -             if (!rate || len != 4) {
>> >> -                     pr_err("%s missing clock-frequency property\n",
>> >> -                             cn->full_name);
>> >> +             clk = of_clk_get(cn, 0);
>> >> +             if (!IS_ERR(clk))
>> >> +                     rate = clk_get_rate(clk);
>>
>> We need the max frequency as it will be used to weight the different
>> CPUs capacity. How do you ensure that the current clock rate is the
>> max one ?
>
> Hmm, the clock-frequency attribute in the ePAPR is defined at the
> current CPU frequency, not the max one.

What means current frequency in device tree when DVFS is involved ?

Vincent

>
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com

[PATCH v2 1/2] ARM: topology: Use a clock if possible to get the CPU frequency

[Maxime Ripard]

On Mon, Jun 30, 2014 at 03:27:21PM +0200, Vincent Guittot wrote:
> >> >> -             rate = of_get_property(cn, "clock-frequency", &len);
> >> >> -             if (!rate || len != 4) {
> >> >> -                     pr_err("%s missing clock-frequency property\n",
> >> >> -                             cn->full_name);
> >> >> +             clk = of_clk_get(cn, 0);
> >> >> +             if (!IS_ERR(clk))
> >> >> +                     rate = clk_get_rate(clk);
> >>
> >> We need the max frequency as it will be used to weight the different
> >> CPUs capacity. How do you ensure that the current clock rate is the
> >> max one ?
> >
> > Hmm, the clock-frequency attribute in the ePAPR is defined at the
> > current CPU frequency, not the max one.
> 
> What means current frequency in device tree when DVFS is involved ?

The ePAPR states that clock-frequency is supposed to be "the current
clock speed of the CPU in Hertz". It's exactly what my patch add.

Now, you're right, DVFS would be an issue here with clock-frequency,
but this patch actually makes it easier to deal with, since you only
get a reference to a clock, and you can get its rate at any given
time.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140630/3955e656/attachment.sig>

[PATCH v2 1/2] ARM: topology: Use a clock if possible to get the CPU frequency

[Vincent Guittot]

On 30 June 2014 16:01, Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
> On Mon, Jun 30, 2014 at 03:27:21PM +0200, Vincent Guittot wrote:
>> >> >> -             rate = of_get_property(cn, "clock-frequency", &len);
>> >> >> -             if (!rate || len != 4) {
>> >> >> -                     pr_err("%s missing clock-frequency property\n",
>> >> >> -                             cn->full_name);
>> >> >> +             clk = of_clk_get(cn, 0);
>> >> >> +             if (!IS_ERR(clk))
>> >> >> +                     rate = clk_get_rate(clk);
>> >>
>> >> We need the max frequency as it will be used to weight the different
>> >> CPUs capacity. How do you ensure that the current clock rate is the
>> >> max one ?
>> >
>> > Hmm, the clock-frequency attribute in the ePAPR is defined at the
>> > current CPU frequency, not the max one.
>>
>> What means current frequency in device tree when DVFS is involved ?
>
> The ePAPR states that clock-frequency is supposed to be "the current
> clock speed of the CPU in Hertz". It's exactly what my patch add.
>
> Now, you're right, DVFS would be an issue here with clock-frequency,
> but this patch actually makes it easier to deal with, since you only
> get a reference to a clock, and you can get its rate at any given
> time.

and what about using clk_round_rate(clk, ULONG_MAX) ?

We will not be dependent of when we parse DT

Vincent


>
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com

[PATCH v2 1/2] ARM: topology: Use a clock if possible to get the CPU frequency

[Maxime Ripard]

On Mon, Jun 30, 2014 at 04:48:35PM +0200, Vincent Guittot wrote:
> On 30 June 2014 16:01, Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
> > On Mon, Jun 30, 2014 at 03:27:21PM +0200, Vincent Guittot wrote:
> >> >> >> -             rate = of_get_property(cn, "clock-frequency", &len);
> >> >> >> -             if (!rate || len != 4) {
> >> >> >> -                     pr_err("%s missing clock-frequency property\n",
> >> >> >> -                             cn->full_name);
> >> >> >> +             clk = of_clk_get(cn, 0);
> >> >> >> +             if (!IS_ERR(clk))
> >> >> >> +                     rate = clk_get_rate(clk);
> >> >>
> >> >> We need the max frequency as it will be used to weight the different
> >> >> CPUs capacity. How do you ensure that the current clock rate is the
> >> >> max one ?
> >> >
> >> > Hmm, the clock-frequency attribute in the ePAPR is defined at the
> >> > current CPU frequency, not the max one.
> >>
> >> What means current frequency in device tree when DVFS is involved ?
> >
> > The ePAPR states that clock-frequency is supposed to be "the current
> > clock speed of the CPU in Hertz". It's exactly what my patch add.
> >
> > Now, you're right, DVFS would be an issue here with clock-frequency,
> > but this patch actually makes it easier to deal with, since you only
> > get a reference to a clock, and you can get its rate at any given
> > time.
> 
> and what about using clk_round_rate(clk, ULONG_MAX) ?

Well, the clock itself might generate a frequency higher that what the
CPU can run at, so I'm not sure it's a valid assumption.

> We will not be dependent of when we parse DT

You lost me there. clk_round_rate and clk_get_rate are available at
the same time in the boot process, aren't they?

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140630/6b84a52e/attachment.sig>

[PATCH v2 1/2] ARM: topology: Use a clock if possible to get the CPU frequency

[Vincent Guittot]

On 30 June 2014 16:58, Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
> On Mon, Jun 30, 2014 at 04:48:35PM +0200, Vincent Guittot wrote:
>> On 30 June 2014 16:01, Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
>> > On Mon, Jun 30, 2014 at 03:27:21PM +0200, Vincent Guittot wrote:
>> >> >> >> -             rate = of_get_property(cn, "clock-frequency", &len);
>> >> >> >> -             if (!rate || len != 4) {
>> >> >> >> -                     pr_err("%s missing clock-frequency property\n",
>> >> >> >> -                             cn->full_name);
>> >> >> >> +             clk = of_clk_get(cn, 0);
>> >> >> >> +             if (!IS_ERR(clk))
>> >> >> >> +                     rate = clk_get_rate(clk);
>> >> >>
>> >> >> We need the max frequency as it will be used to weight the different
>> >> >> CPUs capacity. How do you ensure that the current clock rate is the
>> >> >> max one ?
>> >> >
>> >> > Hmm, the clock-frequency attribute in the ePAPR is defined at the
>> >> > current CPU frequency, not the max one.
>> >>
>> >> What means current frequency in device tree when DVFS is involved ?
>> >
>> > The ePAPR states that clock-frequency is supposed to be "the current
>> > clock speed of the CPU in Hertz". It's exactly what my patch add.
>> >
>> > Now, you're right, DVFS would be an issue here with clock-frequency,
>> > but this patch actually makes it easier to deal with, since you only
>> > get a reference to a clock, and you can get its rate at any given
>> > time.
>>
>> and what about using clk_round_rate(clk, ULONG_MAX) ?
>
> Well, the clock itself might generate a frequency higher that what the
> CPU can run at, so I'm not sure it's a valid assumption.

yes, you're right

>
>> We will not be dependent of when we parse DT
>
> You lost me there. clk_round_rate and clk_get_rate are available at
> the same time in the boot process, aren't they?

yes, but  clk_round_rate(clk, ULONG_MAX) will return the max frequency
and not the current one. But as you mentioned, it doesn't ensure that
it's a possible frequency for the core

>
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com

[PATCH v2 1/2] ARM: topology: Use a clock if possible to get the CPU frequency

[Maxime Ripard]

On Mon, Jun 30, 2014 at 05:06:16PM +0200, Vincent Guittot wrote:
> On 30 June 2014 16:58, Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
> > On Mon, Jun 30, 2014 at 04:48:35PM +0200, Vincent Guittot wrote:
> >> On 30 June 2014 16:01, Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
> >> > On Mon, Jun 30, 2014 at 03:27:21PM +0200, Vincent Guittot wrote:
> >> >> >> >> -             rate = of_get_property(cn, "clock-frequency", &len);
> >> >> >> >> -             if (!rate || len != 4) {
> >> >> >> >> -                     pr_err("%s missing clock-frequency property\n",
> >> >> >> >> -                             cn->full_name);
> >> >> >> >> +             clk = of_clk_get(cn, 0);
> >> >> >> >> +             if (!IS_ERR(clk))
> >> >> >> >> +                     rate = clk_get_rate(clk);
> >> >> >>
> >> >> >> We need the max frequency as it will be used to weight the different
> >> >> >> CPUs capacity. How do you ensure that the current clock rate is the
> >> >> >> max one ?
> >> >> >
> >> >> > Hmm, the clock-frequency attribute in the ePAPR is defined at the
> >> >> > current CPU frequency, not the max one.
> >> >>
> >> >> What means current frequency in device tree when DVFS is involved ?
> >> >
> >> > The ePAPR states that clock-frequency is supposed to be "the current
> >> > clock speed of the CPU in Hertz". It's exactly what my patch add.
> >> >
> >> > Now, you're right, DVFS would be an issue here with clock-frequency,
> >> > but this patch actually makes it easier to deal with, since you only
> >> > get a reference to a clock, and you can get its rate at any given
> >> > time.
> >>
> >> and what about using clk_round_rate(clk, ULONG_MAX) ?
> >
> > Well, the clock itself might generate a frequency higher that what the
> > CPU can run at, so I'm not sure it's a valid assumption.
> 
> yes, you're right
> 
> >
> >> We will not be dependent of when we parse DT
> >
> > You lost me there. clk_round_rate and clk_get_rate are available at
> > the same time in the boot process, aren't they?
> 
> yes, but  clk_round_rate(clk, ULONG_MAX) will return the max frequency
> and not the current one. But as you mentioned, it doesn't ensure that
> it's a possible frequency for the core

Is this an Acked-by ?

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140703/491ce4a6/attachment.sig>

[PATCH v2 1/2] ARM: topology: Use a clock if possible to get the CPU frequency

[Vincent Guittot]

On 3 July 2014 09:10, Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
> On Mon, Jun 30, 2014 at 05:06:16PM +0200, Vincent Guittot wrote:
>> On 30 June 2014 16:58, Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
>> > On Mon, Jun 30, 2014 at 04:48:35PM +0200, Vincent Guittot wrote:
>> >> On 30 June 2014 16:01, Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
>> >> > On Mon, Jun 30, 2014 at 03:27:21PM +0200, Vincent Guittot wrote:
>> >> >> >> >> -             rate = of_get_property(cn, "clock-frequency", &len);
>> >> >> >> >> -             if (!rate || len != 4) {
>> >> >> >> >> -                     pr_err("%s missing clock-frequency property\n",
>> >> >> >> >> -                             cn->full_name);
>> >> >> >> >> +             clk = of_clk_get(cn, 0);
>> >> >> >> >> +             if (!IS_ERR(clk))
>> >> >> >> >> +                     rate = clk_get_rate(clk);
>> >> >> >>
>> >> >> >> We need the max frequency as it will be used to weight the different
>> >> >> >> CPUs capacity. How do you ensure that the current clock rate is the
>> >> >> >> max one ?
>> >> >> >
>> >> >> > Hmm, the clock-frequency attribute in the ePAPR is defined at the
>> >> >> > current CPU frequency, not the max one.
>> >> >>
>> >> >> What means current frequency in device tree when DVFS is involved ?
>> >> >
>> >> > The ePAPR states that clock-frequency is supposed to be "the current
>> >> > clock speed of the CPU in Hertz". It's exactly what my patch add.
>> >> >
>> >> > Now, you're right, DVFS would be an issue here with clock-frequency,
>> >> > but this patch actually makes it easier to deal with, since you only
>> >> > get a reference to a clock, and you can get its rate at any given
>> >> > time.
>> >>
>> >> and what about using clk_round_rate(clk, ULONG_MAX) ?
>> >
>> > Well, the clock itself might generate a frequency higher that what the
>> > CPU can run at, so I'm not sure it's a valid assumption.
>>
>> yes, you're right
>>
>> >
>> >> We will not be dependent of when we parse DT
>> >
>> > You lost me there. clk_round_rate and clk_get_rate are available at
>> > the same time in the boot process, aren't they?
>>
>> yes, but  clk_round_rate(clk, ULONG_MAX) will return the max frequency
>> and not the current one. But as you mentioned, it doesn't ensure that
>> it's a possible frequency for the core
>
> Is this an Acked-by ?

 I still think that using the current rate with clk_get_rate is not a
good solution.

Could you give us more details about why you nee to use current clock ?

AFAICT, your patch only makes sense for HMP system which don't have
DVFS, is it your case ?

Vincent

>
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com

[PATCH v2 1/2] ARM: topology: Use a clock if possible to get the CPU frequency

[Maxime Ripard]

On Thu, Jul 03, 2014 at 09:51:27AM +0200, Vincent Guittot wrote:
> On 3 July 2014 09:10, Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
> > On Mon, Jun 30, 2014 at 05:06:16PM +0200, Vincent Guittot wrote:
> >> On 30 June 2014 16:58, Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
> >> > On Mon, Jun 30, 2014 at 04:48:35PM +0200, Vincent Guittot wrote:
> >> >> On 30 June 2014 16:01, Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
> >> >> > On Mon, Jun 30, 2014 at 03:27:21PM +0200, Vincent Guittot wrote:
> >> >> >> >> >> -             rate = of_get_property(cn, "clock-frequency", &len);
> >> >> >> >> >> -             if (!rate || len != 4) {
> >> >> >> >> >> -                     pr_err("%s missing clock-frequency property\n",
> >> >> >> >> >> -                             cn->full_name);
> >> >> >> >> >> +             clk = of_clk_get(cn, 0);
> >> >> >> >> >> +             if (!IS_ERR(clk))
> >> >> >> >> >> +                     rate = clk_get_rate(clk);
> >> >> >> >>
> >> >> >> >> We need the max frequency as it will be used to weight the different
> >> >> >> >> CPUs capacity. How do you ensure that the current clock rate is the
> >> >> >> >> max one ?
> >> >> >> >
> >> >> >> > Hmm, the clock-frequency attribute in the ePAPR is defined at the
> >> >> >> > current CPU frequency, not the max one.
> >> >> >>
> >> >> >> What means current frequency in device tree when DVFS is involved ?
> >> >> >
> >> >> > The ePAPR states that clock-frequency is supposed to be "the current
> >> >> > clock speed of the CPU in Hertz". It's exactly what my patch add.
> >> >> >
> >> >> > Now, you're right, DVFS would be an issue here with clock-frequency,
> >> >> > but this patch actually makes it easier to deal with, since you only
> >> >> > get a reference to a clock, and you can get its rate at any given
> >> >> > time.
> >> >>
> >> >> and what about using clk_round_rate(clk, ULONG_MAX) ?
> >> >
> >> > Well, the clock itself might generate a frequency higher that what the
> >> > CPU can run at, so I'm not sure it's a valid assumption.
> >>
> >> yes, you're right
> >>
> >> >
> >> >> We will not be dependent of when we parse DT
> >> >
> >> > You lost me there. clk_round_rate and clk_get_rate are available at
> >> > the same time in the boot process, aren't they?
> >>
> >> yes, but  clk_round_rate(clk, ULONG_MAX) will return the max frequency
> >> and not the current one. But as you mentioned, it doesn't ensure that
> >> it's a possible frequency for the core
> >
> > Is this an Acked-by ?
> 
>  I still think that using the current rate with clk_get_rate is not a
> good solution.

Well, it's just as good as using clock-frequency, really. If you want
the CPU max frequency, it's not the right property (plus, since the
changed behaviour is not documented in Linux anywhere, the only
reference we have is the ePAPR on this).

> Could you give us more details about why you nee to use current clock ?

Honestly, I just want to remove that big warning at boot on the
A7-based SoCs we have :)

> AFAICT, your patch only makes sense for HMP system which don't have
> DVFS, is it your case ?

Nope, it's just plain simple SMP.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140704/4224dc53/attachment.sig>

[PATCH v2 1/2] ARM: topology: Use a clock if possible to get the CPU frequency

[Vincent Guittot]

On 4 July 2014 10:02, Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
> On Thu, Jul 03, 2014 at 09:51:27AM +0200, Vincent Guittot wrote:
>> On 3 July 2014 09:10, Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
>> > On Mon, Jun 30, 2014 at 05:06:16PM +0200, Vincent Guittot wrote:
>> >> On 30 June 2014 16:58, Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
>> >> > On Mon, Jun 30, 2014 at 04:48:35PM +0200, Vincent Guittot wrote:
>> >> >> On 30 June 2014 16:01, Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
>> >> >> > On Mon, Jun 30, 2014 at 03:27:21PM +0200, Vincent Guittot wrote:
>> >> >> >> >> >> -             rate = of_get_property(cn, "clock-frequency", &len);
>> >> >> >> >> >> -             if (!rate || len != 4) {
>> >> >> >> >> >> -                     pr_err("%s missing clock-frequency property\n",
>> >> >> >> >> >> -                             cn->full_name);
>> >> >> >> >> >> +             clk = of_clk_get(cn, 0);
>> >> >> >> >> >> +             if (!IS_ERR(clk))
>> >> >> >> >> >> +                     rate = clk_get_rate(clk);
>> >> >> >> >>
>> >> >> >> >> We need the max frequency as it will be used to weight the different
>> >> >> >> >> CPUs capacity. How do you ensure that the current clock rate is the
>> >> >> >> >> max one ?
>> >> >> >> >
>> >> >> >> > Hmm, the clock-frequency attribute in the ePAPR is defined at the
>> >> >> >> > current CPU frequency, not the max one.
>> >> >> >>
>> >> >> >> What means current frequency in device tree when DVFS is involved ?
>> >> >> >
>> >> >> > The ePAPR states that clock-frequency is supposed to be "the current
>> >> >> > clock speed of the CPU in Hertz". It's exactly what my patch add.
>> >> >> >
>> >> >> > Now, you're right, DVFS would be an issue here with clock-frequency,
>> >> >> > but this patch actually makes it easier to deal with, since you only
>> >> >> > get a reference to a clock, and you can get its rate at any given
>> >> >> > time.
>> >> >>
>> >> >> and what about using clk_round_rate(clk, ULONG_MAX) ?
>> >> >
>> >> > Well, the clock itself might generate a frequency higher that what the
>> >> > CPU can run at, so I'm not sure it's a valid assumption.
>> >>
>> >> yes, you're right
>> >>
>> >> >
>> >> >> We will not be dependent of when we parse DT
>> >> >
>> >> > You lost me there. clk_round_rate and clk_get_rate are available at
>> >> > the same time in the boot process, aren't they?
>> >>
>> >> yes, but  clk_round_rate(clk, ULONG_MAX) will return the max frequency
>> >> and not the current one. But as you mentioned, it doesn't ensure that
>> >> it's a possible frequency for the core
>> >
>> > Is this an Acked-by ?
>>
>>  I still think that using the current rate with clk_get_rate is not a
>> good solution.
>
> Well, it's just as good as using clock-frequency, really. If you want
> the CPU max frequency, it's not the right property (plus, since the
> changed behaviour is not documented in Linux anywhere, the only
> reference we have is the ePAPR on this).
>
>> Could you give us more details about why you nee to use current clock ?
>
> Honestly, I just want to remove that big warning at boot on the
> A7-based SoCs we have :)

so Why not changing the log message ? It's clearly not an error to not
have clock-frequency

>
>> AFAICT, your patch only makes sense for HMP system which don't have
>> DVFS, is it your case ?
>
> Nope, it's just plain simple SMP.
>
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com

[PATCH v2 1/2] ARM: topology: Use a clock if possible to get the CPU frequency

[Maxime Ripard]

On Fri, Jul 04, 2014 at 11:22:33AM +0200, Vincent Guittot wrote:
> >>  I still think that using the current rate with clk_get_rate is not a
> >> good solution.
> >
> > Well, it's just as good as using clock-frequency, really. If you want
> > the CPU max frequency, it's not the right property (plus, since the
> > changed behaviour is not documented in Linux anywhere, the only
> > reference we have is the ePAPR on this).
> >
> >> Could you give us more details about why you nee to use current clock ?
> >
> > Honestly, I just want to remove that big warning at boot on the
> > A7-based SoCs we have :)
> 
> so Why not changing the log message ? It's clearly not an error to not
> have clock-frequency

Because the next SoCs we're going to support will be HMP, so we're
only delaying the issue by a few month if we're doing so.

It doesn't fix the underlying issue where you're using a standard
property for a (slightly) different usage.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140708/32eb41b8/attachment.sig>

[PATCH v2 2/2] ARM: sunxi: Add clocks node to the CPU nodes

[Maxime Ripard]

Setting the clock will make the topology code work, and will remove the
following error at boot

[    0.097194] /cpus/cpu at 0 missing clock-frequency property
[    0.103657] /cpus/cpu at 1 missing clock-frequency property
[    0.110698] /cpus/cpu at 2 missing clock-frequency property
[    0.117132] /cpus/cpu at 3 missing clock-frequency property

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 arch/arm/boot/dts/sun6i-a31.dtsi | 4 ++++
 arch/arm/boot/dts/sun7i-a20.dtsi | 2 ++
 2 files changed, 6 insertions(+)

diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi
index a9dfa12eb735..905b84add002 100644
--- a/arch/arm/boot/dts/sun6i-a31.dtsi
+++ b/arch/arm/boot/dts/sun6i-a31.dtsi
@@ -34,24 +34,28 @@
 		cpu at 0 {
 			compatible = "arm,cortex-a7";
 			device_type = "cpu";
+			clocks = <&cpu>;
 			reg = <0>;
 		};
 
 		cpu at 1 {
 			compatible = "arm,cortex-a7";
 			device_type = "cpu";
+			clocks = <&cpu>;
 			reg = <1>;
 		};
 
 		cpu at 2 {
 			compatible = "arm,cortex-a7";
 			device_type = "cpu";
+			clocks = <&cpu>;
 			reg = <2>;
 		};
 
 		cpu at 3 {
 			compatible = "arm,cortex-a7";
 			device_type = "cpu";
+			clocks = <&cpu>;
 			reg = <3>;
 		};
 	};
diff --git a/arch/arm/boot/dts/sun7i-a20.dtsi b/arch/arm/boot/dts/sun7i-a20.dtsi
index 17823759bdc9..1d263b7f02dc 100644
--- a/arch/arm/boot/dts/sun7i-a20.dtsi
+++ b/arch/arm/boot/dts/sun7i-a20.dtsi
@@ -35,12 +35,14 @@
 		cpu at 0 {
 			compatible = "arm,cortex-a7";
 			device_type = "cpu";
+			clocks = <&cpu>;
 			reg = <0>;
 		};
 
 		cpu at 1 {
 			compatible = "arm,cortex-a7";
 			device_type = "cpu";
+			clocks = <&cpu>;
 			reg = <1>;
 		};
 	};
-- 
2.0.0