Re: [U-Boot] [REGRESSION] commit b502611b51... "Change env_get_char from a..." breaks imx31_phycore
On Fri, 5 Sep 2008, Joakim Tjernlund wrote:
-----Original Message----- From: Guennadi Liakhovetski [mailto:lg@denx.de] Sent: den 5 september 2008 20:01 To: U-Boot@lists.denx.de Cc: Joakim Tjernlund Subject: [REGRESSION] commit b502611b51... "Change env_get_char from a..." breaks imx31_phycore
Hi,
The aforementioned commit
commit b502611b51f02718c2d1117d4981dabceb5af6de Author: Joakim Tjernlund joakim.tjernlund@transmode.se Date: Sun Jul 6 12:30:09 2008 +0200
Change env_get_char from a global function ptr to a function This avoids an early global data reference. Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>found by bisection and causes at least the imx31_phycore board to break. The boot process becomes slow, printenv is very slow too, booting does not always come to the bootdelay countdown, tftp wtops working too. Reverting this commit from the current HEAD fixes the problem.
Your board probably don't flip the GD_FLG_RELOC flag after relocation. A few ARM boards had a problem with this too.
Ok, this sounds good, but a grep over the current tree (as of commit 3e3c026ed746a284c6f0ef139b26d859939de7e9) reveals only one ARM board that does this: davinci. It is also set globally if you define CONFIG_SKIP_RELOCATE_UBOOT, which also is done by a couple of boards. From the README:
- CONFIG_SKIP_LOWLEVEL_INIT - CONFIG_SKIP_RELOCATE_UBOOT
[ARM only] If these variables are defined, then certain low level initializations (like setting up the memory controller) are omitted and/or U-Boot does not relocate itself into RAM. Normally these variables MUST NOT be defined. The only exception is when U-Boot is loaded (to RAM) by some other boot loader or by a debugger which performs these initializations itself.
So, this doesn't look like the proper way to force setting of GD_FLG_RELOC. OTOH, other architectures do it centrally in their lib_*/board.c::board_init_[fr](). I certainly do not know all ARM boards (maintainer added to CC), so, the question is: shall / can we do the same on ARM - set this flag centrally, or is there a reason not to do that? I see this email
http://lists.denx.de/pipermail/u-boot/2008-July/037375.html
trying to do exactly this, as a reply came this
http://lists.denx.de/pipermail/u-boot/2008-July/037389.html
promising a fix for all, and that resulted in this:
http://lists.denx.de/pipermail/u-boot/attachments/20080722/92a646d6/attachme...
which does indeed fix it for all boards setting CONFIG_SKIP_RELOCATE_UBOOT, i.e., booting directly from RAM... Please, correct me if I am wrong!
Thanks Guennadi --- Guennadi Liakhovetski, Ph.D.
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@denx.de
-----Original Message----- From: Guennadi Liakhovetski [mailto:lg@denx.de] Sent: den 5 september 2008 21:26 To: Joakim Tjernlund Cc: U-Boot@lists.denx.de; Jean-Christophe PLAGNIOL-VILLARD; Remy Bohmer Subject: RE: [REGRESSION] commit b502611b51... "Change env_get_char from a..." breaks imx31_phycore
On Fri, 5 Sep 2008, Joakim Tjernlund wrote:
-----Original Message----- From: Guennadi Liakhovetski [mailto:lg@denx.de] Sent: den 5 september 2008 20:01 To: U-Boot@lists.denx.de Cc: Joakim Tjernlund Subject: [REGRESSION] commit b502611b51... "Change env_get_char from a..." breaks imx31_phycore
Hi,
The aforementioned commit
commit b502611b51f02718c2d1117d4981dabceb5af6de Author: Joakim Tjernlund joakim.tjernlund@transmode.se Date: Sun Jul 6 12:30:09 2008 +0200
Change env_get_char from a global function ptr to a function This avoids an early global data reference. Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>found by bisection and causes at least the imx31_phycore board to break. The boot process becomes slow, printenv is very slow too, booting does not always come to the bootdelay countdown, tftp wtops working too. Reverting this commit from the current HEAD fixes the problem.
Your board probably don't flip the GD_FLG_RELOC flag after relocation. A few ARM boards had a problem with this too.
Ok, this sounds good, but a grep over the current tree (as of commit 3e3c026ed746a284c6f0ef139b26d859939de7e9) reveals only one ARM board that does this: davinci. It is also set globally if you define CONFIG_SKIP_RELOCATE_UBOOT, which also is done by a couple of boards. From the README:
CONFIG_SKIP_LOWLEVEL_INIT
CONFIG_SKIP_RELOCATE_UBOOT
[ARM only] If these variables are defined, then certain low level initializations (like setting up the memory controller) are omitted and/or U-Boot does not relocate itself into RAM. Normally these variables MUST NOT be defined. The only exception is when U-Boot is loaded (to RAM) by some other boot loader or by a debugger which performs these initializations itself.
So, this doesn't look like the proper way to force setting of GD_FLG_RELOC. OTOH, other architectures do it centrally in their lib_*/board.c::board_init_[fr](). I certainly do not know all ARM boards (maintainer added to CC), so, the question is: shall / can we do the same on ARM - set this flag centrally, or is there a reason not to do that? I see this email
http://lists.denx.de/pipermail/u-boot/2008-July/037375.html
trying to do exactly this, as a reply came this
http://lists.denx.de/pipermail/u-boot/2008-July/037389.html
promising a fix for all, and that resulted in this:
http://lists.denx.de/pipermail/u-boot/attachments/20080722/92a646d6/attachme...
which does indeed fix it for all boards setting CONFIG_SKIP_RELOCATE_UBOOT, i.e., booting directly from RAM... Please, correct me if I am wrong!
I think Remy and friends can best answer this.
Hello Guennadi,
http://lists.denx.de/pipermail/u-boot/attachments/20080722/92a646d6/attachme...
which does indeed fix it for all boards setting CONFIG_SKIP_RELOCATE_UBOOT, i.e., booting directly from RAM... Please, correct me if I am wrong!
You are _not_ wrong. It was my goal to fix it for all boards at once. Have you tried it already with the imx31_phycore board?
Kind Regards,
Remy
On Sat, 6 Sep 2008, Remy Bohmer wrote:
Hello Guennadi,
http://lists.denx.de/pipermail/u-boot/attachments/20080722/92a646d6/attachme...
which does indeed fix it for all boards setting CONFIG_SKIP_RELOCATE_UBOOT, i.e., booting directly from RAM... Please, correct me if I am wrong!
You are _not_ wrong. It was my goal to fix it for all boards at once. Have you tried it already with the imx31_phycore board?
Sorry, do not understand. I tried it - what? I was testing with the current git snapshot, where your patch is in, yes, and that is where I detected the breakage on imx31_phycore. But that board does not set CONFIG_SKIP_RELOCATE_UBOOT, because it is not booting directly from RAM as described in the README. So, your patch does not help, as well as on all other boards which do not or shall not define CONFIG_SKIP_RELOCATE_UBOOT. And you say, that "I am _not_ wrong", which means, a patch that aimed to fix the problem for all ARM boards has been replaced with your patch, which only fixes it for CONFIG_SKIP_RELOCATE_UBOOT boards, of which there probably should be very few or none at all out there.
Hence the question: shell we set GD_FLG_RELOC centrally really for all boards or are there boards out there that really must not have this flag set?
Thanks Guennadi --- Guennadi Liakhovetski, Ph.D.
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@denx.de
Dear Guennadi Liakhovetski,
In message Pine.LNX.4.64.0809061015510.3731@axis700.grange you wrote:
Hence the question: shell we set GD_FLG_RELOC centrally really for all boards or are there boards out there that really must not have this flag set?
No matter how U-Boot starts up (whether it starts itself, for example by running in ROM/NOR flash rightout of the processor's reset, or if it gets loaded into RAM by some other mechanism), at a certain point in the initalization sequence, U-Boot will run out of RAM.
For regular implementations of U-Boot this is implemented in the board_init_r() function, and it is one of the first actions to be done in board_init_r() to add GD_FLG_RELOC to the global flags.
Any other implementation (including that for ARM) must also set GD_FLG_RELOC as soon as it reaches the code whis is run from RAM for all configurations; my understanding is that this is in start_armboot(), right after the global data has been initialized.
Best regards,
Wolfgang Denk
On Sat, 6 Sep 2008, Wolfgang Denk wrote:
Dear Guennadi Liakhovetski,
In message Pine.LNX.4.64.0809061015510.3731@axis700.grange you wrote:
Hence the question: shell we set GD_FLG_RELOC centrally really for all boards or are there boards out there that really must not have this flag set?
No matter how U-Boot starts up (whether it starts itself, for example by running in ROM/NOR flash rightout of the processor's reset, or if it gets loaded into RAM by some other mechanism), at a certain point in the initalization sequence, U-Boot will run out of RAM.
For regular implementations of U-Boot this is implemented in the board_init_r() function, and it is one of the first actions to be done in board_init_r() to add GD_FLG_RELOC to the global flags.
Any other implementation (including that for ARM) must also set GD_FLG_RELOC as soon as it reaches the code whis is run from RAM for all configurations; my understanding is that this is in start_armboot(), right after the global data has been initialized.
Was this patch correct then?
http://lists.denx.de/pipermail/u-boot/2008-July/037375.html
So, we just have to revert this one
http://lists.denx.de/pipermail/u-boot/attachments/20080722/92a646d6/attachme...
and apply the former one? I cannot test now, will be able to do this in about 10 hours:-)
Thanks Guennadi --- Guennadi Liakhovetski, Ph.D.
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@denx.de
Dear Guennadi Liakhovetski,
In message Pine.LNX.4.64.0809061121010.3731@axis700.grange you wrote:
Was this patch correct then?
http://lists.denx.de/pipermail/u-boot/2008-July/037375.html
So, we just have to revert this one
http://lists.denx.de/pipermail/u-boot/attachments/20080722/92a646d6/attachme...
and apply the former one? I cannot test now, will be able to do this in about 10 hours:-)
I don't understand all impleications of the ARM implementation (which really should be fixed one day), but to me this sounds reasonable.
On Sat, 6 Sep 2008, Wolfgang Denk wrote:
Dear Guennadi Liakhovetski,
In message Pine.LNX.4.64.0809061121010.3731@axis700.grange you wrote:
Was this patch correct then?
http://lists.denx.de/pipermail/u-boot/2008-July/037375.html
So, we just have to revert this one
http://lists.denx.de/pipermail/u-boot/attachments/20080722/92a646d6/attachme...
and apply the former one? I cannot test now, will be able to do this in about 10 hours:-)
I don't understand all impleications of the ARM implementation (which really should be fixed one day), but to me this sounds reasonable.
Ok, took a bit longer, like 30 hours, but it's tested now - it works for imx31_phycore.
Thanks Guennadi --- Guennadi Liakhovetski, Ph.D.
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@denx.de
Hi
2008/9/7 Guennadi Liakhovetski lg@denx.de:
On Sat, 6 Sep 2008, Wolfgang Denk wrote:
Dear Guennadi Liakhovetski,
In message Pine.LNX.4.64.0809061121010.3731@axis700.grange you wrote:
Was this patch correct then?
http://lists.denx.de/pipermail/u-boot/2008-July/037375.html
So, we just have to revert this one
http://lists.denx.de/pipermail/u-boot/attachments/20080722/92a646d6/attachme...
and apply the former one? I cannot test now, will be able to do this in about 10 hours:-)
I don't understand all impleications of the ARM implementation (which really should be fixed one day), but to me this sounds reasonable.
Ok, took a bit longer, like 30 hours, but it's tested now - it works for imx31_phycore.
So what is the plan for this one? As far as I can see 2008.10-rc2 hasn't been fixed.
It would be nice to have this fixed in 2008.10.
Regards, Magnus
Dear Magnus,
In message 59b21cf20809142342t16b46b20tf35a1a4513b8cf9b@mail.gmail.com you wrote:
I don't understand all impleications of the ARM implementation (which really should be fixed one day), but to me this sounds reasonable.
Ok, took a bit longer, like 30 hours, but it's tested now - it works for imx31_phycore.
I guess Guennadi fixed only the env_get_char() related issues, not the other problems with the ARM implementation like that it does no relocation at all... ;-)
So what is the plan for this one? As far as I can see 2008.10-rc2 hasn't been fixed.
It would be nice to have this fixed in 2008.10.
Indeed, this must go in.
Best regards,
Wolfgang Denk
2008/9/6 Guennadi Liakhovetski lg@denx.de:
On Sat, 6 Sep 2008, Wolfgang Denk wrote:
Dear Guennadi Liakhovetski,
In message Pine.LNX.4.64.0809061015510.3731@axis700.grange you wrote:
Hence the question: shell we set GD_FLG_RELOC centrally really for all boards or are there boards out there that really must not have this flag set?
No matter how U-Boot starts up (whether it starts itself, for example by running in ROM/NOR flash rightout of the processor's reset, or if it gets loaded into RAM by some other mechanism), at a certain point in the initalization sequence, U-Boot will run out of RAM.
For regular implementations of U-Boot this is implemented in the board_init_r() function, and it is one of the first actions to be done in board_init_r() to add GD_FLG_RELOC to the global flags.
Any other implementation (including that for ARM) must also set GD_FLG_RELOC as soon as it reaches the code whis is run from RAM for all configurations; my understanding is that this is in start_armboot(), right after the global data has been initialized.
Was this patch correct then?
That's what I've been doing locally in order to get CONFIG_CMDLINE_EDITING to work so it seems correct.
Regards/Magnus
participants (5)
-
Guennadi Liakhovetski -
Joakim Tjernlund -
Magnus Lilja -
Remy Bohmer -
Wolfgang Denk