[PATCH] PcAtChipsetPkg: Change the flow of PcRtcInit()

[PATCH] PcAtChipsetPkg: Change the flow of PcRtcInit()

[zhuoran.chao]

REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3913

The original flow of PcRtcInit() is like:
1. Guarantee atomic accesses to the RTC time registers and
   read out the value.
2. Program RTC register B. (adopt 12h mode or 24h mode. Current
   bios code sets RTC to 24h mode by default).
3. Then function ConvertRtcTimeToEfiTime converts the RTC time
   value to their 24h mode by checking the hour format bit
   (1:24h mode,0:12h mode).
And here lies the problem: Step3 will fail to adjust the value
if Step2 already sets RTC to 24h mode. The hour value in 12h mode
will not be converted to its 24h mode.

The solution is to program RTC register B a little later when all
the original RTC registers' value is retrieved, adjusted
and validated.

Cc: Ray Ni <ray.ni@intel.com>

Signed-off-by: Zhuoran Chao <zhuoran.chao@intel.com>
---
 PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c
index 0fbfa4bcee..ea56a66267 100644
--- a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c
+++ b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c
@@ -269,13 +269,6 @@ PcRtcInit (
   Time.Month  = RtcRead (RTC_ADDRESS_MONTH);
   Time.Year   = RtcRead (RTC_ADDRESS_YEAR);
 
-  //
-  // Set RTC configuration after get original time
-  // The value of bit AIE should be reserved.
-  //
-  RegisterB.Data = FixedPcdGet8 (PcdInitialValueRtcRegisterB) | (RegisterB.Data & BIT5);
-  RtcWrite (RTC_ADDRESS_REGISTER_B, RegisterB.Data);
-
   //
   // Release RTC Lock.
   //
@@ -330,6 +323,13 @@ PcRtcInit (
     Time.Daylight   = 0;
   }
 
+  //
+  // Set RTC configuration after get original time
+  // The value of bit AIE should be reserved.
+  //
+  RegisterB.Data = FixedPcdGet8 (PcdInitialValueRtcRegisterB) | (RegisterB.Data & BIT5);
+  RtcWrite (RTC_ADDRESS_REGISTER_B, RegisterB.Data);
+
   //
   // Reset time value according to new RTC configuration
   //
-- 
2.31.1.windows.1

Re: [PATCH] PcAtChipsetPkg: Change the flow of PcRtcInit()

[Zhuoran Chao]

Hi Liming:
Could you help to give a nudge on the below issue because it is related to a bug fixing of Intel server platform enabling team.

Thanks
Zhuoran

-----Original Message-----
From: Chao, Zhuoran <zhuoran.chao@intel.com> 
Sent: Monday, April 25, 2022 11:47 PM
To: devel@edk2.groups.io
Cc: Chao, Zhuoran <zhuoran.chao@intel.com>; Ni, Ray <ray.ni@intel.com>
Subject: [PATCH] PcAtChipsetPkg: Change the flow of PcRtcInit()

REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3913

The original flow of PcRtcInit() is like:
1. Guarantee atomic accesses to the RTC time registers and
   read out the value.
2. Program RTC register B. (adopt 12h mode or 24h mode. Current
   bios code sets RTC to 24h mode by default).
3. Then function ConvertRtcTimeToEfiTime converts the RTC time
   value to their 24h mode by checking the hour format bit
   (1:24h mode,0:12h mode).
And here lies the problem: Step3 will fail to adjust the value if Step2 already sets RTC to 24h mode. The hour value in 12h mode will not be converted to its 24h mode.

The solution is to program RTC register B a little later when all the original RTC registers' value is retrieved, adjusted and validated.

Cc: Ray Ni <ray.ni@intel.com>

Signed-off-by: Zhuoran Chao <zhuoran.chao@intel.com>
---
 PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c
index 0fbfa4bcee..ea56a66267 100644
--- a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c
+++ b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c
@@ -269,13 +269,6 @@ PcRtcInit (
   Time.Month  = RtcRead (RTC_ADDRESS_MONTH);
   Time.Year   = RtcRead (RTC_ADDRESS_YEAR);
 
-  //
-  // Set RTC configuration after get original time
-  // The value of bit AIE should be reserved.
-  //
-  RegisterB.Data = FixedPcdGet8 (PcdInitialValueRtcRegisterB) | (RegisterB.Data & BIT5);
-  RtcWrite (RTC_ADDRESS_REGISTER_B, RegisterB.Data);
-
   //
   // Release RTC Lock.
   //
@@ -330,6 +323,13 @@ PcRtcInit (
     Time.Daylight   = 0;
   }
 
+  //
+  // Set RTC configuration after get original time  // The value of bit 
+ AIE should be reserved.
+  //
+  RegisterB.Data = FixedPcdGet8 (PcdInitialValueRtcRegisterB) | 
+ (RegisterB.Data & BIT5);  RtcWrite (RTC_ADDRESS_REGISTER_B, 
+ RegisterB.Data);
+
   //
   // Reset time value according to new RTC configuration
   //
--
2.31.1.windows.1

回复: [edk2-devel] [PATCH] PcAtChipsetPkg: Change the flow of PcRtcInit()

[gaoliming]

Zhuoran:
  Ray is the maintainer of PcAtChipsetPkg. Can you collect his feedback on
this change?

Thanks
Liming
> -----邮件原件-----
> 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Zhuoran Chao
> 发送时间: 2022年4月29日 13:43
> 收件人: devel@edk2.groups.io; Gao, Liming <gaoliming@byosoft.com.cn>
> 抄送: Ni, Ray <ray.ni@intel.com>
> 主题: Re: [edk2-devel] [PATCH] PcAtChipsetPkg: Change the flow of
> PcRtcInit()
> 
> Hi Liming:
> Could you help to give a nudge on the below issue because it is related to
a
> bug fixing of Intel server platform enabling team.
> 
> Thanks
> Zhuoran
> 
> -----Original Message-----
> From: Chao, Zhuoran <zhuoran.chao@intel.com>
> Sent: Monday, April 25, 2022 11:47 PM
> To: devel@edk2.groups.io
> Cc: Chao, Zhuoran <zhuoran.chao@intel.com>; Ni, Ray <ray.ni@intel.com>
> Subject: [PATCH] PcAtChipsetPkg: Change the flow of PcRtcInit()
> 
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3913
> 
> The original flow of PcRtcInit() is like:
> 1. Guarantee atomic accesses to the RTC time registers and
>    read out the value.
> 2. Program RTC register B. (adopt 12h mode or 24h mode. Current
>    bios code sets RTC to 24h mode by default).
> 3. Then function ConvertRtcTimeToEfiTime converts the RTC time
>    value to their 24h mode by checking the hour format bit
>    (1:24h mode,0:12h mode).
> And here lies the problem: Step3 will fail to adjust the value if Step2
already
> sets RTC to 24h mode. The hour value in 12h mode will not be converted to
its
> 24h mode.
> 
> The solution is to program RTC register B a little later when all the
original RTC
> registers' value is retrieved, adjusted and validated.
> 
> Cc: Ray Ni <ray.ni@intel.com>
> 
> Signed-off-by: Zhuoran Chao <zhuoran.chao@intel.com>
> ---
>  PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c
> b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c
> index 0fbfa4bcee..ea56a66267 100644
> --- a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c
> +++ b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c
> @@ -269,13 +269,6 @@ PcRtcInit (
>    Time.Month  = RtcRead (RTC_ADDRESS_MONTH);
>    Time.Year   = RtcRead (RTC_ADDRESS_YEAR);
> 
> -  //
> -  // Set RTC configuration after get original time
> -  // The value of bit AIE should be reserved.
> -  //
> -  RegisterB.Data = FixedPcdGet8 (PcdInitialValueRtcRegisterB) |
> (RegisterB.Data & BIT5);
> -  RtcWrite (RTC_ADDRESS_REGISTER_B, RegisterB.Data);
> -
>    //
>    // Release RTC Lock.
>    //
> @@ -330,6 +323,13 @@ PcRtcInit (
>      Time.Daylight   = 0;
>    }
> 
> +  //
> +  // Set RTC configuration after get original time  // The value of bit
> + AIE should be reserved.
> +  //
> +  RegisterB.Data = FixedPcdGet8 (PcdInitialValueRtcRegisterB) |
> + (RegisterB.Data & BIT5);  RtcWrite (RTC_ADDRESS_REGISTER_B,
> + RegisterB.Data);
> +
>    //
>    // Reset time value according to new RTC configuration
>    //
> --
> 2.31.1.windows.1
> 
> 
> 
> 
>