[Linaro-uefi,linaro-uefi,v6,35/37] D03: enhance RTC lock acquiring

We would acquire I2C lock only when I2C status in CPLD shows idle,
however, acquiring lock will still fail for BMC might acquire the lock
at exactly the same time.
So we add additional check to see if we really get the lock. Timeout process
is also added to avoid system hang due to possible deadlock or device error.
Code style is improved as well.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Peicong Li <lipeicong@huawei.com>
Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
---
 Platforms/Hisilicon/D03/Include/Library/CpldD03.h  |  4 ++
 .../DS3231RealTimeClockLib.c                       | 83 +++++++++++++++++-----
 .../DS3231RealTimeClockLib.inf                     |  2 +
 3 files changed, 70 insertions(+), 19 deletions(-)

On Tue, Dec 06, 2016 at 06:57:06PM +0800, Heyi Guo wrote:
> We would acquire I2C lock only when I2C status in CPLD shows idle,
> however, acquiring lock will still fail for BMC might acquire the lock
> at exactly the same time.
> So we add additional check to see if we really get the lock. Timeout process
> is also added to avoid system hang due to possible deadlock or device error.
> Code style is improved as well.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Peicong Li <lipeicong@huawei.com>
> Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
> ---
>  Platforms/Hisilicon/D03/Include/Library/CpldD03.h  |  4 ++
>  .../DS3231RealTimeClockLib.c                       | 83 +++++++++++++++++-----
>  .../DS3231RealTimeClockLib.inf                     |  2 +
>  3 files changed, 70 insertions(+), 19 deletions(-)
> 
> diff --git a/Platforms/Hisilicon/D03/Include/Library/CpldD03.h b/Platforms/Hisilicon/D03/Include/Library/CpldD03.h
> index 78aec2f..456bf4b 100644
> --- a/Platforms/Hisilicon/D03/Include/Library/CpldD03.h
> +++ b/Platforms/Hisilicon/D03/Include/Library/CpldD03.h
> @@ -17,5 +17,9 @@
>  #define __CPLD_D03_H__
>  
>  #define CPLD_BIOSINDICATE_FLAG  0x09
> +#define CPLD_I2C_SWITCH_FLAG    0x17
> +#define CPU_GET_I2C_CONTROL     BIT2
> +#define BMC_I2C_STATUS          BIT3
> +
>  
>  #endif
> diff --git a/Platforms/Hisilicon/D03/Library/DS3231RealTimeClockLib/DS3231RealTimeClockLib.c b/Platforms/Hisilicon/D03/Library/DS3231RealTimeClockLib/DS3231RealTimeClockLib.c
> index fa63027..3d4a624 100644
> --- a/Platforms/Hisilicon/D03/Library/DS3231RealTimeClockLib/DS3231RealTimeClockLib.c
> +++ b/Platforms/Hisilicon/D03/Library/DS3231RealTimeClockLib/DS3231RealTimeClockLib.c
> @@ -38,6 +38,7 @@
>  #include <Protocol/RealTimeClock.h>
>  #include <Library/I2CLib.h>
>  #include "DS3231RealTimeClock.h"
> +#include <Library/CpldD03.h>
>  #include <Library/CpldIoLib.h>
>  
>  extern I2C_DEVICE gDS3231RtcDevice;
> @@ -56,6 +57,48 @@ IdentifyDS3231 (
>  }
>  
>  EFI_STATUS
> +SwitchRtcI2cChannelAndLock(VOID)

Coding style:

SwitchRtcI2cChannelAndLock (
  VOID
  )

