Re: [Buildroot] [PATCH] board: Add support for Engicam PX30 SOM board

[Thomas Petazzoni via buildroot <buildroot@buildroot.org>]

Hello Michael/Meena,

Thanks for this contribution, see below my review.

On Wed, 12 Jun 2024 18:33:33 +0530
Meena Murthy <meena.murthy@amarulasolutions.com> wrote:

> From: Michael Trimarchi <michael@amarulasolutions.com>
> 
> Add initial support for Engicam PX30 SOM board
> with below features:
> - U-Boot 2024.01
> - Linux 6.4.16
> - Default packages from buildroot
> 
> px30 ctouch2 with 10 inches display
> 
> https://www.engicam.com/vis-prod/C-Touch-2-0-Carrier-Board/ \
> 	General-purpose-carrier-board-with-capacitive-touch-interface-EDIMM-20-compliant
> 
> Signed-off-by: Meena Murthy <meena.murthy@amarulasolutions.com>

There is a first problem here that the From: field doesn't match the
first Signed-off-by: line. They must *always* match (this is also valid
for Linux kernel contributions, for example).

If Meena is the author, then (s)he needs to be in the From: and first
Signed-off-by: and if the code has then gone through the hands of
Michael, then Michael can add his Signed-off-by *after* Meena's
Signed-off-by.

>  DEVELOPERS                           |  3 ++
>  board/engicam/px30core/extlinux.conf |  4 +++
>  board/engicam/px30core/genimage.cfg  | 30 +++++++++++++++++
>  board/engicam/px30core/post-build.sh |  5 +++
>  board/engicam/px30core/readme.txt    | 40 +++++++++++++++++++++++
>  configs/engicam_px30_core_defconfig  | 49 ++++++++++++++++++++++++++++
>  6 files changed, 131 insertions(+)
>  create mode 100644 board/engicam/px30core/extlinux.conf
>  create mode 100644 board/engicam/px30core/genimage.cfg
>  create mode 100755 board/engicam/px30core/post-build.sh
>  create mode 100644 board/engicam/px30core/readme.txt
>  create mode 100644 configs/engicam_px30_core_defconfig
> 
> diff --git a/DEVELOPERS b/DEVELOPERS
> index 52c9b84a9d..38e4c4e2d9 100644
> --- a/DEVELOPERS
> +++ b/DEVELOPERS
> @@ -2253,6 +2253,9 @@ F:	package/libtraceevent/
>  F:	package/libtracefs
>  F:	package/linux-tools/linux-tool-rtla.mk.in
>  
> +N:  Meena Murthy <meena.murthy@amarulasolutions.com>
> +F:  board/engicam/px30core/

Indentation is not correct: the separator is a tab.

The DEVELOPERS file should also reference the defconfig file.

> +
>  N:	Michael Durrant <mdurrant@arcturusnetworks.com>
>  F:	board/arcturus/
>  F:	configs/arcturus_ucp1020_defconfig
> diff --git a/board/engicam/px30core/extlinux.conf b/board/engicam/px30core/extlinux.conf
> new file mode 100644
> index 0000000000..c3a6740af2
> --- /dev/null
> +++ b/board/engicam/px30core/extlinux.conf
> @@ -0,0 +1,4 @@
> +label RK3399RocPC linux

         ^^^^^^^^^^^ Probably wrong copy/paste ?

> +  kernel /boot/Image
> +  devicetree /boot/px30-engicam-px30-core-ctouch2-of10.dtb
> +  append earlycon=uart8250,mmio32,0xff160000 root=/dev/mmcblk1p1 rootwait
> diff --git a/board/engicam/px30core/genimage.cfg b/board/engicam/px30core/genimage.cfg
> new file mode 100644
> index 0000000000..3bf92b318a
> --- /dev/null
> +++ b/board/engicam/px30core/genimage.cfg
> @@ -0,0 +1,30 @@
> +image sdcard.img {
> +	hdimage {
> +	}
> +
> +	partition u-boot-tpl-spl-dtb {
> +		in-partition-table = "no"
> +		image = "idbloader.img"
> +		offset = 32K
> +	}
> +
> +	partition u-boot-dtb {
> +		in-partition-table = "no"
> +		image = "u-boot.itb"
> +		offset = 8M
> +		size = 30M
> +	}
> +
> +	partition rootfs {
> +		partition-type = 0x83
> +		image = "rootfs.ext4"
> +	}
> +}
> +
> +image sdcard-sparse.img {
> +	android-sparse {
> +		image = sdcard.img
> +	}

We don't have this for any other defconfig/genimage, so please keep
this out of the genimage for now. We can perhaps discuss separately how
we might want to generate android sparse images, but that should be a
separate discussion.

> diff --git a/board/engicam/px30core/readme.txt b/board/engicam/px30core/readme.txt
> new file mode 100644
> index 0000000000..9b10ca7ebc
> --- /dev/null
> +++ b/board/engicam/px30core/readme.txt
> @@ -0,0 +1,40 @@
> +Libre Computer Board ROC-RK3399-PC

Bad copy paste.

We also like to have a short intro about the board, with a link to some
webpage about it.

> +===================================
> +
> +Build:
> +
> +  $ make engicam_px30_core_defconfig
> +  $ make
> +
> +Files created in output directory
> +=================================
> +
> +output/images
> +
> +├── bl31.elf
> +├── idbloader.img
> +├── Image
> +├── px30-engicam-px30-core-ctouch2-of10.dtb
> +├── rootfs.ext2
> +├── rootfs.ext4 -> rootfs.ext2
> +├── rootfs.tar
> +├── sdcard.img
> +├── u-boot.bin
> +└── u-boot.itb
> +
> +Creating bootable SD card:
> +==========================
> +
> +Simply invoke (as root)
> +
> +sudo dd if=output/images/sdcard.img of=/dev/sdX && sync
> +
> +Where X is your SD card device

Hu?

> +
> +Serial console
> +--------------
> +
> +Baudrate for this board is 115200

Please indicate how to connect the board (power, UART), anything that
can be relevant for a quick start.

> +
> +Wiki link:
> +https://wiki.amarulasolutions.com/bsp/rockchip/px30/px30.html
> diff --git a/configs/engicam_px30_core_defconfig b/configs/engicam_px30_core_defconfig
> new file mode 100644
> index 0000000000..72b8a3c045
> --- /dev/null
> +++ b/configs/engicam_px30_core_defconfig
> @@ -0,0 +1,49 @@
> +BR2_aarch64=y
> +BR2_cortex_a72_a53=y
> +BR2_PACKAGE_HOST_LINUX_HEADERS_CUSTOM_6_4=y
> +BR2_BINUTILS_VERSION_2_40_X=y

Please drop this custom binutils version.

> +BR2_TOOLCHAIN_BUILDROOT_CXX=y

Please do not enable C++ support by default.

> +BR2_TARGET_GENERIC_HOSTNAME="engicam-px30-core"
> +BR2_TARGET_GENERIC_ISSUE="Welcome to PX30 engicam core!"
> +BR2_ROOTFS_MERGED_USR=y

Please drop.

> +BR2_SYSTEM_DEFAULT_PATH="/bin:/sbin:/usr/bin:/usr/sbin"

Ditto.

> +BR2_TARGET_TZ_INFO=y

Ditto.

> +BR2_ROOTFS_POST_BUILD_SCRIPT="board/engicam/px30core/post-build.sh"
> +BR2_ROOTFS_POST_IMAGE_SCRIPT="support/scripts/genimage.sh"
> +BR2_ROOTFS_POST_SCRIPT_ARGS="-c board/engicam/px30core/genimage.cfg"
> +BR2_LINUX_KERNEL=y
> +BR2_LINUX_KERNEL_CUSTOM_VERSION=y
> +BR2_LINUX_KERNEL_CUSTOM_VERSION_VALUE="6.4.16"
> +BR2_LINUX_KERNEL_USE_ARCH_DEFAULT_CONFIG=y
> +BR2_LINUX_KERNEL_DTS_SUPPORT=y
> +BR2_LINUX_KERNEL_INTREE_DTS_NAME="rockchip/px30-engicam-px30-core-ctouch2-of10"
> +BR2_LINUX_KERNEL_INSTALL_TARGET=y
> +BR2_LINUX_KERNEL_NEEDS_HOST_OPENSSL=y
> +BR2_TARGET_ROOTFS_EXT2=y
> +BR2_TARGET_ROOTFS_EXT2_4=y
> +BR2_TARGET_ROOTFS_EXT2_SIZE="150M"
> +BR2_TARGET_ROOTFS_EXT2_MKFS_OPTIONS="-O 64bit"
> +BR2_TARGET_ARM_TRUSTED_FIRMWARE=y
> +BR2_TARGET_ARM_TRUSTED_FIRMWARE_CUSTOM_GIT=y
> +BR2_TARGET_ARM_TRUSTED_FIRMWARE_CUSTOM_REPO_URL="https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git"
> +BR2_TARGET_ARM_TRUSTED_FIRMWARE_CUSTOM_REPO_VERSION="v2.9"
> +BR2_TARGET_ARM_TRUSTED_FIRMWARE_PLATFORM="px30"
> +BR2_TARGET_ARM_TRUSTED_FIRMWARE_IMAGES=""

Is this really correct? How does TF-A install any image if this is
empty?

> +BR2_TARGET_ARM_TRUSTED_FIRMWARE_NEEDS_ARM32_TOOLCHAIN=y
> +BR2_TARGET_UBOOT=y
> +BR2_TARGET_UBOOT_BUILD_SYSTEM_KCONFIG=y
> +BR2_TARGET_UBOOT_CUSTOM_VERSION=y
> +BR2_TARGET_UBOOT_CUSTOM_VERSION_VALUE="2024.01"
> +BR2_TARGET_UBOOT_BOARD_DEFCONFIG="px30-core-ctouch2-of10-px30"
> +BR2_TARGET_UBOOT_NEEDS_DTC=y
> +BR2_TARGET_UBOOT_NEEDS_OPENSSL=y
> +BR2_TARGET_UBOOT_NEEDS_ATF_BL31=y
> +BR2_TARGET_UBOOT_NEEDS_ATF_BL31_ELF=y
> +BR2_TARGET_UBOOT_USE_BINMAN=y
> +BR2_TARGET_UBOOT_FORMAT_CUSTOM=y
> +BR2_TARGET_UBOOT_FORMAT_CUSTOM_NAME="u-boot.itb"
> +BR2_TARGET_UBOOT_SPL=y
> +BR2_TARGET_UBOOT_SPL_NAME="idbloader.img"
> +BR2_PACKAGE_HOST_DOSFSTOOLS=y
> +BR2_PACKAGE_HOST_GENIMAGE=y
> +BR2_PACKAGE_HOST_MTOOLS=y

Could you fix those different (relatively minor) issues and send a new
iteration?

Thanks a lot!

Thomas Petazzoni
-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot