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