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

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

[Markus Niebel]

From: Markus Niebel <Markus.Niebel@tq-group.com>

fixup was lost during split between command code and logic.

Signed-off-by: Markus Niebel <Markus.Niebel@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);
-- 
2.1.1

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

[Simon Glass]

Hi Markus,

On 18 November 2014 12:52, Markus Niebel <list-09_u-boot@tqsc.de> wrote:
> From: Markus Niebel <Markus.Niebel@tq-group.com>
>
> fixup was lost during split between command code and logic.
>
> Signed-off-by: Markus Niebel <Markus.Niebel@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.

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()

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

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

[Markus Niebel]

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@tqsc.de> wrote:
>> From: Markus Niebel <Markus.Niebel@tq-group.com>
>>
>> fixup was lost during split between command code and logic.
>>
>> Signed-off-by: Markus Niebel <Markus.Niebel@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.

> 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

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

[Simon Glass]

Hi Markus,

On 25 November 2014 at 01:31, Markus Niebel <list-09_u-boot@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@tqsc.de> wrote:
>>> From: Markus Niebel <Markus.Niebel@tq-group.com>
>>>
>>> fixup was lost during split between command code and logic.
>>>
>>> Signed-off-by: Markus Niebel <Markus.Niebel@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