Re: [edk2] [PATCH 01/10] Platform/NXP: Library to provide helper functions.
> -Original Message-
> From: Leif Lindholm [mailto:leif.lindh...@linaro.org]
> Sent: Monday, November 13, 2017 6:44 PM
> To: Meenakshi Aggarwal
> Cc: ard.biesheu...@linaro.org; michael.d.kin...@intel.com; edk2-
> de...@lists.01.org; Udit Kumar ; Varun Sethi
>
> Subject: Re: [PATCH 01/10] Platform/NXP: Library to provide helper
> functions.
>
> On Tue, Nov 07, 2017 at 08:12:07PM +0530, Meenakshi Aggarwal 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
> > ---
> > 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 000..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%7C6e1f439b12994a95303208d52a987203%7C686ea1d3b
> c2b4
> > +
> c6fa92cd99c5c301635%7C0%7C0%7C636461756611410167&sdata=6p6jnHOrzd
> eMT
> > + mSED4ktCktqavpmbIfZpPGGa2TyM0g%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
> > + * 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)); \
> > +}\
> > +)
>
> This function has been lifted verbatim from the Linux kernel, with the inputs
> converted to upper case, at a date before commit 4f5901f5a6724 went in.
> include/linux/kernel.h does not contain an explicit license, so falls under
> the
> Linux kernel default of GPLv2, which is not compatible with the specified BSD
> license.
>
> Please have a _very_ _close_ look at any other potential license infelicities
> in
> the code.
>
OK, I will check all functions in this file.
> > +
> > +/*
> > + * 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 & 0x) + ((W >> 1) & 0x); Res = (Res &
> > + 0x) + ((Res >> 2) & 0x); Res = (Res & 0x0F0F0F0F) +
> > + ((Res >> 4) & 0x0F0F0F0F); Res = (Res & 0x00FF00FF) + ((Res >> 8) &
> > + 0x00FF00FF);
> > +
> > + return (Res & 0x) + ((Res >> 16) & 0x); }
>
> This looks near-identical to an old version from linux lib/hweight.c.
>
> /
> Leif
>
> > +
> > +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)) \
> > +
> > +/**
> > + Find last (most-significant) bit set
> > +
> > + @param X :the word to search
> > +
> > + Note Fls(0) = 0, Fls(1) = 1, Fls(0x8000) = 32.
> > +
> > +**/
> > +STATIC
> > +inline
> > +INT32
> > +GenericFls (
> > + IN INT32 X
> > + )
> > +{
> > + INT32 R = 32;
> > +
> > + if (!X)
> > +return 0;
> > +
> > + if (!(X & 0xu)) {
> > +X <<= 16;
> > +R -= 16;
> >
Re: [edk2] [PATCH 01/10] Platform/NXP: Library to provide helper functions.
> -Original Message-
> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> Sent: Monday, November 13, 2017 5:36 PM
> To: Meenakshi Aggarwal
> Cc: Leif Lindholm ; Kinney, Michael D
> ; edk2-devel@lists.01.org; Udit Kumar
> ; Varun Sethi
> Subject: Re: [PATCH 01/10] Platform/NXP: Library to provide helper
> functions.
>
> On 7 November 2017 at 14:42, Meenakshi Aggarwal
> 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
> > ---
> > 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 000..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 & 0x) + ((W >> 1) & 0x); Res = (Res &
> > + 0x) + ((Res >> 2) & 0x); Res = (Res & 0x0F0F0F0F) +
> > + ((Res >> 4) & 0x0F0F0F0F); Res = (Res & 0x00FF00FF) + ((Res >> 8) &
> > + 0x00FF00FF);
> > +
> > + return (Res & 0x) + ((Res >> 16) & 0x); }
> > +
>
> 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(0x8000) = 32.
> > +
> > +**/
> > +STATIC
> > +inline
> > +INT32
> > +GenericFls (
> > + IN INT32 X
> > + )
> > +{
> > + INT32 R = 32;
> > +
> > + if (!X)
> > +return 0;
> > +
> > + if (!(X & 0xu)) {
> > +X <<= 16;
> > +R -= 16;
> > + }
> > + if (!(X & 0xff00u)) {
> > +X <<= 8;
> > +R -= 8;
> > + }
> > + if (!(X & 0xf000u)) {
> > +X <<= 4;
> > +R -= 4;
> > + }
> > + if (!(X & 0xc000u)) {
> > +X <<= 2;
> > +R -= 2;
> > + }
> > + if (!(X & 0x8000u)) {
> > +X <<= 1;
> > +R -= 1;
> > + }
> > +
> > + return R;
> > +}
> > +
>
> Which patc
Re: [edk2] [PATCH 01/10] Platform/NXP: Library to provide helper functions.
On Tue, Nov 07, 2017 at 08:12:07PM +0530, Meenakshi Aggarwal 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
> ---
> 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 000..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
> + http://opensource.org/licenses/bsd-license.php
> +
> + 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
> + * 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)); \
> +}\
> +)
This function has been lifted verbatim from the Linux kernel, with the
inputs converted to upper case, at a date before commit 4f5901f5a6724
went in. include/linux/kernel.h does not contain an explicit license,
so falls under the Linux kernel default of GPLv2, which is not
compatible with the specified BSD license.
Please have a _very_ _close_ look at any other potential license
infelicities in the code.
> +
> +/*
> + * 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 & 0x) + ((W >> 1) & 0x);
> + Res = (Res & 0x) + ((Res >> 2) & 0x);
> + Res = (Res & 0x0F0F0F0F) + ((Res >> 4) & 0x0F0F0F0F);
> + Res = (Res & 0x00FF00FF) + ((Res >> 8) & 0x00FF00FF);
> +
> + return (Res & 0x) + ((Res >> 16) & 0x);
> +}
This looks near-identical to an old version from linux lib/hweight.c.
/
Leif
> +
> +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)) \
> +
> +/**
> + Find last (most-significant) bit set
> +
> + @param X :the word to search
> +
> + Note Fls(0) = 0, Fls(1) = 1, Fls(0x8000) = 32.
> +
> +**/
> +STATIC
> +inline
> +INT32
> +GenericFls (
> + IN INT32 X
> + )
> +{
> + INT32 R = 32;
> +
> + if (!X)
> +return 0;
> +
> + if (!(X & 0xu)) {
> +X <<= 16;
> +R -= 16;
> + }
> + if (!(X & 0xff00u)) {
> +X <<= 8;
> +R -= 8;
> + }
> + if (!(X & 0xf000u)) {
> +X <<= 4;
> +R -= 4;
> + }
> + if (!(X & 0xc000u)) {
> +X <<= 2;
> +R -= 2;
> + }
> + if (!(X & 0x8000u)) {
> +X <<= 1;
> +R -= 1;
> + }
> +
> + return R;
> +}
include/asm-generic/bitops/fls.h
/
Leif
> +
> +/*
> + * 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
> + );
> +
> +/* Function to convert a frequency to MHz */
> +CHAR8 *StringToMHz (
> + CHAR8 *Buf,
> + UINT32 Size,
> + UINT64 Hz
> + );
> +
> +#endif
> diff --git a/Platform/NXP/Library/UtilsLib/Utils.c
> b/Platform/NXP/Library/UtilsLib/Utils.c
> new file mode 100644
> index 000..4f5a15c
> --- /dev/null
> +++ b/Platform/NXP/Library/UtilsLib/Utils.c
> @@ -0,0 +1,97 @@
> +/** U
Re: [edk2] [PATCH 01/10] Platform/NXP: Library to provide helper functions.
On 7 November 2017 at 14:42, Meenakshi Aggarwal
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
> ---
> 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 000..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
> + http://opensource.org/licenses/bsd-license.php
> +
> + 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
> + * 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?
> +/*
> + * 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 & 0x) + ((W >> 1) & 0x);
> + Res = (Res & 0x) + ((Res >> 2) & 0x);
> + Res = (Res & 0x0F0F0F0F) + ((Res >> 4) & 0x0F0F0F0F);
> + Res = (Res & 0x00FF00FF) + ((Res >> 8) & 0x00FF00FF);
> +
> + return (Res & 0x) + ((Res >> 16) & 0x);
> +}
> +
Please move this into the patch that uses it.
> +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(0x8000) = 32.
> +
> +**/
> +STATIC
> +inline
> +INT32
> +GenericFls (
> + IN INT32 X
> + )
> +{
> + INT32 R = 32;
> +
> + if (!X)
> +return 0;
> +
> + if (!(X & 0xu)) {
> +X <<= 16;
> +R -= 16;
> + }
> + if (!(X & 0xff00u)) {
> +X <<= 8;
> +R -= 8;
> + }
> + if (!(X & 0xf000u)) {
> +X <<= 4;
> +R -= 4;
> + }
> + if (!(X & 0xc000u)) {
> +X <<= 2;
> +R -= 2;
> + }
> + if (!(X & 0x8000u)) {
> +X <<= 1;
> +R -= 1;
> + }
> +
> + return R;
> +}
> +
Which patch uses this function?
> +/*
> + * 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.
> +#endif
> diff --git a/Platform/NXP/Library/UtilsLib/Utils.c
> b/Platform/NXP/Library/UtilsLib/Utils.c
> new file mode 100644
> index 000..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
> + http://opensource.org/licenses/bsd-license.php
> +
> +
