Skip to content

Conversation

@rpardini
Copy link
Member

  • keep vendor u-boot for vendor branch (with rk's BL31)

… 'current' branch

- keep vendor u-boot for vendor branch (with rk's BL31)
@coderabbitai
Copy link
Contributor

coderabbitai bot commented May 24, 2025

Walkthrough

This change updates the NanoPC T6 board configuration script to ensure that mainline U-Boot and related settings are not applied when the "vendor" branch is selected. It introduces early return checks in several functions to skip mainline-specific configuration for the vendor branch, which uses an older U-Boot version. Function names are updated to remove the "branch_edge" suffix, and the U-Boot target map is adjusted to use a fixed BL31 filename. The logic in multiple functions is modified to enforce these exclusions, preserving distinct behaviors for vendor and mainline branches.

Suggested labels

ready to merge

Suggested reviewers

  • igorpecovnik
  • joekhoobyar

Note

⚡️ AI Code Reviews for VS Code, Cursor, Windsurf

CodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback.
Learn more here.


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/small PR with less then 50 lines Needs review Seeking for review Hardware Hardware related like kernel, U-Boot, ... labels May 24, 2025
@rpardini rpardini marked this pull request as ready for review May 24, 2025 12:53
@coderabbitai coderabbitai bot added the Ready to merge Reviewed, tested and ready for merge label May 24, 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

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

79-81: Remove redundant comment lines.

The same comment is repeated three times unnecessarily, which is a code smell and reduces readability.

-	# Append to the t6 u-boot dtsi file with stuff for enabling gadget/otg/peripheral mode
-	# Append to the t6 u-boot dtsi file with stuff for enabling gadget/otg/peripheral mode
-	# Append to the t6 u-boot dtsi file with stuff for enabling gadget/otg/peripheral mode
📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between d0f708d and d759215.

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

33-40: LGTM: Proper function rename and mainline ATF integration.

The function rename from post_family_config_branch_edge__nanopct6_use_mainline_uboot() to post_family_config__nanopct6_use_mainline_uboot() correctly reflects that this now applies to multiple branches (current and edge). The early return for vendor branch and the switch to BOOT_SCENARIO="tpl-blob-atf-mainline" with prepare_boot_configuration() call properly implements the mainline A-TF integration as stated in the PR objectives.


48-48: LGTM: Fixed BL31 filename usage.

The change from a variable blob path to the fixed bl31.elf filename aligns with using mainline ATF and simplifies the build configuration.


64-66: LGTM: Consistent function rename and vendor branch exclusion.

The function rename removes the "branch_edge" suffix consistently with other changes, and the early return for vendor branch ensures that vendor U-Boot continues to handle device tree detection itself.


70-70: LGTM: Vendor branch exclusion is appropriate.

The early return for vendor branch correctly prevents mainline-specific U-Boot device tree modifications from being applied to the vendor branch.


98-98: LGTM: Vendor branch exclusion for mainline configurations.

The early return correctly prevents mainline U-Boot environment configurations from being applied to the vendor branch, maintaining separation between vendor and mainline behaviors.


147-147: LGTM: Vendor branch exclusion for firmware environment tools.

The early return appropriately excludes vendor branch from the firmware environment configuration, as vendor U-Boot likely has different environment storage requirements.

@rpardini
Copy link
Member Author

nanopct6-lts bootlog, with ATF 2.13.0

U-Boot SPL 2025.04-armbian-2025.04-S3482-Pcff6-H1cc9-V0d2b-B1e5e-R448a-dirty (May 24 2025 - 12:36:52 +0000)
Trying to boot from SPI
## Checking hash(es) for config config-2 ... OK
## Checking hash(es) for Image atf-1 ... sha256+ OK
## Checking hash(es) for Image u-boot ... sha256+ OK
## Checking hash(es) for Image fdt-2 ... sha256+ OK
## Checking hash(es) for Image atf-2 ... sha256+ OK
## Checking hash(es) for Image atf-3 ... sha256+ OK
NOTICE:  BL31: v2.13.0(release):armbian
NOTICE:  BL31: Built : 12:36:21, May 24 2025
INFO:    GICv3 without legacy support detected.
INFO:    ARM GICv3 driver initialized in EL3
INFO:    Maximum SPI INTID supported: 511
INFO:    BL31: Initializing runtime services
INFO:    BL31: Preparing for EL3 exit to normal world
INFO:    Entry point address = 0xa00000
INFO:    SPSR = 0x3c9


U-Boot 2025.04-armbian-2025.04-S3482-Pcff6-H1cc9-V0d2b-B1e5e-R448a-dirty (May 24 2025 - 12:36:52 +0000)

Model: FriendlyElec NanoPC-T6 LTS
SoC:   RK3588
DRAM:  16 GiB
rockchip_rk805 pmic@0: pinctrl_select_state_full: uclass_get_device_by_phandle_id: err=-19
rockchip_rk805 pmic@0: pinctrl_selecselect_state_full: uclass_get_device_by_phandle_id: err=-19
PMIC:  RK806 (on=0x80, off=0x04)
Core:  391 devices, 35 uclasses, devicetree: separate
MMC:   mmc@fe2c0000: 1, mmc@fe2e0000: 0
Loading Environment from SPIFlash... SF: Detected w25q256fw with page size 256 Bytes, erase size 4 KiB, total 32 MiB
OK
In:    serial@feb50000
Out:   serial@feb50000
Err:   serial@feb50000
Model: FriendlyElec NanoPC-T6 LTS
SoC:   RK3588
rockchip_dnl_key_pressed: no saradc device found
Net:   No ethernet found.
Hit any key to stop autoboot:  0
Scanning for bootflows in all bootdevs
Seq  Method       State   Uclass    Part  Name                      Filename
------------
Scanning global bootmeth 'efi_mgr':
Card did not respond to voltage select! : -110
Cannot persist EFI variables without system partition
  0  efi_mgr      ready   (none)       0  <NULL>
** Loading Boot0000 'mmc 0' failed
EFI boot manager: Cannot load any image
Boot failed (err=-14)
Scanning bootdev '[email protected]':
Card did not respond to voltage select! : -110
pcie_dw_rockchip pcie@fe180000: PCIe-0 Link Fail
Scanning bootdev 'nvme#0.blk#1.bootdev':
Scanning bootdev '[email protected]':
  1  script       ready   mmc          1  [email protected] /boot/boot.scr
** Booting bootflow '[email protected]_1' with script
Boot script loaded from mmc 0:1
124 bytes read in 11 ms (10.7 KiB/s)
26431289 bytes read in 173 ms (145.7 MiB/s)
39014912 bytes read in 239 ms (155.7 MiB/s)
182118 bytes read in 55 ms (3.2 MiB/s)
Working FDT set to 12000000
Trying kaslrseed command... Info: Unknown command can be safely ignored since kaslrseed does not apply to all boards.
Unknown command 'kaslrseed' - try 'help'
## Loading init Ramdisk from Legacy Image at 12180000 ...
   Image Name:   uInitrd
   Image Type:   AArch64 Linux RAMDisk Image (gzip compressed)
   Data Size:    26431225 Bytes = 25.2 MiB
   Load Address: 00000000
   Entry Point:  00000000
   Verifying Checksum ... OK
## Flattened Device Tree blob at 12000000
   Booting using the fdt blob at 0x12000000
Working FDT set to 12000000
   Loading Ramdisk to eb27b000, end ecbafef9 ... OK
   Loading Device Tree to 00000000eb1e6000, end 00000000eb27afff ... OK
Working FDT set to eb1e6000

Starting kernel ...

[    0.000000] Booting Linux on physical CPU 0x0000000000 [0x412fd050]
[    0.000000] Linux version 6.15.0-rc7-edge-rockchip64 U Binutils for Ubuntu) 2.42) #1 SMP PREEMPT Sun May 18 20:57:29 UTC 2025
[    0.000000] KASLR enabled
[    0.000000] Machine model: FriendlyElec NanoPC-T6 LTS

