From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.88, mailfrom: tien.hock.loh@intel.com) Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by groups.io with SMTP; Mon, 12 Aug 2019 18:02:43 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 12 Aug 2019 18:02:37 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.64,379,1559545200"; d="scan'208";a="259967943" Received: from kmsmsx153.gar.corp.intel.com ([172.21.73.88]) by orsmga001.jf.intel.com with ESMTP; 12 Aug 2019 18:02:36 -0700 Received: from pgsmsx110.gar.corp.intel.com ([169.254.13.32]) by KMSMSX153.gar.corp.intel.com ([169.254.5.69]) with mapi id 14.03.0439.000; Tue, 13 Aug 2019 09:02:31 +0800 From: "Loh, Tien Hock" To: Leif Lindholm CC: "Kinney, Michael D" , "devel@edk2.groups.io" , "thloh85@gmail.com" , "Ard Biesheuvel" Subject: Re: [PATCH v6 1/1] Platform: Intel: Add Stratix 10 platform support Thread-Topic: [PATCH v6 1/1] Platform: Intel: Add Stratix 10 platform support Thread-Index: AQHVSFRvTVtyx5hSPEW9I8UsgrrDLKbr7I7Q///cVgCABWVMgIAA8DSggAABnYCABjS7sA== Date: Tue, 13 Aug 2019 01:02:30 +0000 Message-ID: References: <20190801103213.118840-1-tien.hock.loh@intel.com> <20190805092616.GB25813@bivouac.eciton.net> <20190809101537.GK25813@bivouac.eciton.net> In-Reply-To: <20190809101537.GK25813@bivouac.eciton.net> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.2.0.6 dlp-reaction: no-action x-ctpclassification: CTP_NT x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiODNlMDVjYWItZmJjZS00YmU1LWIxNDMtZjdmZWM5MzM5NzljIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiTEVBM1J6aHRrdzhlUlhYbFMydndOWGxVUE54ZEV6QURyV21MeW9qdXZGM3kwb1M3WCtiaDE4MFZcL3oxRW9aMDUifQ== x-originating-ip: [172.30.20.206] MIME-Version: 1.0 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hi Leif, I'll update it and send another patch on top of the previously reviewed pat= ch. Thanks!=20 > -----Original Message----- > From: Leif Lindholm > Sent: Friday, August 9, 2019 6:16 PM > To: Loh, Tien Hock > Cc: Kinney, Michael D ; devel@edk2.groups.io; > thloh85@gmail.com; Ard Biesheuvel > Subject: Re: [PATCH v6 1/1] Platform: Intel: Add Stratix 10 platform supp= ort >=20 > Hi Tien Hock, >=20 > Given Mike's review, could you roll a v7 with all of the incorporated fee= dback > from Mike based on this (instead of submitting the v6 updates as a separa= te > patch, which I previously requested)? >=20 > Could you also roll in the following in that patch?: > diff --git a/Maintainers.txt b/Maintainers.txt index > 876ae5612ad8..47d58ffa0b2c 100644 > --- a/Maintainers.txt > +++ b/Maintainers.txt > @@ -98,6 +98,11 @@ M: Shifei A Lu > M: Xiaohu Zhou > M: Isaac W Oram >=20 > +Platform/Intel/Stratix10SocPkg > +M: Leif Lindholm > +M: Michael D Kinney > +R: Tien Hock Loh > + > Platform/Intel/Tools > M: Bob Feng > M: Liming Gao >=20 > Best Regards, >=20 > Leif >=20 > On Fri, Aug 09, 2019 at 02:17:42AM +0000, Loh, Tien Hock wrote: > > > -----Original Message----- > > > From: Kinney, Michael D > > > Sent: Friday, August 9, 2019 3:50 AM > > > To: Leif Lindholm ; Loh, Tien Hock > > > ; Kinney, Michael D > > > > > > Cc: devel@edk2.groups.io; thloh85@gmail.com; Ard Biesheuvel > > > > > > Subject: RE: [PATCH v6 1/1] Platform: Intel: Add Stratix 10 platform > > > support > > > > > > Tien Hock, > > > > > > I have a few comments: > > > > > > 1) Recommend change name of directory > > > > > > Platform/Intel/Startix10 -> Platform/Intel/Startix10SocPkg. > > OK will do that. > > > > > > > > 2) S10ClockManager.c is missing file header with license and > > > copyright > > > 3) S10ClockManager.h is missing file header with license and > > > copyright > > Yeah, I'll submit a fix to that > > > > > 4) PlatformHookLib.inf uses '..' to access sources in a different dir= ectory. > > > '..' should never be used in an INF. This INF also lists many > > > PCDs that are not used by PlatformHookLib.c > > OK I'll remove the dependencies. > > > > > 5) PlatformHookLib.c also uses '..' in an include that should not > > > be used. > > > 6) Can the following files be updated to a BSD+Patent license and > > > use an SPDX identifier? > > > > > > IntelPlatformDxe.inf > > > IntelPlatformDxe.c > > > > > > IntelPlatformLib.inf > > > Stratix10PlatformLib.c > > > Startix10Mmu.c > > > ArmPlatformHelper.S > > > > > OK. Noted, I missed changing these license headers. > > > > > If S10ClockManager is only used by the PlatformHookLib, then I > > > recommend you move the S10ClockManager sources into the > > > PlatformHookLib directory or a subdirectory below PlatformHookLib. > > > > > The S10ClockManager is also being used by > Drivers/IntelPlatformDxe/IntelPlatformDxe.c, so I'm wondering what's the > best approach to this? > > > > > Thanks, > > > > > > Mike