Patchwork [U-Boot] arm: Adds saving of Kernel boot args to NAND flash

login
register
mail settings
Submitter Simon Schwarz
Date July 5, 2011, 1:20 p.m.
Message ID <1309872039-15316-1-git-send-email-simonschwarzcor@gmail.com>
Download mbox | patch
Permalink /patch/103278/
State Superseded
Headers show

Comments

Simon Schwarz - July 5, 2011, 1:20 p.m.
Adds the saving of either ATAGS or FDT kernel argument image to NAND flash.
This image then can be used in SPL boot.

This adds two CONFIG_ paramter to board configuration (in this RFC as example added to devkit8000.h):
CONFIG_SAVE_BOOT_ARGS		makes the feature active
CONFIG_BOOT_ARGS_NAND_OFS	defines the offset in NAND flash where the image
	is saved

For OMAP34XX the image is saved with hw-ecc.

Signed-off-by: Simon Schwarz <simonschwarzcor@gmail.com>
---
This is an RFC on how this can be done.

What I don't like here is the omap specific hw ecc switch. But the nand-spl has just hw-ecc yet - at least afaik.

Belonging discussion: http://marc.info/?t=130942998000003

Regards
Simon

 arch/arm/lib/bootm.c         |   64 ++++++++++++++++++++++++++++++++++++++++++
 include/configs/devkit8000.h |    4 ++
 2 files changed, 68 insertions(+), 0 deletions(-)
Scott Wood - July 5, 2011, 7:36 p.m.
On Tue, 5 Jul 2011 15:20:39 +0200
Simon Schwarz <simonschwarzcor@googlemail.com> wrote:

