Messages in this thread | | | Subject | Re: [PATCH] clocksource: Add driver for the Ingenic JZ47xx OST | From | Daniel Lezcano <> | Date | Tue, 10 Dec 2019 10:19:23 +0100 |
| |
On 07/11/2019 21:34, Paul Cercueil wrote: > > > Le jeu., nov. 7, 2019 at 21:22, Daniel Lezcano > <daniel.lezcano@linaro.org> a écrit : >> On 07/11/2019 20:57, Paul Cercueil wrote: >>> >>> >>> Le jeu., nov. 7, 2019 at 20:39, Daniel Lezcano >>> <daniel.lezcano@linaro.org> a écrit : >>>> On 07/11/2019 16:56, Paul Cercueil wrote: >>>>> Hi Daniel, >>>>> >>>>> On 2019-08-16 16:54, Daniel Lezcano wrote: >>>>>> On 09/08/2019 14:38, Paul Cercueil wrote: >>>>>>> From: Maarten ter Huurne <maarten@treewalker.org> >>>>>>> >>>>>>> OST is the OS Timer, a 64-bit timer/counter with buffered reading. >>>>>>> >>>>>>> SoCs before the JZ4770 had (if any) a 32-bit OST; the JZ4770 and >>>>>>> JZ4780 have a 64-bit OST. >>>>>>> >>>>>>> This driver will register both a clocksource and a sched_clock >>>>>>> to the >>>>>>> system. >>>>>>> >>>>>>> Signed-off-by: Maarten ter Huurne <maarten@treewalker.org> >>>>>>> Signed-off-by: Paul Cercueil <paul@crapouillou.net> >>>>>>> Tested-by: Mathieu Malaterre <malat@debian.org> >>>>>>> Tested-by: Artur Rojek <contact@artur-rojek.eu> >>>>>>> --- >>>>>>> >>>>>> >>>>>> [ ... ] >>>>>> >>>>>>> + err = clocksource_register_hz(cs, rate); >>>>>>> + if (err) { >>>>>>> + dev_err(dev, "clocksource registration failed: %d\n", >>>>>>> err); >>>>>>> + clk_disable_unprepare(ost->clk); >>>>>>> + return err; >>>>>>> + } >>>>>>> + >>>>>>> + /* Cannot register a sched_clock with interrupts on */ >>>>>> >>>>>> Aren't they already disabled? >>>>> >>>>> Sorry for the late reply. >>>>> >>>>> No, they are not already disabled; this is what I get if I >>>>> comment out >>>>> the local_irq_save()/local_irq_restore(): >>>>> >>>>> [ 0.361014] clocksource: ingenic-ost: mask: 0xffffffff >>>>> max_cycles: >>>>> 0xffffffff, max_idle_ns: 159271703898 ns >>>>> [ 0.361515] clocksource: Switched to clocksource ingenic-ost >>>>> [ 0.361686] ------------[ cut here ]------------ >>>>> [ 0.361893] WARNING: CPU: 0 PID: 1 at >>>>> kernel/time/sched_clock.c:179 >>>>> sched_clock_register+0x7c/0x2e4 >>>>> [ 0.362174] CPU: 0 PID: 1 Comm: swapper Not tainted 5.4.0-rc5+ >>>>> #461 >>>>> [ 0.362330] Stack : 80744558 80069b44 80770000 00000000 00000000 >>>>> 00dfd7a7 806e6db4 8106bb74 >>>>> [ 0.362619] 806f0000 81067ca4 806f31c7 80769478 00000020 >>>>> 10000400 8106bb20 00dfd7a7 >>>>> [ 0.362906] 00000000 00000000 80780000 00000000 00000007 >>>>> 00000001 00000049 3563722d >>>>> [ 0.363191] 8106ba61 00000000 ffffffff 00000010 806f0000 >>>>> 00000000 00000000 806f0000 >>>>> [ 0.363477] 00000020 00000000 80714534 80770000 00000002 >>>>> 80319154 00000000 80770000 >>>>> [ 0.363762] ... >>>>> [ 0.363906] Call Trace: >>>>> [ 0.364087] [<8001af14>] show_stack+0x40/0x128 >>>>> [ 0.364289] [<8002fd88>] __warn+0xb8/0xe0 >>>>> [ 0.364478] [<8002fe14>] warn_slowpath_fmt+0x64/0xc0 >>>>> [ 0.364678] [<8072b1c8>] sched_clock_register+0x7c/0x2e4 >>>>> [ 0.364895] [<8073c874>] ingenic_ost_probe+0x224/0x248 >>>>> [ 0.365090] [<803d5394>] platform_drv_probe+0x40/0x94 >>>>> [ 0.365526] [<803d362c>] really_probe+0x104/0x374 >>>>> [ 0.365743] [<803d3ff0>] device_driver_attach+0x78/0x80 >>>>> [ 0.365938] [<803d4070>] __driver_attach+0x78/0x118 >>>>> [ 0.366129] [<803d1700>] bus_for_each_dev+0x7c/0xc8 >>>>> [ 0.366318] [<803d226c>] bus_add_driver+0x1bc/0x204 >>>>> [ 0.366513] [<803d4878>] driver_register+0x84/0x14c >>>>> [ 0.366717] [<8073a144>] __platform_driver_probe+0x98/0x140 >>>>> [ 0.366931] [<80724e38>] do_one_initcall+0x84/0x1b4 >>>>> [ 0.367126] [<807250cc>] kernel_init_freeable+0x164/0x240 >>>>> [ 0.367318] [<805df75c>] kernel_init+0x10/0xf8 >>>>> [ 0.367510] [<8001542c>] ret_from_kernel_thread+0x14/0x1c >>>>> [ 0.367722] ---[ end trace 7fedf00408fa3bed ]--- >>>>> [ 0.367985] sched_clock: 32 bits at 12MHz, resolution 83ns, wraps >>>>> every 178956970966ns >>>>> >>>>> At kernel/time/sched_clock.c:179 there is: >>>>> WARN_ON(!irqs_disabled()); >>>> >>>> That is strange, no drivers is doing that and no warning is appearing. >>>> >>>> Isn't missing a local_irq_disable in the code path in the stack above? >>> >>> I think it comes to the fact that the other drivers are probed much >>> earlier in the boot process, while this one is probed as a regular >>> platform device driver. >> >> >> There are other drivers doing and nobody is disabling the interrupt: >> >> em_sti.c:static struct platform_driver em_sti_device_driver = { >> ingenic-timer.c:static struct platform_driver ingenic_tcu_driver = { >> sh_cmt.c:static struct platform_driver sh_cmt_device_driver = { >> sh_mtu2.c:static struct platform_driver sh_mtu2_device_driver = { >> sh_tmu.c:static struct platform_driver sh_tmu_device_driver = { >> timer-ti-dm.c:static struct platform_driver omap_dm_timer_driver = { > > Yes, but they don't register a sched_clock at all (except for > ingenic-timer, but it does probe using TIMER_OF_DECLARE), and this > warning is in sched_clock_register(). > >> >>>>>>> + local_irq_save(flags); >>>>>>> + if (soc_info->is64bit) >>>>>>> + sched_clock_register(ingenic_ost_read_cntl, 32, rate); >>>>>>> + else >>>>>>> + sched_clock_register(ingenic_ost_read_cnth, 32, rate); >>>>>>> + local_irq_restore(flags); >>>>>>> + >>>>>>> + return 0; >>>>>>> +} >>>>>>> +
Thomas,
the local_irq_save/restore calls shouldn't be in the sched_clock_register() function instead of showing a warning?
-- <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
| |