Re: [edk2-devel] [PATCH 01/19] Silicon/NXP: Add I2c lib

["Ard Biesheuvel" <ard.biesheuvel@linaro.org>]

On Sat, 8 Feb 2020 at 18:14, Leif Lindholm <leif@nuviainc.com> wrote:
>
> On Fri, Feb 07, 2020 at 18:13:10 +0530, Pankaj Bansal wrote:
> > I2c lib is going to be used in PrePeiCore sec module to get the
> > System clock information from devices connected to i2c (like fpga
> > or clcok generator)
> >
> > since we don't have support of DXE modules this early in boot stage,
> > move the i2c controller functionality in library.
> >
> > Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
> > ---
> >  Platform/NXP/NxpQoriqLs.dsc.inc             |   4 +-
> >  Silicon/NXP/Include/Library/I2cLib.h        |  99 ++++
> >  Silicon/NXP/Library/I2cLib/I2cLib.c         | 532 ++++++++++++++++++++
> >  Silicon/NXP/Library/I2cLib/I2cLib.inf       |  30 ++
> >  Silicon/NXP/Library/I2cLib/I2cLibInternal.h |  95 ++++
> >  Silicon/NXP/NxpQoriqLs.dec                  |  10 +-
> >  6 files changed, 768 insertions(+), 2 deletions(-)
> >  create mode 100644 Silicon/NXP/Include/Library/I2cLib.h
> >  create mode 100644 Silicon/NXP/Library/I2cLib/I2cLib.c
> >  create mode 100644 Silicon/NXP/Library/I2cLib/I2cLib.inf
> >  create mode 100644 Silicon/NXP/Library/I2cLib/I2cLibInternal.h
> >
> > diff --git a/Platform/NXP/NxpQoriqLs.dsc.inc b/Platform/NXP/NxpQoriqLs.dsc.inc
> > index fa5f30dd39..b28e0615f7 100644
> > --- a/Platform/NXP/NxpQoriqLs.dsc.inc
> > +++ b/Platform/NXP/NxpQoriqLs.dsc.inc
...
> > + *
> > + * In case of duplicate SCL Divider value, the IBC value
> > + * with high MUL value has been selected.
> > + * A higher MUL value results in a lower sampling rate of the I2C signals.
> > + * This gives the I2C module greater immunity against glitches in the I2C signals.
> > + */
> > +STATIC I2C_CLOCK_DIVIDER_PAIR I2cClockDividerGlitchEnabled[] = {
>
> Module-scope global variables should have 'm' prefix.
>

These should be CONST as well afaict

> > +  { 34, 0x0 }, { 36, 0x1 }, { 38, 0x2 }, { 40, 0x3 },
> > +  { 42, 0x4 }, { 44, 0x8 }, { 48, 0x9 }, { 52, 0xA },
> > +  { 54, 0x7 }, { 56, 0xB }, { 60, 0xC }, { 64, 0x10 },
> > +  { 68, 0x40 }, { 72, 0x41 }, { 76, 0x42 }, { 80, 0x43 },
> > +  { 84, 0x44 }, { 88, 0x48 }, { 96, 0x49 }, { 104, 0x4A },
> > +  { 108, 0x47 }, { 112, 0x4B }, { 120, 0x4C }, { 128, 0x50 },
> > +  { 136, 0x80 }, { 144, 0x81 }, { 152, 0x82 }, { 160, 0x83 },
> > +  { 168, 0x84 }, { 176, 0x88 }, { 192, 0x89 }, { 208, 0x8A },
> > +  { 216, 0x87 }, { 224, 0x8B }, { 240, 0x8C }, { 256, 0x90 },
> > +  { 288, 0x91 }, { 320, 0x92 }, { 336, 0x8F }, { 352, 0x93 },
> > +  { 384, 0x98 }, { 416, 0x95 }, { 448, 0x99 }, { 480, 0x96 },
> > +  { 512, 0x9A }, { 576, 0x9B }, { 640, 0xA0 }, { 704, 0x9D },
> > +  { 768, 0xA1 }, { 832, 0x9E }, { 896, 0xA2 }, { 960, 0x67 },
> > +  { 1024, 0xA3 }, { 1152, 0xA4 }, { 1280, 0xA8 }, { 1536, 0xA9 },
> > +  { 1792, 0xAA }, { 1920, 0xA7 }, { 2048, 0xAB }, { 2304, 0xAC },
> > +  { 2560, 0xB0 }, { 3072, 0xB1 }, { 3584, 0xB2 }, { 3840, 0xAF },
> > +  { 4096, 0xB3 }, { 4608, 0xB4 }, { 5120, 0xB8 }, { 6144, 0xB9 },
> > +  { 7168, 0xBA }, { 7680, 0xB7 }, { 8192, 0xBB }, { 9216, 0xBC },
> > +  { 10240, 0xBD }, { 12288, 0xBE }, { 15360, 0xBF }
> > +};
> > +
> > +/*
> > + * I2C divider and hold values when glitch filter is disabled
> > + * taken from table 21-13, LX2160ARM_RevE 01/2020
> > + *
> > + * In case of duplicate SCL Divider value, the IBC value
> > + * with high MUL value has been selected.
> > + * A higher MUL value results in a lower sampling rate of the I2C signals.
> > + * This gives the I2C module greater immunity against glitches in the I2C signals.
> > + */
> > +STATIC I2C_CLOCK_DIVIDER_PAIR I2cClockDividerGlitchDisabled[] = {
>
> Module-scope global variables should have 'm' prefix.
>
> > +  { 20, 0x0 },{ 22, 0x1 },{ 24, 0x2 },{ 26, 0x3 },
> > +  { 28, 0x8 },{ 30, 0x5 },{ 32, 0x9 },{ 34, 0x6 },
> > +  { 36, 0x0A },{ 40, 0x40 },{ 44, 0x41 },{ 48, 0x42 },
> > +  { 52, 0x43 },{ 56, 0x48 },{ 60, 0x45 },{ 64, 0x49 },
> > +  { 68, 0x46 },{ 72, 0x4A },{ 80, 0x80 },{ 88, 0x81 },
> > +  { 96, 0x82 },{ 104, 0x83 },{ 112, 0x88 },{ 120, 0x85 },
> > +  { 128, 0x89 },{ 136, 0x86 },{ 144, 0x8A },{ 160, 0x8B },
> > +  { 176, 0x8C },{ 192, 0x90 },{ 208, 0x56 },{ 224, 0x91 },
> > +  { 240, 0x1F },{ 256, 0x92 },{ 272, 0x8F },{ 288, 0x93 },
> > +  { 320, 0x98 },{ 352, 0x95 },{ 384, 0x99 },{ 416, 0x96 },
> > +  { 448, 0x9A },{ 480, 0x5F },{ 512, 0x9B },{ 576, 0x9C },
> > +  { 640, 0xA0 },{ 768, 0xA1 },{ 896, 0xA2 },{ 960, 0x9F },
> > +  { 1024, 0xA3 },{ 1152, 0xA4 },{ 1280, 0xA8 },{ 1536, 0xA9 },
> > +  { 1792, 0xAA },{ 1920, 0xA7 },{ 2048, 0xAB },{ 2304, 0xAC },
> > +  { 2560, 0xAD },{ 3072, 0xB1 },{ 3584, 0xB2 },{ 3840, 0xAF },
> > +  { 4096, 0xB3 },{ 4608, 0xB4 },{ 5120, 0xB8 },{ 6144, 0xB9 },
> > +  { 7168, 0xBA },{ 7680, 0xB7 },{ 8192, 0xBB },{ 9216, 0xBC },
> > +  { 10240, 0xBD },{ 12288, 0xBE },{ 15360, 0xBF }
> > +};
> > +
> > +/**
> > +  ERR009203 : I2C may not work reliably with the default setting
>
> What erratum database does this ID refer to?
> I don't need access to it, but an explanation at the end of the file
> header comment would be helpful.
>
> > +
> > +  Description : The clocking circuitry of I2C module may not work reliably due to the slow
> > +                     rise time of SCL signal.
>
> First line too long (wrap after 'due'). Second line weird indentation.
>
> > +  Workaround : Enable the receiver digital filter by setting IBDBG[GLFLT_EN] to 1.
>
> Ideally, wrap this line after 'setting'.
>
> > +*/
> > +STATIC
> > +VOID
> > + I2cErratumA009203 (
>
> No indentation of function name.
> Explanation of where that A prefix of the erratum ID came from would
> also be appreciated.
>
> > +  IN UINTN  Base
> > +  )
> > +{
> > +  I2C_REGS *Regs;
> > +
> > +  Regs = (I2C_REGS *)Base;
> > +
> > +  MmioOr8 ( (UINTN)&Regs->Ibdbg, I2C_IBDBG_GLFLT_EN);
>
> No space before (UINTN).
>
> > +}
> > +
> > +/**
> > +  Early init I2C for reading the sysclk from I2c slave device.
> > +  I2c bus clock is determined from the clock input to I2c controller.
> > +  The clock input to I2c controller is derived from the sysclk.
> > +  sysclk is determined by clock generator, which is controller by i2c.
> > +
> > +  So, it's a chicken-egg problem to read the sysclk from clock generator.
> > +  To break this cycle (i.e. to read the sysclk), we setup the i2c bus clock to
> > +  lowest value, in the hope that it won't be out of clock generator's supported
> > +  i2c clock frequency. Once we have the correct sysclk, we can setup the correct
> > +  i2c bus clock.
> > +
> > +  @param[in] Base   Base Address of I2c controller's registers
> > +
> > +  @return  EFI_SUCCESS           successfuly setup the i2c bus for reading sysclk
> > +**/
> > +EFI_STATUS
> > +I2cEarlyInitialize (
> > +  IN UINTN  Base
> > +  )
> > +{
> > +  I2C_REGS *Regs;
> > +  UINT8    Ibc;
>
> This is a CamelCase project.
> "Regs" is clear enough, but Ibc needs to be written out.
> I'm going to guess it stands for i2c bus clock.
>
> Since we're in a function where the name starts with I2c, we don't
> need to be explicit about that. And we don't seem to be having any
> other clocks to keep track of in this - so how about just calling it
> "Clock"?
>
> But then from the way it's used, should is be called MaxClock?
>
> > +
> > +  Regs = (I2C_REGS *)Base;
> > +  if (FeaturePcdGet (PcdI2cErratumA009203)) {
> > +    I2cErratumA009203 (Base);
> > +  }
> > +
> > +  if (MmioRead8 ( (UINTN)&Regs->Ibdbg) & I2C_IBDBG_GLFLT_EN) {
>
> No space before (UINTN).
> In general, could you do a global search and replace for "( ("?
> I won't mention it again, but since it is used consistently, I expect
> to see much more of it in this set :)
>
> > +    Ibc = I2cClockDividerGlitchEnabled[ARRAY_SIZE (I2cClockDividerGlitchEnabled) - 1].Ibc;
> > +  } else {
> > +    Ibc = I2cClockDividerGlitchDisabled[ARRAY_SIZE (I2cClockDividerGlitchDisabled) - 1].Ibc;
>
> This is way too hard to read.
> Can you add a preprocessor macro for
>   X[ARRAY_SIZE (X) - 1] ?
> with a helpful name like ARRAY_LAST_ELEM or something?
>
> > +  }
> > +
> > +  MmioWrite8 ( (UINTN)&Regs->Ibfd, Ibc);
> > +  // Reset Module
> > +  MmioOr8 ( (UINTN)&Regs->Ibcr, I2C_IBCR_MDIS);
> > +  MmioAnd8 ( (UINTN)&Regs->Ibcr, ~(I2C_IBCR_IBIE | I2C_IBCR_DMAEN));
> > +  MmioAnd8 ( (UINTN)&Regs->Ibic, (UINT8)(~I2C_IBIC_BIIE));
> > +
> > +  return EFI_SUCCESS;
> > +}
> > +
> > +/**
> > +  Configure I2c bus to opearte at a given speed
>
> operate
>
> > +
> > +  @param[in] Base             Base Address of I2c controller's registers
> > +  @param[in] I2cBusClock  Input clock to I2c controller
> > +  @param[in] Speed           speed to be configured for I2c bus
>
> Align comments.
>
> > +**/
> > +EFI_STATUS
> > +I2cInitialize (
> > +  IN UINTN   Base,
> > +  IN UINT64  I2cBusClock,
> > +  IN UINT64  Speed
> > +  )
> > +{
> > +  I2C_REGS                 *Regs;
> > +  UINT16                   ClockDivider;
> > +  UINT8                    Ibc;
> > +  I2C_CLOCK_DIVIDER_PAIR   *ClockDividerPair;
> > +  UINT32                   ClockDividerPairSize;
> > +  UINT32                   Index;
> > +
> > +  Regs = (I2C_REGS *)Base;
> > +  if (FeaturePcdGet (PcdI2cErratumA009203)) {
> > +    I2cErratumA009203 (Base);
> > +  }
> > +
> > +  Ibc = 0;
> > +  ClockDivider = (I2cBusClock + Speed - 1) / Speed;
> > +
> > +  if (MmioRead8 ( (UINTN)&Regs->Ibdbg) & I2C_IBDBG_GLFLT_EN) {
> > +    ClockDividerPair = I2cClockDividerGlitchEnabled;
> > +    ClockDividerPairSize = ARRAY_SIZE (I2cClockDividerGlitchEnabled);
> > +  } else {
> > +    ClockDividerPair = I2cClockDividerGlitchDisabled;
> > +    ClockDividerPairSize = ARRAY_SIZE (I2cClockDividerGlitchDisabled);
> > +  }
> > +
> > +  if (ClockDivider > ClockDividerPair[ClockDividerPairSize - 1].Divider) {
> > +    Ibc = ClockDividerPair[ClockDividerPairSize - 1].Ibc;
> > +  } else {
> > +    for (Index = 0; Index < ClockDividerPairSize; Index++) {
> > +      if (ClockDividerPair[Index].Divider >= ClockDivider) {
> > +        Ibc = ClockDividerPair[Index].Ibc;
> > +        break;
> > +      }
> > +    }
> > +  }
> > +
> > +  MmioWrite8 ( (UINTN)&Regs->Ibfd, Ibc);
> > +  // Reset Module
> > +  MmioOr8 ( (UINTN)&Regs->Ibcr, I2C_IBCR_MDIS);
> > +  MmioAnd8 ( (UINTN)&Regs->Ibcr, ~(I2C_IBCR_IBIE | I2C_IBCR_DMAEN));
> > +  MmioAnd8 ( (UINTN)&Regs->Ibic, (UINT8)(~I2C_IBIC_BIIE));
> > +
> > +  return EFI_SUCCESS;
> > +}
> > +
> > +STATIC
> > +EFI_STATUS
> > +I2cBusTestBusBusy (
> > +  IN  I2C_REGS  *Regs,
> > +  IN  BOOLEAN   TestBusy
> > +  )
> > +{
> > +  UINT8  Index;
> > +  UINT8  Reg;
> > +
> > +  for (Index = 0; Index < 500; Index++) {
>
> What does looping over something 500 times signify?
> Is there a time period we're waiting for or is it just arbitrary?
> If it's just arbitrary, put it in a #define and name it accordingly.
>
> > +    Reg = MmioRead8 ( (UINTN)&Regs->Ibsr);
> > +
> > +    if (Reg & I2C_IBSR_IBAL) {
> > +      MmioWrite8 ( (UINTN)&Regs->Ibsr, Reg);
> > +      return EFI_NOT_READY;
> > +    }
> > +
> > +    if (TestBusy && (Reg & I2C_IBSR_IBB)) {
> > +      break;
> > +    }
> > +
> > +    if (!TestBusy && !(Reg & I2C_IBSR_IBB)) {
> > +      break;
> > +    }
> > +
> > +    MicroSecondDelay (1);
>
> Do we need a delay or do we need a barrier? Or do we need both?
>
> > +  }
> > +
> > +  if (Index == 500) {
> > +    return EFI_TIMEOUT;
> > +  } else {
> > +    return EFI_SUCCESS;
> > +  }
> > +}
> > +
> > +STATIC
> > +EFI_STATUS
> > +I2cTransferComplete (
> > +  IN  I2C_REGS  *Regs,
> > +  IN  BOOLEAN   TestRxAck
> > +)
> > +{
> > +  UINT8      Index;
> > +  UINT8      Reg;
> > +
> > +  for (Index = 0; Index < 500; Index++) {
>
> Same thing.
>
> > +    Reg = MmioRead8 ( (UINTN)&Regs->Ibsr);
> > +
> > +    if (Reg & I2C_IBSR_IBIF) {
> > +      // Write 1 to clear the IBIF field
> > +      MmioWrite8 ( (UINTN)&Regs->Ibsr, Reg);
> > +      break;
> > +    }
> > +
> > +    MicroSecondDelay (1);
>
> Do we need a delay or do we need a barrier? Or do we need both?
>
> > +  }
> > +
> > +  if (Index == 500) {
> > +    return EFI_TIMEOUT;
> > +  }
> > +
> > +  if (TestRxAck && (Reg & I2C_IBSR_RXAK)) {
> > +    return EFI_NO_RESPONSE;
> > +  }
> > +
> > +  if (Reg & I2C_IBSR_TCF) {
> > +    return EFI_SUCCESS;
> > +  }
> > +  return EFI_DEVICE_ERROR;
> > +}
> > +
> > +STATIC
> > +EFI_STATUS
> > +I2cRead (
> > +  IN  I2C_REGS           *Regs,
> > +  IN  UINT32             SlaveAddress,
> > +  IN  EFI_I2C_OPERATION  *Operation,
> > +  IN  BOOLEAN            IsLastOperation
> > +)
> > +{
> > +  EFI_STATUS Status;
> > +  UINTN      Index;
> > +
> > +  // Write Slave Address
> > +  MmioWrite8 ( (UINTN)&Regs->Ibdr, (SlaveAddress << BIT0) | BIT0);
> > +  Status = I2cTransferComplete (Regs, I2C_BUS_TEST_RX_ACK);
> > +  if (EFI_ERROR (Status)) {
> > +    return Status;
> > +  }
> > +  // select Receive mode.
> > +  MmioAnd8 ( (UINTN)&Regs->Ibcr, ~I2C_IBCR_TXRX);
> > +  // Perform a dummy read to initiate the receive operation.
> > +  MmioRead8 ( (UINTN)&Regs->Ibdr);
> > +
> > +  for (Index = 0; Index < Operation->LengthInBytes; Index++) {
> > +    Status = I2cTransferComplete (Regs, I2C_BUS_NO_TEST_RX_ACK);
> > +    if (EFI_ERROR (Status)) {
> > +      return Status;
> > +    }
> > +    if (Index == (Operation->LengthInBytes - 2)) {
> > +      // Set No ACK = 1
> > +      MmioOr8 ( (UINTN)&Regs->Ibcr, I2C_IBCR_NOACK);
> > +    } else if (Index == (Operation->LengthInBytes - 1)) {
> > +      if (!IsLastOperation) {
> > +        // select Transmit mode (for repeat start)
> > +        MmioOr8 ( (UINTN)&Regs->Ibcr, I2C_IBCR_TXRX);
> > +      } else {
> > +        // Generate Stop Signal
> > +        MmioAnd8 ( (UINTN)&Regs->Ibcr, ~(I2C_IBCR_MSSL | I2C_IBCR_TXRX));
> > +        Status = I2cBusTestBusBusy (Regs, I2C_BUS_TEST_IDLE);
> > +        if (EFI_ERROR (Status)) {
> > +          return Status;
> > +        }
> > +      }
> > +    }
> > +    Operation->Buffer[Index] = MmioRead8 ( (UINTN)&Regs->Ibdr);
> > +  }
> > +
> > +  return EFI_SUCCESS;
> > +}
> > +
> > +STATIC
> > +EFI_STATUS
> > +I2cWrite (
> > +  IN  I2C_REGS           *Regs,
> > +  IN  UINT32             SlaveAddress,
> > +  IN  EFI_I2C_OPERATION  *Operation
> > +)
> > +{
> > +  EFI_STATUS Status;
> > +  UINTN      Index;
> > +
> > +  // Write Slave Address
> > +  MmioWrite8 ( (UINTN)&Regs->Ibdr, (SlaveAddress << BIT0) & (UINT8)(~BIT0));
> > +  Status = I2cTransferComplete (Regs, I2C_BUS_TEST_RX_ACK);
> > +  if (EFI_ERROR (Status)) {
> > +    return Status;
> > +  }
> > +
> > +  // Write Data
> > +  for (Index = 0; Index < Operation->LengthInBytes; Index++) {
> > +    MmioWrite8 ( (UINTN)&Regs->Ibdr, Operation->Buffer[Index]);
> > +    Status = I2cTransferComplete (Regs, I2C_BUS_TEST_RX_ACK);
> > +    if (EFI_ERROR (Status)) {
> > +      return Status;
> > +    }
> > +  }
> > +  return EFI_SUCCESS;
> > +}
> > +
> > +STATIC
> > +EFI_STATUS
> > +I2cStop (
> > +  IN  I2C_REGS  *Regs
> > +  )
> > +{
> > +  EFI_STATUS Status;
> > +  UINT8      Reg;
> > +
> > +  Reg = MmioRead8 ( (UINTN)&Regs->Ibsr);
> > +  if (Reg & I2C_IBSR_IBB) {
> > +    // Generate Stop Signal
> > +    MmioAnd8 ( (UINTN)&Regs->Ibcr, ~(I2C_IBCR_MSSL | I2C_IBCR_TXRX));
> > +    Status = I2cBusTestBusBusy (Regs, I2C_BUS_TEST_IDLE);
> > +    if (EFI_ERROR (Status)) {
> > +      return Status;
> > +    }
> > +  }
> > +
> > +  // Disable I2c Controller
> > +  MmioOr8 ( (UINTN)&Regs->Ibcr, I2C_IBCR_MDIS);
> > +
> > +  return Status;
> > +}
> > +
> > +STATIC
> > +EFI_STATUS
> > +I2cStart (
> > +  IN  I2C_REGS  *Regs
> > +  )
> > +{
> > +  EFI_STATUS Status;
> > +
> > +  MmioOr8 ( (UINTN)&Regs->Ibsr, (I2C_IBSR_IBAL | I2C_IBSR_IBIF));
> > +  MmioAnd8 ( (UINTN)&Regs->Ibcr, (UINT8)(~I2C_IBCR_MDIS));
> > +
> > +  // Generate Start Signal
> > +  MmioOr8 ( (UINTN)&Regs->Ibcr, I2C_IBCR_MSSL);
> > +  Status = I2cBusTestBusBusy (Regs, I2C_BUS_TEST_BUSY);
> > +  if (EFI_ERROR (Status)) {
> > +    return Status;
> > +  }
> > +  MmioOr8 ( (UINTN)&Regs->Ibcr, I2C_IBCR_TXRX | I2C_IBCR_NOACK);
> > +  return Status;
> > +}
> > +
> > +/**
> > +  Transfer data to/from I2c slave device
> > +
> > +  @param[in] Base               Base Address of I2c controller's registers
> > +  @param[in] SlaveAddress   Slave Address from which data is to be read
> > +  @param[in] RequestPacket Pointer to an EFI_I2C_REQUEST_PACKET structure
> > +                                          describing the I2C transaction
> > +
> > +  @return  EFI_SUCCESS           successfuly setup the i2c bus for reading sysclk
> > +  @return  EFI_DEVICE_ERROR  There was an error while transferring data through I2c bus
> > +  @return  EFI_NO_RESPONSE    There was no Ack from i2c device
> > +  @return  EFI_TIMEOUT            I2c Bus is busy
> > +  @return  EFI_NOT_READY        I2c Bus Arbitration lost
> > +**/
> > +EFI_STATUS
> > +I2cBusXfer (
> > +  IN UINTN                  Base,
> > +  IN UINT32                 SlaveAddress,
> > +  IN EFI_I2C_REQUEST_PACKET *RequestPacket
> > +  )
> > +{
> > +  UINTN              Index;
> > +  I2C_REGS           *Regs;
> > +  EFI_I2C_OPERATION  *Operation;
> > +  EFI_STATUS         Status;
> > +  BOOLEAN            IsLastOperation;
> > +
> > +  Regs = (I2C_REGS *)Base;
> > +  IsLastOperation = FALSE;
> > +
> > +  Status = I2cBusTestBusBusy (Regs, I2C_BUS_TEST_IDLE);
> > +  if (EFI_ERROR (Status)) {
> > +    goto ErrorExit;
> > +  }
> > +
> > +  Status = I2cStart (Regs);
> > +  if (EFI_ERROR (Status)) {
> > +    goto ErrorExit;
> > +  }
> > +
> > +  for (Index = 0, Operation = RequestPacket->Operation;
> > +       Index < RequestPacket->OperationCount;
> > +       Index++, Operation++) {
> > +    if (Index == (RequestPacket->OperationCount - 1)) {
> > +      IsLastOperation = TRUE;
> > +    }
> > +    // Send repeat start after first transmit/recieve
> > +    if (Index) {
> > +      MmioOr8 ( (UINTN)&Regs->Ibcr, I2C_IBCR_RSTA);
> > +      Status = I2cBusTestBusBusy (Regs, I2C_BUS_TEST_BUSY);
> > +      if (EFI_ERROR (Status)) {
> > +        goto ErrorExit;
> > +      }
> > +    }
> > +    // Read/write data
> > +    if (Operation->Flags & I2C_FLAG_READ) {
> > +      Status = I2cRead (Regs, SlaveAddress, Operation, IsLastOperation);
> > +    } else {
> > +      Status = I2cWrite (Regs, SlaveAddress, Operation);
> > +    }
> > +    if (EFI_ERROR (Status)) {
> > +      goto ErrorExit;
> > +    }
> > +  }
> > +
> > +ErrorExit:
> > +
> > +  I2cStop (Regs);
> > +
> > +  return Status;
> > +}
> > +
> > +/**
> > +  Read a register from I2c slave device. This API is wrapper around I2cBusXfer
> > +
> > +  @param[in]   Base                               Base Address of I2c controller's registers
> > +  @param[in]   SlaveAddress                   Slave Address from which register value is to be read
> > +  @param[in]   RegAddress                     Register Address in Slave's memory map
> > +  @param[in]   RegAddressWidthInBytes  Number of bytes in RegAddress to send to I2c Slave
> > +                                                            for simple reads without any register, make this value = 0
> > +                                                            (RegAddress is don't care in that case)
> > +  @param[out] RegValue                         Value to be read from I2c slave's regiser
> > +  @param[in]   RegValueNumBytes          Number of bytes to read from I2c slave register
> > +
> > +  @return  EFI_SUCCESS           successfuly setup the i2c bus for reading sysclk
> > +  @return  EFI_DEVICE_ERROR  There was an error while transferring data through I2c bus
> > +  @return  EFI_NO_RESPONSE    There was no Ack from i2c device
> > +  @return  EFI_TIMEOUT            I2c Bus is busy
> > +  @return  EFI_NOT_READY        I2c Bus Arbitration lost
>
> Align comments. And try to keep line lengths no longer than 80 characters.
>
> > +**/
> > +EFI_STATUS
> > +I2cBusReadReg (
> > +  IN  UINTN  Base,
> > +  IN  UINT32 SlaveAddress,
> > +  IN  UINT64 RegAddress,
> > +  IN  UINT8  RegAddressWidthInBytes,
> > +  OUT UINT8  *RegValue,
> > +  IN  UINT8  RegValueNumBytes
> > +  )
> > +{
> > +  EFI_I2C_OPERATION       *Operations;
> > +  I2C_REG_REQUEST         RequestPacket;
> > +  UINTN                   OperationCount;
> > +  UINT8                   Address[8];
>
> Create a well named #define for that 8.
>
> > +  UINT8                   *Ptr;
>
> The name Ptr does not convey information.
> Give it a name that describes what it points to.
>
> > +  EFI_STATUS              Status;
> > +
> > +  ZeroMem (&RequestPacket, sizeof (RequestPacket));
> > +  OperationCount = 0;
> > +  Operations = RequestPacket.Operation;
> > +  Ptr = Address;
> > +
> > +  if (RegAddressWidthInBytes > ARRAY_SIZE (Address)) {
> > +    return EFI_INVALID_PARAMETER;
> > +  }
> > +
> > +  if (RegAddressWidthInBytes != 0) {
> > +    Operations[OperationCount].LengthInBytes = RegAddressWidthInBytes;
> > +    Operations[OperationCount].Buffer = Ptr;
> > +    while (RegAddressWidthInBytes--) {
> > +      *Ptr++ = RegAddress >> (8 * RegAddressWidthInBytes);
> > +    }
> > +    OperationCount++;
> > +  }
> > +
> > +  Operations[OperationCount].LengthInBytes = RegValueNumBytes;
> > +  Operations[OperationCount].Buffer = RegValue;
> > +  Operations[OperationCount].Flags = I2C_FLAG_READ;
> > +  OperationCount++;
> > +
> > +  RequestPacket.OperationCount = OperationCount;
> > +
> > +  Status = I2cBusXfer (Base, SlaveAddress, (EFI_I2C_REQUEST_PACKET *)&RequestPacket);
> > +
> > +  return Status;
> > +}
> > +
> > diff --git a/Silicon/NXP/Library/I2cLib/I2cLib.inf b/Silicon/NXP/Library/I2cLib/I2cLib.inf
> > new file mode 100644
> > index 0000000000..9c8aae100b
> > --- /dev/null
> > +++ b/Silicon/NXP/Library/I2cLib/I2cLib.inf
> > @@ -0,0 +1,30 @@
> > +#/**  @file
> > +#
> > +#  Copyright 2020 NXP
> > +#
> > +#  SPDX-License-Identifier: BSD-2-Clause
> > +#
> > +#**/
> > +
> > +[Defines]
> > +  INF_VERSION                    = 1.27
> > +  BASE_NAME                      = I2cLib
> > +  FILE_GUID                      = f22393b1-98b6-4067-9ec2-6aa436321f03
> > +  MODULE_TYPE                    = BASE
> > +  VERSION_STRING                 = 1.0
> > +  LIBRARY_CLASS                  = I2cLib
> > +
> > +[Packages]
> > +  MdePkg/MdePkg.dec
> > +  Silicon/NXP/NxpQoriqLs.dec
> > +
> > +[LibraryClasses]
> > +  TimerLib
> > +  IoLib
>
> Please sort library classes alphabetically.
>
> > +
> > +[Sources.common]
> > +  I2cLib.c
> > +
> > +[FeaturePcd]
> > +  gNxpQoriqLsTokenSpaceGuid.PcdI2cErratumA009203
> > +
> > diff --git a/Silicon/NXP/Library/I2cLib/I2cLibInternal.h b/Silicon/NXP/Library/I2cLib/I2cLibInternal.h
> > new file mode 100644
> > index 0000000000..14be9cb740
> > --- /dev/null
> > +++ b/Silicon/NXP/Library/I2cLib/I2cLibInternal.h
> > @@ -0,0 +1,95 @@
> > +/** @file
> > +  I2c Lib to control I2c controller.
> > +
> > +  Copyright 2020 NXP
> > +  SPDX-License-Identifier: BSD-2-Clause-Patent
> > +
> > +**/
> > +
> > +#ifndef __I2C_LIB_INTERNAL_H__
> > +#define __I2C_LIB_INTERNAL_H__
>
> Please drop leading __ in header guards.
>
> > +
> > +#include <Pi/PiI2c.h>
> > +#include <Uefi.h>
> > +
> > +/* Module Disable
> > + * 0b - The module is enabled. You must clear this field before any other IBCR fields have any effect.
> > + * 1b - The module is reset and disabled. This is the power-on reset situation. When high, the
> > + * interface is held in reset, but registers can still be accessed. Status register fields (IBSR) are not
> > + * valid when the module is disabled.
>
> Please re-wrap lines at 80 characters throughout this file.
>                                                                                   ^
>
> > + */
> > +#define I2C_IBCR_MDIS      BIT7
> > +// I2c Bus Interrupt Enable
> > +#define I2C_IBCR_IBIE      BIT6
> > +/* Master / Slave Mode 0b - Slave mode 1b - Master mode
> > + * When you change this field from 0 to 1, the module generates a START signal on the bus and selects the
> > + * master mode. When you change this field from 1 to 0, the module generates a STOP signal and changes
> > + * the operation mode from master to slave. You should generate a STOP signal only if IBSR[IBIF]=1. The
> > + * module clears this field without generating a STOP signal when the master loses arbitration.
> > +*/
> > +#define I2C_IBCR_MSSL      BIT5
> > +// 0b - Receive 1b - Transmit
> > +#define I2C_IBCR_TXRX      BIT4
> > +/* Data acknowledge disable
> > + * Values written to this field are only used when the I2C module is a receiver, not a transmitter.
> > + * 0b - The module sends an acknowledge signal to the bus at the 9th clock bit after receiving one
> > + * byte of data.
> > + * 1b - The module does not send an acknowledge-signal response (that is, acknowledge bit = 1).
> > + */
> > +#define I2C_IBCR_NOACK     BIT3
> > +/* Repeat START
> > + * If the I2C module is the current bus master, and you program RSTA=1, the I2C module generates a
> > + * repeated START condition. This field always reads as a 0. If you attempt a repeated START at the wrong
> > + * time—if the bus is owned by another master—the result is loss of arbitration.
> > + */
> > +#define I2C_IBCR_RSTA      BIT2
> > +// DMA enable
> > +#define I2C_IBCR_DMAEN     BIT1
> > +
> > +// Transfer Complete
> > +#define I2C_IBSR_TCF       BIT7
> > +// I2C bus Busy. 0b - Bus is idle, 1b - Bus is busy
> > +#define I2C_IBSR_IBB       BIT5
> > +// Arbitration Lost. software must clear this field by writing a one to it.
> > +#define I2C_IBSR_IBAL      BIT4
> > +// I2C bus interrupt flag
> > +#define I2C_IBSR_IBIF      BIT1
> > +// Received acknowledge 0b - Acknowledge received 1b - No acknowledge received
> > +#define I2C_IBSR_RXAK      BIT0
> > +
> > +//Bus idle interrupt enable
> > +#define I2C_IBIC_BIIE      BIT7
> > +
> > +// Glitch filter enable
> > +#define I2C_IBDBG_GLFLT_EN BIT3
> > +
> > +#define I2C_BUS_TEST_BUSY       TRUE
> > +#define I2C_BUS_TEST_IDLE       !I2C_BUS_TEST_BUSY
> > +#define I2C_BUS_TEST_RX_ACK     TRUE
> > +#define I2C_BUS_NO_TEST_RX_ACK  !I2C_BUS_TEST_RX_ACK
> > +
> > +typedef struct _I2C_REGS {
> > +  UINT8 Ibad; // I2c Bus Address Register
> > +  UINT8 Ibfd; // I2c Bus Frequency Dividor Register
> > +  UINT8 Ibcr; // I2c Bus Control Register
> > +  UINT8 Ibsr; // I2c Bus Status Register
> > +  UINT8 Ibdr; // I2C Bus Data I/O Register
> > +  UINT8 Ibic; // I2C Bus Interrupt Config Register
> > +  UINT8 Ibdbg; // I2C Bus Debug Register
> > +} I2C_REGS;
> > +
> > +/*
> > + * sorted list of clock divider, register value pairs
> > + */
> > +typedef struct _I2C_CLOCK_DIVIDER_PAIR {
> > +  UINT16  Divider;
> > +  UINT16  Ibc;
>
> CamelCase - please expand Ibc.
>
> /
>     Leif
>
> > +} I2C_CLOCK_DIVIDER_PAIR;
> > +
> > +typedef struct {
> > +  UINTN                           OperationCount;
> > +  EFI_I2C_OPERATION               Operation[2];
> > +} I2C_REG_REQUEST;
> > +
> > +#endif // __I2C_LIB_INTERNAL_H__
> > +
> > diff --git a/Silicon/NXP/NxpQoriqLs.dec b/Silicon/NXP/NxpQoriqLs.dec
> > index 764b9bb0e2..4a1cfb3e27 100644
> > --- a/Silicon/NXP/NxpQoriqLs.dec
> > +++ b/Silicon/NXP/NxpQoriqLs.dec
> > @@ -1,6 +1,6 @@
> >  #  @file.
> >  #
> > -#  Copyright 2017-2019 NXP
> > +#  Copyright 2017-2020 NXP
> >  #
> >  #  SPDX-License-Identifier: BSD-2-Clause-Patent
> >  #
> > @@ -13,6 +13,10 @@
> >  [Includes]
> >    Include
> >
> > +[LibraryClasses]
> > +  ##  @libraryclass  Provides services to read/write to I2c devices
> > +  I2cLib|Include/Library/I2cLib.h
> > +
> >  [Guids.common]
> >    gNxpQoriqLsTokenSpaceGuid      = {0x98657342, 0x4aee, 0x4fc6, {0xbc, 0xb5, 0xff, 0x45, 0xb7, 0xa8, 0x71, 0xf2}}
> >    gNxpNonDiscoverableI2cMasterGuid = { 0x5f2c099c, 0x54a3, 0x4dd4, {0x9e, 0xc5, 0xe9, 0x12, 0x8c, 0x36, 0x81, 0x6a}}
> > @@ -101,3 +105,7 @@
> >    gNxpQoriqLsTokenSpaceGuid.PcdPciLutBigEndian|FALSE|BOOLEAN|0x00000312
> >    gNxpQoriqLsTokenSpaceGuid.PcdWatchdogBigEndian|FALSE|BOOLEAN|0x00000313
> >    gNxpQoriqLsTokenSpaceGuid.PcdIfcBigEndian|FALSE|BOOLEAN|0x00000314
> > +
> > +[PcdsFeatureFlag]
> > +  gNxpQoriqLsTokenSpaceGuid.PcdI2cErratumA009203|FALSE|BOOLEAN|0x00000315
> > +
> > --
> > 2.17.1
> >
>
> 
>