> +#ifdef CONFIG_SAVE_BOOT_ARGS
> +/* This function writes given bootparams to NAND flash
> + *  adr: Start adress of Kernel parameter image (ATAGS, FDT)
> + *  length: length of the image in byte
> + *
> + * borrowd heavily from common/cmd_nand.c
> + */
> +void boot_params_to_nand(u_char *adr, size_t length) {

Brace on its own line for functions

> +	/* write */
> +	if(nand_write_skip_bad(nand, off, &length, adr, 0))
> +		printf("FAILED!\n");

Space after "if"

> +#ifdef CONFIG_SAVE_BOOT_ARGS
> +	struct tag *t;
> +	size_t size=0;
> +#endif

Spaces around =

>  	if ((flag != 0) && (flag != BOOTM_STATE_OS_GO))
>  		return 1;
>  
> @@ -150,6 +198,17 @@ int do_bootm_linux(int flag, int argc, char *argv[], bootm_headers_t *images)
>  	setup_end_tag(bd);
>  #endif
>  
> +#ifdef CONFIG_SAVE_BOOT_ARGS
> +	printf("write ATAGS to NAND...\n");
> +
> +	/* get size of atags */
> +	for_each_tag(t, (struct tag *)(bd->bi_boot_params))
> +		size += t->hdr.size;
> +	size += 2; /* ATAG_NONE has size 0 */
> +	size *= 4;  /*  words -> byte! */
> +	boot_params_to_nand((u_char *)bd->bi_boot_params, size);
> +#endif
> +
>  	announce_and_cleanup();
>  
>  	kernel_entry(0, machid, bd->bi_boot_params);
> @@ -208,6 +267,11 @@ static int bootm_linux_fdt(int machid, bootm_headers_t *images)
>  
>  	fdt_initrd(*of_flat_tree, *initrd_start, *initrd_end, 1);
>  
> +#ifdef CONFIG_SAVE_BOOT_ARGS
> +	printf("write FDT to NAND...\n");
> +	boot_params_to_nand((u_char *)(*of_flat_tree),of_size);
> +#endif

Why are you writing to NAND as part of bootm?

If you just want access to the results of the FDT/ATAG preparation that
bootm does, consider using the bootm subcommands to prepare them, then
normal U-Boot commands to write to NAND.

-Scott
Simon Schwarz - July 6, 2011, 7:42 a.m.
Hi,

thanks for your feedback!

I will address the style problems - didn't run checkpatch for the RFC
- sorry my fault.

> Why are you writing to NAND as part of bootm?
>
> If you just want access to the results of the FDT/ATAG preparation that
> bootm does, consider using the bootm subcommands to prepare them, then
> normal U-Boot commands to write to NAND.

Hm thats a point. I see this as a feature as it saves new settings
without the need of special commands - you start the same
configuration either with u-boot or the SPL. But you are right - an
extra command would also be nice. I will add this.

Regards
Simon
Simon Schwarz - July 6, 2011, 11:54 a.m.
> If you just want access to the results of the FDT/ATAG preparation that
> bootm does, consider using the bootm subcommands to prepare them, then
> normal U-Boot commands to write to NAND.
Me again. I rethought and the problem with bootm is that the
subfunctions (bootm_linux_fdt/do_bootm_linux) are directly boot the
kernel. That means either I copy almost the whole function and delete
the kernel_entry-call. Or I move the tag/fdt creation out of
bootm-command and create a extra function.

The first way I consider bad because of code duplication - therefore I
would take the second path. Any Objections?

Regards
Simon
Scott Wood - July 6, 2011, 5:27 p.m.
On Wed, 6 Jul 2011 09:42:52 +0200
Simon Schwarz <simonschwarzcor@googlemail.com> wrote:

> Hi,
> 
> thanks for your feedback!
> 
> I will address the style problems - didn't run checkpatch for the RFC
> - sorry my fault.
> 
> > Why are you writing to NAND as part of bootm?
> >
> > If you just want access to the results of the FDT/ATAG preparation that
> > bootm does, consider using the bootm subcommands to prepare them, then
> > normal U-Boot commands to write to NAND.
> 
> Hm thats a point. I see this as a feature as it saves new settings
> without the need of special commands

But that's not what bootm is for.  If you're doing something new, it should
be in a new command -- or in a script of existing commands.

-Scott
Albert ARIBAUD - July 6, 2011, 6:55 p.m.
Hi Simon,

Le 05/07/2011 15:20, Simon Schwarz a écrit :

> This is an RFC on how this can be done.

Then please tag the subject with [RFC] next time!

Amicalement,
Simon Schwarz - July 7, 2011, 3:50 p.m.
> But that's not what bootm is for.  If you're doing something new, it should
> be in a new command -- or in a script of existing commands.

True. The upper patch was the quickest way to do this.

I implemented a first test for a command. This is a bit ugly because I
have to rip apart bootm. For now I heavily used extern and removed
static keywords from bootm - this is ok for the testing but not to get
it into mainline...

I'am not sure how i can handle this to get it into mainline. The main
problem is in arch/arm/lib/bootm.c implementation:
static void setup_start_tag (bd_t *bd);
static void setup_memory_tags (bd_t *bd);
static void setup_commandline_tag (bd_t *bd, char *commandline);
static void setup_initrd_tag (bd_t *bd, ulong initrd_start,
                  ulong initrd_end);
static void setup_end_tag (bd_t *bd);
static struct tag *params;
static ulong get_sp(void);
static int bootm_linux_fdt(int machid, bootm_headers_t *images);

These function i need to build the atags. But they are defined static
and not really meant to be used from the outside. What comes to my
mind here is to create a new setup_atags.c in the same directory + a
header in include. Is this the right place for this? Maybe another
solution?

Regards
Simon
Scott Wood - July 7, 2011, 4:34 p.m.
On Thu, 7 Jul 2011 17:50:12 +0200
Simon Schwarz <simonschwarzcor@googlemail.com> wrote:

> > But that's not what bootm is for.  If you're doing something new, it should
> > be in a new command -- or in a script of existing commands.
> 
> True. The upper patch was the quickest way to do this.
> 
> I implemented a first test for a command. This is a bit ugly because I
> have to rip apart bootm. For now I heavily used extern and removed
> static keywords from bootm - this is ok for the testing but not to get
> it into mainline...
> 
> I'am not sure how i can handle this to get it into mainline. The main
> problem is in arch/arm/lib/bootm.c implementation:
> static void setup_start_tag (bd_t *bd);
> static void setup_memory_tags (bd_t *bd);
> static void setup_commandline_tag (bd_t *bd, char *commandline);
> static void setup_initrd_tag (bd_t *bd, ulong initrd_start,
>                   ulong initrd_end);
> static void setup_end_tag (bd_t *bd);
> static struct tag *params;
> static ulong get_sp(void);
> static int bootm_linux_fdt(int machid, bootm_headers_t *images);
> 
> These function i need to build the atags. But they are defined static
> and not really meant to be used from the outside. What comes to my
> mind here is to create a new setup_atags.c in the same directory + a
> header in include. Is this the right place for this? Maybe another
> solution?

Look at bootm subcommands to run just the portion of the existing bootm
functionality that you need.  Type "help bootm" for details.

-Scott
Simon Schwarz - July 8, 2011, 7:53 a.m.
> Look at bootm subcommands to run just the portion of the existing bootm
> functionality that you need.  Type "help bootm" for details.

Of course I could implement a subcommand - actually this seems to be
the best method.

IMHO "prep" would be the right one to use? correct?

Regards
Simon
Scott Wood - July 8, 2011, 4:03 p.m.
On Fri, 8 Jul 2011 09:53:46 +0200
Simon Schwarz <simonschwarzcor@googlemail.com> wrote:

> > Look at bootm subcommands to run just the portion of the existing bootm
> > functionality that you need.  Type "help bootm" for details.
> 
> Of course I could implement a subcommand - actually this seems to be
> the best method.
> 
> IMHO "prep" would be the right one to use? correct?

Probably.

-Scott

Patch

diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c
index 802e833..57e08d3 100644
--- a/arch/arm/lib/bootm.c
+++ b/arch/arm/lib/bootm.c
@@ -5,6 +5,10 @@ 
  *
  * Copyright (C) 2001  Erik Mouw (J.A.K.Mouw@its.tudelft.nl)
  *
+ * Copyright (C) 2011
+ * Corscience GmbH & Co. KG - Simon Schwarz <schwarz@corscience.de>
+ * - added saving of kernel-parameter-image to NAND flash
+ *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
  * the Free Software Foundation; either version 2 of the License, or
@@ -30,6 +34,14 @@ 
 #include <libfdt.h>
 #include <fdt_support.h>
 
+#ifdef CONFIG_SAVE_BOOT_ARGS
+#include <nand.h>
+#include <asm/setup.h>
+#ifdef CONFIG_OMAP34XX
+#include <asm/arch/sys_proto.h>
+#endif
+#endif
+
 DECLARE_GLOBAL_DATA_PTR;
 
 #if defined (CONFIG_SETUP_MEMORY_TAGS) || \
@@ -58,6 +70,38 @@  static ulong get_sp(void);
 static int bootm_linux_fdt(int machid, bootm_headers_t *images);
 #endif
 
+#ifdef CONFIG_SAVE_BOOT_ARGS
+/* This function writes given bootparams to NAND flash
+ *  adr: Start adress of Kernel parameter image (ATAGS, FDT)
+ *  length: length of the image in byte
+ *
+ * borrowd heavily from common/cmd_nand.c
+ */
+void boot_params_to_nand(u_char *adr, size_t length) {
+	nand_info_t *nand = &nand_info[nand_curr_device]; /* use current dev */
+	loff_t off = CONFIG_BOOT_ARGS_NAND_OFS;
+	nand_erase_options_t opts;
+
+#ifdef CONFIG_OMAP34XX
+	omap_nand_switch_ecc(1); /* use hw ecc on omap for SPL compat */
+#endif
+	/* erase */
+	memset(&opts, 0, sizeof(opts));
+	opts.offset = off;
+	opts.length = length;
+	opts.quiet = 1;
+	opts.spread = 1;
+	nand_erase_opts(nand,&opts);
+
+	/* write */
+	if(nand_write_skip_bad(nand, off, &length, adr, 0))
+		printf("FAILED!\n");
+
+	printf("Written to offset 0x%llX, size: %d bytes\n",
+		off, length);
+}
+#endif /* CONFIG_SAVE_BOOT_ARGS */
+
 void arch_lmb_reserve(struct lmb *lmb)
 {
 	ulong sp;
@@ -104,6 +148,10 @@  int do_bootm_linux(int flag, int argc, char *argv[], bootm_headers_t *images)
 	char *commandline = getenv ("bootargs");
 #endif
 
+#ifdef CONFIG_SAVE_BOOT_ARGS
+	struct tag *t;
+	size_t size=0;
+#endif
 	if ((flag != 0) && (flag != BOOTM_STATE_OS_GO))
 		return 1;
 
@@ -150,6 +198,17 @@  int do_bootm_linux(int flag, int argc, char *argv[], bootm_headers_t *images)
 	setup_end_tag(bd);
 #endif
 
+#ifdef CONFIG_SAVE_BOOT_ARGS
+	printf("write ATAGS to NAND...\n");
+
+	/* get size of atags */
+	for_each_tag(t, (struct tag *)(bd->bi_boot_params))
+		size += t->hdr.size;
+	size += 2; /* ATAG_NONE has size 0 */
+	size *= 4;  /*  words -> byte! */
+	boot_params_to_nand((u_char *)bd->bi_boot_params, size);
+#endif
+
 	announce_and_cleanup();
 
 	kernel_entry(0, machid, bd->bi_boot_params);
@@ -208,6 +267,11 @@  static int bootm_linux_fdt(int machid, bootm_headers_t *images)
 
 	fdt_initrd(*of_flat_tree, *initrd_start, *initrd_end, 1);
 
+#ifdef CONFIG_SAVE_BOOT_ARGS
+	printf("write FDT to NAND...\n");
+	boot_params_to_nand((u_char *)(*of_flat_tree),of_size);
+#endif
+
 	announce_and_cleanup();
 
 	kernel_entry(0, machid, *of_flat_tree);
diff --git a/include/configs/devkit8000.h b/include/configs/devkit8000.h
index 1bf6bea..3061fc2 100644
--- a/include/configs/devkit8000.h
+++ b/include/configs/devkit8000.h
@@ -345,4 +345,8 @@ 
 		                                         CONFIG_SYS_INIT_RAM_SIZE - \
 		                                         GENERATED_GBL_DATA_SIZE)
 
+/* Direct OS boot options */
+#define CONFIG_SAVE_BOOT_ARGS
+#define CONFIG_BOOT_ARGS_NAND_OFS			0x700000
+
 #endif /* __CONFIG_H */