Re: [edk2-platforms/devel-MinPlatform][PATCH v2 1/3] KabylakeSiliconPkg: Add SPI write support in PEI

["Chiu, Chasel" <chasel.chiu@intel.com>]

Hi Michael,

A question in below patch inline, please help to check it.

Thanks!
Chasel

> -----Original Message-----
> From: Desimone, Nathaniel L
> Sent: Tuesday, April 2, 2019 8:09 AM
> To: Kubacki, Michael A <michael.a.kubacki@intel.com>;
> edk2-devel@lists.01.org
> Cc: Chiu, Chasel <chasel.chiu@intel.com>; Gao, Liming <liming.gao@intel.com>;
> Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: RE: [edk2-platforms/devel-MinPlatform][PATCH v2 1/3]
> KabylakeSiliconPkg: Add SPI write support in PEI
> 
> 1. The changes in BasePchSpiCommonLib.inf seems to be no longer necessary.
> 
> 2. Adding #include <Library/PcdLib.h> to SpiCommon.c seems to be no longer
> necessary.
> 
> Thanks,
> Nate
> 
> -----Original Message-----
> From: Kubacki, Michael A
> Sent: Monday, April 1, 2019 3:48 PM
> To: edk2-devel@lists.01.org
> Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Chiu, Chasel
> <chasel.chiu@intel.com>; Gao, Liming <liming.gao@intel.com>; Kinney, Michael
> D <michael.d.kinney@intel.com>
> Subject: [edk2-platforms/devel-MinPlatform][PATCH v2 1/3] KabylakeSiliconPkg:
> Add SPI write support in PEI
> 
> Adds a new library PeiSpiLib to perform the initialization necessary to perform
> SPI write cycles in PEI. After initialization, it installs an instance of the
> PCH_SPI_PPI.
> 
> Cc: Nate DeSimone <mailto:nathaniel.l.desimone@intel.com>
> Cc: Chasel Chiu <mailto:chasel.chiu@intel.com>
> Cc: Liming Gao <mailto:liming.gao@intel.com>
> Cc: Michael D Kinney <mailto:michael.d.kinney@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Michael Kubacki <mailto:michael.a.kubacki@intel.com>
> ---
>  Silicon/Intel/KabylakeSiliconPkg/SiPkgPeiLib.dsc   |   3 +-
>  .../Pch/Library/PeiSpiLib/PeiSpiLib.inf            |  50 +++++
>  .../BasePchSpiCommonLib/BasePchSpiCommonLib.inf    |   6 +-
>  .../Pch/Include/Library/SpiLib.h                   |  32 +++
>  .../Pch/Library/PeiSpiLib/PeiSpiLib.c              | 221 +++++++++++++++++++++
>  .../LibraryPrivate/BasePchSpiCommonLib/SpiCommon.c |   6 +-
>  6 files changed, 313 insertions(+), 5 deletions(-)  create mode 100644
> Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiSpiLib/PeiSpiLib.inf
>  create mode 100644
> Silicon/Intel/KabylakeSiliconPkg/Pch/Include/Library/SpiLib.h
>  create mode 100644
> Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiSpiLib/PeiSpiLib.c
> 
> diff --git a/Silicon/Intel/KabylakeSiliconPkg/SiPkgPeiLib.dsc
> b/Silicon/Intel/KabylakeSiliconPkg/SiPkgPeiLib.dsc
> index b81a736486..bb95ce3888 100644
> --- a/Silicon/Intel/KabylakeSiliconPkg/SiPkgPeiLib.dsc
> +++ b/Silicon/Intel/KabylakeSiliconPkg/SiPkgPeiLib.dsc
> @@ -1,7 +1,7 @@
>  ## @file
>  #  Component description file for the SkyLake SiPkg PEI libraries.
>  #
> -# Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
> +# Copyright (c) 2017 - 2019, Intel Corporation. All rights
> +reserved.<BR>
>  #
>  # This program and the accompanying materials are licensed and made available
> under  # the terms and conditions of the BSD License which accompanies this
> distribution.
> @@ -30,6 +30,7 @@
>  !endif
> 
> ResetSystemLib|$(PLATFORM_SI_PACKAGE)/Pch/Library/PeiResetSystemLib/P
> eiResetSystemLib.inf
> 
> PchResetLib|$(PLATFORM_SI_PACKAGE)/Pch/Library/PeiPchResetLib/PeiPchR
> esetLib.inf
> + SpiLib|$(PLATFORM_SI_PACKAGE)/Pch/Library/PeiSpiLib/PeiSpiLib.inf
> 
>  #
>  # Cpu
> diff --git a/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiSpiLib/PeiSpiLib.inf
> b/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiSpiLib/PeiSpiLib.inf
> new file mode 100644
> index 0000000000..9240b6ef06
> --- /dev/null
> +++ b/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiSpiLib/PeiSpiLib.i
> +++ nf
> @@ -0,0 +1,50 @@
> +## @file
> +# Component description file for PEI PCH SPI Initialization # #
> +Copyright (c) 2019, Intel Corporation. All rights reserved.<BR> # #
> +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.
> +#
> +##
> +
> +[Defines]
> +  INF_VERSION     = 0x00010017
> +  BASE_NAME       = PeiSpiLib
> +  FILE_GUID       = 4998447D-7948-448F-AB75-96E24E18FF23
> +  VERSION_STRING  = 1.0
> +  MODULE_TYPE     = PEIM
> +  LIBRARY_CLASS   = SpiLib|PEIM PEI_CORE
> +  #
> +  # The following information is for reference only and not required by the build
> tools.
> +  #
> +  # VALID_ARCHITECTURES = IA32 X64 IPF
> +  #
> +
> +[LibraryClasses]
> +  DebugLib
> +  MemoryAllocationLib
> +  PcdLib
> +  PchCycleDecodingLib
> +  PchSpiCommonLib
> +  PciSegmentLib
> +  PeiServicesLib
> +  PeiServicesTablePointerLib
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +  KabylakeSiliconPkg/SiPkg.dec
> +
> +[Sources]
> +  PeiSpiLib.c
> +
> +[Pcd]
> +  gSiPkgTokenSpaceGuid.PcdAcpiBaseAddress       ## CONSUMES
> +
> +[Ppis]
> +  gPchSpiPpiGuid      ## PRODUCES
> diff --git
> a/Silicon/Intel/KabylakeSiliconPkg/Pch/LibraryPrivate/BasePchSpiCommonLib/
> BasePchSpiCommonLib.inf
> b/Silicon/Intel/KabylakeSiliconPkg/Pch/LibraryPrivate/BasePchSpiCommonLib/
> BasePchSpiCommonLib.inf
> index 128f7adcea..2c531e7816 100644
> ---
> a/Silicon/Intel/KabylakeSiliconPkg/Pch/LibraryPrivate/BasePchSpiCommonLib/
> BasePchSpiCommonLib.inf
> +++ b/Silicon/Intel/KabylakeSiliconPkg/Pch/LibraryPrivate/BasePchSpiComm
> +++ onLib/BasePchSpiCommonLib.inf
> @@ -1,7 +1,7 @@
>  ## @file
>  #  Component description file for the PchSpiCommonLib  # -# Copyright (c)
> 2017, Intel Corporation. All rights reserved.<BR>
> +# Copyright (c) 2017 - 2019, Intel Corporation. All rights
> +reserved.<BR>
>  #
>  # This program and the accompanying materials are licensed and made available
> under  # the terms and conditions of the BSD License which accompanies this
> distribution.
> @@ -32,4 +32,8 @@
>  [LibraryClasses]
>    IoLib
>    DebugLib
> +  PcdLib
>    PchCycleDecodingLib
> +
> +[Pcd]
> +  gSiPkgTokenSpaceGuid.PcdAcpiBaseAddress       ## CONSUMES
> diff --git a/Silicon/Intel/KabylakeSiliconPkg/Pch/Include/Library/SpiLib.h
> b/Silicon/Intel/KabylakeSiliconPkg/Pch/Include/Library/SpiLib.h
> new file mode 100644
> index 0000000000..6af66f8869
> --- /dev/null
> +++ b/Silicon/Intel/KabylakeSiliconPkg/Pch/Include/Library/SpiLib.h
> @@ -0,0 +1,32 @@
> +/** @file
> +  Library to initialize SPI services for future SPI accesses.
> +
> +Copyright (c) 2019, Intel Corporation. All rights reserved.<BR> This
> +program and the accompanying materials are licensed and made available
> +under the terms and conditions of the BSD License that 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 _SPI_LIB_H_
> +#define _SPI_LIB_H_
> +
> +/**
> +  Initializes SPI for access from future services.
> +
> +  @retval EFI_SUCCESS         The SPI service was initialized successfully.
> +  @retval EFI_OUT_OF_RESOUCES Insufficient memory available to allocate
> structures required for initialization.
> +  @retval Others              An error occurred initializing SPI services.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +SpiServiceInit (
> +  VOID
> +  );
> +
> +#endif
> diff --git a/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiSpiLib/PeiSpiLib.c
> b/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiSpiLib/PeiSpiLib.c
> new file mode 100644
> index 0000000000..954317cd40
> --- /dev/null
> +++ b/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiSpiLib/PeiSpiLib.c
> @@ -0,0 +1,221 @@
> +/** @file
> +  PCH SPI PEI Library implements the SPI Host Controller Interface.
> +
> +Copyright (c) 2019, Intel Corporation. All rights reserved.<BR> This
> +program and the accompanying materials are licensed and made available
> +under the terms and conditions of the BSD License that 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.
> +
> +**/
> +
> +#include <PchReservedResources.h>
> +#include <IndustryStandard/Pci30.h>
> +#include <Ppi/Spi.h>
> +#include <Register/PchRegsLpc.h>
> +#include <Register/PchRegsSpi.h>
> +#include <Library/DebugLib.h>
> +#include <Library/IoLib.h>
> +#include <Library/MemoryAllocationLib.h> #include <Library/PcdLib.h>
> +#include <Library/PchCycleDecodingLib.h> #include
> +<Library/PchSpiCommonLib.h> #include <Library/PciSegmentLib.h> #include
> +<Library/PeiServicesLib.h>
> +
> +typedef struct {
> +  EFI_PEI_PPI_DESCRIPTOR  PpiDescriptor;
> +  SPI_INSTANCE            SpiInstance;
> +} PEI_SPI_INSTANCE;
> +
> +/**
> +  Initializes the SPI BAR0 value to a default value and enables memory space
> decoding.
> +
> +  The SPI BAR0 will be assigned later in PCI enumeration.
> +
> +**/
> +VOID
> +InitSpiBar0 (
> +  VOID
> +  )
> +{
> +  UINT64       PchSpiBase;
> +  PchSpiBase = PCI_SEGMENT_LIB_ADDRESS (
> +                 0,
> +                 0,
> +                 PCI_DEVICE_NUMBER_PCH_SPI,
> +                 PCI_FUNCTION_NUMBER_PCH_SPI,
> +                 0
> +                 );
> +  PciSegmentWrite32 (PchSpiBase + R_PCH_SPI_BAR0,
> +PCH_SPI_BASE_ADDRESS);
> +  PciSegmentOr32 (PchSpiBase + PCI_COMMAND_OFFSET,
> +EFI_PCI_COMMAND_MEMORY_SPACE); }
> +
> +/**
> +  Initializes SPI for access from future services.
> +
> +  @retval EFI_SUCCESS         The SPI service was initialized successfully.
> +  @retval EFI_OUT_OF_RESOUCES Insufficient memory available to allocate
> structures required for initialization.
> +  @retval Others              An error occurred initializing SPI services.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +SpiServiceInit (
> +  VOID
> +  )
> +{
> +  EFI_STATUS        Status;
> +  PEI_SPI_INSTANCE  *PeiSpiInstance;
> +  SPI_INSTANCE      *SpiInstance;
> +  PCH_SPI_PPI       *SpiPpi;
> +  UINT16            AcpiBase;
> +
> +  AcpiBase = 0;
> +
> +  Status = PeiServicesLocatePpi (
> +             &gPchSpiPpiGuid,
> +             0,
> +             NULL,
> +             (VOID **) &SpiPpi
> +             );
> +
> +  if (Status != EFI_SUCCESS) {

Hi Michael,

Why we must initialize AcpiBase as part of this SpiServiceInit ()?
Please leave some comments to explain the usage here.


> +    PchAcpiBaseGet (&AcpiBase);
> +    if (AcpiBase == 0) {
> +      PchAcpiBaseSet (PcdGet16 (PcdAcpiBaseAddress));
> +    }
> +
> +    //
> +    // Prior to PCI enumeration, initialize SPI BAR0 to a default value
> +    // and also enable memory space decoding for SPI
> +    //
> +    InitSpiBar0 ();
> +
> +    PeiSpiInstance = (PEI_SPI_INSTANCE *) AllocateZeroPool (sizeof
> (PEI_SPI_INSTANCE));
> +    if (NULL == PeiSpiInstance) {
> +      return EFI_OUT_OF_RESOURCES;
> +    }
> +
> +    SpiInstance = &(PeiSpiInstance->SpiInstance);
> +    SpiProtocolConstructor (SpiInstance);
> +
> +    PeiSpiInstance->PpiDescriptor.Flags = EFI_PEI_PPI_DESCRIPTOR_PPI |
> EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST;
> +    PeiSpiInstance->PpiDescriptor.Guid = &gPchSpiPpiGuid;
> +    PeiSpiInstance->PpiDescriptor.Ppi = &(SpiInstance->SpiProtocol);
> +
> +    Status = PeiServicesInstallPpi (&PeiSpiInstance->PpiDescriptor);
> +  }
> +  return Status;
> +}
> +
> +/**
> +  Acquires the PCH SPI BAR0 MMIO address.
> +
> +  @param[in] SpiInstance          Pointer to SpiInstance to initialize
> +
> +  @retval    UINTN                The SPIO BAR0 MMIO address
> +
> +**/
> +UINTN
> +AcquireSpiBar0 (
> +  IN  SPI_INSTANCE                *SpiInstance
> +  )
> +{
> +  return MmioRead32 (SpiInstance->PchSpiBase + R_PCH_SPI_BAR0) &
> +~(B_PCH_SPI_BAR0_MASK); }
> +
> +/**
> +  Release the PCH SPI BAR0 MMIO address.
> +
> +  @param[in] SpiInstance          Pointer to SpiInstance to initialize
> +
> +  @retval None
> +**/
> +VOID
> +ReleaseSpiBar0 (
> +  IN  SPI_INSTANCE                *SpiInstance
> +  )
> +{
> +  return;
> +}
> +
> +/**
> +  Disables BIOS Write Protect
> +
> +  @retval EFI_SUCCESS             BIOS Write Protect was disabled successfully
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +DisableBiosWriteProtect (
> +  VOID
> +  )
> +{
> +  UINT64  SpiBaseAddress;
> +
> +  SpiBaseAddress =  PCI_SEGMENT_LIB_ADDRESS (
> +                      0,
> +                      0,
> +                      PCI_DEVICE_NUMBER_PCH_SPI,
> +                      PCI_FUNCTION_NUMBER_PCH_SPI,
> +                      0
> +                      );
> +  //
> +  // Clear EISS bit to allow for SPI use  //
> +  PciSegmentAnd8 (SpiBaseAddress + R_PCH_SPI_BC, (UINT8)
> + ~B_PCH_SPI_BC_EISS);
> +
> +  //
> +  // Write clear BC_SYNC_SS prior to change WPD from 0 to 1.
> +  //
> +  PciSegmentOr8 (
> +    SpiBaseAddress + R_PCH_SPI_BC + 1,
> +    (B_PCH_SPI_BC_SYNC_SS >> 8)
> +    );
> +
> +  //
> +  // Set BIOSWE bit (SPI PCI Offset DCh [0]) = 1b  // Enable the access
> + to the BIOS space for both read and write cycles  //
> +  PciSegmentOr8 (
> +    SpiBaseAddress + R_PCH_SPI_BC,
> +    B_PCH_SPI_BC_WPD
> +    );
> +
> +  ASSERT ((PciSegmentRead8 (SpiBaseAddress + R_PCH_SPI_BC) &
> + B_PCH_SPI_BC_EISS) != 0);
> +
> +  return EFI_SUCCESS;
> +}
> +
> +/**
> +  Enables BIOS Write Protect
> +
> +**/
> +VOID
> +EFIAPI
> +EnableBiosWriteProtect (
> +  VOID
> +  )
> +{
> +  UINT64  SpiBaseAddress;
> +
> +  SpiBaseAddress =  PCI_SEGMENT_LIB_ADDRESS (
> +                      0,
> +                      0,
> +                      PCI_DEVICE_NUMBER_PCH_SPI,
> +                      PCI_FUNCTION_NUMBER_PCH_SPI,
> +                      0
> +                      );
> +
> +  //
> +  // Disable the access to the BIOS space for write cycles
> +  //
> +  PciSegmentAnd8 (
> +    SpiBaseAddress + R_PCH_SPI_BC,
> +    (UINT8) (~B_PCH_SPI_BC_WPD)
> +    );
> +}
> diff --git
> a/Silicon/Intel/KabylakeSiliconPkg/Pch/LibraryPrivate/BasePchSpiCommonLib/
> SpiCommon.c
> b/Silicon/Intel/KabylakeSiliconPkg/Pch/LibraryPrivate/BasePchSpiCommonLib/
> SpiCommon.c
> index 46184d4994..e464aedcbe 100644
> ---
> a/Silicon/Intel/KabylakeSiliconPkg/Pch/LibraryPrivate/BasePchSpiCommonLib/
> SpiCommon.c
> +++ b/Silicon/Intel/KabylakeSiliconPkg/Pch/LibraryPrivate/BasePchSpiComm
> +++ onLib/SpiCommon.c
> @@ -1,7 +1,7 @@
>  /** @file
>    PCH SPI Common Driver implements the SPI Host Controller Compatibility
> Interface.
> 
> -Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2017 - 2019, Intel Corporation. All rights reserved.<BR>
>  This program and the accompanying materials are licensed and made available
> under  the terms and conditions of the BSD License that accompanies this
> distribution.
>  The full text of the license may be found at @@ -14,6 +14,7 @@ WITHOUT
> WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>  #include <Uefi/UefiBaseType.h>
>  #include <Library/IoLib.h>
>  #include <Library/DebugLib.h>
> +#include <Library/PcdLib.h>
>  #include <Library/BaseMemoryLib.h>
>  #include <IndustryStandard/Pci30.h>
>  #include <PchAccess.h>
> @@ -760,9 +761,8 @@ SendSpiCmd (
>    Status            = EFI_SUCCESS;
>    SpiInstance       = SPI_INSTANCE_FROM_SPIPROTOCOL (This);
>    SpiBaseAddress    = SpiInstance->PchSpiBase;
> -  PchSpiBar0        = AcquireSpiBar0 (SpiInstance);
> -  SpiBaseAddress    = SpiInstance->PchSpiBase;
>    ABase             = SpiInstance->PchAcpiBase;
> +  PchSpiBar0        = AcquireSpiBar0 (SpiInstance);
> 
>    //
>    // Disable SMIs to make sure normal mode flash access is not interrupted by an
> SMI
> --
> 2.16.2.windows.1