Re: [PATCH edk2-platforms v1 04/38] Platform/Hisilicon/D06: Add M41T83RealTimeClockLib

[Leif Lindholm <leif.lindholm@linaro.org>]

On Tue, Jul 24, 2018 at 03:08:48PM +0800, Ming Huang wrote:
> Add M41T83RealTimeClockLib for RTC.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ming Huang <ming.huang@linaro.org>
> Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
> ---
>  Platform/Hisilicon/D06/D06.dsc                                                   |   1 +
>  Platform/Hisilicon/D06/Library/M41T83RealTimeClockLib/M41T83RealTimeClock.h      | 168 ++++++
>  Platform/Hisilicon/D06/Library/M41T83RealTimeClockLib/M41T83RealTimeClockLib.c   | 603 ++++++++++++++++++++
>  Platform/Hisilicon/D06/Library/M41T83RealTimeClockLib/M41T83RealTimeClockLib.inf |  45 ++

Move this to Silicon/Hisilicon/Library?

>  4 files changed, 817 insertions(+)
> 
> diff --git a/Platform/Hisilicon/D06/D06.dsc b/Platform/Hisilicon/D06/D06.dsc
> index 91470118b2..e05c97e1c6 100644
> --- a/Platform/Hisilicon/D06/D06.dsc
> +++ b/Platform/Hisilicon/D06/D06.dsc
> @@ -66,6 +66,7 @@
>    CpldIoLib|Silicon/Hisilicon/Library/CpldIoLib/CpldIoLib.inf
>  
>    TimeBaseLib|EmbeddedPkg/Library/TimeBaseLib/TimeBaseLib.inf
> +  RealTimeClockLib|Platform/Hisilicon/D06/Library/M41T83RealTimeClockLib/M41T83RealTimeClockLib.inf
>  
>    CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.inf
>    GenericBdsLib|IntelFrameworkModulePkg/Library/GenericBdsLib/GenericBdsLib.inf
> diff --git a/Platform/Hisilicon/D06/Library/M41T83RealTimeClockLib/M41T83RealTimeClock.h b/Platform/Hisilicon/D06/Library/M41T83RealTimeClockLib/M41T83RealTimeClock.h
> new file mode 100644
> index 0000000000..12a67948c3
> --- /dev/null
> +++ b/Platform/Hisilicon/D06/Library/M41T83RealTimeClockLib/M41T83RealTimeClock.h
> @@ -0,0 +1,168 @@
> +/** @file
> +
> +  Copyright (c) 2018, Hisilicon Limited. All rights reserved.<BR>
> +  Copyright (c) 2018, Linaro Limited. All rights reserved.<BR>
> +
> +  This program and the accompanying materials
> +  are licensed and made available under the terms and conditions of the BSD License
> +  which accompanies this distribution.  The full text of the license may be found at
> +  http://opensource.org/licenses/bsd-license.php
> +
> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +
> +**/
> +
> +#ifndef __M41T83_REAL_TIME_CLOCK_H__
> +#define __M41T83_REAL_TIME_CLOCK_H__
> +
> +#define M41T83_REGADDR_DOTSECONDS       0x00
> +#define M41T83_REGADDR_SECONDS          0x01
> +#define M41T83_REGADDR_MINUTES          0x02
> +#define M41T83_REGADDR_HOURS            0x03
> +#define M41T83_REGADDR_WEEK_DAY         0x04
> +#define M41T83_REGADDR_DAY              0x05
> +#define M41T83_REGADDR_MONTH            0x06
> +#define M41T83_REGADDR_YEAR             0x07
> +#define M41T83_REGADDR_ALARM1SEC        0x0E
> +#define M41T83_REGADDR_ALARM1MIN        0x0D
> +#define M41T83_REGADDR_ALARM1HOUR       0x0C
> +#define M41T83_REGADDR_ALARM1DATE       0x0B
> +#define M41T83_REGADDR_ALARM1MONTH      0x0A
> +
> +#define M41T83_REGADDR_TIMERCONTROL     0x11
> +
> +#define M41T83_REGADDR_ALARM2SEC        0x18
> +#define M41T83_REGADDR_ALARM2MIN        0x17
> +#define M41T83_REGADDR_ALARM2HOUR       0x16
> +#define M41T83_REGADDR_ALARM2DATE       0x15
> +#define M41T83_REGADDR_ALARM2MONTH      0x14
> +
> +#pragma pack(1)

It is not obivous to me why this pragma is needed here.
I don't mind the use of pack where it's helpful, but I don't want it
added "just in case".

> +
> +typedef union {
> +  struct {
> +    UINT8 TD0:1;
> +    UINT8 TD1:1;
> +    UINT8 RSV:3;
> +    UINT8 TIE:1;
> +    UINT8 TITP:1;
> +    UINT8 TE:1;
> +  } bits;

Bits.

> +  UINT8 u8;

Uint8.

(Please follow this pattern throughout.)

> +} RTC_M41T83_TIMERCONTROL;
> +
> +typedef union {
> +  struct {
> +    UINT8 MicroSeconds;
> +  } bits;
> +  UINT8 u8;
> +} RTC_M41T83_DOTSECOND;
> +
> +typedef union {
> +  struct{
> +    UINT8 Seconds:7;
> +    UINT8 ST:1;
> +  } bits;
> +  UINT8 u8;
> +} RTC_M41T83_SECOND;
> +
> +typedef union {
> +  struct {
> +    UINT8 Minutes:7;
> +    UINT8 Rsv:1;
> +  } bits;
> +  UINT8 u8;
> +} RTC_M41T83_MINUTE;
> +
> +typedef union {
> +  struct {
> +    UINT8 Hours:6;
> +    UINT8 CB:2;
> +  } bits;
> +  UINT8 u8;
> +} RTC_M41T83_HOUR;
> +
> +typedef union {
> +  struct{
> +    UINT8 Days:3;
> +    UINT8 Rsv:5;
> +  } bits;
> +  UINT8 u8;
> +} RTC_M41T83_WEEK_DAY;
> +
> +typedef union {
> +  struct{
> +    UINT8 Days:6;
> +    UINT8 Rsv:2;
> +  } bits;
> +  UINT8 u8;
> +} RTC_M41T83_MONTH_DAY;
> +
> +typedef union {
> +  struct {
> +    UINT8 Months:5;
> +    UINT8 Rsv:3;
> +  } bits;
> +  UINT8 u8;
> +} RTC_M41T83_MONTH;
> +
> +typedef union {
> +  struct {
> +    UINT8 Years:8;
> +  } bits;
> +  UINT8 u8;
> +} RTC_M41T83_YEAR;
> +
> +typedef union {
> +  struct {
> +    UINT8 Second:7;
> +    UINT8 RPT11:1;
> +  } bits;
> +  UINT8 u8;
> +} RTC_M41T83_ALARM1SEC;
> +
> +typedef union {
> +  struct {
> +    UINT8 Minute:7;
> +    UINT8 RPT12:1;
> +  } bits;
> +  UINT8 u8;
> +} RTC_M41T83_ALARM1MIN;
> +
> +typedef union {
> +  struct {
> +    UINT8 Hour:6;
> +    UINT8 HT:1;
> +    UINT8 RPT13:1;
> +  } bits;
> +  UINT8 u8;
> +} RTC_M41T83_ALARM1HOUR;
> +
> +typedef struct {
> +  RTC_M41T83_DOTSECOND  Addr0;

Why use AddrX names instead of descriptive ones?

> +  RTC_M41T83_SECOND     Addr1;
> +  RTC_M41T83_MINUTE     Addr2;
> +  RTC_M41T83_HOUR       Addr3;
> +  RTC_M41T83_WEEK_DAY   Addr4;
> +  RTC_M41T83_MONTH_DAY  Addr5;
> +  RTC_M41T83_MONTH      Addr6;
> +  RTC_M41T83_YEAR       Addr7;
> +} RTC_M41T83_TIME;
> +
> +#pragma pack()
> +
> +// Define EPOCH (1970-JANUARY-01) in the Julian Date representation
> +#define EPOCH_JULIAN_DATE                               2440588

Can this be imported from EmbeddedPkg TimeBaseLib?

> +
> +// Seconds per unit
> +#define SEC_PER_MIN                                     ((UINTN)    60)
> +#define SEC_PER_HOUR                                    ((UINTN)  3600)
> +#define SEC_PER_DAY                                     ((UINTN) 86400)
> +
> +#define SEC_PER_MONTH                                   ((UINTN)  2,592,000)
> +#define SEC_PER_YEAR                                    ((UINTN) 31,536,000)

And these?

> +
> +EFI_LOCK  mRtcLock;

Please move this to the .c file, and make it STATIC.

