RK3399: PCie Phy using new helper function

PATCH Applied Anand Moon 2 participants 9 messages Oct 12, 2024 – Dec 8, 2024

Subsystems (2)

Arm Rockchip Generic PHY framework

lore

Share & tools

lore

Anand Moon

author

2024-10-12 07:19 UTC #0 lore raw

v3 cover RK3399: PCie Phy using new helper function

Few clean of the phy change and Using guard notation makes the code more compact and error handling for mutex_lock/mutex_unlock.

Plz review te code changes, I tend to do silly mistake.

v1:
[1] https://lore.kernel.org/all/[email protected]/
v2: Fix some typo in the subjects.
[2] https://lore.kernel.org/all/[email protected]/

Thanks
-Anand

Anand Moon (6):
phy: rockchip-pcie: Simplify error handling with dev_err_probe()
phy: rockchip-pcie: Use devm_clk_get_enabled() helper
phy: rockchip-pcie: Use regmap_read_poll_timeout() for PCIe reference
clk PLL status
phy: rockchip-pcie: Refactor mutex handling in
rockchip_pcie_phy_power_off()
phy: rockchip-pcie: Refactor mutex handling in
rockchip_pcie_phy_power_on()
phy: rockchip-pcie: Use guard notation when acquiring mutex

 drivers/phy/rockchip/phy-rockchip-pcie.c | 148 +++++++----------------
 1 file changed, 47 insertions(+), 101 deletions(-)

base-commit: 09f6b0c8904bfaa1e0601bc102e1b6aa6de8c98f

Signature · 1 line

2.44.0

Anand Moon

author

2024-10-12 07:19 UTC #1 lore raw

PATCH v3 1/6 phy: rockchip-pcie: Simplify error handling with dev_err_probe()

Use the dev_err_probe() helper to simplify error handling during probe. This also handle scenario, when -EDEFER is returned and useless error is printed.

Signed-off-by Anand Moon <[email protected]>

---
v3: None
v3: None

---
 drivers/phy/rockchip/phy-rockchip-pcie.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

drivers/phy/rockchip/phy-rockchip-pcie.c +3 -6

index 51cc5ece0e63..51e636a1ce33 100644
@@ -371,12 +371,9 @@ static int rockchip_pcie_phy_probe(struct platform_device *pdev)
 	mutex_init(&rk_phy->pcie_mutex);
 
 	rk_phy->phy_rst = devm_reset_control_get(dev, "phy");
-	if (IS_ERR(rk_phy->phy_rst)) {
-		if (PTR_ERR(rk_phy->phy_rst) != -EPROBE_DEFER)
-			dev_err(dev,
-				"missing phy property for reset controller\n");
-		return PTR_ERR(rk_phy->phy_rst);
-	}
+	if (IS_ERR(rk_phy->phy_rst))
+		return dev_err_probe(&pdev->dev, PTR_ERR(rk_phy->phy_rst),
+				     "missing phy property for reset controller\n");
 
 	rk_phy->clk_pciephy_ref = devm_clk_get(dev, "refclk");
 	if (IS_ERR(rk_phy->clk_pciephy_ref)) {

Signature · 1 line

2.44.0

Anand Moon

author

2024-10-12 07:19 UTC #2 lore raw

PATCH v3 2/6 phy: rockchip-pcie: Use devm_clk_get_enabled() helper

In reply to Anand Moon

Use devm_clk_get_enabled() instead of devm_clk_get() to make the code cleaner and avoid calling clk_disable_unprepare(), as this is exactly what this function does. Use the dev_err_probe() helper to simplify error handling during probe.

Refactor the mutex handling in the rockchip_pcie_phy_init() function to improve code readability and maintainability. The goto statement has been removed, and the mutex_unlock call is now directly within the conditional block.

Return the result of reset_control_assert() function, with 0 indicating success and an error code indicating failure

Signed-off-by Anand Moon <[email protected]>

---
v3:
Dan Carpenter: Reported below warning.
smatch warnings:
drivers/phy/rockchip/phy-rockchip-pcie.c:278 rockchip_pcie_phy_init() warn: missing error code 'err' So refactor the mutex_lock/mutex_unlock and return the err code.
v2: Change the subject drop: Change to use/Use
v1: New patch in this series

---
 drivers/phy/rockchip/phy-rockchip-pcie.c | 34 +++++++-----------------
 1 file changed, 10 insertions(+), 24 deletions(-)

drivers/phy/rockchip/phy-rockchip-pcie.c +10 ~~-24~~

index 51e636a1ce33..b5b1b1a667b2 100644
@@ -274,30 +274,19 @@ static int rockchip_pcie_phy_init(struct phy *phy)
 
 	mutex_lock(&rk_phy->pcie_mutex);
 
-	if (rk_phy->init_cnt++)
-		goto err_out;
-
-	err = clk_prepare_enable(rk_phy->clk_pciephy_ref);
-	if (err) {
-		dev_err(&phy->dev, "Fail to enable pcie ref clock.\n");
-		goto err_refclk;
+	if (rk_phy->init_cnt++) {
+		mutex_unlock(&rk_phy->pcie_mutex);
+		return 0;
 	}
 
 	err = reset_control_assert(rk_phy->phy_rst);
 	if (err) {
 		dev_err(&phy->dev, "assert phy_rst err %d\n", err);
-		goto err_reset;
+		rk_phy->init_cnt--;
+		mutex_unlock(&rk_phy->pcie_mutex);
+		return err;
 	}
 
-err_out:
-	mutex_unlock(&rk_phy->pcie_mutex);
-	return 0;
-
-err_reset:
-
-	clk_disable_unprepare(rk_phy->clk_pciephy_ref);
-err_refclk:
-	rk_phy->init_cnt--;
 	mutex_unlock(&rk_phy->pcie_mutex);
 	return err;
 }

@@ -312,8 +301,6 @@ static int rockchip_pcie_phy_exit(struct phy *phy)
 	if (--rk_phy->init_cnt)
 		goto err_init_cnt;
 
-	clk_disable_unprepare(rk_phy->clk_pciephy_ref);
-
 err_init_cnt:
 	mutex_unlock(&rk_phy->pcie_mutex);
 	return 0;

@@ -375,11 +362,10 @@ static int rockchip_pcie_phy_probe(struct platform_device *pdev)
 		return dev_err_probe(&pdev->dev, PTR_ERR(rk_phy->phy_rst),
 				     "missing phy property for reset controller\n");
 
-	rk_phy->clk_pciephy_ref = devm_clk_get(dev, "refclk");
-	if (IS_ERR(rk_phy->clk_pciephy_ref)) {
-		dev_err(dev, "refclk not found.\n");
-		return PTR_ERR(rk_phy->clk_pciephy_ref);
-	}
+	rk_phy->clk_pciephy_ref = devm_clk_get_enabled(dev, "refclk");
+	if (IS_ERR(rk_phy->clk_pciephy_ref))
+		return dev_err_probe(&pdev->dev, PTR_ERR(rk_phy->clk_pciephy_ref),
+				     "failed to get phyclk\n");
 
 	/* parse #phy-cells to see if it's legacy PHY model */
 	if (of_property_read_u32(dev->of_node, "#phy-cells", &phy_num))

Signature · 1 line

2.44.0

Anand Moon

author

2024-10-12 07:19 UTC #3 lore raw

PATCH v3 3/6 phy: rockchip-pcie: Use regmap_read_poll_timeout() for PCIe reference clk PLL status

In reply to Anand Moon

Replace open-coded phy PCIe reference clk PLL status polling with regmap_read_poll_timeout API. This change simplifies the code without altering functionality.

Signed-off-by Anand Moon <[email protected]>

---
v3: None.
v2: Fix the subject, add the missing () in the function name,
Fix the typo reference
v1: None.

---
 drivers/phy/rockchip/phy-rockchip-pcie.c | 56 +++++++-----------------
 1 file changed, 15 insertions(+), 41 deletions(-)

drivers/phy/rockchip/phy-rockchip-pcie.c +15 ~~-41~~

index b5b1b1a667b2..e60f24b0b363 100644
@@ -162,7 +162,6 @@ static int rockchip_pcie_phy_power_on(struct phy *phy)
 	struct rockchip_pcie_phy *rk_phy = to_pcie_phy(inst);
 	int err = 0;
 	u32 status;
-	unsigned long timeout;
 
 	mutex_lock(&rk_phy->pcie_mutex);

@@ -191,21 +190,11 @@ static int rockchip_pcie_phy_power_on(struct phy *phy)
 	 * so we make it large enough here. And we use loop-break
 	 * method which should not be harmful.
 	 */
-	timeout = jiffies + msecs_to_jiffies(1000);
-
-	err = -EINVAL;
-	while (time_before(jiffies, timeout)) {
-		regmap_read(rk_phy->reg_base,
-			    rk_phy->phy_data->pcie_status,
-			    &status);
-		if (status & PHY_PLL_LOCKED) {
-			dev_dbg(&phy->dev, "pll locked!\n");
-			err = 0;
-			break;
-		}
-		msleep(20);
-	}
-
+	err = regmap_read_poll_timeout(rk_phy->reg_base,
+				       rk_phy->phy_data->pcie_status,
+				       status,
+				       status & PHY_PLL_LOCKED,
+				       200, 100000);
 	if (err) {
 		dev_err(&phy->dev, "pll lock timeout!\n");
 		goto err_pll_lock;

@@ -214,19 +203,11 @@ static int rockchip_pcie_phy_power_on(struct phy *phy)
 	phy_wr_cfg(rk_phy, PHY_CFG_CLK_TEST, PHY_CFG_SEPE_RATE);
 	phy_wr_cfg(rk_phy, PHY_CFG_CLK_SCC, PHY_CFG_PLL_100M);
 
-	err = -ETIMEDOUT;
-	while (time_before(jiffies, timeout)) {
-		regmap_read(rk_phy->reg_base,
-			    rk_phy->phy_data->pcie_status,
-			    &status);
-		if (!(status & PHY_PLL_OUTPUT)) {
-			dev_dbg(&phy->dev, "pll output enable done!\n");
-			err = 0;
-			break;
-		}
-		msleep(20);
-	}
-
+	err = regmap_read_poll_timeout(rk_phy->reg_base,
+				       rk_phy->phy_data->pcie_status,
+				       status,
+				       !(status & PHY_PLL_OUTPUT),
+				       200, 100000);
 	if (err) {
 		dev_err(&phy->dev, "pll output enable timeout!\n");
 		goto err_pll_lock;

@@ -236,19 +217,12 @@ static int rockchip_pcie_phy_power_on(struct phy *phy)
 		     HIWORD_UPDATE(PHY_CFG_PLL_LOCK,
 				   PHY_CFG_ADDR_MASK,
 				   PHY_CFG_ADDR_SHIFT));
-	err = -EINVAL;
-	while (time_before(jiffies, timeout)) {
-		regmap_read(rk_phy->reg_base,
-			    rk_phy->phy_data->pcie_status,
-			    &status);
-		if (status & PHY_PLL_LOCKED) {
-			dev_dbg(&phy->dev, "pll relocked!\n");
-			err = 0;
-			break;
-		}
-		msleep(20);
-	}
 
+	err = regmap_read_poll_timeout(rk_phy->reg_base,
+				       rk_phy->phy_data->pcie_status,
+				       status,
+				       status & PHY_PLL_LOCKED,
+				       200, 100000);
 	if (err) {
 		dev_err(&phy->dev, "pll relock timeout!\n");
 		goto err_pll_lock;

Signature · 1 line

2.44.0

Anand Moon

author

2024-10-12 07:19 UTC #4 lore raw

PATCH v3 4/6 phy: rockchip-pcie: Refactor mutex handling in rockchip_pcie_phy_power_off()

In reply to Anand Moon

Refactor the mutex handling in the rockchip_pcie_phy_power_off() function to improve code readability and maintainability. The goto statement has been removed, and the mutex_unlock call is now directly within the conditional block.

Return the result of reset_control_assert() function, with 0 indicating success and an error code indicating failure

Signed-off-by Anand Moon <[email protected]>

---
v3: New patch.

---
 drivers/phy/rockchip/phy-rockchip-pcie.c | 26 +++++++++++-------------
 1 file changed, 12 insertions(+), 14 deletions(-)

drivers/phy/rockchip/phy-rockchip-pcie.c +12 ~~-14~~

index e60f24b0b363..c84a3c209315 100644
@@ -132,26 +132,24 @@ static int rockchip_pcie_phy_power_off(struct phy *phy)
 				   PHY_LANE_IDLE_MASK,
 				   PHY_LANE_IDLE_A_SHIFT + inst->index));
 
-	if (--rk_phy->pwr_cnt)
-		goto err_out;
+	if (--rk_phy->pwr_cnt) {
+		mutex_unlock(&rk_phy->pcie_mutex);
+		return 0;
+	}
 
 	err = reset_control_assert(rk_phy->phy_rst);
 	if (err) {
 		dev_err(&phy->dev, "assert phy_rst err %d\n", err);
-		goto err_restore;
+		rk_phy->pwr_cnt++;
+		regmap_write(rk_phy->reg_base,
+			     rk_phy->phy_data->pcie_laneoff,
+			     HIWORD_UPDATE(!PHY_LANE_IDLE_OFF,
+					   PHY_LANE_IDLE_MASK,
+					   PHY_LANE_IDLE_A_SHIFT + inst->index));
+		mutex_unlock(&rk_phy->pcie_mutex);
+		return err;
 	}
 
-err_out:
-	mutex_unlock(&rk_phy->pcie_mutex);
-	return 0;
-
-err_restore:
-	rk_phy->pwr_cnt++;
-	regmap_write(rk_phy->reg_base,
-		     rk_phy->phy_data->pcie_laneoff,
-		     HIWORD_UPDATE(!PHY_LANE_IDLE_OFF,
-				   PHY_LANE_IDLE_MASK,
-				   PHY_LANE_IDLE_A_SHIFT + inst->index));
 	mutex_unlock(&rk_phy->pcie_mutex);
 	return err;
 }

Signature · 1 line

2.44.0

Anand Moon

author

2024-10-12 07:19 UTC #5 lore raw

PATCH v3 5/6 phy: rockchip-pcie: Refactor mutex handling in rockchip_pcie_phy_power_on()

In reply to Anand Moon

Refactor the mutex handling in the rockchip_pcie_phy_power_on() function to improve code readability and maintainability. The goto statement has been removed, and the mutex_unlock call is now directly within the conditional block.

Return the result of reset_control_deassert() or regmap_read_poll_timeout() function, with 0 indicating success and an error code indicating failure.

Signed-off-by Anand Moon <[email protected]>

---
v3: New patch.

---
 drivers/phy/rockchip/phy-rockchip-pcie.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

drivers/phy/rockchip/phy-rockchip-pcie.c +8 -6

index c84a3c209315..6dd014625226 100644
@@ -163,13 +163,17 @@ static int rockchip_pcie_phy_power_on(struct phy *phy)
 
 	mutex_lock(&rk_phy->pcie_mutex);
 
-	if (rk_phy->pwr_cnt++)
-		goto err_out;
+	if (rk_phy->pwr_cnt++) {
+		mutex_unlock(&rk_phy->pcie_mutex);
+		return 0;
+	}
 
 	err = reset_control_deassert(rk_phy->phy_rst);
 	if (err) {
 		dev_err(&phy->dev, "deassert phy_rst err %d\n", err);
-		goto err_pwr_cnt;
+		rk_phy->pwr_cnt--;
+		mutex_unlock(&rk_phy->pcie_mutex);
+		return err;
 	}
 
 	regmap_write(rk_phy->reg_base, rk_phy->phy_data->pcie_conf,

@@ -226,13 +230,11 @@ static int rockchip_pcie_phy_power_on(struct phy *phy)
 		goto err_pll_lock;
 	}
 
