Re: [edk2-devel][edk2-platforms][PATCH V1 10/11] MinPlatformPkg/SpiFvbService: Reduce duplicate code

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

Hi Isaac,

Please see feedback inline.

Thanks,
Nate

> -----Original Message-----
> From: Oram, Isaac W <isaac.w.oram@intel.com>
> Sent: Friday, October 15, 2021 2:26 PM
> To: devel@edk2.groups.io
> Cc: Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L
> <nathaniel.l.desimone@intel.com>; Liming Gao
> <gaoliming@byosoft.com.cn>; Dong, Eric <eric.dong@intel.com>
> Subject: [edk2-devel][edk2-platforms][PATCH V1 10/11]
> MinPlatformPkg/SpiFvbService: Reduce duplicate code
> 
> Consolidate all the entry point functions into SpiFvbServiceMm.c This
> reduces the number of files and makes the code easier to understand.
> 
> Cc: Chasel Chiu <chasel.chiu@intel.com>
> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Eric Dong <eric.dong@intel.com>
> Signed-off-by: Isaac Oram <isaac.w.oram@intel.com>
> ---
>  Platform/Intel/MinPlatformPkg/Flash/SpiFvbService/SpiFvbServiceMm.c
> | 42 ++++++++++++++++++++
>  Platform/Intel/MinPlatformPkg/Flash/SpiFvbService/SpiFvbServiceSmm.inf
> |  1 -
> 
> Platform/Intel/MinPlatformPkg/Flash/SpiFvbService/SpiFvbServiceStandalon
> eMm.c   | 32 ---------------
> 
> Platform/Intel/MinPlatformPkg/Flash/SpiFvbService/SpiFvbServiceStandalon
> eMm.inf |  1 -
> Platform/Intel/MinPlatformPkg/Flash/SpiFvbService/SpiFvbServiceTraditiona
> lMm.c  | 32 ---------------
>  5 files changed, 42 insertions(+), 66 deletions(-)
> 
> diff --git
> a/Platform/Intel/MinPlatformPkg/Flash/SpiFvbService/SpiFvbServiceMm.c
> b/Platform/Intel/MinPlatformPkg/Flash/SpiFvbService/SpiFvbServiceMm.c
> index 1d8d55b8f2..cc9310023e 100644
> ---
> a/Platform/Intel/MinPlatformPkg/Flash/SpiFvbService/SpiFvbServiceMm.c
> +++
> b/Platform/Intel/MinPlatformPkg/Flash/SpiFvbService/SpiFvbServiceMm.
> +++ c
> @@ -278,3 +278,45 @@ FvbInitialize (
> 
>    return EFI_SUCCESS;
>  }
> +
> +/**
> +  The driver Traditional MM entry point.
> +
> +  @param[in] ImageHandle          Image handle of this driver.
> +  @param[in] SystemTable          A pointer to the EFI system table.
> +
> +  @retval EFI_SUCCESS             This function always returns EFI_SUCCESS.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +SpiFvbTraditionalMmInitialize (
> +  IN EFI_HANDLE            ImageHandle,
> +  IN EFI_SYSTEM_TABLE      *SystemTable
> +  )
> +{
> +  FvbInitialize ();

Now that FvbInitialize() returns an EFI_STATUS code. It would be good to add a DEBUG print here in the case where an error is returned. I understand the hesitation to return it back to the core since this would not necessarily be a fatal error.

> +
> +  return EFI_SUCCESS;
> +}
> +
> +/**
> +  The driver Standalone MM entry point.
> +
> +  @param[in] ImageHandle          Image handle of this driver.
> +  @param[in] MmSystemTable        A pointer to the MM system table.
> +
> +  @retval EFI_SUCCESS             This function always returns EFI_SUCCESS.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +SpiFvbStandaloneMmInitialize (
> +  IN EFI_HANDLE            ImageHandle,
> +  IN EFI_MM_SYSTEM_TABLE   *MmSystemTable
> +  )
> +{
> +  FvbInitialize ();

Now that FvbInitialize() returns an EFI_STATUS code. It would be good to add a DEBUG print here in the case where an error is returned. I understand the hesitation to return it back to the core since this would not necessarily be a fatal error.

> +
> +  return EFI_SUCCESS;
> +}
> diff --git
> a/Platform/Intel/MinPlatformPkg/Flash/SpiFvbService/SpiFvbServiceSmm.in
> f
> b/Platform/Intel/MinPlatformPkg/Flash/SpiFvbService/SpiFvbServiceSmm.in
> f
> index 7b69bedcdc..600ad32d9f 100644
> ---
> a/Platform/Intel/MinPlatformPkg/Flash/SpiFvbService/SpiFvbServiceSmm.in
> f
> +++
> b/Platform/Intel/MinPlatformPkg/Flash/SpiFvbService/SpiFvbServiceSmm
> +++ .inf
> @@ -55,7 +55,6 @@
>    SpiFvbServiceCommon.c
>    SpiFvbServiceMm.h
>    SpiFvbServiceMm.c
> -  SpiFvbServiceTraditionalMm.c
> 
>  [Protocols]
>    gEfiDevicePathProtocolGuid                    ## PRODUCES
> diff --git
> a/Platform/Intel/MinPlatformPkg/Flash/SpiFvbService/SpiFvbServiceStandal
> oneMm.c
> b/Platform/Intel/MinPlatformPkg/Flash/SpiFvbService/SpiFvbServiceStandal
> oneMm.c
> deleted file mode 100644
> index 252c818d65..0000000000
> ---
> a/Platform/Intel/MinPlatformPkg/Flash/SpiFvbService/SpiFvbServiceStandal
> oneMm.c
> +++ /dev/null
> @@ -1,32 +0,0 @@
> -/** @file
> -  MM driver source for several Serial Flash devices
> -  which are compliant with the Intel(R) Serial Flash Interface Compatibility
> Specification.
> -
> -  Copyright (c) Microsoft Corporation.<BR>
> -  SPDX-License-Identifier: BSD-2-Clause-Patent
> -
> -**/
> -
> -#include "SpiFvbServiceCommon.h"
> -#include "SpiFvbServiceMm.h"
> -
> -/**
> -  The driver Standalone MM entry point.
> -
> -  @param[in] ImageHandle          Image handle of this driver.
> -  @param[in] MmSystemTable        A pointer to the MM system table.
> -
> -  @retval EFI_SUCCESS             This function always returns EFI_SUCCESS.
> -
> -**/
> -EFI_STATUS
> -EFIAPI
> -SpiFvbStandaloneMmInitialize (
> -  IN EFI_HANDLE            ImageHandle,
> -  IN EFI_MM_SYSTEM_TABLE   *MmSystemTable
> -  )
> -{
> -  FvbInitialize ();
> -
> -  return EFI_SUCCESS;
> -}
> diff --git
> a/Platform/Intel/MinPlatformPkg/Flash/SpiFvbService/SpiFvbServiceStandal
> oneMm.inf
> b/Platform/Intel/MinPlatformPkg/Flash/SpiFvbService/SpiFvbServiceStandal
> oneMm.inf
> index 1dfb2520e7..4e056466a1 100644
> ---
> a/Platform/Intel/MinPlatformPkg/Flash/SpiFvbService/SpiFvbServiceStandal
> oneMm.inf
> +++ b/Platform/Intel/MinPlatformPkg/Flash/SpiFvbService/SpiFvbServiceSta
> +++ ndaloneMm.inf
> @@ -54,7 +54,6 @@
>    SpiFvbServiceCommon.c
>    SpiFvbServiceMm.h
>    SpiFvbServiceMm.c
> -  SpiFvbServiceStandaloneMm.c
> 
>  [Protocols]
>    gEfiDevicePathProtocolGuid                    ## PRODUCES
> diff --git
> a/Platform/Intel/MinPlatformPkg/Flash/SpiFvbService/SpiFvbServiceTraditio
> nalMm.c
> b/Platform/Intel/MinPlatformPkg/Flash/SpiFvbService/SpiFvbServiceTraditio
> nalMm.c
> deleted file mode 100644
> index 1c2dac70e3..0000000000
> ---
> a/Platform/Intel/MinPlatformPkg/Flash/SpiFvbService/SpiFvbServiceTraditio
> nalMm.c
> +++ /dev/null
> @@ -1,32 +0,0 @@
> -/** @file
> -  MM driver source for several Serial Flash devices
> -  which are compliant with the Intel(R) Serial Flash Interface Compatibility
> Specification.
> -
> -  Copyright (c) Microsoft Corporation.<BR>
> -  SPDX-License-Identifier: BSD-2-Clause-Patent
> -
> -**/
> -
> -#include "SpiFvbServiceCommon.h"
> -#include "SpiFvbServiceMm.h"
> -
> -/**
> -  The driver Traditional MM entry point.
> -
> -  @param[in] ImageHandle          Image handle of this driver.
> -  @param[in] SystemTable          A pointer to the EFI system table.
> -
> -  @retval EFI_SUCCESS             This function always returns EFI_SUCCESS.
> -
> -**/
> -EFI_STATUS
> -EFIAPI
> -SpiFvbTraditionalMmInitialize (
> -  IN EFI_HANDLE            ImageHandle,
> -  IN EFI_SYSTEM_TABLE      *SystemTable
> -  )
> -{
> -  FvbInitialize ();
> -
> -  return EFI_SUCCESS;
> -}
> --
> 2.27.0.windows.1