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

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

On 23 April 2018 at 17:50, Meenakshi Aggarwal
<meenakshi.aggarwal@nxp.com> wrote:
> 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.
>

You are correct: the I2C enumeration and platform protocols should be
used in all cases *except* when we are dealing with a I2C device that
is used to produce an architectural protocol that is exposed to the OS
as a runtime service. This is why the PCF8563 invokes the I2C master
protocol directly rather than the I2C I/O protocol.