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

[Simon Glass]

Hi Albert,

On Tue, Jul 30, 2013 at 4:09 PM, Albert ARIBAUD
<albert.u.boot at aribaud.net>wrote:

> Hi Stephen,
>
> On Tue, 30 Jul 2013 15:51:44 -0600, Stephen Warren
> <swarren at 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 at wwwdotorg.org
> > > <mailto:swarren at 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 at wwwdotorg.org>
> > >     > <mailto:swarren at wwwdotorg.org <mailto:swarren at 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?
>

I'm not sure. There must be some reason for those i2c_init calls in
board_init_f() - or are they purely historical?

>
> Amicalement,
> --
> Albert.
>

Regards,
Simon