Date: Mon, 11 May 2015 12:19:30 +0000 From: "andrew (Andrew Turner)" <phabric-noreply@FreeBSD.org> To: freebsd-arm@freebsd.org Subject: [Differential] [Commented On] D2377: Introduce ARM GICv3 support Message-ID: <23af7fad03a81d84c1668be246da3370@localhost.localdomain> In-Reply-To: <differential-rev-PHID-DREV-piql4a7htwxjub743777-req@FreeBSD.org> References: <differential-rev-PHID-DREV-piql4a7htwxjub743777-req@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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<tab>_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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?23af7fad03a81d84c1668be246da3370>