Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753566AbcKUOIa (ORCPT ); Mon, 21 Nov 2016 09:08:30 -0500 Received: from mail-io0-f176.google.com ([209.85.223.176]:33130 "EHLO mail-io0-f176.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752865AbcKUOI2 (ORCPT ); Mon, 21 Nov 2016 09:08:28 -0500 MIME-Version: 1.0 In-Reply-To: <20161118195244.GL1197@leverpostej> References: <1479304148-2965-1-git-send-email-fu.wei@linaro.org> <1479304148-2965-8-git-send-email-fu.wei@linaro.org> <20161118195244.GL1197@leverpostej> From: Fu Wei Date: Mon, 21 Nov 2016 22:08:26 +0800 Message-ID: Subject: Re: [PATCH v16 07/15] clocksource/drivers/arm_arch_timer: Refactor arch_timer_detect_rate to keep dt code in *_of_init To: Mark Rutland Cc: "Rafael J. Wysocki" , Len Brown , Daniel Lezcano , Thomas Gleixner , Marc Zyngier , Lorenzo Pieralisi , Sudeep Holla , Hanjun Guo , linux-arm-kernel@lists.infradead.org, Linaro ACPI Mailman List , Linux Kernel Mailing List , ACPI Devel Maling List , rruigrok@codeaurora.org, "Abdulhamid, Harb" , Christopher Covington , Timur Tabi , G Gregory , Al Stone , Jon Masters , Wei Huang , Arnd Bergmann , Catalin Marinas , Will Deacon , Suravee Suthikulpanit , Leo Duran , Wim Van Sebroeck , Guenter Roeck , linux-watchdog@vger.kernel.org, Tomasz Nowicki , Christoffer Dall , Julien Grall Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by mail.home.local id uALE8ZL6009214 Content-Length: 6150 Lines: 184 Hi Mark On 19 November 2016 at 03:52, Mark Rutland wrote: > On Wed, Nov 16, 2016 at 09:49:00PM +0800, fu.wei@linaro.org wrote: >> From: Fu Wei >> >> The patch refactor original arch_timer_detect_rate function: >> (1) Separate out device-tree code, keep them in device-tree init >> function: >> arch_timer_of_init, >> arch_timer_mem_init; > > Please write a real commit message. Sorry, will do Since this patch will be separated into two patches. the first patch will be separating out device-tree code, so commit message can be: ----------------- clocksource/drivers/arm_arch_timer: Separate out device-tree code from arch_timer_detect_rate Currently, the arch_timer_detect_rate can get system counter frequency from device-tree, sysreg timers and MMIO timers. This is unnecessary and confusing. For ACPI, we don't need a function included device-tree code. This patch factors the device-tree related code out into device-tree init function: arch_timer_of_init and arch_timer_mem_init. ----------------- the second patch will be split arch_timer_detect_rate in two, one is for the MMIO timer, another is for the CP15 timers, so commit message can be: ----------------- clocksource/drivers/arm_arch_timer:split arch_timer_detect_rate for the MMIO and CP15 timers The arch_timer_detect_rate can get system counter frequency sysreg timers and MMIO timers. This is unnecessary. For initializing sysreg timers, we shouldn't try to access MMIO timers. This patch split arch_timer_detect_rate into two function: arch_timer_detect_rate and arch_timer_mem_detect_rate. ----------------- Please correct me if these commit message are inappropriate. Great thanks > >> (2) Improve original mechanism, if getting from memory-mapped timer >> fail, try arch_timer_get_cntfrq() again. > > This is *not* a refactoring. It's completely unrelated to the supposed > refactoring from point (1), and if necessary, should be a separate > patch. > > *Why* are you maknig this change? Does some ACPI platform have an MMIO > timer with an ill-configured CNTFRQ register? If so, report that to the > vendor. Don't add yet another needless bodge. > > I'd really like to split the MMIO and CP15 timers, and this is yet > another hack that'll make it harder to do so. you are right, I should split this founction for the MMIO and CP15 timers > >> Signed-off-by: Fu Wei >> --- >> drivers/clocksource/arm_arch_timer.c | 45 +++++++++++++++++++++++------------- >> 1 file changed, 29 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c >> index af22953..fe4e812 100644 >> --- a/drivers/clocksource/arm_arch_timer.c >> +++ b/drivers/clocksource/arm_arch_timer.c >> @@ -487,27 +487,31 @@ static int arch_timer_starting_cpu(unsigned int cpu) >> return 0; >> } >> >> -static void >> -arch_timer_detect_rate(void __iomem *cntbase, struct device_node *np) >> +static void arch_timer_detect_rate(void __iomem *cntbase) >> { >> /* Who has more than one independent system counter? */ >> if (arch_timer_rate) >> return; >> - >> /* >> - * Try to determine the frequency from the device tree or CNTFRQ, >> - * if ACPI is enabled, get the frequency from CNTFRQ ONLY. >> + * If we got memory-mapped timer(cntbase != NULL), >> + * try to determine the frequency from CNTFRQ in memory-mapped timer. >> */ > > *WHY* ? > > If we're sharing arch_timer_rate across MMIO and sysreg timers, the > sysreg value is alreayd sufficient. yes, we are sharing arch_timer_rate across MMIO and sysreg timers, But for booting with device-tree, we can't make sure which timer will be initialized first, so we may need : if (arch_timer_rate) return; > > If we're not, they should be completely independent. > >> - if (!acpi_disabled || >> - of_property_read_u32(np, "clock-frequency", &arch_timer_rate)) { >> - if (cntbase) >> - arch_timer_rate = readl_relaxed(cntbase + CNTFRQ); >> - else >> - arch_timer_rate = arch_timer_get_cntfrq(); >> - } >> + if (cntbase) >> + arch_timer_rate = readl_relaxed(cntbase + CNTFRQ); >> + /* >> + * Because in a system that implements both Secure and >> + * Non-secure states, CNTFRQ is only accessible in Secure state. > > That's true for the CNTCTLBase frame, but that doesn't matter. > > The CNTBase frames should have a readable CNTFRQ. Sorry, maybe I misunderstand the ARM doc, but in I3.5.7 CNTFRQ, Counter-timer Frequency, it says: ----------------- For the CNTBaseN and CNTEL0BaseN frames: • A RO copy of CNTFRQ is implemented in the CNTBaseN frame when both: — CNTACR.RFRQ is 1. — Bit[0] of CNTTIDR.Frame is 1. Otherwise the encoding in CNTBaseN is RES 0. • When CNTFRQ is RO in the CNTBaseN frame, it is also RO in the CNTEL0BaseN frame if bit[2] of CNTTIDR.Frame is 1 and either: — The value of CNTEL0ACR.EL0VCTEN is 1. — The value of CNTEL0ACR.EL0PCTEN is 1. Otherwise the CNTFRQ encoding in CNTEL0BaseN frame is RES 0. In a system that implements both Secure and Non-secure states, this register is only accessible by Secure accesses. ----------------- So I think this is for both CNTBaseN and CNTEL0BaseN frames? Please correct me. When I tested my patchset on Foundation model, I got "0" from CNTBaseN's CNTFRQ, so mybe is not implemented? > >> + * So the operation above may fail, even if (cntbase != NULL), >> + * especially on ARM64. >> + * In this case, we can try cntfrq_el0(system coprocessor register). >> + */ >> + if (!arch_timer_rate) >> + arch_timer_rate = arch_timer_get_cntfrq(); >> + else >> + return; > > Urrgh. > > Please have separate paths to determine the MMIO frequency and the > sysreg frequency, and use the appropriate one for the counter you want > to know the frequency of. OK, will do > > Thanks, > Mark. -- Best regards, Fu Wei Software Engineer Red Hat