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

[Alex G.]

On 12/28/20 9:33 PM, Simon Glass wrote:
> Hi Alex,
> 
> On Mon, 21 Dec 2020 at 15:24, Alex G. <mr.nuke.me at gmail.com> wrote:
>>
>>
>>
>> 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 at 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 at 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 at 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.
> 
> Here's a suggestoin
> - Write a function that calls the function to load a fit and does some
> checks that it worked correct, e.g. by looking in memory
> - put a call to that function in an SPL C test (as mentioned ealler)
> 
> I suppose you could also boot it, perhaps by switching sandbox to use
> FIT to boot?

Hi Simon,

There seems to be a lot more to wrapping around spl_load_simple_fit(). 
We need populated spl_image_info spl_load_info structure. I'm not even 
sure if the test code runs in SPL, or how to run it in SPL.

There are examples, and unfocused documentation on how to connect this 
into u-boot proper. The current documentation and exampples are not 
helping with what I was trying to accomplish. Unfortunately, I've spent 
a week on this, and wasn't able to make any progress. I'm one guy who's 
getting paid to ship a product. This test infrastructure is more tedious 
than I anticipated, and I need to move on.

ALex