| Message ID | 1386069328-22502-3-git-send-email-hanjun.guo@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Tue, Dec 3, 2013 at 12:15 PM, Hanjun Guo <hanjun.guo@linaro.org> wrote: > + /* if can't be initialised from DT, try ACPI way */ > + if (!arch_timer_get_rate()) > + arch_timer_acpi_init(); > + > arch_timer_rate = arch_timer_get_rate(); This looks a bit fragile. Having a call like arch_timer_get_rate() to check whether there is a DT node for the timer doesn't seem right, can you refactor the code to provide some has_arch_timer_node() or similar call instead, so it's a bit easier to understand & maintain at least? Yours, Linus Walleij
On 2013年12月03日 20:27, Linus Walleij wrote: > On Tue, Dec 3, 2013 at 12:15 PM, Hanjun Guo <hanjun.guo@linaro.org> wrote: > >> + /* if can't be initialised from DT, try ACPI way */ >> + if (!arch_timer_get_rate()) >> + arch_timer_acpi_init(); >> + >> arch_timer_rate = arch_timer_get_rate(); > This looks a bit fragile. Having a call like arch_timer_get_rate() > to check whether there is a DT node for the timer doesn't seem > right, can you refactor the code to provide some > has_arch_timer_node() or similar call instead, so it's a bit easier > to understand & maintain at least? Good point, thanks for the guidance. I will introduce has_arch_timer_node() as you said and use it as follows: if (has_arch_timer_node()) clocksource_of_init(); esle arch_timer_acpi_init(); /* try ACPI way */ Is this make sense to you? Thanks Hanjun
On Tue, Dec 3, 2013 at 2:52 PM, Hanjun Guo <hanjun.guo@linaro.org> wrote: > I will introduce has_arch_timer_node() as you said and use > it as follows: > > if (has_arch_timer_node()) > clocksource_of_init(); > esle > arch_timer_acpi_init(); /* try ACPI way */ > > Is this make sense to you? Sure, go head. Thanks, Linus Walleij
On Tue, Dec 03, 2013 at 02:13:49PM +0000, Linus Walleij wrote: > On Tue, Dec 3, 2013 at 2:52 PM, Hanjun Guo <hanjun.guo@linaro.org> wrote: > > > I will introduce has_arch_timer_node() as you said and use > > it as follows: > > > > if (has_arch_timer_node()) > > clocksource_of_init(); > > esle > > arch_timer_acpi_init(); /* try ACPI way */ > > > > Is this make sense to you? What does arch_timer_acpi_init() do? Is it just detecting the presence of the timer, or grabbing the rate from a property in an ACPI table? Mark.
On 2013年12月03日 22:43, Mark Rutland wrote: > On Tue, Dec 03, 2013 at 02:13:49PM +0000, Linus Walleij wrote: >> On Tue, Dec 3, 2013 at 2:52 PM, Hanjun Guo <hanjun.guo@linaro.org> wrote: >> >>> I will introduce has_arch_timer_node() as you said and use >>> it as follows: >>> >>> if (has_arch_timer_node()) >>> clocksource_of_init(); >>> esle >>> arch_timer_acpi_init(); /* try ACPI way */ >>> >>> Is this make sense to you? > What does arch_timer_acpi_init() do? Is it just detecting the presence > of the timer, or grabbing the rate from a property in an ACPI table? It seems that you didn't get the PATCH 1/2, and my part1/part2 patch set is also missing, I will resend all the patch set, sorry for the noise. Thanks Hanjun
On Tue, Dec 03, 2013 at 09:52:30PM +0800, Hanjun Guo wrote: > On 2013年12月03日 20:27, Linus Walleij wrote: > >On Tue, Dec 3, 2013 at 12:15 PM, Hanjun Guo <hanjun.guo@linaro.org> wrote: > > > >>+ /* if can't be initialised from DT, try ACPI way */ > >>+ if (!arch_timer_get_rate()) > >>+ arch_timer_acpi_init(); > >>+ > >> arch_timer_rate = arch_timer_get_rate(); > >This looks a bit fragile. Having a call like arch_timer_get_rate() > >to check whether there is a DT node for the timer doesn't seem > >right, can you refactor the code to provide some > >has_arch_timer_node() or similar call instead, so it's a bit easier > >to understand & maintain at least? > > Good point, thanks for the guidance. > I will introduce has_arch_timer_node() as you said and use > it as follows: > > if (has_arch_timer_node()) > clocksource_of_init(); > esle > arch_timer_acpi_init(); /* try ACPI way */ > > Is this make sense to you? Even when we boot with ACPI, the boot stub will still create a minimal DTB. We should just make sure that the clocksource (which will be architectured timers anyway, I believe?) is described in that stub. I would rather do that than have dual-path booting in the lowlevel setup, it increases test requirements and makes it hard for someone without ACPI hardware to check for regressions in this code, etc. -Olof
diff --git a/arch/arm64/kernel/time.c b/arch/arm64/kernel/time.c index 29c39d5..fb009da 100644 --- a/arch/arm64/kernel/time.c +++ b/arch/arm64/kernel/time.c @@ -67,6 +67,10 @@ void __init time_init(void) clocksource_of_init(); + /* if can't be initialised from DT, try ACPI way */ + if (!arch_timer_get_rate()) + arch_timer_acpi_init(); + arch_timer_rate = arch_timer_get_rate(); if (!arch_timer_rate) panic("Unable to initialise architected timer.\n");
Use arch_timer_acpi_init() on ARM64 to initialise arch timer in ACPI way when DT is not available. Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> --- arch/arm64/kernel/time.c | 4 ++++ 1 file changed, 4 insertions(+)