From mboxrd@z Thu Jan 1 00:00:00 1970 From: Bin Meng Date: Fri, 8 Jan 2016 09:10:35 +0800 Subject: [U-Boot] [PATCH 1/3] pci: Add error values definitions from the kernel In-Reply-To: <201601080135.20074.marex@denx.de> References: <1452113598-32409-1-git-send-email-festevam@gmail.com> <201601080135.20074.marex@denx.de> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Marek, On Fri, Jan 8, 2016 at 8:35 AM, Marek Vasut wrote: > On Friday, January 08, 2016 at 01:31:17 AM, Bin Meng wrote: >> On Fri, Jan 8, 2016 at 8:15 AM, Fabio Estevam wrote: >> > On Thu, Jan 7, 2016 at 10:02 PM, Bin Meng wrote: >> >> What new feature would benefit from this? These two PCIe drivers are >> >> non-DM drivers and DM PCI is not utilizing those error codes, instead >> >> DM PCI is using U-Boot standard error codes. >> > >> > but what prevents DM PCI to use the kernel error codes in the future? >> > >> > Returning PCIBIOS_DEVICE_NOT_FOUND when the config is invalid is a >> > common pattern in the kernel. >> > >> > Take a look at these drivers: >> > >> > drivers/pci/access.c >> > drivers/pci/host/pci-mvebu.c >> > drivers/pci/host/pci-xgene.c >> > drivers/pci/host/pcie-altera.c >> > drivers/pci/host/pcie-designware.c >> > drivers/pci/host/pcie-rcar.c >> > drivers/pci/xen-pcifront.c >> > >> > I don't see why we can't do the same in U-boot. >> >> I am sorry but this does not convince me. Kernel is using these codes, >> but I don't see any PCI library codes in kernel that actually parses >> these error codes. >> >> > Feel free to submit a patch with your proposal and the maintainer can >> > then decide which one is more adequate for U-boot. >> >> My points are: >> 1). These two drivers are non-DM drivers. We should start converting >> them to DM drivers first instead of adding new stuff which is only >> used by legacy drivers. I have pcie_layerscape on my TODO list. > > This point is invalid, I see this patch series is just fixing bugs and > converting drivers to DM is _far_ beyond the scope of this series. I am not indicating we should convert these drivers to DM to _fix_ this simple issue. I was saying we should be careful to introduce new stuff to legacy drivers from now on given DM PCI is already in a good shape. > >> 2). We should stick to the correct behavior. The PCIe controller is >> buggy that it cannot return 0xFFFFFFFF by hardware, like other >> controllers do. IMHO it's completely a horrible controller, that >> comments in ls_pcie_addr_valid() says: "Controller does not support >> multi-function in RC mode". > > Can you elaborate on why do you think it is a horrible controller ? > Most of the controllers I saw used on ARM were this way. First it does not return 0xFFFFFFFF automatically by hardware when probing non-existent devices. This is suggested by the PCI spec. See the following adopted from PCI local bus spec 3.0 chapter 6.1 Configuration Space Organization: System software may need to scan the PCI bus to determine what devices are actually present. To do this, the configuration software must read the Vendor ID in each possible PCI "slot." The host bus to PCI bridge must unambiguously report attempts to read the Vendor ID of non-existent devices. Since FFFFh is an invalid Vendor ID, it is adequate for the host bus to PCI bridge to return a value of all 1's on read accesses to Configuration Space registers of non-existent devices. (Note that these accesses will be terminated with a Master-Abort.) AFAIK, all Intel chipsets behave this way (no surprise, PCI spec was originally developed by Intel). Freescale's MPC85xx, QorIQ PowerPC processors' PCI/PCIe controllers behave this way too. Secondly, from the comment "Controller does not support multi-function in RC mode", if this is true, then why? I understand that it is a case when the PCIe IP isn't mature or in a very early IP validation phase I would categorize into hardware errata. But looks this Designware IP is widely used in ARM systems today in many SoCs. It's already in production phase but it still has such limitation? "Most of the controllers I saw used on ARM were this way" does not justify it is not horrible. I even saw more "horrible" PCI controller in an ARM SoC from Mindspeed years ago, that accessing PCI memory space from CPU requires indirect access, IOW it is not memory-mapped, directly accessible. > >> 3). If we want to align with kernel, I want to see a plan of at least >> adopting these error codes in DM PCI. This is something which is not >> clear for now. > > No, this is again far beyond the scope of this series. I want PCI on iMX > and LS to work in the current release, period. So from my side, I want > these patches to go in. > I raised this for open discussion. I was worried that we borrowed this stuff from kernel to U-Boot just for fixing this specific issue, and we never touch this again. >> Simon, what's your opinion on this? >> I just figured out a solid reason why this is not a correct fix but still a workaround. Regards, Bin