Re: [Buildroot] [PATCH v1 2/3] add board/versal

["Frager, Neal via buildroot" <buildroot@buildroot.org>]

Hello Thomas,

> Please note that this is not a full review. Just some comments.

> This PATCH 2/3 should be squashed with PATCH 3/3 into a single patch, whose commit title should be:

>	configs/versal_vck190: new defconfig

Ok, no problem.

> More comments below.


> diff --git a/board/versal/post-build.sh b/board/versal/post-build.sh 
> new file mode 100755 index 0000000000..0713bd1b05
> --- /dev/null
> +++ b/board/versal/post-build.sh
> @@ -0,0 +1,16 @@
> +#!/bin/sh
> +
> +# genimage will need to find the extlinux.conf # in the binaries 
> +directory
> +
> +BOARD_DIR="$(dirname $0)"
> +CONSOLE=$2
> +ROOT=$3
> +
> +mkdir -p "${BINARIES_DIR}"
> +cat <<-__HEADER_EOF > "${BINARIES_DIR}/extlinux.conf"
> +	label linux
> +	  kernel /Image
> +	  devicetree /system.dtb
> +	  append console=${CONSOLE} root=/dev/${ROOT} rw rootwait
> +	__HEADER_EOF

> Meeh, I don't know if I like being that smart. What about having an extlinux.conf file per board, and simplify this? Sometimes dumb is better than smart/complicated.

I understand, and usually agree. This post_build.sh actually already exists in buildroot in the board/zynqmp directory.
The reason why it was done this way was because the kria kv260 uses a different serial port and sd card device than the zynqmp zcu boards.
We could be super smart and have versal use the same board/zynqmp/post-build.sh, but mixing zynqmp and versal is probably not so clean.
I could also revert to dumb and easy, but that creates additional extlinux.conf files everywhere.

As I will be maintaining these zynqmp and versal board files, my preference is to keep both the same style.  
Either both dumb with an extlinux.conf file per board or both smart/complicated.

With this in mind, what is your preference?

> diff --git a/board/versal/vck190/uboot.fragment 
> b/board/versal/vck190/uboot.fragment
> new file mode 100644
> index 0000000000..961c4239bd
> --- /dev/null
> +++ b/board/versal/vck190/uboot.fragment
> @@ -0,0 +1 @@
> +CONFIG_DEFAULT_DEVICE_TREE="versal-vck190-rev1.1"

> This can be removed in favor of passing DEVICE_TREE=... in BR2_TARGET_UBOOT_CUSTOM_MAKEOPTS.

Ok, should I update all of the zynq, zynqmp and kria defconfigs as well?

Thanks for your review!

Best regards,
Neal Frager
AMD
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot