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

[Simon Glass]

Hi Alex,

On Tue, 29 Dec 2020 at 17:07, Alex G. <mr.nuke.me at gmail.com> wrote:
>
>
>
> 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.

'make qcheck' will run it

But generally I run the tests directly.

>
> 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.

Sorry to hear that. The SPL tests are very new. I'll take a look at
adding some documentation for that in particular. If you had any
specific things you, or WIP code, please send it through.

One suggestion if this happens again, is to just spend an hour on it
and send out some questions, then start again when you get a response.
It isn't always possible but it would hopefully save you time.

Regards,
Simon