On 11/20/2018 2:30 AM, Leif Lindholm wrote:
> On Fri, Nov 16, 2018 at 02:56:53PM +0800, Ming Huang wrote:
>> As M41T83RealTimeClockLib is common library, so move two cpld
>> relative functions to OemMiscLib and rename this two functions.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ming Huang <[email protected]>
>
> I did not see any of my review comments addressed, or any response
> saying it was not being changed.
>
> My feedback was:
> ---
> This would be more clear as "platform specific" than "cpld relative".
>
> I did not realise this wasn't a Hisilicon component when reviewing the
> original set.
>
> I approve of this change, but can you tell me why it is included in
> this set? If the goal is to make the M41T83 support platform
> independent, should the library also move to Silicon/ST/?
> ---
>
> So could you please update the commit message, and add a subsequent
> patch moving Library/M41T83RealTimeClockLib to
> Silicon/STMicroelectronics (and updating D06.dsc to match)?
> I do not care if it is not perfectly abstracted yet - we can deal with
> that when we have other users of the component in the tree.
Sorry for missing update the commit message this patch.
I will update it in v3. I try to move the library to Silicon/STMicroelectronics,
but M41T83RealTimeClockLib depend on I2CLib in Hisilicon, so can't move the
library to STMicroelectronics. Main gist of this patch
is making this library as a common module in Hisilicon.
>
> /
> Leif
>
>> ---
>> Silicon/Hisilicon/Library/M41T83RealTimeClockLib/M41T83RealTimeClockLib.inf
>> | 1 -
>> Silicon/Hisilicon/Include/Library/OemMiscLib.h
>> | 9 ++
>> Silicon/Hisilicon/Library/M41T83RealTimeClockLib/M41T83RealTimeClock.h
>> | 4 -
>> Platform/Hisilicon/D06/Library/OemMiscLibD06/OemMiscLibD06.c
>> | 82 ++++++++++++++++++
>> Silicon/Hisilicon/Library/M41T83RealTimeClockLib/M41T83RealTimeClockLib.c
>> | 90 ++------------------
>> 5 files changed, 98 insertions(+), 88 deletions(-)
>>
>> diff --git
>> a/Silicon/Hisilicon/Library/M41T83RealTimeClockLib/M41T83RealTimeClockLib.inf
>>
>> b/Silicon/Hisilicon/Library/M41T83RealTimeClockLib/M41T83RealTimeClockLib.inf
>> index e0bf6b3f24db..4e963fd4531a 100644
>> ---
>> a/Silicon/Hisilicon/Library/M41T83RealTimeClockLib/M41T83RealTimeClockLib.inf
>> +++
>> b/Silicon/Hisilicon/Library/M41T83RealTimeClockLib/M41T83RealTimeClockLib.inf
>> @@ -27,7 +27,6 @@ [Sources.common]
>> [Packages]
>> EmbeddedPkg/EmbeddedPkg.dec
>> MdePkg/MdePkg.dec
>> - Platform/Hisilicon/D06/D06.dec
>> Silicon/Hisilicon/HisiPkg.dec
>>
>> [LibraryClasses]
>> diff --git a/Silicon/Hisilicon/Include/Library/OemMiscLib.h
>> b/Silicon/Hisilicon/Include/Library/OemMiscLib.h
>> index 86ea6a1b3deb..0d7bf71b17d2 100644
>> --- a/Silicon/Hisilicon/Include/Library/OemMiscLib.h
>> +++ b/Silicon/Hisilicon/Include/Library/OemMiscLib.h
>> @@ -53,4 +53,13 @@ BOOLEAN OemIsNeedDisableExpanderBuffer(VOID);
>>
>> extern EFI_STRING_ID gDimmToDevLocator[MAX_SOCKET][MAX_CHANNEL][MAX_DIMM];
>> EFI_HII_HANDLE EFIAPI OemGetPackages ();
>> +
>> +VOID
>> +OemReleaseOwnershipOfRtc (
>> + VOID
>> + );
>> +EFI_STATUS
>> +OemSwitchRtcI2cChannelAndLock (
>> + VOID
>> + );
>> #endif
>> diff --git
>> a/Silicon/Hisilicon/Library/M41T83RealTimeClockLib/M41T83RealTimeClock.h
>> b/Silicon/Hisilicon/Library/M41T83RealTimeClockLib/M41T83RealTimeClock.h
>> index d985055d9bb6..f32910885856 100644
>> --- a/Silicon/Hisilicon/Library/M41T83RealTimeClockLib/M41T83RealTimeClock.h
>> +++ b/Silicon/Hisilicon/Library/M41T83RealTimeClockLib/M41T83RealTimeClock.h
>> @@ -17,11 +17,7 @@
>> #define __M41T83_REAL_TIME_CLOCK_H__
>>
>> // 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_1000_MACROSECOND 1000
>> -// The delay is need for cpld and I2C. This is a empirical value.
>> MemoryFence is no need.
>> -#define RTC_DELAY_2_MACROSECOND 2
>>
>> #define M41T83_REGADDR_DOTSECONDS 0x00
>> #define M41T83_REGADDR_SECONDS 0x01
>> diff --git a/Platform/Hisilicon/D06/Library/OemMiscLibD06/OemMiscLibD06.c
>> b/Platform/Hisilicon/D06/Library/OemMiscLibD06/OemMiscLibD06.c
>> index 2a9db46d1ff9..64d167d18ae6 100644
>> --- a/Platform/Hisilicon/D06/Library/OemMiscLibD06/OemMiscLibD06.c
>> +++ b/Platform/Hisilicon/D06/Library/OemMiscLibD06/OemMiscLibD06.c
>> @@ -17,6 +17,7 @@
>> #include <PlatformArch.h>
>> #include <Library/BaseMemoryLib.h>
>> #include <Library/CpldD06.h>
>> +#include <Library/CpldIoLib.h>
>> #include <Library/DebugLib.h>
>> #include <Library/IoLib.h>
>> #include <Library/LpcLib.h>
>> @@ -27,6 +28,12 @@
>> #include <Library/SerdesLib.h>
>> #include <Library/SerialPortLib.h>
>> #include <Library/TimerLib.h>
>> +#include <Library/UefiRuntimeLib.h>
>> +
>> +// 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
>>
>> REPORT_PCIEDIDVID2BMC PcieDeviceToReport[PCIEDEVICE_REPORT_MAX] = {
>> {67,0,0,0},
>> @@ -207,3 +214,78 @@ OemIsNeedDisableExpanderBuffer (
>> {
>> return TRUE;
>> }
>> +
>> +EFI_STATUS
>> +OemSwitchRtcI2cChannelAndLock (
>> + 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
>> +OemReleaseOwnershipOfRtc (
>> + VOID
>> + )
>> +{
>> + UINT8 Temp;
>> +
>> + Temp = ReadCpldReg (CPLD_I2C_SWITCH_FLAG);
>> + Temp = Temp & ~CPU_GET_I2C_CONTROL;
>> + WriteCpldReg (CPLD_I2C_SWITCH_FLAG, Temp);
>> +}
>> diff --git
>> a/Silicon/Hisilicon/Library/M41T83RealTimeClockLib/M41T83RealTimeClockLib.c
>> b/Silicon/Hisilicon/Library/M41T83RealTimeClockLib/M41T83RealTimeClockLib.c
>> index 0670f9c5f47c..1f50ad4b64c4 100644
>> ---
>> a/Silicon/Hisilicon/Library/M41T83RealTimeClockLib/M41T83RealTimeClockLib.c
>> +++
>> b/Silicon/Hisilicon/Library/M41T83RealTimeClockLib/M41T83RealTimeClockLib.c
>> @@ -17,10 +17,10 @@
>> #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/OemMiscLib.h>
>> #include <Library/TimeBaseLib.h>
>> #include <Library/TimerLib.h>
>> #include <Library/UefiLib.h>
>> @@ -32,70 +32,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.
>>
>> @@ -142,18 +78,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 (
>> @@ -178,7 +102,7 @@ InitializeM41T83 (
>> return Status;
>> }
>>
>> - Status = SwitchRtcI2cChannelAndLock ();
>> + Status = OemSwitchRtcI2cChannelAndLock ();
>> if (EFI_ERROR (Status)) {
>> DEBUG ((DEBUG_ERROR, "Get i2c preemption failed: %r\n", Status));
>> if (!EfiAtRuntime ()) {
>> @@ -231,7 +155,7 @@ InitializeM41T83 (
>>
>> Exit:
>> // Release RTC Lock.
>> - ReleaseOwnershipOfRtc ();
>> + OemReleaseOwnershipOfRtc ();
>> if (!EfiAtRuntime ()) {
>> EfiReleaseLock (&mRtcLock);
>> }
>> @@ -274,7 +198,7 @@ LibSetTime (
>> return EFI_INVALID_PARAMETER;
>> }
>>
>> - Status = SwitchRtcI2cChannelAndLock ();
>> + Status = OemSwitchRtcI2cChannelAndLock ();
>> if (EFI_ERROR (Status)) {
>> return Status;
>> }
>> @@ -332,7 +256,7 @@ LibSetTime (
>> }
>>
>> Exit:
>> - ReleaseOwnershipOfRtc ();
>> + OemReleaseOwnershipOfRtc ();
>> // Release RTC Lock.
>> if (!EfiAtRuntime ()) {
>> if (EFI_ERROR (Status)) {
>> @@ -377,7 +301,7 @@ LibGetTime (
>> return EFI_INVALID_PARAMETER;
>> }
>>
>> - Status = SwitchRtcI2cChannelAndLock ();
>> + Status = OemSwitchRtcI2cChannelAndLock ();
>> if (EFI_ERROR (Status)) {
>> return Status;
>> }
>> @@ -422,7 +346,7 @@ LibGetTime (
>> }
>>
>> Exit:
>> - ReleaseOwnershipOfRtc ();
>> + OemReleaseOwnershipOfRtc ();
>> // Release RTC Lock.
>> if (!EfiAtRuntime ()) {
>> if (EFI_ERROR (Status)) {
>> --
>> 2.9.5
>>
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
