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

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

On 18 June 2018 at 16:29, Masahisa Kojima <masahisa.kojima@linaro.org> wrote:
> 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().
>

I think we should fix the shell command instead. Setting the time
should be possible even if getting the time file, precisely for
situations like this one.

> 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
>> >