Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751683AbaKUU6J (ORCPT ); Fri, 21 Nov 2014 15:58:09 -0500 Received: from mail-vc0-f170.google.com ([209.85.220.170]:54039 "EHLO mail-vc0-f170.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750712AbaKUU6H (ORCPT ); Fri, 21 Nov 2014 15:58:07 -0500 MIME-Version: 1.0 X-Originating-IP: [172.19.1.2] In-Reply-To: <20141120165849.GF5447@e104818-lin.cambridge.arm.com> References: <1412753937-29343-1-git-send-email-sonnyrao@chromium.org> <20141120161013.GB5447@e104818-lin.cambridge.arm.com> <20141120165849.GF5447@e104818-lin.cambridge.arm.com> Date: Fri, 21 Nov 2014 12:58:05 -0800 Message-ID: Subject: Re: [PATCH v4] clocksource: arch_timer: Fix code to use physical timers when requested From: Olof Johansson To: Catalin Marinas Cc: Doug Anderson , Sonny Rao , "linux-arm-kernel@lists.infradead.org" , "linux-kernel@vger.kernel.org" , Lorenzo Pieralisi , Sudeep Holla , Thomas Gleixner , Daniel Lezcano , Will Deacon , Russell King , Mark Rutland , Stephen Boyd , Marc Zyngier , "stable@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Nov 20, 2014 at 8:58 AM, Catalin Marinas wrote: > Doug, > > On Thu, Nov 20, 2014 at 04:24:09PM +0000, Doug Anderson wrote: >> On Thu, Nov 20, 2014 at 8:10 AM, Catalin Marinas >> wrote: >> > On Wed, Oct 08, 2014 at 08:38:57AM +0100, Sonny Rao wrote: >> >> This is a bug fix for using physical arch timers when >> >> the arch_timer_use_virtual boolean is false. It restores the >> >> arch_counter_get_cntpct() function after removal in >> >> >> >> 0d651e4e "clocksource: arch_timer: use virtual counters" >> >> >> >> We need this on certain ARMv7 systems which are architected like this: >> >> >> >> * The firmware doesn't know and doesn't care about hypervisor mode and >> >> we don't want to add the complexity of hypervisor there. >> >> >> >> * The firmware isn't involved in SMP bringup or resume. >> >> >> >> * The ARCH timer come up with an uninitialized offset between the >> >> virtual and physical counters. Each core gets a different random >> >> offset. >> >> >> >> * The device boots in "Secure SVC" mode. >> >> >> >> * Nothing has touched the reset value of CNTHCTL.PL1PCEN or >> >> CNTHCTL.PL1PCTEN (both default to 1 at reset) >> >> >> >> One example of such as system is RK3288 where it is much simpler to >> >> use the physical counter since there's nobody managing the offset and >> >> each time a core goes down and comes back up it will get reinitialized >> >> to some other random value. >> >> >> >> Fixes: 0d651e4e65e9 ("clocksource: arch_timer: use virtual counters") >> >> Cc: stable@vger.kernel.org >> >> Signed-off-by: Sonny Rao >> >> Acked-by: Olof Johansson >> > [...] >> >> --- a/arch/arm64/include/asm/arch_timer.h >> >> +++ b/arch/arm64/include/asm/arch_timer.h >> >> @@ -135,6 +135,16 @@ static inline void arch_timer_evtstrm_enable(int divider) >> >> #endif >> >> } >> >> >> >> +static inline u64 arch_counter_get_cntpct(void) >> >> +{ >> >> + u64 cval; >> >> + >> >> + isb(); >> >> + asm volatile("mrs %0, cntpct_el0" : "=r" (cval)); >> >> + >> >> + return cval; >> >> +} >> > >> > Sorry but I have to NAK the arm64 changes here. If the firmware is >> > broken and does not initialise CNTVOFF properly, please fix it (at least >> > on ARMv8 hardware). Also, on arm64 the vdso gettimeofday() >> > implementation relies on using the virtual counter, so correct >> > initialisation of CNTVOFF is essential. >> >> Sonny's patch here just makes it so that we honor the global variable. >> My patch at is the one >> that allows the global variable to be set. You can see in that patch >> that it's impossible for the variable to be set on ARM64. > > It just gives people ideas ;), thinking they only need to remove > IS_ENABLED(CONFIG_ARM) in your patch and get this working on arm64. > >> In previous discussions it was agreed that on ARM64 psci (or something >> similar) was a requirement anyway and that gave us a way to get the >> firmware involved again if we ever need to bring down a processor and >> bring it back up in the kernel. PSCI is not a requirement for ARM32. >> There are systems that don't get the firmware involved when a >> processor loses state (like if it is powered off and powered on again, >> maybe for suspend/resume) and there was pushback against the kernel >> itself transitioning into monitor mode to init CNTVOFF in these cases. >> People agreed a month ago that these two patches were a reasonable >> approach for ARM32. > > I'm not complaining about about arm32 here, just the arm64 > implementation. If you want to avoid #ifdefs in the arch timer driver, > what about, for arm64, defining something like: > > static inline u64 arch_counter_get_cntpct(void) > { > /* > * AArch64 kernel and user space mandate the use of CNTVCT. > */ > BUG(); > return 0; > } Seems like a reasonable approach to me. -Olof -- 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/