[PATCH v2 0/4] Support timer drivers as loadable modules

[PATCH v2 0/4] Support timer drivers as loadable modules

[walter.chang]

From: Walter Chang <walter.chang@mediatek.com>

This set of patches aims to make SoC related timer drivers, such as 
timer-mediatek.c become loadable modules for the Generic Kernel Image
(GKI).

This driver registers an always-on timer as tick_broadcast_device on 
MediaTek SoCs. If the system does not load this module at startup, 
system will also boot normally by using built-in architecture timer 
(in this case is Arm Generic Timer) instead.

The first three patches export functions and remove __init markings to 
support loadable timer modules.

The fourth patch makes timer-mediatek.c become loadable module for GKI.

[v2]
- Convert timer-mediatek.c driver to loadable module

Chun-Hung Wu (4):
  time/sched_clock: Export sched_clock_register()
  clocksource/drivers/mmio: Export clocksource_mmio_init()
  clocksource/drivers/timer-of: Remove __init markings
  clocksource/drivers/timer-mediatek: Make timer-mediatek become
    loadable module

 drivers/clocksource/Kconfig          |  2 +-
 drivers/clocksource/mmio.c           |  8 ++++--
 drivers/clocksource/timer-mediatek.c | 43 ++++++++++++++++++++++++++++
 drivers/clocksource/timer-of.c       | 23 ++++++++-------
 drivers/clocksource/timer-of.h       |  6 ++--
 kernel/time/sched_clock.c            |  4 +--
 6 files changed, 66 insertions(+), 20 deletions(-)

-- 
2.18.0

[PATCH v2 1/4] time/sched_clock: Export sched_clock_register()

[walter.chang]

From: Chun-Hung Wu <chun-hung.wu@mediatek.com>

clocksource driver may use sched_clock_register()
to resigter itself as a sched_clock source.
Export it to support building such driver
as module, like timer-mediatek.c

Signed-off-by: Chun-Hung Wu <chun-hung.wu@mediatek.com>
---
 kernel/time/sched_clock.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/time/sched_clock.c b/kernel/time/sched_clock.c
index 8464c5acc913..8e49e87d1221 100644
--- a/kernel/time/sched_clock.c
+++ b/kernel/time/sched_clock.c
@@ -150,8 +150,7 @@ static enum hrtimer_restart sched_clock_poll(struct hrtimer *hrt)
 	return HRTIMER_RESTART;
 }
 
-void __init
-sched_clock_register(u64 (*read)(void), int bits, unsigned long rate)
+void sched_clock_register(u64 (*read)(void), int bits, unsigned long rate)
 {
 	u64 res, wrap, new_mask, new_epoch, cyc, ns;
 	u32 new_mult, new_shift;
@@ -223,6 +222,7 @@ sched_clock_register(u64 (*read)(void), int bits, unsigned long rate)
 
 	pr_debug("Registered %pS as sched_clock source\n", read);
 }
