Skip to contentSkip to conversation

board/stm32f469-disco: update board configuration

PATCH In review Yauheni Saldatsenka 2 participants 5 messages Jul 23, 2021 – Aug 2, 2021 Arm STM32

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 board/stm32f469-disco: update board configuration

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

---
 .../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
board/stmicroelectronics/stm32f469-disco/flash.sh +38 -9
index 984d2b2599..176e1c9d2c 100755
@@ -1,18 +1,47 @@
 #!/bin/bash
 
 OUTPUT_DIR=$1
+BUILD_TYPE=$2
 
-if ! test -d "${OUTPUT_DIR}" ; then
+if ! test -d "${OUTPUT_DIR}"
+then
     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
board/stmicroelectronics/stm32f469-disco/linux/defconfig new file +120 -0

120 lines not on this page · raw message

board/stmicroelectronics/stm32f469-disco/linux/stm32f469-disco.dts new file +240 -0

240 lines not on this page · raw message

board/stmicroelectronics/stm32f469-disco/patches/afboot-stm32/0002-kernel-start-address.patch new file +21 -0
new file mode 100644
index 0000000000..614effa85b
@@ -0,0 +1,21 @@
+diff --git a/stm32f469i-disco.c b/stm32f469i-disco.c
+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);
+ }
board/stmicroelectronics/stm32f469-disco/patches/afboot-stm32/0003-no-mpu.patch new file +22 -0
new file mode 100644
index 0000000000..bc66d2d0ef
@@ -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();
+ 
configs/stm32f469_disco_xip_defconfig new file +24 -0
new file mode 100644
index 0000000000..81bdb0d6d6
@@ -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
+# BR2_UCLIBC_INSTALL_UTILS is not set
+BR2_GCC_VERSION_11_X=y
+BR2_GCC_ENABLE_LTO=y
+BR2_ROOTFS_POST_BUILD_SCRIPT="board/stmicroelectronics/common/stm32f4xx/stm32-post-build.sh"
+BR2_LINUX_KERNEL=y
+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
+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
Signature · 6 lines
2.32.0

_______________________________________________
buildroot mailing list
[email protected]
http://lists.busybox.net/mailman/listinfo/buildroot
#1 lore raw

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/[email protected]/
[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

Quoted 31 lines · Yauheni Saldatsenka

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

---
 .../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
board/stmicroelectronics/stm32f469-disco/flash.sh +0 -0
index 984d2b2599..176e1c9d2c 100755

board/stmicroelectronics/stm32f469-disco/flash.sh line 4

Quoted 4 lines
board/stmicroelectronics/stm32f469-disco/flash.sh +1 -0
@@ -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).

board/stmicroelectronics/stm32f469-disco/flash.sh line 7

Quoted 4 lines
board/stmicroelectronics/stm32f469-disco/flash.sh +2 -1
-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.

board/stmicroelectronics/stm32f469-disco/linux/defconfig

Quoted 53 lines
board/stmicroelectronics/stm32f469-disco/flash.sh +35 -8
     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
board/stmicroelectronics/stm32f469-disco/linux/defconfig new file +0 -0
new file mode 100644
index 0000000000..2d0ce59b31

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

Quoted 1 line
board/stmicroelectronics/stm32f469-disco/linux/defconfig new file +0 -0
@@ -0,0 +1,120 @@

[snip]

board/stmicroelectronics/stm32f469-disco/linux/stm32f469-disco.dts

Quoted 5 lines
board/stmicroelectronics/stm32f469-disco/linux/stm32f469-disco.dts new file +0 -0
new file mode 100644
index 0000000000..83fe71bbc6

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]

Quoted 2 lines
        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.

board/stmicroelectronics/stm32f469-disco/patches/afboot-stm32/0002-kernel-start-address.patch

Quoted 3 lines
                stdout-path = "serial0:115200n8";
        };[snip]
board/stmicroelectronics/stm32f469-disco/patches/afboot-stm32/0002-kernel-start-address.patch +0 -0

Where is 0001?

board/stmicroelectronics/stm32f469-disco/patches/afboot-stm32/0002-kernel-start-address.patch line 1

Quoted 6 lines

new file mode 100644
index 0000000000..614effa85b

board/stmicroelectronics/stm32f469-disco/patches/afboot-stm32/0002-kernel-start-address.patch new file +1 -0
@@ -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.

configs/stm32f469_disco_xip_defconfig

Quoted 53 lines
board/stmicroelectronics/stm32f469-disco/patches/afboot-stm32/0002-kernel-start-address.patch new file +20 -0
+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);
+ }
board/stmicroelectronics/stm32f469-disco/patches/afboot-stm32/0003-no-mpu.patch new file +22 -0
new file mode 100644
index 0000000000..bc66d2d0ef
@@ -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();
+ 
configs/stm32f469_disco_xip_defconfig new file +0 -0
new file mode 100644
index 0000000000..81bdb0d6d6

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.

configs/stm32f469_disco_xip_defconfig line 4

Quoted 5 lines
configs/stm32f469_disco_xip_defconfig new file +4 -0
@@ -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.

configs/stm32f469_disco_xip_defconfig line 6

Quoted 2 lines
configs/stm32f469_disco_xip_defconfig new file +2 -0
+# 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.

configs/stm32f469_disco_xip_defconfig line 7

Quoted 1 line
configs/stm32f469_disco_xip_defconfig new file +1 -0
+BR2_GCC_ENABLE_LTO=y

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

configs/stm32f469_disco_xip_defconfig line 9

Quoted 2 lines
configs/stm32f469_disco_xip_defconfig new file +2 -0
+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.

configs/stm32f469_disco_xip_defconfig line 19

Quoted 10 lines
configs/stm32f469_disco_xip_defconfig new file +10 -0
+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...

configs/stm32f469_disco_xip_defconfig line 24

Quoted 5 lines
configs/stm32f469_disco_xip_defconfig new file +5 -0
+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
_______________________________________________
buildroot mailing list
[email protected]
http://lists.busybox.net/mailman/listinfo/buildroot

#2 lore raw

Hi Arnout!

Thanks for the review, nice to know that code quality is kept high.

Some background information.
My patch is something that I was doing in February and shelved until this month.
I've sent the previous revision to Christophe and we agreed that it would be nice to have two system configurations: u-boot and xip.

So if you would have more ideological and motivational questions please feel free to ask.

Now to the questions.

First of all, there are a few other patches posted by Dario (in Cc] [1][2].

Quoted 3 lines

How
does this relate to them?

I only saw a previous commit of Dario with u-boot, not very aware of these two.

confgis/stm32f469_disco_xip_defconfig: alternative defconfig for

Quoted 3 lines

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

Agree.

Also, this change seems unrelated to the rest of the patch, so it should be

Quoted 5 lines

in
a separate patch (with a good explanation in the commit message as to why it is
needed).

Do you mean that the update of flash script should be a separate patch included in the patchset?

Quoted 9 lines

board/stmicroelectronics/stm32f469-disco/flash.sh line 7

Quoted 4 lines
board/stmicroelectronics/stm32f469-disco/flash.sh +2 -1
-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.

Is this buildroot bash codestyle?

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

Quoted 1 line

Okay, I see.

Please document in the commit message where you got this dts from. How

Quoted 4 lines

does it
differ from the in-kernel one, and why can't you just patch the in-kernel one?

I can use the kernel one with patch from my side.

