[U-Boot-Users] [PATCH v2] [new uImage] ppc: Re-order ramdisk/fdt handling sequence

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

Kumar Gala wrote:
> Doing the fdt before the ramdisk allows us to grow the fdt w/o concern
> however it does mean we have to go in and fixup the initrd info since
> we don't know where it will be.
> 
> Signed-off-by: Kumar Gala <galak@kernel.crashing.org>

Hi Kumar,

> ---
> 
> Fixed up the error handling.  The old code tried returning an error and
> should have called do_reset().
> 
>  lib_ppc/bootm.c |   40 +++++++++++++++++++++++++++++++++++-----
>  1 files changed, 35 insertions(+), 5 deletions(-)
> 
> diff --git a/lib_ppc/bootm.c b/lib_ppc/bootm.c
> index 5158ccc..09e349e 100644
> --- a/lib_ppc/bootm.c
> +++ b/lib_ppc/bootm.c
> @@ -71,7 +71,7 @@ do_bootm_linux(cmd_tbl_t *cmdtp, int flag,
>  	bd_t	*kbd;
>  	void	(*kernel)(bd_t *, ulong, ulong, ulong, ulong);
> 
> -	int	has_of = 0;
> +	int	ret, has_of = 0;

I would strongly prefer to see "ret" declared on a separate line.  I'm 
not a fan of comma declared variables and, when one is initialized and 
the other isn't, it makes my brain play tricks on me (thinking ret is 
initialized to 0 too, which it isn't).

> 
>  #if defined(CONFIG_OF_LIBFDT)
>  	char	*of_flat_tree;
> @@ -121,9 +121,6 @@ do_bootm_linux(cmd_tbl_t *cmdtp, int flag,
> 
>  	rd_len = rd_data_end - rd_data_start;
> 
> -	alloc_current = ramdisk_high (alloc_current, rd_data_start, rd_len,
> -			sp_limit, get_sp (), &initrd_start, &initrd_end);
> -
>  #if defined(CONFIG_OF_LIBFDT)
>  	/* find flattened device tree */
>  	alloc_current = get_fdt (alloc_current,
> @@ -134,7 +131,8 @@ do_bootm_linux(cmd_tbl_t *cmdtp, int flag,
>  	 * if the user wants it (the logic is in the subroutines).
>  	 */
>  	if (of_flat_tree) {
> -		if (fdt_chosen(of_flat_tree, initrd_start, initrd_end, 0) < 0) {
> +		/* pass in dummy initrd info, we'll fix up later */
> +		if (fdt_chosen(of_flat_tree, rd_data_start, rd_data_end, 0) < 0) {

If you pass in 0,0 for rd_data_start, rd_data_end it will not create the 
ramdisk entry...

>  			fdt_error ("/chosen node create failed");
>  			do_reset (cmdtp, flag, argc, argv);
>  		}
> @@ -157,6 +155,38 @@ do_bootm_linux(cmd_tbl_t *cmdtp, int flag,
>  	}
>  #endif	/* CONFIG_OF_LIBFDT */
> 
> +	alloc_current = ramdisk_high (alloc_current, rd_data_start, rd_len,
> +			sp_limit, get_sp (), &initrd_start, &initrd_end);
> +
> +#if defined(CONFIG_OF_LIBFDT)
> +	/* fixup the initrd now that we know where it should be */
> +	if ((of_flat_tree) && (initrd_start && initrd_end)) {
> +		uint64_t addr, size;
> +		int  total = fdt_num_mem_rsv(of_flat_tree);
> +		int  j;
> +
> +		/* Look for the dummy entry and delete it */
> +		for (j = 0; j < total; j++) {
> +			fdt_get_mem_rsv(of_flat_tree, j, &addr, &size);
> +			if (addr == rd_data_start) {
> +				fdt_del_mem_rsv(of_flat_tree, j);
> +				break;
> +			}
> +		}

...and the above loop and delete will then be unnecessary.

> +		ret = fdt_add_mem_rsv(of_flat_tree, initrd_start,
> +					initrd_end - initrd_start + 1);
> +		if (ret < 0) {
> +			printf("fdt_chosen: %s\n", fdt_strerror(ret));
> +			do_reset (cmdtp, flag, argc, argv);
> +		}
> +
> +		do_fixup_by_path_u32(of_flat_tree, "/chosen",
> +					"linux,initrd-start", initrd_start, 0);
> +		do_fixup_by_path_u32(of_flat_tree, "/chosen",
> +					"linux,initrd-end", initrd_end, 0);
> +	}
> +#endif
>  	debug ("## Transferring control to Linux (at address %08lx) ...\n",
>  		(ulong)kernel);
> 

...and you will have to pass in '1' for the create (last) parameter in 
the do_fixup_by_path_u32() calls to create them if they don't exist.

Is that better, or just a different way to do the same thing?  It seems 
more straight-forward to me.

Best regards,
gvb