Linux-PHY Archive on lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] phy: samsung: usbdrd: propagate GS101 PIPE3 lock failures
@ 2026-06-23 13:53 Pengpeng Hou
  2026-06-23 14:06 ` sashiko-bot
  0 siblings, 1 reply; 2+ messages in thread
From: Pengpeng Hou @ 2026-06-23 13:53 UTC (permalink / raw)
  To: Vinod Koul, Neil Armstrong, Krzysztof Kozlowski, Alim Akhtar,
	Pritam Manohar Sutar, Sam Protsenko, Johan Hovold,
	André Draszik, Peter Griffin
  Cc: Pengpeng Hou, linux-phy, linux-arm-kernel, linux-samsung-soc,
	linux-kernel

The GS101 PIPE3 initialization checks the PMA PLL lock and then checks
the CDR lock. The PLL helper returns an error, but the GS101 PIPE3 init
path dropped that result. The CDR helper only logged a timeout. PHY
initialization could therefore still return success after either the PLL
or the selected receive lane had not locked.

Make the in-file SoC-specific phy_init hook return an int so helpers
with real failure results can propagate them through the existing PHY
init paths. Keep the register-only helpers returning success and use the
new return path to report the GS101 CDR wait failure. Other SoC helper
paths keep their previous success behavior.

Fixes: 32267c29bc7d ("phy: exynos5-usbdrd: support Exynos USBDRD 3.1 combo phy (HS & SS)")
Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
 drivers/phy/samsung/phy-exynos5-usbdrd.c | 78 ++++++++++++++++-------
 1 file changed, 55 insertions(+), 23 deletions(-)

diff --git a/drivers/phy/samsung/phy-exynos5-usbdrd.c b/drivers/phy/samsung/phy-exynos5-usbdrd.c
index 8711a3b62..5a8e0ce77 100644
--- a/drivers/phy/samsung/phy-exynos5-usbdrd.c
+++ b/drivers/phy/samsung/phy-exynos5-usbdrd.c
@@ -471,7 +471,7 @@ struct exynos5_usbdrd_phy;
 struct exynos5_usbdrd_phy_config {
 	u32 id;
 	void (*phy_isol)(struct phy_usb_instance *inst, bool isolate);
-	void (*phy_init)(struct exynos5_usbdrd_phy *phy_drd);
+	int (*phy_init)(struct exynos5_usbdrd_phy *phy_drd);
 	unsigned int (*set_refclk)(struct phy_usb_instance *inst);
 };
 
@@ -708,7 +708,7 @@ exynos5_usbdrd_apply_phy_tunes(struct exynos5_usbdrd_phy *phy_drd,
 	}
 }
 
-static void exynos5_usbdrd_pipe3_init(struct exynos5_usbdrd_phy *phy_drd)
+static int exynos5_usbdrd_pipe3_init(struct exynos5_usbdrd_phy *phy_drd)
 {
 	u32 reg;
 
@@ -721,6 +721,8 @@ static void exynos5_usbdrd_pipe3_init(struct exynos5_usbdrd_phy *phy_drd)
 	reg = readl(phy_drd->reg_phy + EXYNOS5_DRD_PHYTEST);
 	reg &= ~PHYTEST_POWERDOWN_SSP;
 	writel(reg, phy_drd->reg_phy + EXYNOS5_DRD_PHYTEST);
+
+	return 0;
 }
 
 static void
@@ -831,7 +833,7 @@ exynos5_usbdrd_usbdp_g2_v4_pma_check_pll_lock(struct exynos5_usbdrd_phy *phy_drd
 	return err;
 }
 
-static void
+static int
 exynos5_usbdrd_usbdp_g2_v4_pma_check_cdr_lock(struct exynos5_usbdrd_phy *phy_drd)
 {
 	static const unsigned int timeout_us = 40000;
@@ -857,9 +859,11 @@ exynos5_usbdrd_usbdp_g2_v4_pma_check_cdr_lock(struct exynos5_usbdrd_phy *phy_drd
 			((phy_drd->orientation == TYPEC_ORIENTATION_NORMAL)
 			 ? 0
 			 : 2), reg);
+
+	return err;
 }
 
-static void exynos5_usbdrd_utmi_init(struct exynos5_usbdrd_phy *phy_drd)
+static int exynos5_usbdrd_utmi_init(struct exynos5_usbdrd_phy *phy_drd)
 {
 	u32 reg;
 
@@ -881,6 +885,8 @@ static void exynos5_usbdrd_utmi_init(struct exynos5_usbdrd_phy *phy_drd)
 	reg = readl(phy_drd->reg_phy + EXYNOS5_DRD_PHYTEST);
 	reg &= ~PHYTEST_POWERDOWN_HSP;
 	writel(reg, phy_drd->reg_phy + EXYNOS5_DRD_PHYTEST);
+
+	return 0;
 }
 
 static int exynos5_usbdrd_phy_init(struct phy *phy)
@@ -917,7 +923,9 @@ static int exynos5_usbdrd_phy_init(struct phy *phy)
 	writel(reg, phy_drd->reg_phy + EXYNOS5_DRD_PHYUTMICLKSEL);
 
 	/* UTMI or PIPE3 specific init */
-	inst->phy_cfg->phy_init(phy_drd);
+	ret = inst->phy_cfg->phy_init(phy_drd);
+	if (ret)
+		goto disable_clks;
 
 	/* reference clock settings */
 	reg = inst->phy_cfg->set_refclk(inst);
@@ -940,9 +948,10 @@ static int exynos5_usbdrd_phy_init(struct phy *phy)
 	reg &= ~PHYCLKRST_PORTRESET;
 	writel(reg, phy_drd->reg_phy + EXYNOS5_DRD_PHYCLKRST);
 
+disable_clks:
 	clk_bulk_disable_unprepare(phy_drd->drv_data->n_clks, phy_drd->clks);
 
-	return 0;
+	return ret;
 }
 
 static int exynos5_usbdrd_phy_exit(struct phy *phy)
@@ -1203,7 +1212,7 @@ static void exynos7870_usbdrd_phy_isol(struct phy_usb_instance *inst,
 			   EXYNOS7870_USB2PHY_ENABLE, val);
 }
 
-static void exynos7870_usbdrd_utmi_init(struct exynos5_usbdrd_phy *phy_drd)
+static int exynos7870_usbdrd_utmi_init(struct exynos5_usbdrd_phy *phy_drd)
 {
 	u32 reg;
 
@@ -1291,6 +1300,8 @@ static void exynos7870_usbdrd_utmi_init(struct exynos5_usbdrd_phy *phy_drd)
 	if (phy_drd->drv_data->phy_tunes)
 		exynos5_usbdrd_apply_phy_tunes(phy_drd,
 					       PTS_UTMI_POSTINIT);
+
+	return 0;
 }
 
 static int exynos7870_usbdrd_phy_init(struct phy *phy)
@@ -1304,11 +1315,11 @@ static int exynos7870_usbdrd_phy_init(struct phy *phy)
 		return ret;
 
 	/* UTMI or PIPE3 specific init */
-	inst->phy_cfg->phy_init(phy_drd);
+	ret = inst->phy_cfg->phy_init(phy_drd);
 
 	clk_bulk_disable_unprepare(phy_drd->drv_data->n_clks, phy_drd->clks);
 
-	return 0;
+	return ret;
 }
 
 static int exynos7870_usbdrd_phy_exit(struct phy *phy)
@@ -1355,10 +1366,12 @@ static const struct phy_ops exynos7870_usbdrd_phy_ops = {
 	.owner		= THIS_MODULE,
 };
 
-static void exynos2200_usbdrd_utmi_init(struct exynos5_usbdrd_phy *phy_drd)
+static int exynos2200_usbdrd_utmi_init(struct exynos5_usbdrd_phy *phy_drd)
 {
 	/* Configure non-Samsung IP PHY, responsible for UTMI */
-	phy_init(phy_drd->hs_phy);
+	phy_init(phy_drd->hs_phy);
+
+	return 0;
 }
 
 static void exynos2200_usbdrd_link_init(struct exynos5_usbdrd_phy *phy_drd)
@@ -1458,11 +1469,11 @@ static int exynos2200_usbdrd_phy_init(struct phy *phy)
 	exynos2200_usbdrd_link_attach_detach_pipe3_phy(inst);
 
 	/* UTMI or PIPE3 specific init */
-	inst->phy_cfg->phy_init(phy_drd);
+	ret = inst->phy_cfg->phy_init(phy_drd);
 
 	clk_bulk_disable_unprepare(phy_drd->drv_data->n_clks, phy_drd->clks);
 
-	return 0;
+	return ret;
 }
 
 static int exynos2200_usbdrd_phy_exit(struct phy *phy)
@@ -1516,7 +1538,7 @@ exynos5_usbdrd_usb_v3p1_pipe_override(struct exynos5_usbdrd_phy *phy_drd)
 	writel(reg, regs_base + EXYNOS850_DRD_SECPMACTL);
 }
 
-static void exynos850_usbdrd_utmi_init(struct exynos5_usbdrd_phy *phy_drd)
+static int exynos850_usbdrd_utmi_init(struct exynos5_usbdrd_phy *phy_drd)
 {
 	void __iomem *regs_base = phy_drd->reg_phy;
 	u32 reg;
@@ -1622,6 +1644,8 @@ static void exynos850_usbdrd_utmi_init(struct exynos5_usbdrd_phy *phy_drd)
 
 	if (ss_ports)
 		exynos5_usbdrd_usb_v3p1_pipe_override(phy_drd);
+
+	return 0;
 }
 
 static int exynos850_usbdrd_phy_init(struct phy *phy)
@@ -1635,12 +1659,13 @@ static int exynos850_usbdrd_phy_init(struct phy *phy)
 		return ret;
 
 	/* UTMI or PIPE3 specific init */
+	ret = 0;
 	scoped_guard(mutex, &phy_drd->phy_mutex)
-		inst->phy_cfg->phy_init(phy_drd);
+		ret = inst->phy_cfg->phy_init(phy_drd);
 
 	clk_bulk_disable_unprepare(phy_drd->drv_data->n_clks, phy_drd->clks);
 
-	return 0;
+	return ret;
 }
 
 static int exynos850_usbdrd_phy_exit(struct phy *phy)
@@ -1689,11 +1714,12 @@ static const struct phy_ops exynos850_usbdrd_phy_ops = {
 	.owner		= THIS_MODULE,
 };
 
-static void exynos5_usbdrd_gs101_pipe3_init(struct exynos5_usbdrd_phy *phy_drd)
+static int exynos5_usbdrd_gs101_pipe3_init(struct exynos5_usbdrd_phy *phy_drd)
 {
 	void __iomem *regs_pma = phy_drd->reg_pma;
 	void __iomem *regs_phy = phy_drd->reg_phy;
 	u32 reg;
+	int ret;
 
 	exynos5_usbdrd_usbdp_g2_v4_ctrl_pma_ready(phy_drd);
 
@@ -1715,8 +1741,11 @@ static void exynos5_usbdrd_gs101_pipe3_init(struct exynos5_usbdrd_phy *phy_drd)
 		 SECPMACTL_PMA_INIT_SW_RST);
 	writel(reg, regs_phy + EXYNOS850_DRD_SECPMACTL);
 
-	if (!exynos5_usbdrd_usbdp_g2_v4_pma_check_pll_lock(phy_drd))
-		exynos5_usbdrd_usbdp_g2_v4_pma_check_cdr_lock(phy_drd);
+	ret = exynos5_usbdrd_usbdp_g2_v4_pma_check_pll_lock(phy_drd);
+	if (ret)
+		return ret;
+
+	return exynos5_usbdrd_usbdp_g2_v4_pma_check_cdr_lock(phy_drd);
 }
 
 static int exynos5_usbdrd_gs101_phy_init(struct phy *phy)
@@ -1741,7 +1770,15 @@ static int exynos5_usbdrd_gs101_phy_init(struct phy *phy)
 	 */
 	exynos5_usbdrd_phy_isol(inst, false);
 
-	return exynos850_usbdrd_phy_init(phy);
+	ret = exynos850_usbdrd_phy_init(phy);
+	if (ret) {
+		exynos5_usbdrd_phy_isol(inst, true);
+		if (inst->phy_cfg->id == EXYNOS5_DRDPHY_UTMI)
+			regulator_bulk_disable(phy_drd->drv_data->n_regulators,
+					       phy_drd->regulators);
+	}
+
+	return ret;
 }
 
 static int exynos5_usbdrd_gs101_phy_exit(struct phy *phy)
@@ -2277,7 +2314,7 @@ exynosautov920_usb31drd_lane0_reset(struct exynos5_usbdrd_phy *phy_drd, int val)
 	writel(reg, reg_phy + EXYNOSAUTOV920_USB31DRD_PHY_RST_CTRL);
 }
 
-static void
+static int
 exynosautov920_usb31drd_pipe3_init(struct exynos5_usbdrd_phy *phy_drd)
 {
 	void __iomem *reg_phy = phy_drd->reg_phy;
@@ -2335,6 +2372,8 @@ exynosautov920_usb31drd_pipe3_init(struct exynos5_usbdrd_phy *phy_drd)
 
 	/* override values for level settings */
 	exynosautov920_usb31drd_cr_write(phy_drd, 0x22, 0x00F5);
+
+	return 0;
 }
 
 static void
@@ -2353,7 +2392,7 @@ exynosautov920_usb31drd_ssphy_disable(struct exynos5_usbdrd_phy *phy_drd)
 	writel(reg, reg_phy + EXYNOSAUTOV920_USB31DRD_PHY_CONFIG7);
 }
 
-static void
+static int
 exynosautov920_usbdrd_utmi_init(struct exynos5_usbdrd_phy *phy_drd)
 {
 	void __iomem *reg_phy = phy_drd->reg_phy;
@@ -2457,6 +2496,8 @@ exynosautov920_usbdrd_utmi_init(struct exynos5_usbdrd_phy *phy_drd)
 	reg = readl(reg_phy + EXYNOS2200_DRD_CLKRST);
 	reg |= EXYNOS2200_CLKRST_LINK_PCLK_SEL;
 	writel(reg, reg_phy + EXYNOS2200_DRD_CLKRST);
+
+	return 0;
 }
 
 static void
@@ -2504,11 +2545,11 @@ static int exynosautov920_usbdrd_phy_init(struct phy *phy)
 	inst->phy_cfg->phy_isol(inst, false);
 
 	/* UTMI or PIPE3 specific init */
-	inst->phy_cfg->phy_init(phy_drd);
+	ret = inst->phy_cfg->phy_init(phy_drd);
 
 	clk_bulk_disable_unprepare(phy_drd->drv_data->n_clks, phy_drd->clks);
 
-	return 0;
+	return ret;
 }
 
 static int exynosautov920_usbdrd_phy_exit(struct phy *phy)
-- 
2.50.1 (Apple Git-155)


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

^ permalink raw reply related	[flat|nested] 2+ messages in thread
* Re: [PATCH] phy: samsung: usbdrd: propagate GS101 PIPE3 lock failures
  2026-06-23 13:53 [PATCH] phy: samsung: usbdrd: propagate GS101 PIPE3 lock failures Pengpeng Hou
@ 2026-06-23 14:06 ` sashiko-bot
  0 siblings, 0 replies; 2+ messages in thread