Quoted 7 lines
        chosen {
Quoted 1 line
                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.

Honestly I'm not quite aware of that. If it would boot without then I'll remove this param.

Quoted 5 lines

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?

Probably dead during experiments. I'll rename this one to 0001

Patches should preferably be produced by "git format-patch -N" from a git

Quoted 10 lines

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.

The readme.txt should be updated to describe this new defconfig and the

Quoted 3 lines

differences between them, so people know which one to choose for their use case.

Agree.

Please keep the default SAME_AS_KERNEL, and instead set

Quoted 7 lines

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.

Okay, not a problem.

Is GCC11 really needed? We normally want to keep the default GCC version in

Quoted 6 lines

the
defconfigs, so they follow updates of GCC.

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

Nope, I've just chosen it to have latest optimization trick available.

Can be easily changed to default one.

Quoted 3 lines

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

Yep, sure.
LTO saves a bit of footprint, i can comment it out in commit message if needed.

Quoted 8 lines

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

Quoted 1 line

+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.

What do you mean here?

Quoted 16 lines

+BR2_LINUX_KERNEL_USE_CUSTOM_CONFIG=y

Quoted 1 line

+BR2_LINUX_KERNEL_CUSTOM_CONFIG_FILE="board/stmicroelectronics/stm32f469-disco/linux/defconfig"

configs/stm32f469_disco_xip_defconfig line 16

Quoted 6 lines
configs/stm32f469_disco_xip_defconfig new file +5 -0
+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"

configs/stm32f469_disco_xip_defconfig line 19

Quoted 2 lines
configs/stm32f469_disco_xip_defconfig new file +2 -0
+BR2_PACKAGE_BUSYBOX_CONFIG="package/busybox/busybox-minimal.config"
+BR2_PACKAGE_ZLIB=y

Why is zlib needed? That seems weird...

Quoted 8 lines

+BR2_TARGET_ROOTFS_INITRAMFS=y

configs/stm32f469_disco_xip_defconfig line 24

Quoted 4 lines
configs/stm32f469_disco_xip_defconfig new file +4 -0
+# 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.

Yep, looks strange.
I've intentionally disabled compression methods.
Needs some investigation.

So.
After all discussion points would be resolved, what are the next steps? Should I send the patchset directly to you and Christophe via git send-email or there is some other workflow?

Best regards,
Yauheni Saldatsenka

сб, 24 июл. 2021 г. в 00:37, Arnout Vandecappelle <[email protected]>:

Quoted 321 lines

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/[email protected]/
[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

Quoted 12 lines · Yauheni Saldatsenka

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

Quoted 9 lines
---
 .../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

Quoted 1 line
 create mode 100644

board/stmicroelectronics/stm32f469-disco/linux/stm32f469-disco.dts

Quoted 1 line
 create mode 100644

board/stmicroelectronics/stm32f469-disco/patches/afboot-stm32/0002-kernel-start-address.patch

Quoted 1 line
 create mode 100644

board/stmicroelectronics/stm32f469-disco/patches/afboot-stm32/0003-no-mpu.patch

Review

Quoted 3 lines
 create mode 100644 configs/stm32f469_disco_xip_defconfig
(quoted diff) +0 -0

b/board/stmicroelectronics/stm32f469-disco/flash.sh

board/stmicroelectronics/stm32f469-disco/flash.sh line 4

Quoted 8 lines

index 984d2b2599..176e1c9d2c 100755

board/stmicroelectronics/stm32f469-disco/flash.sh +1 -0
@@ -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).

board/stmicroelectronics/stm32f469-disco/flash.sh line 7

Quoted 4 lines
board/stmicroelectronics/stm32f469-disco/flash.sh +2 -1
-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.

board/stmicroelectronics/stm32f469-disco/flash.sh

Quoted 11 lines
board/stmicroelectronics/stm32f469-disco/flash.sh +0 -6
     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" \

board/stmicroelectronics/stm32f469-disco/flash.sh

Quoted 5 lines
board/stmicroelectronics/stm32f469-disco/flash.sh +3 -2
-  -c "reset run" \
-  -c "shutdown"
+if  [[ -z "${BUILD_TYPE}" ]]
+then
+    echo "ERROR: no build type specified, please select 'xip' or

'uboot'."

board/stmicroelectronics/stm32f469-disco/flash.sh

Quoted 7 lines
board/stmicroelectronics/stm32f469-disco/flash.sh +7 -0
+    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 \

board/stmicroelectronics/stm32f469-disco/flash.sh

Quoted 5 lines
board/stmicroelectronics/stm32f469-disco/flash.sh +5 -0
+                                      -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" \

board/stmicroelectronics/stm32f469-disco/flash.sh line 27

Quoted 1 line
board/stmicroelectronics/stm32f469-disco/flash.sh +1 -0
+                                      -c "flash write_image erase

${OUTPUT_DIR}/images/stm32f469-disco.dtb 0x08004000" \

board/stmicroelectronics/stm32f469-disco/flash.sh line 28

Quoted 1 line
board/stmicroelectronics/stm32f469-disco/flash.sh +1 -0
+                                      -c "flash write_image erase

${OUTPUT_DIR}/images/xipImage 0x08010000" \

board/stmicroelectronics/stm32f469-disco/flash.sh

Quoted 6 lines
board/stmicroelectronics/stm32f469-disco/flash.sh +6 -0
+                                      -c "reset run" \
+                                      -c "shutdown"
+             ;;
+     "uboot")
+             FLASH_COMMAND=
+             ${OUTPUT_DIR}/host/bin/openocd -f

board/stm32f469discovery.cfg \

board/stmicroelectronics/stm32f469-disco/flash.sh

Quoted 5 lines
board/stmicroelectronics/stm32f469-disco/flash.sh +5 -0
+                                      -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" \

