Patchwork [U-Boot] bootm: fix conditional controlling call to fixup_silent_linux

login
register
mail settings
Submitter Paul B. Henson
Date Aug. 4, 2013, 4:29 a.m.
Message ID <20130804043856.C05C55C0E1@zaphod.pbhware.com>
Download mbox | patch
Permalink /patch/264456/
State Accepted
Delegated to: Tom Rini
Headers show

Comments

Paul B. Henson - Aug. 4, 2013, 4:29 a.m.
This function is only defined if CONFIG_SILENT_CONSOLE is set and
CONFIG_SILENT_U_BOOT_ONLY is not set, the call to it should be based
on the same conditions.

Signed-off-by: Paul B. Henson <henson@acm.org>
---
 common/cmd_bootm.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Paul B. Henson - Aug. 4, 2013, 7:59 p.m.
Looks like this was broken in commit 35fc84fa1ff51e15ecd3e464dac87eb105ffed30,
the refactor changed the conditional test when the code was moved.


On Sat, Aug 03, 2013 at 09:29:09PM -0700, Paul B. Henson wrote:
> This function is only defined if CONFIG_SILENT_CONSOLE is set and
> CONFIG_SILENT_U_BOOT_ONLY is not set, the call to it should be based
> on the same conditions.
> 
> Signed-off-by: Paul B. Henson <henson@acm.org>
> ---
>  common/cmd_bootm.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c
> index 046e22f..691d16b 100644
> --- a/common/cmd_bootm.c
> +++ b/common/cmd_bootm.c
> @@ -636,7 +636,7 @@ static int do_bootm_states(cmd_tbl_t *cmdtp, int flag, int argc,
>  			goto err;
>  		else if (ret == BOOTM_ERR_OVERLAP)
>  			ret = 0;
> -#ifdef CONFIG_SILENT_CONSOLE
> +#if defined(CONFIG_SILENT_CONSOLE) && !defined(CONFIG_SILENT_U_BOOT_ONLY)
>  		if (images->os.os == IH_OS_LINUX)
>  			fixup_silent_linux();
>  #endif
> -- 
> 1.7.8.6
Simon Glass - Aug. 4, 2013, 8:10 p.m.
Hi Paul,

On Sun, Aug 4, 2013 at 1:59 PM, Paul B. Henson <henson@acm.org> wrote:

> Looks like this was broken in commit
> 35fc84fa1ff51e15ecd3e464dac87eb105ffed30,
> the refactor changed the conditional test when the code was moved.
>
>
This looks right.

Acked-by: Simon Glass <sjg@chromium.org>

But which board does it actually break, please?


> On Sat, Aug 03, 2013 at 09:29:09PM -0700, Paul B. Henson wrote:
> > This function is only defined if CONFIG_SILENT_CONSOLE is set and
> > CONFIG_SILENT_U_BOOT_ONLY is not set, the call to it should be based
> > on the same conditions.
> >
> > Signed-off-by: Paul B. Henson <henson@acm.org>
> > ---
> >  common/cmd_bootm.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c
> > index 046e22f..691d16b 100644
> > --- a/common/cmd_bootm.c
> > +++ b/common/cmd_bootm.c
> > @@ -636,7 +636,7 @@ static int do_bootm_states(cmd_tbl_t *cmdtp, int
> flag, int argc,
> >                       goto err;
> >               else if (ret == BOOTM_ERR_OVERLAP)
> >                       ret = 0;
> > -#ifdef CONFIG_SILENT_CONSOLE
> > +#if defined(CONFIG_SILENT_CONSOLE) &&
> !defined(CONFIG_SILENT_U_BOOT_ONLY)
> >               if (images->os.os == IH_OS_LINUX)
> >                       fixup_silent_linux();
> >  #endif
> > --
> > 1.7.8.6
>

Regards,
Simon
Paul B. Henson - Aug. 5, 2013, 2:43 a.m.
On Sun, Aug 04, 2013 at 02:10:14PM -0600, Simon Glass wrote:

> But which board does it actually break, please?

I don't think any of the default configs included in u-boot would have
broken, while some of them define CONFIG_SILENT_CONSOLE, none of them
define CONFIG_SILENT_U_BOOT_ONLY. I was working with a freescale mx28evk
prototyping something that will eventually be a custom board, and I
modified the config to drop u-boot output but not drop the linux kernel
output. It was working fine with a git checkout from late May, and I
noticed when I tried out the latest release it wouldn't compile anymore
as it tried to call a symbol that was never defined.

So it was really only broken for a custom config, but as the behavior is
documented in doc/README.silent, I assume it's still desirable for it to
work :).

Thanks...
Simon Glass - Aug. 5, 2013, 4:14 a.m.
Hi Paul,

On Sun, Aug 4, 2013 at 8:43 PM, Paul B. Henson <henson@acm.org> wrote:

> On Sun, Aug 04, 2013 at 02:10:14PM -0600, Simon Glass wrote:
>
> > But which board does it actually break, please?
>
> I don't think any of the default configs included in u-boot would have
> broken, while some of them define CONFIG_SILENT_CONSOLE, none of them
> define CONFIG_SILENT_U_BOOT_ONLY. I was working with a freescale mx28evk
> prototyping something that will eventually be a custom board, and I
> modified the config to drop u-boot output but not drop the linux kernel
> output. It was working fine with a git checkout from late May, and I
> noticed when I tried out the latest release it wouldn't compile anymore
> as it tried to call a symbol that was never defined.
>
> So it was really only broken for a custom config, but as the behavior is
> documented in doc/README.silent, I assume it's still desirable for it to
> work :).
>

OK, well at least that explains why I didn't see the problem. Thanks.


>
> Thanks...
>

Regards,
Simon
Tom Rini - Aug. 18, 2013, 9:49 p.m.
On Sat, Aug 03, 2013 at 09:29:09PM -0700, Paul B. Henson wrote:

> This function is only defined if CONFIG_SILENT_CONSOLE is set and
> CONFIG_SILENT_U_BOOT_ONLY is not set, the call to it should be based
> on the same conditions.
> 
> Signed-off-by: Paul B. Henson <henson@acm.org>
> Acked-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/master, thanks!

Patch

diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c
index 046e22f..691d16b 100644
--- a/common/cmd_bootm.c
+++ b/common/cmd_bootm.c
@@ -636,7 +636,7 @@  static int do_bootm_states(cmd_tbl_t *cmdtp, int flag, int argc,
 			goto err;
 		else if (ret == BOOTM_ERR_OVERLAP)
 			ret = 0;
-#ifdef CONFIG_SILENT_CONSOLE
+#if defined(CONFIG_SILENT_CONSOLE) && !defined(CONFIG_SILENT_U_BOOT_ONLY)
 		if (images->os.os == IH_OS_LINUX)
 			fixup_silent_linux();
 #endif