| Message ID | 20170321163122.9183-8-fu.wei@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | acpi, clocksource: add GTDT driver and GTDT support in arm_arch_timer | expand |
Hi Lorenzo, On 29 March 2017 at 18:21, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > On Wed, Mar 29, 2017 at 05:48:17PM +0800, Fu Wei wrote: > > [...] > >> * @platform_timer_count: It points to a integer variable which is used >> * for storing the number of platform timers. >> * This pointer could be NULL, if the caller >> * doesn't need this info. >> >> > >> >> + * >> >> + * Return: 0 if success, -EINVAL if error. >> >> + */ >> >> +int __init acpi_gtdt_init(struct acpi_table_header *table, >> >> + int *platform_timer_count) >> >> +{ >> >> + int ret = 0; >> >> + int timer_count = 0; >> >> + void *platform_timer = NULL; >> >> + struct acpi_table_gtdt *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_warn("Revision:%d doesn't support Platform Timers.\n", >> >> + table->revision); >> > >> > Ok, two points here. First, I am not sure why you should warn if the >> > table revision is < 2, is that a FW bug ? I do not think it is, you >> > can just return 0. >> >> I used pr_debug here before v20, then I got Hanjun's suggestion: >> ------- >> GTDT table revision is updated to 2 in ACPI 5.1, we will >> not support ACPI version under 5.1 and disable ACPI in FADT >> parse before this code is called, so if we get revision >> <2 here, I think we need to print warning (we need to keep >> the firmware stick to the spec on ARM64). >> ------- >> https://lkml.org/lkml/2017/1/19/82 >> >> So I started to use pr_warn. > > Thanks for the explanation, so it is a FW bug and the warning > is granted :) just leave it there. > > Still, please check my comment on acpi_gtdt_init() being called > multiple times on patch 11. Thanks For calling acpi_gtdt_init() twice: (1) 1st time: in early boot(bootmem), for init arch_timer and memory-mapped timer, we initialize the acpi_gtdt_desc. you can see that all the items in this struct are pointer. (2) 2nd time: when system switch from bootmem to slab, all the pointers in the acpi_gtdt_desc are invalid, so we have to re-initialize(re-map) them. I have tested it, if we don't re-initialize the acpi_gtdt_desc, system will go wrong. > > Thanks, > Lorenzo -- Best regards, Fu Wei Software Engineer Red Hat
On Wed, Mar 29, 2017 at 06:48:26PM +0800, Fu Wei wrote: > Hi Lorenzo, > > On 29 March 2017 at 18:21, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > > On Wed, Mar 29, 2017 at 05:48:17PM +0800, Fu Wei wrote: > > > > [...] > > > >> * @platform_timer_count: It points to a integer variable which is used > >> * for storing the number of platform timers. > >> * This pointer could be NULL, if the caller > >> * doesn't need this info. > >> > >> > > >> >> + * > >> >> + * Return: 0 if success, -EINVAL if error. > >> >> + */ > >> >> +int __init acpi_gtdt_init(struct acpi_table_header *table, > >> >> + int *platform_timer_count) > >> >> +{ > >> >> + int ret = 0; > >> >> + int timer_count = 0; > >> >> + void *platform_timer = NULL; > >> >> + struct acpi_table_gtdt *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_warn("Revision:%d doesn't support Platform Timers.\n", > >> >> + table->revision); > >> > > >> > Ok, two points here. First, I am not sure why you should warn if the > >> > table revision is < 2, is that a FW bug ? I do not think it is, you > >> > can just return 0. > >> > >> I used pr_debug here before v20, then I got Hanjun's suggestion: > >> ------- > >> GTDT table revision is updated to 2 in ACPI 5.1, we will > >> not support ACPI version under 5.1 and disable ACPI in FADT > >> parse before this code is called, so if we get revision > >> <2 here, I think we need to print warning (we need to keep > >> the firmware stick to the spec on ARM64). > >> ------- > >> https://lkml.org/lkml/2017/1/19/82 > >> > >> So I started to use pr_warn. > > > > Thanks for the explanation, so it is a FW bug and the warning > > is granted :) just leave it there. > > > > Still, please check my comment on acpi_gtdt_init() being called > > multiple times on patch 11. > > Thanks > > For calling acpi_gtdt_init() twice: > (1) 1st time: in early boot(bootmem), for init arch_timer and > memory-mapped timer, we initialize the acpi_gtdt_desc. > you can see that all the items in this struct are pointer. > (2) 2nd time: when system switch from bootmem to slab, all the > pointers in the acpi_gtdt_desc are invalid, so we have to > re-initialize(re-map) them. > > I have tested it, if we don't re-initialize the acpi_gtdt_desc, > system will go wrong. Ok, that's what I feared. My complaint on patch 11 is that: 1) Stashing the GTDT pointer in acpi_gtdt_desc is not needed to parse SBSA watchdogs 2) It is not clear at all from the code or the commit log _why_ you need to call acpi_gtdt_init() again (ie technically you don't need to call it - you grab a valid pointer to the table and parse the watchdogs in the _same_ function gtdt_sbsa_gwdt_init()) I do not think there is much you can do to improve the arch timer gtdt interface (and it is too late for that anyway) but in patch 11 it would be ideal if you avoid calling acpi_gtdt_init() again, just parse GTDT entries and initialize the corresponding watchdogs (ie pointers stashed in acpi_gtdt_desc are stale anyway but that's __initdata so I can live with that). You should add comments to summarize this issue so that it can be easily understood by anyone maintaining this code, it is not crystal clear by reading the code why you need to multiple acpi_gtdt_init() calls and that's not a piffling detail. The ACPI patches are fine with me otherwise, I will complete the review shortly. Thanks ! Lorenzo
Hi Lorenzo, On 29 March 2017 at 19:33, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > On Wed, Mar 29, 2017 at 06:48:26PM +0800, Fu Wei wrote: >> Hi Lorenzo, >> >> On 29 March 2017 at 18:21, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: >> > On Wed, Mar 29, 2017 at 05:48:17PM +0800, Fu Wei wrote: >> > >> > [...] >> > >> >> * @platform_timer_count: It points to a integer variable which is used >> >> * for storing the number of platform timers. >> >> * This pointer could be NULL, if the caller >> >> * doesn't need this info. >> >> >> >> > >> >> >> + * >> >> >> + * Return: 0 if success, -EINVAL if error. >> >> >> + */ >> >> >> +int __init acpi_gtdt_init(struct acpi_table_header *table, >> >> >> + int *platform_timer_count) >> >> >> +{ >> >> >> + int ret = 0; >> >> >> + int timer_count = 0; >> >> >> + void *platform_timer = NULL; >> >> >> + struct acpi_table_gtdt *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_warn("Revision:%d doesn't support Platform Timers.\n", >> >> >> + table->revision); >> >> > >> >> > Ok, two points here. First, I am not sure why you should warn if the >> >> > table revision is < 2, is that a FW bug ? I do not think it is, you >> >> > can just return 0. >> >> >> >> I used pr_debug here before v20, then I got Hanjun's suggestion: >> >> ------- >> >> GTDT table revision is updated to 2 in ACPI 5.1, we will >> >> not support ACPI version under 5.1 and disable ACPI in FADT >> >> parse before this code is called, so if we get revision >> >> <2 here, I think we need to print warning (we need to keep >> >> the firmware stick to the spec on ARM64). >> >> ------- >> >> https://lkml.org/lkml/2017/1/19/82 >> >> >> >> So I started to use pr_warn. >> > >> > Thanks for the explanation, so it is a FW bug and the warning >> > is granted :) just leave it there. >> > >> > Still, please check my comment on acpi_gtdt_init() being called >> > multiple times on patch 11. >> >> Thanks >> >> For calling acpi_gtdt_init() twice: >> (1) 1st time: in early boot(bootmem), for init arch_timer and >> memory-mapped timer, we initialize the acpi_gtdt_desc. >> you can see that all the items in this struct are pointer. >> (2) 2nd time: when system switch from bootmem to slab, all the >> pointers in the acpi_gtdt_desc are invalid, so we have to >> re-initialize(re-map) them. >> >> I have tested it, if we don't re-initialize the acpi_gtdt_desc, >> system will go wrong. > > Ok, that's what I feared. My complaint on patch 11 is that: > > 1) Stashing the GTDT pointer in acpi_gtdt_desc is not needed to > parse SBSA watchdogs The acpi_gtdt_desc is for sharing the info between acpi_gtdt_init and acpi_gtdt_c3stop, ;acpi_gtdt_map_ppi I re-use it in parsing SBSA watchdogs, because I try to re-use acpi_gtdt_init. > 2) It is not clear at all from the code or the commit log _why_ you > need to call acpi_gtdt_init() again (ie technically you don't need > to call it - you grab a valid pointer to the table and parse the > watchdogs in the _same_ function gtdt_sbsa_gwdt_init()) yes, we can avoid calling acpi_gtdt_init(), do the same thing in parsing SBSA watchdogs info. But if we will do the same thing(getting gtdt, platform_timer, timer_count), why not just re-using the same function? So my suggestion is that: Could we re-use acpi_gtdt_init, and a comment at the head of SBSA watchdogs info parsing function to summarize this issue? The comment like this Note: although the global variable acpi_gtdt_desc has been initialized by acpi_gtdt_init, when we initialized arch_timer. But when we call this function to get SBSA watchdogs info from GTDT, the system has switch from bootmem to slab, so the pointers in acpi_gtdt_desc are dated, we need to re-initialize(remap) them. So we call acpi_gtdt_init again here. Is that OK for you? :-) > > I do not think there is much you can do to improve the arch timer gtdt > interface (and it is too late for that anyway) but in patch 11 it would > be ideal if you avoid calling acpi_gtdt_init() again, just parse GTDT > entries and initialize the corresponding watchdogs (ie pointers stashed > in acpi_gtdt_desc are stale anyway but that's __initdata so I can live > with that). > > You should add comments to summarize this issue so that it can be > easily understood by anyone maintaining this code, it is not crystal > clear by reading the code why you need to multiple acpi_gtdt_init() > calls and that's not a piffling detail. > > The ACPI patches are fine with me otherwise, I will complete the > review shortly. > > Thanks ! > Lorenzo -- Best regards, Fu Wei Software Engineer Red Hat
On Wed, Mar 29, 2017 at 10:31:42PM +0800, Fu Wei wrote: > On 29 March 2017 at 22:29, Fu Wei <fu.wei@linaro.org> wrote: > > Hi Lorenzo, > > > > On 28 March 2017 at 19:35, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > >> On Wed, Mar 22, 2017 at 12:31:18AM +0800, fu.wei@linaro.org wrote: > >>> From: Fu Wei <fu.wei@linaro.org> > >>> > >>> This patch adds support for parsing arch timer info 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> > >>> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > >> > >> Acked-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > >> > >> Some nits below. > >> > >>> Tested-by: Xiongfeng Wang <wangxiongfeng2@huawei.com> > >>> Reviewed-by: Hanjun Guo <hanjun.guo@linaro.org> > >>> Tested-by: Hanjun Guo <hanjun.guo@linaro.org> > >>> --- > >>> arch/arm64/Kconfig | 1 + > >>> drivers/acpi/arm64/Kconfig | 3 + > >>> drivers/acpi/arm64/Makefile | 1 + > >>> drivers/acpi/arm64/gtdt.c | 157 ++++++++++++++++++++++++++++++++++++++++++++ > >>> include/linux/acpi.h | 6 ++ > >>> 5 files changed, 168 insertions(+) > >>> > >>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig > >>> index 3741859..7e2baec 100644 > >>> --- a/arch/arm64/Kconfig > >>> +++ b/arch/arm64/Kconfig > >>> @@ -2,6 +2,7 @@ config ARM64 > >>> def_bool y > >>> select ACPI_CCA_REQUIRED if ACPI > >>> select ACPI_GENERIC_GSI if ACPI > >>> + select ACPI_GTDT if ACPI > >>> select ACPI_REDUCED_HARDWARE_ONLY if ACPI > >>> select ACPI_MCFG if ACPI > >>> select ACPI_SPCR_TABLE if ACPI > >>> diff --git a/drivers/acpi/arm64/Kconfig b/drivers/acpi/arm64/Kconfig > >>> index 4616da4..5a6f80f 100644 > >>> --- a/drivers/acpi/arm64/Kconfig > >>> +++ b/drivers/acpi/arm64/Kconfig > >>> @@ -4,3 +4,6 @@ > >>> > >>> config ACPI_IORT > >>> bool > >>> + > >>> +config ACPI_GTDT > >>> + bool > >>> diff --git a/drivers/acpi/arm64/Makefile b/drivers/acpi/arm64/Makefile > >>> index 72331f2..1017def 100644 > >>> --- a/drivers/acpi/arm64/Makefile > >>> +++ b/drivers/acpi/arm64/Makefile > >>> @@ -1 +1,2 @@ > >>> obj-$(CONFIG_ACPI_IORT) += iort.o > >>> +obj-$(CONFIG_ACPI_GTDT) += gtdt.o > >>> diff --git a/drivers/acpi/arm64/gtdt.c b/drivers/acpi/arm64/gtdt.c > >>> new file mode 100644 > >>> index 0000000..8a03b4b > >>> --- /dev/null > >>> +++ b/drivers/acpi/arm64/gtdt.c > >>> @@ -0,0 +1,157 @@ > >>> +/* > >>> + * 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 <clocksource/arm_arch_timer.h> > >>> + > >>> +#undef pr_fmt > >>> +#define pr_fmt(fmt) "ACPI GTDT: " fmt > >>> + > >>> +/** > >>> + * struct acpi_gtdt_descriptor - Store the key info of GTDT for all functions > >>> + * @gtdt: The pointer to the struct acpi_table_gtdt of GTDT table. > >>> + * @gtdt_end: The pointer to the end of GTDT table. > >>> + * @platform_timer: The pointer to the start of Platform Timer Structure > >>> + * > >>> + * The struct store the key info of GTDT table, it should be initialized by > >>> + * acpi_gtdt_init. > >>> + */ > >>> +struct acpi_gtdt_descriptor { > >>> + struct acpi_table_gtdt *gtdt; > >>> + void *gtdt_end; > >>> + void *platform_timer; > >>> +}; > >>> + > >>> +static struct acpi_gtdt_descriptor acpi_gtdt_desc __initdata; > >>> + > >>> +static int __init map_gt_gsi(u32 interrupt, u32 flags) > >>> +{ > >>> + int trigger, polarity; > >>> + > >>> + 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); > >>> +} > >>> + > >>> +/** > >>> + * acpi_gtdt_map_ppi() - Map the PPIs of per-cpu arch_timer. > >>> + * @type: the type of PPI. > >>> + * > >>> + * Note: Linux on arm64 isn't supported on the secure side. > >> > >> Note: Secure state is not managed by the kernel on ARM64 systems. > >> > >> Is that what you wanted to say ? > >> > >>> + * So we only handle the non-secure timer PPIs, > >>> + * ARCH_TIMER_PHYS_SECURE_PPI is treated as invalid type. > >>> + * > >>> + * Return: the mapped PPI value, 0 if error. > >>> + */ > >>> +int __init acpi_gtdt_map_ppi(int type) > >>> +{ > >>> + struct acpi_table_gtdt *gtdt = acpi_gtdt_desc.gtdt; > >>> + > >>> + switch (type) { > >>> + case ARCH_TIMER_PHYS_NONSECURE_PPI: > >>> + return map_gt_gsi(gtdt->non_secure_el1_interrupt, > >>> + gtdt->non_secure_el1_flags); > >>> + case ARCH_TIMER_VIRT_PPI: > >>> + return map_gt_gsi(gtdt->virtual_timer_interrupt, > >>> + gtdt->virtual_timer_flags); > >>> + > >>> + case ARCH_TIMER_HYP_PPI: > >>> + return map_gt_gsi(gtdt->non_secure_el2_interrupt, > >>> + gtdt->non_secure_el2_flags); > >>> + default: > >>> + pr_err("Failed to map timer interrupt: invalid type.\n"); > >>> + } > >>> + > >>> + return 0; > >>> +} > >>> + > >>> +/** > >>> + * acpi_gtdt_c3stop() - Got c3stop info from GTDT according to the type of PPI. > >>> + * @type: the type of PPI. > >>> + * > >>> + * Return: 1 if the timer can be in deep idle state, 0 otherwise. > >> > >> Return: true if the timer HW state is lost when a CPU enters an idle > >> state, false otherwise > >> > >>> + */ > >>> +bool __init acpi_gtdt_c3stop(int type) > >>> +{ > >>> + struct acpi_table_gtdt *gtdt = acpi_gtdt_desc.gtdt; > >>> + > >>> + switch (type) { > >>> + case ARCH_TIMER_PHYS_NONSECURE_PPI: > >>> + return !(gtdt->non_secure_el1_flags & ACPI_GTDT_ALWAYS_ON); > >>> + > >>> + case ARCH_TIMER_VIRT_PPI: > >>> + return !(gtdt->virtual_timer_flags & ACPI_GTDT_ALWAYS_ON); > >>> + > >>> + case ARCH_TIMER_HYP_PPI: > >>> + return !(gtdt->non_secure_el2_flags & ACPI_GTDT_ALWAYS_ON); > >>> + > >>> + default: > >>> + pr_err("Failed to get c3stop info: invalid type.\n"); > >>> + } > >>> + > >>> + return 0; > >>> +} > >>> + > >>> +/** > >>> + * acpi_gtdt_init() - Get the info of GTDT table to prepare for further init. > >>> + * @table: The pointer to GTDT table. > >>> + * @platform_timer_count: The pointer of int variate for returning the > >> > >> I do not understand what this means. > >> > >>> + * number of platform timers. It can be NULL, if > >>> + * driver don't need this info. > >> > >> driver doesn't > >> > >>> + * > >>> + * Return: 0 if success, -EINVAL if error. > >>> + */ > >>> +int __init acpi_gtdt_init(struct acpi_table_header *table, > >>> + int *platform_timer_count) > >>> +{ > >>> + int ret = 0; > >>> + int timer_count = 0; > >>> + void *platform_timer = NULL; > >>> + struct acpi_table_gtdt *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_warn("Revision:%d doesn't support Platform Timers.\n", > >>> + table->revision); > >> > >> Ok, two points here. First, I am not sure why you should warn if the > >> table revision is < 2, is that a FW bug ? I do not think it is, you > >> can just return 0. > >> > >>> + else if (!gtdt->platform_timer_count) > >>> + pr_debug("No Platform Timer.\n"); > >> > >> Ditto. Why keep executing ? There is nothing to do, just bail out > >> with a return 0. > >> > >>> + else > >>> + timer_count = gtdt->platform_timer_count; > >>> + > >>> + if (timer_count) { > >>> + platform_timer = (void *)gtdt + gtdt->platform_timer_offset; > >>> + if (platform_timer < (void *)table + > >>> + sizeof(struct acpi_table_gtdt)) { > >>> + pr_err(FW_BUG "invalid timer data.\n"); > >>> + timer_count = 0; > >>> + platform_timer = NULL; > >>> + ret = -EINVAL; > >> > >> Same here, I suspect you want to consolidate the return path (I missed > >> previous rounds of reviews so you should not worry too much, I can clean > >> this up later) but to me a: > >> > >> return -EINVAL; > >> > >> would just do. > > > > For this problem, Can I do this: > > > > /** > > * acpi_gtdt_init() - Get the info of GTDT table to prepare for further init. > > * @table: The pointer to GTDT table. > > * @platform_timer_count: It points to a integer variable which is used > > * for storing the number of platform timers. > > * This pointer could be NULL, if the caller > > * doesn't need this info. > > * > > * Return: 0 if success, -EINVAL if error. > > */ > > int __init acpi_gtdt_init(struct acpi_table_header *table, > > int *platform_timer_count) > > { > > void *platform_timer; > > struct acpi_table_gtdt *gtdt; > > > > gtdt = container_of(table, struct acpi_table_gtdt, header); > > acpi_gtdt_desc.gtdt = gtdt; > > acpi_gtdt_desc.gtdt_end = (void *)table + table->length; > > acpi_gtdt_desc.platform_timer = NULL; > > if (platform_timer_count) > > *platform_timer_count = 0; > > > > if (table->revision < 2) { > > pr_warn("Revision:%d doesn't support Platform Timers.\n", > > table->revision); > > return 0; > > } > > > > if (!gtdt->platform_timer_count) { > > pr_debug("No Platform Timer.\n"); > > return 0; > > } > > if (platform_timer_count) > > *platform_timer_count = gtdt->platform_timer_count; > > sorry , this should be moved, > > > > > > platform_timer = (void *)gtdt + gtdt->platform_timer_offset; > > if (platform_timer < (void *)table + sizeof(struct acpi_table_gtdt)) { > > pr_err(FW_BUG "invalid timer data.\n"); > > return -EINVAL; > > } > > acpi_gtdt_desc.platform_timer = platform_timer; > > if (platform_timer_count) > *platform_timer_count = gtdt->platform_timer_count; > > Here is the right place Ok, to avoid adding bugs to code that has been tested already keep the current function version (as of v22) and send me a patch to clean this up at -rc1. Multiple calls to acpi_gtdt_init() were my main concern. Thanks, Lorenzo > > > > > return 0; > > } > > > > > >> > >> Lorenzo > >> > >>> + } > >>> + } > >>> + > >>> + acpi_gtdt_desc.platform_timer = platform_timer; > >>> + if (platform_timer_count) > >>> + *platform_timer_count = timer_count; > >>> + > >>> + return ret; > >>> +} > >>> diff --git a/include/linux/acpi.h b/include/linux/acpi.h > >>> index 9b05886..4b5c146 100644 > >>> --- a/include/linux/acpi.h > >>> +++ b/include/linux/acpi.h > >>> @@ -595,6 +595,12 @@ enum acpi_reconfig_event { > >>> int acpi_reconfig_notifier_register(struct notifier_block *nb); > >>> int acpi_reconfig_notifier_unregister(struct notifier_block *nb); > >>> > >>> +#ifdef CONFIG_ACPI_GTDT > >>> +int acpi_gtdt_init(struct acpi_table_header *table, int *platform_timer_count); > >>> +int acpi_gtdt_map_ppi(int type); > >>> +bool acpi_gtdt_c3stop(int type); > >>> +#endif > >>> + > >>> #else /* !CONFIG_ACPI */ > >>> > >>> #define acpi_disabled 1 > >>> -- > >>> 2.9.3 > >>> > > > > > > > > -- > > Best regards, > > > > Fu Wei > > Software Engineer > > Red Hat > > > > -- > Best regards, > > Fu Wei > Software Engineer > Red Hat
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index 3741859..7e2baec 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -2,6 +2,7 @@ config ARM64 def_bool y select ACPI_CCA_REQUIRED if ACPI select ACPI_GENERIC_GSI if ACPI + select ACPI_GTDT if ACPI select ACPI_REDUCED_HARDWARE_ONLY if ACPI select ACPI_MCFG if ACPI select ACPI_SPCR_TABLE if ACPI diff --git a/drivers/acpi/arm64/Kconfig b/drivers/acpi/arm64/Kconfig index 4616da4..5a6f80f 100644 --- a/drivers/acpi/arm64/Kconfig +++ b/drivers/acpi/arm64/Kconfig @@ -4,3 +4,6 @@ config ACPI_IORT bool + +config ACPI_GTDT + bool diff --git a/drivers/acpi/arm64/Makefile b/drivers/acpi/arm64/Makefile index 72331f2..1017def 100644 --- a/drivers/acpi/arm64/Makefile +++ b/drivers/acpi/arm64/Makefile @@ -1 +1,2 @@ obj-$(CONFIG_ACPI_IORT) += iort.o +obj-$(CONFIG_ACPI_GTDT) += gtdt.o diff --git a/drivers/acpi/arm64/gtdt.c b/drivers/acpi/arm64/gtdt.c new file mode 100644 index 0000000..8a03b4b --- /dev/null +++ b/drivers/acpi/arm64/gtdt.c @@ -0,0 +1,157 @@ +/* + * 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 <clocksource/arm_arch_timer.h> + +#undef pr_fmt +#define pr_fmt(fmt) "ACPI GTDT: " fmt + +/** + * struct acpi_gtdt_descriptor - Store the key info of GTDT for all functions + * @gtdt: The pointer to the struct acpi_table_gtdt of GTDT table. + * @gtdt_end: The pointer to the end of GTDT table. + * @platform_timer: The pointer to the start of Platform Timer Structure + * + * The struct store the key info of GTDT table, it should be initialized by + * acpi_gtdt_init. + */ +struct acpi_gtdt_descriptor { + struct acpi_table_gtdt *gtdt; + void *gtdt_end; + void *platform_timer; +}; + +static struct acpi_gtdt_descriptor acpi_gtdt_desc __initdata; + +static int __init map_gt_gsi(u32 interrupt, u32 flags) +{ + int trigger, polarity; + + 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); +} + +/** + * acpi_gtdt_map_ppi() - Map the PPIs of per-cpu arch_timer. + * @type: the type of PPI. + * + * Note: Linux on arm64 isn't supported on the secure side. + * So we only handle the non-secure timer PPIs, + * ARCH_TIMER_PHYS_SECURE_PPI is treated as invalid type. + * + * Return: the mapped PPI value, 0 if error. + */ +int __init acpi_gtdt_map_ppi(int type) +{ + struct acpi_table_gtdt *gtdt = acpi_gtdt_desc.gtdt; + + switch (type) { + case ARCH_TIMER_PHYS_NONSECURE_PPI: + return map_gt_gsi(gtdt->non_secure_el1_interrupt, + gtdt->non_secure_el1_flags); + case ARCH_TIMER_VIRT_PPI: + return map_gt_gsi(gtdt->virtual_timer_interrupt, + gtdt->virtual_timer_flags); + + case ARCH_TIMER_HYP_PPI: + return map_gt_gsi(gtdt->non_secure_el2_interrupt, + gtdt->non_secure_el2_flags); + default: + pr_err("Failed to map timer interrupt: invalid type.\n"); + } + + return 0; +} + +/** + * acpi_gtdt_c3stop() - Got c3stop info from GTDT according to the type of PPI. + * @type: the type of PPI. + * + * Return: 1 if the timer can be in deep idle state, 0 otherwise. + */ +bool __init acpi_gtdt_c3stop(int type) +{ + struct acpi_table_gtdt *gtdt = acpi_gtdt_desc.gtdt; + + switch (type) { + case ARCH_TIMER_PHYS_NONSECURE_PPI: + return !(gtdt->non_secure_el1_flags & ACPI_GTDT_ALWAYS_ON); + + case ARCH_TIMER_VIRT_PPI: + return !(gtdt->virtual_timer_flags & ACPI_GTDT_ALWAYS_ON); + + case ARCH_TIMER_HYP_PPI: + return !(gtdt->non_secure_el2_flags & ACPI_GTDT_ALWAYS_ON); + + default: + pr_err("Failed to get c3stop info: invalid type.\n"); + } + + return 0; +} + +/** + * acpi_gtdt_init() - Get the info of GTDT table to prepare for further init. + * @table: The pointer to GTDT table. + * @platform_timer_count: The pointer of int variate for returning the + * number of platform timers. It can be NULL, if + * driver don't need this info. + * + * Return: 0 if success, -EINVAL if error. + */ +int __init acpi_gtdt_init(struct acpi_table_header *table, + int *platform_timer_count) +{ + int ret = 0; + int timer_count = 0; + void *platform_timer = NULL; + struct acpi_table_gtdt *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_warn("Revision:%d doesn't support Platform Timers.\n", + table->revision); + else if (!gtdt->platform_timer_count) + pr_debug("No Platform Timer.\n"); + else + timer_count = gtdt->platform_timer_count; + + if (timer_count) { + platform_timer = (void *)gtdt + gtdt->platform_timer_offset; + if (platform_timer < (void *)table + + sizeof(struct acpi_table_gtdt)) { + pr_err(FW_BUG "invalid timer data.\n"); + timer_count = 0; + platform_timer = NULL; + ret = -EINVAL; + } + } + + acpi_gtdt_desc.platform_timer = platform_timer; + if (platform_timer_count) + *platform_timer_count = timer_count; + + return ret; +} diff --git a/include/linux/acpi.h b/include/linux/acpi.h index 9b05886..4b5c146 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -595,6 +595,12 @@ enum acpi_reconfig_event { int acpi_reconfig_notifier_register(struct notifier_block *nb); int acpi_reconfig_notifier_unregister(struct notifier_block *nb); +#ifdef CONFIG_ACPI_GTDT +int acpi_gtdt_init(struct acpi_table_header *table, int *platform_timer_count); +int acpi_gtdt_map_ppi(int type); +bool acpi_gtdt_c3stop(int type); +#endif + #else /* !CONFIG_ACPI */ #define acpi_disabled 1