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

[Heiko Schocher <hs@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
> <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.

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