[PATCH V3] cpufreq: qoriq: Register cooling device based on device tree

[PATCH V3] cpufreq: qoriq: Register cooling device based on device tree

[Jia Hongtao]

Register the qoriq cpufreq driver as a cooling device, based on the
thermal device tree framework. When temperature crosses the passive trip
point cpufreq is used to throttle CPUs.

Signed-off-by: Jia Hongtao <hongtao.jia@freescale.com>
Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>
---
Changes for V3:
* Removed unnecessary cpu node NULL check.

Changes for V2:
* Using ->ready callback for cpu cooling device registering.

 drivers/cpufreq/qoriq-cpufreq.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/cpufreq/qoriq-cpufreq.c b/drivers/cpufreq/qoriq-cpufreq.c
index 4f53fa2..cb6a62c 100644
--- a/drivers/cpufreq/qoriq-cpufreq.c
+++ b/drivers/cpufreq/qoriq-cpufreq.c
@@ -12,6 +12,7 @@
 
 #include <linux/clk.h>
 #include <linux/cpufreq.h>
+#include <linux/cpu_cooling.h>
 #include <linux/errno.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
@@ -33,6 +34,7 @@
 struct cpu_data {
 	struct clk **pclk;
 	struct cpufreq_frequency_table *table;
+	struct thermal_cooling_device *cdev;
 };
 
 /*
@@ -260,6 +262,27 @@ static int qoriq_cpufreq_target(struct cpufreq_policy *policy,
 	return clk_set_parent(policy->clk, parent);
 }
 
+
+static void qoriq_cpufreq_ready(struct cpufreq_policy *policy)
+{
+	struct cpu_data *cpud = policy->driver_data;
+	struct device_node *np = of_get_cpu_node(policy->cpu, NULL);
+
+	if (of_find_property(np, "#cooling-cells", NULL)) {
+		cpud->cdev = of_cpufreq_cooling_register(np,
+							 policy->related_cpus);
+
+		if (IS_ERR(cpud->cdev)) {
+			pr_err("Failed to register cooling device cpu%d: %ld\n",
+					policy->cpu, PTR_ERR(cpud->cdev));
+
+			cpud->cdev = NULL;
+		}
+	}
+
+	of_node_put(np);
+}
+
 static struct cpufreq_driver qoriq_cpufreq_driver = {
 	.name		= "qoriq_cpufreq",
 	.flags		= CPUFREQ_CONST_LOOPS,
@@ -268,6 +291,7 @@ static struct cpufreq_driver qoriq_cpufreq_driver = {
 	.verify		= cpufreq_generic_frequency_table_verify,
 	.target_index	= qoriq_cpufreq_target,
 	.get		= cpufreq_generic_get,
+	.ready		= qoriq_cpufreq_ready,
 	.attr		= cpufreq_generic_attr,
 };
 
-- 
2.1.0.27.g96db324

Re: [PATCH V3] cpufreq: qoriq: Register cooling device based on device tree

[Rafael J. Wysocki]

On Thursday, November 26, 2015 05:21:11 PM Jia Hongtao wrote:
> Register the qoriq cpufreq driver as a cooling device, based on the
> thermal device tree framework. When temperature crosses the passive trip
> point cpufreq is used to throttle CPUs.
> 
> Signed-off-by: Jia Hongtao <hongtao.jia@freescale.com>
> Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>

Applied, thanks!

Rafael

Re: [PATCH V3] cpufreq: qoriq: Register cooling device based on device tree

[Arnd Bergmann]

On Tuesday 15 December 2015 00:58:26 Rafael J. Wysocki wrote:
> On Thursday, November 26, 2015 05:21:11 PM Jia Hongtao wrote:
> > Register the qoriq cpufreq driver as a cooling device, based on the
> > thermal device tree framework. When temperature crosses the passive trip
> > point cpufreq is used to throttle CPUs.
> > 
> > Signed-off-by: Jia Hongtao <hongtao.jia@freescale.com>
> > Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>
> 
> Applied, thanks!
> 

I got a randconfig build error today:

drivers/built-in.o: In function `qoriq_cpufreq_ready':
debugfs.c:(.text+0x1f4688): undefined reference to `of_cpufreq_cooling_register'

CONFIG_OF=y
CONFIG_QORIQ_CPUFREQ=y
CONFIG_THERMAL=m
CONFIG_THERMAL_OF=y

I think you need a 'depends on THERMAL' to prevent the driver from
being built-in when THERMAL=m.

	Arnd

答复: [PATCH V3] cpufreq: qoriq: Register cooling device based on device tree

[Hongtao Jia]

U29ycnkgZm9yIHRoZSBsYXRlIHJlc3BvbnNlLiBJIGdvdCBhIGtuZWUgc3VyZ2VyeSB0byBkby4N
ClNlZSBjb21tZW50cyBhdCB0aGUgZW5kLg0KDQo+IC0tLS0t08q8/tStvP4tLS0tLQ0KPiC3orz+
yMs6IEFybmQgQmVyZ21hbm4gW21haWx0bzphcm5kQGFybmRiLmRlXQ0KPiC3osvNyrG85DogU2F0
dXJkYXksIERlY2VtYmVyIDE5LCAyMDE1IDY6MzMgQU0NCj4gytW8/sjLOiBSYWZhZWwgSi4gV3lz
b2NraSA8cmp3QHJqd3lzb2NraS5uZXQ+DQo+ILOty806IEppYSBIb25ndGFvIDxob25ndGFvLmpp
YUBmcmVlc2NhbGUuY29tPjsgZWR1YmV6dmFsQGdtYWlsLmNvbTsNCj4gdmlyZXNoLmt1bWFyQGxp
bmFyby5vcmc7IGxpbnV4LXBtQHZnZXIua2VybmVsLm9yZzsgbGludXhwcGMtDQo+IGRldkBsaXN0
cy5vemxhYnMub3JnOyBkZXZpY2V0cmVlQHZnZXIua2VybmVsLm9yZzsgU2NvdHQgV29vZA0KPiA8
c2NvdHR3b29kQGZyZWVzY2FsZS5jb20+DQo+INb3zOI6IFJlOiBbUEFUQ0ggVjNdIGNwdWZyZXE6
IHFvcmlxOiBSZWdpc3RlciBjb29saW5nIGRldmljZSBiYXNlZCBvbiBkZXZpY2UNCj4gdHJlZQ0K
PiANCj4gT24gVHVlc2RheSAxNSBEZWNlbWJlciAyMDE1IDAwOjU4OjI2IFJhZmFlbCBKLiBXeXNv
Y2tpIHdyb3RlOg0KPiA+IE9uIFRodXJzZGF5LCBOb3ZlbWJlciAyNiwgMjAxNSAwNToyMToxMSBQ
TSBKaWEgSG9uZ3RhbyB3cm90ZToNCj4gPiA+IFJlZ2lzdGVyIHRoZSBxb3JpcSBjcHVmcmVxIGRy
aXZlciBhcyBhIGNvb2xpbmcgZGV2aWNlLCBiYXNlZCBvbiB0aGUNCj4gPiA+IHRoZXJtYWwgZGV2
aWNlIHRyZWUgZnJhbWV3b3JrLiBXaGVuIHRlbXBlcmF0dXJlIGNyb3NzZXMgdGhlIHBhc3NpdmUN
Cj4gPiA+IHRyaXAgcG9pbnQgY3B1ZnJlcSBpcyB1c2VkIHRvIHRocm90dGxlIENQVXMuDQo+ID4g
Pg0KPiA+ID4gU2lnbmVkLW9mZi1ieTogSmlhIEhvbmd0YW8gPGhvbmd0YW8uamlhQGZyZWVzY2Fs
ZS5jb20+DQo+ID4gPiBSZXZpZXdlZC1ieTogVmlyZXNoIEt1bWFyIDx2aXJlc2gua3VtYXJAbGlu
YXJvLm9yZz4NCj4gPg0KPiA+IEFwcGxpZWQsIHRoYW5rcyENCj4gPg0KPiANCj4gSSBnb3QgYSBy
YW5kY29uZmlnIGJ1aWxkIGVycm9yIHRvZGF5Og0KPiANCj4gZHJpdmVycy9idWlsdC1pbi5vOiBJ
biBmdW5jdGlvbiBgcW9yaXFfY3B1ZnJlcV9yZWFkeSc6DQo+IGRlYnVnZnMuYzooLnRleHQrMHgx
ZjQ2ODgpOiB1bmRlZmluZWQgcmVmZXJlbmNlIHRvDQo+IGBvZl9jcHVmcmVxX2Nvb2xpbmdfcmVn
aXN0ZXInDQo+IA0KPiBDT05GSUdfT0Y9eQ0KPiBDT05GSUdfUU9SSVFfQ1BVRlJFUT15DQo+IENP
TkZJR19USEVSTUFMPW0NCj4gQ09ORklHX1RIRVJNQUxfT0Y9eQ0KPiANCj4gSSB0aGluayB5b3Ug
bmVlZCBhICdkZXBlbmRzIG9uIFRIRVJNQUwnIHRvIHByZXZlbnQgdGhlIGRyaXZlciBmcm9tIGJl
aW5nDQo+IGJ1aWx0LWluIHdoZW4gVEhFUk1BTD1tLg0KPiANCj4gCUFybmQNCg0KQ29ycmVjdC4g
SSBuZWVkIHRvIGFkZCBmb2xsb3dpbmcgbGluZXMgdG8gdGhlIEtjb25maWcgZmlsZToNCmRlcGVu
ZHMgb24gIUNQVV9USEVSTUFMIHx8IFRIRVJNQUw9eQ0KDQpIaSBSYWZhZWwsDQpTaG91bGQgSSBz
ZW5kIGEgbmV3IHBhdGNoIGluY2x1ZGUgdGhpcyBmaXggb3Igc2VuZCBhIGZpeCBwYXRjaD8NCg0K
VGhhbmtzLg0KLUhvbmd0YW8uDQo=

Re: 答复: [PATCH V3] cpufreq: qoriq: Register cooling device based on device tree

[Scott Wood]

T24gMDEvMTEvMjAxNiAwODo1NCBBTSwgSG9uZ3RhbyBKaWEgd3JvdGU6Cj4gU29ycnkgZm9yIHRo
ZSBsYXRlIHJlc3BvbnNlLiBJIGdvdCBhIGtuZWUgc3VyZ2VyeSB0byBkby4KPiBTZWUgY29tbWVu
dHMgYXQgdGhlIGVuZC4KPiAKPj4gLS0tLS3Tyrz+1K28/i0tLS0tCj4+ILeivP7IyzogQXJuZCBC
ZXJnbWFubiBbbWFpbHRvOmFybmRAYXJuZGIuZGVdCj4+ILeiy83KsbzkOiBTYXR1cmRheSwgRGVj
ZW1iZXIgMTksIDIwMTUgNjozMyBBTQo+PiDK1bz+yMs6IFJhZmFlbCBKLiBXeXNvY2tpIDxyandA
cmp3eXNvY2tpLm5ldD4KPj4gs63LzTogSmlhIEhvbmd0YW8gPGhvbmd0YW8uamlhQGZyZWVzY2Fs
ZS5jb20+OyBlZHViZXp2YWxAZ21haWwuY29tOwo+PiB2aXJlc2gua3VtYXJAbGluYXJvLm9yZzsg
bGludXgtcG1Admdlci5rZXJuZWwub3JnOyBsaW51eHBwYy0KPj4gZGV2QGxpc3RzLm96bGFicy5v
cmc7IGRldmljZXRyZWVAdmdlci5rZXJuZWwub3JnOyBTY290dCBXb29kCj4+IDxzY290dHdvb2RA
ZnJlZXNjYWxlLmNvbT4KPj4g1vfM4jogUmU6IFtQQVRDSCBWM10gY3B1ZnJlcTogcW9yaXE6IFJl
Z2lzdGVyIGNvb2xpbmcgZGV2aWNlIGJhc2VkIG9uIGRldmljZQo+PiB0cmVlCj4+Cj4+IE9uIFR1
ZXNkYXkgMTUgRGVjZW1iZXIgMjAxNSAwMDo1ODoyNiBSYWZhZWwgSi4gV3lzb2NraSB3cm90ZToK
Pj4+IE9uIFRodXJzZGF5LCBOb3ZlbWJlciAyNiwgMjAxNSAwNToyMToxMSBQTSBKaWEgSG9uZ3Rh
byB3cm90ZToKPj4+PiBSZWdpc3RlciB0aGUgcW9yaXEgY3B1ZnJlcSBkcml2ZXIgYXMgYSBjb29s
aW5nIGRldmljZSwgYmFzZWQgb24gdGhlCj4+Pj4gdGhlcm1hbCBkZXZpY2UgdHJlZSBmcmFtZXdv
cmsuIFdoZW4gdGVtcGVyYXR1cmUgY3Jvc3NlcyB0aGUgcGFzc2l2ZQo+Pj4+IHRyaXAgcG9pbnQg
Y3B1ZnJlcSBpcyB1c2VkIHRvIHRocm90dGxlIENQVXMuCj4+Pj4KPj4+PiBTaWduZWQtb2ZmLWJ5
OiBKaWEgSG9uZ3RhbyA8aG9uZ3Rhby5qaWFAZnJlZXNjYWxlLmNvbT4KPj4+PiBSZXZpZXdlZC1i
eTogVmlyZXNoIEt1bWFyIDx2aXJlc2gua3VtYXJAbGluYXJvLm9yZz4KPj4+Cj4+PiBBcHBsaWVk
LCB0aGFua3MhCj4+Pgo+Pgo+PiBJIGdvdCBhIHJhbmRjb25maWcgYnVpbGQgZXJyb3IgdG9kYXk6
Cj4+Cj4+IGRyaXZlcnMvYnVpbHQtaW4ubzogSW4gZnVuY3Rpb24gYHFvcmlxX2NwdWZyZXFfcmVh
ZHknOgo+PiBkZWJ1Z2ZzLmM6KC50ZXh0KzB4MWY0Njg4KTogdW5kZWZpbmVkIHJlZmVyZW5jZSB0
bwo+PiBgb2ZfY3B1ZnJlcV9jb29saW5nX3JlZ2lzdGVyJwo+Pgo+PiBDT05GSUdfT0Y9eQo+PiBD
T05GSUdfUU9SSVFfQ1BVRlJFUT15Cj4+IENPTkZJR19USEVSTUFMPW0KPj4gQ09ORklHX1RIRVJN
QUxfT0Y9eQo+Pgo+PiBJIHRoaW5rIHlvdSBuZWVkIGEgJ2RlcGVuZHMgb24gVEhFUk1BTCcgdG8g
cHJldmVudCB0aGUgZHJpdmVyIGZyb20gYmVpbmcKPj4gYnVpbHQtaW4gd2hlbiBUSEVSTUFMPW0u
Cj4+Cj4+IEFybmQKPiAKPiBDb3JyZWN0LiBJIG5lZWQgdG8gYWRkIGZvbGxvd2luZyBsaW5lcyB0
byB0aGUgS2NvbmZpZyBmaWxlOgo+IGRlcGVuZHMgb24gIUNQVV9USEVSTUFMIHx8IFRIRVJNQUw9
eQo+IAo+IEhpIFJhZmFlbCwKPiBTaG91bGQgSSBzZW5kIGEgbmV3IHBhdGNoIGluY2x1ZGUgdGhp
cyBmaXggb3Igc2VuZCBhIGZpeCBwYXRjaD8KCldoeSBUSEVSTUFMPXkgYW5kIG5vdCBqdXN0IFRI
RVJNQUwsIHdoaWNoIHdvdWxkIGFsbG93IGJ1aWxkaW5nIHRoaXMKZHJpdmVyIGFzIGEgbW9kdWxl
PwoKLVNjb3R0Cgo=

Re: 答复: [PATCH V3] cpufreq: qoriq: Register cooling device based on device tree

[Arnd Bergmann]

On Monday 11 January 2016 17:34:52 Scott Wood wrote:
> >>
> >> I think you need a 'depends on THERMAL' to prevent the driver from being
> >> built-in when THERMAL=m.
> >>
> >> Arnd
> > 
> > Correct. I need to add following lines to the Kconfig file:
> > depends on !CPU_THERMAL || THERMAL=y
> > 
> > Hi Rafael,
> > Should I send a new patch include this fix or send a fix patch?
> 
> Why THERMAL=y and not just THERMAL, which would allow building this
> driver as a module?

Right, that would be better, and it is what all other drivers do.

For some reason, some drivers depend on !CPU_THERMAL and others
depend on !THERMAL_OF here, and I think the result is the same, but
we are a bit inconsistent here. CPU_THERMAL cannot be set if THERMAL_OF
is disabled, and the header file only uses the 'extern' declaration
if both are set.

	Arnd

Re: [PATCH V3] cpufreq: qoriq: Register cooling device based on device tree

[Li Yang]

On Fri, Dec 18, 2015 at 4:32 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tuesday 15 December 2015 00:58:26 Rafael J. Wysocki wrote:
>> On Thursday, November 26, 2015 05:21:11 PM Jia Hongtao wrote:
>> > Register the qoriq cpufreq driver as a cooling device, based on the
>> > thermal device tree framework. When temperature crosses the passive trip
>> > point cpufreq is used to throttle CPUs.
>> >
>> > Signed-off-by: Jia Hongtao <hongtao.jia@freescale.com>
>> > Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>
>>
>> Applied, thanks!
>>
>
> I got a randconfig build error today:
>
> drivers/built-in.o: In function `qoriq_cpufreq_ready':
> debugfs.c:(.text+0x1f4688): undefined reference to `of_cpufreq_cooling_register'
>
> CONFIG_OF=y
> CONFIG_QORIQ_CPUFREQ=y
> CONFIG_THERMAL=m
> CONFIG_THERMAL_OF=y
>
> I think you need a 'depends on THERMAL' to prevent the driver from
> being built-in when THERMAL=m.

Maybe this is not the best approach.  The cpufreq feature itself
should be working independently without thermal framework.  I think we
should make the qoriq_cpufreq_ready() defined as null function if
THERMAL is not defined.

Regards,
Leo

Re: [PATCH V3] cpufreq: qoriq: Register cooling device based on device tree

[Arnd Bergmann]

On Friday 26 February 2016 12:04:59 Li Yang wrote:
> On Fri, Dec 18, 2015 at 4:32 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Tuesday 15 December 2015 00:58:26 Rafael J. Wysocki wrote:
> >> On Thursday, November 26, 2015 05:21:11 PM Jia Hongtao wrote:
> >> > Register the qoriq cpufreq driver as a cooling device, based on the
> >> > thermal device tree framework. When temperature crosses the passive trip
> >> > point cpufreq is used to throttle CPUs.
> >> >
> >> > Signed-off-by: Jia Hongtao <hongtao.jia@freescale.com>
> >> > Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>
> >>
> >> Applied, thanks!
> >>
> >
> > I got a randconfig build error today:
> >
> > drivers/built-in.o: In function `qoriq_cpufreq_ready':
> > debugfs.c:(.text+0x1f4688): undefined reference to `of_cpufreq_cooling_register'
> >
> > CONFIG_OF=y
> > CONFIG_QORIQ_CPUFREQ=y
> > CONFIG_THERMAL=m
> > CONFIG_THERMAL_OF=y
> >
> > I think you need a 'depends on THERMAL' to prevent the driver from
> > being built-in when THERMAL=m.
> 
> Maybe this is not the best approach.  The cpufreq feature itself
> should be working independently without thermal framework.  I think we
> should make the qoriq_cpufreq_ready() defined as null function if
> THERMAL is not defined.

It already does this when CONFIG_THERMAL is not defined, and my
patch doesn't change that. I'm not sure what you are asking for now.

Do you want to allow using the cpufreq driver as a built-in driver
even when the thermal code is in a module, and then silently skip
all thermal management as if it was turned off?

That would be this patch:

diff --git a/include/linux/cpu_cooling.h b/include/linux/cpu_cooling.h
index c156f5082758..a8d9241fc1bb 100644
--- a/include/linux/cpu_cooling.h
+++ b/include/linux/cpu_cooling.h
@@ -48,7 +48,7 @@ cpufreq_power_cooling_register(const struct cpumask *clip_cpus,
  * @np: a valid struct device_node to the cooling device device tree node.
  * @clip_cpus: cpumask of cpus where the frequency constraints will happen
  */
