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

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

[Kumar Gala]

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>
---

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;

 #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) {
 			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;
+			}
+		}
+
+		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);

-- 
1.5.3.8

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

[Jerry Van Baren]

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

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

[Kumar Gala]

On Feb 18, 2008, at 11:51 AM, Jerry Van Baren wrote:

> 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...

If rd_data_start, rd_data_end are 0,0 than we don't have a ramdisk, so  
its ok.

>> 			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.

Again, protected by initrd_start/_end being non-zero.

>> +		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.

They should exist by the time we are doing the fixup.

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

Its just re-ordering the way we do things.

- k

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

[Jerry Van Baren]

Kumar Gala wrote:
> 
> On Feb 18, 2008, at 11:51 AM, Jerry Van Baren wrote:
> 
>> 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...
> 
> If rd_data_start, rd_data_end are 0,0 than we don't have a ramdisk, so 
> its ok.

My point is to pass in 0,0 *so that* they are not created at all, rather 
than passing in dummy values, creating entries that we have to 
delete/rewrite later.

>>>             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.
> 
> Again, protected by initrd_start/_end being non-zero.

My point is to pass in 0,0 *so that* they are not created at all, rather 
than passing in dummy values, creating entries that we have to 
delete/rewrite later.

>>> +        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.
> 
> They should exist by the time we are doing the fixup.
> 
>> Is that better, or just a different way to do the same thing?  It 
>> seems more straight-forward to me.
> 
> Its just re-ordering the way we do things.
> 
> - k

The patch is creating dummy initrd entries in the reserved map and in 
/chosen, only to work hard to delete and re-create the reserved map 
entries and rewrite the /chosen entries.

My counter-proposal is to not bother with dummy values.  Simply pass in 
0,0 which will prevent the creation of the initrd entries by 
fdt_chosen().  By not creating dummy entries, you can simply create the 
proper entries once you know what the the correct values are, rather 
than the more complicated rsvmap search & delete + rsvmap creation + 
/chosen modifications.

Best regards,
gvb

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

[Kumar Gala]

On Feb 18, 2008, at 1:15 PM, Jerry Van Baren wrote:

> Kumar Gala wrote:
>> On Feb 18, 2008, at 11:51 AM, Jerry Van Baren wrote:
>>> 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...
>> If rd_data_start, rd_data_end are 0,0 than we don't have a ramdisk,  
>> so its ok.
>
> My point is to pass in 0,0 *so that* they are not created at all,  
> rather than passing in dummy values, creating entries that we have  
> to delete/rewrite later.
>
>>>>            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.
>> Again, protected by initrd_start/_end being non-zero.
>
> My point is to pass in 0,0 *so that* they are not created at all,  
> rather than passing in dummy values, creating entries that we have  
> to delete/rewrite later.
>
>>>> +        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.
>> They should exist by the time we are doing the fixup.
>>> Is that better, or just a different way to do the same thing?  It  
>>> seems more straight-forward to me.
>> Its just re-ordering the way we do things.
>> - k
>
> The patch is creating dummy initrd entries in the reserved map and  
> in /chosen, only to work hard to delete and re-create the reserved  
> map entries and rewrite the /chosen entries.
>
> My counter-proposal is to not bother with dummy values.  Simply pass  
> in 0,0 which will prevent the creation of the initrd entries by  
> fdt_chosen().  By not creating dummy entries, you can simply create  
> the proper entries once you know what the the correct values are,  
> rather than the more complicated rsvmap search & delete + rsvmap  
> creation + /chosen modifications.

Ahh, the reason I wanted them created was to ensure we have enough  
size for them up front rather than figuring that out later.  By  
creating them and replacing them I will not being changing the size at  
all.

- k

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

[Jerry Van Baren]

Kumar Gala wrote:
> 
> On Feb 18, 2008, at 1:15 PM, Jerry Van Baren wrote:
> 
>> Kumar Gala wrote:
>>> On Feb 18, 2008, at 11:51 AM, Jerry Van Baren wrote:
>>>> Kumar Gala wrote:

[[[[snip]]]]

>> The patch is creating dummy initrd entries in the reserved map and in 
>> /chosen, only to work hard to delete and re-create the reserved map 
>> entries and rewrite the /chosen entries.
>>
>> My counter-proposal is to not bother with dummy values.  Simply pass 
>> in 0,0 which will prevent the creation of the initrd entries by 
>> fdt_chosen().  By not creating dummy entries, you can simply create 
>> the proper entries once you know what the the correct values are, 
>> rather than the more complicated rsvmap search & delete + rsvmap 
>> creation + /chosen modifications.
> 
> Ahh, the reason I wanted them created was to ensure we have enough size 
> for them up front rather than figuring that out later.  By creating them 
> and replacing them I will not being changing the size at all.
> 
> - k

OK, I see.

Currently this isn't an issue because our blob has a fixed size that has 
free space inside it, so creating the rsvmap and /chosen entries eat at 
the internal free space and don't change the total blob size.

People are advocating dynamically increasing the blob size, which 
simplifies things for blob generation (don't have to guess how big to 
make the blob when running the dtc to create it), but that would cause 
problems with my counter-proposal.

Thanks,
gvb

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

[Kumar Gala]

On Feb 18, 2008, at 1:46 PM, Jerry Van Baren wrote:

> Kumar Gala wrote:
>> On Feb 18, 2008, at 1:15 PM, Jerry Van Baren wrote:
>>> Kumar Gala wrote:
>>>> On Feb 18, 2008, at 11:51 AM, Jerry Van Baren wrote:
>>>>> Kumar Gala wrote:
>
> [[[[snip]]]]
>
>>> The patch is creating dummy initrd entries in the reserved map and  
>>> in /chosen, only to work hard to delete and re-create the reserved  
>>> map entries and rewrite the /chosen entries.
>>>
>>> My counter-proposal is to not bother with dummy values.  Simply  
>>> pass in 0,0 which will prevent the creation of the initrd entries  
>>> by fdt_chosen().  By not creating dummy entries, you can simply  
>>> create the proper entries once you know what the the correct  
>>> values are, rather than the more complicated rsvmap search &  
>>> delete + rsvmap creation + /chosen modifications.
>> Ahh, the reason I wanted them created was to ensure we have enough  
>> size for them up front rather than figuring that out later.  By  
>> creating them and replacing them I will not being changing the size  
>> at all.
>> - k
>
> OK, I see.
>
> Currently this isn't an issue because our blob has a fixed size that  
> has free space inside it, so creating the rsvmap and /chosen entries  
> eat at the internal free space and don't change the total blob size.
>
> People are advocating dynamically increasing the blob size, which  
> simplifies things for blob generation (don't have to guess how big  
> to make the blob when running the dtc to create it), but that would  
> cause problems with my counter-proposal.

And the whole point of my patch was to enable the ability to  
dynamically grow the blob before we do anything w/the ramdisk.

- k

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

[Wolfgang Denk]

In message <8B11A571-3BC2-42BD-9B66-DF22A736A3A7@kernel.crashing.org> you wrote:
> 
> And the whole point of my patch was to enable the ability to  
> dynamically grow the blob before we do anything w/the ramdisk.

So what is the state of this patch now? Will it go in through the FDT
custodian repo, or has it been rejected, or am I supposed to apply it
directly? Or?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Eureka!                                                 -- Archimedes

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

[Jerry Van Baren]

Wolfgang Denk wrote:
> In message <8B11A571-3BC2-42BD-9B66-DF22A736A3A7@kernel.crashing.org> you wrote:
>> And the whole point of my patch was to enable the ability to  
>> dynamically grow the blob before we do anything w/the ramdisk.
> 
> So what is the state of this patch now? Will it go in through the FDT
> custodian repo, or has it been rejected, or am I supposed to apply it
> directly? Or?
> 
> Best regards,
> 
> Wolfgang Denk

Hi Wolfgang, Kumar,

Most of the email exchanges were with me and the patch is fine IMHO.

I was expecting it to go through the "new-image" custodian, but I would 
be happy to apply it to the u-boot-fdt repo and push it up from there.

Preferences, Kumar?

Best regards,
gvb

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

[Bartlomiej Sieka]

Jerry Van Baren wrote:
> Wolfgang Denk wrote:
>> In message <8B11A571-3BC2-42BD-9B66-DF22A736A3A7@kernel.crashing.org> you wrote:
>>> And the whole point of my patch was to enable the ability to  
>>> dynamically grow the blob before we do anything w/the ramdisk.
>> So what is the state of this patch now? Will it go in through the FDT
>> custodian repo, or has it been rejected, or am I supposed to apply it
>> directly? Or?
[...]
> Most of the email exchanges were with me and the patch is fine IMHO.
> 
> I was expecting it to go through the "new-image" custodian, but I would 
> be happy to apply it to the u-boot-fdt repo and push it up from there.

Jerry, Kumar,

I think it would be better to have this patch (and other ones by Kumar
that touch booting-related code) go through the new-image branch.
Otherwise we'll have merge conflicts down the road, especially since
we've got a big patch coming, that adds framework for new image
handling. I'd like to have this framework patch merged first, then take
care of (re-based) boot-related patches that have been posted recently.

Regards,
Bartlomiej

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

[Marian Balakowicz]

Kumar Gala wrote:
> On Feb 18, 2008, at 1:46 PM, Jerry Van Baren wrote:
> 
>> Kumar Gala wrote:
>>> On Feb 18, 2008, at 1:15 PM, Jerry Van Baren wrote:
>>>> Kumar Gala wrote:
>>>>> On Feb 18, 2008, at 11:51 AM, Jerry Van Baren wrote:
>>>>>> Kumar Gala wrote:
>> [[[[snip]]]]
>>
>>>> The patch is creating dummy initrd entries in the reserved map and  
>>>> in /chosen, only to work hard to delete and re-create the reserved  
>>>> map entries and rewrite the /chosen entries.
>>>>
>>>> My counter-proposal is to not bother with dummy values.  Simply  
>>>> pass in 0,0 which will prevent the creation of the initrd entries  
>>>> by fdt_chosen().  By not creating dummy entries, you can simply  
>>>> create the proper entries once you know what the the correct  
>>>> values are, rather than the more complicated rsvmap search &  
>>>> delete + rsvmap creation + /chosen modifications.
>>> Ahh, the reason I wanted them created was to ensure we have enough  
>>> size for them up front rather than figuring that out later.  By  
>>> creating them and replacing them I will not being changing the size  
>>> at all.
>>> - k
>> OK, I see.
>>
>> Currently this isn't an issue because our blob has a fixed size that  
>> has free space inside it, so creating the rsvmap and /chosen entries  
>> eat at the internal free space and don't change the total blob size.
>>
>> People are advocating dynamically increasing the blob size, which  
>> simplifies things for blob generation (don't have to guess how big  
>> to make the blob when running the dtc to create it), but that would  
>> cause problems with my counter-proposal.
> 
> And the whole point of my patch was to enable the ability to  
> dynamically grow the blob before we do anything w/the ramdisk.

But we don't really grow the blob, we are just allocating the space for
the initrd properties - *if* the blob already has enough free space. If
the blob does not have enough free space we'll hit the bottom anyway,
whether in fdt_chosen() or ft_board_setup(), so it seem that it doesn't
matter whether we pre-allocate space for initrd or not. Or am I missing
something?

I was rather thinking of increasing the total blob size when relocating
it. Currently relocation happens only when the blob is not within
BOOTMAPSZ region, so we would need to always relocate the blob and
figure out the size delta: (1) get it from env variable, if set (2) or
use some default delta. What do you think?

Cheers,
m.

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

[Marian Balakowicz]

Marian Balakowicz wrote:
> Kumar Gala wrote:
>> On Feb 18, 2008, at 1:46 PM, Jerry Van Baren wrote:
>>
>>> Kumar Gala wrote:
>>>> On Feb 18, 2008, at 1:15 PM, Jerry Van Baren wrote:
>>>>> Kumar Gala wrote:
>>>>>> On Feb 18, 2008, at 11:51 AM, Jerry Van Baren wrote:
>>>>>>> Kumar Gala wrote:
>>> [[[[snip]]]]
>>>
>>>>> The patch is creating dummy initrd entries in the reserved map and  
>>>>> in /chosen, only to work hard to delete and re-create the reserved  
>>>>> map entries and rewrite the /chosen entries.
>>>>>
>>>>> My counter-proposal is to not bother with dummy values.  Simply  
>>>>> pass in 0,0 which will prevent the creation of the initrd entries  
>>>>> by fdt_chosen().  By not creating dummy entries, you can simply  
>>>>> create the proper entries once you know what the the correct  
>>>>> values are, rather than the more complicated rsvmap search &  
>>>>> delete + rsvmap creation + /chosen modifications.
>>>> Ahh, the reason I wanted them created was to ensure we have enough  
>>>> size for them up front rather than figuring that out later.  By  
>>>> creating them and replacing them I will not being changing the size  
>>>> at all.
>>>> - k
>>> OK, I see.
>>>
>>> Currently this isn't an issue because our blob has a fixed size that  
>>> has free space inside it, so creating the rsvmap and /chosen entries  
>>> eat at the internal free space and don't change the total blob size.
>>>
>>> People are advocating dynamically increasing the blob size, which  
>>> simplifies things for blob generation (don't have to guess how big  
>>> to make the blob when running the dtc to create it), but that would  
>>> cause problems with my counter-proposal.
>> And the whole point of my patch was to enable the ability to  
>> dynamically grow the blob before we do anything w/the ramdisk.
> 
> But we don't really grow the blob, we are just allocating the space for
> the initrd properties - *if* the blob already has enough free space. If
> the blob does not have enough free space we'll hit the bottom anyway,
> whether in fdt_chosen() or ft_board_setup(), so it seem that it doesn't
> matter whether we pre-allocate space for initrd or not. Or am I missing
> something?
> 
> I was rather thinking of increasing the total blob size when relocating
> it. Currently relocation happens only when the blob is not within
> BOOTMAPSZ region, so we would need to always relocate the blob and
> figure out the size delta: (1) get it from env variable, if set (2) or
> use some default delta. What do you think?

