[U-Boot] [PATCH v3 8/9] tegra: i2c: Enable new CONFIG_SYS_I2C framework

[Albert ARIBAUD <albert.u.boot@aribaud.net>]

Hi Stephen,

On Tue, 30 Jul 2013 15:51:44 -0600, Stephen Warren
<swarren@wwwdotorg.org> wrote:

> On 07/30/2013 03:46 PM, Simon Glass wrote:
> > On Tue, Jul 30, 2013 at 3:32 PM, Stephen Warren <swarren@wwwdotorg.org
> > <mailto:swarren@wwwdotorg.org>> wrote:
> > 
> >     On 07/30/2013 03:21 PM, Simon Glass wrote:
> >     > On Tue, Jul 30, 2013 at 2:00 PM, Stephen Warren
> >     <swarren at wwwdotorg.org <mailto:swarren@wwwdotorg.org>
> >     > <mailto:swarren at wwwdotorg.org <mailto:swarren@wwwdotorg.org>>> 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.

> 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?

Amicalement,
-- 
Albert.