From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it0-x22f.google.com (mail-it0-x22f.google.com [IPv6:2607:f8b0:4001:c0b::22f]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 6FCB221DFA7BA for ; Tue, 28 Mar 2017 10:30:24 -0700 (PDT) Received: by mail-it0-x22f.google.com with SMTP id e75so63821561itd.1 for ; Tue, 28 Mar 2017 10:30:24 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=i4FjYOpDc8RAXT5HQj9Dg8SzUGCnZRfa9FmE4iktFPs=; b=MSlT3GiKJ6kZVJ0d5YAiKV6C4aeTB+sg0qJSXN8BUoweXYtiu4zga2fsUY2zxaXiWg jy4aS5TC6kN/AF9D1nMCSk16/5wybSysz/XWsVBERa/l8acgJpFKQ88WkfvLbcHON8ol OdX6xPR4TIz+0XZO9Q5zuvK5Fl+dhUkOBODJ4= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=i4FjYOpDc8RAXT5HQj9Dg8SzUGCnZRfa9FmE4iktFPs=; b=iVx8ih+jSnUDS5N0SRt6hVA/MjfTUGvf8uXhKqkM8/E7PmVkYlhz2dP+Uy5uIux3Ox dEteLBp1VJFWUkd0jbLwUFFlpLqnoio41G2tk1i4HasqdMwWf7HTlgljbpOprjterO/F PcScmeBm3dIggYd09lU0POqbfBDxVzsLJOaw725utM6/b1chF3qgfvjJgVw9nFUvizIn M/NWKQdQ8ByFPaDBF0EQ/ZcybZ2SRCMtcX83bNwbArD+Qf7oVjq/f0CG3cReoewAv4rD TYxFNIIb9qEewt0mid9bdoIROUi3KWvjWEUIVg10pw3bU5xeLKE8tZ++pwgF2C410/Dm cRxQ== X-Gm-Message-State: AFeK/H31ti04QhM+D6/sc2Fq2VA6kqJAYDYJhGNVXBf/bzjRVez/7FIgHRXAdItELj1zrBKcbOsg/NaNkh76lcXB X-Received: by 10.36.137.4 with SMTP id s4mr15598337itd.63.1490722222880; Tue, 28 Mar 2017 10:30:22 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.10.27 with HTTP; Tue, 28 Mar 2017 10:30:22 -0700 (PDT) In-Reply-To: <86b65788-5506-3e87-807a-f3c75859cde6@redhat.com> References: <20170328161500.2737-1-ard.biesheuvel@linaro.org> <86b65788-5506-3e87-807a-f3c75859cde6@redhat.com> From: Ard Biesheuvel Date: Tue, 28 Mar 2017 18:30:22 +0100 Message-ID: To: Laszlo Ersek Cc: "edk2-devel@lists.01.org" , Marcin Wojtas , Leif Lindholm Subject: Re: [PATCH v2] EmbeddedPkg: add DT platform driver to select between DT and ACPI X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 28 Mar 2017 17:30:24 -0000 Content-Type: text/plain; charset=UTF-8 On 28 March 2017 at 18:26, Laszlo Ersek wrote: > On 03/28/17 18:16, Ard Biesheuvel wrote: >> (use correct address for Marcin -- please take into account when replying) >> >> On 28 March 2017 at 17:15, Ard Biesheuvel wrote: >>> As a follow up to the changes proposed by Laszlo to make ACPI and DT >>> mutually exclusive on ArmVirtQemu, this patch proposed a DT platform > > new typo relative to v1: s/proposed/proposes/ > >>> DXE driver that either installs the NULL protocol PlatformHasAcpiGuid, >>> or installs the FV embedded DTB binary as a configuration table under >>> the appropriate GUID, depending on a preference setting recorded as >>> a UEFI variable, and configurable via a HII screen. >>> >>> The DTB binary can be embedded in the firmware image by adding the >>> following to the platform .fdf file: >>> >>> FILE FREEFORM = 25462CDA-221F-47DF-AC1D-259CFAA4E326 { >>> SECTION RAW = SomePkg/path/to/foo.dtb >>> } >>> >>> Contributed-under: TianoCore Contribution Agreement 1.0 >>> Signed-off-by: Ard Biesheuvel >>> --- >>> v2: fixup various errors and rough edges pointed out by Laszlo >>> >>> EmbeddedPkg/EmbeddedPkg.dec | 6 + >>> EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf | 65 ++++++ >>> EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.h | 31 +++ >>> EmbeddedPkg/Include/Guid/DtPlatformDefaultDtbFile.h | 23 +++ >>> EmbeddedPkg/Include/Guid/DtPlatformFormSet.h | 23 +++ >>> EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformHii.vfr | 51 +++++ >>> EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c | 216 ++++++++++++++++++++ >>> EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformHii.uni | 27 +++ >>> 8 files changed, 442 insertions(+) >>> >>> diff --git a/EmbeddedPkg/EmbeddedPkg.dec b/EmbeddedPkg/EmbeddedPkg.dec >>> index 29736fb4a71d..871fc5ff4016 100644 >>> --- a/EmbeddedPkg/EmbeddedPkg.dec >>> +++ b/EmbeddedPkg/EmbeddedPkg.dec >>> @@ -62,6 +62,12 @@ [Guids.common] >>> ## Include/Guid/PlatformHasDeviceTree.h >>> gEdkiiPlatformHasDeviceTreeGuid = { 0x7ebb920d, 0x1aaf, 0x46d9, { 0xb2, 0xaf, 0x54, 0x1e, 0x1d, 0xce, 0x14, 0x8b } } >>> >>> + # HII form set GUID for DtPlatformDxe driver >>> + gDtPlatformFormSetGuid = { 0x2b7a240d, 0xd5ad, 0x4fd6, { 0xbe, 0x1c, 0xdf, 0xa4, 0x41, 0x5f, 0x55, 0x26 } } >>> + >>> + # File GUID for default DTB image embedded in the firmware volume >>> + gDtPlatformDefaultDtbFileGuid = { 0x25462cda, 0x221f, 0x47df, { 0xac, 0x1d, 0x25, 0x9c, 0xfa, 0xa4, 0xe3, 0x26 } } >>> + >>> [Protocols.common] >>> gHardwareInterruptProtocolGuid = { 0x2890B3EA, 0x053D, 0x1643, { 0xAD, 0x0C, 0xD6, 0x48, 0x08, 0xDA, 0x3F, 0xF1 } } >>> gEfiDebugSupportPeriodicCallbackProtocolGuid = { 0x9546e07c, 0x2cbb, 0x4c88, { 0x98, 0x6c, 0xcd, 0x34, 0x10, 0x86, 0xf0, 0x44 } } >>> diff --git a/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf >>> new file mode 100644 >>> index 000000000000..aa1d3abb4012 >>> --- /dev/null >>> +++ b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf >>> @@ -0,0 +1,65 @@ >>> +## @file >>> +# >>> +# Copyright (c) 2017, Linaro, Ltd. All rights reserved.
>>> +# >>> +# This program and the accompanying materials are licensed and made >>> +# available under the terms and conditions of the BSD License which >>> +# accompanies this distribution. The full text of the license may be >>> +# found at http://opensource.org/licenses/bsd-license.php >>> +# >>> +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, >>> +# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR >>> +# IMPLIED. >>> +# >>> +## >>> + >>> +[Defines] >>> + INF_VERSION = 0x00010019 >>> + BASE_NAME = DtPlatformDxe >>> + FILE_GUID = FC097B3C-2EBD-4A75-A3DA-121DCAB365CC >>> + MODULE_TYPE = DXE_DRIVER >>> + VERSION_STRING = 1.0 >>> + ENTRY_POINT = DtPlatformDxeEntryPoint >>> + >>> +# >>> +# The following information is for reference only and not required by the build tools. >>> +# >>> +# VALID_ARCHITECTURES = IA32 X64 ARM AARCH64 >>> +# >>> + >>> +[Sources] >>> + DtPlatformDxe.c >>> + DtPlatformHii.vfr >>> + DtPlatformHii.uni >>> + >>> +[Packages] >>> + EmbeddedPkg/EmbeddedPkg.dec >>> + MdePkg/MdePkg.dec >>> + MdeModulePkg/MdeModulePkg.dec >>> + >>> +[LibraryClasses] >>> + BaseLib >>> + DebugLib >>> + UefiLib >>> + UefiDriverEntryPoint >>> + UefiBootServicesTableLib >>> + UefiRuntimeServicesTableLib >>> + UefiHiiServicesLib >>> + MemoryAllocationLib >>> + HiiLib >>> + PcdLib >>> + DxeServicesLib >>> + >>> +[Guids] >>> + gDtPlatformFormSetGuid >>> + gEdkiiPlatformHasAcpiGuid >>> + gDtPlatformDefaultDtbFileGuid >>> + gFdtTableGuid >>> + >>> +[Protocols] >>> + gEfiHiiConfigAccessProtocolGuid ## PRODUCES > > I think this should have been dropped as well -- you can remove it when > you commit the patch, if the removal works. (I think it should.) > >>> + >>> +[Depex] >>> + gEfiHiiDatabaseProtocolGuid AND > > IMO this should have been dropped too. You are using UefiHiiServicesLib > (only one instance available: > "MdeModulePkg/Library/UefiHiiServicesLib/UefiHiiServicesLib.inf"), and > that instance gives you a dependency on gEfiHiiDatabaseProtocolGuid > automatically. > > You never use the protocol manually, just through the following chain: > > * HiiAddPackages() is from HiiLib (only one instance: > MdeModulePkg/Library/UefiHiiLib/UefiHiiLib.inf), which relies on > "gHiiDatabase" > > * "gHiiDatabase" is defined / initialized in UefiHiiServicesLib > (UefiHiiServicesLibConstructor()), dependent on > gEfiHiiDatabaseProtocolGuid, and the library instance also spells out > the DEPEX for that. > > So I think, if you can drop this from the depex while keeping things > functional, you should. No need to resubmit just because of this, again. > > > Some more things which look useless for this driver: > - PcdLib (class in INF, hdr inclusion in source) > - ... OK I guess that's all. > > (In my v1 review I wrote "Basically, please make this INF as minimal as > possible" -- apologies for not spelling it out in more detail.) > >>> + gEfiVariableArchProtocolGuid AND >>> + gEfiVariableWriteArchProtocolGuid >>> diff --git a/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.h b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.h >>> new file mode 100644 >>> index 000000000000..2369367277b0 >>> --- /dev/null >>> +++ b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.h >>> @@ -0,0 +1,31 @@ >>> +/** @file >>> +* >>> +* Copyright (c) 2017, Linaro Limited. All rights reserved. >>> +* >>> +* This program and the accompanying materials >>> +* are licensed and made available under the terms and conditions of the BSD License >>> +* which accompanies this distribution. The full text of the license may be found at >>> +* http://opensource.org/licenses/bsd-license.php >>> +* >>> +* THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, >>> +* WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. >>> +* >>> +**/ >>> + >>> +#ifndef __DT_PLATFORM_DXE_H__ >>> +#define __DT_PLATFORM_DXE_H__ >>> + >>> +#include >>> +#include >>> + >>> +#define DT_ACPI_SELECT_DT 0x0 >>> +#define DT_ACPI_SELECT_ACPI 0x1 >>> + >>> +#define DT_ACPI_VARIABLE_NAME L"DtAcpiPref" >>> + >>> +typedef struct { >>> + UINT8 Pref; >>> + UINT8 Reserved[3]; >>> +} DT_ACPI_VARSTORE_DATA; >>> + >>> +#endif >>> diff --git a/EmbeddedPkg/Include/Guid/DtPlatformDefaultDtbFile.h b/EmbeddedPkg/Include/Guid/DtPlatformDefaultDtbFile.h >>> new file mode 100644 >>> index 000000000000..c44b4d9522fe >>> --- /dev/null >>> +++ b/EmbeddedPkg/Include/Guid/DtPlatformDefaultDtbFile.h >>> @@ -0,0 +1,23 @@ >>> +/** @file >>> +* >>> +* Copyright (c) 2017, Linaro Limited. All rights reserved. >>> +* >>> +* This program and the accompanying materials >>> +* are licensed and made available under the terms and conditions of the BSD License >>> +* which accompanies this distribution. The full text of the license may be found at >>> +* http://opensource.org/licenses/bsd-license.php >>> +* >>> +* THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, >>> +* WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. >>> +* >>> +**/ >>> + >>> +#ifndef __DT_PLATFORM_DEFAULT_DTB_FILE_H__ >>> +#define __DT_PLATFORM_DEFAULT_DTB_FILE_H__ >>> + >>> +#define DT_PLATFORM_DEFAULT_DTB_FILE_GUID \ >>> + { 0x25462cda, 0x221f, 0x47df, { 0xac, 0x1d, 0x25, 0x9c, 0xfa, 0xa4, 0xe3, 0x26 } } >>> + >>> +extern EFI_GUID gDtPlatformDefaultDtbFileGuid; >>> + >>> +#endif >>> diff --git a/EmbeddedPkg/Include/Guid/DtPlatformFormSet.h b/EmbeddedPkg/Include/Guid/DtPlatformFormSet.h >>> new file mode 100644 >>> index 000000000000..71e3e7ebf7f3 >>> --- /dev/null >>> +++ b/EmbeddedPkg/Include/Guid/DtPlatformFormSet.h >>> @@ -0,0 +1,23 @@ >>> +/** @file >>> +* >>> +* Copyright (c) 2017, Linaro Limited. All rights reserved. >>> +* >>> +* This program and the accompanying materials >>> +* are licensed and made available under the terms and conditions of the BSD License >>> +* which accompanies this distribution. The full text of the license may be found at >>> +* http://opensource.org/licenses/bsd-license.php >>> +* >>> +* THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, >>> +* WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. >>> +* >>> +**/ >>> + >>> +#ifndef __DT_PLATFORM_FORMSET_H__ >>> +#define __DT_PLATFORM_FORMSET_H__ >>> + >>> +#define DT_PLATFORM_FORMSET_GUID \ >>> + { 0x2b7a240d, 0xd5ad, 0x4fd6, { 0xbe, 0x1c, 0xdf, 0xa4, 0x41, 0x5f, 0x55, 0x26 } } >>> + >>> +extern EFI_GUID gDtPlatformFormSetGuid; >>> + >>> +#endif >>> diff --git a/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformHii.vfr b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformHii.vfr >>> new file mode 100644 >>> index 000000000000..3516746c4d84 >>> --- /dev/null >>> +++ b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformHii.vfr >>> @@ -0,0 +1,51 @@ >>> +/** @file >>> +* >>> +* Copyright (c) 2017, Linaro, Ltd. All rights reserved. >>> +* >>> +* This program and the accompanying materials >>> +* are licensed and made available under the terms and conditions of the BSD License >>> +* which accompanies this distribution. The full text of the license may be found at >>> +* http://opensource.org/licenses/bsd-license.php >>> +* >>> +* THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, >>> +* WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. >>> +* >>> +**/ >>> + >>> +#include "DtPlatformDxe.h" >>> + >>> +// >>> +// EFI Variable attributes >>> +// >>> +#define EFI_VARIABLE_NON_VOLATILE 0x00000001 >>> +#define EFI_VARIABLE_BOOTSERVICE_ACCESS 0x00000002 >>> +#define EFI_VARIABLE_RUNTIME_ACCESS 0x00000004 >>> +#define EFI_VARIABLE_READ_ONLY 0x00000008 >>> + >>> +formset >>> + guid = DT_PLATFORM_FORMSET_GUID, >>> + title = STRING_TOKEN(STR_FORM_SET_TITLE), >>> + help = STRING_TOKEN(STR_FORM_SET_TITLE_HELP), >>> + classguid = EFI_HII_PLATFORM_SETUP_FORMSET_GUID, >>> + >>> + efivarstore DT_ACPI_VARSTORE_DATA, >>> + attribute = EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_NON_VOLATILE, // EFI variable attributes >>> + name = DtAcpiPref, >>> + guid = DT_PLATFORM_FORMSET_GUID; >>> + >>> + form formid = 0x1000, >>> + title = STRING_TOKEN(STR_MAIN_FORM_TITLE); >>> + >>> + oneof varid = DtAcpiPref.Pref, >>> + prompt = STRING_TOKEN(STR_DT_ACPI_SELECT_PROMPT), >>> + help = STRING_TOKEN(STR_DT_ACPI_SELECT_HELP), >>> + flags = NUMERIC_SIZE_1 | INTERACTIVE | RESET_REQUIRED, >>> + option text = STRING_TOKEN(STR_DT_ACPI_SELECT_DT), value = DT_ACPI_SELECT_DT, flags = DEFAULT; >>> + option text = STRING_TOKEN(STR_DT_ACPI_SELECT_ACPI), value = DT_ACPI_SELECT_ACPI, flags = 0; >>> + endoneof; >>> + >>> + subtitle text = STRING_TOKEN(STR_NULL_STRING); >>> + >>> + endform; >>> + >>> +endformset; >>> diff --git a/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c >>> new file mode 100644 >>> index 000000000000..f714551213c2 >>> --- /dev/null >>> +++ b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c >>> @@ -0,0 +1,216 @@ >>> +/** @file >>> +* >>> +* Copyright (c) 2017, Linaro, Ltd. All rights reserved. >>> +* >>> +* This program and the accompanying materials >>> +* are licensed and made available under the terms and conditions of the BSD License >>> +* which accompanies this distribution. The full text of the license may be found at >>> +* http://opensource.org/licenses/bsd-license.php >>> +* >>> +* THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, >>> +* WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. >>> +* >>> +**/ >>> + >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> + >>> +#include "DtPlatformDxe.h" >>> + >>> +extern UINT8 DtPlatformHiiBin[]; >>> +extern UINT8 DtPlatformDxeStrings[]; >>> + >>> +typedef struct { >>> + VENDOR_DEVICE_PATH VendorDevicePath; >>> + EFI_DEVICE_PATH_PROTOCOL End; >>> +} HII_VENDOR_DEVICE_PATH; >>> + >>> +STATIC HII_VENDOR_DEVICE_PATH mDtPlatformDxeVendorDevicePath = { >>> + { >>> + { >>> + HARDWARE_DEVICE_PATH, >>> + HW_VENDOR_DP, >>> + { >>> + (UINT8) (sizeof (VENDOR_DEVICE_PATH)), >>> + (UINT8) ((sizeof (VENDOR_DEVICE_PATH)) >> 8) >>> + } >>> + }, >>> + DT_PLATFORM_FORMSET_GUID >>> + }, >>> + { >>> + END_DEVICE_PATH_TYPE, >>> + END_ENTIRE_DEVICE_PATH_SUBTYPE, >>> + { >>> + (UINT8) (END_DEVICE_PATH_LENGTH), >>> + (UINT8) ((END_DEVICE_PATH_LENGTH) >> 8) >>> + } >>> + } >>> +}; >>> + >>> +STATIC >>> +EFI_STATUS >>> +InstallHiiPages ( >>> + VOID >>> + ) >>> +{ >>> + EFI_STATUS Status; >>> + EFI_HII_HANDLE HiiHandle; >>> + EFI_HANDLE DriverHandle; >>> + >>> + DriverHandle = NULL; >>> + Status = gBS->InstallMultipleProtocolInterfaces (&DriverHandle, >>> + &gEfiDevicePathProtocolGuid, >>> + &mDtPlatformDxeVendorDevicePath, >>> + NULL); >>> + if (EFI_ERROR (Status)) { >>> + return Status; >>> + } >>> + >>> + HiiHandle = HiiAddPackages (&gDtPlatformFormSetGuid, >>> + DriverHandle, >>> + DtPlatformDxeStrings, >>> + DtPlatformHiiBin, >>> + NULL); >>> + >>> + if (HiiHandle == NULL) { >>> + gBS->UninstallMultipleProtocolInterfaces (DriverHandle, >>> + &gEfiDevicePathProtocolGuid, >>> + &mDtPlatformDxeVendorDevicePath, >>> + NULL); >>> + return EFI_OUT_OF_RESOURCES; >>> + } >>> + return EFI_SUCCESS; >>> +} >>> + >>> +/** >>> + The entry point for DtPlatformDxe driver. >>> + >>> + @param[in] ImageHandle The image handle of the driver. >>> + @param[in] SystemTable The system table. >>> + >>> + @retval EFI_ALREADY_STARTED The driver already exists in system. >>> + @retval EFI_OUT_OF_RESOURCES Fail to execute entry point due to lack of >>> + resources. >>> + @retval EFI_SUCCES All the related protocols are installed on >>> + the driver. >>> + >>> +**/ >>> +EFI_STATUS >>> +EFIAPI >>> +DtPlatformDxeEntryPoint ( >>> + IN EFI_HANDLE ImageHandle, >>> + IN EFI_SYSTEM_TABLE *SystemTable >>> + ) >>> +{ >>> + EFI_STATUS Status; >>> + DT_ACPI_VARSTORE_DATA DtAcpiPref; >>> + UINTN BufferSize; >>> + VOID *Dtb; >>> + UINTN DtbSize; >>> + VOID *DtbCopy; >>> + >>> + // >>> + // Check whether a DTB blob is included in the firmware image. >>> + // >>> + Dtb = NULL; >>> + Status = GetSectionFromAnyFv (&gDtPlatformDefaultDtbFileGuid, >>> + EFI_SECTION_RAW, 0, &Dtb, &DtbSize); >>> + if (EFI_ERROR (Status)) { >>> + DEBUG ((DEBUG_WARN, "%a: no DTB blob found, defaulting to ACPI\n", >>> + __FUNCTION__)); >>> + DtAcpiPref.Pref = DT_ACPI_SELECT_ACPI; >>> + } else { >>> + // >>> + // Get the current DT/ACPI preference from the DtAcpiPref variable. >>> + // >>> + BufferSize = sizeof (DtAcpiPref); >>> + Status = gRT->GetVariable(DT_ACPI_VARIABLE_NAME, &gDtPlatformFormSetGuid, >>> + NULL, &BufferSize, &DtAcpiPref); >>> + if (EFI_ERROR (Status)) { >>> + DEBUG ((DEBUG_WARN, "%a: no DT/ACPI preference found, defaulting to DT\n", >>> + __FUNCTION__)); >>> + DtAcpiPref.Pref = DT_ACPI_SELECT_DT; >>> + } >>> + } >>> + >>> + if (!EFI_ERROR (Status) && >>> + DtAcpiPref.Pref != DT_ACPI_SELECT_ACPI && >>> + DtAcpiPref.Pref != DT_ACPI_SELECT_DT) { >>> + DEBUG ((DEBUG_WARN, "%a: invalid value for DtAcpiPref, defaulting to DT\n", >>> + __FUNCTION__)); > > Can you replace this instance of "... DtAcpiPref ..." with "... %a ..." > and DT_ACPI_VARIABLE_NAME? No need to resubmit. > That should be %s, I suppose? > With those warts removed: > > Reviewed-by: Laszlo Ersek > Thanks!