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

["Pankaj Bansal" <pankaj.bansal@nxp.com>]

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