helios64: u-boot: bump to v2026.04 + diagnostics + CMD_FILEENV; v2026.04: generic btrfs zstd fix#9675
Open
iav wants to merge 18 commits intoarmbian:mainfrom
Open
helios64: u-boot: bump to v2026.04 + diagnostics + CMD_FILEENV; v2026.04: generic btrfs zstd fix#9675iav wants to merge 18 commits intoarmbian:mainfrom
iav wants to merge 18 commits intoarmbian:mainfrom
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds support for the Kobol Helios64 RK3399 board to U-Boot v2026.04 by introducing board initialization code, OTP management, device tree definitions, and related patches. The main configuration is updated to reference the new U-Boot version and enable the Changes
Sequence DiagramsequenceDiagram
participant SPL
participant U-Boot
participant GPIO
participant SPI
participant OTP
participant Env
SPL->>GPIO: board_early_init_f()<br/>(Power on Status LED)
U-Boot->>OTP: read_otp_data()<br/>(Initialize SPI, Read OTP)
OTP->>SPI: spi_write_then_read()
SPI-->>OTP: OTP Buffer
OTP->>OTP: Validate CRC32
OTP-->>U-Boot: OTP Data Valid
U-Boot->>GPIO: setup_iodomain()<br/>(Configure IO Voltage)
U-Boot->>GPIO: init_vdd_center()<br/>(Initialize Regulator)
U-Boot->>OTP: mac_read_from_otp()
OTP-->>Env: Set ethaddr/eth1addr
U-Boot->>Env: set_board_info()<br/>(board_rev, serial#)
U-Boot->>GPIO: setup_boottargets()<br/>(Adjust boot sequence)
U-Boot->>GPIO: setup_leds()<br/>(Toggle Status LED)
U-Boot->>GPIO: rockchip_prepare_download_mode()<br/>(Enable download mode GPIOs)
U-Boot->>GPIO: last_stage_init()<br/>(Power on SATA, SCSI scan)
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly Related PRs
Suggested Labels
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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
size/large
iav
force-pushed
the
feat/helios64-uboot-v2026.04
branch
from
788c65c to
feafefa
Compare
iav
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch (1)
131-131: Drop the temporary#warningbefore merge.This will make every Helios64 U-Boot build noisy and can hide real compiler warnings once the MAC debugging work is done.
Suggested cleanup
-#warning "Building image-fdt.c: fdt fixup call chain instrumented for MAC debugging."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch` at line 131, Remove the temporary preprocessor warning line that was added for MAC debugging (the line containing `#warning` "Building image-fdt.c: fdt fixup call chain instrumented for MAC debugging.") from the patch/board code (image-fdt.c / fdt fixup code) before merging so regular builds are not noisy and real compiler warnings aren't obscured; locate the literal `#warning` string in the diff and delete that line (or revert the change) and ensure no other debug-only preprocessor warnings remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@patch/u-boot/v2026.04/board_helios64/board/Kconfig`:
- Around line 21-22: The Kconfig default for ENV_OFFSET (symbol ENV_OFFSET) is
set to 0x460000 which conflicts with the board defconfig's CONFIG_ENV_OFFSET
value (0x3F8000); update the Kconfig default for ENV_OFFSET (the "default ... if
ENV_IS_IN_SPI_FLASH" line) to 0x3F8000 so SPI-flash fallback paths place the
environment at the same address as the helios64 defconfig (ensure you modify the
ENV_OFFSET default and keep the ENV_IS_IN_SPI_FLASH condition intact).
In `@patch/u-boot/v2026.04/board_helios64/board/sys_otp.c`:
- Around line 45-54: is_valid_header currently returns true if any single byte
of otp.magic matches, making the check useless; change it to require the full
magic sequence by checking all bytes match the expected sequence (e.g.,
'H','6','4','N','P','V','1','\0') using logical AND for each comparison or a
single memcmp against the expected string; update the is_valid_header function
to only return 1 when the entire otp.magic equals the expected magic, otherwise
return 0.
- Around line 103-104: The printf currently performs an unaligned 8-byte read by
casting otp.serial_num to uint64_t* (seen where Serial Number is printed and the
second occurrence later), which is undefined and will fault; fix by decoding the
6 bytes explicitly into a 64-bit value (e.g., combine otp.serial_num[0..5] into
a uint64_t with shifts/masks or use an unaligned-read helper) and then
mask/print that 48-bit value instead of dereferencing
*(uint64_t*)otp.serial_num; apply the same change at both places referencing
otp.serial_num to avoid unaligned accesses.
In
`@patch/u-boot/v2026.04/board_helios64/general-fix-btrfs-zstd-decompression.patch`:
- Around line 103-116: zstd_decompress_dctx can return a non-error size smaller
than the expected dlen, so after calling zstd_decompress_dctx (and freeing
workspace) check the returned ret and treat any short decode as an error: if
zstd_is_error(ret) || ret != dlen (or ret < dlen) then free tmp and return -1;
only on exact match ret == dlen proceed to memcpy(tmp -> dbuf), free tmp and
return dlen. Reference zstd_decompress_dctx, ret, dlen, dbuf, tmp, workspace,
out_buf/out_len and cbuf/clen when making the change.
---
Nitpick comments:
In `@patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch`:
- Line 131: Remove the temporary preprocessor warning line that was added for
MAC debugging (the line containing `#warning` "Building image-fdt.c: fdt fixup
call chain instrumented for MAC debugging.") from the patch/board code
(image-fdt.c / fdt fixup code) before merging so regular builds are not noisy
and real compiler warnings aren't obscured; locate the literal `#warning` string
in the diff and delete that line (or revert the change) and ensure no other
debug-only preprocessor warnings remain.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 61483722-d597-4673-908b-e04b1ef3ca58
📒 Files selected for processing (17)
config/boards/helios64.confpatch/u-boot/v2026.04/board_helios64/0000.patching_config.yamlpatch/u-boot/v2026.04/board_helios64/1000-exception-include-string.patchpatch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patchpatch/u-boot/v2026.04/board_helios64/1001-helios64-enable-target.patchpatch/u-boot/v2026.04/board_helios64/board/Kconfigpatch/u-boot/v2026.04/board_helios64/board/MAINTAINERSpatch/u-boot/v2026.04/board_helios64/board/Makefilepatch/u-boot/v2026.04/board_helios64/board/helios64.cpatch/u-boot/v2026.04/board_helios64/board/sys_otp.cpatch/u-boot/v2026.04/board_helios64/board/sys_otp.hpatch/u-boot/v2026.04/board_helios64/cmd-fileenv-read-string-from-file-into-env.patchpatch/u-boot/v2026.04/board_helios64/defconfig/helios64-rk3399_defconfigpatch/u-boot/v2026.04/board_helios64/dt_uboot/rk3399-kobol-helios64-u-boot.dtsipatch/u-boot/v2026.04/board_helios64/dt_upstream_rockchip/rk3399-kobol-helios64.dtspatch/u-boot/v2026.04/board_helios64/general-fix-btrfs-zstd-decompression.patchpatch/u-boot/v2026.04/board_helios64/include_configs/helios64.h
iav
added a commit
to iav/armbian
that referenced
this pull request
Apr 15, 2026
Per CodeRabbit review on PR armbian#9675: board/Kconfig ENV_OFFSET default 0x460000 conflicts with defconfig CONFIG_ENV_OFFSET=0x3F8000. Authoritative source - upstream U-Boot itself: 1. configs/helios64-rk3399_defconfig:7 sets CONFIG_ENV_OFFSET=0x3F8000 - this is what actually ends up in the build; the defconfig overrides the Kconfig default. 2. All 20+ rk3399 boards in upstream configs/*rk3399*_defconfig use 0x3F8000 (evb, firefly, nanopi-m4/r4s, pinebook-pro, rock-4c, khadas-edge, leez, orangepi, ...). 3. The 0x460000 in board/Kconfig is dead code: it is always shadowed by the defconfig, never applied in builds. It is a legacy artefact of the Kobol vendor patch series, not matching any standard SPI layout for rk3399. 4. Physically: on Helios64's 16 MiB W25Q128JV, 0x3F8000 = 4 MiB - 32 KiB reserves the last 32 KiB slot inside the lower 4 MiB window for env - the common rk3399 convention. Aligning the Kconfig default with the defconfig removes the inconsistency and makes the fallback path land at the same address the defconfig expects.
iav
force-pushed
the
feat/helios64-uboot-v2026.04
branch
from
feafefa to
8a8fe50
Compare
iav
added a commit
to iav/armbian
that referenced
this pull request
Apr 15, 2026
Per CodeRabbit review on PR armbian#9675: board/Kconfig ENV_OFFSET default 0x460000 conflicts with defconfig CONFIG_ENV_OFFSET=0x3F8000. Authoritative source - upstream U-Boot itself: 1. configs/helios64-rk3399_defconfig:7 sets CONFIG_ENV_OFFSET=0x3F8000 - this is what actually ends up in the build; the defconfig overrides the Kconfig default. 2. All 20+ rk3399 boards in upstream configs/*rk3399*_defconfig use 0x3F8000 (evb, firefly, nanopi-m4/r4s, pinebook-pro, rock-4c, khadas-edge, leez, orangepi, ...). 3. The 0x460000 in board/Kconfig is dead code: it is always shadowed by the defconfig, never applied in builds. It is a legacy artefact of the Kobol vendor patch series, not matching any standard SPI layout for rk3399. 4. Physically: on Helios64's 16 MiB W25Q128JV, 0x3F8000 = 4 MiB - 32 KiB reserves the last 32 KiB slot inside the lower 4 MiB window for env - the common rk3399 convention. Aligning the Kconfig default with the defconfig removes the inconsistency and makes the fallback path land at the same address the defconfig expects.
iav
added a commit
to iav/armbian
that referenced
this pull request
Apr 15, 2026
Per CodeRabbit review on PR armbian#9675: zstd_decompress_dctx() returns a size_t — the actual number of bytes written, not just an error indicator. The previous code unconditionally returned dlen even if ret < dlen, leaving the tail of dbuf as uninitialised garbage. In practice this path is unreachable for well-formed BTRFS extents: after our fix out_len = max(dlen, fcs), and on success zstd produces exactly fcs bytes, so ret == fcs >= dlen. But the defensive check is trivial and guards against: - a frame header with a falsified content-size that still passes the integrity check; - truncated/corrupted frames that zstd does not always flag as an error. Apply the same fix to the duplicate patch under board_helios64/. This issue was not flagged in PR armbian#9651 (v2026.01) — different review pass surfaced different findings.
iav
force-pushed
the
feat/helios64-uboot-v2026.04
branch
from
8a8fe50 to
f37c9a6
Compare
iav
added a commit
to iav/armbian
that referenced
this pull request
Apr 15, 2026
Per CodeRabbit review on PR armbian#9675: zstd_decompress_dctx() returns a size_t — the actual number of bytes written, not just an error indicator. The previous code unconditionally returned dlen even if ret < dlen, leaving the tail of dbuf as uninitialised garbage. In practice this path is unreachable for well-formed BTRFS extents: after our fix out_len = max(dlen, fcs), and on success zstd produces exactly fcs bytes, so ret == fcs >= dlen. But the defensive check is trivial and guards against: - a frame header with a falsified content-size that still passes the integrity check; - truncated/corrupted frames that zstd does not always flag as an error. Apply the same fix to the duplicate patch under board_helios64/. This issue was not flagged in PR armbian#9651 (v2026.01) — different review pass surfaced different findings.
iav
added a commit
to iav/armbian
that referenced
this pull request
Apr 15, 2026
Per CodeRabbit review on PR armbian#9675: is_valid_header() in board/sys_otp.c uses '||' between byte comparisons, returning 1 if *any* one byte of otp.magic matches the expected sequence. Practically every random byte pattern will trip at least one comparison, so the magic check is effectively meaningless — partially corrupt OTP data is treated as a valid Helios64 header. This is a pre-existing bug in the Kobol vendor code, carried forward unchanged through every U-Boot bump. We are migrating these board files to v2026.04, so the right place to fix it. Replace the chain with a single memcmp() against the expected magic: - removes the '||' vs '&&' confusion entirely; - reads as a normal magic-bytes check; - the constant lives in one place.
iav
force-pushed
the
feat/helios64-uboot-v2026.04
branch
from
f37c9a6 to
6426700
Compare
iav
added a commit
to iav/armbian
that referenced
this pull request
Apr 15, 2026
Per CodeRabbit review on PR armbian#9675: is_valid_header() in board/sys_otp.c uses '||' between byte comparisons, returning 1 if *any* one byte of otp.magic matches the expected sequence. Practically every random byte pattern will trip at least one comparison, so the magic check is effectively meaningless — partially corrupt OTP data is treated as a valid Helios64 header. This is a pre-existing bug in the Kobol vendor code, carried forward unchanged through every U-Boot bump. We are migrating these board files to v2026.04, so the right place to fix it. Replace the chain with a single memcmp() against the expected magic: - removes the '||' vs '&&' confusion entirely; - reads as a normal magic-bytes check; - the constant lives in one place.
iav
added a commit
to iav/armbian
that referenced
this pull request
Apr 15, 2026
…_num Per CodeRabbit review on PR armbian#9675: *((uint64_t *) otp.serial_num) & 0xFFFFFFFFFFFF reads 8 bytes from a 6-byte field inside a __packed__ struct (over-reads into mfg_year), and is unaligned. UB per the C standard, and would fault on stricter alignment requirements. Same pattern affects two call sites the bot pointed at (lines 103-104 and 217-218 of the original vendor file). The bot did not flag it but *(u16 *) otp.mfg_year on the next line has the same problem - packed field, unaligned access - so fix it together. Replace the casts with two small inline helpers that decode the bytes explicitly little-endian (RK3399 is LE, matches the vendor's original intent): otp_serial() - assemble 6 bytes into a u64 otp_mfg_year() - assemble 2 bytes into a u16 This is pre-existing Kobol vendor code carried forward through every U-Boot bump; this PR migrates the board files to v2026.04, so it is the right place to fix it.
iav
force-pushed
the
feat/helios64-uboot-v2026.04
branch
from
6426700 to
023a5c6
Compare
8 tasks
iav
requested review from
ColorfulRhino,
HeyMeco,
NicoD-SBC,
SuperKali,
Tonymac32,
amazingfate,
catalinii and
efectn
as code owners
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch`:
- Around line 33-46: The cached aliases_off breaks the loop's recompute logic:
when do_fixup_by_path() mutates the FDT you must re-resolve the /aliases node
each iteration rather than reuse aliases_off; change the code in the loop that
calls fdt_first_property_offset to call fdt_path_offset(fdt, "/aliases") and
pass that fresh offset into fdt_first_property_offset (or update aliases_off
each iteration after potential mutations) so fdt_first_property_offset receives
a current node offset; update references to aliases_off, fdt_path_offset,
fdt_first_property_offset, and do_fixup_by_path accordingly to keep comment and
behavior consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d9f5fd2a-67ca-4c6b-9ebf-4654cee425dc
📒 Files selected for processing (2)
patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patchpatch/u-boot/v2026.04/board_helios64/board/sys_otp.c
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch (1)
133-133: Drop the compile-time#warningbefore merging.Line 133 will emit on every Helios64 U-Boot build even though the new runtime
log_info()calls already give you the tracing you need. That makes normal builds look unhealthy and adds noise every time this patch is carried forward.Suggested cleanup
-#warning "Building image-fdt.c: fdt fixup call chain instrumented for MAC debugging."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch` at line 133, Remove the temporary compile-time diagnostic line '#warning "Building image-fdt.c: fdt fixup call chain instrumented for MAC debugging."' so it no longer emits on every Helios64 U-Boot build; keep the runtime tracing via log_info() in the fdt fixup call chain (where you added the MAC debugging), and ensure no other stray preprocessor warnings remain in image-fdt.c or the fdt fixup functions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch`:
- Around line 100-104: The new check around status_prop changes behavior by
skipping any node whose status isn't exactly "okay"; revert this so status
remains diagnostic-only and only skip when status is explicitly "disabled":
locate the block using fdt_get_property, status_prop, strcmp and path, and
change the condition to only continue when strcmp((const char
*)status_prop->data, "disabled") == 0, keeping the log_info diagnostic message
and incrementing total_skipped in that disabled-only branch.
---
Nitpick comments:
In `@patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch`:
- Line 133: Remove the temporary compile-time diagnostic line '#warning
"Building image-fdt.c: fdt fixup call chain instrumented for MAC debugging."' so
it no longer emits on every Helios64 U-Boot build; keep the runtime tracing via
log_info() in the fdt fixup call chain (where you added the MAC debugging), and
ensure no other stray preprocessor warnings remain in image-fdt.c or the fdt
fixup functions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 460904ff-4dc9-414a-bcdc-da6cfd20151f
📒 Files selected for processing (2)
patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patchpatch/u-boot/v2026.04/board_helios64/board/sys_otp.c
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch (1)
100-104:⚠️ Potential issue | 🟠 MajorKeep the
statusprobe diagnostic-only.Lines 100-104 add a behavior change on top of the logging: before this patch, only
status = "disabled"was skipped. Now any explicit status other than"okay"is skipped, which can suppress MAC fixups that previously ran.Suggested minimal fix
- const struct fdt_property *status_prop = fdt_get_property(fdt, nodeoff, "status", NULL); - if (status_prop && strcmp((const char *)status_prop->data, "okay")) { - log_info("[fdt_fixup_ethernet] Node '%s' status is '%s', skipping\n", path, (const char *)status_prop->data); - total_skipped++; - continue; - } + const struct fdt_property *status_prop = fdt_get_property(fdt, nodeoff, "status", NULL); + if (status_prop) + log_info("[fdt_fixup_ethernet] Node '%s' status is '%s'\n", + path, (const char *)status_prop->data);Run this read-only check to compare status handling across copies of the patch:
#!/bin/bash set -euo pipefail fd -p '1001-fdt_fixup_ethernet-add-logs.patch' patch/u-boot -x sh -c ' echo "== {} =="; rg -n -C3 "fdt_get_property\\(fdt, nodeoff, \"status\"|strcmp\\(fdt_prop->data, \"disabled\"|strcmp\\(\\(const char \\*\\)status_prop->data, \"okay\"" "{}"; echo 'Expected result: the diagnostic patch should retain the existing
"disabled"skip only. If this copy is the one introducing a separate non-"okay"skip, that confirms the semantic regression.Based on learnings: 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.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch` around lines 100 - 104, The new logic treats any explicit status != "okay" as a skip; restore diagnostic-only behavior by changing the check to skip only when status == "disabled". In the block using fdt_get_property/fdt_property (status_prop), call strcmp((const char *)status_prop->data, "disabled") and skip/continue only when that returns 0, but still call log_info(path, status) for diagnostics; update references to status_prop, log_info, total_skipped and continue accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch`:
- Around line 100-104: The new logic treats any explicit status != "okay" as a
skip; restore diagnostic-only behavior by changing the check to skip only when
status == "disabled". In the block using fdt_get_property/fdt_property
(status_prop), call strcmp((const char *)status_prop->data, "disabled") and
skip/continue only when that returns 0, but still call log_info(path, status)
for diagnostics; update references to status_prop, log_info, total_skipped and
continue accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f389b421-3def-40b3-bb64-7620216023d6
📒 Files selected for processing (2)
patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patchpatch/u-boot/v2026.04/board_helios64/board/sys_otp.c
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch`:
- Line 133: Remove the hard-coded preprocessor warning that was added ("#warning
\"Building image-fdt.c: fdt fixup call chain instrumented for MAC debugging.\"")
so it no longer emits at compile time; rely on the existing runtime log_info()
tracing in the fdt fixup call chain instead, leaving functions and
instrumentation (the fdt fixup call chain and its log_info() calls) unchanged.
In `@patch/u-boot/v2026.04/board_helios64/board/sys_otp.c`:
- Around line 230-243: get_variant() currently reads otp directly; change it to
use the same OTP-ready path as the other helpers by invoking the OTP
initialization/ready routine used by set_board_info() and mac_read_from_otp()
(e.g., call set_board_info() or the common otp cache loader) before accessing
otp; if that initialization fails, return "Unknown", otherwise proceed to
validate otp.variant against BOARD_VARIANT_ENG_SAMPLE and BOARD_VARIANT_MAX and
return the matching string from var_str as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5f49544b-95f7-4fe0-88fe-ad4de2e23ef8
📒 Files selected for processing (2)
patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patchpatch/u-boot/v2026.04/board_helios64/board/sys_otp.c
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch (1)
100-104:⚠️ Potential issue | 🟠 MajorKeep the new
statusprobe diagnostic-only.This still turns an “add logs” patch into a behavior change: nodes are now skipped unless
statusis literally"okay". The original flow only special-cased"disabled", so this can suppress MAC fixups for otherwise-enabled aliases.Minimal fix
- const struct fdt_property *status_prop = fdt_get_property(fdt, nodeoff, "status", NULL); - if (status_prop && strcmp((const char *)status_prop->data, "okay")) { - log_info("[fdt_fixup_ethernet] Node '%s' status is '%s', skipping\n", path, (const char *)status_prop->data); - total_skipped++; - continue; - } + const struct fdt_property *status_prop = fdt_get_property(fdt, nodeoff, "status", NULL); + if (status_prop) + log_info("[fdt_fixup_ethernet] Node '%s' status is '%s'\n", + path, (const char *)status_prop->data);Based on learnings: For the Armbian build project, maintaining consistency with existing patches across U-Boot versions is prioritized over refactoring individual patches for code improvements.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch` around lines 100 - 104, The new status probe must be diagnostic-only: don't change the original logic that only skips nodes when status == "disabled". Modify the block around fdt_get_property/fdt_fixup_ethernet so that you read status_prop and log its value for diagnostics (using log_info with path and status_prop->data) but only increment total_skipped and continue when strcmp(status, "disabled") == 0; leave all other status values (including absent or "okay") to proceed with the existing MAC fixup flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@patch/u-boot/v2026.04/board_helios64/board/sys_otp.c`:
- Around line 219-222: get_revision currently calls is_data_valid() directly and
bypasses ensure_otp_data_ready(), so transient OTP readiness failures aren't
retried; modify get_revision to call ensure_otp_data_ready() first (as other
helpers do), return error if it fails, then proceed to validate/read revision
(replace or precede the is_data_valid() check with ensure_otp_data_ready() and
keep existing error handling to match
get_variant()/set_board_info()/mac_read_from_otp).
---
Duplicate comments:
In `@patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch`:
- Around line 100-104: The new status probe must be diagnostic-only: don't
change the original logic that only skips nodes when status == "disabled".
Modify the block around fdt_get_property/fdt_fixup_ethernet so that you read
status_prop and log its value for diagnostics (using log_info with path and
status_prop->data) but only increment total_skipped and continue when
strcmp(status, "disabled") == 0; leave all other status values (including absent
or "okay") to proceed with the existing MAC fixup flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f53c32cb-ee4d-4f50-a477-8e30d5911d0e
📒 Files selected for processing (2)
patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patchpatch/u-boot/v2026.04/board_helios64/board/sys_otp.c
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
patch/u-boot/v2026.04/board_helios64/board/sys_otp.c (1)
219-222:⚠️ Potential issue | 🟡 MinorMake
get_revision()use the same OTP-ready path as the other accessors.Line 221 still bypasses
ensure_otp_data_ready(), so a transient early SPI failure can be retried byget_variant(),set_board_info(), andmac_read_from_otp(), but not here.🔧 Minimal fix
int get_revision(int *major, int *minor) { - if (!is_data_valid()) + if (ensure_otp_data_ready()) return -1; *major = (otp.revision & 0xf0) >> 4;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/u-boot/v2026.04/board_helios64/board/sys_otp.c` around lines 219 - 222, get_revision() currently bypasses the OTP-ready path by directly calling is_data_valid(); change it to mirror other accessors by calling ensure_otp_data_ready() first (and handling its error), then call is_data_valid() and return -1 if still invalid. Update get_revision to invoke ensure_otp_data_ready() before using is_data_valid() so transient SPI/OTP init failures are retried (refer to function names get_revision, ensure_otp_data_ready, is_data_valid, and callers like get_variant/set_board_info/mac_read_from_otp to match behavior).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@patch/u-boot/v2026.04/board_helios64/board/sys_otp.c`:
- Around line 219-222: get_revision() currently bypasses the OTP-ready path by
directly calling is_data_valid(); change it to mirror other accessors by calling
ensure_otp_data_ready() first (and handling its error), then call
is_data_valid() and return -1 if still invalid. Update get_revision to invoke
ensure_otp_data_ready() before using is_data_valid() so transient SPI/OTP init
failures are retried (refer to function names get_revision,
ensure_otp_data_ready, is_data_valid, and callers like
get_variant/set_board_info/mac_read_from_otp to match behavior).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 67eeae8d-850a-4eb7-a875-39c1a5db5247
📒 Files selected for processing (2)
patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patchpatch/u-boot/v2026.04/board_helios64/board/sys_otp.c
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch (3)
133-133:⚠️ Potential issue | 🟡 MinorDrop the compile-time
#warning.The runtime
log_info()tracing already makes this patch visible. Keeping#warningin a carried patch will spam every build and can become fatal under-Werrortoolchains.Minimal fix
-#warning "Building image-fdt.c: fdt fixup call chain instrumented for MAC debugging."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch` at line 133, Remove the compile-time directive that emits a build warning: delete the added line '#warning "Building image-fdt.c: fdt fixup call chain instrumented for MAC debugging."' from the patch so it no longer spams builds; rely on the existing runtime tracing via log_info() in image-fdt.c (the fdt fixup call chain instrumentation) for visibility instead.
100-104:⚠️ Potential issue | 🟠 MajorKeep the new
statusprobe diagnostic-only.This turns a logging patch into a behavior change. The original flow only skipped explicitly
"disabled"nodes; this now skips any present status other than exact"okay", which can suppress valid aliases that still mean enabled.Minimal fix
- const struct fdt_property *status_prop = fdt_get_property(fdt, nodeoff, "status", NULL); - if (status_prop && strcmp((const char *)status_prop->data, "okay")) { - log_info("[fdt_fixup_ethernet] Node '%s' status is '%s', skipping\n", path, (const char *)status_prop->data); - total_skipped++; - continue; - } + const struct fdt_property *status_prop = fdt_get_property(fdt, nodeoff, "status", NULL); + if (status_prop) + log_info("[fdt_fixup_ethernet] Node '%s' status is '%s'\n", + path, (const char *)status_prop->data);Based on learnings: 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.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch` around lines 100 - 104, The new check using fdt_get_property/status_prop and strcmp changes behavior by skipping any status not equal to "okay" instead of only skipping "disabled"; revert it to be diagnostic-only: keep retrieving status_prop via fdt_get_property and log its value with log_info (referencing status_prop, fdt_get_property, log_info, path, nodeoff), but do not alter control flow—remove or avoid using strcmp to continue/total_skipped so the code only logs the status and preserves the original skipping logic (which should only skip when status equals "disabled").
33-46:⚠️ Potential issue | 🟠 MajorRecompute
/aliasesinside the loop.
do_fixup_by_path()mutates the FDT during iteration, so cachingaliases_offonce can leave laterfdt_first_property_offset()calls walking a stale node offset. It also contradicts the comment on Line 43.Minimal fix
- int aliases_off = fdt_path_offset(fdt, "/aliases"); - if (aliases_off < 0) { + if (fdt_path_offset(fdt, "/aliases") < 0) { log_info("[fdt_fixup_ethernet] /aliases node not found\n"); return; } @@ - offset = fdt_first_property_offset(fdt, aliases_off); + offset = fdt_first_property_offset(fdt, + fdt_path_offset(fdt, "/aliases"));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch` around lines 33 - 46, The cached aliases_off can become stale because do_fixup_by_path() may mutate the FDT while iterating; change the loop so you recompute the /aliases offset each iteration instead of reusing aliases_off (i.e., call fdt_path_offset(fdt, "/aliases") when computing offset inside the for loop), keep the early-not-found check but avoid returning a stale offset, and ensure this logic around aliases_off and offset (used with fdt_first_property_offset) is updated accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch`:
- Line 133: Remove the compile-time directive that emits a build warning: delete
the added line '#warning "Building image-fdt.c: fdt fixup call chain
instrumented for MAC debugging."' from the patch so it no longer spams builds;
rely on the existing runtime tracing via log_info() in image-fdt.c (the fdt
fixup call chain instrumentation) for visibility instead.
- Around line 100-104: The new check using fdt_get_property/status_prop and
strcmp changes behavior by skipping any status not equal to "okay" instead of
only skipping "disabled"; revert it to be diagnostic-only: keep retrieving
status_prop via fdt_get_property and log its value with log_info (referencing
status_prop, fdt_get_property, log_info, path, nodeoff), but do not alter
control flow—remove or avoid using strcmp to continue/total_skipped so the code
only logs the status and preserves the original skipping logic (which should
only skip when status equals "disabled").
- Around line 33-46: The cached aliases_off can become stale because
do_fixup_by_path() may mutate the FDT while iterating; change the loop so you
recompute the /aliases offset each iteration instead of reusing aliases_off
(i.e., call fdt_path_offset(fdt, "/aliases") when computing offset inside the
for loop), keep the early-not-found check but avoid returning a stale offset,
and ensure this logic around aliases_off and offset (used with
fdt_first_property_offset) is updated accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 41f68186-a5b4-440f-8cd8-0b3870cf5517
📒 Files selected for processing (2)
patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patchpatch/u-boot/v2026.04/board_helios64/board/sys_otp.c
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch (3)
100-104:⚠️ Potential issue | 🟠 MajorKeep
statushandling diagnostic-only; skip only explicit"disabled".The current condition changes behavior by rejecting any status not exactly
"okay".Suggested minimal fix
- const struct fdt_property *status_prop = fdt_get_property(fdt, nodeoff, "status", NULL); - if (status_prop && strcmp((const char *)status_prop->data, "okay")) { - log_info("[fdt_fixup_ethernet] Node '%s' status is '%s', skipping\n", path, (const char *)status_prop->data); - total_skipped++; - continue; - } + const struct fdt_property *status_prop = fdt_get_property(fdt, nodeoff, "status", NULL); + if (status_prop) + log_info("[fdt_fixup_ethernet] Node '%s' status is '%s'\n", + path, (const char *)status_prop->data); + if (status_prop && !strcmp((const char *)status_prop->data, "disabled")) { + total_skipped++; + continue; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch` around lines 100 - 104, The code in fdt_fixup_ethernet uses status_prop and rejects any status not exactly "okay", but the review asks to keep status diagnostic-only and only skip when status is explicitly "disabled"; change the condition around fdt_get_property/status_prop so that you log the status (using log_info with path and status_prop->data) but only increment total_skipped and continue when strcmp(status, "disabled") == 0; leave nodes with absent status or other values to be processed normally. Ensure you reference the same symbols: status_prop, fdt_get_property(fdt, nodeoff, "status", NULL), log_info(..., path, status), total_skipped, and the surrounding fdt_fixup_ethernet logic.
33-47:⚠️ Potential issue | 🟠 MajorRecompute
/aliasesoffset inside the loop (cached offset can go stale).This still contradicts the “recompute” intent and can miss later aliases after DT edits in-loop.
Suggested minimal fix
- int aliases_off = fdt_path_offset(fdt, "/aliases"); - if (aliases_off < 0) { + if (fdt_path_offset(fdt, "/aliases") < 0) { log_info("[fdt_fixup_ethernet] /aliases node not found\n"); return; } @@ - /* FDT might have been edited, recompute the offset */ - offset = fdt_first_property_offset(fdt, aliases_off); + /* FDT might have been edited, recompute the offset */ + offset = fdt_first_property_offset(fdt, + fdt_path_offset(fdt, "/aliases"));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch` around lines 33 - 47, The loop currently uses a cached aliases_off and so can miss aliases if the device tree is modified during iteration; instead, recompute the /aliases node offset each iteration by calling fdt_path_offset(fdt, "/aliases") right before fdt_first_property_offset, and handle a negative result (e.g., break or continue) appropriately; update references to aliases_off and the call to fdt_first_property_offset(fdt, aliases_off) inside the for-loop in the fdt_fixup_ethernet code so the offset is fresh on each pass.
133-133:⚠️ Potential issue | 🟡 MinorDrop the hard-coded compile-time
#warning.This is still noisy in carried patches and can fail warning-as-error toolchains.
Suggested minimal fix
-#warning "Building image-fdt.c: fdt fixup call chain instrumented for MAC debugging."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch` at line 133, Remove the hard-coded compile-time '#warning "Building image-fdt.c: fdt fixup call chain instrumented for MAC debugging."' (or wrap it behind a dedicated debug macro) so it doesn't emit noisy warnings or break warning-as-error builds; locate the added '#warning' line in the image-fdt.c patch and either delete it or enclose it in a conditional such as '#ifdef ENABLE_FDT_MAC_DEBUG' / '#endif' so the warning is only produced when explicitly enabled.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch`:
- Around line 100-104: The code in fdt_fixup_ethernet uses status_prop and
rejects any status not exactly "okay", but the review asks to keep status
diagnostic-only and only skip when status is explicitly "disabled"; change the
condition around fdt_get_property/status_prop so that you log the status (using
log_info with path and status_prop->data) but only increment total_skipped and
continue when strcmp(status, "disabled") == 0; leave nodes with absent status or
other values to be processed normally. Ensure you reference the same symbols:
status_prop, fdt_get_property(fdt, nodeoff, "status", NULL), log_info(..., path,
status), total_skipped, and the surrounding fdt_fixup_ethernet logic.
- Around line 33-47: The loop currently uses a cached aliases_off and so can
miss aliases if the device tree is modified during iteration; instead, recompute
the /aliases node offset each iteration by calling fdt_path_offset(fdt,
"/aliases") right before fdt_first_property_offset, and handle a negative result
(e.g., break or continue) appropriately; update references to aliases_off and
the call to fdt_first_property_offset(fdt, aliases_off) inside the for-loop in
the fdt_fixup_ethernet code so the offset is fresh on each pass.
- Line 133: Remove the hard-coded compile-time '#warning "Building image-fdt.c:
fdt fixup call chain instrumented for MAC debugging."' (or wrap it behind a
dedicated debug macro) so it doesn't emit noisy warnings or break
warning-as-error builds; locate the added '#warning' line in the image-fdt.c
patch and either delete it or enclose it in a conditional such as '#ifdef
ENABLE_FDT_MAC_DEBUG' / '#endif' so the warning is only produced when explicitly
enabled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 01d590bc-fc12-47be-8977-a700663f9804
📒 Files selected for processing (2)
patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patchpatch/u-boot/v2026.04/board_helios64/board/sys_otp.c
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch (3)
133-133:⚠️ Potential issue | 🟡 MinorDrop the hard-coded
#warning.The runtime
log_info()tracing already gives you the diagnostics. The compile-time warning fires on every build and can become fatal with-Werror.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch` at line 133, Remove the hard-coded preprocessor warning that was added: delete the line containing `#warning` "Building image-fdt.c: fdt fixup call chain instrumented for MAC debugging." (the compile-time diagnostic is redundant because runtime log_info() provides the tracing and the `#warning` can cause build failures with -Werror); rely on existing log_info() instrumentation instead of emitting a compile-time `#warning`.
100-104:⚠️ Potential issue | 🟠 MajorKeep the
statusprobe diagnostic-only.This now skips every node whose
statusis not literally"okay". The original flow only skipped explicit"disabled"nodes, so this turns a logging patch into a behavior change.Suggested minimal fix
const struct fdt_property *status_prop = fdt_get_property(fdt, nodeoff, "status", NULL); - if (status_prop && strcmp((const char *)status_prop->data, "okay")) { - log_info("[fdt_fixup_ethernet] Node '%s' status is '%s', skipping\n", path, (const char *)status_prop->data); + if (status_prop) + log_info("[fdt_fixup_ethernet] Node '%s' status is '%s'\n", + path, (const char *)status_prop->data); + if (status_prop && !strcmp((const char *)status_prop->data, "disabled")) { + log_info("[fdt_fixup_ethernet] Node '%s' is disabled, skipping\n", path); total_skipped++; continue; }Based on learnings: 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.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch` around lines 100 - 104, The current code treats any non-"okay" status as a reason to skip nodes; change the probe to be diagnostic-only and only skip when status is explicitly "disabled": fetch the "status" with fdt_get_property into status_prop and if status_prop && strcmp((const char *)status_prop->data, "disabled") == 0 then log and increment total_skipped and continue; otherwise, if status_prop exists and is not "disabled", just log the status with log_info but do not continue. Reference symbols: fdt_get_property, status_prop, strcmp, log_info, total_skipped, continue.
32-46:⚠️ Potential issue | 🟠 MajorRecompute
/aliasesinside the loop.This still leaves
aliases_offstale afterdo_fixup_by_path()mutates the blob, which can make later alias iteration unreliable.Suggested minimal fix
- int aliases_off = fdt_path_offset(fdt, "/aliases"); - if (aliases_off < 0) { + if (fdt_path_offset(fdt, "/aliases") < 0) { log_info("[fdt_fixup_ethernet] /aliases node not found\n"); return; } @@ - offset = fdt_first_property_offset(fdt, aliases_off); + offset = fdt_first_property_offset(fdt, + fdt_path_offset(fdt, "/aliases"));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch` around lines 32 - 46, The current code caches aliases_off once before iterating aliases, but do_fixup_by_path() can modify the device tree and make aliases_off stale; inside the for loop before calling fdt_first_property_offset and any use of aliases_off, recompute aliases_off by calling fdt_path_offset(fdt, "/aliases") and handle a negative return (log and break/return) so each iteration uses a fresh /aliases offset; update references in the loop that use aliases_off (and any subsequent calls like fdt_first_property_offset) to use the recomputed value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch`:
- Line 133: Remove the hard-coded preprocessor warning that was added: delete
the line containing `#warning` "Building image-fdt.c: fdt fixup call chain
instrumented for MAC debugging." (the compile-time diagnostic is redundant
because runtime log_info() provides the tracing and the `#warning` can cause build
failures with -Werror); rely on existing log_info() instrumentation instead of
emitting a compile-time `#warning`.
- Around line 100-104: The current code treats any non-"okay" status as a reason
to skip nodes; change the probe to be diagnostic-only and only skip when status
is explicitly "disabled": fetch the "status" with fdt_get_property into
status_prop and if status_prop && strcmp((const char *)status_prop->data,
"disabled") == 0 then log and increment total_skipped and continue; otherwise,
if status_prop exists and is not "disabled", just log the status with log_info
but do not continue. Reference symbols: fdt_get_property, status_prop, strcmp,
log_info, total_skipped, continue.
- Around line 32-46: The current code caches aliases_off once before iterating
aliases, but do_fixup_by_path() can modify the device tree and make aliases_off
stale; inside the for loop before calling fdt_first_property_offset and any use
of aliases_off, recompute aliases_off by calling fdt_path_offset(fdt,
"/aliases") and handle a negative return (log and break/return) so each
iteration uses a fresh /aliases offset; update references in the loop that use
aliases_off (and any subsequent calls like fdt_first_property_offset) to use the
recomputed value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 95c2e09e-d1df-4efd-ad23-2be938e8c83b
📒 Files selected for processing (2)
patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patchpatch/u-boot/v2026.04/board_helios64/board/sys_otp.c
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch (3)
133-133:⚠️ Potential issue | 🟡 MinorRemove the compile-time
#warningfrom carried patch code.Line 133 emits on every build and can become fatal under warning-as-error policies. Runtime
log_info()already provides sufficient diagnostics.Suggested fix
-#warning "Building image-fdt.c: fdt fixup call chain instrumented for MAC debugging."#!/bin/bash set -euo pipefail FILE="patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch" rg -n '#warning "Building image-fdt.c: fdt fixup call chain instrumented for MAC debugging\."' "$FILE"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch` at line 133, Remove the compile-time directive '#warning "Building image-fdt.c: fdt fixup call chain instrumented for MAC debugging."' from the patch so builds won't emit this warning (which can fail under -Werror); keep the existing runtime diagnostic via log_info() in the fdt fixup/ethernet instrumentation code (search for the '#warning' token or the instrumented image-fdt.c fdt fixup call chain message to locate the exact patch hunk and delete that line).
100-104:⚠️ Potential issue | 🟠 MajorThe
statusgate changes behavior beyond diagnostics.Line 101 now skips any node whose
statusis not exactly"okay". That is stricter than the earlier disabled-only behavior and can suppress valid MAC fixups.Suggested minimal fix
- const struct fdt_property *status_prop = fdt_get_property(fdt, nodeoff, "status", NULL); - if (status_prop && strcmp((const char *)status_prop->data, "okay")) { - log_info("[fdt_fixup_ethernet] Node '%s' status is '%s', skipping\n", path, (const char *)status_prop->data); - total_skipped++; - continue; - } + const struct fdt_property *status_prop = fdt_get_property(fdt, nodeoff, "status", NULL); + if (status_prop && !strcmp((const char *)status_prop->data, "disabled")) { + log_info("[fdt_fixup_ethernet] Node '%s' is disabled, skipping\n", path); + total_skipped++; + continue; + }#!/bin/bash set -euo pipefail FILE="patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch" rg -n -C2 'status_prop|status.*okay|status.*disabled' "$FILE"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch` around lines 100 - 104, The change makes fdt_fixup_ethernet skip any node whose status is not exactly "okay", which is too strict and will skip valid nodes; update the check in the fdt_fixup_ethernet logic so it only skips when the "status" property explicitly equals "disabled" (use strcmp(...) == 0), keep using status_prop and fdt_get_property to read the prop, and leave the log_info, total_skipped, path and nodeoff handling as-is so only disabled nodes are skipped instead of every non-"okay" node.
33-46:⚠️ Potential issue | 🟠 MajorRecompute
/aliasesoffset per iteration (current code contradicts Line 43 comment).Line 43 says to recompute after potential FDT edits, but Line 46 uses cached
aliases_off. Since fixups can mutate the blob, this can desync traversal and skip later aliases.Suggested minimal fix
- int aliases_off = fdt_path_offset(fdt, "/aliases"); - if (aliases_off < 0) { + if (fdt_path_offset(fdt, "/aliases") < 0) { log_info("[fdt_fixup_ethernet] /aliases node not found\n"); return; } @@ - offset = fdt_first_property_offset(fdt, aliases_off); + offset = fdt_first_property_offset(fdt, + fdt_path_offset(fdt, "/aliases"));#!/bin/bash set -euo pipefail FILE="patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch" rg -n -C2 'FDT might have been edited|aliases_off|fdt_first_property_offset\(fdt' "$FILE"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch` around lines 33 - 46, The loop comment says the /aliases offset must be recomputed because the FDT may be edited, but the code uses the cached aliases_off; change the loop to recompute the aliases node offset each iteration by calling fdt_path_offset(fdt, "/aliases") and pass that value into fdt_first_property_offset(fdt, ...); handle a negative return (e.g., break out of the loop) if the recomputed offset becomes invalid, and keep the initial not-found check for the first iteration as-is; update the surrounding comment if needed to reflect per-iteration recomputation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch`:
- Line 133: Remove the compile-time directive '#warning "Building image-fdt.c:
fdt fixup call chain instrumented for MAC debugging."' from the patch so builds
won't emit this warning (which can fail under -Werror); keep the existing
runtime diagnostic via log_info() in the fdt fixup/ethernet instrumentation code
(search for the '#warning' token or the instrumented image-fdt.c fdt fixup call
chain message to locate the exact patch hunk and delete that line).
- Around line 100-104: The change makes fdt_fixup_ethernet skip any node whose
status is not exactly "okay", which is too strict and will skip valid nodes;
update the check in the fdt_fixup_ethernet logic so it only skips when the
"status" property explicitly equals "disabled" (use strcmp(...) == 0), keep
using status_prop and fdt_get_property to read the prop, and leave the log_info,
total_skipped, path and nodeoff handling as-is so only disabled nodes are
skipped instead of every non-"okay" node.
- Around line 33-46: The loop comment says the /aliases offset must be
recomputed because the FDT may be edited, but the code uses the cached
aliases_off; change the loop to recompute the aliases node offset each iteration
by calling fdt_path_offset(fdt, "/aliases") and pass that value into
fdt_first_property_offset(fdt, ...); handle a negative return (e.g., break out
of the loop) if the recomputed offset becomes invalid, and keep the initial
not-found check for the first iteration as-is; update the surrounding comment if
needed to reflect per-iteration recomputation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9f633e4a-1e9f-4145-8942-51b7d4a29a46
📒 Files selected for processing (2)
patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patchpatch/u-boot/v2026.04/board_helios64/board/sys_otp.c
Pull two generic v2026.04 patches that helios64 currently misses because BOOTPATCHDIR points at the self-contained board_helios64 subdirectory and does not inherit from patch/u-boot/v2026.04/. - 1001-fdt_fixup_ethernet-add-logs.patch: diagnostic log_info() calls in boot/fdt_support.c and boot/image-fdt.c. helios64 hits this path via its gmac ethernet fixups. - cmd-fileenv-read-string-from-file-into-env.patch: adds the 'fileenv' command. Enable CONFIG_CMD_FILEENV in helios64.conf alongside, matching the convention of odroidm1 / khadas-vim3 / odroidm1s which bring in the patch and enable the option in the same commit. The command is purely additive — it does not affect armbianEnv.txt or the existing env mechanics, only adds a new CLI command to read a file into env. Other parent-level patches (rk3288 VOP, rk3588 I2S MCLK, DW-HDMI disable) are SoC-specific or require config options helios64 does not enable, so they are not pulled in.
Same fix as armbian#9651 for v2026.01 and v2025.10, applied to the common patch/u-boot/v2026.04/ directory so all boards on U-Boot v2026.04 pick it up (cm3588-nas, nanopct6, orangepi5, rock-3a, nanopi-r3s-lts, mekotronics-*, mixtile-blade3, radxa-rock-4d, helios64 via its own self-contained board_helios64 subdir already has a copy). Without this, booting from a BTRFS rootfs with zstd-compressed extents fails with: zstd_decompress: failed to decompress: 70 BTRFS: An error occurred while reading file /boot/boot.scr See commit 5617ff3 for the full rationale.
Per CodeRabbit review on PR armbian#9675: board/Kconfig ENV_OFFSET default 0x460000 conflicts with defconfig CONFIG_ENV_OFFSET=0x3F8000. Authoritative source - upstream U-Boot itself: 1. configs/helios64-rk3399_defconfig:7 sets CONFIG_ENV_OFFSET=0x3F8000 - this is what actually ends up in the build; the defconfig overrides the Kconfig default. 2. All 20+ rk3399 boards in upstream configs/*rk3399*_defconfig use 0x3F8000 (evb, firefly, nanopi-m4/r4s, pinebook-pro, rock-4c, khadas-edge, leez, orangepi, ...). 3. The 0x460000 in board/Kconfig is dead code: it is always shadowed by the defconfig, never applied in builds. It is a legacy artefact of the Kobol vendor patch series, not matching any standard SPI layout for rk3399. 4. Physically: on Helios64's 16 MiB W25Q128JV, 0x3F8000 = 4 MiB - 32 KiB reserves the last 32 KiB slot inside the lower 4 MiB window for env - the common rk3399 convention. Aligning the Kconfig default with the defconfig removes the inconsistency and makes the fallback path land at the same address the defconfig expects.
Per CodeRabbit review on PR armbian#9675: zstd_decompress_dctx() returns a size_t — the actual number of bytes written, not just an error indicator. The previous code unconditionally returned dlen even if ret < dlen, leaving the tail of dbuf as uninitialised garbage. In practice this path is unreachable for well-formed BTRFS extents: after our fix out_len = max(dlen, fcs), and on success zstd produces exactly fcs bytes, so ret == fcs >= dlen. But the defensive check is trivial and guards against: - a frame header with a falsified content-size that still passes the integrity check; - truncated/corrupted frames that zstd does not always flag as an error. Apply the same fix to the duplicate patch under board_helios64/. This issue was not flagged in PR armbian#9651 (v2026.01) — different review pass surfaced different findings.
Per CodeRabbit review on PR armbian#9675: is_valid_header() in board/sys_otp.c uses '||' between byte comparisons, returning 1 if *any* one byte of otp.magic matches the expected sequence. Practically every random byte pattern will trip at least one comparison, so the magic check is effectively meaningless — partially corrupt OTP data is treated as a valid Helios64 header. This is a pre-existing bug in the Kobol vendor code, carried forward unchanged through every U-Boot bump. We are migrating these board files to v2026.04, so the right place to fix it. Replace the chain with a single memcmp() against the expected magic: - removes the '||' vs '&&' confusion entirely; - reads as a normal magic-bytes check; - the constant lives in one place.
…_num Per CodeRabbit review on PR armbian#9675: *((uint64_t *) otp.serial_num) & 0xFFFFFFFFFFFF reads 8 bytes from a 6-byte field inside a __packed__ struct (over-reads into mfg_year), and is unaligned. UB per the C standard, and would fault on stricter alignment requirements. Same pattern affects two call sites the bot pointed at (lines 103-104 and 217-218 of the original vendor file). The bot did not flag it but *(u16 *) otp.mfg_year on the next line has the same problem - packed field, unaligned access - so fix it together. Replace the casts with two small inline helpers that decode the bytes explicitly little-endian (RK3399 is LE, matches the vendor's original intent): otp_serial() - assemble 6 bytes into a u64 otp_mfg_year() - assemble 2 bytes into a u16 This is pre-existing Kobol vendor code carried forward through every U-Boot bump; this PR migrates the board files to v2026.04, so it is the right place to fix it.
When FDT_SEQ_MACADDR_FROM_ENV is defined, nodeoff is already declared and assigned earlier in the function. The later 'int nodeoff = ...' shadowed that variable and performed a redundant fdt_path_offset() lookup. Guard the inner declaration with #ifndef so it only exists when the outer one does not. No behavior change — the same nodeoff value is checked either way. Addresses a CodeRabbit review comment on PR armbian#9675.
show_otp_data() directly indexed var_str[otp.variant] without validation.
If the OTP blob is corrupted (e.g. variant byte out of 0..BOARD_VARIANT_MAX-1
range), this read is out of bounds.
Mirror the check already used in get_variant(): fall back to var_str[0]
("Invalid variant") when otp.variant is out of range.
This is debug-only (wrapped in #ifdef DEBUG) and never reached in
production builds, but keeps behavior consistent with get_variant() and
avoids a latent OOB read if DEBUG is enabled.
Addresses a CodeRabbit review comment on PR armbian#9675.
spi_write_then_read() returned value was only used to set has_been_read, then header/CRC checks ran against whatever ended up in the otp struct. On a real SPI I/O failure, the struct could contain partial garbage, header/CRC would fail, and the caller got a generic -1 — losing the original errno and conflating I/O errors with invalid OTP data. Handle the I/O error path explicitly: - On ret != 0: log, keep has_been_read = 0 (so the next call can retry), and return the original ret. - On ret == 0: set has_been_read = 1 and proceed to header/CRC validation as before. Addresses a CodeRabbit review comment on PR armbian#9675.
The previous commit preserved has_been_read = 0 on SPI I/O failure so a later call could retry. But in the actual boot flow no retry happens: board_early_init_r() ignores the return of read_otp_data(), and the later consumers (set_board_info, mac_read_from_otp) bail out early with 'if (!is_data_valid()) return;' — so a single transient SPI failure leaves board_rev / serial# / ethaddr unset for the whole boot. Add an ensure_otp_data_ready() helper that re-invokes read_otp_data() when the cached data is not yet valid, and use it in both consumers. Valid/invalid-data behavior is unchanged; only the retry path is now actually exercised. Addresses a CodeRabbit review comment on PR armbian#9675.
Match the snprintf(env_str, sizeof(env_str), ...) call just above it. Safe today (48-bit serial fits in 13 bytes), but guards against future changes to otp_serial() width. Suggested-by: coderabbitai[bot]
…p_data The retry-helper commit added a forward declaration so ensure_otp_data_ready() could refer to read_otp_data() (defined further down), but mistakenly marked it static — conflicting with the non-static declaration in sys_otp.h and with the non-static definition at line 159. Just drop the forward declaration; sys_otp.h already provides the prototype. Suggested-by: coderabbitai[bot]
The earlier nodeoff-shadowing fix wrapped the inner declaration in #ifndef FDT_SEQ_MACADDR_FROM_ENV ... #endif, adding two lines to the new-side of the hunk, but the hunk header was left at +686,52 instead of +686,54. GNU patch is strict about counts, so the extra payload lines were flagged as malformed. Bump the count to 54 so the patch applies again.
show_otp_data() printed otp.part_num (a fixed 16-byte field, not a C string) with plain %s. If OTP content lacks a NUL terminator, printf would over-read into the adjacent packed fields. Use %.*s with strnlen(..., sizeof(otp.part_num)) so the output is bounded by both the actual content length and the field size. Only reachable in DEBUG builds, but the UB is real. Suggested-by: coderabbitai[bot]
get_variant() was the only public OTP accessor that read the otp struct without first validating/loading the cache. After a transient SPI failure during board_early_init_r(), it could return a misleading variant string from stale/partial data instead of exercising the retry path that set_board_info() and mac_read_from_otp() already use. Also make var_str static const — it's a read-only lookup table that doesn't need to be rebuilt on every call. Suggested-by: coderabbitai[bot]
…on() Same defensive pattern as get_variant(): a transient early OTP read failure would leave otp.valid=false, causing get_revision() to return -1 without ever retrying. Route through ensure_otp_data_ready() so a late-booting SPI controller gets a second chance. Suggested-by: coderabbitai[bot] <coderabbitai[bot]@users.noreply.github.com>
…atch The patch is Ricardo Pardini's WIP instrumentation for debugging aliases handling in fdt_fixup_ethernet(); carry it under .patch.disabled so the framework skips it until someone explicitly opts in.
iav
force-pushed
the
feat/helios64-uboot-v2026.04
branch
from
002a657 to
4165426
Compare
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Three commits, grouped into one PR since they all touch
patch/u-boot/v2026.04/:1.
helios64: bump u-boot to v2026.04patch/u-boot/v2026.04/board_helios64/— helios64 uses a self-containedBOOTPATCHDIRand does not inherit from the parent dir.2.
helios64: u-boot: cherry-pick fdt_fixup_ethernet logs and fileenv cmdPull two generic v2026.04 patches that helios64 currently misses because
BOOTPATCHDIRpoints at the self-containedboard_helios64subdirectory:1001-fdt_fixup_ethernet-add-logs.patch— diagnosticlog_info()in DT Ethernet fixup.cmd-fileenv-read-string-from-file-into-env.patch— adds thefileenvCLI command.Enables
CONFIG_CMD_FILEENVvia apost_config_uboot_target__helios64_enable_fileenvhook inhelios64.conf, matching the convention of odroidm1 / khadas-vim3 / odroidm1s (patch + enable in the same commit). The command is purely additive — does not affectarmbianEnv.txtor existing env mechanics.Other parent-level v2026.04 patches (rk3288 VOP, rk3588 I2S MCLK, DW-HDMI disable) are SoC-specific or require config options helios64 does not enable, so they are not pulled in.
3.
u-boot: v2026.04: add btrfs zstd decompression fixSame fix as #9651 for v2026.01 and v2025.10, applied to the common
patch/u-boot/v2026.04/directory so all boards on U-Boot v2026.04 pick it up (cm3588-nas, nanopct6, orangepi5, rock-3a, nanopi-r3s-lts, mekotronics-*, mixtile-blade3, radxa-rock-4d — helios64 via its own self-containedboard_helios64subdir already has a copy from commit 1).Without this, booting from a BTRFS rootfs with zstd-compressed extents fails with:
Test plan
./compile.sh iav uboot BOARD=helios64 BRANCH=edge—linux-u-boot-helios64-edge_..._2026.04-S88dc-Pab2e-H1c4d-Vfcb0-B138f-R448a.deb.armbian-install, reboot to login prompt.U-Boot 2026.04_armbian-2026.04-S88dc-Pab2e-....fdt_fixup_ethernetdebug output visible: aliases enumerated, MAC patched intoethernet@fe300000.btrfs+BTRFS_COMPRESSION=zstd), nozstd_decompress: failed to decompress: 70.fileenvavailable in U-Boot CLI (config hook present, command usable when needed).Summary by CodeRabbit
Release Notes
New Features
fileenvcommand to load environment variables from files stored on block devices.Bug Fixes
Chores