Re: [PATCH edk2-platforms 19/39] Silicon/NXP:Add support for PCF2129 Real Time Clock Library

[Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>]

> -----Original Message-----
> From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> Sent: Thursday, April 19, 2018 3:42 PM
> To: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>
> Cc: ard.biesheuvel@linaro.org; edk2-devel@lists.01.org; Udit Kumar
> <udit.kumar@nxp.com>; Varun Sethi <V.Sethi@nxp.com>; Vabhav Sharma
> <vabhav.sharma@nxp.com>
> Subject: Re: [PATCH edk2-platforms 19/39] Silicon/NXP:Add support for
> PCF2129 Real Time Clock Library
> 
> On Fri, Feb 16, 2018 at 02:20:15PM +0530, Meenakshi wrote:
> > From: Vabhav <vabhav.sharma@nxp.com>
> >
> > Library to provide functions for NXP pcf2129 real time clock library
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Vabhav <vabhav.sharma@nxp.com>
> > ---
> >  Silicon/NXP/Library/Pcf2129RtcLib/Pcf2129Rtc.h     |  43 +++
> >  Silicon/NXP/Library/Pcf2129RtcLib/Pcf2129RtcLib.c  | 330
> +++++++++++++++++++++
> >  .../NXP/Library/Pcf2129RtcLib/Pcf2129RtcLib.inf    |  47 +++
> >  3 files changed, 420 insertions(+)
> >  create mode 100644 Silicon/NXP/Library/Pcf2129RtcLib/Pcf2129Rtc.h
> >  create mode 100644 Silicon/NXP/Library/Pcf2129RtcLib/Pcf2129RtcLib.c
> >  create mode 100644 Silicon/NXP/Library/Pcf2129RtcLib/Pcf2129RtcLib.inf
> >
> > diff --git a/Silicon/NXP/Library/Pcf2129RtcLib/Pcf2129Rtc.h
> b/Silicon/NXP/Library/Pcf2129RtcLib/Pcf2129Rtc.h
> > new file mode 100644
> > index 0000000..735f697
> > --- /dev/null
> > +++ b/Silicon/NXP/Library/Pcf2129RtcLib/Pcf2129Rtc.h
> > @@ -0,0 +1,43 @@
> > +/** Pcf2129Rtc.h
> > +*
> > +*  Copyright 2017 NXP
> > +*
> > +*  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
> > +*
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fope
> nsource.org%2Flicenses%2Fbsd-
> license.php&data=02%7C01%7Cmeenakshi.aggarwal%40nxp.com%7Cac3a81
> 31870a4f555cbc08d5a5ddfdc8%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0
> %7C0%7C636597295251146941&sdata=WheF7lR6B7iBUJBG2TGfGzJoiVhO%2F
> I1VZywt3l%2FeXug%3D&reserved=0
> > +*
> > +*  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 __PCF2129RTC_H__
> > +#define __PCF2129RTC_H__
> > +
> > +/*
> > + * RTC register addresses
> > + */
> > +#define PCF2129_CTRL1_REG_ADDR      0x00  // Control Register 1
> > +#define PCF2129_CTRL2_REG_ADDR      0x01  // Control Register 2
> > +#define PCF2129_CTRL3_REG_ADDR      0x02  // Control Register 3
> > +#define PCF2129_SEC_REG_ADDR        0x03
> > +#define PCF2129_MIN_REG_ADDR        0x04
> > +#define PCF2129_HR_REG_ADDR         0x05
> > +#define PCF2129_DAY_REG_ADDR        0x06
> > +#define PCF2129_WEEKDAY_REG_ADDR    0x07
> > +#define PCF2129_MON_REG_ADDR        0x08
> > +#define PCF2129_YR_REG_ADDR         0x09
> > +
> > +#define PCF2129_CTRL3_BIT_BLF       BIT2    /* Battery Low Flag*/
> > +
> > +// Define EPOCH (1998-JANUARY-01) in the Julian Date representation
> > +#define EPOCH_JULIAN_DATE           2450815
> > +
> > +typedef struct {
> > +  UINTN                           OperationCount;
> > +  EFI_I2C_OPERATION               SetAddressOp;
> > +  EFI_I2C_OPERATION               GetSetDateTimeOp;
> > +} RTC_I2C_REQUEST;
> > +
> > +#endif // __PCF2129RTC_H__
> > diff --git a/Silicon/NXP/Library/Pcf2129RtcLib/Pcf2129RtcLib.c
> b/Silicon/NXP/Library/Pcf2129RtcLib/Pcf2129RtcLib.c
> > new file mode 100644
> > index 0000000..2e21014
> > --- /dev/null
> > +++ b/Silicon/NXP/Library/Pcf2129RtcLib/Pcf2129RtcLib.c
> > @@ -0,0 +1,330 @@
> > +/** @PCF2129RtcLib.c
> > +  Implement EFI RealTimeClock with runtime services via RTC Lib for
> PCF2129 RTC.
> > +
> > +  Based on RTC implementation available in
> > +  EmbeddedPkg/Library/TemplateRealTimeClockLib/RealTimeClockLib.c
> > +
> > +  Copyright (c) 2008 - 2009, Apple Inc. All rights reserved.<BR>
> > +  Copyright 2017 NXP
> > +
> > +  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
> > +
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fope
> nsource.org%2Flicenses%2Fbsd-
> license.php&data=02%7C01%7Cmeenakshi.aggarwal%40nxp.com%7Cac3a81
> 31870a4f555cbc08d5a5ddfdc8%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0
> %7C0%7C636597295251146941&sdata=WheF7lR6B7iBUJBG2TGfGzJoiVhO%2F
> I1VZywt3l%2FeXug%3D&reserved=0
> > +
> > +  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 <PiDxe.h>
> > +#include <Base.h>
> > +#include <Library/BaseLib.h>
> > +#include <Library/DebugLib.h>
> > +#include <Library/RealTimeClockLib.h>
> > +#include <Library/UefiBootServicesTableLib.h>
> > +#include <Library/UefiLib.h>
> > +#include <Protocol/I2cMaster.h>
> > +
> > +#include "Pcf2129Rtc.h"
> > +
> > +STATIC EFI_I2C_MASTER_PROTOCOL    *mI2cMaster;
> > +
> > +/**
> > +  returns Day of the week [0-6] 0=Sunday
> > +  Don't try to provide a Year that's before 1998, please !
> 
> (You should probably add an assert for that, in that case.)
> 
OK

> > + **/
> > +UINTN
> > +EfiTimeToWday (
> 
> This looks like a completely generic function. Could it be submitted
> to edk2 EmbeddedPkg/Library/TimeBaseLib?
> 
Will send

> > +  IN  EFI_TIME  *Time
> > +  )
> > +{
> > +  UINTN MonthDiff;
> > +  UINTN Year;
> > +  UINTN Month;
> > +  UINTN JulianDate;  // Absolute Julian Date representation of the
> supplied Time
> > +  UINTN EpochDays;   // Number of days elapsed since
> EPOCH_JULIAN_DAY
> > +
> > +  MonthDiff = (14 - Time->Month) / 12 ;
> > +  Year = Time->Year + 4800 - MonthDiff;
> > +  Month = Time->Month + (12*MonthDiff) - 3;
> > +
> > +  JulianDate = Time->Day + ((153*Month + 2)/5) + (365*Year) + (Year/4) -
> (Year/100) + (Year/400) - 32045;
> > +
> > +  ASSERT (JulianDate >= EPOCH_JULIAN_DATE);
> > +  EpochDays = JulianDate - EPOCH_JULIAN_DATE;
> > +
> > +   // 4=1/1/1998 was a Thursday
> 
> Extra space in indentation.
Will correct, thanks for catching.

> 
> > +
> > +  return (EpochDays + 4) % 7;
> > +}
> > +
> > +/**
> > +  Write RTC register.
> > +
> > +  @param  RtcRegAddr       Register offset of RTC to write.
> > +  @param  Val              Value to be written
> > +
> > +**/
> > +
> > +STATIC
> > +VOID
> > +RtcWrite (
> > +  IN  UINT8                RtcRegAddr,
> > +  IN  UINT8                Val
> > +  )
> > +{
> > +  RTC_I2C_REQUEST          Req;
> > +  EFI_STATUS               Status;
> > +
> > +  Req.OperationCount = 2;
> > +
> > +  Req.SetAddressOp.Flags = 0;
> > +  Req.SetAddressOp.LengthInBytes = 0;
> > +  Req.SetAddressOp.Buffer = &RtcRegAddr;
> > +
> > +  Req.GetSetDateTimeOp.Flags = 0;
> > +  Req.GetSetDateTimeOp.LengthInBytes = sizeof (Val);
> > +  Req.GetSetDateTimeOp.Buffer = &Val;
> > +
> > +  Status = mI2cMaster->StartRequest (mI2cMaster, FixedPcdGet8
> (PcdI2cSlaveAddress),
> > +                                     (VOID *)&Req,
> > +                                     NULL,  NULL);
> > +  if (EFI_ERROR (Status)) {
> > +    DEBUG ((DEBUG_ERROR, "RTC write error at Addr:0x%x\n",
> RtcRegAddr));
> > +  }
> > +
> > +}
> > +
> > +/**
> > +  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.
> > +
> > +**/
> > +
> > +EFI_STATUS
> > +EFIAPI
> > +LibGetTime (
> > +  OUT EFI_TIME                *Time,
> > +  OUT  EFI_TIME_CAPABILITIES  *Capabilities
> > +  )
> > +{
> > +  EFI_STATUS      Status;
> > +  UINT8           Buffer[10];
> > +  RTC_I2C_REQUEST Req;
> > +  UINT8           RtcRegAddr;
> > +
> > +  Status = EFI_SUCCESS;
> > +  RtcRegAddr = PCF2129_CTRL1_REG_ADDR;
> > +  Buffer[0] = 0;
> > +
> > +  if (mI2cMaster == NULL) {
> > +    return EFI_DEVICE_ERROR;
> > +  }
> > +
> > +  RtcWrite (PCF2129_CTRL1_REG_ADDR, Buffer[0]);
> > +
> > +  if (Time == NULL) {
> > +    return EFI_INVALID_PARAMETER;
> > +  }
> > +
> > +  Req.OperationCount = 2;
> > +
> > +  Req.SetAddressOp.Flags = 0;
> > +  Req.SetAddressOp.LengthInBytes = 0;
> > +  Req.SetAddressOp.Buffer = &RtcRegAddr;
> > +
> > +  Req.GetSetDateTimeOp.Flags = I2C_FLAG_READ;
> > +  Req.GetSetDateTimeOp.LengthInBytes = sizeof (Buffer);
> > +  Req.GetSetDateTimeOp.Buffer = Buffer;
> > +
> > +  Status = mI2cMaster->StartRequest (mI2cMaster, FixedPcdGet8
> (PcdI2cSlaveAddress),
> > +                                     (VOID *)&Req,
> > +                                     NULL,  NULL);
> > +  if (EFI_ERROR (Status)) {
> > +    DEBUG ((DEBUG_ERROR, "RTC read error at Addr:0x%x\n",
> RtcRegAddr));
> > +  }
> > +
> > +  if (Buffer[PCF2129_CTRL3_REG_ADDR] & PCF2129_CTRL3_BIT_BLF) {
> > +    DEBUG((DEBUG_INFO, "### Warning: RTC battery status low,
> check/replace RTC battery.\n"));
> > +  }
> > +
> > +  Time->Nanosecond = 0;
> > +  Time->Second  = BcdToDecimal8 (Buffer[PCF2129_SEC_REG_ADDR] &
> 0x7F);
> > +  Time->Minute  = BcdToDecimal8 (Buffer[PCF2129_MIN_REG_ADDR] &
> 0x7F);
> > +  Time->Hour = BcdToDecimal8 (Buffer[PCF2129_HR_REG_ADDR] & 0x3F);
> > +  Time->Day = BcdToDecimal8 (Buffer[PCF2129_DAY_REG_ADDR] & 0x3F);
> > +  Time->Month  = BcdToDecimal8 (Buffer[PCF2129_MON_REG_ADDR] &
> 0x1F);
> 
> Ideally, I would like to see #defines for these masks.
> 
Will define macro

> > +  Time->Year = BcdToDecimal8 (Buffer[PCF2129_YR_REG_ADDR]) + (
> BcdToDecimal8 (Buffer[PCF2129_YR_REG_ADDR]) >= 98 ? 1900 : 2000);
> 
> That is a very long line.
> It could be shortened substantially by using a temporary variable for
> BcdToDecimal8 (Buffer[PCF2129_YR_REG_ADDR]).
> 
Sure
> > +
> > +  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
> > +  )
> > +{
> > +  UINT8           Buffer[8];
> > +  UINT8           Index;
> > +  EFI_STATUS      Status;
> > +  RTC_I2C_REQUEST Req;
> > +  UINT8           RtcRegAddr;
> > +
> > +  Index = 0;
> > +  Status = EFI_SUCCESS;
> > +  RtcRegAddr = PCF2129_CTRL1_REG_ADDR;
> > +
> > +  if (mI2cMaster == NULL) {
> > +    return EFI_DEVICE_ERROR;
> > +  }
> > +
> > +  // start register address
> > +  Buffer[Index++] = PCF2129_SEC_REG_ADDR;
> > +
> > +  // hours, minutes and seconds
> > +  Buffer[Index++] = DecimalToBcd8 (Time->Second);
> > +  Buffer[Index++] = DecimalToBcd8 (Time->Minute);
> > +  Buffer[Index++] = DecimalToBcd8 (Time->Hour);
> > +  Buffer[Index++] = DecimalToBcd8 (Time->Day);
> > +  Buffer[Index++] = EfiTimeToWday (Time) & 0x07;
> 
> Why mask at the call site?
> 
In GetTime function, data is been read from RTC device so we need to mask the unnecessary bits 
[as described in RTC registers] while
In SetTime, we are receiving the appropriate buffer so no need to mask the data.
> /
>     Leif
> 
> > +  Buffer[Index++] = DecimalToBcd8 (Time->Month);
> > +  Buffer[Index++] = DecimalToBcd8 (Time->Year % 100);
> > +
> > +  Req.OperationCount = 2;
> > +  Req.SetAddressOp.Flags = 0;
> > +  Req.SetAddressOp.LengthInBytes = 0;
> > +  Req.SetAddressOp.Buffer = &RtcRegAddr;
> > +
> > +  Req.GetSetDateTimeOp.Flags = 0;
> > +  Req.GetSetDateTimeOp.LengthInBytes = sizeof (Buffer);
> > +  Req.GetSetDateTimeOp.Buffer = Buffer;
> > +
> > +  Status = mI2cMaster->StartRequest (mI2cMaster, FixedPcdGet8
> (PcdI2cSlaveAddress),
> > +                                     (VOID *)&Req,
> > +                                     NULL,  NULL);
> > +  if (EFI_ERROR (Status)) {
> > +    DEBUG ((DEBUG_ERROR, "RTC write error at Addr:0x%x\n",
> RtcRegAddr));
> > +    return Status;
> > +  }
> > +
> > +  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.
> > +  @retval EFI_UNSUPPORTED       A wakeup timer is not supported on this
> platform.
> > +
> > +**/
> > +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.
> > +  @retval EFI_DEVICE_ERROR      The operation could not be started.
> > +
> > +**/
> > +EFI_STATUS
> > +EFIAPI
> > +LibRtcInitialize (
> > +  IN EFI_HANDLE                            ImageHandle,
> > +  IN EFI_SYSTEM_TABLE                      *SystemTable
> > +  )
> > +{
> > +
> > +  EFI_STATUS                    Status;
> > +  EFI_I2C_MASTER_PROTOCOL       *I2cMaster;
> > +  UINTN                         BusFrequency;
> > +
> > +  Status = gBS->LocateProtocol (&gEfiI2cMasterProtocolGuid, NULL, (VOID
> **)&I2cMaster);
> > +
> > +  ASSERT_EFI_ERROR (Status);
> > +
> > +  Status = I2cMaster->Reset (I2cMaster);
> > +  if (EFI_ERROR (Status)) {
> > +    DEBUG ((DEBUG_ERROR, "%a: I2CMaster->Reset () failed - %r\n",
> > +      __FUNCTION__, Status));
> > +    return Status;
> > +  }
> > +
> > +  BusFrequency = FixedPcdGet32 (PcdI2cSpeed);
> > +  Status = I2cMaster->SetBusFrequency (I2cMaster, &BusFrequency);
> > +  if (EFI_ERROR (Status)) {
> > +    DEBUG ((DEBUG_ERROR, "%a: I2CMaster->SetBusFrequency () failed -
> %r\n",
> > +      __FUNCTION__, Status));
> > +    return Status;
> > +  }
> > +
> > +  mI2cMaster = I2cMaster;
> > +
> > +  return EFI_SUCCESS;
> > +}
> > diff --git a/Silicon/NXP/Library/Pcf2129RtcLib/Pcf2129RtcLib.inf
> b/Silicon/NXP/Library/Pcf2129RtcLib/Pcf2129RtcLib.inf
> > new file mode 100644
> > index 0000000..873bcea
> > --- /dev/null
> > +++ b/Silicon/NXP/Library/Pcf2129RtcLib/Pcf2129RtcLib.inf
> > @@ -0,0 +1,47 @@
> > +#/** @Pcf2129RtcLib.inf
> > +#
> > +#  Copyright 2017 NXP
> > +#
> > +#  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
> > +#
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fope
> nsource.org%2Flicenses%2Fbsd-
> license.php&data=02%7C01%7Cmeenakshi.aggarwal%40nxp.com%7Cac3a81
> 31870a4f555cbc08d5a5ddfdc8%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0
> %7C0%7C636597295251146941&sdata=WheF7lR6B7iBUJBG2TGfGzJoiVhO%2F
> I1VZywt3l%2FeXug%3D&reserved=0
> > +#  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                      = Pcf2129RtcLib
> > +  FILE_GUID                      = B661E02D-A90B-42AB-A5F9-CF841AAA43D9
> > +  MODULE_TYPE                    = BASE
> > +  VERSION_STRING                 = 1.0
> > +  LIBRARY_CLASS                  = RealTimeClockLib
> > +
> > +
> > +[Sources.common]
> > +  Pcf2129RtcLib.c
> > +
> > +[Packages]
> > +  EmbeddedPkg/EmbeddedPkg.dec
> > +  MdePkg/MdePkg.dec
> > +  Silicon/NXP/NxpQoriqLs.dec
> > +
> > +[LibraryClasses]
> > +  DebugLib
> > +  UefiBootServicesTableLib
> > +  UefiLib
> > +
> > +[Protocols]
> > +  gEfiDriverBindingProtocolGuid        ## CONSUMES
> > +  gEfiI2cMasterProtocolGuid            ## CONSUMES
> > +
> > +[Pcd]
> > +  gNxpQoriqLsTokenSpaceGuid.PcdI2cBus
> > +  gNxpQoriqLsTokenSpaceGuid.PcdI2cSpeed
> > +  gNxpQoriqLsTokenSpaceGuid.PcdI2cSlaveAddress
> > +
> > +[Depex]
> > +  gEfiI2cMasterProtocolGuid
> > --
> > 1.9.1
> >