From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2a00:1450:4864:20::341; helo=mail-wm1-x341.google.com; envelope-from=leif.lindholm@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-wm1-x341.google.com (mail-wm1-x341.google.com [IPv6:2a00:1450:4864:20::341]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id EE7BA21189FB5 for ; Thu, 1 Nov 2018 10:53:21 -0700 (PDT) Received: by mail-wm1-x341.google.com with SMTP id a8-v6so2017362wmf.1 for ; Thu, 01 Nov 2018 10:53:21 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=mLIglOvW4/IoSJr+AyCgPwpHolBo0y5EuCWFKWOSJRs=; b=MkjnMgh6mzb742VFr46KDrV2VVo9qZvTG5jD1gDGNWEsOLd61W0VwTINIEH4mGwcW3 nRdT1jrkUuKx/HMxaO3K6Talgtz3XEGws8y+1N8PBuZ6OwVmeS0Z3dR7WKENrV54LhQ7 ft1oykQY8uGr+Ti+IwnBp59pYB8xJTd3ZU+T0= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=mLIglOvW4/IoSJr+AyCgPwpHolBo0y5EuCWFKWOSJRs=; b=KToRPKo9PZoLkUyDQav36mpKgSDAY+9HUTvnGk3x0XJuonQI3ek/Z2BwaYrgk4psb2 WP3XS5816jxVr7UE8M16twlM7uqoEycyydlddRqXxZVS0KGXe4KcZm6mC/xM9mLbgKId hOuPTPEYNPeKxPy8Ao4quD6d8itneSxyxo156ml3T9sk/XA9W8KL0SUudTuEoZoV2Ha7 +D9c0hnBADmcbfzUBPdLAwXWXzrfbYpuTjjGbvN7kvZTZN99xqYjjaCSImZbZ1A3eYpg ILks2XkJ4TGoYP1q0AyJVGPMmNP0YSpGi79oap0RQ07lMvYsyQUWtcZbUlKqnKZkTtA+ bJJg== X-Gm-Message-State: AGRZ1gJP2mOujnA7lCQqEI1hk3dRE0Ne+KIlgUJzkNtYqsVPTWrIv9Uc 4a/8cN6vhTBpCMJQuO9WMY1qbWFEk34= X-Google-Smtp-Source: AJdET5f11mNmOxq0SH4roX30HM4QVeU96K+Jn4Nc1k6LjmQKI5Rs3bdvQQkw2b81S1ynGrjIn6ilag== X-Received: by 2002:a1c:607:: with SMTP id 7-v6mr6176908wmg.103.1541094800035; Thu, 01 Nov 2018 10:53:20 -0700 (PDT) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id j72-v6sm20923654wrj.7.2018.11.01.10.53.18 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 01 Nov 2018 10:53:18 -0700 (PDT) Date: Thu, 1 Nov 2018 17:53:16 +0000 From: Leif Lindholm To: Chris Co Cc: "edk2-devel@lists.01.org" , Ard Biesheuvel , Michael D Kinney Message-ID: <20181101175315.tfg4zr742bbxmzbh@bivouac.eciton.net> References: <20180921082542.35768-1-christopher.co@microsoft.com> <20180921082542.35768-7-christopher.co@microsoft.com> MIME-Version: 1.0 In-Reply-To: <20180921082542.35768-7-christopher.co@microsoft.com> User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [PATCH edk2-platforms 06/27] Silicon/NXP: Add I2C library support for i.MX platforms X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 01 Nov 2018 17:53:22 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Fri, Sep 21, 2018 at 08:25:57AM +0000, Chris Co wrote: > This adds support for I2C controller on NXP i.MX platforms. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Christopher Co > Cc: Ard Biesheuvel > Cc: Leif Lindholm > Cc: Michael D Kinney > --- > Silicon/NXP/iMXPlatformPkg/Include/iMXI2cLib.h | 162 +++++++ > Silicon/NXP/iMXPlatformPkg/Library/iMXI2cLib/iMXI2cLib.c | 487 ++++++++++++++++++++ > Silicon/NXP/iMXPlatformPkg/Library/iMXI2cLib/iMXI2cLib.inf | 35 ++ > 3 files changed, 684 insertions(+) > > diff --git a/Silicon/NXP/iMXPlatformPkg/Include/iMXI2cLib.h b/Silicon/NXP/iMXPlatformPkg/Include/iMXI2cLib.h > new file mode 100644 > index 000000000000..de8a1616fe1b > --- /dev/null > +++ b/Silicon/NXP/iMXPlatformPkg/Include/iMXI2cLib.h > @@ -0,0 +1,162 @@ > +/** @file > +* > +* Copyright (c) 2018 Microsoft Corporation. All rights reserved. > +* > +* This program and the accompanying materials > +* are licensed and made available under the terms and conditions of the BSD License > +* which accompanies this distribution. The full text of the license may be found at > +* http://opensource.org/licenses/bsd-license.php > +* > +* THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > +* WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > +* > +**/ > + > +#ifndef _IMX_I2C_H_ > +#define _IMX_I2C_H_ > + > +#pragma pack(push, 1) I don't see where below this has any effect. If not required, please drop. > + > +typedef union { > + UINT16 AsUint16; Not super happy with this name. Could it be just "Raw"? (That pattern is used frequently in EDK2.) Same applies to similar situations below. > + struct { > + UINT16 Reserved0 : 1; > + UINT16 ADR : 7; > + UINT16 Reserved1 : 8; > + }; > +} IMX_I2C_IADR_REG; IADR is the name I guess, but REG should ideally be fully written out as REGISTER. > + > +typedef union { > + UINT16 AsUint16; > + struct { > + UINT16 IC : 6; > + UINT16 Reserved0 : 10; > + }; > +} IMX_I2C_IFDR_REG; > + > +typedef union { > + UINT16 AsUint16; > + struct { > + UINT16 Reserved0 : 2; > + UINT16 RSTA : 1; > + UINT16 TXAK : 1; > + UINT16 MTX : 1; > + UINT16 MSTA : 1; > + UINT16 IIEN : 1; > + UINT16 IEN : 1; > + UINT16 Reserved1 : 8; > + }; > +} IMX_I2C_I2CR_REG; > + > +#define IMX_I2C_I2SR_RXAK 0x0001 > +#define IMX_I2C_I2SR_IIF 0x0002 > +#define IMX_I2C_I2SR_SRW 0x0004 > +#define IMX_I2C_I2SR_IAL 0x0010 > +#define IMX_I2C_I2SR_IBB 0x0020 > +#define IMX_I2C_I2SR_IAAS 0x0040 > +#define IMX_I2C_I2SR_ICF 0x0080 > + > +typedef union { > + UINT16 AsUint16; > + struct { > + UINT16 RXAK : 1; > + UINT16 IIF : 1; > + UINT16 SRW : 1; > + UINT16 Reserved0 : 1; > + UINT16 IAL : 1; > + UINT16 IBB : 1; > + UINT16 IAAS : 1; > + UINT16 ICF : 1; > + UINT16 Reserved1 : 8; > + }; > +} IMX_I2C_I2SR_REG; > + > +typedef union { > + UINT16 AsUint16; > + struct { > + UINT16 DATA : 8; > + UINT16 Reserved0 : 8; > + }; > +} IMX_I2C_I2DR_REG; > + > +typedef struct { > + IMX_I2C_IADR_REG IADR; > + UINT16 Pad0; > + IMX_I2C_IFDR_REG IFDR; > + UINT16 Pad1; > + IMX_I2C_I2CR_REG I2CR; > + UINT16 Pad2; > + IMX_I2C_I2DR_REG I2SR; > + UINT16 Pad3; > + IMX_I2C_I2DR_REG I2DR; > + UINT16 Pad4; > +} IMX_I2C_REGS; And here, REGISTERS. > + > +#pragma pack(pop) > + > +typedef struct { > + UINT32 ControllerAddress; > + UINT32 ControllerSlaveAddress; > + UINT32 ReferenceFreq; > + UINT32 TargetFreq; > + UINT32 SlaveAddress; > + UINT32 TimeoutInUs; > +} IMX_I2C_CONFIG; > + > +typedef struct { > + UINT32 Divider; > + UINT32 IC; Please expand to CamelCase. It's not obvious what IC stands for. > +} IMX_I2C_DIVIDER; > + > +/** > + Perform I2C read operation. > + > + The iMXI2cRead perform I2C read operation by programming the I2C controller. > + The caller is responsible to provide I2C controller configuration. > + > + @param[in] IMX_I2C_CONFIG Pointer to structure containing the targeted > + I2C controller to be used for I2C operation. > + @param[in] RegisterAddress Targeted device register address to start read. > + @param[out] ReadBufferPtr Caller supplied buffer that would be written > + into with data from the read operation. > + @param[in] ReadBufferSize Size of caller supplied buffer. > + > + @retval RETURN_SUCCESS I2C Read operation succeeded. > + @retval RETURN_DEVICE_ERROR The I2C device is not functioning correctly. > + > +**/ > +RETURN_STATUS > +iMXI2cRead ( > + IN IMX_I2C_CONFIG *I2cConfigPtr, > + IN UINT8 RegisterAddress, > + OUT UINT8 *ReadBufferPtr, > + IN UINT32 ReadBufferSize > + ); > + > +/** > + Perform I2C write operation. > + > + The iMXI2cWrite perform I2C write operation by programming the I2C > + controller. The caller is responsible to provide I2C controller > + configuration. > + > + @param[in] IMX_I2C_CONFIG Pointer to structure containing the targeted > + I2C controller to be used for I2C operation. > + @param[in] RegisterAddress Targeted device register address to start write. > + @param[out] WriteBufferPtr Caller supplied buffer that contained data that > + would be read from for I2C write operation. > + @param[in] WriteBufferSize Size of caller supplied buffer. > + > + @retval RETURN_SUCCESS I2C Write operation succeeded. > + @retval RETURN_DEVICE_ERROR The I2C device is not functioning correctly. > + > +**/ > +RETURN_STATUS > +iMXI2cWrite ( > + IN IMX_I2C_CONFIG *I2cConfigPtr, > + IN UINT8 RegisterAddress, > + IN UINT8 *WriteBufferPtr, > + IN UINT32 WriteBufferSize > + ); > + > +#endif > diff --git a/Silicon/NXP/iMXPlatformPkg/Library/iMXI2cLib/iMXI2cLib.c b/Silicon/NXP/iMXPlatformPkg/Library/iMXI2cLib/iMXI2cLib.c > new file mode 100644 > index 000000000000..6cfe04dba571 > --- /dev/null > +++ b/Silicon/NXP/iMXPlatformPkg/Library/iMXI2cLib/iMXI2cLib.c > @@ -0,0 +1,487 @@ > +/** @file > +* > +* Copyright (c) 2018 Microsoft Corporation. All rights reserved. > +* > +* This program and the accompanying materials > +* are licensed and made available under the terms and conditions of the BSD License > +* which accompanies this distribution. The full text of the license may be found at > +* http://opensource.org/licenses/bsd-license.php > +* > +* THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > +* WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > +* > +**/ > + > +#include > +#include > + > +#include > +#include > +#include > +#include > +#include > + > +#include > + > +IMX_I2C_DIVIDER DividerValue[] = { Global variable should have 'm' prefix if scope is this module or 'g' if external visibility. If only this file, please also make it STATIC. I'm not super happy about these live-coded values, but I guess adding a bunch of defines wouldn't help much. But could you add a comment block explaining roughly what the fields mean? > + { 22, 0x20, }, Please drop the second ',' on all lines - IMX_I2C_DIVIDER only has two structure members. > + { 24, 0x21, }, > + { 26, 0x22, }, > + { 28, 0x23, }, > + { 30, 0x00, }, > + { 32, 0x24, }, > + { 36, 0x25, }, > + { 40, 0x26, }, > + { 42, 0x03, }, > + { 44, 0x27, }, > + { 48, 0x28, }, > + { 52, 0x05, }, > + { 56, 0x29, }, > + { 60, 0x06, }, > + { 64, 0x2A, }, > + { 72, 0x2B, }, > + { 80, 0x2C, }, > + { 88, 0x09, }, > + { 96, 0x2D, }, > + { 104, 0x0A, }, > + { 112, 0x2E, }, > + { 128, 0x2F, }, > + { 144, 0x0C, }, > + { 160, 0x30, }, > + { 192, 0x31, }, > + { 224, 0x32, }, > + { 240, 0x0F, }, > + { 256, 0x33, }, > + { 288, 0x10, }, > + { 320, 0x34, }, > + { 384, 0x35, }, > + { 448, 0x36, }, > + { 480, 0x13, }, > + { 512, 0x37, }, > + { 576, 0x14, }, > + { 640, 0x38, }, > + { 768, 0x39, }, > + { 896, 0x3A, }, > + { 960, 0x17, }, > + { 1024, 0x3B, }, > + { 1152, 0x18, }, > + { 1280, 0x3C, }, > + { 1536, 0x3D, }, > + { 1792, 0x3E, }, > + { 1920, 0x1B, }, > + { 2048, 0x3F, }, > + { 2304, 0x1C, }, > + { 2560, 0x1D, }, > + { 3072, 0x1E, }, > + { 3840, 0x1F, }, > +}; > + > +#define DIVIDER_VALUE_TOTAL (sizeof(DividerValue) / sizeof(DividerValue[0])) Can use BaseLib ARRAY_SIZE macro instead of adding this custom one. > + > +BOOLEAN > +iMXI2cWaitStatusSet ( > + IN IMX_I2C_CONFIG *I2cConfigPtr, > + IN UINT16 StatusBits > + ) > +{ > + IMX_I2C_REGS *I2cRegsPtr; > + UINT32 Counter; > + IMX_I2C_I2SR_REG I2srReg; Same as for previous patch - no need to name the variable after the register it's initialised from. > + > + Counter = I2cConfigPtr->TimeoutInUs; > + I2cRegsPtr = (IMX_I2C_REGS*)I2cConfigPtr->ControllerAddress; > + while (Counter) { > + I2srReg = (IMX_I2C_I2SR_REG)MmioRead16 ((UINTN)&I2cRegsPtr->I2SR); > + if ((I2srReg.AsUint16 & StatusBits) == StatusBits) { > + return TRUE; > + } > + MicroSecondDelay (1); > + --Counter; > + } > + > + return FALSE; > +} > + > +BOOLEAN > +iMXI2cWaitStatusUnSet ( > + IN IMX_I2C_CONFIG *I2cConfigPtr, > + IN UINT16 StatusBits > + ) > +{ > + IMX_I2C_REGS *I2cRegsPtr; > + UINT32 Counter; > + IMX_I2C_I2SR_REG I2srReg; > + > + Counter = I2cConfigPtr->TimeoutInUs; > + I2cRegsPtr = (IMX_I2C_REGS*)I2cConfigPtr->ControllerAddress; > + while (Counter) { > + I2srReg = (IMX_I2C_I2SR_REG)MmioRead16 ((UINTN)&I2cRegsPtr->I2SR); > + if ((I2srReg.AsUint16 & StatusBits) == 0) { > + return TRUE; > + } > + MicroSecondDelay (1); > + --Counter; > + } > + > + return FALSE; > +} > + > +BOOLEAN > +iMXI2cSendByte ( > + IN IMX_I2C_CONFIG *I2cConfigPtr, > + IN UINT8 Data > + ) > +{ > + IMX_I2C_REGS *I2cRegsPtr; > + UINT32 Counter; > + IMX_I2C_I2SR_REG I2srReg; > + UINT16 SendData; > + > + SendData = Data; > + Counter = I2cConfigPtr->TimeoutInUs; > + I2cRegsPtr = (IMX_I2C_REGS*)I2cConfigPtr->ControllerAddress; > + > + // Clear status and transfer byte > + MmioWrite16 ((UINTN)&I2cRegsPtr->I2SR, 0); > + MmioWrite16 ((UINTN)&I2cRegsPtr->I2DR, SendData); > + > + while (Counter) { > + I2srReg = (IMX_I2C_I2SR_REG)MmioRead16 ((UINTN)&I2cRegsPtr->I2SR); > + if (I2srReg.IIF == 1) { What does it mean if it's == 1? Can that 1 be given a descriptively named #define? > + return TRUE; > + } else if (I2srReg.IAL == 1) { What does it mean if it's == 1? Can that 1 be given a descriptively named #define? > + DEBUG ((DEBUG_ERROR, "iMXI2cSendByte: fail 0x%04x\n", I2srReg.AsUint16)); > + return FALSE; > + } > + MicroSecondDelay (1); > + --Counter; > + } > + > + DEBUG ((DEBUG_ERROR, "iMXI2cSendByte: Fail timeout\n")); > + return FALSE; > +} > + > +RETURN_STATUS > +iMXI2cSetupController ( > + IN IMX_I2C_CONFIG *I2cConfigPtr > + ) > +{ > + IMX_I2C_REGS *I2cRegsPtr; > + UINT32 Divider; > + UINT32 DividerCount; > + UINT32 IfdrDiv; > + IMX_I2C_I2CR_REG I2crReg; > + > + I2cRegsPtr = (IMX_I2C_REGS *)I2cConfigPtr->ControllerAddress; > + > + // Disable controller and clear any pending interrupt > + MmioWrite16 ((UINTN)&I2cRegsPtr->I2CR, 0); > + MmioWrite16 ((UINTN)&I2cRegsPtr->I2SR, 0); A comment as to which operation does which? > + > + // Setup Divider if reference freq is provided. If no, use value setup by > + // 1st boot loader first > + if (I2cConfigPtr->ReferenceFreq != 0) { > + IfdrDiv = 0; > + Divider = I2cConfigPtr->ReferenceFreq / I2cConfigPtr->TargetFreq; > + > + for (DividerCount = 0; DividerCount < DIVIDER_VALUE_TOTAL; ++DividerCount) { > + if (DividerValue[DividerCount].Divider >= Divider) { > + DEBUG (( > + DEBUG_INFO, > + "iMXI2cSetupController: Divider %d IC 0x%02x\n", > + DividerValue[DividerCount].Divider, > + DividerValue[DividerCount].IC)); > + IfdrDiv = DividerValue[DividerCount].IC; > + break; > + } > + } > + > + if (IfdrDiv == 0) { > + DEBUG (( > + DEBUG_ERROR, > + "iMXI2cSetupController: could not find Divider for %d\n", > + Divider)); > + return RETURN_INVALID_PARAMETER; > + } > + > + MmioWrite16 ((UINTN)&I2cRegsPtr->IFDR, IfdrDiv); > + } > + > + // Setup slave address > + MmioWrite16 ((UINTN)&I2cRegsPtr->IADR, > + (I2cConfigPtr->ControllerSlaveAddress << 1)); Why the shift by 1? Could this use a macro? > + > + // Enable controller and set to master mode. > + I2crReg = (IMX_I2C_I2CR_REG)MmioRead16 ((UINTN)&I2cRegsPtr->I2CR); > + > + // This bit must be set before any other I2C_I2CR bits have an effect Why? > + I2crReg.IEN = 1; > + MmioWrite16 ((UINTN)&I2cRegsPtr->I2CR, I2crReg.AsUint16); What does this write do? > + MicroSecondDelay (100); > + > + MmioWrite16 ((UINTN)&I2cRegsPtr->I2SR, 0); Why this write of 0? > + > + // Wait for bus to be idle > + if (iMXI2cWaitStatusUnSet (I2cConfigPtr, IMX_I2C_I2SR_IBB) == FALSE) { > + DEBUG ((DEBUG_ERROR, "iMXI2cGenerateStart: Controller remains busy\n")); > + return RETURN_DEVICE_ERROR; > + } > + > + I2crReg.MSTA = 1; What does 1 mean? > + MmioWrite16 ((UINTN)&I2cRegsPtr->I2CR, I2crReg.AsUint16); What does this write do? > + > + // Now wait for bus to be busy > + if (iMXI2cWaitStatusSet (I2cConfigPtr, IMX_I2C_I2SR_IBB) == FALSE) { > + DEBUG ((DEBUG_ERROR, "iMXI2cGenerateStart: Controller remains idle\n")); > + return RETURN_DEVICE_ERROR; > + } > + > + return RETURN_SUCCESS; > +} > + > +RETURN_STATUS > +iMXI2cGenerateStart ( > + IN IMX_I2C_CONFIG *I2cConfigPtr, > + IN UINT8 RegisterAddress > + ) > +{ > + IMX_I2C_REGS *I2cRegsPtr; > + IMX_I2C_I2CR_REG I2crReg; > + BOOLEAN Result; > + RETURN_STATUS Status; > + > + I2cRegsPtr = (IMX_I2C_REGS*)I2cConfigPtr->ControllerAddress; > + Status = iMXI2cSetupController (I2cConfigPtr); > + if (RETURN_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "iMXI2cGenerateStart: Fail to setup controller %r\n", > + Status)); > + return Status; > + } > + > + // Set controller to transmit mode > + I2crReg = (IMX_I2C_I2CR_REG)MmioRead16 ((UINTN)&I2cRegsPtr->I2CR); > + I2crReg.MTX = 1; > + MmioWrite16 ((UINTN)&I2cRegsPtr->I2CR, I2crReg.AsUint16); > + > + Result = iMXI2cSendByte (I2cConfigPtr, (I2cConfigPtr->SlaveAddress << 1)); Shift? Macro? > + if (Result == FALSE) { > + DEBUG (( > + DEBUG_ERROR, > + "iMXI2cGenerateStart: Slave address transfer fail 0x%04x\n", > + MmioRead16 ((UINTN)&I2cRegsPtr->I2SR))); > + return RETURN_DEVICE_ERROR; > + } > + > + // Send slave register address > + Result = iMXI2cSendByte (I2cConfigPtr, RegisterAddress); > + if (Result == FALSE) { > + DEBUG (( > + DEBUG_ERROR, > + "iMXI2cGenerateStart: Slave register address transfer fail 0x%04x\n", > + MmioRead16 ((UINTN)&I2cRegsPtr->I2SR))); > + return RETURN_DEVICE_ERROR; > + } > + > + return RETURN_SUCCESS; > +} > + > +RETURN_STATUS > +iMXI2cGenerateStop ( > + IN IMX_I2C_CONFIG *I2cConfigPtr > + ) > +{ > + IMX_I2C_REGS *I2cRegsPtr; > + IMX_I2C_I2CR_REG I2crReg; > + > + I2cRegsPtr = (IMX_I2C_REGS*)I2cConfigPtr->ControllerAddress; > + I2crReg = (IMX_I2C_I2CR_REG)MmioRead16 ((UINTN)&I2cRegsPtr->I2CR); > + I2crReg.MSTA = 0; I'm getting to the point where I am wanting these fields to be given useful names. An awful lot of #defines and comments will be necessary otherwise. > + I2crReg.MTX = 0; > + MmioWrite16 ((UINTN)&I2cRegsPtr->I2CR, I2crReg.AsUint16); > + > + // Bus should go idle > + if (iMXI2cWaitStatusUnSet (I2cConfigPtr, IMX_I2C_I2SR_IBB) == FALSE) { > + DEBUG ((DEBUG_ERROR, "iMXI2cGenerateStop: Controller remains busy\n")); > + return RETURN_DEVICE_ERROR; > + } > + > + return RETURN_SUCCESS; > +} > + > +/** > + Perform I2C read operation. > + > + The iMXI2cRead perform I2C read operation by programming the I2C controller. > + The caller is responsible to provide I2C controller configuration. > + > + @param[in] IMX_I2C_CONFIG Pointer to structure containing the targeted > + I2C controller to be used for I2C operation. > + @param[in] RegisterAddress Targeted device register address to start read. > + @param[out] ReadBufferPtr Caller supplied buffer that would be written > + into with data from the read operation. > + @param[in] ReadBufferSize Size of caller supplied buffer. > + > + @retval RETURN_SUCCESS I2C Read operation succeeded. > + @retval RETURN_DEVICE_ERROR The I2C device is not functioning correctly. > + > +**/ > +RETURN_STATUS > +iMXI2cRead ( > + IN IMX_I2C_CONFIG *I2cConfigPtr, > + IN UINT8 RegisterAddress, > + OUT UINT8 *ReadBufferPtr, > + IN UINT32 ReadBufferSize > + ) > +{ > + IMX_I2C_REGS *I2cRegsPtr; > + IMX_I2C_I2CR_REG I2crReg; > + BOOLEAN Result; > + RETURN_STATUS Status; > + > + I2cRegsPtr = (IMX_I2C_REGS*)I2cConfigPtr->ControllerAddress; > + Status = iMXI2cGenerateStart (I2cConfigPtr, RegisterAddress); > + if (RETURN_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "iMXI2cRead: iMXI2cGenerateStart failed %r\n", Status)); > + goto Exit; > + } > + > + // Send slave address again to begin read > + I2crReg = (IMX_I2C_I2CR_REG)MmioRead16 ((UINTN)&I2cRegsPtr->I2CR); > + I2crReg.RSTA = 1; // Repeated start > + MmioWrite16 ((UINTN)&I2cRegsPtr->I2CR, I2crReg.AsUint16); > + Result = iMXI2cSendByte ( > + I2cConfigPtr, > + (I2cConfigPtr->SlaveAddress << 1 | 1)); | 1 - the plot thickens. Macro please. > + if (Result == FALSE) { > + DEBUG (( > + DEBUG_ERROR, > + "iMXI2cRead: 2nd Slave address transfer failed 0x%04x\n", > + MmioRead16 ((UINTN)&I2cRegsPtr->I2SR))); > + Status = RETURN_DEVICE_ERROR; > + goto Exit; > + } > + > + // Disable master mode > + I2crReg = (IMX_I2C_I2CR_REG)MmioRead16 ((UINTN)&I2cRegsPtr->I2CR); > + > + // NXP application note AN4481 - Only one byte so do not send acknowledge > + if (ReadBufferSize == 1) { > + I2crReg.TXAK = 1; > + } else { > + I2crReg.TXAK = 0; > + } > + > + I2crReg.MTX = 0; > + MmioWrite16 ((UINTN)&I2cRegsPtr->I2CR, I2crReg.AsUint16); > + > + // A data transfer can now be initiated by a read from I2C_I2DR in Slave > + // Receive mode. > + MmioWrite16 ((UINTN)&I2cRegsPtr->I2SR, 0); > + MmioRead16 ((UINTN)&I2cRegsPtr->I2DR); > + > + do { > + // Wait for transfer to complete > + if (iMXI2cWaitStatusSet (I2cConfigPtr, IMX_I2C_I2SR_IIF) == FALSE) { > + DEBUG ((DEBUG_ERROR, "iMXI2cRead: waiting for read fail\n")); > + Status = RETURN_DEVICE_ERROR; > + goto Exit; > + } > + > + if (iMXI2cWaitStatusSet (I2cConfigPtr, IMX_I2C_I2SR_ICF) == FALSE) { > + DEBUG ((DEBUG_ERROR, "iMXI2cRead: waiting for read fail\n")); > + Status = RETURN_DEVICE_ERROR; > + goto Exit; > + } > + > + // Before the last byte is read, a Stop signal must be generated > + if (ReadBufferSize == 1) { > + Status = iMXI2cGenerateStop ( > + I2cConfigPtr); No need to wrap this line. > + if (RETURN_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "iMXI2cRead: iMXI2cGenerateStop fail %r\n", Status)); > + goto Exit; > + } > + } > + > + if (ReadBufferSize == 2) { For second to last byte? Could use a comment. And possibly comment above at == 1 could be expanded to explain why. > + I2crReg = (IMX_I2C_I2CR_REG)MmioRead16 ((UINTN)&I2cRegsPtr->I2CR); > + I2crReg.TXAK = 1; What does this 1 mean? #define? > + MmioWrite16 ((UINTN)&I2cRegsPtr->I2CR, I2crReg.AsUint16); > + } > + > + MmioWrite16 ((UINTN)&I2cRegsPtr->I2SR, 0); Why write 0? > + > + *ReadBufferPtr = MmioRead8 ((UINTN)&I2cRegsPtr->I2DR); > + ++ReadBufferPtr; > + --ReadBufferSize; > + } while (ReadBufferSize > 0); > + > +Exit: > + Status = iMXI2cGenerateStop (I2cConfigPtr); > + if (RETURN_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "iMXI2cRead: Final iMXI2cGenerateStop fail %r\n", Status)); > + } > + > + return Status; > +} > + > +/** > + Perform I2C write operation. > + > + The iMXI2cWrite perform I2C write operation by programming the I2C > + controller. The caller is responsible to provide I2C controller > + configuration. > + > + @param[in] IMX_I2C_CONFIG Pointer to structure containing the targeted > + I2C controller to be used for I2C operation. > + @param[in] RegisterAddress Targeted device register address to start write. > + @param[out] WriteBufferPtr Caller supplied buffer that contained data that > + would be read from for I2C write operation. > + @param[in] WriteBufferSize Size of caller supplied buffer. > + > + @retval RETURN_SUCCESS I2C Write operation succeeded. > + @retval RETURN_DEVICE_ERROR The I2C device is not functioning correctly. > + > +**/ > +RETURN_STATUS > +iMXI2cWrite ( > + IN IMX_I2C_CONFIG *I2cConfigPtr, > + IN UINT8 RegisterAddress, > + IN UINT8 *WriteBufferPtr, > + IN UINT32 WriteBufferSize > + ) > +{ > + IMX_I2C_REGS *I2cRegsPtr; > + BOOLEAN Result; > + RETURN_STATUS Status; > + > + I2cRegsPtr = (IMX_I2C_REGS*)I2cConfigPtr->ControllerAddress; > + Status = iMXI2cGenerateStart (I2cConfigPtr, RegisterAddress); > + if (RETURN_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "iMXI2cWrite: iMXI2cGenerateStart fail %r\n", Status)); > + goto Exit; > + } > + > + while (WriteBufferSize > 0) { > + Result = iMXI2cSendByte (I2cConfigPtr, *WriteBufferPtr); > + if (Result == FALSE) { > + DEBUG (( > + DEBUG_ERROR, Could skip that line break. / Leif > + "iMXI2cWrite: Slave address transfer fail 0x%04x\n", > + MmioRead16 ((UINTN)&I2cRegsPtr->I2SR))); > + Status = RETURN_DEVICE_ERROR; > + goto Exit; > + } > + > + ++WriteBufferPtr; > + --WriteBufferSize; > + } > + > +Exit: > + Status = iMXI2cGenerateStop (I2cConfigPtr); > + if (RETURN_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "iMXI2cWrite: iMXI2cGenerateStop fail %r\n", Status)); > + } > + > + return Status; > +} > diff --git a/Silicon/NXP/iMXPlatformPkg/Library/iMXI2cLib/iMXI2cLib.inf b/Silicon/NXP/iMXPlatformPkg/Library/iMXI2cLib/iMXI2cLib.inf > new file mode 100644 > index 000000000000..ad968e88b1da > --- /dev/null > +++ b/Silicon/NXP/iMXPlatformPkg/Library/iMXI2cLib/iMXI2cLib.inf > @@ -0,0 +1,35 @@ > +## @file > +# > +# Copyright (c) 2018 Microsoft Corporation. All rights reserved. > +# > +# This program and the accompanying materials > +# are licensed and made available under the terms and conditions of the BSD License > +# which accompanies this distribution. The full text of the license may be found at > +# http://opensource.org/licenses/bsd-license.php > +# > +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > +# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > +# > +## > + > +[Defines] > + INF_VERSION = 0x0001001A > + BASE_NAME = iMXI2cLib > + FILE_GUID = C4E4A003-8AEB-4C9B-8E72-D2BAD2134BDE > + MODULE_TYPE = BASE > + VERSION_STRING = 1.0 > + LIBRARY_CLASS = iMXI2cLib > + > +[Packages] > + EmbeddedPkg/EmbeddedPkg.dec > + MdePkg/MdePkg.dec > + Silicon/NXP/iMXPlatformPkg/iMXPlatformPkg.dec > + > +[LibraryClasses] > + BaseMemoryLib > + DebugLib > + IoLib > + TimerLib > + > +[Sources.common] > + iMXI2cLib.c > -- > 2.16.2.gvfs.1.33.gf5370f1 >