On 2020/11/13 16:53, Stephen Boyd wrote: > Quoting Damien Le Moal (2020-11-12 23:40:17) >> On 2020/11/13 16:31, Stephen Boyd wrote: >>> Quoting Damien Le Moal (2020-11-07 00:13:56) >>>> If of_clk_init() is not called in time_init(), clock providers defined >>>> in the system device tree are not initialized, resulting in failures for >>>> other devices to initialize due to missing clocks. >>>> Similarly to other architectures and to the default kernel time_init() >>>> implementation, call of_clk_init() before executing timer_probe() in >>>> time_init(). >>> >>> Do you have timers that need clks to be running or queryable this early? >>> This of_clk_init() call is made here when architectures need to call >>> things like clk_get_rate() to figure out some clk frequency for their >>> clockevent or clocksource. It is OK to have this call here, I'm just >>> curious if this is actually necessary vs. delaying it to later. >>> >> >> I think the clocks could be initialized later, but at least the CLINT will >> depend on one of the clocks, same for the CPU frequency information. So need >> checking. >> >> What this patch fixes is not the need for a super early initialization though, >> it is that _nothing_ was being initialized without it: the clock driver probe >> function was never called with the current riscv time_init() as is. I looked at >> other architectures and at the default kernel time_init(), and mimicked what was >> done, that is, added of_clk_init(). Is there any other way to make sure that the >> needed clock drivers are initialized ? >> > > Yes it's fine. Just the commit text reads as "If of_clk_init() is not > called in time_init() then nothing works" which is true but made me > wonder if it was because it needed to be super early or not. The commit > text could be a little clearer here. OK. I will clarify the commit message in V2. Working on it now. > We don't have any good solution for a fallback to call of_clk_init() > somewhere later. I do wonder if we should generalize this though and > call of_clk_init() from start_kernel() directly via some Kconfig that > architectures select if they need it for their timer and then move it to > an initcall if architectures don't select the config. Or throw it into > the of_platform_default_populate_init() hook if the architecture doesn't > need to call it early. This last idea seems reasonable and probably the easiest. And I think it could be done unconditionally even if the arch calls of_clk_init() early as the already populated clock provider nodes would not be initialized again. -- Damien Le Moal Western Digital Research
