[Differential] [Commented On] D2377: Introduce ARM GICv3 support

[andrew (Andrew Turner)]

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