fix: booting orangepi5 from nvme drive #8885
Conversation
For some reason test -e command that checks for armbianEnv.txt file existence fails to mount the first partition. This leads to ignoring of all boot environment, including the critical rootdev variable. This variable is hardcoded to /dev/mmcblk0p1 which works for the default sdcard case, but doesn't for nvme (and likely sata) At the same time the load command that comes after that works well Therefore the idea behind this change is to not test for file existence and attempt to load it instead on success - import the env variables
github-actions
bot
added
11
WalkthroughTwo Rockchip boot scripts are modified to replace explicit file existence checks with conditional load commands, allowing the load operation itself to determine if the armbianEnv.txt file proceeds to environment import. The control flow shifts from "test-then-load" to "load-then-import-on-success." Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
github-actions
bot
added
Needs review
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
config/bootscripts/boot-rk35xx.cmd (1)
21-23: LGTM! Fix correctly addresses NVMe boot issues.The change from
test -eto directloadin the conditional resolves the NVMe partition mounting failure documented in the PR. Theloadcommand naturally returns success/failure, making the explicit existence check redundant. This pattern is already used consistently throughout the script for overlay loading (lines 50, 53, 56, 59).For consistency and to prevent similar NVMe mounting issues with fixup script loading, consider updating the
test -epatterns at lines 68 and 73 to use directloadconditionals (matching the approach applied at line 21):
- Line 68: kernel provided fixup script (
-fixup.scr)- Line 73: user provided fixup script (
fixup.scr)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
config/bootscripts/boot-rk35xx.cmd(1 hunks)config/bootscripts/boot-rockchip64.cmd(1 hunks)
🧰 Additional context used
🧠 Learnings (13)
📓 Common learnings
Learnt from: EvilOlaf
Repo: armbian/build PR: 8428
File: config/boards/lckfb-taishanpi.csc:5-9
Timestamp: 2025-07-25T03:51:50.830Z
Learning: When reviewing PRs in the Armbian build system, U-Boot defconfig files and patches may be added as part of the PR changes but might not be visible in the current repository clone state during review. It's important to check the actual PR file changes directly via GitHub API (https://api.github.com/repos/armbian/build/pulls/{pr_number}/files) to get the complete picture of what files are being added or modified, especially for U-Boot patches that will be applied during the build process.
Learnt from: amazingfate
Repo: armbian/build PR: 8619
File: config/sources/families/rockchip.conf:222-230
Timestamp: 2025-09-14T06:32:29.806Z
Learning: In the Armbian build system, the write_uboot_platform() function implementations follow different patterns across Rockchip family files. The newer standard (used in rockchip64_common.inc and rk3506) includes 'status=none' parameter in dd commands, while older implementations (rk3288, rk322x) use an older pattern without this parameter. The rk3506 implementation correctly follows the current Rockchip family standard.
Learnt from: amazingfate
Repo: armbian/build PR: 8619
File: config/bootscripts/boot-rk3506.cmd:46-47
Timestamp: 2025-09-12T15:46:16.772Z
Learning: In Armbian boot scripts, uInitrd is always packaged by the build system, so unconditional loading of uInitrd without existence checks is acceptable and won't cause boot failures.
Learnt from: djurny
Repo: armbian/build PR: 8287
File: config/bootscripts/boot-sunxi.cmd:148-161
Timestamp: 2025-06-12T21:03:39.686Z
Learning: In Armbian U-Boot bootscripts, the `${cpu}` environment variable is always pre-set by U-Boot (or defaults to a valid value), so additional guards against it being unset are unnecessary.
Learnt from: djurny
Repo: armbian/build PR: 8272
File: config/bootscripts/boot-mvebu.cmd:182-186
Timestamp: 2025-06-04T23:45:38.860Z
Learning: In config/bootscripts/boot-mvebu.cmd, the `fdtfile` variable is mandatory for booting and is pre-set by U-Boot, but can be overridden via armbianEnv.txt. If `fdtfile` is empty, the subsequent device tree file search logic will eventually fail and trigger the critical error "Cannot find DT!" with proper error handling.
Learnt from: pyavitz
Repo: armbian/build PR: 8388
File: config/boards/kickpik2b.conf:1-14
Timestamp: 2025-07-17T04:12:33.125Z
Learning: In the Armbian build system, board configuration files (*.conf) follow a project standard of not including shebang lines, even though they contain bash code and are sourced by the build system. This is an established Armbian convention that individual contributors are expected to follow, and changes to this standard would require broader project maintainer approval.
Learnt from: leggewie
Repo: armbian/build PR: 8133
File: extensions/apa.sh:1-2
Timestamp: 2025-04-28T08:27:26.890Z
Learning: In the Armbian build system, extension scripts in the `extensions/` directory contain hook functions and are meant to be sourced, not executed directly. These scripts don't require a shebang or `set -euo pipefail`.
Learnt from: djurny
Repo: armbian/build PR: 8315
File: lib/functions/rootfs/distro-agnostic.sh:54-68
Timestamp: 2025-06-22T00:45:26.182Z
Learning: In Armbian build system, external command dependencies like envsubst are handled by adding the required packages (e.g., gettext for envsubst) to host_dependencies in adaptative_prepare_host_dependencies function in lib/functions/host/prepare-host.sh, rather than checking command availability at runtime.
Learnt from: tabrisnet
Repo: armbian/build PR: 8678
File: config/kernel/linux-sm8250-current.config:78-80
Timestamp: 2025-09-27T21:49:55.796Z
Learning: In the Armbian build system, kernel configuration files are generated through an automated process: taking previous config → applying scripted changes from armbian-kernel.sh → running kernel's `make oldconfig` → processing by Armbian machinery back into config files. This automated process properly handles kernel configuration dependencies and reduces the likelihood of manual configuration errors.
Learnt from: tabrisnet
Repo: armbian/build PR: 8678
File: config/kernel/linux-sm8250-edge.config:80-82
Timestamp: 2025-09-27T21:50:04.845Z
Learning: In the Armbian build system, kernel configuration files are generated through this automated process: taking previous config → applying scripted changes from armbian-kernel.sh → running kernel's `make oldconfig` → processing by Armbian machinery back into config files. This automated process properly handles kernel configuration dependencies and reduces the likelihood of manual configuration errors.
Learnt from: EvilOlaf
Repo: armbian/build PR: 8298
File: extensions/https_proxy.sh:1-9
Timestamp: 2025-06-14T05:53:10.627Z
Learning: In Armbian build framework, extensions are loaded and executed via lib/functions/general/extensions.sh using call_extension_method. The framework sets set -o errexit and set -o errtrace globally, and individual commands run with pipefail protection through the command execution functions.
📚 Learning: 2025-09-14T06:32:29.806Z
Learnt from: amazingfate
Repo: armbian/build PR: 8619
File: config/sources/families/rockchip.conf:222-230
Timestamp: 2025-09-14T06:32:29.806Z
Learning: In the Armbian build system, the write_uboot_platform() function implementations follow different patterns across Rockchip family files. The newer standard (used in rockchip64_common.inc and rk3506) includes 'status=none' parameter in dd commands, while older implementations (rk3288, rk322x) use an older pattern without this parameter. The rk3506 implementation correctly follows the current Rockchip family standard.
Applied to files:
config/bootscripts/boot-rockchip64.cmdconfig/bootscripts/boot-rk35xx.cmd
📚 Learning: 2025-06-12T21:03:39.686Z
Learnt from: djurny
Repo: armbian/build PR: 8287
File: config/bootscripts/boot-sunxi.cmd:148-161
Timestamp: 2025-06-12T21:03:39.686Z
Learning: In Armbian U-Boot bootscripts, the `${cpu}` environment variable is always pre-set by U-Boot (or defaults to a valid value), so additional guards against it being unset are unnecessary.
Applied to files:
config/bootscripts/boot-rockchip64.cmdconfig/bootscripts/boot-rk35xx.cmd
📚 Learning: 2025-06-12T21:14:36.024Z
Learnt from: djurny
Repo: armbian/build PR: 8287
File: config/bootscripts/boot-sunxi.cmd:38-44
Timestamp: 2025-06-12T21:14:36.024Z
Learning: In config/bootscripts/boot-sunxi.cmd the unconditional "+1" increment in func_align_addr_next (when align_overlap_oboe_avoidance="on") is intentional. It compensates for a known off-by-one error in U-Boot where the end address is calculated as start+size instead of start+size-1, so the extra page prevents overlap. This behavior should not be “optimized away”.
Applied to files:
config/bootscripts/boot-rockchip64.cmdconfig/bootscripts/boot-rk35xx.cmd
📚 Learning: 2025-06-04T23:45:38.860Z
Learnt from: djurny
Repo: armbian/build PR: 8272
File: config/bootscripts/boot-mvebu.cmd:182-186
Timestamp: 2025-06-04T23:45:38.860Z
Learning: In config/bootscripts/boot-mvebu.cmd, the `fdtfile` variable is mandatory for booting and is pre-set by U-Boot, but can be overridden via armbianEnv.txt. If `fdtfile` is empty, the subsequent device tree file search logic will eventually fail and trigger the critical error "Cannot find DT!" with proper error handling.
Applied to files:
config/bootscripts/boot-rockchip64.cmdconfig/bootscripts/boot-rk35xx.cmd
📚 Learning: 2025-07-17T04:12:33.125Z
Learnt from: pyavitz
Repo: armbian/build PR: 8388
File: config/boards/kickpik2b.conf:1-14
Timestamp: 2025-07-17T04:12:33.125Z
Learning: In the Armbian build system, board configuration files (*.conf) follow a project standard of not including shebang lines, even though they contain bash code and are sourced by the build system. This is an established Armbian convention that individual contributors are expected to follow, and changes to this standard would require broader project maintainer approval.
Applied to files:
config/bootscripts/boot-rockchip64.cmdconfig/bootscripts/boot-rk35xx.cmd
📚 Learning: 2025-09-12T15:46:16.772Z
Learnt from: amazingfate
Repo: armbian/build PR: 8619
File: config/bootscripts/boot-rk3506.cmd:46-47
Timestamp: 2025-09-12T15:46:16.772Z
Learning: In Armbian boot scripts, uInitrd is always packaged by the build system, so unconditional loading of uInitrd without existence checks is acceptable and won't cause boot failures.
Applied to files:
config/bootscripts/boot-rockchip64.cmdconfig/bootscripts/boot-rk35xx.cmd
📚 Learning: 2025-09-01T06:11:43.476Z
Learnt from: wei633
Repo: armbian/build PR: 8557
File: config/bootscripts/boot-xpressreal-t3.cmd:0-0
Timestamp: 2025-09-01T06:11:43.476Z
Learning: In Armbian boot scripts without initramfs support, UUID= and LABEL= style device paths cannot be used for rootdev as they require initramfs to resolve during boot. Only direct device paths (/dev/mmcblkXpY) or PARTUUID= references (which U-Boot can resolve) will work.
Applied to files:
config/bootscripts/boot-rockchip64.cmdconfig/bootscripts/boot-rk35xx.cmd
📚 Learning: 2025-07-23T07:30:52.265Z
Learnt from: EvilOlaf
Repo: armbian/build PR: 8417
File: config/boards/orangepi5pro.csc:57-58
Timestamp: 2025-07-23T07:30:52.265Z
Learning: In the Armbian build system, BOOTPATCHDIR can contain board-specific subdirectories (e.g., board_orangepi5pro) for applying patches to specific boards only. The framework automatically checks if such board-specific subdirectories exist for the board being built and applies those patches accordingly.
Applied to files:
config/bootscripts/boot-rockchip64.cmdconfig/bootscripts/boot-rk35xx.cmd
📚 Learning: 2025-09-27T21:50:04.845Z
Learnt from: tabrisnet
Repo: armbian/build PR: 8678
File: config/kernel/linux-sm8250-edge.config:80-82
Timestamp: 2025-09-27T21:50:04.845Z
Learning: In the Armbian build system, kernel configuration files are generated through this automated process: taking previous config → applying scripted changes from armbian-kernel.sh → running kernel's `make oldconfig` → processing by Armbian machinery back into config files. This automated process properly handles kernel configuration dependencies and reduces the likelihood of manual configuration errors.
Applied to files:
config/bootscripts/boot-rk35xx.cmd
📚 Learning: 2025-09-27T21:49:55.796Z
Learnt from: tabrisnet
Repo: armbian/build PR: 8678
File: config/kernel/linux-sm8250-current.config:78-80
Timestamp: 2025-09-27T21:49:55.796Z
Learning: In the Armbian build system, kernel configuration files are generated through an automated process: taking previous config → applying scripted changes from armbian-kernel.sh → running kernel's `make oldconfig` → processing by Armbian machinery back into config files. This automated process properly handles kernel configuration dependencies and reduces the likelihood of manual configuration errors.
Applied to files:
config/bootscripts/boot-rk35xx.cmd
📚 Learning: 2025-09-27T21:47:58.020Z
Learnt from: tabrisnet
Repo: armbian/build PR: 8678
File: config/kernel/linux-bcm2711-edge.config:859-861
Timestamp: 2025-09-27T21:47:58.020Z
Learning: In the Armbian build system, kernel configuration files in config/kernel/ are generated through an automated process: taking previous config → applying scripted changes from armbian-kernel.sh → running kernel's `make oldconfig` → processing by Armbian machinery back into config files. This automated process properly handles kernel configuration dependencies and reduces the likelihood of manual configuration errors.
Applied to files:
config/bootscripts/boot-rk35xx.cmd
📚 Learning: 2025-09-27T21:50:15.915Z
Learnt from: tabrisnet
Repo: armbian/build PR: 8678
File: config/kernel/linux-sunxi64-current.config:94-94
Timestamp: 2025-09-27T21:50:15.915Z
Learning: When kernel config files are generated through Armbian's automated process (previous config → armbian-kernel.sh changes → make oldconfig → Armbian machinery processing), manual config file edits are not appropriate since they would be overwritten. Deprecated option handling should be implemented in the automated tooling instead.
Applied to files:
config/bootscripts/boot-rk35xx.cmd
🔇 Additional comments (1)
config/bootscripts/boot-rockchip64.cmd (1)
21-23: LGTM! Consistent fix across Rockchip variants.This change is identical to the one in
config/bootscripts/boot-rk35xx.cmd, ensuring consistent NVMe boot behavior across Rockchip board families. The directloadconditional properly handles device mounting wheretest -efails on NVMe, and aligns with the existing pattern used for overlay loading (lines 50, 56).
|
orangepi5 is rockchip64 family iirc. Why the adjustments for rk35xx? Are there known issues with boards within this family? |
|
I needed the change in both current and vendor branches and believe that I
traced both files to be used for opi5.
I’ll double-check later that this is actually the case.
Do you think this change can be harmful? (Just curious to understand how it
can be)
Sent from phone
пт, 7 лист. 2025 р. о 06:35 Werner ***@***.***> пише:
… *EvilOlaf* left a comment (armbian/build#8885)
<#8885 (comment)>
orangepi5 is rockchip64 family iirc. Why the adjustments for rk35xx? Are
there known issues with boards within this family?
—
Reply to this email directly, view it on GitHub
<#8885 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAIQWWZNXJ37UGLLO34WADL33QORHAVCNFSM6AAAAACLL53U2KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTKMBQGY3DSNJSGY>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
No, probably not. Although I'm not deep enough into this to say with a reasonable certainty. |
|
@sashasimkin Looks to me you're mixing vendor blobs in MTD with mainline u-boot. You can tell by the fact SPL is always 2017 no matter the u-boot version. I'd say repeat your tests with matching blobs. |
|
@EvilOlaf yeah - that was it, here's the full chain:
@rpardini sorry - I'm not sure what you mean by this - as I understand the MTD is stage 1 bootloader, and I have updated it once a year or two ago to switch boot priority to nvme and never touched since. Then that was running official armbian 24.something with 5.10 kernel. Do you suggest I should flush latest rockchip bootloader to MTD and repeat the test? |
Yeah, you're running old/crazy blobs and getting crazy results. I recommend you either wipe the MTD and use eMMC/SD to test, or update the MTD with a matching version for the u-boot proper. Remember: the SoC never loads blobs from NVMe or USB, only ever from SPI/eMMC/SD (in that order, save for maskrom button or DIP switches). In your logs, you're using SPL and blobs from MTD, then u-boot proper from (?), then bootscript from NVMe. Make them match and you'll see the original problem go away. |
rpardini
left a comment
rpardini
left a comment
There was a problem hiding this comment.
Wide-ranging impact here, I'd bet due to mismatched blobs. If this was an actual problem then a few dozen boards would also have it, which isn't the case.
|
Also: your UART logs are slightly corrupted -- shorten your cables and make sure ground is connected. |
Got it, will try flushing latest MTD first.
Yeah, this is also the part that weirded me out. I'll try the suggestions and will learn abit more of u-boot. Thank you so much for the inputs, appreciate them!
The ground is there and cables are like 20cm long, I might've touched the wires in the process or sth. In general - I try to do don't ask for permission, ask for forgiveness, which is sort-of implemented in this PR. What might be actual impact of removing that |
3 participants
Description
For some reason test -e command that checks for armbianEnv.txt file existence fails to mount the first partition. This leads to ignoring of all boot environment, including the critical rootdev variable. This variable is hardcoded to /dev/mmcblk0p1 which works for the default sdcard case, but doesn't for nvme (and likely sata) At the same time the load command that comes after that works well
Therefore the idea behind this change is to not test for file existence and attempt to load it instead on success - import the env variables
This fixes the issues I had while working on CRYPTROOT_AUTOUNLOCK #8805 (comment) .
How Has This Been Tested?
First, I did some extra ad-hoc debug while on nvme:
the key output from that test was:
Then I did the fix and tested it with the following combinations, and attaching the uboot output received over UART console. I noticed one weird thing though - the uboot output for the sdcard case versus nvme. For example I didn't see the "Boot script loaded from" line when booting from SDCard.
For each test I'm also attaching the output of UART console.
Current 6.12, SDCard, Before fix - works 🟢
opi5-boot-sdcard-current-6.12-before-fix.txt
Current 6.12, SDCard, After fix - works 🟢
opi5-boot-sdcard-current-6.12-after-fix.txt
Current 6.12, NVME, Before fix - doesn't work 🔴
opi5-boot-nvme-current-6.12-before-fix.txt
Current 6.12, NVME, After fix - works 🟢
opi5-boot-sdcard-current-6.12-after-fix.txt
Vendor, Cryptroot 6.1, NVME, Before fix - doesn't work 🔴
behavior from this thread
Vendor, Cryptroot 6.1, NVME, After fix - works 🟢
opi5-boot-nvme-crypt-vendor-6.1-after-fix-pr.txt
Checklist:
Summary by CodeRabbit