On Wed, Aug 08, 2018 at 04:02:56PM +0800, Ming wrote:
diff --git a/Platform/Hisilicon/D06/Library/M41T83RealTimeClockLib/M41T83RealTimeClockLib.c b/Platform/Hisilicon/D06/Library/M41T83RealTimeClockLib/M41T83RealTimeClockLib.c new file mode 100644 index 0000000000..9b1d7c00e8 --- /dev/null +++ b/Platform/Hisilicon/D06/Library/M41T83RealTimeClockLib/M41T83RealTimeClockLib.c @@ -0,0 +1,603 @@ +/** @file
- Copyright (c) 2018, Hisilicon Limited. All rights reserved.<BR>
- Copyright (c) 2018, 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.
+**/
+#include <Uefi.h> +#include <PiDxe.h> +#include <Library/BaseLib.h> +#include <Library/BaseMemoryLib.h> +#include <Library/CpldD06.h> +#include <Library/CpldIoLib.h> +#include <Library/DebugLib.h> +#include <Library/I2CLib.h> +#include <Library/IoLib.h> +#include <Library/MemoryAllocationLib.h> +#include <Library/PcdLib.h> +#include <Library/TimerLib.h> +#include <Library/UefiLib.h> +#include <Library/UefiBootServicesTableLib.h> +#include <Library/UefiRuntimeLib.h> +#include <Library/UefiRuntimeServicesTableLib.h> +#include <Protocol/RealTimeClock.h> +#include "M41T83RealTimeClock.h"
+extern I2C_DEVICE gDS3231RtcDevice;
Hang on - on the branch you pointed to for the code, this line is
extern I2C_DEVICE gRtcDevice;
Please make sure you only ever send out patches that match the branch you point to! I now find myself wondering how many other things differ between the two versions, and will need to apply the same level of review to the next version of this set.
This RTC library is add to D06 first and then we realize the global name is used by some common modules, so we add a patch to modify the name. Is the rename patch should add before this patch?
Yes please.
+EFI_STATUS +SwitchRtcI2cChannelAndLock (
- VOID
- )
+{
- UINT8 Temp;
- UINT8 Count;
- for (Count = 0; Count < 100; Count++) {
- Temp = ReadCpldReg (CPLD_I2C_SWITCH_FLAG); //To get the other side's state is idle first
Many very long lines in this function. Please move comments to the line preceding the statement to solve this. (Throughout.)
- if (0 != (Temp & BIT3)) {
No jeopardy-tests please. Please turn away the right way, throughout.
(VOID) MicroSecondDelay (30000);Why 30000? Do we need a MemoryFence ()?
The delay is need for cpld and I2C.This is a empirical value. MemoryFance is no need.
Please add the comment as to why it is needed.
if (99 == Count) { //Try 100 times, the other side has not released the bus, return preemption failedif (!EfiAtRuntime ()) {DEBUG ((DEBUG_ERROR, "[%a]:[%dL] Clear cpu_i2c_rtc_state 100 times fail !!!\n", __FUNCTION__, __LINE__));And please break this line after \n",
}return EFI_DEVICE_ERROR;}continue; //The other side occupies, continue polling is idle- }
- Temp = ReadCpldReg (CPLD_I2C_SWITCH_FLAG); //Each other free, can be set 1 preemption
- Temp = Temp | CPU_GET_I2C_CONTROL; //bit2 = 1
Comments should describe what operation is being performed, not which numbers are changing and how. What is bit2?
- WriteCpldReg (CPLD_I2C_SWITCH_FLAG, Temp); //Come up directly write CPU occupied RTC I2C State
- (VOID) MicroSecondDelay (2);
Why 2? Do we need a MemoryFence ()?
The delay is need for cpld and I2C.This is a empirical value. MemoryFance is no need.
Please add the comment as to why it is needed.
- Temp = ReadCpldReg (CPLD_I2C_SWITCH_FLAG); //Whether or not to preempt success
- if(CPU_GET_I2C_CONTROL == (Temp & CPU_GET_I2C_CONTROL)) {
break; //Preemption Successful exit loop continue- }
- if (99 == Count) {//Try 100 times, the other side has not released the bus, return preemption failed
if (!EfiAtRuntime ()) {DEBUG((DEBUG_ERROR, "[%a]:[%dL] Clear cpu_i2c_rtc_state fail !!! \n", __FUNCTION__, __LINE__));Again, please break line after format string.
}return EFI_DEVICE_ERROR;- }
- (VOID) MicroSecondDelay (30000); //Delay 30ms
The code already says we're delaying 30ms. The comment should say why. Also, why 30000? Do we need a MemoryFence ()?
The delay is need for cpld and I2C.This is a empirical value. MemoryFance is no need.
Please add the comment as to why it is needed.
- }
- //Polling BMC RTC I2C status
- for (Count = 0; Count < 100; Count++) {
- Temp = ReadCpldReg (CPLD_I2C_SWITCH_FLAG);
- if (0 == (Temp & BIT3)) {
return EFI_SUCCESS;- }
- (VOID) MicroSecondDelay (30000); //Delay 30ms
The code already says we're delaying 30ms. The comment should say why. Also, why 30000? Do we need a MemoryFence ()?
The delay is need for cpld and I2C.This is a empirical value. MemoryFance is no need.
Please add the comment as to why it is needed.
- }
- //If the BMC occupies the RTC I2C Channel, write back the CPU side is idle or the subsequent BMC will not preempt
- Temp = ReadCpldReg (CPLD_I2C_SWITCH_FLAG);
- Temp = Temp & (~CPU_GET_I2C_CONTROL); //BIT2 = 0
What is bit2? Why are we setting it to 0?
- WriteCpldReg (CPLD_I2C_SWITCH_FLAG, Temp);
- return EFI_NOT_READY;
+}
+/**
- Read RTC content through its registers.
- @param Address Address offset of RTC data.
- @param Size Size of RTC data to read.
- @param Data The data of UINT8 type read from RTC.
- @return EFI_STATUS
+**/ +EFI_STATUS +RtcRead (
- IN UINT8 Address,
- IN UINT8 Size,
- OUT UINT8 *Data
- )
+{
- EFI_STATUS Status;
- Status = I2CRead (&gDS3231RtcDevice, Address, Size, Data);
- MicroSecondDelay (1000);
Why 1000? Do we need a MemoryFence ()?
- return Status;
+}
+/**
- Write RTC through its registers.
- @param Address Address offset of RTC data.
- @param Size Size of RTC data to write.
- @param Data The data of UINT8 type write from RTC.
- @return EFI_STATUS
+**/ +EFI_STATUS +RtcWrite (
- IN UINT8 Address,
- IN UINT8 Size,
- UINT8 *Data
- )
+{
- EFI_STATUS Status;
- Status = I2CWrite(&gDS3231RtcDevice, Address, Size, Data);
Space before (.
- MicroSecondDelay (1000);
Why 1000? Do we need a MemoryFence ()?
- return Status;
+}
+VOID +ReleaseOwnershipOfRtc (
- VOID
- )
+{
- UINT8 Temp;
- Temp = ReadCpldReg (CPLD_I2C_SWITCH_FLAG);
- Temp = Temp & ~CPU_GET_I2C_CONTROL;
- WriteCpldReg (CPLD_I2C_SWITCH_FLAG, Temp);
+}
+EFI_STATUS +InitializeM41T83 (
- VOID
- )
+{
- EFI_STATUS Status;
- RTC_M41T83_ALARM1HOUR Alarm1Hour;
- RTC_M41T83_SECOND Second;
- // Acquire RTC Lock to make access to RTC atomic
- if (!EfiAtRuntime ()) {
- EfiAcquireLock (&mRtcLock);
- }
- Status = I2CInit (gDS3231RtcDevice.Socket, gDS3231RtcDevice.Port, Normal);
- MicroSecondDelay (1000);
Why 1000? Do we need a MemoryFence ()?
- if (EFI_ERROR (Status)) {
- if (!EfiAtRuntime ()) {
EfiReleaseLock (&mRtcLock);- }
- return Status;
- }
- Status = SwitchRtcI2cChannelAndLock ();
- if (EFI_ERROR (Status)) {
- DEBUG ((DEBUG_ERROR, "[%a]:[%dL] Status : %r\n", __FUNCTION__, __LINE__, Status));
- if (!EfiAtRuntime ()) {
EfiReleaseLock (&mRtcLock);- }
- return Status;
- }
- MicroSecondDelay(1000);
Why 1000? Do we need a MemoryFence ()?
The delay is need for cpld and I2C.This is a empirical value. MemoryFance is no need.
Please add the comment as to why it is needed.
- // Set ST at Power up to clear Oscillator fail detection(OF)
- Status = RtcRead (M41T83_REGADDR_SECONDS, 1, &Second.u8);
- if (EFI_ERROR (Status)) {
- DEBUG ((DEBUG_ERROR, "[%a]:[%dL] Status : %r\n", __FUNCTION__, __LINE__, Status));
- }
- Second.bits.ST= 1;
- Status = RtcWrite (M41T83_REGADDR_SECONDS, 1, &Second.u8);
- if (EFI_ERROR (Status)) {
- DEBUG ((DEBUG_ERROR, "[%a]:[%dL] Status : %r\n", __FUNCTION__, __LINE__, Status));
- goto Exit;
- }
- Status = RtcRead (M41T83_REGADDR_SECONDS, 1, &Second.u8);
- if (EFI_ERROR (Status)) {
- DEBUG ((DEBUG_ERROR, "[%a]:[%dL] Status : %r\n", __FUNCTION__, __LINE__, Status));
- }
- Second.bits.ST= 0;
- Status = RtcWrite (M41T83_REGADDR_SECONDS, 1, &Second.u8);
- if (EFI_ERROR (Status)) {
- DEBUG ((DEBUG_ERROR, "[%a]:[%dL] Status : %r\n", __FUNCTION__, __LINE__, Status));
- goto Exit;
- }
- // Clear HT bit to enanle write to the RTC registers (addresses 0-7)
- Status = RtcRead (M41T83_REGADDR_ALARM1HOUR, 1, &Alarm1Hour.u8);
- if (EFI_ERROR (Status)) {
- DEBUG ((DEBUG_ERROR, "[%a]:[%dL] Status : %r\n", __FUNCTION__, __LINE__, Status));
- }
- Alarm1Hour.bits.HT = 0;
- Status = RtcWrite (M41T83_REGADDR_ALARM1HOUR, 1, &Alarm1Hour.u8);
- if (EFI_ERROR (Status)) {
- DEBUG ((DEBUG_ERROR, "[%a]:[%dL] Status : %r\n", __FUNCTION__, __LINE__, Status));
- goto Exit;
- }
+Exit:
- // Release RTC Lock.
- ReleaseOwnershipOfRtc ();
- if (!EfiAtRuntime ()) {
- EfiReleaseLock (&mRtcLock);
- }
- return Status;
+}
+BOOLEAN +IsLeapYear (
Use the one from EmbeddedPkg TimeBaseLib instead?
OK, good idea.
- IN EFI_TIME *Time
- )
+{
- if (Time->Year % 4 == 0) {
- if (Time->Year % 100 == 0) {
if (Time->Year % 400 == 0) {return TRUE;} else {return FALSE;}- } else {
return TRUE;- }
- } else {
- return FALSE;
- }
+}
+BOOLEAN +DayValid (
Use IsDayValid from EmbeddedPkg TimeBaseLib instead?
OK, good idea.
- IN EFI_TIME *Time
- )
+{
- INTN DayOfMonth[12] = { 31, 29, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31 };
- if (Time->Day < 1 ||
Time->Day > DayOfMonth[Time->Month - 1] ||(Time->Month == 2 && (!IsLeapYear (Time) && Time->Day > 28))) {- return FALSE;
- }
- return TRUE;
+}
+BOOLEAN TimeValid(
Use IsTimeValid from EmbeddedPkg TimeBaseLib instead?
OK, good idea.
- IN EFI_TIME *Time
- )
+{
- // Check the input parameters are within the range specified by UEFI
- if ((Time->Year < 2000) ||
(Time->Year > 2399) ||(Time->Month < 1 ) ||(Time->Month > 12 ) ||(!DayValid (Time) ) ||(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;
+}
+/**
- 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;
- RTC_M41T83_TIME BcdTime;
- UINT16 CenturyBase = 2000;
- UINTN LineNum = 0;
- if (NULL == Time) {
- return EFI_INVALID_PARAMETER;
- }
- if (!TimeValid (Time)) {
- if (!EfiAtRuntime ()) {
DEBUG ((DEBUG_ERROR, "[%a]:[%dL] Status : %r\n", __FUNCTION__, __LINE__, Status));DEBUG ((DEBUG_ERROR, "Now RTC Time is : %04d-%02d-%02d %02d:%02d:%02d\n",Time->Year, Time->Month, Time->Day, Time->Hour, Time->Minute, Time->Second));- }
- return EFI_INVALID_PARAMETER;
- }
- Status = SwitchRtcI2cChannelAndLock ();
- if (EFI_ERROR (Status)) {
- return Status;
- }
- (VOID) MicroSecondDelay (1000);
Why 1000? Do we need a MemoryFence ()?
- SetMem (&BcdTime, sizeof (RTC_M41T83_TIME), 0);
- // Acquire RTC Lock to make access to RTC atomic
- if (!EfiAtRuntime ()) {
- EfiAcquireLock (&mRtcLock);
- }
- BcdTime.Addr1.bits.Seconds = DecimalToBcd8 (Time->Second);
- BcdTime.Addr2.bits.Minutes = DecimalToBcd8 (Time->Minute);
- BcdTime.Addr3.bits.Hours = DecimalToBcd8 (Time->Hour);
- BcdTime.Addr5.bits.Days = DecimalToBcd8 (Time->Day);
- BcdTime.Addr6.bits.Months = DecimalToBcd8 (Time->Month);
- BcdTime.Addr7.bits.Years = DecimalToBcd8 (Time->Year % 100);
- BcdTime.Addr3.bits.CB = (Time->Year - CenturyBase) / 100 % 10;
- Status = RtcWrite (M41T83_REGADDR_DOTSECONDS, 1, &BcdTime.Addr0.u8);
Just keep the the one space before 1 (throughout).
- if (EFI_ERROR (Status)) {
- LineNum = __LINE__;
- goto Exit;
- }
- Status = RtcWrite (M41T83_REGADDR_SECONDS, 1, &BcdTime.Addr1.u8);
- if (EFI_ERROR (Status)) {
- LineNum = __LINE__;
- goto Exit;
- }
- Status = RtcWrite (M41T83_REGADDR_MINUTES, 1, &BcdTime.Addr2.u8);
- if (EFI_ERROR (Status)) {
- LineNum = __LINE__;
- goto Exit;
- }
- Status = RtcWrite (M41T83_REGADDR_HOURS, 1, &BcdTime.Addr3.u8);
- if (EFI_ERROR (Status)) {
- LineNum = __LINE__;
- goto Exit;
- }
- Status = RtcWrite (M41T83_REGADDR_DAY, 1, &BcdTime.Addr5.u8);
- if (EFI_ERROR (Status)) {
- LineNum = __LINE__;
- goto Exit;
- }
- Status = RtcWrite (M41T83_REGADDR_MONTH, 1, &BcdTime.Addr6.u8);
- if (EFI_ERROR (Status)) {
- LineNum = __LINE__;
- goto Exit;
- }
- Status = RtcWrite (M41T83_REGADDR_YEAR, 1, &BcdTime.Addr7.u8);
- if (EFI_ERROR (Status)) {
- LineNum = __LINE__;
- goto Exit;
- }
+Exit:
- ReleaseOwnershipOfRtc ();
- // Release RTC Lock.
- if (!EfiAtRuntime ()) {
- if (EFI_ERROR (Status)) {
DEBUG ((DEBUG_ERROR, "[%a]:[%dL] Status : %r\n", __FUNCTION__, LineNum, Status));- }
- EfiReleaseLock (&mRtcLock);
- }
- return Status;
+}
+/**
- 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;
- RTC_M41T83_TIME BcdTime;
- UINT16 CenturyBase = 2000;
- UINTN LineNum = 0;
- BOOLEAN IsTimeInvalid = FALSE;
- UINT8 TimeTemp[7] = {0};
- // Ensure Time is a valid pointer
- if (NULL == Time) {
- return EFI_INVALID_PARAMETER;
- }
- Status = SwitchRtcI2cChannelAndLock ();
- if (EFI_ERROR (Status)) {
- return Status;
- }
- MicroSecondDelay(1000);
Why 1000? Do we need a MemoryFence ()?
The delay is need for cpld and I2C.This is a empirical value. MemoryFance is no need.
Please add the comment as to why it is needed. You get the drill :)
/ Leif