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

login
register
mail settings
Submitter Heiko Schocher
Date May 4, 2013, 12:01 p.m.
Message ID <1367668903-29653-9-git-send-email-hs@denx.de>
Download mbox | patch
Permalink /patch/241452/
State Superseded
Delegated to: Heiko Schocher
Headers show

Comments

Heiko Schocher - May 4, 2013, 12:01 p.m.
From: Simon Glass <sjg@chromium.org>

This enables CONFIG_SYS_I2C on Tegra, updating existing boards and the Tegra
i2c driver to support this.

Signed-off-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Heiko Schocher <hs@denx.de>
---
- changes for v2:
  new in v2
  posted from Simon Glass here:
  http://patchwork.ozlabs.org/patch/195568/

  added to this patchserie, changes:
  - rebased to current head
  - add "struct i2c_adapter *adap" to the i2c adapter functions.
  - rework tegra_i2c_get_bus()
  - add missing adapters in tegra_i2c_adap[]
  - replace CONFIG_SYS_MAX_I2C_BUS through TEGRA_I2C_NUM_CONTROLLERS
  - rename CONFIG_TEGRA_I2C to CONFIG_SYS_I2C_TEGRA

- changes for v3:
  - adapt to the new introduced U_BOOT_I2C_ADAP_COMPLETE define
  - add changes for dalmore and cardhu boards
  - adapt README
  - adapt commit message
  - patch has 1 checkpatch warning:
WARNING: Avoid CamelCase: <get_OPB_freq>
#1274: FILE: drivers/i2c/ppc4xx_i2c.c:128:
+	divisor = (get_OPB_freq() - 1) / 10000000;
    This should be fixed in a seperate step, as it affects a lot
    of boards...
---
 README                      |  5 +++
 drivers/i2c/Makefile        |  2 +-
 drivers/i2c/tegra_i2c.c     | 80 ++++++++++++++++++---------------------------
 include/configs/beaver.h    |  5 ++-
 include/configs/cardhu.h    |  3 +-
 include/configs/dalmore.h   |  3 +-
 include/configs/seaboard.h  |  5 ++-
 include/configs/trimslice.h |  5 ++-
 include/configs/whistler.h  |  5 ++-
 9 Dateien geändert, 50 Zeilen hinzugefügt(+), 63 Zeilen entfernt(-)
Stephen Warren - May 6, 2013, 7:08 p.m.
On 05/04/2013 06:01 AM, Heiko Schocher wrote:
> From: Simon Glass <sjg@chromium.org>
> 
> This enables CONFIG_SYS_I2C on Tegra, updating existing boards and the Tegra
> i2c driver to support this.

>  include/configs/beaver.h    |  5 ++-
>  include/configs/cardhu.h    |  3 +-
>  include/configs/dalmore.h   |  3 +-
>  include/configs/seaboard.h  |  5 ++-
>  include/configs/trimslice.h |  5 ++-
>  include/configs/whistler.h  |  5 ++-

There are a lot more Tegra boards than just those. Shouldn't they all be
updated? You also didn't Cc the Tegra maintainer - I have done on this mail.
Marc Dietrich - May 7, 2013, 8:01 a.m.
Hi,

Am Montag, 6. Mai 2013, 13:08:31 schrieb Stephen Warren:
> On 05/04/2013 06:01 AM, Heiko Schocher wrote:
> > From: Simon Glass <sjg@chromium.org>
> > 
> > This enables CONFIG_SYS_I2C on Tegra, updating existing boards and the
> > Tegra i2c driver to support this.
> > 
> >  include/configs/beaver.h    |  5 ++-
> >  include/configs/cardhu.h    |  3 +-
> >  include/configs/dalmore.h   |  3 +-
> >  include/configs/seaboard.h  |  5 ++-
> >  include/configs/trimslice.h |  5 ++-
> >  include/configs/whistler.h  |  5 ++-
> 
> There are a lot more Tegra boards than just those. Shouldn't they all be
> updated? You also didn't Cc the Tegra maintainer - I have done on this mail.

not all boards use I2C up to now, to the patch seems to change only the boards 
which do. So this is ok for now. The question is if CONFIG_SYS_I2C could be 
moved to tegra_common or so because the device-tree disables I2C be default.

Marc
Heiko Schocher - May 7, 2013, 1:07 p.m.
Hello Stephen,

Am 06.05.2013 21:08, schrieb Stephen Warren:
> On 05/04/2013 06:01 AM, Heiko Schocher wrote:
>> From: Simon Glass <sjg@chromium.org>
>>
>> This enables CONFIG_SYS_I2C on Tegra, updating existing boards and the Tegra
>> i2c driver to support this.
> 
>>  include/configs/beaver.h    |  5 ++-
>>  include/configs/cardhu.h    |  3 +-
>>  include/configs/dalmore.h   |  3 +-
>>  include/configs/seaboard.h  |  5 ++-
>>  include/configs/trimslice.h |  5 ++-
>>  include/configs/whistler.h  |  5 ++-
> 
> There are a lot more Tegra boards than just those. Shouldn't they all be
> updated? You also didn't Cc the Tegra maintainer - I have done on this mail.

I got no compiler warnings ... Hmm.. maybe Simon Glass can help here
more (added to cc), as he did this patch ... Simon?

bye,
Heiko
Heiko Schocher - May 7, 2013, 1:12 p.m.
Hello Tom,

Am 06.05.2013 21:39, schrieb Tom Warren:
> Thanks, Stephen. Adding Yen, who wrote the original Tegra I2C driver.
> 
>> -----Original Message-----
>> From: Stephen Warren [mailto:swarren@wwwdotorg.org]
>> Sent: Monday, May 06, 2013 12:09 PM
>> To: Heiko Schocher
>> Cc: u-boot@lists.denx.de; Tom Warren
>> Subject: Re: [U-Boot] [PATCH v3 8/9] tegra: i2c: Enable new CONFIG_SYS_I2C
>> framework
>>
>> On 05/04/2013 06:01 AM, Heiko Schocher wrote:
>>> From: Simon Glass <sjg@chromium.org>
>>>
>>> This enables CONFIG_SYS_I2C on Tegra, updating existing boards and the
>>> Tegra i2c driver to support this.
>>
>>>  include/configs/beaver.h    |  5 ++-
>>>  include/configs/cardhu.h    |  3 +-
>>>  include/configs/dalmore.h   |  3 +-
>>>  include/configs/seaboard.h  |  5 ++-
>>>  include/configs/trimslice.h |  5 ++-
>>>  include/configs/whistler.h  |  5 ++-
>>
>> There are a lot more Tegra boards than just those. Shouldn't they all be
>> updated? You also didn't Cc the Tegra maintainer - I have done on this mail.
> 
> Heiko - please explain what the new CONFIG_SYS_I2C_TEGRA switch does, what it adds to/improves upon for the extant Tegra I2C driver, and what testing you did.

It just convert the existing i2c driver to the new i2c multibus/
multiadapter framework, see the description in the README for
this new define. I add Simon Glass to this, as he did this
patch for the tegra i2c driver. I did only compile tests ...
IIRC Simon did some tests on real HW.

> The boards listed are the only ones w/CONFIG_TEGRA_I2C currently enabled, so I don't think any others need it, until such time as their maintainers need to add I2C support.

Yes, I think so too, as I get no compilerwarning/error when doing
a "MAKEALL arm" ...

bye,
Heiko
Stephen Warren - May 7, 2013, 2:55 p.m.
On 05/07/2013 02:01 AM, Marc Dietrich wrote:
> Hi,
> 
> Am Montag, 6. Mai 2013, 13:08:31 schrieb Stephen Warren:
>> On 05/04/2013 06:01 AM, Heiko Schocher wrote:
>>> From: Simon Glass <sjg@chromium.org>
>>>
>>> This enables CONFIG_SYS_I2C on Tegra, updating existing boards and the
>>> Tegra i2c driver to support this.
>>>
>>>  include/configs/beaver.h    |  5 ++-
>>>  include/configs/cardhu.h    |  3 +-
>>>  include/configs/dalmore.h   |  3 +-
>>>  include/configs/seaboard.h  |  5 ++-
>>>  include/configs/trimslice.h |  5 ++-
>>>  include/configs/whistler.h  |  5 ++-
>>
>> There are a lot more Tegra boards than just those. Shouldn't they all be
>> updated? You also didn't Cc the Tegra maintainer - I have done on this mail.
> 
> not all boards use I2C up to now, to the patch seems to change only the boards 
> which do. So this is ok for now. The question is if CONFIG_SYS_I2C could be 
> moved to tegra_common or so because the device-tree disables I2C be default.

Yes, we should do that for all the devices that are now
instantiated/configured using device tree. But, we can do that as a
separate patch series after this.
Simon Glass - May 7, 2013, 4:17 p.m.
Hi,

On Tue, May 7, 2013 at 8:55 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 05/07/2013 02:01 AM, Marc Dietrich wrote:
>> Hi,
>>
>> Am Montag, 6. Mai 2013, 13:08:31 schrieb Stephen Warren:
>>> On 05/04/2013 06:01 AM, Heiko Schocher wrote:
>>>> From: Simon Glass <sjg@chromium.org>
>>>>
>>>> This enables CONFIG_SYS_I2C on Tegra, updating existing boards and the
>>>> Tegra i2c driver to support this.
>>>>
>>>>  include/configs/beaver.h    |  5 ++-
>>>>  include/configs/cardhu.h    |  3 +-
>>>>  include/configs/dalmore.h   |  3 +-
>>>>  include/configs/seaboard.h  |  5 ++-
>>>>  include/configs/trimslice.h |  5 ++-
>>>>  include/configs/whistler.h  |  5 ++-
>>>
>>> There are a lot more Tegra boards than just those. Shouldn't they all be
>>> updated? You also didn't Cc the Tegra maintainer - I have done on this mail.
>>
>> not all boards use I2C up to now, to the patch seems to change only the boards
>> which do. So this is ok for now. The question is if CONFIG_SYS_I2C could be
>> moved to tegra_common or so because the device-tree disables I2C be default.
>
> Yes, we should do that for all the devices that are now
> instantiated/configured using device tree. But, we can do that as a
> separate patch series after this.

I don't have anything to add to this - I did the Tegra patch a long
time ago when this was last posted, and it was tested on Seaboard at
the time.

I'll make time to review and test the series at least on snow.

Regards,
Simon
Heiko Schocher - May 8, 2013, 4:11 a.m.
Hello Stephen,

Am 07.05.2013 16:55, schrieb Stephen Warren:
> On 05/07/2013 02:01 AM, Marc Dietrich wrote:
>> Hi,
>>
>> Am Montag, 6. Mai 2013, 13:08:31 schrieb Stephen Warren:
>>> On 05/04/2013 06:01 AM, Heiko Schocher wrote:
>>>> From: Simon Glass <sjg@chromium.org>
>>>>
>>>> This enables CONFIG_SYS_I2C on Tegra, updating existing boards and the
>>>> Tegra i2c driver to support this.
>>>>
>>>>  include/configs/beaver.h    |  5 ++-
>>>>  include/configs/cardhu.h    |  3 +-
>>>>  include/configs/dalmore.h   |  3 +-
>>>>  include/configs/seaboard.h  |  5 ++-
>>>>  include/configs/trimslice.h |  5 ++-
>>>>  include/configs/whistler.h  |  5 ++-
>>>
>>> There are a lot more Tegra boards than just those. Shouldn't they all be
>>> updated? You also didn't Cc the Tegra maintainer - I have done on this mail.
>>
>> not all boards use I2C up to now, to the patch seems to change only the boards 
>> which do. So this is ok for now. The question is if CONFIG_SYS_I2C could be 
>> moved to tegra_common or so because the device-tree disables I2C be default.
> 
> Yes, we should do that for all the devices that are now
> instantiated/configured using device tree. But, we can do that as a
> separate patch series after this.

Yes, that makes sense.

bye,
Heiko
Stephen Warren - July 29, 2013, 4:12 p.m.
On 05/04/2013 06:01 AM, Heiko Schocher wrote:
> From: Simon Glass <sjg@chromium.org>
> 
> This enables CONFIG_SYS_I2C on Tegra, updating existing boards and the Tegra
> i2c driver to support this.

Heiko, the latest U-Boot tree hangs during boot on Tegra, and "git
bisect" points at this patch. Olof reported the issue to me.

Can you take a look at the code and see what might be wrong? Thanks.

I suspect some kind of initialization ordering issue, since the boot
messages are:

-----
U-Boot SPL 2013.07-rc3-00038-g880540d (Jul 29 2013 - 10:04:37)
U-Boot 2013.07-rc3-00038-g880540d (Jul 29 2013 - 10:04:37)

TEGRA30
Board: NVIDIA Beaver
I2C:   Caller requested bad clock: periph=-49, parent=2
-----

... and that "bad clock" message implies to me that the I2C driver is
initializing before it has parsed the correct clock ID out of device tree.

Some later commit causes the hang to happen right after printing "I2C:",
without printing the "bad clock" message. I didn't investigate that,
since I'm assuming the root-cause is the same. Most likely some later
commit causes the uninitialized data to be a valid clock, yet not the
actual I2C clock, so the I2C clock still isn't turned on, and touching
HW (i.e. reading/writing the I2C registers) without a running clock on
Tegra caused hard hangs.
Heiko Schocher - July 30, 2013, 4:28 a.m.
Hello Stephen,

Am 29.07.2013 18:12, schrieb Stephen Warren:
> On 05/04/2013 06:01 AM, Heiko Schocher wrote:
>> From: Simon Glass<sjg@chromium.org>
>>
>> This enables CONFIG_SYS_I2C on Tegra, updating existing boards and the Tegra
>> i2c driver to support this.
>
> Heiko, the latest U-Boot tree hangs during boot on Tegra, and "git

:-(

Could you enable debug printf?

> bisect" points at this patch. Olof reported the issue to me.

Thanks!

> Can you take a look at the code and see what might be wrong? Thanks.

Yep.

> I suspect some kind of initialization ordering issue, since the boot
> messages are:
>
> -----
> U-Boot SPL 2013.07-rc3-00038-g880540d (Jul 29 2013 - 10:04:37)
> U-Boot 2013.07-rc3-00038-g880540d (Jul 29 2013 - 10:04:37)
>
> TEGRA30
> Board: NVIDIA Beaver
> I2C:   Caller requested bad clock: periph=-49, parent=2
> -----
>
> ... and that "bad clock" message implies to me that the I2C driver is
> initializing before it has parsed the correct clock ID out of device tree.

Hmm... looking in the patch ... I can see nothing which changes
some initializing order ...

@Simon: Do you have an idea?

just found some wrong settings for tegra30:

In include/configs/tegra30-common.h:
/* Total I2C ports on Tegra30 */
#define TEGRA_I2C_NUM_CONTROLLERS       5

README says:
                - drivers/i2c/tegra_i2c.c:
                 - activate this driver with CONFIG_SYS_I2C_TEGRA
                 - This driver adds 4 i2c buses with a fix speed from
                   100000 and the slave addr 0!

end yes, in the i2c driver are only 4 ports activated ... this
should be changed ... but I think, this has nothing to do with
your problem ... but try to add in the i2c driver one more i2c adapter
for the case TEGRA_I2C_NUM_CONTROLLERS > 4

> Some later commit causes the hang to happen right after printing "I2C:",
> without printing the "bad clock" message. I didn't investigate that,
> since I'm assuming the root-cause is the same. Most likely some later
> commit causes the uninitialized data to be a valid clock, yet not the
> actual I2C clock, so the I2C clock still isn't turned on, and touching
> HW (i.e. reading/writing the I2C registers) without a running clock on
> Tegra caused hard hangs.

digging deeper, the above "bad clock" message

is a result from calling this function from the i2c driver:
./drivers/i2c/tegra_i2c.c:
static void i2c_init_controller(struct i2c_bus *i2c_bus)
{
         /*
          * Use PLLP - DP-04508-001_v06 datasheet indicates a divisor of 8
          * here, in section 23.3.1, but in fact we seem to need a factor of
          * 16 to get the right frequency.
          */
         clock_start_periph_pll(i2c_bus->periph_id, CLOCK_ID_PERIPH,
                 i2c_bus->speed * 2 * 8);

Please enable debug printfs and look from where i2c_init_controller()
is called. You should see the following debug printf if it go the right
way (Just reading code, I have no HW ...)

process_nodes():
                 debug("%s: controller bus %d at %p, periph_id %d, speed %d: ",
                       is_dvc ? "dvc" : "i2c", i, i2c_bus->regs,
                       i2c_bus->periph_id, i2c_bus->speed);

called from i2c_init_board in this driver.

This should be called from drivers/i2c/i2c_core.c i2c_init_all()
called from arch/arm/lib/board.c init_func_i2c()

I think i2c_bus->periph_id ("periph=-49") is not setup right ... do
you have the correct dt?

bye,
Heiko
Simon Glass - July 30, 2013, 4:34 a.m.
Hi Heiko,

On Mon, Jul 29, 2013 at 10:28 PM, Heiko Schocher <hs@denx.de> wrote:

> Hello Stephen,
>
> Am 29.07.2013 18:12, schrieb Stephen Warren:
>
>  On 05/04/2013 06:01 AM, Heiko Schocher wrote:
>>
>>> From: Simon Glass<sjg@chromium.org>
>>>
>>> This enables CONFIG_SYS_I2C on Tegra, updating existing boards and the
>>> Tegra
>>> i2c driver to support this.
>>>
>>
>> Heiko, the latest U-Boot tree hangs during boot on Tegra, and "git
>>
>
> :-(
>
> Could you enable debug printf?
>
>
>  bisect" points at this patch. Olof reported the issue to me.
>>
>
> Thanks!
>
>
>  Can you take a look at the code and see what might be wrong? Thanks.
>>
>
> Yep.
>
>
>  I suspect some kind of initialization ordering issue, since the boot
>> messages are:
>>
>> -----
>> U-Boot SPL 2013.07-rc3-00038-g880540d (Jul 29 2013 - 10:04:37)
>> U-Boot 2013.07-rc3-00038-g880540d (Jul 29 2013 - 10:04:37)
>>
>> TEGRA30
>> Board: NVIDIA Beaver
>> I2C:   Caller requested bad clock: periph=-49, parent=2
>> -----
>>
>> ... and that "bad clock" message implies to me that the I2C driver is
>> initializing before it has parsed the correct clock ID out of device tree.
>>
>
> Hmm... looking in the patch ... I can see nothing which changes
> some initializing order ...
>
> @Simon: Do you have an idea?
>
> just found some wrong settings for tegra30:
>
> In include/configs/tegra30-**common.h:
> /* Total I2C ports on Tegra30 */
> #define TEGRA_I2C_NUM_CONTROLLERS       5
>
> README says:
>                - drivers/i2c/tegra_i2c.c:
>
>                 - activate this driver with CONFIG_SYS_I2C_TEGRA
>                 - This driver adds 4 i2c buses with a fix speed from
>                   100000 and the slave addr 0!
>
> end yes, in the i2c driver are only 4 ports activated ... this
> should be changed ... but I think, this has nothing to do with
> your problem ... but try to add in the i2c driver one more i2c adapter
> for the case TEGRA_I2C_NUM_CONTROLLERS > 4
>
>
>  Some later commit causes the hang to happen right after printing "I2C:",
>> without printing the "bad clock" message. I didn't investigate that,
>> since I'm assuming the root-cause is the same. Most likely some later
>> commit causes the uninitialized data to be a valid clock, yet not the
>> actual I2C clock, so the I2C clock still isn't turned on, and touching
>> HW (i.e. reading/writing the I2C registers) without a running clock on
>> Tegra caused hard hangs.
>>
>
> digging deeper, the above "bad clock" message
>
> is a result from calling this function from the i2c driver:
> ./drivers/i2c/tegra_i2c.c:
> static void i2c_init_controller(struct i2c_bus *i2c_bus)
> {
>         /*
>          * Use PLLP - DP-04508-001_v06 datasheet indicates a divisor of 8
>          * here, in section 23.3.1, but in fact we seem to need a factor of
>          * 16 to get the right frequency.
>          */
>         clock_start_periph_pll(i2c_**bus->periph_id, CLOCK_ID_PERIPH,
>                 i2c_bus->speed * 2 * 8);
>
> Please enable debug printfs and look from where i2c_init_controller()
> is called. You should see the following debug printf if it go the right
> way (Just reading code, I have no HW ...)
>
> process_nodes():
>                 debug("%s: controller bus %d at %p, periph_id %d, speed
> %d: ",
>                       is_dvc ? "dvc" : "i2c", i, i2c_bus->regs,
>                       i2c_bus->periph_id, i2c_bus->speed);
>
> called from i2c_init_board in this driver.
>
> This should be called from drivers/i2c/i2c_core.c i2c_init_all()
> called from arch/arm/lib/board.c init_func_i2c()
>
> I think i2c_bus->periph_id ("periph=-49") is not setup right ... do
> you have the correct dt?


I am not sure what is wrong here - Stephen if you have a board and can
debug please do, otherwise I might be able to dig one out.

49 looks to be PERIPH_ID_TVO.

Regards,
Simon
Stephen Warren - July 30, 2013, 6:56 p.m.
On 07/29/2013 10:28 PM, Heiko Schocher wrote:
> Hello Stephen,
> 
> Am 29.07.2013 18:12, schrieb Stephen Warren:
>> On 05/04/2013 06:01 AM, Heiko Schocher wrote:
>>> From: Simon Glass<sjg@chromium.org>
>>>
>>> This enables CONFIG_SYS_I2C on Tegra, updating existing boards and
>>> the Tegra
>>> i2c driver to support this.
>>
>> Heiko, the latest U-Boot tree hangs during boot on Tegra, and "git
> 
> :-(
> 
> Could you enable debug printf?
> 
>> bisect" points at this patch. Olof reported the issue to me.
> 
> Thanks!
> 
>> Can you take a look at the code and see what might be wrong? Thanks.
> 
> Yep.
> 
>> I suspect some kind of initialization ordering issue, since the boot
>> messages are:
>>
>> -----
>> U-Boot SPL 2013.07-rc3-00038-g880540d (Jul 29 2013 - 10:04:37)
>> U-Boot 2013.07-rc3-00038-g880540d (Jul 29 2013 - 10:04:37)
>>
>> TEGRA30
>> Board: NVIDIA Beaver
>> I2C:   Caller requested bad clock: periph=-49, parent=2
>> -----
>>
>> ... and that "bad clock" message implies to me that the I2C driver is
>> initializing before it has parsed the correct clock ID out of device
>> tree.
> 
> Hmm... looking in the patch ... I can see nothing which changes
> some initializing order ...
> 
> @Simon: Do you have an idea?
> 
> just found some wrong settings for tegra30:
> 
> In include/configs/tegra30-common.h:
> /* Total I2C ports on Tegra30 */
> #define TEGRA_I2C_NUM_CONTROLLERS       5
> 
> README says:
>                - drivers/i2c/tegra_i2c.c:
>                 - activate this driver with CONFIG_SYS_I2C_TEGRA
>                 - This driver adds 4 i2c buses with a fix speed from
>                   100000 and the slave addr 0!

I think that's just stale documentation; Tegra20 had just 4 I2C
controllers, and the docs probably weren't updated for Tegra30 which has 5.

> end yes, in the i2c driver are only 4 ports activated ... this

I don't see any limit in the driver; everything seems to use
TEGRA_I2C_NUM_CONTROLLERS.

> should be changed ... but I think, this has nothing to do with
> your problem ... but try to add in the i2c driver one more i2c adapter
> for the case TEGRA_I2C_NUM_CONTROLLERS > 4
Stephen Warren - July 30, 2013, 7:22 p.m.
On 07/29/2013 10:28 PM, Heiko Schocher wrote:
> Hello Stephen,
> 
> Am 29.07.2013 18:12, schrieb Stephen Warren:
>> On 05/04/2013 06:01 AM, Heiko Schocher wrote:
>>> From: Simon Glass<sjg@chromium.org>
>>>
>>> This enables CONFIG_SYS_I2C on Tegra, updating existing boards and
>>> the Tegra
>>> i2c driver to support this.
>>
>> Heiko, the latest U-Boot tree hangs during boot on Tegra, and "git
>> bisect" points at this patch. Olof reported the issue to me.
>> Can you take a look at the code and see what might be wrong? Thanks.
>>
>> I suspect some kind of initialization ordering issue, since the boot
>> messages are:
>>
>> -----
>> U-Boot SPL 2013.07-rc3-00038-g880540d (Jul 29 2013 - 10:04:37)
>> U-Boot 2013.07-rc3-00038-g880540d (Jul 29 2013 - 10:04:37)
>>
>> TEGRA30
>> Board: NVIDIA Beaver
>> I2C:   Caller requested bad clock: periph=-49, parent=2
>> -----
>>
>> ... and that "bad clock" message implies to me that the I2C driver is
>> initializing before it has parsed the correct clock ID out of device
>> tree.
> 
> Hmm... looking in the patch ... I can see nothing which changes
> some initializing order ...

Yes, there's some initialization order issue; before this patch, I see
the I2C controller initialization, followed by some usage of it:

----------
U-Boot SPL 2013.07-rc3-00036-gd84eb85-dirty (Jul 30 2013 - 13:04:47)
U-Boot 2013.07-rc3-00036-gd84eb85-dirty (Jul 30 2013 - 13:04:47)

TEGRA30
Board: NVIDIA Beaver
DRAM:  2 GiB
i2c: controller bus 0 at 7000d000, periph_id 47, speed 100000: ok
i2c: controller bus 1 at 7000c000, periph_id 12, speed 100000: ok
i2c: controller bus 2 at 7000c400, periph_id 54, speed 100000: ok
i2c: controller bus 3 at 7000c500, periph_id 67, speed 100000: ok
i2c: controller bus 4 at 7000c700, periph_id 103, speed 100000: ok
MMC:   i2c_write: chip=0x2d, addr=0x32, len=0x1
----------

However with this patch applied, something starts using the controller
immediately, without it having been "probed" from device-tree:

----------
U-Boot SPL 2013.07-rc3-00037-g1f2ba72-dirty (Jul 30 2013 - 13:08:28)
U-Boot 2013.07-rc3-00037-g1f2ba72-dirty (Jul 30 2013 - 13:08:28)

TEGRA30
Board: NVIDIA Beaver
I2C:   i2c_init(speed=100000, slaveaddr=0xfe)
----------

i2c_init touches HW, but since process_nodes() hasn't run, none of the
parameters like register address or clock ID are yet known.

I think this call comes from init_sequence_f[] -> init_func_i2c() ->
i2c_init() -> i2c_init() == __i2c_init() -> i2c_init_bus() ->
I2C_ADAP->init(), although I didn't validate that in the running code,
just by code inspection.

The issue here is that the I2C core and/or Tegra driver seems to be
statically registering the I2C device objects, even though they should
be dynamically registered from device tree.

Should Tegra move its call of i2c_init_board() out of board_init() to
board_init_f(), and/or override __i2c_init() to call i2c_init_board()?

I think when init_sequence_f[] is running, there may be no serial
console to report errors. If so, moving the I2C initialization to that
early point sounds like a really bad idea.
Stephen Warren - July 30, 2013, 8 p.m.
On 07/30/2013 01:22 PM, Stephen Warren wrote:
> On 07/29/2013 10:28 PM, Heiko Schocher wrote:
>> Hello Stephen,
>>
>> Am 29.07.2013 18:12, schrieb Stephen Warren:
>>> On 05/04/2013 06:01 AM, Heiko Schocher wrote:
>>>> From: Simon Glass<sjg@chromium.org>
>>>>
>>>> This enables CONFIG_SYS_I2C on Tegra, updating existing boards and
>>>> the Tegra
>>>> i2c driver to support this.
>>>
>>> Heiko, the latest U-Boot tree hangs during boot on Tegra, and "git
>>> bisect" points at this patch. Olof reported the issue to me.
>>> Can you take a look at the code and see what might be wrong? Thanks.
>>>
>>> I suspect some kind of initialization ordering issue, since the boot
>>> messages are:
>>>
>>> -----
>>> U-Boot SPL 2013.07-rc3-00038-g880540d (Jul 29 2013 - 10:04:37)
>>> U-Boot 2013.07-rc3-00038-g880540d (Jul 29 2013 - 10:04:37)
>>>
>>> TEGRA30
>>> Board: NVIDIA Beaver
>>> I2C:   Caller requested bad clock: periph=-49, parent=2
>>> -----
>>>
>>> ... and that "bad clock" message implies to me that the I2C driver is
>>> initializing before it has parsed the correct clock ID out of device
>>> tree.
>>
>> Hmm... looking in the patch ... I can see nothing which changes
>> some initializing order ...
> 
> Yes, there's some initialization order issue; before this patch, I see
> the I2C controller initialization, followed by some usage of it:
> 
> ----------
> U-Boot SPL 2013.07-rc3-00036-gd84eb85-dirty (Jul 30 2013 - 13:04:47)
> U-Boot 2013.07-rc3-00036-gd84eb85-dirty (Jul 30 2013 - 13:04:47)
> 
> TEGRA30
> Board: NVIDIA Beaver
> DRAM:  2 GiB
> i2c: controller bus 0 at 7000d000, periph_id 47, speed 100000: ok
> i2c: controller bus 1 at 7000c000, periph_id 12, speed 100000: ok
> i2c: controller bus 2 at 7000c400, periph_id 54, speed 100000: ok
> i2c: controller bus 3 at 7000c500, periph_id 67, speed 100000: ok
> i2c: controller bus 4 at 7000c700, periph_id 103, speed 100000: ok
> MMC:   i2c_write: chip=0x2d, addr=0x32, len=0x1
> ----------
> 
> However with this patch applied, something starts using the controller
> immediately, without it having been "probed" from device-tree:
> 
> ----------
> U-Boot SPL 2013.07-rc3-00037-g1f2ba72-dirty (Jul 30 2013 - 13:08:28)
> U-Boot 2013.07-rc3-00037-g1f2ba72-dirty (Jul 30 2013 - 13:08:28)
> 
> TEGRA30
> Board: NVIDIA Beaver
> I2C:   i2c_init(speed=100000, slaveaddr=0xfe)
> ----------
> 
> i2c_init touches HW, but since process_nodes() hasn't run, none of the
> parameters like register address or clock ID are yet known.
> 
> I think this call comes from init_sequence_f[] -> init_func_i2c() ->
> i2c_init() -> i2c_init() == __i2c_init() -> i2c_init_bus() ->
> I2C_ADAP->init(), although I didn't validate that in the running code,
> just by code inspection.

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 think the only way to solve this is not to use DT to instantiate
devices, or to move the I2C initialization after relocation etc.,
although the latter won't work on boards that need I2C up in order to
initialize DRAM.

It seems like much of U-Boot's initialization architecture simply wasn't
designed to accommodate dynamically initializing devices from DT.
Simon Glass - July 30, 2013, 9:19 p.m.
Hi Stephen,

On Tue, Jul 30, 2013 at 1:22 PM, Stephen Warren <swarren@wwwdotorg.org>wrote:

> On 07/29/2013 10:28 PM, Heiko Schocher wrote:
> > Hello Stephen,
> >
> > Am 29.07.2013 18:12, schrieb Stephen Warren:
> >> On 05/04/2013 06:01 AM, Heiko Schocher wrote:
> >>> From: Simon Glass<sjg@chromium.org>
> >>>
> >>> This enables CONFIG_SYS_I2C on Tegra, updating existing boards and
> >>> the Tegra
> >>> i2c driver to support this.
> >>
> >> Heiko, the latest U-Boot tree hangs during boot on Tegra, and "git
> >> bisect" points at this patch. Olof reported the issue to me.
> >> Can you take a look at the code and see what might be wrong? Thanks.
> >>
> >> I suspect some kind of initialization ordering issue, since the boot
> >> messages are:
> >>
> >> -----
> >> U-Boot SPL 2013.07-rc3-00038-g880540d (Jul 29 2013 - 10:04:37)
> >> U-Boot 2013.07-rc3-00038-g880540d (Jul 29 2013 - 10:04:37)
> >>
> >> TEGRA30
> >> Board: NVIDIA Beaver
> >> I2C:   Caller requested bad clock: periph=-49, parent=2
> >> -----
> >>
> >> ... and that "bad clock" message implies to me that the I2C driver is
> >> initializing before it has parsed the correct clock ID out of device
> >> tree.
> >
> > Hmm... looking in the patch ... I can see nothing which changes
> > some initializing order ...
>
> Yes, there's some initialization order issue; before this patch, I see
> the I2C controller initialization, followed by some usage of it:
>
> ----------
> U-Boot SPL 2013.07-rc3-00036-gd84eb85-dirty (Jul 30 2013 - 13:04:47)
> U-Boot 2013.07-rc3-00036-gd84eb85-dirty (Jul 30 2013 - 13:04:47)
>
> TEGRA30
> Board: NVIDIA Beaver
> DRAM:  2 GiB
> i2c: controller bus 0 at 7000d000, periph_id 47, speed 100000: ok
> i2c: controller bus 1 at 7000c000, periph_id 12, speed 100000: ok
> i2c: controller bus 2 at 7000c400, periph_id 54, speed 100000: ok
> i2c: controller bus 3 at 7000c500, periph_id 67, speed 100000: ok
> i2c: controller bus 4 at 7000c700, periph_id 103, speed 100000: ok
> MMC:   i2c_write: chip=0x2d, addr=0x32, len=0x1
> ----------
>
> However with this patch applied, something starts using the controller
> immediately, without it having been "probed" from device-tree:
>
> ----------
> U-Boot SPL 2013.07-rc3-00037-g1f2ba72-dirty (Jul 30 2013 - 13:08:28)
> U-Boot 2013.07-rc3-00037-g1f2ba72-dirty (Jul 30 2013 - 13:08:28)
>
> TEGRA30
> Board: NVIDIA Beaver
> I2C:   i2c_init(speed=100000, slaveaddr=0xfe)
> ----------
>
> i2c_init touches HW, but since process_nodes() hasn't run, none of the
> parameters like register address or clock ID are yet known.
>
> I think this call comes from init_sequence_f[] -> init_func_i2c() ->
> i2c_init() -> i2c_init() == __i2c_init() -> i2c_init_bus() ->
> I2C_ADAP->init(), although I didn't validate that in the running code,
> just by code inspection.
>
> The issue here is that the I2C core and/or Tegra driver seems to be
> statically registering the I2C device objects, even though they should
> be dynamically registered from device tree.
>
> Should Tegra move its call of i2c_init_board() out of board_init() to
> board_init_f(), and/or override __i2c_init() to call i2c_init_board()?


Something like that. We need i2c_init_board() to be called earlier, now
that the init sequence is doing i2c on ARM.


>
> I think when init_sequence_f[] is running, there may be no serial
> console to report errors. If so, moving the I2C initialization to that
> early point sounds like a really bad idea.
>

Not really - when you see the U-Boot banner the console is working, and we
clearly see the U-Boot banner before i2c init happens.

Regards,
Simon
Stephen Warren - July 30, 2013, 9:21 p.m.
On 07/30/2013 03:19 PM, Simon Glass wrote:
> Hi Stephen,
> 
> On Tue, Jul 30, 2013 at 1:22 PM, Stephen Warren <swarren@wwwdotorg.org
> <mailto:swarren@wwwdotorg.org>> wrote:
...
>     I think when init_sequence_f[] is running, there may be no serial
>     console to report errors. If so, moving the I2C initialization to that
>     early point sounds like a really bad idea.
> 
> 
> Not really - when you see the U-Boot banner the console is working, and
> we clearly see the U-Boot banner before i2c init happens.

I thought it got buffered up and only actually sent to the UART much
later, and not in the case when something failed since U-Boot wouldn't
get that far? Or, did that proposal get shot down?
Simon Glass - July 30, 2013, 9:21 p.m.
Hi Stephen,

On Tue, Jul 30, 2013 at 2:00 PM, Stephen Warren <swarren@wwwdotorg.org>wrote:

> On 07/30/2013 01:22 PM, Stephen Warren wrote:
> > On 07/29/2013 10:28 PM, Heiko Schocher wrote:
> >> Hello Stephen,
> >>
> >> Am 29.07.2013 18:12, schrieb Stephen Warren:
> >>> On 05/04/2013 06:01 AM, Heiko Schocher wrote:
> >>>> From: Simon Glass<sjg@chromium.org>
> >>>>
> >>>> This enables CONFIG_SYS_I2C on Tegra, updating existing boards and
> >>>> the Tegra
> >>>> i2c driver to support this.
> >>>
> >>> Heiko, the latest U-Boot tree hangs during boot on Tegra, and "git
> >>> bisect" points at this patch. Olof reported the issue to me.
> >>> Can you take a look at the code and see what might be wrong? Thanks.
> >>>
> >>> I suspect some kind of initialization ordering issue, since the boot
> >>> messages are:
> >>>
> >>> -----
> >>> U-Boot SPL 2013.07-rc3-00038-g880540d (Jul 29 2013 - 10:04:37)
> >>> U-Boot 2013.07-rc3-00038-g880540d (Jul 29 2013 - 10:04:37)
> >>>
> >>> TEGRA30
> >>> Board: NVIDIA Beaver
> >>> I2C:   Caller requested bad clock: periph=-49, parent=2
> >>> -----
> >>>
> >>> ... and that "bad clock" message implies to me that the I2C driver is
> >>> initializing before it has parsed the correct clock ID out of device
> >>> tree.
> >>
> >> Hmm... looking in the patch ... I can see nothing which changes
> >> some initializing order ...
> >
> > Yes, there's some initialization order issue; before this patch, I see
> > the I2C controller initialization, followed by some usage of it:
> >
> > ----------
> > U-Boot SPL 2013.07-rc3-00036-gd84eb85-dirty (Jul 30 2013 - 13:04:47)
> > U-Boot 2013.07-rc3-00036-gd84eb85-dirty (Jul 30 2013 - 13:04:47)
> >
> > TEGRA30
> > Board: NVIDIA Beaver
> > DRAM:  2 GiB
> > i2c: controller bus 0 at 7000d000, periph_id 47, speed 100000: ok
> > i2c: controller bus 1 at 7000c000, periph_id 12, speed 100000: ok
> > i2c: controller bus 2 at 7000c400, periph_id 54, speed 100000: ok
> > i2c: controller bus 3 at 7000c500, periph_id 67, speed 100000: ok
> > i2c: controller bus 4 at 7000c700, periph_id 103, speed 100000: ok
> > MMC:   i2c_write: chip=0x2d, addr=0x32, len=0x1
> > ----------
> >
> > However with this patch applied, something starts using the controller
> > immediately, without it having been "probed" from device-tree:
> >
> > ----------
> > U-Boot SPL 2013.07-rc3-00037-g1f2ba72-dirty (Jul 30 2013 - 13:08:28)
> > U-Boot 2013.07-rc3-00037-g1f2ba72-dirty (Jul 30 2013 - 13:08:28)
> >
> > TEGRA30
> > Board: NVIDIA Beaver
> > I2C:   i2c_init(speed=100000, slaveaddr=0xfe)
> > ----------
> >
> > i2c_init touches HW, but since process_nodes() hasn't run, none of the
> > parameters like register address or clock ID are yet known.
> >
> > I think this call comes from init_sequence_f[] -> init_func_i2c() ->
> > i2c_init() -> i2c_init() == __i2c_init() -> i2c_init_bus() ->
> > I2C_ADAP->init(), although I didn't validate that in the running code,
> > just by code inspection.
>
> 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.


>
> I think the only way to solve this is not to use DT to instantiate
> devices, or to move the I2C initialization after relocation etc.,
> although the latter won't work on boards that need I2C up in order to
> initialize DRAM.
>
> It seems like much of U-Boot's initialization architecture simply wasn't
> designed to accommodate dynamically initializing devices from DT.
>

True, although remember that very little init happens before relocation.
Here, I2C has moved to pre-reloc. But the vast majority of it happens after
reloc, so the actual impact of this problem is small, and there is a
workaround (above).

Regards,
Simon
Stephen Warren - July 30, 2013, 9:32 p.m.
On 07/30/2013 03:21 PM, Simon Glass wrote:
> On Tue, Jul 30, 2013 at 2:00 PM, Stephen Warren <swarren@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.

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.
Simon Glass - July 30, 2013, 9:45 p.m.
On Tue, Jul 30, 2013 at 3:21 PM, Stephen Warren <swarren@wwwdotorg.org>wrote:

> On 07/30/2013 03:19 PM, Simon Glass wrote:
> > Hi Stephen,
> >
> > On Tue, Jul 30, 2013 at 1:22 PM, Stephen Warren <swarren@wwwdotorg.org
> > <mailto:swarren@wwwdotorg.org>> wrote:
> ...
> >     I think when init_sequence_f[] is running, there may be no serial
> >     console to report errors. If so, moving the I2C initialization to
> that
> >     early point sounds like a really bad idea.
> >
> >
> > Not really - when you see the U-Boot banner the console is working, and
> > we clearly see the U-Boot banner before i2c init happens.
>
> I thought it got buffered up and only actually sent to the UART much
> later, and not in the case when something failed since U-Boot wouldn't
> get that far? Or, did that proposal get shot down?
>

There is an option to buffer output until console_init_f() is called, but
that is called very early...
Simon Glass - July 30, 2013, 9:46 p.m.
On Tue, Jul 30, 2013 at 3:32 PM, Stephen Warren <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@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.


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

Regards,
Simon
Stephen Warren - July 30, 2013, 9:51 p.m.
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@wwwdotorg.org <mailto:swarren@wwwdotorg.org>
>     > <mailto:swarren@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.

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, 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.
Simon Glass - July 30, 2013, 10:05 p.m.
Hi Stephen,

On Tue, Jul 30, 2013 at 3:51 PM, 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@wwwdotorg.org <mailto:swarren@wwwdotorg.org>
> >     > <mailto:swarren@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.
>
> 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
Albert ARIBAUD - July 30, 2013, 10:09 p.m.
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@wwwdotorg.org <mailto:swarren@wwwdotorg.org>
> >     > <mailto:swarren@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,
Simon Glass - July 30, 2013, 10:11 p.m.
Hi Albert,

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

> 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@wwwdotorg.org <mailto:swarren@wwwdotorg.org>
> > >     > <mailto:swarren@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?
>

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
Heiko Schocher - July 31, 2013, 4:29 a.m.
Hello Stephen

Am 30.07.2013 20:56, schrieb Stephen Warren:
> On 07/29/2013 10:28 PM, Heiko Schocher wrote:
>> Hello Stephen,
>>
>> Am 29.07.2013 18:12, schrieb Stephen Warren:
>>> On 05/04/2013 06:01 AM, Heiko Schocher wrote:
>>>> From: Simon Glass<sjg@chromium.org>
>>>>
>>>> This enables CONFIG_SYS_I2C on Tegra, updating existing boards and
>>>> the Tegra
>>>> i2c driver to support this.
>>>
>>> Heiko, the latest U-Boot tree hangs during boot on Tegra, and "git
>>
>> :-(
>>
>> Could you enable debug printf?
>>
>>> bisect" points at this patch. Olof reported the issue to me.
>>
>> Thanks!
>>
>>> Can you take a look at the code and see what might be wrong? Thanks.
>>
>> Yep.
>>
>>> I suspect some kind of initialization ordering issue, since the boot
>>> messages are:
>>>
>>> -----
>>> U-Boot SPL 2013.07-rc3-00038-g880540d (Jul 29 2013 - 10:04:37)
>>> U-Boot 2013.07-rc3-00038-g880540d (Jul 29 2013 - 10:04:37)
>>>
>>> TEGRA30
>>> Board: NVIDIA Beaver
>>> I2C:   Caller requested bad clock: periph=-49, parent=2
>>> -----
>>>
>>> ... and that "bad clock" message implies to me that the I2C driver is
>>> initializing before it has parsed the correct clock ID out of device
>>> tree.
>>
>> Hmm... looking in the patch ... I can see nothing which changes
>> some initializing order ...
>>
>> @Simon: Do you have an idea?
>>
>> just found some wrong settings for tegra30:
>>
>> In include/configs/tegra30-common.h:
>> /* Total I2C ports on Tegra30 */
>> #define TEGRA_I2C_NUM_CONTROLLERS       5
>>
>> README says:
>>                 - drivers/i2c/tegra_i2c.c:
>>                  - activate this driver with CONFIG_SYS_I2C_TEGRA
>>                  - This driver adds 4 i2c buses with a fix speed from
>>                    100000 and the slave addr 0!
>
> I think that's just stale documentation; Tegra20 had just 4 I2C
> controllers, and the docs probably weren't updated for Tegra30 which has 5.
>
>> end yes, in the i2c driver are only 4 ports activated ... this
>
> I don't see any limit in the driver; everything seems to use
> TEGRA_I2C_NUM_CONTROLLERS.
>
>> should be changed ... but I think, this has nothing to do with
>> your problem ... but try to add in the i2c driver one more i2c adapter
>> for the case TEGRA_I2C_NUM_CONTROLLERS>  4

As I wrote here, add (at the end of the file):

#if TEGRA_I2C_NUM_CONTROLLERS > 4
U_BOOT_I2C_ADAP_COMPLETE(tegra4, tegra_i2c_init, tegra_i2c_probe,
                          tegra_i2c_read, tegra_i2c_write,
                          tegra_i2c_set_bus_speed, 100000, 0, 4)
#endif

bye,
Heiko
Heiko Schocher - July 31, 2013, 5:01 a.m.
Hello Stephen,

Am 30.07.2013 21:22, schrieb Stephen Warren:
> On 07/29/2013 10:28 PM, Heiko Schocher wrote:
>> Hello Stephen,
>>
>> Am 29.07.2013 18:12, schrieb Stephen Warren:
>>> On 05/04/2013 06:01 AM, Heiko Schocher wrote:
>>>> From: Simon Glass<sjg@chromium.org>
>>>>
>>>> This enables CONFIG_SYS_I2C on Tegra, updating existing boards and
>>>> the Tegra
>>>> i2c driver to support this.
>>>
>>> Heiko, the latest U-Boot tree hangs during boot on Tegra, and "git
>>> bisect" points at this patch. Olof reported the issue to me.
>>> Can you take a look at the code and see what might be wrong? Thanks.
>>>
>>> I suspect some kind of initialization ordering issue, since the boot
>>> messages are:
>>>
>>> -----
>>> U-Boot SPL 2013.07-rc3-00038-g880540d (Jul 29 2013 - 10:04:37)
>>> U-Boot 2013.07-rc3-00038-g880540d (Jul 29 2013 - 10:04:37)
>>>
>>> TEGRA30
>>> Board: NVIDIA Beaver
>>> I2C:   Caller requested bad clock: periph=-49, parent=2
>>> -----
>>>
>>> ... and that "bad clock" message implies to me that the I2C driver is
>>> initializing before it has parsed the correct clock ID out of device
>>> tree.
>>
>> Hmm... looking in the patch ... I can see nothing which changes
>> some initializing order ...
>
> Yes, there's some initialization order issue; before this patch, I see
> the I2C controller initialization, followed by some usage of it:

Ok, great!

> ----------
> U-Boot SPL 2013.07-rc3-00036-gd84eb85-dirty (Jul 30 2013 - 13:04:47)
> U-Boot 2013.07-rc3-00036-gd84eb85-dirty (Jul 30 2013 - 13:04:47)
>
> TEGRA30
> Board: NVIDIA Beaver
> DRAM:  2 GiB
> i2c: controller bus 0 at 7000d000, periph_id 47, speed 100000: ok
> i2c: controller bus 1 at 7000c000, periph_id 12, speed 100000: ok
> i2c: controller bus 2 at 7000c400, periph_id 54, speed 100000: ok
> i2c: controller bus 3 at 7000c500, periph_id 67, speed 100000: ok
> i2c: controller bus 4 at 7000c700, periph_id 103, speed 100000: ok
> MMC:   i2c_write: chip=0x2d, addr=0x32, len=0x1
> ----------
>
> However with this patch applied, something starts using the controller
> immediately, without it having been "probed" from device-tree:

Hmm... do you have a debugger?

> ----------
> U-Boot SPL 2013.07-rc3-00037-g1f2ba72-dirty (Jul 30 2013 - 13:08:28)
> U-Boot 2013.07-rc3-00037-g1f2ba72-dirty (Jul 30 2013 - 13:08:28)
>
> TEGRA30
> Board: NVIDIA Beaver
> I2C:   i2c_init(speed=100000, slaveaddr=0xfe)
> ----------
>
> i2c_init touches HW, but since process_nodes() hasn't run, none of the
> parameters like register address or clock ID are yet known.
>
i> I think this call comes from init_sequence_f[] ->  init_func_i2c() ->
> i2c_init() ->  i2c_init() == __i2c_init() ->  i2c_init_bus() ->

No, we have now defined CONFIG_SYS_I2C and this calls

i2c_init_all() -> i2c_init_board()

and I think, on nvidia board i2c_init_board is defined. Yes, in the
i2c driver there it is ... calling process_nodes() ... so, the i2c
busses should be setup ...

> I2C_ADAP->init(), although I didn't validate that in the running code,
> just by code inspection.
>
> The issue here is that the I2C core and/or Tegra driver seems to be
> statically registering the I2C device objects, even though they should
> be dynamically registered from device tree.

Feel free to post patches.

> Should Tegra move its call of i2c_init_board() out of board_init() to
> board_init_f(), and/or override __i2c_init() to call i2c_init_board()?

No, i2c_init_board gets called here very early:
init_func_i2c() -> i2c_init_all():

static int init_func_i2c(void)
{
         puts("I2C:   ");
#ifdef CONFIG_SYS_I2C
         i2c_init_all();
#else
         i2c_init(CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE);
#endif
         puts("ready\n");
         return (0);
}

and we have CONFIG_SYS_I2C defined (or, did you have it?)

and in drivers/i2c/i2c_core.c:
void i2c_init_all(void)
{
         i2c_init_board();
         i2c_set_bus_num(CONFIG_SYS_SPD_BUS_NUM);
         return;
}

Ah, I see, it is also called in board_init ...

> I think when init_sequence_f[] is running, there may be no serial
> console to report errors. If so, moving the I2C initialization to that
> early point sounds like a really bad idea.

No, we need i2c before relocation for example to read SPD data
from eeprom, but this is on powerpc.
puts() is working, as your log shows.

I added in the comment from i2c_init_all:
/*
  * i2c_init_all():
  *
  * not longer needed, will deleted. Actual init the SPD_BUS
  * for compatibility.
  * i2c_adap[] must be initialized beforehead with function pointers and
  * data, including speed and slaveaddr.
  */

So the question raises, do we need this on arm?

bye,
Heiko
Heiko Schocher - July 31, 2013, 5:03 a.m.
Hello Stephen,

Am 30.07.2013 22:00, schrieb Stephen Warren:
> On 07/30/2013 01:22 PM, Stephen Warren wrote:
>> On 07/29/2013 10:28 PM, Heiko Schocher wrote:
>>> Hello Stephen,
>>>
>>> Am 29.07.2013 18:12, schrieb Stephen Warren:
>>>> On 05/04/2013 06:01 AM, Heiko Schocher wrote:
>>>>> From: Simon Glass<sjg@chromium.org>
>>>>>
>>>>> This enables CONFIG_SYS_I2C on Tegra, updating existing boards and
>>>>> the Tegra
>>>>> i2c driver to support this.
>>>>
>>>> Heiko, the latest U-Boot tree hangs during boot on Tegra, and "git
>>>> bisect" points at this patch. Olof reported the issue to me.
>>>> Can you take a look at the code and see what might be wrong? Thanks.
>>>>
>>>> I suspect some kind of initialization ordering issue, since the boot
>>>> messages are:
>>>>
>>>> -----
>>>> U-Boot SPL 2013.07-rc3-00038-g880540d (Jul 29 2013 - 10:04:37)
>>>> U-Boot 2013.07-rc3-00038-g880540d (Jul 29 2013 - 10:04:37)
>>>>
>>>> TEGRA30
>>>> Board: NVIDIA Beaver
>>>> I2C:   Caller requested bad clock: periph=-49, parent=2
>>>> -----
>>>>
>>>> ... and that "bad clock" message implies to me that the I2C driver is
>>>> initializing before it has parsed the correct clock ID out of device
>>>> tree.
>>>
>>> Hmm... looking in the patch ... I can see nothing which changes
>>> some initializing order ...
>>
>> Yes, there's some initialization order issue; before this patch, I see
>> the I2C controller initialization, followed by some usage of it:
>>
>> ----------
>> U-Boot SPL 2013.07-rc3-00036-gd84eb85-dirty (Jul 30 2013 - 13:04:47)
>> U-Boot 2013.07-rc3-00036-gd84eb85-dirty (Jul 30 2013 - 13:04:47)
>>
>> TEGRA30
>> Board: NVIDIA Beaver
>> DRAM:  2 GiB
>> i2c: controller bus 0 at 7000d000, periph_id 47, speed 100000: ok
>> i2c: controller bus 1 at 7000c000, periph_id 12, speed 100000: ok
>> i2c: controller bus 2 at 7000c400, periph_id 54, speed 100000: ok
>> i2c: controller bus 3 at 7000c500, periph_id 67, speed 100000: ok
>> i2c: controller bus 4 at 7000c700, periph_id 103, speed 100000: ok
>> MMC:   i2c_write: chip=0x2d, addr=0x32, len=0x1
>> ----------
>>
>> However with this patch applied, something starts using the controller
>> immediately, without it having been "probed" from device-tree:
>>
>> ----------
>> U-Boot SPL 2013.07-rc3-00037-g1f2ba72-dirty (Jul 30 2013 - 13:08:28)
>> U-Boot 2013.07-rc3-00037-g1f2ba72-dirty (Jul 30 2013 - 13:08:28)
>>
>> TEGRA30
>> Board: NVIDIA Beaver
>> I2C:   i2c_init(speed=100000, slaveaddr=0xfe)
>> ----------
>>
>> i2c_init touches HW, but since process_nodes() hasn't run, none of the
>> parameters like register address or clock ID are yet known.
>>
>> I think this call comes from init_sequence_f[] ->  init_func_i2c() ->
>> i2c_init() ->  i2c_init() == __i2c_init() ->  i2c_init_bus() ->
>> I2C_ADAP->init(), although I didn't validate that in the running code,
>> just by code inspection.
>
> 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.

Yes, you are right, this would not work on tegra.

> I think the only way to solve this is not to use DT to instantiate
> devices, or to move the I2C initialization after relocation etc.,
> although the latter won't work on boards that need I2C up in order to
> initialize DRAM.

Yes.

> It seems like much of U-Boot's initialization architecture simply wasn't
> designed to accommodate dynamically initializing devices from DT.

Yes.

bye,
Heiko
Wolfgang Denk - July 31, 2013, 5:18 a.m.
Dear Albert ARIBAUD,

In message <20130731000921.724f5c71@lilith> you wrote:
> 
> board_init_f() is supposed to initialize just enough of the system to
> allow relocation. Is initializing i2c necessary in this context?

On some boards, yes.  For example, if they store the environment in an
I2C attached EEPROM. Then you need I2C support early, before console,
to read for example the baudrate setting.

Best regards,

Wolfgang Denk
Heiko Schocher - July 31, 2013, 5:46 a.m.
Hello Simon,

Am 31.07.2013 00:05, schrieb Simon Glass:
> Hi Stephen,
>
> On Tue, Jul 30, 2013 at 3:51 PM, 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@wwwdotorg.org<mailto:swarren@wwwdotorg.org>
>>>      >  <mailto:swarren@wwwdotorg.org<mailto:swarren@wwwdotorg.org>>>
>> wrote:
[...]
>>>      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 thought you are testing the tegra plattforms with this patchset,
as you triggered a few times, when this will go to mainline ...

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

Yes, lets go ahead.

A goal of the i2c rework is to get rid of all i2c_init* calls
around the code, and only use i2c_set_bus_num() before accessing
the i2c subsystem (which looks if it needs to switch to this bus,
is it initialized, muxes setup, ... ) ... maybe add the "bus"
parameter as a second step to the i2c api, and i2c_set_bus_num()
is no longer needed, just internal in the i2c subsystem ...

Maybe we can make i2c_init_func() weak (in the CONFIG_SYS_I2C case),
and define it only on such boards, which need i2c so early?
(Or remove it for the CONFIG_SYS_I2C completely, so boards which
  need early i2c access and switch to the new framework would add
  a i2c_set_bus_num() where they need it ...)

Hmm.. each i2c adapter has its own init function ... why the tegra
driver do not use it? And do the necessary inits in it? So we
initialize an adapater only if we use it, which is also a rule
for u-boot ...

I have no hw, but it should be possible to add to process_nodes()
a parameter "controller_number" and call
process_nodes(controller_number, ...) from the i2c drivers init
function ...

So I see two steps:
- make i2c_init_func weak, and define this function only on boards
   which need i2c so early, or remove it completely ... and see
   which boards fail.
- change i2c tegra driver as described above

Maybe this is a way to go?

bye,
Heiko
Heiko Schocher - July 31, 2013, 5:52 a.m.
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@wwwdotorg.org<mailto:swarren@wwwdotorg.org>
>>>      >  <mailto:swarren@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
Heiko Schocher - July 31, 2013, 5:55 a.m.
Hello Wolfgang,

Am 31.07.2013 07:18, schrieb Wolfgang Denk:
> Dear Albert ARIBAUD,
>
> In message<20130731000921.724f5c71@lilith>  you wrote:
>>
>> board_init_f() is supposed to initialize just enough of the system to
>> allow relocation. Is initializing i2c necessary in this context?
>
> On some boards, yes.  For example, if they store the environment in an
> I2C attached EEPROM. Then you need I2C support early, before console,
> to read for example the baudrate setting.

Ah, yes. Hmm.. how did in such a case the puts() call in init_func_i2c()
work?

bye,
Heiko
Albert ARIBAUD - July 31, 2013, 7:06 a.m.
Hi Wolfgang,

On Wed, 31 Jul 2013 07:18:21 +0200, Wolfgang Denk <wd@denx.de> wrote:

> Dear Albert ARIBAUD,
> 
> In message <20130731000921.724f5c71@lilith> you wrote:
> > 
> > board_init_f() is supposed to initialize just enough of the system to
> > allow relocation. Is initializing i2c necessary in this context?
> 
> On some boards, yes.  For example, if they store the environment in an
> I2C attached EEPROM. Then you need I2C support early, before console,
> to read for example the baudrate setting.

Thanks, so some I2C reads are needed. Next question: on these boards,
do these I2C reads require DT reads? Maybe a few hard-coded low level
I2C reads are enough. I guess DT writes are completely unneeded at
that point. Also, why exactly do I2C and, as the case may be, DT, need
to write to .data?

More generally, while I think the board_init_f() part of U-Boot should
be as short and compact as possible, I understand and admit that it
might have to read from just about any (local) storage resource, be it
environment or DT or any stored information it needs.

On the other hand, it may be hard to immediately know what functions
throughout U-boot are safe to call from within board_init_f(); maybe we
should start thinking about checking and marking these, the simplest
way being to suffix them with "_f" once we have made sure they are safe
to call from within board_init_f().

But we should strictly limit the scope of board_init_f() or we'll find
the board_init_f()/board_init_r() pair following a patch similar to the
SPL/U-Boot pair, where SPL started out as a tiny helper piece of code
and ending up a resizeable (and, I dare to say, sizeable as well) kind
of U-boot. If we let too many features slip in board_init_f(), it'll
blur into a board_init_r() like and before we know it, it'll *require*
DDR, and write access to it too...

So, board_init_() should *strictly* be limited to setting up a console
(for information purposes) and giving access to DDR while in the same
time never writing to it itself. Bonus points if it can limit itself to
*enabling* and postpone any *optimizing*(I am thinking of DDR settings
here and no, I don't have specific existing cases in mind; just sayin').

In the present instance, I'd rather we either:

- removed dependency on DT etc. by using "hard-coded" low level I2C
  reads for those boards that need it (I assume that for each of these
  boards the I2C slave, offset, and length to read are constant) in _f
  phase, or

- parsed the _f phase looking for offending functions or calls which
  write to .data or .bss and fix them, suffixing them with _f; in
  essence, that amounts to starting the implementation of my suggestion
  above alongside fixing the issue at hand.

The first approach is rather "let's bring the thing back up first", so
it does not have my preference, but I would understand the need to
quickly fix things.

The second approach seems to be going in the same direction as Heiko's
proposal of 07:52 +0200, which I thus second provided it is applicable
to all the boards Wolfgang had in mind.

> Best regards,
> 
> Wolfgang Denk

Amicalement,
Heiko Schocher - July 31, 2013, 7:36 a.m.
Hello Albert,

Am 31.07.2013 09:06, schrieb Albert ARIBAUD:
> Hi Wolfgang,
>
> On Wed, 31 Jul 2013 07:18:21 +0200, Wolfgang Denk<wd@denx.de>  wrote:
>
>> Dear Albert ARIBAUD,
>>
>> In message<20130731000921.724f5c71@lilith>  you wrote:
>>>
>>> board_init_f() is supposed to initialize just enough of the system to
>>> allow relocation. Is initializing i2c necessary in this context?
>>
>> On some boards, yes.  For example, if they store the environment in an
>> I2C attached EEPROM. Then you need I2C support early, before console,
>> to read for example the baudrate setting.
>
> Thanks, so some I2C reads are needed. Next question: on these boards,
> do these I2C reads require DT reads? Maybe a few hard-coded low level

If I understand the tegra driver, dt reads are needed for init
the i2c driver ... but this should be done in the i2c driver
init function ... and this should be possible without writes

> I2C reads are enough. I guess DT writes are completely unneeded at
> that point. Also, why exactly do I2C and, as the case may be, DT, need
> to write to .data?

I2c do not write before relocation (I hope so), see:
drivers/i2c/i2c_core.c

All writes to internal i2c structs are (should be) protected by a:

if (gd->flags & GD_FLG_RELOC) {

but using i2c write/reads are possible.

> More generally, while I think the board_init_f() part of U-Boot should
> be as short and compact as possible, I understand and admit that it
> might have to read from just about any (local) storage resource, be it
> environment or DT or any stored information it needs.
>
> On the other hand, it may be hard to immediately know what functions
> throughout U-boot are safe to call from within board_init_f(); maybe we
> should start thinking about checking and marking these, the simplest
> way being to suffix them with "_f" once we have made sure they are safe
> to call from within board_init_f().

Hmmm... Maybe instead we should think (also in thinking common bring
up for all boards) about:

getting rid of board_init_f in u-boot code, instead use for all
boards spl code to init needed things and copy and relocate u-boot
to ram in spl code ... so we have in u-boot no longer such
restictions ... but thats just an idea which whirs in my head ...
without thinking to deep in it.

But this approach would have some advantages ...

> But we should strictly limit the scope of board_init_f() or we'll find
> the board_init_f()/board_init_r() pair following a patch similar to the
> SPL/U-Boot pair, where SPL started out as a tiny helper piece of code
> and ending up a resizeable (and, I dare to say, sizeable as well) kind
> of U-boot. If we let too many features slip in board_init_f(), it'll
> blur into a board_init_r() like and before we know it, it'll *require*
> DDR, and write access to it too...
>
> So, board_init_() should *strictly* be limited to setting up a console
> (for information purposes) and giving access to DDR while in the same
> time never writing to it itself. Bonus points if it can limit itself to
> *enabling* and postpone any *optimizing*(I am thinking of DDR settings
> here and no, I don't have specific existing cases in mind; just sayin').
>
> In the present instance, I'd rather we either:
>
> - removed dependency on DT etc. by using "hard-coded" low level I2C
>    reads for those boards that need it (I assume that for each of these
>    boards the I2C slave, offset, and length to read are constant) in _f
>    phase, or

But DT is used for initializing the i2c driver in tegra ...

> - parsed the _f phase looking for offending functions or calls which
>    write to .data or .bss and fix them, suffixing them with _f; in
>    essence, that amounts to starting the implementation of my suggestion
>    above alongside fixing the issue at hand.
>
> The first approach is rather "let's bring the thing back up first", so
> it does not have my preference, but I would understand the need to
> quickly fix things.

Yes.

> The second approach seems to be going in the same direction as Heiko's
> proposal of 07:52 +0200, which I thus second provided it is applicable
> to all the boards Wolfgang had in mind.

Lets do us this step as fixup ;-)

bye,
Heiko
Albert ARIBAUD - July 31, 2013, 8:16 a.m.
Hi Heiko,

On Wed, 31 Jul 2013 09:36:19 +0200, Heiko Schocher <hs@denx.de> wrote:

> > On the other hand, it may be hard to immediately know what functions
> > throughout U-boot are safe to call from within board_init_f(); maybe we
> > should start thinking about checking and marking these, the simplest
> > way being to suffix them with "_f" once we have made sure they are safe
> > to call from within board_init_f().
> 
> Hmmm... Maybe instead we should think (also in thinking common bring
> up for all boards) about:
> 
> getting rid of board_init_f in u-boot code, instead use for all
> boards spl code to init needed things and copy and relocate u-boot
> to ram in spl code ... so we have in u-boot no longer such
> restictions ... but thats just an idea which whirs in my head ...
> without thinking to deep in it.
> 
> But this approach would have some advantages ...

Well, the original SPL was basically board_init_f() plus some code to
copy U-Boot from wherever it was to DDR, so it was tightly linked to
board_init_f(). But... first, SPL has evolved into a "U-Boot lite"
where much can happen beyond board_init_f() -- think Falcon mode, for
instance -- and second, there are boards which do not have SPL at all,
and their board_init_f() can thus not be "moved to SPL".

So no, I don't think we can move U-Boot's design from "_f/_r" to
"SPL/U-Boot".

> > But we should strictly limit the scope of board_init_f() or we'll find
> > the board_init_f()/board_init_r() pair following a patch similar to the
> > SPL/U-Boot pair, where SPL started out as a tiny helper piece of code
> > and ending up a resizeable (and, I dare to say, sizeable as well) kind
> > of U-boot. If we let too many features slip in board_init_f(), it'll
> > blur into a board_init_r() like and before we know it, it'll *require*
> > DDR, and write access to it too...
> >
> > So, board_init_() should *strictly* be limited to setting up a console
> > (for information purposes) and giving access to DDR while in the same
> > time never writing to it itself. Bonus points if it can limit itself to
> > *enabling* and postpone any *optimizing*(I am thinking of DDR settings
> > here and no, I don't have specific existing cases in mind; just sayin').
> >
> > In the present instance, I'd rather we either:
> >
> > - removed dependency on DT etc. by using "hard-coded" low level I2C
> >    reads for those boards that need it (I assume that for each of these
> >    boards the I2C slave, offset, and length to read are constant) in _f
> >    phase, or
> 
> But DT is used for initializing the i2c driver in tegra ...

Alright, out goes this proposal. Anyway, I didn't like it best. :)

> > - parsed the _f phase looking for offending functions or calls which
> >    write to .data or .bss and fix them, suffixing them with _f; in
> >    essence, that amounts to starting the implementation of my suggestion
> >    above alongside fixing the issue at hand.
> >
> > The first approach is rather "let's bring the thing back up first", so
> > it does not have my preference, but I would understand the need to
> > quickly fix things.
> 
> Yes.
> 
> > The second approach seems to be going in the same direction as Heiko's
> > proposal of 07:52 +0200, which I thus second provided it is applicable
> > to all the boards Wolfgang had in mind.
> 
> Lets do us this step as fixup ;-)

Alright too. :)

> bye,
> Heiko

Amicalement,
Heiko Schocher - July 31, 2013, 8:31 a.m.
Hello Albert,

Am 31.07.2013 10:16, schrieb Albert ARIBAUD:
> Hi Heiko,
>
> On Wed, 31 Jul 2013 09:36:19 +0200, Heiko Schocher<hs@denx.de>  wrote:
>
>>> On the other hand, it may be hard to immediately know what functions
>>> throughout U-boot are safe to call from within board_init_f(); maybe we
>>> should start thinking about checking and marking these, the simplest
>>> way being to suffix them with "_f" once we have made sure they are safe
>>> to call from within board_init_f().
>>
>> Hmmm... Maybe instead we should think (also in thinking common bring
>> up for all boards) about:
>>
>> getting rid of board_init_f in u-boot code, instead use for all
>> boards spl code to init needed things and copy and relocate u-boot
>> to ram in spl code ... so we have in u-boot no longer such
>> restictions ... but thats just an idea which whirs in my head ...
>> without thinking to deep in it.
>>
>> But this approach would have some advantages ...
>
> Well, the original SPL was basically board_init_f() plus some code to
> copy U-Boot from wherever it was to DDR, so it was tightly linked to
> board_init_f(). But... first, SPL has evolved into a "U-Boot lite"
> where much can happen beyond board_init_f() -- think Falcon mode, for
> instance -- and second, there are boards which do not have SPL at all,
> and their board_init_f() can thus not be "moved to SPL".

Hmm... all boards use board_init_f ... and spl do pieces from board_init_f
So why should it not be possible to do all init things in spl code?
Code beyond board_init_f is optional ...

And yes, there are a lot of boards, which have no spl, but they
can execute spl code (thinking of the lof of powerpc boards which
booting from nor flash ... spl code can also run from nor ... and
copy the u-boot piece of the image to ram, relocate it ...)

And yes, a side effect could be, that all boards can use Falcon boot mode.

;-)

> So no, I don't think we can move U-Boot's design from "_f/_r" to
> "SPL/U-Boot".

I am not sure ...

>>> But we should strictly limit the scope of board_init_f() or we'll find
>>> the board_init_f()/board_init_r() pair following a patch similar to the
>>> SPL/U-Boot pair, where SPL started out as a tiny helper piece of code
>>> and ending up a resizeable (and, I dare to say, sizeable as well) kind
>>> of U-boot. If we let too many features slip in board_init_f(), it'll
>>> blur into a board_init_r() like and before we know it, it'll *require*
>>> DDR, and write access to it too...
>>>
>>> So, board_init_() should *strictly* be limited to setting up a console
>>> (for information purposes) and giving access to DDR while in the same
>>> time never writing to it itself. Bonus points if it can limit itself to
>>> *enabling* and postpone any *optimizing*(I am thinking of DDR settings
>>> here and no, I don't have specific existing cases in mind; just sayin').
>>>
>>> In the present instance, I'd rather we either:
>>>
>>> - removed dependency on DT etc. by using "hard-coded" low level I2C
>>>     reads for those boards that need it (I assume that for each of these
>>>     boards the I2C slave, offset, and length to read are constant) in _f
>>>     phase, or
>>
>> But DT is used for initializing the i2c driver in tegra ...
>
> Alright, out goes this proposal. Anyway, I didn't like it best. :)

Yes.

>>> - parsed the _f phase looking for offending functions or calls which
>>>     write to .data or .bss and fix them, suffixing them with _f; in
>>>     essence, that amounts to starting the implementation of my suggestion
>>>     above alongside fixing the issue at hand.
>>>
>>> The first approach is rather "let's bring the thing back up first", so
>>> it does not have my preference, but I would understand the need to
>>> quickly fix things.
>>
>> Yes.
>>
>>> The second approach seems to be going in the same direction as Heiko's
>>> proposal of 07:52 +0200, which I thus second provided it is applicable
>>> to all the boards Wolfgang had in mind.
>>
>> Lets do us this step as fixup ;-)
>
> Alright too. :)

Ok.

@Stephen, Simon: Could anyone from you do this 2 steps?

bye,
Heiko
Albert ARIBAUD - July 31, 2013, 9:38 a.m.
Hi Heiko,

On Wed, 31 Jul 2013 10:31:12 +0200, Heiko Schocher <hs@denx.de> wrote:

> Hello Albert,
> 
> Am 31.07.2013 10:16, schrieb Albert ARIBAUD:
> > Hi Heiko,
> >
> > On Wed, 31 Jul 2013 09:36:19 +0200, Heiko Schocher<hs@denx.de>  wrote:
> >
> >>> On the other hand, it may be hard to immediately know what functions
> >>> throughout U-boot are safe to call from within board_init_f(); maybe we
> >>> should start thinking about checking and marking these, the simplest
> >>> way being to suffix them with "_f" once we have made sure they are safe
> >>> to call from within board_init_f().
> >>
> >> Hmmm... Maybe instead we should think (also in thinking common bring
> >> up for all boards) about:
> >>
> >> getting rid of board_init_f in u-boot code, instead use for all
> >> boards spl code to init needed things and copy and relocate u-boot
> >> to ram in spl code ... so we have in u-boot no longer such
> >> restictions ... but thats just an idea which whirs in my head ...
> >> without thinking to deep in it.
> >>
> >> But this approach would have some advantages ...
> >
> > Well, the original SPL was basically board_init_f() plus some code to
> > copy U-Boot from wherever it was to DDR, so it was tightly linked to
> > board_init_f(). But... first, SPL has evolved into a "U-Boot lite"
> > where much can happen beyond board_init_f() -- think Falcon mode, for
> > instance -- and second, there are boards which do not have SPL at all,
> > and their board_init_f() can thus not be "moved to SPL".
> 
> Hmm... all boards use board_init_f ... and spl do pieces from board_init_f
> So why should it not be possible to do all init things in spl code?
> Code beyond board_init_f is optional ...

It is, in the original "SPL is just board_init_f plus some copying"
view. In the current "SPL is U-boot only not full-featured", it becomes
false.

> And yes, there are a lot of boards, which have no spl, but they
> can execute spl code (thinking of the lof of powerpc boards which
> booting from nor flash ... spl code can also run from nor ... and
> copy the u-boot piece of the image to ram, relocate it ...)
> 
> And yes, a side effect could be, that all boards can use Falcon boot mode.
> 
> ;-)
> 
> > So no, I don't think we can move U-Boot's design from "_f/_r" to
> > "SPL/U-Boot".
> 
> I am not sure ...

I see this approach of likening SPL to _f and U-boot to _r as forcing a
dual-binary model onto all boards whereas not all boards require it. I
prefer a model where _f can exist, _r can exist, and for each target,
the maintainer decides which binaries are built and for each one,
whether _f and/or _r is present and what _r does.

Amicalement,
Simon Glass - July 31, 2013, 12:30 p.m.
Hi,

On Wed, Jul 31, 2013 at 3:38 AM, Albert ARIBAUD
<albert.u.boot@aribaud.net>wrote:

> Hi Heiko,
>
> On Wed, 31 Jul 2013 10:31:12 +0200, Heiko Schocher <hs@denx.de> wrote:
>
> > Hello Albert,
> >
> > Am 31.07.2013 10:16, schrieb Albert ARIBAUD:
> > > Hi Heiko,
> > >
> > > On Wed, 31 Jul 2013 09:36:19 +0200, Heiko Schocher<hs@denx.de>  wrote:
> > >
> > >>> On the other hand, it may be hard to immediately know what functions
> > >>> throughout U-boot are safe to call from within board_init_f(); maybe
> we
> > >>> should start thinking about checking and marking these, the simplest
> > >>> way being to suffix them with "_f" once we have made sure they are
> safe
> > >>> to call from within board_init_f().
> > >>
> > >> Hmmm... Maybe instead we should think (also in thinking common bring
> > >> up for all boards) about:
> > >>
> > >> getting rid of board_init_f in u-boot code, instead use for all
> > >> boards spl code to init needed things and copy and relocate u-boot
> > >> to ram in spl code ... so we have in u-boot no longer such
> > >> restictions ... but thats just an idea which whirs in my head ...
> > >> without thinking to deep in it.
> > >>
> > >> But this approach would have some advantages ...
> > >
> > > Well, the original SPL was basically board_init_f() plus some code to
> > > copy U-Boot from wherever it was to DDR, so it was tightly linked to
> > > board_init_f(). But... first, SPL has evolved into a "U-Boot lite"
> > > where much can happen beyond board_init_f() -- think Falcon mode, for
> > > instance -- and second, there are boards which do not have SPL at all,
> > > and their board_init_f() can thus not be "moved to SPL".
> >
> > Hmm... all boards use board_init_f ... and spl do pieces from
> board_init_f
> > So why should it not be possible to do all init things in spl code?
> > Code beyond board_init_f is optional ...
>
> It is, in the original "SPL is just board_init_f plus some copying"
> view. In the current "SPL is U-boot only not full-featured", it becomes
> false.
>
> > And yes, there are a lot of boards, which have no spl, but they
> > can execute spl code (thinking of the lof of powerpc boards which
> > booting from nor flash ... spl code can also run from nor ... and
> > copy the u-boot piece of the image to ram, relocate it ...)
> >
> > And yes, a side effect could be, that all boards can use Falcon boot
> mode.
> >
> > ;-)
> >
> > > So no, I don't think we can move U-Boot's design from "_f/_r" to
> > > "SPL/U-Boot".
> >
> > I am not sure ...
>
> I see this approach of likening SPL to _f and U-boot to _r as forcing a
> dual-binary model onto all boards whereas not all boards require it. I
> prefer a model where _f can exist, _r can exist, and for each target,
> the maintainer decides which binaries are built and for each one,
> whether _f and/or _r is present and what _r does.
>

