[U-Boot-Users] [PATCH 1/3] ppc: Report back the location we put the device tree if we dont boot
Its useful to know where the device tree is if we have set 'autostart' to 'no. We come back to the prompt after a boot command and we can than post process the device tree but we need to know where it was put report this back via the env variable 'bootm_fdtaddr'.
Signed-off-by: Kumar Gala galak@kernel.crashing.org --- lib_ppc/bootm.c | 11 ++++++++++- 1 files changed, 10 insertions(+), 1 deletions(-)
diff --git a/lib_ppc/bootm.c b/lib_ppc/bootm.c index 81803dd..a872d31 100644 --- a/lib_ppc/bootm.c +++ b/lib_ppc/bootm.c @@ -277,8 +277,17 @@ do_bootm_linux(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[], #if defined(CFG_INIT_RAM_LOCK) && !defined(CONFIG_E500) unlock_ram_in_cache(); #endif - if (!images->autostart) + if (!images->autostart) { +#if defined(CONFIG_OF_LIBFDT) + if (of_flat_tree) { + char buf[32]; + + sprintf (buf, "%llx", (u64)(u32)of_flat_tree); + setenv("bootm_fdtaddr", buf); + } +#endif return ; + }
#if defined(CONFIG_OF_LIBFDT) if (of_flat_tree) { /* device tree; boot new style */
Add a boot command that supports the ePAPR client interface on powerpc.
Signed-off-by: Kumar Gala galak@kernel.crashing.org --- lib_ppc/bootm.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 63 insertions(+), 0 deletions(-)
diff --git a/lib_ppc/bootm.c b/lib_ppc/bootm.c index a872d31..1182c50 100644 --- a/lib_ppc/bootm.c +++ b/lib_ppc/bootm.c @@ -821,3 +821,66 @@ error: return 1; } #endif + +/*******************************************************************/ +/* boote - boot ePAPR */ +/*******************************************************************/ +#if defined(CONFIG_CMD_BOOT_EPAPR) +int do_boot_epapr (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[]) +{ + void (*kernel)(ulong r3, ulong r4, ulong r5, ulong r6, + ulong r7, ulong r8, ulong r9); + ulong kern, of_flat_tree, epapr_magic; + + if (argc != 3) { + printf ("Usage:\n%s\n", cmdtp->usage); + return 1; + } + + kern = simple_strtoul (argv[1], NULL, 16); + of_flat_tree = simple_strtoul (argv[2], NULL, 16); + + if (kern & 0x3) { + printf ("kernel address 0x%lx is not 4-byte aligned\n", kern); + return 1; + } + + if (of_flat_tree & 0x7) { + printf ("Flat device tree address 0x%lx is not 8-byte aligned\n", of_flat_tree); + return 1; + } + + kernel = (void (*)(ulong, ulong, ulong, ulong, + ulong, ulong, ulong))kern; + + disable_interrupts(); + + /* + * Linux Kernel Parameters (passing device tree): + * r3: pointer to the fdt + * r4: 0 + * r5: 0 + * r6: epapr magic + * r7: size of IMA in bytes + * r8: 0 + * r9: 0 + */ +#if defined(CONFIG_85xx) || defined(CONFIG_440) + epapr_magic = 0x45504150; +#else + epapr_magic = 0x65504150; +#endif + + debug (" Booting using OF flat tree...\n"); + (*kernel) (of_flat_tree, 0, 0, epapr_magic, CFG_BOOTMAPSZ, 0, 0); + /* does not return */ + + return 0; +} + +U_BOOT_CMD( + bootepapr, 3, 1, do_boot_epapr, + "bootepapr - boot according to ePAPR client interface\n", + "<entry point> <device tree address>\n" +); +#endif
if the environment variable 'disable_fdt_boardsetup' is set we skip doing the ft_board_setup().
Signed-off-by: Kumar Gala galak@kernel.crashing.org --- lib_ppc/bootm.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/lib_ppc/bootm.c b/lib_ppc/bootm.c index 1182c50..a5b3a45 100644 --- a/lib_ppc/bootm.c +++ b/lib_ppc/bootm.c @@ -192,7 +192,8 @@ do_bootm_linux(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[], } #ifdef CONFIG_OF_BOARD_SETUP /* Call the board-specific fixup routine */ - ft_board_setup(of_flat_tree, gd->bd); + if (!getenv("disable_fdt_boardsetup")) + ft_board_setup(of_flat_tree, gd->bd); #endif }
Dear Kumar,
In message 1218004332-20311-3-git-send-email-galak@kernel.crashing.org you wrote:
if the environment variable 'disable_fdt_boardsetup' is set we skip doing the ft_board_setup().
Such a long variable name is a PITA to type. Please chose a shorter name, and please add documentation for it.
Best regards,
Wolfgang Denk
In message 1218004332-20311-2-git-send-email-galak@kernel.crashing.org you wrote:
Add a boot command that supports the ePAPR client interface on powerpc.
What is the intended use of such a command?
How does it intergrate with with image formats supported by U-Boot? To me it seems that it's mostly intended to be called by other code and not interactively, since I cannot see any interfacing with the existing image types?
Best regards,
Wolfgang Denk
On Aug 6, 2008, at 3:21 AM, Wolfgang Denk wrote:
In message <1218004332-20311-2-git-send-email-galak@kernel.crashing.org
you wrote: Add a boot command that supports the ePAPR client interface on powerpc.
What is the intended use of such a command?
How does it intergrate with with image formats supported by U-Boot? To me it seems that it's mostly intended to be called by other code and not interactively, since I cannot see any interfacing with the existing image types?
I've been using as follows:
setenv autostart no bootm <fdt fixups> bootepapr 0 $bootm_fdtaddr
Additionally these seems like the low level functionality needed if we want to move to the "scriptable" bootm were we decouple image loading from the actual boot mechanism.
- k
In message 2C702584-3214-4E3C-8F59-7C46C2F4E8AF@kernel.crashing.org you wrote:
I've been using as follows:
setenv autostart no bootm
<fdt fixups> bootepapr 0 $bootm_fdtaddr
Now I see where your intentions with the autostart are coming from.
Sorry, this is not the way it is supposed to work, and not the way it is documented.
Additionally these seems like the low level functionality needed if we want to move to the "scriptable" bootm were we decouple image loading from the actual boot mechanism.
In terms of splitting bootm into smaller units you are riught. In terms of implementation it's wrong.
I think I will back out the autostart patch because it introduces incorrect and undocumented behaviour.
Best regards,
Wolfgang Denk
In message 1218004332-20311-1-git-send-email-galak@kernel.crashing.org you wrote:
Its useful to know where the device tree is if we have set 'autostart' to 'no. We come back to the prompt after a boot command and we can than post process the device tree but we need to know where it was put report this back via the env variable 'bootm_fdtaddr'.
NAK.
The whole code sequence in bootm.c seems broken to me:
272 debug ("## Transferring control to Linux (at address %08lx) ...\n", 273 (ulong)kernel); 274 275 show_boot_progress (15); 276 277 #if defined(CFG_INIT_RAM_LOCK) && !defined(CONFIG_E500) 278 unlock_ram_in_cache(); 279 #endif 280 if (!images->autostart) 281 return ; 282 283 #if defined(CONFIG_OF_LIBFDT)
The debug() [272f] should come immediately before booting the kernel (i. e. move below line 282) because it is supposed to show when we branch to Linux. No other code should be inbetween.
And the (!images->autostart) test makes absolutely no sense here. Documentation says:
autostart: if set to "yes", an image loaded using the rarpb, bootp, dhcp, tftp, disk, or docb commands will be automatically started (by internally calling the bootm command).
The "autostart" field introduced with the new image stuff behaves very different, and actually makes no sense to me at all.
Bartek, could you please comment what the intended behaviour was, and how it relates to the documentated behaviour?
Best regards,
Wolfgang Denk
Wolfgang Denk wrote:
In message 1218004332-20311-1-git-send-email-galak@kernel.crashing.org you wrote:
Its useful to know where the device tree is if we have set 'autostart' to 'no. We come back to the prompt after a boot command and we can than post process the device tree but we need to know where it was put report this back via the env variable 'bootm_fdtaddr'.
NAK.
The whole code sequence in bootm.c seems broken to me:
272 debug ("## Transferring control to Linux (at address %08lx) ...\n", 273 (ulong)kernel); 274 275 show_boot_progress (15); 276 277 #if defined(CFG_INIT_RAM_LOCK) && !defined(CONFIG_E500) 278 unlock_ram_in_cache(); 279 #endif 280 if (!images->autostart) 281 return ; 282 283 #if defined(CONFIG_OF_LIBFDT)
The debug() [272f] should come immediately before booting the kernel (i. e. move below line 282) because it is supposed to show when we branch to Linux. No other code should be inbetween.
And the (!images->autostart) test makes absolutely no sense here. Documentation says:
autostart: if set to "yes", an image loaded using the rarpb, bootp, dhcp, tftp, disk, or docb commands will be automatically started (by internally calling the bootm command).
Hi Wolfgang,
The test you're referring to was introduced by commit 75fa002c47171b73fb4c1f2c2fe4d6391c136276 "[new uImage] Respect autostart setting in linux bootm" by Kumar -- he should be better able to explain the details.
The "autostart" field introduced with the new image stuff behaves very different, and actually makes no sense to me at all.
Bartek, could you please comment what the intended behaviour was, and how it relates to the documentated behaviour?
It looks like that the "autostart" field has been added to the bootm_headers structure so that the arch-specific code can make decisions about booting without the need to call getenv("autostart"). Instead, the "autostart" field is set based on the env. variable once, and passed to boot-related functions via a parameter (e.g., "images" in do_bootm_linux()).
Again, this field has beed introduced by Kumar (f5614e7926863bf0225ec860d9b319741a9c4004, "[new uImage] Add autostart flag to bootm_headers structure"), who should be able to comment more.
Regards, Bartlomiej
Dear Bartek,
in message 48995F0F.8070807@semihalf.com you wrote:
The test you're referring to was introduced by commit 75fa002c47171b73fb4c1f2c2fe4d6391c136276 "[new uImage] Respect autostart setting in linux bootm" by Kumar -- he should be better able to explain the details.
Thanks - and sorry for blaming you, I should have checked this myself first.
It looks like that the "autostart" field has been added to the bootm_headers structure so that the arch-specific code can make decisions about booting without the need to call getenv("autostart"). Instead, the "autostart" field is set based on the env. variable once, and passed to boot-related functions via a parameter (e.g., "images" in do_bootm_linux()).
Again, this field has beed introduced by Kumar (f5614e7926863bf0225ec860d9b319741a9c4004, "[new uImage] Add autostart flag to bootm_headers structure"), who should be able to comment more.
Indeed - but as mentioned before, this all makes no sense to me, because with the intended and documented use of the "autostart" variable the bootm command will not be called at all.
Kumar, if I were to back out commit f5614e79 - what exactly would it break in your opinion?
Best regards,
Wolfgang Denk
On Aug 6, 2008, at 3:33 AM, Wolfgang Denk wrote:
Dear Bartek,
in message 48995F0F.8070807@semihalf.com you wrote:
The test you're referring to was introduced by commit 75fa002c47171b73fb4c1f2c2fe4d6391c136276 "[new uImage] Respect autostart setting in linux bootm" by Kumar -- he should be better able to explain the details.
Thanks - and sorry for blaming you, I should have checked this myself first.
It looks like that the "autostart" field has been added to the bootm_headers structure so that the arch-specific code can make decisions about booting without the need to call getenv("autostart"). Instead, the "autostart" field is set based on the env. variable once, and passed to boot-related functions via a parameter (e.g., "images" in do_bootm_linux()).
Again, this field has beed introduced by Kumar (f5614e7926863bf0225ec860d9b319741a9c4004, "[new uImage] Add autostart flag to bootm_headers structure"), who should be able to comment more.
Indeed - but as mentioned before, this all makes no sense to me, because with the intended and documented use of the "autostart" variable the bootm command will not be called at all.
Kumar, if I were to back out commit f5614e79 - what exactly would it break in your opinion?
There is intent and what the old code did. My feeling is that 'autostart = no' means to load the images but not actually jump to the new image.
The old code would load the "kernel" image and set 'filesize' and return (only if the image was of type IH_TYPE_STANDALONE).
I think when sent 'f5614e7926863bf0225ec860d9b319741a9c4004' I didn't notice the IH_TYPE_STANDALONE aspect.
We can revert the commit but it puts be back to square one w/o a solution to my problem.
- k
In message A71F1F8F-5136-40FB-8F4F-2603D284E451@kernel.crashing.org you wrote:
There is intent and what the old code did. My feeling is that 'autostart = no' means to load the images but not actually jump to the new image.
Correct. To be a bit more specific, "load" here means to load the kernel image to RAM (over Ethernet, USB, from disk etc.). The intention of "autostart" is (as documented) to avoid an explicit bootm.
It has never been an intention to split bootm into separate phases and make it terminate early. This cannot work in general.
We can revert the commit but it puts be back to square one w/o a solution to my problem.
I understand this, and I'm sorry for that. But the real fix is to split up bootm as discussed yesterday.
Best regards,
Wolfgang Denk
participants (3)
-
Bartlomiej Sieka -
Kumar Gala -
Wolfgang Denk