public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Leif Lindholm" <leif@nuviainc.com>
To: "Pankaj Bansal (OSS)" <pankaj.bansal@oss.nxp.com>
Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>,
	Michael D Kinney <michael.d.kinney@intel.com>,
	"devel@edk2.groups.io" <devel@edk2.groups.io>,
	Varun Sethi <V.Sethi@nxp.com>,
	Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>,
	Jon Nettleton <jon@solid-run.com>,
	Ard Biesheuvel <ard.biesheuvel@linaro.org>
Subject: Re: [PATCH edk2-platforms v3 01/24] Silicon/NXP: Add I2c lib
Date: Fri, 24 Apr 2020 12:47:57 +0100	[thread overview]
Message-ID: <20200424114757.GK14075@vanye> (raw)
In-Reply-To: <AM0PR04MB5922CFF5EEE792FF2BF671CEF1D00@AM0PR04MB5922.eurprd04.prod.outlook.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;
> > > +}
> > >

  reply	other threads:[~2020-04-24 11:48 UTC|newest]

Thread overview: 54+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-15 12:13 [PATCH edk2-platforms v3 00/24] Add PEI phase to LS1043ARDB Platform Pankaj Bansal
2020-04-15 12:13 ` [PATCH edk2-platforms v3 01/24] Silicon/NXP: Add I2c lib Pankaj Bansal
2020-04-22 16:38   ` Leif Lindholm
2020-04-24  6:53     ` Pankaj Bansal
2020-04-24 11:47       ` Leif Lindholm [this message]
2020-04-15 12:13 ` [PATCH edk2-platforms v3 02/24] Silicon/NXP: changes to use I2clib in i2cdxe Pankaj Bansal
2020-04-22 16:41   ` Leif Lindholm
2020-04-15 12:13 ` [PATCH edk2-platforms v3 03/24] Silicon/NXP/I2cDxe: Fix I2c Timeout with RTC Pankaj Bansal
2020-04-15 12:13 ` [PATCH edk2-platforms v3 04/24] Silicon/Maxim: Fix bug in RtcWrite in Ds1307RtcLib Pankaj Bansal
2020-04-15 12:13 ` [PATCH edk2-platforms v3 05/24] Silicon/Maxim: Add comments " Pankaj Bansal
2020-04-15 12:13 ` [PATCH edk2-platforms v3 06/24] NXP/LS1043aRdb: Move Soc specific components to soc files Pankaj Bansal
2020-04-15 12:13 ` [PATCH edk2-platforms v3 07/24] Silicon/NXP: remove print information from Soc lib Pankaj Bansal
2020-04-22 16:51   ` [PATCH edk2-platforms v3 07/24] Silicon/NXP: remove print information from Soc Leif Lindholm
2020-04-15 12:13 ` [PATCH edk2-platforms v3 08/24] Silicon/NXP: remove not needed components Pankaj Bansal
2020-04-15 12:13 ` [PATCH edk2-platforms v3 09/24] Silicon/NXP: Remove unnecessary PCDs Pankaj Bansal
2020-04-15 12:13 ` [PATCH edk2-platforms v3 10/24] Silicon/NXP: Move dsc file Pankaj Bansal
2020-04-15 12:13 ` [PATCH edk2-platforms v3 11/24] Platform/NXP: rename the ArmPlatformLib as per ArmPlatformPkg Pankaj Bansal
2020-04-15 12:13 ` [PATCH edk2-platforms v3 12/24] Silicon/NXP: Move RAM retrieval from SocLib Pankaj Bansal
2020-04-23  9:15   ` Leif Lindholm
2020-04-15 12:13 ` [PATCH edk2-platforms v3 13/24] Platform/NXP/LS1043aRdbPkg: Add Clock retrieval APIs Pankaj Bansal
2020-04-23  9:19   ` Leif Lindholm
2020-04-15 12:13 ` [PATCH edk2-platforms v3 14/24] Silicon/NXP: Use Clock retrieval PPI in modules Pankaj Bansal
2020-04-23  9:25   ` Leif Lindholm
2020-04-15 12:13 ` [PATCH edk2-platforms v3 15/24] Silicon: NXP: Remove direct calls to SwapMmio* APIs Pankaj Bansal
2020-04-23  9:27   ` Leif Lindholm
2020-04-23 10:11   ` Leif Lindholm
2020-04-15 12:13 ` [PATCH edk2-platforms v3 16/24] Silicon/NXP: Add Chassis2 Package Pankaj Bansal
2020-04-23 10:27   ` Leif Lindholm
2020-04-23 11:38     ` Pankaj Bansal
2020-04-23 11:57       ` Leif Lindholm
2020-04-23 12:02         ` Pankaj Bansal
2020-04-23 12:05           ` Leif Lindholm
2020-04-23 13:41             ` Pankaj Bansal
2020-04-23 14:18               ` Leif Lindholm
2020-04-23 14:45                 ` Pankaj Bansal
2020-04-23 15:26                   ` Leif Lindholm
2020-04-24  2:42                     ` Pankaj Bansal
2020-04-24 15:51                       ` Leif Lindholm
2020-04-28 17:46                         ` [edk2-devel] " Ard Biesheuvel
2020-04-28 17:50                           ` Leif Lindholm
2020-04-15 12:13 ` [PATCH edk2-platforms v3 17/24] Silicon/NXP/LS1043A: Use ChassisLib from Chassis2 Pkg Pankaj Bansal
2020-04-15 12:13 ` [PATCH edk2-platforms v3 18/24] Silicon/NXP/LS1043A: Move SocLib to Soc Package Pankaj Bansal
2020-04-15 12:13 ` [PATCH edk2-platforms v3 19/24] NXP/LS1043aRdbPkg/ArmPlatformLib: Remove extern SocInit Pankaj Bansal
2020-04-15 12:13 ` [PATCH edk2-platforms v3 20/24] NXP: LS1043aRdbPkg: Use ArmPlatformHelper.S from ArmPlatformPkg Pankaj Bansal
2020-04-23 10:28   ` Leif Lindholm
2020-04-15 12:13 ` [PATCH edk2-platforms v3 21/24] Platform/NXP: Use FV rules from ArmVirtPkg Pankaj Bansal
2020-04-23 10:31   ` Leif Lindholm
2020-04-24  6:24     ` Pankaj Bansal
2020-04-24 11:43       ` Leif Lindholm
2020-04-15 12:13 ` [PATCH edk2-platforms v3 22/24] Platform/NXP/LS1043aRdbPkg: Add VarStore Pankaj Bansal
2020-04-15 12:13 ` [PATCH edk2-platforms v3 23/24] Silicon/NXP: move MemoryInitPeiLib as per PEIM structures Pankaj Bansal
2020-04-15 12:13 ` [PATCH edk2-platforms v3 24/24] Platform/NXP/LS1043aRdbPkg: Add PEI Phase Pankaj Bansal
2020-04-23 10:33   ` Leif Lindholm
2020-04-22 10:46 ` [PATCH edk2-platforms v3 00/24] Add PEI phase to LS1043ARDB Platform Leif Lindholm

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200424114757.GK14075@vanye \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox