On Mon, 12 Apr 2021 at 15:03, Pete Batard <[email protected]> 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 <[email protected]> > > Cc: Jeremy Linton <[email protected]> > > Cc: Pete Batard <[email protected]> > > Cc: Ard Biesheuvel <[email protected]> > > Signed-off-by: Sunny Wang <[email protected]> > > --- > > .../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 > > <[email protected]> > > > > * > > > > * 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 > > <[email protected]> > > > > # > > > > # 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 <[email protected]> > > > > - * 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 <[email protected]> > > > > - * 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 <[email protected]> > > > > - * 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 <[email protected]> > > > > * 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 <Guid/EventGroup.h> > > > > #include <Guid/TtyTerm.h> > > > > > > > > +#include <ConfigVars.h> > > I'd remove the extra line between <Guid/TtyTerm.h> and <ConfigVars.h> > > > > > + > > > > #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 > > <[email protected]> > > > > @@ -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 > > <[email protected]> > > > > # 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 <[email protected]> > > > > +# 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 > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#73971): https://edk2.groups.io/g/devel/message/73971 Mute This Topic: https://groups.io/mt/82033650/21656 Group Owner: [email protected] Unsubscribe: https://edk2.groups.io/g/devel/unsub [[email protected]] -=-=-=-=-=-=-=-=-=-=-=-
