Re: [PATCH edk2-platforms v3 01/24] Silicon/NXP: Add I2c lib

["Leif Lindholm" <leif@nuviainc.com>]

On Fri, Apr 24, 2020 at 06:53:15 +0000, Pankaj Bansal (OSS) wrote:
> > > +/**
> > > +  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    Ibfd;
> > > +
> > > +  Regs = (I2C_REGS *)Base;
> > > +  if (FeaturePcdGet (PcdI2cErratumA009203)) {
> > > +    // Apply Erratum A-009203 before writing Ibfd register
> > 
> > It is an improvement, but there is still nothing in here that makes it
> > obvious why this is being done twice. The referenced u-boot patch does
> > it only once.
> 
> It's not necessary to call I2cEarlyInitialize for all I2c controllers.
> This function has been written so that i2c bus can be initialized to read the system clock
> from a clock generator or an FPFA.
> 
> To make a call to I2cInitialize, we need to know the I2cBusClock, which is a derivative of
> System clock. I2cBusClock is calculated after PLL multiplication to system clock.
> Therefore, we won't be able to call I2cInitialize without knowing System clock.
> 
> The idea is that for i2c controller to which clock generator or an FPFA is connected, we first
> Call I2cEarlyInitialize. Then we read system clock and then we can call I2cInitialize for that
> Controller as well as any other i2c controller(s) in the system.
> 
> So, for all i2c controllers (except one) I2cInitialize would be called and not I2cEarlyInitialize

OK. So, I am OK with that as en end result, but since the function is
not used at all in this patchset, can it be purged from here and
introduced just before it is needed with a subsequent set?
That will make its intended use *much* more clear.

/
    Leif

> > 
> > Hmm, furthermore, I don't see this function called at all? Why is it
> > included? If you delete it (and its declaration in .h), I'm OK with
> > the result.
> > 
> > > +    I2cErratumA009203 (Base);
> > > +  }
> > > +
> > > +  if (MmioRead8 ((UINTN)&Regs->Ibdbg) & I2C_IBDBG_GLFLT_EN) {
> > > +    Ibfd = ARRAY_LAST_ELEM (mI2cClockDivisorGlitchEnabled).Ibfd;
> > > +  } else {
> > > +    Ibfd = ARRAY_LAST_ELEM (mI2cClockDivisorGlitchDisabled).Ibfd;
> > > +  }
> > > +
> > > +  MmioWrite8 ((UINTN)&Regs->Ibfd, Ibfd);
> > > +
> > > +  I2cReset (Base);
> > > +
> > > +  return EFI_SUCCESS;
> > > +}
> > >