Ray,
As discussed, the ProcessMaxPayloadSize() gets the minimum payload size for all
devices under a certain root port, at the end of PciFeatureSetupPhase.
The value from the PCI features configuration table is aligned to each of
PciDevice->SetupMPS, under a root port.
The PciDevice->SetupMPS is used for its corresponding device's
MaxReadRequestSize feature conditionally, since there is no PCIe capability for
this feature.
Thus, as per the implementation design formulated here, all the
PciDevice->SetupXXX has to be finalized, considering the device policy, during
the phases - PciFeatureGetDevicePolicy & PciFeatureSetupPhase. Finally, it
shall be programmed during the phase PciFeatureConfigurationPhase.
The OverrideMaxPayloadSize() does not have to use the PCI features
configuration table, as that table is temporarily meant for aligning a feature
value among all devices of a root port.
Responding to your other comments below inline.
Thanks
Ashraf
> -----Original Message-----
> From: Ni, Ray <ray...@intel.com>
> Sent: Wednesday, December 18, 2019 2:40 PM
> To: Javeed, Ashraf <ashraf.jav...@intel.com>; 'devel@edk2.groups.io'
> <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
>
> 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"?
I thought prefixing the name with "Setup" can imply that this data member of
PCI_IO_DEVICE would help to indicate it takes the device policy, transforms
into the final raw value after calculation, which shall be programmed in to
hardware control register. I have used same style for all other features, and I
have to rename all those....I don't see any conflict in the name SetupMPS as we
know that MPS is shorthand for the 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.
> >
I thought the routine name itself informally indicate what type of action it
has to take based on *_AUTO EFI encoding....I shall reconsider this.
> > > > + 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; }
> >
OK
> >
> >
> > > > + } 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.
> >
Ok, I can rename to the suggested name for the translation routine. But, I
cannot use that immediately after the GetDevicePolicy() because some of the
PCIe features are defined in the capability register and its *_AUTO can be used
in co-relation to that; like in case of MaxPayloadSize feature; but in case of
PCIe feature like RelaxOrdering & NoSnoop, the PCI Base Specification has not
defined any corresponding capability or hardware reference value for look-up,
thus *_AUTO for these kind of features would be to just ignore it.
> > > > + }
> > > > + //
> > > > + // 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".
> >
OK
> > > > +
> > > > + 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?
> >
Will check on this
> > > > +
> > > > + 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?
> >
OK
> > > > +{
> > > > + 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?
> >
Just for the sake of defensive programming; in case platform has done some
overrides in between; since numerous callbacks are going to platform code of
PCI Host Bridge Resource Allocation Protocol driver, and PCI Platform Protocol
driver.
> > > > + 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.
> >
OK
> > > > + }
> > > > + } else {
> > > > + DEBUG (( DEBUG_INFO, "No write of MPS=%d,",
> > > > + PciDevice->SetupMPS));
> >
> > 10. Can we skip this debug message?
> >
OK
> > > > + }
> > > > +
> > > > + 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.
> >
OK
> > > > + }
> > > > + 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 (#52353): https://edk2.groups.io/g/devel/message/52353
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]
-=-=-=-=-=-=-=-=-=-=-=-
