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

[Ming Huang <huangming23@huawei.com>]

在 2020/5/19 2:03, Leif Lindholm 写道:
> 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.

Ok

> 
>> +
>> +// 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?

Yes, this is a typo.

> 
> 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?

Ok, modify in v2.

Thanks,
Ming

> 
> /
>     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
>>
> 
> .
>