On Fri, Oct 18, 2019 at 06:23:05AM +0000, Abner Chang wrote: > > -----Original Message----- > > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of > > Leif Lindholm > > Sent: Thursday, October 3, 2019 6:03 AM > > To: devel@edk2.groups.io; Chen, Gilbert <gilbert.c...@hpe.com> > > Cc: Palmer Dabbelt <pal...@sifive.com> > > Subject: Re: [edk2-devel] [plaforms/devel-riscv-v2 PATCHv2 09/14] > > U500Pkg/Library: Initial version of PlatformBootManagerLib > > > > On Thu, Sep 19, 2019 at 11:51:26AM +0800, Gilbert Chen wrote: > > > SiFive RISC-V U500 Platform Boot Manager library. > > > > First of all, let me say that I think before upstreaming to master, you > > ought to > > look into merging PlatformBootManagerLibs for all Risc-V platforms. Like we > > have for *most* ARM/AARCH64 platforms with > > ArmPkg/Library/PlatformBootManagerLib/. > > > > (Longer-term we should merge them all together into a single one.) > > > > > Signed-off-by: Gilbert Chen <gilbert.c...@hpe.com> > > > --- > > > .../Library/PlatformBootManagerLib/MemoryTest.c | 682 > > +++++++++++++++++++++ > > > .../PlatformBootManagerLib/PlatformBootManager.c | 274 +++++++++ > > > .../PlatformBootManagerLib/PlatformBootManager.h | 135 ++++ > > > .../PlatformBootManagerLib.inf | 63 ++ > > > .../Library/PlatformBootManagerLib/PlatformData.c | 49 ++ > > > .../Library/PlatformBootManagerLib/Strings.uni | 28 + > > > 6 files changed, 1231 insertions(+) > > > create mode 100644 > > > > > Platform/RiscV/SiFive/U500Pkg/Library/PlatformBootManagerLib/MemoryT > > es > > > t.c create mode 100644 > > > > > Platform/RiscV/SiFive/U500Pkg/Library/PlatformBootManagerLib/PlatformB > > > ootManager.c create mode 100644 > > > > > Platform/RiscV/SiFive/U500Pkg/Library/PlatformBootManagerLib/PlatformB > > > ootManager.h create mode 100644 > > > > > Platform/RiscV/SiFive/U500Pkg/Library/PlatformBootManagerLib/PlatformB > > > ootManagerLib.inf create mode 100644 > > > > > Platform/RiscV/SiFive/U500Pkg/Library/PlatformBootManagerLib/PlatformD > > > ata.c create mode 100644 > > > Platform/RiscV/SiFive/U500Pkg/Library/PlatformBootManagerLib/Strings.u > > > ni > > > > > > diff --git > > > > > a/Platform/RiscV/SiFive/U500Pkg/Library/PlatformBootManagerLib/Memory > > T > > > est.c > > > > > b/Platform/RiscV/SiFive/U500Pkg/Library/PlatformBootManagerLib/Memor > > yT > > > est.c > > > new file mode 100644 > > > index 00000000..8c6d89e9 > > > --- /dev/null > > > +++ > > b/Platform/RiscV/SiFive/U500Pkg/Library/PlatformBootManagerLib/Mem > > > +++ oryTest.c > > > > Why build a MemoryTest into the PlatformBootManagerLib? > > Why not to do memory if platform provides memory test protocol?
The question was why build it into the PlatformBootManagerLib? I would much prefer something like what is done in existing plaforms with gEfiGenericMemTestProtocolGuid. > > > > > @@ -0,0 +1,682 @@ > > > +/** @file > > > + Perform the RISC-V platform memory test > > > + > > > +Copyright (c) 2019, Hewlett Packard Enterprise Development LP. All > > > +rights reserved.<BR> Copyright (c) 2004 - 2015, Intel Corporation. > > > +All rights reserved.<BR> > > > + > > > +SPDX-License-Identifier: BSD-2-Clause-Patent > > > + > > > +**/ > > > + > > > +#include "PlatformBootManager.h" > > > + > > > +EFI_HII_HANDLE gStringPackHandle = NULL; > > > +EFI_GUID mPlatformBootManagerStringPackGuid = { > > > + 0x154dd51, 0x9079, 0x4a10, { 0x89, 0x5c, 0x9c, 0x7, 0x72, 0x81, > > > +0x57, 0x88 } > > > + }; > > > +// extern UINT8 BdsDxeStrings[]; > > > > No need to keep commented-out code in. > > > > > + > > > +// > > > +// BDS Platform Functions > > > +// > > > +/** > > > + > > > + Show progress bar with title above it. It only works in Graphics mode. > > > + > > > + @param TitleForeground Foreground color for Title. > > > + @param TitleBackground Background color for Title. > > > + @param Title Title above progress bar. > > > + @param ProgressColor Progress bar color. > > > + @param Progress Progress (0-100) > > > + @param PreviousValue The previous value of the progress. > > > + > > > + @retval EFI_STATUS Success update the progress bar > > > + > > > +**/ > > > +EFI_STATUS > > > +PlatformBootManagerShowProgress ( > > > > I'm not a super fan of how this file integrates a custom memory test with a > > direct (copy) graphical progress indicator. There are both common memory > > test and common progress indicators - please use those instead. And > > improve them if they are currently insufficient for your needs. > > Could you indicate where are those common libraries? Thanks. There is MdeModulePkg/Library/DisplayUpdateProgressLibGraphics and MdeModulePkg/Library/DisplayUpdateProgressLibText. / Leif -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#49308): https://edk2.groups.io/g/devel/message/49308 Mute This Topic: https://groups.io/mt/34196357/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-