[U-Boot] [PATCH V3 1/8] arm: Add Prep subcommand support to bootm

[Andreas Bießmann]

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