Re: [edk2-devel] [PATCH] IntelFsp2Pkg: Add FSP*_ARCH_UPD.

["Nate DeSimone" <nathaniel.l.desimone@intel.com>]

Hi Chasel,

Feedback inline below.

Thanks,
Nate

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Chiu,
> Chasel
> Sent: Thursday, June 4, 2020 12:43 AM
> To: devel@edk2.groups.io
> Cc: Ma, Maurice <maurice.ma@intel.com>; Desimone, Nathaniel L
> <nathaniel.l.desimone@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: [edk2-devel] [PATCH] IntelFsp2Pkg: Add FSP*_ARCH_UPD.
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2781
> 
> Introduce FSPT_ARCH_UPD and FSPS_ARCH_UPD to support debug events
> and multi-phase silicon initialization.
> 
> Cc: Maurice Ma <maurice.ma@intel.com>
> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Signed-off-by: Chasel Chiu <chasel.chiu@intel.com>
> ---
>  IntelFsp2Pkg/Include/FspEas/FspApi.h | 55
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
> 
> diff --git a/IntelFsp2Pkg/Include/FspEas/FspApi.h
> b/IntelFsp2Pkg/Include/FspEas/FspApi.h
> index ed40f9538c..88c5b49e61 100644
> --- a/IntelFsp2Pkg/Include/FspEas/FspApi.h
> +++ b/IntelFsp2Pkg/Include/FspEas/FspApi.h
> @@ -106,6 +106,27 @@ typedef struct {
>  } FSP_UPD_HEADER;
> 
>  ///
> +/// FSPT_ARCH_UPD Configuration.
> +///
> +typedef struct {
> +  ///
> +  /// Revision Revision of the structure is 1 for this version of the
> specification.
> +  ///
> +  UINT8                       Revision;
> +  UINT8                       Reserved[3];
> +  ///
> +  /// Length Length of the structure in bytes. The current value for this field
> is 32.
> +  ///
> +  UINT32                      Length;
> +  ///
> +  /// FspDebugHandler Optional debug handler for the bootloader to
> +receive debug messages
> +  /// occurring during FSP execution.
> +  ///
> +  FSP_DEBUG_HANDLER           FspDebugHandler;
> +  UINT8                       Reserved1[20];
> +} FSPT_ARCH_UPD;
> +
> +///
>  /// FSPM_ARCH_UPD Configuration.
>  ///
>  typedef struct {
> @@ -146,6 +167,32 @@ typedef struct {
>    UINT8                       Reserved1[4];
>  } FSPM_ARCH_UPD;
> 
> +typedef struct {
> +  ///
> +  /// Revision Revision of the structure is 1 for this version of the
> specification.
> +  ///
> +  UINT8                      Revision;
> +  UINT8                      Reserved[3];
> +  ///
> +  /// Length Length of the structure in bytes. The current value for this field
> is 32.
> +  ///
> +  UINT32                      Length;
> +  ///
> +  /// FspEventHandler Optional event handler for the bootloader to be
> +informed of events
> +  /// occurring during FSP execution.
> +  ///
> +  FSP_EVENT_HANDLER           FspEventHandler;
> +  ///
> +  /// A FSP binary may optionally implement multi-phase silicon
> +initialization,
> +  /// This is only supported if the FspMultiPhaseSiInitEntryOffset
> +field in FSP_INFO_HEADER
> +  /// is non-zero.
> +  /// To enable multi-phase silicon initialization, the bootloader must
> +set
> +  /// EnableMultiPhaseSiliconInit to a non-zero value.
> +  ///
> +  UINT8                       EnableMultiPhaseSiliconInit;
> +  UINT8                       Reserved1[19];
> +} FSPS_ARCH_UPD;
> +
>  ///
>  /// FSPT_UPD_COMMON Configuration.
>  ///
> @@ -154,6 +201,10 @@ typedef struct {
>    /// FSP_UPD_HEADER Configuration.
>    ///
>    FSP_UPD_HEADER              FspUpdHeader;
> +  ///
> +  /// FSPT_ARCH_UPD Configuration.
> +  ///
> +  FSPT_ARCH_UPD               FsptArchUpd;

We might want to put an #if here. I think some people may want to build an FSP that only supports 2.0 or 2.1 with a newer version of EDK2. Maybe we need a new FixedAtBuildPcd for choosing whether to include FSPS/T_ARCH_UPD or not.

>  } FSPT_UPD_COMMON;
> 
>  ///
> @@ -178,6 +229,10 @@ typedef struct {
>    /// FSP_UPD_HEADER Configuration.
>    ///
>    FSP_UPD_HEADER              FspUpdHeader;
> +  ///
> +  /// FSPS_ARCH_UPD Configuration.
> +  ///
> +  FSPS_ARCH_UPD               FspsArchUpd;

We might want to put an #if here. I think some people may want to build an FSP that only supports 2.0 or 2.1 with a newer version of EDK2. Maybe we need a new FixedAtBuildPcd for choosing whether to include FSPS/T_ARCH_UPD or not.

>  } FSPS_UPD_COMMON;
> 
>  ///
> --
> 2.13.3.windows.1
> 
> 
>