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

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

Hi Leif,

On 2019.08.15 13:13, Leif Lindholm wrote:
> 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.

I thought about that too, but we still have plenty of unused payload to 
go by, which is why I've been adding stuff like the EBC driver, and now 
this, without worrying too much about the extra space taken.

For one, in the current 2 MB firmware image, we're still barely 
scratching 1.2 MB of effective data (for RELEASE. For DEBUG that's ~1.5 
MB), so we have plenty of space to add additional Device Trees, logos, 
and similar non critical data. As a matter of fact, I was almost tempted 
to have this patchset also include the Raspberry Pi 4 Device Tree, so 
that we would just have all of the DT's we might ever be interested in 
available at runtime, regardless of whether they are applicable or not 
(as this would simplify FdtDxe reuse a well GUID handling).

Also, I would expect the build process to complain loudly if we go over 
capacity.

Finally, I'm pretty sure we could relatively easily switch to a 4 MB or 
8 MB firmware image, if we start to run out of space with the current 2 
MB, since, from what I am aware of, the SoC is designed to be able to 
boot large Linux kernels directly and doesn't enforce a hardcoded limit 
on our FV size.

In other words, even if we start to regularly add a tens of KB here and 
there, it's going to take some time before we have to start to worry 
about going over capacity...

>> ---
>>   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..

I mentioned in the cover letter that changes were also added to 
harmonize the debug output to avoid acronyms and provide human readable 
error codes. But you're right, at the very least, these changes should 
have been mentioned in the commit message for actual patch itself, and 
are probably better off on their own.

I'll break them out as a separate patch as requested.

Regards,

/Pete

> 
>> +    /*
>> +     * 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
>>