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

[Ming <ming.huang@linaro.org>]

在 8/3/2018 12:56 AM, Leif Lindholm 写道:
> 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?
> 

OK

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

I think it is not necessary and it will be removed in v2.

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

Descriptive ones are better, modify it in v2.

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

Yes

>> +
>> +// 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?
> 

Modify it in v2.

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

This RTC library is add to D06 first and then we realize the global name
is used by some common modules, so we add a patch to modify the name.
Is the rename patch should add before this patch?

>> +
>> +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 ()?

The delay is need for cpld and I2C.This is a empirical value.
MemoryFance is no need.

> 
>> +      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 ()?
> 

The delay is need for cpld and I2C.This is a empirical value.
MemoryFance is no need.

>> +    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 ()?
> 

The delay is need for cpld and I2C.This is a empirical value.
MemoryFance is no need.

>> +  }
>> +
>> +  //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 ()?
> 

The delay is need for cpld and I2C.This is a empirical value.
MemoryFance is no need.

>> +  }
>> +
>> +  //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 ()?
> 

The delay is need for cpld and I2C.This is a empirical value.
MemoryFance is no need.

>> +
>> +  // 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?
> 

OK, good idea.

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

OK, good idea.

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

OK, good idea.

>> +  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 ()?
> 

The delay is need for cpld and I2C.This is a empirical value.
MemoryFance is no need.

Ming

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