[Buildroot] [PATCH 1/1] board/stm32f469-disco: update board configuration
Yauheni Saldatsenka
eugentoo at gmail.com
Mon Jul 26 16:45:59 UTC 2021
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.
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?
>
> > -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"
>
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.
> 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.
> 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.
> +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.
>
> +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?
> +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.
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 <arnout at mind.be>:
> Hi Yauheni,
>
> Thank you for this patch. I have a few comments that I unfortunately
> can't fix
> myself.
>
> First of all, there are a few other patches posted by Dario (in Cc]
> [1][2]. How
> does this relate to them?
>
> [1]
>
> https://patchwork.ozlabs.org/project/buildroot/patch/20210704141106.1746-2-dariobin@libero.it/
> [2] https://patchwork.ozlabs.org/project/buildroot/list/?series=253877
>
>
>
> The subject line of this patch should be:
>
> confgis/stm32f469_disco_xip_defconfig: alternative defconfig for
> stm32f469_disco
>
> On 23/07/2021 18:04, Yauheni Saldatsenka wrote:
> > Update STM32F469-disco configuration files to operate with new kernel.
> >
> > Result of make tinyconfig was taken as a starting point to fit kernel
> > into flash memory.
> > Current setup kernel + rootfs fits in 1.6MB on-chip flash memory
> >
> > Fixes:
> > - Move kernel to new flash bank due to growth of dtb size
> > - Fix kernel start address in bootloader
> > - Remove outdated path which doesn't affect normal operation mode
> >
> > Signed-off-by: Yauheni Saldatsenka <eugentoo at gmail.com>
> > ---
> > .../stm32f469-disco/flash.sh | 47 +++-
> > .../stm32f469-disco/linux/defconfig | 120 +++++++++
> > .../stm32f469-disco/linux/stm32f469-disco.dts | 240 ++++++++++++++++++
> > .../0002-kernel-start-address.patch | 21 ++
> > .../patches/afboot-stm32/0003-no-mpu.patch | 22 ++
> > configs/stm32f469_disco_xip_defconfig | 24 ++
> > 6 files changed, 465 insertions(+), 9 deletions(-)
> > create mode 100644
> board/stmicroelectronics/stm32f469-disco/linux/defconfig
> > create mode 100644
> board/stmicroelectronics/stm32f469-disco/linux/stm32f469-disco.dts
> > create mode 100644
> board/stmicroelectronics/stm32f469-disco/patches/afboot-stm32/0002-kernel-start-address.patch
> > create mode 100644
> board/stmicroelectronics/stm32f469-disco/patches/afboot-stm32/0003-no-mpu.patch
> > create mode 100644 configs/stm32f469_disco_xip_defconfig
> >
> > diff --git a/board/stmicroelectronics/stm32f469-disco/flash.sh
> b/board/stmicroelectronics/stm32f469-disco/flash.sh
> > index 984d2b2599..176e1c9d2c 100755
> > --- a/board/stmicroelectronics/stm32f469-disco/flash.sh
> > +++ b/board/stmicroelectronics/stm32f469-disco/flash.sh
> > @@ -1,18 +1,47 @@
> > #!/bin/bash
> >
> > OUTPUT_DIR=$1
> > +BUILD_TYPE=$2
>
> This change in the flash script will need a corresponding change in the
> readme.txt.
>
> Also, this change seems unrelated to the rest of the patch, so it should
> be in
> a separate patch (with a good explanation in the commit message as to why
> it is
> needed).
>
> >
> > -if ! test -d "${OUTPUT_DIR}" ; then
> > +if ! test -d "${OUTPUT_DIR}"
> > +then
>
> Spurious change - we normally write the if and the then on one line in
> shell
> scripts.
>
> > echo "ERROR: no output directory specified."
> > echo "Usage: $0 OUTPUT_DIR"
> > exit 1
> > fi
> >
> > -${OUTPUT_DIR}/host/bin/openocd -f board/stm32f469discovery.cfg \
> > - -c "init" \
> > - -c "reset init" \
> > - -c "flash probe 0" \
> > - -c "flash info 0" \
> > - -c "flash write_image erase ${OUTPUT_DIR}/images/u-boot.bin
> 0x08000000" \
> > - -c "reset run" \
> > - -c "shutdown"
> > +if [[ -z "${BUILD_TYPE}" ]]
> > +then
> > + echo "ERROR: no build type specified, please select 'xip' or
> 'uboot'."
> > + echo "Usage: $0 OUTPUT_DIR $1 BUILD_TYPE"
> > + exit 1
> > +fi
> > +
> > +case "${BUILD_TYPE}" in
> > + "xip")
> > + ${OUTPUT_DIR}/host/bin/openocd -f
> board/stm32f469discovery.cfg \
> > + -c "init" \
> > + -c "reset init" \
> > + -c "flash probe 0" \
> > + -c "flash info 0" \
> > + -c "flash write_image erase
> ${OUTPUT_DIR}/images/stm32f469i-disco.bin 0x08000000" \
> > + -c "flash write_image erase
> ${OUTPUT_DIR}/images/stm32f469-disco.dtb 0x08004000" \
> > + -c "flash write_image erase
> ${OUTPUT_DIR}/images/xipImage 0x08010000" \
> > + -c "reset run" \
> > + -c "shutdown"
> > + ;;
> > + "uboot")
> > + FLASH_COMMAND=
> > + ${OUTPUT_DIR}/host/bin/openocd -f
> board/stm32f469discovery.cfg \
> > + -c "init" \
> > + -c "reset init" \
> > + -c "flash probe 0" \
> > + -c "flash info 0" \
> > + -c "flash write_image erase
> ${OUTPUT_DIR}/images/u-boot.bin 0x08000000" \
> > + -c "reset run" \
> > + -c "shutdown"
> > + ;;
> > + *)
> > + echo "Wrong build type. Please select from: xip, uboot"
> > + ;;
> > +esac
> > diff --git a/board/stmicroelectronics/stm32f469-disco/linux/defconfig
> b/board/stmicroelectronics/stm32f469-disco/linux/defconfig
> > new file mode 100644
> > index 0000000000..2d0ce59b31
> > --- /dev/null
> > +++ b/board/stmicroelectronics/stm32f469-disco/linux/defconfig
>
> We normally call the linux config "linux.config", not "linux/defconfig".
>
> > @@ -0,0 +1,120 @@
> [snip]
> > diff --git
> a/board/stmicroelectronics/stm32f469-disco/linux/stm32f469-disco.dts
> b/board/stmicroelectronics/stm32f469-disco/linux/stm32f469-disco.dts
> > new file mode 100644
> > index 0000000000..83fe71bbc6
> > --- /dev/null
> > +++ b/board/stmicroelectronics/stm32f469-disco/linux/stm32f469-disco.dts
>
> Please document in the commit message where you got this dts from. How
> does it
> differ from the in-kernel one, and why can't you just patch the in-kernel
> one?
>
> [snip]
> > chosen {
> > bootargs = "root=/dev/ram fbcon=map:0";
>
> Not very important, but is the root=/dev/ram really needed? When we're
> using
> initramfs, the kernel never looks at the root= parameter so it seems
> redundant.
>
> > stdout-path = "serial0:115200n8";
> > };[snip]
> > diff --git
> a/board/stmicroelectronics/stm32f469-disco/patches/afboot-stm32/0002-kernel-start-address.patch
> b/board/stmicroelectronics/stm32f469-disco/patches/afboot-stm32/0002-kernel-start-address.patch
>
> Where is 0001?
>
> > new file mode 100644
> > index 0000000000..614effa85b
> > --- /dev/null
> > +++
> b/board/stmicroelectronics/stm32f469-disco/patches/afboot-stm32/0002-kernel-start-address.patch
> > @@ -0,0 +1,21 @@
> > +diff --git a/stm32f469i-disco.c b/stm32f469i-disco.c
>
> Patches should preferably be produced by "git format-patch -N" from a git
> clone
> of the upstream afboot-stm32 repository. Concretely, they should have:
>
> - a subject line
> - a good commit message (describing why the patch is needed, also upstream
> status if relevant)
> - your Signed-off-by
>
> Basically, just like a patch submitted to buildroot.
>
> > +index 2da1f4b65f..96e4dff65e 100644
> > +--- a/stm32f469i-disco.c
> > ++++ b/stm32f469i-disco.c
> > +@@ -6,6 +6,7 @@
> > + #include "gpio.h"
> > + #include "mpu.h"
> > +
> > ++#define KERNEL_ADDR 0x08010000
> > + #define CONFIG_HSE_HZ 8000000
> > + #define CONFIG_PLL_M 8
> > + #define CONFIG_PLL_N 360
> > +@@ -85,7 +86,7 @@ static void fmc_wait_busy(void)
> > +
> > + void start_kernel(void)
> > + {
> > +- void (*kernel)(uint32_t reserved, uint32_t mach, uint32_t dt) =
> (void (*)(uint32_t, uint32_t, uint32_t))(0x08008000 | 1);
> > ++ void (*kernel)(uint32_t reserved, uint32_t mach, uint32_t dt) =
> (void (*)(uint32_t, uint32_t, uint32_t))(KERNEL_ADDR | 1);
> > +
> > + kernel(0, ~0UL, 0x08004000);
> > + }
> > diff --git
> a/board/stmicroelectronics/stm32f469-disco/patches/afboot-stm32/0003-no-mpu.patch
> b/board/stmicroelectronics/stm32f469-disco/patches/afboot-stm32/0003-no-mpu.patch
> > new file mode 100644
> > index 0000000000..bc66d2d0ef
> > --- /dev/null
> > +++
> b/board/stmicroelectronics/stm32f469-disco/patches/afboot-stm32/0003-no-mpu.patch
> > @@ -0,0 +1,22 @@
> > +diff --git a/stm32f469i-disco.c b/stm32f469i-disco.c
> > +index d4d0909831..03f823f288 100644
> > +--- a/stm32f469i-disco.c
> > ++++ b/stm32f469i-disco.c
> > +@@ -127,7 +127,7 @@ int main(void)
> > +
> > + int i;
> > +
> > +- mpu_config(0x0);
> > ++ mpu_config(0xc0000000);
> > +
> > + if (*FLASH_CR & FLASH_CR_LOCK) {
> > + *FLASH_KEYR = 0x45670123;
> > +@@ -229,7 +229,7 @@ int main(void)
> > + usart_setup(usart_base, 45000000);
> > + usart_putch(usart_base, '.');
> > +
> > +- *SYSCFG_MEMRMP = 0x4;
> > ++ /* *SYSCFG_MEMRMP = 0x4; */
> > +
> > + start_kernel();
> > +
> > diff --git a/configs/stm32f469_disco_xip_defconfig
> b/configs/stm32f469_disco_xip_defconfig
> > new file mode 100644
> > index 0000000000..81bdb0d6d6
> > --- /dev/null
> > +++ b/configs/stm32f469_disco_xip_defconfig
> The readme.txt should be updated to describe this new defconfig and the
> differences between them, so people know which one to choose for their use
> case.
>
> > @@ -0,0 +1,24 @@
> > +BR2_arm=y
> > +BR2_cortex_m4=y
> > +BR2_GLOBAL_PATCH_DIR="board/stmicroelectronics/stm32f469-disco/patches"
> > +BR2_KERNEL_HEADERS_5_13=y
>
> Please keep the default SAME_AS_KERNEL, and instead set
>
> BR2_PACKAGE_HOST_LINUX_HEADERS_CUSTOM_5_13=y
>
> Also, it would be nice to bump stm32f469_disco_defconfig to use the same
> kernel
> version.
>
> > +# BR2_UCLIBC_INSTALL_UTILS is not set
> > +BR2_GCC_VERSION_11_X=y
>
> Is GCC11 really needed? We normally want to keep the default GCC version
> in the
> defconfigs, so they follow updates of GCC.
>
> If it si really needed, please document it in the commit message.
>
> > +BR2_GCC_ENABLE_LTO=y
>
> I guess this really is needed - so please explain that in the commit
> message.
>
> >
> +BR2_ROOTFS_POST_BUILD_SCRIPT="board/stmicroelectronics/common/stm32f4xx/stm32-post-build.sh"
> > +BR2_LINUX_KERNEL=y
>
> Even though 5.13 is the default now, it's not going to stay that way. So
> you
> need to set BR2_LINUX_KERNEL_CUSTOM_VERSION instead.
>
> > +BR2_LINUX_KERNEL_USE_CUSTOM_CONFIG=y
> >
> +BR2_LINUX_KERNEL_CUSTOM_CONFIG_FILE="board/stmicroelectronics/stm32f469-disco/linux/defconfig"
> > +BR2_LINUX_KERNEL_IMAGE_TARGET_CUSTOM=y
> > +BR2_LINUX_KERNEL_XZ=y
> > +BR2_LINUX_KERNEL_IMAGE_TARGET_NAME="xipImage"
> > +BR2_LINUX_KERNEL_DTS_SUPPORT=y
> > +BR2_LINUX_KERNEL_INTREE_DTS_NAME="stm32f469-disco"
> >
> +BR2_LINUX_KERNEL_CUSTOM_DTS_PATH="board/stmicroelectronics/stm32f469-disco/linux/stm32f469-disco.dts"
> > +BR2_PACKAGE_BUSYBOX_CONFIG="package/busybox/busybox-minimal.config"
> > +BR2_PACKAGE_ZLIB=y
>
> Why is zlib needed? That seems weird...
>
> > +BR2_TARGET_ROOTFS_INITRAMFS=y
> > +# BR2_TARGET_ROOTFS_TAR is not set
> > +BR2_TARGET_AFBOOT_STM32=y
> > +BR2_PACKAGE_HOST_OPENOCD=y
> > +BR2_PACKAGE_HOST_UTIL_LINUX=y
>
> I also don't see where util-linux is used, but I may have missed it.
>
>
> I have marked the patch as Changes Requested in patchwork.
>
> Regards,
> Arnout
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.busybox.net/pipermail/buildroot/attachments/20210726/3ab8023b/attachment.html>
More information about the buildroot
mailing list