[U-Boot] [PATCH] bootz: fix silent console

[Simon Glass]

Hi Markus,

On 25 November 2014 at 01:31, Markus Niebel <list-09_u-boot at tqsc.de> wrote:
> Hello Simon,
>
> Am 20.11.2014 um 20:15 schrieb Simon Glass:
>> Hi Markus,
>>
>> On 18 November 2014 12:52, Markus Niebel <list-09_u-boot at tqsc.de> wrote:
>>> From: Markus Niebel <Markus.Niebel at tq-group.com>
>>>
>>> fixup was lost during split between command code and logic.
>>>
>>> Signed-off-by: Markus Niebel <Markus.Niebel at tq-group.com>
>>> ---
>>>  common/bootm.c     | 2 +-
>>>  common/cmd_bootm.c | 6 ++++++
>>>  include/bootm.h    | 2 ++
>>>  3 files changed, 9 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/common/bootm.c b/common/bootm.c
>>> index 6b3ea8c..94b9503 100644
>>> --- a/common/bootm.c
>>> +++ b/common/bootm.c
>>> @@ -467,7 +467,7 @@ ulong bootm_disable_interrupts(void)
>>>  #define CONSOLE_ARG     "console="
>>>  #define CONSOLE_ARG_LEN (sizeof(CONSOLE_ARG) - 1)
>>>
>>> -static void fixup_silent_linux(void)
>>> +void fixup_silent_linux(void)
>>>  {
>>>         char *buf;
>>>         const char *env_val;
>>> diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c
>>> index 6723360..d3e410a 100644
>>> --- a/common/cmd_bootm.c
>>> +++ b/common/cmd_bootm.c
>>> @@ -596,6 +596,12 @@ int do_bootz(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>>>          * disable interrupts ourselves
>>>          */
>>>         bootm_disable_interrupts();
>>> +#if defined(CONFIG_SILENT_CONSOLE) && !defined(CONFIG_SILENT_U_BOOT_ONLY)
>>> +       /*
>>> +        * same goes for fixup_silent_linux
>>> +        */
>>> +       fixup_silent_linux();
>>> +#endif
>>>
>>>         images.os.os = IH_OS_LINUX;
>>>         ret = do_bootm_states(cmdtp, flag, argc, argv,
>>> diff --git a/include/bootm.h b/include/bootm.h
>>> index b3d1a62..8e094b3 100644
>>> --- a/include/bootm.h
>>> +++ b/include/bootm.h
>>> @@ -50,6 +50,8 @@ ulong bootm_disable_interrupts(void);
>>>
>>>  /* This is a special function used by booti/bootz */
>>>  int bootm_find_ramdisk_fdt(int flag, int argc, char * const argv[]);
>>> +/* This function is used also used by bootz */
>>> +void fixup_silent_linux(void);
>>>
>>>  int do_bootm_states(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[],
>>>                     int states, bootm_headers_t *images, int boot_progress);
>>
>> Quite a bit of effort was expended trying to join this code back
>> together rather than having two separate code paths for bootm and
>> bootz.
>
> I Understand
>
>>
>> Since this is cmdline-related, I suggest something like this:
>>
>> - Enable BOOTM_STATE_OS_CMDLINE for all archs in do_bootm()
>> - Call fixup_silent_linux() in do_bootm_states() when processing
>> BOOTM_STATE_OS_CMDLINE
>> - Add BOOTM_STATE_OS_CMDLINE to the do_bootm_states() call in do_bootz()
>>
>
> I read through the code to find out, what the implications of your
> suggestions were. Since I'm not very familiar with this piece of code,
> please correct me, If i'm wrong:
>
> - rename the arch specific do_bootm_linux to arch_bootm_linux
> - implement a generic version of do_bootm_linux inside bootm_os.c
> - call arch_bootm_linux from generic do_bootm_linux
> - move the call to fixup_silent_linux to new do_bootm_linux for the
>   BOOTM_STATE_OS_CMDLINE case
> - mask BOOTM_STATE_OS_CMDLINE before calling arch_bootm_linux for all but
>   MIPS and PPC
>
> because this seems a bit intrusive I'm not sure. But if this is the wy to go, I
> will prepare a patch.

Yes that sounds complicated. But copying code in multiple places is
how the image/bootm support got so ugly, and it was a big effort to
fix it up. So I'd really like to avoid copying code.

But it should be a lot easier than that.

In do_boot() there is:

return do_bootm_states(cmdtp, flag, argc, argv, BOOTM_STATE_START |
   BOOTM_STATE_FINDOS | BOOTM_STATE_FINDOTHER |
   BOOTM_STATE_LOADOS |
#if defined(CONFIG_PPC) || defined(CONFIG_MIPS)
   BOOTM_STATE_OS_CMDLINE |
#endif
   BOOTM_STATE_OS_PREP | BOOTM_STATE_OS_FAKE_GO |
   BOOTM_STATE_OS_GO, &images, 1);
}

You should be able to remove the #ifdef.

Then in do_bootm_states() there is:

/* Load the OS */
if (!ret && (states & BOOTM_STATE_LOADOS)) {
   ...
#if defined(CONFIG_SILENT_CONSOLE) && !defined(CONFIG_SILENT_U_BOOT_ONLY)
   if (images->os.os == IH_OS_LINUX)
      fixup_silent_linux();
#endif
}

And later:

if (!ret && (states & BOOTM_STATE_OS_CMDLINE))
   ret = boot_fn(BOOTM_STATE_OS_CMDLINE, argc, argv, images);

You should be able to move the '#if defined' code to inside this if ()
- so that fixup_silent_linux() happens in the CMDLINE stage instead of
the LOADOS stage.

Then in do_bootz(), add BOOTM_STATE_OS_CMDLINE to the call to do_bootm_states().

Then bootm and bootz will still go along exactly the same code path.

The tricky bit is that of course this does affect each arch - you need
to make sure that each do_bootm_linux() function in
arch/xxx/lib/bootm.c will handle BOOTM_STATE_OS_CMDLINE (and return 0,
not -1). At a glance I can see that ARM returns -1 - it will need to
return 0.

Also each function in boot_os[] needs to do the same. I looked at most
of them and they check for BOOTM_STATE_OS_GO and return 0 for anything
else, so should be OK. Maybe do_bootm_standalone() will need to do so
too.

>
>> Or similar...that way we keep bootm and bootz in sync. The separate
>> call to bootm_disable_interrupts() is unfortunate but is a problem for
>> another day.
>>
>> Regards,
>> Simon
>>
> Regards,
> Markus

Regards,
Simon