From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f66.google.com (mail-wr1-f66.google.com [209.85.221.66]) by mx.groups.io with SMTP id smtpd.web12.3921.1587728881976873975 for ; Fri, 24 Apr 2020 04:48:02 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@nuviainc-com.20150623.gappssmtp.com header.s=20150623 header.b=0rp7ECdC; spf=pass (domain: nuviainc.com, ip: 209.85.221.66, mailfrom: leif@nuviainc.com) Received: by mail-wr1-f66.google.com with SMTP id d15so8800400wrx.3 for ; Fri, 24 Apr 2020 04:48:01 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nuviainc-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=qpj1abizHyhv+ZnkelUiXE67xzY6UElFl/+hfCxydik=; b=0rp7ECdCY809NFVP0XFvPaekG0XpXWF7hL2jJUpB6bukGseGlWMdO0VuhzkOtat6Ox Ndn2C3q0BRmaH699LZ7w1POncEmWTuD57c0GSc7HKEpdQFqBR9LCA1Anyp/VdCZAd7N9 pcgJs0R/D8CvELPMFt077Y7aTBThEYt3U9w+9C3jDwcmj/C0xKNgZSdHnlkkmSi4aXdN ghlwOLWxzBY0WOfcPe0Kjo2UxtZDNQFWL5IHy45ZdMuzknUbSEDQuWe+RlL5HdvqRieq H3Za+RzXMBv21snUZ6kgvZ7gk27cMn4b9N5Z5NeKYbHmiUOdW7RSh+/zfb/RFZfeh4CK PaOQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=qpj1abizHyhv+ZnkelUiXE67xzY6UElFl/+hfCxydik=; b=bAw4MfPkriRVsn6E2GPiNN29EiLBZPGmUYtfv+XpR9+Fd5/0/9FT2Awk7i6g2NuMkr xJuXEgk6DBhYO+aDONaRYjSXgpPkSe12VfxuV9E9ThmNa3YvjuLOy2X6Qyk+040xf1A1 t2wqehiDVYo8ZHPjoqLQSsG32zf8te8BeZbA+jAqUoQFXxHY5Q8u55kEVMLDv627EqQK i5MmIzSzC20exJaIUYreDCSYybo9WlDcJrSKNkVhefNSXTzo6iTUrAr3AggtAW0lJwXV Tw5T+invcFMdfyz61ZgLT0akuWQYdbEj17bju/BMZJktcY4ZjLx0awzjlktS9pKgYQJv 4kew== X-Gm-Message-State: AGi0PuYQ0SM5/3Quqh5SC+XOwvQnGxhDEpsssFnjsbnwtx1rD99oI+mg W2juCV7w/L5ZtKxFo625nGgWnQ== X-Google-Smtp-Source: APiQypLW1jwZdw7tpyEF29E3a8M/Z5AyJ27oRLWTmBQCSqWyHSCtVt6pSB0Qy2Rs5FlGey+t8ifgHQ== X-Received: by 2002:a5d:4092:: with SMTP id o18mr10561551wrp.227.1587728880580; Fri, 24 Apr 2020 04:48:00 -0700 (PDT) Return-Path: Received: from vanye ([2001:470:1f09:12f0:b26e:bfff:fea9:f1b8]) by smtp.gmail.com with ESMTPSA id q17sm2384141wmj.45.2020.04.24.04.47.59 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 24 Apr 2020 04:47:59 -0700 (PDT) Date: Fri, 24 Apr 2020 12:47:57 +0100 From: "Leif Lindholm" To: "Pankaj Bansal (OSS)" Cc: Meenakshi Aggarwal , Michael D Kinney , "devel@edk2.groups.io" , Varun Sethi , Samer El-Haj-Mahmoud , Jon Nettleton , Ard Biesheuvel Subject: Re: [PATCH edk2-platforms v3 01/24] Silicon/NXP: Add I2c lib Message-ID: <20200424114757.GK14075@vanye> References: <20200415121342.9246-1-pankaj.bansal@oss.nxp.com> <20200415121342.9246-2-pankaj.bansal@oss.nxp.com> <20200422163829.GO14075@vanye> MIME-Version: 1.0 In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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; > > > +} > > >