[RFC part3 PATCH 0/2] Using ACPI GTDT table to initialize arch timer

[RFC part3 PATCH 0/2] Using ACPI GTDT table to initialize arch timer

[Hanjun Guo]

This is the last part of patch set for core of ARM64 ACPI, and is based
on the patch set part2 "Using ACPI MADT table to initialise SMP and GIC".

ACPI GTDT (Generic Timer Description Table) is used for ARM/ARM64 only,
and contains the information for arch timer initialisation.

This patch trys to convert the arch timer to ACPI using GTDT. 

After this patch set was posted, we already finished the SMP, GIC and
arch timer initialisation, which all are essential for ARM64 core system
running, then we will focus on converting the device drivers to ACPI.

Here is the GTDT ASL code I used:
---
 platforms/foundation-v8.acpi/gtdt.asl |   35 ++++++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 12 deletions(-)

diff --git a/platforms/foundation-v8.acpi/gtdt.asl b/platforms/foundation-v8.acpi/gtdt.asl
index 18c821a..714d61c 100644
--- a/platforms/foundation-v8.acpi/gtdt.asl
+++ b/platforms/foundation-v8.acpi/gtdt.asl
@@ -1,5 +1,6 @@
 /*
  * Copyright (c) 2013, Al Stone <al.stone@linaro.org>
+ *                     Hanjun Guo <hanjun.guo@linaro.org>
  * 
  * [GTDT] Generic Timer Description Table
  * Format: [ByteLength]  FieldName : HexFieldValue
@@ -21,22 +22,32 @@
 [0004]              Flags (decoded below) : 00000001
                            Memory Present : 1
 
-[0004]               Secure PL1 Interrupt : 00000000
-[0004]         SPL1 Flags (decoded below) : 00000000
-                             Trigger Mode : 0
+/* In Foundation model's dts file, the last cell of interrupts
+ * is 0xff01, it means its cpu mask is FF, and trigger type
+ * and flag is 1 = low-to-high edge triggered.
+ * 
+ * so in ACPI the Trigger Mode is 1 - Edge triggered, and 
+ * Polarity is 0 - Active high as ACPI spec describled.
+ *
+ * using direct mapping for hwirqs, it means that we using
+ * ID [16, 31] for PPI, not [0, 15] used in FDT.
+ */
+[0004]               Secure PL1 Interrupt : 0000001d
+[0004]         SPL1 Flags (decoded below) : 00000001
+                             Trigger Mode : 1
                                  Polarity : 0
 
-[0004]           Non-Secure PL1 Interrupt : 00000000
-[0004]        NSPL1 Flags (decoded below) : 00000000
-                             Trigger Mode : 0
+[0004]           Non-Secure PL1 Interrupt : 0000001e
+[0004]        NSPL1 Flags (decoded below) : 00000001
+                             Trigger Mode : 1
                                  Polarity : 0
 
-[0004]            Virtual Timer Interrupt : 00000000
-[0004]           VT Flags (decoded below) : 00000000
-                             Trigger Mode : 0
+[0004]            Virtual Timer Interrupt : 0000001b
+[0004]           VT Flags (decoded below) : 00000001
+                             Trigger Mode : 1
                                  Polarity : 0
 
-[0004]           Non-Secure PL2 Interrupt : 00000000
-[0004]        NSPL2 Flags (decoded below) : 00000000
-                             Trigger Mode : 0
+[0004]           Non-Secure PL2 Interrupt : 0000001a
+[0004]        NSPL2 Flags (decoded below) : 00000001
+                             Trigger Mode : 1
                                  Polarity : 0

Hanjun Guo (2):
  clocksource / arch_timer: Use ACPI GTDT table to initialize arch
    timer
  ARM64 / clocksource: Use arch_timer_acpi_init()

 arch/arm64/kernel/time.c             |    4 ++
 drivers/clocksource/arm_arch_timer.c |  129 ++++++++++++++++++++++++++++++----
 include/clocksource/arm_arch_timer.h |    7 +-
 3 files changed, 124 insertions(+), 16 deletions(-)

-- 
1.7.9.5

[RFC part3 PATCH 1/2] clocksource / arch_timer: Use ACPI GTDT table to initialize arch timer

[Hanjun Guo]

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)
+		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 **)&gtdt, &tbl_size))) {
+		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);
+	}
+
+	early_acpi_os_unmap_memory(gtdt, tbl_size);
+	arch_timer_init();
+}
+#else
+void __init arch_timer_acpi_init(void) { return; };
+#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 6d26b40..2654edf 100644
--- a/include/clocksource/arm_arch_timer.h
+++ b/include/clocksource/arm_arch_timer.h
@@ -48,7 +48,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)
@@ -66,6 +66,11 @@ static inline struct timecounter *arch_timer_get_timecounter(void)
 	return NULL;
 }
 
+static inline void arch_timer_acpi_init(void)
+{
+	return;
+}
+
 #endif
 
 #endif
-- 
1.7.9.5

[RFC part3 PATCH 1/2] clocksource / arch_timer: Use ACPI GTDT table to initialize arch timer

[Mark Rutland]

On Tue, Dec 03, 2013 at 04:41:30PM +0000, Hanjun Guo 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(-)
 
[...]

> +#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)
> +		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 **)&gtdt, &tbl_size))) {
> +		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 the core's CNTFRQ register does not hold the correct value, that is a
horrendous firmware bug. The clock-frequency property in the DT is a
horrific workaround for buggy firmware.

We should not duplicate it in ACPI; people should be strongly encouraged
to fix their firmware to do what it is supposed to do.

Rely on CNTFRQ only. If it is wrong, then bail out. Let's not create a
fertile environment for buggy firmware.

> +	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);
> +	}

This pattern looks like it can be factored out. I don't see why the
driver needs to have such intimate knowledge of the interrrupt.

> +	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);
> +	}

Please factor out the interrupt parsing.

Thanks,
Mark.

[RFC part3 PATCH 1/2] clocksource / arch_timer: Use ACPI GTDT table to initialize arch timer

[Hanjun Guo]

On 2013?12?04? 01:04, Mark Rutland wrote:
> On Tue, Dec 03, 2013 at 04:41:30PM +0000, Hanjun Guo 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(-)
>   
> [...]
>
>> +#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)
>> +		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 **)&gtdt, &tbl_size))) {
>> +		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 the core's CNTFRQ register does not hold the correct value, that is a
> horrendous firmware bug. The clock-frequency property in the DT is a
> horrific workaround for buggy firmware.
>
> We should not duplicate it in ACPI; people should be strongly encouraged
> to fix their firmware to do what it is supposed to do.
>
> Rely on CNTFRQ only. If it is wrong, then bail out. Let's not create a
> fertile environment for buggy firmware.

Great, this can make things much simple, and the information which contains
in ACPI GTDT table can be sufficient for timer initialization.


>> +	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);
>> +	}
> This pattern looks like it can be factored out. I don't see why the
> driver needs to have such intimate knowledge of the interrrupt.
>
>> +	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);
>> +	}
> Please factor out the interrupt parsing.

Ok, will try to factor it out in next version.

Thanks for the comments.

Hanjun

[RFC part3 PATCH 1/2] clocksource / arch_timer: Use ACPI GTDT table to initialize arch timer

[Arnd Bergmann]

On Tuesday 03 December 2013, Hanjun Guo wrote:

> +#ifdef CONFIG_ACPI
> +void __init arch_timer_acpi_init(void)
> +{
...
> +}
> +#else
> +void __init arch_timer_acpi_init(void) { return; };
> +#endif
>  

The #else clause is broken in combination with 

> diff --git a/include/clocksource/arm_arch_timer.h b/include/clocksource/arm_arch_timer.h
> index 6d26b40..2654edf 100644
> --- a/include/clocksource/arm_arch_timer.h
> +++ b/include/clocksource/arm_arch_timer.h
> @@ -66,6 +66,11 @@ static inline struct timecounter *arch_timer_get_timecounter(void)
>  	return NULL;
>  }
>  
> +static inline void arch_timer_acpi_init(void)
> +{
> +	return;
> +}
> +
>  #endif
>  

this inline function. Have you build-tested this with CONFIG_ACPI disabled?

	Arnd

[RFC part3 PATCH 1/2] clocksource / arch_timer: Use ACPI GTDT table to initialize arch timer

[Hanjun Guo]

On 2013?12?05? 11:43, Arnd Bergmann wrote:
> On Tuesday 03 December 2013, Hanjun Guo wrote:
>
>> +#ifdef CONFIG_ACPI
>> +void __init arch_timer_acpi_init(void)
>> +{
> ...
>> +}
>> +#else
>> +void __init arch_timer_acpi_init(void) { return; };
>> +#endif
>>   
> The #else clause is broken in combination with
>
>> diff --git a/include/clocksource/arm_arch_timer.h b/include/clocksource/arm_arch_timer.h
>> index 6d26b40..2654edf 100644
>> --- a/include/clocksource/arm_arch_timer.h
>> +++ b/include/clocksource/arm_arch_timer.h
>> @@ -66,6 +66,11 @@ static inline struct timecounter *arch_timer_get_timecounter(void)
>>   	return NULL;
>>   }
>>   
>> +static inline void arch_timer_acpi_init(void)
>> +{
>> +	return;
>> +}
>> +
>>   #endif
>>   
> this inline function. Have you build-tested this with CONFIG_ACPI disabled?

Good catch! Thank you very much, will fix it in next version.

Thanks
hanjun

[RFC part3 PATCH 2/2] ARM64 / clocksource: Use arch_timer_acpi_init()

[Hanjun Guo]

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(+)

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");
-- 
1.7.9.5

[RFC part3 PATCH 2/2] ARM64 / clocksource: Use arch_timer_acpi_init()

[Mark Rutland]

On Tue, Dec 03, 2013 at 04:41:31PM +0000, Hanjun Guo wrote:
> 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(+)
> 
> 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();
> +

As mentioned on the previous patch, I think for the timebeing we should
rely on CNTFREQ.

Additionally, if you need to do this we should have an analagous
mechanism to clocksource_of_init() that performs this initialisation for
ACPI, and here we can call a clocksource_*_init function that does the
right thing.

There's no need for this file to know anything about ACPI.

Mark.

[RFC part3 PATCH 2/2] ARM64 / clocksource: Use arch_timer_acpi_init()

[Hanjun Guo]

On 2013?12?04? 01:08, Mark Rutland wrote:
> On Tue, Dec 03, 2013 at 04:41:31PM +0000, Hanjun Guo wrote:
>> 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(+)
>>
>> 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();
>> +
> As mentioned on the previous patch, I think for the timebeing we should
> rely on CNTFREQ.
>
> Additionally, if you need to do this we should have an analagous
> mechanism to clocksource_of_init() that performs this initialisation for
> ACPI, and here we can call a clocksource_*_init function that does the
> right thing.
>
> There's no need for this file to know anything about ACPI.

Oh, Amit already have some patches to introduce clocksource_acpi_init()
like clocksource_of_init() did, please refer to the link below.

http://marc.info/?l=linaro-acpi&m=138131929721943&w=2

is this the idea you mentioned in your comments?

Thanks
Hanjun

[RFC part3 PATCH 2/2] ARM64 / clocksource: Use arch_timer_acpi_init()

[Mark Rutland]

On Wed, Dec 04, 2013 at 02:27:22PM +0000, Hanjun Guo wrote:
> On 2013?12?04? 01:08, Mark Rutland wrote:
> > On Tue, Dec 03, 2013 at 04:41:31PM +0000, Hanjun Guo wrote:
> >> 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(+)
> >>
> >> 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();
> >> +
> > As mentioned on the previous patch, I think for the timebeing we should
> > rely on CNTFREQ.
> >
> > Additionally, if you need to do this we should have an analagous
> > mechanism to clocksource_of_init() that performs this initialisation for
> > ACPI, and here we can call a clocksource_*_init function that does the
> > right thing.
> >
> > There's no need for this file to know anything about ACPI.
> 
> Oh, Amit already have some patches to introduce clocksource_acpi_init()
> like clocksource_of_init() did, please refer to the link below.
> 
> http://marc.info/?l=linaro-acpi&m=138131929721943&w=2
> 
> is this the idea you mentioned in your comments?

Something along those lines is far better than hardcoding
arch_timer_acpi_init here.

Thanks,
Mark.

[RFC part3 PATCH 2/2] ARM64 / clocksource: Use arch_timer_acpi_init()

[Hanjun Guo]

On 2013?12?04? 23:07, Mark Rutland wrote:
> On Wed, Dec 04, 2013 at 02:27:22PM +0000, Hanjun Guo wrote:
>> On 2013?12?04? 01:08, Mark Rutland wrote:
>>> On Tue, Dec 03, 2013 at 04:41:31PM +0000, Hanjun Guo wrote:
>>>> 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(+)
>>>>
>>>> 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();
>>>> +
>>> As mentioned on the previous patch, I think for the timebeing we should
>>> rely on CNTFREQ.
>>>
>>> Additionally, if you need to do this we should have an analagous
>>> mechanism to clocksource_of_init() that performs this initialisation for
>>> ACPI, and here we can call a clocksource_*_init function that does the
>>> right thing.
>>>
>>> There's no need for this file to know anything about ACPI.
>> Oh, Amit already have some patches to introduce clocksource_acpi_init()
>> like clocksource_of_init() did, please refer to the link below.
>>
>> http://marc.info/?l=linaro-acpi&m=138131929721943&w=2
>>
>> is this the idea you mentioned in your comments?
> Something along those lines is far better than hardcoding
> arch_timer_acpi_init here.

Thanks for the suggestion, will update it.

Hanjun

[RFC part3 PATCH 2/2] ARM64 / clocksource: Use arch_timer_acpi_init()

[Linus Walleij]

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

[RFC part3 PATCH 2/2] ARM64 / clocksource: Use arch_timer_acpi_init()

[Hanjun Guo]

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

[RFC part3 PATCH 2/2] ARM64 / clocksource: Use arch_timer_acpi_init()

[Linus Walleij]

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

[RFC part3 PATCH 2/2] ARM64 / clocksource: Use arch_timer_acpi_init()

[Mark Rutland]

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.

[RFC part3 PATCH 2/2] ARM64 / clocksource: Use arch_timer_acpi_init()

[Hanjun Guo]

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

[RFC part3 PATCH 2/2] ARM64 / clocksource: Use arch_timer_acpi_init()

[Olof Johansson]

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