[PATCH 2/8] spl: fit: Factor out FIT parsing and use a context struct

Alex G.
2020-12-21T22:24:08Z

On 12/21/20 2:23 PM, Simon Glass wrote:
> Hi Alex,
>
> On Mon, 21 Dec 2020 at 12:28, Alex G. <mr.nuke.me@gmail.com> wrote:
>>
>> On 12/18/20 8:28 PM, Simon Glass wrote:
>>> Hi Alexandru,
>>>
>>> On Tue, 15 Dec 2020 at 17:09, Alexandru Gagniuc <mr.nuke.me@gmail.com> wrote:
>>>>
>>>> The logical steps in spl_load_simple_fit() are difficult to follow.
>>>> I think the long comments, ifdefs, and ungodly number of variables
>>>> seriously affect the readability. In particular, it violates section 6
>>>> of the coding style, paragraphs (3), and (4).
>>>>
>>>> The purpose of this patch is to improve the situation by
>>>> - Factoring out initialization and parsing to separate functions
>>>> - Reduce the number of variables by using a context structure
>>>> This change introduces no functional changes.
>>>>
>>>> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
>>>> ---
>>>> common/spl/spl_fit.c | 87 ++++++++++++++++++++++++++++++--------------
>>>> 1 file changed, 60 insertions(+), 27 deletions(-)
>>>
>>> This certainly looks a lot better although your email address does not
>>> inspire confidence...
>>
>> Don't worry. It doesn't bite.
>>
>>> Do you think you could look at creating a sandbox SPL test for this?
>>> It should be possible to write it in C, set up a bit of data, call
>>> your function and check the results.
>>
>> I can look at it. I can't promise anything though, since this is the
>> first time I heard of the sandbox. Maybe doc knows more.
>
> Yes, see doc/arch/sandbox.rst
>
> test/dm/Makefile shows that only one test file is enabled for SPL, but
> you can add more. Let me know if you need pointers.
>
> These aliases might help, if you build into /tmp/b/<board> :
>
> # Run a pytest on sandbox
> # $1: Name of test to run (optional, else run all)
>
> function pyt {
> test/py/test.py -B sandbox --build-dir /tmp/b/sandbox ${1:+"-k $1"}
> }
>
> # Run a pytest on sandbox_spl
> # $1: Name of test to run (optional, else run all SPL tests)
> function pytspl {
> local run=$1
>
> [[ -z "$run" ]] && run=spl
> test/py/test.py -B sandbox_spl --build-dir /tmp/b/sandbox_spl -k $run
> }

You're thinking way ahead of where I am. I know how to build a board,
but I've never used the test infrastructure. After some fumbling, I
figured I'd try sandbox_spl:

$ test/py/test.py -B sandbox_spl --bd sandbox_spl --build

As you can imagine, it kept complaining about SDL. I've never used
environment variables with Kbuild, so using NO_SPL=1 seems unnatural.
But then why would we need SDL for testing an SPL build anyway? 'swig'
was missing too, but that was an easy fix.

Second try:

$ NO_SDL=1 test/py/test.py -B sandbox_spl --bd sandbox_spl \
--build

Went a bit better, but " 29 failed, 502 passed, 212 skipped". Is this
normal?

What I seem to be missing is how to connect this python to calling
spl_load_simple_fit(). In the real world, I'd build u-boot and feed it a
FIT image -- boots, okay.

Alex

> Regards,
> Simon
>
>>
>> Alex
>>
>>>>
>>>> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
>>>> index 1b4a7f6b15..a6f85b6f9d 100644
>>>> --- a/common/spl/spl_fit.c
>>>> +++ b/common/spl/spl_fit.c
>>>> @@ -26,6 +26,12 @@ DECLARE_GLOBAL_DATA_PTR;
>>>> #define CONFIG_SYS_BOOTM_LEN (64 << 20)
>>>> #endif
>>>>
>>>> +struct spl_fit_info {
>>>> + const void *fit;
>>>> + size_t ext_data_offset;
>>>> + int images_node;
>>>> +};
>>>
>>> struct comments
>>>
>
> Regards,
> Simon
>