Skip to content

Conversation

@vidplace7
Copy link
Member

@vidplace7 vidplace7 commented Oct 28, 2025

Description

Add new RK3506 CSC board "Luckfox Lyra Plus".
Additionally, tweaks RK3506 BOOT_SOC-specific settings to allow RK3506g2 to boot. (Else fails with memory allocation issues)

Depends on:

Relates to:

How Has This Been Tested?

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

  • ./compile.sh BOARD=luckfox-lyra-plus BRANCH=vendor KERNEL_CONFIGURE=no DEB_COMPRESS=xz KERNEL_BTF=yes INSTALL_ARMBIAN_FIRMWARE=yes BUILD_DESKTOP=no BUILD_MINIMAL=yes RELEASE=trixie
  • Luckfox Lyra Plus boots, shell works over UART0
  • Ethernet working
  • Boot from SD working

Checklist:

Please delete options that are not relevant.

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

@github-actions github-actions bot added 11 Milestone: Fourth quarter release size/small PR with less then 50 lines Needs review Seeking for review Hardware Hardware related like kernel, U-Boot, ... labels Oct 28, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 28, 2025

Walkthrough

This PR adds support for the Luckfox Lyra Plus board based on the Rockchip RK3506G2 SoC. It includes a new board configuration file (luckfox-lyra-plus.csc) with metadata and boot parameters, a boot environment configuration file (rk3506.txt) with verbosity and memory settings, a boot script (boot-rk3506.cmd) defining ramdisk address parameters, and updates to the Rockchip family configuration (rockchip.conf) to set RK3506-specific bootenv file references and SERIALCON defaults.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Modifications to config/sources/families/rockchip.conf should be verified to ensure the SERIALCON parameter expansion and bootenv file change for rk3506 don't conflict with existing configurations
  • The new board file should be validated against existing board configuration patterns and the documented low-memory extension reference

Possibly related PRs

  • armbian/build#8619: Adds RK3506 boot script and Rockchip family configuration settings alongside new board configurations, modifying the same boot and configuration files
  • armbian/build#8764: Introduces Luckfox Lyra board configurations for RK3506 with overlapping build variables and bootenv file modifications

Suggested labels

Patches, BSP

Suggested reviewers

  • igorpecovnik
  • paolosabatino
  • prahal
  • rpardini
  • krachlatte
  • pyavitz

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The pull request title "rockchip: Add CSC board Luckfox Lyra Plus + adjust RK3506 BOOT_SOC" is directly related to the main changes in the changeset. The title clearly identifies both primary objectives: adding a new board configuration file for the Luckfox Lyra Plus (config/boards/luckfox-lyra-plus.csc) and adjusting RK3506-specific settings across multiple configuration files to enable proper booting. The title is concise, specific, and follows standard commit message conventions with the subsystem prefix, making it clear to reviewers scanning the history.
Description Check ✅ Passed The pull request description is clearly related to the changeset and provides meaningful context about the changes. The author describes adding a new RK3506 CSC board configuration for "Luckfox Lyra Plus" with adjustments to RK3506 BOOT_SOC-specific settings, which directly aligns with the new board configuration file and the modifications to the rockchip.conf family settings. The description explains the motivation behind the changes (fixing memory allocation issues for RK3506g2) and includes testing evidence demonstrating the changes work as intended. The description is neither vague nor off-topic.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 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.

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 (1)
config/bootenv/rk3506.txt (1)

3-3: Consider adding a trailing newline.

The file is missing a trailing newline, which is a minor POSIX compliance and style issue. This is optional but helps with text file handling.

Apply this diff to add a trailing newline:

 bootlogo=false
+
📜 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 f958184 and b066e02.