-err_out:
 	mutex_unlock(&rk_phy->pcie_mutex);
-	return 0;
+	return err;
 
 err_pll_lock:
 	reset_control_assert(rk_phy->phy_rst);
-err_pwr_cnt:
 	rk_phy->pwr_cnt--;
 	mutex_unlock(&rk_phy->pcie_mutex);
 	return err;

Signature · 1 line

2.44.0

Anand Moon

author

2024-10-12 07:20 UTC #6 lore raw

PATCH v3 6/6 phy: rockchip-pcie: Use guard notation when acquiring mutex

In reply to Anand Moon

Using guard notation makes the code more compact and error handling more robust by ensuring that mutexes are released in all code paths when control leaves critical section.

Signed-off-by Anand Moon <[email protected]>

---
v3: New patch

---
 drivers/phy/rockchip/phy-rockchip-pcie.c | 19 ++++---------------
 1 file changed, 4 insertions(+), 15 deletions(-)

drivers/phy/rockchip/phy-rockchip-pcie.c +4 ~~-15~~

index 6dd014625226..bd44af36c67a 100644
@@ -124,7 +124,7 @@ static int rockchip_pcie_phy_power_off(struct phy *phy)
 	struct rockchip_pcie_phy *rk_phy = to_pcie_phy(inst);
 	int err = 0;
 
-	mutex_lock(&rk_phy->pcie_mutex);
+	guard(mutex)(&rk_phy->pcie_mutex);
 
 	regmap_write(rk_phy->reg_base,
 		     rk_phy->phy_data->pcie_laneoff,

@@ -133,7 +133,6 @@ static int rockchip_pcie_phy_power_off(struct phy *phy)
 				   PHY_LANE_IDLE_A_SHIFT + inst->index));
 
 	if (--rk_phy->pwr_cnt) {
-		mutex_unlock(&rk_phy->pcie_mutex);
 		return 0;
 	}

@@ -146,11 +145,9 @@ static int rockchip_pcie_phy_power_off(struct phy *phy)
 			     HIWORD_UPDATE(!PHY_LANE_IDLE_OFF,
 					   PHY_LANE_IDLE_MASK,
 					   PHY_LANE_IDLE_A_SHIFT + inst->index));
-		mutex_unlock(&rk_phy->pcie_mutex);
 		return err;
 	}
 
-	mutex_unlock(&rk_phy->pcie_mutex);
 	return err;
 }

@@ -161,10 +158,9 @@ static int rockchip_pcie_phy_power_on(struct phy *phy)
 	int err = 0;
 	u32 status;
 
-	mutex_lock(&rk_phy->pcie_mutex);
+	guard(mutex)(&rk_phy->pcie_mutex);
 
 	if (rk_phy->pwr_cnt++) {
-		mutex_unlock(&rk_phy->pcie_mutex);
 		return 0;
 	}

@@ -172,7 +168,6 @@ static int rockchip_pcie_phy_power_on(struct phy *phy)
 	if (err) {
 		dev_err(&phy->dev, "deassert phy_rst err %d\n", err);
 		rk_phy->pwr_cnt--;
-		mutex_unlock(&rk_phy->pcie_mutex);
 		return err;
 	}

@@ -230,13 +225,11 @@ static int rockchip_pcie_phy_power_on(struct phy *phy)
 		goto err_pll_lock;
 	}
 
-	mutex_unlock(&rk_phy->pcie_mutex);
 	return err;
 
 err_pll_lock:
 	reset_control_assert(rk_phy->phy_rst);
 	rk_phy->pwr_cnt--;
-	mutex_unlock(&rk_phy->pcie_mutex);
 	return err;
 }

@@ -246,10 +239,9 @@ static int rockchip_pcie_phy_init(struct phy *phy)
 	struct rockchip_pcie_phy *rk_phy = to_pcie_phy(inst);
 	int err = 0;
 
-	mutex_lock(&rk_phy->pcie_mutex);
+	guard(mutex)(&rk_phy->pcie_mutex);
 
 	if (rk_phy->init_cnt++) {
-		mutex_unlock(&rk_phy->pcie_mutex);
 		return 0;
 	}

