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
> 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.
> Signed-off-by: Nhi Pham <n...@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
> )
> {
> EFI_STATUS Status;
> @@ -559,6 +612,7 @@ InternalI2cRead (
> UINTN Count;
> UINTN ReadCount;
> UINTN WriteCount;
> + UINT32 ResponseLen;
>
> Status = EFI_SUCCESS;
> Base = mI2cBusList[Bus].Base;
> @@ -601,6 +655,36 @@ InternalI2cRead (
> }
>
> WriteCount = 0;
> + if (IsSmbus) {
> + //
> + // Read Smbus Data Length, first byte of the Smbus response data.
> + //
> + Status = InternalSmbusReadDataLength (Bus, &ResponseLen);
> + if (EFI_ERROR (Status)) {
> + DEBUG ((DEBUG_ERROR, "%a: InternalSmbusReadDataLength failed\n",
> __func__));
> + goto Exit;
> + }
> +
> + WriteCount++;
> + Buf[ReadCount++] = ResponseLen;
> +
> + //
> + // Abort the transaction when the requested length is shorter than the
> actual response data
> + // or if there is no response data when PEC disabled.
> + //
> + if ((*Length < (ResponseLen + 2)) || (!PecCheck && ResponseLen == 0)) {
> + MmioWrite32 (Base + DW_IC_DATA_CMD, DW_IC_DATA_CMD_CMD |
> DW_IC_DATA_CMD_STOP);
> + I2cSync ();
> + Status = EFI_INVALID_PARAMETER;
> + goto Exit;
> + }
> +
> + *Length = ResponseLen + 1; // Response Data Length + 8-bit Byte Count
> field
> + if (PecCheck) {
> + *Length += 1; // ++ 8-bit PEC field
> + }
> + }
> +
> while ((*Length - ReadCount) != 0) {
> TxLimit = mI2cBusList[Bus].TxFifo - MmioRead32 (Base + DW_IC_TXFLR);
> RxLimit = mI2cBusList[Bus].RxFifo - MmioRead32 (Base + DW_IC_RXFLR);
> @@ -742,7 +826,50 @@ I2cRead (
>
> I2cSetSlaveAddr (Bus, SlaveAddr);
>
> - return InternalI2cRead (Bus, BufCmd, CmdLength, Buf, ReadLength);
> + return InternalI2cRead (Bus, BufCmd, CmdLength, Buf, ReadLength, FALSE,
> FALSE);
> +}
> +
> +/**
> + 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
> + )
> +{
> + if (Bus >= AC01_I2C_MAX_BUS_NUM
> + || Buf == NULL
> + || ReadLength == NULL)
> + {
> + return EFI_INVALID_PARAMETER;
> + }
> +
> + I2cSetSlaveAddr (Bus, SlaveAddr);
> +
> + return InternalI2cRead (Bus, BufCmd, CmdLength, Buf, ReadLength, TRUE,
> PecCheck);
> }
>
> /**
> --
> 2.25.1
>
>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#120181): https://edk2.groups.io/g/devel/message/120181
Mute This Topic: https://groups.io/mt/107662232/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