I am not really any clearer as to what should be done here.

Previously on ARM i2c init generally happened in board_init_r(). This has
changed now, and so boards which need to do some init (e.g. reading and
storing DT information) to make i2c work are going to have problems if they
cannot access any memory (yes we could put it in global_data I suppose).

It sounds like the need for early i2c is rare, so we could perhaps create
an option like CONFIG_SYS_EARLY_I2C to enable this?

While I agree that minimising code in board_init_f() is a great idea, if we
have one case that needs it, then we need to deal with the problem.

Although did I mention that it does seem silly to me to solve what is an
entirely hypothetical problem on Tegra and (I think) any other modern ARM
SOC that uses SPL? After all, SDRAM is fully available on these SOCs and in
fact setting that up and getting U-Boot loaded into RAM is the purpose of
the SPL stage! To my mind, SPL has taken over this responsibility of
board_init_f(). Thoughts? Maybe a minor rethink of
SPL/board_init_f()/board_init_r() is in order?

Regards,
Simon
Heiko Schocher - July 31, 2013, 1:03 p.m.
Hello Simon,

Am 31.07.2013 14:30, schrieb Simon Glass:
> Hi,
>
> On Wed, Jul 31, 2013 at 3:38 AM, Albert ARIBAUD
> <albert.u.boot@aribaud.net>wrote:
>
>> Hi Heiko,
>>
>> On Wed, 31 Jul 2013 10:31:12 +0200, Heiko Schocher<hs@denx.de>  wrote:
>>
>>> Hello Albert,
>>>
>>> Am 31.07.2013 10:16, schrieb Albert ARIBAUD:
>>>> Hi Heiko,
>>>>
>>>> On Wed, 31 Jul 2013 09:36:19 +0200, Heiko Schocher<hs@denx.de>   wrote:
>>>>
>>>>>> On the other hand, it may be hard to immediately know what functions
>>>>>> throughout U-boot are safe to call from within board_init_f(); maybe
>> we
>>>>>> should start thinking about checking and marking these, the simplest
>>>>>> way being to suffix them with "_f" once we have made sure they are
>> safe
>>>>>> to call from within board_init_f().
>>>>>
>>>>> Hmmm... Maybe instead we should think (also in thinking common bring
>>>>> up for all boards) about:
>>>>>
>>>>> getting rid of board_init_f in u-boot code, instead use for all
>>>>> boards spl code to init needed things and copy and relocate u-boot
>>>>> to ram in spl code ... so we have in u-boot no longer such
>>>>> restictions ... but thats just an idea which whirs in my head ...
>>>>> without thinking to deep in it.
>>>>>
>>>>> But this approach would have some advantages ...
>>>>
>>>> Well, the original SPL was basically board_init_f() plus some code to
>>>> copy U-Boot from wherever it was to DDR, so it was tightly linked to
>>>> board_init_f(). But... first, SPL has evolved into a "U-Boot lite"
>>>> where much can happen beyond board_init_f() -- think Falcon mode, for
>>>> instance -- and second, there are boards which do not have SPL at all,
>>>> and their board_init_f() can thus not be "moved to SPL".
>>>
>>> Hmm... all boards use board_init_f ... and spl do pieces from
>> board_init_f
>>> So why should it not be possible to do all init things in spl code?
>>> Code beyond board_init_f is optional ...
>>
>> It is, in the original "SPL is just board_init_f plus some copying"
>> view. In the current "SPL is U-boot only not full-featured", it becomes
>> false.
>>
>>> And yes, there are a lot of boards, which have no spl, but they
>>> can execute spl code (thinking of the lof of powerpc boards which
>>> booting from nor flash ... spl code can also run from nor ... and
>>> copy the u-boot piece of the image to ram, relocate it ...)
>>>
>>> And yes, a side effect could be, that all boards can use Falcon boot
>> mode.
>>>
>>> ;-)
>>>
>>>> So no, I don't think we can move U-Boot's design from "_f/_r" to
>>>> "SPL/U-Boot".
>>>
>>> I am not sure ...
>>
>> I see this approach of likening SPL to _f and U-boot to _r as forcing a
>> dual-binary model onto all boards whereas not all boards require it. I
>> prefer a model where _f can exist, _r can exist, and for each target,
>> the maintainer decides which binaries are built and for each one,
>> whether _f and/or _r is present and what _r does.
>>
>
> I am not really any clearer as to what should be done here.
>
> Previously on ARM i2c init generally happened in board_init_r(). This has