+EXPORT_SYMBOL_GPL(sched_clock_register);
 
 void __init generic_sched_clock_init(void)
 {
-- 
2.18.0

[PATCH v2 2/4] clocksource/drivers/mmio: Export clocksource_mmio_init()

[walter.chang]

From: Chun-Hung Wu <chun-hung.wu@mediatek.com>

Export clocksource_mmio_init() and clocksource_mmio_readl_up()
to support building clocksource driver as module,
such as timer-mediatek.c.

Signed-off-by: Chun-Hung Wu <chun-hung.wu@mediatek.com>
---
 drivers/clocksource/mmio.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/clocksource/mmio.c b/drivers/clocksource/mmio.c
index 9de751531831..b08b2f9d7a8b 100644
--- a/drivers/clocksource/mmio.c
+++ b/drivers/clocksource/mmio.c
@@ -21,6 +21,7 @@ u64 clocksource_mmio_readl_up(struct clocksource *c)
 {
 	return (u64)readl_relaxed(to_mmio_clksrc(c)->reg);
 }
+EXPORT_SYMBOL_GPL(clocksource_mmio_readl_up);
 
 u64 clocksource_mmio_readl_down(struct clocksource *c)
 {
@@ -46,9 +47,9 @@ u64 clocksource_mmio_readw_down(struct clocksource *c)
  * @bits:	Number of valid bits
  * @read:	One of clocksource_mmio_read*() above
  */
-int __init clocksource_mmio_init(void __iomem *base, const char *name,
-	unsigned long hz, int rating, unsigned bits,
-	u64 (*read)(struct clocksource *))
+int clocksource_mmio_init(void __iomem *base, const char *name,
+			  unsigned long hz, int rating, unsigned bits,
+			  u64 (*read)(struct clocksource *))
 {
 	struct clocksource_mmio *cs;
 
@@ -68,3 +69,4 @@ int __init clocksource_mmio_init(void __iomem *base, const char *name,
 
 	return clocksource_register_hz(&cs->clksrc, hz);
 }
+EXPORT_SYMBOL_GPL(clocksource_mmio_init);
-- 
2.18.0

[PATCH v2 3/4] clocksource/drivers/timer-of: Remove __init markings

[walter.chang]

From: Chun-Hung Wu <chun-hung.wu@mediatek.com>

Remove __init markings to allow timer drivers 
can be compiled as modules.

Signed-off-by: Chun-Hung Wu <chun-hung.wu@mediatek.com>
---
 drivers/clocksource/timer-of.c | 23 ++++++++++++-----------
 drivers/clocksource/timer-of.h |  6 +++---
 2 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/drivers/clocksource/timer-of.c b/drivers/clocksource/timer-of.c
index c3f54d9912be..59bc5921acad 100644
--- a/drivers/clocksource/timer-of.c
+++ b/drivers/clocksource/timer-of.c
@@ -19,7 +19,7 @@
  *
  * Free the irq resource
  */
-static __init void timer_of_irq_exit(struct of_timer_irq *of_irq)
+static void timer_of_irq_exit(struct of_timer_irq *of_irq)
 {
 	struct timer_of *to = container_of(of_irq, struct timer_of, of_irq);
 
@@ -47,8 +47,8 @@ static __init void timer_of_irq_exit(struct of_timer_irq *of_irq)
  *
  * Returns 0 on success, < 0 otherwise
  */
-static __init int timer_of_irq_init(struct device_node *np,
-				    struct of_timer_irq *of_irq)
+static int timer_of_irq_init(struct device_node *np,
+			     struct of_timer_irq *of_irq)
 {
 	int ret;
 	struct timer_of *to = container_of(of_irq, struct timer_of, of_irq);
@@ -91,7 +91,7 @@ static __init int timer_of_irq_init(struct device_node *np,
  *
  * Disables and releases the refcount on the clk
  */
-static __init void timer_of_clk_exit(struct of_timer_clk *of_clk)
+static void timer_of_clk_exit(struct of_timer_clk *of_clk)
 {
 	of_clk->rate = 0;
 	clk_disable_unprepare(of_clk->clk);
@@ -107,8 +107,8 @@ static __init void timer_of_clk_exit(struct of_timer_clk *of_clk)
  *
  * Returns 0 on success, < 0 otherwise
  */
-static __init int timer_of_clk_init(struct device_node *np,
-				    struct of_timer_clk *of_clk)
+static int timer_of_clk_init(struct device_node *np,
+			     struct of_timer_clk *of_clk)
 {
 	int ret;
 
@@ -146,13 +146,13 @@ static __init int timer_of_clk_init(struct device_node *np,
 	goto out;
 }
 
-static __init void timer_of_base_exit(struct of_timer_base *of_base)
+static void timer_of_base_exit(struct of_timer_base *of_base)
 {
 	iounmap(of_base->base);
 }
 
-static __init int timer_of_base_init(struct device_node *np,
-				     struct of_timer_base *of_base)
+static int timer_of_base_init(struct device_node *np,
+			      struct of_timer_base *of_base)
 {
 	of_base->base = of_base->name ?
 		of_io_request_and_map(np, of_base->index, of_base->name) :
@@ -165,7 +165,7 @@ static __init int timer_of_base_init(struct device_node *np,
 	return 0;
 }
 
-int __init timer_of_init(struct device_node *np, struct timer_of *to)
+int timer_of_init(struct device_node *np, struct timer_of *to)
 {
 	int ret = -EINVAL;
 	int flags = 0;
@@ -209,6 +209,7 @@ int __init timer_of_init(struct device_node *np, struct timer_of *to)
 		timer_of_base_exit(&to->of_base);
 	return ret;
 }
+EXPORT_SYMBOL_GPL(timer_of_init);
 
 /**
  * timer_of_cleanup - release timer_of resources
@@ -217,7 +218,7 @@ int __init timer_of_init(struct device_node *np, struct timer_of *to)
  * Release the resources that has been used in timer_of_init().
  * This function should be called in init error cases
  */
-void __init timer_of_cleanup(struct timer_of *to)
+void timer_of_cleanup(struct timer_of *to)
 {
 	if (to->flags & TIMER_OF_IRQ)
 		timer_of_irq_exit(&to->of_irq);
diff --git a/drivers/clocksource/timer-of.h b/drivers/clocksource/timer-of.h
index a5478f3e8589..5d1472846346 100644
--- a/drivers/clocksource/timer-of.h
+++ b/drivers/clocksource/timer-of.h
@@ -66,9 +66,9 @@ static inline unsigned long timer_of_period(struct timer_of *to)
 	return to->of_clk.period;
 }
 
-extern int __init timer_of_init(struct device_node *np,
-				struct timer_of *to);
+extern int timer_of_init(struct device_node *np,
+			 struct timer_of *to);
 
-extern void __init timer_of_cleanup(struct timer_of *to);
+extern void timer_of_cleanup(struct timer_of *to);
 
 #endif
-- 
2.18.0

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

[walter.chang]

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/Kconfig b/drivers/clocksource/Kconfig
index 4469e7f555e9..41345827055b 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -472,7 +472,7 @@ config SYS_SUPPORTS_SH_CMT
 	bool
 
 config MTK_TIMER
-	bool "Mediatek timer driver" if COMPILE_TEST
+	tristate "Mediatek timer driver"
 	depends on HAS_IOMEM
 	select TIMER_OF
 	select CLKSRC_MMIO
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
@@ -13,6 +13,9 @@
 #include <linux/clocksource.h>
 #include <linux/interrupt.h>
 #include <linux/irqreturn.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
 #include <linux/sched_clock.h>
 #include <linux/slab.h>
 #include "timer-of.h"
@@ -450,6 +453,46 @@ static int __init mtk_gpt_init(struct device_node *node)
 
 	return 0;
 }
+
+#ifdef MODULE
+static int mtk_timer_probe(struct platform_device *pdev)
+{
+	int (*timer_init)(struct device_node *node);
+	struct device_node *np = pdev->dev.of_node;
+
+	timer_init = of_device_get_match_data(&pdev->dev);
+	return timer_init(np);
+}
+
+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);
+#endif
-- 
2.18.0

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

[Krzysztof Kozlowski]

On 14/02/2023 11:53, 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.

Other questions are unanswered. Please do not ignore feedback and
respond to it.

Best regards,
Krzysztof

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

[John Stultz]

On Tue, Feb 14, 2023 at 3:09 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
> On 14/02/2023 11:53, 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.
>
> Other questions are unanswered. Please do not ignore feedback and
> respond to it.
>

Apologies, I know it can be a pain to repeat yourself, but would you
clarify which questions were unanswered?
My initial skim made it seem like the items you raised were addressed
in some form (though maybe not sufficiently?).

thanks
-john

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

[Krzysztof Kozlowski]

On 15/02/2023 00:20, John Stultz wrote:
> On Tue, Feb 14, 2023 at 3:09 AM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>> On 14/02/2023 11:53, 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.
>>
>> Other questions are unanswered. Please do not ignore feedback and
>> respond to it.
>>
> 
> Apologies, I know it can be a pain to repeat yourself, but would you
> clarify which questions were unanswered?
> My initial skim made it seem like the items you raised were addressed
> in some form (though maybe not sufficiently?).

Questions were:

1. IOW, does the system boot fine? What's the impact of this being a module?

2. It is not the first time there is a proposal to convert the timers to
modules. After asking, nobody came with a real study regarding the
impact of the modularization of these drivers vs the time core framework
and the benefit.

3. We need to tests that involve loading and unloading of such
modules to see if the transition between this timer as broadcast and one
CPU itself as broadcast happens correctly and system survives across such
loading and unloading of the modules.


All these emails or comments were simply ignored.


Best regards,
Krzysztof

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

[John Stultz]

On Wed, Feb 15, 2023 at 10:35 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 15/02/2023 00:20, John Stultz wrote:
> > On Tue, Feb 14, 2023 at 3:09 AM Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> >> On 14/02/2023 11:53, 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.
> >>
> >> Other questions are unanswered. Please do not ignore feedback and
> >> respond to it.
> >>
> >
> > Apologies, I know it can be a pain to repeat yourself, but would you
> > clarify which questions were unanswered?
> > My initial skim made it seem like the items you raised were addressed
> > in some form (though maybe not sufficiently?).
>
> Questions were:
>
> 1. IOW, does the system boot fine? What's the impact of this being a module?

I believe this was answered in the cover letter.
" If the system does not load this module at startup, system will also
boot normally by using built-in architecture timer (in this case is
Arm Generic Timer) instead."

> 2. It is not the first time there is a proposal to convert the timers to
> modules. After asking, nobody came with a real study regarding the
> impact of the modularization of these drivers vs the time core framework
> and the benefit.

Maybe it would be helpful to be more specific in the criteria you're
looking for in such a study?

At least with the GKI, there is a need to support a wide array of
hardware, while minimizing the memory overhead of unnecessary code on
each device. As I mentioned in an earlier reply, this is kernel wide,
so while moving a single driver out to being a module isn't going to
be very substantial, the cumulative effect of not having to build in
every driver needed adds up.

So I think asking to see how much the kernel size changes by
modularizing this one initial driver is a reasonable request, though
I'd not expect it to be a huge gain on its own, but a reduction is a
reduction, and I'm not sure there are many clear downsides.

Again, it's not expected that every driver can be moved to a module,
as there are a number of cases where we need the functionality to be
present early in boot, and that's fine.

> 3. We need to tests that involve loading and unloading of such
> modules to see if the transition between this timer as broadcast and one
> CPU itself as broadcast happens correctly and system survives across such
> loading and unloading of the modules.

Modules may be permanent and not unloadable (this patch doesn't
provide a remove hook). While unloadable modules are abstractly nicer,
for supporting a wide array of hardware with minimal memory impact,
permanent modules are equally beneficial (only load them on hardware
that needs them, from that point there is no need to unload them). So
I'm not sure there's much practical value in unloading them.

As for testing the loading side, that sounds fair, though I'd expect
that to be done as part of developing the patch.

So maybe Walter can confirm the device works appropriately across many
boot ups where the module is loaded in their testing? If there is a
specific test criteria you would like to see, please clarify.

And, many thanks for re-outlining your concerns here! I appreciate it!

thanks
-john

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

[Sudeep Holla]

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)


-- 
Regards,
Sudeep

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

[AngeloGioacchino Del Regno]

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).

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 :-)

Regards,
Angelo

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

[Sudeep Holla]

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 ?

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.

-- 
Regards,
Sudeep

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

[AngeloGioacchino Del Regno]

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

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

[Sudeep Holla]

On Wed, Feb 15, 2023 at 02:30:51PM +0100, AngeloGioacchino Del Regno wrote:
> 
> 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.
> 

Thanks for that. Well then I see no point in making these modules if you
can't have generic Image that boots on all the platform. I now tend to think
that these are made modules just because GKI demands and it *might* work
on one or 2 platforms. One we move this as modules, how will be know the
Image without these timers or with them built as modules will boot or not
on a given mediatek platform. Sorry, I initially saw some point in making
these timers as modules but if they are required for boot on some systems
then I see no point. So if that is the case, NACK for these as it just
creates more confusion after these are merged as why some Images or
even why defconfig image(if we push the config change as well) is not
booting on these platforms.

It is no longer just for system timer useful in low power CPU idle states
as I initial thought.

-- 
Regards,
Sudeep

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

[John Stultz]

On Wed, Feb 15, 2023 at 6:46 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Wed, Feb 15, 2023 at 02:30:51PM +0100, AngeloGioacchino Del Regno wrote:
> >
> > 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.
> >
>
> Thanks for that. Well then I see no point in making these modules if you
> can't have generic Image that boots on all the platform. I now tend to think
> that these are made modules just because GKI demands and it *might* work
> on one or 2 platforms. One we move this as modules, how will be know the
> Image without these timers or with them built as modules will boot or not
> on a given mediatek platform. Sorry, I initially saw some point in making
> these timers as modules but if they are required for boot on some systems
> then I see no point. So if that is the case, NACK for these as it just
> creates more confusion after these are merged as why some Images or
> even why defconfig image(if we push the config change as well) is not
> booting on these platforms.

Indeed, if some hardware does have a hard requirement then the
timer-mediatek driver probably isn't a good candidate for being a
module.

Though I wonder if it would make sense to "virtually" split the driver
in two? Have one config for hardware where it is safe to be modular,
and another for the problematic hardware that forces it to be built
in?
That way we can support the safe approach (ends up built in if both
options are selected), but for GKI devices where the hardware isn't
problematic we can still leave it as a module?

thanks
-john

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

[AngeloGioacchino Del Regno]

Il 15/02/23 15:46, Sudeep Holla ha scritto:
> On Wed, Feb 15, 2023 at 02:30:51PM +0100, AngeloGioacchino Del Regno wrote:
>>
>> 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.
>>
> 
> Thanks for that. Well then I see no point in making these modules if you
> can't have generic Image that boots on all the platform. I now tend to think
> that these are made modules just because GKI demands and it *might* work
> on one or 2 platforms. One we move this as modules, how will be know the
> Image without these timers or with them built as modules will boot or not
> on a given mediatek platform. Sorry, I initially saw some point in making
> these timers as modules but if they are required for boot on some systems
> then I see no point. So if that is the case, NACK for these as it just
> creates more confusion after these are merged as why some Images or
> even why defconfig image(if we push the config change as well) is not
> booting on these platforms.
> 
> It is no longer just for system timer useful in low power CPU idle states
> as I initial thought.
> 

I think that there is still a point in modularization for this driver and I
can propose a rather simple solution, even though this may add some, rather
little, code duplication to the mix.

The platforms that I've described (like mt6795) need the system timer to be
initialized as early as possible - that's true - but that timer is always
"CPUXGPT".

On those platforms, you *still* have multiple timers:
  - CPUX (short for cpuxgpt), used only as system timer;
  - SYST, as another system timer implementation (additional timers) but
    those are always initialized (AFAIK) from the bootloader before booting;
  - GPT (General Purpose Timer).

On one SoC, you may have:
  - CPUX *and* SYST
  - CPUX *and* GPT
  - CPUX *and* SYST *and* GPT

... where the only one that is boot critical and needs to be initialized early
is always only CPUX.

Hence this proposal: to still allow modularization of timers on MediaTek platforms,
we could eventually split the CPUX as a separated driver that *cannot be*, due to
the previously explained constraints, compiled as module, hence always built-in,
from a timer-mediatek driver that could be a module and capable of handling only
SYST and GPT timers.

In that case, we'd hence have...
  - timer-mediatek-cpux.o (bool)
  - timer-mediatek.c (tristate)

Counting that the CPUX timers are actually even using different `tick_resume`
and `set_state_shutdown` callbacks (doing only a IRQ clear/restore and nothing
else), the amount of duplication would be .. well, again, minimal, but then
this means that timer-mediatek-cpux would be `default y if ARCH_MEDIATEK`, or
even selected by ARCH_MEDIATEK itself.

If you think that this could be a good solution, I can send a "fast" patch to
split it out, preparing the ground for the people doing this module work.

Any considerations?

Regards,
Angelo

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

[Matthias Brugger]

On 16/02/2023 11:22, AngeloGioacchino Del Regno wrote:
> Il 15/02/23 15:46, Sudeep Holla ha scritto:
>> On Wed, Feb 15, 2023 at 02:30:51PM +0100, AngeloGioacchino Del Regno wrote:
>>>
>>> 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.
>>>
>>
>> Thanks for that. Well then I see no point in making these modules if you
>> can't have generic Image that boots on all the platform. I now tend to think
>> that these are made modules just because GKI demands and it *might* work
>> on one or 2 platforms. One we move this as modules, how will be know the
>> Image without these timers or with them built as modules will boot or not
>> on a given mediatek platform. Sorry, I initially saw some point in making
>> these timers as modules but if they are required for boot on some systems
>> then I see no point. So if that is the case, NACK for these as it just
>> creates more confusion after these are merged as why some Images or
>> even why defconfig image(if we push the config change as well) is not
>> booting on these platforms.
>>
>> It is no longer just for system timer useful in low power CPU idle states
>> as I initial thought.
>>
> 
> I think that there is still a point in modularization for this driver and I
> can propose a rather simple solution, even though this may add some, rather
> little, code duplication to the mix.
> 
> The platforms that I've described (like mt6795) need the system timer to be
> initialized as early as possible - that's true - but that timer is always
> "CPUXGPT".
> 
> On those platforms, you *still* have multiple timers:
>   - CPUX (short for cpuxgpt), used only as system timer;
>   - SYST, as another system timer implementation (additional timers) but
>     those are always initialized (AFAIK) from the bootloader before booting;
>   - GPT (General Purpose Timer).
> 
> On one SoC, you may have:
>   - CPUX *and* SYST
>   - CPUX *and* GPT
>   - CPUX *and* SYST *and* GPT
> 
> ... where the only one that is boot critical and needs to be initialized early
> is always only CPUX.
> 
> Hence this proposal: to still allow modularization of timers on MediaTek platforms,
> we could eventually split the CPUX as a separated driver that *cannot be*, due to
> the previously explained constraints, compiled as module, hence always built-in,
> from a timer-mediatek driver that could be a module and capable of handling only
> SYST and GPT timers.
> 
> In that case, we'd hence have...
>   - timer-mediatek-cpux.o (bool)
>   - timer-mediatek.c (tristate)
> 
> Counting that the CPUX timers are actually even using different `tick_resume`
> and `set_state_shutdown` callbacks (doing only a IRQ clear/restore and nothing
> else), the amount of duplication would be .. well, again, minimal, but then
> this means that timer-mediatek-cpux would be `default y if ARCH_MEDIATEK`, or
> even selected by ARCH_MEDIATEK itself.
> 
> If you think that this could be a good solution, I can send a "fast" patch to
> split it out, preparing the ground for the people doing this module work.
> 
> Any considerations?
> 

I think your proposal sounds acceptable, but we would need to make sure that all 
SoCs can boot with the CPUX driver. I'm aware of some armv7 SoCs that use a kind 
of hack to enable the architecture timer [1]. This, for one part, should be 
moved to CPUX, if possible. For the other part it makes me wonder if really all 
supported MediaTek platforms will boot with SYST/GPT being a module. I think we 
will need some effort from the community to test that.

So as a resume, yes I think your approach is feasible but we should collect 
tested-by tags before merging it.

Regards,
Matthias


[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/mach-mediatek/mediatek.c?h=v6.2-rc8#n16

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

[AngeloGioacchino Del Regno]

Il 16/02/23 12:23, Matthias Brugger ha scritto:
> 
> 
> On 16/02/2023 11:22, AngeloGioacchino Del Regno wrote:
>> Il 15/02/23 15:46, Sudeep Holla ha scritto:
>>> On Wed, Feb 15, 2023 at 02:30:51PM +0100, AngeloGioacchino Del Regno wrote:
>>>>
>>>> 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.
>>>>
>>>
>>> Thanks for that. Well then I see no point in making these modules if you
>>> can't have generic Image that boots on all the platform. I now tend to think
>>> that these are made modules just because GKI demands and it *might* work
>>> on one or 2 platforms. One we move this as modules, how will be know the
>>> Image without these timers or with them built as modules will boot or not
>>> on a given mediatek platform. Sorry, I initially saw some point in making
>>> these timers as modules but if they are required for boot on some systems
>>> then I see no point. So if that is the case, NACK for these as it just
>>> creates more confusion after these are merged as why some Images or
>>> even why defconfig image(if we push the config change as well) is not
>>> booting on these platforms.
>>>
>>> It is no longer just for system timer useful in low power CPU idle states
>>> as I initial thought.
>>>
>>
>> I think that there is still a point in modularization for this driver and I
>> can propose a rather simple solution, even though this may add some, rather
>> little, code duplication to the mix.
>>
>> The platforms that I've described (like mt6795) need the system timer to be
>> initialized as early as possible - that's true - but that timer is always
>> "CPUXGPT".
>>
>> On those platforms, you *still* have multiple timers:
>>   - CPUX (short for cpuxgpt), used only as system timer;
>>   - SYST, as another system timer implementation (additional timers) but
>>     those are always initialized (AFAIK) from the bootloader before booting;
>>   - GPT (General Purpose Timer).
>>
>> On one SoC, you may have:
>>   - CPUX *and* SYST
>>   - CPUX *and* GPT
>>   - CPUX *and* SYST *and* GPT
>>
>> ... where the only one that is boot critical and needs to be initialized early
>> is always only CPUX.
>>
>> Hence this proposal: to still allow modularization of timers on MediaTek platforms,
>> we could eventually split the CPUX as a separated driver that *cannot be*, due to
>> the previously explained constraints, compiled as module, hence always built-in,
>> from a timer-mediatek driver that could be a module and capable of handling only
>> SYST and GPT timers.
>>
>> In that case, we'd hence have...
>>   - timer-mediatek-cpux.o (bool)
>>   - timer-mediatek.c (tristate)
>>
>> Counting that the CPUX timers are actually even using different `tick_resume`
>> and `set_state_shutdown` callbacks (doing only a IRQ clear/restore and nothing
>> else), the amount of duplication would be .. well, again, minimal, but then
>> this means that timer-mediatek-cpux would be `default y if ARCH_MEDIATEK`, or
>> even selected by ARCH_MEDIATEK itself.
>>
>> If you think that this could be a good solution, I can send a "fast" patch to
>> split it out, preparing the ground for the people doing this module work.
>>
>> Any considerations?
>>
> 
> I think your proposal sounds acceptable, but we would need to make sure that all 
> SoCs can boot with the CPUX driver. I'm aware of some armv7 SoCs that use a kind of 
> hack to enable the architecture timer [1]. This, for one part, should be moved to 
> CPUX, if possible. For the other part it makes me wonder if really all supported 
> MediaTek platforms will boot with SYST/GPT being a module. I think we will need 
> some effort from the community to test that.
> 
> So as a resume, yes I think your approach is feasible but we should collect 
> tested-by tags before merging it.
> 
> Regards,
> Matthias
> 
> 
> [1] 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/mach-mediatek/mediatek.c?h=v6.2-rc8#n16

Right. I completely forgot about those platforms, even though my intention was
to actually try and migrate them once the CPUX was picked. My bad.

Well, I think that this module conversion will take quite a while, so there
should be no need to rush... I'll send a series later with the split *and* a
conversion of those platforms, so that we will see a removal of that
mediatek_timer_init() function.

Some encouraging words ahead: I'm totally confident that the conversion will
Just Work, because the MT6795 SoC had the same handling for CPUXGPT as the
older MT6589/7623/8127/8135... as that SoC had two implementations initially,
one as ARM, one as ARM64.

Whatever - let's see what I can come up with, then.

Cheers,
Angelo

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

[Walter Chang (張維哲)]

On Thu, 2023-02-16 at 12:36 +0100, AngeloGioacchino Del Regno wrote:
> Il 16/02/23 12:23, Matthias Brugger ha scritto:
> > 
> > 
> > On 16/02/2023 11:22, AngeloGioacchino Del Regno wrote:
> > > Il 15/02/23 15:46, Sudeep Holla ha scritto:
> > > > On Wed, Feb 15, 2023 at 02:30:51PM +0100, AngeloGioacchino Del
> > > > Regno wrote:
> > > > > 
> > > > > 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://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/clocksource/timer-mediatek.c?h=next-20230215&id=327e93cf9a59b0d04eb3a31a7fdbf0f11cf13ecb__;!!CTRNKA9wMg0ARbw!jOX04TQn1HXcKOdzAiK3ZlqsSE3j3p6Zo-Cajr0khXC9ANlbkl8kQrUgx6wadR8b_46o9J0SbJgjhoS03rQ7somJfA5Z9L6G_g$ 
> > > > >  
> > > > > 
> > > > > For a nice explanation.
> > > > > 
> > > > 
> > > > Thanks for that. Well then I see no point in making these
> > > > modules if you
> > > > can't have generic Image that boots on all the platform. I now
> > > > tend to think
> > > > that these are made modules just because GKI demands and it
> > > > *might* work
> > > > on one or 2 platforms. One we move this as modules, how will be
> > > > know the
> > > > Image without these timers or with them built as modules will
> > > > boot or not
> > > > on a given mediatek platform. Sorry, I initially saw some point
> > > > in making
> > > > these timers as modules but if they are required for boot on
> > > > some systems
> > > > then I see no point. So if that is the case, NACK for these as
> > > > it just
> > > > creates more confusion after these are merged as why some
> > > > Images or
> > > > even why defconfig image(if we push the config change as well)
> > > > is not
> > > > booting on these platforms.
> > > > 
> > > > It is no longer just for system timer useful in low power CPU
> > > > idle states
> > > > as I initial thought.
> > > > 
> > > 
> > > I think that there is still a point in modularization for this
> > > driver and I
> > > can propose a rather simple solution, even though this may add
> > > some, rather
> > > little, code duplication to the mix.
> > > 
> > > The platforms that I've described (like mt6795) need the system
> > > timer to be
> > > initialized as early as possible - that's true - but that timer
> > > is always
> > > "CPUXGPT".
> > > 
> > > On those platforms, you *still* have multiple timers:
> > >   - CPUX (short for cpuxgpt), used only as system timer;
> > >   - SYST, as another system timer implementation (additional
> > > timers) but
> > >     those are always initialized (AFAIK) from the bootloader
> > > before booting;
> > >   - GPT (General Purpose Timer).
> > > 
> > > On one SoC, you may have:
> > >   - CPUX *and* SYST
> > >   - CPUX *and* GPT
> > >   - CPUX *and* SYST *and* GPT
> > > 
> > > ... where the only one that is boot critical and needs to be
> > > initialized early
> > > is always only CPUX.
> > > 
> > > Hence this proposal: to still allow modularization of timers on
> > > MediaTek platforms,
> > > we could eventually split the CPUX as a separated driver that
> > > *cannot be*, due to
> > > the previously explained constraints, compiled as module, hence
> > > always built-in,
> > > from a timer-mediatek driver that could be a module and capable
> > > of handling only
> > > SYST and GPT timers.
> > > 
> > > In that case, we'd hence have...
> > >   - timer-mediatek-cpux.o (bool)
> > >   - timer-mediatek.c (tristate)
> > > 
> > > Counting that the CPUX timers are actually even using different
> > > `tick_resume`
> > > and `set_state_shutdown` callbacks (doing only a IRQ
> > > clear/restore and nothing
> > > else), the amount of duplication would be .. well, again,
> > > minimal, but then
> > > this means that timer-mediatek-cpux would be `default y if
> > > ARCH_MEDIATEK`, or
> > > even selected by ARCH_MEDIATEK itself.
> > > 
> > > If you think that this could be a good solution, I can send a
> > > "fast" patch to
> > > split it out, preparing the ground for the people doing this
> > > module work.
> > > 
> > > Any considerations?
> > > 
> > 
> > I think your proposal sounds acceptable, but we would need to make
> > sure that all 
> > SoCs can boot with the CPUX driver. I'm aware of some armv7 SoCs
> > that use a kind of 
> > hack to enable the architecture timer [1]. This, for one part,
> > should be moved to 
> > CPUX, if possible. For the other part it makes me wonder if really
> > all supported 
> > MediaTek platforms will boot with SYST/GPT being a module. I think
> > we will need 
> > some effort from the community to test that.
> > 
> > So as a resume, yes I think your approach is feasible but we should
> > collect 
> > tested-by tags before merging it.
> > 
> > Regards,
> > Matthias
> > 
> > 
> > [1] 
> > 
https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/mach-mediatek/mediatek.c?h=v6.2-rc8*n16__;Iw!!CTRNKA9wMg0ARbw!jOX04TQn1HXcKOdzAiK3ZlqsSE3j3p6Zo-Cajr0khXC9ANlbkl8kQrUgx6wadR8b_46o9J0SbJgjhoS03rQ7somJfA5mnCQ6Fw$ 
> >  
> 
> Right. I completely forgot about those platforms, even though my
> intention was
> to actually try and migrate them once the CPUX was picked. My bad.
> 
> Well, I think that this module conversion will take quite a while, so
> there
> should be no need to rush... I'll send a series later with the split
> *and* a
> conversion of those platforms, so that we will see a removal of that
> mediatek_timer_init() function.
> 
> Some encouraging words ahead: I'm totally confident that the
> conversion will
> Just Work, because the MT6795 SoC had the same handling for CPUXGPT
> as the
> older MT6589/7623/8127/8135... as that SoC had two implementations
> initially,
> one as ARM, one as ARM64.
> 
> Whatever - let's see what I can come up with, then.
> 
> Cheers,
> Angelo

First, I'd like to thank Angelo for assisting us in spliting out the
CPUX driver [1], allowing this driver to become a module that can be
loaded when needed.

In response to Matthias's concern about whether SYST/GPT on the old
MediaTek platforms can boot properly with the loadable module, I
recently conducted a few tests and found that both hardwares are
capable of booting normally with this loadable module.

For my experiments, I used MT6768 with this patch:

1. When the module was not loaded or load the module but the
corresponding compatible property was not specified in the dts, the
following results were obtained:

# cat /proc/timer_list | grep "Broadcast device" -A 15
Broadcast device
Clock Event Device: bc_hrtimer
 max_delta_ns:   9223372036854775807
 min_delta_ns:   1
 mult:           1
 shift:          0
 mode:           3
 next_event:     490204000000 nsecs
 set_next_event: <0000000000000000>
 shutdown: bc_shutdown.cfi_jt
 event_handler:  tick_handle_oneshot_broadcast.cfi_jt
 retries:        0

tick_broadcast_mask: ff
tick_broadcast_oneshot_mask: 7e

The built-in `bc_hrtimer` was used as the broadcast device, whereby one
CPU was kept awake to wake up the other CPUs. As a result, one CPU
could not enter the idle state.

2. When the module was loaded and the GPT compatible property was
specified in the dts:

# cat /proc/timer_list | grep "Broadcast device" -A 17
Broadcast device
Clock Event Device: mtk-clkevt
 max_delta_ns:   330382104634
 min_delta_ns:   1000
 mult:           27917287
 shift:          31
 mode:           3
 next_event:     1483221016462 nsecs
 set_next_event: mtk_gpt_clkevt_next_event.cfi_jt
 shutdown: mtk_gpt_clkevt_shutdown.cfi_jt
 periodic: mtk_gpt_clkevt_set_periodic.cfi_jt
 oneshot:  mtk_gpt_clkevt_shutdown.cfi_jt
 resume:   mtk_gpt_clkevt_shutdown.cfi_jt
 event_handler:  tick_handle_oneshot_broadcast.cfi_jt
 retries:        17

tick_broadcast_mask: ff
tick_broadcast_oneshot_mask: bf

3. When the module was loaded and the SYST compatible property was
specified in the dts:

# cat /proc/timer_list | grep "Broadcast device" -A 17
Broadcast device
Clock Event Device: mtk-clkevt
 max_delta_ns:   330382104634
 min_delta_ns:   1000
 mult:           27917287
 shift:          31
 mode:           3
 next_event:     132252000000 nsecs
 set_next_event: mtk_syst_clkevt_next_event.cfi_jt
 shutdown: mtk_syst_clkevt_shutdown.cfi_jt
 oneshot:  mtk_syst_clkevt_oneshot.cfi_jt
 resume:   mtk_syst_clkevt_resume.cfi_jt
 event_handler:  tick_handle_oneshot_broadcast.cfi_jt
 retries:        8

tick_broadcast_mask: ff
tick_broadcast_oneshot_mask: 3f

These two experiments show that SYST/GPT on the old MediaTek platforms
can also work properly with the loadable module, and will not cause any
booting issues. Therefore, making timer-mediatek.c driver into a
loadable module is feasible.

Thanks,
Walter Chang

[1] 
https://lore.kernel.org/lkml/20230309103913.116775-1-angelogioacchino.delregno@collabora.com/

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

[Walter Chang (張維哲)]

On Wed, 2023-03-29 at 14:22 +0800, Walter.Chang wrote:
> First, I'd like to thank Angelo for assisting us in spliting out the
> CPUX driver [1], allowing this driver to become a module that can be
> loaded when needed.
> 
> In response to Matthias's concern about whether SYST/GPT on the old
> MediaTek platforms can boot properly with the loadable module, I
> recently conducted a few tests and found that both hardwares are
> capable of booting normally with this loadable module.
> 
> For my experiments, I used MT6768 with this patch:
> 
> 1. When the module was not loaded or load the module but the
> corresponding compatible property was not specified in the dts, the
> following results were obtained:
> 
> # cat /proc/timer_list | grep "Broadcast device" -A 15
> Broadcast device
> Clock Event Device: bc_hrtimer
>  max_delta_ns:   9223372036854775807
>  min_delta_ns:   1
>  mult:           1
>  shift:          0
>  mode:           3
>  next_event:     490204000000 nsecs
>  set_next_event: <0000000000000000>
>  shutdown: bc_shutdown.cfi_jt
>  event_handler:  tick_handle_oneshot_broadcast.cfi_jt
>  retries:        0
> 
> tick_broadcast_mask: ff
> tick_broadcast_oneshot_mask: 7e
> 
> The built-in `bc_hrtimer` was used as the broadcast device, whereby
> one
> CPU was kept awake to wake up the other CPUs. As a result, one CPU
> could not enter the idle state.
> 
> 2. When the module was loaded and the GPT compatible property was
> specified in the dts:
> 
> # cat /proc/timer_list | grep "Broadcast device" -A 17
> Broadcast device
> Clock Event Device: mtk-clkevt
>  max_delta_ns:   330382104634
>  min_delta_ns:   1000
>  mult:           27917287
>  shift:          31
>  mode:           3
>  next_event:     1483221016462 nsecs
>  set_next_event: mtk_gpt_clkevt_next_event.cfi_jt
>  shutdown: mtk_gpt_clkevt_shutdown.cfi_jt
>  periodic: mtk_gpt_clkevt_set_periodic.cfi_jt
>  oneshot:  mtk_gpt_clkevt_shutdown.cfi_jt
>  resume:   mtk_gpt_clkevt_shutdown.cfi_jt
>  event_handler:  tick_handle_oneshot_broadcast.cfi_jt
>  retries:        17
> 
> tick_broadcast_mask: ff
> tick_broadcast_oneshot_mask: bf
> 
> 3. When the module was loaded and the SYST compatible property was
> specified in the dts:
> 
> # cat /proc/timer_list | grep "Broadcast device" -A 17
> Broadcast device
> Clock Event Device: mtk-clkevt
>  max_delta_ns:   330382104634
>  min_delta_ns:   1000
>  mult:           27917287
>  shift:          31
>  mode:           3
>  next_event:     132252000000 nsecs
>  set_next_event: mtk_syst_clkevt_next_event.cfi_jt
>  shutdown: mtk_syst_clkevt_shutdown.cfi_jt
>  oneshot:  mtk_syst_clkevt_oneshot.cfi_jt
>  resume:   mtk_syst_clkevt_resume.cfi_jt
>  event_handler:  tick_handle_oneshot_broadcast.cfi_jt
>  retries:        8
> 
> tick_broadcast_mask: ff
> tick_broadcast_oneshot_mask: 3f
> 
> These two experiments show that SYST/GPT on the old MediaTek
> platforms
> can also work properly with the loadable module, and will not cause
> any
> booting issues. Therefore, making timer-mediatek.c driver into a
> loadable module is feasible.
> 
> Thanks,
> Walter Chang
> 
> [1] 
> 
https://lore.kernel.org/lkml/20230309103913.116775-1-angelogioacchino.delregno@collabora.com/

Gentle ping.

Thanks,
Walter Chang