Laszlo, My understanding is: you have concern why I only call GetCenturyRtcAddress() in RTC driver's entry point, but don't write the century value to CMOS. Is my understanding correct?
Because RTC's entry point calls PcRtcInit(), which calls PcRtcSetTime(), which writes the century value to CMOS, when the century RTC address is valid. Before the patch, RTC driver doesn't know the century RTC address in its entry point, so the address was assigned to zero. (It thought this address can only be known when the ACPI table callback was called. I guess my head was kicked very heavily when writing this code:() But now with HP's report, I found the driver could know the century RTC address in its entry point. So I made such change. From my point of view, this change is very straightforward and simple to come up. Regards, Ray >-----Original Message----- >From: Laszlo Ersek [mailto:ler...@redhat.com] >Sent: Wednesday, May 18, 2016 8:12 PM >To: Ni, Ruiyu <ruiyu...@intel.com>; edk2-de...@ml01.01.org >Cc: Anbazhagan Baraneedharan <anbazha...@hp.com>; Zeng, Star ><star.z...@intel.com> >Subject: Re: [Patch 2/2] PcAtChipsetPkg/PcRtc: get century RTC address in >entry point > >On 05/18/16 07:20, Ruiyu Ni wrote: >> When ACPI table is installed before PcRtc driver runs, >> the ACPI table installation callback isn't called which causes the >> century value isn't written to the CMOS. >> The patch calls GetCenturyRtcAddress() in entry point to fix >> the bug. >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Ruiyu Ni <ruiyu...@intel.com> >> Cc: Anbazhagan Baraneedharan <anbazha...@hp.com> >> Cc: Laszlo Ersek <ler...@redhat.com> >> Cc: Star Zeng <star.z...@intel.com> >> --- >> PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.h | 12 +++++++++++- >> PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtcEntry.c | 4 ++-- >> 2 files changed, 13 insertions(+), 3 deletions(-) >> >> diff --git a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.h >b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.h >> index 7fc19f9..ba6092d 100644 >> --- a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.h >> +++ b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.h >> @@ -1,7 +1,7 @@ >> /** @file >> Header file for real time clock driver. >> >> -Copyright (c) 2006 - 2015, Intel Corporation. All rights reserved.<BR> >> +Copyright (c) 2006 - 2016, 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 >> @@ -360,6 +360,16 @@ IsLeapYear ( >> ); >> >> /** >> + Get the century RTC address from the ACPI FADT table. >> + >> + @return The century RTC address or 0 if not found. >> +**/ >> +UINT8 >> +GetCenturyRtcAddress ( >> + VOID >> + ); >> + >> +/** >> Notification function of ACPI Table change. >> >> This is a notification function registered on ACPI Table change event. >> diff --git a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtcEntry.c >b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtcEntry.c >> index 1cfb0cb..a61a35e 100644 >> --- a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtcEntry.c >> +++ b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtcEntry.c >> @@ -1,7 +1,7 @@ >> /** @file >> Provides Set/Get time operations. >> >> -Copyright (c) 2006 - 2015, Intel Corporation. All rights reserved.<BR> >> +Copyright (c) 2006 - 2016, 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 >> @@ -135,7 +135,7 @@ InitializePcRtc ( >> EFI_EVENT Event; >> >> EfiInitializeLock (&mModuleGlobal.RtcLock, TPL_CALLBACK); >> - mModuleGlobal.CenturyRtcAddress = 0; >> + mModuleGlobal.CenturyRtcAddress = GetCenturyRtcAddress (); >> >> Status = PcRtcInit (&mModuleGlobal); >> ASSERT_EFI_ERROR (Status); >> > >Before the patches, the Century value is written to the CMOS in two cases: > >(1) if the ACPI platform driver is dispatched later, then for the first >time when the right ACPI tables are installed (and then immediately) > >(2) in the PcRtcSetTime() function (which is practically the >implementation of the SetTime() runtime service) if it is called after (1) > >After the patches, the Century value is written to the CMOS in three cases: > >(1) same as above >(2) same as above >(3) if the ACPI platform driver is dispatched earlier, then in all of >the PcRtcSetTime() calls >(4) the century value is not written in the driver's entry point even if >the ACPI platform driver was dispatched earlier. > >I don't understand the difference between (1) and (4). Namely, > >- if the presence of the right ACPI tables triggers a century write >without a SetTime() runtime service call in case the RTC driver runs first, >- then why does the presence of the same ACPI tables not trigger the >same century write, similarly without a SetTime() runtime service call, >in case the RTC driver runs second? > >In other words, I don't understand why you simply don't call >PcRtcAcpiTableChangeCallback() from the entry point function. > >Why is it important to split the PcRtcAcpiTableChangeCallback() function >in two, and only cache the century offset in a global variable, without >writing to the register itself? > >I mean, I'm not saying it is wrong. It's probably not wrong, but it is >inconsistent with the other code path. If the ACPI tables per se are not >needed to trigger the century register write, then the RtcWrite() call >should be removed from PcRtcAcpiTableChangeCallback(), for consistency, >and only the one in PcRtcSetTime() should remain. > >Thanks >Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
