From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by mx.groups.io with SMTP id smtpd.web12.1803.1589357395236434385 for ; Wed, 13 May 2020 01:09:55 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 134.134.136.65, mailfrom: ray.ni@intel.com) IronPort-SDR: BNmO42hlKdlrItWDKcFd/uX4cAHF6nVWPDo4w9NRueU4viuGHLT+ZkS+C6o2wHjb8luTadEd1T ejyuLBe5DLpQ== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by orsmga103.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 13 May 2020 01:09:54 -0700 IronPort-SDR: kohp+/8WSot61DUbN17o4nKrk3z06GHjzr04Pm5X0XI7IpKay6ETHK6IV+RWF0YmkLtuOD12Bk 90JARRj+dn6w== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.73,387,1583222400"; d="scan'208";a="463846364" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by fmsmga005.fm.intel.com with ESMTP; 13 May 2020 01:09:54 -0700 Received: from fmsmsx122.amr.corp.intel.com (10.18.125.37) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.439.0; Wed, 13 May 2020 01:09:54 -0700 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by fmsmsx122.amr.corp.intel.com (10.18.125.37) with Microsoft SMTP Server (TLS) id 14.3.439.0; Wed, 13 May 2020 01:09:54 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.210]) by SHSMSX152.ccr.corp.intel.com ([169.254.6.77]) with mapi id 14.03.0439.000; Wed, 13 May 2020 16:09:50 +0800 From: "Ni, Ray" To: "devel@edk2.groups.io" , "Javeed, Ashraf" CC: "Wang, Jian J" , "Wu, Hao A" Subject: Re: [edk2-devel] [edk2-staging/UEFI_PCI_ENHANCE-2 PATCH 14/15] MdeModulePkg/PciBusDxe: Enable ExtendedTag feature Thread-Topic: [edk2-devel] [edk2-staging/UEFI_PCI_ENHANCE-2 PATCH 14/15] MdeModulePkg/PciBusDxe: Enable ExtendedTag feature Thread-Index: AQHWJuYUJe62JSGvi0ytnelqFKTApKig+CyAgAStENA= Date: Wed, 13 May 2020 08:09:50 +0000 Message-ID: <734D49CCEBEEF84792F5B80ED585239D5C53B0B6@SHSMSX104.ccr.corp.intel.com> References: <20200510161412.13832-1-ashraf.javeed@intel.com> <20200510161412.13832-15-ashraf.javeed@intel.com> In-Reply-To: <20200510161412.13832-15-ashraf.javeed@intel.com> Accept-Language: en-US, zh-CN X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.2.0.6 dlp-reaction: no-action x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: ray.ni@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable > + > + // > + // BIT0 of the policy value is for 5b or 8b Extended Tag (DeviceContro= l BIT8) > + // BIT1 of the policy value is for 10b Extended Tag (DeviceControl2 BI= T12) > + // > + if ((PciIoDevice->DeviceState.ExtendedTag >> 2) !=3D 0) { > + return EFI_INVALID_PARAMETER; > + } Because there is a separate routine to formalize all policy values, this pa= rameter check may be unnecessary. Similar comments apply to all the scan/program routines for different featu= res. What do you think? > + // > + // the device should be capable of 10b Extended Tag Requester > + // > + if ((PciIoDevice->DeviceState.ExtendedTag & BIT1) && > + (PciIoDevice->PciExpressCapability.DeviceCapability2.Bits.TenBitTa= gRequesterSupported)) { > + // > + // for the Endpoint device 10b Extended Tag Requester Enable, the RC= should be > + // 10b Completer capable > + // > + if (PciIoDevice->PciExpressCapability.Capability.Bits.DevicePortType= =3D=3D PCIE_DEVICE_PORT_TYPE_PCIE_ENDPOINT || > + PciIoDevice->PciExpressCapability.Capability.Bits.DevicePortType= =3D=3D > PCIE_DEVICE_PORT_TYPE_LEGACY_PCIE_ENDPOINT) { > + // > + // check the parent Root Port 10b Extended Tag Completer Capabilit= y > + // > + TenBitCompleterCapable =3D *Context; > + if (*TenBitCompleterCapable =3D=3D TRUE) { > + // > + // since the RC is 10b COmpleter capable, enable the EP as 10b R= equester > + // > + DeviceCtl2.Bits.TenBitTagRequesterEnable =3D 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 pla= tform ask > + // > + DeviceCtl2.Bits.TenBitTagRequesterEnable =3D 1; My understanding is for switches, the enable is not necessary. Because spec says switches forward the request without modifying the Transa= ction ID. > + > + // > + // if no 10b Extended Tag Requester for this device than consider 8b o= r 5b Extended Requester > + // > + if (!DeviceCtl2.Bits.TenBitTagRequesterEnable) { > + // > + // the device should be capable of 8b Extended Tag Requester > + // > + DeviceCtl.Bits.ExtendedTagField =3D (UINT16) > + ((PciIoDevice->DeviceState.ExtendedTag & BIT0) && > + (PciIoDevice->PciExpressCapability.DeviceCapability.Bits.= ExtendedTagField)); Why you don't check the RP's capability for 8bit case?