Re: [PATCH edk2-platforms v1 04/38] Platform/Hisilicon/D06: Add M41T83RealTimeClockLib

[Leif Lindholm <leif.lindholm@linaro.org>]

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