Re: [edk2-platforms][PATCH V1 1/2] KabylakeSiliconPkg: Remove ResetSystemLib.h override

["Chaganty, Rangasai V" <rangasai.v.chaganty@intel.com>]

Hi Michael, 
I agree on eliminating the redundant copies Edk2 APIs from Edk2Platform packages and I also think it can be done without introducing newer dependencies as indicated in my previous response.
That said, the topic of package dependencies, especially for silicon initialization code needs a broader discussion and is outside the scope of this review.
Let's take care of this change for now and address the VS2017 build issue and let's be prepared for further changes as we make progress on the package dependency discussions.


Reviewed-by: Sai Chaganty <rangasai.v.chaganty@intel.com>

Thanks,
Sai


-----Original Message-----
From: Kubacki, Michael A <michael.a.kubacki@intel.com> 
Sent: Wednesday, November 27, 2019 4:31 PM
To: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>; devel@edk2.groups.io
Cc: Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>
Subject: RE: [edk2-platforms][PATCH V1 1/2] KabylakeSiliconPkg: Remove ResetSystemLib.h override

Hi Sai,

I'd like to fix the VS2017 build failure asap. What would you like done to resolve this?

I would prefer to eliminate the local ResetSystemLib.h file in KabylakeSiliconPkg but I'd be happy to revise the patch series based on your suggestion.

Thanks,
Michael

> -----Original Message-----
> From: Kubacki, Michael A
> Sent: Wednesday, November 27, 2019 10:42 AM
> To: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>; 
> devel@edk2.groups.io
> Cc: Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L 
> <nathaniel.l.desimone@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>
> Subject: RE: [edk2-platforms][PATCH V1 1/2] KabylakeSiliconPkg: Remove 
> ResetSystemLib.h override
> 
> This dependency existed prior to this change (and still does exist). 
> It was obfuscated in such a way that contributed to this problem.
> 
> See the previous library header path:
> 
> \Silicon\Intel\KabylakeSiliconPkg\SampleCode\MdeModulePkg\Include\Libr
> ary\ResetSystemLib.h
> 
> The fact KabylakeSiliconPkg implements a MdeModulePkg library API 
> introduces the dependency on MdeModulePkg. Hiding a redundant 
> definition of the API locally does not eliminate the dependency in any 
> meaningful way.
> 
> I think the practice of "freezing" an API with a local copy only works 
> if the codebase is locked onto a specific stable tag in which the 
> upstream API is not expected to change. Zhichao rightfully added the 
> new function definition to the KabylakeSiliconPkg library class 
> implementation because a board package consumer would expect a 
> ResetSystemLib library class instance to be compliant with the API 
> defined in MdeModulePkg and link the ResetSystem
> () function. The only problem was a set of circumstances that led to 
> the duplicate symbol definition for ResetSystem () with PchResetLib.
> 
> So I view the task of eliminating the package dependency as a larger 
> separate effort outside the scope of this change. But I do not agree 
> with maintaining redundant local copies of edk2 APIs in packages in edk2-platforms.
> 
> Thanks,
> Michael
> 
> > -----Original Message-----
> > From: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>
> > Sent: Tuesday, November 26, 2019 10:43 PM
> > To: Kubacki, Michael A <michael.a.kubacki@intel.com>; 
> > devel@edk2.groups.io
> > Cc: Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L 
> > <nathaniel.l.desimone@intel.com>; Gao, Zhichao
> <zhichao.gao@intel.com>
> > Subject: RE: [edk2-platforms][PATCH V1 1/2] KabylakeSiliconPkg: 
> > Remove ResetSystemLib.h override
> >
> > This change is introducing SiliconPkg's dependency on MdeModulePkg.
> > SiliconPkg dependencies should be limited to a selected few packages 
> > and this seems to be an unnecessary addition to the dependency list.
> > The reset interfaces are providing generic reset services and 
> > perhaps better suited in packages like MdePkg.
> >
> > -----Original Message-----
> > From: Kubacki, Michael A
> > Sent: Tuesday, November 26, 2019 6:57 PM
> > To: devel@edk2.groups.io
> > Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>; Chiu, 
> > Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L 
> > <nathaniel.l.desimone@intel.com>; Gao, Zhichao
> <zhichao.gao@intel.com>
> > Subject: [edk2-platforms][PATCH V1 1/2] KabylakeSiliconPkg: Remove 
> > ResetSystemLib.h override
> >
> > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2375
> >
> > Removes a stale ResetSystemLib.h override in KabylakeSiliconPkg that 
> > does not contain the prototype for ResetSystem () and
> ResetPlatformSpecific ().
> >
> > The ResetSystemLib.h file from MdeModulePkg will be used. Any INF 
> > files that did not include the MdeModulePkg.dec under [Packages] 
> > were updated to do so.
> >
> > Cc: Sai Chaganty <rangasai.v.chaganty@intel.com>
> > Cc: Chasel Chiu <chasel.chiu@intel.com>
> > Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> > Cc: Zhichao Gao <zhichao.gao@intel.com>
> > Signed-off-by: Michael Kubacki <michael.a.kubacki@intel.com>
> > ---
> >
> >
> Silicon/Intel/KabylakeSiliconPkg/Pch/Library/DxeResetSystemLib/DxeRese
> tS
> > ystemLib.inf               |  3 +-
> >
> > Silicon/Intel/KabylakeSiliconPkg/Pch/Library/DxeRuntimeResetSystemLi
> > b/
> > Dx
> > eRuntimeResetSystemLib.inf |  3 +-
> >
> >
> Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiPchResetLib/PeiPchResetLib.
> > inf                     |  3 +-
> >
> >
> Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiResetSystemLib/PeiRese
> tSys
> > temLib.inf               |  3 +-
> >
> >
> Silicon/Intel/KabylakeSiliconPkg/SampleCode/MdeModulePkg/Include/Libra
> > ry/ResetSystemLib.h          | 62 --------------------
> >  5 files changed, 8 insertions(+), 66 deletions(-)
> >
> > diff --git
> > a/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/DxeResetSystemLib/Dxe
> > Re
> > se
> > tSystemLib.inf
> > b/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/DxeResetSystemLib/Dxe
> > Re
> > se
> > tSystemLib.inf
> > index aa8877140a..46313bf35f 100644
> > ---
> > a/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/DxeResetSystemLib/Dxe
> > Re
> > se
> > tSystemLib.inf
> > +++ b/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/DxeResetSystemLib
> > +++ /D
> > +++ xe
> > +++ ResetSystemLib.inf
> > @@ -1,7 +1,7 @@
> >  ## @file
> >  # Component description file for Intel Ich7 Reset System Library.
> >  #
> > -# Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
> > +# Copyright (c) 2017 - 2019, Intel Corporation. All rights 
> > +reserved.<BR>
> >  #
> >  # SPDX-License-Identifier: BSD-2-Clause-Patent  # @@ -35,6 +35,7 @@ 
> > PchCycleDecodingLib
> >
> >  [Packages]
> >  MdePkg/MdePkg.dec
> > +MdeModulePkg/MdeModulePkg.dec
> >  KabylakeSiliconPkg/SiPkg.dec
> >
> >
> > diff --git
> > a/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/DxeRuntimeResetSystem
> > Li
> > b/
> > DxeRuntimeResetSystemLib.inf
> > b/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/DxeRuntimeResetSystem
> > Li
> > b/
> > DxeRuntimeResetSystemLib.inf
> > index 6b27661603..c7fad31c71 100644
> > ---
> > a/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/DxeRuntimeResetSystem
> > Li
> > b/
> > DxeRuntimeResetSystemLib.inf
> > +++
> > b/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/DxeRuntimeResetSystem
> > +++ Lib/DxeRuntimeResetSystemLib.inf
> > @@ -1,7 +1,7 @@
> >  ## @file
> >  # Component description file for Intel Ich7 Reset System Library.
> >  #
> > -# Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
> > +# Copyright (c) 2017 - 2019, Intel Corporation. All rights 
> > +reserved.<BR>
> >  #
> >  # SPDX-License-Identifier: BSD-2-Clause-Patent  # @@ -36,6 +36,7 @@ 
> > PchCycleDecodingLib
> >
> >  [Packages]
> >  MdePkg/MdePkg.dec
> > +MdeModulePkg/MdeModulePkg.dec
> >  KabylakeSiliconPkg/SiPkg.dec
> >
> >
> > diff --git
> > a/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiPchResetLib/PeiPch
> > Re
> > setL
> > ib.inf
> > b/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiPchResetLib/PeiPch
> > Re
> > setL
> > ib.inf
> > index b04f4006ef..29f69078a4 100644
> > ---
> > a/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiPchResetLib/PeiPch
> > Re
> > setL
> > ib.inf
> > +++ b/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiPchResetLib/Pe
> > +++ iP
> > +++ ch
> > +++ ResetLib.inf
> > @@ -1,7 +1,7 @@
> >  ## @file
> >  # Component description file for PCH Reset Lib Pei Phase  # -# 
> > Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
> > +# Copyright (c) 2017 - 2019, Intel Corporation. All rights 
> > +reserved.<BR>
> >  #
> >  # SPDX-License-Identifier: BSD-2-Clause-Patent  # @@ -28,6 +28,7 @@ 
> > ResetSystemLib
> >
> >  [Packages]
> >  MdePkg/MdePkg.dec
> > +MdeModulePkg/MdeModulePkg.dec
> >  KabylakeSiliconPkg/SiPkg.dec
> >
> >  [Sources]
> > diff --git
> > a/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiResetSystemLib/Pei
> > Re
> > setS
> > ystemLib.inf
> > b/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiResetSystemLib/Pei
> > Re
> > set
> > SystemLib.inf
> > index 18a92a6f18..3c6ff78863 100644
> > ---
> > a/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiResetSystemLib/Pei
> > Re
> > setS
> > ystemLib.inf
> > +++ b/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiResetSystemLib
> > +++ /P
> > +++ ei
> > +++ ResetSystemLib.inf
> > @@ -1,7 +1,7 @@
> >  ## @file
> >  # Component description file for Intel Ich7 Reset System Library.
> >  #
> > -# Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
> > +# Copyright (c) 2017 - 2019, Intel Corporation. All rights 
> > +reserved.<BR>
> >  #
> >  # SPDX-License-Identifier: BSD-2-Clause-Patent  # @@ -32,6 +32,7 @@ 
> > PchCycleDecodingLib
> >
> >  [Packages]
> >  MdePkg/MdePkg.dec
> > +MdeModulePkg/MdeModulePkg.dec
> >  KabylakeSiliconPkg/SiPkg.dec
> >
> >
> > diff --git
> >
> a/Silicon/Intel/KabylakeSiliconPkg/SampleCode/MdeModulePkg/Include/Lib
> > rary/ResetSystemLib.h
> >
> b/Silicon/Intel/KabylakeSiliconPkg/SampleCode/MdeModulePkg/Include/Lib
> > rary/ResetSystemLib.h
> > deleted file mode 100644
> > index 75d3e15ed7..0000000000
> > ---
> >
> a/Silicon/Intel/KabylakeSiliconPkg/SampleCode/MdeModulePkg/Include/Lib
> > rary/ResetSystemLib.h
> > +++ /dev/null
> > @@ -1,62 +0,0 @@
> > -/** @file
> > -  System reset Library Services.  This library class defines a set 
> > of
> > -  methods that reset the whole system.
> > -
> > -Copyright (c) 2005 - 2010, Intel Corporation. All rights 
> > reserved.<BR>
> > -SPDX-License-Identifier: BSD-2-Clause-Patent
> > -
> > -**/
> > -
> > -#ifndef __RESET_SYSTEM_LIB_H__
> > -#define __RESET_SYSTEM_LIB_H__
> > -
> > -/**
> > -  This function causes a system-wide reset (cold reset), in which
> > -  all circuitry within the system returns to its initial state. 
> > This type of reset
> > -  is asynchronous to system operation and operates without regard 
> > to
> > -  cycle boundaries.
> > -
> > -  If this function returns, it means that the system does not 
> > support cold reset.
> > -**/
> > -VOID
> > -EFIAPI
> > -ResetCold (
> > -  VOID
> > -  );
> > -
> > -/**
> > -  This function causes a system-wide initialization (warm reset), 
> > in which all processors
> > -  are set to their initial state. Pending cycles are not corrupted.
> > -
> > -  If this function returns, it means that the system does not 
> > support warm reset.
> > -**/
> > -VOID
> > -EFIAPI
> > -ResetWarm (
> > -  VOID
> > -  );
> > -
> > -/**
> > -  This function causes the system to enter a power state equivalent
> > -  to the ACPI G2/S5 or G3 states.
> > -
> > -  If this function returns, it means that the system does not 
> > support shutdown reset.
> > -**/
> > -VOID
> > -EFIAPI
> > -ResetShutdown (
> > -  VOID
> > -  );
> > -
> > -/**
> > -  This function causes the system to enter S3 and then wake up
> immediately.
> > -
> > -  If this function returns, it means that the system does not 
> > support
> > S3 feature.
> > -**/
> > -VOID
> > -EFIAPI
> > -EnterS3WithImmediateWake (
> > -  VOID
> > -  );
> > -
> > -#endif
> > --
> > 2.16.2.windows.1
> >