[PATCH RFC] Add cpufreq support

[PATCH RFC] Add cpufreq support

[Mason]

I'm throwing this out there to ask:
Is this the right way to enable cpufreq on my platform?
---
 arch/arm/boot/dts/tango4-common.dtsi  | 4 ++++
 arch/arm/boot/dts/tango4-smp8758.dtsi | 2 ++
 arch/arm/mach-tango/setup.c           | 7 +++++++
 3 files changed, 13 insertions(+)

diff --git a/arch/arm/boot/dts/tango4-common.dtsi b/arch/arm/boot/dts/tango4-common.dtsi
index 41dff0a93419..557cb67d183f 100644
--- a/arch/arm/boot/dts/tango4-common.dtsi
+++ b/arch/arm/boot/dts/tango4-common.dtsi
@@ -193,3 +193,7 @@
 		};
 	};
 };
+
+&cpu0 {
+	clocks = <&clkgen CPU_CLK>;
+};
diff --git a/arch/arm/boot/dts/tango4-smp8758.dtsi b/arch/arm/boot/dts/tango4-smp8758.dtsi
index 7ed88ee629fb..0db290a8334d 100644
--- a/arch/arm/boot/dts/tango4-smp8758.dtsi
+++ b/arch/arm/boot/dts/tango4-smp8758.dtsi
@@ -11,6 +11,8 @@
 			next-level-cache = <&l2cc>;
 			device_type = "cpu";
 			reg = <0>;
+			clock-latency = <300000>;
+			operating-points = <1215000 0 607500 0 405000 0 243000 0 135000 0>;
 		};
 
 		cpu1: cpu at 1 {
diff --git a/arch/arm/mach-tango/setup.c b/arch/arm/mach-tango/setup.c
index a796841c039b..0e8f90f70e85 100644
--- a/arch/arm/mach-tango/setup.c
+++ b/arch/arm/mach-tango/setup.c
@@ -1,6 +1,7 @@
 #include <asm/mach/map.h>
 #include <asm/mach/arch.h>
 #include <asm/hardware/cache-l2x0.h>
+#include <linux/platform_device.h>
 #include "smc.h"
 
 static struct map_desc tango_map_desc[] __initdata = {
@@ -23,6 +24,11 @@ static void tango_l2c_write(unsigned long val, unsigned int reg)
 		tango_set_l2_control(val);
 }
 
+static void __init tango_init_late(void)
+{
+	platform_device_register_simple("cpufreq-dt", -1, NULL, 0);
+}
+
 static const char *const tango_dt_compat[] = { "sigma,tango4", NULL };
 
 DT_MACHINE_START(TANGO_DT, "Sigma Tango DT")
@@ -30,4 +36,5 @@ DT_MACHINE_START(TANGO_DT, "Sigma Tango DT")
 	.l2c_aux_mask	= ~0,
 	.l2c_write_sec	= tango_l2c_write,
 	.map_io		= tango_map_io,
+	.init_late	= tango_init_late,
 MACHINE_END

[PATCH RFC] Add cpufreq support

[Arnd Bergmann]

On Friday 05 February 2016 17:58:38 Mason wrote:
> I'm throwing this out there to ask:
> Is this the right way to enable cpufreq on my platform?
> ---

> @@ -23,6 +24,11 @@ static void tango_l2c_write(unsigned long val, unsigned int reg)
>  		tango_set_l2_control(val);
>  }
>  
> +static void __init tango_init_late(void)
> +{
> +	platform_device_register_simple("cpufreq-dt", -1, NULL, 0);
> +}
> +
>  static const char *const tango_dt_compat[] = { "sigma,tango4", NULL };
>  
>  DT_MACHINE_START(TANGO_DT, "Sigma Tango DT")
> @@ -30,4 +36,5 @@ DT_MACHINE_START(TANGO_DT, "Sigma Tango DT")
>  	.l2c_aux_mask	= ~0,
>  	.l2c_write_sec	= tango_l2c_write,
>  	.map_io		= tango_map_io,
> +	.init_late	= tango_init_late,
>  MACHINE_END
> 

We no longer call platform_device_register_simple() from platform code, at least
for new platforms, and we should probably remove the code from the existing
platforms that still do it. I forget what the replacement was, but I'm not
going to take this version. Viresh should be able to help you do it the right
way.

	Arnd

[PATCH RFC] Add cpufreq support

[Viresh Kumar]

On 05-02-16, 23:24, Arnd Bergmann wrote:
> We no longer call platform_device_register_simple() from platform code, at least
> for new platforms, and we should probably remove the code from the existing
> platforms that still do it. I forget what the replacement was, but I'm not
> going to take this version. Viresh should be able to help you do it the right
> way.

We thought initially that opp-v2's compatible string can be used to probe the
drivers, but that was denied later and all we do today is add platform devices
for cpufreq.

