board/stm32f469-disco: update board configuration
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
@@ -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
@@ -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
@@ -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
@@ -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
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 memoryFixes:
- 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 modeSigned-off-by Yauheni Saldatsenka <[email protected]>--- .../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_defconfigboard/stmicroelectronics/stm32f469-disco/flash.sh +0
-0
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
-8echo "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" + ;; +esacboard/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]
board/stmicroelectronics/stm32f469-disco/linux/stm32f469-disco.dts
Quoted 5 lines
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.
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..614effa85bboard/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@@ -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
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
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}" +thenSpurious 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.patchb/board/stmicroelectronics/stm32f469-disco/patches/afboot-stm32/0002-kernel-start-address.patchWhere 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-byBasically, 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_defconfigto 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 setBR2_LINUX_KERNEL_CUSTOM_VERSIONinstead.
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=yWhy 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=yI 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=253877The subject line of this patch should be:
confgis/stm32f469_disco_xip_defconfig: alternative defconfig for
stm32f469_discoQuoted 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 memoryFixes:
- 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 modeSigned-off-by Yauheni Saldatsenka <[email protected]>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 100644board/stmicroelectronics/stm32f469-disco/linux/defconfig
Quoted 1 line
create mode 100644
board/stmicroelectronics/stm32f469-disco/linux/stm32f469-disco.dtsQuoted 1 line
create mode 100644
board/stmicroelectronics/stm32f469-disco/patches/afboot-stm32/0002-kernel-start-address.patchQuoted 1 line
create mode 100644
board/stmicroelectronics/stm32f469-disco/patches/afboot-stm32/0003-no-mpu.patchReview
Quoted 3 lines
create mode 100644 configs/stm32f469_disco_xip_defconfig(quoted diff) +0
-0
b/board/stmicroelectronics/stm32f469-disco/flash.shboard/stmicroelectronics/stm32f469-disco/flash.sh line 4
Quoted 8 lines
index
984d2b2599..176e1c9d2c100755board/stmicroelectronics/stm32f469-disco/flash.sh +1
-0@@ -1,18 +1,47 @@ #!/bin/bash OUTPUT_DIR=$1 +BUILD_TYPE=$2This 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}" +thenSpurious 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
-6echo "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 -fboard/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 -fboard/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
-0b/board/stmicroelectronics/stm32f469-disco/linux/defconfig
board/stmicroelectronics/stm32f469-disco/linux/defconfig
Quoted 4 lines
new file mode 100644
index 0000000000..2d0ce59b31board/stmicroelectronics/stm32f469-disco/linux/defconfig new file +0
-0We 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.dtsb/board/stmicroelectronics/stm32f469-disco/linux/stm32f469-disco.dtsboard/stmicroelectronics/stm32f469-disco/linux/stm32f469-disco.dts
Quoted 4 lines
new file mode 100644
index 0000000000..83fe71bbc6board/stmicroelectronics/stm32f469-disco/linux/stm32f469-disco.dts new file +0
-0Please 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.patchb/board/stmicroelectronics/stm32f469-disco/patches/afboot-stm32/0002-kernel-start-address.patchWhere 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.patchboard/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.cPatches 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-byBasically, 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.patchb/board/stmicroelectronics/stm32f469-disco/patches/afboot-stm32/0003-no-mpu.patchQuoted 4 lines
new file mode 100644
index 0000000000..bc66d2d0ef
---/dev/null
+++
b/board/stmicroelectronics/stm32f469-disco/patches/afboot-stm32/0003-no-mpu.patchReview
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
-0b/configs/stm32f469_disco_xip_defconfig
configs/stm32f469_disco_xip_defconfig
Quoted 4 lines
new file mode 100644
index 0000000000..81bdb0d6d6configs/stm32f469_disco_xip_defconfig new file +0
-0The 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=yPlease keep the default
SAME_AS_KERNEL, and instead setBR2_PACKAGE_HOST_LINUX_HEADERS_CUSTOM_5_13=y
Also, it would be nice to bump
stm32f469_disco_defconfigto 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=yIs 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=yI 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 setBR2_LINUX_KERNEL_CUSTOM_VERSIONinstead.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=yWhy 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=yI 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
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}" +thenSpurious 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.patchb/board/stmicroelectronics/stm32f469-disco/patches/afboot-stm32/0002-kernel-start-address.patchWhere 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-byBasically, 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 setQuoted 7 lines
BR2_PACKAGE_HOST_LINUX_HEADERS_CUSTOM_5_13=y
Also, it would be nice to bump
stm32f469_disco_defconfigto 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 setBR2_LINUX_KERNEL_CUSTOM_VERSIONinstead.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=yWhy 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=yI 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=253877The subject line of this patch should be:
confgis/stm32f469_disco_xip_defconfig: alternative defconfig for
stm32f469_discoQuoted 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 memoryFixes:
- 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 modeSigned-off-by Yauheni Saldatsenka <[email protected]>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 100644board/stmicroelectronics/stm32f469-disco/linux/defconfig
Quoted 1 line
create mode 100644
board/stmicroelectronics/stm32f469-disco/linux/stm32f469-disco.dtsQuoted 1 line
create mode 100644
board/stmicroelectronics/stm32f469-disco/patches/afboot-stm32/0002-kernel-start-address.patchQuoted 1 line
create mode 100644
board/stmicroelectronics/stm32f469-disco/patches/afboot-stm32/0003-no-mpu.patchReview
Quoted 3 lines
create mode 100644 configs/stm32f469_disco_xip_defconfig(quoted diff) +0
-0
b/board/stmicroelectronics/stm32f469-disco/flash.shboard/stmicroelectronics/stm32f469-disco/flash.sh line 4
Quoted 8 lines
index
984d2b2599..176e1c9d2c100755board/stmicroelectronics/stm32f469-disco/flash.sh +1
-0@@ -1,18 +1,47 @@ #!/bin/bash OUTPUT_DIR=$1 +BUILD_TYPE=$2This 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}" +thenSpurious 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
-6echo "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 -fboard/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 -fboard/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
-0b/board/stmicroelectronics/stm32f469-disco/linux/defconfig
board/stmicroelectronics/stm32f469-disco/linux/defconfig
Quoted 4 lines
new file mode 100644
index 0000000000..2d0ce59b31board/stmicroelectronics/stm32f469-disco/linux/defconfig new file +0
-0We 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.dtsb/board/stmicroelectronics/stm32f469-disco/linux/stm32f469-disco.dtsboard/stmicroelectronics/stm32f469-disco/linux/stm32f469-disco.dts
Quoted 4 lines
new file mode 100644
index 0000000000..83fe71bbc6board/stmicroelectronics/stm32f469-disco/linux/stm32f469-disco.dts new file +0
-0Please 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.patchb/board/stmicroelectronics/stm32f469-disco/patches/afboot-stm32/0002-kernel-start-address.patchWhere 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.patchboard/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.cPatches 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-byBasically, 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.patchb/board/stmicroelectronics/stm32f469-disco/patches/afboot-stm32/0003-no-mpu.patchQuoted 4 lines
new file mode 100644
index 0000000000..bc66d2d0ef
---/dev/null
+++
b/board/stmicroelectronics/stm32f469-disco/patches/afboot-stm32/0003-no-mpu.patchReview
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
-0b/configs/stm32f469_disco_xip_defconfig
configs/stm32f469_disco_xip_defconfig
Quoted 4 lines
new file mode 100644
index 0000000000..81bdb0d6d6configs/stm32f469_disco_xip_defconfig new file +0
-0The 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=yPlease keep the default
SAME_AS_KERNEL, and instead setBR2_PACKAGE_HOST_LINUX_HEADERS_CUSTOM_5_13=y
Also, it would be nice to bump
stm32f469_disco_defconfigto 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=yIs 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=yI 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 setBR2_LINUX_KERNEL_CUSTOM_VERSIONinstead.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=yWhy 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=yI 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
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}" +thenSpurious 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 18722so "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-byBasically, 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 setBR2_PACKAGE_HOST_LINUX_HEADERS_CUSTOM_5_13=y
Also, it would be nice to bump
stm32f469_disco_defconfigto 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_VERSIONinstead.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 withBR2_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=yWhy is zlib needed? That seems weird...
> +BR2_TARGET_ROOTFS_INITRAMFS=y
> +#BR2_TARGET_ROOTFS_TARis not set
> +BR2_TARGET_AFBOOT_STM32=y
> +BR2_PACKAGE_HOST_OPENOCD=y
> +BR2_PACKAGE_HOST_UTIL_LINUX=yI 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 ...
---
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