Add Khadas VIM3L #3899
Add Khadas VIM3L #3899
Conversation
Signed-off-by: Gunjan Gupta <[email protected]>
|
Warning Rate limit exceeded@viraniac has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 42 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis pull request updates the Buildroot support for the Khadas Vim3L board. It adds new configuration entries, scripts, and metadata to refine boot image handling, bootloader management, and system builds. Notable changes include the addition of commented hints in boot configuration files, a new hook for pre/post image processing, updated image/partition settings, detailed board metadata, and enhancements to the U-Boot boot script with a retry mechanism. The Buildroot configuration and bootloader package scripts have also been modified to support conditional processing for different hardware variants. Changes
Sequence Diagram(s)sequenceDiagram
participant U as U-Boot Boot Script
participant E as Boot Environment
participant K as Kernel Boot Process
U->>E: Load boot environment variables
U->>U: Initialize boot parameters (slots A/B, retry counters)
U->>K: Attempt boot from Slot A
alt Slot A Fails
U->>U: Decrement retry counter for Slot A
U->>K: Attempt boot from Slot B
end
alt Both Slots Fail
U->>E: Reset boot parameters and update environment
U->>K: Retry kernel boot
end
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
buildroot-external/board/khadas/vim3l/hassos-hook.sh (2)
1-12: The pre-image hook looks good but consider adding validation.The
hassos_pre_imagefunction correctly copies the necessary files to prepare the boot directory for the Khadas VIM3L board. However, it might be helpful to add some validation to ensure the source files exist before attempting to copy them.Consider adding error checking before copying files:
function hassos_pre_image() { local BOOT_DATA="$(path_boot_dir)" + # Validate that required files exist + for file in "${BINARIES_DIR}/boot.scr" "${BINARIES_DIR}/meson-sm1-khadas-vim3l.dtb" "${BOARD_DIR}/boot-env.txt" "${BOARD_DIR}/cmdline.txt"; do + if [ ! -f "${file}" ]; then + echo "Error: Required file ${file} is missing!" + exit 1 + fi + done + cp "${BINARIES_DIR}/boot.scr" "${BOOT_DATA}/boot.scr" cp "${BINARIES_DIR}/meson-sm1-khadas-vim3l.dtb" "${BOOT_DATA}/meson-sm1-khadas-vim3l.dtb" cp "${BOARD_DIR}/boot-env.txt" "${BOOT_DATA}/haos-config.txt" cp "${BOARD_DIR}/cmdline.txt" "${BOOT_DATA}/cmdline.txt" }
19-19: Remove empty line at EOF.There's an extra empty line at the end of file that should be removed to comply with standard file formatting.
-
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
buildroot-external/board/khadas/vim3l/boot-env.txt(1 hunks)buildroot-external/board/khadas/vim3l/cmdline.txt(1 hunks)buildroot-external/board/khadas/vim3l/hassos-hook.sh(1 hunks)buildroot-external/board/khadas/vim3l/image-spl-spl.cfg(1 hunks)buildroot-external/board/khadas/vim3l/meta(1 hunks)buildroot-external/board/khadas/vim3l/partition-spl-spl.cfg(1 hunks)buildroot-external/board/khadas/vim3l/uboot-boot.ush(1 hunks)buildroot-external/board/khadas/vim3l/uboot.config(1 hunks)buildroot-external/configs/khadas_vim3l_defconfig(1 hunks)buildroot-external/package/khadas-boot/Config.in(1 hunks)buildroot-external/package/khadas-boot/khadas-boot.mk(2 hunks)
✅ Files skipped from review due to trivial changes (4)
- buildroot-external/board/khadas/vim3l/uboot.config
- buildroot-external/board/khadas/vim3l/cmdline.txt
- buildroot-external/board/khadas/vim3l/boot-env.txt
- buildroot-external/board/khadas/vim3l/meta
🔇 Additional comments (18)
buildroot-external/board/khadas/vim3l/partition-spl-spl.cfg (1)
1-7: SPL partition configuration looks good and follows Amlogic boot requirements.The partition configuration correctly defines the SPL (Second Program Loader) at the beginning of the device with appropriate settings:
- Not included in the partition table which is standard for bootloaders
- Starts at offset 0, which is required for Amlogic devices
- Contains a hole at bytes 440-512 which preserves the MBR boot code area
This matches expected configuration for Amlogic S905D3-based devices like the Khadas VIM3L.
buildroot-external/board/khadas/vim3l/image-spl-spl.cfg (2)
1-16: SPL image configuration looks good for Amlogic bootloader.The image configuration correctly defines how to create the SPL image containing the U-Boot bootloader for the Khadas VIM3L. The structure follows expected patterns for Amlogic S905D3 devices.
4-5:❓ Verification inconclusive
Consider investigating if the post-processing is necessary.
The FIXME comment suggests this zeroing operation might be redundant. Since you've kept it for binary-identical output, it's reasonable for now, but you may want to test if removing it causes any issues once the basic implementation is working.
🏁 Script executed:
#!/bin/bash # Description: Check if other Amlogic board configs use similar dd commands echo "Checking for similar dd commands in other board configurations:" rg -l "dd if=/dev/zero" buildroot-external/board/ | grep -v "vim3l"Length of output: 259
Action: Investigate the necessity of the dd post-processing command
The zeroing operation here is currently used only for the Khadas boards and appears to be a board-specific workaround for ensuring binary-identical SPL images. Our repository search shows that aside from the configuration in
vim3l, a similar dd command is found in the relatedvim3configuration. Since there isn’t a broader pattern across other board configurations, it’s worthwhile to test removing this command to see if it impacts functionality or the expected binary output. If testing confirms that it’s redundant, consider cleaning up the configuration accordingly.buildroot-external/board/khadas/vim3l/uboot-boot.ush (4)
1-107: Well-structured boot script with robust A/B boot implementation.This U-Boot boot script implements a solid A/B boot system with retry mechanisms, which is excellent for system reliability. The script has good error handling, clear logging, and follows best practices for U-Boot scripting.
46-57: Good implementation of device tree overlay support.The implementation of DTB overlay support with error handling is excellent. If an overlay fails to apply, the script correctly falls back to the original device tree to ensure the system can still boot.
59-90: Robust A/B boot implementation with retry mechanism.The A/B boot implementation with retry counters is well designed. The script correctly:
- Tries boot slots in the order specified by BOOT_ORDER
- Decrements attempt counters for each failed boot
- Loads the appropriate kernel and sets the correct boot arguments for each slot
This provides excellent reliability for updates and recovery in the Home Assistant OS context.
40-44:✅ Verification successful
Verify the device tree filename is correct for VIM3L.
The script hardcodes the device tree filename to "meson-sm1-khadas-vim3l.dtb". Please verify this is the correct DTB name for the Khadas VIM3L with Amlogic S905D3 processor.
🏁 Script executed:
#!/bin/bash # Description: Check if the DTB file exists in the Linux kernel source # Find the Linux kernel directory KERNEL_DIR=$(find buildroot-external/packages -name "linux-*.tar.gz" -o -name "linux-*.tar.xz" | sort -V | tail -n 1 | xargs dirname) if [ -n "$KERNEL_DIR" ]; then echo "Looking for VIM3L device tree file in kernel source:" find $KERNEL_DIR -name "*vim3l*.dts" -o -name "*khadas*vim3l*.dts" | sort else echo "Linux kernel package not found. Checking for DTB references in other board config files:" rg "meson-sm1-khadas-vim3l" buildroot-external/ fiLength of output: 881
Device Tree Filename Verified
After executing the verification script, we confirmed that the DTB file
"meson-sm1-khadas-vim3l.dtb"is consistently referenced across the board configuration. Both theuboot-boot.ushandhassos-hook.shscripts use this filename, and the defconfig file defines the corresponding DTS name (amlogic/meson-sm1-khadas-vim3l). No changes are required.buildroot-external/board/khadas/vim3l/hassos-hook.sh (1)
15-17: Post-image hook looks good.The
hassos_post_imagefunction properly calls theconvert_disk_image_xzutility to compress the final disk image.buildroot-external/package/khadas-boot/khadas-boot.mk (4)
15-19: The VIM3 configuration block looks good.The block correctly sets
KHADAS_BOOT_FIP_DIRandAML_ENCRYPTvariables for the VIM3 board.
21-26: The new VIM3L configuration looks appropriate.The configuration correctly sets up the VIM3L-specific directories and encryption tool. Good job on maintaining consistency with the existing VIM3 pattern.
28-30: Good implementation of conditional version setting.The use of a conditional to set the version for both VIM3 and VIM3L is a clean approach, ensuring consistent versioning across both board types.
73-107: Great use of the AML_ENCRYPT variable.The build commands now use the
$(AML_ENCRYPT)variable instead of hardcoded values, which is a good practice that accommodates both board types without code duplication.buildroot-external/configs/khadas_vim3l_defconfig (6)
25-25: Correct device tree specification.The Linux kernel device tree name is properly set to
amlogic/meson-sm1-khadas-vim3l, which matches the DTB file copied in the hassos-hook.sh script.
141-142: U-Boot configuration is well-specified.The U-Boot board defconfig is correctly set to
khadas-vim3land includes appropriate configuration fragments.
156-157: Correct Khadas boot package configuration.The configuration correctly enables the Khadas boot package and specifically selects the VIM3L variant, which aligns with the changes made in khadas-boot.mk.
151-151:✅ Verification successful
Boot script source path is appropriate.
The U-Boot boot script source path points to the correct location for the VIM3L board-specific boot script.
🏁 Script executed:
#!/bin/bash # Verify that the uboot-boot.ush exists at the specified path if [ -f "buildroot-external/board/khadas/vim3l/uboot-boot.ush" ]; then echo "✅ uboot-boot.ush file found at the expected location" else echo "❌ uboot-boot.ush file not found at the expected location" fiLength of output: 180
Verification Complete: Boot script source path validated
The verification confirmed that theuboot-boot.ushfile exists atbuildroot-external/board/khadas/vim3l/uboot-boot.ush, matching the expected configuration path. No further modifications are required.
16-16:✅ Verification successful
Configuration looks good, but verify the hassos-hook.sh path.
The post-script args correctly point to the board-specific directory and hook script. This ensures that the Khadas VIM3L-specific build hooks are executed during the build process.
🏁 Script executed:
#!/bin/bash # Verify that the hassos-hook.sh exists at the specified path if [ -f "buildroot-external/board/khadas/vim3l/hassos-hook.sh" ]; then echo "✅ hassos-hook.sh file found at the expected location" else echo "❌ hassos-hook.sh file not found at the expected location" fiLength of output: 180
Configuration Verified: hassos-hook.sh Path is Correct
The file
buildroot-external/board/khadas/vim3l/hassos-hook.shwas confirmed to exist at the expected location. The post-script arguments inbuildroot-external/configs/khadas_vim3l_defconfigcorrectly reference the board-specific directory and hook script, ensuring the intended build hooks will be executed.
- Verified file location:
buildroot-external/board/khadas/vim3l/hassos-hook.shNo further changes are needed.
154-155:❓ Verification inconclusive
Board naming consistency check needed.
The configuration uses both
khadas-vim3l(lowercase with hyphens) andKhadasVim3l(CamelCase) in different settings. This might be intentional based on how different components expect the board name, but it's worth verifying for consistency.Also applies to: 159-159
🏁 Script executed:
#!/bin/bash # Check board naming consistency across config files echo "Checking board naming conventions:" echo "Home Assistant machine name: khadas-vim3l" echo "OS Agent board name: KhadasVim3l" # Check if these naming conventions are used consistently in other configs grep -r "khadas-vim3l\|KhadasVim3l" buildroot-external/package/ buildroot-external/board/ --include="*.mk" --include="*.config" --include="*.sh"Length of output: 540
Board Naming Consistency: Verify OS Agent Naming Convention
The Home Assistant machine name is set to
"khadas-vim3l"in the defconfig (lines 154-155), and our search confirms that board-related files (e.g., inbuildroot-external/board/khadas/vim3l/hassos-hook.sh) use the lowercase hyphenated format. However, the test script’s echo refers to the OS Agent board name as"KhadasVim3l"(CamelCase). Please verify whether the OS Agent component actually requires the CamelCase variant or if the lowercase"khadas-vim3l"should be used instead. Also, ensure that the naming for the corresponding setting on line 159 is consistent.
Signed-off-by: Gunjan Gupta <[email protected]>
Signed-off-by: Gunjan Gupta <[email protected]>
|
Hi @viraniac thanks for your PR! Before we can add a new board there should be an architecture discussion (similar to this discussiion). We are rather busy with maintenance on existing boards, so only if we deem it low effort we can consider it. Finally, we were also start to think about making it easier to add board support via unofficial forks of Home Asisstant OS. This would lower the maintenance effort on our end. |
Sure will start a new discussion page for the same. |
Add Khadas VIM3L board
Summary by CodeRabbit