Hi Anand,
On 2024-08-09 12:41, Anand Moon wrote:
> Hi Jonas
>
> On Fri, 9 Aug 2024 at 15:29, Jonas Karlman <jonas@xxxxxxxxx> wrote:
>>
>> Hi Anand,
>>
>> On 2024-07-29 14:37, Anand Moon wrote:
>>> Add missing pinctrl settings WAKE and CLKREQ for PCIe 3.0 x4, PCIe 3.0 x1
>>> and PCIe 2.1 x1 nodes. Each component of PCIe communication have the
>>> following control signals: PERST, WAKE, CLKREQ, and REFCLK.
>>> These signals work to generate high-speed signals and communicate with
>>> other PCIe devices. Used by root complex to endpoint depending on
>>> the power state.
>>>
>>> PERST# is referred to as a fundamental reset. PERST should be held
>>> low until all the power rails in the system and the reference clock
>>> are stable. A transition from low to high in this signal usually
>>> indicates the beginning of link initialization.
>>>
>>> WAKE# signal is an active-low signal that is used to return the PCIe
>>> interface to an active state when in a low-power state.
>>>
>>> CLKREQ# signal is also an active-low signal and is used to request the
>>> reference clock. L1 sub-states is providing a digital signal
>>> (CLKREQ#) for PHYs to use to wake up and resume normal operation.
>>>
>>> Signed-off-by: Anand Moon <linux.amoon@xxxxxxxxx>
>>> ---
>>> v5: Merged all 3 patch into single patch, reabse on master
>>> Fix the $subject and commit message.
>>> Drop the RK_FUNC_GPIO for WAKE and CLKREQ as these seignal are
>>> ment for was introduced to allow PCI Express devices to enter
>>> even deeper power savings states (“L1.1” and “L1.2”) while still
>>> appearing to legacy software to be in the “L1” state
>>> ---
>>> .../boot/dts/rockchip/rk3588-rock-5b.dts | 46 +++++++++++++------
>>> 1 file changed, 33 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
>>> index 966bbc582d89..a1e83546f1be 100644
>>> --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
>>> +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
>>> @@ -318,7 +318,7 @@ map2 {
>>>
>>> &pcie2x1l0 {
>>> pinctrl-names = "default";
>>> - pinctrl-0 = <&pcie2_0_rst>;
>>> + pinctrl-0 = <&pcie30x1_pins>;
>>> reset-gpios = <&gpio4 RK_PA5 GPIO_ACTIVE_HIGH>;
>>> vpcie3v3-supply = <&vcc3v3_pcie2x1l0>;
>>> status = "okay";
>>> @@ -326,7 +326,7 @@ &pcie2x1l0 {
>>>
>>> &pcie2x1l2 {
>>> pinctrl-names = "default";
>>> - pinctrl-0 = <&pcie2_2_rst>;
>>> + pinctrl-0 = <&pcie20x12_pins>;
>>> reset-gpios = <&gpio3 RK_PB0 GPIO_ACTIVE_HIGH>;
>>> vpcie3v3-supply = <&vcc3v3_pcie2x1l2>;
>>> status = "okay";
>>> @@ -338,7 +338,7 @@ &pcie30phy {
>>>
>>> &pcie3x4 {
>>> pinctrl-names = "default";
>>> - pinctrl-0 = <&pcie3_rst>;
>>> + pinctrl-0 = <&pcie30x4_pins>;
>>> reset-gpios = <&gpio4 RK_PB6 GPIO_ACTIVE_HIGH>;
>>> vpcie3v3-supply = <&vcc3v3_pcie30>;
>>> status = "okay";
>>> @@ -363,28 +363,48 @@ hp_detect: hp-detect {
>>> };
>>> };
>>>
>>> - pcie2 {
>>> - pcie2_0_rst: pcie2-0-rst {
>>> - rockchip,pins = <4 RK_PA5 RK_FUNC_GPIO &pcfg_pull_none>;
>>> + pcie20x1 {
>>> + pcie20x12_pins: pcie20x12-pins {
>>> + rockchip,pins =
>>> + /* PCIE20_1_2_CLKREQn_M1_L */
>>> + <3 RK_PC7 4 &pcfg_pull_up>,
>>> + /* PCIE_PERST_L */
>>> + <3 RK_PB0 RK_FUNC_GPIO &pcfg_pull_up>,
>>> + /* PCIE20_1_2_WAKEn_M1_L */
>>> + <3 RK_PD0 4 &pcfg_pull_up>;
>>
>> Some unanswered questions from v4:
>>
>> How come you use internal pull-up/down on these pins?
>
> As per the schematic radxa_rock5b_v13_sch.pdf [1] pin description
>
> GPIO3_B0_u pin for PCIE_PERST_L (pcfg_pull_up)
> GPIO3_D0_u pin for PCIE20_1_2_WAKEn_M1_L (pcfg_pull_up)
> GPIO4_A4_d pin for PCIE30x1_0_WAKEn_M1_L (pcfg_pull_down)
The highlighting of GPIO3_B0_u etc typically only refers to the default
value at SoC reset, and the pinctrl should describe how the pin should
be configured, not the default value (unless it happens to match).
The schematic tells us that the GPIO3_B0 pin is used for PERST# signal,
your description of this signal tells us that the PERST# should be held
low and a transition from low to high in this signal usually indicates
the beginning of link initialization, so stand to reason that an
internal or external pull-down may be used on a PERST# signal.
Similarly the WAKE# and CLKREQ# signal is an active-low signal, so
use of internal or external pull-up may be correct.
However the mixed use of bias-pull-up/down as pinconf for a pin used
for same signal on different pcie controllers does not make that much
sense to me.
pcie2x1l0 WAKE# use pcfg_pull_down (bias-pull-down)
pcie2x1l2 WAKE# use pcfg_pull_up (bias-pull-up)
pcie3x4 WAKE# use pcfg_pull_down (bias-pull-down)
And similar it is mixed for CLKREQ# and PERST# across the different pcie
controllers.
I am no expert in this area but this mixed bias pinconf of these signals
look strange to me.
>
> [1] https://dl.radxa.com/rock5/5b/docs/hw/radxa_rock5b_v13_sch.pdf
>
>> And why do they differ for each pcie node in this series?
>
> It also depends on how the pins are defined in the schematics.
> I have not made many changes in this series combined in a single patch.
The internal pull is different compared to how the default pcie pins is
configured from rk3588 pinctrl.dtsi, where they are all bias-disable.
And my wondering is why this deviate from the use of pcfg_pull_none from
those existing pcie pins pin groups?
Regards,
Jonas
>
>>
>> Regards,
>> Jonas
>
> Thanks
> -Anand
>>
>>> };
>>> + };
>>>
>>> + pcie30x1 {
>>> pcie2_0_vcc3v3_en: pcie2-0-vcc-en {
>>> rockchip,pins = <1 RK_PD2 RK_FUNC_GPIO &pcfg_pull_none>;
>>> };
>>>
>>> - pcie2_2_rst: pcie2-2-rst {
>>> - rockchip,pins = <3 RK_PB0 RK_FUNC_GPIO &pcfg_pull_none>;
>>> + pcie30x1_pins: pcie30x1-pins {
>>> + rockchip,pins =
>>> + /* PCIE30x1_0_CLKREQn_M1_L */
>>> + <4 RK_PA3 4 &pcfg_pull_down>,
>>> + /* PCIE30x1_0_PERSTn_M1_L */
>>> + <4 RK_PA5 RK_FUNC_GPIO &pcfg_pull_down>,
>>> + /* PCIE30x1_0_WAKEn_M1_L */
>>> + <4 RK_PA4 4 &pcfg_pull_down>;
>>> };
>>> };
>>>
>>> - pcie3 {
>>> - pcie3_rst: pcie3-rst {
>>> - rockchip,pins = <4 RK_PB6 RK_FUNC_GPIO &pcfg_pull_none>;
>>> - };
>>> -
>>> + pcie30x4 {
>>> pcie3_vcc3v3_en: pcie3-vcc3v3-en {
>>> rockchip,pins = <1 RK_PA4 RK_FUNC_GPIO &pcfg_pull_none>;
>>> };
>>> +
>>> + pcie30x4_pins: pcie30x4-pins {
>>> + rockchip,pins =
>>> + /* PCIE30X4_CLKREQn_M1_L */
>>> + <4 RK_PB4 4 &pcfg_pull_up>,
>>> + /* PCIE30X4_PERSTn_M1_L */
>>> + <4 RK_PB6 RK_FUNC_GPIO &pcfg_pull_down>,
>>> + /* PCIE30X4_WAKEn_M1_L */
>>> + <4 RK_PB5 4 &pcfg_pull_down>;
>>> + };
>>> };
>>>
>>> usb {
>>>
>>> base-commit: dc1c8034e31b14a2e5e212104ec508aec44ce1b9
>>
