Re: [RFC part3 PATCH 1/2] clocksource / arch_timer: Use ACPI GTDT table to initialize arch timer
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC part3 PATCH 1/2] clocksource / arch_timer: Use ACPI GTDT table to initialize arch timer
On 2013年12月04日 23:33, Rob Herring wrote:
On Tue, Dec 3, 2013 at 5:15 AM, Hanjun Guo 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 **), _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
--
To unsubscribe from this list: send the
Re: [RFC part3 PATCH 1/2] clocksource / arch_timer: Use ACPI GTDT table to initialize arch timer
On 2013年12月04日 23:33, Rob Herring wrote:
On Tue, Dec 3, 2013 at 5:15 AM, Hanjun Guo hanjun@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 **)gtdt, 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
--
To unsubscribe from this list: send the line
Re: [RFC part3 PATCH 1/2] clocksource / arch_timer: Use ACPI GTDT table to initialize arch timer
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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC part3 PATCH 1/2] clocksource / arch_timer: Use ACPI GTDT table to initialize arch timer
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC part3 PATCH 1/2] clocksource / arch_timer: Use ACPI GTDT table to initialize arch timer
On Tue, Dec 3, 2013 at 5:15 AM, 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
> Signed-off-by: Hanjun Guo
> ---
> 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
> #include
> #include
> +#include
>
> #include
> #include
> @@ -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 **), _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
Re: [RFC part3 PATCH 1/2] clocksource / arch_timer: Use ACPI GTDT table to initialize arch timer
On 2013年12月04日 01:04, Mark Rutland wrote:
On Tue, Dec 03, 2013 at 04:41:30PM +, 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
Signed-off-by: Hanjun Guo
---
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 **), _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
Re: [RFC part3 PATCH 1/2] clocksource / arch_timer: Use ACPI GTDT table to initialize arch timer
On 2013年12月04日 01:04, Mark Rutland wrote:
On Tue, Dec 03, 2013 at 04:41:30PM +, 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.dan...@samsung.com
Signed-off-by: Hanjun Guo hanjun@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
Re: [RFC part3 PATCH 1/2] clocksource / arch_timer: Use ACPI GTDT table to initialize arch timer
On Tue, Dec 3, 2013 at 5:15 AM, Hanjun Guo hanjun@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.dan...@samsung.com
Signed-off-by: Hanjun Guo hanjun@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 **)gtdt, 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
Re: [RFC part3 PATCH 1/2] clocksource / arch_timer: Use ACPI GTDT table to initialize arch timer
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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC part3 PATCH 1/2] clocksource / arch_timer: Use ACPI GTDT table to initialize arch timer
On Tue, Dec 03, 2013 at 04:41:30PM +, 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
> Signed-off-by: Hanjun Guo
> ---
> 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 **), _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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to
Re: [RFC part3 PATCH 1/2] clocksource / arch_timer: Use ACPI GTDT table to initialize arch timer
On Tue, Dec 03, 2013 at 04:41:30PM +, 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.dan...@samsung.com
Signed-off-by: Hanjun Guo hanjun@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.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
