From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2a00:1450:4864:20::441; helo=mail-wr1-x441.google.com; envelope-from=leif.lindholm@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-wr1-x441.google.com (mail-wr1-x441.google.com [IPv6:2a00:1450:4864:20::441]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 85908210E0F80 for ; Wed, 8 Aug 2018 02:12:25 -0700 (PDT) Received: by mail-wr1-x441.google.com with SMTP id f12-v6so1315252wrv.12 for ; Wed, 08 Aug 2018 02:12:25 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=d8iqbJaSctTIRCAgQ/x+BipgGq2LbAOfBQKjeeCb0uU=; b=AHSxRUBq44RjkUhGNFOArle2ZkQeg5QXzbPkYNdonrmC60v2XuOf6SflDp6dTrNiFi NPsTm//EXEsCRuwbkahUQS21yaCI8BYFiwm0cAdtbNrIn2EjJl95VSPzlpAV8Zklu7Vp H7zOTuo8CapI3ItDNGKy4siVU3v7powFDZTHw= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=d8iqbJaSctTIRCAgQ/x+BipgGq2LbAOfBQKjeeCb0uU=; b=G8DRmCoKUkot+Vz1XktBxAioH5L0Hw5jEWgKDF/Yjm0G3xsf6FJDce5a+F13PmMD1M 0WNmtLekNRFuRivqtifADu2cdTB1aPFkett/ScFQq/LfT0nODZwwr82AE2+ruL3O5y8B sZD55ql4mQWnrdXrNYdVdpvvqZBJzHV+oV0H1DenlyjyZ4Tt7l87ZS9z0uQ3fRawq9Dr hMdbairNnH1TumvBqNdghG4/ZbTT5mnw0Op7LZxQbeU4KyOojYl1Ur/Zb9spBjhu/oik tQgr8T+6fQEP8jU2BIEF38QtX0wjxE4uan0q9F/inWmptaNyJt4/mrryTG0f2DsOkeIy vpsg== X-Gm-Message-State: AOUpUlGdY6Y2605jO56VQr69u/Si3XAoQdtKeb8++Rbuj2xK/LWtsA/F vGlEVy+PfkjPHXGx6HP23flfIA== X-Google-Smtp-Source: AA+uWPyOzUpTvvvXNreIutlfh9Kag4pxugqC1IR+pm3sli6evsewJQhKQjJDs+WhDfKtVw8ZApwZEQ== X-Received: by 2002:a5d:67c6:: with SMTP id n6-v6mr1290645wrw.39.1533719543697; Wed, 08 Aug 2018 02:12:23 -0700 (PDT) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id y191-v6sm4895943wmy.4.2018.08.08.02.12.22 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Wed, 08 Aug 2018 02:12:22 -0700 (PDT) Date: Wed, 8 Aug 2018 10:12:20 +0100 From: Leif Lindholm To: Ming Cc: linaro-uefi@lists.linaro.org, edk2-devel@lists.01.org, graeme.gregory@linaro.org, ard.biesheuvel@linaro.org, guoheyi@huawei.com, wanghuiqiang@huawei.com, huangming23@huawei.com, zhangjinsong2@huawei.com, huangdaode@hisilicon.com, john.garry@huawei.com, xinliang.liu@linaro.org, Heyi Guo Message-ID: <20180808091220.ui5sw4mckiblcwih@bivouac.eciton.net> References: <20180724070922.63362-1-ming.huang@linaro.org> <20180724070922.63362-5-ming.huang@linaro.org> <20180802165659.fbu6c7octfcddjkh@bivouac.eciton.net> <50d70550-4290-ad96-afd3-7c0fc8bf8c40@linaro.org> MIME-Version: 1.0 In-Reply-To: <50d70550-4290-ad96-afd3-7c0fc8bf8c40@linaro.org> User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [PATCH edk2-platforms v1 04/38] Platform/Hisilicon/D06: Add M41T83RealTimeClockLib X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.27 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 08 Aug 2018 09:12:26 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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.
> >> + Copyright (c) 2018, Linaro Limited. All rights reserved.
> >> + > >> + 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 > >> +#include > >> +#include > >> +#include > >> +#include > >> +#include > >> +#include > >> +#include > >> +#include > >> +#include > >> +#include > >> +#include > >> +#include > >> +#include > >> +#include > >> +#include > >> +#include > >> +#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