Re: [PATCH 2/4] Platforms: Add ZX RTC driver for Sanchip SoC

[Jun Nie <jun.nie@linaro.org>]

On 2017年08月10日 22:03, Leif Lindholm wrote:
> On Wed, Aug 09, 2017 at 10:12:37PM +0800, Jun Nie wrote:
>> Runtime service is not supported yet.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Jun Nie <jun.nie@linaro.org>
>> ---
>>   .../Zx6718RealTimeClockLib/Zx296718RealTimeClock.c | 376 +++++++++++++++++++++
>>   .../Zx6718RealTimeClockLib/Zx296718RealTimeClock.h | 102 ++++++
>>   .../Zx296718RealTimeClock.inf                      |  42 +++
>>   3 files changed, 520 insertions(+)
>>   create mode 100644 Silicon/Sanchip/Library/Zx6718RealTimeClockLib/Zx296718RealTimeClock.c
>>   create mode 100644 Silicon/Sanchip/Library/Zx6718RealTimeClockLib/Zx296718RealTimeClock.h
>>   create mode 100644 Silicon/Sanchip/Library/Zx6718RealTimeClockLib/Zx296718RealTimeClock.inf
>>
>> diff --git a/Silicon/Sanchip/Library/Zx6718RealTimeClockLib/Zx296718RealTimeClock.c b/Silicon/Sanchip/Library/Zx6718RealTimeClockLib/Zx296718RealTimeClock.c
>> new file mode 100644
>> index 0000000..af6e5bd
>> --- /dev/null
>> +++ b/Silicon/Sanchip/Library/Zx6718RealTimeClockLib/Zx296718RealTimeClock.c
>> @@ -0,0 +1,376 @@
>> +/** @file
>> +  Implement EFI RealTimeClock runtime services via RTC Lib.
>> +
>> +  Currently this driver does not support runtime virtual calling.
>> +
>> +  Copyright (C) 2017 Sanechips Technology Co., Ltd.
>> +  Copyright (c) 2017, Linaro Limited.
>> +
>> +  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.
>> +
>> +  Based on the files under ArmPlatformPkg/Library/PL031RealTimeClockLib/PL031RealTimeClockLib.inf
>> +
>> +**/
>> +
>> +#include <Uefi.h>
>> +#include <PiDxe.h>
> 
> P before U.
> 
>> +#include <Library/BaseLib.h>
>> +#include <Library/BaseMemoryLib.h>
>> +#include <Library/DebugLib.h>
>> +#include <Library/IoLib.h>
>> +#include <Library/MemoryAllocationLib.h>
>> +#include <Library/PcdLib.h>
>> +#include <Library/TimerLib.h>
>> +#include <Library/UefiBootServicesTableLib.h>
>> +#include <Library/UefiLib.h>
>> +#include <Library/UefiRuntimeServicesTableLib.h>
>> +// Use EfiAtRuntime to check stage
>> +#include <Library/UefiRuntimeLib.h>
> 
> L (UefiRuntimeLib) before S (UefiRuntimeServices...).
> No need for a comment explaining why we include headers.
> 
>> +#include <Protocol/RealTimeClock.h>
>> +#include "Zx296718RealTimeClock.h"
>> +
>> +STATIC UINTN       RtcBase;
> 
> mRtcBase.
> 
>> +STATIC BOOLEAN     RTCInitialized = FALSE;
> 
> mRTCInitialized.
> 
>> +
>> +BOOLEAN
>> +EFIAPI
>> +IsTimeValid(
>> +  IN EFI_TIME *Time
>> +  )
>> +{
>> +  // Check the input parameters are within the range specified by UEFI
>> +  if ((Time->Year  < 2000) ||
>> +     (Time->Year   > 2099) ||
>> +     (Time->Month  < 1   ) ||
>> +     (Time->Month  > 12  ) ||
>> +     (Time->Day    < 1   ) ||
>> +     (Time->Day    > 31  ) ||
>> +     (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;
>> +}
> 
> This appears a direct copy of the version in
> EmbeddedPkg/Library/TimeBaseLib. Can you add that TimeBaseLib library
> dependency to decrease duplication?
> (This may not have existed as a standalone library at the time you
> started this port.)
> 
>> +
>> +VOID
> 
> A lot of the functions in this file could do with a STATIC prefix,
> since they are only used internally.
> 
>> +Wait4Busy (
> 
> Semantically, this name is incorrect.
> The function is waiting _while_ the state is RTC_BUSY, so seems to me
> it should be called WaitBusy.
> 
>> +  VOID
>> +  )
>> +{
>> +  UINT32 Val, Retry = 1000;
>> +  do {
>> +    MicroSecondDelay (200);
> 
> Why 200?

It is just a suggested delay time according to RTC clock frequency. You 
want a RTC_WAIT_DELAY macro here for better understanding, right?
> 
>> +    Val = MmioRead32 (RtcBase + RTCSTS);
> 
> MmioRead32 does not imply any barrier semantics.
> If this component will only ever be supported on ARM platforms, you
> could insert an ArmDataMemoryBarrier (). Otherwise, MemoryFence ().
> 
>> +  } while(Val & RTC_BUSY && Retry--);
> 
> Space before (.
> 
>> +
>> +  if (!Retry)
>> +    DEBUG((DEBUG_ERROR, "%a Rtc busy retry timeout\n", __func__));
> 
> Space after DEBUG.
> 
>> +}
>> +
>> +VOID
>> +RTCWriteReg (
>> +  IN UINT32 Reg,
>> +  IN UINT32 Val
>> +  )
>> +{
>> +  Wait4Busy ();
>> +  MmioWrite32 (RtcBase + RTCCFGID, CONFIG_PARMETER);
>> +  Wait4Busy ();
>> +  MmioWrite32 (RtcBase + Reg, Val);
>> +}
>> +
>> +UINT32
>> +RTCReadReg (
>> +  IN UINT32 Reg
>> +  )
>> +{
>> +  Wait4Busy ();
>> +  return MmioRead32 (RtcBase + Reg);
>> +}
>> +
>> +VOID
>> +InitializeRTC (
>> +  VOID
>> +  )
>> +{
>> +  UINTN Val = (UINTN)FixedPcdGet64 (PcdZxRtcClockFreq);
>> +
>> +  RTCWriteReg (RTCCLKCNT, Val - 1);
>> +  Val = RTCReadReg (RTCPOWERINIT1);
>> +  if (RTC_POWER_INI1_PARA != Val) {
>> +    RTCWriteReg (RTCCTL, 0);
>> +    MicroSecondDelay (INIT_DELAY);
>> +    Val = RTCReadReg (RTCCTL);
>> +    Val |= RTC_CTRL_BIT6_1;
>> +    RTCWriteReg (RTCCTL, Val);
>> +    Val = RTCReadReg (RTCCTL);
>> +    Val &= RTC_CTRL_MODULE24 | RTC_CTRL_STOP;
>> +    RTCWriteReg (RTCCTL, Val);
>> +  }
>> +  Val = RTCReadReg (RTCINT);
>> +  Val &= ~RTC_IT_MASK;
>> +  RTCWriteReg (RTCINT, Val);
>> +  Val = RTCReadReg (RTCSTS);
>> +  Val |= (RTC_POWER_UP | RTC_ALARM | RTC_TIMER);
>> +  RTCWriteReg (RTCSTS, Val);
>> +  //Wait4Busy ();
> 
> No disabled code, please.
> 
>> +  // TODO: write 0x6 to AON int clear
> 
> TODO is fine for early pass of review to get some overall feedback,
> but this needs to be resolved before anything is merged.
> 
>> +  RTCWriteReg (RTCPOWERINIT1, RTC_POWER_INI1_PARA);
>> +  RTCWriteReg (RTCPOWERINIT2, RTC_POWER_INI2_PARA);
>> +  Val = RTC_CTRL_MODULE24 | RTC_CTRL_RUN | RTC_CTRL_CLK_32K_OUTEN
>> +        | RTC_CTRL_BIT6_1;
> 
> Could there be a more human readable alias for RTC_CTRL_BIT6_1
> (describing what setting that bit actually does)?

sure.
> 
>> +  RTCWriteReg (RTCCTL, Val);
>> +
>> +  RTCInitialized = TRUE;
>> +}
>> +
>> +/**
>> +  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;
>> +
>> +  // Ensure Time is a valid pointer
>> +  if (NULL == Time) {
> 
> No jeopardy comparisons please.
> if (Time == NULL)
> 
>> +    return EFI_INVALID_PARAMETER;
>> +  }
>> +
>> +  // Initialize the hardware if not already done
>> +  if (!RTCInitialized) {
>> +    InitializeRTC ();
>> +  }
>> +
>> +#if 0
>> +      /* fake time */
>> +      Time->Year          = 2015;
>> +      Time->Month         = 1;
>> +      Time->Day           = 1;
>> +      Time->Hour          = 0;
>> +      Time->Minute        = 0;
>> +      Time->Second        = 0;
>> +      Time->Nanosecond    = 0;
>> +#endif
> 
> No disabled code blocks, please.
> 
>> +
>> +  RTCWriteReg (RTCGETTIME, 0);
>> +  Time->Year = BCD4_2_BIN (RTCReadReg (RTCYEAR));
>> +  Time->Year += TM_YEAR_START;
>> +
>> +  Time->Month = BCD4_2_BIN (RTCReadReg (RTCMONT));
>> +  Time->Day = BCD4_2_BIN (RTCReadReg (RTCDAY));
>> +  Time->Hour = BCD4_2_BIN (RTCReadReg (RTCHOUR));
>> +  Time->Minute = BCD4_2_BIN (RTCReadReg (RTCMIN));
>> +  Time->Second = BCD4_2_BIN (RTCReadReg (RTCSEC));
>> +  Time->Nanosecond = 0;
>> +  Time->Daylight = 0;
>> +  Time->TimeZone = EFI_UNSPECIFIED_TIMEZONE;
>> +
>> +  if(!IsTimeValid (Time)) {
>> +    Status = EFI_UNSUPPORTED;
>> +  }
>> +
>> +  return Status;
>> +
>> +}
>> +
>> +
>> +/**
>> +  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;
>> +
>> +  // Check the input parameters are within the range specified by UEFI
>> +  if(!IsTimeValid (Time)){
>> +    return EFI_INVALID_PARAMETER;
>> +  }
>> +
>> +  // Initialize the hardware if not already done
>> +  if (!RTCInitialized) {
>> +    InitializeRTC ();
>> +  }
>> +
>> +  RTCWriteReg (RTCSEC, BIN2BCD (Time->Second));
>> +  RTCWriteReg (RTCMIN, BIN2BCD (Time->Minute));
>> +  RTCWriteReg (RTCHOUR, BIN2BCD (Time->Hour));
>> +  RTCWriteReg (RTCDAY, BIN2BCD (Time->Day));
>> +  RTCWriteReg (RTCMONT, BIN2BCD (Time->Month));
>> +  RTCWriteReg (RTCYEAR, BIN2BCD (Time->Year - TM_YEAR_START));
>> +  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_HANDLE    Handle;
>> +
>> +
> 
> Why blank lines?
> 
>> +  EFI_TIME      EfiTime;
>> +
>> +  // Setup the setters and getters
>> +  gRT->GetTime       = LibGetTime;
>> +  gRT->SetTime       = LibSetTime;
>> +  gRT->GetWakeupTime = LibGetWakeupTime;
>> +  gRT->SetWakeupTime = LibSetWakeupTime;
>> +
>> +
> 
> Just one blank line is fine.
> 
>> +  RtcBase = (UINTN)FixedPcdGet64 (PcdZxRtcClockBase);
>> +
>> +  (VOID)gRT->GetTime (&EfiTime, NULL);
>> +  if ((EfiTime.Year < 2015) || (EfiTime.Year > 2099)){
>> +      EfiTime.Year          = 2015;
>> +      EfiTime.Month         = 1;
>> +      EfiTime.Day           = 1;
>> +      EfiTime.Hour          = 0;
>> +      EfiTime.Minute        = 0;
>> +      EfiTime.Second        = 0;
>> +      EfiTime.Nanosecond    = 0;
>> +      Status = gRT->SetTime (&EfiTime);
>> +      if (EFI_ERROR (Status))
>> +      {
>> +        DEBUG ((DEBUG_ERROR, "[%a]:[%dL] Status : %r\n", __FUNCTION__,
>> +               __LINE__, Status));
>> +      }
>> +  }
>> +
>> +  // Install the protocol
>> +  Handle = NULL;
>> +  Status = gBS->InstallMultipleProtocolInterfaces (
>> +                  &Handle,
>> +                  &gEfiRealTimeClockArchProtocolGuid,  NULL,
>> +                  NULL
>> +                 );
>> +
>> +  return Status;
>> +}
>> +
>> +
>> +/**
>> +  Fixup internal data so that EFI can be call in virtual mode.
>> +  Call the passed in Child Notify event and convert any pointers in
>> +  lib to virtual mode.
>> +
>> +  @param[in]    Event   The Event that is being processed
>> +  @param[in]    Context Event Context
>> +**/
>> +VOID
>> +EFIAPI
>> +LibRtcVirtualNotifyEvent (
>> +  IN EFI_EVENT        Event,
>> +  IN VOID             *Context
>> +  )
>> +{
>> +  //
>> +  // Only needed if you are going to support the OS calling RTC functions in virtual mode.
>> +  // You will need to call EfiConvertPointer (). To convert any stored physical addresses
>> +  // to virtual address. After the OS transitions to calling in virtual mode, all future
>> +  // runtime calls will be made in virtual mode.
>> +  //
>> +  return;
>> +}
>> diff --git a/Silicon/Sanchip/Library/Zx6718RealTimeClockLib/Zx296718RealTimeClock.h b/Silicon/Sanchip/Library/Zx6718RealTimeClockLib/Zx296718RealTimeClock.h
>> new file mode 100644
>> index 0000000..3b5a4d4
>> --- /dev/null
>> +++ b/Silicon/Sanchip/Library/Zx6718RealTimeClockLib/Zx296718RealTimeClock.h
>> @@ -0,0 +1,102 @@
>> +/** @file
>> +*
>> +*  Copyright (C) 2017 Sanechips Technology Co., Ltd.
>> +*  Copyright (c) 2017, Linaro Ltd.
>> +*
>> +*  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.
>> +*
>> +*  Based on the files under ArmPlatformPkg/Library/PL031RealTimeClockLib/PL031RealTimeClockLib.inf
>> +**/
>> +
>> +
>> +#ifndef __DS3231_REAL_TIME_CLOCK_H__
>> +#define __DS3231_REAL_TIME_CLOCK_H__
>> +
>> +#define RTC_POWER_INI1_PARA     (0xCDBC)
>> +#define RTC_POWER_INI2_PARA     (0xCFCC)
>> +#define CONFIG_PARMETER         (0xC1CD)
>> +
>> +#define ZX_RTC_CMP_VALUE        (0x3FFF)
>> +#define WAIT_FOR_COUNT          (2000)
>> +#define INIT_DELAY              (100)
>> +
>> +
>> +/* RTC Control register description */
>> +#define RTC_CTRL_STOP                   (~(0x1 << 0))
>> +#define RTC_CTRL_RUN                    (0x1 << 0)
>> +#define RTC_CTRL_ROUND30S               (0x1 << 1)
>> +#define RTC_CTRL_AUTO_COMPENSATION      (0x1 << 2)
>> +#define RTC_CTRL_MODULE12               (0x1 << 3)
>> +#define RTC_CTRL_MODULE24               (~(0x1 << 3))
>> +#define RTC_CTRL_SET_32_COUNTER         (0x1 << 5)
>> +#define RTC_CTRL_SOFT_RESET             (0x1 << 6)
>> +#define RTC_CTRL_CLK_32K_OUTEN          (0x1 << 8)
>> +
>> +#define RTC_CTRL_BIT6_0                 ( ~(0x1 << 6))
>> +#define RTC_CTRL_BIT6_1                 (0x1 << 6)
>> +
>> +
>> +
>> +/* RTC Interrupt register description */
>> +#define RTC_EVERY_MASK          (0x3 << 0)
>> +#define RTC_EVERY_SEC           0x00                    /* second periodic intrrupt */
>> +#define RTC_EVERY_MIN           0x01                    /* minute periodic interrupt */
>> +#define RTC_EVERY_HR            0x02                    /* hour periodic interrupt */
>> +#define RTC_EVERY_DAY           0x03                    /* day periodic interrupt */
>> +#define RTC_IT_TIMER            (0x1 << 2)              /* Enable periodic interrupt */
>> +#define RTC_IT_ALARM            (0x1 << 3)              /* Enable alarm clock interrupt */
>> +#define RTC_IT_MASK             (0x3 << 2)
>> +
>> +/* RTC Status register description */
>> +#define RTC_BUSY                (0x1 << 0)              /* Read-only, indicate refresh*/
>> +#define RTC_RUN                 (0x1 << 1)              /* Read-only, RTC is running */
>> +#define RTC_ALARM               (0x1 << 6)              /* Read/Write, Alarm interrupt has been generated */
>> +#define RTC_TIMER               (0x1 << 7)              /* Read/Write, Timer interrupt has been generated */
>> +#define RTC_POWER_UP            (0x1 << 8)              /* Read/Write, Reset */
>> +
>> +#define TM_YEAR_START               1900
>> +
>> +#define TM_MONTH_OFFSET             1
>> +
>> +#define TM_WDAY_SUNDAY              0
>> +#define ZX_RTC_SUNDAY               7
>> +
>> +#define BCD2BIN(val)    (((val) & 0x0f) + ((val) >> 4) * 10)
>> +#define BIN2BCD(val)    ((((val) / 10) << 4) + (val) % 10)
>> +
>> +#define BCD4_2_BIN(x)   (( (x) & 0x0F) +                    \
>> +                        ((((x) & 0x0F0) >>   4) * 10)  +    \
>> +                        ((((x) & 0xF00) >>   8) * 100) +    \
>> +                        ((((x) & 0xF000) >> 12) * 1000))
>> +
>> +
>> +#define BIN_2_BCD4(x)   (((x % 10) & 0x0F)       |          \
>> +                        (((x /10 ) % 10)  <<  4) |          \
>> +                        (((x /100) % 10)  <<  8) |          \
>> +                        (((x /1000) % 10) << 12))
>> +
>> +/* RTC register offset */
>> +#define RTCVER          0
>> +#define RTCSEC          4
>> +#define RTCMIN          8
>> +#define RTCHOUR         0xc
>> +#define RTCDAY          0x10
>> +#define RTCMONT         0x14
>> +#define RTCYEAR         0x18
>> +#define RTCWEEK         0x1c
>> +#define RTCCTL          0x38
>> +#define RTCSTS          0x3c
>> +#define RTCINT          0x40
>> +#define RTCCFGID        0x48
>> +#define RTCPOWERINIT1   0x4c
>> +#define RTCPOWERINIT2   0x50
>> +#define RTCGETTIME      0x54
>> +#define RTCCLKCNT       0x58
>> +
>> +#endif
>> diff --git a/Silicon/Sanchip/Library/Zx6718RealTimeClockLib/Zx296718RealTimeClock.inf b/Silicon/Sanchip/Library/Zx6718RealTimeClockLib/Zx296718RealTimeClock.inf
>> new file mode 100644
>> index 0000000..0a6852b
>> --- /dev/null
>> +++ b/Silicon/Sanchip/Library/Zx6718RealTimeClockLib/Zx296718RealTimeClock.inf
>> @@ -0,0 +1,42 @@
>> +#/** @file
>> +#
>> +# Copyright (c) 2017, 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                    = 0x00010005
> 
> 0x00010019.
> (Or 1.25.)
> 
>> +  BASE_NAME                      = Zx296718RealTimeClockLib
>> +  FILE_GUID                      = 4e1aaa26-597c-4f01-a8fc-fdf1adc1400f
>> +  MODULE_TYPE                    = BASE
>> +  VERSION_STRING                 = 1.0
>> +  LIBRARY_CLASS                  = RealTimeClockLib
>> +
>> +[Sources.common]
>> +  Zx296718RealTimeClock.c
>> +
>> +[Packages]
>> +  MdePkg/MdePkg.dec
>> +  EmbeddedPkg/EmbeddedPkg.dec
>> +  Silicon/Sanchip/SanchipPkg.dec
> 
> Please sort alphabetically.
> 
>> +
>> +[LibraryClasses]
>> +  IoLib
>> +  UefiLib
>> +  DebugLib
>> +  PcdLib
>> +  TimerLib
>> +# Use EFiAtRuntime to check stage
>> +  UefiRuntimeLib
> 
> Please sort alphabetically >
>> +
>> +[FixedPcd]
>> +  gSanchipTokenSpaceGuid.PcdZxRtcClockBase
>> +  gSanchipTokenSpaceGuid.PcdZxRtcClockFreq
>> -- 
>> 1.9.1
>>

All other coding style and inf version comments will be addressed.