[Differential] [Commented On] D2340: Support for Alpine platform from Annapurna Labs

[andrew (Andrew Turner)]

andrew added inline comments.

INLINE COMMENTS
  sys/arm/annapurna/alpine/alpine_machdep.c:144 It's only this way on mv and at91. On all other SoCs we don't reserve a virtual address, instead we let the vm code do it for us. In the at91 case it is to help transition to fdt, however this is not the case here.
  
  Moreover, the only place I can find that uses `fdt_immr_pa`, other than in the fdt code to set it, is in sys/arm/mv. I would prefer we don't use these unless they are absolutely needed, and if this is the case we would need a comment explaining why this is the case.
  sys/arm/annapurna/alpine/alpine_machdep.c:69 These should be `#define<tab>AL_...`
  sys/arm/annapurna/alpine/alpine_machdep_mp.c:96 This should use a function from cpu-v6.h
  sys/arm/annapurna/alpine/alpine_machdep_mp.c:97 You should add something like the following to armreg.h, then use it here.
  
    #define L2CTLR_NPROC_SHIFT 24
    #define L2CTLR_NPROC(r) ((((r) >> L2CTLR_NPROC_SHIFT) & 3) + 1)
  sys/arm/annapurna/alpine/alpine_machdep_mp.c:146 Do these need to be fdt_ functions, or could you call an ofw_ function?
  sys/arm/annapurna/alpine/alpine_machdep_mp.c:201 uint32_t is the wrong type for a number of these.
  sys/arm/annapurna/alpine/alpine_machdep_mp.c:262 Is a dmb enough here?
  sys/arm/annapurna/alpine/alpine_pci.c:86 I suspect what Warner was asking was to remove device_printf_dbg, and use the following in it's place:
  
    if (bootverbose)
      device_printf(dev, "message\n");
  
  sys/arm/annapurna/alpine/alpine_pci.c:238 Are you sure about this?
  sys/arm/annapurna/alpine/alpine_pci.c:382 Why is addr a `void **`?
  sys/arm/annapurna/alpine/alpine_pci.c:633 Where in this file are you referencing? I don't see anything it could be.
  sys/arm/annapurna/alpine/alpine_pci.c:901 Can you collect these at the top of the file.
  sys/arm/annapurna/alpine/alpine_pci.c:1159 We normally name a device_t dev
  sys/arm/annapurna/alpine/alpine_pci.c:1179 fdt_
  sys/arm/annapurna/alpine/alpine_pci.c:1180 (There are a few) extra (parenthesis). And reg_size is not a boolean, it should be `reg_size > 0`
  sys/arm/annapurna/alpine/alpine_pci.c:1347 Extra newline
  sys/arm/annapurna/alpine/alpine_pci.c:1404 This is missing a few spaces.
  sys/arm/arm/machdep.c:167 This could be split out to a new review.

REVISION DETAIL
  https://reviews.freebsd.org/D2340

EMAIL PREFERENCES
  https://reviews.freebsd.org/settings/panel/emailpreferences/

To: jpa-semihalf.com, ian, andrew, imp
Cc: meloun-miracle-cz, onwahe-gmail-com, freebsd-arm