| Message ID | 20180724070922.63362-15-ming.huang@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Upload for D06 platform | expand |
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? > I2C0_ENABLE_STATUS_U I2cEnableStatusReg; > + UINT32 ulTimeCnt = I2C_READ_TIMEOUT; What is ul? > > 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 ()? > + > + 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 > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
在 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 >>
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; I2C0_ENABLE_STATUS_U I2cEnableStatusReg; + UINT32 ulTimeCnt = I2C_READ_TIMEOUT; 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 { - return EFI_SUCCESS; - } - else - { - return EFI_DEVICE_ERROR; - } + I2C_Delay(10000); + + ulTimeCnt--; + I2C_REG_READ(Base + I2C_ENABLE_STATUS_OFFSET, I2cEnableStatusReg.Val32); + if (0 == ulTimeCnt) + { + return EFI_DEVICE_ERROR; + } + }while (0 == I2cEnableStatusReg.bits.ic_en); + + return EFI_SUCCESS; } void I2C_SetTarget(UINT32 Socket,UINT8 Port,UINT32 I2cDeviceAddr)