| Message ID | 20201107081420.60325-9-damien.lemoal@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | RISC-V Kendryte K210 support improvments | expand |
On Sat, Nov 7, 2020 at 12:15 AM Damien Le Moal <damien.lemoal@wdc.com> wrote: > > 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(). > > Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> > --- > arch/riscv/kernel/time.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/arch/riscv/kernel/time.c b/arch/riscv/kernel/time.c > index 4d3a1048ad8b..8a5cf99c0776 100644 > --- a/arch/riscv/kernel/time.c > +++ b/arch/riscv/kernel/time.c > @@ -4,6 +4,7 @@ > * Copyright (C) 2017 SiFive > */ > > +#include <linux/of_clk.h> > #include <linux/clocksource.h> > #include <linux/delay.h> > #include <asm/sbi.h> > @@ -24,6 +25,8 @@ void __init time_init(void) > riscv_timebase = prop; > > lpj_fine = riscv_timebase / HZ; > + > + of_clk_init(NULL); > timer_probe(); > } > > -- > 2.28.0 > > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv Reviewed-by: Atish Patra <atish.patra@wdc.com>
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.
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 ?
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. 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.
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.
Quoting Damien Le Moal (2020-11-12 23:57:19) > On 2020/11/13 16:53, Stephen Boyd wrote: > > > > 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. Thanks! > > > 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. > Also of_clk_init() is for the CLK_OF_DECLARE() users and if they can wait to platform bus population time then they could be converted to regular old platform device drivers. Maybe the problem is the clk driver you're using is only using CLK_OF_DECLARE() and not registering a platform driver?
On 2020/11/13 17:11, Stephen Boyd wrote: > Quoting Damien Le Moal (2020-11-12 23:57:19) >> On 2020/11/13 16:53, Stephen Boyd wrote: >>> >>> 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. > > Thanks! > >> >>> 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. >> > > Also of_clk_init() is for the CLK_OF_DECLARE() users and if they can > wait to platform bus population time then they could be converted to > regular old platform device drivers. Maybe the problem is the clk driver > you're using is only using CLK_OF_DECLARE() and not registering a > platform driver? Yep, correct, that is what I did. SO yes, indeed, if I where to use a regular platform_driver, then the of_clk_init() change would not be needed. For the clock driver, I followed the pattern used by most other drivers with the majority I think using CLK_OF_DECLARE(). I did see some using the platform driver declaration though. I could spend time trying to figure out if I can get away without using CLK_OF_DECLARE(), but since I think other riscv board clock driver are arriving, we may as well keep that of_clk_init() change, no ?
Quoting Damien Le Moal (2020-11-13 00:23:52) > On 2020/11/13 17:11, Stephen Boyd wrote: > > > > Also of_clk_init() is for the CLK_OF_DECLARE() users and if they can > > wait to platform bus population time then they could be converted to > > regular old platform device drivers. Maybe the problem is the clk driver > > you're using is only using CLK_OF_DECLARE() and not registering a > > platform driver? > > Yep, correct, that is what I did. SO yes, indeed, if I where to use a regular > platform_driver, then the of_clk_init() change would not be needed. > > For the clock driver, I followed the pattern used by most other drivers with the > majority I think using CLK_OF_DECLARE(). I did see some using the platform > driver declaration though. > > I could spend time trying to figure out if I can get away without using > CLK_OF_DECLARE(), but since I think other riscv board clock driver are arriving, > we may as well keep that of_clk_init() change, no ? > Sure keeping of_clk_init() in riscv architecture code is fine. It would be good to use a platform driver though and avoid the use of CLK_OF_DECLARE() so we don't burden ourselves with more code that registers clks early unnecessarily.
On 2020/11/16 16:06, Stephen Boyd wrote: > Quoting Damien Le Moal (2020-11-13 00:23:52) >> On 2020/11/13 17:11, Stephen Boyd wrote: >>> >>> Also of_clk_init() is for the CLK_OF_DECLARE() users and if they can >>> wait to platform bus population time then they could be converted to >>> regular old platform device drivers. Maybe the problem is the clk driver >>> you're using is only using CLK_OF_DECLARE() and not registering a >>> platform driver? >> >> Yep, correct, that is what I did. SO yes, indeed, if I where to use a regular >> platform_driver, then the of_clk_init() change would not be needed. >> >> For the clock driver, I followed the pattern used by most other drivers with the >> majority I think using CLK_OF_DECLARE(). I did see some using the platform >> driver declaration though. >> >> I could spend time trying to figure out if I can get away without using >> CLK_OF_DECLARE(), but since I think other riscv board clock driver are arriving, >> we may as well keep that of_clk_init() change, no ? >> > > Sure keeping of_clk_init() in riscv architecture code is fine. It would > be good to use a platform driver though and avoid the use of > CLK_OF_DECLARE() so we don't burden ourselves with more code that > registers clks early unnecessarily. OK. Please let me check if that works. It may not be possible to delay initialization as the CLINT/sched clocksource depends on the clocks being ready.
diff --git a/arch/riscv/kernel/time.c b/arch/riscv/kernel/time.c index 4d3a1048ad8b..8a5cf99c0776 100644 --- a/arch/riscv/kernel/time.c +++ b/arch/riscv/kernel/time.c @@ -4,6 +4,7 @@ * Copyright (C) 2017 SiFive */ +#include <linux/of_clk.h> #include <linux/clocksource.h> #include <linux/delay.h> #include <asm/sbi.h> @@ -24,6 +25,8 @@ void __init time_init(void) riscv_timebase = prop; lpj_fine = riscv_timebase / HZ; + + of_clk_init(NULL); timer_probe(); }
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(). Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> --- arch/riscv/kernel/time.c | 3 +++ 1 file changed, 3 insertions(+)