What do you have in mind Arnd ?

-- 
viresh

[PATCH RFC] Add cpufreq support

[Arnd Bergmann]

On Sunday 07 February 2016 17:52:12 Viresh Kumar wrote:
> On 05-02-16, 23:24, Arnd Bergmann wrote:
> > We no longer call platform_device_register_simple() from platform code, at least
> > for new platforms, and we should probably remove the code from the existing
> > platforms that still do it. I forget what the replacement was, but I'm not
> > going to take this version. Viresh should be able to help you do it the right
> > way.
> 
> We thought initially that opp-v2's compatible string can be used to probe the
> drivers, but that was denied later and all we do today is add platform devices
> for cpufreq.
> 
> What do you have in mind Arnd ?
> 

Whatever you want to do in drivers/cpufreq that keeps this out of arch/arm/

As I have said numerous times, there is absolutely no point in having a
platform device for this, but if you insist on having one, just write one
file that has an early_initcall() function and move all the code creating
those devices in there for platforms using DT, e.g. by matching on the
root compatible string the same way the platform code does today.

For new platforms, please come up with a way to not need that and create
a generic binding that anyone can follow.

	Arnd

[PATCH RFC] Add cpufreq support

[Viresh Kumar]

On 08-02-16, 13:24, Arnd Bergmann wrote:
> Whatever you want to do in drivers/cpufreq that keeps this out of arch/arm/
> 
> As I have said numerous times, there is absolutely no point in having a
> platform device for this, but if you insist on having one

No, I don't insist on that. I just hate it.

But the problem was that we never agreed to a way, by which we could
have probed cpufreq-dt. We were talking about compatible string from
'opp-v2' earlier, but that was soon discarded.

> just write one
> file that has an early_initcall() function and move all the code creating
> those devices in there for platforms using DT, e.g. by matching on the
> root compatible string the same way the platform code does today.
>
> For new platforms, please come up with a way to not need that and create
> a generic binding that anyone can follow.

Do you have any suggestions ?
- We aren't allowed to (re)use opp-v2 compatibility string
- We can't add a DT node for virtual device - cpufreq

What else can be done ?

-- 
viresh

[PATCH RFC] Add cpufreq support

[Arnd Bergmann]

On Monday 08 February 2016 18:01:12 Viresh Kumar wrote:
> On 08-02-16, 13:24, Arnd Bergmann wrote:
> > Whatever you want to do in drivers/cpufreq that keeps this out of arch/arm/
> > 
> > As I have said numerous times, there is absolutely no point in having a
> > platform device for this, but if you insist on having one
> 
> No, I don't insist on that. I just hate it.
> 
> But the problem was that we never agreed to a way, by which we could
> have probed cpufreq-dt. We were talking about compatible string from
> 'opp-v2' earlier, but that was soon discarded.
> 
> > just write one
> > file that has an early_initcall() function and move all the code creating
> > those devices in there for platforms using DT, e.g. by matching on the
> > root compatible string the same way the platform code does today.
> >
> > For new platforms, please come up with a way to not need that and create
> > a generic binding that anyone can follow.
> 
> Do you have any suggestions ?
> - We aren't allowed to (re)use opp-v2 compatibility string
> - We can't add a DT node for virtual device - cpufreq
> 
> What else can be done ?

Maybe add a opp-v3 compatible string? I really don't care what you
match on, as long we don't need any code in arch/arm/ to create a
device we don't need.

Don't add the device to DT, we really don't want that. If there
is too much opposition to looking at the cpus nodes in the initcall,
start with a whitelist for known machines, that at least keeps the
existing behavior.

	Arnd

[PATCH RFC] Add cpufreq support

[Viresh Kumar]

On 08-02-16, 13:34, Arnd Bergmann wrote:
> Maybe add a opp-v3 compatible string?

How will that help?

The problem was that the compatibility string of "opp-v2" specifies
the way we need to parse the bindings and shouldn't be (ab)used to
probe a driver like cpufreq-dt. And so we got stuck.

> I really don't care what you
> match on, as long we don't need any code in arch/arm/ to create a
> device we don't need.

Sure.

> Don't add the device to DT, we really don't want that.

I agree.

> If there
> is too much opposition to looking at the cpus nodes in the initcall,

I didn't get this one, what can we do by looking at CPUs nodes ?

> start with a whitelist for known machines, that at least keeps the
> existing behavior.

That can be a valid solution I would say, but that separate driver
(cpufreq-dt-device.c) needs to be changed for every new platform.

-- 
viresh

[PATCH RFC] Add cpufreq support

[Arnd Bergmann]

On Monday 08 February 2016 18:11:27 Viresh Kumar wrote:
> On 08-02-16, 13:34, Arnd Bergmann wrote:
> > Maybe add a opp-v3 compatible string?
> 
> How will that help?
> 
> The problem was that the compatibility string of "opp-v2" specifies
> the way we need to parse the bindings and shouldn't be (ab)used to
> probe a driver like cpufreq-dt. And so we got stuck.

I don't remember the exact discussion, but the compatible string is
exactly meant to do one thing: it tells you what you can or cannot do
with one device.

I had not realized that we don't even have a compatible string
for opp-v2, so if we are missing that, we obviously can't compare
against that string.

> > I really don't care what you
> > match on, as long we don't need any code in arch/arm/ to create a
> > device we don't need.
> 
> Sure.
> 
> > Don't add the device to DT, we really don't want that.
> 
> I agree.
> 
> > If there
> > is too much opposition to looking at the cpus nodes in the initcall,
> 
> I didn't get this one, what can we do by looking at CPUs nodes ?

I thought there was a compatible property in there that told us
whether the operating-points-v2/cooling-min-level/#cooling-cells/...
properties were considered valid.

> > start with a whitelist for known machines, that at least keeps the
> > existing behavior.
> 
> That can be a valid solution I would say, but that separate driver
> (cpufreq-dt-device.c) needs to be changed for every new platform.

Until we can agree on a way to describe in the DT whether the opp
properties are reliable or not.

	Arnd

[PATCH RFC] Add cpufreq support

[Viresh Kumar]

On 08-02-16, 14:10, Arnd Bergmann wrote:
> I don't remember the exact discussion, but the compatible string is
> exactly meant to do one thing: it tells you what you can or cannot do
> with one device.

Yeah, and many people argued that we can't add two values to that
string like: "cpufreq-dt" and "cpufreq-big-little" for same kind of
OPP bindings, as a different compatible string should be required only
if there is a difference in the bindings.

For example, if a platform (like ST did recently) adds more
platform-specific properties, then they can define a new value of
those strings.

> I had not realized that we don't even have a compatible string
> for opp-v2, so if we are missing that, we obviously can't compare
> against that string.

The binding says that we can have a string, but its not compulsory
yet. Its only used by STM as they have some specific properties of
their own.

> I thought there was a compatible property in there that told us
> whether the operating-points-v2/cooling-min-level/#cooling-cells/...
> properties were considered valid.

Yeah, "OPP-v2" DT node can have a compatible string, which isn't
compulsory as of now, but because of the reasons mentioned earlier, we
can't use it to differentiate between drivers that use exactly same
version of bindings.

-- 
viresh

[PATCH RFC] Add cpufreq support

[Arnd Bergmann]

On Monday 08 February 2016 18:46:25 Viresh Kumar wrote:
> On 08-02-16, 14:10, Arnd Bergmann wrote:
> > I don't remember the exact discussion, but the compatible string is
> > exactly meant to do one thing: it tells you what you can or cannot do
> > with one device.
> 
> Yeah, and many people argued that we can't add two values to that
> string like: "cpufreq-dt" and "cpufreq-big-little" for same kind of
> OPP bindings, as a different compatible string should be required only
> if there is a difference in the bindings.
> 
> For example, if a platform (like ST did recently) adds more
> platform-specific properties, then they can define a new value of
> those strings.
>
> > I had not realized that we don't even have a compatible string
> > for opp-v2, so if we are missing that, we obviously can't compare
> > against that string.
> 
> The binding says that we can have a string, but its not compulsory
> yet. Its only used by STM as they have some specific properties of
> their own.

Right, so when those properties are required for that machine,
that makes it incompatible with the generic driver, and it should
really have its own compatible string.

That's why I said we could introduce a 'v3' with the meaning
it should have had to start with: compatible means actually
compatible with the driver.

I guess we could also start a blacklist and list all machines
that are not compatible with the generic binding before falling
back to using that driver.

> > I thought there was a compatible property in there that told us
> > whether the operating-points-v2/cooling-min-level/#cooling-cells/...
> > properties were considered valid.
> 
> Yeah, "OPP-v2" DT node can have a compatible string, which isn't
> compulsory as of now, but because of the reasons mentioned earlier, we
> can't use it to differentiate between drivers that use exactly same
> version of bindings.

Right, unless we introduce a new compatible string for future machines.

	Arnd

[PATCH RFC] Add cpufreq support

[Mason]

On 08/02/2016 14:45, Arnd Bergmann wrote:

> That's why I said we could introduce a 'v3' with the meaning
> it should have had to start with: compatible means actually
> compatible with the driver.

If I understand correctly, something needs to change in the
framework before I can push cpufreq support for my platform
upstream, correct?

Could you CC me if anything happens on that front?

In my local 4.4 branch, I think I should use whatever method
was recommended at the time.

Was that to define the platform's init_late method, and call
platform_device_register_simple from there? (Could you take
a quick glance at the patch, and see if it is acceptable in
the context of kernel 4.4?)

Regards.