Re: [Buildroot] [PATCH] configs/friendlyarm_nanopi_neo_defconfig: new defconfig

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

Hello Dong,

First of all, thanks a lot for your contribution and patch submission,
and sorry for the delay with which we are getting back to you. I only
have a few comments below.

On Wed, 15 May 2024 08:52:31 +0800
Dong Wang <wangdong115@foxmail.com> wrote:

> This patch adds a new defconfig for the NanoPi NEO board made by
> FriendlyARM. This board is based on the Allwinner H3 SoC.
> 
> See: https://wiki.friendlyelec.com/wiki/index.php/NanoPi_NEO
> 
> .

This empty line with a dot seems quite useless, could you drop it?

> 
> This patch uses the mainline kernel and u-boot for the board.
> 
> The configurations are based on the previously dropped defconfig
> maintained by Yann E. MORIN <yann.morin.1998@free.fr>.
> 
> Signed-off-by: Dong Wang <wangdong115@foxmail.com>


> diff --git a/board/friendlyarm/nanopi-neo/boot.cmd b/board/friendlyarm/nanopi-neo/boot.cmd
> new file mode 100644
> index 0000000000..8c199ed480
> --- /dev/null
> +++ b/board/friendlyarm/nanopi-neo/boot.cmd
> @@ -0,0 +1,5 @@
> +setenv bootargs console=ttyS0,115200 earlyprintk root=/dev/mmcblk0p2 rootwait panic=10
> +
> +load mmc 0:1 ${fdt_addr_r} ${fdtfile}
> +load mmc 0:1 ${kernel_addr_r} zImage
> +bootz ${kernel_addr_r} - ${fdt_addr_r}

Could you drop this custom U-Boot script, and use extlinux.conf like we
try to do in most new defconfigs?

> diff --git a/board/friendlyarm/nanopi-neo/genimage.cfg b/board/friendlyarm/nanopi-neo/genimage.cfg
> new file mode 100644
> index 0000000000..4cfd8c8fc2
> --- /dev/null
> +++ b/board/friendlyarm/nanopi-neo/genimage.cfg
> @@ -0,0 +1,36 @@
> +# Minimal SD card image for the NanoPi NEO.
> +image boot.vfat {
> +	vfat {
> +		files = {
> +			"zImage",
> +			"sun8i-h3-nanopi-neo.dtb",
> +			"boot.scr"
> +		}
> +	}
> +
> +	size = 16M
> +}
> +
> +image sdcard.img {
> +	hdimage {
> +	}
> +
> +	partition u-boot {
> +		in-partition-table = false
> +		image = "u-boot-sunxi-with-spl.bin"
> +		offset = 8K
> +		size = 1000K # 1MB - 8KB(offset) - 16KB(GPT)
> +	}
> +
> +	partition boot {
> +		partition-type = 0xC
> +		bootable = "true"
> +		image = "boot.vfat"
> +	}

Could you drop entirely this VFAT partition, and have the kernel + DTB
+ extlinux.conf directly in the rootfs, and U-Boot directly load
extlinux.conf from this ext4 filesystem?

> +	partition rootfs {
> +		partition-type = 0x83
> +		image = "rootfs.ext4"
> +		size = 512M

Leave out the size = argument, it isn't very useful to have a larger
partition, but a filesystem that is smaller than that size.

All of the rest really looks good to me. Could you just address the
above comments, and submit a v2 ?

Thanks a lot for your contribution and effort!

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