From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heiko Schocher Date: Thu, 01 Aug 2013 08:02:42 +0200 Subject: [U-Boot] [PATCH v3 8/9] tegra: i2c: Enable new CONFIG_SYS_I2C framework In-Reply-To: <51F9F477.5060607@wwwdotorg.org> References: <1367668903-29653-1-git-send-email-hs@denx.de> <1367668903-29653-9-git-send-email-hs@denx.de> <51F6946F.8010500@wwwdotorg.org> <51F740FD.4080505@denx.de> <51F81261.7020008@wwwdotorg.org> <51F81B51.6010305@wwwdotorg.org> <51F83104.5090508@wwwdotorg.org> <51F83570.1010909@wwwdotorg.org> <51F8A4CA.7070702@denx.de> <51F9662D.1070602@wwwdotorg.org> <51F9E4CB.9040004@denx.de> <51F9F477.5060607@wwwdotorg.org> Message-ID: <51F9FA02.1060403@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hello Stephen, Am 01.08.2013 07:39, schrieb Stephen Warren: > On 07/31/2013 10:32 PM, Heiko Schocher wrote: >> Hello Stephen, >> >> Am 31.07.2013 21:31, schrieb Stephen Warren: >>> On 07/30/2013 11:46 PM, Heiko Schocher wrote: >>> ... >>>> Hmm.. each i2c adapter has its own init function ... why the tegra >>>> driver do not use it? And do the necessary inits in it? So we >>>> initialize an adapater only if we use it, which is also a rule >>>> for u-boot ... >>>> >>>> I have no hw, but it should be possible to add to process_nodes() >>>> a parameter "controller_number" and call >>>> process_nodes(controller_number, ...) from the i2c drivers init >>>> function ... >>> >>> There are two steps to initializing I2C on a Tegra system: >>> >>> 1) Parsing the DT to determine which/how-many I2C adapters exist in the >>> SoC. This will yield information such as the register address of the I2C >>> adapters, which clock/reset signal they rely on, perhaps the max bus >>> clock rate, etc. >>> >>> This is a single global operation that needs to happen one single time >>> before anything else. >> >> Why? > > That's simply the way the code is written. Ok. >>> This stage initializes the i2c_controllers[] array, since that's what >>> stores all the data parsed from DT. >>> >>> 2) Actually initializing or using the I2C HW. That could certainly be >>> part of the per-I2C-controller init() function you mention. >> >> For that purpose is the i2c_init function. >> >> And why we could not do step 1 when we do step 2? Why doing step 1 >> for hw we later do not use? > > I suppose you could. It seems conceptually /far/ simpler to just scan > the DT once up-front rather than having to defer all this stuff until on the other hand we ring for every ms boot time ... and here we want to scan a complete dt with maybe a lot of nodes, we do not want to use? > you actually use an I2C bus. If you do that, how can you know how many > I2C buses exist without trying to use every possible one and seeing Do I really need to know that? > which fail? Also, the mapping from I2C bus number to register address is Hmm.. there are max TEGRA_I2C_NUM_CONTROLLERS. If one gets used, it scans the dt ... if only 1 adapter is used, only one is activated through i2c_init ... > only created by actually scanning the whole DT; there's no need to every > I2C DT node to have a /aliases entry that dictates its U-Boot device ID, > so you really do have to scan everything completely up-front before you > can determine which registers to use. But the i2c bus number is coded static all over the u-boot code (Should be changed) in the code. Saying a board has an i2c eeprom on bus "2", it calls i2c_set_bus_number(2) ... this "2" must be somewhere in the dt or? If this "2" is used on different boards with the same u-boot code only different in dt, bus 2 maybe not exist ... or if you change the order of i2c nodes in the dt "2" is no longer "2" ... or? >> saying something like this (just as an example, should be tested): >> >> diff --git a/drivers/i2c/tegra_i2c.c b/drivers/i2c/tegra_i2c.c >> index 9ac3969..7bbd3c7 100644 >> --- a/drivers/i2c/tegra_i2c.c >> +++ b/drivers/i2c/tegra_i2c.c >> @@ -378,81 +378,91 @@ static int i2c_get_config(const void *blob, int >> node, struct i2c_bus *i2c_bus) >> * @param is_scs 1 if this HW uses a single clock source (T114+) >> * @return 0 if ok, -1 on error >> */ >> -static int process_nodes(const void *blob, int node_list[], int count, >> +static int process_nodes(const void *blob, int node_list[], >> + struct i2c_adapter *adap, int count, >> int is_dvc, int is_scs) >> { >> struct i2c_bus *i2c_bus; >> - int i; >> - >> - /* build the i2c_controllers[] for each controller */ >> - for (i = 0; i< count; i++) { >> - int node = node_list[i]; >> + int node = node_list[i]; > > How do you determine i here? There's no guarantee that all I2C DT nodes adap->hwadapnr as in drivers/i2c/tegra_i2c.c: static struct i2c_bus *tegra_i2c_get_bus(struct i2c_adapter *adap) { struct i2c_bus *bus; bus = &i2c_controllers[adap->hwadapnr]; [...] > end up being processed in a single call to process_nodes(); there might > be some "Tegra20 I2C", some "Tegra20 I2C DVC", some "Tegra30 I2C" > modules in the same SoC. bye, Heiko -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany