> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> Sent: Monday, November 13, 2017 5:36 PM
> To: Meenakshi Aggarwal <meenakshi.aggar...@nxp.com>
> Cc: Leif Lindholm <leif.lindh...@linaro.org>; Kinney, Michael D
> <michael.d.kin...@intel.com>; edk2-devel@lists.01.org; Udit Kumar
> <udit.ku...@nxp.com>; Varun Sethi <v.se...@nxp.com>
> Subject: Re: [PATCH 01/10] Platform/NXP: Library to provide helper
> functions.
>
> On 7 November 2017 at 14:42, Meenakshi Aggarwal
> <meenakshi.aggar...@nxp.com> wrote:
> > UtilsLib provide helper functions which will be needed by NXP SoCs
> > Library and Drivers.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Meenakshi Aggarwal <meenakshi.aggar...@nxp.com>
> > ---
> > Platform/NXP/Include/Library/Utils.h | 137
> ++++++++++++++++++++++++++++++++
> > Platform/NXP/Library/UtilsLib/Utils.c | 97 ++++++++++++++++++++++
> > Platform/NXP/Library/UtilsLib/Utils.inf | 30 +++++++
> > 3 files changed, 264 insertions(+)
> > create mode 100644 Platform/NXP/Include/Library/Utils.h
> > create mode 100644 Platform/NXP/Library/UtilsLib/Utils.c
> > create mode 100644 Platform/NXP/Library/UtilsLib/Utils.inf
> >
> > diff --git a/Platform/NXP/Include/Library/Utils.h
> > b/Platform/NXP/Include/Library/Utils.h
> > new file mode 100644
> > index 0000000..8920e4d
> > --- /dev/null
> > +++ b/Platform/NXP/Include/Library/Utils.h
> > @@ -0,0 +1,137 @@
> > +/** Utils.h
> > + Header defining the General Purpose Utilities
> > +
> > + Copyright 2017 NXP
> > +
> > + This program and the accompanying materials are licensed and made
> > + available under the terms and conditions of the BSD License which
> > + accompanies this distribution. The full text of the license may be
> > + found at
> > +
> > +
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fop
> > + ensource.org%2Flicenses%2Fbsd-
> license.php&data=02%7C01%7Cmeenakshi.a
> > +
> ggarwal%40nxp.com%7C1545c29d59ad4644c38b08d52a8eee10%7C686ea1d3
> bc2b4
> > +
> c6fa92cd99c5c301635%7C0%7C0%7C636461715737869891&sdata=bpbipNI3z%
> 2Fm
> > + tF2NTn%2BLELYBfv1C38FPfM14aph2%2BRKA%3D&reserved=0
> > +
> > + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"
> > + BASIS, WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND,
> EITHER EXPRESS OR IMPLIED.
> > +
> > +**/
> > +
> > +#ifndef __UTILS_H__
> > +#define __UTILS_H__
> > +
> > +/*
> > + * Divide positive or negative dividend by positive divisor and round
> > + * to closest UINTNeger. Result is undefined for negative divisors
> > +and
>
> Search/replace error
>
OK, will do this
> > + * for negative dividends if the divisor variable type is unsigned.
> > + */
> > +#define DIV_ROUND_CLOSEST(X, Divisor)( \
> > +{ \
> > + typeof(X) __X = X; \
> > + typeof(Divisor) __D = Divisor; \
> > + (((typeof(X))-1) > 0 || \
> > + ((typeof(Divisor))-1) > 0 || (__X) > 0) ? \
> > + (((__X) + ((__D) / 2)) / (__D)) : \
> > + (((__X) - ((__D) / 2)) / (__D)); \
> > +} \
> > +)
> > +
>
> Which patch uses this function?
>
This function is used by StringToMHz() function, which is used by SocLib.
> > +/*
> > + * HammingWeight32: returns the hamming weight (i.e. the number
> > + * of bits set) of a 32-bit word
> > + */
> > +STATIC
> > +inline
> > +UINTN
> > +HammingWeight32 (
> > + IN UINTN W
> > + )
> > +{
> > + UINTN Res;
> > +
> > + Res = (W & 0x55555555) + ((W >> 1) & 0x55555555); Res = (Res &
> > + 0x33333333) + ((Res >> 2) & 0x33333333); Res = (Res & 0x0F0F0F0F) +
> > + ((Res >> 4) & 0x0F0F0F0F); Res = (Res & 0x00FF00FF) + ((Res >> 8) &
> > + 0x00FF00FF);
> > +
> > + return (Res & 0x0000FFFF) + ((Res >> 16) & 0x0000FFFF); }
> > +
>
> Please move this into the patch that uses it.
>
OK
> > +STATIC
> > +inline
> > +UINTN
> > +CpuMaskNext (
> > + IN UINTN Cpu,
> > + IN UINTN Mask
> > + )
> > +{
> > + for (Cpu++; !((1 << Cpu) & Mask); Cpu++)
> > + ;
> > +
> > + return Cpu;
> > +}
> > +
> > +#define ForEachCpu(Iter, Cpu, NumCpus, Mask) \
> > + for (Iter = 0, Cpu = CpuMaskNext(-1, Mask); \
> > + Iter < NumCpus; \
> > + Iter++, Cpu = CpuMaskNext(Cpu, Mask)) \
> > +
>
> Same here
>
> > +/**
> > + Find last (most-significant) bit set
> > +
> > + @param X : the word to search
> > +
> > + Note Fls(0) = 0, Fls(1) = 1, Fls(0x80000000) = 32.
> > +
> > +**/
> > +STATIC
> > +inline
> > +INT32
> > +GenericFls (
> > + IN INT32 X
> > + )
> > +{
> > + INT32 R = 32;
> > +
> > + if (!X)
> > + return 0;
> > +
> > + if (!(X & 0xffff0000u)) {
> > + X <<= 16;
> > + R -= 16;
> > + }
> > + if (!(X & 0xff000000u)) {
> > + X <<= 8;
> > + R -= 8;
> > + }
> > + if (!(X & 0xf0000000u)) {
> > + X <<= 4;
> > + R -= 4;
> > + }
> > + if (!(X & 0xc0000000u)) {
> > + X <<= 2;
> > + R -= 2;
> > + }
> > + if (!(X & 0x80000000u)) {
> > + X <<= 1;
> > + R -= 1;
> > + }
> > +
> > + return R;
> > +}
> > +
>
> Which patch uses this function?
>
This function will be used by our next set of patches.
I will add this in the next set of patch which are using this function and
remove from here.
> > +/*
> > + * PrINT32 Sizes As "Xxx KiB", "Xxx.Y KiB", "Xxx MiB", "Xxx.Y MiB",
>
> Search/replace error.
>
> > + * Xxx GiB, Xxx.Y GiB, Etc As Needed; Allow for Optional Trailing
> > +String
> > + * (Like "\n")
> > + */
> > +VOID
> > +PrintSize (
> > + IN UINT64 Size,
> > + IN CONST INT8 *S
> > + );
> > +
> > +/* Function to convert a frequency to MHz */
> > +CHAR8 *StringToMHz (
> > + CHAR8 *Buf,
> > + UINT32 Size,
> > + UINT64 Hz
> > + );
> > +
>
> Please move these into the patch that uses them.
>
OK
> > +#endif
> > diff --git a/Platform/NXP/Library/UtilsLib/Utils.c
> > b/Platform/NXP/Library/UtilsLib/Utils.c
> > new file mode 100644
> > index 0000000..4f5a15c
> > --- /dev/null
> > +++ b/Platform/NXP/Library/UtilsLib/Utils.c
> > @@ -0,0 +1,97 @@
> > +/** Utils.c
> > +
> > + Copyright 2017 NXP
> > +
> > + This program and the accompanying materials are licensed and made
> > + available under the terms and conditions of the BSD License which
> > + accompanies this distribution. The full text of the license may be
> > + found at
> > +
> > +
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fop
> > + ensource.org%2Flicenses%2Fbsd-
> license.php&data=02%7C01%7Cmeenakshi.a
> > +
> ggarwal%40nxp.com%7C1545c29d59ad4644c38b08d52a8eee10%7C686ea1d3
> bc2b4
> > +
> c6fa92cd99c5c301635%7C0%7C0%7C636461715737869891&sdata=bpbipNI3z%
> 2Fm
> > + tF2NTn%2BLELYBfv1C38FPfM14aph2%2BRKA%3D&reserved=0
> > +
> > + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"
> > + BASIS, WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND,
> EITHER EXPRESS OR IMPLIED.
> > +
> > +**/
> > +
> > +#include <Library/DebugLib.h>
> > +#include <Library/PrintLib.h>
> > +#include <Library/Utils.h>
> > +
> > +/* Function to convert a frequency to MHz */
> > +CHAR8 *
> > +StringToMHz (
> > + IN CHAR8 *Buf,
> > + IN UINT32 Size,
> > + IN UINT64 Hz
> > + )
> > +{
> > + UINT64 L;
> > + UINT64 M;
> > + UINT64 N;
> > +
> > + N = DIV_ROUND_CLOSEST(Hz, 1000) / 1000L; L = AsciiSPrint (Buf,
> > + Size, "%ld", N);
> > +
> > + Hz -= N * 1000000L;
> > + M = DIV_ROUND_CLOSEST(Hz, 1000L);
> > +
> > + if (M != 0) {
> > + AsciiSPrint (Buf + L, Size, ".%03ld", M); }
> > +
> > + return (Buf);
> > +}
> > +
> > +/*
> > + * PrINT32 Sizes As "Xxx KiB", "Xxx.Y KiB", "Xxx MiB", "Xxx.Y MiB",
> > + * Xxx GiB, Xxx.Y GiB, Etc As Needed; Allow for Optional Trailing
> > +String
> > + * (Like "\n")
> > + */
> > +VOID
> > +PrintSize (
> > + IN UINT64 Size,
> > + IN CONST INT8 *S
> > + )
> > +{
> > + UINT64 M;
> > + UINT64 N;
> > + UINT64 F;
> > + UINT64 D;
> > + CHAR8 C;
> > + UINT32 I;
> > + INT8 Names[6] = {'E', 'P', 'T', 'G', 'M', 'K'};
> > +
> > + M = 0;
> > + D = 10 * ARRAY_SIZE(Names);
> > + C = 0;
> > +
> > + for (I = 0; I < ARRAY_SIZE(Names); I++, D -= 10) {
> > + if (Size >> D) {
> > + C = Names[I];
> > + break;
> > + }
> > + }
> > +
> > + if (!C) {
> > + DEBUG((DEBUG_ERROR, "%Ld Bytes,\n %a", Size, S));
> > + return;
> > + }
> > +
> > + N = Size >> D;
> > + F = Size & ((1ULL << D) - 1);
> > +
> > + /* if There'S A Remainder, Deal With It */ if (F) {
> > + M = (10ULL * F + (1ULL << (D - 1))) >> D;
> > +
> > + if (M >= 10) {
> > + M -= 10;
> > + N += 1;
> > + }
> > + }
> > +
> > + DEBUG((DEBUG_ERROR, "%Ld", N));
> > + if (M) {
> > + DEBUG((DEBUG_ERROR, ".%Ld", M));
> > + }
> > + DEBUG((DEBUG_ERROR, " %ciB, %a ", C, S)); }
>
> Why DEBUG_ERROR ? Don't use that for DEBUG code, and in general, adding
> DEBUG () to a library should be done with care. (It may break
> DXE_RUNTIME_DRIVER modules called by the OS)
>
>
We are not using this function in DXE_RUNTIME_DRIVER.
We used DEBUG_ERROR because we need this information in both DEBUG and RELEASE
build.
Please suggest alternate way to get this information in RELEASE build as well
> > diff --git a/Platform/NXP/Library/UtilsLib/Utils.inf
> > b/Platform/NXP/Library/UtilsLib/Utils.inf
> > new file mode 100644
> > index 0000000..9901445
> > --- /dev/null
> > +++ b/Platform/NXP/Library/UtilsLib/Utils.inf
> > @@ -0,0 +1,30 @@
> > +# @Utils.inf
> > +
> > +# Copyright 2017 NXP
> > +#
> > +# This program and the accompanying materials # are licensed and
> > +made available under the terms and conditions of the BSD License #
> > +which accompanies this distribution. The full text of the license
> > +may be found at #
> >
> +https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fop
> e
> > +nsource.org%2Flicenses%2Fbsd-
> license.php&data=02%7C01%7Cmeenakshi.agg
> >
> +arwal%40nxp.com%7C1545c29d59ad4644c38b08d52a8eee10%7C686ea1d3b
> c2b4c6f
> >
> +a92cd99c5c301635%7C0%7C0%7C636461715737869891&sdata=bpbipNI3z%2
> FmtF2N
> > +Tn%2BLELYBfv1C38FPfM14aph2%2BRKA%3D&reserved=0
> > +#
> > +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"
> > +BASIS, # WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND,
> EITHER EXPRESS OR IMPLIED.
> > +#
> > +
> > +[Defines]
> > + INF_VERSION = 0x00010005
>
> Should be 0x0001001A
OK
>
> > + BASE_NAME = UtilsLib
> > + FILE_GUID = 0985d4e8-5a41-40cf-ad12-2ad5d35e817f
> > + MODULE_TYPE = BASE
> > + VERSION_STRING = 1.0
> > + LIBRARY_CLASS = UtilsLib
> > +
> > +[Packages]
> > + MdePkg/MdePkg.dec
> > + edk2-platforms/Platform/NXP/NxpQoriqLs.dec
>
> Please drop the edk2-platforms prefix, and add it to your PACKAGES_PATH
>
OK, we will look into this.
> > +
> > +[LibraryClasses]
> > + PrintLib
> > +
> > +[Sources.common]
> > + Utils.c
> > --
> > 1.9.1
> >
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
