[PATCH 0/6] Embedded|ArmPlatformPkg: spring cleaning + DtPlatformDxe switch

[PATCH 0/6] Embedded|ArmPlatformPkg: spring cleaning + DtPlatformDxe switch

[Ard Biesheuvel]

This implements the upstream part of switching VExpress TC2 and the AArch64
FVP Foundation/Base models to the new DtPlatformDxe driver, which is much
simpler and only allows ACPI or DT to be enabled but never both.

Patches #1 and #2 tweak the new DtPlatformDxe so it can choose from several
builtin DTBs depending on the actual platform detected at runtime.

Patches #3, #4 and #5 are basically preparatory cleanup that allows patch #6
to radically change ArmFvpDxe without affecting other users.

Patch #6 removes all the handling of FDT device paths, string PCDs that
have to be initialized to 128 spaces and other awkwardness, and simply
sets the default DTB file section index based on the detected platform.

Ard Biesheuvel (6):
  EmbeddedPkg/DtPlatformDxe: allow multiple entries in DTB FV file
  EmbeddedPkg/DtPlatformDxe: declare symbolic name for FILE_GUID
  ArmPlatformPkg/ArmShellCmdRunAxf: remove BdsLib dependency
  ArmPlatformPkg/ArmVExpressDxe: remove ARM support
  ArmPlatformPkg/ArmVExpressDxe: remove unused cruft from ArmHwDxe
  ArmPlatformPkg/ArmVExpressDxe: simply FDT handling in ArmFvpDxe

 ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/AArch64/ArmFvpDxeAArch64.c |  60 +++------
 ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/Arm/ArmFvpDxeArm.c         |  84 ------------
 ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/ArmFvpDxe.c                | 134 ++------------------
 ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/ArmFvpDxe.inf              |  42 ++----
 ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/ArmHwDxe.c                 |  43 +------
 ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/ArmHwDxe.inf               |   3 -
 ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/ArmVExpressCommon.c        |  48 -------
 ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/ArmVExpressInternal.h      |  52 +-------
 ArmPlatformPkg/ArmVExpressPkg/ArmVExpressPkg.dec                        |  28 ----
 ArmPlatformPkg/Library/ArmShellCmdRunAxf/ArmShellCmdRunAxf.inf          |   1 -
 ArmPlatformPkg/Library/ArmShellCmdRunAxf/RunAxf.c                       |  58 ++++++++-
 EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c                       |   5 +-
 EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf                     |   6 +-
 EmbeddedPkg/EmbeddedPkg.dec                                             |   6 +
 14 files changed, 108 insertions(+), 462 deletions(-)
 delete mode 100644 ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/Arm/ArmFvpDxeArm.c
 delete mode 100644 ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/ArmVExpressCommon.c

-- 
2.9.3

[PATCH 1/6] EmbeddedPkg/DtPlatformDxe: allow multiple entries in DTB FV file

[Ard Biesheuvel]

To allow some dynamic behavior in selecting the DTB to expose to the
OS, allow the DTB FV file to contain multiple sections, and indirect
the choice of section via a fixed/dynamic PCD, which defaults to 0.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c   | 5 ++++-
 EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf | 4 ++++
 EmbeddedPkg/EmbeddedPkg.dec                         | 3 +++
 3 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c
index 5778633b4985..72f9f5721cda 100644
--- a/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c
+++ b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c
@@ -17,6 +17,7 @@
 #include <Library/DevicePathLib.h>
 #include <Library/DxeServicesLib.h>
 #include <Library/HiiLib.h>
+#include <Library/PcdLib.h>
 #include <Library/MemoryAllocationLib.h>
 #include <Library/UefiBootServicesTableLib.h>
 #include <Library/UefiDriverEntryPoint.h>
@@ -121,7 +122,9 @@ DtPlatformDxeEntryPoint (
   //
   Dtb = NULL;
   Status = GetSectionFromAnyFv (&gDtPlatformDefaultDtbFileGuid,
-             EFI_SECTION_RAW, 0, &Dtb, &DtbSize);
+             EFI_SECTION_RAW,
+             PcdGet8 (PcdDtPlatformDefaultDtbSectionIndex),
+             &Dtb, &DtbSize);
   if (EFI_ERROR (Status)) {
     DEBUG ((DEBUG_WARN, "%a: no DTB blob found, defaulting to ACPI\n",
       __FUNCTION__));
diff --git a/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf
index b73877a6086b..c16202790ed9 100644
--- a/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf
+++ b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf
@@ -43,6 +43,7 @@ [LibraryClasses]
   DxeServicesLib
   HiiLib
   MemoryAllocationLib
+  PcdLib
   UefiBootServicesTableLib
   UefiDriverEntryPoint
   UefiRuntimeServicesTableLib
@@ -56,3 +57,6 @@ [Guids]
 [Depex]
   gEfiVariableArchProtocolGuid        AND
   gEfiVariableWriteArchProtocolGuid
+
+[Pcd]
+  gEmbeddedTokenSpaceGuid.PcdDtPlatformDefaultDtbSectionIndex
diff --git a/EmbeddedPkg/EmbeddedPkg.dec b/EmbeddedPkg/EmbeddedPkg.dec
index 871fc5ff4016..f1b7af347861 100644
--- a/EmbeddedPkg/EmbeddedPkg.dec
+++ b/EmbeddedPkg/EmbeddedPkg.dec
@@ -198,3 +198,6 @@ [PcdsFixedAtBuild.X64]
 
 [PcdsFixedAtBuild.common, PcdsDynamic.common]
   gEmbeddedTokenSpaceGuid.PcdFdtDevicePaths|L""|VOID*|0x00000055
+
+  # the section containing the default DTB for the current platform
+  gEmbeddedTokenSpaceGuid.PcdDtPlatformDefaultDtbSectionIndex|0|UINT8|0x00000057
-- 
2.9.3

[PATCH 2/6] EmbeddedPkg/DtPlatformDxe: declare symbolic name for FILE_GUID

[Ard Biesheuvel]

Add a definition to the package .dec file to allow DEPEXes to refer
to DtPlatformDxe in a BEFORE/AFTER DEPEX.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf | 2 +-
 EmbeddedPkg/EmbeddedPkg.dec                         | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf
index c16202790ed9..64a674ad94b1 100644
--- a/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf
+++ b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf
@@ -16,7 +16,7 @@
 [Defines]
   INF_VERSION               = 0x00010019
   BASE_NAME                 = DtPlatformDxe
-  FILE_GUID                 = FC097B3C-2EBD-4A75-A3DA-121DCAB365CC
+  FILE_GUID                 = FC097B3C-2EBD-4A75-A3DA-121DCAB365CC # gDtPlatformDxeFileGuid
   MODULE_TYPE               = DXE_DRIVER
   VERSION_STRING            = 1.0
   ENTRY_POINT               = DtPlatformDxeEntryPoint
diff --git a/EmbeddedPkg/EmbeddedPkg.dec b/EmbeddedPkg/EmbeddedPkg.dec
index f1b7af347861..4d0322b73867 100644
--- a/EmbeddedPkg/EmbeddedPkg.dec
+++ b/EmbeddedPkg/EmbeddedPkg.dec
@@ -68,6 +68,9 @@ [Guids.common]
   # File GUID for default DTB image embedded in the firmware volume
   gDtPlatformDefaultDtbFileGuid = { 0x25462cda, 0x221f, 0x47df, { 0xac, 0x1d, 0x25, 0x9c, 0xfa, 0xa4, 0xe3, 0x26 } }
 
+  # FILE_GUID defined in Drivers/DtPlatformDxe/DtPlatformDxe.inf
+  gDtPlatformDxeFileGuid = { 0xFC097B3C, 0x2EBD, 0x4A75, { 0xA3, 0xDA, 0x12, 0x1D, 0xCA, 0xB3, 0x65, 0xCC } }
+
 [Protocols.common]
   gHardwareInterruptProtocolGuid =  { 0x2890B3EA, 0x053D, 0x1643, { 0xAD, 0x0C, 0xD6, 0x48, 0x08, 0xDA, 0x3F, 0xF1 } }
   gEfiDebugSupportPeriodicCallbackProtocolGuid = { 0x9546e07c, 0x2cbb, 0x4c88, { 0x98, 0x6c, 0xcd, 0x34, 0x10, 0x86, 0xf0, 0x44 } }
-- 
2.9.3

[PATCH 3/6] ArmPlatformPkg/ArmShellCmdRunAxf: remove BdsLib dependency

[Ard Biesheuvel]

Remove ArmShellCmdRunAxf's dependency on the deprecated BdsLib by
cloning the ShutdownUefiBootServices() routine into a local source
file; this is the only BdsLib feature 'runaxf' depends on.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPlatformPkg/Library/ArmShellCmdRunAxf/ArmShellCmdRunAxf.inf |  1 -
 ArmPlatformPkg/Library/ArmShellCmdRunAxf/RunAxf.c              | 58 +++++++++++++++++++-
 2 files changed, 57 insertions(+), 2 deletions(-)

diff --git a/ArmPlatformPkg/Library/ArmShellCmdRunAxf/ArmShellCmdRunAxf.inf b/ArmPlatformPkg/Library/ArmShellCmdRunAxf/ArmShellCmdRunAxf.inf
index 9a34f666612a..7d15f6934608 100644
--- a/ArmPlatformPkg/Library/ArmShellCmdRunAxf/ArmShellCmdRunAxf.inf
+++ b/ArmPlatformPkg/Library/ArmShellCmdRunAxf/ArmShellCmdRunAxf.inf
@@ -43,7 +43,6 @@ [Packages]
 [LibraryClasses]
   ArmLib
   BaseLib
-  BdsLib
   DebugLib
   HiiLib
   ShellLib
diff --git a/ArmPlatformPkg/Library/ArmShellCmdRunAxf/RunAxf.c b/ArmPlatformPkg/Library/ArmShellCmdRunAxf/RunAxf.c
index 2abfb6cc1053..9f4fc780307d 100644
--- a/ArmPlatformPkg/Library/ArmShellCmdRunAxf/RunAxf.c
+++ b/ArmPlatformPkg/Library/ArmShellCmdRunAxf/RunAxf.c
@@ -21,7 +21,6 @@
 #include <Library/DevicePathLib.h>
 #include <Library/BaseLib.h>
 #include <Library/BaseMemoryLib.h>
-#include <Library/BdsLib.h>
 #include <Library/MemoryAllocationLib.h>
 #include <Library/DebugLib.h>
 
@@ -35,6 +34,63 @@
 typedef VOID (*ELF_ENTRYPOINT)(UINTN arg0, UINTN arg1,
                                UINTN arg2, UINTN arg3);
 
+STATIC
+EFI_STATUS
+ShutdownUefiBootServices (
+  VOID
+  )
+{
+  EFI_STATUS              Status;
+  UINTN                   MemoryMapSize;
+  EFI_MEMORY_DESCRIPTOR   *MemoryMap;
+  UINTN                   MapKey;
+  UINTN                   DescriptorSize;
+  UINT32                  DescriptorVersion;
+  UINTN                   Pages;
+
+  MemoryMap = NULL;
+  MemoryMapSize = 0;
+  Pages = 0;
+
+  do {
+    Status = gBS->GetMemoryMap (
+                    &MemoryMapSize,
+                    MemoryMap,
+                    &MapKey,
+                    &DescriptorSize,
+                    &DescriptorVersion
+                    );
+    if (Status == EFI_BUFFER_TOO_SMALL) {
+
+      Pages = EFI_SIZE_TO_PAGES (MemoryMapSize) + 1;
+      MemoryMap = AllocatePages (Pages);
+
+      //
+      // Get System MemoryMap
+      //
+      Status = gBS->GetMemoryMap (
+                      &MemoryMapSize,
+                      MemoryMap,
+                      &MapKey,
+                      &DescriptorSize,
+                      &DescriptorVersion
+                      );
+    }
+
+    // Don't do anything between the GetMemoryMap() and ExitBootServices()
+    if (!EFI_ERROR(Status)) {
+      Status = gBS->ExitBootServices (gImageHandle, MapKey);
+      if (EFI_ERROR(Status)) {
+        FreePages (MemoryMap, Pages);
+        MemoryMap = NULL;
+        MemoryMapSize = 0;
+      }
+    }
+  } while (EFI_ERROR(Status));
+
+  return Status;
+}
+
 
 STATIC
 EFI_STATUS
-- 
2.9.3

[PATCH 4/6] ArmPlatformPkg/ArmVExpressDxe: remove ARM support

[Ard Biesheuvel]

The 32-bit ARM support in this driver is unused, and thus untested.
So let's just remove it.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/Arm/ArmFvpDxeArm.c | 84 --------------------
 ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/ArmFvpDxe.inf      |  9 ---
 ArmPlatformPkg/ArmVExpressPkg/ArmVExpressPkg.dec                | 10 ---
 3 files changed, 103 deletions(-)

diff --git a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/Arm/ArmFvpDxeArm.c b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/Arm/ArmFvpDxeArm.c
deleted file mode 100644
index 2057c6e2156a..000000000000
--- a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/Arm/ArmFvpDxeArm.c
+++ /dev/null
@@ -1,84 +0,0 @@
-/** @file
-
-  Copyright (c) 2014, ARM Ltd. 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 "ArmVExpressInternal.h"
-#include <Library/ArmPlatformLib.h>  // To get Core Count
-
-//
-// Description of the four ARM model platforms :
-// Platform ids are defined in ArmVExpressInternal.h for
-// all "ArmVExpress-like" platforms (AARCH64 or ARM architecture,
-// model or hardware platforms).
-//
-CONST ARM_VEXPRESS_PLATFORM ArmVExpressPlatforms[] = {
-  { ARM_FVP_VEXPRESS_A9x4,  FixedPcdGetPtr (PcdFdtVExpressFvpA9x4),  L"rtsm_ve-cortex_a9x4.dtb"  },
-  { ARM_FVP_VEXPRESS_A15x1, FixedPcdGetPtr (PcdFdtVExpressFvpA15x1), L"rtsm_ve-cortex_a15x1.dtb" },
-  { ARM_FVP_VEXPRESS_A15x2, FixedPcdGetPtr (PcdFdtVExpressFvpA15x2), L"rtsm_ve-cortex_a15x2.dtb" },
-  { ARM_FVP_VEXPRESS_A15x4, FixedPcdGetPtr (PcdFdtVExpressFvpA15x4), L"rtsm_ve-cortex_a15x4.dtb" },
-  { ARM_FVP_VEXPRESS_UNKNOWN, }
-};
-
-/**
-  Get information about the VExpress platform the firmware is running on.
-
-  @param[out]  Platform   Address where the pointer to the platform information
-                          (type ARM_VEXPRESS_PLATFORM*) should be stored.
-                          The returned pointer does not point to an allocated
-                          memory area.
-
-  @retval  EFI_SUCCESS    The platform information was returned.
-  @retval  EFI_NOT_FOUND  The platform was not recognised.
-
-**/
-EFI_STATUS
-ArmVExpressGetPlatform (
-  OUT CONST ARM_VEXPRESS_PLATFORM** Platform
-  )
-{
-  UINT32                SysId;
-  UINTN                 CpuType;
-  EFI_STATUS            Status;
-  UINTN                 CoreCount;
-
-  ASSERT (Platform != NULL);
-
-  CpuType   = 0;
-  Status    = EFI_NOT_FOUND;
-  *Platform = NULL;
-
-  SysId = MmioRead32 (ARM_VE_SYS_ID_REG);
-  if (SysId == ARM_RTSM_SYS_ID) {
-    // Get the Cortex-A version
-    CpuType = (ArmReadMidr () >> 4) & ARM_CPU_TYPE_MASK;
-    if (CpuType == ARM_CPU_TYPE_A9) {
-      Status = ArmVExpressGetPlatformFromId (ARM_FVP_VEXPRESS_A9x4, Platform);
-    } else if (CpuType == ARM_CPU_TYPE_A15) {
-      CoreCount = ArmGetCpuCountPerCluster ();
-      if (CoreCount == 1) {
-        Status = ArmVExpressGetPlatformFromId (ARM_FVP_VEXPRESS_A15x1, Platform);
-      } else if (CoreCount == 2) {
-        Status = ArmVExpressGetPlatformFromId (ARM_FVP_VEXPRESS_A15x2, Platform);
-      } else if (CoreCount == 4) {
-        Status = ArmVExpressGetPlatformFromId (ARM_FVP_VEXPRESS_A15x4, Platform);
-      }
-    }
-  }
-
-  if (EFI_ERROR (Status)) {
-    DEBUG ((EFI_D_ERROR, "Unsupported platform (SysId:0x%X, CpuType:0x%X)\n", SysId, CpuType));
-    ASSERT_EFI_ERROR (Status);
-  }
-
-  return Status;
-}
diff --git a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/ArmFvpDxe.inf b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/ArmFvpDxe.inf
index 327c5101ddb5..2a8c8388a3b2 100644
--- a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/ArmFvpDxe.inf
+++ b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/ArmFvpDxe.inf
@@ -24,9 +24,6 @@ [Sources.common]
   ArmFvpDxe.c
   ArmVExpressCommon.c
 
-[Sources.ARM]
-  Arm/ArmFvpDxeArm.c
-
 [Sources.AARCH64]
   AArch64/ArmFvpDxeAArch64.c
 
@@ -61,12 +58,6 @@ [Protocols]
 [FixedPcd]
   gArmVExpressTokenSpaceGuid.PcdFvpFdtDevicePathsBase
 
-[FixedPcd.ARM]
-  gArmVExpressTokenSpaceGuid.PcdFdtVExpressFvpA9x4
-  gArmVExpressTokenSpaceGuid.PcdFdtVExpressFvpA15x1
-  gArmVExpressTokenSpaceGuid.PcdFdtVExpressFvpA15x2
-  gArmVExpressTokenSpaceGuid.PcdFdtVExpressFvpA15x4
-
 [FixedPcd.AARCH64]
   gArmVExpressTokenSpaceGuid.PcdFdtFvpVExpressAEMv8x4
   gArmVExpressTokenSpaceGuid.PcdFdtFvpBaseAEMv8x4GicV2
diff --git a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressPkg.dec b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressPkg.dec
index c774d97541e1..39f046541502 100644
--- a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressPkg.dec
+++ b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressPkg.dec
@@ -60,16 +60,6 @@ [PcdsFixedAtBuild.common]
   #
   # ARM Versatile Express FDT Guids
   #
-  # FVP platforms
-  gArmVExpressTokenSpaceGuid.PcdFdtVExpressFvpA9x4|{ 0x12, 0x7b, 0xdf, 0xa1, 0x60, 0x11, 0xcf, 0x16, 0xb8, 0xc6, 0x98, 0xde, 0xdf, 0xe2, 0xce, 0xae }|VOID*|0x00000007
-  gArmVExpressTokenSpaceGuid.PcdFdtVExpressFvpA15x1|{ 0xe5, 0x1b, 0xc0, 0x96, 0xeb, 0xd7, 0x1a, 0x42, 0xc8, 0xe8, 0x6a, 0xfd, 0x5a, 0x86, 0x1d, 0x84 }|VOID*|0x00000008
-  gArmVExpressTokenSpaceGuid.PcdFdtVExpressFvpA15x2|{ 0x84, 0x43, 0x70, 0x4d, 0x19, 0xf1, 0x29, 0xe3, 0xef, 0xcd, 0xa5, 0x9b, 0x3d, 0x0a, 0x5a, 0x5f }|VOID*|0x00000009
-  gArmVExpressTokenSpaceGuid.PcdFdtVExpressFvpA15x4|{ 0x72, 0x3b, 0x28, 0x27, 0x90, 0x2f, 0xca, 0x4d, 0x9a, 0xb5, 0x98, 0x48, 0xfb, 0xc2, 0xd4, 0xed }|VOID*|0x0000000A
-  # HW platforms
-  gArmVExpressTokenSpaceGuid.PcdFdtVExpressHwA9x4|{ 0xf6, 0x1c, 0xd2, 0x2f, 0xe8, 0xe6, 0xf2, 0x4f, 0xa9, 0xca, 0x3b, 0x9f, 0x00, 0xe9, 0x28, 0x89 }|VOID*|0x0000000B
-  gArmVExpressTokenSpaceGuid.PcdFdtVExpressHwA15x2A7x3|{ 0xeb, 0x06, 0xe6, 0xd5, 0xdf, 0x83, 0x90, 0x4e, 0x81, 0xe8, 0xc3, 0xdb, 0x2f, 0x77, 0x17, 0x9a }|VOID*|0x0000000C
-  gArmVExpressTokenSpaceGuid.PcdFdtVExpressHwA15|{ 0xc2, 0x47, 0x89, 0x6b, 0x87, 0x42, 0x91, 0x4d, 0x8f, 0xe0, 0xa3, 0x81, 0xea, 0x5b, 0x56, 0x8f }|VOID*|0x0000000D
-  gArmVExpressTokenSpaceGuid.PcdFdtVExpressHwA5|{ 0x63, 0x76, 0xcc, 0xa2, 0x7c, 0x4d, 0x8a, 0x44, 0xaa, 0xb5, 0x4c, 0x03, 0x4b, 0x6f, 0xda, 0xb7 }|VOID*|0x0000000E
 
   # AArch64 FVP platforms
   gArmVExpressTokenSpaceGuid.PcdFdtFvpVExpressAEMv8x4|{ 0xa8, 0x95, 0x5f, 0xf6, 0x32, 0x7b, 0xf3, 0x16, 0x12, 0x32, 0x45, 0x50, 0xbd, 0x54, 0xca, 0xe5 }|VOID*|0x00000010
-- 
2.9.3

[PATCH 5/6] ArmPlatformPkg/ArmVExpressDxe: remove unused cruft from ArmHwDxe

[Ard Biesheuvel]

Remove unused cruft from ArmHwDxe -- the only thing that remains is
installation of the 'runaxf' shell command.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/ArmHwDxe.c   | 43 +-------------------
 ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/ArmHwDxe.inf |  3 --
 2 files changed, 1 insertion(+), 45 deletions(-)

diff --git a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/ArmHwDxe.c b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/ArmHwDxe.c
index 351c73312dc4..19efa3c23dea 100644
--- a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/ArmHwDxe.c
+++ b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/ArmHwDxe.c
@@ -12,49 +12,8 @@
 
 **/
 
-#include "ArmVExpressInternal.h"
 #include <Library/ArmShellCmdLib.h>
-
-CONST EFI_GUID ArmHwA9x4Guid = { 0x2fd21cf6, 0xe6e8, 0x4ff2, { 0xa9, 0xca, 0x3b, 0x9f, 0x00, 0xe9, 0x28, 0x89 } };
-CONST EFI_GUID ArmHwA15x2A7x3Guid = { 0xd5e606eb, 0x83df, 0x4e90, { 0x81, 0xe8, 0xc3, 0xdb, 0x2f, 0x77, 0x17, 0x9a } };
-CONST EFI_GUID ArmHwA15Guid = { 0x6b8947c2, 0x4287, 0x4d91, { 0x8f, 0xe0, 0xa3, 0x81, 0xea, 0x5b, 0x56, 0x8f } };
-CONST EFI_GUID ArmHwA5Guid = { 0xa2cc7663, 0x4d7c, 0x448a, { 0xaa, 0xb5, 0x4c, 0x03, 0x4b, 0x6f, 0xda, 0xb7 } };
-CONST EFI_GUID NullGuid = { 0x0, 0x0, 0x0, { 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0 } };
-
-//
-// Description of the four hardware platforms :
-// just the platform id for the time being.
-// Platform ids are defined in ArmVExpressInternal.h for
-// all "ArmVExpress-like" platforms (AARCH64 or ARM architecture,
-// model or hardware platforms).
-//
-//Note: File extensions are stripped with the VExpress NOR Flash FileSystem
-CONST ARM_VEXPRESS_PLATFORM ArmVExpressPlatforms[] = {
-  { ARM_HW_A9x4, &ArmHwA9x4Guid, L"ca9" },
-  { ARM_HW_A15x2_A7x3, &ArmHwA15x2A7x3Guid, L"ca15a7" },
-  { ARM_HW_A15, &ArmHwA15Guid, L"ca15a7" },
-  { ARM_HW_A5, &ArmHwA5Guid, L"ca5s" },
-  { ARM_FVP_VEXPRESS_UNKNOWN, &NullGuid, NULL }
-};
-
-/**
-  Get information about the VExpress platform the firmware is running on.
-
-  @param[out]  Platform   Address where the pointer to the platform information
-                          (type ARM_VEXPRESS_PLATFORM*) should be stored.
-                          The returned pointer does not point to an allocated
-                          memory area. Not used here.
-
-  @retval  EFI_NOT_FOUND  The platform was not recognised.
-
-**/
-EFI_STATUS
-ArmVExpressGetPlatform (
-  OUT CONST ARM_VEXPRESS_PLATFORM** Platform
-  )
-{
-  return EFI_NOT_FOUND;
-}
+#include <Library/DebugLib.h>
 
 /**
  * Generic UEFI Entrypoint for 'ArmHwDxe' driver
diff --git a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/ArmHwDxe.inf b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/ArmHwDxe.inf
index 1a007627ad3f..1ecdbb0b231e 100644
--- a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/ArmHwDxe.inf
+++ b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/ArmHwDxe.inf
@@ -22,12 +22,9 @@ [Defines]
 
 [Sources.common]
   ArmHwDxe.c
-  ArmVExpressCommon.c
 
 [Packages]
-  ArmPkg/ArmPkg.dec
   ArmPlatformPkg/ArmPlatformPkg.dec
-  EmbeddedPkg/EmbeddedPkg.dec
   MdePkg/MdePkg.dec
 
 [LibraryClasses]
-- 
2.9.3

[PATCH 6/6] ArmPlatformPkg/ArmVExpressDxe: simply FDT handling in ArmFvpDxe

[Ard Biesheuvel]

Replace the elaborate but awkward handling of FDT images using device
paths and string PCDs initialized to 128 spaces with a simple scheme
involving a set of builtin DTBs and a bit of runtime logic to select
between them.

This is sufficient for ordinary use, which makes it more suitable as
reference code. Note that overriding the DTB presented to the OS can
easily be done with a UEFI application that simply installs a new DTB
image under the existing FDT configuration table GUID.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/AArch64/ArmFvpDxeAArch64.c |  60 +++------
 ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/ArmFvpDxe.c                | 134 ++------------------
 ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/ArmFvpDxe.inf              |  33 ++---
 ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/ArmVExpressCommon.c        |  48 -------
 ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/ArmVExpressInternal.h      |  52 +-------
 ArmPlatformPkg/ArmVExpressPkg/ArmVExpressPkg.dec                        |  18 ---
 6 files changed, 35 insertions(+), 310 deletions(-)

diff --git a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/AArch64/ArmFvpDxeAArch64.c b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/AArch64/ArmFvpDxeAArch64.c
index c368957dcd3d..fe8e115c97eb 100644
--- a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/AArch64/ArmFvpDxeAArch64.c
+++ b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/AArch64/ArmFvpDxeAArch64.c
@@ -1,6 +1,7 @@
 /** @file
 
   Copyright (c) 2014-2015, ARM Ltd. All rights reserved.
+  Copyright (c) 2017, Linaro Ltd. 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
@@ -15,30 +16,11 @@
 #include "ArmVExpressInternal.h"
 #include <Library/ArmGicLib.h>
 
-//
-// Description of the AARCH64 model platforms :
-// Platform ids are defined in ArmVExpressInternal.h for
-// all "ArmVExpress-like" platforms (AARCH64 or ARM architecture,
-// model or hardware platforms).
-//
-CONST ARM_VEXPRESS_PLATFORM ArmVExpressPlatforms[] = {
-  { ARM_FVP_VEXPRESS_AEMv8x4,                  FixedPcdGetPtr (PcdFdtFvpVExpressAEMv8x4),        L"rtsm_ve-aemv8a.dtb"                  },
-  { ARM_FVP_BASE_AEMv8x4_AEMv8x4_GICV2,        FixedPcdGetPtr (PcdFdtFvpBaseAEMv8x4GicV2),       L"fvp-base-gicv2-psci.dtb"             },
-  { ARM_FVP_BASE_AEMv8x4_AEMv8x4_GICV2_LEGACY, FixedPcdGetPtr (PcdFdtFvpBaseAEMv8x4GicV2Legacy), L"fvp-base-gicv2legacy-psci.dtb"       },
-  { ARM_FVP_BASE_AEMv8x4_AEMv8x4_GICV3,        FixedPcdGetPtr (PcdFdtFvpBaseAEMv8x4GicV3),       L"fvp-base-gicv3-psci.dtb"             },
-  { ARM_FVP_FOUNDATION_GICV2,                  FixedPcdGetPtr (PcdFdtFvpFoundationGicV2),        L"fvp-foundation-gicv2-psci.dtb"       },
-  { ARM_FVP_FOUNDATION_GICV2_LEGACY,           FixedPcdGetPtr (PcdFdtFvpFoundationGicV2Legacy),  L"fvp-foundation-gicv2legacy-psci.dtb" },
-  { ARM_FVP_FOUNDATION_GICV3,                  FixedPcdGetPtr (PcdFdtFvpFoundationGicV3),        L"fvp-foundation-gicv3-psci.dtb"       },
-  { ARM_FVP_VEXPRESS_UNKNOWN }
-};
-
 /**
+
   Get information about the VExpress platform the firmware is running on.
 
-  @param[out]  Platform   Address where the pointer to the platform information
-                          (type ARM_VEXPRESS_PLATFORM*) should be stored.
-                          The returned pointer does not point to an allocated
-                          memory area.
+  @param[out]  PlatformId ARM_VEXPRESS_PLATFORM_ID value of current platform
 
   @retval  EFI_SUCCESS    The platform information was returned.
   @retval  EFI_NOT_FOUND  The platform was not recognised.
@@ -46,19 +28,14 @@ CONST ARM_VEXPRESS_PLATFORM ArmVExpressPlatforms[] = {
 **/
 EFI_STATUS
 ArmVExpressGetPlatform (
-  OUT CONST ARM_VEXPRESS_PLATFORM** Platform
+  OUT   ARM_VEXPRESS_PLATFORM_ID    *PlatformId
   )
 {
-  EFI_STATUS            Status;
   UINT32                SysId;
   UINT32                FvpSysId;
   UINT32                VariantSysId;
   ARM_GIC_ARCH_REVISION GicRevision;
 
-  ASSERT (Platform != NULL);
-
-  Status = EFI_NOT_FOUND;
-
   SysId = MmioRead32 (ARM_VE_SYS_ID_REG);
   if (SysId != ARM_RTSM_SYS_ID) {
     // Remove the GIC variant to identify if we are running on the FVP Base or
@@ -70,44 +47,41 @@ ArmVExpressGetPlatform (
 
     if (FvpSysId == ARM_FVP_BASE_BOARD_SYS_ID) {
       if (VariantSysId == ARM_FVP_GIC_VE_MMAP) {
-        // FVP Base Model with legacy GIC memory map
-        Status = ArmVExpressGetPlatformFromId (ARM_FVP_BASE_AEMv8x4_AEMv8x4_GICV2_LEGACY, Platform);
+        // FVP Base Model with legacy GIC memory map -- no longer supported
+        goto NotFound;
       } else {
         GicRevision = ArmGicGetSupportedArchRevision ();
 
         if (GicRevision == ARM_GIC_ARCH_REVISION_2) {
           // FVP Base Model with GICv2 support
-          Status = ArmVExpressGetPlatformFromId (ARM_FVP_BASE_AEMv8x4_AEMv8x4_GICV2, Platform);
+          *PlatformId = ARM_FVP_BASE_AEMv8x4_AEMv8x4_GICV2;
         } else {
           // FVP Base Model with GICv3 support
-          Status = ArmVExpressGetPlatformFromId (ARM_FVP_BASE_AEMv8x4_AEMv8x4_GICV3, Platform);
+          *PlatformId = ARM_FVP_BASE_AEMv8x4_AEMv8x4_GICV3;
         }
       }
     } else if (FvpSysId == ARM_FVP_FOUNDATION_BOARD_SYS_ID) {
       if (VariantSysId == ARM_FVP_GIC_VE_MMAP) {
-        // FVP Foundation Model with legacy GIC memory map
-        Status = ArmVExpressGetPlatformFromId (ARM_FVP_FOUNDATION_GICV2_LEGACY, Platform);
+        // FVP Foundation Model with legacy GIC memory map -- no longer supported
+        goto NotFound;
       } else {
         GicRevision = ArmGicGetSupportedArchRevision ();
 
         if (GicRevision == ARM_GIC_ARCH_REVISION_2) {
           // FVP Foundation Model with GICv2
-          Status = ArmVExpressGetPlatformFromId (ARM_FVP_FOUNDATION_GICV2, Platform);
+          *PlatformId = ARM_FVP_FOUNDATION_GICV2;
         } else {
           // FVP Foundation Model with GICv3
-          Status = ArmVExpressGetPlatformFromId (ARM_FVP_FOUNDATION_GICV3, Platform);
+          *PlatformId = ARM_FVP_FOUNDATION_GICV3;
         }
       }
     }
-  } else {
-    // FVP Versatile Express AEMv8
-    Status = ArmVExpressGetPlatformFromId (ARM_FVP_VEXPRESS_AEMv8x4, Platform);
+    return EFI_SUCCESS;
   }
 
-  if (EFI_ERROR (Status)) {
-    DEBUG ((EFI_D_ERROR, "Unsupported AArch64 RTSM (SysId:0x%X).\n", SysId));
-    ASSERT_EFI_ERROR (Status);
-  }
+NotFound:
+  DEBUG ((EFI_D_ERROR, "Unsupported AArch64 RTSM (SysId:0x%X).\n", SysId));
+  ASSERT_EFI_ERROR (EFI_NOT_FOUND);
 
-  return Status;
+  return EFI_NOT_FOUND;
 }
diff --git a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/ArmFvpDxe.c b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/ArmFvpDxe.c
index 8c42814a04dc..bf100bbaa822 100644
--- a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/ArmFvpDxe.c
+++ b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/ArmFvpDxe.c
@@ -55,95 +55,6 @@ VIRTIO_BLK_DEVICE_PATH mVirtioBlockDevicePath =
   }
 };
 
