[U-Boot] [PATCH V3 2/8] Add savebp command
Simon Schwarz
simonschwarzcor at googlemail.com
Fri Aug 26 11:35:32 CEST 2011
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 at 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 at 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
More information about the U-Boot
mailing list