Re: [edk2-devel] [edk2-staging/UEFI_PCI_ENHANCE-2 PATCH 05/12] PciBusDxe: Setup sub-phases for PCI feature enumeration

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

About the "AtomicOp", I want to retract about the check for the Routing Capability to set the platform policy about the blocking the AtomicOp requests. The implementation seems good.

Thanks
Ashraf

> -----Original Message-----
> From: Javeed, Ashraf
> Sent: Tuesday, April 21, 2020 11:33 AM
> 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
> 05/12] PciBusDxe: Setup sub-phases for PCI feature enumeration
> 
> About the "CompletionTimeout ", I want to retract about the AUTO option;
> the implementation is good and device initialization should be skipped for
> this option.
> 
> Regards
> Ashraf
> 
> > -----Original Message-----
> > From: Javeed, Ashraf <ashraf.javeed@intel.com>
> > Sent: Monday, April 20, 2020 6:53 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>; Javeed, Ashraf <ashraf.javeed@intel.com>
> > Subject: RE: [edk2-devel] [edk2-staging/UEFI_PCI_ENHANCE-2 PATCH
> > 05/12] PciBusDxe: Setup sub-phases for PCI feature enumeration
> >
> > Ray,
> > Sorry for late response as I got into other priorities. Let's try to
> > finish this ASAP.
> > Certainly the implementation of yours has reduced the code size in
> > great deal by deprecating the EFI encodings for the PCIe features, and
> > using the actual HW values along with the global definition of AUTO
> > and NOT_APPLICABLE (as an alias to DO_NOT_TOUCH). I am concerned
> that
> > lot of explanation in the text of the PCI Express Protocol has to be
> > provided about the rules that platform FW developer has to follow
> > w.r.t. AUTO and DO_NOT_TOUCH in the GetDevicePolicy(); that needs to
> > be published in the PI Specification. I shall take your implementation
> > and add the other 3 PCIe features that I had implemented (ExtendedTag,
> > Aspm,
> > CommonClockConfiguration) into this scheme, and also provide how
> these
> > global options rules can be defined for the other non-implemented PCIe
> > features.
> > However, I have other suggestions/corrections for the present 7 PCIe
> > features implementation, which I want to discuss by highlighting the
> > platform policy of AUTO and DO_NOT_DISTURB.
> >
> > (1) MaxPayloadSize:- This feature requires aligning all the PCI
> > devices in the hierarchy to have a common value. (The device policy by
> > platform shall be used to align with all the devices in hierarchy
> > based on their capability, thus value less than capability shall
> > result in all devices in hierarchy supporting that value in capability to be
> programmed).
> > 	(a) AUTO: The DeviceCapability.MaxPayloadSize value is considered
> > while scan all the nodes in the hierarchy
> > 	(b) DO_NOT_TOUCH: Should be considered as invalid value to skip
> for
> > this feature. Two options in this scenario...
> > 		(i) The PciBusDxe shall use this option to force the HW
> default
> > value of 128B
> > 		(ii) Behave same as AUTO.
> > 	I vote for option (i) above.
> >
> > (2) MaxReadReqSize:- Does not need all the devices in hierarchy to be
> > align to common value. However, the spec imposes that for an
> > isochronous configured device its memory read request should be equal
> > to MayPayloadSize; thus I accept your implementation since determining
> > whether that device in Isochronous mode is beyond the scope of
> PciBusDxe
> > 	(a) AUTO: MaxReadReqSize = MaxPayloadSize; shall cover the device
> > configured in Isochronous case.
> > 	(b) DO_NOT_TOUCH: skip programming the DeviceControl register
> of the
> > device
> >
> > (3) RelaxOrdering:- Specific to device to enable or disable; does not
> > require any coherency with all the devices in the hierarchy
> > 	(a) AUTO: device initialization is skipped
> > 	(b) DO_NOT_TOUCH: device initialization is skipped
> >
> > (4) NoSnoop:- Specific to device to enable or disable; does not
> > require any coherency with all the devices in the hierarchy
> > 	(a) AUTO: device initialization is skipped
> > 	(b) DO_NOT_TOUCH: device initialization is skipped
> >
> > (5) CompletionTimeout:-  The range and detection mechanism disable can
> > be set specifically for any device; does not require to be aligned
> > with all devices in the hierarchy
> > 	(a) AUTO: your implementation ignores it (same as do not touch); I
> > propose that platform can provide this option to set the
> > DeviceControl2.CompletionTimeoutValue as per its high range value of
> > DeviceCapability2.CompletionTimeoutRangesSupported, ignoring the
> > CompletionTimeoutDisableSupported. For example; if the
> > DeviceCapability2 supports Ranges A, B, C and D; than its
> > DeviceControl2.CompletionTimeoutValue can be programmed to 1110b
> (17s
> > to 64s) for higher subrange.
> > 	(b) DO_NOT_TOUCH: device initialization is skipped
> > 	Your implementation about the device policy from platform appears
> OK
> > to me.
> >
> > (6) Ltr: As per PCIe specification, the LTR capability is provided for
> > all the PCI devices, disabled by default, and if LTR needs to be
> > enabled for Endpoint device than all the in-between component from
> > Root Port to Endpoint needs to be enabled, and it is not required that
> > all Endpoints have to be LTR enabled.
> > 	(a) AUTO: as per your implementation, this option is ignored as
> > invalid, and overwritten with its child device and if no valid for
> > child device than device initialization is skipped
> > 	(b) DO_NOT_TOUCH: as per your implementation, this option is
> ignored
> > as invalid. and overwritten with its child device and if no valid for
> > child device than device initialization is skipped
> > 	Why does your implementation consider changing only the parent
> N as
> > per its child N+1, and not vice versa? If platform only updates the
> > Root port (RP) and not the Endpoint device than this implementation
> > only programs the RP, right?
> > 	In real scenario, the platform FW does not provide the policy for
> > PCIe Endpoint devices mounted on the PCIe slots since those policies
> > are statically defined to the fixed components of the chip. Thus,
> > platform expects enabling the PCIe Root Port LTR mechanism, in hope
> > that its valid PCIe Endpoint device gets enabled if all the devices
> > are capable of LTR mechanism.
> > 	I propose, even if platform provides one device to be LTR enable in
> > the path from Root Port to Endpoint device, than PciBusDxe can enable
> > all the devices in the path if all are LTR capable. This shall be also
> > for Switch with multiple downstream ports with each port connected to
> > independent Endpoint devices, if platform provide LTR enable to Root
> > Port than enable for each path of devices if the downstream port's
> > Endpoint device is LTR capable. If any one downstream port's Endpoint
> > device is not capable than it need not set for that device.
> >
> > (7) AtomicOp: As per PCIe specification, the DeviceControl2 register
> > provides the AtomicOp Requester Enable bit (6) and blocking of
> > AtomicOp request bit (7) for the Port device.
> > 	The AtomicOp requester capability determination is out of scope,
> > hence it can be just relied on platform policy request. However, the
> > blocking AtomicOp request on the port device depends on the Device
> > Routing capability bit defined in the DeviceCapability2 register.
> > 	(a) AUTO: device initialization is skipped
> > 	(b) DO_NOT_TOUCH: device initialization is skipped
> > 	I think the change required in this implementation is the check for
> > the Routing Capability to enable the blocking of AtomicOp request for
> > the port device; do you agree?
> >
> > Thanks
> > Ashraf
> >
> > > -----Original Message-----
> > > From: Ni, Ray <ray.ni@intel.com>
> > > Sent: Tuesday, March 17, 2020 9: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
> > > 05/12] PciBusDxe: Setup sub-phases for PCI feature enumeration
> > >
> > > Ashraf,
> > > Thank you for the prompt response!
> > >
> > > My response in below.
> > > (I removed some earlier conversations, but the context is still
> > > kept.)
> > >
> > > > > I take it as an agree of using the global AUTO. Thanks for that.
> > > > Please note that there is another request of "Do not touch", and I
> > > > am still contemplating whether both AUTO and DO_NOT_TOUCH
> > options
> > > could
> > > > be applicable to all the PCIe features or not. It could be
> > > > possible to take
> > > AUTO for some of them and DO_NOT_TOUCH for others. Need to
> > consider
> > > each PCIe feature separately for both these options.
> > >
> > >
> > > I think we are basically aligned on this.
> > > I agree that each PCIe feature should have clear statement about how
> > > to interpret AUTO and DO_NOT_TOUCH.
> > > DO_NOT_TOUCH in my code change is NOT_APPLICABLE.
> > > If you check some of the features I already implemented, the
> > > comments say clearly what AUTO and NOT_APPLICABLE mean for that
> > > specific
> > feature.
> > > Looking forward to your comments on how to interpret AUTO and
> > > NOT_APPLICABLE for each PCIe feature.
> > >
> > > > > Supposing a root bridge contains 1 RCiEP and two root ports, the
> > > > > implementation ensures there will be 3 separate contexts for each.
> > > > > As you can see, every time the code starts enumeration from a
> > > > > RCiEP or a root port, a new Context[] is setup. Context[i] is
> > > > > associated with the
> > > feature _i_.
> > > > >
> > > > I realize now, that there are 3 nested for-loops in the
> > > > EnumerateRootBridgePcieFeatures(), the first one enables the
> > > > processing of first level nodes of the Root Bridge instance, and
> > > > in this way
> > > the context[i] used for each node will be valid as it covers each
> > > node's hierarchy.
> > >
> > > I think we are aligned on this.
> > >
> > > > >
> > > > > The level N and N+1 is needed for AtomicOp or Ltr feature (which
> > > > > I need to go back to check the code).
> > > > > I want to avoid the individual feature scan/program routine
> > > > > checks device's parent/children.
> > > > > I understand that in PCIE spec, a switch consists of an upstream
> > > > > port and several downstream ports.
> > > > > It means a switch populates a bridge which connects to the
> > > > > upstream and several bridges under the upstream bridge which
> > > > > connect to the downstream.
> > > > > But from software perspective, I don't think we should care
> > > > > about
> > that.
> > > > > Maybe I am wrong. Can you please give a real example that
> > > > > treating in my way would cause some issues?
> > > > >
> > > > Let just say we have PCIe switch connected to Root Port on a host,
> > > > and it has 2 downstream ports, each connected to one Endpoint
> > > > device
> > each.
> > > > Platform has selected to enable LTR for second downstream port's
> > > > endpoint device, not both. As per your code logic of LTR, where
> > > > you use the N and N+1 to compare between the parent and child,
> > > > will not be
> > > applicable if you use the same condition to check downstream port
> > > (N+1), and its parent upstream port, because N would be the Endpoint
> > > device of first downstream port of the PCIe switch and not its
> > > upstream
> > port.
> > > > Root Port ->					1|0|0
> > > > Switch upstream port ->				2|0|0 (M-1)
> > > > Switch downstream ports ->	3|0|0,(M)		5|0|0 (N
> > (=M+2))
> > > > Endpoint devices ->		4|0|0 (M+1)		6|0|0 (N+1
> > (=M+3))
> > > > The level+1 logic when the EnumeratePcieDevices() is called
> > > > recursively, will result in one value greater than the Endpoint
> > > > device for
> > > the second downstream port.
> > >
> > > The level value in my code will be:
> > > Root Port ->					1|0|0 (level 0)
> > > Switch upstream port ->				2|0|0 (level 1)
> > > Switch downstream ports ->	3|0|0,(level 2)		5|0|0,(level
> 2)
> > > Endpoint devices ->		4|0|0,(level 3)		6|0|0,(level
> 3)
> > >
> > > Level of device 5|0|0 is still 2, not 4.
> > >
> > > I also did the unit test to ensure the LTR feature logic is good.
> > >
> > > For below device hierarchy with LTR enabled in 5/0/0 and 11/0/0,
> > > disabled in 9/0/0 The LTR setting for upstream bridges should be as
> > following:
> > >                     0/4/0[LTR:1]
> > >   2/0/0[1]  |       2/1/0[1]         | 2/2/0[0]
> > >   3/0/0[1]  |       7/0/0[1]
> > >   4/0/0[1]  |       8/0/0[1]
> > >   5/0/0[1]  | 9/0/0[0]   9/1/0[1]
> > >                          10/0/0[1]
> > >                          11/0/0[1]
> > >
> > > The unit test code is in commit "Test case for LTR".
> > >
> > > >
> > > > > >
> > > > > > I liked the Pre & Post order flag that is used to processing
> > > > > > parent-to-child
> > > > > vs. child-to-parent nodes.
> > > > >
> > > > > Actually I didn't have this flag in the first version of my code
> > > > > change. But later I found it's needed for some features like LTR
> > > > > which needs to be programmed from rootbridge to endpoint
> > according
> > > to spec.
> > > > >
> > > > >
> > > > > > You are calling the first
> > > > > > and last features as fakes, when you actually parse all the
> > > > > > nodes for the GetDevicePolicy() and NotifyDeviceState(); to me
> > > > > > it is actually an un-named phase where all the nodes are
> > > > > > queried or informed to the
> > > > > platform; that seems fine to save the NULL on the
> > > > > mPcieFeatures[]
> > > table.
> > > > >
> > > > > Yes I put GetDevicePolicy() and NotifyDeviceState() in the
> > > > > feature array in my first version of code. And later I called
> > > > > the two separately for better code readability. But it was my
> > > > > fault that I didn't update the commit message. It caused confusion
> to you.
> > > > >
> > > > > >
> > > > > > Thanks
> > > > > > Ashraf
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Ni, Ray <ray.ni@intel.com>
> > > > > > > Sent: Thursday, March 5, 2020 7:43 PM
> > > > > > > To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>;
> > > > > > > Javeed, Ashraf <ashraf.javeed@intel.com>
> > > > > > > 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
> > > > > > > 05/12] PciBusDxe: Setup sub-phases for PCI feature
> > > > > > > enumeration
> > > > > > >
> > > > > > > Ashraf,
> > > > > > > I think it might be better to describe my review comments
> > > > > > > with code implementation.
> > > > > > > Can you please check this branch where I did some
> > > > > > > modification based on your code?
> > > > > > > https://github.com/niruiyu/edk2/tree/pci/pcie2
> > > > > > >
> > > > > > > Let's firstly align on the feature initialization framework
> > > > > implementation.
> > > > > > > To be specific, this commit:
> > > > > > > MdeModulePkg/PciBus: Add the framework to init PCIE features
> > > > > > >
> > > > >
> > >
> >
> https://github.com/niruiyu/edk2/commit/9fb9a3dcef06de98a76825e2fc0
> > > > > > > 7167446ee6fd9
> > > > > > >
> > > > > > > Thanks,
> > > > > > > Ray
> > > > > > >
> > > > > > > > -----Original Message-----
> > > > > > > > From: devel@edk2.groups.io <devel@edk2.groups.io> On
> > > > > > > > Behalf
> > > Of
> > > > > Ni,
> > > > > > > Ray
> > > > > > > > Sent: Thursday, December 19, 2019 1:49 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 05/12]
> > > > > > > > PciBusDxe: Setup sub-phases for PCI feature enumeration
> > > > > > > >
> > > > > > > > After I reviewed the patch of enabling MaxPayloadSize,
> > > > > > > > MaxReadReqSize and more PCIE features, I can now
> > > > > > > > understand the phases more than earlier.
> > > > > > > >
> > > > > > > > Your patch proposed five phases:
> > > > > > > >   //
> > > > > > > >   // initial phase in configuring the other PCI features
> > > > > > > > to record the
> > > > > > > primary
> > > > > > > >   // root ports
> > > > > > > >   //
> > > > > > > >   PciFeatureRootBridgeScan,
> > > > > > > >   //
> > > > > > > >   // get the PCI device-specific platform policies and
> > > > > > > > align with device
> > > > > > > capabilities
> > > > > > > >   //
> > > > > > > >   PciFeatureGetDevicePolicy,
> > > > > > > >   //
> > > > > > > >   // align all PCI nodes in the PCI heirarchical tree
> > > > > > > >   //
> > > > > > > >   PciFeatureSetupPhase,
> > > > > > > >   //
> > > > > > > >   // finally override to complete configuration of the PCI
> feature
> > > > > > > >   //
> > > > > > > >   PciFeatureConfigurationPhase,
> > > > > > > >   //
> > > > > > > >   // PCI feature configuration complete
> > > > > > > >   //
> > > > > > > >   PciFeatureConfigurationComplete
> > > > > > > >
> > > > > > > >
> > > > > > > > I have several comments to the five phases.
> > > > > > > > 1. Scan phase is not do the scanning but creates a list to
> > > > > > > > hold all root ports under the root bridge.
> > > > > > > > 2. Root ports collection is not required by all of the
> > > > > > > > features, only by MPS and MRRS.
> > > > > > > > But the collection always happens even when platform
> > > > > > > > doesn't require PciBus to initialize MPS or MRRS.
> > > > > > > > 3. GetDevicePolicy phase is not just call the
> > > > > > > > GetDevicePolicy for each device. It also reads the PCIE
> > > > > > > > configuration space to get the device's feature related
> > > > > > > > capabilities, for some of the
> > > features.
> > > > > > > >
> > > > > > > > With that, I propose to define 4 phases:
> > > > > > > > 1. Initialize phase
> > > > > > > > This phase is similar to your Scan phase.
> > > > > > > > For some features, this phase does nothing.
> > > > > > > > For MPS and MRRS, this phase creates a list holding all
> > > > > > > > root
> > ports.
> > > > > > > >
> > > > > > > > 2. Scan phase
> > > > > > > > This phase is similar to your GetDevicePolicy phase.
> > > > > > > > For some features, this phase needs nothing do to.
> > > > > > > > For MPS and MRRS, this phase scan all devices and get the
> > > > > > > > aligned value of MPS or MRRS.
> > > > > > > >
> > > > > > > > 3. Program phase or Configuration phase This phase is
> > > > > > > > similar to your Configuration phase.
> > > > > > > > The Setup phase can be merged to this phase.
> > > > > > > >
> > > > > > > > 4. Finalize phase.
> > > > > > > > This phase is similar to your ConfigurationComplete phase.
> > > > > > > > This phase frees the resources occupied/allocated in
> > > > > > > > Initialize
> > > phase.
> > > > > > > > For some of the features, this phase may do nothing.
> > > > > > > >
> > > > > > > > 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.
> > > > > > > >
> > > > > > > >
> > > > > > > > > -----Original Message-----
> > > > > > > > > From: Javeed, Ashraf <ashraf.javeed@intel.com>
> > > > > > > > > Sent: Wednesday, December 18, 2019 3:14 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
> > > > > > > > > 05/12]
> > > > > > > > > PciBusDxe: Setup sub-phases for PCI feature enumeration
> > > > > > > > >
> > > > > > > > > Thanks for the review, Ray!
> > > > > > > > > My response in line
> > > > > > > > >
> > > > > > > > > > -----Original Message-----
> > > > > > > > > > From: Ni, Ray <ray.ni@intel.com>
> > > > > > > > > > Sent: Tuesday, December 17, 2019 5:26 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
> > > > > > > > > 05/12]
> > > > > > > > > > PciBusDxe: Setup sub-phases for PCI feature
> > > > > > > > > > enumeration
> > > > > > > > > >
> > > > > > > > > > Please check comments below.
> > > > > > > > > > I may have more comments regarding to the four phases
> > > > > > > > > > after I finish
> > > > > > > > > review of
> > > > > > > > > > further patches.
> > > > > > > > > >
> > > > > > > > > > Besides the comments below, I have a general comments
> > > > > > > > > > to the debug
> > > > > > > > > > message: can you please review the existing debug
> > > > > > > > > > message in the PciBus
> > > > > > > > > driver
> > > > > > > > > > and make sure your newly added debug message aligns to
> > > > > > > > > > existing
> > > > > > > style.
> > > > > > > > > And try
> > > > > > > > > > to use less lines of debug messages with still enough
> > > > > > > > > > debug
> > > > > > > information.
> > > > > > > > > Ok, will look into that.
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > > > +PRIMARY_ROOT_PORT_NODE
> > > > > > > *mPrimaryRootPortList;
> > > > > > > > > > > > +
> > > > > > > > > > > > +/**
> > > > > > > > > > > > +  A global pointer to
> > > > > > > > > PCI_FEATURE_CONFIGURATION_COMPLETION_LIST,
> > > > > > > > > > > > which
> > > > > > > > > > > > +stores all
> > > > > > > > > > > > +  the PCI Root Bridge instances that are
> > > > > > > > > > > > +enumerated for the other PCI features,
> > > > > > > > > > > > +  like MaxPayloadSize & MaxReadReqSize; during
> > > > > > > > > > > > +the the
> > > > > > > > > > > > +Start() interface of the
> > > > > > > > > > > > +  driver binding protocol. The records pointed by
> > > > > > > > > > > > +this pointer would be destroyed
> > > > > > > > > > > > +  when the DXE core invokes the Stop() interface.
> > > > > > > > > > > > +**/
> > > > > > > > > > > > +PCI_FEATURE_CONFIGURATION_COMPLETION_LIST
> > > > > > > > > > > > *mPciFeaturesConfigurationCompletionList = NULL;
> > > > > > > > > >
> > > > > > > > > > 1. Please follow existing linked list usage style. The
> > > > > > > > > > first node in the list is an empty header node.
> > > > > > > > > >
> > > > > > > > > > LIST_ENTRY   mPrimaryRootPortList;
> > > > > > > > > > LIST_ENTRY   mPciFeaturesConfigurationCompletionList;
> > > > > > > > > >
> > > > > > > > > Ok, will make the change when I incorporate the ECR 0.75
> > > > > > > > > or greater
> > > > > > > version.
> > > > > > > > >
> > > > > > > > > > > > +BOOLEAN
> > > > > > > > > > > > +CheckPciFeatureConfigurationRecordExist (
> > > > > > > > > > > > +  IN  PCI_IO_DEVICE                             *RootBridge,
> > > > > > > > > > > > +  OUT
> > PCI_FEATURE_CONFIGURATION_COMPLETION_LIST
> > > > > > > > > > > > +**PciFeatureConfigRecord
> > > > > > > > > > > > +  )
> > > > > > > > > >
> > > > > > > > > > 2. Is this function to check whether the PCIE features
> > > > > > > > > > under a root bridge is already initialized?
> > > > > > > > > > Can you use the existing variable gFullEnumeration?
> > > > > > > > > > The variable is set to TRUE when the enumeration is
> > > > > > > > > > done to a host bridge
> > > > > > > > > in the
> > > > > > > > > > first time.
> > > > > > > > > > By using gFullEnumeration, the entire function is not
> needed.
> > > > > > > > > >
> > > > > > > > > Ok, will look into this.
> > > > > > > > >
> > > > > > > > > > > > +EFI_STATUS
> > > > > AddRootBridgeInPciFeaturesConfigCompletionList (
> > > > > > > > > > > > +  IN PCI_IO_DEVICE          *RootBridge,
> > > > > > > > > > > > +  IN BOOLEAN                ReEnumerationRequired
> > > > > > > > > > > > +  )
> > > > > > > > > >
> > > > > > > > > > 3. Same question as #2. I think by using
> > > > > > > > > > gFullEnumeration, this function is
> > > > > > > > > not
> > > > > > > > > > needed.
> > > > > > > > > >
> > > > > > > > > OK
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > > > +BOOLEAN
> > > > > > > > > > > > +IsPciRootPortEmpty (
> > > > > > > > > > > > +  IN  PCI_IO_DEVICE                           *PciDevice
> > > > > > > > > > > > +  )
> > > > > > > > > >
> > > > > > > > > > 4. Please use IsListEmpty() directly from callers and
> > > > > > > > > > remove this
> > > > > > > function.
> > > > > > > > > >
> > > > > > > > > Will consider this.
> > > > > > > > >
> > > > > > > > > > > > +**/
> > > > > > > > > > > > +EFI_STATUS
> > > > > > > > > > > > +EnumerateOtherPciFeatures (
> > > > > > > > > >
> > > > > > > > > > 5. Can it be "EnumeratePcieFeatures"?
> > > > > > > > > >
> > > > > > > > > Yes, with the change to ECR 0.75, this routine name
> > > > > > > > > shall be
> > > > > changed.
> > > > > > > > >
> > > > > > > > > > > > +  IN PCI_IO_DEVICE          *RootBridge
> > > > > > > > > > > > +  )
> > > > > > > > > > > > +{
> > > > > > > > > > > > +  EFI_STATUS            Status;
> > > > > > > > > > > > +  UINTN                 OtherPciFeatureConfigPhase;
> > > > > > > > > > > > +
> > > > > > > > > > > > +  //
> > > > > > > > > > > > +  // check on PCI features configuration is
> > > > > > > > > > > > + complete and re-enumeration is required  //  if
> > > > > > > > > > > > + (!CheckPciFeaturesConfigurationRequired
> > > > > > > > > > > > + (RootBridge)) {
> > > > > > > > > > > > +    return EFI_ALREADY_STARTED;  }
> > > > > > > > > > > > +
> > > > > > > > > >  > > +  CHAR16                *Str;
> > > > > > > > > > > > +  Str = ConvertDevicePathToText (
> > > > > > > > > > > > +          DevicePathFromHandle (RootBridge->Handle),
> > > > > > > > > > > > +          FALSE,
> > > > > > > > > > > > +          FALSE
> > > > > > > > > > > > +        );
> > > > > > > > > > > > +  DEBUG ((DEBUG_INFO, "Enumerating PCI features
> > > > > > > > > > > > + for Root Bridge %s\n", Str != NULL ? Str :
> > > > > > > > > > > > + L""));
> > > > > > > > > >
> > > > > > > > > > 6. Please use DEBUG_CODE macro to include
> > > > > > > > > > ConvertDevicePathToText()
> > > > > > > > > and
> > > > > > > > > > DEBUG().
> > > > > > > > > > Please remember to call FreePool().
> > > > > > > > > >
> > > > > > > > > Ok, will can under DEBUG_CODE, and free pool is called
> > > > > > > > > in the end
> > > > > > > > >
> > > > > > > > > > > > +
> > > > > > > > > > > > +  for ( OtherPciFeatureConfigPhase =
> > > > > PciFeatureRootBridgeScan
> > > > > > > > > > > > +      ; OtherPciFeatureConfigPhase <=
> > > > > > > > > PciFeatureConfigurationComplete
> > > > > > > > > > > > +      ; OtherPciFeatureConfigPhase++
> > > > > > > > > > > > +      ) {
> > > > > > > > > > > > +    switch (OtherPciFeatureConfigPhase){
> > > > > > > > > > > > +      case  PciFeatureRootBridgeScan:
> > > > > > > > > > > > +        SetupPciFeaturesConfigurationDefaults ();
> > > > > > > > > > > > +        //
> > > > > > > > > > > > +        //first scan the entire root bridge
> > > > > > > > > > > > + heirarchy for the primary PCI root
> > > > > > > > > > > ports
> > > > > > > > > > > > +        //
> > > > > > > > > > > > +        RecordPciRootPortBridges (RootBridge);
> > > > > > > > > >
> > > > > > > > > > 7. How about "RecordPciRootPorts (RootBridge)"? The
> > > "Bridges"
> > > > > > > > > > suffix is a
> > > > > > > > > bit
> > > > > > > > > > confusing.
> > > > > > > > > >
> > > > > > > > > Fine, will change.
> > > > > > > > >
> > > > > > > > > > > > +      case  PciFeatureGetDevicePolicy:
> > > > > > > > > > > > +      case  PciFeatureSetupPhase:
> > > > > > > > > >
> > > > > > > > > > 8. In SetupPciFeatures(), why do you need to call
> > DeviceExist()?
> > > > > > > > > > Did you see any case that a device is detected in the
> > > > > > > > > > beginning of PciBus
> > > > > > > > > scan
> > > > > > > > > > but is hidden when calling SetupPciFeatures()?
> > > > > > > > > >
> > > > > > > > > Yes, that is the case; device detected during the
> > > > > > > > > beginning of PciBus scan appears to be hidden by the
> > > > > > > > > platform drivers, since numerous legacy callbacks are
> > > > > > > > > initiated at different phase of PCI enumeration to the
> > > > > > > > > PCI Host Bridge, and
> > > PciPlatform drivers.
> > > > > > > > > This can be avoided if the PciBus driver is enhanced to
> > > > > > > > > check for PCI device existence before the publication of
> > > > > > > > > the PCI IO Protocol, and removal of the PCI_IO_DEVICE
> > > > > > > > > instance from the
> > > > > linked list.
> > > > > > > > >
> > > > > > > > > > 9. In GetPciFeaturesConfigurationTable() when checking
> > > > > > > > > > whether a PCI
> > > > > > > > > device
> > > > > > > > > > belongs to a root port, we can use below simpler logic:
> > > > > > > > > >   SizeOfPciDevicePath = GetDevicePathSize (PciDevicePath);
> > > > > > > > > >   SizeOfRootPortDevicePath = GetDevicePathSize
> > > (RootPortPath);
> > > > > > > > > >   if ((SizeOfRootPortDevicePath < SizeOfPciDevicePath) &&
> > > > > > > > > >       CompareMem (PciDevicePath, RootPortPath,
> > > > > > > > > SizeOfRootPortDevicePath -
> > > > > > > > > > END_DEVICE_PATH_LENGTH) == 0)) {
> > > > > > > > > >     // PCI device belongs to the root port.
> > > > > > > > > >   }
> > > > > > > > > >
> > > > > > > > > Ok.
> > > > > > > > >
> > > > > > > > > > > > +        Status = ProgramPciFeatures (RootBridge);
> > > > > > > > > > 10. ProgramPcieFeatures()?
> > > > > > > > > >
> > > > > > > > > OK
> > > > > > > > >
> > > > > > > > > > > > +
> > > > > > > > > > > > +  if (Str != NULL) {
> > > > > > > > > > > > +    FreePool (Str);  }
> > > > > > > > > >
> > > > > > > > > > 11. OK the Str is freed here because Str is needed for
> > > > > > > > > > other debug
> > > > > > > > > messages
> > > > > > > > > > inside the function.
> > > > > > > > > >
> > > > > > > > > Yes
> > > > > > > > >
> > > > > > > > > > > > +  //
> > > > > > > > > > > > +  // mark this root bridge as PCI features
> > > > > > > > > > > > +configuration complete, and no new
> > > > > > > > > > > > +  // enumeration is required
> > > > > > > > > > > > +  //
> > > > > > > > > > > > +  AddRootBridgeInPciFeaturesConfigCompletionList
> > > > > > > > > > > > +(RootBridge, FALSE);
> > > > > > > > > >
> > > > > > > > > > 12. Not needed.
> > > > > > > > > >
> > > > > > > > > ok, after incorporating the logic of gFullEnumeration it
> > > > > > > > > won't be required
> > > > > > > > >
> > > > > > > > > > > > +_PRIMARY_ROOT_PORT_NODE {
> > > > > > > > > >
> > > > > > > > > > > > +  //
> > > > > > > > > > > > +  // Signature header
> > > > > > > > > > > > +  //
> > > > > > > > > > > > +  UINT32                                    Signature;
> > > > > > > > > > > > +  //
> > > > > > > > > > > > +  // linked list pointers to next node
> > > > > > > > > > > > +  //
> > > > > > > > > > > > +  LIST_ENTRY                                NeighborRootPort;
> > > > > > > > > > > > +  //
> > > > > > > > > > > > +  // pointer to PCI_IO_DEVICE of the primary PCI
> > > > > > > > > > > > +Controller device
> > > > > > > > > > > > +  //
> > > > > > > > > > > > +  EFI_DEVICE_PATH_PROTOCOL
> > > > > *RootPortDevicePath;
> > > > > > > > > > > > +  //
> > > > > > > > > > > > +  // pointer to the corresponding PCI feature
> > > > > > > > > > > > +configuration Table node
> > > > > > > > > > > > +  // all the child PCI devices of the controller
> > > > > > > > > > > > +are aligned based on this table
> > > > > > > > > > > > +  //
> > > > > > > > > > > > +  OTHER_PCI_FEATURES_CONFIGURATION_TABLE
> > > > > > > > > > > > *OtherPciFeaturesConfigurationTable;
> > > > > > > > > > > > +};
> > > > > > > > > >
> > > > > > > > > > 13. Can you add the
> > > > > OTHER_PCI_FEATURES_CONFIGURATION_TABLE
> > > > > > > field
> > > > > > > > > to
> > > > > > > > > > PCI_IO_DEVICE structure?
> > > > > > > > > > So this structure PRIMARY_ROOT_PORT_NODE is not
> > needed.
> > > > > > > > > >
> > > > > > > > > I think it is better to maintain separately as this
> > > > > > > > > configuration table is confined to a group of PCI
> > > > > > > > > devices and for the RCiEP it is not applicable hence not
> required.
> > > > > > > > > Moreover, I am maintaining a variable for each PCIe
> > > > > > > > > feature in the PCI_IO_DEVICE; perhaps I can
> > > > > > > consider having just pointer of it....
> > > > > > > > >
> > > > > > > > > > > > +struct
> > > _PCI_FEATURE_CONFIGURATION_COMPLETION_LIST {
> > > > > > > > > > 14. This structure is not needed if using gFullEnumeration.
> > > > > > > > > Yes.
> > > > > > > >
> > > > > > > >