| Message ID | 20240916014039.1918-2-naoki@radxa.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3,1/2] arm64: dts: rockchip: fix PCIe2 regulators for Radxa ROCK 3A | expand |
Hi, Am Montag, 16. September 2024, 03:40:39 CEST schrieb FUKAUMI Naoki: > on Radxa ROCK 3A, GPIO0_D4 is used to enable both pi6c PCIe clock > generator and "vcc3v3_pcie" regulator (PCIe3 M.2 M key connector). So now I've gone ahead and looked up a rock 3a schematics. By the way, the Radxa site is broken. On https://wiki.radxa.com/Rock3/hardware the v1.3 schematics link only leads to a not-found page. > since pi6c needs to be enabled before using PCIe3, GPIO0_D4 need to be > controlled by "vcc3v3_pi6c_03" regulator. then, make "vcc3v3_pi6c_03" > vin-supply for "vcc3v3_pcie30x1". You're hacking around the thing here. That gpio0-d4 controls two separate regulators. - the one creating VCC3V3_PCIE30X1 - the one creating VCC3V3_PI6C_03 Both are directly supplied from VCC5V0_SYS . So please don't create artificial hierarchies where none exist. Instead you could go with just having multiple phandles, like for example the vcc5v0_usb* set of regulators on the rock 5 itx: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3588-rock-5-itx.dts#n182 > also, "pcie30_avdd0v9" and "pcie30_avdd1v8" are unused. remove them. > > Fixes: 0522cd811220 ("arm64: dts: rockchip: Add PCIe v3 nodes to rock-3a") > Signed-off-by: FUKAUMI Naoki <naoki@radxa.com> > --- > Changes in v3: > - fix pinctrl just for reset pin as GPIO > Changes in v2: > - split patches for PCIe2 and PCIe3 > - change regulator name from "vcc3v3_pcie" to "vcc3v3_pcie30x1" > - add comment for vin-supply of "vcc3v3_pcie30x1" regulator > - remove unused "pcie30_avdd0v9" and "pcie30_avdd1v8" > - fix pinctrl node name to overwrite rk3568-pinctrl.dtsi > --- > .../boot/dts/rockchip/rk3568-rock-3a.dts | 45 +++++++------------ > 1 file changed, 15 insertions(+), 30 deletions(-) > > diff --git a/arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts b/arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts > index f94cbddf0f0c2..6b3f3ee7f22c7 100644 > --- a/arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts > +++ b/arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts > @@ -86,29 +86,13 @@ vcc12v_dcin: vcc12v-dcin-regulator { > regulator-boot-on; > }; > > - pcie30_avdd0v9: pcie30-avdd0v9-regulator { > - compatible = "regulator-fixed"; > - regulator-name = "pcie30_avdd0v9"; > - regulator-always-on; > - regulator-boot-on; > - regulator-min-microvolt = <900000>; > - regulator-max-microvolt = <900000>; > - vin-supply = <&vcc3v3_sys>; > - }; > - > - pcie30_avdd1v8: pcie30-avdd1v8-regulator { > - compatible = "regulator-fixed"; > - regulator-name = "pcie30_avdd1v8"; > - regulator-always-on; > - regulator-boot-on; > - regulator-min-microvolt = <1800000>; > - regulator-max-microvolt = <1800000>; > - vin-supply = <&vcc3v3_sys>; > - }; > - This change looks correct, but please make this a separate patch. One of the pmic regulators provides these voltage inputs to the soc side of the pcie controller. > /* pi6c pcie clock generator */ > vcc3v3_pi6c_03: vcc3v3-pi6c-03-regulator { > compatible = "regulator-fixed"; > + enable-active-high; > + gpios = <&gpio0 RK_PD4 GPIO_ACTIVE_HIGH>; > + pinctrl-names = "default"; > + pinctrl-0 = <&pcie_pwren_h>; > regulator-name = "vcc3v3_pi6c_03"; > regulator-always-on; > regulator-boot-on; > @@ -117,16 +101,13 @@ vcc3v3_pi6c_03: vcc3v3-pi6c-03-regulator { > vin-supply = <&vcc5v0_sys>; > }; > > - vcc3v3_pcie: vcc3v3-pcie-regulator { > + /* actually fed by vcc5v0_sys, dependent on pi6c clock generator */ > + vcc3v3_pcie30x1: vcc3v3-pcie30x1-regulator { > compatible = "regulator-fixed"; > - enable-active-high; > - gpios = <&gpio0 RK_PD4 GPIO_ACTIVE_HIGH>; > - pinctrl-names = "default"; > - pinctrl-0 = <&pcie_enable_h>; > - regulator-name = "vcc3v3_pcie"; > + regulator-name = "vcc3v3_pcie30x1"; > regulator-min-microvolt = <3300000>; > regulator-max-microvolt = <3300000>; > - vin-supply = <&vcc5v0_sys>; > + vin-supply = <&vcc3v3_pi6c_03>; > }; > > vcc3v3_sys: vcc3v3-sys-regulator { > @@ -615,9 +596,9 @@ &pcie30phy { > > &pcie3x2 { > pinctrl-names = "default"; > - pinctrl-0 = <&pcie30x2m1_pins>; > + pinctrl-0 = <&pcie30x2_perstn_m1>; you're again mixing in pinctrl changes into a patch about the regulator hirarchy. Please don't do that. Heiko
diff --git a/arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts b/arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts index f94cbddf0f0c2..6b3f3ee7f22c7 100644 --- a/arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts +++ b/arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts @@ -86,29 +86,13 @@ vcc12v_dcin: vcc12v-dcin-regulator { regulator-boot-on; }; - pcie30_avdd0v9: pcie30-avdd0v9-regulator { - compatible = "regulator-fixed"; - regulator-name = "pcie30_avdd0v9"; - regulator-always-on; - regulator-boot-on; - regulator-min-microvolt = <900000>; - regulator-max-microvolt = <900000>; - vin-supply = <&vcc3v3_sys>; - }; - - pcie30_avdd1v8: pcie30-avdd1v8-regulator { - compatible = "regulator-fixed"; - regulator-name = "pcie30_avdd1v8"; - regulator-always-on; - regulator-boot-on; - regulator-min-microvolt = <1800000>; - regulator-max-microvolt = <1800000>; - vin-supply = <&vcc3v3_sys>; - }; - /* pi6c pcie clock generator */ vcc3v3_pi6c_03: vcc3v3-pi6c-03-regulator { compatible = "regulator-fixed"; + enable-active-high; + gpios = <&gpio0 RK_PD4 GPIO_ACTIVE_HIGH>; + pinctrl-names = "default"; + pinctrl-0 = <&pcie_pwren_h>; regulator-name = "vcc3v3_pi6c_03"; regulator-always-on; regulator-boot-on; @@ -117,16 +101,13 @@ vcc3v3_pi6c_03: vcc3v3-pi6c-03-regulator { vin-supply = <&vcc5v0_sys>; }; - vcc3v3_pcie: vcc3v3-pcie-regulator { + /* actually fed by vcc5v0_sys, dependent on pi6c clock generator */ + vcc3v3_pcie30x1: vcc3v3-pcie30x1-regulator { compatible = "regulator-fixed"; - enable-active-high; - gpios = <&gpio0 RK_PD4 GPIO_ACTIVE_HIGH>; - pinctrl-names = "default"; - pinctrl-0 = <&pcie_enable_h>; - regulator-name = "vcc3v3_pcie"; + regulator-name = "vcc3v3_pcie30x1"; regulator-min-microvolt = <3300000>; regulator-max-microvolt = <3300000>; - vin-supply = <&vcc5v0_sys>; + vin-supply = <&vcc3v3_pi6c_03>; }; vcc3v3_sys: vcc3v3-sys-regulator { @@ -615,9 +596,9 @@ &pcie30phy { &pcie3x2 { pinctrl-names = "default"; - pinctrl-0 = <&pcie30x2m1_pins>; + pinctrl-0 = <&pcie30x2_perstn_m1>; reset-gpios = <&gpio2 RK_PD6 GPIO_ACTIVE_HIGH>; - vpcie3v3-supply = <&vcc3v3_pcie>; + vpcie3v3-supply = <&vcc3v3_pcie30x1>; status = "okay"; }; @@ -657,7 +638,11 @@ pcie20_perstn_m1: pcie20-perstn-m1 { rockchip,pins = <3 RK_PC1 RK_FUNC_GPIO &pcfg_pull_none>; }; - pcie_enable_h: pcie-enable-h { + pcie30x2_perstn_m1: pcie30x2-perstn-m1 { + rockchip,pins = <2 RK_PD6 RK_FUNC_GPIO &pcfg_pull_none>; + }; + + pcie_pwren_h: pcie-pwren-h { rockchip,pins = <0 RK_PD4 RK_FUNC_GPIO &pcfg_pull_none>; }; };
on Radxa ROCK 3A, GPIO0_D4 is used to enable both pi6c PCIe clock generator and "vcc3v3_pcie" regulator (PCIe3 M.2 M key connector). since pi6c needs to be enabled before using PCIe3, GPIO0_D4 need to be controlled by "vcc3v3_pi6c_03" regulator. then, make "vcc3v3_pi6c_03" vin-supply for "vcc3v3_pcie30x1". also, "pcie30_avdd0v9" and "pcie30_avdd1v8" are unused. remove them. Fixes: 0522cd811220 ("arm64: dts: rockchip: Add PCIe v3 nodes to rock-3a") Signed-off-by: FUKAUMI Naoki <naoki@radxa.com> --- Changes in v3: - fix pinctrl just for reset pin as GPIO Changes in v2: - split patches for PCIe2 and PCIe3 - change regulator name from "vcc3v3_pcie" to "vcc3v3_pcie30x1" - add comment for vin-supply of "vcc3v3_pcie30x1" regulator - remove unused "pcie30_avdd0v9" and "pcie30_avdd1v8" - fix pinctrl node name to overwrite rk3568-pinctrl.dtsi --- .../boot/dts/rockchip/rk3568-rock-3a.dts | 45 +++++++------------ 1 file changed, 15 insertions(+), 30 deletions(-)