Re: [edk2-devel] [edk2-staging/UEFI_PCI_ENHANCE-2 PATCH 01/12] MdeModulePkg/PciBusDxe:New PCI features separation with PCD

["Ni, Ray" <ray.ni@intel.com>]

With the new protocol discussed in https://bugzilla.tianocore.org/show_bug.cgi?id=1954,
this patch review is skipped.


> -----Original Message-----
> From: Javeed, Ashraf <ashraf.javeed@intel.com>
> Sent: Wednesday, November 13, 2019 11:22 AM
> To: devel@edk2.groups.io; Javeed, Ashraf <ashraf.javeed@intel.com>
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>
> Subject: RE: [edk2-devel] [edk2-staging/UEFI_PCI_ENHANCE-2 PATCH 01/12] MdeModulePkg/PciBusDxe:New PCI features
> separation with PCD
> 
> This patch is also uploaded in the following Repo:-
> https://github.com/ashrafj/edk2-staging/commit/0cc1a9555e1546ad94dd368160ece526d10d96a6
> 
> Please review.
> Thanks
> Ashraf
> 
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Javeed,
> > Ashraf
> > Sent: Friday, November 1, 2019 8:40 PM
> > To: devel@edk2.groups.io
> > Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
> > Ni, Ray <ray.ni@intel.com>
> > Subject: [edk2-devel] [edk2-staging/UEFI_PCI_ENHANCE-2 PATCH 01/12]
> > MdeModulePkg/PciBusDxe:New PCI features separation with PCD
> >
> > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2194
> >
> > Definition of bit masks for the new PCD for the following new PCI feature
> > set:-
> > 1.	Maximum Payload Size (MPS)
> > 2.	Maximum Read Request Size (MRRS)
> > 3.	Completion Timeout (CTO)
> > 4.	Relax Order (RO) Enable
> > 5.	No Snoop (NS) Enable
> > 6.	Extended Tag
> > 7.	ASPM support
> > 8.	Common Clock Configuration
> > 9.	Extended SYNC
> > 10.	Atomic Op
> > 11.	LTR Enable
> > 12.	PTM support
> >
> > Code changes made to the PCI Bus driver to adopt to these new PCD defini- tion,
> > helper routines defined for features that needs to be supported in.
> > future.
> >
> > Signed-off-by: Ashraf Javeed <ashraf.javeed@intel.com>
> > Cc: Jian J Wang <jian.j.wang@intel.com>
> > Cc: Hao A Wu <hao.a.wu@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > ---
> >  MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf       |   5 ++++-
> >  MdeModulePkg/Bus/Pci/PciBusDxe/PciFeatureSupport.c | 177
> > +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > +++++++++++++++++++++++++++++++++++++++++++++++
> >  MdeModulePkg/Bus/Pci/PciBusDxe/PciFeatureSupport.h |  26
> > ++++++++++++++++++++++++++
> >  MdeModulePkg/MdeModulePkg.dec                      |  22
> > ++++++++++++++++++++++
> >  4 files changed, 229 insertions(+), 1 deletion(-)
> >
> > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
> > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
> > index 05c2202..6dab970 100644
> > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
> > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
> > @@ -2,7 +2,7 @@
> >  #  The PCI bus driver will probe all PCI devices and allocate MMIO and IO space
> > for these devices.
> >  #  Please use PCD feature flag PcdPciBusHotplugDeviceSupport to enable hot
> > plug supporting.
> >  #
> > -#  Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
> > +#  Copyright (c) 2006 - 2019, Intel Corporation. All rights
> > +reserved.<BR>
> >  #
> >  #  SPDX-License-Identifier: BSD-2-Clause-Patent  # @@ -57,6 +57,8 @@
> >    PciCommand.h
> >    PciIo.h
> >    PciBus.h
> > +  PciFeatureSupport.c
> > +  PciFeatureSupport.h
> >
> >  [Packages]
> >    MdePkg/MdePkg.dec
> > @@ -104,6 +106,7 @@
> >    gEfiMdeModulePkgTokenSpaceGuid.PcdAriSupport                  ## CONSUMES
> >    gEfiMdeModulePkgTokenSpaceGuid.PcdMrIovSupport                ## CONSUMES
> >    gEfiMdeModulePkgTokenSpaceGuid.PcdPciDisableBusEnumeration    ##
> > SOMETIMES_CONSUMES
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdOtherPciFeatures            ##
> > CONSUMES
> >
> >  [UserExtensions.TianoCore."ExtraFiles"]
> >    PciBusDxeExtra.uni
> > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciFeatureSupport.c
> > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciFeatureSupport.c
> > new file mode 100644
> > index 0000000..8be227a
> > --- /dev/null
> > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciFeatureSupport.c
> > @@ -0,0 +1,177 @@
> > +/** @file
> > +  PCI standard feature support functions implementation for PCI Bus module..
> > +
> > +Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
> > +SPDX-License-Identifier: BSD-2-Clause-Patent
> > +
> > +**/
> > +
> > +#include "PciBus.h"
> > +#include "PciFeatureSupport.h"
> > +
> > +/**
> > +  Main routine to indicate whether the platform has selected the
> > +Max_Payload_Size
> > +  PCI feature to be configured by this driver
> > +
> > +  @retval TRUE    platform has selected the Max_Payload_Size to be configured
> > +          FALSE   platform has not selected this feature
> > +**/
> > +BOOLEAN
> > +SetupMaxPayloadSize (
> > +  )
> > +{
> > +  return (PcdGet32 (PcdOtherPciFeatures) &
> > +PCI_FEATURE_SUPPORT_FLAG_MPS) ? TRUE : FALSE; }
> > +
> > +/**
> > +  Main routine to indicate whether the platform has selected the
> > +Max_Read_Req_Size
> > +  PCI feature to be configured by this driver
> > +
> > +  @retval TRUE    platform has selected the Max_Read_Req_Size to be
> > configured
> > +          FALSE   platform has not selected this feature
> > +**/
> > +BOOLEAN
> > +SetupMaxReadReqSize (
> > +  )
> > +{
> > +  return (PcdGet32 (PcdOtherPciFeatures) &
> > +PCI_FEATURE_SUPPORT_FLAG_MRRS) ? TRUE : FALSE; }
> > +
> > +/**
> > +  Main routine to indicate whether the platform has selected the Relax
> > +Ordering
> > +  PCI feature to be configured by this driver
> > +
> > +  @retval TRUE    platform has selected the Relax Ordering to be configured
> > +          FALSE   platform has not selected this feature
> > +**/
> > +BOOLEAN
> > +SetupRelaxOrder (
> > +  )
> > +{
> > +  return (PcdGet32 (PcdOtherPciFeatures) & PCI_FEATURE_SUPPORT_FLAG_RO)
> > +? TRUE : FALSE; }
> > +
> > +/**
> > +  Main routine to indicate whether the platform has selected the
> > +No-Snoop
> > +  PCI feature to be configured by this driver
> > +
> > +  @retval TRUE    platform has selected the No-Snoop to be configured
> > +          FALSE   platform has not selected this feature
> > +**/
> > +BOOLEAN
> > +SetupNoSnoop (
> > +  )
> > +{
> > +  return (PcdGet32 (PcdOtherPciFeatures) & PCI_FEATURE_SUPPORT_FLAG_NS)
> > +? TRUE : FALSE; }
> > +
> > +/**
> > +  Main routine to indicate whether the platform has selected the
> > +Completion Timeout
> > +  PCI feature to be configured by this driver
> > +
> > +  @retval TRUE    platform has selected the Completion Timeout to be
> > configured
> > +          FALSE   platform has not selected this feature
> > +**/
> > +BOOLEAN
> > +SetupCompletionTimeout (
> > +  )
> > +{
> > +  return (PcdGet32 (PcdOtherPciFeatures) &
> > +PCI_FEATURE_SUPPORT_FLAG_CTO) ? TRUE : FALSE; }
> > +
> > +/**
> > +  Main routine to indicate whether the platform has selected the
> > +Extended Tag
> > +  PCI feature to be configured by this driver
> > +
> > +  @retval TRUE    platform has selected the Completion Timeout to be
> > configured
> > +          FALSE   platform has not selected this feature
> > +**/
> > +BOOLEAN
> > +SetupExtendedTag (
> > +  )
> > +{
> > +  return (PcdGet32 (PcdOtherPciFeatures) &
> > +PCI_FEATURE_SUPPORT_FLAG_ETAG) ? TRUE : FALSE; }
> > +
> > +/**
> > +  Main routine to indicate whether the platform has selected the Atomic
> > +Op
> > +  PCI feature to be configured by this driver
> > +
> > +  @retval TRUE    platform has selected the Completion Timeout to be
> > configured
> > +          FALSE   platform has not selected this feature
> > +**/
> > +BOOLEAN
> > +SetupAtomicOp (
> > +  )
> > +{
> > +  return (PcdGet32 (PcdOtherPciFeatures) &
> > +PCI_FEATURE_SUPPORT_FLAG_AOP) ? TRUE : FALSE; }
> > +/**
> > +  Main routine to indicate whether the platform has selected the LTR
> > +  PCI feature to be configured by this driver
> > +
> > +  @retval TRUE    platform has selected the Completion Timeout to be
> > configured
> > +          FALSE   platform has not selected this feature
> > +**/
> > +BOOLEAN
> > +SetupLtr (
> > +  )
> > +{
> > +  return (PcdGet32 (PcdOtherPciFeatures) &
> > +PCI_FEATURE_SUPPORT_FLAG_LTR) ? TRUE : FALSE; }
> > +
> > +/**
> > +  Main routine to indicate whether the platform has selected the ASPM
> > +state
> > +  PCI feature to be configured by this driver
> > +
> > +  @retval TRUE    platform has selected the Completion Timeout to be
> > configured
> > +          FALSE   platform has not selected this feature
> > +**/
> > +BOOLEAN
> > +SetupAspm (
> > +  )
> > +{
> > +  return (PcdGet32 (PcdOtherPciFeatures) &
> > +PCI_FEATURE_SUPPORT_FLAG_ASPM) ? TRUE : FALSE; }
> > +
> > +/**
> > +  Main routine to indicate whether the platform has selected the Common
> > +Clock Configuration
> > +  PCI feature to be configured by this driver
> > +
> > +  @retval TRUE    platform has selected the Completion Timeout to be
> > configured
> > +          FALSE   platform has not selected this feature
> > +**/
> > +BOOLEAN
> > +SetupCommonClkCfg (
> > +  )
> > +{
> > +  return (PcdGet32 (PcdOtherPciFeatures) &
> > +PCI_FEATURE_SUPPORT_FLAG_CCC) ? TRUE : FALSE; }
> > +
> > +/**
> > +  Main routine to indicate whether the platform has selected the
> > +Extended Synch
> > +  PCI feature to be configured by this driver
> > +
> > +  @retval TRUE    platform has selected the Completion Timeout to be
> > configured
> > +          FALSE   platform has not selected this feature
> > +**/
> > +BOOLEAN
> > +SetupExtendedSynch (
> > +  )
> > +{
> > +  return (PcdGet32 (PcdOtherPciFeatures) &
> > +PCI_FEATURE_SUPPORT_FLAG_ESYN) ? TRUE : FALSE; }
> > +
> > +/**
> > +  Main routine to indicate whether the platform has selected the PIM
> > +Control
> > +  PCI feature to be configured by this driver
> > +
> > +  @retval TRUE    platform has selected the Completion Timeout to be
> > configured
> > +          FALSE   platform has not selected this feature
> > +**/
> > +BOOLEAN
> > +SetupPtm (
> > +  )
> > +{
> > +  return (PcdGet32 (PcdOtherPciFeatures) &
> > +PCI_FEATURE_SUPPORT_FLAG_PTM) ? TRUE : FALSE; }
> > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciFeatureSupport.h
> > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciFeatureSupport.h
> > new file mode 100644
> > index 0000000..d06a5e8
> > --- /dev/null
> > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciFeatureSupport.h
> > @@ -0,0 +1,26 @@
> > +/** @file
> > +  PCI standard feature support functions implementation for PCI Bus module..
> > +
> > +Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
> > +SPDX-License-Identifier: BSD-2-Clause-Patent
> > +
> > +**/
> > +
> > +#ifndef _EFI_PCI_FEATURES_SUPPORT_H_
> > +#define _EFI_PCI_FEATURES_SUPPORT_H_
> > +//
> > +// Macro definitions for the PCI Features support PCD // #define
> > +PCI_FEATURE_SUPPORT_FLAG_MPS  BIT0 #define
> > +PCI_FEATURE_SUPPORT_FLAG_MRRS BIT1
> > +#define PCI_FEATURE_SUPPORT_FLAG_RO   BIT2
> > +#define PCI_FEATURE_SUPPORT_FLAG_NS   BIT3
> > +#define PCI_FEATURE_SUPPORT_FLAG_CTO  BIT4 #define
> > +PCI_FEATURE_SUPPORT_FLAG_ETAG BIT5 #define
> > PCI_FEATURE_SUPPORT_FLAG_AOP
> > +BIT6 #define PCI_FEATURE_SUPPORT_FLAG_LTR  BIT7 #define
> > +PCI_FEATURE_SUPPORT_FLAG_ASPM BIT12 #define
> > +PCI_FEATURE_SUPPORT_FLAG_CCC  BIT13 #define
> > +PCI_FEATURE_SUPPORT_FLAG_ESYN BIT14 #define
> > +PCI_FEATURE_SUPPORT_FLAG_PTM  BIT20 #endif
> > diff --git a/MdeModulePkg/MdeModulePkg.dec
> > b/MdeModulePkg/MdeModulePkg.dec index 12e0bbf..ed82e85 100644
> > --- a/MdeModulePkg/MdeModulePkg.dec
> > +++ b/MdeModulePkg/MdeModulePkg.dec
> > @@ -1036,6 +1036,28 @@
> >    # @Prompt Enable UEFI Stack Guard.
> >
> > gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard|FALSE|BOOLEAN|0x30
> > 001055
> >
> > +  ## This PCD is to indicate the PCI Bus driver to setup other new PCI features.
> > +  #  Each PCI feature is represented by its mask bit position and it
> > + configures  #  if that bit is set.
> > +  #
> > +  #   Bit 0 - if set, the PCI Bus driver programs the device's
> > Max_Payload_Size.<BR>
> > +  #   Bit 1 - if set, the PCI Bus driver programs the device's
> > Max_Read_Req_Size.<BR>
> > +  #   Bit 2 - if set, the PCI Bus driver programs the device's Relax Ordering
> > state.<BR>
> > +  #   Bit 3 - if set, the PCI Bus driver programs the device's No-Snoop state.<BR>
> > +  #   Bit 4 - if set, the PCI Bus driver programs the device's Completion Timeout
> > range.<BR>
> > +  #   Bit 5 - if set, the PCI Bus driver programs the device's Extended Tag
> > range.<BR>
> > +  #   Bit 6 - if set, the PCI Bus driver programs the device's AtomicOp
> > feature.<BR>
> > +  #   Bit 7 - if set, the PCI Bus driver programs the device's LTR feature.<BR>
> > +  #   Bit 8 to 11 - Reserved for future use by the PCI Bus driver.<BR>
> > +  #   Bit 12 - if set, the PCI Bus driver programs the PCIe link ASPM state.<BR>
> > +  #   Bit 13 - if set, the PCI Bus driver programs the PCIe link Common Clock
> > Configuration.<BR>
> > +  #   Bit 14 - if set, the PCI Bus driver programs the PCIe link Extended Synch
> > state.<BR>
> > +  #   Bit 15 to 19 - Reserved for future use by the PCI Bus driver.<BR>
> > +  #   Bit 20 - if set, the PCI Bus driver programs the device's PTM feature.<BR>
> > +  #   Bit 21 to 31 - Reserved for future use by the PCI Bus driver.<BR>
> > +  # @Prompt The UEFI PCI Bus driver enables the new set of other PCI Features.
> > +
> > +
> > gEfiMdeModulePkgTokenSpaceGuid.PcdOtherPciFeatures|0x001070FF|UINT32|
> > 0
> > + x30001056
> > +
> >  [PcdsFixedAtBuild, PcdsPatchableInModule]
> >    ## Dynamic type PCD can be registered callback function for Pcd setting
> > action.
> >    #  PcdMaxPeiPcdCallBackNumberPerPcdEntry indicates the maximum number
> > of callback function
> > --
> > 2.21.0.windows.1
> >
> >
> >