Ashraf,
At the end of PciFeatureGetDevicePolicy, value from the PCI features
configuration table is the minimum one.
I prefer to defer the finalize of PciDevice->SetupMPS in the
OverrideMaxPayloadSize().
Through this, the 4 phases can be reduced to 3 phases.
Thanks,
Ray
> -----Original Message-----
> From: Javeed, Ashraf <ashraf.jav...@intel.com>
> Sent: Wednesday, December 18, 2019 10:36 PM
> To: Ni, Ray <ray...@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
>
> 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 (#52394): https://edk2.groups.io/g/devel/message/52394
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]
-=-=-=-=-=-=-=-=-=-=-=-
