Re: [PATCH v3 1/1] ArmPlatformPkg/PrePeiCore: Print the firmware version early in boot

["Leif Lindholm" <quic_llindhol@quicinc.com>]

On Tue, Oct 25, 2022 at 07:46:13 -0600, Rebecca Cran wrote:
> Thanks, that's a good change.
> 
> Reviewed-by: Rebecca Cran <rebecca@quicinc.com>

(I won't add an R-b from you on your own patch.)

Reviewed-by: Leif Lindholm <quic_llindhol@quicinc.com>

After fixing a couple of style issues in my diff below:
Pushed as 56035d1c8b25.

Thanks!

/
    Leif

> On 10/25/22 05:41, Leif Lindholm wrote:
> > On Tue, Oct 11, 2022 at 14:59:52 -0600, Rebecca Cran wrote:
> > > Copy code from PrePi to PrePeiCore that prints the firmware version
> > > and build date early in the boot process.
> > I'm good with this, but I'd prefer to break the printout into a helper
> > function in order to reduce clutter in CEntryPoint.
> > 
> > i.e. fold in
> > 
> > ---
> > diff --git a/ArmPlatformPkg/PrePeiCore/PrePeiCore.c
> > b/ArmPlatformPkg/PrePeiCore/PrePeiCore.c
> > index 674f4f7df498..225e22f75c23 100644
> > --- a/ArmPlatformPkg/PrePeiCore/PrePeiCore.c
> > +++ b/ArmPlatformPkg/PrePeiCore/PrePeiCore.c
> > @@ -54,15 +54,32 @@ CreatePpiList (
> >     *PpiListSize = sizeof (gCommonPpiTable) + PlatformPpiListSize;
> >      }
> > 
> > +STATIC
> > +VOID
> > +PrintFirmwareVersion (
> > +  VOID
> > +  )
> > +{
> > +  CHAR8  Buffer[100];
> > +  UINTN  CharCount;
> > +
> > +  CharCount = AsciiSPrint (
> > +                Buffer,
> > +                sizeof (Buffer),
> > +                "UEFI firmware (version %s built at %a on %a)\n\r",
> > +                (CHAR16 *)PcdGetPtr (PcdFirmwareVersionString),
> > +                __TIME__,
> > +                __DATE__
> > +                );
> > +  SerialPortWrite ((UINT8 *)Buffer, CharCount);
> > +}
> > +
> >   VOID
> >   CEntryPoint (
> >     IN  UINTN                     MpId,
> >     IN  EFI_PEI_CORE_ENTRY_POINT  PeiCoreEntryPoint
> >     )
> >   {
> > -  CHAR8  Buffer[100];
> > -  UINTN  CharCount;
> > -
> >     if (!ArmMmuEnabled ()) {
> >       // Data Cache enabled on Primary core when MMU is enabled.
> >       ArmDisableDataCache ();
> > @@ -100,15 +117,8 @@ CEntryPoint (
> >       // Invoke "ProcessLibraryConstructorList" to have all library constructors
> >       // called.
> >       ProcessLibraryConstructorList ();
> > -    CharCount = AsciiSPrint (
> > -                  Buffer,
> > -                  sizeof (Buffer),
> > -                  "UEFI firmware (version %s built at %a on %a)\n\r",
> > -                  (CHAR16 *)PcdGetPtr (PcdFirmwareVersionString),
> > -                  __TIME__,
> > -                  __DATE__
> > -                  );
> > -    SerialPortWrite ((UINT8 *)Buffer, CharCount);
> > +
> > +    PrintFirmwareVersion();
> > 
> >       // Initialize the Debug Agent for Source Level Debugging
> >            InitializeDebugAgent (DEBUG_AGENT_INIT_POSTMEM_SEC, NULL,
> >            NULL);
> > ---
> > 
> > If you're happy with that, I can apply and push.
> > 
> > /
> >      Leif
> > 
> > > Signed-off-by: Rebecca Cran <rebecca@quicinc.com>
> > > Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>
> > > Tested-by: Oliver Steffen <osteffen@redhat.com>
> > > ---
> > >   ArmPlatformPkg/PrePeiCore/PrePeiCoreMPCore.inf  |  3 +++
> > >   ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf |  3 +++
> > >   ArmPlatformPkg/PrePeiCore/PrePeiCore.c          | 14 ++++++++++++++
> > >   3 files changed, 20 insertions(+)
> > > 
> > > diff --git a/ArmPlatformPkg/PrePeiCore/PrePeiCoreMPCore.inf b/ArmPlatformPkg/PrePeiCore/PrePeiCoreMPCore.inf
> > > index a5b4722459d1..4a3112b58dcb 100644
> > > --- a/ArmPlatformPkg/PrePeiCore/PrePeiCoreMPCore.inf
> > > +++ b/ArmPlatformPkg/PrePeiCore/PrePeiCoreMPCore.inf
> > > @@ -54,6 +54,9 @@ [Ppis]
> > >     gEfiTemporaryRamSupportPpiGuid
> > >     gArmMpCoreInfoPpiGuid
> > > +[Pcd]
> > > +  gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVersionString
> > > +
> > >   [FeaturePcd]
> > >     gArmPlatformTokenSpaceGuid.PcdSendSgiToBringUpSecondaryCores
> > > diff --git a/ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf b/ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf
> > > index 466a2b01c384..ab5bf1dac2d8 100644
> > > --- a/ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf
> > > +++ b/ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf
> > > @@ -52,6 +52,9 @@ [LibraryClasses]
> > >   [Ppis]
> > >     gEfiTemporaryRamSupportPpiGuid
> > > +[Pcd]
> > > +  gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVersionString
> > > +
> > >   [FeaturePcd]
> > >     gArmPlatformTokenSpaceGuid.PcdSendSgiToBringUpSecondaryCores
> > > diff --git a/ArmPlatformPkg/PrePeiCore/PrePeiCore.c b/ArmPlatformPkg/PrePeiCore/PrePeiCore.c
> > > index 9c4b25df953d..1d4f6969b660 100644
> > > --- a/ArmPlatformPkg/PrePeiCore/PrePeiCore.c
> > > +++ b/ArmPlatformPkg/PrePeiCore/PrePeiCore.c
> > > @@ -11,6 +11,8 @@
> > >   #include <Library/CacheMaintenanceLib.h>
> > >   #include <Library/DebugAgentLib.h>
> > >   #include <Library/ArmLib.h>
> > > +#include <Library/PrintLib.h>
> > > +#include <Library/SerialPortLib.h>
> > >   #include "PrePeiCore.h"
> > > @@ -58,6 +60,9 @@ CEntryPoint (
> > >     IN  EFI_PEI_CORE_ENTRY_POINT  PeiCoreEntryPoint
> > >     )
> > >   {
> > > +  CHAR8  Buffer[100];
> > > +  UINTN  CharCount;
> > > +
> > >     // Data Cache enabled on Primary core when MMU is enabled.
> > >     ArmDisableDataCache ();
> > >     // Invalidate instruction cache
> > > @@ -93,6 +98,15 @@ CEntryPoint (
> > >       // Invoke "ProcessLibraryConstructorList" to have all library constructors
> > >       // called.
> > >       ProcessLibraryConstructorList ();
> > > +    CharCount = AsciiSPrint (
> > > +                  Buffer,
> > > +                  sizeof (Buffer),
> > > +                  "UEFI firmware (version %s built at %a on %a)\n\r",
> > > +                  (CHAR16 *)PcdGetPtr (PcdFirmwareVersionString),
> > > +                  __TIME__,
> > > +                  __DATE__
> > > +                  );
> > > +    SerialPortWrite ((UINT8 *)Buffer, CharCount);
> > >       // Initialize the Debug Agent for Source Level Debugging
> > >       InitializeDebugAgent (DEBUG_AGENT_INIT_POSTMEM_SEC, NULL, NULL);
> > > -- 
> > > 2.30.2
> > > 
>