sunxi a523: Adding support for PCIe, SPI Flash and Iommu #9280
Conversation
rvdr
requested review from
AGM1968,
JohnTheCoolingFan,
PanderMusubi,
TheSnowfield,
Tonymac32,
chraac,
igorpecovnik,
janprunk,
krachlatte,
pyavitz,
schwar3kat,
sgjava and
teknoid
as code owners
📝 WalkthroughWalkthroughAdds Allwinner A523 (sun55i) platform support across Linux kernel and U-Boot: PCIe RC, INNO combo PHY (PCIe/USB3), SPI controllers and DT nodes, IOMMU driver, clock/reset updates, NVMe boot support, and related board/device-tree and defconfig entries. Changes
Sequence Diagram(s)sequenceDiagram
participant UBoot as U-Boot
participant Clk as CCU/Clock
participant PHY as INNO_Combophy
participant PCIe as PCIe_RC
participant IOMMU as sun55i_IOMMU
participant Device as PCIe_Device
UBoot->>Clk: enable CLK_USB3_REF / CLK_PCIE_AUX
Clk-->>UBoot: clocks ready
UBoot->>PHY: probe & init combophy (DT refs)
PHY-->>UBoot: PHY provider ready
UBoot->>PCIe: probe PCIe RC (acquire clocks/resets/regulators)
PCIe->>PHY: bind to combophy provider
PCIe->>PCIe: configure LTSSM, lanes, ATU
PCIe-->>UBoot: link trained
UBoot->>IOMMU: register fault cb / enable domains
IOMMU-->>UBoot: mappings active
UBoot->>Device: enumerate via PCIe RC
Device->>PCIe: config/dbi accesses
PCIe->>IOMMU: translate addresses (if enabled)
IOMMU-->>Device: translate/handle faults
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
Hey @rvdr! 👋Thanks for submitting your first pull request to the Armbian project — we're excited to have you contributing! 🧡 If you'd like to stay informed about project updates or collaborate more closely with the team, Also, don’t forget to ⭐ star the repo if you haven’t already — and welcome aboard! 🚀 |
github-actions
bot
added
size/large
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Fix all issues with AI agents
In `@config/kernel/linux-sunxi64-edge.config`:
- Around line 2419-2423: The kernel config enables CONFIG_VFIO=m but explicitly
disables CONFIG_VFIO_GROUP, which prevents user-space VFIO device assignment;
change CONFIG_VFIO_GROUP from "not set" to a module (CONFIG_VFIO_GROUP=m) to
match CONFIG_VFIO and enable /dev/vfio/<group_id> support (ensure the
CONFIG_VFIO_GROUP entry is present alongside CONFIG_VFIO and CONFIG_VFIO_PCI in
the same config block).
- Around line 3054-3061: The CONFIG_LSM list does not include "apparmor" while
CONFIG_DEFAULT_SECURITY_APPARMOR is enabled; update the configuration so the
build-time LSM list matches the default security choice by adding "apparmor"
into the CONFIG_LSM string (e.g., include apparmor among
"yama,loadpin,safesetid,integrity") or, if AppArmor should not be enabled by
default, disable CONFIG_DEFAULT_SECURITY_APPARMOR instead; modify the CONFIG_LSM
and/or CONFIG_DEFAULT_SECURITY_APPARMOR settings accordingly to ensure AppArmor
is actually initialized at boot.
In
`@patch/kernel/archive/sunxi-6.18/patches.armbian/01-phy-pcie-usb3-add-driver.patch`:
- Around line 275-385: The pcie_usb3_sub_system_init function uses spaces
instead of kernel-style tabs and lacks proper cleanup on failure of
clk_prepare_enable calls; update indentation to tabs and add error unwind logic:
when any clk_prepare_enable or regulator_enable succeeds (e.g., phyclk_ref,
phyclk_cfg, phy_hclk, phy_axi, phy_mclk, select3v3_supply) and a later step
fails, call the corresponding clk_disable_unprepare/clk_disable_* (or
regulator_disable) and assert any deasserted resets (use reset_control_assert
for reset and noppu_reset) before returning; locate logic in
pcie_usb3_sub_system_init and ensure every resource acquired has a matching
release on all error paths, especially around calls to clk_prepare_enable,
clk_set_rate, regulator_enable, reset_control_deassert, and
pcie_usb3_sub_system_enable.
In
`@patch/kernel/archive/sunxi-6.18/patches.armbian/03-iommu-add-sunxi-driver.patch`:
- Around line 1017-1066: Multiple functions index fixed-size arrays without
validating master_id leading to OOB access; add a bounds check using
ARRAY_SIZE(sunxi_iommu_fault_notify_cbs) (or a hard 7) and return early if
master_id is out-of-range. Specifically, in sun55i_iommu_register_fault_cb,
sun55i_reset_device_iommu and sun55i_enable_device_iommu validate master_id
before accessing sunxi_iommu_fault_notify_cbs or master_id_bitmap (use
WARN_ON_ONCE or dev_warn where a device pointer is available, otherwise just
return) to prevent invalid indexing and memory corruption.
- Around line 1233-1265: In sunxi_iommu_unmap, fix handling of
sunxi_pgtable_delete_l2_tables() by changing iova_tail_size to a signed int (or
int ret) and checking for negative returns before using it; if ret < 0
handle/propagate the error (e.g., break/return 0 or appropriate size) instead of
casting to u32 which wraps and loops indefinitely, and only call
sun55i_ptw_cache_invalid() with a valid non-zero range (use iova_start and
iova_start + ret or skip the PTW invalidation when ret <= 0) to avoid
programming an invalid end range.
In `@patch/u-boot/sunxi-dev-u-boot-a523/add-spl-spi-support.patch`:
- Around line 21-23: SUN55I_BUF_STA_REG is only defined under the
CONFIG_MACH_SUN55I_A523 guard but is referenced unconditionally elsewhere,
causing build failures; either define SUN55I_BUF_STA_REG unconditionally with a
safe default or guard all usages with the same
IS_ENABLED(CONFIG_MACH_SUN55I_A523) check. Locate the definition added and
replace it with an unconditional definition of SUN55I_BUF_STA_REG (so other
sunxi targets compile) or wrap every reference to SUN55I_BUF_STA_REG in `#if`
IS_ENABLED(CONFIG_MACH_SUN55I_A523) ... `#endif` blocks (search for uses of
SUN55I_BUF_STA_REG in the patch and surrounding files) to ensure the macro is
only referenced when CONFIG_MACH_SUN55I_A523 is enabled.
In `@patch/u-boot/sunxi-dev-u-boot-a523/add-sun55i-pcie-driver.patch`:
- Around line 940-942: The probe currently blindly overwrites DT-parsed values
by assigning pci->lanes = data->num_lanes and pci->link_gen =
data->max_link_speed; change this to preserve DT values parsed in
sun55i_pcie_plat_hw_init() by only copying from of_data when the DT fields are
unset (e.g., pci->lanes == 0 or pci->link_gen == 0/unknown). Update the probe's
assignment logic to check pci->lanes and pci->link_gen before assigning from
data->num_lanes / data->max_link_speed so DT-provided settings take precedence.
- Around line 580-619: The CFG0/CFG1 selection in sun55i_pcie_rd_other_conf and
sun55i_pcie_wr_other_conf is wrong: replace the current check (if PCI_BUS(d) !=
0 => CFG0 else CFG1) with logic that uses CFG0 only for the first downstream bus
and CFG1 for all other buses (e.g., if PCI_BUS(d) == pp->root_bus_nr + 1 then
type = PCIE_ATU_TYPE_CFG0 else type = PCIE_ATU_TYPE_CFG1), and apply the same
change in both sun55i_pcie_rd_other_conf and sun55i_pcie_wr_other_conf; also
ensure pp->root_bus_nr is set during probe (initialize from pci->first_busno).
In `@patch/u-boot/sunxi-dev-u-boot-a523/add-sun55i-usb3-phy-driver.patch`:
- Around line 507-533: The combo_phy_mode_set function builds tmp incorrectly
(starts at 0 then only ORs into val) and never properly clears mode or reset
bits; fix it by reading the current PHYCTRL into val, create a mask of the mode
bits (PHY_USE_SEL | PHY_CLK_SEL | PHY_RSTN), compute tmp from val (e.g., tmp =
val & ~mask) or clear those bits in val first, then set/clear PHY_USE_SEL and
PHY_CLK_SEL based on combphy->user and combphy->ref, toggle PHY_RSTN according
to enable, and finally write back a combined value that replaces only the masked
bits (e.g., val = (val & ~mask) | new_bits) and writel to
COMBO_REG_PHYCTRL(combphy->phy_ctl) so mode/clock/reset bits are correctly
cleared and set.
- Around line 58-66: Remove the forbidden test header by deleting the `#include`
<test/common.h> line added in the driver patch and either replace it with
`#include` <common.h> if the driver uses common helper macros, or remove the
include entirely if no symbols from test/common.h are referenced; check the
driver initialization/probe code (e.g., any sun55i_usb3_phy_* functions) for
usages of test-only symbols and adjust includes to only public U-Boot headers
like <common.h>, <dm.h>, <clk-uclass.h>, etc., then rebuild to confirm the
compilation error is resolved.
In `@patch/u-boot/sunxi-dev-u-boot-a523/edit-u-boot-orangepi-4a-dts.patch`:
- Around line 39-44: The node spi0_pin_default is misleading because it sets
function = "gpio_in" and allwinner,pinmux = <0> (a sleep/disabled state) while
its name implies SPI defaults; rename the node to something like spi0_pins_sleep
or spi0_pins_disabled, or if the intent was to set SPI mode make the node set
the correct function/ pinmux for SPI instead; update any references to
spi0_pin_default accordingly to keep names consistent with behavior.
♻️ Duplicate comments (1)
patch/u-boot/sunxi-dev-u-boot-a523/add-sun55i-pcie-driver.patch (1)
62-66: Same<test/common.h>include concern as in the combophy driver.Also applies to: 510-513
🧹 Nitpick comments (7)
patch/kernel/archive/sunxi-6.18/patches.armbian/edit-kernel-cubie-a5e-dts.patch (1)
93-104: Inconsistent indentation in spi0 node.The spi0 node has mixed indentation - line 95 has leading spaces before
pinctrl-names, and the child node uses different tab depth than typical DT style.♻️ Suggested formatting fix
&spi0 { pinctrl-0 = <&spi0_pins>, <&spi0_cs_pin>; - pinctrl-names = "default"; - status = "okay"; - w25q128: flash@0 { - compatible = "winbond,w25q128fw", "jedec,spi-nor"; - reg = <0>; - spi-max-frequency = <24000000>; - vcc-supply = <®_cldo1>; - status = "okay"; - }; + pinctrl-names = "default"; + status = "okay"; + w25q128: flash@0 { + compatible = "winbond,w25q128fw", "jedec,spi-nor"; + reg = <0>; + spi-max-frequency = <24000000>; + vcc-supply = <®_cldo1>; + status = "okay"; + }; };patch/u-boot/sunxi-dev-u-boot-a523/u-boot-sun55iw3-edge-dtsi.patch (1)
46-59: Inconsistent indentation in spi0 node.The spi0 node uses spaces for indentation instead of tabs, which is inconsistent with the rest of the file and DT conventions. Also,
device_type = "spi0"is unusual—device_typeis typically reserved formemory,cpu, orpcinodes.♻️ Suggested fix
spi0: spi@4025000 { - `#address-cells` = <1>; - `#size-cells` = <0>; - compatible = "allwinner,sun55i-a523-spi"; - device_type = "spi0"; - reg = <0x04025000 0x1000>; - interrupts = <GIC_SPI 16 IRQ_TYPE_LEVEL_HIGH>; - clocks = <&ccu CLK_BUS_SPI0>, <&ccu CLK_SPI0>; - clock-names = "ahb", "mod"; - resets = <&ccu RST_BUS_SPI0>; - dmas = <&dma 22>, <&dma 22>; - dma-names = "tx", "rx"; - status = "disabled"; - }; + `#address-cells` = <1>; + `#size-cells` = <0>; + compatible = "allwinner,sun55i-a523-spi"; + reg = <0x04025000 0x1000>; + interrupts = <GIC_SPI 16 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&ccu CLK_BUS_SPI0>, <&ccu CLK_SPI0>; + clock-names = "ahb", "mod"; + resets = <&ccu RST_BUS_SPI0>; + dmas = <&dma 22>, <&dma 22>; + dma-names = "tx", "rx"; + status = "disabled"; + };patch/u-boot/sunxi-dev-u-boot-a523/edit-u-boot-radxa-cubie-a5e-dts.patch (2)
48-58: Commented-out code should be removed or documented.Line 53 has a commented-out
regulator-enable-ramp-delayproperty. If this is intentionally disabled, consider removing the comment entirely or adding a brief explanation of why it was removed.
113-119: Minor: inline comment style.Line 118 has
bias-disable; //bias-pull-up; or bias-pull-down;as an inline comment. Consider using/* */style for consistency with DT conventions, or remove if it's just development notes.patch/u-boot/sunxi-dev-u-boot-a523/add-sun55i-spi-sunxi.patch (1)
46-59: Inconsistent indentation in FIFO reset timeout block.This code block uses spaces for indentation instead of tabs, which is inconsistent with the U-Boot coding style and the rest of the file.
♻️ Suggested fix
- 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))), - 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))), + 1000); + if (ret) { + printf("ERROR: sun4i_spi: FIFO reset timeout\n"); + sun4i_spi_set_cs(bus, slave_plat->cs[0], false); + return ret; + } + }patch/kernel/archive/sunxi-6.18/patches.armbian/01-phy-pcie-usb3-add-driver.patch (1)
484-630: Magic register offsets reduce maintainability.Functions like
sunxi_combphy_usb3_phy_setandsunxi_combphy_pcie_phy_100Muse hardcoded offsets (0x1418, 0x0674, 0x0704, 0x0400, etc.) without symbolic names. While this is common in vendor-derived code, adding defines would improve readability and maintenance.This is optional given the vendor-code origin, but consider adding defines like:
`#define` INNO_PHY_CDR_CFG 0x0400 `#define` INNO_PHY_RX_EQ 0x0674 /* etc. */patch/u-boot/sunxi-dev-u-boot-a523/add-spl-spi-support.patch (1)
176-179: Replace the German inline note with a neutral English comment (or drop it).The
HIER IST IHR WICHTIGER FIXremark reads like a temporary note and makes upstreaming harder.
patch/kernel/archive/sunxi-6.18/patches.armbian/01-phy-pcie-usb3-add-driver.patch
Outdated
Show resolved
Hide resolved
patch/kernel/archive/sunxi-6.18/patches.armbian/drv-iommu-sunxi-add-iommu-driver.patch
Show resolved
Hide resolved
patch/kernel/archive/sunxi-6.18/patches.armbian/03-iommu-add-sunxi-driver.patch
Show resolved
Hide resolved
patch/u-boot/sunxi-dev-u-boot-a523/pci-sunxi-enable-pcie-support.patch
Outdated
Show resolved
Hide resolved
patch/u-boot/sunxi-dev-u-boot-a523/phy-allwinner-add-pcie-usb3-driver.patch
Show resolved
Hide resolved
patch/u-boot/sunxi-dev-u-boot-a523/phy-allwinner-add-pcie-usb3-driver.patch
Show resolved
Hide resolved
patch/u-boot/sunxi-dev-u-boot-a523/edit-u-boot-orangepi-4a-dts.patch
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 Fix all issues with AI agents
In `@config/kernel/linux-sunxi64-edge.config`:
- Around line 3054-3061: The config sets CONFIG_DEFAULT_SECURITY_APPARMOR=y
while CONFIG_LSM omits apparmor, causing a mismatch; fix by either adding
"apparmor" into the CONFIG_LSM value (e.g., include apparmor alongside
yama,loadpin,safesetid,integrity) so CONFIG_SECURITY_APPARMOR can be selected as
the default, or remove/clear CONFIG_DEFAULT_SECURITY_APPARMOR to avoid declaring
AppArmor as default; update the symbols CONFIG_LSM and/or
CONFIG_DEFAULT_SECURITY_APPARMOR (and ensure CONFIG_SECURITY_APPARMOR remains
set consistently) so the LSM list and default selection are aligned.
In
`@patch/kernel/archive/sunxi-6.18/patches.armbian/01-phy-pcie-usb3-add-driver.patch`:
- Around line 1110-1130: The remove function declares an unused local variable
and leaves trailing whitespace; replace the current two-step call/assignment to
pcie_usb3_sub_system_exit with a single inline check (if
(pcie_usb3_sub_system_exit(pdev)) { dev_err(&pdev->dev, "failed to exit sub
system\n"); }) so there's no unused ret variable, ensure the dev_err uses the
existing dev variable or &pdev->dev consistently with sunxi_combphy_remove, and
remove the trailing whitespace on the blank lines around that block and at the
end of the function; keep the
atomic_notifier_chain_unregister(&inno_subsys_notifier_list, &combphy->pwr_nb)
and runtime PM lines unchanged.
In
`@patch/kernel/archive/sunxi-6.18/patches.armbian/03-iommu-add-sunxi-driver.patch`:
- Around line 1017-1069: The exported APIs lack defensive checks: validate
master_id is within bounds before indexing arrays (sunxi_iommu_fault_notify_cbs
and master_id_bitmap) in sun55i_iommu_register_fault_cb,
sun55i_reset_device_iommu and sun55i_enable_device_iommu, and ensure
global_iommu_dev is non-NULL before use in sun55i_reset_device_iommu; also keep
the existing iommu NULL guard in sun55i_enable_device_iommu but add a master_id
bounds check there too. Return early (or no-op) on invalid master_id or NULL
global/iommu, and avoid silently indexing out-of-range arrays when registering
the callback (sunxi_iommu_fault_notify_cbs) or toggling bypass
(master_id_bitmap).
- Around line 2176-2192: The remove path (sun55i_iommu_remove) currently leaks
the identity domain allocated in probe; update sun55i_iommu_remove to check
sunxi_iommu->identity_domain and free it (e.g. call
iommu_domain_free(sunxi_iommu->identity_domain) or the appropriate release API
used when allocating the domain) before tearing down other resources, and clear
the pointer after freeing to avoid use-after-free.
In
`@patch/kernel/archive/sunxi-6.18/patches.armbian/edit-kernel-orangepi_4a-dts.patch`:
- Around line 22-35: Add a proper regulator node definition for ®_pcie_vcc3v3
in the root /{ ... } section (modeled after edit-kernel-cubie-a5e-dts) using
compatible = "regulator-fixed", supply/voltage properties (e.g.,
regulator-fixed, regulator-name, compatible,
regulator-min-microvolt/regulator-max-microvolt or fixed-voltage-microvolt), and
any GPIO controls required so the &pcie pcie3v3-supply reference resolves; then
fix indentation in the &combophy and &pcie node blocks to use consistent spaces
(remove mixed tabs) and strip trailing whitespace from the pcie3v3-supply line
so the properties reset-gpios, wake-gpios, num-lanes, clk-freq-100M,
pcie3v3-supply = <®_pcie_vcc3v3>; and status = "okay" are properly aligned.
In `@patch/u-boot/sunxi-dev-u-boot-a523/add-spl-spi-support.patch`:
- Around line 177-182: Replace the German debug comment before the conditional
loop with a concise English explanation that clarifies why different FIFO masks
are required: explain that when IS_ENABLED(CONFIG_MACH_SUN55I_A523) the
readl(spi_fifo_reg) value must be masked with 0xFF to account for the full 8-bit
FIFO count, whereas other variants use 0x7F to mask only the valid 7-bit count;
keep the conditional code using readl(spi_fifo_reg) & 0xFF for the SUN55I A523
path and & 0x7F for the else branch and place the English comment immediately
above those while loops referencing spi_fifo_reg, readl and bufsize.
In `@patch/u-boot/sunxi-dev-u-boot-a523/add-sun55i-pcie-driver.patch`:
- Around line 580-618: In both sun55i_pcie_rd_other_conf and
sun55i_pcie_wr_other_conf the CFG0/CFG1 selection is reversed for downstream
buses: change the conditional that sets 'type' so that when PCI_BUS(d) != 0 it
assigns PCIE_ATU_TYPE_CFG1 and otherwise assigns PCIE_ATU_TYPE_CFG0 (use the
existing PCIE_ATU_TYPE_CFG0 and PCIE_ATU_TYPE_CFG1 symbols), leaving the rest of
the ATU programming and cfg read/write logic intact.
- Around line 62-70: The driver includes a test header "<test/common.h>" which
must be removed from runtime code; edit the include block (around the PCI/PCIE
includes and functions in this patch) to either drop the test header entirely or
replace it with the proper "<common.h>" if shared utilities from common.h are
actually used by functions in pcie-sun55i.c / pci.c / related initialization
routines, ensuring no test headers remain in the driver includes (also apply the
same change for the other occurrence at lines ~510-517).
In `@patch/u-boot/sunxi-dev-u-boot-a523/add-sun55i-usb3-phy-driver.patch`:
- Around line 507-533: The bug in combo_phy_mode_set is that tmp is initialized
to 0 so operations like tmp &= ~PHY_USE_SEL/PHY_CLK_SEL never clear bits in the
read value; instead base the modifications on the current register value: read
val = readl(COMBO_REG_PHYCTRL(combphy->phy_ctl)) and set tmp = val (or operate
directly on val), then modify PHY_USE_SEL and PHY_CLK_SEL according to
combphy->user and combphy->ref, toggle PHY_RSTN based on enable, and finally
writel the updated value back; ensure you reference the symbols
combo_phy_mode_set, COMBO_REG_PHYCTRL, PHY_USE_SEL, PHY_CLK_SEL, PHY_RSTN,
combphy->user and combphy->ref when making the change.
- Around line 58-75: The driver erroneously includes the test-only header
"<test/common.h>"; remove that include or replace it with the runtime header
"<common.h>" so the driver does not depend on test framework symbols. Edit the
include block to either delete the line '#include <test/common.h>' or change it
to '#include <common.h>', then rebuild to ensure no missing symbol errors in the
USB3 PHY driver code (check other includes like <clk-uclass.h>, <dm.h>, and
functions that reference common helpers).
In `@patch/u-boot/sunxi-dev-u-boot-a523/edit-orangepi-4a-defconfig.patch`:
- Line 31: Replace the incorrect config symbol CONFIG_CMD_LSBK with the correct
CONFIG_CMD_LSBLK; locate the added line containing CONFIG_CMD_LSBK in the patch
and change it to CONFIG_CMD_LSBLK so the U-Boot build enables the lsblk command
properly.
In `@patch/u-boot/sunxi-dev-u-boot-a523/edit-radxa-cubie-a5e-defconfig.patch`:
- Line 31: The patch adds an incorrect U-Boot config symbol CONFIG_CMD_LSBK;
change that symbol to the correct CONFIG_CMD_LSBLK so the lsblk command is
enabled (replace every instance of CONFIG_CMD_LSBK with CONFIG_CMD_LSBLK, e.g.,
the added line in the edit-radxa-cubie-a5e-defconfig patch).
🧹 Nitpick comments (11)
patch/u-boot/sunxi-dev-u-boot-a523/add-spl-spi-support.patch (2)
34-36: Indentation uses spaces instead of tabs.Line 35 uses spaces for indentation which is inconsistent with U-Boot/kernel coding style that requires 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); }
162-199: Inconsistent indentation throughout function body.The entire
sunxi_spi0_read_datafunction body uses 4-space indentation instead of tabs. While this appears to be a formatting change from the original code, U-Boot follows Linux kernel coding style which requires tabs for indentation.patch/u-boot/sunxi-dev-u-boot-a523/edit-u-boot-orangepi-4a-dts.patch (2)
39-44: Misleading node name: configures GPIO input, not SPI function.The node
spi0_pin_defaultsetsfunction = "gpio_in"andallwinner,pinmux = <0>, which configures pins as GPIO inputs rather than the SPI function. This appears to be a power-down/sleep configuration. Consider renaming tospi0_pins_sleepor similar for clarity, or update to configure actual SPI function if this should be the default operating state.Note: The Radxa Cubie A5E patch uses separate
spi0_pins(withfunction = "spi0") andspi0_pins_sleep(withfunction = "gpio_in"), which is clearer.
78-78: Trailing whitespace.There's trailing whitespace after
<®_pcie_vcc3v3>;.Suggested fix
- slot-3v3-supply = <®_pcie_vcc3v3>; + slot-3v3-supply = <®_pcie_vcc3v3>;patch/u-boot/sunxi-dev-u-boot-a523/edit-u-boot-radxa-cubie-a5e-dts.patch (1)
40-40: Use DT-style comments instead of C++ style.Device-tree sources should use
/* comment */style comments rather than// commentfor consistency with DT conventions. Multiple instances found:
- Line 40:
// PL11- Line 53:
// regulator-enable-ramp-delay = <1000>;- Line 118:
//bias-pull-up; or bias-pull-down;- Line 150:
//PL11 PCIE_PWREN_HSuggested fixes
- gpio = <&r_pio 0 11 GPIO_ACTIVE_HIGH>; // PL11 + gpio = <&r_pio 0 11 GPIO_ACTIVE_HIGH>; /* PL11 */- // regulator-enable-ramp-delay = <1000>; + /* regulator-enable-ramp-delay = <1000>; */- bias-disable; //bias-pull-up; or bias-pull-down; + bias-disable; /* bias-pull-up; or bias-pull-down; */- slot-3v3-supply = <&pcie_pwren_reg>; //PL11 PCIE_PWREN_H + slot-3v3-supply = <&pcie_pwren_reg>; /* PL11 PCIE_PWREN_H */Also applies to: 53-53, 118-118, 150-150
patch/kernel/archive/sunxi-6.18/patches.armbian/01-phy-pcie-usb3-add-driver.patch (1)
275-385: Inconsistent indentation inpcie_usb3_sub_system_init.This function uses spaces for indentation instead of tabs, which is inconsistent with kernel coding style and the rest of the driver. While this is vendor-derived code, consider running it through checkpatch or reformatting for consistency.
patch/u-boot/sunxi-dev-u-boot-a523/add-sun55i-spi-sunxi.patch (1)
46-59: Mixed indentation in FIFO reset timeout handling.The new FIFO reset timeout code block uses spaces for indentation instead of tabs, which is inconsistent with U-Boot coding style. The logic itself is correct - waiting for FIFO reset completion before proceeding with the transfer prevents potential data corruption.
Suggested fix for consistent indentation
- 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))), - 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))), + 1000); + if (ret) { + printf("ERROR: sun4i_spi: FIFO reset timeout\n"); + sun4i_spi_set_cs(bus, slave_plat->cs[0], false); + return ret; + } + }patch/kernel/archive/sunxi-6.18/patches.armbian/edit-kernel-cubie-a5e-dts.patch (4)
67-72: Minor formatting inconsistency in pin list.The spacing in the
pinsproperty is inconsistent:"PC2","PC4", "PC12"— missing space after"PC2".Suggested fix
spi0_pins: spi0-pins { - pins = "PC2","PC4", "PC12"; + pins = "PC2", "PC4", "PC12"; allwinner,pinmux = <4>; function = "spi0"; drive-strength = <10>; };
93-104: Inconsistent indentation and trailing whitespace.This block has mixed indentation (tabs vs spaces) and trailing whitespace on lines 95 and 98. This can cause issues with patch tools or style checks.
Suggested fix (normalized indentation)
&spi0 { pinctrl-0 = <&spi0_pins>, <&spi0_cs_pin>; - pinctrl-names = "default"; - status = "okay"; - w25q128: flash@0 { - compatible = "winbond,w25q128fw", "jedec,spi-nor"; - reg = <0>; - spi-max-frequency = <24000000>; - vcc-supply = <®_cldo1>; - status = "okay"; - }; + pinctrl-names = "default"; + status = "okay"; + + w25q128: flash@0 { + compatible = "winbond,w25q128fw", "jedec,spi-nor"; + reg = <0>; + spi-max-frequency = <24000000>; + vcc-supply = <®_cldo1>; + status = "okay"; + }; };
107-111: Trailing whitespace on line 107.Remove the trailing space after
&combophy {.Suggested fix
/* PCIE and USB Switch */ -&combophy { +&combophy { resets = <&ccu RST_BUS_PCIE_USB3>; phy_use_sel = <0>; /* 0:PCIE; 1:USB3 */ status = "okay"; };
113-120: LGTM with minor whitespace nit.PCIe configuration is correct. Line 118 has trailing whitespace that should be removed for consistency.
patch/kernel/archive/sunxi-6.18/patches.armbian/01-phy-pcie-usb3-add-driver.patch
Show resolved
Hide resolved
patch/kernel/archive/sunxi-6.18/patches.armbian/drv-iommu-sunxi-add-iommu-driver.patch
Show resolved
Hide resolved
patch/kernel/archive/sunxi-6.18/patches.armbian/drv-iommu-sunxi-add-iommu-driver.patch
Show resolved
Hide resolved
...hive/sunxi-6.18/patches.armbian/arm64-dts-sun55i-t527-orangepi-4a-enable-pcie-combophy.patch
Show resolved
Hide resolved
patch/u-boot/sunxi-dev-u-boot-a523/phy-allwinner-add-pcie-usb3-driver.patch
Show resolved
Hide resolved
patch/u-boot/sunxi-dev-u-boot-a523/phy-allwinner-add-pcie-usb3-driver.patch
Show resolved
Hide resolved
patch/u-boot/sunxi-dev-u-boot-a523/edit-orangepi-4a-defconfig.patch
Outdated
Show resolved
Hide resolved
patch/u-boot/sunxi-dev-u-boot-a523/edit-radxa-cubie-a5e-defconfig.patch
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 Fix all issues with AI agents
In
`@patch/kernel/archive/sunxi-6.18/patches.armbian/01-phy-pcie-usb3-add-driver.patch`:
- Around line 208-234: The combo_phy_mode_set function currently uses a
zero-initialized tmp so all "&=" clears are no-ops and disable doesn't assert
reset; change the logic to operate on the register value (val) or initialize tmp
= val, then set/clear PHY_USE_SEL based on combphy->user (clear for
PHY_USE_BY_PCIE and PHY_USE_BY_PCIE_USB3_U2, set for PHY_USE_BY_USB3), set/clear
PHY_CLK_SEL based on combphy->ref (clear for INTER_SIG_REF_CLK, set for
EXTER_DIF_REF_CLK), and ensure PHY_RSTN is set when enable is true and cleared
when enable is false; finally write the modified val back with
writel(COMBO_REG_PHYCTRL(combphy->phy_ctl)).
- Around line 865-896: sunxi_combphy_power_on() and sunxi_combphy_power_off()
ignore the return values from the USB3 helpers, masking regulator errors; modify
both functions (sunxi_combphy_power_on and sunxi_combphy_power_off) to capture
the int return from sunxi_combphy_usb3_power_on() /
sunxi_combphy_usb3_power_off() (which ultimately call
regulator_enable()/regulator_disable()) and if the return is negative return
that error to the caller instead of silently proceeding.
- Around line 942-981: The probe passes potential ERR_PTR clocks to
clk_set_parent which can dereference them; before calling clk_set_parent check
IS_ERR() on combphy->phyclk_ref and combphy->refclk_par (for the PCIe branch)
and on combphy->phyclk_cfg and combphy->cfgclk_par (for has_cfg_clk), and handle
failures by either returning an appropriate error from the probe or skipping the
parent-set when the clock is optional; update the probe code around the
clk_set_parent calls (reference combphy->phyclk_ref, combphy->refclk_par,
combphy->phyclk_cfg, combphy->cfgclk_par and the clk_set_parent calls) to
perform these IS_ERR checks and proper error handling.
In
`@patch/kernel/archive/sunxi-6.18/patches.armbian/03-iommu-add-sunxi-driver.patch`:
- Around line 2104-2117: The sysfs attributes created by
sun55i_iommu_sysfs_create(pdev, sunxi_iommu) are not removed if
iommu_device_sysfs_add(&sunxi_iommu->iommu, dev, NULL, dev_name(dev)) fails;
update the error path so you call sun55i_iommu_sysfs_remove(pdev, sunxi_iommu)
before cleaning up the clock and returning. Concretely, add a call to
sun55i_iommu_sysfs_remove(...) in the failure branch after detecting ret from
iommu_device_sysfs_add (or jump to a new err_sysfs_remove label that calls
sun55i_iommu_sysfs_remove then falls through to the existing err_clk cleanup) so
the three custom sysfs attributes are always unregistered on error.
- Around line 1256-1265: The loop assigns the int return of
sunxi_pgtable_delete_l2_tables() into u32 iova_tail_size which lets negative
error codes wrap and break the loop; change handling so you call
sunxi_pgtable_delete_l2_tables(sunxi_domain->pgtable, iova_start, iova_end) into
a signed int ret (or check if return < 0), and on ret < 0 handle the error
(unlock dt_lock with spin_unlock_irqrestore, perform any necessary cleanup) and
return or break instead of casting to u32; on success convert ret to a u32 size
and advance iova_start by that size as before.
- Around line 1407-1429: sun55i_iommu_of_xlate reads args->args[1]
unconditionally but the DT binding uses `#iommu-cells` = <1>, causing an
out-of-bounds read; update sun55i_iommu_of_xlate to check args->args_count (or
args->np provided cell count) before accessing args->args[1] and only assign
owner->flag when args->args_count > 1, otherwise set a safe default (e.g., 0) or
handle the missing flag accordingly while leaving owner->tlbid populated from
args->args[0].
- Around line 1022-1067: Add explicit bounds checking for master_id before
indexing sunxi_iommu_fault_notify_cbs and master_id_bitmap: in
sun55i_iommu_register_fault_cb validate master_id is within 0 ..
ARRAY_SIZE(sunxi_iommu_fault_notify_cbs)-1 (or a defined MAX_MASTER_ID) and
return or WARN_ON_INVALID otherwise; in sun55i_reset_device_iommu validate
master_id and that global_iommu_dev is non-NULL before using it; in
sun55i_enable_device_iommu validate master_id before touching
master_id_bitmap[master_id] and keep the existing iommu null check; use
ARRAY_SIZE() to compute the safe upper bound and log/WARN_ONCE on invalid
master_id to avoid out-of-bounds accesses.
In `@patch/u-boot/sunxi-dev-u-boot-a523/add-sun55i-pcie-driver.patch`:
- Around line 834-860: In sun55i_pcie_host_speed_change, the PCIe capability
base (offset) is found but not used when accessing LNKCTL2; update the
reads/writes that currently reference LINK_CONTROL2_LINK_STATUS2 directly to use
the discovered capability base (e.g., compute the register address as offset +
LINK_CONTROL2_LINK_STATUS2) so the LNKCTL2 updates target the correct capability
block (use the same pattern for both sun55i_pcie_readl_dbi and
sun55i_pcie_writel_dbi calls), leaving the dbi_ro_wr_en/dbi_ro_wr_dis logic
unchanged.
- Around line 940-942: In sun55i_pcie_plat_hw_init(), the unconditional
assignments pci->lanes = data->num_lanes and pci->link_gen =
data->max_link_speed overwrite values populated from DT; change this so the
of_data defaults are only applied when DT parsing failed (e.g., only assign
data->num_lanes to pci->lanes if pci->lanes is zero/invalid or similarly only
set pci->link_gen when DT did not provide a valid link_gen), or simply remove
these two assignments so DT-provided values remain authoritative; touch the
sun55i_pcie_plat_hw_init() function and the references to
data->num_lanes/data->max_link_speed to implement the guarded-assignment or
removal.
In `@patch/u-boot/sunxi-dev-u-boot-a523/add-sun55i-usb3-phy-driver.patch`:
- Around line 601-608: The code currently reads-modifies PCIE_BGR_REG and a
second register at 0x2001a84 into reg_value, but then unconditionally assigns
reg_value = 0x81000001 which discards the read/OR result; either preserve the
RMW or remove the pointless read: update the second sequence so that after
reg_value = readl(0x2001a84) and reg_value |= (1 << PCIE_REF_CLK_GATING) you
write that reg_value back (writel(reg_value, 0x2001a84)), or if the intended
value is the literal 0x81000001 remove the preceding readl/OR lines for
0x2001a84 entirely; adjust only the block using reg_value, PCIE_BGR_REG,
PCIE_BRG_REG_RST and PCIE_REF_CLK_GATING to implement one consistent choice.
- Around line 676-678: The probe currently treats failure to get
"select3v3-supply" as fatal (ret != 0); change this so the supply is optional
like the kernel driver: either call the optional API variant (e.g.,
device_get_supply_regulator_optional or devm_regulator_get_optional if
available) to populate combphy->select3v3_supply, or, if keeping
device_get_supply_regulator, detect the specific "not present" error (e.g.,
-ENODEV) and clear combphy->select3v3_supply and continue (return 0) instead of
failing probe; keep the existing error log for other failures but do not abort
probe when the supply is simply absent.
In `@patch/u-boot/sunxi-dev-u-boot-a523/edit-orangepi-4a-defconfig.patch`:
- Line 31: The config option has a typo: CONFIG_CMD_LSBK is not valid and should
be CONFIG_CMD_LSBLK; replace the incorrect symbol CONFIG_CMD_LSBK with
CONFIG_CMD_LSBLK in the defconfig entry so U-Boot enables the correct "list
block devices" command (update the line that currently reads CONFIG_CMD_LSBK=y
to CONFIG_CMD_LSBLK=y).
In `@patch/u-boot/sunxi-dev-u-boot-a523/edit-u-boot-orangepi-4a-dts.patch`:
- Around line 38-44: The spi0_pin_default node currently configures pins
("PC2","PC3","PC4","PC12") as GPIO inputs; change the node spi0_pin_default so
that the pins property remains the same but set function = "spi0" and
allwinner,pinmux = <4> (keep drive-strength and other properties) so the pins
are assigned to the SPI0 function rather than gpio_in.
- Around line 74-80: The &pcie node references a missing phandle reg_pcie_vcc3v3
via slot-3v3-supply; add a regulator node with the phandle label reg_pcie_vcc3v3
in the same u-boot DTS patch so the DT compiles. Create a regulator node
(phandle label reg_pcie_vcc3v3) using an appropriate compatible (e.g.,
fixed-regulator or the SoC regulator type used elsewhere), set regulator-name
and the correct voltage (3.3V) and enable/always-on properties as needed, and
ensure any supply phandle or parent supply matches existing u-boot regulator
naming so &pcie::slot-3v3-supply resolves.
🧹 Nitpick comments (9)
patch/u-boot/sunxi-dev-u-boot-a523/u-boot-sun55iw3-edge-dtsi.patch (2)
46-58: Drop non-standarddevice_typein SPI node.
Bindings for SPI controllers don’t definedevice_type, and it can confuse tooling. Consider removing it unless you can point to a binding that requires it.♻️ Proposed change
@@ - device_type = "spi0";
72-106: Verify default enablement and required properties for combophy/PCIe.
Please confirm against the upstream sun55i-a523.dtsi (or binding docs) whether these blocks should be default-enabled at the SoC level. If not, setstatus = "disabled"here and enable in board DTS. Also double-check required properties (e.g., resets/interrupts/lanes) for U-Boot’s driver expectations to avoid silent probe failures. Based on learnings, this is especially important if patches were rewritten.♻️ Possible adjustment (if upstream defaults to disabled)
@@ combophy: phy@4f00000 { @@ reset-names = "phy_rst"; + status = "disabled"; }; @@ pcie: pcie@4800000 { @@ power-domains = <&pck600 PD_PCIE>; + status = "disabled"; };patch/u-boot/sunxi-dev-u-boot-a523/add-spl-spi-support.patch (1)
173-182: Keep SPI FIFO comment in English for maintainability.Consider replacing the German note with a short English explanation (or drop it if redundant).
♻️ Suggested tweak
- /* HIER IST IHR WICHTIGER FIX: & 0xFF */ + /* A523 FIFO count uses 8-bit mask (0xFF) */patch/u-boot/sunxi-dev-u-boot-a523/add-sun55i-pcie-driver.patch (1)
1087-1097: Deduplicate ATU type macros to avoid redefinition warnings.
PCIE_ATU_TYPE_*andPCIE_ATU_FUNC_NUMare defined twice in the same header. Keeping a single definition avoids macro-redefine warnings and confusion.♻️ Suggested cleanup
-#define PCIE_ATU_TYPE_MEM (0x0 << 0) -#define PCIE_ATU_TYPE_IO (0x2 << 0) -#define PCIE_ATU_TYPE_CFG0 (0x4 << 0) -#define PCIE_ATU_TYPE_CFG1 (0x5 << 0) -#define PCIE_ATU_FUNC_NUM(pf) ((pf) << 20) ... -#define PCIE_ATU_FUNC_NUM(pf) ((pf) << 20)patch/kernel/archive/sunxi-6.18/patches.armbian/edit-kernel-cubie-a5e-dts.patch (1)
93-104: Inconsistent indentation in spi0 node.The
&spi0block uses mixed tabs and spaces for indentation. Lines 95-103 have leading spaces followed by tabs, which differs from the rest of the patch. This may cause issues with some DT parsers or make maintenance harder.🔧 Suggested fix for consistent indentation
&spi0 { pinctrl-0 = <&spi0_pins>, <&spi0_cs_pin>; - pinctrl-names = "default"; - status = "okay"; - w25q128: flash@0 { - compatible = "winbond,w25q128fw", "jedec,spi-nor"; - reg = <0>; - spi-max-frequency = <24000000>; - vcc-supply = <®_cldo1>; - status = "okay"; - }; + pinctrl-names = "default"; + status = "okay"; + w25q128: flash@0 { + compatible = "winbond,w25q128fw", "jedec,spi-nor"; + reg = <0>; + spi-max-frequency = <24000000>; + vcc-supply = <®_cldo1>; + status = "okay"; + }; };patch/u-boot/sunxi-dev-u-boot-a523/add-sun55i-usb3-phy-driver.patch (3)
58-58: Remove test header include.Including
<test/common.h>in a production driver is unusual and may pull in unnecessary test framework dependencies.🔧 Suggested fix
-#include <test/common.h>
88-94: Hardcoded CCMU base addresses.The CCMU base address and PCIE_BGR_REG are hardcoded. Consider obtaining these from device tree or platform data for better portability.
170-171: Inconsistent indentation throughout the driver.Multiple sections use spaces instead of tabs for indentation (lines 170-171, 187-188, 628-638). This should be consistent with kernel/U-Boot coding style (tabs).
Also applies to: 187-188, 628-638
patch/u-boot/sunxi-dev-u-boot-a523/add-sun55i-spi-sunxi.patch (1)
46-59: Inconsistent indentation in FIFO reset wait block.The new code block uses spaces for indentation instead of tabs, inconsistent with the surrounding code style.
🔧 Suggested fix for consistent indentation
- 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))), - 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))), + 1000); + if (ret) { + printf("ERROR: sun4i_spi: FIFO reset timeout\n"); + sun4i_spi_set_cs(bus, slave_plat->cs[0], false); + return ret; + } + }
patch/kernel/archive/sunxi-6.18/patches.armbian/01-phy-pcie-usb3-add-driver.patch
Show resolved
Hide resolved
patch/kernel/archive/sunxi-6.18/patches.armbian/01-phy-pcie-usb3-add-driver.patch
Show resolved
Hide resolved
patch/kernel/archive/sunxi-6.18/patches.armbian/01-phy-pcie-usb3-add-driver.patch
Show resolved
Hide resolved
patch/kernel/archive/sunxi-6.18/patches.armbian/drv-iommu-sunxi-add-iommu-driver.patch
Show resolved
Hide resolved
patch/kernel/archive/sunxi-6.18/patches.armbian/03-iommu-add-sunxi-driver.patch
Show resolved
Hide resolved
patch/u-boot/sunxi-dev-u-boot-a523/pci-sunxi-enable-pcie-support.patch
Outdated
Show resolved
Hide resolved
patch/u-boot/sunxi-dev-u-boot-a523/phy-allwinner-add-pcie-usb3-driver.patch
Outdated
Show resolved
Hide resolved
patch/u-boot/sunxi-dev-u-boot-a523/edit-orangepi-4a-defconfig.patch
Outdated
Show resolved
Hide resolved
patch/u-boot/sunxi-dev-u-boot-a523/edit-u-boot-orangepi-4a-dts.patch
Outdated
Show resolved
Hide resolved
...boot/sunxi-dev-u-boot-a523/arm64-dts-sun55i-t527-orangepi-4a-enable-spi0-pcie-combophy.patch
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 Fix all issues with AI agents
In `@config/kernel/linux-sunxi64-edge.config`:
- Around line 3058-3062: The config currently enables CONFIG_INIT_STACK_NONE
which disables kernel stack initialization; confirm this hardening/performance
trade-off and if unintended change the option to an initialization mode such as
CONFIG_INIT_STACK_ALL_ZERO (or CONFIG_INIT_STACK_ALL_PATTERN) by replacing the
CONFIG_INIT_STACK_NONE entry with the desired
CONFIG_INIT_STACK_ALL_ZERO/CONFIG_INIT_STACK_ALL_PATTERN symbol so stack memory
is zeroed/pattern-initialized rather than left uninitialized.
In
`@patch/kernel/archive/sunxi-6.18/patches.armbian/01-phy-pcie-usb3-add-driver.patch`:
- Around line 208-233: In combo_phy_mode_set, tmp is incorrectly initialized to
0 so clears (tmp &= ~PHY_USE_SEL / PHY_CLK_SEL) do nothing and disable may not
clear PHY_RSTN; fix by initializing tmp from the current register value (val) or
masking the relevant bits from val (e.g. tmp = val & (PHY_USE_SEL | PHY_CLK_SEL
| PHY_RSTN)), then update tmp based on combphy->user and combphy->ref using |=
and &= ~ as currently done, and finally apply the enable/disable by setting or
clearing PHY_RSTN in val (val |= tmp for enable, val &= ~PHY_RSTN for disable)
before writel to COMBO_REG_PHYCTRL; use readl(COMBO_REG_PHYCTRL(...)) and
writel(...) on val to ensure clears take effect.
- Around line 865-881: sunxi_combphy_power_on currently ignores failures from
sunxi_combphy_usb3_power_on and always returns 0; change it to capture the
return value from sunxi_combphy_usb3_power_on (e.g., int ret =
sunxi_combphy_usb3_power_on(combphy)) and if ret < 0 return that error to the
PHY core (optionally log with dev_err before returning); keep the existing
behavior for PHY_TYPE_PCIE and the default error path.
- Around line 955-982: The code currently calls clk_set_parent with potential
ERR_PTRs from devm_clk_get; update the blocks that get combphy->refclk_par,
combphy->phyclk_cfg and combphy->cfgclk_par so that each devm_clk_get call is
followed by an IS_ERR() check that returns PTR_ERR(...) immediately (propagating
errors like -EPROBE_DEFER) instead of only logging with dev_dbg; also ensure
combphy->phyclk_ref and combphy->phyclk_cfg are verified non-ERR before calling
clk_set_parent so clk_set_parent never receives ERR_PTRs.
In
`@patch/kernel/archive/sunxi-6.18/patches.armbian/03-iommu-add-sunxi-driver.patch`:
- Around line 2080-2086: The probe currently calls of_clk_get_by_name and checks
IS_ERR(sunxi_iommu->clk) but overwrites sunxi_iommu->clk with NULL before
calling PTR_ERR, causing ret to be 0; change the sequence so you capture the
error code immediately (ret = PTR_ERR(sunxi_iommu->clk)) when IS_ERR() is true,
then optionally set sunxi_iommu->clk = NULL, log via dev_dbg/dev_err, and goto
err_clk; update the block around of_clk_get_by_name, IS_ERR, PTR_ERR,
sunxi_iommu->clk and err_clk to preserve the real error code returned by
PTR_ERR.
- Around line 1407-1432: The sun55i_iommu_of_xlate function reads args->args[0]
and args->args[1] without validating args_count which can lead to out-of-bounds
indexing (master_id_bitmap has 7 entries); update sun55i_iommu_of_xlate to first
check args->args_count is at least 2 and then validate that args->args[0] (the
master_id) is less than 7 before using it or storing it in owner->tlbid; return
-EINVAL if these checks fail to prevent malformed device-tree inputs from
causing out-of-bounds access.
In `@patch/u-boot/sunxi-dev-u-boot-a523/add-sun55i-pcie-driver.patch`:
- Around line 370-405: The failure path after calling
generic_phy_power_on(&pci->phy) currently jumps to err_assert_reset and skips
calling generic_phy_power_off(), potentially leaving the PHY powered; change the
goto on the generic_phy_power_on() error to jump to err_power_off_phy (the label
that calls generic_phy_power_off(&pci->phy)) so the PHY is properly powered off
on failure, and verify subsequent labels (err_power_off_phy and
err_assert_reset) still handle rst_gpio and drvdata->need_pcie_rst correctly.
- Around line 940-942: Remove the unconditional overrides of DT-provided lane
and speed values: stop assigning pci->lanes = data->num_lanes and pci->link_gen
= data->max_link_speed in the current init path so DT `num-lanes` /
`max-link-speed` read earlier by sun55i_pcie_plat_hw_init() are preserved;
either delete these two assignments or guard them so they only apply when DT
values are not present (e.g., check for zero/invalid pci->lanes or pci->link_gen
before writing), referencing the pci->lanes/pci->link_gen fields and
data->num_lanes/data->max_link_speed in your change.
In `@patch/u-boot/sunxi-dev-u-boot-a523/add-sun55i-usb3-phy-driver.patch`:
- Around line 593-615: In pcie_usb3_sub_system_init the code reads the CCU
register at 0x2001a84 into reg_value, ORs in (1 << PCIE_REF_CLK_GATING) and then
immediately overwrites reg_value with 0x81000001 which clobbers unrelated bits;
instead preserve the original read value and only set the needed bits: remove
the reg_value = 0x81000001 assignment and write back the modified reg_value (or
OR in any other required masks) to the same register (use the existing
readl/writel sequence in pcie_usb3_sub_system_init and prefer a named register
macro instead of the hardcoded 0x2001a84 if available).
- Around line 507-533: In combo_phy_mode_set, tmp is initialized to 0 so
operations like tmp &= ~PHY_USE_SEL/PHY_CLK_SEL do nothing; instead operate on
the register value (val) when toggling mode bits and reset: start from val =
readl(COMBO_REG_PHYCTRL(combphy->phy_ctl)), then clear or set PHY_USE_SEL and
PHY_CLK_SEL on val based on combphy->user and combphy->ref, and finally set or
clear PHY_RSTN on val according to the enable parameter before writel(val,
COMBO_REG_PHYCTRL(combphy->phy_ctl)); this ensures correct bit updates across
re-inits.
In `@patch/u-boot/sunxi-dev-u-boot-a523/edit-orangepi-4a-defconfig.patch`:
- Line 31: Replace the mistyped Kconfig symbol CONFIG_CMD_LSBK with the correct
CONFIG_CMD_LSBLK in the defconfig so the lsblk command is built, and ensure the
block device support symbol BLK is enabled (add or verify CONFIG_BLK=y) in the
same defconfig; update references to CONFIG_CMD_LSBK (if any) to
CONFIG_CMD_LSBLK and confirm no other occurrences of the typo remain.
In `@patch/u-boot/sunxi-dev-u-boot-a523/edit-u-boot-orangepi-4a-dts.patch`:
- Around line 74-79: The &pcie node is missing the required switch-sel-gpios
property which sun55i_pcie_plat_hw_init() expects (see
add-sun55i-pcie-driver.patch lines ~444-448); add a switch-sel-gpios = <&pio 1 6
GPIO_ACTIVE_HIGH> (or the correct Orangepi-4a GPIO after verification) to the
&pcie block alongside reset-gpios/wake-gpios/num-lanes/slot-3v3-supply and
ensure the GPIO phandle and flags use the same GPIO controller symbol (&pio)
convention used elsewhere in the DTS (compare radxa-cubie-a5e for reference).
- Around line 39-44: The spi0_pin_default pinctrl group currently sets function
= "gpio_in", leaving SPI pins as GPIOs; change the pinctrl group referenced by
&spi0 (spi0_pin_default) to use function = "spi0" so the pins (pins =
"PC2","PC3","PC4","PC12") are muxed into SPI mode for the SPI NOR device; update
only the function string in the spi0_pin_default node to match Allwinner pinctrl
conventions used elsewhere.
♻️ Duplicate comments (1)
patch/kernel/archive/sunxi-6.18/patches.armbian/edit-kernel-orangepi_4a-dts.patch (1)
22-35: Looks good; see binding ID verification noted earlier.
🧹 Nitpick comments (3)
patch/u-boot/sunxi-dev-u-boot-a523/edit-u-boot-radxa-cubie-a5e-dts.patch (2)
80-100: Minor: Inconsistent formatting and unconventional node naming.Two minor observations:
- Line 81: Inconsistent spacing in pins list (
"PC2","PC4", "PC12"- missing space after first comma).- Line 95: Node name
spi0@2uses unit-address notation without a correspondingregproperty. Consider using a descriptive name likespi0-pins-sleepinstead.🔧 Suggested formatting fixes
spi0_pins: spi0-pins { - pins = "PC2","PC4", "PC12"; + pins = "PC2", "PC4", "PC12"; allwinner,pinmux = <4>; function = "spi0"; drive-strength = <10>; };- spi0_pins_sleep: spi0@2 { + spi0_pins_sleep: spi0-pins-sleep { pins = "PC2","PC3","PC4", "PC12"; allwinner,pinmux = <0>; function = "gpio_in"; drive-strength = <10>; };
113-119: Minor: Property naming inconsistency.This node uses
allwinner,pins(line 114) while other pin definitions in the patch usepins(e.g., lines 72, 81, 88, 96). Both work for Allwinner device trees, but consistency would improve readability.patch/u-boot/sunxi-dev-u-boot-a523/add-spl-spi-support.patch (1)
176-182: Replace the German comment with a neutral English note.Keep comments consistent and clear for all contributors.
✏️ Suggested edit
- /* HIER IST IHR WICHTIGER FIX: & 0xFF */ + /* SUN55I RX FIFO count uses an 8-bit field */
patch/kernel/archive/sunxi-6.18/patches.armbian/01-phy-pcie-usb3-add-driver.patch
Show resolved
Hide resolved
patch/kernel/archive/sunxi-6.18/patches.armbian/01-phy-pcie-usb3-add-driver.patch
Show resolved
Hide resolved
patch/kernel/archive/sunxi-6.18/patches.armbian/01-phy-pcie-usb3-add-driver.patch
Show resolved
Hide resolved
patch/kernel/archive/sunxi-6.18/patches.armbian/drv-iommu-sunxi-add-iommu-driver.patch
Show resolved
Hide resolved
patch/u-boot/sunxi-dev-u-boot-a523/phy-allwinner-add-pcie-usb3-driver.patch
Show resolved
Hide resolved
patch/u-boot/sunxi-dev-u-boot-a523/phy-allwinner-add-pcie-usb3-driver.patch
Show resolved
Hide resolved
patch/u-boot/sunxi-dev-u-boot-a523/edit-orangepi-4a-defconfig.patch
Outdated
Show resolved
Hide resolved
patch/u-boot/sunxi-dev-u-boot-a523/edit-u-boot-orangepi-4a-dts.patch
Outdated
Show resolved
Hide resolved
...boot/sunxi-dev-u-boot-a523/arm64-dts-sun55i-t527-orangepi-4a-enable-spi0-pcie-combophy.patch
Show resolved
Hide resolved
rpardini
changed the title
EvilOlaf
left a comment
EvilOlaf
left a comment
There was a problem hiding this comment.
- I've noticed the patches were all marked as John Doe john.doe@somewhere.on.planet. Please replace with something proper.
- If the patches come from mailing list and are pending, consider moving to
patches.backports. - Did not take a closer look yet, but make sure your patches neiter have cross-patch dependencies nor introduce new ones with existing ones.
- There are quite a few changes to the kernel config. Are those necessary? Were those automatically introduced? Do we need nfs support buit-in (just for example)?
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In
`@patch/kernel/archive/sunxi-6.18/patches.armbian/01-phy-pcie-usb3-add-driver.patch`:
- Around line 1092-1095: The probe currently calls
pcie_usb3_sub_system_init(pdev), warns on error and continues which leaves a
non-functional PHY and prevents deferral; change the probe to check ret after
calling pcie_usb3_sub_system_init and return ret (propagate the error, e.g.,
-EPROBE_DEFER) instead of calling dev_warn and continuing so the driver probe
fails/defers properly; update the block referencing pcie_usb3_sub_system_init,
ret, pdev and dev to return the error from the probe path (or route to the probe
error cleanup) when ret is non-zero.
- Around line 949-957: The init path must not continue with ERR_PTR resets:
after calling devm_reset_control_get for combphy->reset and combphy->noppu_reset
(only when combphy->drvdata->need_noppu_rst is true), check IS_ERR() and return
the appropriate error (use PTR_ERR(combphy->reset) or
PTR_ERR(combphy->noppu_reset)) instead of just dev_dbg; this prevents calling
reset_control_deassert() on error pointers and aligns with the existing error
handling on the exit path.
- Around line 315-342: The clk_set_rate() calls must be guarded against ERR_PTR
clocks to avoid dereferencing error pointers: before calling
clk_set_rate(combphy->phyclk_ref, ...) and clk_set_rate(combphy->phyclk_cfg,
...), check !IS_ERR(combphy->phyclk_ref) and !IS_ERR(combphy->phyclk_cfg)
respectively (and keep the existing already_enabled checks), so only valid
clocks are passed to clk_set_rate; leave the subsequent clk_prepare_enable() and
error paths (err_assert_noppu / err_disable_phyclk_ref) unchanged.
In
`@patch/kernel/archive/sunxi-6.18/patches.armbian/03-iommu-add-sunxi-driver.patch`:
- Around line 1238-1271: The function sunxi_iommu_unmap wrongly uses "break"
(invalid outside a loop) and fails to release dt_lock on error; change the error
branch after calling sunxi_pgtable_delete_l2_tables to release the spinlock and
return an appropriate unmapped size (e.g., 0) instead of breaking: replace the
"if (iova_tail_size < 0) break;" with "if (iova_tail_size < 0) {
spin_unlock_irqrestore(&sunxi_domain->dt_lock, flags); return 0; }" and ensure
the subsequent sun55i_ptw_cache_invalid and the final spin_unlock_irqrestore
remain executed only for non-error paths so the lock is always released.
In `@patch/u-boot/sunxi-dev-u-boot-a523/u-boot-sun55iw3-edge-dtsi.patch`:
- Around line 46-59: Fix the spi0 device tree node formatting and properties:
convert the leading spaces to tabs to match DT style, remove the non-standard
device_type = "spi0" property from the spi0 node, and reorder dma-names to match
the dmas phandle order (set dma-names = "rx", "tx" to correspond to dmas = <&dma
22>, <&dma 22>); keep the rest of the properties (`#address-cells`, `#size-cells`,
compatible, reg, interrupts, clocks, clock-names, resets, dmas, status)
unchanged.
♻️ Duplicate comments (17)
patch/u-boot/sunxi-dev-u-boot-a523/add-spl-spi-support.patch (1)
175-180: German comment should be in English.The comment "HIER IST IHR WICHTIGER FIX: & 0xFF" should be replaced with an English explanation of why A523 uses
0xFFmask versus0x7Ffor other variants.Suggested fix
- /* HIER IST IHR WICHTIGER FIX: & 0xFF */ + /* SUN55I uses 8-bit RX FIFO counter (0xFF mask) vs 7-bit (0x7F) for others */patch/u-boot/sunxi-dev-u-boot-a523/add-sun55i-usb3-phy-driver.patch (3)
507-533:combo_phy_mode_set()bit manipulation is ineffective.
tmpis initialized to 0, so operations liketmp &= ~PHY_USE_SELhave no effect (clearing a bit in 0 yields 0). The disable path (val &= ~tmp) also becomes a no-op whentmpis 0. This can leave the PHY in an incorrect mode.🐛 Proposed fix
static void combo_phy_mode_set(struct sun55i_combphy *combphy, bool enable) { - u32 val, tmp = 0; + u32 val; val = readl(COMBO_REG_PHYCTRL(combphy->phy_ctl)); + val &= ~(PHY_USE_SEL | PHY_CLK_SEL | PHY_RSTN); - if (combphy->user == PHY_USE_BY_PCIE) - tmp &= ~PHY_USE_SEL; - else if (combphy->user == PHY_USE_BY_USB3) - tmp |= PHY_USE_SEL; - else if (combphy->user == PHY_USE_BY_PCIE_USB3_U2) - tmp &= ~PHY_USE_SEL; + if (combphy->user == PHY_USE_BY_USB3) + val |= PHY_USE_SEL; - if (combphy->ref == INTER_SIG_REF_CLK) - tmp &= ~PHY_CLK_SEL; - else if (combphy->ref == EXTER_DIF_REF_CLK) - tmp |= PHY_CLK_SEL; + if (combphy->ref == EXTER_DIF_REF_CLK) + val |= PHY_CLK_SEL; - if (enable) { - tmp |= PHY_RSTN; - val |= tmp; - } else { - tmp &= ~PHY_RSTN; - val &= ~tmp; - } + if (enable) + val |= PHY_RSTN; + writel(val, COMBO_REG_PHYCTRL(combphy->phy_ctl)); }
601-608: Hardcoded register write clobbers read-modify-write logic.Line 607 unconditionally sets
reg_value = 0x81000001, making the precedingreadl()and|=operations pointless. Either preserve the RMW pattern or remove the unnecessary read.🐛 Proposed fix - preserve RMW
reg_value = readl(0x2001a84); reg_value |= (1 << PCIE_REF_CLK_GATING); - reg_value = 0x81000001; writel(reg_value, 0x2001a84);
676-678:select3v3-supplyshould be optional.The kernel driver uses
devm_regulator_get_optional()for this supply. The U-Boot probe fails if the regulator is missing, but not all boards may have this supply. Consider treating-ENODEVor-ENOENTas non-fatal.Suggested fix
ret = device_get_supply_regulator(dev, "select3v3-supply", &combphy->select3v3_supply); - if (ret) + if (ret && ret != -ENODEV && ret != -ENOENT) { + dev_err(dev, "Failed to get select3v3-supply: %d\n", ret); return ret; + }patch/u-boot/sunxi-dev-u-boot-a523/edit-u-boot-orangepi-4a-dts.patch (1)
63-69: Missingreg_pcie_vcc3v3regulator definition andswitch-sel-gpios.The
&pcienode references<®_pcie_vcc3v3>but this regulator is not defined in this patch or visible in the context. Additionally, per theadd-sun55i-pcie-driver.patch, the driver'ssun55i_pcie_plat_hw_init()requiresswitch-sel-gpiosfor proper operation.Since Orange Pi 4A testing wasn't possible due to lack of hardware, these may need verification once hardware is available.
Items to address
- Add
reg_pcie_vcc3v3fixed-regulator node to this DTS or reference an existing regulator.- Add
switch-sel-gpiosproperty to&pcienode (seeedit-u-boot-radxa-cubie-a5e-dts.patchfor reference).patch/kernel/archive/sunxi-6.18/patches.armbian/01-phy-pcie-usb3-add-driver.patch (3)
208-233: Fix PHY mode bit updates—clears never take effect.
tmpstarts at 0, so the clear paths are no-ops andPHY_RSTNisn’t reliably cleared on disable. This leaves stale mode/clock bits and can keep the PHY enabled.🐛 Proposed fix
- u32 val, tmp = 0; + u32 val; val = readl(COMBO_REG_PHYCTRL(combphy->phy_ctl)); - if (combphy->user == PHY_USE_BY_PCIE) - tmp &= ~PHY_USE_SEL; - else if (combphy->user == PHY_USE_BY_USB3) - tmp |= PHY_USE_SEL; - else if (combphy->user == PHY_USE_BY_PCIE_USB3_U2) - tmp &= ~PHY_USE_SEL; + if (combphy->user == PHY_USE_BY_PCIE || + combphy->user == PHY_USE_BY_PCIE_USB3_U2) + val &= ~PHY_USE_SEL; + else if (combphy->user == PHY_USE_BY_USB3) + val |= PHY_USE_SEL; - if (combphy->ref == INTER_SIG_REF_CLK) - tmp &= ~PHY_CLK_SEL; - else if (combphy->ref == EXTER_DIF_REF_CLK) - tmp |= PHY_CLK_SEL; + if (combphy->ref == INTER_SIG_REF_CLK) + val &= ~PHY_CLK_SEL; + else if (combphy->ref == EXTER_DIF_REF_CLK) + val |= PHY_CLK_SEL; - if (enable) { - tmp |= PHY_RSTN; - val |= tmp; - } else { - tmp &= ~PHY_RSTN; - val &= ~tmp; - } + if (enable) + val |= PHY_RSTN; + else + val &= ~PHY_RSTN;
958-985: Avoid clk_set_parent on ERR_PTR clocks.
Passing error pointers toclk_set_parent()can crash and masks-EPROBE_DEFER.🔧 Suggested guard pattern
combphy->refclk_par = devm_clk_get(&pdev->dev, "refclk_par"); - if (IS_ERR(combphy->refclk_par)) - dev_dbg(dev, "failed to get refclk_par\n"); + if (IS_ERR(combphy->refclk_par)) + return PTR_ERR(combphy->refclk_par); + if (IS_ERR(combphy->phyclk_ref)) + return PTR_ERR(combphy->phyclk_ref);
868-901: Propagate USB3 power sequencing errors.
sunxi_combphy_usb3_power_on/off()can fail (regulator errors) but results are ignored.🔧 Proposed fix
case PHY_TYPE_USB3: - sunxi_combphy_usb3_power_on(combphy); - break; + return sunxi_combphy_usb3_power_on(combphy); @@ case PHY_TYPE_USB3: - sunxi_combphy_usb3_power_off(combphy); - break; + return sunxi_combphy_usb3_power_off(combphy);patch/kernel/archive/sunxi-6.18/patches.armbian/03-iommu-add-sunxi-driver.patch (5)
1043-1053: Add NULL/bounds guards in sun55i_reset_device_iommu.
global_iommu_devcan be NULL andmaster_idis unchecked, risking NULL deref/OOB.🔧 Proposed fix
void sun55i_reset_device_iommu(unsigned int master_id) { unsigned int regval; struct sunxi_iommu_dev *iommu = global_iommu_dev; + if (!iommu || master_id >= ARRAY_SIZE(master_id_bitmap)) + return; regval = sunxi_iommu_read(iommu, IOMMU_RESET_REG);
1411-1433: Validate iommus phandle args_count before args[1].
The binding advertises#iommu-cells = <1>; readingargs[1]is out-of-bounds.🔧 Proposed fix
static int sun55i_iommu_of_xlate(struct device *dev, const struct of_phandle_args *args) { + if (args->args_count < 1) + return -EINVAL; ... if (!owner) { owner = kzalloc(sizeof(*owner), GFP_KERNEL); if (!owner) return -ENOMEM; owner->tlbid = args->args[0]; - owner->flag = args->args[1]; + owner->flag = (args->args_count > 1) ? args->args[1] : 0;
2084-2089: Preserve PTR_ERR before NULLing clock.
Settingclk = NULLbeforePTR_ERR()makesretbecome 0, masking the real error.🔧 Proposed fix
sunxi_iommu->clk = of_clk_get_by_name(dev->of_node, "iommu"); if (IS_ERR(sunxi_iommu->clk)) { - sunxi_iommu->clk = NULL; - dev_dbg(dev, "Unable to find clock\n"); - ret = PTR_ERR(sunxi_iommu->clk); + ret = PTR_ERR(sunxi_iommu->clk); + sunxi_iommu->clk = NULL; + dev_dbg(dev, "Unable to find clock\n"); goto err_clk; }
2108-2114: Remove custom sysfs attrs if iommu_device_sysfs_add fails.
Otherwise the three custom attributes are left behind on error.🔧 Proposed fix
ret = iommu_device_sysfs_add(&sunxi_iommu->iommu, dev, NULL, dev_name(dev)); if (ret) { dev_err(dev, "Failed to register iommu in sysfs\n"); + sun55i_iommu_sysfs_remove(pdev); clk_disable_unprepare(sunxi_iommu->clk); goto err_clk; }
2180-2195: Free identity_domain on remove.
The identity domain allocated in probe is leaked on driver unload.🔧 Proposed fix
static void sun55i_iommu_remove(struct platform_device *pdev) { struct sunxi_iommu_dev *sunxi_iommu = platform_get_drvdata(pdev); struct iommu_resv_region *entry, *next; + if (sunxi_iommu->identity_domain) + iommu_domain_free(sunxi_iommu->identity_domain);patch/u-boot/sunxi-dev-u-boot-a523/add-sun55i-pcie-driver.patch (4)
580-618: CFG0/CFG1 type selection is inverted.This issue was previously flagged. The current logic uses CFG0 for
bus != 0and CFG1 forbus == 0, which is backwards for DesignWare PCIe controllers. CFG0 should be used for the immediate downstream bus, and CFG1 for buses beyond that.
834-860: LNKCTL2 access doesn't use discovered capability offset.This issue was previously flagged. The code finds the PCIe capability offset but then accesses
LINK_CONTROL2_LINK_STATUS2at a hardcoded address instead ofoffset + PCI_EXP_LNKCTL2.
940-942: DT-provided num-lanes and max-link-speed values are overwritten.This issue was previously flagged.
sun55i_pcie_plat_hw_init()parses DT values, but these lines unconditionally overwrite them with static defaults fromof_data.
370-378: PHY power-on failure doesn't properly clean up.This issue was previously flagged. If
generic_phy_power_on()fails, the code jumps toerr_assert_reset, skipping the PHY power-off call.
🧹 Nitpick comments (9)
patch/kernel/archive/sunxi-6.18/patches.armbian/edit-kernel-sun55-dtsi.patch (1)
64-77: Property naming convention:phy_refclk_seluses underscores.Device tree properties conventionally use kebab-case (e.g.,
phy-refclk-sel) rather than snake_case. While this may be intentional to match vendor driver expectations, verify the driver binding expects this exact property name.patch/u-boot/sunxi-dev-u-boot-a523/add-sun55i-spi-sunxi.patch (1)
46-59: FIFO reset timeout handling looks correct but has mixed indentation.The timeout logic properly polls for FIFO reset completion and handles errors by deasserting CS. However, the indentation uses spaces instead of tabs, which is inconsistent with U-Boot coding style. The kernel/U-Boot use tabs for indentation.
Indentation fix example
- if (priv->variant->has_bsr) { - u32 reg; - int ret; + if (priv->variant->has_bsr) { + u32 reg; + int ret;patch/u-boot/sunxi-dev-u-boot-a523/add-spl-spi-support.patch (1)
160-198: Inconsistent indentation insunxi_spi0_read_datafunction body.The function body uses 4-space indentation instead of tabs, which is inconsistent with U-Boot/kernel coding style. While functional, this should be corrected for consistency.
patch/u-boot/sunxi-dev-u-boot-a523/add-sun55i-usb3-phy-driver.patch (1)
170-171: Inconsistent indentation: spaces instead of tabs.Several struct members and variables use 4-space indentation instead of tabs, which is inconsistent with U-Boot coding style.
Also applies to: 187-188
config/kernel/linux-sunxi64-edge.config (1)
1141-1141: SPI_DEBUG enabled—confirm this is intentional for production images.💡 Optional change
-CONFIG_SPI_DEBUG=y +# CONFIG_SPI_DEBUG is not setpatch/u-boot/sunxi-dev-u-boot-a523/u-boot-sun55iw3-edge-dtsi.patch (1)
72-82: Minor formatting issue in combophy node.Line 79 is missing a space after the comma in
clock-names:"phyclk_ref","refclk_par"should be"phyclk_ref", "refclk_par".patch/u-boot/sunxi-dev-u-boot-a523/add-sun55i-pcie-driver.patch (3)
775-779: Debug printf statements should be removed or guarded.These verbose debug prints in
sun55i_pcie_host_link_up_status()will output on every link status check during boot. Consider removing them or guarding with a debug macro.♻️ Suggested fix
- printf(" Link Status: 0x%08x\n", val); - printf(" RDLH_LINK_UP: %d\n", !!(val & RDLH_LINK_UP)); - printf(" SMLH_LINK_UP: %d\n", !!(val & SMLH_LINK_UP)); - printf(" LINK_SPEED: %d\n", (val >> 16) & 0xF); - printf(" LINK_WIDTH: %d\n", (val >> 20) & 0x3F); + debug(" Link Status: 0x%08x\n", val); + debug(" RDLH_LINK_UP: %d\n", !!(val & RDLH_LINK_UP)); + debug(" SMLH_LINK_UP: %d\n", !!(val & SMLH_LINK_UP)); + debug(" LINK_SPEED: %d\n", (val >> 16) & 0xF); + debug(" LINK_WIDTH: %d\n", (val >> 20) & 0x3F);
684-690: Inconsistent indentation in write_config.Line 687 is missing proper indentation for
size_len = 2;inside theelse ifblock.♻️ Suggested fix
if (size == PCI_SIZE_8) size_len = 1; else if (size == PCI_SIZE_16) - size_len = 2; + size_len = 2; else if (size == PCI_SIZE_32) size_len = 4;
98-120: Inconsistent indentation style in cfg_read.The
sun55i_pcie_cfg_read()function uses spaces for indentation while the rest of the file uses tabs. This should be consistent with the codebase style.
patch/kernel/archive/sunxi-6.18/patches.armbian/01-phy-pcie-usb3-add-driver.patch
Outdated
Show resolved
Hide resolved
patch/kernel/archive/sunxi-6.18/patches.armbian/01-phy-pcie-usb3-add-driver.patch
Show resolved
Hide resolved
patch/kernel/archive/sunxi-6.18/patches.armbian/01-phy-pcie-usb3-add-driver.patch
Show resolved
Hide resolved
patch/kernel/archive/sunxi-6.18/patches.armbian/drv-iommu-sunxi-add-iommu-driver.patch
Show resolved
Hide resolved
patch/u-boot/sunxi-dev-u-boot-a523/arm64-dtsi-sun55i-add-spi0-pcie-combophy-nodes.patch
Show resolved
Hide resolved
|
I removed all unnecessary changes to the config, so the only ones added are the ones needed by my patches. |
|
building fails |
rvdr
requested review from
ColorfulRhino,
HeyMeco,
NicoD-SBC,
SuperKali,
adeepn,
ahoneybun,
amazingfate,
brentr,
catalinii,
chainsx,
efectn,
joekhoobyar,
lanefu and
prahal
as code owners
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@patch/kernel/archive/sunxi-6.18/patches.armbian/drv-phy-allwinner-add-pcie-usb3-driver.patch`:
- Around line 436-450: sunxi_inno_combophy_power_event is registered on an
atomic notifier chain but calls sleepable APIs; replace the atomic notifier with
a blocking notifier by changing the notifier head declaration to use
BLOCKING_NOTIFIER_HEAD (instead of ATOMIC_NOTIFIER_HEAD) and update the three
sites that register/unregister this notifier to use
blocking_notifier_chain_register and blocking_notifier_chain_unregister (or the
equivalent blocking registration APIs) for the pwr_nb notifier_block on struct
sunxi_combphy so the callback runs in a context that allows sleepable calls.
- Around line 965-1001: The code currently treats failures from devm_clk_get()
and clk_set_parent() as generic -EINVAL, which hides -EPROBE_DEFER; update the
error handling in the PCI/USB3 block (symbols: combphy->refclk_par,
combphy->phyclk_ref, combphy->phyclk_cfg, combphy->cfgclk_par, devm_clk_get,
clk_set_parent) to call dev_err_probe(&pdev->dev, PTR_ERR(clk_ptr), "failed to
get %s\n", "<clock-name>") when devm_clk_get() returns an error and to use
dev_err_probe(&pdev->dev, PTR_ERR(ret), "failed to set %s parent\n",
"<clock-name>") when clk_set_parent() fails so the real error code (including
-EPROBE_DEFER) is propagated; apply the same change to the has_pcie_axi_clk
block where clk_set_parent() is checked.
🧹 Nitpick comments (4)
patch/u-boot/sunxi-dev-u-boot-a523/phy-allwinner-add-pcie-usb3-driver.patch (3)
345-350: Remove commented-out dead code.The
sun55i_combphy_usb3_exitfunction is entirely commented out. Consider removing it to reduce clutter. If exit functionality is needed later, it can be implemented at that time.
597-604: Consider defining a constant for the CCU clock register address.The address
0x2001a84appears to be a CCU register (PCIe reference clock gating). For consistency with other register definitions in this file (likePCIE_BGR_REG), consider defining this as a named constant:`#define` PCIE_REF_CLK_REG (SUNXI_CCM_BASE + 0xa84)Also note the mixed indentation (tabs and spaces) in this block—U-Boot style prefers tabs.
613-655: Indentation uses spaces instead of tabs.This function uses 4-space indentation, but U-Boot coding style requires tabs. Consider fixing the indentation to maintain consistency.
The error handling with proper cleanup (asserting reset and disabling regulator on failure) looks good.
config/kernel/linux-sunxi64-edge.config (1)
1141-1141: Confirm SPI_DEBUG is intended for shipping configs.Line 1141 enables
CONFIG_SPI_DEBUG; if this was only for bring‑up, consider dropping it to avoid log noise/perf overhead in edge builds.
patch/kernel/archive/sunxi-6.18/patches.armbian/drv-phy-allwinner-add-pcie-usb3-driver.patch
Show resolved
Hide resolved
patch/kernel/archive/sunxi-6.18/patches.armbian/drv-phy-allwinner-add-pcie-usb3-driver.patch
Show resolved
Hide resolved
rvdr
force-pushed
the
spi-nvme-patches
branch
from
52db80a to
170ebf2
Compare
github-actions
bot
added
size/small
github-actions
bot
added
size/large
rvdr
force-pushed
the
spi-nvme-patches
branch
from
6d0ea88 to
5adc521
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@patch/u-boot/sunxi-dev-u-boot-a523/pci-sunxi-enable-pcie-support.patch`:
- Around line 773-778: The debug prints in sun55i_pcie_host_wait_for_link()
(printing val, RDLH_LINK_UP, SMLH_LINK_UP, LINK_SPEED, LINK_WIDTH) run on every
poll and flood the console; remove them or guard them behind a verbose/debug
condition (e.g., only when a PCI debug/verbose flag is enabled) or limit
printing to terminal events (on link-up or final timeout/failure). Locate the
printf lines referencing RDLH_LINK_UP, SMLH_LINK_UP, LINK_SPEED, and LINK_WIDTH
and either delete them or wrap them in a conditional that checks a debug/verbose
macro or runtime flag so they do not execute on every loop iteration.
- Around line 541-549: The dev_dbg macro uses the undefined symbol __dev_printk
when DEBUG is defined, causing build failures; change the DEBUG branch to map
dev_dbg to an existing logging helper (e.g., dev_err) or include/use the correct
kernel logging function instead of __dev_printk so the symbol resolves. Locate
the dev_dbg definition in the patch and replace the DEBUG-case reference to
__dev_printk with a call to the existing dev_err (or another available dev_*
logging function) ensuring the macro signature remains dev_dbg(dev, fmt, ...),
or alternatively include the header that provides __dev_printk if that is
preferred.
In `@patch/u-boot/sunxi-dev-u-boot-a523/phy-allwinner-add-pcie-usb3-driver.patch`:
- Around line 589-605: In pcie_usb3_sub_system_init the code uses shifts like (1
<< PCIE_BRG_REG) which can invoke undefined behavior for bit 31 on 32-bit signed
ints; change these to use the BIT() macro (e.g. BIT(PCIE_BRG_REG),
BIT(PCIE_REF_CLK_GATING)) or otherwise use unsigned shifts to avoid UB when
setting PCIE_BGR_REG and the 0x2001a84 register; update the two occurrences in
pcie_usb3_sub_system_init (references: PCIE_BGR_REG, PCIE_BRG_REG,
PCIE_REF_CLK_GATING) so the bit-twiddling uses BIT() consistently.
🧹 Nitpick comments (4)
patch/u-boot/sunxi-dev-u-boot-a523/pci-sunxi-enable-pcie-support.patch (3)
26-32: Kconfig help text formatting issue.The help text indentation uses 3 spaces instead of the standard tab for Kconfig. Line 32 also has trailing whitespace which will trigger checkpatch warnings.
🔧 Suggested fix
config PCIE_SUN55I_RC bool "Allwinner SUN55I DesignWare PCIe controller" default n depends on ARCH_SUNXI help - Enables support for the DW PCIe controller in the Allwinner Sun55i SoC. - + Enables support for the DW PCIe controller in the Allwinner Sun55i SoC. + endif
96-118: Inconsistent indentation style.This function uses 4-space indentation while the rest of the file (e.g.,
sun55i_pcie_cfg_writeabove) uses tabs. U-Boot coding style requires tabs for indentation.🔧 Suggested fix
int sun55i_pcie_cfg_read(void __iomem *addr, int size, ulong *val) { - - if ((uintptr_t)addr & (size - 1)) { - *val = 0; - printf("CFG_READ: Bad alignment\n"); - return PCIBIOS_BAD_REGISTER_NUMBER; - } - - if (size == 4) { - *val = readl(addr); - } else if (size == 2) { - *val = readw(addr); - } else if (size == 1) { - *val = readb(addr); - } else { - *val = 0; - printf("CFG_READ: Bad size\n"); - return PCIBIOS_BAD_REGISTER_NUMBER; - } - - return PCIBIOS_SUCCESSFUL; + if ((uintptr_t)addr & (size - 1)) { + *val = 0; + printf("CFG_READ: Bad alignment\n"); + return PCIBIOS_BAD_REGISTER_NUMBER; + } + + if (size == 4) { + *val = readl(addr); + } else if (size == 2) { + *val = readw(addr); + } else if (size == 1) { + *val = readb(addr); + } else { + *val = 0; + printf("CFG_READ: Bad size\n"); + return PCIBIOS_BAD_REGISTER_NUMBER; + } + + return PCIBIOS_SUCCESSFUL; }The same inconsistency appears in multiple other locations (lines 333-338, 447-458, etc.). Consider running the file through checkpatch or an automated formatter.
1021-1031: Duplicate header includes.
<asm/io.h>and<asm/gpio.h>are included twice in this header file.🔧 Suggested fix
`#include` <asm/io.h> `#include` <asm/gpio.h> `#include` <clk.h> `#include` <reset.h> `#include` <generic-phy.h> `#include` <pci.h> `#include` <power-domain.h> `#include` <power/regulator.h> -#include <asm/io.h> -#include <asm/gpio.h> -#include <errno.h> +#include <errno.h>config/kernel/linux-sunxi64-edge.config (1)
1141-1147: Consider dropping CONFIG_SPI_DEBUG for release kernels.
This can spam logs and add overhead unless you’re actively debugging SPI bring-up.♻️ Suggested tweak
-CONFIG_SPI_DEBUG=y
patch/u-boot/sunxi-dev-u-boot-a523/phy-allwinner-add-pcie-usb3-driver.patch
Show resolved
Hide resolved
There was a problem hiding this comment.
cannot test, no hw. But it builds.
uboot patches could be enhanced by moving generic SoC stuff to v2026.01 patch folder and board-specific patches into board_whateverboardsname. Though this and also rewriting all uboot patches can be done later.
@igorpecovnik rabbit needs further adjustments I think. It was extremely chatty on this one and probably also (without checking) a bunch of slop included.
3 participants
Description
Summary: These changes add support for PCIe, SPI-Flash and Iommu on the Radxa Cubie A5E and Orange Pi 4a. The changes for the Pi 4a couldnt be tested due to a lack of hardware.
These are the build logs:
https://paste.armbian.eu/vesojokuva
Documentation summary for feature / change
[x] short description: Add PCIe, USB3-PHY, and IOMMU support for sun55i boards.
[x] summary: Users of Cubie A5E and Orange Pi 4A can now use PCIe/NVMe and USB3.0 stable.
[x] example of usage: Booting from NVMe or using lspci to list connected PCIe devices.
How Has This Been Tested?
[x] Test A: Flashing the bootloader to an SD-Card and Booting from the NVMe. No Issues after 24 hours of runtime.
[x] Test B: Booting directly from the SD-Card. SPI-Flash and NVMe SSD are getting detected.
Checklist:
[x] My code follows the style guidelines of this project
[x] I have performed a self-review of my own code
[x] My changes generate no new warnings
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.