Re: [edk2-devel] [edk2-platforms][PATCH 1/5] AmpereAltraPkg/DwI2cLib: Add SmbusRead() function

["Nhi Pham via groups.io" <nhi=os.amperecomputing.com@groups.io>]

On 8/1/2024 10:14 PM, Leif Lindholm wrote:
> On Thu, Aug 01, 2024 at 16:36:14 +0700, Nhi Pham via groups.io wrote:
>> This adds the SmbusRead() function designed for SMBUS transaction to
>> support the extraction of the data lenth byte from the initial byte of
> 
> length

I will fix, thanks.

> 
>> the SMBUS Block Read, allowing the I2C master to accurately read the
>> SMBUS response with the exact data length. This addresses the issue
>> where the SmbusLib:SmBusReadBlock() function consistently reads a
>> 32-byte block of data.
> 
> Doh, I managed to start reviewing on 2/5 instead 1/5.
> Regardless, since this is new code, please update to controller/target
> terminology to match current versions of protocol specifications,
> throughout the set.

I will update, thanks.

> 
>> Signed-off-by: Nhi Pham <nhi@os.amperecomputing.com>
>> ---
>>   Silicon/Ampere/AmpereAltraPkg/Include/Library/I2cLib.h    |  31 +++++
>>   Silicon/Ampere/AmpereAltraPkg/Library/DwI2cLib/DwI2cLib.c | 137 +++++++++++++++++++-
>>   2 files changed, 163 insertions(+), 5 deletions(-)
>>
>> diff --git a/Silicon/Ampere/AmpereAltraPkg/Include/Library/I2cLib.h b/Silicon/Ampere/AmpereAltraPkg/Include/Library/I2cLib.h
>> index f13794171029..d460f49efccb 100644
>> --- a/Silicon/Ampere/AmpereAltraPkg/Include/Library/I2cLib.h
>> +++ b/Silicon/Ampere/AmpereAltraPkg/Include/Library/I2cLib.h
>> @@ -65,6 +65,37 @@ I2cRead (
>>     IN OUT UINT32 *ReadLength
>>     );
>>   
>> +/**
>> +  SMBUS block read.
>> +
>> +  @param[in]     Bus          I2C bus Id.
>> +  @param[in]     SlaveAddr    The address of slave device on the bus.
>> +  @param[in]     BufCmd       Buffer where to send the command.
>> +  @param[in]     CmdLength    Length of BufCmd.
>> +  @param[in,out] Buf          Buffer where to put the read data to.
>> +  @param[in,out] ReadLength   Pointer to length of buffer.
>> +  @param[in]     PecCheck     If Packet Error Code (PEC) checking is required for this operation.
>> +
>> +  @retval EFI_SUCCESS            Read successfully.
>> +  @retval EFI_INVALID_PARAMETER  A parameter is invalid.
>> +  @retval EFI_UNSUPPORTED        The bus is not supported.
>> +  @retval EFI_NOT_READY          The device/bus is not ready.
>> +  @retval EFI_TIMEOUT            Timeout when transferring data.
>> +  @retval EFI_CRC_ERROR          There are errors on receiving data.
>> +
>> +**/
>> +EFI_STATUS
>> +EFIAPI
>> +SmbusRead (
>> +  IN     UINT32  Bus,
>> +  IN     UINT32  SlaveAddr,
>> +  IN     UINT8   *BufCmd,
>> +  IN     UINT32  CmdLength,
>> +  IN OUT UINT8   *Buf,
>> +  IN OUT UINT32  *ReadLength,
>> +  IN     BOOLEAN PecCheck
>> +  );
>> +
>>   /**
>>    Setup new transaction with I2C slave device.
>>   
>> diff --git a/Silicon/Ampere/AmpereAltraPkg/Library/DwI2cLib/DwI2cLib.c b/Silicon/Ampere/AmpereAltraPkg/Library/DwI2cLib/DwI2cLib.c
>> index 669ba2ea98a4..9e52ae69e7cd 100644
>> --- a/Silicon/Ampere/AmpereAltraPkg/Library/DwI2cLib/DwI2cLib.c
>> +++ b/Silicon/Ampere/AmpereAltraPkg/Library/DwI2cLib/DwI2cLib.c
>> @@ -337,6 +337,11 @@ I2cWaitTxData (
>>         DEBUG ((DEBUG_ERROR, "%a: Timeout waiting for TX buffer available\n", __FUNCTION__));
>>         return EFI_TIMEOUT;
>>       }
>> +
>> +    if ((I2cCheckErrors (Bus) & DW_IC_INTR_TX_ABRT) != 0) {
>> +      return EFI_ABORTED;
>> +    }
>> +
>>       MicroSecondDelay (mI2cBusList[Bus].PollingTime);
>>     }
>>   
>> @@ -542,13 +547,61 @@ InternalI2cWrite (
>>     return Status;
>>   }
>>   
>> +EFI_STATUS
>> +InternalSmbusReadDataLength (
>> +  UINT32  Bus,
>> +  UINT32 *Length
>> +  )
>> +{
>> +  EFI_STATUS Status;
>> +  UINTN      Base;
>> +  UINT32     CmdSend;
>> +
>> +  Base = mI2cBusList[Bus].Base;
>> +
>> +  CmdSend = DW_IC_DATA_CMD_CMD;
>> +  MmioWrite32 (Base + DW_IC_DATA_CMD, CmdSend);
>> +  I2cSync ();
>> +
>> +  if (I2cCheckErrors (Bus) != 0) {
>> +    DEBUG ((DEBUG_ERROR, "%a: Sending reading command error\n", __func__));
>> +    return EFI_CRC_ERROR;
>> +  }
>> +
>> +  Status = I2cWaitRxData (Bus);
>> +  if (EFI_ERROR (Status)) {
>> +    DEBUG ((DEBUG_ERROR,
>> +      "%a: Reading Smbus data length failed to wait data\n",
>> +      __func__
>> +      ));
>> +
>> +    if (Status != EFI_ABORTED) {
>> +      MmioWrite32 (Base + DW_IC_DATA_CMD, DW_IC_DATA_CMD_STOP);
>> +      I2cSync ();
>> +    }
>> +
>> +    return Status;
>> +  }
>> +
>> +  *Length = MmioRead32 (Base + DW_IC_DATA_CMD) & DW_IC_DATA_CMD_DAT_MASK;
>> +  I2cSync ();
>> +
>> +  if (I2cCheckErrors (Bus) != 0) {
>> +    DEBUG ((DEBUG_ERROR, "%a: Sending reading command error\n", __func__));
>> +    return EFI_CRC_ERROR;
>> +  }
>> +  return EFI_SUCCESS;
>> +}
>> +
>>   EFI_STATUS
>>   InternalI2cRead (
>>     UINT32  Bus,
>> -  UINT8  *BufCmd,
>> -  UINT32 CmdLength,
>> -  UINT8  *Buf,
>> -  UINT32 *Length
>> +  UINT8   *BufCmd,
>> +  UINT32  CmdLength,
>> +  UINT8   *Buf,
>> +  UINT32  *Length,
>> +  BOOLEAN IsSmbus,
> 
> Can this really change on a per-transaction basis?
> If not, can it move to a state held in DW_I2C_CONTEXT_T?
> 
>> +  BOOLEAN PecCheck
> 
> I guess the same question as above really.
> 
> /
>      Leif

That makes sense, thanks Leif. I will pass IsSmbus and PecCheck in 
I2cProbe() and cache in the DW_I2C_CONTEXT_T.

Regards,
Nhi


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#120220): https://edk2.groups.io/g/devel/message/120220
Mute This Topic: https://groups.io/mt/107662232/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-