[PATCH] arm64: dts: rockchip: align bindings to PCIe spec

[PATCH] arm64: dts: rockchip: align bindings to PCIe spec

[Geraldo Nascimento]

The PERST# side-band signal is defined by PCIe spec as an open-drain
active-low signal that depends on a pull-up resistor to keep the
signal high when deasserted. Align bindings to the spec.

Note that the relevant driver hacks the active-low signal as
active-high and switches the normal polarity of PERST#
assertion/deassertion, 1 and 0 in that order, and instead uses
0 to signal low (assertion) and 1 to signal deassertion.

Incidentally, this change makes hardware that refused to work
with the Rockchip-IP PCIe core working for me, which was the
object of many fool's errands.

Signed-off-by: Geraldo Nascimento <geraldogabriel@gmail.com>
---
 arch/arm64/boot/dts/rockchip/rk3399pro-vmarc-som.dtsi | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3399pro-vmarc-som.dtsi b/arch/arm64/boot/dts/rockchip/rk3399pro-vmarc-som.dtsi
index aa70776e898a..8dcb03708145 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399pro-vmarc-som.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399pro-vmarc-som.dtsi
@@ -383,9 +383,9 @@ &pcie_phy {
 };
 
 &pcie0 {
-	ep-gpios = <&gpio0 RK_PB4 GPIO_ACTIVE_HIGH>;
+	ep-gpios = <&gpio0 RK_PB4 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
 	num-lanes = <4>;
-	pinctrl-0 = <&pcie_clkreqnb_cpm>;
+	pinctrl-0 = <&pcie_clkreqnb_cpm>, <&pcie_perst>;
 	pinctrl-names = "default";
 	vpcie0v9-supply = <&vcca_0v9>;	/* VCC_0V9_S0 */
 	vpcie1v8-supply = <&vcca_1v8>;	/* VCC_1V8_S0 */
@@ -408,6 +408,10 @@ pcie {
 		pcie_pwr: pcie-pwr {
 			rockchip,pins = <4 RK_PD4 RK_FUNC_GPIO &pcfg_pull_up>;
 		};
+		pcie_perst: pcie-perst {
+			rockchip,pins = <0 RK_PB4 RK_FUNC_GPIO &pcfg_pull_up>;
+		};
+
 	};
 
 	pmic {
-- 
2.49.0

Re: [PATCH] arm64: dts: rockchip: align bindings to PCIe spec

[Shawn Lin]

Hi Geraldo,

在 2025/11/05 星期三 13:55, Geraldo Nascimento 写道:
> The PERST# side-band signal is defined by PCIe spec as an open-drain

I couldn't find any clue that says PERST# is an open-drain signal. Could
you quote it from PCI Express Card Electromechanical Specification?

> active-low signal that depends on a pull-up resistor to keep the
> signal high when deasserted. Align bindings to the spec.

This is not true from my POV. An open-drain PCIe side-band  signal
is used for both of EP and RC to achieve some special work-flow, like
CLKREQ# for L1ss, etc. Since both ends could control it. But PERST# is a
fundamental reset signal driven by RC which should be in sure state,
high or low, has nothing to do with open-drain.

> 
> Note that the relevant driver hacks the active-low signal as
> active-high and switches the normal polarity of PERST#
> assertion/deassertion, 1 and 0 in that order, and instead uses
> 0 to signal low (assertion) and 1 to signal deassertion.
> 
> Incidentally, this change makes hardware that refused to work
> with the Rockchip-IP PCIe core working for me, which was the
> object of many fool's errands.
> 
> Signed-off-by: Geraldo Nascimento <geraldogabriel@gmail.com>
> ---
>   arch/arm64/boot/dts/rockchip/rk3399pro-vmarc-som.dtsi | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3399pro-vmarc-som.dtsi b/arch/arm64/boot/dts/rockchip/rk3399pro-vmarc-som.dtsi
> index aa70776e898a..8dcb03708145 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3399pro-vmarc-som.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3399pro-vmarc-som.dtsi
> @@ -383,9 +383,9 @@ &pcie_phy {
>   };
>   
>   &pcie0 {
> -	ep-gpios = <&gpio0 RK_PB4 GPIO_ACTIVE_HIGH>;
> +	ep-gpios = <&gpio0 RK_PB4 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;

So my biggest guess is we don't need this change at all.
gpio0b4 is used as gpio function, the problem you faced is that it
didn't set gpio0b4 as pull-up, because the defaut state is pull-down.

Maybe the drive current of this IO is too weak, making it unable to 
fully drive high in the pull-down state? If that's the case, can you see 
a half-level signal on the oscilloscope?

>   	num-lanes = <4>;
> -	pinctrl-0 = <&pcie_clkreqnb_cpm>;
> +	pinctrl-0 = <&pcie_clkreqnb_cpm>, <&pcie_perst>;
>   	pinctrl-names = "default";
>   	vpcie0v9-supply = <&vcca_0v9>;	/* VCC_0V9_S0 */
>   	vpcie1v8-supply = <&vcca_1v8>;	/* VCC_1V8_S0 */
> @@ -408,6 +408,10 @@ pcie {
>   		pcie_pwr: pcie-pwr {
>   			rockchip,pins = <4 RK_PD4 RK_FUNC_GPIO &pcfg_pull_up>;
>   		};
> +		pcie_perst: pcie-perst {
> +			rockchip,pins = <0 RK_PB4 RK_FUNC_GPIO &pcfg_pull_up>;
> +		};
> +
>   	};
>   
>   	pmic {

Re: [PATCH] arm64: dts: rockchip: align bindings to PCIe spec

[Geraldo Nascimento]

On Wed, Nov 05, 2025 at 02:35:28PM +0800, Shawn Lin wrote:
> Hi Geraldo,
> 
> 在 2025/11/05 星期三 13:55, Geraldo Nascimento 写道:
> > The PERST# side-band signal is defined by PCIe spec as an open-drain
> 
> I couldn't find any clue that says PERST# is an open-drain signal. Could
> you quote it from PCI Express Card Electromechanical Specification?
> 
> > active-low signal that depends on a pull-up resistor to keep the
> > signal high when deasserted. Align bindings to the spec.
> 
> This is not true from my POV. An open-drain PCIe side-band  signal
> is used for both of EP and RC to achieve some special work-flow, like
> CLKREQ# for L1ss, etc. Since both ends could control it. But PERST# is a
> fundamental reset signal driven by RC which should be in sure state,
> high or low, has nothing to do with open-drain.
> 
> > 
> > Note that the relevant driver hacks the active-low signal as
> > active-high and switches the normal polarity of PERST#
> > assertion/deassertion, 1 and 0 in that order, and instead uses
> > 0 to signal low (assertion) and 1 to signal deassertion.
> > 
> > Incidentally, this change makes hardware that refused to work
> > with the Rockchip-IP PCIe core working for me, which was the
> > object of many fool's errands.
> > 
> > Signed-off-by: Geraldo Nascimento <geraldogabriel@gmail.com>
> > ---
> >   arch/arm64/boot/dts/rockchip/rk3399pro-vmarc-som.dtsi | 8 ++++++--
> >   1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3399pro-vmarc-som.dtsi b/arch/arm64/boot/dts/rockchip/rk3399pro-vmarc-som.dtsi
> > index aa70776e898a..8dcb03708145 100644
> > --- a/arch/arm64/boot/dts/rockchip/rk3399pro-vmarc-som.dtsi
> > +++ b/arch/arm64/boot/dts/rockchip/rk3399pro-vmarc-som.dtsi
> > @@ -383,9 +383,9 @@ &pcie_phy {
> >   };
> >   
> >   &pcie0 {
> > -	ep-gpios = <&gpio0 RK_PB4 GPIO_ACTIVE_HIGH>;
> > +	ep-gpios = <&gpio0 RK_PB4 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
> 
> So my biggest guess is we don't need this change at all.
> gpio0b4 is used as gpio function, the problem you faced is that it
> didn't set gpio0b4 as pull-up, because the defaut state is pull-down.
> 
> Maybe the drive current of this IO is too weak, making it unable to 
> fully drive high in the pull-down state? If that's the case, can you see 
> a half-level signal on the oscilloscope?
> 
> >   	num-lanes = <4>;
> > -	pinctrl-0 = <&pcie_clkreqnb_cpm>;
> > +	pinctrl-0 = <&pcie_clkreqnb_cpm>, <&pcie_perst>;
> >   	pinctrl-names = "default";
> >   	vpcie0v9-supply = <&vcca_0v9>;	/* VCC_0V9_S0 */
> >   	vpcie1v8-supply = <&vcca_1v8>;	/* VCC_1V8_S0 */
> > @@ -408,6 +408,10 @@ pcie {
> >   		pcie_pwr: pcie-pwr {
> >   			rockchip,pins = <4 RK_PD4 RK_FUNC_GPIO &pcfg_pull_up>;
> >   		};
> > +		pcie_perst: pcie-perst {
> > +			rockchip,pins = <0 RK_PB4 RK_FUNC_GPIO &pcfg_pull_up>;
> > +		};
> > +
> >   	};
> >   
> >   	pmic {
>

Hi Shawn, glad to hear from you.

Perhaps the following change is better? It resolves the issue
without the added complication of open drain. After you questioned
if open drain is actually part of the spec, I remembered that
GPIO_OPEN_DRAIN is actually (GPIO_SINGLE_ENDED | GPIO_LINE_OPEN_DRAIN)
so I decided to test with just GPIO_SINGLE_ENDED and it works.

Thanks,
Geraldo Nascimento

---

diff --git a/arch/arm64/boot/dts/rockchip/rk3399pro-vmarc-som.dtsi b/arch/arm64/boot/dts/rockchip/rk3399pro-vmarc-som.dtsi
index aa70776e898a..b3d19dce539f 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399pro-vmarc-som.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399pro-vmarc-som.dtsi
@@ -383,7 +383,7 @@ &pcie_phy {
 };
 
 &pcie0 {
-	ep-gpios = <&gpio0 RK_PB4 GPIO_ACTIVE_HIGH>;
+	ep-gpios = <&gpio0 RK_PB4 (GPIO_ACTIVE_HIGH | GPIO_SINGLE_ENDED)>;
 	num-lanes = <4>;
 	pinctrl-0 = <&pcie_clkreqnb_cpm>;
 	pinctrl-names = "default";

Re: [PATCH] arm64: dts: rockchip: align bindings to PCIe spec

[Shawn Lin]

在 2025/11/05 星期三 16:18, Geraldo Nascimento 写道:
> On Wed, Nov 05, 2025 at 02:35:28PM +0800, Shawn Lin wrote:
>> Hi Geraldo,
>>
>> 在 2025/11/05 星期三 13:55, Geraldo Nascimento 写道:
>>> The PERST# side-band signal is defined by PCIe spec as an open-drain
>>
>> I couldn't find any clue that says PERST# is an open-drain signal. Could
>> you quote it from PCI Express Card Electromechanical Specification?
>>
>>> active-low signal that depends on a pull-up resistor to keep the
>>> signal high when deasserted. Align bindings to the spec.
>>
>> This is not true from my POV. An open-drain PCIe side-band  signal
>> is used for both of EP and RC to achieve some special work-flow, like
>> CLKREQ# for L1ss, etc. Since both ends could control it. But PERST# is a
>> fundamental reset signal driven by RC which should be in sure state,
>> high or low, has nothing to do with open-drain.
>>
>>>
>>> Note that the relevant driver hacks the active-low signal as
>>> active-high and switches the normal polarity of PERST#
>>> assertion/deassertion, 1 and 0 in that order, and instead uses
>>> 0 to signal low (assertion) and 1 to signal deassertion.
>>>
>>> Incidentally, this change makes hardware that refused to work
>>> with the Rockchip-IP PCIe core working for me, which was the
>>> object of many fool's errands.
>>>
>>> Signed-off-by: Geraldo Nascimento <geraldogabriel@gmail.com>
>>> ---
>>>    arch/arm64/boot/dts/rockchip/rk3399pro-vmarc-som.dtsi | 8 ++++++--
>>>    1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3399pro-vmarc-som.dtsi b/arch/arm64/boot/dts/rockchip/rk3399pro-vmarc-som.dtsi
>>> index aa70776e898a..8dcb03708145 100644
>>> --- a/arch/arm64/boot/dts/rockchip/rk3399pro-vmarc-som.dtsi
>>> +++ b/arch/arm64/boot/dts/rockchip/rk3399pro-vmarc-som.dtsi
>>> @@ -383,9 +383,9 @@ &pcie_phy {
>>>    };
>>>    
>>>    &pcie0 {
>>> -	ep-gpios = <&gpio0 RK_PB4 GPIO_ACTIVE_HIGH>;
>>> +	ep-gpios = <&gpio0 RK_PB4 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
>>
>> So my biggest guess is we don't need this change at all.
>> gpio0b4 is used as gpio function, the problem you faced is that it
>> didn't set gpio0b4 as pull-up, because the defaut state is pull-down.
>>
>> Maybe the drive current of this IO is too weak, making it unable to
>> fully drive high in the pull-down state? If that's the case, can you see
>> a half-level signal on the oscilloscope?
>>
>>>    	num-lanes = <4>;
>>> -	pinctrl-0 = <&pcie_clkreqnb_cpm>;
>>> +	pinctrl-0 = <&pcie_clkreqnb_cpm>, <&pcie_perst>;
>>>    	pinctrl-names = "default";
>>>    	vpcie0v9-supply = <&vcca_0v9>;	/* VCC_0V9_S0 */
>>>    	vpcie1v8-supply = <&vcca_1v8>;	/* VCC_1V8_S0 */
>>> @@ -408,6 +408,10 @@ pcie {
>>>    		pcie_pwr: pcie-pwr {
>>>    			rockchip,pins = <4 RK_PD4 RK_FUNC_GPIO &pcfg_pull_up>;
>>>    		};
>>> +		pcie_perst: pcie-perst {
>>> +			rockchip,pins = <0 RK_PB4 RK_FUNC_GPIO &pcfg_pull_up>;
>>> +		};
>>> +
>>>    	};
>>>    
>>>    	pmic {
>>
> 
> Hi Shawn, glad to hear from you.
> 
> Perhaps the following change is better? It resolves the issue
> without the added complication of open drain. After you questioned
> if open drain is actually part of the spec, I remembered that
> GPIO_OPEN_DRAIN is actually (GPIO_SINGLE_ENDED | GPIO_LINE_OPEN_DRAIN)
> so I decided to test with just GPIO_SINGLE_ENDED and it works.


Does that work for you too?

&pcie0 {
	ep-gpios = <&gpio0 RK_PB4 GPIO_ACTIVE_HIGH>;
  	num-lanes = <4>;
-	pinctrl-0 = <&pcie_clkreqnb_cpm>;
+	pinctrl-0 = <&pcie_clkreqnb_cpm>, <&pcie_perst>;
  	pinctrl-names = "default";
  	vpcie0v9-supply = <&vcca_0v9>;	/* VCC_0V9_S0 */
  	vpcie1v8-supply = <&vcca_1v8>;	/* VCC_1V8_S0 */
@@ -408,6 +408,10 @@ pcie {
  		pcie_pwr: pcie-pwr {
  			rockchip,pins = <4 RK_PD4 RK_FUNC_GPIO &pcfg_pull_up>;
  		};
+		pcie_perst: pcie-perst {
+			rockchip,pins = <0 RK_PB4 RK_FUNC_GPIO &pcfg_pull_up>;
+		};
+
  	};

> 
> Thanks,
> Geraldo Nascimento
> 
> ---
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3399pro-vmarc-som.dtsi b/arch/arm64/boot/dts/rockchip/rk3399pro-vmarc-som.dtsi
> index aa70776e898a..b3d19dce539f 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3399pro-vmarc-som.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3399pro-vmarc-som.dtsi
> @@ -383,7 +383,7 @@ &pcie_phy {
>   };
>   
>   &pcie0 {
> -	ep-gpios = <&gpio0 RK_PB4 GPIO_ACTIVE_HIGH>;
> +	ep-gpios = <&gpio0 RK_PB4 (GPIO_ACTIVE_HIGH | GPIO_SINGLE_ENDED)>;
>   	num-lanes = <4>;
>   	pinctrl-0 = <&pcie_clkreqnb_cpm>;
>   	pinctrl-names = "default";
> 

Re: [PATCH] arm64: dts: rockchip: align bindings to PCIe spec

[Geraldo Nascimento]

On Wed, Nov 05, 2025 at 04:56:36PM +0800, Shawn Lin wrote:
> 在 2025/11/05 星期三 16:18, Geraldo Nascimento 写道:
> > 
> > Hi Shawn, glad to hear from you.
> > 
> > Perhaps the following change is better? It resolves the issue
> > without the added complication of open drain. After you questioned
> > if open drain is actually part of the spec, I remembered that
> > GPIO_OPEN_DRAIN is actually (GPIO_SINGLE_ENDED | GPIO_LINE_OPEN_DRAIN)
> > so I decided to test with just GPIO_SINGLE_ENDED and it works.
> 
> 
> Does that work for you too?
> 
> &pcie0 {
> 	ep-gpios = <&gpio0 RK_PB4 GPIO_ACTIVE_HIGH>;
>   	num-lanes = <4>;
> -	pinctrl-0 = <&pcie_clkreqnb_cpm>;
> +	pinctrl-0 = <&pcie_clkreqnb_cpm>, <&pcie_perst>;
>   	pinctrl-names = "default";
>   	vpcie0v9-supply = <&vcca_0v9>;	/* VCC_0V9_S0 */
>   	vpcie1v8-supply = <&vcca_1v8>;	/* VCC_1V8_S0 */
> @@ -408,6 +408,10 @@ pcie {
>   		pcie_pwr: pcie-pwr {
>   			rockchip,pins = <4 RK_PD4 RK_FUNC_GPIO &pcfg_pull_up>;
>   		};
> +		pcie_perst: pcie-perst {
> +			rockchip,pins = <0 RK_PB4 RK_FUNC_GPIO &pcfg_pull_up>;
> +		};
> +
>   	};

Hi Shawn,

No, that does not work.

I believe the pull-up mux became needed because I was forcing open drain
on PERST#.

Thanks,
Geraldo Nascimento

Re: [PATCH] arm64: dts: rockchip: align bindings to PCIe spec

[Geraldo Nascimento]

On Wed, Nov 05, 2025 at 04:56:36PM +0800, Shawn Lin wrote:
> 在 2025/11/05 星期三 16:18, Geraldo Nascimento 写道:
> > Hi Shawn, glad to hear from you.
> > 
> > Perhaps the following change is better? It resolves the issue
> > without the added complication of open drain. After you questioned
> > if open drain is actually part of the spec, I remembered that
> > GPIO_OPEN_DRAIN is actually (GPIO_SINGLE_ENDED | GPIO_LINE_OPEN_DRAIN)
> > so I decided to test with just GPIO_SINGLE_ENDED and it works.

Shawn,

I quote from the PCIe Mini Card Electromechanical Specification Rev 1.2

"3.4.1. Logic Signal Requirements

The 3.3V card logic levels for single-ended digital signals (WAKE#,
CLKREQ#, PERST#, and W_DISABLE#) are given in Table 3-7. [...]"

So while you are correct that PERST# is most definitely not Open Drain,
there's evidence on the spec that defines this signal as Single-Ended.

Thanks,
Geraldo Nascimento

Re: [PATCH] arm64: dts: rockchip: align bindings to PCIe spec

[Shawn Lin]

+ Ye Zhang

在 2025/11/07 星期五 10:43, Geraldo Nascimento 写道:
> On Wed, Nov 05, 2025 at 04:56:36PM +0800, Shawn Lin wrote:
>> 在 2025/11/05 星期三 16:18, Geraldo Nascimento 写道:
>>> Hi Shawn, glad to hear from you.
>>>
>>> Perhaps the following change is better? It resolves the issue
>>> without the added complication of open drain. After you questioned
>>> if open drain is actually part of the spec, I remembered that
>>> GPIO_OPEN_DRAIN is actually (GPIO_SINGLE_ENDED | GPIO_LINE_OPEN_DRAIN)
>>> so I decided to test with just GPIO_SINGLE_ENDED and it works.
> 
> Shawn,
> 
> I quote from the PCIe Mini Card Electromechanical Specification Rev 1.2
> 
> "3.4.1. Logic Signal Requirements
> 
> The 3.3V card logic levels for single-ended digital signals (WAKE#,
> CLKREQ#, PERST#, and W_DISABLE#) are given in Table 3-7. [...]"
> 
> So while you are correct that PERST# is most definitely not Open Drain,
> there's evidence on the spec that defines this signal as Single-Ended.
> 

This's true. But I couldn't find any user in dts using either
GPIO_SINGLE_ENDED or GPIO_OPEN_DRAIN for PCIe PERST#. I'm curious
how these two flags affect actual behavior of chips. Ye, could you
please help check it?

> Thanks,
> Geraldo Nascimento
> 

Re: [PATCH] arm64: dts: rockchip: align bindings to PCIe spec

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1911 bytes --]

Hi,

On Fri, Nov 07, 2025 at 11:01:04AM +0800, Shawn Lin wrote:
> + Ye Zhang
> 
> 在 2025/11/07 星期五 10:43, Geraldo Nascimento 写道:
> > On Wed, Nov 05, 2025 at 04:56:36PM +0800, Shawn Lin wrote:
> > > 在 2025/11/05 星期三 16:18, Geraldo Nascimento 写道:
> > > > Hi Shawn, glad to hear from you.
> > > > 
> > > > Perhaps the following change is better? It resolves the issue
> > > > without the added complication of open drain. After you questioned
> > > > if open drain is actually part of the spec, I remembered that
> > > > GPIO_OPEN_DRAIN is actually (GPIO_SINGLE_ENDED | GPIO_LINE_OPEN_DRAIN)
> > > > so I decided to test with just GPIO_SINGLE_ENDED and it works.
> > 
> > Shawn,
> > 
> > I quote from the PCIe Mini Card Electromechanical Specification Rev 1.2
> > 
> > "3.4.1. Logic Signal Requirements
> > 
> > The 3.3V card logic levels for single-ended digital signals (WAKE#,
> > CLKREQ#, PERST#, and W_DISABLE#) are given in Table 3-7. [...]"
> > 
> > So while you are correct that PERST# is most definitely not Open Drain,
> > there's evidence on the spec that defines this signal as Single-Ended.
> > 
> 
> This's true. But I couldn't find any user in dts using either
> GPIO_SINGLE_ENDED or GPIO_OPEN_DRAIN for PCIe PERST#. I'm curious
> how these two flags affect actual behavior of chips. Ye, could you
> please help check it?

FWIW I assume single-ended in the spec means it's not differential
like all the highspeed signals on the PCIe connection. This says
nothing about open-drain, open-source or push-pull being used. The
kernel on the other hand has a very specific understanding of
GPIO_SINGLE_ENDED:

	if (flags & OF_GPIO_SINGLE_ENDED) {
		if (flags & OF_GPIO_OPEN_DRAIN)
			lflags |= GPIO_OPEN_DRAIN;
		else
			lflags |= GPIO_OPEN_SOURCE;
	}

I.e. it is the same as configuring open-source ;)

Greetings,

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] arm64: dts: rockchip: align bindings to PCIe spec

[Geraldo Nascimento]

On Sat, Nov 08, 2025 at 11:12:54PM +0100, Sebastian Reichel wrote:
> Hi,

Hi Sebastian,

> 
> On Fri, Nov 07, 2025 at 11:01:04AM +0800, Shawn Lin wrote:
> > + Ye Zhang
> > 
> > 在 2025/11/07 星期五 10:43, Geraldo Nascimento 写道:
> > > On Wed, Nov 05, 2025 at 04:56:36PM +0800, Shawn Lin wrote:
> > > > 在 2025/11/05 星期三 16:18, Geraldo Nascimento 写道:
> > > > > Hi Shawn, glad to hear from you.
> > > > > 
> > > > > Perhaps the following change is better? It resolves the issue
> > > > > without the added complication of open drain. After you questioned
> > > > > if open drain is actually part of the spec, I remembered that
> > > > > GPIO_OPEN_DRAIN is actually (GPIO_SINGLE_ENDED | GPIO_LINE_OPEN_DRAIN)
> > > > > so I decided to test with just GPIO_SINGLE_ENDED and it works.
> > > 
> > > Shawn,
> > > 
> > > I quote from the PCIe Mini Card Electromechanical Specification Rev 1.2
> > > 
> > > "3.4.1. Logic Signal Requirements
> > > 
> > > The 3.3V card logic levels for single-ended digital signals (WAKE#,
> > > CLKREQ#, PERST#, and W_DISABLE#) are given in Table 3-7. [...]"
> > > 
> > > So while you are correct that PERST# is most definitely not Open Drain,
> > > there's evidence on the spec that defines this signal as Single-Ended.
> > > 
> > 
> > This's true. But I couldn't find any user in dts using either
> > GPIO_SINGLE_ENDED or GPIO_OPEN_DRAIN for PCIe PERST#. I'm curious
> > how these two flags affect actual behavior of chips. Ye, could you
> > please help check it?
> 
> FWIW I assume single-ended in the spec means it's not differential
> like all the highspeed signals on the PCIe connection. This says
> nothing about open-drain, open-source or push-pull being used. The

yes, I agree. It was an oversight on my part to assume open-drain on
PERST# was part of the spec just because many cores implement it that
way. Kudos to Shawn for correcting me.

> kernel on the other hand has a very specific understanding of
> GPIO_SINGLE_ENDED:
> 
> 	if (flags & OF_GPIO_SINGLE_ENDED) {
> 		if (flags & OF_GPIO_OPEN_DRAIN)
> 			lflags |= GPIO_OPEN_DRAIN;
> 		else
> 			lflags |= GPIO_OPEN_SOURCE;
> 	}
> 
> I.e. it is the same as configuring open-source ;)

Yup, I had noticed that. This works because the reset value of PMU GRF
register PMUGRF_GPIO0B_P sets the relevant PERST# GPIO (GPIO0-12) on my
board to pull-down, which can work with Open Source/Emitter. If we set
the GPIO to Open Drain/Collector we must on the other hand set that pin
to pull-up. Either way it works.

I've been investigating why that GPIO isn't properly working as
Push-Pull for my board (Rock PI N10) but so far I'm clueless.

Thank you,
Geraldo Nascimento

> 
> Greetings,
> 
> -- Sebastian

Re: [PATCH] arm64: dts: rockchip: align bindings to PCIe spec

[Geraldo Nascimento]

On Fri, Nov 07, 2025 at 11:01:04AM +0800, Shawn Lin wrote:
> + Ye Zhang
> 
> 在 2025/11/07 星期五 10:43, Geraldo Nascimento 写道:
> > On Wed, Nov 05, 2025 at 04:56:36PM +0800, Shawn Lin wrote:
> >> 在 2025/11/05 星期三 16:18, Geraldo Nascimento 写道:
> >>> Hi Shawn, glad to hear from you.
> >>>
> >>> Perhaps the following change is better? It resolves the issue
> >>> without the added complication of open drain. After you questioned
> >>> if open drain is actually part of the spec, I remembered that
> >>> GPIO_OPEN_DRAIN is actually (GPIO_SINGLE_ENDED | GPIO_LINE_OPEN_DRAIN)
> >>> so I decided to test with just GPIO_SINGLE_ENDED and it works.
> > 
> > Shawn,
> > 
> > I quote from the PCIe Mini Card Electromechanical Specification Rev 1.2
> > 
> > "3.4.1. Logic Signal Requirements
> > 
> > The 3.3V card logic levels for single-ended digital signals (WAKE#,
> > CLKREQ#, PERST#, and W_DISABLE#) are given in Table 3-7. [...]"
> > 
> > So while you are correct that PERST# is most definitely not Open Drain,
> > there's evidence on the spec that defines this signal as Single-Ended.
> > 
> 
> This's true. But I couldn't find any user in dts using either
> GPIO_SINGLE_ENDED or GPIO_OPEN_DRAIN for PCIe PERST#. I'm curious
> how these two flags affect actual behavior of chips. Ye, could you
> please help check it?
>

While I haven't heard from Ye Zhang still your comment instigated
me to dig deeper, thank you Shawn Lin. What I discovered I believe
is a bug in the Rockchip driver for the GPIO subsystem. Look:

diff --git a/drivers/gpio/gpio-rockchip.c b/drivers/gpio/gpio-rockchip.c
index 47174eb3ba76..5387c78ea11c 100644
--- a/drivers/gpio/gpio-rockchip.c
+++ b/drivers/gpio/gpio-rockchip.c
@@ -272,9 +272,10 @@ static int rockchip_gpio_direction_input(struct gpio_chip *gc,
 static int rockchip_gpio_direction_output(struct gpio_chip *gc,
 					  unsigned int offset, int value)
 {
-	rockchip_gpio_set(gc, offset, value);
 
-	return rockchip_gpio_set_direction(gc, offset, false);
+	rockchip_gpio_set_direction(gc, offset, false);
+
+	return rockchip_gpio_set(gc, offset, value);
 }
 
 /*

It seems to me the current logic is inverted, i.e. GPIO Port A Data
Register can't be successfully written if direction output is not set
yet.

I have to double-check with printk() but from what I see here it may
be very possible that first call to gpiod_get_index() will not set
proper value and only subsequent calls made to gpiod set_value()
will begin to set value.

For what it is worth, with the diff the workaround to set as open
source/emitter with pulldown or set open drain with pullup no longer
works, i.e. PCIe initial link training fails.

The workaround to drop TPVPERL still works, i.e. PCIe initial link
training proceeds, system operational.

Thanks,
Geraldo Nascimento

> > Thanks,
> > Geraldo Nascimento
> > 
> 

Re: [PATCH] arm64: dts: rockchip: align bindings to PCIe spec

[Geraldo Nascimento]

On Tue, Nov 11, 2025 at 03:47:04PM +0800, 张烨 wrote:
> Hi Geraldo,
> 
> In standard GPIO operations, the typical practice is to set the output level first before configuring the direction as output. This approach helps avoid outputting an uncertain voltage level at the instant when the direction switches from input to output.

Thanks for the explanation Ye Zhang, it makes sense to me. It avoids the
pin to not be floating so to speak. I kept hammering at this problem, by
the way is PCIe PERST# side-band signal refusing to co-operate and
failing PCIe initial link-training.

You're not going to like this:

diff --git a/drivers/gpio/gpio-rockchip.c b/drivers/gpio/gpio-rockchip.c
index 47174eb3ba76..fea2c55992e8 100644
--- a/drivers/gpio/gpio-rockchip.c
+++ b/drivers/gpio/gpio-rockchip.c
@@ -183,11 +183,13 @@ static int rockchip_gpio_set(struct gpio_chip *gc, unsigned int offset,
 	struct rockchip_pin_bank *bank = gpiochip_get_data(gc);
 	unsigned long flags;
 
+	rockchip_gpio_set_direction(gc, offset, true);
+
 	raw_spin_lock_irqsave(&bank->slock, flags);
 	rockchip_gpio_writel_bit(bank, offset, value, bank->gpio_regs->port_dr);
 	raw_spin_unlock_irqrestore(&bank->slock, flags);
 
-	return 0;
+	return rockchip_gpio_set_direction(gc, offset, false);
 }
 
 static int rockchip_gpio_get(struct gpio_chip *gc, unsigned int offset)

By setting direction INPUT, then writing out, then setting OUTPUT again
miraculously it doesn't fail initial link training, with no other
changes that already have been rejected by PCI folks and Shawn Lin.

Everything works as expected. Is this an explainable behaviour by
Rockchip GPIO core?

The problem I am observing is that once I set PERST# it becomes
unsettable again. So that's why those open-drain/open-source hacks
worked (gpiolib will hack the pull polarity to INPUT).

Thanks,
Geraldo Nascimento

> 
> Additionally, for Rockchip's GPIO controller specifically, setting the level value should not be affected by the direction setting - the data register write should be effective regardless of whether the pin is configured as input or output.

Re: [PATCH] arm64: dts: rockchip: align bindings to PCIe spec

[Geraldo Nascimento]

On Wed, Nov 12, 2025 at 05:03:32AM -0300, Geraldo Nascimento wrote:
> On Tue, Nov 11, 2025 at 03:47:04PM +0800, 张烨 wrote:
> > Hi Geraldo,
> > 
> > In standard GPIO operations, the typical practice is to set the output level first before configuring the direction as output. This approach helps avoid outputting an uncertain voltage level at the instant when the direction switches from input to output.
> 
> Thanks for the explanation Ye Zhang, it makes sense to me. It avoids the
> pin to not be floating so to speak. I kept hammering at this problem, by
> the way is PCIe PERST# side-band signal refusing to co-operate and
> failing PCIe initial link-training.
> 
> You're not going to like this:
> 
> diff --git a/drivers/gpio/gpio-rockchip.c b/drivers/gpio/gpio-rockchip.c
> index 47174eb3ba76..fea2c55992e8 100644
> --- a/drivers/gpio/gpio-rockchip.c
> +++ b/drivers/gpio/gpio-rockchip.c
> @@ -183,11 +183,13 @@ static int rockchip_gpio_set(struct gpio_chip *gc, unsigned int offset,
>  	struct rockchip_pin_bank *bank = gpiochip_get_data(gc);
>  	unsigned long flags;
>  
> +	rockchip_gpio_set_direction(gc, offset, true);
> +
>  	raw_spin_lock_irqsave(&bank->slock, flags);
>  	rockchip_gpio_writel_bit(bank, offset, value, bank->gpio_regs->port_dr);
>  	raw_spin_unlock_irqrestore(&bank->slock, flags);
>  
> -	return 0;
> +	return rockchip_gpio_set_direction(gc, offset, false);
>  }
>  
>  static int rockchip_gpio_get(struct gpio_chip *gc, unsigned int offset)
> 
> By setting direction INPUT, then writing out, then setting OUTPUT again
> miraculously it doesn't fail initial link training, with no other
> changes that already have been rejected by PCI folks and Shawn Lin.

Hi Ye, Shawn,

Here's more contained workaround without resorting to clearing DDR to
INPUT for every GPIO:

diff --git a/drivers/pci/controller/pcie-rockchip-host.c b/drivers/pci/controller/pcie-rockchip-host.c
index ee1822ca01db..1d89131ec6ac 100644
--- a/drivers/pci/controller/pcie-rockchip-host.c
+++ b/drivers/pci/controller/pcie-rockchip-host.c
@@ -315,7 +315,8 @@ static int rockchip_pcie_host_init_port(struct rockchip_pcie *rockchip)
 			    PCIE_CLIENT_CONFIG);
 
 	msleep(PCIE_T_PVPERL_MS);
-	gpiod_set_value_cansleep(rockchip->perst_gpio, 1);
+	gpiod_direction_input(rockchip->perst_gpio);
+	gpiod_direction_output(rockchip->perst_gpio, 1);
 
 	msleep(PCIE_RESET_CONFIG_WAIT_MS);
 
This results in working PCIe for me, pass initial link training.

I think I need to provide more details:

GPIO in question is GPIO0-12 / GPIO0-PB4. On RK3399PRO VMARC schematic
it's called PCIE_PERST#_3.3V and it's in the PMUIO1 domain.

Without workaround I have about 6 milliseconds, from driver probe and
parsing of DT that sets initial value 0 for GPIO, to deassert PERST#
by setting it high. Which is why just removing msleep(PCIE_T_PVPERL_MS)
produced working PCIe for me.

After the ~6 ms it becomes necessary to hack direction to input then
setting direction to output to properly deassert PERST# and proceed
with initial link training - which is why, again, hacking PERST# as
either open-drain with pull-up or open-source with pull-down worked:
gpiolib hacks that by setting direction INPUT.

Thanks,
Geraldo Nascimento

Re: [PATCH] arm64: dts: rockchip: align bindings to PCIe spec

[Geraldo Nascimento]

On Wed, Nov 12, 2025 at 10:09:15PM -0300, Geraldo Nascimento wrote:
> Hi Ye, Shawn,
> 
> Here's more contained workaround without resorting to clearing DDR to
> INPUT for every GPIO:
> 
> diff --git a/drivers/pci/controller/pcie-rockchip-host.c b/drivers/pci/controller/pcie-rockchip-host.c
> index ee1822ca01db..1d89131ec6ac 100644
> --- a/drivers/pci/controller/pcie-rockchip-host.c
> +++ b/drivers/pci/controller/pcie-rockchip-host.c
> @@ -315,7 +315,8 @@ static int rockchip_pcie_host_init_port(struct rockchip_pcie *rockchip)
>  			    PCIE_CLIENT_CONFIG);
>  
>  	msleep(PCIE_T_PVPERL_MS);
> -	gpiod_set_value_cansleep(rockchip->perst_gpio, 1);
> +	gpiod_direction_input(rockchip->perst_gpio);
> +	gpiod_direction_output(rockchip->perst_gpio, 1);
>  
>  	msleep(PCIE_RESET_CONFIG_WAIT_MS);
>  
> This results in working PCIe for me, pass initial link training.

Sorry for the inconvenience of more mail, but I'm providing as much
detail as I can.

This hack has been confirmed to work in U-boot also.

diff --git a/drivers/pci/pcie_rockchip.c b/drivers/pci/pcie_rockchip.c
index 19f9e58a640..5702b607ee6 100644
--- a/drivers/pci/pcie_rockchip.c
+++ b/drivers/pci/pcie_rockchip.c
@@ -329,8 +329,10 @@ static int rockchip_pcie_init_port(struct udevice *dev)
 	writel(PCIE_CLIENT_LINK_TRAIN_ENABLE,
 	       priv->apb_base + PCIE_CLIENT_CONFIG);
 
-	if (dm_gpio_is_valid(&priv->ep_gpio))
-		dm_gpio_set_value(&priv->ep_gpio, 1);
+	if (dm_gpio_is_valid(&priv->ep_gpio)) {
+		dm_gpio_set_dir_flags(&priv->ep_gpio, (priv->ep_gpio.flags & ~GPIOD_IS_OUT) | GPIOD_IS_IN);
+		dm_gpio_set_dir_flags(&priv->ep_gpio, (priv->ep_gpio.flags & ~GPIOD_IS_IN) | GPIOD_IS_OUT | GPIOD_IS_OUT_ACTIVE);
+	}
 
 	ret = readl_poll_sleep_timeout
 			(priv->apb_base + PCIE_CLIENT_BASIC_STATUS1,

So my report suggests this is not specific to Linux and because same
workaround works in U-boot simplified driver model I suggest you check
from your side.

Previously PCIe link training timeout, not working. Now I'm very happy
with working PCIe in Linux and U-boot.

Thanks,
Geraldo Nascimento

Re: [PATCH] arm64: dts: rockchip: align bindings to PCIe spec

[Shawn Lin]

在 2025/11/14 星期五 12:41, Geraldo Nascimento 写道:
> On Wed, Nov 12, 2025 at 10:09:15PM -0300, Geraldo Nascimento wrote:
>> Hi Ye, Shawn,
>>
>> Here's more contained workaround without resorting to clearing DDR to
>> INPUT for every GPIO:
>>
>> diff --git a/drivers/pci/controller/pcie-rockchip-host.c b/drivers/pci/controller/pcie-rockchip-host.c
>> index ee1822ca01db..1d89131ec6ac 100644
>> --- a/drivers/pci/controller/pcie-rockchip-host.c
>> +++ b/drivers/pci/controller/pcie-rockchip-host.c
>> @@ -315,7 +315,8 @@ static int rockchip_pcie_host_init_port(struct rockchip_pcie *rockchip)
>>   			    PCIE_CLIENT_CONFIG);
>>   
>>   	msleep(PCIE_T_PVPERL_MS);
>> -	gpiod_set_value_cansleep(rockchip->perst_gpio, 1);
>> +	gpiod_direction_input(rockchip->perst_gpio);
>> +	gpiod_direction_output(rockchip->perst_gpio, 1);
>>   
>>   	msleep(PCIE_RESET_CONFIG_WAIT_MS);
>>   
>> This results in working PCIe for me, pass initial link training.
> 
> Sorry for the inconvenience of more mail, but I'm providing as much
> detail as I can.
> 

Don't worry, it's helpful, so I think Ye could have a look.
May I ask if the failure only happened to one specific board?

Another thing I noticed is about one commit:
114b06ee108c ("PCI: rockchip: Set Target Link Speed to 5.0 GT/s before 
retraining")

It said: "Rockchip controllers can support up to 5.0 GT/s link speed."
But we issued an errata long time ago to announced it doesn't, you could
also check the PCIe part of RK3399 datasheet:
https://opensource.rock-chips.com/images/d/d7/Rockchip_RK3399_Datasheet_V2.1-20200323.pdf

Also we set max-link-speed to ONE in rk3399-base.dtsi but seems another
patch slip in: 755fff528b1b ("arm64: dts: rockchip: add variables for 
pcie completion to helios64")

> This hack has been confirmed to work in U-boot also.
> 
> diff --git a/drivers/pci/pcie_rockchip.c b/drivers/pci/pcie_rockchip.c
> index 19f9e58a640..5702b607ee6 100644
> --- a/drivers/pci/pcie_rockchip.c
> +++ b/drivers/pci/pcie_rockchip.c
> @@ -329,8 +329,10 @@ static int rockchip_pcie_init_port(struct udevice *dev)
>   	writel(PCIE_CLIENT_LINK_TRAIN_ENABLE,
>   	       priv->apb_base + PCIE_CLIENT_CONFIG);
>   
> -	if (dm_gpio_is_valid(&priv->ep_gpio))
> -		dm_gpio_set_value(&priv->ep_gpio, 1);
> +	if (dm_gpio_is_valid(&priv->ep_gpio)) {
> +		dm_gpio_set_dir_flags(&priv->ep_gpio, (priv->ep_gpio.flags & ~GPIOD_IS_OUT) | GPIOD_IS_IN);
> +		dm_gpio_set_dir_flags(&priv->ep_gpio, (priv->ep_gpio.flags & ~GPIOD_IS_IN) | GPIOD_IS_OUT | GPIOD_IS_OUT_ACTIVE);
> +	}
>   
>   	ret = readl_poll_sleep_timeout
>   			(priv->apb_base + PCIE_CLIENT_BASIC_STATUS1,
> 
> So my report suggests this is not specific to Linux and because same
> workaround works in U-boot simplified driver model I suggest you check
> from your side.
> 
> Previously PCIe link training timeout, not working. Now I'm very happy
> with working PCIe in Linux and U-boot.
> 
> Thanks,
> Geraldo Nascimento
> 

Re: [PATCH] arm64: dts: rockchip: align bindings to PCIe spec

[Geraldo Nascimento]

On Fri, Nov 14, 2025 at 05:16:21PM +0800, Shawn Lin wrote:
> Don't worry, it's helpful, so I think Ye could have a look.
> May I ask if the failure only happened to one specific board?

Hi Shawn,

Yes, testing is restricted to my Radxa Rock Pi N10 board.

> 
> Another thing I noticed is about one commit:
> 114b06ee108c ("PCI: rockchip: Set Target Link Speed to 5.0 GT/s before 
> retraining")
> 
> It said: "Rockchip controllers can support up to 5.0 GT/s link speed."
> But we issued an errata long time ago to announced it doesn't, you could
> also check the PCIe part of RK3399 datasheet:
> https://opensource.rock-chips.com/images/d/d7/Rockchip_RK3399_Datasheet_V2.1-20200323.pdf

OK, I'm partly responsible for that as author of the commit in question.

First off let me say I do not intend to send any patches setting
max-link-speed to TWO for this platform.

I understand you issued an erratum, but are you absolutely sure about
that erratum? Because my testing shows otherwise:

---
With max-link-speed = <2>
pci 0000:01:00.0: 16.000 Gb/s available PCIe bandwidth, limited by 5.0 GT/s PCIe x4 link at 0000:00:00.0 (capable of 31.504 Gb/s with 8.0 GT/s PCIe x4 link)

/dev/nvme0n1:
 Timing cached reads:   3002 MB in  2.00 seconds = 1502.21 MB/sec
 Timing buffered disk reads: 2044 MB in  3.00 seconds = 680.79 MB/sec
---
With max-link-speed = <1>
pci 0000:01:00.0: 8.000 Gb/s available PCIe bandwidth, limited by 2.5 GT/s PCIe x4 link at 0000:00:00.0 (capable of 31.504 Gb/s with 8.0 GT/s PCIe x4 link)

/dev/nvme0n1:
 Timing cached reads:   2730 MB in  2.00 seconds = 1366.15 MB/sec
 Timing buffered disk reads: 2028 MB in  3.00 seconds = 675.71 MB/sec
---

As you can see, not only the kernel PCI driver recognizes 5.0 GT/s PCIe
link but there's even a marginal increase in cached reads as measured by
hdparm, the gains are of course limited by CPU performance.

> Also we set max-link-speed to ONE in rk3399-base.dtsi but seems another
> patch slip in: 755fff528b1b ("arm64: dts: rockchip: add variables for 
> pcie completion to helios64")

I can't speak for patches I haven't authored, but I believe you're
welcome to send a correction.

Thank you,
Geraldo Nascimento