| Message ID | 1386088891-2917-2-git-send-email-hanjun.guo@linaro.org |
|---|---|
| State | New |
| Headers | show |
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 **)>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 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.
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 **)>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 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
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
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
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