| Message ID | 20241001235046.1710-1-naoki@radxa.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] arm64: dts: rockchip: change pinctrl for pcie2x1l2 for Radxa ROCK 5A | expand |
On Tue, 1 Oct 2024 23:50:46 +0000, FUKAUMI Naoki wrote: > for pcie2x1l2, only pcie20x1_2_perstn_m0 is required, and its function > needs to be GPIO to avoid freeze at "pci enum" without PCIe device on > u-boot. > > change pinctrl definitions for pcie2x1l2. no functional change is > intended on Linux kernel. > > [...] Applied, thanks! [1/1] arm64: dts: rockchip: change pinctrl for pcie2x1l2 for Radxa ROCK 5A commit: a86fca38c8129aac1cc7cec886e1eb306816b7d5 Best regards,
Hi, On 2024-10-02 01:50, FUKAUMI Naoki wrote: > for pcie2x1l2, only pcie20x1_2_perstn_m0 is required, and its function > needs to be GPIO to avoid freeze at "pci enum" without PCIe device on > u-boot. > > change pinctrl definitions for pcie2x1l2. no functional change is > intended on Linux kernel. After the split and addition of pcie2_reset I think this patch is no longer needed? The issue this patch tried to fix was already fixed/changed in "arm64: dts: rockchip: Split up RK3588's PCIe pinctrls". Looks like this now just rename pcie2_reset to pcie20x1_2_perstn_m0? Regards, Jonas > > Signed-off-by: FUKAUMI Naoki <naoki@radxa.com> > --- > Changed in v3: > - rebase on next/master > Changed in v2: > - reword commit message > --- > arch/arm64/boot/dts/rockchip/rk3588s-rock-5a.dts | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5a.dts b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5a.dts > index 87fce8d9a964..841ac9a30628 100644 > --- a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5a.dts > +++ b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5a.dts > @@ -310,7 +310,7 @@ rgmii_phy1: ethernet-phy@1 { > }; > > &pcie2x1l2 { > - pinctrl-0 = <&pcie2_reset>, <&pcie20x1m0_clkreqn>, <&pcie20x1m0_waken>; > + pinctrl-0 = <&pcie20x1_2_perstn_m0>; > pinctrl-names = "default"; > reset-gpios = <&gpio3 RK_PD1 GPIO_ACTIVE_HIGH>; > vpcie3v3-supply = <&vcc3v3_wf>; > @@ -325,12 +325,12 @@ io_led: io-led { > }; > > pcie { > - pow_en: pow-en { > - rockchip,pins = <0 RK_PC5 RK_FUNC_GPIO &pcfg_pull_none>; > + pcie20x1_2_perstn_m0: pcie20x1-2-perstn-m0 { > + rockchip,pins = <3 RK_PD1 RK_FUNC_GPIO &pcfg_pull_none>; > }; > > - pcie2_reset: pcie2-reset { > - rockchip,pins = <3 RK_PD1 RK_FUNC_GPIO &pcfg_pull_none>; > + pow_en: pow-en { > + rockchip,pins = <0 RK_PC5 RK_FUNC_GPIO &pcfg_pull_none>; > }; > }; >
Am Mittwoch, 2. Oktober 2024, 12:50:29 CEST schrieb Jonas Karlman: > Hi, > > On 2024-10-02 01:50, FUKAUMI Naoki wrote: > > for pcie2x1l2, only pcie20x1_2_perstn_m0 is required, and its function > > needs to be GPIO to avoid freeze at "pci enum" without PCIe device on > > u-boot. > > > > change pinctrl definitions for pcie2x1l2. no functional change is > > intended on Linux kernel. > > After the split and addition of pcie2_reset I think this patch is no > longer needed? The issue this patch tried to fix was already > fixed/changed in "arm64: dts: rockchip: Split up RK3588's PCIe pinctrls". > > Looks like this now just rename pcie2_reset to pcie20x1_2_perstn_m0? and removes the other pinctrl states clkreqn and waken . In a previous version they mentioned that this somehow affects u-boot. But you're right, that renaming of the pinctrl reset entry should definitly not be in there. > > Signed-off-by: FUKAUMI Naoki <naoki@radxa.com> > > --- > > Changed in v3: > > - rebase on next/master > > Changed in v2: > > - reword commit message > > --- > > arch/arm64/boot/dts/rockchip/rk3588s-rock-5a.dts | 10 +++++----- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5a.dts b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5a.dts > > index 87fce8d9a964..841ac9a30628 100644 > > --- a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5a.dts > > +++ b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5a.dts > > @@ -310,7 +310,7 @@ rgmii_phy1: ethernet-phy@1 { > > }; > > > > &pcie2x1l2 { > > - pinctrl-0 = <&pcie2_reset>, <&pcie20x1m0_clkreqn>, <&pcie20x1m0_waken>; > > + pinctrl-0 = <&pcie20x1_2_perstn_m0>; > > pinctrl-names = "default"; > > reset-gpios = <&gpio3 RK_PD1 GPIO_ACTIVE_HIGH>; > > vpcie3v3-supply = <&vcc3v3_wf>; > > @@ -325,12 +325,12 @@ io_led: io-led { > > }; > > > > pcie { > > - pow_en: pow-en { > > - rockchip,pins = <0 RK_PC5 RK_FUNC_GPIO &pcfg_pull_none>; > > + pcie20x1_2_perstn_m0: pcie20x1-2-perstn-m0 { > > + rockchip,pins = <3 RK_PD1 RK_FUNC_GPIO &pcfg_pull_none>; > > }; > > > > - pcie2_reset: pcie2-reset { > > - rockchip,pins = <3 RK_PD1 RK_FUNC_GPIO &pcfg_pull_none>; > > + pow_en: pow-en { > > + rockchip,pins = <0 RK_PC5 RK_FUNC_GPIO &pcfg_pull_none>; > > }; > > }; > > > >
hi, On 10/2/24 20:21, Heiko Stübner wrote: > Am Mittwoch, 2. Oktober 2024, 12:50:29 CEST schrieb Jonas Karlman: >> Hi, >> >> On 2024-10-02 01:50, FUKAUMI Naoki wrote: >>> for pcie2x1l2, only pcie20x1_2_perstn_m0 is required, and its function >>> needs to be GPIO to avoid freeze at "pci enum" without PCIe device on >>> u-boot. >>> >>> change pinctrl definitions for pcie2x1l2. no functional change is >>> intended on Linux kernel. >> >> After the split and addition of pcie2_reset I think this patch is no >> longer needed? The issue this patch tried to fix was already >> fixed/changed in "arm64: dts: rockchip: Split up RK3588's PCIe pinctrls". >> >> Looks like this now just rename pcie2_reset to pcie20x1_2_perstn_m0? > > and removes the other pinctrl states clkreqn and waken . > > In a previous version they mentioned that this somehow affects u-boot. > > But you're right, that renaming of the pinctrl reset entry should > definitly not be in there. I wonder where "pcie2_reset" comes from. in schematic, only "pcie20x1_2_perstn_m0" is used. Best regards, -- FUKAUMI Naoki Radxa Computer (Shenzhen) Co., Ltd. >>> Signed-off-by: FUKAUMI Naoki <naoki@radxa.com> >>> --- >>> Changed in v3: >>> - rebase on next/master >>> Changed in v2: >>> - reword commit message >>> --- >>> arch/arm64/boot/dts/rockchip/rk3588s-rock-5a.dts | 10 +++++----- >>> 1 file changed, 5 insertions(+), 5 deletions(-) >>> >>> diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5a.dts b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5a.dts >>> index 87fce8d9a964..841ac9a30628 100644 >>> --- a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5a.dts >>> +++ b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5a.dts >>> @@ -310,7 +310,7 @@ rgmii_phy1: ethernet-phy@1 { >>> }; >>> >>> &pcie2x1l2 { >>> - pinctrl-0 = <&pcie2_reset>, <&pcie20x1m0_clkreqn>, <&pcie20x1m0_waken>; >>> + pinctrl-0 = <&pcie20x1_2_perstn_m0>; >>> pinctrl-names = "default"; >>> reset-gpios = <&gpio3 RK_PD1 GPIO_ACTIVE_HIGH>; >>> vpcie3v3-supply = <&vcc3v3_wf>; >>> @@ -325,12 +325,12 @@ io_led: io-led { >>> }; >>> >>> pcie { >>> - pow_en: pow-en { >>> - rockchip,pins = <0 RK_PC5 RK_FUNC_GPIO &pcfg_pull_none>; >>> + pcie20x1_2_perstn_m0: pcie20x1-2-perstn-m0 { >>> + rockchip,pins = <3 RK_PD1 RK_FUNC_GPIO &pcfg_pull_none>; >>> }; >>> >>> - pcie2_reset: pcie2-reset { >>> - rockchip,pins = <3 RK_PD1 RK_FUNC_GPIO &pcfg_pull_none>; >>> + pow_en: pow-en { >>> + rockchip,pins = <0 RK_PC5 RK_FUNC_GPIO &pcfg_pull_none>; >>> }; >>> }; >>> >> >> > > > > >
On 2024-10-02 13:21, Heiko Stübner wrote: > Am Mittwoch, 2. Oktober 2024, 12:50:29 CEST schrieb Jonas Karlman: >> Hi, >> >> On 2024-10-02 01:50, FUKAUMI Naoki wrote: >>> for pcie2x1l2, only pcie20x1_2_perstn_m0 is required, and its function >>> needs to be GPIO to avoid freeze at "pci enum" without PCIe device on >>> u-boot. >>> >>> change pinctrl definitions for pcie2x1l2. no functional change is >>> intended on Linux kernel. >> >> After the split and addition of pcie2_reset I think this patch is no >> longer needed? The issue this patch tried to fix was already >> fixed/changed in "arm64: dts: rockchip: Split up RK3588's PCIe pinctrls". >> >> Looks like this now just rename pcie2_reset to pcie20x1_2_perstn_m0? > > and removes the other pinctrl states clkreqn and waken . This is not something that I think should have been done in the first place, the pins/signals exists in hw and schematics, however software is not using these signals. > > In a previous version they mentioned that this somehow affects u-boot. The issue with U-Boot is that for pcie the pin used for reset-gpios must use gpio func pinconf, or pci enumerating in U-Boot will freeze the board. "arm64: dts: rockchip: Split up RK3588's PCIe pinctrls" already changed to use the gpio func for the perstn pin, so the U-Boot issue should already have been resolved with that patch. For Linux the pins gpio func is implicitly configured when the pcie driver request the reset-gpios pin. In U-Boot there exists some pinctrl overrides to solve freeze issues, those should also be sent upstream ;-) Regards, Jonas > > But you're right, that renaming of the pinctrl reset entry should > definitly not be in there. > > >>> Signed-off-by: FUKAUMI Naoki <naoki@radxa.com> >>> --- >>> Changed in v3: >>> - rebase on next/master >>> Changed in v2: >>> - reword commit message >>> --- >>> arch/arm64/boot/dts/rockchip/rk3588s-rock-5a.dts | 10 +++++----- >>> 1 file changed, 5 insertions(+), 5 deletions(-) >>> >>> diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5a.dts b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5a.dts >>> index 87fce8d9a964..841ac9a30628 100644 >>> --- a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5a.dts >>> +++ b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5a.dts >>> @@ -310,7 +310,7 @@ rgmii_phy1: ethernet-phy@1 { >>> }; >>> >>> &pcie2x1l2 { >>> - pinctrl-0 = <&pcie2_reset>, <&pcie20x1m0_clkreqn>, <&pcie20x1m0_waken>; >>> + pinctrl-0 = <&pcie20x1_2_perstn_m0>; >>> pinctrl-names = "default"; >>> reset-gpios = <&gpio3 RK_PD1 GPIO_ACTIVE_HIGH>; >>> vpcie3v3-supply = <&vcc3v3_wf>; >>> @@ -325,12 +325,12 @@ io_led: io-led { >>> }; >>> >>> pcie { >>> - pow_en: pow-en { >>> - rockchip,pins = <0 RK_PC5 RK_FUNC_GPIO &pcfg_pull_none>; >>> + pcie20x1_2_perstn_m0: pcie20x1-2-perstn-m0 { >>> + rockchip,pins = <3 RK_PD1 RK_FUNC_GPIO &pcfg_pull_none>; >>> }; >>> >>> - pcie2_reset: pcie2-reset { >>> - rockchip,pins = <3 RK_PD1 RK_FUNC_GPIO &pcfg_pull_none>; >>> + pow_en: pow-en { >>> + rockchip,pins = <0 RK_PC5 RK_FUNC_GPIO &pcfg_pull_none>; >>> }; >>> }; >>> >> >> > > > >
Am Mittwoch, 2. Oktober 2024, 13:42:05 CEST schrieb Jonas Karlman: > On 2024-10-02 13:21, Heiko Stübner wrote: > > Am Mittwoch, 2. Oktober 2024, 12:50:29 CEST schrieb Jonas Karlman: > >> Hi, > >> > >> On 2024-10-02 01:50, FUKAUMI Naoki wrote: > >>> for pcie2x1l2, only pcie20x1_2_perstn_m0 is required, and its function > >>> needs to be GPIO to avoid freeze at "pci enum" without PCIe device on > >>> u-boot. > >>> > >>> change pinctrl definitions for pcie2x1l2. no functional change is > >>> intended on Linux kernel. > >> > >> After the split and addition of pcie2_reset I think this patch is no > >> longer needed? The issue this patch tried to fix was already > >> fixed/changed in "arm64: dts: rockchip: Split up RK3588's PCIe pinctrls". > >> > >> Looks like this now just rename pcie2_reset to pcie20x1_2_perstn_m0? > > > > and removes the other pinctrl states clkreqn and waken . > > This is not something that I think should have been done in the first > place, the pins/signals exists in hw and schematics, however software > is not using these signals. > > > > > In a previous version they mentioned that this somehow affects u-boot. > > The issue with U-Boot is that for pcie the pin used for reset-gpios must > use gpio func pinconf, or pci enumerating in U-Boot will freeze the > board. > > "arm64: dts: rockchip: Split up RK3588's PCIe pinctrls" already changed > to use the gpio func for the perstn pin, so the U-Boot issue should > already have been resolved with that patch. > > For Linux the pins gpio func is implicitly configured when the pcie > driver request the reset-gpios pin. In U-Boot there exists some pinctrl > overrides to solve freeze issues, those should also be sent upstream ;-) ok, so I'll just drop this patch for now. If some other use comes up, please resubmit. Thanks Heiko
diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5a.dts b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5a.dts index 87fce8d9a964..841ac9a30628 100644 --- a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5a.dts +++ b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5a.dts @@ -310,7 +310,7 @@ rgmii_phy1: ethernet-phy@1 { }; &pcie2x1l2 { - pinctrl-0 = <&pcie2_reset>, <&pcie20x1m0_clkreqn>, <&pcie20x1m0_waken>; + pinctrl-0 = <&pcie20x1_2_perstn_m0>; pinctrl-names = "default"; reset-gpios = <&gpio3 RK_PD1 GPIO_ACTIVE_HIGH>; vpcie3v3-supply = <&vcc3v3_wf>; @@ -325,12 +325,12 @@ io_led: io-led { }; pcie { - pow_en: pow-en { - rockchip,pins = <0 RK_PC5 RK_FUNC_GPIO &pcfg_pull_none>; + pcie20x1_2_perstn_m0: pcie20x1-2-perstn-m0 { + rockchip,pins = <3 RK_PD1 RK_FUNC_GPIO &pcfg_pull_none>; }; - pcie2_reset: pcie2-reset { - rockchip,pins = <3 RK_PD1 RK_FUNC_GPIO &pcfg_pull_none>; + pow_en: pow-en { + rockchip,pins = <0 RK_PC5 RK_FUNC_GPIO &pcfg_pull_none>; }; };
for pcie2x1l2, only pcie20x1_2_perstn_m0 is required, and its function needs to be GPIO to avoid freeze at "pci enum" without PCIe device on u-boot. change pinctrl definitions for pcie2x1l2. no functional change is intended on Linux kernel. Signed-off-by: FUKAUMI Naoki <naoki@radxa.com> --- Changed in v3: - rebase on next/master Changed in v2: - reword commit message --- arch/arm64/boot/dts/rockchip/rk3588s-rock-5a.dts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)