@@ -257,11 +249,9 @@ static int rockchip_pcie_phy_init(struct phy *phy)
 	if (err) {
 		dev_err(&phy->dev, "assert phy_rst err %d\n", err);
 		rk_phy->init_cnt--;
-		mutex_unlock(&rk_phy->pcie_mutex);
 		return err;
 	}
 
-	mutex_unlock(&rk_phy->pcie_mutex);
 	return err;
 }

@@ -270,13 +260,12 @@ static int rockchip_pcie_phy_exit(struct phy *phy)
 	struct phy_pcie_instance *inst = phy_get_drvdata(phy);
 	struct rockchip_pcie_phy *rk_phy = to_pcie_phy(inst);
 
-	mutex_lock(&rk_phy->pcie_mutex);
+	guard(mutex)(&rk_phy->pcie_mutex);
 
 	if (--rk_phy->init_cnt)
 		goto err_init_cnt;
 
 err_init_cnt:
-	mutex_unlock(&rk_phy->pcie_mutex);
 	return 0;
 }

Signature · 1 line

2.44.0

Anand Moon

author

2024-12-02 16:01 UTC #7 lore raw

In reply to Anand Moon

Hi All,

Quoted 11 lines · Anand Moon

Few clean of the phy change and Using guard notation makes the code more compact and error handling for mutex_lock/mutex_unlock.
Plz review te code changes, I tend to do silly mistake.
v1:
[1] https://lore.kernel.org/all/[email protected]/
v2: Fix some typo in the subjects.
[2] https://lore.kernel.org/all/[email protected]/

Do you have any device comments on these code changes?

Quoted 3 lines

Thanks
-Anand

Thank
-Anand

Quoted 19 lines

Anand Moon (6):
phy: rockchip-pcie: Simplify error handling with dev_err_probe()
phy: rockchip-pcie: Use devm_clk_get_enabled() helper
phy: rockchip-pcie: Use regmap_read_poll_timeout() for PCIe reference
clk PLL status
phy: rockchip-pcie: Refactor mutex handling in
rockchip_pcie_phy_power_off()
phy: rockchip-pcie: Refactor mutex handling in
rockchip_pcie_phy_power_on()
phy: rockchip-pcie: Use guard notation when acquiring mutex
 drivers/phy/rockchip/phy-rockchip-pcie.c | 148 +++++++----------------
 1 file changed, 47 insertions(+), 101 deletions(-)
base-commit: 09f6b0c8904bfaa1e0601bc102e1b6aa6de8c98f --
2.44.0

Vinod Koul

maintainer Applied

2024-12-08 17:02 UTC #8 lore raw

In reply to Anand Moon

Quoted 11 lines · Anand Moon

Few clean of the phy change and Using guard notation makes the code more compact and error handling for mutex_lock/mutex_unlock.
Plz review te code changes, I tend to do silly mistake.
v1:
[1] https://lore.kernel.org/all/[email protected]/
v2: Fix some typo in the subjects.
[2] https://lore.kernel.org/all/[email protected]/
[...]

Applied, thanks!

[1/6] phy: rockchip-pcie: Simplify error handling with dev_err_probe()

      commit: 84de918083d09500d93d991d8989addfaae1611e
[2/6] phy: rockchip-pcie: Use devm_clk_get_enabled() helper
      commit: e96397db55e5fbe290ff1462ddf6c24ed94eb7df
[3/6] phy: rockchip-pcie: Use regmap_read_poll_timeout() for PCIe reference clk PLL status
      commit: cb0ba26ad09398d3d0d10f518af4ccae69c8b64e
[4/6] phy: rockchip-pcie: Refactor mutex handling in rockchip_pcie_phy_power_off()
      commit: bb70d1aae565fd52e6a50e643d1ad6e7d419c2a5
[5/6] phy: rockchip-pcie: Refactor mutex handling in rockchip_pcie_phy_power_on()
      commit: 96522eeb8735449957272e9c6f8ea3b72dcbdeb8
[6/6] phy: rockchip-pcie: Use guard notation when acquiring mutex
      commit: c90a7a685a5d90228cedf7a5068f50b3da23ddfc

Best regards,

Signature · 1 line

~Vinod