From owner-freebsd-arm@FreeBSD.ORG Tue May 19 13:01:18 2015 Return-Path: Delivered-To: freebsd-arm@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 92953950 for ; Tue, 19 May 2015 13:01:18 +0000 (UTC) Received: from phabric-backend.isc.freebsd.org (phabric-backend.isc.freebsd.org [IPv6:2001:4f8:3:ffe0:406a:0:50:2]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 7744B1719 for ; Tue, 19 May 2015 13:01:18 +0000 (UTC) Received: from phabric-backend.isc.freebsd.org (phabric-backend.isc.freebsd.org [127.0.1.5]) by phabric-backend.isc.freebsd.org (8.14.9/8.14.9) with ESMTP id t4JD1IHj015764 for ; Tue, 19 May 2015 13:01:18 GMT (envelope-from daemon-user@phabric-backend.isc.freebsd.org) Received: (from daemon-user@localhost) by phabric-backend.isc.freebsd.org (8.14.9/8.14.9/Submit) id t4JD1I2s015763; Tue, 19 May 2015 13:01:18 GMT (envelope-from daemon-user) Date: Tue, 19 May 2015 13:01:18 +0000 To: freebsd-arm@freebsd.org From: "andrew (Andrew Turner)" Subject: [Differential] [Updated] D2579: PCI support for Alpine platform from Annapurna Labs Message-ID: <710c664cf05eb310be96af7f8827b4d1@localhost.localdomain> X-Priority: 3 Thread-Topic: D2579: PCI support for Alpine platform from Annapurna Labs X-Herald-Rules: none X-Phabricator-To: X-Phabricator-To: X-Phabricator-To: X-Phabricator-To: X-Phabricator-Cc: In-Reply-To: References: Thread-Index: NjYxMzQxMmI2MjBmNTRlMzhjMzBkMTMzMmNhIFVbNB4= Precedence: bulk X-Phabricator-Sent-This-Message: Yes X-Mail-Transport-Agent: MetaMTA X-Auto-Response-Suppress: All X-Phabricator-Mail-Tags: , MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Type: text/plain; charset="utf-8" X-BeenThere: freebsd-arm@freebsd.org X-Mailman-Version: 2.1.20 List-Id: "Porting FreeBSD to ARM processors." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 19 May 2015 13:01:18 -0000 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