| Message ID | 20190402161256.11044-1-daniel.lezcano@linaro.org |
|---|---|
| State | Accepted |
| Commit | 554b3529fe018e74cb5d0d0f476ee793b58b030a |
| Headers | show |
| Series | [1/7] thermal/drivers/core: Remove the module Kconfig's option | expand |
Hi Daniel, On Tue, Apr 02, 2019 at 06:12:44PM +0200, Daniel Lezcano wrote: > The module support for the thermal subsystem makes little sense: > - some subsystems relying on it are not modules, thus forcing the > framework to be compiled in > - it is compiled in for almost every configs, the remaining ones > are a few platforms where I don't see why we can not switch the thermal > to 'y'. The drivers can stay in tristate. > - platforms need the thermal to be ready as soon as possible at boot time > in order to mitigate Nit: mitigate what? High temperatures? It feels like this sentence was cut short. > Usually the subsystems framework are compiled-in and the plugs are as module. > > Remove the module option. The removal of the module related dead code will > come after this patch gets in or is acked. > > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> > Acked-by: Guenter Roeck <groeck@chromium.org> > For mini2440: > Acked-by: Krzysztof Kozlowski <krzk@kernel.org> Acked-by: Paul Burton <paul.burton@mips.com> # MIPS part Thanks, Paul
Daniel Lezcano <daniel.lezcano@linaro.org> writes: > The module support for the thermal subsystem makes little sense: > - some subsystems relying on it are not modules, thus forcing the > framework to be compiled in > - it is compiled in for almost every configs, the remaining ones > are a few platforms where I don't see why we can not switch the thermal > to 'y'. The drivers can stay in tristate. > - platforms need the thermal to be ready as soon as possible at boot time > in order to mitigate > > Usually the subsystems framework are compiled-in and the plugs are as module. > > Remove the module option. The removal of the module related dead code will > come after this patch gets in or is acked. > > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> > Acked-by: Guenter Roeck <groeck@chromium.org> > For mini2440: > Acked-by: Krzysztof Kozlowski <krzk@kernel.org> For pxa: Acked-by: Robert Jarzmik <robert.jarzmik@free.fr> Cheers. -- Robert
Hi, Daniel,
Thanks for the patches, it looks good to me except this one and patch
4/7.
First, I don't think this is a cyclic dependency issue as they are in
the same module.
Second, I have not read include/asm-generic/vmlinux.lds.h before, it
seems that it is used for architecture specific stuff. Fix a thermal
issue in this way seems overkill to me.
IMO, to make the code clean, we can build the governors as separate
modules just like we do for cpu governors.
This brings to the old commit 80a26a5c22b9("Thermal: build thermal
governors into thermal_sys module"), and that was introduced to fix a
problem when CONFIG_THERMAL is set to 'm'. So I think we can switch
back to the old way as the problem is gone now.
what do you think?
Patch 1,2,5,6,7 applied first.
thanks,
rui
On 二, 2019-04-02 at 18:12 +0200, Daniel Lezcano wrote:
> Currently the governors are declared in their respective files but
> they
> export their [un]register functions which in turn call the
> [un]register
> the governors core's functions. That implies a cyclic dependency
> which is
> not desirable. There is a way to self-encapsulate the governors by
> letting
> them to declare themselves in a __init section table.
>
> Define the table in the asm generic linker description like the other
> tables and provide the specific macros to deal with.
>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
> drivers/thermal/thermal_core.h | 16 ++++++++++++++++
> include/asm-generic/vmlinux.lds.h | 11 +++++++++++
> 2 files changed, 27 insertions(+)
>
> diff --git a/drivers/thermal/thermal_core.h
> b/drivers/thermal/thermal_core.h
> index 0df190ed82a7..28d18083e969 100644
> --- a/drivers/thermal/thermal_core.h
> +++ b/drivers/thermal/thermal_core.h
> @@ -15,6 +15,22 @@
> /* Initial state of a cooling device during binding */
> #define THERMAL_NO_TARGET -1UL
>
> +/* Init section thermal table */
> +extern struct thermal_governor * __governor_thermal_table[];
> +extern struct thermal_governor * __governor_thermal_table_end[];
> +
> +#define THERMAL_TABLE_ENTRY(table, name) \
> + static typeof(name) * __thermal_table_entry_##name \
> + __used __section(__##table##_thermal_table) \
> + = &name;
> +
> +#define THERMAL_GOVERNOR_DECLARE(name) THERMAL_TABLE_ENTRY(go
> vernor, name)
> +
> +#define for_each_governor_table(__governor) \
> + for (__governor = __governor_thermal_table; \
> + __governor < __governor_thermal_table_end; \
> + __governor++)
> +
> /*
> * This structure is used to describe the behavior of
> * a certain cooling device on a certain trip point
> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-
> generic/vmlinux.lds.h
> index f8f6f04c4453..9893a3ed242a 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -239,6 +239,16 @@
> #define ACPI_PROBE_TABLE(name)
> #endif
>
> +#ifdef CONFIG_THERMAL
> +#define THERMAL_TABLE(name)
> \
> + . = ALIGN(8);
> \
> + __##name##_thermal_table = .;
> \
> + KEEP(*(__##name##_thermal_table))
> \
> + __##name##_thermal_table_end = .;
> +#else
> +#define THERMAL_TABLE(name)
> +#endif
> +
> #define KERNEL_DTB()
> \
> STRUCT_ALIGN();
> \
> __dtb_start = .;
> \
> @@ -609,6 +619,7 @@
> IRQCHIP_OF_MATCH_TABLE()
> \
> ACPI_PROBE_TABLE(irqchip)
> \
> ACPI_PROBE_TABLE(timer)
> \
> + THERMAL_TABLE(governor)
> \
> EARLYCON_TABLE()
> \
> LSM_TABLE()
>
Hi Zhang, On 22/04/2019 10:43, Zhang Rui wrote: > Hi, Daniel, > > Thanks for the patches, it looks good to me except this one and patch > 4/7. > > First, I don't think this is a cyclic dependency issue as they are in > the same module. The governors have to export their [un]register functions in order to have the core to use them. The core has to export the [un]register function in order to have the governors to use them. From my point of view it is a cyclic dependency. In any other subsystems, the plugins/governor/drivers/whatever don't have to export their functions to the core, they use the core's exported functions. > Second, I have not read include/asm-generic/vmlinux.lds.h before, it > seems that it is used for architecture specific stuff. Fix a thermal > issue in this way seems overkill to me. It is not architecture specific, it belongs to asm-generic. All init calls are defined in it and more. It is a common way to define static tables from different files without adding dependency and unload it after init. All clk, timers, acpi tables, irq chip, cpuidle and cpu methods are defined this way. When the thermal_core.c uses at the end fs_initcall it uses the same mechanism. > IMO, to make the code clean, we can build the governors as separate > modules just like we do for cpu governors. > This brings to the old commit 80a26a5c22b9("Thermal: build thermal > governors into thermal_sys module"), and that was introduced to fix a > problem when CONFIG_THERMAL is set to 'm'. So I think we can switch > back to the old way as the problem is gone now. > > what do you think? IMO, having the governors built as module is not a good thing because the SoC needs the governor to be ready as soon as possible at boot time. I've been told some boards reboot at boot time because the governor comes too late with the userspace governor for example. If you don't like the vmlinuz.lds.h approch (but again it is a common way to initialize table and I wrote it to extend to more thermal table in the future) we can change the thermal core to replace fs_initcall() by core_initcall() and use postcore_initcall() in the governor. > Patch 1,2,5,6,7 applied first. > > thanks, > rui > > On 二, 2019-04-02 at 18:12 +0200, Daniel Lezcano wrote: >> Currently the governors are declared in their respective files but >> they >> export their [un]register functions which in turn call the >> [un]register >> the governors core's functions. That implies a cyclic dependency >> which is >> not desirable. There is a way to self-encapsulate the governors by >> letting >> them to declare themselves in a __init section table. >> >> Define the table in the asm generic linker description like the other >> tables and provide the specific macros to deal with. >> >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> >> --- >> drivers/thermal/thermal_core.h | 16 ++++++++++++++++ >> include/asm-generic/vmlinux.lds.h | 11 +++++++++++ >> 2 files changed, 27 insertions(+) >> >> diff --git a/drivers/thermal/thermal_core.h >> b/drivers/thermal/thermal_core.h >> index 0df190ed82a7..28d18083e969 100644 >> --- a/drivers/thermal/thermal_core.h >> +++ b/drivers/thermal/thermal_core.h >> @@ -15,6 +15,22 @@ >> /* Initial state of a cooling device during binding */ >> #define THERMAL_NO_TARGET -1UL >> >> +/* Init section thermal table */ >> +extern struct thermal_governor * __governor_thermal_table[]; >> +extern struct thermal_governor * __governor_thermal_table_end[]; >> + >> +#define THERMAL_TABLE_ENTRY(table, name) \ >> + static typeof(name) * __thermal_table_entry_##name \ >> + __used __section(__##table##_thermal_table) \ >> + = &name; >> + >> +#define THERMAL_GOVERNOR_DECLARE(name) THERMAL_TABLE_ENTRY(go >> vernor, name) >> + >> +#define for_each_governor_table(__governor) \ >> + for (__governor = __governor_thermal_table; \ >> + __governor < __governor_thermal_table_end; \ >> + __governor++) >> + >> /* >> * This structure is used to describe the behavior of >> * a certain cooling device on a certain trip point >> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm- >> generic/vmlinux.lds.h >> index f8f6f04c4453..9893a3ed242a 100644 >> --- a/include/asm-generic/vmlinux.lds.h >> +++ b/include/asm-generic/vmlinux.lds.h >> @@ -239,6 +239,16 @@ >> #define ACPI_PROBE_TABLE(name) >> #endif >> >> +#ifdef CONFIG_THERMAL >> +#define THERMAL_TABLE(name) >> \ >> + . = ALIGN(8); >> \ >> + __##name##_thermal_table = .; >> \ >> + KEEP(*(__##name##_thermal_table)) >> \ >> + __##name##_thermal_table_end = .; >> +#else >> +#define THERMAL_TABLE(name) >> +#endif >> + >> #define KERNEL_DTB() >> \ >> STRUCT_ALIGN(); >> \ >> __dtb_start = .; >> \ >> @@ -609,6 +619,7 @@ >> IRQCHIP_OF_MATCH_TABLE() >> \ >> ACPI_PROBE_TABLE(irqchip) >> \ >> ACPI_PROBE_TABLE(timer) >> \ >> + THERMAL_TABLE(governor) >> \ >> EARLYCON_TABLE() >> \ >> LSM_TABLE() >> -- <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog
Hello, On Tue, Apr 02, 2019 at 06:12:44PM +0200, Daniel Lezcano wrote: > The module support for the thermal subsystem makes little sense: > - some subsystems relying on it are not modules, thus forcing the > framework to be compiled in > - it is compiled in for almost every configs, the remaining ones > are a few platforms where I don't see why we can not switch the thermal > to 'y'. The drivers can stay in tristate. > - platforms need the thermal to be ready as soon as possible at boot time > in order to mitigate > > Usually the subsystems framework are compiled-in and the plugs are as module. > > Remove the module option. The removal of the module related dead code will > come after this patch gets in or is acked. I remember some buzilla entry around this some time back. Rui, do you remember why you made this to be module? I dont have strong opinion here, but I would like to see a better description why we are going this direction rather than "most people dont use it as module". Was there any particular specific technical motivation? > > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> > Acked-by: Guenter Roeck <groeck@chromium.org> > For mini2440: > Acked-by: Krzysztof Kozlowski <krzk@kernel.org> > --- > arch/arm/configs/mini2440_defconfig | 2 +- > arch/arm/configs/pxa_defconfig | 2 +- > arch/mips/configs/ip22_defconfig | 2 +- > arch/mips/configs/ip27_defconfig | 2 +- > arch/unicore32/configs/unicore32_defconfig | 2 +- > drivers/thermal/Kconfig | 4 ++-- > 6 files changed, 7 insertions(+), 7 deletions(-) > > diff --git a/arch/arm/configs/mini2440_defconfig b/arch/arm/configs/mini2440_defconfig > index d95a8059d30b..0cf1c120c4bb 100644 > --- a/arch/arm/configs/mini2440_defconfig > +++ b/arch/arm/configs/mini2440_defconfig > @@ -152,7 +152,7 @@ CONFIG_SPI_S3C24XX=y > CONFIG_SPI_SPIDEV=y > CONFIG_GPIO_SYSFS=y > CONFIG_SENSORS_LM75=y > -CONFIG_THERMAL=m > +CONFIG_THERMAL=y > CONFIG_WATCHDOG=y > CONFIG_S3C2410_WATCHDOG=y > CONFIG_FB=y > diff --git a/arch/arm/configs/pxa_defconfig b/arch/arm/configs/pxa_defconfig > index d4654755b09c..d4f9dda3a52f 100644 > --- a/arch/arm/configs/pxa_defconfig > +++ b/arch/arm/configs/pxa_defconfig > @@ -387,7 +387,7 @@ CONFIG_SENSORS_LM75=m > CONFIG_SENSORS_LM90=m > CONFIG_SENSORS_LM95245=m > CONFIG_SENSORS_NTC_THERMISTOR=m > -CONFIG_THERMAL=m > +CONFIG_THERMAL=y > CONFIG_WATCHDOG=y > CONFIG_XILINX_WATCHDOG=m > CONFIG_SA1100_WATCHDOG=m > diff --git a/arch/mips/configs/ip22_defconfig b/arch/mips/configs/ip22_defconfig > index ff40fbc2f439..21a1168ae301 100644 > --- a/arch/mips/configs/ip22_defconfig > +++ b/arch/mips/configs/ip22_defconfig > @@ -228,7 +228,7 @@ CONFIG_SERIAL_IP22_ZILOG=m > # CONFIG_HW_RANDOM is not set > CONFIG_RAW_DRIVER=m > # CONFIG_HWMON is not set > -CONFIG_THERMAL=m > +CONFIG_THERMAL=y > CONFIG_WATCHDOG=y > CONFIG_INDYDOG=m > # CONFIG_VGA_CONSOLE is not set > diff --git a/arch/mips/configs/ip27_defconfig b/arch/mips/configs/ip27_defconfig > index 81c47e18131b..54db5dedf776 100644 > --- a/arch/mips/configs/ip27_defconfig > +++ b/arch/mips/configs/ip27_defconfig > @@ -271,7 +271,7 @@ CONFIG_I2C_PARPORT_LIGHT=m > CONFIG_I2C_TAOS_EVM=m > CONFIG_I2C_STUB=m > # CONFIG_HWMON is not set > -CONFIG_THERMAL=m > +CONFIG_THERMAL=y > CONFIG_MFD_PCF50633=m > CONFIG_PCF50633_ADC=m > CONFIG_PCF50633_GPIO=m > diff --git a/arch/unicore32/configs/unicore32_defconfig b/arch/unicore32/configs/unicore32_defconfig > index aebd01fc28e5..360cc9abcdb0 100644 > --- a/arch/unicore32/configs/unicore32_defconfig > +++ b/arch/unicore32/configs/unicore32_defconfig > @@ -119,7 +119,7 @@ CONFIG_I2C_PUV3=y > # Hardware Monitoring support > #CONFIG_SENSORS_LM75=m > # Generic Thermal sysfs driver > -#CONFIG_THERMAL=m > +#CONFIG_THERMAL=y > #CONFIG_THERMAL_HWMON=y > > # Multimedia support > diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig > index 653aa27a25a4..ccf5b9408d7a 100644 > --- a/drivers/thermal/Kconfig > +++ b/drivers/thermal/Kconfig > @@ -3,7 +3,7 @@ > # > > menuconfig THERMAL > - tristate "Generic Thermal sysfs driver" > + bool "Generic Thermal sysfs driver" > help > Generic Thermal Sysfs driver offers a generic mechanism for > thermal management. Usually it's made up of one or more thermal > @@ -11,7 +11,7 @@ menuconfig THERMAL > Each thermal zone contains its own temperature, trip points, > cooling devices. > All platforms with ACPI thermal support can use this driver. > - If you want this support, you should say Y or M here. > + If you want this support, you should say Y here. > > if THERMAL > > -- > 2.17.1 >
On Tue, Apr 23, 2019 at 9:22 PM Eduardo Valentin <edubezval@gmail.com> wrote: > > Hello, > > On Tue, Apr 02, 2019 at 06:12:44PM +0200, Daniel Lezcano wrote: > > The module support for the thermal subsystem makes little sense: > > - some subsystems relying on it are not modules, thus forcing the > > framework to be compiled in > > - it is compiled in for almost every configs, the remaining ones > > are a few platforms where I don't see why we can not switch the thermal > > to 'y'. The drivers can stay in tristate. > > - platforms need the thermal to be ready as soon as possible at boot time > > in order to mitigate > > > > Usually the subsystems framework are compiled-in and the plugs are as module. > > > > Remove the module option. The removal of the module related dead code will > > come after this patch gets in or is acked. > > > I remember some buzilla entry around this some time back. > > Rui, do you remember why you made this to be module? > > I dont have strong opinion here, but I would like to see > a better description why we are going this direction rather > than "most people dont use it as module". Was there any particular > specific technical motivation? Speaking for Qualcomm platforms, we want the thermal subsystem available as soon as possible for boot time thermal mitigation since faster boot times equals hotter cpus. Also the dependency on cpufreq subsystem due to the cpufreq cooling device would be simplified with this. In fact, I now have a follow on patch to move thermal init earlier than fs_initcall since we'd now not wait on modules to be available. /Amit
On 二, 2019-04-23 at 08:52 -0700, Eduardo Valentin wrote: > Hello, > > On Tue, Apr 02, 2019 at 06:12:44PM +0200, Daniel Lezcano wrote: > > > > The module support for the thermal subsystem makes little sense: > > - some subsystems relying on it are not modules, thus forcing the > > framework to be compiled in > > - it is compiled in for almost every configs, the remaining ones > > are a few platforms where I don't see why we can not switch the > > thermal > > to 'y'. The drivers can stay in tristate. > > - platforms need the thermal to be ready as soon as possible at > > boot time > > in order to mitigate > > > > Usually the subsystems framework are compiled-in and the plugs are > > as module. > > > > Remove the module option. The removal of the module related dead > > code will > > come after this patch gets in or is acked. > > I remember some buzilla entry around this some time back. > > Rui, do you remember why you made this to be module? > > I dont have strong opinion here, but I would like to see > a better description why we are going this direction rather > than "most people dont use it as module". Was there any particular > specific technical motivation? > I checked the change log, it seems that we make CONFIG_THERMAL from bool to tristate long time ago. commit 63c4ec905d63834a97ec7dbbf0a2ec89ef5872be Author: Zhang Rui <rui.zhang@intel.com> AuthorDate: Mon Apr 21 16:07:13 2008 +0800 Commit: Len Brown <len.brown@intel.com> CommitDate: Tue Apr 29 02:44:00 2008 -0400 thermal: add the support for building the generic thermal as a module Build the generic thermal driver as module "thermal_sys". Make ACPI thermal, video, processor and fan SELECT the generic thermal driver, as these drivers rely on it to build the sysfs I/F. Signed-off-by: Zhang Rui <rui.zhang@intel.com> Acked-by: Jean Delvare <khali@linux-fr.org> Signed-off-by: Len Brown <len.brown@intel.com> But the things described in the changelog does not seem to be a solid reason why we need thermal to be a module. Anyway, let's try bool instead of tristate and see if everything still works well. thanks, rui > > > > > > > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> > > Acked-by: Guenter Roeck <groeck@chromium.org> > > For mini2440: > > Acked-by: Krzysztof Kozlowski <krzk@kernel.org> > > --- > > arch/arm/configs/mini2440_defconfig | 2 +- > > arch/arm/configs/pxa_defconfig | 2 +- > > arch/mips/configs/ip22_defconfig | 2 +- > > arch/mips/configs/ip27_defconfig | 2 +- > > arch/unicore32/configs/unicore32_defconfig | 2 +- > > drivers/thermal/Kconfig | 4 ++-- > > 6 files changed, 7 insertions(+), 7 deletions(-) > > > > diff --git a/arch/arm/configs/mini2440_defconfig > > b/arch/arm/configs/mini2440_defconfig > > index d95a8059d30b..0cf1c120c4bb 100644 > > --- a/arch/arm/configs/mini2440_defconfig > > +++ b/arch/arm/configs/mini2440_defconfig > > @@ -152,7 +152,7 @@ CONFIG_SPI_S3C24XX=y > > CONFIG_SPI_SPIDEV=y > > CONFIG_GPIO_SYSFS=y > > CONFIG_SENSORS_LM75=y > > -CONFIG_THERMAL=m > > +CONFIG_THERMAL=y > > CONFIG_WATCHDOG=y > > CONFIG_S3C2410_WATCHDOG=y > > CONFIG_FB=y > > diff --git a/arch/arm/configs/pxa_defconfig > > b/arch/arm/configs/pxa_defconfig > > index d4654755b09c..d4f9dda3a52f 100644 > > --- a/arch/arm/configs/pxa_defconfig > > +++ b/arch/arm/configs/pxa_defconfig > > @@ -387,7 +387,7 @@ CONFIG_SENSORS_LM75=m > > CONFIG_SENSORS_LM90=m > > CONFIG_SENSORS_LM95245=m > > CONFIG_SENSORS_NTC_THERMISTOR=m > > -CONFIG_THERMAL=m > > +CONFIG_THERMAL=y > > CONFIG_WATCHDOG=y > > CONFIG_XILINX_WATCHDOG=m > > CONFIG_SA1100_WATCHDOG=m > > diff --git a/arch/mips/configs/ip22_defconfig > > b/arch/mips/configs/ip22_defconfig > > index ff40fbc2f439..21a1168ae301 100644 > > --- a/arch/mips/configs/ip22_defconfig > > +++ b/arch/mips/configs/ip22_defconfig > > @@ -228,7 +228,7 @@ CONFIG_SERIAL_IP22_ZILOG=m > > # CONFIG_HW_RANDOM is not set > > CONFIG_RAW_DRIVER=m > > # CONFIG_HWMON is not set > > -CONFIG_THERMAL=m > > +CONFIG_THERMAL=y > > CONFIG_WATCHDOG=y > > CONFIG_INDYDOG=m > > # CONFIG_VGA_CONSOLE is not set > > diff --git a/arch/mips/configs/ip27_defconfig > > b/arch/mips/configs/ip27_defconfig > > index 81c47e18131b..54db5dedf776 100644 > > --- a/arch/mips/configs/ip27_defconfig > > +++ b/arch/mips/configs/ip27_defconfig > > @@ -271,7 +271,7 @@ CONFIG_I2C_PARPORT_LIGHT=m > > CONFIG_I2C_TAOS_EVM=m > > CONFIG_I2C_STUB=m > > # CONFIG_HWMON is not set > > -CONFIG_THERMAL=m > > +CONFIG_THERMAL=y > > CONFIG_MFD_PCF50633=m > > CONFIG_PCF50633_ADC=m > > CONFIG_PCF50633_GPIO=m > > diff --git a/arch/unicore32/configs/unicore32_defconfig > > b/arch/unicore32/configs/unicore32_defconfig > > index aebd01fc28e5..360cc9abcdb0 100644 > > --- a/arch/unicore32/configs/unicore32_defconfig > > +++ b/arch/unicore32/configs/unicore32_defconfig > > @@ -119,7 +119,7 @@ CONFIG_I2C_PUV3=y > > # Hardware Monitoring support > > #CONFIG_SENSORS_LM75=m > > # Generic Thermal sysfs driver > > -#CONFIG_THERMAL=m > > +#CONFIG_THERMAL=y > > #CONFIG_THERMAL_HWMON=y > > > > # Multimedia support > > diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig > > index 653aa27a25a4..ccf5b9408d7a 100644 > > --- a/drivers/thermal/Kconfig > > +++ b/drivers/thermal/Kconfig > > @@ -3,7 +3,7 @@ > > # > > > > menuconfig THERMAL > > - tristate "Generic Thermal sysfs driver" > > + bool "Generic Thermal sysfs driver" > > help > > Generic Thermal Sysfs driver offers a generic mechanism > > for > > thermal management. Usually it's made up of one or more > > thermal > > @@ -11,7 +11,7 @@ menuconfig THERMAL > > Each thermal zone contains its own temperature, trip > > points, > > cooling devices. > > All platforms with ACPI thermal support can use this > > driver. > > - If you want this support, you should say Y or M here. > > + If you want this support, you should say Y here. > > > > if THERMAL > >
On 23/04/2019 07:59, Zhang Rui wrote: > Hi, Daniel, > > thanks for clarifying. > It is true that we need to make thermal framework ready as early as > possible. And a static table works for me as long as vmlinux.lds.h is > the proper place. > > Arnd, > are you okay with this patch? if yes, I suppose I can take it through > my tree, right? Hi Zhang, given the Acked-by from Arnd, will you add the missing patches in the tree for 5.2? > On 一, 2019-04-22 at 14:11 +0200, Daniel Lezcano wrote: >> Hi Zhang, >> >> >> On 22/04/2019 10:43, Zhang Rui wrote: >>> >>> Hi, Daniel, >>> >>> Thanks for the patches, it looks good to me except this one and >>> patch >>> 4/7. >>> >>> First, I don't think this is a cyclic dependency issue as they are >>> in >>> the same module. >> The governors have to export their [un]register functions in order to >> have the core to use them. >> >> The core has to export the [un]register function in order to have the >> governors to use them. >> >> From my point of view it is a cyclic dependency. In any other >> subsystems, the plugins/governor/drivers/whatever don't have to >> export >> their functions to the core, they use the core's exported functions. >> >>> >>> Second, I have not read include/asm-generic/vmlinux.lds.h before, >>> it >>> seems that it is used for architecture specific stuff. Fix a >>> thermal >>> issue in this way seems overkill to me. >> It is not architecture specific, it belongs to asm-generic. All init >> calls are defined in it and more. It is a common way to define static >> tables from different files without adding dependency and unload it >> after init. >> >> All clk, timers, acpi tables, irq chip, cpuidle and cpu methods are >> defined this way. >> >> When the thermal_core.c uses at the end fs_initcall it uses the same >> mechanism. >> >> >>> >>> IMO, to make the code clean, we can build the governors as separate >>> modules just like we do for cpu governors. >>> This brings to the old commit 80a26a5c22b9("Thermal: build thermal >>> governors into thermal_sys module"), and that was introduced to fix >>> a >>> problem when CONFIG_THERMAL is set to 'm'. So I think we can switch >>> back to the old way as the problem is gone now. >>> >>> what do you think? >> IMO, having the governors built as module is not a good thing because >> the SoC needs the governor to be ready as soon as possible at boot >> time. >> I've been told some boards reboot at boot time because the governor >> comes too late with the userspace governor for example. >> >> If you don't like the vmlinuz.lds.h approch (but again it is a common >> way to initialize table and I wrote it to extend to more thermal >> table >> in the future) we can change the thermal core to replace >> fs_initcall() >> by core_initcall() and use postcore_initcall() in the governor. >> >> >> >>> >>> Patch 1,2,5,6,7 applied first. >>> >>> thanks, >>> rui >>> >>> On 二, 2019-04-02 at 18:12 +0200, Daniel Lezcano wrote: >>>> >>>> Currently the governors are declared in their respective files >>>> but >>>> they >>>> export their [un]register functions which in turn call the >>>> [un]register >>>> the governors core's functions. That implies a cyclic dependency >>>> which is >>>> not desirable. There is a way to self-encapsulate the governors >>>> by >>>> letting >>>> them to declare themselves in a __init section table. >>>> >>>> Define the table in the asm generic linker description like the >>>> other >>>> tables and provide the specific macros to deal with. >>>> >>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> >>>> --- >>>> drivers/thermal/thermal_core.h | 16 ++++++++++++++++ >>>> include/asm-generic/vmlinux.lds.h | 11 +++++++++++ >>>> 2 files changed, 27 insertions(+) >>>> >>>> diff --git a/drivers/thermal/thermal_core.h >>>> b/drivers/thermal/thermal_core.h >>>> index 0df190ed82a7..28d18083e969 100644 >>>> --- a/drivers/thermal/thermal_core.h >>>> +++ b/drivers/thermal/thermal_core.h >>>> @@ -15,6 +15,22 @@ >>>> /* Initial state of a cooling device during binding */ >>>> #define THERMAL_NO_TARGET -1UL >>>> >>>> +/* Init section thermal table */ >>>> +extern struct thermal_governor * __governor_thermal_table[]; >>>> +extern struct thermal_governor * __governor_thermal_table_end[]; >>>> + >>>> +#define THERMAL_TABLE_ENTRY(table, name) >>>> \ >>>> + static typeof(name) * __thermal_table_entry_##name >>>> \ >>>> + __used __section(__##table##_thermal_table) >>>> \ >>>> + = &name; >>>> + >>>> +#define THERMAL_GOVERNOR_DECLARE(name) THERMAL_TABLE_ENTR >>>> Y(go >>>> vernor, name) >>>> + >>>> +#define for_each_governor_table(__governor) \ >>>> + for (__governor = __governor_thermal_table; \ >>>> + __governor < __governor_thermal_table_end; \ >>>> + __governor++) >>>> + >>>> /* >>>> * This structure is used to describe the behavior of >>>> * a certain cooling device on a certain trip point >>>> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm- >>>> generic/vmlinux.lds.h >>>> index f8f6f04c4453..9893a3ed242a 100644 >>>> --- a/include/asm-generic/vmlinux.lds.h >>>> +++ b/include/asm-generic/vmlinux.lds.h >>>> @@ -239,6 +239,16 @@ >>>> #define ACPI_PROBE_TABLE(name) >>>> #endif >>>> >>>> +#ifdef CONFIG_THERMAL >>>> +#define THERMAL_TABLE(name) >>>> >>>> \ >>>> + . = ALIGN(8); >>>> \ >>>> + __##name##_thermal_table = .; >>>> \ >>>> + KEEP(*(__##name##_thermal_table)) >>>> >>>> \ >>>> + __##name##_thermal_table_end = .; >>>> +#else >>>> +#define THERMAL_TABLE(name) >>>> +#endif >>>> + >>>> #define KERNEL_DTB() >>>> >>>> \ >>>> STRUCT_ALIGN(); >>>> \ >>>> __dtb_start = .; >>>> \ >>>> @@ -609,6 +619,7 @@ >>>> IRQCHIP_OF_MATCH_TABLE() >>>> \ >>>> ACPI_PROBE_TABLE(irqchip) >>>> >>>> \ >>>> ACPI_PROBE_TABLE(timer) >>>> \ >>>> + THERMAL_TABLE(governor) >>>> \ >>>> EARLYCON_TABLE() >>>> \ >>>> LSM_TABLE() >>>> >> -- <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog
diff --git a/arch/arm/configs/mini2440_defconfig b/arch/arm/configs/mini2440_defconfig index d95a8059d30b..0cf1c120c4bb 100644 --- a/arch/arm/configs/mini2440_defconfig +++ b/arch/arm/configs/mini2440_defconfig @@ -152,7 +152,7 @@ CONFIG_SPI_S3C24XX=y CONFIG_SPI_SPIDEV=y CONFIG_GPIO_SYSFS=y CONFIG_SENSORS_LM75=y -CONFIG_THERMAL=m +CONFIG_THERMAL=y CONFIG_WATCHDOG=y CONFIG_S3C2410_WATCHDOG=y CONFIG_FB=y diff --git a/arch/arm/configs/pxa_defconfig b/arch/arm/configs/pxa_defconfig index d4654755b09c..d4f9dda3a52f 100644 --- a/arch/arm/configs/pxa_defconfig +++ b/arch/arm/configs/pxa_defconfig @@ -387,7 +387,7 @@ CONFIG_SENSORS_LM75=m CONFIG_SENSORS_LM90=m CONFIG_SENSORS_LM95245=m CONFIG_SENSORS_NTC_THERMISTOR=m -CONFIG_THERMAL=m +CONFIG_THERMAL=y CONFIG_WATCHDOG=y CONFIG_XILINX_WATCHDOG=m CONFIG_SA1100_WATCHDOG=m diff --git a/arch/mips/configs/ip22_defconfig b/arch/mips/configs/ip22_defconfig index ff40fbc2f439..21a1168ae301 100644 --- a/arch/mips/configs/ip22_defconfig +++ b/arch/mips/configs/ip22_defconfig @@ -228,7 +228,7 @@ CONFIG_SERIAL_IP22_ZILOG=m # CONFIG_HW_RANDOM is not set CONFIG_RAW_DRIVER=m # CONFIG_HWMON is not set -CONFIG_THERMAL=m +CONFIG_THERMAL=y CONFIG_WATCHDOG=y CONFIG_INDYDOG=m # CONFIG_VGA_CONSOLE is not set diff --git a/arch/mips/configs/ip27_defconfig b/arch/mips/configs/ip27_defconfig index 81c47e18131b..54db5dedf776 100644 --- a/arch/mips/configs/ip27_defconfig +++ b/arch/mips/configs/ip27_defconfig @@ -271,7 +271,7 @@ CONFIG_I2C_PARPORT_LIGHT=m CONFIG_I2C_TAOS_EVM=m CONFIG_I2C_STUB=m # CONFIG_HWMON is not set -CONFIG_THERMAL=m +CONFIG_THERMAL=y CONFIG_MFD_PCF50633=m CONFIG_PCF50633_ADC=m CONFIG_PCF50633_GPIO=m diff --git a/arch/unicore32/configs/unicore32_defconfig b/arch/unicore32/configs/unicore32_defconfig index aebd01fc28e5..360cc9abcdb0 100644 --- a/arch/unicore32/configs/unicore32_defconfig +++ b/arch/unicore32/configs/unicore32_defconfig @@ -119,7 +119,7 @@ CONFIG_I2C_PUV3=y # Hardware Monitoring support #CONFIG_SENSORS_LM75=m # Generic Thermal sysfs driver -#CONFIG_THERMAL=m +#CONFIG_THERMAL=y #CONFIG_THERMAL_HWMON=y # Multimedia support diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig index 653aa27a25a4..ccf5b9408d7a 100644 --- a/drivers/thermal/Kconfig +++ b/drivers/thermal/Kconfig @@ -3,7 +3,7 @@ # menuconfig THERMAL - tristate "Generic Thermal sysfs driver" + bool "Generic Thermal sysfs driver" help Generic Thermal Sysfs driver offers a generic mechanism for thermal management. Usually it's made up of one or more thermal @@ -11,7 +11,7 @@ menuconfig THERMAL Each thermal zone contains its own temperature, trip points, cooling devices. All platforms with ACPI thermal support can use this driver. - If you want this support, you should say Y or M here. + If you want this support, you should say Y here. if THERMAL