Date: Wed, 22 Apr 2015 09:27:43 +0000 From: "andrew (Andrew Turner)" <phabric-noreply@FreeBSD.org> To: freebsd-arm@freebsd.org Subject: [Differential] [Updated] D2340: Support for Alpine platform from Annapurna Labs Message-ID: <a12a2546a49e1c4432d8f960b1940cb1@localhost.localdomain> In-Reply-To: <differential-rev-PHID-DREV-gclxe3zq7xamd5hpjagv-req@FreeBSD.org> References: <differential-rev-PHID-DREV-gclxe3zq7xamd5hpjagv-req@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?a12a2546a49e1c4432d8f960b1940cb1>