1. Patchew
  2. EDK2
  3. [edk2-devel] [PATCH v2 00/28] Add PEI phase to LS1043ARDB Platform
  4. View patch

[edk2-devel] [PATCH v2 21/28] Slicon/NXP: Add PlatformPei Lib

Pankaj Bansal posted 28 patches 5 years, 10 months ago
There is a newer version of this series
[edk2-devel] [PATCH v2 21/28] Slicon/NXP: Add PlatformPei Lib
Posted by Pankaj Bansal 5 years, 10 months ago 
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>
+
+#define XPRINT(x) PRINT(x)
+#define PRINT(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)));
+
+  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
+  BASE_NAME                      = ArmPlatformPeiLib
+  FILE_GUID                      = 49d37060-70b5-11e0-aa2d-0002a5d5c51b
+  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)"
+
+[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/BaseUefiDecompressLib.inf
@@ -111,7 +113,6 @@ [LibraryClasses.common.SEC]
   PrePiHobListPointerLib|ArmPlatformPkg/Library/PrePiHobListPointerLib/PrePiHobListPointerLib.inf
   MemoryAllocationLib|EmbeddedPkg/Library/PrePiMemoryAllocationLib/PrePiMemoryAllocationLib.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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#56015): https://edk2.groups.io/g/devel/message/56015
Mute This Topic: https://groups.io/mt/72077454/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2 21/28] Slicon/NXP: Add PlatformPei Lib
Posted by Leif Lindholm 5 years, 10 months ago 
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.

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

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

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

/
    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/BaseUefiDecompressLib.inf
> @@ -111,7 +113,6 @@ [LibraryClasses.common.SEC]
>    PrePiHobListPointerLib|ArmPlatformPkg/Library/PrePiHobListPointerLib/PrePiHobListPointerLib.inf
>    MemoryAllocationLib|EmbeddedPkg/Library/PrePiMemoryAllocationLib/PrePiMemoryAllocationLib.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
> 

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#56857): https://edk2.groups.io/g/devel/message/56857
Mute This Topic: https://groups.io/mt/72077454/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2 21/28] Slicon/NXP: Add PlatformPei Lib
Posted by Pankaj Bansal 5 years, 10 months ago 

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

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#56989): https://edk2.groups.io/g/devel/message/56989
Mute This Topic: https://groups.io/mt/72077454/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2 21/28] Slicon/NXP: Add PlatformPei Lib
Posted by Leif Lindholm 5 years, 10 months ago 
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
> > >

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#57023): https://edk2.groups.io/g/devel/message/57023
Mute This Topic: https://groups.io/mt/72077454/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2 21/28] Slicon/NXP: Add PlatformPei Lib
Posted by Pankaj Bansal 5 years, 10 months ago 
> 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.

Not just the commit info. The plan is to print core/cluster info also from
this peim. As I have already stated in PATCH 10/28, that some print information would
be put into common code instead of SOC specific code.

In fact the further plan is to fixup device tree in this library before DXE phase starts.
So, this fork is needed.

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

OK, I can use this for printing git commit info.

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

I agree. Actually IMO, best place would be python script to do this.
I don't have much experience with python script.
That is why I made changes in shell script.

> (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].

As I said, that version print is just one of the many things that we wish to
do in platform PEIM.

> 
> /
>     Leif
> 
> On Mon, Apr 06, 2020 at 14:53:02 +0000, Pankaj Bansal (OSS) wrote:


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#57027): https://edk2.groups.io/g/devel/message/57027
Mute This Topic: https://groups.io/mt/72077454/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2 21/28] Slicon/NXP: Add PlatformPei Lib
Posted by Leif Lindholm 5 years, 10 months ago 
On Tue, Apr 07, 2020 at 17:00:50 +0000, Pankaj Bansal wrote:
> > 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.
> 
> Not just the commit info. The plan is to print core/cluster info also from
> this peim. As I have already stated in PATCH 10/28, that some print information would
> be put into common code instead of SOC specific code.

Sure, but there are 11 patches in between. There is no way I am going
to recall what you stated was going to happen to something as basic as
a DEBUG printout by the time I get here. And that comment stated "a
common PEIM", not "a forked PlatformPeiLib containing only the
printouts".

> In fact the further plan is to fixup device tree in this library
> before DXE phase starts. So, this fork is needed.

Not at this time. See below.

> > 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.
> > 
> 
> OK, I can use this for printing git commit info.
> 
> > 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.
> 
> I agree. Actually IMO, best place would be python script to do this.
> I don't have much experience with python script.
> That is why I made changes in shell script.

Yes, but edk2 is a cross-os project, so Python is needed.

To clarify, I think there shouldn't be a separate script to call, but
that this should be integrated into the build command itself, and
accessible as other predefined variables (like $ARCH).
If you would like to raise a feature request in
https://bugzilla.tianocore.org/enter_bug.cgi?product=EDK2, that would
be a good start even if you're not able to do the work yourself.

> > (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].
> 
> As I said, that version print is just one of the many things that we wish to
> do in platform PEIM.

OK, but it makes no sense to split something out *now* to be given
actual functionality at some later date. Please leave it out for now,
either completely or moving the printout to some standalone Peim, and
add it in when there is actual content.
At that point, please give it a unique FILE_GUID.

/
    Leif

> > 
> > /
> >     Leif
> > 
> > On Mon, Apr 06, 2020 at 14:53:02 +0000, Pankaj Bansal (OSS) wrote:
> 
> 
> 
> 

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#57066): https://edk2.groups.io/g/devel/message/57066
Mute This Topic: https://groups.io/mt/72077454/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.