Re: [edk2-devel] [edk2-staging/UEFI_PCI_ENHANCE-2 PATCH 08/12] PciBusDxe: New PCI feature Max_Payload_Size

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

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