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

["Pete Batard" <pete@akeo.ie>]

Two non-blocking minor remarks inline:

On 2020.04.30 22:16, Andrei Warkentin wrote:
> 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;

I don't believe the NULL assignation is needed...

>   
>     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);

...since no code actually uses that variable before we assign it here. 
But I don't see a problem keeping it.

>     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));

If we're going to modify that line, maybe we want to switch to using 
'%ld' instead of '0x%lx' as hex sizes aren't enormously helpful when 
dealing with user-handled files.

But that's not something I want to respin this patch for, especially for 
debug output, so 0x%lx is fine too.

>   
>     /*
>      * 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]
>     #
> 

Reviewed-by: Pete Batard <pete@akeo.ie>
Tested-on: Pi 3B, Pi 3B+, Pi 4B, with and without a Device Tree provided