| Message ID | cover.1547481320.git.amit.kucheria@linaro.org |
|---|---|
| Headers | show |
| Series | cpufreq: Add flag to auto-register as cooling device | expand |
On Mon, Jan 14, 2019 at 10:04:56PM +0530, Amit Kucheria wrote: > Add the CPUFREQ_AUTO_REGISTER_COOLING_DEV flag to allow the cpufreq core > to auto-register the driver as a cooling device. > > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org> > --- > drivers/cpufreq/qcom-cpufreq-hw.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c > index d83939a1b3d4..ed32849a3d40 100644 > --- a/drivers/cpufreq/qcom-cpufreq-hw.c > +++ b/drivers/cpufreq/qcom-cpufreq-hw.c > @@ -231,7 +231,8 @@ static struct freq_attr *qcom_cpufreq_hw_attr[] = { > > static struct cpufreq_driver cpufreq_qcom_hw_driver = { > .flags = CPUFREQ_STICKY | CPUFREQ_NEED_INITIAL_FREQ_CHECK | > - CPUFREQ_HAVE_GOVERNOR_PER_POLICY, > + CPUFREQ_HAVE_GOVERNOR_PER_POLICY | > + CPUFREQ_AUTO_REGISTER_COOLING_DEV, > .verify = cpufreq_generic_frequency_table_verify, > .target_index = qcom_cpufreq_hw_target_index, > .get = qcom_cpufreq_hw_get, Reviewed-by: Matthias Kaehlcke <mka@chromium.org> Tested-by: Matthias Kaehlcke <mka@chromium.org>
On Mon, Jan 14, 2019 at 10:05:01PM +0530, Amit Kucheria wrote: > Use the CPUFREQ_AUTO_REGISTER_COOLING_DEV flag to allow cpufreq core to > automatically register as a thermal cooling device. > > This allows removal of boiler plate code from the driver. > For this and the next patch(scpi), Acked-by: Sudeep Holla <sudeep.holla@arm.com> -- Regards, Sudeep
On Monday, January 14, 2019 5:34:55 PM CET Amit Kucheria wrote: > Minor clean-up to use BIT() and keep checkpatch happy. Clean up the > comment formatting while we're at it to make it easier to read. > > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org> Can you please do this cleanup as the first patch in the series, so it doesn't depend on the other patches in it, which isn't necessary IMO? > --- > include/linux/cpufreq.h | 27 ++++++++++++++------------- > 1 file changed, 14 insertions(+), 13 deletions(-) > > diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h > index 70ad02088825..ea303dfd5f05 100644 > --- a/include/linux/cpufreq.h > +++ b/include/linux/cpufreq.h > @@ -351,14 +351,15 @@ struct cpufreq_driver { > }; > > /* flags */ > -#define CPUFREQ_STICKY (1 << 0) /* driver isn't removed even if > - all ->init() calls failed */ > -#define CPUFREQ_CONST_LOOPS (1 << 1) /* loops_per_jiffy or other > - kernel "constants" aren't > - affected by frequency > - transitions */ > -#define CPUFREQ_PM_NO_WARN (1 << 2) /* don't warn on suspend/resume > - speed mismatches */ > + > +/* driver isn't removed even if all ->init() calls failed */ > +#define CPUFREQ_STICKY BIT(0) > + > +/* loops_per_jiffy or other kernel "constants" aren't affected by frequency transitions */ > +#define CPUFREQ_CONST_LOOPS BIT(1) > + > +/* don't warn on suspend/resume speed mismatches */ > +#define CPUFREQ_PM_NO_WARN BIT(2) > > /* > * This should be set by platforms having multiple clock-domains, i.e. > @@ -366,14 +367,14 @@ struct cpufreq_driver { > * be created in cpu/cpu<num>/cpufreq/ directory and so they can use the same > * governor with different tunables for different clusters. > */ > -#define CPUFREQ_HAVE_GOVERNOR_PER_POLICY (1 << 3) > +#define CPUFREQ_HAVE_GOVERNOR_PER_POLICY BIT(3) > > /* > * Driver will do POSTCHANGE notifications from outside of their ->target() > * routine and so must set cpufreq_driver->flags with this flag, so that core > * can handle them specially. > */ > -#define CPUFREQ_ASYNC_NOTIFICATION (1 << 4) > +#define CPUFREQ_ASYNC_NOTIFICATION BIT(4) > > /* > * Set by drivers which want cpufreq core to check if CPU is running at a > @@ -382,19 +383,19 @@ struct cpufreq_driver { > * from the table. And if that fails, we will stop further boot process by > * issuing a BUG_ON(). > */ > -#define CPUFREQ_NEED_INITIAL_FREQ_CHECK (1 << 5) > +#define CPUFREQ_NEED_INITIAL_FREQ_CHECK BIT(5) > > /* > * Set by drivers to disallow use of governors with "dynamic_switching" flag > * set. > */ > -#define CPUFREQ_NO_AUTO_DYNAMIC_SWITCHING (1 << 6) > +#define CPUFREQ_NO_AUTO_DYNAMIC_SWITCHING BIT(6) > > /* > * Set by drivers that want the core to automatically register the cpufreq > * driver as a thermal cooling device > */ > -#define CPUFREQ_AUTO_REGISTER_COOLING_DEV (1 << 7) > +#define CPUFREQ_AUTO_REGISTER_COOLING_DEV BIT(7) > > int cpufreq_register_driver(struct cpufreq_driver *driver_data); > int cpufreq_unregister_driver(struct cpufreq_driver *driver_data); >
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 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.
On Thu, Jan 17, 2019 at 4:26 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > On Monday, January 14, 2019 5:34:55 PM CET Amit Kucheria wrote: > > Minor clean-up to use BIT() and keep checkpatch happy. Clean up the > > comment formatting while we're at it to make it easier to read. > > > > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org> > > Can you please do this cleanup as the first patch in the series, so it > doesn't depend on the other patches in it, which isn't necessary IMO? I've sent it out separately now, unconnected to the series. Thanks. Regards, Amit > > --- > > include/linux/cpufreq.h | 27 ++++++++++++++------------- > > 1 file changed, 14 insertions(+), 13 deletions(-) > > > > diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h > > index 70ad02088825..ea303dfd5f05 100644 > > --- a/include/linux/cpufreq.h > > +++ b/include/linux/cpufreq.h > > @@ -351,14 +351,15 @@ struct cpufreq_driver { > > }; > > > > /* flags */ > > -#define CPUFREQ_STICKY (1 << 0) /* driver isn't removed even if > > - all ->init() calls failed */ > > -#define CPUFREQ_CONST_LOOPS (1 << 1) /* loops_per_jiffy or other > > - kernel "constants" aren't > > - affected by frequency > > - transitions */ > > -#define CPUFREQ_PM_NO_WARN (1 << 2) /* don't warn on suspend/resume > > - speed mismatches */ > > + > > +/* driver isn't removed even if all ->init() calls failed */ > > +#define CPUFREQ_STICKY BIT(0) > > + > > +/* loops_per_jiffy or other kernel "constants" aren't affected by frequency transitions */ > > +#define CPUFREQ_CONST_LOOPS BIT(1) > > + > > +/* don't warn on suspend/resume speed mismatches */ > > +#define CPUFREQ_PM_NO_WARN BIT(2) > > > > /* > > * This should be set by platforms having multiple clock-domains, i.e. > > @@ -366,14 +367,14 @@ struct cpufreq_driver { > > * be created in cpu/cpu<num>/cpufreq/ directory and so they can use the same > > * governor with different tunables for different clusters. > > */ > > -#define CPUFREQ_HAVE_GOVERNOR_PER_POLICY (1 << 3) > > +#define CPUFREQ_HAVE_GOVERNOR_PER_POLICY BIT(3) > > > > /* > > * Driver will do POSTCHANGE notifications from outside of their ->target() > > * routine and so must set cpufreq_driver->flags with this flag, so that core > > * can handle them specially. > > */ > > -#define CPUFREQ_ASYNC_NOTIFICATION (1 << 4) > > +#define CPUFREQ_ASYNC_NOTIFICATION BIT(4) > > > > /* > > * Set by drivers which want cpufreq core to check if CPU is running at a > > @@ -382,19 +383,19 @@ struct cpufreq_driver { > > * from the table. And if that fails, we will stop further boot process by > > * issuing a BUG_ON(). > > */ > > -#define CPUFREQ_NEED_INITIAL_FREQ_CHECK (1 << 5) > > +#define CPUFREQ_NEED_INITIAL_FREQ_CHECK BIT(5) > > > > /* > > * Set by drivers to disallow use of governors with "dynamic_switching" flag > > * set. > > */ > > -#define CPUFREQ_NO_AUTO_DYNAMIC_SWITCHING (1 << 6) > > +#define CPUFREQ_NO_AUTO_DYNAMIC_SWITCHING BIT(6) > > > > /* > > * Set by drivers that want the core to automatically register the cpufreq > > * driver as a thermal cooling device > > */ > > -#define CPUFREQ_AUTO_REGISTER_COOLING_DEV (1 << 7) > > +#define CPUFREQ_AUTO_REGISTER_COOLING_DEV BIT(7) > > > > int cpufreq_register_driver(struct cpufreq_driver *driver_data); > > int cpufreq_unregister_driver(struct cpufreq_driver *driver_data); > > > >
On Thu, Jan 17, 2019 at 10:12 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 17-01-19, 00:03, Rafael J. Wysocki wrote: > > On Monday, January 14, 2019 5:34:54 PM CET Amit Kucheria wrote: > > > All cpufreq drivers do similar things to register as a cooling device. > > > Provide a cpufreq driver flag so drivers can just ask the cpufreq core > > > to register the cooling device on their behalf. This allows us to get > > > rid of duplicated code in the drivers. > > > > > > Suggested-by: Stephen Boyd <swboyd@chromium.org> > > > Suggested-by: Viresh Kumar <viresh.kumar@linaro.org> > > > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org> > > > --- > > > drivers/cpufreq/cpufreq.c | 17 +++++++++++++++++ > > > include/linux/cpufreq.h | 6 ++++++ > > > 2 files changed, 23 insertions(+) > > > > > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > > > index 6f23ebb395f1..cd6e750d3d82 100644 > > > --- a/drivers/cpufreq/cpufreq.c > > > +++ b/drivers/cpufreq/cpufreq.c > > > @@ -30,6 +30,7 @@ > > > #include <linux/syscore_ops.h> > > > #include <linux/tick.h> > > > #include <trace/events/power.h> > > > +#include <linux/cpu_cooling.h> > > > > > > static LIST_HEAD(cpufreq_policy_list); > > > > > > @@ -1318,6 +1319,14 @@ static int cpufreq_online(unsigned int cpu) > > > if (cpufreq_driver->ready) > > > cpufreq_driver->ready(policy); > > > > > > +#ifdef CONFIG_CPU_THERMAL > > > + if (cpufreq_driver->flags & CPUFREQ_AUTO_REGISTER_COOLING_DEV) { > > > + struct thermal_cooling_device **cdev = &policy->cooldev; > > We use cdev for the cooling device everywhere in the kernel, so please > do s/cooldev/cdev/ in your patches. Fixed > > > + > > > + *cdev = of_cpufreq_cooling_register(policy); > > > > What would be wrong with > > > > policy->cooldev = of_cpufreq_cooling_register(policy); > > > > > + } > > > +#endif > > > > Please remove the #ifdefs from cpufreq_online() and cpufreq_offline(). > > > > Use wrappers that would become empty stubs for CONFIG_CPU_THERMAL unset. > > > > > + > > > pr_debug("initialization complete\n"); > > > > > > return 0; > > > @@ -1411,6 +1420,14 @@ static int cpufreq_offline(unsigned int cpu) > > > if (has_target()) > > > cpufreq_exit_governor(policy); > > > > > > +#ifdef CONFIG_CPU_THERMAL > > > + if (cpufreq_driver->flags & CPUFREQ_AUTO_REGISTER_COOLING_DEV) { > > > + struct thermal_cooling_device **cdev = &policy->cooldev; > > > + > > > + cpufreq_cooling_unregister(*cdev); > > > > Again, why don't you simply pass policy->cooldev here? > > I also had the same comments when I looked at your patch :) > > I also think we must do the unregistering before calling stop_cpu() > callback. Fixed. > > Also, would it make sense to clear policy->cooldev at this point? It points > > to freed memory after cpufreq_cooling_unregister(). > > Since the core doesn't refer to this field at all and uses it only > while registering/unregistering as a cooling device, there is no > technical issue that we will have today. If someone uses the dangling > pointer later on in future, it will be a bug. So I wouldn't care much > about resetting it here. > > > > + } > > > +#endif > > > + > > > /* > > > * Perform the ->exit() even during light-weight tear-down, > > > * since this is a core component, and is essential for the > > > diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h > > > index 7d0cf54125fa..70ad02088825 100644 > > > --- a/include/linux/cpufreq.h > > > +++ b/include/linux/cpufreq.h > > > @@ -390,6 +390,12 @@ struct cpufreq_driver { > > > */ > > > #define CPUFREQ_NO_AUTO_DYNAMIC_SWITCHING (1 << 6) > > > > > > +/* > > > + * Set by drivers that want the core to automatically register the cpufreq > > > + * driver as a thermal cooling device > > Add a full-stop here please. Fixed Thanks for the review. > > > + */ > > > +#define CPUFREQ_AUTO_REGISTER_COOLING_DEV (1 << 7) > > > + > > > int cpufreq_register_driver(struct cpufreq_driver *driver_data); > > > int cpufreq_unregister_driver(struct cpufreq_driver *driver_data); > > >