Prepare 14.0.rc2 #3694
Prepare 14.0.rc2 #3694
Conversation
If HAOS on Yellow is booted for the first time with NVMe data disk present, it should be preferred over the empty eMMC data partition. This will ease reinstall of the system and migration from CM4 to CM5. All other data disks (e.g. if a USB drive is used for them) are still treated as before, requiring manual adoption using the Supervisor repair. (cherry picked from commit 98ac7f0)
Sync the DTS with changes added in newer commits merged after the initial Yellow/CM5 DTS was written. The sdio1 node now has HS400 mode enabled and sd_io_1v8_reg has been changed to regulator-fixed. (cherry picked from commit b288cd2)
📝 WalkthroughWalkthroughThe changes include modifications to several configuration files and scripts for the Raspberry Pi platform. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Script
participant eMMC
participant NVMe
User->>Script: Detect data partitions
Script->>Script: Check VARIANT_ID
alt VARIANT_ID is "yellow"
Script->>eMMC: Identify eMMC data partition
Script->>NVMe: Identify NVMe data partition
alt Both partitions found
Script->>eMMC: Disable eMMC partition
else Only eMMC found
Script->>eMMC: Keep eMMC partition enabled
else Only NVMe found
Script->>NVMe: Keep NVMe partition enabled
end
else VARIANT_ID is not "yellow"
Script->>Script: Disable non-root data partitions
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
🧹 Outside diff range and nitpick comments (2)
buildroot-external/board/raspberrypi/yellow/patches/linux/0017-ARM-dts-bcm2712-yellow-Sync-sdio1-and-sd_io_1v8_reg-.patch (1)
1-50: Consider implementing a comprehensive testing strategy.Given the significant changes to both voltage regulation and SDIO capabilities, consider:
- Documenting the test scenarios that validate these changes
- Creating a test matrix covering different hardware configurations
- Adding regression tests to catch any future compatibility issues
Would you like assistance in creating a test plan template or documentation structure for these hardware changes?
buildroot-external/rootfs-overlay/usr/libexec/haos-data-disk-detach (1)
23-45: Handle potential failures in device enumerationIn the loop over
datapartitions, the script assumes thatdatadevwill always be determined successfully. Consider adding a check to ensure thatdatadevis not empty before proceeding with the case statement to prevent unexpected behavior iflsblkfails.Apply this diff to add the check:
for datapart in ${datapartitions}; do datadev=$(lsblk -no pkname "${datapart}") + if [ -z "${datadev}" ]; then + echo "Unable to determine parent device for ${datapart}, skipping..." + continue + fi case "${datadev}" in mmc*)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
buildroot-external/board/raspberrypi/cmdline.txt(1 hunks)buildroot-external/board/raspberrypi/yellow/patches/linux/0017-ARM-dts-bcm2712-yellow-Sync-sdio1-and-sd_io_1v8_reg-.patch(1 hunks)buildroot-external/kernel/v6.6.y/device-support.config(1 hunks)buildroot-external/kernel/v6.6.y/kernel-arm64-rockchip.config(0 hunks)buildroot-external/meta(1 hunks)buildroot-external/rootfs-overlay/usr/libexec/haos-data-disk-detach(2 hunks)
💤 Files with no reviewable changes (1)
- buildroot-external/kernel/v6.6.y/kernel-arm64-rockchip.config
✅ Files skipped from review due to trivial changes (2)
- buildroot-external/board/raspberrypi/cmdline.txt
- buildroot-external/meta
🔇 Additional comments (6)
buildroot-external/board/raspberrypi/yellow/patches/linux/0017-ARM-dts-bcm2712-yellow-Sync-sdio1-and-sd_io_1v8_reg-.patch (3)
1-17: LGTM! Well-structured patch with clear commit message.
The patch header follows best practices with proper formatting and a clear explanation of the changes.
42-50: Verify signal integrity for new high-speed modes.
The addition of HS400 mode and enhanced strobe support will enable higher performance, but please ensure:
- The PCB design supports these high-speed modes
- Signal integrity has been validated at these speeds
- Proper testing has been done with actual eMMC devices
Consider documenting the maximum supported speeds and any hardware requirements in the documentation.
25-39: Verify the impact of removing voltage switching capability.
The change from a GPIO-controlled regulator to a fixed 1.8V regulator removes the ability to switch to 3.3V. While this aligns with the CM5 DTS, please verify:
- That all connected devices support 1.8V operation
- That removing the 3.3V option won't break compatibility with any existing hardware
✅ Verification successful
Voltage regulator change is intentionally aligned with CM5
The patch clearly shows this is an intentional synchronization with the vanilla CM5 device tree. The changes are part of making the Yellow's DTS consistent with CM5, where SDIO1 is specifically configured for 1.8V operation with enhanced capabilities (hs400-1_8v and enhanced-strobe support). The removal of voltage switching is deliberate as the hardware is designed to operate at 1.8V, not a compatibility concern.
Key evidence from the patch:
- Commit message explicitly states: "SDIO1 and its 1V8 regulator have been changed in vanilla CM5 device tree"
- SDIO1 node shows enhanced 1.8V-specific capabilities being added
- Changes are signed-off by maintainer
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any references to 3.3V operation or voltage switching in documentation and other DTS files
echo "Searching for voltage references in documentation..."
rg -i "3\.3\s*v|3300\s*mv" --type md
echo "Checking other DTS files for similar regulator patterns..."
rg "sd_io_1v8_reg|vdd-sd-io" --type dts -A 5
Length of output: 355
Script:
#!/bin/bash
# Search for voltage references in device tree files with .dts and .dtsi extensions
echo "Searching for voltage references in DTS files..."
fd -e dts -e dtsi -x rg -l "3\.3\s*[vV]|3300000|vdd-sd-io|sd_io_1v8_reg" {}
echo "Checking patch content and context..."
fd -g "*0017-ARM-dts-bcm2712-yellow-Sync-sdio1-and-sd_io_1v8_reg-*" -x cat {}
# Look for related patches that might provide context
echo "Checking for related patches..."
fd -g "*bcm2712*" -g "*.patch" -x grep -l "sd_io|regulator|sdio" {}
Length of output: 2239
buildroot-external/kernel/v6.6.y/device-support.config (1)
16-16: LGTM! The Cypress M8 USB serial driver is properly configured.
The driver is correctly configured as a module, consistent with other USB serial drivers in the configuration.
Let's verify that this driver is enabled across all kernel configurations as mentioned in PR objectives (#3693):
buildroot-external/rootfs-overlay/usr/libexec/haos-data-disk-detach (2)
47-50: Correctly prioritize NVMe data partition over eMMC
The logic correctly disables the eMMC data partition when both eMMC and NVMe data partitions are present on the Yellow device, ensuring that the system uses the faster NVMe data disk as intended.
52-61: Maintain standard data disk handling for other variants
The script preserves the existing behavior for non-Yellow variants by disabling external data disks that do not match the root device, which is appropriate.
| disable_data_partition() { | ||
| e2label "${1}" hassos-data-dis | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure error handling in disable_data_partition function
The disable_data_partition function does not check the exit status of the e2label command. To enhance the robustness of the script, consider adding error handling to ensure that the label change was successful.
Apply this diff to add error checking:
disable_data_partition() {
e2label "${1}" hassos-data-dis
+ if [ $? -ne 0 ]; then
+ echo "Failed to disable data partition ${1}" >&2
+ exit 1
+ fi
}📝 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.
| disable_data_partition() { | |
| e2label "${1}" hassos-data-dis | |
| } | |
| disable_data_partition() { | |
| e2label "${1}" hassos-data-dis | |
| if [ $? -ne 0 ]; then | |
| echo "Failed to disable data partition ${1}" >&2 | |
| exit 1 | |
| fi | |
| } |
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes