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 failed
> >> + if (!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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
