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

["Kubacki, Michael A" <michael.a.kubacki@intel.com>]

Comment added in v4

> -----Original Message-----
> From: Chiu, Chasel
> Sent: Monday, April 1, 2019 7:29 PM
> To: Kubacki, Michael A <michael.a.kubacki@intel.com>; edk2-
> devel@lists.01.org
> Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Gao, Liming
> <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: RE: [edk2-platforms/devel-MinPlatform][PATCH v3 1/3]
> KabylakeSiliconPkg: Add SPI write support in PEI
> 
> 
> Hi Michael,
> 
> Please see below inline.
> 
> Thanks!
> Chasel
> 
> 
> > -----Original Message-----
> > From: Kubacki, Michael A
> > Sent: Tuesday, April 2, 2019 9:23 AM
> > 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 v3 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 <nathaniel.l.desimone@intel.com>
> > Cc: Chasel Chiu <chasel.chiu@intel.com>
> > Cc: Liming Gao <liming.gao@intel.com>
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Michael Kubacki <michael.a.kubacki@intel.com>
> > ---
> >  Silicon/Intel/KabylakeSiliconPkg/SiPkgPeiLib.dsc   |   3 +-
> >  .../Pch/Library/PeiSpiLib/PeiSpiLib.inf            |  50 +++++
> >  .../Pch/Include/Library/SpiLib.h                   |  32 +++
> >  .../Pch/Library/PeiSpiLib/PeiSpiLib.c              | 221 +++++++++++++++++++++
> >  .../LibraryPrivate/BasePchSpiCommonLib/SpiCommon.c |   5 +-
> >  5 files changed, 307 insertions(+), 4 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/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) {
> 
> Would you please add comments to explain why we must initialize ACPI BASE
> as part of the SpiServiceInit()?
> 
> > +    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/BasePchSpiCommon
> > Lib/
> > SpiCommon.c
> > b/Silicon/Intel/KabylakeSiliconPkg/Pch/LibraryPrivate/BasePchSpiCommon
> > Lib/
> > SpiCommon.c
> > index 46184d4994..0b708d4aad 100644
> > ---
> > a/Silicon/Intel/KabylakeSiliconPkg/Pch/LibraryPrivate/BasePchSpiCommon
> > Lib/
> > SpiCommon.c
> > +++ b/Silicon/Intel/KabylakeSiliconPkg/Pch/LibraryPrivate/BasePchSpiCo
> > +++ mm
> > +++ 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 @@ -760,9 +760,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