Re: [Patch 1/1] PcAtChipsetPkg: Add PCD for RTC default year

["Ni, Ray" <ray.ni@intel.com>]

I see. Thanks for explanation.

Reviewed-by: Ray Ni <ray.ni@intel.com>

> -----Original Message-----
> From: Kinney, Michael D <michael.d.kinney@intel.com>
> Sent: Monday, March 27, 2023 1:06 AM
> To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: RE: [Patch 1/1] PcAtChipsetPkg: Add PCD for RTC default year
> 
> Hi Ray,
> 
> A second example from the UEFI SCTs (https://github.com/tianocore/edk2-
> test)
> 
> Calls UEFI RT service SetTime() with year 2001.
> 
> https://github.com/tianocore/edk2-
> test/blob/39f27f233aeab16f71fc8f5ed6b287eb9f884ec6/uefi-
> sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/TimeServices/BlackBoxTest/
> TimeServicesBBTestConformance.c#L572
> 
> Mike
> 
> > -----Original Message-----
> > From: Kinney, Michael D <michael.d.kinney@intel.com>
> > Sent: Sunday, March 26, 2023 10:00 AM
> > To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io
> > Cc: Kinney, Michael D <michael.d.kinney@intel.com>
> > Subject: RE: [Patch 1/1] PcAtChipsetPkg: Add PCD for RTC default year
> >
> > Hi Ray,
> >
> > Firmware tests such as FWTS (https://github.com/ColinIanKing/fwts) have
> test
> > Cases for the UEFI time services.  In this case, FWTS has test cases to set
> > the year to 2012 and 2016.  If the minimum year is set to 2023, these test
> > cases fail.
> >
> >
> https://github.com/ColinIanKing/fwts/blob/4e8be7ab0cee56a85845d21ef79
> 6aa618a6a83e3/src/uefi/uefirttime/uefirttime.c#L328
> >
> > I agree that a reasonable approach is to set the default year to the year
> that
> > a system is manufactured.  However, if this is done, the FWTS tests fail
> > because the current edk2 sources use the same setting for both the
> minimum
> > year and the default year.
> >
> > The min/max year should really be based on the hardware capabilities of
> the
> > device that provides time/date information.  The default year should be a
> > platform policy setting.
> >
> > Adding a new PCD for the default year provides a platform setting that that
> > can be set independent of the min/max year hardware capabilities.
> >
> > Mike
> >
> > > -----Original Message-----
> > > From: Ni, Ray <ray.ni@intel.com>
> > > Sent: Sunday, March 26, 2023 2:30 AM
> > > To: Kinney, Michael D <michael.d.kinney@intel.com>;
> devel@edk2.groups.io
> > > Subject: RE: [Patch 1/1] PcAtChipsetPkg: Add PCD for RTC default year
> > >
> > > Mike,
> > > When adding the Min/Max year PCD, the idea was Min year is the default
> year value.
> > > The idea assumed that when a PC is shipped to customers the Min year is
> set to
> > > the current year.
> > >
> > > Can we explain why need the default year PCD?
> > >
> > > Thanks,
> > > Ray
> > >
> > > > -----Original Message-----
> > > > From: Kinney, Michael D <michael.d.kinney@intel.com>
> > > > Sent: Sunday, March 26, 2023 11:23 AM
> > > > To: devel@edk2.groups.io
> > > > Cc: Ni, Ray <ray.ni@intel.com>
> > > > Subject: [Patch 1/1] PcAtChipsetPkg: Add PCD for RTC default year
> > > >
> > > > Add PcdRtcDefaultYear to specify the default year to use when
> > > > the RTC is in an invalid state. Make sure PcdRtcDefaultYear is
> > > > >= PcdMinimalValidYear and <= PcdMaximalValidYear.  Set the
> > > > default value for this PCD to PcdMinimalValidYear to preserve
> > > > the existing behavior. A platform DSC file can override this
> > > > default value setting.
> > > >
> > > > Cc: Ray Ni <ray.ni@intel.com>
> > > > Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
> > > > ---
> > > >  PcAtChipsetPkg/PcAtChipsetPkg.dec                           | 6 ++++++
> > > >  PcAtChipsetPkg/PcAtChipsetPkg.uni                           | 4 ++++
> > > >  PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c          | 4 ++--
> > > >  .../PcatRealTimeClockRuntimeDxe.inf                         | 1 +
> > > >  4 files changed, 13 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/PcAtChipsetPkg/PcAtChipsetPkg.dec
> > > > b/PcAtChipsetPkg/PcAtChipsetPkg.dec
> > > > index a53ca777e85a..0db385fb901f 100644
> > > > --- a/PcAtChipsetPkg/PcAtChipsetPkg.dec
> > > > +++ b/PcAtChipsetPkg/PcAtChipsetPkg.dec
> > > > @@ -76,6 +76,12 @@ [PcdsFixedAtBuild, PcdsDynamic, PcdsDynamicEx,
> > > > PcdsPatchableInModule]
> > > >    # @Expression 0x80000001 |
> > > > gPcAtChipsetPkgTokenSpaceGuid.PcdMaximalValidYear <
> > > > gPcAtChipsetPkgTokenSpaceGuid.PcdMinimalValidYear + 100
> > > >
> > > >
> gPcAtChipsetPkgTokenSpaceGuid.PcdMaximalValidYear|2097|UINT16|0x000
> > > > 0000E
> > > >
> > > > +  ## This PCD specifies the RTC default year when the RTC is in an
> invalid
> > > > state.
> > > > +  # @Prompt Default year in RTC.
> > > > +  # @Expression 0x80000001 |
> > > > gPcAtChipsetPkgTokenSpaceGuid.PcdRtcDefaultYear >=
> > > > gPcAtChipsetPkgTokenSpaceGuid.PcdMinimalValidYear
> > > > +  # @Expression 0x80000001 |
> > > > gPcAtChipsetPkgTokenSpaceGuid.PcdRtcDefaultYear <=
> > > > gPcAtChipsetPkgTokenSpaceGuid.PcdMaximalValidYear
> > > > +
> > > >
> gPcAtChipsetPkgTokenSpaceGuid.PcdRtcDefaultYear|gPcAtChipsetPkgToke
> > > > nSpaceGuid.PcdMinimalValidYear|UINT16|0x0000000F
> > > > +
> > > >    ## Specifies RTC Index Register address in MMIO space.
> > > >    # @Prompt RTC Index Register address
> > > >
> > > >
> gPcAtChipsetPkgTokenSpaceGuid.PcdRtcIndexRegister64|0x0|UINT64|0x00
> > > > 000022
> > > > diff --git a/PcAtChipsetPkg/PcAtChipsetPkg.uni
> > > > b/PcAtChipsetPkg/PcAtChipsetPkg.uni
> > > > index d290dcf1650f..8eb7970c58aa 100644
> > > > --- a/PcAtChipsetPkg/PcAtChipsetPkg.uni
> > > > +++ b/PcAtChipsetPkg/PcAtChipsetPkg.uni
> > > > @@ -79,6 +79,10 @@
> > > >
> > > >  #string
> STR_gPcAtChipsetPkgTokenSpaceGuid_PcdMaximalValidYear_HELP
> > > > #language en-US "This PCD specifies the maximal valid year in RTC."
> > > >
> > > > +#string
> > > > STR_gPcAtChipsetPkgTokenSpaceGuid_PcdRtcDefaultYear_PROMPT
> > > > #language en-US "Default year in RTC"
> > > > +
> > > > +#string
> STR_gPcAtChipsetPkgTokenSpaceGuid_PcdRtcDefaultYear_HELP
> > > > #language en-US "This PCD specifies the RTC default year when the RTC
> is in
> > > > an invalid state."
> > > > +
> > > >  #string
> > > >
> STR_gPcAtChipsetPkgTokenSpaceGuid_PcdAcpiIoPortBaseAddressMask_PR
> > > > OMPT   #language en-US  "ACPI IO Port Base Address Mask"
> > > >
> > > >  #string
> > > >
> STR_gPcAtChipsetPkgTokenSpaceGuid_PcdAcpiIoPortBaseAddressMask_HE
> > > > LP     #language en-US  "Defines the bit mask to retrieve ACPI IO Port
> Base
> > > > Address."
> > > > diff --git a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c
> > > > b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c
> > > > index 9242a2e82600..b059e92f02dc 100644
> > > > --- a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c
> > > > +++ b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c
> > > > @@ -317,7 +317,7 @@ PcRtcInit (
> > > >      Time.Hour       = RTC_INIT_HOUR;
> > > >      Time.Day        = RTC_INIT_DAY;
> > > >      Time.Month      = RTC_INIT_MONTH;
> > > > -    Time.Year       = PcdGet16 (PcdMinimalValidYear);
> > > > +    Time.Year       = PcdGet16 (PcdRtcDefaultYear);
> > > >      Time.Nanosecond = 0;
> > > >      Time.TimeZone   = EFI_UNSPECIFIED_TIMEZONE;
> > > >      Time.Daylight   = 0;
> > > > @@ -357,7 +357,7 @@ PcRtcInit (
> > > >    Time.Hour       = RTC_INIT_HOUR;
> > > >    Time.Day        = RTC_INIT_DAY;
> > > >    Time.Month      = RTC_INIT_MONTH;
> > > > -  Time.Year       = PcdGet16 (PcdMinimalValidYear);
> > > > +  Time.Year       = PcdGet16 (PcdRtcDefaultYear);
> > > >    Time.Nanosecond = 0;
> > > >    Time.TimeZone   = Global->SavedTimeZone;
> > > >    Time.Daylight   = Global->Daylight;
> > > > diff --git
> > > >
> a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcatRealTimeClockRuntim
> > > > eDxe.inf
> > > >
> b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcatRealTimeClockRuntim
> > > > eDxe.inf
> > > > index 0d8eca28b659..c344b059878c 100644
> > > > ---
> > > >
> a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcatRealTimeClockRuntim
> > > > eDxe.inf
> > > > +++
> > > >
> b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcatRealTimeClockRuntim
> > > > eDxe.inf
> > > > @@ -76,6 +76,7 @@ [Pcd]
> > > >    gPcAtChipsetPkgTokenSpaceGuid.PcdRealTimeClockUpdateTimeout
> ##
> > > > CONSUMES
> > > >    gPcAtChipsetPkgTokenSpaceGuid.PcdMinimalValidYear             ##
> > > > CONSUMES
> > > >    gPcAtChipsetPkgTokenSpaceGuid.PcdMaximalValidYear             ##
> > > > CONSUMES
> > > > +  gPcAtChipsetPkgTokenSpaceGuid.PcdRtcDefaultYear               ##
> > > > CONSUMES
> > > >    gPcAtChipsetPkgTokenSpaceGuid.PcdRtcIndexRegister             ##
> > > > CONSUMES
> > > >    gPcAtChipsetPkgTokenSpaceGuid.PcdRtcTargetRegister            ##
> > > > CONSUMES
> > > >    gPcAtChipsetPkgTokenSpaceGuid.PcdRtcIndexRegister64           ##
> > > > CONSUMES
> > > > --
> > > > 2.39.1.windows.1