Also, for the blob growing changes it would be nice to first merge your
LMB patches to sort out the memory allocation. I didn't have a time to
review your changes in detail but it's definitely step in the right
direction. Current bootm allocation is pretty messy, I cleaned it a bit
but didn't want to touch too many pieces at the same time. For the same
reason I would like to finish the current patch I am working on that
adds dual format framework before we add LMB and dynamic blob growing.

m.

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

[Jerry Van Baren]

Marian Balakowicz wrote:
> Kumar Gala wrote:
>> On Feb 18, 2008, at 1:46 PM, Jerry Van Baren wrote:
>>
>>> Kumar Gala wrote:
>>>> On Feb 18, 2008, at 1:15 PM, Jerry Van Baren wrote:
>>>>> Kumar Gala wrote:
>>>>>> On Feb 18, 2008, at 11:51 AM, Jerry Van Baren wrote:
>>>>>>> Kumar Gala wrote:
>>> [[[[snip]]]]
>>>
>>>>> The patch is creating dummy initrd entries in the reserved map and  
>>>>> in /chosen, only to work hard to delete and re-create the reserved  
>>>>> map entries and rewrite the /chosen entries.
>>>>>
>>>>> My counter-proposal is to not bother with dummy values.  Simply  
>>>>> pass in 0,0 which will prevent the creation of the initrd entries  
>>>>> by fdt_chosen().  By not creating dummy entries, you can simply  
>>>>> create the proper entries once you know what the the correct  
>>>>> values are, rather than the more complicated rsvmap search &  
>>>>> delete + rsvmap creation + /chosen modifications.
>>>> Ahh, the reason I wanted them created was to ensure we have enough  
>>>> size for them up front rather than figuring that out later.  By  
>>>> creating them and replacing them I will not being changing the size  
>>>> at all.
>>>> - k
>>> OK, I see.
>>>
>>> Currently this isn't an issue because our blob has a fixed size that  
>>> has free space inside it, so creating the rsvmap and /chosen entries  
>>> eat at the internal free space and don't change the total blob size.
>>>
>>> People are advocating dynamically increasing the blob size, which  
>>> simplifies things for blob generation (don't have to guess how big  
>>> to make the blob when running the dtc to create it), but that would  
>>> cause problems with my counter-proposal.
>> And the whole point of my patch was to enable the ability to  
>> dynamically grow the blob before we do anything w/the ramdisk.
> 
> But we don't really grow the blob, we are just allocating the space for
> the initrd properties - *if* the blob already has enough free space. If
> the blob does not have enough free space we'll hit the bottom anyway,
> whether in fdt_chosen() or ft_board_setup(), so it seem that it doesn't
> matter whether we pre-allocate space for initrd or not. Or am I missing
> something?
> 
> I was rather thinking of increasing the total blob size when relocating
> it. Currently relocation happens only when the blob is not within
> BOOTMAPSZ region, so we would need to always relocate the blob and
> figure out the size delta: (1) get it from env variable, if set (2) or
> use some default delta. What do you think?
> 
> Cheers,
> m.

The missing part is libfdt doesn't exactly support dynamic resizing and 
our current code doesn't do in-place resizing (which it could do by 
doing a move to the same location, but with a larger/smaller length).

Kumar is lining up the pieces to get there, but we aren't there yet...

gvb

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

[Marian Balakowicz]

