From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by mx.groups.io with SMTP id smtpd.web08.436.1618248438003712495 for ; Mon, 12 Apr 2021 10:27:18 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=RabNEGMr; spf=pass (domain: kernel.org, ip: 198.145.29.99, mailfrom: ardb@kernel.org) Received: by mail.kernel.org (Postfix) with ESMTPSA id 1AD3461289 for ; Mon, 12 Apr 2021 17:27:17 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1618248437; bh=YDJAvG6WdvGpLTUjkM275QVc4rK7e1E2A7Hfmx8ePTI=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=RabNEGMrq0xwiphnIXHCDMv8rH6AL/E9iSxxWpNCTAo55pfmxga7j55qIGpFRA1Q4 TdhknnVubjhVUP1jgJWZNlH6lnSyVTJd9frp8dZAyQ9CHT5WrBxqKxoD8Rs5ZhVyT3 x78m2kDI5CtWFaS0x7ZkenoiGoG1DiEscYMQRnKZr/CYK1ZVnuZ4mL1FMVZjmkrdjO kErCR7g/pBj46Am9HpwW2XuM8TQvOnx3QHQJS1XzyY0JPRzUCKmcm1g3AZXFUchRhI AWQYjr9omPVXZJiPgfs6SdHQNAjsC7WVR3EfXHxuPiQAILLEQGA7ciP8DLsnph4H3u SJMzwOIRCR90Q== Received: by mail-oi1-f181.google.com with SMTP id n140so14171310oig.9 for ; Mon, 12 Apr 2021 10:27:17 -0700 (PDT) X-Gm-Message-State: AOAM533/FIfDx7t7rHl5VufPgxgmXdET+p3btG+eHFK5IGyCOWgdvzmX 9N9sKAq+n8Fu4IATXeI33YZ7KccB54YMh2i4H1s= X-Google-Smtp-Source: ABdhPJw0PAmL8JNqE7eZXPg1Quh4+QZFJGyU72Re/tpQFbE4sv+q5EURgJpsZ106F8lo3QDVjVsy1bzWyS5FVqOZO6k= X-Received: by 2002:aca:7c4:: with SMTP id 187mr168217oih.47.1618248436309; Mon, 12 Apr 2021 10:27:16 -0700 (PDT) MIME-Version: 1.0 References: <20210412090545.2130-1-Sunny.Wang@arm.com> In-Reply-To: From: "Ard Biesheuvel" Date: Mon, 12 Apr 2021 19:27:04 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH 1/1] Platform/RaspberryPi: Setup option for disabling Fast Boot To: Pete Batard Cc: Sunny Wang , edk2-devel-groups-io , Samer El-Haj-Mahmoud , Jeremy Linton , Ard Biesheuvel Content-Type: text/plain; charset="UTF-8" On Mon, 12 Apr 2021 at 15:03, Pete Batard wrote: > > Hi Sunny, > > Functionaly, I see no issues with this patch, and it's indeed a good > solution to the problem of trying to satisfy everyone while fixing the > issues we are seeing. > > There are however a few minor typos and whitespace issues, that I'm > detailing below. So could you please send a v2 to address these? > Agree with Pete. Thanks for providing a fix for this, and please incorporate the changes suggested by Pete. Then, could we *please* figure out a way to get this patch on the mailing list without Windows whitespace damage? Not sure if anything changed recently, but not a single EDK2 patch has arrived in my mailbox recently in a form that I could apply with 'git am'. Thanks, Ard. > On 2021.04.12 10:05, Sunny Wang wrote: > > This is a fix for https://github.com/pftf/RPi4/issues/114. > > > > Changes: > > 1. Add a setup option called Boot Policy and consume the setting > > during boot to whether perform or skip ConnectAll. > > 2. The Default setting is set to Full discovery because it is not > > worth enabling Fast boot by default on RaspberryPi systems. > > Enabling it just saves boot time about 1 second, but caused a > > lot of issues. > > > > Testing Done: > > - Booted to Standalone UEFI shell on SD card and use drivers > > command to check the result with Fast Boot and Full discovery > > settings. Then, child/device handles are created as expected. > > > > Note and to-do items: > > - The root cause looks like that boot loaders and some tools like > > grub and iPXE haven't supported selective connect/Fast boot. > > However, system firmware should still provide a setup option for > > user to enable Fast boot with old version boot loaders and tools > > , which is why we proposed this change. We will also report this > > issue to boot loader and tool vendors/open source GitHubs. > > - We Will add more options for connecting specific type devices so > > that we can still have the shortest boot time for all use cases. > > > > Cc: Samer El-Haj-Mahmoud > > Cc: Jeremy Linton > > Cc: Pete Batard > > Cc: Ard Biesheuvel > > Signed-off-by: Sunny Wang > > --- > > .../RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c | 11 ++++++++++- > > .../RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf | 3 ++- > > .../Drivers/ConfigDxe/ConfigDxeHii.uni | 10 +++++++++- > > .../Drivers/ConfigDxe/ConfigDxeHii.vfr | 15 ++++++++++++++- > > Platform/RaspberryPi/Include/ConfigVars.h | 12 +++++++++++- > > .../Library/PlatformBootManagerLib/PlatformBm.c | 16 ++++++++++++++-- > > .../PlatformBootManagerLib.inf | 1 + > > Platform/RaspberryPi/RPi3/RPi3.dsc | 10 +++++++++- > > Platform/RaspberryPi/RPi4/RPi4.dsc | 9 ++++++++- > > Platform/RaspberryPi/RaspberryPi.dec | 2 ++ > > 10 files changed, 80 insertions(+), 9 deletions(-) > > > > diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c > > index 22f86d4d44..d3c5869949 100644 > > --- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c > > +++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c > > @@ -1,6 +1,6 @@ > > /** @file > > > > * > > > > - * Copyright (c) 2019 - 2020, ARM Limited. All rights reserved. > > > > + * Copyright (c) 2019 - 2021, ARM Limited. All rights reserved. > > > > * Copyright (c) 2018 - 2020, Andrei Warkentin > > > > * > > > > * SPDX-License-Identifier: BSD-2-Clause-Patent > > > > @@ -281,6 +281,15 @@ SetupVariables ( > > ); > > > > } > > > > > > > > + Size = sizeof (UINT32); > > > > + Status = gRT->GetVariable (L"BootPolicy", > > > > + &gConfigDxeFormSetGuid, > > > > + NULL, &Size, &Var32); > > > > + if (EFI_ERROR (Status)) { > > > > + Status = PcdSet32S (PcdBootPolicy, PcdGet32 (PcdBootPolicy)); > > > > + ASSERT_EFI_ERROR (Status); > > > > + } > > > > + > > > > Size = sizeof (UINT32); > > > > Status = gRT->GetVariable (L"SdIsArasan", > > > > &gConfigDxeFormSetGuid, > > > > diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf > > index d51e54e010..032e40b0c3 100644 > > --- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf > > +++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf > > @@ -2,7 +2,7 @@ > > # > > > > # Component description file for the RasbperryPi DXE platform config driver. > > > > # > > > > -# Copyright (c) 2019 - 2020, ARM Limited. All rights reserved. > > > > +# Copyright (c) 2019 - 2021, ARM Limited. All rights reserved. > > > > # Copyright (c) 2018 - 2020, Andrei Warkentin > > > > # > > > > # SPDX-License-Identifier: BSD-2-Clause-Patent > > > > @@ -93,6 +93,7 @@ > > gRaspberryPiTokenSpaceGuid.PcdRamLimitTo3GB > > > > gRaspberryPiTokenSpaceGuid.PcdFanOnGpio > > > > gRaspberryPiTokenSpaceGuid.PcdFanTemp > > > > + gRaspberryPiTokenSpaceGuid.PcdBootPolicy > > > > > > > > [Depex] > > > > gPcdProtocolGuid AND gRaspberryPiFirmwareProtocolGuid > > > > diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni > > index 466fa852cb..7b14fdf39f 100644 > > --- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni > > +++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni > > @@ -1,7 +1,7 @@ > > /** @file > > > > * > > > > * Copyright (c) 2018, Andrei Warkentin > > > > - * Copyright (c) 2020, ARM Limited. All rights reserved. > > > > + * Copyright (c) 2020 - 2021, ARM Limited. All rights reserved. > > > > * > > > > * SPDX-License-Identifier: BSD-2-Clause-Patent > > > > * > > > > @@ -60,6 +60,14 @@ > > #string STR_ADVANCED_ASSET_TAG_PROMPT #language en-US "Asset Tag" > > > > #string STR_ADVANCED_ASSET_TAG_HELP #language en-US "Set the system Asset Tag" > > > > > > Personally, I would move the Boot Policy section between "System Table > Selection" and "ACPI Fan Control" if we try to keep the advanced > settings in the order we think they will be most used. > > > > > +#string STR_BOOT_POLICY_PROMPT #language en-US "Boot Policy" > > > > +#string STR_BOOT_POLICY_HELP #language en-US "When Fast Boot is selected, only required devices will be discoverd for reducing " > > "discoverd" -> "discovered" > > > > > + "the boot time." > > Missing a space after "the boot time.". It should be "the boot time. ", > else the help text will display as "(...) the boot time.When Full > Discovery (...)" > > > > > + "When Full Discovery is selected, all the devices will be discoverd for some " > > "discoverd" -> "discovered" > > > > > + "scenarios such as system deployement and diagnostic tests" > > "deployement" -> "deployment" > > > > > +#string STR_FAST_BOOT #language en-US "Fast Boot" > > > > +#string STR_FULL_DISCOVERY #language en-US "Full Discovery" > > > > + > > > > /* > > > > * MMC/SD configuration. > > > > */ > > > > diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr > > index cc7a09cfb7..5dc558ec08 100644 > > --- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr > > +++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr > > @@ -1,7 +1,7 @@ > > /** @file > > > > * > > > > * Copyright (c) 2018 Andrei Warkentin > > > > - * Copyright (c) 2020, ARM Limited. All rights reserved. > > > > + * Copyright (c) 2020 - 2021, ARM Limited. All rights reserved. > > > > * > > > > * SPDX-License-Identifier: BSD-2-Clause-Patent > > > > * > > > > @@ -116,6 +116,11 @@ formset > > name = DisplayEnableSShot, > > > > guid = CONFIGDXE_FORM_SET_GUID; > > > > > > > > + efivarstore BOOT_POLICY_VARSTORE_DATA, > > > > + attribute = EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS | EFI_VARIABLE_NON_VOLATILE, > > > > + name = BootPolicy, > > > > + guid = CONFIGDXE_FORM_SET_GUID; > > > > + > > > > form formid = 1, > > > > title = STRING_TOKEN(STR_FORM_SET_TITLE); > > > > subtitle text = STRING_TOKEN(STR_NULL_STRING); > > > > @@ -220,6 +225,14 @@ formset > > minsize = 0, > > > > maxsize = ASSET_TAG_STR_MAX_LEN, > > > > endstring; > > > > + > > > > + oneof varid = BootPolicy.BootPolicy, > > > > + prompt = STRING_TOKEN(STR_BOOT_POLICY_PROMPT), > > > > + help = STRING_TOKEN(STR_BOOT_POLICY_HELP), > > > > + flags = NUMERIC_SIZE_4 | INTERACTIVE | RESET_REQUIRED, > > > > + option text = STRING_TOKEN(STR_FAST_BOOT ), value = FAST_BOOT , flags = 0; > > "STRING_TOKEN(STR_FAST_BOOT )" -> "STRING_TOKEN(STR_FAST_BOOT)" > > > > > + option text = STRING_TOKEN(STR_FULL_DISCOVERY), value = FULL_DISCOVERY, flags = DEFAULT; > > > > + endoneof; > > > > endform; > > > > > > > > form formid = 0x1003, > > > > diff --git a/Platform/RaspberryPi/Include/ConfigVars.h b/Platform/RaspberryPi/Include/ConfigVars.h > > index 142317985a..3347f899df 100644 > > --- a/Platform/RaspberryPi/Include/ConfigVars.h > > +++ b/Platform/RaspberryPi/Include/ConfigVars.h > > @@ -1,7 +1,7 @@ > > /** @file > > > > * > > > > * Copyright (c) 2020, Andrei Warkentin > > > > - * Copyright (c) 2020, ARM Limited. All rights reserved. > > > > + * Copyright (c) 2020 - 2021, ARM Limited. All rights reserved. > > > > * > > > > * SPDX-License-Identifier: BSD-2-Clause-Patent > > > > * > > > > @@ -143,4 +143,14 @@ typedef struct { > > UINT32 EnableDma; > > > > } MMC_EMMC_DMA_VARSTORE_DATA; > > > > > > > > +#define FAST_BOOT 0 > > > > +#define FULL_DISCOVERY 1 > > > > +typedef struct { > > > > + /* > > > > + * 0 - Fast Boot > > > > + * 1 - Full Discovrey (Connect All) > > "Discovrey" -> "Discovery" > > > > > + */ > > > > + UINT32 BootPolicy; > > > > +} BOOT_POLICY_VARSTORE_DATA; > > > > + > > > > #endif /* CONFIG_VARS_H */ > > > > diff --git a/Platform/RaspberryPi/Library/PlatformBootManagerLib/PlatformBm.c b/Platform/RaspberryPi/Library/PlatformBootManagerLib/PlatformBm.c > > index c2fc40b8ea..abbe4fb3d0 100644 > > --- a/Platform/RaspberryPi/Library/PlatformBootManagerLib/PlatformBm.c > > +++ b/Platform/RaspberryPi/Library/PlatformBootManagerLib/PlatformBm.c > > @@ -4,7 +4,7 @@ > > * Copyright (c) 2017-2018, Andrei Warkentin > > > > * Copyright (c) 2016, Linaro Ltd. All rights reserved. > > > > * Copyright (c) 2015-2016, Red Hat, Inc. > > > > - * Copyright (c) 2014-2020, ARM Ltd. All rights reserved. > > > > + * Copyright (c) 2014-2021, ARM Ltd. All rights reserved. > > > > * Copyright (c) 2004-2016, Intel Corporation. All rights reserved. > > > > * > > > > * SPDX-License-Identifier: BSD-2-Clause-Patent > > > > @@ -26,9 +26,11 @@ > > #include > > > > #include > > > > > > > > +#include > > I'd remove the extra line between and > > > > > + > > > > #include "PlatformBm.h" > > > > > > > > -#define BOOT_PROMPT L"ESC (setup), F1 (shell), ENTER (boot)" > > > > +#define BOOT_PROMPT L"ESC (setup), F1 (shell), ENTER (boot)\n" > > Why are you adding the carriage return? Was there an issue with the > display of this prompt? > > > > > > > > > #define DP_NODE_LEN(Type) { (UINT8)sizeof (Type), (UINT8)(sizeof (Type) >> 8) } > > > > > > > > @@ -633,6 +635,16 @@ PlatformBootManagerAfterConsole ( > > Print (BOOT_PROMPT); > > > > } > > > > > > > > + // > > > > + // Connect the rest of the devices if the boot polcy is set to Full discovery > > > > + // > > > > + if (PcdGet32 (PcdBootPolicy) == FULL_DISCOVERY) { > > > > + DEBUG ((DEBUG_INFO, "Boot Policy is Full Discovery. Connect All devices\n")); > > "All" -> "all" > > > > > + EfiBootManagerConnectAll (); > > > > + } else if (PcdGet32 (PcdBootPolicy) == FAST_BOOT) { > > > > + DEBUG ((DEBUG_INFO, "Boot Policy is Fast Boot. Skip connecting all devices\n")); > > > > + } > > > > + > > > > Status = gBS->LocateProtocol (&gEsrtManagementProtocolGuid, NULL, (VOID**)&EsrtManagement); > > > > if (!EFI_ERROR (Status)) { > > > > EsrtManagement->SyncEsrtFmp (); > > > > diff --git a/Platform/RaspberryPi/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf b/Platform/RaspberryPi/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf > > index 88f6f8fe09..fbf510ab96 100644 > > --- a/Platform/RaspberryPi/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf > > +++ b/Platform/RaspberryPi/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf > > @@ -63,6 +63,7 @@ > > [Pcd] > > > > gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut > > > > gRaspberryPiTokenSpaceGuid.PcdSdIsArasan > > > > + gRaspberryPiTokenSpaceGuid.PcdBootPolicy > > > > > > > > [Guids] > > > > gEfiFileInfoGuid > > > > diff --git a/Platform/RaspberryPi/RPi3/RPi3.dsc b/Platform/RaspberryPi/RPi3/RPi3.dsc > > index 0961133ae9..ddb03e405f 100644 > > --- a/Platform/RaspberryPi/RPi3/RPi3.dsc > > +++ b/Platform/RaspberryPi/RPi3/RPi3.dsc > > @@ -1,6 +1,6 @@ > > # @file > > > > # > > > > -# Copyright (c) 2011 - 2020, ARM Limited. All rights reserved. > > > > +# Copyright (c) 2011 - 2021, ARM Limited. All rights reserved. > > > > # Copyright (c) 2014, Linaro Limited. All rights reserved. > > > > # Copyright (c) 2015 - 2021, Intel Corporation. All rights reserved. > > > > # Copyright (c) 2017 - 2018, Andrei Warkentin > > > > @@ -512,6 +512,14 @@ > > gRaspberryPiTokenSpaceGuid.PcdFanOnGpio|L"FanOnGpio"|gConfigDxeFormSetGuid|0x0|0 > > > > gRaspberryPiTokenSpaceGuid.PcdFanTemp|L"FanTemp"|gConfigDxeFormSetGuid|0x0|0 > > > > > > > > + # > > > > + # Boot Policy > > > > + # 0 - Fast Boot > > > > + # 1 - Full Discovrey (Connect All) > > "Discovrey" -> "Discovery" > > > > > + # > > > > + gRaspberryPiTokenSpaceGuid.PcdBootPolicy|L"BootPolicy"|gConfigDxeFormSetGuid|0x0|1 > > > > + > > > > + > > Please remove the extra line. There should be just one blank like > between the sections. > > > > > # > > > > # Reset-related. > > > > # > > > > diff --git a/Platform/RaspberryPi/RPi4/RPi4.dsc b/Platform/RaspberryPi/RPi4/RPi4.dsc > > index ff802d8347..8ee1922a44 100644 > > --- a/Platform/RaspberryPi/RPi4/RPi4.dsc > > +++ b/Platform/RaspberryPi/RPi4/RPi4.dsc > > @@ -1,6 +1,6 @@ > > # @file > > > > # > > > > -# Copyright (c) 2011 - 2020, ARM Limited. All rights reserved. > > > > +# Copyright (c) 2011 - 2021, ARM Limited. All rights reserved. > > > > # Copyright (c) 2017 - 2018, Andrei Warkentin > > > > # Copyright (c) 2015 - 2021, Intel Corporation. All rights reserved. > > > > # Copyright (c) 2014, Linaro Limited. All rights reserved. > > > > @@ -528,6 +528,13 @@ > > gRaspberryPiTokenSpaceGuid.PcdFanOnGpio|L"FanOnGpio"|gConfigDxeFormSetGuid|0x0|0 > > > > gRaspberryPiTokenSpaceGuid.PcdFanTemp|L"FanTemp"|gConfigDxeFormSetGuid|0x0|60 > > > > > > > > + # > > > > + # Boot Policy > > > > + # 0 - Fast Boot > > > > + # 1 - Full Discovrey (Connect All) > > "Discovrey" -> "Discovery" > > Regards, > > /Pete > > > > > + # > > > > + gRaspberryPiTokenSpaceGuid.PcdBootPolicy|L"BootPolicy"|gConfigDxeFormSetGuid|0x0|1 > > > > + > > > > # > > > > # Reset-related. > > > > # > > > > diff --git a/Platform/RaspberryPi/RaspberryPi.dec b/Platform/RaspberryPi/RaspberryPi.dec > > index 08135717ed..8eb1c2bac7 100644 > > --- a/Platform/RaspberryPi/RaspberryPi.dec > > +++ b/Platform/RaspberryPi/RaspberryPi.dec > > @@ -2,6 +2,7 @@ > > # > > > > # Copyright (c) 2016, Linaro, Ltd. All rights reserved. > > > > # Copyright (c) 2017-2018, Andrei Warkentin > > > > +# Copyright (c) 2021, ARM Limited. All rights reserved. > > > > # > > > > # SPDX-License-Identifier: BSD-2-Clause-Patent > > > > # > > > > @@ -70,3 +71,4 @@ > > gRaspberryPiTokenSpaceGuid.PcdFanTemp|0|UINT32|0x0000001D > > > > gRaspberryPiTokenSpaceGuid.PcdPlatformResetDelay|0|UINT32|0x0000001E > > > > gRaspberryPiTokenSpaceGuid.PcdMmcEnableDma|0|UINT32|0x0000001F > > > > + gRaspberryPiTokenSpaceGuid.PcdBootPolicy|0|UINT32|0x00000020 > > >