From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by mx.groups.io with SMTP id smtpd.web09.19220.1581324387065813599 for ; Mon, 10 Feb 2020 00:46:27 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.120, mailfrom: ray.ni@intel.com) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 10 Feb 2020 00:46:26 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.70,424,1574150400"; d="scan'208";a="312681288" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by orsmga001.jf.intel.com with ESMTP; 10 Feb 2020 00:46:26 -0800 Received: from fmsmsx156.amr.corp.intel.com (10.18.116.74) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.439.0; Mon, 10 Feb 2020 00:46:25 -0800 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by fmsmsx156.amr.corp.intel.com (10.18.116.74) with Microsoft SMTP Server (TLS) id 14.3.439.0; Mon, 10 Feb 2020 00:46:25 -0800 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.5]) by SHSMSX103.ccr.corp.intel.com ([169.254.4.196]) with mapi id 14.03.0439.000; Mon, 10 Feb 2020 16:46:23 +0800 From: "Ni, Ray" To: "Javeed, Ashraf" , "devel@edk2.groups.io" CC: "Wang, Jian J" , "Wu, Hao A" Subject: Re: [edk2-devel] [edk2-staging/UEFI_PCI_ENHANCE-2 PATCH 02/12] MdeModulePkg/PciBusDxe: Setup PCI Express init phase Thread-Topic: [edk2-devel] [edk2-staging/UEFI_PCI_ENHANCE-2 PATCH 02/12] MdeModulePkg/PciBusDxe: Setup PCI Express init phase Thread-Index: AQHV3fO7JJaepXTAh06CmUc/tEZvt6gUCM1A//+OhYCAAIfUgA== Date: Mon, 10 Feb 2020 08:46:22 +0000 Message-ID: <734D49CCEBEEF84792F5B80ED585239D5C429588@SHSMSX104.ccr.corp.intel.com> References: <20200207200447.10536-1-ashraf.javeed@intel.com> <15F137802DC889C4.7869@groups.io> <95C5C2B113DE604FB208120C742E9824579AAF7C@BGSMSX101.gar.corp.intel.com> <734D49CCEBEEF84792F5B80ED585239D5C42933D@SHSMSX104.ccr.corp.intel.com> <95C5C2B113DE604FB208120C742E9824579AB46F@BGSMSX101.gar.corp.intel.com> In-Reply-To: <95C5C2B113DE604FB208120C742E9824579AB46F@BGSMSX101.gar.corp.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 My proposal defines four phases: Initialize, Scan, Program, Finalize. The f= our phases are very similar to your December's patch with just one phase opt out. As below: ---BEGIN---- Each feature needs to provide function pointers for each phase and NULL mea= ns the feature doesn't need to do anything in the specific phase. With that, we can define a structure: Typedef struct { BOOLEAN Enable; PCIE_FEATURE_INITILAIZE Initialize; PCIE_FEATURE_SCAN Scan; PCIE_FEATURE_PROGRAM Program; PCIE_FEATURE_FINALIZE Finalize; } PCIE_FEATURE_ENTRY; With that, we can define a module level global variable: PCIE_FEATURE_ENTRY mPcieFeatures[] =3D { { TRUE, MaxPayloadInitialize, MaxPayloadScan, MaxPayloadProgram, MaxPaylo= adFinalize}, { TRUE, MaxReadRequestInitialize, MaxReadRequestScan, MaxReadRequestProgr= am, MaxReadRequestFinalize}, { TRUE, NULL, NULL, RelaxOrderProgram, NULL}, { TRUE, NULL, CompletionTimeoutScan, CompletionTimeoutProgram, NULL }, ... }; PCIE_FEATURE_ENTRY.Enable can be set to FALSE according to the platform pol= icy. The enable of PCIE features can be written as a feature agnostic for-loop. This can make the new feature enabling code easy to add and review. ---END---- While your today's patch defines a different 5-phase as below. typedef enum { PciExpressFeaturePreProcessPhase, PciExpressFeatureSetupPhase, PciExpressFeatureEntendedSetupPhase, PciExpressFeatureProgramPhase, PciExpressFeatureEndPhase } PCI_EXPRESS_FEATURE_CONFIGURATION_PHASE; I don't think they are very similar. And your new proposal is far different= from what you proposed in December's patch. Did you meet any issues that triggered this new proposal? Thanks, Ray > -----Original Message----- > From: Javeed, Ashraf > Sent: Monday, February 10, 2020 4:33 PM > To: Ni, Ray ; devel@edk2.groups.io > Cc: Wang, Jian J ; Wu, Hao A > Subject: RE: [edk2-devel] [edk2-staging/UEFI_PCI_ENHANCE-2 PATCH 02/12] M= deModulePkg/PciBusDxe: Setup PCI Express > init phase >=20 > Thanks > Ashraf >=20 > > -----Original Message----- > > From: Ni, Ray > > Sent: Monday, February 10, 2020 1:07 PM > > To: Javeed, Ashraf ; devel@edk2.groups.io > > Cc: Wang, Jian J ; Wu, Hao A > > Subject: RE: [edk2-devel] [edk2-staging/UEFI_PCI_ENHANCE-2 PATCH 02/12] > > MdeModulePkg/PciBusDxe: Setup PCI Express init phase > > > > > > + Status =3D EnumeratePciExpressFeatures ( > > 1. "enumerate" means "visit". But I think this function is not just vis= iting the > > features but also > > programming them. So, How about "ProgramPciExpressFeatures"? > > (I gave a similar review comment in last time review in Dec.) > > > Actually I have already used the "ProgramPciExpressFeatures" in one of th= e sub-phases of the PCI Express initialization > code...do you have any other name for this? How about "ConfigurePciExpres= sFeatures"? >=20 > > 2. In mail https://edk2.groups.io/g/devel/message/52399 I proposed to s= implify > > to 4 phases. > > Did you find any issue with my proposal? > I did simplify to 4 phases, please check. The fifth phase is to report ou= t the device state to the platform through the > protocol interface "NotifyDeviceState". >=20 > > > > Thanks, > > Ray