Dear Andreas, On 08/25/2011 12:37 PM, Andreas Bießmann wrote: > Dear Simon, > > Am 25.08.2011 10:33, schrieb Simon Schwarz: >> This adds a savebp command to the u-boot. >> >> Related config: >> CONFIG_CMD_SAVEBP >> activate/deactivate the command >> CONFIG_CMD_SAVEBP_NAND_OFS >> Offset in NAND to use >> CONFIG_SYS_NAND_SPL_KERNEL_OFFS >> Offset in NAND of direct boot kernel image to use in SPL > > This is not used in the code ... why defining it then (here)?
Will move the description to the proper patch. > >> CONFIG_SYS_SPL_ARGS_ADDR >> Address where the kernel boot arguments are expected - this is >> normally RAM-start + 0x100 (on ARM) > > Well this is gd->bd->bi_boot_params (at least on ARM), so why don't you > use that parameter here? Because this is used in SPL where the bd struct is not fully initialized. > >> >> Signed-off-by: Simon Schwarz<simonschwarz...@gmail.com> >> --- >> >> V2 changes: >> CHG corrected bootm call. Now bootm is called with five parameters including >> Address of FDT in RAM. This fixes the hang on savebp fdt call. >> ADD debug output of the actual bootm parameter call >> CHG help message >> >> V3 changes: >> FIX added missing brackets >> --- >> common/Makefile | 1 + >> common/cmd_savebp.c | 149 >> ++++++++++++++++++++++++++++++++++++++++++ >> include/configs/devkit8000.h | 6 ++ > > where is the documentation? Last patch - will squash it. > >> 3 files changed, 156 insertions(+), 0 deletions(-) >> create mode 100644 common/cmd_savebp.c >> >> diff --git a/common/Makefile b/common/Makefile >> index 124a427..0b42968 100644 >> --- a/common/Makefile >> +++ b/common/Makefile >> @@ -158,6 +158,7 @@ COBJS-$(CONFIG_USB_STORAGE) += usb_storage.o >> endif >> COBJS-$(CONFIG_CMD_XIMG) += cmd_ximg.o >> COBJS-$(CONFIG_YAFFS2) += cmd_yaffs2.o >> +COBJS-$(CONFIG_CMD_SAVEBP) += cmd_savebp.o >> >> # others >> COBJS-$(CONFIG_DDR_SPD) += ddr_spd.o >> diff --git a/common/cmd_savebp.c b/common/cmd_savebp.c >> new file mode 100644 >> index 0000000..4091ccb >> --- /dev/null >> +++ b/common/cmd_savebp.c >> @@ -0,0 +1,149 @@ >> +/* Copyright (C) 2011 >> + * Corscience GmbH& Co. KG - Simon Schwarz<schw...@corscience.de> >> + * >> + * See file CREDITS for list of people who contributed to this >> + * project. >> + * >> + * 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 (at your option) any later version. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + * >> + * You should have received a copy of the GNU General Public License >> + * along with this program; if not, write to the Free Software >> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, >> + * MA 02111-1307 USA >> + */ >> + >> +#include<common.h> >> +#include<command.h> >> + >> +#define TYPE_FDT 0 >> +#define TYPE_ATAGS 1 > > should'nt this go into some header? How about enum here? > Will change. >> + >> +static inline int str2off(const char *p, loff_t *num) >> +{ >> + char *endptr; >> + >> + *num = simple_strtoull(p,&endptr, 16); >> + return *p != '\0'&& *endptr == '\0'; >> +} > > could be merged with the one in cmd_nand.c. Maybe move the one from > cmd_nand.c to some header? Hm, what would be the best place for that? It's at least useful for mmc and nand. common.h? mtd_common.h? > >> + >> +int do_savebp(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) >> +{ >> + loff_t offset; >> + int img_type = TYPE_ATAGS; >> + int ret = 0; >> + int bootm_argc = 5; >> + char *bootm_argsv[] = {"do_bootm", "xxxxxxx", "0x82000000", "-", >> + "0x80000100"}; >> + >> + offset = (loff_t)CONFIG_CMD_SAVEBP_NAND_OFS; >> + bootm_argsv[2] = getenv("loadaddr"); >> + /* - Validate args - */ >> + switch (argc) { >> + case 6: /* 4. fdt addr */ > ^ wrong number? Changed. > >> + if (strcmp(argv[5], "-")) >> + strcpy(bootm_argsv[4], argv[5]); > > If one set '-' as fifth argument bootm_argsv will stay with your given > "0x80000100" ... shouldn't the default argument be pre-calculated at > compile time. I guess the 0x80000100 is CONFIG_SYS_SDRAM_BASE + 0x100 in > your case (or tell it CONFIG_SYS_SPL_ARGS_ADDR). Will change. > > BTW: it is unsafe to use strcpy() here cause you may have some greater > string in 'src' than in 'dst' you should use strncpy() with strlen(dst) > as 'count' or even strdup() here. will do. > > In my opinion it would be best to use something like this here: > > ---8<--- > char *bootm_argsv[5]; > > bootm_argsv[0] = "bootm"; > ... > bootm_argsv[4] = strdup(argv[5]); > ... > --->8--- > > Or even > > bootm_argsv[4] = argv[5]; > > But that have to be investigated, I don't know currently if it will work. > Will think about... >> + case 5: /* 5. initrd addr */ > ^ wrong number? changed. > <snip> >> + } >> + /* call arch specific handlers */ >> + if (img_type == TYPE_FDT) >> + do_savebp_fdt(offset); >> + else >> + do_savebp_atags(offset); > > where are these two do_savebp_x() functions? later patch will change sequence. Regards, thanks for the feedback! Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot