[Buildroot] [PATCH 1/1] board/stm32f469-disco: update board configuration

Arnout Vandecappelle arnout at mind.be
Fri Jul 23 21:37:27 UTC 2021


 Hi Yauheni,

 Thank you for this patch. I have a few comments that I unfortunately can't fix
myself.

 First of all, there are a few other patches posted by Dario (in Cc] [1][2]. How
does this relate to them?

[1]
https://patchwork.ozlabs.org/project/buildroot/patch/20210704141106.1746-2-dariobin@libero.it/
[2] https://patchwork.ozlabs.org/project/buildroot/list/?series=253877



 The subject line of this patch should be:

confgis/stm32f469_disco_xip_defconfig: alternative defconfig for stm32f469_disco

On 23/07/2021 18:04, Yauheni Saldatsenka wrote:
> Update STM32F469-disco configuration files to operate with new kernel.
> 
> Result of make tinyconfig was taken as a starting point to fit kernel
> into flash memory.
> Current setup kernel + rootfs fits in 1.6MB on-chip flash memory
> 
> Fixes:
>     - Move kernel to new flash bank due to growth of dtb size
>     - Fix kernel start address in bootloader
>     - Remove outdated path which doesn't affect normal operation mode
> 
> Signed-off-by: Yauheni Saldatsenka <eugentoo at gmail.com>
> ---
>  .../stm32f469-disco/flash.sh                  |  47 +++-
>  .../stm32f469-disco/linux/defconfig           | 120 +++++++++
>  .../stm32f469-disco/linux/stm32f469-disco.dts | 240 ++++++++++++++++++
>  .../0002-kernel-start-address.patch           |  21 ++
>  .../patches/afboot-stm32/0003-no-mpu.patch    |  22 ++
>  configs/stm32f469_disco_xip_defconfig         |  24 ++
>  6 files changed, 465 insertions(+), 9 deletions(-)
>  create mode 100644 board/stmicroelectronics/stm32f469-disco/linux/defconfig
>  create mode 100644 board/stmicroelectronics/stm32f469-disco/linux/stm32f469-disco.dts
>  create mode 100644 board/stmicroelectronics/stm32f469-disco/patches/afboot-stm32/0002-kernel-start-address.patch
>  create mode 100644 board/stmicroelectronics/stm32f469-disco/patches/afboot-stm32/0003-no-mpu.patch
>  create mode 100644 configs/stm32f469_disco_xip_defconfig
> 
> diff --git a/board/stmicroelectronics/stm32f469-disco/flash.sh b/board/stmicroelectronics/stm32f469-disco/flash.sh
> index 984d2b2599..176e1c9d2c 100755
> --- a/board/stmicroelectronics/stm32f469-disco/flash.sh
> +++ b/board/stmicroelectronics/stm32f469-disco/flash.sh
> @@ -1,18 +1,47 @@
>  #!/bin/bash
>  
>  OUTPUT_DIR=$1
> +BUILD_TYPE=$2

 This change in the flash script will need a corresponding change in the readme.txt.

 Also, this change seems unrelated to the rest of the patch, so it should be in
a separate patch (with a good explanation in the commit message as to why it is
needed).

>  
> -if ! test -d "${OUTPUT_DIR}" ; then
> +if ! test -d "${OUTPUT_DIR}"
> +then

 Spurious change - we normally write the if and the then on one line in shell
scripts.

>      echo "ERROR: no output directory specified."
>      echo "Usage: $0 OUTPUT_DIR"
>      exit 1
>  fi
>  
> -${OUTPUT_DIR}/host/bin/openocd -f board/stm32f469discovery.cfg \
> -  -c "init" \
> -  -c "reset init" \
> -  -c "flash probe 0" \
> -  -c "flash info 0" \
> -  -c "flash write_image erase ${OUTPUT_DIR}/images/u-boot.bin 0x08000000" \
> -  -c "reset run" \
> -  -c "shutdown"
> +if  [[ -z "${BUILD_TYPE}" ]]
> +then
> +    echo "ERROR: no build type specified, please select 'xip' or 'uboot'."
> +    echo "Usage: $0 OUTPUT_DIR $1 BUILD_TYPE"
> +    exit 1
> +fi
> +
> +case "${BUILD_TYPE}" in
> +	"xip")
> +		${OUTPUT_DIR}/host/bin/openocd -f board/stm32f469discovery.cfg \
> +					 -c "init" \
> +					 -c "reset init" \
> +					 -c "flash probe 0" \
> +					 -c "flash info 0" \
> +					 -c "flash write_image erase ${OUTPUT_DIR}/images/stm32f469i-disco.bin 0x08000000" \
> +					 -c "flash write_image erase ${OUTPUT_DIR}/images/stm32f469-disco.dtb 0x08004000" \
> +					 -c "flash write_image erase ${OUTPUT_DIR}/images/xipImage 0x08010000" \
> +					 -c "reset run" \
> +					 -c "shutdown"
> +		;;
> +	"uboot")
> +		FLASH_COMMAND=
> +		${OUTPUT_DIR}/host/bin/openocd -f board/stm32f469discovery.cfg \
> +					 -c "init" \
> +					 -c "reset init" \
> +					 -c "flash probe 0" \
> +					 -c "flash info 0" \
> +					 -c "flash write_image erase ${OUTPUT_DIR}/images/u-boot.bin 0x08000000" \
> +					 -c "reset run" \
> +					 -c "shutdown"
> +	    ;;
> +	*)
> +		echo "Wrong build type. Please select from: xip, uboot"
> +		;;
> +esac
> diff --git a/board/stmicroelectronics/stm32f469-disco/linux/defconfig b/board/stmicroelectronics/stm32f469-disco/linux/defconfig
> new file mode 100644
> index 0000000000..2d0ce59b31
> --- /dev/null
> +++ b/board/stmicroelectronics/stm32f469-disco/linux/defconfig

 We normally call the linux config "linux.config", not "linux/defconfig".

> @@ -0,0 +1,120 @@
[snip]
> diff --git a/board/stmicroelectronics/stm32f469-disco/linux/stm32f469-disco.dts b/board/stmicroelectronics/stm32f469-disco/linux/stm32f469-disco.dts
> new file mode 100644
> index 0000000000..83fe71bbc6
> --- /dev/null
> +++ b/board/stmicroelectronics/stm32f469-disco/linux/stm32f469-disco.dts

 Please document in the commit message where you got this dts from. How does it
differ from the in-kernel one, and why can't you just patch the in-kernel one?

[snip]
>         chosen {
>                 bootargs = "root=/dev/ram fbcon=map:0";

 Not very important, but is the root=/dev/ram really needed? When we're using
initramfs, the kernel never looks at the root= parameter so it seems redundant.

>                 stdout-path = "serial0:115200n8";
>         };[snip]
> diff --git a/board/stmicroelectronics/stm32f469-disco/patches/afboot-stm32/0002-kernel-start-address.patch b/board/stmicroelectronics/stm32f469-disco/patches/afboot-stm32/0002-kernel-start-address.patch

 Where is 0001?

> new file mode 100644
> index 0000000000..614effa85b
> --- /dev/null
> +++ b/board/stmicroelectronics/stm32f469-disco/patches/afboot-stm32/0002-kernel-start-address.patch
> @@ -0,0 +1,21 @@
> +diff --git a/stm32f469i-disco.c b/stm32f469i-disco.c

 Patches should preferably be produced by "git format-patch -N" from a git clone
of the upstream afboot-stm32 repository. Concretely, they should have:

- a subject line
- a good commit message (describing why the patch is needed, also upstream
status if relevant)
- your Signed-off-by

Basically, just like a patch submitted to buildroot.

