Fixed yy3568 the NVMe SSD detection issue#9832
Conversation
Fixed the NVMe SSD detection issue https://forum.armbian.com/topic/49775-board-bring-up-youyeetoo-yy3568-rockchip-rk3568/
Fixed the NVMe SSD detection issue https://forum.armbian.com/topic/49775-board-bring-up-youyeetoo-yy3568-rockchip-rk3568/
Arthur97172
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 updates RK3568 YY3568 device tree files for both kernel versions 6.12 and 6.18 with parallel changes: adding new PCIe power management regulators, rewiring PCIe supply chains, enabling I2S audio output, introducing PCIe-specific pinctrl groups, and adjusting Ethernet pinctrl formatting. ChangesRK3568 YY3568 Device Tree Updates (6.12 and 6.18 kernels)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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
size/medium
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
patch/kernel/archive/rockchip64-6.18/dt/rk3568-yy3568.dts (1)
587-589: ⚡ Quick winAvoid enabling
&i2s0_8chwithout a board-level consumer.This DTS already routes board audio through
&i2s1_8ch, and nothing in this file binds&i2s0_8chto a card or codec. Dropping this keeps the NVMe fix scoped and avoids pinmuxing an otherwise unused interface.🤖 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-6.18/dt/rk3568-yy3568.dts` around lines 587 - 589, The patch enables &i2s0_8ch even though no board consumer or codec binds to it; remove or disable that node instead of setting status = "okay" so you don't enable unused pinmuxing—locate the &i2s0_8ch node in rk3568-yy3568.dts and either delete the status = "okay" line or change it to status = "disabled", leaving the existing &i2s1_8ch routing intact.patch/kernel/archive/rockchip64-6.12/dt/rk3568-yy3568.dts (1)
587-589: ⚡ Quick winAvoid enabling
&i2s0_8chwithout a board-level consumer.This DTS already routes board audio through
&i2s1_8ch, and nothing in this file binds&i2s0_8chto a card or codec. Dropping this keeps the NVMe fix scoped and avoids pinmuxing an otherwise unused interface.🤖 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-6.12/dt/rk3568-yy3568.dts` around lines 587 - 589, The DTS enables the &i2s0_8ch node without any board-level consumer or codec binding; remove the status enablement to avoid exposing an unused interface—locate the &i2s0_8ch node in rk3568-yy3568.dts and either delete the status = "okay"; line or change it to status = "disabled"; so only the actual board audio path (&i2s1_8ch) remains enabled.
🤖 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-6.12/dt/rk3568-yy3568.dts`:
- Around line 139-148: The pcie_oe_regulator node is attached to the wrong PHY
(&combphy1) and its enable polarity is misencoded: move the pcie_oe_regulator
node so it is under the PCIe 3.0 PHY node (&pcie30phy) that pairs with &pcie3x2,
and correct the enable polarity by adding/uncommenting enable-active-high for
the regulator; ensure the gpio line remains gpio = <&gpio3 RK_PA7
GPIO_ACTIVE_HIGH> and that the regulator node (pcie_oe_regulator) references the
&pcie30phy parent so the OE GPIO high truly enables the regulator.
In `@patch/kernel/archive/rockchip64-6.18/dt/rk3568-yy3568.dts`:
- Around line 139-148: The pcie_oe_regulator definition uses gpio = <&gpio3
RK_PA7 GPIO_ACTIVE_HIGH> but is attached to &combphy1 while the NVMe path uses
&pcie3x2 with phys = <&pcie30phy>; update the regulator node (pcie_oe_regulator)
to be referenced/placed under the correct PHY node (&pcie30phy or the pcie3x2
controller) so the OE/REFCLK GPIO (GPIO3_A7) actually controls the PCIe3.0 PHY,
and reconcile polarity by either uncommenting and using enable-active-high in
pcie_oe_regulator or changing the GPIO phandle flag to GPIO_ACTIVE_LOW so the
regulator-fixed binding's polarity matches the GPIO direction; ensure all
occurrences (the pcie_oe_regulator node and its references under &combphy1,
&pcie30phy, &pcie3x2) are updated consistently.
---
Nitpick comments:
In `@patch/kernel/archive/rockchip64-6.12/dt/rk3568-yy3568.dts`:
- Around line 587-589: The DTS enables the &i2s0_8ch node without any
board-level consumer or codec binding; remove the status enablement to avoid
exposing an unused interface—locate the &i2s0_8ch node in rk3568-yy3568.dts and
either delete the status = "okay"; line or change it to status = "disabled"; so
only the actual board audio path (&i2s1_8ch) remains enabled.
In `@patch/kernel/archive/rockchip64-6.18/dt/rk3568-yy3568.dts`:
- Around line 587-589: The patch enables &i2s0_8ch even though no board consumer
or codec binds to it; remove or disable that node instead of setting status =
"okay" so you don't enable unused pinmuxing—locate the &i2s0_8ch node in
rk3568-yy3568.dts and either delete the status = "okay" line or change it to
status = "disabled", leaving the existing &i2s1_8ch routing intact.
🪄 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: e822e5a9-4b7a-4f57-8c50-398857bea02e
📒 Files selected for processing (2)
patch/kernel/archive/rockchip64-6.12/dt/rk3568-yy3568.dtspatch/kernel/archive/rockchip64-6.18/dt/rk3568-yy3568.dts
| pcie_oe_regulator: pcie-oe-regulator { | ||
| compatible = "regulator-fixed"; | ||
| regulator-name = "pcie_oe"; | ||
| gpio = <&gpio3 RK_PA7 GPIO_ACTIVE_HIGH>; | ||
| pinctrl-names = "default"; | ||
| pinctrl-0 = <&pcie_oe>; | ||
| //enable-active-high; | ||
| regulator-always-on; | ||
| regulator-boot-on; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
1. In the Linux regulator-fixeddevice-tree binding, how is enable polarity determined whengpio = <...>is present butenable-active-high;is omitted? 2. On RK3568 in upstream DTS/bindings, which PHY node is associated with thepcie3x2host controller:combphy1orpcie30phy?
💡 Result:
In the Linux regulator-fixed device-tree binding, when the gpio property is present but the enable-active-high; property is omitted, the enable polarity defaults to active low [1]. Regarding the RK3568 SoC in upstream Linux device-tree (DTS) and bindings, the pcie3x2 host controller is associated with the pcie30phy node [2][3]. The combphy1 (or naneng-combphy) nodes are distinct and are used for other interfaces such as SATA, USB3, or PCIe 2.0, rather than the PCIe 3.0 controller [4][5][6].
Citations:
- 1: https://www.kernel.org/doc/Documentation/devicetree/bindings/regulator/fixed-regulator.txt
- 2: https://lists.infradead.org/pipermail/linux-rockchip/2022-April/031429.html
- 3: https://kernel.googlesource.com/pub/scm/linux/kernel/git/torvalds/linux/+/88afbb21d4b36fee6acaa167641f9f0fc122f01b/arch/arm64/boot/dts/rockchip/rk3568.dtsi
- 4: https://dl.radxa.com/rock3/docs/hw/datasheet/Rockchip-RK3568-Datasheet-V1.0-20201210.pdf
- 5: https://mjmwired.net/kernel/Documentation/devicetree/bindings/phy/phy-rockchip-naneng-combphy.yaml
- 6: https://lwn.net/Articles/880042/
🏁 Script executed:
cat -n patch/kernel/archive/rockchip64-6.12/dt/rk3568-yy3568.dts | sed -n '139,148p; 201,203p; 292,300p'Repository: armbian/build
Length of output: 768
Attach the REFCLK/OE regulator to the correct PCIe 3.0 PHY node and fix the enable polarity encoding.
The patch routes the NVMe slot through &pcie3x2 which pairs with &pcie30phy (lines 299), but line 202 attaches pcie_oe_regulator to &combphy1 instead. This PHY mismatch will prevent the OE regulator from controlling the PCIe 3.0 interface.
Additionally, the regulator has a polarity encoding issue: gpio = <&gpio3 RK_PA7 GPIO_ACTIVE_HIGH> declares an electrically active-high signal, but line 145 comments out enable-active-high;, causing the regulator to default to active-low enable semantics. This inverts the signal interpretation—GPIO high would deactivate the regulator instead of activating it.
Move the pcie_oe_regulator supply to &pcie30phy and either uncomment enable-active-high; or verify that the board schematic actually requires active-low enable.
🤖 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-6.12/dt/rk3568-yy3568.dts` around lines 139 -
148, The pcie_oe_regulator node is attached to the wrong PHY (&combphy1) and its
enable polarity is misencoded: move the pcie_oe_regulator node so it is under
the PCIe 3.0 PHY node (&pcie30phy) that pairs with &pcie3x2, and correct the
enable polarity by adding/uncommenting enable-active-high for the regulator;
ensure the gpio line remains gpio = <&gpio3 RK_PA7 GPIO_ACTIVE_HIGH> and that
the regulator node (pcie_oe_regulator) references the &pcie30phy parent so the
OE GPIO high truly enables the regulator.
| pcie_oe_regulator: pcie-oe-regulator { | ||
| compatible = "regulator-fixed"; | ||
| regulator-name = "pcie_oe"; | ||
| gpio = <&gpio3 RK_PA7 GPIO_ACTIVE_HIGH>; | ||
| pinctrl-names = "default"; | ||
| pinctrl-0 = <&pcie_oe>; | ||
| //enable-active-high; | ||
| regulator-always-on; | ||
| regulator-boot-on; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
1. In the Linux regulator-fixeddevice-tree binding, how is enable polarity determined whengpio = <...>is present butenable-active-high;is omitted? 2. On RK3568 in upstream DTS/bindings, which PHY node is associated with thepcie3x2host controller:combphy1orpcie30phy?
💡 Result:
- In the Linux
regulator-fixeddevice-tree binding, when thegpioproperty is present butenable-active-high;is omitted, the enable polarity defaults to active low [1]. Historically, this behavior is explicitly documented in the kernel's device-tree bindings for fixed regulators [1]. While modern Linux kernel implementations have transitioned to using GPIO descriptors (gpiod) which can handle polarity flags directly within the GPIO phandle specifier, theenable-active-highproperty remains the standard mechanism for defining this polarity in theregulator-fixedbinding [2][1]. 2. On the RK3568 SoC, thepcie3x2host controller is associated with thepcie30phynode in upstream Linux device-tree bindings [3]. The RK3568 utilizes a dedicated PCIe 3.0 PHY (often referred to as the SNPS PCIe3 PHY) for its PCIe 3.0 controllers, which is distinct from the "combphy" (Naneng combo PHY) nodes used for PCIe 2.0, SATA, and USB 3.0 interfaces [3][4].
Citations:
- 1: https://www.kernel.org/doc/Documentation/devicetree/bindings/regulator/fixed-regulator.txt
- 2: https://lkml.iu.edu/1810.0/00725.html
- 3: https://lwn.net/Articles/892691/
- 4: https://dl.radxa.com/rock3/docs/hw/datasheet/Rockchip-RK3568-Datasheet-V1.0-20201210.pdf
🏁 Script executed:
# First, verify the file exists and examine the relevant sections
cd patch/kernel/archive/rockchip64-6.18/dt/ 2>/dev/null && ls -la rk3568-yy3568.dts || echo "File not found, searching for it..."
find . -name "rk3568-yy3568.dts" -type f 2>/dev/null | head -5Repository: armbian/build
Length of output: 145
🏁 Script executed:
# Examine the file sections mentioned in the review comment
echo "=== Lines 139-148 (pcie_oe_regulator definition) ==="
sed -n '139,148p' ./rk3568-yy3568.dts
echo ""
echo "=== Lines 201-203 (combphy1 phy-supply usage) ==="
sed -n '201,203p' ./rk3568-yy3568.dts
echo ""
echo "=== Lines 292-300 (pcie3x2 + pcie30phy routing) ==="
sed -n '292,300p' ./rk3568-yy3568.dtsRepository: armbian/build
Length of output: 421
🏁 Script executed:
# Read the file sections using the correct path
echo "=== Lines 139-148 (pcie_oe_regulator definition) ==="
sed -n '139,148p' patch/kernel/archive/rockchip64-6.18/dt/rk3568-yy3568.dts
echo ""
echo "=== Lines 201-203 (combphy1 phy-supply usage) ==="
sed -n '201,203p' patch/kernel/archive/rockchip64-6.18/dt/rk3568-yy3568.dts
echo ""
echo "=== Lines 292-300 (pcie3x2 + pcie30phy routing) ==="
sed -n '292,300p' patch/kernel/archive/rockchip64-6.18/dt/rk3568-yy3568.dts
echo ""
echo "=== Full context around combphy1 (lines 195-210) ==="
sed -n '195,210p' patch/kernel/archive/rockchip64-6.18/dt/rk3568-yy3568.dts
echo ""
echo "=== Full context around pcie3x2 (lines 286-305) ==="
sed -n '286,305p' patch/kernel/archive/rockchip64-6.18/dt/rk3568-yy3568.dtsRepository: armbian/build
Length of output: 1430
Verify that the REFCLK/OE regulator is attached to the correct PCIe PHY node and has matching GPIO polarity.
Line 202 attaches pcie_oe_regulator to &combphy1, while Lines 299 shows the NVMe path in this patch routes through &pcie3x2 with phys = <&pcie30phy>. On RK3568, pcie3x2 uses the dedicated pcie30phy (PCIe 3.0 PHY), not combphy1 (which handles PCIe 2.0, SATA, and USB 3.0). If GPIO3_A7 gates the PCIe 3.0 REFCLK, the regulator must be attached to &pcie30phy, not &combphy1.
Additionally, Line 145 has enable-active-high; commented out, which means the regulator defaults to active-low polarity per the regulator-fixed binding. However, Line 143 specifies GPIO_ACTIVE_HIGH in the GPIO phandle. These polarities conflict and must be reconciled before merging.
Also applies to: 201-203, 286-290, 292-299
🤖 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-6.18/dt/rk3568-yy3568.dts` around lines 139 -
148, The pcie_oe_regulator definition uses gpio = <&gpio3 RK_PA7
GPIO_ACTIVE_HIGH> but is attached to &combphy1 while the NVMe path uses &pcie3x2
with phys = <&pcie30phy>; update the regulator node (pcie_oe_regulator) to be
referenced/placed under the correct PHY node (&pcie30phy or the pcie3x2
controller) so the OE/REFCLK GPIO (GPIO3_A7) actually controls the PCIe3.0 PHY,
and reconcile polarity by either uncommenting and using enable-active-high in
pcie_oe_regulator or changing the GPIO phandle flag to GPIO_ACTIVE_LOW so the
regulator-fixed binding's polarity matches the GPIO direction; ensure all
occurrences (the pcie_oe_regulator node and its references under &combphy1,
&pcie30phy, &pcie3x2) are updated consistently.
EvilOlaf
left a comment
EvilOlaf
left a comment
There was a problem hiding this comment.
missing rockchip64-7.0 and 7.1
rockchip64-6.12 is no longer in use
|
If this is fixed within a week, it can go into upcoming release. |
4 participants
Description
Fixed NVMe SSD detection issue on Youyeetoo YY3568.
Problem
rockchip_p3phy_rk3568_init: lock failed)Phy link never came up)Root Cause
Missing / incorrect configuration for:
Solution
pcie30_pwrandpcie_oepinctrlvcc3v3_pcie+pcie_oe_regulatorwith active-low)&combphy1,&pcie30phyand&pcie3x2with correct clocks, phy-supply and reset polaritystartup-delay-usfor power stabilityThis makes the M.2 NVMe SSD work reliably on YY3568.
GitHub issue reference: None (new fix)
Documentation summary for feature / change
How Has This Been Tested?
lspcishows NVMe device andlsblk/nvme listworksTest Configuration:
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes