> -----Original Message----- > From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org] > Sent: Monday, November 13, 2017 6:13 PM > To: Meenakshi Aggarwal <meenakshi.aggar...@nxp.com> > Cc: Leif Lindholm <leif.lindh...@linaro.org>; Kinney, Michael D > <michael.d.kin...@intel.com>; edk2-devel@lists.01.org; Udit Kumar > <udit.ku...@nxp.com>; Varun Sethi <v.se...@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.aggar...@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.aggar...@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 > > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel