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 <[email protected]> > Sent: Monday, April 20, 2020 6:53 PM > To: Ni, Ray <[email protected]>; [email protected] > Cc: Wang, Jian J <[email protected]>; Wu, Hao A > <[email protected]>; Javeed, Ashraf <[email protected]> > 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 <[email protected]> > > Sent: Tuesday, March 17, 2020 9:07 PM > > To: Javeed, Ashraf <[email protected]>; [email protected] > > Cc: Wang, Jian J <[email protected]>; Wu, Hao A > > <[email protected]> > > 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 <[email protected]> > > > > > > Sent: Thursday, March 5, 2020 7:43 PM > > > > > > To: [email protected]; Ni, Ray <[email protected]>; Javeed, > > > > > > Ashraf <[email protected]> > > > > > > Cc: Wang, Jian J <[email protected]>; Wu, Hao A > > > > > > <[email protected]> > > > > > > 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: [email protected] <[email protected]> On Behalf > > Of > > > > Ni, > > > > > > Ray > > > > > > > Sent: Thursday, December 19, 2019 1:49 PM > > > > > > > To: Javeed, Ashraf <[email protected]>; > > > > > > > [email protected] > > > > > > > Cc: Wang, Jian J <[email protected]>; Wu, Hao A > > > > > > > <[email protected]> > > > > > > > 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 <[email protected]> > > > > > > > > Sent: Wednesday, December 18, 2019 3:14 PM > > > > > > > > To: Ni, Ray <[email protected]>; [email protected] > > > > > > > > Cc: Wang, Jian J <[email protected]>; Wu, Hao A > > > > > > > > <[email protected]> > > > > > > > > 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 <[email protected]> > > > > > > > > > Sent: Tuesday, December 17, 2019 5:26 PM > > > > > > > > > To: Javeed, Ashraf <[email protected]>; > > > > > > > > > [email protected] > > > > > > > > > Cc: Wang, Jian J <[email protected]>; Wu, Hao A > > > > > > > > > <[email protected]> > > > > > > > > > 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. > > > > > > > > > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#57705): https://edk2.groups.io/g/devel/message/57705 Mute This Topic: https://groups.io/mt/55158724/21656 Group Owner: [email protected] Unsubscribe: https://edk2.groups.io/g/devel/unsub [[email protected]] -=-=-=-=-=-=-=-=-=-=-=-
