[PATCH v1 2/2] timer: mt6873: porting Mediatek timer driver to loadable module
Freddy.Hsin
freddy.hsin at mediatek.com
Mon Sep 28 21:59:48 EDT 2020
On Thu, 2020-08-27 at 17:22 +0800, Freddy.Hsin wrote:
> 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 S
> W
> [ 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 W
> 5.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