From patchwork Wed Apr 30 16:56:53 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Sebastian Andrzej Siewior X-Patchwork-Id: 344227 Return-Path: X-Original-To: incoming-imx@patchwork.ozlabs.org Delivered-To: patchwork-incoming-imx@bilbo.ozlabs.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.9]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id CB44F140083 for ; Thu, 1 May 2014 03:00:02 +1000 (EST) Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1WfXp1-0006nF-5N; Wed, 30 Apr 2014 16:57:27 +0000 Received: from galois.linutronix.de ([2001:470:1f0b:db:abcd:42:0:1]) by bombadil.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1WfXoy-0006kr-FB for linux-arm-kernel@lists.infradead.org; Wed, 30 Apr 2014 16:57:25 +0000 Received: from bigeasy by Galois.linutronix.de with local (Exim 4.80) (envelope-from ) id 1WfXoT-00077H-6x; Wed, 30 Apr 2014 18:56:53 +0200 Date: Wed, 30 Apr 2014 18:56:53 +0200 From: Sebastian Andrzej Siewior To: Will Deacon Subject: Re: [RFC PATCH] sched_clock: also call register_current_timer_delay() if possible Message-ID: <20140430165653.GA26716@linutronix.de> References: <1398860614-29469-1-git-send-email-bigeasy@linutronix.de> <20140430124800.GC21876@arm.com> <5360F42C.9080401@linutronix.de> <20140430132628.GC15719@arm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20140430132628.GC15719@arm.com> X-Key-Id: 97C4700B X-Key-Fingerprint: 09E2 D1F3 9A3A FF13 C3D3 961C 0688 1C1E 97C4 700B User-Agent: Mutt/1.5.21 (2010-09-15) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20140430_095724_692276_6DFDF16B X-CRM114-Status: GOOD ( 16.19 ) X-Spam-Score: -0.7 (/) X-Spam-Report: SpamAssassin version 3.3.2 on bombadil.infradead.org summary: Content analysis details: (-0.7 points) pts rule name description ---- ---------------------- -------------------------------------------------- -0.7 RP_MATCHES_RCVD Envelope sender domain matches handover relay domain Cc: Russell King , Stephen Boyd , "linux-kernel@vger.kernel.org" , John Stultz , Theodore Ts o , "linux-arm-kernel@lists.infradead.org" X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.15 Precedence: list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+incoming-imx=patchwork.ozlabs.org@lists.infradead.org List-Id: linux-imx-kernel.lists.patchwork.ozlabs.org * Will Deacon | 2014-04-30 14:26:28 [+0100]: Hi Will, >I don't think that's the problem I was referring to. What I mean is that a >clocksource might overflow at any number of bits, so the delay calculation >needs to take this into account when it does: > > while ((get_cycles() - start) < cycles) > >because a premature overflow from get_cycles() will cause us to return >early. The solution is to mask the result of the subtraction before the >comparison to match the width of the clock. So I got this: Is this what you had in mind? If so, there is one user of register_current_timer_delay() which I didn't convert. That is arch_timer_delay_timer_register(). It returns arch_counter_get_cntvct() which seems to return an u64 (which is truncated to 32bit). However arch_counter_register() registers the clocksource with 56bits. So this does not look too good, right? The other thing I noticed is |arch/arm/include/asm/timex.h:typedef unsigned long cycles_t; This works for clocksource because timekeeping is using |include/linux/clocksource.h:typedef u64 cycle_t; instead. Do I assume correct, that the arch_timer is really providing a number wider than 32bit? Shouldn't I then promote cycles_t to 64bit if that timer is active? Unless you have better suggestions of course :) >Will Sebastian diff --git a/arch/arm/include/asm/delay.h b/arch/arm/include/asm/delay.h index dff714d..49c2e93 100644 --- a/arch/arm/include/asm/delay.h +++ b/arch/arm/include/asm/delay.h @@ -18,6 +18,7 @@ struct delay_timer { unsigned long (*read_current_timer)(void); unsigned long freq; + unsigned int bits; }; extern struct arm_delay_ops { @@ -25,6 +26,7 @@ extern struct arm_delay_ops { void (*const_udelay)(unsigned long); void (*udelay)(unsigned long); unsigned long ticks_per_jiffy; + u32 mask; } arm_delay_ops; #define __delay(n) arm_delay_ops.delay(n) diff --git a/arch/arm/lib/delay.c b/arch/arm/lib/delay.c index 5306de3..dd32cc9 100644 --- a/arch/arm/lib/delay.c +++ b/arch/arm/lib/delay.c @@ -50,8 +50,9 @@ EXPORT_SYMBOL_GPL(read_current_timer); static void __timer_delay(unsigned long cycles) { cycles_t start = get_cycles(); + cycles_t mask = arm_delay_ops.mask; - while ((get_cycles() - start) < cycles) + while (((get_cycles() - start) & mask) < cycles) cpu_relax(); } @@ -69,6 +70,10 @@ static void __timer_udelay(unsigned long usecs) void __init register_current_timer_delay(const struct delay_timer *timer) { + if (timer->bits > 32) { + pr_err("timer delay bits are limited to 32bit.\n"); + return; + } if (!delay_calibrated) { pr_info("Switching to timer-based delay loop\n"); delay_timer = timer; @@ -79,6 +84,7 @@ void __init register_current_timer_delay(const struct delay_timer *timer) arm_delay_ops.delay = __timer_delay; arm_delay_ops.const_udelay = __timer_const_udelay; arm_delay_ops.udelay = __timer_udelay; + arm_delay_ops.mask = (1ULL << timer->bits) - 1; delay_calibrated = true; } else { diff --git a/arch/arm/mach-imx/time.c b/arch/arm/mach-imx/time.c index 65222ea..7ee80f5 100644 --- a/arch/arm/mach-imx/time.c +++ b/arch/arm/mach-imx/time.c @@ -131,6 +131,7 @@ static int __init mxc_clocksource_init(struct clk *timer_clk) imx_delay_timer.read_current_timer = &imx_read_current_timer; imx_delay_timer.freq = c; + imx_delay_timer.bits = 32; register_current_timer_delay(&imx_delay_timer); sched_clock_reg = reg; diff --git a/drivers/clocksource/nomadik-mtu.c b/drivers/clocksource/nomadik-mtu.c index a709cfa..aec6a61 100644 --- a/drivers/clocksource/nomadik-mtu.c +++ b/drivers/clocksource/nomadik-mtu.c @@ -241,6 +241,7 @@ static void __init nmdk_timer_init(void __iomem *base, int irq, mtu_delay_timer.read_current_timer = &nmdk_timer_read_current_timer; mtu_delay_timer.freq = rate; + mtu_delay_timer.bits = 32; register_current_timer_delay(&mtu_delay_timer); } diff --git a/drivers/clocksource/timer-u300.c b/drivers/clocksource/timer-u300.c index 5dcf756..39633aa 100644 --- a/drivers/clocksource/timer-u300.c +++ b/drivers/clocksource/timer-u300.c @@ -389,6 +389,7 @@ static void __init u300_timer_init_of(struct device_node *np) u300_delay_timer.read_current_timer = &u300_read_current_timer; u300_delay_timer.freq = rate; + u300_delay_timer.bits = 32; register_current_timer_delay(&u300_delay_timer); /*