Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1161881AbbBDS7b (ORCPT ); Wed, 4 Feb 2015 13:59:31 -0500 Received: from foss-mx-na.foss.arm.com ([217.140.108.86]:41839 "EHLO foss-mx-na.foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1161462AbbBDS72 (ORCPT ); Wed, 4 Feb 2015 13:59:28 -0500 Date: Wed, 4 Feb 2015 18:59:29 +0000 From: Lorenzo Pieralisi To: Hanjun Guo Cc: Catalin Marinas , "Rafael J. Wysocki" , Olof Johansson , Arnd Bergmann , Mark Rutland , "grant.likely@linaro.org" , Will Deacon , "graeme.gregory@linaro.org" , Sudeep Holla , "jcm@redhat.com" , Jason Cooper , Marc Zyngier , Bjorn Helgaas , Daniel Lezcano , Mark Brown , Rob Herring , Robert Richter , Randy Dunlap , Charles Garcia-Tobin , "phoenix.liyi@huawei.com" , Timur Tabi , Ashwin Chaugule , "suravee.suthikulpanit@amd.com" , Mark Langsdorf , "wangyijing@huawei.com" , "linux-acpi@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "linux-kernel@vger.kernel.org" , "linaro-acpi@lists.linaro.org" Subject: Re: [PATCH v8 17/21] clocksource / arch_timer: Parse GTDT to initialize arch timer Message-ID: <20150204185929.GH22035@red-moon> References: <1422881149-8177-1-git-send-email-hanjun.guo@linaro.org> <1422881149-8177-18-git-send-email-hanjun.guo@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1422881149-8177-18-git-send-email-hanjun.guo@linaro.org> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4414 Lines: 127 On Mon, Feb 02, 2015 at 12:45:45PM +0000, Hanjun Guo wrote: > Using the information presented by GTDT (Generic Timer Description Table) > to initialize the arch timer (not memory-mapped). Why are you not initializing the memory mapped timer ? > CC: Daniel Lezcano > Originally-by: Amit Daniel Kachhap > Tested-by: Suravee Suthikulpanit > Tested-by: Yijing Wang > Tested-by: Mark Langsdorf > Tested-by: Jon Masters > Tested-by: Timur Tabi > Signed-off-by: Hanjun Guo > --- > arch/arm64/kernel/time.c | 7 ++ > drivers/clocksource/arm_arch_timer.c | 132 ++++++++++++++++++++++++++++------- > include/linux/clocksource.h | 6 ++ > 3 files changed, 118 insertions(+), 27 deletions(-) > > diff --git a/arch/arm64/kernel/time.c b/arch/arm64/kernel/time.c > index 1a7125c..42f9195 100644 > --- a/arch/arm64/kernel/time.c > +++ b/arch/arm64/kernel/time.c > @@ -35,6 +35,7 @@ > #include > #include > #include > +#include > > #include > > @@ -72,6 +73,12 @@ void __init time_init(void) > > tick_setup_hrtimer_broadcast(); > > + /* > + * Since ACPI or FDT will only one be available in the system, > + * we can use acpi_generic_timer_init() here safely > + */ > + acpi_generic_timer_init(); > + > arch_timer_rate = arch_timer_get_rate(); > if (!arch_timer_rate) > panic("Unable to initialise architected timer.\n"); > diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c > index 095c177..407aa63 100644 > --- a/drivers/clocksource/arm_arch_timer.c > +++ b/drivers/clocksource/arm_arch_timer.c > @@ -21,6 +21,7 @@ > #include > #include > #include > +#include > > #include > #include > @@ -370,8 +371,12 @@ arch_timer_detect_rate(void __iomem *cntbase, struct device_node *np) > if (arch_timer_rate) > return; > > - /* Try to determine the frequency from the device tree or CNTFRQ */ > - if (of_property_read_u32(np, "clock-frequency", &arch_timer_rate)) { > + /* > + * Try to determine the frequency from the device tree or CNTFRQ, > + * if ACPI is enabled, get the frequency from CNTFRQ ONLY. > + */ > + if (!acpi_disabled || > + of_property_read_u32(np, "clock-frequency", &arch_timer_rate)) { This is getting a mess. cntbase tells you it is a memory mapped timer, node pointer that you are probing through DT, and to top it all acpi_disabled detects if you are probing in ACPI or DT mode. I think this function should be simplified, this driver is also pending a refactoring to split arch timer and the memory mapped one so I think you'd better wait that work to make things simpler. [...] > +/* Initialize all the generic timers presented in GTDT */ > +void __init acpi_generic_timer_init(void) > +{ > + if (acpi_disabled) > + return; acpi_disabled used again here, I repeat myself this is going to be hard to track. You should try to organize the code something like: if (acpi_disabled) timer_dt_probe(); else timer_acpi_probe(); mixing the code paths is getting unwieldy, see above to see my reasoning. > + > + acpi_table_parse(ACPI_SIG_GTDT, arch_timer_acpi_init); > +} > +#endif > diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h > index abcafaa..af6155a 100644 > --- a/include/linux/clocksource.h > +++ b/include/linux/clocksource.h > @@ -346,4 +346,10 @@ extern void clocksource_of_init(void); > static inline void clocksource_of_init(void) {} > #endif > > +#ifdef CONFIG_ACPI > +void acpi_generic_timer_init(void); > +#else > +static inline void acpi_generic_timer_init(void) { } > +#endif > + That's not nice, it is a generic header, arch specific stuff should be avoided. I think you should have an ACPI generic layer similar to clocksource_of_init(), and probe from there when matching the respective timers. Lorenzo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/