> +{
> +  UINT8   Temp;
> +  UINT8   Count;
> +
> +  for (Count = 0; Count < 20; Count++){
> +    Temp = ReadCpldReg(CPLD_I2C_SWITCH_FLAG);

       Temp = ReadCpldReg (CPLD_I2C_SWITCH_FLAG);

> +
> +    if ((Temp & BMC_I2C_STATUS) != 0){

       if ((Temp & BMC_I2C_STATUS) != 0) {

> +      //The I2C channel is shared with BMC,
> +      //and we need some time to get the idle status,
> +      //we only switch I2C channel to RTC when it is not busy

So, what I am seeing here is an arbitration scheme designed such that:
- Check if BMC has taken ownership of I2C.
  - If so, wait 30us.
    - Try again.
  - If not, start using I2C.

What prevents the BMC from starting a transaction after this CPU has
started this transaction?

> +      MicroSecondDelay(30000);

         MicroSecondDelay (30000);

> +
> +      continue;
> +    }
> +
> +    Temp = ReadCpldReg(CPLD_I2C_SWITCH_FLAG);

Another missing space. Please review and adjust consistently.

> +    Temp = Temp | CPU_GET_I2C_CONTROL;
> +    WriteCpldReg(CPLD_I2C_SWITCH_FLAG, Temp);

Add a blank line please.

> +    //This is empirical value,give cpld some time to make sure the
> +    //value is wrote in
> +    MicroSecondDelay(2);

This probably needs a barrier.
Please check whether the delay is still needed after adding a barrier.

> +    Temp = ReadCpldReg(CPLD_I2C_SWITCH_FLAG);
> +
> +    if ((Temp & CPU_GET_I2C_CONTROL) == CPU_GET_I2C_CONTROL){
> +      return EFI_SUCCESS;
> +    }

Blank line please.

The rest of the file has further minor style issues, but nothing further.

By the way - I appreciate the code improvements done as part of this
modification.

Regards,

Leif

> +    //There need 30ms to keep consistent with the previous loops if the CPU failed
> +    //to get control of I2C
> +    MicroSecondDelay(30000);
> +  }
> +
> +  Temp = ReadCpldReg(CPLD_I2C_SWITCH_FLAG);
> +  Temp = Temp & ~CPU_GET_I2C_CONTROL;
> +  WriteCpldReg(CPLD_I2C_SWITCH_FLAG, Temp);
> +
> +  return EFI_NOT_READY;
> +}
> +
> +
> +EFI_STATUS
>  InitializeDS3231 (
>    VOID
>    )
> @@ -136,19 +179,17 @@ LibGetTime (
>      return EFI_INVALID_PARAMETER;
>    }
>  
> -
> -
> -  Temp = ReadCpldReg(0x17);
> -  while( (Temp & BIT3) != 0)
> -  {
> -      Temp = ReadCpldReg(0x17);
> +  Status = SwitchRtcI2cChannelAndLock();
> +  if(EFI_ERROR (Status)) {
> +    return Status;
>    }
> -  WriteCpldReg(0x17,0x4);
> +
>    // Initialize the hardware if not already done
>    if (!mDS3231Initialized) {
>      Status = InitializeDS3231 ();
>      if (EFI_ERROR (Status)) {
> -      return EFI_NOT_READY;
> +      Status = EFI_NOT_READY;
> +      goto GExit;
>      }
>    }
>  
> @@ -175,7 +216,8 @@ LibGetTime (
>  
>    BaseHour = 0;
>    if((Temp&0x30) == 0x30){
> -    return EFI_DEVICE_ERROR;
> +    Status = EFI_DEVICE_ERROR;
> +    goto GExit;
>    }else if(Temp&0x20){
>      BaseHour = 20;
>    }else if(Temp&0x10){
> @@ -196,11 +238,15 @@ LibGetTime (
>    Time->TimeZone    = EFI_UNSPECIFIED_TIMEZONE;
>  
>    if((EFI_ERROR(Status)) || (!IsTimeValid(Time)) || ((Time->Year - BaseYear) > 99)) {
> -    return EFI_DEVICE_ERROR;
> +    Status = EFI_UNSUPPORTED;
>    }
>  
> -  WriteCpldReg(0x17,0x0);
> -  return EFI_SUCCESS;
> +GExit:
> +  Temp = ReadCpldReg(CPLD_I2C_SWITCH_FLAG);
> +  Temp = Temp & ~CPU_GET_I2C_CONTROL;
> +  WriteCpldReg(CPLD_I2C_SWITCH_FLAG, Temp);
> +
> +  return Status;
>  
>  }
>  
> @@ -234,13 +280,10 @@ LibSetTime (
>      return EFI_INVALID_PARAMETER;
>    }
>  
> -
> -  Temp = ReadCpldReg(0x17);
> -  while( (Temp & BIT3) != 0)
> -  {
> -      Temp = ReadCpldReg(0x17);
> +  Status = SwitchRtcI2cChannelAndLock();
> +  if(EFI_ERROR (Status)) {
> +    return Status;
>    }
> -  WriteCpldReg(0x17,0x4);
>  
>    // Initialize the hardware if not already done
>    if (!mDS3231Initialized) {
> @@ -313,7 +356,9 @@ LibSetTime (
>  
>    EXIT:
>  
> -  WriteCpldReg(0x17,0x0);
> +  Temp = ReadCpldReg(CPLD_I2C_SWITCH_FLAG);
> +  Temp = Temp & ~CPU_GET_I2C_CONTROL;
> +  WriteCpldReg(CPLD_I2C_SWITCH_FLAG, Temp);
>  
>    return Status;
>  }
> diff --git a/Platforms/Hisilicon/D03/Library/DS3231RealTimeClockLib/DS3231RealTimeClockLib.inf b/Platforms/Hisilicon/D03/Library/DS3231RealTimeClockLib/DS3231RealTimeClockLib.inf
> index 8121f37..ae1b9b8 100644
> --- a/Platforms/Hisilicon/D03/Library/DS3231RealTimeClockLib/DS3231RealTimeClockLib.inf
> +++ b/Platforms/Hisilicon/D03/Library/DS3231RealTimeClockLib/DS3231RealTimeClockLib.inf
> @@ -30,6 +30,7 @@
>    MdePkg/MdePkg.dec
>    EmbeddedPkg/EmbeddedPkg.dec
>    OpenPlatformPkg/OpenPlatformPkg.dec
> +  OpenPlatformPkg/Platforms/Hisilicon/D03/D03.dec
>    OpenPlatformPkg/Chips/Hisilicon/HisiPkg.dec
>  
>  [LibraryClasses]
> @@ -41,6 +42,7 @@
>    TimerLib
>  # Use EFiAtRuntime to check stage
>    UefiRuntimeLib
> +  CpldIoLib
>    EfiTimeBaseLib
>  
>  [Pcd]
> -- 
> 1.9.1
>

Hi Leif,


在 12/6/2016 8:42 PM, Leif Lindholm 写道:
> On Tue, Dec 06, 2016 at 06:57:06PM +0800, Heyi Guo wrote:
>> We would acquire I2C lock only when I2C status in CPLD shows idle,
>> however, acquiring lock will still fail for BMC might acquire the lock
>> at exactly the same time.
>> So we add additional check to see if we really get the lock. Timeout process
>> is also added to avoid system hang due to possible deadlock or device error.
>> Code style is improved as well.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Peicong Li <lipeicong@huawei.com>
>> Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
>> ---
>>   Platforms/Hisilicon/D03/Include/Library/CpldD03.h  |  4 ++
>>   .../DS3231RealTimeClockLib.c                       | 83 +++++++++++++++++-----
>>   .../DS3231RealTimeClockLib.inf                     |  2 +
>>   3 files changed, 70 insertions(+), 19 deletions(-)
>>
>> diff --git a/Platforms/Hisilicon/D03/Include/Library/CpldD03.h b/Platforms/Hisilicon/D03/Include/Library/CpldD03.h
>> index 78aec2f..456bf4b 100644
>> --- a/Platforms/Hisilicon/D03/Include/Library/CpldD03.h
>> +++ b/Platforms/Hisilicon/D03/Include/Library/CpldD03.h
>> @@ -17,5 +17,9 @@
>>   #define __CPLD_D03_H__
>>   
>>   #define CPLD_BIOSINDICATE_FLAG  0x09
>> +#define CPLD_I2C_SWITCH_FLAG    0x17
>> +#define CPU_GET_I2C_CONTROL     BIT2
>> +#define BMC_I2C_STATUS          BIT3
>> +
>>   
>>   #endif
>> diff --git a/Platforms/Hisilicon/D03/Library/DS3231RealTimeClockLib/DS3231RealTimeClockLib.c b/Platforms/Hisilicon/D03/Library/DS3231RealTimeClockLib/DS3231RealTimeClockLib.c
>> index fa63027..3d4a624 100644
>> --- a/Platforms/Hisilicon/D03/Library/DS3231RealTimeClockLib/DS3231RealTimeClockLib.c
>> +++ b/Platforms/Hisilicon/D03/Library/DS3231RealTimeClockLib/DS3231RealTimeClockLib.c
>> @@ -38,6 +38,7 @@
>>   #include <Protocol/RealTimeClock.h>
>>   #include <Library/I2CLib.h>
>>   #include "DS3231RealTimeClock.h"
>> +#include <Library/CpldD03.h>
>>   #include <Library/CpldIoLib.h>
>>   
>>   extern I2C_DEVICE gDS3231RtcDevice;
>> @@ -56,6 +57,48 @@ IdentifyDS3231 (
>>   }
>>   
>>   EFI_STATUS
>> +SwitchRtcI2cChannelAndLock(VOID)
> Coding style:
>
> SwitchRtcI2cChannelAndLock (
>    VOID
>    )
>
>> +{
>> +  UINT8   Temp;
>> +  UINT8   Count;
>> +
>> +  for (Count = 0; Count < 20; Count++){
>> +    Temp = ReadCpldReg(CPLD_I2C_SWITCH_FLAG);
>         Temp = ReadCpldReg (CPLD_I2C_SWITCH_FLAG);
>
>> +
>> +    if ((Temp & BMC_I2C_STATUS) != 0){
>         if ((Temp & BMC_I2C_STATUS) != 0) {
>
>> +      //The I2C channel is shared with BMC,
>> +      //and we need some time to get the idle status,
>> +      //we only switch I2C channel to RTC when it is not busy
> So, what I am seeing here is an arbitration scheme designed such that:
> - Check if BMC has taken ownership of I2C.
>    - If so, wait 30us.
>      - Try again.
>    - If not, start using I2C.
>
> What prevents the BMC from starting a transaction after this CPU has
> started this transaction?
>
>> +      MicroSecondDelay(30000);
>           MicroSecondDelay (30000);
>
>> +
>> +      continue;
>> +    }
>> +
>> +    Temp = ReadCpldReg(CPLD_I2C_SWITCH_FLAG);
> Another missing space. Please review and adjust consistently.
>
>> +    Temp = Temp | CPU_GET_I2C_CONTROL;
>> +    WriteCpldReg(CPLD_I2C_SWITCH_FLAG, Temp);
> Add a blank line please.
>
>> +    //This is empirical value,give cpld some time to make sure the
>> +    //value is wrote in
>> +    MicroSecondDelay(2);
> This probably needs a barrier.
> Please check whether the delay is still needed after adding a barrier.
It is not a barrier problem, the CPU can operate the command one by one,
but the cpld is a independent hardware, it has its own firmware and need 
some time to
process data.

Thanks,
Heyi

>> +    Temp = ReadCpldReg(CPLD_I2C_SWITCH_FLAG);
>> +
>> +    if ((Temp & CPU_GET_I2C_CONTROL) == CPU_GET_I2C_CONTROL){
>> +      return EFI_SUCCESS;
>> +    }
> Blank line please.
>
> The rest of the file has further minor style issues, but nothing further.
>
> By the way - I appreciate the code improvements done as part of this
> modification.
>
> Regards,
>
> Leif
>
>> +    //There need 30ms to keep consistent with the previous loops if the CPU failed
>> +    //to get control of I2C
>> +    MicroSecondDelay(30000);
>> +  }
>> +
>> +  Temp = ReadCpldReg(CPLD_I2C_SWITCH_FLAG);
>> +  Temp = Temp & ~CPU_GET_I2C_CONTROL;
>> +  WriteCpldReg(CPLD_I2C_SWITCH_FLAG, Temp);
>> +
>> +  return EFI_NOT_READY;
>> +}
>> +
>> +
>> +EFI_STATUS
>>   InitializeDS3231 (
>>     VOID
>>     )
>> @@ -136,19 +179,17 @@ LibGetTime (
>>       return EFI_INVALID_PARAMETER;
>>     }
>>   
>> -
>> -
>> -  Temp = ReadCpldReg(0x17);
>> -  while( (Temp & BIT3) != 0)
>> -  {
>> -      Temp = ReadCpldReg(0x17);
>> +  Status = SwitchRtcI2cChannelAndLock();
>> +  if(EFI_ERROR (Status)) {
>> +    return Status;
>>     }
>> -  WriteCpldReg(0x17,0x4);
>> +
>>     // Initialize the hardware if not already done
>>     if (!mDS3231Initialized) {
>>       Status = InitializeDS3231 ();
>>       if (EFI_ERROR (Status)) {
>> -      return EFI_NOT_READY;
>> +      Status = EFI_NOT_READY;
>> +      goto GExit;
>>       }
>>     }
>>   
>> @@ -175,7 +216,8 @@ LibGetTime (
>>   
>>     BaseHour = 0;
>>     if((Temp&0x30) == 0x30){
>> -    return EFI_DEVICE_ERROR;
>> +    Status = EFI_DEVICE_ERROR;
>> +    goto GExit;
>>     }else if(Temp&0x20){
>>       BaseHour = 20;
>>     }else if(Temp&0x10){
>> @@ -196,11 +238,15 @@ LibGetTime (
>>     Time->TimeZone    = EFI_UNSPECIFIED_TIMEZONE;
>>   
>>     if((EFI_ERROR(Status)) || (!IsTimeValid(Time)) || ((Time->Year - BaseYear) > 99)) {
>> -    return EFI_DEVICE_ERROR;
>> +    Status = EFI_UNSUPPORTED;
>>     }
>>   
>> -  WriteCpldReg(0x17,0x0);
>> -  return EFI_SUCCESS;
>> +GExit:
>> +  Temp = ReadCpldReg(CPLD_I2C_SWITCH_FLAG);
>> +  Temp = Temp & ~CPU_GET_I2C_CONTROL;
>> +  WriteCpldReg(CPLD_I2C_SWITCH_FLAG, Temp);
>> +
>> +  return Status;
>>   
>>   }
>>   
>> @@ -234,13 +280,10 @@ LibSetTime (
>>       return EFI_INVALID_PARAMETER;
>>     }
>>   
>> -
>> -  Temp = ReadCpldReg(0x17);
>> -  while( (Temp & BIT3) != 0)
>> -  {
>> -      Temp = ReadCpldReg(0x17);
>> +  Status = SwitchRtcI2cChannelAndLock();
>> +  if(EFI_ERROR (Status)) {
>> +    return Status;
>>     }
>> -  WriteCpldReg(0x17,0x4);
>>   
>>     // Initialize the hardware if not already done
>>     if (!mDS3231Initialized) {
>> @@ -313,7 +356,9 @@ LibSetTime (
>>   
>>     EXIT:
>>   
>> -  WriteCpldReg(0x17,0x0);
>> +  Temp = ReadCpldReg(CPLD_I2C_SWITCH_FLAG);
>> +  Temp = Temp & ~CPU_GET_I2C_CONTROL;
>> +  WriteCpldReg(CPLD_I2C_SWITCH_FLAG, Temp);
>>   
>>     return Status;
>>   }
>> diff --git a/Platforms/Hisilicon/D03/Library/DS3231RealTimeClockLib/DS3231RealTimeClockLib.inf b/Platforms/Hisilicon/D03/Library/DS3231RealTimeClockLib/DS3231RealTimeClockLib.inf
>> index 8121f37..ae1b9b8 100644
>> --- a/Platforms/Hisilicon/D03/Library/DS3231RealTimeClockLib/DS3231RealTimeClockLib.inf
>> +++ b/Platforms/Hisilicon/D03/Library/DS3231RealTimeClockLib/DS3231RealTimeClockLib.inf
>> @@ -30,6 +30,7 @@
>>     MdePkg/MdePkg.dec
>>     EmbeddedPkg/EmbeddedPkg.dec
>>     OpenPlatformPkg/OpenPlatformPkg.dec
>> +  OpenPlatformPkg/Platforms/Hisilicon/D03/D03.dec
>>     OpenPlatformPkg/Chips/Hisilicon/HisiPkg.dec
>>   
>>   [LibraryClasses]
>> @@ -41,6 +42,7 @@
>>     TimerLib
>>   # Use EFiAtRuntime to check stage
>>     UefiRuntimeLib
>> +  CpldIoLib
>>     EfiTimeBaseLib
>>   
>>   [Pcd]
>> -- 
>> 1.9.1
>>

On Wed, Dec 07, 2016 at 04:51:52PM +0800, Heyi Guo wrote:
> Hi Leif,
> 
> 
> 在 12/6/2016 8:42 PM, Leif Lindholm 写道:
> >On Tue, Dec 06, 2016 at 06:57:06PM +0800, Heyi Guo wrote:
> >>We would acquire I2C lock only when I2C status in CPLD shows idle,
> >>however, acquiring lock will still fail for BMC might acquire the lock
> >>at exactly the same time.
> >>So we add additional check to see if we really get the lock. Timeout process
> >>is also added to avoid system hang due to possible deadlock or device error.
> >>Code style is improved as well.
> >>
> >>Contributed-under: TianoCore Contribution Agreement 1.0
> >>Signed-off-by: Peicong Li <lipeicong@huawei.com>
> >>Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
> >>---
> >>  Platforms/Hisilicon/D03/Include/Library/CpldD03.h  |  4 ++
> >>  .../DS3231RealTimeClockLib.c                       | 83 +++++++++++++++++-----
> >>  .../DS3231RealTimeClockLib.inf                     |  2 +
> >>  3 files changed, 70 insertions(+), 19 deletions(-)
> >>
> >>diff --git a/Platforms/Hisilicon/D03/Include/Library/CpldD03.h b/Platforms/Hisilicon/D03/Include/Library/CpldD03.h
> >>index 78aec2f..456bf4b 100644
> >>--- a/Platforms/Hisilicon/D03/Include/Library/CpldD03.h
> >>+++ b/Platforms/Hisilicon/D03/Include/Library/CpldD03.h
> >>@@ -17,5 +17,9 @@
> >>  #define __CPLD_D03_H__
> >>  #define CPLD_BIOSINDICATE_FLAG  0x09
> >>+#define CPLD_I2C_SWITCH_FLAG    0x17
> >>+#define CPU_GET_I2C_CONTROL     BIT2
> >>+#define BMC_I2C_STATUS          BIT3
> >>+
> >>  #endif
> >>diff --git a/Platforms/Hisilicon/D03/Library/DS3231RealTimeClockLib/DS3231RealTimeClockLib.c b/Platforms/Hisilicon/D03/Library/DS3231RealTimeClockLib/DS3231RealTimeClockLib.c
> >>index fa63027..3d4a624 100644
> >>--- a/Platforms/Hisilicon/D03/Library/DS3231RealTimeClockLib/DS3231RealTimeClockLib.c
> >>+++ b/Platforms/Hisilicon/D03/Library/DS3231RealTimeClockLib/DS3231RealTimeClockLib.c
> >>@@ -38,6 +38,7 @@
> >>  #include <Protocol/RealTimeClock.h>
> >>  #include <Library/I2CLib.h>
> >>  #include "DS3231RealTimeClock.h"
> >>+#include <Library/CpldD03.h>
> >>  #include <Library/CpldIoLib.h>
> >>  extern I2C_DEVICE gDS3231RtcDevice;
> >>@@ -56,6 +57,48 @@ IdentifyDS3231 (
> >>  }
> >>  EFI_STATUS
> >>+SwitchRtcI2cChannelAndLock(VOID)
> >Coding style:
> >
> >SwitchRtcI2cChannelAndLock (
> >   VOID
> >   )
> >
> >>+{
> >>+  UINT8   Temp;
> >>+  UINT8   Count;
> >>+
> >>+  for (Count = 0; Count < 20; Count++){
> >>+    Temp = ReadCpldReg(CPLD_I2C_SWITCH_FLAG);
> >        Temp = ReadCpldReg (CPLD_I2C_SWITCH_FLAG);
> >
> >>+
> >>+    if ((Temp & BMC_I2C_STATUS) != 0){
> >        if ((Temp & BMC_I2C_STATUS) != 0) {
> >
> >>+      //The I2C channel is shared with BMC,
> >>+      //and we need some time to get the idle status,
> >>+      //we only switch I2C channel to RTC when it is not busy
> >So, what I am seeing here is an arbitration scheme designed such that:
> >- Check if BMC has taken ownership of I2C.
> >   - If so, wait 30us.
> >     - Try again.
> >   - If not, start using I2C.
> >
> >What prevents the BMC from starting a transaction after this CPU has
> >started this transaction?
> >
> >>+      MicroSecondDelay(30000);
> >          MicroSecondDelay (30000);
> >
> >>+
> >>+      continue;
> >>+    }
> >>+
> >>+    Temp = ReadCpldReg(CPLD_I2C_SWITCH_FLAG);
> >Another missing space. Please review and adjust consistently.
> >
> >>+    Temp = Temp | CPU_GET_I2C_CONTROL;
> >>+    WriteCpldReg(CPLD_I2C_SWITCH_FLAG, Temp);
> >Add a blank line please.
> >
> >>+    //This is empirical value,give cpld some time to make sure the
> >>+    //value is wrote in
> >>+    MicroSecondDelay(2);
> >This probably needs a barrier.
> >Please check whether the delay is still needed after adding a barrier.
> It is not a barrier problem, the CPU can operate the command one by one,
> but the cpld is a independent hardware, it has its own firmware and need
> some time to
> process data.

OK.

/
    Leif

> Thanks,
> Heyi
> 
> >>+    Temp = ReadCpldReg(CPLD_I2C_SWITCH_FLAG);
> >>+
> >>+    if ((Temp & CPU_GET_I2C_CONTROL) == CPU_GET_I2C_CONTROL){
> >>+      return EFI_SUCCESS;
> >>+    }
> >Blank line please.
> >
> >The rest of the file has further minor style issues, but nothing further.
> >
> >By the way - I appreciate the code improvements done as part of this
> >modification.
> >
> >Regards,
> >
> >Leif
> >
> >>+    //There need 30ms to keep consistent with the previous loops if the CPU failed
> >>+    //to get control of I2C
> >>+    MicroSecondDelay(30000);
> >>+  }
> >>+
> >>+  Temp = ReadCpldReg(CPLD_I2C_SWITCH_FLAG);
> >>+  Temp = Temp & ~CPU_GET_I2C_CONTROL;
> >>+  WriteCpldReg(CPLD_I2C_SWITCH_FLAG, Temp);
> >>+
> >>+  return EFI_NOT_READY;
> >>+}
> >>+
> >>+
> >>+EFI_STATUS
> >>  InitializeDS3231 (
> >>    VOID
> >>    )
> >>@@ -136,19 +179,17 @@ LibGetTime (
> >>      return EFI_INVALID_PARAMETER;
> >>    }
> >>-
> >>-
> >>-  Temp = ReadCpldReg(0x17);
> >>-  while( (Temp & BIT3) != 0)
> >>-  {
> >>-      Temp = ReadCpldReg(0x17);
> >>+  Status = SwitchRtcI2cChannelAndLock();
> >>+  if(EFI_ERROR (Status)) {
> >>+    return Status;
> >>    }
> >>-  WriteCpldReg(0x17,0x4);
> >>+
> >>    // Initialize the hardware if not already done
> >>    if (!mDS3231Initialized) {
> >>      Status = InitializeDS3231 ();
> >>      if (EFI_ERROR (Status)) {
> >>-      return EFI_NOT_READY;
> >>+      Status = EFI_NOT_READY;
> >>+      goto GExit;
> >>      }
> >>    }
> >>@@ -175,7 +216,8 @@ LibGetTime (
> >>    BaseHour = 0;
> >>    if((Temp&0x30) == 0x30){
> >>-    return EFI_DEVICE_ERROR;
> >>+    Status = EFI_DEVICE_ERROR;
> >>+    goto GExit;
> >>    }else if(Temp&0x20){
> >>      BaseHour = 20;
> >>    }else if(Temp&0x10){
> >>@@ -196,11 +238,15 @@ LibGetTime (
> >>    Time->TimeZone    = EFI_UNSPECIFIED_TIMEZONE;
> >>    if((EFI_ERROR(Status)) || (!IsTimeValid(Time)) || ((Time->Year - BaseYear) > 99)) {
> >>-    return EFI_DEVICE_ERROR;
> >>+    Status = EFI_UNSUPPORTED;
> >>    }
> >>-  WriteCpldReg(0x17,0x0);
> >>-  return EFI_SUCCESS;
> >>+GExit:
> >>+  Temp = ReadCpldReg(CPLD_I2C_SWITCH_FLAG);
> >>+  Temp = Temp & ~CPU_GET_I2C_CONTROL;
> >>+  WriteCpldReg(CPLD_I2C_SWITCH_FLAG, Temp);
> >>+
> >>+  return Status;
> >>  }
> >>@@ -234,13 +280,10 @@ LibSetTime (
> >>      return EFI_INVALID_PARAMETER;
> >>    }
> >>-
> >>-  Temp = ReadCpldReg(0x17);
> >>-  while( (Temp & BIT3) != 0)
> >>-  {
> >>-      Temp = ReadCpldReg(0x17);
> >>+  Status = SwitchRtcI2cChannelAndLock();
> >>+  if(EFI_ERROR (Status)) {
> >>+    return Status;
> >>    }
> >>-  WriteCpldReg(0x17,0x4);
> >>    // Initialize the hardware if not already done
> >>    if (!mDS3231Initialized) {
> >>@@ -313,7 +356,9 @@ LibSetTime (
> >>    EXIT:
> >>-  WriteCpldReg(0x17,0x0);
> >>+  Temp = ReadCpldReg(CPLD_I2C_SWITCH_FLAG);
> >>+  Temp = Temp & ~CPU_GET_I2C_CONTROL;
> >>+  WriteCpldReg(CPLD_I2C_SWITCH_FLAG, Temp);
> >>    return Status;
> >>  }
> >>diff --git a/Platforms/Hisilicon/D03/Library/DS3231RealTimeClockLib/DS3231RealTimeClockLib.inf b/Platforms/Hisilicon/D03/Library/DS3231RealTimeClockLib/DS3231RealTimeClockLib.inf
> >>index 8121f37..ae1b9b8 100644
> >>--- a/Platforms/Hisilicon/D03/Library/DS3231RealTimeClockLib/DS3231RealTimeClockLib.inf
> >>+++ b/Platforms/Hisilicon/D03/Library/DS3231RealTimeClockLib/DS3231RealTimeClockLib.inf
> >>@@ -30,6 +30,7 @@
> >>    MdePkg/MdePkg.dec
> >>    EmbeddedPkg/EmbeddedPkg.dec
> >>    OpenPlatformPkg/OpenPlatformPkg.dec
> >>+  OpenPlatformPkg/Platforms/Hisilicon/D03/D03.dec
> >>    OpenPlatformPkg/Chips/Hisilicon/HisiPkg.dec
> >>  [LibraryClasses]
> >>@@ -41,6 +42,7 @@
> >>    TimerLib
> >>  # Use EFiAtRuntime to check stage
> >>    UefiRuntimeLib
> >>+  CpldIoLib
> >>    EfiTimeBaseLib
> >>  [Pcd]
> >>-- 
> >>1.9.1
> >>
>