Skip to content

Conversation

@EvilOlaf
Copy link
Member

Description

  • enhance board feature description
  • use mainline uboot for all branches (suggested here: radxa-zero3 maintenance #8794 (comment))
  • remove unneeded variables
  • shift functions around
  • enable current as test target because
    • it works
    • will be even more interesting with next LTS kernel

How Has This Been Tested?

  • build/boot current image
  • build/boot vendor image
  • install to eMMC to 3W - nope, no hw

Checklist:

  • 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

@github-actions github-actions bot added 11 Milestone: Fourth quarter release size/medium PR with more then 50 and less then 250 lines labels Oct 23, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 23, 2025

Walkthrough

This pull request modifies the Radxa Zero 3 board configuration file to modernize U-Boot handling. The changes update the kernel test target, remove the BOOTFS_TYPE configuration, add a new post_family_config__use_mainline_uboot function that conditionally configures mainline U-Boot parameters (including BOOTCONFIG, BOOTSOURCE, BOOTBRANCH, and UBOOT_TARGET_MAP), and remove legacy U-Boot configuration blocks. The refactor consolidates branch-specific U-Boot handling into a unified approach while maintaining vendor branch compatibility.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

The change involves significant board configuration logic with conditional branching, U-Boot parameter management, and removal of legacy code patterns. Understanding the interaction between mainline and vendor branches, boot configuration parameters, and the implications of unset helper functions requires domain knowledge of the build system and U-Boot handling patterns.

Possibly related PRs

Suggested labels

Needs review, size/large, Hardware, Patches, BSP

Suggested reviewers

  • igorpecovnik
  • amazingfate
  • chainsx
  • pyavitz

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title Check ✅ Passed The title "radxa-zero3: even more board config maint, abandon vendor uboot" is directly related to the main changes in the pull request. It accurately identifies the board being modified (radxa-zero3) and captures the primary technical change: transitioning from vendor U-Boot to mainline U-Boot. The raw summary confirms this is the core modification, with the addition of a post_family_config__use_mainline_uboot function and removal of legacy U-Boot configuration blocks. The title is concise, clear, and specific enough for a developer scanning history to understand the primary intent of the changeset.
Description Check ✅ Passed The pull request description is clearly related to the changeset. It outlines key objectives including using mainline U-Boot for all branches, enabling current as a test target, removing unneeded variables, and enhancing board feature descriptions. These points align with the actual changes documented in the raw summary: the addition of the post_family_config__use_mainline_uboot function, the update to KERNEL_TEST_TARGET, and removal of BOOTFS_TYPE. The description also includes testing details and a self-review checklist, demonstrating substantive engagement with the changes.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions github-actions bot added Needs review Seeking for review Hardware Hardware related like kernel, U-Boot, ... labels Oct 23, 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/radxa-zero3.csc (2)

43-45: Quote paths and guard writes in writer.
Minor robustness: quote args; optionally check file existence to fail fast.

-	function write_uboot_platform() {
-		dd if=$1/u-boot-rockchip.bin of=$2 seek=64 conv=notrunc status=none
-	}
+	function write_uboot_platform() {
+		local src="${1%/}/u-boot-rockchip.bin"
+		local dst="$2"
+		[[ -f "$src" ]] || { display_alert "$BOARD" "Missing $src" "err"; return 1; }
+		dd if="$src" of="$dst" seek=64 conv=notrunc status=none
+	}

Based on learnings.


12-12: Consider setting BOOT_SCENARIO conditionally within post_family_config__use_mainline_uboot() for future flexibility.

The current BOOT_SCENARIO="spl-blobs" at line 12 applies globally to both vendor and mainline builds. While your write_uboot_platform() override (lines 43–45) correctly handles the mainline case with seek=64, the scenario variable remains unchanged. If mainline u-boot requires binman support in the future—as your comment at line 38–39 acknowledges—you would also need to update BOOT_SCENARIO conditionally.

Moving the scenario assignment into the post_family_config__use_mainline_uboot() function alongside the existing overrides would make this dependency explicit and simplify future maintenance:

function post_family_config__use_mainline_uboot() {
	BOOT_SCENARIO="spl-blobs"  # Explicit per-build assignment
	# ... rest of function
}

The equivalent offset math (seek=64 at default 512-byte sectors = bs=32k seek=1) validates correctly across SD/eMMC paths.

📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 4a7b997 and e1563d9.

📒 Files selected for processing (1)
  • config/boards/radxa-zero3.csc (1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-09-14T06:32:29.806Z
Learnt from: amazingfate
PR: armbian/build#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/boards/radxa-zero3.csc
📚 Learning: 2025-10-22T07:51:52.987Z
Learnt from: igorpecovnik
PR: armbian/build#8789
File: config/kernel/linux-sunxi64-edge.config:839-839
Timestamp: 2025-10-22T07:51:52.987Z
Learning: In Armbian's wireless driver configuration, CONFIG_RK_WIFI_DEVICE_* options are Rockchip-specific device registration settings, while CONFIG_WLAN_UWE* are the actual cross-platform driver modules for UWE5622 wireless chips. The UWE5622 chip (manufactured by Unisoc/Spreadtrum) is used on multiple Allwinner boards including Orange Pi Zero 2/2W/3 and Orange Pi 3 LTS, so CONFIG_WLAN_UWE5621/5622 along with Unisoc infrastructure options (CONFIG_SPARD_WLAN_SUPPORT, CONFIG_SC23XX, CONFIG_UNISOC_WIFI_PS, CONFIG_WCN_BSP_DRIVER_BUILDIN) are correct in sunxi64 kernel configs.

Applied to files:

  • config/boards/radxa-zero3.csc
📚 Learning: 2025-09-14T06:29:18.958Z
Learnt from: amazingfate
PR: armbian/build#8619
File: config/sources/families/rockchip.conf:64-70
Timestamp: 2025-09-14T06:29:18.958Z
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/radxa-zero3.csc
🔇 Additional comments (7)
config/boards/radxa-zero3.csc (7)

1-1: Header tweak looks fine.


22-27: FDT handling logic is correct for mainline vs vendor.
Leaving FDT selection to U‑Boot on non‑vendor kernels avoids mismatched DT names.


41-41: Unsetting old writers is sensible.
Prevents legacy writers from shadowing the new one. LGTM.


31-33: Configuration is correctly aligned.

Tag v2025.10 exists in the u-boot repository, and BOOTPATCHDIR="v2025.10" correctly follows the established pattern used throughout the codebase (confirmed by checking nanopct6.conf which uses identical settings). The build system automatically resolves BOOTPATCHDIR to patch/u-boot/v2025.10/, which exists in the tree.


29-29: The original review comment is incorrect—line 29 already uses the correct upstream defconfig name.

The web search confirms the upstream U-Boot defconfig for Radxa Zero 3 RK3566 is radxa-zero-3-rk3566_defconfig (with dash). The Armbian codebase shows:

  • Line 5: BOOTCONFIG="radxa-zero3-rk3566_defconfig" (incorrect—missing dash)
  • Line 29: BOOTCONFIG="radxa-zero-3-rk3566_defconfig" (correct—matches upstream)

The review comment incorrectly suggested changing line 29 to remove the dash. Line 29 should remain unchanged; line 5 is the one that requires correction to match upstream naming.

Likely an incorrect or invalid review comment.


34-34: No changes required; format and variables are correctly configured.

The UBOOT_TARGET_MAP syntax at line 34 follows the established Armbian pattern and is correct. BL31_BLOB and DDR_BLOB variables are inherited from the rk3566 configuration in rockchip64_common.inc (DDR_BLOB="rk35/rk3566_ddr_1056MHz_v1.21.bin", BL31_BLOB="rk35/rk3568_bl31_v1.44.elf"), and RKBIN_DIR is defined as "$SRC/cache/sources/rkbin-tools" in the family config. The ";;" delimiter correctly separates u-boot build arguments from output filenames, as documented in the compilation parser at lib/functions/compilation/uboot.sh which loops over UBOOT_TARGET_MAP targets. The board explicitly inherits BOOT_SCENARIO="spl-blobs" and BOARDFAMILY="rk35xx", which properly chains to the family configuration where all required variables are set.


7-7: Enable current kernel testing for radxa-zero3 board.

The change enables testing of both vendor and current kernel branches during build list generation, following the pattern established by similar boards. The KERNEL_TEST_TARGET setting is an internal build system configuration variable that controls which kernel releases are tested.

The optional suggestion to add CI smoke boot testing is aspirational and not required for this board configuration change.

@igorpecovnik
Copy link
Member

igorpecovnik commented Oct 23, 2025

Mine is without eMMC, can't test that. Should we assume it works? Also perhaps moving to "supported" as it was tested this much?

@EvilOlaf
Copy link
Member Author

I wouldn't either merge or move to standard support unless emmc install/boot is tested.

@igorpecovnik
Copy link
Member

igorpecovnik commented Oct 23, 2025

emmc install/boot is tested.

Finding someone with the hardware + time and efforts to build and test image might be a challenge / can take some time. Asking in Radxa discord?

Image for testing:

https://netcup-01.armbian.com/incoming/igorpecovnik/radxa-zero3/archive/Armbian-unofficial_25.11.0-trunk_Radxa-zero3_noble_current_6.12.54_minimal.img.xz

@igorpecovnik igorpecovnik added the Help needed We need your involvement label Oct 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

11 Milestone: Fourth quarter release Hardware Hardware related like kernel, U-Boot, ... Help needed We need your involvement Needs review Seeking for review size/medium PR with more then 50 and less then 250 lines

Development

Successfully merging this pull request may close these issues.

2 participants