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;
> > > +}
> > >
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#58052): https://edk2.groups.io/g/devel/message/58052
Mute This Topic: https://groups.io/mt/73008796/21656
Group Owner: [email protected]
Unsubscribe: https://edk2.groups.io/g/devel/unsub [[email protected]]
-=-=-=-=-=-=-=-=-=-=-=-