From: sashiko-bot @ 2026-06-23 14:06 UTC (permalink / raw)
  To: Pengpeng Hou; +Cc: vkoul, olteanv, linux-phy, neil.armstrong

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Resource leak (regulators and PHY isolation state) in error path of exynos2200_usbdrd_phy_init.
--

commit 20eac936223675450b3e9e458566653299d0b09b
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>
Subject: phy: samsung: usbdrd: propagate GS101 PIPE3 lock failures

This commit updates the SoC-specific phy_init hooks to return an integer
error code instead of void. This allows helpers with real failure results,
such as the GS101 PIPE3 PLL or CDR locks failing, to propagate those errors
through the existing PHY initialization paths.

> diff --git a/drivers/phy/samsung/phy-exynos5-usbdrd.c b/drivers/phy/samsung/phy-exynos5-usbdrd.c
> index 5a181cb4597e9..513a8f9c25e9a 100644
> --- a/drivers/phy/samsung/phy-exynos5-usbdrd.c
> +++ b/drivers/phy/samsung/phy-exynos5-usbdrd.c

[ ... ]

> @@ -1458,11 +1471,11 @@ static int exynos2200_usbdrd_phy_init(struct phy *phy)
>  	exynos2200_usbdrd_link_attach_detach_pipe3_phy(inst);
>  
>  	/* UTMI or PIPE3 specific init */
> -	inst->phy_cfg->phy_init(phy_drd);
> +	ret = inst->phy_cfg->phy_init(phy_drd);
>  
>  	clk_bulk_disable_unprepare(phy_drd->drv_data->n_clks, phy_drd->clks);
>  
> -	return 0;
> +	return ret;
>  }

