mini-boot: ship a recovery image that fits the bootrom SRAM window - #7
Merged
Conversation
PR Summary by QodoShip SRAM-safe UART recovery mini-boot images
AI Description
Diagram
High-Level Assessment
Files changed (8)
|
Code Review by Qodo
1.
|
openipc-ai
changed the base branch from
fix/mini-boot-fits-bootrom-sram-window
to
master
August 25, 2026 06:51
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
|
Code review by qodo was updated up to the latest commit 9ba991f |
OpenIPC/firmware#2299 is a hi3518ev200 that cannot be revived. Its mini-boot prelude is 17408 bytes and the boot ROM's SRAM window is 14336, so the UART upload runs off the end of SRAM into the boot ROM's own stack and the chip stops answering. The previous commit cut the prelude as far as compiler flags can and gated it, but 3072 bytes over is still 3072 bytes over: that image cannot be uploaded, and the reporter's camera stays dead. Build a second image for the recovery path instead of trying to make one image serve both. DDR training is by far the largest thing in the prelude, and it is the one part a bench recovery can safely do without: the static DDR setup from reg_info still runs, and the image only has to survive a single decompression into RAM with an operator present. The flashed image keeps training untouched -- that is what makes RAM work across silicon and temperature spread, and dropping it there would trade a recovery defect for an intermittent field one. variant prelude target boots in QEMU universal 17408 14336 yes recovery 11264 14336 yes, 3072 bytes of margin Both were built with the vendor arm-hisiv510 toolchain for both SoCs and booted to a U-Boot prompt in qemu-hisilicon's hi3516cv200 machine. The recovery image's output is identical to the flashed one's, including "Uncompress.........Ok" -- DDR came up from reg_info alone and the LZMA payload decompressed into it. Turning training off exposed a trap worth naming: start.S guarded the `bl start_ddr_training` with CONFIG_DDR_TRAINING_V2, but the function it calls is itself guarded internally and *also* performs the post-training DMC write the vendor comments as required for chip compatibility. Guarding both meant a training-less build silently lost that write. The call site is now unconditional, which changes nothing for the flashed image and keeps the write in the recovery one -- verified in the disassembly, where start_ddr_training reduces to exactly that single store. The size ASSERT now comes from the Makefile via --defsym so the two variants can carry different limits: the recovery image is held to 0x3800 for real, the flashed one keeps ratcheting against its current size. Linking with no --defsym is an error rather than a default, so neither variant can quietly lose its gate. CI checks and boots both, and publishes both. No change is needed in defib: its payload scan starts at 0x4000, so on the recovery image it finds nothing and falls back to the profile's 0x3B00 -- exactly the SRAM window, which covers the whole 11264-byte SPL. Existing installs work as-is. Not validated on real hardware -- I have neither SoC on a bench.
Review raised that the two variants share object file names and are compiled with different flags, which make does not track, so building one after the other could relink the other's objects -- and in the direction that matters that is silent: the flashed image would come out with DDR training compiled out and still pass its looser ceiling. It does not currently happen, because this Makefile leaves no objects behind between invocations and recompiles all 19 every time; the exact sequence produces a correct image today. But that is an accident of how make treats these files, not a property anyone declared, and one .PRECIOUS or a directly-requested .o would undo it silently. The failure it guards against is an unreliable flashed bootloader, which is not something to leave resting on an accident. Record the variant the objects belong to and wipe them when it changes, so build order cannot matter whoever invokes the directory. Verified in both orders: universal -> recovery -> universal, and recovery -> universal, each giving 17408 with training and 11264 without. This also subsumes the explicit clean the top-level recovery target was doing, which only protected the one call path that went through it.
openipc-ai
force-pushed
the
fix/uart-recovery-image
branch
from
August 25, 2026 07:19
f9c86c0 to
8072c10
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #5. Fixes OpenIPC/firmware#2299.
Contains #6 (this branched from it), so the diff here is #6 + the recovery image. Merging this closes both; merging #6 first and then this also works.
The problem #6 left open
#6 cut the mini-boot prelude from 19376 to 17408 and made the size impossible to regress. The boot ROM's window is 14336, so that image still cannot be uploaded over UART, and the camera in OpenIPC/firmware#2299 stays dead.
One image can't serve both paths, so build two
The prelude has to shed 3072 bytes. DDR training is by far the largest thing in it — and it is the one part a bench recovery can safely do without: the static DDR setup from
reg_infostill runs, and the image only has to survive a single decompression into RAM with an operator present.The flashed image keeps training untouched. That is what makes RAM work across silicon and temperature spread, and dropping it there would trade a recovery defect for an intermittent field one.
u-boot-<soc>-universal.bin(flashed)u-boot-<soc>-recovery.bin(UART)--gc-sectionsdoes the removal on its own onceCONFIG_DDR_TRAINING_V2is off — no object list to keep in sync.A trap this turned up
start.Sguarded thebl start_ddr_trainingwithCONFIG_DDR_TRAINING_V2. But the function it calls is itself guarded internally, and also does this:— deliberately outside the
#ifdef. Guarding both meant a training-less build silently lost that write. The call site is now unconditional, which changes nothing for the flashed image (the macro is defined there) and keeps the write in the recovery one. Verified in the disassembly, where the whole function reduces to exactly that store:Gating
The size
ASSERTnow comes from the Makefile via--defsym, so the two variants can carry different limits — the recovery image is held to0x3800for real, the flashed one keeps ratcheting. Linking with no--defsymis an error rather than a default, so neither variant can quietly lose its gate:CI builds, size-checks, boots and publishes both images. The recovery check does not ratchet:
No defib change needed
defib's payload scan starts at
0x4000, so on the recovery image it finds nothing and falls back to the profile's0x3B00— exactly the SRAM window, which covers the whole 11264-byte SPL and stops precisely at0x04014000. Checked against defib's own_detect_spl_size:So existing installs work as-is. Recovery flow is unchanged apart from which file you point at:
…then flash
u-boot-<soc>-universal.binto NOR from the U-Boot prompt, so the camera runs with full DDR training.Verification
Vendor
arm-hisiv510toolchain (the onebuild.ymldownloads), both SoCs, both variants, inqemu-hisilicon'shi3516cv200machine — the same model the existingqemu_smokejob uses. All four reach a U-Boot prompt. The recovery image's output is byte-identical to the flashed one's:Uncompress.........Okis the line that matters: DDR came up fromreg_infoalone and the LZMA payload decompressed into it.Gates exercised in both directions — recovery build forced past its ceiling fails the link, an artifact with the payload pushed past 14336 fails the CI check, and the shipped sizes pass.
Not validated on real hardware — I have neither SoC on a bench. The reporter in OpenIPC/firmware#2299 has a blank hi3518ev200 and a working UART setup, so this wants their confirmation before it is treated as proven. QEMU does not model DDR timing, so it cannot tell us that a training-less DDR init holds on real silicon — that is the one claim here resting on reasoning rather than measurement.