orangepi5: overhaul board config, update mainline uboot #8632
+248
−63
Conversation
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
efectn
requested review from
HeyMeco,
NicoD-SBC,
SuperKali,
TheSnowfield,
Tonymac32,
amazingfate,
catalinii,
pyavitz,
rpardini,
a team and
igorpecovnik
as code owners
WalkthroughAdds BLUETOOTH_HCIATTACH_PARAMS and enables the bluetooth-hciattach extension. Renames post_family_config_branch_current__orangepi5_use_mainline_uboot() to post_family_config__orangepi5_use_mainline_uboot(), adding an early return for BRANCH=vendor. Updates Orange Pi 5 mainline U-Boot mapping (BOOTBRANCH/BOOTPATCHDIR -> v2025.07) and introduces separate SATA/non‑SATA UBOOT_TARGET_MAP entries using BOOTCONFIG_SATA. Adds SATA-focused hooks for patching, keeping, and packaging U-Boot defconfigs, broadens branch-skip guards to include current, removes edge-only overrides and legacy write_uboot_platform* functions, and modifies rockchip64_common.inc to support u-boot-rockchip-spi*.bin via a FILE_ARRAY and adjusted dd/flashcp logic. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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. Comment |
github-actions
bot
added
Needs review
efectn
force-pushed
the
opi5-mainline-uboot-update2
branch
from
948acc1 to
f0cbf64
Compare
coderabbitai
bot
requested review from
clee,
janprunk,
krachlatte and
PanderMusubi
10 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
config/sources/families/include/rockchip64_common.inc (1)
309-313: Hard-coded file scan is OK, but robustness can be improved (spaces/newlines, deterministic order).
- Use null-delimited find and stable sort; avoids whitespace pitfalls and makes auto-pick deterministic.
Apply:
-FILES=$(find "$1" -maxdepth 1 -type f \( -name "rkspi_loader*.img" -o -name "u-boot-rockchip-spi*.bin" \)) +mapfile -d '' -t FILE_ARRAY < <(find "$1" -maxdepth 1 -type f \( -name "rkspi_loader*.img" -o -name "u-boot-rockchip-spi*.bin" \) -print0 | sort -z) +FILES_COUNT=${#FILE_ARRAY[@]} +if (( FILES_COUNT == 0 )); then + echo "No SPI image found." + exit 1 +fiThen drop the later FILE_ARRAY construction in Lines 317-325 (see next comment).
config/boards/orangepi5.conf (3)
41-55: Target make hack: guard for unexpected patterns and avoid partial matches.
String substitution ontarget_makeis brittle. Anchor on whitespace to avoid corrupting other targets with similar substrings.Apply:
- if [[ $target_make == *"orangepi-5-sata-rk3588s_defconfig"* ]]; then + if [[ " $target_make " == *" orangepi-5-sata-rk3588s_defconfig "* ]]; then BOOTCONFIG="orangepi-5-sata-rk3588s_defconfig" - target_make=${target_make/orangepi-5-sata-rk3588s_defconfig/} + target_make=${target_make/ orangepi-5-sata-rk3588s_defconfig / } else BOOTCONFIG="orangepi-5-rk3588s_defconfig" - target_make=${target_make/orangepi-5-rk3588s_defconfig/} + target_make=${target_make/ orangepi-5-rk3588s_defconfig / } fi
58-69: Quote paths; ensure .config exists before copying.
Avoid failures in unusual tempdirs; be defensive.Apply:
- cp .config ${uboottempdir}/.config.sata + [[ -f .config ]] || exit_with_error "u-boot .config not found" + cp ".config" "${uboottempdir}/.config.sata"
70-77: Quote paths and don’t fail if the SATA config isn’t present.
Use-ffor rm and quote destinations.Apply:
- run_host_command_logged cp ${uboottempdir}/.config.sata "$uboottempdir/usr/lib/u-boot/orangepi-5-sata-rk3588s_defconfig" - run_host_command_logged rm ${uboottempdir}/.config.sata + if [[ -f "${uboottempdir}/.config.sata" ]]; then + run_host_command_logged cp "${uboottempdir}/.config.sata" "${uboottempdir}/usr/lib/u-boot/orangepi-5-sata-rk3588s_defconfig" + run_host_command_logged rm -f "${uboottempdir}/.config.sata" + fi
📜 Review details
Configuration used: CodeRabbit UI
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 ignored due to path filters (1)
patch/u-boot/v2025.04/board_orangepi5/0001-add-sata-dts-and-defconfig-for-OPi5.patchis excluded by!patch/**
📒 Files selected for processing (2)
config/boards/orangepi5.conf(5 hunks)config/sources/families/include/rockchip64_common.inc(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: EvilOlaf
PR: armbian/build#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 or the PR API to get the complete picture of what files are being added or modified.
Learnt from: EvilOlaf
PR: armbian/build#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: rpardini
PR: armbian/build#8044
File: patch/u-boot/v2025.04/cmd-fileenv-read-string-from-file-into-env.patch:76-86
Timestamp: 2025-03-31T22:20:48.475Z
Learning: For the Armbian build project, maintaining consistency with existing patches across U-Boot versions (such as between 2025.01 and 2025.04) is prioritized over refactoring individual patches for code improvements.
Learnt from: rpardini
PR: armbian/build#8044
File: patch/u-boot/v2025.04/cmd-fileenv-read-string-from-file-into-env.patch:73-75
Timestamp: 2025-03-31T22:20:41.849Z
Learning: When porting patches between U-Boot versions (like from 2025.01 to 2025.04), rpardini prefers to maintain patches as-is rather than introducing refactoring changes, even when potential improvements are identified. This approach prioritizes consistency and reduces the risk of introducing new issues.
📚 Learning: 2025-09-11T06:12:54.213Z
Learnt from: SuperKali
PR: armbian/build#8609
File: config/boards/nanopi-r76s.conf:5-5
Timestamp: 2025-09-11T06:12:54.213Z
Learning: In the Armbian build system, board family configuration files (like config/sources/families/rk35xx.conf) can inherit kernel branch definitions from common include files (like config/sources/families/include/rockchip64_common.inc). Even if a branch like "edge" is not defined directly in the family conf file, it may be available through the sourced include file.
Applied to files:
config/boards/orangepi5.conf
📚 Learning: 2025-07-23T07:30:52.265Z
Learnt from: EvilOlaf
PR: armbian/build#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/boards/orangepi5.conf
📚 Learning: 2025-09-12T19:28:38.385Z
Learnt from: Grippy98
PR: armbian/build#8622
File: config/sources/families/k3.conf:66-66
Timestamp: 2025-09-12T19:28:38.385Z
Learning: In the Armbian k3 family build system (config/sources/families/k3.conf), builds do not fail when TIBOOT3_BOOTCONFIG is unset, even though tiboot3.bin is still listed in UBOOT_TARGET_MAP. The gating mechanism in pre_config_uboot_target__build_first_stage function works as intended to conditionally build/copy tiboot3.bin only when TIBOOT3_BOOTCONFIG is defined.
Applied to files:
config/boards/orangepi5.conf
📚 Learning: 2025-03-31T22:20:48.475Z
Learnt from: rpardini
PR: armbian/build#8044
File: patch/u-boot/v2025.04/cmd-fileenv-read-string-from-file-into-env.patch:76-86
Timestamp: 2025-03-31T22:20:48.475Z
Learning: For the Armbian build project, maintaining consistency with existing patches across U-Boot versions (such as between 2025.01 and 2025.04) is prioritized over refactoring individual patches for code improvements.
Applied to files:
config/boards/orangepi5.conf
📚 Learning: 2025-08-02T05:46:10.664Z
Learnt from: EvilOlaf
PR: armbian/build#0
File: :0-0
Timestamp: 2025-08-02T05:46:10.664Z
Learning: In the Armbian build system, the modern recommended approach for kernel configuration is to use the kernel-config command via "./compile.sh BOARD=boardname BRANCH=branchname kernel-config" instead of the deprecated KERNEL_CONFIGURE=yes flag. This provides a two-step workflow: configure using menuconfig, then build, with better transparency and control over configuration changes.
Applied to files:
config/boards/orangepi5.conf
🧬 Code graph analysis (2)
config/sources/families/include/rockchip64_common.inc (3)
config/sources/vendors/mekotronics/mekotronics-rk3588.hooks.sh (1)
write_uboot_platform(39-41)lib/functions/compilation/uboot.sh (1)
uboot_postinst_base(544-562)lib/functions/image/loop.sh (1)
write_uboot_to_loop_image(64-105)
config/boards/orangepi5.conf (2)
extensions/bluetooth-hciattach.sh (3)
post_family_tweaks_bsp__bluetooth_hciattach_add_systemd_service(31-60)post_family_config__bluetooth_hciattach_add_bluetooth_packages(25-28)post_family_tweaks__bluetooth_hciattach_enable_bt_service_in_image(63-69)lib/functions/compilation/uboot.sh (3)
compile_uboot_target(52-299)compile_uboot(373-542)patch_uboot_target(22-48)
🔇 Additional comments (7)
config/sources/families/include/rockchip64_common.inc (2)
317-325: ACK: Tracking absolute paths via FILE_ARRAY is the right call.
No issues. If you adopt the mapfile approach above, you can remove this loop-time FILE_ARRAY population to avoid duplication.
341-346: UI flow LGTM.
Dialog integration and backtitle reuse look correct.config/boards/orangepi5.conf (5)
79-90: USB2 service: broadened skip to current/edge looks right.
Matches the new “mainline on current/edge” intent; no issues.
92-103: Enable USB2 service gating: LGTM.
Consistent with the BSP copy hook above.
105-119: Audio naming gating: LGTM.
Skip on current/edge aligns with the new split.
120-129: Legacy (vendor) SATA/non-SATA target map kept: LGTM.
Clear separation vs. mainline path above.
131-149: Vendor-only SATA SPI image creation: LGTM.
Matches the vendor branch scope.
Comment on lines
+19
to
+21
| declare -g BLUETOOTH_HCIATTACH_PARAMS="-s 115200 /dev/ttyS9 bcm43xx 1500000" # For the bluetooth-hciattach extension | ||
| enable_extension "bluetooth-hciattach" # Enable the bluetooth-hciattach extension | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Bluetooth hciattach: define BLUETOOTH_HCIATTACH_RKFILL_NUM or default to 'all'.
The extension’s script calls rfkill unblock ${BLUETOOTH_HCIATTACH_RKFILL_NUM}; if unset, the unit may fail. Set a safe default.
Apply:
-declare -g BLUETOOTH_HCIATTACH_PARAMS="-s 115200 /dev/ttyS9 bcm43xx 1500000" # For the bluetooth-hciattach extension
+declare -g BLUETOOTH_HCIATTACH_RKFILL_NUM="${BLUETOOTH_HCIATTACH_RKFILL_NUM:-all}" # used by bluetooth-hciattach extension
+declare -g BLUETOOTH_HCIATTACH_PARAMS="-s 115200 /dev/ttyS9 bcm43xx 1500000" # used by bluetooth-hciattach extension🤖 Prompt for AI Agents
In config/boards/orangepi5.conf around lines 19-21,
BLUETOOTH_HCIATTACH_RKFILL_NUM is not defined and the bluetooth-hciattach
extension calls rfkill unblock ${BLUETOOTH_HCIATTACH_RKFILL_NUM}, which can fail
if unset; define a global variable with a safe default (e.g. "all") before
enabling the extension, using the existing declare -g pattern and ensure it uses
the current value if already set (i.e. set to existing value or fallback to
"all") so rfkill always receives a valid argument.
Comment on lines
23
to
39
| function post_family_config__orangepi5_use_mainline_uboot() { | ||
| if [[ $BRANCH == "vendor" ]]; then | ||
| return | ||
| fi | ||
|
|
||
| display_alert "$BOARD" "Mainline U-Boot overrides for $BOARD - $BRANCH" "info" | ||
|
|
||
| declare -g BOOTCONFIG="orangepi-5-rk3588s_defconfig" # override the default for the board/family | ||
| declare -g BOOTDELAY=1 # Wait for UART interrupt to enter UMS/RockUSB mode etc | ||
| declare -g BOOTSOURCE="https://github.com/u-boot/u-boot.git" # We ❤️ mainline U-Boot | ||
| declare -g BOOTBRANCH="tag:v2025.01" | ||
| declare -g BOOTPATCHDIR="v2025.01" | ||
| declare -g BOOTBRANCH="tag:v2025.04" | ||
| declare -g BOOTPATCHDIR="v2025.04" | ||
| declare -g BOOTDIR="u-boot-${BOARD}" # do not share u-boot directory | ||
| declare -g UBOOT_TARGET_MAP="BL31=${RKBIN_DIR}/${BL31_BLOB} ROCKCHIP_TPL=${RKBIN_DIR}/${DDR_BLOB};;u-boot-rockchip.bin u-boot-rockchip-spi.bin" | ||
| unset uboot_custom_postprocess write_uboot_platform write_uboot_platform_mtd # disable stuff from rockchip64_common; we're using binman here which does all the work already | ||
| declare -g UBOOT_TARGET_MAP="BL31=${RKBIN_DIR}/${BL31_BLOB} ROCKCHIP_TPL=${RKBIN_DIR}/${DDR_BLOB} $BOOTCONFIG_SATA;;u-boot-rockchip-spi-sata.bin | ||
| BL31=${RKBIN_DIR}/${BL31_BLOB} ROCKCHIP_TPL=${RKBIN_DIR}/${DDR_BLOB} $BOOTCONFIG;;u-boot-rockchip.bin u-boot-rockchip-spi.bin" | ||
| unset uboot_custom_postprocess # disable stuff from rockchip64_common; we're using binman here which does all the work already | ||
| } |
There was a problem hiding this comment.
Disabling uboot_custom_postprocess: use unset -f or override with a no-op.
unset uboot_custom_postprocess only unsets a variable, not the function. The original function from rockchip64_common will still run.
Apply one of:
-unset uboot_custom_postprocess # disable stuff from rockchip64_common; we're using binman here which does all the work already
+unset -f uboot_custom_postprocess 2>/dev/null || trueor:
+uboot_custom_postprocess() { :; } # override to no-op (binman builds provide the needed artifacts)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function post_family_config__orangepi5_use_mainline_uboot() { | |
| if [[ $BRANCH == "vendor" ]]; then | |
| return | |
| fi | |
| display_alert "$BOARD" "Mainline U-Boot overrides for $BOARD - $BRANCH" "info" | |
| declare -g BOOTCONFIG="orangepi-5-rk3588s_defconfig" # override the default for the board/family | |
| declare -g BOOTDELAY=1 # Wait for UART interrupt to enter UMS/RockUSB mode etc | |
| declare -g BOOTSOURCE="https://github.com/u-boot/u-boot.git" # We ❤️ mainline U-Boot | |
| declare -g BOOTBRANCH="tag:v2025.01" | |
| declare -g BOOTPATCHDIR="v2025.01" | |
| declare -g BOOTBRANCH="tag:v2025.04" | |
| declare -g BOOTPATCHDIR="v2025.04" | |
| declare -g BOOTDIR="u-boot-${BOARD}" # do not share u-boot directory | |
| declare -g UBOOT_TARGET_MAP="BL31=${RKBIN_DIR}/${BL31_BLOB} ROCKCHIP_TPL=${RKBIN_DIR}/${DDR_BLOB};;u-boot-rockchip.bin u-boot-rockchip-spi.bin" | |
| unset uboot_custom_postprocess write_uboot_platform write_uboot_platform_mtd # disable stuff from rockchip64_common; we're using binman here which does all the work already | |
| declare -g UBOOT_TARGET_MAP="BL31=${RKBIN_DIR}/${BL31_BLOB} ROCKCHIP_TPL=${RKBIN_DIR}/${DDR_BLOB} $BOOTCONFIG_SATA;;u-boot-rockchip-spi-sata.bin | |
| BL31=${RKBIN_DIR}/${BL31_BLOB} ROCKCHIP_TPL=${RKBIN_DIR}/${DDR_BLOB} $BOOTCONFIG;;u-boot-rockchip.bin u-boot-rockchip-spi.bin" | |
| unset uboot_custom_postprocess # disable stuff from rockchip64_common; we're using binman here which does all the work already | |
| } | |
| function post_family_config__orangepi5_use_mainline_uboot() { | |
| if [[ $BRANCH == "vendor" ]]; then | |
| return | |
| fi | |
| display_alert "$BOARD" "Mainline U-Boot overrides for $BOARD - $BRANCH" "info" | |
| declare -g BOOTCONFIG="orangepi-5-rk3588s_defconfig" # override the default for the board/family | |
| declare -g BOOTDELAY=1 # Wait for UART interrupt to enter UMS/RockUSB mode etc | |
| declare -g BOOTSOURCE="https://github.com/u-boot/u-boot.git" # We ❤️ mainline U-Boot | |
| declare -g BOOTBRANCH="tag:v2025.04" | |
| declare -g BOOTPATCHDIR="v2025.04" | |
| declare -g BOOTDIR="u-boot-${BOARD}" # do not share u-boot directory | |
| declare -g UBOOT_TARGET_MAP="BL31=${RKBIN_DIR}/${BL31_BLOB} ROCKCHIP_TPL=${RKBIN_DIR}/${DDR_BLOB} $BOOTCONFIG_SATA;;u-boot-rockchip-spi-sata.bin | |
| BL31=${RKBIN_DIR}/${BL31_BLOB} ROCKCHIP_TPL=${RKBIN_DIR}/${DDR_BLOB} $BOOTCONFIG;;u-boot-rockchip.bin u-boot-rockchip-spi.bin" | |
| unset -f uboot_custom_postprocess 2>/dev/null || true # disable stuff from rockchip64_common; we're using binman here which does all the work already | |
| } |
🤖 Prompt for AI Agents
In config/boards/orangepi5.conf around lines 23 to 39, the current call unset
uboot_custom_postprocess only removes a variable but does not remove/override
the function defined in rockchip64_common, so that original function will still
execute; replace that line either with unset -f uboot_custom_postprocess to
remove the function definition, or define a no-op function
(uboot_custom_postprocess() { :; }) to override it in this file so the
rockchip64_common implementation is not invoked.
💡 Verification agent
🧩 Analysis chain
Sanity-check that packaging picks up the intended artifacts and no legacy rkspi_loader.img sneaks in.
Run a quick verification after a test build to ensure only the new binman outputs are packaged for current/edge and that the MTD writer uses the passed device.
Also applies to: 41-55, 308-359
🏁 Script executed:
#!/bin/bash
# 1) List packaged u-boot artifacts for OPi5 current/edge; expect u-boot-rockchip*.bin entries:
fd -a --type f --hidden -g 'u-boot-rockchip*' .
fd -a --type f --hidden -g 'rkspi_loader*.img' . | sed 's/^/LEGACY_SPI_IMG_FOUND: /'
# 2) Confirm uboot_custom_postprocess isn't present at runtime (function overridden/unset):
rg -n 'uboot_custom_postprocess\(' config/boards/orangepi5.conf config/sources/families/include/rockchip64_common.inc || true
# 3) Check write_uboot_platform_mtd writes to the passed device, not /dev/mtd0:
rg -n 'write_uboot_platform_mtd' -C3 config/sources/families/include/rockchip64_common.inc
rg -n 'flashcp.* /dev/mtd0' config/sources/families/include/rockchip64_common.incLength of output: 404
Fix hard-coded MTD device, stop legacy rkspi_loader.img selection, and properly disable uboot_custom_postprocess
- config/sources/families/include/rockchip64_common.inc:
- write_uboot_platform_mtd() (lines ~308–353) hard-codes /dev/mtd0 (flashcp at lines 332 and 353). Change to use the passed device parameter instead of /dev/mtd0.
- FILES find (line ~309) still matches rkspi_loader*.img — remove that pattern or make the selection prefer u-boot-rockchip-spi*.bin so legacy rkspi_loader.img cannot be chosen.
- config/boards/orangepi5.conf (lines ~23–39) uses
unset uboot_custom_postprocess— that does not remove a shell function. Useunset -f uboot_custom_postprocessor explicitly override the function to disable rockchip64_common.inc's behavior.
🤖 Prompt for AI Agents
In config/boards/orangepi5.conf around lines 23-39 and in
config/sources/families/include/rockchip64_common.inc (write_uboot_platform_mtd
function ~lines 308-353), fix three issues: (1) stop hard-coding /dev/mtd0 in
write_uboot_platform_mtd — use the device parameter passed into the function
wherever /dev/mtd0 is referenced (including the flashcp calls) so the target MTD
device is honored; (2) adjust the FILES discovery logic so it no longer matches
legacy rkspi_loader*.img (remove that pattern or change selection to prefer
u-boot-rockchip-spi*.bin first and only fall back to rkspi_loader if explicitly
intended) to avoid accidental selection of the old loader; and (3) in
config/boards/orangepi5.conf replace the ineffective unset
uboot_custom_postprocess with a proper function removal/override — either call
unset -f uboot_custom_postprocess or provide an empty stub function definition
to disable the rockchip64_common.inc postprocess behavior.
|
Any reason not to jump directly to 2025.07 uboot? |
Done, updated |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
config/boards/orangepi5.conf (2)
19-21: Bluetooth hciattach: define rfkill index with safe default.Without BLUETOOTH_HCIATTACH_RKFILL_NUM, rfkill unblock may fail in the extension.
Apply:
+declare -g BLUETOOTH_HCIATTACH_RKFILL_NUM="${BLUETOOTH_HCIATTACH_RKFILL_NUM:-all}" # used by bluetooth-hciattach extension declare -g BLUETOOTH_HCIATTACH_PARAMS="-s 115200 /dev/ttyS9 bcm43xx 1500000" # For the bluetooth-hciattach extension enable_extension "bluetooth-hciattach" # Enable the bluetooth-hciattach extension
38-38:unset uboot_custom_postprocessdoesn’t remove the function.The original function from rockchip64_common.inc will still run.
Apply one:
-unset uboot_custom_postprocess # disable stuff from rockchip64_common; we're using binman here which does all the work already +unset -f uboot_custom_postprocess 2>/dev/null || trueor:
-unset uboot_custom_postprocess # disable stuff from rockchip64_common; we're using binman here which does all the work already +uboot_custom_postprocess() { :; } # no-op override; binman already provides artifacts
🧹 Nitpick comments (2)
config/boards/orangepi5.conf (2)
41-55: SATA bootconfig “hack”: reduce brittleness by reusing vars and precise replacement.Safer to reference BOOTCONFIG[_SATA] vars and avoid hardcoded strings.
Apply:
function pre_config_uboot_target__orangepi5_patch_uboot_bootconfig_hack_for_sata() { @@ - if [[ $target_make == *"orangepi-5-sata-rk3588s_defconfig"* ]]; then - BOOTCONFIG="orangepi-5-sata-rk3588s_defconfig" - target_make=${target_make/orangepi-5-sata-rk3588s_defconfig/} + local default_defconfig="${BOOTCONFIG}" + local sata_defconfig="${BOOTCONFIG_SATA}" + if [[ "$target_make" == *"$sata_defconfig"* ]]; then + BOOTCONFIG="$sata_defconfig" + target_make="${target_make/$sata_defconfig/}" else - BOOTCONFIG="orangepi-5-rk3588s_defconfig" - target_make=${target_make/orangepi-5-rk3588s_defconfig/} + BOOTCONFIG="$default_defconfig" + target_make="${target_make/$default_defconfig/}" fi
58-68: Use logged wrapper and quote paths when copying .config.Align with surrounding style and avoid issues with spaces.
Apply:
- if [[ $BOOTCONFIG == "orangepi-5-sata-rk3588s_defconfig" ]]; then - cp .config ${uboottempdir}/.config.sata + if [[ "$BOOTCONFIG" == "orangepi-5-sata-rk3588s_defconfig" ]]; then + run_host_command_logged cp .config "${uboottempdir}/.config.sata" fi
📜 Review details
Configuration used: CodeRabbit UI
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 ignored due to path filters (1)
patch/u-boot/v2025.07/board_orangepi5/0001-add-sata-dts-and-defconfig-for-OPi5.patchis excluded by!patch/**
📒 Files selected for processing (1)
config/boards/orangepi5.conf(5 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: rpardini
PR: armbian/build#8044
File: patch/u-boot/v2025.04/cmd-fileenv-read-string-from-file-into-env.patch:76-86
Timestamp: 2025-03-31T22:20:48.475Z
Learning: For the Armbian build project, maintaining consistency with existing patches across U-Boot versions (such as between 2025.01 and 2025.04) is prioritized over refactoring individual patches for code improvements.
Learnt from: amazingfate
PR: armbian/build#8619
File: config/sources/families/rockchip.conf:222-230
Timestamp: 2025-09-14T06:32:29.786Z
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: rpardini
PR: armbian/build#8044
File: patch/u-boot/v2025.04/cmd-fileenv-read-string-from-file-into-env.patch:73-75
Timestamp: 2025-03-31T22:20:41.849Z
Learning: When porting patches between U-Boot versions (like from 2025.01 to 2025.04), rpardini prefers to maintain patches as-is rather than introducing refactoring changes, even when potential improvements are identified. This approach prioritizes consistency and reduces the risk of introducing new issues.
📚 Learning: 2025-09-14T06:32:29.786Z
Learnt from: amazingfate
PR: armbian/build#8619
File: config/sources/families/rockchip.conf:222-230
Timestamp: 2025-09-14T06:32:29.786Z
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/boards/orangepi5.conf
📚 Learning: 2025-09-12T19:28:38.385Z
Learnt from: Grippy98
PR: armbian/build#8622
File: config/sources/families/k3.conf:66-66
Timestamp: 2025-09-12T19:28:38.385Z
Learning: In the Armbian k3 family build system (config/sources/families/k3.conf), builds do not fail when TIBOOT3_BOOTCONFIG is unset, even though tiboot3.bin is still listed in UBOOT_TARGET_MAP. The gating mechanism in pre_config_uboot_target__build_first_stage function works as intended to conditionally build/copy tiboot3.bin only when TIBOOT3_BOOTCONFIG is defined.
Applied to files:
config/boards/orangepi5.conf
📚 Learning: 2025-09-14T06:29:18.937Z
Learnt from: amazingfate
PR: armbian/build#8619
File: config/sources/families/rockchip.conf:64-70
Timestamp: 2025-09-14T06:29:18.937Z
Learning: In the Armbian build system, vendor branch configurations in family files are designed to be shared across multiple SoCs within the same family that use the same vendor kernel tree. For example, rk35xx and rockchip-rk3588 families both use identical vendor branch settings (same KERNELSOURCE, KERNELBRANCH, and KERNELPATCHDIR), demonstrating that vendor branches are intentionally generic rather than SoC-specific.
Applied to files:
config/boards/orangepi5.conf
📚 Learning: 2025-09-11T06:12:54.213Z
Learnt from: SuperKali
PR: armbian/build#8609
File: config/boards/nanopi-r76s.conf:5-5
Timestamp: 2025-09-11T06:12:54.213Z
Learning: In the Armbian build system, board family configuration files (like config/sources/families/rk35xx.conf) can inherit kernel branch definitions from common include files (like config/sources/families/include/rockchip64_common.inc). Even if a branch like "edge" is not defined directly in the family conf file, it may be available through the sourced include file.
Applied to files:
config/boards/orangepi5.conf
📚 Learning: 2025-07-23T07:30:52.265Z
Learnt from: EvilOlaf
PR: armbian/build#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/boards/orangepi5.conf
📚 Learning: 2025-03-31T22:20:48.475Z
Learnt from: rpardini
PR: armbian/build#8044
File: patch/u-boot/v2025.04/cmd-fileenv-read-string-from-file-into-env.patch:76-86
Timestamp: 2025-03-31T22:20:48.475Z
Learning: For the Armbian build project, maintaining consistency with existing patches across U-Boot versions (such as between 2025.01 and 2025.04) is prioritized over refactoring individual patches for code improvements.
Applied to files:
config/boards/orangepi5.conf
📚 Learning: 2025-08-02T05:46:10.664Z
Learnt from: EvilOlaf
PR: armbian/build#0
File: :0-0
Timestamp: 2025-08-02T05:46:10.664Z
Learning: In the Armbian build system, the modern recommended approach for kernel configuration is to use the kernel-config command via "./compile.sh BOARD=boardname BRANCH=branchname kernel-config" instead of the deprecated KERNEL_CONFIGURE=yes flag. This provides a two-step workflow: configure using menuconfig, then build, with better transparency and control over configuration changes.
Applied to files:
config/boards/orangepi5.conf
🔇 Additional comments (7)
config/boards/orangepi5.conf (7)
23-27: LGTM: vendor branch guard.Early-return for BRANCH=vendor is correct.
80-80: LGTM: skip USB2 service on current/edge.
93-93: LGTM: gating BSP tweaks to vendor only.
106-106: LGTM: vendor-only audio renames.
121-121: LGTM: vendor-only SATA rkspi target map retained.
132-132: LGTM: vendor-only rkspi_loader_sata.img creation.
33-37: v2025.07 bump & dual-target map OK — patch dir missing; fix that.
- patch/u-boot/v2025.07 is absent — add the patch dir or update BOOTPATCHDIR in config/boards/orangepi5.conf (currently set to v2025.07).
- config/boards/orangepi5.conf unsets uboot_custom_postprocess, so adding declare -g BOOT_SPI_RKSPI_LOADER="no" is redundant but harmless if you want it explicit.
⛔ Skipped due to learnings
Learnt from: EvilOlaf PR: armbian/build#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.Learnt from: rpardini PR: armbian/build#8044 File: patch/u-boot/v2025.04/cmd-fileenv-read-string-from-file-into-env.patch:76-86 Timestamp: 2025-03-31T22:20:48.475Z Learning: For the Armbian build project, maintaining consistency with existing patches across U-Boot versions (such as between 2025.01 and 2025.04) is prioritized over refactoring individual patches for code improvements.Learnt from: rpardini PR: armbian/build#8044 File: patch/u-boot/v2025.04/cmd-fileenv-read-string-from-file-into-env.patch:73-75 Timestamp: 2025-03-31T22:20:41.849Z Learning: When porting patches between U-Boot versions (like from 2025.01 to 2025.04), rpardini prefers to maintain patches as-is rather than introducing refactoring changes, even when potential improvements are identified. This approach prioritizes consistency and reduces the risk of introducing new issues.Learnt from: amazingfate PR: armbian/build#8619 File: config/sources/families/rockchip.conf:65-72 Timestamp: 2025-09-14T11:37:35.077Z Learning: In the Armbian build system, patch directories referenced in BOOTPATCHDIR and KERNELPATCHDIR configurations can be non-existent without causing build failures. Empty patch directories are also ignored by git, so missing patch directories should not be flagged as errors during code review.Learnt from: amazingfate PR: armbian/build#8619 File: config/sources/families/rockchip.conf:64-70 Timestamp: 2025-09-14T06:29:18.937Z Learning: In the Armbian build system, vendor branch configurations in family files are designed to be shared across multiple SoCs within the same family that use the same vendor kernel tree. For example, rk35xx and rockchip-rk3588 families both use identical vendor branch settings (same KERNELSOURCE, KERNELBRANCH, and KERNELPATCHDIR), demonstrating that vendor branches are intentionally generic rather than SoC-specific.Learnt from: SuperKali PR: armbian/build#8609 File: config/boards/nanopi-r76s.conf:5-5 Timestamp: 2025-09-11T06:12:54.213Z Learning: In the Armbian build system, board family configuration files (like config/sources/families/rk35xx.conf) can inherit kernel branch definitions from common include files (like config/sources/families/include/rockchip64_common.inc). Even if a branch like "edge" is not defined directly in the family conf file, it may be available through the sourced include file.Learnt from: amazingfate PR: armbian/build#8619 File: config/sources/families/rockchip.conf:222-230 Timestamp: 2025-09-14T06:32:29.786Z 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: Grippy98 PR: armbian/build#8622 File: config/sources/families/k3.conf:66-66 Timestamp: 2025-09-12T19:28:38.385Z Learning: In the Armbian k3 family build system (config/sources/families/k3.conf), builds do not fail when TIBOOT3_BOOTCONFIG is unset, even though tiboot3.bin is still listed in UBOOT_TARGET_MAP. The gating mechanism in pre_config_uboot_target__build_first_stage function works as intended to conditionally build/copy tiboot3.bin only when TIBOOT3_BOOTCONFIG is defined.Learnt from: EvilOlaf PR: armbian/build#8586 File: config/boards/nanopi-r76s.conf:15-21 Timestamp: 2025-09-07T17:39:32.272Z Learning: In the Armbian build system, the variables $BOARD and $SDCARD are always set by the build framework, so guard checks for these variables are unnecessary in board configuration files and hook functions.
Comment on lines
+70
to
+77
| function pre_package_uboot_image__orangepi5_copy_sataconfig_to_pacage() { | ||
| if [[ $BRANCH == "vendor" ]]; then | ||
| return | ||
| fi | ||
|
|
||
| run_host_command_logged cp ${uboottempdir}/.config.sata "$uboottempdir/usr/lib/u-boot/orangepi-5-sata-rk3588s_defconfig" | ||
| run_host_command_logged rm ${uboottempdir}/.config.sata | ||
| } |
There was a problem hiding this comment.
Packaging hook can fail when .config.sata doesn’t exist; also fix function name typo.
Guard the copy/remove, quote paths, and make rm idempotent.
Apply:
-function pre_package_uboot_image__orangepi5_copy_sataconfig_to_pacage() {
+function pre_package_uboot_image__orangepi5_copy_sataconfig_to_package() {
@@
- run_host_command_logged cp ${uboottempdir}/.config.sata "$uboottempdir/usr/lib/u-boot/orangepi-5-sata-rk3588s_defconfig"
- run_host_command_logged rm ${uboottempdir}/.config.sata
+ if [[ -f "${uboottempdir}/.config.sata" ]]; then
+ run_host_command_logged cp "${uboottempdir}/.config.sata" "${uboottempdir}/usr/lib/u-boot/orangepi-5-sata-rk3588s_defconfig"
+ run_host_command_logged rm -f "${uboottempdir}/.config.sata"
+ fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function pre_package_uboot_image__orangepi5_copy_sataconfig_to_pacage() { | |
| if [[ $BRANCH == "vendor" ]]; then | |
| return | |
| fi | |
| run_host_command_logged cp ${uboottempdir}/.config.sata "$uboottempdir/usr/lib/u-boot/orangepi-5-sata-rk3588s_defconfig" | |
| run_host_command_logged rm ${uboottempdir}/.config.sata | |
| } | |
| function pre_package_uboot_image__orangepi5_copy_sataconfig_to_package() { | |
| if [[ $BRANCH == "vendor" ]]; then | |
| return | |
| fi | |
| if [[ -f "${uboottempdir}/.config.sata" ]]; then | |
| run_host_command_logged cp "${uboottempdir}/.config.sata" "${uboottempdir}/usr/lib/u-boot/orangepi-5-sata-rk3588s_defconfig" | |
| run_host_command_logged rm -f "${uboottempdir}/.config.sata" | |
| fi | |
| } |
🤖 Prompt for AI Agents
In config/boards/orangepi5.conf around lines 70 to 77, the function name has a
typo and the packaging hook can fail if ${uboottempdir}/.config.sata is missing;
update the function name to
pre_package_uboot_image__orangepi5_copy_sataconfig_to_package, wrap all path
expansions in double quotes, check for the file's existence before attempting to
cp, and make the removal idempotent (e.g., only rm if the file exists or ignore
errors) so the hook won't error when .config.sata is absent.
7 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
How Has This Been Tested?
Checklist: