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(-)
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 **)>dt, &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
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
On 2013年12月04日 23:33, Rob Herring wrote:
On Tue, Dec 3, 2013 at 5:15 AM, Hanjun Guo hanjun.guo@linaro.org wrote:
[...]
+#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?
You inspired me for patches to remove some redundant if (acpi_disabled) check for the current ACPI code, but this function will be called even 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?
I'm trying to do that but it is wrong as you said, I will remove above code and only keep
arch_timer_rate = arch_timer_get_cntfrq() here.
}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.
Yes, you are right, I will use the ACPI core function acpi_table_parse() to fix it, thanks for you guidance.
Hanjun
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");
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
linaro-kernel@lists.linaro.org
-
Hanjun Guo -
Linus Walleij -
Mark Rutland -
Olof Johansson -
Rob Herring