Ashraf,
Can ProcessMaxPayloadSize() get the minimum payload size for all devices under
a certain root port?
I can understand that the payload size stored in the PCI features configuration
table is the minimum value.
But the value stored in each PciDevice->SetupMPS is not the minimum value.
So OverrideMaxPayloadSize() should use the value stored in the PCI features
configuration table instead of the value stored in PciDevice->SetupMPS.
Thanks,
Ray
> -----Original Message-----
> From: Ni, Ray
> Sent: Wednesday, December 18, 2019 4:38 PM
> To: Javeed, Ashraf <ashraf.jav...@intel.com>; devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.w...@intel.com>; Wu, Hao A <hao.a...@intel.com>
> Subject: RE: [edk2-devel] [edk2-staging/UEFI_PCI_ENHANCE-2 PATCH 08/12]
> PciBusDxe: New PCI feature Max_Payload_Size
>
> > > + 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 (#52340): https://edk2.groups.io/g/devel/message/52340
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]
-=-=-=-=-=-=-=-=-=-=-=-
