[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