Re: [edk2-devel] [plaforms/devel-riscv-v2 PATCHv2 09/14] U500Pkg/Library: Initial version of PlatformBootManagerLib

["Leif Lindholm" <leif.lindholm@linaro.org>]

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.chen@hpe.com>
> > Cc: Palmer Dabbelt <palmer@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.chen@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