On Tue, Dec 3, 2013 at 5:15 AM, Hanjun Guo hanjun.guo@linaro.org wrote:
ACPI GTDT (Generic Timer Description Table) contains information for arch timer initialization, this patch use this table to probe arm timer.
GTDT table is used for ARM/ARM64 only, please refer to chapter 5.2.24 of ACPI 5.0 spec for detailed inforamtion
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 | 129 ++++++++++++++++++++++++++++++---- include/clocksource/arm_arch_timer.h | 7 +- 2 files changed, 120 insertions(+), 16 deletions(-)
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c index 95fb944..c968041 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> @@ -632,20 +633,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@@ -667,8 +656,118 @@ 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;acpi_size tbl_size;int trigger, polarity;void __iomem *base = NULL;if (acpi_disabled)
Wouldn't the core ACPI code never call this function if ACPI is disabled?
return;if (arch_timers_present & ARCH_CP15_TIMER) {pr_warn("arch_timer: already initialized, skipping\n");return;}if (ACPI_FAILURE(acpi_get_table_with_size(ACPI_SIG_GTDT, 0,(struct acpi_table_header **)>dt, &tbl_size))) {pr_err("arch_timer: GTDT table not defined\n");return;}arch_timers_present |= ARCH_CP15_TIMER;
So you have marked the timer as initialized, but then may fail on error later on here.
/** 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);
This is for memory mapped timer? If so, then isn't setting ARCH_CP15_TIMER the wrong thing to do?
}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) {
Really, I think the kernel should just ignore the secure interrupt. The DT code has the same issue, but that doesn't affect the code size.
trigger = (gtdt->secure_pl1_flags & ACPI_GTDT_INTERRUPT_MODE) ?ACPI_EDGE_SENSITIVE : ACPI_LEVEL_SENSITIVE;
Why not use the already defined linux irq trigger types here and make acpi_register_gsi use them?
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);}early_acpi_os_unmap_memory(gtdt, tbl_size);
Who did the mapping? acpi_get_table_with_size? I think the core code should handle the mapping and unmapping of ACPI tables. We don't want to have to duplicate this in every initialization function. This seems error prone.
Rob