From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755922AbcIMKtk (ORCPT ); Tue, 13 Sep 2016 06:49:40 -0400 Received: from foss.arm.com ([217.140.101.70]:45766 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751433AbcIMKth (ORCPT ); Tue, 13 Sep 2016 06:49:37 -0400 Subject: Re: [PATCH v11 7/8] clocksource/drivers/arm_arch_timer: Add GTDT support for memory-mapped timer To: fu.wei@linaro.org, rjw@rjwysocki.net, lenb@kernel.org, daniel.lezcano@linaro.org, tglx@linutronix.de, lorenzo.pieralisi@arm.com, sudeep.holla@arm.com, hanjun.guo@linaro.org References: <1473168352-5156-1-git-send-email-fu.wei@linaro.org> <1473168352-5156-8-git-send-email-fu.wei@linaro.org> Cc: linux-arm-kernel@lists.infradead.org, linaro-acpi@lists.linaro.org, linux-kernel@vger.kernel.org, linux-acpi@vger.kernel.org, rruigrok@codeaurora.org, harba@codeaurora.org, cov@codeaurora.org, timur@codeaurora.org, graeme.gregory@linaro.org, al.stone@linaro.org, jcm@redhat.com, wei@redhat.com, arnd@arndb.de, wim@iguana.be, catalin.marinas@arm.com, will.deacon@arm.com, Suravee.Suthikulpanit@amd.com, leo.duran@amd.com, linux@roeck-us.net, linux-watchdog@vger.kernel.org From: Marc Zyngier X-Enigmail-Draft-Status: N1110 Organization: ARM Ltd Message-ID: <57D7D9BA.6070100@arm.com> Date: Tue, 13 Sep 2016 11:49:30 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Icedove/38.7.0 MIME-Version: 1.0 In-Reply-To: <1473168352-5156-8-git-send-email-fu.wei@linaro.org> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 06/09/16 14:25, fu.wei@linaro.org wrote: > From: Fu Wei > > The patch add memory-mapped timer register support by using the information > provided by the new GTDT driver of ACPI. > > Signed-off-by: Fu Wei > --- > drivers/clocksource/arm_arch_timer.c | 127 ++++++++++++++++++++++++++++++++++- > 1 file changed, 124 insertions(+), 3 deletions(-) > > diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c > index a01cf22..c80a2f2 100644 > --- a/drivers/clocksource/arm_arch_timer.c > +++ b/drivers/clocksource/arm_arch_timer.c > @@ -888,7 +888,128 @@ CLOCKSOURCE_OF_DECLARE(armv7_arch_timer_mem, "arm,armv7-timer-mem", > arch_timer_mem_init); > > #ifdef CONFIG_ACPI_GTDT > -/* Initialize per-processor generic timer */ > +static struct gt_timer_data __init *arch_timer_mem_get_timer( > + struct gt_block_data *gt_blocks) > +{ > + struct gt_block_data *gt_block = gt_blocks; > + struct gt_timer_data *best_frame = NULL; > + void __iomem *cntctlbase; > + u32 cnttidr; > + int i; > + > + /* > + * According to ARMv8 Architecture Reference Manual(ARM), > + * the size of CNTCTLBase frame of memory-mapped timer > + * is SZ_4K(Offset 0x000 – 0xFFF). > + */ > + cntctlbase = ioremap(gt_block->cntctlbase_phy, SZ_4K); > + if (!cntctlbase) { > + pr_err("Failed to map mem timer control frame base address\n"); > + return NULL; > + } > + cnttidr = readl_relaxed(cntctlbase + CNTTIDR); > + > + /* > + * Try to find a virtual capable frame. Otherwise fall back to a > + * physical capable frame. > + */ > + for (i = 0; i < gt_block->timer_count; i++) { > + int n; > + u32 cntacr; > + > + n = gt_block->timer[i].frame_nr; > + > + /* Try enabling everything, and see what sticks */ > + cntacr = CNTACR_RFRQ | CNTACR_RWPT | CNTACR_RPCT | > + CNTACR_RWVT | CNTACR_RVOFF | CNTACR_RVCT; > + writel_relaxed(cntacr, cntctlbase + CNTACR(n)); > + cntacr = readl_relaxed(cntctlbase + CNTACR(n)); > + > + if ((cnttidr & CNTTIDR_VIRT(n)) && > + !(~cntacr & (CNTACR_RWVT | CNTACR_RVCT))) { > + best_frame = >_block->timer[i]; > + arch_timer_mem_use_virtual = true; > + break; > + } > + > + if (~cntacr & (CNTACR_RWPT | CNTACR_RPCT)) > + continue; > + > + best_frame = >_block->timer[i]; > + } > + iounmap(cntctlbase); So you're duplicating the code that is already in arch_timer_mem_init(). I don't think that's a good idea. Please change the existing code to a set of functions that can be used in a firmware agnostic way, and plug your ACPI code onto in. Look at what we've done for the GIC: Firmware-specific code that decode their respective tables, stash that information in a firmware-independent way if required, and call into into common code. > + > + return best_frame; > +} > + > +static int __init arch_timer_mem_acpi_init(size_t timer_count) > +{ > + struct gt_block_data *gt_blocks; > + struct gt_timer_data *gt_timer; > + void __iomem *timer_cntbase; > + int ret = -EINVAL; > + int timer_irq; > + > + /* > + * If we don't have any Platform Timer Structures, just return. > + */ > + if (!timer_count) > + return 0; > + > + /* > + * before really check all the Platform Timer Structures, > + * we assume they are GT block, and allocate memory for them. > + * We will free these memory once we finish the initialization. > + */ > + gt_blocks = kcalloc(timer_count, sizeof(*gt_blocks), GFP_KERNEL); > + if (!gt_blocks) > + return -ENOMEM; > + > + if (gtdt_arch_timer_mem_init(gt_blocks) > 0) { > + gt_timer = arch_timer_mem_get_timer(gt_blocks); > + if (!gt_timer) { > + pr_err("Failed to get mem timer info.\n"); > + goto error; > + } > + > + if (arch_timer_mem_use_virtual) > + timer_irq = gt_timer->virtual_irq; > + else > + timer_irq = gt_timer->irq; > + if (!timer_irq) { > + pr_err("Failed to get %s irq for mem timer.", > + arch_timer_mem_use_virtual ? "virt" : "phys"); > + goto error; > + } > + > + /* > + * According to ARMv8 Architecture Reference Manual(ARM), > + * the size of CNTBaseN frames of memory-mapped timer > + * is SZ_4K(Offset 0x000 – 0xFFF). > + */ > + timer_cntbase = ioremap(gt_timer->cntbase_phy, SZ_4K); > + if (!timer_cntbase) { > + pr_err("Failed to map mem timer base address.\n"); > + goto error; > + } > + > + ret = arch_timer_mem_register(timer_cntbase, timer_irq); > + if (ret) { > + iounmap(timer_cntbase); > + pr_err("Failed to register mem timer.\n"); > + goto error; > + } > + > + arch_counter_base = timer_cntbase; > + arch_timers_present |= ARCH_MEM_TIMER; > + } Same here. A lot of that is a duplication of the DT code. > + > +error: > + kfree(gt_blocks); > + return ret; > +} > + > +/* Initialize per-processor generic timer and memory-mapped timer(if present) */ > static int __init arch_timer_acpi_init(struct acpi_table_header *table) > { > int timer_count; > @@ -912,8 +1033,8 @@ static int __init arch_timer_acpi_init(struct acpi_table_header *table) > /* Get the frequency from CNTFRQ */ > arch_timer_detect_rate(NULL, NULL); > > - if (timer_count < 0) > - pr_err("Failed to get platform timer info, skipping.\n"); > + if (timer_count < 0 || arch_timer_mem_acpi_init((size_t)timer_count)) > + pr_err("Failed to initialize memory-mapped timer, skipping.\n"); > > return arch_timer_init(); > } > Thanks, M. -- Jazz is not dead. It just smells funny...