Re: [edk2-platforms: PATCH 1/1] Platforms/RPi3: Add multiple embedded Device Tree selection

["Leif Lindholm" <leif.lindholm@linaro.org>]

On Mon, Aug 12, 2019 at 02:06:34PM +0100, Pete Batard wrote:
> The Raspberry Pi 3 platform currently has 2 different models, each with a
> different Device Tree. Rather than embedding a single one, and requiring
> users to manually provide the other, this patch ensures that we now embed
> both and and serve the relevant one at runtime.
> 
> Signed-off-by: Pete Batard <pete@akeo.ie>

Changeset looks very useful. Some style issues. And one question.

First the question: is there no practical size restriction on the
firmware image? This is a 24k increase in image size.

> ---
>  Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c   | 57 ++++++++++++++++----
>  Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.inf |  3 +-
>  Platform/RaspberryPi/RPi3/RPi3.dec                  |  3 +-
>  Platform/RaspberryPi/RPi3/RPi3.fdf                  |  6 ++-
>  4 files changed, 55 insertions(+), 14 deletions(-)
> 
> diff --git a/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c b/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c
> index c84e5b2767e2..7c0ab75e7cb1 100644
> --- a/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c
> +++ b/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c
> @@ -20,6 +20,11 @@
>  
>  #include <Guid/Fdt.h>
>  
> +CONST  EFI_GUID                         *mFdtGuid[] = {
> +                                           &gRaspberryPiFdtGuid1,
> +                                           &gRaspberryPiFdtGuid2,
> +                                        };
> +
>  STATIC VOID                             *mFdtImage;
>  
>  STATIC RASPBERRY_PI_FIRMWARE_PROTOCOL   *mFwProtocol;
> @@ -368,7 +373,9 @@ FdtDxeInitialize (
>    EFI_STATUS Status;
>    VOID       *FdtImage;
>    UINTN      FdtSize;
> +  UINTN      FdtIndex;
>    INT32      Retval;
> +  UINT32     BoardRevision;
>    BOOLEAN    Internal;
>  
>    Status = gBS->LocateProtocol (&gRaspberryPiFirmwareProtocolGuid, NULL,
> @@ -383,13 +390,41 @@ FdtDxeInitialize (
>       * Have FDT passed via config.txt.
>       */
>      FdtSize = fdt_totalsize (FdtImage);

This
> -    DEBUG ((DEBUG_INFO, "DTB passed via config.txt of 0x%lx bytes\n", FdtSize));
> +    DEBUG ((DEBUG_INFO, "Device Tree passed via config.txt (0x%lx bytes)\n", FdtSize));
looks unrelated to the changeset.

>      Status = EFI_SUCCESS;
>    } else {
> +    /*
> +     * Use one of the embedded FDT's.
> +     */
>      Internal = TRUE;

This DEBUG changes bit
> -    DEBUG ((DEBUG_INFO, "No/bad FDT at %p (%a), trying internal DTB...\n",
> -      FdtImage, fdt_strerror (Retval)));
> -    Status = GetSectionFromAnyFv (&gRaspberryPiFdtFileGuid, EFI_SECTION_RAW, 0,
> +    DEBUG ((DEBUG_INFO, "No/Bad Device Tree found at address 0x%p (%a), "
> +      "looking up internal one...\n", FdtImage, fdt_strerror (Retval)));
to here looks completely unrelated to the changeset.
Which garbles the diff somewhat with regards to the functional line in
the middle..

> +    /*
> +     * Query the board revision to differentiate between models.
> +     */
> +    Status = mFwProtocol->GetModelRevision (&BoardRevision);
> +    if (EFI_ERROR (Status)) {
> +      FdtIndex = 0;
> +      DEBUG ((DEBUG_ERROR, "Failed to get board type: %r\n", Status));
> +    } else {
> +      // www.raspberrypi.org/documentation/hardware/raspberrypi/revision-codes/README.md
> +      switch ((BoardRevision >> 4) & 0xFF) {
> +      case 0x08:  // Raspberry 3 Model B
> +        DEBUG ((DEBUG_INFO, "Using RPi3 Model B internal Device Tree\n"));
> +        FdtIndex = 0;
> +        break;
> +      case 0x0D:  // Raspberry 3 Model B+
> +        DEBUG ((DEBUG_INFO, "Using RPi3 Model B+ internal Device Tree\n"));
> +        FdtIndex = 1;
> +        break;
> +      default:
> +        DEBUG ((DEBUG_INFO, "Using default internal Device Tree\n"));
> +        FdtIndex = 0;
> +        break;
> +      }
> +    }
> +    ASSERT (FdtIndex < ARRAY_SIZE (mFdtGuid));
> +    Status = GetSectionFromAnyFv (mFdtGuid[FdtIndex], EFI_SECTION_RAW, 0,
>                 &FdtImage, &FdtSize);
>      if (Status == EFI_SUCCESS) {
>        if (fdt_check_header (FdtImage) != 0) {

Everything from here...

> @@ -419,27 +454,27 @@ FdtDxeInitialize (
>  
>    Status = SanitizePSCI ();
>    if (EFI_ERROR (Status)) {
> -    Print (L"Failed to sanitize PSCI (error %d)\n", Status);
> +    Print (L"Failed to sanitize PSCI: %r\n", Status);
>    }
>  
>    Status = CleanMemoryNodes ();
>    if (EFI_ERROR (Status)) {
> -    Print (L"Failed to clean memory nodes (error %d)\n", Status);
> +    Print (L"Failed to clean memory nodes: %r\n", Status);
>    }
>  
>    Status = CleanSimpleFramebuffer ();
>    if (EFI_ERROR (Status)) {
> -    Print (L"Failed to clean frame buffer (error %d)\n", Status);
> +    Print (L"Failed to clean frame buffer: %r\n", Status);
>    }
>  
>    Status = FixEthernetAliases ();
>    if (EFI_ERROR (Status)) {
> -    Print (L"Failed to fix ethernet aliases (error %d)\n", Status);
> +    Print (L"Failed to fix ethernet aliases: %r\n", Status);
>    }
>  
>    Status = UpdateMacAddress ();
>    if (EFI_ERROR (Status)) {
> -    Print (L"Failed to update MAC address (error %d)\n", Status);
> +    Print (L"Failed to update MAC address: %r\n", Status);
>    }
>  
>    if (Internal) {
> @@ -448,11 +483,11 @@ FdtDxeInitialize (
>       */
>      Status = UpdateBootArgs ();
>      if (EFI_ERROR (Status)) {
> -      Print (L"Failed to update boot arguments (error %d)\n", Status);
> +      Print (L"Failed to update boot arguments: %r\n", Status);
>      }
>    }
>  
> -  DEBUG ((DEBUG_INFO, "Installed FDT is at %p\n", mFdtImage));
> +  DEBUG ((DEBUG_INFO, "Installed Device Tree at address 0x%p\n", mFdtImage));
>    Status = gBS->InstallConfigurationTable (&gFdtTableGuid, mFdtImage);
>    ASSERT_EFI_ERROR (Status);
>

...to here is unrelated printout message changes.

So, I don't object to the debug printout changes as such, but they
obscure the functional changeset. Can you please break them out as a
separate patch?

Best Regards,

Leif

> diff --git a/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.inf b/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.inf
> index 5b0b1a09f374..c994a2b69d78 100644
> --- a/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.inf
> +++ b/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.inf
> @@ -35,7 +35,8 @@ [LibraryClasses]
>  
>  [Guids]
>    gFdtTableGuid
> -  gRaspberryPiFdtFileGuid
> +  gRaspberryPiFdtGuid1
> +  gRaspberryPiFdtGuid2
>  
>  [Protocols]
>    gRaspberryPiFirmwareProtocolGuid              ## CONSUMES
> diff --git a/Platform/RaspberryPi/RPi3/RPi3.dec b/Platform/RaspberryPi/RPi3/RPi3.dec
> index 22de439fde8f..d90ea8329818 100644
> --- a/Platform/RaspberryPi/RPi3/RPi3.dec
> +++ b/Platform/RaspberryPi/RPi3/RPi3.dec
> @@ -24,7 +24,8 @@ [Protocols]
>  
>  [Guids]
>    gRaspberryPiTokenSpaceGuid = {0xCD7CC258, 0x31DB, 0x11E6, {0x9F, 0xD3, 0x63, 0xB0, 0xB8, 0xEE, 0xD6, 0xB5}}
> -  gRaspberryPiFdtFileGuid = {0xDF5DA223, 0x1D27, 0x47C3, { 0x8D, 0x1B, 0x9A, 0x41, 0xB5, 0x5A, 0x18, 0xBC}}
> +  gRaspberryPiFdtGuid1 = {0xDF5DA223, 0x1D27, 0x47C3, { 0x8D, 0x1B, 0x9A, 0x41, 0xB5, 0x5A, 0x18, 0xBC}}
> +  gRaspberryPiFdtGuid2 = {0x3D523012, 0x73FE, 0x40E5, { 0x89, 0x2E, 0x1A, 0x4D, 0xF6, 0x0F, 0x3C, 0x0C}}
>    gRaspberryPiEventResetGuid = {0xCD7CC258, 0x31DB, 0x11E6, {0x9F, 0xD3, 0x63, 0xB4, 0xB4, 0xE4, 0xD4, 0xB4}}
>    gConfigDxeFormSetGuid = {0xCD7CC258, 0x31DB, 0x22E6, {0x9F, 0x22, 0x63, 0xB0, 0xB8, 0xEE, 0xD6, 0xB5}}
>  
> diff --git a/Platform/RaspberryPi/RPi3/RPi3.fdf b/Platform/RaspberryPi/RPi3/RPi3.fdf
> index c62d649834c7..83753d84badc 100644
> --- a/Platform/RaspberryPi/RPi3/RPi3.fdf
> +++ b/Platform/RaspberryPi/RPi3/RPi3.fdf
> @@ -300,11 +300,15 @@ [FV.FvMain]
>    INF Platform/RaspberryPi/$(PLATFORM_NAME)/Drivers/LogoDxe/LogoDxe.inf
>  
>    #
> -  # FDT (GUID matches gRaspberryPiFdtFileGuid in FdtDxe)
> +  # Device Tree support
> +  # GUIDs match gRaspberryPiFdtGuid# from FdtDxe.
>    #
>    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
> -- 
> 2.21.0.windows.1
>