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

Message ID 2396230.MApG9VTrxr@wuerfel
State Not Applicable, archived
Headers show

Commit Message

Arnd Bergmann Feb. 26, 2016, 8:20 p.m. UTC
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:



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

Li Yang-R58472 Feb. 26, 2016, 11:07 p.m. UTC | #1
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
--
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
Arnd Bergmann Feb. 26, 2016, 11:16 p.m. UTC | #2
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
--
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

Patch
diff mbox

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);