really? The init_func_i2c() call is not introduced through the
i2c multibus approach ...

> changed now, and so boards which need to do some init (e.g. reading and
> storing DT information) to make i2c work are going to have problems if they
> cannot access any memory (yes we could put it in global_data I suppose).
>
> It sounds like the need for early i2c is rare, so we could perhaps create
> an option like CONFIG_SYS_EARLY_I2C to enable this?
>
> While I agree that minimising code in board_init_f() is a great idea, if we
> have one case that needs it, then we need to deal with the problem.

 From my side, yes. But this different to powerpc!

> Although did I mention that it does seem silly to me to solve what is an
> entirely hypothetical problem on Tegra and (I think) any other modern ARM
> SOC that uses SPL? After all, SDRAM is fully available on these SOCs and in
> fact setting that up and getting U-Boot loaded into RAM is the purpose of
> the SPL stage! To my mind, SPL has taken over this responsibility of
> board_init_f(). Thoughts? Maybe a minor rethink of
> SPL/board_init_f()/board_init_r() is in order?

Yes, but board_init_f is used in spl code, or? And it is not only
ram settings, maybe read baudrate setting in an i2c epprom ...

We can make init_func_i2c() weak, and in the first step a
dummy function and see, which boards really need it.

Nevertheless, why the tegra i2c driver do not call process_node()
from the i2c_init function? This must be fixed I think, as it
is not good to do some setup needed for i2c in board_init()
and other in i2c_init to have a working i2c adapter ...

