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.
> > +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;
> > +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.
> > +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.
> > +BOOLEAN
> > +IsPciRootPortEmpty (
> > + IN PCI_IO_DEVICE *PciDevice
> > + )
4. Please use IsListEmpty() directly from callers and remove this function.
> > +**/
> > +EFI_STATUS
> > +EnumerateOtherPciFeatures (
5. Can it be "EnumeratePcieFeatures"?
> > + 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().
> > +
> > + 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.
> > + 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()?
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.
}
> > + Status = ProgramPciFeatures (RootBridge);
10. ProgramPcieFeatures()?
> > +
> > + if (Str != NULL) {
> > + FreePool (Str);
> > + }
11. OK the Str is freed here because Str is needed for other debug messages
inside the function.
> > + //
> > + // mark this root bridge as PCI features configuration complete, and
> > +no new
> > + // enumeration is required
> > + //
> > + AddRootBridgeInPciFeaturesConfigCompletionList (RootBridge, FALSE);
12. Not needed.
> > +_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.
> > +struct _PCI_FEATURE_CONFIGURATION_COMPLETION_LIST {
14. This structure is not needed if using gFullEnumeration.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#52285): https://edk2.groups.io/g/devel/message/52285
Mute This Topic: https://groups.io/mt/55158724/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
