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

2018-04-23 Thread Ard Biesheuvel 
On 23 April 2018 at 17:50, Meenakshi Aggarwal
<meenakshi.aggar...@nxp.com> wrote:
> Hi Ard
>
>> -Original Message-
>> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
>> Sent: Monday, April 23, 2018 7:10 PM
>> To: Meenakshi Aggarwal <meenakshi.aggar...@nxp.com>
>> Cc: Leif Lindholm <leif.lindh...@linaro.org>; edk2-devel@lists.01.org; Udit
>> Kumar <udit.ku...@nxp.com>; Varun Sethi <v.se...@nxp.com>
>> Subject: Re: [PATCH edk2-platforms 05/39] Silicon/NXP: Add support for I2c
>> driver
>>
>> On 23 April 2018 at 12:34, Meenakshi Aggarwal
>> <meenakshi.aggar...@nxp.com> wrote:
>> > Hi Leif
>> >
>> >> -Original Message-
>> >> From: Leif Lindholm [mailto:leif.lindh...@linaro.org]
>> >> Sent: Monday, April 23, 2018 2:08 PM
>> >> To: Meenakshi Aggarwal <meenakshi.aggar...@nxp.com>
>> >> Cc: ard.biesheu...@linaro.org; edk2-devel@lists.01.org; Udit Kumar
>> >> <udit.ku...@nxp.com>; Varun Sethi <v.se...@nxp.com>
>> >> Subject: Re: [PATCH edk2-platforms 05/39] Silicon/NXP: Add support for
>> I2c
>> >> driver
>> >>
>> >> On Mon, Apr 23, 2018 at 08:21:22AM +, Meenakshi Aggarwal wrote:
>> >> > > > +/**
>> >> > > > +  Function to read data using i2c bus
>> >> > > > +
>> >> > > > +  @param   I2cBus  I2c Controller number
>> >> > > > +  @param   ChipAddress of slave device from where data 
>> >> > > > to
>> be
>> >> read
>> >> > > > +  @param   Offset  Offset of slave memory
>> >> > > > +  @param   AlenAddress 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  UINT8Chip,
>> >> > > > +  IN  UINT32   Offset,
>> >> > > > +  IN  UINT32   Alen,
>> >> > > > +  IN  UINT8*Buffer,
>> >> > > > +  IN  UINT32   Len
>> >> > > > +  )
>> >> > > > +{
>> >> > > > +  EFI_STATUS   Status;
>> >> > > > +  UINT32   Temp;
>> >> > > > +  INT32I;
>> >> > > > +  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.
>> >>
>> >> What it gives is the ability to cover more than one controller by this
>> >> driver, regardless of whether you need it for this particular platform
>> >> port or not.
>> >>
>> > Current board needs one controller.
>>
>> It is not about what the board wants. It is about reusing code.
>>
> Ideally, we should reuse as much as possible,
>
>> If you implemented a driver using the UEFI driver model, you get all
>> the binding machinery for free, and you only need to declare the
>> existence of a controller and the core will bind and discover drivers.
>
> But in c

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

2018-04-23 Thread Meenakshi Aggarwal 
Hi Ard

> -Original Message-
> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> Sent: Monday, April 23, 2018 7:10 PM
> To: Meenakshi Aggarwal <meenakshi.aggar...@nxp.com>
> Cc: Leif Lindholm <leif.lindh...@linaro.org>; edk2-devel@lists.01.org; Udit
> Kumar <udit.ku...@nxp.com>; Varun Sethi <v.se...@nxp.com>
> Subject: Re: [PATCH edk2-platforms 05/39] Silicon/NXP: Add support for I2c
> driver
> 
> On 23 April 2018 at 12:34, Meenakshi Aggarwal
> <meenakshi.aggar...@nxp.com> wrote:
> > Hi Leif
> >
> >> -Original Message-
> >> From: Leif Lindholm [mailto:leif.lindh...@linaro.org]
> >> Sent: Monday, April 23, 2018 2:08 PM
> >> To: Meenakshi Aggarwal <meenakshi.aggar...@nxp.com>
> >> Cc: ard.biesheu...@linaro.org; edk2-devel@lists.01.org; Udit Kumar
> >> <udit.ku...@nxp.com>; Varun Sethi <v.se...@nxp.com>
> >> Subject: Re: [PATCH edk2-platforms 05/39] Silicon/NXP: Add support for
> I2c
> >> driver
> >>
> >> On Mon, Apr 23, 2018 at 08:21:22AM +, Meenakshi Aggarwal wrote:
> >> > > > +/**
> >> > > > +  Function to read data using i2c bus
> >> > > > +
> >> > > > +  @param   I2cBus  I2c Controller number
> >> > > > +  @param   ChipAddress of slave device from where data 
> >> > > > to
> be
> >> read
> >> > > > +  @param   Offset  Offset of slave memory
> >> > > > +  @param   AlenAddress 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  UINT8Chip,
> >> > > > +  IN  UINT32   Offset,
> >> > > > +  IN  UINT32   Alen,
> >> > > > +  IN  UINT8*Buffer,
> >> > > > +  IN  UINT32   Len
> >> > > > +  )
> >> > > > +{
> >> > > > +  EFI_STATUS   Status;
> >> > > > +  UINT32   Temp;
> >> > > > +  INT32I;
> >> > > > +  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.
> >>
> >> What it gives is the ability to cover more than one controller by this
> >> driver, regardless of whether you need it for this particular platform
> >> port or not.
> >>
> > Current board needs one controller.
> 
> It is not about what the board wants. It is about reusing code.
> 
Ideally, we should reuse as much as possible,

> If you implemented a driver using the UEFI driver model, you get all
> the binding machinery for free, and you only need to declare the
> existence of a controller and the core will bind and discover drivers.

But in case of this particular driver, even if we are using UEFI driver model. 
Then from code perspective more or less, we need same code to declare i2c master
Protocol.

> This also allows you to unbind a driver from a single controller, for
> instance, and keeping the other connections alive.
> 


I don't see any case in which we need to unbind driver for i2c.

Coming back to implementation part,  there are some 
advantages using UEFI driver mo

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

2018-04-23 Thread Ard Biesheuvel 
On 23 April 2018 at 12:34, Meenakshi Aggarwal
<meenakshi.aggar...@nxp.com> wrote:
> Hi Leif
>
>> -Original Message-
>> From: Leif Lindholm [mailto:leif.lindh...@linaro.org]
>> Sent: Monday, April 23, 2018 2:08 PM
>> To: Meenakshi Aggarwal <meenakshi.aggar...@nxp.com>
>> Cc: ard.biesheu...@linaro.org; edk2-devel@lists.01.org; Udit Kumar
>> <udit.ku...@nxp.com>; Varun Sethi <v.se...@nxp.com>
>> Subject: Re: [PATCH edk2-platforms 05/39] Silicon/NXP: Add support for I2c
>> driver
>>
>> On Mon, Apr 23, 2018 at 08:21:22AM +, Meenakshi Aggarwal wrote:
>> > > > +/**
>> > > > +  Function to read data using i2c bus
>> > > > +
>> > > > +  @param   I2cBus  I2c Controller number
>> > > > +  @param   ChipAddress of slave device from where data to 
>> > > > be
>> read
>> > > > +  @param   Offset  Offset of slave memory
>> > > > +  @param   AlenAddress 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  UINT8Chip,
>> > > > +  IN  UINT32   Offset,
>> > > > +  IN  UINT32   Alen,
>> > > > +  IN  UINT8*Buffer,
>> > > > +  IN  UINT32   Len
>> > > > +  )
>> > > > +{
>> > > > +  EFI_STATUS   Status;
>> > > > +  UINT32   Temp;
>> > > > +  INT32I;
>> > > > +  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.
>>
>> What it gives is the ability to cover more than one controller by this
>> driver, regardless of whether you need it for this particular platform
>> port or not.
>>
> Current board needs one controller.

It is not about what the board wants. It is about reusing code.

If you implemented a driver using the UEFI driver model, you get all
the binding machinery for free, and you only need to declare the
existence of a controller and the core will bind and discover drivers.
This also allows you to unbind a driver from a single controller, for
instance, and keeping the other connections alive.

> In case of multiple controller, we can use a loop to install multiple 
> protocols and
> If needed then our preference would be to use I2c enumeration protocol.
> This will allow to use correct controller for connected devices.
>

I2C enumeration protocol has nothing to do with this.

> With sample implementation of SynQuacer code,
> Please advise, how a right controller is being connected to driver
> e.g. we are registering two controllers mI2c0Desc and mI2c1Desc and
> both are exporting same protocols.
> Its user, RTC lib just checking  gEfiI2cMasterProtocolGuid. There is 
> possibility
> to connect with any of the controller. I dont see in code where it is 
> assuring connection with right controller.
> And how we can assure we are connecting to the correct controller.
>

The PCF8563 RTC driver has a special GUIDed protocol that identifies
the I2C controller that has the RTC connected to it. This goes outside
of the UDM because RTC is an architectural protocol. Note that the
SynQuacer I2C driver is a DXE_RUNTIME_DRIVER module that supports boot
time and runtime I2C support for controllers, the latter especially
for things like RTC and varstore support over I2C.

>> > There also base address is hard coded SYNQUACER_I2C1_BASE in
>> > PlatformDxe driver.
>>
>> Yes, the PlatformDxe driver statically instantiates dynamic drivers
>> for non-discoverable buses. But there is no longer any hard-coded
>> addresses in the device driver itself.
>>
>> /
>> Leif
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

2018-04-23 Thread Meenakshi Aggarwal 
Hi Leif

> -Original Message-
> From: Leif Lindholm [mailto:leif.lindh...@linaro.org]
> Sent: Monday, April 23, 2018 2:08 PM
> To: Meenakshi Aggarwal <meenakshi.aggar...@nxp.com>
> Cc: ard.biesheu...@linaro.org; edk2-devel@lists.01.org; Udit Kumar
> <udit.ku...@nxp.com>; Varun Sethi <v.se...@nxp.com>
> Subject: Re: [PATCH edk2-platforms 05/39] Silicon/NXP: Add support for I2c
> driver
> 
> On Mon, Apr 23, 2018 at 08:21:22AM +, Meenakshi Aggarwal wrote:
> > > > +/**
> > > > +  Function to read data using i2c bus
> > > > +
> > > > +  @param   I2cBus  I2c Controller number
> > > > +  @param   ChipAddress of slave device from where data to 
> > > > be
> read
> > > > +  @param   Offset  Offset of slave memory
> > > > +  @param   AlenAddress 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  UINT8Chip,
> > > > +  IN  UINT32   Offset,
> > > > +  IN  UINT32   Alen,
> > > > +  IN  UINT8*Buffer,
> > > > +  IN  UINT32   Len
> > > > +  )
> > > > +{
> > > > +  EFI_STATUS   Status;
> > > > +  UINT32   Temp;
> > > > +  INT32I;
> > > > +  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.
> 
> What it gives is the ability to cover more than one controller by this
> driver, regardless of whether you need it for this particular platform
> port or not.
> 
Current board needs one controller.
In case of multiple controller, we can use a loop to install multiple protocols 
and 
If needed then our preference would be to use I2c enumeration protocol. 
This will allow to use correct controller for connected devices. 

With sample implementation of SynQuacer code, 
Please advise, how a right controller is being connected to driver 
e.g. we are registering two controllers mI2c0Desc and mI2c1Desc and 
both are exporting same protocols.
Its user, RTC lib just checking  gEfiI2cMasterProtocolGuid. There is 
possibility 
to connect with any of the controller. I dont see in code where it is assuring 
connection with right controller.
And how we can assure we are connecting to the correct controller.

> > There also base address is hard coded SYNQUACER_I2C1_BASE in
> > PlatformDxe driver.
> 
> Yes, the PlatformDxe driver statically instantiates dynamic drivers
> for non-discoverable buses. But there is no longer any hard-coded
> addresses in the device driver itself.
> 
> /
> Leif
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

2018-04-23 Thread Leif Lindholm 
On Mon, Apr 23, 2018 at 08:21:22AM +, Meenakshi Aggarwal wrote:
> > > +/**
> > > +  Function to read data using i2c bus
> > > +
> > > +  @param   I2cBus  I2c Controller number
> > > +  @param   ChipAddress of slave device from where data to be 
> > > read
> > > +  @param   Offset  Offset of slave memory
> > > +  @param   AlenAddress 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  UINT8Chip,
> > > +  IN  UINT32   Offset,
> > > +  IN  UINT32   Alen,
> > > +  IN  UINT8*Buffer,
> > > +  IN  UINT32   Len
> > > +  )
> > > +{
> > > +  EFI_STATUS   Status;
> > > +  UINT32   Temp;
> > > +  INT32I;
> > > +  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.

What it gives is the ability to cover more than one controller by this
driver, regardless of whether you need it for this particular platform
port or not.

> There also base address is hard coded SYNQUACER_I2C1_BASE in
> PlatformDxe driver.

Yes, the PlatformDxe driver statically instantiates dynamic drivers
for non-discoverable buses. But there is no longer any hard-coded
addresses in the device driver itself.

/
Leif
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

2018-04-23 Thread Meenakshi Aggarwal 


> -Original Message-
> From: Leif Lindholm [mailto:leif.lindh...@linaro.org]
> Sent: Tuesday, April 17, 2018 10:07 PM
> To: Meenakshi Aggarwal <meenakshi.aggar...@nxp.com>
> Cc: ard.biesheu...@linaro.org; edk2-devel@lists.01.org; Udit Kumar
> <udit.ku...@nxp.com>; Varun Sethi <v.se...@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.aggar...@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.aggar...@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 000..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=02%7C01%7Cmeenakshi.aggarwal%40nxp.com%7Cf09845
> 08e3e6425a971708d5a48171ca%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0
> %7C0%7C636595798265709617=lqPp%2BaHIRYNO%2B9buGRqcgvffpW
> nWzpIEpeLwdxPhZAk%3D=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 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +#include 
> > +
> > +#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 us

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

2018-04-17 Thread Leif Lindholm 
On Fri, Feb 16, 2018 at 02:20:01PM +0530, Meenakshi wrote:
> From: Meenakshi Aggarwal 
> 
> I2C driver produces gEfiI2cMasterProtocolGuid which can be
> used by other modules.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Meenakshi Aggarwal 
> ---
>  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 000..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
> +  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 "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   I2cRegsPointer to I2C registers
> +  @param   State  i2c state need to be checked
> +
> +  @retval  EFI_NOT_READY  Arbitration was lost
> +  @retval  EFI_TIMEOUTTimeout 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 < 5; Cnt++) {
> +MemoryFence ();
> +CurrState = MmioRead8 ((UINTN)>I2cSr);
> +if (CurrState & I2C_SR_IAL) {
> +   MmioWrite8 ((UINTN)>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   I2cRegsPointer to i2c registers
> +  @param   Byte   Byte to be transferred on i2c bus
> +
> +  @retval  EFI_NOT_READY  Arbitration was lost
> +  @retval  EFI_TIMEOUTTimeout occured
> +  @retval  EFI_NOT_FOUND  ACK was not recieved
> +  @retval  EFI_SUCCESSData transfer was succesful
> +
> +**/
> +STATIC
> +EFI_STATUS
> +TransferByte (
> +  IN  I2C_REGS*I2cRegs,
> +  IN  UINT8   Byte
> +  )
> +{
> +  EFI_STATUS  Ret;
> +
> +  MmioWrite8 ((UINTN)>I2cSr, I2C_SR_IIF_CLEAR);
> +  MmioWrite8 ((UINTN)>I2cDr, Byte);
> +
> +  Ret =