* [PATCH v1] Porting Mediatek timer driver to kernel module @ 2020-07-28 10:16 Freddy Hsin 2020-07-28 10:16 ` [PATCH v1 1/2] kernel: time: export sched_clock_register function Freddy Hsin 2020-07-28 10:16 ` [PATCH v1 2/2] timer: mt6873: porting Mediatek timer driver to loadable module Freddy Hsin 0 siblings, 2 replies; 7+ messages in thread From: Freddy Hsin @ 2020-07-28 10:16 UTC (permalink / raw) To: linux-mediatek, linux-arm-kernel, Ahmed S. Darwish, Paul Cercueil, Ben Dooks (Codethink), Matthias Brugger, Thomas Gleixner, Daniel Lezcano Cc: Saravana Kannan, linux-kernel, chang-an.chen, Baolin Wang, wsd_upstream, kuohong.wang, stanley.chu 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 + ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v1 1/2] kernel: time: export sched_clock_register function 2020-07-28 10:16 [PATCH v1] Porting Mediatek timer driver to kernel module Freddy Hsin @ 2020-07-28 10:16 ` Freddy Hsin 2020-07-29 12:47 ` Thomas Gleixner 2020-07-29 14:09 ` Daniel Lezcano 2020-07-28 10:16 ` [PATCH v1 2/2] timer: mt6873: porting Mediatek timer driver to loadable module Freddy Hsin 1 sibling, 2 replies; 7+ messages in thread From: Freddy Hsin @ 2020-07-28 10:16 UTC (permalink / raw) To: linux-mediatek, linux-arm-kernel, Ahmed S. Darwish, Paul Cercueil, Ben Dooks (Codethink), Matthias Brugger, Thomas Gleixner, Daniel Lezcano Cc: Saravana Kannan, linux-kernel, chang-an.chen, Baolin Wang, wsd_upstream, kuohong.wang, stanley.chu, 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v1 1/2] kernel: time: export sched_clock_register function 2020-07-28 10:16 ` [PATCH v1 1/2] kernel: time: export sched_clock_register function Freddy Hsin @ 2020-07-29 12:47 ` Thomas Gleixner 2020-07-29 14:09 ` Daniel Lezcano 1 sibling, 0 replies; 7+ messages in thread From: Thomas Gleixner @ 2020-07-29 12:47 UTC (permalink / raw) To: Freddy Hsin, linux-mediatek, linux-arm-kernel, Ahmed S. Darwish, Paul Cercueil, Ben Dooks (Codethink), Matthias Brugger, Daniel Lezcano Cc: Saravana Kannan, linux-kernel, chang-an.chen, Baolin Wang, wsd_upstream, kuohong.wang, stanley.chu, Freddy Hsin 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v1 1/2] kernel: time: export sched_clock_register function 2020-07-28 10:16 ` [PATCH v1 1/2] kernel: time: export sched_clock_register function Freddy Hsin 2020-07-29 12:47 ` Thomas Gleixner @ 2020-07-29 14:09 ` Daniel Lezcano 1 sibling, 0 replies; 7+ messages in thread From: Daniel Lezcano @ 2020-07-29 14:09 UTC (permalink / raw) To: Freddy Hsin, linux-mediatek, linux-arm-kernel, Ahmed S. Darwish, Paul Cercueil, Ben Dooks (Codethink), Matthias Brugger, Thomas Gleixner Cc: Saravana Kannan, linux-kernel, chang-an.chen, Baolin Wang, wsd_upstream, kuohong.wang, stanley.chu 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v1 2/2] timer: mt6873: porting Mediatek timer driver to loadable module 2020-07-28 10:16 [PATCH v1] Porting Mediatek timer driver to kernel module Freddy Hsin 2020-07-28 10:16 ` [PATCH v1 1/2] kernel: time: export sched_clock_register function Freddy Hsin @ 2020-07-28 10:16 ` Freddy Hsin 2020-07-29 13:02 ` Thomas Gleixner 1 sibling, 1 reply; 7+ messages in thread From: Freddy Hsin @ 2020-07-28 10:16 UTC (permalink / raw) To: linux-mediatek, linux-arm-kernel, Ahmed S. Darwish, Paul Cercueil, Ben Dooks (Codethink), Matthias Brugger, Thomas Gleixner, Daniel Lezcano Cc: Saravana Kannan, linux-kernel, chang-an.chen, Baolin Wang, wsd_upstream, kuohong.wang, stanley.chu, 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v1 2/2] timer: mt6873: porting Mediatek timer driver to loadable module 2020-07-28 10:16 ` [PATCH v1 2/2] timer: mt6873: porting Mediatek timer driver to loadable module Freddy Hsin @ 2020-07-29 13:02 ` Thomas Gleixner 2020-07-29 18:42 ` Saravana Kannan 0 siblings, 1 reply; 7+ messages in thread From: Thomas Gleixner @ 2020-07-29 13:02 UTC (permalink / raw) To: Freddy Hsin, linux-mediatek, linux-arm-kernel, Ahmed S. Darwish, Paul Cercueil, Ben Dooks (Codethink), Matthias Brugger, Daniel Lezcano Cc: Saravana Kannan, linux-kernel, chang-an.chen, Baolin Wang, wsd_upstream, kuohong.wang, stanley.chu, Freddy Hsin 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v1 2/2] timer: mt6873: porting Mediatek timer driver to loadable module 2020-07-29 13:02 ` Thomas Gleixner @ 2020-07-29 18:42 ` Saravana Kannan 0 siblings, 0 replies; 7+ messages in thread From: Saravana Kannan @ 2020-07-29 18:42 UTC (permalink / raw) To: Thomas Gleixner Cc: Freddy Hsin, moderated list:ARM/Mediatek SoC support, linux-arm-kernel, Ahmed S. Darwish, Paul Cercueil, Ben Dooks (Codethink), Matthias Brugger, Daniel Lezcano, LKML, chang-an.chen, Baolin Wang, wsd_upstream, kuohong.wang, stanley.chu 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-07-29 18:43 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-07-28 10:16 [PATCH v1] Porting Mediatek timer driver to kernel module Freddy Hsin 2020-07-28 10:16 ` [PATCH v1 1/2] kernel: time: export sched_clock_register function Freddy Hsin 2020-07-29 12:47 ` Thomas Gleixner 2020-07-29 14:09 ` Daniel Lezcano 2020-07-28 10:16 ` [PATCH v1 2/2] timer: mt6873: porting Mediatek timer driver to loadable module Freddy Hsin 2020-07-29 13:02 ` Thomas Gleixner 2020-07-29 18:42 ` Saravana Kannan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).