On 11/15/2013 03:04 AM, Hanjun Guo wrote:

GTDT (Generic Timer Description Table) contains information for
arch timer initialization, this patch use this table to probe
arm timer.

[Hanjun: modify the patch to use GTDT table presented in ACPI 5.0,
and modify the change log]

Signed-off-by: Amit Daniel Kachhap <amit.daniel@samsung.com>
Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
---
  drivers/clocksource/arm_arch_timer.c |  122 +++++++++++++++++++++++++++++-----
  include/clocksource/arm_arch_timer.h |    7 +-
  2 files changed, 113 insertions(+), 16 deletions(-)

diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index fbd9ccd..714d7f9 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -19,6 +19,7 @@
  #include <linux/of_address.h>
  #include <linux/io.h>
  #include <linux/slab.h>
+#include <linux/acpi.h>

  #include <asm/arch_timer.h>
  #include <asm/virt.h>
@@ -579,20 +580,8 @@ static void __init arch_timer_common_init(void)
        arch_timer_arch_init();
  }

-static void __init arch_timer_init(struct device_node *np)
+static void __init arch_timer_init(void)
  {
-       int i;
-
-       if (arch_timers_present & ARCH_CP15_TIMER) {
-               pr_warn("arch_timer: multiple nodes in dt, skipping\n");
-               return;
-       }
-
-       arch_timers_present |= ARCH_CP15_TIMER;
-       for (i = PHYS_SECURE_PPI; i < MAX_TIMER_PPI; i++)
-               arch_timer_ppi[i] = irq_of_parse_and_map(np, i);
-       arch_timer_detect_rate(NULL, np);
-
        /*
         * If HYP mode is available, we know that the physical timer
         * has been configured to be accessible from PL1. Use it, so
@@ -614,8 +603,111 @@ static void __init arch_timer_init(struct device_node *np)
        arch_timer_register();
        arch_timer_common_init();
  }
-CLOCKSOURCE_OF_DECLARE(armv7_arch_timer, "arm,armv7-timer", arch_timer_init);
-CLOCKSOURCE_OF_DECLARE(armv8_arch_timer, "arm,armv8-timer", arch_timer_init);
+
+static void __init arch_timer_of_init(struct device_node *np)
+{
+       int i;
+
+       if (arch_timers_present & ARCH_CP15_TIMER) {
+               pr_warn("arch_timer: multiple nodes in dt, skipping\n");
+               return;
+       }
+
+       arch_timers_present |= ARCH_CP15_TIMER;
+       for (i = PHYS_SECURE_PPI; i < MAX_TIMER_PPI; i++)
+               arch_timer_ppi[i] = irq_of_parse_and_map(np, i);
+       arch_timer_detect_rate(NULL, np);
+
+       arch_timer_init();
+}
+CLOCKSOURCE_OF_DECLARE(armv7_arch_timer, "arm,armv7-timer", arch_timer_of_init);
+CLOCKSOURCE_OF_DECLARE(armv8_arch_timer, "arm,armv8-timer", arch_timer_of_init);
+
+#ifdef CONFIG_ACPI
+void __init arch_timer_acpi_init(void)
+{
+       struct acpi_table_gtdt *gtdt;
+       int trigger, polarity;
+       void __iomem *base = NULL;
+
+       if (arch_timers_present & ARCH_CP15_TIMER) {
+               pr_warn("arch_timer: already initialized, skipping\n");
+               return;
+       }
+
+       if (ACPI_FAILURE(acpi_get_table(ACPI_SIG_GTDT, 0,
+                               (struct acpi_table_header **)&gtdt))) {
+               pr_err("arch_timer: GTDT table not defined\n");
+               return;
+       }
+
+       arch_timers_present |= ARCH_CP15_TIMER;
+
+       /*
+        * Get the timer frequency. Since there is no frequency info
+        * in the GTDT table, so we should read it from CNTFREG register
+        * or hard code here to wait for the new ACPI spec available.
+        */
+       if (!gtdt->address) {
+               arch_timer_rate = arch_timer_get_cntfrq();
+       } else {
+               base = ioremap(gtdt->address, CNTFRQ);
+               if (!base) {
+                       pr_warn("arch_timer: unable to map arch timer base address\n");
+                       return;
+               }
+
+               arch_timer_rate = readl_relaxed(base + CNTFRQ);
+               iounmap(base);
+       }
+
+       if (!arch_timer_rate) {
+               /* Hard code here to set frequence ? */
+               pr_warn("arch_timer: Could not get frequency from GTDT table or CNTFREG\n");
+       }
+
+       if (gtdt->secure_pl1_interrupt) {
+               trigger = (gtdt->secure_pl1_flags & ACPI_GTDT_INTERRUPT_MODE) ?
+                       ACPI_EDGE_SENSITIVE : ACPI_LEVEL_SENSITIVE;
+               polarity =
+                       (gtdt->secure_pl1_flags & ACPI_GTDT_INTERRUPT_POLARITY)
+                       ? ACPI_ACTIVE_LOW : ACPI_ACTIVE_HIGH;
+               arch_timer_ppi[0] = acpi_register_gsi(NULL,
+                               gtdt->secure_pl1_interrupt, trigger, polarity);
+       }
+       if (gtdt->non_secure_pl1_interrupt) {
+               trigger =
+                       (gtdt->non_secure_pl1_flags & ACPI_GTDT_INTERRUPT_MODE)
+                       ? ACPI_EDGE_SENSITIVE : ACPI_LEVEL_SENSITIVE;
+               polarity =
+               (gtdt->non_secure_pl1_flags & ACPI_GTDT_INTERRUPT_POLARITY)
+                       ? ACPI_ACTIVE_LOW : ACPI_ACTIVE_HIGH;
+               arch_timer_ppi[1] = acpi_register_gsi(NULL,
+                       gtdt->non_secure_pl1_interrupt, trigger, polarity);
+       }
+       if (gtdt->virtual_timer_interrupt) {
+               trigger = (gtdt->virtual_timer_flags & ACPI_GTDT_INTERRUPT_MODE)
+                       ? ACPI_EDGE_SENSITIVE : ACPI_LEVEL_SENSITIVE;
+               polarity =
+               (gtdt->virtual_timer_flags & ACPI_GTDT_INTERRUPT_POLARITY)
+                       ? ACPI_ACTIVE_LOW : ACPI_ACTIVE_HIGH;
+               arch_timer_ppi[2] = acpi_register_gsi(NULL,
+                       gtdt->virtual_timer_interrupt, trigger, polarity);
+       }
+       if (gtdt->non_secure_pl2_interrupt) {
+               trigger =
+                       (gtdt->non_secure_pl2_flags & ACPI_GTDT_INTERRUPT_MODE)
+                       ? ACPI_EDGE_SENSITIVE : ACPI_LEVEL_SENSITIVE;
+               polarity =
+               (gtdt->non_secure_pl2_flags & ACPI_GTDT_INTERRUPT_POLARITY)
+                       ? ACPI_ACTIVE_LOW : ACPI_ACTIVE_HIGH;
+               arch_timer_ppi[3] = acpi_register_gsi(NULL,
+                       gtdt->non_secure_pl2_interrupt, trigger, polarity);
+       }
+
+       arch_timer_init();
+}
+#endif

  static void __init arch_timer_mem_init(struct device_node *np)
  {
diff --git a/include/clocksource/arm_arch_timer.h b/include/clocksource/arm_arch_timer.h
index 93b7f96..eeb9ce3 100644
--- a/include/clocksource/arm_arch_timer.h
+++ b/include/clocksource/arm_arch_timer.h
@@ -38,7 +38,7 @@ enum arch_timer_reg {
  extern u32 arch_timer_get_rate(void);
  extern u64 (*arch_timer_read_counter)(void);
  extern struct timecounter *arch_timer_get_timecounter(void);
-
+extern void __init arch_timer_acpi_init(void);
  #else

  static inline u32 arch_timer_get_rate(void)
@@ -56,6 +56,11 @@ static inline struct timecounter *arch_timer_get_timecounter(void)
        return NULL;
  }

+static inline void arch_timer_acpi_init(void)
+{
+       return;
+}
+
  #endif

  #endif

I have no objection to this patch but I am curious: why is it that
the ACPI code seems so much larger?  I understand there's some more
code needed to replace arch_timer_detect_rate() and it looks like
the rest is to replace irq_of_parse_and_map(); do I have that right?

If that's the case, would it be possible (or even make any sense)
to create an irq_parse_and_map() that reads either DT or ACPI in
this case?  What I'm thinking, I guess, is that what I'd really
like to do is minimize the differences between arch_timer_of_init()
and arch_timer_acpi_init() somehow such that the only real change
in the driver is in how the config info is retrieved (from DT or
ACPI) and everything above that stays the same.

Does that make sense?  Telling me that I am hopelessly deluded
is also one of the possible responses to this question :).