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

["Kubacki, Michael A" <michael.a.kubacki@intel.com>]

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/DxeResetS
> > ystemLib.inf               |  3 +-
> >
> > Silicon/Intel/KabylakeSiliconPkg/Pch/Library/DxeRuntimeResetSystemLib/
> > Dx
> > eRuntimeResetSystemLib.inf |  3 +-
> >
> >
> Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiPchResetLib/PeiPchResetLib.
> > inf                     |  3 +-
> >
> >
> Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiResetSystemLib/PeiResetSys
> > 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/DxeRe
> > se
> > tSystemLib.inf
> > b/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/DxeResetSystemLib/DxeRe
> > se
> > tSystemLib.inf
> > index aa8877140a..46313bf35f 100644
> > ---
> > a/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/DxeResetSystemLib/DxeRe
> > 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/DxeRuntimeResetSystemLi
> > b/
> > DxeRuntimeResetSystemLib.inf
> > b/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/DxeRuntimeResetSystemLi
> > b/
> > DxeRuntimeResetSystemLib.inf
> > index 6b27661603..c7fad31c71 100644
> > ---
> > a/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/DxeRuntimeResetSystemLi
> > 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/PeiPchRe
> > setL
> > ib.inf
> > b/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiPchResetLib/PeiPchRe
> > setL
> > ib.inf
> > index b04f4006ef..29f69078a4 100644
> > ---
> > a/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiPchResetLib/PeiPchRe
> > setL
> > ib.inf
> > +++ b/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiPchResetLib/PeiP
> > +++ 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/PeiRe
> > setS
> > ystemLib.inf
> > b/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiResetSystemLib/PeiRe
> > set
> > SystemLib.inf
> > index 18a92a6f18..3c6ff78863 100644
> > ---
> > a/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiResetSystemLib/PeiRe
> > 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
> >