Re: [PATCH v2] Silicon/NXP/Pcf8563RealTimeClockLib: add Voltage-low handling

[Masahisa Kojima <masahisa.kojima@linaro.org>]

Hi Ard,

Yes, you are correct. I will send update patch.
Thank you.
On Wed, 20 Jun 2018 at 15:21, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
> On 20 June 2018 at 05:02,  <masahisa.kojima@linaro.org> wrote:
> > From: Masahisa Kojima <masahisa.kojima@linaro.org>
> >
> > BcdToDecimal8() in LibGetTime() asserts with the
> > following condition.
> >  1) RTC device has not been initialized yet, RTC device
> >     returns indeterminate value with VL bit(1)
> >  2) DEBUG build
> >
> > UEFI boot fails with assertion when it satisfies above conditions.
> >
> > Aside form boot failure, UEFI shell commands "date/time" expect
> > that getting time from RTC should success when user sets the time.
> > ShellCommandRunTime() performs GetTime()->update time->SetTime(),
> > if the first GetTime() fails, user can not set time.
> > With that, simply returning EFI_DEVICE_ERROR in LibGetTime()
> > is not applicable to VL bit handling.
> >
> > To avoid this situation, even if it only occurs in DEBUG build,
> > RTC driver should check the VL bit in the VL_seconds register.
> > This VL bit is voltage-low detector, it means integrity of the
> > clock information is not guaranteed if it sets to 1. In this
> > case, driver return dummy date/time(01/01/2000 00:00:00) to
> > proceed succeeding process.
> >
> > linux driver also checks this bit when driver gets the time
> > from RTC. If VL bit is 1, linux driver discard the retreived
> > time data.
> >
> > Note that if VL bit is 1, GetTime() returns always
> > 01/01/2000 00:00:00 until user sets date/time.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > Signed-off-by: Yoshitoyo Osaki <osaki.yoshitoyo@socionext.com>
> > ---
> >  .../Pcf8563RealTimeClockLib.c                      | 31 +++++++++++++++-------
> >  1 file changed, 22 insertions(+), 9 deletions(-)
> >
> > diff --git a/Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.c b/Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.c
> > index fb58e1feb4..9f49ae91de 100644
> > --- a/Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.c
> > +++ b/Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.c
> > @@ -19,11 +19,13 @@
> >  #include <Library/UefiBootServicesTableLib.h>
> >  #include <Library/UefiLib.h>
> >  #include <Library/UefiRuntimeLib.h>
> > +#include <Library/BaseMemoryLib.h>
> >  #include <Protocol/I2cMaster.h>
> >
> >  #define SLAVE_ADDRESS             (FixedPcdGet8 (PcdI2cSlaveAddress))
> >  #define PCF8563_DATA_REG_OFFSET   0x2
> >
> > +#define PCF8563_CLOCK_INVALID     0x80
> >  #define PCF8563_SECONDS_MASK      0x7f
> >  #define PCF8563_MINUTES_MASK      0x7f
> >  #define PCF8563_HOURS_MASK        0x3f
> > @@ -95,6 +97,7 @@ LibGetTime (
> >    RTC_DATETIME                DateTime;
> >    EFI_STATUS                  Status;
> >    UINT8                       Reg;
> > +  EFI_TIME                    InitTime;
> >
> >    if (Time == NULL) {
> >      return EFI_INVALID_PARAMETER;
> > @@ -122,15 +125,25 @@ LibGetTime (
> >      return EFI_DEVICE_ERROR;
> >    }
> >
> > -  Time->Second  = BcdToDecimal8 (DateTime.VL_seconds & PCF8563_SECONDS_MASK);
> > -  Time->Minute  = BcdToDecimal8 (DateTime.Minutes & PCF8563_MINUTES_MASK);
> > -  Time->Hour    = BcdToDecimal8 (DateTime.Hours & PCF8563_HOURS_MASK);
> > -  Time->Day     = BcdToDecimal8 (DateTime.Days & PCF8563_DAYS_MASK);
> > -  Time->Month   = BcdToDecimal8 (DateTime.Century_months & PCF8563_MONTHS_MASK);
> > -  Time->Year    = BcdToDecimal8 (DateTime.Years) + EPOCH_BASE;
> > -
> > -  if (DateTime.Century_months & PCF8563_CENTURY_MASK) {
> > -    Time->Year += 100;
> > +  if ((DateTime.VL_seconds & PCF8563_CLOCK_INVALID) != 0) {
> > +      InitTime.Second  = 0;
> > +      InitTime.Minute  = 0;
> > +      InitTime.Hour    = 0;
> > +      InitTime.Day     = 1;
> > +      InitTime.Month   = 1;
> > +      InitTime.Year    = EPOCH_BASE;
> > +      (VOID)CopyMem (Time, &InitTime, sizeof(EFI_TIME));
>
> I don't think we need InitTime or this CopyMem(). instead, we can just
> set the Time-> fields directly, no?
>
> > +  } else {
> > +      Time->Second  = BcdToDecimal8 (DateTime.VL_seconds & PCF8563_SECONDS_MASK);
> > +      Time->Minute  = BcdToDecimal8 (DateTime.Minutes & PCF8563_MINUTES_MASK);
> > +      Time->Hour    = BcdToDecimal8 (DateTime.Hours & PCF8563_HOURS_MASK);
> > +      Time->Day     = BcdToDecimal8 (DateTime.Days & PCF8563_DAYS_MASK);
> > +      Time->Month   = BcdToDecimal8 (DateTime.Century_months & PCF8563_MONTHS_MASK);
> > +      Time->Year    = BcdToDecimal8 (DateTime.Years) + EPOCH_BASE;
> > +
> > +      if (DateTime.Century_months & PCF8563_CENTURY_MASK) {
> > +          Time->Year += 100;
> > +      }
> >    }
> >
> >    if (Capabilities != NULL) {
> > --
> > 2.14.2
> >