Re: [Linaro-uefi] [PATCH v2 1/6] Platforms/Marvell: Add PciEmulation driver

On 15 November 2016 at 03:15, Marcin Wojtas mw@semihalf.com wrote:

In order to use numerous UEFI drivers based on PCI bus, PciEmulation driver is implemented. It enables proper registration of platform devices as NonDiscoverableDevices and use generic EDK2 PciEmulation solution.

Configuration and setting amount of devices is enabled via PCD.

Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Marcin Wojtas mw@semihalf.com Signed-off-by: Jan Dabros jsd@semihalf.com

---

.../Marvell/PortingGuide/PciEmulation.txt | 46 +++++++++ Platforms/Marvell/Marvell.dec | 6 ++ Platforms/Marvell/PciEmulation/PciEmulation.c | 112 +++++++++++++++++++++ Platforms/Marvell/PciEmulation/PciEmulation.inf | 60 +++++++++++ 4 files changed, 224 insertions(+) create mode 100644 Documentation/Marvell/PortingGuide/PciEmulation.txt create mode 100644 Platforms/Marvell/PciEmulation/PciEmulation.c create mode 100644 Platforms/Marvell/PciEmulation/PciEmulation.inf

diff --git a/Documentation/Marvell/PortingGuide/PciEmulation.txt b/Documentation/Marvell/PortingGuide/PciEmulation.txt new file mode 100644 index 0000000..14dc9dc --- /dev/null +++ b/Documentation/Marvell/PortingGuide/PciEmulation.txt @@ -0,0 +1,46 @@ +PciEmulation configuration +-------------------------- +Installation of various NonDiscoverable devices via PciEmulation driver is performed +via set of PCDs. Following are available:

• gMarvellTokenSpaceGuid.PcdPciEDevCount

+Indicates amount of used devices.

+Below PCD is an unicode string format, containing settings for all devices +separated with semicolon. It indicates base address of register space:

• gMarvellTokenSpaceGuid.PcdPciEDevBaseAddress

+Devices' types are represented in an array comprising hexadecimal values:

• gMarvellTokenSpaceGuid.PcdPciEDevType

+They correspond to DEV_TYPE enum:

• typedef enum {
• 0 DEV_XHCI,
• 1 DEV_AHCI,
• 2 DEV_SDHCI,
• 3 DEV_MAX
• } DEV_TYPE;

+Devices' DMA types are represented in an array comprising hexadecimal values:

• gMarvellTokenSpaceGuid.PcdPciEDmaType

+They correspond to DMA_TYPE enum:

• typedef enum {
• 0 DMA_COHERENT,
• 1 DMA_NONCOHERENT,
• 2 DMA_TYPE_MAX
• } DMA_TYPE;

+Examples +-------- +Assuming that there are two XHCI controllers with register space starting at +0xF2500000 and 0xF2510000, with coherent DMA, following PCD values should be set:

• gMarvellTokenSpaceGuid.PcdPciEDevCount|2
• gMarvellTokenSpaceGuid.PcdPciEDevBaseAddress|L"0xF2500000;0xF2510000"
• gMarvellTokenSpaceGuid.PcdPciEDevType|{ 0x0, 0x0 }
• gMarvellTokenSpaceGuid.PcdPciEDmaType|{ 0x0, 0x0 }

diff --git a/Platforms/Marvell/Marvell.dec b/Platforms/Marvell/Marvell.dec index db99230..13fffbb 100644 --- a/Platforms/Marvell/Marvell.dec +++ b/Platforms/Marvell/Marvell.dec @@ -209,6 +209,12 @@ gMarvellTokenSpaceGuid.PcdPp2XlgBaseAddress|0|UINT64|0x3000031 gMarvellTokenSpaceGuid.PcdPp2XlgDevSize|0|UINT32|0x3000032

+#PciEmulation

• gMarvellTokenSpaceGuid.PcdPciEDevBaseAddress|{ 0x0 }|VOID*|0x30000058
• gMarvellTokenSpaceGuid.PcdPciEDevCount|0|UINT32|0x30000059
• gMarvellTokenSpaceGuid.PcdPciEDevType|{ 0x0 }|VOID*|0x3000005A
• gMarvellTokenSpaceGuid.PcdPciEDmaType|{ 0x0 }|VOID*|0x3000005B

#ResetLib gMarvellTokenSpaceGuid.PcdResetRegAddress|0|UINT64|0x40000050 gMarvellTokenSpaceGuid.PcdResetRegMask|0|UINT32|0x4000051 diff --git a/Platforms/Marvell/PciEmulation/PciEmulation.c b/Platforms/Marvell/PciEmulation/PciEmulation.c new file mode 100644 index 0000000..437dec9 --- /dev/null +++ b/Platforms/Marvell/PciEmulation/PciEmulation.c @@ -0,0 +1,112 @@ +/** @file

• Copyright (c) 2008 - 2009, Apple Inc. All rights reserved.<BR>
• Copyright (c) 2016, Marvell. All rights reserved.
• This program and the accompanying materials
• are licensed and made available under the terms and conditions of the BSD
• License which accompanies this distribution. The full text of the license
• may be found at http://opensource.org/licenses/bsd-license.php
• THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
• WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.

+**/

+#include <PiDxe.h>

+#include <Library/BaseLib.h> +#include <Library/DebugLib.h> +#include <Library/IoLib.h> +#include <Library/NonDiscoverableDeviceRegistrationLib.h> +#include <Library/ParsePcdLib.h> +#include <Library/UefiBootServicesTableLib.h>

+#include <Protocol/EmbeddedExternalDevice.h>

+typedef enum {

• DEV_XHCI,
• DEV_AHCI,
• DEV_SDHCI,
• DEV_MAX

+} DEV_TYPE;

+typedef enum {

• DMA_COHERENT,
• DMA_NONCOHERENT,
• DMA_TYPE_MAX,

+} DMA_TYPE;

+// +// Below function is used to parse devices information from PCD strings. +// Once obtained, the resources are used for registration of +// NonDiscoverable devices. +// +EFI_STATUS +EFIAPI +PciEmulationEntryPoint (

• IN EFI_HANDLE ImageHandle,
• IN EFI_SYSTEM_TABLE *SystemTable
• )

+{

• EFI_STATUS Status;
• UINT8 i;
• UINT8 DevCount = PcdGet32 (PcdPciEDevCount);
• UINT8 *DeviceType = PcdGetPtr (PcdPciEDevType);
• UINT8 *DmaType = PcdGetPtr (PcdPciEDmaType);

Please get rid of these initializers, and use ordinary assignments.

• UINTN BaseAddrTable[PcdGet32 (PcdPciEDevCount)];
• NON_DISCOVERABLE_DEVICE_TYPE DeviceTypeTable[PcdGet32 (PcdPciEDevCount)];
• NON_DISCOVERABLE_DEVICE_DMA_TYPE DmaTypeTable[PcdGet32 (PcdPciEDevCount)];
• ASSERT (PcdGetSize (PcdPciEDevType) == DevCount);
• ASSERT (PcdGetSize (PcdPciEDmaType) == DevCount);

If you are allowing dynamic PCDs for these values, these should not be asserts but runtime errors.

So either switch to FixedPcdGetXXX everywhere (and use a [FixedPcd] section in the .inf), or return an error here if the PCDs are not of the expected format (Note that ASSERT()s are compiled out in RELEASE mode). Note that that would also allow you to move DevCount/*DeviceType/*DmaType out of the function, and keep the static initializers.

• Status = ParsePcdString ((CHAR16 *) PcdGetPtr (PcdPciEDevBaseAddress), DevCount, BaseAddrTable, NULL);
• if (EFI_ERROR(Status)) {
• DEBUG((DEBUG_ERROR, "PciEmulation: Wrong PcdPciEDevBaseAddress format\n"));
• return EFI_INVALID_PARAMETER;
• }
• for (i = 0; i < DevCount; i++) {
• switch (DeviceType[i]) {
• case DEV_XHCI:

•  DeviceTypeTable[i] = NonDiscoverableDeviceTypeXhci;
  

•  break;
  

• case DEV_AHCI:

•  DeviceTypeTable[i] = NonDiscoverableDeviceTypeAhci;
  

•  break;
  

• case DEV_SDHCI:

•  DeviceTypeTable[i] = NonDiscoverableDeviceTypeSdhci;
  

•  break;
  

• default:

•  DEBUG((DEBUG_ERROR, "PciEmulation: Unsupported device type with ID=%d\n", i));
  

•  return EFI_INVALID_PARAMETER;
  

• }
• switch (DmaType[i]) {
• case DMA_COHERENT:

•  DmaTypeTable[i] = NonDiscoverableDeviceDmaTypeCoherent;
  

•  break;
  

• case DMA_NONCOHERENT:

•  DmaTypeTable[i] = NonDiscoverableDeviceDmaTypeNonCoherent;
  

•  break;
  

• default:

•  DEBUG((DEBUG_ERROR, "PciEmulation: Unsupported DMA type with ID=%d\n", i));
  

•  return EFI_INVALID_PARAMETER;
  

• }
• Status = RegisterNonDiscoverableDevice (

•                 BaseAddrTable[i],
  

•                 DeviceTypeTable[i],
  

•                 DmaTypeTable[i],
  

•                 NULL,
  

•                 NULL
  

•               );
  

• if (EFI_ERROR(Status)) {

•  DEBUG((DEBUG_ERROR, "PciEmulation: Cannot install device with ID=%d\n", i));
  

• }
• }
• return Status;

You are ignoring failures (except for printing a DEBUG message) so you should return EFI_SUCCESS here. Otherwise, the return value depends only on the last registered device, which is odd.

+} diff --git a/Platforms/Marvell/PciEmulation/PciEmulation.inf b/Platforms/Marvell/PciEmulation/PciEmulation.inf new file mode 100644 index 0000000..9021d1e --- /dev/null +++ b/Platforms/Marvell/PciEmulation/PciEmulation.inf @@ -0,0 +1,60 @@ +/** @file

• Copyright (c) 2009, Apple Inc. All rights reserved.<BR>
• Copyright (c) 2016, Marvell. All rights reserved.
• This program and the accompanying materials
• are licensed and made available under the terms and conditions of the BSD
• License which accompanies this distribution. The full text of the license may
• be found at http://opensource.org/licenses/bsd-license.php
• THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
• WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.

+**/

+[Defines]

• INF_VERSION = 0x00010019
• BASE_NAME = PciEmulation
• FILE_GUID = 3dfa08da-923b-4841-9435-c77a604d7493
• MODULE_TYPE = DXE_DRIVER
• VERSION_STRING = 1.0
• ENTRY_POINT = PciEmulationEntryPoint

+[Sources.common]

• PciEmulation.c

+[Packages]

• ArmPkg/ArmPkg.dec
• EmbeddedPkg/EmbeddedPkg.dec
• IntelFrameworkPkg/IntelFrameworkPkg.dec
• MdeModulePkg/MdeModulePkg.dec
• MdePkg/MdePkg.dec
• OpenPlatformPkg/Platforms/Marvell/Marvell.dec
• ShellPkg/ShellPkg.dec

Do we really need all of these?

+[LibraryClasses]

• BaseLib
• DebugLib
• IoLib
• NonDiscoverableDeviceRegistrationLib
• ParsePcdLib
• UefiBootServicesTableLib
• UefiDriverEntryPoint

Same here

+[Protocols]

• gEfiDevicePathProtocolGuid
• gEfiPciHostBridgeResourceAllocationProtocolGuid
• gEfiPciIoProtocolGuid
• gEfiPciRootBridgeIoProtocolGuid
• gEmbeddedExternalDeviceProtocolGuid

+[Pcd]

• gMarvellTokenSpaceGuid.PcdPciEDevBaseAddress
• gMarvellTokenSpaceGuid.PcdPciEDevCount
• gMarvellTokenSpaceGuid.PcdPciEDevType
• gMarvellTokenSpaceGuid.PcdPciEDmaType

+[Depex]

• TRUE

-- 1.8.3.1