GNU bug report logs - #31599
[PATCH] system: Add u-boot-puma-rk3399.

Previous Next

Full log

Message #11 received at 31599 <at> debbugs.gnu.org (full text, mbox):

From: Vagrant Cascadian <vagrant <at> debian.org>
To: Danny Milosavljevic <dannym <at> scratchpost.org>
Cc: 31599 <at> debbugs.gnu.org
Subject: Re: [bug#31599] [PATCH] system: Add u-boot-puma-rk3399.
Date: Sat, 26 May 2018 09:46:04 -0700

[Message part 1 (text/plain, inline)]

On 2018-05-26, Danny Milosavljevic wrote:
> On Fri, 25 May 2018 15:29:40 -0700
> Vagrant Cascadian <vagrant <at> debian.org> wrote:
>> [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?

A couple hours after I submitted this a patch was submitted upstream
that fixes the issue:

  https://patchwork.ozlabs.org/patch/920785/


>>+                 ;; The u-boot.itb is not built by default
>
> ??? How can that be?  Isn't it required for booting?

There may be other implementations of boot firmware that consume the
various parts of u-boot and don't use u-boot.itb.

The Debian u-boot package, for example, does not yet have
arm-trusted-frimware or the cortex-m0 firmware available, so it needs to
build without it and let the user build the other components and
manually construct the u-boot.itb.


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

An earlier patch I did added the tools directory to PATH and used the
in-tree mkimage, but I opted for using the mkimage from u-boot-tools
when I submitted. Wasn't sure which was better.


> All they'd have to do is add 
>
>  ALL-y += u-boot.itb
>
> to the Makefile.  Does upstream know about it?

I'll bring the issue upstream; it may need to be added conditionally, as
not all u-boot targets support generating a u-boot.itb.


>>+                 (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")))) .

Ok.


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

Please forgive my ignorance, but I'm not sure where it belongs or even
why... just a rank beginner with this. :)


>>+      (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))

Will do.


> Sometimes the indentation you used is slightly off, like this:
>
>   `((foo)
>    (bar))
>
> It should be
>
>   `((foo)
>     (bar))

Will try to sort those out...


Thanks for the review!


live well,
  vagrant

[signature.asc (application/pgp-signature, inline)]

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.