From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f66.google.com (mail-wm1-f66.google.com [209.85.128.66]) by mx.groups.io with SMTP id smtpd.web09.815.1571669482078399570 for ; Mon, 21 Oct 2019 07:51:22 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=Dxy8iGpU; spf=pass (domain: linaro.org, ip: 209.85.128.66, mailfrom: leif.lindholm@linaro.org) Received: by mail-wm1-f66.google.com with SMTP id f22so13118949wmc.2 for ; Mon, 21 Oct 2019 07:51:21 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=Tnf2LhxF82k+LjC0rCTdafjTDVLAdR3PLZnlGE0ED9k=; b=Dxy8iGpUeJdYBQ1EY+gMPWSddKxDwTeQiVY3R8hxOtb7yhVEQjVOhdNIyJWE4gvLm7 OxBKUOauLxH8g+Pw8mRqRIvI3BDb6rthUwizmcFrprvHchuCwbZ5xz0Kc1zdFk5bqlv4 AZO8I+CyBGlO8yUIkrwDRxn06o5rBz3fBN9gUOF0iekTefDLUGDBCFYexe1KNivZU2tu Sj/UanHdKXfQLZszCQdefe+O+Ed6RJpW/o/GS7ZaEXfqyFqINcrp6FyXtCQYWAPK/T5t 7dcZx+56h+jd/9Iw1J2R7Ggbo/dsiy0ASE08U3j85uNXV8Dhyh1MjuW6/mTLCLkVrRR0 LQ4Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=Tnf2LhxF82k+LjC0rCTdafjTDVLAdR3PLZnlGE0ED9k=; b=r4XY+2Sk4lOkldUKwJPNURnfjkSK6d6Jr7wMosj/hCXXZgxuic3RQHgrBx/R/IG5DH YRzaFCIBsYDYD1zaNEE0MKALXjH9LTzkfmOukixmPm9LAq6vOGC6MgBF2Y6nFcCncSGb B016fvZjyaV/XqZgSRjOzWN/bToN3gJK77GyaC8V4ITBTLs5ZvHzxKGLNirrF9UFNOnR x+urW5L0Q4wL5ZKTNUSE+S9tbEKm6b0W5hxPxFScGwwALNSANNGNiystfm3K/dSZgJMW LklNlh3dJLVIWVtkXzlgs4fQUrytAasZGSWGCBrn322ZToMPbn/N9p8Xxmy19rXJY0JY mZRQ== X-Gm-Message-State: APjAAAW8nM7OLGQNsvASUlm10sVC/g4cz+0IUtnxPJDqFVj/lwwfLSmZ Q1OGDtOuXawBiZJYCPYsY05DjQdOI28= X-Google-Smtp-Source: APXvYqyxf1+FUh2W51ZtTBfnNtXLXamW5mM+HOGifQjTR4/cPxC6jLuBjnJGvklEbP38UN2S4MJc2Q== X-Received: by 2002:a1c:f709:: with SMTP id v9mr20773762wmh.62.1571669479949; Mon, 21 Oct 2019 07:51:19 -0700 (PDT) Return-Path: Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id r27sm26690099wrc.55.2019.10.21.07.51.19 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 21 Oct 2019 07:51:19 -0700 (PDT) Date: Mon, 21 Oct 2019 15:51:17 +0100 From: "Leif Lindholm" To: devel@edk2.groups.io, abner.chang@hpe.com Cc: "Chen, Gilbert" , Palmer Dabbelt Subject: Re: [edk2-devel] [plaforms/devel-riscv-v2 PATCHv2 09/14] U500Pkg/Library: Initial version of PlatformBootManagerLib Message-ID: <20191021145117.GF16820@bivouac.eciton.net> References: <20190919035131.4700-1-gilbert.chen@hpe.com> <20190919035131.4700-10-gilbert.chen@hpe.com> <20191002220248.GI25504@bivouac.eciton.net> MIME-Version: 1.0 In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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 > > Cc: Palmer Dabbelt > > 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 > > > --- > > > .../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.
Copyright (c) 2004 - 2015, Intel Corporation. > > > +All rights reserved.
> > > + > > > +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