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

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

Hi Ard

> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Monday, April 23, 2018 7:10 PM
> To: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>
> Cc: Leif Lindholm <leif.lindholm@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 23 April 2018 at 12:34, Meenakshi Aggarwal
> <meenakshi.aggarwal@nxp.com> wrote:
> > Hi Leif
> >
> >> -----Original Message-----
> >> From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> >> Sent: Monday, April 23, 2018 2:08 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 Mon, Apr 23, 2018 at 08:21:22AM +0000, Meenakshi Aggarwal wrote:
> >> > > > +/**
> >> > > > +  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.
> >>
> >> 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 model. At present we 
are not using those features for i2c driver but if you think we should have these in code.
We will add this support.  

> > 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.
> 

I don’t say much on this because this is implementation , which can vary mind to mind. 
But in order to maintain many controllers
and devices on the top of it, my preference would be to 
go with bus configuration and bus enumeration protocol. 

You can say, why to complicate the code, when this can be done
in easy way.

> >> > 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