| Message ID | cover.1547481320.git.amit.kucheria@linaro.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | cpufreq: Add flag to auto-register as cooling device | expand |
On 14-01-19, 22:04, Amit Kucheria wrote: > Add a flag to be used by cpufreq drivers to tell cpufreq core to > auto-register themselves as a thermal cooling device. > > There series converts over all the drivers except arm_big_little.c. > Tested on SDM845 with the qcom-cpufreq-hw driver. Only compile-tested the > others. > > Things needing fixing: > - Look at how to detect that we're not in IKS mode in arm_big_little's > .ready callback. is_bL_switching_enabled() lets you know if IKS is enabled or not. Set/clear flag conditionally before the cpufreq-driver is registered, based on the output of is_bL_switching_enabled(). > - The other pending issue is to fix allmodconfig that leaves us with > CPU_FREQ=y and THERMAL=m (CPU_THERMAL=y). That leads to undefined > references for functions defined in cpu_cooling.c Okay, that's a terrible thing and the solution looks to be rather difficult. For others who may not be aware of the issue here, currently the cpufreq drivers use helpers of cpu_cooling.c (CONFIG_CPU_THERMAL), which uses helpers of the thermal core (CONFIG_THERMAL). CONFIG_THERMAL is defined as tristate and CONFIG_CPU_THERMAL as bool in Kconfigs. The cpufreq drivers using the cpu_cooling.c file have this in their Kconfig entry: # if CPU_THERMAL is on and THERMAL=m, ARM_BIT_LITTLE_CPUFREQ cannot be =y # depends on !CPU_THERMAL || THERMAL This series now places the cpufreq core in place of the cpufreq driver and it messes up everything. It is not just about allmodconfig, but any configuration which makes the compilation fail. What are the solutions we have now ? 1. Have following for CONFIG_CPU_FREQ depends on !CPU_THERMAL || THERMAL The platforms which don't need CPU_THERMAL (like x86) should not enable CPU_THERMAL anymore if they want CONFIG_THERMAL=m. @amit: If this gets accepted, please update the Kconfig entries for all those drivers to not have above lines anymore. - Change CONFIG_THERMAL to bool instead of tristate ? - Anything else ?
On Thu, Jan 17, 2019 at 6:49 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 14-01-19, 22:04, Amit Kucheria wrote: > > Add a flag to be used by cpufreq drivers to tell cpufreq core to > > auto-register themselves as a thermal cooling device. > > > > There series converts over all the drivers except arm_big_little.c. > > Tested on SDM845 with the qcom-cpufreq-hw driver. Only compile-tested the > > others. > > > > Things needing fixing: > > - Look at how to detect that we're not in IKS mode in arm_big_little's > > .ready callback. > > is_bL_switching_enabled() lets you know if IKS is enabled or not. > Set/clear flag conditionally before the cpufreq-driver is registered, > based on the output of is_bL_switching_enabled(). > > > - The other pending issue is to fix allmodconfig that leaves us with > > CPU_FREQ=y and THERMAL=m (CPU_THERMAL=y). That leads to undefined > > references for functions defined in cpu_cooling.c > > Okay, that's a terrible thing and the solution looks to be rather > difficult. > > For others who may not be aware of the issue here, currently the > cpufreq drivers use helpers of cpu_cooling.c (CONFIG_CPU_THERMAL), > which uses helpers of the thermal core (CONFIG_THERMAL). > CONFIG_THERMAL is defined as tristate and CONFIG_CPU_THERMAL as bool > in Kconfigs. And CONFIG_CPU_THERMAL is defined under "if THERMAL". > The cpufreq drivers using the cpu_cooling.c file have this in their > Kconfig entry: > > # if CPU_THERMAL is on and THERMAL=m, ARM_BIT_LITTLE_CPUFREQ cannot be =y > # depends on !CPU_THERMAL || THERMAL > > > This series now places the cpufreq core in place of the cpufreq driver > and it messes up everything. It is not just about allmodconfig, but > any configuration which makes the compilation fail. > > What are the solutions we have now ? > > 1. Have following for CONFIG_CPU_FREQ > depends on !CPU_THERMAL || THERMAL Sorry, but this makes my teeth hurt. > The platforms which don't need CPU_THERMAL (like x86) should not > enable CPU_THERMAL anymore if they want CONFIG_THERMAL=m. > > @amit: If this gets accepted, please update the Kconfig entries for > all those drivers to not have above lines anymore. > > - Change CONFIG_THERMAL to bool instead of tristate ? > > - Anything else ? The design in the thermal subsystem seems to be upside-down. Non-modular code should never be made depend on anything only defined in a module. Would an explicit "select THERMAL" under CPU_THERMAL cause THERMAL to be 'y'?
On Thu, Jan 17, 2019 at 11:08 AM Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Thu, Jan 17, 2019 at 6:49 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > On 14-01-19, 22:04, Amit Kucheria wrote: > > > Add a flag to be used by cpufreq drivers to tell cpufreq core to > > > auto-register themselves as a thermal cooling device. > > > > > > There series converts over all the drivers except arm_big_little.c. > > > Tested on SDM845 with the qcom-cpufreq-hw driver. Only compile-tested the > > > others. > > > > > > Things needing fixing: > > > - Look at how to detect that we're not in IKS mode in arm_big_little's > > > .ready callback. > > > > is_bL_switching_enabled() lets you know if IKS is enabled or not. > > Set/clear flag conditionally before the cpufreq-driver is registered, > > based on the output of is_bL_switching_enabled(). > > > > > - The other pending issue is to fix allmodconfig that leaves us with > > > CPU_FREQ=y and THERMAL=m (CPU_THERMAL=y). That leads to undefined > > > references for functions defined in cpu_cooling.c > > > > Okay, that's a terrible thing and the solution looks to be rather > > difficult. > > > > For others who may not be aware of the issue here, currently the > > cpufreq drivers use helpers of cpu_cooling.c (CONFIG_CPU_THERMAL), > > which uses helpers of the thermal core (CONFIG_THERMAL). > > CONFIG_THERMAL is defined as tristate and CONFIG_CPU_THERMAL as bool > > in Kconfigs. > > And CONFIG_CPU_THERMAL is defined under "if THERMAL". > > > The cpufreq drivers using the cpu_cooling.c file have this in their > > Kconfig entry: > > > > # if CPU_THERMAL is on and THERMAL=m, ARM_BIT_LITTLE_CPUFREQ cannot be =y > > # depends on !CPU_THERMAL || THERMAL > > > > > > This series now places the cpufreq core in place of the cpufreq driver > > and it messes up everything. It is not just about allmodconfig, but > > any configuration which makes the compilation fail. > > > > What are the solutions we have now ? > > > > 1. Have following for CONFIG_CPU_FREQ > > depends on !CPU_THERMAL || THERMAL > > Sorry, but this makes my teeth hurt. > > > The platforms which don't need CPU_THERMAL (like x86) should not > > enable CPU_THERMAL anymore if they want CONFIG_THERMAL=m. > > > > @amit: If this gets accepted, please update the Kconfig entries for > > all those drivers to not have above lines anymore. > > > > - Change CONFIG_THERMAL to bool instead of tristate ? > > > > - Anything else ? > > The design in the thermal subsystem seems to be upside-down. > Non-modular code should never be made depend on anything only defined > in a module. > > Would an explicit "select THERMAL" under CPU_THERMAL cause THERMAL to be 'y'? Alternatively, "depends on THERMAL=y" under CPU_THERMAL should work too, shouldn't it? "!CPU_THERMAL || THERMAL" would be always true then, AFAICS.
On 17-01-19, 11:08, Rafael J. Wysocki wrote: > On Thu, Jan 17, 2019 at 6:49 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > 1. Have following for CONFIG_CPU_FREQ > > depends on !CPU_THERMAL || THERMAL > > Sorry, but this makes my teeth hurt. I knew it :) > > The platforms which don't need CPU_THERMAL (like x86) should not > > enable CPU_THERMAL anymore if they want CONFIG_THERMAL=m. > > > > @amit: If this gets accepted, please update the Kconfig entries for > > all those drivers to not have above lines anymore. > > > > - Change CONFIG_THERMAL to bool instead of tristate ? > > > > - Anything else ? > > The design in the thermal subsystem seems to be upside-down. > Non-modular code should never be made depend on anything only defined > in a module. > > Would an explicit "select THERMAL" under CPU_THERMAL cause THERMAL to be 'y'? That causes recursive-dependency issues: drivers/thermal/Kconfig:5:error: recursive dependency detected! drivers/thermal/Kconfig:5: symbol THERMAL is selected by CPU_THERMAL drivers/thermal/Kconfig:16: symbol CPU_THERMAL depends on THERMAL_OF drivers/thermal/Kconfig:70: symbol THERMAL_OF depends on THERMAL For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" something like this works though: diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig index 30323426902e..ee9f9f2a795b 100644 --- a/drivers/thermal/Kconfig +++ b/drivers/thermal/Kconfig @@ -13,6 +13,20 @@ menuconfig THERMAL All platforms with ACPI thermal support can use this driver. If you want this support, you should say Y or M here. +config CPU_THERMAL + bool "generic cpu cooling support" + depends on CPU_FREQ + select THERMAL_OF + select THERMAL + help + This implements the generic cpu cooling mechanism through frequency + reduction. An ACPI version of this already exists + (drivers/acpi/processor_thermal.c). + This will be useful for platforms using the generic thermal interface + and not the ACPI interface. + + If you want this support, you should say Y here. + if THERMAL config THERMAL_STATISTICS @@ -148,19 +162,6 @@ config THERMAL_GOV_POWER_ALLOCATOR Enable this to manage platform thermals by dynamically allocating and limiting power to devices. -config CPU_THERMAL - bool "generic cpu cooling support" - depends on CPU_FREQ - depends on THERMAL_OF - help - This implements the generic cpu cooling mechanism through frequency - reduction. An ACPI version of this already exists - (drivers/acpi/processor_thermal.c). - This will be useful for platforms using the generic thermal interface - and not the ACPI interface. - - If you want this support, you should say Y here. - config CLOCK_THERMAL bool "Generic clock cooling support" depends on COMMON_CLK What about make CONFIG_THERMAL bool instead ? Who wants it to be a module ? $ git grep "CONFIG_THERMAL=m" arch/arm/configs/mini2440_defconfig:CONFIG_THERMAL=m arch/arm/configs/omap2plus_defconfig:CONFIG_THERMAL=m arch/arm/configs/pxa_defconfig:CONFIG_THERMAL=m arch/mips/configs/ip22_defconfig:CONFIG_THERMAL=m arch/mips/configs/ip27_defconfig:CONFIG_THERMAL=m arch/unicore32/configs/unicore32_defconfig:#CONFIG_THERMAL=m Not sure if they really want this code out :(
On 17-01-19, 11:14, Rafael J. Wysocki wrote: > Alternatively, "depends on THERMAL=y" under CPU_THERMAL should work > too, shouldn't it? "!CPU_THERMAL || THERMAL" would be always true > then, AFAICS. That works and this is the easiest of the changes to make :)
On Thu, Jan 17, 2019 at 11:21 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 17-01-19, 11:08, Rafael J. Wysocki wrote: > > On Thu, Jan 17, 2019 at 6:49 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > 1. Have following for CONFIG_CPU_FREQ > > > depends on !CPU_THERMAL || THERMAL > > > > Sorry, but this makes my teeth hurt. > > I knew it :) > > > > The platforms which don't need CPU_THERMAL (like x86) should not > > > enable CPU_THERMAL anymore if they want CONFIG_THERMAL=m. > > > > > > @amit: If this gets accepted, please update the Kconfig entries for > > > all those drivers to not have above lines anymore. > > > > > > - Change CONFIG_THERMAL to bool instead of tristate ? > > > > > > - Anything else ? > > > > The design in the thermal subsystem seems to be upside-down. > > Non-modular code should never be made depend on anything only defined > > in a module. > > > > Would an explicit "select THERMAL" under CPU_THERMAL cause THERMAL to be 'y'? > > That causes recursive-dependency issues: > > drivers/thermal/Kconfig:5:error: recursive dependency detected! > drivers/thermal/Kconfig:5: symbol THERMAL is selected by CPU_THERMAL > drivers/thermal/Kconfig:16: symbol CPU_THERMAL depends on THERMAL_OF > drivers/thermal/Kconfig:70: symbol THERMAL_OF depends on THERMAL > For a resolution refer to Documentation/kbuild/kconfig-language.txt > subsection "Kconfig recursive dependency limitations" > > something like this works though: > > diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig > index 30323426902e..ee9f9f2a795b 100644 > --- a/drivers/thermal/Kconfig > +++ b/drivers/thermal/Kconfig > @@ -13,6 +13,20 @@ menuconfig THERMAL > All platforms with ACPI thermal support can use this driver. > If you want this support, you should say Y or M here. > > +config CPU_THERMAL > + bool "generic cpu cooling support" > + depends on CPU_FREQ > + select THERMAL_OF > + select THERMAL > + help > + This implements the generic cpu cooling mechanism through frequency > + reduction. An ACPI version of this already exists > + (drivers/acpi/processor_thermal.c). > + This will be useful for platforms using the generic thermal interface > + and not the ACPI interface. > + > + If you want this support, you should say Y here. > + My sort of educated guess is that it is under the "if THERMAL" so that it doesn't show up at all when THERMAL is unset. > if THERMAL > > config THERMAL_STATISTICS > @@ -148,19 +162,6 @@ config THERMAL_GOV_POWER_ALLOCATOR > Enable this to manage platform thermals by dynamically > allocating and limiting power to devices. > > -config CPU_THERMAL > - bool "generic cpu cooling support" > - depends on CPU_FREQ But what about adding depends on THERMAL=y here as I said in the other message? > - depends on THERMAL_OF > - help > - This implements the generic cpu cooling mechanism through frequency > - reduction. An ACPI version of this already exists > - (drivers/acpi/processor_thermal.c). > - This will be useful for platforms using the generic thermal interface > - and not the ACPI interface. > - > - If you want this support, you should say Y here. > - > config CLOCK_THERMAL > bool "Generic clock cooling support" > depends on COMMON_CLK > > > What about make CONFIG_THERMAL bool instead ? Who wants it to be a module ? > > $ git grep "CONFIG_THERMAL=m" > arch/arm/configs/mini2440_defconfig:CONFIG_THERMAL=m > arch/arm/configs/omap2plus_defconfig:CONFIG_THERMAL=m > arch/arm/configs/pxa_defconfig:CONFIG_THERMAL=m > arch/mips/configs/ip22_defconfig:CONFIG_THERMAL=m > arch/mips/configs/ip27_defconfig:CONFIG_THERMAL=m > arch/unicore32/configs/unicore32_defconfig:#CONFIG_THERMAL=m > > Not sure if they really want this code out :( Me neither.
On Thu, Jan 17, 2019 at 11:28 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 17-01-19, 11:14, Rafael J. Wysocki wrote: > > Alternatively, "depends on THERMAL=y" under CPU_THERMAL should work > > too, shouldn't it? "!CPU_THERMAL || THERMAL" would be always true > > then, AFAICS. > > That works and this is the easiest of the changes to make :) Let's do that, then, I think.