Re: [Buildroot] [PATCH RESEND 2/2] configs/stm32mp135f-dk: new defconfig

["Raphaël Gallais-Pou" <rgallaispou@gmail.com>]

Le 15/08/2024 à 10:56, Thomas Petazzoni a écrit :
> Hello Raphael,

Hi Thomas,

> 
> On Wed, 14 Aug 2024 20:37:03 +0200
> Raphael Gallais-Pou <rgallaispou@gmail.com> wrote:
> 
>> Add new defconfig for STMicroelectronics board STM32MP135F-DK.
>>
>> STM32MP135F-DK features can be found here:
>>    https://www.st.com/en/evaluation-tools/stm32mp135f-dk.html
>>
>> Signed-off-by: Raphael Gallais-Pou <rgallaispou@gmail.com>
> 
> Thanks for the resend, but there was no need to resend: your previous
> patch was still in our queue of patches to review/merge. I have a
> number of comments below.

Sorry for the inconvenience. I had no news of the patchset which led me 
to resend it. Thanks for taking the time to review this one and merge 
the preview.

> 
>> ---
>>   .../overlay/boot/extlinux/extlinux.conf            |  4 ++
>>   board/stmicroelectronics/stm32mp135f-dk/readme.txt | 38 ++++++++++++++
>>   configs/stm32mp135f_dk_defconfig                   | 59 ++++++++++++++++++++++
>>   3 files changed, 101 insertions(+)
> 
> First of all, you need to enable BR2_DOWNLOAD_FORCE_CHECK_HASHES=y,
> which requires adding:
> 
> BR2_GLOBAL_PATCH_DIR="board/stmicroelectronics/stm32mp135f-dk/patches/"
> 
> and then run ./utils/add-custom-hashes, which will automatically
> populate this folder.

For this one I think that it will be better to set the directory as the 
same of the common directory, eg. 
"board/stmicroelectronics/common/stm32mp1xx/patches/". So that the 
versions between TF-A/U-Boot, the Linux kernel as well as OP-TEE, if it 
is activated in the future for the other platforms, will be synced 
between all stm32mp1* platforms.

> 
> 
>> diff --git a/configs/stm32mp135f_dk_defconfig b/configs/stm32mp135f_dk_defconfig
>> new file mode 100644
>> index 0000000000..cc01a2bd40
>> --- /dev/null
>> +++ b/configs/stm32mp135f_dk_defconfig
>> @@ -0,0 +1,59 @@
>> +# Architecture
>> +BR2_arm=y
>> +BR2_cortex_a7=y
>> +
>> +# Linux headers same as kernel, a 6.9 series
>> +BR2_PACKAGE_HOST_LINUX_HEADERS_CUSTOM_6_9=y
>> +
>> +# System configuration
>> +BR2_ROOTFS_DEVICE_CREATION_DYNAMIC_MDEV=y
>> +BR2_ROOTFS_OVERLAY="board/stmicroelectronics/stm32mp135f-dk/overlay"
>> +BR2_ROOTFS_POST_IMAGE_SCRIPT="board/stmicroelectronics/common/stm32mp1xx/post-image.sh"
>> +
>> +# Kernel
>> +BR2_LINUX_KERNEL=y
>> +BR2_LINUX_KERNEL_CUSTOM_VERSION=y
>> +BR2_LINUX_KERNEL_CUSTOM_VERSION_VALUE="6.9.8"
>> +BR2_LINUX_KERNEL_DEFCONFIG="multi_v7"
> 
> I see on STM32MP157, we're using some custom kernel configuration
> files, with presumably a more "optimized" configuration than multi_v7.
> Should we do the same here?

Effectively this reduces the overall kernel size. My thought on this 
would be to set the general multi_v7 kernel defconfig on the initial 
stm32mp13 support, and fine tune after. Because there is many hardware 
differences between stm32mp15 and stm32mp13 platforms (for example there 
is no audio jack in my knowledge), I feel like this would take quite 
some time for me to sort every drivers.

What do you think about this process ?

> 
> Or maybe we should even have a single common kernel config file for all
> STM32MP1 platforms?

That could also be a good idea on the mid term.

> 
>> +BR2_LINUX_KERNEL_DTS_SUPPORT=y
>> +BR2_LINUX_KERNEL_INTREE_DTS_NAME="st/stm32mp135f-dk"
>> +BR2_LINUX_KERNEL_INSTALL_TARGET=y
>> +
>> +# Filesystem
>> +BR2_LINUX_KERNEL_NEEDS_HOST_OPENSSL=y
> 
> Please group this with the Linux kernel options above, this is not
> "filesystem related".

Ack.

> 
>> +BR2_PACKAGE_OPTEE_CLIENT=y
>> +BR2_TARGET_ROOTFS_EXT2=y
>> +BR2_TARGET_ROOTFS_EXT2_4=y
>> +BR2_TARGET_ROOTFS_EXT2_SIZE="120M"
>> +# BR2_TARGET_ROOTFS_TAR is not set
>> +
>> +# Bootloaders
>> +BR2_TARGET_ARM_TRUSTED_FIRMWARE=y
>> +BR2_TARGET_ARM_TRUSTED_FIRMWARE_CUSTOM_VERSION=y
>> +BR2_TARGET_ARM_TRUSTED_FIRMWARE_CUSTOM_VERSION_VALUE="v2.9"
>> +BR2_TARGET_ARM_TRUSTED_FIRMWARE_PLATFORM="stm32mp1"
>> +BR2_TARGET_ARM_TRUSTED_FIRMWARE_FIP=y
>> +BR2_TARGET_ARM_TRUSTED_FIRMWARE_BL31=y
>> +BR2_TARGET_ARM_TRUSTED_FIRMWARE_BL32_OPTEE=y
>> +BR2_TARGET_ARM_TRUSTED_FIRMWARE_UBOOT_AS_BL33=y
>> +BR2_TARGET_ARM_TRUSTED_FIRMWARE_UBOOT_BL33_IMAGE="u-boot-nodtb.bin"
>> +BR2_TARGET_ARM_TRUSTED_FIRMWARE_ADDITIONAL_VARIABLES="STM32MP_SDMMC=1 DTB_FILE_NAME=stm32mp135f-dk.dtb E=0 BL33_CFG=$(BINARIES_DIR)/u-boot.dtb"
>> +BR2_TARGET_ARM_TRUSTED_FIRMWARE_IMAGES="fip.bin *.stm32"
>> +BR2_TARGET_ARM_TRUSTED_FIRMWARE_NEEDS_DTC=y
>> +BR2_TARGET_OPTEE_OS=y
> 
> Please use the "custom" version mechanism to specify the exact version
> of OP-TEE to be used, like you did for TF-A, Linux and U-Boot.

Ack, forgot about that one. I saw that there was no "custom version" 
option, like there is for U-Boot and the Kernel. And greping through the 
other defconfig there is no hardcoded versions. Maybe this can be a idea 
for the future. For now I will set a custom tarball pointing to the 
official 4.3.0 tarball. Would this be okay ?

cf. https://github.com/OP-TEE/optee_os/archive/refs/tags/4.3.0.tar.gz

> 
>> +BR2_TARGET_OPTEE_OS_PLATFORM="stm32mp1"
>> +BR2_TARGET_OPTEE_OS_PLATFORM_FLAVOR="135F_DK"
>> +BR2_TARGET_UBOOT=y
>> +BR2_TARGET_UBOOT_BUILD_SYSTEM_KCONFIG=y
>> +BR2_TARGET_UBOOT_CUSTOM_VERSION=y
>> +BR2_TARGET_UBOOT_CUSTOM_VERSION_VALUE="2024.07"
>> +BR2_TARGET_UBOOT_BOARD_DEFCONFIG="stm32mp13"
>> +BR2_TARGET_UBOOT_NEEDS_PYLIBFDT=y
>> +# BR2_TARGET_UBOOT_FORMAT_BIN is not set
>> +BR2_TARGET_UBOOT_FORMAT_CUSTOM=y
>> +BR2_TARGET_UBOOT_FORMAT_CUSTOM_NAME="u-boot-nodtb.bin u-boot.dtb"
>> +BR2_TARGET_UBOOT_CUSTOM_MAKEOPTS="DEVICE_TREE=stm32mp135f-dk"
>> +
>> +# Additional tools
>> +BR2_PACKAGE_HOST_BMAP_TOOLS=y
> 
> Please drop, we don't enable bmap-tools in our defconfigs, we leave
> that choice up to the user.

Ack. Mind that I will leave the BR2_PACKAGE_HOST_GENIMAGE=y option since 
it is mandatory for building the image.

Thanks again,

Regards,
Raphaël

> 
> Thanks!
> 
> Thomas
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot