From: "Leif Lindholm" <quic_llindhol@quicinc.com>
To: <devel@edk2.groups.io>, <nhi@os.amperecomputing.com>
Cc: <chuong@os.amperecomputing.com>, <rebecca@os.amperecomputing.com>
Subject: Re: [edk2-devel] [edk2-platforms][PATCH 1/5] AmpereAltraPkg/DwI2cLib: Add SmbusRead() function
Date: Thu, 1 Aug 2024 16:14:00 +0100 [thread overview]
Message-ID: <ZqumOCamVgWCj0NL@qc-i7.hemma.eciton.net> (raw)
In-Reply-To: <20240801093618.191274-2-nhi@os.amperecomputing.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
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 <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
> )
> {
> 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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
next prev parent reply other threads:[~2024-08-01 15:14 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-08-01 9:36 [edk2-devel] [edk2-platforms][PATCH 0/5] Add IPMI SSIF support Nhi Pham via groups.io
2024-08-01 9:36 ` [edk2-devel] [edk2-platforms][PATCH 1/5] AmpereAltraPkg/DwI2cLib: Add SmbusRead() function Nhi Pham via groups.io
2024-08-01 15:14 ` Leif Lindholm [this message]
2024-08-05 3:44 ` Nhi Pham via groups.io
2024-08-01 15:18 ` Leif Lindholm
2024-08-05 3:46 ` Nhi Pham via groups.io
2024-08-01 9:36 ` [edk2-devel] [edk2-platforms][PATCH 2/5] AmpereSiliconPkg: Define PCDs for SMBUS and BMC Nhi Pham via groups.io
2024-08-01 15:02 ` Leif Lindholm
2024-08-05 3:18 ` Nhi Pham via groups.io
2024-08-01 9:36 ` [edk2-devel] [edk2-platforms][PATCH 3/5] AmpereAltraPkg: Add SmbusHc PEI and DXE drivers Nhi Pham via groups.io
2024-08-01 9:36 ` [edk2-devel] [edk2-platforms][PATCH 4/5] JadePkg: Add PlatformBmcReadyLib to support BMC ready check Nhi Pham via groups.io
2024-08-01 9:36 ` [edk2-devel] [edk2-platforms][PATCH 5/5] Ampere/Jade: Enable IPMI SSIF Nhi Pham via groups.io
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZqumOCamVgWCj0NL@qc-i7.hemma.eciton.net \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox