From owner-freebsd-arm@FreeBSD.ORG Mon May 11 12:19:31 2015 Return-Path: Delivered-To: freebsd-arm@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 007C7467 for ; Mon, 11 May 2015 12:19:30 +0000 (UTC) Received: from phabric-backend.isc.freebsd.org (phabric-backend.isc.freebsd.org [IPv6:2001:4f8:3:ffe0:406a:0:50:2]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id D58071986 for ; Mon, 11 May 2015 12:19:30 +0000 (UTC) Received: from phabric-backend.isc.freebsd.org (phabric-backend.isc.freebsd.org [127.0.1.5]) by phabric-backend.isc.freebsd.org (8.14.9/8.14.9) with ESMTP id t4BCJUkx093911 for ; Mon, 11 May 2015 12:19:30 GMT (envelope-from daemon-user@phabric-backend.isc.freebsd.org) Received: (from daemon-user@localhost) by phabric-backend.isc.freebsd.org (8.14.9/8.14.9/Submit) id t4BCJUDG093910; Mon, 11 May 2015 12:19:30 GMT (envelope-from daemon-user) Date: Mon, 11 May 2015 12:19:30 +0000 To: freebsd-arm@freebsd.org From: "andrew (Andrew Turner)" Subject: [Differential] [Commented On] D2377: Introduce ARM GICv3 support Message-ID: <23af7fad03a81d84c1668be246da3370@localhost.localdomain> X-Priority: 3 Thread-Topic: D2377: Introduce ARM GICv3 support X-Herald-Rules: <28>, <31>, <32>, <34>, <8> X-Phabricator-To: X-Phabricator-To: X-Phabricator-To: X-Phabricator-To: X-Phabricator-To: X-Phabricator-To: X-Phabricator-To: X-Phabricator-To: X-Phabricator-Cc: X-Phabricator-Cc: X-Phabricator-Cc: X-Phabricator-Cc: X-Phabricator-Cc: In-Reply-To: References: Thread-Index: MjA1ZGZkZWI5MTE4ZDk2Nzk0YzAyZGRiMWRiIFVQnlI= Precedence: bulk X-Phabricator-Sent-This-Message: Yes X-Mail-Transport-Agent: MetaMTA X-Auto-Response-Suppress: All X-Phabricator-Mail-Tags: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Type: text/plain; charset="utf-8" X-BeenThere: freebsd-arm@freebsd.org X-Mailman-Version: 2.1.20 List-Id: "Porting FreeBSD to ARM processors." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 11 May 2015 12:19:31 -0000 andrew added inline comments. INLINE COMMENTS sys/arm64/arm64/gic_v3.c:151 4 lines of magic numbers sys/arm64/arm64/gic_v3.c:163 Why the extra inderection? It's calling a list of 3 functions that don't change. sys/arm64/arm64/gic_v3.c:191 Why do we need `i` in this loop? And there could be a comment on what resources are being released, i.e. why `sc->gic_redists.nregions + 1`. sys/arm64/arm64/gic_v3.c:319 This magic value should come from armreg.h sys/arm64/arm64/gic_v3.c:341 `if ((sre & ICC_SRE_EL1_SRE) != 0) {` sys/arm64/arm64/gic_v3.c:391 Magic number, should be something like: #define GICD_INTS_PER_foo 16 sys/arm64/arm64/gic_v3.c:395 And here sys/arm64/arm64/gic_v3.c:404 And here sys/arm64/arm64/gic_v3.c:473 Magic number, should be something like: #define GICR_TYPER_foo_SHIFT 32 sys/arm64/arm64/gic_v3.c:540 `if (err != 0)` sys/arm64/arm64/gic_v3.c:555 Magic number sys/arm64/arm64/gic_v3.c:563 If there is no clean up you can just return above. sys/arm64/arm64/gic_v3_fdt.c:81 You should create a parent bus for these, then add them there. sys/arm64/arm64/gic_v3_fdt.c:135 err is not a bool, it should be `if (err != 0)`, or change it to if (err != 0) { /* The failure case below */ if (bootverbose) { ... } gic_v3_fdt_detach(dev); } return (err); sys/arm64/arm64/gic_v3_fdt.c:155 You don't need this. The parent should set the detach functions to `gic_v3_detach`, then don't set one in the child. The child should only define a function if it needs to do something extra. sys/arm64/arm64/gic_v3_reg.h:31 Should be `#define_GIC...` sys/arm64/arm64/gic_v3_var.h:79 No need for `({` and `})` sys/arm64/arm64/locore.S:192 What is the GIC field? It's generally better to explain what is happening when writing asm so you still understand what is happening in 6 months time. sys/arm64/include/armreg.h:106 This should be `ICC_CTLR_EL1_EOIMODE` sys/arm64/include/armreg.h:112 I don't see this in the ARMv8 ARM, is it documented differently in the GICv3 docs? sys/arm64/include/armreg.h:113 In general I've tried to add all the used bits when adding new register values, and the registers should be alphabetically sorted by name. REVISION DETAIL https://reviews.freebsd.org/D2377 EMAIL PREFERENCES https://reviews.freebsd.org/settings/panel/emailpreferences/ To: zbb, emaste, ian, imp, andrew, brueffer, joel, wblock Cc: kostikbel, emaste, andrew, imp, freebsd-arm