From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heiko Schocher Date: Wed, 31 Jul 2013 07:52:58 +0200 Subject: [U-Boot] [PATCH v3 8/9] tegra: i2c: Enable new CONFIG_SYS_I2C framework In-Reply-To: <20130731000921.724f5c71@lilith> 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> <20130731000921.724f5c71@lilith> Message-ID: <51F8A63A.5080203@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 Albert, Am 31.07.2013 00:09, schrieb Albert ARIBAUD: > Hi Stephen, > > On Tue, 30 Jul 2013 15:51:44 -0600, Stephen Warren > wrote: > >> On 07/30/2013 03:46 PM, Simon Glass wrote: >>> On Tue, Jul 30, 2013 at 3:32 PM, Stephen Warren>> > wrote: >>> >>> On 07/30/2013 03:21 PM, Simon Glass wrote: >>> > On Tue, Jul 30, 2013 at 2:00 PM, Stephen Warren >>> >>> > >> wrote: >>> ... >>> > Oh, with the options Tegra has enabled, perhaps the call >>> sequence is: >>> > >>> > board_init_f() (which uses init_sequence_f[]) -> >>> init_func_i2c() -> >>> > i2c_init_all(), which then calls: >>> > >>> > * i2c_init_board(), which is supposed to parse DT >>> > * i2c_set_bus_num(), which will call I2C_ADAP->init >>> > >>> > However, according to the comments near the top of >>> arch/arm/lib/crt0.S, >>> > board_init_f() is called in an environment where variable data >>> (.data, >>> > .bss) is not available, hence i2c_init_board() cannot possibly >>> operate >>> > correctly since its whole purpose is to fill in variable data >>> structures >>> > from DT. >>> > >>> > >>> > I suppose you could mark i2c_controllers so that it is in the data >>> > section with __attribute__((section(".data"))). That's what eynos >>> does, >>> > for example. It is valid since SPL or BCT has set up the SDRAM. >>> >>> Neither .data nor .bss is available. Only .rodata and .text are. >>> >>> >>> .data is available, honest. We rely on it. During relocation it gets copied. >> >> It gets copied so that it ends up in RAM. It is assumed that before >> relocation, all .text/.rodata/.data is in ROM and can't be modified, and >> .bss in inaccessible. Technically that means we could read .data before >> relocation, but certainly not write to it. > > Indeed, initialized data happens to be readable before relocation, but > writing to data, on the other hand, is strictly forbidden. Before > relocation, that is, while within board_init_f() the only writable area > is GD. Yes. >> Now in practice yes, it does work to write to .data before relocation on >> platforms where the U-Boot binary isn't actually in flash, but is >> already in ROM. However as I mention, code cannot rely on that. > > Already in RAM, not ROM -- and indeed, one should not rely on this. > >> If any of this isn't true, then the documentation in crt0.S is wrong. >> I'm CC'ing Albert to see if that's the case. >> >>> In practice, perhaps we can assume that it will work on Tegra because we >>> know the DRAM is already set up, but then that makes Tegra work in some >>> strange special-case way, and completely violates the constraints >>> described in crt0.S. We should be striving to unify how all the >>> different chips work, rather than adding yet more strange special-cases >>> to the initialization sequence to hack around systemic problems. >>> >>> >>> Sure, this is up to you. I was just suggesting something that works and >>> requires little effort. It isn't pure, agreed. >> >> The simplest approach is probably to revert the patch in question, since >> it clearly violates how U-Boot is supposed to work. >> >> It's not really up to me; I think someone like Albert should make the >> decision since he controls the ARM U-Boot architecture, or Tom as Tegra >> maintainer, or perhaps you as your patch broke the code. > > board_init_f() is supposed to initialize just enough of the system to > allow relocation. Is initializing i2c necessary in this context? Not on tegra I think. It is needed for example for reading ram setup data from an eeprom ... maybe we should do here: - remove init_func_i2c() completly from board_init_f, as a goal from the new i2c framework is, to get rid of i2c_init* calls all over the code. If boards fail, they should add i2c_set_bus_num() where they need it - or at least make init_func_i2c() weak, and define it only on boards, which need it. - adapt i2c tegra driver: each i2c adapter has its own init function. Do the necessary inits there for the i2c tegra driver. bye, Heiko -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany