Conversation
nikvoid
requested review from
NicoD-SBC,
SuperKali,
TheSnowfield,
Tonymac32,
amazingfate,
catalinii,
igorpecovnik,
pyavitz and
rpardini
as code owners
WalkthroughAdds support for the Luckfox Nova W (Rockchip RK3308B) by introducing a board configuration (config/boards/luckfox-nova-w.csc) that specifies boot/kernel/runtime settings, kernel source/versions, firmware blobs, and board-specific tweaks. Adds a Rockchip64 U-Boot bootscript for ttyS4 (config/bootscripts/boot-rockchip64-ttyS4.cmd) implementing armbianEnv merging, DT overlay/fixup handling, and kernel boot logic. Adds a vendor Linux kernel config (config/kernel/linux-luckfox-nova-w-rk3308-vendor.config) tailored for RK3308 with extensive drivers and options. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes Reason: multiple new files with moderate logic density (board config functions and a nontrivial U-Boot bootscript) and a large kernel config file. Changes are heterogeneous (init scripts, shell functions, config content) requiring separate checks per file but limited in file count. Possibly related PRs
Suggested labels
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
size/large
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
config/bootscripts/boot-rockchip64-ttyS4.cmd (1)
62-66: Overlay restore path leavesoverlay_errorset to trueAfter restoring the original DT,
overlay_erroris left unchanged. Subsequent logic or scripts that inspect this variable will still think an overlay error is active.-load ${devtype} ${devnum}:${distro_bootpart} ${fdt_addr_r} ${prefix}dtb/${fdtfile} +load ${devtype} ${devnum}:${distro_bootpart} ${fdt_addr_r} ${prefix}dtb/${fdtfile} +setenv overlay_error "false"config/boards/luckfox-nova-w.csc (1)
53-55:mvmay abort if the firmware path does not existOn early images the upper-case directory might already be in lower-case form,
causingmvto fail and abort the whole build script.-mv "${SDCARD}/lib/firmware/aic8800/SDIO/aic8800DC" "${SDCARD}/lib/firmware/aic8800/SDIO/aic8800dc" +mv -f "${SDCARD}/lib/firmware/aic8800/SDIO/aic8800DC" \ + "${SDCARD}/lib/firmware/aic8800/SDIO/aic8800dc" 2>/dev/null || trueconfig/kernel/linux-luckfox-nova-w-rk3308-vendor.config (1)
465-488: Heavy debug & tracing options will waste scarce 512 MB DRAM
CONFIG_DYNAMIC_DEBUG,FTRACE,DEBUG_INFO_*,PROVE_LOCKING, full
PREEMPT,HZ_1000, etc. are all enabled. Great for bring-up, but they cost
both memory (≈ 40-60 MB extra) and performance on a tiny Cortex-A35.Consider shipping two defconfigs:
• debug (current), and
• production with
CONFIG_PREEMPT_VOLUNTARYCONFIG_HZ_250# CONFIG_DYNAMIC_DEBUG is not set# CONFIG_DEBUG_INFO_* is not set# CONFIG_FUNCTION_TRACER is not setThis drops idle RAM usage by ~10 % on RK3308.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
patch/kernel/rockchip64-vendor-6.1/0000.patching_config.yamlis excluded by!patch/**patch/kernel/rockchip64-vendor-6.1/aic8800-select-rk-platform.patchis excluded by!patch/**patch/kernel/rockchip64-vendor-6.1/dt/rk3308-luckfox-nova-w.dtsis excluded by!patch/**patch/kernel/rockchip64-vendor-6.1/qcaspi-rkspi-fix.patchis excluded by!patch/**patch/kernel/rockchip64-vendor-6.1/rfkill-rk-sdio-rescan.patchis excluded by!patch/**patch/kernel/rockchip64-vendor-6.1/usbotg-fix.patchis excluded by!patch/**patch/u-boot/v2025.04/board_luckfox-nova-w/add-board-luckfox-nova-w-rk3308b.patchis excluded by!patch/**
📒 Files selected for processing (3)
config/boards/luckfox-nova-w.csc(1 hunks)config/bootscripts/boot-rockchip64-ttyS4.cmd(1 hunks)config/kernel/linux-luckfox-nova-w-rk3308-vendor.config(1 hunks)
🔇 Additional comments (1)
config/bootscripts/boot-rockchip64-ttyS4.cmd (1)
37-41:rootdevremains a hard-coded/dev/mmcblk0p1whilepartuuidis resolvedYou correctly detect the partition UUID (line 38) but do not use it.
Using the device node in one place and PARTUUID in another breaks when
/dev/mmcblk0numbering changes (USB-boot, NVMe, different eMMC order, etc.).-setenv rootdev "/dev/mmcblk0p1" +setenv rootdev "PARTUUID=${partuuid}"Alternatively keep
rootdevdefault and replace${rootdev}with
PARTUUID=${partuuid}directly inbootargs.
github-actions
bot
removed
the
Ready to merge
There was a problem hiding this comment.
Please note that these patches must not break existing hardware support, and it's best to submit them to this repository: https://github.com/armbian/linux-rockchip.
There was a problem hiding this comment.
I know, but they may break, and now this is a single board with these patches
| # eMMC works properly only in vendor branch | ||
| KERNEL_TARGET="vendor" | ||
|
|
||
| KERNELSOURCE='https://github.com/armbian/linux-rockchip.git' |
There was a problem hiding this comment.
I suggest referring to other RK3308-based devices, and ideally using the mainline kernel.
There was a problem hiding this comment.
Agreed. AFAIR rk bsp is mostly designed to support rk35xx family, not sure how much work has been done for rk33xx. Though most of if is mainlined and this shall be preferred.
There was a problem hiding this comment.
I could not get emmc working in mainline kernels. May be I'll revisit this later to derive patches
There was a problem hiding this comment.
Please ensure this patch does not break aic8800 support on other Rockchip devices.
There was a problem hiding this comment.
Maybe just make patches board-specific to be sure? Could you say how to do that?
| # eMMC works properly only in vendor branch | ||
| KERNEL_TARGET="vendor" | ||
|
|
||
| KERNELSOURCE='https://github.com/armbian/linux-rockchip.git' |
There was a problem hiding this comment.
Agreed. AFAIR rk bsp is mostly designed to support rk35xx family, not sure how much work has been done for rk33xx. Though most of if is mainlined and this shall be preferred.
|
@nikvoid are you still interested to get your work merged? |
|
Yes. I made some of requested changes, but not sure what to do about kernel patch directory. I cannot test patches on other boards, so do I need to rename directory to |
|
Also dedicated bootloader script just for this board is not acceptable. We already have a maintaining nightmare and @djurny is trying to come out with some generic solution. Perhaps he can provide some tips how to address this part. Adding new board should have as minimal impact on common things as possible. |
|
@nikvoid Thank you for your work. As you can see, it is not easy. And supporting a new board can only ever be done if it doesn't break other boards. The onerous is on the new entrant. Keep asking questions and also keep asking for help to get this done. Don't be discouraged. |
A quick look at the bootscript shows only a change in the serial device. Next week I will have the time to take a good look at this. |
djurny
left a comment
djurny
left a comment
There was a problem hiding this comment.
For the bootscript, let's keep them all the same as much as possible. Any improvement can be made at a later stage.
| fi | ||
| fi | ||
|
|
||
| booti ${kernel_addr_r} ${ramdisk_addr_r} ${fdt_addr_r} |
There was a problem hiding this comment.
We're missing the KASLR seed portion from the [default] rockchip64 bootscripts:
echo "Trying 'kaslrseed' command... Info: 'Unknown command' can be safely ignored since 'kaslrseed' does not apply to all boards."
kaslrseed # @TODO: This gives an error (Unknown command ' kaslrseed ' - try 'help') on many devices since CONFIG_CMD_KASLRSEED is not enabled
Notes:
- The snippet is verbatim.
- The bootscript for
rockchip64-ttyS0is also missing the KASLR seeding.
|
|
||
| if test "${logo}" = "disabled"; then setenv logo "logo.nologo"; fi | ||
|
|
||
| if test "${console}" = "display" || test "${console}" = "both"; then setenv consoleargs "console=tty0"; fi |
There was a problem hiding this comment.
if test "${console}" = "display" || test "${console}" = "both"; then setenv consoleargs "console=tty0"; fi
The other rockchip64 bootscripts have tty1 as 'display' TTY. Did you confirm that tty0 is indeed displaying on any video-enabled output?
| setenv rootfstype "ext4" | ||
| setenv docker_optimizations "off" | ||
| setenv earlycon "on" | ||
| setenv usbstoragequirks "" |
There was a problem hiding this comment.
setenv usbstoragequirks ""
Propose to remove this, as the other rockchip64 bootscripts also not clear this environment variable. Let's improve all in another round.
| setenv console "both" | ||
| setenv bootlogo "false" | ||
| setenv rootfstype "ext4" | ||
| setenv docker_optimizations "off" |
There was a problem hiding this comment.
Any reason to disable the docker_optimizations?
|
@nikvoid Have you gotten any insight as to why the vendor kernel is required to access the board's EMMC? |
|
I just tried all kernel branches and only vendor booted. I did not check SD card so it's surprise for me that it is not working. Maybe some devicetree requires some tuning? P.S. I'm too busy with other things right now, so probably will be inactive on this PR for some time. |
|
Bump on this, please. It would be really helpful to have this working |
9 participants
Description
Add initial support for board Luckfox Nova W.
Luckfox Nova W is recently produced board on SoC Rockchip RK3308B: 4 x Cortex-A35 CPU, 512 RAM, 8 GB eMMC.
It also has Ethernet, WIFI & BT, USB-A and USB-C ports, soldered microphone.
Product page: https://www.luckfox.com/Luckfox-Nova?ci=646
But beware: it is very fresh board, has almost no documentation and only buildroot SDK with kernel 5.10 is available from vendor.
Documentation
I decided to create separate directory for vendor kernel patches. This kernel must be the same as
rk35xx-vendor-6.1, but this SoC is not RK35XX.Used U-Boot is mainline.
Instead of DKMS AIC8800 driver I enabled the one from kernel sources, because I needed to apply patches to it. And image builds faster than with DKMS.
UART4 is used for serial console and by proprietary firmware blobs.
How Has This Been Tested?
currentandedgerk6.1Checklist:
Please delete options that are not relevant.