| Message ID | 1440435683-7343-9-git-send-email-fu.wei@linaro.org |
|---|---|
| State | New |
| Headers | show |
Hi Thomas, On 25 August 2015 at 01:50, Thomas Gleixner <tglx@linutronix.de> wrote: > On Tue, 25 Aug 2015, fu.wei@linaro.org wrote: > > You Cc the world and some more on your patch, but you fail to add the > maintainers of the clocksource code to the Cc list. Sigh. my fault So sorry for that, I will add the maintainers of clocksource into cc list. > >> From: Fu Wei <fu.wei@linaro.org> >> >> The patch update arm_arch_timer driver to use the function >> provided by the new GTDT driver of ACPI. >> By this way, arm_arch_timer.c can be simplified, and separate >> all the ACPI GTDT knowledge from this timer driver. > > That's not a proper changelog and this patch want's to be split in two: > > 1) Implement the new ACPI function > > 2) Make use of it Yes, you are right , will improve this > >> index 0aa135d..99505bb 100644 >> --- a/drivers/clocksource/arm_arch_timer.c >> +++ b/drivers/clocksource/arm_arch_timer.c >> @@ -817,68 +817,30 @@ CLOCKSOURCE_OF_DECLARE(armv7_arch_timer_mem, "arm,armv7-timer-mem", >> arch_timer_mem_init); >> >> #ifdef CONFIG_ACPI >> -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); >> -} >> - >> /* Initialize per-processor generic timer */ >> -static int __init arch_timer_acpi_init(struct acpi_table_header *table) >> +void __init arch_timer_acpi_init(void) >> { > > And how is that supposed to work when we have next generation CPUs > which implement a different timer? You break multisystem kernels that > way. yes, you are right, If there is a next generation CPUs which implement a different timer, (maybe) this driver can not work. we may need a new timer driver. But, (1) for now, aarch64 core always has the arch timer(this timer is part of aarch64 architecture). and the existing code make ARM64 kernel "select ARM_ARCH_TIMER " (2) GTDT is designed for generic timer, so in this call " arch_timer_acpi_init" we parse the gtdt info. (3) once we have a ARM64 CPUs which implement a different timer, we may need to select a right timer in the config stage. and this timer may not be described in GTDT. So we can implement another arch_timer_acpi_init by that time in new timer driver.. if the new time still uses GTDT(or new version GTDT), we may need to update gtdt.c for new timer by that time. but before we really have this new timer, I think this code is OK to use. Do I understand your comment correctly? :-) > > Thanks, > > tglx
Hi Thomas, Thanks for the comments, I got some questions and reply below. On 08/26/2015 03:17 AM, Thomas Gleixner wrote: > On Wed, 26 Aug 2015, Fu Wei wrote: >>>> /* Initialize per-processor generic timer */ >>>> -static int __init arch_timer_acpi_init(struct acpi_table_header *table) >>>> +void __init arch_timer_acpi_init(void) >>>> { >>> >>> And how is that supposed to work when we have next generation CPUs >>> which implement a different timer? You break multisystem kernels that >>> way. Sorry, I think I missed some context here that I don't understand why the code here will break multisystem kernels? I'm trying to understand the problem here and update the code :) >> >> yes, you are right, If there is a next generation CPUs which >> implement a different timer, (maybe) this driver can not work. >> we may need a new timer driver. >> >> But, >> (1) for now, aarch64 core always has the arch timer(this timer is >> part of aarch64 architecture). >> and the existing code make ARM64 kernel "select ARM_ARCH_TIMER " >> (2) GTDT is designed for generic timer, so in this call " >> arch_timer_acpi_init" we parse the gtdt info. >> (3) once we have a ARM64 CPUs which implement a different timer, we >> may need to select a right timer in the config stage. >> and this timer may not be described in GTDT. So we can implement >> another arch_timer_acpi_init by that time in new timer driver.. >> if the new time still uses GTDT(or new version GTDT), we may need to >> update gtdt.c for new timer by that time. > > That's simply wrong. You want to build kernels which run on both cpus > and the selection of the timer happens at runtime depending on the > ACPI info. We do the same thing with device tree. I think the code can do that if I understand correctly. The code for now is that we only support arch timer on ARM64, and this patch set is adding SBSA watchdog timer support which need same function in arch timer, so we move that function to common place. We will load the driver (arch timer, memory mapped timer) when the ACPI table defines them, which when new timer is coming, that will defined in the ACPI table and load the driver as needed. Please correct me if I misse something, thanks. Hanjun -- To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/27/2015 08:08 PM, Thomas Gleixner wrote: > On Thu, 27 Aug 2015, Hanjun Guo wrote: >> On 08/26/2015 03:17 AM, Thomas Gleixner wrote: >>> On Wed, 26 Aug 2015, Fu Wei wrote: >>>>>> /* Initialize per-processor generic timer */ >>>>>> -static int __init arch_timer_acpi_init(struct acpi_table_header >>>>>> *table) >>>>>> +void __init arch_timer_acpi_init(void) >>>>>> { >>>>> >>>>> And how is that supposed to work when we have next generation CPUs >>>>> which implement a different timer? You break multisystem kernels that >>>>> way. >> >> Sorry, I think I missed some context here that I don't understand >> why the code here will break multisystem kernels? I'm trying to >> understand the problem here and update the code :) >> >>>> >>>> yes, you are right, If there is a next generation CPUs which >>>> implement a different timer, (maybe) this driver can not work. >>>> we may need a new timer driver. >>>> >>>> But, >>>> (1) for now, aarch64 core always has the arch timer(this timer is >>>> part of aarch64 architecture). >>>> and the existing code make ARM64 kernel "select ARM_ARCH_TIMER " >>>> (2) GTDT is designed for generic timer, so in this call " >>>> arch_timer_acpi_init" we parse the gtdt info. >>>> (3) once we have a ARM64 CPUs which implement a different timer, we >>>> may need to select a right timer in the config stage. >>>> and this timer may not be described in GTDT. So we can implement >>>> another arch_timer_acpi_init by that time in new timer driver.. >>>> if the new time still uses GTDT(or new version GTDT), we may need to >>>> update gtdt.c for new timer by that time. >>> >>> That's simply wrong. You want to build kernels which run on both cpus >>> and the selection of the timer happens at runtime depending on the >>> ACPI info. We do the same thing with device tree. >> >> I think the code can do that if I understand correctly. The code for >> now is that we only support arch timer on ARM64, and this patch set >> is adding SBSA watchdog timer support which need same function in >> arch timer, so we move that function to common place. >> >> We will load the driver (arch timer, memory mapped timer) when the >> ACPI table defines them, which when new timer is coming, that will >> defined in the ACPI table and load the driver as needed. >> >> Please correct me if I misse something, thanks. > > arch_timer_acpi_init() is called from the architecture boot code. So > how is that supposed to work with different timers? > > Are you going to have bla_timer_acpi_init() and foo_timer_acpi_init() > calls as well? > > Why not having a something like DT has: DECLARE_.... > > and the arch_timer_acpi_init() using that to figure out which of the > timers to initialize. Ah, ok, I can fully understand you now, thanks for your patience. Yes, I agree with you, so this is not a problem for this patch, but for the code implementation of previous code. Actually we are on the road to do as you suggested, we introduced something like #define ACPI_DECLARE(table, name, table_id, subtable, data, fn) [1] in the GICv3 and GIC self probe patch set, and I said that infrastructure can be used as clock declare too, we just trying to not add such dependence on that patch set (it's still on discussion), [1]: https://lkml.org/lkml/2015/7/29/236 If that is ok with you, we will introduce similar DECLARE_ thing for clock declare. Thanks Hanjun -- To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/27/2015 08:28 PM, Hanjun Guo wrote: > On 08/27/2015 08:08 PM, Thomas Gleixner wrote: >> On Thu, 27 Aug 2015, Hanjun Guo wrote: >>> On 08/26/2015 03:17 AM, Thomas Gleixner wrote: >>>> On Wed, 26 Aug 2015, Fu Wei wrote: >>>>>>> /* Initialize per-processor generic timer */ >>>>>>> -static int __init arch_timer_acpi_init(struct acpi_table_header >>>>>>> *table) >>>>>>> +void __init arch_timer_acpi_init(void) >>>>>>> { >>>>>> >>>>>> And how is that supposed to work when we have next generation CPUs >>>>>> which implement a different timer? You break multisystem kernels that >>>>>> way. >>> >>> Sorry, I think I missed some context here that I don't understand >>> why the code here will break multisystem kernels? I'm trying to >>> understand the problem here and update the code :) >>> >>>>> >>>>> yes, you are right, If there is a next generation CPUs which >>>>> implement a different timer, (maybe) this driver can not work. >>>>> we may need a new timer driver. >>>>> >>>>> But, >>>>> (1) for now, aarch64 core always has the arch timer(this timer is >>>>> part of aarch64 architecture). >>>>> and the existing code make ARM64 kernel "select ARM_ARCH_TIMER " >>>>> (2) GTDT is designed for generic timer, so in this call " >>>>> arch_timer_acpi_init" we parse the gtdt info. >>>>> (3) once we have a ARM64 CPUs which implement a different timer, we >>>>> may need to select a right timer in the config stage. >>>>> and this timer may not be described in GTDT. So we can implement >>>>> another arch_timer_acpi_init by that time in new timer driver.. >>>>> if the new time still uses GTDT(or new version GTDT), we may need to >>>>> update gtdt.c for new timer by that time. >>>> >>>> That's simply wrong. You want to build kernels which run on both cpus >>>> and the selection of the timer happens at runtime depending on the >>>> ACPI info. We do the same thing with device tree. >>> >>> I think the code can do that if I understand correctly. The code for >>> now is that we only support arch timer on ARM64, and this patch set >>> is adding SBSA watchdog timer support which need same function in >>> arch timer, so we move that function to common place. >>> >>> We will load the driver (arch timer, memory mapped timer) when the >>> ACPI table defines them, which when new timer is coming, that will >>> defined in the ACPI table and load the driver as needed. >>> >>> Please correct me if I misse something, thanks. >> >> arch_timer_acpi_init() is called from the architecture boot code. So >> how is that supposed to work with different timers? >> >> Are you going to have bla_timer_acpi_init() and foo_timer_acpi_init() >> calls as well? >> >> Why not having a something like DT has: DECLARE_.... >> >> and the arch_timer_acpi_init() using that to figure out which of the >> timers to initialize. > > Ah, ok, I can fully understand you now, thanks for your patience. > > Yes, I agree with you, so this is not a problem for this patch, but > for the code implementation of previous code. Actually we are on the > road to do as you suggested, we introduced something like > #define ACPI_DECLARE(table, name, table_id, subtable, data, fn) [1] > in the GICv3 and GIC self probe patch set, and I said that > infrastructure can be used as clock declare too, we just trying > to not add such dependence on that patch set (it's still on discussion), > > [1]: https://lkml.org/lkml/2015/7/29/236 > > If that is ok with you, we will introduce similar DECLARE_ thing > for clock declare. Or we can drop this patch from this patch set, and clean up this patch when the ACPI_DECLARE() infrastructure is ready for upstream. Thanks Hanjun -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Thomas, Hanjun On 27 August 2015 at 21:40, Thomas Gleixner <tglx@linutronix.de> wrote: > On Thu, 27 Aug 2015, Hanjun Guo wrote: >> > [1]: https://lkml.org/lkml/2015/7/29/236 >> > >> > If that is ok with you, we will introduce similar DECLARE_ thing >> > for clock declare. > > Yes. Thanks > >> Or we can drop this patch from this patch set, and clean up this >> patch when the ACPI_DECLARE() infrastructure is ready for upstream. > > Works either way. I just noticed that hard coded init thing and > decided to rant about it :) OK, good idea, this patch will be improve by DECLARE_ thing, then upstream. drop this from this patchset. Great thanks for your help > > Thanks, > > tglx > -- > To unsubscribe from this list: send the line "unsubscribe devicetree" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On 27/08/15 14:51, Fu Wei wrote: > Hi Thomas, Hanjun > > On 27 August 2015 at 21:40, Thomas Gleixner <tglx@linutronix.de> wrote: >> On Thu, 27 Aug 2015, Hanjun Guo wrote: >>>> [1]: https://lkml.org/lkml/2015/7/29/236 >>>> >>>> If that is ok with you, we will introduce similar DECLARE_ thing >>>> for clock declare. >> >> Yes. > > Thanks > >> >>> Or we can drop this patch from this patch set, and clean up this >>> patch when the ACPI_DECLARE() infrastructure is ready for upstream. >> >> Works either way. I just noticed that hard coded init thing and >> decided to rant about it :) > > OK, good idea, this patch will be improve by DECLARE_ thing, then upstream. > drop this from this patchset. > > Great thanks for your help You probably want to keep an eye on this: https://lwn.net/Articles/657238/ which implements the necessary infrastructure. I'd appreciate if you could give it a go to find out if it works for you. Thanks, M.
diff --git a/arch/arm64/kernel/time.c b/arch/arm64/kernel/time.c index 42f9195..2cabea6 100644 --- a/arch/arm64/kernel/time.c +++ b/arch/arm64/kernel/time.c @@ -75,9 +75,9 @@ void __init time_init(void) /* * Since ACPI or FDT will only one be available in the system, - * we can use acpi_generic_timer_init() here safely + * we can use arch_timer_acpi_init() here safely */ - acpi_generic_timer_init(); + arch_timer_acpi_init(); arch_timer_rate = arch_timer_get_rate(); if (!arch_timer_rate) diff --git a/drivers/acpi/gtdt.c b/drivers/acpi/gtdt.c index bbe3a2e..3559babf 100644 --- a/drivers/acpi/gtdt.c +++ b/drivers/acpi/gtdt.c @@ -17,6 +17,8 @@ #include <linux/module.h> #include <linux/platform_device.h> +#include <clocksource/arm_arch_timer.h> + static int __init map_generic_timer_interrupt(u32 interrupt, u32 flags) { int trigger, polarity; @@ -33,6 +35,47 @@ static int __init map_generic_timer_interrupt(u32 interrupt, u32 flags) return acpi_register_gsi(NULL, interrupt, trigger, polarity); } +static struct arch_timer_data __initdata *arch_timer_data_p; + +static int __init arch_timer_data_init(struct acpi_table_header *table) +{ + struct acpi_table_gtdt *gtdt; + + gtdt = container_of(table, struct acpi_table_gtdt, header); + + arch_timer_data_p->phys_secure_ppi = + map_generic_timer_interrupt(gtdt->secure_el1_interrupt, + gtdt->secure_el1_flags); + + arch_timer_data_p->phys_nonsecure_ppi = + map_generic_timer_interrupt(gtdt->non_secure_el1_interrupt, + gtdt->non_secure_el1_flags); + + arch_timer_data_p->virt_ppi = + map_generic_timer_interrupt(gtdt->virtual_timer_interrupt, + gtdt->virtual_timer_flags); + + arch_timer_data_p->hyp_ppi = + map_generic_timer_interrupt(gtdt->non_secure_el2_interrupt, + gtdt->non_secure_el2_flags); + + arch_timer_data_p->c3stop = !(gtdt->non_secure_el1_flags & + ACPI_GTDT_ALWAYS_ON); + + return 0; +} + +/* Initialize the arch_timer_data struct for arm_arch_timer by GTDT info */ +int __init gtdt_arch_timer_data_init(struct arch_timer_data *data) +{ + if (acpi_disabled || !data) + return -EINVAL; + + arch_timer_data_p = data; + + return acpi_table_parse(ACPI_SIG_GTDT, arch_timer_data_init); +} + /* * Initialize a SBSA generic Watchdog platform device info from GTDT * According to SBSA specification the size of refresh and control diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig index 4e57730..e111025 100644 --- a/drivers/clocksource/Kconfig +++ b/drivers/clocksource/Kconfig @@ -119,6 +119,7 @@ config CLKSRC_STM32 config ARM_ARCH_TIMER bool select CLKSRC_OF if OF + select ACPI_GTDT if ACPI config ARM_ARCH_TIMER_EVTSTREAM bool "Support for ARM architected timer event stream generation" diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c index 0aa135d..99505bb 100644 --- a/drivers/clocksource/arm_arch_timer.c +++ b/drivers/clocksource/arm_arch_timer.c @@ -817,68 +817,30 @@ CLOCKSOURCE_OF_DECLARE(armv7_arch_timer_mem, "arm,armv7-timer-mem", arch_timer_mem_init); #ifdef CONFIG_ACPI -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); -} - /* Initialize per-processor generic timer */ -static int __init arch_timer_acpi_init(struct acpi_table_header *table) +void __init arch_timer_acpi_init(void) { - struct acpi_table_gtdt *gtdt; + struct arch_timer_data data; if (arch_timers_present & ARCH_CP15_TIMER) { pr_warn("arch_timer: already initialized, skipping\n"); - return -EINVAL; + return; } - gtdt = container_of(table, struct acpi_table_gtdt, header); - arch_timers_present |= ARCH_CP15_TIMER; - arch_timer_ppi[PHYS_SECURE_PPI] = - map_generic_timer_interrupt(gtdt->secure_el1_interrupt, - gtdt->secure_el1_flags); - - arch_timer_ppi[PHYS_NONSECURE_PPI] = - map_generic_timer_interrupt(gtdt->non_secure_el1_interrupt, - gtdt->non_secure_el1_flags); - - arch_timer_ppi[VIRT_PPI] = - map_generic_timer_interrupt(gtdt->virtual_timer_interrupt, - gtdt->virtual_timer_flags); + if (gtdt_arch_timer_data_init(&data)) + return; - arch_timer_ppi[HYP_PPI] = - map_generic_timer_interrupt(gtdt->non_secure_el2_interrupt, - gtdt->non_secure_el2_flags); + arch_timer_ppi[PHYS_SECURE_PPI] = data.phys_secure_ppi; + arch_timer_ppi[PHYS_NONSECURE_PPI] = data.phys_nonsecure_ppi; + arch_timer_ppi[VIRT_PPI] = data.virt_ppi; + arch_timer_ppi[HYP_PPI] = data.hyp_ppi; + /* Always-on capability */ + arch_timer_c3stop = data.c3stop; /* Get the frequency from CNTFRQ */ arch_timer_detect_rate(NULL, NULL); - - /* Always-on capability */ - arch_timer_c3stop = !(gtdt->non_secure_el1_flags & ACPI_GTDT_ALWAYS_ON); - arch_timer_init(); - return 0; -} - -/* Initialize all the generic timers presented in GTDT */ -void __init acpi_generic_timer_init(void) -{ - if (acpi_disabled) - return; - - acpi_table_parse(ACPI_SIG_GTDT, arch_timer_acpi_init); } #endif diff --git a/include/clocksource/arm_arch_timer.h b/include/clocksource/arm_arch_timer.h index 9916d0e..5deed9d 100644 --- a/include/clocksource/arm_arch_timer.h +++ b/include/clocksource/arm_arch_timer.h @@ -43,6 +43,14 @@ enum arch_timer_reg { #define ARCH_TIMER_EVT_STREAM_FREQ 10000 /* 100us */ +struct arch_timer_data { + int phys_secure_ppi; + int phys_nonsecure_ppi; + int virt_ppi; + int hyp_ppi; + bool c3stop; +}; + #ifdef CONFIG_ARM_ARCH_TIMER extern u32 arch_timer_get_rate(void); diff --git a/include/linux/acpi.h b/include/linux/acpi.h index 57c13df..6d7a290 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -449,6 +449,11 @@ 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 +struct arch_timer_data; +int __init gtdt_arch_timer_data_init(struct arch_timer_data *data); +#endif + #else /* !CONFIG_ACPI */ #define acpi_disabled 1 diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h index 278dd27..b250b75 100644 --- a/include/linux/clocksource.h +++ b/include/linux/clocksource.h @@ -253,9 +253,9 @@ static inline void clocksource_of_init(void) {} #endif #ifdef CONFIG_ACPI -void acpi_generic_timer_init(void); +void arch_timer_acpi_init(void); #else -static inline void acpi_generic_timer_init(void) { } +static inline void arch_timer_acpi_init(void) { } #endif #endif /* _LINUX_CLOCKSOURCE_H */