sunxi/mvebu: U-Boot calc load addresses, bootscript enhancements #8203
Conversation
armbian#8178 - Add switch to turn avoidance on/off - Increment base address before alignment to resolve the oboe - oboe observed in U-Boot v2021.04 and in particular with DT loading
- Check all load actions - Add optional early exit with explanation in case a load fails
- Added FDT resize/trim to allow big DT and overlays and still use configured load addresses in case `setexpr` not available - Moved FDT resize and next load address alignment - Added filename information to load instructions - Moved all "global" variable determinations to the top of the bootscript
- Determine the load address for kernel and initial ramdisk based on DT and kernel image size. - Added FDT resize/trim to allow big DT and overlays and still use configured load addresses in case `setexpr` not available - Moved FDT resize and next load address alignment - Added filename information to load instructions - Check all load actions - Add optional early exit with explanation in case a load fails - Add switch to turn avoidance on/off - Increment base address before alignment to resolve the oboe - oboe observed in U-Boot v2021.04 and in particular with DT loading
armbian#8178 - Add switch to turn avoidance on/off - Increment base address before alignment to resolve the oboe - oboe observed in U-Boot v2021.04 and in particular with DT loading
- Check all load actions - Add optional early exit with explanation in case a load fails
- Added FDT resize/trim to allow big DT and overlays and still use configured load addresses in case `setexpr` not available - Moved FDT resize and next load address alignment - Added filename information to load instructions - Moved all "global" variable determinations to the top of the bootscript
- reformatted - added "functions" for messaging - added load address calculation for sunxi - added overlap avoidance for sunxi - added more messaging towards console - introduced some U-Boot commonly used variables
|
""" WalkthroughThis pull request extensively refactors the The Helios4 board configuration is updated to add the "ethtool" package for Wake-on-LAN (WoL) support. Correspondingly, the Helios4 WoL systemd service is modified to enable WoL on all network interfaces matching "en*" or "eth*", replacing the previous fixed single interface configuration. Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure ✨ Finishing Touches🧪 Generate Unit Tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
size/small
github-actions
bot
added
size/large
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
config/bootscripts/boot-sunxi.cmd (1)
253-259:fdt_totalsizefallback can underestimate after overlays are appliedThe manual size calculation re-uses the original
${fdtfilesize}captured before overlays & fixups, so the resulting total may be too small and cause kernel overwrite when large overlays are applied.Consider:
- setexpr fdt_totalsize ${fdtfilesize} / 0x1000 + /* Use current header size, not original file size */ + fdt header get tmp_totalsize totalsize || true + if test "${tmp_totalsize}" != "" ; then + setexpr fdtfilesize ${tmp_totalsize} + fi + setexpr fdt_totalsize ${fdtfilesize} / 0x1000config/bootscripts/boot-mvebu.cmd (1)
102-123: Run-time test coverage is fragile – consider adding CI unit-tests for script logicBoth bugs above slipped through because boot scripts are executed only on hardware.
A minimal CI step that runsmkimage -T scriptfollowed by agrepof expected log strings (e.g. “Applied DT overlay …”) would have caught them early.Also applies to: 130-151
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
config/bootscripts/boot-mvebu.cmd(3 hunks)config/bootscripts/boot-sunxi.cmd(2 hunks)
🔇 Additional comments (3)
config/bootscripts/boot-sunxi.cmd (2)
119-134: Empty ‘then’ branch hampers readability, makes flow harder to followThe outer
if test -e … ; thenblock has no body, pushing all useful work into the nestedelse …chain.
A no-op branch is legal but confusing, especially in a 9-level cascade. Consider flipping the condition or collapsing the logic to make intent clearer and maintenance easier.[ suggest_optional_refactor ]
158-178:❌ Incorrect review comment
overlay_prefixmay be undefined – produces “//…dtbo” pathsBoth overlay loops rely on
${overlay_prefix}but the variable is never set in this script.
If absent in the imported environment the generated filename becomes"/overlay/-foo.dtbo"which will never exist, silently causing warnings.Recommendation:
- Provide a sane default early in the script, e.g.
setenv overlay_prefix "${overlay_prefix:=sunxi}".- Or guard each usage with
if test "${overlay_prefix}" != "" ; then ….Also applies to: 186-206
I’ll search the repo for any definitions or usage of
overlay_prefixto confirm whether it’s ever initialized.
🏁 Script executed:
#!/bin/bash # Search for overlay_prefix across the entire repository rg overlay_prefix -n # Narrow down to occurrences in the boot-sunxi script rg overlay_prefix -n config/bootscripts/boot-sunxi.cmdLength of output: 8010
I’ll check whether
boot-sunxi.cmdloadsarmbianEnv.txtor otherwise initializesoverlay_prefix:
🏁 Script executed:
#!/bin/bash # Look for environment import or armbianEnv in boot-sunxi script rg "env import" -n config/bootscripts/boot-sunxi.cmd rg "armbianEnv" -n config/bootscripts/boot-sunxi.cmdLength of output: 262
overlay_prefix is initialized via armbianEnv.txt import – no changes needed
The script sets
setenv envfile 'armbianEnv.txt' … if load ${devtype} ${devnum}:${distro_bootpart} ${load_addr} ${prefix}${envfile}; then env import -t ${load_addr} ${filesize} … fiearly on, which populates
${overlay_prefix}(it’s written into armbianEnv.txt by the build). The loops at lines 158–178 and 186–206 run after that import, so${overlay_prefix}will always be defined.As other board scripts make the same assumption, no default or extra guard is required here.
Likely an incorrect or invalid review comment.
config/bootscripts/boot-mvebu.cmd (1)
49-55: Self-test forsetexprclobbers variable namespaceUsing the variable name
setexprshadows the command itself in subsequent calls on some U-Boot versions that supportenv setexpr.
Safer to store the flag in a differently named variable, e.g.has_setexpr.[ suggest_nitpick ]
|
Wow looks like I stared myself blind and got a reality check by some AI 🤭 |
sunxi:
- sync up with mvebu
- add earlycon option
- improve on messages
mvebu:
- sync up with sunxi
- fix loading proper ${overlay}
- fix loading proper ${user_overlay}
- improve on messages
github-actions
bot
removed
the
Ready to merge
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
config/bootscripts/boot-sunxi.cmd (1)
114-117: Valid implementation of string substitution in U-Boot.The
setexpr subcommand is correctly used for string operations to remove the vendor prefix from the fdtfile variable, as documented in U-Boot's setexpr command documentation when CONFIG_REGEX=y is enabled.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
config/bootscripts/boot-mvebu.cmd(3 hunks)config/bootscripts/boot-sunxi.cmd(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- config/bootscripts/boot-mvebu.cmd
🧰 Additional context used
🧠 Learnings (1)
config/bootscripts/boot-sunxi.cmd (1)
Learnt from: djurny
PR: armbian/build#8203
File: config/bootscripts/boot-sunxi.cmd:111-114
Timestamp: 2025-05-17T23:08:31.722Z
Learning: In U-Boot scripts, the `setexpr sub` command is valid for string operations when CONFIG_REGEX=y is enabled. This allows for pattern substitution in strings, as documented in https://docs.u-boot.org/en/latest/usage/cmd/setexpr.html. The syntax is `setexpr <variable> sub <pattern> <replacement> <string>`.
🔇 Additional comments (13)
config/bootscripts/boot-sunxi.cmd (13)
6-25: Well-structured environment initialization with comprehensive defaults.The environment initialization block is well organized with logical grouping of variables. The addition of variables for alignment control, error handling, and verbosity will improve script robustness and debugging capabilities.
34-39: Good implementation of helper functions for consistent messaging and error handling.The helper functions for alignment and message handling create a consistent approach to user feedback throughout the script. The
critical_errorfunction with conditional exit is particularly valuable for preventing boot continuation after fatal errors.
40-53: Robust handling of mandatory and optional environment variables.The script now properly enforces required variables (
fdtfile) with clear error messages, while providing sensible defaults for optional variables. This prevents silent failures later in the boot process.
61-76: Well-implemented environment file loading with proper error handling.The environment file loading sequence now has comprehensive checks for file existence, load success, and import success, with appropriate success and failure messaging based on verbosity settings.
109-137: Comprehensive device tree file path resolution with fallbacks.The script now intelligently attempts multiple possible device tree file locations, handling vendor prefixes and falling back to defaults if needed. This improves compatibility across different system configurations.
142-150: Improved device tree loading with clear error messaging.The device tree loading now has proper error handling with informative success messages and critical error exits on failure, improving reliability and troubleshooting.
156-182: Robust kernel overlay loading with individual success/failure reporting.The overlay loading process now properly checks for file existence, loads each overlay individually, and reports success or failure for each overlay. This provides clear feedback about which overlays were successfully applied.
184-210: Well-implemented user overlay support with detailed error reporting.The user overlay handling mirrors the kernel overlay implementation with consistent checks and messaging, making the user experience more predictable and debugging easier.
212-245: Robust error recovery for overlay failures.If any overlay fails to apply, the script now properly reloads the original device tree and continues. The fixup script handling also has proper error checking and reporting.
247-263: Well-implemented device tree size calculation.The script now properly resizes the device tree after overlays and calculates or approximates the total size including the extra size. This is important for determining the next load address.
265-290: Improved load address calculation to prevent memory overlaps.The memory address calculation for kernel and ramdisk loading now properly avoids overlaps by calculating each address based on the previous load's size and applying alignment. This is essential for preventing boot failures due to memory corruption.
291-294: Added error handling for boot command.The script now reports a critical error if the boot command fails, rather than silently continuing or failing without explanation.
294-300: Sequential loading in legacy boot path.The legacy boot path now explicitly loads ramdisk and kernel before device tree script, which provides a more predictable loading sequence.
Removed duplicate block for earlycon enabling
github-actions
bot
removed
the
Ready to merge
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
config/bootscripts/boot-sunxi.cmd (1)
158-177: Set a default foroverlay_prefixto prevent malformed file pathsWhen
overlay_prefixis unset the generated path becomes
${fdtdir}/overlay/-${overlay}.dtbo, guaranteeing a ❌ “file not found” and noisy warnings.
Defaulting the variable to the vendor (or a safe fallback likesunxi) keeps the behaviour intact for existing users while eliminating spurious errors.+# fall back to vendor name when overlay_prefix not supplied +if test "${overlay_prefix}" = ""; then + setenv overlay_prefix "${vendor}" +fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
config/bootscripts/boot-sunxi.cmd(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
config/bootscripts/boot-sunxi.cmd (1)
Learnt from: djurny
PR: armbian/build#8203
File: config/bootscripts/boot-sunxi.cmd:111-114
Timestamp: 2025-05-17T23:08:31.722Z
Learning: In U-Boot scripts, the `setexpr sub` command is valid for string operations when CONFIG_REGEX=y is enabled. This allows for pattern substitution in strings, as documented in https://docs.u-boot.org/en/latest/usage/cmd/setexpr.html. The syntax is `setexpr <variable> sub <pattern> <replacement> <string>`.
🔇 Additional comments (1)
config/bootscripts/boot-sunxi.cmd (1)
88-90: Early-console is now appended only once – good catch!
The previous double-injection has been removed;bootargsstay clean.
- add ethtool - enable wol on all found ethernet interfaces - support commonly used network device naming schemes
github-actions
bot
added
Ready to merge
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/bsp/mvebu/helios4/helios4-wol.service (1)
2-2: Description should be updated to match implementation.The unit Description still references only "eth0" but the implementation now handles multiple network interfaces.
-Description=Enable Wake-on-LAN for Helios4 eth0 +Description=Enable Wake-on-LAN for Helios4 network interfaces
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
config/boards/helios4.conf(1 hunks)packages/bsp/mvebu/helios4/helios4-wol.service(1 hunks)
🔇 Additional comments (2)
config/boards/helios4.conf (1)
16-16: LGTM! Package addition is well-documented.The addition of the ethtool package is well-documented with a clear comment explaining its purpose for the WoL service.
packages/bsp/mvebu/helios4/helios4-wol.service (1)
7-7: Improved implementation to handle multiple network interfaces.The updated script now properly handles all network interfaces matching "en*" or "eth*" patterns rather than just eth0, making the service more flexible. Good use of parameter expansion with
:?for error checking.
github-actions
bot
removed
the
Ready to merge
|
Prepare to close, add lessons learned from rockchip boot script, then split into per arch pull requests. |
Description
GitHub issue reference:
Jira reference number AR-2680
Fix Helios4 wake-on-lan service.
Documentation summary for feature / change
load,fdt applyand other operationsload,fdt applyand other operationsHow Has This Been Tested?
sunxi:
Built
25.08.trunk: OKDowngraded to
25.2.2: OKChecklist:
Please delete options that are not relevant.