Re: [PATCH 07/10] Platform/NXP : Add support for DS1307 RTC library

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

> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Monday, November 13, 2017 6:13 PM
> To: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>; Kinney, Michael D
> <michael.d.kinney@intel.com>; edk2-devel@lists.01.org; Udit Kumar
> <udit.kumar@nxp.com>; Varun Sethi <V.Sethi@nxp.com>
> Subject: Re: [PATCH 07/10] Platform/NXP : Add support for DS1307 RTC
> library
> 
> On 7 November 2017 at 14:42, Meenakshi Aggarwal
> <meenakshi.aggarwal@nxp.com> wrote:
> > Real time clock Apis on top of I2C Apis
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>
> > ---
> >  Platform/NXP/Library/Ds1307RtcLib/Ds1307Rtc.h      |  40 ++++
> >  Platform/NXP/Library/Ds1307RtcLib/Ds1307RtcLib.c   | 226
> +++++++++++++++++++++
> >  Platform/NXP/Library/Ds1307RtcLib/Ds1307RtcLib.inf |  40 ++++
> >  3 files changed, 306 insertions(+)
> >  create mode 100644 Platform/NXP/Library/Ds1307RtcLib/Ds1307Rtc.h
> >  create mode 100644 Platform/NXP/Library/Ds1307RtcLib/Ds1307RtcLib.c
> >  create mode 100644 Platform/NXP/Library/Ds1307RtcLib/Ds1307RtcLib.inf
> >
> > diff --git a/Platform/NXP/Library/Ds1307RtcLib/Ds1307Rtc.h
> > b/Platform/NXP/Library/Ds1307RtcLib/Ds1307Rtc.h
> > new file mode 100644
> > index 0000000..952933f
> > --- /dev/null
> > +++ b/Platform/NXP/Library/Ds1307RtcLib/Ds1307Rtc.h
> > @@ -0,0 +1,40 @@
> > +/** Ds1307Rtc.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%2Fop
> e
> > +nsource.org%2Flicenses%2Fbsd-
> license.php&data=02%7C01%7Cmeenakshi.agg
> >
> +arwal%40nxp.com%7Caa920efb2ccb4db769b908d52a940c38%7C686ea1d3bc
> 2b4c6f
> >
> +a92cd99c5c301635%7C0%7C0%7C636461737713319807&sdata=VpAhiubKgpc
> F6DxWQ
> > +Rtbt602oqXGCTsdKdmijMVJt2U%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 __DS1307RTC_H__
> > +#define __DS1307RTC_H__
> > +
> > +#define Bin(Bcd) ((Bcd) & 0x0f) + ((Bcd) >> 4) * 10 #define Bcd(Bin)
> > +(((Bin / 10) << 4) | (Bin % 10))
> 
> Please use BcdToDecimal8 and DecimalToBcd8 from BaseLib instead
> 
OK
> > +
> > +/*
> > + * RTC register addresses
> > + */
> > +#define DS1307_SEC_REG_ADDR        0x00
> > +#define DS1307_MIN_REG_ADDR        0x01
> > +#define DS1307_HR_REG_ADDR         0x02
> > +#define DS1307_DAY_REG_ADDR        0x03
> > +#define DS1307_DATE_REG_ADDR       0x04
> > +#define DS1307_MON_REG_ADDR        0x05
> > +#define DS1307_YR_REG_ADDR         0x06
> > +#define DS1307_CTL_REG_ADDR        0x07
> > +
> > +#define DS1307_SEC_BIT_CH          0x80  /* Clock Halt (in Register 0)   */
> > +
> > +#define DS1307_CTL_BIT_RS0         0x01  /* Rate select 0                */
> > +#define DS1307_CTL_BIT_RS1         0x02  /* Rate select 1                */
> > +#define DS1307_CTL_BIT_SQWE        0x10  /* Square Wave Enable           */
> > +#define DS1307_CTL_BIT_OUT         0x80  /* Output Control               */
> > +
> > +#endif // __DS1307RTC_H__
> > diff --git a/Platform/NXP/Library/Ds1307RtcLib/Ds1307RtcLib.c
> > b/Platform/NXP/Library/Ds1307RtcLib/Ds1307RtcLib.c
> > new file mode 100644
> > index 0000000..5f620a3
> > --- /dev/null
> > +++ b/Platform/NXP/Library/Ds1307RtcLib/Ds1307RtcLib.c
> > @@ -0,0 +1,226 @@
> > +/** Ds1307RtcLib.c
> > +  Implement EFI RealTimeClock with runtime services via RTC Lib for
> DS1307 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%2Fop
> > + ensource.org%2Flicenses%2Fbsd-
> license.php&data=02%7C01%7Cmeenakshi.a
> > +
> ggarwal%40nxp.com%7Caa920efb2ccb4db769b908d52a940c38%7C686ea1d3b
> c2b4
> > +
> c6fa92cd99c5c301635%7C0%7C0%7C636461737713319807&sdata=VpAhiubKg
> pcF6
> > + DxWQRtbt602oqXGCTsdKdmijMVJt2U%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 <Base.h>
> > +#include <Library/BaseLib.h>
> > +#include <Library/DebugLib.h>
> > +#include <Library/I2c.h>
> 
> In general, please try to use the PI protocols for I2C instead of rolling your
> own. This will allow this driver to be shared between platforms.
> 
> OK, will rework
> > +#include <Library/RealTimeClockLib.h>
> > +
> > +#include "Ds1307Rtc.h"
> > +
> > +/**
> > +  Read RTC register.
> > +
> > +  @param  RtcRegAddr       Register offset of RTC to be read.
> > +
> > +  @retval                  Register Value read
> > +
> > +**/
> > +
> > +UINT8
> > +RtcRead (
> > +  IN  UINT8  RtcRegAddr
> > +  )
> > +{
> > +  INT32 Status;
> > +  UINT8 Val;
> > +
> > +  Val = 0;
> > +  Status = I2cDataRead(PcdGet32(PcdRtcI2cBus),
> PcdGet32(PcdDs1307I2cAddress),
> > +                       RtcRegAddr, 0x1, &Val, sizeof(Val));  if
> > + (EFI_ERROR(Status))
> > +    DEBUG((DEBUG_ERROR, "RTC read error at Addr:0x%x\n",
> > + RtcRegAddr));
> > +
> 
> Always use { } after if, and add space before (
> 
sure
> > +  return Val;
> > +}
> > +/**
> > +  Write RTC register.
> > +
> > +  @param  RtcRegAddr       Register offset of RTC to write.
> > +  @param  Val              Value to be written
> > +
> > +**/
> > +
> > +VOID
> > +RtcWrite(
> > +  IN  UINT8  RtcRegAddr,
> > +  IN  UINT8  Val
> > +  )
> > +{
> > +  EFI_STATUS Status;
> > +
> > +  Status = I2cDataWrite(PcdGet32(PcdRtcI2cBus),
> PcdGet32(PcdDs1307I2cAddress),
> > +                        RtcRegAddr, 0x1, &Val, sizeof(Val));  if
> > + (EFI_ERROR(Status))
> > +    DEBUG((DEBUG_ERROR, "RTC write error at Addr:0x%x\n",
> > + RtcRegAddr));
> 
> same here
> 
> 
> > +}
> > +
> > +/**
> > +  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 Second;
> > +  UINT8 Minute;
> > +  UINT8 Hour;
> > +  UINT8 Day;
> > +  UINT8 Month;
> > +  UINT8 Year;
> > +
> > +  Status = EFI_SUCCESS;
> > +
> > +  Second = RtcRead (DS1307_SEC_REG_ADDR);  Minute = RtcRead
> > + (DS1307_MIN_REG_ADDR);  Hour = RtcRead (DS1307_HR_REG_ADDR);
> Day =
> > + RtcRead (DS1307_DATE_REG_ADDR);  Month = RtcRead
> > + (DS1307_MON_REG_ADDR);  Year = RtcRead (DS1307_YR_REG_ADDR);
> > +
> 
> Is it safe to read the RTC using separate I2C transactions? What happens if
> there is a carry between any of those registers?
> 
DS1307 allows reading RTC registers separately.
We will see if any optimization is possible and specification allows.

> > +  if (Second & DS1307_SEC_BIT_CH) {
> > +    DEBUG ((DEBUG_ERROR, "### Warning: RTC oscillator has
> stopped\n"));
> > +    /* clear the CH flag */
> > +    RtcWrite (DS1307_SEC_REG_ADDR,
> > +              RtcRead (DS1307_SEC_REG_ADDR) & ~DS1307_SEC_BIT_CH);
> > +    Status = EFI_DEVICE_ERROR;
> > +  }
> > +
> > +  Time->Second  = Bin (Second & 0x7F);  Time->Minute  = Bin (Minute &
> > + 0x7F);  Time->Hour = Bin (Hour & 0x3F);  Time->Day = Bin (Day &
> > + 0x3F);  Time->Month  = Bin (Month & 0x1F);  Time->Year = Bin (Year)
> > + + ( Bin (Year) >= 70 ? 1900 : 2000);
> > +
> 
> Please use symbolic constants and some comments to clarify how the 2-digit
> year counter maps onto a real year.
> 
OK
> > +  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
> > +  )
> > +{
> > +  if (Time->Year < 1970 || Time->Year > 2069)
> > +    DEBUG((DEBUG_ERROR, "WARNING: Year should be between 1970 and
> > +2069!\n"));
> > +
> 
> Missing { }
> 
OK
> > +  RtcWrite (DS1307_YR_REG_ADDR, Bcd (Time->Year % 100));  RtcWrite
> > + (DS1307_MON_REG_ADDR, Bcd (Time->Month));  RtcWrite
> > + (DS1307_DATE_REG_ADDR, Bcd (Time->Day));  RtcWrite
> > + (DS1307_HR_REG_ADDR, Bcd (Time->Hour));  RtcWrite
> > + (DS1307_MIN_REG_ADDR, Bcd (Time->Minute));  RtcWrite
> > + (DS1307_SEC_REG_ADDR, Bcd (Time->Second));
> > +
> 
> Can you use a single bus transaction here?
> 
We can try.

> > +  return EFI_SUCCESS;
> > +}
> > +
> > +/**
> > +  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
> 
> ... but does the IP support it?
> I don’t think DS1307 supports alarm

> > +  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
> > +  )
> > +{
> > +  return I2cBusInit(PcdGet32(PcdRtcI2cBus), PcdGet32(PcdI2cSpeed)); }
> > diff --git a/Platform/NXP/Library/Ds1307RtcLib/Ds1307RtcLib.inf
> > b/Platform/NXP/Library/Ds1307RtcLib/Ds1307RtcLib.inf
> > new file mode 100644
> > index 0000000..b068b43
> > --- /dev/null
> > +++ b/Platform/NXP/Library/Ds1307RtcLib/Ds1307RtcLib.inf
> > @@ -0,0 +1,40 @@
> > +#  @Ds1307RtcLib.inf
> > +#
> > +#  Lib to provide support for DS1307 Real Time Clock # #  Copyright
> > +(c) 2016, Freescale Semiconductor, Inc. All rights reserved.
> > +#  Copyright (c) 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%2Fop
> e
> > +nsource.org%2Flicenses%2Fbsd-
> license.php&data=02%7C01%7Cmeenakshi.agg
> >
> +arwal%40nxp.com%7Caa920efb2ccb4db769b908d52a940c38%7C686ea1d3bc
> 2b4c6f
> >
> +a92cd99c5c301635%7C0%7C0%7C636461737713319807&sdata=VpAhiubKgpc
> F6DxWQ
> > +Rtbt602oqXGCTsdKdmijMVJt2U%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                    = 0x00010005
> 
> 0x0001001A
>
OK
 
> > +  BASE_NAME                      = Ds1307RtcLib
> > +  FILE_GUID                      = B661E02D-A90B-42AB-A5F9-CF841AAA43D9
> 
> Please don't reuse the GUID of TemplateRealTimeClockLib.inf
> 
OK
> > +  MODULE_TYPE                    = BASE
> > +  VERSION_STRING                 = 1.0
> > +  LIBRARY_CLASS                  = RealTimeClockLib
> > +
> > +[Sources.common]
> > +  Ds1307RtcLib.c
> > +
> > +[Packages]
> > +  EmbeddedPkg/EmbeddedPkg.dec
> > +  MdePkg/MdePkg.dec
> > +  edk2-platforms/Platform/NXP/NxpQoriqLs.dec
> > +
> > +[LibraryClasses]
> > +  DebugLib
> > +  I2cLib
> > +
> > +[Pcd]
> > +  gNxpQoriqLsTokenSpaceGuid.PcdRtcI2cBus
> > +  gNxpQoriqLsTokenSpaceGuid.PcdI2cSpeed
> > +  gNxpQoriqLsTokenSpaceGuid.PcdDs1307I2cAddress
> > --
> > 1.9.1
> >