[Severity: High]
This is a pre-existing issue, but while reviewing the error path updates in
exynos2200_usbdrd_phy_init(), I noticed a potential resource leak earlier
in the same function.

If clk_bulk_prepare_enable() fails, the function returns immediately:

	if (inst->phy_cfg->id == EXYNOS5_DRDPHY_UTMI) {
		ret = regulator_bulk_enable(phy_drd->drv_data->n_regulators,
					    phy_drd->regulators);
		if (ret) {
			...
			return ret;
		}
	}

	exynos5_usbdrd_phy_isol(inst, false);

	ret = clk_bulk_prepare_enable(phy_drd->drv_data->n_clks, phy_drd->clks);
	if (ret)
		return ret;

Does this skip tearing down the regulators (enabled for EXYNOS5_DRDPHY_UTMI)
and leave the PHY un-isolated, considering the generic PHY framework does
not call .exit() when .init() fails?

Would it be appropriate to add an error label here to disable the regulators
and re-assert isolation via exynos5_usbdrd_phy_isol(inst, true), similar to
the cleanup path recently added for exynos5_usbdrd_gs101_phy_init()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623135349.42739-1-pengpeng@iscas.ac.cn?part=1

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

^ permalink raw reply	[flat|nested] 2+ messages in thread
end of thread, other threads:[~2026-06-23 14:06 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-06-23 13:53 [PATCH] phy: samsung: usbdrd: propagate GS101 PIPE3 lock failures Pengpeng Hou
2026-06-23 14:06 ` sashiko-bot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox