From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757896AbcBBU0G (ORCPT ); Tue, 2 Feb 2016 15:26:06 -0500 Received: from mout.kundenserver.de ([217.72.192.75]:49785 "EHLO mout.kundenserver.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757484AbcBBU0D (ORCPT ); Tue, 2 Feb 2016 15:26:03 -0500 From: Arnd Bergmann To: Joao Pinto Cc: Vineet.Gupta1@synopsys.com, helgaas@kernel.org, linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org, linux-snps-arc@lists.infradead.org, CARLOS.PALMINHA@synopsys.com, Alexey.Brodkin@synopsys.com, robh+dt@kernel.org, pawel.moll@arm.com, mark.rutland@arm.com, ijc+devicetree@hellion.org.uk, galak@codeaurora.org Subject: Re: [PATCH v7 2/2] add new platform driver for PCI RC Date: Tue, 02 Feb 2016 21:25:25 +0100 Message-ID: <8223655.W63T0XOVm1@wuerfel> User-Agent: KMail/4.11.5 (Linux/3.16.0-10-generic; KDE/4.11.5; x86_64; ; ) In-Reply-To: <10b29adcdb0a31eb8f8071b271da267e44ad8a04.1454349430.git.jpinto@synopsys.com> References: <10b29adcdb0a31eb8f8071b271da267e44ad8a04.1454349430.git.jpinto@synopsys.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-Provags-ID: V03:K0:L/k2mY4ZOx4wE1Io5NRpFATSFuwLGh3kw+8ctFLDQsC5fsvgSmz M1BsZXDudOEELHfMUydhocOl0zxcPDVcanU6EdabqxsJhlI5w+BT7JAWtSMd2+uPn2DgOBA lhooGe8V0XYAYvYg7417y6Z09R6FQ5aX+oY5z/8TWhrFcV4pEEnBIMfM1PaRNr/TykgLCW/ MSrAShwm+gGYH1Q6vdFTA== X-UI-Out-Filterresults: notjunk:1;V01:K0:Zj6drMLJaTw=:djS/hgUSh80ijmh1inKt2T wpx2h4TovWIL1/Ym5vCpS3ovq+waQVzPZMyG8RXU5svOkWGkILebr6VLOhNVJx78Nr7bnxLX0 KaVx/jMd+Qm1Qe82Hg5a39TSDuMhGy/vx8fFQZu8bwsQugG7REPYrc4tAHZkawPu/S2+TliWS y+96//tyhoKAhqDY1CSXrVjAvIjK0/b992rkl+u6aucKTjUxpK5eHxzpwEZutMAeiWAn/zVAZ gt4lSmkr5wtC+oLDhG8z2ySiYuqUF3qsi4KzxOBOafDWeJyOJonvDgd2PCDyU0klzgClpxd2n S8A5gv2RPVzfSFuXwA0G2XymhUHeHcx4n5Q92irQhQ08cWZUlurI4Z+26Bg/zrgBIXDm0NbCR wTFch/Zpk+da1BjOCw8V7vUVyShNJgUjAl+Oi1Wr2M2MozF37jjX3cbf76fC89QxoyPeo24pK O0eai43L/CBN9ADRguLf/+ZwXelDUd2mNjN4k8m/ZWjGFopCiqfdu4gzakYhQYjZukH878f5p QRE02dt+88kmXIQrMhi4g+9BeLDYdHcHxt3J6TWDXvioab36BudLTUlPGfO/hO0ws6ulWQ4mY ioyGQxi/9AaVrqKw+vswb3QTTiOhay44Pgxt+Jhodo40ihFsYMrba8QKqsiBIlvBrtdk5OiKQ sWEYxs7BiXQIBatJkTvOY0Fq69MMFcLhXA2fB2h4zkUWWOHFulp6VTigcUU2yR1cW2lPtHiE+ FssoGIAN3wGRtfLu Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Monday 01 February 2016 18:07:45 Joao Pinto wrote: > This patch adds a new driver that will be the reference platform driver > for all PCI RC IP Protoyping Kits based on ARC SDP. > This patch is composed by: > > -MAINTAINERS file was updated to include the new driver > -Documentation/devicetree/bindings/pci was updated to include the new > driver documentation > -New driver called pcie-synopsys > > Signed-off-by: Joao Pinto I must have completely missed all the earlier submissions and just happened to see this one now that it was merged. I have a couple of comments, maybe you can send a follow up patch to address them: > +---------------------------------- > + > +This is the reference platform driver to be used in the Synopsys PCI Root > +Complex IP Prototyping Kit. > + > +Required properties: > +- compatible: set to "snps,pcie-synopsys"; Please also include the exact version of the designware PCIe part that is used in here, such as "snps,dw-pcie-1.234". I'm sure you can find out the version. "snps,pcie-synopsys" by itself is about the worst possible compatible string because it says nothing about the specific hardware. Half the machines we support all have a designware PCIe host that was made by synopsys. Please use the name of the SoC for the most specific string. > +- reg: base address and length of the pcie controller registers. > +- #address-cells: set to <3> > +- #size-cells: set to <2> > +- device_type: set to "pci" > +- ranges: ranges for the PCI memory and I/O regions. > +- interrupts: one interrupt source for MSI interrupts, followed by interrupt > + source for hardware related interrupts. > +- #interrupt-cells: set to <1> > +- num-lanes: set to <1>; > + > +Example configuration: > + > + pcie: pcie@0xdffff000 { > + compatible = "snps,pcie-synopsys"; > + reg = <0xdffff000 0x1000>; > + #address-cells = <3>; > + #size-cells = <2>; > + device_type = "pci"; > + ranges = <0x00000800 0 0xd0000000 0xd0000000 0 0x00002000>, This line looks misplaced here and does not match the documentation. It's probably a leftover from an earlier version that had the config space in the ranges, so you need to remove that. > +/* This handler was created for future work */ > +static irqreturn_t synopsys_pcie_irq_handler(int irq, void *arg) > +{ > + return IRQ_NONE; > +} It's not harmful, but we generally don't add code "for future work", better remove it for now. > +static void synopsys_pcie_establish_link(struct pcie_port *pp) > +{ > + int retries = 0; > + > + /* check if the link is up or not */ > + for (retries = 0; retries < 10; retries++) { > + if (dw_pcie_link_up(pp)) { > + dev_info(pp->dev, "Link up\n"); > + return; > + } > + mdelay(100); > + } > +} That mdelay() needs to be an msleep(). You should never waste a whole second worth of CPU time in a driver. Arnd