bye,
Heiko
Stephen Warren - July 31, 2013, 7:31 p.m.
On 07/30/2013 11:46 PM, Heiko Schocher wrote:
...
> Hmm.. each i2c adapter has its own init function ... why the tegra
> driver do not use it? And do the necessary inits in it? So we
> initialize an adapater only if we use it, which is also a rule
> for u-boot ...
> 
> I have no hw, but it should be possible to add to process_nodes()
> a parameter "controller_number" and call
> process_nodes(controller_number, ...) from the i2c drivers init
> function ...

There are two steps to initializing I2C on a Tegra system:

1) Parsing the DT to determine which/how-many I2C adapters exist in the
SoC. This will yield information such as the register address of the I2C
adapters, which clock/reset signal they rely on, perhaps the max bus
clock rate, etc.

This is a single global operation that needs to happen one single time
before anything else.

This stage initializes the i2c_controllers[] array, since that's what
stores all the data parsed from DT.

2) Actually initializing or using the I2C HW. That could certainly be
part of the per-I2C-controller init() function you mention.


Now, I think the big disconnect here is that historically, the U-Boot
binary has hard-coded all the details that step (1) above parses out of
DT, so that step (1) did not need to exist.

However, Simon has been pushing Tegra towards not hard-coding any of
this information, but instead having a single binary that can work on
arbitrary Tegra boards and even across different Tegra SoCs which have a
different number of I2C controllers at different register addresses.
This is quite fundamentally different to how U-Boot has worked in the
past, and evidently has some problems fitting into the typical U-Boot
initialization sequence.
Wolfgang Denk - July 31, 2013, 7:39 p.m.
Dear Albert,

In message <20130731090644.2b4ce215@lilith> you wrote:
> 
> > On some boards, yes.  For example, if they store the environment in an
> > I2C attached EEPROM. Then you need I2C support early, before console,
> > to read for example the baudrate setting.
> 
> Thanks, so some I2C reads are needed. Next question: on these boards,
> do these I2C reads require DT reads? Maybe a few hard-coded low level
> I2C reads are enough. I guess DT writes are completely unneeded at
> that point. Also, why exactly do I2C and, as the case may be, DT, need
> to write to .data?

A quick check shows that it's actually more than 50 boards that store
the env in EEPROM (I'm surprised myself about this large number, given
the disadvantages of such an approach), but AFAICT none of these needs
to access the DT that early.

> More generally, while I think the board_init_f() part of U-Boot should
> be as short and compact as possible, I understand and admit that it
> might have to read from just about any (local) storage resource, be it
> environment or DT or any stored information it needs.

Agreed - actually we're entering an area hear that smells pretty
strong like device model reorganization ;-)

> So, board_init_() should *strictly* be limited to setting up a console
> (for information purposes) and giving access to DDR while in the same
> time never writing to it itself. Bonus points if it can limit itself to
> *enabling* and postpone any *optimizing*(I am thinking of DDR settings
> here and no, I don't have specific existing cases in mind; just sayin').

Agreed!


Best regards,

