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::343; helo=mail-wm1-x343.google.com; envelope-from=leif.lindholm@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-wm1-x343.google.com (mail-wm1-x343.google.com [IPv6:2a00:1450:4864:20::343]) (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 668522118FF00 for ; Tue, 13 Nov 2018 16:04:56 -0800 (PST) Received: by mail-wm1-x343.google.com with SMTP id r63-v6so13792546wma.4 for ; Tue, 13 Nov 2018 16:04:56 -0800 (PST) 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=GMMaJ3YATj+4rKy/9dM92QeQ/ZD5vXATqtyLzkklo4I=; b=SPh2shc6WC51XBGskdMLJlLAgkRlB8b6oKss6CrxEQ4Ge7AExX9zGZAlsBvv2wmBDs a3plhQL6AiOq+ZpHRKcUHvZzG9GOjLcgfvDrDVEo79FF5s0fwrR8KEoOnl4y4mqesvhf kgkITHvyaAAQPVD+sPI2g56wXhAphntE6Ulec= 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=GMMaJ3YATj+4rKy/9dM92QeQ/ZD5vXATqtyLzkklo4I=; b=iDacfrCDMoSIlmBpxLGiqhpFRrO+MCPKWYuTybm0BDdfdG5aKb+CE+7a0CsAQwycYx hudik5zO0f3dEQpSUOvMxhHp7zebimIS7Z3vpvX2/b7ubvcXkkD4+cg/Gdhgi1ykxvCS VpeKupSz1Kiuldg9JBt0iK3GoiJZwqDcSP7LnknZCjqc2LrjXaOhSCNTar+X7+aBJVo2 CycJg1/ECk65DTbBfP3YQJzCCOhcr6BNPNY66zBzfuUUAqTpU8t8twfYf6ekkGCSAG76 PUg9vh3emLdhisqGkS5XWQ1U+4hfRfDD7TSzNNHhIF5rqnBTi9imHvX+CSe1Gdemr5v/ 8IAg== X-Gm-Message-State: AGRZ1gKv2bb9ZeLOUvKCMUr+QEVEadreqsoNwvzgwzSCs8bjwWQ3A/XD NZ2wjiUfrZ0bEClcPn87bOoS1A== X-Google-Smtp-Source: AJdET5cWyxlMBhc7uN2XPawqcXCofQ5n72YlCZijxawT8B1RuynpYShEWJmRCFQTPUwD1wJsCg/eDg== X-Received: by 2002:a7b:c7d3:: with SMTP id z19-v6mr41556wmk.151.1542153894760; Tue, 13 Nov 2018 16:04:54 -0800 (PST) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id h133-v6sm12423376wmf.42.2018.11.13.16.04.53 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 13 Nov 2018 16:04:53 -0800 (PST) Date: Wed, 14 Nov 2018 00:04:52 +0000 From: Leif Lindholm To: Ming Huang Cc: linaro-uefi@lists.linaro.org, edk2-devel@lists.01.org, graeme.gregory@linaro.org, ard.biesheuvel@linaro.org, michael.d.kinney@intel.com, lersek@redhat.com, wanghuiqiang@huawei.com, huangming23@huawei.com, zhangjinsong2@huawei.com, huangdaode@hisilicon.com, john.garry@huawei.com, xinliang.liu@linaro.org, zhangfeng56@huawei.com Message-ID: <20181114000452.gk366schvazwpzrm@bivouac.eciton.net> References: <20181029033249.45363-1-ming.huang@linaro.org> <20181029033249.45363-6-ming.huang@linaro.org> MIME-Version: 1.0 In-Reply-To: <20181029033249.45363-6-ming.huang@linaro.org> User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [PATCH edk2-platforms v1 05/12] Hisilicon/D06: Move some functions to OemMiscLib X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 14 Nov 2018 00:04:56 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Mon, Oct 29, 2018 at 11:32:42AM +0800, Ming Huang wrote: > As M41T83RealTimeClockLib is common library, so move two cpld > relative functions to OemMiscLib and rename this two functions. 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/? / Leif > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Ming Huang > --- > 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 e0bf6b3f24..4e963fd453 100644 > --- a/Silicon/Hisilicon/Library/M41T83RealTimeClockLib/M41T83RealTimeClockLib.inf > +++ b/Silicon/Hisilicon/Library/M41T83RealTimeClockLib/M41T83RealTimeClockLib.inf > @@ -27,7 +27,6 @@ > [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 86ea6a1b3d..0d7bf71b17 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 d985055d9b..f329108858 100644 > --- a/Silicon/Hisilicon/Library/M41T83RealTimeClockLib/M41T83RealTimeClock.h > +++ b/Silicon/Hisilicon/Library/M41T83RealTimeClockLib/M41T83RealTimeClock.h > @@ -16,12 +16,8 @@ > #ifndef __M41T83_REAL_TIME_CLOCK_H__ > #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 2a9db46d1f..64d167d18a 100644 > --- a/Platform/Hisilicon/D06/Library/OemMiscLibD06/OemMiscLibD06.c > +++ b/Platform/Hisilicon/D06/Library/OemMiscLibD06/OemMiscLibD06.c > @@ -17,6 +17,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -27,6 +28,12 @@ > #include > #include > #include > +#include > + > +// 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 0670f9c5f4..1f50ad4b64 100644 > --- a/Silicon/Hisilicon/Library/M41T83RealTimeClockLib/M41T83RealTimeClockLib.c > +++ b/Silicon/Hisilicon/Library/M41T83RealTimeClockLib/M41T83RealTimeClockLib.c > @@ -17,10 +17,10 @@ > #include > #include > #include > -#include > #include > #include > #include > +#include > #include > #include > #include > @@ -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.18.0 >