From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=pass header.i=@akeo-ie.20150623.gappssmtp.com header.s=20150623 header.b=lgfLj9zZ; spf=none, err=permanent DNS error (domain: akeo.ie, ip: 209.85.128.67, mailfrom: pete@akeo.ie) Received: from mail-wm1-f67.google.com (mail-wm1-f67.google.com [209.85.128.67]) by groups.io with SMTP; Thu, 15 Aug 2019 06:12:04 -0700 Received: by mail-wm1-f67.google.com with SMTP id z23so1223133wmf.2 for ; Thu, 15 Aug 2019 06:12:03 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=akeo-ie.20150623.gappssmtp.com; s=20150623; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=BgkT6CLC2T91m1dVP0IRv64BEcbmi42dBVDGTfTNd/M=; b=lgfLj9zZ9lieXtpnN1sMpvP+J/f9ch5uxHLvnrEZnKqQ25LNyAOWsU/9Ao0f/DiUPd wUGsNkRJ3cPRVu/3VDdPypwhjdgbzhonAHG+Mvr8SEWaCeaSX1zMblHaflxhc0ZViLyT gQpAK32Bl0glgpdqLbBCvbMq4KEEupRCPsoPCrSn9kG3ofirSim+P4HVwwhVIvtGIeaJ wOxFeQxlR19NjY350w205tE0aLV8lqByq+KSOQBoogjWaZQ0lDh9csJBRumIZQFzK+GW 6lxfYmXyESXv/O8f0k6Exd3XZSCmyUJ9nQUGuadMYuxuPmX55LxBPQXwSMg1wqfPLnu2 JNyA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=BgkT6CLC2T91m1dVP0IRv64BEcbmi42dBVDGTfTNd/M=; b=K64zvJy+0Nglq7tGSaVQq7a3wsiUcGNZciGhG4ETucBlcAKZrhTDr1nXfHKVwVlJJ5 SF+AFQLrsUEPvmWDsu7CGsZsSqYtZCdNfqlGEn6Zy+xdzBjufnwfnGBn8nnjTYJ7Qzli sZYHX8R89hTBdShQkbRNO7/uIoifgpvM2qsy4oe9TIt8BEhlJMz1kyz3ChySkuTwScbU ANZ4BrKojJ8t25AvJf7FHOxhLBd3tavKnvyWWmUXsz6lxQ5z0Ri7wJZcG7a6Cn+OihsY tNOSKolZie1nl0EE9rItDNAYP7f4zRPFTnxomGIluXsFbEPq9G9ugsRfuGnekTWmFrDY eMYw== X-Gm-Message-State: APjAAAUtGtB5MH84VFICVh9zrzwOXapiuVK6mcc60npMFr5IsrHi2WhA fGkAPz/lnk6awJc8C/UnYS98ww== X-Google-Smtp-Source: APXvYqyFnHxS0YC2rV8gSZyxGq/SGERGqAEvpTEncnYOaK7wb7AAsnARCXn4Dm9RU0URjFFnW6xrLw== X-Received: by 2002:a1c:c90c:: with SMTP id f12mr2620136wmb.23.1565874722458; Thu, 15 Aug 2019 06:12:02 -0700 (PDT) Return-Path: Received: from [10.0.0.122] ([84.203.61.230]) by smtp.googlemail.com with ESMTPSA id f7sm4766683wrf.8.2019.08.15.06.11.58 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 15 Aug 2019 06:11:59 -0700 (PDT) Subject: Re: [edk2-platforms: PATCH 1/1] Platforms/RPi3: Add multiple embedded Device Tree selection To: Leif Lindholm Cc: devel@edk2.groups.io, ard.biesheuvel@linaro.org References: <20190812130634.7224-1-pete@akeo.ie> <20190812130634.7224-2-pete@akeo.ie> <20190815121353.GL29255@bivouac.eciton.net> From: "Pete Batard" Message-ID: Date: Thu, 15 Aug 2019 14:11:57 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101 Thunderbird/60.8.0 MIME-Version: 1.0 In-Reply-To: <20190815121353.GL29255@bivouac.eciton.net> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB Content-Transfer-Encoding: 7bit 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 > > 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 >> >> +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 >>