Skip to content

orangepi5: overhaul uboot config #8124

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

efectn
Copy link
Member

@efectn efectn commented Apr 26, 2025

Description

GitHub issue reference:
Jira reference number [AR-9999]

Documentation summary for feature / change

Please delete this section if entry to main documentation is not needed.

If documentation entry is predicted, please provide key elements for further implementation into main documentation and set label to "Needs Documentation". You are welcome to open a PR to documentation or you can leave following information for technical writer:

  • short description (copy / paste of PR title)
  • summary (description relevant for end users)
  • example of usage (how to see this in function)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Please also note any relevant details for your test configuration.

  • Tested sata spi image boot.
  • Tested sata doesn't boot with normal spi image.

Checklist:

Please delete options that are not relevant.

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

Copy link
Contributor

coderabbitai bot commented Apr 26, 2025

Walkthrough

This change introduces Bluetooth HCI attach support for the Orange Pi 5 by enabling relevant parameters and extensions. It updates U-Boot handling to use the 2025.04 mainline branch, adds SATA SPI image support, and rewrites the SPI image flashing logic to allow interactive selection or automatic flashing in non-interactive environments. Several new functions manage SATA-specific boot configuration and package handling. Functions that previously skipped execution on the "edge" branch now also skip on the "current" branch. Legacy edge branch overrides are removed, and internal improvements are made to the SPI image flashing logic for clarity and robustness.

Possibly related PRs

Suggested labels

ready to merge

Suggested reviewers

  • efectn
  • rpardini
  • igorpecovnik
  • catalinii

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need 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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@github-actions github-actions bot added 05 Milestone: Second quarter release size/large PR with 250 lines or more labels Apr 26, 2025
@efectn efectn changed the title orangepi5: overhaul board config, update mainline uboot orangepi5: overhaul uboot config Apr 26, 2025
@github-actions github-actions bot added Needs review Seeking for review Hardware Hardware related like kernel, U-Boot, ... Patches Patches related to kernel, U-Boot, ... labels Apr 26, 2025
@efectn efectn force-pushed the opi5-mainline-uboot-update branch from 4f090e8 to ad35423 Compare April 26, 2025 20:37
@coderabbitai coderabbitai bot added the Ready to merge Reviewed, tested and ready for merge label Apr 26, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (2)
config/boards/orangepi5.conf (2)

46-52: Prefer robust array capture over string parsing

FILES=$(find …) followed by while read … <<< "$FILES" works, but:

  • filenames containing spaces or newlines can still break the loop;
  • reading through a here-string iterates only once if $FILES contains NULs (unlikely, but still).

A safer, purely array-based approach:

-FILES=$(find "$1" -maxdepth 1 -type f -name "u-boot-rockchip-spi*.bin")
+readarray -t FILES < <(find "$1" -maxdepth 1 -type f -name 'u-boot-rockchip-spi*.bin' -print0 | tr '\0' '\n')

and then iterate over "${FILES[@]}".


47-50: Use return instead of exit inside sourced functions

write_uboot_platform_mtd is sourced by the Armbian build framework.
Calling exit 1 propagates all the way up and aborts the entire build, even in cases where the caller might want to handle the error.
Replacing with return 1 keeps the failure local while still signalling an error.

-  echo "No SPI image found."
-  exit 1
+  echo "No SPI image found."
+  return 1-  echo "No SPI image chosen."
-  exit 1
+  echo "No SPI image chosen."
+  return 1

Also applies to: 81-83

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a4b1edd and ad35423.

