Re: [Buildroot] [PATCH 1/1] board: Add support for Seeed STM32MP157C-Odyssey

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

Hello Simon,

Thanks a lot for your contribution, and sorry for the huge delay in
getting back to you. Your patch is really mostly good, there are only a
few comments/questions, see below.

On Fri, 30 Apr 2021 10:56:53 +0000
Simon Doppler <dopsi@dopsi.ch> wrote:

> STM32MP157C-Odyssey reference:
> https://wiki.seeedstudio.com/ODYSSEY-STM32MP157C/
> 
> Signed-off-by: Simon Doppler <dopsi@dopsi.ch>
> ---
>  board/seeed/stm32mp157c-odyssey/genimage.cfg  |  22 +++
>  board/seeed/stm32mp157c-odyssey/linux.config  | 182 ++++++++++++++++++
>  .../overlay/boot/extlinux/extlinux.conf       |   4 +
>  .../linux/5.10.1/0001-sdmmc1-broken-cd.patch  |  13 ++
>  board/seeed/stm32mp157c-odyssey/post-image.sh |  12 ++
>  .../stm32mp157c-odyssey/uboot-fragment.config |   1 +
>  configs/stm32mp157c_odyssey_defconfig         |  31 +++
>  7 files changed, 265 insertions(+)
>  create mode 100644 board/seeed/stm32mp157c-odyssey/genimage.cfg
>  create mode 100644 board/seeed/stm32mp157c-odyssey/linux.config
>  create mode 100644 board/seeed/stm32mp157c-odyssey/overlay/boot/extlinux/extlinux.conf
>  create mode 100644 board/seeed/stm32mp157c-odyssey/patches/linux/5.10.1/0001-sdmmc1-broken-cd.patch
>  create mode 100755 board/seeed/stm32mp157c-odyssey/post-image.sh
>  create mode 100644 board/seeed/stm32mp157c-odyssey/uboot-fragment.config
>  create mode 100644 configs/stm32mp157c_odyssey_defconfig

Could you change the commit title to:

	configs/stm32mp157c_odyssey: new defconfig

and add an entry in the DEVELOPERS file with your name/email associated
to the new defconfig and new folder in board/seeed/ ?

