Re: [edk2-devel] [edk2-staging/UEFI_PCI_ENHANCE-2 PATCH 02/12] MdeModulePkg/PciBusDxe: Setup PCI Express init phase

["Javeed, Ashraf" <ashraf.javeed@intel.com>]

> -----Original Message-----
> From: Ni, Ray <ray.ni@intel.com>
> Sent: Monday, February 10, 2020 2:16 PM
> To: Javeed, Ashraf <ashraf.javeed@intel.com>; devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>
> Subject: RE: [edk2-devel] [edk2-staging/UEFI_PCI_ENHANCE-2 PATCH 02/12]
> MdeModulePkg/PciBusDxe: Setup PCI Express init phase
> 
> My proposal defines four phases: Initialize, Scan, Program, Finalize. The four
> phases are very similar to your December's patch with just one phase opt out.
> 
> As below:
> ---BEGIN----
> Each feature needs to provide function pointers for each phase and NULL means
> the feature doesn't need to do anything in the specific phase.
> With that, we can define a structure:
> Typedef struct {
>   BOOLEAN                          Enable;
>   PCIE_FEATURE_INITILAIZE Initialize;
>   PCIE_FEATURE_SCAN  Scan;
>   PCIE_FEATURE_PROGRAM Program;
>   PCIE_FEATURE_FINALIZE Finalize;
> } PCIE_FEATURE_ENTRY;
> 
> With that, we can define a module level global variable:
> PCIE_FEATURE_ENTRY mPcieFeatures[] = {
>   { TRUE, MaxPayloadInitialize, MaxPayloadScan, MaxPayloadProgram,
> MaxPayloadFinalize},
>   { TRUE, MaxReadRequestInitialize, MaxReadRequestScan,
> MaxReadRequestProgram, MaxReadRequestFinalize},
>   { TRUE, NULL, NULL, RelaxOrderProgram, NULL},
>   { TRUE, NULL, CompletionTimeoutScan, CompletionTimeoutProgram, NULL },
>   ...
> };
> 
> PCIE_FEATURE_ENTRY.Enable can be set to FALSE according to the platform
> policy.
> 
> The enable of PCIE features can be written as a feature agnostic for-loop.
> This can make the new feature enabling code easy to add and review.
> ---END----
> 
I have seen this, in essence the concept is adopted, but it has been further enhanced to achieve the objectives.

