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

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

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----

While your today's patch defines a different 5-phase as below.
typedef enum {
  PciExpressFeaturePreProcessPhase,
  PciExpressFeatureSetupPhase,
  PciExpressFeatureEntendedSetupPhase,
  PciExpressFeatureProgramPhase,
  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?

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