[PATCH v1] Porting Mediatek timer driver to kernel module

[PATCH v1] Porting Mediatek timer driver to kernel module

[Freddy Hsin]

Porting Mediatek timer driver to kernel module for GKI
(Generic Kernel Image). Clocksource mmio functions are
exported for Mediatek timer init function.

This patch depends on the timer-of patch submitted by Baolin Wang
https://lkml.org/lkml/2020/3/24/72


Freddy Hsin (2):
  kernel: time: remove init section label of sched_clock_register
  timer: mt6873: porting timer driver to loadable module

 drivers/clocksource/Kconfig          |    2 +-
 drivers/clocksource/mmio.c           |    4 +++-
 drivers/clocksource/timer-mediatek.c |   39 ++++++++++++++++++++++++++++++++++
 kernel/time/sched_clock.c            |    1 +

[PATCH v1 1/2] kernel: time: export sched_clock_register function

[Freddy Hsin]

export sched_clock_register function, because the Mediatek timer
loadable module depends on this function

Signed-off-by: Freddy Hsin <freddy.hsin@mediatek.com>
---
 kernel/time/sched_clock.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/time/sched_clock.c b/kernel/time/sched_clock.c
index fa3f800..e1d5afe 100644
--- a/kernel/time/sched_clock.c
+++ b/kernel/time/sched_clock.c
@@ -239,6 +239,7 @@ static enum hrtimer_restart sched_clock_poll(struct hrtimer *hrt)
 
 	pr_debug("Registered %pS as sched_clock source\n", read);
 }
+EXPORT_SYMBOL(sched_clock_register);
 
 void __init generic_sched_clock_init(void)
 {
-- 
1.7.9.5

[PATCH v1 2/2] timer: mt6873: porting Mediatek timer driver to loadable module

[Freddy Hsin]

porting Mediatek timer driver to loadable module

Signed-off-by: Freddy Hsin <freddy.hsin@mediatek.com>
---
 drivers/clocksource/Kconfig          |    2 +-
 drivers/clocksource/mmio.c           |    4 +++-
 drivers/clocksource/timer-mediatek.c |   39 ++++++++++++++++++++++++++++++++++
 3 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index 9141838..538351a 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/mmio.c b/drivers/clocksource/mmio.c
index 9de7515..5504569 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(clocksource_mmio_readl_up);
 
 u64 clocksource_mmio_readl_down(struct clocksource *c)
 {
@@ -46,7 +47,7 @@ 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,
+int clocksource_mmio_init(void __iomem *base, const char *name,
 	unsigned long hz, int rating, unsigned bits,
 	u64 (*read)(struct clocksource *))
 {
@@ -68,3 +69,4 @@ int __init clocksource_mmio_init(void __iomem *base, const char *name,
 
 	return clocksource_register_hz(&cs->clksrc, hz);
 }
+EXPORT_SYMBOL(clocksource_mmio_init);
diff --git a/drivers/clocksource/timer-mediatek.c b/drivers/clocksource/timer-mediatek.c
index 9318edc..5c89b6b 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"
@@ -309,5 +312,41 @@ 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,
+	},
+	{}
+};
+
+static struct platform_driver mtk_timer_driver = {
+	.probe = mtk_timer_probe,
+	.driver = {
+		.name = "mtk-timer",
+		.of_match_table = mtk_timer_match_table,
+	},
+};
+MODULE_DESCRIPTION("MEDIATEK Module timer driver");
+MODULE_LICENSE("GPL v2");
+
+module_platform_driver(mtk_timer_driver);
+#else
 TIMER_OF_DECLARE(mtk_mt6577, "mediatek,mt6577-timer", mtk_gpt_init);
 TIMER_OF_DECLARE(mtk_mt6765, "mediatek,mt6765-timer", mtk_syst_init);
+#endif
-- 
1.7.9.5

Re: [PATCH v1 1/2] kernel: time: export sched_clock_register function

[Thomas Gleixner]

Freddy,

Freddy Hsin <freddy.hsin@mediatek.com> writes:

please do not come up with random prefixes. Just look at the topmost two
commits on that file:

2c8bd58812ee ("time/sched_clock: Expire timer in hardirq context")
2707745533d6 ("time/sched_clock: Disable interrupts in sched_clock_register()")

Also the sentence after the colon starts with an uppercase letter.

Please change 'sched_clock_register function' to
'sched_clock_register()' which makes it clear that this is a function

> export sched_clock_register function, because the Mediatek timer
> loadable module depends on this function

Again sentences start with an uppercase letter. Also the reason for
exporting this is wrong. There is no mediatek timer module now.

The point of exporting this (if at all) is that it's required for any
ARM SoC timer driver to be modularized.
> @@ -239,6 +239,7 @@ static enum hrtimer_restart sched_clock_poll(struct hrtimer *hrt)
>  
>  	pr_debug("Registered %pS as sched_clock source\n", read);
>  }
> +EXPORT_SYMBOL(sched_clock_register);

EXPORT_SYMBOL_GPL() please.

Thanks,

        tglx

Re: [PATCH v1 2/2] timer: mt6873: porting Mediatek timer driver to loadable module

[Thomas Gleixner]

Freddy,

Freddy Hsin <freddy.hsin@mediatek.com> writes:

again, please be more careful with subject lines. git log $FILE will
give you a hint. 

> porting Mediatek timer driver to loadable module

Repeating the sentence in the subject line is not giving any
information. Also changelogs want to tell the WHY and not the WHAT. This
also lacks any information why this is actually safe when booting such a
system w/o this particular driver built in. What is early boot - up to
module load - using as clocksource and timer?

> diff --git a/drivers/clocksource/mmio.c b/drivers/clocksource/mmio.c
> index 9de7515..5504569 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(clocksource_mmio_readl_up);

Again EXPORT_SYMBOL_GPL() and this wants to be a seperate patch. It has
absolutely no business with the mediatek timer changes. 
  
>  u64 clocksource_mmio_readl_down(struct clocksource *c)
>  {
> @@ -46,7 +47,7 @@ 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,
> +int clocksource_mmio_init(void __iomem *base, const char *name,
>
>  	unsigned long hz, int rating, unsigned bits,
>  	u64 (*read)(struct clocksource *))
>  {
> @@ -68,3 +69,4 @@ int __init clocksource_mmio_init(void __iomem *base, const char *name,
>  
>  	return clocksource_register_hz(&cs->clksrc, hz);
>  }
> +EXPORT_SYMBOL(clocksource_mmio_init);

See above.

> diff --git a/drivers/clocksource/timer-mediatek.c b/drivers/clocksource/timer-mediatek.c
> index 9318edc..5c89b6b 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"
> @@ -309,5 +312,41 @@ 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,
> +	},
> +	{}
> +};
> +
> +static struct platform_driver mtk_timer_driver = {
> +	.probe = mtk_timer_probe,
> +	.driver = {
> +		.name = "mtk-timer",
> +		.of_match_table = mtk_timer_match_table,
> +	},
> +};
> +MODULE_DESCRIPTION("MEDIATEK Module timer driver");
> +MODULE_LICENSE("GPL v2");
> +
> +module_platform_driver(mtk_timer_driver);
> +#else
>  TIMER_OF_DECLARE(mtk_mt6577, "mediatek,mt6577-timer", mtk_gpt_init);
>  TIMER_OF_DECLARE(mtk_mt6765, "mediatek,mt6765-timer", mtk_syst_init);
> +#endif

Sorry no. This is not going to happen.

The above probe, match table and platform driver structs plus the module*
thingies are going to be repeated in every single driver which is going
to support module build. Tons of boilerplate copied over and over
again.

We had exactly the same before TIMER_OF_DECLARE() came around, so pretty
please this want's to be some smart macro which handles all of this
automatically.

Thanks,

        tglx

Re: [PATCH v1 1/2] kernel: time: export sched_clock_register function

[Daniel Lezcano]

Hi Freddy,

On 28/07/2020 12:16, Freddy Hsin wrote:
> export sched_clock_register function, because the Mediatek timer
> loadable module depends on this function

That is not a sufficient reason to ask for exporting a symbol from the
core framework.

It is the second patch asking for exporting the symbols because of GKI.

If we want to do this move, it is important to understand what that will
imply by doing the changes as a whole:

 - Identify the routines which will need EXPORT_SYMBOL_GPL

 - Show it is safe to use them with what that implies as module. eg.