-#ifdef CONFIG_THERMAL_OF
+#if IS_REACHABLE(CONFIG_THERMAL_OF)
 struct thermal_cooling_device *
 of_cpufreq_cooling_register(struct device_node *np,
 			    const struct cpumask *clip_cpus);


but my feeling is that this would cause more surprises when users
find their thermal management is not active even though it was
enabled in Kconfig and the thermal module gets loaded.

	Arnd

Re: [PATCH V3] cpufreq: qoriq: Register cooling device based on device tree

[Li Yang]

On Fri, Feb 26, 2016 at 2:20 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday 26 February 2016 12:04:59 Li Yang wrote:
>> On Fri, Dec 18, 2015 at 4:32 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> > On Tuesday 15 December 2015 00:58:26 Rafael J. Wysocki wrote:
>> >> On Thursday, November 26, 2015 05:21:11 PM Jia Hongtao wrote:
>> >> > Register the qoriq cpufreq driver as a cooling device, based on the
>> >> > thermal device tree framework. When temperature crosses the passive trip
>> >> > point cpufreq is used to throttle CPUs.
>> >> >
>> >> > Signed-off-by: Jia Hongtao <hongtao.jia@freescale.com>
>> >> > Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>
>> >>
>> >> Applied, thanks!
>> >>
>> >
>> > I got a randconfig build error today:
>> >
>> > drivers/built-in.o: In function `qoriq_cpufreq_ready':
>> > debugfs.c:(.text+0x1f4688): undefined reference to `of_cpufreq_cooling_register'
>> >
>> > CONFIG_OF=y
>> > CONFIG_QORIQ_CPUFREQ=y
>> > CONFIG_THERMAL=m
>> > CONFIG_THERMAL_OF=y
>> >
>> > I think you need a 'depends on THERMAL' to prevent the driver from
>> > being built-in when THERMAL=m.
>>
>> Maybe this is not the best approach.  The cpufreq feature itself
>> should be working independently without thermal framework.  I think we
>> should make the qoriq_cpufreq_ready() defined as null function if
>> THERMAL is not defined.
>
> It already does this when CONFIG_THERMAL is not defined, and my
> patch doesn't change that. I'm not sure what you are asking for now.