⛔ Files ignored due to path filters (1)
  • patch/u-boot/v2025.04/board_orangepi5/0001-add-sata-dts-and-defconfig-for-OPi5.patch is excluded by !patch/**
📒 Files selected for processing (1)
  • config/boards/orangepi5.conf (6 hunks)
🔇 Additional comments (5)
config/boards/orangepi5.conf (5)

19-21: Bluetooth extension correctly enabled

The bluetooth-hciattach extension is enabled with appropriate BLUETOOTH_HCIATTACH_PARAMS; no issues spotted.


33-38: Verify multiline UBOOT_TARGET_MAP formatting

UBOOT_TARGET_MAP is declared as a single-quoted string spanning two physical lines.
Some downstream helpers treat newlines as delimiters; others treat the value verbatim.
Please double-check that the newline embedded here is really expected by the parser; if not, replace the literal line-break with a single space (or explicit \n) to avoid subtle split/IFS problems.


63-66: Single-image fast-path looks good

Fast-flashing when only one SPI image is present avoids an unnecessary dialog and is implemented correctly.


94-100: Confirm BOOTCONFIG override order

pre_config_uboot_target__orangepi5_patch_uboot_bootconfig_hack_for_sata() rewrites BOOTCONFIG, but post_family_config__orangepi5_use_mainline_uboot() (executed earlier) sets the same variable unconditionally.
Because pre_config_* hooks run later, this is probably fine; nevertheless, please verify that:

  1. the final value of BOOTCONFIG matches the selected target_make;
  2. subsequent scripts (e.g. post_uboot_*) don’t rely on the old value.

A quick echo in the build logs would suffice for validation.


104-105: Branch gating widened – double-check vendor-only expectations

These functions now skip for both edge and current, leaving only vendor builds to receive USB2/audio/SATA tweaks.
Ensure this is intentional; otherwise, current images may lose USB2 initialisation, udev audio naming, and SATA SPI generation unexpectedly.

Also applies to: 117-118, 130-131, 145-146, 156-157

@efectn efectn force-pushed the opi5-mainline-uboot-update branch from ad35423 to f765512 Compare April 26, 2025 20:40
@github-actions github-actions bot removed the Ready to merge Reviewed, tested and ready for merge label Apr 26, 2025
@coderabbitai coderabbitai bot added the Ready to merge Reviewed, tested and ready for merge label Apr 26, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (1)
config/boards/orangepi5.conf (1)

45-84: ⚠️ Potential issue

Fix array-index arithmetic – current form causes a syntax error

bash does not accept parentheses inside the sub-script arithmetic context.
${MENU_ITEMS[($CHOICE*3)-2]} raises:

bash: MENU_ITEMS[(1*3)-2]: syntax error: operand expected

This is a runtime blocker because the function exits before flashing.

Apply this fix:

-			flashcp -v -p "$1/${MENU_ITEMS[($CHOICE*3)-2]}" /dev/mtd0
+			idx=$((CHOICE*3-2))
+			flashcp -v -p "$1/${MENU_ITEMS[$idx]}" /dev/mtd0

The rest of the SPI image selection logic is well implemented, providing a user-friendly way to choose between multiple images or automatically selecting the only available image.

🧹 Nitpick comments (1)
config/boards/orangepi5.conf (1)

87-101: New SATA boot configuration function works but has inconsistent indentation

The new function correctly patches the U-Boot configuration for SATA support based on the target configuration. However, there's inconsistent indentation in the implementation.

Apply this fix to maintain consistent indentation:

 	if [[ $target_make == *"orangepi-5-sata-rk3588s_defconfig"* ]]; then
-  		BOOTCONFIG="orangepi-5-sata-rk3588s_defconfig"
+		BOOTCONFIG="orangepi-5-sata-rk3588s_defconfig"
 		target_make=${target_make/orangepi-5-sata-rk3588s_defconfig/}
 	else
-  		BOOTCONFIG="orangepi-5-rk3588s_defconfig"
+		BOOTCONFIG="orangepi-5-rk3588s_defconfig"
 		target_make=${target_make/orangepi-5-rk3588s_defconfig/}
 	fi
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ad35423 and f765512.

⛔ Files ignored due to path filters (1)
  • patch/u-boot/v2025.04/board_orangepi5/0001-add-sata-dts-and-defconfig-for-OPi5.patch is excluded by !patch/**
📒 Files selected for processing (1)
  • config/boards/orangepi5.conf (6 hunks)
🔇 Additional comments (4)
config/boards/orangepi5.conf (4)

19-20: LGTM: Bluetooth configuration is properly added

The Bluetooth HCI attach parameters are properly configured for the BCM43xx chipset with the appropriate UART port and baud rate. Enabling the bluetooth-hciattach extension is correctly done.


23-38: Improved U-Boot configuration for better branch handling

The function rename from branch-specific to generic naming allows unified handling for both current and edge branches. The U-Boot version update to 2025.04 is a good improvement. The UBOOT_TARGET_MAP now properly includes SATA SPI images.


104-105: Branch handling now unified for edge and current branches

Functions now correctly skip execution for both "edge" and "current" branches instead of just "edge", reducing redundancy and aligning with the PR objective to use the same U-Boot for both builds.

Also applies to: 117-118, 130-131, 145-146, 156-157


62-66: LGTM: Efficient handling of single SPI image case

The code correctly detects when only one SPI image is available and intelligently bypasses the dialog, directly flashing the image. This is a good user experience improvement.

@Kwiboo
Copy link
Contributor

Kwiboo commented Apr 27, 2025

@efectn why are armbian creating a special rkspi_loader*.img? why not just use the u-boot-rockchip-spi.bin that mainline U-Boot build will produce?

@efectn
Copy link
Member Author

efectn commented Apr 27, 2025

@efectn why are armbian creating a special rkspi_loader*.img? why not just use the u-boot-rockchip-spi.bin that mainline U-Boot build will produce?

Actually we use both for rk3588 boards despite the fact binman is not used by main rockchip64 files. For legacy uboot, i am not sure whether binman is supported. However, we use binman for mainline by overriding uboot procedure like https://github.com/armbian/build/blob/main/config/boards/orangepi5.conf#L33

In fact, i am not sure why another boards having mainline uboot still use rkspi_loader*.img, maybe some maintenance reasons?

@The-going
Copy link
Contributor

  • Add sata spi image to current, edge uboot builds. It is a bit hacky, but it works.

Please tell me how do you connect a SATA drive to this orangepi5 device?

@efectn
Copy link
Member Author

efectn commented May 6, 2025

  • Add sata spi image to current, edge uboot builds. It is a bit hacky, but it works.

Please tell me how do you connect a SATA drive to this orangepi5 device?

I use mSata.

@The-going
Copy link
Contributor

  • Add sata spi image to current, edge uboot builds. It is a bit hacky, but it works.

Please tell me how do you connect a SATA drive to this orangepi5 device?

I use mSata.

Please don't think I'm being rude.
My device only has a PCIe 2.0 connector.
Are you using a PCIe-mSATA adapter?

@rpardini
Copy link
Member

rpardini commented May 6, 2025

However, we use binman for mainline by overriding uboot procedure like main/config/boards/orangepi5.conf#L33

"Hook-based custom u-boot setup for binman", from before we had BOOT_SCENARIO=binman -- also I think the only option for mixed vendor/mainline u-boot boards.

I will ! one day find it in me to refactor all those boards into an rk-u-boot-mainline extension, but it is not today -- let the copy/paste reign 😈 at least while vendor/non-binman u-boot for some reason still exists.

why another boards having mainline uboot still use rkspi_loader*.img

I can only speculate: rkspi_loader was for wrapping the u-boot and idbloader into a gpt with labels the way vendor u-boot wanted it to be. I think when mainline was introduced (pre-binman) they kept the name to reuse the gpt-making/idb-writing function.

When binman came around (via hooks) I used binman's nomenclature, but again only in specific boards, not rockchip64_common.

@efectn
Copy link
Member Author

efectn commented May 6, 2025

  • Add sata spi image to current, edge uboot builds. It is a bit hacky, but it works.

Please tell me how do you connect a SATA drive to this orangepi5 device?

I use mSata.

Please don't think I'm being rude. My device only has a PCIe 2.0 connector. Are you using a PCIe-mSATA adapter?

The Opi5 board provides M key m2 connector which is connected to combphy of RK3588. By activating pcie connected to combphy, you enable pcie pins to use pcie2. On the other hand, you can do the same for sata devices without using external pcie msata adapter as m2 connecter can also be used for m2 sata devices.
Screenshot_2025-05-06-21-42-32-313_com google android apps docs-edit
Screenshot_2025-05-06-21-41-44-312_com google android apps docs-edit

@igorpecovnik
Copy link
Member

Ready?

@efectn
Copy link
Member Author

efectn commented May 12, 2025

Ready?

Will add some conditions to check whether the terminal is interactive, then it should be ready to merge.

@igorpecovnik igorpecovnik removed the Ready to merge Reviewed, tested and ready for merge label May 13, 2025
@igorpecovnik igorpecovnik added the Work in progress Unfinished / work in progress label May 13, 2025
@efectn efectn force-pushed the opi5-mainline-uboot-update branch from f765512 to 45e7554 Compare May 25, 2025 19:37
@efectn efectn requested review from NicoD-SBC, rpardini, a team and igorpecovnik as code owners May 25, 2025 19:37
@github-actions github-actions bot added the Framework Framework components label May 25, 2025
@coderabbitai coderabbitai bot added the Ready to merge Reviewed, tested and ready for merge label May 25, 2025
@efectn efectn force-pushed the opi5-mainline-uboot-update branch from 45e7554 to 8f0af6f Compare May 25, 2025 19:39
@github-actions github-actions bot removed the Ready to merge Reviewed, tested and ready for merge label May 25, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.

Actionable comments posted: 4

♻️ Duplicate comments (1)
config/boards/orangepi5.conf (1)

79-80: Array indexing fix implemented correctly.

The array indexing issue from the past review has been properly addressed by calculating the index in a separate variable before using it. This matches the fix applied in the common include file.

🛑 Comments failed to post (4)
config/boards/orangepi5.conf (3)

36-37: 💡 Verification agent

🧩 Analysis chain

Verify SATA defconfig availability.

The UBOOT_TARGET_MAP references $BOOTCONFIG_SATA (orangepi-5-sata-rk3588s_defconfig). Based on past review comments, this defconfig may not exist in the expected location.


🏁 Script executed:

#!/bin/bash
# Description: Check for SATA defconfig availability in U-Boot source
echo "Searching for orangepi-5-sata-rk3588s_defconfig in U-Boot configs..."
fd "orangepi-5-sata-rk3588s_defconfig" || echo "SATA defconfig not found"

echo "Checking for similar Orange Pi 5 configs..."
fd "orangepi.*5.*defconfig" | head -10

Length of output: 323


Missing SATA defconfig: orangepi-5-sata-rk3588s_defconfig

The UBOOT_TARGET_MAP in config/boards/orangepi5.conf lines 36–37 references $BOOTCONFIG_SATA (orangepi-5-sata-rk3588s_defconfig), but no matching defconfig was found in the U-Boot configs/ directory. Please either:

  • Add orangepi-5-sata-rk3588s_defconfig to the U-Boot configs/ tree, or
  • Update/replace $BOOTCONFIG_SATA to point at an existing defconfig (e.g. orangepi-5-rk3588s_defconfig), or
  • Remove the SATA entry if it isn’t supported for this board.

• Location: config/boards/orangepi5.conf (lines 36–37)
• Issue: Missing defconfig file for SATA build target

🤖 Prompt for AI Agents
In config/boards/orangepi5.conf at lines 36-37, the UBOOT_TARGET_MAP references
$BOOTCONFIG_SATA which points to orangepi-5-sata-rk3588s_defconfig, but this
defconfig file does not exist in the U-Boot configs directory. To fix this,
either add the missing orangepi-5-sata-rk3588s_defconfig file to the U-Boot
configs tree, or update $BOOTCONFIG_SATA to reference an existing defconfig like
orangepi-5-rk3588s_defconfig, or remove the SATA entry from UBOOT_TARGET_MAP if
SATA support is not intended for this board.

122-123: 🛠️ Refactor suggestion

Potential issue with SATA config file handling.

The function assumes ${uboottempdir}/.config.sata exists, but if the SATA config wasn't saved in the previous hook, this will fail. Also, the removal of the temporary file should be conditional.

-	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 ${uboottempdir}/.config.sata
+	else
+		display_alert "Warning: SATA config not found, skipping copy" "" "warn"
+	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.

    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 ${uboottempdir}/.config.sata
    else
        display_alert "Warning: SATA config not found, skipping copy" "" "warn"
    fi
🤖 Prompt for AI Agents
In config/boards/orangepi5.conf around lines 122 to 123, the code assumes the
file ${uboottempdir}/.config.sata exists before copying and removing it, which
can cause errors if the file is missing. Modify the script to first check if the
file exists before attempting to copy and remove it, ensuring the removal
command only runs conditionally when the file is present.

88-102: 🛠️ Refactor suggestion

Review SATA bootconfig manipulation logic.

The function manipulates target_make by removing defconfig strings and sets BOOTCONFIG based on the presence of SATA defconfig in target_make. However, this approach seems fragile and could lead to incorrect behavior if the string replacement fails.

Consider a more robust approach:

-	if [[ $target_make == *"orangepi-5-sata-rk3588s_defconfig"* ]]; then
-  		BOOTCONFIG="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/}
-	fi
+	if [[ $target_make == *"orangepi-5-sata-rk3588s_defconfig"* ]]; then
+		BOOTCONFIG="orangepi-5-sata-rk3588s_defconfig"
+		target_make="${target_make//orangepi-5-sata-rk3588s_defconfig/}"
+	elif [[ $target_make == *"orangepi-5-rk3588s_defconfig"* ]]; then
+		BOOTCONFIG="orangepi-5-rk3588s_defconfig"
+		target_make="${target_make//orangepi-5-rk3588s_defconfig/}"
+	else
+		display_alert "Warning: Unknown defconfig in target_make" "$target_make" "warn"
+	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.

function pre_config_uboot_target__orangepi5_patch_uboot_bootconfig_hack_for_sata() {
    if [[ $BRANCH == "vendor" ]]; then
        return
    fi

    display_alert "u-boot for ${BOARD}" "u-boot: hack bootconfig for sata spi image" "info"

    if [[ $target_make == *"orangepi-5-sata-rk3588s_defconfig"* ]]; then
        BOOTCONFIG="orangepi-5-sata-rk3588s_defconfig"
        target_make="${target_make//orangepi-5-sata-rk3588s_defconfig/}"
    elif [[ $target_make == *"orangepi-5-rk3588s_defconfig"* ]]; then
        BOOTCONFIG="orangepi-5-rk3588s_defconfig"
        target_make="${target_make//orangepi-5-rk3588s_defconfig/}"
    else
        display_alert "Warning: Unknown defconfig in target_make" "$target_make" "warn"
    fi
}
🤖 Prompt for AI Agents
In config/boards/orangepi5.conf around lines 88 to 102, the current function
modifies target_make by removing defconfig substrings and sets BOOTCONFIG based
on a string match, which is fragile and error-prone. Refactor the logic to first
check explicitly if target_make contains the SATA defconfig substring, set
BOOTCONFIG accordingly, and then remove the matched defconfig substring from
target_make in a safer, more explicit manner to avoid accidental replacements or
failures.
lib/functions/compilation/uboot.sh (1)

529-530: ⚠️ Potential issue

Remove temporary debugging statements.

These debug lines appear to be temporary debugging aids that should not be committed to the codebase. They add no value and clutter the output.

-	echo "bruhmoment"
-	ls -la .
📝 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.


🤖 Prompt for AI Agents
In lib/functions/compilation/uboot.sh at lines 529 to 530, remove the temporary
debugging statements "echo 'bruhmoment'" and "ls -la ." as they are unnecessary
and clutter the output.

@coderabbitai coderabbitai bot added the Ready to merge Reviewed, tested and ready for merge label May 25, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 45e7554 and 8f0af6f.

📒 Files selected for processing (1)
  • config/boards/orangepi5.conf (6 hunks)
🔇 Additional comments (6)
config/boards/orangepi5.conf (6)

19-20: LGTM! Bluetooth support addition looks good.

The Bluetooth HCI attach parameters and extension enablement are properly configured for the Orange Pi 5's BCM43xx Bluetooth module.


23-27: Good refactoring of branch handling.

The function rename from post_family_config_branch_current__orangepi5_use_mainline_uboot to post_family_config__orangepi5_use_mainline_uboot and the change to skip on "vendor" branch instead of running only on "current" makes the logic clearer and more maintainable.


36-37: LGTM! SATA SPI image support properly added.

The U-Boot target map correctly includes both standard and SATA variants, with appropriate binaries for each configuration.


45-85: Excellent improvement to SPI image flashing logic.

The rewritten function properly handles multiple SPI images with interactive selection and good non-interactive fallback behavior. The array indexing issue from the previous review has been correctly addressed.

The improvements include:

  • Dynamic discovery of SPI images
  • Interactive menu when multiple options exist
  • Automatic selection for single images or non-interactive terminals
  • Proper error handling for missing images
  • Fixed array indexing syntax

127-127: Good consolidation of branch conditions.

The change from skipping only on "edge" to skipping on both "edge" and "current" branches properly consolidates the behavior and aligns with the mainline U-Boot approach used for both branches.

Also applies to: 140-140, 153-153, 168-168, 179-179


33-34:

❓ Verification inconclusive

Verify U-Boot 2025.04 compatibility and availability.

The update from v2025.01 to v2025.04 should be verified to ensure this version exists and is stable for the Orange Pi 5.


🏁 Script executed:

#!/bin/bash
# Description: Verify U-Boot 2025.04 tag exists and check for any known issues
echo "Checking if U-Boot v2025.04 tag exists..."
git ls-remote --tags https://github.com/u-boot/u-boot.git | grep "v2025.04"

echo "Checking recent U-Boot releases..."
git ls-remote --tags https://github.com/u-boot/u-boot.git | grep "v2025" | tail -10

Length of output: 1764


Confirm Up-Boot v2025.04 Tag and Orange Pi 5 Support

  • Verified that the v2025.04 tag exists upstream (git ls-remote --tags … | grep v2025.04).
  • Please ensure your patch directory for v2025.04 is present (e.g. config/patches/v2025.04/).
  • Test building U-Boot v2025.04 for Orange Pi 5 to confirm support and catch any regressions before merging.

Comment on lines +88 to +102
function pre_config_uboot_target__orangepi5_patch_uboot_bootconfig_hack_for_sata() {
if [[ $BRANCH == "vendor" ]]; then
return
fi

display_alert "u-boot for ${BOARD}" "u-boot: hack bootconfig for sata spi image" "info"

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
if [[ $target_make == *"orangepi-5-sata-rk3588s_defconfig"* ]]; then
BOOTCONFIG="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/}
fi
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Verify SATA defconfig file existence and path handling.

The SATA configuration handling logic looks reasonable, but based on the past review comment, there may be an issue with the orangepi-5-sata-rk3588s_defconfig file not existing in the expected location.


🏁 Script executed:

#!/bin/bash
# Description: Check for SATA defconfig files in U-Boot source and package locations
echo "Searching for SATA defconfig files..."
fd -t f "orangepi-5-sata.*defconfig" .

echo "Checking if SATA defconfig exists in common U-Boot defconfig locations..."
fd -t f "*sata*defconfig" . | head -20

echo "Looking for Orange Pi 5 related defconfigs..."
fd -t f "*orangepi*5*defconfig" .

Length of output: 1056


🏁 Script executed:

#!/bin/bash
# Re-run fd with glob patterns to locate defconfig files

echo "Searching for the specific SATA defconfig for Orange Pi 5..."
fd -t f --glob "orangepi-5-sata*defconfig" .

echo "Searching for any defconfig files containing 'sata'..."
fd -t f --glob "*sata*defconfig" .

echo "Searching for all Orange Pi 5 defconfig files..."
fd -t f --glob "*orangepi*5*defconfig" .

Length of output: 489


Missing SATA defconfig: add or correct path

Our search didn’t find any orangepi-5-sata-rk3588s_defconfig (or any *sata*defconfig / *orangepi*5*defconfig) in the repo, so the patch logic will fail at runtime:

  • File: config/boards/orangepi5.conf (lines 88-102)
    References orangepi-5-sata-rk3588s_defconfig but no such file exists.

Please ensure the SATA defconfig is present (e.g. add orangepi-5-sata-rk3588s_defconfig to your U-Boot defconfigs directory) or update the script to use the correct defconfig name/path.

🤖 Prompt for AI Agents
In config/boards/orangepi5.conf around lines 88 to 102, the script references
the defconfig file "orangepi-5-sata-rk3588s_defconfig" which does not exist in
the repository, causing runtime failures. To fix this, either add the missing
"orangepi-5-sata-rk3588s_defconfig" file to the U-Boot defconfigs directory or
update the script to reference the correct existing SATA defconfig file name or
path that matches the repository contents.

Comment on lines +117 to +124
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
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Potential issue with SATA config file handling.

The function assumes .config.sata exists when copying it to the package, but there's no check to ensure the file was actually created. This could cause the build to fail if the SATA configuration step didn't execute properly.

Add a check before copying the SATA config:

 function pre_package_uboot_image__orangepi5_copy_sataconfig_to_pacage() {
 	if [[ $BRANCH == "vendor" ]]; then
 		return
 	fi
 
+	# Only copy SATA config if it exists
+	if [[ ! -f "${uboottempdir}/.config.sata" ]]; then
+		display_alert "u-boot for ${BOARD}" "No SATA config found, skipping copy" "info"
+		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
 }
📝 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_pacage() {
if [[ $BRANCH == "vendor" ]]; then
return
fi
# Only copy SATA config if it exists
if [[ ! -f "${uboottempdir}/.config.sata" ]]; then
display_alert "u-boot for ${BOARD}" "No SATA config found, skipping copy" "info"
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
}
🤖 Prompt for AI Agents
In config/boards/orangepi5.conf around lines 117 to 124, the function
pre_package_uboot_image__orangepi5_copy_sataconfig_to_pacage assumes the file
.config.sata exists without verifying its presence, which may cause build
failures. Add a conditional check to verify that ${uboottempdir}/.config.sata
exists before attempting to copy it. If the file does not exist, skip the copy
and removal steps to prevent errors during the build process.

@github-actions github-actions bot removed the Needs review Seeking for review label May 29, 2025
@EvilOlaf
Copy link
Member

EvilOlaf commented Jun 9, 2025

Status?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
05 Milestone: Second quarter release Framework Framework components Hardware Hardware related like kernel, U-Boot, ... Patches Patches related to kernel, U-Boot, ... Ready to merge Reviewed, tested and ready for merge size/large PR with 250 lines or more Work in progress Unfinished / work in progress
Development

Successfully merging this pull request may close these issues.

6 participants