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

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

Ashraf,
Thanks for the reply. Inline comments below.

> -----Original Message-----
> From: Javeed, Ashraf <ashraf.javeed@intel.com>
> Sent: Tuesday, April 21, 2020 2:22 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
> 
> 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

1. Platform can set MaxPayloadSize policy value to 128B if platform wants
to force to use 128B.
Why do you think skipping this feature is invalid?



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

2. I take your answer as an agreement😊

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

3. Agreement.

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

4. Agreement.

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

5. Based on your latest reply, I consider you agree to skip this feature for AUTO.


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

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

6.2. That's what I am not sure about. In real scenario, LTR is requested by
end point device driver. When PciBus receives the request, it checks all devices
between the root complex and the end point and enables the LTR in the whole path if
all devices in the path are capable of LTR.

The PCIE_PLATFORM protocol is useful to provide policy for fixed components
of the chip.



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

6.3. Let me confirm my understanding. You are saying:
if platform policy wants any individual device between root complex and the
end point to enable LTR, PciBus driver enables LTR for all downside child devices and
all upside devices in the path to root complex.
In the following device tree, if platform policy for switch#2 is TRUE, PciBus enables LTR
for
a. switch#2
b. EP#2 and EP#3
c. switch#1.downstream that connects to switch#2.upstream, 
d. switch#1.upstream

RootComplex
          |
    switch#1
      /       \
EP#1    switch#2
              /         \
         EP#2     EP#3


I don't agree with this interpretation of platform policy. The policy is for individual
pcie device. If platform wants PciBus to enable LTR for certain devices, it should
set the policy to TRUE for those devices. In above device tree, platform needs to
return TRUE for devices #a, #b, #c, #d.
 
> > >
> > > (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?

7. Based on your latest reply, I think my proposed implementation is ok to you.

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