diff mbox

[U-Boot,01/15] Make bootm optionally use pre-existing atags for Linux kernel boot.

Message ID 1314876881-9669-1-git-send-email-pali.rohar@gmail.com
State Changes Requested
Headers show

Commit Message

Pali Rohár Sept. 1, 2011, 11:34 a.m. UTC 
From: Alistair Buxton <a.j.buxton@gmail.com>

This patch adapts the bootm command so that it can use an existing atags command
set up by a previous bootloader. If the environment variable "atags" is unset,
bootm behaves as normal. If "atags" is set, bootm will skip all boot args setup
entirely, and pass the address found in "atags". For example, if a previous boot
loader already set up the atags struct at 0x80000100:

setenv atags 0x80000100; bootm 0x80008000

Signed-off-by: Alistair Buxton <a.j.buxton@gmail.com>
---
 arch/arm/lib/bootm.c |   37 ++++++++++++++++++++++++++++---------
 1 files changed, 28 insertions(+), 9 deletions(-)

Comments

Mike Frysinger Sept. 1, 2011, 1:52 p.m. UTC | #1 
> +#ifdef CONFIG_CHAINLOADER
> +	uint	params;
> +	#define PARAMS params
> +#else
> +	#define PARAMS (bd->bi_boot_params)
> +#endif

do not indent the "#" with preprocessors

also, this can be rewritten a bit.  always declare uint params regardless of 
the define, and then when in the chainloader logic, set it to the env value, 
otherwise set it to bd->bi_boot_params.  then you can delete this dedicated 
and ugly "PARAMS" define.

> +#ifdef CONFIG_CHAINLOADER

this is kind of a crappy define.  how about "CONFIG_ATAGSADDR".

also, you'll need to update toplevel README to document the new option.

> +	s = getenv ("atags");

"atagsaddr" is probably a better name as a plain "atags" can be interpreted 
multiple ways

> +		params = simple_strtoul (s, NULL, 16);

no spaces before the open paren in func calls.
wrong: foo (a);
right: foo(a);

> +		printf ("Using existing atags at 0x%x\n", params);

0x%x -> %#x
-mike
Pali Rohár Oct. 8, 2011, 11:37 p.m. UTC | #2 
On Thursday 01 September 2011 09:52:08 you wrote:
> > +#ifdef CONFIG_CHAINLOADER
> > +	uint	params;
> > +	#define PARAMS params
> > +#else
> > +	#define PARAMS (bd->bi_boot_params)
> > +#endif
> 
> do not indent the "#" with preprocessors

ok

> 
> also, this can be rewritten a bit.  always declare uint params regardless of
> the define, and then when in the chainloader logic, set it to the env
> value, otherwise set it to bd->bi_boot_params.  then you can delete this
> dedicated and ugly "PARAMS" define.

ok, PARAMS was deleted.

> 
> > +#ifdef CONFIG_CHAINLOADER
> 
> this is kind of a crappy define.  how about "CONFIG_ATAGSADDR".

I think that here CONFIG_CHAINLOADER is not needed. I think that bootm can use 
other atag without needed defined CONFIG_CHAINLOADER.

> 
> also, you'll need to update toplevel README to document the new option.

ok, I update toplevel README file.

> 
> > +	s = getenv ("atags");
> 
> "atagsaddr" is probably a better name as a plain "atags" can be interpreted
> multiple ways

renamed to atagaddr.

> 
> > +		params = simple_strtoul (s, NULL, 16);
> 
> no spaces before the open paren in func calls.
> wrong: foo (a);
> right: foo(a);

fixed

> 
> > +		printf ("Using existing atags at 0x%x\n", params);
> 
> 0x%x -> %#x

fixed

> -mike
diff mbox

Patch

diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c
index 802e833..262efda 100644
--- a/arch/arm/lib/bootm.c
+++ b/arch/arm/lib/bootm.c
@@ -100,6 +100,13 @@  int do_bootm_linux(int flag, int argc, char *argv[], bootm_headers_t *images)
 	int	machid = bd->bi_arch_number;
 	void	(*kernel_entry)(int zero, int arch, uint params);
 
+#ifdef CONFIG_CHAINLOADER
+	uint	params;
+	#define PARAMS params
+#else
+	#define PARAMS (bd->bi_boot_params)
+#endif
+
 #ifdef CONFIG_CMDLINE_TAG
 	char *commandline = getenv ("bootargs");
 #endif
@@ -125,34 +132,46 @@  int do_bootm_linux(int flag, int argc, char *argv[], bootm_headers_t *images)
 	debug ("## Transferring control to Linux (at address %08lx) ...\n",
 	       (ulong) kernel_entry);
 
+#ifdef CONFIG_CHAINLOADER
+	s = getenv ("atags");
+	if (s) {
+		params = simple_strtoul (s, NULL, 16);
+		printf ("Using existing atags at 0x%x\n", params);
+	} else {
+#endif
+
 #if defined (CONFIG_SETUP_MEMORY_TAGS) || \
     defined (CONFIG_CMDLINE_TAG) || \
     defined (CONFIG_INITRD_TAG) || \
     defined (CONFIG_SERIAL_TAG) || \
     defined (CONFIG_REVISION_TAG)
-	setup_start_tag (bd);
+		setup_start_tag (bd);
 #ifdef CONFIG_SERIAL_TAG
-	setup_serial_tag (&params);
+		setup_serial_tag (&params);
 #endif
 #ifdef CONFIG_REVISION_TAG
-	setup_revision_tag (&params);
+		setup_revision_tag (&params);
 #endif
 #ifdef CONFIG_SETUP_MEMORY_TAGS
-	setup_memory_tags (bd);
+		setup_memory_tags (bd);
 #endif
 #ifdef CONFIG_CMDLINE_TAG
-	setup_commandline_tag (bd, commandline);
+		setup_commandline_tag (bd, commandline);
 #endif
 #ifdef CONFIG_INITRD_TAG
-	if (images->rd_start && images->rd_end)
-		setup_initrd_tag (bd, images->rd_start, images->rd_end);
+		if (images->rd_start && images->rd_end)
+			setup_initrd_tag (bd, images->rd_start, images->rd_end);
+#endif
+		setup_end_tag(bd);
 #endif
-	setup_end_tag(bd);
+
+#ifdef CONFIG_CHAINLOADER
+	}
 #endif
 
 	announce_and_cleanup();
 
-	kernel_entry(0, machid, bd->bi_boot_params);
+	kernel_entry(0, machid, PARAMS);
 	/* does not return */
 
 	return 1;