> +
> +#endif
> diff --git a/Platform/Hisilicon/D06/Library/M41T83RealTimeClockLib/M41T83RealTimeClockLib.c b/Platform/Hisilicon/D06/Library/M41T83RealTimeClockLib/M41T83RealTimeClockLib.c
> new file mode 100644
> index 0000000000..9b1d7c00e8
> --- /dev/null
> +++ b/Platform/Hisilicon/D06/Library/M41T83RealTimeClockLib/M41T83RealTimeClockLib.c
> @@ -0,0 +1,603 @@
> +/** @file
> +
> +  Copyright (c) 2018, Hisilicon Limited. All rights reserved.<BR>
> +  Copyright (c) 2018, Linaro Limited. All rights reserved.<BR>
> +
> +  This program and the accompanying materials
> +  are licensed and made available under the terms and conditions of the BSD License
> +  which accompanies this distribution.  The full text of the license may be found at
> +  http://opensource.org/licenses/bsd-license.php
> +
> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +
> +**/
> +
> +#include <Uefi.h>
> +#include <PiDxe.h>
> +#include <Library/BaseLib.h>
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/CpldD06.h>
> +#include <Library/CpldIoLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/I2CLib.h>
> +#include <Library/IoLib.h>
> +#include <Library/MemoryAllocationLib.h>
> +#include <Library/PcdLib.h>
> +#include <Library/TimerLib.h>
> +#include <Library/UefiLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +#include <Library/UefiRuntimeLib.h>
> +#include <Library/UefiRuntimeServicesTableLib.h>
> +#include <Protocol/RealTimeClock.h>
> +#include "M41T83RealTimeClock.h"
> +
> +extern I2C_DEVICE gDS3231RtcDevice;

Hang on - on the branch you pointed to for the code, this line is

extern I2C_DEVICE gRtcDevice;

Please make sure you only ever send out patches that match the branch
you point to! I now find myself wondering how many other things differ
between the two versions, and will need to apply the same level of
review to the next version of this set.

