Re: [edk2-devel] [PATCH] IntelFsp2Pkg: Adopt FSP 2.3 specification.

2021-10-03 Thread Chiu, Chasel


Thanks Nate! I will remove it.

> -Original Message-
> From: Desimone, Nathaniel L 
> Sent: Saturday, October 2, 2021 6:15 AM
> To: Chiu, Chasel ; devel@edk2.groups.io
> Cc: Ma, Maurice ; Zeng, Star 
> Subject: RE: [PATCH] IntelFsp2Pkg: Adopt FSP 2.3 specification.
> 
> Hi Chasel,
> 
> My only feedback is let's not add the EDK1 style GUID definition to
> FspNonVolatileStorageHob2.h. It is redundant with the EDK2 style definition.
> Please see inline.
> 
> With that change...
> 
> Reviewed-by: Nate DeSimone 
> 
> > -Original Message-
> > From: Chiu, Chasel 
> > Sent: Friday, October 1, 2021 1:29 AM
> > To: devel@edk2.groups.io
> > Cc: Chiu, Chasel ; Ma, Maurice
> > ; Desimone, Nathaniel L
> > ; Zeng, Star 
> > Subject: [PATCH] IntelFsp2Pkg: Adopt FSP 2.3 specification.
> >
> > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3674
> >
> > Add ExtendedImageRevision in FSP_INFO_HEADER structure, also add
> > FSP_NON_VOLATILE_STORAGE_HOB2 header.
> >
> > Cc: Maurice Ma 
> > Cc: Nate DeSimone 
> > Cc: Star Zeng 
> > Signed-off-by: Chasel Chiu 
> > ---
> >  IntelFsp2Pkg/Include/Guid/FspHeaderFile.h | 33
> > -
> >  IntelFsp2Pkg/Include/Guid/FspNonVolatileStorageHob2.h | 27
> > +++
> >  IntelFsp2Pkg/IntelFsp2Pkg.dec |  1 +
> >  3 files changed, 60 insertions(+), 1 deletion(-)
> >
> > diff --git a/IntelFsp2Pkg/Include/Guid/FspHeaderFile.h
> > b/IntelFsp2Pkg/Include/Guid/FspHeaderFile.h
> > index 3474bac1de..11e0ede65b 100644
> > --- a/IntelFsp2Pkg/Include/Guid/FspHeaderFile.h
> > +++ b/IntelFsp2Pkg/Include/Guid/FspHeaderFile.h
> > @@ -2,7 +2,7 @@
> >Intel FSP Header File definition from Intel Firmware Support
> > Package External
> >
> >Architecture Specification v2.0 and above.
> >
> >
> >
> > -  Copyright (c) 2014 - 2020, Intel Corporation. All rights
> > reserved.
> >
> > +  Copyright (c) 2014 - 2021, Intel Corporation. All rights
> > + reserved.
> >
> >SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> >
> >
> >  **/
> >
> > @@ -44,14 +44,28 @@ typedef struct {
> >UINT8   Reserved1[2];
> >
> >///
> >
> >/// Byte 0x0A: Indicates compliance with a revision of this
> > specification in the BCD format.
> >
> > +  ///For revision v2.3 the value will be 0x23.
> >
> >///
> >
> >UINT8   SpecVersion;
> >
> >///
> >
> >/// Byte 0x0B: Revision of the FSP Information Header.
> >
> > +  ///The Current value for this field is 0x6.
> >
> >///
> >
> >UINT8   HeaderRevision;
> >
> >///
> >
> >/// Byte 0x0C: Revision of the FSP binary.
> >
> > +  ///Major.Minor.Revision.Build
> >
> > +  ///If FSP HeaderRevision is <= 5, the ImageRevision can be 
> > decoded
> > as follows:
> >
> > +  ///   7 : 0  - Build Number
> >
> > +  ///  15 : 8  - Revision
> >
> > +  ///  23 : 16 - Minor Version
> >
> > +  ///  31 : 24 - Major Version
> >
> > +  ///If FSP HeaderRevision is >= 6, ImageRevision specifies 
> > the low-
> > order bytes of the build number and revision
> >
> > +  ///while ExtendedImageRevision specifies the high-order 
> > bytes of
> > the build number and revision.
> >
> > +  ///   7 : 0  - Low Byte of Build Number
> >
> > +  ///  15 : 8  - Low Byte of Revision
> >
> > +  ///  23 : 16 - Minor Version
> >
> > +  ///  31 : 24 - Major Version
> >
> >///
> >
> >UINT32  ImageRevision;
> >
> >///
> >
> > @@ -116,6 +130,23 @@ typedef struct {
> >///If the value is set to 0x, then this API is not 
> > available in
> > this component.
> >
> >///
> >
> >UINT32  FspMultiPhaseSiInitEntryOffset;
> >
> > +  ///
> >
> > +  /// Byte 0x4C: Extended revision of the FSP binary.
> >
> > +  ///This value is only valid if FSP HeaderRevision is >= 6.
> >
> > +  ///ExtendedImageRevision specifies the high-order byte of the
> > revision and build number in the FSP binary revision.
> >
> > +  ///   7 : 0 - High Byte of Build Number
> >
> > +  ///  15 : 8 - High Byte of Revision
> >
> > +  ///The FSP binary build number can be decoded as follows:
> >
> > +  ///Build Number = (ExtendedImageRevision[7:0] << 8) |
> > ImageRevision[7:0]
> >
> > +  ///Revision = (ExtendedImageRevision[15:8] << 8) |
> > ImageRevision[15:8]
> >
> > +  ///Minor Version = ImageRevision[23:16]
> >
> > +  ///Major Version = ImageRevision[31:24]
> >
> > +  ///
> >
> > +  UINT16  ExtendedImageRevision;
> >
> > +  ///
> >
> > +  /// Byte 0x4E: Reserved4.
> >
> > +  ///
> >
> > +  UINT16  Reserved4;
> >
> >  } FSP_INFO_HEADER;
> >
> >
> >
> >  ///
> >
> > diff --git a/IntelFsp2Pkg/Include/Guid/FspNonVolatileStorageHob2.h
> > b/IntelFsp2Pkg/Include/Guid/FspNonVolatileStorageHob2.h
> > new file mode 100644
> 

Re: [edk2-devel] [PATCH] IntelFsp2Pkg: Adopt FSP 2.3 specification.

2021-10-01 Thread Nate DeSimone
Hi Chasel,

My only feedback is let's not add the EDK1 style GUID definition to 
FspNonVolatileStorageHob2.h. It is redundant with the EDK2 style definition. 
Please see inline.

With that change...

Reviewed-by: Nate DeSimone 

> -Original Message-
> From: Chiu, Chasel 
> Sent: Friday, October 1, 2021 1:29 AM
> To: devel@edk2.groups.io
> Cc: Chiu, Chasel ; Ma, Maurice
> ; Desimone, Nathaniel L
> ; Zeng, Star 
> Subject: [PATCH] IntelFsp2Pkg: Adopt FSP 2.3 specification.
> 
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3674
> 
> Add ExtendedImageRevision in FSP_INFO_HEADER structure, also add
> FSP_NON_VOLATILE_STORAGE_HOB2 header.
> 
> Cc: Maurice Ma 
> Cc: Nate DeSimone 
> Cc: Star Zeng 
> Signed-off-by: Chasel Chiu 
> ---
>  IntelFsp2Pkg/Include/Guid/FspHeaderFile.h | 33
> -
>  IntelFsp2Pkg/Include/Guid/FspNonVolatileStorageHob2.h | 27
> +++
>  IntelFsp2Pkg/IntelFsp2Pkg.dec |  1 +
>  3 files changed, 60 insertions(+), 1 deletion(-)
> 
> diff --git a/IntelFsp2Pkg/Include/Guid/FspHeaderFile.h
> b/IntelFsp2Pkg/Include/Guid/FspHeaderFile.h
> index 3474bac1de..11e0ede65b 100644
> --- a/IntelFsp2Pkg/Include/Guid/FspHeaderFile.h
> +++ b/IntelFsp2Pkg/Include/Guid/FspHeaderFile.h
> @@ -2,7 +2,7 @@
>Intel FSP Header File definition from Intel Firmware Support Package
> External
> 
>Architecture Specification v2.0 and above.
> 
> 
> 
> -  Copyright (c) 2014 - 2020, Intel Corporation. All rights reserved.
> 
> +  Copyright (c) 2014 - 2021, Intel Corporation. All rights
> + reserved.
> 
>SPDX-License-Identifier: BSD-2-Clause-Patent
> 
> 
> 
>  **/
> 
> @@ -44,14 +44,28 @@ typedef struct {
>UINT8   Reserved1[2];
> 
>///
> 
>/// Byte 0x0A: Indicates compliance with a revision of this specification 
> in
> the BCD format.
> 
> +  ///For revision v2.3 the value will be 0x23.
> 
>///
> 
>UINT8   SpecVersion;
> 
>///
> 
>/// Byte 0x0B: Revision of the FSP Information Header.
> 
> +  ///The Current value for this field is 0x6.
> 
>///
> 
>UINT8   HeaderRevision;
> 
>///
> 
>/// Byte 0x0C: Revision of the FSP binary.
> 
> +  ///Major.Minor.Revision.Build
> 
> +  ///If FSP HeaderRevision is <= 5, the ImageRevision can be 
> decoded
> as follows:
> 
> +  ///   7 : 0  - Build Number
> 
> +  ///  15 : 8  - Revision
> 
> +  ///  23 : 16 - Minor Version
> 
> +  ///  31 : 24 - Major Version
> 
> +  ///If FSP HeaderRevision is >= 6, ImageRevision specifies the 
> low-
> order bytes of the build number and revision
> 
> +  ///while ExtendedImageRevision specifies the high-order bytes 
> of
> the build number and revision.
> 
> +  ///   7 : 0  - Low Byte of Build Number
> 
> +  ///  15 : 8  - Low Byte of Revision
> 
> +  ///  23 : 16 - Minor Version
> 
> +  ///  31 : 24 - Major Version
> 
>///
> 
>UINT32  ImageRevision;
> 
>///
> 
> @@ -116,6 +130,23 @@ typedef struct {
>///If the value is set to 0x, then this API is not 
> available in
> this component.
> 
>///
> 
>UINT32  FspMultiPhaseSiInitEntryOffset;
> 
> +  ///
> 
> +  /// Byte 0x4C: Extended revision of the FSP binary.
> 
> +  ///This value is only valid if FSP HeaderRevision is >= 6.
> 
> +  ///ExtendedImageRevision specifies the high-order byte of the
> revision and build number in the FSP binary revision.
> 
> +  ///   7 : 0 - High Byte of Build Number
> 
> +  ///  15 : 8 - High Byte of Revision
> 
> +  ///The FSP binary build number can be decoded as follows:
> 
> +  ///Build Number = (ExtendedImageRevision[7:0] << 8) |
> ImageRevision[7:0]
> 
> +  ///Revision = (ExtendedImageRevision[15:8] << 8) |
> ImageRevision[15:8]
> 
> +  ///Minor Version = ImageRevision[23:16]
> 
> +  ///Major Version = ImageRevision[31:24]
> 
> +  ///
> 
> +  UINT16  ExtendedImageRevision;
> 
> +  ///
> 
> +  /// Byte 0x4E: Reserved4.
> 
> +  ///
> 
> +  UINT16  Reserved4;
> 
>  } FSP_INFO_HEADER;
> 
> 
> 
>  ///
> 
> diff --git a/IntelFsp2Pkg/Include/Guid/FspNonVolatileStorageHob2.h
> b/IntelFsp2Pkg/Include/Guid/FspNonVolatileStorageHob2.h
> new file mode 100644
> index 00..8c07dc7e7e
> --- /dev/null
> +++ b/IntelFsp2Pkg/Include/Guid/FspNonVolatileStorageHob2.h
> @@ -0,0 +1,27 @@
> +/** @file
> 
> +  Intel FSP Non-Volatile Storage (NVS) HOB version 2 definition from
> 
> +  Intel Firmware Support Package External Architecture Specification v2.3.
> 
> +
> 
> +  Copyright (c) 2021, Intel Corporation. All rights reserved.
> 
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> 
> +
> 
> +**/
> 
> +
> 
> +#ifndef __FSP_NON_VOLATILE_STORAGE_HOB2_H__
> 
> +#define __FSP_NON_VOLATILE_STORAGE_HOB2_H__
> 
> +
> 
> +///
> 
>