Re: [PATCH v2 0/5] MdeModulePkg: add support for non-discoverable devices

[Marcin Wojtas <mw@semihalf.com>]

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 <ard.biesheuvel@linaro.org>:
> 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
>