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

[Arnout Vandecappelle]

On 26/07/2021 18:45, Yauheni Saldatsenka wrote:
> 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?

 Yes indeed that's what I mean.

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

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


> 
>     >         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?

 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.


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

 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]