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

[Simon Glass]

HI Alex,

On Wed, 30 Dec 2020 at 19:57, Simon Glass <sjg at chromium.org> wrote:
>
> 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.

Just to follow up on this email, I have sent a v2 series with a simple
FIT test and some documentation updates. I hope it i useful.

Regards,
Simon