Date: Tue, 19 May 2015 13:01:18 +0000 From: "andrew (Andrew Turner)" <phabric-noreply@FreeBSD.org> To: freebsd-arm@freebsd.org Subject: [Differential] [Updated] D2579: PCI support for Alpine platform from Annapurna Labs Message-ID: <710c664cf05eb310be96af7f8827b4d1@localhost.localdomain> In-Reply-To: <differential-rev-PHID-DREV-y2vdtziki6lwmyrnc5ux-req@FreeBSD.org> References: <differential-rev-PHID-DREV-y2vdtziki6lwmyrnc5ux-req@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
andrew added a comment.
This is from my first pass over the driver.
INLINE COMMENTS
sys/arm/annapurna/alpine/alpine_pci.c:247 When would not be defined?
sys/arm/annapurna/alpine/alpine_pci.c:284-307 This code looks similar to `sys/powerpc/ofw/ofw_pci.c` `ofw_pci_nranges` but using fdt_* in place of the correct OF_* functions. It would be nice to not hve to do the same parsing each time.
sys/arm/annapurna/alpine/alpine_pci.c:414 Why the else given the return above?
sys/arm/annapurna/alpine/alpine_pci.c:422 (bus)?
sys/arm/annapurna/alpine/alpine_pci.c:429 What is magic about 0xff?
sys/arm/annapurna/alpine/alpine_pci.c:441
Extra newline.
sys/arm/annapurna/alpine/alpine_pci.c:450 Should these be under `bootverbose`?
sys/arm/annapurna/alpine/alpine_pci.c:479 0xff?
sys/arm/annapurna/alpine/alpine_pci.c:496 No need for the brace, and is this check needed? When would `ofw_bus_search_compatible` match something that's not a pci device?
sys/arm/annapurna/alpine/alpine_pci.c:527 This doesn't look how we would normally structure this, it would be something like:
struct al_pcie_atu_region io_atu_region;
if (sc->sc_type == AL_PCI_TYPE_INTERNAL)
return (0);
io_atu_region.enable = AL_TRUE;
io_atu_region.direction = AL_PCIE_ATU_DIR_OUTBOUND;
...
sys/arm/annapurna/alpine/alpine_pci.c:547 Should this be under bootverbose?
sys/arm/annapurna/alpine/alpine_pci.c:596 We have `setbit`, `clrbit`, `isset`, and `isclr` in `sys/param.h` for this sort of thing.
sys/arm/annapurna/alpine/alpine_pci.c:679 What's special about that value and why does it need to be written before the value can be read?
sys/arm/annapurna/alpine/alpine_pci.c:733 Shouldn't this be a `bus_size_t`?
sys/arm/annapurna/alpine/alpine_pci.c:847 What's special about 5?
sys/arm/annapurna/alpine/alpine_pci.c:962 Why 12?
sys/arm/annapurna/alpine/alpine_pci.c:979 You can combine `v` and `tmp` on one line, and `mps` and `reg` on another.
sys/arm/annapurna/alpine/alpine_pci.c:1098 `(hdrtype & PCIM_MFDEV) != 0`
sys/arm/annapurna/alpine/alpine_pci.c:1180 This looks wrong, why are you parsing the fdt data in the clikd class? It should be being provided by the parent, then you use `bus_alloc_resource` or similar to get the resource.
sys/arm/annapurna/alpine/alpine_pci.c:1221 Don't use `fdtbus_bs_tag` in this driver, it should come from the parent. `fdtbus_bs_tag` should only be used by the early console.
sys/arm/annapurna/alpine/alpine_pci.c:1240 Doesn't this leave the hardware partially configuredon failure?
sys/arm/annapurna/alpine/alpine_pci.c:1391 Why `0xffffffff`?
sys/arm/annapurna/alpine/alpine_pci.c:1470 Unneeded
REVISION DETAIL
https://reviews.freebsd.org/D2579
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?710c664cf05eb310be96af7f8827b4d1>