From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=tu3w0eQm; spf=pass (domain: linaro.org, ip: 209.85.221.65, mailfrom: leif.lindholm@linaro.org) Received: from mail-wr1-f65.google.com (mail-wr1-f65.google.com [209.85.221.65]) by groups.io with SMTP; Thu, 15 Aug 2019 05:13:57 -0700 Received: by mail-wr1-f65.google.com with SMTP id g17so2038070wrr.5 for ; Thu, 15 Aug 2019 05:13:57 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=DRPW0zCcO0kbm8pXJV3Xf+Yfr8CGl4x5AC3UTD41G+U=; b=tu3w0eQm50+QP7PUoBkHkOsYYq8o7IlhNLD/ETrsucrv9kgqPc93RKpyMjj+upy6e2 qYWIfPNQNSlwv5e6rCjyEoyxfTb3PVoxnKWwRjEYHdnUXRBagXkTaxMxXZeq1uTfp6xd GNjNlRG+U4EwDKSd6RxBWce+J7qAX8QWjYgUMgJRh0XvkjdPQAqTj2ZMEBShWbbmSBgQ 8DV9Nns1JKtwQSCMbZamCqac6SQ3o2VDlUuQwXXy0+YqFjr1OHcpuInMaN+HCbD2YLGT ytCdk7OEZ9JOxDZHKbcVGmZA3mGeXP/NS7SNuqI8vI66fr19CZQwmUNdjkJXo9Fs9eAx D0hQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=DRPW0zCcO0kbm8pXJV3Xf+Yfr8CGl4x5AC3UTD41G+U=; b=iKdrBtkrw60qjSrXcg74BVWA17s7/2VxOCcgUMIT+HA0+v/gznfdw5wX5VpPMaYwNi bc1VoArgUCSuGnPwv/3TqlaKsib8dKEBbaFi/lxHER7rYmbRD3QeQNQmB5/Z+md1kk8b E4ZsxdbqtgzkmrCv2u4pk/vu9G4iElpgADA6anEpq7Cf//kmDecfJjU/On5M4GUn5ixh KZT5QnZxCkclq/gKanTcurZwDkMTOCFjml9JfEGww63CrhzA/mOylpCB4tDjbHumfb1S +tXfHM/65DVZTQhDoH1XSCRQ5mtWTtCVxEHmVgSnRBiS3JHrjpz+pzFATfqKRU2jkHei SPiA== X-Gm-Message-State: APjAAAXS1Rg3lq/7gutuAjS/X9Ee/ZE5jiN6eBHugoTwsacO7sbOOXmu J+0IEvG6ekC8qkBSfdU4597OFw== X-Google-Smtp-Source: APXvYqzD2dB5esmRbrezI4Y7BGTQVmBNjJRcNBpRmTMWD+g8RW0UWM8ZBtMNsDUcrJa8rvoeTRLXvQ== X-Received: by 2002:adf:f04d:: with SMTP id t13mr5106404wro.133.1565871235860; Thu, 15 Aug 2019 05:13:55 -0700 (PDT) Return-Path: Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id a84sm1564578wmf.29.2019.08.15.05.13.54 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 15 Aug 2019 05:13:55 -0700 (PDT) Date: Thu, 15 Aug 2019 13:13:53 +0100 From: "Leif Lindholm" To: Pete Batard Cc: devel@edk2.groups.io, ard.biesheuvel@linaro.org Subject: Re: [edk2-platforms: PATCH 1/1] Platforms/RPi3: Add multiple embedded Device Tree selection Message-ID: <20190815121353.GL29255@bivouac.eciton.net> References: <20190812130634.7224-1-pete@akeo.ie> <20190812130634.7224-2-pete@akeo.ie> MIME-Version: 1.0 In-Reply-To: <20190812130634.7224-2-pete@akeo.ie> User-Agent: Mutt/1.10.1 (2018-07-13) Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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. > --- > 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.. > + /* > + * 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 >