[U-Boot] [RFC PATCH 01/11] SPL: FIT: refactor FDT loading

André Przywara andre.przywara at arm.com
Sat Jan 28 01:38:00 CET 2017


On 27/01/17 21:29, Simon Glass wrote:
> Hi Andre,
> 
> On 19 January 2017 at 18:53, Andre Przywara <andre.przywara at arm.com> wrote:
>>
>> Currently the SPL FIT loader uses the spl_fit_select_fdt() function to
>> find the offset to the right DTB within the FIT image.
>> For this it iterates over all subnodes of the /configuration node in
>> the FIT tree and compares all "description" strings therein using a
>> board specific matching function.
>> If that finds a match, it uses the string in the "fdt" property of that
>> subnode to locate the matching subnode in the /images node, which points
>> to the DTB data.
>> Now this works very well, but is quite specific to cover this particular
>> use case. To open up the door for a more generic usage, let's split this
>> function into:
>> 1) a function that just returns the node offset for the matching
>>    configuration node (spl_fit_find_config_node())
>> 2) a function that returns the image data any given property in a given
>>    configuration node points to, additionally using a given index into
>>    a possbile list of strings (spl_fit_select_index())
>> This allows us to replace the specific function above by asking for the
>> image the _first string of the "fdt" property_ in the matching
>> configuration subnode points to.
>>
>> This patch introduces no functional changes, it just refactors the code
>> to allow reusing it later.
>>
>> (diff is overly clever here and produces a hard-to-read patch, so I
>> recommend to throw a look at the result instead).
>>
>> Signed-off-by: Andre Przywara <andre.przywara at arm.com>
>> ---
>>  common/spl/spl_fit.c | 82 ++++++++++++++++++++++++++++++++--------------------
>>  1 file changed, 51 insertions(+), 31 deletions(-)
>>
> 
> Reviewed-by: Simon Glass <sjg at chromium.org>

Thanks a lot!

> 
> Please check below.

....

>> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
>> index aae556f..c4e2f02 100644
>> --- a/common/spl/spl_fit.c
>> +++ b/common/spl/spl_fit.c
>> @@ -22,13 +22,11 @@ static ulong fdt_getprop_u32(const void *fdt, int node, const char *prop)
>>         return fdt32_to_cpu(*cell);
>>  }
>>
>> -static int spl_fit_select_fdt(const void *fdt, int images, int *fdt_offsetp)
>> +static int spl_fit_find_config_node(const void *fdt)
>>  {
>> -       const char *name, *fdt_name;
>> -       int conf, node, fdt_node;
>> -       int len;
>> +       const char *name;
>> +       int conf, node, len;
>>
>> -       *fdt_offsetp = 0;
>>         conf = fdt_path_offset(fdt, FIT_CONFS_PATH);
>>         if (conf < 0) {
>>                 debug("%s: Cannot find /configurations node: %d\n", __func__,
>> @@ -50,39 +48,60 @@ static int spl_fit_select_fdt(const void *fdt, int images, int *fdt_offsetp)
>>                         continue;
>>
>>                 debug("Selecting config '%s'", name);
>> -               fdt_name = fdt_getprop(fdt, node, FIT_FDT_PROP, &len);
>> -               if (!fdt_name) {
>> -                       debug("%s: Cannot find fdt name property: %d\n",
>> -                             __func__, len);
>> -                       return -EINVAL;
>> -               }
>>
>> -               debug(", fdt '%s'\n", fdt_name);
>> -               fdt_node = fdt_subnode_offset(fdt, images, fdt_name);
>> -               if (fdt_node < 0) {
>> -                       debug("%s: Cannot find fdt node '%s': %d\n",
>> -                             __func__, fdt_name, fdt_node);
>> -                       return -EINVAL;
>> +               return node;
>> +       }
>> +
>> +       return -1;
>> +}
>> +
>> +static int spl_fit_select_index(const void *fit, int images, int *offsetp,
>> +                               const char *type, int index)
>> +{
>> +       const char *name, *img_name;
>> +       int node, conf_node;
>> +       int len, i;
>> +
>> +       *offsetp = 0;
>> +       conf_node = spl_fit_find_config_node(fit);
>> +       if (conf_node < 0) {
>> +#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
>> +               printf("No matching DT out of these options:\n");
>> +               for (node = fdt_first_subnode(fit, conf_node);
>> +                    node >= 0;
>> +                    node = fdt_next_subnode(fit, node)) {
>> +                       name = fdt_getprop(fit, node, "description", &len);
>> +                       printf("   %s\n", name);
>>                 }
>> +#endif
>> +               return -ENOENT;
>> +       }
>>
>> -               *fdt_offsetp = fdt_getprop_u32(fdt, fdt_node, "data-offset");
>> -               len = fdt_getprop_u32(fdt, fdt_node, "data-size");
>> -               debug("FIT: Selected '%s'\n", name);
>> +       img_name = fdt_getprop(fit, conf_node, type, &len);
>> +       if (!img_name) {
>> +               debug("cannot find property '%s': %d\n", type, len);
>> +               return -EINVAL;
>> +       }
>>
>> -               return len;
>> +       for (i = 0; i < index; i++) {
>> +               img_name = strchr(img_name, '\0') + 1;
> 
> Don't you need to check against strchr() returning NULL?

I think in a valid DT a string is always terminated by a NUL, but you
are right that this code is broken. There is no double NUL at the end as
I erroneously thought, instead we have to compare against the "len"
value from above.

Thanks for spotting this!

Cheers,
Andre.

>> +               if (*img_name == '\0') {
>> +                       debug("no string for index %d\n", index);
>> +                       return -E2BIG;
>> +               }
>>         }
>>
>> -#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
>> -       printf("No matching DT out of these options:\n");
>> -       for (node = fdt_first_subnode(fdt, conf);
>> -            node >= 0;
>> -            node = fdt_next_subnode(fdt, node)) {
>> -               name = fdt_getprop(fdt, node, "description", &len);
>> -               printf("   %s\n", name);
>> +       debug("%s: '%s'\n", type, img_name);
>> +       node = fdt_subnode_offset(fit, images, img_name);
>> +       if (node < 0) {
>> +               debug("cannot find image node '%s': %d\n", img_name, node);
>> +               return -EINVAL;
>>         }
>> -#endif
>>
>> -       return -ENOENT;
>> +       *offsetp = fdt_getprop_u32(fit, node, "data-offset");
>> +       len = fdt_getprop_u32(fit, node, "data-size");
>> +
>> +       return len;
>>  }
>>
>>  static int get_aligned_image_offset(struct spl_load_info *info, int offset)
>> @@ -218,7 +237,8 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
>>         memcpy(dst, src, data_size);
>>
>>         /* Figure out which device tree the board wants to use */
>> -       fdt_len = spl_fit_select_fdt(fit, images, &fdt_offset);
>> +       fdt_len = spl_fit_select_index(fit, images, &fdt_offset,
>> +                                      FIT_FDT_PROP, 0);
>>         if (fdt_len < 0)
>>                 return fdt_len;
>>
>> --
>> 2.8.2
>>
> 
> Regards,
> Simon
> 



More information about the U-Boot mailing list