From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it0-x22f.google.com (mail-it0-x22f.google.com [IPv6:2607:f8b0:4001:c0b::22f]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 93EBB81DFE for ; Mon, 14 Nov 2016 12:52:47 -0800 (PST) Received: by mail-it0-x22f.google.com with SMTP id c20so104401581itb.0 for ; Mon, 14 Nov 2016 12:52:52 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=semihalf-com.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=H0KVOxJAX6+5dlKIptaqPC1kGU0gFtwrxpeS0MLXVQU=; b=KCB4+uwZAbug1gCHmNNXyD8K//yBGkSr9tMiU48TwLqezY1Q7a4Eu4ooCvIvO3KCFt yIMSqZlAdaiT00+mMKt+hMdmUvmzOwUzIrdbfFIvqtpykS7LNqkeRhJOIo8Hytl0kbv1 B5aQNTMp8Pu6S2xleFUBrlOW4qbjV4j0Md3hwhV9w1mL49eMsqzjpoaA+ncNaQpIfhC3 BOqC8GPZiMch5JgVVkfHJXcgWhB67L080Y9P2Y06IhApzZ4xUgDms1LhTMRhKuQn4UYp Pf/RPnexQDSJQF5sbhIK4j2C7NXaXt1pjiHDHaC4mpml/9BfocC3ZyLok3d8E0zXAFLT RaWA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=H0KVOxJAX6+5dlKIptaqPC1kGU0gFtwrxpeS0MLXVQU=; b=gVb7n1JNEDE9yzZxiHfd6vN6fr5BghpLTZ9S+MF4cPx0739DPaF2NHnHtnhAY5gd4W 1q2qfOwU6h5PCaNylFUBy0cmtinKWmrs+1+BUOXsee0rOn4sxYhQlyjM2Da9mFFv6dqU DIw1NcpBE50bVa2TIj4nPJNFl+jH+0CxIs8W+Up++oplnCfo2adt/cLh0KSb0IGTUCTg ALxiNgk80cUFB98Z+eYZHPI+10g4q0053+HtItT2F7MM644wTT7zWdXxg+Z+oBRGBNWk dHe3etFI68Q6FKVVcJ1ssjYn1X6It86IqzFF895W+kfVwkKjRpt+7v5VFgylLkLWzE7x v+pg== X-Gm-Message-State: ABUngvfM2B7nqQLue2iDKZPDO2P9faRdwlhxygEKOto3sbrZTX1ycQ8ymqdeb0iTzaKQXk9I5antAt8b4DBfmw== X-Received: by 10.36.101.5 with SMTP id u5mr241178itb.45.1479156771304; Mon, 14 Nov 2016 12:52:51 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.25.140 with HTTP; Mon, 14 Nov 2016 12:52:50 -0800 (PST) In-Reply-To: <1478173239-22270-1-git-send-email-ard.biesheuvel@linaro.org> References: <1478173239-22270-1-git-send-email-ard.biesheuvel@linaro.org> From: Marcin Wojtas Date: Mon, 14 Nov 2016 21:52:50 +0100 Message-ID: To: Ard Biesheuvel Cc: edk2-devel@lists.01.org, Leif Lindholm , "Kinney, Michael D" , afish@apple.com, "Tian, Feng" , star.zeng@intel.com Subject: Re: [PATCH v2 0/5] MdeModulePkg: add support for non-discoverable devices X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 14 Nov 2016 20:52:47 -0000 Content-Type: text/plain; charset=UTF-8 Hi Ard, I tested your solution on Marvell Armada 70x0, whose upstream process to OpenPlatformPkg is being finalized now. So far I was able to check generic XHCI IP (MdeModulePkg/Bus/Pci/XhciDxe) and custom SD/MMC driver - they both work fine with following modification in your code: --- a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c +++ b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c @@ -942,7 +942,7 @@ InitializePciIoProtocol ( Dev->ConfigSpace.Hdr.ClassCode[1] = PCI_CLASS_SERIAL_USB; Dev->ConfigSpace.Hdr.ClassCode[2] = PCI_CLASS_SERIAL; Dev->BarIndex = 0; - Dev->BarSize = SIZE_2KB; + Dev->BarSize = SIZE_16KB; break; case NonDiscoverableDeviceTypeAhci: @@ -958,7 +958,7 @@ InitializePciIoProtocol ( Dev->ConfigSpace.Hdr.ClassCode[1] = PCI_SUBCLASS_SD_HOST_CONTROLLER; Dev->ConfigSpace.Hdr.ClassCode[2] = PCI_CLASS_SYSTEM_PERIPHERAL; Dev->BarIndex = 0; - Dev->BarSize = 0x100; + Dev->BarSize = 0x300; break; Any value lower than SIZE_16KB of BarSize resulted in XhcReadExtCapReg function failing due to reaching out of declared space. So happened with custom SD/MMC driver. Is it a problem to extend the space? Did you consider extending RegisterNonDiscoverableDevice() function with possible custom BarSize value? I think enabling the users overriding the default value could be possible. What do you think? I also reviewed the code during debug and have no objections. Best regards, Marcin 2016-11-03 12:40 GMT+01:00 Ard Biesheuvel : > This v2 is based on 'EmbeddedPkg: generic support for reusing PCI drivers for > platform devices' [0] but it has seen some updates that justified changing the > name, and at Leif's request, it now targets MdeModulePkg not EmbeddedPkg. > > The rationale for this series is the fact that many ARM platforms implement > some form of PCI 'emulation', to allow non-discoverable devices that implement > standardized host controller interfaces (i.e., EHCI, AHCI) to be controlled by > the generic EDK2 drivers, which are layered on top of the PCI I/O protocols > (even though the respective host controller specifications don't mandate that) > > There are a couple of problems with that approach: > - Most implementations are based on the original code created for BeagleBoard, > which is a 32-bit platform. Unlike x86, which usually does not perform PCI > DMA above 4 GB, the ARM ecosystem is much more heterogeneous, and platforms > that have memory both above and below the 4 GB mark may ship with, e.g., EHCI > controllers that do not implement 64-bit DMA addressing. > - Implementations depend on the DmaLib library class in EmbeddedPkg, of which > coherent and non-coherent implementations exists. However, both types of > devices may appear on a single platform, requiring several instances of the > same driver. > - Existing implementations do not follow the UEFI driver model, but instantiate > a fixed number of PCI I/O protocol handles, and bring up all the devices when > doing so. However, the UEFI philosophy is to only instantiate (and thus > initialize) devices that are involved in booting. > > So instead, let's define a base protocol that simply asserts the presence of > a certain kind of device at a certain memory offset, allowing platforms to > instantiate any number of these statically, and leave it to post-DXE driver > dispatch to actually bind the drivers as usual. This is implemented in patch #1. > Note that it includes an AMBA device type, which we intend to use in the future > to move ARM AMBA drivers to the UEFI driver model as well (i.e., LCD controller, > SD/MMC controller) > > Patch #2 implements a utility library to register non-discoverable devices. > > Patch #3 implements the UEFI driver that instantiates PCI I/O protocol handles > for non-discoverable devices that we know can be driven by a generic driver in > EDK2. The initial version implements coherent DMA only. > > Patch #4 implements non-coherent DMA for the driver added in patch #3. > > Patch #5 is included for reference. It ports the BeagleBoard platform to the > new driver stack. > > [0] https://lists.01.org/pipermail/edk2-devel/2016-October/003842.html > > Ard Biesheuvel (5): > MdeModulePkg: introduce non-discoverable device protocol > MdeModule: introduce helper library to register non-discoverable > devices > MdeModulePkg: implement generic PCI I/O driver for non-discoverable > devices > MdeModulePkg/NonDiscoverablePciDeviceDxe: add support for non-coherent > DMA > Omap35xxPkg/PciEmulation: port to new non-discoverable device > infrastructure > > BeagleBoardPkg/BeagleBoardPkg.dsc | 2 + > BeagleBoardPkg/BeagleBoardPkg.fdf | 1 + > MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/ComponentName.c | 75 ++ > MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.c | 208 ++++ > MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.inf | 44 + > MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c | 994 ++++++++++++++++++++ > MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.h | 82 ++ > MdeModulePkg/Include/Library/NonDiscoverableDeviceRegistrationLib.h | 47 + > MdeModulePkg/Include/Protocol/NonDiscoverableDevice.h | 88 ++ > MdeModulePkg/Library/NonDiscoverableDeviceRegistrationLib/NonDiscoverableDeviceRegistrationLib.c | 119 +++ > MdeModulePkg/Library/NonDiscoverableDeviceRegistrationLib/NonDiscoverableDeviceRegistrationLib.inf | 34 + > MdeModulePkg/MdeModulePkg.dec | 7 + > MdeModulePkg/MdeModulePkg.dsc | 3 + > Omap35xxPkg/Omap35xxPkg.dsc | 2 +- > Omap35xxPkg/PciEmulation/PciEmulation.c | 571 +---------- > Omap35xxPkg/PciEmulation/PciEmulation.h | 292 ------ > Omap35xxPkg/PciEmulation/PciEmulation.inf | 16 +- > Omap35xxPkg/PciEmulation/PciRootBridgeIo.c | 306 ------ > 18 files changed, 1728 insertions(+), 1163 deletions(-) > create mode 100644 MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/ComponentName.c > create mode 100644 MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.c > create mode 100644 MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.inf > create mode 100644 MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c > create mode 100644 MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.h > create mode 100644 MdeModulePkg/Include/Library/NonDiscoverableDeviceRegistrationLib.h > create mode 100644 MdeModulePkg/Include/Protocol/NonDiscoverableDevice.h > create mode 100644 MdeModulePkg/Library/NonDiscoverableDeviceRegistrationLib/NonDiscoverableDeviceRegistrationLib.c > create mode 100644 MdeModulePkg/Library/NonDiscoverableDeviceRegistrationLib/NonDiscoverableDeviceRegistrationLib.inf > delete mode 100644 Omap35xxPkg/PciEmulation/PciEmulation.h > delete mode 100644 Omap35xxPkg/PciEmulation/PciRootBridgeIo.c > > -- > 2.7.4 >