Re: [RFC edk2-platforms v1 1/2] Hisilicon/Library: Move two functions to RtcHelperLib

["Leif Lindholm" <leif@nuviainc.com>]

On Mon, May 18, 2020 at 21:37:23 +0800, Ming Huang wrote:
> The functions of acquiring ownership of RTC will be used for other
> RTC library, so move them to RtcHelperLib.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ming Huang <huangming23@huawei.com>
> ---
>  Platform/Hisilicon/D06/D06.dsc                                              |  1 +
>  Silicon/Hisilicon/Include/Library/RtcHelperLib.h                            | 28 ++++++
>  Silicon/Hisilicon/Library/M41T83RealTimeClockLib/M41T83RealTimeClockLib.c   | 80 +----------------
>  Silicon/Hisilicon/Library/M41T83RealTimeClockLib/M41T83RealTimeClockLib.inf |  1 +
>  Silicon/Hisilicon/Library/RtcHelperLib/RtcHelperLib.c                       | 94 ++++++++++++++++++++
>  Silicon/Hisilicon/Library/RtcHelperLib/RtcHelperLib.inf                     | 32 +++++++
>  6 files changed, 157 insertions(+), 79 deletions(-)
> 
> diff --git a/Platform/Hisilicon/D06/D06.dsc b/Platform/Hisilicon/D06/D06.dsc
> index 000a4ee..eb20224 100644
> --- a/Platform/Hisilicon/D06/D06.dsc
> +++ b/Platform/Hisilicon/D06/D06.dsc
> @@ -54,6 +54,7 @@
>    CpldIoLib|Silicon/Hisilicon/Library/CpldIoLib/CpldIoLib.inf
>  
>    TimeBaseLib|EmbeddedPkg/Library/TimeBaseLib/TimeBaseLib.inf
> +  RtcHelperLib|Silicon/Hisilicon/Library/RtcHelperLib/RtcHelperLib.inf
>    RealTimeClockLib|Silicon/Hisilicon/Library/M41T83RealTimeClockLib/M41T83RealTimeClockLib.inf
>    OemMiscLib|Platform/Hisilicon/D06/Library/OemMiscLibD06/OemMiscLibD06.inf
>    OemAddressMapLib|Platform/Hisilicon/D06/Library/OemAddressMapD06/OemAddressMapD06.inf
> diff --git a/Silicon/Hisilicon/Include/Library/RtcHelperLib.h b/Silicon/Hisilicon/Include/Library/RtcHelperLib.h
> new file mode 100644
> index 0000000..d6c1d39
> --- /dev/null
> +++ b/Silicon/Hisilicon/Include/Library/RtcHelperLib.h
> @@ -0,0 +1,28 @@
> +/** @file
> +
> +  Copyright (c) 2018, Hisilicon Limited. All rights reserved.<BR>
> +  Copyright (c) 2018, Linaro Limited. All rights reserved.<BR>
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#ifndef __RTC_HELPER_LIB_H__
> +#define __RTC_HELPER_LIB_H__

Please drop leading __ from macros. Those are reserved for toolchain
use.

> +
> +// The delay is need for cpld and I2C. This is a empirical value. MemoryFence is no need.
> +#define RTC_DELAY_30_MS            30000
> +// The delay is need for cpld and I2C. This is a empirical value. MemoryFence is no need.
> +#define RTC_DELAY_2_MACROSECOND    2

I guess  MACROSECOND is a
typo for MICROSECOND?

But we are now duplicating these macros between M41T83RealTimeClock.h
and RtcHelperLib.h. And M41T83RealTimeClock.h still holds a bunch of
other _MACROSECOND #defines.

Could we move all of the macro definitions to this file instead?
Could we also fix the spelling, in a patch preceding this one?

/
    Leif

> +
> +EFI_STATUS
> +SwitchRtcI2cChannelAndLock (
> +  VOID
> +  );
> +
> +VOID
> +ReleaseOwnershipOfRtc (
> +  VOID
> +  );
> +
> +#endif
> diff --git a/Silicon/Hisilicon/Library/M41T83RealTimeClockLib/M41T83RealTimeClockLib.c b/Silicon/Hisilicon/Library/M41T83RealTimeClockLib/M41T83RealTimeClockLib.c
> index 5131ce7..e3fecac 100644
> --- a/Silicon/Hisilicon/Library/M41T83RealTimeClockLib/M41T83RealTimeClockLib.c
> +++ b/Silicon/Hisilicon/Library/M41T83RealTimeClockLib/M41T83RealTimeClockLib.c
> @@ -11,10 +11,9 @@
>  #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/RtcHelperLib.h>
>  #include <Library/TimeBaseLib.h>
>  #include <Library/TimerLib.h>
>  #include <Library/UefiLib.h>
> @@ -26,70 +25,6 @@ extern I2C_DEVICE gRtcDevice;
>  
>  STATIC EFI_LOCK  mRtcLock;
>  
> -EFI_STATUS
> -SwitchRtcI2cChannelAndLock (
> -  VOID
> -  )
> -{
> -  UINT8   Temp;
> -  UINT8   Count;
> -
> -  for (Count = 0; Count < 100; Count++) {
> -    // To get the other side's state is idle first
> -    Temp = ReadCpldReg (CPLD_I2C_SWITCH_FLAG);
> -    if ((Temp & BIT3) != 0) {
> -      (VOID) MicroSecondDelay (RTC_DELAY_30_MS);
> -      // Try 100 times, if BMC has not released the bus, return preemption failed
> -      if (Count == 99) {
> -        if (!EfiAtRuntime ()) {
> -          DEBUG ((DEBUG_ERROR, "[%a]:[%dL] Clear cpu_i2c_rtc_state 100 times fail!\n",
> -            __FUNCTION__, __LINE__));
> -        }
> -        return EFI_DEVICE_ERROR;
> -      }
> -      continue;
> -    }
> -
> -    // if BMC free the bus, can be set 1 preemption
> -    Temp = ReadCpldReg (CPLD_I2C_SWITCH_FLAG);
> -    Temp = Temp | CPU_GET_I2C_CONTROL;
> -    // CPU occupied RTC I2C State
> -    WriteCpldReg (CPLD_I2C_SWITCH_FLAG, Temp);
> -    (VOID) MicroSecondDelay (RTC_DELAY_2_MACROSECOND);
> -    Temp = ReadCpldReg (CPLD_I2C_SWITCH_FLAG);
> -    // Is preempt success
> -    if(CPU_GET_I2C_CONTROL == (Temp & CPU_GET_I2C_CONTROL)) {
> -      break;
> -    }
> -    if (Count == 99) {
> -      if (!EfiAtRuntime ()) {
> -        DEBUG((DEBUG_ERROR, "[%a]:[%dL]  Clear cpu_i2c_rtc_state fail !!! \n",
> -          __FUNCTION__, __LINE__));
> -      }
> -      return EFI_DEVICE_ERROR;
> -    }
> -    (VOID) MicroSecondDelay (RTC_DELAY_30_MS);
> -  }
> -
> -  //Polling BMC RTC I2C status
> -  for (Count = 0; Count < 100; Count++) {
> -    Temp = ReadCpldReg (CPLD_I2C_SWITCH_FLAG);
> -    if ((Temp & BIT3) == 0) {
> -      return EFI_SUCCESS;
> -    }
> -    (VOID) MicroSecondDelay (RTC_DELAY_30_MS);
> -  }
> -
> -  //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);
> -  WriteCpldReg (CPLD_I2C_SWITCH_FLAG, Temp);
> -
> -  return EFI_NOT_READY;
> -}
> -
> -
>  /**
>    Read RTC content through its registers.
>  
> @@ -136,19 +71,6 @@ RtcWrite (
>    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
> diff --git a/Silicon/Hisilicon/Library/M41T83RealTimeClockLib/M41T83RealTimeClockLib.inf b/Silicon/Hisilicon/Library/M41T83RealTimeClockLib/M41T83RealTimeClockLib.inf
> index 9344c6d..5970c0e 100644
> --- a/Silicon/Hisilicon/Library/M41T83RealTimeClockLib/M41T83RealTimeClockLib.inf
> +++ b/Silicon/Hisilicon/Library/M41T83RealTimeClockLib/M41T83RealTimeClockLib.inf
> @@ -31,6 +31,7 @@
>    I2CLib
>    IoLib
>    PcdLib
> +  RtcHelperLib
>    TimeBaseLib
>    TimerLib
>    UefiLib
> diff --git a/Silicon/Hisilicon/Library/RtcHelperLib/RtcHelperLib.c b/Silicon/Hisilicon/Library/RtcHelperLib/RtcHelperLib.c
> new file mode 100644
> index 0000000..bc8e6c5
> --- /dev/null
> +++ b/Silicon/Hisilicon/Library/RtcHelperLib/RtcHelperLib.c
> @@ -0,0 +1,94 @@
> +/** @file
> +
> +  Copyright (c) 2020, Hisilicon Limited. All rights reserved.<BR>
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include <Uefi.h>
> +#include <PiDxe.h>
> +#include <Library/BaseLib.h>
> +#include <Library/CpldD06.h>
> +#include <Library/CpldIoLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/RtcHelperLib.h>
> +#include <Library/TimeBaseLib.h>
> +#include <Library/TimerLib.h>
> +#include <Library/UefiRuntimeLib.h>
> +
> +EFI_STATUS
> +SwitchRtcI2cChannelAndLock (
> +  VOID
> +  )
> +{
> +  UINT8   Temp;
> +  UINT8   Count;
> +
> +  for (Count = 0; Count < 100; Count++) {
> +    // To get the other side's state is idle first
> +    Temp = ReadCpldReg (CPLD_I2C_SWITCH_FLAG);
> +    if ((Temp & BIT3) != 0) {
> +      (VOID) MicroSecondDelay (RTC_DELAY_30_MS);
> +      // Try 100 times, if BMC has not released the bus, return preemption failed
> +      if (Count == 99) {
> +        if (!EfiAtRuntime ()) {
> +          DEBUG ((DEBUG_ERROR, "[%a]:[%dL] Clear cpu_i2c_rtc_state 100 times fail!\n",
> +            __FUNCTION__, __LINE__));
> +        }
> +        return EFI_DEVICE_ERROR;
> +      }
> +      continue;
> +    }
> +
> +    // if BMC free the bus, can be set 1 preemption
> +    Temp = ReadCpldReg (CPLD_I2C_SWITCH_FLAG);
> +    Temp = Temp | CPU_GET_I2C_CONTROL;
> +    // CPU occupied RTC I2C State
> +    WriteCpldReg (CPLD_I2C_SWITCH_FLAG, Temp);
> +    (VOID)MicroSecondDelay (RTC_DELAY_2_MACROSECOND);
> +    Temp = ReadCpldReg (CPLD_I2C_SWITCH_FLAG);
> +    // Is preempt success
> +    if (CPU_GET_I2C_CONTROL == (Temp & CPU_GET_I2C_CONTROL)) {
> +      break;
> +    }
> +    if (Count == 99) {
> +      if (!EfiAtRuntime ()) {
> +        DEBUG ((DEBUG_ERROR, "[%a]:[%dL]  Clear cpu_i2c_rtc_state fail !!! \n",
> +          __FUNCTION__, __LINE__));
> +      }
> +      return EFI_DEVICE_ERROR;
> +    }
> +    (VOID)MicroSecondDelay (RTC_DELAY_30_MS);
> +  }
> +
> +  //Polling BMC RTC I2C status
> +  for (Count = 0; Count < 100; Count++) {
> +    Temp = ReadCpldReg (CPLD_I2C_SWITCH_FLAG);
> +    if ((Temp & BIT3) == 0) {
> +      return EFI_SUCCESS;
> +    }
> +    (VOID)MicroSecondDelay (RTC_DELAY_30_MS);
> +  }
> +
> +  //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);
> +  WriteCpldReg (CPLD_I2C_SWITCH_FLAG, Temp);
> +
> +  return EFI_NOT_READY;
> +}
> +
> +VOID
> +ReleaseOwnershipOfRtc (
> +  VOID
> +  )
> +{
> +  UINT8   Temp;
> +
> +  Temp = ReadCpldReg (CPLD_I2C_SWITCH_FLAG);
> +  Temp = Temp & ~CPU_GET_I2C_CONTROL;
> +  WriteCpldReg (CPLD_I2C_SWITCH_FLAG, Temp);
> +  return ;
> +}
> diff --git a/Silicon/Hisilicon/Library/RtcHelperLib/RtcHelperLib.inf b/Silicon/Hisilicon/Library/RtcHelperLib/RtcHelperLib.inf
> new file mode 100644
> index 0000000..1a36e64
> --- /dev/null
> +++ b/Silicon/Hisilicon/Library/RtcHelperLib/RtcHelperLib.inf
> @@ -0,0 +1,32 @@
> +#/** @file
> +#
> +#  Copyright (c) 2020, Hisilicon Limited. All rights reserved.<BR>
> +#
> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +#**/
> +
> +[Defines]
> +  INF_VERSION                    = 0x0001001A
> +  BASE_NAME                      = RtcHelperLib
> +  FILE_GUID                      = 5cb1a98f-2408-4fef-b68f-d5d04ff6a91f
> +  MODULE_TYPE                    = BASE
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = RtcHelperLib
> +
> +[Sources.common]
> +  RtcHelperLib.c
> +
> +[Packages]
> +  EmbeddedPkg/EmbeddedPkg.dec
> +  MdePkg/MdePkg.dec
> +  Platform/Hisilicon/D06/D06.dec
> +  Silicon/Hisilicon/HisiPkg.dec
> +
> +[LibraryClasses]
> +  CpldIoLib
> +  DebugLib
> +  IoLib
> +
> +[Depex]
> +  TRUE
> -- 
> 2.8.1
>