| Message ID | 1468432402-4872-5-git-send-email-fu.wei@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jul 13, 2016 at 7:53 PM, <fu.wei@linaro.org> wrote: > From: Fu Wei <fu.wei@linaro.org> > > This patch adds support for parsing arch timer in GTDT, > provides some kernel APIs to parse all the PPIs and > always-on info in GTDT and export them. > > By this driver, we can simplify arm_arch_timer drivers, and > separate the ACPI GTDT knowledge from it. > > Signed-off-by: Fu Wei <fu.wei@linaro.org> > Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> > --- > drivers/acpi/Kconfig | 5 ++ > drivers/acpi/Makefile | 1 + > drivers/acpi/arm64/Kconfig | 15 ++++ > drivers/acpi/arm64/Makefile | 1 + > drivers/acpi/arm64/acpi_gtdt.c | 170 +++++++++++++++++++++++++++++++++++++++++ > include/linux/acpi.h | 6 ++ > 6 files changed, 198 insertions(+) > > diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig > index b7e2e77..1cdc7d2 100644 > --- a/drivers/acpi/Kconfig > +++ b/drivers/acpi/Kconfig > @@ -521,4 +521,9 @@ config XPOWER_PMIC_OPREGION > > endif > > +if ARM64 > +source "drivers/acpi/arm64/Kconfig" > + > +endif > + > endif # ACPI > diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile > index 251ce85..1a94ff7 100644 > --- a/drivers/acpi/Makefile > +++ b/drivers/acpi/Makefile > @@ -99,5 +99,6 @@ obj-$(CONFIG_ACPI_EXTLOG) += acpi_extlog.o > obj-$(CONFIG_PMIC_OPREGION) += pmic/intel_pmic.o > obj-$(CONFIG_CRC_PMIC_OPREGION) += pmic/intel_pmic_crc.o > obj-$(CONFIG_XPOWER_PMIC_OPREGION) += pmic/intel_pmic_xpower.o > +obj-$(CONFIG_ARM64) += arm64/ > > video-objs += acpi_video.o video_detect.o > diff --git a/drivers/acpi/arm64/Kconfig b/drivers/acpi/arm64/Kconfig > new file mode 100644 > index 0000000..ff5c253 > --- /dev/null > +++ b/drivers/acpi/arm64/Kconfig > @@ -0,0 +1,15 @@ > +# > +# ACPI Configuration for ARM64 > +# > + > +menu "The ARM64-specific ACPI Support" > + > +config ACPI_GTDT > + bool "ACPI GTDT table Support" This should depend on ARM64. Also I wonder if it needs to be user-selectable? Wouldn't it be better to enable it by default when building for ARM64 with ACPI? > + help > + GTDT (Generic Timer Description Table) provides information > + for per-processor timers and Platform (memory-mapped) timers > + for ARM platforms. Select this option to provide information > + needed for the timers init. > + > +endmenu > diff --git a/drivers/acpi/arm64/Makefile b/drivers/acpi/arm64/Makefile > new file mode 100644 > index 0000000..466de6b > --- /dev/null > +++ b/drivers/acpi/arm64/Makefile > @@ -0,0 +1 @@ > +obj-$(CONFIG_ACPI_GTDT) += acpi_gtdt.o > diff --git a/drivers/acpi/arm64/acpi_gtdt.c b/drivers/acpi/arm64/acpi_gtdt.c > new file mode 100644 > index 0000000..9ee977d > --- /dev/null > +++ b/drivers/acpi/arm64/acpi_gtdt.c > @@ -0,0 +1,170 @@ > +/* > + * ARM Specific GTDT table Support > + * > + * Copyright (C) 2016, Linaro Ltd. > + * Author: Daniel Lezcano <daniel.lezcano@linaro.org> > + * Fu Wei <fu.wei@linaro.org> > + * Hanjun Guo <hanjun.guo@linaro.org> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + */ > + > +#include <linux/acpi.h> > +#include <linux/init.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > + > +#include <clocksource/arm_arch_timer.h> > + > +#undef pr_fmt > +#define pr_fmt(fmt) "GTDT: " fmt I would add "ACPI" to the prefix too if I were you, but that's me. > + > +typedef struct { > + struct acpi_table_gtdt *gtdt; > + void *platform_timer_start; > + void *gtdt_end; > +} acpi_gtdt_desc_t; > + > +static acpi_gtdt_desc_t acpi_gtdt_desc __initdata; > + > +static inline void *next_platform_timer(void *platform_timer) > +{ > + struct acpi_gtdt_header *gh = platform_timer; > + > + platform_timer += gh->length; > + if (platform_timer < acpi_gtdt_desc.gtdt_end) > + return platform_timer; > + > + return NULL; > +} > + > +#define for_each_platform_timer(_g) \ > + for (_g = acpi_gtdt_desc.platform_timer_start; _g; \ > + _g = next_platform_timer(_g)) > + > +static inline bool is_timer_block(void *platform_timer) > +{ > + struct acpi_gtdt_header *gh = platform_timer; > + > + if (gh->type == ACPI_GTDT_TYPE_TIMER_BLOCK) > + return true; > + > + return false; This is just too much code. It would suffice to do return gh->type == ACPI_GTDT_TYPE_TIMER_BLOCK; > +} > + > +static inline bool is_watchdog(void *platform_timer) > +{ > + struct acpi_gtdt_header *gh = platform_timer; > + > + if (gh->type == ACPI_GTDT_TYPE_WATCHDOG) > + return true; > + > + return false; Just like above. > +} > + > +/* > + * Get some basic info from GTDT table, and init the global variables above > + * for all timers initialization of Generic Timer. > + * This function does some validation on GTDT table. > + */ > +static int __init acpi_gtdt_desc_init(struct acpi_table_header *table) > +{ > + struct acpi_table_gtdt *gtdt = container_of(table, > + struct acpi_table_gtdt, > + header); > + > + acpi_gtdt_desc.gtdt = gtdt; > + acpi_gtdt_desc.gtdt_end = (void *)table + table->length; > + > + if (table->revision < 2) { > + pr_info("Revision:%d doesn't support Platform Timers.\n", > + table->revision); Is it really useful to print this message (and the one below) at the "info" level? What about changing them to pr_debug()? > + return 0; > + } > + > + if (!gtdt->platform_timer_count) { > + pr_info("No Platform Timer.\n"); > + return 0; > + } > + > + acpi_gtdt_desc.platform_timer_start = (void *)gtdt + > + gtdt->platform_timer_offset; > + if (acpi_gtdt_desc.platform_timer_start < > + (void *)table + sizeof(struct acpi_table_gtdt)) { > + pr_err(FW_BUG "Platform Timer pointer error.\n"); Why pr_err()? > + acpi_gtdt_desc.platform_timer_start = NULL; > + return -EINVAL; > + } > + > + return gtdt->platform_timer_count; > +} > + > +static int __init map_generic_timer_interrupt(u32 interrupt, u32 flags) > +{ > + int trigger, polarity; > + > + if (!interrupt) > + return 0; > + > + trigger = (flags & ACPI_GTDT_INTERRUPT_MODE) ? ACPI_EDGE_SENSITIVE > + : ACPI_LEVEL_SENSITIVE; > + > + polarity = (flags & ACPI_GTDT_INTERRUPT_POLARITY) ? ACPI_ACTIVE_LOW > + : ACPI_ACTIVE_HIGH; > + > + return acpi_register_gsi(NULL, interrupt, trigger, polarity); > +} > + > +/* > + * Map the PPIs of per-cpu arch_timer. > + * @type: the type of PPI > + * Returns 0 if error. > + */ > +int __init acpi_gtdt_map_ppi(int type) > +{ > + struct acpi_table_gtdt *gtdt = acpi_gtdt_desc.gtdt; > + > + switch (type) { > + case PHYS_SECURE_PPI: > + return map_generic_timer_interrupt(gtdt->secure_el1_interrupt, > + gtdt->secure_el1_flags); > + case PHYS_NONSECURE_PPI: > + return map_generic_timer_interrupt(gtdt->non_secure_el1_interrupt, > + gtdt->non_secure_el1_flags); > + case VIRT_PPI: > + return map_generic_timer_interrupt(gtdt->virtual_timer_interrupt, > + gtdt->virtual_timer_flags); > + > + case HYP_PPI: > + return map_generic_timer_interrupt(gtdt->non_secure_el2_interrupt, > + gtdt->non_secure_el2_flags); > + default: > + pr_err("ppi type error.\n"); > + } > + > + return 0; > +} > + > +/* > + * acpi_gtdt_c3stop - got c3stop info from GTDT > + * > + * Returns 1 if the timer is powered in deep idle state, 0 otherwise. > + */ > +int __init acpi_gtdt_c3stop(void) Why not bool? > +{ > + struct acpi_table_gtdt *gtdt = acpi_gtdt_desc.gtdt; > + > + return !(gtdt->non_secure_el1_flags & ACPI_GTDT_ALWAYS_ON); > +} > + > +int __init gtdt_arch_timer_init(struct acpi_table_header *table) > +{ > + if (table) > + return acpi_gtdt_desc_init(table); > + > + pr_err("table pointer error.\n"); This message is totally unuseful. > + > + return -EINVAL; > +} What is supposed to be calling this function? > diff --git a/include/linux/acpi.h b/include/linux/acpi.h > index 288fac5..8439579 100644 > --- a/include/linux/acpi.h > +++ b/include/linux/acpi.h > @@ -532,6 +532,12 @@ void acpi_walk_dep_device_list(acpi_handle handle); > struct platform_device *acpi_create_platform_device(struct acpi_device *); > #define ACPI_PTR(_ptr) (_ptr) > > +#ifdef CONFIG_ACPI_GTDT > +int __init gtdt_arch_timer_init(struct acpi_table_header *table); > +int __init acpi_gtdt_map_ppi(int type); > +int __init acpi_gtdt_c3stop(void); The __init thing is not necessary here. > +#endif > + > #else /* !CONFIG_ACPI */ > > #define acpi_disabled 1 > -- Thanks, Rafael
On Wed, Jul 13, 2016 at 10:30 PM, Rafael J. Wysocki <rafael@kernel.org> wrote: > On Wed, Jul 13, 2016 at 7:53 PM, <fu.wei@linaro.org> wrote: >> From: Fu Wei <fu.wei@linaro.org> >> >> This patch adds support for parsing arch timer in GTDT, >> provides some kernel APIs to parse all the PPIs and >> always-on info in GTDT and export them. >> >> By this driver, we can simplify arm_arch_timer drivers, and >> separate the ACPI GTDT knowledge from it. >> >> Signed-off-by: Fu Wei <fu.wei@linaro.org> >> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> >> --- >> drivers/acpi/Kconfig | 5 ++ >> drivers/acpi/Makefile | 1 + >> drivers/acpi/arm64/Kconfig | 15 ++++ >> drivers/acpi/arm64/Makefile | 1 + >> drivers/acpi/arm64/acpi_gtdt.c | 170 +++++++++++++++++++++++++++++++++++++++++ >> include/linux/acpi.h | 6 ++ >> 6 files changed, 198 insertions(+) >> >> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig >> index b7e2e77..1cdc7d2 100644 >> --- a/drivers/acpi/Kconfig >> +++ b/drivers/acpi/Kconfig >> @@ -521,4 +521,9 @@ config XPOWER_PMIC_OPREGION >> >> endif >> >> +if ARM64 >> +source "drivers/acpi/arm64/Kconfig" >> + >> +endif >> + >> endif # ACPI >> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile >> index 251ce85..1a94ff7 100644 >> --- a/drivers/acpi/Makefile >> +++ b/drivers/acpi/Makefile >> @@ -99,5 +99,6 @@ obj-$(CONFIG_ACPI_EXTLOG) += acpi_extlog.o >> obj-$(CONFIG_PMIC_OPREGION) += pmic/intel_pmic.o >> obj-$(CONFIG_CRC_PMIC_OPREGION) += pmic/intel_pmic_crc.o >> obj-$(CONFIG_XPOWER_PMIC_OPREGION) += pmic/intel_pmic_xpower.o >> +obj-$(CONFIG_ARM64) += arm64/ >> >> video-objs += acpi_video.o video_detect.o >> diff --git a/drivers/acpi/arm64/Kconfig b/drivers/acpi/arm64/Kconfig >> new file mode 100644 >> index 0000000..ff5c253 >> --- /dev/null >> +++ b/drivers/acpi/arm64/Kconfig >> @@ -0,0 +1,15 @@ >> +# >> +# ACPI Configuration for ARM64 >> +# >> + >> +menu "The ARM64-specific ACPI Support" >> + >> +config ACPI_GTDT >> + bool "ACPI GTDT table Support" > > This should depend on ARM64. > > Also I wonder if it needs to be user-selectable? Wouldn't it be > better to enable it by default when building for ARM64 with ACPI? > >> + help >> + GTDT (Generic Timer Description Table) provides information >> + for per-processor timers and Platform (memory-mapped) timers >> + for ARM platforms. Select this option to provide information >> + needed for the timers init. >> + >> +endmenu >> diff --git a/drivers/acpi/arm64/Makefile b/drivers/acpi/arm64/Makefile >> new file mode 100644 >> index 0000000..466de6b >> --- /dev/null >> +++ b/drivers/acpi/arm64/Makefile >> @@ -0,0 +1 @@ >> +obj-$(CONFIG_ACPI_GTDT) += acpi_gtdt.o >> diff --git a/drivers/acpi/arm64/acpi_gtdt.c b/drivers/acpi/arm64/acpi_gtdt.c >> new file mode 100644 >> index 0000000..9ee977d >> --- /dev/null >> +++ b/drivers/acpi/arm64/acpi_gtdt.c >> @@ -0,0 +1,170 @@ >> +/* >> + * ARM Specific GTDT table Support >> + * >> + * Copyright (C) 2016, Linaro Ltd. >> + * Author: Daniel Lezcano <daniel.lezcano@linaro.org> >> + * Fu Wei <fu.wei@linaro.org> >> + * Hanjun Guo <hanjun.guo@linaro.org> >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 as >> + * published by the Free Software Foundation. >> + */ >> + >> +#include <linux/acpi.h> >> +#include <linux/init.h> >> +#include <linux/kernel.h> >> +#include <linux/module.h> >> + >> +#include <clocksource/arm_arch_timer.h> >> + >> +#undef pr_fmt >> +#define pr_fmt(fmt) "GTDT: " fmt > > I would add "ACPI" to the prefix too if I were you, but that's me. > >> + >> +typedef struct { >> + struct acpi_table_gtdt *gtdt; >> + void *platform_timer_start; >> + void *gtdt_end; >> +} acpi_gtdt_desc_t; >> + >> +static acpi_gtdt_desc_t acpi_gtdt_desc __initdata; >> + >> +static inline void *next_platform_timer(void *platform_timer) >> +{ >> + struct acpi_gtdt_header *gh = platform_timer; >> + >> + platform_timer += gh->length; >> + if (platform_timer < acpi_gtdt_desc.gtdt_end) >> + return platform_timer; >> + >> + return NULL; >> +} >> + >> +#define for_each_platform_timer(_g) \ >> + for (_g = acpi_gtdt_desc.platform_timer_start; _g; \ >> + _g = next_platform_timer(_g)) >> + >> +static inline bool is_timer_block(void *platform_timer) >> +{ >> + struct acpi_gtdt_header *gh = platform_timer; >> + >> + if (gh->type == ACPI_GTDT_TYPE_TIMER_BLOCK) >> + return true; >> + >> + return false; > > This is just too much code. It would suffice to do > > return gh->type == ACPI_GTDT_TYPE_TIMER_BLOCK; > >> +} >> + >> +static inline bool is_watchdog(void *platform_timer) >> +{ >> + struct acpi_gtdt_header *gh = platform_timer; >> + >> + if (gh->type == ACPI_GTDT_TYPE_WATCHDOG) >> + return true; >> + >> + return false; > > Just like above. > >> +} >> + >> +/* >> + * Get some basic info from GTDT table, and init the global variables above >> + * for all timers initialization of Generic Timer. >> + * This function does some validation on GTDT table. >> + */ >> +static int __init acpi_gtdt_desc_init(struct acpi_table_header *table) >> +{ >> + struct acpi_table_gtdt *gtdt = container_of(table, >> + struct acpi_table_gtdt, >> + header); >> + >> + acpi_gtdt_desc.gtdt = gtdt; >> + acpi_gtdt_desc.gtdt_end = (void *)table + table->length; >> + >> + if (table->revision < 2) { >> + pr_info("Revision:%d doesn't support Platform Timers.\n", >> + table->revision); > > Is it really useful to print this message (and the one below) at the > "info" level? What about changing them to pr_debug()? > >> + return 0; >> + } >> + >> + if (!gtdt->platform_timer_count) { >> + pr_info("No Platform Timer.\n"); >> + return 0; >> + } >> + >> + acpi_gtdt_desc.platform_timer_start = (void *)gtdt + >> + gtdt->platform_timer_offset; BTW, breaking lines this way doesn't make the code particularly more readable. >> + if (acpi_gtdt_desc.platform_timer_start < >> + (void *)table + sizeof(struct acpi_table_gtdt)) { Same here. It might be avoided by using a local (void *) variable "start", ie. start = (void *)gtdt + gtdt->platform_timer_offset; if (start < (void *)table + sizeof(struct acpi_table_gtdt))) { print message; return -EINVAL; } acpi_gtdt_desc.platform_timer_start = start; Thanks, Rafael
On Wed, Jul 13, 2016 at 10:30:37PM +0200, Rafael J. Wysocki wrote: > On Wed, Jul 13, 2016 at 7:53 PM, <fu.wei@linaro.org> wrote: > > From: Fu Wei <fu.wei@linaro.org> > > > > This patch adds support for parsing arch timer in GTDT, > > provides some kernel APIs to parse all the PPIs and > > always-on info in GTDT and export them. > > > > By this driver, we can simplify arm_arch_timer drivers, and > > separate the ACPI GTDT knowledge from it. > > > > Signed-off-by: Fu Wei <fu.wei@linaro.org> > > Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> > > --- > > drivers/acpi/Kconfig | 5 ++ > > drivers/acpi/Makefile | 1 + > > drivers/acpi/arm64/Kconfig | 15 ++++ > > drivers/acpi/arm64/Makefile | 1 + > > drivers/acpi/arm64/acpi_gtdt.c | 170 +++++++++++++++++++++++++++++++++++++++++ > > include/linux/acpi.h | 6 ++ > > 6 files changed, 198 insertions(+) > > > > diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig > > index b7e2e77..1cdc7d2 100644 > > --- a/drivers/acpi/Kconfig > > +++ b/drivers/acpi/Kconfig > > @@ -521,4 +521,9 @@ config XPOWER_PMIC_OPREGION > > > > endif > > > > +if ARM64 > > +source "drivers/acpi/arm64/Kconfig" > > + > > +endif > > + > > endif # ACPI > > diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile > > index 251ce85..1a94ff7 100644 > > --- a/drivers/acpi/Makefile > > +++ b/drivers/acpi/Makefile > > @@ -99,5 +99,6 @@ obj-$(CONFIG_ACPI_EXTLOG) += acpi_extlog.o > > obj-$(CONFIG_PMIC_OPREGION) += pmic/intel_pmic.o > > obj-$(CONFIG_CRC_PMIC_OPREGION) += pmic/intel_pmic_crc.o > > obj-$(CONFIG_XPOWER_PMIC_OPREGION) += pmic/intel_pmic_xpower.o > > +obj-$(CONFIG_ARM64) += arm64/ > > > > video-objs += acpi_video.o video_detect.o > > diff --git a/drivers/acpi/arm64/Kconfig b/drivers/acpi/arm64/Kconfig > > new file mode 100644 > > index 0000000..ff5c253 > > --- /dev/null > > +++ b/drivers/acpi/arm64/Kconfig > > @@ -0,0 +1,15 @@ > > +# > > +# ACPI Configuration for ARM64 > > +# > > + > > +menu "The ARM64-specific ACPI Support" > > + > > +config ACPI_GTDT > > + bool "ACPI GTDT table Support" > > This should depend on ARM64. > > Also I wonder if it needs to be user-selectable? Wouldn't it be > better to enable it by default when building for ARM64 with ACPI? > It is currently selected in patch 9, in the watchdog driver's Kconfig entry. Not sure if I like that; maybe the watchdog driver should depend on it instead ? Guenter > > + help > > + GTDT (Generic Timer Description Table) provides information > > + for per-processor timers and Platform (memory-mapped) timers > > + for ARM platforms. Select this option to provide information > > + needed for the timers init. > > + > > +endmenu > > diff --git a/drivers/acpi/arm64/Makefile b/drivers/acpi/arm64/Makefile > > new file mode 100644 > > index 0000000..466de6b > > --- /dev/null > > +++ b/drivers/acpi/arm64/Makefile > > @@ -0,0 +1 @@ > > +obj-$(CONFIG_ACPI_GTDT) += acpi_gtdt.o > > diff --git a/drivers/acpi/arm64/acpi_gtdt.c b/drivers/acpi/arm64/acpi_gtdt.c > > new file mode 100644 > > index 0000000..9ee977d > > --- /dev/null > > +++ b/drivers/acpi/arm64/acpi_gtdt.c > > @@ -0,0 +1,170 @@ > > +/* > > + * ARM Specific GTDT table Support > > + * > > + * Copyright (C) 2016, Linaro Ltd. > > + * Author: Daniel Lezcano <daniel.lezcano@linaro.org> > > + * Fu Wei <fu.wei@linaro.org> > > + * Hanjun Guo <hanjun.guo@linaro.org> > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License version 2 as > > + * published by the Free Software Foundation. > > + */ > > + > > +#include <linux/acpi.h> > > +#include <linux/init.h> > > +#include <linux/kernel.h> > > +#include <linux/module.h> > > + > > +#include <clocksource/arm_arch_timer.h> > > + > > +#undef pr_fmt > > +#define pr_fmt(fmt) "GTDT: " fmt > > I would add "ACPI" to the prefix too if I were you, but that's me. > > > + > > +typedef struct { > > + struct acpi_table_gtdt *gtdt; > > + void *platform_timer_start; > > + void *gtdt_end; > > +} acpi_gtdt_desc_t; > > + > > +static acpi_gtdt_desc_t acpi_gtdt_desc __initdata; > > + > > +static inline void *next_platform_timer(void *platform_timer) > > +{ > > + struct acpi_gtdt_header *gh = platform_timer; > > + > > + platform_timer += gh->length; > > + if (platform_timer < acpi_gtdt_desc.gtdt_end) > > + return platform_timer; > > + > > + return NULL; > > +} > > + > > +#define for_each_platform_timer(_g) \ > > + for (_g = acpi_gtdt_desc.platform_timer_start; _g; \ > > + _g = next_platform_timer(_g)) > > + > > +static inline bool is_timer_block(void *platform_timer) > > +{ > > + struct acpi_gtdt_header *gh = platform_timer; > > + > > + if (gh->type == ACPI_GTDT_TYPE_TIMER_BLOCK) > > + return true; > > + > > + return false; > > This is just too much code. It would suffice to do > > return gh->type == ACPI_GTDT_TYPE_TIMER_BLOCK; > > > +} > > + > > +static inline bool is_watchdog(void *platform_timer) > > +{ > > + struct acpi_gtdt_header *gh = platform_timer; > > + > > + if (gh->type == ACPI_GTDT_TYPE_WATCHDOG) > > + return true; > > + > > + return false; > > Just like above. > > > +} > > + > > +/* > > + * Get some basic info from GTDT table, and init the global variables above > > + * for all timers initialization of Generic Timer. > > + * This function does some validation on GTDT table. > > + */ > > +static int __init acpi_gtdt_desc_init(struct acpi_table_header *table) > > +{ > > + struct acpi_table_gtdt *gtdt = container_of(table, > > + struct acpi_table_gtdt, > > + header); > > + > > + acpi_gtdt_desc.gtdt = gtdt; > > + acpi_gtdt_desc.gtdt_end = (void *)table + table->length; > > + > > + if (table->revision < 2) { > > + pr_info("Revision:%d doesn't support Platform Timers.\n", > > + table->revision); > > Is it really useful to print this message (and the one below) at the > "info" level? What about changing them to pr_debug()? > > > + return 0; > > + } > > + > > + if (!gtdt->platform_timer_count) { > > + pr_info("No Platform Timer.\n"); > > + return 0; > > + } > > + > > + acpi_gtdt_desc.platform_timer_start = (void *)gtdt + > > + gtdt->platform_timer_offset; > > + if (acpi_gtdt_desc.platform_timer_start < > > + (void *)table + sizeof(struct acpi_table_gtdt)) { > > + pr_err(FW_BUG "Platform Timer pointer error.\n"); > > Why pr_err()? > > > + acpi_gtdt_desc.platform_timer_start = NULL; > > + return -EINVAL; > > + } > > + > > + return gtdt->platform_timer_count; > > +} > > + > > +static int __init map_generic_timer_interrupt(u32 interrupt, u32 flags) > > +{ > > + int trigger, polarity; > > + > > + if (!interrupt) > > + return 0; > > + > > + trigger = (flags & ACPI_GTDT_INTERRUPT_MODE) ? ACPI_EDGE_SENSITIVE > > + : ACPI_LEVEL_SENSITIVE; > > + > > + polarity = (flags & ACPI_GTDT_INTERRUPT_POLARITY) ? ACPI_ACTIVE_LOW > > + : ACPI_ACTIVE_HIGH; > > + > > + return acpi_register_gsi(NULL, interrupt, trigger, polarity); > > +} > > + > > +/* > > + * Map the PPIs of per-cpu arch_timer. > > + * @type: the type of PPI > > + * Returns 0 if error. > > + */ > > +int __init acpi_gtdt_map_ppi(int type) > > +{ > > + struct acpi_table_gtdt *gtdt = acpi_gtdt_desc.gtdt; > > + > > + switch (type) { > > + case PHYS_SECURE_PPI: > > + return map_generic_timer_interrupt(gtdt->secure_el1_interrupt, > > + gtdt->secure_el1_flags); > > + case PHYS_NONSECURE_PPI: > > + return map_generic_timer_interrupt(gtdt->non_secure_el1_interrupt, > > + gtdt->non_secure_el1_flags); > > + case VIRT_PPI: > > + return map_generic_timer_interrupt(gtdt->virtual_timer_interrupt, > > + gtdt->virtual_timer_flags); > > + > > + case HYP_PPI: > > + return map_generic_timer_interrupt(gtdt->non_secure_el2_interrupt, > > + gtdt->non_secure_el2_flags); > > + default: > > + pr_err("ppi type error.\n"); > > + } > > + > > + return 0; > > +} > > + > > +/* > > + * acpi_gtdt_c3stop - got c3stop info from GTDT > > + * > > + * Returns 1 if the timer is powered in deep idle state, 0 otherwise. > > + */ > > +int __init acpi_gtdt_c3stop(void) > > Why not bool? > > > +{ > > + struct acpi_table_gtdt *gtdt = acpi_gtdt_desc.gtdt; > > + > > + return !(gtdt->non_secure_el1_flags & ACPI_GTDT_ALWAYS_ON); > > +} > > + > > +int __init gtdt_arch_timer_init(struct acpi_table_header *table) > > +{ > > + if (table) > > + return acpi_gtdt_desc_init(table); > > + > > + pr_err("table pointer error.\n"); > > This message is totally unuseful. > > > + > > + return -EINVAL; > > +} > > What is supposed to be calling this function? > > > diff --git a/include/linux/acpi.h b/include/linux/acpi.h > > index 288fac5..8439579 100644 > > --- a/include/linux/acpi.h > > +++ b/include/linux/acpi.h > > @@ -532,6 +532,12 @@ void acpi_walk_dep_device_list(acpi_handle handle); > > struct platform_device *acpi_create_platform_device(struct acpi_device *); > > #define ACPI_PTR(_ptr) (_ptr) > > > > +#ifdef CONFIG_ACPI_GTDT > > +int __init gtdt_arch_timer_init(struct acpi_table_header *table); > > +int __init acpi_gtdt_map_ppi(int type); > > +int __init acpi_gtdt_c3stop(void); > > The __init thing is not necessary here. > > > +#endif > > + > > #else /* !CONFIG_ACPI */ > > > > #define acpi_disabled 1 > > -- > > Thanks, > Rafael
On Wed, Jul 13, 2016 at 11:08 PM, Guenter Roeck <linux@roeck-us.net> wrote: > On Wed, Jul 13, 2016 at 10:30:37PM +0200, Rafael J. Wysocki wrote: >> On Wed, Jul 13, 2016 at 7:53 PM, <fu.wei@linaro.org> wrote: >> > From: Fu Wei <fu.wei@linaro.org> >> > >> > This patch adds support for parsing arch timer in GTDT, >> > provides some kernel APIs to parse all the PPIs and >> > always-on info in GTDT and export them. >> > >> > By this driver, we can simplify arm_arch_timer drivers, and >> > separate the ACPI GTDT knowledge from it. >> > >> > Signed-off-by: Fu Wei <fu.wei@linaro.org> >> > Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> >> > --- >> > drivers/acpi/Kconfig | 5 ++ >> > drivers/acpi/Makefile | 1 + >> > drivers/acpi/arm64/Kconfig | 15 ++++ >> > drivers/acpi/arm64/Makefile | 1 + >> > drivers/acpi/arm64/acpi_gtdt.c | 170 +++++++++++++++++++++++++++++++++++++++++ >> > include/linux/acpi.h | 6 ++ >> > 6 files changed, 198 insertions(+) >> > >> > diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig >> > index b7e2e77..1cdc7d2 100644 >> > --- a/drivers/acpi/Kconfig >> > +++ b/drivers/acpi/Kconfig >> > @@ -521,4 +521,9 @@ config XPOWER_PMIC_OPREGION >> > >> > endif >> > >> > +if ARM64 >> > +source "drivers/acpi/arm64/Kconfig" >> > + >> > +endif >> > + >> > endif # ACPI >> > diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile >> > index 251ce85..1a94ff7 100644 >> > --- a/drivers/acpi/Makefile >> > +++ b/drivers/acpi/Makefile >> > @@ -99,5 +99,6 @@ obj-$(CONFIG_ACPI_EXTLOG) += acpi_extlog.o >> > obj-$(CONFIG_PMIC_OPREGION) += pmic/intel_pmic.o >> > obj-$(CONFIG_CRC_PMIC_OPREGION) += pmic/intel_pmic_crc.o >> > obj-$(CONFIG_XPOWER_PMIC_OPREGION) += pmic/intel_pmic_xpower.o >> > +obj-$(CONFIG_ARM64) += arm64/ >> > >> > video-objs += acpi_video.o video_detect.o >> > diff --git a/drivers/acpi/arm64/Kconfig b/drivers/acpi/arm64/Kconfig >> > new file mode 100644 >> > index 0000000..ff5c253 >> > --- /dev/null >> > +++ b/drivers/acpi/arm64/Kconfig >> > @@ -0,0 +1,15 @@ >> > +# >> > +# ACPI Configuration for ARM64 >> > +# >> > + >> > +menu "The ARM64-specific ACPI Support" >> > + >> > +config ACPI_GTDT >> > + bool "ACPI GTDT table Support" >> >> This should depend on ARM64. >> >> Also I wonder if it needs to be user-selectable? Wouldn't it be >> better to enable it by default when building for ARM64 with ACPI? >> > It is currently selected in patch 9, in the watchdog driver's Kconfig > entry. Well, it still doesn't have to be user-selectable for that. :-) > Not sure if I like that; maybe the watchdog driver should depend > on it instead ? If the watchdog is not the only user of it (and I don't think it is), it would be better to arrange things this way. Thanks, Rafael
On Wed, Jul 13, 2016 at 1:53 PM, <fu.wei@linaro.org> wrote: > From: Fu Wei <fu.wei@linaro.org> > > This patch adds support for parsing arch timer in GTDT, > provides some kernel APIs to parse all the PPIs and > always-on info in GTDT and export them. > > By this driver, we can simplify arm_arch_timer drivers, and > separate the ACPI GTDT knowledge from it. > > Signed-off-by: Fu Wei <fu.wei@linaro.org> > Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> > --- > drivers/acpi/Kconfig | 5 ++ > drivers/acpi/Makefile | 1 + > drivers/acpi/arm64/Kconfig | 15 ++++ > drivers/acpi/arm64/Makefile | 1 + > drivers/acpi/arm64/acpi_gtdt.c | 170 +++++++++++++++++++++++++++++++++++++++++ > include/linux/acpi.h | 6 ++ > 6 files changed, 198 insertions(+) > > diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig > index b7e2e77..1cdc7d2 100644 > --- a/drivers/acpi/Kconfig > +++ b/drivers/acpi/Kconfig > @@ -521,4 +521,9 @@ config XPOWER_PMIC_OPREGION > > endif > > +if ARM64 > +source "drivers/acpi/arm64/Kconfig" > + > +endif > + > endif # ACPI > diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile > index 251ce85..1a94ff7 100644 > --- a/drivers/acpi/Makefile > +++ b/drivers/acpi/Makefile > @@ -99,5 +99,6 @@ obj-$(CONFIG_ACPI_EXTLOG) += acpi_extlog.o > obj-$(CONFIG_PMIC_OPREGION) += pmic/intel_pmic.o > obj-$(CONFIG_CRC_PMIC_OPREGION) += pmic/intel_pmic_crc.o > obj-$(CONFIG_XPOWER_PMIC_OPREGION) += pmic/intel_pmic_xpower.o > +obj-$(CONFIG_ARM64) += arm64/ > > video-objs += acpi_video.o video_detect.o > diff --git a/drivers/acpi/arm64/Kconfig b/drivers/acpi/arm64/Kconfig > new file mode 100644 > index 0000000..ff5c253 > --- /dev/null > +++ b/drivers/acpi/arm64/Kconfig > @@ -0,0 +1,15 @@ > +# > +# ACPI Configuration for ARM64 > +# > + > +menu "The ARM64-specific ACPI Support" > + > +config ACPI_GTDT > + bool "ACPI GTDT table Support" > + help > + GTDT (Generic Timer Description Table) provides information > + for per-processor timers and Platform (memory-mapped) timers > + for ARM platforms. Select this option to provide information > + needed for the timers init. > + > +endmenu > diff --git a/drivers/acpi/arm64/Makefile b/drivers/acpi/arm64/Makefile > new file mode 100644 > index 0000000..466de6b > --- /dev/null > +++ b/drivers/acpi/arm64/Makefile > @@ -0,0 +1 @@ > +obj-$(CONFIG_ACPI_GTDT) += acpi_gtdt.o > diff --git a/drivers/acpi/arm64/acpi_gtdt.c b/drivers/acpi/arm64/acpi_gtdt.c > new file mode 100644 > index 0000000..9ee977d > --- /dev/null > +++ b/drivers/acpi/arm64/acpi_gtdt.c > @@ -0,0 +1,170 @@ > +/* > + * ARM Specific GTDT table Support > + * > + * Copyright (C) 2016, Linaro Ltd. > + * Author: Daniel Lezcano <daniel.lezcano@linaro.org> > + * Fu Wei <fu.wei@linaro.org> > + * Hanjun Guo <hanjun.guo@linaro.org> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + */ > + > +#include <linux/acpi.h> > +#include <linux/init.h> > +#include <linux/kernel.h> > +#include <linux/module.h> Please do not use module.h in drivers that are using a bool Kconfig setting. Thanks, Paul.
Hi Rafael, On 14 July 2016 at 04:30, Rafael J. Wysocki <rafael@kernel.org> wrote: > On Wed, Jul 13, 2016 at 7:53 PM, <fu.wei@linaro.org> wrote: >> From: Fu Wei <fu.wei@linaro.org> >> >> This patch adds support for parsing arch timer in GTDT, >> provides some kernel APIs to parse all the PPIs and >> always-on info in GTDT and export them. >> >> By this driver, we can simplify arm_arch_timer drivers, and >> separate the ACPI GTDT knowledge from it. >> >> Signed-off-by: Fu Wei <fu.wei@linaro.org> >> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> >> --- >> drivers/acpi/Kconfig | 5 ++ >> drivers/acpi/Makefile | 1 + >> drivers/acpi/arm64/Kconfig | 15 ++++ >> drivers/acpi/arm64/Makefile | 1 + >> drivers/acpi/arm64/acpi_gtdt.c | 170 +++++++++++++++++++++++++++++++++++++++++ >> include/linux/acpi.h | 6 ++ >> 6 files changed, 198 insertions(+) >> >> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig >> index b7e2e77..1cdc7d2 100644 >> --- a/drivers/acpi/Kconfig >> +++ b/drivers/acpi/Kconfig >> @@ -521,4 +521,9 @@ config XPOWER_PMIC_OPREGION >> >> endif >> >> +if ARM64 >> +source "drivers/acpi/arm64/Kconfig" >> + >> +endif >> + >> endif # ACPI >> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile >> index 251ce85..1a94ff7 100644 >> --- a/drivers/acpi/Makefile >> +++ b/drivers/acpi/Makefile >> @@ -99,5 +99,6 @@ obj-$(CONFIG_ACPI_EXTLOG) += acpi_extlog.o >> obj-$(CONFIG_PMIC_OPREGION) += pmic/intel_pmic.o >> obj-$(CONFIG_CRC_PMIC_OPREGION) += pmic/intel_pmic_crc.o >> obj-$(CONFIG_XPOWER_PMIC_OPREGION) += pmic/intel_pmic_xpower.o >> +obj-$(CONFIG_ARM64) += arm64/ >> >> video-objs += acpi_video.o video_detect.o >> diff --git a/drivers/acpi/arm64/Kconfig b/drivers/acpi/arm64/Kconfig >> new file mode 100644 >> index 0000000..ff5c253 >> --- /dev/null >> +++ b/drivers/acpi/arm64/Kconfig >> @@ -0,0 +1,15 @@ >> +# >> +# ACPI Configuration for ARM64 >> +# >> + >> +menu "The ARM64-specific ACPI Support" >> + >> +config ACPI_GTDT >> + bool "ACPI GTDT table Support" > > This should depend on ARM64. > > Also I wonder if it needs to be user-selectable? Wouldn't it be > better to enable it by default when building for ARM64 with ACPI? > >> + help >> + GTDT (Generic Timer Description Table) provides information >> + for per-processor timers and Platform (memory-mapped) timers >> + for ARM platforms. Select this option to provide information >> + needed for the timers init. >> + >> +endmenu >> diff --git a/drivers/acpi/arm64/Makefile b/drivers/acpi/arm64/Makefile >> new file mode 100644 >> index 0000000..466de6b >> --- /dev/null >> +++ b/drivers/acpi/arm64/Makefile >> @@ -0,0 +1 @@ >> +obj-$(CONFIG_ACPI_GTDT) += acpi_gtdt.o >> diff --git a/drivers/acpi/arm64/acpi_gtdt.c b/drivers/acpi/arm64/acpi_gtdt.c >> new file mode 100644 >> index 0000000..9ee977d >> --- /dev/null >> +++ b/drivers/acpi/arm64/acpi_gtdt.c >> @@ -0,0 +1,170 @@ >> +/* >> + * ARM Specific GTDT table Support >> + * >> + * Copyright (C) 2016, Linaro Ltd. >> + * Author: Daniel Lezcano <daniel.lezcano@linaro.org> >> + * Fu Wei <fu.wei@linaro.org> >> + * Hanjun Guo <hanjun.guo@linaro.org> >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 as >> + * published by the Free Software Foundation. >> + */ >> + >> +#include <linux/acpi.h> >> +#include <linux/init.h> >> +#include <linux/kernel.h> >> +#include <linux/module.h> >> + >> +#include <clocksource/arm_arch_timer.h> >> + >> +#undef pr_fmt >> +#define pr_fmt(fmt) "GTDT: " fmt > > I would add "ACPI" to the prefix too if I were you, but that's me. good idea, you are right, will do > >> + >> +typedef struct { >> + struct acpi_table_gtdt *gtdt; >> + void *platform_timer_start; >> + void *gtdt_end; >> +} acpi_gtdt_desc_t; >> + >> +static acpi_gtdt_desc_t acpi_gtdt_desc __initdata; >> + >> +static inline void *next_platform_timer(void *platform_timer) >> +{ >> + struct acpi_gtdt_header *gh = platform_timer; >> + >> + platform_timer += gh->length; >> + if (platform_timer < acpi_gtdt_desc.gtdt_end) >> + return platform_timer; >> + >> + return NULL; >> +} >> + >> +#define for_each_platform_timer(_g) \ >> + for (_g = acpi_gtdt_desc.platform_timer_start; _g; \ >> + _g = next_platform_timer(_g)) >> + >> +static inline bool is_timer_block(void *platform_timer) >> +{ >> + struct acpi_gtdt_header *gh = platform_timer; >> + >> + if (gh->type == ACPI_GTDT_TYPE_TIMER_BLOCK) >> + return true; >> + >> + return false; > > This is just too much code. It would suffice to do > > return gh->type == ACPI_GTDT_TYPE_TIMER_BLOCK; > >> +} >> + >> +static inline bool is_watchdog(void *platform_timer) >> +{ >> + struct acpi_gtdt_header *gh = platform_timer; >> + >> + if (gh->type == ACPI_GTDT_TYPE_WATCHDOG) >> + return true; >> + >> + return false; > > Just like above. Thanks, this is better :-) will do > >> +} >> + >> +/* >> + * Get some basic info from GTDT table, and init the global variables above >> + * for all timers initialization of Generic Timer. >> + * This function does some validation on GTDT table. >> + */ >> +static int __init acpi_gtdt_desc_init(struct acpi_table_header *table) >> +{ >> + struct acpi_table_gtdt *gtdt = container_of(table, >> + struct acpi_table_gtdt, >> + header); >> + >> + acpi_gtdt_desc.gtdt = gtdt; >> + acpi_gtdt_desc.gtdt_end = (void *)table + table->length; >> + >> + if (table->revision < 2) { >> + pr_info("Revision:%d doesn't support Platform Timers.\n", >> + table->revision); > > Is it really useful to print this message (and the one below) at the > "info" level? What about changing them to pr_debug()? yes, pr_debug is better, thanks :-) will do > >> + return 0; >> + } >> + >> + if (!gtdt->platform_timer_count) { >> + pr_info("No Platform Timer.\n"); >> + return 0; >> + } >> + >> + acpi_gtdt_desc.platform_timer_start = (void *)gtdt + >> + gtdt->platform_timer_offset; >> + if (acpi_gtdt_desc.platform_timer_start < >> + (void *)table + sizeof(struct acpi_table_gtdt)) { >> + pr_err(FW_BUG "Platform Timer pointer error.\n"); > > Why pr_err()? if (true), that means the GTDT table has bugs. > >> + acpi_gtdt_desc.platform_timer_start = NULL; >> + return -EINVAL; >> + } >> + >> + return gtdt->platform_timer_count; >> +} >> + >> +static int __init map_generic_timer_interrupt(u32 interrupt, u32 flags) >> +{ >> + int trigger, polarity; >> + >> + if (!interrupt) >> + return 0; >> + >> + trigger = (flags & ACPI_GTDT_INTERRUPT_MODE) ? ACPI_EDGE_SENSITIVE >> + : ACPI_LEVEL_SENSITIVE; >> + >> + polarity = (flags & ACPI_GTDT_INTERRUPT_POLARITY) ? ACPI_ACTIVE_LOW >> + : ACPI_ACTIVE_HIGH; >> + >> + return acpi_register_gsi(NULL, interrupt, trigger, polarity); >> +} >> + >> +/* >> + * Map the PPIs of per-cpu arch_timer. >> + * @type: the type of PPI >> + * Returns 0 if error. >> + */ >> +int __init acpi_gtdt_map_ppi(int type) >> +{ >> + struct acpi_table_gtdt *gtdt = acpi_gtdt_desc.gtdt; >> + >> + switch (type) { >> + case PHYS_SECURE_PPI: >> + return map_generic_timer_interrupt(gtdt->secure_el1_interrupt, >> + gtdt->secure_el1_flags); >> + case PHYS_NONSECURE_PPI: >> + return map_generic_timer_interrupt(gtdt->non_secure_el1_interrupt, >> + gtdt->non_secure_el1_flags); >> + case VIRT_PPI: >> + return map_generic_timer_interrupt(gtdt->virtual_timer_interrupt, >> + gtdt->virtual_timer_flags); >> + >> + case HYP_PPI: >> + return map_generic_timer_interrupt(gtdt->non_secure_el2_interrupt, >> + gtdt->non_secure_el2_flags); >> + default: >> + pr_err("ppi type error.\n"); >> + } >> + >> + return 0; >> +} >> + >> +/* >> + * acpi_gtdt_c3stop - got c3stop info from GTDT >> + * >> + * Returns 1 if the timer is powered in deep idle state, 0 otherwise. >> + */ >> +int __init acpi_gtdt_c3stop(void) > > Why not bool? I forget to fix it, sorry, will do. > >> +{ >> + struct acpi_table_gtdt *gtdt = acpi_gtdt_desc.gtdt; >> + >> + return !(gtdt->non_secure_el1_flags & ACPI_GTDT_ALWAYS_ON); >> +} >> + >> +int __init gtdt_arch_timer_init(struct acpi_table_header *table) >> +{ >> + if (table) >> + return acpi_gtdt_desc_init(table); >> + >> + pr_err("table pointer error.\n"); > > This message is totally unuseful. will delete it acpi_gtdt_desc_init, and move the code to here. > >> + >> + return -EINVAL; >> +} > > What is supposed to be calling this function? at this point, I think I can simplify this code a little bit. Thanks :-) I will delete one > >> diff --git a/include/linux/acpi.h b/include/linux/acpi.h >> index 288fac5..8439579 100644 >> --- a/include/linux/acpi.h >> +++ b/include/linux/acpi.h >> @@ -532,6 +532,12 @@ void acpi_walk_dep_device_list(acpi_handle handle); >> struct platform_device *acpi_create_platform_device(struct acpi_device *); >> #define ACPI_PTR(_ptr) (_ptr) >> >> +#ifdef CONFIG_ACPI_GTDT >> +int __init gtdt_arch_timer_init(struct acpi_table_header *table); >> +int __init acpi_gtdt_map_ppi(int type); >> +int __init acpi_gtdt_c3stop(void); > > The __init thing is not necessary here. will delete them, thanks > >> +#endif >> + >> #else /* !CONFIG_ACPI */ >> >> #define acpi_disabled 1 >> -- > > Thanks, > Rafael
Hi Rafael, On 14 July 2016 at 04:39, Rafael J. Wysocki <rafael@kernel.org> wrote: > On Wed, Jul 13, 2016 at 10:30 PM, Rafael J. Wysocki <rafael@kernel.org> wrote: >> On Wed, Jul 13, 2016 at 7:53 PM, <fu.wei@linaro.org> wrote: >>> From: Fu Wei <fu.wei@linaro.org> >>> >>> This patch adds support for parsing arch timer in GTDT, >>> provides some kernel APIs to parse all the PPIs and >>> always-on info in GTDT and export them. >>> >>> By this driver, we can simplify arm_arch_timer drivers, and >>> separate the ACPI GTDT knowledge from it. >>> >>> Signed-off-by: Fu Wei <fu.wei@linaro.org> >>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> >>> --- >>> drivers/acpi/Kconfig | 5 ++ >>> drivers/acpi/Makefile | 1 + >>> drivers/acpi/arm64/Kconfig | 15 ++++ >>> drivers/acpi/arm64/Makefile | 1 + >>> drivers/acpi/arm64/acpi_gtdt.c | 170 +++++++++++++++++++++++++++++++++++++++++ >>> include/linux/acpi.h | 6 ++ >>> 6 files changed, 198 insertions(+) >>> >>> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig >>> index b7e2e77..1cdc7d2 100644 >>> --- a/drivers/acpi/Kconfig >>> +++ b/drivers/acpi/Kconfig >>> @@ -521,4 +521,9 @@ config XPOWER_PMIC_OPREGION >>> >>> endif >>> >>> +if ARM64 >>> +source "drivers/acpi/arm64/Kconfig" >>> + >>> +endif >>> + >>> endif # ACPI >>> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile >>> index 251ce85..1a94ff7 100644 >>> --- a/drivers/acpi/Makefile >>> +++ b/drivers/acpi/Makefile >>> @@ -99,5 +99,6 @@ obj-$(CONFIG_ACPI_EXTLOG) += acpi_extlog.o >>> obj-$(CONFIG_PMIC_OPREGION) += pmic/intel_pmic.o >>> obj-$(CONFIG_CRC_PMIC_OPREGION) += pmic/intel_pmic_crc.o >>> obj-$(CONFIG_XPOWER_PMIC_OPREGION) += pmic/intel_pmic_xpower.o >>> +obj-$(CONFIG_ARM64) += arm64/ >>> >>> video-objs += acpi_video.o video_detect.o >>> diff --git a/drivers/acpi/arm64/Kconfig b/drivers/acpi/arm64/Kconfig >>> new file mode 100644 >>> index 0000000..ff5c253 >>> --- /dev/null >>> +++ b/drivers/acpi/arm64/Kconfig >>> @@ -0,0 +1,15 @@ >>> +# >>> +# ACPI Configuration for ARM64 >>> +# >>> + >>> +menu "The ARM64-specific ACPI Support" >>> + >>> +config ACPI_GTDT >>> + bool "ACPI GTDT table Support" >> >> This should depend on ARM64. >> >> Also I wonder if it needs to be user-selectable? Wouldn't it be >> better to enable it by default when building for ARM64 with ACPI? >> >>> + help >>> + GTDT (Generic Timer Description Table) provides information >>> + for per-processor timers and Platform (memory-mapped) timers >>> + for ARM platforms. Select this option to provide information >>> + needed for the timers init. >>> + >>> +endmenu >>> diff --git a/drivers/acpi/arm64/Makefile b/drivers/acpi/arm64/Makefile >>> new file mode 100644 >>> index 0000000..466de6b >>> --- /dev/null >>> +++ b/drivers/acpi/arm64/Makefile >>> @@ -0,0 +1 @@ >>> +obj-$(CONFIG_ACPI_GTDT) += acpi_gtdt.o >>> diff --git a/drivers/acpi/arm64/acpi_gtdt.c b/drivers/acpi/arm64/acpi_gtdt.c >>> new file mode 100644 >>> index 0000000..9ee977d >>> --- /dev/null >>> +++ b/drivers/acpi/arm64/acpi_gtdt.c >>> @@ -0,0 +1,170 @@ >>> +/* >>> + * ARM Specific GTDT table Support >>> + * >>> + * Copyright (C) 2016, Linaro Ltd. >>> + * Author: Daniel Lezcano <daniel.lezcano@linaro.org> >>> + * Fu Wei <fu.wei@linaro.org> >>> + * Hanjun Guo <hanjun.guo@linaro.org> >>> + * >>> + * This program is free software; you can redistribute it and/or modify >>> + * it under the terms of the GNU General Public License version 2 as >>> + * published by the Free Software Foundation. >>> + */ >>> + >>> +#include <linux/acpi.h> >>> +#include <linux/init.h> >>> +#include <linux/kernel.h> >>> +#include <linux/module.h> >>> + >>> +#include <clocksource/arm_arch_timer.h> >>> + >>> +#undef pr_fmt >>> +#define pr_fmt(fmt) "GTDT: " fmt >> >> I would add "ACPI" to the prefix too if I were you, but that's me. >> >>> + >>> +typedef struct { >>> + struct acpi_table_gtdt *gtdt; >>> + void *platform_timer_start; >>> + void *gtdt_end; >>> +} acpi_gtdt_desc_t; >>> + >>> +static acpi_gtdt_desc_t acpi_gtdt_desc __initdata; >>> + >>> +static inline void *next_platform_timer(void *platform_timer) >>> +{ >>> + struct acpi_gtdt_header *gh = platform_timer; >>> + >>> + platform_timer += gh->length; >>> + if (platform_timer < acpi_gtdt_desc.gtdt_end) >>> + return platform_timer; >>> + >>> + return NULL; >>> +} >>> + >>> +#define for_each_platform_timer(_g) \ >>> + for (_g = acpi_gtdt_desc.platform_timer_start; _g; \ >>> + _g = next_platform_timer(_g)) >>> + >>> +static inline bool is_timer_block(void *platform_timer) >>> +{ >>> + struct acpi_gtdt_header *gh = platform_timer; >>> + >>> + if (gh->type == ACPI_GTDT_TYPE_TIMER_BLOCK) >>> + return true; >>> + >>> + return false; >> >> This is just too much code. It would suffice to do >> >> return gh->type == ACPI_GTDT_TYPE_TIMER_BLOCK; >> >>> +} >>> + >>> +static inline bool is_watchdog(void *platform_timer) >>> +{ >>> + struct acpi_gtdt_header *gh = platform_timer; >>> + >>> + if (gh->type == ACPI_GTDT_TYPE_WATCHDOG) >>> + return true; >>> + >>> + return false; >> >> Just like above. >> >>> +} >>> + >>> +/* >>> + * Get some basic info from GTDT table, and init the global variables above >>> + * for all timers initialization of Generic Timer. >>> + * This function does some validation on GTDT table. >>> + */ >>> +static int __init acpi_gtdt_desc_init(struct acpi_table_header *table) >>> +{ >>> + struct acpi_table_gtdt *gtdt = container_of(table, >>> + struct acpi_table_gtdt, >>> + header); >>> + >>> + acpi_gtdt_desc.gtdt = gtdt; >>> + acpi_gtdt_desc.gtdt_end = (void *)table + table->length; >>> + >>> + if (table->revision < 2) { >>> + pr_info("Revision:%d doesn't support Platform Timers.\n", >>> + table->revision); >> >> Is it really useful to print this message (and the one below) at the >> "info" level? What about changing them to pr_debug()? >> >>> + return 0; >>> + } >>> + >>> + if (!gtdt->platform_timer_count) { >>> + pr_info("No Platform Timer.\n"); >>> + return 0; >>> + } >>> + >>> + acpi_gtdt_desc.platform_timer_start = (void *)gtdt + >>> + gtdt->platform_timer_offset; > > BTW, breaking lines this way doesn't make the code particularly more readable. > >>> + if (acpi_gtdt_desc.platform_timer_start < >>> + (void *)table + sizeof(struct acpi_table_gtdt)) { > > Same here. > > It might be avoided by using a local (void *) variable "start", ie. > > start = (void *)gtdt + gtdt->platform_timer_offset; > if (start < (void *)table + sizeof(struct acpi_table_gtdt))) { > print message; > return -EINVAL; > } > acpi_gtdt_desc.platform_timer_start = start; Pretty good idea, will do , thanks :-) > > Thanks, > Rafael
Hi Paul On 15 July 2016 at 04:33, Paul Gortmaker <paul.gortmaker@windriver.com> wrote: > On Wed, Jul 13, 2016 at 1:53 PM, <fu.wei@linaro.org> wrote: >> From: Fu Wei <fu.wei@linaro.org> >> >> This patch adds support for parsing arch timer in GTDT, >> provides some kernel APIs to parse all the PPIs and >> always-on info in GTDT and export them. >> >> By this driver, we can simplify arm_arch_timer drivers, and >> separate the ACPI GTDT knowledge from it. >> >> Signed-off-by: Fu Wei <fu.wei@linaro.org> >> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> >> --- >> drivers/acpi/Kconfig | 5 ++ >> drivers/acpi/Makefile | 1 + >> drivers/acpi/arm64/Kconfig | 15 ++++ >> drivers/acpi/arm64/Makefile | 1 + >> drivers/acpi/arm64/acpi_gtdt.c | 170 +++++++++++++++++++++++++++++++++++++++++ >> include/linux/acpi.h | 6 ++ >> 6 files changed, 198 insertions(+) >> >> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig >> index b7e2e77..1cdc7d2 100644 >> --- a/drivers/acpi/Kconfig >> +++ b/drivers/acpi/Kconfig >> @@ -521,4 +521,9 @@ config XPOWER_PMIC_OPREGION >> >> endif >> >> +if ARM64 >> +source "drivers/acpi/arm64/Kconfig" >> + >> +endif >> + >> endif # ACPI >> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile >> index 251ce85..1a94ff7 100644 >> --- a/drivers/acpi/Makefile >> +++ b/drivers/acpi/Makefile >> @@ -99,5 +99,6 @@ obj-$(CONFIG_ACPI_EXTLOG) += acpi_extlog.o >> obj-$(CONFIG_PMIC_OPREGION) += pmic/intel_pmic.o >> obj-$(CONFIG_CRC_PMIC_OPREGION) += pmic/intel_pmic_crc.o >> obj-$(CONFIG_XPOWER_PMIC_OPREGION) += pmic/intel_pmic_xpower.o >> +obj-$(CONFIG_ARM64) += arm64/ >> >> video-objs += acpi_video.o video_detect.o >> diff --git a/drivers/acpi/arm64/Kconfig b/drivers/acpi/arm64/Kconfig >> new file mode 100644 >> index 0000000..ff5c253 >> --- /dev/null >> +++ b/drivers/acpi/arm64/Kconfig >> @@ -0,0 +1,15 @@ >> +# >> +# ACPI Configuration for ARM64 >> +# >> + >> +menu "The ARM64-specific ACPI Support" >> + >> +config ACPI_GTDT >> + bool "ACPI GTDT table Support" >> + help >> + GTDT (Generic Timer Description Table) provides information >> + for per-processor timers and Platform (memory-mapped) timers >> + for ARM platforms. Select this option to provide information >> + needed for the timers init. >> + >> +endmenu >> diff --git a/drivers/acpi/arm64/Makefile b/drivers/acpi/arm64/Makefile >> new file mode 100644 >> index 0000000..466de6b >> --- /dev/null >> +++ b/drivers/acpi/arm64/Makefile >> @@ -0,0 +1 @@ >> +obj-$(CONFIG_ACPI_GTDT) += acpi_gtdt.o >> diff --git a/drivers/acpi/arm64/acpi_gtdt.c b/drivers/acpi/arm64/acpi_gtdt.c >> new file mode 100644 >> index 0000000..9ee977d >> --- /dev/null >> +++ b/drivers/acpi/arm64/acpi_gtdt.c >> @@ -0,0 +1,170 @@ >> +/* >> + * ARM Specific GTDT table Support >> + * >> + * Copyright (C) 2016, Linaro Ltd. >> + * Author: Daniel Lezcano <daniel.lezcano@linaro.org> >> + * Fu Wei <fu.wei@linaro.org> >> + * Hanjun Guo <hanjun.guo@linaro.org> >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 as >> + * published by the Free Software Foundation. >> + */ >> + >> +#include <linux/acpi.h> >> +#include <linux/init.h> >> +#include <linux/kernel.h> >> +#include <linux/module.h> > > Please do not use module.h in drivers that are using a > bool Kconfig setting. yes, you are right, I forget to delete it, sorry. Will do, thanks > > Thanks, > Paul.
On Friday, July 15, 2016 03:32:35 PM Fu Wei wrote: > Hi Rafael, > > > > On 14 July 2016 at 04:30, Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Wed, Jul 13, 2016 at 7:53 PM, <fu.wei@linaro.org> wrote: > >> From: Fu Wei <fu.wei@linaro.org> > >> > >> This patch adds support for parsing arch timer in GTDT, > >> provides some kernel APIs to parse all the PPIs and > >> always-on info in GTDT and export them. > >> > >> By this driver, we can simplify arm_arch_timer drivers, and > >> separate the ACPI GTDT knowledge from it. > >> > >> Signed-off-by: Fu Wei <fu.wei@linaro.org> > >> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> > >> --- > >> drivers/acpi/Kconfig | 5 ++ > >> drivers/acpi/Makefile | 1 + > >> drivers/acpi/arm64/Kconfig | 15 ++++ > >> drivers/acpi/arm64/Makefile | 1 + > >> drivers/acpi/arm64/acpi_gtdt.c | 170 +++++++++++++++++++++++++++++++++++++++++ > >> include/linux/acpi.h | 6 ++ > >> 6 files changed, 198 insertions(+) > >> > >> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig > >> index b7e2e77..1cdc7d2 100644 > >> --- a/drivers/acpi/Kconfig > >> +++ b/drivers/acpi/Kconfig > >> @@ -521,4 +521,9 @@ config XPOWER_PMIC_OPREGION > >> > >> endif > >> > >> +if ARM64 > >> +source "drivers/acpi/arm64/Kconfig" > >> + > >> +endif > >> + > >> endif # ACPI > >> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile > >> index 251ce85..1a94ff7 100644 > >> --- a/drivers/acpi/Makefile > >> +++ b/drivers/acpi/Makefile > >> @@ -99,5 +99,6 @@ obj-$(CONFIG_ACPI_EXTLOG) += acpi_extlog.o > >> obj-$(CONFIG_PMIC_OPREGION) += pmic/intel_pmic.o > >> obj-$(CONFIG_CRC_PMIC_OPREGION) += pmic/intel_pmic_crc.o > >> obj-$(CONFIG_XPOWER_PMIC_OPREGION) += pmic/intel_pmic_xpower.o > >> +obj-$(CONFIG_ARM64) += arm64/ > >> > >> video-objs += acpi_video.o video_detect.o > >> diff --git a/drivers/acpi/arm64/Kconfig b/drivers/acpi/arm64/Kconfig > >> new file mode 100644 > >> index 0000000..ff5c253 > >> --- /dev/null > >> +++ b/drivers/acpi/arm64/Kconfig > >> @@ -0,0 +1,15 @@ > >> +# > >> +# ACPI Configuration for ARM64 > >> +# > >> + > >> +menu "The ARM64-specific ACPI Support" > >> + > >> +config ACPI_GTDT > >> + bool "ACPI GTDT table Support" > > > > This should depend on ARM64. > > > > Also I wonder if it needs to be user-selectable? Wouldn't it be > > better to enable it by default when building for ARM64 with ACPI? > > > >> + help > >> + GTDT (Generic Timer Description Table) provides information > >> + for per-processor timers and Platform (memory-mapped) timers > >> + for ARM platforms. Select this option to provide information > >> + needed for the timers init. > >> + > >> +endmenu > >> diff --git a/drivers/acpi/arm64/Makefile b/drivers/acpi/arm64/Makefile > >> new file mode 100644 > >> index 0000000..466de6b > >> --- /dev/null > >> +++ b/drivers/acpi/arm64/Makefile > >> @@ -0,0 +1 @@ > >> +obj-$(CONFIG_ACPI_GTDT) += acpi_gtdt.o > >> diff --git a/drivers/acpi/arm64/acpi_gtdt.c b/drivers/acpi/arm64/acpi_gtdt.c > >> new file mode 100644 > >> index 0000000..9ee977d > >> --- /dev/null > >> +++ b/drivers/acpi/arm64/acpi_gtdt.c > >> @@ -0,0 +1,170 @@ > >> +/* > >> + * ARM Specific GTDT table Support > >> + * > >> + * Copyright (C) 2016, Linaro Ltd. > >> + * Author: Daniel Lezcano <daniel.lezcano@linaro.org> > >> + * Fu Wei <fu.wei@linaro.org> > >> + * Hanjun Guo <hanjun.guo@linaro.org> > >> + * > >> + * This program is free software; you can redistribute it and/or modify > >> + * it under the terms of the GNU General Public License version 2 as > >> + * published by the Free Software Foundation. > >> + */ > >> + > >> +#include <linux/acpi.h> > >> +#include <linux/init.h> > >> +#include <linux/kernel.h> > >> +#include <linux/module.h> > >> + > >> +#include <clocksource/arm_arch_timer.h> > >> + > >> +#undef pr_fmt > >> +#define pr_fmt(fmt) "GTDT: " fmt > > > > I would add "ACPI" to the prefix too if I were you, but that's me. > > good idea, you are right, will do > > > > >> + > >> +typedef struct { > >> + struct acpi_table_gtdt *gtdt; > >> + void *platform_timer_start; > >> + void *gtdt_end; > >> +} acpi_gtdt_desc_t; > >> + > >> +static acpi_gtdt_desc_t acpi_gtdt_desc __initdata; > >> + > >> +static inline void *next_platform_timer(void *platform_timer) > >> +{ > >> + struct acpi_gtdt_header *gh = platform_timer; > >> + > >> + platform_timer += gh->length; > >> + if (platform_timer < acpi_gtdt_desc.gtdt_end) > >> + return platform_timer; > >> + > >> + return NULL; > >> +} > >> + > >> +#define for_each_platform_timer(_g) \ > >> + for (_g = acpi_gtdt_desc.platform_timer_start; _g; \ > >> + _g = next_platform_timer(_g)) > >> + > >> +static inline bool is_timer_block(void *platform_timer) > >> +{ > >> + struct acpi_gtdt_header *gh = platform_timer; > >> + > >> + if (gh->type == ACPI_GTDT_TYPE_TIMER_BLOCK) > >> + return true; > >> + > >> + return false; > > > > This is just too much code. It would suffice to do > > > > return gh->type == ACPI_GTDT_TYPE_TIMER_BLOCK; > > > >> +} > >> + > >> +static inline bool is_watchdog(void *platform_timer) > >> +{ > >> + struct acpi_gtdt_header *gh = platform_timer; > >> + > >> + if (gh->type == ACPI_GTDT_TYPE_WATCHDOG) > >> + return true; > >> + > >> + return false; > > > > Just like above. > > > Thanks, this is better :-) will do > > > > > >> +} > >> + > >> +/* > >> + * Get some basic info from GTDT table, and init the global variables above > >> + * for all timers initialization of Generic Timer. > >> + * This function does some validation on GTDT table. > >> + */ > >> +static int __init acpi_gtdt_desc_init(struct acpi_table_header *table) > >> +{ > >> + struct acpi_table_gtdt *gtdt = container_of(table, > >> + struct acpi_table_gtdt, > >> + header); > >> + > >> + acpi_gtdt_desc.gtdt = gtdt; > >> + acpi_gtdt_desc.gtdt_end = (void *)table + table->length; > >> + > >> + if (table->revision < 2) { > >> + pr_info("Revision:%d doesn't support Platform Timers.\n", > >> + table->revision); > > > > Is it really useful to print this message (and the one below) at the > > "info" level? What about changing them to pr_debug()? > > yes, pr_debug is better, thanks :-) will do > > > > > >> + return 0; > >> + } > >> + > >> + if (!gtdt->platform_timer_count) { > >> + pr_info("No Platform Timer.\n"); > >> + return 0; > >> + } > >> + > >> + acpi_gtdt_desc.platform_timer_start = (void *)gtdt + > >> + gtdt->platform_timer_offset; > >> + if (acpi_gtdt_desc.platform_timer_start < > >> + (void *)table + sizeof(struct acpi_table_gtdt)) { > >> + pr_err(FW_BUG "Platform Timer pointer error.\n"); > > > > Why pr_err()? > > if (true), that means the GTDT table has bugs. > And that's not a very useful piece of information unless you're debugging the platform, is it? Thanks, Rafael
On Friday, July 15, 2016 02:15:27 PM Rafael J. Wysocki wrote: > On Friday, July 15, 2016 03:32:35 PM Fu Wei wrote: > > Hi Rafael, > > [cut] > > > > > >> + return 0; > > >> + } > > >> + > > >> + if (!gtdt->platform_timer_count) { > > >> + pr_info("No Platform Timer.\n"); > > >> + return 0; > > >> + } > > >> + > > >> + acpi_gtdt_desc.platform_timer_start = (void *)gtdt + > > >> + gtdt->platform_timer_offset; > > >> + if (acpi_gtdt_desc.platform_timer_start < > > >> + (void *)table + sizeof(struct acpi_table_gtdt)) { > > >> + pr_err(FW_BUG "Platform Timer pointer error.\n"); > > > > > > Why pr_err()? > > > > if (true), that means the GTDT table has bugs. > > > > And that's not a very useful piece of information unless you're debugging the > platform, is it? FWIW, I'm not a big fan of printing "your firmware is buggy" type of messages (especially at the "error" log level or higher) unless they can be clearly connected to a specific type of functional failure. So if you want to pring an error-level message, something like "I cannot do X because of the firmware bug Y" would be better IMO. Thanks, Rafael
Hi Rafael, On 15 July 2016 at 21:07, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > On Friday, July 15, 2016 02:15:27 PM Rafael J. Wysocki wrote: >> On Friday, July 15, 2016 03:32:35 PM Fu Wei wrote: >> > Hi Rafael, >> > > > [cut] > >> > > >> > >> + return 0; >> > >> + } >> > >> + >> > >> + if (!gtdt->platform_timer_count) { >> > >> + pr_info("No Platform Timer.\n"); >> > >> + return 0; >> > >> + } >> > >> + >> > >> + acpi_gtdt_desc.platform_timer_start = (void *)gtdt + >> > >> + gtdt->platform_timer_offset; >> > >> + if (acpi_gtdt_desc.platform_timer_start < >> > >> + (void *)table + sizeof(struct acpi_table_gtdt)) { >> > >> + pr_err(FW_BUG "Platform Timer pointer error.\n"); >> > > >> > > Why pr_err()? >> > >> > if (true), that means the GTDT table has bugs. >> > >> >> And that's not a very useful piece of information unless you're debugging the >> platform, is it? > > FWIW, I'm not a big fan of printing "your firmware is buggy" type of messages > (especially at the "error" log level or higher) unless they can be clearly > connected to a specific type of functional failure. > > So if you want to pring an error-level message, something like "I cannot do X > because of the firmware bug Y" would be better IMO. So can I do this: pr_err(FW_BUG "Can NOT init platform_timer pointer, because of the GTDT table bug\n"); or pr_debug(FW_BUG "Can NOT init platform_timer_start, because of platform_timer_offset bug in GTDT\n"); or just delete it? which one do you prefer? I think maybe should provide some clue for users to fix the problem :-) any thought ? > > Thanks, > Rafael >
On Saturday, July 16, 2016 12:32:14 AM Fu Wei wrote: > Hi Rafael, > > On 15 July 2016 at 21:07, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > On Friday, July 15, 2016 02:15:27 PM Rafael J. Wysocki wrote: > >> On Friday, July 15, 2016 03:32:35 PM Fu Wei wrote: > >> > Hi Rafael, > >> > > > > > [cut] > > > >> > > > >> > >> + return 0; > >> > >> + } > >> > >> + > >> > >> + if (!gtdt->platform_timer_count) { > >> > >> + pr_info("No Platform Timer.\n"); > >> > >> + return 0; > >> > >> + } > >> > >> + > >> > >> + acpi_gtdt_desc.platform_timer_start = (void *)gtdt + > >> > >> + gtdt->platform_timer_offset; > >> > >> + if (acpi_gtdt_desc.platform_timer_start < > >> > >> + (void *)table + sizeof(struct acpi_table_gtdt)) { > >> > >> + pr_err(FW_BUG "Platform Timer pointer error.\n"); > >> > > > >> > > Why pr_err()? > >> > > >> > if (true), that means the GTDT table has bugs. > >> > > >> > >> And that's not a very useful piece of information unless you're debugging the > >> platform, is it? > > > > FWIW, I'm not a big fan of printing "your firmware is buggy" type of messages > > (especially at the "error" log level or higher) unless they can be clearly > > connected to a specific type of functional failure. > > > > So if you want to pring an error-level message, something like "I cannot do X > > because of the firmware bug Y" would be better IMO. > > So can I do this: > pr_err(FW_BUG "Can NOT init platform_timer pointer, because of the > GTDT table bug\n"); > > or pr_debug(FW_BUG "Can NOT init platform_timer_start, because of > platform_timer_offset bug in GTDT\n"); > > or just delete it? > > which one do you prefer? I think maybe should provide some clue for > users to fix the problem :-) And how exactly would they fix it then? > > any thought ? If you print variable or function names and the like, the message should be a debug one, because that's information that can only be understood by developers (some developers are users too, but they are a minority). If you want to report an error, say what is not working (or not available etc) and why (if you know the reason at the time the message is printed). Thanks, Rafael
Hi Rafeal, On 16 July 2016 at 05:22, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > On Saturday, July 16, 2016 12:32:14 AM Fu Wei wrote: >> Hi Rafael, >> >> On 15 July 2016 at 21:07, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: >> > On Friday, July 15, 2016 02:15:27 PM Rafael J. Wysocki wrote: >> >> On Friday, July 15, 2016 03:32:35 PM Fu Wei wrote: >> >> > Hi Rafael, >> >> > >> > >> > [cut] >> > >> >> > > >> >> > >> + return 0; >> >> > >> + } >> >> > >> + >> >> > >> + if (!gtdt->platform_timer_count) { >> >> > >> + pr_info("No Platform Timer.\n"); >> >> > >> + return 0; >> >> > >> + } >> >> > >> + >> >> > >> + acpi_gtdt_desc.platform_timer_start = (void *)gtdt + >> >> > >> + gtdt->platform_timer_offset; >> >> > >> + if (acpi_gtdt_desc.platform_timer_start < >> >> > >> + (void *)table + sizeof(struct acpi_table_gtdt)) { >> >> > >> + pr_err(FW_BUG "Platform Timer pointer error.\n"); >> >> > > >> >> > > Why pr_err()? >> >> > >> >> > if (true), that means the GTDT table has bugs. >> >> > >> >> >> >> And that's not a very useful piece of information unless you're debugging the >> >> platform, is it? >> > >> > FWIW, I'm not a big fan of printing "your firmware is buggy" type of messages >> > (especially at the "error" log level or higher) unless they can be clearly >> > connected to a specific type of functional failure. >> > >> > So if you want to pring an error-level message, something like "I cannot do X >> > because of the firmware bug Y" would be better IMO. >> >> So can I do this: >> pr_err(FW_BUG "Can NOT init platform_timer pointer, because of the >> GTDT table bug\n"); >> >> or pr_debug(FW_BUG "Can NOT init platform_timer_start, because of >> platform_timer_offset bug in GTDT\n"); >> >> or just delete it? >> >> which one do you prefer? I think maybe should provide some clue for >> users to fix the problem :-) > > And how exactly would they fix it then? > >> >> any thought ? > > If you print variable or function names and the like, the message should be > a debug one, because that's information that can only be understood by > developers (some developers are users too, but they are a minority). > > If you want to report an error, say what is not working (or not available > etc) and why (if you know the reason at the time the message is printed). Great thanks, I guess I got you point. maybe just a very simple message like: pr_err(FW_BUG "Failed to init table: GTDT table is buggy.\n"); I will also check other pr_* , if I can update them > > Thanks, > Rafael >
On Saturday, July 16, 2016 10:24:35 AM Fu Wei wrote: > Hi Rafeal, > > On 16 July 2016 at 05:22, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > On Saturday, July 16, 2016 12:32:14 AM Fu Wei wrote: > >> Hi Rafael, > >> > >> On 15 July 2016 at 21:07, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > >> > On Friday, July 15, 2016 02:15:27 PM Rafael J. Wysocki wrote: > >> >> On Friday, July 15, 2016 03:32:35 PM Fu Wei wrote: > >> >> > Hi Rafael, > >> >> > > >> > > >> > [cut] > >> > > >> >> > > > >> >> > >> + return 0; > >> >> > >> + } > >> >> > >> + > >> >> > >> + if (!gtdt->platform_timer_count) { > >> >> > >> + pr_info("No Platform Timer.\n"); > >> >> > >> + return 0; > >> >> > >> + } > >> >> > >> + > >> >> > >> + acpi_gtdt_desc.platform_timer_start = (void *)gtdt + > >> >> > >> + gtdt->platform_timer_offset; > >> >> > >> + if (acpi_gtdt_desc.platform_timer_start < > >> >> > >> + (void *)table + sizeof(struct acpi_table_gtdt)) { > >> >> > >> + pr_err(FW_BUG "Platform Timer pointer error.\n"); > >> >> > > > >> >> > > Why pr_err()? > >> >> > > >> >> > if (true), that means the GTDT table has bugs. > >> >> > > >> >> > >> >> And that's not a very useful piece of information unless you're debugging the > >> >> platform, is it? > >> > > >> > FWIW, I'm not a big fan of printing "your firmware is buggy" type of messages > >> > (especially at the "error" log level or higher) unless they can be clearly > >> > connected to a specific type of functional failure. > >> > > >> > So if you want to pring an error-level message, something like "I cannot do X > >> > because of the firmware bug Y" would be better IMO. > >> > >> So can I do this: > >> pr_err(FW_BUG "Can NOT init platform_timer pointer, because of the > >> GTDT table bug\n"); > >> > >> or pr_debug(FW_BUG "Can NOT init platform_timer_start, because of > >> platform_timer_offset bug in GTDT\n"); > >> > >> or just delete it? > >> > >> which one do you prefer? I think maybe should provide some clue for > >> users to fix the problem :-) > > > > And how exactly would they fix it then? > > > >> > >> any thought ? > > > > If you print variable or function names and the like, the message should be > > a debug one, because that's information that can only be understood by > > developers (some developers are users too, but they are a minority). > > > > If you want to report an error, say what is not working (or not available > > etc) and why (if you know the reason at the time the message is printed). > > Great thanks, I guess I got you point. > > maybe just a very simple message like: > pr_err(FW_BUG "Failed to init table: GTDT table is buggy.\n"); To understand this message one needs to know what "table" means here and what "GTDT" is. Also the prefix already will be something like "ACPI: GTDT:", so repeating part of it is not really useful IMO. Can you tell me please what's not going to work when that message is printed? Will the system boot at all then? If so, the functionality will be limited somehow I suppose. How is it going to be limited? > I will also check other pr_* , if I can update them OK, great! Thanks, Rafael
Hi Rafael, On 16 July 2016 at 20:35, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > On Saturday, July 16, 2016 10:24:35 AM Fu Wei wrote: >> Hi Rafeal, >> >> On 16 July 2016 at 05:22, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: >> > On Saturday, July 16, 2016 12:32:14 AM Fu Wei wrote: >> >> Hi Rafael, >> >> >> >> On 15 July 2016 at 21:07, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: >> >> > On Friday, July 15, 2016 02:15:27 PM Rafael J. Wysocki wrote: >> >> >> On Friday, July 15, 2016 03:32:35 PM Fu Wei wrote: >> >> >> > Hi Rafael, >> >> >> > >> >> > >> >> > [cut] >> >> > >> >> >> > > >> >> >> > >> + return 0; >> >> >> > >> + } >> >> >> > >> + >> >> >> > >> + if (!gtdt->platform_timer_count) { >> >> >> > >> + pr_info("No Platform Timer.\n"); >> >> >> > >> + return 0; >> >> >> > >> + } >> >> >> > >> + >> >> >> > >> + acpi_gtdt_desc.platform_timer_start = (void *)gtdt + >> >> >> > >> + gtdt->platform_timer_offset; >> >> >> > >> + if (acpi_gtdt_desc.platform_timer_start < >> >> >> > >> + (void *)table + sizeof(struct acpi_table_gtdt)) { >> >> >> > >> + pr_err(FW_BUG "Platform Timer pointer error.\n"); >> >> >> > > >> >> >> > > Why pr_err()? >> >> >> > >> >> >> > if (true), that means the GTDT table has bugs. >> >> >> > >> >> >> >> >> >> And that's not a very useful piece of information unless you're debugging the >> >> >> platform, is it? >> >> > >> >> > FWIW, I'm not a big fan of printing "your firmware is buggy" type of messages >> >> > (especially at the "error" log level or higher) unless they can be clearly >> >> > connected to a specific type of functional failure. >> >> > >> >> > So if you want to pring an error-level message, something like "I cannot do X >> >> > because of the firmware bug Y" would be better IMO. >> >> >> >> So can I do this: >> >> pr_err(FW_BUG "Can NOT init platform_timer pointer, because of the >> >> GTDT table bug\n"); >> >> >> >> or pr_debug(FW_BUG "Can NOT init platform_timer_start, because of >> >> platform_timer_offset bug in GTDT\n"); >> >> >> >> or just delete it? >> >> >> >> which one do you prefer? I think maybe should provide some clue for >> >> users to fix the problem :-) >> > >> > And how exactly would they fix it then? >> > >> >> >> >> any thought ? >> > >> > If you print variable or function names and the like, the message should be >> > a debug one, because that's information that can only be understood by >> > developers (some developers are users too, but they are a minority). >> > >> > If you want to report an error, say what is not working (or not available >> > etc) and why (if you know the reason at the time the message is printed). >> >> Great thanks, I guess I got you point. >> >> maybe just a very simple message like: >> pr_err(FW_BUG "Failed to init table: GTDT table is buggy.\n"); > > To understand this message one needs to know what "table" means here > and what "GTDT" is. Also the prefix already will be something like > "ACPI: GTDT:", so repeating part of it is not really useful IMO. > > Can you tell me please what's not going to work when that message is printed? > > Will the system boot at all then? If so, the functionality will be limited > somehow I suppose. How is it going to be limited? when that message is printed, all kind of platform timer(memory-mapped timer and SBSA watchdog) can't work. actually, I think system can boot without them. I have updated this on my v8(just posted), let's discuss this on v8 :-) > >> I will also check other pr_* , if I can update them > > OK, great! > > Thanks, > Rafael >
diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig index b7e2e77..1cdc7d2 100644 --- a/drivers/acpi/Kconfig +++ b/drivers/acpi/Kconfig @@ -521,4 +521,9 @@ config XPOWER_PMIC_OPREGION endif +if ARM64 +source "drivers/acpi/arm64/Kconfig" + +endif + endif # ACPI diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile index 251ce85..1a94ff7 100644 --- a/drivers/acpi/Makefile +++ b/drivers/acpi/Makefile @@ -99,5 +99,6 @@ obj-$(CONFIG_ACPI_EXTLOG) += acpi_extlog.o obj-$(CONFIG_PMIC_OPREGION) += pmic/intel_pmic.o obj-$(CONFIG_CRC_PMIC_OPREGION) += pmic/intel_pmic_crc.o obj-$(CONFIG_XPOWER_PMIC_OPREGION) += pmic/intel_pmic_xpower.o +obj-$(CONFIG_ARM64) += arm64/ video-objs += acpi_video.o video_detect.o diff --git a/drivers/acpi/arm64/Kconfig b/drivers/acpi/arm64/Kconfig new file mode 100644 index 0000000..ff5c253 --- /dev/null +++ b/drivers/acpi/arm64/Kconfig @@ -0,0 +1,15 @@ +# +# ACPI Configuration for ARM64 +# + +menu "The ARM64-specific ACPI Support" + +config ACPI_GTDT + bool "ACPI GTDT table Support" + help + GTDT (Generic Timer Description Table) provides information + for per-processor timers and Platform (memory-mapped) timers + for ARM platforms. Select this option to provide information + needed for the timers init. + +endmenu diff --git a/drivers/acpi/arm64/Makefile b/drivers/acpi/arm64/Makefile new file mode 100644 index 0000000..466de6b --- /dev/null +++ b/drivers/acpi/arm64/Makefile @@ -0,0 +1 @@ +obj-$(CONFIG_ACPI_GTDT) += acpi_gtdt.o diff --git a/drivers/acpi/arm64/acpi_gtdt.c b/drivers/acpi/arm64/acpi_gtdt.c new file mode 100644 index 0000000..9ee977d --- /dev/null +++ b/drivers/acpi/arm64/acpi_gtdt.c @@ -0,0 +1,170 @@ +/* + * ARM Specific GTDT table Support + * + * Copyright (C) 2016, Linaro Ltd. + * Author: Daniel Lezcano <daniel.lezcano@linaro.org> + * Fu Wei <fu.wei@linaro.org> + * Hanjun Guo <hanjun.guo@linaro.org> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#include <linux/acpi.h> +#include <linux/init.h> +#include <linux/kernel.h> +#include <linux/module.h> + +#include <clocksource/arm_arch_timer.h> + +#undef pr_fmt +#define pr_fmt(fmt) "GTDT: " fmt + +typedef struct { + struct acpi_table_gtdt *gtdt; + void *platform_timer_start; + void *gtdt_end; +} acpi_gtdt_desc_t; + +static acpi_gtdt_desc_t acpi_gtdt_desc __initdata; + +static inline void *next_platform_timer(void *platform_timer) +{ + struct acpi_gtdt_header *gh = platform_timer; + + platform_timer += gh->length; + if (platform_timer < acpi_gtdt_desc.gtdt_end) + return platform_timer; + + return NULL; +} + +#define for_each_platform_timer(_g) \ + for (_g = acpi_gtdt_desc.platform_timer_start; _g; \ + _g = next_platform_timer(_g)) + +static inline bool is_timer_block(void *platform_timer) +{ + struct acpi_gtdt_header *gh = platform_timer; + + if (gh->type == ACPI_GTDT_TYPE_TIMER_BLOCK) + return true; + + return false; +} + +static inline bool is_watchdog(void *platform_timer) +{ + struct acpi_gtdt_header *gh = platform_timer; + + if (gh->type == ACPI_GTDT_TYPE_WATCHDOG) + return true; + + return false; +} + +/* + * Get some basic info from GTDT table, and init the global variables above + * for all timers initialization of Generic Timer. + * This function does some validation on GTDT table. + */ +static int __init acpi_gtdt_desc_init(struct acpi_table_header *table) +{ + struct acpi_table_gtdt *gtdt = container_of(table, + struct acpi_table_gtdt, + header); + + acpi_gtdt_desc.gtdt = gtdt; + acpi_gtdt_desc.gtdt_end = (void *)table + table->length; + + if (table->revision < 2) { + pr_info("Revision:%d doesn't support Platform Timers.\n", + table->revision); + return 0; + } + + if (!gtdt->platform_timer_count) { + pr_info("No Platform Timer.\n"); + return 0; + } + + acpi_gtdt_desc.platform_timer_start = (void *)gtdt + + gtdt->platform_timer_offset; + if (acpi_gtdt_desc.platform_timer_start < + (void *)table + sizeof(struct acpi_table_gtdt)) { + pr_err(FW_BUG "Platform Timer pointer error.\n"); + acpi_gtdt_desc.platform_timer_start = NULL; + return -EINVAL; + } + + return gtdt->platform_timer_count; +} + +static int __init map_generic_timer_interrupt(u32 interrupt, u32 flags) +{ + int trigger, polarity; + + if (!interrupt) + return 0; + + trigger = (flags & ACPI_GTDT_INTERRUPT_MODE) ? ACPI_EDGE_SENSITIVE + : ACPI_LEVEL_SENSITIVE; + + polarity = (flags & ACPI_GTDT_INTERRUPT_POLARITY) ? ACPI_ACTIVE_LOW + : ACPI_ACTIVE_HIGH; + + return acpi_register_gsi(NULL, interrupt, trigger, polarity); +} + +/* + * Map the PPIs of per-cpu arch_timer. + * @type: the type of PPI + * Returns 0 if error. + */ +int __init acpi_gtdt_map_ppi(int type) +{ + struct acpi_table_gtdt *gtdt = acpi_gtdt_desc.gtdt; + + switch (type) { + case PHYS_SECURE_PPI: + return map_generic_timer_interrupt(gtdt->secure_el1_interrupt, + gtdt->secure_el1_flags); + case PHYS_NONSECURE_PPI: + return map_generic_timer_interrupt(gtdt->non_secure_el1_interrupt, + gtdt->non_secure_el1_flags); + case VIRT_PPI: + return map_generic_timer_interrupt(gtdt->virtual_timer_interrupt, + gtdt->virtual_timer_flags); + + case HYP_PPI: + return map_generic_timer_interrupt(gtdt->non_secure_el2_interrupt, + gtdt->non_secure_el2_flags); + default: + pr_err("ppi type error.\n"); + } + + return 0; +} + +/* + * acpi_gtdt_c3stop - got c3stop info from GTDT + * + * Returns 1 if the timer is powered in deep idle state, 0 otherwise. + */ +int __init acpi_gtdt_c3stop(void) +{ + struct acpi_table_gtdt *gtdt = acpi_gtdt_desc.gtdt; + + return !(gtdt->non_secure_el1_flags & ACPI_GTDT_ALWAYS_ON); +} + +int __init gtdt_arch_timer_init(struct acpi_table_header *table) +{ + if (table) + return acpi_gtdt_desc_init(table); + + pr_err("table pointer error.\n"); + + return -EINVAL; +} diff --git a/include/linux/acpi.h b/include/linux/acpi.h index 288fac5..8439579 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -532,6 +532,12 @@ void acpi_walk_dep_device_list(acpi_handle handle); struct platform_device *acpi_create_platform_device(struct acpi_device *); #define ACPI_PTR(_ptr) (_ptr) +#ifdef CONFIG_ACPI_GTDT +int __init gtdt_arch_timer_init(struct acpi_table_header *table); +int __init acpi_gtdt_map_ppi(int type); +int __init acpi_gtdt_c3stop(void); +#endif + #else /* !CONFIG_ACPI */ #define acpi_disabled 1