[PATCH v2 0/1] PcAtChipsetPkg: RTC Runtime unable to get correct IO port by PCD

[PATCH v2 0/1] PcAtChipsetPkg: RTC Runtime unable to get correct IO port by PCD

[Guo, Gua]

From: Gua Guo <gua.guo@intel.com>

PR: https://github.com/tianocore/edk2/pull/4416

V2: Add more dynamic PCD for runtime service support instead of taking for FixedAtBuild case

V1: After https://bugzilla.tianocore.org/show_bug.cgi?id=4193
RTC Runtime Services unable to get correct RTC time.
Solution: Cache RTC data, update RTC driver not to use PcdGet after driver init.

Gua Guo (1):
  PcAtChipsetPkg: RTC Runtime unable to get correct IO port by PCD

 .../PcatRealTimeClockRuntimeDxe/PcRtc.c       | 38 ++++++++++---------
 .../PcatRealTimeClockRuntimeDxe/PcRtcEntry.c  | 16 +++++++-
 2 files changed, 35 insertions(+), 19 deletions(-)

--
2.39.2.windows.1

[PATCH v2 1/1] PcAtChipsetPkg: RTC Runtime unable to get correct IO port by PCD

[Guo, Gua]

From: Gua Guo <gua.guo@intel.com>

RTC runtime is unable to get dynamic PCD value after booting to
OS using runtime services.

Resolution: Cache the dynamic PCD value in RTC driver entry point

Cc: Ray Ni <ray.ni@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Signed-off-by: Gua Guo <gua.guo@intel.com>
---
 .../PcatRealTimeClockRuntimeDxe/PcRtc.c       | 38 ++++++++++---------
 .../PcatRealTimeClockRuntimeDxe/PcRtcEntry.c  | 16 +++++++-
 2 files changed, 35 insertions(+), 19 deletions(-)

diff --git a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c
index d8b9fa8376..795ec950a8 100644
--- a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c
+++ b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c
@@ -11,9 +11,13 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 
 #include "PcRtc.h"
 
-extern UINTN  mRtcIndexRegister;
-extern UINTN  mRtcTargetRegister;
-
+extern UINTN   mRtcIndexRegister;
+extern UINTN   mRtcTargetRegister;
+extern UINTN   mRtcIndexRegisterIo;
+extern UINTN   mRtcTargetRegisterIo;
+extern UINT16  mRtcDefaultYear;
+extern UINT16  mMinimalValidYear;
+extern UINT16  mMaximalValidYear;
 //
 // Days of month.
 //
