| Message ID | 56B4D4BE.2040008@free.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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 ?
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
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 ?
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
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.
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
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.
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
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.
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