Wolfgang Denk
Stephen Warren - July 31, 2013, 7:41 p.m.
On 07/31/2013 02:31 AM, Heiko Schocher wrote:
> Am 31.07.2013 10:16, schrieb Albert ARIBAUD:
>> On Wed, 31 Jul 2013 09:36:19 +0200, Heiko Schocher<hs@denx.de>  wrote:
...
>>>> In the present instance, I'd rather we either:
>>>>
>>>> - removed dependency on DT etc. by using "hard-coded" low level I2C
>>>>     reads for those boards that need it (I assume that for each of
>>>> these
>>>>     boards the I2C slave, offset, and length to read are constant)
>>>> in _f
>>>>     phase, or
>>>
>>> But DT is used for initializing the i2c driver in tegra ...
>>
>> Alright, out goes this proposal. Anyway, I didn't like it best. :)
> 
> Yes.
> 
>>>> - parsed the _f phase looking for offending functions or calls which
>>>>     write to .data or .bss and fix them, suffixing them with _f; in
>>>>     essence, that amounts to starting the implementation of my
>>>> suggestion
>>>>     above alongside fixing the issue at hand.
>>>>
>>>> The first approach is rather "let's bring the thing back up first", so
>>>> it does not have my preference, but I would understand the need to
>>>> quickly fix things.
>>>
>>> Yes.
>>>
>>>> The second approach seems to be going in the same direction as Heiko's
>>>> proposal of 07:52 +0200, which I thus second provided it is applicable
>>>> to all the boards Wolfgang had in mind.
>>>
>>> Lets do us this step as fixup ;-)
>>
>> Alright too. :)
> 
> Ok.
> 
> @Stephen, Simon: Could anyone from you do this 2 steps?

Sorry, I really can't tell which 2 steps you mean? I don't see any email
in any timezone with minutes==:52...
Heiko Schocher - Aug. 1, 2013, 4:32 a.m.
Hello Stephen,

Am 31.07.2013 21:41, schrieb Stephen Warren:
> On 07/31/2013 02:31 AM, Heiko Schocher wrote:
>> Am 31.07.2013 10:16, schrieb Albert ARIBAUD:
>>> On Wed, 31 Jul 2013 09:36:19 +0200, Heiko Schocher<hs@denx.de>   wrote:
> ...
>>>>> In the present instance, I'd rather we either:
>>>>>
>>>>> - removed dependency on DT etc. by using "hard-coded" low level I2C
>>>>>      reads for those boards that need it (I assume that for each of
>>>>> these
>>>>>      boards the I2C slave, offset, and length to read are constant)
>>>>> in _f
>>>>>      phase, or
>>>>
>>>> But DT is used for initializing the i2c driver in tegra ...
>>>
>>> Alright, out goes this proposal. Anyway, I didn't like it best. :)
>>
>> Yes.
>>
>>>>> - parsed the _f phase looking for offending functions or calls which
>>>>>      write to .data or .bss and fix them, suffixing them with _f; in
>>>>>      essence, that amounts to starting the implementation of my
>>>>> suggestion
>>>>>      above alongside fixing the issue at hand.
>>>>>
>>>>> The first approach is rather "let's bring the thing back up first", so
>>>>> it does not have my preference, but I would understand the need to
>>>>> quickly fix things.
>>>>
>>>> Yes.
>>>>
>>>>> The second approach seems to be going in the same direction as Heiko's
>>>>> proposal of 07:52 +0200, which I thus second provided it is applicable
>>>>> to all the boards Wolfgang had in mind.
>>>>
>>>> Lets do us this step as fixup ;-)
>>>
>>> Alright too. :)
>>
>> Ok.
>>
>> @Stephen, Simon: Could anyone from you do this 2 steps?
>
> Sorry, I really can't tell which 2 steps you mean? I don't see any email
> in any timezone with minutes==:52...

http://lists.denx.de/pipermail/u-boot/2013-July/159853.html
(you were also on Cc ...)

bye,
Heiko
Stephen Warren - Aug. 5, 2013, 7:21 p.m.
On 07/30/2013 02:00 PM, Stephen Warren wrote:
...
(discussion of instantiating/initializing I2C devices from device tree,
and the fact U-Boot attempts to do that before .data or malloc can be
touched/used, which doesn't work)
...
> It seems like much of U-Boot's initialization architecture simply wasn't
> designed to accommodate dynamically initializing devices from DT.

I thought about this a little over the weekend. I think the solution
here may be to break up U-Boot initialization steps more explicitly,
even into more separate binaries.

There are at least the following separate things going on right now
(admittedly some of these are slightly loosely defined, and some are
currently lumped into the main image's _f _r steps/tables):

* Basic CPU initialization.

* Initializing SDRAM controller (not required on Tegra; boot ROM already
does this). This is what the SPL is typically used for.

* Tegra-specific: Run code on the AVP (boot) CPU which starts the main
CPU complex running. On Tegra, we've hijacked the SPL to do this
separate step.

* Copying U-Boot to SDRAM now it's available and/or performing
initialization steps that require SDRAM.

* Sizing SDRAM, perhaps by

* Relocating the U-Boot binary to top of RAM (at least if relocation is
enabled).

* The main U-Boot binary.

Some more steps I've probably forgotten while I write this email!

Now, obviously some of those steps run in a very restricted environment.

For example, on systems where the boot ROM doesn't exist or doesn't
enable SDRAM, then steps before U-Boot initializes it can't touch
.data/malloc etc.

It's probably not a good idea to make the code that initializes and
probes SDRAM have to retrieve information about the SPD I2C controller
(if one exists) from DT; it'd probably be better to hard-code the I2C
controller information into that part of U-Boot, and defer any complex
dynamic stuff to later. This is at least partially fallout from the
previous point.

The order of some of those bullet points above might even vary a little
depending on the SoC.

For example, on Tegra, we have to do basic CPU initialization on two
different CPUs; once on the AVP when we first start running any code at
all, and separately on the main CPU complex after it has been booted.
Those two CPUs are different ARM architectures too. And on Tegra we
don't have to initialize SDRAM at all; the boot ROM does it.

So, I wonder if it wouldn't be worth splitting U-Boot up into more
separate binaries for these steps, so that any restrictions on the steps
can be much better defined and segregated. Then, the final U-Boot binary
can be constructed not just by:

cat spl.bin u-boot.bin

But perhaps more like:

cat sdram-init.bin relocater.bin u-boot.bin

or something like that?

Everything in sdram-init.bin would use no .data (the linker script could
enforce this) and hard-coded devices. Everything in u-boot.bin could
immediately assume that SDRAM and .data was available, and use DT for
everything, and not need any pointer relocation fixup phase, since there
would be nothing in .data to fix up.

Each board or SoC could define a list of the steps they need, their
order, and the restrictions on their execution (e.g. no .data, ROMable
code, hard-coded vs. dynamic devices possibly from DT, etc.)

Another thing that made me think of this: I briefly experimented with
getting Tegra's U-Boot SPL to jump directly to a Linux zImage. A few
things were missing, since Tegra's SPL runs on the AVP CPU not the main
CPU, and hence never initializes anything on the main CPU, leaving that
responsibility to the main U-Boot binary). Hence, some main-CPU cache
setup was missing from Tegra's SPL. Solving this issue requires 3
separate binaries:

1) AVP boot code, to start the main CPU complex
2) Main CPU low-level initialization
3) Main U-Boot binary

To boot U-Boot, all 3 steps would be used.

To directly boot a zImage, only steps (1) and (2) would be needed.

Right now, we need to hack up a separate binary for (2) for the
boot-a-zImage-directly case, and hence concatenate SPL, CPU init,
zImage. It'd be nice if the "CPU init" part of that set of binaries was
something we could easily pull out of the U-Boot build process, rather
than something custom.
Tom Rini - Aug. 5, 2013, 8:08 p.m.
On Mon, Aug 05, 2013 at 01:21:50PM -0600, Stephen Warren wrote:
> On 07/30/2013 02:00 PM, Stephen Warren wrote:
> ...
> (discussion of instantiating/initializing I2C devices from device tree,
> and the fact U-Boot attempts to do that before .data or malloc can be
> touched/used, which doesn't work)
> ...
> > It seems like much of U-Boot's initialization architecture simply wasn't
> > designed to accommodate dynamically initializing devices from DT.
> 
> I thought about this a little over the weekend. I think the solution
> here may be to break up U-Boot initialization steps more explicitly,
> even into more separate binaries.

Have you seen the 'TPL' code Freescale has been posting?  That might be
a handy concept, but I'm concerned we're going to be heading towards N
separate little programs, and that's possibly a big complex (and thus
fragile) web.

[snip]
> Another thing that made me think of this: I briefly experimented with
> getting Tegra's U-Boot SPL to jump directly to a Linux zImage. A few
> things were missing, since Tegra's SPL runs on the AVP CPU not the main
> CPU, and hence never initializes anything on the main CPU, leaving that
> responsibility to the main U-Boot binary). Hence, some main-CPU cache
> setup was missing from Tegra's SPL. Solving this issue requires 3
> separate binaries:
> 
> 1) AVP boot code, to start the main CPU complex
> 2) Main CPU low-level initialization
> 3) Main U-Boot binary
> 
> To boot U-Boot, all 3 steps would be used.
> 
> To directly boot a zImage, only steps (1) and (2) would be needed.
> 
> Right now, we need to hack up a separate binary for (2) for the
> boot-a-zImage-directly case, and hence concatenate SPL, CPU init,
> zImage. It'd be nice if the "CPU init" part of that set of binaries was
> something we could easily pull out of the U-Boot build process, rather
> than something custom.

So I guess you're trying to do something that's not quite falcon mode
here?
Stephen Warren - Aug. 5, 2013, 9:06 p.m.
On 08/05/2013 02:08 PM, Tom Rini wrote:
> On Mon, Aug 05, 2013 at 01:21:50PM -0600, Stephen Warren wrote:
>> On 07/30/2013 02:00 PM, Stephen Warren wrote: ... (discussion of
>> instantiating/initializing I2C devices from device tree, and the
>> fact U-Boot attempts to do that before .data or malloc can be 
>> touched/used, which doesn't work) ...
>>> It seems like much of U-Boot's initialization architecture
>>> simply wasn't designed to accommodate dynamically initializing
>>> devices from DT.
>> 
>> I thought about this a little over the weekend. I think the
>> solution here may be to break up U-Boot initialization steps more
>> explicitly, even into more separate binaries.
> 
> Have you seen the 'TPL' code Freescale has been posting?  That
> might be a handy concept, but I'm concerned we're going to be
> heading towards N separate little programs, and that's possibly a
> big complex (and thus fragile) web.

No, I'll go try and find it and take a look.

> [snip]
>> Another thing that made me think of this: I briefly experimented
>> with getting Tegra's U-Boot SPL to jump directly to a Linux
>> zImage. A few things were missing, since Tegra's SPL runs on the
>> AVP CPU not the main CPU, and hence never initializes anything on
>> the main CPU, leaving that responsibility to the main U-Boot
>> binary). Hence, some main-CPU cache setup was missing from
>> Tegra's SPL. Solving this issue requires 3 separate binaries:
>> 
>> 1) AVP boot code, to start the main CPU complex 2) Main CPU
>> low-level initialization 3) Main U-Boot binary
>> 
>> To boot U-Boot, all 3 steps would be used.
>> 
>> To directly boot a zImage, only steps (1) and (2) would be
>> needed.
>> 
>> Right now, we need to hack up a separate binary for (2) for the 
>> boot-a-zImage-directly case, and hence concatenate SPL, CPU
>> init, zImage. It'd be nice if the "CPU init" part of that set of
>> binaries was something we could easily pull out of the U-Boot
>> build process, rather than something custom.
> 
> So I guess you're trying to do something that's not quite falcon
> mode here?

I thought it was very similar to falcon mode, but reading
doc/README.falcon, it's actually pretty different.

My use-case is that as a kernel developer, I want to repeatedly boot
new kernels. I can do the following now:

* Copy new kernel to SD card, put it in the target system, reboot it
* Copy new kernel to TFTP server, reboot target

Either of these are slower than I'd like either due to swapping SD
cards about, or slow TFTP transfers. Tegra at least has another boot
option: When the system boots, it can immediately go into special mode
whereby a USB-hosted protocol can be used download arbitrary code to
SDRAM and execute it. I want to make that downloaded code be the
zImage, perhaps with some stuff concatenated on the front to do a
little basic initialization first, e.g. setting up caches, setting r2
to point at a DTB, etc.

Now, I was hoping to re-use some code from U-Boot as the stuff I
concatenate with the zImage, so I wouldn't have to maintain it
separately. The bits I need area almost just the SPL, although since
our SPL and main U-Boot run on different CPUs, I also need a bit extra
CPU initialization, so for now I hacked up a very simple stub to do this.

Finally, I was then hoping to be able to burn that concatenated image
into flash and have the system always boot it. This is the part that's
most similar to falcon mode, I think.

My use case here is for that flashed kernel/initrd be able to choose a
boot target (SD, TFTP, ...), get a kernel from there, and kexec it.
That would make the kernel+initrd replace the main U-Boot binary. Then
I can write bash/... scripts and use kernel features for my boot menu,
rather than U-Boot scripts.
Stephen Warren - Aug. 5, 2013, 11:18 p.m.
On 08/05/2013 02:08 PM, Tom Rini wrote:
> On Mon, Aug 05, 2013 at 01:21:50PM -0600, Stephen Warren wrote:
>> On 07/30/2013 02:00 PM, Stephen Warren wrote: ... (discussion of
>> instantiating/initializing I2C devices from device tree, and the
>> fact U-Boot attempts to do that before .data or malloc can be 
>> touched/used, which doesn't work) ...
>>> It seems like much of U-Boot's initialization architecture
>>> simply wasn't designed to accommodate dynamically initializing
>>> devices from DT.
>> 
>> I thought about this a little over the weekend. I think the
>> solution here may be to break up U-Boot initialization steps more
>> explicitly, even into more separate binaries.
> 
> Have you seen the 'TPL' code Freescale has been posting?  That
> might be a handy concept, but I'm concerned we're going to be
> heading towards N separate little programs, and that's possibly a
> big complex (and thus fragile) web.

TPL is certainly similar, but the implementation is pretty different.

On Tegra, the boot ROM initializes SDRAM, so there aren't any max size
requirements on SPL/U-Boot; they're concatenated together in flash and
both placed into SDRAM by the boot ROM in all cases. SPL+U-Boot is
just one big binary as far as our boot ROM is concerned, but just
happens to be made out of a few chunks that are concatenated together
as far as the U-Boot build process is concerned.

So, my proposal to further split up the U-Boot binary was more to allow:

a) A more obvious boundary for various restrictions, such as lack of
.data access, to applied or lifted.

b) Re-using some of the component parts of U-Boot to build other things.

Freescale's TPL patches are all about limitations on the size of the
various components. Hence, each of SPL, TPL is a separate entity in
flash too, and each contains a flash driver to read the next component
in the chain.

I suppose the two concepts could be unified by simply having SPL/TPL
on Tegra not contain any flash drivers, and both use the "jump to
hard-coded address" method of dispatching to the next binary just as
we do today for the SPL->u-boot.bin handoff. However, I'd still rather
explicitly call out what each component binary is for with a
per-board/soc list, rather than re-using the names SPL/TPL to do
different things on different systems.

