From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-qk1-f194.google.com (mail-qk1-f194.google.com [209.85.222.194]) by mx.groups.io with SMTP id smtpd.web12.786.1588281389443071225 for ; Thu, 30 Apr 2020 14:16:29 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@gmail.com header.s=20161025 header.b=ErU72AWS; spf=pass (domain: gmail.com, ip: 209.85.222.194, mailfrom: andrey.warkentin@gmail.com) Received: by mail-qk1-f194.google.com with SMTP id q7so7393518qkf.3 for ; Thu, 30 Apr 2020 14:16:29 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:date:message-id:in-reply-to:references; bh=dV43Pd9WClWB2pJktDt7irSb8ZBL4ZtZz7p/uALTj84=; b=ErU72AWSV9WkEGOPHuLBd7NhMJ+SVa2vYEMcdfN7QoHtU8zZGF8zen/ZI3GxIyJ6Jp SbTY5cCKsklFDrEid1Xsme437EtzKsk8ljpOeytWUBQqJ4/Q2+KOWSEZYgUIqrA+vDlS l6LuhXdyqyVV6952jczywPt5ciy+rpa0/cXe8KZQsWSSYhrTsy0VBKY4h9AOkKRTB8dJ YkAB2RljTBoqMDt0WceTyy2/Ol36E+ho5fasE2t6CsVOfu+Ajc+RZzgcuHu2LfEzInVb pmH997j0tBYJyPR4OAhEC4u+imZPVD7e/tD+GjCQQfY6k8MstWRWM7RjTBPMWkcT71km qfRQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references; bh=dV43Pd9WClWB2pJktDt7irSb8ZBL4ZtZz7p/uALTj84=; b=HfeGMTTreacdcwFSFammog5Pz2PpJ9OqD36l/beViP+wPxXIEVPUd7Gzy7Tjo/aM82 60s0ApLvhsqT2uCy+/P/8MvQ8hzktJ1uxVW/q392BBBeMQ7Q5PfLmIe9GXp70v/R6ALr AZXJIUPzkGI4sDvGlA+FWaa9BCJ+HJUyT9M+xZLJzB5I5969wVSPzFP72chQQcMJ0BTq 8JGizkRrv4jvN+dF44S6suP1XA3fRBvcrac25gL+G3lcbP4aIZZFygMKSx28fefUa9F5 cvzc3a5eQXMGAZGtYN8OYWbAV4ae4mPjYDQgya0Nj580LMOfSklLU2m8OSlc4WjLU28d /ZiA== X-Gm-Message-State: AGi0PuZSOMBQAToMiNo8pk7cIYYVi5EUC82r9LhnJsIRzgx4HHjFOMn4 qTAdOOOecnzm6npu84s2GDTFqSLlPak= X-Google-Smtp-Source: APiQypL9ydAYFV7rw6o0MH7syINEUFItM/d9AQyeCJdmtKfXjWbFHCX+vKTboYnBaXYvrvEDtw5EwQ== X-Received: by 2002:a37:b587:: with SMTP id e129mr533948qkf.226.1588281388066; Thu, 30 Apr 2020 14:16:28 -0700 (PDT) Return-Path: Received: from localhost.localdomain (c-98-214-99-181.hsd1.il.comcast.net. [98.214.99.181]) by smtp.gmail.com with ESMTPSA id n136sm1090441qke.97.2020.04.30.14.16.27 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 30 Apr 2020 14:16:27 -0700 (PDT) From: "Andrei Warkentin" To: devel@edk2.groups.io Cc: ard.biesheuvel@arm.com, leif@nuviainc.com, pete@akeo.ie, philmd@redhat.com Subject: [edk2-platforms][PATCH 1/2] RPi: rip out FdtDxe logic to use internal DTB Date: Thu, 30 Apr 2020 14:16:16 -0700 Message-Id: <20200430211617.120926-2-andrey.warkentin@gmail.com> X-Mailer: git-send-email 2.17.1 In-Reply-To: <20200430211617.120926-1-andrey.warkentin@gmail.com> References: <20200430211617.120926-1-andrey.warkentin@gmail.com> 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; 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); 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)); /* * 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] # -- 2.17.1