Hi,

On Fri, 25 May 2018 15:29:40 -0700
Vagrant Cascadian <vagrant@debian.org> wrote:

> Tested running on a puma-rk3399-haikou board running GuixSD!

Cool!

> [fdtfile u-boot variable needs to manually be set at boot]
>This is likely to be fixed in future u-boot versions.

Does upstream know about it?

>+                 ;; The u-boot.itb is not built by default

??? How can that be?  Isn't it required for booting?

I checked the source code - apparently they use mkimage
to build the itb from the its.  So now we are using two
"different" mkimage tools.  OK I guess - but weird.

All they'd have to do is add 

 ALL-y += u-boot.itb

to the Makefile.  Does upstream know about it?

>+                 (zero? (apply system* "make" `(,@make-flags ,"u-boot.itb")))))

Please use "invoke". It's shorthand for (zero? (system* ...)) but it also
raises an exception on error.
That's easier to maintain (when people add a second invocation they
don't have to add "(and ...)").

So here it would be (apply invoke "make" `(,@make-flags ,"u-boot.itb")))) .

>(add-after 'unpack 'set-environment
>+               (lambda* (#:key inputs #:allow-other-keys)
>+                 ;; Need to copy the firmware into u-boot build
>+                 ;; directory.
>+                 (copy-file (string-append (assoc-ref inputs "firmware")
>+                                           "/bl31.bin") "bl31-rk3399.bin")
>+                 (copy-file (string-append (assoc-ref inputs "firmware-m0")
>+                                           "/rk3399m0.bin") "rk3399m0.bin")))

Please end this phase with "#t".

>+      (version (string-append "1.3-" revision "." (string-take commit 7)))

Please use (git-version "1.3" revision commit) instead of 

(string-append "1.3-" revision "." (string-take commit 7))

> [...]

Sometimes the indentation you used is slightly off, like this:

  `((foo)
   (bar))

It should be

  `((foo)
    (bar))