Dear Simon,
Am 26.08.2011 11:57, schrieb Simon Schwarz:
> Dear Andreas,
>
> On 08/25/2011 11:40 AM, Andreas Bießmann wrote:
>> Dear Simon,
<snip>
>>> void arch_lmb_reserve(struct lmb *lmb)
>>> @@ -98,63 +101,67 @@ int do_bootm_linux(int flag, int argc, char
>>> *argv[], bootm_headers_t *images)
>>> bd_t *bd = gd->bd;
>>> char *s;
>>> int machid = bd->bi_arch_number;
>>> - void (*kernel_entry)(int zero, int arch, uint params);
>>> + void (*kernel_entry)(int zero, int arch, uint params) = NULL;
>>
>> This should not be necessary, kernel_entry would be on bss which should
>> be initialized to zero by start.S.
>
> I added this because of compiler warnings that it could be used
> uninitialized.
I see, you changed the flow of this function elementary. Sorry, did not
realize that.
>>>
>>> #ifdef CONFIG_CMDLINE_TAG
>>> char *commandline = getenv ("bootargs");
>>> #endif
>>> -
>>> - if ((flag != 0)&& (flag != BOOTM_STATE_OS_GO))
>>> - return 1;
>>> -
>>> - s = getenv ("machid");
>>> - if (s) {
>>> - machid = simple_strtoul (s, NULL, 16);
>>> - printf ("Using machid 0x%x from environment\n", machid);
>>> - }
>>> -
>>> - show_boot_progress (15);
>>> + if ((flag != 0)&& (!(flag& BOOTM_STATE_OS_GO ||
>>> + flag& BOOTM_STATE_OS_PREP)))
>>
>> switch'n'case would be much cleaner here. And seperating the
>> functionality into functions would be nice too.
>
> Hehe. Somehow I did know that this topic will come up. I intended to
> change the code as little as possible.
Well, switch'n'case is not correct here, sorry for misleading comment.
> So essentially this would mean to rewrite this to reflect the structure
> of the ppc version. Will do if there are no objections.
+1 for rewrite, Albert how do you think about?
> If there are no objections I also would like to separate this patch from
> this series. This has some advantages:
> - Support for the prep subcommand is essential for saving the boot
> parameters. (if prep is in saving can also be done manually)
> - I think that there won't be much discussion about the usefulness of
> implementing this - just some about the how.
I think it would be good idea to add the 'bootm prep' command to arm
implementation of bootm in a separate step.
<snip>
>>> + if (flag == 0 || flag& BOOTM_STATE_OS_GO) {
>>
>> flag == 0? Shouldn't that be flag == BOOTM_STATE_OS_GO?
>
> flag = 0 means that no subcommand was issued -> execute everything.
again, sorry for the noise.
<snip>
best regards
Andreas Bießmann
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
