Re: [PATCH edk2-platforms 3/3] Silicon/NXP: Add Support for git commit info print

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

> > index 91b19f832f00..bc782e7e3873 100644
> > --- a/Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.c
> > +++ b/Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.c
> > @@ -12,6 +12,7 @@
> >  #include <Library/IoAccessLib.h>
> >  #include <Library/IoLib.h>
> >  #include <Library/PcdLib.h>
> > +#include <Library/PrintLib.h>
> >  #include <Library/SerialPortLib.h>
> >
> >  /**
> > @@ -89,10 +90,26 @@ ChassisInit (
> >    VOID
> >    )
> >  {
> > +  CHAR8                         Buffer[100];
> > +  UINTN                         CharCount;
> > +
> >    //
> >    // Early init serial Port to get board information.
> >    //
> >    SerialPortInitialize ();
> >
> > +  CharCount = AsciiSPrint (
> > +                Buffer, sizeof (Buffer),
> > +                "UEFI firmware built at %a on %a. version:\n\r",
> > +                __TIME__, __DATE__
> > +              );
> > +  SerialPortWrite ((UINT8 *) Buffer, CharCount);
> 
> Drop space before Buffer.

OK.

> 
> > +
> > +  CharCount = AsciiSPrint (
> > +                Buffer, sizeof (Buffer), "%s\n\r",
> > +                (CHAR16 *)PcdGetPtr (PcdFirmwareVersionString)
> > +              );
> > +  SerialPortWrite ((UINT8 *) Buffer, CharCount);
> > +
> >    SmmuInit ();
> >  }
> > diff --git a/Silicon/NXP/Chassis3V2/Library/ChassisLib/ChassisLib.c
> b/Silicon/NXP/Chassis3V2/Library/ChassisLib/ChassisLib.c
> > index 30f8f945b233..6d546f4754f9 100644
> > --- a/Silicon/NXP/Chassis3V2/Library/ChassisLib/ChassisLib.c
> > +++ b/Silicon/NXP/Chassis3V2/Library/ChassisLib/ChassisLib.c
> > @@ -12,6 +12,7 @@
> >  #include <Library/IoAccessLib.h>
> >  #include <Library/IoLib.h>
> >  #include <Library/PcdLib.h>
> > +#include <Library/PrintLib.h>
> >  #include <Library/SerialPortLib.h>
> >
> >  /**
> > @@ -64,8 +65,24 @@ ChassisInit (
> >    VOID
> >    )
> >  {
> > +  CHAR8                         Buffer[100];
> > +  UINTN                         CharCount;
> > +
> >    //
> >    // Early init serial Port to get board information.
> >    //
> >    SerialPortInitialize ();
> > +
> > +  CharCount = AsciiSPrint (
> > +                Buffer, sizeof (Buffer),
> > +                "UEFI firmware built at %a on %a. version:\n\r",
> > +                __TIME__, __DATE__
> > +              );
> > +  SerialPortWrite ((UINT8 *) Buffer, CharCount);
> 
> Drop space before Buffer.

OK.

> 
> I will note here that you are adding identical content to two
> different ChassisLib implementations.
> 
> This is a strong indicator that the abstraction level is currently
> incorrect and should be revisited.
> 

I am preparing the patches to get the core/cluster info in NXP SOCs.
I will move the version print in that code.

> > +
> > +  CharCount = AsciiSPrint (
> > +                Buffer, sizeof (Buffer), "%s\n\r",
> > +                (CHAR16 *)PcdGetPtr (PcdFirmwareVersionString)
> > +              );
> > +  SerialPortWrite ((UINT8 *) Buffer, CharCount);
> >  }
> > diff --git a/Silicon/NXP/set_firmware_ver.sh
> b/Silicon/NXP/set_firmware_ver.sh
> 
> This script doesn't *set* anything (which is good, since I would have
> rejected that outright), so the name should reflect the actual function.

it *does* set the FIRMWARE_VER environment variable.
FIRMWARE_VER is just like WORKSPACE and PACKAGES_PATH.

> 
> > new file mode 100755
> > index 000000000000..ba2336ad23dc
> > --- /dev/null
> > +++ b/Silicon/NXP/set_firmware_ver.sh
> > @@ -0,0 +1,36 @@
> > +#!/bin/sh
> > +#
> > +# Copyright 2020 NXP
> > +#
> > +# SPDX-License-Identifier: BSD-2-Clause-Patent
> > +#
> > +# parse PACKAGES_PATH and set FIRMWARE_VER based on that
> 
> This too should reflect actual function.

see explanation above.

> 
> > +
> > +get_git_version()
> > +{
> > +  command -v git>/dev/null 2>&1
> > +  if [ $? -eq 0 ] && [ -n "$1" ]
> > +  then
> > +    head_or_tag=`git -C $1 describe --tag --dirty --broken --always 2>/dev/null`
> > +    printf $head_or_tag
> > +  fi
> > +}
> > +
> > +if [ -n "$FIRMWARE_VER" ]; then
> > +  echo "Warning FIRMWARE_VER=$FIRMWARE_VER is already set"
> > +  echo "Please retry after 'unset FIRMWARE_VER' command"
> > +fi
> 
> Just do
> FIRMWARE_VER=
> before the first code in the script.

OK.

> 
> > +
> > +directories=$(echo $PACKAGES_PATH | tr ":" "\n")
> 
> Could set IFS=: instead, it would have the same effect.

Good suggestion. will modify.

> 
> > +for dir in $directories; do
> > +  if [ -n "$FIRMWARE_VER" ]; then
> > +    FIRMWARE_VER="$FIRMWARE_VER;$(basename $dir):$(get_git_version
> $dir)"
> > +  else
> > +    FIRMWARE_VER=$(basename $dir):$(get_git_version $dir)
> > +  fi
> > +done
> > +
> > +echo "FIRMWARE_VER=$FIRMWARE_VER"
> > +export FIRMWARE_VER=$FIRMWARE_VER
> 
> What is this line supposed to achieve?

It would set the FIRMWARE_VER environment variable when "set_firmware_ver.sh" script is executed.
After that user can build platform firmware with -D FIRMWARE_VER=$ FIRMWARE_VER flag.
I have put this suggestion for users in "set_firmware_ver.sh" script.

> 
> /
>     Leif
> 
> > +
> > +echo "build edk2 firmware with -D FIRMWARE_VER=\$FIRMWARE_VER"
> > --
> > 2.17.1
> >