Patchwork [U-Boot] Improve output of bootm command

login
register
mail settings
Submitter Andrew Murray
Date Sept. 10, 2011, 2:57 p.m.
Message ID <1315666667-6270-1-git-send-email-amurray@theiet.org>
Download mbox | patch
Permalink /patch/114187/
State Rejected
Headers show

Comments

Andrew Murray - Sept. 10, 2011, 2:57 p.m.
It's common for the bootm command to move a provided image in memory
prior to it's execution - this move can contribute to increased boot
times. This move is often seen in poorly configured devices which
boot from NAND.

This patch improves the output of the bootm command such that when
a move occurs it is made clear to the user.

Signed-off-by: Andrew Murray <amurray@theiet.org>
---
 common/cmd_bootm.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
Wolfgang Denk - Sept. 10, 2011, 3:06 p.m.
Dear Andrew Murray,

In message <1315666667-6270-1-git-send-email-amurray@theiet.org> you wrote:
> It's common for the bootm command to move a provided image in memory
> prior to it's execution - this move can contribute to increased boot
> times. This move is often seen in poorly configured devices which
> boot from NAND.
> 
> This patch improves the output of the bootm command such that when
> a move occurs it is made clear to the user.

Sorry, but I don't want to have this output.  It would be always be
printed for all systems booting from NOR flash, where the copy
operation is absolutely normal.

Also, on all PowerPC systems it is absolutely normal that you must
load the image to a different address in RAM, because otherwise you
would overwrite the exception vectors and thus crash U-Boot.

What you are trying to "warn" about is an absolutely normal use case,
and there is no reason for additional output.

Yes, there may be systems that could work more efficiently, but there
are many areas where you can optimize (starting with the Linux image
building).

Sorry, but NAK.


Best regards,

Wolfgang Denk
Andrew Murray - Sept. 10, 2011, 3:14 p.m.
Hi,

On 10 September 2011 16:06, Wolfgang Denk <wd@denx.de> wrote:

> Sorry, but I don't want to have this output.  It would be always be
> printed for all systems booting from NOR flash, where the copy
> operation is absolutely normal.
>
> Also, on all PowerPC systems it is absolutely normal that you must
> load the image to a different address in RAM, because otherwise you
> would overwrite the exception vectors and thus crash U-Boot.
>
> What you are trying to "warn" about is an absolutely normal use case,
> and there is no reason for additional output.
>
> Yes, there may be systems that could work more efficiently, but there
> are many areas where you can optimize (starting with the Linux image
> building).
>

Yes I understand your point but perhaps my description was not very useful.

The patch doesn't add any additional messages or output or anything which
indicates something is not ideal. Instead it renames the exiting 'Loading'
text to a arguably more descriptive 'Moving' text - and removes the message
all together when the move operation doesn't occur.

Thanks,

Andrew Murray
Mike Frysinger - Sept. 11, 2011, 7:22 p.m.
On Saturday, September 10, 2011 10:57:47 Andrew Murray wrote:
> --- a/common/cmd_bootm.c
> +++ b/common/cmd_bootm.c
> 
>  		if (load == blob_start || load == image_start) {
>  			..........
> -		} else {
> +		} else if (load != image_start) {

sorry, but why does this new if() make any sense ?  the only way this else 
branch could execute is if load != image_start since load == image_start was 
explicitly handled in the first if check.
-mike
Andrew Murray - Sept. 11, 2011, 8:13 p.m.
On 11 September 2011 20:22, Mike Frysinger <vapier@gentoo.org> wrote:

> On Saturday, September 10, 2011 10:57:47 Andrew Murray wrote:
> > --- a/common/cmd_bootm.c
> > +++ b/common/cmd_bootm.c
> >
> >               if (load == blob_start || load == image_start) {
> >                       ..........
> > -             } else {
> > +             } else if (load != image_start) {
>
> sorry, but why does this new if() make any sense ?  the only way this else
> branch could execute is if load != image_start since load == image_start
> was
> explicitly handled in the first if check.
> -mike
>

Yes that's correct. The move is executed and a print statement displayed -
only when the load address differs from the image start address. In other
words the patch prevents unnecessary/confusing output and a call to a
function that doesn't do anything when load == image_start.

Andrew Murray
Mike Frysinger - Sept. 11, 2011, 8:17 p.m.
On Sunday, September 11, 2011 16:13:26 Andrew Murray wrote:
> On 11 September 2011 20:22, Mike Frysinger <vapier@gentoo.org> wrote:
> > On Saturday, September 10, 2011 10:57:47 Andrew Murray wrote:
> > > --- a/common/cmd_bootm.c
> > > +++ b/common/cmd_bootm.c
> > > 
> > >               if (load == blob_start || load == image_start) {
> > >                       ..........
> > > -             } else {
> > > +             } else if (load != image_start) {
> > 
> > sorry, but why does this new if() make any sense ?  the only way this
> > else branch could execute is if load != image_start since load ==
> > image_start was
> > explicitly handled in the first if check.
> 
> Yes that's correct. The move is executed and a print statement displayed -
> only when the load address differs from the image start address. In other
> words the patch prevents unnecessary/confusing output and a call to a
> function that doesn't do anything when load == image_start.

i think you missed my point.  your proposed change to the "else" branch makes 
no difference to the existing code.

current code:
if (load == image_start) {
	...
} else {
	...
}

your new code:
if (load == image_start) {
	...
} else if (load != image_start) {
	...
}

your change to the if statement is pointless ?
-mike
Andrew Murray - Sept. 11, 2011, 10:25 p.m.
Hi Mike,

On 11 September 2011 21:17, Mike Frysinger <vapier@gentoo.org> wrote:
>
> your change to the if statement is pointless ?

You are right - I thought I was going crazy then realized I made this
patch on an earlier version of the code which didn't have the load ==
image_start check in the first line (see
http://git.denx.de/?p=u-boot.git;a=commitdiff;h=02cf345973a7fe9986626448a089ed54f1a26d13
from January). I must have blindly moved this patch forward. I will
get my tools in order.

Many thanks for you patience.

Andrew Murray

Patch

diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c
index 147e8de..b5f28b0 100644
--- a/common/cmd_bootm.c
+++ b/common/cmd_bootm.c
@@ -346,8 +346,8 @@  static int bootm_load_os(image_info_t os, ulong *load_end, int boot_progress)
 	case IH_COMP_NONE:
 		if (load == blob_start || load == image_start) {
 			printf ("   XIP %s ... ", type_name);
-		} else {
-			printf ("   Loading %s ... ", type_name);
+		} else if (load != image_start) {
+			printf ("   Moving %s ... ", type_name);
 			memmove_wd ((void *)load, (void *)image_start,
 					image_len, CHUNKSZ);
 		}