Re: [PATCH v2 4/4] clocksource/drivers/timer-mediatek: Make timer-mediatek become loadable module

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

Il 15/02/23 14:18, Sudeep Holla ha scritto:
> On Wed, Feb 15, 2023 at 01:43:19PM +0100, AngeloGioacchino Del Regno wrote:
>> Il 14/02/23 23:20, Sudeep Holla ha scritto:
>>> On Tue, Feb 14, 2023 at 06:53:14PM +0800, walter.chang@mediatek.com wrote:
>>>> From: Chun-Hung Wu <chun-hung.wu@mediatek.com>
>>>>
>>>> Make the timer-mediatek driver which can register
>>>> an always-on timer as tick_broadcast_device on
>>>> MediaTek SoCs become loadable module in GKI.
>>>>
>>>> Signed-off-by: Chun-Hung Wu <chun-hung.wu@mediatek.com>
>>>> ---
>>>>    drivers/clocksource/Kconfig          |  2 +-
>>>>    drivers/clocksource/timer-mediatek.c | 43 ++++++++++++++++++++++++++++
>>>>    2 files changed, 44 insertions(+), 1 deletion(-)
>>>
>>> [...]
>>>
>>>> diff --git a/drivers/clocksource/timer-mediatek.c b/drivers/clocksource/timer-mediatek.c
>>>> index d5b29fd03ca2..3358758ea694 100644
>>>> --- a/drivers/clocksource/timer-mediatek.c
>>>> +++ b/drivers/clocksource/timer-mediatek.c
>>>
>>> [...]
>>>
>>>> +static const struct of_device_id mtk_timer_match_table[] = {
>>>> +	{
>>>> +		.compatible = "mediatek,mt6577-timer",
>>>> +		.data = mtk_gpt_init,
>>>> +	},
>>>> +	{
>>>> +		.compatible = "mediatek,mt6765-timer",
>>>> +		.data = mtk_syst_init,
>>>> +	},
>>>> +	{
>>>> +		.compatible = "mediatek,mt6795-systimer",
>>>> +		.data = mtk_cpux_init,
>>>> +	},
>>>> +	{}
>>>> +};
>>>> +
>>>> +static struct platform_driver mtk_timer_driver = {
>>>> +	.probe = mtk_timer_probe,
>>>> +	.driver = {
>>>> +		.name = "mtk-timer",
>>>> +		.of_match_table = mtk_timer_match_table,
>>>> +	},
>>>> +};
>>>> +module_platform_driver(mtk_timer_driver);
>>>> +
>>>> +MODULE_DESCRIPTION("MediaTek Module Timer driver");
>>>> +MODULE_LICENSE("GPL v2");
>>>> +#else
>>>>    TIMER_OF_DECLARE(mtk_mt6577, "mediatek,mt6577-timer", mtk_gpt_init);
>>>>    TIMER_OF_DECLARE(mtk_mt6765, "mediatek,mt6765-timer", mtk_syst_init);
>>>>    TIMER_OF_DECLARE(mtk_mt6795, "mediatek,mt6795-systimer", mtk_cpux_init);
>>>
>>> Why do you need these ? If this driver can work as a module, it can be
>>> built-in module and doesn't need to be initialised early using of_timer_init
>>> (can't recall the exact name)
>>>
>>
>> Some platforms need early initialization; this is seen on ones for which the
>> bootloader does not initialize the "CPUXGPT" timer, which is used as the ARM
>> arch timer. (No, on those platforms you can't upgrade the bootloader, as it's
>> signed with a OEM key which is not obtainable, and signature verified earlier
>> in the bootchain).
>>
> 
> Is this arm32 or arm64 platform? Do you mean that these platforms don't have
> working architected timers ?
> 

Both. I mean that these platforms do have architected timers, but they are stopped
before the bootloader jumps to the kernel, or they are never started at all.

Please refer to:

https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/clocksource/timer-mediatek.c?h=next-20230215&id=327e93cf9a59b0d04eb3a31a7fdbf0f11cf13ecb

For a nice explanation.

> Quick grep suggests the below list of platforms/SoC:
> 
>   | arch/arm64/boot/dts/mediatek/mt6795.dtsi
>   | arch/arm64/boot/dts/mediatek/mt8173.dtsi
>   | arch/arm64/boot/dts/mediatek/mt8183.dtsi
>   | arch/arm64/boot/dts/mediatek/mt8186.dtsi
>   | arch/arm64/boot/dts/mediatek/mt8192.dtsi
>   | arch/arm64/boot/dts/mediatek/mt8195.dtsi
>   | arch/arm64/boot/dts/mediatek/mt8516.dtsi
>   | arch/arm/boot/dts/mt7623.dtsi
>   | arch/arm/boot/dts/mt7629.dtsi
>   | arch/arm/boot/dts/mt8127.dtsi
>   | arch/arm/boot/dts/mt8135.dtsi
> 
> All the above has architected timers and can have the other timer initialised
> at module_initcall level.
> 
>   | arch/arm/boot/dts/mt2701.dtsi
>   | arch/arm/boot/dts/mt6580.dtsi
>   | arch/arm/boot/dts/mt6582.dtsi
>   | arch/arm/boot/dts/mt6589.dtsi
>   | arch/arm/boot/dts/mt6592.dtsi
> 
> The above ones have cortex-a7 but still don't have architected timers listed
> in the DT. Anyways all use "mediatek,mt6577-timer", so except that other
> 2 can be dropped from the else and force them to be initialised later.
> 
>> As a matter of fact (and somehow obvious), on those platforms (for example,
>> MT6795.. but many other as well, really), you *need* this driver to be
>> built-in and, well, initialize the CPUX timer as early as possible :-)
>>
> 
> Built-in is not a problem, you can still remove TIMER_OF_DECLARE as
> the initialisation happens later at module_initcall level.
>