phy: rockchip-snps-pcie3:phy: Configure clkreq_n and PowerDown for all lanes

[PATCH v1] phy: rockchip-snps-pcie3:phy: Configure clkreq_n and PowerDown for all lanes

Posted by Anand Moon 2 months, 1 week ago

During the rk3588_p3phy_init sequence, the driver now explicitly
configures each lane's CON0 register to ensure
- PIPE 4.3 Compliance: clkreq_n (bit 6) is forced low (asserted) to meet
  sideband signal requirements.
- Active Power State: PowerDown[3:0] (bits 11:8) is set to P0
  (Normal Operational State) to ensure the PHY is fully powered and ready
  for link training.

These changes ensure that all lanes are consistently transitioned from
reset into a known-good operational state, preventing undefined behavior
and ensuring the PHY is ready for high-speed data transmission.

Cc: Niklas Cassel <cassel@kernel.org>
Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
 .../phy/rockchip/phy-rockchip-snps-pcie3.c    | 28 +++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/drivers/phy/rockchip/phy-rockchip-snps-pcie3.c b/drivers/phy/rockchip/phy-rockchip-snps-pcie3.c
index 4e8ffd173096..f46e13e79a0e 100644
--- a/drivers/phy/rockchip/phy-rockchip-snps-pcie3.c
+++ b/drivers/phy/rockchip/phy-rockchip-snps-pcie3.c
@@ -7,6 +7,7 @@
 
 #include <linux/clk.h>
 #include <linux/delay.h>
+#include <linux/hw_bitfield.h>
 #include <linux/io.h>
 #include <linux/iopoll.h>
 #include <linux/kernel.h>
@@ -35,10 +36,14 @@
 #define RK3588_PCIE3PHY_GRF_CMN_CON0		0x0
 #define RK3588_PCIE3PHY_GRF_PHY0_STATUS1	0x904
 #define RK3588_PCIE3PHY_GRF_PHY1_STATUS1	0xa04
+#define RK3588_PCIE3PHY_GRF_PHY0_LN0_CON0	0x1000
 #define RK3588_PCIE3PHY_GRF_PHY0_LN0_CON1	0x1004
 #define RK3588_PCIE3PHY_GRF_PHY0_LN1_CON1	0x1104
+#define RK3588_PCIE3PHY_GRF_PHY0_LN1_CON0	0x1100
+#define RK3588_PCIE3PHY_GRF_PHY1_LN0_CON0	0x2000
 #define RK3588_PCIE3PHY_GRF_PHY1_LN0_CON1	0x2004
 #define RK3588_PCIE3PHY_GRF_PHY1_LN1_CON1	0x2104
+#define RK3588_PCIE3PHY_GRF_PHY1_LN1_CON0	0x2100
 #define RK3588_SRAM_INIT_DONE(reg)		(reg & BIT(0))
 
 #define RK3588_BIFURCATION_LANE_0_1		BIT(0)
@@ -49,6 +54,13 @@
 #define RK3588_PCIE1LN_SEL_EN			(GENMASK(1, 0) << 16)
 #define RK3588_PCIE30_PHY_MODE_EN		(GENMASK(2, 0) << 16)
 
