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]
-=-=-=-=-=-=-=-=-=-=-=-
