Re: [PATCH v2 21/28] Slicon/NXP: Add PlatformPei Lib

["Leif Lindholm" <leif@nuviainc.com>]

OK, taking another look at this patch, this simply needs to be
deleted. Here is the sum total relevant difference compared to the
ArmPlatformPkg one.

  DEBUG ((DEBUG_INIT, "Edk2 version is %a\n", XPRINT (WORKSPACE_GIT_VERSION)));
  DEBUG ((DEBUG_INIT, "Edk2 platforms version is %a\n", XPRINT (PACKAGES_PATH_GIT_VERSION)));

If all you want to do is to print that sort of thing, please don't
fork a core library to do so.

First of all, please do like most other platforms and override
gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVersionString
if -D FIRMWARE_VER is specified on your build command line.

You can then extract current top commits of your respective
repositories and not worry about getting this.
I would suggest iterating across all locations in PACKAGES_PATH and
then doing something similar to
https://git.linaro.org/uefi/uefi-tools.git/tree/edk2-build.sh#n400
appending together.

If something like this should be integrated into the build system
(which might not be a bad idea), then it needs to be so properly,
rather than shoehorned in for each platform.
(In the past, this was difficult because we supported both git and
svn, but I would say we have given up pretending that is possible.)

For now, you could add the printout in a standalone Depex TRUE PEIM
added to your [FV.FVMAIN_COMPACT].

/
    Leif