+static const u32 rk3588_lane_con0[] = {
+	RK3588_PCIE3PHY_GRF_PHY0_LN0_CON0,
+	RK3588_PCIE3PHY_GRF_PHY0_LN1_CON0,
+	RK3588_PCIE3PHY_GRF_PHY1_LN0_CON0,
+	RK3588_PCIE3PHY_GRF_PHY1_LN1_CON0,
+};
+
 struct rockchip_p3phy_ops;
 
 struct rockchip_p3phy_priv {
@@ -142,7 +154,7 @@ static int rockchip_p3phy_rk3588_init(struct rockchip_p3phy_priv *priv)
 {
 	u32 reg = 0;
 	u8 mode = RK3588_LANE_AGGREGATION; /* default */
-	int ret;
+	int ret, i;
 
 	regmap_write(priv->phy_grf, RK3588_PCIE3PHY_GRF_PHY0_LN0_CON1,
 		     priv->rx_cmn_refclk_mode[0] ? RK3588_RX_CMN_REFCLK_MODE_EN :
@@ -161,7 +173,7 @@ static int rockchip_p3phy_rk3588_init(struct rockchip_p3phy_priv *priv)
 	regmap_write(priv->phy_grf, RK3588_PCIE3PHY_GRF_CMN_CON0, BIT(8) | BIT(24));
 
 	/* Set bifurcation if needed */
-	for (int i = 0; i < priv->num_lanes; i++) {
+	for (i = 0; i < priv->num_lanes; i++) {
 		if (priv->lanes[i] > 1)
 			mode &= ~RK3588_LANE_AGGREGATION;
 		if (priv->lanes[i] == 3)
@@ -174,6 +186,18 @@ static int rockchip_p3phy_rk3588_init(struct rockchip_p3phy_priv *priv)
 	regmap_write(priv->phy_grf, RK3588_PCIE3PHY_GRF_CMN_CON0,
 		     RK3588_PCIE30_PHY_MODE_EN | reg);
 
+	for (i = 0; i < priv->num_lanes && i < ARRAY_SIZE(rk3588_lane_con0); i++) {
+		u32 base = rk3588_lane_con0[i];
+
+		/* clkreq_n = 0 (asserted low for PIPE 4.3) */
+		regmap_write(priv->phy_grf, base,
+			     FIELD_PREP_WM16(BIT(6), 0));
+
+		/* PowerDown = P0 (0x0, fully active) */
+		regmap_write(priv->phy_grf, base,
+			     FIELD_PREP_WM16(GENMASK(11, 8), 0x0));
+	}
+
 	/* Set pcie1ln_sel in PHP_GRF_PCIESEL_CON */
 	if (!IS_ERR(priv->pipe_grf)) {
 		reg = mode & (RK3588_BIFURCATION_LANE_0_1 | RK3588_BIFURCATION_LANE_2_3);

base-commit: 7f87a5ea75f011d2c9bc8ac0167e5e2d1adb1594
-- 
2.50.1

Re: [PATCH v1] phy: rockchip-snps-pcie3:phy: Configure clkreq_n and PowerDown for all lanes

Posted by Shawn Lin 2 months, 1 week ago

Hi Anand

在 2026/04/09 星期四 12:49, Anand Moon 写道:
> During the rk3588_p3phy_init sequence, the driver now explicitly
> configures each lane's CON0 register to ensure
> - PIPE 4.3 Compliance: clkreq_n (bit 6) is forced low (asserted) to meet
>    sideband signal requirements.

clkreq_n is now force asserted via controller driver if supports_clkreq
is not set.

> - Active Power State: PowerDown[3:0] (bits 11:8) is set to P0
>    (Normal Operational State) to ensure the PHY is fully powered and ready
>    for link training.
> 

P0 is the nature state when linking up. I don't know why it should be P0
before we even don't know whether the device is present.

> These changes ensure that all lanes are consistently transitioned from
> reset into a known-good operational state, preventing undefined behavior
> and ensuring the PHY is ready for high-speed data transmission.
> 
> Cc: Niklas Cassel <cassel@kernel.org>
> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> ---
>   .../phy/rockchip/phy-rockchip-snps-pcie3.c    | 28 +++++++++++++++++--
>   1 file changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/phy/rockchip/phy-rockchip-snps-pcie3.c b/drivers/phy/rockchip/phy-rockchip-snps-pcie3.c
> index 4e8ffd173096..f46e13e79a0e 100644
> --- a/drivers/phy/rockchip/phy-rockchip-snps-pcie3.c
> +++ b/drivers/phy/rockchip/phy-rockchip-snps-pcie3.c
> @@ -7,6 +7,7 @@
>   
>   #include <linux/clk.h>
>   #include <linux/delay.h>
> +#include <linux/hw_bitfield.h>
>   #include <linux/io.h>
>   #include <linux/iopoll.h>
>   #include <linux/kernel.h>
> @@ -35,10 +36,14 @@
>   #define RK3588_PCIE3PHY_GRF_CMN_CON0		0x0
>   #define RK3588_PCIE3PHY_GRF_PHY0_STATUS1	0x904
>   #define RK3588_PCIE3PHY_GRF_PHY1_STATUS1	0xa04
> +#define RK3588_PCIE3PHY_GRF_PHY0_LN0_CON0	0x1000
>   #define RK3588_PCIE3PHY_GRF_PHY0_LN0_CON1	0x1004
>   #define RK3588_PCIE3PHY_GRF_PHY0_LN1_CON1	0x1104
> +#define RK3588_PCIE3PHY_GRF_PHY0_LN1_CON0	0x1100
> +#define RK3588_PCIE3PHY_GRF_PHY1_LN0_CON0	0x2000
>   #define RK3588_PCIE3PHY_GRF_PHY1_LN0_CON1	0x2004
>   #define RK3588_PCIE3PHY_GRF_PHY1_LN1_CON1	0x2104
> +#define RK3588_PCIE3PHY_GRF_PHY1_LN1_CON0	0x2100
>   #define RK3588_SRAM_INIT_DONE(reg)		(reg & BIT(0))
>   
>   #define RK3588_BIFURCATION_LANE_0_1		BIT(0)
> @@ -49,6 +54,13 @@
>   #define RK3588_PCIE1LN_SEL_EN			(GENMASK(1, 0) << 16)
>   #define RK3588_PCIE30_PHY_MODE_EN		(GENMASK(2, 0) << 16)
>   
> +static const u32 rk3588_lane_con0[] = {
> +	RK3588_PCIE3PHY_GRF_PHY0_LN0_CON0,
> +	RK3588_PCIE3PHY_GRF_PHY0_LN1_CON0,
> +	RK3588_PCIE3PHY_GRF_PHY1_LN0_CON0,
> +	RK3588_PCIE3PHY_GRF_PHY1_LN1_CON0,
> +};
> +
>   struct rockchip_p3phy_ops;
>   
>   struct rockchip_p3phy_priv {
> @@ -142,7 +154,7 @@ static int rockchip_p3phy_rk3588_init(struct rockchip_p3phy_priv *priv)
>   {
>   	u32 reg = 0;
>   	u8 mode = RK3588_LANE_AGGREGATION; /* default */
> -	int ret;
> +	int ret, i;
>   
>   	regmap_write(priv->phy_grf, RK3588_PCIE3PHY_GRF_PHY0_LN0_CON1,
>   		     priv->rx_cmn_refclk_mode[0] ? RK3588_RX_CMN_REFCLK_MODE_EN :
> @@ -161,7 +173,7 @@ static int rockchip_p3phy_rk3588_init(struct rockchip_p3phy_priv *priv)
>   	regmap_write(priv->phy_grf, RK3588_PCIE3PHY_GRF_CMN_CON0, BIT(8) | BIT(24));
>   
>   	/* Set bifurcation if needed */
> -	for (int i = 0; i < priv->num_lanes; i++) {
> +	for (i = 0; i < priv->num_lanes; i++) {
>   		if (priv->lanes[i] > 1)
>   			mode &= ~RK3588_LANE_AGGREGATION;
>   		if (priv->lanes[i] == 3)
> @@ -174,6 +186,18 @@ static int rockchip_p3phy_rk3588_init(struct rockchip_p3phy_priv *priv)
>   	regmap_write(priv->phy_grf, RK3588_PCIE3PHY_GRF_CMN_CON0,
>   		     RK3588_PCIE30_PHY_MODE_EN | reg);
>   
> +	for (i = 0; i < priv->num_lanes && i < ARRAY_SIZE(rk3588_lane_con0); i++) {
> +		u32 base = rk3588_lane_con0[i];
> +
> +		/* clkreq_n = 0 (asserted low for PIPE 4.3) */
> +		regmap_write(priv->phy_grf, base,
> +			     FIELD_PREP_WM16(BIT(6), 0));
> +
> +		/* PowerDown = P0 (0x0, fully active) */
> +		regmap_write(priv->phy_grf, base,
> +			     FIELD_PREP_WM16(GENMASK(11, 8), 0x0));
> +	}
> +
>   	/* Set pcie1ln_sel in PHP_GRF_PCIESEL_CON */
>   	if (!IS_ERR(priv->pipe_grf)) {
>   		reg = mode & (RK3588_BIFURCATION_LANE_0_1 | RK3588_BIFURCATION_LANE_2_3);
> 
> base-commit: 7f87a5ea75f011d2c9bc8ac0167e5e2d1adb1594

Re: [PATCH v1] phy: rockchip-snps-pcie3:phy: Configure clkreq_n and PowerDown for all lanes

Posted by Anand Moon 2 months, 1 week ago

Hi Shawn,

Thanks for your review comments

On Fri, 10 Apr 2026 at 06:16, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>
> Hi Anand
>
> 在 2026/04/09 星期四 12:49, Anand Moon 写道:
> > During the rk3588_p3phy_init sequence, the driver now explicitly
> > configures each lane's CON0 register to ensure
> > - PIPE 4.3 Compliance: clkreq_n (bit 6) is forced low (asserted) to meet
> >    sideband signal requirements.
>
> clkreq_n is now force asserted via controller driver if supports_clkreq
> is not set.
>
> > - Active Power State: PowerDown[3:0] (bits 11:8) is set to P0
> >    (Normal Operational State) to ensure the PHY is fully powered and ready
> >    for link training.
> >
>
> P0 is the nature state when linking up. I don't know why it should be P0
> before we even don't know whether the device is present.
>
Ok understood. This step resets the lanes to their default state for
initialization.
I’ll collect additional input and verify if any configurations are
still missing.

Thanks
-Anand

Re: [PATCH v1] phy: rockchip-snps-pcie3:phy: Configure clkreq_n and PowerDown for all lanes

Posted by Niklas Cassel 2 months, 1 week ago

+Shawn

Hello Anand,

On Thu, Apr 09, 2026 at 10:19:30AM +0530, Anand Moon wrote:
> During the rk3588_p3phy_init sequence, the driver now explicitly

Please use imperative mood, active voice.


> configures each lane's CON0 register to ensure
> - PIPE 4.3 Compliance: clkreq_n (bit 6) is forced low (asserted) to meet
>   sideband signal requirements.
> - Active Power State: PowerDown[3:0] (bits 11:8) is set to P0
>   (Normal Operational State) to ensure the PHY is fully powered and ready
>   for link training.
> 
> These changes ensure that all lanes are consistently transitioned from
> reset into a known-good operational state, preventing undefined behavior
> and ensuring the PHY is ready for high-speed data transmission.

First describe the problem, then describe how you fix it.


Kind regards,
Niklas

Re: [PATCH v1] phy: rockchip-snps-pcie3:phy: Configure clkreq_n and PowerDown for all lanes

Posted by Anand Moon 2 months, 1 week ago

Hi Niklas,

Thanks for your review comments.

On Thu, 9 Apr 2026 at 15:19, Niklas Cassel <cassel@kernel.org> wrote:
>
> +Shawn
>
> Hello Anand,
>
> On Thu, Apr 09, 2026 at 10:19:30AM +0530, Anand Moon wrote:
> > During the rk3588_p3phy_init sequence, the driver now explicitly
>
> Please use imperative mood, active voice.
>
>
> > configures each lane's CON0 register to ensure
> > - PIPE 4.3 Compliance: clkreq_n (bit 6) is forced low (asserted) to meet
> >   sideband signal requirements.
> > - Active Power State: PowerDown[3:0] (bits 11:8) is set to P0
> >   (Normal Operational State) to ensure the PHY is fully powered and ready
> >   for link training.
> >
> > These changes ensure that all lanes are consistently transitioned from
> > reset into a known-good operational state, preventing undefined behavior
> > and ensuring the PHY is ready for high-speed data transmission.
>
Ok, I will update this.I f
> First describe the problem, then describe how you fix it.
I was investigating the PCIE30X4_CLKREQn issue highlighted by Shawn Lin,
analyzing the RK3588 TRM clock request configurations (page 878)

[1] https://lore.kernel.org/all/77f0d3c2-649f-770d-1636-6fd52f3b5f5e@rock-chips.com/

Looking into the power management state on Intel’s PCI Express Power Management
documentation, which defines states such as P0, P0s, P1, P1.1, P1.2, and P2
Understanding this mapping is for interpreting the behavior of lane-specific
clock requests, so initialize this to the P0 state..

[2] https://www.intel.com/content/dam/www/public/us/en/documents/white-papers/phy-interface-pci-express-sata-usb30-architectures-3.1.pdf
>
>
> Kind regards,
> Niklas

Thanks
-Anand