diff mbox

[RFC] Add cpufreq support

Message ID 56B4D4BE.2040008@free.fr (mailing list archive)
State RFC, archived
Headers show

Commit Message

Mason Feb. 5, 2016, 4:58 p.m. UTC
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(+)

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Arnd Bergmann Feb. 5, 2016, 10:24 p.m. UTC | #1
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
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Viresh Kumar Feb. 7, 2016, 12:22 p.m. UTC | #2
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 ?
Arnd Bergmann Feb. 8, 2016, 12:24 p.m. UTC | #3
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
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Viresh Kumar Feb. 8, 2016, 12:31 p.m. UTC | #4
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 ?
Arnd Bergmann Feb. 8, 2016, 12:34 p.m. UTC | #5
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
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Viresh Kumar Feb. 8, 2016, 12:41 p.m. UTC | #6
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.
Arnd Bergmann Feb. 8, 2016, 1:10 p.m. UTC | #7
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
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Viresh Kumar Feb. 8, 2016, 1:16 p.m. UTC | #8
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.
Arnd Bergmann Feb. 8, 2016, 1:45 p.m. UTC | #9
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
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mason Feb. 9, 2016, 10:17 a.m. UTC | #10
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.

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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@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