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 <p...@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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-