* [edk2-platforms: PATCH 0/1] Platforms/RPi3: Add multiple embedded Device Tree selection @ 2019-08-12 13:06 Pete Batard 2019-08-12 13:06 ` [edk2-platforms: PATCH 1/1] " Pete Batard 0 siblings, 1 reply; 4+ messages in thread From: Pete Batard @ 2019-08-12 13:06 UTC (permalink / raw) To: devel; +Cc: ard.biesheuvel, leif.lindholm The purpose of this patch is to enable the automatic provision of a relevant Raspberry Pi 3 Device Tree at runtime, as there currently exists 2 models of Raspberry Pi 3 (model B and model B+) but we only embed the Device Tree for a single one (model B). Obviously, the provision of only a single Device Tree is inconvenient, as it currently forces users of the model B+ to locate and provide their own Device Tree in config.txt, which might not be a straightforward operation. It also adds to the number of extra files that must be copied to the boot media. With this patch, both Device Tree binaries are embedded in the firmware, with the relevant being served at runtime according to the detected hardware. Note that we tried to future-proof the modified code with regards to expected introduction of new Raspberry Pi platforms (such as the Pi 4). Therefore, while the switch in FdtDxe.c may seem like overkill for now (with cases that could be merged), it should make the reuse of that driver easier. We also tried to harmonize debug output if FtdDxe (avoid acronyms and provide human readable error codes). Pete Batard (1): Platforms/RPi3: Add multiple embedded Device Tree selection Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c | 54 ++++++++++++++++---- Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.inf | 3 +- Platform/RaspberryPi/RPi3/RPi3.dec | 3 +- Platform/RaspberryPi/RPi3/RPi3.fdf | 6 ++- 4 files changed, 52 insertions(+), 14 deletions(-) -- 2.21.0.windows.1 ^ permalink raw reply [flat|nested] 4+ messages in thread
* [edk2-platforms: PATCH 1/1] Platforms/RPi3: Add multiple embedded Device Tree selection 2019-08-12 13:06 [edk2-platforms: PATCH 0/1] Platforms/RPi3: Add multiple embedded Device Tree selection Pete Batard @ 2019-08-12 13:06 ` Pete Batard 2019-08-15 12:13 ` Leif Lindholm 0 siblings, 1 reply; 4+ messages in thread From: Pete Batard @ 2019-08-12 13:06 UTC (permalink / raw) To: devel; +Cc: ard.biesheuvel, leif.lindholm 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> --- 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); - 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)); Status = EFI_SUCCESS; } else { + /* + * Use one of the embedded FDT's. + */ Internal = TRUE; - 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))); + /* + * 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) { @@ -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); 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [edk2-platforms: PATCH 1/1] Platforms/RPi3: Add multiple embedded Device Tree selection 2019-08-12 13:06 ` [edk2-platforms: PATCH 1/1] " Pete Batard @ 2019-08-15 12:13 ` Leif Lindholm 2019-08-15 13:11 ` Pete Batard 0 siblings, 1 reply; 4+ messages in thread From: Leif Lindholm @ 2019-08-15 12:13 UTC (permalink / raw) To: Pete Batard; +Cc: devel, ard.biesheuvel 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 > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [edk2-platforms: PATCH 1/1] Platforms/RPi3: Add multiple embedded Device Tree selection 2019-08-15 12:13 ` Leif Lindholm @ 2019-08-15 13:11 ` Pete Batard 0 siblings, 0 replies; 4+ messages in thread From: Pete Batard @ 2019-08-15 13:11 UTC (permalink / raw) To: Leif Lindholm; +Cc: devel, ard.biesheuvel 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 >> ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-08-15 13:12 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-08-12 13:06 [edk2-platforms: PATCH 0/1] Platforms/RPi3: Add multiple embedded Device Tree selection Pete Batard 2019-08-12 13:06 ` [edk2-platforms: PATCH 1/1] " Pete Batard 2019-08-15 12:13 ` Leif Lindholm 2019-08-15 13:11 ` Pete Batard
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox