Date: Wed, 29 Apr 2015 16:01:33 +0000 From: "andrew (Andrew Turner)" <phabric-noreply@FreeBSD.org> To: freebsd-arm@freebsd.org Subject: [Differential] [Commented On] D2340: Support for Alpine platform from Annapurna Labs Message-ID: <5655869a8db86a285004290a10fa6258@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 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?5655869a8db86a285004290a10fa6258>