Jerry Van Baren wrote:
> Marian Balakowicz wrote:
>> Kumar Gala wrote:
>>> On Feb 18, 2008, at 1:46 PM, Jerry Van Baren wrote:
>>>
>>>> Kumar Gala wrote:
>>>>> On Feb 18, 2008, at 1:15 PM, Jerry Van Baren wrote:
>>>>>> Kumar Gala wrote:
>>>>>>> On Feb 18, 2008, at 11:51 AM, Jerry Van Baren wrote:
>>>>>>>> Kumar Gala wrote:
>>>> [[[[snip]]]]
>>>>
>>>>>> The patch is creating dummy initrd entries in the reserved map
>>>>>> and  in /chosen, only to work hard to delete and re-create the
>>>>>> reserved  map entries and rewrite the /chosen entries.
>>>>>>
>>>>>> My counter-proposal is to not bother with dummy values.  Simply 
>>>>>> pass in 0,0 which will prevent the creation of the initrd entries 
>>>>>> by fdt_chosen().  By not creating dummy entries, you can simply 
>>>>>> create the proper entries once you know what the the correct 
>>>>>> values are, rather than the more complicated rsvmap search & 
>>>>>> delete + rsvmap creation + /chosen modifications.
>>>>> Ahh, the reason I wanted them created was to ensure we have enough 
>>>>> size for them up front rather than figuring that out later.  By 
>>>>> creating them and replacing them I will not being changing the
>>>>> size  at all.
>>>>> - k
>>>> OK, I see.
>>>>
>>>> Currently this isn't an issue because our blob has a fixed size
>>>> that  has free space inside it, so creating the rsvmap and /chosen
>>>> entries  eat at the internal free space and don't change the total
>>>> blob size.
>>>>
>>>> People are advocating dynamically increasing the blob size, which 
>>>> simplifies things for blob generation (don't have to guess how big 
>>>> to make the blob when running the dtc to create it), but that would 
>>>> cause problems with my counter-proposal.
>>> And the whole point of my patch was to enable the ability to 
>>> dynamically grow the blob before we do anything w/the ramdisk.
>>
>> But we don't really grow the blob, we are just allocating the space for
>> the initrd properties - *if* the blob already has enough free space. If
>> the blob does not have enough free space we'll hit the bottom anyway,
>> whether in fdt_chosen() or ft_board_setup(), so it seem that it doesn't
>> matter whether we pre-allocate space for initrd or not. Or am I missing
>> something?
>>
>> I was rather thinking of increasing the total blob size when relocating
>> it. Currently relocation happens only when the blob is not within
>> BOOTMAPSZ region, so we would need to always relocate the blob and
>> figure out the size delta: (1) get it from env variable, if set (2) or
>> use some default delta. What do you think?
>>

> The missing part is libfdt doesn't exactly support dynamic resizing and
> our current code doesn't do in-place resizing (which it could do by
> doing a move to the same location, but with a larger/smaller length).
> 
> Kumar is lining up the pieces to get there, but we aren't there yet...

I see, but how about resizing to a new location:

-   err = fdt_open_into (fdt_blob, (void *)of_start, of_len);
+   err = fdt_open_into (fdt_blob, (void *)of_start, of_len + delta);

Should that work?

If we add LMB and rework bootm memory allocation, putting things
(kernel, cmdline, kdb, initrd (optionally), fdt) in sequence starting
from bootm_low then we may want to always relocate fdt to avoid
overlapping. And, in case of new uImage FDT blob will be embedded in a
new uImage shell which is a blob itself. So, in this case in-place
resizing is not really a clean option, we would need to resize the
embedding new uImage blob first, and this one may have significant size,
so I suspect it may impact performance.

m.

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

[Kumar Gala]

On Feb 22, 2008, at 8:36 AM, Bartlomiej Sieka wrote:

> Jerry Van Baren wrote:
>> Wolfgang Denk wrote:
>>> In message <8B11A571-3BC2-42BD-9B66- 
>>> DF22A736A3A7 at kernel.crashing.org> you wrote:
>>>> And the whole point of my patch was to enable the ability to   
>>>> dynamically grow the blob before we do anything w/the ramdisk.
>>> So what is the state of this patch now? Will it go in through the  
>>> FDT
>>> custodian repo, or has it been rejected, or am I supposed to apply  
>>> it
>>> directly? Or?
> [...]
>> Most of the email exchanges were with me and the patch is fine IMHO.
>> I was expecting it to go through the "new-image" custodian, but I  
>> would be happy to apply it to the u-boot-fdt repo and push it up  
>> from there.
>
> Jerry, Kumar,
>
> I think it would be better to have this patch (and other ones by Kumar
> that touch booting-related code) go through the new-image branch.
> Otherwise we'll have merge conflicts down the road, especially since
> we've got a big patch coming, that adds framework for new image
> handling. I'd like to have this framework patch merged first, then  
> take
> care of (re-based) boot-related patches that have been posted  
> recently.

Agreed.  Was off having some fun for a few days.

I think going through the new image tree is best.

- k

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

[Kumar Gala]

On Feb 22, 2008, at 11:08 AM, Marian Balakowicz wrote:

> Jerry Van Baren wrote:
>> Marian Balakowicz wrote:
>>> Kumar Gala wrote:
>>>> On Feb 18, 2008, at 1:46 PM, Jerry Van Baren wrote:
>>>>
>>>>> Kumar Gala wrote:
>>>>>> On Feb 18, 2008, at 1:15 PM, Jerry Van Baren wrote:
>>>>>>> Kumar Gala wrote:
>>>>>>>> On Feb 18, 2008, at 11:51 AM, Jerry Van Baren wrote:
>>>>>>>>> Kumar Gala wrote:
>>>>> [[[[snip]]]]
>>>>>
>>>>>>> The patch is creating dummy initrd entries in the reserved map
>>>>>>> and  in /chosen, only to work hard to delete and re-create the
>>>>>>> reserved  map entries and rewrite the /chosen entries.
>>>>>>>
>>>>>>> My counter-proposal is to not bother with dummy values.  Simply
>>>>>>> pass in 0,0 which will prevent the creation of the initrd  
>>>>>>> entries
>>>>>>> by fdt_chosen().  By not creating dummy entries, you can simply
>>>>>>> create the proper entries once you know what the the correct
>>>>>>> values are, rather than the more complicated rsvmap search &
>>>>>>> delete + rsvmap creation + /chosen modifications.
>>>>>> Ahh, the reason I wanted them created was to ensure we have  
>>>>>> enough
>>>>>> size for them up front rather than figuring that out later.  By
>>>>>> creating them and replacing them I will not being changing the
>>>>>> size  at all.
>>>>>> - k
>>>>> OK, I see.
>>>>>
>>>>> Currently this isn't an issue because our blob has a fixed size
>>>>> that  has free space inside it, so creating the rsvmap and /chosen
>>>>> entries  eat at the internal free space and don't change the total
>>>>> blob size.
>>>>>
>>>>> People are advocating dynamically increasing the blob size, which
>>>>> simplifies things for blob generation (don't have to guess how big
>>>>> to make the blob when running the dtc to create it), but that  
>>>>> would
>>>>> cause problems with my counter-proposal.
>>>> And the whole point of my patch was to enable the ability to
>>>> dynamically grow the blob before we do anything w/the ramdisk.
>>>
>>> But we don't really grow the blob, we are just allocating the  
>>> space for
>>> the initrd properties - *if* the blob already has enough free  
>>> space. If
>>> the blob does not have enough free space we'll hit the bottom  
>>> anyway,
>>> whether in fdt_chosen() or ft_board_setup(), so it seem that it  
>>> doesn't
>>> matter whether we pre-allocate space for initrd or not. Or am I  
>>> missing
>>> something?
>>>
>>> I was rather thinking of increasing the total blob size when  
>>> relocating
>>> it. Currently relocation happens only when the blob is not within
>>> BOOTMAPSZ region, so we would need to always relocate the blob and
>>> figure out the size delta: (1) get it from env variable, if set  
>>> (2) or
>>> use some default delta. What do you think?
>>>
>
>> The missing part is libfdt doesn't exactly support dynamic resizing  
>> and
>> our current code doesn't do in-place resizing (which it could do by
>> doing a move to the same location, but with a larger/smaller length).
>>
>> Kumar is lining up the pieces to get there, but we aren't there  
>> yet...
>
> I see, but how about resizing to a new location:
>
> -   err = fdt_open_into (fdt_blob, (void *)of_start, of_len);
> +   err = fdt_open_into (fdt_blob, (void *)of_start, of_len + delta);
>
> Should that work?
>
> If we add LMB and rework bootm memory allocation, putting things
> (kernel, cmdline, kdb, initrd (optionally), fdt) in sequence starting
> from bootm_low then we may want to always relocate fdt to avoid
> overlapping. And, in case of new uImage FDT blob will be embedded in a
> new uImage shell which is a blob itself. So, in this case in-place
> resizing is not really a clean option, we would need to resize the
> embedding new uImage blob first, and this one may have significant  
> size,
> so I suspect it may impact performance.

I felt the sequence (on PPC) is either:
kernel, cmdline, kdb, initrd

or

kernel, fdt, initrd

The reason being is that initrd doesn't need to be constrained to  
BOOTMAPSZ but cmdline, kdb, and fdt would/should be.

- k

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

[Marian Balakowicz]

Kumar Gala wrote:
> 
> On Feb 22, 2008, at 11:08 AM, Marian Balakowicz wrote:
...
>>
>> If we add LMB and rework bootm memory allocation, putting things
>> (kernel, cmdline, kdb, initrd (optionally), fdt) in sequence starting
>> from bootm_low then we may want to always relocate fdt to avoid
>> overlapping. And, in case of new uImage FDT blob will be embedded in a
>> new uImage shell which is a blob itself. So, in this case in-place
>> resizing is not really a clean option, we would need to resize the
>> embedding new uImage blob first, and this one may have significant size,
>> so I suspect it may impact performance.
> 
> I felt the sequence (on PPC) is either:
> kernel, cmdline, kdb, initrd
> 
> or
> 
> kernel, fdt, initrd
> 
> The reason being is that initrd doesn't need to be constrained to
> BOOTMAPSZ but cmdline, kdb, and fdt would/should be.

That's right.

My point was just to have two steps:

1) Move all the stuff to the final locations (whatever the sequence)

- kernel

- fdt - *always* relocate to within BOOTMAPSZ, increase size dynamically.

fdt blob can be delivered using of the three different methods: (1)
raw fdt blob, (2) fdt blob embedded in legacy format uImage, (3) fdt
blob embedded in new uImage format. To simplify things always relocate
it to a allocated spot within BOOTMAPSZ.

- initrd - within or outside of the BOOTMAPSZ boundaries

2) Update fdt blob, knowing we have enough free blob space: set initrd
params, other fixups, etc.


With such approach we don't need special treatment for
initrd,start/initrd,end (and other fixups). We assure that there is
enough free space in fdt blob when we relocate it.

m.

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

[Kumar Gala]

On Feb 26, 2008, at 3:11 AM, Marian Balakowicz wrote:

> Kumar Gala wrote:
>>
>> On Feb 22, 2008, at 11:08 AM, Marian Balakowicz wrote:
> ...
>>>
>>> If we add LMB and rework bootm memory allocation, putting things
>>> (kernel, cmdline, kdb, initrd (optionally), fdt) in sequence  
>>> starting
>>> from bootm_low then we may want to always relocate fdt to avoid
>>> overlapping. And, in case of new uImage FDT blob will be embedded  
>>> in a
>>> new uImage shell which is a blob itself. So, in this case in-place
>>> resizing is not really a clean option, we would need to resize the
>>> embedding new uImage blob first, and this one may have significant  
>>> size,
>>> so I suspect it may impact performance.
>>
>> I felt the sequence (on PPC) is either:
>> kernel, cmdline, kdb, initrd
>>
>> or
>>
>> kernel, fdt, initrd
>>
>> The reason being is that initrd doesn't need to be constrained to
>> BOOTMAPSZ but cmdline, kdb, and fdt would/should be.
>
> That's right.
>
> My point was just to have two steps:
>
> 1) Move all the stuff to the final locations (whatever the sequence)
>
> - kernel
>
> - fdt - *always* relocate to within BOOTMAPSZ, increase size  
> dynamically.
>
> fdt blob can be delivered using of the three different methods: (1)
> raw fdt blob, (2) fdt blob embedded in legacy format uImage, (3) fdt
> blob embedded in new uImage format. To simplify things always relocate
> it to a allocated spot within BOOTMAPSZ.
>
> - initrd - within or outside of the BOOTMAPSZ boundaries
>
> 2) Update fdt blob, knowing we have enough free blob space: set initrd
> params, other fixups, etc.
>
>
> With such approach we don't need special treatment for
> initrd,start/initrd,end (and other fixups). We assure that there is
> enough free space in fdt blob when we relocate it.

How we get the fdt blob doesn't matter as much as its size.  At this  
point we still don't know how large it might need to be. I think the  
changes I made make sense in that we'd want to process the majority of  
the fdt before we do anything w/the initrd.

We can cleanup the fixup once we have better support for dealing w/the  
sizing of the fdt.

- k

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

[Marian Balakowicz]

