Re: [Buildroot] [PATCH v3] board: Add support for terasic DE10 Nano

[Thomas Petazzoni <thomas.petazzoni@bootlin.com>]

Hello,

On Wed, 21 Jul 2021 14:59:08 +0200
Gwenhael Goavec-Merou <gwenj@trabucayre.com> wrote:

> From: Gwenhael Goavec-Merou <gwenhael.goavec-merou@trabucayre.com>
> 
> Terasic DE10 Nano is an SoCFPGA cycloneV based board.
> References:
> - https://www.terasic.com.tw/cgi-bin/page/archive.pl?Language=English&CategoryNo=205&No=1046
> - https://rocketboards.org/foswiki/Documentation/DE10NanoDevelopmentBoard
> 
> Note: there is no dts in kernel (mainline or intel), but since DE0 Nano Soc is
> similar, the corresponding dts is usually used
> 
> Signed-off-by: Gwenhael Goavec-Merou <gwenhael.goavec-merou@trabucayre.com>

Thanks for the patch. It looks mostly good, expect for a few details,
see below.

> diff --git a/board/terasic/de10nano_cyclone5/genimage.cfg b/board/terasic/de10nano_cyclone5/genimage.cfg
> new file mode 100644
> index 0000000000..3261c62741
> --- /dev/null
> +++ b/board/terasic/de10nano_cyclone5/genimage.cfg
> @@ -0,0 +1,57 @@
> +image boot.vfat {
> +	vfat {
> +		file zImage {
> +            image = "zImage"
> +        }

Starting from here the indentation is not correct.

> +
> +        file socfpga_cyclone5_de0_nano_soc.dtb {
> +            image = "socfpga_cyclone5_de0_nano_soc.dtb"
> +        }
> +
> +        file barebox.bin {
> +            image = "barebox-socfpga-de10_nano.img"
> +        }
> +
> +        file barebox.env {
> +            image = "barebox-env"
> +        }
> +	}
> +	size = 8M

Could you fix the indentation in this block ?

> +}
> +
> +image boot.img {
> +	hdimage {
> +		partition-table = "no"
> +	}
> +
> +	partition spl {
> +		in-partition-table = "no"
> +		image = "barebox-socfpga-de10_nano-xload.img"
> +		offset = 0
> +		size = 64k
> +	}
> +
> +	size = 1M
> +}

I don't understand why you have a boot.img, which apparently only
contains the raw contacts of this xload.img file. Why not use this
xload.img file directly...

> +
> +image sdcard.img {
> +	hdimage {
> +	}
> +
> +	partition spl {
> +		partition-type = 0xa2
> +		image = "boot.img"

.. here ?

Example from another genimage.cfg:

        partition spl {
                in-partition-table = "no"
                image = "sunxi-spl.bin"
                offset = 8192
        }

        partition u-boot {
                in-partition-table = "no"
                image = "u-boot.itb"
                offset = 40K
                size = 1M # 1MB - 40K
        }

> +	partition rootfs {
> +		partition-type = 0x83
> +		image = "rootfs.ext2"
> +		size = 500M

I'd suggest to not put a size here, and instead tweak the size of the
ext2 filesystem in the defconfig. Indeed, having a larger partition but
not a larger filesystem is kind of useless (or requires a resize of the
filesystem when booting).

> diff --git a/configs/terasic_de10nano_cyclone5_defconfig b/configs/terasic_de10nano_cyclone5_defconfig
> new file mode 100644
> index 0000000000..4a356f31a8
> --- /dev/null
> +++ b/configs/terasic_de10nano_cyclone5_defconfig
> @@ -0,0 +1,28 @@
> +BR2_arm=y
> +BR2_cortex_a9=y
> +BR2_ARM_ENABLE_NEON=y
> +BR2_ARM_ENABLE_VFP=y
> +BR2_ARM_FPU_NEON=y
> +BR2_KERNEL_HEADERS_5_11=y

Could you use:

BR2_PACKAGE_HOST_LINUX_HEADERS_CUSTOM_5_11=y

instead ?

Could you adjust those small details and send a v4?

Thanks a lot!

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