> While your today's patch defines a different 5-phase as below.
> typedef enum {
  //
  // preprocessing applicable only to few PCI Express features to bind all devices
  // under the common root bridge device (root port), that would be useful to align
  // all devices with a common value. This would be optional phase based on the
  // type of the PCI Express feature to be programmed based on platform policy
  //
>   PciExpressFeaturePreProcessPhase,

  //
  // mandatory phase to setup the PCI Express feature to its applicable attribute,
  // based on its device-specific platform policies, matching with its device capabilities
  //
>   PciExpressFeatureSetupPhase,

  //
  // optional phase primarily to align all devices, specially required when PCI
  // switch is present in the hierarchy, applicable to certain few PCI Express
  // features only
  //
>   PciExpressFeatureEntendedSetupPhase,

  //
  // mandatory programming phase to complete the configuration of the PCI Express
  // features
  //
>   PciExpressFeatureProgramPhase,

  //
  // optional phase to clean up temporary buffers, like those that were prepared
  // during the preprocessing phase above
  //
>   PciExpressFeatureEndPhase
> } PCI_EXPRESS_FEATURE_CONFIGURATION_PHASE;
> 
> I don't think they are very similar. And your new proposal is far different from
> what you proposed in December's patch.
> Did you meet any issues that triggered this new proposal?
> 
The details are as follows:-
(1) Your proposal of data structure PCIE_FEATURE_ENTRY will add up 1 byte for Boolean and 8 * 4 bytes for 4 different types of function pointers. For 10 PCIe features, it is going to take 330 bytes fixed size.
(2) I wanted to optimized to avoid fixed size, added the phase, and feature Id enum (4 bytes each), with just single pointer to function for the feature initialization phase into the data structure PCI_EXPRESS_FEATURE_INITIALIZATION_POINT, since I know that many don't need all the phases for initialization. So, the total size turn outs to be 400 bytes, which is not what I had desired. But, I can further optimized the data structure PCI_EXPRESS_FEATURE_INITIALIZATION_POINT so that its size can be 10 bytes. With this, for 10 PCIe features initialization the routines defined in the "mPciExpressFeatureInitializationList " shall be 250 bytes, shall be less compare to using the PCIE_FEATURE_ENTRY; this can be in next version.
(3) The pointer-to-function name to be used as subphases appears fine for PCIe feature specific action, but not for general action items, like free up of temporary memory allocated resources. Explicit declarations has helped in optimizing a code for both feature specific and for common action items, which the subsequent patches reflect those.
(4) The phase names are matching as per their scope of actions, whose comments in the code section are also captured (above). 
(5) As per the nature of the PCIe feature to align common value among all the PCI devices, every node in the tree has to be traversed twice to arrive at final value for the initialization (especially required for the PCIe switch multi-port device, to connect to other EndPoint devices), and the next phase would be the actual write to the PCI device. The phases for traversing are the - PciExpressFeatureSetupPhase, PciExpressFeatureEntendedSetupPhase; and write operation phase is - PciExpressFeatureProgramPhase.
(6) The preprocessing phase is - PciExpressFeaturePreProcessPhase, where the resources are allocated specific to PCIe feature, that needs to be bind to every root bridge device. 
(7) The final phase is - PciExpressFeatureEndPhase; this phase is use for 2 purpose: (i) to invoke the NotifyDeviceState() to indicate to platform about its current device state after the PCIe feature programming is complete, and (ii) free up the resource allocated in the preprocessing phase.

> Thanks,
> Ray
> 
> > -----Original Message-----
> > From: Javeed, Ashraf <ashraf.javeed@intel.com>
> > Sent: Monday, February 10, 2020 4:33 PM
> > To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io
> > Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> > <hao.a.wu@intel.com>
> > Subject: RE: [edk2-devel] [edk2-staging/UEFI_PCI_ENHANCE-2 PATCH
> > 02/12] MdeModulePkg/PciBusDxe: Setup PCI Express init phase
> >
> > Thanks
> > Ashraf
> >
> > > -----Original Message-----
> > > From: Ni, Ray <ray.ni@intel.com>
> > > Sent: Monday, February 10, 2020 1:07 PM
> > > To: Javeed, Ashraf <ashraf.javeed@intel.com>; devel@edk2.groups.io
> > > Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> > > <hao.a.wu@intel.com>
> > > Subject: RE: [edk2-devel] [edk2-staging/UEFI_PCI_ENHANCE-2 PATCH
> > > 02/12]
> > > MdeModulePkg/PciBusDxe: Setup PCI Express init phase
> > >
> > > > > +      Status = EnumeratePciExpressFeatures (
> > > 1. "enumerate" means "visit". But I think this function is not just
> > > visiting the features but also
> > >      programming them. So, How about "ProgramPciExpressFeatures"?
> > >      (I gave a similar review comment in last time review in Dec.)
> > >
> > Actually I have already used the "ProgramPciExpressFeatures" in one of
> > the sub-phases of the PCI Express initialization code...do you have any other
> name for this? How about "ConfigurePciExpressFeatures"?
> >
> > > 2. In mail https://edk2.groups.io/g/devel/message/52399 I proposed
> > > to simplify to 4 phases.
> > >     Did you find any issue with my proposal?
> > I did simplify to 4 phases, please check. The fifth phase is to report
> > out the device state to the platform through the protocol interface
> "NotifyDeviceState".
> >
> > >
> > > Thanks,
> > > Ray