Review

Quoted 8 lines
board/stmicroelectronics/stm32f469-disco/flash.sh +7 -0
+                                      -c "reset run" \
+                                      -c "shutdown"
+         ;;
+     *)
+             echo "Wrong build type. Please select from: xip, uboot"
+             ;;
+esac
(quoted diff) +0 -0

b/board/stmicroelectronics/stm32f469-disco/linux/defconfig

board/stmicroelectronics/stm32f469-disco/linux/defconfig

Quoted 4 lines

new file mode 100644
index 0000000000..2d0ce59b31

board/stmicroelectronics/stm32f469-disco/linux/defconfig new file +0 -0

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

Quoted 1 line
board/stmicroelectronics/stm32f469-disco/linux/defconfig new file +0 -0
@@ -0,0 +1,120 @@

[snip]

Quoted 1 line

diff --git

a/board/stmicroelectronics/stm32f469-disco/linux/stm32f469-disco.dts b/board/stmicroelectronics/stm32f469-disco/linux/stm32f469-disco.dts

board/stmicroelectronics/stm32f469-disco/linux/stm32f469-disco.dts

Quoted 4 lines

new file mode 100644
index 0000000000..83fe71bbc6

board/stmicroelectronics/stm32f469-disco/linux/stm32f469-disco.dts new file +0 -0

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]

Quoted 2 lines
        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.

Quoted 3 lines
                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?

Quoted 4 lines

new file mode 100644
index 0000000000..614effa85b
--- /dev/null
+++

b/board/stmicroelectronics/stm32f469-disco/patches/afboot-stm32/0002-kernel-start-address.patch

board/stmicroelectronics/stm32f469-disco/linux/stm32f469-disco.dts line 1

Quoted 2 lines
board/stmicroelectronics/stm32f469-disco/linux/stm32f469-disco.dts new file +1 -0
@@ -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.

Quoted 6 lines
board/stmicroelectronics/stm32f469-disco/linux/stm32f469-disco.dts new file +6 -0
+index 2da1f4b65f..96e4dff65e 100644
+--- a/stm32f469i-disco.c
++++ b/stm32f469i-disco.c
+@@ -6,6 +6,7 @@
+ #include "gpio.h"
+ #include "mpu.h"

board/stmicroelectronics/stm32f469-disco/linux/stm32f469-disco.dts line 17

Quoted 10 lines
board/stmicroelectronics/stm32f469-disco/linux/stm32f469-disco.dts new file +10 -0
+
++#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);

board/stmicroelectronics/stm32f469-disco/linux/stm32f469-disco.dts line 18

Quoted 1 line
board/stmicroelectronics/stm32f469-disco/linux/stm32f469-disco.dts new file +1 -0
++    void (*kernel)(uint32_t reserved, uint32_t mach, uint32_t dt) =

(void (*)(uint32_t, uint32_t, uint32_t))(KERNEL_ADDR | 1);

Quoted 4 lines
board/stmicroelectronics/stm32f469-disco/linux/stm32f469-disco.dts new file +3 -0
+
+     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

Quoted 4 lines

new file mode 100644
index 0000000000..bc66d2d0ef
--- /dev/null
+++

b/board/stmicroelectronics/stm32f469-disco/patches/afboot-stm32/0003-no-mpu.patch

Review

Quoted 24 lines
board/stmicroelectronics/stm32f469-disco/linux/stm32f469-disco.dts new file +22 -0
@@ -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();
+
(quoted diff) +0 -0

b/configs/stm32f469_disco_xip_defconfig

configs/stm32f469_disco_xip_defconfig

Quoted 4 lines

new file mode 100644
index 0000000000..81bdb0d6d6

configs/stm32f469_disco_xip_defconfig new file +0 -0

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.

configs/stm32f469_disco_xip_defconfig line 4

Quoted 5 lines
configs/stm32f469_disco_xip_defconfig new file +4 -0
@@ -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.

configs/stm32f469_disco_xip_defconfig line 6

Quoted 2 lines
configs/stm32f469_disco_xip_defconfig new file +2 -0
+# 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.

configs/stm32f469_disco_xip_defconfig line 7

Quoted 1 line
configs/stm32f469_disco_xip_defconfig new file +1 -0
+BR2_GCC_ENABLE_LTO=y

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

Quoted 1 line

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

Quoted 1 line

+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.

Quoted 2 lines

+BR2_LINUX_KERNEL_USE_CUSTOM_CONFIG=y

