From mboxrd@z Thu Jan 1 00:00:00 1970
Received: from mail-wm1-f65.google.com (mail-wm1-f65.google.com [209.85.128.65])
by mx.groups.io with SMTP id smtpd.web11.8745.1596638685300158562
for <devel@edk2.groups.io>;
Wed, 05 Aug 2020 07:44:45 -0700
Authentication-Results: mx.groups.io;
dkim=pass header.i=@nuviainc-com.20150623.gappssmtp.com header.s=20150623 header.b=xAOfFQFY;
spf=pass (domain: nuviainc.com, ip: 209.85.128.65, mailfrom: leif@nuviainc.com)
Received: by mail-wm1-f65.google.com with SMTP id c80so6067250wme.0
for <devel@edk2.groups.io>; Wed, 05 Aug 2020 07:44:45 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
d=nuviainc-com.20150623.gappssmtp.com; s=20150623;
h=date:from:to:cc:subject:message-id:references:mime-version
:content-disposition:in-reply-to:user-agent;
bh=sesdMwvRgh/yhgW7LHHGcPpAinU8KvfEBOHCslp/NRU=;
b=xAOfFQFYlPB9zp8u2/QzacGoQpRWtRVdqCpJvR9+0qCaOyJWnV2n7xrDaxyNGp5cc7
fP6PdbgaSlsuOr/3/d8I4v7HsCN/82fDaj1fUuYydjbD/6y8zKG4pQ+rdJSK8Z0xOM5H
xY6o+tevNlJ0rh5/sqav8j4IZSI7SgT/oukAmYr1sB1zdjAyYiwCrmAPTsiQSl+QGH3+
6fX1kR3/Wq454ipvonlb3VgqJULe5eQih0KFQrFFNbzmhHXo8ByuN16C0x7deBrljLsT
/f9qujxrBwY9vL32xfZbl5o2pJMHsERrWhyFjJfmLcfklk27HUvQk0VBWRhnO1jqpZ3e
1rEA==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
d=1e100.net; s=20161025;
h=x-gm-message-state:date:from:to:cc:subject:message-id:references
:mime-version:content-disposition:in-reply-to:user-agent;
bh=sesdMwvRgh/yhgW7LHHGcPpAinU8KvfEBOHCslp/NRU=;
b=TeuG4pYY4In3oCVHgf4dNN+2XlMLiyDv51ZB7PfQpGqAf1t4ilQZEMYzYGx+LOslTw
81c0A5gCO5z8VwFe+ecderR6mCGqa9Co/qboss/fVbQqOEvcOWtu82znoqwUny0dne/V
1WkBrXlBiOLS8V5qki0tk/Cvp9JU+2ld/gvqy0ZyoNW3Ammrlrg/+Kmm5HMelCr0uBlC
FhfHQehHsU0PGBD1xSQra0l/7F/6cviWkPAopSnAwY8jHh53Xp75cvm4WXUdq/+4JFvn
J4e9aMz+kjPHqxKq1EoKQ50AxObJbb8Obq/OLsI7J7TVHaqkQIPcH40UCEd5fESA9Yj/
XBDw==
X-Gm-Message-State: AOAM532LYJO9JZTu05kPPK6l9tuA/uawiQq3TN1LSxSid32PKZnD8fxh
0yTpe1IOemeAoRkRvUZVmJ7Phw==
X-Google-Smtp-Source: ABdhPJwU2kkc++32j1oZVIjDByErlrlUkKFDaPrZWtz7FKVTWUxI0fam1VPWitVqtdSfYS6ytVnNKw==
X-Received: by 2002:a7b:c084:: with SMTP id r4mr3515030wmh.23.1596638683666;
Wed, 05 Aug 2020 07:44:43 -0700 (PDT)
Return-Path: <leif@nuviainc.com>
Received: from vanye ([2001:470:1f09:12f0:b26e:bfff:fea9:f1b8])
by smtp.gmail.com with ESMTPSA id 15sm2803928wmo.33.2020.08.05.07.44.42
(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);
Wed, 05 Aug 2020 07:44:43 -0700 (PDT)
Date: Wed, 5 Aug 2020 15:44:41 +0100
From: "Leif Lindholm" <leif@nuviainc.com>
To: Pankaj Bansal <pankaj.bansal@oss.nxp.com>
Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>, devel@edk2.groups.io,
Ard Biesheuvel <ard.biesheuvel@linaro.org>
Subject: Re: [PATCH edk2-platforms 3/3] Silicon/NXP: Add Support for git commit info print
Message-ID: <20200805144441.GO31778@vanye>
References: <20200708051933.8123-1-pankaj.bansal@oss.nxp.com>
<20200708051933.8123-4-pankaj.bansal@oss.nxp.com>
MIME-Version: 1.0
In-Reply-To: <20200708051933.8123-4-pankaj.bansal@oss.nxp.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
On Wed, Jul 08, 2020 at 00:19:33 -0500, Pankaj Bansal wrote:
> From: Pankaj Bansal <pankaj.bansal@nxp.com>
>
> This patch adds the Support for printing the git commit information
> in linux build environment.
>
> Ideal place of retrieving this information should be python script in
> BaseTools.
>
> A Feature request for the same has been created:
> https://bugzilla.tianocore.org/show_bug.cgi?id=2838
>
> Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
> ---
> Silicon/NXP/NxpQoriqLs.dsc.inc | 3 ++
> Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.inf | 5 +++
> Silicon/NXP/Chassis3V2/Library/ChassisLib/ChassisLib.inf | 5 +++
> Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.c | 17 +++++++++
> Silicon/NXP/Chassis3V2/Library/ChassisLib/ChassisLib.c | 17 +++++++++
> Silicon/NXP/set_firmware_ver.sh | 36 ++++++++++++++++++++
> 6 files changed, 83 insertions(+)
>
> diff --git a/Silicon/NXP/NxpQoriqLs.dsc.inc b/Silicon/NXP/NxpQoriqLs.dsc.inc
> index 06ee012c227a..a0762a6ef61d 100644
> --- a/Silicon/NXP/NxpQoriqLs.dsc.inc
> +++ b/Silicon/NXP/NxpQoriqLs.dsc.inc
> @@ -224,6 +224,9 @@ [PcdsDynamicHii.common.DEFAULT]
> gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|L"Timeout"|gEfiGlobalVariableGuid|0x0|10
>
> [PcdsFixedAtBuild.common]
> + !ifdef $(FIRMWARE_VER)
> + gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVersionString|L"$(FIRMWARE_VER)"
> + !endif
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800
> gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange|FALSE
> diff --git a/Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.inf b/Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.inf
> index f5dbd1349dc5..69f884af9e34 100644
> --- a/Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.inf
> +++ b/Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.inf
> @@ -16,6 +16,7 @@ [Defines]
>
> [Packages]
> ArmPkg/ArmPkg.dec
> + MdeModulePkg/MdeModulePkg.dec
> MdePkg/MdePkg.dec
> Silicon/NXP/Chassis2/Chassis2.dec
> Silicon/NXP/NxpQoriqLs.dec
> @@ -24,6 +25,7 @@ [LibraryClasses]
> IoAccessLib
> IoLib
> PcdLib
> + PrintLib
> SerialPortLib
>
> [Sources.common]
> @@ -31,3 +33,6 @@ [Sources.common]
>
> [FeaturePcd]
> gNxpQoriqLsTokenSpaceGuid.PcdDcfgBigEndian
> +
> +[FixedPcd]
> + gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVersionString
> diff --git a/Silicon/NXP/Chassis3V2/Library/ChassisLib/ChassisLib.inf b/Silicon/NXP/Chassis3V2/Library/ChassisLib/ChassisLib.inf
> index 75b68cc4ca2d..632acc52b20a 100644
> --- a/Silicon/NXP/Chassis3V2/Library/ChassisLib/ChassisLib.inf
> +++ b/Silicon/NXP/Chassis3V2/Library/ChassisLib/ChassisLib.inf
> @@ -16,6 +16,7 @@ [Defines]
>
> [Packages]
> ArmPkg/ArmPkg.dec
> + MdeModulePkg/MdeModulePkg.dec
> MdePkg/MdePkg.dec
> Silicon/NXP/Chassis3V2/Chassis3V2.dec
> Silicon/NXP/NxpQoriqLs.dec
> @@ -24,6 +25,7 @@ [LibraryClasses]
> IoAccessLib
> IoLib
> PcdLib
> + PrintLib
> SerialPortLib
>
> [Sources.common]
> @@ -31,3 +33,6 @@ [Sources.common]
>
> [FeaturePcd]
> gNxpQoriqLsTokenSpaceGuid.PcdDcfgBigEndian
> +
> +[FixedPcd]
> + gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVersionString
> diff --git a/Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.c b/Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.c
> 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.
> +
> + 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.
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.
> +
> + 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.
> 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.
> +
> +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.
> +
> +directories=$(echo $PACKAGES_PATH | tr ":" "\n")
Could set IFS=: instead, it would have the same effect.
> +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?
/
Leif
> +
> +echo "build edk2 firmware with -D FIRMWARE_VER=\$FIRMWARE_VER"
> --
> 2.17.1
>