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

[Freddy.Hsin <freddy.hsin@mediatek.com>]

On Wed, 2020-07-29 at 11:42 -0700, Saravana Kannan wrote:
> 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

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel