rockchip64: fix RK3528 USB2 PHY regmap split for high-speed NCM#9785
rockchip64: fix RK3528 USB2 PHY regmap split for high-speed NCM#9785rubycomm wants to merge 2 commits into
Conversation
… + 7.1) The merged RK3528 USB2 PHY support patch (PR armbian#9500) collapsed rphy->grf onto rphy->phy_base when the DT node had rockchip,usbgrf, making GRF/port-control register accesses hit the standalone PHY MMIO window instead of the syscon. On NanoPi Zero2 this prevents the USB-C gadget from completing high-speed NCM enumeration: the UDC stays at default/full-speed, Windows does not enumerate a usable UsbNcm adapter, and usb0 stays DOWN/NO-CARRIER. The patch description already documented the intended topology (phy_base for the standalone PHY window, grf via the rockchip,usbgrf phandle for port-control fields), but the implementation drifted. Restore the split: keep phy_base mapped to the PHY node, and resolve rphy->grf from the rockchip,usbgrf phandle. Fall back to the parent syscon for older SoC layouts without the phandle. Apply identically to rockchip64-6.18, rockchip64-7.0, and the rockchip64-7.1 archive (added in 9d967ec, inherited the bug). Tested on FriendlyElec NanoPi Zero2 (RK3528) with a configfs NCM gadget over /sys/kernel/config/usb_gadget: Before this patch: /sys/class/udc/fe500000.usb/state = default /sys/class/udc/fe500000.usb/current_speed = full-speed usb0: DOWN, NO-CARRIER Windows host: no UsbNcm Host Device After this patch: /sys/class/udc/fe500000.usb/state = configured /sys/class/udc/fe500000.usb/current_speed = high-speed usb0: UP, LOWER_UP, 10.10.10.1/24, fe80::12:34ff:fe56:789a/64 Windows host: UsbNcm Host Device armbian#5, Up, 426.0 Mbps ping fe80::12:34ff:fe56:789a%122: 3 sent / 3 received / 0% loss Note: a separate u_ether NULL-deref on gadget teardown is tracked under PR armbian#9608 and is not addressed here. Signed-off-by: Shlomi Marco <s.marco@rubycomm.com>
rubycomm
requested review from
ColorfulRhino,
HeyMeco,
JohnTheCoolingFan,
SuperKali,
TheSnowfield,
Tonymac32,
ahoneybun,
amazingfate,
brentr,
catalinii,
chainsx,
efectn,
iav,
igorpecovnik,
joekhoobyar,
krachlatte,
lanefu,
paolosabatino,
prahal,
pyavitz,
rpardini and
schwar3kat
as code owners
📝 WalkthroughWalkthroughThis PR adds RK3528 USB2 PHY driver support across three Rockchip kernel versions by introducing a PHY regmap (phy_base), adding a PHY-based clkout descriptor, refactoring regmap handling, implementing RK3528-specific tuning, and adding device-tree bindings. ChangesRK3528 USB2 PHY Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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
Needs review
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
patch/kernel/archive/rockchip64-7.1/rk3528-10-phy-rockchip-inno-usb2-Add-support-for-RK3528.patch (1)
154-179:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMisleading error message when parent-syscon lookup fails.
The consolidated
IS_ERR(rphy->grf)check at the end covers both code paths, but prints"failed to locate usbgrf"even when the failure came fromsyscon_node_to_regmap(dev->parent->of_node)in theelsebranch — which has nothing to do with ausbgrfphandle. The upstream kernel keeps the error handling inline and per-branch (confirmed by the livetorvalds/linuxsource).🐛 Proposed fix — separate error paths with accurate messages
if (!dev->parent || !dev->parent->of_node || of_property_present(np, "rockchip,usbgrf")) { rphy->phy_base = device_node_to_regmap(np); if (IS_ERR(rphy->phy_base)) return PTR_ERR(rphy->phy_base); rphy->grf = syscon_regmap_lookup_by_phandle(np, "rockchip,usbgrf"); + if (IS_ERR(rphy->grf)) { + dev_err(dev, "failed to locate usbgrf\n"); + return PTR_ERR(rphy->grf); + } } else { rphy->grf = syscon_node_to_regmap(dev->parent->of_node); + if (IS_ERR(rphy->grf)) + return PTR_ERR(rphy->grf); rphy->phy_base = rphy->grf; } - if (IS_ERR(rphy->grf)) { - dev_err(dev, "failed to locate usbgrf\n"); - return PTR_ERR(rphy->grf); - }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/kernel/archive/rockchip64-7.1/rk3528-10-phy-rockchip-inno-usb2-Add-support-for-RK3528.patch` around lines 154 - 179, The error message is misleading because the consolidated IS_ERR(rphy->grf) check prints "failed to locate usbgrf" even when syscon_node_to_regmap(dev->parent->of_node) fails; split the error handling so each branch checks its own return and emits an accurate message: after syscon_regmap_lookup_by_phandle(np, "rockchip,usbgrf") validate IS_ERR(rphy->grf) and dev_err(dev, "failed to locate usbgrf") (or return PTR_ERR), and separately after syscon_node_to_regmap(dev->parent->of_node) validate IS_ERR and dev_err(dev, "failed to locate parent syscon/grf") (or return PTR_ERR); keep assignment rphy->phy_base = rphy->grf only in the parent branch as appropriate and ensure each branch returns the correct PTR_ERR on failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@patch/kernel/archive/rockchip64-7.0/rk3528-10-phy-rockchip-inno-usb2-Add-support-for-RK3528.patch`:
- Around line 176-179: The error message for IS_ERR(rphy->grf) is misleading
because the same message is used for both the standalone-PHY lookup and the
parent syscon_node_to_regmap() failure; update the logic to distinguish the two
failure cases: after the standalone branch that assigns rphy->grf from
devm_regmap_init_mmio() keep the original "failed to locate usbgrf" error (or
use dev_err_probe() there), and add a separate check right after the
syscon_node_to_regmap(dev->parent->of_node) call that logs a neutral, accurate
message (e.g., "failed to get parent syscon regmap" or use dev_err_probe()) and
returns PTR_ERR(rphy->grf); alternatively move the IS_ERR() check into each
branch so each path emits a specific diagnostic referencing rphy->grf and
syscon_node_to_regmap() respectively.
---
Outside diff comments:
In
`@patch/kernel/archive/rockchip64-7.1/rk3528-10-phy-rockchip-inno-usb2-Add-support-for-RK3528.patch`:
- Around line 154-179: The error message is misleading because the consolidated
IS_ERR(rphy->grf) check prints "failed to locate usbgrf" even when
syscon_node_to_regmap(dev->parent->of_node) fails; split the error handling so
each branch checks its own return and emits an accurate message: after
syscon_regmap_lookup_by_phandle(np, "rockchip,usbgrf") validate
IS_ERR(rphy->grf) and dev_err(dev, "failed to locate usbgrf") (or return
PTR_ERR), and separately after syscon_node_to_regmap(dev->parent->of_node)
validate IS_ERR and dev_err(dev, "failed to locate parent syscon/grf") (or
return PTR_ERR); keep assignment rphy->phy_base = rphy->grf only in the parent
branch as appropriate and ensure each branch returns the correct PTR_ERR on
failure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fc14d898-32a1-4df3-81bd-eeaf3e641f45
📒 Files selected for processing (3)
patch/kernel/archive/rockchip64-6.18/rk3528-10-phy-rockchip-inno-usb2-Add-support-for-RK3528.patchpatch/kernel/archive/rockchip64-7.0/rk3528-10-phy-rockchip-inno-usb2-Add-support-for-RK3528.patchpatch/kernel/archive/rockchip64-7.1/rk3528-10-phy-rockchip-inno-usb2-Add-support-for-RK3528.patch
| + if (IS_ERR(rphy->grf)) { | ||
| + dev_err(dev, "failed to locate usbgrf\n"); | ||
| + return PTR_ERR(rphy->grf); | ||
| + } |
There was a problem hiding this comment.
Misleading error message when the parent-syscon path fails.
The consolidated IS_ERR(rphy->grf) check now also covers the else branch where rphy->grf = syscon_node_to_regmap(dev->parent->of_node). If that parent-syscon lookup fails (older SoCs without rockchip,usbgrf), the user will see "failed to locate usbgrf" even though no usbgrf phandle was ever consulted, which is unhelpful for debugging DT misconfiguration on those SoCs. Behavior is correct; only the diagnostic is off. Consider differentiating the two branches or using dev_err_probe() with a more neutral phrasing.
🩹 Suggested phrasing tweak (or split per-branch)
- if (IS_ERR(rphy->grf)) {
- dev_err(dev, "failed to locate usbgrf\n");
- return PTR_ERR(rphy->grf);
- }
+ if (IS_ERR(rphy->grf))
+ return dev_err_probe(dev, PTR_ERR(rphy->grf),
+ "failed to locate GRF/usbgrf regmap\n");Or, if you prefer keeping the original message for the standalone-PHY branch only, move the check back inside that branch and add a separate check after syscon_node_to_regmap() in the else branch.
📝 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.
| + if (IS_ERR(rphy->grf)) { | |
| + dev_err(dev, "failed to locate usbgrf\n"); | |
| + return PTR_ERR(rphy->grf); | |
| + } | |
| if (IS_ERR(rphy->grf)) | |
| return dev_err_probe(dev, PTR_ERR(rphy->grf), | |
| "failed to locate GRF/usbgrf regmap\n"); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@patch/kernel/archive/rockchip64-7.0/rk3528-10-phy-rockchip-inno-usb2-Add-support-for-RK3528.patch`
around lines 176 - 179, The error message for IS_ERR(rphy->grf) is misleading
because the same message is used for both the standalone-PHY lookup and the
parent syscon_node_to_regmap() failure; update the logic to distinguish the two
failure cases: after the standalone branch that assigns rphy->grf from
devm_regmap_init_mmio() keep the original "failed to locate usbgrf" error (or
use dev_err_probe() there), and add a separate check right after the
syscon_node_to_regmap(dev->parent->of_node) call that logs a neutral, accurate
message (e.g., "failed to get parent syscon regmap" or use dev_err_probe()) and
returns PTR_ERR(rphy->grf); alternatively move the IS_ERR() check into each
branch so each path emits a specific diagnostic referencing rphy->grf and
syscon_node_to_regmap() respectively.
EvilOlaf
left a comment
EvilOlaf
left a comment
There was a problem hiding this comment.
Either address or dismiss Rabbit's concern, then should be good
|
✅ This PR has been reviewed and approved — all set for merge! |
github-actions
Bot
added
Ready to merge
|
I would recommend you to instead adopt the upstream patches as-is. Heiko was kind enough to resubmit them, see https://patchwork.kernel.org/project/linux-rockchip/list/?submitter=24481 |
Address CodeRabbit's review feedback on PR armbian#9785: the consolidated IS_ERR(rphy->grf) check at the end of both branches printed "failed to locate usbgrf" even when the parent-syscon lookup in the else branch failed (which has nothing to do with the usbgrf phandle). Split the check per-branch so the diagnostic only fires when the lookup it refers to actually failed; the else branch now fails silently before the phy_base alias, matching upstream Linux's existing per-branch error-handling convention for this driver. No behavioral change beyond more accurate error logging on the older-SoC path. Applied identically to rockchip64-6.18, 7.0, 7.1. Signed-off-by: Shlomi Marco <s.marco@rubycomm.com>
github-actions
Bot
added
Needs review
|
Addressed CodeRabbit's nit. The |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
patch/kernel/archive/rockchip64-7.1/rk3528-10-phy-rockchip-inno-usb2-Add-support-for-RK3528.patch (1)
67-70:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
get_reg_base()simplification is correct for all non-RV1108 SoCs but is the downstream consequence of the probe regression above.For standalone PHYs (no parent syscon): old
usbgrf = NULL→ oldget_reg_base()returnedgrf, which was already the USBGRF phandle target. Newget_reg_base()returnsgrf, which is still the USBGRF phandle target. No behavioural change. ✓For GRF-child PHYs (no
rockchip,usbgrf): old and new both return the parent syscon regmap viagrf. ✓The only regression is RV1108, where the old driver returned
usbgrf(the rv1108-usbgrf regmap) fromget_reg_base()for port register access, whilegrfpointed to the parent main-GRF. If the probe fix above re-assignsrphy->grfto the rv1108-usbgrf for that SoC, this simplification becomes correct across all SoCs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/kernel/archive/rockchip64-7.1/rk3528-10-phy-rockchip-inno-usb2-Add-support-for-RK3528.patch` around lines 67 - 70, The change to get_reg_base() always returning rphy->grf is fine except RV1108 relies on rphy->grf pointing at the rv1108-usbgrf regmap; fix the probe so the RV1108 code path assigns rphy->grf = rphy->usbgrf (or otherwise set rphy->grf to the USBGRF regmap) when the rv1108-compatible branch runs, ensuring get_reg_base() continues to return the proper USBGRF regmap for RV1108; locate the RV1108 handling in the driver probe (e.g., rockchip_usb2phy_probe or the SoC-specific probe branch) and add the assignment where rphy->usbgrf is obtained.
🧹 Nitpick comments (1)
patch/kernel/archive/rockchip64-7.1/rk3528-10-phy-rockchip-inno-usb2-Add-support-for-RK3528.patch (1)
189-216: 💤 Low valueTwo
regmap_writecalls targeting the same PHY register offsets (0x30 and 0x430) — functional but mergeable.Each pair targets different bit fields encoded in the upper-16 (HIWORD-mask) half, so with the standard Rockchip regmap HIWORD_UPDATE configuration the writes are non-conflicting. No correctness issue.
That said, the two writes to 0x30 ("diff. receiver off" on bit 18, "HS eye height" on bits 22:20) could be folded into a single write:
(BIT(18) | GENMASK(22, 20)) | 0x0000. Same applies to 0x430. This reduces register traffic and makes the intent clearer.♻️ Merge the two writes per register into one
- /* Turn off otg port differential receiver in suspend mode */ - ret = regmap_write(rphy->phy_base, 0x30, BIT(18) | 0x0000); - if (ret) - return ret; - - /* Turn off host port differential receiver in suspend mode */ - ret = regmap_write(rphy->phy_base, 0x430, BIT(18) | 0x0000); - if (ret) - return ret; - - /* Set otg port HS eye height to 400mv (default is 450mv) */ - ret = regmap_write(rphy->phy_base, 0x30, GENMASK(22, 20) | 0x0000); - if (ret) - return ret; - - /* Set host port HS eye height to 400mv (default is 450mv) */ - ret = regmap_write(rphy->phy_base, 0x430, GENMASK(22, 20) | 0x0000); + /* Turn off otg port diff. receiver in suspend; set HS eye to 400mv */ + ret = regmap_write(rphy->phy_base, 0x30, + (BIT(18) | GENMASK(22, 20)) | 0x0000); + if (ret) + return ret; + + /* Turn off host port diff. receiver in suspend; set HS eye to 400mv */ + ret = regmap_write(rphy->phy_base, 0x430, + (BIT(18) | GENMASK(22, 20)) | 0x0000); if (ret) return ret;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/kernel/archive/rockchip64-7.1/rk3528-10-phy-rockchip-inno-usb2-Add-support-for-RK3528.patch` around lines 189 - 216, Fold the two regmap_write calls that target the same offsets in rk3528_usb2phy_tuning into single writes: replace the separate writes to offset 0x30 (BIT(18) and GENMASK(22,20)) with one regmap_write(rphy->phy_base, 0x30, BIT(18) | GENMASK(22,20) | 0x0000) and do the same for offset 0x430 (BIT(18) | GENMASK(22,20) | 0x0000), leaving the final regmap_write to 0x94 unchanged; keep the same ret/error-check pattern so each combined write still returns on error.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@patch/kernel/archive/rockchip64-7.1/rk3528-10-phy-rockchip-inno-usb2-Add-support-for-RK3528.patch`:
- Around line 67-70: The change to get_reg_base() always returning rphy->grf is
fine except RV1108 relies on rphy->grf pointing at the rv1108-usbgrf regmap; fix
the probe so the RV1108 code path assigns rphy->grf = rphy->usbgrf (or otherwise
set rphy->grf to the USBGRF regmap) when the rv1108-compatible branch runs,
ensuring get_reg_base() continues to return the proper USBGRF regmap for RV1108;
locate the RV1108 handling in the driver probe (e.g., rockchip_usb2phy_probe or
the SoC-specific probe branch) and add the assignment where rphy->usbgrf is
obtained.
---
Nitpick comments:
In
`@patch/kernel/archive/rockchip64-7.1/rk3528-10-phy-rockchip-inno-usb2-Add-support-for-RK3528.patch`:
- Around line 189-216: Fold the two regmap_write calls that target the same
offsets in rk3528_usb2phy_tuning into single writes: replace the separate writes
to offset 0x30 (BIT(18) and GENMASK(22,20)) with one
regmap_write(rphy->phy_base, 0x30, BIT(18) | GENMASK(22,20) | 0x0000) and do the
same for offset 0x430 (BIT(18) | GENMASK(22,20) | 0x0000), leaving the final
regmap_write to 0x94 unchanged; keep the same ret/error-check pattern so each
combined write still returns on error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0bb89108-1a57-4a01-9114-61cff44ce296
📒 Files selected for processing (3)
patch/kernel/archive/rockchip64-6.18/rk3528-10-phy-rockchip-inno-usb2-Add-support-for-RK3528.patchpatch/kernel/archive/rockchip64-7.0/rk3528-10-phy-rockchip-inno-usb2-Add-support-for-RK3528.patchpatch/kernel/archive/rockchip64-7.1/rk3528-10-phy-rockchip-inno-usb2-Add-support-for-RK3528.patch
🚧 Files skipped from review as they are similar to previous changes (2)
- patch/kernel/archive/rockchip64-6.18/rk3528-10-phy-rockchip-inno-usb2-Add-support-for-RK3528.patch
- patch/kernel/archive/rockchip64-7.0/rk3528-10-phy-rockchip-inno-usb2-Add-support-for-RK3528.patch
3 participants
Problem
Images built since our PR #9500 broke USB-C NCM on NanoPi Zero2:
state=default,current_speed=full-speed(12 Mbps)usb0DOWN / NO-CARRIERUsbNcm Host DeviceUSB-C NCM is the canonical user-facing USB gadget configuration on the NanoPi Zero2, so this regression effectively breaks the board's USB-Ethernet interface for any downstream that relies on it.
Root cause
rk3528-10-phy-rockchip-inno-usb2-Add-support-for-RK3528.patch(merged in #9500) introducedphy_basefor RK3528's standalone PHY MMIO window correctly, but also assigned:when the DT node carried
rockchip,usbgrf. That collapsed the GRF/syscon regmap onto the standalone PHY MMIO. Code that logically usesrphy->grffor port-control fields then accesses the wrong register window. The patch's own commit message already named the right topology — this just brings the code in line with that description.Fix
plus the matching
IS_ERR(rphy->grf)failure path. Older SoC layouts that don't carryrockchip,usbgrfkeep the existing parent-syscon fallback.Applied identically to
rockchip64-6.18,rockchip64-7.0, androckchip64-7.1(the latter inherited the bug from the recent9d967eccf rockchip64: copy patchset from 7.0 to 7.1).Test results: NanoPi Zero2
defaultconfiguredcurrent_speedfull-speedhigh-speedusb010.10.10.1/24UsbNcm Host DeviceUsbNcm Host Device #5, Up,426.0 MbpsThe decisive boundary test was an Apr 7 image built at the same
v6.18.20kernel tag as the known-good 2026-03-25 image — both reproduced the bug at the same kernel source rev, ruling out kernel-stable / toolchain hypotheses and pinning the regression to this patch'sgrf = phy_baseassignment.Hardware-validated 2026-05-03 with image revision
26.05.0-rk3528grf2.Out of scope
A separate
u_etherNULL-deref on gadget teardown (eth_get_drvinfo) is tracked in #9608; this PR doesn't try to address that.Summary by CodeRabbit