Re: [PATCH edk2-platforms v1 14/38] Silicon/Hisilicon/D06: Fix I2C enable fail issue for D06

[Ming <ming.huang@linaro.org>]

在 8/3/2018 6:40 PM, Leif Lindholm 写道:
> On Tue, Jul 24, 2018 at 03:08:58PM +0800, Ming Huang wrote:
>> From: shaochangliang <shaochangliang@huawei.com>
>>
>> I2C may enable failed in D06, so retry I2C enable while
>> enable failed.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: shaochangliang <shaochangliang@huawei.com>
>> Signed-off-by: Ming Huang <ming.huang@linaro.org>
>> Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
>> ---
>>  Silicon/Hisilicon/Library/I2CLib/I2CLib.c | 22 ++++++++++++--------
>>  1 file changed, 13 insertions(+), 9 deletions(-)
>>
>> diff --git a/Silicon/Hisilicon/Library/I2CLib/I2CLib.c b/Silicon/Hisilicon/Library/I2CLib/I2CLib.c
>> index b5b388d756..ecd2f07c4d 100644
>> --- a/Silicon/Hisilicon/Library/I2CLib/I2CLib.c
>> +++ b/Silicon/Hisilicon/Library/I2CLib/I2CLib.c
>> @@ -83,6 +83,7 @@ I2C_Enable(UINT32 Socket,UINT8 Port)
>>  {
>>      I2C0_ENABLE_U           I2cEnableReg;
> 
> Ugh, indentation is incorrect in the original.
> Can you correct that in a patch preceding this one?
> 

OK, I will correct it in v2.

>>      I2C0_ENABLE_STATUS_U    I2cEnableStatusReg;
>> +    UINT32                  ulTimeCnt = I2C_READ_TIMEOUT;
> 
> What is ul?
> 

It is wrong name and will be corrected in v2.

>>  
>>      UINTN Base = GetI2cBase(Socket, Port);
>>  
>> @@ -91,16 +92,19 @@ I2C_Enable(UINT32 Socket,UINT8 Port)
>>      I2cEnableReg.bits.enable = 1;
>>      I2C_REG_WRITE(Base + I2C_ENABLE_OFFSET, I2cEnableReg.Val32);
>>  
>> -
>> -    I2C_REG_READ(Base + I2C_ENABLE_STATUS_OFFSET, I2cEnableStatusReg.Val32);
>> -    if (1 == I2cEnableStatusReg.bits.ic_en)
>> +    do
>>      {
> 
> Move that brace up to the previous line.
> 
>> -        return EFI_SUCCESS;
>> -    }
>> -    else
>> -    {
>> -        return EFI_DEVICE_ERROR;
>> -    }
>> +        I2C_Delay(10000);
> 
> Why 10000?
> Do we need a MemoryFence ()?
> 

The delay is need for I2C. This is a empirical value.
MemoryFance is no need. I will add this as comment.

>> +
>> +        ulTimeCnt--;
>> +        I2C_REG_READ(Base + I2C_ENABLE_STATUS_OFFSET, I2cEnableStatusReg.Val32);
>> +        if (0 == ulTimeCnt)
>> +        {
> 
> Move that brace up to previous line.
> 
>> +            return EFI_DEVICE_ERROR;
>> +        }
>> +    }while (0 == I2cEnableStatusReg.bits.ic_en);
> 
> Space after }
> 
> /
>     Leif
> 
>> +
>> +    return EFI_SUCCESS;
>>  }
>>  
>>  void I2C_SetTarget(UINT32 Socket,UINT8 Port,UINT32 I2cDeviceAddr)
>> -- 
>> 2.17.0
>>