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

[andrew (Andrew Turner)]

andrew added a comment.

First pass though the change.


INLINE COMMENTS
  sys/arm/annapurna/alpine/alpine_machdep.c:144 What is this for, and why does it need a fixed virtual address?
  sys/arm/annapurna/alpine/alpine_machdep.c:169 These should use the functions in <machine/cpu-v6.h>
  sys/arm/annapurna/alpine/alpine_machdep_mp.c:91 You should use ofw_cpu_early_foreach for this.
  sys/arm/annapurna/alpine/alpine_pci.c:310 What is this magic number? And, in general the style is:
  ```
  if ((cell0 & MAGIC_VALUE) == MAGIC_VALUE)
  ```
  or:
  ```
  if ((cell0 & MAGIC_VALUE) != 0)
  ```
  sys/arm/annapurna/alpine/alpine_pci.c:372 What's special about 255?
  sys/arm/annapurna/alpine/alpine_pci.c:375 And why are you shifting these variables by this amount?
  sys/arm/annapurna/alpine/alpine_pci.c:399 Workaround for what?
  sys/arm/annapurna/alpine/alpine_pci.c:477 Don't use fdt_ functions here, use the ofw_ version
  sys/arm/annapurna/alpine/alpine_pci.c:494 These should be using ofw_bus_search_compatible, and the probe functions shouldn't be setting bits of the softc, move those to the attach function.
  sys/arm/annapurna/alpine/alpine_pci.c:762 This is an odd spelling of powerof2(n)
  sys/arm/annapurna/alpine/alpine_pci.c:904 What's special about 5?
  sys/arm/annapurna/alpine/alpine_pci.c:980 What's special about 12?
  sys/arm/annapurna/alpine/alpine_pci.c:1063 What are all these magic numbers?
  sys/arm/annapurna/alpine/alpine_pci.c:1476 I assume 4 as some meaning?
  sys/arm/annapurna/alpine/files.alpine:11 you don't need the cpufunc_asm_ files here.
  sys/arm/annapurna/alpine/std.alpine:9 I don't think this is needed.
  sys/arm/annapurna/alpine/std.alpine:12 You only need to set KERNPHYSADDR if you need kernel.bin
  sys/arm/annapurna/alpine/std.alpine:15 This will probably cause issues with loader.
  sys/arm/conf/ALPINE:50 Can you try to update this file to be closer to the other armv6 configs. for example the above is missing a number of options.
  sys/arm/conf/ALPINE:64 Why is this hard coded?
  sys/arm/conf/ALPINE:110 Do you need a static dtb?
  sys/boot/fdt/dts/arm/annapurna-alpine.dts:2 How similar is this to the upstream dts? We are aiming to use dts files as close to the Linux version as possible.

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

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

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