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.lezc...@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
