From: Ming Huang <ming.huang@linaro.org>
To: Leif Lindholm <leif.lindholm@linaro.org>
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
Subject: Re: [PATCH edk2-platforms v2 06/15] Hisilicon/D06: Move some functions to OemMiscLib
Date: Tue, 20 Nov 2018 14:38:32 +0800 [thread overview]
Message-ID: <a6fa2cc7-6d57-df10-3fab-87e7941dca0c@linaro.org> (raw)
In-Reply-To: <20181119183049.drqgb5uefxkwyqkx@bivouac.eciton.net>
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 <ming.huang@linaro.org>
>
> 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
>>
next prev parent reply other threads:[~2018-11-20 6:38 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-11-16 6:56 [PATCH edk2-platforms v2 00/15] Fix D06 SBSA/SBBR issue and improve Ming Huang
2018-11-16 6:56 ` [PATCH edk2-platforms v2 01/15] Hisilicon/D0x: Modify IORT Ming Huang
2018-11-16 6:56 ` [PATCH edk2-platforms v2 02/15] Silicon/Hisilicon/D06: Add watchdog to GTDT Ming Huang
2018-11-16 6:56 ` [PATCH edk2-platforms v2 03/15] Silicon/Hisilicon/D06: Drop _CID for fwts issue Ming Huang
2018-11-16 6:56 ` [PATCH edk2-platforms v2 04/15] Silicon/Hisilicon/D06: Fix fwts issue in Dbg2 Ming Huang
2018-11-16 6:56 ` [PATCH edk2-platforms v2 05/15] Silicon/Hisilicon/D06: Fix fwts issue in FADT Ming Huang
2018-11-16 6:56 ` [PATCH edk2-platforms v2 06/15] Hisilicon/D06: Move some functions to OemMiscLib Ming Huang
2018-11-19 18:30 ` Leif Lindholm
2018-11-20 6:38 ` Ming Huang [this message]
2018-11-20 10:38 ` Leif Lindholm
2018-11-16 6:56 ` [PATCH edk2-platforms v2 07/15] Hisilicon/D0x: Fix secure boot bug in FlashFvbDxe Ming Huang
2018-11-19 18:13 ` Leif Lindholm
2018-11-20 6:42 ` Ming Huang
2018-11-19 18:19 ` Ard Biesheuvel
2018-11-20 6:44 ` Ming Huang
2018-11-16 6:56 ` [PATCH edk2-platforms v2 08/15] Silicon/Hisilicon/D06: Reserve ECAM resource in DSDT Ming Huang
2018-11-16 6:56 ` [PATCH edk2-platforms v2 09/15] Silicon/Hisilicon/D06: Modify GTDT timer flag Ming Huang
2018-11-16 6:56 ` [PATCH edk2-platforms v2 10/15] Hisilicon/D06: Modify Gic base Ming Huang
2018-11-19 18:20 ` Leif Lindholm
2018-11-20 6:55 ` Ming Huang
2018-11-20 10:40 ` Leif Lindholm
2018-11-16 6:56 ` [PATCH edk2-platforms v2 11/15] Silicon/Hisilicon/D06: Set TA as Node 0 for TA boot Ming Huang
2018-11-16 6:56 ` [PATCH edk2-platforms v2 12/15] Silicon/Hisilicon/D03: Drop _CID for fwts issue Ming Huang
2018-11-16 6:57 ` [PATCH edk2-platforms v2 13/15] Silicon/Hisilicon/D05: " Ming Huang
2018-11-16 6:57 ` [PATCH edk2-platforms v2 14/15] Hisilicon: Drop Pv660 source code Ming Huang
2018-11-16 6:57 ` [PATCH edk2-platforms v2 15/15] Hisilicon/D06: Correct HIDs/UIDs of PCI host bridges Ming Huang
2018-11-19 18:42 ` [PATCH edk2-platforms v2 00/15] Fix D06 SBSA/SBBR issue and improve Leif Lindholm
2018-11-20 7:02 ` Ming Huang
2018-11-20 10:32 ` Leif Lindholm
2018-11-20 12:42 ` Ming Huang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a6fa2cc7-6d57-df10-3fab-87e7941dca0c@linaro.org \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox