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

["Leif Lindholm" <quic_llindhol@quicinc.com>]

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
> 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.

This change also changes the API to pass the PecCheck parameter,
without actuaklly using it for anyone. Regardless of my previous
comment, I don't think that's very useful.
So if you can't move it to the struct, can you move it to the later
patch instead?

/
    Leif

> 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,
> +  BOOLEAN PecCheck
>    )
>  {
>    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 (#120182): https://edk2.groups.io/g/devel/message/120182
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]
-=-=-=-=-=-=-=-=-=-=-=-