Kumar Gala wrote:
> 
> On Feb 26, 2008, at 3:11 AM, Marian Balakowicz wrote:
> 
>> Kumar Gala wrote:
>>>
>>> On Feb 22, 2008, at 11:08 AM, Marian Balakowicz wrote:
>> ...
>>>>
>>>> If we add LMB and rework bootm memory allocation, putting things
>>>> (kernel, cmdline, kdb, initrd (optionally), fdt) in sequence starting
>>>> from bootm_low then we may want to always relocate fdt to avoid
>>>> overlapping. And, in case of new uImage FDT blob will be embedded in a
>>>> new uImage shell which is a blob itself. So, in this case in-place
>>>> resizing is not really a clean option, we would need to resize the
>>>> embedding new uImage blob first, and this one may have significant
>>>> size,
>>>> so I suspect it may impact performance.
>>>
>>> I felt the sequence (on PPC) is either:
>>> kernel, cmdline, kdb, initrd
>>>
>>> or
>>>
>>> kernel, fdt, initrd
>>>
>>> The reason being is that initrd doesn't need to be constrained to
>>> BOOTMAPSZ but cmdline, kdb, and fdt would/should be.
>>
>> That's right.
>>
>> My point was just to have two steps:
>>
>> 1) Move all the stuff to the final locations (whatever the sequence)
>>
>> - kernel
>>
>> - fdt - *always* relocate to within BOOTMAPSZ, increase size dynamically.
>>
>> fdt blob can be delivered using of the three different methods: (1)
>> raw fdt blob, (2) fdt blob embedded in legacy format uImage, (3) fdt
>> blob embedded in new uImage format. To simplify things always relocate
>> it to a allocated spot within BOOTMAPSZ.
>>
>> - initrd - within or outside of the BOOTMAPSZ boundaries
>>
>> 2) Update fdt blob, knowing we have enough free blob space: set initrd
>> params, other fixups, etc.
>>
>>
>> With such approach we don't need special treatment for
>> initrd,start/initrd,end (and other fixups). We assure that there is
>> enough free space in fdt blob when we relocate it.
> 
> How we get the fdt blob doesn't matter as much as its size.  At this
> point we still don't know how large it might need to be. I think the
> changes I made make sense in that we'd want to process the majority of
> the fdt before we do anything w/the initrd.

Correct me if I am wrong but my understanding is that it will not help
if the total size of the blob is too small.

It will just use/allocate free internal blob space - if the internal
free space is sufficient. And if it is it can be used at any time. If
it is not sufficient, then we need to grow the blob total size first.

m.

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

[Kumar Gala]

On Feb 27, 2008, at 5:20 AM, Marian Balakowicz wrote:

> Kumar Gala wrote:
>>
>> On Feb 26, 2008, at 3:11 AM, Marian Balakowicz wrote:
>>
>>> Kumar Gala wrote:
>>>>
>>>> On Feb 22, 2008, at 11:08 AM, Marian Balakowicz wrote:
>>> ...
>>>>>
>>>>> If we add LMB and rework bootm memory allocation, putting things
>>>>> (kernel, cmdline, kdb, initrd (optionally), fdt) in sequence  
>>>>> starting
>>>>> from bootm_low then we may want to always relocate fdt to avoid
>>>>> overlapping. And, in case of new uImage FDT blob will be  
>>>>> embedded in a
>>>>> new uImage shell which is a blob itself. So, in this case in-place
>>>>> resizing is not really a clean option, we would need to resize the
>>>>> embedding new uImage blob first, and this one may have significant
>>>>> size,
>>>>> so I suspect it may impact performance.
>>>>
>>>> I felt the sequence (on PPC) is either:
>>>> kernel, cmdline, kdb, initrd
>>>>
>>>> or
>>>>
>>>> kernel, fdt, initrd
>>>>
>>>> The reason being is that initrd doesn't need to be constrained to
>>>> BOOTMAPSZ but cmdline, kdb, and fdt would/should be.
>>>
>>> That's right.
>>>
>>> My point was just to have two steps:
>>>
>>> 1) Move all the stuff to the final locations (whatever the sequence)
>>>
>>> - kernel
>>>
>>> - fdt - *always* relocate to within BOOTMAPSZ, increase size  
>>> dynamically.
>>>
>>> fdt blob can be delivered using of the three different methods: (1)
>>> raw fdt blob, (2) fdt blob embedded in legacy format uImage, (3) fdt
>>> blob embedded in new uImage format. To simplify things always  
>>> relocate
>>> it to a allocated spot within BOOTMAPSZ.
>>>
>>> - initrd - within or outside of the BOOTMAPSZ boundaries
>>>
>>> 2) Update fdt blob, knowing we have enough free blob space: set  
>>> initrd
>>> params, other fixups, etc.
>>>
>>>
>>> With such approach we don't need special treatment for
>>> initrd,start/initrd,end (and other fixups). We assure that there is
>>> enough free space in fdt blob when we relocate it.
>>
>> How we get the fdt blob doesn't matter as much as its size.  At this
>> point we still don't know how large it might need to be. I think the
>> changes I made make sense in that we'd want to process the majority  
>> of
>> the fdt before we do anything w/the initrd.
>
> Correct me if I am wrong but my understanding is that it will not help
> if the total size of the blob is too small.
>
> It will just use/allocate free internal blob space - if the internal
> free space is sufficient. And if it is it can be used at any time. If
> it is not sufficient, then we need to grow the blob total size first.

At this point in time you are correct.  However I'm looking forward to  
having the possibility of growing things w/regards to the fdt.

- k