Oh.  Actually I didn't see you just sent a patch for this.  I
accidentally get into this topic when I tried to find out why cpufreq
is not working on ARMv8 platforms.  I didn't notice that
of_cpufreq_cooling_register() is already ifdef-ed.

>
> Do you want to allow using the cpufreq driver as a built-in driver
> even when the thermal code is in a module, and then silently skip
> all thermal management as if it was turned off?

Having thermal code to be built as module and qoriq_cpufreq to be
built-in is a valid situation.  Making cpufreq not possible to be used
when thermal is a module doesn't seem to be right.

>
> That would be this patch:
>
> diff --git a/include/linux/cpu_cooling.h b/include/linux/cpu_cooling.h
> index c156f5082758..a8d9241fc1bb 100644
> --- a/include/linux/cpu_cooling.h
> +++ b/include/linux/cpu_cooling.h
> @@ -48,7 +48,7 @@ cpufreq_power_cooling_register(const struct cpumask *clip_cpus,
>   * @np: a valid struct device_node to the cooling device device tree node.
>   * @clip_cpus: cpumask of cpus where the frequency constraints will happen
>   */
> -#ifdef CONFIG_THERMAL_OF
> +#if IS_REACHABLE(CONFIG_THERMAL_OF)
>  struct thermal_cooling_device *
>  of_cpufreq_cooling_register(struct device_node *np,
>                             const struct cpumask *clip_cpus);
>
>
> but my feeling is that this would cause more surprises when users
> find their thermal management is not active even though it was
> enabled in Kconfig and the thermal module gets loaded.

I don't have a perfect solution either.  But I think this is still
better than making cpufreq not usable.  The cpufreq driver will print
out an error message if thermal is not reachable.  Maybe this can
relief the confusion a little bit?

Regards,
Leo

Re: [PATCH V3] cpufreq: qoriq: Register cooling device based on device tree

[Arnd Bergmann]

On Friday 26 February 2016 17:07:09 Li Yang wrote:
> 
> I don't have a perfect solution either.  But I think this is still
> better than making cpufreq not usable.  The cpufreq driver will print
> out an error message if thermal is not reachable.  Maybe this can
> relief the confusion a little bit?

With my patch, the configuration will just force the cpufreq
driver to be a loadable module as well if thermal is a module,
so the dependency can be resolved by loading the thermal module first.

I think that is really the best way around the problem, and it
matches what other platforms do for the same problem.

	Arnd

Re: [PATCH V3] cpufreq: qoriq: Register cooling device based on device tree

[Li Yang]

On Fri, Feb 26, 2016 at 5:16 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday 26 February 2016 17:07:09 Li Yang wrote:
>>
>> I don't have a perfect solution either.  But I think this is still
>> better than making cpufreq not usable.  The cpufreq driver will print
>> out an error message if thermal is not reachable.  Maybe this can
>> relief the confusion a little bit?
>
> With my patch, the configuration will just force the cpufreq
> driver to be a loadable module as well if thermal is a module,
> so the dependency can be resolved by loading the thermal module first.

It would be perfect if this it true.  But I tried with the following
change, it just makes QORIQ_CPUFREQ non-selectable if THERMAL=m.

diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
index dcb972a38fbc..ca05037dd565 100644
--- a/drivers/cpufreq/Kconfig
+++ b/drivers/cpufreq/Kconfig
@@ -297,6 +297,7 @@ endif
 config QORIQ_CPUFREQ
        tristate "CPU frequency scaling driver for Freescale QorIQ SoCs"
        depends on OF && COMMON_CLK && (PPC_E500MC || ARM)
+       depends on !CPU_THERMAL || THERMAL=y
        select CLK_QORIQ
        help
          This adds the CPUFreq driver support for Freescale QorIQ SoCs

Regards,
Leo

Re: [PATCH V3] cpufreq: qoriq: Register cooling device based on device tree

[Li Yang]

On Fri, Feb 26, 2016 at 5:31 PM, Li Yang <leoli@freescale.com> wrote:
> On Fri, Feb 26, 2016 at 5:16 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Friday 26 February 2016 17:07:09 Li Yang wrote:
>>>
>>> I don't have a perfect solution either.  But I think this is still
>>> better than making cpufreq not usable.  The cpufreq driver will print
>>> out an error message if thermal is not reachable.  Maybe this can
>>> relief the confusion a little bit?
>>
>> With my patch, the configuration will just force the cpufreq
>> driver to be a loadable module as well if thermal is a module,
>> so the dependency can be resolved by loading the thermal module first.
>
> It would be perfect if this it true.  But I tried with the following
> change, it just makes QORIQ_CPUFREQ non-selectable if THERMAL=m.
>
> diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
> index dcb972a38fbc..ca05037dd565 100644
> --- a/drivers/cpufreq/Kconfig
> +++ b/drivers/cpufreq/Kconfig
> @@ -297,6 +297,7 @@ endif
>  config QORIQ_CPUFREQ
>         tristate "CPU frequency scaling driver for Freescale QorIQ SoCs"
>         depends on OF && COMMON_CLK && (PPC_E500MC || ARM)
> +       depends on !CPU_THERMAL || THERMAL=y
>         select CLK_QORIQ
>         help
>           This adds the CPUFreq driver support for Freescale QorIQ SoCs


I find we can achieve your desired result with the following change instead:

+       depends on (THERMAL=m && m) || THERMAL=y || THERMAL=n

Regards,
Leo

Re: [PATCH V3] cpufreq: qoriq: Register cooling device based on device tree

[Scott Wood]

On Fri, 2016-02-26 at 18:04 -0600, Li Yang wrote:
> On Fri, Feb 26, 2016 at 5:31 PM, Li Yang <leoli@freescale.com> wrote:
> > On Fri, Feb 26, 2016 at 5:16 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > > On Friday 26 February 2016 17:07:09 Li Yang wrote:
> > > > 
> > > > I don't have a perfect solution either.  But I think this is still
> > > > better than making cpufreq not usable.  The cpufreq driver will print
> > > > out an error message if thermal is not reachable.  Maybe this can
> > > > relief the confusion a little bit?
> > > 
> > > With my patch, the configuration will just force the cpufreq
> > > driver to be a loadable module as well if thermal is a module,
> > > so the dependency can be resolved by loading the thermal module first.
> > 
> > It would be perfect if this it true.  But I tried with the following
> > change, it just makes QORIQ_CPUFREQ non-selectable if THERMAL=m.
> > 
> > diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
> > index dcb972a38fbc..ca05037dd565 100644
> > --- a/drivers/cpufreq/Kconfig
> > +++ b/drivers/cpufreq/Kconfig
> > @@ -297,6 +297,7 @@ endif
> >  config QORIQ_CPUFREQ
> >         tristate "CPU frequency scaling driver for Freescale QorIQ SoCs"
> >         depends on OF && COMMON_CLK && (PPC_E500MC || ARM)
> > +       depends on !CPU_THERMAL || THERMAL=y
> >         select CLK_QORIQ
> >         help
> >           This adds the CPUFreq driver support for Freescale QorIQ SoCs
> 
> 
> I find we can achieve your desired result with the following change instead:
> 
> +       depends on (THERMAL=m && m) || THERMAL=y || THERMAL=n

"depends on THERMAL || !THERMAL" should also work.

-Scott

Re: [PATCH V3] cpufreq: qoriq: Register cooling device based on device tree

[Li Yang]

On Fri, Feb 26, 2016 at 6:08 PM, Scott Wood <oss@buserror.net> wrote:
> On Fri, 2016-02-26 at 18:04 -0600, Li Yang wrote:
>> On Fri, Feb 26, 2016 at 5:31 PM, Li Yang <leoli@freescale.com> wrote:
>> > On Fri, Feb 26, 2016 at 5:16 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> > > On Friday 26 February 2016 17:07:09 Li Yang wrote:
>> > > >
>> > > > I don't have a perfect solution either.  But I think this is still
>> > > > better than making cpufreq not usable.  The cpufreq driver will print
>> > > > out an error message if thermal is not reachable.  Maybe this can
>> > > > relief the confusion a little bit?
>> > >
>> > > With my patch, the configuration will just force the cpufreq
>> > > driver to be a loadable module as well if thermal is a module,
>> > > so the dependency can be resolved by loading the thermal module first.
>> >
>> > It would be perfect if this it true.  But I tried with the following
>> > change, it just makes QORIQ_CPUFREQ non-selectable if THERMAL=m.
>> >
>> > diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
>> > index dcb972a38fbc..ca05037dd565 100644
>> > --- a/drivers/cpufreq/Kconfig
>> > +++ b/drivers/cpufreq/Kconfig
>> > @@ -297,6 +297,7 @@ endif
>> >  config QORIQ_CPUFREQ
>> >         tristate "CPU frequency scaling driver for Freescale QorIQ SoCs"
>> >         depends on OF && COMMON_CLK && (PPC_E500MC || ARM)
>> > +       depends on !CPU_THERMAL || THERMAL=y
>> >         select CLK_QORIQ
>> >         help
>> >           This adds the CPUFreq driver support for Freescale QorIQ SoCs
>>
>>
>> I find we can achieve your desired result with the following change instead:
>>
>> +       depends on (THERMAL=m && m) || THERMAL=y || THERMAL=n
>
> "depends on THERMAL || !THERMAL" should also work.

Right.  And this is more simpler.

Regards,
Leo

Re: [PATCH V3] cpufreq: qoriq: Register cooling device based on device tree

[Arnd Bergmann]

On Friday 26 February 2016 18:41:06 Li Yang wrote:
> >> >
> >> > It would be perfect if this it true.  But I tried with the following
> >> > change, it just makes QORIQ_CPUFREQ non-selectable if THERMAL=m.
> >> >
> >> > diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
> >> > index dcb972a38fbc..ca05037dd565 100644
> >> > --- a/drivers/cpufreq/Kconfig
> >> > +++ b/drivers/cpufreq/Kconfig
> >> > @@ -297,6 +297,7 @@ endif
> >> >  config QORIQ_CPUFREQ
> >> >         tristate "CPU frequency scaling driver for Freescale QorIQ SoCs"
> >> >         depends on OF && COMMON_CLK && (PPC_E500MC || ARM)
> >> > +       depends on !CPU_THERMAL || THERMAL=y
> >> >         select CLK_QORIQ
> >> >         help
> >> >           This adds the CPUFreq driver support for Freescale QorIQ SoCs
> >>

Oops.

> >> I find we can achieve your desired result with the following change instead:
> >>
> >> +       depends on (THERMAL=m && m) || THERMAL=y || THERMAL=n
> >
> > "depends on THERMAL || !THERMAL" should also work.
> 
> Right.  And this is more simpler.


Note the check on !CPU_THERMAL rather than !THERMAL in my patch, that
part was correct. I think the line should be

	depends on !CPU_THERMAL || THERMAL

as some other drivers do. I must have copied the line
from ARM_MT8173_CPUFREQ, which is a 'bool' symbol, when
it should have been the same as ARM_BIG_LITTLE_CPUFREQ and
CPUFREQ_DT.

	Arnd

Re: [PATCH V3] cpufreq: qoriq: Register cooling device based on device tree

[Rafael J. Wysocki]

On Mon, Feb 29, 2016 at 11:05 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday 26 February 2016 18:41:06 Li Yang wrote:
>> >> >
>> >> > It would be perfect if this it true.  But I tried with the following
>> >> > change, it just makes QORIQ_CPUFREQ non-selectable if THERMAL=m.
>> >> >
>> >> > diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
>> >> > index dcb972a38fbc..ca05037dd565 100644
>> >> > --- a/drivers/cpufreq/Kconfig
>> >> > +++ b/drivers/cpufreq/Kconfig
>> >> > @@ -297,6 +297,7 @@ endif
>> >> >  config QORIQ_CPUFREQ
>> >> >         tristate "CPU frequency scaling driver for Freescale QorIQ SoCs"
>> >> >         depends on OF && COMMON_CLK && (PPC_E500MC || ARM)
>> >> > +       depends on !CPU_THERMAL || THERMAL=y
>> >> >         select CLK_QORIQ
>> >> >         help
>> >> >           This adds the CPUFreq driver support for Freescale QorIQ SoCs
>> >>
>
> Oops.
>
>> >> I find we can achieve your desired result with the following change instead:
>> >>
>> >> +       depends on (THERMAL=m && m) || THERMAL=y || THERMAL=n
>> >
>> > "depends on THERMAL || !THERMAL" should also work.
>>
>> Right.  And this is more simpler.
>
>
> Note the check on !CPU_THERMAL rather than !THERMAL in my patch, that
> part was correct. I think the line should be
>
>         depends on !CPU_THERMAL || THERMAL
>
> as some other drivers do. I must have copied the line
> from ARM_MT8173_CPUFREQ, which is a 'bool' symbol, when
> it should have been the same as ARM_BIG_LITTLE_CPUFREQ and
> CPUFREQ_DT.

OK, so any chance to update the patch?

Thanks,
Rafael

Re: [PATCH V3] cpufreq: qoriq: Register cooling device based on device tree

[Arnd Bergmann]

On Monday 29 February 2016 15:33:37 Rafael J. Wysocki wrote:
> >
> > Note the check on !CPU_THERMAL rather than !THERMAL in my patch, that
> > part was correct. I think the line should be
> >
> >         depends on !CPU_THERMAL || THERMAL
> >
> > as some other drivers do. I must have copied the line
> > from ARM_MT8173_CPUFREQ, which is a 'bool' symbol, when
> > it should have been the same as ARM_BIG_LITTLE_CPUFREQ and
> > CPUFREQ_DT.
> 
> OK, so any chance to update the patch?
> 

I've committed the update locally now, and will follow up tomorrow
with a new patch, unless it causes another build-time regression.

I'll also send the respective update for the mediatek driver, making
that a tristate option.

	Arnd