> +
> +EFI_STATUS
> +SwitchRtcI2cChannelAndLock (
> +  VOID
> +  )
> +{
> +  UINT8   Temp;
> +  UINT8   Count;
> +
> +  for (Count = 0; Count < 100; Count++) {
> +    Temp = ReadCpldReg (CPLD_I2C_SWITCH_FLAG);  //To get the other side's state is idle first

Many very long lines in this function.
Please move comments to the line preceding the statement to solve
this. (Throughout.)

> +    if (0 != (Temp & BIT3)) {

No jeopardy-tests please.
Please turn away the right way, throughout.

> +      (VOID) MicroSecondDelay (30000);

Why 30000?
Do we need a MemoryFence ()?

> +      if (99 == Count) {                        //Try 100 times, the other side has not released the bus, return preemption failed
> +        if (!EfiAtRuntime ()) {
> +          DEBUG ((DEBUG_ERROR, "[%a]:[%dL] Clear cpu_i2c_rtc_state 100 times fail !!!\n", __FUNCTION__, __LINE__));

And please break this line after \n",

> +        }
> +        return EFI_DEVICE_ERROR;
> +      }
> +      continue;                                 //The other side occupies, continue polling is idle
> +    }
> +
> +    Temp = ReadCpldReg (CPLD_I2C_SWITCH_FLAG);  //Each other free, can be set 1 preemption
> +    Temp = Temp | CPU_GET_I2C_CONTROL;          //bit2 = 1

Comments should describe what operation is being performed, not which
numbers are changing and how. What is bit2?

> +    WriteCpldReg (CPLD_I2C_SWITCH_FLAG, Temp);  //Come up directly write CPU occupied RTC I2C State
> +    (VOID) MicroSecondDelay (2);

Why 2?
Do we need a MemoryFence ()?

> +    Temp = ReadCpldReg (CPLD_I2C_SWITCH_FLAG);  //Whether or not to preempt success
> +    if(CPU_GET_I2C_CONTROL == (Temp & CPU_GET_I2C_CONTROL)) {
> +      break;                                    //Preemption Successful exit loop continue
> +    }
> +    if (99 == Count) {//Try 100 times, the other side has not released the bus, return preemption failed
> +      if (!EfiAtRuntime ()) {
> +        DEBUG((DEBUG_ERROR, "[%a]:[%dL]  Clear cpu_i2c_rtc_state fail !!! \n", __FUNCTION__, __LINE__));

Again, please break line after format string.

> +      }
> +      return EFI_DEVICE_ERROR;
> +    }
> +    (VOID) MicroSecondDelay (30000);            //Delay 30ms

The code already says we're delaying 30ms. The comment should say why.
Also, why 30000?
Do we need a MemoryFence ()?

> +  }
> +
> +  //Polling BMC RTC I2C status
> +  for (Count = 0; Count < 100; Count++) {
> +    Temp = ReadCpldReg (CPLD_I2C_SWITCH_FLAG);
> +    if (0 == (Temp & BIT3)) {
> +      return EFI_SUCCESS;
> +    }
> +    (VOID) MicroSecondDelay (30000);            //Delay 30ms

The code already says we're delaying 30ms. The comment should say why.
Also, why 30000?
Do we need a MemoryFence ()?

> +  }
> +
> +  //If the BMC occupies the RTC I2C Channel, write back the CPU side is idle or the subsequent BMC will not preempt
> +  Temp = ReadCpldReg (CPLD_I2C_SWITCH_FLAG);
> +  Temp = Temp & (~CPU_GET_I2C_CONTROL);         //BIT2 = 0

What is bit2? Why are we setting it to 0?

> +  WriteCpldReg (CPLD_I2C_SWITCH_FLAG, Temp);
> +
> +  return EFI_NOT_READY;
> +}
> +
> +
> +/**
> +  Read RTC content through its registers.
> +
> +  @param  Address   Address offset of RTC data.
> +  @param  Size      Size of RTC data to read.
> +  @param  Data      The data of UINT8 type read from RTC.
> +
> +  @return EFI_STATUS
> +**/
> +EFI_STATUS
> +RtcRead (
> +  IN  UINT8   Address,
> +  IN  UINT8   Size,
> +  OUT UINT8   *Data
> +  )
> +{
> +  EFI_STATUS  Status;
> +
> +  Status = I2CRead (&gDS3231RtcDevice, Address, Size, Data);
> +  MicroSecondDelay (1000);

Why 1000?
Do we need a MemoryFence ()?

> +  return Status;
> +}
> +
> +/**
> +  Write RTC through its registers.
> +
> +  @param  Address   Address offset of RTC data.
> +  @param  Size      Size of RTC data to write.
> +  @param  Data      The data of UINT8 type write from RTC.
> +
> +  @return EFI_STATUS
> +**/
> +EFI_STATUS
> +RtcWrite (
> +  IN  UINT8   Address,
> +  IN  UINT8   Size,
> +  UINT8       *Data
> +  )
> +{
> +  EFI_STATUS  Status;
> +
> +  Status = I2CWrite(&gDS3231RtcDevice, Address, Size, Data);

Space before (.

> +  MicroSecondDelay (1000);

Why 1000?
Do we need a MemoryFence ()?

> +  return Status;
> +}
> +
> +VOID
> +ReleaseOwnershipOfRtc (
> +  VOID
> +  )
> +{
> +  UINT8   Temp;
> +
> +  Temp = ReadCpldReg (CPLD_I2C_SWITCH_FLAG);
> +  Temp = Temp & ~CPU_GET_I2C_CONTROL;
> +  WriteCpldReg (CPLD_I2C_SWITCH_FLAG, Temp);
> +}
> +
> +
> +EFI_STATUS
> +InitializeM41T83 (
> +  VOID
> +  )
> +{
> +  EFI_STATUS                Status;
> +  RTC_M41T83_ALARM1HOUR     Alarm1Hour;
> +  RTC_M41T83_SECOND         Second;
> +  // Acquire RTC Lock to make access to RTC atomic
> +  if (!EfiAtRuntime ()) {
> +    EfiAcquireLock (&mRtcLock);
> +  }
> +
> +  Status = I2CInit (gDS3231RtcDevice.Socket, gDS3231RtcDevice.Port, Normal);
> +  MicroSecondDelay (1000);

Why 1000?
Do we need a MemoryFence ()?

> +  if (EFI_ERROR (Status)) {
> +    if (!EfiAtRuntime ()) {
> +      EfiReleaseLock (&mRtcLock);
> +    }
> +    return Status;
> +  }
> +
> +  Status = SwitchRtcI2cChannelAndLock ();
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "[%a]:[%dL] Status : %r\n", __FUNCTION__, __LINE__, Status));
> +    if (!EfiAtRuntime ()) {
> +      EfiReleaseLock (&mRtcLock);
> +    }
> +    return Status;
> +  }
> +
> +  MicroSecondDelay(1000);

Why 1000?
Do we need a MemoryFence ()?

