From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4864:20::442; helo=mail-pf1-x442.google.com; envelope-from=ming.huang@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-pf1-x442.google.com (mail-pf1-x442.google.com [IPv6:2607:f8b0:4864:20::442]) (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 5673221A02937 for ; Mon, 19 Nov 2018 22:38:44 -0800 (PST) Received: by mail-pf1-x442.google.com with SMTP id z9so514880pfi.2 for ; Mon, 19 Nov 2018 22:38:44 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=D+Wc8NrbG35QEJ9rwPTrEY4fMpV/WY7qQsSgw3fmGak=; b=AzdscEct9RCFJa5XxpHCSi2P0PUkjEC2+Mk9idgF3CmrIgkS2m0VPatbOyTRUUNTrM EzUTf1sscUOcm5oeU6tMIPtYpFgA8M+BeBSREXVixzw/ppNuhzPhSOSkhg8jeyZuG/+k J/1hFjaHggDQ/I0l4broxk24LQC5t2BWRtx8I= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=D+Wc8NrbG35QEJ9rwPTrEY4fMpV/WY7qQsSgw3fmGak=; b=rQSNoYIUfx1xe+cAqhdQKaP04mt3ucSBXNo0ix4ctqI0HIJiquq65u6yU37sfTIjal VG5vxGrj3sMvGhSBf7tjODT28oBeiikIevwwC70QJBZrBBenVSglkMEU/2jPyZAg8SVZ II+3vKSJvnPsHQOJg3cb/Ylgiie2wHmAPd20r3Ebv+PidQ9sTg9PqXUIpCoTB6WP9J0t Xzj8BKGIhrEJH7FmsEo6wng5kwoi4ouTR+uvEy6hHgeLgHqAHZc3Dq6Go6Rp1b7+RkgJ rAiRpcoCwmnyDp2xOow6QHxAMFHtBF7vPJSKnHhdXfXY1394/8pzpbrobU02lYPJMBoI WQrg== X-Gm-Message-State: AGRZ1gLqtMp1ubLdi09kgANQaMTyz3cvG1owk781KDkqZzIQj474ax6v 0m8IBT5sp/sM0TqBaEq5Wrjzeg== X-Google-Smtp-Source: AJdET5ePmUrhxA1Vn3XI8tM+shmyWjt3og9xnKHu3rCmk3SPy1QdnYOB2FE4IwpkYFnKMxQ4TZ1GBw== X-Received: by 2002:a62:d504:: with SMTP id d4mr977014pfg.38.1542695923344; Mon, 19 Nov 2018 22:38:43 -0800 (PST) Received: from [10.139.0.118] ([64.64.108.162]) by smtp.gmail.com with ESMTPSA id d8-v6sm50252771pfj.106.2018.11.19.22.38.35 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 19 Nov 2018 22:38:42 -0800 (PST) To: Leif Lindholm 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 References: <20181116065702.30559-1-ming.huang@linaro.org> <20181116065702.30559-7-ming.huang@linaro.org> <20181119183049.drqgb5uefxkwyqkx@bivouac.eciton.net> From: Ming Huang Message-ID: Date: Tue, 20 Nov 2018 14:38:32 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101 Thunderbird/60.0 MIME-Version: 1.0 In-Reply-To: <20181119183049.drqgb5uefxkwyqkx@bivouac.eciton.net> Subject: Re: [PATCH edk2-platforms v2 06/15] 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: Tue, 20 Nov 2018 06:38:44 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 > > 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 >> #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 0670f9c5f47c..1f50ad4b64c4 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.9.5 >>