Re: [PATCH v2] PcAtChipsetPkg: Add PeiAcpiTimerLib to save PerformanceCounterFrequency in HOB

["Gao, Liming" <liming.gao@intel.com>]

Star:
  Thanks for you feedback. I will update the patch and push it. 

>-----Original Message-----
>From: Zeng, Star
>Sent: Friday, January 26, 2018 1:57 PM
>To: Gao, Liming <liming.gao@intel.com>; edk2-devel@lists.01.org
>Cc: Zeng, Star <star.zeng@intel.com>
>Subject: RE: [edk2] [PATCH v2] PcAtChipsetPkg: Add PeiAcpiTimerLib to save
>PerformanceCounterFrequency in HOB
>
>Hi Liming,
>
>Reviewed-by: Star Zeng <star.zeng@intel.com>
>
>Two minor comments. Just for your information.
>1. Notice the title, it may be too long.
>2. In PeiAcpiTimerLib.c, you may need to add ASSERT
>(PerformanceCounterFrequency != NULL) after BuildGuidHob.
>And you can move " PerformanceCounterFrequency =
>(UINT64*)GET_GUID_HOB_DATA (GuidHob); " to the else condition, then
>only one "return" is needed.
>
>+  PerformanceCounterFrequency = NULL;
>+  GuidHob = GetFirstGuidHob (&mFrequencyHobGuid);
>+  if (GuidHob == NULL) {
>+    PerformanceCounterFrequency  =
>(UINT64*)BuildGuidHob(&mFrequencyHobGuid, sizeof
>(*PerformanceCounterFrequency));
>+    *PerformanceCounterFrequency = InternalCalculateTscFrequency ();
>+    return *PerformanceCounterFrequency;
>+  }
>+  PerformanceCounterFrequency = (UINT64*)GET_GUID_HOB_DATA
>(GuidHob);
>+
>+  return  *PerformanceCounterFrequency;
>+}
>
>
>Thanks,
>Star
>-----Original Message-----
>From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>Liming Gao
>Sent: Tuesday, January 23, 2018 10:24 AM
>To: edk2-devel@lists.01.org
>Cc: Zeng, Star <star.zeng@intel.com>
>Subject: [edk2] [PATCH v2] PcAtChipsetPkg: Add PeiAcpiTimerLib to save
>PerformanceCounterFrequency in HOB
>
>In V2:
>1) Update PeiAcpiTimerLib base name to PeiAcpiTimerLib
>2) Update PeiAcpiTimerLib to add the missing constructor to enable ACPI IO
>space
>3) Update DxeAcpiTimerLib to cache frequency in constructor.
>
>PeiAcpiTimerLib caches PerformanceCounterFrequency in HOB, then Pei and
>Dxe
>AcpiTimerLib can share the same PerformanceCounterFrequency.
>
>Contributed-under: TianoCore Contribution Agreement 1.1
>Signed-off-by: Liming Gao <liming.gao@intel.com>
>Cc: Star Zeng <star.zeng@intel.com>
>---
> PcAtChipsetPkg/Library/AcpiTimerLib/AcpiTimerLib.c |  4 +-
> .../Library/AcpiTimerLib/DxeAcpiTimerLib.c         | 57 +++++++++++++++++-
> .../Library/AcpiTimerLib/DxeAcpiTimerLib.inf       |  7 ++-
> .../Library/AcpiTimerLib/PeiAcpiTimerLib.c         | 68
>++++++++++++++++++++++
> .../Library/AcpiTimerLib/PeiAcpiTimerLib.inf       | 56 ++++++++++++++++++
> .../Library/AcpiTimerLib/PeiAcpiTimerLib.uni       | 23 ++++++++
> PcAtChipsetPkg/PcAtChipsetPkg.dsc                  |  4 +-
> 7 files changed, 211 insertions(+), 8 deletions(-)
> create mode 100644 PcAtChipsetPkg/Library/AcpiTimerLib/PeiAcpiTimerLib.c
> create mode 100644
>PcAtChipsetPkg/Library/AcpiTimerLib/PeiAcpiTimerLib.inf
> create mode 100644
>PcAtChipsetPkg/Library/AcpiTimerLib/PeiAcpiTimerLib.uni
>
>diff --git a/PcAtChipsetPkg/Library/AcpiTimerLib/AcpiTimerLib.c
>b/PcAtChipsetPkg/Library/AcpiTimerLib/AcpiTimerLib.c
>index 792781a33f..2f3cb4bca4 100644
>--- a/PcAtChipsetPkg/Library/AcpiTimerLib/AcpiTimerLib.c
>+++ b/PcAtChipsetPkg/Library/AcpiTimerLib/AcpiTimerLib.c
>@@ -1,7 +1,7 @@
> /** @file
>   ACPI Timer implements one instance of Timer Library.
>
>-  Copyright (c) 2013 - 2016, Intel Corporation. All rights reserved.<BR>
>+  Copyright (c) 2013 - 2018, Intel Corporation. All rights reserved.<BR>
>   This program and the accompanying materials
>   are licensed and made available under the terms and conditions of the BSD
>License
>   which accompanies this distribution.  The full text of the license may be
>found at
>@@ -21,6 +21,8 @@
> #include <Library/DebugLib.h>
> #include <IndustryStandard/Acpi.h>
>
>+GUID mFrequencyHobGuid = { 0x3fca54f6, 0xe1a2, 0x4b20, { 0xbe, 0x76,
>0x92, 0x6b, 0x4b, 0x48, 0xbf, 0xaa }};
>+
> /**
>   Internal function to retrieves the 64-bit frequency in Hz.
>
>diff --git a/PcAtChipsetPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.c
>b/PcAtChipsetPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.c
>index b141c680fb..67e18a1360 100644
>--- a/PcAtChipsetPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.c
>+++ b/PcAtChipsetPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.c
>@@ -12,9 +12,27 @@
>
> **/
>
>-#include <Base.h>
>+#include <PiDxe.h>
> #include <Library/TimerLib.h>
> #include <Library/BaseLib.h>
>+#include <Library/HobLib.h>
>+
>+extern GUID mFrequencyHobGuid;
>+
>+/**
>+  The constructor function enables ACPI IO space.
>+
>+  If ACPI I/O space not enabled, this function will enable it.
>+  It will always return RETURN_SUCCESS.
>+
>+  @retval EFI_SUCCESS   The constructor always returns RETURN_SUCCESS.
>+
>+**/
>+RETURN_STATUS
>+EFIAPI
>+AcpiTimerLibConstructor (
>+  VOID
>+  );
>
> /**
>   Calculate TSC frequency.
>@@ -54,8 +72,41 @@ InternalGetPerformanceCounterFrequency (
>   VOID
>   )
> {
>-  if (mPerformanceCounterFrequency == 0) {
>+  return  mPerformanceCounterFrequency;
>+}
>+
>+/**
>+  The constructor function enables ACPI IO space, and caches
>PerformanceCounterFrequency.
>+
>+  @param  ImageHandle   The firmware allocated handle for the EFI image.
>+  @param  SystemTable   A pointer to the EFI System Table.
>+
>+  @retval EFI_SUCCESS   The constructor always returns RETURN_SUCCESS.
>+
>+**/
>+EFI_STATUS
>+EFIAPI
>+DxeAcpiTimerLibConstructor (
>+  IN EFI_HANDLE        ImageHandle,
>+  IN EFI_SYSTEM_TABLE  *SystemTable
>+  )
>+{
>+  EFI_HOB_GUID_TYPE   *GuidHob;
>+
>+  //
>+  // Enable ACPI IO space.
>+  //
>+  AcpiTimerLibConstructor ();
>+
>+  //
>+  // Initialize PerformanceCounterFrequency
>+  //
>+  GuidHob = GetFirstGuidHob (&mFrequencyHobGuid);
>+  if (GuidHob != NULL) {
>+    mPerformanceCounterFrequency = *(UINT64*)GET_GUID_HOB_DATA
>(GuidHob);
>+  } else {
>     mPerformanceCounterFrequency = InternalCalculateTscFrequency ();
>   }
>-  return  mPerformanceCounterFrequency;
>+
>+  return EFI_SUCCESS;
> }
>diff --git a/PcAtChipsetPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
>b/PcAtChipsetPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
>index 2c1cc7d33c..601041c801 100644
>--- a/PcAtChipsetPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
>+++ b/PcAtChipsetPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
>@@ -7,7 +7,7 @@
> #  Note: The implementation uses the lower 24-bits of the ACPI timer and
> #  is compatible with both 24-bit and 32-bit ACPI timers.
> #
>-#  Copyright (c) 2013 - 2016, Intel Corporation. All rights reserved.<BR>
>+#  Copyright (c) 2013 - 2018, Intel Corporation. All rights reserved.<BR>
> #  This program and the accompanying materials
> #  are licensed and made available under the terms and conditions of the BSD
>License
> #  which accompanies this distribution.  The full text of the license may be
>found at
>@@ -22,10 +22,10 @@
>   INF_VERSION                    = 0x00010005
>   BASE_NAME                      = DxeAcpiTimerLib
>   FILE_GUID                      = E624B98C-845A-4b94-9B50-B20475D552B9
>-  MODULE_TYPE                    = BASE
>+  MODULE_TYPE                    = DXE_DRIVER
>   VERSION_STRING                 = 1.0
>   LIBRARY_CLASS                  = TimerLib|DXE_CORE DXE_DRIVER
>DXE_RUNTIME_DRIVER DXE_SMM_DRIVER UEFI_APPLICATION UEFI_DRIVER
>SMM_CORE
>-  CONSTRUCTOR                    = AcpiTimerLibConstructor
>+  CONSTRUCTOR                    = DxeAcpiTimerLibConstructor
>   MODULE_UNI_FILE                = DxeAcpiTimerLib.uni
>
> [Sources]
>@@ -42,6 +42,7 @@
>   PciLib
>   IoLib
>   DebugLib
>+  HobLib
>
> [Pcd]
>   gPcAtChipsetPkgTokenSpaceGuid.PcdAcpiIoPciBusNumber             ##
>CONSUMES
>diff --git a/PcAtChipsetPkg/Library/AcpiTimerLib/PeiAcpiTimerLib.c
>b/PcAtChipsetPkg/Library/AcpiTimerLib/PeiAcpiTimerLib.c
>new file mode 100644
>index 0000000000..a4ed6a4434
>--- /dev/null
>+++ b/PcAtChipsetPkg/Library/AcpiTimerLib/PeiAcpiTimerLib.c
>@@ -0,0 +1,68 @@
>+/** @file
>+  ACPI Timer implements one instance of Timer Library.
>+
>+  Copyright (c) 2013 - 2018, Intel Corporation. All rights reserved.<BR>
>+  This program and the accompanying materials
>+  are licensed and made available under the terms and conditions of the BSD
>License
>+  which accompanies this distribution.  The full text of the license may be
>found at
>+  http://opensource.org/licenses/bsd-license.php
>+
>+  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"
>BASIS,
>+  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER
>EXPRESS OR IMPLIED.
>+
>+**/
>+
>+#include <PiPei.h>
>+#include <Library/TimerLib.h>
>+#include <Library/BaseLib.h>
>+#include <Library/HobLib.h>
>+
>+extern GUID mFrequencyHobGuid;
>+
>+/**
>+  Calculate TSC frequency.
>+
>+  The TSC counting frequency is determined by comparing how far it counts
>+  during a 101.4 us period as determined by the ACPI timer.
>+  The ACPI timer is used because it counts at a known frequency.
>+  The TSC is sampled, followed by waiting 363 counts of the ACPI timer,
>+  or 101.4 us. The TSC is then sampled again. The difference multiplied by
>+  9861 is the TSC frequency. There will be a small error because of the
>+  overhead of reading the ACPI timer. An attempt is made to determine and
>+  compensate for this error.
>+
>+  @return The number of TSC counts per second.
>+
>+**/
>+UINT64
>+InternalCalculateTscFrequency (
>+  VOID
>+  );
>+
>+/**
>+  Internal function to retrieves the 64-bit frequency in Hz.
>+
>+  Internal function to retrieves the 64-bit frequency in Hz.
>+
>+  @return The frequency in Hz.
>+
>+**/
>+UINT64
>+InternalGetPerformanceCounterFrequency (
>+  VOID
>+  )
>+{
>+  UINT64              *PerformanceCounterFrequency;
>+  EFI_HOB_GUID_TYPE   *GuidHob;
>+
>+  PerformanceCounterFrequency = NULL;
>+  GuidHob = GetFirstGuidHob (&mFrequencyHobGuid);
>+  if (GuidHob == NULL) {
>+    PerformanceCounterFrequency  =
>(UINT64*)BuildGuidHob(&mFrequencyHobGuid, sizeof
>(*PerformanceCounterFrequency));
>+    *PerformanceCounterFrequency = InternalCalculateTscFrequency ();
>+    return *PerformanceCounterFrequency;
>+  }
>+  PerformanceCounterFrequency = (UINT64*)GET_GUID_HOB_DATA
>(GuidHob);
>+
>+  return  *PerformanceCounterFrequency;
>+}
>diff --git a/PcAtChipsetPkg/Library/AcpiTimerLib/PeiAcpiTimerLib.inf
>b/PcAtChipsetPkg/Library/AcpiTimerLib/PeiAcpiTimerLib.inf
>new file mode 100644
>index 0000000000..1d8c95a3ec
>--- /dev/null
>+++ b/PcAtChipsetPkg/Library/AcpiTimerLib/PeiAcpiTimerLib.inf
>@@ -0,0 +1,56 @@
>+## @file
>+#  PEI ACPI Timer Library
>+#
>+#  Provides basic timer support using the ACPI timer hardware.  The
>performance
>+#  counter features are provided by the processors time stamp counter.
>+#
>+#  Note: The implementation uses the lower 24-bits of the ACPI timer and
>+#  is compatible with both 24-bit and 32-bit ACPI timers.
>+#
>+#  Copyright (c) 2013 - 2018, Intel Corporation. All rights reserved.<BR>
>+#  This program and the accompanying materials
>+#  are licensed and made available under the terms and conditions of the
>BSD License
>+#  which accompanies this distribution.  The full text of the license may be
>found at
>+#  http://opensource.org/licenses/bsd-license.php
>+#
>+#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"
>BASIS,
>+#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER
>EXPRESS OR IMPLIED.
>+#
>+##
>+
>+[Defines]
>+  INF_VERSION                    = 0x00010005
>+  BASE_NAME                      = PeiAcpiTimerLib
>+  FILE_GUID                      = 3FCA54F6-E1A2-4B20-BE76-926B4B48BFAA
>+  MODULE_TYPE                    = BASE
>+  VERSION_STRING                 = 1.0
>+  LIBRARY_CLASS                  = TimerLib|PEI_CORE PEIM
>+  CONSTRUCTOR                    = AcpiTimerLibConstructor
>+  MODULE_UNI_FILE                = PeiAcpiTimerLib.uni
>+
>+[Sources]
>+  AcpiTimerLib.c
>+  PeiAcpiTimerLib.c
>+
>+[Packages]
>+  MdePkg/MdePkg.dec
>+  PcAtChipsetPkg/PcAtChipsetPkg.dec
>+
>+[LibraryClasses]
>+  BaseLib
>+  PcdLib
>+  PciLib
>+  IoLib
>+  DebugLib
>+  HobLib
>+
>+[Pcd]
>+  gPcAtChipsetPkgTokenSpaceGuid.PcdAcpiIoPciBusNumber             ##
>CONSUMES
>+  gPcAtChipsetPkgTokenSpaceGuid.PcdAcpiIoPciDeviceNumber          ##
>CONSUMES
>+  gPcAtChipsetPkgTokenSpaceGuid.PcdAcpiIoPciFunctionNumber        ##
>CONSUMES
>+  gPcAtChipsetPkgTokenSpaceGuid.PcdAcpiIoPciEnableRegisterOffset  ##
>CONSUMES
>+  gPcAtChipsetPkgTokenSpaceGuid.PcdAcpiIoBarEnableMask            ##
>CONSUMES
>+  gPcAtChipsetPkgTokenSpaceGuid.PcdAcpiIoPciBarRegisterOffset     ##
>CONSUMES
>+  gPcAtChipsetPkgTokenSpaceGuid.PcdAcpiIoPortBaseAddress          ##
>CONSUMES
>+  gPcAtChipsetPkgTokenSpaceGuid.PcdAcpiPm1TmrOffset               ##
>CONSUMES
>+  gPcAtChipsetPkgTokenSpaceGuid.PcdAcpiIoPortBaseAddressMask      ##
>CONSUMES
>diff --git a/PcAtChipsetPkg/Library/AcpiTimerLib/PeiAcpiTimerLib.uni
>b/PcAtChipsetPkg/Library/AcpiTimerLib/PeiAcpiTimerLib.uni
>new file mode 100644
>index 0000000000..668161b14a
>--- /dev/null
>+++ b/PcAtChipsetPkg/Library/AcpiTimerLib/PeiAcpiTimerLib.uni
>@@ -0,0 +1,23 @@
>+// /** @file
>+// PEI ACPI Timer Library
>+//
>+// Provides basic timer support using the ACPI timer hardware.  The
>performance
>+// counter features are provided by the processors time stamp counter.
>+//
>+// Copyright (c) 2018, Intel Corporation. All rights reserved.<BR>
>+//
>+// This program and the accompanying materials
>+// are licensed and made available under the terms and conditions of the
>BSD License
>+// which accompanies this distribution.  The full text of the license may be
>found at
>+// http://opensource.org/licenses/bsd-license.php
>+//
>+// THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"
>BASIS,
>+// WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER
>EXPRESS OR IMPLIED.
>+//
>+// **/
>+
>+
>+#string STR_MODULE_ABSTRACT             #language en-US "ACPI Timer
>Library"
>+
>+#string STR_MODULE_DESCRIPTION          #language en-US "Provides basic
>timer support using the ACPI timer hardware."
>+
>diff --git a/PcAtChipsetPkg/PcAtChipsetPkg.dsc
>b/PcAtChipsetPkg/PcAtChipsetPkg.dsc
>index b740f00e63..2395e9cf26 100644
>--- a/PcAtChipsetPkg/PcAtChipsetPkg.dsc
>+++ b/PcAtChipsetPkg/PcAtChipsetPkg.dsc
>@@ -1,7 +1,7 @@
> ## @file
> #  PC/AT Chipset Package
> #
>-#  Copyright (c) 2007 - 2016, Intel Corporation. All rights reserved.<BR>
>+#  Copyright (c) 2007 - 2018, Intel Corporation. All rights reserved.<BR>
> #
> #  This program and the accompanying materials
> #  are licensed and made available under the terms and conditions of the BSD
>License
>@@ -46,6 +46,7 @@
>   IoApicLib|PcAtChipsetPkg/Library/BaseIoApicLib/BaseIoApicLib.inf
>   LocalApicLib|UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.inf
>
>ReportStatusCodeLib|MdePkg/Library/BaseReportStatusCodeLibNull/BaseRe
>portStatusCodeLibNull.inf
>+  HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
>
> [Components]
>   PcAtChipsetPkg/8254TimerDxe/8254Timer.inf
>@@ -57,6 +58,7 @@
>   PcAtChipsetPkg/Library/BaseIoApicLib/BaseIoApicLib.inf
>   PcAtChipsetPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.inf
>   PcAtChipsetPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
>+  PcAtChipsetPkg/Library/AcpiTimerLib/PeiAcpiTimerLib.inf
>
>PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcatRealTimeClockRuntimeD
>xe.inf
>
> [BuildOptions]
>--
>2.11.0.windows.1
>
>_______________________________________________
>edk2-devel mailing list
>edk2-devel@lists.01.org
>https://lists.01.org/mailman/listinfo/edk2-devel