On Tue, Mar 24, 2020 at 11:34 AM Saravana Kannan <saravanak@xxxxxxxxxx> wrote: > > On Tue, Mar 24, 2020 at 11:00 AM Ionela Voinescu > <ionela.voinescu@xxxxxxx> wrote: > > > > Hi guys, > > > > On Thursday 19 Mar 2020 at 08:47:46 (-0000), tip-bot2 for Saravana Kannan wrote: > > > The following commit has been merged into the timers/core branch of tip: > > > > > > Commit-ID: 4f41fe386a94639cd9a1831298d4f85db5662f1e > > > Gitweb: https://git.kernel.org/tip/4f41fe386a94639cd9a1831298d4f85db5662f1e > > > Author: Saravana Kannan <saravanak@xxxxxxxxxx> > > > AuthorDate: Fri, 10 Jan 2020 21:21:25 -08:00 > > > Committer: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx> > > > CommitterDate: Tue, 17 Mar 2020 13:10:07 +01:00 > > > > > > clocksource/drivers/timer-probe: Avoid creating dead devices > > > > > > Timer initialization is done during early boot way before the driver > > > core starts processing devices and drivers. Timers initialized during > > > this early boot period don't really need or use a struct device. > > > > > > However, for timers represented as device tree nodes, the struct devices > > > are still created and sit around unused and wasting memory. This change > > > avoid this by marking the device tree nodes as "populated" if the > > > corresponding timer is successfully initialized. > > > > > > Signed-off-by: Saravana Kannan <saravanak@xxxxxxxxxx> > > > Signed-off-by: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx> > > > Link: https://lore.kernel.org/r/20200111052125.238212-1-saravanak@xxxxxxxxxx > > > --- > > > drivers/clocksource/timer-probe.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/drivers/clocksource/timer-probe.c b/drivers/clocksource/timer-probe.c > > > index ee9574d..a10f28d 100644 > > > --- a/drivers/clocksource/timer-probe.c > > > +++ b/drivers/clocksource/timer-probe.c > > > @@ -27,8 +27,10 @@ void __init timer_probe(void) > > > > > > init_func_ret = match->data; > > > > > > + of_node_set_flag(np, OF_POPULATED); > > > ret = init_func_ret(np); > > > if (ret) { > > > + of_node_clear_flag(np, OF_POPULATED); > > > if (ret != -EPROBE_DEFER) > > > pr_err("Failed to initialize '%pOF': %d\n", np, > > > ret); > > > > > > > This patch is creating problems on some vexpress platforms - ones that > > are using CLKSRC_VERSATILE (drivers/clocksource/timer-versatile.c). > > I noticed issues on TC2 and FVPs (fixed virtual platforms) starting with > > next-20200318 and still reproducible with next-20200323. > > > > It seems the issue this patch causes on TC2 and FVP is related to the > > vexpress-sysreg node being used early for sched_clock_init > > (timer_versatile.c: versatile_sched_clock_init). At this point (at > > time_init) the node will be marked as OF_POPULATED, which flags that a > > device is already created for it, but it is not, in this case. > > > > Later at sysreg_init (vexpress-sysreg.c) a device will fail to be created > > for it, as one already exists. This will result in a failure to create a > > bridge and a system controller for a bunch of devices (mostly clocks and > > regulators). > > > > I think on the FVP it does not cause many issues as clocks are fixed and > > regulator settings are probably nops so it boots fine and throws only > > some warnings. On TC2 on the other hand it fails to boot and it hangs at > > starting the kernel. > > > > In my opinion the idea of the patch is not bad, but I'm not an expert on > > this so the most I can offer for now is the basic understanding of the > > issue. I've Cc-ed a few folks to potentially suggest alternatives/fixes. > > > > For now, reverting this patch solves the problems on both platforms. > > I tested this on next-20200318 which introduced the problem. > > I'll reply later today after I take a closer look. But will something > like what timer-ingenic.c did work for you? You can clear the flag > inside your early init. Firstly, sorry my patch broke your platform. I took a closer look. So two different drivers [1] [2] are saying they know how to handle "arm,vexpress-sysreg" and are expecting to run at the same time. That seems a bit unusual to me. I wonder if this is a violation of the device-driver model because this expectation would never be allowed if these device drivers were actual drivers registered with driver-core. But that's a discussion for another time. To fix this issue you are facing, this patch should work: https://lore.kernel.org/lkml/20200324195302.203115-1-saravanak@xxxxxxxxxx/T/#u Can you please test it and give a Tested-by? Thanks, Saravana [1] drivers/mfd/vexpress-sysreg.c: { .compatible = "arm,vexpress-sysreg", }, [2] drivers/clocksource/timer-versatile.c:TIMER_OF_DECLARE(vexpress, "arm,vexpress-sysreg"
![]() |