| Message ID | 1475086637-1914-5-git-send-email-fu.wei@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Hi, As a heads-up, on v4.9-rc1 I see conflicts at least against arch/arm64/Kconfig. Luckily git am -3 seems to be able to fix that up automatically, but this will need to be rebased before the next posting and/or merging. On Thu, Sep 29, 2016 at 02:17:12AM +0800, fu.wei@linaro.org wrote: > +static int __init map_gt_gsi(u32 interrupt, u32 flags) > +{ > + int trigger, polarity; > + > + if (!interrupt) > + return 0; Urgh. Only the secure interrupt (which we do not need) is optional in this manner, and (hilariously), zero appears to also be a valid GSIV, per figure 5-24 in the ACPI 6.1 spec. So, I think that: (a) we should not bother parsing the secure interrupt (b) we should drop the check above (c) we should report the spec issue to the ASWG > +/* > + * acpi_gtdt_c3stop - got c3stop info from GTDT > + * > + * Returns 1 if the timer is powered in deep idle state, 0 otherwise. > + */ > +bool __init acpi_gtdt_c3stop(void) > +{ > + struct acpi_table_gtdt *gtdt = acpi_gtdt_desc.gtdt; > + > + return !(gtdt->non_secure_el1_flags & ACPI_GTDT_ALWAYS_ON); > +} It looks like this can differ per interrupt. Shouldn't we check the appropriate one? > +int __init acpi_gtdt_init(struct acpi_table_header *table) > +{ > + void *start; > + 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_debug("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; > + } > + > + start = (void *)gtdt + gtdt->platform_timer_offset; > + if (start < (void *)table + sizeof(struct acpi_table_gtdt)) { > + pr_err(FW_BUG "Failed to retrieve timer info from firmware: invalid data.\n"); > + return -EINVAL; > + } > + acpi_gtdt_desc.platform_timer_start = start; > + > + return gtdt->platform_timer_count; > +} This is never used as anything other than a status code. Just return zero; we haven't parsed the platform timers themselves at this point anyway. Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Mark, On 21 October 2016 at 00:37, Mark Rutland <mark.rutland@arm.com> wrote: > Hi, > > As a heads-up, on v4.9-rc1 I see conflicts at least against > arch/arm64/Kconfig. Luckily git am -3 seems to be able to fix that up > automatically, but this will need to be rebased before the next posting > and/or merging. > > On Thu, Sep 29, 2016 at 02:17:12AM +0800, fu.wei@linaro.org wrote: >> +static int __init map_gt_gsi(u32 interrupt, u32 flags) >> +{ >> + int trigger, polarity; >> + >> + if (!interrupt) >> + return 0; > > Urgh. > > Only the secure interrupt (which we do not need) is optional in this > manner, and (hilariously), zero appears to also be a valid GSIV, per > figure 5-24 in the ACPI 6.1 spec. > > So, I think that: > > (a) we should not bother parsing the secure interrupt If I understand correctly, from this point of view, kernel don't handle the secure interrupt. But the current arm_arch_timer driver still enable/disable/request PHYS_SECURE_PPI with PHYS_NONSECURE_PPI. That means we still need to parse the secure interrupt. Please correct me, if I misunderstand something? :-) > (b) we should drop the check above yes, if zero is a valid GSIV, this makes sense. > (c) we should report the spec issue to the ASWG > >> +/* >> + * acpi_gtdt_c3stop - got c3stop info from GTDT >> + * >> + * Returns 1 if the timer is powered in deep idle state, 0 otherwise. >> + */ >> +bool __init acpi_gtdt_c3stop(void) >> +{ >> + struct acpi_table_gtdt *gtdt = acpi_gtdt_desc.gtdt; >> + >> + return !(gtdt->non_secure_el1_flags & ACPI_GTDT_ALWAYS_ON); >> +} > > It looks like this can differ per interrupt. Shouldn't we check the > appropriate one? yes, I think you are right. > >> +int __init acpi_gtdt_init(struct acpi_table_header *table) >> +{ >> + void *start; >> + 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_debug("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; >> + } >> + >> + start = (void *)gtdt + gtdt->platform_timer_offset; >> + if (start < (void *)table + sizeof(struct acpi_table_gtdt)) { >> + pr_err(FW_BUG "Failed to retrieve timer info from firmware: invalid data.\n"); >> + return -EINVAL; >> + } >> + acpi_gtdt_desc.platform_timer_start = start; >> + >> + return gtdt->platform_timer_count; >> +} > > This is never used as anything other than a status code. > > Just return zero; we haven't parsed the platform timers themselves at > this point anyway. Sorry, in my driver, I use this return value to inform driver negative number : error 0 : we don't have platform timer in GTDT table. positive number: the number of platform timer we have in GTDT table. please correct me, if I am doing it in wrong way. Thanks :-) > > Thanks, > Mark.
On 26/10/16 12:10, Fu Wei wrote: > Hi Mark, > > On 21 October 2016 at 00:37, Mark Rutland <mark.rutland@arm.com> wrote: >> Hi, >> >> As a heads-up, on v4.9-rc1 I see conflicts at least against >> arch/arm64/Kconfig. Luckily git am -3 seems to be able to fix that up >> automatically, but this will need to be rebased before the next posting >> and/or merging. >> >> On Thu, Sep 29, 2016 at 02:17:12AM +0800, fu.wei@linaro.org wrote: >>> +static int __init map_gt_gsi(u32 interrupt, u32 flags) >>> +{ >>> + int trigger, polarity; >>> + >>> + if (!interrupt) >>> + return 0; >> >> Urgh. >> >> Only the secure interrupt (which we do not need) is optional in this >> manner, and (hilariously), zero appears to also be a valid GSIV, per >> figure 5-24 in the ACPI 6.1 spec. >> >> So, I think that: >> >> (a) we should not bother parsing the secure interrupt > > If I understand correctly, from this point of view, kernel don't > handle the secure interrupt. > But the current arm_arch_timer driver still enable/disable/request > PHYS_SECURE_PPI > with PHYS_NONSECURE_PPI. > That means we still need to parse the secure interrupt. > Please correct me, if I misunderstand something? :-) That's because we can use the per-cpu timer when 32bit Linux is running on the secure side (and we cannot distinguish between secure and non-secure at runtime). ACPI is 64bit only, and Linux on 64bit isn't supported on the secure side, so only registering the non-secure timer is perfectly acceptable. Thanks, M.
Hi Marc, On 26 October 2016 at 20:11, Marc Zyngier <marc.zyngier@arm.com> wrote: > On 26/10/16 12:10, Fu Wei wrote: >> Hi Mark, >> >> On 21 October 2016 at 00:37, Mark Rutland <mark.rutland@arm.com> wrote: >>> Hi, >>> >>> As a heads-up, on v4.9-rc1 I see conflicts at least against >>> arch/arm64/Kconfig. Luckily git am -3 seems to be able to fix that up >>> automatically, but this will need to be rebased before the next posting >>> and/or merging. >>> >>> On Thu, Sep 29, 2016 at 02:17:12AM +0800, fu.wei@linaro.org wrote: >>>> +static int __init map_gt_gsi(u32 interrupt, u32 flags) >>>> +{ >>>> + int trigger, polarity; >>>> + >>>> + if (!interrupt) >>>> + return 0; >>> >>> Urgh. >>> >>> Only the secure interrupt (which we do not need) is optional in this >>> manner, and (hilariously), zero appears to also be a valid GSIV, per >>> figure 5-24 in the ACPI 6.1 spec. >>> >>> So, I think that: >>> >>> (a) we should not bother parsing the secure interrupt >> >> If I understand correctly, from this point of view, kernel don't >> handle the secure interrupt. >> But the current arm_arch_timer driver still enable/disable/request >> PHYS_SECURE_PPI >> with PHYS_NONSECURE_PPI. >> That means we still need to parse the secure interrupt. >> Please correct me, if I misunderstand something? :-) > > That's because we can use the per-cpu timer when 32bit Linux is running > on the secure side (and we cannot distinguish between secure and > non-secure at runtime). ACPI is 64bit only, and Linux on 64bit isn't > supported on the secure side, so only registering the non-secure timer > is perfectly acceptable. Great thanks for your explanation :-) So we just don't need to fill arch_timer_ppi[PHYS_SECURE_PPI] , just skip it. > > Thanks, > > M. > -- > Jazz is not dead. It just smells funny...
On 10/26/2016 07:10 PM, Fu Wei wrote: > Hi Mark, > > On 21 October 2016 at 00:37, Mark Rutland <mark.rutland@arm.com> wrote: >> Hi, >> >> As a heads-up, on v4.9-rc1 I see conflicts at least against >> arch/arm64/Kconfig. Luckily git am -3 seems to be able to fix that up >> automatically, but this will need to be rebased before the next posting >> and/or merging. >> >> On Thu, Sep 29, 2016 at 02:17:12AM +0800, fu.wei@linaro.org wrote: >>> +static int __init map_gt_gsi(u32 interrupt, u32 flags) >>> +{ >>> + int trigger, polarity; >>> + >>> + if (!interrupt) >>> + return 0; >> >> Urgh. >> >> Only the secure interrupt (which we do not need) is optional in this >> manner, and (hilariously), zero appears to also be a valid GSIV, per >> figure 5-24 in the ACPI 6.1 spec. >> >> So, I think that: >> >> (a) we should not bother parsing the secure interrupt > > If I understand correctly, from this point of view, kernel don't > handle the secure interrupt. > But the current arm_arch_timer driver still enable/disable/request > PHYS_SECURE_PPI > with PHYS_NONSECURE_PPI. > That means we still need to parse the secure interrupt. > Please correct me, if I misunderstand something? :-) > >> (b) we should drop the check above > > yes, if zero is a valid GSIV, this makes sense. > >> (c) we should report the spec issue to the ASWG >> >>> +/* >>> + * acpi_gtdt_c3stop - got c3stop info from GTDT >>> + * >>> + * Returns 1 if the timer is powered in deep idle state, 0 otherwise. >>> + */ >>> +bool __init acpi_gtdt_c3stop(void) >>> +{ >>> + struct acpi_table_gtdt *gtdt = acpi_gtdt_desc.gtdt; >>> + >>> + return !(gtdt->non_secure_el1_flags & ACPI_GTDT_ALWAYS_ON); >>> +} >> >> It looks like this can differ per interrupt. Shouldn't we check the >> appropriate one? > > yes, I think you are right. I think Mark already clarified this it's a global flag which defined in the spec, and we don't need to update it. Thanks Hanjun -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Mark, Sorry for the late reply. On 10/21/2016 12:37 AM, Mark Rutland wrote: > Hi, > > As a heads-up, on v4.9-rc1 I see conflicts at least against > arch/arm64/Kconfig. Luckily git am -3 seems to be able to fix that up > automatically, but this will need to be rebased before the next posting > and/or merging. > > On Thu, Sep 29, 2016 at 02:17:12AM +0800, fu.wei@linaro.org wrote: >> +static int __init map_gt_gsi(u32 interrupt, u32 flags) >> +{ >> + int trigger, polarity; >> + >> + if (!interrupt) >> + return 0; > > Urgh. > > Only the secure interrupt (which we do not need) is optional in this > manner, and (hilariously), zero appears to also be a valid GSIV, per > figure 5-24 in the ACPI 6.1 spec. > > So, I think that: > > (a) we should not bother parsing the secure interrupt > (b) we should drop the check above > (c) we should report the spec issue to the ASWG Sorry, I willing to do that, but I need to figure out the issue here. What kind of issue in detail? do you mean that zero should not be valid for arch timer interrupts? Thanks Hanjun -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/11/2016 09:46 PM, Hanjun Guo wrote: > Hi Mark, > > Sorry for the late reply. > > On 10/21/2016 12:37 AM, Mark Rutland wrote: >> Hi, >> >> As a heads-up, on v4.9-rc1 I see conflicts at least against >> arch/arm64/Kconfig. Luckily git am -3 seems to be able to fix that up >> automatically, but this will need to be rebased before the next posting >> and/or merging. >> >> On Thu, Sep 29, 2016 at 02:17:12AM +0800, fu.wei@linaro.org wrote: >>> +static int __init map_gt_gsi(u32 interrupt, u32 flags) >>> +{ >>> + int trigger, polarity; >>> + >>> + if (!interrupt) >>> + return 0; >> >> Urgh. >> >> Only the secure interrupt (which we do not need) is optional in this >> manner, and (hilariously), zero appears to also be a valid GSIV, per >> figure 5-24 in the ACPI 6.1 spec. >> >> So, I think that: >> >> (a) we should not bother parsing the secure interrupt >> (b) we should drop the check above >> (c) we should report the spec issue to the ASWG > > Sorry, I willing to do that, but I need to figure out the issue here. > What kind of issue in detail? do you mean that zero should not be valid > for arch timer interrupts? OK, I think you are referring to "we don't need the secure interrupt", correct me if I'm wrong (still in jet lag...). Thanks Hanjun -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Nov 11, 2016 at 09:46:29PM +0800, Hanjun Guo wrote: > On 10/21/2016 12:37 AM, Mark Rutland wrote: > >On Thu, Sep 29, 2016 at 02:17:12AM +0800, fu.wei@linaro.org wrote: > >>+static int __init map_gt_gsi(u32 interrupt, u32 flags) > >>+{ > >>+ int trigger, polarity; > >>+ > >>+ if (!interrupt) > >>+ return 0; > > > >Urgh. > > > >Only the secure interrupt (which we do not need) is optional in this > >manner, and (hilariously), zero appears to also be a valid GSIV, per > >figure 5-24 in the ACPI 6.1 spec. > > > >So, I think that: > > > >(a) we should not bother parsing the secure interrupt > >(b) we should drop the check above > >(c) we should report the spec issue to the ASWG > > Sorry, I willing to do that, but I need to figure out the issue here. > What kind of issue in detail? do you mean that zero should not be valid > for arch timer interrupts? As above, zero is a valid GSIV, and is valid for the non-secure timer interrupts. The check is wrong for non-secure interrupts. We can ignore the secure timer interrupt since it's irrelevant to us, and remove the check. Regardless, the spec is inconsistent w.r.t. the secure interrupt being zero if not present, since zero is a valid GSIV. That should be reported to the ASWG. Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index bc3f00f..0607728 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 ARCH_HAS_DEVMEM_IS_ALLOWED 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..b24844d --- /dev/null +++ b/drivers/acpi/arm64/gtdt.c @@ -0,0 +1,152 @@ +/* + * 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 { + struct acpi_table_gtdt *gtdt; + void *platform_timer_start; + void *gtdt_end; +}; + +static struct acpi_gtdt_descriptor 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; + + return gh->type == ACPI_GTDT_TYPE_TIMER_BLOCK; +} + +static inline bool is_watchdog(void *platform_timer) +{ + struct acpi_gtdt_header *gh = platform_timer; + + return gh->type == ACPI_GTDT_TYPE_WATCHDOG; +} + +static int __init map_gt_gsi(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_gt_gsi(gtdt->secure_el1_interrupt, + gtdt->secure_el1_flags); + case PHYS_NONSECURE_PPI: + return map_gt_gsi(gtdt->non_secure_el1_interrupt, + gtdt->non_secure_el1_flags); + case VIRT_PPI: + return map_gt_gsi(gtdt->virtual_timer_interrupt, + gtdt->virtual_timer_flags); + + case 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 + * + * Returns 1 if the timer is powered in deep idle state, 0 otherwise. + */ +bool __init acpi_gtdt_c3stop(void) +{ + struct acpi_table_gtdt *gtdt = acpi_gtdt_desc.gtdt; + + return !(gtdt->non_secure_el1_flags & ACPI_GTDT_ALWAYS_ON); +} + +/* + * 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. + */ +int __init acpi_gtdt_init(struct acpi_table_header *table) +{ + void *start; + 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_debug("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; + } + + start = (void *)gtdt + gtdt->platform_timer_offset; + if (start < (void *)table + sizeof(struct acpi_table_gtdt)) { + pr_err(FW_BUG "Failed to retrieve timer info from firmware: invalid data.\n"); + return -EINVAL; + } + acpi_gtdt_desc.platform_timer_start = start; + + return gtdt->platform_timer_count; +} diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig index 5677886..b58d259 100644 --- a/drivers/clocksource/Kconfig +++ b/drivers/clocksource/Kconfig @@ -285,7 +285,6 @@ config CLKSRC_MPS2 config ARM_ARCH_TIMER bool select CLKSRC_OF if OF - select CLKSRC_ACPI if ACPI config ARM_ARCH_TIMER_EVTSTREAM bool "Enable ARM architected timer event stream generation by default" diff --git a/include/linux/acpi.h b/include/linux/acpi.h index c5eaf2f..e2f9841 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -567,6 +567,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 acpi_gtdt_map_ppi(int type); +bool acpi_gtdt_c3stop(void); +#endif + #else /* !CONFIG_ACPI */ #define acpi_disabled 1