Add initial support for Raspberry Pi CM5 on Yellow #3667
Conversation
We will need to use different series for 2024.10 which would be used as the base for CM5 support.
It doesn't seem to be used and common.h has been removed in newer U-Boot.
Use rebased patchset from v2024.01 with the first patch removed. Add patches needed for PCIe initialization and use rpi_arm64_defconfig as the base config for both CM4 and CM5.
Add device tree definition based on the CM5 device tree with BCM2712D0 changes applied, and add nodes required for the on-board peripherals of Yellow. Currently the difference in serial numbering still requires either changes in this device tree, or userspace changes to create correct symlinks to make HA configuration directly compatible with CM4 on Yellow.
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces multiple changes to the U-Boot and Linux configurations for the Raspberry Pi platform, particularly focusing on the Compute Module 4 and 5. Key modifications include the addition of NVMe as a boot target, enhancements to the NVMe driver for better address handling, and the introduction of new reset controllers for the Raspberry Pi 5. Additionally, device tree configurations are updated to support new hardware variants, and the U-Boot configuration is enhanced to improve USB gadget functionalities and overall compatibility. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant U-Boot
participant NVMe Driver
participant Device Tree
User->>U-Boot: Boot system
U-Boot->>Device Tree: Load device tree
Device Tree-->>U-Boot: Provide device configurations
U-Boot->>NVMe Driver: Initialize NVMe
NVMe Driver->>U-Boot: Confirm NVMe readiness
U-Boot->>User: Boot complete
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: 13
🧹 Outside diff range and nitpick comments (24)
buildroot-external/board/raspberrypi/yellow/config.txt (1)
31-37: Consider adding configuration comments for CM4/CM5 sectionsWhile the configuration is functionally correct, it would be helpful to add comments explaining:
- The purpose of separate CM4/CM5 sections
- Any specific requirements or limitations for each module
- The relationship between these sections and the boot process
Here's a suggested improvement:
+# CM4 specific configuration [cm4] device_tree=bcm2711-rpi-cm4-ha-yellow.dtb +# CM5 specific configuration +# Note: Requires updated OS and rpiboot utility for eMMC installations [cm5] device_tree=bcm2712-rpi-cm5-ha-yellow.dtb +# Common configurations for all compute modules [all]buildroot-external/board/raspberrypi/patches/uboot/2024.10/0002-Revert-nvme-Correct-the-prps-per-page-calculation-me.patch (1)
33-34: Correct implementation of NVMe specification requirementsThe modification to subtract 1 from the PRP count (
prps_per_page = (page_size >> 3) - 1) is technically correct and necessary. This ensures that the last entry in each page is properly reserved for the PRP List pointer when handling multi-page transfers, as required by the NVMe specification.This change is particularly important for the CM5's NVMe boot support, as it ensures proper handling of large transfers during the boot process.
When implementing NVMe drivers, always ensure strict adherence to the specification's requirements for PRP list management, as incorrect implementations can lead to data corruption or failed transfers.
buildroot-external/board/raspberrypi/patches/uboot/2024.10/0004-nvme-improve-readability-of-nvme_setup_prps.patch (2)
6-8: Fix typo and enhance commit message clarity
- "discriptive" should be "descriptive"
- Consider expanding the commit message to briefly explain what PRPs (Physical Region Pages) are and their significance in NVMe context
-Improve readability by introducing consts, reuse consts where -appropriate and adding variables with discriptive name. +Improve readability by introducing consts, reuse consts where +appropriate and adding variables with descriptive names. + +PRPs (Physical Region Pages) are used in NVMe for DMA scatter-gather +operations. This patch makes the PRP setup logic more maintainable.
36-42: Good improvement to pointer arithmetic clarityThe introduction of
next_prp_listmakes the PRP list linking more explicit and easier to understand. Consider adding a brief comment explaining the PRP list chaining mechanism for future maintainers.+ /* Chain PRP lists when more pages are needed */ u64 next_prp_list = (u64)prp_pool + page_size; *(prp_pool + i) = cpu_to_le64(next_prp_list);buildroot-external/board/raspberrypi/patches/uboot/2024.10/0007-Revert-pci-Check-region-ranges-are-addressable.patch (1)
23-43: Consider documenting the architectural assumptions.While removing these boundary checks fixes the immediate issue, it would be valuable to document why these checks are problematic specifically for the RPi4 architecture. This will help prevent future regressions if similar checks are attempted.
Consider adding a comment in the code explaining:
- Why standard PC architecture assumptions about PCI address ranges don't apply here
- The specific RPi4 memory layout constraints that make these checks unnecessary
- The relationship with USB/XHCI functionality
if (!IS_ENABLED(CONFIG_SYS_PCI_64BIT) && type == PCI_REGION_MEM && upper_32_bits(pci_addr)) { + /* Note: Standard PCI address range checks are disabled for RPi4 + * as they conflict with its specific memory layout and USB/XHCI + * requirements. The hardware ensures valid address ranges. + */ debug(" - beyond the 32-bit boundary, ignoring\n"); continue; }buildroot-external/board/raspberrypi/patches/uboot/2024.10/0011-ARM-bcm2835-add-BCM2712-config-option.patch (1)
27-31: Consider enhancing the help text for BCM2712 config.The config option is well-structured and correctly placed. However, consider adding a help text to provide more details about the 64-bit requirement and its implications.
config BCM2712 bool "Broadcom BCM2712 SoC support (64-bit only)" depends on ARCH_BCM283X select ARM64 + help + Enable support for Broadcom BCM2712 SoC used in Raspberry Pi 5. + This SoC only supports 64-bit operation mode.buildroot-external/board/raspberrypi/patches/uboot/2024.10/0012-pci-pcie-brcmstb-HACK-add-DT-property-for-skipping-d.patch (2)
10-16: Well-documented temporary workaround, but needs trackingThe commit message clearly explains the problem and the temporary nature of this solution. However, since this is marked as a HACK, we should track this technical debt.
Would you like me to create a GitHub issue to track the proper implementation of RP1 control in U-Boot?
35-43: Consider enhancing the debug messageThe implementation is correct, but the debug message could be more informative.
Consider this enhancement:
- printf("PCIe BRCM: skipping probe\n"); + printf("PCIe BRCM: skipping probe for %s\n", dev->name);This would help identify which PCIe controller is being skipped in the boot logs.
buildroot-external/patches/uboot/0001-CMD-read-string-from-fileinto-env.patch (5)
Line range hint
1689-1689: Enhance configuration help text with security considerationsThe help text should provide more details about security implications of reading files into environment variables, including potential buffer overflow risks and the importance of validating input files.
config CMD_FILEENV bool "fileenv" help - Read a file into memory and store it to env. + Read a file into memory and store it to env. Care should be taken when + reading untrusted files as they could contain malicious content or + exceed available memory. Only use with trusted files from verified sources.Also applies to: 1694-1694
Line range hint
53-53: Improve error handling and input validationThe command needs better error handling and input validation:
- No validation of the address range
- No validation of the interface and device parameters
- The return value
1is inconsistent with U-Boot'sCMD_RET_FAILUREint do_fileenv(struct cmd_tbl *cmdtp, int flag, int argc, char * const argv[]) { + ulong addr; + char *endp; + if (argc < 6) return CMD_RET_USAGE; + addr = simple_strtoul(argv[3], &endp, 16); + if (*endp != '\0' || addr == 0) { + printf("Invalid address: %s\n", argv[3]); + return CMD_RET_FAILURE; + } + fs_argv[0] = "fatload"; fs_argv[1] = argv[1]; fs_argv[2] = argv[2]; - fs_argv[3] = argv[3]; + fs_argv[3] = (char *)addr; fs_argv[4] = argv[4]; if (do_fat_fsload(cmdtp, 0, 5, fs_argv) != 0) - return 1; + return CMD_RET_FAILURE;Also applies to: 67-67
Line range hint
69-69: Fix critical memory safety issuesThe current implementation has several memory safety issues:
- No size validation before writing null terminator
- No bounds checking during string sanitization
- No maximum file size limit
- char *addr = (char *)simple_strtoul(argv[3], NULL, 16); + char *addr = (char *)addr; size_t size = env_get_hex("filesize", 0); + if (size > CONFIG_SYS_MALLOC_LEN) { + printf("File too large: %zu bytes\n", size); + return CMD_RET_FAILURE; + } + // Prepare string - addr[size] = 0x00; + if (size > 0) { + addr[size - 1] = '\0'; + } else { + printf("Empty file\n"); + return CMD_RET_FAILURE; + } + char *s = addr; - while(*s != 0x00) { + char *end = addr + size - 1; + while(s < end && *s != '\0') { if (isprint(*s)) { s++; - } - else { - *s = 0x00; + } else { + *s = '\0'; + break; } }Also applies to: 85-85
Line range hint
87-87: Add error handling for env_setThe return value from
env_setshould be properly handled and converted to a U-Boot command return code.- return env_set(argv[5], addr); + return env_set(argv[5], addr) ? CMD_RET_SUCCESS : CMD_RET_FAILURE;Also applies to: 87-87
Line range hint
89-89: Enhance command documentationThe command help text should include:
- Maximum file size limitations
- Security considerations
- Example usage
U_BOOT_CMD( fileenv, 6, 0, do_fileenv, "Read file and store it into env.", - "<interface> <dev:part> <addr> <filename> <envname>\n" - " - Read file from fat32 and store it as env." + "<interface> <dev:part> <addr> <filename> <envname>\n" + " - Read file from FAT32 and store it as env.\n" + " - File must be text and smaller than " __stringify(CONFIG_SYS_MALLOC_LEN) " bytes\n" + " - Only printable ASCII characters are preserved\n" + "Example:\n" + " fileenv mmc 0:1 ${loadaddr} config.txt bootargs" );Also applies to: 95-95
buildroot-external/ota/rauc-hook (1)
94-94: Document the removal timeline for temporary codeThe comment indicates this is temporary for HAOS 15, but doesn't specify when or under what conditions it can be removed.
Consider updating the comment to be more specific:
-# Add CM5 support for Yellow. Can be removed in HAOS 15. +# TODO(HAOS-XXX): Temporary CM5 support for Yellow. Can be removed after HAOS 15 release when all systems have been updated to support both CM4 and CM5.buildroot-external/configs/yellow_defconfig (1)
146-147: LGTM: U-Boot configuration aligns with multi-platform support strategyThe changes demonstrate good architectural decisions:
- Upgrading to U-Boot 2024.10 brings in basic BCM2712 support
- Switching to
rpi_arm64board config provides a unified target for both CM4 and CM5This configuration change sets up a good foundation for:
- Supporting future Raspberry Pi ARM64 modules
- Maintaining a single U-Boot configuration for all supported modules
- Reducing maintenance overhead compared to separate configs
buildroot-external/board/raspberrypi/patches/uboot/2024.10/0008-reset-reset-brcmstb-Add-Broadcom-STB-reset-controlle.patch (2)
70-70: Remove unnecessary header<malloc.h>The code does not use any functions or definitions from
<malloc.h>, so this include can be removed to clean up the code.Apply this diff to remove the unnecessary include:
-#include <malloc.h>
67-75: Review included headers and remove unused onesSome of the included headers may not be necessary if their functions or definitions are not used. For example, consider removing
<malloc.h>and<log.h>if they are not required.Please ensure that all included headers are essential for this driver.
buildroot-external/board/raspberrypi/patches/uboot/2024.10/0009-reset-reset-brcmstb-rescal-Add-Broadcom-RESCAL-reset.patch (3)
85-88: Add explanatory comment for emptyreset_assertfunctionThe
brcm_rescal_reset_assertfunction currently performs no action. Adding a comment clarifies that this is intentional and improves code readability.Apply this diff:
+/* No action required on assert for RESCAL reset */ static int brcm_rescal_reset_assert(struct reset_ctl *rst) { return 0; }
100-101: Usedev_errinstead ofprintffor error messagesUsing
dev_errprovides better context by including device-specific information in error messages and aligns with U-Boot's logging practices.Apply this diff to enhance error handling:
- printf("failed to start SATA/PCIe rescal\n"); + dev_err(rst->dev, "Failed to start SATA/PCIe RESCAL\n"); ... - printf("time out on SATA/PCIe rescal\n"); + dev_err(rst->dev, "Timeout on SATA/PCIe RESCAL\n");Also applies to: 107-108
122-123: Usedev_errinstead ofprintffor error messages inof_xlateConsistently using
dev_errimproves error reporting across the driver.Apply this diff:
- printf("Invalid args_count: %d\n", args->args_count); + dev_err(reset_ctl->dev, "Invalid args_count: %d\n", args->args_count);buildroot-external/board/raspberrypi/patches/uboot/2024.10/0006-nvme-translate-virtual-addresses-into-the-bus-s-addr.patch (1)
13-16: Clarify the commit message for better coherenceThe sentence starting at line 15 seems incomplete:
"Introduce nvme_virt_to_bus() and nvme_bus_to_virt() to cater with these limitations, and make sure we don't break non DM users. For devices where PCIe's view of host memory doesn't match the memory as seen by the CPU."
Consider revising it for clarity. Perhaps merge the sentences or adjust the punctuation to improve readability.
buildroot-external/board/raspberrypi/patches/uboot/2024.10/0010-pci-pcie-brcmstb-Add-basic-support-for-BCM2712-PCIe.patch (3)
265-275: Replace magic numbers with defined constantsThe use of magic numbers like
0x12can reduce code readability and maintainability. Defining them as constants with meaningful names enhances clarity.Define a constant for the PM clock period:
+#define PM_CLK_PERIOD_18_52NS 0x12 /* Fix for L1SS errata */ tmp = readl(base + PCIE_RC_PL_PHY_CTL_15); tmp &= ~PCIE_RC_PL_PHY_CTL_15_PM_CLK_PERIOD_MASK; /* PM clock period is 18.52ns (rounded down) */ - tmp |= 0x12; + tmp |= PM_CLK_PERIOD_18_52NS; writel(tmp, base + PCIE_RC_PL_PHY_CTL_15);This makes the code self-documenting and easier to understand.
160-172: Provide context for hard-coded register valuesThe arrays
regsanddatacontain hard-coded values without explanation. Adding comments or defining constants for these magic numbers improves maintainability and aids future developers.Add comments to explain the purpose of the registers and data:
u8 regs[] = { 0x16, 0x17, 0x18, 0x19, 0x1b, 0x1c, 0x1e }; u16 data[] = { 0x50b9, 0xbda1, 0x0094, 0x97b4, 0x5030, 0x5030, 0x0007 }; + /* The 'regs' array contains SerDes register addresses required for PLL adjustment */ + /* The 'data' array contains corresponding values to be written to these registers */ brcm_pcie_mdio_write(pcie->base, MDIO_PORT0, SET_ADDR_OFFSET, 0x1600); for (i = 0; i < ARRAY_SIZE(regs); i++) { brcm_pcie_mdio_read(pcie->base, MDIO_PORT0, regs[i], &tmp); }Alternatively, define constants with descriptive names if possible.
266-267: Use professional language in commentsThe comment mentions "Hack from RPi downstream," which is informal. For professional codebases, it's better to use formal language to describe the workaround.
Rephrase the comment for clarity:
- /* Hack from RPi downstream, unable to probe without it */ + /* Workaround from Raspberry Pi downstream to enable probing */This enhances professionalism and readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (18)
buildroot-external/board/raspberrypi/patches/uboot/2024.10/0001-rpi-add-NVMe-to-boot-order.patch(1 hunks)buildroot-external/board/raspberrypi/patches/uboot/2024.10/0002-Revert-nvme-Correct-the-prps-per-page-calculation-me.patch(1 hunks)buildroot-external/board/raspberrypi/patches/uboot/2024.10/0003-usb-xhci-brcm-Make-driver-compatible-with-downstream.patch(1 hunks)buildroot-external/board/raspberrypi/patches/uboot/2024.10/0004-nvme-improve-readability-of-nvme_setup_prps.patch(1 hunks)buildroot-external/board/raspberrypi/patches/uboot/2024.10/0005-nvme-Use-pointer-for-CPU-addressed-buffers.patch(1 hunks)buildroot-external/board/raspberrypi/patches/uboot/2024.10/0006-nvme-translate-virtual-addresses-into-the-bus-s-addr.patch(1 hunks)buildroot-external/board/raspberrypi/patches/uboot/2024.10/0007-Revert-pci-Check-region-ranges-are-addressable.patch(1 hunks)buildroot-external/board/raspberrypi/patches/uboot/2024.10/0008-reset-reset-brcmstb-Add-Broadcom-STB-reset-controlle.patch(1 hunks)buildroot-external/board/raspberrypi/patches/uboot/2024.10/0009-reset-reset-brcmstb-rescal-Add-Broadcom-RESCAL-reset.patch(1 hunks)buildroot-external/board/raspberrypi/patches/uboot/2024.10/0010-pci-pcie-brcmstb-Add-basic-support-for-BCM2712-PCIe.patch(1 hunks)buildroot-external/board/raspberrypi/patches/uboot/2024.10/0011-ARM-bcm2835-add-BCM2712-config-option.patch(1 hunks)buildroot-external/board/raspberrypi/patches/uboot/2024.10/0012-pci-pcie-brcmstb-HACK-add-DT-property-for-skipping-d.patch(1 hunks)buildroot-external/board/raspberrypi/yellow/config.txt(1 hunks)buildroot-external/board/raspberrypi/yellow/patches/linux/0016-ARM-dts-bcm2712-Add-device-tree-for-CM5-on-HA-Yellow.patch(1 hunks)buildroot-external/board/raspberrypi/yellow/uboot.config(1 hunks)buildroot-external/configs/yellow_defconfig(2 hunks)buildroot-external/ota/rauc-hook(2 hunks)buildroot-external/patches/uboot/0001-CMD-read-string-from-fileinto-env.patch(3 hunks)
🔇 Additional comments (28)
buildroot-external/board/raspberrypi/yellow/uboot.config (5)
21-22: LGTM: EFI loader correctly disabled
The explicit disabling of EFI loader and tools is appropriate for the Raspberry Pi boot process.
4-4: Verify NVMe boot configuration
While SDHCI support is correctly enabled for BCM2712, the configuration seems to be missing explicit NVMe boot support mentioned in the PR objectives.
#!/bin/bash
# Check for NVMe-related configurations in other U-Boot config files
rg "CONFIG_.*NVME|CONFIG_.*PCIE" -g "uboot.config"
# Look for NVMe driver configurations
rg "CONFIG_.*NVME|CONFIG_.*PCIE" -g "Kconfig*"15-19: Verify USB mass storage and LED functionality
Given the PR objectives mention:
- USB functionality issues affecting USB ports
- Non-functional factory reset due to GPIO handling
Please verify if:
- The USB mass storage command will work despite the USB issues
- The LED GPIO configuration is correct for the new hardware
#!/bin/bash
# Look for GPIO pin definitions for LEDs
rg "gpio-led|GPIO_LED" -g "*.dts"
# Check for USB mass storage implementations
rg "usb_mass_storage|do_usb_mass_storage" -g "*.c"6-13:
Review USB gadget configuration concerns
- The PR objectives mention USB functionality issues, yet extensive USB gadget support is being enabled. Please clarify if these configurations will work as intended.
- The manufacturer string "FSL" appears incorrect for a Raspberry Pi device.
- The vendor/product IDs need verification.
Consider these changes:
-CONFIG_USB_GADGET_MANUFACTURER="FSL"
+CONFIG_USB_GADGET_MANUFACTURER="Raspberry Pi"#!/bin/bash
# Check for USB vendor/product IDs in other Raspberry Pi configs
rg "CONFIG_USB_GADGET_(VENDOR|PRODUCT)_NUM" -g "*rpi*"
# Look for manufacturer strings in other configs
rg "CONFIG_USB_GADGET_MANUFACTURER" -g "*rpi*"1-2: LGTM: Target configuration enables both CM4 and CM5 support
The configuration correctly enables both the RPi4 target and BCM2712 SoC support, which is necessary for supporting both CM4 and CM5 modules on Yellow.
buildroot-external/board/raspberrypi/patches/uboot/2024.10/0001-rpi-add-NVMe-to-boot-order.patch (2)
22-23: Boot order change looks good but needs testing
The addition of 'nvme' as a boot target aligns with the PR objectives to support PCIe/NVMe on the CM5. The ordering (mmc → nvme → usb → pxe → dhcp) is logical, maintaining backward compatibility while adding NVMe support.
However, given the PR objectives mention some limitations:
- Factory reset capabilities in U-Boot are non-functional due to GPIO handling
- USB functionality has issues
Please ensure thorough testing of the boot sequence, particularly:
- Successful booting from NVMe devices
- Fallback behavior when NVMe boot fails
- Interaction with both CM4 and CM5 hardware
#!/bin/bash
# Description: Look for related NVMe configurations and potential issues
# Check for NVMe-related configurations
rg -A 5 "CONFIG_.*NVME.*"
# Look for any known issues or TODOs related to NVMe boot
rg -i "todo.*nvme|fixme.*nvme|bug.*nvme" 1-13: Verify patch applicability to U-Boot 2024.10
This patch dates from December 2020 but is being applied to U-Boot 2024.10. While the change itself is straightforward, we should verify it still applies cleanly and remains relevant to the current U-Boot codebase.
✅ Verification successful
Patch is valid and properly supported for U-Boot 2024.10
The patch is well-structured and remains relevant for U-Boot 2024.10:
- NVMe support is properly configured in U-Boot (CONFIG_NVME=y, CONFIG_NVME_PCI=y, CONFIG_CMD_NVME=y)
- The boot order change is consistent with the hardware capabilities of CM4/CM5
- Similar patch was also applied to U-Boot 2024.01, showing consistent maintenance
- The modification is minimal and focused, only adding 'nvme' to the boot targets list
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the patch applies cleanly and if similar changes exist upstream
# Check if the file exists in current U-Boot
fd rpi.env$ -t f
# Check if similar changes exist upstream
rg "boot_targets.*nvme" -A 5 -B 5
Length of output: 6597
Script:
#!/bin/bash
# Let's check the actual patch content and verify U-Boot configs
# First, check the full patch content
cat buildroot-external/board/raspberrypi/patches/uboot/2024.10/0001-rpi-add-NVMe-to-boot-order.patch
# Check if NVMe support is properly configured in U-Boot for RPi
cat buildroot-external/board/raspberrypi/uboot.config | grep -i "nvme"
# Check for any other boot order related configurations
rg "boot_targets.*=.*" -A 2 -B 2
Length of output: 2188
buildroot-external/board/raspberrypi/yellow/config.txt (4)
34-35: Device tree naming convention follows CM4 pattern
The device tree naming for CM5 (bcm2712-rpi-cm5-ha-yellow.dtb) follows the same pattern as CM4 (bcm2711-rpi-cm4-ha-yellow.dtb), which is good for consistency. The BCM2712 chip designation aligns with the PR objectives mentioning the new chip.
37-37: Clarify the purpose of the empty [all] section
The empty [all] section at the end of the file needs clarification. If it's intended for future common configurations, consider adding a comment. If it's not needed, it should be removed.
#!/bin/bash
# Description: Check if empty [all] sections are common in other Raspberry Pi configurations
# and if there's any documentation explaining their purpose
echo "Checking for similar empty [all] sections in other config files..."
rg -A 1 "^\[all\]$" --type txt --type conf
echo "Searching for documentation about [all] section usage..."
rg -l "\[all\].*section" --type md --type txtLine range hint 1-37: Verify PCIe/NVMe boot configuration
The PR objectives mention PCIe/NVMe support for BCM2712, but there are no explicit NVMe-related configurations in this file. We should verify if additional configurations are needed.
#!/bin/bash
# Description: Check for NVMe-related configurations in other files and documentation
echo "Searching for NVMe-related configurations in other Raspberry Pi config files..."
rg -l "nvme|pcie" --type txt --type conf
echo "Checking U-Boot configurations for NVMe support..."
rg -l "nvme|pcie" --type=h --type=c --type=dts
echo "Looking for documentation about NVMe boot configuration..."
rg -l "nvme.*boot|pcie.*boot" --type md --type txt31-32: Verify the impact of renaming [all] to [cm4]
The section has been renamed from [all] to [cm4] to be more specific. While this change makes the configuration clearer, we should verify that this doesn't affect existing CM4 installations.
✅ Verification successful
Renaming [all] to [cm4] is safe and appropriate
The change from [all] to [cm4] in the Yellow board's config.txt is correct and safe because:
- The repository contains multiple config.txt files, each serving different boards
- The Yellow board's config.txt specifically handles CM4 and CM5 configurations separately
- The [all] section is commonly used across config.txt files for general settings, but in this case, the device tree specification needs to be board-specific
- No other configuration files or scripts directly reference or depend on the [all] section from the Yellow board's config.txt
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there are any references to the old [all] section in other configuration files
# or documentation that might need updating
echo "Searching for references to [all] section in config files and documentation..."
rg -l "\[all\]" --type txt --type md --type conf
echo "Checking for any conditional logic in boot scripts that might depend on [all] section..."
rg -l "config.txt.*\[all\]" --type sh --type py
Length of output: 608
Script:
#!/bin/bash
# Let's try a different approach to find config.txt files and their sections
# First, let's find all config.txt files in the repository
echo "Finding all config.txt files..."
fd "config.txt$"
# Then let's examine their contents
echo -e "\nChecking contents of config.txt files..."
fd "config.txt$" -x cat {}
# Let's also check if there are any documentation files mentioning config.txt sections
echo -e "\nChecking documentation for config.txt section references..."
rg -i "config.txt.*section|config.txt.*\[.*\]" --type md
Length of output: 5138
buildroot-external/board/raspberrypi/patches/uboot/2024.10/0003-usb-xhci-brcm-Make-driver-compatible-with-downstream.patch (2)
1-14: LGTM! Well-documented patch metadata.
The commit message clearly explains the purpose of the change and its relationship to downstream kernel compatibility.
15-27: Verify compatibility with both CM4 and CM5 device trees.
The change to simplify the compatible string from "brcm,generic-xhci" to "generic-xhci" aligns with downstream Linux kernel expectations. However, given the PR's goal to support both CM4 and CM5, we should verify this works with both platforms.
✅ Verification successful
From analyzing the output, I can see that both CM4 and CM5 device trees are present in the codebase, particularly in the Yellow board configuration which supports both platforms. The search results show that USB host mode is enabled by default and the XHCI driver is used, but I need to specifically verify the XHCI compatible string usage in both CM4 and CM5 device trees.
Based on the analysis of the device trees for both CM4 and CM5 on Yellow board, I can now provide a definitive response about the XHCI compatibility:
The change to use "generic-xhci" is compatible with both CM4 and CM5 platforms.
The device trees show that both platforms use the same USB controller configuration:
- CM4 (bcm2711): Uses
brcm,bcm2835-usbwith host mode - CM5 (bcm2712): Uses the same
brcm,bcm2835-usbwith host mode
The XHCI driver's compatible string change from "brcm,generic-xhci" to "generic-xhci" is safe because:
- Both platforms use the same underlying USB controller architecture
- The device trees don't specify any XHCI-specific compatible strings, relying on the generic USB host controller setup
- The change aligns with the downstream kernel's expectations without breaking either platform's functionality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the XHCI compatible string usage across CM4 and CM5 device trees
# Expected: Both CM4 and CM5 device trees should use "generic-xhci" for XHCI nodes
# Search for XHCI nodes in device tree source files
echo "Checking device tree sources for XHCI nodes..."
rg -A 5 "xhci|usb.*controller" --type dts
# Check for any other potential XHCI compatible strings that might conflict
echo "Checking for other XHCI compatible strings..."
rg "compatible.*xhci" --type dts
Length of output: 338
Script:
#!/bin/bash
# Let's try a different approach to find device tree files and XHCI nodes
# First, find all .dts and .dtsi files
echo "Finding all device tree files..."
fd "\.(dts|dtsi)$"
# Then search for XHCI/USB controller related content in these files
echo -e "\nSearching for XHCI/USB controller nodes..."
fd "\.(dts|dtsi)$" --exec grep -A 5 -i "xhci\|usb.*controller" {} \;
# Search for compatible strings related to XHCI
echo -e "\nSearching for XHCI compatible strings..."
fd "\.(dts|dtsi)$" --exec grep -i "compatible.*xhci" {} \;
Length of output: 431
Script:
#!/bin/bash
# Let's check the patch directory for device tree files and related patches
echo "Checking patch directory for device tree files and patches..."
fd . buildroot-external/board/raspberrypi/
echo -e "\nChecking content of device tree related files and patches..."
fd . buildroot-external/board/raspberrypi/ --exec grep -l -i "xhci\|usb.*controller\|dtb\|dts" {} \;
echo -e "\nChecking specific content of any found files..."
fd . buildroot-external/board/raspberrypi/ --exec cat {} \; | grep -A 5 -i "xhci\|usb.*controller\|compatible"
Length of output: 29214
Script:
#!/bin/bash
# Let's specifically check the device trees for CM4 and CM5 on Yellow board
echo "Checking CM4 and CM5 device trees for XHCI nodes..."
fd . buildroot-external/board/raspberrypi/yellow/patches/linux/ --exec cat {} \; | grep -B 5 -A 5 "xhci\|usb.*controller"
# Also check the kernel config to ensure XHCI support is enabled for both
echo -e "\nChecking kernel config for XHCI support..."
cat buildroot-external/board/raspberrypi/kernel.config | grep -i "xhci"
Length of output: 412
Script:
#!/bin/bash
# Let's check the content of the specific device tree patches for CM4 and CM5
echo "Checking CM4 device tree patch..."
cat buildroot-external/board/raspberrypi/yellow/patches/linux/0001-ARM-dts-bcm2711-Add-device-tree-for-Home-Assistant-Y.patch
echo -e "\nChecking CM5 device tree patch..."
cat buildroot-external/board/raspberrypi/yellow/patches/linux/0016-ARM-dts-bcm2712-Add-device-tree-for-CM5-on-HA-Yellow.patch
# Also check the downstream kernel patches for any XHCI related changes
echo -e "\nChecking downstream kernel patches..."
cat buildroot-external/board/raspberrypi/patches/linux/0001-ARM-dts-bcm283x-add-compatible-picked-up-by-U-Boot.patch
Length of output: 40714
buildroot-external/board/raspberrypi/patches/uboot/2024.10/0002-Revert-nvme-Correct-the-prps-per-page-calculation-me.patch (1)
8-17: Well-documented rationale for the revert!
The commit message excellently explains the technical necessity for this change by referencing the NVM Express specification. It clearly states why the last entry within a memory page must be reserved as a PRP List pointer when multiple pages are involved in data transfer.
buildroot-external/board/raspberrypi/patches/uboot/2024.10/0004-nvme-improve-readability-of-nvme_setup_prps.patch (1)
22-30: Good improvements to variable declarations
The changes enhance code readability and maintainability by:
- Making constants explicit with
constkeyword - Moving the
prps_per_pagecalculation closer to related declarations - Maintaining clear variable naming
buildroot-external/board/raspberrypi/patches/uboot/2024.10/0007-Revert-pci-Check-region-ranges-are-addressable.patch (1)
8-10: LGTM - Revert appears necessary for USB functionality.
The removal of these PCI region range checks is justified given the boot hangs with USB/XHCI on RPi4 32-bit mode. The original checks were likely too restrictive for RPi4's specific architecture and memory layout.
Let's verify the USB functionality after this revert:
✅ Verification successful
Revert patch is justified and necessary for USB functionality
The patches provide clear evidence that:
- The XHCI driver patch (0003) ensures compatibility with downstream device tree by using "generic-xhci" compatible string
- The revert patch (0007) removes overly restrictive PCI region checks that were causing USB/XHCI boot hangs specifically on RPi4 32-bit
- The removed checks were causing false positives for valid memory ranges on RPi4's architecture
The revert is well-targeted to fix a specific issue while maintaining basic 32-bit boundary checks. The security impact is minimal since:
- Basic 32-bit PCI address boundary check is preserved
- RPi4 has a fixed hardware configuration
- The issue only affects 32-bit mode where address space is already constrained
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any related USB/XHCI issues or fixes in the codebase
# Look for related USB/XHCI patches or known issues
# Search for related USB/XHCI patches
fd -e patch -e diff -x rg -l 'usb|xhci|brcm' 'buildroot-external/board/raspberrypi/patches/uboot/2024.10/'
# Search for known USB issues in documentation
rg -i 'usb|xhci' README.md docs/
Length of output: 97570
Script:
#!/bin/bash
# Let's examine the USB/XHCI related patches more closely
# First, check the content of the USB XHCI driver patch
cat buildroot-external/board/raspberrypi/patches/uboot/2024.10/0003-usb-xhci-brcm-Make-driver-compatible-with-downstream.patch
# Then check the content of the PCI region check revert patch
cat buildroot-external/board/raspberrypi/patches/uboot/2024.10/0007-Revert-pci-Check-region-ranges-are-addressable.patch
Length of output: 2657
buildroot-external/board/raspberrypi/patches/uboot/2024.10/0011-ARM-bcm2835-add-BCM2712-config-option.patch (2)
1-18: LGTM! Well-structured commit message.
The commit message clearly describes the changes and their purpose, following good commit message practices with a proper sign-off.
43-44: Verify the existence of required reset controller configs.
The changes correctly tie the reset controllers to BCM2712, but let's verify that these configs are properly defined elsewhere.
✅ Verification successful
Reset controller configs are properly defined in the patch series
Both RESET_BRCMSTB and RESET_BRCMSTB_RESCAL configs are correctly defined in the patch series:
RESET_BRCMSTBis introduced in patch 0008 as a Broadcom STB reset controllerRESET_BRCMSTB_RESCALis introduced in patch 0009 as a Broadcom STB RESCAL reset controller specifically for BCM2712
The dependencies are properly ordered in the patch series, with the base reset controller added first, followed by the RESCAL controller, and finally the BCM2712-specific selections.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the required reset controller configs exist in Kconfig files
# Search for RESET_BRCMSTB config
echo "Searching for RESET_BRCMSTB config definition:"
rg -l 'config RESET_BRCMSTB\b' --type kconfig
# Search for RESET_BRCMSTB_RESCAL config
echo "Searching for RESET_BRCMSTB_RESCAL config definition:"
rg -l 'config RESET_BRCMSTB_RESCAL\b' --type kconfig
Length of output: 386
Script:
#!/bin/bash
# Description: Verify that the required reset controller configs exist in Kconfig files
# Using ripgrep without file type filter since Kconfig isn't recognized
# Search for RESET_BRCMSTB config
echo "Searching for RESET_BRCMSTB config definition:"
rg 'config RESET_BRCMSTB\b' -A 3
# Search for RESET_BRCMSTB_RESCAL config
echo "Searching for RESET_BRCMSTB_RESCAL config definition:"
rg 'config RESET_BRCMSTB_RESCAL\b' -A 3
# Also search in patch files since this might be part of the changes
echo "Searching in patch files:"
fd -e patch -x rg -l 'config RESET_BRCMSTB' {}
fd -e patch -x rg -l 'config RESET_BRCMSTB_RESCAL' {}
Length of output: 2788
buildroot-external/board/raspberrypi/patches/uboot/2024.10/0012-pci-pcie-brcmstb-HACK-add-DT-property-for-skipping-d.patch (3)
27-32: LGTM: Clean struct extension
The no_probe boolean is appropriately added to the brcm_pcie struct, maintaining consistent style with existing boolean members like ssc.
1-54: Consider potential race conditions with Linux initialization
While this workaround solves the immediate issue with RP1 initialization, we should verify there are no timing-sensitive dependencies that could be affected by skipping the probe.
#!/bin/bash
# Look for any timing-sensitive initialization code in the Linux driver
rg -A 10 "brcm,rp1" arch/arm/boot/dts/
rg -A 10 "pcie.*init" drivers/pci/controller/pcie-brcmstb.c47-52: Verify the device tree binding documentation
The implementation correctly reads the property, but we should ensure this new property is documented.
buildroot-external/patches/uboot/0001-CMD-read-string-from-fileinto-env.patch (1)
Line range hint 164-164: LGTM!
The Makefile integration follows the established pattern and is correctly placed.
Also applies to: 164-164
buildroot-external/ota/rauc-hook (2)
147-149: LGTM: Proper integration of post_install_boot
The integration of the new function in the slot-post-install hook is correct and well-placed.
105-105: Verify device tree files and config.txt compatibility
Let's verify that the required device tree files exist and the config.txt modifications are compatible with both CM4 and CM5.
✅ Verification successful
Device tree files and config.txt modifications are properly configured
The verification shows that:
- Both device tree files are properly defined in the kernel configuration (
buildroot-external/configs/yellow_defconfig) - The device tree files are implemented in the codebase with appropriate patches
- The
config.txtinbuildroot-external/board/raspberrypi/yellow/config.txtcorrectly implements the CM4/CM5 sections with their respective device trees - The modifications in the rauc-hook script align with the existing boot configuration structure
The changes are safe and maintain compatibility with both CM4 and CM5 hardware.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify device tree files and config.txt modifications
# Check if both device tree files exist
echo "Checking device tree files..."
fd "bcm2711-rpi-cm4-ha-yellow.dtb|bcm2712-rpi-cm5-ha-yellow.dtb"
# Check config.txt patterns in the codebase
echo "Checking config.txt patterns..."
rg "\[cm[45]\]|device_tree=bcm27[12][12]-rpi-cm[45]-ha-yellow.dtb" -g "config.txt"
Length of output: 669
Script:
#!/bin/bash
# Let's verify the device tree files in the kernel config and build files
echo "Checking kernel configs and build files for device trees..."
rg "bcm27[12][12]-rpi-cm[45]-ha-yellow" -A 2 -B 2
# Check the full config.txt content to understand the context
echo "Checking full config.txt content..."
cat buildroot-external/board/raspberrypi/yellow/config.txt
# Verify if there are any other related boot configurations
echo "Checking for other boot-related configurations..."
rg "bootloader|cmdline|config.txt" buildroot-external/board/raspberrypi/yellow/
Length of output: 36932
buildroot-external/configs/yellow_defconfig (1)
25-25: LGTM: Device tree configuration supports both CM4 and CM5
The configuration correctly maintains support for both CM4 and CM5 modules, allowing for backward compatibility while adding new hardware support.
Let's verify the device tree files are properly referenced:
✅ Verification successful
Device tree configuration is properly implemented for both CM4 and CM5
The search results confirm that:
- Both device trees are properly referenced in the kernel build system through patches
- The CM4 DTS is located at
arch/arm/boot/dts/broadcom/bcm2711-rpi-cm4-ha-yellow.dts - The CM5 DTS is located at
arch/arm64/boot/dts/broadcom/bcm2712-rpi-cm5-ha-yellow.dts - The configuration is correctly used in:
buildroot-external/configs/yellow_defconfigbuildroot-external/board/raspberrypi/yellow/config.txtbuildroot-external/ota/rauc-hookfor handling updates
The implementation maintains proper compatibility between CM4 and CM5 modules while following the correct architecture-specific paths (arm vs arm64).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify DTS files exist in kernel source
# Note: Using fd to find files as paths may vary
# Check for CM4 and CM5 device tree sources
fd -e dts "bcm2711-rpi-cm4-ha-yellow.dts$|bcm2712-rpi-cm5-ha-yellow.dts$"
Length of output: 75
Script:
#!/bin/bash
# Let's try a broader search to understand the DTS structure
# First, check for any DTS files in the repository
fd -e dts
# Then check for any files containing CM4/CM5 references
rg -i "cm[45]" --type dts
Length of output: 67
Script:
#!/bin/bash
# Let's try a broader search without file type restrictions
# Search for any files containing CM4/CM5 references
rg -i "cm[45]"
# Also check if we have any references to these DTS files in other configuration files
rg "bcm27(11|12)-rpi-cm[45]-ha-yellow"
Length of output: 37544
buildroot-external/board/raspberrypi/patches/uboot/2024.10/0006-nvme-translate-virtual-addresses-into-the-bus-s-addr.patch (3)
165-169: Verify the availability of dev_phys_to_bus() across all platforms
The function nvme_virt_to_bus() relies on dev_phys_to_bus():
static inline dma_addr_t nvme_virt_to_bus(struct nvme_dev *dev, void *addr)
{
return dev_phys_to_bus(nvme_to_dev(dev)->parent, virt_to_phys(addr));
}Ensure that dev_phys_to_bus() is available and correctly implemented on all target platforms. On some architectures, this function may not be defined, potentially leading to compilation errors or runtime issues.
Run the following script to check for the definition of dev_phys_to_bus() in the codebase:
#!/bin/bash
# Description: Verify that 'dev_phys_to_bus()' is defined for all architectures
# Search for the definition of 'dev_phys_to_bus' in header files
rg -A 2 "dev_phys_to_bus\s*\(" include/ arch/ drivers/
# List all architecture-specific implementations
fd -e c -e h "dev_phys_to_bus" arch/ | xargs rg "dev_phys_to_bus"
# Check for any conditional compilation flags that might exclude 'dev_phys_to_bus'
rg "ifndef" -A 2 include/phys2bus.hThis script helps identify whether dev_phys_to_bus() is consistently defined and available across different platforms and configurations.
154-154: Confirm inclusion of necessary headers
You included <phys2bus.h>:
#include <phys2bus.h>Ensure that this header file is present in all build environments and does not cause issues on platforms where it might be absent.
Run the following script to verify the existence and usage of phys2bus.h:
This ensures that including <phys2bus.h> will not break the build on certain configurations.
✅ Verification successful
Header inclusion is appropriate and necessary
The inclusion of <phys2bus.h> is correct and necessary for this patch. The patch implements address translation for NVMe controllers on platforms where PCIe's view of host memory doesn't match the CPU's view, specifically targeting Raspberry Pi 4. The header is used to implement nvme_virt_to_bus() which uses dev_phys_to_bus() from the phys2bus API.
Key points:
- The patch is part of U-Boot's Raspberry Pi support
- The functionality is similar to an existing XHCI controller implementation (referenced in commit message)
- The header is used in conjunction with
<asm/io.h>for address translation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for the presence of 'phys2bus.h' in the codebase
# Find the location of 'phys2bus.h'
fd "phys2bus.h" include/ arch/
# Check where 'phys2bus.h' is included
rg '#include\s+"phys2bus.h"' --type c
# Verify platform-specific dependencies
rg "phys2bus" arch/ configs/
Length of output: 491
Script:
#!/bin/bash
# Let's check the repository structure first
ls -la
# Then search for phys2bus.h in the entire repository
fd phys2bus.h
# Also search for any mentions of phys2bus
rg -i "phys2bus"
# Check the patch file content to understand the context better
cat buildroot-external/board/raspberrypi/patches/uboot/2024.10/0006-nvme-translate-virtual-addresses-into-the-bus-s-addr.patch
Length of output: 7777
153-158:
Ensure compatibility with non-Device Model (non-DM) configurations
The macro nvme_to_dev(dev) is defined as:
#define nvme_to_dev(_dev) _dev->udevIn non-DM configurations, the udev member of struct nvme_dev may not be initialized, which could lead to null pointer dereferences when accessing nvme_to_dev(dev)->parent. This might cause issues on platforms that do not use the Device Model.
Run the following script to verify the initialization of udev in all instances of struct nvme_dev:
This script searches for instances where struct nvme_dev is used and checks if udev is properly assigned. Ensure that all non-DM users of the NVMe driver initialize dev->udev appropriately.
buildroot-external/patches/uboot/0001-CMD-read-string-from-fileinto-env.patch
Show resolved
Hide resolved
...errypi/patches/uboot/2024.10/0008-reset-reset-brcmstb-Add-Broadcom-STB-reset-controlle.patch
Show resolved
Hide resolved
...errypi/patches/uboot/2024.10/0008-reset-reset-brcmstb-Add-Broadcom-STB-reset-controlle.patch
Show resolved
Hide resolved
...errypi/patches/uboot/2024.10/0008-reset-reset-brcmstb-Add-Broadcom-STB-reset-controlle.patch
Outdated
Show resolved
Hide resolved
...berrypi/patches/uboot/2024.10/0010-pci-pcie-brcmstb-Add-basic-support-for-BCM2712-PCIe.patch
Outdated
Show resolved
Hide resolved
...berrypi/patches/uboot/2024.10/0010-pci-pcie-brcmstb-Add-basic-support-for-BCM2712-PCIe.patch
Outdated
Show resolved
Hide resolved
...berrypi/yellow/patches/linux/0016-ARM-dts-bcm2712-Add-device-tree-for-CM5-on-HA-Yellow.patch
Show resolved
Hide resolved
...berrypi/yellow/patches/linux/0016-ARM-dts-bcm2712-Add-device-tree-for-CM5-on-HA-Yellow.patch
Show resolved
Hide resolved
...berrypi/yellow/patches/linux/0016-ARM-dts-bcm2712-Add-device-tree-for-CM5-on-HA-Yellow.patch
Show resolved
Hide resolved
There was a problem hiding this comment.
I did not dive into the device trees very deep, they are a bit PITA to review. I see we have a couple of workarounds and things relating to downstream/upstream differences 😢 I hope this get fewer as time moves on. And of course, ideally the PCIe/NVMe patches should get obsolete at one time too 🤞 .
Generally LGTM, thanks 🙏 !
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (7)
buildroot-external/board/raspberrypi/patches/uboot/2024.10/0008-reset-reset-brcmstb-Add-Broadcom-STB-reset-controlle.patch (3)
104-104: Fix indentation inconsistencyThere are inconsistent indentations in the
brcmstb_reset_deassertfunction.Apply this diff to fix the indentation:
- unsigned int off = SW_INIT_BANK(rst->id) * SW_INIT_BANK_SIZE; + unsigned int off = SW_INIT_BANK(rst->id) * SW_INIT_BANK_SIZE; - return 0; + return 0;Also applies to: 114-114
108-113: Consider making the delay configurableThe 200us delay is hardcoded with some slack over the typical 14us worst case. Consider making this configurable through device tree or a define to allow platform-specific tuning if needed.
131-134: Consider adding debug message for probe failureThe probe function could benefit from a debug message when the address mapping fails.
Apply this diff to add debug output:
priv->base = dev_remap_addr(dev); - if (!priv->base) + if (!priv->base) { + debug("%s: Failed to map device address\n", __func__); return -EINVAL; + }buildroot-external/board/raspberrypi/patches/uboot/2024.10/0010-pci-pcie-brcmstb-Add-basic-support-for-BCM2712-PCIe.patch (3)
154-173: Consider improving PLL configuration documentation and error handlingThe
brcm_pcie_munge_pllfunction contains hardcoded PLL register values without clear documentation of their purpose. Additionally, the function reads values but doesn't verify them.Consider:
- Adding comments explaining the purpose of each register value
- Adding error handling for MDIO read/write operations
- Adding debug logging for register values
static void brcm_pcie_munge_pll(struct brcm_pcie *pcie) { - /* Upstream Linux doesn't touch these so maybe there's other way */ + /* + * Configure PLL registers to support 54MHz reference clock. + * Register mapping: + * 0x16: PLL control 1 + * 0x17: PLL control 2 + * ... + */ u32 tmp; int i; u8 regs[] = { 0x16, 0x17, 0x18, 0x19, 0x1b, 0x1c, 0x1e }; u16 data[] = { 0x50b9, 0xbda1, 0x0094, 0x97b4, 0x5030, 0x5030, 0x0007 }; + int ret; brcm_pcie_mdio_write(pcie->base, MDIO_PORT0, SET_ADDR_OFFSET, 0x1600); for (i = 0; i < ARRAY_SIZE(regs); i++) { - brcm_pcie_mdio_read(pcie->base, MDIO_PORT0, regs[i], &tmp); + ret = brcm_pcie_mdio_read(pcie->base, MDIO_PORT0, regs[i], &tmp); + if (ret) + printf("PCIe BRCM: failed to read PLL reg 0x%x: err=%d\n", + regs[i], ret); + debug("PCIe BRCM: PLL reg 0x%x = 0x%x\n", regs[i], tmp); }
265-275: Document magic numbers and add error handlingThe clock configuration contains magic numbers without clear documentation. Additionally, there's no error handling for register operations.
if (pcie->soc_base == BCM2712) { - /* Hack from RPi downstream, unable to probe without it */ - /* Allow a 54MHz (xosc) refclk source */ + /* + * Configure PCIe controller for 54MHz reference clock (xosc). + * This configuration is required for BCM2712 probe to succeed. + */ brcm_pcie_munge_pll(pcie); + /* Fix for L1SS errata */ tmp = readl(base + PCIE_RC_PL_PHY_CTL_15); tmp &= ~PCIE_RC_PL_PHY_CTL_15_PM_CLK_PERIOD_MASK; - /* PM clock period is 18.52ns (round down) */ - tmp |= 0x12; + /* + * Set PM clock period to 18.52ns (rounded down to 0x12). + * Calculation: period = 1 / 54MHz ≈ 18.52ns + */ + tmp |= 0x12; writel(tmp, base + PCIE_RC_PL_PHY_CTL_15); }
316-321: Add comment explaining the RESCAL access blockingThe code prevents bridge reset on BCM2712 but the comment could be more detailed about why this is necessary.
/* Shutdown bridge */ - /* On BCM2712 it will block access to the RESCAL, so don't do it. */ + /* + * Skip bridge reset on BCM2712 as it blocks access to the RESCAL + * controller, which is required for proper shutdown sequence. + * The RESCAL controller manages SerDes calibration. + */ if (pcie->soc_base != BCM2712) pcie->bridge_sw_init_set(pcie, 1);buildroot-external/board/raspberrypi/patches/uboot/2024.10/0009-reset-reset-brcmstb-rescal-Add-Broadcom-RESCAL-reset.patch (1)
58-58: Use C-style comment for SPDX license identifierU-Boot coding standards require the SPDX license identifier to be enclosed in a C-style comment for consistency.
Apply this diff to fix the issue:
-// SPDX-License-Identifier: GPL-2.0 +/* SPDX-License-Identifier: GPL-2.0 */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
buildroot-external/board/raspberrypi/patches/uboot/2024.10/0008-reset-reset-brcmstb-Add-Broadcom-STB-reset-controlle.patch(1 hunks)buildroot-external/board/raspberrypi/patches/uboot/2024.10/0009-reset-reset-brcmstb-rescal-Add-Broadcom-RESCAL-reset.patch(1 hunks)buildroot-external/board/raspberrypi/patches/uboot/2024.10/0010-pci-pcie-brcmstb-Add-basic-support-for-BCM2712-PCIe.patch(1 hunks)
🔇 Additional comments (9)
buildroot-external/board/raspberrypi/patches/uboot/2024.10/0008-reset-reset-brcmstb-Add-Broadcom-STB-reset-controlle.patch (4)
10-14: Well-structured commit message!
The commit message clearly explains the purpose, source, and adaptations made. Good job including the Signed-off-by line.
30-35: LGTM! Kconfig changes look good.
The configuration option is properly defined with appropriate help text and correctly uses bool type.
47-47: LGTM! Makefile addition is correct.
The object file is properly added and follows the existing pattern.
122-125: Verify device tree bindings and reset ID assignments
The driver looks well-implemented, but we should verify:
- Device tree bindings match upstream Linux
- Reset ID assignments are consistent with hardware documentation
buildroot-external/board/raspberrypi/patches/uboot/2024.10/0010-pci-pcie-brcmstb-Add-basic-support-for-BCM2712-PCIe.patch (4)
40-41: LGTM: Memory map extension for PCIe region
The memory map is correctly extended to include the PCIe memory region with appropriate attributes for device memory.
Also applies to: 45-57
192-215: LGTM: Robust reset handling implementation
The brcm_pcie_bridge_sw_init_set_generic function properly handles both hardware reset controller and software reset scenarios with appropriate error handling.
335-349: LGTM: Proper driver initialization with error handling
The driver initialization code properly handles optional reset controllers and initializes all required function pointers.
358-378: LGTM: Well-structured SoC-specific configurations
The SoC-specific configurations are well organized using separate data structures for BCM2711 and BCM2712, making it easy to add support for new SoCs in the future.
buildroot-external/board/raspberrypi/patches/uboot/2024.10/0009-reset-reset-brcmstb-rescal-Add-Broadcom-RESCAL-reset.patch (1)
85-88: Confirm necessity of empty brcm_rescal_reset_assert function
The brcm_rescal_reset_assert function currently does nothing and returns 0. If the RESCAL reset controller does not support the assert operation, consider adding a comment to explain this behavior for future maintainability.
| + printf("failed to start SATA/PCIe rescal\n"); | ||
| + return -EIO; | ||
| + } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace printf with dev_err for error messages
Using dev_err provides device context and adheres to U-Boot's logging practices. It is recommended to replace printf with dev_err for better consistency.
Apply this diff to fix the issue:
- printf("failed to start SATA/PCIe rescal\n");
+ dev_err(rst->dev, "failed to start SATA/PCIe rescal\n");📝 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.
| + printf("failed to start SATA/PCIe rescal\n"); | |
| + return -EIO; | |
| + } | |
| + dev_err(rst->dev, "failed to start SATA/PCIe rescal\n"); | |
| + return -EIO; | |
| + } |
| + printf("Invalid args_count: %d\n", args->args_count); | ||
| + return -EINVAL; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace printf with dev_err for error messages
In the brcm_rescal_reset_of_xlate function, using dev_err provides better context for error messages.
Apply this diff:
- printf("Invalid args_count: %d\n", args->args_count);
+ dev_err(reset_ctl->dev, "Invalid args_count: %d\n", args->args_count);📝 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.
| + printf("Invalid args_count: %d\n", args->args_count); | |
| + return -EINVAL; | |
| + dev_err(reset_ctl->dev, "Invalid args_count: %d\n", args->args_count); | |
| + return -EINVAL; |
| + printf("time out on SATA/PCIe rescal\n"); | ||
| + return ret; | ||
| + } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace printf with dev_err for error messages
Similarly, replace printf with dev_err to maintain consistency with logging practices.
Apply this diff:
- printf("time out on SATA/PCIe rescal\n");
+ dev_err(rst->dev, "timeout on SATA/PCIe rescal\n");Committable suggestion skipped: line range outside the PR's diff.
Add support for Raspberry Pi CM5 on Yellow. Most of the core work is the device tree with definition of Yellow peripherals, and U-Boot patches adding support for PCIe/NVMe for BCM2712, which is not present in the upstream U-Boot.
Yellow has been migrated to U-Boot 2024.10 which already has basic support for BCM2712. Other Raspberry Pi targets are still left 2024.01, so we will temporarily have two patchsets for RPi.
The PCIe/NVMe implementation in U-Boot is inspired by Raspberry Pi's downstream Linux implementation, and it might be worthwhile to check upstream implementation [1] once it gets finished but at the time of writing the patches, I was unable to make the controller work with changes from the v2 of the series.
The device tree for CM5 uses the same aliases of serial ports and other peripherals to match CM4 on Yellow, making CM5 a drop-in replacement of the CM4. Of course, the OS must be updated first before swapping the modules. For NVMe-only installs (i.e. those using CM4 Lite or being installed wittingly directly to NVMe) no further steps should be needed. For installations running from eMMC, the CM5 must be bootstrapped using the
rpibootutility, because the EEPROM bootloader doesn't supportBCM-USB-MSDboot mode on CM5 anymore.Things known to be not working:
Factory reset in U-Boot
Because GPIOs are handled by RP1 chipset connected over PCIe, there is no support for reading and setting GPIOs in current U-Boot. Adding at least some rudimentary driver to control GPIOs will be needed to make the standard Yellow's factory reset working.
U-Boot USB
USB (the dwc2 peripheral) doesn't seem to be working and can't access the ports behind the USB hub, neither can't be used for USB Mass Storage (after running
umscommand in U-Boot the USB device fails to be enumerated on host). This is also blocker for reflashing the Yellow's CM from U-Boot.[1] https://lore.kernel.org/all/[email protected]/
Summary by CodeRabbit
New Features
fileenvin U-Boot to read files into environment variables.Bug Fixes
u-boot,no-probeproperty.Documentation
Chores