> +index 2da1f4b65f..96e4dff65e 100644
> +--- a/stm32f469i-disco.c
> ++++ b/stm32f469i-disco.c
> +@@ -6,6 +6,7 @@
> + #include "gpio.h"
> + #include "mpu.h"
> + 
> ++#define KERNEL_ADDR     0x08010000
> + #define CONFIG_HSE_HZ	8000000
> + #define CONFIG_PLL_M	8
> + #define CONFIG_PLL_N	360
> +@@ -85,7 +86,7 @@ static void fmc_wait_busy(void)
> + 
> + void start_kernel(void)
> + {
> +-	void (*kernel)(uint32_t reserved, uint32_t mach, uint32_t dt) = (void (*)(uint32_t, uint32_t, uint32_t))(0x08008000 | 1);
> ++	void (*kernel)(uint32_t reserved, uint32_t mach, uint32_t dt) = (void (*)(uint32_t, uint32_t, uint32_t))(KERNEL_ADDR | 1);
> + 
> + 	kernel(0, ~0UL, 0x08004000);
> + }
> diff --git a/board/stmicroelectronics/stm32f469-disco/patches/afboot-stm32/0003-no-mpu.patch b/board/stmicroelectronics/stm32f469-disco/patches/afboot-stm32/0003-no-mpu.patch
> new file mode 100644
> index 0000000000..bc66d2d0ef
> --- /dev/null
> +++ b/board/stmicroelectronics/stm32f469-disco/patches/afboot-stm32/0003-no-mpu.patch
> @@ -0,0 +1,22 @@
> +diff --git a/stm32f469i-disco.c b/stm32f469i-disco.c
> +index d4d0909831..03f823f288 100644
> +--- a/stm32f469i-disco.c
> ++++ b/stm32f469i-disco.c
> +@@ -127,7 +127,7 @@ int main(void)
> + 
> + 	int i;
> + 
> +-	mpu_config(0x0);
> ++	mpu_config(0xc0000000); 
> + 
> + 	if (*FLASH_CR & FLASH_CR_LOCK) {
> + 		*FLASH_KEYR = 0x45670123;
> +@@ -229,7 +229,7 @@ int main(void)
> + 	usart_setup(usart_base, 45000000);
> + 	usart_putch(usart_base, '.');
> + 
> +-	*SYSCFG_MEMRMP = 0x4;
> ++	/* *SYSCFG_MEMRMP = 0x4; */
> + 
> + 	start_kernel();
> + 
> diff --git a/configs/stm32f469_disco_xip_defconfig b/configs/stm32f469_disco_xip_defconfig
> new file mode 100644
> index 0000000000..81bdb0d6d6
> --- /dev/null
> +++ b/configs/stm32f469_disco_xip_defconfig
 The readme.txt should be updated to describe this new defconfig and the
differences between them, so people know which one to choose for their use case.

> @@ -0,0 +1,24 @@
> +BR2_arm=y
> +BR2_cortex_m4=y
> +BR2_GLOBAL_PATCH_DIR="board/stmicroelectronics/stm32f469-disco/patches"
> +BR2_KERNEL_HEADERS_5_13=y

 Please keep the default SAME_AS_KERNEL, and instead set

BR2_PACKAGE_HOST_LINUX_HEADERS_CUSTOM_5_13=y

 Also, it would be nice to bump stm32f469_disco_defconfig to use the same kernel
version.

> +# BR2_UCLIBC_INSTALL_UTILS is not set
> +BR2_GCC_VERSION_11_X=y

 Is GCC11 really needed? We normally want to keep the default GCC version in the
defconfigs, so they follow updates of GCC.

 If it si really needed, please document it in the commit message.

> +BR2_GCC_ENABLE_LTO=y

 I guess this really is needed - so please explain that in the commit message.

> +BR2_ROOTFS_POST_BUILD_SCRIPT="board/stmicroelectronics/common/stm32f4xx/stm32-post-build.sh"
> +BR2_LINUX_KERNEL=y

 Even though 5.13 is the default now, it's not going to stay that way. So you
need to set BR2_LINUX_KERNEL_CUSTOM_VERSION instead.

> +BR2_LINUX_KERNEL_USE_CUSTOM_CONFIG=y
> +BR2_LINUX_KERNEL_CUSTOM_CONFIG_FILE="board/stmicroelectronics/stm32f469-disco/linux/defconfig"
> +BR2_LINUX_KERNEL_IMAGE_TARGET_CUSTOM=y
> +BR2_LINUX_KERNEL_XZ=y
> +BR2_LINUX_KERNEL_IMAGE_TARGET_NAME="xipImage"
> +BR2_LINUX_KERNEL_DTS_SUPPORT=y
> +BR2_LINUX_KERNEL_INTREE_DTS_NAME="stm32f469-disco"
> +BR2_LINUX_KERNEL_CUSTOM_DTS_PATH="board/stmicroelectronics/stm32f469-disco/linux/stm32f469-disco.dts"
> +BR2_PACKAGE_BUSYBOX_CONFIG="package/busybox/busybox-minimal.config"
> +BR2_PACKAGE_ZLIB=y

 Why is zlib needed? That seems weird...

> +BR2_TARGET_ROOTFS_INITRAMFS=y
> +# BR2_TARGET_ROOTFS_TAR is not set
> +BR2_TARGET_AFBOOT_STM32=y
> +BR2_PACKAGE_HOST_OPENOCD=y
> +BR2_PACKAGE_HOST_UTIL_LINUX=y

 I also don't see where util-linux is used, but I may have missed it.


 I have marked the patch as Changes Requested in patchwork.

 Regards,
 Arnout


More information about the buildroot mailing list