-STATIC
-EFI_STATUS
-InternalFindFdtByGuid (
-  IN OUT   EFI_DEVICE_PATH  **FdtDevicePath,
-  IN CONST EFI_GUID         *FdtGuid
-  )
-{
-  MEDIA_FW_VOL_FILEPATH_DEVICE_PATH    FileDevicePath;
-  EFI_HANDLE                           *HandleBuffer;
-  UINTN                                HandleCount;
-  UINTN                                Index;
-  EFI_FIRMWARE_VOLUME2_PROTOCOL        *FvProtocol;
-  EFI_GUID                             NameGuid;
-  UINTN                                Size;
-  VOID                                 *Key;
-  EFI_FV_FILETYPE                      FileType;
-  EFI_FV_FILE_ATTRIBUTES               Attributes;
-  EFI_DEVICE_PATH                      *FvDevicePath;
-  EFI_STATUS                           Status;
-
-  if (FdtGuid == NULL) {
-    return EFI_NOT_FOUND;
-  }
-
-  EfiInitializeFwVolDevicepathNode (&FileDevicePath, FdtGuid);
-
-  HandleBuffer = NULL;
-  Status = gBS->LocateHandleBuffer (
-                  ByProtocol,
-                  &gEfiFirmwareVolume2ProtocolGuid,
-                  NULL,
-                  &HandleCount,
-                  &HandleBuffer
-                  );
-  if (EFI_ERROR (Status)) {
-    return Status;
-  }
-
-  for (Index = 0; Index < HandleCount; Index++) {
-    Status = gBS->HandleProtocol (
-                    HandleBuffer[Index],
-                    &gEfiFirmwareVolume2ProtocolGuid,
-                    (VOID **) &FvProtocol
-                    );
-    if (EFI_ERROR (Status)) {
-      return Status;
-    }
-
-    // Allocate Key
-    Key = AllocatePool (FvProtocol->KeySize);
-    ASSERT (Key != NULL);
-    ZeroMem (Key, FvProtocol->KeySize);
-
-    do {
-      FileType = EFI_FV_FILETYPE_RAW;
-      Status = FvProtocol->GetNextFile (FvProtocol, Key, &FileType, &NameGuid, &Attributes, &Size);
-      if (Status == EFI_NOT_FOUND) {
-        break;
-      }
-      if (EFI_ERROR (Status)) {
-        return Status;
-      }
-
-      //
-      // Check whether this file is the one we are looking for. If so,
-      // create a device path for it and return it to the caller.
-      //
-      if (CompareGuid (&NameGuid, FdtGuid)) {
-          Status = gBS->HandleProtocol (HandleBuffer[Index], &gEfiDevicePathProtocolGuid, (VOID **)&FvDevicePath);
-          if (!EFI_ERROR (Status)) {
-            *FdtDevicePath = AppendDevicePathNode (FvDevicePath,
-                               (EFI_DEVICE_PATH_PROTOCOL *)&FileDevicePath);
-          }
-          goto Done;
-      }
-    } while (TRUE);
-    FreePool (Key);
-  }
-
-  if (Index == HandleCount) {
-    Status = EFI_NOT_FOUND;
-  }
-  return Status;
-
-Done:
-  FreePool (Key);
-  return Status;
-}
-
 /**
  * Generic UEFI Entrypoint for 'ArmFvpDxe' driver
  * See UEFI specification for the details of the parameters
@@ -155,12 +66,15 @@ ArmFvpInitialise (
   IN EFI_SYSTEM_TABLE   *SystemTable
   )
 {
-  CONST ARM_VEXPRESS_PLATFORM* Platform;
   EFI_STATUS                   Status;
-  CHAR16                       *TextDevicePath;
-  UINTN                        TextDevicePathSize;
-  VOID                         *Buffer;
-  EFI_DEVICE_PATH              *FdtDevicePath;
+  ARM_VEXPRESS_PLATFORM_ID     PlatformId;
+
+  Status = ArmVExpressGetPlatform (&PlatformId);
+  ASSERT_EFI_ERROR (Status);
+
+  if (!EFI_ERROR (Status)) {
+    PcdSet8 (PcdDtPlatformDefaultDtbSectionIndex, (UINT8)PlatformId);
+  }
 
   Status = gBS->InstallProtocolInterface (&ImageHandle,
                  &gEfiDevicePathProtocolGuid, EFI_NATIVE_INTERFACE,
@@ -169,38 +83,6 @@ ArmFvpInitialise (
     return Status;
   }
 
-  Status = ArmVExpressGetPlatform (&Platform);
-  if (!EFI_ERROR (Status)) {
-    FdtDevicePath = NULL;
-    Status = InternalFindFdtByGuid (&FdtDevicePath, Platform->FdtGuid);
-    if (!EFI_ERROR (Status)) {
-      TextDevicePath = ConvertDevicePathToText (FdtDevicePath, FALSE, FALSE);
-      if (TextDevicePath != NULL) {
-        TextDevicePathSize = StrSize (TextDevicePath);
-      }
-      FreePool (FdtDevicePath);
-    } else {
-      TextDevicePathSize  = StrSize ((CHAR16*)PcdGetPtr (PcdFvpFdtDevicePathsBase)) - sizeof (CHAR16);
-      TextDevicePathSize += StrSize (Platform->FdtName);
-
-      TextDevicePath = AllocatePool (TextDevicePathSize);
-      if (TextDevicePath != NULL) {
-        StrCpy (TextDevicePath, ((CHAR16*)PcdGetPtr (PcdFvpFdtDevicePathsBase)));
-        StrCat (TextDevicePath, Platform->FdtName);
-      }
-    }
-    if (TextDevicePath != NULL) {
-      Buffer = PcdSetPtr (PcdFdtDevicePaths, &TextDevicePathSize, TextDevicePath);
-      if (Buffer == NULL) {
-        DEBUG ((
-          EFI_D_ERROR,
-          "ArmFvpDxe: Setting of FDT device path in PcdFdtDevicePaths failed - %r\n", EFI_BUFFER_TOO_SMALL
-          ));
-      }
-      FreePool (TextDevicePath);
-    }
-  }
-
   // Declare the Virtio BlockIo device
   Status = VirtioMmioInstallDevice (ARM_FVP_BASE_VIRTIO_BLOCK_BASE, ImageHandle);
   if (EFI_ERROR (Status)) {
diff --git a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/ArmFvpDxe.inf b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/ArmFvpDxe.inf
index 2a8c8388a3b2..605ca160391d 100644
--- a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/ArmFvpDxe.inf
+++ b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/ArmFvpDxe.inf
@@ -22,7 +22,6 @@ [Defines]
 
 [Sources.common]
   ArmFvpDxe.c
-  ArmVExpressCommon.c
 
 [Sources.AARCH64]
   AArch64/ArmFvpDxeAArch64.c
@@ -31,41 +30,25 @@ [Packages]
   MdePkg/MdePkg.dec
   ArmPkg/ArmPkg.dec
   ArmPlatformPkg/ArmPlatformPkg.dec
-  ArmPlatformPkg/ArmVExpressPkg/ArmVExpressPkg.dec
   EmbeddedPkg/EmbeddedPkg.dec
   OvmfPkg/OvmfPkg.dec
 
 [LibraryClasses]
-  PcdLib
+  #ArmLib
+  #ArmPlatformLib
   ArmShellCmdRunAxfLib
-  ArmLib
-  ArmPlatformLib
   BaseMemoryLib
-  DxeServicesTableLib
-  MemoryAllocationLib
+  IoLib
+  PcdLib
   UefiDriverEntryPoint
   UefiBootServicesTableLib
   VirtioMmioDeviceLib
-  DevicePathLib
 
 [LibraryClasses.AARCH64]
   ArmGicLib
 
-[Protocols]
-  gEfiFirmwareVolume2ProtocolGuid
-  gEfiDevicePathProtocolGuid
-
-[FixedPcd]
-  gArmVExpressTokenSpaceGuid.PcdFvpFdtDevicePathsBase
-
-[FixedPcd.AARCH64]
-  gArmVExpressTokenSpaceGuid.PcdFdtFvpVExpressAEMv8x4
-  gArmVExpressTokenSpaceGuid.PcdFdtFvpBaseAEMv8x4GicV2
-  gArmVExpressTokenSpaceGuid.PcdFdtFvpBaseAEMv8x4GicV2Legacy
-  gArmVExpressTokenSpaceGuid.PcdFdtFvpBaseAEMv8x4GicV3
-  gArmVExpressTokenSpaceGuid.PcdFdtFvpFoundationGicV2
-  gArmVExpressTokenSpaceGuid.PcdFdtFvpFoundationGicV2Legacy
-  gArmVExpressTokenSpaceGuid.PcdFdtFvpFoundationGicV3
-
 [Pcd]
-  gEmbeddedTokenSpaceGuid.PcdFdtDevicePaths
+  gEmbeddedTokenSpaceGuid.PcdDtPlatformDefaultDtbSectionIndex
+
+[Depex]
+  BEFORE gDtPlatformDxeFileGuid
diff --git a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/ArmVExpressCommon.c b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/ArmVExpressCommon.c
deleted file mode 100644
index e1cac7fb3714..000000000000
--- a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/ArmVExpressCommon.c
+++ /dev/null
@@ -1,48 +0,0 @@
-/** @file
-
-  Copyright (c) 2014, ARM Ltd. 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 "ArmVExpressInternal.h"
-
-/**
-  Get information about the VExpress platform the firmware is running on given its Id.
-
-  @param[in]   PlatformId  Id of the VExpress platform.
-  @param[out]  Platform    Address where the pointer to the platform information
-                           (type ARM_VEXPRESS_PLATFORM*) should be stored.
-                           The returned pointer does not point to an allocated
-                           memory area.
-
-  @retval  EFI_SUCCESS    The platform information was returned.
-  @retval  EFI_NOT_FOUND  The platform was not recognised.
-
-**/
-EFI_STATUS
-ArmVExpressGetPlatformFromId (
-  IN  CONST ARM_VEXPRESS_PLATFORM_ID PlatformId,
-  OUT CONST ARM_VEXPRESS_PLATFORM**  Platform
-  )
-{
-  UINTN Index;
-
-  ASSERT (Platform != NULL);
-
-  for (Index = 0; ArmVExpressPlatforms[Index].Id != ARM_FVP_VEXPRESS_UNKNOWN; Index++) {
-    if (ArmVExpressPlatforms[Index].Id == PlatformId) {
-      *Platform = &ArmVExpressPlatforms[Index];
-      return EFI_SUCCESS;
-    }
-  }
-
-  return EFI_NOT_FOUND;
-}
diff --git a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/ArmVExpressInternal.h b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/ArmVExpressInternal.h
index e123eea2d28b..2676f2d4f903 100644
--- a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/ArmVExpressInternal.h
+++ b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/ArmVExpressInternal.h
@@ -22,70 +22,22 @@
 #include <Library/IoLib.h>
 #include <Library/PcdLib.h>
 #include <Library/UefiBootServicesTableLib.h>
-#include <Library/UefiLib.h>
 
 #include <VExpressMotherBoard.h>
 
 // This 'enum' is needed as variations based on existing platform exist
 typedef enum {
-  ARM_FVP_VEXPRESS_UNKNOWN = 0,
-  ARM_FVP_VEXPRESS_A9x4,
-  ARM_FVP_VEXPRESS_A15x1,
-  ARM_FVP_VEXPRESS_A15x2,
-  ARM_FVP_VEXPRESS_A15x4,
-  ARM_FVP_VEXPRESS_A15x1_A7x1,
-  ARM_FVP_VEXPRESS_A15x4_A7x4,
-  ARM_FVP_VEXPRESS_AEMv8x4,
   ARM_FVP_BASE_AEMv8x4_AEMv8x4_GICV2,
-  ARM_FVP_BASE_AEMv8x4_AEMv8x4_GICV2_LEGACY,
   ARM_FVP_BASE_AEMv8x4_AEMv8x4_GICV3,
   ARM_FVP_FOUNDATION_GICV2,
-  ARM_FVP_FOUNDATION_GICV2_LEGACY,
   ARM_FVP_FOUNDATION_GICV3,
-  ARM_HW_A9x4,
-  ARM_HW_A15x2_A7x3,
-  ARM_HW_A15,
-  ARM_HW_A5
 } ARM_VEXPRESS_PLATFORM_ID;
 
-typedef struct {
-  ARM_VEXPRESS_PLATFORM_ID  Id;
-
-  // Flattened Device Tree (FDT) File
-  CONST EFI_GUID            *FdtGuid; /// Name of the FDT when present into the FV
-  CONST CHAR16              *FdtName; /// Name of the FDT when present into a File System
-} ARM_VEXPRESS_PLATFORM;
-
-// Array that contains the list of the VExpress based platform supported by this DXE driver
-extern CONST ARM_VEXPRESS_PLATFORM ArmVExpressPlatforms[];
-
-/**
-  Get information about the VExpress platform the firmware is running on given its Id.
-
-  @param[in]   PlatformId  Id of the VExpress platform.
-  @param[out]  Platform    Address where the pointer to the platform information
-                           (type ARM_VEXPRESS_PLATFORM*) should be stored.
-                           The returned pointer does not point to an allocated
-                           memory area.
-
-  @retval  EFI_SUCCESS    The platform information was returned.
-  @retval  EFI_NOT_FOUND  The platform was not recognised.
-
-**/
-EFI_STATUS
-ArmVExpressGetPlatformFromId (
-  IN  CONST ARM_VEXPRESS_PLATFORM_ID PlatformId,
-  OUT CONST ARM_VEXPRESS_PLATFORM**  Platform
-  );
-
 /**
 
   Get information about the VExpress platform the firmware is running on.
 
-  @param[out]  Platform   Address where the pointer to the platform information
-                          (type ARM_VEXPRESS_PLATFORM*) should be stored.
-                          The returned pointer does not point to an allocated
-                          memory area.
+  @param[out]  PlatformId ARM_VEXPRESS_PLATFORM_ID value of current platform
 
   @retval  EFI_SUCCESS    The platform information was returned.
   @retval  EFI_NOT_FOUND  The platform was not recognised.
@@ -93,7 +45,7 @@ ArmVExpressGetPlatformFromId (
 **/
 EFI_STATUS
 ArmVExpressGetPlatform (
-  OUT CONST ARM_VEXPRESS_PLATFORM** Platform
+  OUT   ARM_VEXPRESS_PLATFORM_ID    *PlatformId
   );
 
 #endif // __ARM_VEXPRESS_INTERNAL_H__
diff --git a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressPkg.dec b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressPkg.dec
index 39f046541502..3814513c2241 100644
--- a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressPkg.dec
+++ b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressPkg.dec
@@ -51,21 +51,3 @@ [PcdsFixedAtBuild.common]
   # Device path of block device on which Fastboot will flash partitions
   #
   gArmVExpressTokenSpaceGuid.PcdAndroidFastbootNvmDevicePath|""|VOID*|0x00000004
-
-  # FVP platforms : install FDT from SemiHosting
-  gArmVExpressTokenSpaceGuid.PcdFvpFdtDevicePathsBase|L"VenHw(C5B9C74A-6D72-4719-99AB-C59F199091EB)/"|VOID*|0x00000005
-  # HW platforms : install FDT from NOR Flash
-  gArmVExpressTokenSpaceGuid.PcdHwFdtDevicePathsBase|L"VenHw(E7223039-5836-41E1-B542-D7EC736C5E59)/"|VOID*|0x00000006
-
-  #
-  # ARM Versatile Express FDT Guids
-  #
-
-  # AArch64 FVP platforms
-  gArmVExpressTokenSpaceGuid.PcdFdtFvpVExpressAEMv8x4|{ 0xa8, 0x95, 0x5f, 0xf6, 0x32, 0x7b, 0xf3, 0x16, 0x12, 0x32, 0x45, 0x50, 0xbd, 0x54, 0xca, 0xe5 }|VOID*|0x00000010
-  gArmVExpressTokenSpaceGuid.PcdFdtFvpBaseAEMv8x4GicV2|{ 0x66, 0xcf, 0x57, 0xa4, 0xac, 0x7e, 0x7f, 0x3d, 0x21, 0x88, 0x3a, 0x58, 0x3c, 0x27, 0xd7, 0xe8 }|VOID*|0x00000011
-  gArmVExpressTokenSpaceGuid.PcdFdtFvpBaseAEMv8x4GicV2Legacy|{ 0x8b, 0xcb, 0xe0, 0x14, 0xd1, 0x46, 0x79, 0xae, 0x7f, 0x20, 0xcf, 0x84, 0x22, 0xc7, 0x94, 0x4a }|VOID*|0x00000012
-  gArmVExpressTokenSpaceGuid.PcdFdtFvpBaseAEMv8x4GicV3|{ 0x4d, 0x03, 0xb8, 0x77, 0x63, 0x25, 0x0a, 0x7f, 0xe9, 0x72, 0xfa, 0x68, 0x74, 0xc7, 0x5e, 0xb5 }|VOID*|0x00000013
-  gArmVExpressTokenSpaceGuid.PcdFdtFvpFoundationGicV2|{ 0x36, 0x4f, 0x61, 0x92, 0x86, 0xb1, 0xa2, 0x16, 0x32, 0x65, 0x35, 0x3f, 0x01, 0xf3, 0x3b, 0x64 }|VOID*|0x00000014
-  gArmVExpressTokenSpaceGuid.PcdFdtFvpFoundationGicV2Legacy|{ 0xf6, 0xcb, 0x9d, 0x86, 0x38, 0x74, 0x8a, 0xb0, 0xfe, 0x40, 0x08, 0x0f, 0x3f, 0xb3, 0x50, 0x7c }|VOID*|0x00000015
-  gArmVExpressTokenSpaceGuid.PcdFdtFvpFoundationGicV3|{ 0x51, 0xd0, 0x75, 0x6b, 0x9d, 0x35, 0x1b, 0x1b, 0xa6, 0xc6, 0xab, 0xa0, 0x90, 0xf9, 0xf0, 0x0a }|VOID*|0x00000016
-- 
2.9.3

Re: [PATCH 0/6] Embedded|ArmPlatformPkg: spring cleaning + DtPlatformDxe switch

[Laszlo Ersek]

On 03/29/17 15:48, Ard Biesheuvel wrote:
> This implements the upstream part of switching VExpress TC2 and the AArch64
> FVP Foundation/Base models to the new DtPlatformDxe driver, which is much
> simpler and only allows ACPI or DT to be enabled but never both.
> 
> Patches #1 and #2 tweak the new DtPlatformDxe so it can choose from several
> builtin DTBs depending on the actual platform detected at runtime.
> 
> Patches #3, #4 and #5 are basically preparatory cleanup that allows patch #6
> to radically change ArmFvpDxe without affecting other users.
> 
> Patch #6 removes all the handling of FDT device paths, string PCDs that
> have to be initialized to 128 spaces and other awkwardness, and simply
> sets the default DTB file section index based on the detected platform.
> 
> Ard Biesheuvel (6):
>   EmbeddedPkg/DtPlatformDxe: allow multiple entries in DTB FV file
>   EmbeddedPkg/DtPlatformDxe: declare symbolic name for FILE_GUID
>   ArmPlatformPkg/ArmShellCmdRunAxf: remove BdsLib dependency
>   ArmPlatformPkg/ArmVExpressDxe: remove ARM support
>   ArmPlatformPkg/ArmVExpressDxe: remove unused cruft from ArmHwDxe
>   ArmPlatformPkg/ArmVExpressDxe: simply FDT handling in ArmFvpDxe
> 
>  ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/AArch64/ArmFvpDxeAArch64.c |  60 +++------
>  ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/Arm/ArmFvpDxeArm.c         |  84 ------------
>  ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/ArmFvpDxe.c                | 134 ++------------------
>  ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/ArmFvpDxe.inf              |  42 ++----
>  ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/ArmHwDxe.c                 |  43 +------
>  ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/ArmHwDxe.inf               |   3 -
>  ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/ArmVExpressCommon.c        |  48 -------
>  ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/ArmVExpressInternal.h      |  52 +-------
>  ArmPlatformPkg/ArmVExpressPkg/ArmVExpressPkg.dec                        |  28 ----
>  ArmPlatformPkg/Library/ArmShellCmdRunAxf/ArmShellCmdRunAxf.inf          |   1 -
>  ArmPlatformPkg/Library/ArmShellCmdRunAxf/RunAxf.c                       |  58 ++++++++-
>  EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c                       |   5 +-
>  EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf                     |   6 +-
>  EmbeddedPkg/EmbeddedPkg.dec                                             |   6 +
>  14 files changed, 108 insertions(+), 462 deletions(-)
>  delete mode 100644 ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/Arm/ArmFvpDxeArm.c
>  delete mode 100644 ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/ArmVExpressCommon.c
> 

What do you think of the following proposal instead (I have no strong
feelings about this, just picking your mind):

(1) In commit 779cc439e881 ("EmbeddedPkg: add DT platform driver to
select between DT and ACPI", 2017-03-27), a driver was added that, based
on an HII checkbox,

- either produces the platform-has-ACPI NULL protocol,

- or immediately installs the DTB, found in any FV section.

(Glossing over the details here, such as, if there is no DTB embedded in
the firmware image, we always go with ACPI etc.)

I reviewed that patch. I slightly disliked that in the DT case, we
immediately installed the DT as a sysconfig table, but I figured, given
that this driver actually "owned" the DTB, it was okay. Therefore, I
didn't suggest producing the platform-has-DeviceTree NULL protocol, and
then acting upon that protocol within the exact same driver (i.e., to
install the DTB as a sysconfig table in a protocol notify).

(2) I feel that, with this set, the DTB ownership is changing. I think
the following restructuring would be an improvement:

- DtPlatformDxe should only concern itself with translating the HII
checkbox to the appropriate NULL protocol, namely platform-has-ACPI
versus platform-has-DeviceTree. A corresponding rename for the driver
might be in order too.

- The owner of the (multiple possible) DTBs is now ArmVExpressDxe (or
any similar driver included in any given platform DSC). IMO, this is the
driver to look up the DTB within the firmware image, based on the
platform type determination that it already performs. Then,
ArmVExpressDxe should install the selected DTB as a sysconfig table,
specifically in a protocol notify callback for platform-has-DeviceTree.

(3) Here's an excerpt from the message of commit 65a69b214840,
"EmbeddedPkg: introduce EDKII Platform Has Device Tree GUID", 2017-03-17:

> In the DXE phase, the protocol is meant to be consumed by the platform
> driver that
> - owns the Device Tree description of the hardware, and
> - is responsible for installing it as a system configuration table.

I think that the above description matches the current situation 1:1,
and that the proposed driver structuring would keep the responsibilities
better separated.

Plus, you could eliminate the butt-ugly BEFORE depex :)

The "EmbeddedPkg/Drivers/DtPlatformDxe" driver is not yet used in any
upstream edk2 platform DSC, so I think it's the appropriate time to set
the responsibilities right. I haven't looked at your series

  [Linaro-uefi] [PATCH 0/6] EDK2 spring cleaning -- OpenPlatformPkg
                            edition
  https://lists.linaro.org/pipermail/linaro-uefi/2017-March/004196.html

yet, but there you call both patch sets inter-dependent, so I guess this
is the one we should be discussing first.

(4) I'm missing a whole lot of details about ArmVExpressDxe, so the
above might not be feasible or desirable. For example, while it seems to
identify and expose the DTB to install -- very elaborately, as you say
--, ultimately it only sets PcdFdtDevicePaths.

The PCD is then consumed by "EmbeddedPkg/Drivers/FdtPlatformDxe". As far
as I can see, this driver reads the DTB from the EFI system partition,
as directed by the PCD?

In this patch set, you don't seem to touch FdtPlatformDxe, so I think
that you are replacing all of the FdtPlatformDxe functionality by
embedding the DTBs in the FV image.

In that case, my proposal above should not conflict with (or require
updates for) FdtPlatformDxe either.

(5) Anyway, I just wanted to float the idea. What do you think of it?

Thanks
Laszlo

Re: [PATCH 0/6] Embedded|ArmPlatformPkg: spring cleaning + DtPlatformDxe switch

[Ard Biesheuvel]

On 30 March 2017 at 19:28, Laszlo Ersek <lersek@redhat.com> wrote:
[...]
>
> What do you think of the following proposal instead (I have no strong
> feelings about this, just picking your mind):
>
> (1) In commit 779cc439e881 ("EmbeddedPkg: add DT platform driver to
> select between DT and ACPI", 2017-03-27), a driver was added that, based
> on an HII checkbox,
>
> - either produces the platform-has-ACPI NULL protocol,
>
> - or immediately installs the DTB, found in any FV section.
>
> (Glossing over the details here, such as, if there is no DTB embedded in
> the firmware image, we always go with ACPI etc.)
>
> I reviewed that patch. I slightly disliked that in the DT case, we
> immediately installed the DT as a sysconfig table, but I figured, given
> that this driver actually "owned" the DTB, it was okay. Therefore, I
> didn't suggest producing the platform-has-DeviceTree NULL protocol, and
> then acting upon that protocol within the exact same driver (i.e., to
> install the DTB as a sysconfig table in a protocol notify).
>
> (2) I feel that, with this set, the DTB ownership is changing. I think
> the following restructuring would be an improvement:
>
> - DtPlatformDxe should only concern itself with translating the HII
> checkbox to the appropriate NULL protocol, namely platform-has-ACPI
> versus platform-has-DeviceTree. A corresponding rename for the driver
> might be in order too.
>
> - The owner of the (multiple possible) DTBs is now ArmVExpressDxe (or
> any similar driver included in any given platform DSC). IMO, this is the
> driver to look up the DTB within the firmware image, based on the
> platform type determination that it already performs. Then,
> ArmVExpressDxe should install the selected DTB as a sysconfig table,
> specifically in a protocol notify callback for platform-has-DeviceTree.
>

Well, what I would like to avoid is having awareness of DT in more
places than necessary. The nice thing of having a DtPlatformDxe driver
that takes care of it all is that removing the module (and the
PlatformHasAcpi NULL library class resolution) makes a platform
completely ACPI only

> (3) Here's an excerpt from the message of commit 65a69b214840,
> "EmbeddedPkg: introduce EDKII Platform Has Device Tree GUID", 2017-03-17:
>
>> In the DXE phase, the protocol is meant to be consumed by the platform
>> driver that
>> - owns the Device Tree description of the hardware, and
>> - is responsible for installing it as a system configuration table.
>
> I think that the above description matches the current situation 1:1,
> and that the proposed driver structuring would keep the responsibilities
> better separated.
>
> Plus, you could eliminate the butt-ugly BEFORE depex :)
>
> The "EmbeddedPkg/Drivers/DtPlatformDxe" driver is not yet used in any
> upstream edk2 platform DSC, so I think it's the appropriate time to set
> the responsibilities right. I haven't looked at your series
>
>   [Linaro-uefi] [PATCH 0/6] EDK2 spring cleaning -- OpenPlatformPkg
>                             edition
>   https://lists.linaro.org/pipermail/linaro-uefi/2017-March/004196.html
>
> yet, but there you call both patch sets inter-dependent, so I guess this
> is the one we should be discussing first.
>
> (4) I'm missing a whole lot of details about ArmVExpressDxe, so the
> above might not be feasible or desirable. For example, while it seems to
> identify and expose the DTB to install -- very elaborately, as you say
> --, ultimately it only sets PcdFdtDevicePaths.
>
> The PCD is then consumed by "EmbeddedPkg/Drivers/FdtPlatformDxe". As far
> as I can see, this driver reads the DTB from the EFI system partition,
> as directed by the PCD?
>
> In this patch set, you don't seem to touch FdtPlatformDxe, so I think
> that you are replacing all of the FdtPlatformDxe functionality by
> embedding the DTBs in the FV image.
>
> In that case, my proposal above should not conflict with (or require
> updates for) FdtPlatformDxe either.
>
> (5) Anyway, I just wanted to float the idea. What do you think of it?
>

I want to get rid of FdtPlatformDxe. It uses things like semihosting
device paths to pull .dtb files that are known by name from various
places. While this is nice for debugging in theory, nobody actually
uses it. On it does not belong on a production system at all. Also,
for a laugh, please check out
7aec2926b926ad90d09fb026af0ee04c4c831237, specifically the hunk that
adds gEmbeddedTokenSpaceGuid.PcdFdtDevicePaths.

I agree about the butt-uglines of BEFORE depexes, so what I would
prefer is an alternative way to allow DtPlatformDxe to 'own' DT
installation entirely. To enforce the ordering, I could also update
DtPlatformDxe to load the correct FV section at EndOfDxe. Would that
be acceptable to you?

Re: [PATCH 0/6] Embedded|ArmPlatformPkg: spring cleaning + DtPlatformDxe switch

[Laszlo Ersek]

On 03/31/17 11:22, Ard Biesheuvel wrote:
> On 30 March 2017 at 19:28, Laszlo Ersek <lersek@redhat.com> wrote:
> [...]
>>
>> What do you think of the following proposal instead (I have no strong
>> feelings about this, just picking your mind):
>>
>> (1) In commit 779cc439e881 ("EmbeddedPkg: add DT platform driver to
>> select between DT and ACPI", 2017-03-27), a driver was added that, based
>> on an HII checkbox,
>>
>> - either produces the platform-has-ACPI NULL protocol,
>>
>> - or immediately installs the DTB, found in any FV section.
>>
>> (Glossing over the details here, such as, if there is no DTB embedded in
>> the firmware image, we always go with ACPI etc.)
>>
>> I reviewed that patch. I slightly disliked that in the DT case, we
>> immediately installed the DT as a sysconfig table, but I figured, given
>> that this driver actually "owned" the DTB, it was okay. Therefore, I
>> didn't suggest producing the platform-has-DeviceTree NULL protocol, and
>> then acting upon that protocol within the exact same driver (i.e., to
>> install the DTB as a sysconfig table in a protocol notify).
>>
>> (2) I feel that, with this set, the DTB ownership is changing. I think
>> the following restructuring would be an improvement:
>>
>> - DtPlatformDxe should only concern itself with translating the HII
>> checkbox to the appropriate NULL protocol, namely platform-has-ACPI
>> versus platform-has-DeviceTree. A corresponding rename for the driver
>> might be in order too.
>>
>> - The owner of the (multiple possible) DTBs is now ArmVExpressDxe (or
>> any similar driver included in any given platform DSC). IMO, this is the
>> driver to look up the DTB within the firmware image, based on the
>> platform type determination that it already performs. Then,
>> ArmVExpressDxe should install the selected DTB as a sysconfig table,
>> specifically in a protocol notify callback for platform-has-DeviceTree.
>>
> 
> Well, what I would like to avoid is having awareness of DT in more
> places than necessary. The nice thing of having a DtPlatformDxe driver
> that takes care of it all is that removing the module (and the
> PlatformHasAcpi NULL library class resolution) makes a platform
> completely ACPI only

But ArmVExpressDxe remains DT-aware anyway, because it selects the right
DT to install, no? In this patch set, patch #6 does not set an abstract
"platform identifier" PCD, what it sets is called

  PcdDtPlatformDefaultDtbSectionIndex

which has "DTB" in the name.

And, if you remove the DtPlatformDxe driver only (but not
ArmVExpressDxe), for permanent ACPI adoption, then
PcdDtPlatformDefaultDtbSectionIndex will become useless (and maybe
ArmVExpressDxe will become wholly useless too).

So I think you'd always remove more than just DtPlatformDxe and the
PlatformHasAcpi plugin lib.

> 
>> (3) Here's an excerpt from the message of commit 65a69b214840,
>> "EmbeddedPkg: introduce EDKII Platform Has Device Tree GUID", 2017-03-17:
>>
>>> In the DXE phase, the protocol is meant to be consumed by the platform
>>> driver that
>>> - owns the Device Tree description of the hardware, and
>>> - is responsible for installing it as a system configuration table.
>>
>> I think that the above description matches the current situation 1:1,
>> and that the proposed driver structuring would keep the responsibilities
>> better separated.
>>
>> Plus, you could eliminate the butt-ugly BEFORE depex :)
>>
>> The "EmbeddedPkg/Drivers/DtPlatformDxe" driver is not yet used in any
>> upstream edk2 platform DSC, so I think it's the appropriate time to set
>> the responsibilities right. I haven't looked at your series
>>
>>   [Linaro-uefi] [PATCH 0/6] EDK2 spring cleaning -- OpenPlatformPkg
>>                             edition
>>   https://lists.linaro.org/pipermail/linaro-uefi/2017-March/004196.html
>>
>> yet, but there you call both patch sets inter-dependent, so I guess this
>> is the one we should be discussing first.
>>
>> (4) I'm missing a whole lot of details about ArmVExpressDxe, so the
>> above might not be feasible or desirable. For example, while it seems to
>> identify and expose the DTB to install -- very elaborately, as you say
>> --, ultimately it only sets PcdFdtDevicePaths.
>>
>> The PCD is then consumed by "EmbeddedPkg/Drivers/FdtPlatformDxe". As far
>> as I can see, this driver reads the DTB from the EFI system partition,
>> as directed by the PCD?
>>
>> In this patch set, you don't seem to touch FdtPlatformDxe, so I think
>> that you are replacing all of the FdtPlatformDxe functionality by
>> embedding the DTBs in the FV image.
>>
>> In that case, my proposal above should not conflict with (or require
>> updates for) FdtPlatformDxe either.
>>
>> (5) Anyway, I just wanted to float the idea. What do you think of it?
>>
> 
> I want to get rid of FdtPlatformDxe. It uses things like semihosting
> device paths to pull .dtb files that are known by name from various
> places. While this is nice for debugging in theory, nobody actually
> uses it. On it does not belong on a production system at all. Also,
> for a laugh, please check out
> 7aec2926b926ad90d09fb026af0ee04c4c831237, specifically the hunk that
> adds gEmbeddedTokenSpaceGuid.PcdFdtDevicePaths.

Right.

My suggestion seems compatible with killing FdtPlatformDxe for good though.

> I agree about the butt-uglines of BEFORE depexes, so what I would
> prefer is an alternative way to allow DtPlatformDxe to 'own' DT
> installation entirely.

What do you have in mind? The below, or something in addition to that?

> To enforce the ordering, I could also update
> DtPlatformDxe to load the correct FV section at EndOfDxe. Would that
> be acceptable to you?

I think it remains a bit ugly that DT selection and DT installation
happen in two different drivers. Again, if you rip DtPlatformDxe out of
a platform at some point -- because the platform becomes ACPI only --
there won't be any reason to preserve the other driver that does the DT
selection (or minimally, the DT-selection logic in the other driver).
This seems to imply that DT selection and DT installation belong in the
same place.

Also, you know my opinion about doing stuff in "late" callbacks :)

Nonetheless, I think an EndOfDxe callback would be a significant
improvement over the BEFORE depex.

So, feel free to pick whichever method you deem best, I'm ready to ack
either; I just wanted to raise some ideas. I don't intend to obsess
about them forever :)

Thanks!
Laszlo