Re: [PATCH] Silicon/NXP/Pcf8563RealTimeClockLib: add rtc device initialization

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

Hi Ard,

Thank you for comment.

> Wouldn't it be better to return EFI_DEVICE_ERROR in this case?

It is first option I come up with to fix this issue.
But edk2/ShellPkg/Library/UefiShellLevel2CommandsLib/TimeDate.c::CheckAndSetTime()
performs GetTime()->update with user specified time->SetTime(),
If GetTime() failes, SetTime() never called and user can not set time.
 # It really depends on the RTC device, we failed to set time in 70% devices.

Another place to perform this dummy time/date setting is inside of
LibRtcInitialize().
Current error occurs in setting time, and I prefer to add this process
in GetTime().

On Mon, 18 Jun 2018 at 22:03, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
> On 18 June 2018 at 14:31,  <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
> >  2) DEBUG build
> >
> > 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.
> >
> > 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 set dummy date/time(01/01/2000 00:00:00) to
> > proceed succeeding SetTime 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.
> >
> > 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                      | 32 ++++++++++++++++------
> >  1 file changed, 23 insertions(+), 9 deletions(-)
> >
> > diff --git a/Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.c b/Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.c
> > index fb58e1feb4..7be0d23eea 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,26 @@ 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) {
>
>
> Wouldn't it be better to return EFI_DEVICE_ERROR in this case?
>
> > +      InitTime.Second  = 0;
> > +      InitTime.Minute  = 0;
> > +      InitTime.Hour    = 0;
> > +      InitTime.Day     = 1;
> > +      InitTime.Month   = 1;
> > +      InitTime.Year    = EPOCH_BASE;
> > +      (VOID)LibSetTime (&InitTime);
> > +      (VOID)CopyMem (Time, &InitTime, sizeof(EFI_TIME));
> > +  } 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
> >