[U-Boot-Users] [PATCH] Add sub-commands to fdt

[Jerry Van Baren <gerald.vanbaren@ge.com>]

Kumar Gala wrote:
> fdt header                          - Display header info
> fdt bootcpu <id>                    - Set boot cpuid
> fdt memory <addr> <size>            - Add/Update memory node
> fdt memrsv print                    - Show current mem reserves
> fdt memrsv add    <addr> <size>     - Add a mem reserve
> fdt memrsv delete <index>           - Delete a mem reserves
> 
> Signed-off-by: Kumar Gala <galak@kernel.crashing.org>

Cool, procrastination pays off again!  :-)

> ---
>  common/cmd_fdt.c |  111 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 111 insertions(+), 0 deletions(-)
> 
> diff --git a/common/cmd_fdt.c b/common/cmd_fdt.c
> index 9cd22ee..d0f8c27 100644
> --- a/common/cmd_fdt.c
> +++ b/common/cmd_fdt.c
> @@ -296,6 +296,111 @@ int do_fdt (cmd_tbl_t * cmdtp, int flag, int argc, char *argv[])
>  				return err;
>  			}
>  		}
> +
> +	/********************************************************************
> +	 * Display header info
> +	 ********************************************************************/
> +	} else if (argv[1][0] == 'h') {
> +		u32 version = fdt_version(fdt);
> +		printf("magic:\t\t\t0x%x\n", fdt_magic(fdt));
> +		printf("totalsize:\t\t0x%x (%d)\n", fdt_totalsize(fdt), fdt_totalsize(fdt));
> +		printf("off_dt_struct:\t\t0x%x\n", fdt_off_dt_struct(fdt));
> +		printf("off_dt_strings:\t\t0x%x\n", fdt_off_dt_strings(fdt));
> +		printf("off_mem_rsvmap:\t\t0x%x\n", fdt_off_mem_rsvmap(fdt));
> +		printf("version:\t\t%d\n", version);
> +		printf("last_comp_version:\t%d\n", fdt_last_comp_version(fdt));
> +		if (version >= 2)
> +			printf("boot_cpuid_phys:\t0x%x\n",
> +				fdt_boot_cpuid_phys(fdt));
> +		if (version >= 3)
> +			printf("size_dt_strings:\t0x%x\n",
> +				fdt_size_dt_strings(fdt));
> +		if (version >= 17)
> +			printf("size_dt_struct:\t\t0x%x\n",
> +				fdt_size_dt_struct(fdt));
> +		printf("\n");

Very nice.
* I think we should add the size of the reserved map.
* It would be very useful to know how much space is available in the 
reserved map
* It would be very useful to know how much space is available in the 
dt_struct and dt_space space (I believe the "spare" in the blob can be 
used for both the dt_struct and the dt_strings, but I'm not sure, don't 
have time to look it up right now).

> +	/********************************************************************
> +	 * Set boot cpu id
> +	 ********************************************************************/
> +	} else if ((argv[1][0] == 'b') && (argv[1][1] == 'o')) {
> +		unsigned long tmp = simple_strtoul(argv[2], NULL, 16);
> +		fdt_set_boot_cpuid_phys(fdt, tmp);
> +
> +	/********************************************************************
> +	 * memory command
> +	 ********************************************************************/
> +	} else if ((argv[1][0] == 'm') && (argv[1][1] == 'e') &&
> +		   (argv[1][3] == 'o')) {

Did we miss (argv[1][2] == 'm'), is the term checking (argv[1][3] == 
'o') wrong, or are we trying to accommodate dyslecix people?  I can 
picture our fearless leader typing "fdt meromy", assuming he types that 
many characters. :-D

> +		uint64_t addr, size;
> +		int err;
> +#ifdef CFG_64BIT_STRTOUL
> +			addr = simple_strtoull(argv[2], NULL, 16);
> +			size = simple_strtoull(argv[3], NULL, 16);
> +#else
> +			addr = simple_strtoul(argv[2], NULL, 16);
> +			size = simple_strtoul(argv[3], NULL, 16);
> +#endif
> +		err = fdt_fixup_memory(fdt, addr, size);
> +		if (err < 0)
> +			return err;
> +
> +	/********************************************************************
> +	 * mem reserve commands
> +	 ********************************************************************/
> +	} else if ((argv[1][0] == 'm') && (argv[1][1] == 'e') &&
> +		   (argv[1][3] == 'r')) {

Hmm, missing (argv[1][2] == 'm') again.  Once is an accident, twice is 
by design.  I take it you are skipping the argv[1][2] check since it 
isn't a significant character - code savings.  I can accept that, 
although it really hurts my humor to have a logical explanation (pun 
intended ;-).

> +		if (argv[2][0] == 'p') {
> +			uint64_t addr, size;
> +			int total = fdt_num_mem_rsv(fdt);
> +			int j, err;
> +			printf("index\t\t   start\t\t    size\n");
> +			printf("-------------------------------"
> +				"-----------------\n");
> +			for (j = 0; j < total; j++) {
> +				err = fdt_get_mem_rsv(fdt, j, &addr, &size);
> +				if (err < 0) {
> +					printf("libfdt fdt_get_mem_rsv():  %s\n",
> +							fdt_strerror(err));
> +					return err;
> +				}
> +				printf("    %x\t%08x%08x\t%08x%08x\n", j,
> +					(u32)(addr >> 32),
> +					(u32)(addr & 0xffffffff),
> +					(u32)(size >> 32),
> +					(u32)(size & 0xffffffff));

Trivia: If the compiler supports CFG_64BIT_STRTOUL, it /probably/ 
supports the "%llx" format.  I ran into trouble with this originally 
(the compiler for my mpc8360 had problems passing a 64 bit varargs arg, 
IIRC).  The above works in both 32bit and 64bit support cases and I 
personally prefer the above slightly extra work rather than Yet Another 
#ifdef (YA#).

> +			}
> +		} else if (argv[2][0] == 'a') {
> +			uint64_t addr, size;
> +			int err;
> +#ifdef CFG_64BIT_STRTOUL

...but some are unavoidable.

> +			addr = simple_strtoull(argv[3], NULL, 16);
> +			size = simple_strtoull(argv[4], NULL, 16);
> +#else
> +			addr = simple_strtoul(argv[3], NULL, 16);
> +			size = simple_strtoul(argv[4], NULL, 16);
> +#endif

[snip]

>  #ifdef CONFIG_OF_BOARD_SETUP
>  	/* Call the board-specific fixup routine */
> @@ -689,6 +794,12 @@ U_BOOT_CMD(
>  	"fdt set    <path> <prop> [<val>]    - Set <property> [to <val>]\n"
>  	"fdt mknode <path> <node>            - Create a new node after <path>\n"
>  	"fdt rm     <path> [<prop>]          - Delete the node or <property>\n"
> +	"fdt header                          - Display header info\n"
> +	"fdt bootcpu <id>                    - Set boot cpuid\n"
> +	"fdt memory <addr> <size>            - Add/Update memory node\n"
> +	"fdt memrsv print                    - Show current mem reserves\n"
> +	"fdt memrsv add    <addr> <size>     - Add a mem reserve\n"
                         ^^^
 > +	"fdt memrsv add <addr> <size>        - Add a mem reserve\n"
It is a weakly held personal preference, but I would kill the extra 
spaces between add and <addr>, I don't see that it helps readability.

> +	"fdt memrsv delete <index>           - Delete a mem reserves\n"
>  	"fdt chosen - Add/update the /chosen branch in the tree\n"
>  #ifdef CONFIG_OF_HAS_UBOOT_ENV
>  	"fdt env    - Add/replace the /u-boot-env branch in the tree\n"

THANKS!
gvb