From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f68.google.com (mail-wm1-f68.google.com [209.85.128.68]) by mx.groups.io with SMTP id smtpd.web10.7545.1588329917488406292 for ; Fri, 01 May 2020 03:45:18 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@akeo-ie.20150623.gappssmtp.com header.s=20150623 header.b=AvwYvCRW; spf=none, err=permanent DNS error (domain: akeo.ie, ip: 209.85.128.68, mailfrom: pete@akeo.ie) Received: by mail-wm1-f68.google.com with SMTP id r26so5933130wmh.0 for ; Fri, 01 May 2020 03:45:17 -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=LQUPtlCd7ESepen8jZ4loTqhuWDnbAucPWXItx7MJ/o=; b=AvwYvCRW1kkPu+1TpHaYFOvM2+Wbz+aVUCIu7jqjB/kb3yOG99PRH5PM+gMXXPOkR8 /jEwnJyJAVpuJVFh0wKz5IHQmOg1mQ4c+p7+8+1icMNAb1cRevuhqtCu78IoLFX2l0ZR IBMcytkZojEae0yWTxapaDmqhMMdXh5fR+kC/bqLnxG3lRkR5MLhdmQy6HpjaBksr6nm gu0VElEzyBmfI6maU+HDiumV+iq3S/ZsYG6AtFpf+yYncZTxATL20zBOVlsLdXcwJ+Sg NnVr/ItPZ76H6zWyf2TZm9OgTtUWCIfgI50MXT6qLwbfecrlH4AXVXRs/PBwdY1713yK C8Aw== 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=LQUPtlCd7ESepen8jZ4loTqhuWDnbAucPWXItx7MJ/o=; b=rCr/5StVqDdWvScx3ZTIrTOk89RPZkmtIzuY3SssxBgFLnhTmpDwoQZuvVdp1XICRt fSHuQ64V1yBOEaizKgkgXVAWIdtZ//IGqvCDss0EuXUxzDtjSuh4OIRtCF32G8T+bcJO Ha08eC0XJqh/pyceXY4jXJAwixtZuu/O8nfa3ZBwCGua+F5faFZtZNtkR5Qhd+3UeWZ/ k0jW+BOSaZfsVc5o+MhHK3Y5c8Mx79NS7kJhWuDBNQM33lEw5rvi8Wy3U/udVsHThFi7 NCjwSK1ITwA7MBYH8httc2uGLoqD5D1r0B5M1MjmP6AePQANVGRCcEeXLiwq3vkquOaR K3nA== X-Gm-Message-State: AGi0PubnTavL6PcN6tdlnBuSjXzU69jAOeZmFF6KCqSDi2IQDvmiPEYd gWUt6zjvOGkWvVc3SVS0udtY5A== X-Google-Smtp-Source: APiQypJ+lmjmq/+MspMIYEk9p0O+axjknClFPHrzMkw+Midre76cjPNEmJGF4QSr+8p0Agj05peyng== X-Received: by 2002:a1c:dc8b:: with SMTP id t133mr3452021wmg.117.1588329915813; Fri, 01 May 2020 03:45:15 -0700 (PDT) Return-Path: Received: from [10.0.0.122] ([84.203.75.87]) by smtp.googlemail.com with ESMTPSA id q143sm3613487wme.31.2020.05.01.03.45.14 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 01 May 2020 03:45:15 -0700 (PDT) Subject: Re: [edk2-platforms][PATCH 1/2] RPi: rip out FdtDxe logic to use internal DTB To: Andrei Warkentin , devel@edk2.groups.io Cc: ard.biesheuvel@arm.com, leif@nuviainc.com, philmd@redhat.com References: <20200430211617.120926-1-andrey.warkentin@gmail.com> <20200430211617.120926-2-andrey.warkentin@gmail.com> From: "Pete Batard" Message-ID: <5e16e385-a0be-1f0b-b6a0-24d729de135a@akeo.ie> Date: Fri, 1 May 2020 11:45:13 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 MIME-Version: 1.0 In-Reply-To: <20200430211617.120926-2-andrey.warkentin@gmail.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB Content-Transfer-Encoding: 7bit Two non-blocking minor remarks inline: On 2020.04.30 22:16, Andrei Warkentin wrote: > Initially, FdtDxe used an internal (embedded in UEFI) FDT, > because it was neither understood how to consume the > one loaded by the VideoCore firmware, nor understood just > how important it is to use the DTB provided by config.txt. > > Embedding the DT was a bad idea, because: > - Permanently stale > - No overlays > > Also, on devices like the Pi 4 you _have_ to have a DT > around for the start4 VPU firmware to pick up, otherwise > the board is left in an inconsistent state. So we're being > proscriptive now about DT use with config.txt, which means > this internal DT logic is deadc code. > > Further FdtDxe cleanups are possible and will be handled > separately, specifically: > - probably no need to use a separate allocation for patched DT (optimize memory used) > - suspicious use of EfiBootServicesData (I filed https://github.com/ARM-software/ebbr/issues/45 to sort out the real requirements) > > Testing: Booted Ubuntu 18.04 on Pi 2B (1.2). > > Signed-off-by: Andrei Warkentin > --- > Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c | 206 ++++---------------- > Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.inf | 4 - > Platform/RaspberryPi/RPi3/RPi3.fdf | 11 -- > Platform/RaspberryPi/RPi4/RPi4.fdf | 8 - > Platform/RaspberryPi/RaspberryPi.dec | 7 - > 5 files changed, 38 insertions(+), 198 deletions(-) > > diff --git a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c > index e7143f57..187b9566 100644 > --- a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c > +++ b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c > @@ -335,90 +335,6 @@ CleanSimpleFramebuffer ( > return EFI_SUCCESS; > } > > -#define MAX_CMDLINE_SIZE 512 > - > -STATIC > -EFI_STATUS > -UpdateBootArgs ( > - VOID > - ) > -{ > - INTN Node; > - INTN Retval; > - EFI_STATUS Status; > - CHAR8 *CommandLine; > - > - // > - // Locate the /chosen node > - // > - Node = fdt_path_offset (mFdtImage, "/chosen"); > - if (Node < 0) { > - DEBUG ((DEBUG_ERROR, "%a: failed to locate /chosen node\n", __FUNCTION__)); > - return EFI_NOT_FOUND; > - } > - > - // > - // If /chosen/bootargs already exists, we want to add a space character > - // before adding the firmware supplied arguments. However, the RpiFirmware > - // protocol expects a 32-bit aligned buffer. So let's allocate 4 bytes of > - // slack, and skip the first 3 when passing this buffer into libfdt. > - // > - CommandLine = AllocatePool (MAX_CMDLINE_SIZE) + 4; > - if (!CommandLine) { > - DEBUG ((DEBUG_ERROR, "%a: failed to allocate memory\n", __FUNCTION__)); > - return EFI_OUT_OF_RESOURCES; > - } > - > - // > - // Get the command line from the firmware > - // > - Status = mFwProtocol->GetCommandLine (MAX_CMDLINE_SIZE, CommandLine + 4); > - if (EFI_ERROR (Status)) { > - DEBUG ((DEBUG_ERROR, "%a: failed to retrieve command line\n", __FUNCTION__)); > - return Status; > - } > - > - if (AsciiStrLen (CommandLine + 4) == 0) { > - DEBUG ((DEBUG_INFO, "%a: empty command line received\n", __FUNCTION__)); > - return EFI_SUCCESS; > - } > - > - CommandLine[3] = ' '; > - > - Retval = fdt_appendprop_string (mFdtImage, Node, "bootargs", &CommandLine[3]); > - if (Retval != 0) { > - DEBUG ((DEBUG_ERROR, "%a: failed to set /chosen/bootargs property (%d)\n", > - __FUNCTION__, Retval)); > - } > - > - DEBUG_CODE_BEGIN (); > - CONST CHAR8 *Prop; > - INT32 Length; > - INT32 Index; > - > - Node = fdt_path_offset (mFdtImage, "/chosen"); > - ASSERT (Node >= 0); > - > - Prop = fdt_getprop (mFdtImage, Node, "bootargs", &Length); > - ASSERT (Prop != NULL); > - > - DEBUG ((DEBUG_INFO, "Command line set from firmware (length %d):\n'", Length)); > - > - for (Index = 0; Index < Length; Index++, Prop++) { > - if (*Prop == '\0') { > - continue; > - } > - DEBUG ((DEBUG_INFO, "%c", *Prop)); > - } > - > - DEBUG ((DEBUG_INFO, "'\n")); > - DEBUG_CODE_END (); > - > - FreePool (CommandLine - 4); > - return EFI_SUCCESS; > -} > - > - > /** > @param ImageHandle of the loaded driver > @param SystemTable Pointer to the System Table > @@ -435,13 +351,10 @@ FdtDxeInitialize ( > IN EFI_SYSTEM_TABLE *SystemTable > ) > { > + INT32 Retval; > EFI_STATUS Status; > - EFI_GUID *FdtGuid; > - VOID *FdtImage; > UINTN FdtSize; > - INT32 Retval; > - UINT32 BoardRevision; > - BOOLEAN Internal; > + VOID *FdtImage = NULL; I don't believe the NULL assignation is needed... > > if (PcdGet32 (PcdOptDeviceTree) == 0) { > DEBUG ((DEBUG_INFO, "Device Tree disabled per user configuration\n")); > @@ -450,77 +363,28 @@ FdtDxeInitialize ( > > Status = gBS->LocateProtocol (&gRaspberryPiFirmwareProtocolGuid, NULL, > (VOID**)&mFwProtocol); > - ASSERT_EFI_ERROR (Status); > + if (mFwProtocol == NULL) { > + /* > + * Impossible because of the depex... > + */ > + ASSERT_EFI_ERROR (Status); > + return EFI_NOT_FOUND; > + } > > - Internal = FALSE; > FdtImage = (VOID*)(UINTN)PcdGet32 (PcdFdtBaseAddress); ...since no code actually uses that variable before we assign it here. But I don't see a problem keeping it. > Retval = fdt_check_header (FdtImage); > - if (Retval == 0) { > - /* > - * Have FDT passed via config.txt. > - */ > - FdtSize = fdt_totalsize (FdtImage); > - 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 Device Tree found at address 0x%p (%a), " > - "looking up internal one...\n", FdtImage, fdt_strerror (Retval))); > + if (Retval != 0) { > /* > - * Query the board revision to differentiate between models. > + * Any one of: > + * - Invalid config.txt device_tree_address (not PcdFdtBaseAddress) > + * - Missing FDT for your Pi variant (if not overriding via device_tree=) > */ > - Status = mFwProtocol->GetModelRevision (&BoardRevision); > - if (EFI_ERROR (Status)) { > - DEBUG ((DEBUG_ERROR, "Failed to get board type: %r\n", Status)); > - DEBUG ((DEBUG_INFO, "Using default internal Device Tree\n")); > - FdtGuid = &gRaspberryPiDefaultFdtGuid; > - } else { > - // www.raspberrypi.org/documentation/hardware/raspberrypi/revision-codes/README.md > - switch ((BoardRevision >> 4) & 0xFF) { > - case 0x08: > - DEBUG ((DEBUG_INFO, "Using Raspberry Pi 3 Model B internal Device Tree\n")); > - FdtGuid = &gRaspberryPi3ModelBFdtGuid; > - break; > - case 0x0D: > - DEBUG ((DEBUG_INFO, "Using Raspberry Pi 3 Model B+ internal Device Tree\n")); > - FdtGuid = &gRaspberryPi3ModelBPlusFdtGuid; > - break; > - case 0x11: > - DEBUG ((DEBUG_INFO, "Using Raspberry Pi 4 Model B internal Device Tree\n")); > - FdtGuid = &gRaspberryPi4ModelBFdtGuid; > - break; > - default: > - DEBUG ((DEBUG_INFO, "Using default internal Device Tree\n")); > - FdtGuid = &gRaspberryPiDefaultFdtGuid; > - break; > - } > - } > - do { > - Status = GetSectionFromAnyFv (FdtGuid, EFI_SECTION_RAW, 0, &FdtImage, &FdtSize); > - if (Status == EFI_SUCCESS) { > - if (fdt_check_header (FdtImage) != 0) { > - Status = EFI_INCOMPATIBLE_VERSION; > - } > - } > - // No retry needed if we are successful or are dealing with the default Fdt. > - if ( (Status == EFI_SUCCESS) || > - (CompareGuid (FdtGuid, &gRaspberryPiDefaultFdtGuid)) ) > - break; > - // Otherwise, try one more time with the default Fdt. An example of this > - // is if we detected a non-default Fdt, that isn't included in the FDF. > - DEBUG ((DEBUG_INFO, "Internal Device Tree was not found for this platform, " > - "falling back to default...\n")); > - FdtGuid = &gRaspberryPiDefaultFdtGuid; > - } while (1); > + DEBUG ((DEBUG_ERROR, "No devicetree passed via config.txt\n")); > + return EFI_NOT_FOUND; > } > > - if (EFI_ERROR (Status)) { > - DEBUG ((DEBUG_ERROR, "Failed to locate Device Tree: %r\n", Status)); > - return Status; > - } > + FdtSize = fdt_totalsize (FdtImage); > + DEBUG ((DEBUG_INFO, "Devicetree passed via config.txt (0x%lx bytes)\n", FdtSize)); If we're going to modify that line, maybe we want to switch to using '%ld' instead of '0x%lx' as hex sizes aren't enormously helpful when dealing with user-handled files. But that's not something I want to respin this patch for, especially for debug output, so 0x%lx is fine too. > > /* > * Probably overkill. > @@ -529,12 +393,19 @@ FdtDxeInitialize ( > Status = gBS->AllocatePages (AllocateAnyPages, EfiBootServicesData, > EFI_SIZE_TO_PAGES (FdtSize), (EFI_PHYSICAL_ADDRESS*)&mFdtImage); > if (EFI_ERROR (Status)) { > - DEBUG ((DEBUG_ERROR, "Failed to allocate Device Tree: %r\n", Status)); > - return Status; > + DEBUG ((DEBUG_ERROR, "Failed to allocate devicetree: %r\n", Status)); > + goto out; > } > > Retval = fdt_open_into (FdtImage, mFdtImage, FdtSize); > - ASSERT (Retval == 0); > + if (Retval != 0) { > + DEBUG ((DEBUG_ERROR, "fdt_open_into failed: %d\n", Retval)); > + goto out; > + } > + > + /* > + * These are all best-effort. > + */ > > Status = SanitizePSCI (); > if (EFI_ERROR (Status)) { > @@ -566,19 +437,18 @@ FdtDxeInitialize ( > Print (L"Failed to update USB compatible properties: %r\n", Status); > } > > - if (Internal) { > - /* > - * A GPU-provided DTB already has the full command line. > - */ > - Status = UpdateBootArgs (); > - if (EFI_ERROR (Status)) { > - Print (L"Failed to update boot arguments: %r\n", Status); > - } > - } > - > - DEBUG ((DEBUG_INFO, "Installed Device Tree at address 0x%p\n", mFdtImage)); > + DEBUG ((DEBUG_INFO, "Installed devicetree at address %p\n", mFdtImage)); > Status = gBS->InstallConfigurationTable (&gFdtTableGuid, mFdtImage); > - ASSERT_EFI_ERROR (Status); > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "Couldn't register devicetree: %r\n", Status)); > + goto out; > + } > > +out: > + if (EFI_ERROR(Status)) { > + if (mFdtImage != NULL) { > + gBS->FreePages ((EFI_PHYSICAL_ADDRESS) mFdtImage, EFI_SIZE_TO_PAGES (FdtSize)); > + } > + } > return Status; > } > diff --git a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.inf b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.inf > index fc37353f..49ba5ba1 100644 > --- a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.inf > +++ b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.inf > @@ -35,10 +35,6 @@ > > [Guids] > gFdtTableGuid > - gRaspberryPi3ModelBFdtGuid > - gRaspberryPi3ModelBPlusFdtGuid > - gRaspberryPi4ModelBFdtGuid > - gRaspberryPiDefaultFdtGuid > > [Protocols] > gRaspberryPiFirmwareProtocolGuid ## CONSUMES > diff --git a/Platform/RaspberryPi/RPi3/RPi3.fdf b/Platform/RaspberryPi/RPi3/RPi3.fdf > index daedc443..e854cd21 100644 > --- a/Platform/RaspberryPi/RPi3/RPi3.fdf > +++ b/Platform/RaspberryPi/RPi3/RPi3.fdf > @@ -303,17 +303,6 @@ READ_LOCK_STATUS = TRUE > # > INF Platform/RaspberryPi/Drivers/LogoDxe/LogoDxe.inf > > - # > - # Device Tree support (used by FdtDxe) > - # GUIDs should match gRaspberryPi#####FdtGuid's from the .dec > - # > - 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 > ERASE_POLARITY = 1 > diff --git a/Platform/RaspberryPi/RPi4/RPi4.fdf b/Platform/RaspberryPi/RPi4/RPi4.fdf > index c3e9cfc4..b1f7aa23 100644 > --- a/Platform/RaspberryPi/RPi4/RPi4.fdf > +++ b/Platform/RaspberryPi/RPi4/RPi4.fdf > @@ -307,14 +307,6 @@ READ_LOCK_STATUS = TRUE > # > INF Platform/RaspberryPi/Drivers/LogoDxe/LogoDxe.inf > > - # > - # Device Tree support (used by FdtDxe) > - # GUIDs should match gRaspberryPi#####FdtGuid's from the .dec > - # > - FILE FREEFORM = 80AB6833-CAE4-4CEE-B59D-EB2039B05551 { > - SECTION RAW = Platform/RaspberryPi/$(PLATFORM_NAME)/DeviceTree/bcm2711-rpi-4-b.dtb > - } > - > [FV.FVMAIN_COMPACT] > FvAlignment = 16 > ERASE_POLARITY = 1 > diff --git a/Platform/RaspberryPi/RaspberryPi.dec b/Platform/RaspberryPi/RaspberryPi.dec > index b66322be..66ef6186 100644 > --- a/Platform/RaspberryPi/RaspberryPi.dec > +++ b/Platform/RaspberryPi/RaspberryPi.dec > @@ -26,13 +26,6 @@ > gRaspberryPiTokenSpaceGuid = {0xCD7CC258, 0x31DB, 0x11E6, {0x9F, 0xD3, 0x63, 0xB0, 0xB8, 0xEE, 0xD6, 0xB5}} > gRaspberryPiEventResetGuid = {0xCD7CC258, 0x31DB, 0x11E6, {0x9F, 0xD3, 0x63, 0xB4, 0xB4, 0xE4, 0xD4, 0xB4}} > gConfigDxeFormSetGuid = {0xCD7CC258, 0x31DB, 0x22E6, {0x9F, 0x22, 0x63, 0xB0, 0xB8, 0xEE, 0xD6, 0xB5}} > - # GUIDs used by FdtDxe to serve a Device Tree at runtime. Not all of these need to apply > - # to the current platform or match an actual FDF binary, but they need to be defined. > - gRaspberryPi3ModelBFdtGuid = { 0xDF5DA223, 0x1D27, 0x47C3, { 0x8D, 0x1B, 0x9A, 0x41, 0xB5, 0x5A, 0x18, 0xBC } } > - gRaspberryPi3ModelBPlusFdtGuid = { 0x3D523012, 0x73FE, 0x40E5, { 0x89, 0x2E, 0x1A, 0x4D, 0xF6, 0x0F, 0x3C, 0x0C } } > - gRaspberryPi4ModelBFdtGuid = { 0x80AB6833, 0xCAE4, 0x4CEE, { 0xB5, 0x9D, 0xEB, 0x20, 0x39, 0xB0, 0x55, 0x51 } } > - # Default Fdt to serve if the hardware model can't be detected. Should match one of the above. > - gRaspberryPiDefaultFdtGuid = {0xDF5DA223, 0x1D27, 0x47C3, { 0x8D, 0x1B, 0x9A, 0x41, 0xB5, 0x5A, 0x18, 0xBC}} > > [PcdsFixedAtBuild.common] > # > Reviewed-by: Pete Batard Tested-on: Pi 3B, Pi 3B+, Pi 4B, with and without a Device Tree provided