📒 Files selected for processing (4)
  • config/boards/luckfox-lyra-plus.csc (1 hunks)
  • config/bootenv/rk3506.txt (1 hunks)
  • config/bootscripts/boot-rk3506.cmd (1 hunks)
  • config/sources/families/rockchip.conf (2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-26T10:41:35.118Z
Learnt from: HackingGate
PR: armbian/build#8665
File: config/boards/photonicat2.csc:4-4
Timestamp: 2025-10-26T10:41:35.118Z
Learning: In the Armbian build system, rk3576 boards consistently use BOARDFAMILY="rk35xx" for both vendor and edge kernel targets. The rk35xx family configuration sources rockchip64_common.inc, which provides edge and current kernel branch definitions, making these branches available even though they're not defined directly in rk35xx.conf.

Applied to files:

  • config/sources/families/rockchip.conf
  • config/boards/luckfox-lyra-plus.csc
📚 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/sources/families/rockchip.conf
🪛 LanguageTool
config/bootenv/rk3506.txt

[grammar] ~3-~3: Dit kan een fout zijn.
Context: ...xtraargs=coherent_pool=4M bootlogo=false

(QB_NEW_NL)

🔇 Additional comments (6)
config/bootenv/rk3506.txt (1)

2-2: Verify coherent_pool size for 128MB device.

The coherent_pool=4M setting allocates approximately 3% of the total 128MB RAM for DMA coherent memory. While this may be appropriate, please verify that this allocation doesn't constrain the system too much, especially given the low-memory profile of this board. Consider testing with different values if boot or runtime issues arise.

config/bootscripts/boot-rk3506.cmd (1)

7-7: Memory address layout looks reasonable for 128MB device.

The ramdisk_addr_r at 0x02800000 (42MB) provides adequate spacing from load_addr at 0x2000000 (32MB) and should work well for a device with 128MB total RAM. The address is properly used in the load command (line 47) and bootz command (line 80).

If you encounter memory-related boot issues, consider verifying the complete memory map to ensure kernel, ramdisk, and DTB don't overlap:

  • kernel_addr_r (inherited from u-boot environment)
  • ramdisk_addr_r = 0x02800000 (42MB)
  • fdt_addr_r (inherited from u-boot environment)
  • load_addr = 0x2000000 (32MB)
config/sources/families/rockchip.conf (2)

17-17: Good use of parameter expansion for overridable default.

The change to SERIALCON=${SERIALCON:="ttyS2"} correctly provides a default value while allowing board configurations to override it. This works well with the new Luckfox Lyra Plus board that sets SERIALCON="ttyFIQ0" in its board config.


51-51: Boot environment file correctly points to new RK3506-specific config.

The change to BOOTENV_FILE='rk3506.txt' properly references the new boot environment file created in this PR with RK3506-specific settings (coherent_pool=4M, etc.). This separation from the generic rockchip.txt is appropriate for the distinct requirements of the RK3506 SoC.

config/boards/luckfox-lyra-plus.csc (2)

1-13: Configuration looks good overall.

The board configuration properly sets up the RK3506G2 with appropriate settings:

  • SERIALCON="ttyFIQ0" correctly overrides the family default for this board's serial console
  • BOOT_SOC="rk3506" properly selects the RK3506 boot configuration
  • DDR_BLOB and other boot parameters are appropriately specified

2-2: Clarify board name discrepancy.

The PR title mentions "Luckfox Lyra Zero W" but the board configuration specifies BOARD_NAME="Luckfox Lyra Plus". Please verify which is the correct name for this board to ensure consistency between the PR description and the actual configuration.

Comment on lines +14 to +15
# Board only has 128MB RAM; use 'lowmem' extension to optimize for this.
# enable_extension "lowmem"
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 28, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Uncomment the lowmem extension for 128MB device.

The comment states "Board only has 128MB RAM; use 'lowmem' extension to optimize for this," but the extension is commented out. For a device with only 128MB RAM, the lowmem extension should be enabled to optimize memory usage and prevent out-of-memory issues during build and runtime.

Apply this diff to enable the lowmem extension:

 # Board only has 128MB RAM; use 'lowmem' extension to optimize for this.
-# enable_extension "lowmem"
+enable_extension "lowmem"
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
# Board only has 128MB RAM; use 'lowmem' extension to optimize for this.
# enable_extension "lowmem"
# Board only has 128MB RAM; use 'lowmem' extension to optimize for this.
enable_extension "lowmem"
🤖 Prompt for AI Agents
In config/boards/luckfox-lyra-plus.csc around lines 14 to 15, the lowmem
extension is currently commented out despite the board having only 128MB of RAM;
enable the lowmem extension by uncommenting or adding the enable_extension
"lowmem" line so the build/system uses the low-memory optimizations for this
device.

Copy link
Member Author

Choose a reason for hiding this comment

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

This can be un-commented when #8839 is merged

Copy link
Contributor

Choose a reason for hiding this comment

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

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Fetch details about PR #8839 to understand the dependency
gh pr view 8839 --repo armbian/build --json title,state,url,body

Length of output: 1880


@vidplace7, thank you for the clarification! I see that PR #8839 ("LowMem extension (for boards with < 256MB RAM)") is currently open and introduces the lowmem extension that this board needs. It makes perfect sense to wait for that PR to merge before uncommenting the extension here.

Since PR #8839 was actually tested with this exact board (RK3506g2 128MB Luckfox Lyra Plus), the dependency is well-founded.

Would you like me to create a follow-up issue to track uncommenting the lowmem extension once PR #8839 is merged? This would help ensure the optimization isn't forgotten after the dependency lands.


🧠 Learnings used
Learnt from: HackingGate
PR: armbian/build#8665
File: config/boards/photonicat2.csc:4-4
Timestamp: 2025-10-26T10:41:35.118Z
Learning: In the Armbian build system, rk3576 boards consistently use BOARDFAMILY="rk35xx" for both vendor and edge kernel targets. The rk35xx family configuration sources rockchip64_common.inc, which provides edge and current kernel branch definitions, making these branches available even though they're not defined directly in rk35xx.conf.

Learnt from: tabrisnet
PR: armbian/build#8661
File: lib/functions/compilation/armbian-kernel.sh:194-199
Timestamp: 2025-09-25T18:37:00.330Z
Learning: In PR armbian/build#8661, line 235 of lib/functions/compilation/armbian-kernel.sh already contains the corrected comment "BPF link support for netfilter hooks" for NETFILTER_BPF_LINK, not the misleading "BPF_SYSCALL" comment that was flagged during review.

Learnt from: tabrisnet
PR: armbian/build#8661
File: lib/functions/compilation/armbian-kernel.sh:194-199
Timestamp: 2025-09-25T18:37:00.330Z
Learning: In PR armbian/build#8661, line 235 of lib/functions/compilation/armbian-kernel.sh already contains the corrected comment "BPF link support for netfilter hooks" for NETFILTER_BPF_LINK, not the misleading "BPF_SYSCALL" comment that was flagged during review.

@@ -0,0 +1,3 @@
verbosity=1
extraargs=coherent_pool=4M
Copy link
Member Author

Choose a reason for hiding this comment

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

coherent_pool=2M (as found in bootenv/rockchip.txt) is causing memory coherent DMA errors when initializing Ethernet.
=4M resolves this.

#

setenv load_addr "0x2000000"
setenv ramdisk_addr_r "0x02800000"
Copy link
Member Author

@vidplace7 vidplace7 Oct 28, 2025

Choose a reason for hiding this comment

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

Required for RK3506g2 (128MB) else memory overlaps at boot
@amazingfate

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, ... Needs review Seeking for review size/small PR with less then 50 lines

Development

Successfully merging this pull request may close these issues.

1 participant