What about cpuidle drivers using the broadcast timers?

 - As pointed by Thomas, TIMER_OF_DECLARE must be adapted to handle the
module cases

The conversion to all modules is not as straightforward as just
exporting a symbol and the changes must be provided with the proof the
impact has been studied and anticipated.


> Signed-off-by: Freddy Hsin <freddy.hsin@mediatek.com>
> ---
>  kernel/time/sched_clock.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/kernel/time/sched_clock.c b/kernel/time/sched_clock.c
> index fa3f800..e1d5afe 100644
> --- a/kernel/time/sched_clock.c
> +++ b/kernel/time/sched_clock.c
> @@ -239,6 +239,7 @@ static enum hrtimer_restart sched_clock_poll(struct hrtimer *hrt)
>  
>  	pr_debug("Registered %pS as sched_clock source\n", read);
>  }
> +EXPORT_SYMBOL(sched_clock_register);



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

Re: [PATCH v1 2/2] timer: mt6873: porting Mediatek timer driver to loadable module

[Saravana Kannan]

On Wed, Jul 29, 2020 at 6:02 AM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> Freddy,
>
> Freddy Hsin <freddy.hsin@mediatek.com> writes:
>
> again, please be more careful with subject lines. git log $FILE will
> give you a hint.
>
> > porting Mediatek timer driver to loadable module
>
> Repeating the sentence in the subject line is not giving any
> information. Also changelogs want to tell the WHY and not the WHAT. This
> also lacks any information why this is actually safe when booting such a
> system w/o this particular driver built in. What is early boot - up to
> module load - using as clocksource and timer?
>
> > diff --git a/drivers/clocksource/mmio.c b/drivers/clocksource/mmio.c
> > index 9de7515..5504569 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(clocksource_mmio_readl_up);
>
> Again EXPORT_SYMBOL_GPL() and this wants to be a seperate patch. It has
> absolutely no business with the mediatek timer changes.
>
> >  u64 clocksource_mmio_readl_down(struct clocksource *c)
> >  {
> > @@ -46,7 +47,7 @@ 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,
> > +int clocksource_mmio_init(void __iomem *base, const char *name,
> >
> >       unsigned long hz, int rating, unsigned bits,
> >       u64 (*read)(struct clocksource *))
> >  {
> > @@ -68,3 +69,4 @@ int __init clocksource_mmio_init(void __iomem *base, const char *name,
> >
> >       return clocksource_register_hz(&cs->clksrc, hz);
> >  }
> > +EXPORT_SYMBOL(clocksource_mmio_init);
>
> See above.
>
> > diff --git a/drivers/clocksource/timer-mediatek.c b/drivers/clocksource/timer-mediatek.c
> > index 9318edc..5c89b6b 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"
> > @@ -309,5 +312,41 @@ 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,
> > +     },
> > +     {}
> > +};
> > +
> > +static struct platform_driver mtk_timer_driver = {
> > +     .probe = mtk_timer_probe,
> > +     .driver = {
> > +             .name = "mtk-timer",
> > +             .of_match_table = mtk_timer_match_table,
> > +     },
> > +};
> > +MODULE_DESCRIPTION("MEDIATEK Module timer driver");
> > +MODULE_LICENSE("GPL v2");
> > +
> > +module_platform_driver(mtk_timer_driver);
> > +#else
> >  TIMER_OF_DECLARE(mtk_mt6577, "mediatek,mt6577-timer", mtk_gpt_init);
> >  TIMER_OF_DECLARE(mtk_mt6765, "mediatek,mt6765-timer", mtk_syst_init);
> > +#endif
>
> Sorry no. This is not going to happen.
>
> The above probe, match table and platform driver structs plus the module*
> thingies are going to be repeated in every single driver which is going
> to support module build. Tons of boilerplate copied over and over
> again.
>
> We had exactly the same before TIMER_OF_DECLARE() came around, so pretty
> please this want's to be some smart macro which handles all of this
> automatically.

Probably something like what I did for IRQCHIP?
https://lore.kernel.org/lkml/20200718000637.3632841-1-saravanak@google.com/

Also, one point that came up with IRQCHIP is that if these drivers can
be platform drivers, then they should stay that way even when it's
built in. I'm not sure if that has any other special implications for
timer code, but raising it in case you'd prefer that here too.


-Saravana