Re: [PATCH 08/32] riscv: Fix kernel time_init()

[Damien Le Moal <Damien.LeMoal@wdc.com>]

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