+BR2_LINUX_KERNEL_CUSTOM_CONFIG_FILE="board/stmicroelectronics/stm32f469-disco/linux/defconfig"

configs/stm32f469_disco_xip_defconfig line 16

Quoted 6 lines
configs/stm32f469_disco_xip_defconfig new file +5 -0
+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"

configs/stm32f469_disco_xip_defconfig line 19

Quoted 2 lines
configs/stm32f469_disco_xip_defconfig new file +2 -0
+BR2_PACKAGE_BUSYBOX_CONFIG="package/busybox/busybox-minimal.config"
+BR2_PACKAGE_ZLIB=y

Why is zlib needed? That seems weird...

configs/stm32f469_disco_xip_defconfig line 24

Quoted 5 lines
configs/stm32f469_disco_xip_defconfig new file +5 -0
+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.

Quoted 5 lines

I have marked the patch as Changes Requested in patchwork.

Regards,
Arnout

_______________________________________________
buildroot mailing list
[email protected]
http://lists.busybox.net/mailman/listinfo/buildroot

#3 lore raw

Hi Arnout!
A friendly reminder

Best regards,
Yauheni Saldatsenka

пн, 26 июл. 2021 г. в 19:45, Yauheni Saldatsenka <[email protected]>:

Quoted 191 lines

Hi Arnout!

Thanks for the review, nice to know that code quality is kept high.

Some background information.
My patch is something that I was doing in February and shelved until this month.
I've sent the previous revision to Christophe and we agreed that it would be nice to have two system configurations: u-boot and xip.

So if you would have more ideological and motivational questions please feel free to ask.

Now to the questions.

First of all, there are a few other patches posted by Dario (in Cc]

Quoted 3 lines

[1][2]. How
does this relate to them?

I only saw a previous commit of Dario with u-boot, not very aware of these two.

confgis/stm32f469_disco_xip_defconfig: alternative defconfig for

Quoted 3 lines

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

Agree.

Also, this change seems unrelated to the rest of the patch, so it should

Quoted 5 lines

be in
a separate patch (with a good explanation in the commit message as to why it is
needed).

Do you mean that the update of flash script should be a separate patch included in the patchset?

Quoted 9 lines

board/stmicroelectronics/stm32f469-disco/flash.sh line 7

Quoted 4 lines
board/stmicroelectronics/stm32f469-disco/flash.sh +2 -1
-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.

Is this buildroot bash codestyle?

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

Quoted 1 line

Okay, I see.

Please document in the commit message where you got this dts from. How

Quoted 4 lines

does it
differ from the in-kernel one, and why can't you just patch the in-kernel one?

I can use the kernel one with patch from my side.

Quoted 7 lines
        chosen {
Quoted 1 line
                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.

Honestly I'm not quite aware of that. If it would boot without then I'll remove this param.

Quoted 5 lines

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?

Probably dead during experiments. I'll rename this one to 0001

Patches should preferably be produced by "git format-patch -N" from a git

Quoted 10 lines

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.

The readme.txt should be updated to describe this new defconfig and the

Quoted 3 lines

differences between them, so people know which one to choose for their use case.

Agree.

Please keep the default SAME_AS_KERNEL, and instead set

Quoted 7 lines

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.

Okay, not a problem.

Is GCC11 really needed? We normally want to keep the default GCC version

Quoted 6 lines

in the
defconfigs, so they follow updates of GCC.

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

Nope, I've just chosen it to have latest optimization trick available.

Can be easily changed to default one.

Quoted 3 lines

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

Yep, sure.
LTO saves a bit of footprint, i can comment it out in commit message if needed.

Quoted 8 lines

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

Quoted 1 line

+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.

What do you mean here?

Quoted 16 lines

+BR2_LINUX_KERNEL_USE_CUSTOM_CONFIG=y

Quoted 1 line

+BR2_LINUX_KERNEL_CUSTOM_CONFIG_FILE="board/stmicroelectronics/stm32f469-disco/linux/defconfig"

configs/stm32f469_disco_xip_defconfig line 16

Quoted 6 lines
configs/stm32f469_disco_xip_defconfig new file +5 -0
+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"

configs/stm32f469_disco_xip_defconfig line 19

Quoted 2 lines
configs/stm32f469_disco_xip_defconfig new file +2 -0
+BR2_PACKAGE_BUSYBOX_CONFIG="package/busybox/busybox-minimal.config"
+BR2_PACKAGE_ZLIB=y

Why is zlib needed? That seems weird...

Quoted 8 lines

+BR2_TARGET_ROOTFS_INITRAMFS=y

configs/stm32f469_disco_xip_defconfig line 24

Quoted 4 lines
configs/stm32f469_disco_xip_defconfig new file +4 -0
+# 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.

Yep, looks strange.
I've intentionally disabled compression methods.
Needs some investigation.

So.
After all discussion points would be resolved, what are the next steps? Should I send the patchset directly to you and Christophe via git send-email or there is some other workflow?

Best regards,
Yauheni Saldatsenka

Quoted 329 lines

сб, 24 июл. 2021 г. в 00:37, Arnout Vandecappelle <[email protected]>:

Quoted 326 lines

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/[email protected]/
[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

Quoted 12 lines · Yauheni Saldatsenka

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

Quoted 9 lines
---
 .../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

Quoted 1 line
 create mode 100644

board/stmicroelectronics/stm32f469-disco/linux/stm32f469-disco.dts

Quoted 1 line
 create mode 100644

board/stmicroelectronics/stm32f469-disco/patches/afboot-stm32/0002-kernel-start-address.patch

Quoted 1 line
 create mode 100644

board/stmicroelectronics/stm32f469-disco/patches/afboot-stm32/0003-no-mpu.patch

Review

Quoted 3 lines
 create mode 100644 configs/stm32f469_disco_xip_defconfig
(quoted diff) +0 -0

b/board/stmicroelectronics/stm32f469-disco/flash.sh

board/stmicroelectronics/stm32f469-disco/flash.sh line 4

Quoted 8 lines

index 984d2b2599..176e1c9d2c 100755

board/stmicroelectronics/stm32f469-disco/flash.sh +1 -0
@@ -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).

board/stmicroelectronics/stm32f469-disco/flash.sh line 7

Quoted 4 lines
board/stmicroelectronics/stm32f469-disco/flash.sh +2 -1
-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.

board/stmicroelectronics/stm32f469-disco/flash.sh

Quoted 11 lines
board/stmicroelectronics/stm32f469-disco/flash.sh +0 -6
     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" \

board/stmicroelectronics/stm32f469-disco/flash.sh

Quoted 5 lines
board/stmicroelectronics/stm32f469-disco/flash.sh +3 -2
-  -c "reset run" \
-  -c "shutdown"
+if  [[ -z "${BUILD_TYPE}" ]]
+then
+    echo "ERROR: no build type specified, please select 'xip' or

'uboot'."

board/stmicroelectronics/stm32f469-disco/flash.sh

Quoted 7 lines
board/stmicroelectronics/stm32f469-disco/flash.sh +7 -0
+    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 \

board/stmicroelectronics/stm32f469-disco/flash.sh

Quoted 5 lines
board/stmicroelectronics/stm32f469-disco/flash.sh +5 -0
+                                      -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" \

board/stmicroelectronics/stm32f469-disco/flash.sh line 27

Quoted 1 line
board/stmicroelectronics/stm32f469-disco/flash.sh +1 -0
+                                      -c "flash write_image erase

${OUTPUT_DIR}/images/stm32f469-disco.dtb 0x08004000" \

board/stmicroelectronics/stm32f469-disco/flash.sh line 28

Quoted 1 line
board/stmicroelectronics/stm32f469-disco/flash.sh +1 -0
+                                      -c "flash write_image erase

${OUTPUT_DIR}/images/xipImage 0x08010000" \

board/stmicroelectronics/stm32f469-disco/flash.sh

Quoted 6 lines
board/stmicroelectronics/stm32f469-disco/flash.sh +6 -0
+                                      -c "reset run" \
+                                      -c "shutdown"
+             ;;
+     "uboot")
+             FLASH_COMMAND=
+             ${OUTPUT_DIR}/host/bin/openocd -f

board/stm32f469discovery.cfg \

board/stmicroelectronics/stm32f469-disco/flash.sh

Quoted 5 lines
board/stmicroelectronics/stm32f469-disco/flash.sh +5 -0
+                                      -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" \

Review

Quoted 8 lines
board/stmicroelectronics/stm32f469-disco/flash.sh +7 -0
+                                      -c "reset run" \
+                                      -c "shutdown"
+         ;;
+     *)
+             echo "Wrong build type. Please select from: xip, uboot"
+             ;;
+esac
(quoted diff) +0 -0

b/board/stmicroelectronics/stm32f469-disco/linux/defconfig

board/stmicroelectronics/stm32f469-disco/linux/defconfig

Quoted 4 lines

new file mode 100644
index 0000000000..2d0ce59b31

board/stmicroelectronics/stm32f469-disco/linux/defconfig new file +0 -0

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

Quoted 1 line
board/stmicroelectronics/stm32f469-disco/linux/defconfig new file +0 -0
@@ -0,0 +1,120 @@

[snip]

Quoted 1 line

diff --git

a/board/stmicroelectronics/stm32f469-disco/linux/stm32f469-disco.dts b/board/stmicroelectronics/stm32f469-disco/linux/stm32f469-disco.dts

board/stmicroelectronics/stm32f469-disco/linux/stm32f469-disco.dts

Quoted 4 lines

new file mode 100644
index 0000000000..83fe71bbc6

board/stmicroelectronics/stm32f469-disco/linux/stm32f469-disco.dts new file +0 -0

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]

Quoted 2 lines
        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.

Quoted 3 lines
                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?

Quoted 4 lines

new file mode 100644
index 0000000000..614effa85b
--- /dev/null
+++

b/board/stmicroelectronics/stm32f469-disco/patches/afboot-stm32/0002-kernel-start-address.patch

board/stmicroelectronics/stm32f469-disco/linux/stm32f469-disco.dts line 1

Quoted 2 lines
board/stmicroelectronics/stm32f469-disco/linux/stm32f469-disco.dts new file +1 -0
@@ -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.

Quoted 6 lines
board/stmicroelectronics/stm32f469-disco/linux/stm32f469-disco.dts new file +6 -0
+index 2da1f4b65f..96e4dff65e 100644
+--- a/stm32f469i-disco.c
++++ b/stm32f469i-disco.c
+@@ -6,6 +6,7 @@
+ #include "gpio.h"
+ #include "mpu.h"

board/stmicroelectronics/stm32f469-disco/linux/stm32f469-disco.dts line 17

Quoted 10 lines
board/stmicroelectronics/stm32f469-disco/linux/stm32f469-disco.dts new file +10 -0
+
++#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);

board/stmicroelectronics/stm32f469-disco/linux/stm32f469-disco.dts line 18

Quoted 1 line
board/stmicroelectronics/stm32f469-disco/linux/stm32f469-disco.dts new file +1 -0
++    void (*kernel)(uint32_t reserved, uint32_t mach, uint32_t dt) =

(void (*)(uint32_t, uint32_t, uint32_t))(KERNEL_ADDR | 1);

Quoted 4 lines
board/stmicroelectronics/stm32f469-disco/linux/stm32f469-disco.dts new file +3 -0
+
+     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

Quoted 4 lines

new file mode 100644
index 0000000000..bc66d2d0ef
--- /dev/null
+++

b/board/stmicroelectronics/stm32f469-disco/patches/afboot-stm32/0003-no-mpu.patch

Review

Quoted 24 lines
board/stmicroelectronics/stm32f469-disco/linux/stm32f469-disco.dts new file +22 -0
@@ -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();
+
(quoted diff) +0 -0

b/configs/stm32f469_disco_xip_defconfig

configs/stm32f469_disco_xip_defconfig

Quoted 4 lines

new file mode 100644
index 0000000000..81bdb0d6d6

configs/stm32f469_disco_xip_defconfig new file +0 -0

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.

configs/stm32f469_disco_xip_defconfig line 4

Quoted 5 lines
configs/stm32f469_disco_xip_defconfig new file +4 -0
@@ -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.

configs/stm32f469_disco_xip_defconfig line 6

Quoted 2 lines
configs/stm32f469_disco_xip_defconfig new file +2 -0
+# 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.

configs/stm32f469_disco_xip_defconfig line 7

Quoted 1 line
configs/stm32f469_disco_xip_defconfig new file +1 -0
+BR2_GCC_ENABLE_LTO=y

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

Quoted 1 line

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

Quoted 1 line

+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.

Quoted 2 lines

+BR2_LINUX_KERNEL_USE_CUSTOM_CONFIG=y

+BR2_LINUX_KERNEL_CUSTOM_CONFIG_FILE="board/stmicroelectronics/stm32f469-disco/linux/defconfig"

configs/stm32f469_disco_xip_defconfig line 16

Quoted 6 lines
configs/stm32f469_disco_xip_defconfig new file +5 -0
+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"

