Re: [PATCH edk2-platforms 05/39] Silicon/NXP: Add support for I2c driver

[Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>]

> -----Original Message-----
> From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> Sent: Tuesday, April 17, 2018 10:07 PM
> To: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>
> Cc: ard.biesheuvel@linaro.org; edk2-devel@lists.01.org; Udit Kumar
> <udit.kumar@nxp.com>; Varun Sethi <V.Sethi@nxp.com>
> Subject: Re: [PATCH edk2-platforms 05/39] Silicon/NXP: Add support for I2c
> driver
> 
> On Fri, Feb 16, 2018 at 02:20:01PM +0530, Meenakshi wrote:
> > From: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>
> >
> > I2C driver produces gEfiI2cMasterProtocolGuid which can be
> > used by other modules.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>
> > ---
> >  Silicon/NXP/Drivers/I2cDxe/I2cDxe.c   | 726
> ++++++++++++++++++++++++++++++++++
> >  Silicon/NXP/Drivers/I2cDxe/I2cDxe.h   |  65 +++
> >  Silicon/NXP/Drivers/I2cDxe/I2cDxe.inf |  55 +++
> >  3 files changed, 846 insertions(+)
> >  create mode 100644 Silicon/NXP/Drivers/I2cDxe/I2cDxe.c
> >  create mode 100644 Silicon/NXP/Drivers/I2cDxe/I2cDxe.h
> >  create mode 100644 Silicon/NXP/Drivers/I2cDxe/I2cDxe.inf
> >
> > diff --git a/Silicon/NXP/Drivers/I2cDxe/I2cDxe.c
> b/Silicon/NXP/Drivers/I2cDxe/I2cDxe.c
> > new file mode 100644
> > index 0000000..80a8826
> > --- /dev/null
> > +++ b/Silicon/NXP/Drivers/I2cDxe/I2cDxe.c
> > @@ -0,0 +1,726 @@
> > +/** I2cDxe.c
> > +  I2c driver APIs for read, write, initialize, set speed and reset
> > +
> > +  Copyright 2017 NXP
> > +
> > +  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
> > +
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fope
> nsource.org%2Flicenses%2Fbsd-
> license.php&data=02%7C01%7Cmeenakshi.aggarwal%40nxp.com%7Cf09845
> 08e3e6425a971708d5a48171ca%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0
> %7C0%7C636595798265709617&sdata=lqPp%2BaHIRYNO%2B9buGRqcgvffpW
> nWzpIEpeLwdxPhZAk%3D&reserved=0
> > +
> > +  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 <Library/DebugLib.h>
> > +#include <Library/IoLib.h>
> > +#include <Library/TimerLib.h>
> > +#include <Library/UefiBootServicesTableLib.h>
> > +#include <Library/UefiLib.h>
> > +
> > +#include <Protocol/I2cMaster.h>
> > +
> > +#include "I2cDxe.h"
> > +
> > +STATIC CONST UINT16 ClkDiv[60][2] = {
> > +  { 20,  0x00 }, { 22, 0x01 },  { 24, 0x02 },  { 26, 0x03 },
> > +  { 28,  0x04 }, { 30,  0x05 }, { 32,  0x09 }, { 34, 0x06 },
> > +  { 36,  0x0A }, { 40, 0x07 },  { 44, 0x0C },  { 48, 0x0D },
> > +  { 52,  0x43 }, { 56,  0x0E }, { 60, 0x45 },  { 64, 0x12 },
> > +  { 68,  0x0F }, { 72,  0x13 }, { 80,  0x14 }, { 88,  0x15 },
> > +  { 96,  0x19 }, { 104, 0x16 }, { 112, 0x1A }, { 128, 0x17 },
> > +  { 136, 0x4F }, { 144, 0x1C }, { 160, 0x1D }, { 176, 0x55 },
> > +  { 192, 0x1E }, { 208, 0x56 }, { 224, 0x22 }, { 228, 0x24 },
> > +  { 240, 0x1F }, { 256, 0x23 }, { 288, 0x5C }, { 320, 0x25 },
> > +  { 384, 0x26 }, { 448, 0x2A }, { 480, 0x27 }, { 512, 0x2B },
> > +  { 576, 0x2C }, { 640, 0x2D }, { 768, 0x31 }, { 896, 0x32 },
> > +  { 960, 0x2F }, { 1024, 0x33 }, { 1152, 0x34 },{ 1280, 0x35 },
> > +  { 1536, 0x36 }, { 1792, 0x3A }, { 1920, 0x37 }, { 2048, 0x3B },
> > +  { 2304, 0x3C }, { 2560, 0x3D }, { 3072, 0x3E }, { 3584, 0x7A },
> > +  { 3840, 0x3F }, { 4096, 0x7B }, { 5120, 0x7D }, { 6144, 0x7E },
> > +};
> > +
> > +/**
> > +  Calculate and return proper clock divider
> > +
> > +  @param  Rate       clock rate
> > +
> > +  @retval ClkDiv     Value used to get frequency divider value
> > +
> > +**/
> > +STATIC
> > +UINT8
> > +GetClkDiv (
> > +  IN  UINT32         Rate
> > +  )
> > +{
> > +  UINTN              ClkRate;
> > +  UINT32             Div;
> > +  UINT8              ClkDivx;
> > +
> > +  ClkRate = GetBusFrequency ();
> > +
> > +  Div = (ClkRate + Rate - 1) / Rate;
> > +
> > +  if (Div < ClkDiv[0][0]) {
> > +    ClkDivx = 0;
> > +  } else if (Div > ClkDiv[ARRAY_SIZE (ClkDiv) - 1][0]){
> > +    ClkDivx = ARRAY_SIZE (ClkDiv) - 1;
> > +  } else {
> > +    for (ClkDivx = 0; ClkDiv[ClkDivx][0] < Div; ClkDivx++);
> > +  }
> > +
> > +  return ClkDivx;
> > +}
> > +
> > +/**
> > +  Function used to check if i2c is in mentioned state or not
> > +
> > +  @param   I2cRegs        Pointer to I2C registers
> > +  @param   State          i2c state need to be checked
> > +
> > +  @retval  EFI_NOT_READY  Arbitration was lost
> > +  @retval  EFI_TIMEOUT    Timeout occured
> > +  @retval  CurrState      Value of state register
> > +
> > +**/
> > +STATIC
> > +EFI_STATUS
> > +WaitForI2cState (
> > +  IN  I2C_REGS            *I2cRegs,
> > +  IN  UINT32              State
> > +  )
> > +{
> > +  UINT8                   CurrState;
> > +  UINT64                  Cnt;
> > +
> > +  for (Cnt = 0; Cnt < 50000; Cnt++) {
> > +    MemoryFence ();
> > +    CurrState = MmioRead8 ((UINTN)&I2cRegs->I2cSr);
> > +    if (CurrState & I2C_SR_IAL) {
> > +       MmioWrite8 ((UINTN)&I2cRegs->I2cSr, CurrState | I2C_SR_IAL);
> > +        return EFI_NOT_READY;
> > +    }
> > +
> > +    if ((CurrState & (State >> 8)) == (UINT8)State) {
> > +      return CurrState;
> > +    }
> > +  }
> > +
> > +  return EFI_TIMEOUT;
> > +}
> > +
> > +/**
> > +  Function to transfer byte on i2c
> > +
> > +  @param   I2cRegs        Pointer to i2c registers
> > +  @param   Byte           Byte to be transferred on i2c bus
> > +
> > +  @retval  EFI_NOT_READY  Arbitration was lost
> > +  @retval  EFI_TIMEOUT    Timeout occured
> > +  @retval  EFI_NOT_FOUND  ACK was not recieved
> > +  @retval  EFI_SUCCESS    Data transfer was succesful
> > +
> > +**/
> > +STATIC
> > +EFI_STATUS
> > +TransferByte (
> > +  IN  I2C_REGS            *I2cRegs,
> > +  IN  UINT8               Byte
> > +  )
> > +{
> > +  EFI_STATUS              Ret;
> > +
> > +  MmioWrite8 ((UINTN)&I2cRegs->I2cSr, I2C_SR_IIF_CLEAR);
> > +  MmioWrite8 ((UINTN)&I2cRegs->I2cDr, Byte);
> > +
> > +  Ret = WaitForI2cState (I2cRegs, IIF);
> > +  if ((Ret == EFI_TIMEOUT) || (Ret == EFI_NOT_READY)) {
> > +    return Ret;
> > +  }
> > +
> > +  if (Ret & I2C_SR_RX_NO_AK) {
> > +    return EFI_NOT_FOUND;
> > +  }
> > +
> > +  return EFI_SUCCESS;
> > +}
> > +
> > +/**
> > +  Function to stop transaction on i2c bus
> > +
> > +  @param   I2cRegs          Pointer to i2c registers
> > +
> > +  @retval  EFI_NOT_READY    Arbitration was lost
> > +  @retval  EFI_TIMEOUT      Timeout occured
> > +  @retval  EFI_SUCCESS      Stop operation was successful
> > +
> > +**/
> > +STATIC
> > +EFI_STATUS
> > +I2cStop (
> > +  IN  I2C_REGS             *I2cRegs
> > +  )
> > +{
> > +  INT32                    Ret;
> > +  UINT32                   Temp;
> > +
> > +  Temp = MmioRead8 ((UINTN)&I2cRegs->I2cCr);
> > +
> > +  Temp &= ~(I2C_CR_MSTA | I2C_CR_MTX);
> > +  MmioWrite8 ((UINTN)&I2cRegs->I2cCr, Temp);
> > +
> > +  Ret = WaitForI2cState (I2cRegs, BUS_IDLE);
> > +
> > +  if (Ret < 0) {
> > +    return Ret;
> > +  } else {
> > +    return EFI_SUCCESS;
> > +  }
> > +}
> > +
> > +/**
> > +  Function to send start signal, Chip Address and
> > +  memory offset
> > +
> > +  @param   I2cRegs         Pointer to i2c base registers
> > +  @param   Chip            Chip Address
> > +  @param   Offset          Slave memory's offset
> > +  @param   Alen            length of chip address
> > +
> > +  @retval  EFI_NOT_READY   Arbitration lost
> > +  @retval  EFI_TIMEOUT     Failed to initialize data transfer in predefined
> time
> > +  @retval  EFI_NOT_FOUND   ACK was not recieved
> > +  @retval  EFI_SUCCESS     Read was successful
> > +
> > +**/
> > +STATIC
> > +EFI_STATUS
> > +InitTransfer (
> > +  IN  I2C_REGS             *I2cRegs,
> > +  IN  UINT8                Chip,
> > +  IN  UINT32               Offset,
> > +  IN  INT32                Alen
> > +  )
> > +{
> > +  UINT32                   Temp;
> > +  EFI_STATUS               Ret;
> > +
> > +  // Enable I2C controller
> > +  if (MmioRead8 ((UINTN)&I2cRegs->I2cCr) & I2C_CR_IDIS) {
> > +    MmioWrite8 ((UINTN)&I2cRegs->I2cCr, I2C_CR_IEN);
> > +  }
> > +
> > +  if (MmioRead8 ((UINTN)&I2cRegs->I2cAdr) == (Chip << 1)) {
> > +    MmioWrite8 ((UINTN)&I2cRegs->I2cAdr, (Chip << 1) ^ 2);
> > +  }
> > +
> > +  MmioWrite8 ((UINTN)&I2cRegs->I2cSr, I2C_SR_IIF_CLEAR);
> > +  Ret = WaitForI2cState (I2cRegs, BUS_IDLE);
> > +  if ((Ret == EFI_TIMEOUT) || (Ret == EFI_NOT_READY)) {
> > +    return Ret;
> > +  }
> > +
> > +  // Start I2C transaction
> > +  Temp = MmioRead8 ((UINTN)&I2cRegs->I2cCr);
> > +  // set to master mode
> > +  Temp |= I2C_CR_MSTA;
> > +  MmioWrite8 ((UINTN)&I2cRegs->I2cCr, Temp);
> > +
> > +  Ret = WaitForI2cState (I2cRegs, BUS_BUSY);
> > +  if ((Ret == EFI_TIMEOUT) || (Ret == EFI_NOT_READY)) {
> > +    return Ret;
> > +  }
> > +
> > +  Temp |= I2C_CR_MTX | I2C_CR_TX_NO_AK;
> > +  MmioWrite8 ((UINTN)&I2cRegs->I2cCr, Temp);
> > +
> > +  // write slave Address
> > +  Ret = TransferByte (I2cRegs, Chip << 1);
> > +  if (Ret != EFI_SUCCESS) {
> > +    return Ret;
> > +  }
> > +
> > +  if (Alen >= 0) {
> > +    while (Alen--) {
> > +      Ret = TransferByte (I2cRegs, (Offset >> (Alen * 8)) & 0xff);
> > +      if (Ret != EFI_SUCCESS)
> > +        return Ret;
> > +    }
> > +  }
> > +  return EFI_SUCCESS;
> > +}
> > +
> > +/**
> > +  Function to check if i2c bus is idle
> > +
> > +  @param   Base          Pointer to base address of I2c controller
> > +
> > +  @retval  EFI_SUCCESS
> > +
> > +**/
> > +STATIC
> > +INT32
> > +I2cBusIdle (
> > +  IN  VOID               *Base
> > +  )
> > +{
> > +  return EFI_SUCCESS;
> > +}
> > +
> > +/**
> > +  Function to initiate data transfer on i2c bus
> > +
> > +  @param   I2cRegs         Pointer to i2c base registers
> > +  @param   Chip            Chip Address
> > +  @param   Offset          Slave memory's offset
> > +  @param   Alen            length of chip address
> > +
> > +  @retval  EFI_NOT_READY   Arbitration lost
> > +  @retval  EFI_TIMEOUT     Failed to initialize data transfer in predefined
> time
> > +  @retval  EFI_NOT_FOUND   ACK was not recieved
> > +  @retval  EFI_SUCCESS     Read was successful
> > +
> > +**/
> > +STATIC
> > +EFI_STATUS
> > +InitDataTransfer (
> > +  IN  I2C_REGS             *I2cRegs,
> > +  IN  UINT8                Chip,
> > +  IN  UINT32               Offset,
> > +  IN  INT32                Alen
> > +  )
> > +{
> > +  EFI_STATUS               Status;
> > +  INT32                    Retry;
> > +
> > +  for (Retry = 0; Retry < 3; Retry++) {
> > +    Status = InitTransfer (I2cRegs, Chip, Offset, Alen);
> > +    if (Status == EFI_SUCCESS) {
> > +      return EFI_SUCCESS;
> > +    }
> > +
> > +    I2cStop (I2cRegs);
> > +
> > +    if (EFI_NOT_FOUND == Status) {
> > +      return Status;
> > +    }
> > +
> > +    // Disable controller
> > +    if (Status != EFI_NOT_READY) {
> > +      MmioWrite8 ((UINTN)&I2cRegs->I2cCr, I2C_CR_IDIS);
> > +    }
> > +
> > +    if (I2cBusIdle (I2cRegs) < 0) {
> > +      break;
> > +    }
> > +  }
> > +  return Status;
> > +}
> > +
> > +/**
> > +  Function to read data using i2c bus
> > +
> > +  @param   I2cBus          I2c Controller number
> > +  @param   Chip            Address of slave device from where data to be read
> > +  @param   Offset          Offset of slave memory
> > +  @param   Alen            Address length of slave
> > +  @param   Buffer          A pointer to the destination buffer for the data
> > +  @param   Len             Length of data to be read
> > +
> > +  @retval  EFI_NOT_READY   Arbitration lost
> > +  @retval  EFI_TIMEOUT     Failed to initialize data transfer in predefined
> time
> > +  @retval  EFI_NOT_FOUND   ACK was not recieved
> > +  @retval  EFI_SUCCESS     Read was successful
> > +
> > +**/
> > +STATIC
> > +EFI_STATUS
> > +I2cDataRead (
> > +  IN  UINT32               I2cBus,
> > +  IN  UINT8                Chip,
> > +  IN  UINT32               Offset,
> > +  IN  UINT32               Alen,
> > +  IN  UINT8                *Buffer,
> > +  IN  UINT32               Len
> > +  )
> > +{
> > +  EFI_STATUS               Status;
> > +  UINT32                   Temp;
> > +  INT32                    I;
> > +  I2C_REGS                 *I2cRegs;
> > +
> > +  I2cRegs = (I2C_REGS *)(FixedPcdGet64 (PcdI2c0BaseAddr +
> > +                         (I2cBus * FixedPcdGet32 (PcdI2cSize))));
> 
> Please get rid of this hardcoded base address and use NonDiscoverable
> Have a look at Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/ and
> Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/
> for example.
> 
I have checked SynQuacer code and i dont see its adding much advantage.
There also base address is hard coded SYNQUACER_I2C1_BASE in PlatformDxe driver.

What we can do is, instead of using pcds here, we can define address in nxp's i2c struct itself 
and use from there.

 > > +
> > +  Status = InitDataTransfer (I2cRegs, Chip, Offset, Alen);
> > +  if (Status != EFI_SUCCESS) {
> > +    return Status;
> > +  }
> > +
> > +  Temp = MmioRead8 ((UINTN)&I2cRegs->I2cCr);
> > +  Temp |= I2C_CR_RSTA;
> > +  MmioWrite8 ((UINTN)&I2cRegs->I2cCr, Temp);
> > +
> > +  Status = TransferByte (I2cRegs, (Chip << 1) | 1);
> > +  if (Status != EFI_SUCCESS) {
> > +    I2cStop (I2cRegs);
> > +    return Status;
> > +  }
> > +
> > +  // setup bus to read data
> > +  Temp = MmioRead8 ((UINTN)&I2cRegs->I2cCr);
> > +  Temp &= ~(I2C_CR_MTX | I2C_CR_TX_NO_AK);
> > +  if (Len == 1) {
> > +    Temp |= I2C_CR_TX_NO_AK;
> > +  }
> > +
> > +  MmioWrite8 ((UINTN)&I2cRegs->I2cCr, Temp);
> > +  MmioWrite8 ((UINTN)&I2cRegs->I2cSr, I2C_SR_IIF_CLEAR);
> > +
> > +  // Dummy Read to initiate recieve operation
> > +  MmioRead8 ((UINTN)&I2cRegs->I2cDr);
> > +
> > +  for (I = 0; I < Len; I++) {
> > +    Status = WaitForI2cState (I2cRegs, IIF);
> > +    if ((Status == EFI_TIMEOUT) || (Status == EFI_NOT_READY)) {
> > +       I2cStop (I2cRegs);
> > +       return Status;
> > +    }
> > +    //
> > +    // It must generate STOP before read I2DR to prevent
> > +    // controller from generating another clock cycle
> > +    //
> > +    if (I == (Len - 1)) {
> > +      I2cStop (I2cRegs);
> > +    } else if (I == (Len - 2)) {
> > +      Temp = MmioRead8 ((UINTN)&I2cRegs->I2cCr);
> > +      Temp |= I2C_CR_TX_NO_AK;
> > +      MmioWrite8 ((UINTN)&I2cRegs->I2cCr, Temp);
> > +    }
> > +    MmioWrite8 ((UINTN)&I2cRegs->I2cSr, I2C_SR_IIF_CLEAR);
> > +    Buffer[I] = MmioRead8 ((UINTN)&I2cRegs->I2cDr);
> > +  }
> > +
> > +  I2cStop (I2cRegs);
> > +
> > +  return EFI_SUCCESS;
> > +}
> > +
> > +/**
> > +  Function to write data using i2c bus
> > +
> > +  @param   I2cBus          I2c Controller number
> > +  @param   Chip            Address of slave device where data to be written
> > +  @param   Offset          Offset of slave memory
> > +  @param   Alen            Address length of slave
> > +  @param   Buffer          A pointer to the source buffer for the data
> > +  @param   Len             Length of data to be write
> > +
> > +  @retval  EFI_NOT_READY   Arbitration lost
> > +  @retval  EFI_TIMEOUT     Failed to initialize data transfer in predefined
> time
> > +  @retval  EFI_NOT_FOUND   ACK was not recieved
> > +  @retval  EFI_SUCCESS     Read was successful
> > +
> > +**/
> > +STATIC
> > +EFI_STATUS
> > +I2cDataWrite (
> > +  IN  UINT32               I2cBus,
> > +  IN  UINT8                Chip,
> > +  IN  UINT32               Offset,
> > +  IN  INT32                Alen,
> > +  OUT UINT8                *Buffer,
> > +  IN  INT32                Len
> > +  )
> > +{
> > +  EFI_STATUS               Status;
> > +  I2C_REGS                 *I2cRegs;
> > +  INT32                    I;
> > +
> > +  I2cRegs = (I2C_REGS *)(FixedPcdGet64 (PcdI2c0BaseAddr +
> > +                         (I2cBus * FixedPcdGet32 (PcdI2cSize))));
> > +
> > +  Status = InitDataTransfer (I2cRegs, Chip, Offset, Alen);
> > +  if (Status != EFI_SUCCESS) {
> > +    return Status;
> > +  }
> > +
> > +  // Write operation
> > +  for (I = 0; I < Len; I++) {
> > +    Status = TransferByte (I2cRegs, Buffer[I]);
> > +    if (Status != EFI_SUCCESS) {
> > +      break;
> > +    }
> > +  }
> > +
> > +  I2cStop (I2cRegs);
> > +  return Status;
> > +}
> > +
> > +/**
> > +  Function to set i2c bus frequency
> > +
> > +  @param   This            Pointer to I2c master protocol
> > +  @param   BusClockHertz   value to be set
> > +
> > +  @retval EFI_SUCCESS      Operation successfull
> > +**/
> > +
> > +EFI_STATUS
> > +EFIAPI
> > +SetBusFrequency (
> > +  IN CONST EFI_I2C_MASTER_PROTOCOL   *This,
> > +  IN OUT UINTN                       *BusClockHertz
> > + )
> > +{
> > +  I2C_REGS                 *I2cRegs;
> > +  UINT8                    ClkId;
> > +  UINT8                    SpeedId;
> > +
> > +  I2cRegs = (I2C_REGS *)(FixedPcdGet64 (PcdI2c0BaseAddr +
> > +                         (PcdGet32 (PcdI2cBus) * FixedPcdGet32 (PcdI2cSize))));
> > +
> > +  ClkId = GetClkDiv (*BusClockHertz);
> > +  SpeedId = ClkDiv[ClkId][1];
> > +
> > +  // Store divider value
> > +  MmioWrite8 ((UINTN)&I2cRegs->I2cFdr, SpeedId);
> > +
> > +  MemoryFence ();
> > +
> > +  return EFI_SUCCESS;
> > +}
> > +
> > +/**
> > +  Function to reset I2c Controller
> > +
> > +  @param  This             Pointer to I2c master protocol
> > +
> > +  @return EFI_SUCCESS      Operation successfull
> > +**/
> > +EFI_STATUS
> > +EFIAPI
> > +Reset (
> > +  IN CONST EFI_I2C_MASTER_PROTOCOL *This
> > +  )
> > +{
> > +  I2C_REGS                         *I2cRegs;
> > +
> > +  I2cRegs = (I2C_REGS *)(FixedPcdGet64 (PcdI2c0BaseAddr +
> > +                         (PcdGet32 (PcdI2cBus) * FixedPcdGet32 (PcdI2cSize))));
> > +
> > +  // Reset module
> > +  MmioWrite8 ((UINTN)&I2cRegs->I2cCr, I2C_CR_IDIS);
> > +  MmioWrite8 ((UINTN)&I2cRegs->I2cSr, 0);
> > +
> > +  MemoryFence ();
> > +
> > +  return EFI_SUCCESS;
> > +}
> > +
> > +EFI_STATUS
> > +EFIAPI
> > +StartRequest (
> > +  IN CONST EFI_I2C_MASTER_PROTOCOL *This,
> > +  IN UINTN                         SlaveAddress,
> > +  IN EFI_I2C_REQUEST_PACKET        *RequestPacket,
> > +  IN EFI_EVENT                     Event            OPTIONAL,
> > +  OUT EFI_STATUS                   *I2cStatus       OPTIONAL
> > +  )
> > +{
> > +  UINT32                           Count;
> > +  INT32                            Ret;
> > +  UINT32                           Length;
> > +  UINT8                            *Buffer;
> > +  UINT32                           Flag;
> > +  UINT32                           RegAddress;
> > +  UINT32                           OffsetLength;
> > +
> > +  RegAddress = 0;
> > +
> > +  if (RequestPacket->OperationCount <= 0) {
> > +    DEBUG ((DEBUG_ERROR,"%a: Operation count is not valid %d\n",
> > +           __FUNCTION__, RequestPacket->OperationCount));
> > +    return EFI_INVALID_PARAMETER;
> > +  }
> > +
> > +  OffsetLength = RequestPacket->Operation[0].LengthInBytes;
> > +  RegAddress = *RequestPacket->Operation[0].Buffer;
> > +
> > +  for (Count = 1; Count < RequestPacket->OperationCount; Count++) {
> > +    Flag = RequestPacket->Operation[Count].Flags;
> > +    Length = RequestPacket->Operation[Count].LengthInBytes;
> > +    Buffer = RequestPacket->Operation[Count].Buffer;
> > +
> > +    if (Length <= 0) {
> > +      DEBUG ((DEBUG_ERROR,"%a: Invalid length of buffer %d\n",
> > +             __FUNCTION__, Length));
> > +      return EFI_INVALID_PARAMETER;
> > +    }
> > +
> > +    if (Flag == I2C_FLAG_READ) {
> > +      Ret = I2cDataRead (PcdGet32 (PcdI2cBus), SlaveAddress,
> > +              RegAddress, OffsetLength, Buffer, Length);
> > +      if (Ret != EFI_SUCCESS) {
> > +        DEBUG ((DEBUG_ERROR,"%a: I2c read operation failed (error %d)\n",
> > +               __FUNCTION__, Ret));
> > +        return Ret;
> > +      }
> > +    } else if (Flag == I2C_FLAG_WRITE) {
> > +      Ret = I2cDataWrite (PcdGet32 (PcdI2cBus), SlaveAddress,
> > +              RegAddress, OffsetLength, Buffer, Length);
> > +      if (Ret != EFI_SUCCESS) {
> > +        DEBUG ((DEBUG_ERROR,"%a: I2c write operation failed (error %d)\n",
> > +               __FUNCTION__, Ret));
> > +        return Ret;
> > +      }
> > +    } else {
> > +      DEBUG ((DEBUG_ERROR,"%a: Invalid Flag %d\n",
> > +             __FUNCTION__, Flag));
> > +      return EFI_INVALID_PARAMETER;
> > +    }
> > +  }
> > +
> > +  return EFI_SUCCESS;
> > +}
> > +
> > +CONST EFI_I2C_CONTROLLER_CAPABILITIES I2cControllerCapabilities = {
> > +  0,
> > +  0,
> > +  0,
> > +  0
> > +};
> > +
> > +STATIC EFI_I2C_MASTER_PROTOCOL gI2c = {
> > +  ///
> > +  /// Set the clock frequency for the I2C bus.
> > +  ///
> > +  SetBusFrequency,
> > +  ///
> > +  /// Reset the I2C host controller.
> > +  ///
> > +  Reset,
> > +  ///
> > +  /// Start an I2C transaction in master mode on the host controller.
> > +  ///
> > +  StartRequest,
> > +  ///
> > +  /// Pointer to an EFI_I2C_CONTROLLER_CAPABILITIES data structure
> containing
> > +  /// the capabilities of the I2C host controller.
> > +  ///
> > +  &I2cControllerCapabilities
> > +};
> > +
> > +STATIC I2C_DEVICE_PATH gDevicePath = {
> > +  {
> > +    {
> > +      HARDWARE_DEVICE_PATH, HW_VENDOR_DP,
> > +      {
> > +        sizeof (VENDOR_DEVICE_PATH), 0
> > +      }
> > +    },
> > +    EFI_CALLER_ID_GUID
> > +  },
> > +  {
> > +    END_DEVICE_PATH_TYPE,
> > +    END_ENTIRE_DEVICE_PATH_SUBTYPE,
> > +    {
> > +      sizeof (EFI_DEVICE_PATH_PROTOCOL), 0
> > +    }
> > +  }
> > +};
> > +
> > +/**
> > +  The Entry Point for I2C driver.
> > +
> > +  @param[in] ImageHandle    The firmware allocated handle for the EFI
> image.
> > +  @param[in] SystemTable    A pointer to the EFI System Table.
> > +
> > +  @retval EFI_SUCCESS       The entry point is executed successfully.
> > +  @retval other             Some error occurs when executing this entry point.
> > +
> > +**/
> > +EFI_STATUS
> > +EFIAPI
> > +I2cDxeEntryPoint (
> > +  IN EFI_HANDLE             ImageHandle,
> > +  IN EFI_SYSTEM_TABLE       *SystemTable
> > +  )
> > +{
> > +  EFI_STATUS                Status;
> > +
> > +  //
> > +  // Install I2c Master protocol on this controller
> > +  //
> > +  Status = gBS->InstallMultipleProtocolInterfaces (
> > +                &ImageHandle,
> 
> Indentation - + 2 spaces.
> 
> > +                &gEfiI2cMasterProtocolGuid,
> > +                (VOID**)&gI2c,
> > +                &gEfiDevicePathProtocolGuid,
> > +                &gDevicePath,
> > +                NULL
> > +                );
> > +
> > +  ASSERT_EFI_ERROR (Status);
> > +
> > +  return Status;
> > +}
> > +
> > +/**
> > +  Unload function for the I2c Driver.
> > +
> > +  @param  ImageHandle[in]        The allocated handle for the EFI image
> > +
> > +  @retval EFI_SUCCESS            The driver was unloaded successfully
> > +  @retval EFI_INVALID_PARAMETER  ImageHandle is not a valid image
> handle.
> > +
> > +**/
> > +EFI_STATUS
> > +EFIAPI
> > +I2cDxeUnload (
> > +  IN EFI_HANDLE                  ImageHandle
> > +  )
> > +{
> > +  EFI_STATUS                     Status;
> > +  EFI_HANDLE                     *HandleBuffer;
> > +  UINTN                          HandleCount;
> > +  UINTN                          Index;
> > +
> > +  //
> > +  // Retrieve all I2c handles in the handle database
> > +  //
> > +  Status = gBS->LocateHandleBuffer (ByProtocol,
> > +                                    &gEfiI2cMasterProtocolGuid,
> > +                                    NULL,
> > +                                    &HandleCount,
> > +                                    &HandleBuffer);
> > +  if (EFI_ERROR (Status)) {
> > +    return Status;
> > +  }
> > +
> > +  //
> > +  // Disconnect the driver from the handles in the handle database
> > +  //
> > +  for (Index = 0; Index < HandleCount; Index++) {
> > +    Status = gBS->DisconnectController (HandleBuffer[Index],
> > +                                        gImageHandle,
> > +                                        NULL);
> > +  }
> > +
> > +  //
> > +  // Free the handle array
> > +  //
> > +  gBS->FreePool (HandleBuffer);
> > +
> > +  //
> > +  // Uninstall protocols installed by the driver in its entrypoint
> > +  //
> > +  Status = gBS->UninstallMultipleProtocolInterfaces (ImageHandle,
> > +                  &gEfiI2cMasterProtocolGuid, &gI2c,
> > +                  &gEfiDevicePathProtocolGuid, &gDevicePath,
> > +                  NULL);
> > +
> > +  return Status;
> > +}
> > diff --git a/Silicon/NXP/Drivers/I2cDxe/I2cDxe.h
> b/Silicon/NXP/Drivers/I2cDxe/I2cDxe.h
> > new file mode 100644
> > index 0000000..4a562d3
> > --- /dev/null
> > +++ b/Silicon/NXP/Drivers/I2cDxe/I2cDxe.h
> > @@ -0,0 +1,65 @@
> > +/** I2cDxe.h
> > +  Header defining the constant, base address amd function for I2C
> controller
> > +
> > +  Copyright 2017 NXP
> > +
> > +  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
> > +
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fope
> nsource.org%2Flicenses%2Fbsd-
> license.php&data=02%7C01%7Cmeenakshi.aggarwal%40nxp.com%7Cf09845
> 08e3e6425a971708d5a48171ca%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0
> %7C0%7C636595798265709617&sdata=lqPp%2BaHIRYNO%2B9buGRqcgvffpW
> nWzpIEpeLwdxPhZAk%3D&reserved=0
> > +
> > +  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 __I2C_DXE_H___
> 
> Typo in include guard, making it not a guard. (Extra '_'.)
> 
> > +#define __I2C_DXE_H__
> > +
> > +#include <Uefi.h>
> > +
> > +#define I2C_CR_IIEN           (1 << 6)
> > +#define I2C_CR_MSTA           (1 << 5)
> > +#define I2C_CR_MTX            (1 << 4)
> > +#define I2C_CR_TX_NO_AK       (1 << 3)
> > +#define I2C_CR_RSTA           (1 << 2)
> > +
> > +#define I2C_SR_ICF            (1 << 7)
> > +#define I2C_SR_IBB            (1 << 5)
> > +#define I2C_SR_IAL            (1 << 4)
> > +#define I2C_SR_IIF            (1 << 1)
> > +#define I2C_SR_RX_NO_AK       (1 << 0)
> > +
> > +#define I2C_CR_IEN            (0 << 7)
> > +#define I2C_CR_IDIS           (1 << 7)
> > +#define I2C_SR_IIF_CLEAR      (1 << 1)
> > +
> > +#define BUS_IDLE              (0 | (I2C_SR_IBB << 8))
> > +#define BUS_BUSY              (I2C_SR_IBB | (I2C_SR_IBB << 8))
> > +#define IIF                   (I2C_SR_IIF | (I2C_SR_IIF << 8))
> > +
> > +#define I2C_FLAG_WRITE        0x0
> > +
> > +typedef struct {
> > +  VENDOR_DEVICE_PATH        Guid;
> > +  EFI_DEVICE_PATH_PROTOCOL  End;
> > +} I2C_DEVICE_PATH;
> > +
> > +/**
> > +  Record defining i2c registers
> > +**/
> > +typedef struct {
> > +  UINT8     I2cAdr;
> > +  UINT8     I2cFdr;
> > +  UINT8     I2cCr;
> > +  UINT8     I2cSr;
> > +  UINT8     I2cDr;
> > +} I2C_REGS ;
> > +
> > +extern
> > +UINT64
> > +GetBusFrequency (
> > +  VOID
> > +  );
> > +
> > +#endif
> > diff --git a/Silicon/NXP/Drivers/I2cDxe/I2cDxe.inf
> b/Silicon/NXP/Drivers/I2cDxe/I2cDxe.inf
> > new file mode 100644
> > index 0000000..ceb1b11
> > --- /dev/null
> > +++ b/Silicon/NXP/Drivers/I2cDxe/I2cDxe.inf
> > @@ -0,0 +1,55 @@
> > +#  @file
> > +#
> > +#  Component description file for I2c driver
> > +#
> > +#  Copyright (c) 2015, Freescale Semiconductor, Inc. All rights reserved.
> > +#  Copyright 2017 NXP
> > +#
> > +#  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
> > +#
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fope
> nsource.org%2Flicenses%2Fbsd-
> license.php&data=02%7C01%7Cmeenakshi.aggarwal%40nxp.com%7Cf09845
> 08e3e6425a971708d5a48171ca%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0
> %7C0%7C636595798265709617&sdata=lqPp%2BaHIRYNO%2B9buGRqcgvffpW
> nWzpIEpeLwdxPhZAk%3D&reserved=0
> > +#
> > +#  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                      = I2cDxe
> > +  FILE_GUID                      = 5f2927ba-1b04-4d5f-8bef-2b50c635d1e7
> > +  MODULE_TYPE                    = DXE_DRIVER
> > +  VERSION_STRING                 = 1.0
> > +  ENTRY_POINT                    = I2cDxeEntryPoint
> > +  UNLOAD                         = I2cDxeUnload
> > +
> > +[Sources.common]
> > +  I2cDxe.c
> > +
> > +[LibraryClasses]
> > +  ArmLib
> > +  IoLib
> > +  MemoryAllocationLib
> > +  PcdLib
> > +  SocLib
> > +  TimerLib
> > +  UefiDriverEntryPoint
> > +  UefiLib
> > +
> > +[Packages]
> > +  MdePkg/MdePkg.dec
> > +  Silicon/NXP/NxpQoriqLs.dec
> > +
> > +[Protocols]
> > +  gEfiI2cMasterProtocolGuid
> > +
> > +[Pcd]
> > +  gNxpQoriqLsTokenSpaceGuid.PcdI2cBus
> > +  gNxpQoriqLsTokenSpaceGuid.PcdI2cSpeed
> > +  gNxpQoriqLsTokenSpaceGuid.PcdI2c0BaseAddr
> > +  gNxpQoriqLsTokenSpaceGuid.PcdI2cSize
> > +  gNxpQoriqLsTokenSpaceGuid.PcdNumI2cController
> > +
> > +[Depex]
> > +  TRUE
> 
> Migrating to NonDiscoverableDeviceRegistrationLib should also let you
> get rid of this hardcoded depex.
> 
> /
>     Leif
> 
> > --
> > 1.9.1
> >