linux and u-boot for pinenote-rk3568

[Vagrant Cascadian <vagrant@debian.org>]

[-- Attachment #1: Type: text/plain, Size: 10046 bytes --]

On 2022-06-30, phodina via wrote:
> I'm attempting to build the Linux kernel and u-boot for ARM64 target -
> eink tablet.

I can infer what you're talking about from the later patches, but it
would be good to mention what specific hardware you're talking about up
front and/or in the subject. :)


> Unfotutnately the patches are not yet in the upstream so I had to
> tweak little bit in the gnu/packages/linux.scm and
> gnu/bootloader/bootloaders.scm files.

Had to do similar things early on to get the pinebook-pro support
available in guix before it was fully upstream... wheee!


A partial review with comments inline follows...


> Subject: [PATCH 4/7] gnu: Provide private implementation with selection of the
>  u-boot package.
>
> * gnu/packages/bootloaders.scm: Provide private implementation of
>   make-u-boot-package and make-u-boot-sunxi64-package so that the public API
>   does not change. The private allows to specify the u-boot package.

This seems unecessarily complicated; you could just use a different
source for the eventual u-boot-pinenote* packages rather than renaming
existing functions and/or adding new ones... see what
u-boot-nintendo-nes-classic-edition does, mentioned below...


> From 44b1385e6dbcc79bafebddc7699ed302fcb5fe91 Mon Sep 17 00:00:00 2001
> From: Petr Hodina <phodina@protonmail.com>
> Date: Thu, 30 Jun 2022 11:20:55 +0200
> Subject: [PATCH 6/7] gnu: Add install-pinenote-rk3568-u-boot and
>  u-boot-pinenote-rk3568-bootloader.
>
> * gnu/bootloader/u-boot.scm (install-pinenote-rk3568-u-boot,
>   u-boot-pinenote-rk3568-bootloader): New variables.

The patch order is wrong, this is patch 6/7 followed by patch 5/7
... sometimes the order doesn't matter, but this order was actually a
bit confusing for me. Please submit patches in the appropriate order. :)


> diff --git a/gnu/bootloader/u-boot.scm b/gnu/bootloader/u-boot.scm
> index 6cad33b741..4410cc497a 100644
> --- a/gnu/bootloader/u-boot.scm
> +++ b/gnu/bootloader/u-boot.scm
...
> @@ -127,6 +129,15 @@ (define install-rockpro64-rk3399-u-boot
>  
>  (define install-pinebook-pro-rk3399-u-boot install-rockpro64-rk3399-u-boot)
>  
> +;; TODO: Supply correct offsets
> +(define install-pinenote-rk3568-u-boot
> +  #~(lambda (bootloader root-index image)
> +      (let ((idb (string-append bootloader "/libexec/idbloader.img"))
> +            (u-boot (string-append bootloader "/libexec/u-boot.itb")))
> +        (write-file-on-device idb (stat:size (stat idb))
> +                              image (* 64 512))
> +        (write-file-on-device u-boot (stat:size (stat u-boot))
> +                              image (* 16384 512)))))

Does it really use different offsets than other rockchip platforms, or
is that just a note to confirm if it does?


