Conversation
EvilOlaf
requested review from
AGM1968,
ColorfulRhino,
JohnTheCoolingFan,
NicoD-SBC,
PanderMusubi,
SuperKali,
TheSnowfield,
Tonymac32,
amazingfate,
chraac,
igorpecovnik,
janprunk,
krachlatte,
pyavitz,
rpardini,
schwar3kat,
sgjava and
teknoid
as code owners
📝 WalkthroughWalkthroughUpdated boot patch directory reference to v2026.01 and applied multiple kernel and U-Boot changes for Allwinner sun55i platforms: USB3 clock additions, IOMMU driver introduction, PCIe/PHY/SPI enablement and DT/config adjustments, clock/reset rework, and NVMe boot target addition. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
github-actions
bot
added
02
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
patch/u-boot/v2026.01/pcie-sunxi-add-dw-pcie-support-for-sun55i.patch (1)
40-41:⚠️ Potential issue | 🟡 MinorMissing newline at end of Makefile.
The patch adds a line to
drivers/pci/Makefilewithout a trailing newline (\ No newline at end of file). This can cause issues with some tools and should be fixed.Proposed fix
+obj-$(CONFIG_PCIE_SUN55I_RC) += pcie-sun55i-plat.o pcie-sun55i.o -\ No newline at end of file +patch/kernel/archive/sunxi-6.18/patches.armbian/drv-iommu-sunxi-add-iommu-driver.patch (2)
46-49:⚠️ Potential issue | 🟡 MinorMissing newline at end of Makefile.
Similar to the PCIe patch, the Makefile modification is missing a trailing newline.
Proposed fix
obj-$(CONFIG_SUN55I_IOMMU) += sunxi-iommu.o sunxi-iommu-objs := sun55i-iommu-pgtable.o -sunxi-iommu-objs += sun55i-iommu.o \ No newline at end of file +sunxi-iommu-objs += sun55i-iommu.o
1769-1773:⚠️ Potential issue | 🟠 MajorUnreachable code in sunxi_iommu_profilling_show.
The
return 0;at line 1771 (labelerr:) makes all subsequent code unreachable, including the scnprintf call that formats the actual profile data. This appears to be a bug that causes the sysfs profiling interface to always return empty output.Proposed fix - remove erroneous return and err label
spin_unlock(&iommu->iommu_lock); -err: - return 0; len = scnprintf( buf, PAGE_SIZE,patch/u-boot/v2026.01/clk-sunxi-add-sun55i-a523-pcie-usb3-clocks.patch (1)
37-43:⚠️ Potential issue | 🟠 MajorU-Boot clock header must define CLK_NPU before CLK_USB3_REF to match kernel bindings.
The U-Boot DT bindings header defines
CLK_USB3_REF = 179, but the kernel patch definesCLK_NPU = 179followed byCLK_USB3_REF = 180. The U-Boot patch is missing theCLK_NPUdefinition. This causes clock ID mismatch: the same device tree binding identifier will resolve to different numeric values (179 in U-Boot, 180 in kernel), breaking binary compatibility.Add
#define CLK_NPU 179before#define CLK_USB3_REF 180in the U-Boot header to align with kernel definitions.
🧹 Nitpick comments (7)
patch/u-boot/v2026.01/pcie-sunxi-add-dw-pcie-support-for-sun55i.patch (2)
23-30: Trailing whitespace in Kconfig entry.Line 30 has trailing whitespace after the help text. Additionally, the indentation uses 7 spaces instead of the standard tab character for Kconfig entries.
680-684: Inconsistent indentation in pcie-sun55i-plat.c.The code mixes tabs and spaces for indentation (e.g., lines 681-684 use spaces instead of tabs). While this is existing vendor code being ported, consistent indentation would improve maintainability.
patch/kernel/archive/sunxi-6.18/patches.armbian/drv-iommu-sunxi-add-iommu-driver.patch (1)
1621-1625: Mixed declarations and code (C90 violation).Variable
lenis declared after executable code (kmalloc and conditional). While this compiles with C99/GNU extensions, kernel code traditionally uses C90-style declarations at block start.Proposed fix
+ int len; iommu_profile = kmalloc(sizeof(*iommu_profile), GFP_KERNEL); if (!iommu_profile) return 0; - int len; spin_lock(&iommu->iommu_lock);patch/u-boot/v2026.01/board_orangepi-4a/arm64-dts-sun55i-t527-orangepi-4a-enable-spi0-pcie-combophy.patch (1)
57-63: Trailing whitespace on line 61.There's a trailing space after
<®_pcie_vcc3v3>;which should be removed for cleaner patches.Proposed fix
&pcie { reset-gpios = <&pio 7 11 GPIO_ACTIVE_HIGH>; wake-gpios = <&pio 7 12 GPIO_ACTIVE_HIGH>; num-lanes = <2>; - slot-3v3-supply = <®_pcie_vcc3v3>; + slot-3v3-supply = <®_pcie_vcc3v3>; status = "okay"; };patch/u-boot/v2026.01/board_radxa-cubie-a5e/arm64-dts-sun55i-a527-cubie-a5e-enable-spi0-pcie-combophy.patch (3)
21-32: Trailing whitespace on line 26.There's a trailing space after the GPIO reference
<&r_pio 0 11 GPIO_ACTIVE_HIGH>;which should be removed.Proposed fix
reg_pcie_vcc3v3: regulator-pcie-vcc3v3 { compatible = "regulator-fixed"; regulator-name = "pcie-pwren"; regulator-min-microvolt = <3300000>; regulator-max-microvolt = <3300000>; - gpio = <&r_pio 0 11 GPIO_ACTIVE_HIGH>; + gpio = <&r_pio 0 11 GPIO_ACTIVE_HIGH>; enable-active-high;
83-89: Use DTS-style comment instead of C++ style.Line 88 uses C++ style comment (
//) which is non-standard for device tree source files. DTS conventionally uses/* */style comments.Proposed fix
pcie_pwren_pins: pcie-pwren-pins { allwinner,pins = "PL11"; function = "gpio_out"; allwinner,pinmux = <1>; drive-strength = <10>; - bias-disable; //bias-pull-up; or bias-pull-down; + bias-disable; /* bias-pull-up; or bias-pull-down; */ };
60-66: Minor: Inconsistent spacing in pin list.Line 62 has
"PC2","PC4"without a space after the first comma, while other entries use consistent spacing.Proposed fix
spi0_pins: spi0-pins { - pins = "PC2","PC4", "PC12"; + pins = "PC2", "PC4", "PC12"; allwinner,pinmux = <4>; function = "spi0"; drive-strength = <10>; };
igorpecovnik
force-pushed
the
sunxi-a523-aftercare
branch
from
e28745c to
68bc00d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
patch/u-boot/v2026.01/board_radxa-cubie-a5e/arm64-dts-sun55i-a527-cubie-a5e-enable-spi0-pcie-combophy.patch (3)
26-26:⚠️ Potential issue | 🟡 MinorTrailing whitespace after semicolon.
Line 26 has a trailing space after the semicolon which should be removed for consistency.
- gpio = <&r_pio 0 11 GPIO_ACTIVE_HIGH>; + gpio = <&r_pio 0 11 GPIO_ACTIVE_HIGH>;
62-62:⚠️ Potential issue | 🟡 MinorInconsistent spacing in pin list.
Missing space after the first comma in the pins list.
- pins = "PC2","PC4", "PC12"; + pins = "PC2", "PC4", "PC12";
88-88:⚠️ Potential issue | 🟡 MinorUse C-style comment instead of C++ style.
DTS files conventionally use C-style comments (
/* */) rather than C++ style (//).- bias-disable; //bias-pull-up; or bias-pull-down; + bias-disable; /* bias-pull-up; or bias-pull-down; */patch/u-boot/v2026.01/phy-allwinner-add-pcie-usb3-driver.patch (2)
610-652:⚠️ Potential issue | 🟠 MajorGuard regulator disable on error paths.
select3v3_supplyis treated as optional earlier, but error paths disable it unconditionally. If the supply is absent, this risks a NULL deref. Add guards before disabling, or make the supply mandatory everywhere.🛡️ Suggested guard
ret = reset_deassert(&combphy->reset); if (ret) { printf("PHY: Failed to deassert reset: %d\n", ret); - regulator_set_enable(combphy->select3v3_supply, false); + if (combphy->select3v3_supply) + regulator_set_enable(combphy->select3v3_supply, false); return ret; } ret = pcie_usb3_sub_system_init(combphy); if (ret) { printf("PHY: failed to init sub system\n"); reset_assert(&combphy->reset); - regulator_set_enable(combphy->select3v3_supply, false); + if (combphy->select3v3_supply) + regulator_set_enable(combphy->select3v3_supply, false); return ret; } ret = sun55i_combphy_set_mode(combphy); if (ret) { printf("PHY: invalid number of arguments\n"); reset_assert(&combphy->reset); - regulator_set_enable(combphy->select3v3_supply, false); + if (combphy->select3v3_supply) + regulator_set_enable(combphy->select3v3_supply, false); return ret; }
482-501:⚠️ Potential issue | 🟡 MinorThe code path is currently unreachable due to hardcoded defaults, but the initialization logic is inconsistent and should be fixed.
In the
sun55i_combphy_set_mode()function at lines 488-493, whenmode == PHY_TYPE_USB3anduser == PHY_USE_BY_PCIE_USB3_U2, onlysun55i_combphy_pcie_init()runs, skipping the USB3 PHY tuning insun55i_combphy_usb3_init().However, this is inconsistent with
pcie_usb3_sub_system_enable()at lines 578-581, which enables both PCIe and USB3 clocks (combo_pcie_clk_set()andcombo_usb3_clk_set()) for the combined mode. If hardware clocks are enabled for both interfaces, the PHY initialization should initialize both as well.Note: Currently,
sun55i_inno_phy_probe()hardcodesmode = PHY_TYPE_PCIEanduser = PHY_USE_BY_PCIE, making this code path unreachable. However, the enum values suggest combined mode support was intended, and the inconsistency should be fixed before such support is enabled:Suggested fix
case PHY_TYPE_USB3: if (combphy->user == PHY_USE_BY_PCIE_USB3_U2) { sun55i_combphy_pcie_init(combphy); + sun55i_combphy_usb3_init(combphy); } else if (combphy->user == PHY_USE_BY_USB3) { sun55i_combphy_usb3_init(combphy); } break;patch/u-boot/v2026.01/sunxi-add-nvme-boot-target.patch (1)
19-33:⚠️ Potential issue | 🟡 MinorAdd CONFIG_CMD_NVME to defconfigs for NVMe boot to function.
The
BOOT_TARGET_DEVICES_NVME()macro is compiled out ifCONFIG_CMD_NVMEis not set. The defconfig patches for radxa-cubie-a5e and orangepi-4a addCONFIG_NVMEandCONFIG_NVME_PCIbut omitCONFIG_CMD_NVME, leaving NVMe boot non-functional.Add
CONFIG_CMD_NVME=yto both:
configs/radxa-cubie-a5e_defconfigconfigs/orangepi_4a_defconfigpatch/kernel/archive/sunxi-6.18/patches.armbian/drv-iommu-sunxi-add-iommu-driver.patch (3)
46-49:⚠️ Potential issue | 🟡 MinorMissing newline at end of Makefile.
The Makefile lacks a trailing newline, which can cause issues with some tools and is flagged by many linters.
🔧 Suggested fix
obj-$(CONFIG_SUN55I_IOMMU) += sunxi-iommu.o sunxi-iommu-objs := sun55i-iommu-pgtable.o -sunxi-iommu-objs += sun55i-iommu.o \ No newline at end of file +sunxi-iommu-objs += sun55i-iommu.o
1770-1772:⚠️ Potential issue | 🔴 CriticalUnreachable code - function always returns 0 without output.
The
sunxi_iommu_profilling_show()function has a prematurereturn 0;at line 1771 (mapped toerr:label), making all the code after it (lines 1773-1840) unreachable. The function will never produce any profiling output.This appears to be a bug where the
goto err;label was misplaced or a debugging artifact was left in.🐛 Suggested fix: Remove the erroneous return
spin_unlock(&iommu->iommu_lock); -err: - return 0; len = scnprintf( buf, PAGE_SIZE,Note: The
err:label appears to be dead code as nothing jumps to it. If error handling is needed, it should be properly structured.
2101-2106:⚠️ Potential issue | 🟠 MajorPTR_ERR called after setting pointer to NULL.
After the
IS_ERRcheck, the clock pointer is set toNULLbefore callingPTR_ERR. This will return an incorrect error code (likely 0 or garbage) instead of the actual error.🐛 Suggested fix: Capture error before nullifying pointer
sunxi_iommu->clk = of_clk_get_by_name(dev->of_node, "iommu"); if (IS_ERR(sunxi_iommu->clk)) { + ret = PTR_ERR(sunxi_iommu->clk); sunxi_iommu->clk = NULL; dev_dbg(dev, "Unable to find clock\n"); - ret = PTR_ERR(sunxi_iommu->clk); goto err_clk; }
🧹 Nitpick comments (5)
patch/u-boot/v2026.01/phy-allwinner-add-pcie-usb3-driver.patch (1)
342-347: Remove or implement the commented-out exit path.Either wire this into a
.exitop for proper power-down, or delete it to avoid dead code.patch/u-boot/v2026.01/spi-sunxi-add-sun55i-a523-support.patch (1)
36-49: Inconsistent indentation in FIFO reset timeout block.The new code block uses spaces for indentation while the surrounding U-Boot code uses tabs. This creates a visual inconsistency in the patch.
🔧 Suggested fix: Convert spaces to tabs
- if (priv->variant->has_bsr) { - u32 reg; - int ret; - - ret = readl_poll_timeout(SPI_REG(priv, SPI_FCR), reg, - !(reg & (SPI_BIT(priv, SPI_FCR_RF_RST) | - SPI_BIT(priv, SPI_FCR_TF_RST))), - SUN4I_SPI_TIMEOUT_MS * 1000); - if (ret) { - printf("ERROR: sun4i_spi: FIFO reset timeout\n"); - sun4i_spi_set_cs(bus, slave_plat->cs[0], false); - return ret; - } - } + if (priv->variant->has_bsr) { + u32 reg; + int ret; + + ret = readl_poll_timeout(SPI_REG(priv, SPI_FCR), reg, + !(reg & (SPI_BIT(priv, SPI_FCR_RF_RST) | + SPI_BIT(priv, SPI_FCR_TF_RST))), + SUN4I_SPI_TIMEOUT_MS * 1000); + if (ret) { + printf("ERROR: sun4i_spi: FIFO reset timeout\n"); + sun4i_spi_set_cs(bus, slave_plat->cs[0], false); + return ret; + } + }patch/u-boot/v2026.01/spi-sunxi-add-sun55i-a523-spl-support.patch (2)
28-30: Inconsistent indentation in A523 pinmux setup.The A523-specific pinmux code uses spaces while the rest of the function uses tabs.
🔧 Suggested fix
if (IS_ENABLED(CONFIG_MACH_SUN55I_A523)) { - sunxi_gpio_set_cfgpin(SUNXI_GPC(12), pin_function); + sunxi_gpio_set_cfgpin(SUNXI_GPC(12), pin_function); }
101-173: Significant indentation change from tabs to 4-space indentation.The entire
sunxi_spi0_read_data()function body has been re-indented from tabs to 4-space indentation. This is inconsistent with the U-Boot coding style and the rest of the file.If this was unintentional (possibly due to editor settings), consider reverting the whitespace changes to maintain consistency. Based on learnings, maintaining consistency with existing patches is prioritized over refactoring.
patch/kernel/archive/sunxi-6.18/patches.armbian/drv-iommu-sunxi-add-iommu-driver.patch (1)
25-34: Kconfig uses spaces instead of tabs for indentation.Kernel Kconfig files should use tabs for indentation per kernel coding style.
|
|
✅ This PR has been reviewed and approved — all set for merge! |
2 participants
Description
How Has This Been Tested?
Checklist:
Summary by CodeRabbit
New Features
Improvements
Revert
✏️ Tip: You can customize this high-level summary in your review settings.