From patchwork Mon Aug 13 15:30:23 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Marc Zyngier X-Patchwork-Id: 1313861 Return-Path: X-Original-To: patchwork-linux-arm@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork2.kernel.org Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) by patchwork2.kernel.org (Postfix) with ESMTP id 3A2F7DF223 for ; Mon, 13 Aug 2012 15:34:14 +0000 (UTC) Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.76 #1 (Red Hat Linux)) id 1T0wbD-0006jW-Ig; Mon, 13 Aug 2012 15:30:35 +0000 Received: from service87.mimecast.com ([91.220.42.44]) by merlin.infradead.org with esmtp (Exim 4.76 #1 (Red Hat Linux)) id 1T0wb7-0006jI-IP for linux-arm-kernel@lists.infradead.org; Mon, 13 Aug 2012 15:30:32 +0000 Received: from cam-owa2.Emea.Arm.com (fw-tnat.cambridge.arm.com [217.140.96.21]) by service87.mimecast.com; Mon, 13 Aug 2012 16:30:26 +0100 Received: from [10.1.70.140] ([10.1.255.212]) by cam-owa2.Emea.Arm.com with Microsoft SMTPSVC(6.0.3790.3959); Mon, 13 Aug 2012 16:32:05 +0100 Message-ID: <50291D8F.8000103@arm.com> Date: Mon, 13 Aug 2012 16:30:23 +0100 From: Marc Zyngier User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:12.0) Gecko/20120430 Thunderbird/12.0.1 MIME-Version: 1.0 To: Cyril Chemparathy Subject: Re: [PATCH v2 1/2] ARM: arch_timers: enable the use of the virtual timer References: <1344681091-12652-1-git-send-email-marc.zyngier@arm.com> <1344681091-12652-2-git-send-email-marc.zyngier@arm.com> <50266C23.7040802@ti.com> In-Reply-To: <50266C23.7040802@ti.com> X-Enigmail-Version: 1.5pre X-OriginalArrivalTime: 13 Aug 2012 15:32:06.0039 (UTC) FILETIME=[CB719270:01CD7968] X-MC-Unique: 112081316302610601 X-Spam-Note: CRM114 invocation failed X-Spam-Score: -2.6 (--) X-Spam-Report: SpamAssassin version 3.3.2 on merlin.infradead.org summary: Content analysis details: (-2.6 points) pts rule name description ---- ---------------------- -------------------------------------------------- -0.7 RCVD_IN_DNSWL_LOW RBL: Sender listed at http://www.dnswl.org/, low trust [91.220.42.44 listed in list.dnswl.org] -0.0 SPF_PASS SPF: sender matches SPF record -1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1% [score: 0.0000] Cc: "rvaswani@codeaurora.org" , "linux-arm-kernel@lists.infradead.org" X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-arm-kernel-bounces@lists.infradead.org Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org On 11/08/12 15:28, Cyril Chemparathy wrote: > On 8/11/2012 6:31 AM, Marc Zyngier wrote: >> At the moment, the arch_timer driver only uses the physical timer, >> which can cause problem if PL2 hasn't enabled PL1 access in CNTHCTL, >> which is likely in a virtualized environment. Instead, the virtual >> timer is always available. >> >> This patch enables the use of both the virtual timer, unless no >> interrupt is provided in the DT for it, in which case is falls >> back to the physical timer. >> > > I'm curious about the cost of the added pointer chasing introduced by > this patch. > > The original code gets nicely inlined by the compiler, and this hides > all the switch-case register index stuff. For instance: > > 10797 c001288c : > 10798 c001288c: ee1e3f32 mrc 15, 0, r3, cr14, cr2, {1} > 10799 c0012890: e3c33002 bic r3, r3, #2 > 10800 c0012894: ee0e0f12 mcr 15, 0, r0, cr14, cr2, {0} > 10801 c0012898: f57ff06f isb sy > 10802 c001289c: e3833001 orr r3, r3, #1 > 10803 c00128a0: ee0e3f32 mcr 15, 0, r3, cr14, cr2, {1} > 10804 c00128a4: f57ff06f isb sy > 10805 c00128a8: e3a00000 mov r0, #0 > 10806 c00128ac: e12fff1e bx lr > > With the added pointer chasing, we unfortunately lose out on all the > work that the compiler used to do for us. We now end up having to snake > our way through the following: [...] Yup, that doesn't look too good. > I think we'd be better off separating between these (virt, phys, ...) > implementations at a higher level of operations (set_mode, > set_next_event, ...) rather than separating at a register operations > level as you have in this patch. This approach leads to a lot of code duplication, unless we do some ugly preprocessor hackery (see patch attached). I then get much better code (smaller, faster), but I'm not sure we want to live with this... M. From 1c2d8adcc83261e6fd7e8852d9a2ce61acf584d7 Mon Sep 17 00:00:00 2001 From: Marc Zyngier Date: Thu, 10 May 2012 13:57:03 +0100 Subject: [PATCH] ARM: arch_timers: enable the use of the virtual timer At the moment, the arch_timer driver only uses the physical timer, which can cause problem if PL2 hasn't enabled PL1 access in CNTHCTL, which is likely in a virtualized environment. Instead, the virtual timer is always available. This patch enables the use of the virtual timer, unless no interrupt is provided in the DT for it, in which case it falls back to the physical timer. Signed-off-by: Marc Zyngier --- arch/arm/kernel/arch_timer.c | 325 +++++++++++++++++++++++++++++------------- 1 file changed, 222 insertions(+), 103 deletions(-) diff --git a/arch/arm/kernel/arch_timer.c b/arch/arm/kernel/arch_timer.c index cf25880..b314f57 100644 --- a/arch/arm/kernel/arch_timer.c +++ b/arch/arm/kernel/arch_timer.c @@ -27,13 +27,23 @@ #include static unsigned long arch_timer_rate; -static int arch_timer_ppi; -static int arch_timer_ppi2; + +enum ppi_nr { + PHYS_SECURE_PPI, + PHYS_NONSECURE_PPI, + VIRT_PPI, + HYP_PPI, + MAX_TIMER_PPI +}; + +static int arch_timer_ppi[MAX_TIMER_PPI]; static struct clock_event_device __percpu **arch_timer_evt; extern void init_current_timer_delay(unsigned long freq); +static bool arch_timer_use_virtual = true; + /* * Architected system timer support. */ @@ -43,10 +53,9 @@ extern void init_current_timer_delay(unsigned long freq); #define ARCH_TIMER_CTRL_IT_STAT (1 << 2) #define ARCH_TIMER_REG_CTRL 0 -#define ARCH_TIMER_REG_FREQ 1 -#define ARCH_TIMER_REG_TVAL 2 +#define ARCH_TIMER_REG_TVAL 1 -static void arch_timer_reg_write(int reg, u32 val) +static void arch_timer_phys_reg_write(int reg, u32 val) { switch (reg) { case ARCH_TIMER_REG_CTRL: @@ -60,100 +69,175 @@ static void arch_timer_reg_write(int reg, u32 val) isb(); } -static u32 arch_timer_reg_read(int reg) +static u32 arch_timer_phys_reg_read(int reg) { - u32 val; + u32 val = 0; switch (reg) { case ARCH_TIMER_REG_CTRL: asm volatile("mrc p15, 0, %0, c14, c2, 1" : "=r" (val)); break; - case ARCH_TIMER_REG_FREQ: - asm volatile("mrc p15, 0, %0, c14, c0, 0" : "=r" (val)); - break; case ARCH_TIMER_REG_TVAL: asm volatile("mrc p15, 0, %0, c14, c2, 0" : "=r" (val)); break; - default: - BUG(); } return val; } -static irqreturn_t arch_timer_handler(int irq, void *dev_id) +static inline cycle_t arch_counter_get_cntpct(void) { - struct clock_event_device *evt = *(struct clock_event_device **)dev_id; - unsigned long ctrl; - - ctrl = arch_timer_reg_read(ARCH_TIMER_REG_CTRL); - if (ctrl & ARCH_TIMER_CTRL_IT_STAT) { - ctrl |= ARCH_TIMER_CTRL_IT_MASK; - arch_timer_reg_write(ARCH_TIMER_REG_CTRL, ctrl); - evt->event_handler(evt); - return IRQ_HANDLED; - } + u32 cvall, cvalh; + + asm volatile("mrrc p15, 0, %0, %1, c14" : "=r" (cvall), "=r" (cvalh)); - return IRQ_NONE; + return ((cycle_t) cvalh << 32) | cvall; } -static void arch_timer_disable(void) +static void arch_timer_virt_reg_write(int reg, u32 val) { - unsigned long ctrl; + switch (reg) { + case ARCH_TIMER_REG_CTRL: + asm volatile("mcr p15, 0, %0, c14, c3, 1" : : "r" (val)); + break; + case ARCH_TIMER_REG_TVAL: + asm volatile("mcr p15, 0, %0, c14, c3, 0" : : "r" (val)); + break; + } - ctrl = arch_timer_reg_read(ARCH_TIMER_REG_CTRL); - ctrl &= ~ARCH_TIMER_CTRL_ENABLE; - arch_timer_reg_write(ARCH_TIMER_REG_CTRL, ctrl); + isb(); } -static void arch_timer_set_mode(enum clock_event_mode mode, - struct clock_event_device *clk) +static u32 arch_timer_virt_reg_read(int reg) { - switch (mode) { - case CLOCK_EVT_MODE_UNUSED: - case CLOCK_EVT_MODE_SHUTDOWN: - arch_timer_disable(); + u32 val = 0; + + switch (reg) { + case ARCH_TIMER_REG_CTRL: + asm volatile("mrc p15, 0, %0, c14, c3, 1" : "=r" (val)); break; - default: + case ARCH_TIMER_REG_TVAL: + asm volatile("mrc p15, 0, %0, c14, c3, 0" : "=r" (val)); break; } + + return val; } -static int arch_timer_set_next_event(unsigned long evt, - struct clock_event_device *unused) +static inline cycle_t arch_counter_get_cntvct(void) +{ + u32 cvall, cvalh; + + asm volatile("mrrc p15, 1, %0, %1, c14" : "=r" (cvall), "=r" (cvalh)); + + return ((cycle_t) cvalh << 32) | cvall; +} + +#define timer_handler(type, evt) \ +({ \ + unsigned long ctrl; \ + ctrl = arch_timer_##type##_reg_read(ARCH_TIMER_REG_CTRL); \ + (ctrl & ARCH_TIMER_CTRL_IT_STAT) ? ({ \ + ctrl |= ARCH_TIMER_CTRL_IT_MASK; \ + arch_timer_##type##_reg_write(ARCH_TIMER_REG_CTRL, ctrl);\ + evt->event_handler(evt); \ + IRQ_HANDLED; \ + }) : IRQ_NONE; \ +}) + +static irqreturn_t arch_timer_handler_virt(int irq, void *dev_id) +{ + struct clock_event_device *evt = *(struct clock_event_device **)dev_id; + + return timer_handler(virt, evt); +} + +static irqreturn_t arch_timer_handler_phys(int irq, void *dev_id) { - unsigned long ctrl; + struct clock_event_device *evt = *(struct clock_event_device **)dev_id; - ctrl = arch_timer_reg_read(ARCH_TIMER_REG_CTRL); - ctrl |= ARCH_TIMER_CTRL_ENABLE; - ctrl &= ~ARCH_TIMER_CTRL_IT_MASK; + return timer_handler(phys, evt); +} + +#define timer_set_mode(type,mode) \ +do { \ + unsigned long ctrl; \ + switch (mode) { \ + case CLOCK_EVT_MODE_UNUSED: \ + case CLOCK_EVT_MODE_SHUTDOWN: \ + ctrl = arch_timer_##type##_reg_read(ARCH_TIMER_REG_CTRL);\ + ctrl &= ~ARCH_TIMER_CTRL_ENABLE; \ + arch_timer_##type##_reg_write(ARCH_TIMER_REG_CTRL, ctrl);\ + break; \ + default: \ + break; \ + } \ +} while(0) + +static void arch_timer_set_mode_virt(enum clock_event_mode mode, + struct clock_event_device *clk) +{ + timer_set_mode(virt, mode); +} - arch_timer_reg_write(ARCH_TIMER_REG_TVAL, evt); - arch_timer_reg_write(ARCH_TIMER_REG_CTRL, ctrl); +static void arch_timer_set_mode_phys(enum clock_event_mode mode, + struct clock_event_device *clk) +{ + timer_set_mode(phys, mode); +} +#define set_next_event(type, evt) \ +do { \ + unsigned long ctrl; \ + ctrl = arch_timer_##type##_reg_read(ARCH_TIMER_REG_CTRL); \ + ctrl |= ARCH_TIMER_CTRL_ENABLE; \ + ctrl &= ~ARCH_TIMER_CTRL_IT_MASK; \ + arch_timer_##type##_reg_write(ARCH_TIMER_REG_TVAL, evt); \ + arch_timer_##type##_reg_write(ARCH_TIMER_REG_CTRL, ctrl); \ +} while(0) + +static int arch_timer_set_next_event_virt(unsigned long evt, + struct clock_event_device *unused) +{ + set_next_event(virt, evt); return 0; } -static int __cpuinit arch_timer_setup(struct clock_event_device *clk) +static int arch_timer_set_next_event_phys(unsigned long evt, + struct clock_event_device *unused) { - /* Be safe... */ - arch_timer_disable(); + set_next_event(phys, evt); + return 0; +} +static int __cpuinit arch_timer_setup(struct clock_event_device *clk) +{ clk->features = CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_C3STOP; clk->name = "arch_sys_timer"; clk->rating = 450; - clk->set_mode = arch_timer_set_mode; - clk->set_next_event = arch_timer_set_next_event; - clk->irq = arch_timer_ppi; + if (arch_timer_use_virtual) { + arch_timer_set_mode_virt(CLOCK_EVT_MODE_SHUTDOWN, NULL); + clk->set_mode = arch_timer_set_mode_virt; + clk->set_next_event = arch_timer_set_next_event_virt; + } else { + arch_timer_set_mode_phys(CLOCK_EVT_MODE_SHUTDOWN, NULL); + clk->set_mode = arch_timer_set_mode_phys; + clk->set_next_event = arch_timer_set_next_event_phys; + } + + clk->irq = arch_timer_ppi[PHYS_SECURE_PPI]; clockevents_config_and_register(clk, arch_timer_rate, 0xf, 0x7fffffff); *__this_cpu_ptr(arch_timer_evt) = clk; - enable_percpu_irq(clk->irq, 0); - if (arch_timer_ppi2) - enable_percpu_irq(arch_timer_ppi2, 0); + if (arch_timer_use_virtual) + enable_percpu_irq(arch_timer_ppi[VIRT_PPI], 0); + else { + enable_percpu_irq(arch_timer_ppi[PHYS_SECURE_PPI], 0); + enable_percpu_irq(arch_timer_ppi[PHYS_NONSECURE_PPI], 0); + } return 0; } @@ -173,8 +257,7 @@ static int arch_timer_available(void) return -ENXIO; if (arch_timer_rate == 0) { - arch_timer_reg_write(ARCH_TIMER_REG_CTRL, 0); - freq = arch_timer_reg_read(ARCH_TIMER_REG_FREQ); + asm volatile("mrc p15, 0, %0, c14, c0, 0" : "=r" (freq)); /* Check the timer frequency. */ if (freq == 0) { @@ -185,43 +268,42 @@ static int arch_timer_available(void) arch_timer_rate = freq; } - pr_info_once("Architected local timer running at %lu.%02luMHz.\n", - arch_timer_rate / 1000000, (arch_timer_rate / 10000) % 100); + pr_info_once("Architected local timer running at %lu.%02luMHz (%s).\n", + arch_timer_rate / 1000000, (arch_timer_rate / 10000) % 100, + arch_timer_use_virtual ? "virt" : "phys"); return 0; } -static inline cycle_t arch_counter_get_cntpct(void) +static u32 notrace arch_counter_get_cntpct32(void) { - u32 cvall, cvalh; + cycle_t cnt = arch_counter_get_cntpct(); - asm volatile("mrrc p15, 0, %0, %1, c14" : "=r" (cvall), "=r" (cvalh)); - - return ((cycle_t) cvalh << 32) | cvall; -} - -static inline cycle_t arch_counter_get_cntvct(void) -{ - u32 cvall, cvalh; - - asm volatile("mrrc p15, 1, %0, %1, c14" : "=r" (cvall), "=r" (cvalh)); - - return ((cycle_t) cvalh << 32) | cvall; + /* + * The sched_clock infrastructure only knows about counters + * with at most 32bits. Forget about the upper 24 bits for the + * time being... + */ + return (u32)(cnt & (u32)~0); } static u32 notrace arch_counter_get_cntvct32(void) { - cycle_t cntvct = arch_counter_get_cntvct(); + cycle_t cnt = arch_counter_get_cntvct(); /* * The sched_clock infrastructure only knows about counters * with at most 32bits. Forget about the upper 24 bits for the * time being... */ - return (u32)(cntvct & (u32)~0); + return (u32)(cnt & (u32)~0); } static cycle_t arch_counter_read(struct clocksource *cs) { + /* + * Always use the physical counter for the clocksource. + * CNTHCTL.PL1PCTEN must be set to 1. + */ return arch_counter_get_cntpct(); } @@ -245,10 +327,15 @@ static void __cpuinit arch_timer_stop(struct clock_event_device *clk) { pr_debug("arch_timer_teardown disable IRQ%d cpu #%d\n", clk->irq, smp_processor_id()); - disable_percpu_irq(clk->irq); - if (arch_timer_ppi2) - disable_percpu_irq(arch_timer_ppi2); - arch_timer_set_mode(CLOCK_EVT_MODE_UNUSED, clk); + + if (arch_timer_use_virtual) { + disable_percpu_irq(arch_timer_ppi[VIRT_PPI]); + arch_timer_set_mode_phys(CLOCK_EVT_MODE_UNUSED, clk); + } else { + disable_percpu_irq(arch_timer_ppi[PHYS_SECURE_PPI]); + disable_percpu_irq(arch_timer_ppi[PHYS_NONSECURE_PPI]); + arch_timer_set_mode_phys(CLOCK_EVT_MODE_UNUSED, clk); + } } static struct local_timer_ops arch_timer_ops __cpuinitdata = { @@ -261,36 +348,44 @@ static struct clock_event_device arch_timer_global_evt; static int __init arch_timer_register(void) { int err; + int ppi; err = arch_timer_available(); if (err) - return err; + goto out; arch_timer_evt = alloc_percpu(struct clock_event_device *); - if (!arch_timer_evt) - return -ENOMEM; + if (!arch_timer_evt) { + err = -ENOMEM; + goto out; + } clocksource_register_hz(&clocksource_counter, arch_timer_rate); - err = request_percpu_irq(arch_timer_ppi, arch_timer_handler, - "arch_timer", arch_timer_evt); + if (arch_timer_use_virtual) { + ppi = arch_timer_ppi[VIRT_PPI]; + err = request_percpu_irq(ppi, arch_timer_handler_virt, + "arch_timer", arch_timer_evt); + } else { + ppi = arch_timer_ppi[PHYS_SECURE_PPI]; + err = request_percpu_irq(ppi, arch_timer_handler_phys, + "arch_timer", arch_timer_evt); + if (!err) { + ppi = arch_timer_ppi[PHYS_NONSECURE_PPI]; + err = request_percpu_irq(ppi, arch_timer_handler_phys, + "arch_timer", arch_timer_evt); + if (err) + free_percpu_irq(arch_timer_ppi[PHYS_SECURE_PPI], + arch_timer_evt); + } + } + if (err) { pr_err("arch_timer: can't register interrupt %d (%d)\n", - arch_timer_ppi, err); + ppi, err); goto out_free; } - if (arch_timer_ppi2) { - err = request_percpu_irq(arch_timer_ppi2, arch_timer_handler, - "arch_timer", arch_timer_evt); - if (err) { - pr_err("arch_timer: can't register interrupt %d (%d)\n", - arch_timer_ppi2, err); - arch_timer_ppi2 = 0; - goto out_free_irq; - } - } - err = local_timer_register(&arch_timer_ops); if (err) { /* @@ -310,13 +405,18 @@ static int __init arch_timer_register(void) return 0; out_free_irq: - free_percpu_irq(arch_timer_ppi, arch_timer_evt); - if (arch_timer_ppi2) - free_percpu_irq(arch_timer_ppi2, arch_timer_evt); + if (arch_timer_use_virtual) + free_percpu_irq(arch_timer_ppi[VIRT_PPI], arch_timer_evt); + else { + free_percpu_irq(arch_timer_ppi[PHYS_SECURE_PPI], + arch_timer_evt); + free_percpu_irq(arch_timer_ppi[PHYS_NONSECURE_PPI], + arch_timer_evt); + } out_free: free_percpu(arch_timer_evt); - +out: return err; } @@ -329,6 +429,7 @@ int __init arch_timer_of_register(void) { struct device_node *np; u32 freq; + int i; np = of_find_matching_node(NULL, arch_timer_of_match); if (!np) { @@ -340,22 +441,40 @@ int __init arch_timer_of_register(void) if (!of_property_read_u32(np, "clock-frequency", &freq)) arch_timer_rate = freq; - arch_timer_ppi = irq_of_parse_and_map(np, 0); - arch_timer_ppi2 = irq_of_parse_and_map(np, 1); - pr_info("arch_timer: found %s irqs %d %d\n", - np->name, arch_timer_ppi, arch_timer_ppi2); + for (i = PHYS_SECURE_PPI; i < MAX_TIMER_PPI; i++) + arch_timer_ppi[i] = irq_of_parse_and_map(np, i); + + /* + * If no interrupt provided for virtual timer, we'll have to + * stick to the physical timer. It'd better be accessible... + */ + if (!arch_timer_ppi[VIRT_PPI]) { + arch_timer_use_virtual = false; + + if (!arch_timer_ppi[PHYS_SECURE_PPI] || + !arch_timer_ppi[PHYS_NONSECURE_PPI]) { + pr_warn("arch_timer: No interrupt available, giving up\n"); + return -EINVAL; + } + } return arch_timer_register(); } int __init arch_timer_sched_clock_init(void) { + u32 (*cnt32)(void); int err; err = arch_timer_available(); if (err) return err; - setup_sched_clock(arch_counter_get_cntvct32, 32, arch_timer_rate); + if (arch_timer_use_virtual) + cnt32 = arch_counter_get_cntvct32; + else + cnt32 = arch_counter_get_cntpct32; + + setup_sched_clock(cnt32, 32, arch_timer_rate); return 0; } -- 1.7.10.3