[edk2-devel] [edk2-platforms][PATCH V1 1/2] KabylakeSiliconPkg: Remove ResetSystemLib.h override
Kubacki, Michael A via Groups.Io
michael.a.kubacki=intel.com at groups.io
Wed Nov 27 18:41:33 UTC 2019
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\Library\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 at intel.com>
> Sent: Tuesday, November 26, 2019 10:43 PM
> To: Kubacki, Michael A <michael.a.kubacki at intel.com>;
> devel at edk2.groups.io
> Cc: Chiu, Chasel <chasel.chiu at intel.com>; Desimone, Nathaniel L
> <nathaniel.l.desimone at intel.com>; Gao, Zhichao <zhichao.gao at 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 at edk2.groups.io
> Cc: Chaganty, Rangasai V <rangasai.v.chaganty at intel.com>; Chiu, Chasel
> <chasel.chiu at intel.com>; Desimone, Nathaniel L
> <nathaniel.l.desimone at intel.com>; Gao, Zhichao <zhichao.gao at 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 at intel.com>
> Cc: Chasel Chiu <chasel.chiu at intel.com>
> Cc: Nate DeSimone <nathaniel.l.desimone at intel.com>
> Cc: Zhichao Gao <zhichao.gao at intel.com>
> Signed-off-by: Michael Kubacki <michael.a.kubacki at 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/DxeRese
> tSystemLib.inf
> b/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/DxeResetSystemLib/DxeRese
> tSystemLib.inf
> index aa8877140a..46313bf35f 100644
> ---
> a/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/DxeResetSystemLib/DxeRese
> tSystemLib.inf
> +++ b/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/DxeResetSystemLib/Dxe
> +++ 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/DxeRuntimeResetSystemLib/
> DxeRuntimeResetSystemLib.inf
> b/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/DxeRuntimeResetSystemLib/
> DxeRuntimeResetSystemLib.inf
> index 6b27661603..c7fad31c71 100644
> ---
> a/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/DxeRuntimeResetSystemLib/
> 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/PeiPchResetL
> ib.inf
> b/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiPchResetLib/PeiPchResetL
> ib.inf
> index b04f4006ef..29f69078a4 100644
> ---
> a/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiPchResetLib/PeiPchResetL
> ib.inf
> +++ b/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiPchResetLib/PeiPch
> +++ 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/PeiResetS
> ystemLib.inf
> b/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiResetSystemLib/PeiReset
> SystemLib.inf
> index 18a92a6f18..3c6ff78863 100644
> ---
> a/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiResetSystemLib/PeiResetS
> ystemLib.inf
> +++ b/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiResetSystemLib/Pei
> +++ 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
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#51362): https://edk2.groups.io/g/devel/message/51362
Mute This Topic: https://groups.io/mt/62194086/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list