Re: [PATCH v12 26/36] phy: rockchip: usbdp: Avoid xHCI SErrors

[sashiko-bot@kernel.org]

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

New issues:
- [High] Clock and power leak in `rk_udphy_usb3_phy_exit` when Type-C mode drops USB support.
- [High] The patch unconditionally re-enables the SuperSpeed U3 port during PHY initialization and re-initialization, ignoring the high-speed restriction (`udphy->hs`).

Pre-existing issues:
- [High] When a Type-C device is unplugged, the PHY re-initialization is flagged but never executed, leaving the USB3 port physically disabled and breaking subsequent USB3 connections.
--

--- Patch [26]: [PATCH v12 26/36] phy: rockchip: usbdp: Avoid xHCI SErrors ---
commit 66857e87fceaec79fad3845d46dbfd48c9340977
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

phy: rockchip: usbdp: Avoid xHCI SErrors

This commit addresses an SError caused by the dwc3 driver accessing xHCI
registers while the USBDP PHY is temporarily disabled during a mode switch.
It resolves this by disabling the USB3 port prior to re-initialization, which
safely re-routes the PIPE clock to the UTMI fallback clock.

> diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> index edee27933d890..ff768e5b43ca0 100644
> --- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
> +++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> @@ -1000,12 +1000,15 @@ static int rk_udphy_power_on(struct rk_udphy *udphy, u8 mode)
>  			rk_udphy_u3_port_disable(udphy, false);
>  		udphy->phy_needs_reinit = false;
>  	} else if (udphy->phy_needs_reinit) {
> -		if (udphy->mode == UDPHY_MODE_DP)
> -			rk_udphy_u3_port_disable(udphy, true);
> +		rk_udphy_u3_port_disable(udphy, true);
> +		udelay(10);
>  
>  		ret = rk_udphy_init(udphy);
>  		if (ret)
>  			return ret;
> +
> +		if (udphy->mode & UDPHY_MODE_USB)
> +			rk_udphy_u3_port_disable(udphy, false);

[Severity: High]
Does this code unconditionally re-enable the SuperSpeed U3 port purely based on
the mode?

During rk_udphy_usb3_phy_init(), the U3 port is kept disabled if udphy->hs is
true. However, when a mode switch or DP power-on triggers a PHY
re-initialization in rk_udphy_power_on(), this new logic executes without
checking !udphy->hs.

Could this result in misconfiguring the hardware by enabling the SuperSpeed U3
port on boards that explicitly restricted the PHY to high-speed
(maximum-speed = "high-speed")?

>  		udphy->phy_needs_reinit = false;

[Severity: High]
This is a pre-existing issue, but does the phy_needs_reinit state machine get
stuck when a Type-C device is unplugged?

If a DP-only cable is unplugged after a standard USB3 cable was active,
typec_mux_set() reverts to UDPHY_MODE_DP_USB and flags
phy_needs_reinit = true. The DP framework calls rk_udphy_dp_phy_power_off(),
which clears the DP bit from udphy->status:

drivers/phy/rockchip/phy-rockchip-usbdp.c:rk_udphy_dp_phy_power_off() {
    rk_udphy_dp_lane_enable(udphy, 0);
    rk_udphy_power_off(udphy, UDPHY_MODE_DP);
}

Because the USB3 controller never suspended, udphy->status remains non-zero
(set to UDPHY_MODE_USB), so the PHY isn't disabled. Crucially, no driver calls
rk_udphy_power_on() to process the pending phy_needs_reinit state.

Will this strand the active USB3 PHY in a disabled physical state, breaking
subsequent USB3 connections?

>  	}

[Severity: High]
Regarding the related commit in this series that clears USB status on PHY exit,
is there a clock and power leak in rk_udphy_usb3_phy_exit() when the Type-C
mode drops USB support?

In rk_udphy_usb3_phy_exit():

	if (!(udphy->mode & UDPHY_MODE_USB) || udphy->hs) {
		udphy->status &= ~UDPHY_MODE_USB;
		return 0;
	}

When the Type-C mode dynamically changes to a state without USB support (e.g.,
a DP-only cable is connected), udphy->mode becomes UDPHY_MODE_DP. If the USB
controller then suspends and calls phy_exit(), rk_udphy_usb3_phy_exit()
executes this early return.

This clears the UDPHY_MODE_USB bit from udphy->status, but misses checking if
udphy->status == UDPHY_MODE_NONE. Could this completely fail to call
rk_udphy_disable(), wasting power and potentially blocking system suspend?

Additionally, rk_udphy_power_off() inappropriately refuses to turn off active
hardware features if the negotiated Type-C udphy->mode has already shifted
away from the requested mode:

drivers/phy/rockchip/phy-rockchip-usbdp.c:rk_udphy_power_off() {
	if (!(udphy->mode & mode)) {
		dev_info(udphy->dev, "mode 0x%02x is not support\n", mode);
		return;
	}
    ...
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710-rockchip-usbdp-cleanup-v12-0-8b41a9a9bef0@collabora.com?part=26