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