Copy link
Member

@SuperKali SuperKali left a comment

Choose a reason for hiding this comment

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

i can't test on non-LTS because i have to shutdown my production infrastructure, but when i will update it i will test

@SuperKali SuperKali removed the Needs review Seeking for review label May 24, 2025
@timsurber
Copy link
Contributor

Could we simply switch to tpl-blob-atf-mainline in the general RK3588 configuration?

@rpardini
Copy link
Member Author

Could we simply switch to tpl-blob-atf-mainline in the general RK3588 configuration?

This board has a vendor branch, with 2017 u-boot and vendor kernel. I kept that, added mainline atf+uboot to current.

@timsurber
Copy link
Contributor

Could we simply switch to tpl-blob-atf-mainline in the general RK3588 configuration?

This board has a vendor branch, with 2017 u-boot and vendor kernel. I kept that, added mainline atf+uboot to current.

I thought about setting it here:

BOOT_SCENARIO="${BOOT_SCENARIO:=spl-blobs}"

Because I think it would be useful that every 3588 board using current would use atf-mainline. But it is probably better to change it for every board

@igorpecovnik
Copy link
Member

I thought about setting it here

We can do such move as its some time before next release. @efectn @amazingfate @SuperKali @prahal

@igorpecovnik igorpecovnik added Discussion Being discussed - Voice your opinions :) and removed Ready to merge Reviewed, tested and ready for merge labels May 29, 2025
@rpardini
Copy link
Member Author

Yeah, but the reality is many boards override what's in rockchip64_common with hooks. It's mostly my fault, as I did the first example, but I gotta find time to rework a fez dozen boards with copypasta all over into a couple extensions before we can "one-shot" change them all. I've promised to do it enough times to be shameful, but cycles are sparse.

@igorpecovnik
Copy link
Member

OK, then we move this in now and move this wish on 2do.

@Tonymac32
Copy link
Member

I've tried to mass add boards to mainline ATF and uboot in rockchip64 before, the reality is the Rockchip blobs are often masking complete and utter garbage from the board vendors l, and half the boards break when expected to act like something with half an hour of engineering put into their design.

Only a word of caution. I still feel a move away from having family include definitions is preferable since it honestly gets more complicated than one config per board.

@rpardini rpardini merged commit f149a11 into armbian:main May 31, 2025
1 check passed
@rpardini
Copy link
Member Author

@Tonymac32 yeah, this here T6 example has vendor uboot+rk bl31 for vendor branch and mainline uboot+mainline atf for edge and current, thus it has "2 scenarios per board determined by BRANCH", while rockchip64_common only ever supports one-per-board-across-all-branches.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

05 Milestone: Second quarter release Discussion Being discussed - Voice your opinions :) Hardware Hardware related like kernel, U-Boot, ... size/small PR with less then 50 lines

Development

Successfully merging this pull request may close these issues.

5 participants