Hello Haavard, Wolfgang

Hmm...maybe this is a stupid question, but why can't it _always_ use the RAM area to look up the environment? env_init() is one of the first things to be called during bootstrap... Maybe it's wishful thinking, but if we could get rid of this extra logic, u-boot would become simpler, smaller and faster, right?

There are no stupid questions, just stupid answers... ;-)

I think it is nicer too, but that appears to me as a good thing to investigate for the next release of U-boot, not changing this on the deadline of the current release.

I think the patch I posted yesterday should be integrated in the current release, because it is limited to fixing a very annoying bug (bogus environment settings on at least the Atmel at91 boards using dataflash as environment settings storage, maybe also other boards) In fact, If it was up to me, I would classify such a bug as a showstopper...

Wolfgang, have you looked at this patch already?

Kind Regards,

Remy

In message 20080506172309.1dff34bc@hskinnemo-gx620.norway.atmel.com you wrote:

Hmm...maybe this is a stupid question, but why can't it _always_ use the RAM area to look up the environment? env_init() is one of the first things to be called during bootstrap...

One of the first accesses to the environment happens when we need to read the console baudrate to initialize the serial interface. This happens very, very early in the initialization, long before RAM is even usable.

Maybe it's wishful thinking, but if we could get rid of this extra logic, u-boot would become simpler, smaller and faster, right?

Unfortunately this is not the case.

Best regards,

Wolfgang Denk

In message 3efb10970805060705l112c623at96bf0521eed8a211@mail.gmail.com you wrote:

...

Commit c0559be371b2a64b1a817088c3308688e2182f93 introduces a bug in the environment setting storage in U-boot-1.3.3-rc3. Settings are retrieved from dataflash when only settings in RAM are valid, resulting in corrupt environment settings, failing printenv command, and duplicate variables.

This patch fixes this by always using the RAM area when it is created and initialized. (Matches more the behavior as it was prior to this particular commit.)

Sorry, but this patch makes littles sense to me.

...

See attached (Sorry, my mailer does not handle inline-patches properly)

Chose another one? Or rather use "git-send-email" directly?

• /* if relocated to RAM */
• if (gd->flags & GD_FLG_RELOC)

• /* if relocated to RAM, OR if the environment in Malloc-ed RAM is valid */
• if ((gd->flags & GD_FLG_RELOC) || (gd->env_valid))

Let's keep in mind that the normal logic of the U-Boot startup sequence is like this:

• U-Boot boots and initializes the RAM
• U-Boot relocates itself into RAM, sets GD_FLG_RELOC and continues

running from RAM

• U-Boot continues with the initialization, for xample by setting up

the malloc arena, loading the working copy of the environment into RAM (after which it set's gd->env_valid), etc.

When you have a dataflash, this is not the normal behaviour.

A typical behaviour of an AT91 would be:

1) BootROM copies initial bootloader (at91bootstrap/dataflashboot) to internal SRAM 2) BootROM jumps to start of initial bootloader 3) Initial bootloader does low-level init (including configuring the SDRAM controller) 4) Initial bootloader copies U-Boot from dataflash to SDRAM 5) Initial bootloader jumps to U-Boot 6) U-Boot needs to skip low-level init

So the relocation to RAM always preceeds any use of malloc() and the setting of gd->env_valid. Or, put the other way round, we always set GD_FLG_RELOC long before gd->env_valid get's set.

Thus your change above is just redundant.

Now, if your board does not perform proper relocation for some reason, it should still set GD_FLG_RELOC at the appropriate place, i. e. as soon as U-Boot is ready for and starts running out of RAM.

Best regards,

Wolfgang Denk

Best Regards Ulf Samuelsson

Hello Wolfgang,

2008/5/9 Wolfgang Denk wd@denx.de:

In message 3efb10970805060705l112c623at96bf0521eed8a211@mail.gmail.com you wrote:

...

Commit c0559be371b2a64b1a817088c3308688e2182f93 introduces a bug in the environment setting storage in U-boot-1.3.3-rc3. Settings are retrieved from dataflash when only settings in RAM are valid, resulting in corrupt environment settings, failing printenv command, and duplicate variables. This patch fixes this by always using the RAM area when it is created and initialized. (Matches more the behavior as it was prior to this particular commit.)

Sorry, but this patch makes littles sense to me.

If you do not like the way it is fixed, then remove git-commit c0559be371b2a64b1a817088c3308688e2182f93 which causes this regression for the time being, to buy more time to fix it in a different/better way for the next release. Now with this commit the complete at91-board series (boards that boot from dataflash or nandflash) are broken. So,we have a serious regression here.

BTW: If you look at the code _before_ commit c0559be371b2a64b1a817088c3308688e2182f93 you will see that immediately when the gd->env_valid is set to 1, _always_ the memory routines are used to read the environment settings. It has been that for many years. Commit c0559be371b2a64b1a817088c3308688e2182f93 changes this behavior, although it suggests that it only would change the use of early global variables. Notice that this commit is more like a cosmetic change, so it would do no harm to leave it out until the next release. With my patch I tried to get this old behavior back in the tree, and therefor it should work properly for other boards as well. Even if it sounds redundant.

...

See attached (Sorry, my mailer does not handle inline-patches properly)

Chose another one? Or rather use "git-send-email" directly?

Stuck to company rules, and from behind a http-proxy, which blocks this kind of traffic? No way is that going to work...

Let's keep in mind that the normal logic of the U-Boot startup sequence is like this:

• U-Boot boots and initializes the RAM

Nope, not on AT91 with dataflash (or nandflash) boot... AT91Bootstrap is used to initialize the RAM, copies the U-boot code from serial dataflash (SPI bus) to RAM, and then starts executing U-Boot.

• U-Boot relocates itself into RAM, sets GD_FLG_RELOC and continues

running from RAM

U-boot will not _relocate_ itself in RAM, because it is already there. CONFIG_SKIP_RELOCATE_UBOOT is set for these boards, so GD_FLG_RELOC is never set.

• U-Boot continues with the initialization, for xample by setting up

the malloc arena, loading the working copy of the environment into RAM (after which it set's gd->env_valid), etc. So the relocation to RAM always preceeds any use of malloc() and the setting of gd->env_valid. Or, put the other way round, we always set GD_FLG_RELOC long before gd->env_valid get's set. Thus your change above is just redundant.

Not on AT91, because GD_FLG_RELOC is never set on AT91.

Now, if your board does not perform proper relocation for some reason, it should still set GD_FLG_RELOC at the appropriate place, i. e. as soon as U-Boot is ready for and starts running out of RAM.

Figuring this out would take some time, and if you want to freeze 1.3.3 in the mean time, I would suggest to leave this commit out until there is a proper fix for this regression. I can start looking at it again next week, maybe Stelian (who created the AT91 board support in the first place) is faster than me...

Kind Regards,

Remy

On Mon, 2008-05-12 at 00:43 +0200, Wolfgang Denk wrote:

In message 3efb10970805100653xc8c3848jf46c840cf33f95e7@mail.gmail.com you wrote:

...

If you do not like the way it is fixed, then remove git-commit c0559be371b2a64b1a817088c3308688e2182f93 which causes this regression for the time being, to buy more time to fix it in a different/better way for the next release. Now with this commit the complete at91-board series (boards that boot from dataflash or nandflash) are broken. So,we have a serious regression here.

That's what I did now: I reverted commit c0559be3.

Joakim, please re-submit after the problems were understood and fixed.

You want me to resubmit something I got no control over? That won't happen and I doubt the problem boards will care either so I guess this improvement is lost.

Jocke