[PATCH v1 2/2] timer: mt6873: porting Mediatek timer driver to loadable module
Freddy.Hsin
freddy.hsin at mediatek.com
Mon Oct 5 02:12:58 EDT 2020
On Wed, 2020-07-29 at 11:42 -0700, Saravana Kannan wrote:
> On Wed, Jul 29, 2020 at 6:02 AM Thomas Gleixner <tglx at linutronix.de> wrote:
> >
> > Freddy,
> >
> > Freddy Hsin <freddy.hsin at 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
Hi Sravana,
Thanks for your advice. I already tried to add a common macro for
timer ko drivers as you did for irqchip, but there are some issues
need to be solved. e.g. the event handler cannot be changed to
tick_handle_oneshot_broadcast and rcu warning caused by some cores
are already in idle loop.
cat /proc/timer_list
==========================
Tick Device: mode: 1
Broadcast device
Clock Event Device: mtk-clkevt
max_delta_ns: 330382104634
min_delta_ns: 1000
mult: 27917287
shift: 31
mode: 3
next_event: 86709384386 nsecs
set_next_event: mtk_syst_clkevt_next_event
shutdown: mtk_syst_clkevt_shutdown
oneshot: mtk_syst_clkevt_oneshot
resume: mtk_syst_clkevt_resume
event_handler: tick_handle_periodic_broadcast
retries: 4
log of loading timer-mediatek.ko
============================
[ 25.658884] init 7: Loaded kernel
module /lib/modules/timer-mediatek.ko
[ 25.658946] =============================
[ 25.660010] init 7: Loading module /lib/modules/reboot-mode.ko with
args ""
[ 25.660364] WARNING: suspicious RCU usage
[ 25.661831] 5.4.46-g325aef33bcb1-dirty #17 Tainted: G SW
[ 25.662707] -----------------------------
[ 25.663254] include/linux/rcupdate.h:601 rcu_read_lock() used
illegally while idle!
[ 25.664279]
[ 25.664279] other info that might help us debug this:
[ 25.665350] RCU used illegally from idle CPU!
[ 25.665350] rcu_scheduler_active = 2, debug_locks = 1
[ 25.666796] RCU used illegally from extended quiescent state!
[ 25.667571] 2 locks held by swapper/1/0:
[ 25.668105] #0: ffffffec240b9a58 (tick_broadcast_lock){-.-.},
at:__tick_broadcast_oneshot_control+0x6c/0x444
[ 25.669460] #1: ffffffec2410e020 (rcu_read_lock){....}, at:
rcu_lock_acquire+0x0/0x40
[ 25.670536] stack backtrace:
[ 25.671131] CPU: 1 PID: 0 Comm: swapper/1 Tainted: G S
W5.4.46-g325aef33bcb1-dirty #17
[ 25.672350] Hardware name: mediatek,mt6873 (DT)
[ 25.672964] Call trace:
[ 25.673310] dump_backtrace.cfi_jt+0x0/0x4
[ 25.673872] dump_stack+0xf4/0x178
[ 25.674344] lockdep_rcu_suspicious+0x138/0x154
[ 25.674961] __cfi_slowpath+0x7c/0x1ec
[ 25.675475] clockevents_program_event+0x288/0x290
[ 25.676126] __tick_broadcast_oneshot_control+0x2f8/0x444
[ 25.676856] tick_broadcast_oneshot_control+0x58/0x68
[ 25.677544] cpuidle_enter_state+0x4c/0x314
[ 25.678115] cpuidle_enter+0x38/0x50
[ 25.678608] do_idle.llvm.7895800599653321206+0x1e0/0x2e8
[ 25.679338] cpu_startup_entry+0x24/0x28
BRs,
FreddyHsin
More information about the linux-arm-kernel
mailing list