> +
> +  // Set ST at Power up to clear Oscillator fail detection(OF)
> +  Status = RtcRead (M41T83_REGADDR_SECONDS, 1, &Second.u8);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "[%a]:[%dL] Status : %r\n", __FUNCTION__, __LINE__, Status));
> +  }
> +  Second.bits.ST= 1;
> +  Status = RtcWrite (M41T83_REGADDR_SECONDS, 1, &Second.u8);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "[%a]:[%dL] Status : %r\n", __FUNCTION__, __LINE__, Status));
> +    goto Exit;
> +  }
> +  Status = RtcRead (M41T83_REGADDR_SECONDS, 1, &Second.u8);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "[%a]:[%dL] Status : %r\n", __FUNCTION__, __LINE__, Status));
> +  }
> +  Second.bits.ST= 0;
> +  Status = RtcWrite (M41T83_REGADDR_SECONDS, 1, &Second.u8);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "[%a]:[%dL] Status : %r\n", __FUNCTION__, __LINE__, Status));
> +    goto Exit;
> +  }
> +
> +  // Clear HT bit to enanle write to the RTC registers (addresses 0-7)
> +  Status = RtcRead (M41T83_REGADDR_ALARM1HOUR, 1, &Alarm1Hour.u8);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "[%a]:[%dL] Status : %r\n", __FUNCTION__, __LINE__, Status));
> +  }
> +  Alarm1Hour.bits.HT = 0;
> +  Status = RtcWrite (M41T83_REGADDR_ALARM1HOUR, 1, &Alarm1Hour.u8);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "[%a]:[%dL] Status : %r\n", __FUNCTION__, __LINE__, Status));
> +    goto Exit;
> +  }
> +
> +Exit:
> +  // Release RTC Lock.
> +  ReleaseOwnershipOfRtc ();
> +  if (!EfiAtRuntime ()) {
> +    EfiReleaseLock (&mRtcLock);
> +  }
> +  return Status;
> +}
> +
> +BOOLEAN
> +IsLeapYear (

Use the one from EmbeddedPkg TimeBaseLib instead?

> +  IN EFI_TIME   *Time
> +  )
> +{
> +  if (Time->Year % 4 == 0) {
> +    if (Time->Year % 100 == 0) {
> +      if (Time->Year % 400 == 0) {
> +        return TRUE;
> +      } else {
> +        return FALSE;
> +      }
> +    } else {
> +      return TRUE;
> +    }
> +  } else {
> +    return FALSE;
> +  }
> +}
> +
> +BOOLEAN
> +DayValid (

Use IsDayValid from EmbeddedPkg TimeBaseLib instead?

> +  IN  EFI_TIME  *Time
> +  )
> +{
> +  INTN  DayOfMonth[12] = { 31, 29, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31 };
> +
> +  if (Time->Day < 1 ||
> +      Time->Day > DayOfMonth[Time->Month - 1] ||
> +      (Time->Month == 2 && (!IsLeapYear (Time) && Time->Day > 28))
> +     ) {
> +    return FALSE;
> +  }
> +
> +  return TRUE;
> +}
> +
> +BOOLEAN TimeValid(

Use IsTimeValid from EmbeddedPkg TimeBaseLib instead?

> +  IN  EFI_TIME  *Time
> +  )
> +{
> +  // Check the input parameters are within the range specified by UEFI
> +  if ((Time->Year   < 2000) ||
> +      (Time->Year   > 2399) ||
> +      (Time->Month  < 1   ) ||
> +      (Time->Month  > 12  ) ||
> +      (!DayValid (Time)   ) ||
> +      (Time->Hour   > 23  ) ||
> +      (Time->Minute > 59  ) ||
> +      (Time->Second > 59  ) ||
> +      (Time->Nanosecond > 999999999) ||
> +      (!(Time->TimeZone == EFI_UNSPECIFIED_TIMEZONE || (Time->TimeZone >= -1440 && Time->TimeZone <= 1440))) ||
> +      (Time->Daylight & (~(EFI_TIME_ADJUST_DAYLIGHT | EFI_TIME_IN_DAYLIGHT)))
> +  ) {
> +    return FALSE;
> +  }
> +
> +  return TRUE;
> +}
> +
> +
> +/**
> +  Sets the current local time and date information.
> +
> +  @param  Time                  A pointer to the current time.
> +
> +  @retval EFI_SUCCESS           The operation completed successfully.
> +  @retval EFI_INVALID_PARAMETER A time field is out of range.
> +  @retval EFI_DEVICE_ERROR      The time could not be set due due to hardware error.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +LibSetTime (
> +  IN  EFI_TIME                *Time
> +  )
> +{
> +  EFI_STATUS                  Status = EFI_SUCCESS;
> +  RTC_M41T83_TIME             BcdTime;
> +  UINT16                      CenturyBase = 2000;
> +  UINTN                       LineNum = 0;
> +
> +  if (NULL == Time) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  if (!TimeValid (Time)) {
> +    if (!EfiAtRuntime ()) {
> +      DEBUG ((DEBUG_ERROR, "[%a]:[%dL] Status : %r\n", __FUNCTION__, __LINE__, Status));
> +      DEBUG ((
> +        DEBUG_ERROR, "Now RTC Time is : %04d-%02d-%02d %02d:%02d:%02d\n",
> +        Time->Year, Time->Month, Time->Day, Time->Hour, Time->Minute, Time->Second
> +      ));
> +    }
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  Status = SwitchRtcI2cChannelAndLock ();
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +  (VOID) MicroSecondDelay (1000);

Why 1000?
Do we need a MemoryFence ()?

> +
> +  SetMem (&BcdTime, sizeof (RTC_M41T83_TIME), 0);
> +
> +  // Acquire RTC Lock to make access to RTC atomic
> +  if (!EfiAtRuntime ()) {
> +    EfiAcquireLock (&mRtcLock);
> +  }
> +
> +  BcdTime.Addr1.bits.Seconds  = DecimalToBcd8 (Time->Second);
> +  BcdTime.Addr2.bits.Minutes  = DecimalToBcd8 (Time->Minute);
> +  BcdTime.Addr3.bits.Hours    = DecimalToBcd8 (Time->Hour);
> +  BcdTime.Addr5.bits.Days     = DecimalToBcd8 (Time->Day);
> +  BcdTime.Addr6.bits.Months   = DecimalToBcd8 (Time->Month);
> +  BcdTime.Addr7.bits.Years    = DecimalToBcd8 (Time->Year % 100);
> +  BcdTime.Addr3.bits.CB       = (Time->Year - CenturyBase) / 100 % 10;
> +
> +  Status = RtcWrite (M41T83_REGADDR_DOTSECONDS,  1, &BcdTime.Addr0.u8);

Just keep the the one space before 1 (throughout).

> +  if (EFI_ERROR (Status)) {
> +    LineNum = __LINE__;
> +    goto Exit;
> +  }
> +  Status = RtcWrite (M41T83_REGADDR_SECONDS,     1, &BcdTime.Addr1.u8);
> +  if (EFI_ERROR (Status)) {
> +    LineNum = __LINE__;
> +    goto Exit;
> +  }
> +  Status = RtcWrite (M41T83_REGADDR_MINUTES,     1, &BcdTime.Addr2.u8);
> +  if (EFI_ERROR (Status)) {
> +    LineNum = __LINE__;
> +    goto Exit;
> +  }
> +  Status = RtcWrite (M41T83_REGADDR_HOURS,       1, &BcdTime.Addr3.u8);
> +  if (EFI_ERROR (Status)) {
> +    LineNum = __LINE__;
> +    goto Exit;
> +  }
> +  Status = RtcWrite (M41T83_REGADDR_DAY,         1, &BcdTime.Addr5.u8);
> +  if (EFI_ERROR (Status)) {
> +    LineNum = __LINE__;
> +    goto Exit;
> +  }
> +  Status = RtcWrite (M41T83_REGADDR_MONTH,       1, &BcdTime.Addr6.u8);
> +  if (EFI_ERROR (Status)) {
> +    LineNum = __LINE__;
> +    goto Exit;
> +  }
> +  Status = RtcWrite (M41T83_REGADDR_YEAR,        1, &BcdTime.Addr7.u8);
> +  if (EFI_ERROR (Status)) {
> +    LineNum = __LINE__;
> +    goto Exit;
> +  }
> +
> +Exit:
> +  ReleaseOwnershipOfRtc ();
> +  // Release RTC Lock.
> +  if (!EfiAtRuntime ()) {
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_ERROR, "[%a]:[%dL] Status : %r\n", __FUNCTION__, LineNum, Status));
> +    }
> +    EfiReleaseLock (&mRtcLock);
> +  }
> +  return Status;
> +}
> +
> +
> +/**
> +  Returns the current time and date information, and the time-keeping capabilities
> +  of the hardware platform.
> +
> +  @param  Time                   A pointer to storage to receive a snapshot of the current time.
> +  @param  Capabilities           An optional pointer to a buffer to receive the real time clock
> +                                 device's capabilities.
> +
> +  @retval EFI_SUCCESS            The operation completed successfully.
> +  @retval EFI_INVALID_PARAMETER  Time is NULL.
> +  @retval EFI_DEVICE_ERROR       The time could not be retrieved due to hardware error.
> +  @retval EFI_SECURITY_VIOLATION The time could not be retrieved due to an authentication failure.
> +**/
> +EFI_STATUS
> +EFIAPI
> +LibGetTime (
> +  OUT EFI_TIME                *Time,
> +  OUT EFI_TIME_CAPABILITIES   *Capabilities
> +  )
> +{
> +  EFI_STATUS                  Status = EFI_SUCCESS;
> +  RTC_M41T83_TIME             BcdTime;
> +  UINT16                      CenturyBase = 2000;
> +  UINTN                       LineNum = 0;
> +  BOOLEAN                     IsTimeInvalid = FALSE;
> +  UINT8                       TimeTemp[7] = {0};
> +
> +  // Ensure Time is a valid pointer
> +  if (NULL == Time) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  Status = SwitchRtcI2cChannelAndLock ();
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  MicroSecondDelay(1000);

Why 1000?
Do we need a MemoryFence ()?

/
    Leif

> +
> +  SetMem (&BcdTime, sizeof (RTC_M41T83_TIME), 0);
> +  SetMem (Time    , sizeof (EFI_TIME)       , 0);
> +
> +  // Acquire RTC Lock to make access to RTC atomic
> +  if (!EfiAtRuntime ()) {
> +    EfiAcquireLock (&mRtcLock);
> +  }
> +
> +  Status = RtcRead (M41T83_REGADDR_SECONDS, 7, TimeTemp);
> +  if (EFI_ERROR (Status)) {
> +    LineNum = __LINE__;
> +    goto Exit;
> +  }
> +
> +  BcdTime.Addr1.u8 = TimeTemp[0];  //SECONDS
> +  BcdTime.Addr2.u8 = TimeTemp[1];  //MINUTES
> +  BcdTime.Addr3.u8 = TimeTemp[2];  //HOURS
> +  BcdTime.Addr5.u8 = TimeTemp[4];  //DAY
> +  BcdTime.Addr6.u8 = TimeTemp[5];  //MONTH
> +  BcdTime.Addr7.u8 = TimeTemp[6];  //Year
> +
> +  Time->Year        = BcdToDecimal8 (BcdTime.Addr7.bits.Years);
> +  Time->Year       += CenturyBase + BcdTime.Addr3.bits.CB * 100;
> +  Time->Month       = BcdToDecimal8 (BcdTime.Addr6.bits.Months);
> +  Time->Day         = BcdToDecimal8 (BcdTime.Addr5.bits.Days);
> +  Time->Hour        = BcdToDecimal8 (BcdTime.Addr3.bits.Hours);
> +  Time->Minute      = BcdToDecimal8 (BcdTime.Addr2.bits.Minutes);
> +  Time->Second      = BcdToDecimal8 (BcdTime.Addr1.bits.Seconds);
> +  Time->TimeZone    = EFI_UNSPECIFIED_TIMEZONE;
> +
> +  if (!TimeValid (Time)) {
> +      Status = EFI_DEVICE_ERROR;
> +      LineNum = __LINE__;
> +      IsTimeInvalid = TRUE;
> +      goto Exit;
> +  }
> +
> +Exit:
> +  ReleaseOwnershipOfRtc ();
> +  // Release RTC Lock.
> +  if (!EfiAtRuntime ()) {
> +    if (EFI_ERROR (Status)) {
> +      if (IsTimeInvalid == TRUE) {
> +        DEBUG((DEBUG_ERROR, "%a(%d) Time invalid.\r\n",__FUNCTION__, LineNum));
> +      } else {
> +        DEBUG ((DEBUG_ERROR, "[%a]:[%dL] Status : %r\n", __FUNCTION__, LineNum, Status));
> +      }
> +    }
> +    EfiReleaseLock (&mRtcLock);
> +  }
> +  return Status;
> +}
> +
> +
> +/**
> +  Returns the current wakeup alarm clock setting.
> +
> +  @param  Enabled               Indicates if the alarm is currently enabled or disabled.
> +  @param  Pending               Indicates if the alarm signal is pending and requires acknowledgement.
> +  @param  Time                  The current alarm setting.
> +
> +  @retval EFI_SUCCESS           The alarm settings were returned.
> +  @retval EFI_INVALID_PARAMETER Any parameter is NULL.
> +  @retval EFI_DEVICE_ERROR      The wakeup time could not be retrieved due to a hardware error.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +LibGetWakeupTime (
> +  OUT BOOLEAN     *Enabled,
> +  OUT BOOLEAN     *Pending,
> +  OUT EFI_TIME    *Time
> +  )
> +{
> +  // Not a required feature
> +  return EFI_UNSUPPORTED;
> +}
> +
> +
> +/**
> +  Sets the system wakeup alarm clock time.
> +
> +  @param  Enabled               Enable or disable the wakeup alarm.
> +  @param  Time                  If Enable is TRUE, the time to set the wakeup alarm for.
> +
> +  @retval EFI_SUCCESS           If Enable is TRUE, then the wakeup alarm was enabled. If
> +                                Enable is FALSE, then the wakeup alarm was disabled.
> +  @retval EFI_INVALID_PARAMETER A time field is out of range.
> +  @retval EFI_DEVICE_ERROR      The wakeup time could not be set due to a hardware error.
> +  @retval EFI_UNSUPPORTED       A wakeup timer is not supported on this platform.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +LibSetWakeupTime (
> +  IN BOOLEAN      Enabled,
> +  OUT EFI_TIME    *Time
> +  )
> +{
> +  // Not a required feature
> +  return EFI_UNSUPPORTED;
> +}
> +
> +
> +/**
> +  This is the declaration of an EFI image entry point. This can be the entry point to an application
> +  written to this specification, an EFI boot service driver, or an EFI runtime driver.
> +
> +  @param  ImageHandle           Handle that identifies the loaded image.
> +  @param  SystemTable           System Table for this image.
> +
> +  @retval EFI_SUCCESS           The operation completed successfully.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +LibRtcInitialize (
> +  IN EFI_HANDLE                            ImageHandle,
> +  IN EFI_SYSTEM_TABLE                      *SystemTable
> +  )
> +{
> +  EFI_STATUS    Status = EFI_SUCCESS;
> +  EFI_TIME      EfiTime;
> +
> +  EfiInitializeLock (&mRtcLock, TPL_CALLBACK);
> +
> +  // Setup the setters and getters
> +  gRT->GetTime       = LibGetTime;
> +  gRT->SetTime       = LibSetTime;
> +  gRT->GetWakeupTime = LibGetWakeupTime;
> +  gRT->SetWakeupTime = LibSetWakeupTime;
> +
> +  Status = InitializeM41T83 ();
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "[%a]:[%dL] Status : %r\nRTC M41T83 Init Failed !!!\n",
> +            __FUNCTION__, __LINE__, Status));
> +    /*
> +     * Returning ERROR on failure of RTC initilization will cause the system to hang up.
> +     * So we add some debug message to indecate the RTC initilization failed,
> +     * and continue without returning with error to avoid system hanging up.
> +     *
> +     *return Status;
> +     */
> +  }
> +
> +  LibGetTime (&EfiTime, NULL);
> +  if (!TimeValid (&EfiTime)) {
> +    EfiTime.Year    = 2015;
> +    EfiTime.Month   = 1;
> +    EfiTime.Day     = 1;
> +    EfiTime.Hour    = 0;
> +    EfiTime.Minute  = 0;
> +    EfiTime.Second  = 0;
> +    Status = LibSetTime (&EfiTime);
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_ERROR, "[%a]:[%dL] RTC settime Status : %r\n", __FUNCTION__, __LINE__, Status));
> +    }
> +  }
> +
> +  DEBUG ((
> +    DEBUG_ERROR, "Now RTC Time is : %04d-%02d-%02d %02d:%02d:%02d\n",
> +    EfiTime.Year, EfiTime.Month, EfiTime.Day, EfiTime.Hour, EfiTime.Minute, EfiTime.Second
> +    ));
> +    /*
> +     * Returning ERROR on failure of RTC initilization will cause the system to hang up.
> +     * So we add some debug message to indecate the RTC initilization failed,
> +     * and return success to avoid system hanging up.
> +     */
> +  return EFI_SUCCESS;
> +}
> diff --git a/Platform/Hisilicon/D06/Library/M41T83RealTimeClockLib/M41T83RealTimeClockLib.inf b/Platform/Hisilicon/D06/Library/M41T83RealTimeClockLib/M41T83RealTimeClockLib.inf
> new file mode 100644
> index 0000000000..0d0bb37557
> --- /dev/null
> +++ b/Platform/Hisilicon/D06/Library/M41T83RealTimeClockLib/M41T83RealTimeClockLib.inf
> @@ -0,0 +1,45 @@
> +#/** @file
> +#
> +#  Copyright (c) 2018, Hisilicon Limited. All rights reserved.<BR>
> +#  Copyright (c) 2018, Linaro Limited. All rights reserved.<BR>
> +#
> +#  This program and the accompanying materials
> +#  are licensed and made available under the terms and conditions of the BSD License
> +#  which accompanies this distribution.  The full text of the license may be found at
> +#  http://opensource.org/licenses/bsd-license.php
> +#
> +#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +#
> +#**/
> +
> +[Defines]
> +  INF_VERSION                    = 0x0001001A
> +  BASE_NAME                      = M41T83RealTimeClockLib
> +  FILE_GUID                      = 470DFB96-E205-4515-A75E-2E60F853E79D
> +  MODULE_TYPE                    = BASE
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = RealTimeClockLib
> +
> +[Sources.common]
> +  M41T83RealTimeClockLib.c
> +
> +[Packages]
> +  EmbeddedPkg/EmbeddedPkg.dec
> +  MdePkg/MdePkg.dec
> +  Platform/Hisilicon/D06/D06.dec
> +  Silicon/Hisilicon/HisiPkg.dec
> +
> +[LibraryClasses]
> +  BaseMemoryLib
> +  CpldIoLib
> +  DebugLib
> +  I2CLib
> +  IoLib
> +  PcdLib
> +  TimerLib
> +  UefiLib
> +  UefiRuntimeLib        # Use EFiAtRuntime to check stage
> +
> +[Depex]
> +  gEfiCpuArchProtocolGuid
> -- 
> 2.17.0
>