> > + UINT8 SetupMPS;
1. Can it be "MaxPayloadSize"?
> > +
> > + if (PciConfigPhase == PciFeatureGetDevicePolicy) {
> > + if (SetupMpsAsPerDeviceCapability (PciDevice->SetupMPS)) {
2. Can you replace " SetupMpsAsPerDeviceCapability (PciDevice->SetupMPS" with
"PciDevice->MaxPayloadSize == EFI_PCI_CONF_MAX_PAYLOAD_SIZE_AUTO"?
This makes the code more readable.
> > + MpsValue = (UINT8)PciDeviceCap.Bits.MaxPayloadSize;
> > + //
> > + // no change to PCI Root ports without any endpoint device
> > + //
> > + if (IS_PCI_BRIDGE (&PciDevice->Pci) &&
> > PciDeviceCap.Bits.MaxPayloadSize)
> > {
> > + if (IsPciRootPortEmpty (PciDevice)) {
> > + MpsValue = PCIE_MAX_PAYLOAD_SIZE_128B;
> > + }
> > + }
3. Above two if-s can be simplified as below? and please also copy the spec
requirements here as comments.
if (IsListEmpty (&PciDevice->ChildList)) {
MpsValue = PCIE_MAX_PAYLOAD_SIZE_128B;
}
> > + } else {
> > + MpsValue = TranslateMpsSetupValueToPci (PciDevice->SetupMPS);
4. The function name can be "UefiToPciMaxPayloadSize()". And I suggest the
value stored in PciDevice->SetupMPS (MaxPayloadSize) is the macro value
defined in PciExpress21.h. We could do the conversion just after the
GetDevicePolicy() call.
> > + }
> > + //
> > + // discard device policy override request if greater than PCI device
> > capability
> > + //
> > + PciDevice->SetupMPS = MIN ((UINT8)PciDeviceCap.Bits.MaxPayloadSize,
> > + MpsValue); }
> > +
> > + //
> > + // align the MPS of the tree to the HCF with this device // if
> > + (PciFeaturesConfigurationTable) {
> > + MpsValue = PciFeaturesConfigurationTable->Max_Payload_Size;
5. Max_Payload_Size can be "MaxPayloadSize".
MpsValue can be "MaxPayloadSize".
> > +
> > + MpsValue = MIN (PciDevice->SetupMPS, MpsValue);
> > + PciDevice->SetupMPS = MIN (PciDevice->SetupMPS, MpsValue);
> > +
> > + if (MpsValue != PciFeaturesConfigurationTable->Max_Payload_Size) {
> > + PciFeaturesConfigurationTable->Max_Payload_Size = MpsValue;
> > + }
> > + }
6. Can you simplify the above logic?
> > +
> > + DEBUG (( DEBUG_INFO,
> > + "MPS: %d [DevCap:%d],",
> > + PciDevice->SetupMPS, PciDeviceCap.Bits.MaxPayloadSize
> > + ));
> > + return EFI_SUCCESS;
> > +}
> > +
> > +/**
> > + Overrides the PCI Device Control register MaxPayloadSize register
> > +field; if
> > + the hardware value is different than the intended value.
> > +
> > + @param PciDevice A pointer to the PCI_IO_DEVICE instance.
> > +
> > + @retval EFI_SUCCESS The data was read from or written to the
> > PCI
> > device.
> > + @retval EFI_UNSUPPORTED The address range specified by Offset,
> > Width,
> > and Count is not
> > + valid for the PCI configuration header of
> > the PCI controller.
> > + @retval EFI_INVALID_PARAMETER Buffer is NULL or Width is invalid.
> > +
> > +**/
> > +EFI_STATUS
> > +OverrideMaxPayloadSize (
> > + IN PCI_IO_DEVICE *PciDevice
> > + )
7. Can this name be "ProgramMaxPayloadSize" because the function does
the register programming?
> > +{
> > + PCI_REG_PCIE_DEVICE_CONTROL PcieDev;
> > + UINT32 Offset;
> > + EFI_STATUS Status;
> > + EFI_TPL OldTpl;
> > +
> > + PcieDev.Uint16 = 0;
> > + Offset = PciDevice->PciExpressCapabilityOffset +
> > + OFFSET_OF (PCI_CAPABILITY_PCIEXP, DeviceControl);
> > + Status = PciDevice->PciIo.Pci.Read (
> > + &PciDevice->PciIo,
> > + EfiPciIoWidthUint16,
> > + Offset,
> > + 1,
> > + &PcieDev.Uint16
> > + );
8. The PciExp is cached in PciExp field in the PciDevice structure. Why do you
need
to read it from HW again?
> > + if (EFI_ERROR(Status)){
> > + DEBUG (( DEBUG_ERROR, "Unexpected DeviceControl register (0x%x) read
> > error!",
> > + Offset
> > + ));
> > + return Status;
> > + }
> > + if (PcieDev.Bits.MaxPayloadSize != PciDevice->SetupMPS) {
> > + PcieDev.Bits.MaxPayloadSize = PciDevice->SetupMPS;
> > + DEBUG (( DEBUG_INFO, "MPS=%d,", PciDevice->SetupMPS));
> > +
> > + //
> > + // Raise TPL to high level to disable timer interrupt while the write
> > operation
> > completes
> > + //
> > + OldTpl = gBS->RaiseTPL (TPL_HIGH_LEVEL);
> > +
> > + Status = PciDevice->PciIo.Pci.Write (
> > + &PciDevice->PciIo,
> > + EfiPciIoWidthUint16,
> > + Offset,
> > + 1,
> > + &PcieDev.Uint16
> > + );
> > + //
> > + // Restore TPL to its original level
> > + //
> > + gBS->RestoreTPL (OldTpl);
> > +
> > + if (!EFI_ERROR(Status)) {
> > + PciDevice->PciExpStruct.DeviceControl.Uint16 = PcieDev.Uint16;
> > + } else {
> > + DEBUG (( DEBUG_ERROR, "Unexpected DeviceControl register (0x%x) write
> > error!",
> > + Offset
> > + ));
9. We can use ASSERT_EFI_ERROR() here. Failure of register writing is a fatal
error.
> > + }
> > + } else {
> > + DEBUG (( DEBUG_INFO, "No write of MPS=%d,", PciDevice->SetupMPS));
10. Can we skip this debug message?
> > + }
> > +
> > + return Status;
> > +}
> >
> > /**
> > helper routine to dump the PCIe Device Port Type @@ -669,6 +809,18 @@
> > SetupDevicePciFeatures (
> > }
> > }
> >
> > + DEBUG ((DEBUG_INFO, "["));
> > + //
> > + // process the PCI device Max_Payload_Size feature // if
> > + (SetupMaxPayloadSize ()) {
> > + Status = ProcessMaxPayloadSize (
> > + PciDevice,
> > + PciConfigPhase,
> > + OtherPciFeaturesConfigTable
> > + );
11. Can this function be "CalculatemaxPayloadSize"? Process is too general.
> > + }
> > + DEBUG ((DEBUG_INFO, "]\n"));
> > return Status;
> > }
> >
> > @@ -765,6 +917,10 @@ ProgramDevicePciFeatures ( {
> > EFI_STATUS Status = EFI_SUCCESS;
> >
> > + if (SetupMaxPayloadSize ()) {
> > + Status = OverrideMaxPayloadSize (PciDevice); } DEBUG ((
> > + DEBUG_INFO, "\n"));
> > return Status;
> > }
> >
> > @@ -878,6 +1034,7 @@ AddPrimaryRootPortNode (
> > );
> > if (PciConfigTable) {
> > PciConfigTable->ID = PortNumber;
> > + PciConfigTable->Max_Payload_Size =
> > PCIE_MAX_PAYLOAD_SIZE_4096B;
> > }
> > RootPortNode->OtherPciFeaturesConfigurationTable = PciConfigTable;
> >
> > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciFeatureSupport.h
> > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciFeatureSupport.h
> > index f92d008..e5ac2a3 100644
> > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciFeatureSupport.h
> > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciFeatureSupport.h
> > @@ -79,6 +79,11 @@ struct _OTHER_PCI_FEATURES_CONFIGURATION_TABLE
> > {
> > // Configuration Table ID
> > //
> > UINTN ID;
> > + //
> > + // to configure the PCI feature Maximum payload size to maintain the
> > + data packet // size among all the PCI devices in the PCI hierarchy
> > + //
> > + UINT8 Max_Payload_Size;
> > };
> >
> >
> > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciPlatformSupport.c
> > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciPlatformSupport.c
> > index 238959e..99badd6 100644
> > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciPlatformSupport.c
> > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciPlatformSupport.c
> > @@ -356,6 +356,63 @@ GetPlatformPciOptionRom (
> > return Status;
> > }
> >
> > +/**
> > + Helper routine to indicate whether the given PCI device specific
> > +policy value
> > + dictates to override the Max_Payload_Size to a particular value, or
> > +set as per
> > + device capability.
> > +
> > + @param MPS Input device-specific policy should be in terms of type
> > + EFI_PCI_CONF_MAX_PAYLOAD_SIZE
> > +
> > + @retval TRUE Setup Max_Payload_Size as per device capability
> > + FALSE override as per device-specific platform policy
> > +**/
> > +BOOLEAN
> > +SetupMpsAsPerDeviceCapability (
> > + IN UINT8 MPS
> > +)
> > +{
> > + if (MPS == EFI_PCI_CONF_MAX_PAYLOAD_SIZE_AUTO) {
> > + return TRUE;
> > + } else {
> > + return FALSE;
> > + }
> > +}
> > +
> > +/**
> > + Routine to translate the given device-specific platform policy from
> > +type
> > + EFI_PCI_CONF_MAX_PAYLOAD_SIZE to HW-specific value, as per PCI Base
> > +Specification
> > + Revision 4.0; for the PCI feature Max_Payload_Size.
> > +
> > + @param MPS Input device-specific policy should be in terms of type
> > + EFI_PCI_CONF_MAX_PAYLOAD_SIZE
> > +
> > + @retval Range values for the Max_Payload_Size as defined in the
> > PCI
> > + Base Specification 4.0 **/
> > +UINT8
> > +TranslateMpsSetupValueToPci (
> > + IN UINT8 MPS
> > +)
> > +{
> > + switch (MPS) {
> > + case EFI_PCI_CONF_MAX_PAYLOAD_SIZE_128B:
> > + return PCIE_MAX_PAYLOAD_SIZE_128B;
> > + case EFI_PCI_CONF_MAX_PAYLOAD_SIZE_256B:
> > + return PCIE_MAX_PAYLOAD_SIZE_256B;
> > + case EFI_PCI_CONF_MAX_PAYLOAD_SIZE_512B:
> > + return PCIE_MAX_PAYLOAD_SIZE_512B;
> > + case EFI_PCI_CONF_MAX_PAYLOAD_SIZE_1024B:
> > + return PCIE_MAX_PAYLOAD_SIZE_1024B;
> > + case EFI_PCI_CONF_MAX_PAYLOAD_SIZE_2048B:
> > + return PCIE_MAX_PAYLOAD_SIZE_2048B;
> > + case EFI_PCI_CONF_MAX_PAYLOAD_SIZE_4096B:
> > + return PCIE_MAX_PAYLOAD_SIZE_4096B;
> > + default:
> > + return PCIE_MAX_PAYLOAD_SIZE_128B;
> > + }
> > +}
> > +
> > /**
> > Generic routine to setup the PCI features as per its predetermined
> > defaults.
> > **/
> > @@ -364,6 +421,7 @@ SetupDefaultsDevicePlatformPolicy (
> > IN PCI_IO_DEVICE *PciDevice
> > )
> > {
> > + PciDevice->SetupMPS = EFI_PCI_CONF_MAX_PAYLOAD_SIZE_AUTO;
> > }
> >
> > /**
> > @@ -399,6 +457,7 @@ GetPciDevicePlatformPolicyEx (
> > //
> > // platform chipset policies are returned for this PCI device
> > //
> > + PciIoDevice->SetupMPS = PciPlatformExtendedPolicy.DeviceCtlMPS;
> >
> > DEBUG ((
> > DEBUG_INFO, "[device policy: platform]"
> > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciPlatformSupport.h
> > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciPlatformSupport.h
> > index a13131c..786c00d 100644
> > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciPlatformSupport.h
> > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciPlatformSupport.h
> > @@ -124,4 +124,36 @@ EFI_STATUS
> > GetPciDevicePlatformPolicy (
> > IN PCI_IO_DEVICE *PciDevice
> > );
> > +
> > +/**
> > + Helper routine to indicate whether the given PCI device specific
> > +policy value
> > + dictates to override the Max_Payload_Size to a particular value, or
> > +set as per
> > + device capability.
> > +
> > + @param MPS Input device-specific policy should be in terms of type
> > + EFI_PCI_CONF_MAX_PAYLOAD_SIZE
> > +
> > + @retval TRUE Setup Max_Payload_Size as per device capability
> > + FALSE override as per device-specific platform policy
> > +**/
> > +BOOLEAN
> > +SetupMpsAsPerDeviceCapability (
> > + IN UINT8 MPS
> > +);
> > +
> > +/**
> > + Routine to translate the given device-specific platform policy from
> > +type
> > + EFI_PCI_CONF_MAX_PAYLOAD_SIZE to HW-specific value, as per PCI Base
> > +Specification
> > + Revision 4.0; for the PCI feature Max_Payload_Size.
> > +
> > + @param MPS Input device-specific policy should be in terms of type
> > + EFI_PCI_CONF_MAX_PAYLOAD_SIZE
> > +
> > + @retval Range values for the Max_Payload_Size as defined in the
> > PCI
> > + Base Specification 4.0 **/
> > +UINT8
> > +TranslateMpsSetupValueToPci (
> > + IN UINT8 MPS
> > +);
> > #endif
> > --
> > 2.21.0.windows.1
> >
> >
> >
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#52337): https://edk2.groups.io/g/devel/message/52337
Mute This Topic: https://groups.io/mt/55162470/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