Not that I likely have any time to actually implement any of this:-(

Patch

diff --git a/README b/README
index e322f51..513d84f 100644
--- a/README
+++ b/README
@@ -1920,6 +1920,11 @@  CBFS (Coreboot Filesystem) support
 		    CONFIG_SYS_FSL_I2C2_SLAVE for the slave address of the
 		    second bus.
 
+		- drivers/i2c/tegra_i2c.c:
+		 - activate this driver with CONFIG_SYS_I2C_TEGRA
+		 - This driver adds 4 i2c buses with a fix speed from
+		   100000 and the slave addr 0!
+
 		additional defines:
 
 		CONFIG_SYS_NUM_I2C_BUSES
diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile
index 2162173..564d061 100644
--- a/drivers/i2c/Makefile
+++ b/drivers/i2c/Makefile
@@ -39,7 +39,6 @@  COBJS-$(CONFIG_PCA9564_I2C) += pca9564_i2c.o
 COBJS-$(CONFIG_PPC4XX_I2C) += ppc4xx_i2c.o
 COBJS-$(CONFIG_DRIVER_S3C24X0_I2C) += s3c24x0_i2c.o
 COBJS-$(CONFIG_S3C44B0_I2C) += s3c44b0_i2c.o
-COBJS-$(CONFIG_TEGRA_I2C) += tegra_i2c.o
 COBJS-$(CONFIG_TSI108_I2C) += tsi108_i2c.o
 COBJS-$(CONFIG_U8500_I2C) += u8500_i2c.o
 COBJS-$(CONFIG_SH_I2C) += sh_i2c.o
@@ -47,6 +46,7 @@  COBJS-$(CONFIG_SH_SH7734_I2C) += sh_sh7734_i2c.o
 COBJS-$(CONFIG_SYS_I2C) += i2c_core.o
 COBJS-$(CONFIG_SYS_I2C_FSL) += fsl_i2c.o
 COBJS-$(CONFIG_SYS_I2C_SOFT) += soft_i2c.o
+COBJS-$(CONFIG_SYS_I2C_TEGRA) += tegra_i2c.o
 
 COBJS	:= $(COBJS-y)
 SRCS	:= $(COBJS:.o=.c)
diff --git a/drivers/i2c/tegra_i2c.c b/drivers/i2c/tegra_i2c.c
index 8fa1cda..9bf71a6 100644
--- a/drivers/i2c/tegra_i2c.c
+++ b/drivers/i2c/tegra_i2c.c
@@ -35,8 +35,6 @@ 
 
 DECLARE_GLOBAL_DATA_PTR;
 
-static unsigned int i2c_bus_num;
-
 /* Information about i2c controller */
 struct i2c_bus {
 	int			id;
@@ -332,38 +330,25 @@  static int tegra_i2c_read_data(struct i2c_bus *bus, u32 addr, u8 *data,
  * @param bus_num	Bus number to check / return
  * @return pointer to bus, if valid, else NULL
  */
-static struct i2c_bus *tegra_i2c_get_bus(unsigned int bus_num)
+static struct i2c_bus *tegra_i2c_get_bus(struct i2c_adapter *adap)
 {
 	struct i2c_bus *bus;
 
-	if (bus_num >= TEGRA_I2C_NUM_CONTROLLERS) {
-		debug("%s: Invalid bus number %u\n", __func__, bus_num);
-		return NULL;
-	}
-	bus = &i2c_controllers[bus_num];
+	bus = &i2c_controllers[adap->hwadapnr];
 	if (!bus->inited) {
-		debug("%s: Bus %u not available\n", __func__, bus_num);
+		debug("%s: Bus %u not available\n", __func__, adap->hwadapnr);
 		return NULL;
 	}
 
 	return bus;
 }
 
-unsigned int i2c_get_bus_speed(void)
-{
-	struct i2c_bus *bus;
-
-	bus = tegra_i2c_get_bus(i2c_bus_num);
-	if (!bus)
-		return 0;
-	return bus->speed;
-}
-
-int i2c_set_bus_speed(unsigned int speed)
+static unsigned int tegra_i2c_set_bus_speed(struct i2c_adapter *adap,
+			unsigned int speed)
 {
 	struct i2c_bus *bus;
 
-	bus = tegra_i2c_get_bus(i2c_bus_num);
+	bus = tegra_i2c_get_bus(adap);
 	if (!bus)
 		return 0;
 	bus->speed = speed;
@@ -482,7 +467,7 @@  void i2c_init_board(void)
 		return;
 }
 
-void i2c_init(int speed, int slaveaddr)
+static void tegra_i2c_init(struct i2c_adapter *adap, int speed, int slaveaddr)
 {
 	/* This will override the speed selected in the fdt for that port */
 	debug("i2c_init(speed=%u, slaveaddr=0x%x)\n", speed, slaveaddr);
@@ -532,14 +517,14 @@  int i2c_read_data(struct i2c_bus *bus, uchar chip, uchar *buffer, int len)
 }
 
 /* Probe to see if a chip is present. */
-int i2c_probe(uchar chip)
+static int tegra_i2c_probe(struct i2c_adapter *adap, uchar chip)
 {
 	struct i2c_bus *bus;
 	int rc;
 	uchar reg;
 
 	debug("i2c_probe: addr=0x%x\n", chip);
-	bus = tegra_i2c_get_bus(i2c_get_bus_num());
+	bus = tegra_i2c_get_bus(adap);
 	if (!bus)
 		return 1;
 	reg = 0;
@@ -558,7 +543,8 @@  static int i2c_addr_ok(const uint addr, const int alen)
 }
 
 /* Read bytes */
-int i2c_read(uchar chip, uint addr, int alen, uchar *buffer, int len)
+static int tegra_i2c_read(struct i2c_adapter *adap, uchar chip, uint addr,
+			int alen, uchar *buffer, int len)
 {
 	struct i2c_bus *bus;
 	uint offset;
@@ -566,7 +552,7 @@  int i2c_read(uchar chip, uint addr, int alen, uchar *buffer, int len)
 
 	debug("i2c_read: chip=0x%x, addr=0x%x, len=0x%x\n",
 				chip, addr, len);
-	bus = tegra_i2c_get_bus(i2c_bus_num);
+	bus = tegra_i2c_get_bus(adap);
 	if (!bus)
 		return 1;
 	if (!i2c_addr_ok(addr, alen)) {
@@ -596,7 +582,8 @@  int i2c_read(uchar chip, uint addr, int alen, uchar *buffer, int len)
 }
 
 /* Write bytes */
-int i2c_write(uchar chip, uint addr, int alen, uchar *buffer, int len)
+static int tegra_i2c_write(struct i2c_adapter *adap, uchar chip, uint addr,
+			int alen, uchar *buffer, int len)
 {
 	struct i2c_bus *bus;
 	uint offset;
@@ -604,7 +591,7 @@  int i2c_write(uchar chip, uint addr, int alen, uchar *buffer, int len)
 
 	debug("i2c_write: chip=0x%x, addr=0x%x, len=0x%x\n",
 				chip, addr, len);
-	bus = tegra_i2c_get_bus(i2c_bus_num);
+	bus = tegra_i2c_get_bus(adap);
 	if (!bus)
 		return 1;
 	if (!i2c_addr_ok(addr, alen)) {
@@ -625,30 +612,11 @@  int i2c_write(uchar chip, uint addr, int alen, uchar *buffer, int len)
 	return 0;
 }
 
-#if defined(CONFIG_I2C_MULTI_BUS)
-/*
- * Functions for multiple I2C bus handling
- */
-unsigned int i2c_get_bus_num(void)
-{
-	return i2c_bus_num;
-}
-
-int i2c_set_bus_num(unsigned int bus)
-{
-	if (bus >= TEGRA_I2C_NUM_CONTROLLERS || !i2c_controllers[bus].inited)
-		return -1;
-	i2c_bus_num = bus;
-
-	return 0;
-}
-#endif
-
 int tegra_i2c_get_dvc_bus_num(void)
 {
 	int i;
 
-	for (i = 0; i < CONFIG_SYS_MAX_I2C_BUS; i++) {
+	for (i = 0; i < TEGRA_I2C_NUM_CONTROLLERS; i++) {
 		struct i2c_bus *bus = &i2c_controllers[i];
 
 		if (bus->inited && bus->is_dvc)
@@ -657,3 +625,19 @@  int tegra_i2c_get_dvc_bus_num(void)
 
 	return -1;
 }
+
+/*
+ * Register soft i2c adapters
+ */
+U_BOOT_I2C_ADAP_COMPLETE(tegra0, tegra_i2c_init, tegra_i2c_probe,
+			 tegra_i2c_read, tegra_i2c_write,
+			 tegra_i2c_set_bus_speed, 100000, 0, 0)
+U_BOOT_I2C_ADAP_COMPLETE(tegra1, tegra_i2c_init, tegra_i2c_probe,
+			 tegra_i2c_read, tegra_i2c_write,
+			 tegra_i2c_set_bus_speed, 100000, 0, 1)
+U_BOOT_I2C_ADAP_COMPLETE(tegra2, tegra_i2c_init, tegra_i2c_probe,
+			 tegra_i2c_read, tegra_i2c_write,
+			 tegra_i2c_set_bus_speed, 100000, 0, 2)
+U_BOOT_I2C_ADAP_COMPLETE(tegra3, tegra_i2c_init, tegra_i2c_probe,
+			 tegra_i2c_read, tegra_i2c_write,
+			 tegra_i2c_set_bus_speed, 100000, 0, 3)
diff --git a/include/configs/beaver.h b/include/configs/beaver.h
index 058da4f..dce08d5 100644
--- a/include/configs/beaver.h
+++ b/include/configs/beaver.h
@@ -41,12 +41,11 @@ 
 #define CONFIG_BOARD_EARLY_INIT_F
 
 /* I2C */
-#define CONFIG_TEGRA_I2C
+#define CONFIG_SYS_I2C_TEGRA
 #define CONFIG_SYS_I2C_INIT_BOARD
-#define CONFIG_I2C_MULTI_BUS
-#define CONFIG_SYS_MAX_I2C_BUS		TEGRA_I2C_NUM_CONTROLLERS
 #define CONFIG_SYS_I2C_SPEED		100000
 #define CONFIG_CMD_I2C
+#define CONFIG_SYS_I2C
 
 /* SD/MMC */
 #define CONFIG_MMC
diff --git a/include/configs/cardhu.h b/include/configs/cardhu.h
index 6a99175..4c67446 100644
--- a/include/configs/cardhu.h
+++ b/include/configs/cardhu.h
@@ -40,12 +40,13 @@ 
 #define CONFIG_BOARD_EARLY_INIT_F
 
 /* I2C */
-#define CONFIG_TEGRA_I2C
+#define CONFIG_SYS_I2C_TEGRA
 #define CONFIG_SYS_I2C_INIT_BOARD
 #define CONFIG_I2C_MULTI_BUS
 #define CONFIG_SYS_MAX_I2C_BUS		TEGRA_I2C_NUM_CONTROLLERS
 #define CONFIG_SYS_I2C_SPEED		100000
 #define CONFIG_CMD_I2C
+#define CONFIG_SYS_I2C
 
 /* SD/MMC */
 #define CONFIG_MMC
diff --git a/include/configs/dalmore.h b/include/configs/dalmore.h
index 7b68f7c..f76f93f 100644
--- a/include/configs/dalmore.h
+++ b/include/configs/dalmore.h
@@ -43,12 +43,13 @@ 
 #define CONFIG_BOARD_EARLY_INIT_F
 
 /* I2C */
-#define CONFIG_TEGRA_I2C
+#define CONFIG_SYS_I2C_TEGRA
 #define CONFIG_SYS_I2C_INIT_BOARD
 #define CONFIG_I2C_MULTI_BUS
 #define CONFIG_SYS_MAX_I2C_BUS		TEGRA_I2C_NUM_CONTROLLERS
 #define CONFIG_SYS_I2C_SPEED		100000
 #define CONFIG_CMD_I2C
+#define CONFIG_SYS_I2C
 
 /* SD/MMC */
 #define CONFIG_MMC
diff --git a/include/configs/seaboard.h b/include/configs/seaboard.h
index f66173e..58bf60a 100644
--- a/include/configs/seaboard.h
+++ b/include/configs/seaboard.h
@@ -57,12 +57,11 @@ 
 #define CONFIG_BOARD_LATE_INIT		/* Make sure LCD init is complete */
 
 /* I2C */
-#define CONFIG_TEGRA_I2C
+#define CONFIG_SYS_I2C_TEGRA
 #define CONFIG_SYS_I2C_INIT_BOARD
-#define CONFIG_I2C_MULTI_BUS
-#define CONFIG_SYS_MAX_I2C_BUS		4
 #define CONFIG_SYS_I2C_SPEED		100000
 #define CONFIG_CMD_I2C
+#define CONFIG_SYS_I2C
 
 /* SD/MMC */
 #define CONFIG_MMC
diff --git a/include/configs/trimslice.h b/include/configs/trimslice.h
index b925314..209c60e 100644
--- a/include/configs/trimslice.h
+++ b/include/configs/trimslice.h
@@ -54,12 +54,11 @@ 
 #define CONFIG_CMD_SF
 
 /* I2C */
-#define CONFIG_TEGRA_I2C
+#define CONFIG_SYS_I2C_TEGRA
 #define CONFIG_SYS_I2C_INIT_BOARD
-#define CONFIG_I2C_MULTI_BUS
-#define CONFIG_SYS_MAX_I2C_BUS		4
 #define CONFIG_SYS_I2C_SPEED		100000
 #define CONFIG_CMD_I2C
+#define CONFIG_SYS_I2C
 
 /* SD/MMC */
 #define CONFIG_MMC
diff --git a/include/configs/whistler.h b/include/configs/whistler.h
index 9542c7e..11a7904 100644
--- a/include/configs/whistler.h
+++ b/include/configs/whistler.h
@@ -46,12 +46,11 @@ 
 #define CONFIG_BOARD_EARLY_INIT_F
 
 /* I2C */
-#define CONFIG_TEGRA_I2C
+#define CONFIG_SYS_I2C_TEGRA
 #define CONFIG_SYS_I2C_INIT_BOARD
-#define CONFIG_I2C_MULTI_BUS
-#define CONFIG_SYS_MAX_I2C_BUS		4
 #define CONFIG_SYS_I2C_SPEED		100000
 #define CONFIG_CMD_I2C
+#define CONFIG_SYS_I2C
 
 /* SD/MMC */
 #define CONFIG_MMC