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

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

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