[U-Boot] [PATCH 02/17] SPL: FIT: refactor FDT loading

[Andre Przywara]

Hi Simon,

On 03/03/17 04:53, Simon Glass wrote:
> Hi Andre,
> 
> On 28 February 2017 at 19:25, 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).
> 
> First I want to commend you on your excellent commit messages. For
> example this one explains the current situation, the change your
> commit performs and the motivation for that change. With these more
> complicated / core pieces, it is very valuable and you are an example
> to us all :-)

Thank you very much, you made my day. That is a welcome departure from
the usual Linux ML communication style ;-)

And yes, will fix those things you mentioned below, though have to wrap
my mind about pytest first.


Now back into the rough waters of the Linux mailing lists ...

Cheers,
Andre.

> 
>>
>> Signed-off-by: Andre Przywara <andre.przywara at arm.com>
>> ---
>>  common/spl/spl_fit.c | 83 ++++++++++++++++++++++++++++++++--------------------
>>  1 file changed, 52 insertions(+), 31 deletions(-).
> 
> Reviewed-by: Simon Glass <sjg at chromium.org>
> 
> I think we need a pytest for this somewhere in this series. With
> sandbox_spl we can run spl/u-boot-spl and it jumps to u-boot. Can we
> use this to check that the right thing happens in a few simple cases?
> 
>>
>> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
>> index aae556f..ba45e65 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)
> 
> Can you comment this function please? I should have done this myself.
> 
>>  {
>> -       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,61 @@ 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;
> 
> How about -ENOENT?
> 
>> +}
>> +
>> +static int spl_fit_select_index(const void *fit, int images, int *offsetp,
>> +                               const char *type, int index)
> 
> And this one.
>