configs/stm32f469_disco_xip_defconfig line 19

Quoted 2 lines
configs/stm32f469_disco_xip_defconfig new file +2 -0
+BR2_PACKAGE_BUSYBOX_CONFIG="package/busybox/busybox-minimal.config"
+BR2_PACKAGE_ZLIB=y

Why is zlib needed? That seems weird...

configs/stm32f469_disco_xip_defconfig line 24

Quoted 5 lines
configs/stm32f469_disco_xip_defconfig new file +5 -0
+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

_______________________________________________
buildroot mailing list
[email protected]
http://lists.busybox.net/mailman/listinfo/buildroot

#4 lore raw
In reply to Yauheni Saldatsenka
Quoted 26 lines · Yauheni Saldatsenka

Hi Arnout!

Thanks for the review, nice to know that code quality is kept high.

Some background information.
My patch is something that I was doing in February and shelved until this month. I've sent the previous revision to Christophe and we agreed that it would be nice to have two system configurations: u-boot and xip.

So if you would have more ideological and motivational questions please feel free to ask.

Now to the questions.

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

I only saw a previous commit of Dario with u-boot, not very aware of these two.

confgis/stm32f469_disco_xip_defconfig: alternative defconfig for stm32f469_disco
This change in the flash script will need a corresponding change in the readme.txt.

Quoted 8 lines

Agree.

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).

Do you mean that the update of flash script should be a separate patch included in the patchset?

Yes indeed that's what I mean.

Quoted 10 lines

board/stmicroelectronics/stm32f469-disco/flash.sh line 7

Quoted 5 lines

board/stmicroelectronics/stm32f469-disco/flash.sh +2 -1
-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.

Is this buildroot bash codestyle?

We don't have a formally defined coding style for shell. If in doubt, I usually
use grep to do a popularity contest of the alternatives.

$ git grep '^[[:space:]]*if ' -- \*.sh \*.mk Makefile\* | wc
    247    2175   19947
$ git grep '^[[:space:]]*if .*; then' -- \*.sh \*.mk Makefile\* | wc
    231    2047   18722

so "if ...; then" seems to be pretty popular.

Quoted 10 lines

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

Okay, I see.

 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?

I can use the kernel one with patch from my side.

Note that I don't necessarily want you to do that - the important thing is that
the commit message explains why. So when someone later on wants to e.g. update the kernel, they have an idea about what to do with the dts file.

If the changes are small, then obviously a patch is better.

Quoted 62 lines
Quoted 3 lines

         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.

Honestly I'm not quite aware of that. If it would boot without then I'll remove this param.

Quoted 1 line

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?

Probably dead during experiments. I'll rename this one to 0001

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.

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.

Agree.

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.

Okay, not a problem.

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.

Nope, I've just chosen it to have latest optimization trick available. Can be easily changed to default one.

Quoted 1 line

+BR2_GCC_ENABLE_LTO=y

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

Yep, sure.
LTO saves a bit of footprint, i can comment it out in commit message if needed.

Quoted 1 line
Quoted 7 lines

+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.

What do you mean here?

If you don't specify the kernel version explicitly, then when the default
kernel version is updated in the future, this board config will start using the newer version. However, that version was never tested so it may fail. In addition, it's not unlikely that the config fragment and dts are no longer entirely correct.

So the board config needs to have a fixed kernel version, specified with
BR2_LINUX_KERNEL_CUSTOM_VERSION.

Quoted 28 lines

configs/stm32f469_disco_xip_defconfig line 19

Quoted 11 lines
configs/stm32f469_disco_xip_defconfig new file +10 -0
+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.


Yep, looks strange.
I've intentionally disabled compression methods.
Needs some investigation.

Quoted 4 lines

So.
After all discussion points would be resolved, what are the next steps? Should I send the patchset directly to you and Christophe via git send-email or there is some other workflow?

Just send it to the list with v2 in the subject and the people involved in Cc.
You should also add a patch version log to the commit message, like so:

[PATCH v2 1/1] configs/stm32f469_disco_xip_defconfig: alternative defconfig for XIP

... Commit message ...

Signed-off-by ...

---
v1 -> v2:
- Changed commit message
- Apply dts changes by patching the kernel instead of including a full dts copy
- ...

Regards,
Arnout

[snip]
_______________________________________________
buildroot mailing list
[email protected]
http://lists.busybox.net/mailman/listinfo/buildroot