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 <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.
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, Leifdiff --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.fdfindex 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 (#45727): https://edk2.groups.io/g/devel/message/45727 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] -=-=-=-=-=-=-=-=-=-=-=-
