| Message ID | 20240912235923.1013-1-naoki@radxa.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: dts: rockchip: fix PCIe regulators for Radxa ROCK 3A | expand |
> @@ -119,14 +123,10 @@ vcc3v3_pi6c_03: vcc3v3-pi6c-03-regulator { Please add /* actually fed by vcc5v0_sys */ > vcc3v3_pcie: vcc3v3-pcie-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-min-microvolt = <3300000>; > regulator-max-microvolt = <3300000>; > - vin-supply = <&vcc5v0_sys>; > + vin-supply = <&vcc3v3_pi6c_03>; > }; I recommend renaming vcc3v3_pcie to vcc3v3_pcie30x1, which better matches the schematic. > &pcie2x1 { > pinctrl-names = "default"; > - pinctrl-0 = <&pcie_reset_h>; > + pinctrl-0 = <&pcie2x1m1_pins>; > reset-gpios = <&gpio3 RK_PC1 GPIO_ACTIVE_HIGH>; > - vpcie3v3-supply = <&vcc3v3_pcie>; > + vpcie3v3-supply = <&vcc3v3_wf>; > status = "okay"; > }; Please separate the changes for pcie2x1 and pcie3 into 2 patches. > + pcie2x1m1_pins: pcie2x1m1-pins { > + rockchip,pins = > + /* pcie20_clkreqnm1 */ > + <2 RK_PD0 4 &pcfg_pull_none>, > + /* pcie20_perstnm1 */ > + <3 RK_PC1 RK_FUNC_GPIO &pcfg_pull_none>, > + /* pcie20_wakenm1 */ > + <2 RK_PD1 4 &pcfg_pull_none>; Why not pcie20m1_pins? > + pcie3x2m1_pins: pcie3x2m1-pins { > + rockchip,pins = > + /* pcie30x2_clkreqnm1 */ > + <2 RK_PD4 4 &pcfg_pull_none>, > + /* pcie30x2_perstnm1 */ > + <2 RK_PD6 RK_FUNC_GPIO &pcfg_pull_none>, > + /* pcie30x2_wakenm1 */ > + <2 RK_PD5 4 &pcfg_pull_none>; > + }; Why not pcie30x2m1_pins? Also missing blank line. > + pcie_pwren_h: pcie-pwren-h { > + rockchip,pins = <0 RK_PD4 RK_FUNC_GPIO &pcfg_pull_none>; > }; Thanks, Chukun
hi, On 9/13/24 12:30, Chukun Pan wrote: >> @@ -119,14 +123,10 @@ vcc3v3_pi6c_03: vcc3v3-pi6c-03-regulator { > > Please add /* actually fed by vcc5v0_sys */ > >> vcc3v3_pcie: vcc3v3-pcie-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-min-microvolt = <3300000>; >> regulator-max-microvolt = <3300000>; >> - vin-supply = <&vcc5v0_sys>; >> + vin-supply = <&vcc3v3_pi6c_03>; >> }; > > I recommend renaming vcc3v3_pcie to vcc3v3_pcie30x1, which better > matches the schematic. ok >> &pcie2x1 { >> pinctrl-names = "default"; >> - pinctrl-0 = <&pcie_reset_h>; >> + pinctrl-0 = <&pcie2x1m1_pins>; >> reset-gpios = <&gpio3 RK_PC1 GPIO_ACTIVE_HIGH>; >> - vpcie3v3-supply = <&vcc3v3_pcie>; >> + vpcie3v3-supply = <&vcc3v3_wf>; >> status = "okay"; >> }; > > Please separate the changes for pcie2x1 and pcie3 into 2 patches. ok >> + pcie2x1m1_pins: pcie2x1m1-pins { >> + rockchip,pins = >> + /* pcie20_clkreqnm1 */ >> + <2 RK_PD0 4 &pcfg_pull_none>, >> + /* pcie20_perstnm1 */ >> + <3 RK_PC1 RK_FUNC_GPIO &pcfg_pull_none>, >> + /* pcie20_wakenm1 */ >> + <2 RK_PD1 4 &pcfg_pull_none>; > > Why not pcie20m1_pins? $ make dtbs DTC arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dtb arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts:656.32-664.5: ERROR (duplicate_label): /pinctrl/pcie/pcie20m1-pins: Duplicate label 'pcie20m1_pins' on /pinctrl/pcie/pcie20m1-pins and /pinctrl/pcie20/pcie20m1-pins we need gpio pin for reset. >> + pcie3x2m1_pins: pcie3x2m1-pins { >> + rockchip,pins = >> + /* pcie30x2_clkreqnm1 */ >> + <2 RK_PD4 4 &pcfg_pull_none>, >> + /* pcie30x2_perstnm1 */ >> + <2 RK_PD6 RK_FUNC_GPIO &pcfg_pull_none>, >> + /* pcie30x2_wakenm1 */ >> + <2 RK_PD5 4 &pcfg_pull_none>; >> + }; > > Why not pcie30x2m1_pins? $ make dtbs DTC arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dtb arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts:665.36-673.5: ERROR (duplicate_label): /pinctrl/pcie/pcie30x2m1-pins: Duplicate label 'pcie30x2m1_pins' on /pinctrl/pcie/pcie30x2m1-pins and /pinctrl/pcie30x2/pcie30x2m1-pins > Also missing blank line. > >> + pcie_pwren_h: pcie-pwren-h { >> + rockchip,pins = <0 RK_PD4 RK_FUNC_GPIO &pcfg_pull_none>; >> }; > > Thanks, > Chukun thank you very much for your review. Best regards, -- FUKAUMI Naoki Radxa Computer (Shenzhen) Co., Ltd.
> arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts:656.32-664.5: ERROR > (duplicate_label): /pinctrl/pcie/pcie20m1-pins: Duplicate label > 'pcie20m1_pins' on /pinctrl/pcie/pcie20m1-pins and > /pinctrl/pcie20/pcie20m1-pins > > we need gpio pin for reset. Can't it work just written like this? &pcie2x1 { pinctrl-names = "default"; pinctrl-0 = <&pcie20m1_pins>; reset-gpios = <&gpio3 RK_PC1 GPIO_ACTIVE_HIGH>; vpcie3v3-supply = <&vcc3v3_pcie>; status = "okay"; }; If not, please overwrite the pinctrl: pcie20 { pcie20m1_pins: pcie20m1-pins { Thanks, Chukun
hi, On 9/13/24 16:02, Chukun Pan wrote: >> arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts:656.32-664.5: ERROR >> (duplicate_label): /pinctrl/pcie/pcie20m1-pins: Duplicate label >> 'pcie20m1_pins' on /pinctrl/pcie/pcie20m1-pins and >> /pinctrl/pcie20/pcie20m1-pins >> >> we need gpio pin for reset. sorry, "U-Boot need gpio pin" > Can't it work just written like this? > > &pcie2x1 { > pinctrl-names = "default"; > pinctrl-0 = <&pcie20m1_pins>; > reset-gpios = <&gpio3 RK_PC1 GPIO_ACTIVE_HIGH>; > vpcie3v3-supply = <&vcc3v3_pcie>; > status = "okay"; > }; > > If not, please overwrite the pinctrl: > > pcie20 { > pcie20m1_pins: pcie20m1-pins { I didn't know how to overwrite it. thank you. Best regards, -- FUKAUMI Naoki Radxa Computer (Shenzhen) Co., Ltd. > Thanks, > Chukun >
> sorry, "U-Boot need gpio pin" > I didn't know how to overwrite it. thank you. You can look at the two files: rk3568-rock-3b.dts rk3568-pinctrl.dtsi > <3 RK_PC1 RK_FUNC_GPIO &pcfg_pull_none> > <2 RK_PD6 RK_FUNC_GPIO &pcfg_pull_none> So this is only needed by uboot? Thanks, Chukun
hi On 9/13/24 19:00, Chukun Pan wrote: >> sorry, "U-Boot need gpio pin" >> I didn't know how to overwrite it. thank you. > > You can look at the two files: > rk3568-rock-3b.dts > rk3568-pinctrl.dtsi thanks. now you know what I want to do :) >> <3 RK_PC1 RK_FUNC_GPIO &pcfg_pull_none> >> <2 RK_PD6 RK_FUNC_GPIO &pcfg_pull_none> > > So this is only needed by uboot? hmm. sorry, anything which resets PCIe via GPIO. Best regards, -- FUKAUMI Naoki Radxa Computer (Shenzhen) Co., Ltd. > Thanks, > Chukun >
hi btw, how do you think about pcie30_avdd0v9 and pcie30_avdd1v8? as far as I can see, it's unused. Best regards, -- FUKAUMI Naoki Radxa Computer (Shenzhen) Co., Ltd. On 9/13/24 19:00, Chukun Pan wrote: >> sorry, "U-Boot need gpio pin" >> I didn't know how to overwrite it. thank you. > > You can look at the two files: > rk3568-rock-3b.dts > rk3568-pinctrl.dtsi > >> <3 RK_PC1 RK_FUNC_GPIO &pcfg_pull_none> >> <2 RK_PD6 RK_FUNC_GPIO &pcfg_pull_none> > > So this is only needed by uboot? > > Thanks, > Chukun >
> btw, how do you think about pcie30_avdd0v9 and pcie30_avdd1v8? > as far as I can see, it's unused. The schematic notes that PCIe uses VDDA_0V9 and VCCA_1V8, so they can be removed. Thanks, Chukun
diff --git a/arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts b/arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts index 59f1403b4fa56..885196c58b915 100644 --- a/arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts +++ b/arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts @@ -109,6 +109,10 @@ pcie30_avdd1v8: pcie30-avdd1v8-regulator { /* 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; @@ -119,14 +123,10 @@ vcc3v3_pi6c_03: vcc3v3-pi6c-03-regulator { vcc3v3_pcie: vcc3v3-pcie-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-min-microvolt = <3300000>; regulator-max-microvolt = <3300000>; - vin-supply = <&vcc5v0_sys>; + vin-supply = <&vcc3v3_pi6c_03>; }; vcc3v3_sys: vcc3v3-sys-regulator { @@ -136,7 +136,17 @@ vcc3v3_sys: vcc3v3-sys-regulator { regulator-boot-on; regulator-min-microvolt = <3300000>; regulator-max-microvolt = <3300000>; - vin-supply = <&vcc12v_dcin>; + vin-supply = <&vcc5v0_sys>; + }; + + vcc3v3_wf: vcc3v3-wf-regulator { + compatible = "regulator-fixed"; + regulator-name = "vcc3v3_wf"; + regulator-always-on; + regulator-boot-on; + regulator-min-microvolt = <3300000>; + regulator-max-microvolt = <3300000>; + vin-supply = <&vcc3v3_sys>; }; vcc5v0_sys: vcc5v0-sys-regulator { @@ -592,9 +602,9 @@ rgmii_phy1: ethernet-phy@0 { &pcie2x1 { pinctrl-names = "default"; - pinctrl-0 = <&pcie_reset_h>; + pinctrl-0 = <&pcie2x1m1_pins>; reset-gpios = <&gpio3 RK_PC1 GPIO_ACTIVE_HIGH>; - vpcie3v3-supply = <&vcc3v3_pcie>; + vpcie3v3-supply = <&vcc3v3_wf>; status = "okay"; }; @@ -605,7 +615,7 @@ &pcie30phy { &pcie3x2 { pinctrl-names = "default"; - pinctrl-0 = <&pcie30x2m1_pins>; + pinctrl-0 = <&pcie3x2m1_pins>; reset-gpios = <&gpio2 RK_PD6 GPIO_ACTIVE_HIGH>; vpcie3v3-supply = <&vcc3v3_pcie>; status = "okay"; @@ -643,12 +653,26 @@ led_user_en: led_user_en { }; pcie { - pcie_enable_h: pcie-enable-h { - rockchip,pins = <0 RK_PD4 RK_FUNC_GPIO &pcfg_pull_none>; + pcie2x1m1_pins: pcie2x1m1-pins { + rockchip,pins = + /* pcie20_clkreqnm1 */ + <2 RK_PD0 4 &pcfg_pull_none>, + /* pcie20_perstnm1 */ + <3 RK_PC1 RK_FUNC_GPIO &pcfg_pull_none>, + /* pcie20_wakenm1 */ + <2 RK_PD1 4 &pcfg_pull_none>; }; - - pcie_reset_h: pcie-reset-h { - rockchip,pins = <3 RK_PC1 RK_FUNC_GPIO &pcfg_pull_none>; + pcie3x2m1_pins: pcie3x2m1-pins { + rockchip,pins = + /* pcie30x2_clkreqnm1 */ + <2 RK_PD4 4 &pcfg_pull_none>, + /* pcie30x2_perstnm1 */ + <2 RK_PD6 RK_FUNC_GPIO &pcfg_pull_none>, + /* pcie30x2_wakenm1 */ + <2 RK_PD5 4 &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. so make "vcc3v3_pi6c_03" vin-supply for "vcc3v3_pcie". then, currently "vcc3v3_pcie" regulator is used for PCIe2 M.2 E key connector, but by schematic[1], it's wrong. "vcc3v3_wf" regulator is right one, add it and fix related vin-supply. in addition to above fixes, some cosmetic changes for pinctrl node names. no functional change is intended. [1] https://dl.radxa.com/rock3/docs/hw/3a/radxa_rock_3a_v1310_schematic.pdf tested with Radxa Wireless Module A8 on PCIe2 and Dual 2.5G Router HAT on PCIe3. $ lspci 0000:00:00.0 PCI bridge: Rockchip Electronics Co., Ltd RK3568 Remote Signal Processor (rev 01) 0000:01:00.0 Network controller: Realtek Semiconductor Co., Ltd. RTL8852BE PCIe 802.11ax Wireless Network Controller 0002:00:00.0 PCI bridge: Rockchip Electronics Co., Ltd RK3568 Remote Signal Processor (rev 01) 0002:01:00.0 PCI bridge: ASMedia Technology Inc. ASM2806 4-Port PCIe x2 Gen3 Packet Switch (rev 01) 0002:02:00.0 PCI bridge: ASMedia Technology Inc. ASM2806 4-Port PCIe x2 Gen3 Packet Switch (rev 01) 0002:02:02.0 PCI bridge: ASMedia Technology Inc. ASM2806 4-Port PCIe x2 Gen3 Packet Switch (rev 01) 0002:02:06.0 PCI bridge: ASMedia Technology Inc. ASM2806 4-Port PCIe x2 Gen3 Packet Switch (rev 01) 0002:02:0e.0 PCI bridge: ASMedia Technology Inc. ASM2806 4-Port PCIe x2 Gen3 Packet Switch (rev 01) 0002:03:00.0 Non-Volatile memory controller: ADATA Technology Co., Ltd. LEGEND 700, XPG GAMMIX S20 NVMe SSD (DRAM-less) (rev 03) 0002:05:00.0 Ethernet controller: Realtek Semiconductor Co., Ltd. RTL8125 2.5GbE Controller (rev 05) 0002:06:00.0 Ethernet controller: Realtek Semiconductor Co., Ltd. RTL8125 2.5GbE Controller (rev 05) $ lspci -t -[0000:00]---00.0-[01-ff]----00.0 -[0002:00]---00.0-[01-ff]----00.0-[02-06]--+-00.0-[03]----00.0 +-02.0-[04]-- +-06.0-[05]----00.0 \-0e.0-[06]----00.0 Fixes: 0522cd811220 ("arm64: dts: rockchip: Add PCIe v3 nodes to rock-3a") Signed-off-by: FUKAUMI Naoki <naoki@radxa.com> --- .../boot/dts/rockchip/rk3568-rock-3a.dts | 52 ++++++++++++++----- 1 file changed, 38 insertions(+), 14 deletions(-)