From patchwork Fri Mar 27 20:33:41 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mason X-Patchwork-Id: 6111831 Return-Path: X-Original-To: patchwork-linux-arm@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 426C1BF4A6 for ; Fri, 27 Mar 2015 20:36:41 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 6A634203EC for ; Fri, 27 Mar 2015 20:36:40 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.9]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id D1DB1203A4 for ; Fri, 27 Mar 2015 20:36:38 +0000 (UTC) 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 1YbaxN-0004zb-Mb; Fri, 27 Mar 2015 20:34:17 +0000 Received: from smtp4-g21.free.fr ([2a01:e0c:1:1599::13]) by bombadil.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1YbaxE-0004oE-Ox for linux-arm-kernel@lists.infradead.org; Fri, 27 Mar 2015 20:34:09 +0000 Received: from [192.168.1.37] (unknown [77.207.132.113]) (Authenticated sender: shill) by smtp4-g21.free.fr (Postfix) with ESMTPSA id 567DB4C8046; Fri, 27 Mar 2015 21:32:17 +0100 (CET) Message-ID: <5515BEA5.6040307@free.fr> Date: Fri, 27 Mar 2015 21:33:41 +0100 From: Mason User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:36.0) Gecko/20100101 Firefox/36.0 SeaMonkey/2.33.1 MIME-Version: 1.0 To: Marc Zyngier Subject: Re: Reading twd_base at run-time References: <55158261.9010108@free.fr> <551586E6.3020200@arm.com> In-Reply-To: <551586E6.3020200@arm.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20150327_133408_968385_116000FE X-CRM114-Status: GOOD ( 18.96 ) X-Spam-Score: -0.7 (/) Cc: Linux ARM X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00,FREEMAIL_FROM, RCVD_IN_DNSWL_MED, T_RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Heya, On 27/03/2015 17:35, Marc Zyngier wrote: > Hi Mason, > > On 27/03/15 16:16, Mason wrote: > >> In arch/arm/kernel/smp_twd.c, twd_local_timer_register() receives a >> struct twd_local_timer argument, which specifies >> >> 1) the physical address of twd_base >> 2) the twd interrupt number >> >> There's a helper to fill out the static struct: DEFINE_TWD_LOCAL_TIMER() >> >> But it seems to me (please correct me if I'm wrong) that the address >> of twd_base can be read at run-time, making it one less parameter to >> specify by hand at compile-time (with the risk that a HW engineer >> change the address in the next chip with no warning). > > The main problem with that is that said HW engineer has ^%$%^%-up > PERIPHBASE, and that you can't trust it. I've seen a number of these > kicking around. I don't understand your remark, perhaps mine was unclear? The value of PERIPHBASE is left to the implementer, right? So the HW engineer picks addrA for revA, then changes his mind for revB, and picks addrB. No screw-up there, right? The problem comes when said engineer forgets to notify the grunts in software that they're supposed to change #define PERIPH_BASE ADDR_A to #define PERIPH_BASE ADDR_B in the header files for revB. Looking up PERIPH_BASE at run-time through cp15 solves that particular issue. Do you disagree? >> Here's an incomplete patch to express my intent: >> >> diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c >> index 6591e26..5177db8 100644 >> --- a/arch/arm/kernel/smp_twd.c >> +++ b/arch/arm/kernel/smp_twd.c >> @@ -369,14 +369,14 @@ out_free: >> return err; >> } >> >> -int __init twd_local_timer_register(struct twd_local_timer *tlt) >> +int __init twd_local_timer_register(void) >> { >> if (twd_base || twd_evt) >> return -EBUSY; >> >> - twd_ppi = tlt->res[1].start; >> + twd_ppi = 29; >> >> - twd_base = ioremap(tlt->res[0].start, resource_size(&tlt->res[0])); >> + twd_base = ioremap(scu_a9_get_base() + 0x600, 0x10); >> if (!twd_base) >> return -ENOMEM; >> >> >> As far as I can tell, all platforms use 29 for twd_ppi, but I can make >> sure if people agree this patch is indeed an improvement. > > I don't think we'll see that many new platforms not using DT, so this > would effectively remove one line from the helper, and generate a bit of > churn on platforms that are usually left in a dusty corner... OK, I understand your point about what is now considered "legacy" code. I suppose it might even be removed in the not-too-distant future? That being said, the principle stands for the of function! By the way, have you seen my other thread? ("Dropping "depends on SMP" for HAVE_ARM_TWD") I CCed you as the author of twd_local_timer_of_register. Regards. diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c index 172c6a05..94fb0cd 100644 --- a/arch/arm/kernel/smp_twd.c +++ b/arch/arm/kernel/smp_twd.c @@ -391,13 +391,13 @@ static void __init twd_local_timer_of_register(struct device_node *np) if (!is_smp() || !setup_max_cpus) return; - twd_ppi = irq_of_parse_and_map(np, 0); + twd_ppi = 29; if (!twd_ppi) { err = -EINVAL; goto out; } - twd_base = of_iomap(np, 0); + twd_base = ioremap(scu_a9_get_base() + 0x600, 0x10); if (!twd_base) { err = -ENOMEM; goto out;