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

2017-11-13 Thread Meenakshi Aggarwal


> -Original Message-
> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> Sent: Monday, November 13, 2017 6:13 PM
> To: Meenakshi Aggarwal 
> Cc: Leif Lindholm ; Kinney, Michael D
> ; edk2-devel@lists.01.org; Udit Kumar
> ; Varun Sethi 
> Subject: Re: [PATCH 07/10] Platform/NXP : Add support for DS1307 RTC
> library
> 
> On 7 November 2017 at 14:42, Meenakshi Aggarwal
>  wrote:
> > Real time clock Apis on top of I2C Apis
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Meenakshi Aggarwal 
> > ---
> >  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 000..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=02%7C01%7Cmeenakshi.agg
> >
> +arwal%40nxp.com%7Caa920efb2ccb4db769b908d52a940c38%7C686ea1d3bc
> 2b4c6f
> >
> +a92cd99c5c301635%7C0%7C0%7C636461737713319807=VpAhiubKgpc
> F6DxWQ
> > +Rtbt602oqXGCTsdKdmijMVJt2U%3D=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_ADDR0x00
> > +#define DS1307_MIN_REG_ADDR0x01
> > +#define DS1307_HR_REG_ADDR 0x02
> > +#define DS1307_DAY_REG_ADDR0x03
> > +#define DS1307_DATE_REG_ADDR   0x04
> > +#define DS1307_MON_REG_ADDR0x05
> > +#define DS1307_YR_REG_ADDR 0x06
> > +#define DS1307_CTL_REG_ADDR0x07
> > +
> > +#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_SQWE0x10  /* 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 000..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.
> > + 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=02%7C01%7Cmeenakshi.a
> > +
> ggarwal%40nxp.com%7Caa920efb2ccb4db769b908d52a940c38%7C686ea1d3b
> c2b4
> > +
> c6fa92cd99c5c301635%7C0%7C0%7C636461737713319807=VpAhiubKg
> pcF6
> > + DxWQRtbt602oqXGCTsdKdmijMVJt2U%3D=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 
> > +#include 
> > +#include 
> > +#include 
> 
> In general, please try to use the PI protocols for I2C instead of rolling your
> own. This will 

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

2017-11-13 Thread Ard Biesheuvel
On 7 November 2017 at 14:42, Meenakshi Aggarwal
 wrote:
> Real time clock Apis on top of I2C Apis
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Meenakshi Aggarwal 
> ---
>  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 000..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
> +*  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 __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

> +
> +/*
> + * RTC register addresses
> + */
> +#define DS1307_SEC_REG_ADDR0x00
> +#define DS1307_MIN_REG_ADDR0x01
> +#define DS1307_HR_REG_ADDR 0x02
> +#define DS1307_DAY_REG_ADDR0x03
> +#define DS1307_DATE_REG_ADDR   0x04
> +#define DS1307_MON_REG_ADDR0x05
> +#define DS1307_YR_REG_ADDR 0x06
> +#define DS1307_CTL_REG_ADDR0x07
> +
> +#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_SQWE0x10  /* 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 000..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.
> +  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
> +  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 
> +#include 
> +#include 
> +#include 

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.


> +#include 
> +
> +#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, , 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 (

> +  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, , sizeof(Val));
> +  if (EFI_ERROR(Status))
> +DEBUG((DEBUG_ERROR, "RTC write error at Addr:0x%x\n", RtcRegAddr));

same here


> +}
> +
> +/**
> 

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

2017-11-07 Thread Meenakshi Aggarwal
Real time clock Apis on top of I2C Apis

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Meenakshi Aggarwal 
---
 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 000..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
+*  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 __DS1307RTC_H__
+#define __DS1307RTC_H__
+
+#define Bin(Bcd) ((Bcd) & 0x0f) + ((Bcd) >> 4) * 10
+#define Bcd(Bin) (((Bin / 10) << 4) | (Bin % 10))
+
+/*
+ * RTC register addresses
+ */
+#define DS1307_SEC_REG_ADDR0x00
+#define DS1307_MIN_REG_ADDR0x01
+#define DS1307_HR_REG_ADDR 0x02
+#define DS1307_DAY_REG_ADDR0x03
+#define DS1307_DATE_REG_ADDR   0x04
+#define DS1307_MON_REG_ADDR0x05
+#define DS1307_YR_REG_ADDR 0x06
+#define DS1307_CTL_REG_ADDR0x07
+
+#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_SQWE0x10  /* 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 000..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.
+  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
+  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 
+#include 
+#include 
+#include 
+#include 
+
+#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, , sizeof(Val));
+  if (EFI_ERROR(Status))
+DEBUG((DEBUG_ERROR, "RTC read error at Addr:0x%x\n", RtcRegAddr));
+
+  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, , sizeof(Val));
+  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