On Mon, Apr 06, 2020 at 14:53:02 +0000, Pankaj Bansal (OSS) wrote:
> 
> 
> > -----Original Message-----
> > From: Leif Lindholm <leif@nuviainc.com>
> > Sent: Wednesday, April 1, 2020 8:23 PM
> > To: Pankaj Bansal (OSS) <pankaj.bansal@oss.nxp.com>
> > Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>; Michael D Kinney
> > <michael.d.kinney@intel.com>; devel@edk2.groups.io; Varun Sethi
> > <V.Sethi@nxp.com>; Samer El-Haj-Mahmoud <Samer.El-Haj-
> > Mahmoud@arm.com>; Jon Nettleton <jon@solid-run.com>
> > Subject: Re: [PATCH v2 21/28] Slicon/NXP: Add PlatformPei Lib
> > 
> > On Fri, Mar 20, 2020 at 20:05:36 +0530, Pankaj Bansal wrote:
> > > From: Pankaj Bansal <pankaj.bansal@nxp.com>
> > >
> > > PlatformPeiLib is going to be linked to Platform PEIM.
> > >
> > > Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
> > > ---
> > >  .../Library/PlatformPeiLib/PlatformPeiLib.c   | 30 ++++++++++++++
> > >  .../Library/PlatformPeiLib/PlatformPeiLib.inf | 41 +++++++++++++++++++
> > >  Silicon/NXP/NxpQoriqLs.dsc.inc                |  3 +-
> > >  3 files changed, 73 insertions(+), 1 deletion(-)
> > >  create mode 100644 Silicon/NXP/Library/PlatformPeiLib/PlatformPeiLib.c
> > >  create mode 100644 Silicon/NXP/Library/PlatformPeiLib/PlatformPeiLib.inf
> > >
> > > diff --git a/Silicon/NXP/Library/PlatformPeiLib/PlatformPeiLib.c
> > b/Silicon/NXP/Library/PlatformPeiLib/PlatformPeiLib.c
> > > new file mode 100644
> > > index 000000000000..f64e564469f8
> > > --- /dev/null
> > > +++ b/Silicon/NXP/Library/PlatformPeiLib/PlatformPeiLib.c
> > > @@ -0,0 +1,30 @@
> > > +/** @file
> > > +*
> > > +*  Copyright (c) 2011-2014, ARM Limited. All rights reserved.
> > > +*  Copyright 2020 NXP
> > > +*
> > > +*  SPDX-License-Identifier: BSD-2-Clause-Patent
> > > +*
> > > +**/
> > > +
> > > +#include <PiPei.h>
> > > +
> > > +#include <Library/HobLib.h>
> > > +#include <Library/DebugLib.h>
> > > +#include <Library/PcdLib.h>
> > 
> > Although this is based on an existing library, please sort includes
> > alphabetically here.
> > 
> > > +
> > > +#define XPRINT(x) PRINT(x)
> > > +#define PRINT(x) #x
> > 
> > This isn't a PRINT operation, this is a Stringize operation.
> 
> OK, I can rename these to 
> #define PRINTSTR(x) STR(x)
> #define STR(x) #x
> 
> > 
> > > +
> > > +EFI_STATUS
> > > +EFIAPI
> > > +PlatformPeim (
> > > +  VOID
> > > +  )
> > > +{
> > > +  BuildFvHob (PcdGet64 (PcdFvBaseAddress), PcdGet32 (PcdFvSize));
> > > +  DEBUG ((DEBUG_INIT, "Edk2 version is %a\n", XPRINT
> > (WORKSPACE_GIT_VERSION)));
> > > +  DEBUG ((DEBUG_INIT, "Edk2 platforms version is %a\n", XPRINT
> > (PACKAGES_PATH_GIT_VERSION)));
> > 
> > The only benefit I can see from the macro as opposed to using '#'
> > directly is that it permits wrapping of too long lines, so please do
> > that.
> 
> OK.
> 
> > 
> > > +
> > > +  return EFI_SUCCESS;
> > > +}
> > > diff --git a/Silicon/NXP/Library/PlatformPeiLib/PlatformPeiLib.inf
> > b/Silicon/NXP/Library/PlatformPeiLib/PlatformPeiLib.inf
> > > new file mode 100644
> > > index 000000000000..fb42693daa20
> > > --- /dev/null
> > > +++ b/Silicon/NXP/Library/PlatformPeiLib/PlatformPeiLib.inf
> > > @@ -0,0 +1,41 @@
> > > +#/** @file
> > > +#
> > > +#  Copyright (c) 2011-2012, ARM Limited. All rights reserved.
> > > +#  Copyright 2020 NXP
> > > +#
> > > +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> > > +#
> > > +#**/
> > > +
> > > +[Defines]
> > > +  INF_VERSION                    = 0x00010005
> > 
> > Update version.
> > 
> > > +  BASE_NAME                      = ArmPlatformPeiLib
> > > +  FILE_GUID                      = 49d37060-70b5-11e0-aa2d-0002a5d5c51b
> > 
> > Unless this is another magic GUID filename (like ACPI), please
> > generate a new GUID.
> 
> Since this Library is replacement implementation of
> ArmPlatformPkg/PlatformPei/PlatformPeiLib.inf,
> Couldn't we use the same GUID ?
>
> > 
> > > +  MODULE_TYPE                    = PEIM
> > > +  VERSION_STRING                 = 1.0
> > > +  LIBRARY_CLASS                  = PlatformPeiLib
> > > +
> > > +[BuildOptions]
> > > +  GCC:*_*_*_CC_FLAGS = -
> > DWORKSPACE_GIT_VERSION="$(WORKSPACE_GIT_VERSION)"
> > > +  GCC:*_*_*_CC_FLAGS = -
> > DPACKAGES_PATH_GIT_VERSION="$(PACKAGES_PATH_GIT_VERSION)"
> > 
> > Does this not require special magic build command line options to do
> > anything useful? This needs documenting.
> 
> Actually I had submitted a patch is BaseTools for this: 
> https://edk2.groups.io/g/devel/message/53146
> 
> This patch makes use of that BaseTools patch.
> But the BaseTools patch was not accepted because that is Linux specific.
> Still these changes don't cause any negative affect.
> Without BaseTools patch, empty string would be printed.
> 
> > 
> > /
> >     Leif
> > 
> > > +
> > > +[Sources]
> > > +  PlatformPeiLib.c
> > > +
> > > +[Packages]
> > > +  ArmPkg/ArmPkg.dec
> > > +  MdeModulePkg/MdeModulePkg.dec
> > > +  MdePkg/MdePkg.dec
> > > +  Silicon/NXP/NxpQoriqLs.dec
> > > +
> > > +[LibraryClasses]
> > > +  DebugLib
> > > +  HobLib
> > > +  PcdLib
> > > +
> > > +[FixedPcd]
> > > +  gArmTokenSpaceGuid.PcdFvBaseAddress
> > > +  gArmTokenSpaceGuid.PcdFvSize
> > > +
> > > +[depex]
> > > +  TRUE
> > > diff --git a/Silicon/NXP/NxpQoriqLs.dsc.inc b/Silicon/NXP/NxpQoriqLs.dsc.inc
> > > index 234a5e2707cd..5f77f47f0399 100644
> > > --- a/Silicon/NXP/NxpQoriqLs.dsc.inc
> > > +++ b/Silicon/NXP/NxpQoriqLs.dsc.inc
> > > @@ -101,6 +101,8 @@ [LibraryClasses.common]
> > >    PciExpressLib|MdePkg/Library/BasePciExpressLib/BasePciExpressLib.inf
> > >    PciLib|MdePkg/Library/BasePciLibPciExpress/BasePciLibPciExpress.inf
> > >
> > > +  PlatformPeiLib|Silicon/NXP/Library/PlatformPeiLib/PlatformPeiLib.inf
> > > +
> > >  [LibraryClasses.common.SEC]
> > >    PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
> > >
> > UefiDecompressLib|MdePkg/Library/BaseUefiDecompressLib/BaseUefiDecomp
> > ressLib.inf
> > > @@ -111,7 +113,6 @@ [LibraryClasses.common.SEC]
> > >
> > PrePiHobListPointerLib|ArmPlatformPkg/Library/PrePiHobListPointerLib/PrePiH
> > obListPointerLib.inf
> > >
> > MemoryAllocationLib|EmbeddedPkg/Library/PrePiMemoryAllocationLib/PrePiM
> > emoryAllocationLib.inf
> > >
> > PerformanceLib|MdeModulePkg/Library/PeiPerformanceLib/PeiPerformanceLib
> > .inf
> > > -  PlatformPeiLib|ArmPlatformPkg/PlatformPei/PlatformPeiLib.inf
> > >    MemoryInitPeiLib|Silicon/NXP/Library/MemoryInitPei/MemoryInitPeiLib.inf
> > >
> > >    # 1/123 faster than Stm or Vstm version
> > > --
> > > 2.17.1
> > >