[edk2-platforms][PATCH 1/2] RPi: rip out FdtDxe logic to use internal DTB

["Andrei Warkentin" <andrey.warkentin@gmail.com>]

Initially, FdtDxe used an internal (embedded in UEFI) FDT,
because it was neither understood how to consume the
one loaded by the VideoCore firmware, nor understood just
how important it is to use the DTB provided by config.txt.

Embedding the DT was a bad idea, because:
- Permanently stale
- No overlays

Also, on devices like the Pi 4 you _have_ to have a DT
around for the start4 VPU firmware to pick up, otherwise
the board is left in an inconsistent state. So we're being
proscriptive now about DT use with config.txt, which means
this internal DT logic is deadc code.

Further FdtDxe cleanups are possible and will be handled
separately, specifically:
- probably no need to use a separate allocation for patched DT (optimize memory used)
- suspicious use of EfiBootServicesData (I filed https://github.com/ARM-software/ebbr/issues/45 to sort out the real requirements)

Testing: Booted Ubuntu 18.04 on Pi 2B (1.2).

Signed-off-by: Andrei Warkentin <andrey.warkentin@gmail.com>
---
 Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c   | 206 ++++----------------
 Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.inf |   4 -
 Platform/RaspberryPi/RPi3/RPi3.fdf             |  11 --
 Platform/RaspberryPi/RPi4/RPi4.fdf             |   8 -
 Platform/RaspberryPi/RaspberryPi.dec           |   7 -
 5 files changed, 38 insertions(+), 198 deletions(-)

diff --git a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c
index e7143f57..187b9566 100644
--- a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c
+++ b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c
@@ -335,90 +335,6 @@ CleanSimpleFramebuffer (
   return EFI_SUCCESS;
 }
 
-#define MAX_CMDLINE_SIZE    512
-
-STATIC
-EFI_STATUS
-UpdateBootArgs (
-  VOID
-  )
-{
-  INTN          Node;
-  INTN          Retval;
-  EFI_STATUS    Status;
-  CHAR8         *CommandLine;
-
-  //
-  // Locate the /chosen node
-  //
-  Node = fdt_path_offset (mFdtImage, "/chosen");
-  if (Node < 0) {
-    DEBUG ((DEBUG_ERROR, "%a: failed to locate /chosen node\n", __FUNCTION__));
-    return EFI_NOT_FOUND;
-  }
-
-  //
-  // If /chosen/bootargs already exists, we want to add a space character
-  // before adding the firmware supplied arguments. However, the RpiFirmware
-  // protocol expects a 32-bit aligned buffer. So let's allocate 4 bytes of
-  // slack, and skip the first 3 when passing this buffer into libfdt.
-  //
-  CommandLine = AllocatePool (MAX_CMDLINE_SIZE) + 4;
-  if (!CommandLine) {
-    DEBUG ((DEBUG_ERROR, "%a: failed to allocate memory\n", __FUNCTION__));
-    return EFI_OUT_OF_RESOURCES;
-  }
-
-  //
-  // Get the command line from the firmware
-  //
-  Status = mFwProtocol->GetCommandLine (MAX_CMDLINE_SIZE, CommandLine + 4);
-  if (EFI_ERROR (Status)) {
-    DEBUG ((DEBUG_ERROR, "%a: failed to retrieve command line\n", __FUNCTION__));
-    return Status;
-  }
-
-  if (AsciiStrLen (CommandLine + 4) == 0) {
-    DEBUG ((DEBUG_INFO, "%a: empty command line received\n", __FUNCTION__));
-    return EFI_SUCCESS;
-  }
-
-  CommandLine[3] = ' ';
-
-  Retval = fdt_appendprop_string (mFdtImage, Node, "bootargs", &CommandLine[3]);
-  if (Retval != 0) {
-    DEBUG ((DEBUG_ERROR, "%a: failed to set /chosen/bootargs property (%d)\n",
-      __FUNCTION__, Retval));
-  }
-
-  DEBUG_CODE_BEGIN ();
-    CONST CHAR8    *Prop;
-    INT32         Length;
-    INT32         Index;
-
-    Node = fdt_path_offset (mFdtImage, "/chosen");
-    ASSERT (Node >= 0);
-
-    Prop = fdt_getprop (mFdtImage, Node, "bootargs", &Length);
-    ASSERT (Prop != NULL);
-
-    DEBUG ((DEBUG_INFO, "Command line set from firmware (length %d):\n'", Length));
-
-    for (Index = 0; Index < Length; Index++, Prop++) {
-      if (*Prop == '\0') {
-        continue;
-      }
-      DEBUG ((DEBUG_INFO, "%c", *Prop));
-    }
-
-    DEBUG ((DEBUG_INFO, "'\n"));
-  DEBUG_CODE_END ();
-
-  FreePool (CommandLine - 4);
-  return EFI_SUCCESS;
-}
-
-
 /**
   @param  ImageHandle   of the loaded driver
   @param  SystemTable   Pointer to the System Table
@@ -435,13 +351,10 @@ FdtDxeInitialize (
   IN EFI_SYSTEM_TABLE   *SystemTable
   )
 {
+  INT32      Retval;
   EFI_STATUS Status;
-  EFI_GUID   *FdtGuid;
-  VOID       *FdtImage;
   UINTN      FdtSize;
-  INT32      Retval;
-  UINT32     BoardRevision;
-  BOOLEAN    Internal;
+  VOID       *FdtImage = NULL;
 
   if (PcdGet32 (PcdOptDeviceTree) == 0) {
     DEBUG ((DEBUG_INFO, "Device Tree disabled per user configuration\n"));
@@ -450,77 +363,28 @@ FdtDxeInitialize (
 
   Status = gBS->LocateProtocol (&gRaspberryPiFirmwareProtocolGuid, NULL,
                   (VOID**)&mFwProtocol);
-  ASSERT_EFI_ERROR (Status);
+  if (mFwProtocol == NULL) {
+    /*
+     * Impossible because of the depex...
+     */
+    ASSERT_EFI_ERROR (Status);
+    return EFI_NOT_FOUND;
+  }
 
-  Internal = FALSE;
   FdtImage = (VOID*)(UINTN)PcdGet32 (PcdFdtBaseAddress);
   Retval = fdt_check_header (FdtImage);
-  if (Retval == 0) {
-    /*
-     * Have FDT passed via config.txt.
-     */
-    FdtSize = fdt_totalsize (FdtImage);
-    DEBUG ((DEBUG_INFO, "Device Tree passed via config.txt (0x%lx bytes)\n", FdtSize));
-    Status = EFI_SUCCESS;
-  } else {
-    /*
-     * Use one of the embedded FDT's.
-     */
-    Internal = TRUE;
-    DEBUG ((DEBUG_INFO, "No/Bad Device Tree found at address 0x%p (%a), "
-      "looking up internal one...\n", FdtImage, fdt_strerror (Retval)));
+  if (Retval != 0) {
     /*
-     * Query the board revision to differentiate between models.
+     * Any one of:
+     * - Invalid config.txt device_tree_address (not PcdFdtBaseAddress)
+     * - Missing FDT for your Pi variant (if not overriding via device_tree=)
      */
-    Status = mFwProtocol->GetModelRevision (&BoardRevision);
-    if (EFI_ERROR (Status)) {
-      DEBUG ((DEBUG_ERROR, "Failed to get board type: %r\n", Status));
-      DEBUG ((DEBUG_INFO, "Using default internal Device Tree\n"));
-      FdtGuid = &gRaspberryPiDefaultFdtGuid;
-    } else {
-      // www.raspberrypi.org/documentation/hardware/raspberrypi/revision-codes/README.md
-      switch ((BoardRevision >> 4) & 0xFF) {
-      case 0x08:
-        DEBUG ((DEBUG_INFO, "Using Raspberry Pi 3 Model B internal Device Tree\n"));
-        FdtGuid = &gRaspberryPi3ModelBFdtGuid;
-        break;
-      case 0x0D:
-        DEBUG ((DEBUG_INFO, "Using Raspberry Pi 3 Model B+ internal Device Tree\n"));
-        FdtGuid = &gRaspberryPi3ModelBPlusFdtGuid;
-        break;
-      case 0x11:
-        DEBUG ((DEBUG_INFO, "Using Raspberry Pi 4 Model B internal Device Tree\n"));
-        FdtGuid = &gRaspberryPi4ModelBFdtGuid;
-        break;
-      default:
-        DEBUG ((DEBUG_INFO, "Using default internal Device Tree\n"));
-        FdtGuid = &gRaspberryPiDefaultFdtGuid;
-        break;
-      }
-    }
-    do {
-      Status = GetSectionFromAnyFv (FdtGuid, EFI_SECTION_RAW, 0, &FdtImage, &FdtSize);
-      if (Status == EFI_SUCCESS) {
-        if (fdt_check_header (FdtImage) != 0) {
-          Status = EFI_INCOMPATIBLE_VERSION;
-        }
-      }
-      // No retry needed if we are successful or are dealing with the default Fdt.
-      if ( (Status == EFI_SUCCESS) ||
-           (CompareGuid (FdtGuid, &gRaspberryPiDefaultFdtGuid)) )
-        break;
-      // Otherwise, try one more time with the default Fdt. An example of this
-      // is if we detected a non-default Fdt, that isn't included in the FDF.
-      DEBUG ((DEBUG_INFO, "Internal Device Tree was not found for this platform, "
-        "falling back to default...\n"));
-      FdtGuid = &gRaspberryPiDefaultFdtGuid;
-    } while (1);
+    DEBUG ((DEBUG_ERROR, "No devicetree passed via config.txt\n"));
+    return EFI_NOT_FOUND;
   }
 
-  if (EFI_ERROR (Status)) {
-    DEBUG ((DEBUG_ERROR, "Failed to locate Device Tree: %r\n", Status));
-    return Status;
-  }
+  FdtSize = fdt_totalsize (FdtImage);
+  DEBUG ((DEBUG_INFO, "Devicetree passed via config.txt (0x%lx bytes)\n", FdtSize));
 
   /*
    * Probably overkill.
@@ -529,12 +393,19 @@ FdtDxeInitialize (
   Status = gBS->AllocatePages (AllocateAnyPages, EfiBootServicesData,
                   EFI_SIZE_TO_PAGES (FdtSize), (EFI_PHYSICAL_ADDRESS*)&mFdtImage);
   if (EFI_ERROR (Status)) {
-    DEBUG ((DEBUG_ERROR, "Failed to allocate Device Tree: %r\n", Status));
-    return Status;
+    DEBUG ((DEBUG_ERROR, "Failed to allocate devicetree: %r\n", Status));
+    goto out;
   }
 
   Retval = fdt_open_into (FdtImage, mFdtImage, FdtSize);
-  ASSERT (Retval == 0);
+  if (Retval != 0) {
+     DEBUG ((DEBUG_ERROR, "fdt_open_into failed: %d\n", Retval));
+     goto out;
+  }
+
+  /*
+   * These are all best-effort.
+   */
 
   Status = SanitizePSCI ();
   if (EFI_ERROR (Status)) {
@@ -566,19 +437,18 @@ FdtDxeInitialize (
     Print (L"Failed to update USB compatible properties: %r\n", Status);
   }
 
-  if (Internal) {
-    /*
-     * A GPU-provided DTB already has the full command line.
-     */
-    Status = UpdateBootArgs ();
-    if (EFI_ERROR (Status)) {
-      Print (L"Failed to update boot arguments: %r\n", Status);
-    }
-  }
-
-  DEBUG ((DEBUG_INFO, "Installed Device Tree at address 0x%p\n", mFdtImage));
+  DEBUG ((DEBUG_INFO, "Installed devicetree at address %p\n", mFdtImage));
   Status = gBS->InstallConfigurationTable (&gFdtTableGuid, mFdtImage);
-  ASSERT_EFI_ERROR (Status);
+  if (EFI_ERROR (Status)) {
+     DEBUG ((DEBUG_ERROR, "Couldn't register devicetree: %r\n", Status));
+     goto out;
+  }
 
+out:
+  if (EFI_ERROR(Status)) {
+    if (mFdtImage != NULL) {
+      gBS->FreePages ((EFI_PHYSICAL_ADDRESS) mFdtImage, EFI_SIZE_TO_PAGES (FdtSize));
+    }
+  }
   return Status;
 }
diff --git a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.inf b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.inf
index fc37353f..49ba5ba1 100644
--- a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.inf
+++ b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.inf
@@ -35,10 +35,6 @@
 
 [Guids]
   gFdtTableGuid
-  gRaspberryPi3ModelBFdtGuid
-  gRaspberryPi3ModelBPlusFdtGuid
-  gRaspberryPi4ModelBFdtGuid
-  gRaspberryPiDefaultFdtGuid
 
 [Protocols]
   gRaspberryPiFirmwareProtocolGuid              ## CONSUMES
diff --git a/Platform/RaspberryPi/RPi3/RPi3.fdf b/Platform/RaspberryPi/RPi3/RPi3.fdf
index daedc443..e854cd21 100644
--- a/Platform/RaspberryPi/RPi3/RPi3.fdf
+++ b/Platform/RaspberryPi/RPi3/RPi3.fdf
@@ -303,17 +303,6 @@ READ_LOCK_STATUS   = TRUE
   #
   INF Platform/RaspberryPi/Drivers/LogoDxe/LogoDxe.inf
 
-  #
-  # Device Tree support (used by FdtDxe)
-  # GUIDs should match gRaspberryPi#####FdtGuid's from the .dec
-  #
-  FILE FREEFORM = DF5DA223-1D27-47C3-8D1B-9A41B55A18BC {
-    SECTION RAW = Platform/RaspberryPi/$(PLATFORM_NAME)/DeviceTree/bcm2710-rpi-3-b.dtb
-  }
-  FILE FREEFORM = 3D523012-73FE-40E5-892E-1A4DF60F3C0C {
-    SECTION RAW = Platform/RaspberryPi/$(PLATFORM_NAME)/DeviceTree/bcm2710-rpi-3-b-plus.dtb
-  }
-
 [FV.FVMAIN_COMPACT]
 FvAlignment        = 16
 ERASE_POLARITY     = 1
diff --git a/Platform/RaspberryPi/RPi4/RPi4.fdf b/Platform/RaspberryPi/RPi4/RPi4.fdf
index c3e9cfc4..b1f7aa23 100644
--- a/Platform/RaspberryPi/RPi4/RPi4.fdf
+++ b/Platform/RaspberryPi/RPi4/RPi4.fdf
@@ -307,14 +307,6 @@ READ_LOCK_STATUS   = TRUE
   #
   INF Platform/RaspberryPi/Drivers/LogoDxe/LogoDxe.inf
 
-  #
-  # Device Tree support (used by FdtDxe)
-  # GUIDs should match gRaspberryPi#####FdtGuid's from the .dec
-  #
- FILE FREEFORM = 80AB6833-CAE4-4CEE-B59D-EB2039B05551 {
-    SECTION RAW = Platform/RaspberryPi/$(PLATFORM_NAME)/DeviceTree/bcm2711-rpi-4-b.dtb
-  }
-
 [FV.FVMAIN_COMPACT]
 FvAlignment        = 16
 ERASE_POLARITY     = 1
diff --git a/Platform/RaspberryPi/RaspberryPi.dec b/Platform/RaspberryPi/RaspberryPi.dec
index b66322be..66ef6186 100644
--- a/Platform/RaspberryPi/RaspberryPi.dec
+++ b/Platform/RaspberryPi/RaspberryPi.dec
@@ -26,13 +26,6 @@
   gRaspberryPiTokenSpaceGuid = {0xCD7CC258, 0x31DB, 0x11E6, {0x9F, 0xD3, 0x63, 0xB0, 0xB8, 0xEE, 0xD6, 0xB5}}
   gRaspberryPiEventResetGuid = {0xCD7CC258, 0x31DB, 0x11E6, {0x9F, 0xD3, 0x63, 0xB4, 0xB4, 0xE4, 0xD4, 0xB4}}
   gConfigDxeFormSetGuid = {0xCD7CC258, 0x31DB, 0x22E6, {0x9F, 0x22, 0x63, 0xB0, 0xB8, 0xEE, 0xD6, 0xB5}}
-  # GUIDs used by FdtDxe to serve a Device Tree at runtime. Not all of these need to apply
-  # to the current platform or match an actual FDF binary, but they need to be defined.
-  gRaspberryPi3ModelBFdtGuid = { 0xDF5DA223, 0x1D27, 0x47C3, { 0x8D, 0x1B, 0x9A, 0x41, 0xB5, 0x5A, 0x18, 0xBC } }
-  gRaspberryPi3ModelBPlusFdtGuid = { 0x3D523012, 0x73FE, 0x40E5, { 0x89, 0x2E, 0x1A, 0x4D, 0xF6, 0x0F, 0x3C, 0x0C } }
-  gRaspberryPi4ModelBFdtGuid = { 0x80AB6833, 0xCAE4, 0x4CEE, { 0xB5, 0x9D, 0xEB, 0x20, 0x39, 0xB0, 0x55, 0x51 } }
-  # Default Fdt to serve if the hardware model can't be detected. Should match one of the above.
-  gRaspberryPiDefaultFdtGuid = {0xDF5DA223, 0x1D27, 0x47C3, { 0x8D, 0x1B, 0x9A, 0x41, 0xB5, 0x5A, 0x18, 0xBC}}
 
 [PcdsFixedAtBuild.common]
   #
-- 
2.17.1