Hi Lorenzo,

On 2015?02?05? 02:59, Lorenzo Pieralisi wrote:

On Mon, Feb 02, 2015 at 12:45:45PM +0000, Hanjun Guo wrote:

quoted

Using the information presented by GTDT (Generic Timer Description Table)
to initialize the arch timer (not memory-mapped).

Why are you not initializing the memory mapped timer ?

We left it for later work because no need for that to boot
available ARM64 hardware at now, and we have no hardware to
test unless I missed some of platforms.

quoted

CC: Daniel Lezcano <redacted>
Originally-by: Amit Daniel Kachhap [off-list ref]
Tested-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
Tested-by: Yijing Wang <redacted>
Tested-by: Mark Langsdorf <redacted>
Tested-by: Jon Masters <redacted>
Tested-by: Timur Tabi <redacted>
Signed-off-by: Hanjun Guo <redacted>
---
  arch/arm64/kernel/time.c             |   7 ++
  drivers/clocksource/arm_arch_timer.c | 132 ++++++++++++++++++++++++++++-------
  include/linux/clocksource.h          |   6 ++
  3 files changed, 118 insertions(+), 27 deletions(-)

diff --git a/arch/arm64/kernel/time.c b/arch/arm64/kernel/time.c
index 1a7125c..42f9195 100644
--- a/arch/arm64/kernel/time.c
+++ b/arch/arm64/kernel/time.c

@@ -35,6 +35,7 @@
  #include <linux/delay.h>
  #include <linux/clocksource.h>
  #include <linux/clk-provider.h>
+#include <linux/acpi.h>

  #include <clocksource/arm_arch_timer.h>

@@ -72,6 +73,12 @@ void __init time_init(void)

  	tick_setup_hrtimer_broadcast();

+	/*
+	 * Since ACPI or FDT will only one be available in the system,
+	 * we can use acpi_generic_timer_init() here safely
+	 */
+	acpi_generic_timer_init();
+
  	arch_timer_rate = arch_timer_get_rate();
  	if (!arch_timer_rate)
  		panic("Unable to initialise architected timer.\n");

diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 095c177..407aa63 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c

@@ -21,6 +21,7 @@
  #include <linux/io.h>
  #include <linux/slab.h>
  #include <linux/sched_clock.h>
+#include <linux/acpi.h>

  #include <asm/arch_timer.h>
  #include <asm/virt.h>

@@ -370,8 +371,12 @@ arch_timer_detect_rate(void __iomem *cntbase, struct device_node *np)
  	if (arch_timer_rate)
  		return;

-	/* Try to determine the frequency from the device tree or CNTFRQ */
-	if (of_property_read_u32(np, "clock-frequency", &arch_timer_rate)) {
+	/*
+	 * Try to determine the frequency from the device tree or CNTFRQ,
+	 * if ACPI is enabled, get the frequency from CNTFRQ ONLY.
+	 */
+	if (!acpi_disabled ||
+	    of_property_read_u32(np, "clock-frequency", &arch_timer_rate)) {

This is getting a mess. cntbase tells you it is a memory mapped timer,
node pointer that you are probing through DT, and to top it all
acpi_disabled detects if you are probing in ACPI or DT mode.

I think this function should be simplified, this driver is also
pending a refactoring to split arch timer and the memory mapped one
so I think you'd better wait that work to make things simpler.

Does anyone working on this now?

[...]

quoted

+/* Initialize all the generic timers presented in GTDT */
+void __init acpi_generic_timer_init(void)
+{
+	if (acpi_disabled)
+		return;

acpi_disabled used again here, I repeat myself this is going to be
hard to track. You should try to organize the code something like:

if (acpi_disabled)
	timer_dt_probe();
else
	timer_acpi_probe();

mixing the code paths is getting unwieldy, see above to see my
reasoning.

Olof is unhappy with such approach, I think if (acpi_disabled) is
self-contained because we only get DT or ACPI in the system, we
can call this function time_init() without more if (acpi_disabled).

quoted

+
+	acpi_table_parse(ACPI_SIG_GTDT, arch_timer_acpi_init);
+}
+#endif

diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
index abcafaa..af6155a 100644
--- a/include/linux/clocksource.h
+++ b/include/linux/clocksource.h

@@ -346,4 +346,10 @@ extern void clocksource_of_init(void);
  static inline void clocksource_of_init(void) {}
  #endif

+#ifdef CONFIG_ACPI
+void acpi_generic_timer_init(void);
+#else
+static inline void acpi_generic_timer_init(void) { }
+#endif
+

That's not nice, it is a generic header, arch specific stuff should be
avoided. I think you should have an ACPI generic layer similar to
clocksource_of_init(), and probe from there when matching the respective
timers.

I think I'm OK with it, but do we really need to introduce a heavy
framework to init just arm arch timer (memory mapped or not) ?

Thanks
Hanjun