Conversation
terrillmoore
left a comment
There was a problem hiding this comment.
This PR bundles two independent bodies of work: (1) mccibootloader_image tool improvements (fix #7 header probing, --debug flag, argv fix, Doxygen, comment fixes) and (2) full STM32H7B3 bootloader platform support. Reviewing both below.
We'll pull the tool changes out as a separate PR so they can be merged independently. Terry will guide that.
Build-breakers
Missing libmcci_bootloader_cm7.a in bootloader link list. LIBS_McciBootloader_stm32h7b3 lists only libmcci_bootloader_stm32h7.a and libmcci_bootloader_stm32h7b3.a. The MCCI_DO_BOOTLOADER macro appends libmcci_bootloader.a and libmcci_tweetnacl.a, but the cm7 architecture library is never linked. The symbols from platform/arch/cm7/src/ won't be found.
No CFLAGS_CPU override for Cortex-M7. setup.mk sets CFLAGS_CPU ?= -mcpu=cortex-m0plus -mthumb. The STM32H7B3 is a Cortex-M7, and there's no mechanism to override CPU flags per-bootloader target. Building the H7B3 bootloader with CM0+ flags will produce wrong code (or fail to compile if CM7-specific instructions are used).
Possible duplicate symbols. SOURCES_libmcci_bootloader_stm32h7 includes both SOC-level files and arch-level files (mccibootloaderplatform_stm32h7_checkimagevalid.c, _getappinfo.c, _getsignatureblock.c, _startapp.c). These appear to duplicate files that also exist in platform/arch/cm7/src/ and are compiled into libmcci_bootloader_cm7.a. If both libraries end up linked, there will be duplicate symbol errors. If only the stm32h7 library is linked (and cm7 is not), then the cm7 library definition is dead code in the Makefile. Either way, the intent needs clarification.
Flash driver concern
waitForDone() clears status via rSR + 4 in mccibootloader_stm32h7_systemflash.c. This writes to whatever register is 4 bytes after FLASH_SR. On STM32H7, FLASH_SR bits are w1c (write-1-to-clear) and should be cleared by writing back to SR itself. If SR+4 happens to be FLASH_CCR on this part variant, it might work by coincidence, but this needs verification against the STM32H7B3 reference manual.
Copy-paste artifacts
mccibootloaderboard_stm32h7b3_storage.cline 3: Module saysmccibootloaderboard_catenaabz_storage.c. End-of-file comment (line 106) also sayscatenaabz_storage.c.mccibootloaderboard_stm32h7b3_platforminterface.cline 6: Function description says "for MCCI Catena 4801 boards".mccibootloaderboard_stm32h7b3_storage.cpropagates the "constraings" typo (should be "constraints").
Minor issues
platform/board/st/stm32h7b3/mk/mccibootloader.ldis missing a final newline.- The Doxyfile is 2539 lines of mostly defaults. Consider a minimal Doxyfile that only sets non-default values, or add it in a separate PR.
Tool changes look good
The mccibootloader_image refactoring to support multiple AppInfo offsets via templates is clean. The probeHeader() approach with vAppInfoOffsets[] table is straightforward. The argv increment fix for --app-version is a real bug fix. The PENDSTCLR bit-position fix and comment corrections in mcci_arm_cm0plus.h are correct.
|
Update: The following changes from this PR have already been cherry-picked and merged to
Please rebase this branch onto current |
The devel branch contains stm32h7b3 bootloader.