[PATCH] system: Add u-boot-puma-rk3399.

Done

Details

2 participants

Danny Milosavljevic
Vagrant Cascadian

Owner: unassigned

Submitted by: Vagrant Cascadian

Severity: normal

Vagrant Cascadian wrote on 26 May 2018 00:29

Recipients:(address . guix-patches@gnu.org)

Message-ID:878t87s71n.fsf@aikidev.net

The attached patch enables u-boot for puma-rk3399. It enables a build of

the vendor's arm-trusted-firmware/rk3399-cortex-m0, and the installation

offsets to install the u-boot to microSD.

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

There is one glitch, which is that the fdtfile variable in u-boot

(rockchip/rk3399-puma-ddr1600.dtb) does not match the device-tree

present in linux-libre (rockchip/rk3399-puma-haikou.dtb), so the fdtfile

u-boot variable needs to manually be set at boot to match

linux-libre. This is likely to be fixed in future u-boot versions.

Vagrant Cascadian (1):

system: Add u-boot-puma-rk3399.

gnu/bootloader/u-boot.scm | 16 +++++++++++

gnu/packages/bootloaders.scm | 36 ++++++++++++++++++++++++-

gnu/packages/firmware.scm | 64 ++++++++++++++++++++++++++++++++++++++++++++

gnu/system/install.scm | 6 +++++

4 files changed, 121 insertions(+), 1 deletion(-)

2.11.0

Attachment: 0001-system-Add-u-boot-puma-rk3399.patch

Danny Milosavljevic wrote on 26 May 2018 09:19

Recipients:(name . Vagrant Cascadian)(address . vagrant@debian.org)(address . 31599@debbugs.gnu.org)

Message-ID:20180526091953.2e81f92f@scratchpost.org

Hi,

On Fri, 25 May 2018 15:29:40 -0700

Vagrant Cascadian <vagrant@debian.org> wrote:

Toggle quote (2 lines)

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

Cool!

Toggle quote (3 lines)

> [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?

Toggle quote (2 lines)

>+ ;; 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?

Toggle quote (2 lines)

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

Toggle quote (9 lines)

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

Toggle quote (2 lines)

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

Toggle quote (2 lines)

> [...]

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

`((foo)

(bar))

It should be

`((foo)

(bar))

-----BEGIN PGP SIGNATURE-----

iQEzBAEBCAAdFiEEds7GsXJ0tGXALbPZ5xo1VCwwuqUFAlsJCpkACgkQ5xo1VCww

uqXxHwgAkA5gR/OXX9rgKadfnG5E7dv8+jZzXjVyC6iS3iByC/NCbv5u1vCe0u08

tNs4UAecQ2TdIewokzBJPlq38ExpN4wzGQx3coxJWKBsJrGvwwVXTqI3oIK+g9pv

sQwjm/2ejQlbRJY283FUAdOlabBizDxhqAJpxPd3t7OnzN7x/YG7GWe4b4vH/A8p

wawkKjDDhVhlhfjwz4/iGJ47cBbP4FEgKH05WyyB5gW9inbFh7Wc+zNsjKZS8kw1

6Ew75LdUaI58bNiRu+pBWqMh4RLEYxqd0DEz+IRPMkMfQIWOTK0H72adlvBbMH1h

r2VQKIXSQJnbyX9Wt8Q6BqFjFixKGw==

=0giQ

-----END PGP SIGNATURE-----

Vagrant Cascadian wrote on 26 May 2018 18:46

Recipients:(name . Danny Milosavljevic)(address . dannym@scratchpost.org)(address . 31599@debbugs.gnu.org)

Message-ID:87603as6ur.fsf@aikidev.net

On 2018-05-26, Danny Milosavljevic wrote:

Toggle quote (7 lines)

> On Fri, 25 May 2018 15:29:40 -0700

> Vagrant Cascadian <vagrant@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/

Toggle quote (4 lines)

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

Toggle quote (4 lines)

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

Toggle quote (6 lines)

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

Toggle quote (9 lines)

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

Toggle quote (11 lines)>>(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. :)

Toggle quote (6 lines)

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

Toggle quote (10 lines)> 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

Danny Milosavljevic wrote on 26 May 2018 19:12

Recipients:(name . Vagrant Cascadian)(address . vagrant@debian.org)(address . 31599@debbugs.gnu.org)

Message-ID:20180526191202.268bd0e6@scratchpost.org

Toggle quote (5 lines)

> A couple hours after I submitted this a patch was submitted upstream

> that fixes the issue:

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

Good.

Toggle quote (12 lines)> >>+                 ;; 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 see. OK, I'm fine with building u-boot.itb manually for now. I just asked

because when we aren't careful we get into whack-a-mole quickly.

(There's a huge amount of ARM boards and we shouldn't do the u-boot maintainer's

job for them. It's more work *and* eventually we'd diverge from each other)

Toggle quote (4 lines)

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

Right now, they are the same because u-boot-tools derives from u-boot in Guix.

That said, for the sake of modularization I'd use the package-local mkimage

if there is one. One reason is that there are weird u-boot forks for some

boards (mainline u-boot doesn't work for them). These might have local

quirks in mkimage and it's better to track these quirks when building that

u-boot.

(As long as we know the above in the back of our heads as a potential source

of problems it doesn't matter much which mkimage we use in practice)

u-boot-tools is meant for the end user. I didn't intend it to be used

by the u-boots themselves (not that it's bad--but it's redundant right now).

Toggle quote (13 lines)> >>(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. :)

Earlier guile didn't support exceptions--so phases have to return a boolean

(whether the phase succeeded or not).

In a lambda, the return value is the value of the last expression.

There's an effort in core-updates to get it all up-to-speed so that these

all actually return a boolean--and long term we'd get rid of this requirement.

Some procedures are written especially for phases ("invoke" is, for example)

and do what you'd expect (return #t if OK, otherwise raise exception).

Other procedures you have to be careful what they actually return.

In this case, copy-file's documentation[1] states "The return value is

unspecified" (that implies that error handling is via exceptions).

The unspecified return value is clearly not good.

[1] https://www.gnu.org/software/guile/manual/html_node/File-System.html

-----BEGIN PGP SIGNATURE-----

iQEzBAEBCAAdFiEEds7GsXJ0tGXALbPZ5xo1VCwwuqUFAlsJlWIACgkQ5xo1VCww

uqXrowf+LXuD/AGR8m+i1Yf2eMkoYrHc1+40BFy3tNW3xbldodw/O+LJ4TPc0FUl

QaTLczZeRSCBui6/LROtfRtUvw4mwlR1XabKNBNbvtYP5oSEKCF3n8xnlgOa7yRS

fI9EoxOl6QHKrvDVyBtHvhgx93t1sXHORzQj/WAXr70wPWvLWnC4PfDdjtwyab2I

Ujad6W81osSi/g1VjSoYJ7AVwe6I4+eqclqkDcBwRgyhWy9bnMuZHK0arcJZI0FI

kQ3ZWW2oWLmWjilWcZbGOB6xoei8/9HMJ9AdjXfyCtidmrC/dzZBo1VpURo3v6Th

hDkXu310LJsWpfjhChKQym7l4KBAhA==

=cLfg

-----END PGP SIGNATURE-----

Vagrant Cascadian wrote on 29 May 2018 06:10

Re: [bug#31599] [PATCH v2] system: Add u-boot-puma-rk3399.

Recipients:(name . Danny Milosavljevic)(address . dannym@scratchpost.org)(address . 31599@debbugs.gnu.org)

Message-ID:87sh6bqezc.fsf@aikidev.net

Thanks for the review!

I think the attached patch addresses the issues raised:

* Fix several indentation and numerous whitespace issues.

* u-boot-puma-rk3399: Drop use of u-boot-tools as an input, calling

./tools/mkimage directly.

* arm-trusted-firmware-puma-rk3399: Use git-version to set version.

* u-boot-puma-rk3399: Complete block using #t

* u-boot-puma-rk3399: Use "apply invoke" to make system calls.

live well,

vagrant

Attachment: 0001-system-Add-u-boot-puma-rk3399.patch

Danny Milosavljevic wrote on 29 May 2018 10:51

Recipients:(name . Vagrant Cascadian)(address . vagrant@debian.org)(address . 31599-done@debbugs.gnu.org)

Message-ID:20180529105140.5abe89f0@scratchpost.org

On Mon, 28 May 2018 21:10:15 -0700

Vagrant Cascadian <vagrant@debian.org> wrote:

Toggle quote (3 lines)

> + (apply invoke "./tools/mkimage"

> + `(,"-T" ,"rksd" ,"-n" ,"rk3399" ,"-d" ,"spl/u-boot-spl.bin" ,"u-boot-spl.rksd")

Heh, this is neutral. So

(invoke "./tools/mkimage" "-T" "rksd" "-n" "rk3399" "-d" "spl/u-boot-spl.bin" "u-boot-spl.rksd")

works fine. So I've changed it to that.

I've pushed it to guix master.

Let's see what our aarch64 machines say about the cross-compilation (probably doesn't work yet - but meh).

-----BEGIN PGP SIGNATURE-----

iQEzBAEBCAAdFiEEds7GsXJ0tGXALbPZ5xo1VCwwuqUFAlsNFJwACgkQ5xo1VCww

uqWE1wf+INhL7Ga/MDVuCrLvIL5IwRSGYJaCs9ysIvoc40HB3yejeUMTGJwJOPUz

OqJCoA72t1+D06OiDn9cafijvwdTiVdJMH/PoTKgA6g3PilbmwS1iF/xwrRwwc4X

+/LoosC26VzdYRxkms9xcXbE6QG3J5LiudDXG/CxNbrTFf8MuYZEuNoD4znfV8X0

chpGlIMEsqKeb1C5hWr3zZO/DNTJgjVEjZhG05SdtLr9BTVBfmN3gFrLHWUsGzgg

AKnRQSoUSwMceLLMOP5gPIgUB1Pwk2gDqAY1lRe1zOx8OcrPSSBeJSII8ufoVKf5

fWiB12frHLvtujYSTmtZJFYl+G/EBA==

=yFD6

-----END PGP SIGNATURE-----

Closed

Danny Milosavljevic wrote on 17 Jun 2018 14:15

Recipients:(name . Vagrant Cascadian)(address . vagrant@debian.org)(address . 31599-done@debbugs.gnu.org)

Message-ID:20180617141539.1016a9cd@scratchpost.org

Hi Vagrant,

could you send a patch adding puma-rk3399 to buildroot?

I'm thinking of using buildroot for the u-boot installation in GuixSD

instead of our DIY installers (in the long term).

Puma-rk3399 is one of the few u-boots that isn't in buildroot yet.

I've attached an example patch adding Bananapi M2 Ultra.

In order to build this example:

$ git clone git://git.buildroot.net/buildroot

$ cd buildroot

$ patch -p1 < .../0001-Added-support-for-Sinovoip-BananaPi-M2-Ultra-board.patch

$ make bananapi_m2_ultra_defconfig

$ make # takes extremely long and takes lots of space

I'm planning to copy genimage.cfg from buildroot - not everything.

That would enable us to have a generic install-u-boot.

Buildroot: https://buildroot.org/

Git web: https://git.busybox.net/buildroot/

Git: git clone git://git.buildroot.net/buildroot

Git: git clone https://git.buildroot.net/buildroot

Patches: http://lists.buildroot.org/mailman/listinfo/buildroot

From 95be66f410ea89a72c77695dacce938edf97e9d0 Mon Sep 17 00:00:00 2001
From: Danny Milosavljevic <dannym@scratchpost.org>
Date: Sat, 16 Jun 2018 23:47:35 +0200
Subject: [PATCH 1/1] Added support for Sinovoip BananaPi M2 Ultra board.
Tags: patch

Signed-off-by: Danny Milosavljevic <dannym@scratchpost.org>
---
 board/sinovoip/m2-ultra/boot.cmd     |  7 +++++++
 board/sinovoip/m2-ultra/genimage.cfg | 33 +++++++++++++++++++++++++++++++++
 configs/bananapi_m2_ultra_defconfig  | 27 +++++++++++++++++++++++++++
 3 files changed, 67 insertions(+)
 create mode 100644 board/sinovoip/m2-ultra/boot.cmd
 create mode 100644 board/sinovoip/m2-ultra/genimage.cfg
 create mode 100644 configs/bananapi_m2_ultra_defconfig

Toggle diff (85 lines)diff --git a/board/sinovoip/m2-ultra/boot.cmd b/board/sinovoip/m2-ultra/boot.cmd
new file mode 100644
index 0000000000..9c24eabff6
--- /dev/null
+++ b/board/sinovoip/m2-ultra/boot.cmd
@@ -0,0 +1,7 @@
+setenv bootargs console=ttyS0,115200 earlyprintk root=/dev/mmcblk0p2 rootwait
+
+mmc dev 0
+fatload mmc 0 $kernel_addr_r zImage
+fatload mmc 0 $fdt_addr_r sun8i-r40-bananapi-m2-ultra.dtb
+
+bootz $kernel_addr_r - $fdt_addr_r
diff --git a/board/sinovoip/m2-ultra/genimage.cfg b/board/sinovoip/m2-ultra/genimage.cfg
new file mode 100644
index 0000000000..675253af23
--- /dev/null
+++ b/board/sinovoip/m2-ultra/genimage.cfg
@@ -0,0 +1,33 @@
+image boot.vfat {
+	vfat {
+		files = {
+			"zImage",
+			"sun8i-r40-bananapi-m2-ultra.dtb",
+			"boot.scr"
+		}
+	}
+	size = 64M
+}
+
+image sdcard.img {
+	hdimage {
+	}
+
+	partition u-boot {
+		in-partition-table = "no"
+		image = "u-boot-sunxi-with-spl.bin"
+		offset = 8192
+		size = 1040384 # 1MB - 8192
+	}
+
+	partition boot {
+		partition-type = 0xC
+		bootable = "true"
+		image = "boot.vfat"
+	}
+
+	partition rootfs {
+		partition-type = 0x83
+		image = "rootfs.ext4"
+	}
+}
diff --git a/configs/bananapi_m2_ultra_defconfig b/configs/bananapi_m2_ultra_defconfig
new file mode 100644
index 0000000000..a86d5923c6
--- /dev/null
+++ b/configs/bananapi_m2_ultra_defconfig
@@ -0,0 +1,27 @@
+BR2_arm=y
+BR2_cortex_a7=y
+BR2_PACKAGE_HOST_LINUX_HEADERS_CUSTOM_4_13=y
+BR2_TARGET_GENERIC_ISSUE="Welcome to Bananapi M2 Ultra"
+BR2_ROOTFS_POST_IMAGE_SCRIPT="support/scripts/genimage.sh"
+BR2_ROOTFS_POST_SCRIPT_ARGS="-c board/sinovoip/m2-ultra/genimage.cfg"
+BR2_LINUX_KERNEL=y
+BR2_LINUX_KERNEL_CUSTOM_VERSION=n
+BR2_LINUX_KERNEL_DEFCONFIG="sunxi"
+BR2_LINUX_KERNEL_DTS_SUPPORT=y
+BR2_LINUX_KERNEL_INTREE_DTS_NAME="sun8i-r40-bananapi-m2-ultra"
+BR2_TARGET_ROOTFS_EXT2=y
+BR2_TARGET_ROOTFS_EXT2_4=y
+# BR2_TARGET_ROOTFS_TAR is not set
+BR2_TARGET_UBOOT=y
+BR2_TARGET_UBOOT_BUILD_SYSTEM_KCONFIG=y
+BR2_TARGET_UBOOT_CUSTOM_VERSION=n
+BR2_TARGET_UBOOT_BOARD_DEFCONFIG="Bananapi_M2_Ultra"
+BR2_TARGET_UBOOT_NEEDS_DTC=y
+BR2_TARGET_UBOOT_NEEDS_PYLIBFDT=y
+BR2_TARGET_UBOOT_FORMAT_CUSTOM=y
+BR2_TARGET_UBOOT_FORMAT_CUSTOM_NAME="u-boot-sunxi-with-spl.bin"
+BR2_TARGET_UBOOT_BOOT_SCRIPT=y
+BR2_TARGET_UBOOT_BOOT_SCRIPT_SOURCE="board/sinovoip/m2-ultra/boot.cmd"
+BR2_PACKAGE_HOST_DOSFSTOOLS=y
+BR2_PACKAGE_HOST_GENIMAGE=y
+BR2_PACKAGE_HOST_MTOOLS=y

-----BEGIN PGP SIGNATURE-----

iQEzBAEBCAAdFiEEds7GsXJ0tGXALbPZ5xo1VCwwuqUFAlsmUOsACgkQ5xo1VCww

uqV6AAf/WXhWDGjYCKpWVjv456zUTYJYsA49spc1rwky6w6B+DFZLrhbuWdVvqdO

n6/zKmkJeeo23XlXn4o4pi55cpNDO994PfKeXOUTaNL7xpOFoBdytGNaX6J/Klhj

FCr2T7QYJyS/AMpR7YiYa/SqXrjcgjESDu9sEdvJr+tk+1VryfAJs6C986RWejcE

cv+eQGFcDnuaddlK3d0WdrxcnytXxLyJmZif2QFi4n6bieeLBMWwGPWGhrF6LqSG

0mXqqJngD4iP5l6+TcMw/fKAIYC2vc3spEgBc4zVfsh5rswDk/w4Ad4MdipOAb1W

1t2Qvp1c0p+cwXxl+XpT5eEj4LzJYA==

=c6kI

-----END PGP SIGNATURE-----

Closed

Your comment

This issue is archived.

To comment on this conversation send an email to 31599@debbugs.gnu.org

is:open	open issues
is:done	closed issues
submitter:<who>	search issue submitter
author:<who>	search by message author
date:yesterday..now	search by issue date
mdate:3m..2d	search by message date