[edk2-devel] [edk2-platforms: PATCH 1/1] Platforms/RPi3: Add multiple embedded Device Tree selection
Leif Lindholm
leif.lindholm at linaro.org
Thu Aug 15 12:13:53 UTC 2019
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 at 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
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#45719): https://edk2.groups.io/g/devel/message/45719
Mute This Topic: https://groups.io/mt/32840411/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list