Re: [edk2-devel] [edk2-staging/UEFI_PCI_ENHANCE-2 PATCH 14/15] MdeModulePkg/PciBusDxe: Enable ExtendedTag feature

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

> +
> +  //
> +  // BIT0 of the policy value is for 5b or 8b Extended Tag (DeviceControl BIT8)
> +  // BIT1 of the policy value is for 10b Extended Tag (DeviceControl2 BIT12)
> +  //
> +  if ((PciIoDevice->DeviceState.ExtendedTag >> 2) != 0) {
> +    return EFI_INVALID_PARAMETER;
> +  }

Because there is a separate routine to formalize all policy values, this parameter check may be unnecessary.
Similar comments apply to all the scan/program routines for different features.
What do you think?


> +  //
> +  // the device should be capable of 10b Extended Tag Requester
> +  //
> +  if ((PciIoDevice->DeviceState.ExtendedTag & BIT1) &&
> +      (PciIoDevice->PciExpressCapability.DeviceCapability2.Bits.TenBitTagRequesterSupported)) {
> +    //
> +    // for the Endpoint device 10b Extended Tag Requester Enable, the RC should be
> +    // 10b Completer capable
> +    //
> +    if (PciIoDevice->PciExpressCapability.Capability.Bits.DevicePortType == PCIE_DEVICE_PORT_TYPE_PCIE_ENDPOINT ||
> +        PciIoDevice->PciExpressCapability.Capability.Bits.DevicePortType ==
> PCIE_DEVICE_PORT_TYPE_LEGACY_PCIE_ENDPOINT) {
> +      //
> +      // check the parent Root Port 10b Extended Tag Completer Capability
> +      //
> +      TenBitCompleterCapable = *Context;
> +      if (*TenBitCompleterCapable == TRUE) {
> +        //
> +        // since the RC is 10b COmpleter capable, enable the EP as 10b Requester
> +        //
> +        DeviceCtl2.Bits.TenBitTagRequesterEnable = 1;


I read through PCIE spec 5.0 chapter 2.2.6.2 but cannot find any statement that says:
Enable EP only when RP supports.

My understanding is:
Each EP can have its own 8-bit/10-bit enable setting.
Can you please help to explain?

> +      }
> +    } else {
> +      //
> +      // enable the device as 10b Requester if it is capable and per platform ask
> +      //
> +      DeviceCtl2.Bits.TenBitTagRequesterEnable = 1;

My understanding is for switches, the enable is not necessary.
Because spec says switches forward the request without modifying the Transaction ID.

> +
> +  //
> +  // if no 10b Extended Tag Requester for this device than consider 8b or 5b Extended Requester
> +  //
> +  if (!DeviceCtl2.Bits.TenBitTagRequesterEnable) {
> +    //
> +    // the device should be capable of 8b Extended Tag Requester
> +    //
> +    DeviceCtl.Bits.ExtendedTagField = (UINT16)
> +              ((PciIoDevice->DeviceState.ExtendedTag & BIT0) &&
> +               (PciIoDevice->PciExpressCapability.DeviceCapability.Bits.ExtendedTagField));

Why you don't check the RP's capability for 8bit case?