@@ -72,10 +76,10 @@ IoRtcRead (
   )
 {
   IoWrite8 (
-    PcdGet8 (PcdRtcIndexRegister),
-    (UINT8)(Address | (UINT8)(IoRead8 (PcdGet8 (PcdRtcIndexRegister)) & 0x80))
+    mRtcIndexRegisterIo,
+    (UINT8)(Address | (UINT8)(IoRead8 (mRtcIndexRegisterIo) & 0x80))
     );
-  return IoRead8 (PcdGet8 (PcdRtcTargetRegister));
+  return IoRead8 (mRtcTargetRegisterIo);
 }
 
 /**
@@ -94,10 +98,10 @@ IoRtcWrite (
   )
 {
   IoWrite8 (
-    PcdGet8 (PcdRtcIndexRegister),
-    (UINT8)(Address | (UINT8)(IoRead8 (PcdGet8 (PcdRtcIndexRegister)) & 0x80))
+    mRtcIndexRegisterIo,
+    (UINT8)(Address | (UINT8)(IoRead8 (mRtcIndexRegisterIo) & 0x80))
     );
-  IoWrite8 (PcdGet8 (PcdRtcTargetRegister), Data);
+  IoWrite8 (mRtcTargetRegisterIo, Data);
 }
 
 /**
@@ -317,8 +321,8 @@ PcRtcInit (
     Time.Hour       = RTC_INIT_HOUR;
     Time.Day        = RTC_INIT_DAY;
     Time.Month      = RTC_INIT_MONTH;
-    Time.Year       = MAX (PcdGet16 (PcdRtcDefaultYear), PcdGet16 (PcdMinimalValidYear));
-    Time.Year       = MIN (Time.Year, PcdGet16 (PcdMaximalValidYear));
+    Time.Year       = MAX (mRtcDefaultYear, mMinimalValidYear);
+    Time.Year       = MIN (Time.Year, mMaximalValidYear);
     Time.Nanosecond = 0;
     Time.TimeZone   = EFI_UNSPECIFIED_TIMEZONE;
     Time.Daylight   = 0;
@@ -358,8 +362,8 @@ PcRtcInit (
   Time.Hour       = RTC_INIT_HOUR;
   Time.Day        = RTC_INIT_DAY;
   Time.Month      = RTC_INIT_MONTH;
-  Time.Year       = MAX (PcdGet16 (PcdRtcDefaultYear), PcdGet16 (PcdMinimalValidYear));
-  Time.Year       = MIN (Time.Year, PcdGet16 (PcdMaximalValidYear));
+  Time.Year       = MAX (mRtcDefaultYear, mMinimalValidYear);
+  Time.Year       = MIN (Time.Year, mMaximalValidYear);
   Time.Nanosecond = 0;
   Time.TimeZone   = Global->SavedTimeZone;
   Time.Daylight   = Global->Daylight;
@@ -1031,8 +1035,8 @@ ConvertRtcTimeToEfiTime (
   //   Century is 19 if RTC year >= 70,
   //   Century is 20 otherwise.
   //
-  Century = (UINT8)(PcdGet16 (PcdMinimalValidYear) / 100);
-  if (Time->Year < PcdGet16 (PcdMinimalValidYear) % 100) {
+  Century = (UINT8)(mMinimalValidYear / 100);
+  if (Time->Year < mMinimalValidYear % 100) {
     Century++;
   }
 
@@ -1114,8 +1118,8 @@ RtcTimeFieldsValid (
   IN EFI_TIME  *Time
   )
 {
-  if ((Time->Year < PcdGet16 (PcdMinimalValidYear)) ||
-      (Time->Year > PcdGet16 (PcdMaximalValidYear)) ||
+  if ((Time->Year < mMinimalValidYear) ||
+      (Time->Year > mMaximalValidYear) ||
       (Time->Month < 1) ||
       (Time->Month > 12) ||
       (!DayValid (Time)) ||
diff --git a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtcEntry.c b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtcEntry.c
index e6e9458d75..ba7e85cdfb 100644
--- a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtcEntry.c
+++ b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtcEntry.c
@@ -16,8 +16,13 @@ EFI_HANDLE  mHandle = NULL;
 
 STATIC EFI_EVENT  mVirtualAddrChangeEvent;
 
-UINTN  mRtcIndexRegister;
-UINTN  mRtcTargetRegister;
+UINTN   mRtcIndexRegister;
+UINTN   mRtcTargetRegister;
+UINTN   mRtcIndexRegisterIo;
+UINTN   mRtcTargetRegisterIo;
+UINT16  mRtcDefaultYear;
+UINT16  mMinimalValidYear;
+UINT16  mMaximalValidYear;
 
 /**
   Returns the current time and date information, and the time-keeping capabilities
@@ -164,8 +169,15 @@ InitializePcRtc (
   if (FeaturePcdGet (PcdRtcUseMmio)) {
     mRtcIndexRegister  = (UINTN)PcdGet64 (PcdRtcIndexRegister64);
     mRtcTargetRegister = (UINTN)PcdGet64 (PcdRtcTargetRegister64);
+  } else {
+    mRtcIndexRegisterIo  = (UINTN)PcdGet8 (PcdRtcIndexRegister);
+    mRtcTargetRegisterIo = (UINTN)PcdGet8 (PcdRtcTargetRegister);
   }
 
+  mRtcDefaultYear   = PcdGet16 (PcdRtcDefaultYear);
+  mMinimalValidYear = PcdGet16 (PcdMinimalValidYear);
+  mMaximalValidYear = PcdGet16 (PcdMaximalValidYear);
+
   Status = PcRtcInit (&mModuleGlobal);
   ASSERT_EFI_ERROR (Status);
 
-- 
2.39.2.windows.1

Re: [edk2-devel] [PATCH v2 1/1] PcAtChipsetPkg: RTC Runtime unable to get correct IO port by PCD

[Ni, Ray]

> 
> -extern UINTN  mRtcIndexRegister;
> 
> -extern UINTN  mRtcTargetRegister;
> 
> -
> 
> +extern UINTN   mRtcIndexRegister;
> 
> +extern UINTN   mRtcTargetRegister;
> 
> +extern UINTN   mRtcIndexRegisterIo;
> 
> +extern UINTN   mRtcTargetRegisterIo;

Can you avoid adding the mRtcIndexRegisterIo/ mRtcTargetRegisterIo?
Can you just use the old variables mRtcIndexRegister/ mRtcTargetRegister.
 

Re: [edk2-devel] [PATCH v2 1/1] PcAtChipsetPkg: RTC Runtime unable to get correct IO port by PCD

[Guo, Gua]

Thank for the review, will update v4 to take care it.

-----Original Message-----
From: Ni, Ray <ray.ni@intel.com> 
Sent: Wednesday, May 24, 2023 4:14 PM
To: devel@edk2.groups.io; Guo, Gua <gua.guo@intel.com>
Cc: Kinney, Michael D <michael.d.kinney@intel.com>
Subject: RE: [edk2-devel] [PATCH v2 1/1] PcAtChipsetPkg: RTC Runtime unable to get correct IO port by PCD

> 
> -extern UINTN  mRtcIndexRegister;
> 
> -extern UINTN  mRtcTargetRegister;
> 
> -
> 
> +extern UINTN   mRtcIndexRegister;
> 
> +extern UINTN   mRtcTargetRegister;
> 
> +extern UINTN   mRtcIndexRegisterIo;
> 
> +extern UINTN   mRtcTargetRegisterIo;

Can you avoid adding the mRtcIndexRegisterIo/ mRtcTargetRegisterIo?
Can you just use the old variables mRtcIndexRegister/ mRtcTargetRegister.