From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f68.google.com (mail-wr1-f68.google.com [209.85.221.68]) by mx.groups.io with SMTP id smtpd.web10.6413.1581248986227221669 for ; Sun, 09 Feb 2020 03:49:47 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=C/N85LFX; spf=pass (domain: linaro.org, ip: 209.85.221.68, mailfrom: ard.biesheuvel@linaro.org) Received: by mail-wr1-f68.google.com with SMTP id t3so4074038wru.7 for ; Sun, 09 Feb 2020 03:49:45 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=uaH+n9mjfTvJnRoQszVpOKyFMepjDUy1O2z8PPQgvzI=; b=C/N85LFXYoJZYwXoR7PnKKHZBraanOMO9CpAV5h5qFt7RukerP5DJfM+o5V9dPu6sP WwEuD7vEFmOSIWX9d21yYzX7wBe6LBpv/LWF560OFqN/p4A/qeEbxjR0/oy7R0gmyMel 66LLPc4/xTah2lu4YvyBtfppzH/1G7JQuLBeMNPRg+/XkjOT2tgERyIha2sMefp4tUpt I3tMMY54FJ0fsF3Y4sS38yfmuZdemeCPJqO2wTPurAJLtRNdiWlkcR0hWGQy4g+8qdXG 83okibrR+iTIwfCm2GNd+blDszBkx5sknTDlVTt4c1h4tto1npcw3b9u1MMx/0jKZdvi 9DrA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=uaH+n9mjfTvJnRoQszVpOKyFMepjDUy1O2z8PPQgvzI=; b=lPHlt5TVgt1wfFmj4juTE3D8T6OyHh6FbFhZpWIjW0ndB9l/WOijhzeuufw+UXaKxx BQY8hsIvueWLYh04mZfmJ83L2A0KnUdHjHP05m8jXlcUym0mry/OsvLNDn2VNaxtSfMY drEljED23XplSHWV9d6GYf1Kf3MnQTSliZg2PXP1psTZlsTMDTkZlHMAhS+96/e/ffxS tF5/f4xKPhqJzxmfVo4kcGln3KNv+oXQp72jTKg/XLFO+j+m6nlrWWrU8hXnDTJfTn1E ch2/4LDbciAXZMhISWNX6g8jCsTJErLah3/+tM5vxvsq1xgRh59piF42qAS56oHCF3uc u2aw== X-Gm-Message-State: APjAAAWv/vFO9on3LJzQZ6XkQQLl2wkQk3sNsln9Tl9+efa6mP7NwSZx CXb2yNfXtkvaqnIvJDd1bCBgPPgpdG+sH9D4gfKAStt6WqG6Pg== X-Google-Smtp-Source: APXvYqw+jMs+OBwSNfwtva0+q5cS5XUaQ6uv0PzSMl+c4EdGlX9BANTbJPgBWtnMv6ALTfg/9jl+E0ttHfVut2Wo2fI= X-Received: by 2002:a5d:65cf:: with SMTP id e15mr10740336wrw.126.1581248983841; Sun, 09 Feb 2020 03:49:43 -0800 (PST) MIME-Version: 1.0 References: <20200207124328.8723-1-pankaj.bansal@nxp.com> <20200207124328.8723-2-pankaj.bansal@nxp.com> <20200208171357.GD23627@bivouac.eciton.net> In-Reply-To: <20200208171357.GD23627@bivouac.eciton.net> From: "Ard Biesheuvel" Date: Sun, 9 Feb 2020 12:49:33 +0100 Message-ID: Subject: Re: [edk2-devel] [PATCH 01/19] Silicon/NXP: Add I2c lib To: edk2-devel-groups-io , Leif Lindholm Cc: Pankaj Bansal , Meenakshi Aggarwal , Michael D Kinney , Varun Sethi Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Sat, 8 Feb 2020 at 18:14, Leif Lindholm 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 > > --- > > 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 sig= nals. > > + * This gives the I2C module greater immunity against glitches in the= I2C signals. > > + */ > > +STATIC I2C_CLOCK_DIVIDER_PAIR I2cClockDividerGlitchEnabled[] =3D { > > 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 sig= nals. > > + * This gives the I2C module greater immunity against glitches in the= I2C signals. > > + */ > > +STATIC I2C_CLOCK_DIVIDER_PAIR I2cClockDividerGlitchDisabled[] =3D { > > 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 rel= iably 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[GL= FLT_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 =3D (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 genera= tor. > > + 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 re= ading 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 =3D (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 =3D I2cClockDividerGlitchEnabled[ARRAY_SIZE (I2cClockDividerG= litchEnabled) - 1].Ibc; > > + } else { > > + Ibc =3D I2cClockDividerGlitchDisabled[ARRAY_SIZE (I2cClockDivider= GlitchDisabled) - 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 regist= ers > > + @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 =3D (I2C_REGS *)Base; > > + if (FeaturePcdGet (PcdI2cErratumA009203)) { > > + I2cErratumA009203 (Base); > > + } > > + > > + Ibc =3D 0; > > + ClockDivider =3D (I2cBusClock + Speed - 1) / Speed; > > + > > + if (MmioRead8 ( (UINTN)&Regs->Ibdbg) & I2C_IBDBG_GLFLT_EN) { > > + ClockDividerPair =3D I2cClockDividerGlitchEnabled; > > + ClockDividerPairSize =3D ARRAY_SIZE (I2cClockDividerGlitchEnabled= ); > > + } else { > > + ClockDividerPair =3D I2cClockDividerGlitchDisabled; > > + ClockDividerPairSize =3D ARRAY_SIZE (I2cClockDividerGlitchDisable= d); > > + } > > + > > + if (ClockDivider > ClockDividerPair[ClockDividerPairSize - 1].Divid= er) { > > + Ibc =3D ClockDividerPair[ClockDividerPairSize - 1].Ibc; > > + } else { > > + for (Index =3D 0; Index < ClockDividerPairSize; Index++) { > > + if (ClockDividerPair[Index].Divider >=3D ClockDivider) { > > + Ibc =3D 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 =3D 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 =3D 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 =3D=3D 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 =3D 0; Index < 500; Index++) { > > Same thing. > > > + Reg =3D 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 =3D=3D 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 =3D 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 =3D 0; Index < Operation->LengthInBytes; Index++) { > > + Status =3D I2cTransferComplete (Regs, I2C_BUS_NO_TEST_RX_ACK); > > + if (EFI_ERROR (Status)) { > > + return Status; > > + } > > + if (Index =3D=3D (Operation->LengthInBytes - 2)) { > > + // Set No ACK =3D 1 > > + MmioOr8 ( (UINTN)&Regs->Ibcr, I2C_IBCR_NOACK); > > + } else if (Index =3D=3D (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_TXR= X)); > > + Status =3D I2cBusTestBusBusy (Regs, I2C_BUS_TEST_IDLE); > > + if (EFI_ERROR (Status)) { > > + return Status; > > + } > > + } > > + } > > + Operation->Buffer[Index] =3D 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 =3D I2cTransferComplete (Regs, I2C_BUS_TEST_RX_ACK); > > + if (EFI_ERROR (Status)) { > > + return Status; > > + } > > + > > + // Write Data > > + for (Index =3D 0; Index < Operation->LengthInBytes; Index++) { > > + MmioWrite8 ( (UINTN)&Regs->Ibdr, Operation->Buffer[Index]); > > + Status =3D 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 =3D MmioRead8 ( (UINTN)&Regs->Ibsr); > > + if (Reg & I2C_IBSR_IBB) { > > + // Generate Stop Signal > > + MmioAnd8 ( (UINTN)&Regs->Ibcr, ~(I2C_IBCR_MSSL | I2C_IBCR_TXRX)); > > + Status =3D 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 =3D 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 regi= sters > > + @param[in] SlaveAddress Slave Address from which data is to be re= ad > > + @param[in] RequestPacket Pointer to an EFI_I2C_REQUEST_PACKET struc= ture > > + describing the I2C transact= ion > > + > > + @return EFI_SUCCESS successfuly setup the i2c bus for re= ading sysclk > > + @return EFI_DEVICE_ERROR There was an error while transferring da= ta 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 =3D (I2C_REGS *)Base; > > + IsLastOperation =3D FALSE; > > + > > + Status =3D I2cBusTestBusBusy (Regs, I2C_BUS_TEST_IDLE); > > + if (EFI_ERROR (Status)) { > > + goto ErrorExit; > > + } > > + > > + Status =3D I2cStart (Regs); > > + if (EFI_ERROR (Status)) { > > + goto ErrorExit; > > + } > > + > > + for (Index =3D 0, Operation =3D RequestPacket->Operation; > > + Index < RequestPacket->OperationCount; > > + Index++, Operation++) { > > + if (Index =3D=3D (RequestPacket->OperationCount - 1)) { > > + IsLastOperation =3D TRUE; > > + } > > + // Send repeat start after first transmit/recieve > > + if (Index) { > > + MmioOr8 ( (UINTN)&Regs->Ibcr, I2C_IBCR_RSTA); > > + Status =3D I2cBusTestBusBusy (Regs, I2C_BUS_TEST_BUSY); > > + if (EFI_ERROR (Status)) { > > + goto ErrorExit; > > + } > > + } > > + // Read/write data > > + if (Operation->Flags & I2C_FLAG_READ) { > > + Status =3D I2cRead (Regs, SlaveAddress, Operation, IsLastOperat= ion); > > + } else { > > + Status =3D 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 I= 2cBusXfer > > + > > + @param[in] Base Base Address of I2c= controller's registers > > + @param[in] SlaveAddress Slave Address from whic= h register value is to be read > > + @param[in] RegAddress Register Address in Sla= ve's memory map > > + @param[in] RegAddressWidthInBytes Number of bytes in RegAddress = to send to I2c Slave > > + for simpl= e reads without any register, make this value =3D 0 > > + (RegAddre= ss 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 re= ading sysclk > > + @return EFI_DEVICE_ERROR There was an error while transferring da= ta 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 character= s. > > > +**/ > > +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 =3D 0; > > + Operations =3D RequestPacket.Operation; > > + Ptr =3D Address; > > + > > + if (RegAddressWidthInBytes > ARRAY_SIZE (Address)) { > > + return EFI_INVALID_PARAMETER; > > + } > > + > > + if (RegAddressWidthInBytes !=3D 0) { > > + Operations[OperationCount].LengthInBytes =3D RegAddressWidthInByt= es; > > + Operations[OperationCount].Buffer =3D Ptr; > > + while (RegAddressWidthInBytes--) { > > + *Ptr++ =3D RegAddress >> (8 * RegAddressWidthInBytes); > > + } > > + OperationCount++; > > + } > > + > > + Operations[OperationCount].LengthInBytes =3D RegValueNumBytes; > > + Operations[OperationCount].Buffer =3D RegValue; > > + Operations[OperationCount].Flags =3D I2C_FLAG_READ; > > + OperationCount++; > > + > > + RequestPacket.OperationCount =3D OperationCount; > > + > > + Status =3D I2cBusXfer (Base, SlaveAddress, (EFI_I2C_REQUEST_PACKET = *)&RequestPacket); > > + > > + return Status; > > +} > > + > > diff --git a/Silicon/NXP/Library/I2cLib/I2cLib.inf b/Silicon/NXP/Libra= ry/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 =3D 1.27 > > + BASE_NAME =3D I2cLib > > + FILE_GUID =3D f22393b1-98b6-4067-9ec2-6aa43632= 1f03 > > + MODULE_TYPE =3D BASE > > + VERSION_STRING =3D 1.0 > > + LIBRARY_CLASS =3D 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 > > +#include > > + > > +/* Module Disable > > + * 0b - The module is enabled. You must clear this field before any o= ther 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. S= tatus 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 STA= RT signal on the bus and selects the > > + * master mode. When you change this field from 1 to 0, the module ge= nerates a STOP signal and changes > > + * the operation mode from master to slave. You should generate a STO= P signal only if IBSR[IBIF]=3D1. 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 =3D 1). > > + */ > > +#define I2C_IBCR_NOACK BIT3 > > +/* Repeat START > > + * If the I2C module is the current bus master, and you program RSTA= =3D1, the I2C module generates a > > + * repeated START condition. This field always reads as a 0. If you a= ttempt a repeated START at the wrong > > + * time=E2=80=94if the bus is owned by another master=E2=80=94the res= ult 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 =3D {0x98657342, 0x4aee, 0x4fc6, {0x= bc, 0xb5, 0xff, 0x45, 0xb7, 0xa8, 0x71, 0xf2}} > > gNxpNonDiscoverableI2cMasterGuid =3D { 0x5f2c099c, 0x54a3, 0x4dd4, = {0x9e, 0xc5, 0xe9, 0x12, 0x8c, 0x36, 0x81, 0x6a}} > > @@ -101,3 +105,7 @@ > > gNxpQoriqLsTokenSpaceGuid.PcdPciLutBigEndian|FALSE|BOOLEAN|0x000003= 12 > > gNxpQoriqLsTokenSpaceGuid.PcdWatchdogBigEndian|FALSE|BOOLEAN|0x0000= 0313 > > gNxpQoriqLsTokenSpaceGuid.PcdIfcBigEndian|FALSE|BOOLEAN|0x00000314 > > + > > +[PcdsFeatureFlag] > > + gNxpQoriqLsTokenSpaceGuid.PcdI2cErratumA009203|FALSE|BOOLEAN|0x0000= 0315 > > + > > -- > > 2.17.1 > > > >=20 >