> diff --git a/board/seeed/stm32mp157c-odyssey/genimage.cfg b/board/seeed/stm32mp157c-odyssey/genimage.cfg
> new file mode 100644
> index 0000000000..227fbb600c
> --- /dev/null
> +++ b/board/seeed/stm32mp157c-odyssey/genimage.cfg
> @@ -0,0 +1,22 @@
> +image sdcard.img {
> +	hdimage {
> +		gpt = "true"
> +	}
> +
> +	partition fsbl1 {
> +		image = "u-boot-spl.stm32"
> +	}
> +
> +	partition fsbl2 {
> +		image = "u-boot-spl.stm32"
> +	}

Are you sure you want to stick to the U-Boot SPL booting process, as
opposed to the TF-A based booting process? ST now recommends using the
TF-A booting process.


> diff --git a/board/seeed/stm32mp157c-odyssey/patches/linux/5.10.1/0001-sdmmc1-broken-cd.patch b/board/seeed/stm32mp157c-odyssey/patches/linux/5.10.1/0001-sdmmc1-broken-cd.patch
> new file mode 100644
> index 0000000000..f15a096d91
> --- /dev/null
> +++ b/board/seeed/stm32mp157c-odyssey/patches/linux/5.10.1/0001-sdmmc1-broken-cd.patch
> @@ -0,0 +1,13 @@

We need patches to have a proper commit log and Signed-off-by line.

> +++ b/board/seeed/stm32mp157c-odyssey/post-image.sh
> @@ -0,0 +1,12 @@
> +#!/usr/bin/env bash
> +
> +main()
> +{
> +	local GENIMAGE_CFG="board/seeed/stm32mp157-odyssey/genimage.cfg"
> +
> +	support/scripts/genimage.sh -c ${GENIMAGE_CFG}

This should not be needed, you can do it directly in the defconfig:

BR2_ROOTFS_POST_IMAGE_SCRIPT="support/scripts/genimage.sh"
BR2_ROOTFS_POST_SCRIPT_ARGS="-c board/terasic/de10nano_cyclone5/genimage.cfg"

> diff --git a/board/seeed/stm32mp157c-odyssey/uboot-fragment.config b/board/seeed/stm32mp157c-odyssey/uboot-fragment.config
> new file mode 100644
> index 0000000000..a278ce9e23
> --- /dev/null
> +++ b/board/seeed/stm32mp157c-odyssey/uboot-fragment.config
> @@ -0,0 +1 @@
> +# CONFIG_STM32MP_WATCHDOG is not set

This issue has apparently been fixed since Linux 5.6, see commit
fe2e655666f3b3bc5ebc38036585fe9e0bbb7e30 in Buildroot (applicable to
the STM32MP1 DK2 board).

> diff --git a/configs/stm32mp157c_odyssey_defconfig b/configs/stm32mp157c_odyssey_defconfig
> new file mode 100644
> index 0000000000..b001757bf5
> --- /dev/null
> +++ b/configs/stm32mp157c_odyssey_defconfig
> @@ -0,0 +1,31 @@
> +BR2_arm=y
> +BR2_cortex_a7=y
> +BR2_CCACHE=y

ccache shouldn't be enabled in a defconfig.

> +BR2_GLOBAL_PATCH_DIR="$(TOPDIR)/board/seeed/stm32mp157c-odyssey/patches"

$(TOPDIR)/ not needed.

> +BR2_PACKAGE_HOST_LINUX_HEADERS_CUSTOM_5_10=y
> +BR2_ROOTFS_OVERLAY="board/seeed/stm32mp157c-odyssey/overlay/"
> +BR2_ROOTFS_POST_IMAGE_SCRIPT="board/seeed/stm32mp157c-odyssey/post-image.sh"
> +BR2_LINUX_KERNEL=y
> +BR2_LINUX_KERNEL_CUSTOM_VERSION=y
> +BR2_LINUX_KERNEL_CUSTOM_VERSION_VALUE="5.10.1"
> +BR2_LINUX_KERNEL_USE_CUSTOM_CONFIG=y
> +BR2_LINUX_KERNEL_CUSTOM_CONFIG_FILE="board/seeed/stm32mp157c-odyssey/linux.config"
> +BR2_LINUX_KERNEL_DTS_SUPPORT=y
> +BR2_LINUX_KERNEL_INTREE_DTS_NAME="stm32mp157c-odyssey"
> +BR2_LINUX_KERNEL_INSTALL_TARGET=y
> +BR2_TARGET_ROOTFS_EXT2=y
> +BR2_TARGET_ROOTFS_EXT2_4=y
> +BR2_TARGET_ROOTFS_EXT2_SIZE="120M"
> +# BR2_TARGET_ROOTFS_TAR is not set
> +BR2_TARGET_UBOOT=y
> +BR2_TARGET_UBOOT_BOARD_DEFCONFIG="stm32mp15_basic"
> +BR2_TARGET_UBOOT_CONFIG_FRAGMENT_FILES="board/seeed/stm32mp157c-odyssey/uboot-fragment.config"
> +BR2_TARGET_UBOOT_NEEDS_DTC=y
> +# BR2_TARGET_UBOOT_FORMAT_BIN is not set
> +BR2_TARGET_UBOOT_FORMAT_IMG=y
> +BR2_TARGET_UBOOT_FORMAT_STM32=y
> +BR2_TARGET_UBOOT_SPL=y
> +BR2_TARGET_UBOOT_SPL_NAME="spl/u-boot-spl.stm32"
> +BR2_TARGET_UBOOT_CUSTOM_MAKEOPTS="DEVICE_TREE=stm32mp157c-odyssey"
> +BR2_PACKAGE_HOST_GENEXT2FS=y

This should not be needed. We don't use genext2fs anymore in fact to
generate ext2/3/4 images.

Could you take into account the few comments above, and send an updated
version?

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