Re: [edk2-platforms][PATCH v4 37/41] IntelSiliconPkg: Identify flash regions by GUID

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

Reviewed-by: Nate DeSimone <nathaniel.l.desimone@intel.com>

> -----Original Message-----
> From: mikuback@linux.microsoft.com <mikuback@linux.microsoft.com>
> Sent: Friday, June 25, 2021 2:21 PM
> To: devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>; Chaganty, Rangasai V
> <rangasai.v.chaganty@intel.com>; Desimone, Nathaniel L
> <nathaniel.l.desimone@intel.com>
> Subject: [edk2-platforms][PATCH v4 37/41] IntelSiliconPkg: Identify flash
> regions by GUID
> 
> From: Michael Kubacki <michael.kubacki@microsoft.com>
> 
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3307
> 
> Updates the SPI protocol to identify flash regions by GUID instead of fixed
> values defined in an enum. Packages consuming IntelSiliconPkg are able to
> associate a given GUID with their chosen values based on their SPI flash
> details as implemented in their PCH_SPI_PROTOCOL instance.
> 
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Rangasai V Chaganty <rangasai.v.chaganty@intel.com>
> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
> ---
> 
> Silicon/Intel/IntelSiliconPkg/Library/SmmSpiFlashCommonLib/SmmSpiFlashC
> ommonLib.c   |  2 +-
> 
> Silicon/Intel/IntelSiliconPkg/Library/SmmSpiFlashCommonLib/SpiFlashComm
> on.c         |  4 +-
>  Silicon/Intel/IntelSiliconPkg/Include/Protocol/Spi.h                                | 43
> ++++----------------
> 
> Silicon/Intel/IntelSiliconPkg/Library/SmmSpiFlashCommonLib/SmmSpiFlashC
> ommonLib.inf |  3 ++
>  4 files changed, 14 insertions(+), 38 deletions(-)
> 
> diff --git
> a/Silicon/Intel/IntelSiliconPkg/Library/SmmSpiFlashCommonLib/SmmSpiFlas
> hCommonLib.c
> b/Silicon/Intel/IntelSiliconPkg/Library/SmmSpiFlashCommonLib/SmmSpiFlas
> hCommonLib.c
> index 7941b8f8720c..2c9c889e7f48 100644
> ---
> a/Silicon/Intel/IntelSiliconPkg/Library/SmmSpiFlashCommonLib/SmmSpiFlas
> hCommonLib.c
> +++
> b/Silicon/Intel/IntelSiliconPkg/Library/SmmSpiFlashCommonLib/SmmSpiF
> +++ lashCommonLib.c
> @@ -52,7 +52,7 @@ SmmSpiFlashCommonLibConstructor (
>                      );
>    ASSERT_EFI_ERROR (Status);
> 
> -  mSpiProtocol->GetRegionAddress (mSpiProtocol, FlashRegionBios,
> &BaseAddr, &RegionSize);
> +  mSpiProtocol->GetRegionAddress (mSpiProtocol, &gFlashRegionBiosGuid,
> + &BaseAddr, &RegionSize);
>    mBiosOffset = BaseAddr;
>    return Status;
>  }
> diff --git
> a/Silicon/Intel/IntelSiliconPkg/Library/SmmSpiFlashCommonLib/SpiFlashCom
> mon.c
> b/Silicon/Intel/IntelSiliconPkg/Library/SmmSpiFlashCommonLib/SpiFlashCom
> mon.c
> index daebaf8e5e33..62c1d099fc2c 100644
> ---
> a/Silicon/Intel/IntelSiliconPkg/Library/SmmSpiFlashCommonLib/SpiFlashCom
> mon.c
> +++ b/Silicon/Intel/IntelSiliconPkg/Library/SmmSpiFlashCommonLib/SpiFlas
> +++ hCommon.c
> @@ -127,7 +127,7 @@ SpiFlashWrite (
>      }
>      Status = mSpiProtocol->FlashWrite (
>                               mSpiProtocol,
> -                             FlashRegionBios,
> +                             &gFlashRegionBiosGuid,
>                               (UINT32) Offset,
>                               Length,
>                               Buffer
> @@ -201,7 +201,7 @@ SpiFlashBlockErase (
> 
>    Status = mSpiProtocol->FlashErase (
>                             mSpiProtocol,
> -                           FlashRegionBios,
> +                           &gFlashRegionBiosGuid,
>                             (UINT32) Offset,
>                             (UINT32) RemainingBytes
>                             );
> diff --git a/Silicon/Intel/IntelSiliconPkg/Include/Protocol/Spi.h
> b/Silicon/Intel/IntelSiliconPkg/Include/Protocol/Spi.h
> index c13dc5a5f5f5..2b09ca1faf02 100644
> --- a/Silicon/Intel/IntelSiliconPkg/Include/Protocol/Spi.h
> +++ b/Silicon/Intel/IntelSiliconPkg/Include/Protocol/Spi.h
> @@ -19,33 +19,6 @@ extern EFI_GUID                   gPchSmmSpiProtocolGuid;
>  //
>  typedef struct _PCH_SPI_PROTOCOL  PCH_SPI_PROTOCOL;
> 
> -//
> -// SPI protocol data structures and definitions -//
> -
> -/**
> -  Flash Region Type
> -**/
> -typedef enum {
> -  FlashRegionDescriptor,
> -  FlashRegionBios,
> -  FlashRegionMe,
> -  FlashRegionGbE,
> -  FlashRegionPlatformData,
> -  FlashRegionDer,
> -  FlashRegionSecondaryBios,
> -  FlashRegionuCodePatch,
> -  FlashRegionEC,
> -  FlashRegionDeviceExpansion2,
> -  FlashRegionIE,
> -  FlashRegion10Gbe_A,
> -  FlashRegion10Gbe_B,
> -  FlashRegion13,
> -  FlashRegion14,
> -  FlashRegion15,
> -  FlashRegionAll,
> -  FlashRegionMax
> -} FLASH_REGION_TYPE;
>  //
>  // Protocol member functions
>  //
> @@ -54,7 +27,7 @@ typedef enum {
>    Read data from the flash part.
> 
>    @param[in] This                 Pointer to the PCH_SPI_PROTOCOL instance.
> -  @param[in] FlashRegionType      The Flash Region type for flash cycle which
> is listed in the Descriptor.
> +  @param[in] FlashRegionGuid      The Flash Region GUID for flash cycle
> which corresponds to the type in the descriptor.
>    @param[in] Address              The Flash Linear Address must fall within a
> region for which BIOS has access permissions.
>    @param[in] ByteCount            Number of bytes in the data portion of the SPI
> cycle.
>    @param[out] Buffer              The Pointer to caller-allocated buffer containing
> the dada received.
> @@ -68,7 +41,7 @@ typedef
>  EFI_STATUS
>  (EFIAPI *PCH_SPI_FLASH_READ) (
>    IN     PCH_SPI_PROTOCOL   *This,
> -  IN     FLASH_REGION_TYPE  FlashRegionType,
> +  IN     EFI_GUID           *FlashRegionGuid,
>    IN     UINT32             Address,
>    IN     UINT32             ByteCount,
>    OUT    UINT8              *Buffer
> @@ -78,7 +51,7 @@ EFI_STATUS
>    Write data to the flash part. Remark: Erase may be needed before write to
> the flash part.
> 
>    @param[in] This                 Pointer to the PCH_SPI_PROTOCOL instance.
> -  @param[in] FlashRegionType      The Flash Region type for flash cycle which
> is listed in the Descriptor.
> +  @param[in] FlashRegionGuid      The Flash Region GUID for flash cycle
> which corresponds to the type in the descriptor.
>    @param[in] Address              The Flash Linear Address must fall within a
> region for which BIOS has access permissions.
>    @param[in] ByteCount            Number of bytes in the data portion of the SPI
> cycle.
>    @param[in] Buffer               Pointer to caller-allocated buffer containing the
> data sent during the SPI cycle.
> @@ -91,7 +64,7 @@ typedef
>  EFI_STATUS
>  (EFIAPI *PCH_SPI_FLASH_WRITE) (
>    IN     PCH_SPI_PROTOCOL   *This,
> -  IN     FLASH_REGION_TYPE  FlashRegionType,
> +  IN     EFI_GUID           *FlashRegionGuid,
>    IN     UINT32             Address,
>    IN     UINT32             ByteCount,
>    IN     UINT8              *Buffer
> @@ -101,7 +74,7 @@ EFI_STATUS
>    Erase some area on the flash part.
> 
>    @param[in] This                 Pointer to the PCH_SPI_PROTOCOL instance.
> -  @param[in] FlashRegionType      The Flash Region type for flash cycle which
> is listed in the Descriptor.
> +  @param[in] FlashRegionGuid      The Flash Region GUID for flash cycle
> which corresponds to the type in the descriptor.
>    @param[in] Address              The Flash Linear Address must fall within a
> region for which BIOS has access permissions.
>    @param[in] ByteCount            Number of bytes in the data portion of the SPI
> cycle.
> 
> @@ -113,7 +86,7 @@ typedef
>  EFI_STATUS
>  (EFIAPI *PCH_SPI_FLASH_ERASE) (
>    IN     PCH_SPI_PROTOCOL   *This,
> -  IN     FLASH_REGION_TYPE  FlashRegionType,
> +  IN     EFI_GUID           *FlashRegionGuid,
>    IN     UINT32             Address,
>    IN     UINT32             ByteCount
>    );
> @@ -206,7 +179,7 @@ EFI_STATUS
>    Get the SPI region base and size, based on the enum type
> 
>    @param[in] This                 Pointer to the PCH_SPI_PROTOCOL instance.
> -  @param[in] FlashRegionType      The Flash Region type for for the base
> address which is listed in the Descriptor.
> +  @param[in] FlashRegionGuid      The Flash Region GUID for flash cycle
> which corresponds to the type in the descriptor.
>    @param[out] BaseAddress         The Flash Linear Address for the Region 'n'
> Base
>    @param[out] RegionSize          The size for the Region 'n'
> 
> @@ -218,7 +191,7 @@ typedef
>  EFI_STATUS
>  (EFIAPI *PCH_SPI_GET_REGION_ADDRESS) (
>    IN     PCH_SPI_PROTOCOL   *This,
> -  IN     FLASH_REGION_TYPE  FlashRegionType,
> +  IN     EFI_GUID           *FlashRegionGuid,
>    OUT    UINT32             *BaseAddress,
>    OUT    UINT32             *RegionSize
>    );
> diff --git
> a/Silicon/Intel/IntelSiliconPkg/Library/SmmSpiFlashCommonLib/SmmSpiFlas
> hCommonLib.inf
> b/Silicon/Intel/IntelSiliconPkg/Library/SmmSpiFlashCommonLib/SmmSpiFlas
> hCommonLib.inf
> index f6a06351ace5..b28ea3c2719d 100644
> ---
> a/Silicon/Intel/IntelSiliconPkg/Library/SmmSpiFlashCommonLib/SmmSpiFlas
> hCommonLib.inf
> +++
> b/Silicon/Intel/IntelSiliconPkg/Library/SmmSpiFlashCommonLib/SmmSpiF
> +++ lashCommonLib.inf
> @@ -37,6 +37,9 @@ [Pcd]
>    gIntelSiliconPkgTokenSpaceGuid.PcdBiosAreaBaseAddress   ## CONSUMES
>    gIntelSiliconPkgTokenSpaceGuid.PcdBiosSize              ## CONSUMES
> 
> +[Guids]
> +  gFlashRegionBiosGuid
> +
>  [Sources]
>    SmmSpiFlashCommonLib.c
>    SpiFlashCommon.c
> --
> 2.28.0.windows.1