[U-Boot] [PATCH V3 2/8] Add savebp command

[Simon Schwarz <simonschwarzcor@googlemail.com>]

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<simonschwarzcor@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<schwarz@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