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

[imp (Warner Losh)]

imp added a comment.

Decent start, want to know more about the future of the HAL. Is this a one-off never to be repeated HAL release, or will there be ongoing releases?

INLINE COMMENTS
  sys/arm/annapurna/alpine/alpine_machdep.c:2 Here and elsewhere: is this date right?
  sys/arm/annapurna/alpine/alpine_machdep.c:165 Perhaps this belongs in the main db_machdep.c?
  
  sys/arm/annapurna/alpine/alpine_machdep.c:199 ditto? Both with appropriate #ifdef maybe?
  sys/arm/annapurna/alpine/alpine_machdep_mp.c:143 Shouldn't this be a kassert?
  
  sys/arm/annapurna/alpine/alpine_machdep_mp.c:172 kassert?
  sys/arm/annapurna/alpine/alpine_pci.c:84 Given how this macro is used in this file, I might be tempted to make it available under bootverbose instead of 'DEBUG'
  sys/arm/annapurna/alpine/alpine_pci.c:667 ~0 is signed, but this takes an unsigned value. 0xfffffffful is likely a better match.
  
  sys/arm/annapurna/alpine/alpine_pci.c:1097 Why enable these explicitly here? Drivers are supposed to do so...
  sys/arm/annapurna/alpine/alpine_pci.c:1138 but here it's OK since the bridge is supposed to  do this on enable...
  sys/arm/annapurna/alpine/alpine_pci.c:1372 why do this if it is unused?
  sys/arm/annapurna/alpine/alpine_pci.c:1378 ~3 seems magic.
  
  sys/arm/annapurna/alpine/alpine_pci.c:1387 Most places use NBBY instead of a bare 8 for 8 bits in a byte.
  sys/arm/annapurna/alpine/alpine_pci.c:1412 why get it?
  
  sys/arm/annapurna/alpine/hal/al_hal_iofic.h:1 For the HAL files, will we need to worry about updates? For Octeon/Cavium, we imported their SDK/HAL routines separately. Does it make sense to do that here?

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

To: jpa-semihalf.com, ian, andrew, imp
Cc: freebsd-arm