Re: [PATCH v6 1/1] Platform: Intel: Add Stratix 10 platform support

["Leif Lindholm" <leif.lindholm@linaro.org>]

Hi Tien Hock,

Given Mike's review, could you roll a v7 with all of the incorporated
feedback from Mike based on this (instead of submitting the v6 updates
as a separate patch, which I previously requested)?

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 <shifei.a.lu@intel.com>
 M: Xiaohu Zhou <bowen.zhou@intel.com>
 M: Isaac W Oram <isaac.w.oram@intel.com>
 
+Platform/Intel/Stratix10SocPkg
+M: Leif Lindholm <leif.lindholm@linaro.org>
+M: Michael D Kinney <michael.d.kinney@intel.com>
+R: Tien Hock Loh <tien.hock.loh@intel.com>
+
 Platform/Intel/Tools
 M: Bob Feng <bob.c.feng@intel.com>
 M: Liming Gao <liming.gao@intel.com>

Best Regards,

Leif

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 <leif.lindholm@linaro.org>; Loh, Tien Hock
> > <tien.hock.loh@intel.com>; Kinney, Michael D
> > <michael.d.kinney@intel.com>
> > Cc: devel@edk2.groups.io; thloh85@gmail.com; Ard Biesheuvel
> > <ard.biesheuvel@linaro.org>
> > 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 directory.
> >    '..' 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