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

[Simon Glass]

Hi Stephen,

On Tue, Jul 30, 2013 at 3:51 PM, 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.
>
> 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.
>
> 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 in the SPL case (which tegra sort-of uses) there is earlier
step before U-Boot which sets up SDRAM, so it is (I think) always true that
U-Boot is running from RAM. I guess the longer term plan is to allow some
sort of pre-reloc malloc(), which would work for any machine, but I'm not
sure if anyone is working on it.


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

My '(just for illustration, please don't merge)' patch from last October?
:-)

I did offer to look at this for seaboard if it helps, once we agree on a
solution, but if you have a solution in mind, please go ahead.

Regards,
SImon