[PATCH v3 0/4] phy: rockchip: inno-usb2: ensure PHY is running when clock is requested

[PATCH v3 0/4] phy: rockchip: inno-usb2: ensure PHY is running when clock is requested

[Sebastian Reichel]

This was noticed on RK3588 EVB1 when resuming from system suspend. This
is technically a fix, but its unclear when the bug was introduced and
system suspend is broken on RK3588 for quite a while and not just due
to this problem. So I think this fix can be merged the normal way via
linux-next.

Changes in v3:
- Link to v2: https://patch.msgid.link/20261001-phy-rockchip-inno-usb2-clock-fix-v2-0-e01bf509359e@collabora.com
- Fix do_suspend negation resulting in SError on RK3576 (Sashiko, CI testing)
- Ensure PHY ports array is initialized in clock enable path (Sashiko)
- Cleanup properly in clock enable error path (Sashiko)

Changes in v2:
- Link to v1: https://patch.msgid.link/20260908-phy-rockchip-inno-usb2-clock-fix-v1-1-f7d59c31b908@collabora.com
- Move suspend exit code from rockchip_usb2phy_power_on into a
  separate patch and reuse it in the clock prepare function;
  move is done in a separate patch
- Updated commit messages with proper rationale
- Added one more patch dropping useless assignment
  of rport->suspended directly after calling
  rockchip_usb2phy_power_on/off
- Add one more patch unifying and simplifying the debug
  messages for phy init/power_on/power_off/exit

---
To: Vinod Koul <vkoul@kernel.org>
To: Neil Armstrong <neil.armstrong@linaro.org>
To: Manivannan Sadhasivam <mani@kernel.org>
To: Heiko Stuebner <heiko@sntech.de>
Cc: linux-phy@lists.infradead.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-rockchip@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Cc: Igor Paunovic <royalnet026@gmail.com>
Cc: kernel@collabora.com
Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>

---
Sebastian Reichel (4):
      phy: rockchip: inno-usb2: simplify and unify PHY op debug messages
      phy: rockchip: inno-usb2: drop duplicated update of rport->suspended
      phy: rockchip: inno-usb2: move suspend handling into new function
      phy: rockchip: inno-usb2: move suspend handling into clock prepare/unprepare for RK3588/RK3576

 drivers/phy/rockchip/phy-rockchip-inno-usb2.c | 92 +++++++++++++++++++--------
 1 file changed, 66 insertions(+), 26 deletions(-)
---
base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
change-id: 20260908-phy-rockchip-inno-usb2-clock-fix-edd65b57f884

Best regards,
--  
Sebastian Reichel <sebastian.reichel@collabora.com>


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

[PATCH v3 1/4] phy: rockchip: inno-usb2: simplify and unify PHY op debug messages

[Sebastian Reichel]

Add debug message for init/exit in addition to power_on/power_off and
avoid the useless extra dereference in the existing power_on/power_off
to make the prints consistent.

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 drivers/phy/rockchip/phy-rockchip-inno-usb2.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
index e47b89015586..0b61f152eb3f 100644
--- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
+++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
@@ -508,6 +508,8 @@ static int rockchip_usb2phy_init(struct phy *phy)
 	struct rockchip_usb2phy *rphy = dev_get_drvdata(phy->dev.parent);
 	int ret = 0;
 
+	dev_dbg(&phy->dev, "phy port init\n");
+
 	mutex_lock(&rport->mutex);
 
 	if (rport->port_id == USB2PHY_PORT_OTG) {
@@ -592,7 +594,7 @@ static int rockchip_usb2phy_power_on(struct phy *phy)
 	struct rockchip_usb2phy *rphy = dev_get_drvdata(phy->dev.parent);
 	int ret;
 
-	dev_dbg(&rport->phy->dev, "port power on\n");
+	dev_dbg(&phy->dev, "port power on\n");
 
 	if (!rport->suspended)
 		return 0;
@@ -632,7 +634,7 @@ static int rockchip_usb2phy_power_off(struct phy *phy)
 	struct rockchip_usb2phy *rphy = dev_get_drvdata(phy->dev.parent);
 	int ret;
 
-	dev_dbg(&rport->phy->dev, "port power off\n");
+	dev_dbg(&phy->dev, "port power off\n");
 
 	if (rport->suspended)
 		return 0;
@@ -651,6 +653,8 @@ static int rockchip_usb2phy_exit(struct phy *phy)
 {
 	struct rockchip_usb2phy_port *rport = phy_get_drvdata(phy);
 
+	dev_dbg(&phy->dev, "phy port exit\n");
+
 	if (rport->port_id == USB2PHY_PORT_OTG &&
 	    rport->mode != USB_DR_MODE_HOST &&
 	    rport->mode != USB_DR_MODE_UNKNOWN) {

-- 
2.53.0


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

[PATCH v3 2/4] phy: rockchip: inno-usb2: drop duplicated update of rport->suspended

[Sebastian Reichel]

rockchip_usb2phy_power_on and rockchip_usb2phy_power_off already update
rport->suspended in their success path. In case of error it's not
sensible to update rport->suspended. So drop the duplicated update.

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 drivers/phy/rockchip/phy-rockchip-inno-usb2.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
index 0b61f152eb3f..51afbcc40c4e 100644
--- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
+++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
@@ -994,7 +994,6 @@ static void rockchip_usb2phy_sm_work(struct work_struct *work)
 		if (rport->suspended) {
 			dev_dbg(&rport->phy->dev, "Connected\n");
 			rockchip_usb2phy_power_on(rport->phy);
-			rport->suspended = false;
 		} else {
 			/* D+ line pull-up, D- line pull-down */
 			dev_dbg(&rport->phy->dev, "FS/LS online\n");
@@ -1004,7 +1003,6 @@ static void rockchip_usb2phy_sm_work(struct work_struct *work)
 		if (!rport->suspended) {
 			dev_dbg(&rport->phy->dev, "Disconnected\n");
 			rockchip_usb2phy_power_off(rport->phy);
-			rport->suspended = true;
 		}
 
 		/*

-- 
2.53.0


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

[PATCH v3 3/4] phy: rockchip: inno-usb2: move suspend handling into new function

[Sebastian Reichel]

Move handling of the PHY suspend handling into its own dedicated
function. No functional changes intended.

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 drivers/phy/rockchip/phy-rockchip-inno-usb2.c | 48 +++++++++++++++++----------
 1 file changed, 31 insertions(+), 17 deletions(-)

diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
index 51afbcc40c4e..2919c2e11968 100644
--- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
+++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
@@ -332,6 +332,35 @@ rockchip_usb2phy_clk480m_clkout_ctl(struct clk_hw *hw, struct regmap **base,
 	}
 }
 
+static int rockchip_usb2phy_set_suspend(struct rockchip_usb2phy *rphy,
+					struct rockchip_usb2phy_port *rport,
+					bool do_suspend)
+{
+	int ret;
+
+	ret = property_enable(rphy->grf, &rport->port_cfg->phy_sus, do_suspend);
+	if (ret)
+		return ret;
+
+	if (!do_suspend) {
+		/*
+		 * For rk3588, it needs to reset phy when exit from suspend
+		 * mode with common_on_n 1'b1(aka REFCLK_LOGIC, Bias, and PLL
+		 * blocks are powered down) for lower power consumption. If you
+		 * don't want to reset phy, please keep the common_on_n 1'b0 to
+		 * set these blocks remain powered.
+		 */
+		ret = rockchip_usb2phy_reset(rphy);
+		if (ret)
+			return ret;
+
+		/* waiting for the utmi_clk to become stable */
+		usleep_range(1500, 2000);
+	}
+
+	return 0;
+}
+
 static int rockchip_usb2phy_clk480m_prepare(struct clk_hw *hw)
 {
 	const struct usb2phy_reg *clkout_ctl;
@@ -603,27 +632,12 @@ static int rockchip_usb2phy_power_on(struct phy *phy)
 	if (ret)
 		return ret;
 
-	ret = property_enable(rphy->grf, &rport->port_cfg->phy_sus, false);
+	ret = rockchip_usb2phy_set_suspend(rphy, rport, false);
 	if (ret) {
 		clk_disable_unprepare(rphy->clk480m);
 		return ret;
 	}
 
-	/*
-	 * For rk3588, it needs to reset phy when exit from
-	 * suspend mode with common_on_n 1'b1(aka REFCLK_LOGIC,
-	 * Bias, and PLL blocks are powered down) for lower
-	 * power consumption. If you don't want to reset phy,
-	 * please keep the common_on_n 1'b0 to set these blocks
-	 * remain powered.
-	 */
-	ret = rockchip_usb2phy_reset(rphy);
-	if (ret)
-		return ret;
-
-	/* waiting for the utmi_clk to become stable */
-	usleep_range(1500, 2000);
-
 	rport->suspended = false;
 	return 0;
 }
@@ -639,7 +653,7 @@ static int rockchip_usb2phy_power_off(struct phy *phy)
 	if (rport->suspended)
 		return 0;
 
-	ret = property_enable(rphy->grf, &rport->port_cfg->phy_sus, true);
+	ret = rockchip_usb2phy_set_suspend(rphy, rport, true);
 	if (ret)
 		return ret;
 

-- 
2.53.0


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

[PATCH v3 4/4] phy: rockchip: inno-usb2: move suspend handling into clock prepare/unprepare for RK3588/RK3576

[Sebastian Reichel]

On RK3588 the 480MHz PHY clock must be running to access registers on
the OHCI and EHCI controllers. This requires that the clock output bit
is configured correctly (already happening) and that the PHY PLL itself
is running. The PHY PLL is only running when the PHY is not suspended.
This is currently handled independently of the clock and thus the clock
might be enabled with the PHY being suspended resulting in a
non-functional clock despite the clock being marked as prepared and
enabled according to the common clock framework.

This is especially a problem with OHCI system resume on RK3588, which
does:

ohci_platform_resume
    -> ohci_platform_resume_common
        -> deassert resets
        -> ohci_platform_power_on -> enable clocks
        -> ohci_resume
            -> ohci_readl(ohci, &ohci->regs->control); // SError !
            -> ...
        -> ...
    -> root hub resume (this resumes the PHY)

Fix this by fully powering the PHY from the clock prepare function.

This does not work on older platforms, which have multiple ports and
only one 480MHz clock as we do not know which port should be resumed.
But as far as I can tell these platforms do not have the clock
dependency from their USB controllers and thus are not affected.

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 drivers/phy/rockchip/phy-rockchip-inno-usb2.c | 38 ++++++++++++++++++++++-----
 1 file changed, 31 insertions(+), 7 deletions(-)

diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
index 2919c2e11968..b2633b83f7ab 100644
--- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
+++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
@@ -363,10 +363,15 @@ static int rockchip_usb2phy_set_suspend(struct rockchip_usb2phy *rphy,
 
 static int rockchip_usb2phy_clk480m_prepare(struct clk_hw *hw)
 {
+	struct rockchip_usb2phy *rphy =
+		container_of(hw, struct rockchip_usb2phy, clk480m_hw);
 	const struct usb2phy_reg *clkout_ctl;
 	struct regmap *base;
 	int ret;
 
+	if (rphy->phy_cfg->num_ports == 1 && !rphy->ports[0].port_cfg)
+		return -EAGAIN;
+
 	rockchip_usb2phy_clk480m_clkout_ctl(hw, &base, &clkout_ctl);
 
 	/* turn on 480m clk output if it is off */
@@ -379,16 +384,31 @@ static int rockchip_usb2phy_clk480m_prepare(struct clk_hw *hw)
 		usleep_range(1200, 1300);
 	}
 
+	if (rphy->phy_cfg->num_ports == 1) {
+		ret = rockchip_usb2phy_set_suspend(rphy, &rphy->ports[0], false);
+		if (ret)
+			goto disable_clkout;
+	}
+
 	return 0;
+
+disable_clkout:
+	property_enable(base, clkout_ctl, false);
+	return ret;
 }
 
 static void rockchip_usb2phy_clk480m_unprepare(struct clk_hw *hw)
 {
+	struct rockchip_usb2phy *rphy =
+		container_of(hw, struct rockchip_usb2phy, clk480m_hw);
 	const struct usb2phy_reg *clkout_ctl;
 	struct regmap *base;
 
 	rockchip_usb2phy_clk480m_clkout_ctl(hw, &base, &clkout_ctl);
 
+	if (rphy->phy_cfg->num_ports == 1)
+		rockchip_usb2phy_set_suspend(rphy, &rphy->ports[0], true);
+
 	/* turn off 480m clk output */
 	property_enable(base, clkout_ctl, false);
 }
@@ -632,10 +652,12 @@ static int rockchip_usb2phy_power_on(struct phy *phy)
 	if (ret)
 		return ret;
 
-	ret = rockchip_usb2phy_set_suspend(rphy, rport, false);
-	if (ret) {
-		clk_disable_unprepare(rphy->clk480m);
-		return ret;
+	if (rphy->phy_cfg->num_ports > 1) {
+		ret = rockchip_usb2phy_set_suspend(rphy, rport, false);
+		if (ret) {
+			clk_disable_unprepare(rphy->clk480m);
+			return ret;
+		}
 	}
 
 	rport->suspended = false;
@@ -653,9 +675,11 @@ static int rockchip_usb2phy_power_off(struct phy *phy)
 	if (rport->suspended)
 		return 0;
 
-	ret = rockchip_usb2phy_set_suspend(rphy, rport, true);
-	if (ret)
-		return ret;
+	if (rphy->phy_cfg->num_ports > 1) {
+		ret = rockchip_usb2phy_set_suspend(rphy, rport, true);
+		if (ret)
+			return ret;
+	}
 
 	rport->suspended = true;
 	clk_disable_unprepare(rphy->clk480m);

-- 
2.53.0


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

Re: [PATCH v3 2/4] phy: rockchip: inno-usb2: drop duplicated update of rport->suspended

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] phy: rockchip: inno-usb2: hotplug wedge on rockchip_usb2phy_power_off() error
--

commit c83a3edf6251c7ef6aad3ae18e1a303508f3dab1
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

phy: rockchip: inno-usb2: drop duplicated update of rport->suspended

This commit removes the unconditional assignments to rport->suspended in
rockchip_usb2phy_sm_work() after calling rockchip_usb2phy_power_on() and
rockchip_usb2phy_power_off(), relying on those functions to update
rport->suspended on success.

> diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> index 0b61f152eb3f3..51afbcc40c4eb 100644
> --- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> +++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> @@ -994,7 +994,6 @@ static void rockchip_usb2phy_sm_work(struct work_struct *work)
>  		if (rport->suspended) {
>  			dev_dbg(&rport->phy->dev, "Connected\n");
>  			rockchip_usb2phy_power_on(rport->phy);
> -			rport->suspended = false;
>  		} else {
>  			/* D+ line pull-up, D- line pull-down */
>  			dev_dbg(&rport->phy->dev, "FS/LS online\n");
> @@ -1004,7 +1003,6 @@ static void rockchip_usb2phy_sm_work(struct work_struct *work)
>  		if (!rport->suspended) {
>  			dev_dbg(&rport->phy->dev, "Disconnected\n");
>  			rockchip_usb2phy_power_off(rport->phy);
> -			rport->suspended = true;
>  		}

[Severity: Medium]
As reported by Sashiko on v2 [1], if rockchip_usb2phy_power_off() fails in
rockchip_usb2phy_sm_work() under PHY_STATE_DISCONNECT (when
property_enable() on phy_sus returns an error), rport->suspended now remains
false and rphy->clk480m stays enabled.

However, the PHY_STATE_DISCONNECT path does not check the return value of
rockchip_usb2phy_power_off() and still proceeds as if the port were
suspended, enabling ls_det_clr and ls_det_en and returning early without
rescheduling rport->sm_work:

	case PHY_STATE_DISCONNECT:
		if (!rport->suspended) {
			dev_dbg(&rport->phy->dev, "Disconnected\n");
			rockchip_usb2phy_power_off(rport->phy);
		}

		property_enable(rphy->grf, &rport->port_cfg->ls_det_clr, true);
		property_enable(rphy->grf, &rport->port_cfg->ls_det_en, true);

		mutex_unlock(&rport->mutex);
		return;

When the next linestate interrupt fires, rockchip_usb2phy_linestate_irq()
disables ls_det_en and only restarts rockchip_usb2phy_sm_work() when
rport->suspended is true:

	property_enable(rphy->grf, &rport->port_cfg->ls_det_en, false);
	property_enable(rphy->grf, &rport->port_cfg->ls_det_clr, true);

	mutex_unlock(&rport->mutex);

	if (rport->suspended && rport->port_id == USB2PHY_PORT_HOST)
		rockchip_usb2phy_sm_work(&rport->sm_work.work);

Because rport->suspended is false, would this leave ls_det_en disabled and
rport->sm_work stopped, permanently wedging host port hotplug detection and
leaking the rphy->clk480m enable count? Should rockchip_usb2phy_sm_work()
jump to next_schedule when rockchip_usb2phy_power_off() returns an error?

[1] https://lore.kernel.org/r/20261001-phy-rockchip-inno-usb2-clock-fix-v2-2-e01bf509359e@collabora.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105-phy-rockchip-inno-usb2-clock-fix-v3-0-b20608c08b94@collabora.com?part=2

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

Re: [PATCH v3 4/4] phy: rockchip: inno-usb2: move suspend handling into clock prepare/unprepare for RK3588/RK3576

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] phy: rockchip: inno-usb2: NULL deref in rockchip_usb2phy_clk480m_unprepare()
- [High] phy: rockchip: inno-usb2: -EAGAIN in rockchip_usb2phy_clk480m_prepare()
--

commit 0477bc2cc59adc2dfda01c728633637be8e1f046
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

phy: rockchip: inno-usb2: move suspend handling into clock prepare/unprepare for RK3588/RK3576

This commit moves PHY suspend handling on single-port PHYs (RK3588 and
RK3576) from the PHY power_on and power_off callbacks into the 480MHz clock
prepare and unprepare callbacks so the PHY PLL is running whenever the clock
is prepared.

> diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> index 2919c2e11968b..b2633b83f7ab3 100644
> --- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> +++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> @@ -363,10 +363,15 @@ static int rockchip_usb2phy_set_suspend(struct rockchip_usb2phy *rphy,
>  
>  static int rockchip_usb2phy_clk480m_prepare(struct clk_hw *hw)
>  {
> +	struct rockchip_usb2phy *rphy =
> +		container_of(hw, struct rockchip_usb2phy, clk480m_hw);
>  	const struct usb2phy_reg *clkout_ctl;
>  	struct regmap *base;
>  	int ret;
>  
> +	if (rphy->phy_cfg->num_ports == 1 && !rphy->ports[0].port_cfg)
> +		return -EAGAIN;

[Severity: High]
As reported by Sashiko on v2 [1], in rockchip_usb2phy_probe(), the 480MHz
clock provider is registered via rockchip_usb2phy_clk480m_register() and
of_clk_add_provider() before input clocks are enabled, before phy_tuning()
runs, and before the child port loop initializes rphy->ports[0].port_cfg:

drivers/phy/rockchip/phy-rockchip-inno-usb2.c:rockchip_usb2phy_probe() {
	...
	ret = rockchip_usb2phy_clk480m_register(rphy);
	if (ret)
		return dev_err_probe(dev, ret, "failed to register 480m output clock\n");

	ret = clk_bulk_prepare_enable(rphy->num_clks, rphy->clks);
	if (ret)
		return dev_err_probe(dev, ret, "failed to enable usb2phy clks\n");
	...
	for_each_available_child_of_node(np, child_np) {
		struct rockchip_usb2phy_port *rport = &rphy->ports[index];
		...
}

Clock consumers on RK3588 and RK3576 include ohci-platform and ehci-platform
(both of which set PROBE_PREFER_ASYNCHRONOUS and call pdata->power_on() ->
clk_prepare_enable() in their probe functions before usb_add_hcd() requests
the PHY) as well as phy-rockchip-usbdp (which prepares and enables the utmi
clock in rk_udphy_setup()).

If an asynchronous consumer calls clk_prepare_enable() after
of_clk_add_provider() publishes the clock but before rphy->ports[0].port_cfg
is initialized (or on boards where the parent usb2phy node is enabled to
supply the utmi clock to usbdp_phy while the child port subnode is disabled
in DT), rockchip_usb2phy_clk480m_prepare() will return -EAGAIN.

Neither clk_prepare_enable() nor the callers in ohci_platform_probe(),
ehci_platform_probe(), or rk_udphy_setup() retry on -EAGAIN, and the driver
core only defers probing on -EPROBE_DEFER. Can returning -EAGAIN here cause
consumer probing or PHY setup to fail permanently instead of waiting for port
initialization?

> +
>  	rockchip_usb2phy_clk480m_clkout_ctl(hw, &base, &clkout_ctl);
>  
>  	/* turn on 480m clk output if it is off */
> @@ -379,16 +384,31 @@ static int rockchip_usb2phy_clk480m_prepare(struct clk_hw *hw)
>  		usleep_range(1200, 1300);
>  	}
>  
> +	if (rphy->phy_cfg->num_ports == 1) {
> +		ret = rockchip_usb2phy_set_suspend(rphy, &rphy->ports[0], false);
> +		if (ret)
> +			goto disable_clkout;
> +	}
> +
>  	return 0;
> +
> +disable_clkout:
> +	property_enable(base, clkout_ctl, false);
> +	return ret;
>  }
>  
>  static void rockchip_usb2phy_clk480m_unprepare(struct clk_hw *hw)
>  {
> +	struct rockchip_usb2phy *rphy =
> +		container_of(hw, struct rockchip_usb2phy, clk480m_hw);
>  	const struct usb2phy_reg *clkout_ctl;
>  	struct regmap *base;
>  
>  	rockchip_usb2phy_clk480m_clkout_ctl(hw, &base, &clkout_ctl);
>  
> +	if (rphy->phy_cfg->num_ports == 1)
> +		rockchip_usb2phy_set_suspend(rphy, &rphy->ports[0], true);

[Severity: High]
What happens in rockchip_usb2phy_clk480m_unprepare() if
rphy->ports[0].port_cfg is NULL?

In rk3588-base.dtsi, rk3588-extra.dtsi, and rk3576.dtsi, the parent usb2phy
nodes and their child otg-port or host-port nodes have independent status
properties. If a parent usb2phy node is enabled while its child port node is
disabled in DT, rockchip_usb2phy_probe() registers clk480m with
rphy->ports[0].port_cfg left as NULL.

Because rockchip_usb2phy_clkout_ops defines .is_prepared =
rockchip_usb2phy_clk480m_prepared(), which checks property_enabled(base,
clkout_ctl), and clkout_ctl defaults to enabled in hardware at reset on
RK3588 and RK3576, clk_unprepare_unused_subtree() will see the clock as
prepared and call rockchip_usb2phy_clk480m_unprepare() during
clk_disable_unused() even if rockchip_usb2phy_clk480m_prepare() was never
called or returned -EAGAIN.

Unlike rockchip_usb2phy_clk480m_prepare(), rockchip_usb2phy_clk480m_unprepare()
does not check rphy->ports[0].port_cfg before calling
rockchip_usb2phy_set_suspend(rphy, &rphy->ports[0], true), which then
dereferences &rport->port_cfg->phy_sus:

rockchip_usb2phy_clk480m_unprepare()
  -> rockchip_usb2phy_set_suspend()
       -> property_enable(rphy->grf, &rport->port_cfg->phy_sus, do_suspend)

Can this cause a NULL pointer dereference when unpreparing unused clocks at
boot?

[1] https://lore.kernel.org/r/20261001-phy-rockchip-inno-usb2-clock-fix-v2-4-e01bf509359e@collabora.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105-phy-rockchip-inno-usb2-clock-fix-v3-0-b20608c08b94@collabora.com?part=4

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