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

[Tom Rini]

On Tue, Dec 29, 2020 at 06:07:08PM -0600, Alex G. 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.
> 
> 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.

Thanks for the feedback.  We need to make the tests easier to add then.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20201230/3b6d1f1b/attachment.sig>