mvebu: bootscript load address calculation a.o. #8272
Conversation
|
""" WalkthroughThe boot script Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)config/bootscripts/boot-mvebu.cmd (4)🔇 Additional comments (10)
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/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: 2
🧹 Nitpick comments (2)
config/bootscripts/boot-mvebu.cmd (2)
438-448: Document endianness assumption for magic number check.The code assumes little-endian byte order when reading and comparing the 'ARMd' magic number. While ARM systems are typically little-endian, this assumption should be documented.
Add a comment explaining the endianness assumption:
# determine the load address for the initial ramdisk if test "${load_addr_calc}" = "on" ; then + # Note: The following magic number check assumes little-endian byte order # vmlinux image + 0x38 contain magic (le-double) 'ARMd' setexpr l_ptr ${l_kernel_addr_r} + 0x00000038
519-528: Simplify redundant boot command logic.The compressed kernel type case and the default case both use
bootz, making the nested conditions redundant.if test "${l_kernel_image_type}" = "flat" ; then booti ${l_kernel_addr_r} ${l_ramdisk_addr_r} ${fdt_addr_r} else - if test "${l_kernel_image_type}" = "compressed" ; then - bootz ${l_kernel_addr_r} ${l_ramdisk_addr_r} ${fdt_addr_r} - else - # default booting command - bootz ${l_kernel_addr_r} ${l_ramdisk_addr_r} ${fdt_addr_r} - fi + # default booting command for compressed and other types + bootz ${l_kernel_addr_r} ${l_ramdisk_addr_r} ${fdt_addr_r} fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
config/bootscripts/boot-mvebu.cmd(1 hunks)
🔇 Additional comments (2)
config/bootscripts/boot-mvebu.cmd (2)
1-543: Well-structured boot script with excellent error handling.The refactoring introduces valuable features including:
- Dynamic load address calculation with proper alignment
- Comprehensive DT overlay support with multiple prefix fallbacks
- Robust error handling with informative user messages
- Good variable scoping using local (l_) and preset_ prefixes
- Flexible configuration through armbianEnv.txt
The implementation is thorough and follows U-Boot scripting best practices.
47-47:⚠️ Potential issueFix syntax error in conditional statement.
The semicolon is incorrectly placed after the conditional. In U-Boot shell scripting, it should come before the command to be executed.
- test "${align_overlap_oboe_avoidance}" = "on" && setexpr l_addr_next ${l_addr_next} + 1 ; + test "${align_overlap_oboe_avoidance}" = "on" && setexpr l_addr_next ${l_addr_next} + 1Likely an incorrect or invalid review comment.
github-actions
bot
removed
the
Ready to merge
Does mvebu have hdmi or video out support in general? If not I think adding such large delays don't make sense, just annoys users why stuff takes so long. Users connected to debug serial are usually a bit more mindful and the overall output of boot loader stage isn't that big so errors should be spotted easily without extensive search. |
|
Hi @EvilOlaf, I'm fine with removing the delay :-) The idea for the delay was to have some way of indicating to end users that their Please do keep the comments coming! |
github-actions
bot
removed
the
Ready to merge
Follow conventional `if-then-else-fi` constructions by not having `else` followed by semicolon. Hush is accepting, but other commonly used shells do not accept this.
Remove the 10 sec delay in case "critical error" raised.
djurny
force-pushed
the
feat/mvebu-boot-cmd-calc-load-addr-ao
branch
from
d56d1c3 to
f849a99
Compare
github-actions
bot
removed
the
Ready to merge
|
Anything else here or ready to merge? |
|
Hi @igorpecovnik, |
mvebu was busy in early days. sunxi and especially rockchip is more interesting, yes. |
3 participants
Description
Attempt to work towards one U-Boot bootscript for (at least) mvebu, sunxi and rockchip64.
This adds:
This will remove the need to update any
kernel_load_addr_rorramdisk_addr_rin case kernel image increases.Calculation is based on either
image_size+text_offsetas specified in the vmlinux(/Image) header infokverinarmbianEnv.txtAlso:
verbosityto0Documentation summary for feature / change
Load address calculation can be disabled by adding
load_addr_calc=offtoarmbianEnv.txtLoad address calculation OBOE avoidance can be disabled by adding
align_overlap_oboe_avoidance=offtoarmbianEnv.txtUser can set custom
fdtdirandfdtfileinarmbianEnv.txt, but make sure to only specify DT filename infdtfile.fdtdirwill be used to both load DT, DT overlays and fixup scriptsHow Has This Been Tested?
Checklist:
New warnings introduced:
Prequisite U-Boot
setexprcommand already merged via rockchip64/rk3328: U-Boot v2022.04/07 add setexpr #8260.