> From 4d53d2bdb8526f72ed6cd3183ff2a2141990c900 Mon Sep 17 00:00:00 2001
> From: Petr Hodina <phodina@protonmail.com>
> Date: Thu, 30 Jun 2022 11:19:33 +0200
> Subject: [PATCH 5/7] gnu: Add u-boot-pinenote-rk3568.
>
> * gnu/packages/bootloaders.scm (u-boot-pinenote-rk3568): New variable.
>
> diff --git a/gnu/packages/bootloaders.scm b/gnu/packages/bootloaders.scm
> index cab97852f1..9c266b7bed 100644
> --- a/gnu/packages/bootloaders.scm
> +++ b/gnu/packages/bootloaders.scm
> @@ -834,6 +834,9 @@ (define-public u-boot-pine64-plus
>  (define-public u-boot-pine64-lts
>    (make-u-boot-sunxi64-package "pine64-lts" "aarch64-linux-gnu"))
>  
> +(define-public u-boot-pinenote-rk3568
> +  (make-u-boot-sunxi64-package-priv "pinenote" "aarch64-linux-gnu" u-boot-pinenote))
> +
>  (define-public u-boot-pinebook
>    (let ((base (make-u-boot-sunxi64-package "pinebook" "aarch64-linux-gnu")))
>      (package

Definitely not u-boot-sunxi64-package. Probably more similar to the
u-boot-pinebook-pro-rk3399 or u-boot-rock64-rk3328, which i think use
the standard make-u-boot-package.

For using an entirely different source repository for a specific
package, see the u-boot-nintendo-nes-classic-edition and how that uses a
different source, e.g.

(define-public u-boot-nintendo-nes-classic-edition
  (let ((base (make-u-boot-package "Nintendo_NES_Classic_Edition"
                                   "arm-linux-gnueabihf")))
    (package
      (inherit base)
      ;; Starting with 2019.01, FEL doesn't work anymore on A33.
      (version "2018.11")
      (source (origin
                ...

> From a6499c4c384dd3c0e06968cc6987da0e47af6290 Mon Sep 17 00:00:00 2001
> From: Petr Hodina <phodina@protonmail.com>
> Date: Thu, 30 Jun 2022 10:12:39 +0200
> Subject: [PATCH 1/7] gnu: Add linux-libre-arm64-pinenote.
>
> * gnu/packages/linux.scm (linux-libre-arm64-pinenote): New variable.
> * gnu/local.mk: Add patches.
> * linux-libre-arm64-pinenote-battery-level.patch: New file.
> * linux-libre-arm64-pinenote-defconfig.patch: New file.
> * linux-libre-arm64-pinenote-dtsi.patch: New file.
> * linux-libre-arm64-pinenote-ebc-patches.patch: New file.
> * linux-libre-arm64-pinenote-touchscreen-1.patch: New file.
> * linux-libre-arm64-pinenote-touchscreen-2.patch: New file.
> * linux-libre-arm64-rockchip-add-hdmi-sound.patch: New file.

I'm going to make some comments on this patch, but I get plenty confused
with patches in gnu/packages/linux.scm as it can be a little more
complicated than your typical packaging, so any other reviewers really
ought to jump in too! :)


> diff --git a/gnu/local.mk b/gnu/local.mk
> index 353b91cfd2..c19f1f418b 100644
> --- a/gnu/local.mk
> +++ b/gnu/local.mk
> @@ -1444,6 +1444,13 @@ dist_patch_DATA =						\
>    %D%/packages/patches/linbox-fix-pkgconfig.patch		\
>    %D%/packages/patches/linphone-desktop-without-sdk.patch           \
>    %D%/packages/patches/linux-libre-support-for-Pinebook-Pro.patch \
> +  %D%/packages/patches/linux-libre-arm64-pinenote-touchscreen-1.patch \
> +  %D%/packages/patches/linux-libre-arm64-pinenote-touchscreen-2.patch \
> +   %D%/packages/patches/linux-libre-arm64-pinenote-battery-level.patch \
> +   %D%/packages/patches/linux-libre-arm64-rockchip-add-hdmi-sound.patch \
> +    %D%/packages/patches/linux-libre-arm64-pinenote-defconfig.patch \
> +   %D%/packages/patches/linux-libre-arm64-pinenote-dtsi.patch \

whitespace inconsistancy on the previous few lines...


> diff --git a/gnu/packages/linux.scm b/gnu/packages/linux.scm
> index 8f7b4f4f5b..bae8ffb959 100644
> --- a/gnu/packages/linux.scm
> +++ b/gnu/packages/linux.scm
> @@ -349,6 +349,12 @@ (define (%upstream-linux-source version hash)
>                          "linux-" version ".tar.xz"))
>      (sha256 hash)))
>  
> +(define (%pinenote-linux-source version hash)
> +  (origin
> +    (method url-fetch)
> +    (uri (string-append "https://github.com/phodina/linux-pinenote/tarball/" version))
> +    (sha256 hash)))
> +

I think tarballs from github are generally discouraged; although with
the kernel that can be quite a large git checkout... but...


>  ;; The current "stable" kernels. That is, the most recently released major
>  ;; versions that are still supported upstream.
>  
> @@ -367,6 +373,14 @@ (define-public linux-libre-5.17-pristine-source
>                              (%upstream-linux-source version hash)
>                              deblob-scripts-5.17)))
>  
> +(define-public linux-libre-arm64-pinenote-pristine-source
> +  (let ((version linux-libre-5.17-version)
> +        (commit "c91a48e028fe1f6a0e5748fd87c446aa7e31811b")
> +        (hash (base32 "1xwyvvps1r3zl1n9szlgrj8ylw5sgj6fr52fig9f2cc6ai331bbn")))
> +   (make-linux-libre-source version
> +                            (%pinenote-linux-source commit hash)
> +                            deblob-scripts-5.17)))
> +

Oh, wait, you do pull from git? Is the tarball used, or the git
checkout? Seems like you shouldn't need both.


> +(define-public linux-libre-arm64-pinenote-source
> +  (source-with-patches linux-libre-arm64-pinenote-pristine-source
> +                       (list %boot-logo-patch
> +                             %linux-libre-arm-export-__sync_icache_dcache-patch
> +                             (search-patch
> +"linux-libre-arm64-pinenote-touchscreen-1.patch")
> +                             (search-patch
> +"linux-libre-arm64-pinenote-touchscreen-2.patch")
> +                             (search-patch
> +"linux-libre-arm64-pinenote-battery-level.patch")
> +                             (search-patch
> +"linux-libre-arm64-rockchip-add-hdmi-sound.patch")
> +                             (search-patch
> +"linux-libre-arm64-pinenote-defconfig.patch")
> +                             (search-patch
> +"linux-libre-arm64-pinenote-dtsi.patch")
> +                             (search-patch
> +"linux-libre-arm64-pinenote-ebc-patches.patch"))))
> +

Maybe use:

(search-patches "A.patch"
                "B.patch"
                ...
                "N.patch")

Though you may have to append two lists together for that to work. As to
how... ask someone who understands guile. :)

Also wondering if there is a branch with all the patches you need
already applied, since you're using a separate source origin already...


> +(define-public linux-libre-arm64-pinenote
> +  (make-linux-libre* linux-libre-version
> +                     linux-libre-gnu-revision
> +                     linux-libre-arm64-pinenote-source
> +                     '("aarch64-linux")
> +                     #:defconfig "pinenote_defconfig"
> +                     #:extra-options
> +                      %default-extra-linux-options))

You might want to not assume to use the same versions as
(linux-libre-version, linux-libre-gnu-revision), as they might change
out from under you... those were recently updated to 5.18.x ... instead
just pick the versions relevent to the upstream source.


Hope that's helpful, good luck and thanks for working on adding support
for this very interesting platform!


live well,
  vagrant

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]