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

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

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