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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

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
>