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

Done

Details

2 participants

Danny Milosavljevic
Vagrant Cascadian

Owner: unassigned

Submitted by: Vagrant Cascadian

Severity: normal

Debbugs page

Vagrant Cascadian wrote 7 years ago

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 7 years ago

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 7 years ago

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 7 years ago

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 7 years ago

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 7 years ago

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 7 years ago

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

To respond to this issue using the mumi CLI, first switch to it

mumi current 31599

Then, you may apply the latest patchset in this issue (with sign off)

mumi am -- -s

Or, compose a reply to this issue

mumi compose

Or, send patches to this issue

mumi send-email *.patch

You may also tag this issue. See list of standard tags. For example, to set the confirmed and easy tags

mumi command -t +confirmed -t +easy

Or, remove the moreinfo tag and set the help tag

mumi command -t -moreinfo -t +help

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